Add `wallet_revokePermissions` RPC method by FrederikBolding · Pull Request #1889 · MetaMask/core

FrederikBolding · 2023-10-23T18:58:53Z

Explanation

Adds a wallet_revokePermissions RPC method that can be used to revoke permissions that a subject has been granted in the PermissionController. This initial version does not support revoking caveats and will revoke the entire requested permission key.

References

Changelog

`@metamask/permission-controller`

Added: Added wallet_revokePermissions RPC method

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

shanejonas · 2023-11-15T13:43:10Z

packages/permission-controller/src/rpc-methods/revokePermissions.test.ts

+      jsonrpc: '2.0',
+      id: 1,
+      method: 'wallet_revokePermissions',
+      params: { snap_dialog: { caveats: [{ type: 'foo', value: 'bar' }] } },


does adding the caveats here only remove that particular caveat?

Not for this implementation no, but the idea is that we can add that down the line without breaking the API.

So we allow passing in caveats now, but they are effectively ignored until we know how to handle them.

the previous PR to extension used permissionController.updateCaveat, is there anything wrong with that approach? https://github.com/MetaMask/metamask-extension/pull/21216/files#diff-6fbff2cfe97ac01b77296ef2122c7e0a5b3ff6a84b584b4d1a87482f35eea3d6R3779

Yeah, it is a flawed approach. Caveat values can be arbitrary JSON and are not standardized: https://github.com/MetaMask/core/blob/main/packages/permission-controller/src/Caveat.ts#L31C19-L31C23

Therefore it is not safe to assume we can modify them in this way.

We need to standardize the caveat format (effectively allowing for caveat merging, easy removal etc) before we implement this. That would be the next iteration of this API, but we have yet to discuss making that change in the PermissionController.

shanejonas · 2023-11-15T13:43:25Z

packages/permission-controller/src/rpc-methods/revokePermissions.test.ts

+      jsonrpc: '2.0',
+      id: 1,
+      method: 'wallet_revokePermissions',
+      params: [],


shouldnt empty just revoke all permissions?

That seems like it could be a bit confusing to me, but not sure

the MIP says it should revoke all permissions. but that probably should say to revoke all per domain not all permissions.

https://github.com/MetaMask/metamask-improvement-proposals/pull/24/files#diff-b9e4917cc96e9807875c63b66f48695f929d4e346175ad78546148cc40404a01R52

I think that is a dangerous API to expose by default. From my perspective a saner default is to have dapps specify the exact permissions they wish to revoke.

This changes the `revokePermissions` rpc method params to be by-position similar to `requestPermissions`

adonesky1 · 2023-11-20T19:49:43Z

packages/permission-controller/src/rpc-methods/revokePermissions.ts

+  revokePermissionsForOrigin(
+    permissionKeys as NonEmptyArray<PermissionConstraint['parentCapability']>,
+  );


Do we need pass along origin here? Or will the hook have that context somehow?

Normally the hooks are bound to the requesting origin and thus not needed to be passed

oh hmm looks like we pass it as an arg for many other hooks. see setNetworkClientIdForDomain and handleWatchAssetRequest for example

ah I see what you mean: https://github.com/MetaMask/metamask-extension/blob/00b9612ceb15ba8916f4b05675c182e65cff7bdb/app/scripts/metamask-controller.js#L4496

yeah ok we can do that. We should go and use this pattern for the other handlers that need origin .

It depends on how the RPC method is implemented. The origin cannot currently be bound in a restricted RPC method handler for instance.

adonesky1

LGTM

packages/permission-controller/src/rpc-methods/revokePermissions.ts

packages/permission-controller/src/rpc-methods/revokePermissions.test.ts

