diff --git a/packages/snap-account-service/CHANGELOG.md b/packages/snap-account-service/CHANGELOG.md index 44fcb39bcd..9372edbbd4 100644 --- a/packages/snap-account-service/CHANGELOG.md +++ b/packages/snap-account-service/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Prevent double-lock in `:handleKeyringSnapMessage` for some events/methods ([#8860](https://github.com/MetaMask/core/pull/8860)) + - The service messenger now requires the `KeyringController:withKeyringUnsafe` action. + - We now check if the keyring is available before delegating those messages. + - We still auto-create the keyring in some specific calls (e.g `notify:accountCreated`). + ## [0.2.0] ### Added diff --git a/packages/snap-account-service/package.json b/packages/snap-account-service/package.json index 16265c0054..835f031167 100644 --- a/packages/snap-account-service/package.json +++ b/packages/snap-account-service/package.json @@ -56,7 +56,9 @@ "@metamask/account-api": "^1.0.4", "@metamask/account-tree-controller": "^7.4.0", "@metamask/eth-snap-keyring": "^22.0.1", + "@metamask/keyring-api": "^23.1.0", "@metamask/keyring-controller": "^25.5.0", + "@metamask/keyring-snap-sdk": "^9.0.1", "@metamask/messenger": "^1.2.0", "@metamask/snaps-controllers": "^19.0.0", "@metamask/snaps-sdk": "^11.0.0", diff --git a/packages/snap-account-service/src/SnapAccountService.test.ts b/packages/snap-account-service/src/SnapAccountService.test.ts index e1569d63a0..5e560fea12 100644 --- a/packages/snap-account-service/src/SnapAccountService.test.ts +++ b/packages/snap-account-service/src/SnapAccountService.test.ts @@ -1,6 +1,9 @@ import type { AccountGroupId } from '@metamask/account-api'; import type { SnapKeyring, SnapMessage } from '@metamask/eth-snap-keyring'; +import { KeyringEvent } from '@metamask/keyring-api'; import { + KeyringControllerError, + KeyringControllerErrorMessage, KeyringControllerState, KeyringTypes, } from '@metamask/keyring-controller'; @@ -8,6 +11,7 @@ import type { KeyringEntry, RestrictedController, } from '@metamask/keyring-controller'; +import { SnapManageAccountsMethod } from '@metamask/keyring-snap-sdk'; import { Messenger, MOCK_ANY_NAMESPACE } from '@metamask/messenger'; import type { MockAnyNamespace, @@ -62,6 +66,7 @@ type Mocks = { KeyringController: { getState: jest.MockedFunction<() => { keyrings: { type: string }[] }>; withController: jest.Mock; + withKeyringUnsafe: jest.Mock; }; // eslint-disable-next-line @typescript-eslint/naming-convention AccountTreeController: { @@ -103,6 +108,7 @@ function getMessenger( 'SnapController:getRunnableSnaps', 'KeyringController:getState', 'KeyringController:withController', + 'KeyringController:withKeyringUnsafe', 'AccountTreeController:getAccountGroupObject', 'AccountTreeController:getSelectedAccountGroup', ], @@ -323,6 +329,7 @@ function mockWithController( * @param keyring - The mocked Snap keyring methods. * @param keyring.handleKeyringSnapMessage - The mocked implementation. * @param keyring.setSelectedAccounts - The mocked implementation. + * @returns The mocked Snap keyring for assertions. */ function mockLegacySnapKeyring( mocks: Mocks, @@ -337,7 +344,7 @@ function mockLegacySnapKeyring( SnapKeyring['setSelectedAccounts'] >; }, -): void { +): MockSnapKeyring { const snapKeyring: MockSnapKeyring = { type: KeyringTypes.snap, handleKeyringSnapMessage, @@ -357,6 +364,28 @@ function mockLegacySnapKeyring( removeKeyring: jest.fn(), }), ); + mocks.KeyringController.withKeyringUnsafe.mockImplementation( + async (_selector, operation) => + operation({ + keyring: snapKeyring as KeyringEntry['keyring'], + metadata: { id: 'id-snap', name: KeyringTypes.snap }, + }), + ); + return snapKeyring; +} + +/** + * Configures `mocks.KeyringController.withKeyringUnsafe` to reject as if the + * legacy Snap keyring did not exist yet. + * + * @param mocks - The mocks object from {@link setup}. + */ +function mockLegacySnapKeyringMissing(mocks: Mocks): void { + mocks.KeyringController.withKeyringUnsafe.mockImplementation(async () => { + throw new KeyringControllerError( + KeyringControllerErrorMessage.KeyringNotFound, + ); + }); } /** @@ -398,6 +427,7 @@ function setup({ KeyringController: { getState: jest.fn().mockReturnValue({ keyrings }), withController: jest.fn(), + withKeyringUnsafe: jest.fn(), }, AccountTreeController: { getAccountGroupObject: jest.fn().mockReturnValue(undefined), @@ -421,6 +451,10 @@ function setup({ 'KeyringController:withController', mocks.KeyringController.withController, ); + rootMessenger.registerActionHandler( + 'KeyringController:withKeyringUnsafe', + mocks.KeyringController.withKeyringUnsafe, + ); rootMessenger.registerActionHandler( 'AccountTreeController:getAccountGroupObject', mocks.AccountTreeController.getAccountGroupObject, @@ -634,7 +668,7 @@ describe('SnapAccountService', () => { describe('handleKeyringSnapMessage', () => { const MOCK_MESSAGE = { - method: 'keyring_listAccounts', + method: KeyringEvent.AccountUpdated, params: {}, } as unknown as SnapMessage; @@ -689,6 +723,133 @@ describe('SnapAccountService', () => { ); expect(result).toBe('pong'); }); + + it('throws when the legacy Snap keyring does not exist yet for a non-AccountCreated message', async () => { + const { service, mocks } = setup(); + mockLegacySnapKeyringMissing(mocks); + + await expect( + service.handleKeyringSnapMessage(MOCK_SNAP_ID, MOCK_MESSAGE), + ).rejects.toThrow( + `Legacy Snap keyring does not exist yet for snap "${MOCK_SNAP_ID}".`, + ); + expect(mocks.KeyringController.withController).not.toHaveBeenCalled(); + }); + + it('propagates non-KeyringNotFound errors from withKeyringUnsafe', async () => { + const { service, mocks } = setup(); + mocks.KeyringController.withKeyringUnsafe.mockImplementation(async () => { + throw new Error('boom'); + }); + + await expect( + service.handleKeyringSnapMessage(MOCK_SNAP_ID, MOCK_MESSAGE), + ).rejects.toThrow('boom'); + }); + + describe('when the message is an AccountCreated event', () => { + const ACCOUNT_CREATED_MESSAGE = { + method: KeyringEvent.AccountCreated, + params: {}, + } as unknown as SnapMessage; + + it('auto-creates the legacy Snap keyring when it does not exist yet', async () => { + const { service, mocks } = setup(); + mockLegacySnapKeyringMissing(mocks); + const handleKeyringSnapMessage = jest + .fn() + .mockResolvedValue({ ok: true }); + // `getLegacySnapKeyring` goes through `withController` and creates the + // keyring if missing. + mocks.KeyringController.withController.mockImplementation( + async (operation) => + operation({ + get keyrings() { + return Object.freeze([]); + }, + addNewKeyring: jest.fn(async () => ({ + keyring: { + type: KeyringTypes.snap, + handleKeyringSnapMessage, + } as unknown as KeyringEntry['keyring'], + metadata: { id: 'id-snap', name: KeyringTypes.snap }, + })), + removeKeyring: jest.fn(), + }), + ); + + const result = await service.handleKeyringSnapMessage( + MOCK_SNAP_ID, + ACCOUNT_CREATED_MESSAGE, + ); + + expect(mocks.KeyringController.withController).toHaveBeenCalled(); + expect(handleKeyringSnapMessage).toHaveBeenCalledWith( + MOCK_SNAP_ID, + ACCOUNT_CREATED_MESSAGE, + ); + expect(result).toStrictEqual({ ok: true }); + }); + + it('uses the existing legacy Snap keyring when it is already available', async () => { + const { service, mocks } = setup(); + const handleKeyringSnapMessage = jest + .fn() + .mockResolvedValue({ ok: true }); + mockLegacySnapKeyring(mocks, { handleKeyringSnapMessage }); + + const result = await service.handleKeyringSnapMessage( + MOCK_SNAP_ID, + ACCOUNT_CREATED_MESSAGE, + ); + + expect(mocks.KeyringController.withController).not.toHaveBeenCalled(); + expect(handleKeyringSnapMessage).toHaveBeenCalledWith( + MOCK_SNAP_ID, + ACCOUNT_CREATED_MESSAGE, + ); + expect(result).toStrictEqual({ ok: true }); + }); + }); + + describe('when the message is a GetSelectedAccounts request', () => { + const GET_SELECTED_ACCOUNTS_MESSAGE = { + method: SnapManageAccountsMethod.GetSelectedAccounts, + params: {}, + } as unknown as SnapMessage; + + it('delegates to the legacy Snap keyring when it is available', async () => { + const { service, mocks } = setup(); + const handleKeyringSnapMessage = jest + .fn() + .mockResolvedValue(['account-1']); + mockLegacySnapKeyring(mocks, { handleKeyringSnapMessage }); + + const result = await service.handleKeyringSnapMessage( + MOCK_SNAP_ID, + GET_SELECTED_ACCOUNTS_MESSAGE, + ); + + expect(handleKeyringSnapMessage).toHaveBeenCalledWith( + MOCK_SNAP_ID, + GET_SELECTED_ACCOUNTS_MESSAGE, + ); + expect(result).toStrictEqual(['account-1']); + }); + + it('returns an empty list when the legacy Snap keyring does not exist yet', async () => { + const { service, mocks } = setup(); + mockLegacySnapKeyringMissing(mocks); + + const result = await service.handleKeyringSnapMessage( + MOCK_SNAP_ID, + GET_SELECTED_ACCOUNTS_MESSAGE, + ); + + expect(result).toStrictEqual([]); + expect(mocks.KeyringController.withController).not.toHaveBeenCalled(); + }); + }); }); describe('on AccountTreeController:selectedAccountGroupChange', () => { @@ -773,6 +934,28 @@ describe('SnapAccountService', () => { consoleErrorSpy.mockRestore(); }); + + it('skips silently when the legacy Snap keyring does not exist yet', async () => { + const { service, rootMessenger, mocks } = setup(); + mockLegacySnapKeyringMissing(mocks); + mocks.AccountTreeController.getAccountGroupObject.mockReturnValue( + buildGroup(MOCK_GROUP_ID, MOCK_ACCOUNTS), + ); + const consoleErrorSpy = jest + .spyOn(console, 'error') + .mockImplementation(() => undefined); + expect(service).toBeDefined(); + + publishSelectedAccountGroupChange(rootMessenger, MOCK_GROUP_ID); + await flushMicrotasks(); + + // The forwarder must NOT auto-create the keyring through `withController`. + expect(mocks.KeyringController.withController).not.toHaveBeenCalled(); + // And it must NOT bubble the "keyring not found" condition as an error. + expect(consoleErrorSpy).not.toHaveBeenCalled(); + + consoleErrorSpy.mockRestore(); + }); }); describe('on KeyringController:unlock', () => { diff --git a/packages/snap-account-service/src/SnapAccountService.ts b/packages/snap-account-service/src/SnapAccountService.ts index 7f73096178..24f41aa1fc 100644 --- a/packages/snap-account-service/src/SnapAccountService.ts +++ b/packages/snap-account-service/src/SnapAccountService.ts @@ -1,8 +1,9 @@ import { AccountGroupId } from '@metamask/account-api'; -import type { +import { SnapKeyring as LegacySnapKeyring, SnapMessage, } from '@metamask/eth-snap-keyring'; +import { KeyringEvent } from '@metamask/keyring-api'; import type { KeyringControllerGetStateAction, KeyringControllerStateChangeEvent, @@ -10,7 +11,12 @@ import type { KeyringControllerWithControllerAction, KeyringEntry, } from '@metamask/keyring-controller'; -import { KeyringTypes } from '@metamask/keyring-controller'; +import { + isKeyringNotFoundError, + KeyringTypes, +} from '@metamask/keyring-controller'; +import { KeyringControllerWithKeyringUnsafeAction } from '@metamask/keyring-controller'; +import { SnapManageAccountsMethod } from '@metamask/keyring-snap-sdk'; import type { AccountId, BaseKeyring } from '@metamask/keyring-utils'; import type { Messenger } from '@metamask/messenger'; import type { @@ -83,6 +89,7 @@ type AllowedActions = | SnapControllerGetRunnableSnapsAction | KeyringControllerGetStateAction | KeyringControllerWithControllerAction + | KeyringControllerWithKeyringUnsafeAction | AccountTreeControllerGetAccountGroupObjectAction | AccountTreeControllerGetSelectedAccountGroupAction; @@ -348,6 +355,39 @@ export class SnapAccountService { return (result as Result).snapKeyring; } + /** + * Gets the legacy (v1) Snap keyring but do not auto-create it if it doesn't exist. + * + * @returns The existing Snap keyring instance, or undefined if it doesn't exist. + */ + async #getLegacySnapKeyringIfAvailable(): Promise< + LegacySnapKeyring | undefined + > { + type Result = { + snapKeyring: LegacySnapKeyring; + }; + + try { + const result = await this.#messenger.call( + 'KeyringController:withKeyringUnsafe', + { filter: isLegacySnapKeyring }, + async ({ keyring }): Promise => { + // The legacy Snap keyring is not compatible with `EthKeyring`, so we need to cast here. + return { snapKeyring: keyring } as unknown as Result; + }, + ); + + return (result as Result).snapKeyring; + } catch (error) { + if (isKeyringNotFoundError(error)) { + log('Legacy Snap keyring not available yet.'); + return undefined; + } + + throw error; + } + } + /** * Handle a message from a Snap. * @@ -359,7 +399,43 @@ export class SnapAccountService { snapId: SnapId, message: SnapMessage, ): Promise { - const snapKeyring = await this.getLegacySnapKeyring(); + let snapKeyring: LegacySnapKeyring | undefined = + await this.#getLegacySnapKeyringIfAvailable(); + + // Handle specific methods first. + if (message.method === SnapManageAccountsMethod.GetSelectedAccounts) { + if (snapKeyring) { + // The legacy Snap keyring already maintain a local list of selected accounts per Snaps, so we + // just delegate the call. + return snapKeyring.handleKeyringSnapMessage(snapId, message); + } + + // Some Snaps might be using `getSelectedAccounts` early in their lifecycle, before the keyring is created. So we + // do not throw in that case to avoid messing up their lifecycle. + return []; + } + + const event = message.method as KeyringEvent; // We assume the Snap platform always sends a valid `KeyringEvent` here. + log( + `Forwarding message "${event}" from Snap "${snapId}" to its keyring...`, + ); + + // We can create a new keyring if the message is an AccountCreated event. + const isAccountCreatedMessage = event === KeyringEvent.AccountCreated; + + // Create the Snap keyring if it doesn't exist yet (in an atomic way). We cannot assume + // the keyring exists (e.g for the MMI Snap). + // NOTE: We only auto-create it for v1 account creation flows. + if (isAccountCreatedMessage && !snapKeyring) { + snapKeyring = await this.getLegacySnapKeyring(); + } + + if (!snapKeyring) { + throw new Error( + `Legacy Snap keyring does not exist yet for snap "${snapId}".`, + ); + } + return snapKeyring.handleKeyringSnapMessage(snapId, message); } @@ -397,7 +473,14 @@ export class SnapAccountService { log(`Clearing selected accounts (from "${groupId}")`); } - const snapKeyring = await this.getLegacySnapKeyring(); + const snapKeyring = await this.#getLegacySnapKeyringIfAvailable(); + if (!snapKeyring) { + log( + 'No legacy Snap keyring available, skipping forwarding selected accounts.', + ); + return; + } + await snapKeyring.setSelectedAccounts(accounts); }; diff --git a/yarn.lock b/yarn.lock index 9eff13004d..3489069ede 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5465,7 +5465,9 @@ __metadata: "@metamask/account-tree-controller": "npm:^7.4.0" "@metamask/auto-changelog": "npm:^6.1.0" "@metamask/eth-snap-keyring": "npm:^22.0.1" + "@metamask/keyring-api": "npm:^23.1.0" "@metamask/keyring-controller": "npm:^25.5.0" + "@metamask/keyring-snap-sdk": "npm:^9.0.1" "@metamask/keyring-utils": "npm:^3.2.1" "@metamask/messenger": "npm:^1.2.0" "@metamask/snaps-controllers": "npm:^19.0.0"