Skip to content

**BREAKING** Add PermissionTree class & introduce children to Permission specifications #1513

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

**BREAKING** Add PermissionTree class & introduce children to Permission specifications #1513

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3fc69dc
added PermissionTree class
hmalik88 Jul 12, 2023
1cca663
updated PermissionTree and added test
hmalik88 Jul 12, 2023
9f97a0a
updated PermissionTree & tests, added jsdoc comments
hmalik88 Jul 15, 2023
4866d6c
Merge branch 'main' into hm/add-permission-groups
hmalik88 Jul 15, 2023
3cbf32d
fix linting errors
hmalik88 Jul 15, 2023
c849eae
add spacing
hmalik88 Jul 19, 2023
05b5734
Merge branch 'main' into hm/add-permission-groups
hmalik88 Jul 19, 2023
55c27a8
added tests for permission groups integration
hmalik88 Jul 25, 2023
635f23b
Merge branch 'main' into hm/add-permission-groups
hmalik88 Jul 25, 2023
d888e56
lint fix
hmalik88 Jul 25, 2023
332c8ac
another fix
hmalik88 Jul 25, 2023
34e8b0d
update test names
hmalik88 Jul 25, 2023
34620f2
move side effects to grant permissions and update tests
hmalik88 Jul 31, 2023
53da778
Merge branch 'main' into hm/add-permission-groups
hmalik88 Jul 31, 2023
b6bfbb0
lint fixes
hmalik88 Jul 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/permission-controller/src/Permission.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,12 @@ type PermissionSpecificationBase<Type extends PermissionType> = {
* Leaving this as undefined uses default behaviour where the permission is available to request for all subject types.
*/
subjectTypes?: readonly SubjectType[];

/**
* 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[]>;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

};

/**
Expand Down
286 changes: 277 additions & 9 deletions packages/permission-controller/src/PermissionController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ import assert from 'assert';
import { JsonRpcEngine } from 'json-rpc-engine';
import type { PendingJsonRpcResponse } from 'json-rpc-engine';

import {
CaveatMutatorOperation,
constructPermission,
MethodNames,
PermissionController,
PermissionType,
} from '.';
import type {
AsyncRestrictedMethod,
Caveat,
Expand All @@ -26,13 +33,6 @@ import type {
RestrictedMethodParameters,
ValidPermission,
} from '.';
import {
CaveatMutatorOperation,
constructPermission,
MethodNames,
PermissionController,
PermissionType,
} from '.';
import * as errors from './errors';
import type { EndowmentGetterParams } from './Permission';
import { SubjectType } from './SubjectMetadataController';
Expand Down Expand Up @@ -200,6 +200,11 @@ const PermissionKeys = {
wallet_noopWithRequiredCaveat: 'wallet_noopWithRequiredCaveat',
wallet_noopWithFactory: 'wallet_noopWithFactory',
snap_foo: 'snap_foo',
snap_bar: 'snap_bar',
snap_baz: 'snap_baz',
snap_xyz: 'snap_xyz',
snap_abc: 'snap_abc',
snap_def: 'snap_def',
endowmentAnySubject: 'endowmentAnySubject',
endowmentSnapsOnly: 'endowmentSnapsOnly',
} as const;
Expand Down Expand Up @@ -232,6 +237,11 @@ const PermissionNames = {
wallet_noopWithRequiredCaveat: PermissionKeys.wallet_noopWithRequiredCaveat,
wallet_noopWithFactory: PermissionKeys.wallet_noopWithFactory,
snap_foo: PermissionKeys.snap_foo,
snap_bar: PermissionKeys.snap_bar,
snap_baz: PermissionKeys.snap_baz,
snap_xyz: PermissionKeys.snap_xyz,
snap_abc: PermissionKeys.snap_abc,
snap_def: PermissionKeys.snap_def,
endowmentAnySubject: PermissionKeys.endowmentAnySubject,
endowmentSnapsOnly: PermissionKeys.endowmentSnapsOnly,
} as const;
Expand Down Expand Up @@ -419,6 +429,55 @@ function getDefaultPermissionSpecifications() {
return null;
},
subjectTypes: [SubjectType.Snap],
children: [PermissionKeys.snap_bar],
},
[PermissionKeys.snap_bar]: {
permissionType: PermissionType.RestrictedMethod,
targetName: PermissionKeys.snap_bar,
allowedCaveats: null,
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
return null;
},
subjectTypes: [SubjectType.Snap],
children: [PermissionKeys.snap_baz],
},
[PermissionKeys.snap_baz]: {
permissionType: PermissionType.RestrictedMethod,
targetName: PermissionKeys.snap_baz,
allowedCaveats: [CaveatTypes.filterArrayResponse],
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
return null;
},
subjectTypes: [SubjectType.Snap],
},
[PermissionKeys.snap_xyz]: {
permissionType: PermissionType.RestrictedMethod,
targetName: PermissionKeys.snap_xyz,
allowedCaveats: null,
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
return null;
},
subjectTypes: [SubjectType.Snap],
children: [PermissionKeys.snap_baz],
},
[PermissionKeys.snap_abc]: {
permissionType: PermissionType.RestrictedMethod,
targetName: PermissionKeys.snap_abc,
allowedCaveats: null,
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
return null;
},
subjectTypes: [SubjectType.Snap],
children: [PermissionKeys.snap_def],
},
[PermissionKeys.snap_def]: {
permissionType: PermissionType.RestrictedMethod,
targetName: PermissionKeys.snap_def,
allowedCaveats: null,
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
return null;
},
subjectTypes: [SubjectType.Snap],
},
[PermissionKeys.endowmentAnySubject]: {
permissionType: PermissionType.Endowment,
Expand Down Expand Up @@ -612,6 +671,7 @@ describe('PermissionController', () => {
beforeEach(() => {
jest.clearAllMocks();
});

describe('constructor', () => {
it('initializes a new PermissionController', () => {
const controller = getDefaultPermissionController();
Expand Down Expand Up @@ -3844,12 +3904,13 @@ describe('PermissionController', () => {
},
]);

expect(callActionSpy).toHaveBeenCalledTimes(4);
expect(callActionSpy).toHaveBeenCalledTimes(6);
expect(callActionSpy).toHaveBeenNthCalledWith(
1,
'SubjectMetadataController:getSubjectMetadata',
origin,
);

expect(callActionSpy).toHaveBeenNthCalledWith(
2,
'ApprovalController:addRequest',
Expand All @@ -3864,11 +3925,13 @@ describe('PermissionController', () => {
},
true,
);

expect(callActionSpy).toHaveBeenNthCalledWith(
3,
'SubjectMetadataController:getSubjectMetadata',
origin,
);

expect(callActionSpy).toHaveBeenNthCalledWith(
4,
'SubjectMetadataController:getSubjectMetadata',
Expand Down Expand Up @@ -5108,7 +5171,7 @@ describe('PermissionController', () => {

const updateCaveatSpy = jest.spyOn(controller, 'updateCaveat');

await messenger.call(
messenger.call(
'PermissionController:updateCaveat',
'metamask.io',
'wallet_getSecretArray',
Expand Down Expand Up @@ -5337,4 +5400,209 @@ describe('PermissionController', () => {
expect(error).toMatchObject(expect.objectContaining(expectedError));
});
});

describe('permission groups', () => {
it('are properly handled when a single permission group is granted', () => {
const options = getPermissionControllerOptions();
const { messenger } = options;
const origin = 'npm:@metamask/test-snap-bip44';

const callActionSpy = jest
.spyOn(messenger, 'call')
.mockImplementation(() => {
return {
origin,
name: origin,
subjectType: SubjectType.Snap,
iconUrl: null,
extensionId: null,
};
});

const controller = getDefaultPermissionController(options);

controller.grantPermissions({
subject: { origin },
approvedPermissions: {
snap_foo: {},
},
});

expect(controller.state).toStrictEqual({
subjects: {
[origin]: {
origin,
permissions: {
snap_foo: getPermissionMatcher({
parentCapability: 'snap_foo',
invoker: origin,
}),
snap_bar: getPermissionMatcher({
parentCapability: 'snap_bar',
invoker: origin,
}),
snap_baz: getPermissionMatcher({
parentCapability: 'snap_baz',
invoker: origin,
}),
},
},
},
});

expect(callActionSpy).toHaveBeenCalledTimes(3);
expect(callActionSpy).toHaveBeenCalledWith(
'SubjectMetadataController:getSubjectMetadata',
origin,
);
});

it('are properly handled when multiple permission groups are granted', () => {
const options = getPermissionControllerOptions();
const { messenger } = options;
const origin = 'npm:@metamask/test-snap-bip44';

const callActionSpy = jest
.spyOn(messenger, 'call')
.mockImplementation(() => {
return {
origin,
name: origin,
subjectType: SubjectType.Snap,
iconUrl: null,
extensionId: null,
};
});

const controller = getDefaultPermissionController(options);

controller.grantPermissions({
subject: { origin },
approvedPermissions: {
snap_foo: {},
snap_abc: {},
},
});

expect(controller.state).toStrictEqual({
subjects: {
[origin]: {
origin,
permissions: {
snap_foo: getPermissionMatcher({
parentCapability: 'snap_foo',
invoker: origin,
}),
snap_bar: getPermissionMatcher({
parentCapability: 'snap_bar',
invoker: origin,
}),
snap_baz: getPermissionMatcher({
parentCapability: 'snap_baz',
invoker: origin,
}),
snap_abc: getPermissionMatcher({
parentCapability: 'snap_abc',
invoker: origin,
}),
snap_def: getPermissionMatcher({
parentCapability: 'snap_def',
invoker: origin,
}),
},
},
},
});

expect(callActionSpy).toHaveBeenCalledTimes(5);
expect(callActionSpy).toHaveBeenCalledWith(
'SubjectMetadataController:getSubjectMetadata',
origin,
);
});

it('are properly handled when overlapping permission groups are granted', () => {
const options = getPermissionControllerOptions();
const { messenger } = options;
const origin = 'npm:@metamask/test-snap-bip44';

const callActionSpy = jest
.spyOn(messenger, 'call')
.mockImplementation(() => {
return {
origin,
name: origin,
subjectType: SubjectType.Snap,
iconUrl: null,
extensionId: null,
};
});

const controller = getDefaultPermissionController(options);
const filterArrayResponse = {
type: 'filterArrayResponse',
value: ['foo'],
};

controller.grantPermissions({
subject: { origin },
approvedPermissions: {
snap_foo: {},
snap_xyz: {},
snap_baz: {
caveats: [filterArrayResponse],
},
},
});

expect(controller.state).toStrictEqual({
subjects: {
[origin]: {
origin,
permissions: {
snap_foo: getPermissionMatcher({
parentCapability: 'snap_foo',
invoker: origin,
}),
snap_bar: getPermissionMatcher({
parentCapability: 'snap_bar',
invoker: origin,
}),
snap_baz: getPermissionMatcher({
parentCapability: 'snap_baz',
caveats: [filterArrayResponse],
invoker: origin,
}),
snap_xyz: getPermissionMatcher({
parentCapability: 'snap_xyz',
invoker: origin,
}),
},
},
},
});

expect(callActionSpy).toHaveBeenCalledTimes(4);
expect(callActionSpy).toHaveBeenCalledWith(
'SubjectMetadataController:getSubjectMetadata',
origin,
);
});
});

it('are properly handled when child permissions are requested outside of their parent', () => {
const controller = getDefaultPermissionController();
const origin = 'npm:@metamask/example-snap';

expect(() =>
controller.grantPermissions({
subject: { origin },
approvedPermissions: {
snap_baz: {},
},
}),
).toThrow(
'Invalid permission request, child permissions must also have their parent permissions requested.',
);
});
});
Loading