From d6b28387276e6dd887fdeeba50407d005df72afe Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Thu, 5 Dec 2019 09:29:56 -0800 Subject: [PATCH 01/11] update accounts permission history on accountsChanged Create PermissionsLogController Fix permissions activity log pruning Add selectors, background hooks for better UX Make selected account the first account returned --- app/scripts/controllers/permissions/enums.js | 61 ++++ app/scripts/controllers/permissions/index.js | 265 +++++++++------ .../permissions/loggerMiddleware.js | 168 ---------- .../permissions/permissions-safe-methods.json | 51 --- .../controllers/permissions/permissionsLog.js | 309 ++++++++++++++++++ .../permissions/restrictedMethods.js | 2 +- app/scripts/controllers/preferences.js | 18 + app/scripts/metamask-controller.js | 17 +- package.json | 2 +- .../account-menu/account-menu.container.js | 17 +- ui/app/selectors/permissions.js | 65 ++++ ui/app/selectors/selectors.js | 18 + ui/app/store/actions.js | 1 + yarn.lock | 8 +- 14 files changed, 668 insertions(+), 334 deletions(-) create mode 100644 app/scripts/controllers/permissions/enums.js delete mode 100644 app/scripts/controllers/permissions/loggerMiddleware.js delete mode 100644 app/scripts/controllers/permissions/permissions-safe-methods.json create mode 100644 app/scripts/controllers/permissions/permissionsLog.js create mode 100644 ui/app/selectors/permissions.js diff --git a/app/scripts/controllers/permissions/enums.js b/app/scripts/controllers/permissions/enums.js new file mode 100644 index 000000000000..ef0d883d017c --- /dev/null +++ b/app/scripts/controllers/permissions/enums.js @@ -0,0 +1,61 @@ +module.exports = { + WALLET_PREFIX: 'wallet_', + HISTORY_STORE_KEY: 'permissionsHistory', + LOG_STORE_KEY: 'permissionsLog', + METADATA_STORE_KEY: 'domainMetadata', + CAVEAT_NAMES: { + exposedAccounts: 'exposedAccounts', + }, + NOTIFICATION_NAMES: { + accountsChanged: 'wallet_accountsChanged', + }, + SAFE_METHODS: [ + 'web3_sha3', + 'net_listening', + 'net_peerCount', + 'net_version', + 'eth_blockNumber', + 'eth_call', + 'eth_chainId', + 'eth_coinbase', + 'eth_estimateGas', + 'eth_gasPrice', + 'eth_getBalance', + 'eth_getBlockByHash', + 'eth_getBlockByNumber', + 'eth_getBlockTransactionCountByHash', + 'eth_getBlockTransactionCountByNumber', + 'eth_getCode', + 'eth_getFilterChanges', + 'eth_getFilterLogs', + 'eth_getLogs', + 'eth_getStorageAt', + 'eth_getTransactionByBlockHashAndIndex', + 'eth_getTransactionByBlockNumberAndIndex', + 'eth_getTransactionByHash', + 'eth_getTransactionCount', + 'eth_getTransactionReceipt', + 'eth_getUncleByBlockHashAndIndex', + 'eth_getUncleByBlockNumberAndIndex', + 'eth_getUncleCountByBlockHash', + 'eth_getUncleCountByBlockNumber', + 'eth_getWork', + 'eth_hashrate', + 'eth_mining', + 'eth_newBlockFilter', + 'eth_newFilter', + 'eth_newPendingTransactionFilter', + 'eth_protocolVersion', + 'eth_sendRawTransaction', + 'eth_sendTransaction', + 'eth_sign', + 'personal_sign', + 'eth_signTypedData', + 'eth_signTypedData_v1', + 'eth_signTypedData_v3', + 'eth_submitHashrate', + 'eth_submitWork', + 'eth_syncing', + 'eth_uninstallFilter', + ], +} diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index 5b7a1318ff57..40258f83d0a4 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -4,19 +4,21 @@ import ObservableStore from 'obs-store' import log from 'loglevel' import { CapabilitiesController as RpcCap } from 'rpc-cap' import { ethErrors } from 'eth-json-rpc-errors' + import getRestrictedMethods from './restrictedMethods' import createMethodMiddleware from './methodMiddleware' -import createLoggerMiddleware from './loggerMiddleware' +import PermissionsLogController from './permissionsLog' // Methods that do not require any permissions to use: -import SAFE_METHODS from './permissions-safe-methods.json' - -// some constants -const METADATA_STORE_KEY = 'domainMetadata' -const LOG_STORE_KEY = 'permissionsLog' -const HISTORY_STORE_KEY = 'permissionsHistory' -const WALLET_METHOD_PREFIX = 'wallet_' -const ACCOUNTS_CHANGED_NOTIFICATION = 'wallet_accountsChanged' +import { + SAFE_METHODS, // methods that do not require any permissions to use + WALLET_PREFIX, + METADATA_STORE_KEY, + LOG_STORE_KEY, + HISTORY_STORE_KEY, + CAVEAT_NAMES, + NOTIFICATION_NAMES, +} from './enums' export const CAVEAT_NAMES = { exposedAccounts: 'exposedAccounts', @@ -26,20 +28,30 @@ export class PermissionsController { constructor ( { - platform, notifyDomain, notifyAllDomains, keyringController, + platform, notifyDomain, notifyAllDomains, getKeyringAccounts, } = {}, restoredPermissions = {}, restoredState = {}) { + this.store = new ObservableStore({ [METADATA_STORE_KEY]: restoredState[METADATA_STORE_KEY] || {}, [LOG_STORE_KEY]: restoredState[LOG_STORE_KEY] || [], [HISTORY_STORE_KEY]: restoredState[HISTORY_STORE_KEY] || {}, }) - this.notifyDomain = notifyDomain + + this._notifyDomain = notifyDomain this.notifyAllDomains = notifyAllDomains - this.keyringController = keyringController + this.getKeyringAccounts = getKeyringAccounts this._platform = platform this._restrictedMethods = getRestrictedMethods(this) + this.permissionsLogController = new PermissionsLogController({ + walletPrefix: WALLET_PREFIX, + restrictedMethods: Object.keys(this._restrictedMethods), + ignoreMethods: [ 'wallet_sendDomainMetadata' ], + store: this.store, + logStoreKey: LOG_STORE_KEY, + historyStoreKey: HISTORY_STORE_KEY, + }) this._initializePermissions(restoredPermissions) } @@ -56,14 +68,7 @@ export class PermissionsController { const engine = new JsonRpcEngine() - engine.push(createLoggerMiddleware({ - walletPrefix: WALLET_METHOD_PREFIX, - restrictedMethods: Object.keys(this._restrictedMethods), - ignoreMethods: [ 'wallet_sendDomainMetadata' ], - store: this.store, - logStoreKey: LOG_STORE_KEY, - historyStoreKey: HISTORY_STORE_KEY, - })) + engine.push(this.permissionsLogController.createMiddleware()) engine.push(createMethodMiddleware({ store: this.store, @@ -106,31 +111,6 @@ export class PermissionsController { }) } - /** - * Submits a permissions request to rpc-cap. Internal use only. - * - * @param {string} origin - The origin string. - * @param {IRequestedPermissions} permissions - The requested permissions. - */ - _requestPermissions (origin, permissions) { - return new Promise((resolve, reject) => { - - const req = { method: 'wallet_requestPermissions', params: [permissions] } - const res = {} - this.permissions.providerMiddlewareFunction( - { origin }, req, res, () => {}, _end - ) - - function _end (err) { - if (err || res.error) { - reject(err || res.error) - } else { - resolve(res.result) - } - } - }) - } - /** * User approval callback. The request can fail if the request is invalid. * @@ -181,6 +161,65 @@ export class PermissionsController { delete this.pendingApprovals[id] } + /** + * Finalizes a permissions request. + * Throws if request validation fails. + * + * @param {Object} requestedPermissions - The requested permissions. + * @param {string[]} accounts - The accounts to expose, if any. + */ + async finalizePermissionsRequest (requestedPermissions, accounts) { + + const { eth_accounts: ethAccounts } = requestedPermissions + + if (ethAccounts) { + + await this.validatePermittedAccounts(accounts) + + if (!ethAccounts.caveats) { + ethAccounts.caveats = [] + } + + // caveat names are unique, and we will only construct this caveat here + ethAccounts.caveats = ethAccounts.caveats.filter(c => ( + c.name !== CAVEAT_NAMES.exposedAccounts + )) + + ethAccounts.caveats.push( + { + type: 'filterResponse', + value: accounts, + name: CAVEAT_NAMES.exposedAccounts, + }, + ) + } + } + + /** + * Removes the given permissions for the given domain. + * @param {object} domains { origin: [permissions] } + */ + removePermissionsFor (domains) { + + Object.entries(domains).forEach(([origin, perms]) => { + + this.permissions.removePermissionsFor( + origin, + perms.map(methodName => { + + if (methodName === 'eth_accounts') { + this.notifyDomain( + origin, + { method: NOTIFICATION_NAMES.accountsChanged, result: [] } + ) + } + + return { parentCapability: methodName } + }) + ) + }) + } + /** * 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, @@ -222,74 +261,44 @@ export class PermissionsController { } /** - * Update the accounts exposed to the given origin. + * Update the accounts exposed to the given origin. Changes the eth_accounts + * permissions and emits accountsChanged. + * At least one account must be exposed. If no accounts are to be exposed, the + * eth_accounts permissions should be removed completely. + * * Throws error if the update fails. * * @param {string} origin - The origin to change the exposed accounts for. * @param {string[]} accounts - The new account(s) to expose. */ - async updateExposedAccounts (origin, accounts) { + async updatePermittedAccounts (origin, accounts) { - await this.validateExposedAccounts(accounts) + await this.validatePermittedAccounts(accounts) this.permissions.updateCaveatFor( origin, 'eth_accounts', CAVEAT_NAMES.exposedAccounts, accounts ) this.notifyDomain(origin, { - method: ACCOUNTS_CHANGED_NOTIFICATION, + method: NOTIFICATION_NAMES.accountsChanged, result: accounts, }) } - /** - * Finalizes a permissions request. - * Throws if request validation fails. - * - * @param {Object} requestedPermissions - The requested permissions. - * @param {string[]} accounts - The accounts to expose, if any. - */ - async finalizePermissionsRequest (requestedPermissions, accounts) { - - const { eth_accounts: ethAccounts } = requestedPermissions - - if (ethAccounts) { - - await this.validateExposedAccounts(accounts) - - if (!ethAccounts.caveats) { - ethAccounts.caveats = [] - } - - // caveat names are unique, and we will only construct this caveat here - ethAccounts.caveats = ethAccounts.caveats.filter(c => ( - c.name !== CAVEAT_NAMES.exposedAccounts - )) - - ethAccounts.caveats.push( - { - type: 'filterResponse', - value: accounts, - name: CAVEAT_NAMES.exposedAccounts, - }, - ) - } - } - /** * Validate an array of accounts representing accounts to be exposed * to a domain. Throws error if validation fails. * * @param {string[]} accounts - An array of addresses. */ - async validateExposedAccounts (accounts) { + async validatePermittedAccounts (accounts) { if (!Array.isArray(accounts) || accounts.length === 0) { throw new Error('Must provide non-empty array of account(s).') } // assert accounts exist - const allAccounts = await this.keyringController.getAccounts() + const allAccounts = await this.getKeyringAccounts() accounts.forEach(acc => { if (!allAccounts.includes(acc)) { throw new Error(`Unknown account: ${acc}`) @@ -297,29 +306,50 @@ export class PermissionsController { }) } + notifyDomain (origin, payload) { + + // if the accounts changed from the perspective of the dapp, + // update "last seen" time for the origin and account(s) + // exception: no accounts -> no times to update + if ( + payload.method === NOTIFICATION_NAMES.accountsChanged && + Array.isArray(payload.result) + ) { + this.permissionsLogController.updateAccountsHistory( + origin, payload.result + ) + } + + this._notifyDomain(origin, payload) + + // NOTE: + // we don't check for accounts changing in the notifyAllDomains case, + // because the log only records when accounts were last seen, + // and the accounts only change for all domains at once when permissions + // are removed + } + /** - * Removes the given permissions for the given domain. - * @param {Object} domains { origin: [permissions] } + * When a new account is selected in the UI for 'origin', emit accountsChanged + * to 'origin' if the selected account is permitted. + * @param {string} origin - The origin. + * @param {string} account - The newly selected account's address. */ - removePermissionsFor (domains) { + async handleNewAccountSelected (origin, account) { - Object.entries(domains).forEach(([origin, perms]) => { + const permittedAccounts = await this.getAccounts(origin) - this.permissions.removePermissionsFor( - origin, - perms.map(methodName => { + if (!account || !permittedAccounts.includes(account)) { + return + } - if (methodName === 'eth_accounts') { - this.notifyDomain( - origin, - { method: ACCOUNTS_CHANGED_NOTIFICATION, result: [] } - ) - } + const newPermittedAccounts = [account].concat( + permittedAccounts.filter(_account => _account !== account) + ) - return { parentCapability: methodName } - }) - ) - }) + // update permitted accounts to ensure that accounts are returned + // in the same order every time + this.updatePermittedAccounts(origin, newPermittedAccounts) } /** @@ -328,11 +358,36 @@ export class PermissionsController { clearPermissions () { this.permissions.clearDomains() this.notifyAllDomains({ - method: ACCOUNTS_CHANGED_NOTIFICATION, + method: NOTIFICATION_NAMES.accountsChanged, result: [], }) } + /** + * Submits a permissions request to rpc-cap. Internal, background use only. + * + * @param {string} origin - The origin string. + * @param {IRequestedPermissions} permissions - The requested permissions. + */ + _requestPermissions (origin, permissions) { + return new Promise((resolve, reject) => { + + const req = { method: 'wallet_requestPermissions', params: [permissions] } + const res = {} + this.permissions.providerMiddlewareFunction( + { origin }, req, res, () => {}, _end + ) + + function _end (err) { + if (err || res.error) { + reject(err || res.error) + } else { + resolve(res.result) + } + } + }) + } + /** * A convenience method for retrieving a login object * or creating a new one if needed. @@ -352,7 +407,7 @@ export class PermissionsController { safeMethods: SAFE_METHODS, // optional prefix for internal methods - methodPrefix: WALLET_METHOD_PREFIX, + methodPrefix: WALLET_PREFIX, restrictedMethods: this._restrictedMethods, @@ -379,5 +434,5 @@ export class PermissionsController { } export function addInternalMethodPrefix (method) { - return WALLET_METHOD_PREFIX + method + return WALLET_PREFIX + method } diff --git a/app/scripts/controllers/permissions/loggerMiddleware.js b/app/scripts/controllers/permissions/loggerMiddleware.js deleted file mode 100644 index 3c9325442581..000000000000 --- a/app/scripts/controllers/permissions/loggerMiddleware.js +++ /dev/null @@ -1,168 +0,0 @@ -import clone from 'clone' -import { isValidAddress } from 'ethereumjs-util' - -const LOG_LIMIT = 100 - -/** - * Create middleware for logging requests and responses to restricted and - * permissions-related methods. - */ -export default function createLoggerMiddleware ({ - walletPrefix, restrictedMethods, store, logStoreKey, historyStoreKey, ignoreMethods, -}) { - return (req, res, next, _end) => { - let activityEntry, requestedMethods - const { origin, method } = req - const isInternal = method.startsWith(walletPrefix) - if ((isInternal || restrictedMethods.includes(method)) && !ignoreMethods.includes(method)) { - activityEntry = logActivity(req, isInternal) - if (method === `${walletPrefix}requestPermissions`) { - requestedMethods = getRequestedMethods(req) - } - } else if (method === 'eth_requestAccounts') { - activityEntry = logActivity(req, isInternal) - requestedMethods = [ 'eth_accounts' ] - } else { - return next() - } - - next(cb => { - const time = Date.now() - addResponse(activityEntry, res, time) - if (!res.error && requestedMethods) { - logHistory(requestedMethods, origin, res.result, time, method === 'eth_requestAccounts') - } - cb() - }) - } - - function logActivity (request, isInternal) { - const activityEntry = { - id: request.id, - method: request.method, - methodType: isInternal ? 'internal' : 'restricted', - origin: request.origin, - request: cloneObj(request), - requestTime: Date.now(), - response: null, - responseTime: null, - success: null, - } - commitActivity(activityEntry) - return activityEntry - } - - function addResponse (activityEntry, response, time) { - if (!response) { - return - } - activityEntry.response = cloneObj(response) - activityEntry.responseTime = time - activityEntry.success = !response.error - } - - function commitActivity (entry) { - const logs = store.getState()[logStoreKey] - if (logs.length > LOG_LIMIT - 2) { - logs.pop() - } - logs.push(entry) - store.updateState({ [logStoreKey]: logs }) - } - - function getRequestedMethods (request) { - if ( - !request.params || - typeof request.params[0] !== 'object' || - Array.isArray(request.params[0]) - ) { - return null - } - return Object.keys(request.params[0]) - } - - function logHistory (requestedMethods, origin, result, time, isEthRequestAccounts) { - let accounts, entries - if (isEthRequestAccounts) { - accounts = result - const accountToTimeMap = accounts.reduce((acc, account) => ({ ...acc, [account]: time }), {}) - entries = { 'eth_accounts': { accounts: accountToTimeMap, lastApproved: time } } - } else { - entries = result - ? result - .map(perm => { - if (perm.parentCapability === 'eth_accounts') { - accounts = getAccountsFromPermission(perm) - } - return perm.parentCapability - }) - .reduce((acc, m) => { - if (requestedMethods.includes(m)) { - if (m === 'eth_accounts') { - const accountToTimeMap = accounts.reduce((acc, account) => ({ ...acc, [account]: time }), {}) - acc[m] = { lastApproved: time, accounts: accountToTimeMap } - } else { - acc[m] = { lastApproved: time } - } - } - return acc - }, {}) - : {} - } - - if (Object.keys(entries).length > 0) { - commitHistory(origin, entries) - } - } - - function commitHistory (origin, entries) { - const history = store.getState()[historyStoreKey] || {} - const newOriginHistory = { - ...history[origin], - ...entries, - } - - if (history[origin] && history[origin]['eth_accounts'] && entries['eth_accounts']) { - newOriginHistory['eth_accounts'] = { - lastApproved: entries['eth_accounts'].lastApproved, - accounts: { - ...history[origin]['eth_accounts'].accounts, - ...entries['eth_accounts'].accounts, - }, - } - } - - history[origin] = newOriginHistory - - store.updateState({ [historyStoreKey]: history }) - } -} - -// the call to clone is set to disallow circular references -// we attempt cloning at a depth of 3 and 2, then return a -// shallow copy of the object -function cloneObj (obj) { - for (let i = 3; i > 1; i--) { - try { - return clone(obj, false, i) - } catch (_) {} - } - return { ...obj } -} - -function getAccountsFromPermission (perm) { - if (perm.parentCapability !== 'eth_accounts' || !perm.caveats) { - return [] - } - const accounts = {} - for (const c of perm.caveats) { - if (c.type === 'filterResponse' && Array.isArray(c.value)) { - for (const v of c.value) { - if (isValidAddress(v)) { - accounts[v] = true - } - } - } - } - return Object.keys(accounts) -} diff --git a/app/scripts/controllers/permissions/permissions-safe-methods.json b/app/scripts/controllers/permissions/permissions-safe-methods.json deleted file mode 100644 index 780a52ede7f4..000000000000 --- a/app/scripts/controllers/permissions/permissions-safe-methods.json +++ /dev/null @@ -1,51 +0,0 @@ -[ - "web3_sha3", - "net_listening", - "net_peerCount", - "net_version", - "eth_blockNumber", - "eth_call", - "eth_chainId", - "eth_coinbase", - "eth_estimateGas", - "eth_gasPrice", - "eth_getBalance", - "eth_getBlockByHash", - "eth_getBlockByNumber", - "eth_getBlockTransactionCountByHash", - "eth_getBlockTransactionCountByNumber", - "eth_getCode", - "eth_getFilterChanges", - "eth_getFilterLogs", - "eth_getLogs", - "eth_getStorageAt", - "eth_getTransactionByBlockHashAndIndex", - "eth_getTransactionByBlockNumberAndIndex", - "eth_getTransactionByHash", - "eth_getTransactionCount", - "eth_getTransactionReceipt", - "eth_getUncleByBlockHashAndIndex", - "eth_getUncleByBlockNumberAndIndex", - "eth_getUncleCountByBlockHash", - "eth_getUncleCountByBlockNumber", - "eth_getWork", - "eth_hashrate", - "eth_mining", - "eth_newBlockFilter", - "eth_newFilter", - "eth_newPendingTransactionFilter", - "eth_protocolVersion", - "eth_sendRawTransaction", - "eth_sendTransaction", - "eth_sign", - "personal_sign", - "eth_signTypedData", - "eth_signTypedData_v1", - "eth_signTypedData_v3", - "eth_submitHashrate", - "eth_submitWork", - "eth_syncing", - "eth_uninstallFilter", - "wallet_watchAsset", - "metamask_watchAsset" -] diff --git a/app/scripts/controllers/permissions/permissionsLog.js b/app/scripts/controllers/permissions/permissionsLog.js new file mode 100644 index 000000000000..7bfb735707eb --- /dev/null +++ b/app/scripts/controllers/permissions/permissionsLog.js @@ -0,0 +1,309 @@ + +const clone = require('clone') +const { isValidAddress } = require('ethereumjs-util') +const { CAVEAT_NAMES } = require('./enums') + +const LOG_LIMIT = 100 + +const getAccountToTimeMap = (accounts, time) => accounts.reduce( + (acc, account) => ({ ...acc, [account]: time }), {} +) + +/** + * Create middleware for logging requests and responses to restricted and + * permissions-related methods. + */ +class PermissionsLogController { + + constructor ({ + walletPrefix, restrictedMethods, store, + logStoreKey, historyStoreKey, ignoreMethods, + }) { + this.walletPrefix = walletPrefix + this.restrictedMethods = restrictedMethods + this.store = store + this.logStoreKey = logStoreKey + this.historyStoreKey = historyStoreKey + this.ignoreMethods = ignoreMethods + } + + getLogStore () { + return this.store.getState()[this.logStoreKey] || [] + } + + updateLogStore (logs) { + this.store.updateState({ [this.logStoreKey]: logs }) + } + + getHistoryStore () { + return this.store.getState()[this.logStoreKey] || {} + } + + updateHistoryStore (history) { + this.store.updateState({ [this.historyStoreKey]: history }) + } + + createMiddleware () { + return (req, res, next, _end) => { + + let activityEntry, requestedMethods + const { origin, method } = req + const isInternal = method.startsWith(this.walletPrefix) + + // we only log certain methods + if ( + (isInternal || this.restrictedMethods.includes(method)) && + !this.ignoreMethods.includes(method) + ) { + + activityEntry = this.logActivity(req, isInternal) + + if (method === `${this.walletPrefix}requestPermissions`) { + // get the corresponding methods from the requested permissions + requestedMethods = this.getRequestedMethods(req) + } + } else if (method === 'eth_requestAccounts') { + + // eth_requestAccounts is a special case; we need to extract the accounts + // from it + activityEntry = this.logActivity(req, isInternal) + requestedMethods = [ 'eth_accounts' ] + } else { + // no-op + return next() + } + + // call next with a return handler for capturing the response + next(cb => { + + const time = Date.now() + this.addResponse(activityEntry, res, time) + + if (!res.error && requestedMethods) { + // any permissions or accounts changes will be recorded on the response, + // so we only log permissions history here + this.logPermissionsHistory( + requestedMethods, origin, res.result, time, + method === 'eth_requestAccounts', + ) + } + cb() + }) + } + } + + // creates and commits an activity log entry, without response data + logActivity (request, isInternal) { + const activityEntry = { + id: request.id, + method: request.method, + methodType: isInternal ? 'internal' : 'restricted', + origin: request.origin, + request: cloneObj(request), + requestTime: Date.now(), + response: null, + responseTime: null, + success: null, + } + this.commitActivity(activityEntry) + return activityEntry + } + + // adds response data to an activity entry + addResponse (activityEntry, response, time) { + if (!response) { + return + } + activityEntry.response = cloneObj(response) + activityEntry.responseTime = time + activityEntry.success = !response.error + } + + // get requested methods from a permissions request + getRequestedMethods (request) { + if ( + !request.params || + typeof request.params[0] !== 'object' || + Array.isArray(request.params[0]) + ) { + return null + } + return Object.keys(request.params[0]) + } + + + getAccountsFromPermission (perm) { + + if (perm.parentCapability !== 'eth_accounts' || !perm.caveats) { + return [] + } + + const accounts = {} + for (const c of perm.caveats) { + if (c.name === CAVEAT_NAMES.exposedAccounts && Array.isArray(c.value)) { + for (const v of c.value) { + if (isValidAddress(v)) { + accounts[v] = true + } + } + } + } + return Object.keys(accounts) + } + + // commit an activity entry to the log + commitActivity (entry) { + + const logs = this.getLogStore() + + // add new entry to end of log + logs.push(entry) + + // remove oldest log if exceeding size limit + if (logs.length > LOG_LIMIT) { + logs.shift() + } + + this.updateLogStore() + } + + /** + * Create new permissions history log entries, if any, and commit them. + * + * @param {Array} requestedMethods - The method names corresponding to the requested permissions. + * @param {string} origin - The origin of the permissions request. + * @param {Array { + + if (perm.parentCapability === 'eth_accounts') { + accounts = this.getAccountsFromPermission(perm) + } + + return perm.parentCapability + }) + .reduce((acc, method) => { + + if (requestedMethods.includes(method)) { + + if (method === 'eth_accounts') { + + const accountToTimeMap = getAccountToTimeMap(accounts, time) + + acc[method] = { + lastApproved: time, + accounts: accountToTimeMap, + } + + } else { + acc[method] = { lastApproved: time } + } + } + + return acc + }, {}) + : {} // no result (e.g. in case of error), no log + } + + if (Object.keys(newEntries).length > 0) { + this.commitHistory(origin, newEntries) + } + } + + updateAccountsHistory (origin, accounts) { + + if (accounts.length === 0) { + return + } + + const accountToTimeMap = getAccountToTimeMap(accounts, Date.now()) + + this.commitHistory(origin, { + eth_accounts: { + accounts: accountToTimeMap, + }, + }) + } + + // commit a history log entry + commitHistory (origin, newEntries) { + + // a simple merge updates most permissions + const history = this.getHistoryStore() + const newOriginHistory = { + ...history[origin], + ...newEntries, + } + + // eth_accounts requires special handling, because of information + // we store about the accounts + const existingEthAccountsEntry = ( + history[origin] && history[origin]['eth_accounts'] + ) + const newEthAccountsEntry = newEntries['eth_accounts'] + if (existingEthAccountsEntry && newEthAccountsEntry) { + + // we may intend to update just the accounts, not the permission + // itself + const lastApproved = ( + newEthAccountsEntry.lastApproved || + existingEthAccountsEntry.lastApproved + ) + + // merge old and new eth_accounts history entries + newOriginHistory['eth_accounts'] = { + lastApproved, + accounts: { + ...existingEthAccountsEntry.accounts, + ...newEthAccountsEntry.accounts, + }, + } + } + + history[origin] = newOriginHistory + + this.updateHistoryStore(history) + } +} + +// helper functions + +// the call to clone is set to disallow circular references +// we attempt cloning at a depth of 3 and 2, then return a +// shallow copy of the object +function cloneObj (obj) { + + for (let i = 3; i > 1; i--) { + try { + return clone(obj, false, i) + } catch (_) {} + } + return { ...obj } +} + +module.exports = PermissionsLogController diff --git a/app/scripts/controllers/permissions/restrictedMethods.js b/app/scripts/controllers/permissions/restrictedMethods.js index 14d38b05c8ee..b77336cfe993 100644 --- a/app/scripts/controllers/permissions/restrictedMethods.js +++ b/app/scripts/controllers/permissions/restrictedMethods.js @@ -4,7 +4,7 @@ export default function getRestrictedMethods (permissionsController) { 'eth_accounts': { description: 'View the address of the selected account', method: (_, res, __, end) => { - permissionsController.keyringController.getAccounts() + permissionsController.getKeyringAccounts() .then((accounts) => { res.result = accounts end() diff --git a/app/scripts/controllers/preferences.js b/app/scripts/controllers/preferences.js index 76bb242f2443..7c8c713a10f3 100644 --- a/app/scripts/controllers/preferences.js +++ b/app/scripts/controllers/preferences.js @@ -61,6 +61,8 @@ class PreferencesController { // ENS decentralized website resolution ipfsGateway: 'ipfs.dweb.link', + + selectedAddressHistory: {}, }, opts.initState) this.diagnostics = opts.diagnostics @@ -369,6 +371,22 @@ class PreferencesController { return this.store.getState().selectedAddress } + /** + * Update the last selected address for the given origin. + * @param {string} origin - The origin for which the address was selected. + * @param {string} address - The new selected address. + */ + updateSelectedAddressHistory (origin, address) { + + const { selectedAddressHistory } = this.store.getState() + + // only update state if it's necessary + if (selectedAddressHistory[origin] !== address) { + selectedAddressHistory[origin] = address + this.store.updateState({ selectedAddressHistory }) + } + } + /** * Contains data about tokens users add to their account. * @typedef {Object} AddedToken diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 6ce3e05be1c3..ef7be96ef558 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -205,7 +205,7 @@ export default class MetamaskController extends EventEmitter { this.keyringController.memStore.subscribe((s) => this._onKeyringControllerUpdate(s)) this.permissionsController = new PermissionsController({ - keyringController: this.keyringController, + getKeyringAccounts: this.keyringController.getAccounts.bind(this.keyringController), platform: opts.platform, notifyDomain: this.notifyConnections.bind(this), notifyAllDomains: this.notifyAllConnections.bind(this), @@ -557,8 +557,9 @@ export default class MetamaskController extends EventEmitter { getApprovedAccounts: nodeify(permissionsController.getAccounts.bind(permissionsController)), rejectPermissionsRequest: nodeify(permissionsController.rejectPermissionsRequest, permissionsController), removePermissionsFor: permissionsController.removePermissionsFor.bind(permissionsController), - updateExposedAccounts: nodeify(permissionsController.updateExposedAccounts, permissionsController), + updatePermittedAccounts: nodeify(permissionsController.updatePermittedAccounts, permissionsController), legacyExposeAccounts: nodeify(permissionsController.legacyExposeAccounts, permissionsController), + handleNewAccountSelected: nodeify(permissionsController.handleNewAccountSelected, permissionsController), getRequestAccountTabIds: (cb) => cb(null, this.getRequestAccountTabIds()), getOpenMetamaskTabsIds: (cb) => cb(null, this.getOpenMetamaskTabsIds()), @@ -1020,6 +1021,18 @@ export default class MetamaskController extends EventEmitter { await this.preferencesController.setSelectedAddress(accounts[0]) } + /** + * Handle when a new account is selected for the given origin in the UI. + * Stores the address by origin and notifies external providers associated + * with the origin. + * @param {string} origin - The origin for which the address was selected. + * @param {string} address - The new selected address. + */ + async handleNewAccountSelected (origin, address) { + this.permissionsController.handleNewAccountSelected(origin, address) + this.preferencesController.updateSelectedAddressHistory(origin, address) + } + // --------------------------------------------------------------------------- // Identity Management (signature operations) diff --git a/package.json b/package.json index 5d2c3a90c633..15d9a2d9840b 100644 --- a/package.json +++ b/package.json @@ -173,7 +173,7 @@ "redux-thunk": "^2.2.0", "request-promise": "^4.2.1", "reselect": "^3.0.1", - "rpc-cap": "^1.0.1", + "rpc-cap": "^1.0.3", "safe-event-emitter": "^1.0.1", "safe-json-stringify": "^1.2.0", "single-call-balance-checker-abi": "^1.0.0", diff --git a/ui/app/components/app/account-menu/account-menu.container.js b/ui/app/components/app/account-menu/account-menu.container.js index 7182550a5f08..669dade98891 100644 --- a/ui/app/components/app/account-menu/account-menu.container.js +++ b/ui/app/components/app/account-menu/account-menu.container.js @@ -17,6 +17,8 @@ import { getMetaMaskKeyrings, getOriginOfCurrentTab, getSelectedAddress, + // getLastSelectedAddress, + // getPermittedAccounts, } from '../../../selectors/selectors' import AccountMenu from './account-menu.component' @@ -28,12 +30,23 @@ const SHOW_SEARCH_ACCOUNTS_MIN_COUNT = 5 function mapStateToProps (state) { const { metamask: { isAccountMenuOpen } } = state const accounts = getMetaMaskAccountsOrdered(state) + const origin = getOriginOfCurrentTab(state) + const selectedAddress = getSelectedAddress(state) + + /** + * TODO:LoginPerSite + * - dispatch background.newAccountSelected() if selectedAddress !== lastSelectedAddress + * - propagate the relevant props below after computing them + */ + // const lastSelectedAddress = getLastSelectedAddress(state, origin) + // const permittedAccounts = getPermittedAccounts(state, origin) + // const selectedAccountIsPermitted = permittedAccounts.includes(selectedAddress) return { isAccountMenuOpen, addressConnectedDomainMap: getAddressConnectedDomainMap(state), - originOfCurrentTab: getOriginOfCurrentTab(state), - selectedAddress: getSelectedAddress(state), + originOfCurrentTab: origin, + selectedAddress: selectedAddress, keyrings: getMetaMaskKeyrings(state), accounts, shouldShowAccountsSearch: accounts.length >= SHOW_SEARCH_ACCOUNTS_MIN_COUNT, diff --git a/ui/app/selectors/permissions.js b/ui/app/selectors/permissions.js new file mode 100644 index 000000000000..98f8111afd14 --- /dev/null +++ b/ui/app/selectors/permissions.js @@ -0,0 +1,65 @@ + +import { createSelector, createSelectorCreator, defaultMemoize } from 'reselect' +import deepEqual from 'fast-deep-equal' +import { + CAVEAT_NAMES, +} from '../../../app/scripts/controllers/permissions/enums' + +/** + * TODO:LoginPerSite + * There are performance gains here once `domain.permissions` is converted + * to key:value instead of an array. (requires update to rpc-cap) + */ + +const permissionsSelector = (state, origin) => { + console.log('permissionsSelector', origin) + return origin && state.metamask.domains && state.metamask.domains[origin] +} + +// all permissions for the origin probably too expensive for deep equality check +const accountsPermissionSelector = createSelector( + permissionsSelector, + (domain = {}) => { + + console.log('accountsPermissionSelector:domain', domain) + return ( + Array.isArray(domain.permissions) + ? domain.permissions.find( + perm => perm.parentCapability === 'eth_accounts' + ) + : {} + ) + } +) + +// comparing caveats should be cheap, at least for now +const createCaveatEqualSelector = createSelectorCreator( + defaultMemoize, + (a = {}, b = {}) => deepEqual(a.caveats, b.caveats) +) + +/** + * Selects the permitted accounts from an eth_accounts permission. + * Expects input from accountsPermissionsSelector. + * @returns - An empty array or an array of accounts. + */ +export const permittedAccountsSelector = createCaveatEqualSelector( + accountsPermissionSelector, // deep equal check performed on this output + (accountsPermission = {}) => { + + console.log('permittedAccountsSelector:accountsPermission', accountsPermission) + const accountsCaveat = ( + Array.isArray(accountsPermission.caveats) && + accountsPermission.caveats.find( + c => c.name === CAVEAT_NAMES.exposedAccounts + ) + ) + + console.log('permittedAccountsSelector:accountsCaveat', accountsCaveat) + return ( + accountsCaveat && Array.isArray(accountsCaveat.value) + ? accountsCaveat.value + : [] + ) + } +) diff --git a/ui/app/selectors/selectors.js b/ui/app/selectors/selectors.js index 4b79cdf79bac..48a3bfcfc4f8 100644 --- a/ui/app/selectors/selectors.js +++ b/ui/app/selectors/selectors.js @@ -11,6 +11,7 @@ import { formatDate, getOriginFromUrl, } from '../helpers/utils/util' +import { permittedAccountsSelector } from './permissions' export function getNetworkIdentifier (state) { const { metamask: { provider: { type, nickname, rpcTarget } } } = state @@ -87,6 +88,23 @@ export function getSelectedAddress (state) { return selectedAddress } +function lastSelectedAddressSelector (state, origin) { + return state.metamask.selectedAddressHistory[origin] || null +} + +// not using reselect here since the returns are contingent; +// we have no reasons to recompute the permitted accounts if there +// exists a lastSelectedAddress +export function getLastSelectedAddress (state, origin) { + return ( + lastSelectedAddressSelector(state, origin) || + permittedAccountsSelector(state, origin)[0] || // always returns array + getSelectedAddress(state) + ) +} + +export const getPermittedAccounts = (state, origin) => permittedAccountsSelector(state, origin) + export function getSelectedIdentity (state) { const selectedAddress = getSelectedAddress(state) const identities = state.metamask.identities diff --git a/ui/app/store/actions.js b/ui/app/store/actions.js index 6f1717afecad..c5ed76e909cb 100644 --- a/ui/app/store/actions.js +++ b/ui/app/store/actions.js @@ -1248,6 +1248,7 @@ export function showAccountDetail (address) { if (err) { return dispatch(displayWarning(err.message)) } + background.handleNewAccountSelected(origin, address) dispatch(updateTokens(tokens)) dispatch({ type: actionConstants.SHOW_ACCOUNT_DETAIL, diff --git a/yarn.lock b/yarn.lock index fa2607a44a54..6b35179b4177 100644 --- a/yarn.lock +++ b/yarn.lock @@ -24101,10 +24101,10 @@ rn-host-detect@^1.1.5: resolved "https://registry.yarnpkg.com/rn-host-detect/-/rn-host-detect-1.1.5.tgz#fbecb982b73932f34529e97932b9a63e58d8deb6" integrity sha512-ufk2dFT3QeP9HyZ/xTuMtW27KnFy815CYitJMqQm+pgG3ZAtHBsrU8nXizNKkqXGy3bQmhEoloVbrfbvMJMqkg== -rpc-cap@^1.0.1: - version "1.0.1" - resolved "https://registry.yarnpkg.com/rpc-cap/-/rpc-cap-1.0.1.tgz#c19f6651d9d003256c73831422e0bd60b4fa8b55" - integrity sha512-M75F5IfohYkwGvitWmstimP9OL+9h10m1ZRC2zCB1Nli4EPzL8n5re58xlrcOnwOO38FdSSPfcwcCzMuVT8K2g== +rpc-cap@^1.0.3: + version "1.0.3" + resolved "https://registry.yarnpkg.com/rpc-cap/-/rpc-cap-1.0.3.tgz#c58f99ee97a92441f4310f407c0f40fecdbf0e78" + integrity sha512-6lheD7UU4IY+OpILTL65E5NQWFPfG1Igd/CAGbnMJY+3szmQ9mUrf4/3bbcvNhu64Q/KYfCstVhxJREmTeFLOg== dependencies: clone "^2.1.2" eth-json-rpc-errors "^2.0.0" From a1c79218dc5d59363e8fa2f149e4e26f08da09b7 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Mon, 16 Dec 2019 14:36:20 -0800 Subject: [PATCH 02/11] undo perm controller reorganization --- app/scripts/controllers/permissions/index.js | 168 +++++++++---------- 1 file changed, 84 insertions(+), 84 deletions(-) diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index 40258f83d0a4..35b53cd33962 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -111,6 +111,31 @@ export class PermissionsController { }) } + /** + * Submits a permissions request to rpc-cap. Internal, background use only. + * + * @param {string} origin - The origin string. + * @param {IRequestedPermissions} permissions - The requested permissions. + */ + _requestPermissions (origin, permissions) { + return new Promise((resolve, reject) => { + + const req = { method: 'wallet_requestPermissions', params: [permissions] } + const res = {} + this.permissions.providerMiddlewareFunction( + { origin }, req, res, () => {}, _end + ) + + function _end (err) { + if (err || res.error) { + reject(err || res.error) + } else { + resolve(res.result) + } + } + }) + } + /** * User approval callback. The request can fail if the request is invalid. * @@ -161,65 +186,6 @@ export class PermissionsController { delete this.pendingApprovals[id] } - /** - * Finalizes a permissions request. - * Throws if request validation fails. - * - * @param {Object} requestedPermissions - The requested permissions. - * @param {string[]} accounts - The accounts to expose, if any. - */ - async finalizePermissionsRequest (requestedPermissions, accounts) { - - const { eth_accounts: ethAccounts } = requestedPermissions - - if (ethAccounts) { - - await this.validatePermittedAccounts(accounts) - - if (!ethAccounts.caveats) { - ethAccounts.caveats = [] - } - - // caveat names are unique, and we will only construct this caveat here - ethAccounts.caveats = ethAccounts.caveats.filter(c => ( - c.name !== CAVEAT_NAMES.exposedAccounts - )) - - ethAccounts.caveats.push( - { - type: 'filterResponse', - value: accounts, - name: CAVEAT_NAMES.exposedAccounts, - }, - ) - } - } - - /** - * Removes the given permissions for the given domain. - * @param {object} domains { origin: [permissions] } - */ - removePermissionsFor (domains) { - - Object.entries(domains).forEach(([origin, perms]) => { - - this.permissions.removePermissionsFor( - origin, - perms.map(methodName => { - - if (methodName === 'eth_accounts') { - this.notifyDomain( - origin, - { method: NOTIFICATION_NAMES.accountsChanged, result: [] } - ) - } - - return { parentCapability: methodName } - }) - ) - }) - } - /** * 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, @@ -285,6 +251,40 @@ export class PermissionsController { }) } + /** + * Finalizes a permissions request. + * Throws if request validation fails. + * + * @param {Object} requestedPermissions - The requested permissions. + * @param {string[]} accounts - The accounts to expose, if any. + */ + async finalizePermissionsRequest (requestedPermissions, accounts) { + + const { eth_accounts: ethAccounts } = requestedPermissions + + if (ethAccounts) { + + await this.validatePermittedAccounts(accounts) + + if (!ethAccounts.caveats) { + ethAccounts.caveats = [] + } + + // caveat names are unique, and we will only construct this caveat here + ethAccounts.caveats = ethAccounts.caveats.filter(c => ( + c.name !== CAVEAT_NAMES.exposedAccounts + )) + + ethAccounts.caveats.push( + { + type: 'filterResponse', + value: accounts, + name: CAVEAT_NAMES.exposedAccounts, + }, + ) + } + } + /** * Validate an array of accounts representing accounts to be exposed * to a domain. Throws error if validation fails. @@ -329,6 +329,31 @@ export class PermissionsController { // are removed } + /** + * Removes the given permissions for the given domain. + * @param {object} domains { origin: [permissions] } + */ + removePermissionsFor (domains) { + + Object.entries(domains).forEach(([origin, perms]) => { + + this.permissions.removePermissionsFor( + origin, + perms.map(methodName => { + + if (methodName === 'eth_accounts') { + this.notifyDomain( + origin, + { method: NOTIFICATION_NAMES.accountsChanged, result: [] } + ) + } + + return { parentCapability: methodName } + }) + ) + }) + } + /** * When a new account is selected in the UI for 'origin', emit accountsChanged * to 'origin' if the selected account is permitted. @@ -363,31 +388,6 @@ export class PermissionsController { }) } - /** - * Submits a permissions request to rpc-cap. Internal, background use only. - * - * @param {string} origin - The origin string. - * @param {IRequestedPermissions} permissions - The requested permissions. - */ - _requestPermissions (origin, permissions) { - return new Promise((resolve, reject) => { - - const req = { method: 'wallet_requestPermissions', params: [permissions] } - const res = {} - this.permissions.providerMiddlewareFunction( - { origin }, req, res, () => {}, _end - ) - - function _end (err) { - if (err || res.error) { - reject(err || res.error) - } else { - resolve(res.result) - } - } - }) - } - /** * A convenience method for retrieving a login object * or creating a new one if needed. From cdbbaa1b6f1f0436c40dd0c0272e1eb8dc1dd166 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 15 Jan 2020 13:38:31 -0800 Subject: [PATCH 03/11] fixup! undo perm controller reorganization --- app/scripts/controllers/permissions/enums.js | 2 +- app/scripts/controllers/permissions/index.js | 4 ---- app/scripts/controllers/permissions/permissionsLog.js | 10 ++++------ 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/app/scripts/controllers/permissions/enums.js b/app/scripts/controllers/permissions/enums.js index ef0d883d017c..db97de465fa6 100644 --- a/app/scripts/controllers/permissions/enums.js +++ b/app/scripts/controllers/permissions/enums.js @@ -1,4 +1,4 @@ -module.exports = { +export default { WALLET_PREFIX: 'wallet_', HISTORY_STORE_KEY: 'permissionsHistory', LOG_STORE_KEY: 'permissionsLog', diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index 35b53cd33962..dd569a454434 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -20,10 +20,6 @@ import { NOTIFICATION_NAMES, } from './enums' -export const CAVEAT_NAMES = { - exposedAccounts: 'exposedAccounts', -} - export class PermissionsController { constructor ( diff --git a/app/scripts/controllers/permissions/permissionsLog.js b/app/scripts/controllers/permissions/permissionsLog.js index 7bfb735707eb..e56eb983fe99 100644 --- a/app/scripts/controllers/permissions/permissionsLog.js +++ b/app/scripts/controllers/permissions/permissionsLog.js @@ -1,7 +1,7 @@ -const clone = require('clone') -const { isValidAddress } = require('ethereumjs-util') -const { CAVEAT_NAMES } = require('./enums') +import clone from 'clone' +import { isValidAddress } from 'ethereumjs-util' +import { CAVEAT_NAMES } from './enums' const LOG_LIMIT = 100 @@ -13,7 +13,7 @@ const getAccountToTimeMap = (accounts, time) => accounts.reduce( * Create middleware for logging requests and responses to restricted and * permissions-related methods. */ -class PermissionsLogController { +export default class PermissionsLogController { constructor ({ walletPrefix, restrictedMethods, store, @@ -305,5 +305,3 @@ function cloneObj (obj) { } return { ...obj } } - -module.exports = PermissionsLogController From f4597b7bd17b7387e20d4bc215a3f7585568c688 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 15 Jan 2020 13:50:44 -0800 Subject: [PATCH 04/11] fixup! fixup! undo perm controller reorganization --- app/scripts/controllers/permissions/enums.js | 125 ++++++++++--------- 1 file changed, 65 insertions(+), 60 deletions(-) diff --git a/app/scripts/controllers/permissions/enums.js b/app/scripts/controllers/permissions/enums.js index db97de465fa6..c9dad1e1cbf8 100644 --- a/app/scripts/controllers/permissions/enums.js +++ b/app/scripts/controllers/permissions/enums.js @@ -1,61 +1,66 @@ -export default { - WALLET_PREFIX: 'wallet_', - HISTORY_STORE_KEY: 'permissionsHistory', - LOG_STORE_KEY: 'permissionsLog', - METADATA_STORE_KEY: 'domainMetadata', - CAVEAT_NAMES: { - exposedAccounts: 'exposedAccounts', - }, - NOTIFICATION_NAMES: { - accountsChanged: 'wallet_accountsChanged', - }, - SAFE_METHODS: [ - 'web3_sha3', - 'net_listening', - 'net_peerCount', - 'net_version', - 'eth_blockNumber', - 'eth_call', - 'eth_chainId', - 'eth_coinbase', - 'eth_estimateGas', - 'eth_gasPrice', - 'eth_getBalance', - 'eth_getBlockByHash', - 'eth_getBlockByNumber', - 'eth_getBlockTransactionCountByHash', - 'eth_getBlockTransactionCountByNumber', - 'eth_getCode', - 'eth_getFilterChanges', - 'eth_getFilterLogs', - 'eth_getLogs', - 'eth_getStorageAt', - 'eth_getTransactionByBlockHashAndIndex', - 'eth_getTransactionByBlockNumberAndIndex', - 'eth_getTransactionByHash', - 'eth_getTransactionCount', - 'eth_getTransactionReceipt', - 'eth_getUncleByBlockHashAndIndex', - 'eth_getUncleByBlockNumberAndIndex', - 'eth_getUncleCountByBlockHash', - 'eth_getUncleCountByBlockNumber', - 'eth_getWork', - 'eth_hashrate', - 'eth_mining', - 'eth_newBlockFilter', - 'eth_newFilter', - 'eth_newPendingTransactionFilter', - 'eth_protocolVersion', - 'eth_sendRawTransaction', - 'eth_sendTransaction', - 'eth_sign', - 'personal_sign', - 'eth_signTypedData', - 'eth_signTypedData_v1', - 'eth_signTypedData_v3', - 'eth_submitHashrate', - 'eth_submitWork', - 'eth_syncing', - 'eth_uninstallFilter', - ], + +export const WALLET_PREFIX = 'wallet_' + +export const HISTORY_STORE_KEY = 'permissionsHistory' + +export const LOG_STORE_KEY = 'permissionsLog' + +export const METADATA_STORE_KEY = 'domainMetadata' + +export const CAVEAT_NAMES = { + exposedAccounts: 'exposedAccounts', } + +export const NOTIFICATION_NAMES = { + accountsChanged: 'wallet_accountsChanged', +} + +export const SAFE_METHODS = [ + 'web3_sha3', + 'net_listening', + 'net_peerCount', + 'net_version', + 'eth_blockNumber', + 'eth_call', + 'eth_chainId', + 'eth_coinbase', + 'eth_estimateGas', + 'eth_gasPrice', + 'eth_getBalance', + 'eth_getBlockByHash', + 'eth_getBlockByNumber', + 'eth_getBlockTransactionCountByHash', + 'eth_getBlockTransactionCountByNumber', + 'eth_getCode', + 'eth_getFilterChanges', + 'eth_getFilterLogs', + 'eth_getLogs', + 'eth_getStorageAt', + 'eth_getTransactionByBlockHashAndIndex', + 'eth_getTransactionByBlockNumberAndIndex', + 'eth_getTransactionByHash', + 'eth_getTransactionCount', + 'eth_getTransactionReceipt', + 'eth_getUncleByBlockHashAndIndex', + 'eth_getUncleByBlockNumberAndIndex', + 'eth_getUncleCountByBlockHash', + 'eth_getUncleCountByBlockNumber', + 'eth_getWork', + 'eth_hashrate', + 'eth_mining', + 'eth_newBlockFilter', + 'eth_newFilter', + 'eth_newPendingTransactionFilter', + 'eth_protocolVersion', + 'eth_sendRawTransaction', + 'eth_sendTransaction', + 'eth_sign', + 'personal_sign', + 'eth_signTypedData', + 'eth_signTypedData_v1', + 'eth_signTypedData_v3', + 'eth_submitHashrate', + 'eth_submitWork', + 'eth_syncing', + 'eth_uninstallFilter', +] From 95446d0d0e85b6b6dbaf29bf3e2328702f3574a8 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 15 Jan 2020 14:05:52 -0800 Subject: [PATCH 05/11] use enums for store keys in log controller --- app/scripts/controllers/permissions/index.js | 2 -- .../controllers/permissions/permissionsLog.js | 19 ++++++++++--------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index dd569a454434..d9d68aee62bd 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -45,8 +45,6 @@ export class PermissionsController { restrictedMethods: Object.keys(this._restrictedMethods), ignoreMethods: [ 'wallet_sendDomainMetadata' ], store: this.store, - logStoreKey: LOG_STORE_KEY, - historyStoreKey: HISTORY_STORE_KEY, }) this._initializePermissions(restoredPermissions) } diff --git a/app/scripts/controllers/permissions/permissionsLog.js b/app/scripts/controllers/permissions/permissionsLog.js index e56eb983fe99..9bbac3764708 100644 --- a/app/scripts/controllers/permissions/permissionsLog.js +++ b/app/scripts/controllers/permissions/permissionsLog.js @@ -1,7 +1,11 @@ import clone from 'clone' import { isValidAddress } from 'ethereumjs-util' -import { CAVEAT_NAMES } from './enums' +import { + CAVEAT_NAMES, + HISTORY_STORE_KEY, + LOG_STORE_KEY +} from './enums' const LOG_LIMIT = 100 @@ -16,31 +20,28 @@ const getAccountToTimeMap = (accounts, time) => accounts.reduce( export default class PermissionsLogController { constructor ({ - walletPrefix, restrictedMethods, store, - logStoreKey, historyStoreKey, ignoreMethods, + walletPrefix, restrictedMethods, store, ignoreMethods }) { this.walletPrefix = walletPrefix this.restrictedMethods = restrictedMethods this.store = store - this.logStoreKey = logStoreKey - this.historyStoreKey = historyStoreKey this.ignoreMethods = ignoreMethods } getLogStore () { - return this.store.getState()[this.logStoreKey] || [] + return this.store.getState()[LOG_STORE_KEY] || [] } updateLogStore (logs) { - this.store.updateState({ [this.logStoreKey]: logs }) + this.store.updateState({ [LOG_STORE_KEY]: logs }) } getHistoryStore () { - return this.store.getState()[this.logStoreKey] || {} + return this.store.getState()[HISTORY_STORE_KEY] || {} } updateHistoryStore (history) { - this.store.updateState({ [this.historyStoreKey]: history }) + this.store.updateState({ [HISTORY_STORE_KEY]: history }) } createMiddleware () { From 24302d477d40d257e76b07e1d5799c0c3270a23d Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 15 Jan 2020 18:35:45 -0800 Subject: [PATCH 06/11] fixup! use enums for store keys in log controller --- app/scripts/controllers/permissions/permissionsLog.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/scripts/controllers/permissions/permissionsLog.js b/app/scripts/controllers/permissions/permissionsLog.js index 9bbac3764708..3c96debbc041 100644 --- a/app/scripts/controllers/permissions/permissionsLog.js +++ b/app/scripts/controllers/permissions/permissionsLog.js @@ -4,7 +4,7 @@ import { isValidAddress } from 'ethereumjs-util' import { CAVEAT_NAMES, HISTORY_STORE_KEY, - LOG_STORE_KEY + LOG_STORE_KEY, } from './enums' const LOG_LIMIT = 100 @@ -20,7 +20,7 @@ const getAccountToTimeMap = (accounts, time) => accounts.reduce( export default class PermissionsLogController { constructor ({ - walletPrefix, restrictedMethods, store, ignoreMethods + walletPrefix, restrictedMethods, store, ignoreMethods, }) { this.walletPrefix = walletPrefix this.restrictedMethods = restrictedMethods From 890fc36621d0aa0877dc20e60358b07d42334521 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 15 Jan 2020 18:44:14 -0800 Subject: [PATCH 07/11] fixup! fixup! use enums for store keys in log controller --- app/scripts/controllers/permissions/enums.js | 4 ++++ app/scripts/controllers/permissions/index.js | 2 -- .../controllers/permissions/permissionsLog.js | 16 +++++++--------- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/app/scripts/controllers/permissions/enums.js b/app/scripts/controllers/permissions/enums.js index c9dad1e1cbf8..4d26ce83f25f 100644 --- a/app/scripts/controllers/permissions/enums.js +++ b/app/scripts/controllers/permissions/enums.js @@ -15,6 +15,10 @@ export const NOTIFICATION_NAMES = { accountsChanged: 'wallet_accountsChanged', } +export const LOG_IGNORE_METHODS = [ + 'wallet_sendDomainMetadata', +] + export const SAFE_METHODS = [ 'web3_sha3', 'net_listening', diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index d9d68aee62bd..96233915228e 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -41,9 +41,7 @@ export class PermissionsController { this._platform = platform this._restrictedMethods = getRestrictedMethods(this) this.permissionsLogController = new PermissionsLogController({ - walletPrefix: WALLET_PREFIX, restrictedMethods: Object.keys(this._restrictedMethods), - ignoreMethods: [ 'wallet_sendDomainMetadata' ], store: this.store, }) this._initializePermissions(restoredPermissions) diff --git a/app/scripts/controllers/permissions/permissionsLog.js b/app/scripts/controllers/permissions/permissionsLog.js index 3c96debbc041..b2885a40a239 100644 --- a/app/scripts/controllers/permissions/permissionsLog.js +++ b/app/scripts/controllers/permissions/permissionsLog.js @@ -5,6 +5,8 @@ import { CAVEAT_NAMES, HISTORY_STORE_KEY, LOG_STORE_KEY, + LOG_IGNORE_METHODS, + WALLET_PREFIX, } from './enums' const LOG_LIMIT = 100 @@ -19,13 +21,9 @@ const getAccountToTimeMap = (accounts, time) => accounts.reduce( */ export default class PermissionsLogController { - constructor ({ - walletPrefix, restrictedMethods, store, ignoreMethods, - }) { - this.walletPrefix = walletPrefix + constructor ({ restrictedMethods, store }) { this.restrictedMethods = restrictedMethods this.store = store - this.ignoreMethods = ignoreMethods } getLogStore () { @@ -49,17 +47,17 @@ export default class PermissionsLogController { let activityEntry, requestedMethods const { origin, method } = req - const isInternal = method.startsWith(this.walletPrefix) + const isInternal = method.startsWith(WALLET_PREFIX) // we only log certain methods if ( - (isInternal || this.restrictedMethods.includes(method)) && - !this.ignoreMethods.includes(method) + !LOG_IGNORE_METHODS.includes(method) && + (isInternal || this.restrictedMethods.includes(method)) ) { activityEntry = this.logActivity(req, isInternal) - if (method === `${this.walletPrefix}requestPermissions`) { + if (method === `${WALLET_PREFIX}requestPermissions`) { // get the corresponding methods from the requested permissions requestedMethods = this.getRequestedMethods(req) } From d935cb11261d2c51e1b9cc5f0b777f9c2571c5fc Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Thu, 16 Jan 2020 09:06:22 -0800 Subject: [PATCH 08/11] update TODOs --- .../components/app/account-menu/account-menu.container.js | 2 +- ui/app/selectors/permissions.js | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/ui/app/components/app/account-menu/account-menu.container.js b/ui/app/components/app/account-menu/account-menu.container.js index 669dade98891..fd1c589f1ce1 100644 --- a/ui/app/components/app/account-menu/account-menu.container.js +++ b/ui/app/components/app/account-menu/account-menu.container.js @@ -34,7 +34,7 @@ function mapStateToProps (state) { const selectedAddress = getSelectedAddress(state) /** - * TODO:LoginPerSite + * TODO:LoginPerSite:ui * - dispatch background.newAccountSelected() if selectedAddress !== lastSelectedAddress * - propagate the relevant props below after computing them */ diff --git a/ui/app/selectors/permissions.js b/ui/app/selectors/permissions.js index 98f8111afd14..bb9ff57beeaf 100644 --- a/ui/app/selectors/permissions.js +++ b/ui/app/selectors/permissions.js @@ -5,12 +5,6 @@ import { CAVEAT_NAMES, } from '../../../app/scripts/controllers/permissions/enums' -/** - * TODO:LoginPerSite - * There are performance gains here once `domain.permissions` is converted - * to key:value instead of an array. (requires update to rpc-cap) - */ - const permissionsSelector = (state, origin) => { console.log('permissionsSelector', origin) return origin && state.metamask.domains && state.metamask.domains[origin] From dd67f3477b73c00287319d4ae82a88a1b95bc964 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Mon, 27 Jan 2020 09:31:51 -0800 Subject: [PATCH 09/11] address review; fixup permissions log --- app/scripts/controllers/permissions/enums.js | 2 + app/scripts/controllers/permissions/index.js | 9 +- .../controllers/permissions/permissionsLog.js | 257 ++++++++++++------ app/scripts/metamask-controller.js | 2 +- .../account-menu/account-menu.container.js | 1 - ui/app/selectors/permissions.js | 6 +- ui/app/selectors/selectors.js | 9 +- 7 files changed, 194 insertions(+), 92 deletions(-) diff --git a/app/scripts/controllers/permissions/enums.js b/app/scripts/controllers/permissions/enums.js index 4d26ce83f25f..1e68eddbeb3c 100644 --- a/app/scripts/controllers/permissions/enums.js +++ b/app/scripts/controllers/permissions/enums.js @@ -67,4 +67,6 @@ export const SAFE_METHODS = [ 'eth_submitWork', 'eth_syncing', 'eth_uninstallFilter', + 'metamask_watchAsset', + 'wallet_watchAsset', ] diff --git a/app/scripts/controllers/permissions/index.js b/app/scripts/controllers/permissions/index.js index 96233915228e..ec45d9b2ed21 100644 --- a/app/scripts/controllers/permissions/index.js +++ b/app/scripts/controllers/permissions/index.js @@ -323,7 +323,7 @@ export class PermissionsController { /** * Removes the given permissions for the given domain. - * @param {object} domains { origin: [permissions] } + * @param {Object} domains { origin: [permissions] } */ removePermissionsFor (domains) { @@ -356,7 +356,12 @@ export class PermissionsController { const permittedAccounts = await this.getAccounts(origin) - if (!account || !permittedAccounts.includes(account)) { + // do nothing if the account is not permitted for the origin, or + // if it's already first in the array of permitted accounts + if ( + !account || !permittedAccounts.includes(account) || + permittedAccounts[0] === account + ) { return } diff --git a/app/scripts/controllers/permissions/permissionsLog.js b/app/scripts/controllers/permissions/permissionsLog.js index b2885a40a239..caa369a2e9e6 100644 --- a/app/scripts/controllers/permissions/permissionsLog.js +++ b/app/scripts/controllers/permissions/permissionsLog.js @@ -11,13 +11,9 @@ import { const LOG_LIMIT = 100 -const getAccountToTimeMap = (accounts, time) => accounts.reduce( - (acc, account) => ({ ...acc, [account]: time }), {} -) - /** - * Create middleware for logging requests and responses to restricted and - * permissions-related methods. + * Controller with middleware for logging requests and responses to restricted + * and permissions-related methods. */ export default class PermissionsLogController { @@ -26,27 +22,74 @@ export default class PermissionsLogController { this.store = store } - getLogStore () { + /** + * Get the activity log. + * + * @returns {Array} - The activity log. + */ + getActivityLog () { return this.store.getState()[LOG_STORE_KEY] || [] } - updateLogStore (logs) { + /** + * Update the activity log. + * + * @param {Array} logs - The new activity log array. + */ + updateActivityLog (logs) { this.store.updateState({ [LOG_STORE_KEY]: logs }) } - getHistoryStore () { + /** + * Get the permissions history log. + * + * @returns {Object} - The permissions history log. + */ + getHistory () { return this.store.getState()[HISTORY_STORE_KEY] || {} } - updateHistoryStore (history) { + /** + * Update the permissions history log. + * + * @param {Object} history - The new permissions history log object. + */ + updateHistory (history) { this.store.updateState({ [HISTORY_STORE_KEY]: history }) } + /** + * Updates the exposed account history for the given origin. + * Sets the 'last seen' time to Date.now() for the given accounts. + * + * @param {string} origin - The origin that the accounts are exposed to. + * @param {Array} accounts - The accounts. + */ + updateAccountsHistory (origin, accounts) { + + if (accounts.length === 0) { + return + } + + const accountToTimeMap = getAccountToTimeMap(accounts, Date.now()) + + this.commitNewHistory(origin, { + eth_accounts: { + accounts: accountToTimeMap, + }, + }) + } + + /** + * Create a permissions log middleware. + * + * @returns {JsonRpcEngineMiddleware} - The permissions log middleware. + */ createMiddleware () { return (req, res, next, _end) => { - let activityEntry, requestedMethods - const { origin, method } = req + let requestedMethods + const { origin, method, id: requestId } = req const isInternal = method.startsWith(WALLET_PREFIX) // we only log certain methods @@ -55,7 +98,7 @@ export default class PermissionsLogController { (isInternal || this.restrictedMethods.includes(method)) ) { - activityEntry = this.logActivity(req, isInternal) + this.logActivityRequest(req, isInternal) if (method === `${WALLET_PREFIX}requestPermissions`) { // get the corresponding methods from the requested permissions @@ -65,7 +108,7 @@ export default class PermissionsLogController { // eth_requestAccounts is a special case; we need to extract the accounts // from it - activityEntry = this.logActivity(req, isInternal) + this.logActivityRequest(req, isInternal) requestedMethods = [ 'eth_accounts' ] } else { // no-op @@ -76,7 +119,7 @@ export default class PermissionsLogController { next(cb => { const time = Date.now() - this.addResponse(activityEntry, res, time) + this.logActivityResponse(requestId, res, time) if (!res.error && requestedMethods) { // any permissions or accounts changes will be recorded on the response, @@ -91,8 +134,13 @@ export default class PermissionsLogController { } } - // creates and commits an activity log entry, without response data - logActivity (request, isInternal) { + /** + * Creates and commits an activity log entry, without response data. + * + * @param {Object} request - The request object. + * @param {boolean} isInternal - Whether the request is internal. + */ + logActivityRequest (request, isInternal) { const activityEntry = { id: request.id, method: request.method, @@ -104,56 +152,45 @@ export default class PermissionsLogController { responseTime: null, success: null, } - this.commitActivity(activityEntry) - return activityEntry + this.commitNewActivity(activityEntry) } - // adds response data to an activity entry - addResponse (activityEntry, response, time) { - if (!response) { + /** + * Adds response data to an existing activity log entry and re-commits it. + * + * @param {string} id - The original request id. + * @param {Object} response - The response object. + * @param {number} time - Output from Date.now() + */ + logActivityResponse (id, response, time) { + + if (!id || !response) { return } - activityEntry.response = cloneObj(response) - activityEntry.responseTime = time - activityEntry.success = !response.error - } - // get requested methods from a permissions request - getRequestedMethods (request) { - if ( - !request.params || - typeof request.params[0] !== 'object' || - Array.isArray(request.params[0]) - ) { - return null + const logs = this.getActivityLog() + const index = getLastIndexOfObjectArray(logs, 'id', id) + if (index === -1) { + return } - return Object.keys(request.params[0]) - } - - - getAccountsFromPermission (perm) { - if (perm.parentCapability !== 'eth_accounts' || !perm.caveats) { - return [] - } + const entry = logs[index] + entry.response = cloneObj(response) + entry.responseTime = time + entry.success = !response.error - const accounts = {} - for (const c of perm.caveats) { - if (c.name === CAVEAT_NAMES.exposedAccounts && Array.isArray(c.value)) { - for (const v of c.value) { - if (isValidAddress(v)) { - accounts[v] = true - } - } - } - } - return Object.keys(accounts) + this.updateActivityLog(logs) } - // commit an activity entry to the log - commitActivity (entry) { + /** + * Commit a new entry to the activity log. + * Removes the oldest entry from the log if it exceeds the log limit. + * + * @param {Object} entry - The activity log entry. + */ + commitNewActivity (entry) { - const logs = this.getLogStore() + const logs = this.getActivityLog() // add new entry to end of log logs.push(entry) @@ -163,7 +200,7 @@ export default class PermissionsLogController { logs.shift() } - this.updateLogStore() + this.updateActivityLog(logs) } /** @@ -190,7 +227,6 @@ export default class PermissionsLogController { lastApproved: time, }, } - } else { // Records new "lastApproved" times for the granted permissions, if any. @@ -218,7 +254,6 @@ export default class PermissionsLogController { lastApproved: time, accounts: accountToTimeMap, } - } else { acc[method] = { lastApproved: time } } @@ -230,30 +265,22 @@ export default class PermissionsLogController { } if (Object.keys(newEntries).length > 0) { - this.commitHistory(origin, newEntries) + this.commitNewHistory(origin, newEntries) } } - updateAccountsHistory (origin, accounts) { - - if (accounts.length === 0) { - return - } - - const accountToTimeMap = getAccountToTimeMap(accounts, Date.now()) - - this.commitHistory(origin, { - eth_accounts: { - accounts: accountToTimeMap, - }, - }) - } - - // commit a history log entry - commitHistory (origin, newEntries) { + /** + * Commit new entries to the permissions history log. + * Merges the history for the given origin, overwriting existing entries + * with the same key (permission name). + * + * @param {string} origin - The requesting origin. + * @param {Object} newEntries - The new entries to commit. + */ + commitNewHistory (origin, newEntries) { // a simple merge updates most permissions - const history = this.getHistoryStore() + const history = this.getHistory() const newOriginHistory = { ...history[origin], ...newEntries, @@ -286,7 +313,55 @@ export default class PermissionsLogController { history[origin] = newOriginHistory - this.updateHistoryStore(history) + this.updateHistory(history) + } + + /** + * Get all requested methods from a permissions request. + * + * @param {Object} request - The request object. + * @returns {Array} - The names of the requested permissions. + */ + getRequestedMethods (request) { + if ( + !request.params || + typeof request.params[0] !== 'object' || + Array.isArray(request.params[0]) + ) { + return null + } + return Object.keys(request.params[0]) + } + + /** + * Get the permitted accounts from an eth_accounts permissions object. + * Returns an empty array if the permission is not eth_accounts. + * + * @param {Object} perm - The permissions object. + * @returns {Array} - The permitted accounts. + */ + getAccountsFromPermission (perm) { + + if (perm.parentCapability !== 'eth_accounts' || !perm.caveats) { + return [] + } + + const accounts = {} + for (const caveat of perm.caveats) { + + if ( + caveat.name === CAVEAT_NAMES.exposedAccounts && + Array.isArray(caveat.value) + ) { + + for (const value of caveat.value) { + if (isValidAddress(value)) { + accounts[value] = true + } + } + } + } + return Object.keys(accounts) } } @@ -304,3 +379,27 @@ function cloneObj (obj) { } return { ...obj } } + +function getAccountToTimeMap (accounts, time) { + return accounts.reduce( + (acc, account) => ({ ...acc, [account]: time }), {} + ) +} + +function getLastIndexOfObjectArray (array, key, value) { + + if (Array.isArray(array) && array.length > 0) { + + for (let i = array.length - 1; i >= 0; i--) { + + if (typeof array[i] !== 'object') { + throw new Error(`Encountered non-Object element at index ${i}`) + } + + if (array[i][key] === value) { + return i + } + } + } + return -1 +} diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index ef7be96ef558..1c3a6863b31f 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -559,7 +559,7 @@ export default class MetamaskController extends EventEmitter { removePermissionsFor: permissionsController.removePermissionsFor.bind(permissionsController), updatePermittedAccounts: nodeify(permissionsController.updatePermittedAccounts, permissionsController), legacyExposeAccounts: nodeify(permissionsController.legacyExposeAccounts, permissionsController), - handleNewAccountSelected: nodeify(permissionsController.handleNewAccountSelected, permissionsController), + handleNewAccountSelected: nodeify(this.handleNewAccountSelected, this), getRequestAccountTabIds: (cb) => cb(null, this.getRequestAccountTabIds()), getOpenMetamaskTabsIds: (cb) => cb(null, this.getOpenMetamaskTabsIds()), diff --git a/ui/app/components/app/account-menu/account-menu.container.js b/ui/app/components/app/account-menu/account-menu.container.js index fd1c589f1ce1..889e1b97ff80 100644 --- a/ui/app/components/app/account-menu/account-menu.container.js +++ b/ui/app/components/app/account-menu/account-menu.container.js @@ -35,7 +35,6 @@ function mapStateToProps (state) { /** * TODO:LoginPerSite:ui - * - dispatch background.newAccountSelected() if selectedAddress !== lastSelectedAddress * - propagate the relevant props below after computing them */ // const lastSelectedAddress = getLastSelectedAddress(state, origin) diff --git a/ui/app/selectors/permissions.js b/ui/app/selectors/permissions.js index bb9ff57beeaf..60a5f37e9f23 100644 --- a/ui/app/selectors/permissions.js +++ b/ui/app/selectors/permissions.js @@ -6,7 +6,6 @@ import { } from '../../../app/scripts/controllers/permissions/enums' const permissionsSelector = (state, origin) => { - console.log('permissionsSelector', origin) return origin && state.metamask.domains && state.metamask.domains[origin] } @@ -15,7 +14,6 @@ const accountsPermissionSelector = createSelector( permissionsSelector, (domain = {}) => { - console.log('accountsPermissionSelector:domain', domain) return ( Array.isArray(domain.permissions) ? domain.permissions.find( @@ -37,11 +35,10 @@ const createCaveatEqualSelector = createSelectorCreator( * Expects input from accountsPermissionsSelector. * @returns - An empty array or an array of accounts. */ -export const permittedAccountsSelector = createCaveatEqualSelector( +export const getPermittedAccounts = createCaveatEqualSelector( accountsPermissionSelector, // deep equal check performed on this output (accountsPermission = {}) => { - console.log('permittedAccountsSelector:accountsPermission', accountsPermission) const accountsCaveat = ( Array.isArray(accountsPermission.caveats) && accountsPermission.caveats.find( @@ -49,7 +46,6 @@ export const permittedAccountsSelector = createCaveatEqualSelector( ) ) - console.log('permittedAccountsSelector:accountsCaveat', accountsCaveat) return ( accountsCaveat && Array.isArray(accountsCaveat.value) ? accountsCaveat.value diff --git a/ui/app/selectors/selectors.js b/ui/app/selectors/selectors.js index 48a3bfcfc4f8..3a241dfe096e 100644 --- a/ui/app/selectors/selectors.js +++ b/ui/app/selectors/selectors.js @@ -11,7 +11,10 @@ import { formatDate, getOriginFromUrl, } from '../helpers/utils/util' -import { permittedAccountsSelector } from './permissions' + +import { getPermittedAccounts } from './permissions' + +export { getPermittedAccounts } from './permissions' export function getNetworkIdentifier (state) { const { metamask: { provider: { type, nickname, rpcTarget } } } = state @@ -98,13 +101,11 @@ function lastSelectedAddressSelector (state, origin) { export function getLastSelectedAddress (state, origin) { return ( lastSelectedAddressSelector(state, origin) || - permittedAccountsSelector(state, origin)[0] || // always returns array + getPermittedAccounts(state, origin)[0] || // always returns array getSelectedAddress(state) ) } -export const getPermittedAccounts = (state, origin) => permittedAccountsSelector(state, origin) - export function getSelectedIdentity (state) { const selectedAddress = getSelectedAddress(state) const identities = state.metamask.identities From cafb3ca9d3f704535de6be5fd9fc5daf740ee1e3 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Mon, 27 Jan 2020 11:25:47 -0800 Subject: [PATCH 10/11] prune last selected address history --- app/scripts/controllers/preferences.js | 46 ++++++++++++++++++++++---- app/scripts/metamask-controller.js | 4 ++- ui/app/selectors/selectors.js | 2 +- ui/app/store/actions.js | 2 ++ 4 files changed, 46 insertions(+), 8 deletions(-) diff --git a/app/scripts/controllers/preferences.js b/app/scripts/controllers/preferences.js index 7c8c713a10f3..e9a10ebce2a1 100644 --- a/app/scripts/controllers/preferences.js +++ b/app/scripts/controllers/preferences.js @@ -62,7 +62,7 @@ class PreferencesController { // ENS decentralized website resolution ipfsGateway: 'ipfs.dweb.link', - selectedAddressHistory: {}, + lastSelectedAddressByOrigin: {}, }, opts.initState) this.diagnostics = opts.diagnostics @@ -373,18 +373,52 @@ class PreferencesController { /** * Update the last selected address for the given origin. + * * @param {string} origin - The origin for which the address was selected. * @param {string} address - The new selected address. */ - updateSelectedAddressHistory (origin, address) { + setLastSelectedAddress (origin, address) { - const { selectedAddressHistory } = this.store.getState() + const { lastSelectedAddressByOrigin } = this.store.getState() // only update state if it's necessary - if (selectedAddressHistory[origin] !== address) { - selectedAddressHistory[origin] = address - this.store.updateState({ selectedAddressHistory }) + if (lastSelectedAddressByOrigin[origin] !== address) { + lastSelectedAddressByOrigin[origin] = address + this.store.updateState({ lastSelectedAddressByOrigin }) + } + } + + /** + * Remove the selected address history for the given origin. + * + * @param {Array} origins - The origin to remove the last selected address for. + */ + removeLastSelectedAddressesFor (origins) { + + if ( + !Array.isArray(origins) || + (origins.length > 0 && typeof origins[0] !== 'string') + ) { + throw new Error('Expected array of strings') } + + if (origins.length === 0) { + return + } + + const { lastSelectedAddressByOrigin } = this.store.getState() + + origins.forEach(origin => { + delete lastSelectedAddressByOrigin[origin] + }) + this.store.updateState({ lastSelectedAddressByOrigin }) + } + + /** + * Clears the selected address history. + */ + clearLastSelectedAddressHistory () { + this.store.updateState({ lastSelectedAddressByOrigin: {} }) } /** diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 1c3a6863b31f..9dc8f0b2c2fc 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -490,6 +490,8 @@ export default class MetamaskController extends EventEmitter { setPreference: nodeify(preferencesController.setPreference, preferencesController), completeOnboarding: nodeify(preferencesController.completeOnboarding, preferencesController), addKnownMethodData: nodeify(preferencesController.addKnownMethodData, preferencesController), + clearLastSelectedAddressHistory: nodeify(preferencesController.clearLastSelectedAddressHistory, preferencesController), + removeLastSelectedAddressesFor: nodeify(preferencesController.removeLastSelectedAddressesFor, preferencesController), // BlacklistController whitelistPhishingDomain: this.whitelistPhishingDomain.bind(this), @@ -1030,7 +1032,7 @@ export default class MetamaskController extends EventEmitter { */ async handleNewAccountSelected (origin, address) { this.permissionsController.handleNewAccountSelected(origin, address) - this.preferencesController.updateSelectedAddressHistory(origin, address) + this.preferencesController.setLastSelectedAddress(origin, address) } // --------------------------------------------------------------------------- diff --git a/ui/app/selectors/selectors.js b/ui/app/selectors/selectors.js index 3a241dfe096e..5deb9da4bdac 100644 --- a/ui/app/selectors/selectors.js +++ b/ui/app/selectors/selectors.js @@ -92,7 +92,7 @@ export function getSelectedAddress (state) { } function lastSelectedAddressSelector (state, origin) { - return state.metamask.selectedAddressHistory[origin] || null + return state.metamask.lastSelectedAddressByOrigin[origin] || null } // not using reselect here since the returns are contingent; diff --git a/ui/app/store/actions.js b/ui/app/store/actions.js index c5ed76e909cb..b851ea719b1e 100644 --- a/ui/app/store/actions.js +++ b/ui/app/store/actions.js @@ -2242,6 +2242,7 @@ export function legacyExposeAccounts (origin, accounts) { export function removePermissionsFor (domains) { return () => { background.removePermissionsFor(domains) + background.removeLastSelectedAddressesFor(Object.keys(domains)) } } @@ -2251,6 +2252,7 @@ export function removePermissionsFor (domains) { export function clearPermissions () { return () => { background.clearPermissions() + background.clearLastSelectedAddressHistory() } } From e71376e7bef31c2189c09b93d8e4fbb84b7c3a21 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Mon, 27 Jan 2020 11:53:43 -0800 Subject: [PATCH 11/11] remove custom caveat selector --- ui/app/selectors/permissions.js | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/ui/app/selectors/permissions.js b/ui/app/selectors/permissions.js index 60a5f37e9f23..9399934a90a6 100644 --- a/ui/app/selectors/permissions.js +++ b/ui/app/selectors/permissions.js @@ -1,6 +1,5 @@ -import { createSelector, createSelectorCreator, defaultMemoize } from 'reselect' -import deepEqual from 'fast-deep-equal' +import { createSelector } from 'reselect' import { CAVEAT_NAMES, } from '../../../app/scripts/controllers/permissions/enums' @@ -24,18 +23,12 @@ const accountsPermissionSelector = createSelector( } ) -// comparing caveats should be cheap, at least for now -const createCaveatEqualSelector = createSelectorCreator( - defaultMemoize, - (a = {}, b = {}) => deepEqual(a.caveats, b.caveats) -) - /** * Selects the permitted accounts from an eth_accounts permission. * Expects input from accountsPermissionsSelector. * @returns - An empty array or an array of accounts. */ -export const getPermittedAccounts = createCaveatEqualSelector( +export const getPermittedAccounts = createSelector( accountsPermissionSelector, // deep equal check performed on this output (accountsPermission = {}) => {