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

@@ -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.

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

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

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

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

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))
 }
 
 /**