refactor(permission-controller): Rationalize permission and caveat va…

…lidation (#5062)

## Explanation

Via discussion during
MetaMask/metamask-extension#27847, it became
apparent that there was confusion re: the purpose of permission vs.
caveat validators and when to use one vs. the other. This has now been
properly documented in multiple places in the permission controller
package.

In addition, the permission controller implementation contained some
irregularities regarding its own permission validation logic. This
surfaced in the form of redundant permission and caveat validator calls,
both of which have now been removed. (In addition, one overly
complicated "optimization" to avoid redundant caveat validator calls has
been removed.)

We have verified that permission validators aren't used for caveat
validation anywhere in our codebase.

## References

-
MetaMask/metamask-extension#27847 (comment)

## Changelog

### `@metamask/permission-controller`

- **CHANGED**: Remove redundant caveat validator calls
- In some cases, caveats were being validated multiple times or without
the possibility of having changed.
- The intended purpose of permission and caveat validators has also been
documented. See `ARCHITECTURE.md`.

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
- [x] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes

packages/permission-controller/ARCHITECTURE.md

@@ -165,6 +165,38 @@ C = { foo: 'baz', life: 42 };
 Delta = { foo: 'baz' };
 ```
 
+### Specifying permissions and caveats
+
+Permissions and caveats are specified by constructing _specification objects_,
+which are passed to the `PermissionController` constructor. See the [construction examples](#construction)
+for how to do this.
+
+#### Permission and caveat validators
+
+Permission and caveat specifications optionally include a `validator` function.
+This function is called to validate the permission or caveat when they change.
+If validation fails, the validator function should throw an appropriate JSON-RPC error.
+
+The validators are invoked in the following cases:
+
+- Permission validators
+  - When a permission is granted
+  - When a permission's caveat array is mutated
+- Caveat validators
+  - When a caveat is constructed
+  - When a caveat's value is mutated
+
+Notice that permission validators are only invoked when a permission's caveat array is mutated,
+not when an individual caveat is mutated. This means that permission validators **must not**
+be relied upon to validate caveat values.
+
+This establishes a separation of concerns between permission validators and caveat validators.
+In brief:
+
+- Caveat validators are inherently unaware of the permissions that they are caveats of.
+- Permission validators **should** be unaware of the internal structure of their caveats.
+  - However, they **may** be used to verify the membership of its caveat array.
+
 ### Requesting permissions
 
 The `PermissionController` provides two methods for requesting permissions:
@@ -221,6 +253,15 @@ const caveatSpecifications = {
           caveat.value.includes(resultValue),
         );
       },
+    validator: (caveat: { type: 'filterArrayResponse'; value: Json }) => {
+      // This function is called to validate the value of a caveat.
+      // If the value is invalid, the request will fail. By way of example,
+      // we could check that the value is an array of strings:
+      return (
+        Array.isArray(caveat.value) &&
+        caveat.value.every((v) => typeof v === 'string')
+      );
+    },
     // This function is called if two caveats of this type have to be merged
     // due to an incremental permissions request. The values must be merged
     // in the fashion of a right-biased union.
@@ -238,6 +279,18 @@ const permissionSpecifications = {
     // i.e. the restricted method name
     targetName: 'wallet_getSecretArray',
     allowedCaveats: ['filterArrayResponse'],
+    validator: (permission: PermissionConstraint) => {
+      // This function is called to validate the permission.
+      // If the permission is invalid, the request will fail.
+      // By way of example, we could check that the permission has at least
+      // one caveat of type 'filterArrayResponse'.
+      assert.ok(
+        permission.caveats?.some(
+          (caveat) => caveat.type === CaveatTypes.filterArrayResponse,
+        ),
+        'getSecretArray permission validation failed',
+      );
+    },
     // Every restricted method must specify its implementation in its
     // specification.
     methodImplementation: (

packages/permission-controller/src/Caveat.ts

@@ -137,14 +137,15 @@ export type CaveatSpecificationBase = {
 
   /**
    * The validator function used to validate caveats of the associated type
-   * whenever they are instantiated. Caveat are instantiated whenever they are
-   * created or mutated.
+   * whenever they are constructed or mutated.
    *
    * The validator should throw an appropriate JSON-RPC error if validation fails.
    *
    * If no validator is specified, no validation of caveat values will be
-   * performed. Although caveats can also be validated by permission validators,
-   * validating caveat values separately is strongly recommended.
+   * performed. In instances where caveats are mutated but a permission's caveat
+   * array has not changed, any corresponding permission validator will not be
+   * called. For this reason, permission validators **must not** be relied upon
+   * to validate caveats.
    */
   // TODO: Replace `any` with type
   // eslint-disable-next-line @typescript-eslint/no-explicit-any

packages/permission-controller/src/Permission.ts

@@ -405,8 +405,12 @@ type PermissionSpecificationBase<Type extends PermissionType> = {
 
   /**
    * The validator function used to validate permissions of the associated type
-   * whenever they are mutated. The only way a permission can be legally mutated
-   * is when its caveats are modified by the permission controller.
+   * whenever they are granted or their caveat arrays are mutated.
+   *
+   * Permission validators are **not** invoked when a caveat is mutated, provided
+   * the caveat array has not changed. For this reason, permission validators
+   * **must not** be used to validate caveats. To validate caveats, use the
+   * corresponding caveat specification property.
    *
    * The validator should throw an appropriate JSON-RPC error if validation fails.
    */

packages/permission-controller/src/PermissionController.ts

@@ -95,6 +95,14 @@ import { getPermissionMiddlewareFactory } from './permission-middleware';
 import type { GetSubjectMetadata } from './SubjectMetadataController';
 import { collectUniqueAndPairedCaveats, MethodNames } from './utils';
 
+/**
+ * Flags for controlling the validation behavior of certain internal methods.
+ */
+type PermissionValidationFlags = {
+  invokePermissionValidator: boolean;
+  performCaveatValidation: boolean;
+};
+
 /**
  * Metadata associated with {@link PermissionController} subjects.
  */
@@ -1363,13 +1371,15 @@ export class PermissionController<
       };
       this.validateCaveat(caveat, origin, target);
 
+      let addedCaveat = false;
       if (permission.caveats) {
         const caveatIndex = permission.caveats.findIndex(
           (existingCaveat) => existingCaveat.type === caveat.type,
         );
 
         if (caveatIndex === -1) {
           permission.caveats.push(caveat);
+          addedCaveat = true;
         } else {
           permission.caveats.splice(caveatIndex, 1, caveat);
         }
@@ -1380,9 +1390,17 @@ export class PermissionController<
         // the permission validator is also called.
         // @ts-expect-error See above comment
         permission.caveats = [caveat];
+        addedCaveat = true;
       }
 
-      this.validateModifiedPermission(permission, origin);
+      // Mutating a caveat does not warrant permission validation, but mutating
+      // the caveat array does.
+      if (addedCaveat) {
+        this.validateModifiedPermission(permission, origin, {
+          invokePermissionValidator: true,
+          performCaveatValidation: false, // We just validated the caveat
+        });
+      }
     });
   }
 
@@ -1556,23 +1574,28 @@ export class PermissionController<
       permission.caveats.splice(caveatIndex, 1);
     }
 
-    this.validateModifiedPermission(permission, origin);
+    this.validateModifiedPermission(permission, origin, {
+      invokePermissionValidator: true,
+      performCaveatValidation: false, // No caveat object was mutated
+    });
   }
 
   /**
    * Validates the specified modified permission. Should **always** be invoked
-   * on a permission after its caveats have been modified.
+   * on a permission when its caveat array has been mutated.
    *
    * Just like {@link PermissionController.validatePermission}, except that the
    * corresponding target name and specification are retrieved first, and an
    * error is thrown if the target name does not exist.
    *
    * @param permission - The modified permission to validate.
    * @param origin - The origin associated with the permission.
+   * @param validationFlags - Validation flags. See {@link PermissionController.validatePermission}.
    */
   private validateModifiedPermission(
     permission: Draft<PermissionConstraint>,
     origin: OriginString,
+    validationFlags: PermissionValidationFlags,
   ): void {
     /* istanbul ignore if: this should be impossible */
     if (!this.targetExists(permission.parentCapability)) {
@@ -1585,6 +1608,7 @@ export class PermissionController<
       this.getPermissionSpecification(permission.parentCapability),
       permission as PermissionConstraint,
       origin,
+      validationFlags,
     );
   }
 
@@ -1773,17 +1797,10 @@ export class PermissionController<
         ControllerPermissionSpecification,
         ControllerCaveatSpecification
       >;
-      let performCaveatValidation = true;
-
       if (specification.factory) {
         permission = specification.factory(permissionOptions, requestData);
       } else {
         permission = constructPermission(permissionOptions);
-
-        // We do not need to validate caveats in this case, because the plain
-        // permission constructor function does not modify the caveats, which
-        // were already validated by `constructCaveats` above.
-        performCaveatValidation = false;
       }
 
       if (mergePermissions) {
@@ -1795,7 +1812,7 @@ export class PermissionController<
 
       this.validatePermission(specification, permission, origin, {
         invokePermissionValidator: true,
-        performCaveatValidation,
+        performCaveatValidation: true,
       });
       permissions[targetName] = permission;
     }
@@ -1829,10 +1846,10 @@ export class PermissionController<
     specification: PermissionSpecificationConstraint,
     permission: PermissionConstraint,
     origin: OriginString,
-    { invokePermissionValidator, performCaveatValidation } = {
-      invokePermissionValidator: true,
-      performCaveatValidation: true,
-    },
+    {
+      invokePermissionValidator,
+      performCaveatValidation,
+    }: PermissionValidationFlags,
   ): void {
     const { allowedCaveats, validator, targetName } = specification;