m/metamask-extension
1
0
Fork 0
mirror of https://github.com/kremalicious/metamask-extension.git synced 2024-11-22 18:00:18 +01:00
Code

Add approval controller (#9401)

Browse Source 
This PR introduces the new approval controller to the extension codebase. We use it for the permissions controller's pending approval functionality.

The approval controller sets us up for a new pattern of requesting and managing user confirmations in RPC methods. Along with the generic RPC method middleware, the approval controller will allow us to eliminate our message managers, and decouple various method handlers from our provider stack, making the implementations more portable between the extension and mobile.
This commit is contained in:
Erik Marks 2020-12-14 08:04:26 -08:00 committed by GitHub
parent 5e01602a01
commit 8f40d03299
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 121 additions and 228 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
app/scripts/background.js
View File

@ -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)

2
app/scripts/controllers/permissions/enums.js
View File

@ -1,3 +1,5 @@
export const APPROVAL_TYPE = 'wallet_requestPermissions'
export const WALLET_PREFIX = 'wallet_'
export const HISTORY_STORE_KEY = 'permissionsHistory'

94
app/scripts/controllers/permissions/index.js
View File

@ -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,
})
},
},

8
app/scripts/metamask-controller.js
View File

@ -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))

9
test/unit/app/controllers/permissions/mocks.js
View File

@ -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.`,
}
},
},

116
test/unit/app/controllers/permissions/permissions-controller-test.js
View File

@ -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',
)
})
})
})

111
test/unit/app/controllers/permissions/permissions-middleware-test.js
View File

@ -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