From 6ca18c3573bcbb6bdcdd46eb737e193b9af8a562 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Mon, 15 Jun 2020 10:27:27 -0300 Subject: [PATCH] Fix handling of permissions of removed accounts (#8803) Imported accounts can be removed, but the permissions controller is not informed when this happens. Permissions are now removed as part of the account removal process. Additionally, the `getPermittedIdentitiesForCurrentTab` selector now filters out any non-existent accounts, in case a render occurs in the middle of an account removal. This was resulting in a render crash upon opening the popup on a site that was connected to the removed account. --- app/scripts/controllers/permissions/index.js | 18 +++ app/scripts/metamask-controller.js | 2 + .../controllers/metamask-controller-test.js | 5 + .../permissions-controller-test.js | 106 ++++++++++++++++++ test/unit/ui/app/actions.spec.js | 13 ++- ui/app/selectors/permissions.js | 4 +- 6 files changed, 141 insertions(+), 7 deletions(-) diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index 876b2cd2f..4c55e7b8a 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -341,6 +341,24 @@ export class PermissionsController { this.notifyAccountsChanged(origin, newPermittedAccounts) } + /** + * Remove all permissions associated with a particular account. Any eth_accounts + * permissions left with no permitted accounts will be removed as well. + * + * Throws error if the account is invalid, or if the update fails. + * + * @param {string} account - The account to remove. + */ + async removeAllAccountPermissions (account) { + this.validatePermittedAccounts([account]) + + const domains = this.permissions.getDomains() + const connectedOrigins = Object.keys(domains) + .filter((origin) => this._getPermittedAccounts(origin).includes(account)) + + await Promise.all(connectedOrigins.map((origin) => this.removePermittedAccount(origin, account))) + } + /** * Finalizes a permissions request. Throws if request validation fails. * Clones the passed-in parameters to prevent inadvertent modification. diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 1ec0ac43d..9e50a1f51 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -997,6 +997,8 @@ export default class MetamaskController extends EventEmitter { * */ async removeAccount (address) { + // Remove all associated permissions + await this.permissionsController.removeAllAccountPermissions(address) // Remove account from the preferences controller this.preferencesController.removeAddress(address) // Remove account from the account tracker controller diff --git a/test/unit/app/controllers/metamask-controller-test.js b/test/unit/app/controllers/metamask-controller-test.js index 3201e695c..262202419 100644 --- a/test/unit/app/controllers/metamask-controller-test.js +++ b/test/unit/app/controllers/metamask-controller-test.js @@ -599,6 +599,7 @@ describe('MetaMaskController', function () { sinon.stub(metamaskController.preferencesController, 'removeAddress') sinon.stub(metamaskController.accountTracker, 'removeAccount') sinon.stub(metamaskController.keyringController, 'removeAccount') + sinon.stub(metamaskController.permissionsController, 'removeAllAccountPermissions') ret = await metamaskController.removeAccount(addressToRemove) @@ -608,6 +609,7 @@ describe('MetaMaskController', function () { metamaskController.keyringController.removeAccount.restore() metamaskController.accountTracker.removeAccount.restore() metamaskController.preferencesController.removeAddress.restore() + metamaskController.permissionsController.removeAllAccountPermissions.restore() }) it('should call preferencesController.removeAddress', async function () { @@ -619,6 +621,9 @@ describe('MetaMaskController', function () { it('should call keyringController.removeAccount', async function () { assert(metamaskController.keyringController.removeAccount.calledWith(addressToRemove)) }) + it('should call permissionsController.removeAllAccountPermissions', async function () { + assert(metamaskController.permissionsController.removeAllAccountPermissions.calledWith(addressToRemove)) + }) it('should return address', async function () { assert.equal(ret, '0x1') }) diff --git a/test/unit/app/controllers/permissions/permissions-controller-test.js b/test/unit/app/controllers/permissions/permissions-controller-test.js index b5958baa5..f2272e311 100644 --- a/test/unit/app/controllers/permissions/permissions-controller-test.js +++ b/test/unit/app/controllers/permissions/permissions-controller-test.js @@ -660,6 +660,112 @@ describe('permissions controller', function () { }) }) + + describe('removeAllAccountPermissions', function () { + let permController, notifications + + beforeEach(function () { + notifications = initNotifications() + permController = initPermController(notifications) + grantPermissions( + permController, DOMAINS.a.origin, + PERMS.finalizedRequests.eth_accounts(ACCOUNTS.a.permitted) + ) + grantPermissions( + permController, DOMAINS.b.origin, + PERMS.finalizedRequests.eth_accounts(ACCOUNTS.b.permitted) + ) + grantPermissions( + permController, DOMAINS.c.origin, + PERMS.finalizedRequests.eth_accounts(ACCOUNTS.b.permitted) + ) + }) + + it('should throw if account is not a string', async function () { + await assert.rejects( + () => permController.removeAllAccountPermissions({}), + ERRORS.validatePermittedAccounts.nonKeyringAccount({}), + 'should throw on non-string account param' + ) + }) + + it('should throw if given account is not in keyring', async function () { + await assert.rejects( + () => permController.removeAllAccountPermissions(DUMMY_ACCOUNT), + ERRORS.validatePermittedAccounts.nonKeyringAccount(DUMMY_ACCOUNT), + 'should throw on non-keyring account' + ) + }) + + it('should remove permitted account from single origin', async function () { + await permController.removeAllAccountPermissions(ACCOUNTS.a.permitted[1]) + + const accounts = await permController._getPermittedAccounts(DOMAINS.a.origin) + + assert.deepEqual( + accounts, ACCOUNTS.a.permitted.filter((acc) => acc !== ACCOUNTS.a.permitted[1]), + 'origin should have correct accounts' + ) + + assert.deepEqual( + notifications[DOMAINS.a.origin][0], + NOTIFICATIONS.newAccounts([ACCOUNTS.a.primary]), + 'origin should have correct notification' + ) + }) + + it('should permitted account from multiple origins', async function () { + await permController.removeAllAccountPermissions(ACCOUNTS.b.permitted[0]) + + const bAccounts = await permController.getAccounts(DOMAINS.b.origin) + assert.deepEqual( + bAccounts, [], + 'first origin should no accounts' + ) + + const cAccounts = await permController.getAccounts(DOMAINS.c.origin) + assert.deepEqual( + cAccounts, [], + 'second origin no accounts' + ) + + assert.deepEqual( + notifications[DOMAINS.b.origin][0], + NOTIFICATIONS.removedAccounts(), + 'first origin should have correct notification' + ) + + assert.deepEqual( + notifications[DOMAINS.c.origin][0], + NOTIFICATIONS.removedAccounts(), + 'second origin should have correct notification' + ) + }) + + it('should remove eth_accounts permission if removing only permitted account', async function () { + await permController.removeAllAccountPermissions(ACCOUNTS.b.permitted[0]) + + const accounts = await permController.getAccounts(DOMAINS.b.origin) + + assert.deepEqual( + accounts, [], + 'origin should have no accounts' + ) + + const permission = await permController.permissions.getPermission( + DOMAINS.b.origin, PERM_NAMES.eth_accounts + ) + + assert.equal(permission, undefined, 'origin should not have eth_accounts permission') + + assert.deepEqual( + notifications[DOMAINS.b.origin][0], + NOTIFICATIONS.removedAccounts(), + 'origin should have correct notification' + ) + }) + }) + describe('finalizePermissionsRequest', function () { let permController diff --git a/test/unit/ui/app/actions.spec.js b/test/unit/ui/app/actions.spec.js index b9cdd5716..0affebe46 100644 --- a/test/unit/ui/app/actions.spec.js +++ b/test/unit/ui/app/actions.spec.js @@ -221,10 +221,10 @@ describe('Actions', function () { }) describe('#removeAccount', function () { - let removeAccountSpy + let removeAccountStub afterEach(function () { - removeAccountSpy.restore() + removeAccountStub.restore() }) it('calls removeAccount in background and expect actions to show account', async function () { @@ -238,10 +238,11 @@ describe('Actions', function () { 'SHOW_ACCOUNTS_PAGE', ] - removeAccountSpy = sinon.spy(background, 'removeAccount') + removeAccountStub = sinon.stub(background, 'removeAccount') + removeAccountStub.callsFake((_, callback) => callback()) await store.dispatch(actions.removeAccount('0xe18035bf8712672935fdb4e5e431b1a0183d2dfc')) - assert(removeAccountSpy.calledOnce) + assert(removeAccountStub.calledOnce) const actionTypes = store .getActions() .map((action) => action.type) @@ -257,8 +258,8 @@ describe('Actions', function () { 'HIDE_LOADING_INDICATION', ] - removeAccountSpy = sinon.stub(background, 'removeAccount') - removeAccountSpy.callsFake((_, callback) => { + removeAccountStub = sinon.stub(background, 'removeAccount') + removeAccountStub.callsFake((_, callback) => { callback(new Error('error')) }) diff --git a/ui/app/selectors/permissions.js b/ui/app/selectors/permissions.js index 684bedac2..2960a753f 100644 --- a/ui/app/selectors/permissions.js +++ b/ui/app/selectors/permissions.js @@ -122,7 +122,9 @@ export function getConnectedDomainsForSelectedAddress (state) { export function getPermittedIdentitiesForCurrentTab (state) { const permittedAccounts = getPermittedAccountsForCurrentTab(state) const identities = getMetaMaskIdentities(state) - return permittedAccounts.map((address) => identities[address]) + return permittedAccounts + .map((address) => identities[address]) + .filter((identity) => Boolean(identity)) } /**