**BREAKING** Add PermissionTree class & introduce children to Permission specifications
#1513
Conversation
packages/permission-controller/src/PermissionController.test.ts
Outdated
Show resolved
Hide resolved
hmalik88
changed the title
PermissionTree class & introduce children to Permission specificationsPermissionTree class & introduce children to Permission specifications
|
Could you update the architecture document to explain the purpose of permission trees? https://github.com/MetaMask/core/blob/main/packages/permission-controller/ARCHITECTURE.md It would be great especially to have a sentence or two at least on what problem this solves, e.g. when adding permissions, when should you consider using a permission tree/group? |
| let error; | ||
| try { | ||
| await controller.grantPermissions({ | ||
| subject: { origin: '' }, | ||
| approvedPermissions: { | ||
| wallet_getSecretArray: {}, | ||
| }, | ||
| }), | ||
| ).toThrow(new errors.InvalidSubjectIdentifierError('')); | ||
| }); | ||
| } catch (err) { | ||
| error = err; | ||
| } | ||
| expect(error).toStrictEqual(new errors.InvalidSubjectIdentifierError('')); |
There was a problem hiding this comment.
await expect(() => ...).rejects.toThrow(...)?
| * Controller passing in permissions specifications for the tree to analyze. | ||
| */ | ||
| export class PermissionTree { | ||
| private readonly _permissionSpecifications: Readonly< |
There was a problem hiding this comment.
Hash private? This applies to all private variables and methods in this class.
| * An array of associated permission names that fall under this "parent" permission. Target names would be limited to the | ||
| * array of child permissions associated with this particular permission. | ||
| */ | ||
| children?: Readonly<TargetName[]>; |
There was a problem hiding this comment.
Should we enforce that only a special type of permission can have children?
I am thinking it might be useful to define a PermissionGroup similarly to how we have endowments and RPC methods currently and only allow permissions of type "permission group" to actually function as a group.
There was a problem hiding this comment.
Re-doing this PR based on a convo w/ @rekmarks to keep a concept of groups as well, minus the need for an extraneous permission in state.
| const populatedRequest = | ||
| this.permissionTree.getPopulatedRequest(approvedPermissions); | ||
|
|
||
| const sideEffects = this.getSideEffects(populatedRequest); |
There was a problem hiding this comment.
I'm not sure I understand the reasoning behind this. Side-effects were intentionally previously only executed for requested permissions 🤔
There was a problem hiding this comment.
The side-effect is defined in the permission spec, the child permission is requested implicitly, I'm not sure what's the issue?
| @@ -1573,8 +1581,26 @@ export class PermissionController< | |||
| > | |||
| >; | |||
|
|
|||
| const populatedRequest = | |||
There was a problem hiding this comment.
With the current permission tree approach if we change a permission group after its creation a user might be able to get in a weird state where they don't have all the permissions they expect
There was a problem hiding this comment.
True, hmm...maybe that's something that should be taken care of in the migration?
There was a problem hiding this comment.
We need to be explicit with this at least, whether we expect migrations to be written when permission groups change or whether permission groups are dynamic and will automatically change when we update specifications
There was a problem hiding this comment.
I'll mention it in the docs, maybe even leave a code snippet for a migration.
Closes #1459
What:
With the addition of the
childrenproperty to a specification, thePermissionTreeclass was created to analyze the relationships between the permissions we pass into thePermissionController. This class can determine the root permissions and properly walk a permission chain to populate permission requests and also determine if they are invalid.Why:
This gives developers the ability to make their permission requests less verbose and makes it easier for the end user to accept permission requests when they have permission groups to simplify what the origin is asking for. This change in the short term is intended for breaking up the
snap_manageStatemethod into child permissions and for the upcoming custom UI work.How:
Permission requests won't change, the snap developer can keep the manifest the same, only specifying child permissions IF they need to specify a caveat.
Note: Currently, I've allowed the implementation to be flexible enough to just enforce parent permissions being called, but In the future, this class can be used to enforce strict permission chains starting from root permissions if we so wish OR even a hybrid approach for certain permission chains.