-
-
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
Add caveat merging and "incremental" permission requests #4163
Labels
Comments
rekmarks
added a commit
that referenced
this issue
May 22, 2024
…d caveat merger functions (#4222) ## 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 the `preserveExistingPermissions` 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 the `merger` property required in the caveat specification. Finally, this PR commits us to the following principle in caveat design: > The existence of an authority **must** be represented by the **presence** of a value. For an elaboration and justification for this principle, again see `ARCHITECTURE.md`. ## References Closes: #4163 ## Changelog ### `@metamask/permission-controller` - **ADDED**: `requestIncrementalPermissions()` - Adds a method for incrementally requesting permissions, in the fashion of a right-biased union, where the requested permissions are the right-hand side. - This enables callers to request additional authority down to the individual caveat, without disturbing other permissions or caveats. In addition, the method returns the diff of the resulting changes, which can be used by future user confirmations for this method. - This method will fail if it attempts to merge caveats for which no caveat merger has been specified. - **ADDED**: Consumer-specified caveat merger functions - Adds a new property, `merger`, to caveat specifications, enabling consumers to describe how their caveats should be merged during incremental permission requests. ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Note
The source of truth for how caveat merging should work has moved to #4222.
Due to the introduction of "dynamic" (i.e. "requested / modified at runtime") permissions in Snaps, we need to introduce a notion of "merging" to the
PermissionController
's caveat abstraction. Let's consider a simplified motivating use case:A subject has a caveat with a value of
{ foo: 'bar' }
. At runtime, the subject requests to change the caveat to a value of{ foo: 'baz' }
. Since this equates to a change in the subject's authority, we must display a confirmation to the user. Currently, we have two options for how to handle it:PermissionController:updateCaveat
to update the caveat directly.PermissionController:revokePermissions
followed byPermissionController:grantPermissions
in order to replace the existing permission entirely.Unfortunately, neither of these options work:
1
is currently the only option afforded by our API—viawallet_requestPermissions
—which will overwrite all existing permissions of the requesting subject and replace them with the new ones. This is a non-starter for the dynamic permissions use case, because we need to represent an incremental change in authority, rather than a complete rewrite of the subject's authority. It also places the burden of merging existing authorities on the caller, which is not their job.2.i
is what we do foreth_accounts
today, while we employ2.ii
during e.g. snap updates when their permissions differ, but we do not expose this capability via our API. Moreover, the permissions diff is calculated manually in an ad hoc fashion, which is a brittle approach.What we want is to let the subject call something like
wallet_requestIncrementalPermissions
, enabling consumers to make incremental permission requests without worrying about how to calculate the diff. MetaMask's job is to calculate the diff and present a confirmation to the user explaining what the change in authority is. Thus, in total, "caveat merging" consists of these steps:PermissionController
stateIf we are to ship something like the proposed RPC method, we should establish the pattern for how to do caveat merging in our codebase. We could decide to do caveat merging "manually" for each permission and just make use of
updateCaveat
, but ideally merge operations should receive the same level of scrutiny as the implementation of a caveat or permission specification. This is an argument for associating the merge operation with the caveat specifications themselves, perhaps by means of a property likemerger?: CaveatMerger<unknown>
.Appendix
Here follows a draft specification of
wallet_requestIncrementalPermissions
in set theory notation:A
B
, whereA ∩ B = ∅
(i.e.A
andB
are disjoint)A ∩ B ≠ ∅
, any permissions in the setA ∩ B
are preserved unmodified inC
.B = A
, the request is a no-op and returnsA
.C
, whereC = A ∪ B
andC ⊇ B
The text was updated successfully, but these errors were encountered: