-
-
Notifications
You must be signed in to change notification settings - Fork 208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(permission-controller): Add requestPermissionsIncremental()
and caveat merger functions
#4222
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds `requestPermissionsIncremental()` to the permission controller. Tests are still to-do.
Replaces the shared(!) mocks used for permission controller side-effect testing with freshly instantiated mocks for each test case. The mess of boilerplate created in the wake of this change we will have to clean up in the future.
Adds initial test cases for `requestPermissionsIncremental()` by duplicating the existing test cases for `requestPermissions()` using `describe.each()` and a new utility function.
One of the `subjectTypes` test cases was commented because Jest encountered a type error when attempting to run it. This test case is now restored by re-implementing the messenger action calling spy, and relaxing the expectations thereon. (The exact number and order of action calls differ between incremental and non-incremental permission requests.)
requestPermissionsIncremental()
requestPermissionsIncremental()
and caveat merger functions
7 tasks
hmalik88
reviewed
Apr 30, 2024
hmalik88
reviewed
Apr 30, 2024
hmalik88
previously approved these changes
May 1, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
7 tasks
FrederikBolding
approved these changes
May 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note
The diff is not as bad as it seems; just hide whitespace on the diff view.
Actual changes are about
+700
of source code and+1000
of tests.Commits are individually reviewable.
Explanation
Adds a new method
PermissionController.requestPermissionsIncremental()
, which allows callers to incrementally request more authority for a subject without erasing any existing permissions or caveats.requestPermissions()
can, based on thepreserveExistingPermissions
flag, already preserve any existing permissions not named in the request, but will always erase existing caveats.requestPermissionsIncremental()
, on the other hand, will merge the requested permissions and their caveats with the existing permissions and their caveats, if any. It will also return the diff of the resulting changes, intended for consumption in the UI.Permissions are merged in the fashion of a right-biased union, where requested permissions are the right-hand side. This operation is like a union in set theory, except the right-hand operand overwrites values of the left-hand operand in case of collisions. At present, it is impossible to define a generic implementation of caveat merging. Therefore, caveat specifications now include an optional property
merger
, where consumers can specify the function that will merge caveats together during incremental permission requests. This function must also implement a right-biased union, returning the new caveat object and the diff of the caveat values expressed in the caveat's value type.See
ARCHITECTURE.md
for a specification of the right-biased operation.Notably,
requestPermissionsIncremental()
will fail if it needs to merge a caveat for which no merger function is specified. If we are to expose incremental permission requests to third parties, every caveat will require a merger. The intention is therefore for all caveats to specify merger functions, and then make themerger
property required in the caveat specification.Finally, this PR commits us to the following principle in caveat design:
References
Closes: #4163
Changelog
@metamask/permission-controller
requestIncrementalPermissions()
merger
, to caveat specifications, enabling consumers to describe how their caveats should be merged during incremental permission requests.Checklist