Skip to content

Added wallet_revokePermissions rpc method#21948

Merged
adonesky1 merged 4 commits intodevelopfrom
add-revoke-permissions-rpc-method
Nov 23, 2023
Merged

Added wallet_revokePermissions rpc method#21948
adonesky1 merged 4 commits intodevelopfrom
add-revoke-permissions-rpc-method

Conversation

@shanejonas
Copy link
Copy Markdown
Contributor

@shanejonas shanejonas commented Nov 22, 2023 •
edited
Loading

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.

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:
await window.ethereum.request({
  "method": "wallet_revokePermissions",
  "params": [
    {
      "eth_accounts": {}
    }
  ]
});
  1. Click Send Request to get the permissions
  2. see that there is no permissions and they have been revoked.

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). 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.

@shanejonas shanejonas requested a review from a team as a code owner November 22, 2023 20:32
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 22, 2023

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@socket-security
Copy link
Copy Markdown

socket-security bot commented Nov 22, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@metamask/permission-controller 6.0.0 None +1 523 kB

@socket-security
Copy link
Copy Markdown

socket-security bot commented Nov 22, 2023 •
edited
Loading

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: @metamask/permission-controller@6.0.0, @metamask/approval-controller@5.0.0

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

@adonesky1 adonesky1 added the team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead label Nov 22, 2023
BelfordZ
BelfordZ previously approved these changes Nov 22, 2023
View reviewed changes
adonesky1
adonesky1 reviewed Nov 22, 2023
View reviewed changes
app/scripts/metamask-controller.js
Comment on lines +4532 to +4535
revokePermissionsForOrigin: (permissionKeys) => {
this.permissionController.revokePermissions({
[origin]: permissionKeys,
});
Copy link
Copy Markdown
Contributor

@adonesky1 adonesky1 Nov 22, 2023

Choose a reason for hiding this comment

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

@shanejonas and I discussed and we may want to change the method signature for the hook so we don't necessarily have to do this formatting here. But for now this works.

adonesky1
adonesky1 previously approved these changes Nov 22, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

I've tested and it works for me! LGTM. Could we just get a lil PR description

@adonesky1
Copy link
Copy Markdown
Contributor

adonesky1 commented Nov 22, 2023

@metamaskbot update-policies

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 22, 2023
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Nov 22, 2023

Policies updated

@metamaskbot metamaskbot dismissed stale reviews from adonesky1 and BelfordZ via cf19790 November 22, 2023 20:52
@metamaskbot metamaskbot requested review from a team as code owners November 22, 2023 20:52
@jiexi
Copy link
Copy Markdown
Member

jiexi commented Nov 22, 2023

Tested. Working for me too

@adonesky1
Copy link
Copy Markdown
Contributor

adonesky1 commented Nov 22, 2023

@SocketSecurity ignore @metamask/permission-controller@6.0.0

@adonesky1
Copy link
Copy Markdown
Contributor

adonesky1 commented Nov 22, 2023

@SocketSecurity ignore @metamask/approval-controller@5.0.0

@tmashuang tmashuang mentioned this pull request Nov 22, 2023
Closed
13 tasks
@adonesky1 adonesky1 merged commit d885012 into develop Nov 23, 2023
@adonesky1 adonesky1 deleted the add-revoke-permissions-rpc-method branch November 23, 2023 01:40
@github-actions github-actions bot locked and limited conversation to collaborators Nov 23, 2023
@metamaskbot metamaskbot added the release-11.7.0 Issue or pull request that will be included in release 11.7.0 label Nov 23, 2023
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Nov 23, 2023

Builds ready [8d38120]
Page Load Metrics (497 ± 246 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint801008753
domContentLoaded63927794
load751241497512246
domInteractive63927794
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 2.14 KiB (0.06%)
  • ui: 0 Bytes (0.00%)
  • common: 146.19 KiB (3.10%)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jiexi jiexi jiexi approved these changes

@adonesky1 adonesky1 adonesky1 approved these changes

+1 more reviewer

@BelfordZ BelfordZ BelfordZ left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

INVALID-PR-TEMPLATE PR's body doesn't match template release-11.7.0 Issue or pull request that will be included in release 11.7.0 team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shanejonas @adonesky1 @metamaskbot @jiexi @BelfordZ