From 2301d9980eda9629c01a1d1ca4ae264f7545941e Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Mon, 23 Mar 2020 09:25:55 -0700 Subject: [PATCH] Wait for extension unlock before processing eth_requestAccounts (#8149) * eth_requestAccounts: wait on unlock return error on duplicate eth_requestAccounts add getUnlockPromise mock to permissions unit tests * only await unlock if already permitted * add notification badge for wait on unlock * fixup * more fixup * cleanup * update keyring controller, us its unlock event * move keyring update unlock logic to unlock event handler * fix unit tests * delete onUnlock handler * fix eth-keyring-controller resolution * update eth-keyring-controller --- app/scripts/background.js | 4 +- app/scripts/controllers/app-state.js | 47 ++++++++++++++++++- app/scripts/controllers/permissions/index.js | 25 ++++++++-- .../permissions/methodMiddleware.js | 23 ++++++++- app/scripts/metamask-controller.js | 39 ++++++++------- package.json | 2 +- .../unit/app/controllers/permissions/mocks.js | 13 ++++- .../permissions-controller-test.js | 43 +++++++++++++++++ .../permissions-middleware-test.js | 46 ++++++++++++++++++ yarn.lock | 10 ++-- 10 files changed, 222 insertions(+), 30 deletions(-) diff --git a/app/scripts/background.js b/app/scripts/background.js index dbac8854f..fa5bd7498 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -421,6 +421,7 @@ function setupController (initState, initLangCode) { controller.encryptionPublicKeyManager.on('updateBadge', updateBadge) controller.typedMessageManager.on('updateBadge', updateBadge) controller.permissionsController.permissions.subscribe(updateBadge) + controller.appStateController.on('updateBadge', updateBadge) /** * Updates the Web Extension's "badge" number, on the little fox in the toolbar. @@ -435,8 +436,9 @@ function setupController (initState, initLangCode) { const unapprovedEncryptionPublicKeyMsgCount = controller.encryptionPublicKeyManager.unapprovedEncryptionPublicKeyMsgCount const unapprovedTypedMessagesCount = controller.typedMessageManager.unapprovedTypedMessagesCount const pendingPermissionRequests = Object.keys(controller.permissionsController.permissions.state.permissionsRequests).length + const waitingForUnlockCount = controller.appStateController.waitingForUnlock.length const count = unapprovedTxCount + unapprovedMsgCount + unapprovedPersonalMsgCount + unapprovedDecryptMsgCount + unapprovedEncryptionPublicKeyMsgCount + - unapprovedTypedMessagesCount + pendingPermissionRequests + unapprovedTypedMessagesCount + pendingPermissionRequests + waitingForUnlockCount if (count) { label = String(count) } diff --git a/app/scripts/controllers/app-state.js b/app/scripts/controllers/app-state.js index 27e3a1acb..1aac34650 100644 --- a/app/scripts/controllers/app-state.js +++ b/app/scripts/controllers/app-state.js @@ -1,14 +1,23 @@ import ObservableStore from 'obs-store' +import EventEmitter from 'events' -class AppStateController { +class AppStateController extends EventEmitter { /** * @constructor * @param opts */ constructor (opts = {}) { - const { initState, onInactiveTimeout, preferencesStore } = opts + const { + addUnlockListener, + isUnlocked, + initState, + onInactiveTimeout, + preferencesStore, + } = opts const { preferences } = preferencesStore.getState() + super() + this.onInactiveTimeout = onInactiveTimeout || (() => {}) this.store = new ObservableStore(Object.assign({ timeoutMinutes: 0, @@ -16,6 +25,10 @@ class AppStateController { }, initState)) this.timer = null + this.isUnlocked = isUnlocked + this.waitingForUnlock = [] + addUnlockListener(this.handleUnlock.bind(this)) + preferencesStore.subscribe((state) => { this._setInactiveTimeout(state.preferences.autoLockTimeLimit) }) @@ -23,6 +36,36 @@ class AppStateController { this._setInactiveTimeout(preferences.autoLockTimeLimit) } + /** + * Get a Promise that resolves when the extension is unlocked. + * This Promise will never reject. + * + * @returns {Promise} A promise that resolves when the extension is + * unlocked, or immediately if the extension is already unlocked. + */ + getUnlockPromise () { + return new Promise((resolve) => { + if (this.isUnlocked()) { + resolve() + } else { + this.waitingForUnlock.push({ resolve }) + this.emit('updateBadge') + } + }) + } + + /** + * Drains the waitingForUnlock queue, resolving all the related Promises. + */ + handleUnlock () { + if (this.waitingForUnlock.length > 0) { + while (this.waitingForUnlock.length > 0) { + this.waitingForUnlock.shift().resolve() + } + this.emit('updateBadge') + } + } + setMkrMigrationReminderTimestamp (timestamp) { this.store.updateState({ mkrMigrationReminderTimestamp: timestamp, diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index eb62bb219..637208f32 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -24,8 +24,12 @@ export class PermissionsController { constructor ( { - platform, notifyDomain, notifyAllDomains, - getKeyringAccounts, getRestrictedMethods, + getKeyringAccounts, + getRestrictedMethods, + getUnlockPromise, + notifyDomain, + notifyAllDomains, + platform, } = {}, restoredPermissions = {}, restoredState = {}) { @@ -36,10 +40,12 @@ export class PermissionsController { [HISTORY_STORE_KEY]: restoredState[HISTORY_STORE_KEY] || {}, }) + this.getKeyringAccounts = getKeyringAccounts + this.getUnlockPromise = getUnlockPromise this._notifyDomain = notifyDomain this.notifyAllDomains = notifyAllDomains - this.getKeyringAccounts = getKeyringAccounts this._platform = platform + this._restrictedMethods = getRestrictedMethods(this) this.permissionsLog = new PermissionsLogController({ restrictedMethods: Object.keys(this._restrictedMethods), @@ -73,6 +79,8 @@ export class PermissionsController { store: this.store, storeKey: METADATA_STORE_KEY, getAccounts: this.getAccounts.bind(this, origin), + getUnlockPromise: this.getUnlockPromise, + hasPermission: this.hasPermission.bind(this, origin), requestAccountsPermission: this._requestPermissions.bind( this, origin, { eth_accounts: {} } ), @@ -111,6 +119,17 @@ export class PermissionsController { }) } + /** + * Returns whether the given origin has the given permission. + * + * @param {string} origin - The origin to check. + * @param {string} permission - The permission to check for. + * @returns {boolean} Whether the origin has the permission. + */ + hasPermission (origin, permission) { + return Boolean(this.permissions.getPermission(origin, permission)) + } + /** * Submits a permissions request to rpc-cap. Internal, background use only. * diff --git a/app/scripts/controllers/permissions/methodMiddleware.js b/app/scripts/controllers/permissions/methodMiddleware.js index 0656981ff..3eed9c085 100644 --- a/app/scripts/controllers/permissions/methodMiddleware.js +++ b/app/scripts/controllers/permissions/methodMiddleware.js @@ -5,8 +5,16 @@ import { ethErrors } from 'eth-json-rpc-errors' * Create middleware for handling certain methods and preprocessing permissions requests. */ export default function createMethodMiddleware ({ - store, storeKey, getAccounts, requestAccountsPermission, + getAccounts, + getUnlockPromise, + hasPermission, + requestAccountsPermission, + store, + storeKey, }) { + + let isProcessingRequestAccounts = false + return createAsyncMiddleware(async (req, res, next) => { switch (req.method) { @@ -21,6 +29,19 @@ export default function createMethodMiddleware ({ case 'eth_requestAccounts': + if (isProcessingRequestAccounts) { + res.error = ethErrors.rpc.resourceUnavailable( + 'Already processing eth_requestAccounts. Please wait.' + ) + return + } + + if (hasPermission('eth_accounts')) { + isProcessingRequestAccounts = true + await getUnlockPromise() + isProcessingRequestAccounts = false + } + // first, just try to get accounts let accounts = await getAccounts() if (accounts.length > 0) { diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index a70bcd5f1..68e4f6f03 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -121,9 +121,11 @@ export default class MetamaskController extends EventEmitter { }) this.appStateController = new AppStateController({ - preferencesStore: this.preferencesController.store, - onInactiveTimeout: () => this.setLocked(), + addUnlockListener: this.on.bind(this, 'unlock'), + isUnlocked: this.isUnlocked.bind(this), initState: initState.AppStateController, + onInactiveTimeout: () => this.setLocked(), + preferencesStore: this.preferencesController.store, }) this.currencyRateController = new CurrencyRateController(undefined, initState.CurrencyController) @@ -206,13 +208,15 @@ export default class MetamaskController extends EventEmitter { encryptor: opts.encryptor || undefined, }) this.keyringController.memStore.subscribe((s) => this._onKeyringControllerUpdate(s)) + this.keyringController.on('unlock', () => this.emit('unlock')) this.permissionsController = new PermissionsController({ getKeyringAccounts: this.keyringController.getAccounts.bind(this.keyringController), - platform: opts.platform, + getRestrictedMethods, + getUnlockPromise: this.appStateController.getUnlockPromise.bind(this.appStateController), notifyDomain: this.notifyConnections.bind(this), notifyAllDomains: this.notifyAllConnections.bind(this), - getRestrictedMethods, + platform: opts.platform, }, initState.PermissionsController, initState.PermissionsMetadata) this.detectTokensController = new DetectTokensController({ @@ -352,9 +356,7 @@ export default class MetamaskController extends EventEmitter { if (origin === 'metamask') { const selectedAddress = this.preferencesController.getSelectedAddress() return selectedAddress ? [selectedAddress] : [] - } else if ( - this.keyringController.memStore.getState().isUnlocked - ) { + } else if (this.isUnlocked()) { return await this.permissionsController.getAccounts(origin) } return [] // changing this is a breaking change @@ -613,11 +615,11 @@ export default class MetamaskController extends EventEmitter { this.preferencesController.setAddresses(accounts) this.selectFirstIdentity() } - releaseLock() return vault } catch (err) { - releaseLock() throw err + } finally { + releaseLock() } } @@ -657,11 +659,11 @@ export default class MetamaskController extends EventEmitter { // set new identities this.preferencesController.setAddresses(accounts) this.selectFirstIdentity() - releaseLock() return vault } catch (err) { - releaseLock() throw err + } finally { + releaseLock() } } @@ -1730,9 +1732,8 @@ export default class MetamaskController extends EventEmitter { */ notifyConnections (origin, payload) { - const { isUnlocked } = this.getState() const connections = this.connections[origin] - if (!isUnlocked || !connections) { + if (!this.isUnlocked() || !connections) { return } @@ -1750,8 +1751,7 @@ export default class MetamaskController extends EventEmitter { */ notifyAllConnections (payload) { - const { isUnlocked } = this.getState() - if (!isUnlocked) { + if (!this.isUnlocked()) { return } @@ -1793,6 +1793,13 @@ export default class MetamaskController extends EventEmitter { this.emit('update', this.getState()) } + /** + * @returns {boolean} Whether the extension is unlocked. + */ + isUnlocked () { + return this.keyringController.memStore.getState().isUnlocked + } + //============================================================================= // MISCELLANEOUS //============================================================================= @@ -2083,7 +2090,7 @@ export default class MetamaskController extends EventEmitter { */ set isClientOpen (open) { this._isClientOpen = open - this.isClientOpenAndUnlocked = this.getState().isUnlocked && open + this.isClientOpenAndUnlocked = this.isUnlocked() && open this.detectTokensController.isOpen = open } diff --git a/package.json b/package.json index 5107f687b..81996d183 100644 --- a/package.json +++ b/package.json @@ -98,7 +98,7 @@ "eth-json-rpc-filters": "^4.1.1", "eth-json-rpc-infura": "^4.0.2", "eth-json-rpc-middleware": "^4.4.1", - "eth-keyring-controller": "^5.5.0", + "eth-keyring-controller": "^5.6.1", "eth-method-registry": "^1.2.0", "eth-phishing-detect": "^1.1.4", "eth-query": "^2.1.2", diff --git a/test/unit/app/controllers/permissions/mocks.js b/test/unit/app/controllers/permissions/mocks.js index 448de3fc9..9b6e2d39b 100644 --- a/test/unit/app/controllers/permissions/mocks.js +++ b/test/unit/app/controllers/permissions/mocks.js @@ -58,6 +58,8 @@ const getRestrictedMethods = (permController) => { } } +const getUnlockPromise = () => Promise.resolve() + /** * Gets default mock constructor options for a permissions controller. * @@ -67,9 +69,10 @@ export function getPermControllerOpts () { return { platform, getKeyringAccounts, + getUnlockPromise, + getRestrictedMethods, notifyDomain: noop, notifyAllDomains: noop, - getRestrictedMethods, } } @@ -398,6 +401,14 @@ export const getters = deepFreeze({ } }, }, + + eth_requestAccounts: { + requestAlreadyPending: () => { + return { + message: 'Already processing eth_requestAccounts. 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 b2327b5fb..9a516631a 100644 --- a/test/unit/app/controllers/permissions/permissions-controller-test.js +++ b/test/unit/app/controllers/permissions/permissions-controller-test.js @@ -105,6 +105,49 @@ describe('permissions controller', function () { }) }) + describe('hasPermission', function () { + + it('returns correct values', async function () { + + const permController = initPermController() + grantPermissions( + permController, ORIGINS.a, + PERMS.finalizedRequests.eth_accounts(ACCOUNT_ARRAYS.a) + ) + grantPermissions( + permController, ORIGINS.b, + PERMS.finalizedRequests.test_method() + ) + + assert.ok( + permController.hasPermission(ORIGINS.a, 'eth_accounts'), + 'should return true for granted permission' + ) + assert.ok( + permController.hasPermission(ORIGINS.b, 'test_method'), + 'should return true for granted permission' + ) + + assert.ok( + !permController.hasPermission(ORIGINS.a, 'test_method'), + 'should return false for non-granted permission' + ) + assert.ok( + !permController.hasPermission(ORIGINS.b, 'eth_accounts'), + 'should return true for non-granted permission' + ) + + assert.ok( + !permController.hasPermission('foo', 'eth_accounts'), + 'should return false for unknown origin' + ) + assert.ok( + !permController.hasPermission(ORIGINS.b, 'foo'), + 'should return false for unknown permission' + ) + }) + }) + describe('clearPermissions', function () { it('notifies all appropriate domains and removes permissions', async function () { diff --git a/test/unit/app/controllers/permissions/permissions-middleware-test.js b/test/unit/app/controllers/permissions/permissions-middleware-test.js index b27669072..2cabda60b 100644 --- a/test/unit/app/controllers/permissions/permissions-middleware-test.js +++ b/test/unit/app/controllers/permissions/permissions-middleware-test.js @@ -640,6 +640,52 @@ describe('permissions middleware', function () { 'response should have correct result' ) }) + + it('rejects new requests when request already pending', async function () { + + let unlock + const unlockPromise = new Promise((resolve) => { + unlock = resolve + }) + + permController.getUnlockPromise = () => unlockPromise + + const cMiddleware = getPermissionsMiddleware(permController, ORIGINS.c) + + grantPermissions( + permController, ORIGINS.c, + PERMS.finalizedRequests.eth_accounts(ACCOUNT_ARRAYS.c) + ) + + const req = RPC_REQUESTS.eth_requestAccounts(ORIGINS.c) + const res = {} + + // this will block until we resolve the unlock Promise + const requestApproval = assert.doesNotReject( + cMiddleware(req, res), + 'should not reject' + ) + + // this will reject because of the already pending request + await assert.rejects( + cMiddleware({ ...req }, {}), + ERRORS.eth_requestAccounts.requestAlreadyPending() + ) + + // now unlock and let through the first request + unlock() + + await requestApproval + + assert.ok( + res.result && !res.error, + 'response should have result and no error' + ) + assert.deepEqual( + res.result, ACCOUNT_ARRAYS.c, + 'response should have correct result' + ) + }) }) describe('wallet_sendDomainMetadata', function () { diff --git a/yarn.lock b/yarn.lock index 72ced360f..685ff389e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -10386,10 +10386,10 @@ eth-json-rpc-middleware@^4.1.4, eth-json-rpc-middleware@^4.1.5, eth-json-rpc-mid pify "^3.0.0" safe-event-emitter "^1.0.1" -eth-keyring-controller@^5.3.0, eth-keyring-controller@^5.5.0: - version "5.5.0" - resolved "https://registry.yarnpkg.com/eth-keyring-controller/-/eth-keyring-controller-5.5.0.tgz#f8b78f69a0b0005873af2d1a6b2c655d6de51351" - integrity sha512-kWaukiHLMYNYtB/1vZyj1r1G6wU8u+DIYVMq8QUyFAxwcBnemsKISVPIXgltgXkuUiB/t9oXsA54bWBredgrVg== +eth-keyring-controller@^5.3.0, eth-keyring-controller@^5.6.1: + version "5.6.1" + resolved "https://registry.yarnpkg.com/eth-keyring-controller/-/eth-keyring-controller-5.6.1.tgz#7b7268400704c8f5ce98a055910341177dd207ca" + integrity sha512-sxJ87bJg7PvvPzj1sY1jJYHQL1HVUhh84Q/a4QPrcnzAAng1yibvvUfww0pCez4XJfHuMkJvUxfF8eAusJM8fQ== dependencies: bip39 "^2.4.0" bluebird "^3.5.0" @@ -19161,7 +19161,7 @@ minimist-options@^3.0.1: arrify "^1.0.1" is-plain-obj "^1.1.0" -minimist@~0.0.1, minimist@0.0.8, minimist@1.1.x, minimist@1.2.0, minimist@~1.2.0, minimist@^1.1.0, minimist@^1.1.1, minimist@^1.1.3, minimist@^1.2.0, minimist@^1.2.5: +minimist@0.0.8, minimist@1.1.x, minimist@1.2.0, minimist@^1.1.0, minimist@^1.1.1, minimist@^1.1.3, minimist@^1.2.0, minimist@^1.2.5, minimist@~0.0.1, minimist@~1.2.0: version "1.2.5" resolved "https://registry.yarnpkg.com/minimist/-/minimist-1.2.5.tgz#67d66014b66a6a8aaa0c083c5fd58df4e4e97602" integrity sha512-FM9nNUYrRBAELZQT3xeZQ7fmMOBg6nWNmJKTcgsJeaLstP/UODVpGsr5OhXhhXg6f+qtJ8uiZ+PUxkDWcgIXLw==