From 17af3bb11e6b4168714e96a028155739b3d84845 Mon Sep 17 00:00:00 2001
From: Jyoti Puri <jyotipuri@gmail.com>
Date: Mon, 31 Oct 2022 11:22:31 +0530
Subject: [PATCH] Making permission and approval controller methods idempotent
 (#15848)

---
 .../metamask-controller.actions.test.js       | 138 ++++++++++++++++++
 app/scripts/metamask-controller.js            |  79 +++++++---
 2 files changed, 198 insertions(+), 19 deletions(-)

diff --git a/app/scripts/metamask-controller.actions.test.js b/app/scripts/metamask-controller.actions.test.js
index c0e438c66..b3eff23d7 100644
--- a/app/scripts/metamask-controller.actions.test.js
+++ b/app/scripts/metamask-controller.actions.test.js
@@ -1,6 +1,11 @@
 import { strict as assert } from 'assert';
 import sinon from 'sinon';
 import proxyquire from 'proxyquire';
+
+import {
+  ApprovalRequestNotFoundError,
+  PermissionsRequestNotFoundError,
+} from '@metamask/controllers';
 import { ORIGIN_METAMASK } from '../../shared/constants/app';
 
 const Ganache = require('../../test/e2e/ganache');
@@ -238,4 +243,137 @@ describe('MetaMaskController', function () {
       assert.deepEqual(transaction1, transaction2);
     });
   });
+
+  describe('#removePermissionsFor', function () {
+    it('should not propagate PermissionsRequestNotFoundError', function () {
+      const error = new PermissionsRequestNotFoundError('123');
+      metamaskController.permissionController = {
+        revokePermissions: () => {
+          throw error;
+        },
+      };
+      // Line below will not throw error, in case it throws this test case will fail.
+      metamaskController.removePermissionsFor({ subject: 'test_subject' });
+    });
+
+    it('should propagate Error other than PermissionsRequestNotFoundError', function () {
+      const error = new Error();
+      metamaskController.permissionController = {
+        revokePermissions: () => {
+          throw error;
+        },
+      };
+      assert.throws(() => {
+        metamaskController.removePermissionsFor({ subject: 'test_subject' });
+      }, error);
+    });
+  });
+
+  describe('#rejectPermissionsRequest', function () {
+    it('should not propagate PermissionsRequestNotFoundError', function () {
+      const error = new PermissionsRequestNotFoundError('123');
+      metamaskController.permissionController = {
+        rejectPermissionsRequest: () => {
+          throw error;
+        },
+      };
+      // Line below will not throw error, in case it throws this test case will fail.
+      metamaskController.rejectPermissionsRequest('DUMMY_ID');
+    });
+
+    it('should propagate Error other than PermissionsRequestNotFoundError', function () {
+      const error = new Error();
+      metamaskController.permissionController = {
+        rejectPermissionsRequest: () => {
+          throw error;
+        },
+      };
+      assert.throws(() => {
+        metamaskController.rejectPermissionsRequest('DUMMY_ID');
+      }, error);
+    });
+  });
+
+  describe('#acceptPermissionsRequest', function () {
+    it('should not propagate PermissionsRequestNotFoundError', function () {
+      const error = new PermissionsRequestNotFoundError('123');
+      metamaskController.permissionController = {
+        acceptPermissionsRequest: () => {
+          throw error;
+        },
+      };
+      // Line below will not throw error, in case it throws this test case will fail.
+      metamaskController.acceptPermissionsRequest('DUMMY_ID');
+    });
+
+    it('should propagate Error other than PermissionsRequestNotFoundError', function () {
+      const error = new Error();
+      metamaskController.permissionController = {
+        acceptPermissionsRequest: () => {
+          throw error;
+        },
+      };
+      assert.throws(() => {
+        metamaskController.acceptPermissionsRequest('DUMMY_ID');
+      }, error);
+    });
+  });
+
+  describe('#resolvePendingApproval', function () {
+    it('should not propagate ApprovalRequestNotFoundError', function () {
+      const error = new ApprovalRequestNotFoundError('123');
+      metamaskController.approvalController = {
+        accept: () => {
+          throw error;
+        },
+      };
+      // Line below will not throw error, in case it throws this test case will fail.
+      metamaskController.resolvePendingApproval('DUMMY_ID', 'DUMMY_VALUE');
+    });
+
+    it('should propagate Error other than ApprovalRequestNotFoundError', function () {
+      const error = new Error();
+      metamaskController.approvalController = {
+        accept: () => {
+          throw error;
+        },
+      };
+      assert.throws(() => {
+        metamaskController.resolvePendingApproval('DUMMY_ID', 'DUMMY_VALUE');
+      }, error);
+    });
+  });
+
+  describe('#rejectPendingApproval', function () {
+    it('should not propagate ApprovalRequestNotFoundError', function () {
+      const error = new ApprovalRequestNotFoundError('123');
+      metamaskController.approvalController = {
+        reject: () => {
+          throw error;
+        },
+      };
+      // Line below will not throw error, in case it throws this test case will fail.
+      metamaskController.rejectPendingApproval('DUMMY_ID', {
+        code: 1,
+        message: 'DUMMY_MESSAGE',
+        data: 'DUMMY_DATA',
+      });
+    });
+
+    it('should propagate Error other than ApprovalRequestNotFoundError', function () {
+      const error = new Error();
+      metamaskController.approvalController = {
+        reject: () => {
+          throw error;
+        },
+      };
+      assert.throws(() => {
+        metamaskController.rejectPendingApproval('DUMMY_ID', {
+          code: 1,
+          message: 'DUMMY_MESSAGE',
+          data: 'DUMMY_DATA',
+        });
+      }, error);
+    });
+  });
 });
diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js
index 67380cb6c..fbc9847c6 100644
--- a/app/scripts/metamask-controller.js
+++ b/app/scripts/metamask-controller.js
@@ -39,6 +39,8 @@ import {
   CollectibleDetectionController,
   PermissionController,
   SubjectMetadataController,
+  PermissionsRequestNotFoundError,
+  ApprovalRequestNotFoundError,
   ///: BEGIN:ONLY_INCLUDE_IN(flask)
   RateLimitController,
   NotificationController,
@@ -1498,7 +1500,6 @@ export default class MetamaskController extends EventEmitter {
     const {
       addressBookController,
       alertController,
-      approvalController,
       appStateController,
       collectiblesController,
       collectibleDetectionController,
@@ -1820,16 +1821,9 @@ export default class MetamaskController extends EventEmitter {
       initializeThreeBox: this.initializeThreeBox.bind(this),
 
       // permissions
-      removePermissionsFor:
-        permissionController.revokePermissions.bind(permissionController),
-      approvePermissionsRequest:
-        permissionController.acceptPermissionsRequest.bind(
-          permissionController,
-        ),
-      rejectPermissionsRequest:
-        permissionController.rejectPermissionsRequest.bind(
-          permissionController,
-        ),
+      removePermissionsFor: this.removePermissionsFor,
+      approvePermissionsRequest: this.acceptPermissionsRequest,
+      rejectPermissionsRequest: this.rejectPermissionsRequest,
       ...getPermissionBackgroundApiMethods(permissionController),
 
       ///: BEGIN:ONLY_INCLUDE_IN(flask)
@@ -1947,14 +1941,8 @@ export default class MetamaskController extends EventEmitter {
       ),
 
       // approval controller
-      resolvePendingApproval:
-        approvalController.accept.bind(approvalController),
-      rejectPendingApproval: async (id, error) => {
-        approvalController.reject(
-          id,
-          new EthereumRpcError(error.code, error.message, error.data),
-        );
-      },
+      resolvePendingApproval: this.resolvePendingApproval,
+      rejectPendingApproval: this.rejectPendingApproval,
 
       // Notifications
       updateViewedNotifications: announcementController.updateViewed.bind(
@@ -4361,4 +4349,57 @@ export default class MetamaskController extends EventEmitter {
 
     return this.keyringController.setLocked();
   }
+
+  removePermissionsFor = (subjects) => {
+    try {
+      this.permissionController.revokePermissions(subjects);
+    } catch (exp) {
+      if (!(exp instanceof PermissionsRequestNotFoundError)) {
+        throw exp;
+      }
+    }
+  };
+
+  rejectPermissionsRequest = (requestId) => {
+    try {
+      this.permissionController.rejectPermissionsRequest(requestId);
+    } catch (exp) {
+      if (!(exp instanceof PermissionsRequestNotFoundError)) {
+        throw exp;
+      }
+    }
+  };
+
+  acceptPermissionsRequest = (request) => {
+    try {
+      this.permissionController.acceptPermissionsRequest(request);
+    } catch (exp) {
+      if (!(exp instanceof PermissionsRequestNotFoundError)) {
+        throw exp;
+      }
+    }
+  };
+
+  resolvePendingApproval = (id, value) => {
+    try {
+      this.approvalController.accept(id, value);
+    } catch (exp) {
+      if (!(exp instanceof ApprovalRequestNotFoundError)) {
+        throw exp;
+      }
+    }
+  };
+
+  rejectPendingApproval = (id, error) => {
+    try {
+      this.approvalController.reject(
+        id,
+        new EthereumRpcError(error.code, error.message, error.data),
+      );
+    } catch (exp) {
+      if (!(exp instanceof ApprovalRequestNotFoundError)) {
+        throw exp;
+      }
+    }
+  };
 }