Robustify permissions controller requestUserApproval tests (#9064)

* convert requestUserApproval mock to wrapper

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

@@ -19,25 +19,44 @@ export function grantPermissions (permController, origin, permissions) {
 }
 
 /**
- * Sets the underlying rpc-cap requestUserApproval function, and returns
+ * Returns a wrapper for the given permissions controller's requestUserApproval
- * a promise that's resolved once it has been set.
+ * function, so we don't have to worry about its internals.
  *
- * This function must be called on the given permissions controller every
+ * @param {PermissionsController} permController - The permissions controller.
- * time you want such a Promise. As of writing, it's only called once per test.
+ * @return {Function} A convenient wrapper for the requestUserApproval function.
+ */
+export function getRequestUserApprovalHelper (permController) {
+  /**
+   * Returns a request object that can be passed to requestUserApproval.
+   *
+   * @param {string} id - The internal permissions request ID (not the RPC request ID).
+   * @param {string} [origin] - The origin of the request, if necessary.
+   * @returns {Object} The corresponding request object.
+   */
+  return (id, origin = 'defaultOrigin') => {
+    return permController.permissions.requestUserApproval({ metadata: { id, origin } })
+  }
+}
+
+/**
+ * Returns a Promise that resolves once a pending user approval has been set.
+ * Calls the underlying requestUserApproval function as normal, and restores it
+ * once the Promise is resolved.
+ *
+ * This function must be called on the permissions controller for each request.
  *
  * @param {PermissionsController} - A permissions controller.
  * @returns {Promise<void>} A Promise that resolves once a pending approval
  * has been set.
  */
 export function getUserApprovalPromise (permController) {
-  return new Promise((resolveForCaller) => {
+  const originalFunction = permController.permissions.requestUserApproval
-    permController.permissions.requestUserApproval = async (req) => {
+  return new Promise((resolveHelperPromise) => {
-      const { origin, metadata: { id } } = req
+    permController.permissions.requestUserApproval = (req) => {
-
+      const userApprovalPromise = originalFunction(req)
-      return new Promise((resolve, reject) => {
+      permController.permissions.requestUserApproval = originalFunction
-        permController.pendingApprovals.set(id, { origin, resolve, reject })
+      resolveHelperPromise()
-        resolveForCaller()
+      return userApprovalPromise
-      })
     }
   })
 }

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

@@ -15,6 +15,7 @@ import {
 } from '../../../../../app/scripts/controllers/permissions'
 
 import {
+  getRequestUserApprovalHelper,
   grantPermissions,
 } from './helpers'
 
@@ -58,12 +59,6 @@ const initPermController = (notifications = initNotifications()) => {
   })
 }
 
-const getMockRequestUserApprovalFunction = (permController) => (id, origin) => {
-  return new Promise((resolve, reject) => {
-    permController.pendingApprovals.set(id, { origin, resolve, reject })
-  })
-}
-
 describe('permissions controller', function () {
 
   describe('getAccounts', function () {
@@ -951,13 +946,11 @@ describe('permissions controller', function () {
 
   describe('approvePermissionsRequest', function () {
 
-    let permController, mockRequestUserApproval
+    let permController, requestUserApproval
 
     beforeEach(function () {
       permController = initPermController()
-      mockRequestUserApproval = getMockRequestUserApprovalFunction(
+      requestUserApproval = getRequestUserApprovalHelper(permController)
-        permController,
-      )
     })
 
     it('does nothing if called on non-existing request', async function () {
@@ -994,14 +987,14 @@ describe('permissions controller', function () {
         PERMS.requests.eth_accounts(),
       )
 
-      const requestRejection = assert.rejects(
+      const rejectionPromise = assert.rejects(
-        mockRequestUserApproval(REQUEST_IDS.a),
+        requestUserApproval(REQUEST_IDS.a),
         ERRORS.validatePermittedAccounts.invalidParam(),
-        'should reject bad accounts',
+        'should reject with "null" accounts',
       )
 
       await permController.approvePermissionsRequest(request, null)
-      await requestRejection
+      await rejectionPromise
 
       assert.equal(
         permController.pendingApprovals.size, 0,
@@ -1014,7 +1007,7 @@ describe('permissions controller', function () {
       const request = PERMS.approvedRequest(REQUEST_IDS.a, {})
 
       const requestRejection = assert.rejects(
-        mockRequestUserApproval(REQUEST_IDS.a),
+        requestUserApproval(REQUEST_IDS.a),
         ERRORS.approvePermissionsRequest.noPermsRequested(),
         'should reject if no permissions in request',
       )
@@ -1036,7 +1029,7 @@ describe('permissions controller', function () {
 
       const requestApproval = assert.doesNotReject(
         async () => {
-          perms = await mockRequestUserApproval(REQUEST_IDS.a)
+          perms = await requestUserApproval(REQUEST_IDS.a)
         },
         'should not reject single valid request',
       )
@@ -1065,14 +1058,14 @@ describe('permissions controller', function () {
 
       const approval1 = assert.doesNotReject(
         async () => {
-          perms1 = await mockRequestUserApproval(REQUEST_IDS.a)
+          perms1 = await requestUserApproval(REQUEST_IDS.a, DOMAINS.a.origin)
         },
         'should not reject request',
       )
 
       const approval2 = assert.doesNotReject(
         async () => {
-          perms2 = await mockRequestUserApproval(REQUEST_IDS.b)
+          perms2 = await requestUserApproval(REQUEST_IDS.b, DOMAINS.b.origin)
         },
         'should not reject request',
       )
@@ -1105,13 +1098,11 @@ describe('permissions controller', function () {
 
   describe('rejectPermissionsRequest', function () {
 
-    let permController, mockRequestUserApproval
+    let permController, requestUserApproval
 
     beforeEach(async function () {
       permController = initPermController()
-      mockRequestUserApproval = getMockRequestUserApprovalFunction(
+      requestUserApproval = getRequestUserApprovalHelper(permController)
-        permController,
-      )
     })
 
     it('does nothing if called on non-existing request', async function () {
@@ -1135,7 +1126,7 @@ describe('permissions controller', function () {
     it('rejects single existing request', async function () {
 
       const requestRejection = assert.rejects(
-        mockRequestUserApproval(REQUEST_IDS.a),
+        requestUserApproval(REQUEST_IDS.a),
         ERRORS.rejectPermissionsRequest.rejection(),
         'should reject with expected error',
       )
@@ -1152,13 +1143,13 @@ describe('permissions controller', function () {
     it('rejects requests regardless of order', async function () {
 
       const requestRejection1 = assert.rejects(
-        mockRequestUserApproval(REQUEST_IDS.b),
+        requestUserApproval(REQUEST_IDS.b, DOMAINS.b.origin),
         ERRORS.rejectPermissionsRequest.rejection(),
         'should reject with expected error',
       )
 
       const requestRejection2 = assert.rejects(
-        mockRequestUserApproval(REQUEST_IDS.c),
+        requestUserApproval(REQUEST_IDS.c, DOMAINS.c.origin),
         ERRORS.rejectPermissionsRequest.rejection(),
         'should reject with expected error',
       )

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

@@ -70,11 +70,15 @@ describe('permissions middleware', function () {
       )
       const res = {}
 
+      const userApprovalPromise = getUserApprovalPromise(permController)
+
       const pendingApproval = assert.doesNotReject(
         aMiddleware(req, res),
         'should not reject permissions request',
       )
 
+      await userApprovalPromise
+
       assert.equal(
         permController.pendingApprovals.size, 1,
         'perm controller should have single pending approval',
@@ -131,11 +135,15 @@ describe('permissions middleware', function () {
       // send, approve, and validate first request
       // note use of ACCOUNTS.a.permitted
 
+      let userApprovalPromise = getUserApprovalPromise(permController)
+
       const pendingApproval1 = assert.doesNotReject(
         aMiddleware(req1, res1),
         'should not reject permissions request',
       )
 
+      await userApprovalPromise
+
       const id1 = permController.pendingApprovals.keys().next().value
       const approvedReq1 = PERMS.approvedRequest(id1, PERMS.requests.eth_accounts())
 
@@ -187,11 +195,15 @@ describe('permissions middleware', function () {
       // send, approve, and validate second request
       // note use of ACCOUNTS.b.permitted
 
+      userApprovalPromise = getUserApprovalPromise(permController)
+
       const pendingApproval2 = assert.doesNotReject(
         aMiddleware(req2, res2),
         'should not reject permissions request',
       )
 
+      await userApprovalPromise
+
       const id2 = permController.pendingApprovals.keys().next().value
       const approvedReq2 = PERMS.approvedRequest(id2, { ...requestedPerms2 })
 
@@ -251,12 +263,16 @@ describe('permissions middleware', function () {
 
       const expectedError = ERRORS.rejectPermissionsRequest.rejection()
 
+      const userApprovalPromise = getUserApprovalPromise(permController)
+
       const requestRejection = assert.rejects(
         aMiddleware(req, res),
         expectedError,
         'request should be rejected with correct error',
       )
 
+      await userApprovalPromise
+
       assert.equal(
         permController.pendingApprovals.size, 1,
         'perm controller should have single pending approval',
@@ -343,11 +359,15 @@ describe('permissions middleware', function () {
       )
       const resA1 = {}
 
+      let userApprovalPromise = getUserApprovalPromise(permController)
+
       const requestApproval1 = assert.doesNotReject(
         aMiddleware(reqA1, resA1),
         'should not reject permissions request',
       )
 
+      await userApprovalPromise
+
       // create and start processing first request for second origin
 
       const reqB1 = RPC_REQUESTS.requestPermission(
@@ -355,11 +375,15 @@ describe('permissions middleware', function () {
       )
       const resB1 = {}
 
+      userApprovalPromise = getUserApprovalPromise(permController)
+
       const requestApproval2 = assert.doesNotReject(
         bMiddleware(reqB1, resB1),
         'should not reject permissions request',
       )
 
+      await userApprovalPromise
+
       assert.equal(
         permController.pendingApprovals.size, 2,
         'perm controller should have expected number of pending approvals',
@@ -373,12 +397,17 @@ describe('permissions middleware', function () {
       )
       const resA2 = {}
 
-      await assert.rejects(
+      userApprovalPromise = getUserApprovalPromise(permController)
+
+      const requestApprovalFail = assert.rejects(
         aMiddleware(reqA2, resA2),
         expectedError,
         'request should be rejected with correct error',
       )
 
+      await userApprovalPromise
+      await requestApprovalFail
+
       assert.ok(
         (
           !resA2.result && resA2.error &&