diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index e1e23131d..b33349063 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -47,7 +47,10 @@ export class PermissionsController { this.notifyAllDomains = notifyAllDomains this._showPermissionRequest = showPermissionRequest - this._restrictedMethods = getRestrictedMethods(this) + this._restrictedMethods = getRestrictedMethods({ + getKeyringAccounts: this.getKeyringAccounts.bind(this), + getIdentities: this._getIdentities.bind(this), + }) this.permissionsLog = new PermissionsLogController({ restrictedMethods: Object.keys(this._restrictedMethods), store: this.store, @@ -56,6 +59,7 @@ export class PermissionsController { this.pendingApprovalOrigins = new Set() this._initializePermissions(restoredPermissions) this._lastSelectedAddress = preferences.getState().selectedAddress + this.preferences = preferences preferences.subscribe(async ({ selectedAddress }) => { if (selectedAddress && selectedAddress !== this._lastSelectedAddress) { @@ -139,6 +143,15 @@ export class PermissionsController { return Boolean(this.permissions.getPermission(origin, permission)) } + /** + * Gets the identities from the preferences controller store + * + * @returns {Object} identities + */ + _getIdentities () { + return this.preferences.getState().identities + } + /** * Submits a permissions request to rpc-cap. Internal, background use only. * @@ -293,31 +306,6 @@ export class PermissionsController { } } - /** - * Update the accounts exposed to the given origin. Changes the eth_accounts - * permissions and emits accountsChanged. - * At least one account must be exposed. If no accounts are to be exposed, the - * eth_accounts permissions should be removed completely. - * - * Throws error if the update fails. - * - * @param {string} origin - The origin to change the exposed accounts for. - * @param {string[]} accounts - The new account(s) to expose. - */ - async updatePermittedAccounts (origin, accounts) { - - await this.validatePermittedAccounts(accounts) - - this.permissions.updateCaveatFor( - origin, 'eth_accounts', CAVEAT_NAMES.exposedAccounts, accounts - ) - - this.notifyDomain(origin, { - method: NOTIFICATION_NAMES.accountsChanged, - result: accounts, - }) - } - /** * Finalizes a permissions request. Throws if request validation fails. * Clones the passed-in parameters to prevent inadvertent modification. @@ -434,7 +422,7 @@ export class PermissionsController { /** * When a new account is selected in the UI, emit accountsChanged to each origin - * where the account order has changed. + * where the selected account is exposed. * * Note: This will emit "false positive" accountsChanged events, but they are * handled by the inpage provider. @@ -461,46 +449,26 @@ export class PermissionsController { await Promise.all( connectedDomains .map( - (origin) => this._handleConnectedAccountSelected(origin, account) + (origin) => this._handleConnectedAccountSelected(origin) ) ) } /** - * When a new account is selected in the UI for 'origin', emit accountsChanged - * to 'origin' if the selected account is permitted. + * When a new account is selected in the UI, emit accountsChanged to 'origin' * * Note: This will emit "false positive" accountsChanged events, but they are * handled by the inpage provider. * - * @param {string} origin - The origin. - * @param {string} account - The newly selected account's address. + * @param {string} origin - The origin */ - async _handleConnectedAccountSelected (origin, account) { + async _handleConnectedAccountSelected (origin) { const permittedAccounts = await this.getAccounts(origin) - if ( - typeof origin !== 'string' || !origin.length - ) { - throw new Error('Origin should be a non-empty string.') - } - - // do nothing if the account is not permitted for the origin, or - // if it's already first in the array of permitted accounts - if ( - !permittedAccounts.includes(account) || - permittedAccounts[0] === account - ) { - return - } - - const newPermittedAccounts = [account].concat( - permittedAccounts.filter((_account) => _account !== account) - ) - - // update permitted accounts to ensure that accounts are returned - // in the same order every time - await this.updatePermittedAccounts(origin, newPermittedAccounts) + this.notifyDomain(origin, { + method: NOTIFICATION_NAMES.accountsChanged, + result: permittedAccounts, + }) } /** diff --git a/app/scripts/controllers/permissions/restrictedMethods.js b/app/scripts/controllers/permissions/restrictedMethods.js index 73ac7d94e..717361d96 100644 --- a/app/scripts/controllers/permissions/restrictedMethods.js +++ b/app/scripts/controllers/permissions/restrictedMethods.js @@ -1,12 +1,28 @@ -export default function getRestrictedMethods (permissionsController) { +export default function getRestrictedMethods ({ getIdentities, getKeyringAccounts }) { return { 'eth_accounts': { description: `View the addresses of the user's chosen accounts.`, method: (_, res, __, end) => { - permissionsController.getKeyringAccounts() + getKeyringAccounts() .then((accounts) => { + const identities = getIdentities() res.result = accounts + .sort((firstAddress, secondAddress) => { + if (!identities[firstAddress]) { + throw new Error(`Missing identity for address ${firstAddress}`) + } else if (!identities[secondAddress]) { + throw new Error(`Missing identity for address ${secondAddress}`) + } else if (identities[firstAddress].lastSelected === identities[secondAddress].lastSelected) { + return 0 + } else if (identities[firstAddress].lastSelected === undefined) { + return 1 + } else if (identities[secondAddress].lastSelected === undefined) { + return -1 + } + + return identities[secondAddress].lastSelected - identities[firstAddress].lastSelected + }) end() }) .catch( diff --git a/app/scripts/controllers/preferences.js b/app/scripts/controllers/preferences.js index a8fc7df6d..facb65ebf 100644 --- a/app/scripts/controllers/preferences.js +++ b/app/scripts/controllers/preferences.js @@ -370,8 +370,15 @@ class PreferencesController { setSelectedAddress (_address) { const address = normalizeAddress(_address) this._updateTokens(address) - this.store.updateState({ selectedAddress: address }) - const tokens = this.store.getState().tokens + + const { identities, tokens } = this.store.getState() + const selectedIdentity = identities[address] + if (!selectedIdentity) { + throw new Error(`Identity for '${address} not found`) + } + + selectedIdentity.lastSelected = Date.now() + this.store.updateState({ identities, selectedAddress: address }) return Promise.resolve(tokens) } diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 3e8777942..5f01893d2 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -576,7 +576,6 @@ export default class MetamaskController extends EventEmitter { getApprovedAccounts: nodeify(permissionsController.getAccounts.bind(permissionsController)), rejectPermissionsRequest: nodeify(permissionsController.rejectPermissionsRequest, permissionsController), removePermissionsFor: permissionsController.removePermissionsFor.bind(permissionsController), - updatePermittedAccounts: nodeify(permissionsController.updatePermittedAccounts, permissionsController), legacyExposeAccounts: nodeify(permissionsController.legacyExposeAccounts, permissionsController), getRequestAccountTabIds: (cb) => cb(null, this.getRequestAccountTabIds()), diff --git a/test/unit/app/controllers/metamask-controller-test.js b/test/unit/app/controllers/metamask-controller-test.js index 6c82fccf3..f3b92872c 100644 --- a/test/unit/app/controllers/metamask-controller-test.js +++ b/test/unit/app/controllers/metamask-controller-test.js @@ -243,18 +243,45 @@ describe('MetaMaskController', function () { return Promise.resolve('0x0') }) + let startTime = Date.now() await metamaskController.createNewVaultAndRestore('foobar1337', TEST_SEED) - assert.deepEqual(metamaskController.getState().identities, { + let endTime = Date.now() + + const firstVaultIdentities = cloneDeep(metamaskController.getState().identities) + assert.ok( + ( + firstVaultIdentities[TEST_ADDRESS].lastSelected >= startTime && + firstVaultIdentities[TEST_ADDRESS].lastSelected <= endTime + ), + `'${firstVaultIdentities[TEST_ADDRESS].lastSelected}' expected to be between '${startTime}' and '${endTime}'` + ) + delete firstVaultIdentities[TEST_ADDRESS].lastSelected + assert.deepEqual(firstVaultIdentities, { [TEST_ADDRESS]: { address: TEST_ADDRESS, name: DEFAULT_LABEL }, }) await metamaskController.preferencesController.setAccountLabel(TEST_ADDRESS, 'Account Foo') - assert.deepEqual(metamaskController.getState().identities, { + + const labelledFirstVaultIdentities = cloneDeep(metamaskController.getState().identities) + delete labelledFirstVaultIdentities[TEST_ADDRESS].lastSelected + assert.deepEqual(labelledFirstVaultIdentities, { [TEST_ADDRESS]: { address: TEST_ADDRESS, name: 'Account Foo' }, }) + startTime = Date.now() await metamaskController.createNewVaultAndRestore('foobar1337', TEST_SEED_ALT) - assert.deepEqual(metamaskController.getState().identities, { + endTime = Date.now() + + const secondVaultIdentities = cloneDeep(metamaskController.getState().identities) + assert.ok( + ( + secondVaultIdentities[TEST_ADDRESS_ALT].lastSelected >= startTime && + secondVaultIdentities[TEST_ADDRESS_ALT].lastSelected <= endTime + ), + `'${secondVaultIdentities[TEST_ADDRESS_ALT].lastSelected}' expected to be between '${startTime}' and '${endTime}'` + ) + delete secondVaultIdentities[TEST_ADDRESS_ALT].lastSelected + assert.deepEqual(secondVaultIdentities, { [TEST_ADDRESS_ALT]: { address: TEST_ADDRESS_ALT, name: DEFAULT_LABEL }, }) }) @@ -271,8 +298,13 @@ describe('MetaMaskController', function () { return Promise.resolve('0x14ced5122ce0a000') }) + const startTime = Date.now() await metamaskController.createNewVaultAndRestore('foobar1337', TEST_SEED) - assert.deepEqual(metamaskController.getState().identities, { + + const identities = cloneDeep(metamaskController.getState().identities) + assert.ok(identities[TEST_ADDRESS].lastSelected >= startTime && identities[TEST_ADDRESS].lastSelected <= Date.now()) + delete identities[TEST_ADDRESS].lastSelected + assert.deepEqual(identities, { [TEST_ADDRESS]: { address: TEST_ADDRESS, name: DEFAULT_LABEL }, [TEST_ADDRESS_2]: { address: TEST_ADDRESS_2, name: DEFAULT_LABEL_2 }, }) diff --git a/test/unit/app/controllers/permissions/mocks.js b/test/unit/app/controllers/permissions/mocks.js index 8bdce55c6..7e44dc62b 100644 --- a/test/unit/app/controllers/permissions/mocks.js +++ b/test/unit/app/controllers/permissions/mocks.js @@ -34,6 +34,16 @@ const keyringAccounts = deepFreeze([ const getKeyringAccounts = async () => [ ...keyringAccounts ] +const getIdentities = () => { + return keyringAccounts.reduce( + (identities, address, index) => { + identities[address] = { address, name: `Account ${index}` } + return identities + }, + {} + ) +} + // perm controller initialization helper const getRestrictedMethods = (permController) => { return { @@ -73,7 +83,10 @@ export function getPermControllerOpts () { notifyAllDomains: noop, preferences: { getState: () => { - return { selectedAddress: keyringAccounts[0] } + return { + identities: getIdentities(), + selectedAddress: keyringAccounts[0], + } }, subscribe: noop, }, @@ -317,14 +330,6 @@ export const getters = deepFreeze({ }, }, - updatePermittedAccounts: { - invalidOrigin: () => { - return { - message: 'No such permission exists for the given domain.', - } - }, - }, - legacyExposeAccounts: { badOrigin: () => { return { @@ -584,6 +589,7 @@ export const getters = deepFreeze({ * Objects with immutable mock values. */ export const constants = deepFreeze({ + ALL_ACCOUNTS: keyringAccounts, DUMMY_ACCOUNT: '0xabc', diff --git a/test/unit/app/controllers/permissions/permissions-controller-test.js b/test/unit/app/controllers/permissions/permissions-controller-test.js index db5fb5fae..ac3b54865 100644 --- a/test/unit/app/controllers/permissions/permissions-controller-test.js +++ b/test/unit/app/controllers/permissions/permissions-controller-test.js @@ -33,6 +33,7 @@ const { } = getters const { + ALL_ACCOUNTS, ACCOUNT_ARRAYS, DUMMY_ACCOUNT, ORIGINS, @@ -460,115 +461,6 @@ describe('permissions controller', function () { }) }) - describe('updatePermittedAccounts', function () { - - let permController, notifications - - beforeEach(function () { - notifications = initNotifications() - permController = initPermController(notifications) - grantPermissions( - permController, ORIGINS.a, - PERMS.finalizedRequests.eth_accounts(ACCOUNT_ARRAYS.a) - ) - grantPermissions( - permController, ORIGINS.b, - PERMS.finalizedRequests.eth_accounts(ACCOUNT_ARRAYS.b) - ) - }) - - it('throws on invalid accounts', async function () { - - await assert.rejects( - permController.updatePermittedAccounts(ORIGINS.a, {}), - ERRORS.validatePermittedAccounts.invalidParam(), - 'should throw on non-array accounts param' - ) - - await assert.rejects( - permController.updatePermittedAccounts(ORIGINS.a, []), - ERRORS.validatePermittedAccounts.invalidParam(), - 'should throw on empty array accounts param' - ) - - await assert.rejects( - permController.updatePermittedAccounts(ORIGINS.a, [DUMMY_ACCOUNT]), - ERRORS.validatePermittedAccounts.nonKeyringAccount(DUMMY_ACCOUNT), - 'should throw on non-keyring account' - ) - }) - - it('throws if origin invalid or lacks eth_accounts permission', async function () { - - await assert.rejects( - permController.updatePermittedAccounts(false, ACCOUNT_ARRAYS.a), - ERRORS.updatePermittedAccounts.invalidOrigin(), - 'should throw on invalid origin' - ) - - await assert.rejects( - permController.updatePermittedAccounts(ORIGINS.c, ACCOUNT_ARRAYS.a), - ERRORS.updatePermittedAccounts.invalidOrigin(), - 'should throw on origin without eth_accounts permission' - ) - }) - - it('successfully updates permitted accounts', async function () { - - await permController.updatePermittedAccounts(ORIGINS.a, ACCOUNT_ARRAYS.b) - await permController.updatePermittedAccounts(ORIGINS.b, ACCOUNT_ARRAYS.c) - - let aAccounts = await permController.getAccounts(ORIGINS.a) - let bAccounts = await permController.getAccounts(ORIGINS.b) - - assert.deepEqual( - aAccounts, ACCOUNT_ARRAYS.b, - 'first origin should have correct accounts' - ) - assert.deepEqual( - bAccounts, ACCOUNT_ARRAYS.c, - 'first origin should have correct accounts' - ) - - assert.deepEqual( - notifications[ORIGINS.a][0], - NOTIFICATIONS.newAccounts(ACCOUNT_ARRAYS.b), - 'first origin should have correct notification' - ) - assert.deepEqual( - notifications[ORIGINS.b][0], - NOTIFICATIONS.newAccounts(ACCOUNT_ARRAYS.c), - 'second origin should have correct notification' - ) - - await permController.updatePermittedAccounts(ORIGINS.a, ACCOUNT_ARRAYS.c) - await permController.updatePermittedAccounts(ORIGINS.b, ACCOUNT_ARRAYS.a) - - aAccounts = await permController.getAccounts(ORIGINS.a) - bAccounts = await permController.getAccounts(ORIGINS.b) - - assert.deepEqual( - aAccounts, ACCOUNT_ARRAYS.c, - 'first origin should have correct accounts' - ) - assert.deepEqual( - bAccounts, ACCOUNT_ARRAYS.a, - 'first origin should have correct accounts' - ) - - assert.deepEqual( - notifications[ORIGINS.a][1], - NOTIFICATIONS.newAccounts(ACCOUNT_ARRAYS.c), - 'first origin should have correct notification' - ) - assert.deepEqual( - notifications[ORIGINS.b][1], - NOTIFICATIONS.newAccounts(ACCOUNT_ARRAYS.a), - 'second origin should have correct notification' - ) - }) - }) - describe('finalizePermissionsRequest', function () { let permController @@ -740,15 +632,24 @@ describe('permissions controller', function () { describe('preferences state update', function () { - let permController, notifications, preferences + let permController, notifications, preferences, identities beforeEach(function () { - preferences = { - getState: () => { - return { selectedAddress: DUMMY_ACCOUNT } + identities = ALL_ACCOUNTS.reduce( + (identities, account) => { + identities[account] = {} + return identities }, + {} + ) + preferences = { + getState: sinon.stub(), subscribe: sinon.stub(), } + preferences.getState.returns({ + identities, + selectedAddress: DUMMY_ACCOUNT, + }) notifications = initNotifications() permController = new PermissionsController({ ...getPermControllerOpts(), @@ -767,6 +668,7 @@ describe('permissions controller', 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] @@ -795,7 +697,8 @@ describe('permissions controller', function () { ) }) - it('should do nothing if account already first in array for each connected site', async function () { + it('should emit notification if account already first in array for each connected site', async function () { + identities[ACCOUNT_ARRAYS.a[0]] = { lastSelected: 1000 } assert(preferences.subscribe.calledOnce) assert(preferences.subscribe.firstCall.args.length === 1) const onPreferencesUpdate = preferences.subscribe.firstCall.args[0] @@ -803,16 +706,19 @@ describe('permissions controller', function () { await onPreferencesUpdate({ selectedAddress: ACCOUNT_ARRAYS.a[0] }) assert.deepEqual( - notifications[ORIGINS.b], [], + notifications[ORIGINS.b], + [NOTIFICATIONS.newAccounts([...ACCOUNT_ARRAYS.a, EXTRA_ACCOUNT])], 'should not have emitted notification' ) assert.deepEqual( - notifications[ORIGINS.c], [], + notifications[ORIGINS.c], + [NOTIFICATIONS.newAccounts(ACCOUNT_ARRAYS.a)], 'should not have emitted notification' ) }) it('should emit notification just for connected domains', async function () { + identities[EXTRA_ACCOUNT] = { lastSelected: 1000 } assert(preferences.subscribe.calledOnce) assert(preferences.subscribe.firstCall.args.length === 1) const onPreferencesUpdate = preferences.subscribe.firstCall.args[0] @@ -831,6 +737,7 @@ describe('permissions controller', function () { }) it('should emit notification for multiple connected domains', async function () { + identities[ACCOUNT_ARRAYS.a[1]] = { lastSelected: 1000 } assert(preferences.subscribe.calledOnce) assert(preferences.subscribe.firstCall.args.length === 1) const onPreferencesUpdate = preferences.subscribe.firstCall.args[0] diff --git a/test/unit/app/controllers/permissions/restricted-methods-test.js b/test/unit/app/controllers/permissions/restricted-methods-test.js index 26f477bed..6b95cdb69 100644 --- a/test/unit/app/controllers/permissions/restricted-methods-test.js +++ b/test/unit/app/controllers/permissions/restricted-methods-test.js @@ -1,35 +1,147 @@ import { strict as assert } from 'assert' +import pify from 'pify' import getRestrictedMethods from '../../../../../app/scripts/controllers/permissions/restrictedMethods' describe('restricted methods', function () { - - // this method is tested extensively in other permissions tests describe('eth_accounts', function () { - - it('handles failure', async function () { + it('should handle getKeyringAccounts error', async function () { const restrictedMethods = getRestrictedMethods({ getKeyringAccounts: async () => { throw new Error('foo') }, }) + const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method) const res = {} - restrictedMethods.eth_accounts.method(null, res, null, (err) => { + const fooError = new Error('foo') + await assert.rejects( + ethAccountsMethod(null, res, null), + fooError, + 'Should reject with expected error' + ) - const fooError = new Error('foo') + assert.deepEqual( + res, { error: fooError }, + 'response should have expected error and no result' + ) + }) - assert.deepEqual( - err, fooError, - 'should end with expected error' - ) + it('should handle missing identity for first account when sorting', async function () { + const restrictedMethods = getRestrictedMethods({ + getIdentities: () => { + return { '0x7e57e2': {} } + }, + getKeyringAccounts: async () => ['0x7e57e2', '0x7e57e3'], + }) + const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method) - assert.deepEqual( - res, { error: fooError }, - 'response should have expected error and no result' - ) + const res = {} + await assert.rejects(ethAccountsMethod(null, res, null)) + assert.ok(res.error instanceof Error, 'result should have error') + assert.deepEqual(Object.keys(res), ['error'], 'result should only contain error') + }) + + it('should handle missing identity for second account when sorting', async function () { + const restrictedMethods = getRestrictedMethods({ + getIdentities: () => { + return { '0x7e57e3': {} } + }, + getKeyringAccounts: async () => ['0x7e57e2', '0x7e57e3'], }) + const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method) + + const res = {} + await assert.rejects(ethAccountsMethod(null, res, null)) + assert.ok(res.error instanceof Error, 'result should have error') + assert.deepEqual(Object.keys(res), ['error'], 'result should only contain error') + }) + + it('should return accounts in keyring order when none are selected', async function () { + const keyringAccounts = ['0x7e57e2', '0x7e57e3', '0x7e57e4', '0x7e57e5'] + const restrictedMethods = getRestrictedMethods({ + getIdentities: () => { + return keyringAccounts.reduce( + (identities, address) => { + identities[address] = {} + return identities + }, + {} + ) + }, + getKeyringAccounts: async () => [...keyringAccounts], + }) + const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method) + + const res = {} + await ethAccountsMethod(null, res, null) + assert.deepEqual(res, { result: keyringAccounts }, 'should return accounts in correct order') + }) + + it('should return accounts in keyring order when all have same last selected time', async function () { + const keyringAccounts = ['0x7e57e2', '0x7e57e3', '0x7e57e4', '0x7e57e5'] + const restrictedMethods = getRestrictedMethods({ + getIdentities: () => { + return keyringAccounts.reduce( + (identities, address) => { + identities[address] = { lastSelected: 1000 } + return identities + }, + {} + ) + }, + getKeyringAccounts: async () => [...keyringAccounts], + }) + const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method) + + const res = {} + await ethAccountsMethod(null, res, null) + assert.deepEqual(res, { result: keyringAccounts }, 'should return accounts in correct order') + }) + + it('should return accounts sorted by last selected (descending)', async function () { + const keyringAccounts = ['0x7e57e2', '0x7e57e3', '0x7e57e4', '0x7e57e5'] + const expectedResult = keyringAccounts.slice().reverse() + const restrictedMethods = getRestrictedMethods({ + getIdentities: () => { + return keyringAccounts.reduce( + (identities, address, index) => { + identities[address] = { lastSelected: index * 1000 } + return identities + }, + {} + ) + }, + getKeyringAccounts: async () => [...keyringAccounts], + }) + const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method) + + const res = {} + await ethAccountsMethod(null, res, null) + assert.deepEqual(res, { result: expectedResult }, 'should return accounts in correct order') + }) + + it('should return accounts sorted by last selected (descending) with unselected accounts last, in keyring order', async function () { + const keyringAccounts = ['0x7e57e2', '0x7e57e3', '0x7e57e4', '0x7e57e5', '0x7e57e6'] + const expectedResult = ['0x7e57e4', '0x7e57e2', '0x7e57e3', '0x7e57e5', '0x7e57e6'] + const restrictedMethods = getRestrictedMethods({ + getIdentities: () => { + return { + '0x7e57e2': { lastSelected: 1000 }, + '0x7e57e3': {}, + '0x7e57e4': { lastSelected: 2000 }, + '0x7e57e5': {}, + '0x7e57e6': {}, + } + }, + getKeyringAccounts: async () => [...keyringAccounts], + }) + const ethAccountsMethod = pify(restrictedMethods.eth_accounts.method) + + const res = {} + await ethAccountsMethod(null, res, null) + assert.deepEqual(res, { result: expectedResult }, 'should return accounts in correct order') }) }) })