diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index 5b205d779..fc51c41a3 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -1,3 +1,4 @@ +import nanoid from 'nanoid' import JsonRpcEngine from 'json-rpc-engine' import asMiddleware from 'json-rpc-engine/src/asMiddleware' import ObservableStore from 'obs-store' @@ -95,7 +96,7 @@ export class PermissionsController { getUnlockPromise: () => this._getUnlockPromise(true), hasPermission: this.hasPermission.bind(this, origin), requestAccountsPermission: this._requestPermissions.bind( - this, origin, { eth_accounts: {} } + this, { origin }, { eth_accounts: {} }, ), })) @@ -106,6 +107,17 @@ export class PermissionsController { return asMiddleware(engine) } + /** + * Request {@code eth_accounts} permissions + * @param {string} origin - The origin + * @returns {Promise} the request ID + */ + async requestAccountsPermission (origin) { + const id = nanoid() + this._requestPermissions({ origin, id }, { eth_accounts: {} }) + return id + } + /** * Returns the accounts that should be exposed for the given origin domain, * if any. This method exists for when a trusted context needs to know @@ -155,10 +167,10 @@ export class PermissionsController { /** * Submits a permissions request to rpc-cap. Internal, background use only. * - * @param {string} origin - The origin string. + * @param {IOriginMetadata} metadata - The origin metadata. * @param {IRequestedPermissions} permissions - The requested permissions. */ - _requestPermissions (origin, permissions) { + _requestPermissions (metadata, permissions) { return new Promise((resolve, reject) => { // rpc-cap assigns an id to the request if there is none, as expected by @@ -166,7 +178,7 @@ export class PermissionsController { const req = { method: 'wallet_requestPermissions', params: [permissions] } const res = {} this.permissions.providerMiddlewareFunction( - { origin }, req, res, () => {}, _end + metadata, req, res, () => {}, _end ) function _end (_err) { @@ -244,70 +256,6 @@ export class PermissionsController { this._removePendingApproval(id) } - /** - * @deprecated - * Grants the given origin the eth_accounts permission for the given account(s). - * This method should ONLY be called as a result of direct user action in the UI, - * with the intention of supporting legacy dapps that don't support EIP 1102. - * - * @param {string} origin - The origin to expose the account(s) to. - * @param {Array} accounts - The account(s) to expose. - */ - async legacyExposeAccounts (origin, accounts) { - - // accounts are validated by finalizePermissionsRequest - if (typeof origin !== 'string' || !origin.length) { - throw new Error('Must provide non-empty string origin.') - } - - const existingAccounts = await this.getAccounts(origin) - - if (existingAccounts.length > 0) { - throw new Error( - 'May not call legacyExposeAccounts on origin with exposed accounts.' - ) - } - - const permissions = await this.finalizePermissionsRequest( - { eth_accounts: {} }, accounts - ) - - try { - - await new Promise((resolve, reject) => { - this.permissions.grantNewPermissions( - origin, permissions, {}, _end - ) - - function _end (err) { - if (err) { - reject(err) - } else { - resolve() - } - } - }) - - const newPermittedAccounts = await this.getAccounts(origin) - - this.notifyDomain(origin, { - method: NOTIFICATION_NAMES.accountsChanged, - result: newPermittedAccounts, - }) - this.permissionsLog.logAccountExposure(origin, accounts) - - } catch (error) { - - throw ethErrors.rpc.internal({ - message: `Failed to add 'eth_accounts' to '${origin}'.`, - data: { - originalError: error, - accounts, - }, - }) - } - } - /** * Expose an account to the given origin. Changes the eth_accounts * permissions and emits accountsChanged. diff --git a/app/scripts/controllers/permissions/permissionsLog.js b/app/scripts/controllers/permissions/permissionsLog.js index 71db901dc..927dbf018 100644 --- a/app/scripts/controllers/permissions/permissionsLog.js +++ b/app/scripts/controllers/permissions/permissionsLog.js @@ -202,33 +202,6 @@ export default class PermissionsLogController { this.updateActivityLog(logs) } - /** - * Record account exposure and eth_accounts permissions history for the given - * origin. - * - * @param {string} origin - The origin accounts were exposed to. - * @param {Array} accounts - The accounts that were exposed. - */ - logAccountExposure (origin, accounts) { - - if ( - typeof origin !== 'string' || !origin.length || - !Array.isArray(accounts) || accounts.length === 0 - ) { - throw new Error( - 'Must provide non-empty string origin and array of accounts.' - ) - } - - this.logPermissionsHistory( - ['eth_accounts'], - origin, - accounts, - Date.now(), - true - ) - } - /** * Create new permissions history log entries, if any, and commit them. * diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 8aa7e5a39..6fb68b182 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -573,7 +573,7 @@ export default class MetamaskController extends EventEmitter { removePermissionsFor: permissionsController.removePermissionsFor.bind(permissionsController), addPermittedAccount: nodeify(permissionsController.addPermittedAccount, permissionsController), removePermittedAccount: nodeify(permissionsController.removePermittedAccount, permissionsController), - legacyExposeAccounts: nodeify(permissionsController.legacyExposeAccounts, permissionsController), + requestAccountsPermission: nodeify(permissionsController.requestAccountsPermission, permissionsController), getRequestAccountTabIds: (cb) => cb(null, this.getRequestAccountTabIds()), getOpenMetamaskTabsIds: (cb) => cb(null, this.getOpenMetamaskTabsIds()), diff --git a/test/unit/app/controllers/permissions/mocks.js b/test/unit/app/controllers/permissions/mocks.js index 80f9509af..d83e95cef 100644 --- a/test/unit/app/controllers/permissions/mocks.js +++ b/test/unit/app/controllers/permissions/mocks.js @@ -380,20 +380,6 @@ export const getters = deepFreeze({ }, }, - legacyExposeAccounts: { - badOrigin: () => { - return { - message: 'Must provide non-empty string origin.', - } - }, - forbiddenUsage: () => { - return { - name: 'Error', - message: 'May not call legacyExposeAccounts on origin with exposed accounts.', - } - }, - }, - _handleAccountSelected: { invalidParams: () => { return { @@ -440,14 +426,6 @@ export const getters = deepFreeze({ }, }, - logAccountExposure: { - invalidParams: () => { - return { - message: 'Must provide non-empty string origin and array of accounts.', - } - }, - }, - pendingApprovals: { duplicateOriginOrId: (id, origin) => { return { diff --git a/test/unit/app/controllers/permissions/permissions-controller-test.js b/test/unit/app/controllers/permissions/permissions-controller-test.js index 86a173829..2d1de4630 100644 --- a/test/unit/app/controllers/permissions/permissions-controller-test.js +++ b/test/unit/app/controllers/permissions/permissions-controller-test.js @@ -716,119 +716,6 @@ describe('permissions controller', function () { }) }) - describe('legacyExposeAccounts', function () { - - let permController, notifications - - beforeEach(function () { - notifications = initNotifications() - permController = initPermController(notifications) - }) - - it('successfully exposes accounts and updates permissions history', async function () { - - let aAccounts = await permController.getAccounts(ORIGINS.a) - assert.deepEqual(aAccounts, [], 'origin should have no accounts') - - await permController.legacyExposeAccounts(ORIGINS.a, ACCOUNTS.a.permitted) - - aAccounts = await permController.getAccounts(ORIGINS.a) - assert.deepEqual(aAccounts, [ACCOUNTS.a.primary], 'origin should have correct accounts') - - // now, permissions history should be updated - const permissionsHistory = permController.permissionsLog.getHistory() - const historyOrigins = Object.keys(permissionsHistory) - - assert.equal(historyOrigins.length, 1, 'should have single origin') - assert.equal(historyOrigins[0], ORIGINS.a, 'should have correct origin') - - assert.ok( - permissionsHistory[ORIGINS.a].eth_accounts, - 'history should have eth_accounts entry' - ) - - assert.deepEqual( - Object.keys(permissionsHistory[ORIGINS.a].eth_accounts.accounts), - ACCOUNTS.a.permitted, - 'should have expected eth_accounts entry accounts' - ) - - // notification should also have been sent - assert.deepEqual( - notifications[ORIGINS.a][0], - NOTIFICATIONS.newAccounts([ACCOUNTS.a.primary]), - 'first origin should have correct notification' - ) - }) - - it('throws if called on origin with existing exposed accounts', async function () { - - grantPermissions( - permController, ORIGINS.a, - PERMS.finalizedRequests.eth_accounts(ACCOUNTS.a.permitted) - ) - - const aAccounts = await permController.getAccounts(ORIGINS.a) - assert.deepEqual(aAccounts, [ACCOUNTS.a.primary], 'origin should have correct accounts') - - await assert.rejects( - permController.legacyExposeAccounts(ORIGINS.a, ACCOUNTS.b.permitted), - ERRORS.legacyExposeAccounts.forbiddenUsage(), - 'should throw if called on origin with existing exposed accounts' - ) - - const permissionsHistory = permController.permissionsLog.getHistory() - assert.deepEqual( - permissionsHistory, {}, - 'should not have modified history' - ) - assert.deepEqual( - notifications[ORIGINS.a], [], - 'should not have sent notification' - ) - }) - - it('throws if called with bad accounts', async function () { - - await assert.rejects( - permController.legacyExposeAccounts(ORIGINS.a, []), - ERRORS.validatePermittedAccounts.invalidParam(), - 'should throw if called with no accounts' - ) - - const permissionsHistory = permController.permissionsLog.getHistory() - assert.deepEqual( - permissionsHistory, {}, - 'should not have modified history' - ) - assert.deepEqual( - notifications[ORIGINS.a], [], - 'should not have sent notification' - ) - }) - - it('throws if called with bad origin', async function () { - - await assert.rejects( - permController.legacyExposeAccounts(null, ACCOUNTS.a.permitted), - ERRORS.legacyExposeAccounts.badOrigin(), - 'should throw if called with invalid origin' - ) - - const permissionsHistory = permController.permissionsLog.getHistory() - assert.deepEqual( - permissionsHistory, {}, - 'should not have modified history' - ) - Object.keys(notifications).forEach((domain) => { - assert.deepEqual( - notifications[domain], [], - 'should not have sent notification' - ) - }) - }) - }) - describe('preferences state update', function () { let permController, notifications, preferences, identities @@ -1504,6 +1391,16 @@ describe('permissions controller', function () { permController = initPermController() }) + it('requestAccountsPermission calls _requestAccountsPermission with an explicit request ID', async function () { + const _requestPermissions = sinon.stub(permController, '_requestPermissions').resolves() + await permController.requestAccountsPermission('example.com') + assert.ok(_requestPermissions.calledOnceWithExactly( + sinon.match.object.and(sinon.match.has('origin')).and(sinon.match.has('id')), + { eth_accounts: {} }, + )) + _requestPermissions.restore() + }) + it('_addPendingApproval: should throw if adding origin twice', function () { const id = nanoid() diff --git a/test/unit/app/controllers/permissions/permissions-log-controller-test.js b/test/unit/app/controllers/permissions/permissions-log-controller-test.js index 5590d5a96..ea3a14714 100644 --- a/test/unit/app/controllers/permissions/permissions-log-controller-test.js +++ b/test/unit/app/controllers/permissions/permissions-log-controller-test.js @@ -22,7 +22,6 @@ import { } from './mocks' const { - ERRORS, PERMS, RPC_REQUESTS, } = getters @@ -647,44 +646,4 @@ describe('permissions log', function () { ) }) }) - - describe('instance method edge cases', function () { - - it('logAccountExposure errors on invalid params', function () { - - const permLog = initPermLog() - - assert.throws( - () => { - permLog.logAccountExposure('', ACCOUNTS.a.permitted) - }, - ERRORS.logAccountExposure.invalidParams(), - 'should throw expected error' - ) - - assert.throws( - () => { - permLog.logAccountExposure(null, ACCOUNTS.a.permitted) - }, - ERRORS.logAccountExposure.invalidParams(), - 'should throw expected error' - ) - - assert.throws( - () => { - permLog.logAccountExposure('foo', {}) - }, - ERRORS.logAccountExposure.invalidParams(), - 'should throw expected error' - ) - - assert.throws( - () => { - permLog.logAccountExposure('foo', []) - }, - ERRORS.logAccountExposure.invalidParams(), - 'should throw expected error' - ) - }) - }) }) diff --git a/ui/app/pages/connected-sites/connected-sites.component.js b/ui/app/pages/connected-sites/connected-sites.component.js index 11b0c3926..6249dc274 100644 --- a/ui/app/pages/connected-sites/connected-sites.component.js +++ b/ui/app/pages/connected-sites/connected-sites.component.js @@ -20,11 +20,11 @@ export default class ConnectedSites extends Component { disconnectAllAccounts: PropTypes.func.isRequired, disconnectAccount: PropTypes.func.isRequired, getOpenMetamaskTabsIds: PropTypes.func.isRequired, - legacyExposeAccount: PropTypes.func.isRequired, permittedAccountsByOrigin: PropTypes.objectOf( PropTypes.arrayOf(PropTypes.string), ).isRequired, tabToConnect: PropTypes.object, + requestAccountsPermission: PropTypes.func.isRequired, } state = { @@ -76,13 +76,12 @@ export default class ConnectedSites extends Component { } renderConnectedSitesPopover () { - const { accountLabel, closePopover, connectedDomains, - legacyExposeAccount, tabToConnect, + requestAccountsPermission, } = this.props const { t } = this.context @@ -100,7 +99,7 @@ export default class ConnectedSites extends Component { ? ( {t('connectManually')} diff --git a/ui/app/pages/connected-sites/connected-sites.container.js b/ui/app/pages/connected-sites/connected-sites.container.js index e09c39226..32456670c 100644 --- a/ui/app/pages/connected-sites/connected-sites.container.js +++ b/ui/app/pages/connected-sites/connected-sites.container.js @@ -2,7 +2,7 @@ import { connect } from 'react-redux' import ConnectedSites from './connected-sites.component' import { getOpenMetamaskTabsIds, - legacyExposeAccounts, + requestAccountsPermission, removePermissionsFor, removePermittedAccount, } from '../../store/actions' @@ -14,7 +14,7 @@ import { getPermittedAccountsByOrigin, getSelectedAddress, } from '../../selectors' -import { DEFAULT_ROUTE } from '../../helpers/constants/routes' +import { CONNECT_ROUTE, DEFAULT_ROUTE } from '../../helpers/constants/routes' const mapStateToProps = (state) => { const { openMetaMaskTabs } = state.appState @@ -57,7 +57,7 @@ const mapDispatchToProps = (dispatch) => { [domainKey]: permissionMethodNames, })) }, - legacyExposeAccounts: (origin, account) => dispatch(legacyExposeAccounts(origin, [account])), + requestAccountsPermission: (origin) => dispatch(requestAccountsPermission(origin)), } } @@ -71,7 +71,7 @@ const mergeProps = (stateProps, dispatchProps, ownProps) => { const { disconnectAccount, disconnectAllAccounts, - legacyExposeAccounts: dispatchLegacyExposeAccounts, + requestAccountsPermission: dispatchRequestAccountsPermission, } = dispatchProps const { history } = ownProps @@ -94,7 +94,10 @@ const mergeProps = (stateProps, dispatchProps, ownProps) => { closePopover() } }, - legacyExposeAccount: () => dispatchLegacyExposeAccounts(tabToConnect.origin, selectedAddress), + requestAccountsPermission: async () => { + const id = await dispatchRequestAccountsPermission(tabToConnect.origin) + history.push(`${CONNECT_ROUTE}/${id}`) + }, } } diff --git a/ui/app/store/actions.js b/ui/app/store/actions.js index 71e9bfce8..e45822cfe 100644 --- a/ui/app/store/actions.js +++ b/ui/app/store/actions.js @@ -2035,6 +2035,14 @@ export function setPendingTokens (pendingTokens) { // Permissions +export function requestAccountsPermission (origin) { + return async (dispatch) => { + const id = await promisifiedBackground.requestAccountsPermission(origin) + await forceUpdateMetamaskState(dispatch) + return id + } +} + /** * Approves the permissions request. * @param {Object} request - The permissions request to approve @@ -2064,16 +2072,6 @@ export function rejectPermissionsRequest (requestId) { } } -/** - * Exposes the given account(s) to the given origin. - * Call ONLY as a result of direct user action. - */ -export function legacyExposeAccounts (origin, accounts) { - return () => { - return background.legacyExposeAccounts(origin, accounts) - } -} - /** * Clears the given permissions for the given origin. */