From 61a79e0942561a5d9f2d2e698eedb00a4faad5ea Mon Sep 17 00:00:00 2001 From: cryptodev-2s <109512101+cryptodev-2s@users.noreply.github.com> Date: Fri, 1 Mar 2024 14:48:38 +0100 Subject: [PATCH] feat: remove PermissionLogController in favor of core implementation (#23182) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Following the successful migration of PermissionLogController to the core monorepo (https://github.com/MetaMask/core/issues/1826), this commit removes the redundant PermissionLogController logic from the extension. All future developments and maintenance will be concentrated on the core implementation to streamline efforts and enhance functionality coherence across the platform. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/23182?quickstart=1) ## **Related issues** - Fixes: 23181 ## **Changes** The transition of this controller from the extension repo to the core monorepo unfolded in three phases: 1. The controller was integrated into Core, with more information available at https://github.com/MetaMask/core/pull/1871 2. The logic of the controller was streamlined, with additional details at https://github.com/MetaMask/core/pull/3662 3. The tests for the controller were overhauled, with further information at https://github.com/MetaMask/core/pull/3937 ## **Manual testing steps** These instructions outline the process for conducting manual testing locally. 1. Launch the extension from the latest development branch. 2. Navigate to the [test-dapp](https://metamask.github.io/test-dapp/). 3. Initiate the REQUEST_PERMISSIONS action from the Permissions Actions menu. 4. Open the background.html inspect window. 5. Execute the script `chrome.storage.local.get(null, ({data}) => console.log(data.PermissionLogController))` in the console. 6. Record the output from the previous step. 7. Switch to the branch named `feature/23181-remove-Permissionlogcontroller`. 8. Repeat steps 2 through 6 for this branch. 9. Compare the outputs from step 6 for both the development and feature branches. Look for matching entries in `permissionHistory` and `permissionActivityLog` from the initial run in the second run's output. Note that the log history is limited to 100 entries. ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [ ] I've included screenshots/recordings if applicable - [ ] 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. - [ ] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **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: MetaMask Bot --- app/scripts/controllers/permissions/enums.js | 17 - app/scripts/controllers/permissions/index.js | 1 - .../controllers/permissions/permission-log.js | 374 ---------- .../permissions/permission-log.test.js | 668 ------------------ app/scripts/metamask-controller.js | 11 +- lavamoat/browserify/beta/policy.json | 6 + lavamoat/browserify/desktop/policy.json | 6 + lavamoat/browserify/flask/policy.json | 6 + lavamoat/browserify/main/policy.json | 6 + lavamoat/browserify/mmi/policy.json | 6 + package.json | 1 + test/mocks/permissions.js | 369 ---------- yarn.lock | 12 + 13 files changed, 50 insertions(+), 1433 deletions(-) delete mode 100644 app/scripts/controllers/permissions/permission-log.js delete mode 100644 app/scripts/controllers/permissions/permission-log.test.js delete mode 100644 test/mocks/permissions.js diff --git a/app/scripts/controllers/permissions/enums.js b/app/scripts/controllers/permissions/enums.js index 577540d49bfd..4ede66f22cb9 100644 --- a/app/scripts/controllers/permissions/enums.js +++ b/app/scripts/controllers/permissions/enums.js @@ -1,22 +1,5 @@ -export const WALLET_PREFIX = 'wallet_'; - export const NOTIFICATION_NAMES = { accountsChanged: 'metamask_accountsChanged', unlockStateChanged: 'metamask_unlockStateChanged', chainChanged: 'metamask_chainChanged', }; - -export const LOG_IGNORE_METHODS = [ - 'wallet_registerOnboarding', - 'wallet_watchAsset', -]; - -export const LOG_METHOD_TYPES = { - restricted: 'restricted', - internal: 'internal', -}; - -/** - * The permission activity log size limit. - */ -export const LOG_LIMIT = 100; diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index 419d039934de..b0ec94b175f1 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -1,6 +1,5 @@ export * from './caveat-mutators'; export * from './background-api'; export * from './enums'; -export * from './permission-log'; export * from './specifications'; export * from './selectors'; diff --git a/app/scripts/controllers/permissions/permission-log.js b/app/scripts/controllers/permissions/permission-log.js deleted file mode 100644 index 60b34b6c3d73..000000000000 --- a/app/scripts/controllers/permissions/permission-log.js +++ /dev/null @@ -1,374 +0,0 @@ -import { ObservableStore } from '@metamask/obs-store'; -import { CaveatTypes } from '../../../../shared/constants/permissions'; -import { - LOG_IGNORE_METHODS, - LOG_LIMIT, - LOG_METHOD_TYPES, - WALLET_PREFIX, -} from './enums'; - -/** - * Controller with middleware for logging requests and responses to restricted - * and permissions-related methods. - */ -export class PermissionLogController { - /** - * @param {{ restrictedMethods: Set, initState: Record }} options - Options bag. - */ - constructor({ restrictedMethods, initState }) { - this.restrictedMethods = restrictedMethods; - this.store = new ObservableStore({ - permissionHistory: {}, - permissionActivityLog: [], - ...initState, - }); - } - - /** - * Get the restricted method activity log. - * - * @returns {Array} The activity log. - */ - getActivityLog() { - return this.store.getState().permissionActivityLog; - } - - /** - * Update the restricted method activity log. - * - * @param {Array} logs - The new activity log array. - */ - updateActivityLog(logs) { - this.store.updateState({ permissionActivityLog: logs }); - } - - /** - * Get the permission history log. - * - * @returns {object} The permissions history log. - */ - getHistory() { - return this.store.getState().permissionHistory; - } - - /** - * Update the permission history log. - * - * @param {object} history - The new permissions history log object. - */ - updateHistory(history) { - this.store.updateState({ permissionHistory: history }); - } - - /** - * Updates the exposed account history for the given origin. - * Sets the 'last seen' time to Date.now() for the given accounts. - * Does **not** update the 'lastApproved' time for the permission itself. - * Returns if the accounts array is empty. - * - * @param {string} origin - The origin that the accounts are exposed to. - * @param {Array} accounts - The accounts. - */ - updateAccountsHistory(origin, accounts) { - if (accounts.length === 0) { - return; - } - - const accountToTimeMap = getAccountToTimeMap(accounts, Date.now()); - - this.commitNewHistory(origin, { - eth_accounts: { - accounts: accountToTimeMap, - }, - }); - } - - /** - * Create a permissions log middleware. Records permissions activity and history: - * - * Activity: requests and responses for restricted and most wallet_ methods. - * - * History: for each origin, the last time a permission was granted, including - * which accounts were exposed, if any. - * - * @returns {JsonRpcEngineMiddleware} The permissions log middleware. - */ - createMiddleware() { - return (req, res, next, _end) => { - let activityEntry, requestedMethods; - const { origin, method } = req; - const isInternal = method.startsWith(WALLET_PREFIX); - - // we only log certain methods - if ( - !LOG_IGNORE_METHODS.includes(method) && - (isInternal || this.restrictedMethods.has(method)) - ) { - activityEntry = this.logRequest(req, isInternal); - - if (method === `${WALLET_PREFIX}requestPermissions`) { - // get the corresponding methods from the requested permissions so - // that we can record permissions history - requestedMethods = this.getRequestedMethods(req); - } - } else if (method === 'eth_requestAccounts') { - // eth_requestAccounts is a special case; we need to extract the accounts - // from it - activityEntry = this.logRequest(req, isInternal); - requestedMethods = ['eth_accounts']; - } else { - // no-op - next(); - return; - } - - // call next with a return handler for capturing the response - next((cb) => { - const time = Date.now(); - this.logResponse(activityEntry, res, time); - - if (requestedMethods && !res.error && res.result) { - // any permissions or accounts changes will be recorded on the response, - // so we only log permissions history here - this.logPermissionsHistory( - requestedMethods, - origin, - res.result, - time, - method === 'eth_requestAccounts', - ); - } - cb(); - }); - }; - } - - /** - * Creates and commits an activity log entry, without response data. - * - * @param {object} request - The request object. - * @param {boolean} isInternal - Whether the request is internal. - */ - logRequest(request, isInternal) { - const activityEntry = { - id: request.id, - method: request.method, - methodType: isInternal - ? LOG_METHOD_TYPES.internal - : LOG_METHOD_TYPES.restricted, - origin: request.origin, - requestTime: Date.now(), - responseTime: null, - success: null, - }; - this.commitNewActivity(activityEntry); - return activityEntry; - } - - /** - * Adds response data to an existing activity log entry. - * Entry assumed already committed (i.e., in the log). - * - * @param {object} entry - The entry to add a response to. - * @param {object} response - The response object. - * @param {number} time - Output from Date.now() - */ - logResponse(entry, response, time) { - if (!entry || !response) { - return; - } - - // The JSON-RPC 2.0 specification defines "success" by the presence of - // either the "result" or "error" property. The specification forbids - // both properties from being present simultaneously, and our JSON-RPC - // stack is spec-compliant at the time of writing. - entry.success = Object.hasOwnProperty.call(response, 'result'); - entry.responseTime = time; - } - - /** - * Commit a new entry to the activity log. - * Removes the oldest entry from the log if it exceeds the log limit. - * - * @param {object} entry - The activity log entry. - */ - commitNewActivity(entry) { - const logs = this.getActivityLog(); - - // add new entry to end of log - logs.push(entry); - - // remove oldest log if exceeding size limit - if (logs.length > LOG_LIMIT) { - logs.shift(); - } - - this.updateActivityLog(logs); - } - - /** - * Create new permissions history log entries, if any, and commit them. - * - * @param {Array} requestedMethods - The method names corresponding to the requested permissions. - * @param {string} origin - The origin of the permissions request. - * @param {Array { - if (perm.parentCapability === 'eth_accounts') { - accounts = this.getAccountsFromPermission(perm); - } - - return perm.parentCapability; - }) - .reduce((acc, method) => { - // all approved permissions will be included in the response, - // not just the newly requested ones - if (requestedMethods.includes(method)) { - if (method === 'eth_accounts') { - const accountToTimeMap = getAccountToTimeMap(accounts, time); - - acc[method] = { - lastApproved: time, - accounts: accountToTimeMap, - }; - } else { - acc[method] = { lastApproved: time }; - } - } - - return acc; - }, {}); - } - - if (Object.keys(newEntries).length > 0) { - this.commitNewHistory(origin, newEntries); - } - } - - /** - * Commit new entries to the permissions history log. - * Merges the history for the given origin, overwriting existing entries - * with the same key (permission name). - * - * @param {string} origin - The requesting origin. - * @param {object} newEntries - The new entries to commit. - */ - commitNewHistory(origin, newEntries) { - // a simple merge updates most permissions - const history = this.getHistory(); - const newOriginHistory = { - ...history[origin], - ...newEntries, - }; - - // eth_accounts requires special handling, because of information - // we store about the accounts - const existingEthAccountsEntry = - history[origin] && history[origin].eth_accounts; - const newEthAccountsEntry = newEntries.eth_accounts; - - if (existingEthAccountsEntry && newEthAccountsEntry) { - // we may intend to update just the accounts, not the permission - // itself - const lastApproved = - newEthAccountsEntry.lastApproved || - existingEthAccountsEntry.lastApproved; - - // merge old and new eth_accounts history entries - newOriginHistory.eth_accounts = { - lastApproved, - accounts: { - ...existingEthAccountsEntry.accounts, - ...newEthAccountsEntry.accounts, - }, - }; - } - - history[origin] = newOriginHistory; - - this.updateHistory(history); - } - - /** - * Get all requested methods from a permissions request. - * - * @param {object} request - The request object. - * @returns {Array} The names of the requested permissions. - */ - getRequestedMethods(request) { - if ( - !request.params || - !request.params[0] || - typeof request.params[0] !== 'object' || - Array.isArray(request.params[0]) - ) { - return null; - } - return Object.keys(request.params[0]); - } - - /** - * Get the permitted accounts from an eth_accounts permissions object. - * Returns an empty array if the permission is not eth_accounts. - * - * @param {object} perm - The permissions object. - * @returns {Array} The permitted accounts. - */ - getAccountsFromPermission(perm) { - if (perm.parentCapability !== 'eth_accounts' || !perm.caveats) { - return []; - } - - const accounts = new Set(); - for (const caveat of perm.caveats) { - if ( - caveat.type === CaveatTypes.restrictReturnedAccounts && - Array.isArray(caveat.value) - ) { - for (const value of caveat.value) { - accounts.add(value); - } - } - } - return [...accounts]; - } -} - -// helper functions - -/** - * Get a map from account addresses to the given time. - * - * @param {Array} accounts - An array of addresses. - * @param {number} time - A time, e.g. Date.now(). - * @returns {object} A string:number map of addresses to time. - */ -function getAccountToTimeMap(accounts, time) { - return accounts.reduce((acc, account) => ({ ...acc, [account]: time }), {}); -} diff --git a/app/scripts/controllers/permissions/permission-log.test.js b/app/scripts/controllers/permissions/permission-log.test.js deleted file mode 100644 index a8d58f692542..000000000000 --- a/app/scripts/controllers/permissions/permission-log.test.js +++ /dev/null @@ -1,668 +0,0 @@ -import nanoid from 'nanoid'; -import { useFakeTimers } from 'sinon'; -import { constants, getters, noop } from '../../../../test/mocks/permissions'; -import { PermissionLogController } from './permission-log'; -import { LOG_LIMIT, LOG_METHOD_TYPES } from './enums'; - -const { PERMS, RPC_REQUESTS } = getters; -const { - ACCOUNTS, - EXPECTED_HISTORIES, - SUBJECTS, - PERM_NAMES, - REQUEST_IDS, - RESTRICTED_METHODS, -} = constants; - -let clock; - -const initPermLog = (initState = {}) => { - return new PermissionLogController({ - restrictedMethods: RESTRICTED_METHODS, - initState, - }); -}; - -const mockNext = (handler) => { - if (handler) { - handler(noop); - } -}; - -const initMiddleware = (permLog) => { - const middleware = permLog.createMiddleware(); - return (req, res, next = mockNext) => { - middleware(req, res, next); - }; -}; - -const initClock = () => { - // useFakeTimers, is in fact, not a react-hook - // eslint-disable-next-line - clock = useFakeTimers(1); -}; - -const tearDownClock = () => { - clock.restore(); -}; - -const getSavedMockNext = (arr) => (handler) => { - arr.push(handler); -}; - -describe('PermissionLogController', () => { - describe('restricted method activity log', () => { - let permLog, logMiddleware; - - beforeEach(() => { - permLog = initPermLog(); - logMiddleware = initMiddleware(permLog); - }); - - it('records activity for restricted methods', () => { - let log, req, res; - - // test_method, success - - req = RPC_REQUESTS.test_method(SUBJECTS.a.origin); - req.id = REQUEST_IDS.a; - res = { result: 'bar' }; - - logMiddleware({ ...req }, res); - - log = permLog.getActivityLog(); - const entry1 = log[0]; - - expect(log).toHaveLength(1); - validateActivityEntry( - entry1, - { ...req }, - { ...res }, - LOG_METHOD_TYPES.restricted, - true, - ); - - // eth_accounts, failure - - req = RPC_REQUESTS.eth_accounts(SUBJECTS.b.origin); - req.id = REQUEST_IDS.b; - res = { error: new Error('Unauthorized.') }; - - logMiddleware({ ...req }, res); - - log = permLog.getActivityLog(); - const entry2 = log[1]; - - expect(log).toHaveLength(2); - validateActivityEntry( - entry2, - { ...req }, - { ...res }, - LOG_METHOD_TYPES.restricted, - false, - ); - - // eth_requestAccounts, success - - req = RPC_REQUESTS.eth_requestAccounts(SUBJECTS.c.origin); - req.id = REQUEST_IDS.c; - res = { result: ACCOUNTS.c.permitted }; - - logMiddleware({ ...req }, res); - - log = permLog.getActivityLog(); - const entry3 = log[2]; - - expect(log).toHaveLength(3); - validateActivityEntry( - entry3, - { ...req }, - { ...res }, - LOG_METHOD_TYPES.restricted, - true, - ); - - // test_method, no response - - req = RPC_REQUESTS.test_method(SUBJECTS.a.origin); - req.id = REQUEST_IDS.a; - res = null; - - logMiddleware({ ...req }, res); - - log = permLog.getActivityLog(); - const entry4 = log[3]; - - expect(log).toHaveLength(4); - validateActivityEntry( - entry4, - { ...req }, - null, - LOG_METHOD_TYPES.restricted, - false, - ); - - // Validate final state - expect(entry1).toStrictEqual(log[0]); - expect(entry2).toStrictEqual(log[1]); - expect(entry3).toStrictEqual(log[2]); - expect(entry4).toStrictEqual(log[3]); - - // Regression test: ensure "response" and "request" properties - // are not present - log.forEach((entry) => - expect('request' in entry && 'response' in entry).toBe(false), - ); - }); - - it('handles responses added out of order', () => { - let log; - - const handlerArray = []; - - const id1 = nanoid(); - const id2 = nanoid(); - const id3 = nanoid(); - - const req = RPC_REQUESTS.test_method(SUBJECTS.a.origin); - - // get make requests - req.id = id1; - const res1 = { result: id1 }; - logMiddleware({ ...req }, { ...res1 }, getSavedMockNext(handlerArray)); - - req.id = id2; - const res2 = { result: id2 }; - logMiddleware({ ...req }, { ...res2 }, getSavedMockNext(handlerArray)); - - req.id = id3; - const res3 = { result: id3 }; - logMiddleware({ ...req }, { ...res3 }, getSavedMockNext(handlerArray)); - - // verify log state - log = permLog.getActivityLog(); - expect(log).toHaveLength(3); - const entry1 = log[0]; - const entry2 = log[1]; - const entry3 = log[2]; - - // all entries should be in correct order - expect(entry1).toMatchObject({ id: id1, responseTime: null }); - expect(entry2).toMatchObject({ id: id2, responseTime: null }); - expect(entry3).toMatchObject({ id: id3, responseTime: null }); - - // call response handlers - for (const i of [1, 2, 0]) { - handlerArray[i](noop); - } - - // verify log state again - log = permLog.getActivityLog(); - expect(log).toHaveLength(3); - - // verify all entries - log = permLog.getActivityLog(); - - validateActivityEntry( - log[0], - { ...req, id: id1 }, - { ...res1 }, - LOG_METHOD_TYPES.restricted, - true, - ); - - validateActivityEntry( - log[1], - { ...req, id: id2 }, - { ...res2 }, - LOG_METHOD_TYPES.restricted, - true, - ); - - validateActivityEntry( - log[2], - { ...req, id: id3 }, - { ...res3 }, - LOG_METHOD_TYPES.restricted, - true, - ); - }); - - it('handles a lack of response', () => { - let req = RPC_REQUESTS.test_method(SUBJECTS.a.origin); - req.id = REQUEST_IDS.a; - let res = { result: 'bar' }; - - // noop for next handler prevents recording of response - logMiddleware({ ...req }, res, noop); - - let log = permLog.getActivityLog(); - const entry1 = log[0]; - - expect(log).toHaveLength(1); - validateActivityEntry( - entry1, - { ...req }, - null, - LOG_METHOD_TYPES.restricted, - true, - ); - - // next request should be handled as normal - req = RPC_REQUESTS.eth_accounts(SUBJECTS.b.origin); - req.id = REQUEST_IDS.b; - res = { result: ACCOUNTS.b.permitted }; - - logMiddleware({ ...req }, res); - - log = permLog.getActivityLog(); - const entry2 = log[1]; - expect(log).toHaveLength(2); - validateActivityEntry( - entry2, - { ...req }, - { ...res }, - LOG_METHOD_TYPES.restricted, - true, - ); - - // validate final state - expect(entry1).toStrictEqual(log[0]); - expect(entry2).toStrictEqual(log[1]); - }); - - it('ignores expected methods', () => { - let log = permLog.getActivityLog(); - expect(log).toHaveLength(0); - - const res = { result: 'bar' }; - const req1 = RPC_REQUESTS.metamask_sendDomainMetadata( - SUBJECTS.c.origin, - 'foobar', - ); - const req2 = RPC_REQUESTS.custom(SUBJECTS.b.origin, 'eth_getBlockNumber'); - const req3 = RPC_REQUESTS.custom(SUBJECTS.b.origin, 'net_version'); - - logMiddleware(req1, res); - logMiddleware(req2, res); - logMiddleware(req3, res); - - log = permLog.getActivityLog(); - expect(log).toHaveLength(0); - }); - - it('enforces log limit', () => { - const req = RPC_REQUESTS.test_method(SUBJECTS.a.origin); - const res = { result: 'bar' }; - - // max out log - let lastId; - for (let i = 0; i < LOG_LIMIT; i++) { - lastId = nanoid(); - logMiddleware({ ...req, id: lastId }, { ...res }); - } - - // check last entry valid - let log = permLog.getActivityLog(); - expect(log).toHaveLength(LOG_LIMIT); - - validateActivityEntry( - log[LOG_LIMIT - 1], - { ...req, id: lastId }, - res, - LOG_METHOD_TYPES.restricted, - true, - ); - - // store the id of the current second entry - const nextFirstId = log[1].id; - - // add one more entry to log, putting it over the limit - lastId = nanoid(); - logMiddleware({ ...req, id: lastId }, { ...res }); - - // check log length - log = permLog.getActivityLog(); - expect(log).toHaveLength(LOG_LIMIT); - - // check first and last entries - validateActivityEntry( - log[0], - { ...req, id: nextFirstId }, - res, - LOG_METHOD_TYPES.restricted, - true, - ); - - validateActivityEntry( - log[LOG_LIMIT - 1], - { ...req, id: lastId }, - res, - LOG_METHOD_TYPES.restricted, - true, - ); - }); - }); - - describe('permission history log', () => { - let permLog, logMiddleware; - - beforeEach(() => { - permLog = initPermLog(); - logMiddleware = initMiddleware(permLog); - initClock(); - }); - - afterEach(() => { - tearDownClock(); - }); - - it('only updates history on responses', () => { - const req = RPC_REQUESTS.requestPermission( - SUBJECTS.a.origin, - PERM_NAMES.test_method, - ); - const res = { result: [PERMS.granted.test_method()] }; - - // noop => no response - logMiddleware({ ...req }, { ...res }, noop); - - expect(permLog.getHistory()).toStrictEqual({}); - - // response => records granted permissions - logMiddleware({ ...req }, { ...res }); - - const permHistory = permLog.getHistory(); - expect(Object.keys(permHistory)).toHaveLength(1); - expect(permHistory[SUBJECTS.a.origin]).toBeDefined(); - }); - - it('ignores malformed permissions requests', () => { - const req = RPC_REQUESTS.requestPermission( - SUBJECTS.a.origin, - PERM_NAMES.test_method, - ); - delete req.params; - const res = { result: [PERMS.granted.test_method()] }; - - // no params => no response - logMiddleware({ ...req }, { ...res }); - - expect(permLog.getHistory()).toStrictEqual({}); - }); - - it('records and updates account history as expected', async () => { - const req = RPC_REQUESTS.requestPermission( - SUBJECTS.a.origin, - PERM_NAMES.eth_accounts, - ); - const res = { - result: [PERMS.granted.eth_accounts(ACCOUNTS.a.permitted)], - }; - - logMiddleware({ ...req }, { ...res }); - - expect(permLog.getHistory()).toStrictEqual(EXPECTED_HISTORIES.case1[0]); - - // mock permission requested again, with another approved account - - clock.tick(1); - - res.result = [PERMS.granted.eth_accounts([ACCOUNTS.a.permitted[0]])]; - - logMiddleware({ ...req }, { ...res }); - - expect(permLog.getHistory()).toStrictEqual(EXPECTED_HISTORIES.case1[1]); - }); - - it('handles eth_accounts response without caveats', async () => { - const req = RPC_REQUESTS.requestPermission( - SUBJECTS.a.origin, - PERM_NAMES.eth_accounts, - ); - const res = { - result: [PERMS.granted.eth_accounts(ACCOUNTS.a.permitted)], - }; - delete res.result[0].caveats; - - logMiddleware({ ...req }, { ...res }); - - expect(permLog.getHistory()).toStrictEqual(EXPECTED_HISTORIES.case2[0]); - }); - - it('handles extra caveats for eth_accounts', async () => { - const req = RPC_REQUESTS.requestPermission( - SUBJECTS.a.origin, - PERM_NAMES.eth_accounts, - ); - const res = { - result: [PERMS.granted.eth_accounts(ACCOUNTS.a.permitted)], - }; - res.result[0].caveats.push({ foo: 'bar' }); - - logMiddleware({ ...req }, { ...res }); - - expect(permLog.getHistory()).toStrictEqual(EXPECTED_HISTORIES.case1[0]); - }); - - // wallet_requestPermissions returns all permissions approved for the - // requesting origin, including old ones - it('handles unrequested permissions on the response', async () => { - const req = RPC_REQUESTS.requestPermission( - SUBJECTS.a.origin, - PERM_NAMES.eth_accounts, - ); - const res = { - result: [ - PERMS.granted.eth_accounts(ACCOUNTS.a.permitted), - PERMS.granted.test_method(), - ], - }; - - logMiddleware({ ...req }, { ...res }); - - expect(permLog.getHistory()).toStrictEqual(EXPECTED_HISTORIES.case1[0]); - }); - - it('does not update history if no new permissions are approved', async () => { - let req = RPC_REQUESTS.requestPermission( - SUBJECTS.a.origin, - PERM_NAMES.test_method, - ); - let res = { - result: [PERMS.granted.test_method()], - }; - - logMiddleware({ ...req }, { ...res }); - - expect(permLog.getHistory()).toStrictEqual(EXPECTED_HISTORIES.case4[0]); - - // new permission requested, but not approved - - clock.tick(1); - - req = RPC_REQUESTS.requestPermission( - SUBJECTS.a.origin, - PERM_NAMES.eth_accounts, - ); - res = { - result: [PERMS.granted.test_method()], - }; - - logMiddleware({ ...req }, { ...res }); - - // history should be unmodified - expect(permLog.getHistory()).toStrictEqual(EXPECTED_HISTORIES.case4[0]); - }); - - it('records and updates history for multiple origins, regardless of response order', async () => { - // make first round of requests - - const round1 = []; - const handlers1 = []; - - // first origin - round1.push({ - req: RPC_REQUESTS.requestPermission( - SUBJECTS.a.origin, - PERM_NAMES.test_method, - ), - res: { - result: [PERMS.granted.test_method()], - }, - }); - - // second origin - round1.push({ - req: RPC_REQUESTS.requestPermission( - SUBJECTS.b.origin, - PERM_NAMES.eth_accounts, - ), - res: { - result: [PERMS.granted.eth_accounts(ACCOUNTS.b.permitted)], - }, - }); - - // third origin - round1.push({ - req: RPC_REQUESTS.requestPermissions(SUBJECTS.c.origin, { - [PERM_NAMES.test_method]: {}, - [PERM_NAMES.eth_accounts]: {}, - }), - res: { - result: [ - PERMS.granted.test_method(), - PERMS.granted.eth_accounts(ACCOUNTS.c.permitted), - ], - }, - }); - - // make requests and process responses out of order - round1.forEach((x) => { - logMiddleware({ ...x.req }, { ...x.res }, getSavedMockNext(handlers1)); - }); - - for (const i of [1, 2, 0]) { - handlers1[i](noop); - } - - expect(permLog.getHistory()).toStrictEqual(EXPECTED_HISTORIES.case3[0]); - - // make next round of requests - - clock.tick(1); - - const round2 = []; - // we're just gonna process these in order - - // first origin - round2.push({ - req: RPC_REQUESTS.requestPermission( - SUBJECTS.a.origin, - PERM_NAMES.test_method, - ), - res: { - result: [PERMS.granted.test_method()], - }, - }); - - // nothing for second origin - - // third origin - round2.push({ - req: RPC_REQUESTS.requestPermissions(SUBJECTS.c.origin, { - [PERM_NAMES.eth_accounts]: {}, - }), - res: { - result: [PERMS.granted.eth_accounts(ACCOUNTS.b.permitted)], - }, - }); - - // make requests - round2.forEach((x) => { - logMiddleware({ ...x.req }, { ...x.res }); - }); - - expect(permLog.getHistory()).toStrictEqual(EXPECTED_HISTORIES.case3[1]); - }); - }); - - describe('updateAccountsHistory', () => { - beforeEach(() => { - initClock(); - }); - - afterEach(() => { - tearDownClock(); - }); - - it('does nothing if the list of accounts is empty', () => { - const permLog = initPermLog(); - permLog.updateAccountsHistory('foo.com', []); - - expect(permLog.getHistory()).toStrictEqual({}); - }); - - it('updates the account history', () => { - const permLog = initPermLog({ - permissionHistory: { - 'foo.com': { - [PERM_NAMES.eth_accounts]: { - accounts: { - '0x1': 1, - }, - lastApproved: 1, - }, - }, - }, - }); - - clock.tick(1); - permLog.updateAccountsHistory('foo.com', ['0x1', '0x2']); - - expect(permLog.getHistory()).toStrictEqual({ - 'foo.com': { - [PERM_NAMES.eth_accounts]: { - accounts: { - '0x1': 2, - '0x2': 2, - }, - lastApproved: 1, - }, - }, - }); - }); - }); -}); - -/** - * Validates an activity log entry with respect to a request, response, and - * relevant metadata. - * - * @param {object} entry - The activity log entry to validate. - * @param {object} req - The request that generated the entry. - * @param {object} [res] - The response for the request, if any. - * @param {'restricted'|'internal'} methodType - The method log controller method type of the request. - * @param {boolean} success - Whether the request succeeded or not. - */ -function validateActivityEntry(entry, req, res, methodType, success) { - expect(entry).toBeDefined(); - - expect(entry.id).toStrictEqual(req.id); - expect(entry.method).toStrictEqual(req.method); - expect(entry.origin).toStrictEqual(req.origin); - expect(entry.methodType).toStrictEqual(methodType); - - expect(Number.isInteger(entry.requestTime)).toBe(true); - if (res) { - expect(Number.isInteger(entry.responseTime)).toBe(true); - expect(entry.requestTime <= entry.responseTime).toBe(true); - expect(entry.success).toStrictEqual(success); - } else { - expect(entry.requestTime > 0).toBe(true); - expect(entry).toMatchObject({ - responseTime: null, - success: null, - }); - } -} diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 83134d01ba48..237e2f3ced34 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -72,6 +72,7 @@ import { createSelectedNetworkMiddleware, } from '@metamask/selected-network-controller'; import { LoggingController, LogType } from '@metamask/logging-controller'; +import { PermissionLogController } from '@metamask/permission-log-controller'; ///: BEGIN:ONLY_INCLUDE_IF(snaps) import { RateLimitController } from '@metamask/rate-limit-controller'; @@ -282,7 +283,6 @@ import { getPermissionSpecifications, getPermittedAccountsByOrigin, NOTIFICATION_NAMES, - PermissionLogController, unrestrictedMethods, } from './controllers/permissions'; import createRPCMethodTrackingMiddleware from './lib/createRPCMethodTrackingMiddleware'; @@ -1117,8 +1117,11 @@ export default class MetamaskController extends EventEmitter { }); this.permissionLogController = new PermissionLogController({ + messenger: this.controllerMessenger.getRestricted({ + name: 'PermissionLogController', + }), restrictedMethods: new Set(Object.keys(RestrictedMethods)), - initState: initState.PermissionLogController, + state: initState.PermissionLogController, }); this.subjectMetadataController = new SubjectMetadataController({ @@ -1958,7 +1961,7 @@ export default class MetamaskController extends EventEmitter { AlertController: this.alertController.store, OnboardingController: this.onboardingController.store, PermissionController: this.permissionController, - PermissionLogController: this.permissionLogController.store, + PermissionLogController: this.permissionLogController, SubjectMetadataController: this.subjectMetadataController, AnnouncementController: this.announcementController, NetworkOrderController: this.networkOrderController, @@ -2010,7 +2013,7 @@ export default class MetamaskController extends EventEmitter { AlertController: this.alertController.store, OnboardingController: this.onboardingController.store, PermissionController: this.permissionController, - PermissionLogController: this.permissionLogController.store, + PermissionLogController: this.permissionLogController, SubjectMetadataController: this.subjectMetadataController, AnnouncementController: this.announcementController, NetworkOrderController: this.networkOrderController, diff --git a/lavamoat/browserify/beta/policy.json b/lavamoat/browserify/beta/policy.json index e02799a1129a..53f0d00f1703 100644 --- a/lavamoat/browserify/beta/policy.json +++ b/lavamoat/browserify/beta/policy.json @@ -1625,6 +1625,12 @@ "crypto.getRandomValues": true } }, + "@metamask/permission-log-controller": { + "packages": { + "@metamask/base-controller": true, + "@metamask/utils": true + } + }, "@metamask/phishing-controller": { "globals": { "fetch": true diff --git a/lavamoat/browserify/desktop/policy.json b/lavamoat/browserify/desktop/policy.json index 44b1bc9d4dec..c2d7b0d6f0e4 100644 --- a/lavamoat/browserify/desktop/policy.json +++ b/lavamoat/browserify/desktop/policy.json @@ -1729,6 +1729,12 @@ "crypto.getRandomValues": true } }, + "@metamask/permission-log-controller": { + "packages": { + "@metamask/base-controller": true, + "@metamask/utils": true + } + }, "@metamask/phishing-controller": { "globals": { "fetch": true diff --git a/lavamoat/browserify/flask/policy.json b/lavamoat/browserify/flask/policy.json index 8519a2391c4f..605958f666ac 100644 --- a/lavamoat/browserify/flask/policy.json +++ b/lavamoat/browserify/flask/policy.json @@ -1729,6 +1729,12 @@ "crypto.getRandomValues": true } }, + "@metamask/permission-log-controller": { + "packages": { + "@metamask/base-controller": true, + "@metamask/utils": true + } + }, "@metamask/phishing-controller": { "globals": { "fetch": true diff --git a/lavamoat/browserify/main/policy.json b/lavamoat/browserify/main/policy.json index 8701cc042b27..9476d015310f 100644 --- a/lavamoat/browserify/main/policy.json +++ b/lavamoat/browserify/main/policy.json @@ -1652,6 +1652,12 @@ "crypto.getRandomValues": true } }, + "@metamask/permission-log-controller": { + "packages": { + "@metamask/base-controller": true, + "@metamask/utils": true + } + }, "@metamask/phishing-controller": { "globals": { "fetch": true diff --git a/lavamoat/browserify/mmi/policy.json b/lavamoat/browserify/mmi/policy.json index 79aacd33bc0f..60febb389a4b 100644 --- a/lavamoat/browserify/mmi/policy.json +++ b/lavamoat/browserify/mmi/policy.json @@ -1784,6 +1784,12 @@ "crypto.getRandomValues": true } }, + "@metamask/permission-log-controller": { + "packages": { + "@metamask/base-controller": true, + "@metamask/utils": true + } + }, "@metamask/phishing-controller": { "globals": { "fetch": true diff --git a/package.json b/package.json index 93dbdbcdb738..777a008111d0 100644 --- a/package.json +++ b/package.json @@ -279,6 +279,7 @@ "@metamask/notification-controller": "^3.0.0", "@metamask/obs-store": "^8.1.0", "@metamask/permission-controller": "^8.0.0", + "@metamask/permission-log-controller": "^1.0.0", "@metamask/phishing-controller": "^8.0.0", "@metamask/polling-controller": "^4.0.0", "@metamask/post-message-stream": "^8.0.0", diff --git a/test/mocks/permissions.js b/test/mocks/permissions.js deleted file mode 100644 index 6d9fac4757cf..000000000000 --- a/test/mocks/permissions.js +++ /dev/null @@ -1,369 +0,0 @@ -import deepFreeze from 'deep-freeze-strict'; -import { CaveatTypes } from '../../shared/constants/permissions'; - -/** - * This file contains mocks for the PermissionLogController tests. - */ - -export const noop = () => undefined; - -const keyringAccounts = deepFreeze([ - '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc', - '0xc42edfcc21ed14dda456aa0756c153f7985d8813', - '0x7ae1cdd37bcbdb0e1f491974da8022bfdbf9c2bf', - '0xcc74c7a59194e5d9268476955650d1e285be703c', -]); - -const SUBJECTS = { - a: { origin: 'https://foo.xyz' }, - b: { origin: 'https://bar.abc' }, - c: { origin: 'https://baz.def' }, -}; - -const PERM_NAMES = { - eth_accounts: 'eth_accounts', - test_method: 'test_method', - does_not_exist: 'does_not_exist', -}; - -const ACCOUNTS = { - a: { - permitted: keyringAccounts.slice(0, 3), - primary: keyringAccounts[0], - }, - b: { - permitted: [keyringAccounts[0]], - primary: keyringAccounts[0], - }, - c: { - permitted: [keyringAccounts[1]], - primary: keyringAccounts[1], - }, -}; - -/** - * Helpers for getting mock caveats. - */ -const CAVEATS = { - /** - * Gets a correctly formatted eth_accounts restrictReturnedAccounts caveat. - * - * @param {Array} accounts - The accounts for the caveat - * @returns {object} An eth_accounts restrictReturnedAccounts caveats - */ - eth_accounts: (accounts) => { - return [ - { - type: CaveatTypes.restrictReturnedAccounts, - value: accounts, - }, - ]; - }, -}; - -/** - * Each function here corresponds to what would be a type or interface consumed - * by permissions controller functions if we used TypeScript. - */ -const PERMS = { - /** - * Requested permissions objects, as passed to wallet_requestPermissions. - */ - requests: { - /** - * @returns {object} A permissions request object with eth_accounts - */ - eth_accounts: () => { - return { eth_accounts: {} }; - }, - - /** - * @returns {object} A permissions request object with test_method - */ - test_method: () => { - return { test_method: {} }; - }, - - /** - * @returns {object} A permissions request object with does_not_exist - */ - does_not_exist: () => { - return { does_not_exist: {} }; - }, - }, - - /** - * Partial members of res.result for successful: - * - wallet_requestPermissions - * - wallet_getPermissions - */ - granted: { - /** - * @param {Array} accounts - The accounts for the eth_accounts permission caveat - * @returns {object} A granted permissions object with eth_accounts and its caveat - */ - eth_accounts: (accounts) => { - return { - parentCapability: PERM_NAMES.eth_accounts, - caveats: CAVEATS.eth_accounts(accounts), - }; - }, - - /** - * @returns {object} A granted permissions object with test_method - */ - test_method: () => { - return { - parentCapability: PERM_NAMES.test_method, - }; - }, - }, -}; - -/** - * Objects with function values for getting correctly formatted permissions, - * caveats, errors, permissions requests etc. - */ -export const getters = deepFreeze({ - PERMS, - - /** - * Getters for mock RPC request objects. - */ - RPC_REQUESTS: { - /** - * Gets an arbitrary RPC request object. - * - * @param {string} origin - The origin of the request - * @param {string} method - The request method - * @param {Array} params - The request parameters - * @param {string} [id] - The request id - * @returns {object} An RPC request object - */ - custom: (origin, method, params = [], id) => { - const req = { - origin, - method, - params, - }; - if (id !== undefined) { - req.id = id; - } - return req; - }, - - /** - * Gets an eth_accounts RPC request object. - * - * @param {string} origin - The origin of the request - * @returns {object} An RPC request object - */ - eth_accounts: (origin) => { - return { - origin, - method: 'eth_accounts', - params: [], - }; - }, - - /** - * Gets a test_method RPC request object. - * - * @param {string} origin - The origin of the request - * @param {boolean} param - The request param - * @returns {object} An RPC request object - */ - test_method: (origin, param = false) => { - return { - origin, - method: 'test_method', - params: [param], - }; - }, - - /** - * Gets an eth_requestAccounts RPC request object. - * - * @param {string} origin - The origin of the request - * @returns {object} An RPC request object - */ - eth_requestAccounts: (origin) => { - return { - origin, - method: 'eth_requestAccounts', - params: [], - }; - }, - - /** - * Gets a wallet_requestPermissions RPC request object, - * for a single permission. - * - * @param {string} origin - The origin of the request - * @param {string} permissionName - The name of the permission to request - * @returns {object} An RPC request object - */ - requestPermission: (origin, permissionName) => { - return { - origin, - method: 'wallet_requestPermissions', - params: [PERMS.requests[permissionName]()], - }; - }, - - /** - * Gets a wallet_requestPermissions RPC request object, - * for multiple permissions. - * - * @param {string} origin - The origin of the request - * @param {object} permissions - A permission request object - * @returns {object} An RPC request object - */ - requestPermissions: (origin, permissions = {}) => { - return { - origin, - method: 'wallet_requestPermissions', - params: [permissions], - }; - }, - - /** - * Gets a metamask_sendDomainMetadata RPC request object. - * - * @param {string} origin - The origin of the request - * @param {object} name - The subjectMetadata name - * @param {Array} [args] - Any other data for the request's subjectMetadata - * @returns {object} An RPC request object - */ - metamask_sendDomainMetadata: (origin, name, ...args) => { - return { - origin, - method: 'metamask_sendDomainMetadata', - params: { - ...args, - name, - }, - }; - }, - }, -}); - -/** - * Objects with immutable mock values. - */ -export const constants = deepFreeze({ - REQUEST_IDS: { - a: '1', - b: '2', - c: '3', - }, - - SUBJECTS: { ...SUBJECTS }, - - ACCOUNTS: { ...ACCOUNTS }, - - PERM_NAMES: { ...PERM_NAMES }, - - RESTRICTED_METHODS: new Set(['eth_accounts', 'test_method']), - - /** - * Mock permissions history objects. - */ - EXPECTED_HISTORIES: { - case1: [ - { - [SUBJECTS.a.origin]: { - [PERM_NAMES.eth_accounts]: { - lastApproved: 1, - accounts: { - [ACCOUNTS.a.permitted[0]]: 1, - [ACCOUNTS.a.permitted[1]]: 1, - [ACCOUNTS.a.permitted[2]]: 1, - }, - }, - }, - }, - { - [SUBJECTS.a.origin]: { - [PERM_NAMES.eth_accounts]: { - lastApproved: 2, - accounts: { - [ACCOUNTS.a.permitted[0]]: 2, - [ACCOUNTS.a.permitted[1]]: 1, - [ACCOUNTS.a.permitted[2]]: 1, - }, - }, - }, - }, - ], - - case2: [ - { - [SUBJECTS.a.origin]: { - [PERM_NAMES.eth_accounts]: { - lastApproved: 1, - accounts: {}, - }, - }, - }, - ], - - case3: [ - { - [SUBJECTS.a.origin]: { - [PERM_NAMES.test_method]: { lastApproved: 1 }, - }, - [SUBJECTS.b.origin]: { - [PERM_NAMES.eth_accounts]: { - lastApproved: 1, - accounts: { - [ACCOUNTS.b.permitted[0]]: 1, - }, - }, - }, - [SUBJECTS.c.origin]: { - [PERM_NAMES.test_method]: { lastApproved: 1 }, - [PERM_NAMES.eth_accounts]: { - lastApproved: 1, - accounts: { - [ACCOUNTS.c.permitted[0]]: 1, - }, - }, - }, - }, - { - [SUBJECTS.a.origin]: { - [PERM_NAMES.test_method]: { lastApproved: 2 }, - }, - [SUBJECTS.b.origin]: { - [PERM_NAMES.eth_accounts]: { - lastApproved: 1, - accounts: { - [ACCOUNTS.b.permitted[0]]: 1, - }, - }, - }, - [SUBJECTS.c.origin]: { - [PERM_NAMES.test_method]: { lastApproved: 1 }, - [PERM_NAMES.eth_accounts]: { - lastApproved: 2, - accounts: { - [ACCOUNTS.c.permitted[0]]: 1, - [ACCOUNTS.b.permitted[0]]: 2, - }, - }, - }, - }, - ], - - case4: [ - { - [SUBJECTS.a.origin]: { - [PERM_NAMES.test_method]: { - lastApproved: 1, - }, - }, - }, - ], - }, -}); diff --git a/yarn.lock b/yarn.lock index 03facb7815be..0f144c375dda 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4835,6 +4835,17 @@ __metadata: languageName: node linkType: hard +"@metamask/permission-log-controller@npm:^1.0.0": + version: 1.0.0 + resolution: "@metamask/permission-log-controller@npm:1.0.0" + dependencies: + "@metamask/base-controller": "npm:^4.1.1" + "@metamask/json-rpc-engine": "npm:^7.3.2" + "@metamask/utils": "npm:^8.3.0" + checksum: c9a53516aabe9af206e97222a169d440a2287075046fadc03b27d72e6db0063fe116a4368c0d785ad0157d8ee5c0f9337c4f2772e10418e39c59091b704d69aa + languageName: node + linkType: hard + "@metamask/phishing-controller@npm:^8.0.0, @metamask/phishing-controller@npm:^8.0.1, @metamask/phishing-controller@npm:^8.0.2": version: 8.0.2 resolution: "@metamask/phishing-controller@npm:8.0.2" @@ -24811,6 +24822,7 @@ __metadata: "@metamask/notification-controller": "npm:^3.0.0" "@metamask/obs-store": "npm:^8.1.0" "@metamask/permission-controller": "npm:^8.0.0" + "@metamask/permission-log-controller": "npm:^1.0.0" "@metamask/phishing-controller": "npm:^8.0.0" "@metamask/phishing-warning": "npm:^3.0.3" "@metamask/polling-controller": "npm:^4.0.0"