## **Description** This PR adds `wallet_revokePermissions` rpc method support that was added in `permission-controller/rpc-methods` that went in this PR MetaMask/core#1889. All that is needed is to add the hook as the rpc-method gets automatically added [here](https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/lib/rpc-method-middleware/createMethodMiddleware.js#L13). ## **Related issues** Fixes: https://github.com/MetaMask/MetaMask-planning/issues/1681 ## **Manual testing steps** 1. Go to this page https://docs.metamask.io/wallet/reference/wallet_requestpermissions/ 2. Hit `Send Request` to request permissions 3. Click Next, Click Connect 4. Go to this page https://docs.metamask.io/wallet/reference/wallet_getpermissions/ 5. Click `Send Request` to get the permissions 6. See that there is permissions 7. paste this in the console: ```js await window.ethereum.request({ "method": "wallet_revokePermissions", "params": [ { "eth_accounts": {} } ] }); ``` 8. Click `Send Request` to get the permissions 9. see that there is no permissions and they have been revoked. ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [ ] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: Alex Donesky <adonesky@gmail.com>

This PR adds `wallet_revokePermissions` rpc method support that was added in `permission-controller/rpc-methods` that went in this PR MetaMask/core#1889. All that is needed is to add the hook as the rpc-method gets automatically added [here](https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/lib/rpc-method-middleware/createMethodMiddleware.js#L13). --------- Co-authored-by: Alex Donesky <adonesky@gmail.com>

adonesky1 mentioned this pull request Oct 24, 2023

Add revokePermissions method MetaMask/metamask-improvement-proposals#24

Closed

shanejonas reviewed Nov 15, 2023

View reviewed changes

FrederikBolding added 3 commits November 15, 2023 16:25

Add wallet_revokePermissions RPC method

3446c7e

Fix lint

8388ce6

Fix test name

7d4f637

FrederikBolding force-pushed the fb/wallet-revoke-permissions branch from 16e3f3f to 7d4f637 Compare November 15, 2023 15:25

FrederikBolding marked this pull request as ready for review November 15, 2023 15:25

FrederikBolding requested a review from a team as a code owner November 15, 2023 15:25

shanejonas and others added 2 commits November 20, 2023 09:37

wallet revoke permissions (#2066)

42f4d6f

This changes the `revokePermissions` rpc method params to be by-position similar to `requestPermissions`

Merge branch 'main' into fb/wallet-revoke-permissions

cd01f41

adonesky1 reviewed Nov 20, 2023

View reviewed changes

adonesky1 previously approved these changes Nov 20, 2023

View reviewed changes

jiexi reviewed Nov 20, 2023

View reviewed changes

packages/permission-controller/src/rpc-methods/revokePermissions.ts Outdated Show resolved Hide resolved

jiexi reviewed Nov 20, 2023

View reviewed changes

packages/permission-controller/src/rpc-methods/revokePermissions.test.ts Show resolved Hide resolved

Change length check to isNonEmptyArray

be12528

shanejonas dismissed adonesky1’s stale review via be12528 November 20, 2023 21:41

adonesky1 previously approved these changes Nov 20, 2023

View reviewed changes

jiexi previously approved these changes Nov 20, 2023

View reviewed changes

Mrtenz previously approved these changes Nov 21, 2023

View reviewed changes

FrederikBolding added 2 commits November 21, 2023 09:50

Merge branch 'main' into fb/wallet-revoke-permissions

89786b7

Fix RPC method name

0f60648

FrederikBolding dismissed stale reviews from adonesky1, Mrtenz, and jiexi via 0f60648 November 21, 2023 09:00

Mrtenz approved these changes Nov 21, 2023

View reviewed changes

FrederikBolding merged commit 77a7e38 into main Nov 21, 2023

FrederikBolding deleted the fb/wallet-revoke-permissions branch November 21, 2023 10:55

shanejonas mentioned this pull request Nov 22, 2023

Added wallet_revokePermissions rpc method MetaMask/metamask-extension#21948

Merged

13 tasks

adonesky1 mentioned this pull request Nov 23, 2023

Version v11.6.0 cherrypick revoke permissions MetaMask/metamask-extension#21957

Closed

rekmarks mentioned this pull request Apr 26, 2024

Add revoke method to allow invalidating own permissions #723

Closed

Uh oh!

Conversation

FrederikBolding commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

References

Changelog

@metamask/permission-controller

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shanejonas Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adonesky1 Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adonesky1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

FrederikBolding commented Oct 23, 2023 •

edited

Loading

`@metamask/permission-controller`

shanejonas Nov 15, 2023 •

edited

Loading

adonesky1 Nov 20, 2023 •

edited

Loading