From 1eaeeb07fa713e91153878477dd45a0d2fa68160 Mon Sep 17 00:00:00 2001 From: jiexi Date: Thu, 20 Jun 2024 16:22:36 -0700 Subject: [PATCH] v12 cherrypick: Bump `queued-request-controller` to `^1.0.0` (#25310) (#25451) cherrypicks https://github.com/MetaMask/metamask-extension/commit/ba8d84eafd6012457e2cc7276473e5de5c03990d which bumps `queued-request-controller` to `^1.0.0` and fixes the `eth_requestAccounts` handling bug Merge conflicts on the lavamoat policies Fixes: https://github.com/MetaMask/metamask-extension/issues/25407 --------- Co-authored-by: MetaMask Bot --- app/scripts/metamask-controller.js | 16 +- lavamoat/browserify/beta/policy.json | 9 +- lavamoat/browserify/desktop/policy.json | 9 +- lavamoat/browserify/flask/policy.json | 9 +- lavamoat/browserify/main/policy.json | 9 +- lavamoat/browserify/mmi/policy.json | 9 +- package.json | 2 +- ...-switch-dapp2-eth-request-accounts.spec.js | 151 ++++++++++++++++++ .../permissions-connect.component.js | 27 ---- yarn.lock | 20 +-- 10 files changed, 216 insertions(+), 45 deletions(-) create mode 100644 test/e2e/tests/request-queuing/dapp1-switch-dapp2-eth-request-accounts.spec.js diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index a97553ddd6ab..0731a771a3d0 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -449,7 +449,8 @@ export default class MetamaskController extends EventEmitter { ], allowedEvents: ['SelectedNetworkController:stateChange'], }), - methodsRequiringNetworkSwitch, + shouldRequestSwitchNetwork: ({ method }) => + methodsRequiringNetworkSwitch.includes(method), clearPendingConfirmations, }); @@ -5113,7 +5114,18 @@ export default class MetamaskController extends EventEmitter { useRequestQueue: this.preferencesController.getUseRequestQueue.bind( this.preferencesController, ), - methodsWithConfirmation, + shouldEnqueueRequest: (request) => { + if ( + request.method === 'eth_requestAccounts' && + this.permissionController.hasPermission( + request.origin, + PermissionNames.eth_accounts, + ) + ) { + return false; + } + return methodsWithConfirmation.includes(request.method); + }, }); engine.push(requestQueueMiddleware); diff --git a/lavamoat/browserify/beta/policy.json b/lavamoat/browserify/beta/policy.json index 078a6e778e3f..ceda4ecc16ff 100644 --- a/lavamoat/browserify/beta/policy.json +++ b/lavamoat/browserify/beta/policy.json @@ -1990,9 +1990,9 @@ }, "@metamask/queued-request-controller": { "packages": { - "@metamask/network-controller>@metamask/json-rpc-engine": true, "@metamask/providers>@metamask/rpc-errors": true, "@metamask/queued-request-controller>@metamask/base-controller": true, + "@metamask/queued-request-controller>@metamask/json-rpc-engine": true, "@metamask/selected-network-controller": true, "@metamask/utils": true } @@ -2005,6 +2005,13 @@ "immer": true } }, + "@metamask/queued-request-controller>@metamask/json-rpc-engine": { + "packages": { + "@metamask/providers>@metamask/rpc-errors": true, + "@metamask/safe-event-emitter": true, + "@metamask/utils": true + } + }, "@metamask/rpc-methods-flask>nanoid": { "globals": { "crypto.getRandomValues": true diff --git a/lavamoat/browserify/desktop/policy.json b/lavamoat/browserify/desktop/policy.json index 2c42b202f01b..5dc9e06d53c0 100644 --- a/lavamoat/browserify/desktop/policy.json +++ b/lavamoat/browserify/desktop/policy.json @@ -2162,9 +2162,9 @@ }, "@metamask/queued-request-controller": { "packages": { - "@metamask/network-controller>@metamask/json-rpc-engine": true, "@metamask/providers>@metamask/rpc-errors": true, "@metamask/queued-request-controller>@metamask/base-controller": true, + "@metamask/queued-request-controller>@metamask/json-rpc-engine": true, "@metamask/selected-network-controller": true, "@metamask/utils": true } @@ -2177,6 +2177,13 @@ "immer": true } }, + "@metamask/queued-request-controller>@metamask/json-rpc-engine": { + "packages": { + "@metamask/providers>@metamask/rpc-errors": true, + "@metamask/safe-event-emitter": true, + "@metamask/utils": true + } + }, "@metamask/rate-limit-controller": { "globals": { "setTimeout": true diff --git a/lavamoat/browserify/flask/policy.json b/lavamoat/browserify/flask/policy.json index 943501f5d897..99decf305b07 100644 --- a/lavamoat/browserify/flask/policy.json +++ b/lavamoat/browserify/flask/policy.json @@ -2214,9 +2214,9 @@ }, "@metamask/queued-request-controller": { "packages": { - "@metamask/network-controller>@metamask/json-rpc-engine": true, "@metamask/providers>@metamask/rpc-errors": true, "@metamask/queued-request-controller>@metamask/base-controller": true, + "@metamask/queued-request-controller>@metamask/json-rpc-engine": true, "@metamask/selected-network-controller": true, "@metamask/utils": true } @@ -2229,6 +2229,13 @@ "immer": true } }, + "@metamask/queued-request-controller>@metamask/json-rpc-engine": { + "packages": { + "@metamask/providers>@metamask/rpc-errors": true, + "@metamask/safe-event-emitter": true, + "@metamask/utils": true + } + }, "@metamask/rate-limit-controller": { "globals": { "setTimeout": true diff --git a/lavamoat/browserify/main/policy.json b/lavamoat/browserify/main/policy.json index 1b40c94878aa..4a35b2301dbe 100644 --- a/lavamoat/browserify/main/policy.json +++ b/lavamoat/browserify/main/policy.json @@ -2069,9 +2069,9 @@ }, "@metamask/queued-request-controller": { "packages": { - "@metamask/network-controller>@metamask/json-rpc-engine": true, "@metamask/providers>@metamask/rpc-errors": true, "@metamask/queued-request-controller>@metamask/base-controller": true, + "@metamask/queued-request-controller>@metamask/json-rpc-engine": true, "@metamask/selected-network-controller": true, "@metamask/utils": true } @@ -2084,6 +2084,13 @@ "immer": true } }, + "@metamask/queued-request-controller>@metamask/json-rpc-engine": { + "packages": { + "@metamask/providers>@metamask/rpc-errors": true, + "@metamask/safe-event-emitter": true, + "@metamask/utils": true + } + }, "@metamask/rate-limit-controller": { "globals": { "setTimeout": true diff --git a/lavamoat/browserify/mmi/policy.json b/lavamoat/browserify/mmi/policy.json index fa2ba932f48f..2e16735c03ff 100644 --- a/lavamoat/browserify/mmi/policy.json +++ b/lavamoat/browserify/mmi/policy.json @@ -2354,9 +2354,9 @@ }, "@metamask/queued-request-controller": { "packages": { - "@metamask/network-controller>@metamask/json-rpc-engine": true, "@metamask/providers>@metamask/rpc-errors": true, "@metamask/queued-request-controller>@metamask/base-controller": true, + "@metamask/queued-request-controller>@metamask/json-rpc-engine": true, "@metamask/selected-network-controller": true, "@metamask/utils": true } @@ -2369,6 +2369,13 @@ "immer": true } }, + "@metamask/queued-request-controller>@metamask/json-rpc-engine": { + "packages": { + "@metamask/providers>@metamask/rpc-errors": true, + "@metamask/safe-event-emitter": true, + "@metamask/utils": true + } + }, "@metamask/rate-limit-controller": { "globals": { "setTimeout": true diff --git a/package.json b/package.json index df815c18f2ce..b1bd0fe251dd 100644 --- a/package.json +++ b/package.json @@ -329,7 +329,7 @@ "@metamask/post-message-stream": "^8.0.0", "@metamask/ppom-validator": "^0.30.0", "@metamask/providers": "^14.0.2", - "@metamask/queued-request-controller": "^0.10.0", + "@metamask/queued-request-controller": "^1.0.0", "@metamask/rate-limit-controller": "^5.0.1", "@metamask/safe-event-emitter": "^3.1.1", "@metamask/scure-bip39": "^2.0.3", diff --git a/test/e2e/tests/request-queuing/dapp1-switch-dapp2-eth-request-accounts.spec.js b/test/e2e/tests/request-queuing/dapp1-switch-dapp2-eth-request-accounts.spec.js new file mode 100644 index 000000000000..60690d4ab66b --- /dev/null +++ b/test/e2e/tests/request-queuing/dapp1-switch-dapp2-eth-request-accounts.spec.js @@ -0,0 +1,151 @@ +const { strict: assert } = require('assert'); + +const FixtureBuilder = require('../../fixture-builder'); +const { + withFixtures, + openDapp, + unlockWallet, + DAPP_URL, + DAPP_ONE_URL, + regularDelayMs, + WINDOW_TITLES, + defaultGanacheOptions, + switchToNotificationWindow, +} = require('../../helpers'); + +describe('Request Queuing Dapp 1 Send Tx -> Dapp 2 Request Accounts Tx', function () { + it('should queue `eth_requestAccounts` requests when the requesting dapp does not already have connected accounts', async function () { + const port = 8546; + const chainId = 1338; + await withFixtures( + { + dapp: true, + fixtures: new FixtureBuilder() + .withNetworkControllerDoubleGanache() + .withPreferencesControllerUseRequestQueueEnabled() + .withPermissionControllerConnectedToTestDapp() + .build(), + dappOptions: { numberOfDapps: 2 }, + ganacheOptions: { + ...defaultGanacheOptions, + concurrent: [ + { + port, + chainId, + ganacheOptions2: defaultGanacheOptions, + }, + ], + }, + title: this.test.fullTitle(), + }, + async ({ driver }) => { + await unlockWallet(driver); + + // Open Dapp One + await openDapp(driver, undefined, DAPP_URL); + + // Dapp Send Button + await driver.clickElement('#sendButton'); + + // Leave the confirmation pending + + await openDapp(driver, undefined, DAPP_ONE_URL); + + const accountsOnload = await ( + await driver.findElement('#accounts') + ).getText(); + assert.deepStrictEqual(accountsOnload, ''); + + await driver.findClickableElement({ text: 'Connect', tag: 'button' }); + await driver.clickElement('#connectButton'); + + await driver.delay(regularDelayMs); + + const accountsBeforeConnect = await ( + await driver.findElement('#accounts') + ).getText(); + assert.deepStrictEqual(accountsBeforeConnect, ''); + + // Reject the pending confirmation from the first dapp + await switchToNotificationWindow(driver); + await driver.clickElement({ text: 'Reject', tag: 'button' }); + + // Wait for switch confirmation to close then request accounts confirmation to show for the second dapp + await driver.delay(regularDelayMs); + await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); + + await driver.clickElement({ + text: 'Next', + tag: 'button', + css: '[data-testid="page-container-footer-next"]', + }); + + await driver.clickElement({ + text: 'Confirm', + tag: 'button', + css: '[data-testid="page-container-footer-next"]', + }); + + await driver.switchToWindowWithUrl(DAPP_ONE_URL); + + await driver.waitForSelector({ + text: '0x5cfe73b6021e818b776b421b1c4db2474086a7e1', + css: '#accounts', + }); + }, + ); + }); + + it('should not queue the `eth_requestAccounts` requests when the requesting dapp already has connected accounts', async function () { + const port = 8546; + const chainId = 1338; + await withFixtures( + { + dapp: true, + fixtures: new FixtureBuilder() + .withNetworkControllerDoubleGanache() + .withPreferencesControllerUseRequestQueueEnabled() + .withPermissionControllerConnectedToTwoTestDapps() + .build(), + dappOptions: { numberOfDapps: 2 }, + ganacheOptions: { + ...defaultGanacheOptions, + concurrent: [ + { + port, + chainId, + ganacheOptions2: defaultGanacheOptions, + }, + ], + }, + title: this.test.fullTitle(), + }, + async ({ driver }) => { + await unlockWallet(driver); + + // Open Dapp One + await openDapp(driver, undefined, DAPP_URL); + + // Dapp Send Button + await driver.clickElement('#sendButton'); + + // Leave the confirmation pending + + await openDapp(driver, undefined, DAPP_ONE_URL); + + const ethRequestAccounts = JSON.stringify({ + jsonrpc: '2.0', + method: 'eth_requestAccounts', + }); + + const accounts = await driver.executeScript( + `return window.ethereum.request(${ethRequestAccounts})`, + ); + + assert.deepStrictEqual(accounts, [ + '0x5cfe73b6021e818b776b421b1c4db2474086a7e1', + ]); + }, + ); + }); +}); diff --git a/ui/pages/permissions-connect/permissions-connect.component.js b/ui/pages/permissions-connect/permissions-connect.component.js index c147bb5cd67f..2dd1982f1abc 100644 --- a/ui/pages/permissions-connect/permissions-connect.component.js +++ b/ui/pages/permissions-connect/permissions-connect.component.js @@ -5,8 +5,6 @@ import { Switch, Route } from 'react-router-dom'; import { ethErrors, serializeError } from 'eth-rpc-errors'; import { SubjectType } from '@metamask/permission-controller'; ///: END:ONLY_INCLUDE_IF -import { getEnvironmentType } from '../../../app/scripts/lib/util'; -import { ENVIRONMENT_TYPE_NOTIFICATION } from '../../../shared/constants/app'; import { MILLISECOND } from '../../../shared/constants/time'; import { DEFAULT_ROUTE } from '../../helpers/constants/routes'; import PermissionPageContainer from '../../components/app/permission-page-container'; @@ -129,22 +127,6 @@ export default class PermissionConnect extends Component { ///: END:ONLY_INCLUDE_IF }; - beforeUnload = () => { - const { permissionsRequestId, rejectPermissionsRequest } = this.props; - const { permissionsApproved } = this.state; - - if (permissionsApproved === null && permissionsRequestId) { - rejectPermissionsRequest(permissionsRequestId); - } - }; - - removeBeforeUnload = () => { - const environmentType = getEnvironmentType(); - if (environmentType === ENVIRONMENT_TYPE_NOTIFICATION) { - window.removeEventListener('beforeunload', this.beforeUnload); - } - }; - componentDidMount() { const { connectPath, @@ -168,11 +150,6 @@ export default class PermissionConnect extends Component { return; } - const environmentType = getEnvironmentType(); - if (environmentType === ENVIRONMENT_TYPE_NOTIFICATION) { - window.addEventListener('beforeunload', this.beforeUnload); - } - if (history.location.pathname === connectPath && !isRequestingAccounts) { ///: BEGIN:ONLY_INCLUDE_IF(snaps) @@ -274,7 +251,6 @@ export default class PermissionConnect extends Component { redirecting: shouldRedirect, permissionsApproved: approved, }); - this.removeBeforeUnload(); if (shouldRedirect && approved) { setTimeout(() => history.push(DEFAULT_ROUTE), APPROVE_TIMEOUT); @@ -471,7 +447,6 @@ export default class PermissionConnect extends Component { serializeError(ethErrors.provider.userRejectedRequest()), ); this.setState({ permissionsApproved: true }); - this.removeBeforeUnload(); }} targetSubjectMetadata={targetSubjectMetadata} /> @@ -504,7 +479,6 @@ export default class PermissionConnect extends Component { serializeError(ethErrors.provider.userRejectedRequest()), ); this.setState({ permissionsApproved: false }); - this.removeBeforeUnload(); }} targetSubjectMetadata={targetSubjectMetadata} /> @@ -526,7 +500,6 @@ export default class PermissionConnect extends Component { approveSnapResult={(requestId) => { approvePendingApproval(requestId); this.setState({ permissionsApproved: true }); - this.removeBeforeUnload(); }} targetSubjectMetadata={targetSubjectMetadata} /> diff --git a/yarn.lock b/yarn.lock index 8cb2c1fb06f8..d18a17c67c4a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6086,20 +6086,20 @@ __metadata: languageName: node linkType: hard -"@metamask/queued-request-controller@npm:^0.10.0": - version: 0.10.0 - resolution: "@metamask/queued-request-controller@npm:0.10.0" +"@metamask/queued-request-controller@npm:^1.0.0": + version: 1.0.0 + resolution: "@metamask/queued-request-controller@npm:1.0.0" dependencies: - "@metamask/base-controller": "npm:^5.0.2" - "@metamask/controller-utils": "npm:^9.1.0" - "@metamask/json-rpc-engine": "npm:^8.0.2" + "@metamask/base-controller": "npm:^6.0.0" + "@metamask/controller-utils": "npm:^11.0.0" + "@metamask/json-rpc-engine": "npm:^9.0.0" "@metamask/rpc-errors": "npm:^6.2.1" "@metamask/swappable-obj-proxy": "npm:^2.2.0" "@metamask/utils": "npm:^8.3.0" peerDependencies: - "@metamask/network-controller": ^18.0.0 - "@metamask/selected-network-controller": ^13.0.0 - checksum: 10/b6caba5889b1e6219d33a5e80c6a3baf77c64b52cfdde8123f4e44d21ef501b78c49e111b050bb4aacd0bf12bd3593f88058f2a230ff5693d7b9d8ff1521e5bc + "@metamask/network-controller": ^19.0.0 + "@metamask/selected-network-controller": ^15.0.0 + checksum: 10/84b5442035ef4843ad5c3effdb961c9725c07fa3caf37928c4315ffb4929160a1032b8e0f814061fa8487f499d147766f3cfdd4172c8a3941c2996b11628a46b languageName: node linkType: hard @@ -25092,7 +25092,7 @@ __metadata: "@metamask/post-message-stream": "npm:^8.0.0" "@metamask/ppom-validator": "npm:^0.30.0" "@metamask/providers": "npm:^14.0.2" - "@metamask/queued-request-controller": "npm:^0.10.0" + "@metamask/queued-request-controller": "npm:^1.0.0" "@metamask/rate-limit-controller": "npm:^5.0.1" "@metamask/safe-event-emitter": "npm:^3.1.1" "@metamask/scure-bip39": "npm:^2.0.3"