diff --git a/packages/examples/examples/insights/snap.manifest.json b/packages/examples/examples/insights/snap.manifest.json index 2d193aa6b2..a1a06b1b7e 100644 --- a/packages/examples/examples/insights/snap.manifest.json +++ b/packages/examples/examples/insights/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps-monorepo.git" }, "source": { - "shasum": "Ndblid1yVfWgEbdlNKR6snzgiTCNBLF1Zxd7EDpDORk=", + "shasum": "+rfL2d5iP9dQMMKAsoGzQx/MpsLoH1px8GXFwg+xHvs=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/examples/signer/packages/core/snap.manifest.json b/packages/examples/examples/signer/packages/core/snap.manifest.json index d7692d772a..b26b2b85c8 100644 --- a/packages/examples/examples/signer/packages/core/snap.manifest.json +++ b/packages/examples/examples/signer/packages/core/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps-monorepo.git" }, "source": { - "shasum": "wSZHQDJImPu8oR0+tQBzszPWlr/XIDcI+v7Kf/Pq/vA=", + "shasum": "HBXPKk8+lODtF82apQ4pbnenuhGwtbgo/ItWgSxKvgk=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/rpc-methods/jest.config.js b/packages/rpc-methods/jest.config.js index f9c921cc7d..adfa6ddd28 100644 --- a/packages/rpc-methods/jest.config.js +++ b/packages/rpc-methods/jest.config.js @@ -10,10 +10,10 @@ module.exports = deepmerge(baseConfig, { ], coverageThreshold: { global: { - branches: 84.95, + branches: 85.45, functions: 94.73, - lines: 94.37, - statements: 94.4, + lines: 95.01, + statements: 95.03, }, }, }); diff --git a/packages/rpc-methods/package.json b/packages/rpc-methods/package.json index da5d702e21..9435090598 100644 --- a/packages/rpc-methods/package.json +++ b/packages/rpc-methods/package.json @@ -30,7 +30,7 @@ "dependencies": { "@metamask/browser-passworder": "^4.0.2", "@metamask/key-tree": "^7.0.0", - "@metamask/permission-controller": "^3.0.0", + "@metamask/permission-controller": "^3.1.0", "@metamask/snaps-ui": "^0.31.0", "@metamask/snaps-utils": "^0.31.0", "@metamask/types": "^1.1.0", diff --git a/packages/rpc-methods/src/permitted/requestSnaps.test.ts b/packages/rpc-methods/src/permitted/requestSnaps.test.ts index 8e0546ea37..f2575a2909 100644 --- a/packages/rpc-methods/src/permitted/requestSnaps.test.ts +++ b/packages/rpc-methods/src/permitted/requestSnaps.test.ts @@ -190,12 +190,13 @@ describe('implementation', () => { invoker: 'https://metamask.github.io', parentCapability: WALLET_SNAP_PERMISSION_KEY, }, + { + data: { + [WALLET_SNAP_PERMISSION_KEY]: { [MOCK_SNAP_ID]: getTruncatedSnap() }, + }, + }, ]); - hooks.installSnaps.mockImplementation(() => ({ - [MOCK_SNAP_ID]: getTruncatedSnap(), - })); - const engine = new JsonRpcEngine(); engine.push((req, res, next, end) => { const result = implementation( @@ -226,10 +227,6 @@ describe('implementation', () => { }, }); - expect(hooks.installSnaps).toHaveBeenCalledWith({ - [MOCK_SNAP_ID]: {}, - }); - expect(response.result).toStrictEqual({ [MOCK_SNAP_ID]: getTruncatedSnap(), }); @@ -294,4 +291,50 @@ describe('implementation', () => { [MOCK_SNAP_ID]: getTruncatedSnap(), }); }); + + it('throws with the appropriate error if the side-effect fails', async () => { + const { implementation } = requestSnapsHandler; + + const hooks = getMockHooks(); + + hooks.requestPermissions.mockImplementation(async () => { + throw new Error('error'); + }); + + const engine = new JsonRpcEngine(); + engine.push((req, res, next, end) => { + const result = implementation( + req as JsonRpcRequest, + res as PendingJsonRpcResponse, + next, + end, + hooks, + ); + + result?.catch(end); + }); + + const response = (await engine.handle({ + jsonrpc: '2.0', + id: 1, + method: 'wallet_requestSnaps', + params: { + [MOCK_SNAP_ID]: {}, + }, + })) as JsonRpcSuccess; + + expect(hooks.requestPermissions).toHaveBeenCalledWith({ + [WALLET_SNAP_PERMISSION_KEY]: { + caveats: [ + { type: SnapCaveatType.SnapIds, value: { [MOCK_SNAP_ID]: {} } }, + ], + }, + }); + + expect(response).toStrictEqual({ + error: { code: -32603, data: { originalError: {} }, message: 'error' }, + id: 1, + jsonrpc: '2.0', + }); + }); }); diff --git a/packages/rpc-methods/src/permitted/requestSnaps.ts b/packages/rpc-methods/src/permitted/requestSnaps.ts index a29571873a..69fe757f80 100644 --- a/packages/rpc-methods/src/permitted/requestSnaps.ts +++ b/packages/rpc-methods/src/permitted/requestSnaps.ts @@ -57,7 +57,12 @@ export type RequestSnapsHooks = { */ requestPermissions: ( permissions: RequestedPermissions, - ) => Promise; + ) => Promise< + [ + Record, + { data: Record; id: string; origin: string }, + ] + >; /** * Gets the current permissions for the requesting origin. @@ -177,9 +182,6 @@ async function requestSnapsImplementation( ); } - // Request the permission for the installing DApp to talk to the snap, if needed - // TODO: Should this be part of the install flow? - try { if (!Object.keys(requestedSnaps).length) { throw new Error('Request must have at least one requested snap.'); @@ -192,33 +194,27 @@ async function requestSnapsImplementation( } as RequestedPermissions; const existingPermissions = await getPermissions(); - let approvedPermissions = []; - if (!existingPermissions) { - approvedPermissions = await requestPermissions(requestedPermissions); - } else if (!hasRequestedSnaps(existingPermissions, requestedSnaps)) { + const [, metadata] = await requestPermissions(requestedPermissions); + res.result = metadata.data[ + WALLET_SNAP_PERMISSION_KEY + ] as InstallSnapsResult; + } else if (hasRequestedSnaps(existingPermissions, requestedSnaps)) { + res.result = await handleInstallSnaps(requestedSnaps, installSnaps); + } else { const mergedPermissionsRequest = getSnapPermissionsRequest( existingPermissions, requestedPermissions, ); - approvedPermissions = await requestPermissions(mergedPermissionsRequest); - } - if ( - (!existingPermissions || - !hasRequestedSnaps(existingPermissions, requestedSnaps)) && - !approvedPermissions?.length - ) { - throw ethErrors.provider.userRejectedRequest({ data: req }); + const [, metadata] = await requestPermissions(mergedPermissionsRequest); + res.result = metadata.data[ + WALLET_SNAP_PERMISSION_KEY + ] as InstallSnapsResult; } - } catch (error) { - return end(error); - } - - try { - res.result = await handleInstallSnaps(requestedSnaps, installSnaps); } catch (error) { res.error = error; } + return end(); } diff --git a/packages/rpc-methods/src/restricted/invokeSnap.test.ts b/packages/rpc-methods/src/restricted/invokeSnap.test.ts index 2e9179b034..6d4cf68171 100644 --- a/packages/rpc-methods/src/restricted/invokeSnap.test.ts +++ b/packages/rpc-methods/src/restricted/invokeSnap.test.ts @@ -1,6 +1,7 @@ import { Caveat, OriginString, + PermissionsRequest, PermissionType, } from '@metamask/permission-controller'; import { SnapCaveatType, SnapId } from '@metamask/snaps-utils'; @@ -8,6 +9,7 @@ import { MOCK_SNAP_ID, MOCK_ORIGIN, getTruncatedSnap, + MockControllerMessenger, } from '@metamask/snaps-utils/test-utils'; import { Json } from '@metamask/utils'; @@ -17,6 +19,8 @@ import { validateCaveat, InvokeSnapCaveatSpecifications, WALLET_SNAP_PERMISSION_KEY, + handleSnapInstall, + InstallSnaps, } from './invokeSnap'; describe('builder', () => { @@ -224,3 +228,56 @@ describe('implementation', () => { expect(hooks.handleSnapRpcRequest).not.toHaveBeenCalled(); }); }); + +describe('handleSnapInstall', () => { + it('calls SnapController:install with the right parameters', async () => { + const messenger = new MockControllerMessenger(); + + const sideEffectMessenger = messenger.getRestricted({ + name: 'PermissionController', + allowedActions: ['SnapController:install'], + }); + + const expectedResult = { + [MOCK_SNAP_ID]: getTruncatedSnap(), + }; + + messenger.registerActionHandler( + 'SnapController:install', + async () => expectedResult, + ); + + jest.spyOn(sideEffectMessenger, 'call'); + + const requestedSnaps = { + [MOCK_SNAP_ID]: {}, + }; + + const requestData = { + permissions: { + [WALLET_SNAP_PERMISSION_KEY]: { + caveats: [ + { + type: SnapCaveatType.SnapIds, + value: requestedSnaps, + }, + ], + }, + }, + metadata: { origin: MOCK_ORIGIN, id: 'foo' }, + } as PermissionsRequest; + + const result = await handleSnapInstall({ + requestData, + messagingSystem: sideEffectMessenger, + }); + + expect(sideEffectMessenger.call).toHaveBeenCalledWith( + 'SnapController:install', + MOCK_ORIGIN, + requestedSnaps, + ); + + expect(result).toBe(expectedResult); + }); +}); diff --git a/packages/rpc-methods/src/restricted/invokeSnap.ts b/packages/rpc-methods/src/restricted/invokeSnap.ts index d4d1b0419c..4a45f74152 100644 --- a/packages/rpc-methods/src/restricted/invokeSnap.ts +++ b/packages/rpc-methods/src/restricted/invokeSnap.ts @@ -7,6 +7,7 @@ import { Caveat, RestrictedMethodParameters, PermissionValidatorConstraint, + PermissionSideEffect, } from '@metamask/permission-controller'; import { Snap, @@ -15,6 +16,8 @@ import { SnapRpcHookArgs, SnapCaveatType, assertIsValidSnapId, + RequestedSnapPermissions, + InstallSnapsResult, } from '@metamask/snaps-utils'; import { isJsonRpcRequest, @@ -30,6 +33,17 @@ import { MethodHooksObject } from '../utils'; export const WALLET_SNAP_PERMISSION_KEY = 'wallet_snap'; +// Redeclare installSnaps action type to avoid circular dependencies +export type InstallSnaps = { + type: `SnapController:install`; + handler: ( + origin: string, + requestedSnaps: RequestedSnapPermissions, + ) => Promise; +}; + +type AllowedActions = InstallSnaps; + export type InvokeSnapMethodHooks = { getSnap: (snapId: SnapId) => Snap | undefined; handleSnapRpcRequest: ({ @@ -51,6 +65,9 @@ type InvokeSnapSpecification = ValidPermissionSpecification<{ methodImplementation: ReturnType; allowedCaveats: Readonly> | null; validator: PermissionValidatorConstraint; + sideEffect: { + onPermitted: PermissionSideEffect['onPermitted']; + }; }>; type InvokeSnapParams = { @@ -77,6 +94,26 @@ export function validateCaveat(caveat: Caveat) { } } +/** + * The side-effect method to handle the snap install. + * + * @param params - The side-effect params. + * @param params.requestData - The request data associated to the requested permission. + * @param params.messagingSystem - The messenger to call an action. + */ +export const handleSnapInstall: PermissionSideEffect< + AllowedActions, + never +>['onPermitted'] = async ({ requestData, messagingSystem }) => { + const snaps = requestData.permissions[WALLET_SNAP_PERMISSION_KEY].caveats?.[0] + .value as RequestedSnapPermissions; + + return messagingSystem.call( + `SnapController:install`, + requestData.metadata.origin, + snaps, + ); +}; /** * The specification builder for the `wallet_snap_*` permission. * @@ -106,6 +143,9 @@ const specificationBuilder: PermissionSpecificationBuilder< }); } }, + sideEffect: { + onPermitted: handleSnapInstall, + }, }; }; diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 807d2514fd..b793e36ac9 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { - "branches": 89.53, + "branches": 89.51, "functions": 95.17, "lines": 96.64, - "statements": 96.55 + "statements": 96.54 } diff --git a/packages/snaps-controllers/package.json b/packages/snaps-controllers/package.json index f6214f1415..71fcdfd597 100644 --- a/packages/snaps-controllers/package.json +++ b/packages/snaps-controllers/package.json @@ -37,7 +37,7 @@ "@metamask/approval-controller": "^2.0.0", "@metamask/base-controller": "^2.0.0", "@metamask/object-multiplex": "^1.1.0", - "@metamask/permission-controller": "^3.0.0", + "@metamask/permission-controller": "^3.1.0", "@metamask/post-message-stream": "^6.1.1", "@metamask/rpc-methods": "^0.31.0", "@metamask/snaps-execution-environments": "^0.31.0", diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts index 34f1143f3a..2875e261a3 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.test.ts @@ -591,15 +591,10 @@ describe('SnapController', () => { [MOCK_SNAP_ID]: expectedSnapObject, }); - expect(messenger.call).toHaveBeenCalledTimes(9); - expect(messenger.call).toHaveBeenNthCalledWith( - 1, - 'PermissionController:getPermissions', - MOCK_ORIGIN, - ); + expect(messenger.call).toHaveBeenCalledTimes(8); expect(messenger.call).toHaveBeenNthCalledWith( - 2, + 1, 'ApprovalController:addRequest', expect.objectContaining({ requestData: { @@ -617,12 +612,12 @@ describe('SnapController', () => { true, ); - expect(messenger.call).toHaveBeenNthCalledWith(3, 'SnapsRegistry:get', { + expect(messenger.call).toHaveBeenNthCalledWith(2, 'SnapsRegistry:get', { [MOCK_SNAP_ID]: { version: '1.0.0', checksum: DEFAULT_SNAP_SHASUM }, }); expect(messenger.call).toHaveBeenNthCalledWith( - 4, + 3, 'ApprovalController:updateRequestState', { id: expect.any(String), @@ -634,13 +629,13 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 5, + 4, 'PermissionController:grantPermissions', expect.any(Object), ); expect(messenger.call).toHaveBeenNthCalledWith( - 6, + 5, 'ApprovalController:addRequest', expect.objectContaining({ requestData: { @@ -659,20 +654,20 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 7, + 6, 'ExecutionService:executeSnap', expect.any(Object), ); expect(messenger.call).toHaveBeenNthCalledWith( - 8, + 7, 'PermissionController:hasPermission', MOCK_SNAP_ID, SnapEndowments.LongRunning, ); expect(messenger.call).toHaveBeenNthCalledWith( - 9, + 8, 'ApprovalController:updateRequestState', { id: expect.any(String), @@ -788,7 +783,6 @@ describe('SnapController', () => { ); const authorizeSpy = jest.spyOn(controller as any, 'authorize'); - const messengerCallMock = jest.spyOn(messenger, 'call'); await controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: { version: '>0.9.0 <1.1.0' }, @@ -798,12 +792,6 @@ describe('SnapController', () => { expect(newSnap).toStrictEqual(getSnapObject()); expect(authorizeSpy).not.toHaveBeenCalled(); - expect(messengerCallMock).toHaveBeenCalledTimes(1); - expect(messengerCallMock).toHaveBeenNthCalledWith( - 1, - 'PermissionController:getPermissions', - MOCK_ORIGIN, - ); controller.destroy(); }); @@ -834,7 +822,7 @@ describe('SnapController', () => { ).rejects.toThrow('User rejected the request.'); expect(messenger.call).toHaveBeenNthCalledWith( - 2, + 1, 'ApprovalController:addRequest', expect.objectContaining({ id: expect.any(String), @@ -855,7 +843,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 5, + 4, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -912,15 +900,10 @@ describe('SnapController', () => { }), ).rejects.toThrow('foo'); - expect(messengerCallMock).toHaveBeenCalledTimes(9); - expect(messengerCallMock).toHaveBeenNthCalledWith( - 1, - 'PermissionController:getPermissions', - MOCK_ORIGIN, - ); + expect(messengerCallMock).toHaveBeenCalledTimes(8); expect(messengerCallMock).toHaveBeenNthCalledWith( - 4, + 3, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -1828,11 +1811,7 @@ describe('SnapController', () => { [MOCK_SNAP_ID]: {}, }); expect(result).toStrictEqual({ [MOCK_SNAP_ID]: truncatedSnap }); - expect(messenger.call).toHaveBeenCalledTimes(1); - expect(messenger.call).toHaveBeenCalledWith( - 'PermissionController:getPermissions', - MOCK_ORIGIN, - ); + expect(authorizeSpy).not.toHaveBeenCalled(); snapController.destroy(); @@ -1882,15 +1861,10 @@ describe('SnapController', () => { expect(result).toStrictEqual({ [MOCK_LOCAL_SNAP_ID]: truncatedSnap }); - expect(messenger.call).toHaveBeenCalledTimes(11); - expect(messenger.call).toHaveBeenNthCalledWith( - 1, - 'PermissionController:getPermissions', - MOCK_ORIGIN, - ); + expect(messenger.call).toHaveBeenCalledTimes(10); expect(messenger.call).toHaveBeenNthCalledWith( - 2, + 1, 'ApprovalController:addRequest', expect.objectContaining({ type: SNAP_APPROVAL_INSTALL, @@ -1910,7 +1884,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 6, + 5, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -1922,7 +1896,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 7, + 6, 'PermissionController:grantPermissions', { approvedPermissions: permissions, @@ -1939,7 +1913,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 8, + 7, 'ApprovalController:addRequest', expect.objectContaining({ type: SNAP_APPROVAL_RESULT, @@ -1959,20 +1933,20 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 9, + 8, 'ExecutionService:executeSnap', expect.objectContaining({}), ); expect(messenger.call).toHaveBeenNthCalledWith( - 10, + 9, 'PermissionController:hasPermission', MOCK_LOCAL_SNAP_ID, SnapEndowments.LongRunning, ); expect(messenger.call).toHaveBeenNthCalledWith( - 11, + 10, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -2042,15 +2016,10 @@ describe('SnapController', () => { [MOCK_LOCAL_SNAP_ID]: truncatedSnap, }); - expect(messenger.call).toHaveBeenCalledTimes(21); - expect(messenger.call).toHaveBeenNthCalledWith( - 1, - 'PermissionController:getPermissions', - MOCK_ORIGIN, - ); + expect(messenger.call).toHaveBeenCalledTimes(19); expect(messenger.call).toHaveBeenNthCalledWith( - 2, + 1, 'ApprovalController:addRequest', expect.objectContaining({ type: SNAP_APPROVAL_INSTALL, @@ -2070,7 +2039,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 4, + 3, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -2082,7 +2051,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 5, + 4, 'PermissionController:grantPermissions', { approvedPermissions: permissions, @@ -2099,7 +2068,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 6, + 5, 'ApprovalController:addRequest', expect.objectContaining({ id: expect.any(String), @@ -2120,20 +2089,20 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 7, + 6, 'ExecutionService:executeSnap', expect.anything(), ); expect(messenger.call).toHaveBeenNthCalledWith( - 8, + 7, 'PermissionController:hasPermission', MOCK_LOCAL_SNAP_ID, SnapEndowments.LongRunning, ); expect(messenger.call).toHaveBeenNthCalledWith( - 9, + 8, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -2145,13 +2114,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 10, - 'PermissionController:getPermissions', - MOCK_ORIGIN, - ); - - expect(messenger.call).toHaveBeenNthCalledWith( - 11, + 9, 'ApprovalController:addRequest', expect.objectContaining({ type: SNAP_APPROVAL_INSTALL, @@ -2171,13 +2134,13 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 12, + 10, 'ExecutionService:terminateSnap', MOCK_LOCAL_SNAP_ID, ); expect(messenger.call).toHaveBeenNthCalledWith( - 16, + 14, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -2189,7 +2152,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 17, + 15, 'PermissionController:grantPermissions', { approvedPermissions: permissions, @@ -2206,7 +2169,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 18, + 16, 'ApprovalController:addRequest', expect.objectContaining({ id: expect.any(String), @@ -2227,20 +2190,20 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 19, + 17, 'ExecutionService:executeSnap', expect.objectContaining({ snapId: MOCK_LOCAL_SNAP_ID }), ); expect(messenger.call).toHaveBeenNthCalledWith( - 20, + 18, 'PermissionController:hasPermission', MOCK_LOCAL_SNAP_ID, SnapEndowments.LongRunning, ); expect(messenger.call).toHaveBeenNthCalledWith( - 21, + 19, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -2298,7 +2261,7 @@ describe('SnapController', () => { ).rejects.toThrow('User rejected the request.'); expect(messenger.call).toHaveBeenNthCalledWith( - 2, + 1, 'ApprovalController:addRequest', expect.objectContaining({ type: SNAP_APPROVAL_INSTALL, @@ -2315,7 +2278,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 7, + 6, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -2400,15 +2363,10 @@ describe('SnapController', () => { expect(result).toStrictEqual({ [MOCK_SNAP_ID]: truncatedSnap, }); - expect(messenger.call).toHaveBeenCalledTimes(9); - expect(messenger.call).toHaveBeenNthCalledWith( - 1, - 'PermissionController:getPermissions', - MOCK_ORIGIN, - ); + expect(messenger.call).toHaveBeenCalledTimes(8); expect(messenger.call).toHaveBeenNthCalledWith( - 2, + 1, 'ApprovalController:addRequest', expect.objectContaining({ type: SNAP_APPROVAL_INSTALL, @@ -2425,7 +2383,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 4, + 3, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -2437,7 +2395,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 5, + 4, 'PermissionController:grantPermissions', { approvedPermissions: permissions, @@ -2454,7 +2412,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 6, + 5, 'ApprovalController:addRequest', expect.objectContaining({ id: expect.any(String), @@ -2475,20 +2433,20 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 7, + 6, 'ExecutionService:executeSnap', expect.objectContaining({}), ); expect(messenger.call).toHaveBeenNthCalledWith( - 8, + 7, 'PermissionController:hasPermission', MOCK_SNAP_ID, SnapEndowments.LongRunning, ); expect(messenger.call).toHaveBeenNthCalledWith( - 9, + 8, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -2636,7 +2594,7 @@ describe('SnapController', () => { }); expect(messenger.call).toHaveBeenNthCalledWith( - 2, + 1, 'ApprovalController:addRequest', expect.objectContaining({ type: SNAP_APPROVAL_INSTALL, @@ -2653,7 +2611,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 4, + 3, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -2686,7 +2644,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 5, + 4, 'PermissionController:grantPermissions', { approvedPermissions: { @@ -2760,7 +2718,7 @@ describe('SnapController', () => { }; expect(messenger.call).toHaveBeenNthCalledWith( - 2, + 1, 'ApprovalController:addRequest', expect.objectContaining({ type: SNAP_APPROVAL_INSTALL, @@ -2777,7 +2735,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 4, + 3, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -2793,7 +2751,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 5, + 4, 'PermissionController:grantPermissions', { approvedPermissions: { @@ -2917,31 +2875,6 @@ describe('SnapController', () => { controller.destroy(); }); - it('returns an error if an origin does not have the permission to install a snap', async () => { - const rootMessenger = getControllerMessenger(); - - rootMessenger.registerActionHandler( - 'PermissionController:getPermissions', - () => ({}), - ); - - const controller = getSnapController( - getSnapControllerOptions({ - messenger: getSnapControllerMessenger(rootMessenger), - }), - ); - - await expect( - controller.installSnaps(MOCK_ORIGIN, { - [MOCK_SNAP_ID]: {}, - }), - ).rejects.toThrow( - `Not authorized to install snap "${MOCK_SNAP_ID}". Request the permission for the snap before attempting to install it.`, - ); - - controller.destroy(); - }); - it('updates a snap', async () => { const newVersion = '1.0.2'; const newVersionRange = '>=1.0.1'; @@ -2984,16 +2917,10 @@ describe('SnapController', () => { [MOCK_SNAP_ID]: { version: newVersionRange }, }); - expect(messenger.call).toHaveBeenCalledTimes(19); + expect(messenger.call).toHaveBeenCalledTimes(17); expect(messenger.call).toHaveBeenNthCalledWith( - 11, - 'PermissionController:getPermissions', - MOCK_ORIGIN, - ); - - expect(messenger.call).toHaveBeenNthCalledWith( - 12, + 10, 'ApprovalController:addRequest', { origin: MOCK_ORIGIN, @@ -3015,13 +2942,13 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 14, + 12, 'PermissionController:getPermissions', MOCK_SNAP_ID, ); expect(messenger.call).toHaveBeenNthCalledWith( - 15, + 13, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -3037,7 +2964,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 16, + 14, 'ApprovalController:addRequest', expect.objectContaining({ id: expect.any(String), @@ -3058,20 +2985,20 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 17, + 15, 'ExecutionService:executeSnap', expect.objectContaining({}), ); expect(messenger.call).toHaveBeenNthCalledWith( - 18, + 16, 'PermissionController:hasPermission', MOCK_SNAP_ID, SnapEndowments.LongRunning, ); expect(messenger.call).toHaveBeenNthCalledWith( - 19, + 17, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -3130,11 +3057,7 @@ describe('SnapController', () => { }), ).rejects.toThrow(errorMessage); - expect(messenger.call).toHaveBeenCalledTimes(3); - expect(messenger.call).toHaveBeenCalledWith( - 'PermissionController:getPermissions', - MOCK_ORIGIN, - ); + expect(messenger.call).toHaveBeenCalledTimes(2); expect(messenger.call).toHaveBeenCalledWith( 'ApprovalController:updateRequestState', @@ -3182,11 +3105,7 @@ describe('SnapController', () => { }), ).rejects.toThrow('foo'); - expect(messenger.call).toHaveBeenCalledTimes(3); - expect(messenger.call).toHaveBeenCalledWith( - 'PermissionController:getPermissions', - MOCK_ORIGIN, - ); + expect(messenger.call).toHaveBeenCalledTimes(2); expect(detect).toHaveBeenCalledTimes(1); expect(detect).toHaveBeenCalledWith( MOCK_SNAP_ID, @@ -3593,10 +3512,10 @@ describe('SnapController', () => { date: expect.any(Number), }, ]); - expect(callActionSpy).toHaveBeenCalledTimes(18); + expect(callActionSpy).toHaveBeenCalledTimes(17); expect(callActionSpy).toHaveBeenNthCalledWith( - 11, + 10, 'ApprovalController:addRequest', { origin: MOCK_ORIGIN, @@ -3617,14 +3536,14 @@ describe('SnapController', () => { true, ); - expect(callActionSpy).toHaveBeenNthCalledWith( - 13, + expect(messenger.call).toHaveBeenNthCalledWith( + 12, 'PermissionController:getPermissions', MOCK_SNAP_ID, ); expect(messenger.call).toHaveBeenNthCalledWith( - 14, + 13, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -3640,7 +3559,7 @@ describe('SnapController', () => { ); expect(messenger.call).toHaveBeenNthCalledWith( - 15, + 14, 'ApprovalController:addRequest', expect.objectContaining({ id: expect.any(String), @@ -3661,20 +3580,20 @@ describe('SnapController', () => { ); expect(callActionSpy).toHaveBeenNthCalledWith( - 16, + 15, 'ExecutionService:executeSnap', expect.objectContaining({}), ); expect(callActionSpy).toHaveBeenNthCalledWith( - 17, + 16, 'PermissionController:hasPermission', MOCK_SNAP_ID, SnapEndowments.LongRunning, ); expect(messenger.call).toHaveBeenNthCalledWith( - 18, + 17, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -4126,10 +4045,10 @@ describe('SnapController', () => { await controller.updateSnap(MOCK_ORIGIN, MOCK_SNAP_ID, detect()); - expect(callActionSpy).toHaveBeenCalledTimes(20); + expect(callActionSpy).toHaveBeenCalledTimes(19); expect(callActionSpy).toHaveBeenNthCalledWith( - 11, + 10, 'ApprovalController:addRequest', { origin: MOCK_ORIGIN, @@ -4152,12 +4071,6 @@ describe('SnapController', () => { expect(callActionSpy).toHaveBeenNthCalledWith( 13, - 'PermissionController:getPermissions', - MOCK_SNAP_ID, - ); - - expect(callActionSpy).toHaveBeenNthCalledWith( - 14, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), @@ -4179,7 +4092,7 @@ describe('SnapController', () => { ); expect(callActionSpy).toHaveBeenNthCalledWith( - 15, + 14, 'ApprovalController:addRequest', { origin: MOCK_ORIGIN, @@ -4201,13 +4114,13 @@ describe('SnapController', () => { ); expect(callActionSpy).toHaveBeenNthCalledWith( - 16, + 15, 'PermissionController:revokePermissions', { [MOCK_SNAP_ID]: ['snap_manageState'] }, ); expect(callActionSpy).toHaveBeenNthCalledWith( - 17, + 16, 'PermissionController:grantPermissions', { approvedPermissions: { 'endowment:network-access': {} }, @@ -4224,20 +4137,20 @@ describe('SnapController', () => { ); expect(callActionSpy).toHaveBeenNthCalledWith( - 18, + 17, 'ExecutionService:executeSnap', expect.anything(), ); expect(callActionSpy).toHaveBeenNthCalledWith( - 19, + 18, 'PermissionController:hasPermission', MOCK_SNAP_ID, SnapEndowments.LongRunning, ); expect(callActionSpy).toHaveBeenNthCalledWith( - 20, + 19, 'ApprovalController:updateRequestState', expect.objectContaining({ id: expect.any(String), diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 478b16cf8c..e0ee5b3067 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -59,7 +59,6 @@ import { VirtualFile, logError, logWarning, - isSnapPermitted, } from '@metamask/snaps-utils'; import { GetSubjectMetadata, @@ -1603,17 +1602,6 @@ export class SnapController extends BaseController< ); } - const permissions = this.messagingSystem.call( - 'PermissionController:getPermissions', - origin, - ) as SubjectPermissions; - - if (!isSnapPermitted(permissions, snapId)) { - throw ethErrors.provider.unauthorized( - `Not authorized to install snap "${snapId}". Request the permission for the snap before attempting to install it.`, - ); - } - const location = this.#detectSnapLocation(snapId, { versionRange: version, fetch: this.#fetchFunction, diff --git a/packages/snaps-controllers/src/test-utils/controller.ts b/packages/snaps-controllers/src/test-utils/controller.ts index fce027bcc6..508a06461a 100644 --- a/packages/snaps-controllers/src/test-utils/controller.ts +++ b/packages/snaps-controllers/src/test-utils/controller.ts @@ -1,10 +1,4 @@ import { ApprovalRequest } from '@metamask/approval-controller'; -import { - ActionConstraint, - ActionHandler, - ControllerMessenger, - EventConstraint, -} from '@metamask/base-controller'; import { PermissionConstraint, SubjectPermissions, @@ -14,6 +8,7 @@ import { import { WALLET_SNAP_PERMISSION_KEY } from '@metamask/rpc-methods'; import { SnapCaveatType } from '@metamask/snaps-utils'; import { + MockControllerMessenger, getPersistedSnapObject, getTruncatedSnap, MOCK_LOCAL_SNAP_ID, @@ -49,25 +44,6 @@ const asyncNoOp = async () => Promise.resolve(); * A controller messenger, that allows overwriting the action handlers, without * the need to call `unregisterActionHandler` first. */ -export class MockControllerMessenger< - Action extends ActionConstraint, - Event extends EventConstraint, -> extends ControllerMessenger { - /** - * Registers an action handler for the given action type. If an action handler - * already exists for the given action type, it will be overwritten. - * - * @param actionType - The action type to register the handler for. - * @param handler - The action handler to register. - */ - registerActionHandler( - actionType: T, - handler: ActionHandler, - ) { - super.unregisterActionHandler(actionType); - super.registerActionHandler(actionType, handler); - } -} export class MockApprovalController { #approval?: { diff --git a/packages/snaps-controllers/src/test-utils/execution-environment.ts b/packages/snaps-controllers/src/test-utils/execution-environment.ts index 4aa4c3b91b..32be049f1e 100644 --- a/packages/snaps-controllers/src/test-utils/execution-environment.ts +++ b/packages/snaps-controllers/src/test-utils/execution-environment.ts @@ -1,4 +1,5 @@ import { SnapRpcHookArgs } from '@metamask/snaps-utils'; +import { MockControllerMessenger } from '@metamask/snaps-utils/test-utils'; import { JsonRpcEngine } from 'json-rpc-engine'; import { createEngineStream } from 'json-rpc-middleware-stream'; import pump from 'pump'; @@ -11,7 +12,6 @@ import { setupMultiplex, SnapExecutionData, } from '../services'; -import { MockControllerMessenger } from './controller'; export const MOCK_BLOCK_NUMBER = '0xa70e75'; diff --git a/packages/snaps-utils/package.json b/packages/snaps-utils/package.json index d34609f7d7..60ee3801e9 100644 --- a/packages/snaps-utils/package.json +++ b/packages/snaps-utils/package.json @@ -55,7 +55,8 @@ "dependencies": { "@babel/core": "^7.18.6", "@babel/types": "^7.18.7", - "@metamask/permission-controller": "^3.0.0", + "@metamask/base-controller": "^2.0.0", + "@metamask/permission-controller": "^3.1.0", "@metamask/providers": "^10.2.1", "@metamask/snaps-registry": "^1.2.0", "@metamask/snaps-ui": "^0.31.0", diff --git a/packages/snaps-utils/src/test-utils/controller.ts b/packages/snaps-utils/src/test-utils/controller.ts new file mode 100644 index 0000000000..ce476201ea --- /dev/null +++ b/packages/snaps-utils/src/test-utils/controller.ts @@ -0,0 +1,26 @@ +import { + ActionConstraint, + ActionHandler, + ControllerMessenger, + EventConstraint, +} from '@metamask/base-controller'; + +export class MockControllerMessenger< + Action extends ActionConstraint, + Event extends EventConstraint, +> extends ControllerMessenger { + /** + * Registers an action handler for the given action type. If an action handler + * already exists for the given action type, it will be overwritten. + * + * @param actionType - The action type to register the handler for. + * @param handler - The action handler to register. + */ + registerActionHandler( + actionType: T, + handler: ActionHandler, + ) { + super.unregisterActionHandler(actionType); + super.registerActionHandler(actionType, handler); + } +} diff --git a/packages/snaps-utils/src/test-utils/index.ts b/packages/snaps-utils/src/test-utils/index.ts index 9275aa93e1..ca54e02351 100644 --- a/packages/snaps-utils/src/test-utils/index.ts +++ b/packages/snaps-utils/src/test-utils/index.ts @@ -10,3 +10,4 @@ export * from './sleep'; export * from './snap'; export * from './spy'; export * from './stream'; +export * from './controller'; diff --git a/yarn.lock b/yarn.lock index c9f457b738..6504197d8a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2914,16 +2914,18 @@ __metadata: languageName: node linkType: hard -"@metamask/controller-utils@npm:^3.0.0": - version: 3.0.0 - resolution: "@metamask/controller-utils@npm:3.0.0" +"@metamask/controller-utils@npm:^3.0.0, @metamask/controller-utils@npm:^3.1.0": + version: 3.1.0 + resolution: "@metamask/controller-utils@npm:3.1.0" dependencies: + "@metamask/utils": ^3.3.1 + "@spruceid/siwe-parser": 1.1.3 eth-ens-namehash: ^2.0.8 eth-rpc-errors: ^4.0.0 ethereumjs-util: ^7.0.10 ethjs-unit: ^0.1.6 fast-deep-equal: ^3.1.3 - checksum: 44227aa9f716f86373a1a4fb86b7ae1c51199dd819f30a3a310a9f87838b7e11c1a3bb024572253bd3cb0258281596cfab8fbf317c3fe90962fa6cf426aa6858 + checksum: 811fc4b9da98ca406a0e002c87933687e745d20f802305bb2af0affcdad454189c705caae9389444da1f5f88f2d10269b3ae8354aa1f600a11ddb9315cfa5718 languageName: node linkType: hard @@ -3037,13 +3039,13 @@ __metadata: languageName: node linkType: hard -"@metamask/permission-controller@npm:^3.0.0": - version: 3.0.0 - resolution: "@metamask/permission-controller@npm:3.0.0" +"@metamask/permission-controller@npm:^3.0.0, @metamask/permission-controller@npm:^3.1.0": + version: 3.1.0 + resolution: "@metamask/permission-controller@npm:3.1.0" dependencies: "@metamask/approval-controller": ^2.0.0 "@metamask/base-controller": ^2.0.0 - "@metamask/controller-utils": ^3.0.0 + "@metamask/controller-utils": ^3.1.0 "@metamask/types": ^1.1.0 "@types/deep-freeze-strict": ^1.1.0 deep-freeze-strict: ^1.1.1 @@ -3053,7 +3055,7 @@ __metadata: nanoid: ^3.1.31 peerDependencies: "@metamask/approval-controller": ^2.0.0 - checksum: 67e104d21b3f0258863ecaabd11cb587f10dbcc91ff9b081d8b9569d163d6d54dd5b8fb267d5416ab8e41c8c48b5103e92e013fd1916d492b72706cda2474962 + checksum: 91c615e2faf9c1ce16371602e99354178d79a0af0697f1c3c81c860c2e9972f23e8189290cd41423c3df3048906bbf55c01fef8326677f2ea5cbcfdfb6a58db6 languageName: node linkType: hard @@ -3099,7 +3101,7 @@ __metadata: "@metamask/eslint-config-nodejs": ^11.0.1 "@metamask/eslint-config-typescript": ^11.0.0 "@metamask/key-tree": ^7.0.0 - "@metamask/permission-controller": ^3.0.0 + "@metamask/permission-controller": ^3.1.0 "@metamask/snaps-ui": ^0.31.0 "@metamask/snaps-utils": ^0.31.0 "@metamask/types": ^1.1.0 @@ -3264,7 +3266,7 @@ __metadata: "@metamask/eslint-config-nodejs": ^11.0.1 "@metamask/eslint-config-typescript": ^11.0.0 "@metamask/object-multiplex": ^1.1.0 - "@metamask/permission-controller": ^3.0.0 + "@metamask/permission-controller": ^3.1.0 "@metamask/post-message-stream": ^6.1.1 "@metamask/rpc-methods": ^0.31.0 "@metamask/snaps-execution-environments": ^0.31.0 @@ -3534,12 +3536,13 @@ __metadata: "@esbuild-plugins/node-modules-polyfill": ^0.2.2 "@lavamoat/allow-scripts": ^2.0.3 "@metamask/auto-changelog": ^3.1.0 + "@metamask/base-controller": ^2.0.0 "@metamask/eslint-config": ^11.0.0 "@metamask/eslint-config-jest": ^11.0.0 "@metamask/eslint-config-nodejs": ^11.0.1 "@metamask/eslint-config-typescript": ^11.0.0 "@metamask/key-tree": ^7.0.0 - "@metamask/permission-controller": ^3.0.0 + "@metamask/permission-controller": ^3.1.0 "@metamask/post-message-stream": ^6.1.1 "@metamask/providers": ^10.2.1 "@metamask/snaps-registry": ^1.2.0 @@ -3660,15 +3663,15 @@ __metadata: languageName: node linkType: hard -"@metamask/utils@npm:^3.3.0, @metamask/utils@npm:^3.4.1": - version: 3.4.1 - resolution: "@metamask/utils@npm:3.4.1" +"@metamask/utils@npm:^3.3.0, @metamask/utils@npm:^3.3.1, @metamask/utils@npm:^3.4.1": + version: 3.6.0 + resolution: "@metamask/utils@npm:3.6.0" dependencies: "@types/debug": ^4.1.7 debug: ^4.3.4 semver: ^7.3.8 superstruct: ^1.0.3 - checksum: 0799cefc17effecba4b4cd34879113f9f826a7aff4d21bfdcca64ef31c117be3e6a30cdd49c0b91289f22efbf7e56901322f4ce1b4d638dd2fc3bc3e81e3c87d + checksum: 1ebc6677bb017e4d09d4af143621fe27194d8ed815234cfd76469c3c734dc1db2ea7b577c01a2096c21c04d8c9c4d721d3035b5353fe2ded3b4737f326755e43 languageName: node linkType: hard @@ -3974,6 +3977,15 @@ __metadata: languageName: node linkType: hard +"@spruceid/siwe-parser@npm:1.1.3": + version: 1.1.3 + resolution: "@spruceid/siwe-parser@npm:1.1.3" + dependencies: + apg-js: ^4.1.1 + checksum: 708786ba2f10987c45c1fd8a6243ba6572ee7f320531616d71ff66044828bc24af66f5537ce09c9272bdae93fcc35b566a7804fe7997284f2ee5445a36e6add2 + languageName: node + linkType: hard + "@swc/core-darwin-arm64@npm:1.3.36": version: 1.3.36 resolution: "@swc/core-darwin-arm64@npm:1.3.36" @@ -5656,6 +5668,13 @@ __metadata: languageName: node linkType: hard +"apg-js@npm:^4.1.1": + version: 4.1.3 + resolution: "apg-js@npm:4.1.3" + checksum: fa815838fc3c4b2fa5801419d050c5cf0ac8b5dbbc7476a9c9b8e1ae5fc78feccf01ff3ff52ef1e80c72f8b7bf39f589f1b8aaad5f5aeba399523a9ba18da127 + languageName: node + linkType: hard + "append-buffer@npm:^1.0.2": version: 1.0.2 resolution: "append-buffer@npm:1.0.2"