diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index 9f5f6cd45..f5852687a 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/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. diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 370e58251..91cd0ec44 100644 --- a/app/scripts/metamask-controller.js +++ b/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()), diff --git a/test/unit/app/controllers/permissions/mocks.js b/test/unit/app/controllers/permissions/mocks.js index dbc08dfc0..4220e8119 100644 --- a/test/unit/app/controllers/permissions/mocks.js +++ b/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`, } }, }, diff --git a/test/unit/app/controllers/permissions/permissions-controller-test.js b/test/unit/app/controllers/permissions/permissions-controller-test.js index 80c1e7341..8740243c8 100644 --- a/test/unit/app/controllers/permissions/permissions-controller-test.js +++ b/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 diff --git a/ui/app/store/actions.js b/ui/app/store/actions.js index 318572a70..0265f7195 100644 --- a/ui/app/store/actions.js +++ b/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,