Add revokePermissions method #24
Conversation
|
The Summary, Motivation/Rationale, and User Experience considerations need some refinement. I'm not sure I see revokePermission is as adding anything that user's cannot currently do themselves through the MetaMask UI. Can you be more specific about how this method helps from a dapp perspective? What other approaches were considered and why were they not sufficient? (I'm guessing this has something to do with dapps preferring to use MetaMask's wallet_getPermission method to determine whether a given account is deemed "connected" or not - vs managing this state on dapp-side state/storage) Answering these questions would help other dapp developers understand it's value and when to use it better. |
Great, thanks for your feedback @vandan. I just pushed up a commit that addresses your CR feedback. I think the motivations for this proposal are much more clear with these updates, but please let me know if there's anything that's unclear. |
Nice improvements here. I think the dapp-side considerations are much more clear now. Thank you! |
| The updated method signature will be as follows: | ||
|
|
||
| ``` | ||
| await window.ethereum.request({ | ||
| "method": "wallet_revokePermissions", | ||
| "params": { | ||
| "permission": { | ||
| "caveatValue": string[], // value of the caveat to be revoked from the target permission | ||
| "target": "eth_accounts" | "wallet_snap" | "snap_dialog" | "snap_notify" | "snap_manageState" | ||
| "caveatType": string // type of the caveat to update, | ||
| "id": string // id of permission to be revoked | ||
| } | ||
| } | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
Could we include an example of an expected success response we'd return to the requester?
There was a problem hiding this comment.
this should probably be a Permission to be consistent
which looks like:
requestPermissions looks like it takes that as well with caveats: [] in this example:
There was a problem hiding this comment.
There was a problem hiding this comment.
{
jsonrpc: '2.0',
id: 1,
method: 'wallet_revokePermissions',
params: [
{
snap_dialog: {
caveats: [
{
type: 'foo',
value: 'bar'
}
]
}
}
]
}
| The MetaMask team will be responsible for implementing the proposed changes to the `wallet_revokePermissions` method. | ||
|
|
||
| ## Developer Adoption Considerations | ||
| 1. Backward Compatibility: Dapps that currently manage permissions using custom logic will need to update their code to integrate this new method, however the method is backwards compatible. |
There was a problem hiding this comment.
Dapps that currently manage permissions using custom logic will need to update their code to integrate this new method
not sure what this refers to? Could you spell out what you mean here a bit more?
|
|
||
| Streamlining Disconnection: Reducing the number of clicks and steps needed to disconnect from a dApp, making the experience more user-friendly. | ||
|
|
||
| Consistency in Connection Management: Providing a disconnect feature directly within the dApp aligns with user expectations and creates a consistent experience. |
There was a problem hiding this comment.
can you elaborate on what you mean by consistent here? Consistent that the connect and disconnect can both be requested by the dapp?
There was a problem hiding this comment.
Re: UX. Like on connect to a dapp, I would expect to have MetaMask notify me of disconnect. I would not necessarily trust that a dapp has disconnected itself just from the dapp ui itself
| ## Security Considerations | ||
| The introduction of the `wallet_revokePermissions` method bolsters security by providing users with more control over permission revocation, reducing the potential attack surface. By allowing users to revoke permissions either partially or entirely, it minimizes the risk of unauthorized or malicious activity. | ||
|
|
||
| However, the security model depends on users actively managing these permissions, and there's a minor risk that poorly implemented dApps could confuse users about what they're revoking, potentially leading to unwanted outcomes. |
There was a problem hiding this comment.
what sorts of unwanted outcomes do you foresee?
|
@FrederikBolding has a draft implementation of this method here: MetaMask/core#1889 |
|
Link to original extension PR: MetaMask/metamask-extension#21216 |
|
|
||
| By implementing wallet_revokePermissions, we achieve feature parity with traditional permission systems, offering a more robust, secure, and user-friendly environment. | ||
|
|
||
| # Usage Example |
There was a problem hiding this comment.
I would avoid usage examples upfront before defining the signature on line 68
|
I've pushed this branch directly to the repo to edit more efficiently. Going to close this in favour of the other PR. |
Overview
The proposed MIP introduces a new JSON-RPC method,
wallet_revokePermissions, to MetaMask. This method aims to empower users with more control over permission management by offering the ability to revoke either specific or all permissions associated with connected dApps (e.g. disconnecting one or many accounts). Users can specify the target permission and the associated accounts for which permissions should be revoked.