Send accountsChanged notification for wallet_requestPermissions (#8742)

* emit accountsChanged for eth_accounts via wallet_requestPermissions

* add/update tests

app/scripts/controllers/permissions/index.js

@@ -47,7 +47,7 @@ export class PermissionsController {
     this.getKeyringAccounts = getKeyringAccounts
     this._getUnlockPromise = getUnlockPromise
     this._notifyDomain = notifyDomain
-    this.notifyAllDomains = notifyAllDomains
+    this._notifyAllDomains = notifyAllDomains
     this._showPermissionRequest = showPermissionRequest
 
     this._restrictedMethods = getRestrictedMethods({
@@ -95,6 +95,7 @@ export class PermissionsController {
       getAccounts: this.getAccounts.bind(this, origin),
       getUnlockPromise: () => this._getUnlockPromise(true),
       hasPermission: this.hasPermission.bind(this, origin),
+      notifyAccountsChanged: this.notifyAccountsChanged.bind(this, origin),
       requestAccountsPermission: this._requestPermissions.bind(
         this, { origin }, { eth_accounts: {} },
       ),
@@ -196,6 +197,7 @@ export class PermissionsController {
    * User approval callback. Resolves the Promise for the permissions request
    * waited upon by rpc-cap, see requestUserApproval in _initializePermissions.
    * The request will be rejected if finalizePermissionsRequest fails.
+   * Idempotent for a given request id.
    *
    * @param {Object} approved - The request object approved by the user
    * @param {Array} accounts - The accounts to expose, if any
@@ -206,7 +208,7 @@ export class PermissionsController {
     const approval = this.pendingApprovals.get(id)
 
     if (!approval) {
-      log.error(`Permissions request with id '${id}' not found`)
+      log.debug(`Permissions request with id '${id}' not found`)
       return
     }
 
@@ -241,6 +243,7 @@ export class PermissionsController {
   /**
    * User rejection callback. Rejects the Promise for the permissions request
    * waited upon by rpc-cap, see requestUserApproval in _initializePermissions.
+   * Idempotent for a given id.
    *
    * @param {string} id - The id of the request rejected by the user
    */
@@ -248,7 +251,7 @@ export class PermissionsController {
     const approval = this.pendingApprovals.get(id)
 
     if (!approval) {
-      log.error(`Permissions request with id '${id}' not found`)
+      log.debug(`Permissions request with id '${id}' not found`)
       return
     }
 
@@ -289,10 +292,7 @@ export class PermissionsController {
 
     const permittedAccounts = await this.getAccounts(origin)
 
-    this.notifyDomain(origin, {
-      method: NOTIFICATION_NAMES.accountsChanged,
-      result: permittedAccounts,
-    })
+    this.notifyAccountsChanged(origin, permittedAccounts)
   }
 
   /**
@@ -338,10 +338,7 @@ export class PermissionsController {
       newPermittedAccounts = await this.getAccounts(origin)
     }
 
-    this.notifyDomain(origin, {
-      method: NOTIFICATION_NAMES.accountsChanged,
-      result: newPermittedAccounts,
-    })
+    this.notifyAccountsChanged(origin, newPermittedAccounts)
   }
 
   /**
@@ -410,21 +407,34 @@ export class PermissionsController {
     })
   }
 
-  notifyDomain (origin, payload) {
+  /**
+   * Notify a domain that its permitted accounts have changed.
+   * Also updates the accounts history log.
+   *
+   * @param {string} origin - The origin of the domain to notify.
+   * @param {Array<string>} newAccounts - The currently permitted accounts.
+   */
+  notifyAccountsChanged (origin, newAccounts) {
+
+    if (typeof origin !== 'string' || !origin) {
+      throw new Error(`Invalid origin: '${origin}'`)
+    }
+
+    if (!Array.isArray(newAccounts)) {
+      throw new Error('Invalid accounts', newAccounts)
+    }
+
+    this._notifyDomain(origin, {
+      method: NOTIFICATION_NAMES.accountsChanged,
+      result: newAccounts,
+    })
 
     // 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.permissionsLog.updateAccountsHistory(
-        origin, payload.result
-      )
-    }
-
-    this._notifyDomain(origin, payload)
+    this.permissionsLog.updateAccountsHistory(
+      origin, newAccounts
+    )
 
     // NOTE:
     // we don't check for accounts changing in the notifyAllDomains case,
@@ -438,7 +448,8 @@ export class PermissionsController {
    * Should only be called after confirming that the permissions exist, to
    * avoid sending unnecessary notifications.
    *
-   * @param {Object} domains { origin: [permissions] }
+   * @param {Object} domains { origin: [permissions] } - The map of domain
+   * origins to permissions to remove.
    */
   removePermissionsFor (domains) {
 
@@ -449,10 +460,7 @@ export class PermissionsController {
         perms.map((methodName) => {
 
           if (methodName === 'eth_accounts') {
-            this.notifyDomain(
-              origin,
-              { method: NOTIFICATION_NAMES.accountsChanged, result: [] }
-            )
+            this.notifyAccountsChanged(origin, [])
           }
 
           return { parentCapability: methodName }
@@ -466,7 +474,7 @@ export class PermissionsController {
    */
   clearPermissions () {
     this.permissions.clearDomains()
-    this.notifyAllDomains({
+    this._notifyAllDomains({
       method: NOTIFICATION_NAMES.accountsChanged,
       result: [],
     })
@@ -583,6 +591,7 @@ export class PermissionsController {
    * @param {string} account - The newly selected account's address.
    */
   async _handleAccountSelected (account) {
+
     if (typeof account !== 'string') {
       throw new Error('Selected account should be a non-empty string.')
     }
@@ -618,10 +627,7 @@ export class PermissionsController {
   async _handleConnectedAccountSelected (origin) {
     const permittedAccounts = await this.getAccounts(origin)
 
-    this.notifyDomain(origin, {
-      method: NOTIFICATION_NAMES.accountsChanged,
-      result: permittedAccounts,
-    })
+    this.notifyAccountsChanged(origin, permittedAccounts)
   }
 
   /**

app/scripts/controllers/permissions/methodMiddleware.js

@@ -9,6 +9,7 @@ export default function createMethodMiddleware ({
   getAccounts,
   getUnlockPromise,
   hasPermission,
+  notifyAccountsChanged,
   requestAccountsPermission,
 }) {
 
@@ -16,6 +17,8 @@ export default function createMethodMiddleware ({
 
   return createAsyncMiddleware(async (req, res, next) => {
 
+    let responseHandler
+
     switch (req.method) {
 
       // Intercepting eth_accounts requests for backwards compatibility:
@@ -81,10 +84,33 @@ export default function createMethodMiddleware ({
         res.result = true
         return
 
+      // register return handler to send accountsChanged notification
+      case 'wallet_requestPermissions':
+
+        if ('eth_accounts' in req.params?.[0]) {
+
+          responseHandler = async () => {
+
+            if (Array.isArray(res.result)) {
+              for (const permission of res.result) {
+                if (permission.parentCapability === 'eth_accounts') {
+                  notifyAccountsChanged(await getAccounts())
+                }
+              }
+            }
+          }
+        }
+        break
+
       default:
         break
     }
 
-    next()
+    // when this promise resolves, the response is on its way back
+    await next()
+
+    if (responseHandler) {
+      responseHandler()
+    }
   })
 }

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

@@ -446,6 +446,19 @@ export const getters = deepFreeze({
         }
       },
     },
+
+    notifyAccountsChanged: {
+      invalidOrigin: (origin) => {
+        return {
+          message: `Invalid origin: '${origin}'`,
+        }
+      },
+      invalidAccounts: () => {
+        return {
+          message: 'Invalid accounts',
+        }
+      },
+    },
   },
 
   /**
@@ -477,18 +490,6 @@ export const getters = deepFreeze({
         result: accounts,
       }
     },
-
-    /**
-     * Gets a test notification that doesn't occur in practice.
-     *
-     * @returns {Object} A notification with the 'test_notification' method name
-     */
-    test: () => {
-      return {
-        method: 'test_notification',
-        result: true,
-      }
-    },
   },
 
   /**

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

@@ -1163,7 +1163,7 @@ describe('permissions controller', function () {
     })
   })
 
-  describe('notifyDomain', function () {
+  describe('notifyAccountsChanged', function () {
 
     let notifications, permController
 
@@ -1173,11 +1173,11 @@ describe('permissions controller', function () {
       sinon.spy(permController.permissionsLog, 'updateAccountsHistory')
     })
 
-    it('notifyDomain handles accountsChanged', async function () {
+    it('notifyAccountsChanged records history and sends notification', async function () {
 
-      permController.notifyDomain(
+      permController.notifyAccountsChanged(
         ORIGINS.a,
-        NOTIFICATIONS.newAccounts(ACCOUNTS.a.permitted),
+        ACCOUNTS.a.permitted,
       )
 
       assert.ok(
@@ -1192,19 +1192,45 @@ describe('permissions controller', function () {
       )
     })
 
-    it('notifyDomain handles notifications other than accountsChanged', async function () {
+    it('notifyAccountsChanged throws on invalid origin', async function () {
 
-      permController.notifyDomain(ORIGINS.a, NOTIFICATIONS.test())
-
-      assert.ok(
-        permController.permissionsLog.updateAccountsHistory.notCalled,
-        'permissionsLog.updateAccountsHistory should not have been called'
+      assert.throws(
+        () => permController.notifyAccountsChanged(
+          4,
+          ACCOUNTS.a.permitted,
+        ),
+        ERRORS.notifyAccountsChanged.invalidOrigin(4),
+        'should throw expected error for non-string origin'
       )
 
-      assert.deepEqual(
-        notifications[ORIGINS.a],
-        [ NOTIFICATIONS.test() ],
-        'origin should have correct notification'
+      assert.throws(
+        () => permController.notifyAccountsChanged(
+          '',
+          ACCOUNTS.a.permitted,
+        ),
+        ERRORS.notifyAccountsChanged.invalidOrigin(''),
+        'should throw expected error for empty string origin'
+      )
+    })
+
+    it('notifyAccountsChanged throws on invalid accounts', async function () {
+
+      assert.throws(
+        () => permController.notifyAccountsChanged(
+          ORIGINS.a,
+          4,
+        ),
+        ERRORS.notifyAccountsChanged.invalidAccounts(),
+        'should throw expected error for truthy non-array accounts'
+      )
+
+      assert.throws(
+        () => permController.notifyAccountsChanged(
+          ORIGINS.a,
+          null,
+        ),
+        ERRORS.notifyAccountsChanged.invalidAccounts(),
+        'should throw expected error for falsy non-array accounts'
       )
     })
   })

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

@@ -1,5 +1,5 @@
 import { strict as assert } from 'assert'
-import { useFakeTimers } from 'sinon'
+import sinon from 'sinon'
 
 import {
   METADATA_STORE_KEY,
@@ -58,6 +58,7 @@ describe('permissions middleware', function () {
 
     beforeEach(function () {
       permController = initPermController()
+      permController.notifyAccountsChanged = sinon.fake()
     })
 
     it('grants permissions on user approval', async function () {
@@ -107,6 +108,13 @@ describe('permissions middleware', function () {
         aAccounts, [ACCOUNTS.a.primary],
         'origin should have correct accounts'
       )
+
+      assert.ok(
+        permController.notifyAccountsChanged.calledOnceWith(
+          ORIGINS.a, aAccounts,
+        ),
+        'expected notification call should have been made'
+      )
     })
 
     it('handles serial approved requests that overwrite existing permissions', async function () {
@@ -157,6 +165,13 @@ describe('permissions middleware', function () {
         'origin should have correct accounts'
       )
 
+      assert.ok(
+        permController.notifyAccountsChanged.calledOnceWith(
+          ORIGINS.a, accounts1,
+        ),
+        'expected notification call should have been made'
+      )
+
       // create second request
 
       const requestedPerms2 = {
@@ -211,6 +226,18 @@ describe('permissions middleware', function () {
         accounts2, [ACCOUNTS.b.primary],
         'origin should have correct accounts'
       )
+
+      assert.equal(
+        permController.notifyAccountsChanged.callCount, 2,
+        'should have called notification method 2 times in total'
+      )
+
+      assert.ok(
+        permController.notifyAccountsChanged.lastCall.calledWith(
+          ORIGINS.a, accounts2,
+        ),
+        'expected notification call should have been made'
+      )
     })
 
     it('rejects permissions on user rejection', async function () {
@@ -252,6 +279,11 @@ describe('permissions middleware', function () {
       assert.deepEqual(
         aAccounts, [], 'origin should have have correct accounts'
       )
+
+      assert.ok(
+        permController.notifyAccountsChanged.notCalled,
+        'should not have called notification method'
+      )
     })
 
     it('rejects requests with unknown permissions', async function () {
@@ -288,6 +320,11 @@ describe('permissions middleware', function () {
         ),
         'response should have expected error and no result'
       )
+
+      assert.ok(
+        permController.notifyAccountsChanged.notCalled,
+        'should not have called notification method'
+      )
     })
 
     it('accepts only a single pending permissions request per origin', async function () {
@@ -695,7 +732,7 @@ describe('permissions middleware', function () {
 
     beforeEach(function () {
       permController = initPermController()
-      clock = useFakeTimers(1)
+      clock = sinon.useFakeTimers(1)
     })
 
     afterEach(function () {