Skip to content
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

Closed
rekmarks opened this issue Apr 15, 2024 · 0 comments · Fixed by #4222
Closed

Add caveat merging and "incremental" permission requests #4163

rekmarks opened this issue Apr 15, 2024 · 0 comments · Fixed by #4222
Assignees
Labels
enhancement New feature or request PermissionController Related to the PermissionController.

Comments

@rekmarks
Copy link
Member

rekmarks commented Apr 15, 2024

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:

  1. Overwrite the existing permission (i.e. treat it as a brand new request for the permission)
  2. Manually handle the merge in the client, and then either:
    1. call PermissionController:updateCaveat to update the caveat directly.
    2. or, call PermissionController:revokePermissions followed by PermissionController: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—via wallet_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 for eth_accounts today, while we employ 2.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:

  1. Calculating the permission diff
  2. Presenting the user with a confirmation that explains the diff
  3. Applying the diff to PermissionController state

If 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 like merger?: CaveatMerger<unknown>.

Appendix

Here follows a draft specification of wallet_requestIncrementalPermissions in set theory notation:

Note that, in software engineering terms, we are performing a "right-biased union", which is undefinable in set theory. Consider that:
A: { foo: 'bar' }
B: { foo: 'baz' }
Should yield:
C: { foo: 'baz' }
In set theory, you'd end up with C = [A, B], whereas we end up with C = [B].

Nevertheless, I find it helpful to consider the algorithm in the formalisms of set theory. Just recall that the below specification does not account for merging objects or non-primitive arrays.

  • The subject has a set of permissions A
  • The subject requests incremental permissions B, where A ∩ B = ∅ (i.e. A and B are disjoint)
    • If A ∩ B ≠ ∅, any permissions in the set A ∩ B are preserved unmodified in C.
    • If B = A, the request is a no-op and returns A.
  • If the user approves the request, the subject ends up with the set of permissions C, where C = A ∪ B and C ⊇ B
@rekmarks rekmarks self-assigned this Apr 15, 2024
@rekmarks rekmarks added enhancement New feature or request PermissionController Related to the PermissionController. labels Apr 15, 2024
@rekmarks rekmarks changed the title PermissionController: Add caveat merging and "incremental" permission requests Add caveat merging and "incremental" permission requests May 1, 2024
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
enhancement New feature or request PermissionController Related to the PermissionController.
Projects
None yet
1 participant