From d2fd2824de0fa6beee89a1f5476d2aae7d83a341 Mon Sep 17 00:00:00 2001 From: jiexi Date: Tue, 26 Nov 2024 11:49:15 -0800 Subject: [PATCH] Jl/caip25 permission migration/update mutators (#28709) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Replaces existing caveat mutators. Handles ensuring `wallet:eip155` is only upserted for permissions granted to snaps. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28709?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/develop/.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/develop/.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. --- .../permissions/background-api.test.js | 29 -------- .../permissions/caveat-mutators.js | 71 ------------------- .../permissions/caveat-mutators.test.js | 67 ----------------- app/scripts/controllers/permissions/index.js | 1 - .../handlers/request-accounts.test.ts | 8 ++- .../handlers/request-accounts.ts | 8 ++- .../wallet-requestPermissions.test.ts | 8 ++- .../handlers/wallet-requestPermissions.ts | 9 ++- app/scripts/metamask-controller.js | 9 --- package.json | 2 +- yarn.lock | 10 +-- 11 files changed, 33 insertions(+), 189 deletions(-) delete mode 100644 app/scripts/controllers/permissions/caveat-mutators.js delete mode 100644 app/scripts/controllers/permissions/caveat-mutators.test.js diff --git a/app/scripts/controllers/permissions/background-api.test.js b/app/scripts/controllers/permissions/background-api.test.js index ff0ca9887ba7..bed2129354cb 100644 --- a/app/scripts/controllers/permissions/background-api.test.js +++ b/app/scripts/controllers/permissions/background-api.test.js @@ -120,14 +120,6 @@ describe('permission background API methods', () => { 'eip155:1:0x4', ], }, - 'wallet:eip155': { - accounts: [ - 'wallet:eip155:0x1', - 'wallet:eip155:0x2', - 'wallet:eip155:0x3', - 'wallet:eip155:0x4', - ], - }, }, isMultichainOrigin: true, }, @@ -245,15 +237,6 @@ describe('permission background API methods', () => { 'eip155:1:0x5', ], }, - 'wallet:eip155': { - accounts: [ - 'wallet:eip155:0x1', - 'wallet:eip155:0x2', - 'wallet:eip155:0x3', - 'wallet:eip155:0x4', - 'wallet:eip155:0x5', - ], - }, }, isMultichainOrigin: true, }, @@ -424,9 +407,6 @@ describe('permission background API methods', () => { 'eip155:1': { accounts: ['eip155:1:0x1', 'eip155:1:0x3'], }, - 'wallet:eip155': { - accounts: ['wallet:eip155:0x1', 'wallet:eip155:0x3'], - }, }, isMultichainOrigin: true, }, @@ -510,9 +490,6 @@ describe('permission background API methods', () => { 'eip155:5': { accounts: ['eip155:5:0xdeadbeef'], }, - 'wallet:eip155': { - accounts: ['wallet:eip155:0xdeadbeef'], - }, }, isMultichainOrigin: false, }, @@ -619,9 +596,6 @@ describe('permission background API methods', () => { 'eip155:1337': { accounts: ['eip155:1337:0x1', 'eip155:1337:0x2'], }, - 'wallet:eip155': { - accounts: ['wallet:eip155:0x1', 'wallet:eip155:0x2'], - }, }, isMultichainOrigin: true, }, @@ -727,9 +701,6 @@ describe('permission background API methods', () => { 'eip155:5': { accounts: ['eip155:5:0x1', 'eip155:5:0x2'], }, - 'wallet:eip155': { - accounts: ['wallet:eip155:0x1', 'wallet:eip155:0x2'], - }, }, isMultichainOrigin: true, }, diff --git a/app/scripts/controllers/permissions/caveat-mutators.js b/app/scripts/controllers/permissions/caveat-mutators.js deleted file mode 100644 index 047341e34770..000000000000 --- a/app/scripts/controllers/permissions/caveat-mutators.js +++ /dev/null @@ -1,71 +0,0 @@ -import { CaveatMutatorOperation } from '@metamask/permission-controller'; -import { CaveatTypes } from '../../../../shared/constants/permissions'; -import { normalizeSafeAddress } from '../../lib/multichain/address'; - -/** - * Factories that construct caveat mutator functions that are passed to - * PermissionController.updatePermissionsByCaveat. - */ -export const CaveatMutatorFactories = { - [CaveatTypes.restrictReturnedAccounts]: { - removeAccount, - }, - [CaveatTypes.restrictNetworkSwitching]: { - removeChainId, - }, -}; - -/** - * Removes the target account from the value arrays of all - * `restrictReturnedAccounts` caveats. No-ops if the target account is not in - * the array, and revokes the parent permission if it's the only account in - * the array. - * - * @param {string} targetAccount - The address of the account to remove from - * all accounts permissions. - * @param {string[]} existingAccounts - The account address array from the - * account permissions. - */ -function removeAccount(targetAccount, existingAccounts) { - const checkSumTargetAccount = normalizeSafeAddress(targetAccount); - const newAccounts = existingAccounts.filter( - (address) => normalizeSafeAddress(address) !== checkSumTargetAccount, - ); - - if (newAccounts.length === existingAccounts.length) { - return { operation: CaveatMutatorOperation.Noop }; - } else if (newAccounts.length > 0) { - return { - operation: CaveatMutatorOperation.UpdateValue, - value: newAccounts, - }; - } - return { operation: CaveatMutatorOperation.RevokePermission }; -} - -/** - * Removes the target chain ID from the value arrays of all - * `restrictNetworkSwitching` caveats. No-ops if the target chain ID is not in - * the array, and revokes the parent permission if it's the only chain ID in - * the array. - * - * @param {string} targetChainId - The chain ID to remove from - * all network switching permissions. - * @param {string[]} existingChainIds - The chain ID array from the - * network switching permissions. - */ -function removeChainId(targetChainId, existingChainIds) { - const newChainIds = existingChainIds.filter( - (chainId) => chainId !== targetChainId, - ); - - if (newChainIds.length === existingChainIds.length) { - return { operation: CaveatMutatorOperation.Noop }; - } else if (newChainIds.length > 0) { - return { - operation: CaveatMutatorOperation.UpdateValue, - value: newChainIds, - }; - } - return { operation: CaveatMutatorOperation.RevokePermission }; -} diff --git a/app/scripts/controllers/permissions/caveat-mutators.test.js b/app/scripts/controllers/permissions/caveat-mutators.test.js deleted file mode 100644 index 8c16924514f4..000000000000 --- a/app/scripts/controllers/permissions/caveat-mutators.test.js +++ /dev/null @@ -1,67 +0,0 @@ -import { CaveatMutatorOperation } from '@metamask/permission-controller'; -import { CaveatTypes } from '../../../../shared/constants/permissions'; -import { CaveatMutatorFactories } from './caveat-mutators'; - -const address1 = '0xbf16f7f5db8ae6af2512399bace3101debbde7fc'; -const address2 = '0xb6d5abeca51bfc3d53d00afed06b17eeea32ecdf'; -const nonEvmAddress = 'bc1qdkwac3em6mvlur4fatn2g4q050f4kkqadrsmnp'; - -describe('caveat mutators', () => { - describe('restrictReturnedAccounts', () => { - const { removeAccount } = - CaveatMutatorFactories[CaveatTypes.restrictReturnedAccounts]; - - describe('removeAccount', () => { - it('returns the no-op operation if the target account is not permitted', () => { - expect(removeAccount(address2, [address1])).toStrictEqual({ - operation: CaveatMutatorOperation.Noop, - }); - }); - - it('returns the update operation and a new value if the target account is permitted', () => { - expect(removeAccount(address2, [address1, address2])).toStrictEqual({ - operation: CaveatMutatorOperation.UpdateValue, - value: [address1], - }); - }); - - it('returns the revoke permission operation the target account is the only permitted account', () => { - expect(removeAccount(address1, [address1])).toStrictEqual({ - operation: CaveatMutatorOperation.RevokePermission, - }); - }); - - it('returns the revoke permission operation even if the target account is a checksummed address', () => { - const address3 = '0x95222290dd7278aa3ddd389cc1e1d165cc4baee5'; - const checksummedAddress3 = - '0x95222290dd7278AA3DDd389cc1E1d165Cc4BaeE5'; - expect(removeAccount(checksummedAddress3, [address3])).toStrictEqual({ - operation: CaveatMutatorOperation.RevokePermission, - }); - }); - - describe('Multichain behaviour', () => { - it('returns the no-op operation if the target account is not permitted', () => { - expect(removeAccount(address2, [nonEvmAddress])).toStrictEqual({ - operation: CaveatMutatorOperation.Noop, - }); - }); - - it('can revoke permission for non-EVM addresses', () => { - expect(removeAccount(nonEvmAddress, [nonEvmAddress])).toStrictEqual({ - operation: CaveatMutatorOperation.RevokePermission, - }); - }); - - it('returns the update operation and a new value if the target non-EVM account is permitted', () => { - expect( - removeAccount(nonEvmAddress, [address1, nonEvmAddress]), - ).toStrictEqual({ - operation: CaveatMutatorOperation.UpdateValue, - value: [address1], - }); - }); - }); - }); - }); -}); diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index b0ec94b175f1..76a460487dfe 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -1,4 +1,3 @@ -export * from './caveat-mutators'; export * from './background-api'; export * from './enums'; export * from './specifications'; diff --git a/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.test.ts b/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.test.ts index 569d3f8ba5e8..e354a1701494 100644 --- a/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.test.ts +++ b/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.test.ts @@ -231,14 +231,18 @@ describe('requestEthereumAccountsHandler', () => { expect(MockMultichain.setPermittedEthChainIds).not.toHaveBeenCalled(); }); - it('sets the approved accounts on an empty CAIP-25 caveat with isMultichainOrigin: false if origin is snapId', async () => { + it('sets the approved accounts for the `wallet:eip155` scope with isMultichainOrigin: false if origin is snapId', async () => { const { handler } = createMockedHandler(); await handler({ ...baseRequest, origin: 'npm:snap' }); expect(MockMultichain.setEthAccounts).toHaveBeenCalledWith( { requiredScopes: {}, - optionalScopes: {}, + optionalScopes: { + 'wallet:eip155': { + accounts: [], + }, + }, isMultichainOrigin: false, }, ['0xdeadbeef'], diff --git a/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.ts b/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.ts index 1ce6c60e7f21..22b5ee5b3695 100644 --- a/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.ts +++ b/app/scripts/lib/rpc-method-middleware/handlers/request-accounts.ts @@ -143,7 +143,13 @@ async function requestEthereumAccountsHandler( isMultichainOrigin: false, }; - if (!isSnapId(origin)) { + if (isSnapId(origin)) { + caveatValue.optionalScopes = { + 'wallet:eip155': { + accounts: [], + }, + }; + } else { caveatValue = setPermittedEthChainIds( caveatValue, legacyApproval.approvedChainIds, diff --git a/app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.test.ts b/app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.test.ts index c4418d9378c2..2e8a4ad3eb24 100644 --- a/app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.test.ts +++ b/app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.test.ts @@ -579,14 +579,18 @@ describe('requestPermissionsHandler', () => { expect(MockMultichain.setPermittedEthChainIds).not.toHaveBeenCalled(); }); - it('sets the approved accounts on an empty CAIP-25 caveat with isMultichainOrigin: false if origin is snapId', async () => { + it('sets the approved accounts for the `wallet:eip155` scope with isMultichainOrigin: false if origin is snapId', async () => { const { handler } = createMockedHandler(); await handler({ ...getBaseRequest(), origin: 'npm:snapm' }); expect(MockMultichain.setEthAccounts).toHaveBeenCalledWith( { requiredScopes: {}, - optionalScopes: {}, + optionalScopes: { + 'wallet:eip155': { + accounts: [], + }, + }, isMultichainOrigin: false, }, ['0xdeadbeef'], diff --git a/app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.ts b/app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.ts index a059900a0622..143d2e970dfd 100644 --- a/app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.ts +++ b/app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.ts @@ -176,7 +176,14 @@ async function requestPermissionsImplementation( optionalScopes: {}, isMultichainOrigin: false, }; - if (!isSnapId(origin)) { + + if (isSnapId(origin)) { + newCaveatValue.optionalScopes = { + 'wallet:eip155': { + accounts: [], + }, + }; + } else { newCaveatValue = setPermittedEthChainIds( newCaveatValue, legacyApproval.approvedChainIds, diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index af1b43085084..896b652fe8aa 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -192,7 +192,6 @@ import { } from '../../shared/constants/hardware-wallets'; import { KeyringType } from '../../shared/constants/keyring'; import { - CaveatTypes, RestrictedMethods, EndowmentPermissions, ExcludedSnapPermissions, @@ -325,7 +324,6 @@ import EncryptionPublicKeyController from './controllers/encryption-public-key'; import AppMetadataController from './controllers/app-metadata'; import { - CaveatMutatorFactories, getCaveatSpecifications, diffMap, getPermissionBackgroundApiMethods, @@ -5148,13 +5146,6 @@ export default class MetamaskController extends EventEmitter { * to third parties. */ removeAllAccountPermissions(targetAccount) { - this.permissionController.updatePermissionsByCaveat( - CaveatTypes.restrictReturnedAccounts, - (existingAccounts) => - CaveatMutatorFactories[ - CaveatTypes.restrictReturnedAccounts - ].removeAccount(targetAccount, existingAccounts), - ); this.permissionController.updatePermissionsByCaveat( Caip25CaveatType, (existingScopes) => diff --git a/package.json b/package.json index 7afed937b5fe..997455a093e0 100644 --- a/package.json +++ b/package.json @@ -326,7 +326,7 @@ "@metamask/message-manager": "^10.1.0", "@metamask/message-signing-snap": "^0.4.0", "@metamask/metamask-eth-abis": "^3.1.1", - "@metamask/multichain": "^1.0.0", + "@metamask/multichain": "^1.1.0", "@metamask/name-controller": "^8.0.0", "@metamask/network-controller": "patch:@metamask/network-controller@npm%3A21.0.0#~/.yarn/patches/@metamask-network-controller-npm-21.0.0-559aa8e395.patch", "@metamask/notification-controller": "^6.0.0", diff --git a/yarn.lock b/yarn.lock index 34965d7d49d4..1080fbc5d744 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5951,9 +5951,9 @@ __metadata: languageName: node linkType: hard -"@metamask/multichain@npm:^1.0.0": - version: 1.0.0 - resolution: "@metamask/multichain@npm:1.0.0" +"@metamask/multichain@npm:^1.1.0": + version: 1.1.0 + resolution: "@metamask/multichain@npm:1.1.0" dependencies: "@metamask/api-specs": "npm:^0.10.12" "@metamask/controller-utils": "npm:^11.4.3" @@ -5964,7 +5964,7 @@ __metadata: peerDependencies: "@metamask/network-controller": ^22.0.0 "@metamask/permission-controller": ^11.0.0 - checksum: 10/ec4ae86bb91a002863bf2b50900488bcb3851981d9b5e91f4d14bba05acf89dd7fce23738b11b1c5079fd3e0edc3702849e28e5ecd59b99a1f9ddedf19d3fd14 + checksum: 10/dab9a223c6cf1b11705f53cab3c1d6f2ecfb5f0c2b7b44d32847ea42b5cd2c3a9936b10239d3dbf0215b2656b3584a08a2ee979004cedd2edfb27813ec25665d languageName: node linkType: hard @@ -26883,7 +26883,7 @@ __metadata: "@metamask/message-manager": "npm:^10.1.0" "@metamask/message-signing-snap": "npm:^0.4.0" "@metamask/metamask-eth-abis": "npm:^3.1.1" - "@metamask/multichain": "npm:^1.0.0" + "@metamask/multichain": "npm:^1.1.0" "@metamask/name-controller": "npm:^8.0.0" "@metamask/network-controller": "patch:@metamask/network-controller@npm%3A21.0.0#~/.yarn/patches/@metamask-network-controller-npm-21.0.0-559aa8e395.patch" "@metamask/notification-controller": "npm:^6.0.0"