diff --git a/app/scripts/background.js b/app/scripts/background.js index 92f55319a..af9fbed2f 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -119,6 +119,7 @@ initialize().catch(log.error) * @property {number} unapprovedDecryptMsgCount - The number of messages in unapprovedDecryptMsgs. * @property {Object} unapprovedTypedMsgs - An object of messages pending approval, mapping a unique ID to the options. * @property {number} unapprovedTypedMsgCount - The number of messages in unapprovedTypedMsgs. + * @property {number} pendingApprovalCount - The number of pending request in the approval controller. * @property {string[]} keyringTypes - An array of unique keyring identifying strings, representing available strategies for creating accounts. * @property {Keyring[]} keyrings - An array of keyring descriptions, summarizing the accounts that are available for use, and what keyrings they belong to. * @property {string} selectedAddress - A lower case hex string of the currently selected address. @@ -394,7 +395,7 @@ function setupController(initState, initLangCode) { controller.decryptMessageManager.on('updateBadge', updateBadge) controller.encryptionPublicKeyManager.on('updateBadge', updateBadge) controller.typedMessageManager.on('updateBadge', updateBadge) - controller.permissionsController.permissions.subscribe(updateBadge) + controller.approvalController.subscribe(updateBadge) controller.appStateController.on('updateBadge', updateBadge) /** @@ -411,9 +412,7 @@ function setupController(initState, initLangCode) { unapprovedEncryptionPublicKeyMsgCount, } = controller.encryptionPublicKeyManager const { unapprovedTypedMessagesCount } = controller.typedMessageManager - const pendingPermissionRequests = Object.keys( - controller.permissionsController.permissions.state.permissionsRequests, - ).length + const pendingApprovalCount = controller.approvalController.getTotalApprovalCount() const waitingForUnlockCount = controller.appStateController.waitingForUnlock.length const count = @@ -423,7 +422,7 @@ function setupController(initState, initLangCode) { unapprovedDecryptMsgCount + unapprovedEncryptionPublicKeyMsgCount + unapprovedTypedMessagesCount + - pendingPermissionRequests + + pendingApprovalCount + waitingForUnlockCount if (count) { label = String(count) diff --git a/app/scripts/controllers/permissions/enums.js b/app/scripts/controllers/permissions/enums.js index d5ca52e30..cfb595144 100644 --- a/app/scripts/controllers/permissions/enums.js +++ b/app/scripts/controllers/permissions/enums.js @@ -1,3 +1,5 @@ +export const APPROVAL_TYPE = 'wallet_requestPermissions' + export const WALLET_PREFIX = 'wallet_' export const HISTORY_STORE_KEY = 'permissionsHistory' diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index 454bd8e56..b9d4ddd47 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -11,6 +11,7 @@ import PermissionsLogController from './permissionsLog' // Methods that do not require any permissions to use: import { + APPROVAL_TYPE, SAFE_METHODS, // methods that do not require any permissions to use WALLET_PREFIX, METADATA_STORE_KEY, @@ -22,9 +23,13 @@ import { CAVEAT_TYPES, } from './enums' +// instanbul ignore next +const noop = () => undefined + export class PermissionsController { constructor( { + approvals, getKeyringAccounts, getRestrictedMethods, getUnlockPromise, @@ -32,7 +37,6 @@ export class PermissionsController { notifyDomain, notifyAllDomains, preferences, - showPermissionRequest, } = {}, restoredPermissions = {}, restoredState = {}, @@ -47,7 +51,6 @@ export class PermissionsController { this._getUnlockPromise = getUnlockPromise this._notifyDomain = notifyDomain this._notifyAllDomains = notifyAllDomains - this._showPermissionRequest = showPermissionRequest this._isUnlocked = isUnlocked this._restrictedMethods = getRestrictedMethods({ @@ -58,8 +61,12 @@ export class PermissionsController { restrictedMethods: Object.keys(this._restrictedMethods), store: this.store, }) - this.pendingApprovals = new Map() - this.pendingApprovalOrigins = new Set() + + /** + * @type {import('@metamask/controllers').ApprovalController} + * @public + */ + this.approvals = approvals this._initializePermissions(restoredPermissions) this._lastSelectedAddress = preferences.getState().selectedAddress this.preferences = preferences @@ -139,7 +146,7 @@ export class PermissionsController { { origin }, req, res, - () => undefined, + noop, _end, ) @@ -193,13 +200,7 @@ export class PermissionsController { } const res = {} - this.permissions.providerMiddlewareFunction( - domain, - req, - res, - () => undefined, - _end, - ) + this.permissions.providerMiddlewareFunction(domain, req, res, noop, _end) function _end(_err) { const err = _err || res.error @@ -223,16 +224,16 @@ export class PermissionsController { */ async approvePermissionsRequest(approved, accounts) { const { id } = approved.metadata - const approval = this.pendingApprovals.get(id) - if (!approval) { - log.debug(`Permissions request with id '${id}' not found`) + if (!this.approvals.has({ id })) { + log.debug(`Permissions request with id '${id}' not found.`) return } try { if (Object.keys(approved.permissions).length === 0) { - approval.reject( + this.approvals.reject( + id, ethErrors.rpc.invalidRequest({ message: 'Must request at least one permission.', }), @@ -244,19 +245,18 @@ export class PermissionsController { approved.permissions, accounts, ) - approval.resolve(approved.permissions) + this.approvals.resolve(id, approved.permissions) } } catch (err) { // if finalization fails, reject the request - approval.reject( + this.approvals.reject( + id, ethErrors.rpc.invalidRequest({ message: err.message, data: err, }), ) } - - this._removePendingApproval(id) } /** @@ -267,15 +267,12 @@ export class PermissionsController { * @param {string} id - The id of the request rejected by the user */ async rejectPermissionsRequest(id) { - const approval = this.pendingApprovals.get(id) - - if (!approval) { - log.debug(`Permissions request with id '${id}' not found`) + if (!this.approvals.has({ id })) { + log.debug(`Permissions request with id '${id}' not found.`) return } - approval.reject(ethErrors.provider.userRejectedRequest()) - this._removePendingApproval(id) + this.approvals.reject(id, ethErrors.provider.userRejectedRequest()) } /** @@ -670,37 +667,6 @@ export class PermissionsController { this.notifyAccountsChanged(origin, permittedAccounts) } - /** - * Adds a pending approval. - * @param {string} id - The id of the pending approval. - * @param {string} origin - The origin of the pending approval. - * @param {Function} resolve - The function resolving the pending approval Promise. - * @param {Function} reject - The function rejecting the pending approval Promise. - */ - _addPendingApproval(id, origin, resolve, reject) { - if ( - this.pendingApprovalOrigins.has(origin) || - this.pendingApprovals.has(id) - ) { - throw new Error( - `Pending approval with id '${id}' or origin '${origin}' already exists.`, - ) - } - - this.pendingApprovals.set(id, { origin, resolve, reject }) - this.pendingApprovalOrigins.add(origin) - } - - /** - * Removes the pending approval with the given id. - * @param {string} id - The id of the pending approval to remove. - */ - _removePendingApproval(id) { - const { origin } = this.pendingApprovals.get(id) - this.pendingApprovalOrigins.delete(origin) - this.pendingApprovals.delete(id) - } - /** * A convenience method for retrieving a login object * or creating a new one if needed. @@ -735,16 +701,10 @@ export class PermissionsController { metadata: { id, origin }, } = req - if (this.pendingApprovalOrigins.has(origin)) { - throw ethErrors.rpc.resourceUnavailable( - 'Permissions request already pending; please wait.', - ) - } - - this._showPermissionRequest() - - return new Promise((resolve, reject) => { - this._addPendingApproval(id, origin, resolve, reject) + return this.approvals.addAndShowApprovalRequest({ + id, + origin, + type: APPROVAL_TYPE, }) }, }, diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 85135f759..f86bb1468 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -18,6 +18,7 @@ import nanoid from 'nanoid' import contractMap from '@metamask/contract-metadata' import { AddressBookController, + ApprovalController, CurrencyRateController, PhishingController, } from '@metamask/controllers' @@ -102,6 +103,10 @@ export default class MetamaskController extends EventEmitter { // next, we will initialize the controllers // controller initialization order matters + this.approvalController = new ApprovalController({ + showApprovalRequest: opts.showUserConfirmation, + }) + this.networkController = new NetworkController(initState.NetworkController) this.networkController.setInfuraProjectId(opts.infuraProjectId) @@ -225,6 +230,7 @@ export default class MetamaskController extends EventEmitter { this.permissionsController = new PermissionsController( { + approvals: this.approvalController, getKeyringAccounts: this.keyringController.getAccounts.bind( this.keyringController, ), @@ -393,8 +399,8 @@ export default class MetamaskController extends EventEmitter { PermissionsMetadata: this.permissionsController.store, ThreeBoxController: this.threeBoxController.store, SwapsController: this.swapsController.store, - // ENS Controller EnsController: this.ensController.store, + ApprovalController: this.approvalController, }) this.memStore.subscribe(this.sendUpdate.bind(this)) diff --git a/test/unit/app/controllers/permissions/mocks.js b/test/unit/app/controllers/permissions/mocks.js index d6b93dd68..a3e72e835 100644 --- a/test/unit/app/controllers/permissions/mocks.js +++ b/test/unit/app/controllers/permissions/mocks.js @@ -1,6 +1,8 @@ import { ethErrors, ERROR_CODES } from 'eth-json-rpc-errors' import deepFreeze from 'deep-freeze-strict' +import { ApprovalController } from '@metamask/controllers' + import _getRestrictedMethods from '../../../../../app/scripts/controllers/permissions/restrictedMethods' import { @@ -67,6 +69,9 @@ const getRestrictedMethods = (permController) => { */ export function getPermControllerOpts() { return { + approvals: new ApprovalController({ + showApprovalRequest: noop, + }), getKeyringAccounts: async () => [...keyringAccounts], getUnlockPromise: () => Promise.resolve(), getRestrictedMethods, @@ -423,9 +428,9 @@ export const getters = deepFreeze({ message: `Pending approval with id '${id}' or origin '${origin}' already exists.`, } }, - requestAlreadyPending: () => { + requestAlreadyPending: (origin) => { return { - message: 'Permissions request already pending; please wait.', + message: `Request of type 'wallet_requestPermissions' already pending for origin ${origin}. Please wait.`, } }, }, diff --git a/test/unit/app/controllers/permissions/permissions-controller-test.js b/test/unit/app/controllers/permissions/permissions-controller-test.js index 434ae6bd3..2eee261f1 100644 --- a/test/unit/app/controllers/permissions/permissions-controller-test.js +++ b/test/unit/app/controllers/permissions/permissions-controller-test.js @@ -1,6 +1,5 @@ import { strict as assert } from 'assert' import { find } from 'lodash' -import nanoid from 'nanoid' import sinon from 'sinon' import { @@ -13,7 +12,6 @@ import { PermissionsController } from '../../../../../app/scripts/controllers/pe import { getRequestUserApprovalHelper, grantPermissions } from './helpers' import { - noop, constants, getters, getNotifyDomain, @@ -49,6 +47,15 @@ const initPermController = (notifications = initNotifications()) => { } describe('permissions controller', function () { + describe('constructor', function () { + it('throws on undefined argument', function () { + assert.throws( + () => new PermissionsController(), + 'should throw on undefined argument', + ) + }) + }) + describe('getAccounts', function () { let permController @@ -1006,12 +1013,6 @@ describe('permissions controller', function () { }) it('does nothing if called on non-existing request', async function () { - assert.equal( - permController.pendingApprovals.size, - 0, - 'pending approvals should be empty on init', - ) - sinon.spy(permController, 'finalizePermissionsRequest') const request = PERMS.approvedRequest(REQUEST_IDS.a, null) @@ -1025,12 +1026,6 @@ describe('permissions controller', function () { permController.finalizePermissionsRequest.notCalled, 'should not call finalizePermissionRequest', ) - - assert.equal( - permController.pendingApprovals.size, - 0, - 'pending approvals should still be empty after request', - ) }) it('rejects request with bad accounts param', async function () { @@ -1047,12 +1042,6 @@ describe('permissions controller', function () { await permController.approvePermissionsRequest(request, null) await rejectionPromise - - assert.equal( - permController.pendingApprovals.size, - 0, - 'pending approvals should be empty after rejection', - ) }) it('rejects request with no permissions', async function () { @@ -1069,12 +1058,6 @@ describe('permissions controller', function () { ACCOUNTS.a.permitted, ) await requestRejection - - assert.equal( - permController.pendingApprovals.size, - 0, - 'pending approvals should be empty after rejection', - ) }) it('approves valid request', async function () { @@ -1100,12 +1083,6 @@ describe('permissions controller', function () { PERMS.finalizedRequests.eth_accounts(ACCOUNTS.a.permitted), 'should produce expected approved permissions', ) - - assert.equal( - permController.pendingApprovals.size, - 0, - 'pending approvals should be empty after approval', - ) }) it('approves valid requests regardless of order', async function () { @@ -1161,12 +1138,6 @@ describe('permissions controller', function () { PERMS.finalizedRequests.eth_accounts(ACCOUNTS.b.permitted), 'second request should produce expected approved permissions', ) - - assert.equal( - permController.pendingApprovals.size, - 0, - 'pending approvals should be empty after approvals', - ) }) }) @@ -1179,22 +1150,14 @@ describe('permissions controller', function () { }) it('does nothing if called on non-existing request', async function () { - assert.equal( - permController.pendingApprovals.size, - 0, - 'pending approvals should be empty on init', + permController.approvals.add = sinon.fake.throws( + new Error('should not call add'), ) await assert.doesNotReject( permController.rejectPermissionsRequest(REQUEST_IDS.a), 'should not throw on non-existing request', ) - - assert.equal( - permController.pendingApprovals.size, - 0, - 'pending approvals should still be empty after request', - ) }) it('rejects single existing request', async function () { @@ -1206,12 +1169,6 @@ describe('permissions controller', function () { await permController.rejectPermissionsRequest(REQUEST_IDS.a) await requestRejection - - assert.equal( - permController.pendingApprovals.size, - 0, - 'pending approvals should be empty after rejection', - ) }) it('rejects requests regardless of order', async function () { @@ -1235,12 +1192,6 @@ describe('permissions controller', function () { await requestRejection1 await requestRejection2 - - assert.equal( - permController.pendingApprovals.size, - 0, - 'pending approvals should be empty after approval', - ) }) }) @@ -1568,13 +1519,8 @@ describe('permissions controller', function () { }) describe('miscellanea and edge cases', function () { - let permController - - beforeEach(function () { - permController = initPermController() - }) - it('requestAccountsPermissionWithId calls _requestAccountsPermission with an explicit request ID', async function () { + const permController = initPermController() const _requestPermissions = sinon .stub(permController, '_requestPermissions') .resolves() @@ -1588,43 +1534,5 @@ describe('permissions controller', function () { ) _requestPermissions.restore() }) - - it('_addPendingApproval: should throw if adding origin twice', function () { - const id = nanoid() - const origin = DOMAINS.a - - permController._addPendingApproval(id, origin, noop, noop) - - const otherId = nanoid() - - assert.throws( - () => permController._addPendingApproval(otherId, origin, noop, noop), - ERRORS.pendingApprovals.duplicateOriginOrId(otherId, origin), - 'should throw expected error', - ) - - assert.equal( - permController.pendingApprovals.size, - 1, - 'pending approvals should have single entry', - ) - - assert.equal( - permController.pendingApprovalOrigins.size, - 1, - 'pending approval origins should have single item', - ) - - assert.deepEqual( - permController.pendingApprovals.get(id), - { origin, resolve: noop, reject: noop }, - 'pending approvals should have expected entry', - ) - - assert.ok( - permController.pendingApprovalOrigins.has(origin), - 'pending approval origins should have expected item', - ) - }) }) }) diff --git a/test/unit/app/controllers/permissions/permissions-middleware-test.js b/test/unit/app/controllers/permissions/permissions-middleware-test.js index ff4e832c9..9e2769ed6 100644 --- a/test/unit/app/controllers/permissions/permissions-middleware-test.js +++ b/test/unit/app/controllers/permissions/permissions-middleware-test.js @@ -18,6 +18,20 @@ const { CAVEATS, ERRORS, PERMS, RPC_REQUESTS } = getters const { ACCOUNTS, DOMAINS, PERM_NAMES } = constants +const initPermController = () => { + return new PermissionsController({ + ...getPermControllerOpts(), + }) +} + +const createApprovalSpies = (permController) => { + sinon.spy(permController.approvals, '_add') +} + +const getNextApprovalId = (permController) => { + return permController.approvals._approvals.keys().next().value +} + const validatePermission = (perm, name, origin, caveats) => { assert.equal( name, @@ -36,12 +50,6 @@ const validatePermission = (perm, name, origin, caveats) => { } } -const initPermController = () => { - return new PermissionsController({ - ...getPermControllerOpts(), - }) -} - describe('permissions middleware', function () { describe('wallet_requestPermissions', function () { let permController @@ -52,6 +60,8 @@ describe('permissions middleware', function () { }) it('grants permissions on user approval', async function () { + createApprovalSpies(permController) + const aMiddleware = getPermissionsMiddleware( permController, DOMAINS.a.origin, @@ -72,13 +82,12 @@ describe('permissions middleware', function () { await userApprovalPromise - assert.equal( - permController.pendingApprovals.size, - 1, - 'perm controller should have single pending approval', + assert.ok( + permController.approvals._add.calledOnce, + 'should have added single approval request', ) - const id = permController.pendingApprovals.keys().next().value + const id = getNextApprovalId(permController) const approvedReq = PERMS.approvedRequest( id, PERMS.requests.eth_accounts(), @@ -150,7 +159,7 @@ describe('permissions middleware', function () { await userApprovalPromise - const id1 = permController.pendingApprovals.keys().next().value + const id1 = getNextApprovalId(permController) const approvedReq1 = PERMS.approvedRequest( id1, PERMS.requests.eth_accounts(), @@ -219,7 +228,7 @@ describe('permissions middleware', function () { await userApprovalPromise - const id2 = permController.pendingApprovals.keys().next().value + const id2 = getNextApprovalId(permController) const approvedReq2 = PERMS.approvedRequest(id2, { ...requestedPerms2 }) await permController.approvePermissionsRequest( @@ -275,6 +284,8 @@ describe('permissions middleware', function () { }) it('rejects permissions on user rejection', async function () { + createApprovalSpies(permController) + const aMiddleware = getPermissionsMiddleware( permController, DOMAINS.a.origin, @@ -298,13 +309,12 @@ describe('permissions middleware', function () { await userApprovalPromise - assert.equal( - permController.pendingApprovals.size, - 1, - 'perm controller should have single pending approval', + assert.ok( + permController.approvals._add.calledOnce, + 'should have added single approval request', ) - const id = permController.pendingApprovals.keys().next().value + const id = getNextApprovalId(permController) await permController.rejectPermissionsRequest(id) await requestRejection @@ -328,6 +338,8 @@ describe('permissions middleware', function () { }) it('rejects requests with unknown permissions', async function () { + createApprovalSpies(permController) + const aMiddleware = getPermissionsMiddleware( permController, DOMAINS.a.origin, @@ -349,10 +361,9 @@ describe('permissions middleware', function () { 'request should be rejected with correct error', ) - assert.equal( - permController.pendingApprovals.size, - 0, - 'perm controller should have no pending approvals', + assert.ok( + permController.approvals._add.notCalled, + 'no approval requests should have been added', ) assert.ok( @@ -367,7 +378,7 @@ describe('permissions middleware', function () { }) it('accepts only a single pending permissions request per origin', async function () { - const expectedError = ERRORS.pendingApprovals.requestAlreadyPending() + createApprovalSpies(permController) // two middlewares for two origins @@ -414,10 +425,9 @@ describe('permissions middleware', function () { await userApprovalPromise - assert.equal( - permController.pendingApprovals.size, - 2, - 'perm controller should have expected number of pending approvals', + assert.ok( + permController.approvals._add.calledTwice, + 'should have added two approval requests', ) // create and start processing second request for first origin, @@ -431,6 +441,10 @@ describe('permissions middleware', function () { userApprovalPromise = getUserApprovalPromise(permController) + const expectedError = ERRORS.pendingApprovals.requestAlreadyPending( + DOMAINS.a.origin, + ) + const requestApprovalFail = assert.rejects( aMiddleware(reqA2, resA2), expectedError, @@ -447,17 +461,20 @@ describe('permissions middleware', function () { 'response should have expected error and no result', ) - // first requests for both origins should remain - assert.equal( - permController.pendingApprovals.size, + permController.approvals._add.callCount, + 3, + 'should have attempted to create three pending approvals', + ) + assert.equal( + permController.approvals._approvals.size, 2, - 'perm controller should have expected number of pending approvals', + 'should only have created two pending approvals', ) // now, remaining pending requests should be approved without issue - for (const id of permController.pendingApprovals.keys()) { + for (const id of permController.approvals._approvals.keys()) { await permController.approvePermissionsRequest( PERMS.approvedRequest(id, PERMS.requests.test_method()), ) @@ -484,12 +501,6 @@ describe('permissions middleware', function () { 1, 'second origin should have single approved permission', ) - - assert.equal( - permController.pendingApprovals.size, - 0, - 'perm controller should have expected number of pending approvals', - ) }) }) @@ -609,6 +620,8 @@ describe('permissions middleware', function () { }) it('requests accounts for unpermitted origin, and approves on user approval', async function () { + createApprovalSpies(permController) + const userApprovalPromise = getUserApprovalPromise(permController) const aMiddleware = getPermissionsMiddleware( @@ -626,13 +639,12 @@ describe('permissions middleware', function () { await userApprovalPromise - assert.equal( - permController.pendingApprovals.size, - 1, - 'perm controller should have single pending approval', + assert.ok( + permController.approvals._add.calledOnce, + 'should have added single approval request', ) - const id = permController.pendingApprovals.keys().next().value + const id = getNextApprovalId(permController) const approvedReq = PERMS.approvedRequest( id, PERMS.requests.eth_accounts(), @@ -685,6 +697,8 @@ describe('permissions middleware', function () { }) it('requests accounts for unpermitted origin, and rejects on user rejection', async function () { + createApprovalSpies(permController) + const userApprovalPromise = getUserApprovalPromise(permController) const aMiddleware = getPermissionsMiddleware( @@ -705,13 +719,12 @@ describe('permissions middleware', function () { await userApprovalPromise - assert.equal( - permController.pendingApprovals.size, - 1, - 'perm controller should have single pending approval', + assert.ok( + permController.approvals._add.calledOnce, + 'should have added single approval request', ) - const id = permController.pendingApprovals.keys().next().value + const id = getNextApprovalId(permController) await permController.rejectPermissionsRequest(id) await requestRejection @@ -788,7 +801,7 @@ describe('permissions middleware', function () { // this will reject because of the already pending request await assert.rejects( cMiddleware({ ...req }, {}), - ERRORS.eth_requestAccounts.requestAlreadyPending(), + ERRORS.eth_requestAccounts.requestAlreadyPending(DOMAINS.c.origin), ) // now unlock and let through the first request