From c8a995dd9ba38c9ee7f26f24c23d52880c2c47f4 Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Thu, 4 Jun 2020 12:15:52 -0700 Subject: [PATCH] Send accountsChanged notification for wallet_requestPermissions (#8742) * emit accountsChanged for eth_accounts via wallet_requestPermissions * add/update tests --- app/scripts/controllers/permissions/index.js | 70 ++++++++++--------- .../permissions/methodMiddleware.js | 28 +++++++- .../unit/app/controllers/permissions/mocks.js | 25 +++---- .../permissions-controller-test.js | 54 ++++++++++---- .../permissions-middleware-test.js | 41 ++++++++++- 5 files changed, 157 insertions(+), 61 deletions(-) diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index fc51c41a3..bb61831f3 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -47,7 +47,7 @@ export class PermissionsController { this.getKeyringAccounts = getKeyringAccounts this._getUnlockPromise = getUnlockPromise this._notifyDomain = notifyDomain - this.notifyAllDomains = notifyAllDomains + this._notifyAllDomains = notifyAllDomains this._showPermissionRequest = showPermissionRequest this._restrictedMethods = getRestrictedMethods({ @@ -95,6 +95,7 @@ export class PermissionsController { getAccounts: this.getAccounts.bind(this, origin), getUnlockPromise: () => this._getUnlockPromise(true), hasPermission: this.hasPermission.bind(this, origin), + notifyAccountsChanged: this.notifyAccountsChanged.bind(this, origin), requestAccountsPermission: this._requestPermissions.bind( this, { origin }, { eth_accounts: {} }, ), @@ -196,6 +197,7 @@ export class PermissionsController { * User approval callback. Resolves the Promise for the permissions request * waited upon by rpc-cap, see requestUserApproval in _initializePermissions. * The request will be rejected if finalizePermissionsRequest fails. + * Idempotent for a given request id. * * @param {Object} approved - The request object approved by the user * @param {Array} accounts - The accounts to expose, if any @@ -206,7 +208,7 @@ export class PermissionsController { const approval = this.pendingApprovals.get(id) if (!approval) { - log.error(`Permissions request with id '${id}' not found`) + log.debug(`Permissions request with id '${id}' not found`) return } @@ -241,6 +243,7 @@ export class PermissionsController { /** * User rejection callback. Rejects the Promise for the permissions request * waited upon by rpc-cap, see requestUserApproval in _initializePermissions. + * Idempotent for a given id. * * @param {string} id - The id of the request rejected by the user */ @@ -248,7 +251,7 @@ export class PermissionsController { const approval = this.pendingApprovals.get(id) if (!approval) { - log.error(`Permissions request with id '${id}' not found`) + log.debug(`Permissions request with id '${id}' not found`) return } @@ -289,10 +292,7 @@ export class PermissionsController { const permittedAccounts = await this.getAccounts(origin) - this.notifyDomain(origin, { - method: NOTIFICATION_NAMES.accountsChanged, - result: permittedAccounts, - }) + this.notifyAccountsChanged(origin, permittedAccounts) } /** @@ -338,10 +338,7 @@ export class PermissionsController { newPermittedAccounts = await this.getAccounts(origin) } - this.notifyDomain(origin, { - method: NOTIFICATION_NAMES.accountsChanged, - result: newPermittedAccounts, - }) + this.notifyAccountsChanged(origin, newPermittedAccounts) } /** @@ -410,21 +407,34 @@ export class PermissionsController { }) } - notifyDomain (origin, payload) { + /** + * Notify a domain that its permitted accounts have changed. + * Also updates the accounts history log. + * + * @param {string} origin - The origin of the domain to notify. + * @param {Array} newAccounts - The currently permitted accounts. + */ + notifyAccountsChanged (origin, newAccounts) { + + if (typeof origin !== 'string' || !origin) { + throw new Error(`Invalid origin: '${origin}'`) + } + + if (!Array.isArray(newAccounts)) { + throw new Error('Invalid accounts', newAccounts) + } + + this._notifyDomain(origin, { + method: NOTIFICATION_NAMES.accountsChanged, + result: newAccounts, + }) // if the accounts changed from the perspective of the dapp, // update "last seen" time for the origin and account(s) // exception: no accounts -> no times to update - if ( - payload.method === NOTIFICATION_NAMES.accountsChanged && - Array.isArray(payload.result) - ) { - this.permissionsLog.updateAccountsHistory( - origin, payload.result - ) - } - - this._notifyDomain(origin, payload) + this.permissionsLog.updateAccountsHistory( + origin, newAccounts + ) // NOTE: // we don't check for accounts changing in the notifyAllDomains case, @@ -438,7 +448,8 @@ export class PermissionsController { * Should only be called after confirming that the permissions exist, to * avoid sending unnecessary notifications. * - * @param {Object} domains { origin: [permissions] } + * @param {Object} domains { origin: [permissions] } - The map of domain + * origins to permissions to remove. */ removePermissionsFor (domains) { @@ -449,10 +460,7 @@ export class PermissionsController { perms.map((methodName) => { if (methodName === 'eth_accounts') { - this.notifyDomain( - origin, - { method: NOTIFICATION_NAMES.accountsChanged, result: [] } - ) + this.notifyAccountsChanged(origin, []) } return { parentCapability: methodName } @@ -466,7 +474,7 @@ export class PermissionsController { */ clearPermissions () { this.permissions.clearDomains() - this.notifyAllDomains({ + this._notifyAllDomains({ method: NOTIFICATION_NAMES.accountsChanged, result: [], }) @@ -583,6 +591,7 @@ export class PermissionsController { * @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.') } @@ -618,10 +627,7 @@ export class PermissionsController { async _handleConnectedAccountSelected (origin) { const permittedAccounts = await this.getAccounts(origin) - this.notifyDomain(origin, { - method: NOTIFICATION_NAMES.accountsChanged, - result: permittedAccounts, - }) + this.notifyAccountsChanged(origin, permittedAccounts) } /** diff --git a/app/scripts/controllers/permissions/methodMiddleware.js b/app/scripts/controllers/permissions/methodMiddleware.js index bd6fc8450..76dc363bb 100644 --- a/app/scripts/controllers/permissions/methodMiddleware.js +++ b/app/scripts/controllers/permissions/methodMiddleware.js @@ -9,6 +9,7 @@ export default function createMethodMiddleware ({ getAccounts, getUnlockPromise, hasPermission, + notifyAccountsChanged, requestAccountsPermission, }) { @@ -16,6 +17,8 @@ export default function createMethodMiddleware ({ return createAsyncMiddleware(async (req, res, next) => { + let responseHandler + switch (req.method) { // Intercepting eth_accounts requests for backwards compatibility: @@ -81,10 +84,33 @@ export default function createMethodMiddleware ({ res.result = true return + // register return handler to send accountsChanged notification + case 'wallet_requestPermissions': + + if ('eth_accounts' in req.params?.[0]) { + + responseHandler = async () => { + + if (Array.isArray(res.result)) { + for (const permission of res.result) { + if (permission.parentCapability === 'eth_accounts') { + notifyAccountsChanged(await getAccounts()) + } + } + } + } + } + break + default: break } - next() + // when this promise resolves, the response is on its way back + await next() + + if (responseHandler) { + responseHandler() + } }) } diff --git a/test/unit/app/controllers/permissions/mocks.js b/test/unit/app/controllers/permissions/mocks.js index d83e95cef..6b8fad89f 100644 --- a/test/unit/app/controllers/permissions/mocks.js +++ b/test/unit/app/controllers/permissions/mocks.js @@ -446,6 +446,19 @@ export const getters = deepFreeze({ } }, }, + + notifyAccountsChanged: { + invalidOrigin: (origin) => { + return { + message: `Invalid origin: '${origin}'`, + } + }, + invalidAccounts: () => { + return { + message: 'Invalid accounts', + } + }, + }, }, /** @@ -477,18 +490,6 @@ export const getters = deepFreeze({ result: accounts, } }, - - /** - * Gets a test notification that doesn't occur in practice. - * - * @returns {Object} A notification with the 'test_notification' method name - */ - test: () => { - return { - method: 'test_notification', - result: true, - } - }, }, /** diff --git a/test/unit/app/controllers/permissions/permissions-controller-test.js b/test/unit/app/controllers/permissions/permissions-controller-test.js index b085e0bf7..1f511235a 100644 --- a/test/unit/app/controllers/permissions/permissions-controller-test.js +++ b/test/unit/app/controllers/permissions/permissions-controller-test.js @@ -1163,7 +1163,7 @@ describe('permissions controller', function () { }) }) - describe('notifyDomain', function () { + describe('notifyAccountsChanged', function () { let notifications, permController @@ -1173,11 +1173,11 @@ describe('permissions controller', function () { sinon.spy(permController.permissionsLog, 'updateAccountsHistory') }) - it('notifyDomain handles accountsChanged', async function () { + it('notifyAccountsChanged records history and sends notification', async function () { - permController.notifyDomain( + permController.notifyAccountsChanged( ORIGINS.a, - NOTIFICATIONS.newAccounts(ACCOUNTS.a.permitted), + ACCOUNTS.a.permitted, ) assert.ok( @@ -1192,19 +1192,45 @@ describe('permissions controller', function () { ) }) - it('notifyDomain handles notifications other than accountsChanged', async function () { + it('notifyAccountsChanged throws on invalid origin', async function () { - permController.notifyDomain(ORIGINS.a, NOTIFICATIONS.test()) - - assert.ok( - permController.permissionsLog.updateAccountsHistory.notCalled, - 'permissionsLog.updateAccountsHistory should not have been called' + assert.throws( + () => permController.notifyAccountsChanged( + 4, + ACCOUNTS.a.permitted, + ), + ERRORS.notifyAccountsChanged.invalidOrigin(4), + 'should throw expected error for non-string origin' ) - assert.deepEqual( - notifications[ORIGINS.a], - [ NOTIFICATIONS.test() ], - 'origin should have correct notification' + assert.throws( + () => permController.notifyAccountsChanged( + '', + ACCOUNTS.a.permitted, + ), + ERRORS.notifyAccountsChanged.invalidOrigin(''), + 'should throw expected error for empty string origin' + ) + }) + + it('notifyAccountsChanged throws on invalid accounts', async function () { + + assert.throws( + () => permController.notifyAccountsChanged( + ORIGINS.a, + 4, + ), + ERRORS.notifyAccountsChanged.invalidAccounts(), + 'should throw expected error for truthy non-array accounts' + ) + + assert.throws( + () => permController.notifyAccountsChanged( + ORIGINS.a, + null, + ), + ERRORS.notifyAccountsChanged.invalidAccounts(), + 'should throw expected error for falsy non-array accounts' ) }) }) diff --git a/test/unit/app/controllers/permissions/permissions-middleware-test.js b/test/unit/app/controllers/permissions/permissions-middleware-test.js index 80f995ea0..7988564ae 100644 --- a/test/unit/app/controllers/permissions/permissions-middleware-test.js +++ b/test/unit/app/controllers/permissions/permissions-middleware-test.js @@ -1,5 +1,5 @@ import { strict as assert } from 'assert' -import { useFakeTimers } from 'sinon' +import sinon from 'sinon' import { METADATA_STORE_KEY, @@ -58,6 +58,7 @@ describe('permissions middleware', function () { beforeEach(function () { permController = initPermController() + permController.notifyAccountsChanged = sinon.fake() }) it('grants permissions on user approval', async function () { @@ -107,6 +108,13 @@ describe('permissions middleware', function () { aAccounts, [ACCOUNTS.a.primary], 'origin should have correct accounts' ) + + assert.ok( + permController.notifyAccountsChanged.calledOnceWith( + ORIGINS.a, aAccounts, + ), + 'expected notification call should have been made' + ) }) it('handles serial approved requests that overwrite existing permissions', async function () { @@ -157,6 +165,13 @@ describe('permissions middleware', function () { 'origin should have correct accounts' ) + assert.ok( + permController.notifyAccountsChanged.calledOnceWith( + ORIGINS.a, accounts1, + ), + 'expected notification call should have been made' + ) + // create second request const requestedPerms2 = { @@ -211,6 +226,18 @@ describe('permissions middleware', function () { accounts2, [ACCOUNTS.b.primary], 'origin should have correct accounts' ) + + assert.equal( + permController.notifyAccountsChanged.callCount, 2, + 'should have called notification method 2 times in total' + ) + + assert.ok( + permController.notifyAccountsChanged.lastCall.calledWith( + ORIGINS.a, accounts2, + ), + 'expected notification call should have been made' + ) }) it('rejects permissions on user rejection', async function () { @@ -252,6 +279,11 @@ describe('permissions middleware', function () { assert.deepEqual( aAccounts, [], 'origin should have have correct accounts' ) + + assert.ok( + permController.notifyAccountsChanged.notCalled, + 'should not have called notification method' + ) }) it('rejects requests with unknown permissions', async function () { @@ -288,6 +320,11 @@ describe('permissions middleware', function () { ), 'response should have expected error and no result' ) + + assert.ok( + permController.notifyAccountsChanged.notCalled, + 'should not have called notification method' + ) }) it('accepts only a single pending permissions request per origin', async function () { @@ -695,7 +732,7 @@ describe('permissions middleware', function () { beforeEach(function () { permController = initPermController() - clock = useFakeTimers(1) + clock = sinon.useFakeTimers(1) }) afterEach(function () {