fix: Implement eth_accounts ahead of the permission middleware

app/scripts/lib/rpc-method-middleware/createMethodMiddleware.js

@@ -2,69 +2,92 @@ import { permissionRpcMethods } from '@metamask/permission-controller';
 import { selectHooks } from '@metamask/snaps-rpc-methods';
 import { hasProperty } from '@metamask/utils';
 import { ethErrors } from 'eth-rpc-errors';
-import localHandlers from './handlers';
+import { handlers as localHandlers, legacyHandlers } from './handlers';
 
 const allHandlers = [...localHandlers, ...permissionRpcMethods.handlers];
 
-const handlerMap = allHandlers.reduce((map, handler) => {
-  for (const methodName of handler.methodNames) {
-    map[methodName] = handler;
-  }
-  return map;
-}, {});
+// The primary home of RPC method implementations in MetaMask. **MUST** _succeed_
+// our permissioning logic in the JSON-RPC middleware pipeline.
+export const createMethodMiddleware = makeMethodMiddlewareMaker(allHandlers);
 
-const expectedHookNames = new Set(
-  allHandlers.flatMap(({ hookNames }) => Object.getOwnPropertyNames(hookNames)),
-);
+// A collection of RPC method implementations that, for legacy reasons, **MAY**
+// _precede_ our permissioning logic in the JSON-RPC middleware pipeline.
+export const createLegacyMethodMiddleware =
+  makeMethodMiddlewareMaker(legacyHandlers);
 
 /**
- * Creates a json-rpc-engine middleware of RPC method implementations.
- *
- * Handlers consume functions that hook into the background, and only depend
- * on their signatures, not e.g. controller internals.
+ * Creates a method middleware factory function given a set of method handlers.
  *
- * @param  {Record<string, (...args: unknown[]) => unknown | Promise<unknown>>} hooks - Required "hooks" into our
- * controllers.
- * @returns {import('json-rpc-engine').JsonRpcMiddleware<unknown, unknown>} The method middleware function.
+ * @param {Record<string, import('@metamask/permission-controller').PermittedHandlerExport>} handlers - The RPC method
+ * handler implementations.
+ * @returns The method middleware factory function.
  */
