From b2882aa778fed29bbaa5339ab9b227c70a30458a Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Thu, 16 Apr 2020 13:16:53 -0300 Subject: [PATCH] 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. --- app/scripts/controllers/permissions/index.js | 51 ++++++- app/scripts/metamask-controller.js | 13 +- .../unit/app/controllers/permissions/mocks.js | 18 ++- .../permissions-controller-test.js | 132 +++++++++++------- ui/app/store/actions.js | 2 - 5 files changed, 145 insertions(+), 71 deletions(-) diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index bc8d6811a..e1e23131d 100644 --- 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 diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 0098e9454..3e8777942 100644 --- 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) diff --git a/test/unit/app/controllers/permissions/mocks.js b/test/unit/app/controllers/permissions/mocks.js index fdb70ea77..8bdce55c6 100644 --- 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, }, }, }, diff --git a/test/unit/app/controllers/permissions/permissions-controller-test.js b/test/unit/app/controllers/permissions/permissions-controller-test.js index 9a516631a..db5fb5fae 100644 --- 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) - grantPermissions( - permController, ORIGINS.a, - PERMS.finalizedRequests.eth_accounts(ACCOUNT_ARRAYS.a) - ) + permController = new PermissionsController({ + ...getPermControllerOpts(), + notifyDomain: getNotifyDomain(notifications), + notifyAllDomains: getNotifyAllDomains(notifications), + preferences, + }) grantPermissions( permController, ORIGINS.b, - PERMS.finalizedRequests.eth_accounts(ACCOUNT_ARRAYS.b) + PERMS.finalizedRequests.eth_accounts([...ACCOUNT_ARRAYS.a, EXTRA_ACCOUNT]) + ) + grantPermissions( + 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' - ) - - await assert.rejects( - permController.handleNewAccountSelected('', DUMMY_ACCOUNT), - ERRORS.handleNewAccountSelected.invalidParams(), - 'should throw if origin empty string' - ) - - await assert.rejects( - permController.handleNewAccountSelected(ORIGINS.a, {}), - ERRORS.handleNewAccountSelected.invalidParams(), - 'should throw if account non-string' - ) - - await assert.rejects( - permController.handleNewAccountSelected(ORIGINS.a, ''), - ERRORS.handleNewAccountSelected.invalidParams(), - 'should throw if account empty string' + () => onPreferencesUpdate({ selectedAddress: {} }), + ERRORS._handleAccountSelected.invalidParams(), + 'should throw if account is not a string' ) }) - it('does nothing if account not permitted for origin', async function () { + 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 permController.handleNewAccountSelected( - ORIGINS.b, ACCOUNT_ARRAYS.c[0] - ) + await onPreferencesUpdate({ selectedAddress: DUMMY_ACCOUNT }) assert.deepEqual( notifications[ORIGINS.b], [], 'should not have emitted notification' ) - }) - - it('does nothing if account already first in array', async function () { - - await permController.handleNewAccountSelected( - ORIGINS.a, ACCOUNT_ARRAYS.a[0] - ) - assert.deepEqual( - notifications[ORIGINS.a], [], + notifications[ORIGINS.c], [], 'should not have emitted notification' ) }) - it('emits notification if selected account not first in array', 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.a, ACCOUNT_ARRAYS.a[1] - ) + await onPreferencesUpdate({ selectedAddress: ACCOUNT_ARRAYS.a[0] }) assert.deepEqual( - notifications[ORIGINS.a], - [NOTIFICATIONS.newAccounts([ ...ACCOUNT_ARRAYS.a ].reverse())], + notifications[ORIGINS.b], [], + 'should not have emitted notification' + ) + assert.deepEqual( + notifications[ORIGINS.c], [], + 'should not have emitted notification' + ) + }) + + 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 onPreferencesUpdate({ selectedAddress: EXTRA_ACCOUNT }) + + assert.deepEqual( + 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('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 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.b], + [NOTIFICATIONS.newAccounts([ ...expectedAccounts, EXTRA_ACCOUNT ])], + 'should have emitted notification' + ) + assert.deepEqual( + notifications[ORIGINS.c], + [NOTIFICATIONS.newAccounts(expectedAccounts)], 'should have emitted notification' ) }) diff --git a/ui/app/store/actions.js b/ui/app/store/actions.js index 5ae4d1518..26806d7f2 100644 --- 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,