fix: fix permissions not correctly being updated when all network cli…

…ents are removed for a chainId (#29855)

<!--
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 a bug where permissions are not being updated when all network
clients for a chainId are removed. This results in permittedChains
permissions referencing chainIds that are no longer supported and makes
it so that the existing permittedChains permissions cannot be modified
in any way until either the permission is revoked entirely or the
missing network is readded.

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

## **Related issues**

Fixes: MetaMask/MetaMask-planning#3735

**Requires:** MetaMask/core#5183

## **Manual testing steps**

1. Add a new RPC endpoint for a chainId you do not have network clients
for already
2. Add another different RPC endpoint for that same chainId above
3. Permit a dapp to access that chainId
4. Check that you can modify the permitted chains vs the wallet UI
5. Make sure that calling `wallet_getPermissions` shows the chainId in
the `endowment:permitted-chains` permission
6. Remove one RPC endpoint for that chainId
7. Check that you can modify the permitted chains vs the wallet UI
8. Make sure that calling `wallet_getPermissions` shows the chainId in
the `endowment:permitted-chains` permission
9. Remove the chainId entirely
7. Check that you can modify the permitted chains vs the wallet UI
8. Make sure that calling `wallet_getPermissions` shows that the chainId
is no longer in the `endowment:permitted-chains` permission

## **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]>

app/scripts/metamask-controller.js

@@ -3144,6 +3144,13 @@ export default class MetamaskController extends EventEmitter {
       getPermittedChainsByOrigin,
     );
 
+    this.controllerMessenger.subscribe(
+      'NetworkController:networkRemoved',
+      ({ chainId }) => {
+        this.removeAllChainIdPermissions(chainId);
+      },
+    );
+
     this.controllerMessenger.subscribe(
       'NetworkController:networkDidChange',
       async () => {

app/scripts/metamask-controller.test.js

@@ -2063,6 +2063,25 @@ describe('MetaMaskController', () => {
       });
     });
 
+    describe('NetworkConfiguration is removed', () => {
+      it('should remove the permitted chain from all existing permissions', () => {
+        jest
+          .spyOn(metamaskController, 'removeAllChainIdPermissions')
+          .mockReturnValue();
+
+        metamaskController.controllerMessenger.publish(
+          'NetworkController:networkRemoved',
+          {
+            chainId: '0xdeadbeef',
+          },
+        );
+
+        expect(
+          metamaskController.removeAllChainIdPermissions,
+        ).toHaveBeenCalledWith('0xdeadbeef');
+      });
+    });
+
     describe('#getApi', () => {
       it('getState', () => {
         const getApi = metamaskController.getApi();

test/e2e/fixture-builder.js

@@ -488,7 +488,7 @@ class FixtureBuilder {
     });
   }
 
-  withPermissionControllerConnectedToTestDappWithChain() {
+  withPermissionControllerConnectedToTestDappWithChains(chainIds) {
     return this.withPermissionController({
       subjects: {
         [DAPP_URL]: {
@@ -513,7 +513,7 @@ class FixtureBuilder {
               caveats: [
                 {
                   type: 'restrictNetworkSwitching',
-                  value: ['0x539'],
+                  value: chainIds,
                 },
               ],
               date: 1664388714637,

test/e2e/json-rpc/wallet_revokePermissions.spec.ts

@@ -11,7 +11,7 @@ describe('Revoke Dapp Permissions', function () {
       {
         dapp: true,
         fixtures: new FixtureBuilder()
-          .withPermissionControllerConnectedToTestDappWithChain()
+          .withPermissionControllerConnectedToTestDappWithChains(['0x539'])
           .build(),
         title: this.test?.fullTitle(),
       },
@@ -69,7 +69,7 @@ describe('Revoke Dapp Permissions', function () {
       {
         dapp: true,
         fixtures: new FixtureBuilder()
-          .withPermissionControllerConnectedToTestDappWithChain()
+          .withPermissionControllerConnectedToTestDappWithChains(['0x539'])
           .build(),
         title: this.test?.fullTitle(),
       },
@@ -127,7 +127,7 @@ describe('Revoke Dapp Permissions', function () {
       {
         dapp: true,
         fixtures: new FixtureBuilder()
-          .withPermissionControllerConnectedToTestDappWithChain()
+          .withPermissionControllerConnectedToTestDappWithChains(['0x539'])
           .build(),
         title: this.test?.fullTitle(),
       },

test/e2e/tests/network/remove-network.spec.ts

@@ -0,0 +1,223 @@
+import { strict as assert } from 'assert';
+import { Suite } from 'mocha';
+import {
+  CaveatConstraint,
+  PermissionConstraint,
+} from '@metamask/permission-controller';
+import FixtureBuilder from '../../fixture-builder';
+import {
+  defaultGanacheOptions,
+  openDapp,
+  regularDelayMs,
+  unlockWallet,
+  WINDOW_TITLES,
+  withFixtures,
+} from '../../helpers';
+import { Driver } from '../../webdriver/driver';
+import { PermissionNames } from '../../../../app/scripts/controllers/permissions';
+import { CaveatTypes } from '../../../../shared/constants/permissions';
+
+const getPermittedChains = async (driver: Driver) => {
+  const getPermissionsRequest = JSON.stringify({
+    method: 'wallet_getPermissions',
+  });
+  const getPermissionsResult = await driver.executeScript(
+    `return window.ethereum.request(${getPermissionsRequest})`,
+  );
+
+  const permittedChains =
+    getPermissionsResult
+      ?.find(
+        (permission: PermissionConstraint) =>
+          permission.parentCapability === PermissionNames.permittedChains,
+      )
+      ?.caveats.find(
+        (caveat: CaveatConstraint) =>
+          caveat.type === CaveatTypes.restrictNetworkSwitching,
+      )?.value || [];
+
+  return permittedChains;
+};
+
+describe('Remove Network:', function (this: Suite) {
+  it('should remove the chainId from existing permissions when a network configuration is removed entirely', async function () {
+    await withFixtures(
+      {
+        dapp: true,
+        fixtures: new FixtureBuilder()
+          .withPermissionControllerConnectedToTestDappWithChains([
+            '0x539',
+            '0x53a',
+          ])
+          .withNetworkController({
+            providerConfig: {
+              rpcPrefs: { blockExplorerUrl: 'https://etherscan.io/' },
+            },
+            networkConfigurations: {
+              networkConfigurationId: {
+                chainId: '0x539',
+                nickname: 'Localhost 8545',
+                rpcUrl: 'http://localhost:8545',
+                ticker: 'ETH',
+                rpcPrefs: { blockExplorerUrl: 'https://etherscan.io/' },
+              },
+              '2ce66016-8aab-47df-b27f-318c80865eb0': {
+                chainId: '0x53a',
+                id: '2ce66016-8aab-47df-b27f-318c80865eb0',
+                nickname: 'Localhost 8546',
+                rpcPrefs: {},
+                rpcUrl: 'http://localhost:8546',
+                ticker: 'ETH',
+              },
+            },
+            selectedNetworkClientId: 'networkConfigurationId',
+          })
+          .build(),
+        ganacheOptions: {
+          ...defaultGanacheOptions,
+          concurrent: [{ port: 8546, chainId: 1338 }],
+        },
+        title: this.test?.fullTitle(),
+      },
+      async ({ driver }: { driver: Driver }) => {
+        await unlockWallet(driver);
+        await openDapp(driver);
+
+        const beforePermittedChains = await getPermittedChains(driver);
+
+        assert.deepEqual(beforePermittedChains, ['0x539', '0x53a']);
+
+        await driver.switchToWindowWithTitle(
+          WINDOW_TITLES.ExtensionInFullScreenView,
+        );
+
+        // Avoid a stale element error
+        await driver.delay(regularDelayMs);
+        await driver.clickElement('[data-testid="network-display"]');
+
+        // Go to Edit Menu
+        await driver.clickElement(
+          '[data-testid="network-list-item-options-button-0x53a"]',
+        );
+
+        await driver.delay(regularDelayMs);
+        await driver.clickElement(
+          '[data-testid="network-list-item-options-delete"]',
+        );
+
+        await driver.delay(regularDelayMs);
+        await driver.clickElement({ text: 'Delete', tag: 'button' });
+
+        await driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp);
+
+        const afterPermittedChains = await getPermittedChains(driver);
+
+        assert.deepEqual(afterPermittedChains, ['0x539']);
+      },
+    );
+  });
+
+  it('should not remove the chainId from existing permissions when a network client is removed but other network clients still exist for the chainId', async function () {
+    await withFixtures(
+      {
+        dapp: true,
+        fixtures: new FixtureBuilder()
+          .withPermissionControllerConnectedToTestDappWithChains([
+            '0x539',
+            '0x53a',
+          ])
+          .withNetworkController({
+            providerConfig: {
+              rpcPrefs: { blockExplorerUrl: 'https://etherscan.io/' },
+            },
+            networkConfigurations: {
+              networkConfigurationId: {
+                chainId: '0x539',
+                nickname: 'Localhost 8545',
+                rpcUrl: 'http://localhost:8545',
+                ticker: 'ETH',
+                rpcPrefs: { blockExplorerUrl: 'https://etherscan.io/' },
+              },
+              '2ce66016-8aab-47df-b27f-318c80865eb0': {
+                chainId: '0x53a',
+                id: '2ce66016-8aab-47df-b27f-318c80865eb0',
+                nickname: 'Localhost 8546',
+                rpcPrefs: {},
+                rpcUrl: 'http://localhost:8546',
+                ticker: 'ETH',
+              },
+              '2ce66016-8aab-47df-b27f-318c80865eb1': {
+                chainId: '0x53a',
+                id: '2ce66016-8aab-47df-b27f-318c80865eb1',
+                nickname: 'Localhost 8546 alternative',
+                rpcPrefs: {},
+                rpcUrl: 'http://127.0.0.1:8546',
+                ticker: 'ETH',
+              },
+            },
+            selectedNetworkClientId: 'networkConfigurationId',
+          })
+          .build(),
+        ganacheOptions: {
+          ...defaultGanacheOptions,
+          concurrent: [{ port: 8546, chainId: 1338 }],
+        },
+        title: this.test?.fullTitle(),
+      },
+      async ({ driver }: { driver: Driver }) => {
+        await unlockWallet(driver);
+        await openDapp(driver);
+
+        const beforePermittedChains = await getPermittedChains(driver);
+
+        assert.deepEqual(beforePermittedChains, ['0x539', '0x53a']);
+
+        await driver.switchToWindowWithTitle(
+          WINDOW_TITLES.ExtensionInFullScreenView,
+        );
+
+        // Avoid a stale element error
+        await driver.delay(regularDelayMs);
+        await driver.clickElement('[data-testid="network-display"]');
+
+        // Go to Edit Menu
+        await driver.clickElement(
+          '[data-testid="network-list-item-options-button-0x53a"]',
+        );
+
+        await driver.delay(regularDelayMs);
+        await driver.clickElement(
+          '[data-testid="network-list-item-options-edit"]',
+        );
+
+        await driver.delay(regularDelayMs);
+        await driver.clickElement('[data-testid="test-add-rpc-drop-down"]');
+        await driver.delay(regularDelayMs);
+
+        // Assert the endpoint is in the list
+        await driver.findElement({
+          text: '127.0.0.1:8546',
+          tag: 'p',
+        });
+
+        // Delete it
+        await driver.clickElement('[data-testid="delete-item-1"]');
+
+        // Verify it went away
+        await driver.assertElementNotPresent({
+          text: '127.0.0.1:8546',
+          tag: 'p',
+        });
+
+        // Save the network
+        await driver.clickElement({ text: 'Save', tag: 'button' });
+
+        await driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp);
+
+        const afterPermittedChains = await getPermittedChains(driver);
+
+        assert.deepEqual(afterPermittedChains, ['0x539', '0x53a']);
+      },
+    );
+  });
+});