-export function createMethodMiddleware(hooks) {
-  assertExpectedHook(hooks);
+function makeMethodMiddlewareMaker(handlers) {
+  const handlerMap = handlers.reduce((map, handler) => {
+    for (const methodName of handler.methodNames) {
+      map[methodName] = handler;
+    }
+    return map;
+  }, {});
+
+  const expectedHookNames = new Set(
+    handlers.flatMap(({ hookNames }) => Object.getOwnPropertyNames(hookNames)),
+  );
 
-  return async function methodMiddleware(req, res, next, end) {
-    const handler = handlerMap[req.method];
-    if (handler) {
-      const { implementation, hookNames } = handler;
-      try {
-        // Implementations may or may not be async, so we must await them.
-        return await implementation(
-          req,
-          res,
-          next,
-          end,
-          selectHooks(hooks, hookNames),
-        );
-      } catch (error) {
-        if (process.env.METAMASK_DEBUG) {
-          console.error(error);
+  /**
+   * Creates a json-rpc-engine middleware of RPC method implementations.
+   *
+   * Handlers consume functions that hook into the background, and only depend
+   * on their signatures, not e.g. controller internals.
+   *
+   * @param  {Record<string, (...args: unknown[]) => unknown | Promise<unknown>>} hooks - Required "hooks" into our
+   * controllers.
+   * @returns {import('json-rpc-engine').JsonRpcMiddleware<unknown, unknown>} The method middleware function.
+   */
+  const makeMethodMiddleware = (hooks) => {
+    assertExpectedHook(hooks, expectedHookNames);
+
+    const methodMiddleware = async (req, res, next, end) => {
+      const handler = handlerMap[req.method];
+      if (handler) {
+        const { implementation, hookNames } = handler;
+        try {
+          // Implementations may or may not be async, so we must await them.
+          return await implementation(
+            req,
+            res,
+            next,
+            end,
+            selectHooks(hooks, hookNames),
+          );
+        } catch (error) {
+          if (process.env.METAMASK_DEBUG) {
+            console.error(error);
+          }
+          return end(
+            error instanceof Error
+              ? error
+              : ethErrors.rpc.internal({ data: error }),
+          );
         }
-        return end(
-          error instanceof Error
-            ? error
-            : ethErrors.rpc.internal({ data: error }),
-        );
       }
-    }
 
-    return next();
+      return next();
+    };
+
+    return methodMiddleware;
   };
+
+  return makeMethodMiddleware;
 }
 
 /**
- * Asserts that the hooks object only has all expected hooks and no extraneous ones.
+ * Asserts that the specified hooks object only has all expected hooks and no extraneous ones.
  *
  * @param {Record<string, unknown>} hooks - Required "hooks" into our controllers.
+ * @param {string[]} expectedHookNames - The expected hook names.
  */
-function assertExpectedHook(hooks) {
+function assertExpectedHook(hooks, expectedHookNames) {
   const missingHookNames = [];
   expectedHookNames.forEach((hookName) => {
     if (!hasProperty(hooks, hookName)) {

app/scripts/lib/rpc-method-middleware/createMethodMiddleware.test.js

@@ -3,14 +3,14 @@ import {
   assertIsJsonRpcFailure,
   assertIsJsonRpcSuccess,
 } from '@metamask/utils';
-import { createMethodMiddleware } from '.';
+import { createMethodMiddleware, createLegacyMethodMiddleware } from '.';
 
 jest.mock('@metamask/permission-controller', () => ({
   permissionRpcMethods: { handlers: [] },
 }));
 
-jest.mock('./handlers', () => [
-  {
+jest.mock('./handlers', () => {
+  const getHandler = () => ({
     implementation: (req, res, _next, end, hooks) => {
       if (Array.isArray(req.params)) {
         switch (req.params[0]) {
@@ -35,10 +35,18 @@ jest.mock('./handlers', () => [
     },
     hookNames: { hook1: true, hook2: true },
     methodNames: ['method1', 'method2'],
-  },
-]);
+  });
+
+  return {
+    handlers: [getHandler()],
+    legacyHandlers: [getHandler()],
+  };
+});
 
-describe('createMethodMiddleware', () => {
+describe.each([
+  ['createMethodMiddleware', createMethodMiddleware],
+  ['createLegacyMethodMiddleware', createLegacyMethodMiddleware],
+])('%s', (_name, createMiddleware) => {
   const method1 = 'method1';
 
   const getDefaultHooks = () => ({
@@ -47,17 +55,15 @@ describe('createMethodMiddleware', () => {
   });
 
   it('should return a function', () => {
-    const middleware = createMethodMiddleware(getDefaultHooks());
+    const middleware = createMiddleware(getDefaultHooks());
     expect(typeof middleware).toBe('function');
   });
 
   it('should throw an error if a required hook is missing', () => {
     const hooks = { hook1: () => 42 };
 
     // @ts-expect-error Intentional destructive testing
-    expect(() => createMethodMiddleware(hooks)).toThrow(
-      'Missing expected hooks',
-    );
+    expect(() => createMiddleware(hooks)).toThrow('Missing expected hooks');
   });
 
   it('should throw an error if an extraneous hook is provided', () => {
@@ -66,13 +72,11 @@ describe('createMethodMiddleware', () => {
       extraneousHook: () => 100,
     };
 
-    expect(() => createMethodMiddleware(hooks)).toThrow(
-      'Received unexpected hooks',
-    );
+    expect(() => createMiddleware(hooks)).toThrow('Received unexpected hooks');
   });
 
   it('should call the handler for the matching method (uses hook1)', async () => {
-    const middleware = createMethodMiddleware(getDefaultHooks());
+    const middleware = createMiddleware(getDefaultHooks());
     const engine = new JsonRpcEngine();
     engine.push(middleware);
 
@@ -88,7 +92,7 @@ describe('createMethodMiddleware', () => {
   });
 
   it('should call the handler for the matching method (uses hook2)', async () => {
-    const middleware = createMethodMiddleware(getDefaultHooks());
+    const middleware = createMiddleware(getDefaultHooks());
     const engine = new JsonRpcEngine();
     engine.push(middleware);
 
@@ -104,7 +108,7 @@ describe('createMethodMiddleware', () => {
   });
 
   it('should not call the handler for a non-matching method', async () => {
-    const middleware = createMethodMiddleware(getDefaultHooks());
+    const middleware = createMiddleware(getDefaultHooks());
     const engine = new JsonRpcEngine();
     engine.push(middleware);
 
@@ -123,7 +127,7 @@ describe('createMethodMiddleware', () => {
   });
 
   it('should handle errors returned by the implementation', async () => {
-    const middleware = createMethodMiddleware(getDefaultHooks());
+    const middleware = createMiddleware(getDefaultHooks());
     const engine = new JsonRpcEngine();
     engine.push(middleware);
 
@@ -139,7 +143,7 @@ describe('createMethodMiddleware', () => {
   });
 
   it('should handle errors thrown by the implementation', async () => {
-    const middleware = createMethodMiddleware(getDefaultHooks());
+    const middleware = createMiddleware(getDefaultHooks());
     const engine = new JsonRpcEngine();
     engine.push(middleware);
 
@@ -155,7 +159,7 @@ describe('createMethodMiddleware', () => {
   });
 
   it('should handle non-errors thrown by the implementation', async () => {
-    const middleware = createMethodMiddleware(getDefaultHooks());
+    const middleware = createMiddleware(getDefaultHooks());
     const engine = new JsonRpcEngine();
     engine.push(middleware);
 

app/scripts/lib/rpc-method-middleware/handlers/index.ts

@@ -16,9 +16,8 @@ import mmiSetAccountAndNetwork from './institutional/mmi-set-account-and-network
 import mmiOpenAddHardwareWallet from './institutional/mmi-open-add-hardware-wallet';
 ///: END:ONLY_INCLUDE_IF
 
-const handlers = [
+export const handlers = [
   addEthereumChain,
-  ethAccounts,
   getProviderState,
   logWeb3ShimUsage,
   requestAccounts,
@@ -35,4 +34,4 @@ const handlers = [
   ///: END:ONLY_INCLUDE_IF
 ];
 
-export default handlers;
+export const legacyHandlers = [ethAccounts];

app/scripts/metamask-controller.js

@@ -272,6 +272,7 @@ import AccountTracker from './lib/account-tracker';
 import createDupeReqFilterMiddleware from './lib/createDupeReqFilterMiddleware';
 import createLoggerMiddleware from './lib/createLoggerMiddleware';
 import {
+  createLegacyMethodMiddleware,
   createMethodMiddleware,
   createUnsupportedMethodMiddleware,
 } from './lib/rpc-method-middleware';
@@ -5109,6 +5110,14 @@ export default class MetamaskController extends EventEmitter {
 
     engine.push(createUnsupportedMethodMiddleware());
 
+    // Legacy RPC methods that need to be implemented _ahead of_ the permission
+    // middleware.
+    engine.push(
+      createLegacyMethodMiddleware({
+        getAccounts: this.getPermittedAccounts.bind(this, origin),
+      }),
+    );
+
     if (subjectType !== SubjectType.Internal) {
       engine.push(
         this.permissionController.createPermissionMiddleware({
@@ -5126,7 +5135,8 @@ export default class MetamaskController extends EventEmitter {
       );
     }
 
-    // Unrestricted/permissionless RPC method implementations
+    // Unrestricted/permissionless RPC method implementations.
+    // They must nevertheless be placed _behind_ the permission middleware.
     engine.push(
       createMethodMiddleware({
         origin,

app/scripts/metamask-controller.test.js

@@ -89,6 +89,9 @@ const rpcMethodMiddlewareMock = {
   createMethodMiddleware: () => (_req, _res, next, _end) => {
     next();
   },
+  createLegacyMethodMiddleware: () => (_req, _res, next, _end) => {
+    next();
+  },
   createUnsupportedMethodMiddleware: () => (_req, _res, next, _end) => {
     next();
   },