update getRemovedAuthorizations to include removed scopes as well as …

…entirely revoked permissions
MetaMask · Feb 5, 2025 · f90be7c · f90be7c
1 parent fd30905
commit f90be7c
Show file tree

Hide file tree

Showing 3 changed files with 102 additions and 36 deletions.
diff --git a/app/scripts/controllers/permissions/selectors.js b/app/scripts/controllers/permissions/selectors.js
@@ -142,7 +142,7 @@ export const diffMap = (currentMap, previousMap) => {
 
 /**
  * Given the current and previous exposed CAIP-25 authorization for each PermissionController
- * subject, returns a new map containing all authorizations that have changed.
+ * subject, returns a new map containing the current value of scopes added/changed in an authorization.
  * The values of each map must be immutable values directly from the
  * PermissionController state, or an empty object instantiated in this
  * function.
@@ -193,10 +193,12 @@ export const getChangedAuthorizations = (
 };
 
 /**
+ * Given the current and previous exposed CAIP-25 authorization for each PermissionController
+ * subject, returns a new map containing the only the scopes removed entirely from an authorization.
  *
  * @param {Map<string, Caip25Authorization>} newAuthorizationsMap - The new origin:authorization map.
  * @param {Map<string, Caip25Authorization>} [previousAuthorizationsMap] - The previous origin:authorization map.
- * @returns {Map<string, Caip25Authorization>} The origin:authorization map of changed authorizations.
+ * @returns {Map<string, Caip25Authorization>} The origin:authorization map of scopes removed from authorizations.
  */
 export const getRemovedAuthorizations = (
   newAuthorizationsMap,
@@ -214,13 +216,44 @@ export const getRemovedAuthorizations = (
     return removedAuthorizations;
   }
 
-  const previousOrigins = new Set([...previousAuthorizationsMap.keys()]);
-  for (const origin of newAuthorizationsMap.keys()) {
-    previousOrigins.delete(origin);
-  }
+  for (const origin of previousAuthorizationsMap.keys()) {
+    const previousAuthorization = previousAuthorizationsMap.get(origin);
 
-  for (const origin of previousOrigins.keys()) {
-    removedAuthorizations.set(origin, previousAuthorizationsMap.get(origin));
+    const newAuthorization = newAuthorizationsMap.get(origin);
+    if (!newAuthorization) {
+      removedAuthorizations.set(origin, previousAuthorization);
+      continue;
+    }
+
+    const removedRequiredScopes = {};
+    Object.entries(previousAuthorization.requiredScopes).forEach(
+      ([scope, prevScopeObject]) => {
+        const newScopeObject = newAuthorization.requiredScopes[scope];
+        if (!newScopeObject) {
+          removedRequiredScopes[scope] = prevScopeObject;
+        }
+      },
+    );
+
+    const removedOptionalScopes = {};
+    Object.entries(previousAuthorization.optionalScopes).forEach(
+      ([scope, prevScopeObject]) => {
+        const newScopeObject = newAuthorization.optionalScopes[scope];
+        if (!newScopeObject) {
+          removedOptionalScopes[scope] = prevScopeObject;
+        }
+      },
+    );
+
+    if (
+      Object.keys(removedRequiredScopes).length > 0 ||
+      Object.keys(removedOptionalScopes).length > 0
+    ) {
+      removedAuthorizations.set(origin, {
+        requiredScopes: removedRequiredScopes,
+        optionalScopes: removedOptionalScopes,
+      });
+    }
   }
 
   return removedAuthorizations;

diff --git a/app/scripts/controllers/permissions/selectors.test.js b/app/scripts/controllers/permissions/selectors.test.js
@@ -176,7 +176,66 @@ describe('PermissionController selectors', () => {
       ).toStrictEqual(new Map());
     });
 
-    it('returns a new map of the removed authorizations if the new and previous values differ', () => {
+    it('returns a new map of the removed scopes in authorizations', () => {
+      const previousAuthorizations = new Map([
+        [
+          'foo.bar',
+          {
+            requiredScopes: {
+              'eip155:1': {
+                accounts: [],
+              },
+            },
+            optionalScopes: {
+              'eip155:5': {
+                accounts: [],
+              },
+              'eip155:10': {
+                accounts: [],
+              },
+            },
+          },
+        ],
+      ]);
+
+      const newAuthorizations = new Map([
+        [
+          'foo.bar',
+          {
+            requiredScopes: {
+              'eip155:1': {
+                accounts: [],
+              },
+            },
+            optionalScopes: {
+              'eip155:10': {
+                accounts: [],
+              },
+            },
+          },
+        ],
+      ]);
+
+      expect(
+        getRemovedAuthorizations(newAuthorizations, previousAuthorizations),
+      ).toStrictEqual(
+        new Map([
+          [
+            'foo.bar',
+            {
+              requiredScopes: {},
+              optionalScopes: {
+                'eip155:5': {
+                  accounts: [],
+                },
+              },
+            },
+          ],
+        ]),
+      );
+    });
+
+    it('returns a new map of the revoked authorizations', () => {
       const mockAuthorization = {
         requiredScopes: {
           'eip155:1': {

diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js
@@ -3124,7 +3124,7 @@ export default class MetamaskController extends EventEmitter {
           });
         }
 
-        // add new notification subscriptions for changed authorizations
+        // add new notification subscriptions for added/changed authorizations
         for (const [origin, authorization] of changedAuthorizations.entries()) {
           const sessionScopes = getSessionScopes(authorization);
 
@@ -3150,32 +3150,6 @@ export default class MetamaskController extends EventEmitter {
               });
             }
           });
-
-          // TODO: could be pushed into selectors?
-          const previousAuthorization = previousValue.get(origin);
-          if (previousAuthorization) {
-            const previousSessionScopes = getSessionScopes(
-              previousAuthorization,
-            );
-
-            Object.entries(previousSessionScopes).forEach(
-              ([scope, scopeObject]) => {
-                if (!sessionScopes[scope]) {
-                  if (
-                    scopeObject.notifications.includes('eth_subscription') &&
-                    scopeObject.methods.includes('eth_subscribe')
-                  ) {
-                    this.removeMultichainApiEthSubscriptionMiddleware({
-                      scope,
-                      origin,
-                    });
-                  }
-                }
-              },
-            );
-          }
-
-          this._notifyAuthorizationChange(origin, authorization);
         }
       },
       getAuthorizedScopesByOrigin,