refactor(permission-controller): Rationalize permission and caveat validation

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;