From a6a91d5a84d5b188579346c4880cdaea62d1ebb4 Mon Sep 17 00:00:00 2001 From: jiexi Date: Thu, 16 Jan 2025 14:10:47 -0800 Subject: [PATCH 1/2] test: Cleanup snap-account-signature e2e tests. Add permittedChains scenario to wallet_revokePermissions e2e test (#29761) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** * Fix incorrect snap-account-signature e2e test fixtures / starting state (accounts permissioned before they exist in the wallet) * Add `endowment:permitted-chains` scenario to `wallet_revokePermissions` e2e test [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29761?quickstart=1) ## **Related issues** See: https://github.com/MetaMask/metamask-extension/pull/27847 ## **Manual testing steps** 1. Go to this page... 2. 3. ## **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 Co-authored-by: Mark Stacey --- test/e2e/fixture-builder.js | 36 +++++++ .../json-rpc/wallet_revokePermissions.spec.ts | 100 ++++++++++++++++-- ...account-signatures-and-disconnects.spec.ts | 11 +- .../account/snap-account-signatures.spec.ts | 12 +-- 4 files changed, 138 insertions(+), 21 deletions(-) diff --git a/test/e2e/fixture-builder.js b/test/e2e/fixture-builder.js index 6a629f97f503..fbb68ad598de 100644 --- a/test/e2e/fixture-builder.js +++ b/test/e2e/fixture-builder.js @@ -494,6 +494,42 @@ class FixtureBuilder { }); } + withPermissionControllerConnectedToTestDappWithChain() { + return this.withPermissionController({ + subjects: { + [DAPP_URL]: { + origin: DAPP_URL, + permissions: { + eth_accounts: { + id: 'ZaqPEWxyhNCJYACFw93jE', + parentCapability: 'eth_accounts', + invoker: DAPP_URL, + caveats: [ + { + type: 'restrictReturnedAccounts', + value: [DEFAULT_FIXTURE_ACCOUNT.toLowerCase()], + }, + ], + date: 1664388714636, + }, + 'endowment:permitted-chains': { + id: 'D7cac0a2e3BD8f349506a', + parentCapability: 'endowment:permitted-chains', + invoker: DAPP_URL, + caveats: [ + { + type: 'restrictNetworkSwitching', + value: ['0x539'], + }, + ], + date: 1664388714637, + }, + }, + }, + }, + }); + } + withPermissionControllerConnectedToTestDappWithTwoAccounts() { const subjects = { [DAPP_URL]: { diff --git a/test/e2e/json-rpc/wallet_revokePermissions.spec.ts b/test/e2e/json-rpc/wallet_revokePermissions.spec.ts index 5c444b5ecf01..8d7a06ac4ccc 100644 --- a/test/e2e/json-rpc/wallet_revokePermissions.spec.ts +++ b/test/e2e/json-rpc/wallet_revokePermissions.spec.ts @@ -1,16 +1,17 @@ import { strict as assert } from 'assert'; -import { ACCOUNT_1, withFixtures } from '../helpers'; +import { PermissionConstraint } from '@metamask/permission-controller'; +import { withFixtures } from '../helpers'; import FixtureBuilder from '../fixture-builder'; import TestDapp from '../page-objects/pages/test-dapp'; import { loginWithBalanceValidation } from '../page-objects/flows/login.flow'; describe('Revoke Dapp Permissions', function () { - it('should revoke dapp permissions ', async function () { + it('should revoke dapp permissions for "eth_accounts"', async function () { await withFixtures( { dapp: true, fixtures: new FixtureBuilder() - .withPermissionControllerConnectedToTestDapp() + .withPermissionControllerConnectedToTestDappWithChain() .build(), title: this.test?.fullTitle(), }, @@ -18,9 +19,22 @@ describe('Revoke Dapp Permissions', function () { await loginWithBalanceValidation(driver); const testDapp = new TestDapp(driver); await testDapp.openTestDappPage(); - await testDapp.check_connectedAccounts(ACCOUNT_1); - // wallet_revokePermissions request + const beforeGetPermissionsRequest = JSON.stringify({ + jsonrpc: '2.0', + method: 'wallet_getPermissions', + }); + const beforeGetPermissionsResult = await driver.executeScript( + `return window.ethereum.request(${beforeGetPermissionsRequest})`, + ); + const beforeGetPermissionsNames = beforeGetPermissionsResult.map( + (permission: PermissionConstraint) => permission.parentCapability, + ); + assert.deepEqual(beforeGetPermissionsNames, [ + 'eth_accounts', + 'endowment:permitted-chains', + ]); + const revokePermissionsRequest = JSON.stringify({ jsonrpc: '2.0', method: 'wallet_revokePermissions', @@ -30,14 +44,82 @@ describe('Revoke Dapp Permissions', function () { }, ], }); + const revokePermissionsResult = await driver.executeScript( + `return window.ethereum.request(${revokePermissionsRequest})`, + ); + assert.deepEqual(revokePermissionsResult, null); + + const afterGetPermissionsRequest = JSON.stringify({ + jsonrpc: '2.0', + method: 'wallet_getPermissions', + }); + const afterGetPermissionsResult = await driver.executeScript( + `return window.ethereum.request(${afterGetPermissionsRequest})`, + ); + const afterGetPermissionsNames = afterGetPermissionsResult.map( + (permission: PermissionConstraint) => permission.parentCapability, + ); + assert.deepEqual(afterGetPermissionsNames, [ + 'endowment:permitted-chains', + ]); + }, + ); + }); + + it('should revoke dapp permissions for "endowment:permitted-chains"', async function () { + await withFixtures( + { + dapp: true, + fixtures: new FixtureBuilder() + .withPermissionControllerConnectedToTestDappWithChain() + .build(), + title: this.test?.fullTitle(), + }, + async ({ driver }) => { + await loginWithBalanceValidation(driver); + const testDapp = new TestDapp(driver); + await testDapp.openTestDappPage(); + + const beforeGetPermissionsRequest = JSON.stringify({ + jsonrpc: '2.0', + method: 'wallet_getPermissions', + }); + const beforeGetPermissionsResult = await driver.executeScript( + `return window.ethereum.request(${beforeGetPermissionsRequest})`, + ); + const beforeGetPermissionsNames = beforeGetPermissionsResult.map( + (permission: PermissionConstraint) => permission.parentCapability, + ); + assert.deepEqual(beforeGetPermissionsNames, [ + 'eth_accounts', + 'endowment:permitted-chains', + ]); - const result = await driver.executeScript( + const revokePermissionsRequest = JSON.stringify({ + jsonrpc: '2.0', + method: 'wallet_revokePermissions', + params: [ + { + 'endowment:permitted-chains': {}, + }, + ], + }); + const revokePermissionsResult = await driver.executeScript( `return window.ethereum.request(${revokePermissionsRequest})`, ); - // Response of method call - assert.deepEqual(result, null); + assert.deepEqual(revokePermissionsResult, null); - await testDapp.check_connectedAccounts(ACCOUNT_1, false); + const afterGetPermissionsRequest = JSON.stringify({ + jsonrpc: '2.0', + method: 'wallet_getPermissions', + }); + const afterGetPermissionsResult = await driver.executeScript( + `return window.ethereum.request(${afterGetPermissionsRequest})`, + ); + const afterGetPermissionsNames = afterGetPermissionsResult.map( + (permission: PermissionConstraint) => permission.parentCapability, + ); + assert.deepEqual(afterGetPermissionsNames, ['eth_accounts']); }, ); }); diff --git a/test/e2e/tests/account/snap-account-signatures-and-disconnects.spec.ts b/test/e2e/tests/account/snap-account-signatures-and-disconnects.spec.ts index d3c177ff7fdd..7a97130a3ef5 100644 --- a/test/e2e/tests/account/snap-account-signatures-and-disconnects.spec.ts +++ b/test/e2e/tests/account/snap-account-signatures-and-disconnects.spec.ts @@ -19,11 +19,7 @@ describe('Snap Account Signatures and Disconnects @no-mmi', function (this: Suit await withFixtures( { dapp: true, - fixtures: new FixtureBuilder() - .withPermissionControllerConnectedToTestDapp({ - restrictReturnedAccounts: false, - }) - .build(), + fixtures: new FixtureBuilder().build(), title: this.test?.fullTitle(), }, async ({ driver }: { driver: Driver }) => { @@ -49,9 +45,12 @@ describe('Snap Account Signatures and Disconnects @no-mmi', function (this: Suit await experimentalSettings.check_pageIsLoaded(); await experimentalSettings.toggleRedesignedSignature(); - // Open the Test Dapp and signTypedDataV3 + // Open the Test Dapp and connect const testDapp = new TestDapp(driver); await testDapp.openTestDappPage(); + await testDapp.connectAccount({ publicAddress: newPublicKey }); + + // SignedTypedDataV3 with Test Dapp await signTypedDataV3WithSnapAccount(driver, newPublicKey, false, true); // Disconnect from Test Dapp and reconnect to Test Dapp diff --git a/test/e2e/tests/account/snap-account-signatures.spec.ts b/test/e2e/tests/account/snap-account-signatures.spec.ts index e04987561fc5..dc9ef21d2e52 100644 --- a/test/e2e/tests/account/snap-account-signatures.spec.ts +++ b/test/e2e/tests/account/snap-account-signatures.spec.ts @@ -30,11 +30,7 @@ describe('Snap Account Signatures @no-mmi', function (this: Suite) { await withFixtures( { dapp: true, - fixtures: new FixtureBuilder() - .withPermissionControllerConnectedToTestDapp({ - restrictReturnedAccounts: false, - }) - .build(), + fixtures: new FixtureBuilder().build(), title, }, async ({ driver }: { driver: Driver }) => { @@ -62,8 +58,12 @@ describe('Snap Account Signatures @no-mmi', function (this: Suite) { await experimentalSettings.check_pageIsLoaded(); await experimentalSettings.toggleRedesignedSignature(); + // Connect the SSK account + const testDapp = new TestDapp(driver); + await testDapp.openTestDappPage(); + await testDapp.connectAccount({ publicAddress: newPublicKey }); + // Run all 5 signature types - await new TestDapp(driver).openTestDappPage(); await personalSignWithSnapAccount( driver, newPublicKey, From 8ac211699a9f4c316d8a22f62acb7c38cf9901cd Mon Sep 17 00:00:00 2001 From: jiexi Date: Thu, 16 Jan 2025 15:24:13 -0800 Subject: [PATCH 2/2] test: Remove unused `restrictReturnedAccounts` param option (#29767) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** * Removes the `restrictReturnedAccounts` param option in many of the fixtures for eth_accounts in the fixture builder since it is no longer used anywhere [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29767?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **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. --- test/e2e/fixture-builder.js | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/test/e2e/fixture-builder.js b/test/e2e/fixture-builder.js index fbb68ad598de..7074a22ea109 100644 --- a/test/e2e/fixture-builder.js +++ b/test/e2e/fixture-builder.js @@ -463,7 +463,6 @@ class FixtureBuilder { } withPermissionControllerConnectedToTestDapp({ - restrictReturnedAccounts = true, account = '', useLocalhostHostname = false, } = {}) { @@ -477,7 +476,7 @@ class FixtureBuilder { id: 'ZaqPEWxyhNCJYACFw93jE', parentCapability: 'eth_accounts', invoker: DAPP_URL, - caveats: restrictReturnedAccounts && [ + caveats: [ { type: 'restrictReturnedAccounts', value: [ @@ -558,9 +557,7 @@ class FixtureBuilder { }); } - withPermissionControllerSnapAccountConnectedToTestDapp( - restrictReturnedAccounts = true, - ) { + withPermissionControllerSnapAccountConnectedToTestDapp() { return this.withPermissionController({ subjects: { [DAPP_URL]: { @@ -570,7 +567,7 @@ class FixtureBuilder { id: 'ZaqPEWxyhNCJYACFw93jE', parentCapability: 'eth_accounts', invoker: DAPP_URL, - caveats: restrictReturnedAccounts && [ + caveats: [ { type: 'restrictReturnedAccounts', value: ['0x09781764c08de8ca82e156bbf156a3ca217c7950'], @@ -584,9 +581,7 @@ class FixtureBuilder { }); } - withPermissionControllerConnectedToTwoTestDapps( - restrictReturnedAccounts = true, - ) { + withPermissionControllerConnectedToTwoTestDapps() { return this.withPermissionController({ subjects: { [DAPP_URL]: { @@ -596,7 +591,7 @@ class FixtureBuilder { id: 'ZaqPEWxyhNCJYACFw93jE', parentCapability: 'eth_accounts', invoker: DAPP_URL, - caveats: restrictReturnedAccounts && [ + caveats: [ { type: 'restrictReturnedAccounts', value: [ @@ -616,7 +611,7 @@ class FixtureBuilder { id: 'AqPEWxyhNCJYACFw93jE4', parentCapability: 'eth_accounts', invoker: DAPP_ONE_URL, - caveats: restrictReturnedAccounts && [ + caveats: [ { type: 'restrictReturnedAccounts', value: [