From 305ea7ef12313c0c10ad9c1b531d3ace10d04292 Mon Sep 17 00:00:00 2001 From: jiexi Date: Thu, 23 Jan 2025 13:06:31 -0800 Subject: [PATCH] fix: fix permissions not correctly being updated when all network clients are removed for a chainId (#29855) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **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: https://github.com/MetaMask/MetaMask-planning/issues/3735 **Requires:** https://github.com/MetaMask/core/pull/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** ### **Before** ### **After** ## **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 --- app/scripts/metamask-controller.js | 7 + app/scripts/metamask-controller.test.js | 19 ++ test/e2e/fixture-builder.js | 4 +- .../json-rpc/wallet_revokePermissions.spec.ts | 6 +- test/e2e/tests/network/remove-network.spec.ts | 223 ++++++++++++++++++ 5 files changed, 254 insertions(+), 5 deletions(-) create mode 100644 test/e2e/tests/network/remove-network.spec.ts diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 7e5e11ae32ed..684f7b4ca32a 100644 --- a/app/scripts/metamask-controller.js +++ b/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 () => { diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js index 096e7799192d..a5c4e02bad21 100644 --- a/app/scripts/metamask-controller.test.js +++ b/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(); diff --git a/test/e2e/fixture-builder.js b/test/e2e/fixture-builder.js index 60cbdea72d2c..27f1976c8f84 100644 --- a/test/e2e/fixture-builder.js +++ b/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, diff --git a/test/e2e/json-rpc/wallet_revokePermissions.spec.ts b/test/e2e/json-rpc/wallet_revokePermissions.spec.ts index d1ac0d39f580..1f7758db3fc8 100644 --- a/test/e2e/json-rpc/wallet_revokePermissions.spec.ts +++ b/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(), }, diff --git a/test/e2e/tests/network/remove-network.spec.ts b/test/e2e/tests/network/remove-network.spec.ts new file mode 100644 index 000000000000..9d522156024c --- /dev/null +++ b/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']); + }, + ); + }); +});