Handle account selection on all domains that can view the selection (#8341)

Selecting a new account now results in all domains that can view this change being notified. Previously only the dapp in the active tab was being notified (though not correctly, as the `origin` was accidentally set to the MetaMask chrome extension origin). This handling of account selection has been moved into the background to minimize the gap between account selection and the notification being sent out. It's simpler for the UI to not be involved anyway.
5 years ago · b2882aa778
parent c2e95d8672
commit b2882aa778
5 changed files with 136 additions and 62 deletions
--- a/app/scripts/controllers/permissions/index.js
+++ b/app/scripts/controllers/permissions/index.js
@ -29,6 +29,7 @@ export class PermissionsController {
      getUnlockPromise,
      notifyDomain,
      notifyAllDomains,
+      preferences,
      showPermissionRequest,
    } = {},
    restoredPermissions = {},
@ -54,6 +55,14 @@ export class PermissionsController {
    this.pendingApprovals = new Map()
    this.pendingApprovalOrigins = new Set()
    this._initializePermissions(restoredPermissions)
+    this._lastSelectedAddress = preferences.getState().selectedAddress
+
+    preferences.subscribe(async ({ selectedAddress }) => {
+      if (selectedAddress && selectedAddress !== this._lastSelectedAddress) {
+        this._lastSelectedAddress = selectedAddress
+        await this._handleAccountSelected(selectedAddress)
+      }
+    })
  }

  createMiddleware ({ origin, extensionId }) {
@ -423,6 +432,40 @@ export class PermissionsController {
    })
  }

+  /**
+   * When a new account is selected in the UI, emit accountsChanged to each origin
+   * where the account order has changed.
+   *
+   * Note: This will emit "false positive" accountsChanged events, but they are
+   * handled by the inpage provider.
+   *
+   * @param {string} account - The newly selected account's address.
+   */
+  async _handleAccountSelected (account) {
+    if (typeof account !== 'string') {
+      throw new Error('Selected account should be a non-empty string.')
+    }
+
+    const domains = this.permissions.getDomains() || {}
+    const connectedDomains = Object.entries(domains)
+      .filter(([_, { permissions }]) => {
+        const ethAccounts = permissions.find((permission) => permission.parentCapability === 'eth_accounts')
+        const exposedAccounts = ethAccounts
+          ?.caveats
+          .find((caveat) => caveat.name === 'exposedAccounts')
+          ?.value
+        return exposedAccounts?.includes(account)
+      })
+      .map(([domain]) => domain)
+
+    await Promise.all(
+      connectedDomains
+        .map(
+          (origin) => this._handleConnectedAccountSelected(origin, account)
+        )
+    )
+  }
+
  /**
   * When a new account is selected in the UI for 'origin', emit accountsChanged
   * to 'origin' if the selected account is permitted.
@ -433,15 +476,13 @@ export class PermissionsController {
   * @param {string} origin - The origin.
   * @param {string} account - The newly selected account's address.
   */
