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

feat(permission-controller): Add requestPermissionsIncremental() and caveat merger functions #4222

Merged
merged 27 commits into from
May 22, 2024

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Apr 26, 2024

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 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

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

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.)
@rekmarks rekmarks changed the title feat(permission-controller): Add caveat merging and requestPermissionsIncremental() feat(permission-controller): Add requestPermissionsIncremental() and caveat merger functions Apr 29, 2024
@rekmarks rekmarks marked this pull request as ready for review April 29, 2024 03:08
@rekmarks rekmarks requested a review from a team as a code owner April 29, 2024 03:08
hmalik88
hmalik88 previously approved these changes May 1, 2024
Copy link
Contributor

@hmalik88 hmalik88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rekmarks rekmarks merged commit 3adbaf1 into main May 22, 2024
147 checks passed
@rekmarks rekmarks deleted the caveat-merging branch May 22, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add caveat merging and "incremental" permission requests
3 participants