fix(snap-account-service)!: relax keyring messages/methods handling

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

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",

packages/snap-account-service/src/SnapAccountService.test.ts

@@ -1,13 +1,17 @@
 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';
 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', () => {