From d28ee8eea6beec08128fd5733c6e7a18ebeb7062 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Thu, 31 Oct 2024 12:36:23 +0100 Subject: [PATCH 01/11] Move `snap_manageAccounts` to a gated permitted method --- .../snaps-rpc-methods/src/permissions.test.ts | 9 - .../src/permitted/handlers.ts | 2 + .../snaps-rpc-methods/src/permitted/index.ts | 4 +- .../src/permitted/manageAccounts.test.ts | 208 ++++++++++++++++++ .../src/permitted/manageAccounts.ts | 128 +++++++++++ .../snaps-rpc-methods/src/restricted/index.ts | 4 - .../src/restricted/manageAccounts.test.ts | 158 ------------- .../src/restricted/manageAccounts.ts | 119 ---------- .../src/types/methods/manage-accounts.ts | 12 +- .../snaps-utils/src/manifest/validation.ts | 1 - 10 files changed, 345 insertions(+), 300 deletions(-) create mode 100644 packages/snaps-rpc-methods/src/permitted/manageAccounts.test.ts create mode 100644 packages/snaps-rpc-methods/src/permitted/manageAccounts.ts delete mode 100644 packages/snaps-rpc-methods/src/restricted/manageAccounts.test.ts delete mode 100644 packages/snaps-rpc-methods/src/restricted/manageAccounts.ts diff --git a/packages/snaps-rpc-methods/src/permissions.test.ts b/packages/snaps-rpc-methods/src/permissions.test.ts index 50f911b100..5f1c57d907 100644 --- a/packages/snaps-rpc-methods/src/permissions.test.ts +++ b/packages/snaps-rpc-methods/src/permissions.test.ts @@ -211,15 +211,6 @@ describe('buildSnapRestrictedMethodSpecifications', () => { ], "targetName": "snap_getPreferences", }, - "snap_manageAccounts": { - "allowedCaveats": null, - "methodImplementation": [Function], - "permissionType": "RestrictedMethod", - "subjectTypes": [ - "snap", - ], - "targetName": "snap_manageAccounts", - }, "snap_manageState": { "allowedCaveats": null, "methodImplementation": [Function], diff --git a/packages/snaps-rpc-methods/src/permitted/handlers.ts b/packages/snaps-rpc-methods/src/permitted/handlers.ts index 83730b1a15..32cca4ff6f 100644 --- a/packages/snaps-rpc-methods/src/permitted/handlers.ts +++ b/packages/snaps-rpc-methods/src/permitted/handlers.ts @@ -9,6 +9,7 @@ import { getInterfaceStateHandler } from './getInterfaceState'; import { getSnapsHandler } from './getSnaps'; import { invokeKeyringHandler } from './invokeKeyring'; import { invokeSnapSugarHandler } from './invokeSnapSugar'; +import { manageAccountsHandler } from './manageAccounts'; import { requestSnapsHandler } from './requestSnaps'; import { resolveInterfaceHandler } from './resolveInterface'; import { updateInterfaceHandler } from './updateInterface'; @@ -29,6 +30,7 @@ export const methodHandlers = { snap_resolveInterface: resolveInterfaceHandler, snap_getCurrencyRate: getCurrencyRateHandler, snap_experimentalProviderRequest: providerRequestHandler, + snap_manageAccounts: manageAccountsHandler, }; /* eslint-enable @typescript-eslint/naming-convention */ diff --git a/packages/snaps-rpc-methods/src/permitted/index.ts b/packages/snaps-rpc-methods/src/permitted/index.ts index 5aa676fce3..1b0293b9c4 100644 --- a/packages/snaps-rpc-methods/src/permitted/index.ts +++ b/packages/snaps-rpc-methods/src/permitted/index.ts @@ -5,6 +5,7 @@ import type { GetClientStatusHooks } from './getClientStatus'; import type { GetCurrencyRateMethodHooks } from './getCurrencyRate'; import type { GetInterfaceStateMethodHooks } from './getInterfaceState'; import type { GetSnapsHooks } from './getSnaps'; +import type { ManageAccountsMethodHooks } from './manageAccounts'; import type { RequestSnapsHooks } from './requestSnaps'; import type { ResolveInterfaceMethodHooks } from './resolveInterface'; import type { UpdateInterfaceMethodHooks } from './updateInterface'; @@ -18,7 +19,8 @@ export type PermittedRpcMethodHooks = GetAllSnapsHooks & GetInterfaceStateMethodHooks & ResolveInterfaceMethodHooks & GetCurrencyRateMethodHooks & - ProviderRequestMethodHooks; + ProviderRequestMethodHooks & + ManageAccountsMethodHooks; export * from './handlers'; export * from './middleware'; diff --git a/packages/snaps-rpc-methods/src/permitted/manageAccounts.test.ts b/packages/snaps-rpc-methods/src/permitted/manageAccounts.test.ts new file mode 100644 index 0000000000..e4774c4362 --- /dev/null +++ b/packages/snaps-rpc-methods/src/permitted/manageAccounts.test.ts @@ -0,0 +1,208 @@ +import { JsonRpcEngine } from '@metamask/json-rpc-engine'; +import { rpcErrors } from '@metamask/rpc-errors'; +import type { ManageAccountsResult } from '@metamask/snaps-sdk'; +import type { + JsonRpcFailure, + JsonRpcRequest, + PendingJsonRpcResponse, +} from '@metamask/utils'; + +import type { ManageAccountsParameters } from './manageAccounts'; +import { manageAccountsHandler } from './manageAccounts'; + +describe('snap_manageAccounts', () => { + describe('manageAccountsHandler', () => { + it('has the expected shape', () => { + expect(manageAccountsHandler).toMatchObject({ + methodNames: ['snap_manageAccounts'], + implementation: expect.any(Function), + hookNames: { + hasPermission: true, + handleKeyringSnapMessage: true, + }, + }); + }); + }); + + describe('implementation', () => { + it('returns the result from the `handleKeyringSnapMessage` hook', async () => { + const { implementation } = manageAccountsHandler; + + const hasPermission = jest.fn().mockReturnValue(true); + const handleKeyringSnapMessage = jest.fn().mockReturnValue('foo'); + + const hooks = { + hasPermission, + handleKeyringSnapMessage, + }; + + const engine = new JsonRpcEngine(); + + engine.push((request, response, next, end) => { + const result = implementation( + request as JsonRpcRequest, + response as PendingJsonRpcResponse, + next, + end, + hooks, + ); + + result?.catch(end); + }); + + const response = await engine.handle({ + jsonrpc: '2.0', + id: 1, + method: 'snap_manageAccounts', + params: { + method: 'foo', + params: { bar: 'baz' }, + }, + }); + + expect(handleKeyringSnapMessage).toHaveBeenCalledWith({ + method: 'foo', + params: { bar: 'baz' }, + }); + + expect(response).toStrictEqual({ jsonrpc: '2.0', id: 1, result: 'foo' }); + }); + + it('throws an error if the snap does not have permission', async () => { + const { implementation } = manageAccountsHandler; + + const hasPermission = jest.fn().mockReturnValue(false); + const handleKeyringSnapMessage = jest.fn().mockReturnValue('foo'); + + const hooks = { + hasPermission, + handleKeyringSnapMessage, + }; + + const engine = new JsonRpcEngine(); + + engine.push((request, response, next, end) => { + const result = implementation( + request as JsonRpcRequest, + response as PendingJsonRpcResponse, + next, + end, + hooks, + ); + + result?.catch(end); + }); + + const response = (await engine.handle({ + jsonrpc: '2.0', + id: 1, + method: 'snap_manageAccounts', + params: { + method: 'foo', + params: { bar: 'baz' }, + }, + })) as JsonRpcFailure; + + expect(response.error).toStrictEqual({ + ...rpcErrors.methodNotFound().serialize(), + stack: expect.any(String), + }); + }); + + it('throws an error if the `handleKeyringSnapMessage` hook throws', async () => { + const { implementation } = manageAccountsHandler; + + const hasPermission = jest.fn().mockReturnValue(true); + const handleKeyringSnapMessage = jest + .fn() + .mockRejectedValue(new Error('foo')); + + const hooks = { + hasPermission, + handleKeyringSnapMessage, + }; + + const engine = new JsonRpcEngine(); + + engine.push((request, response, next, end) => { + const result = implementation( + request as JsonRpcRequest, + response as PendingJsonRpcResponse, + next, + end, + hooks, + ); + + result?.catch(end); + }); + + const response = (await engine.handle({ + jsonrpc: '2.0', + id: 1, + method: 'snap_manageAccounts', + params: { + method: 'foo', + params: { bar: 'baz' }, + }, + })) as JsonRpcFailure; + + expect(response.error).toStrictEqual({ + code: -32603, + message: 'foo', + data: { + cause: { + message: 'foo', + stack: expect.any(String), + }, + }, + }); + }); + + it('throws on invalid params', async () => { + const { implementation } = manageAccountsHandler; + + const hasPermission = jest.fn().mockReturnValue(true); + const handleKeyringSnapMessage = jest.fn().mockReturnValue('foo'); + + const hooks = { + hasPermission, + handleKeyringSnapMessage, + }; + + const engine = new JsonRpcEngine(); + + engine.push((request, response, next, end) => { + const result = implementation( + request as JsonRpcRequest, + response as PendingJsonRpcResponse, + next, + end, + hooks, + ); + + result?.catch(end); + }); + + const response = await engine.handle({ + jsonrpc: '2.0', + id: 1, + method: 'snap_manageAccounts', + params: { + method: 'foo', + params: 42, + }, + }); + + expect(response).toStrictEqual({ + jsonrpc: '2.0', + id: 1, + error: { + code: -32602, + message: + 'Invalid params: At path: params -- Expected the value to satisfy a union of `array | record`, but received: 42.', + stack: expect.any(String), + }, + }); + }); + }); +}); diff --git a/packages/snaps-rpc-methods/src/permitted/manageAccounts.ts b/packages/snaps-rpc-methods/src/permitted/manageAccounts.ts new file mode 100644 index 0000000000..ea2be888a3 --- /dev/null +++ b/packages/snaps-rpc-methods/src/permitted/manageAccounts.ts @@ -0,0 +1,128 @@ +import type { JsonRpcEngineEndCallback } from '@metamask/json-rpc-engine'; +import type { PermittedHandlerExport } from '@metamask/permission-controller'; +import { rpcErrors } from '@metamask/rpc-errors'; +import type { + ManageAccountsParams, + ManageAccountsResult, +} from '@metamask/snaps-sdk'; +import { type InferMatching } from '@metamask/snaps-utils'; +import { + array, + create, + object, + optional, + record, + string, + StructError, + union, +} from '@metamask/superstruct'; +import type { + Json, + JsonRpcRequest, + PendingJsonRpcResponse, +} from '@metamask/utils'; +import { JsonStruct } from '@metamask/utils'; + +import { SnapEndowments } from '../endowments'; +import type { MethodHooksObject } from '../utils'; + +const hookNames: MethodHooksObject = { + hasPermission: true, + handleKeyringSnapMessage: true, +}; + +export type ManageAccountsMethodHooks = { + /** + * Checks if the current snap has a permission. + * + * @param permissionName - The name of the permission. + * @returns Whether the snap has the permission. + */ + hasPermission: (permissionName: string) => boolean; + /** + * Handles the keyring snap message. + * + * @returns The snap keyring message result. + */ + handleKeyringSnapMessage: ( + message: ManageAccountsParameters, + ) => Promise; +}; + +export const manageAccountsHandler: PermittedHandlerExport< + ManageAccountsMethodHooks, + ManageAccountsParams, + ManageAccountsResult +> = { + methodNames: ['snap_manageAccounts'], + implementation: getManageAccountsImplementation, + hookNames, +}; + +const ManageAccountsParametersStruct = object({ + method: string(), + params: optional(union([array(JsonStruct), record(string(), JsonStruct)])), +}); + +export type ManageAccountsParameters = InferMatching< + typeof ManageAccountsParametersStruct, + ManageAccountsParams +>; + +/** + * The `snap_manageAccounts` method implementation. + * + * @param req - The JSON-RPC request object. + * @param res - The JSON-RPC response object. + * @param _next - The `json-rpc-engine` "next" callback. Not used by this + * function. + * @param end - The `json-rpc-engine` "end" callback. + * @param hooks - The RPC method hooks. + * @param hooks.hasPermission - The function to check if the snap has a permission. + * @param hooks.handleKeyringSnapMessage - The function to handle the keyring snap message. + * @returns Nothing. + */ +async function getManageAccountsImplementation( + req: JsonRpcRequest, + res: PendingJsonRpcResponse, + _next: unknown, + end: JsonRpcEngineEndCallback, + { hasPermission, handleKeyringSnapMessage }: ManageAccountsMethodHooks, +): Promise { + if (!hasPermission(SnapEndowments.Keyring)) { + return end(rpcErrors.methodNotFound()); + } + + const { params } = req; + + try { + const validatedParams = getValidatedParams(params); + + res.result = await handleKeyringSnapMessage(validatedParams); + } catch (error) { + return end(error); + } + + return end(); +} + +/** + * Validate the manageAccounts method `params` and returns them cast to the correct + * type. Throws if validation fails. + * + * @param params - The unvalidated params object from the method request. + * @returns The validated manageAccounts method parameter object. + */ +function getValidatedParams(params: unknown): ManageAccountsParameters { + try { + return create(params, ManageAccountsParametersStruct); + } catch (error) { + if (error instanceof StructError) { + throw rpcErrors.invalidParams({ + message: `Invalid params: ${error.message}.`, + }); + } + /* istanbul ignore next */ + throw rpcErrors.internal(); + } +} diff --git a/packages/snaps-rpc-methods/src/restricted/index.ts b/packages/snaps-rpc-methods/src/restricted/index.ts index 93722da6d8..bc2c2c2f99 100644 --- a/packages/snaps-rpc-methods/src/restricted/index.ts +++ b/packages/snaps-rpc-methods/src/restricted/index.ts @@ -14,8 +14,6 @@ import type { GetPreferencesMethodHooks } from './getPreferences'; import { getPreferencesBuilder } from './getPreferences'; import type { InvokeSnapMethodHooks } from './invokeSnap'; import { invokeSnapBuilder } from './invokeSnap'; -import type { ManageAccountsMethodHooks } from './manageAccounts'; -import { manageAccountsBuilder } from './manageAccounts'; import type { ManageStateMethodHooks } from './manageState'; import { manageStateBuilder } from './manageState'; import type { NotifyMethodHooks } from './notify'; @@ -32,7 +30,6 @@ export type RestrictedMethodHooks = DialogMethodHooks & InvokeSnapMethodHooks & ManageStateMethodHooks & NotifyMethodHooks & - ManageAccountsMethodHooks & GetLocaleMethodHooks & GetPreferencesMethodHooks; @@ -45,7 +42,6 @@ export const restrictedMethodPermissionBuilders = { [invokeSnapBuilder.targetName]: invokeSnapBuilder, [manageStateBuilder.targetName]: manageStateBuilder, [notifyBuilder.targetName]: notifyBuilder, - [manageAccountsBuilder.targetName]: manageAccountsBuilder, [getLocaleBuilder.targetName]: getLocaleBuilder, [getPreferencesBuilder.targetName]: getPreferencesBuilder, } as const; diff --git a/packages/snaps-rpc-methods/src/restricted/manageAccounts.test.ts b/packages/snaps-rpc-methods/src/restricted/manageAccounts.test.ts deleted file mode 100644 index 44503f44a3..0000000000 --- a/packages/snaps-rpc-methods/src/restricted/manageAccounts.test.ts +++ /dev/null @@ -1,158 +0,0 @@ -import { SubjectType, PermissionType } from '@metamask/permission-controller'; -import { MOCK_SNAP_ID } from '@metamask/snaps-utils/test-utils'; - -import { - methodName, - manageAccountsBuilder, - manageAccountsImplementation, - specificationBuilder, -} from './manageAccounts'; - -// To Do: -// Move the class SnapKeyring to it's own module -// add mock the method in this test instead of the entire class -class SnapKeyringMock { - static type = 'Snap Keyring'; - - accounts: string[] = []; - - handleKeyringSnapMessage = async ( - _origin: string, - _params: any, - ): Promise => { - return true; - }; -} - -describe('specification', () => { - it('builds specification', () => { - const methodHooks = { - getSnapKeyring: jest.fn(), - }; - - expect( - specificationBuilder({ - allowedCaveats: null, - methodHooks, - }), - ).toStrictEqual({ - allowedCaveats: null, - methodImplementation: expect.anything(), - permissionType: PermissionType.RestrictedMethod, - targetName: methodName, - subjectTypes: [SubjectType.Snap], - }); - }); -}); - -describe('builder', () => { - it('has the expected shape', () => { - expect(manageAccountsBuilder).toMatchObject({ - targetName: methodName, - specificationBuilder: expect.any(Function), - methodHooks: { - getSnapKeyring: true, - }, - }); - }); - - it('builder outputs expected specification', () => { - expect( - manageAccountsBuilder.specificationBuilder({ - methodHooks: { - getSnapKeyring: jest.fn(), - }, - }), - ).toMatchObject({ - permissionType: PermissionType.RestrictedMethod, - targetName: methodName, - allowedCaveats: null, - methodImplementation: expect.any(Function), - }); - }); -}); - -describe('manageAccountsImplementation', () => { - const MOCK_CAIP_10_ACCOUNT = - 'eip155:1:0xab16a96D359eC26a11e2C2b3d8f8B8942d5Bfcdb'; - - afterEach(() => { - jest.clearAllMocks(); - }); - - it('should throw params are not set', async () => { - const mockKeyring = new SnapKeyringMock(); - const getSnapKeyring = jest.fn().mockResolvedValue(mockKeyring); - - const manageAccounts = manageAccountsImplementation({ - getSnapKeyring, - }); - - await expect( - manageAccounts({ - method: 'snap_manageAccounts', - context: { - origin: MOCK_SNAP_ID, - }, - // @ts-expect-error Error expected. - params: {}, - }), - ).rejects.toThrow( - 'Expected the value to satisfy a union of `object | object`, but received: [object Object]', - ); - }); - - it('should throw params accountId is not set', async () => { - const mockKeyring = new SnapKeyringMock(); - const getSnapKeyring = jest.fn().mockResolvedValue(mockKeyring); - - const manageAccounts = manageAccountsImplementation({ - getSnapKeyring, - }); - - await expect( - manageAccounts({ - method: 'snap_manageAccounts', - context: { - origin: MOCK_SNAP_ID, - }, - // @ts-expect-error Error expected. - params: { method: 123, params: {} }, - }), - ).rejects.toThrow( - 'Expected the value to satisfy a union of `object | object`, but received: [object Object]', - ); - }); - - it('should route request to snap keyring', async () => { - const mockKeyring = new SnapKeyringMock(); - const getSnapKeyring = jest.fn().mockResolvedValue(mockKeyring); - - const createAccountSpy = jest - .spyOn(mockKeyring, 'handleKeyringSnapMessage') - .mockResolvedValue(true); - - const manageAccounts = manageAccountsImplementation({ - getSnapKeyring, - }); - - const requestResponse = await manageAccounts({ - method: 'snap_manageAccounts', - context: { - origin: MOCK_SNAP_ID, - }, - params: { - method: 'deleteAccount', - params: { accountId: MOCK_CAIP_10_ACCOUNT }, - }, - }); - - expect(createAccountSpy).toHaveBeenCalledTimes(1); - expect(createAccountSpy).toHaveBeenCalledWith(MOCK_SNAP_ID, { - method: 'deleteAccount', - params: { accountId: MOCK_CAIP_10_ACCOUNT }, - }); - expect(requestResponse).toBe(true); - createAccountSpy.mockClear(); - }); -}); diff --git a/packages/snaps-rpc-methods/src/restricted/manageAccounts.ts b/packages/snaps-rpc-methods/src/restricted/manageAccounts.ts deleted file mode 100644 index 220fa25ba2..0000000000 --- a/packages/snaps-rpc-methods/src/restricted/manageAccounts.ts +++ /dev/null @@ -1,119 +0,0 @@ -import type { - RestrictedMethodOptions, - ValidPermissionSpecification, - PermissionSpecificationBuilder, -} from '@metamask/permission-controller'; -import { SubjectType, PermissionType } from '@metamask/permission-controller'; -import type { - ManageAccountsParams, - ManageAccountsResult, -} from '@metamask/snaps-sdk'; -import type { InferMatching } from '@metamask/snaps-utils'; -import { - assert, - string, - object, - union, - array, - record, -} from '@metamask/superstruct'; -import type { Json, NonEmptyArray } from '@metamask/utils'; -import { JsonStruct } from '@metamask/utils'; - -const SnapMessageStruct = union([ - object({ - method: string(), - }), - object({ - method: string(), - params: union([array(JsonStruct), record(string(), JsonStruct)]), - }), -]); - -type Message = InferMatching; - -export const methodName = 'snap_manageAccounts'; - -export type ManageAccountsMethodHooks = { - /** - * Gets the snap keyring implementation. - */ - getSnapKeyring: (snapOrigin: string) => Promise<{ - handleKeyringSnapMessage: ( - snapId: string, - message: Message, - ) => Promise; - }>; -}; - -type ManageAccountsSpecificationBuilderOptions = { - allowedCaveats?: Readonly> | null; - methodHooks: ManageAccountsMethodHooks; -}; - -type ManageAccountsSpecification = ValidPermissionSpecification<{ - permissionType: PermissionType.RestrictedMethod; - targetName: typeof methodName; - methodImplementation: ReturnType; - allowedCaveats: Readonly> | null; -}>; - -/** - * The specification builder for the `snap_manageAccounts` permission. - * `snap_manageAccounts` lets the Snap manage a set of accounts via a custom keyring. - * - * @param options - The specification builder options. - * @param options.allowedCaveats - The optional allowed caveats for the permission. - * @param options.methodHooks - The RPC method hooks needed by the method implementation. - * @returns The specification for the `snap_manageAccounts` permission. - */ -export const specificationBuilder: PermissionSpecificationBuilder< - PermissionType.RestrictedMethod, - ManageAccountsSpecificationBuilderOptions, - ManageAccountsSpecification -> = ({ - allowedCaveats = null, - methodHooks, -}: ManageAccountsSpecificationBuilderOptions) => { - return { - permissionType: PermissionType.RestrictedMethod, - targetName: methodName, - allowedCaveats, - methodImplementation: manageAccountsImplementation(methodHooks), - subjectTypes: [SubjectType.Snap], - }; -}; - -/** - * Builds the method implementation for `snap_manageAccounts`. - * - * @param hooks - The RPC method hooks. - * @param hooks.getSnapKeyring - A function to get the snap keyring. - * @returns The method implementation which either returns `null` for a - * successful state update/deletion or returns the decrypted state. - * @throws If the params are invalid. - */ -export function manageAccountsImplementation({ - getSnapKeyring, -}: ManageAccountsMethodHooks) { - return async function manageAccounts( - options: RestrictedMethodOptions, - ): Promise { - const { - context: { origin }, - params, - } = options; - - assert(params, SnapMessageStruct); - const keyring = await getSnapKeyring(origin); - return await keyring.handleKeyringSnapMessage(origin, params); - }; -} - -export const manageAccountsBuilder = Object.freeze({ - targetName: methodName, - specificationBuilder, - methodHooks: { - getSnapKeyring: true, - }, -} as const); diff --git a/packages/snaps-sdk/src/types/methods/manage-accounts.ts b/packages/snaps-sdk/src/types/methods/manage-accounts.ts index 192fe683df..948f9ece4a 100644 --- a/packages/snaps-sdk/src/types/methods/manage-accounts.ts +++ b/packages/snaps-sdk/src/types/methods/manage-accounts.ts @@ -6,14 +6,10 @@ import type { Json } from '@metamask/utils'; * @property method - The method to call on the Snap. * @property params - The optional parameters to pass to the Snap method. */ -export type ManageAccountsParams = - | { - method: string; - } - | { - method: string; - params: Json[] | Record; - }; +export type ManageAccountsParams = { + method: string; + params?: Json[] | Record; +}; /** * The result returned by the `snap_manageAccounts` method, which is the result diff --git a/packages/snaps-utils/src/manifest/validation.ts b/packages/snaps-utils/src/manifest/validation.ts index dd65132a10..2e312a3a9f 100644 --- a/packages/snaps-utils/src/manifest/validation.ts +++ b/packages/snaps-utils/src/manifest/validation.ts @@ -232,7 +232,6 @@ export const PermissionsStruct: Describe = type({ 'endowment:webassembly': optional(EmptyObjectStruct), snap_dialog: optional(EmptyObjectStruct), snap_manageState: optional(EmptyObjectStruct), - snap_manageAccounts: optional(EmptyObjectStruct), snap_notify: optional(EmptyObjectStruct), snap_getBip32Entropy: optional(SnapGetBip32EntropyPermissionsStruct), snap_getBip32PublicKey: optional(SnapGetBip32EntropyPermissionsStruct), From 709b3479c25a2b9078fc60638b367d67ec345595 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Thu, 31 Oct 2024 14:19:18 +0100 Subject: [PATCH 02/11] Fix simulation test --- .../snaps-simulation/src/methods/specifications.test.ts | 9 --------- 1 file changed, 9 deletions(-) diff --git a/packages/snaps-simulation/src/methods/specifications.test.ts b/packages/snaps-simulation/src/methods/specifications.test.ts index 487eda7235..f2e6b3afb3 100644 --- a/packages/snaps-simulation/src/methods/specifications.test.ts +++ b/packages/snaps-simulation/src/methods/specifications.test.ts @@ -242,15 +242,6 @@ describe('getPermissionSpecifications', () => { ], "targetName": "snap_getPreferences", }, - "snap_manageAccounts": { - "allowedCaveats": null, - "methodImplementation": [Function], - "permissionType": "RestrictedMethod", - "subjectTypes": [ - "snap", - ], - "targetName": "snap_manageAccounts", - }, "snap_manageState": { "allowedCaveats": null, "methodImplementation": [Function], From a848a7ee1249eda7e0ea51ae1c919efefb19be03 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Mon, 4 Nov 2024 12:52:01 +0100 Subject: [PATCH 03/11] Filter removed permissions --- packages/snaps-rpc-methods/src/permissions.ts | 51 ++++++++++++++----- 1 file changed, 38 insertions(+), 13 deletions(-) diff --git a/packages/snaps-rpc-methods/src/permissions.ts b/packages/snaps-rpc-methods/src/permissions.ts index 6c0e6d43dc..1b97bc78c2 100644 --- a/packages/snaps-rpc-methods/src/permissions.ts +++ b/packages/snaps-rpc-methods/src/permissions.ts @@ -15,6 +15,21 @@ import { } from './restricted'; import { selectHooks } from './utils'; +const REMOVED_PERMISSIONS = Object.freeze(['snap_manageAccounts']); + +/** + * Filters out permissions that have been removed from the Snap API. + * + * @param initialPermission - The initial permission to filter. + * @returns Whether the permission has been removed. + */ +export const filterRemovedPermissions = ( + initialPermission: [string, unknown], +) => { + const [value] = initialPermission; + return REMOVED_PERMISSIONS.some((permission) => permission === value); +}; + /** * Map initial permissions as defined in a Snap manifest to something that can * be processed by the PermissionsController. Each caveat mapping function @@ -30,22 +45,32 @@ export function processSnapPermissions( initialPermissions: SnapPermissions, ): Record> { return Object.fromEntries( - Object.entries(initialPermissions).map(([initialPermission, value]) => { - if (hasProperty(caveatMappers, initialPermission)) { - return [initialPermission, caveatMappers[initialPermission](value)]; - } else if (hasProperty(endowmentCaveatMappers, initialPermission)) { + Object.entries(initialPermissions) + .filter(filterRemovedPermissions) + .map(([initialPermission, value]) => { + if ( + REMOVED_PERMISSIONS.some( + (permission) => permission === initialPermission, + ) + ) { + return []; + } + + if (hasProperty(caveatMappers, initialPermission)) { + return [initialPermission, caveatMappers[initialPermission](value)]; + } else if (hasProperty(endowmentCaveatMappers, initialPermission)) { + return [ + initialPermission, + endowmentCaveatMappers[initialPermission](value), + ]; + } + + // If we have no mapping, this may be a non-snap permission, return as-is return [ initialPermission, - endowmentCaveatMappers[initialPermission](value), + value as Pick, ]; - } - - // If we have no mapping, this may be a non-snap permission, return as-is - return [ - initialPermission, - value as Pick, - ]; - }), + }), ); } From ddb532ade60b0e88fdbf57ddd15aa23f74e16ea5 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Wed, 6 Nov 2024 11:56:53 +0100 Subject: [PATCH 04/11] Add test --- .../src/snaps/SnapController.test.tsx | 38 ++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.tsx b/packages/snaps-controllers/src/snaps/SnapController.test.tsx index 2653ec545b..d2442f9aec 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.tsx +++ b/packages/snaps-controllers/src/snaps/SnapController.test.tsx @@ -18,7 +18,7 @@ import { handlerEndowments, SnapEndowments, } from '@metamask/snaps-rpc-methods'; -import type { SnapId } from '@metamask/snaps-sdk'; +import type { Snap, SnapId } from '@metamask/snaps-sdk'; import { AuxiliaryFileEncoding, text } from '@metamask/snaps-sdk'; import { Text } from '@metamask/snaps-sdk/jsx'; import type { SnapPermissions, RpcOrigins } from '@metamask/snaps-utils'; @@ -1011,6 +1011,42 @@ describe('SnapController', () => { snapController.destroy(); }); + it('filters out removed permissions', async () => { + const messenger = getSnapControllerMessenger(); + const initialPermissions: SnapPermissions = { + [handlerEndowments.onRpcRequest as string]: { snaps: false, dapps: true }, + // eslint-disable-next-line @typescript-eslint/naming-convention + snap_manageAccounts: {}, + }; + + const { manifest } = await getMockSnapFilesWithUpdatedChecksum({ + manifest: getSnapManifest({ + initialPermissions, + }), + }); + + const snapController = getSnapController( + getSnapControllerOptions({ + messenger, + detectSnapLocation: loopbackDetect({ + manifest: manifest.result, + }), + }), + ); + + const snap = await snapController.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: {}, + }); + + const permissions = (snap[MOCK_SNAP_ID] as Snap).initialPermissions; + + expect(permissions).toStrictEqual({ + [handlerEndowments.onRpcRequest as string]: { snaps: false, dapps: true }, + }); + + snapController.destroy(); + }); + it('throws an error if the installation is disabled during installSnaps', async () => { const controller = getSnapController( getSnapControllerOptions({ From c2dca1aa68eaf3d5c2f1930ef441e24ae46e0f51 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Wed, 6 Nov 2024 12:15:57 +0100 Subject: [PATCH 05/11] forgotten file --- packages/snaps-rpc-methods/src/permissions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snaps-rpc-methods/src/permissions.ts b/packages/snaps-rpc-methods/src/permissions.ts index 1b97bc78c2..c43641da6b 100644 --- a/packages/snaps-rpc-methods/src/permissions.ts +++ b/packages/snaps-rpc-methods/src/permissions.ts @@ -27,7 +27,7 @@ export const filterRemovedPermissions = ( initialPermission: [string, unknown], ) => { const [value] = initialPermission; - return REMOVED_PERMISSIONS.some((permission) => permission === value); + return !REMOVED_PERMISSIONS.some((permission) => permission === value); }; /** From 6df462c8112b382bf9c2b59c5645e8ae3a9abe22 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Fri, 15 Nov 2024 15:09:55 +0100 Subject: [PATCH 06/11] [WIP] Test --- .../src/snaps/SnapController.test.tsx | 29 +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.tsx b/packages/snaps-controllers/src/snaps/SnapController.test.tsx index d2442f9aec..d3ee96dda8 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.tsx +++ b/packages/snaps-controllers/src/snaps/SnapController.test.tsx @@ -1034,15 +1034,32 @@ describe('SnapController', () => { }), ); - const snap = await snapController.installSnaps(MOCK_ORIGIN, { + await snapController.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {}, }); - const permissions = (snap[MOCK_SNAP_ID] as Snap).initialPermissions; - - expect(permissions).toStrictEqual({ - [handlerEndowments.onRpcRequest as string]: { snaps: false, dapps: true }, - }); + expect(messenger.call).toHaveBeenNthCalledWith( + 5, + 'PermissionController:grantPermissions', + { + approvedPermissions: { + [SnapEndowments.Rpc]: { + caveats: [ + { type: 'rpcOrigin', value: { dapps: true, snaps: false } }, + ], + }, + }, + subject: { origin: MOCK_SNAP_ID }, + requestData: { + metadata: { + dappOrigin: MOCK_ORIGIN, + id: expect.any(String), + origin: MOCK_SNAP_ID, + }, + snapId: MOCK_SNAP_ID, + }, + }, + ); snapController.destroy(); }); From d7815709ce8597d74b7a2afc439ecee42022188a Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Fri, 22 Nov 2024 12:13:09 +0100 Subject: [PATCH 07/11] update coverage --- packages/snaps-rpc-methods/jest.config.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/snaps-rpc-methods/jest.config.js b/packages/snaps-rpc-methods/jest.config.js index c02c90e6d6..e314603800 100644 --- a/packages/snaps-rpc-methods/jest.config.js +++ b/packages/snaps-rpc-methods/jest.config.js @@ -10,10 +10,10 @@ module.exports = deepmerge(baseConfig, { ], coverageThreshold: { global: { - branches: 92.88, - functions: 97.26, - lines: 97.84, - statements: 97.36, + branches: 92.54, + functions: 95.67, + lines: 97.36, + statements: 96.72, }, }, }); From 0ab28e06aa05fbc7bb429ea0b9e8465eb8177cd2 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Fri, 22 Nov 2024 12:38:29 +0100 Subject: [PATCH 08/11] add tests --- packages/snaps-rpc-methods/jest.config.js | 8 +- .../snaps-rpc-methods/src/permissions.test.ts | 101 ++++++++++++++++++ packages/snaps-rpc-methods/src/permissions.ts | 8 -- 3 files changed, 105 insertions(+), 12 deletions(-) diff --git a/packages/snaps-rpc-methods/jest.config.js b/packages/snaps-rpc-methods/jest.config.js index e314603800..c4a5c7b740 100644 --- a/packages/snaps-rpc-methods/jest.config.js +++ b/packages/snaps-rpc-methods/jest.config.js @@ -10,10 +10,10 @@ module.exports = deepmerge(baseConfig, { ], coverageThreshold: { global: { - branches: 92.54, - functions: 95.67, - lines: 97.36, - statements: 96.72, + branches: 94.09, + functions: 98.36, + lines: 98.57, + statements: 98.06, }, }, }); diff --git a/packages/snaps-rpc-methods/src/permissions.test.ts b/packages/snaps-rpc-methods/src/permissions.test.ts index 5f1c57d907..56fc74f06d 100644 --- a/packages/snaps-rpc-methods/src/permissions.test.ts +++ b/packages/snaps-rpc-methods/src/permissions.test.ts @@ -1,8 +1,109 @@ +import type { Bip32Entropy } from '@metamask/snaps-sdk'; + import { buildSnapEndowmentSpecifications, buildSnapRestrictedMethodSpecifications, + filterRemovedPermissions, + processSnapPermissions, } from './permissions'; +describe('filterRemovedPermissions', () => { + it('returns true for a permission that is not removed', () => { + const result = filterRemovedPermissions(['snap_dialog', {}]); + expect(result).toBe(true); + }); + + it('returns false for a permission that is removed', () => { + const result = filterRemovedPermissions(['snap_manageAccounts', {}]); + expect(result).toBe(false); + }); +}); + +/* eslint-disable @typescript-eslint/naming-convention */ +describe('processSnapPermissions', () => { + it('returns the expected object', () => { + const permissions = { + snap_dialog: {}, + + snap_manageAccounts: {}, + }; + const result = processSnapPermissions(permissions); + expect(result).toStrictEqual({ + snap_dialog: {}, + }); + }); + + it('returns the expected object when the permission is not a snap permission', () => { + const permissions = { + snap_dialog: {}, + snap_manageAccounts: {}, + wallet_foobar: {}, + }; + const result = processSnapPermissions(permissions); + expect(result).toStrictEqual({ + snap_dialog: {}, + wallet_foobar: {}, + }); + }); + + it('returns the expected object when the permission is a snap endowment with a mapper', () => { + const permissions = { + snap_dialog: {}, + snap_manageAccounts: {}, + 'endowment:rpc': { + dapps: true, + snaps: true, + }, + }; + const result = processSnapPermissions(permissions); + expect(result).toStrictEqual({ + snap_dialog: {}, + 'endowment:rpc': { + caveats: [ + { + type: 'rpcOrigin', + value: { + dapps: true, + snaps: true, + }, + }, + ], + }, + }); + }); + + it('returns the expected object when the permission is a snap permission with a mapper', () => { + const permissions = { + snap_dialog: {}, + snap_manageAccounts: {}, + snap_getBip32Entropy: [ + { + path: ['m', "44'", "3'"], + curve: 'secp256k1', + } as Bip32Entropy, + ], + }; + const result = processSnapPermissions(permissions); + expect(result).toStrictEqual({ + snap_dialog: {}, + snap_getBip32Entropy: { + caveats: [ + { + type: 'permittedDerivationPaths', + value: [ + { + path: ['m', "44'", "3'"], + curve: 'secp256k1', + }, + ], + }, + ], + }, + }); + }); +}); +/* eslint-enable @typescript-eslint/naming-convention */ + describe('buildSnapEndowmentSpecifications', () => { it('returns the expected object', () => { const specifications = buildSnapEndowmentSpecifications([]); diff --git a/packages/snaps-rpc-methods/src/permissions.ts b/packages/snaps-rpc-methods/src/permissions.ts index c43641da6b..377b7b381a 100644 --- a/packages/snaps-rpc-methods/src/permissions.ts +++ b/packages/snaps-rpc-methods/src/permissions.ts @@ -48,14 +48,6 @@ export function processSnapPermissions( Object.entries(initialPermissions) .filter(filterRemovedPermissions) .map(([initialPermission, value]) => { - if ( - REMOVED_PERMISSIONS.some( - (permission) => permission === initialPermission, - ) - ) { - return []; - } - if (hasProperty(caveatMappers, initialPermission)) { return [initialPermission, caveatMappers[initialPermission](value)]; } else if (hasProperty(endowmentCaveatMappers, initialPermission)) { From 2d13c9e35a9e44b4a1ca34a54c20ec0b25656b30 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Fri, 22 Nov 2024 12:45:59 +0100 Subject: [PATCH 09/11] revert to previous params --- packages/snaps-rpc-methods/jest.config.js | 8 ++-- .../src/permitted/manageAccounts.test.ts | 43 ++++++++++++++++++- .../src/permitted/manageAccounts.ts | 26 ++++++++--- 3 files changed, 65 insertions(+), 12 deletions(-) diff --git a/packages/snaps-rpc-methods/jest.config.js b/packages/snaps-rpc-methods/jest.config.js index c4a5c7b740..f64fc7ba63 100644 --- a/packages/snaps-rpc-methods/jest.config.js +++ b/packages/snaps-rpc-methods/jest.config.js @@ -10,10 +10,10 @@ module.exports = deepmerge(baseConfig, { ], coverageThreshold: { global: { - branches: 94.09, - functions: 98.36, - lines: 98.57, - statements: 98.06, + branches: 94.11, + functions: 98.37, + lines: 98.58, + statements: 98.07, }, }, }); diff --git a/packages/snaps-rpc-methods/src/permitted/manageAccounts.test.ts b/packages/snaps-rpc-methods/src/permitted/manageAccounts.test.ts index e4774c4362..758647082a 100644 --- a/packages/snaps-rpc-methods/src/permitted/manageAccounts.test.ts +++ b/packages/snaps-rpc-methods/src/permitted/manageAccounts.test.ts @@ -25,7 +25,7 @@ describe('snap_manageAccounts', () => { }); describe('implementation', () => { - it('returns the result from the `handleKeyringSnapMessage` hook', async () => { + it('returns the result from the `handleKeyringSnapMessage` hook with params', async () => { const { implementation } = manageAccountsHandler; const hasPermission = jest.fn().mockReturnValue(true); @@ -68,6 +68,47 @@ describe('snap_manageAccounts', () => { expect(response).toStrictEqual({ jsonrpc: '2.0', id: 1, result: 'foo' }); }); + it('returns the result from the `handleKeyringSnapMessage` hook without params', async () => { + const { implementation } = manageAccountsHandler; + + const hasPermission = jest.fn().mockReturnValue(true); + const handleKeyringSnapMessage = jest.fn().mockReturnValue('foo'); + + const hooks = { + hasPermission, + handleKeyringSnapMessage, + }; + + const engine = new JsonRpcEngine(); + + engine.push((request, response, next, end) => { + const result = implementation( + request as JsonRpcRequest, + response as PendingJsonRpcResponse, + next, + end, + hooks, + ); + + result?.catch(end); + }); + + const response = await engine.handle({ + jsonrpc: '2.0', + id: 1, + method: 'snap_manageAccounts', + params: { + method: 'foo', + }, + }); + + expect(handleKeyringSnapMessage).toHaveBeenCalledWith({ + method: 'foo', + }); + + expect(response).toStrictEqual({ jsonrpc: '2.0', id: 1, result: 'foo' }); + }); + it('throws an error if the snap does not have permission', async () => { const { implementation } = manageAccountsHandler; diff --git a/packages/snaps-rpc-methods/src/permitted/manageAccounts.ts b/packages/snaps-rpc-methods/src/permitted/manageAccounts.ts index ea2be888a3..818f87ea93 100644 --- a/packages/snaps-rpc-methods/src/permitted/manageAccounts.ts +++ b/packages/snaps-rpc-methods/src/permitted/manageAccounts.ts @@ -1,16 +1,16 @@ import type { JsonRpcEngineEndCallback } from '@metamask/json-rpc-engine'; import type { PermittedHandlerExport } from '@metamask/permission-controller'; import { rpcErrors } from '@metamask/rpc-errors'; -import type { - ManageAccountsParams, - ManageAccountsResult, +import { + selectiveUnion, + type ManageAccountsParams, + type ManageAccountsResult, } from '@metamask/snaps-sdk'; import { type InferMatching } from '@metamask/snaps-utils'; import { array, create, object, - optional, record, string, StructError, @@ -21,7 +21,7 @@ import type { JsonRpcRequest, PendingJsonRpcResponse, } from '@metamask/utils'; -import { JsonStruct } from '@metamask/utils'; +import { hasProperty, JsonStruct } from '@metamask/utils'; import { SnapEndowments } from '../endowments'; import type { MethodHooksObject } from '../utils'; @@ -59,9 +59,21 @@ export const manageAccountsHandler: PermittedHandlerExport< hookNames, }; -const ManageAccountsParametersStruct = object({ +const ManageAccountsParametersWithParamsStruct = object({ + method: string(), + params: union([array(JsonStruct), record(string(), JsonStruct)]), +}); + +const ManageAccountsParametersWithoutParamsStruct = object({ method: string(), - params: optional(union([array(JsonStruct), record(string(), JsonStruct)])), +}); + +const ManageAccountsParametersStruct = selectiveUnion((value) => { + if (hasProperty(value, 'params')) { + return ManageAccountsParametersWithParamsStruct; + } + + return ManageAccountsParametersWithoutParamsStruct; }); export type ManageAccountsParameters = InferMatching< From 96f9c0c4d46806cc0828b9807385266b6a4b99b1 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Fri, 22 Nov 2024 12:47:08 +0100 Subject: [PATCH 10/11] fogot file --- .../snaps-sdk/src/types/methods/manage-accounts.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/snaps-sdk/src/types/methods/manage-accounts.ts b/packages/snaps-sdk/src/types/methods/manage-accounts.ts index 948f9ece4a..192fe683df 100644 --- a/packages/snaps-sdk/src/types/methods/manage-accounts.ts +++ b/packages/snaps-sdk/src/types/methods/manage-accounts.ts @@ -6,10 +6,14 @@ import type { Json } from '@metamask/utils'; * @property method - The method to call on the Snap. * @property params - The optional parameters to pass to the Snap method. */ -export type ManageAccountsParams = { - method: string; - params?: Json[] | Record; -}; +export type ManageAccountsParams = + | { + method: string; + } + | { + method: string; + params: Json[] | Record; + }; /** * The result returned by the `snap_manageAccounts` method, which is the result From 3543200697231b1e7bb8b893f5d56de0f82c1100 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Fri, 22 Nov 2024 12:51:58 +0100 Subject: [PATCH 11/11] fix lint --- packages/snaps-controllers/src/snaps/SnapController.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.tsx b/packages/snaps-controllers/src/snaps/SnapController.test.tsx index d3ee96dda8..c43b12e366 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.tsx +++ b/packages/snaps-controllers/src/snaps/SnapController.test.tsx @@ -18,7 +18,7 @@ import { handlerEndowments, SnapEndowments, } from '@metamask/snaps-rpc-methods'; -import type { Snap, SnapId } from '@metamask/snaps-sdk'; +import type { SnapId } from '@metamask/snaps-sdk'; import { AuxiliaryFileEncoding, text } from '@metamask/snaps-sdk'; import { Text } from '@metamask/snaps-sdk/jsx'; import type { SnapPermissions, RpcOrigins } from '@metamask/snaps-utils';