MetaMask / MetaMask/core

Remove `RestrictedMethod` permission type

Open
#4,238 0 comments 0 reactions 0 assignees View on GitHub
team-wallet-framework wf-permission-controller-improvements
Dominant language
TypeScript
Stars
413
Forks
308
Avg merge
1d 4h
Merged PRs (30d)
253

Description

Requires: #4239

As elaborated in the permission controller's [architecture documentation](https://github.com/MetaMask/core/blob/3227bd51bc4eca0531a53f4be6468404e6a5f032/packages/permission-controller/ARCHITECTURE.md), its original purpose was to control access to RPC methods. To this end, we enshrined this responsibility within the current implementation of the permission controller in the form of the `RestrictedMethod` permission type. In the almost 2.5 years since [we began using this implementation](https://github.com/MetaMask/metamask-extension/pull/12243), we (special credit to @Gudahtt) have determined that the `RestrictedMethod` permission is misguided, and should be removed.

First, the `RestrictedMethod` permission type adds significant complexity to the implementation of the permission controller. `/restrictedmethod/i` is currently mentioned 46 times in the permission controller, and methods like `executeRestrictedMethod` and imports like `decorateWithCaveats` exist solely to support this permission type.

Second, and more importantly, although the `RestrictedMethod` permission type is supposed to encapsulate the enforcement of RPC method permissions within the permission controller, this goal has never been, and can never be, accomplished. For one thing, RPC method implementations still need to ask the permission controller if an action is permitted or not. Yet, the controller cannot and should not assume responsibility of the entire RPC pipeline. For another thing, we have been making ad hoc mutations of permissions since the days of `rpc-cap`, namely by editing the `eth_accounts` array according to our needs.

In other words, the `RestrictedMethod` permission type exists to uphold a lie at the cost of convoluting one of our most security-critical abstractions. The permission controller should have a single responsibility: at all times maintaining a valid permissions state. To that end, we should remove the `RestrictedMethod` permission type, replacing it entirely with the use-case agnostic `Endowment` permission type, and ultimately get rid of the notion of "permission types" entirely.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with the permission controller architecture documentation and inspect the permission controller references to `/restrictedmethod/i`, including `executeRestrictedMethod` and `decorateWithCaveats`. Review the prerequisite issue #4239 before changing anything. Done means the RestrictedMethod permission type and its supporting concepts are removed and replaced with the use-case-agnostic Endowment type.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
authorization, security
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.