diff --git a/packages/permission-controller/ARCHITECTURE.md b/packages/permission-controller/ARCHITECTURE.md index 8c715234bac..bce0a401778 100644 --- a/packages/permission-controller/ARCHITECTURE.md +++ b/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: ( diff --git a/packages/permission-controller/src/Caveat.ts b/packages/permission-controller/src/Caveat.ts index 7bfb57b199e..63f455560bc 100644 --- a/packages/permission-controller/src/Caveat.ts +++ b/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 diff --git a/packages/permission-controller/src/Permission.ts b/packages/permission-controller/src/Permission.ts index d5aa7e7b8ce..809cc168dd0 100644 --- a/packages/permission-controller/src/Permission.ts +++ b/packages/permission-controller/src/Permission.ts @@ -405,8 +405,12 @@ type PermissionSpecificationBase = { /** * 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. */ diff --git a/packages/permission-controller/src/PermissionController.ts b/packages/permission-controller/src/PermissionController.ts index 359566f43a4..b5e33393f86 100644 --- a/packages/permission-controller/src/PermissionController.ts +++ b/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,6 +1371,7 @@ export class PermissionController< }; this.validateCaveat(caveat, origin, target); + let addedCaveat = false; if (permission.caveats) { const caveatIndex = permission.caveats.findIndex( (existingCaveat) => existingCaveat.type === caveat.type, @@ -1370,6 +1379,7 @@ export class PermissionController< 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,12 +1574,15 @@ 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 @@ -1569,10 +1590,12 @@ export class PermissionController< * * @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, 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;