Add PermissionsController.removePermittedAccount (#8354)

* add PermissionsController.removePermittedAccount and corresponding UI action

* remove eth_accounts permission on removing last account

app/scripts/controllers/permissions/index.js

@@ -316,22 +316,27 @@ export class PermissionsController {
    * @param {string} account - The new account to expose.
    */
   async addPermittedAccount (origin, account) {
+
     const domains = this.permissions.getDomains()
     if (!domains[origin]) {
       throw new Error('Unrecognized domain')
     }
+
     this.validatePermittedAccounts([account])
 
     const oldPermittedAccounts = this._getPermittedAccounts(origin)
     if (!oldPermittedAccounts) {
-      throw new Error('Origin does not have \'eth_accounts\' permission')
+      throw new Error(`Origin does not have 'eth_accounts' permission`)
     } else if (oldPermittedAccounts.includes(account)) {
-      throw new Error('Account is already permitted')
+      throw new Error('Account is already permitted for origin')
     }
 
     this.permissions.updateCaveatFor(
-      origin, 'eth_accounts', CAVEAT_NAMES.exposedAccounts, [...oldPermittedAccounts, account]
+      origin, 'eth_accounts',
+      CAVEAT_NAMES.exposedAccounts,
+      [...oldPermittedAccounts, account]
     )
+
     const permittedAccounts = await this.getAccounts(origin)
 
     this.notifyDomain(origin, {
@@ -340,6 +345,58 @@ export class PermissionsController {
     })
   }
 
+  /**
+   * Removes an exposed account from the given origin. Changes the eth_accounts
+   * permission and emits accountsChanged.
+   * If origin only has a single permitted account, removes the eth_accounts
+   * permission from the origin.
+   *
+   * Throws error if the origin or account is invalid, or if the update fails.
+   *
+   * @param {string} origin - The origin to remove the account from.
+   * @param {string} account - The account to remove.
+   */
+  async removePermittedAccount (origin, account) {
+
+    const domains = this.permissions.getDomains()
+    if (!domains[origin]) {
+      throw new Error('Unrecognized domain')
+    }
+
+    this.validatePermittedAccounts([account])
+
+    const oldPermittedAccounts = this._getPermittedAccounts(origin)
+    if (!oldPermittedAccounts) {
+      throw new Error(`Origin does not have 'eth_accounts' permission`)
+    } else if (!oldPermittedAccounts.includes(account)) {
+      throw new Error('Account is not permitted for origin')
+    }
+
+    let newPermittedAccounts = oldPermittedAccounts
+      .filter((acc) => acc !== account)
+
+    if (newPermittedAccounts.length === 0) {
+
+      this.permissions.removePermissionsFor(
+        origin, [{ parentCapability: 'eth_accounts' }]
+      )
+    } else {
+
+      this.permissions.updateCaveatFor(
+        origin, 'eth_accounts',
+        CAVEAT_NAMES.exposedAccounts,
+        newPermittedAccounts,
+      )
+
+      newPermittedAccounts = await this.getAccounts(origin)
+    }
+
+    this.notifyDomain(origin, {
+      method: NOTIFICATION_NAMES.accountsChanged,
+      result: newPermittedAccounts,
+    })
+  }
+
   /**
    * Finalizes a permissions request. Throws if request validation fails.
    * Clones the passed-in parameters to prevent inadvertent modification.

app/scripts/metamask-controller.js

@@ -577,6 +577,7 @@ export default class MetamaskController extends EventEmitter {
       rejectPermissionsRequest: nodeify(permissionsController.rejectPermissionsRequest, permissionsController),
       removePermissionsFor: permissionsController.removePermissionsFor.bind(permissionsController),
       addPermittedAccount: nodeify(permissionsController.addPermittedAccount, permissionsController),
+      removePermittedAccount: nodeify(permissionsController.removePermittedAccount, permissionsController),
       legacyExposeAccounts: nodeify(permissionsController.legacyExposeAccounts, permissionsController),
 
       getRequestAccountTabIds: (cb) => cb(null, this.getRequestAccountTabIds()),

test/unit/app/controllers/permissions/mocks.js

@@ -333,7 +333,7 @@ export const getters = deepFreeze({
     addPermittedAccount: {
       alreadyPermitted: () => {
         return {
-          message: 'Account is already permitted',
+          message: 'Account is already permitted for origin',
         }
       },
       invalidOrigin: () => {
@@ -343,7 +343,25 @@ export const getters = deepFreeze({
       },
       noEthAccountsPermission: () => {
         return {
-          message: 'Origin does not have \'eth_accounts\' permission',
+          message: `Origin does not have 'eth_accounts' permission`,
+        }
+      },
+    },
+
+    removePermittedAccount: {
+      notPermitted: () => {
+        return {
+          message: 'Account is not permitted for origin',
+        }
+      },
+      invalidOrigin: () => {
+        return {
+          message: 'Unrecognized domain',
+        }
+      },
+      noEthAccountsPermission: () => {
+        return {
+          message: `Origin does not have 'eth_accounts' permission`,
         }
       },
     },

test/unit/app/controllers/permissions/permissions-controller-test.js

@@ -524,7 +524,7 @@ describe('permissions controller', function () {
 
     it('should throw if account is already permitted', async function () {
       await assert.rejects(
-        () => permController.addPermittedAccount(ORIGINS.a, ACCOUNT_ARRAYS.c[0]),
+        () => permController.addPermittedAccount(ORIGINS.a, ACCOUNT_ARRAYS.a[0]),
         ERRORS.addPermittedAccount.alreadyPermitted(),
         'should throw if account is already permitted'
       )
@@ -548,6 +548,116 @@ describe('permissions controller', function () {
     })
   })
 
+  describe('removePermittedAccount', 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('should throw if account is not a string', async function () {
+      await assert.rejects(
+        () => permController.removePermittedAccount(ORIGINS.a, {}),
+        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.removePermittedAccount(ORIGINS.a, DUMMY_ACCOUNT),
+        ERRORS.validatePermittedAccounts.nonKeyringAccount(DUMMY_ACCOUNT),
+        'should throw on non-keyring account'
+      )
+    })
+
+    it('should throw if origin is invalid', async function () {
+      await assert.rejects(
+        () => permController.removePermittedAccount(false, EXTRA_ACCOUNT),
+        ERRORS.removePermittedAccount.invalidOrigin(),
+        'should throw on invalid origin'
+      )
+    })
+
+    it('should throw if origin lacks any permissions', async function () {
+      await assert.rejects(
+        () => permController.removePermittedAccount(ORIGINS.c, EXTRA_ACCOUNT),
+        ERRORS.removePermittedAccount.invalidOrigin(),
+        'should throw on origin without permissions'
+      )
+    })
+
+    it('should throw if origin lacks eth_accounts permission', async function () {
+      grantPermissions(
+        permController, ORIGINS.c,
+        PERMS.finalizedRequests.test_method()
+      )
+
+      await assert.rejects(
+        () => permController.removePermittedAccount(ORIGINS.c, EXTRA_ACCOUNT),
+        ERRORS.removePermittedAccount.noEthAccountsPermission(),
+        'should throw on origin without eth_accounts permission'
+      )
+    })
+
+    it('should throw if account is not permitted', async function () {
+      await assert.rejects(
+        () => permController.removePermittedAccount(ORIGINS.b, ACCOUNT_ARRAYS.c[0]),
+        ERRORS.removePermittedAccount.notPermitted(),
+        'should throw if account is not permitted'
+      )
+    })
+
+    it('should successfully remove permitted account', async function () {
+      await permController.removePermittedAccount(ORIGINS.a, ACCOUNT_ARRAYS.a[1])
+
+      const accounts = await permController.getAccounts(ORIGINS.a)
+
+      assert.deepEqual(
+        accounts, ACCOUNT_ARRAYS.a.filter((acc) => acc !== ACCOUNT_ARRAYS.a[1]),
+        'origin should have correct accounts'
+      )
+
+      assert.deepEqual(
+        notifications[ORIGINS.a][0],
+        NOTIFICATIONS.newAccounts(ACCOUNT_ARRAYS.a.filter((acc) => acc !== ACCOUNT_ARRAYS.a[1])),
+        'origin should have correct notification'
+      )
+    })
+
+    it('should remove eth_accounts permission if removing only permitted account', async function () {
+      await permController.removePermittedAccount(ORIGINS.b, ACCOUNT_ARRAYS.b[0])
+
+      const accounts = await permController.getAccounts(ORIGINS.b)
+
+      assert.deepEqual(
+        accounts, [],
+        'origin should have no accounts'
+      )
+
+      const permission = await permController.permissions.getPermission(
+        ORIGINS.b, PERM_NAMES.eth_accounts
+      )
+
+      assert.equal(permission, undefined, 'origin should not have eth_accounts permission')
+
+      assert.deepEqual(
+        notifications[ORIGINS.b][0],
+        NOTIFICATIONS.removedAccounts(),
+        'origin should have correct notification'
+      )
+    })
+  })
+
   describe('finalizePermissionsRequest', function () {
 
     let permController

ui/app/store/actions.js

@@ -1241,6 +1241,20 @@ export function addPermittedAccount (origin, address) {
   }
 }
 
+export function removePermittedAccount (origin, address) {
+  return async (dispatch) => {
+    await new Promise((resolve, reject) => {
+      background.removePermittedAccount(origin, address, (error) => {
+        if (error) {
+          return reject(error)
+        }
+        resolve()
+      })
+    })
+    await forceUpdateMetamaskState(dispatch)
+  }
+}
+
 export function showAccountsPage () {
   return {
     type: actionConstants.SHOW_ACCOUNTS_PAGE,