-  async handleNewAccountSelected (origin, account) {
-
+  async _handleConnectedAccountSelected (origin, account) {
    const permittedAccounts = await this.getAccounts(origin)

    if (
-      typeof origin !== 'string' || !origin.length ||
-      typeof account !== 'string' || !account.length
+      typeof origin !== 'string' || !origin.length
    ) {
-      throw new Error('Should provide non-empty origin and account strings.')
+      throw new Error('Origin should be a non-empty string.')
    }

    // do nothing if the account is not permitted for the origin, or
--- a/app/scripts/metamask-controller.js
+++ b/app/scripts/metamask-controller.js
@ -216,6 +216,7 @@ export default class MetamaskController extends EventEmitter {
      getUnlockPromise: this.appStateController.getUnlockPromise.bind(this.appStateController),
      notifyDomain: this.notifyConnections.bind(this),
      notifyAllDomains: this.notifyAllConnections.bind(this),
+      preferences: this.preferencesController.store,
      showPermissionRequest: opts.showPermissionRequest,
    }, initState.PermissionsController, initState.PermissionsMetadata)

@ -577,7 +578,6 @@ export default class MetamaskController extends EventEmitter {
      removePermissionsFor: permissionsController.removePermissionsFor.bind(permissionsController),
      updatePermittedAccounts: nodeify(permissionsController.updatePermittedAccounts, permissionsController),
      legacyExposeAccounts: nodeify(permissionsController.legacyExposeAccounts, permissionsController),
-      handleNewAccountSelected: nodeify(this.handleNewAccountSelected, this),

      getRequestAccountTabIds: (cb) => cb(null, this.getRequestAccountTabIds()),
      getOpenMetamaskTabsIds: (cb) => cb(null, this.getOpenMetamaskTabsIds()),
@ -1041,17 +1041,6 @@ export default class MetamaskController extends EventEmitter {
    await this.preferencesController.setSelectedAddress(accounts[0])
  }

-  /**
-   * Handle when a new account is selected for the given origin in the UI.
-   * Stores the address by origin and notifies external providers associated
-   * with the origin.
-   * @param {string} origin - The origin for which the address was selected.
-   * @param {string} address - The new selected address.
-   */
-  async handleNewAccountSelected (origin, address) {
-    this.permissionsController.handleNewAccountSelected(origin, address)
-  }
-
  // ---------------------------------------------------------------------------
  // Identity Management (signature operations)

--- a/test/unit/app/controllers/permissions/mocks.js
+++ b/test/unit/app/controllers/permissions/mocks.js
@ -28,6 +28,8 @@ export const noop = () => {}
 const keyringAccounts = deepFreeze([
  '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc',
  '0xc42edfcc21ed14dda456aa0756c153f7985d8813',
+  '0x7ae1cdd37bcbdb0e1f491974da8022bfdbf9c2bf',
+  '0xcc74c7a59194e5d9268476955650d1e285be703c',
 ])

 const getKeyringAccounts = async () => [ ...keyringAccounts ]
@ -69,6 +71,12 @@ export function getPermControllerOpts () {
    getRestrictedMethods,
    notifyDomain: noop,
    notifyAllDomains: noop,
+    preferences: {
+      getState: () => {
+        return { selectedAddress: keyringAccounts[0] }
+      },
+      subscribe: noop,
+    },
  }
 }

@ -142,7 +150,7 @@ const PERM_NAMES = {
 }

 const ACCOUNT_ARRAYS = {
-  a: [ ...keyringAccounts ],
+  a: [ ...keyringAccounts.slice(0, 3) ],
  b: [keyringAccounts[0]],
  c: [keyringAccounts[1]],
 }
@ -331,11 +339,11 @@ export const getters = deepFreeze({
      },
    },

-    handleNewAccountSelected: {
+    _handleAccountSelected: {
      invalidParams: () => {
        return {
          name: 'Error',
-          message: 'Should provide non-empty origin and account strings.',
+          message: 'Selected account should be a non-empty string.',
        }
      },
    },
@ -579,6 +587,8 @@ export const constants = deepFreeze({

  DUMMY_ACCOUNT: '0xabc',

+  EXTRA_ACCOUNT: keyringAccounts[3],
+
  REQUEST_IDS: {
    a: '1',
    b: '2',
@ -609,6 +619,7 @@ export const constants = deepFreeze({
            accounts: {
              [ACCOUNT_ARRAYS.a[0]]: 1,
              [ACCOUNT_ARRAYS.a[1]]: 1,
+              [ACCOUNT_ARRAYS.a[2]]: 1,
            },
          },
        },
@ -620,6 +631,7 @@ export const constants = deepFreeze({
            accounts: {
              [ACCOUNT_ARRAYS.a[0]]: 2,
              [ACCOUNT_ARRAYS.a[1]]: 1,
+              [ACCOUNT_ARRAYS.a[2]]: 1,
            },
          },
        },
--- a/test/unit/app/controllers/permissions/permissions-controller-test.js
+++ b/test/unit/app/controllers/permissions/permissions-controller-test.js
@ -38,6 +38,7 @@ const {
  ORIGINS,
  PERM_NAMES,
  REQUEST_IDS,
+  EXTRA_ACCOUNT,
 } = constants

 const initNotifications = () => {
@ -737,83 +738,116 @@ describe('permissions controller', function () {
    })
  })

-  describe('handleNewAccountSelected', function () {
+  describe('preferences state update', function () {

-    let permController, notifications
+    let permController, notifications, preferences

    beforeEach(function () {
+      preferences = {
+        getState: () => {
+          return { selectedAddress: DUMMY_ACCOUNT }
+        },
+        subscribe: sinon.stub(),
+      }
      notifications = initNotifications()
-      permController = initPermController(notifications)
+      permController = new PermissionsController({
+        ...getPermControllerOpts(),
+        notifyDomain: getNotifyDomain(notifications),
+        notifyAllDomains: getNotifyAllDomains(notifications),
+        preferences,
+      })
      grantPermissions(
-        permController, ORIGINS.a,
-        PERMS.finalizedRequests.eth_accounts(ACCOUNT_ARRAYS.a)
+        permController, ORIGINS.b,
+        PERMS.finalizedRequests.eth_accounts([...ACCOUNT_ARRAYS.a, EXTRA_ACCOUNT])
      )
      grantPermissions(
-        permController, ORIGINS.b,
-        PERMS.finalizedRequests.eth_accounts(ACCOUNT_ARRAYS.b)
+        permController, ORIGINS.c,
+        PERMS.finalizedRequests.eth_accounts(ACCOUNT_ARRAYS.a)
      )
    })

-    it('throws if invalid origin or account', async function () {
+    it('should throw if given invalid account', async function () {
+      assert(preferences.subscribe.calledOnce)
+      assert(preferences.subscribe.firstCall.args.length === 1)
+      const onPreferencesUpdate = preferences.subscribe.firstCall.args[0]

      await assert.rejects(
-        permController.handleNewAccountSelected({}, DUMMY_ACCOUNT),
-        ERRORS.handleNewAccountSelected.invalidParams(),
-        'should throw if origin non-string'
+        () => onPreferencesUpdate({ selectedAddress: {} }),
+        ERRORS._handleAccountSelected.invalidParams(),
+        'should throw if account is not a string'
      )
+    })

-      await assert.rejects(
-        permController.handleNewAccountSelected('', DUMMY_ACCOUNT),
-        ERRORS.handleNewAccountSelected.invalidParams(),
-        'should throw if origin empty string'
-      )
+    it('should do nothing if account not permitted for any origins', async function () {
+      assert(preferences.subscribe.calledOnce)
+      assert(preferences.subscribe.firstCall.args.length === 1)
+      const onPreferencesUpdate = preferences.subscribe.firstCall.args[0]

-      await assert.rejects(
-        permController.handleNewAccountSelected(ORIGINS.a, {}),
-        ERRORS.handleNewAccountSelected.invalidParams(),
-        'should throw if account non-string'
-      )
+      await onPreferencesUpdate({ selectedAddress: DUMMY_ACCOUNT })

-      await assert.rejects(
-        permController.handleNewAccountSelected(ORIGINS.a, ''),
-        ERRORS.handleNewAccountSelected.invalidParams(),
-        'should throw if account empty string'
+      assert.deepEqual(
+        notifications[ORIGINS.b], [],
+        'should not have emitted notification'
+      )
+      assert.deepEqual(
+        notifications[ORIGINS.c], [],
+        'should not have emitted notification'
      )
    })

-    it('does nothing if account not permitted for origin', async function () {
+    it('should do nothing if account already first in array for each connected site', async function () {
+      assert(preferences.subscribe.calledOnce)
+      assert(preferences.subscribe.firstCall.args.length === 1)
+      const onPreferencesUpdate = preferences.subscribe.firstCall.args[0]

-      await permController.handleNewAccountSelected(
-        ORIGINS.b, ACCOUNT_ARRAYS.c[0]
-      )
+      await onPreferencesUpdate({ selectedAddress: ACCOUNT_ARRAYS.a[0] })

      assert.deepEqual(
        notifications[ORIGINS.b], [],
        'should not have emitted notification'
      )
+      assert.deepEqual(
+        notifications[ORIGINS.c], [],
+        'should not have emitted notification'
+      )
    })

-    it('does nothing if account already first in array', async function () {
+    it('should emit notification just for connected domains', async function () {
+      assert(preferences.subscribe.calledOnce)
+      assert(preferences.subscribe.firstCall.args.length === 1)
+      const onPreferencesUpdate = preferences.subscribe.firstCall.args[0]

-      await permController.handleNewAccountSelected(
-        ORIGINS.a, ACCOUNT_ARRAYS.a[0]
-      )
+      await onPreferencesUpdate({ selectedAddress: EXTRA_ACCOUNT })

      assert.deepEqual(
-        notifications[ORIGINS.a], [],
+        notifications[ORIGINS.b],
+        [NOTIFICATIONS.newAccounts([ EXTRA_ACCOUNT, ...ACCOUNT_ARRAYS.a ])],
+        'should have emitted notification'
+      )
+      assert.deepEqual(
+        notifications[ORIGINS.c], [],
        'should not have emitted notification'
      )
    })

-    it('emits notification if selected account not first in array', async function () {
+    it('should emit notification for multiple connected domains', async function () {
+      assert(preferences.subscribe.calledOnce)
+      assert(preferences.subscribe.firstCall.args.length === 1)
+      const onPreferencesUpdate = preferences.subscribe.firstCall.args[0]

-      await permController.handleNewAccountSelected(
-        ORIGINS.a, ACCOUNT_ARRAYS.a[1]
-      )
+      await onPreferencesUpdate({ selectedAddress: ACCOUNT_ARRAYS.a[1] })

+      const accountsWithoutFirst = ACCOUNT_ARRAYS.a
+        .filter((account) => account !== ACCOUNT_ARRAYS.a[1])
+      const expectedAccounts = [ ACCOUNT_ARRAYS.a[1], ...accountsWithoutFirst ]
      assert.deepEqual(
-        notifications[ORIGINS.a],
-        [NOTIFICATIONS.newAccounts([ ...ACCOUNT_ARRAYS.a ].reverse())],
+        notifications[ORIGINS.b],
+        [NOTIFICATIONS.newAccounts([ ...expectedAccounts, EXTRA_ACCOUNT ])],
+        'should have emitted notification'
+      )
+      assert.deepEqual(
+        notifications[ORIGINS.c],
+        [NOTIFICATIONS.newAccounts(expectedAccounts)],
        'should have emitted notification'
      )
    })
--- a/ui/app/store/actions.js
+++ b/ui/app/store/actions.js
@ -1217,8 +1217,6 @@ export function showAccountDetail (address) {
      if (err) {
        return dispatch(displayWarning(err.message))
      }
-      // TODO: Use correct `origin` here
-      background.handleNewAccountSelected(window.origin, address)
      dispatch(updateTokens(tokens))
      dispatch({
        type: actionConstants.SHOW_ACCOUNT_DETAIL,