From 8012ac46a0f426464ce136f5c12c1d7de29ebe77 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 28 Mar 2022 11:06:38 +0100 Subject: [PATCH] permissions: ensure returned conditions match permission in `PermissionPolicy#handle` (#10075) * permission-node: fix signature of permission rule in test suites Signed-off-by: Mike Lewis * permission-common: add isPermission helper for comparing permissions Signed-off-by: Mike Lewis * permission-node: adjust createConditionExports for more type safety Signed-off-by: Mike Lewis * permissions: add resourceType property to PermissionCondition and PermissionRule Signed-off-by: Mike Lewis * catalog: handle changes to PermissionCondition and PermissionRule types Signed-off-by: Mike Lewis * catalog-backend: avoid re-exporting alpha import cf. https://github.com/backstage/backstage/pull/10128 Signed-off-by: Mike Lewis * Update changeset Signed-off-by: Joe Porpeglia * Resolve api-report conflict Signed-off-by: Joon Park Co-authored-by: Joe Porpeglia Co-authored-by: Joon Park --- .changeset/beige-avocados-matter.md | 28 +++ .changeset/calm-walls-heal.md | 5 + .changeset/dry-fans-arrive.md | 63 +++++++ .changeset/modern-actors-notice.md | 5 + .changeset/sour-cameras-deliver.md | 10 + .changeset/sour-lobsters-sniff.md | 5 + .changeset/stale-carrots-smile.md | 5 + plugins/catalog-backend/api-report.md | 65 +++++-- .../src/permissions/conditionExports.ts | 32 ++-- .../catalog-backend/src/permissions/index.ts | 2 +- .../permissions/rules/createPropertyRule.ts | 2 + .../src/permissions/rules/hasAnnotation.ts | 2 + .../src/permissions/rules/hasLabel.ts | 2 + .../src/permissions/rules/index.ts | 1 + .../src/permissions/rules/isEntityKind.ts | 2 + .../src/permissions/rules/isEntityOwner.ts | 2 + .../src/permissions/rules/util.ts | 19 +- .../service/AuthorizedEntitiesCatalog.test.ts | 12 +- .../src/service/CatalogBuilder.ts | 23 +-- .../src/service/createRouter.test.ts | 16 +- .../PermissionIntegrationClient.test.ts | 51 +++-- plugins/permission-common/api-report.md | 12 +- .../permission-common/src/PermissionClient.ts | 1 + .../permission-common/src/permissions/util.ts | 11 ++ plugins/permission-common/src/types/api.ts | 6 +- plugins/permission-node/api-report.md | 63 ++++--- .../createConditionExports.test.ts | 45 ++++- .../src/integration/createConditionExports.ts | 38 ++-- .../createConditionFactory.test.ts | 7 +- .../src/integration/createConditionFactory.ts | 8 +- .../createConditionTransformer.test.ts | 95 ++++++++-- .../integration/createConditionTransformer.ts | 4 +- .../createPermissionIntegrationRouter.test.ts | 175 +++++++++++++++--- .../createPermissionIntegrationRouter.ts | 28 ++- .../src/integration/createPermissionRule.ts | 7 +- .../src/integration/util.test.ts | 11 +- .../permission-node/src/integration/util.ts | 4 +- plugins/permission-node/src/types.ts | 2 + 38 files changed, 676 insertions(+), 193 deletions(-) create mode 100644 .changeset/beige-avocados-matter.md create mode 100644 .changeset/calm-walls-heal.md create mode 100644 .changeset/dry-fans-arrive.md create mode 100644 .changeset/modern-actors-notice.md create mode 100644 .changeset/sour-cameras-deliver.md create mode 100644 .changeset/sour-lobsters-sniff.md create mode 100644 .changeset/stale-carrots-smile.md diff --git a/.changeset/beige-avocados-matter.md b/.changeset/beige-avocados-matter.md new file mode 100644 index 0000000000..d6c1e3b2a5 --- /dev/null +++ b/.changeset/beige-avocados-matter.md @@ -0,0 +1,28 @@ +--- +'@backstage/plugin-catalog-backend': minor +--- + +**BREAKING (alpha api):** Replace `createCatalogPolicyDecision` export with `createCatalogConditionalDecision`, which accepts a permission parameter of type `ResourcePermission<'catalog-entity'>` along with conditions. The permission passed is expected to be the handled permission in `PermissionPolicy#handle`, whose type must first be narrowed using methods like `isPermission` and `isResourcePermission`: + +```typescript +class TestPermissionPolicy implements PermissionPolicy { + async handle( + request: PolicyQuery, + _user?: BackstageIdentityResponse, + ): Promise { + if ( + // Narrow type of `request.permission` to `ResourcePermission<'catalog-entity'> + isResourcePermission(request.permission, RESOURCE_TYPE_CATALOG_ENTITY) + ) { + return createCatalogConditionalDecision( + request.permission, + catalogConditions.isEntityOwner( + _user?.identity.ownershipEntityRefs ?? [], + ), + ); + } + + return { + result: AuthorizeResult.ALLOW, + }; +``` diff --git a/.changeset/calm-walls-heal.md b/.changeset/calm-walls-heal.md new file mode 100644 index 0000000000..f47ce4a0e9 --- /dev/null +++ b/.changeset/calm-walls-heal.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-permission-common': minor +--- + +Add `resourceType` property to `PermissionCondition` type to allow matching them with `ResourcePermission` instances. diff --git a/.changeset/dry-fans-arrive.md b/.changeset/dry-fans-arrive.md new file mode 100644 index 0000000000..6947703e7f --- /dev/null +++ b/.changeset/dry-fans-arrive.md @@ -0,0 +1,63 @@ +--- +'@backstage/plugin-permission-node': minor +--- + +**BREAKING**: Stronger typing in `PermissionPolicy` 🎉. + +Previously, it was entirely the responsibility of the `PermissionPolicy` author to only return `CONDITIONAL` decisions for permissions that are associated with a resource, and to return the correct kind of `PermissionCondition` instances inside the decision. Now, the policy authoring helpers provided in this package now ensure that the decision and permission match. + +**For policy authors**: rename and adjust api of `createConditionExports`. Previously, the function returned a factory for creating conditional decisions named `createPolicyDecision`, which had a couple of drawbacks: + +1. The function always creates a _conditional_ policy decision, but this was not reflected in the name. +2. Conditional decisions should only ever be returned from `PermissionPolicy#handle` for resource permissions, but there was nothing in the API that encoded this constraint. + +This change addresses the drawbacks above by making the following changes for policy authors: + +- The `createPolicyDecision` method has been renamed to `createConditionalDecision`. +- Along with conditions, the method now accepts a permission, which must be a `ResourcePermission`. This is expected to be the handled permission in `PermissionPolicy#handle`, whose type must first be narrowed using methods like `isPermission` and `isResourcePermission`: + +```typescript +class TestPermissionPolicy implements PermissionPolicy { + async handle( + request: PolicyQuery, + _user?: BackstageIdentityResponse, + ): Promise { + if ( + // Narrow type of `request.permission` to `ResourcePermission<'catalog-entity'> + isResourcePermission(request.permission, RESOURCE_TYPE_CATALOG_ENTITY) + ) { + return createCatalogConditionalDecision( + request.permission, + catalogConditions.isEntityOwner( + _user?.identity.ownershipEntityRefs ?? [], + ), + ); + } + + return { + result: AuthorizeResult.ALLOW, + }; +``` + +**BREAKING**: when creating `PermissionRule`s, provide a `resourceType`. + +```diff +export const isEntityOwner = createCatalogPermissionRule({ + name: 'IS_ENTITY_OWNER', + description: 'Allow entities owned by the current user', ++ resourceType: RESOURCE_TYPE_CATALOG_ENTITY, + apply: (resource: Entity, claims: string[]) => { + if (!resource.relations) { + return false; + } + + return resource.relations + .filter(relation => relation.type === RELATION_OWNED_BY) + .some(relation => claims.includes(relation.targetRef)); + }, + toQuery: (claims: string[]) => ({ + key: 'relations.ownedBy', + values: claims, + }), +}); +``` diff --git a/.changeset/modern-actors-notice.md b/.changeset/modern-actors-notice.md new file mode 100644 index 0000000000..20dde7f42d --- /dev/null +++ b/.changeset/modern-actors-notice.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': minor +--- + +**BREAKING:** Mark CatalogBuilder#addPermissionRules as @alpha. diff --git a/.changeset/sour-cameras-deliver.md b/.changeset/sour-cameras-deliver.md new file mode 100644 index 0000000000..21e23e7833 --- /dev/null +++ b/.changeset/sour-cameras-deliver.md @@ -0,0 +1,10 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Handle changes to @alpha permission-related types. + +- All exported permission rules and conditions now have a `resourceType`. +- `createCatalogConditionalDecision` now expects supplied conditions to have the appropriate `resourceType`. +- `createCatalogPermissionRule` now expects `resourceType` as part of the supplied rule object. +- Introduce new `CatalogPermissionRule` convenience type. diff --git a/.changeset/sour-lobsters-sniff.md b/.changeset/sour-lobsters-sniff.md new file mode 100644 index 0000000000..fb8d1f2076 --- /dev/null +++ b/.changeset/sour-lobsters-sniff.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-permission-node': patch +--- + +Fix signature of permission rule in test suites diff --git a/.changeset/stale-carrots-smile.md b/.changeset/stale-carrots-smile.md new file mode 100644 index 0000000000..20b77dd834 --- /dev/null +++ b/.changeset/stale-carrots-smile.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-permission-common': patch +--- + +Add `isPermission` helper method. diff --git a/plugins/catalog-backend/api-report.md b/plugins/catalog-backend/api-report.md index 0df10cf944..bd99205189 100644 --- a/plugins/catalog-backend/api-report.md +++ b/plugins/catalog-backend/api-report.md @@ -25,6 +25,7 @@ import { PermissionRule } from '@backstage/plugin-permission-node'; import { PluginDatabaseManager } from '@backstage/backend-common'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { Readable } from 'stream'; +import { ResourcePermission } from '@backstage/plugin-permission-common'; import { Router } from 'express'; import { ScmIntegrationRegistry } from '@backstage/integration'; import { TokenManager } from '@backstage/backend-common'; @@ -109,13 +110,8 @@ export class BuiltinKindsEntityProcessor implements CatalogProcessor { export class CatalogBuilder { addEntityPolicy(...policies: EntityPolicy[]): CatalogBuilder; addEntityProvider(...providers: EntityProvider[]): CatalogBuilder; - addPermissionRules( - ...permissionRules: PermissionRule< - Entity, - EntitiesSearchFilter, - unknown[] - >[] - ): void; + // @alpha + addPermissionRules(...permissionRules: CatalogPermissionRule[]): void; addProcessor(...processors: CatalogProcessor[]): CatalogBuilder; build(): Promise<{ processingEngine: CatalogProcessingEngine; @@ -143,23 +139,37 @@ export const catalogConditions: Conditions<{ hasAnnotation: PermissionRule< Entity, EntitiesSearchFilter, + 'catalog-entity', [annotation: string] >; - hasLabel: PermissionRule; + hasLabel: PermissionRule< + Entity, + EntitiesSearchFilter, + 'catalog-entity', + [label: string] + >; hasMetadata: PermissionRule< Entity, EntitiesSearchFilter, + 'catalog-entity', [key: string, value?: string | undefined] >; hasSpec: PermissionRule< Entity, EntitiesSearchFilter, + 'catalog-entity', [key: string, value?: string | undefined] >; - isEntityKind: PermissionRule; + isEntityKind: PermissionRule< + Entity, + EntitiesSearchFilter, + 'catalog-entity', + [kinds: string[]] + >; isEntityOwner: PermissionRule< Entity, EntitiesSearchFilter, + 'catalog-entity', [claims: string[]] >; }>; @@ -173,6 +183,10 @@ export type CatalogEnvironment = { permissions: PermissionAuthorizer; }; +// @alpha +export type CatalogPermissionRule = + PermissionRule; + // @public (undocumented) export interface CatalogProcessingEngine { // (undocumented) @@ -277,14 +291,17 @@ export class CodeOwnersProcessor implements CatalogProcessor { } // @alpha -export const createCatalogPermissionRule: ( - rule: PermissionRule, -) => PermissionRule; +export const createCatalogConditionalDecision: ( + permission: ResourcePermission<'catalog-entity'>, + conditions: PermissionCriteria< + PermissionCondition<'catalog-entity', unknown[]> + >, +) => ConditionalPolicyDecision; // @alpha -export const createCatalogPolicyDecision: ( - conditions: PermissionCriteria>, -) => ConditionalPolicyDecision; +export const createCatalogPermissionRule: ( + rule: PermissionRule, +) => PermissionRule; // @public export function createRandomProcessingInterval(options: { @@ -469,23 +486,37 @@ export const permissionRules: { hasAnnotation: PermissionRule< Entity, EntitiesSearchFilter, + 'catalog-entity', [annotation: string] >; - hasLabel: PermissionRule; + hasLabel: PermissionRule< + Entity, + EntitiesSearchFilter, + 'catalog-entity', + [label: string] + >; hasMetadata: PermissionRule< Entity, EntitiesSearchFilter, + 'catalog-entity', [key: string, value?: string | undefined] >; hasSpec: PermissionRule< Entity, EntitiesSearchFilter, + 'catalog-entity', [key: string, value?: string | undefined] >; - isEntityKind: PermissionRule; + isEntityKind: PermissionRule< + Entity, + EntitiesSearchFilter, + 'catalog-entity', + [kinds: string[]] + >; isEntityOwner: PermissionRule< Entity, EntitiesSearchFilter, + 'catalog-entity', [claims: string[]] >; }; diff --git a/plugins/catalog-backend/src/permissions/conditionExports.ts b/plugins/catalog-backend/src/permissions/conditionExports.ts index 2846c8c3ec..1c1ba70631 100644 --- a/plugins/catalog-backend/src/permissions/conditionExports.ts +++ b/plugins/catalog-backend/src/permissions/conditionExports.ts @@ -18,41 +18,49 @@ import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; import { createConditionExports } from '@backstage/plugin-permission-node'; import { permissionRules } from './rules'; -const conditionExports = createConditionExports({ +const { conditions, createConditionalDecision } = createConditionExports({ pluginId: 'catalog', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, rules: permissionRules, }); /** - * These conditions are used when creating conditional decisions that are returned - * by authorization policies. + * These conditions are used when creating conditional decisions for catalog + * entities that are returned by authorization policies. * * @alpha */ -export const catalogConditions = conditionExports.conditions; +export const catalogConditions = conditions; /** - * `createCatalogPolicyDecision` can be used when authoring policies to create - * conditional decisions. + * `createCatalogConditionalDecision` can be used when authoring policies to + * create conditional decisions. It requires a permission of type + * `ResourcePermission<'catalog-entity'>` to be passed as the first parameter. + * It's recommended that you use the provided `isResourcePermission` and + * `isPermission` helper methods to narrow the type of the permission passed to + * the handle method as shown below. * * ``` * // MyAuthorizationPolicy.ts * ... * import { createCatalogPolicyDecision } from '@backstage/plugin-catalog-backend'; + * import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; * * class MyAuthorizationPolicy implements PermissionPolicy { * async handle(request, user) { * ... * - * return createCatalogPolicyDecision({ - * anyOf: [...insert conditions here...], - * }); - * } + * if (isResourcePermission(request.permission, RESOURCE_TYPE_CATALOG_ENTITY)) { + * return createCatalogConditionalDecision( + * request.permission, + * { anyOf: [...insert conditions here...] } + * ); + * } + * + * ... * } * ``` * * @alpha */ -export const createCatalogPolicyDecision = - conditionExports.createPolicyDecision; +export const createCatalogConditionalDecision = createConditionalDecision; diff --git a/plugins/catalog-backend/src/permissions/index.ts b/plugins/catalog-backend/src/permissions/index.ts index cefc17ed99..19d57de908 100644 --- a/plugins/catalog-backend/src/permissions/index.ts +++ b/plugins/catalog-backend/src/permissions/index.ts @@ -16,6 +16,6 @@ export { catalogConditions, - createCatalogPolicyDecision, + createCatalogConditionalDecision, } from './conditionExports'; export * from './rules'; diff --git a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts index 27a6033362..3808eced7b 100644 --- a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts +++ b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts @@ -16,12 +16,14 @@ import { get } from 'lodash'; import { Entity } from '@backstage/catalog-model'; +import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; import { createCatalogPermissionRule } from './util'; export const createPropertyRule = (propertyType: 'metadata' | 'spec') => createCatalogPermissionRule({ name: `HAS_${propertyType.toUpperCase()}`, description: `Allow entities which have the specified ${propertyType} subfield.`, + resourceType: RESOURCE_TYPE_CATALOG_ENTITY, apply: (resource: Entity, key: string, value?: string) => { const foundValue = get(resource[propertyType], key); if (value !== undefined) { diff --git a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts index c6c5951153..68ca65418d 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts @@ -15,6 +15,7 @@ */ import { Entity } from '@backstage/catalog-model'; +import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; import { createCatalogPermissionRule } from './util'; /** @@ -27,6 +28,7 @@ export const hasAnnotation = createCatalogPermissionRule({ name: 'HAS_ANNOTATION', description: 'Allow entities which are annotated with the specified annotation', + resourceType: RESOURCE_TYPE_CATALOG_ENTITY, apply: (resource: Entity, annotation: string) => !!resource.metadata.annotations?.hasOwnProperty(annotation), toQuery: (annotation: string) => ({ diff --git a/plugins/catalog-backend/src/permissions/rules/hasLabel.ts b/plugins/catalog-backend/src/permissions/rules/hasLabel.ts index 7296eecd89..9f37cbfb59 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasLabel.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasLabel.ts @@ -15,6 +15,7 @@ */ import { Entity } from '@backstage/catalog-model'; +import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; import { createCatalogPermissionRule } from './util'; /** @@ -25,6 +26,7 @@ import { createCatalogPermissionRule } from './util'; export const hasLabel = createCatalogPermissionRule({ name: 'HAS_LABEL', description: 'Allow entities which have the specified label metadata.', + resourceType: RESOURCE_TYPE_CATALOG_ENTITY, apply: (resource: Entity, label: string) => !!resource.metadata.labels?.hasOwnProperty(label), toQuery: (label: string) => ({ diff --git a/plugins/catalog-backend/src/permissions/rules/index.ts b/plugins/catalog-backend/src/permissions/rules/index.ts index f9d192781e..d3f405621a 100644 --- a/plugins/catalog-backend/src/permissions/rules/index.ts +++ b/plugins/catalog-backend/src/permissions/rules/index.ts @@ -36,4 +36,5 @@ export const permissionRules = { isEntityOwner, }; +export type { CatalogPermissionRule } from './util'; export { createCatalogPermissionRule } from './util'; diff --git a/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts b/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts index c330c903d9..3bd600e8fe 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts @@ -14,6 +14,7 @@ * limitations under the License. */ import { Entity } from '@backstage/catalog-model'; +import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; import { EntitiesSearchFilter } from '../../catalog/types'; import { createCatalogPermissionRule } from './util'; @@ -25,6 +26,7 @@ import { createCatalogPermissionRule } from './util'; export const isEntityKind = createCatalogPermissionRule({ name: 'IS_ENTITY_KIND', description: 'Allow entities with the specified kind', + resourceType: RESOURCE_TYPE_CATALOG_ENTITY, apply(resource: Entity, kinds: string[]) { const resourceKind = resource.kind.toLocaleLowerCase('en-US'); return kinds.some(kind => kind.toLocaleLowerCase('en-US') === resourceKind); diff --git a/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts b/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts index 23f118abc4..5403da2310 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts @@ -15,6 +15,7 @@ */ import { Entity, RELATION_OWNED_BY } from '@backstage/catalog-model'; +import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; import { createCatalogPermissionRule } from './util'; /** @@ -26,6 +27,7 @@ import { createCatalogPermissionRule } from './util'; export const isEntityOwner = createCatalogPermissionRule({ name: 'IS_ENTITY_OWNER', description: 'Allow entities owned by the current user', + resourceType: RESOURCE_TYPE_CATALOG_ENTITY, apply: (resource: Entity, claims: string[]) => { if (!resource.relations) { return false; diff --git a/plugins/catalog-backend/src/permissions/rules/util.ts b/plugins/catalog-backend/src/permissions/rules/util.ts index a1316c7fb2..36b351595d 100644 --- a/plugins/catalog-backend/src/permissions/rules/util.ts +++ b/plugins/catalog-backend/src/permissions/rules/util.ts @@ -15,9 +15,23 @@ */ import { Entity } from '@backstage/catalog-model'; -import { makeCreatePermissionRule } from '@backstage/plugin-permission-node'; +import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; +import { + makeCreatePermissionRule, + PermissionRule, +} from '@backstage/plugin-permission-node'; import { EntitiesSearchFilter } from '../../catalog/types'; +/** + * Convenience type for {@link @backstage/plugin-permission-node#PermissionRule} + * instances with the correct resource type, resource, and filter to work with + * the catalog. + * + * @alpha + */ +export type CatalogPermissionRule = + PermissionRule; + /** * Helper function for creating correctly-typed * {@link @backstage/plugin-permission-node#PermissionRule}s for the @@ -27,5 +41,6 @@ import { EntitiesSearchFilter } from '../../catalog/types'; */ export const createCatalogPermissionRule = makeCreatePermissionRule< Entity, - EntitiesSearchFilter + EntitiesSearchFilter, + typeof RESOURCE_TYPE_CATALOG_ENTITY >(); diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts index 1f0e5ea644..b29e1121b3 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts @@ -14,15 +14,11 @@ * limitations under the License. */ -import { Entity } from '@backstage/catalog-model'; import { NotAllowedError } from '@backstage/errors'; import { AuthorizeResult } from '@backstage/plugin-permission-common'; -import { - createConditionTransformer, - PermissionRule, -} from '@backstage/plugin-permission-node'; -import { EntitiesSearchFilter } from '../catalog/types'; +import { createConditionTransformer } from '@backstage/plugin-permission-node'; import { isEntityKind } from '../permissions/rules/isEntityKind'; +import { CatalogPermissionRule } from '../permissions/rules'; import { AuthorizedEntitiesCatalog } from './AuthorizedEntitiesCatalog'; describe('AuthorizedEntitiesCatalog', () => { @@ -36,9 +32,7 @@ describe('AuthorizedEntitiesCatalog', () => { authorize: jest.fn(), }; - const createCatalog = ( - ...rules: PermissionRule[] - ) => + const createCatalog = (...rules: CatalogPermissionRule[]) => new AuthorizedEntitiesCatalog( fakeCatalog, fakePermissionApi, diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index fb4ed6086f..99d7f0e41a 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -17,7 +17,6 @@ import { PluginDatabaseManager, UrlReader } from '@backstage/backend-common'; import { DefaultNamespaceEntityPolicy, - Entity, EntityPolicies, EntityPolicy, FieldFormatEntityPolicy, @@ -32,7 +31,6 @@ import { ScmIntegrations } from '@backstage/integration'; import { createHash } from 'crypto'; import { Router } from 'express'; import lodash, { keyBy } from 'lodash'; -import { EntitiesSearchFilter } from '../catalog'; import { CatalogProcessor, @@ -77,10 +75,12 @@ import { DefaultCatalogRulesEnforcer } from '../ingestion/CatalogRules'; import { Config } from '@backstage/config'; import { Logger } from 'winston'; import { connectEntityProviders } from '../processing/connectEntityProviders'; -import { permissionRules as catalogPermissionRules } from '../permissions/rules'; +import { + CatalogPermissionRule, + permissionRules as catalogPermissionRules, +} from '../permissions/rules'; import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; import { - PermissionRule, createConditionTransformer, createPermissionIntegrationRouter, } from '@backstage/plugin-permission-node'; @@ -135,11 +135,7 @@ export class CatalogBuilder { maxSeconds: 150, }); private locationAnalyzer: LocationAnalyzer | undefined = undefined; - private permissionRules: PermissionRule< - Entity, - EntitiesSearchFilter, - unknown[] - >[]; + private permissionRules: CatalogPermissionRule[]; /** * Creates a catalog builder. @@ -339,14 +335,9 @@ export class CatalogBuilder { * {@link @backstage/plugin-permission-node#PermissionRule}. * * @param permissionRules - Additional permission rules + * @alpha */ - addPermissionRules( - ...permissionRules: PermissionRule< - Entity, - EntitiesSearchFilter, - unknown[] - >[] - ) { + addPermissionRules(...permissionRules: CatalogPermissionRule[]) { this.permissionRules.push(...permissionRules); } diff --git a/plugins/catalog-backend/src/service/createRouter.test.ts b/plugins/catalog-backend/src/service/createRouter.test.ts index 1602d07d28..6def04a11c 100644 --- a/plugins/catalog-backend/src/service/createRouter.test.ts +++ b/plugins/catalog-backend/src/service/createRouter.test.ts @@ -26,7 +26,10 @@ import { LocationInput, LocationService, RefreshService } from './types'; import { basicEntityFilter } from './request'; import { createRouter } from './createRouter'; import { AuthorizeResult } from '@backstage/plugin-permission-common'; -import { createPermissionIntegrationRouter } from '@backstage/plugin-permission-node'; +import { + createPermissionIntegrationRouter, + createPermissionRule, +} from '@backstage/plugin-permission-node'; import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; describe('createRouter readonly disabled', () => { @@ -568,12 +571,13 @@ describe('NextRouter permissioning', () => { let app: express.Express; let refreshService: RefreshService; - const fakeRule = { + const fakeRule = createPermissionRule({ name: 'FAKE_RULE', description: 'fake rule', + resourceType: RESOURCE_TYPE_CATALOG_ENTITY, apply: () => true, toQuery: () => ({ key: '', values: [] }), - }; + }); beforeAll(async () => { entitiesCatalog = { @@ -631,7 +635,11 @@ describe('NextRouter permissioning', () => { id: '123', resourceType: 'catalog-entity', resourceRef: 'component:default/spidey-sense', - conditions: { rule: 'FAKE_RULE', params: ['user:default/spiderman'] }, + conditions: { + rule: 'FAKE_RULE', + resourceType: 'catalog-entity', + params: ['user:default/spiderman'], + }, }, ], }; diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts index 9981496658..451a6895a9 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts @@ -25,7 +25,10 @@ import { PermissionCondition, PermissionCriteria, } from '@backstage/plugin-permission-common'; -import { createPermissionIntegrationRouter } from '@backstage/plugin-permission-node'; +import { + createPermissionIntegrationRouter, + createPermissionRule, +} from '@backstage/plugin-permission-node'; import { PermissionIntegrationClient } from './PermissionIntegrationClient'; describe('PermissionIntegrationClient', () => { @@ -35,8 +38,8 @@ describe('PermissionIntegrationClient', () => { const mockConditions: PermissionCriteria = { not: { allOf: [ - { rule: 'RULE_1', params: [] }, - { rule: 'RULE_2', params: ['abc'] }, + { rule: 'RULE_1', resourceType: 'test-resource', params: [] }, + { rule: 'RULE_2', resourceType: 'test-resource', params: ['abc'] }, ], }, }; @@ -272,22 +275,24 @@ describe('PermissionIntegrationClient', () => { id: resourceRef, })), rules: [ - { + createPermissionRule({ name: 'RULE_1', description: 'Test rule 1', + resourceType: 'test-resource', apply: (_resource: any, input: 'yes' | 'no') => input === 'yes', toQuery: () => { throw new Error('Not implemented'); }, - }, - { + }), + createPermissionRule({ name: 'RULE_2', description: 'Test rule 2', + resourceType: 'test-resource', apply: (_resource: any, input: 'yes' | 'no') => input === 'yes', toQuery: () => { throw new Error('Not implemented'); }, - }, + }), ], }), ); @@ -336,7 +341,11 @@ describe('PermissionIntegrationClient', () => { id: '123', resourceRef: 'testResource1', resourceType: 'test-resource', - conditions: { rule: 'RULE_1', params: ['no'] }, + conditions: { + rule: 'RULE_1', + resourceType: 'test-resource', + params: ['no'], + }, }, ]), ).resolves.toEqual([{ id: '123', result: AuthorizeResult.DENY }]); @@ -353,15 +362,33 @@ describe('PermissionIntegrationClient', () => { allOf: [ { allOf: [ - { rule: 'RULE_1', params: ['yes'] }, - { not: { rule: 'RULE_2', params: ['no'] } }, + { + rule: 'RULE_1', + resourceType: 'test-resource', + params: ['yes'], + }, + { + not: { + rule: 'RULE_2', + resourceType: 'test-resource', + params: ['no'], + }, + }, ], }, { not: { allOf: [ - { rule: 'RULE_1', params: ['no'] }, - { rule: 'RULE_2', params: ['yes'] }, + { + rule: 'RULE_1', + resourceType: 'test-resource', + params: ['no'], + }, + { + rule: 'RULE_2', + resourceType: 'test-resource', + params: ['yes'], + }, ], }, }, diff --git a/plugins/permission-common/api-report.md b/plugins/permission-common/api-report.md index 07c1904183..ab0c68470d 100644 --- a/plugins/permission-common/api-report.md +++ b/plugins/permission-common/api-report.md @@ -89,6 +89,12 @@ export function isCreatePermission(permission: Permission): boolean; // @public export function isDeletePermission(permission: Permission): boolean; +// @public +export function isPermission( + permission: Permission, + comparedPermission: T, +): permission is T; + // @public export function isReadPermission(permission: Permission): boolean; @@ -141,7 +147,11 @@ export class PermissionClient implements PermissionAuthorizer { } // @public -export type PermissionCondition = { +export type PermissionCondition< + TResourceType extends string = string, + TParams extends unknown[] = unknown[], +> = { + resourceType: TResourceType; rule: string; params: TParams; }; diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 3027234b72..72f6b04567 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -41,6 +41,7 @@ const permissionCriteriaSchema: z.ZodSchema< z .object({ rule: z.string(), + resourceType: z.string(), params: z.array(z.unknown()), }) .strict() diff --git a/plugins/permission-common/src/permissions/util.ts b/plugins/permission-common/src/permissions/util.ts index 2843d97419..1a5ed434a7 100644 --- a/plugins/permission-common/src/permissions/util.ts +++ b/plugins/permission-common/src/permissions/util.ts @@ -16,6 +16,17 @@ import { Permission, ResourcePermission } from '../types'; +/** + * Check if the two parameters are equivalent permissions. + * @public + */ +export function isPermission( + permission: Permission, + comparedPermission: T, +): permission is T { + return permission.name === comparedPermission.name; +} + /** * Check if a given permission is a {@link ResourcePermission}. When * `resourceType` is supplied as the second parameter, also checks if diff --git a/plugins/permission-common/src/types/api.ts b/plugins/permission-common/src/types/api.ts index 66693ccf1b..6aaabf978d 100644 --- a/plugins/permission-common/src/types/api.ts +++ b/plugins/permission-common/src/types/api.ts @@ -98,7 +98,11 @@ export type PolicyDecision = * claims from a identity token. * @public */ -export type PermissionCondition = { +export type PermissionCondition< + TResourceType extends string = string, + TParams extends unknown[] = unknown[], +> = { + resourceType: TResourceType; rule: string; params: TParams; }; diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index fe4e70abf9..57379dd68b 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -21,6 +21,7 @@ import { PermissionCondition } from '@backstage/plugin-permission-common'; import { PermissionCriteria } from '@backstage/plugin-permission-common'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { PolicyDecision } from '@backstage/plugin-permission-common'; +import { ResourcePermission } from '@backstage/plugin-permission-common'; import { TokenManager } from '@backstage/backend-common'; // @public @@ -48,14 +49,15 @@ export type ApplyConditionsResponseEntry = export type Condition = TRule extends PermissionRule< any, any, + infer TResourceType, infer TParams > - ? (...params: TParams) => PermissionCondition + ? (...params: TParams) => PermissionCondition : never; // @public export type Conditions< - TRules extends Record>, + TRules extends Record>, > = { [Name in keyof TRules]: Condition; }; @@ -67,39 +69,49 @@ export type ConditionTransformer = ( // @public export const createConditionExports: < + TResourceType extends string, TResource, - TRules extends Record>, + TRules extends Record< + string, + PermissionRule + >, >(options: { pluginId: string; - resourceType: string; + resourceType: TResourceType; rules: TRules; }) => { conditions: Conditions; - createPolicyDecision: ( - conditions: PermissionCriteria, + createConditionalDecision: ( + permission: ResourcePermission, + conditions: PermissionCriteria< + PermissionCondition + >, ) => ConditionalPolicyDecision; }; // @public -export const createConditionFactory: ( - rule: PermissionRule, -) => (...params: TParams) => { - rule: string; - params: TParams; -}; +export const createConditionFactory: < + TResourceType extends string, + TParams extends any[], +>( + rule: PermissionRule, +) => (...params: TParams) => PermissionCondition; // @public export const createConditionTransformer: < TQuery, - TRules extends PermissionRule[], + TRules extends PermissionRule[], >( permissionRules: [...TRules], ) => ConditionTransformer; // @public -export const createPermissionIntegrationRouter: (options: { - resourceType: string; - rules: PermissionRule[]; +export const createPermissionIntegrationRouter: < + TResourceType extends string, + TResource, +>(options: { + resourceType: TResourceType; + rules: PermissionRule, unknown[]>[]; getResources: (resourceRefs: string[]) => Promise<(TResource | undefined)[]>; }) => express.Router; @@ -107,10 +119,11 @@ export const createPermissionIntegrationRouter: (options: { export const createPermissionRule: < TResource, TQuery, + TResourceType extends string, TParams extends unknown[], >( - rule: PermissionRule, -) => PermissionRule; + rule: PermissionRule, +) => PermissionRule; // @alpha export const isAndCriteria: ( @@ -128,11 +141,13 @@ export const isOrCriteria: ( ) => criteria is AnyOfCriteria; // @public -export const makeCreatePermissionRule: () => < - TParams extends unknown[], ->( - rule: PermissionRule, -) => PermissionRule; +export const makeCreatePermissionRule: < + TResource, + TQuery, + TResourceType extends string, +>() => ( + rule: PermissionRule, +) => PermissionRule; // @public export interface PermissionPolicy { @@ -147,10 +162,12 @@ export interface PermissionPolicy { export type PermissionRule< TResource, TQuery, + TResourceType extends string, TParams extends unknown[] = unknown[], > = { name: string; description: string; + resourceType: TResourceType; apply(resource: TResource, ...params: TParams): boolean; toQuery(...params: TParams): PermissionCriteria; }; diff --git a/plugins/permission-node/src/integration/createConditionExports.test.ts b/plugins/permission-node/src/integration/createConditionExports.test.ts index f3585d11ad..54d5652996 100644 --- a/plugins/permission-node/src/integration/createConditionExports.test.ts +++ b/plugins/permission-node/src/integration/createConditionExports.test.ts @@ -14,17 +14,22 @@ * limitations under the License. */ -import { AuthorizeResult } from '@backstage/plugin-permission-common'; +import { + AuthorizeResult, + createPermission, +} from '@backstage/plugin-permission-common'; import { createConditionExports } from './createConditionExports'; +import { createPermissionRule } from './createPermissionRule'; const testIntegration = () => createConditionExports({ pluginId: 'test-plugin', resourceType: 'test-resource', rules: { - testRule1: { + testRule1: createPermissionRule({ name: 'testRule1', description: 'Test rule 1', + resourceType: 'test-resource', apply: jest.fn( (_resource: any, _firstParam: string, _secondParam: number) => true, ), @@ -32,16 +37,17 @@ const testIntegration = () => query: 'testRule1', params: [firstParam, secondParam], })), - }, - testRule2: { + }), + testRule2: createPermissionRule({ name: 'testRule2', description: 'Test rule 2', - apply: jest.fn((_firstParam: object) => false), + resourceType: 'test-resource', + apply: jest.fn((_resource: any, _firstParam: object) => false), toQuery: jest.fn((firstParam: object) => ({ query: 'testRule2', params: [firstParam], })), - }, + }), }, }); @@ -52,11 +58,13 @@ describe('createConditionExports', () => { expect(conditions.testRule1('a', 1)).toEqual({ rule: 'testRule1', + resourceType: 'test-resource', params: ['a', 1], }); expect(conditions.testRule2({ baz: 'quux' })).toEqual({ rule: 'testRule2', + resourceType: 'test-resource', params: [{ baz: 'quux' }], }); }); @@ -64,18 +72,35 @@ describe('createConditionExports', () => { describe('createPolicyDecisions', () => { it('wraps conditions in an object with resourceType and pluginId', () => { - const { createPolicyDecision } = testIntegration(); + const { createConditionalDecision } = testIntegration(); + const testPermission = createPermission({ + name: 'test.permission', + attributes: {}, + resourceType: 'test-resource', + }); expect( - createPolicyDecision({ - allOf: [{ rule: 'testRule1', params: ['a', 1] }], + createConditionalDecision(testPermission, { + allOf: [ + { + rule: 'testRule1', + resourceType: 'test-resource', + params: ['a', 1], + }, + ], }), ).toEqual({ result: AuthorizeResult.CONDITIONAL, pluginId: 'test-plugin', resourceType: 'test-resource', conditions: { - allOf: [{ rule: 'testRule1', params: ['a', 1] }], + allOf: [ + { + rule: 'testRule1', + resourceType: 'test-resource', + params: ['a', 1], + }, + ], }, }); }); diff --git a/plugins/permission-node/src/integration/createConditionExports.ts b/plugins/permission-node/src/integration/createConditionExports.ts index ab88987389..ba34c07fed 100644 --- a/plugins/permission-node/src/integration/createConditionExports.ts +++ b/plugins/permission-node/src/integration/createConditionExports.ts @@ -19,6 +19,7 @@ import { ConditionalPolicyDecision, PermissionCondition, PermissionCriteria, + ResourcePermission, } from '@backstage/plugin-permission-common'; import { PermissionRule } from '../types'; import { createConditionFactory } from './createConditionFactory'; @@ -32,9 +33,10 @@ import { createConditionFactory } from './createConditionFactory'; export type Condition = TRule extends PermissionRule< any, any, + infer TResourceType, infer TParams > - ? (...params: TParams) => PermissionCondition + ? (...params: TParams) => PermissionCondition : never; /** @@ -44,39 +46,44 @@ export type Condition = TRule extends PermissionRule< * @public */ export type Conditions< - TRules extends Record>, + TRules extends Record>, > = { [Name in keyof TRules]: Condition; }; /** - * Creates the recommended condition-related exports for a given plugin based on the built-in - * {@link PermissionRule}s it supports. + * Creates the recommended condition-related exports for a given plugin based on + * the built-in {@link PermissionRule}s it supports. * * @remarks * * The function returns a `conditions` object containing a - * {@link @backstage/plugin-permission-common#PermissionCondition} factory for each of the - * supplied {@link PermissionRule}s, along with a `createConditions` function which builds the - * wrapper object needed to enclose conditions when authoring {@link PermissionPolicy} implementations. + * {@link @backstage/plugin-permission-common#PermissionCondition} factory for + * each of the supplied {@link PermissionRule}s, along with a + * `createConditionalDecision` function which builds the wrapper object needed + * to enclose conditions when authoring {@link PermissionPolicy} + * implementations. * - * Plugin authors should generally call this method with all the built-in {@link PermissionRule}s - * the plugin supports, and export the resulting `conditions` object and `createConditions` - * function so that they can be used by {@link PermissionPolicy} authors. + * Plugin authors should generally call this method with all the built-in + * {@link PermissionRule}s the plugin supports, and export the resulting + * `conditions` object and `createConditionalDecision` function so that they can + * be used by {@link PermissionPolicy} authors. * * @public */ export const createConditionExports = < + TResourceType extends string, TResource, - TRules extends Record>, + TRules extends Record>, >(options: { pluginId: string; - resourceType: string; + resourceType: TResourceType; rules: TRules; }): { conditions: Conditions; - createPolicyDecision: ( - conditions: PermissionCriteria, + createConditionalDecision: ( + permission: ResourcePermission, + conditions: PermissionCriteria>, ) => ConditionalPolicyDecision; } => { const { pluginId, resourceType, rules } = options; @@ -89,7 +96,8 @@ export const createConditionExports = < }), {} as Conditions, ), - createPolicyDecision: ( + createConditionalDecision: ( + _permission: ResourcePermission, conditions: PermissionCriteria, ) => ({ result: AuthorizeResult.CONDITIONAL, diff --git a/plugins/permission-node/src/integration/createConditionFactory.test.ts b/plugins/permission-node/src/integration/createConditionFactory.test.ts index 8dd43f5da6..1941b5ac0b 100644 --- a/plugins/permission-node/src/integration/createConditionFactory.test.ts +++ b/plugins/permission-node/src/integration/createConditionFactory.test.ts @@ -15,14 +15,16 @@ */ import { createConditionFactory } from './createConditionFactory'; +import { createPermissionRule } from './createPermissionRule'; describe('createConditionFactory', () => { - const testRule = { + const testRule = createPermissionRule({ name: 'test-rule', description: 'test-description', + resourceType: 'test-resource', apply: jest.fn(), toQuery: jest.fn(), - }; + }); it('returns a function', () => { expect(createConditionFactory(testRule)).toEqual(expect.any(Function)); @@ -33,6 +35,7 @@ describe('createConditionFactory', () => { const conditionFactory = createConditionFactory(testRule); expect(conditionFactory('a', 'b', 1, 2)).toEqual({ rule: 'test-rule', + resourceType: 'test-resource', params: ['a', 'b', 1, 2], }); }); diff --git a/plugins/permission-node/src/integration/createConditionFactory.ts b/plugins/permission-node/src/integration/createConditionFactory.ts index 84a8dc86a6..15a5b4fd08 100644 --- a/plugins/permission-node/src/integration/createConditionFactory.ts +++ b/plugins/permission-node/src/integration/createConditionFactory.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { PermissionCondition } from '@backstage/plugin-permission-common'; import { PermissionRule } from '../types'; /** @@ -33,8 +34,11 @@ import { PermissionRule } from '../types'; * @public */ export const createConditionFactory = - (rule: PermissionRule) => - (...params: TParams) => ({ + ( + rule: PermissionRule, + ) => + (...params: TParams): PermissionCondition => ({ rule: rule.name, + resourceType: rule.resourceType, params, }); diff --git a/plugins/permission-node/src/integration/createConditionTransformer.test.ts b/plugins/permission-node/src/integration/createConditionTransformer.test.ts index 8a93a4ea27..917449561d 100644 --- a/plugins/permission-node/src/integration/createConditionTransformer.test.ts +++ b/plugins/permission-node/src/integration/createConditionTransformer.test.ts @@ -19,25 +19,28 @@ import { PermissionCriteria, } from '@backstage/plugin-permission-common'; import { createConditionTransformer } from './createConditionTransformer'; +import { createPermissionRule } from './createPermissionRule'; const transformConditions = createConditionTransformer([ - { + createPermissionRule({ name: 'test-rule-1', description: 'Test rule 1', + resourceType: 'test-resource', apply: jest.fn(), toQuery: jest.fn( (firstParam: string, secondParam: number) => `test-rule-1:${firstParam}/${secondParam}`, ), - }, - { + }), + createPermissionRule({ name: 'test-rule-2', description: 'Test rule 2', + resourceType: 'test-resource', apply: jest.fn(), toQuery: jest.fn( (firstParam: object) => `test-rule-2:${JSON.stringify(firstParam)}`, ), - }, + }), ]); describe('createConditionTransformer', () => { @@ -46,18 +49,30 @@ describe('createConditionTransformer', () => { expectedResult: PermissionCriteria; }[] = [ { - conditions: { rule: 'test-rule-1', params: ['abc', 123] }, + conditions: { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['abc', 123], + }, expectedResult: 'test-rule-1:abc/123', }, { - conditions: { rule: 'test-rule-2', params: [{ foo: 0 }] }, + conditions: { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [{ foo: 0 }], + }, expectedResult: 'test-rule-2:{"foo":0}', }, { conditions: { anyOf: [ - { rule: 'test-rule-1', params: ['a', 1] }, - { rule: 'test-rule-2', params: [{}] }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['a', 1], + }, + { rule: 'test-rule-2', resourceType: 'test-resource', params: [{}] }, ], }, expectedResult: { @@ -67,8 +82,12 @@ describe('createConditionTransformer', () => { { conditions: { allOf: [ - { rule: 'test-rule-1', params: ['a', 1] }, - { rule: 'test-rule-2', params: [{}] }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['a', 1], + }, + { rule: 'test-rule-2', resourceType: 'test-resource', params: [{}] }, ], }, expectedResult: { @@ -77,7 +96,11 @@ describe('createConditionTransformer', () => { }, { conditions: { - not: { rule: 'test-rule-2', params: [{}] }, + not: { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [{}], + }, }, expectedResult: { not: 'test-rule-2:{}', @@ -88,15 +111,31 @@ describe('createConditionTransformer', () => { allOf: [ { anyOf: [ - { rule: 'test-rule-1', params: ['a', 1] }, - { rule: 'test-rule-2', params: [{}] }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['a', 1], + }, + { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [{}], + }, ], }, { not: { allOf: [ - { rule: 'test-rule-1', params: ['b', 2] }, - { rule: 'test-rule-2', params: [{ c: 3 }] }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['b', 2], + }, + { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [{ c: 3 }], + }, ], }, }, @@ -120,15 +159,33 @@ describe('createConditionTransformer', () => { allOf: [ { anyOf: [ - { rule: 'test-rule-1', params: ['a', 1] }, - { rule: 'test-rule-2', params: [{ b: 2 }] }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['a', 1], + }, + { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [{ b: 2 }], + }, ], }, { not: { allOf: [ - { rule: 'test-rule-1', params: ['c', 3] }, - { not: { rule: 'test-rule-2', params: [{ d: 4 }] } }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['c', 3], + }, + { + not: { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [{ d: 4 }], + }, + }, ], }, }, diff --git a/plugins/permission-node/src/integration/createConditionTransformer.ts b/plugins/permission-node/src/integration/createConditionTransformer.ts index ee4224b8bd..112bb30d01 100644 --- a/plugins/permission-node/src/integration/createConditionTransformer.ts +++ b/plugins/permission-node/src/integration/createConditionTransformer.ts @@ -29,7 +29,7 @@ import { const mapConditions = ( criteria: PermissionCriteria, - getRule: (name: string) => PermissionRule, + getRule: (name: string) => PermissionRule, ): PermissionCriteria => { if (isAndCriteria(criteria)) { return { @@ -70,7 +70,7 @@ export type ConditionTransformer = ( */ export const createConditionTransformer = < TQuery, - TRules extends PermissionRule[], + TRules extends PermissionRule[], >( permissionRules: [...TRules], ): ConditionTransformer => { diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 6408de46a1..65c43c7f30 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -18,6 +18,7 @@ import { AuthorizeResult } from '@backstage/plugin-permission-common'; import express, { Express, Router } from 'express'; import request, { Response } from 'supertest'; import { createPermissionIntegrationRouter } from './createPermissionIntegrationRouter'; +import { createPermissionRule } from './createPermissionRule'; const mockGetResources: jest.MockedFunction< Parameters[0]['getResources'] @@ -25,21 +26,23 @@ const mockGetResources: jest.MockedFunction< resourceRefs.map(resourceRef => ({ id: resourceRef })), ); -const testRule1 = { +const testRule1 = createPermissionRule({ name: 'test-rule-1', description: 'Test rule 1', + resourceType: 'test-resource', apply: jest.fn( (_resource: any, _firstParam: string, _secondParam: number) => true, ), toQuery: jest.fn(), -}; +}); -const testRule2 = { +const testRule2 = createPermissionRule({ name: 'test-rule-2', description: 'Test rule 2', - apply: jest.fn((_firstParam: object) => false), + resourceType: 'test-resource', + apply: jest.fn((_resource: any, _firstParam: object) => false), toQuery: jest.fn(), -}; +}); describe('createPermissionIntegrationRouter', () => { let app: Express; @@ -65,29 +68,57 @@ describe('createPermissionIntegrationRouter', () => { describe('POST /.well-known/backstage/permissions/apply-conditions', () => { it.each([ - { rule: 'test-rule-1', params: ['abc', 123] }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['abc', 123], + }, { anyOf: [ - { rule: 'test-rule-1', params: ['a', 1] }, - { rule: 'test-rule-2', params: [{}] }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['a', 1], + }, + { rule: 'test-rule-2', resourceType: 'test-resource', params: [{}] }, ], }, { - not: { rule: 'test-rule-2', params: [{}] }, + not: { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [{}], + }, }, { allOf: [ { anyOf: [ - { rule: 'test-rule-1', params: ['a', 1] }, - { rule: 'test-rule-2', params: [{}] }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['a', 1], + }, + { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [{}], + }, ], }, { not: { allOf: [ - { rule: 'test-rule-1', params: ['b', 2] }, - { rule: 'test-rule-2', params: [{ c: 3 }] }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['b', 2], + }, + { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [{ c: 3 }], + }, ], }, }, @@ -119,26 +150,52 @@ describe('createPermissionIntegrationRouter', () => { }); it.each([ - { rule: 'test-rule-2', params: [{ foo: 0 }] }, + { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [{ foo: 0 }], + }, { allOf: [ - { rule: 'test-rule-1', params: ['a', 1] }, - { rule: 'test-rule-2', params: [{}] }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['a', 1], + }, + { rule: 'test-rule-2', resourceType: 'test-resource', params: [{}] }, ], }, { allOf: [ { anyOf: [ - { rule: 'test-rule-1', params: ['a', 1] }, - { rule: 'test-rule-2', params: [{ b: 2 }] }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['a', 1], + }, + { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [{ b: 2 }], + }, ], }, { not: { allOf: [ - { rule: 'test-rule-1', params: ['c', 3] }, - { not: { rule: 'test-rule-2', params: [{ d: 4 }] } }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: ['c', 3], + }, + { + not: { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [{ d: 4 }], + }, + }, ], }, }, @@ -179,25 +236,45 @@ describe('createPermissionIntegrationRouter', () => { id: '123', resourceRef: 'default:test/resource-1', resourceType: 'test-resource', - conditions: { rule: 'test-rule-1', params: [] }, + conditions: { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: [], + }, }, { id: '234', resourceRef: 'default:test/resource-1', resourceType: 'test-resource', - conditions: { rule: 'test-rule-2', params: [] }, + conditions: { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [], + }, }, { id: '345', resourceRef: 'default:test/resource-2', resourceType: 'test-resource', - conditions: { not: { rule: 'test-rule-1', params: [] } }, + conditions: { + not: { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: [], + }, + }, }, { id: '456', resourceRef: 'default:test/resource-3', resourceType: 'test-resource', - conditions: { not: { rule: 'test-rule-2', params: [] } }, + conditions: { + not: { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [], + }, + }, }, { id: '567', @@ -205,8 +282,16 @@ describe('createPermissionIntegrationRouter', () => { resourceType: 'test-resource', conditions: { anyOf: [ - { rule: 'test-rule-1', params: [] }, - { rule: 'test-rule-2', params: [] }, + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: [], + }, + { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: [], + }, ], }, }, @@ -248,6 +333,7 @@ describe('createPermissionIntegrationRouter', () => { resourceType: 'test-incorrect-resource-1', conditions: { rule: 'test-rule-1', + resourceType: 'test-incorrect-resource-1', params: [{}], }, }, @@ -257,6 +343,7 @@ describe('createPermissionIntegrationRouter', () => { resourceType: 'test-resource', conditions: { rule: 'test-rule-1', + resourceType: 'test-resource', params: [{}], }, }, @@ -266,6 +353,7 @@ describe('createPermissionIntegrationRouter', () => { resourceType: 'test-incorrect-resource-2', conditions: { rule: 'test-rule-1', + resourceType: 'test-incorrect-resource-2', params: [{}], }, }, @@ -291,7 +379,11 @@ describe('createPermissionIntegrationRouter', () => { id: '123', resourceRef: 'default:test/resource', resourceType: 'test-resource', - conditions: { rule: 'test-rule-1', params: [] }, + conditions: { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: [], + }, }, ], }); @@ -324,19 +416,31 @@ describe('createPermissionIntegrationRouter', () => { id: '123', resourceRef: 'default:test/resource-1', resourceType: 'test-resource', - conditions: { rule: 'test-rule-1', params: [] }, + conditions: { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: [], + }, }, { id: '234', resourceRef: 'default:test/missing-resource', resourceType: 'test-resource', - conditions: { rule: 'test-rule-1', params: [] }, + conditions: { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: [], + }, }, { id: '345', resourceRef: 'default:test/resource-2', resourceType: 'test-resource', - conditions: { rule: 'test-rule-1', params: [] }, + conditions: { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: [], + }, }, ], }); @@ -377,6 +481,17 @@ describe('createPermissionIntegrationRouter', () => { ], }, { items: [{ conditions: { anyOf: [] } }] }, + { + items: [ + { conditions: { rule: 'TEST_RULE', resourceType: 'test-resource' } }, + ], + }, + { items: [{ conditions: { rule: 'TEST_RULE', params: ['foo'] } }] }, + { + items: [ + { conditions: { resourceType: 'test-resource', params: ['foo'] } }, + ], + }, ])(`returns 400 for invalid input %#`, async input => { const response = await request(app) .post('/.well-known/backstage/permissions/apply-conditions') diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index b598487062..b4cf4ac08c 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -44,6 +44,7 @@ const permissionCriteriaSchema: z.ZodSchema< z .object({ rule: z.string(), + resourceType: z.string(), params: z.array(z.unknown()), }) .strict(), @@ -100,10 +101,10 @@ export type ApplyConditionsResponse = { items: ApplyConditionsResponseEntry[]; }; -const applyConditions = ( - criteria: PermissionCriteria, +const applyConditions = ( + criteria: PermissionCriteria>, resource: TResource | undefined, - getRule: (name: string) => PermissionRule, + getRule: (name: string) => PermissionRule, ): boolean => { // If resource was not found, deny. This avoids leaking information from the // apply-conditions API which would allow a user to differentiate between @@ -127,6 +128,14 @@ const applyConditions = ( return getRule(criteria.rule).apply(resource, ...criteria.params); }; +/** + * Prevent use of type parameter from contributing to type inference. + * + * https://github.com/Microsoft/TypeScript/issues/14829#issuecomment-980401795 + * @ignore + */ +type NoInfer = T extends infer S ? S : never; + /** * Create an express Router which provides an authorization route to allow * integration between the permission backend and other Backstage backend @@ -162,9 +171,16 @@ const applyConditions = ( * * @public */ -export const createPermissionIntegrationRouter = (options: { - resourceType: string; - rules: PermissionRule[]; +export const createPermissionIntegrationRouter = < + TResourceType extends string, + TResource, +>(options: { + resourceType: TResourceType; + // Do not infer value of TResourceType from supplied rules. + // instead only consider the resourceType parameter, and + // consider any rules whose resource type does not match + // to be an error. + rules: PermissionRule>[]; getResources: ( resourceRefs: string[], ) => Promise>; diff --git a/plugins/permission-node/src/integration/createPermissionRule.ts b/plugins/permission-node/src/integration/createPermissionRule.ts index f3a0519a70..6673fe76de 100644 --- a/plugins/permission-node/src/integration/createPermissionRule.ts +++ b/plugins/permission-node/src/integration/createPermissionRule.ts @@ -24,9 +24,10 @@ import { PermissionRule } from '../types'; export const createPermissionRule = < TResource, TQuery, + TResourceType extends string, TParams extends unknown[], >( - rule: PermissionRule, + rule: PermissionRule, ) => rule; /** @@ -38,8 +39,8 @@ export const createPermissionRule = < * @public */ export const makeCreatePermissionRule = - () => + () => ( - rule: PermissionRule, + rule: PermissionRule, ) => createPermissionRule(rule); diff --git a/plugins/permission-node/src/integration/util.test.ts b/plugins/permission-node/src/integration/util.test.ts index d27c0a4243..17e27b6028 100644 --- a/plugins/permission-node/src/integration/util.test.ts +++ b/plugins/permission-node/src/integration/util.test.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { createPermissionRule } from './createPermissionRule'; import { createGetRule, isAndCriteria, @@ -25,19 +26,21 @@ describe('permission integration utils', () => { describe('createGetRule', () => { let getRule: ReturnType; - const testRule1 = { + const testRule1 = createPermissionRule({ name: 'test-rule-1', description: 'Test rule 1', + resourceType: 'test-resource', apply: jest.fn(), toQuery: jest.fn(), - }; + }); - const testRule2 = { + const testRule2 = createPermissionRule({ name: 'test-rule-2', description: 'Test rule 2', + resourceType: 'test-resource', apply: jest.fn(), toQuery: jest.fn(), - }; + }); beforeEach(() => { getRule = createGetRule([testRule1, testRule2]); diff --git a/plugins/permission-node/src/integration/util.ts b/plugins/permission-node/src/integration/util.ts index 9102f092c4..68b7fff2b8 100644 --- a/plugins/permission-node/src/integration/util.ts +++ b/plugins/permission-node/src/integration/util.ts @@ -62,11 +62,11 @@ export const isNotCriteria = ( Object.prototype.hasOwnProperty.call(criteria, 'not'); export const createGetRule = ( - rules: PermissionRule[], + rules: PermissionRule[], ) => { const rulesMap = new Map(Object.values(rules).map(rule => [rule.name, rule])); - return (name: string): PermissionRule => { + return (name: string): PermissionRule => { const rule = rulesMap.get(name); if (!rule) { diff --git a/plugins/permission-node/src/types.ts b/plugins/permission-node/src/types.ts index d5035f10dc..66b4a7b22b 100644 --- a/plugins/permission-node/src/types.ts +++ b/plugins/permission-node/src/types.ts @@ -35,10 +35,12 @@ import type { PermissionCriteria } from '@backstage/plugin-permission-common'; export type PermissionRule< TResource, TQuery, + TResourceType extends string, TParams extends unknown[] = unknown[], > = { name: string; description: string; + resourceType: TResourceType; /** * Apply this rule to a resource already loaded from a backing data source. The params are