fix: fix granted permissions when adding an existing network via wallet_addEthereumChain (#29837)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

Fixes bug with the automatically permittedChains permission grant when
triggered via wallet_addEthereumChain in certain scenarios. See ticket
for details.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29837?quickstart=1)

## **Related issues**

Fixes: MetaMask/MetaMask-planning#3814

## **Manual testing steps**

1. Add a new network to the dapp via
[wallet_addEthereumChain](https://docs.metamask.io/wallet/reference/json-rpc-methods/wallet_addethereumchain/)
2. After approval, the dapp should also have
`endowment:permitted-chains` permission for the recently added chain via
`wallet_getPermissions`
3. switch the dapp to a different chain via the wallet UI
4. revoke dapp permissions via the wallet UI
6. Add a new network to the dapp via
[wallet_addEthereumChain](https://docs.metamask.io/wallet/reference/json-rpc-methods/wallet_addethereumchain/)
using a DIFFERENT rpc endpoint, but for the same chainId added in step 1
7. After approval, the dapp should also have
`endowment:permitted-chains` permission for the recently added chain via
`wallet_getPermissions`
8. switch the dapp to a different chain via the wallet UI
9. revoke dapp permissions via the wallet UI
10. Add a new network to the dapp via
[wallet_addEthereumChain](https://docs.metamask.io/wallet/reference/json-rpc-methods/wallet_addethereumchain/)
using the SAME rpc endpoint in step 1 (i.e. repeat step 1 exactly)
11. You should get an approval for chain switch, NOT network adding.
Approve it.
12. After approval, the dapp should also have
`endowment:permitted-chains` permission for the recently added chain via
`wallet_getPermissions`


## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Alex Donesky <[email protected]>

Author: jiexi

app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.js

@@ -88,12 +88,14 @@ async function addEthereumChainHandler(
     : undefined;
 
   // If there's something to add or update
-  if (
+
+  const shouldAddOrUpdateNetwork =
     !existingNetwork ||
     rpcIndex !== existingNetwork.defaultRpcEndpointIndex ||
     (firstValidBlockExplorerUrl &&
-      blockExplorerIndex !== existingNetwork.defaultBlockExplorerUrlIndex)
-  ) {
+      blockExplorerIndex !== existingNetwork.defaultBlockExplorerUrlIndex);
+
+  if (shouldAddOrUpdateNetwork) {
     try {
       await requestUserApproval({
         origin,
@@ -180,20 +182,14 @@ async function addEthereumChainHandler(
     }
   }
 
-  // If the added or updated network is not the current chain, prompt the user to switch
-  if (chainId !== currentChainIdForDomain) {
-    const { networkClientId } =
-      updatedNetwork.rpcEndpoints[updatedNetwork.defaultRpcEndpointIndex];
-
-    return switchChain(res, end, chainId, networkClientId, {
-      isAddFlow: true,
-      setActiveNetwork,
-      getCaveat,
-      requestPermittedChainsPermissionForOrigin,
-      requestPermittedChainsPermissionIncrementalForOrigin,
-    });
-  }
+  const { networkClientId } =
+    updatedNetwork.rpcEndpoints[updatedNetwork.defaultRpcEndpointIndex];
 
-  res.result = null;
-  return end();
+  return switchChain(res, end, chainId, networkClientId, {
+    autoApprove: shouldAddOrUpdateNetwork,
+    setActiveNetwork,
+    getCaveat,
+    requestPermittedChainsPermissionForOrigin,
+    requestPermittedChainsPermissionIncrementalForOrigin,
+  });
 }

app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.test.js

@@ -184,7 +184,7 @@ describe('addEthereumChainHandler', () => {
       NON_INFURA_CHAIN_ID,
       123,
       {
-        isAddFlow: true,
+        autoApprove: true,
         getCaveat: mocks.getCaveat,
         setActiveNetwork: mocks.setActiveNetwork,
         requestPermittedChainsPermissionForOrigin:
@@ -251,7 +251,7 @@ describe('addEthereumChainHandler', () => {
           '0x1',
           123,
           {
-            isAddFlow: true,
+            autoApprove: true,
             getCaveat: mocks.getCaveat,
             setActiveNetwork: mocks.setActiveNetwork,
             requestPermittedChainsPermissionForOrigin:
@@ -264,7 +264,7 @@ describe('addEthereumChainHandler', () => {
     });
 
     describe('if the proposed networkConfiguration does not have a different rpcUrl from the one already in state', () => {
-      it('should only switch to the existing networkConfiguration if one already exists for the given chain id', async () => {
+      it('should only switch to the existing networkConfiguration if one already exists for the given chain id without auto approving the chain permission', async () => {
         const { mocks, end, handler } = createMockedHandler();
         mocks.getCurrentChainIdForDomain.mockReturnValue(CHAIN_IDS.MAINNET);
         mocks.getNetworkConfigurationByChainId.mockReturnValue(
@@ -298,7 +298,7 @@ describe('addEthereumChainHandler', () => {
           '0xa',
           createMockOptimismConfiguration().rpcEndpoints[0].networkClientId,
           {
-            isAddFlow: true,
+            autoApprove: false,
             getCaveat: mocks.getCaveat,
             setActiveNetwork: mocks.setActiveNetwork,
             requestPermittedChainsPermissionForOrigin:
@@ -338,30 +338,4 @@ describe('addEthereumChainHandler', () => {
       }),
     );
   });
-
-  it('should add result set to null to response object if the requested rpcUrl (and chainId) is currently selected', async () => {
-    const CURRENT_RPC_CONFIG = createMockNonInfuraConfiguration();
-
-    const { mocks, response, handler } = createMockedHandler();
-    mocks.getCurrentChainIdForDomain.mockReturnValue(
-      CURRENT_RPC_CONFIG.chainId,
-    );
-    mocks.getNetworkConfigurationByChainId.mockReturnValue(CURRENT_RPC_CONFIG);
-    await handler({
-      origin: 'example.com',
-      params: [
-        {
-          chainId: CURRENT_RPC_CONFIG.chainId,
-          chainName: 'Custom Network',
-          rpcUrls: [CURRENT_RPC_CONFIG.rpcEndpoints[0].url],
-          nativeCurrency: {
-            symbol: CURRENT_RPC_CONFIG.nativeCurrency,
-            decimals: 18,
-          },
-          blockExplorerUrls: ['https://custom.blockexplorer'],
-        },
-      ],
-    });
-    expect(response.result).toBeNull();
-  });
 });

app/scripts/lib/rpc-method-middleware/handlers/ethereum-chain-utils.js

@@ -165,20 +165,20 @@ export function validateAddEthereumChainParams(params) {
  * @param {string} chainId - The chainId being switched to.
  * @param {string} networkClientId - The network client being switched to.
  * @param {object} hooks - The hooks object.
- * @param {boolean} hooks.isAddFlow - The boolean determining if this call originates from wallet_addEthereumChain.
+ * @param {boolean} [hooks.autoApprove] - A boolean indicating whether the request should prompt the user or be automatically approved.
  * @param {Function} hooks.setActiveNetwork - The callback to change the current network for the origin.
  * @param {Function} hooks.getCaveat - The callback to get the CAIP-25 caveat for the origin.
  * @param {Function} hooks.requestPermittedChainsPermissionForOrigin - The callback to request a new permittedChains-equivalent CAIP-25 permission.
  * @param {Function} hooks.requestPermittedChainsPermissionIncrementalForOrigin - The callback to add a new chain to the permittedChains-equivalent CAIP-25 permission.
- * @returns a null response on success or an error if user rejects an approval when isAddFlow is false or on unexpected errors.
+ * @returns a null response on success or an error if user rejects an approval when autoApprove is false or on unexpected errors.
  */
 export async function switchChain(
   response,
   end,
   chainId,
   networkClientId,
   {
-    isAddFlow,
+    autoApprove,
     setActiveNetwork,
     getCaveat,
     requestPermittedChainsPermissionForOrigin,
@@ -197,13 +197,13 @@ export async function switchChain(
       if (!ethChainIds.includes(chainId)) {
         await requestPermittedChainsPermissionIncrementalForOrigin({
           chainId,
-          autoApprove: isAddFlow,
+          autoApprove,
         });
       }
     } else {
       await requestPermittedChainsPermissionForOrigin({
         chainId,
-        autoApprove: isAddFlow,
+        autoApprove,
       });
     }
 

app/scripts/lib/rpc-method-middleware/handlers/ethereum-chain-utils.test.ts

@@ -1,4 +1,4 @@
-import { rpcErrors } from '@metamask/rpc-errors';
+import { errorCodes, rpcErrors } from '@metamask/rpc-errors';
 import {
   Caip25CaveatType,
   Caip25EndowmentPermissionName,
@@ -10,7 +10,7 @@ describe('Ethereum Chain Utils', () => {
   const createMockedSwitchChain = () => {
     const end = jest.fn();
     const mocks = {
-      isAddFlow: false,
+      autoApprove: false,
       setActiveNetwork: jest.fn(),
       getCaveat: jest.fn(),
       requestPermittedChainsPermissionForOrigin: jest.fn(),
@@ -39,21 +39,10 @@ describe('Ethereum Chain Utils', () => {
       });
     });
 
-    it('passes through unexpected errors', async () => {
-      const { mocks, end, switchChain } = createMockedSwitchChain();
-      mocks.requestPermittedChainsPermissionForOrigin.mockRejectedValueOnce(
-        new Error('unexpected error'),
-      );
-
-      await switchChain('0x1', 'mainnet');
-
-      expect(end).toHaveBeenCalledWith(new Error('unexpected error'));
-    });
-
     describe('with no existing CAIP-25 permission', () => {
-      it('requests a switch chain approval without autoApprove if isAddFlow: false', async () => {
+      it('requests a switch chain approval without autoApprove if autoApprove: false', async () => {
         const { mocks, switchChain } = createMockedSwitchChain();
-        mocks.isAddFlow = false;
+        mocks.autoApprove = false;
         await switchChain('0x1', 'mainnet');
 
         expect(
@@ -68,28 +57,28 @@ describe('Ethereum Chain Utils', () => {
         expect(mocks.setActiveNetwork).toHaveBeenCalledWith('mainnet');
       });
 
-      it('should handle errors if the switch chain grant fails', async () => {
+      it('should throw an error if the switch chain approval is rejected', async () => {
         const { mocks, end, switchChain } = createMockedSwitchChain();
-        mocks.requestPermittedChainsPermissionForOrigin.mockRejectedValueOnce(
-          new Error('failed to grant permittedChains'),
-        );
+        mocks.requestPermittedChainsPermissionForOrigin.mockRejectedValueOnce({
+          code: errorCodes.provider.userRejectedRequest,
+        });
 
         await switchChain('0x1', 'mainnet');
 
         expect(
           mocks.requestPermittedChainsPermissionForOrigin,
         ).toHaveBeenCalled();
         expect(mocks.setActiveNetwork).not.toHaveBeenCalled();
-        expect(end).toHaveBeenCalledWith(
-          new Error('failed to grant permittedChains'),
-        );
+        expect(end).toHaveBeenCalledWith({
+          code: errorCodes.provider.userRejectedRequest,
+        });
       });
     });
 
     describe('with an existing CAIP-25 permission granted from the legacy flow (isMultichainOrigin: false) and the chainId is not already permissioned', () => {
-      it('requests a switch chain approval with autoApprove and switches to it if isAddFlow: true', async () => {
+      it('requests a switch chain approval with autoApprove and switches to it if autoApprove: true', async () => {
         const { mocks, switchChain } = createMockedSwitchChain();
-        mocks.isAddFlow = true;
+        mocks.autoApprove = true;
         mocks.getCaveat.mockReturnValue({
           value: {
             requiredScopes: {},
@@ -105,9 +94,9 @@ describe('Ethereum Chain Utils', () => {
         expect(mocks.setActiveNetwork).toHaveBeenCalledWith('mainnet');
       });
 
-      it('requests permittedChains approval without autoApprove then switches to it if isAddFlow: false', async () => {
+      it('requests permittedChains approval without autoApprove then switches to it if autoApprove: false', async () => {
         const { mocks, switchChain } = createMockedSwitchChain();
-        mocks.isAddFlow = false;
+        mocks.autoApprove = false;
         mocks.getCaveat.mockReturnValue({
           value: {
             requiredScopes: {},
@@ -123,10 +112,10 @@ describe('Ethereum Chain Utils', () => {
         expect(mocks.setActiveNetwork).toHaveBeenCalledWith('mainnet');
       });
 
-      it('should handle errors if the permittedChains grant fails', async () => {
+      it('should throw errors if the permittedChains grant fails', async () => {
         const { mocks, end, switchChain } = createMockedSwitchChain();
         mocks.requestPermittedChainsPermissionIncrementalForOrigin.mockRejectedValueOnce(
-          new Error('failed to grant permittedChains'),
+          new Error('failed to auto grant permittedChains'),
         );
         mocks.getCaveat.mockReturnValue({
           value: {
@@ -142,7 +131,7 @@ describe('Ethereum Chain Utils', () => {
         ).toHaveBeenCalled();
         expect(mocks.setActiveNetwork).not.toHaveBeenCalled();
         expect(end).toHaveBeenCalledWith(
-          new Error('failed to grant permittedChains'),
+          new Error('failed to auto grant permittedChains'),
         );
       });
     });