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

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

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<void>} 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,

app/scripts/controllers/permissions/index.js

@@ -24,8 +24,12 @@ export class PermissionsController {
 
   constructor (
     {
-      platform, notifyDomain, notifyAllDomains,
+      getKeyringAccounts,
-      getKeyringAccounts, getRestrictedMethods,
+      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.
    *

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

app/scripts/metamask-controller.js

@@ -121,9 +121,11 @@ export default class MetamaskController extends EventEmitter {
     })
 
     this.appStateController = new AppStateController({
-      preferencesStore: this.preferencesController.store,
+      addUnlockListener: this.on.bind(this, 'unlock'),
-      onInactiveTimeout: () => this.setLocked(),
+      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 (
+        } else if (this.isUnlocked()) {
-          this.keyringController.memStore.getState().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 (!this.isUnlocked()) {
-    if (!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
   }
 

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",

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.',
+        }
+      },
+    },
   },
 
   /**

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 () {

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 () {

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:
+eth-keyring-controller@^5.3.0, eth-keyring-controller@^5.6.1:
-  version "5.5.0"
+  version "5.6.1"
-  resolved "https://registry.yarnpkg.com/eth-keyring-controller/-/eth-keyring-controller-5.5.0.tgz#f8b78f69a0b0005873af2d1a6b2c655d6de51351"
+  resolved "https://registry.yarnpkg.com/eth-keyring-controller/-/eth-keyring-controller-5.6.1.tgz#7b7268400704c8f5ce98a055910341177dd207ca"
-  integrity sha512-kWaukiHLMYNYtB/1vZyj1r1G6wU8u+DIYVMq8QUyFAxwcBnemsKISVPIXgltgXkuUiB/t9oXsA54bWBredgrVg==
+  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==