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