From e9b41910717694bd30c0cdf6960068b2a08c7013 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Wed, 28 Sep 2022 15:07:28 +0100 Subject: [PATCH 01/33] Added parameter scheamas to permission rules Signed-off-by: Harry Hogg --- .../src/permissions/rules/createPropertyRule.ts | 5 +++++ .../catalog-backend/src/permissions/rules/hasAnnotation.ts | 5 +++++ plugins/catalog-backend/src/permissions/rules/hasLabel.ts | 2 ++ .../catalog-backend/src/permissions/rules/isEntityKind.ts | 2 ++ .../catalog-backend/src/permissions/rules/isEntityOwner.ts | 2 ++ plugins/catalog-backend/src/service/createRouter.test.ts | 2 ++ .../src/service/PermissionIntegrationClient.test.ts | 3 +++ .../src/integration/createConditionExports.test.ts | 3 +++ .../src/integration/createConditionFactory.test.ts | 2 ++ .../src/integration/createConditionTransformer.test.ts | 3 +++ .../integration/createPermissionIntegrationRouter.test.ts | 3 +++ plugins/permission-node/src/integration/util.test.ts | 3 +++ plugins/permission-node/src/types.ts | 6 ++++++ plugins/playlist-backend/package.json | 3 ++- plugins/playlist-backend/src/permissions/rules.ts | 3 +++ yarn.lock | 1 + 16 files changed, 47 insertions(+), 1 deletion(-) diff --git a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts index 57de5eb922..12725c1fcd 100644 --- a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts +++ b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts @@ -18,12 +18,17 @@ import { get } from 'lodash'; import { Entity } from '@backstage/catalog-model'; import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; import { createCatalogPermissionRule } from './util'; +import { z } from 'zod'; 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, + schema: z.tuple([ + z.string().describe('Property name'), + z.string().optional().describe('Property value'), + ]), apply: (resource: Entity, key: string, value?: string) => { const foundValue = get(resource[propertyType], key); diff --git a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts index 22dbd307a6..4042c34636 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts @@ -16,6 +16,7 @@ import { Entity } from '@backstage/catalog-model'; import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; +import { z } from 'zod'; import { createCatalogPermissionRule } from './util'; /** @@ -31,6 +32,10 @@ export const hasAnnotation = createCatalogPermissionRule({ description: 'Allow entities which are annotated with the specified annotation', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, + schema: z.tuple([ + z.string().describe('Annotation name'), + z.string().optional().describe('Annotation value'), + ]), apply: (resource: Entity, annotation: string, value?: string) => !!resource.metadata.annotations?.hasOwnProperty(annotation) && (value === undefined diff --git a/plugins/catalog-backend/src/permissions/rules/hasLabel.ts b/plugins/catalog-backend/src/permissions/rules/hasLabel.ts index 9f37cbfb59..3d1a18f8b3 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasLabel.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasLabel.ts @@ -16,6 +16,7 @@ import { Entity } from '@backstage/catalog-model'; import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; +import { z } from 'zod'; import { createCatalogPermissionRule } from './util'; /** @@ -27,6 +28,7 @@ export const hasLabel = createCatalogPermissionRule({ name: 'HAS_LABEL', description: 'Allow entities which have the specified label metadata.', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, + schema: z.tuple([z.string().describe('Label name')]), apply: (resource: Entity, label: string) => !!resource.metadata.labels?.hasOwnProperty(label), toQuery: (label: string) => ({ diff --git a/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts b/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts index 3bd600e8fe..70467558dd 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts @@ -15,6 +15,7 @@ */ import { Entity } from '@backstage/catalog-model'; import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; +import { z } from 'zod'; import { EntitiesSearchFilter } from '../../catalog/types'; import { createCatalogPermissionRule } from './util'; @@ -27,6 +28,7 @@ export const isEntityKind = createCatalogPermissionRule({ name: 'IS_ENTITY_KIND', description: 'Allow entities with the specified kind', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, + schema: z.tuple([z.array(z.string().describe('List of entity kinds'))]), 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 5403da2310..c0cc7c9911 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts @@ -16,6 +16,7 @@ import { Entity, RELATION_OWNED_BY } from '@backstage/catalog-model'; import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; +import { z } from 'zod'; import { createCatalogPermissionRule } from './util'; /** @@ -28,6 +29,7 @@ export const isEntityOwner = createCatalogPermissionRule({ name: 'IS_ENTITY_OWNER', description: 'Allow entities owned by the current user', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, + schema: z.tuple([z.array(z.string().describe('List of owners'))]), apply: (resource: Entity, claims: string[]) => { if (!resource.relations) { return false; diff --git a/plugins/catalog-backend/src/service/createRouter.test.ts b/plugins/catalog-backend/src/service/createRouter.test.ts index 16a36eb8c4..163e5e0747 100644 --- a/plugins/catalog-backend/src/service/createRouter.test.ts +++ b/plugins/catalog-backend/src/service/createRouter.test.ts @@ -36,6 +36,7 @@ import { } from '@backstage/plugin-permission-node'; import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; import { CatalogProcessingOrchestrator } from '../processing/types'; +import { z } from 'zod'; describe('createRouter readonly disabled', () => { let entitiesCatalog: jest.Mocked; @@ -695,6 +696,7 @@ describe('NextRouter permissioning', () => { name: 'FAKE_RULE', description: 'fake rule', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, + schema: z.tuple([]), apply: () => true, toQuery: () => ({ key: '', values: [] }), }); diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts index 4c50e1646f..6c05a825ab 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts @@ -30,6 +30,7 @@ import { createPermissionRule, } from '@backstage/plugin-permission-node'; import { PermissionIntegrationClient } from './PermissionIntegrationClient'; +import { z } from 'zod'; describe('PermissionIntegrationClient', () => { describe('applyConditions', () => { @@ -279,6 +280,7 @@ describe('PermissionIntegrationClient', () => { name: 'RULE_1', description: 'Test rule 1', resourceType: 'test-resource', + schema: z.tuple([z.enum(['yes', 'no'])]), apply: (_resource: any, input: 'yes' | 'no') => input === 'yes', toQuery: () => { throw new Error('Not implemented'); @@ -288,6 +290,7 @@ describe('PermissionIntegrationClient', () => { name: 'RULE_2', description: 'Test rule 2', resourceType: 'test-resource', + schema: z.tuple([z.enum(['yes', 'no'])]), apply: (_resource: any, input: 'yes' | 'no') => input === 'yes', toQuery: () => { throw new Error('Not implemented'); diff --git a/plugins/permission-node/src/integration/createConditionExports.test.ts b/plugins/permission-node/src/integration/createConditionExports.test.ts index 54d5652996..ed0ab49b5c 100644 --- a/plugins/permission-node/src/integration/createConditionExports.test.ts +++ b/plugins/permission-node/src/integration/createConditionExports.test.ts @@ -18,6 +18,7 @@ import { AuthorizeResult, createPermission, } from '@backstage/plugin-permission-common'; +import { z } from 'zod'; import { createConditionExports } from './createConditionExports'; import { createPermissionRule } from './createPermissionRule'; @@ -30,6 +31,7 @@ const testIntegration = () => name: 'testRule1', description: 'Test rule 1', resourceType: 'test-resource', + schema: z.tuple([z.string(), z.number()]), apply: jest.fn( (_resource: any, _firstParam: string, _secondParam: number) => true, ), @@ -42,6 +44,7 @@ const testIntegration = () => name: 'testRule2', description: 'Test rule 2', resourceType: 'test-resource', + schema: z.tuple([z.object({})]), apply: jest.fn((_resource: any, _firstParam: object) => false), toQuery: jest.fn((firstParam: object) => ({ query: 'testRule2', diff --git a/plugins/permission-node/src/integration/createConditionFactory.test.ts b/plugins/permission-node/src/integration/createConditionFactory.test.ts index 1941b5ac0b..433d37b3f6 100644 --- a/plugins/permission-node/src/integration/createConditionFactory.test.ts +++ b/plugins/permission-node/src/integration/createConditionFactory.test.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { z } from 'zod'; import { createConditionFactory } from './createConditionFactory'; import { createPermissionRule } from './createPermissionRule'; @@ -22,6 +23,7 @@ describe('createConditionFactory', () => { name: 'test-rule', description: 'test-description', resourceType: 'test-resource', + schema: z.tuple([]), apply: jest.fn(), toQuery: jest.fn(), }); diff --git a/plugins/permission-node/src/integration/createConditionTransformer.test.ts b/plugins/permission-node/src/integration/createConditionTransformer.test.ts index 917449561d..421bbcb2f4 100644 --- a/plugins/permission-node/src/integration/createConditionTransformer.test.ts +++ b/plugins/permission-node/src/integration/createConditionTransformer.test.ts @@ -18,6 +18,7 @@ import { PermissionCondition, PermissionCriteria, } from '@backstage/plugin-permission-common'; +import { z } from 'zod'; import { createConditionTransformer } from './createConditionTransformer'; import { createPermissionRule } from './createPermissionRule'; @@ -26,6 +27,7 @@ const transformConditions = createConditionTransformer([ name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', + schema: z.tuple([]), apply: jest.fn(), toQuery: jest.fn( (firstParam: string, secondParam: number) => @@ -36,6 +38,7 @@ const transformConditions = createConditionTransformer([ name: 'test-rule-2', description: 'Test rule 2', resourceType: 'test-resource', + schema: z.tuple([]), apply: jest.fn(), toQuery: jest.fn( (firstParam: object) => `test-rule-2:${JSON.stringify(firstParam)}`, diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index ac4005c487..829b1b3614 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -21,6 +21,7 @@ import { } from '@backstage/plugin-permission-common'; import express, { Express, Router } from 'express'; import request, { Response } from 'supertest'; +import { z } from 'zod'; import { createPermissionIntegrationRouter } from './createPermissionIntegrationRouter'; import { createPermissionRule } from './createPermissionRule'; @@ -39,6 +40,7 @@ const testRule1 = createPermissionRule({ name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', + schema: z.tuple([z.string(), z.number()]), apply: (_resource: any, _firstParam: string, _secondParam: number) => true, toQuery: (_firstParam: string, _secondParam: number) => ({}), }); @@ -47,6 +49,7 @@ const testRule2 = createPermissionRule({ name: 'test-rule-2', description: 'Test rule 2', resourceType: 'test-resource', + schema: z.tuple([z.object({})]), apply: (_resource: any, _firstParam: object) => false, toQuery: (_firstParam: object) => ({}), }); diff --git a/plugins/permission-node/src/integration/util.test.ts b/plugins/permission-node/src/integration/util.test.ts index 6c1c270953..1572964a7d 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 { z } from 'zod'; import { createPermissionRule } from './createPermissionRule'; import { createGetRule, @@ -30,6 +31,7 @@ describe('permission integration utils', () => { name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', + schema: z.tuple([]), apply: jest.fn(), toQuery: jest.fn(), }); @@ -38,6 +40,7 @@ describe('permission integration utils', () => { name: 'test-rule-2', description: 'Test rule 2', resourceType: 'test-resource', + schema: z.tuple([]), apply: jest.fn(), toQuery: jest.fn(), }); diff --git a/plugins/permission-node/src/types.ts b/plugins/permission-node/src/types.ts index 66b4a7b22b..7beb5bb92d 100644 --- a/plugins/permission-node/src/types.ts +++ b/plugins/permission-node/src/types.ts @@ -15,6 +15,7 @@ */ import type { PermissionCriteria } from '@backstage/plugin-permission-common'; +import { z } from 'zod'; /** * A conditional rule that can be provided in an @@ -42,6 +43,11 @@ export type PermissionRule< description: string; resourceType: TResourceType; + /** + * A ZodSchema that documents the parameters that this rule accepts. + */ + schema: z.ZodSchema; + /** * Apply this rule to a resource already loaded from a backing data source. The params are * arguments supplied for the rule; for example, a rule could be `isOwner` with entityRefs as the diff --git a/plugins/playlist-backend/package.json b/plugins/playlist-backend/package.json index 9e4ab86020..bee5d292b9 100644 --- a/plugins/playlist-backend/package.json +++ b/plugins/playlist-backend/package.json @@ -39,7 +39,8 @@ "node-fetch": "^2.6.7", "uuid": "^8.2.0", "winston": "^3.2.1", - "yn": "^4.0.0" + "yn": "^4.0.0", + "zod": "^3.11.6" }, "devDependencies": { "@backstage/cli": "workspace:^", diff --git a/plugins/playlist-backend/src/permissions/rules.ts b/plugins/playlist-backend/src/permissions/rules.ts index cbf81bee65..de2d3df5e4 100644 --- a/plugins/playlist-backend/src/permissions/rules.ts +++ b/plugins/playlist-backend/src/permissions/rules.ts @@ -19,6 +19,7 @@ import { PLAYLIST_LIST_RESOURCE_TYPE, PlaylistMetadata, } from '@backstage/plugin-playlist-common'; +import { z } from 'zod'; import { ListPlaylistsFilter } from '../service'; @@ -32,6 +33,7 @@ const isOwner = createPlaylistPermissionRule({ name: 'IS_OWNER', description: 'Should allow only if the playlist belongs to the user', resourceType: PLAYLIST_LIST_RESOURCE_TYPE, + schema: z.tuple([z.array(z.string().describe('List of owners'))]), apply: (list: PlaylistMetadata, userOwnershipRefs: string[]) => userOwnershipRefs.includes(list.owner), toQuery: (userOwnershipRefs: string[]) => ({ @@ -44,6 +46,7 @@ const isPublic = createPlaylistPermissionRule({ name: 'IS_PUBLIC', description: 'Should allow only if the playlist is public', resourceType: PLAYLIST_LIST_RESOURCE_TYPE, + schema: z.tuple([]), apply: (list: PlaylistMetadata) => list.public, toQuery: () => ({ key: 'public', values: [true] }), }); diff --git a/yarn.lock b/yarn.lock index e5f10c942a..a6bb179e7c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6427,6 +6427,7 @@ __metadata: uuid: ^8.2.0 winston: ^3.2.1 yn: ^4.0.0 + zod: ^3.11.6 languageName: unknown linkType: soft From 9fe88c4fab903b293a43f0bfdc87469dc05fa359 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Wed, 28 Sep 2022 15:08:43 +0100 Subject: [PATCH 02/33] Added parameter validation using the param schemas Signed-off-by: Harry Hogg --- .../createConditionTransformer.test.ts | 4 +-- .../integration/createConditionTransformer.ts | 10 +++++++- .../createPermissionIntegrationRouter.test.ts | 25 +++++++++++-------- .../createPermissionIntegrationRouter.ts | 9 ++++++- 4 files changed, 33 insertions(+), 15 deletions(-) diff --git a/plugins/permission-node/src/integration/createConditionTransformer.test.ts b/plugins/permission-node/src/integration/createConditionTransformer.test.ts index 421bbcb2f4..e7e1e89d06 100644 --- a/plugins/permission-node/src/integration/createConditionTransformer.test.ts +++ b/plugins/permission-node/src/integration/createConditionTransformer.test.ts @@ -27,7 +27,7 @@ const transformConditions = createConditionTransformer([ name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', - schema: z.tuple([]), + schema: z.tuple([z.string(), z.number()]), apply: jest.fn(), toQuery: jest.fn( (firstParam: string, secondParam: number) => @@ -38,7 +38,7 @@ const transformConditions = createConditionTransformer([ name: 'test-rule-2', description: 'Test rule 2', resourceType: 'test-resource', - schema: z.tuple([]), + schema: z.tuple([z.object({})]), apply: jest.fn(), toQuery: jest.fn( (firstParam: object) => `test-rule-2:${JSON.stringify(firstParam)}`, diff --git a/plugins/permission-node/src/integration/createConditionTransformer.ts b/plugins/permission-node/src/integration/createConditionTransformer.ts index 112bb30d01..b40063ec13 100644 --- a/plugins/permission-node/src/integration/createConditionTransformer.ts +++ b/plugins/permission-node/src/integration/createConditionTransformer.ts @@ -13,6 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +import { InputError } from '@backstage/errors'; import { AllOfCriteria, AnyOfCriteria, @@ -45,7 +46,14 @@ const mapConditions = ( }; } - return getRule(criteria.rule).toQuery(...criteria.params); + const rule = getRule(criteria.rule); + const result = rule.schema.safeParse(criteria.params); + + if (rule.schema && !result.success) { + throw new InputError(`Parameters to rule are invalid`, result.error); + } + + return rule.toQuery(...criteria.params); }; /** diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 829b1b3614..5649480a9c 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -40,7 +40,10 @@ const testRule1 = createPermissionRule({ name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', - schema: z.tuple([z.string(), z.number()]), + schema: z.tuple([ + z.string().describe('firstParam'), + z.number().describe('secondParam'), + ]), apply: (_resource: any, _firstParam: string, _secondParam: number) => true, toQuery: (_firstParam: string, _secondParam: number) => ({}), }); @@ -49,7 +52,7 @@ const testRule2 = createPermissionRule({ name: 'test-rule-2', description: 'Test rule 2', resourceType: 'test-resource', - schema: z.tuple([z.object({})]), + schema: z.tuple([z.object({}).describe('firstParam')]), apply: (_resource: any, _firstParam: object) => false, toQuery: (_firstParam: object) => ({}), }); @@ -250,7 +253,7 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: ['a', 1], }, }, { @@ -260,7 +263,7 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [], + params: [{}], }, }, { @@ -271,7 +274,7 @@ describe('createPermissionIntegrationRouter', () => { not: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: ['a', 1], }, }, }, @@ -283,7 +286,7 @@ describe('createPermissionIntegrationRouter', () => { not: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [], + params: [{}], }, }, }, @@ -296,12 +299,12 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: ['a', 1], }, { rule: 'test-rule-2', resourceType: 'test-resource', - params: [], + params: [{}], }, ], }, @@ -430,7 +433,7 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: ['a', 1], }, }, { @@ -440,7 +443,7 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: ['a', 1], }, }, { @@ -450,7 +453,7 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: ['a', 1], }, }, ], diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index 28abc2f48d..6c4df1763f 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -124,7 +124,14 @@ const applyConditions = ( return !applyConditions(criteria.not, resource, getRule); } - return getRule(criteria.rule).apply(resource, ...criteria.params); + const rule = getRule(criteria.rule); + const result = rule.schema.safeParse(criteria.params); + + if (rule.schema && !result.success) { + throw new InputError(`Parameters to rule are invalid`, result.error); + } + + return rule.apply(resource, ...criteria.params); }; /** From eec3f766f220f9935bb1a182b10b886987d36769 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Thu, 29 Sep 2022 09:27:53 +0100 Subject: [PATCH 03/33] Output a JSON schema from the .well-known metadata endpoint Signed-off-by: Harry Hogg --- plugins/permission-node/package.json | 3 +- .../createPermissionIntegrationRouter.test.ts | 30 +++++++++++++++++++ .../createPermissionIntegrationRouter.ts | 2 ++ yarn.lock | 10 +++++++ 4 files changed, 44 insertions(+), 1 deletion(-) diff --git a/plugins/permission-node/package.json b/plugins/permission-node/package.json index 5461c2132e..3d6ef790f9 100644 --- a/plugins/permission-node/package.json +++ b/plugins/permission-node/package.json @@ -41,7 +41,8 @@ "@types/express": "^4.17.6", "express": "^4.17.1", "express-promise-router": "^4.1.0", - "zod": "^3.11.6" + "zod": "^3.11.6", + "zod-to-json-schema": "^3.18.1" }, "devDependencies": { "@backstage/backend-test-utils": "workspace:^", diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 5649480a9c..7d80459ba3 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -530,12 +530,42 @@ describe('createPermissionIntegrationRouter', () => { name: testRule1.name, description: testRule1.description, resourceType: testRule1.resourceType, + schema: { + $schema: 'http://json-schema.org/draft-07/schema#', + items: [ + { + description: 'firstParam', + type: 'string', + }, + { + description: 'secondParam', + type: 'number', + }, + ], + maxItems: 2, + minItems: 2, + type: 'array', + }, parameters: { count: 2 }, }, { name: testRule2.name, description: testRule2.description, resourceType: testRule2.resourceType, + schema: { + $schema: 'http://json-schema.org/draft-07/schema#', + items: [ + { + additionalProperties: false, + description: 'firstParam', + properties: {}, + type: 'object', + }, + ], + maxItems: 1, + minItems: 1, + type: 'array', + }, parameters: { count: 1 }, }, ], diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index 6c4df1763f..28268b7f98 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -17,6 +17,7 @@ import express, { Response } from 'express'; import Router from 'express-promise-router'; import { z } from 'zod'; +import zodToJsonSchema from 'zod-to-json-schema'; import { InputError } from '@backstage/errors'; import { errorHandler } from '@backstage/backend-common'; import { @@ -201,6 +202,7 @@ export const createPermissionIntegrationRouter = < name: rule.name, description: rule.description, resourceType: rule.resourceType, + schema: zodToJsonSchema(rule.schema), parameters: { count: rule.toQuery.length, }, diff --git a/yarn.lock b/yarn.lock index a6bb179e7c..bef0e188c6 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6359,6 +6359,7 @@ __metadata: msw: ^0.47.0 supertest: ^6.1.3 zod: ^3.11.6 + zod-to-json-schema: ^3.18.1 languageName: unknown linkType: soft @@ -40077,6 +40078,15 @@ __metadata: languageName: node linkType: hard +"zod-to-json-schema@npm:^3.18.1": + version: 3.18.1 + resolution: "zod-to-json-schema@npm:3.18.1" + peerDependencies: + zod: ^3.18.0 + checksum: e55d0de83b50fbd1caa7541d037858815964477b52a9e6495496e447107386cf16e2c08b007fcfbffd7fbe069ca2c19018425a53eaee36aff5dda942d3db71f4 + languageName: node + linkType: hard + "zod@npm:^3.11.6, zod@npm:^3.9.5": version: 3.18.0 resolution: "zod@npm:3.18.0" From 46b4a72ceea915006f20f0eca38a030d4845bfe6 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Thu, 29 Sep 2022 11:08:16 +0100 Subject: [PATCH 04/33] Added changeset Signed-off-by: Harry Hogg --- .changeset/kind-bees-suffer.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .changeset/kind-bees-suffer.md diff --git a/.changeset/kind-bees-suffer.md b/.changeset/kind-bees-suffer.md new file mode 100644 index 0000000000..95899bf064 --- /dev/null +++ b/.changeset/kind-bees-suffer.md @@ -0,0 +1,8 @@ +--- +'@backstage/plugin-catalog-backend': minor +'@backstage/plugin-permission-backend': minor +'@backstage/plugin-permission-node': minor +'@backstage/plugin-playlist-backend': minor +--- + +Permission rules now require a schema (ZodSchema) that details the parameters a rule expects. This is to validate the parameters given to a rule and to provide more details of a rule via the metadata endpoint From 1893c3d4da5c0e3681719f0761696dd5d5371d75 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Thu, 29 Sep 2022 11:28:18 +0100 Subject: [PATCH 05/33] Updated API reports Signed-off-by: Harry Hogg --- plugins/permission-node/api-report.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 702d363137..055325f5a7 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -24,6 +24,7 @@ import { PolicyDecision } from '@backstage/plugin-permission-common'; import { QueryPermissionRequest } from '@backstage/plugin-permission-common'; import { ResourcePermission } from '@backstage/plugin-permission-common'; import { TokenManager } from '@backstage/backend-common'; +import { z } from 'zod'; // @public export type ApplyConditionsRequest = { @@ -170,6 +171,7 @@ export type PermissionRule< name: string; description: string; resourceType: TResourceType; + schema: z.ZodSchema; apply(resource: TResource, ...params: TParams): boolean; toQuery(...params: TParams): PermissionCriteria; }; From 6d447843fa4657918f4b450ed34a129c8f479e4d Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Mon, 3 Oct 2022 10:19:47 +0100 Subject: [PATCH 06/33] Changing over permission rules params API to accept a single object Signed-off-by: Harry Hogg --- .../rules/createPropertyRule.test.ts | 52 +++-- .../permissions/rules/createPropertyRule.ts | 16 +- .../permissions/rules/hasAnnotation.test.ts | 41 ++-- .../src/permissions/rules/hasAnnotation.ts | 16 +- .../src/permissions/rules/hasLabel.test.ts | 16 +- .../src/permissions/rules/hasLabel.ts | 9 +- .../permissions/rules/isEntityKind.test.ts | 18 +- .../src/permissions/rules/isEntityKind.ts | 9 +- .../permissions/rules/isEntityOwner.test.ts | 30 ++- .../src/permissions/rules/isEntityOwner.ts | 10 +- .../src/permissions/rules/util.ts | 5 +- .../service/AuthorizedEntitiesCatalog.test.ts | 8 +- .../src/service/createRouter.test.ts | 8 +- .../PermissionIntegrationClient.test.ts | 41 ++-- .../src/PermissionClient.test.ts | 4 +- .../permission-common/src/PermissionClient.ts | 2 +- plugins/permission-common/src/types/api.ts | 2 +- .../createConditionExports.test.ts | 55 ++++-- .../src/integration/createConditionExports.ts | 2 +- .../createConditionFactory.test.ts | 18 +- .../src/integration/createConditionFactory.ts | 4 +- .../createConditionTransformer.test.ts | 99 +++++++--- .../integration/createConditionTransformer.ts | 2 +- .../createPermissionIntegrationRouter.test.ts | 180 ++++++++++++------ .../createPermissionIntegrationRouter.ts | 4 +- .../src/integration/createPermissionRule.ts | 4 +- .../src/integration/util.test.ts | 4 +- plugins/permission-node/src/types.ts | 12 +- .../DefaultPlaylistPermissionPolicy.test.ts | 32 ++-- .../DefaultPlaylistPermissionPolicy.ts | 10 +- .../playlist-backend/src/permissions/rules.ts | 17 +- 31 files changed, 495 insertions(+), 235 deletions(-) diff --git a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.test.ts b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.test.ts index cb11add9ed..5b34c98ca6 100644 --- a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.test.ts +++ b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.test.ts @@ -41,7 +41,9 @@ describe('createPropertyRule', () => { name: 'test-component', }, }, - 'org.name', + { + key: 'org.name', + }, ), ).toBe(false); }); @@ -57,7 +59,9 @@ describe('createPropertyRule', () => { tags: [], }, }, - 'tags', + { + key: 'tags', + }, ), ).toBe(false); }); @@ -75,7 +79,9 @@ describe('createPropertyRule', () => { }, }, }, - 'org.name', + { + key: 'org.name', + }, ), ).toBe(true); }); @@ -91,7 +97,9 @@ describe('createPropertyRule', () => { tags: ['java'], }, }, - 'tags', + { + key: 'tags', + }, ), ).toBe(true); }); @@ -108,8 +116,10 @@ describe('createPropertyRule', () => { name: 'test-component', }, }, - 'org.name', - 'test-org', + { + key: 'org.name', + value: 'test-org', + }, ), ).toBe(false); }); @@ -127,8 +137,10 @@ describe('createPropertyRule', () => { }, }, }, - 'org.name', - 'test-org', + { + key: 'org.name', + value: 'test-org', + }, ), ).toBe(false); }); @@ -144,8 +156,10 @@ describe('createPropertyRule', () => { tags: ['java'], }, }, - 'tags', - 'python', + { + key: 'tags', + value: 'python', + }, ), ).toBe(false); }); @@ -163,8 +177,10 @@ describe('createPropertyRule', () => { }, }, }, - 'org.name', - 'test-org', + { + key: 'org.name', + value: 'test-org', + }, ), ).toBe(true); }); @@ -180,8 +196,10 @@ describe('createPropertyRule', () => { tags: ['java', 'java11'], }, }, - 'tags', - 'java', + { + key: 'tags', + value: 'java', + }, ), ).toBe(true); }); @@ -190,7 +208,11 @@ describe('createPropertyRule', () => { describe('toQuery', () => { it('returns an appropriate catalog-backend filter', () => { - expect(toQuery('backstage.io/test-component')).toEqual({ + expect( + toQuery({ + key: 'backstage.io/test-component', + }), + ).toEqual({ key: 'metadata.backstage.io/test-component', }); }); diff --git a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts index 12725c1fcd..da487c112c 100644 --- a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts +++ b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts @@ -15,7 +15,6 @@ */ import { get } from 'lodash'; -import { Entity } from '@backstage/catalog-model'; import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; import { createCatalogPermissionRule } from './util'; import { z } from 'zod'; @@ -25,11 +24,14 @@ export const createPropertyRule = (propertyType: 'metadata' | 'spec') => name: `HAS_${propertyType.toUpperCase()}`, description: `Allow entities which have the specified ${propertyType} subfield.`, resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - schema: z.tuple([ - z.string().describe('Property name'), - z.string().optional().describe('Property value'), - ]), - apply: (resource: Entity, key: string, value?: string) => { + schema: z.object({ + key: z.string().describe(`The key of the ${propertyType} to match on`), + value: z + .string() + .optional() + .describe(`Optional value of the ${propertyType} to match on`), + }), + apply: (resource, { key, value }) => { const foundValue = get(resource[propertyType], key); if (Array.isArray(foundValue)) { @@ -43,7 +45,7 @@ export const createPropertyRule = (propertyType: 'metadata' | 'spec') => } return !!foundValue; }, - toQuery: (key: string, value?: string) => ({ + toQuery: ({ key, value }) => ({ key: `${propertyType}.${key}`, ...(value !== undefined && { values: [value] }), }), diff --git a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.test.ts b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.test.ts index 609114be3d..73fce20a71 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.test.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.test.ts @@ -31,7 +31,9 @@ describe('hasAnnotation permission rule', () => { }, }, }, - 'backstage.io/test-annotation', + { + annotation: 'backstage.io/test-annotation', + }, ), ).toEqual(false); }); @@ -46,7 +48,9 @@ describe('hasAnnotation permission rule', () => { name: 'test-component', }, }, - 'backstage.io/test-annotation', + { + annotation: 'backstage.io/test-annotation', + }, ), ).toEqual(false); expect( @@ -58,8 +62,10 @@ describe('hasAnnotation permission rule', () => { name: 'test-component', }, }, - 'backstage.io/test-annotation', - 'some value', + { + annotation: 'backstage.io/test-annotation', + value: 'some value', + }, ), ).toEqual(false); }); @@ -78,7 +84,9 @@ describe('hasAnnotation permission rule', () => { }, }, }, - 'backstage.io/test-annotation', + { + annotation: 'backstage.io/test-annotation', + }, ), ).toEqual(true); }); @@ -97,8 +105,10 @@ describe('hasAnnotation permission rule', () => { }, }, }, - 'backstage.io/test-annotation', - 'baz', + { + annotation: 'backstage.io/test-annotation', + value: 'baz', + }, ), ).toEqual(false); }); @@ -117,8 +127,10 @@ describe('hasAnnotation permission rule', () => { }, }, }, - 'backstage.io/test-annotation', - 'bar', + { + annotation: 'backstage.io/test-annotation', + value: 'bar', + }, ), ).toEqual(true); }); @@ -126,7 +138,11 @@ describe('hasAnnotation permission rule', () => { describe('toQuery', () => { it('returns an appropriate catalog-backend filter', () => { - expect(hasAnnotation.toQuery('backstage.io/test-annotation')).toEqual({ + expect( + hasAnnotation.toQuery({ + annotation: 'backstage.io/test-annotation', + }), + ).toEqual({ key: 'metadata.annotations.backstage.io/test-annotation', }); }); @@ -134,7 +150,10 @@ describe('hasAnnotation permission rule', () => { it('returns an appropriate catalog-backend filter with values', () => { expect( - hasAnnotation.toQuery('backstage.io/test-annotation', 'foo'), + hasAnnotation.toQuery({ + annotation: 'backstage.io/test-annotation', + value: 'foo', + }), ).toEqual({ key: 'metadata.annotations.backstage.io/test-annotation', values: ['foo'], diff --git a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts index 4042c34636..9651e9d925 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts @@ -14,7 +14,6 @@ * limitations under the License. */ -import { Entity } from '@backstage/catalog-model'; import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; import { z } from 'zod'; import { createCatalogPermissionRule } from './util'; @@ -32,16 +31,19 @@ export const hasAnnotation = createCatalogPermissionRule({ description: 'Allow entities which are annotated with the specified annotation', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - schema: z.tuple([ - z.string().describe('Annotation name'), - z.string().optional().describe('Annotation value'), - ]), - apply: (resource: Entity, annotation: string, value?: string) => + schema: z.object({ + annotation: z.string().describe('The name of the annotation to match on'), + value: z + .string() + .optional() + .describe('Optional value of the annotation to match on'), + }), + apply: (resource, { annotation, value }) => !!resource.metadata.annotations?.hasOwnProperty(annotation) && (value === undefined ? true : resource.metadata.annotations?.[annotation] === value), - toQuery: (annotation: string, value?: string) => + toQuery: ({ annotation, value }) => value === undefined ? { key: `metadata.annotations.${annotation}`, diff --git a/plugins/catalog-backend/src/permissions/rules/hasLabel.test.ts b/plugins/catalog-backend/src/permissions/rules/hasLabel.test.ts index a1b9cb5ad0..8aa43402c5 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasLabel.test.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasLabel.test.ts @@ -31,7 +31,9 @@ describe('hasLabel permission rule', () => { }, }, }, - 'backstage.io/testlabel', + { + label: 'backstage.io/testlabel', + }, ), ).toEqual(false); }); @@ -46,7 +48,9 @@ describe('hasLabel permission rule', () => { name: 'test-component', }, }, - 'backstage.io/testlabel', + { + label: 'backstage.io/testlabel', + }, ), ).toEqual(false); }); @@ -65,7 +69,7 @@ describe('hasLabel permission rule', () => { }, }, }, - 'backstage.io/testlabel', + { label: 'backstage.io/testlabel' }, ), ).toEqual(true); }); @@ -73,7 +77,11 @@ describe('hasLabel permission rule', () => { describe('toQuery', () => { it('returns an appropriate catalog-backend filter', () => { - expect(hasLabel.toQuery('backstage.io/testlabel')).toEqual({ + expect( + hasLabel.toQuery({ + label: 'backstage.io/testlabel', + }), + ).toEqual({ key: 'metadata.labels.backstage.io/testlabel', }); }); diff --git a/plugins/catalog-backend/src/permissions/rules/hasLabel.ts b/plugins/catalog-backend/src/permissions/rules/hasLabel.ts index 3d1a18f8b3..fbad7e42d0 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasLabel.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasLabel.ts @@ -14,7 +14,6 @@ * limitations under the License. */ -import { Entity } from '@backstage/catalog-model'; import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; import { z } from 'zod'; import { createCatalogPermissionRule } from './util'; @@ -28,10 +27,12 @@ export const hasLabel = createCatalogPermissionRule({ name: 'HAS_LABEL', description: 'Allow entities which have the specified label metadata.', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - schema: z.tuple([z.string().describe('Label name')]), - apply: (resource: Entity, label: string) => + schema: z.object({ + label: z.string().describe('Name of the label'), + }), + apply: (resource, { label }) => !!resource.metadata.labels?.hasOwnProperty(label), - toQuery: (label: string) => ({ + toQuery: ({ label }) => ({ key: `metadata.labels.${label}`, }), }); diff --git a/plugins/catalog-backend/src/permissions/rules/isEntityKind.test.ts b/plugins/catalog-backend/src/permissions/rules/isEntityKind.test.ts index e11e102728..28983f6bec 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityKind.test.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityKind.test.ts @@ -27,7 +27,11 @@ describe('isEntityKind', () => { name: 'some-component', }, }; - expect(isEntityKind.apply(component, ['b'])).toBe(true); + expect( + isEntityKind.apply(component, { + kinds: ['b'], + }), + ).toBe(true); }); it('returns false when entity is not the correct kind', () => { @@ -38,13 +42,21 @@ describe('isEntityKind', () => { name: 'some-component', }, }; - expect(isEntityKind.apply(component, ['c'])).toBe(false); + expect( + isEntityKind.apply(component, { + kinds: ['c'], + }), + ).toBe(false); }); }); describe('toQuery', () => { it('returns an appropriate catalog-backend filter', () => { - expect(isEntityKind.toQuery(['b'])).toEqual({ + expect( + isEntityKind.toQuery({ + kinds: ['b'], + }), + ).toEqual({ key: 'kind', values: ['b'], }); diff --git a/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts b/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts index 70467558dd..8c16f4c5a1 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts @@ -13,7 +13,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { Entity } from '@backstage/catalog-model'; import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; import { z } from 'zod'; import { EntitiesSearchFilter } from '../../catalog/types'; @@ -28,12 +27,14 @@ export const isEntityKind = createCatalogPermissionRule({ name: 'IS_ENTITY_KIND', description: 'Allow entities with the specified kind', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - schema: z.tuple([z.array(z.string().describe('List of entity kinds'))]), - apply(resource: Entity, kinds: string[]) { + schema: z.object({ + kinds: z.array(z.string()), + }), + apply(resource, { kinds }) { const resourceKind = resource.kind.toLocaleLowerCase('en-US'); return kinds.some(kind => kind.toLocaleLowerCase('en-US') === resourceKind); }, - toQuery(kinds: string[]): EntitiesSearchFilter { + toQuery({ kinds }): EntitiesSearchFilter { return { key: 'kind', values: kinds.map(kind => kind.toLocaleLowerCase('en-US')), diff --git a/plugins/catalog-backend/src/permissions/rules/isEntityOwner.test.ts b/plugins/catalog-backend/src/permissions/rules/isEntityOwner.test.ts index 11304c768c..f09e484c32 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityOwner.test.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityOwner.test.ts @@ -33,9 +33,11 @@ describe('isEntityOwner', () => { }, ], }; - expect(isEntityOwner.apply(component, ['user:default/spiderman'])).toBe( - true, - ); + expect( + isEntityOwner.apply(component, { + claims: ['user:default/spiderman'], + }), + ).toBe(true); }); it('returns false when entity is not owned by the given user', () => { @@ -52,9 +54,11 @@ describe('isEntityOwner', () => { }, ], }; - expect(isEntityOwner.apply(component, ['user:default/spiderman'])).toBe( - false, - ); + expect( + isEntityOwner.apply(component, { + claims: ['user:default/spiderman'], + }), + ).toBe(false); }); it('returns false when entity does not have an owner', () => { @@ -65,15 +69,21 @@ describe('isEntityOwner', () => { name: 'some-component', }, }; - expect(isEntityOwner.apply(component, ['user:default/spiderman'])).toBe( - false, - ); + expect( + isEntityOwner.apply(component, { + claims: ['user:default/spiderman'], + }), + ).toBe(false); }); }); describe('toQuery', () => { it('returns an appropriate catalog-backend filter', () => { - expect(isEntityOwner.toQuery(['user:default/spiderman'])).toEqual({ + expect( + isEntityOwner.toQuery({ + claims: ['user:default/spiderman'], + }), + ).toEqual({ key: 'relations.ownedBy', values: ['user:default/spiderman'], }); diff --git a/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts b/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts index c0cc7c9911..27431f09a7 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { Entity, RELATION_OWNED_BY } from '@backstage/catalog-model'; +import { RELATION_OWNED_BY } from '@backstage/catalog-model'; import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; import { z } from 'zod'; import { createCatalogPermissionRule } from './util'; @@ -29,8 +29,10 @@ export const isEntityOwner = createCatalogPermissionRule({ name: 'IS_ENTITY_OWNER', description: 'Allow entities owned by the current user', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - schema: z.tuple([z.array(z.string().describe('List of owners'))]), - apply: (resource: Entity, claims: string[]) => { + schema: z.object({ + claims: z.array(z.string()), + }), + apply: (resource, { claims }) => { if (!resource.relations) { return false; } @@ -39,7 +41,7 @@ export const isEntityOwner = createCatalogPermissionRule({ .filter(relation => relation.type === RELATION_OWNED_BY) .some(relation => claims.includes(relation.targetRef)); }, - toQuery: (claims: string[]) => ({ + toQuery: ({ claims }) => ({ key: 'relations.ownedBy', values: claims, }), diff --git a/plugins/catalog-backend/src/permissions/rules/util.ts b/plugins/catalog-backend/src/permissions/rules/util.ts index 36b351595d..37544a14f8 100644 --- a/plugins/catalog-backend/src/permissions/rules/util.ts +++ b/plugins/catalog-backend/src/permissions/rules/util.ts @@ -29,8 +29,9 @@ import { EntitiesSearchFilter } from '../../catalog/types'; * * @alpha */ -export type CatalogPermissionRule = - PermissionRule; +export type CatalogPermissionRule< + TParams extends Record = Record, +> = PermissionRule; /** * Helper function for creating correctly-typed diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts index b15d1df4bb..66764dc89f 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts @@ -65,7 +65,7 @@ describe('AuthorizedEntitiesCatalog', () => { fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, - conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, + conditions: { rule: 'IS_ENTITY_KIND', params: { kinds: ['b'] } }, }, ]); const catalog = createCatalog(isEntityKind); @@ -117,7 +117,7 @@ describe('AuthorizedEntitiesCatalog', () => { fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, - conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, + conditions: { rule: 'IS_ENTITY_KIND', params: { kinds: ['b'] } }, }, ]); fakeCatalog.entities.mockResolvedValue({ entities: [] }); @@ -136,7 +136,7 @@ describe('AuthorizedEntitiesCatalog', () => { fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, - conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, + conditions: { rule: 'IS_ENTITY_KIND', params: { kinds: ['b'] } }, }, ]); fakeCatalog.entities.mockResolvedValue({ @@ -272,7 +272,7 @@ describe('AuthorizedEntitiesCatalog', () => { fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, - conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, + conditions: { rule: 'IS_ENTITY_KIND', params: { kinds: ['b'] } }, }, ]); const catalog = createCatalog(isEntityKind); diff --git a/plugins/catalog-backend/src/service/createRouter.test.ts b/plugins/catalog-backend/src/service/createRouter.test.ts index 163e5e0747..088e6c71e2 100644 --- a/plugins/catalog-backend/src/service/createRouter.test.ts +++ b/plugins/catalog-backend/src/service/createRouter.test.ts @@ -696,7 +696,9 @@ describe('NextRouter permissioning', () => { name: 'FAKE_RULE', description: 'fake rule', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - schema: z.tuple([]), + schema: z.object({ + foo: z.string(), + }), apply: () => true, toQuery: () => ({ key: '', values: [] }), }); @@ -760,7 +762,9 @@ describe('NextRouter permissioning', () => { conditions: { rule: 'FAKE_RULE', resourceType: 'catalog-entity', - params: ['user:default/spiderman'], + params: { + foo: 'user:default/spiderman', + }, }, }, ], diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts index 6c05a825ab..8cbeeaebd9 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts @@ -39,8 +39,12 @@ describe('PermissionIntegrationClient', () => { const mockConditions: PermissionCriteria = { not: { allOf: [ - { rule: 'RULE_1', resourceType: 'test-resource', params: [] }, - { rule: 'RULE_2', resourceType: 'test-resource', params: ['abc'] }, + { rule: 'RULE_1', resourceType: 'test-resource', params: {} }, + { + rule: 'RULE_2', + resourceType: 'test-resource', + params: { foo: 'abc' }, + }, ], }, }; @@ -280,8 +284,10 @@ describe('PermissionIntegrationClient', () => { name: 'RULE_1', description: 'Test rule 1', resourceType: 'test-resource', - schema: z.tuple([z.enum(['yes', 'no'])]), - apply: (_resource: any, input: 'yes' | 'no') => input === 'yes', + schema: z.object({ + input: z.enum(['yes', 'no']), + }), + apply: (_resource, { input }) => input === 'yes', toQuery: () => { throw new Error('Not implemented'); }, @@ -290,8 +296,11 @@ describe('PermissionIntegrationClient', () => { name: 'RULE_2', description: 'Test rule 2', resourceType: 'test-resource', - schema: z.tuple([z.enum(['yes', 'no'])]), - apply: (_resource: any, input: 'yes' | 'no') => input === 'yes', + + schema: z.object({ + input: z.enum(['yes', 'no']), + }), + apply: (_resource, { input }) => input === 'yes', toQuery: () => { throw new Error('Not implemented'); }, @@ -347,7 +356,9 @@ describe('PermissionIntegrationClient', () => { conditions: { rule: 'RULE_1', resourceType: 'test-resource', - params: ['no'], + params: { + input: 'no', + }, }, }, ]), @@ -368,13 +379,17 @@ describe('PermissionIntegrationClient', () => { { rule: 'RULE_1', resourceType: 'test-resource', - params: ['yes'], + params: { + input: 'yes', + }, }, { not: { rule: 'RULE_2', resourceType: 'test-resource', - params: ['no'], + params: { + input: 'no', + }, }, }, ], @@ -385,12 +400,16 @@ describe('PermissionIntegrationClient', () => { { rule: 'RULE_1', resourceType: 'test-resource', - params: ['no'], + params: { + input: 'no', + }, }, { rule: 'RULE_2', resourceType: 'test-resource', - params: ['yes'], + params: { + input: 'yes', + }, }, ], }, diff --git a/plugins/permission-common/src/PermissionClient.test.ts b/plugins/permission-common/src/PermissionClient.test.ts index 9b2bdcb9ff..bcc6ddd6f6 100644 --- a/plugins/permission-common/src/PermissionClient.test.ts +++ b/plugins/permission-common/src/PermissionClient.test.ts @@ -227,7 +227,7 @@ describe('PermissionClient', () => { conditions: { resourceType: 'test-resource', rule: 'FOO', - params: ['bar'], + params: { foo: 'bar' }, }, }), ); @@ -275,7 +275,7 @@ describe('PermissionClient', () => { conditions: { rule: 'FOO', resourceType: 'test-resource', - params: ['bar'], + params: { foo: 'bar' }, }, }), ); diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 9a7bb0bba4..7b429dc46f 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -41,7 +41,7 @@ const permissionCriteriaSchema: z.ZodSchema< .object({ rule: z.string(), resourceType: z.string(), - params: z.array(z.unknown()), + params: z.record(z.unknown()), }) .or(z.object({ anyOf: z.array(permissionCriteriaSchema).nonempty() })) .or(z.object({ allOf: z.array(permissionCriteriaSchema).nonempty() })) diff --git a/plugins/permission-common/src/types/api.ts b/plugins/permission-common/src/types/api.ts index f29a639d18..70b9e902b8 100644 --- a/plugins/permission-common/src/types/api.ts +++ b/plugins/permission-common/src/types/api.ts @@ -101,7 +101,7 @@ export type PolicyDecision = */ export type PermissionCondition< TResourceType extends string = string, - TParams extends unknown[] = unknown[], + TParams extends Record = Record, > = { resourceType: TResourceType; rule: string; diff --git a/plugins/permission-node/src/integration/createConditionExports.test.ts b/plugins/permission-node/src/integration/createConditionExports.test.ts index ed0ab49b5c..a989206fdd 100644 --- a/plugins/permission-node/src/integration/createConditionExports.test.ts +++ b/plugins/permission-node/src/integration/createConditionExports.test.ts @@ -31,25 +31,28 @@ const testIntegration = () => name: 'testRule1', description: 'Test rule 1', resourceType: 'test-resource', - schema: z.tuple([z.string(), z.number()]), - apply: jest.fn( - (_resource: any, _firstParam: string, _secondParam: number) => true, - ), - toQuery: jest.fn((firstParam: string, secondParam: number) => ({ + schema: z.object({ + foo: z.string(), + bar: z.number(), + }), + apply: (_resource: any, _params) => true, + toQuery: params => ({ query: 'testRule1', - params: [firstParam, secondParam], - })), + params, + }), }), testRule2: createPermissionRule({ name: 'testRule2', description: 'Test rule 2', resourceType: 'test-resource', - schema: z.tuple([z.object({})]), - apply: jest.fn((_resource: any, _firstParam: object) => false), - toQuery: jest.fn((firstParam: object) => ({ + schema: z.object({ + foo: z.object({}), + }), + apply: (_resource: any) => false, + toQuery: params => ({ query: 'testRule2', - params: [firstParam], - })), + params, + }), }), }, }); @@ -59,16 +62,26 @@ describe('createConditionExports', () => { it('creates condition factories for the supplied rules', () => { const { conditions } = testIntegration(); - expect(conditions.testRule1('a', 1)).toEqual({ + expect( + conditions.testRule1({ + foo: 'a', + bar: 1, + }), + ).toEqual({ rule: 'testRule1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }); - expect(conditions.testRule2({ baz: 'quux' })).toEqual({ + expect(conditions.testRule2({ foo: { baz: 'quux' } })).toEqual({ rule: 'testRule2', resourceType: 'test-resource', - params: [{ baz: 'quux' }], + params: { + foo: { baz: 'quux' }, + }, }); }); }); @@ -88,7 +101,10 @@ describe('createConditionExports', () => { { rule: 'testRule1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }, ], }), @@ -101,7 +117,10 @@ describe('createConditionExports', () => { { rule: 'testRule1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }, ], }, diff --git a/plugins/permission-node/src/integration/createConditionExports.ts b/plugins/permission-node/src/integration/createConditionExports.ts index ba34c07fed..3921c2dfe3 100644 --- a/plugins/permission-node/src/integration/createConditionExports.ts +++ b/plugins/permission-node/src/integration/createConditionExports.ts @@ -36,7 +36,7 @@ export type Condition = TRule extends PermissionRule< infer TResourceType, infer TParams > - ? (...params: TParams) => PermissionCondition + ? (params: TParams) => PermissionCondition : never; /** diff --git a/plugins/permission-node/src/integration/createConditionFactory.test.ts b/plugins/permission-node/src/integration/createConditionFactory.test.ts index 433d37b3f6..191688f0dd 100644 --- a/plugins/permission-node/src/integration/createConditionFactory.test.ts +++ b/plugins/permission-node/src/integration/createConditionFactory.test.ts @@ -23,9 +23,11 @@ describe('createConditionFactory', () => { name: 'test-rule', description: 'test-description', resourceType: 'test-resource', - schema: z.tuple([]), - apply: jest.fn(), - toQuery: jest.fn(), + schema: z.object({ + foo: z.string(), + }), + apply: (_resource, _params) => true, + toQuery: _params => ({}), }); it('returns a function', () => { @@ -35,10 +37,16 @@ describe('createConditionFactory', () => { describe('return value', () => { it('constructs a condition with the rule name and supplied params', () => { const conditionFactory = createConditionFactory(testRule); - expect(conditionFactory('a', 'b', 1, 2)).toEqual({ + expect( + conditionFactory({ + foo: 'bar', + }), + ).toEqual({ rule: 'test-rule', resourceType: 'test-resource', - params: ['a', 'b', 1, 2], + params: { + foo: 'bar', + }, }); }); }); diff --git a/plugins/permission-node/src/integration/createConditionFactory.ts b/plugins/permission-node/src/integration/createConditionFactory.ts index 15a5b4fd08..5b499ab512 100644 --- a/plugins/permission-node/src/integration/createConditionFactory.ts +++ b/plugins/permission-node/src/integration/createConditionFactory.ts @@ -34,10 +34,10 @@ import { PermissionRule } from '../types'; * @public */ export const createConditionFactory = - ( + >( rule: PermissionRule, ) => - (...params: TParams): PermissionCondition => ({ + (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 e7e1e89d06..5021b7766d 100644 --- a/plugins/permission-node/src/integration/createConditionTransformer.test.ts +++ b/plugins/permission-node/src/integration/createConditionTransformer.test.ts @@ -27,22 +27,22 @@ const transformConditions = createConditionTransformer([ name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', - schema: z.tuple([z.string(), z.number()]), + schema: z.object({ + foo: z.string(), + bar: z.number(), + }), apply: jest.fn(), - toQuery: jest.fn( - (firstParam: string, secondParam: number) => - `test-rule-1:${firstParam}/${secondParam}`, - ), + toQuery: jest.fn(({ foo, bar }) => `test-rule-1:${foo}/${bar}`), }), createPermissionRule({ name: 'test-rule-2', description: 'Test rule 2', resourceType: 'test-resource', - schema: z.tuple([z.object({})]), + schema: z.object({ + foo: z.object({}), + }), apply: jest.fn(), - toQuery: jest.fn( - (firstParam: object) => `test-rule-2:${JSON.stringify(firstParam)}`, - ), + toQuery: jest.fn(({ foo }) => `test-rule-2:${JSON.stringify(foo)}`), }), ]); @@ -55,7 +55,10 @@ describe('createConditionTransformer', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['abc', 123], + params: { + foo: 'abc', + bar: 123, + }, }, expectedResult: 'test-rule-1:abc/123', }, @@ -63,7 +66,9 @@ describe('createConditionTransformer', () => { conditions: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{ foo: 0 }], + params: { + foo: { foo: 0 }, + }, }, expectedResult: 'test-rule-2:{"foo":0}', }, @@ -73,9 +78,18 @@ describe('createConditionTransformer', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, + }, + { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: { + foo: {}, + }, }, - { rule: 'test-rule-2', resourceType: 'test-resource', params: [{}] }, ], }, expectedResult: { @@ -88,9 +102,18 @@ describe('createConditionTransformer', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, + }, + { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: { + foo: {}, + }, }, - { rule: 'test-rule-2', resourceType: 'test-resource', params: [{}] }, ], }, expectedResult: { @@ -102,7 +125,9 @@ describe('createConditionTransformer', () => { not: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{}], + params: { + foo: {}, + }, }, }, expectedResult: { @@ -117,12 +142,17 @@ describe('createConditionTransformer', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }, { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{}], + params: { + foo: {}, + }, }, ], }, @@ -132,12 +162,19 @@ describe('createConditionTransformer', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['b', 2], + params: { + foo: 'b', + bar: 2, + }, }, { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{ c: 3 }], + params: { + foo: { + c: 3, + }, + }, }, ], }, @@ -165,12 +202,19 @@ describe('createConditionTransformer', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }, { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{ b: 2 }], + params: { + foo: { + b: 2, + }, + }, }, ], }, @@ -180,13 +224,20 @@ describe('createConditionTransformer', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['c', 3], + params: { + foo: 'c', + bar: 3, + }, }, { not: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{ d: 4 }], + params: { + foo: { + d: 4, + }, + }, }, }, ], diff --git a/plugins/permission-node/src/integration/createConditionTransformer.ts b/plugins/permission-node/src/integration/createConditionTransformer.ts index b40063ec13..afbd4a9316 100644 --- a/plugins/permission-node/src/integration/createConditionTransformer.ts +++ b/plugins/permission-node/src/integration/createConditionTransformer.ts @@ -53,7 +53,7 @@ const mapConditions = ( throw new InputError(`Parameters to rule are invalid`, result.error); } - return rule.toQuery(...criteria.params); + return rule.toQuery(criteria.params); }; /** diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 7d80459ba3..9c6b345faf 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -40,21 +40,23 @@ const testRule1 = createPermissionRule({ name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', - schema: z.tuple([ - z.string().describe('firstParam'), - z.number().describe('secondParam'), - ]), - apply: (_resource: any, _firstParam: string, _secondParam: number) => true, - toQuery: (_firstParam: string, _secondParam: number) => ({}), + schema: z.object({ + foo: z.string(), + bar: z.number().describe('bar'), + }), + apply: (_resource: any, _params) => true, + toQuery: _params => ({}), }); const testRule2 = createPermissionRule({ name: 'test-rule-2', description: 'Test rule 2', resourceType: 'test-resource', - schema: z.tuple([z.object({}).describe('firstParam')]), - apply: (_resource: any, _firstParam: object) => false, - toQuery: (_firstParam: object) => ({}), + schema: z.object({ + foo: z.object({}).describe('foo'), + }), + apply: (_resource: any, _foo) => false, + toQuery: _foo => ({}), }); describe('createPermissionIntegrationRouter', () => { @@ -85,23 +87,35 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['abc', 123], + params: { + foo: 'abc', + bar: 123, + }, }, { anyOf: [ { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, + }, + { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: { foo: {} }, }, - { rule: 'test-rule-2', resourceType: 'test-resource', params: [{}] }, ], }, { not: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{}], + params: { + foo: {}, + }, }, }, { @@ -111,12 +125,17 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }, { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{}], + params: { + foo: {}, + }, }, ], }, @@ -126,12 +145,17 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['b', 2], + params: { + foo: 'b', + bar: 2, + }, }, { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{ c: 3 }], + params: { + foo: { c: 3 }, + }, }, ], }, @@ -167,16 +191,25 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{ foo: 0 }], + params: { + foo: { foo: 0 }, + }, }, { allOf: [ { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, + }, + { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: { foo: {} }, }, - { rule: 'test-rule-2', resourceType: 'test-resource', params: [{}] }, ], }, { @@ -186,12 +219,17 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }, { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{ b: 2 }], + params: { + foo: { b: 2 }, + }, }, ], }, @@ -201,13 +239,18 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['c', 3], + params: { + foo: 'c', + bar: 3, + }, }, { not: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{ d: 4 }], + params: { + foo: { d: 4 }, + }, }, }, ], @@ -253,7 +296,10 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }, }, { @@ -263,7 +309,9 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{}], + params: { + foo: {}, + }, }, }, { @@ -274,7 +322,10 @@ describe('createPermissionIntegrationRouter', () => { not: { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }, }, }, @@ -286,7 +337,9 @@ describe('createPermissionIntegrationRouter', () => { not: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{}], + params: { + foo: {}, + }, }, }, }, @@ -299,12 +352,17 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }, { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{}], + params: { + foo: {}, + }, }, ], }, @@ -348,7 +406,9 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-incorrect-resource-1', - params: [{}], + params: { + foo: {}, + }, }, }, { @@ -358,7 +418,9 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [{}], + params: { + foo: {}, + }, }, }, { @@ -368,7 +430,9 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-incorrect-resource-2', - params: [{}], + params: { + foo: {}, + }, }, }, ], @@ -396,7 +460,7 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: {}, }, }, ], @@ -433,7 +497,10 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }, }, { @@ -443,7 +510,10 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }, }, { @@ -453,7 +523,10 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }, }, ], @@ -532,21 +605,20 @@ describe('createPermissionIntegrationRouter', () => { resourceType: testRule1.resourceType, schema: { $schema: 'http://json-schema.org/draft-07/schema#', - items: [ - { - description: 'firstParam', + additionalProperties: false, + properties: { + foo: { type: 'string', }, - { - description: 'secondParam', + bar: { + description: 'bar', type: 'number', }, - ], - maxItems: 2, - minItems: 2, - type: 'array', + }, + required: ['foo', 'bar'], + type: 'object', }, - parameters: { count: 2 }, + parameters: { count: 1 }, }, { name: testRule2.name, @@ -554,17 +626,17 @@ describe('createPermissionIntegrationRouter', () => { resourceType: testRule2.resourceType, schema: { $schema: 'http://json-schema.org/draft-07/schema#', - items: [ - { + additionalProperties: false, + properties: { + foo: { additionalProperties: false, - description: 'firstParam', + description: 'foo', properties: {}, type: 'object', }, - ], - maxItems: 1, - minItems: 1, - type: 'array', + }, + required: ['foo'], + type: 'object', }, parameters: { count: 1 }, }, diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index 28268b7f98..c5843f6a84 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -46,7 +46,7 @@ const permissionCriteriaSchema: z.ZodSchema< z.object({ rule: z.string(), resourceType: z.string(), - params: z.array(z.unknown()), + params: z.record(z.unknown()), }), ]), ); @@ -132,7 +132,7 @@ const applyConditions = ( throw new InputError(`Parameters to rule are invalid`, result.error); } - return rule.apply(resource, ...criteria.params); + return rule.apply(resource, criteria.params); }; /** diff --git a/plugins/permission-node/src/integration/createPermissionRule.ts b/plugins/permission-node/src/integration/createPermissionRule.ts index 6673fe76de..ef84a5a26f 100644 --- a/plugins/permission-node/src/integration/createPermissionRule.ts +++ b/plugins/permission-node/src/integration/createPermissionRule.ts @@ -25,7 +25,7 @@ export const createPermissionRule = < TResource, TQuery, TResourceType extends string, - TParams extends unknown[], + TParams extends Record, >( rule: PermissionRule, ) => rule; @@ -40,7 +40,7 @@ export const createPermissionRule = < */ export const makeCreatePermissionRule = () => - ( + >( 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 1572964a7d..924ab30757 100644 --- a/plugins/permission-node/src/integration/util.test.ts +++ b/plugins/permission-node/src/integration/util.test.ts @@ -31,7 +31,7 @@ describe('permission integration utils', () => { name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', - schema: z.tuple([]), + schema: z.object({}), apply: jest.fn(), toQuery: jest.fn(), }); @@ -40,7 +40,7 @@ describe('permission integration utils', () => { name: 'test-rule-2', description: 'Test rule 2', resourceType: 'test-resource', - schema: z.tuple([]), + schema: z.object({}), apply: jest.fn(), toQuery: jest.fn(), }); diff --git a/plugins/permission-node/src/types.ts b/plugins/permission-node/src/types.ts index 7beb5bb92d..02ebe07ac4 100644 --- a/plugins/permission-node/src/types.ts +++ b/plugins/permission-node/src/types.ts @@ -37,7 +37,7 @@ export type PermissionRule< TResource, TQuery, TResourceType extends string, - TParams extends unknown[] = unknown[], + TParams extends Record = Record, > = { name: string; description: string; @@ -46,19 +46,23 @@ export type PermissionRule< /** * A ZodSchema that documents the parameters that this rule accepts. */ - schema: z.ZodSchema; + schema: z.ZodObject<{ + [P in keyof TParams]-?: TParams[P] extends undefined + ? z.ZodOptionalType> + : z.ZodType; + }>; /** * Apply this rule to a resource already loaded from a backing data source. The params are * arguments supplied for the rule; for example, a rule could be `isOwner` with entityRefs as the * params. */ - apply(resource: TResource, ...params: TParams): boolean; + apply(resource: TResource, params: TParams): boolean; /** * Translate this rule to criteria suitable for use in querying a backing data store. The criteria * can be used for loading a collection of resources efficiently with conditional criteria already * applied. */ - toQuery(...params: TParams): PermissionCriteria; + toQuery(params: TParams): PermissionCriteria; }; diff --git a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts index 179b0d7f9b..3d761bfd35 100644 --- a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts +++ b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts @@ -67,11 +67,10 @@ describe('DefaultPlaylistPermissionPolicy', () => { resourceType: PLAYLIST_LIST_RESOURCE_TYPE, conditions: { anyOf: [ - playlistConditions.isOwner([ - 'user:default/me', - 'group:default/owner', - ]), - playlistConditions.isPublic(), + playlistConditions.isOwner({ + owners: ['user:default/me', 'group:default/owner'], + }), + playlistConditions.isPublic({}), ], }, }); @@ -89,11 +88,10 @@ describe('DefaultPlaylistPermissionPolicy', () => { resourceType: PLAYLIST_LIST_RESOURCE_TYPE, conditions: { anyOf: [ - playlistConditions.isOwner([ - 'user:default/me', - 'group:default/owner', - ]), - playlistConditions.isPublic(), + playlistConditions.isOwner({ + owners: ['user:default/me', 'group:default/owner'], + }), + playlistConditions.isPublic({}), ], }, }); @@ -109,10 +107,9 @@ describe('DefaultPlaylistPermissionPolicy', () => { result: AuthorizeResult.CONDITIONAL, pluginId: 'playlist', resourceType: PLAYLIST_LIST_RESOURCE_TYPE, - conditions: playlistConditions.isOwner([ - 'user:default/me', - 'group:default/owner', - ]), + conditions: playlistConditions.isOwner({ + owners: ['user:default/me', 'group:default/owner'], + }), }); }); @@ -126,10 +123,9 @@ describe('DefaultPlaylistPermissionPolicy', () => { result: AuthorizeResult.CONDITIONAL, pluginId: 'playlist', resourceType: PLAYLIST_LIST_RESOURCE_TYPE, - conditions: playlistConditions.isOwner([ - 'user:default/me', - 'group:default/owner', - ]), + conditions: playlistConditions.isOwner({ + owners: ['user:default/me', 'group:default/owner'], + }), }); }); }); diff --git a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts index 86d103e789..317f5da9c8 100644 --- a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts +++ b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts @@ -68,8 +68,10 @@ export class DefaultPlaylistPermissionPolicy implements PermissionPolicy { ) { return createPlaylistConditionalDecision(request.permission, { anyOf: [ - playlistConditions.isOwner(user?.identity.ownershipEntityRefs ?? []), - playlistConditions.isPublic(), + playlistConditions.isOwner({ + owners: user?.identity.ownershipEntityRefs ?? [], + }), + playlistConditions.isPublic({}), ], }); } @@ -81,7 +83,9 @@ export class DefaultPlaylistPermissionPolicy implements PermissionPolicy { ) { return createPlaylistConditionalDecision( request.permission, - playlistConditions.isOwner(user?.identity.ownershipEntityRefs ?? []), + playlistConditions.isOwner({ + owners: user?.identity.ownershipEntityRefs ?? [], + }), ); } diff --git a/plugins/playlist-backend/src/permissions/rules.ts b/plugins/playlist-backend/src/permissions/rules.ts index de2d3df5e4..897d2e19eb 100644 --- a/plugins/playlist-backend/src/permissions/rules.ts +++ b/plugins/playlist-backend/src/permissions/rules.ts @@ -29,16 +29,19 @@ const createPlaylistPermissionRule = makeCreatePermissionRule< typeof PLAYLIST_LIST_RESOURCE_TYPE >(); -const isOwner = createPlaylistPermissionRule({ +const isOwner = createPlaylistPermissionRule<{ + owners: string[]; +}>({ name: 'IS_OWNER', description: 'Should allow only if the playlist belongs to the user', resourceType: PLAYLIST_LIST_RESOURCE_TYPE, - schema: z.tuple([z.array(z.string().describe('List of owners'))]), - apply: (list: PlaylistMetadata, userOwnershipRefs: string[]) => - userOwnershipRefs.includes(list.owner), - toQuery: (userOwnershipRefs: string[]) => ({ + schema: z.object({ + owners: z.array(z.string().describe('List of owner entity refs')), + }), + apply: (list: PlaylistMetadata, { owners }) => owners.includes(list.owner), + toQuery: ({ owners }) => ({ key: 'owner', - values: userOwnershipRefs, + values: owners, }), }); @@ -46,7 +49,7 @@ const isPublic = createPlaylistPermissionRule({ name: 'IS_PUBLIC', description: 'Should allow only if the playlist is public', resourceType: PLAYLIST_LIST_RESOURCE_TYPE, - schema: z.tuple([]), + schema: z.object({}), apply: (list: PlaylistMetadata) => list.public, toQuery: () => ({ key: 'public', values: [true] }), }); From 1e621ba7c859ccd730802216a7f36b535c1fe204 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Mon, 3 Oct 2022 12:45:48 +0100 Subject: [PATCH 07/33] Updated changeset Signed-off-by: Harry Hogg --- .changeset/kind-bees-suffer.md | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/.changeset/kind-bees-suffer.md b/.changeset/kind-bees-suffer.md index 95899bf064..44bfdb6297 100644 --- a/.changeset/kind-bees-suffer.md +++ b/.changeset/kind-bees-suffer.md @@ -1,8 +1,31 @@ --- '@backstage/plugin-catalog-backend': minor '@backstage/plugin-permission-backend': minor +'@backstage/plugin-permission-common': minor '@backstage/plugin-permission-node': minor '@backstage/plugin-playlist-backend': minor --- -Permission rules now require a schema (ZodSchema) that details the parameters a rule expects. This is to validate the parameters given to a rule and to provide more details of a rule via the metadata endpoint +**BREAKING**: When defining permission rules, they must now also include a ZodSchema that details the parameters the rule expects. This has been added to help better describe the parameters in the response of the metadata endpoint and to validate the parameters before a rule is executed. + +To help with this, we have also made a change to the API of permission rules. Before, the permission rules `toQuery` and `apply` signature expected parameters to be seprate arguments, like so... + +```ts +createPermissionRule({ + apply: (resource, foo, bar) => true, + toQuery: (foo, bar) => {}, +}); +``` + +The API has now changed to expect the parameters as a single object + +```ts +createPermissionRule({ + schema: z.object({ + foo: z.string().describe('Foo value to match'), + bar: z.string().describe('Bar value to match'), + }), + apply: (resource, { foo, bar }) => true, + toQuery: ({ foo, bar }) => {}, +}); +``` From 708ff4761a26d76466f5ce6a91f7d163f6c571a9 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Mon, 3 Oct 2022 13:05:16 +0100 Subject: [PATCH 08/33] Explicitly type optional parameters so the definitions are accurate Signed-off-by: Harry Hogg --- .../src/permissions/rules/createPropertyRule.ts | 5 ++++- .../catalog-backend/src/permissions/rules/hasAnnotation.ts | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts index da487c112c..9ea67ec0b2 100644 --- a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts +++ b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts @@ -20,7 +20,10 @@ import { createCatalogPermissionRule } from './util'; import { z } from 'zod'; export const createPropertyRule = (propertyType: 'metadata' | 'spec') => - createCatalogPermissionRule({ + createCatalogPermissionRule<{ + key: string; + value?: string; + }>({ name: `HAS_${propertyType.toUpperCase()}`, description: `Allow entities which have the specified ${propertyType} subfield.`, resourceType: RESOURCE_TYPE_CATALOG_ENTITY, diff --git a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts index 9651e9d925..28c99e39a1 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts @@ -26,7 +26,10 @@ import { createCatalogPermissionRule } from './util'; * * @alpha */ -export const hasAnnotation = createCatalogPermissionRule({ +export const hasAnnotation = createCatalogPermissionRule<{ + annotation: string; + value?: string; +}>({ name: 'HAS_ANNOTATION', description: 'Allow entities which are annotated with the specified annotation', From 2c37e5efe849ab9cd01e573978b97844c77010a3 Mon Sep 17 00:00:00 2001 From: Harrison Hogg <7130591+HHogg@users.noreply.github.com> Date: Mon, 3 Oct 2022 13:34:29 +0100 Subject: [PATCH 09/33] Update .changeset/kind-bees-suffer.md Co-authored-by: MT Lewis Signed-off-by: Harrison Hogg <7130591+HHogg@users.noreply.github.com> Signed-off-by: Harry Hogg --- .changeset/kind-bees-suffer.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/kind-bees-suffer.md b/.changeset/kind-bees-suffer.md index 44bfdb6297..8e6e03d1bd 100644 --- a/.changeset/kind-bees-suffer.md +++ b/.changeset/kind-bees-suffer.md @@ -6,7 +6,7 @@ '@backstage/plugin-playlist-backend': minor --- -**BREAKING**: When defining permission rules, they must now also include a ZodSchema that details the parameters the rule expects. This has been added to help better describe the parameters in the response of the metadata endpoint and to validate the parameters before a rule is executed. +**BREAKING**: When defining permission rules, it's now necessary to provide a ZodSchema that specifies the parameters the rule expects. This has been added to help better describe the parameters in the response of the metadata endpoint and to validate the parameters before a rule is executed. To help with this, we have also made a change to the API of permission rules. Before, the permission rules `toQuery` and `apply` signature expected parameters to be seprate arguments, like so... From 7a3a0e3422760d53d49c88e0e7fa3c59bb5dcfdc Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 4 Oct 2022 08:59:05 +0100 Subject: [PATCH 10/33] Fixed typo in changeset Signed-off-by: Harry Hogg --- .changeset/kind-bees-suffer.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/kind-bees-suffer.md b/.changeset/kind-bees-suffer.md index 8e6e03d1bd..372d7c1d86 100644 --- a/.changeset/kind-bees-suffer.md +++ b/.changeset/kind-bees-suffer.md @@ -8,7 +8,7 @@ **BREAKING**: When defining permission rules, it's now necessary to provide a ZodSchema that specifies the parameters the rule expects. This has been added to help better describe the parameters in the response of the metadata endpoint and to validate the parameters before a rule is executed. -To help with this, we have also made a change to the API of permission rules. Before, the permission rules `toQuery` and `apply` signature expected parameters to be seprate arguments, like so... +To help with this, we have also made a change to the API of permission rules. Before, the permission rules `toQuery` and `apply` signature expected parameters to be separate arguments, like so... ```ts createPermissionRule({ From 1d4b847c98ddecf4421c8a2f3827db45dcc8463a Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 4 Oct 2022 09:11:10 +0100 Subject: [PATCH 11/33] Explicitly use the schema to infer the types for the permission rule Signed-off-by: Harry Hogg --- .../integration/createPermissionIntegrationRouter.ts | 10 +--------- plugins/permission-node/src/types.ts | 12 ++++++++++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index c5843f6a84..9bcc6c57eb 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -28,7 +28,7 @@ import { PermissionCondition, PermissionCriteria, } from '@backstage/plugin-permission-common'; -import { PermissionRule } from '../types'; +import { NoInfer, PermissionRule } from '../types'; import { createGetRule, isAndCriteria, @@ -135,14 +135,6 @@ const applyConditions = ( return 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 diff --git a/plugins/permission-node/src/types.ts b/plugins/permission-node/src/types.ts index 02ebe07ac4..8725316de0 100644 --- a/plugins/permission-node/src/types.ts +++ b/plugins/permission-node/src/types.ts @@ -17,6 +17,14 @@ import type { PermissionCriteria } from '@backstage/plugin-permission-common'; import { z } from 'zod'; +/** + * Prevent use of type parameter from contributing to type inference. + * + * https://github.com/Microsoft/TypeScript/issues/14829#issuecomment-980401795 + * @ignore + */ +export type NoInfer = T extends infer S ? S : never; + /** * A conditional rule that can be provided in an * {@link @backstage/permission-common#AuthorizeDecision} response to an authorization request. @@ -57,12 +65,12 @@ export type PermissionRule< * arguments supplied for the rule; for example, a rule could be `isOwner` with entityRefs as the * params. */ - apply(resource: TResource, params: TParams): boolean; + apply(resource: TResource, params: NoInfer): boolean; /** * Translate this rule to criteria suitable for use in querying a backing data store. The criteria * can be used for loading a collection of resources efficiently with conditional criteria already * applied. */ - toQuery(params: TParams): PermissionCriteria; + toQuery(params: NoInfer): PermissionCriteria; }; From 755361681cea12a511843c2c6e1c3a00234c1e65 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 4 Oct 2022 09:12:05 +0100 Subject: [PATCH 12/33] Add explanation comment around the schema type and whay we need to remove the optional def for the schema Signed-off-by: Harry Hogg --- plugins/permission-node/src/types.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/plugins/permission-node/src/types.ts b/plugins/permission-node/src/types.ts index 8725316de0..521c8ac3ad 100644 --- a/plugins/permission-node/src/types.ts +++ b/plugins/permission-node/src/types.ts @@ -55,6 +55,10 @@ export type PermissionRule< * A ZodSchema that documents the parameters that this rule accepts. */ schema: z.ZodObject<{ + // Parameters can be optional, however we we want to make sure that the + // parameters are always present in the schema, even if they are undefined. + // We remove the optional flag from the schema, and then add it back in + // with an optional zod type. [P in keyof TParams]-?: TParams[P] extends undefined ? z.ZodOptionalType> : z.ZodType; From 42fa9cdcdbd2d5569de517fb0fb5b00a4045e6d8 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 4 Oct 2022 09:15:33 +0100 Subject: [PATCH 13/33] Removed the parameters count from the permissions metadata endpoint Signed-off-by: Harry Hogg --- .../src/integration/createPermissionIntegrationRouter.test.ts | 2 -- .../src/integration/createPermissionIntegrationRouter.ts | 3 --- 2 files changed, 5 deletions(-) diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 9c6b345faf..99e0da8287 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -618,7 +618,6 @@ describe('createPermissionIntegrationRouter', () => { required: ['foo', 'bar'], type: 'object', }, - parameters: { count: 1 }, }, { name: testRule2.name, @@ -638,7 +637,6 @@ describe('createPermissionIntegrationRouter', () => { required: ['foo'], type: 'object', }, - parameters: { count: 1 }, }, ], }); diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index 9bcc6c57eb..7f355467c3 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -195,9 +195,6 @@ export const createPermissionIntegrationRouter = < description: rule.description, resourceType: rule.resourceType, schema: zodToJsonSchema(rule.schema), - parameters: { - count: rule.toQuery.length, - }, })); return res.json({ permissions, rules: serializableRules }); From 445c5f41a5464f8a7a6ab8c90c6ed1e34131b539 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 4 Oct 2022 09:34:16 +0100 Subject: [PATCH 14/33] Reworded and added missing parameter descriptions Signed-off-by: Harry Hogg --- .../src/permissions/rules/createPropertyRule.ts | 6 ++++-- .../catalog-backend/src/permissions/rules/hasAnnotation.ts | 4 ++-- plugins/catalog-backend/src/permissions/rules/hasLabel.ts | 2 +- .../catalog-backend/src/permissions/rules/isEntityKind.ts | 4 +++- .../catalog-backend/src/permissions/rules/isEntityOwner.ts | 6 +++++- plugins/permission-node/src/types.ts | 2 +- plugins/playlist-backend/src/permissions/rules.ts | 2 +- 7 files changed, 17 insertions(+), 9 deletions(-) diff --git a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts index 9ea67ec0b2..5662b27d48 100644 --- a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts +++ b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts @@ -28,11 +28,13 @@ export const createPropertyRule = (propertyType: 'metadata' | 'spec') => description: `Allow entities which have the specified ${propertyType} subfield.`, resourceType: RESOURCE_TYPE_CATALOG_ENTITY, schema: z.object({ - key: z.string().describe(`The key of the ${propertyType} to match on`), + key: z + .string() + .describe(`Property within the entities ${propertyType} to match on`), value: z .string() .optional() - .describe(`Optional value of the ${propertyType} to match on`), + .describe(`Value of the given property to match on`), }), apply: (resource, { key, value }) => { const foundValue = get(resource[propertyType], key); diff --git a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts index 28c99e39a1..0d80977c09 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts @@ -35,11 +35,11 @@ export const hasAnnotation = createCatalogPermissionRule<{ 'Allow entities which are annotated with the specified annotation', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, schema: z.object({ - annotation: z.string().describe('The name of the annotation to match on'), + annotation: z.string().describe('Name of the annotation to match on'), value: z .string() .optional() - .describe('Optional value of the annotation to match on'), + .describe('Value of the annotation to match on'), }), apply: (resource, { annotation, value }) => !!resource.metadata.annotations?.hasOwnProperty(annotation) && diff --git a/plugins/catalog-backend/src/permissions/rules/hasLabel.ts b/plugins/catalog-backend/src/permissions/rules/hasLabel.ts index fbad7e42d0..6b7b9d9da0 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasLabel.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasLabel.ts @@ -28,7 +28,7 @@ export const hasLabel = createCatalogPermissionRule({ description: 'Allow entities which have the specified label metadata.', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, schema: z.object({ - label: z.string().describe('Name of the label'), + label: z.string().describe('Name of the label to match one'), }), apply: (resource, { label }) => !!resource.metadata.labels?.hasOwnProperty(label), diff --git a/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts b/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts index 8c16f4c5a1..d64b357d9f 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts @@ -28,7 +28,9 @@ export const isEntityKind = createCatalogPermissionRule({ description: 'Allow entities with the specified kind', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, schema: z.object({ - kinds: z.array(z.string()), + kinds: z + .array(z.string()) + .describe('List of kinds to match at least one of'), }), apply(resource, { kinds }) { const resourceKind = resource.kind.toLocaleLowerCase('en-US'); diff --git a/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts b/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts index 27431f09a7..824e5e9ca6 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts @@ -30,7 +30,11 @@ export const isEntityOwner = createCatalogPermissionRule({ description: 'Allow entities owned by the current user', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, schema: z.object({ - claims: z.array(z.string()), + claims: z + .array(z.string()) + .describe( + `List of claims to match at least one on within ${RELATION_OWNED_BY}`, + ), }), apply: (resource, { claims }) => { if (!resource.relations) { diff --git a/plugins/permission-node/src/types.ts b/plugins/permission-node/src/types.ts index 521c8ac3ad..e69c1f3d78 100644 --- a/plugins/permission-node/src/types.ts +++ b/plugins/permission-node/src/types.ts @@ -61,7 +61,7 @@ export type PermissionRule< // with an optional zod type. [P in keyof TParams]-?: TParams[P] extends undefined ? z.ZodOptionalType> - : z.ZodType; + : z.ZodType; }>; /** diff --git a/plugins/playlist-backend/src/permissions/rules.ts b/plugins/playlist-backend/src/permissions/rules.ts index 897d2e19eb..79214b050d 100644 --- a/plugins/playlist-backend/src/permissions/rules.ts +++ b/plugins/playlist-backend/src/permissions/rules.ts @@ -36,7 +36,7 @@ const isOwner = createPlaylistPermissionRule<{ description: 'Should allow only if the playlist belongs to the user', resourceType: PLAYLIST_LIST_RESOURCE_TYPE, schema: z.object({ - owners: z.array(z.string().describe('List of owner entity refs')), + owners: z.array(z.string()).describe('List of owner entity refs'), }), apply: (list: PlaylistMetadata, { owners }) => owners.includes(list.owner), toQuery: ({ owners }) => ({ From fbc636c4a505ed96b2c8da41aed43eb561f4479b Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 4 Oct 2022 10:57:52 +0100 Subject: [PATCH 15/33] Use `z.input` to corrently type the input to correctly reflect optional fields Signed-off-by: Harry Hogg --- .../permissions/rules/createPropertyRule.ts | 5 +--- .../src/permissions/rules/hasAnnotation.ts | 5 +--- plugins/permission-node/src/types.ts | 23 ++++++++++--------- .../playlist-backend/src/permissions/rules.ts | 4 +--- 4 files changed, 15 insertions(+), 22 deletions(-) diff --git a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts index 5662b27d48..29ce67ad53 100644 --- a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts +++ b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts @@ -20,10 +20,7 @@ import { createCatalogPermissionRule } from './util'; import { z } from 'zod'; export const createPropertyRule = (propertyType: 'metadata' | 'spec') => - createCatalogPermissionRule<{ - key: string; - value?: string; - }>({ + createCatalogPermissionRule({ name: `HAS_${propertyType.toUpperCase()}`, description: `Allow entities which have the specified ${propertyType} subfield.`, resourceType: RESOURCE_TYPE_CATALOG_ENTITY, diff --git a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts index 0d80977c09..c8450f3d08 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts @@ -26,10 +26,7 @@ import { createCatalogPermissionRule } from './util'; * * @alpha */ -export const hasAnnotation = createCatalogPermissionRule<{ - annotation: string; - value?: string; -}>({ +export const hasAnnotation = createCatalogPermissionRule({ name: 'HAS_ANNOTATION', description: 'Allow entities which are annotated with the specified annotation', diff --git a/plugins/permission-node/src/types.ts b/plugins/permission-node/src/types.ts index e69c1f3d78..6d39d7f922 100644 --- a/plugins/permission-node/src/types.ts +++ b/plugins/permission-node/src/types.ts @@ -46,6 +46,15 @@ export type PermissionRule< TQuery, TResourceType extends string, TParams extends Record = Record, + TSchema extends z.ZodType = z.ZodObject<{ + // Parameters can be optional, however we we want to make sure that the + // parameters are always present in the schema, even if they are undefined. + // We remove the optional flag from the schema, and then add it back in + // with an optional zod type. + [P in keyof TParams]-?: TParams[P] extends undefined + ? z.ZodOptionalType> + : z.ZodType; + }>, > = { name: string; description: string; @@ -54,27 +63,19 @@ export type PermissionRule< /** * A ZodSchema that documents the parameters that this rule accepts. */ - schema: z.ZodObject<{ - // Parameters can be optional, however we we want to make sure that the - // parameters are always present in the schema, even if they are undefined. - // We remove the optional flag from the schema, and then add it back in - // with an optional zod type. - [P in keyof TParams]-?: TParams[P] extends undefined - ? z.ZodOptionalType> - : z.ZodType; - }>; + schema: TSchema; /** * Apply this rule to a resource already loaded from a backing data source. The params are * arguments supplied for the rule; for example, a rule could be `isOwner` with entityRefs as the * params. */ - apply(resource: TResource, params: NoInfer): boolean; + apply(resource: TResource, params: NoInfer>): boolean; /** * Translate this rule to criteria suitable for use in querying a backing data store. The criteria * can be used for loading a collection of resources efficiently with conditional criteria already * applied. */ - toQuery(params: NoInfer): PermissionCriteria; + toQuery(params: NoInfer>): PermissionCriteria; }; diff --git a/plugins/playlist-backend/src/permissions/rules.ts b/plugins/playlist-backend/src/permissions/rules.ts index 79214b050d..bebb7905b4 100644 --- a/plugins/playlist-backend/src/permissions/rules.ts +++ b/plugins/playlist-backend/src/permissions/rules.ts @@ -29,9 +29,7 @@ const createPlaylistPermissionRule = makeCreatePermissionRule< typeof PLAYLIST_LIST_RESOURCE_TYPE >(); -const isOwner = createPlaylistPermissionRule<{ - owners: string[]; -}>({ +const isOwner = createPlaylistPermissionRule({ name: 'IS_OWNER', description: 'Should allow only if the playlist belongs to the user', resourceType: PLAYLIST_LIST_RESOURCE_TYPE, From 4eb0f6d23dd02fe7a8f62e547f1997a22c761c3a Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 4 Oct 2022 11:39:46 +0100 Subject: [PATCH 16/33] Limited the permission rule parameters to JsonPrimatives and array of Signed-off-by: Harry Hogg --- .../src/permissions/rules/util.ts | 3 +- plugins/permission-common/package.json | 1 + .../permission-common/src/PermissionClient.ts | 2 +- plugins/permission-common/src/types/api.ts | 13 +++++- plugins/permission-common/src/types/index.ts | 2 + .../createConditionExports.test.ts | 6 +-- .../src/integration/createConditionFactory.ts | 10 ++++- .../createConditionTransformer.test.ts | 42 ++++++++----------- .../createPermissionIntegrationRouter.test.ts | 28 ++++++------- .../createPermissionIntegrationRouter.ts | 2 +- .../src/integration/createPermissionRule.ts | 5 ++- plugins/permission-node/src/types.ts | 7 +++- yarn.lock | 1 + 13 files changed, 70 insertions(+), 52 deletions(-) diff --git a/plugins/catalog-backend/src/permissions/rules/util.ts b/plugins/catalog-backend/src/permissions/rules/util.ts index 37544a14f8..ea25e115d2 100644 --- a/plugins/catalog-backend/src/permissions/rules/util.ts +++ b/plugins/catalog-backend/src/permissions/rules/util.ts @@ -16,6 +16,7 @@ import { Entity } from '@backstage/catalog-model'; import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; +import { PermissionRuleParams } from '@backstage/plugin-permission-common'; import { makeCreatePermissionRule, PermissionRule, @@ -30,7 +31,7 @@ import { EntitiesSearchFilter } from '../../catalog/types'; * @alpha */ export type CatalogPermissionRule< - TParams extends Record = Record, + TParams extends PermissionRuleParams = PermissionRuleParams, > = PermissionRule; /** diff --git a/plugins/permission-common/package.json b/plugins/permission-common/package.json index fcf1c47ea6..4c31ecc673 100644 --- a/plugins/permission-common/package.json +++ b/plugins/permission-common/package.json @@ -43,6 +43,7 @@ "dependencies": { "@backstage/config": "workspace:^", "@backstage/errors": "workspace:^", + "@backstage/types": "workspace:^", "cross-fetch": "^3.1.5", "uuid": "^8.0.0", "zod": "^3.11.6" diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 7b429dc46f..c5f56be159 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -41,7 +41,7 @@ const permissionCriteriaSchema: z.ZodSchema< .object({ rule: z.string(), resourceType: z.string(), - params: z.record(z.unknown()), + params: z.record(z.any()), }) .or(z.object({ anyOf: z.array(permissionCriteriaSchema).nonempty() })) .or(z.object({ allOf: z.array(permissionCriteriaSchema).nonempty() })) diff --git a/plugins/permission-common/src/types/api.ts b/plugins/permission-common/src/types/api.ts index 70b9e902b8..ca915a65da 100644 --- a/plugins/permission-common/src/types/api.ts +++ b/plugins/permission-common/src/types/api.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { JsonPrimitive } from '@backstage/types'; import { ResourcePermission } from '.'; import { Permission } from './permission'; @@ -101,7 +102,7 @@ export type PolicyDecision = */ export type PermissionCondition< TResourceType extends string = string, - TParams extends Record = Record, + TParams extends PermissionRuleParams = PermissionRuleParams, > = { resourceType: TResourceType; rule: string; @@ -148,6 +149,16 @@ export type PermissionCriteria = | NotCriteria | TQuery; +/** + * A parameter to a permission rule. + */ +export type PermissionRuleParam = undefined | JsonPrimitive | JsonPrimitive[]; + +/** + * Types that can be used as parameters to permission rules. + */ +export type PermissionRuleParams = Record; + /** * An individual request sent to the permission backend. * @public diff --git a/plugins/permission-common/src/types/index.ts b/plugins/permission-common/src/types/index.ts index f244a70b1a..5702f47c1c 100644 --- a/plugins/permission-common/src/types/index.ts +++ b/plugins/permission-common/src/types/index.ts @@ -33,6 +33,8 @@ export type { PolicyDecision, PermissionCondition, PermissionCriteria, + PermissionRuleParam, + PermissionRuleParams, AllOfCriteria, AnyOfCriteria, NotCriteria, diff --git a/plugins/permission-node/src/integration/createConditionExports.test.ts b/plugins/permission-node/src/integration/createConditionExports.test.ts index a989206fdd..59639f5119 100644 --- a/plugins/permission-node/src/integration/createConditionExports.test.ts +++ b/plugins/permission-node/src/integration/createConditionExports.test.ts @@ -46,7 +46,7 @@ const testIntegration = () => description: 'Test rule 2', resourceType: 'test-resource', schema: z.object({ - foo: z.object({}), + foo: z.string(), }), apply: (_resource: any) => false, toQuery: params => ({ @@ -76,11 +76,11 @@ describe('createConditionExports', () => { }, }); - expect(conditions.testRule2({ foo: { baz: 'quux' } })).toEqual({ + expect(conditions.testRule2({ foo: 'baz' })).toEqual({ rule: 'testRule2', resourceType: 'test-resource', params: { - foo: { baz: 'quux' }, + foo: 'baz', }, }); }); diff --git a/plugins/permission-node/src/integration/createConditionFactory.ts b/plugins/permission-node/src/integration/createConditionFactory.ts index 5b499ab512..8edeb6230f 100644 --- a/plugins/permission-node/src/integration/createConditionFactory.ts +++ b/plugins/permission-node/src/integration/createConditionFactory.ts @@ -14,7 +14,10 @@ * limitations under the License. */ -import { PermissionCondition } from '@backstage/plugin-permission-common'; +import { + PermissionCondition, + PermissionRuleParams, +} from '@backstage/plugin-permission-common'; import { PermissionRule } from '../types'; /** @@ -34,7 +37,10 @@ import { PermissionRule } from '../types'; * @public */ export const createConditionFactory = - >( + < + TResourceType extends string, + TParams extends PermissionRuleParams = PermissionRuleParams, + >( rule: PermissionRule, ) => (params: TParams): PermissionCondition => ({ diff --git a/plugins/permission-node/src/integration/createConditionTransformer.test.ts b/plugins/permission-node/src/integration/createConditionTransformer.test.ts index 5021b7766d..0544ff3ef8 100644 --- a/plugins/permission-node/src/integration/createConditionTransformer.test.ts +++ b/plugins/permission-node/src/integration/createConditionTransformer.test.ts @@ -39,10 +39,10 @@ const transformConditions = createConditionTransformer([ description: 'Test rule 2', resourceType: 'test-resource', schema: z.object({ - foo: z.object({}), + foo: z.string(), }), apply: jest.fn(), - toQuery: jest.fn(({ foo }) => `test-rule-2:${JSON.stringify(foo)}`), + toQuery: jest.fn(({ foo }) => `test-rule-2:${foo}`), }), ]); @@ -67,10 +67,10 @@ describe('createConditionTransformer', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: { foo: 0 }, + foo: '0', }, }, - expectedResult: 'test-rule-2:{"foo":0}', + expectedResult: 'test-rule-2:0', }, { conditions: { @@ -87,13 +87,13 @@ describe('createConditionTransformer', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: {}, + foo: 'b', }, }, ], }, expectedResult: { - anyOf: ['test-rule-1:a/1', 'test-rule-2:{}'], + anyOf: ['test-rule-1:a/1', 'test-rule-2:b'], }, }, { @@ -111,13 +111,13 @@ describe('createConditionTransformer', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: {}, + foo: 'b', }, }, ], }, expectedResult: { - allOf: ['test-rule-1:a/1', 'test-rule-2:{}'], + allOf: ['test-rule-1:a/1', 'test-rule-2:b'], }, }, { @@ -126,12 +126,12 @@ describe('createConditionTransformer', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: {}, + foo: 'a', }, }, }, expectedResult: { - not: 'test-rule-2:{}', + not: 'test-rule-2:a', }, }, { @@ -151,7 +151,7 @@ describe('createConditionTransformer', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: {}, + foo: 'b', }, }, ], @@ -171,9 +171,7 @@ describe('createConditionTransformer', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: { - c: 3, - }, + foo: 'c', }, }, ], @@ -184,11 +182,11 @@ describe('createConditionTransformer', () => { expectedResult: { allOf: [ { - anyOf: ['test-rule-1:a/1', 'test-rule-2:{}'], + anyOf: ['test-rule-1:a/1', 'test-rule-2:b'], }, { not: { - allOf: ['test-rule-1:b/2', 'test-rule-2:{"c":3}'], + allOf: ['test-rule-1:b/2', 'test-rule-2:c'], }, }, ], @@ -211,9 +209,7 @@ describe('createConditionTransformer', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: { - b: 2, - }, + foo: 'b', }, }, ], @@ -234,9 +230,7 @@ describe('createConditionTransformer', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: { - d: 4, - }, + foo: 'd', }, }, }, @@ -248,11 +242,11 @@ describe('createConditionTransformer', () => { expectedResult: { allOf: [ { - anyOf: ['test-rule-1:a/1', 'test-rule-2:{"b":2}'], + anyOf: ['test-rule-1:a/1', 'test-rule-2:b'], }, { not: { - allOf: ['test-rule-1:c/3', { not: 'test-rule-2:{"d":4}' }], + allOf: ['test-rule-1:c/3', { not: 'test-rule-2:d' }], }, }, ], diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 99e0da8287..adfa57b863 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -53,7 +53,7 @@ const testRule2 = createPermissionRule({ description: 'Test rule 2', resourceType: 'test-resource', schema: z.object({ - foo: z.object({}).describe('foo'), + foo: z.string().describe('foo'), }), apply: (_resource: any, _foo) => false, toQuery: _foo => ({}), @@ -105,7 +105,7 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-2', resourceType: 'test-resource', - params: { foo: {} }, + params: { foo: 'b' }, }, ], }, @@ -114,7 +114,7 @@ describe('createPermissionIntegrationRouter', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: {}, + foo: 'c', }, }, }, @@ -134,7 +134,7 @@ describe('createPermissionIntegrationRouter', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: {}, + foo: 'b', }, }, ], @@ -154,7 +154,7 @@ describe('createPermissionIntegrationRouter', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: { c: 3 }, + foo: 'c', }, }, ], @@ -192,7 +192,7 @@ describe('createPermissionIntegrationRouter', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: { foo: 0 }, + foo: 'a', }, }, { @@ -208,7 +208,7 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-2', resourceType: 'test-resource', - params: { foo: {} }, + params: { foo: 'b' }, }, ], }, @@ -228,7 +228,7 @@ describe('createPermissionIntegrationRouter', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: { b: 2 }, + foo: 'b', }, }, ], @@ -249,7 +249,7 @@ describe('createPermissionIntegrationRouter', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: { d: 4 }, + foo: 'd', }, }, }, @@ -310,7 +310,7 @@ describe('createPermissionIntegrationRouter', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: {}, + foo: 'b', }, }, }, @@ -338,7 +338,7 @@ describe('createPermissionIntegrationRouter', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: {}, + foo: 'c', }, }, }, @@ -361,7 +361,7 @@ describe('createPermissionIntegrationRouter', () => { rule: 'test-rule-2', resourceType: 'test-resource', params: { - foo: {}, + foo: 'd', }, }, ], @@ -628,10 +628,8 @@ describe('createPermissionIntegrationRouter', () => { additionalProperties: false, properties: { foo: { - additionalProperties: false, description: 'foo', - properties: {}, - type: 'object', + type: 'string', }, }, required: ['foo'], diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index 7f355467c3..a01006c194 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -46,7 +46,7 @@ const permissionCriteriaSchema: z.ZodSchema< z.object({ rule: z.string(), resourceType: z.string(), - params: z.record(z.unknown()), + params: z.record(z.any()), }), ]), ); diff --git a/plugins/permission-node/src/integration/createPermissionRule.ts b/plugins/permission-node/src/integration/createPermissionRule.ts index ef84a5a26f..ea2e37a671 100644 --- a/plugins/permission-node/src/integration/createPermissionRule.ts +++ b/plugins/permission-node/src/integration/createPermissionRule.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { PermissionRuleParams } from '@backstage/plugin-permission-common'; import { PermissionRule } from '../types'; /** @@ -25,7 +26,7 @@ export const createPermissionRule = < TResource, TQuery, TResourceType extends string, - TParams extends Record, + TParams extends PermissionRuleParams = PermissionRuleParams, >( rule: PermissionRule, ) => rule; @@ -40,7 +41,7 @@ export const createPermissionRule = < */ export const makeCreatePermissionRule = () => - >( + ( rule: PermissionRule, ) => createPermissionRule(rule); diff --git a/plugins/permission-node/src/types.ts b/plugins/permission-node/src/types.ts index 6d39d7f922..34ff52391a 100644 --- a/plugins/permission-node/src/types.ts +++ b/plugins/permission-node/src/types.ts @@ -14,7 +14,10 @@ * limitations under the License. */ -import type { PermissionCriteria } from '@backstage/plugin-permission-common'; +import type { + PermissionCriteria, + PermissionRuleParams, +} from '@backstage/plugin-permission-common'; import { z } from 'zod'; /** @@ -45,7 +48,7 @@ export type PermissionRule< TResource, TQuery, TResourceType extends string, - TParams extends Record = Record, + TParams extends PermissionRuleParams = PermissionRuleParams, TSchema extends z.ZodType = z.ZodObject<{ // Parameters can be optional, however we we want to make sure that the // parameters are always present in the schema, even if they are undefined. diff --git a/yarn.lock b/yarn.lock index bef0e188c6..168ca96586 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6334,6 +6334,7 @@ __metadata: "@backstage/cli": "workspace:^" "@backstage/config": "workspace:^" "@backstage/errors": "workspace:^" + "@backstage/types": "workspace:^" cross-fetch: ^3.1.5 msw: ^0.47.0 uuid: ^8.0.0 From 1ad9969a0f61eb8b973fe7e13a3871a61be87263 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 4 Oct 2022 11:44:06 +0100 Subject: [PATCH 17/33] Updated changeset to include the new limitation of permission params types Signed-off-by: Harry Hogg --- .changeset/kind-bees-suffer.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.changeset/kind-bees-suffer.md b/.changeset/kind-bees-suffer.md index 372d7c1d86..528d06394e 100644 --- a/.changeset/kind-bees-suffer.md +++ b/.changeset/kind-bees-suffer.md @@ -29,3 +29,5 @@ createPermissionRule({ toQuery: ({ foo, bar }) => {}, }); ``` + +One final change made is to limit the possible values for a parameter to primitives and arrays of primitives. From 5a8a8010eed7ca81fdd31c2bba5ae998ac2980d9 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 4 Oct 2022 12:08:38 +0100 Subject: [PATCH 18/33] Removed plugin-permission-backend from changeset Signed-off-by: Harry Hogg --- .changeset/kind-bees-suffer.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.changeset/kind-bees-suffer.md b/.changeset/kind-bees-suffer.md index 528d06394e..41c9874590 100644 --- a/.changeset/kind-bees-suffer.md +++ b/.changeset/kind-bees-suffer.md @@ -1,6 +1,5 @@ --- '@backstage/plugin-catalog-backend': minor -'@backstage/plugin-permission-backend': minor '@backstage/plugin-permission-common': minor '@backstage/plugin-permission-node': minor '@backstage/plugin-playlist-backend': minor From 26e5513c32ecf334364beb7d369a570cb756158c Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 4 Oct 2022 12:50:08 +0100 Subject: [PATCH 19/33] Update API reports Signed-off-by: Harry Hogg --- plugins/catalog-backend/api-report.md | 66 ++++++++++++++----- plugins/permission-common/api-report.md | 9 ++- plugins/permission-common/src/types/api.ts | 4 ++ plugins/permission-node/api-report.md | 44 +++++++++---- .../createPermissionIntegrationRouter.ts | 3 +- .../permission-node/src/integration/util.ts | 8 +++ plugins/permission-node/src/types.ts | 37 ++++++----- plugins/playlist-backend/api-report.md | 9 ++- 8 files changed, 130 insertions(+), 50 deletions(-) diff --git a/plugins/catalog-backend/api-report.md b/plugins/catalog-backend/api-report.md index f18d396114..ea9b0b2277 100644 --- a/plugins/catalog-backend/api-report.md +++ b/plugins/catalog-backend/api-report.md @@ -40,6 +40,7 @@ import { PermissionCondition } from '@backstage/plugin-permission-common'; import { PermissionCriteria } from '@backstage/plugin-permission-common'; import { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { PermissionRule } from '@backstage/plugin-permission-node'; +import { PermissionRuleParams } from '@backstage/plugin-permission-common'; import { PluginDatabaseManager } from '@backstage/backend-common'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { processingResult } from '@backstage/plugin-catalog-node'; @@ -177,37 +178,52 @@ export const catalogConditions: Conditions<{ Entity, EntitiesSearchFilter, 'catalog-entity', - [annotation: string, value?: string | undefined] + { + annotation: string; + value: string | undefined; + } >; hasLabel: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [label: string] + { + label: string; + } >; hasMetadata: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [key: string, value?: string | undefined] + { + key: string; + value: string | undefined; + } >; hasSpec: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [key: string, value?: string | undefined] + { + key: string; + value: string | undefined; + } >; isEntityKind: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [kinds: string[]] + { + kinds: string[]; + } >; isEntityOwner: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [claims: string[]] + { + claims: string[]; + } >; }>; @@ -221,8 +237,9 @@ export type CatalogEnvironment = { }; // @alpha -export type CatalogPermissionRule = - PermissionRule; +export type CatalogPermissionRule< + TParams extends PermissionRuleParams = PermissionRuleParams, +> = PermissionRule; // @alpha export const catalogPlugin: (options?: undefined) => BackendFeature; @@ -280,12 +297,14 @@ export class CodeOwnersProcessor implements CatalogProcessor { export const createCatalogConditionalDecision: ( permission: ResourcePermission<'catalog-entity'>, conditions: PermissionCriteria< - PermissionCondition<'catalog-entity', unknown[]> + PermissionCondition<'catalog-entity', PermissionRuleParams> >, ) => ConditionalPolicyDecision; // @alpha -export const createCatalogPermissionRule: ( +export const createCatalogPermissionRule: < + TParams extends PermissionRuleParams = PermissionRuleParams, +>( rule: PermissionRule, ) => PermissionRule; @@ -448,37 +467,52 @@ export const permissionRules: { Entity, EntitiesSearchFilter, 'catalog-entity', - [annotation: string, value?: string | undefined] + { + annotation: string; + value: string | undefined; + } >; hasLabel: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [label: string] + { + label: string; + } >; hasMetadata: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [key: string, value?: string | undefined] + { + key: string; + value: string | undefined; + } >; hasSpec: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [key: string, value?: string | undefined] + { + key: string; + value: string | undefined; + } >; isEntityKind: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [kinds: string[]] + { + kinds: string[]; + } >; isEntityOwner: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [claims: string[]] + { + claims: string[]; + } >; }; diff --git a/plugins/permission-common/api-report.md b/plugins/permission-common/api-report.md index c0c351d6cd..b51b82c907 100644 --- a/plugins/permission-common/api-report.md +++ b/plugins/permission-common/api-report.md @@ -4,6 +4,7 @@ ```ts import { Config } from '@backstage/config'; +import { JsonPrimitive } from '@backstage/types'; // @public export type AllOfCriteria = { @@ -172,7 +173,7 @@ export class PermissionClient implements PermissionEvaluator { // @public export type PermissionCondition< TResourceType extends string = string, - TParams extends unknown[] = unknown[], + TParams extends PermissionRuleParams = PermissionRuleParams, > = { resourceType: TResourceType; rule: string; @@ -203,6 +204,12 @@ export type PermissionMessageBatch = { items: IdentifiedPermissionMessage[]; }; +// @public +export type PermissionRuleParam = undefined | JsonPrimitive | JsonPrimitive[]; + +// @public +export type PermissionRuleParams = Record; + // @public export type PolicyDecision = | DefinitivePolicyDecision diff --git a/plugins/permission-common/src/types/api.ts b/plugins/permission-common/src/types/api.ts index ca915a65da..77d6206c07 100644 --- a/plugins/permission-common/src/types/api.ts +++ b/plugins/permission-common/src/types/api.ts @@ -151,11 +151,15 @@ export type PermissionCriteria = /** * A parameter to a permission rule. + * + * @public */ export type PermissionRuleParam = undefined | JsonPrimitive | JsonPrimitive[]; /** * Types that can be used as parameters to permission rules. + * + * @public */ export type PermissionRuleParams = Record; diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 055325f5a7..5168eefebc 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -19,6 +19,7 @@ import { Permission } from '@backstage/plugin-permission-common'; import { PermissionCondition } from '@backstage/plugin-permission-common'; import { PermissionCriteria } from '@backstage/plugin-permission-common'; import { PermissionEvaluator } from '@backstage/plugin-permission-common'; +import { PermissionRuleParams } from '@backstage/plugin-permission-common'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { PolicyDecision } from '@backstage/plugin-permission-common'; import { QueryPermissionRequest } from '@backstage/plugin-permission-common'; @@ -54,7 +55,7 @@ export type Condition = TRule extends PermissionRule< infer TResourceType, infer TParams > - ? (...params: TParams) => PermissionCondition + ? (params: TParams) => PermissionCondition : never; // @public @@ -75,7 +76,7 @@ export const createConditionExports: < TResource, TRules extends Record< string, - PermissionRule + PermissionRule >, >(options: { pluginId: string; @@ -86,7 +87,7 @@ export const createConditionExports: < createConditionalDecision: ( permission: ResourcePermission, conditions: PermissionCriteria< - PermissionCondition + PermissionCondition >, ) => ConditionalPolicyDecision; }; @@ -94,15 +95,15 @@ export const createConditionExports: < // @public export const createConditionFactory: < TResourceType extends string, - TParams extends any[], + TParams extends PermissionRuleParams = PermissionRuleParams, >( rule: PermissionRule, -) => (...params: TParams) => PermissionCondition; +) => (params: TParams) => PermissionCondition; // @public export const createConditionTransformer: < TQuery, - TRules extends PermissionRule[], + TRules extends PermissionRule[], >( permissionRules: [...TRules], ) => ConditionTransformer; @@ -114,7 +115,12 @@ export const createPermissionIntegrationRouter: < >(options: { resourceType: TResourceType; permissions?: Permission[] | undefined; - rules: PermissionRule, unknown[]>[]; + rules: PermissionRule< + TResource, + any, + NoInfer, + PermissionRuleParams + >[]; getResources: (resourceRefs: string[]) => Promise<(TResource | undefined)[]>; }) => express.Router; @@ -123,7 +129,7 @@ export const createPermissionRule: < TResource, TQuery, TResourceType extends string, - TParams extends unknown[], + TParams extends PermissionRuleParams = PermissionRuleParams, >( rule: PermissionRule, ) => PermissionRule; @@ -148,7 +154,7 @@ export const makeCreatePermissionRule: < TResource, TQuery, TResourceType extends string, ->() => ( +>() => ( rule: PermissionRule, ) => PermissionRule; @@ -166,16 +172,28 @@ export type PermissionRule< TResource, TQuery, TResourceType extends string, - TParams extends unknown[] = unknown[], + TParams extends PermissionRuleParams = PermissionRuleParams, > = { name: string; description: string; resourceType: TResourceType; - schema: z.ZodSchema; - apply(resource: TResource, ...params: TParams): boolean; - toQuery(...params: TParams): PermissionCriteria; + schema: PermissionRuleSchema; + apply( + resource: TResource, + params: NoInfer>>, + ): boolean; + toQuery( + params: NoInfer>>, + ): PermissionCriteria; }; +// @public +export type PermissionRuleSchema = z.ZodObject<{ + [P in keyof TParams]-?: TParams[P] extends undefined + ? z.ZodOptionalType> + : z.ZodType; +}>; + // @public export type PolicyQuery = { permission: Permission; diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index a01006c194..4f98597e6c 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -28,8 +28,9 @@ import { PermissionCondition, PermissionCriteria, } from '@backstage/plugin-permission-common'; -import { NoInfer, PermissionRule } from '../types'; +import { PermissionRule } from '../types'; import { + NoInfer, createGetRule, isAndCriteria, isNotCriteria, diff --git a/plugins/permission-node/src/integration/util.ts b/plugins/permission-node/src/integration/util.ts index 68b7fff2b8..e448068e17 100644 --- a/plugins/permission-node/src/integration/util.ts +++ b/plugins/permission-node/src/integration/util.ts @@ -22,6 +22,14 @@ import { } from '@backstage/plugin-permission-common'; import { PermissionRule } from '../types'; +/** + * Prevent use of type parameter from contributing to type inference. + * + * https://github.com/Microsoft/TypeScript/issues/14829#issuecomment-980401795 + * @ignore + */ +export type NoInfer = T extends infer S ? S : never; + /** * Utility function used to parse a PermissionCriteria * @param criteria - a PermissionCriteria diff --git a/plugins/permission-node/src/types.ts b/plugins/permission-node/src/types.ts index 34ff52391a..b16c0cac32 100644 --- a/plugins/permission-node/src/types.ts +++ b/plugins/permission-node/src/types.ts @@ -19,14 +19,23 @@ import type { PermissionRuleParams, } from '@backstage/plugin-permission-common'; import { z } from 'zod'; +import { NoInfer } from './integration/util'; /** - * Prevent use of type parameter from contributing to type inference. + * A ZodSchema that reflects the structure of the parameters that are passed to + * into a {@link PermissionRule}. * - * https://github.com/Microsoft/TypeScript/issues/14829#issuecomment-980401795 - * @ignore + * @public */ -export type NoInfer = T extends infer S ? S : never; +export type PermissionRuleSchema = z.ZodObject<{ + // Parameters can be optional, however we we want to make sure that the + // parameters are always present in the schema, even if they are undefined. + // We remove the optional flag from the schema, and then add it back in + // with an optional zod type. + [P in keyof TParams]-?: TParams[P] extends undefined + ? z.ZodOptionalType> + : z.ZodType; +}>; /** * A conditional rule that can be provided in an @@ -49,15 +58,6 @@ export type PermissionRule< TQuery, TResourceType extends string, TParams extends PermissionRuleParams = PermissionRuleParams, - TSchema extends z.ZodType = z.ZodObject<{ - // Parameters can be optional, however we we want to make sure that the - // parameters are always present in the schema, even if they are undefined. - // We remove the optional flag from the schema, and then add it back in - // with an optional zod type. - [P in keyof TParams]-?: TParams[P] extends undefined - ? z.ZodOptionalType> - : z.ZodType; - }>, > = { name: string; description: string; @@ -66,19 +66,24 @@ export type PermissionRule< /** * A ZodSchema that documents the parameters that this rule accepts. */ - schema: TSchema; + schema: PermissionRuleSchema; /** * Apply this rule to a resource already loaded from a backing data source. The params are * arguments supplied for the rule; for example, a rule could be `isOwner` with entityRefs as the * params. */ - apply(resource: TResource, params: NoInfer>): boolean; + apply( + resource: TResource, + params: NoInfer>>, + ): boolean; /** * Translate this rule to criteria suitable for use in querying a backing data store. The criteria * can be used for loading a collection of resources efficiently with conditional criteria already * applied. */ - toQuery(params: NoInfer>): PermissionCriteria; + toQuery( + params: NoInfer>>, + ): PermissionCriteria; }; diff --git a/plugins/playlist-backend/api-report.md b/plugins/playlist-backend/api-report.md index c7109efbfb..faae97a734 100644 --- a/plugins/playlist-backend/api-report.md +++ b/plugins/playlist-backend/api-report.md @@ -15,6 +15,7 @@ import { PermissionCriteria } from '@backstage/plugin-permission-common'; import { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { PermissionPolicy } from '@backstage/plugin-permission-node'; import { PermissionRule } from '@backstage/plugin-permission-node'; +import { PermissionRuleParams } from '@backstage/plugin-permission-common'; import { PlaylistMetadata } from '@backstage/plugin-playlist-common'; import { PluginDatabaseManager } from '@backstage/backend-common'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; @@ -26,7 +27,7 @@ import { ResourcePermission } from '@backstage/plugin-permission-common'; export const createPlaylistConditionalDecision: ( permission: ResourcePermission<'playlist-list'>, conditions: PermissionCriteria< - PermissionCondition<'playlist-list', unknown[]> + PermissionCondition<'playlist-list', PermissionRuleParams> >, ) => ConditionalPolicyDecision; @@ -70,13 +71,15 @@ export const playlistConditions: Conditions<{ PlaylistMetadata, ListPlaylistsFilter, 'playlist-list', - [userOwnershipRefs: string[]] + { + owners: string[]; + } >; isPublic: PermissionRule< PlaylistMetadata, ListPlaylistsFilter, 'playlist-list', - [] + PermissionRuleParams >; }>; From db63ce8b07cc52083ba8da6708a4e0b41b6eadda Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Thu, 6 Oct 2022 09:36:54 +0100 Subject: [PATCH 20/33] Rename schema to paramsSchema Signed-off-by: Harry Hogg --- .changeset/kind-bees-suffer.md | 2 +- .../src/permissions/rules/createPropertyRule.ts | 2 +- .../src/permissions/rules/hasAnnotation.ts | 2 +- plugins/catalog-backend/src/permissions/rules/hasLabel.ts | 2 +- .../catalog-backend/src/permissions/rules/isEntityKind.ts | 2 +- .../src/permissions/rules/isEntityOwner.ts | 2 +- plugins/catalog-backend/src/service/createRouter.test.ts | 2 +- .../src/service/PermissionIntegrationClient.test.ts | 4 ++-- .../src/integration/createConditionExports.test.ts | 4 ++-- .../src/integration/createConditionFactory.test.ts | 2 +- .../src/integration/createConditionTransformer.test.ts | 4 ++-- .../src/integration/createConditionTransformer.ts | 4 ++-- .../integration/createPermissionIntegrationRouter.test.ts | 8 ++++---- .../src/integration/createPermissionIntegrationRouter.ts | 6 +++--- plugins/permission-node/src/integration/util.test.ts | 4 ++-- plugins/permission-node/src/types.ts | 2 +- plugins/playlist-backend/src/permissions/rules.ts | 4 ++-- 17 files changed, 28 insertions(+), 28 deletions(-) diff --git a/.changeset/kind-bees-suffer.md b/.changeset/kind-bees-suffer.md index 41c9874590..18e8afa947 100644 --- a/.changeset/kind-bees-suffer.md +++ b/.changeset/kind-bees-suffer.md @@ -20,7 +20,7 @@ The API has now changed to expect the parameters as a single object ```ts createPermissionRule({ - schema: z.object({ + paramSchema: z.object({ foo: z.string().describe('Foo value to match'), bar: z.string().describe('Bar value to match'), }), diff --git a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts index 29ce67ad53..19a4423846 100644 --- a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts +++ b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts @@ -24,7 +24,7 @@ export const createPropertyRule = (propertyType: 'metadata' | 'spec') => name: `HAS_${propertyType.toUpperCase()}`, description: `Allow entities which have the specified ${propertyType} subfield.`, resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - schema: z.object({ + paramsSchema: z.object({ key: z .string() .describe(`Property within the entities ${propertyType} to match on`), diff --git a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts index c8450f3d08..c24620f41c 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts @@ -31,7 +31,7 @@ export const hasAnnotation = createCatalogPermissionRule({ description: 'Allow entities which are annotated with the specified annotation', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - schema: z.object({ + paramsSchema: z.object({ annotation: z.string().describe('Name of the annotation to match on'), value: z .string() diff --git a/plugins/catalog-backend/src/permissions/rules/hasLabel.ts b/plugins/catalog-backend/src/permissions/rules/hasLabel.ts index 6b7b9d9da0..376534ca6f 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasLabel.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasLabel.ts @@ -27,7 +27,7 @@ export const hasLabel = createCatalogPermissionRule({ name: 'HAS_LABEL', description: 'Allow entities which have the specified label metadata.', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - schema: z.object({ + paramsSchema: z.object({ label: z.string().describe('Name of the label to match one'), }), apply: (resource, { label }) => diff --git a/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts b/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts index d64b357d9f..eee7a790b2 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts @@ -27,7 +27,7 @@ export const isEntityKind = createCatalogPermissionRule({ name: 'IS_ENTITY_KIND', description: 'Allow entities with the specified kind', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - schema: z.object({ + paramsSchema: z.object({ kinds: z .array(z.string()) .describe('List of kinds to match at least one of'), diff --git a/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts b/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts index 824e5e9ca6..c3bfff5c2e 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts @@ -29,7 +29,7 @@ export const isEntityOwner = createCatalogPermissionRule({ name: 'IS_ENTITY_OWNER', description: 'Allow entities owned by the current user', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - schema: z.object({ + paramsSchema: z.object({ claims: z .array(z.string()) .describe( diff --git a/plugins/catalog-backend/src/service/createRouter.test.ts b/plugins/catalog-backend/src/service/createRouter.test.ts index 088e6c71e2..f383df7364 100644 --- a/plugins/catalog-backend/src/service/createRouter.test.ts +++ b/plugins/catalog-backend/src/service/createRouter.test.ts @@ -696,7 +696,7 @@ describe('NextRouter permissioning', () => { name: 'FAKE_RULE', description: 'fake rule', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - schema: z.object({ + paramsSchema: z.object({ foo: z.string(), }), apply: () => true, diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts index 8cbeeaebd9..80782848e7 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts @@ -284,7 +284,7 @@ describe('PermissionIntegrationClient', () => { name: 'RULE_1', description: 'Test rule 1', resourceType: 'test-resource', - schema: z.object({ + paramsSchema: z.object({ input: z.enum(['yes', 'no']), }), apply: (_resource, { input }) => input === 'yes', @@ -297,7 +297,7 @@ describe('PermissionIntegrationClient', () => { description: 'Test rule 2', resourceType: 'test-resource', - schema: z.object({ + paramsSchema: z.object({ input: z.enum(['yes', 'no']), }), apply: (_resource, { input }) => input === 'yes', diff --git a/plugins/permission-node/src/integration/createConditionExports.test.ts b/plugins/permission-node/src/integration/createConditionExports.test.ts index 59639f5119..424b3c3c24 100644 --- a/plugins/permission-node/src/integration/createConditionExports.test.ts +++ b/plugins/permission-node/src/integration/createConditionExports.test.ts @@ -31,7 +31,7 @@ const testIntegration = () => name: 'testRule1', description: 'Test rule 1', resourceType: 'test-resource', - schema: z.object({ + paramsSchema: z.object({ foo: z.string(), bar: z.number(), }), @@ -45,7 +45,7 @@ const testIntegration = () => name: 'testRule2', description: 'Test rule 2', resourceType: 'test-resource', - schema: z.object({ + paramsSchema: z.object({ foo: z.string(), }), apply: (_resource: any) => false, diff --git a/plugins/permission-node/src/integration/createConditionFactory.test.ts b/plugins/permission-node/src/integration/createConditionFactory.test.ts index 191688f0dd..2b3acff342 100644 --- a/plugins/permission-node/src/integration/createConditionFactory.test.ts +++ b/plugins/permission-node/src/integration/createConditionFactory.test.ts @@ -23,7 +23,7 @@ describe('createConditionFactory', () => { name: 'test-rule', description: 'test-description', resourceType: 'test-resource', - schema: z.object({ + paramsSchema: z.object({ foo: z.string(), }), apply: (_resource, _params) => true, diff --git a/plugins/permission-node/src/integration/createConditionTransformer.test.ts b/plugins/permission-node/src/integration/createConditionTransformer.test.ts index 0544ff3ef8..a5507f0b44 100644 --- a/plugins/permission-node/src/integration/createConditionTransformer.test.ts +++ b/plugins/permission-node/src/integration/createConditionTransformer.test.ts @@ -27,7 +27,7 @@ const transformConditions = createConditionTransformer([ name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', - schema: z.object({ + paramsSchema: z.object({ foo: z.string(), bar: z.number(), }), @@ -38,7 +38,7 @@ const transformConditions = createConditionTransformer([ name: 'test-rule-2', description: 'Test rule 2', resourceType: 'test-resource', - schema: z.object({ + paramsSchema: z.object({ foo: z.string(), }), apply: jest.fn(), diff --git a/plugins/permission-node/src/integration/createConditionTransformer.ts b/plugins/permission-node/src/integration/createConditionTransformer.ts index afbd4a9316..dc0b11e90e 100644 --- a/plugins/permission-node/src/integration/createConditionTransformer.ts +++ b/plugins/permission-node/src/integration/createConditionTransformer.ts @@ -47,9 +47,9 @@ const mapConditions = ( } const rule = getRule(criteria.rule); - const result = rule.schema.safeParse(criteria.params); + const result = rule.paramsSchema.safeParse(criteria.params); - if (rule.schema && !result.success) { + if (rule.paramsSchema && !result.success) { throw new InputError(`Parameters to rule are invalid`, result.error); } diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index adfa57b863..55d4d5f4cf 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -40,7 +40,7 @@ const testRule1 = createPermissionRule({ name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', - schema: z.object({ + paramsSchema: z.object({ foo: z.string(), bar: z.number().describe('bar'), }), @@ -52,7 +52,7 @@ const testRule2 = createPermissionRule({ name: 'test-rule-2', description: 'Test rule 2', resourceType: 'test-resource', - schema: z.object({ + paramsSchema: z.object({ foo: z.string().describe('foo'), }), apply: (_resource: any, _foo) => false, @@ -603,7 +603,7 @@ describe('createPermissionIntegrationRouter', () => { name: testRule1.name, description: testRule1.description, resourceType: testRule1.resourceType, - schema: { + paramsSchema: { $schema: 'http://json-schema.org/draft-07/schema#', additionalProperties: false, properties: { @@ -623,7 +623,7 @@ describe('createPermissionIntegrationRouter', () => { name: testRule2.name, description: testRule2.description, resourceType: testRule2.resourceType, - schema: { + paramsSchema: { $schema: 'http://json-schema.org/draft-07/schema#', additionalProperties: false, properties: { diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index 4f98597e6c..c2d99ac4a4 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -127,9 +127,9 @@ const applyConditions = ( } const rule = getRule(criteria.rule); - const result = rule.schema.safeParse(criteria.params); + const result = rule.paramsSchema.safeParse(criteria.params); - if (rule.schema && !result.success) { + if (rule.paramsSchema && !result.success) { throw new InputError(`Parameters to rule are invalid`, result.error); } @@ -195,7 +195,7 @@ export const createPermissionIntegrationRouter = < name: rule.name, description: rule.description, resourceType: rule.resourceType, - schema: zodToJsonSchema(rule.schema), + paramsSchema: zodToJsonSchema(rule.paramsSchema), })); return res.json({ permissions, rules: serializableRules }); diff --git a/plugins/permission-node/src/integration/util.test.ts b/plugins/permission-node/src/integration/util.test.ts index 924ab30757..b1961ae532 100644 --- a/plugins/permission-node/src/integration/util.test.ts +++ b/plugins/permission-node/src/integration/util.test.ts @@ -31,7 +31,7 @@ describe('permission integration utils', () => { name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', - schema: z.object({}), + paramsSchema: z.object({}), apply: jest.fn(), toQuery: jest.fn(), }); @@ -40,7 +40,7 @@ describe('permission integration utils', () => { name: 'test-rule-2', description: 'Test rule 2', resourceType: 'test-resource', - schema: z.object({}), + paramsSchema: z.object({}), apply: jest.fn(), toQuery: jest.fn(), }); diff --git a/plugins/permission-node/src/types.ts b/plugins/permission-node/src/types.ts index b16c0cac32..df21889fb9 100644 --- a/plugins/permission-node/src/types.ts +++ b/plugins/permission-node/src/types.ts @@ -66,7 +66,7 @@ export type PermissionRule< /** * A ZodSchema that documents the parameters that this rule accepts. */ - schema: PermissionRuleSchema; + paramsSchema: PermissionRuleSchema; /** * Apply this rule to a resource already loaded from a backing data source. The params are diff --git a/plugins/playlist-backend/src/permissions/rules.ts b/plugins/playlist-backend/src/permissions/rules.ts index bebb7905b4..cd6711befb 100644 --- a/plugins/playlist-backend/src/permissions/rules.ts +++ b/plugins/playlist-backend/src/permissions/rules.ts @@ -33,7 +33,7 @@ const isOwner = createPlaylistPermissionRule({ name: 'IS_OWNER', description: 'Should allow only if the playlist belongs to the user', resourceType: PLAYLIST_LIST_RESOURCE_TYPE, - schema: z.object({ + paramsSchema: z.object({ owners: z.array(z.string()).describe('List of owner entity refs'), }), apply: (list: PlaylistMetadata, { owners }) => owners.includes(list.owner), @@ -47,7 +47,7 @@ const isPublic = createPlaylistPermissionRule({ name: 'IS_PUBLIC', description: 'Should allow only if the playlist is public', resourceType: PLAYLIST_LIST_RESOURCE_TYPE, - schema: z.object({}), + paramsSchema: z.object({}), apply: (list: PlaylistMetadata) => list.public, toQuery: () => ({ key: 'public', values: [true] }), }); From a3fef466ef733370e55fbb7fc1bc1e36c3fbb0bf Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Thu, 6 Oct 2022 10:06:42 +0100 Subject: [PATCH 21/33] Updated API reports Signed-off-by: Harry Hogg --- plugins/permission-node/api-report.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 5168eefebc..b272aa9b1c 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -177,7 +177,7 @@ export type PermissionRule< name: string; description: string; resourceType: TResourceType; - schema: PermissionRuleSchema; + paramsSchema: PermissionRuleSchema; apply( resource: TResource, params: NoInfer>>, From fa40df2bc7f0f42173fbcdfc2b1f6cc22afd444e Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Thu, 6 Oct 2022 12:04:31 +0100 Subject: [PATCH 22/33] Made changs to allow params and schemas to be defaulted and required only when there is params defined. Co-authored-by: Vincenzo Scamporlino Co-authored-by: Mike Lewis Signed-off-by: Harry Hogg --- plugins/catalog-backend/api-report.md | 2 +- plugins/permission-common/api-report.md | 6 ++- plugins/permission-common/src/types/api.ts | 6 ++- plugins/permission-node/api-report.md | 12 ++--- .../src/integration/createConditionExports.ts | 4 +- .../createConditionFactory.test.ts | 1 + .../src/integration/createConditionFactory.ts | 26 ++++++----- .../integration/createConditionTransformer.ts | 6 +-- .../createPermissionIntegrationRouter.test.ts | 44 ++----------------- .../createPermissionIntegrationRouter.ts | 10 ++--- .../src/integration/createPermissionRule.ts | 4 +- .../src/integration/util.test.ts | 3 -- plugins/permission-node/src/types.ts | 4 +- plugins/playlist-backend/api-report.md | 2 +- .../DefaultPlaylistPermissionPolicy.test.ts | 4 +- .../DefaultPlaylistPermissionPolicy.ts | 2 +- .../playlist-backend/src/permissions/rules.ts | 1 - 17 files changed, 53 insertions(+), 84 deletions(-) diff --git a/plugins/catalog-backend/api-report.md b/plugins/catalog-backend/api-report.md index ea9b0b2277..6935243cf5 100644 --- a/plugins/catalog-backend/api-report.md +++ b/plugins/catalog-backend/api-report.md @@ -303,7 +303,7 @@ export const createCatalogConditionalDecision: ( // @alpha export const createCatalogPermissionRule: < - TParams extends PermissionRuleParams = PermissionRuleParams, + TParams extends PermissionRuleParams = undefined, >( rule: PermissionRule, ) => PermissionRule; diff --git a/plugins/permission-common/api-report.md b/plugins/permission-common/api-report.md index b51b82c907..1c2476d20d 100644 --- a/plugins/permission-common/api-report.md +++ b/plugins/permission-common/api-report.md @@ -177,7 +177,7 @@ export type PermissionCondition< > = { resourceType: TResourceType; rule: string; - params: TParams; + params?: TParams; }; // @public @@ -208,7 +208,9 @@ export type PermissionMessageBatch = { export type PermissionRuleParam = undefined | JsonPrimitive | JsonPrimitive[]; // @public -export type PermissionRuleParams = Record; +export type PermissionRuleParams = + | undefined + | Record; // @public export type PolicyDecision = diff --git a/plugins/permission-common/src/types/api.ts b/plugins/permission-common/src/types/api.ts index 77d6206c07..5e7a2bb349 100644 --- a/plugins/permission-common/src/types/api.ts +++ b/plugins/permission-common/src/types/api.ts @@ -106,7 +106,7 @@ export type PermissionCondition< > = { resourceType: TResourceType; rule: string; - params: TParams; + params?: TParams; }; /** @@ -161,7 +161,9 @@ export type PermissionRuleParam = undefined | JsonPrimitive | JsonPrimitive[]; * * @public */ -export type PermissionRuleParams = Record; +export type PermissionRuleParams = + | undefined + | Record; /** * An individual request sent to the permission backend. diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index b272aa9b1c..4bc654cd73 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -55,7 +55,9 @@ export type Condition = TRule extends PermissionRule< infer TResourceType, infer TParams > - ? (params: TParams) => PermissionCondition + ? undefined extends TParams + ? () => PermissionCondition + : (params: TParams) => PermissionCondition : never; // @public @@ -98,7 +100,7 @@ export const createConditionFactory: < TParams extends PermissionRuleParams = PermissionRuleParams, >( rule: PermissionRule, -) => (params: TParams) => PermissionCondition; +) => (args_0: TParams) => PermissionCondition; // @public export const createConditionTransformer: < @@ -129,7 +131,7 @@ export const createPermissionRule: < TResource, TQuery, TResourceType extends string, - TParams extends PermissionRuleParams = PermissionRuleParams, + TParams extends PermissionRuleParams = undefined, >( rule: PermissionRule, ) => PermissionRule; @@ -154,7 +156,7 @@ export const makeCreatePermissionRule: < TResource, TQuery, TResourceType extends string, ->() => ( +>() => ( rule: PermissionRule, ) => PermissionRule; @@ -177,7 +179,7 @@ export type PermissionRule< name: string; description: string; resourceType: TResourceType; - paramsSchema: PermissionRuleSchema; + paramsSchema?: PermissionRuleSchema; apply( resource: TResource, params: NoInfer>>, diff --git a/plugins/permission-node/src/integration/createConditionExports.ts b/plugins/permission-node/src/integration/createConditionExports.ts index 3921c2dfe3..1c82434add 100644 --- a/plugins/permission-node/src/integration/createConditionExports.ts +++ b/plugins/permission-node/src/integration/createConditionExports.ts @@ -36,7 +36,9 @@ export type Condition = TRule extends PermissionRule< infer TResourceType, infer TParams > - ? (params: TParams) => PermissionCondition + ? undefined extends TParams + ? () => PermissionCondition + : (params: TParams) => PermissionCondition : never; /** diff --git a/plugins/permission-node/src/integration/createConditionFactory.test.ts b/plugins/permission-node/src/integration/createConditionFactory.test.ts index 2b3acff342..520fcc01f9 100644 --- a/plugins/permission-node/src/integration/createConditionFactory.test.ts +++ b/plugins/permission-node/src/integration/createConditionFactory.test.ts @@ -37,6 +37,7 @@ describe('createConditionFactory', () => { describe('return value', () => { it('constructs a condition with the rule name and supplied params', () => { const conditionFactory = createConditionFactory(testRule); + expect( conditionFactory({ foo: 'bar', diff --git a/plugins/permission-node/src/integration/createConditionFactory.ts b/plugins/permission-node/src/integration/createConditionFactory.ts index 8edeb6230f..3a31e07dac 100644 --- a/plugins/permission-node/src/integration/createConditionFactory.ts +++ b/plugins/permission-node/src/integration/createConditionFactory.ts @@ -36,15 +36,17 @@ import { PermissionRule } from '../types'; * * @public */ -export const createConditionFactory = - < - TResourceType extends string, - TParams extends PermissionRuleParams = PermissionRuleParams, - >( - rule: PermissionRule, - ) => - (params: TParams): PermissionCondition => ({ - rule: rule.name, - resourceType: rule.resourceType, - params, - }); +export const createConditionFactory = < + TResourceType extends string, + TParams extends PermissionRuleParams = PermissionRuleParams, +>( + rule: PermissionRule, +) => { + return (...args: [TParams]): PermissionCondition => { + return { + rule: rule.name, + resourceType: rule.resourceType, + params: args[0], + }; + }; +}; diff --git a/plugins/permission-node/src/integration/createConditionTransformer.ts b/plugins/permission-node/src/integration/createConditionTransformer.ts index dc0b11e90e..4b6f933f79 100644 --- a/plugins/permission-node/src/integration/createConditionTransformer.ts +++ b/plugins/permission-node/src/integration/createConditionTransformer.ts @@ -47,13 +47,13 @@ const mapConditions = ( } const rule = getRule(criteria.rule); - const result = rule.paramsSchema.safeParse(criteria.params); + const result = rule.paramsSchema?.safeParse(criteria.params); - if (rule.paramsSchema && !result.success) { + if (result && !result.success) { throw new InputError(`Parameters to rule are invalid`, result.error); } - return rule.toQuery(criteria.params); + return rule.toQuery(criteria.params ?? {}); }; /** diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 55d4d5f4cf..32d814465f 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -52,11 +52,8 @@ const testRule2 = createPermissionRule({ name: 'test-rule-2', description: 'Test rule 2', resourceType: 'test-resource', - paramsSchema: z.object({ - foo: z.string().describe('foo'), - }), - apply: (_resource: any, _foo) => false, - toQuery: _foo => ({}), + apply: (_resource: any) => false, + toQuery: () => ({}), }); describe('createPermissionIntegrationRouter', () => { @@ -105,7 +102,6 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-2', resourceType: 'test-resource', - params: { foo: 'b' }, }, ], }, @@ -113,9 +109,6 @@ describe('createPermissionIntegrationRouter', () => { not: { rule: 'test-rule-2', resourceType: 'test-resource', - params: { - foo: 'c', - }, }, }, { @@ -133,9 +126,6 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-2', resourceType: 'test-resource', - params: { - foo: 'b', - }, }, ], }, @@ -153,9 +143,6 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-2', resourceType: 'test-resource', - params: { - foo: 'c', - }, }, ], }, @@ -191,9 +178,6 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-2', resourceType: 'test-resource', - params: { - foo: 'a', - }, }, { allOf: [ @@ -208,7 +192,6 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-2', resourceType: 'test-resource', - params: { foo: 'b' }, }, ], }, @@ -227,9 +210,6 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-2', resourceType: 'test-resource', - params: { - foo: 'b', - }, }, ], }, @@ -248,9 +228,6 @@ describe('createPermissionIntegrationRouter', () => { not: { rule: 'test-rule-2', resourceType: 'test-resource', - params: { - foo: 'd', - }, }, }, ], @@ -309,9 +286,6 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-2', resourceType: 'test-resource', - params: { - foo: 'b', - }, }, }, { @@ -337,9 +311,6 @@ describe('createPermissionIntegrationRouter', () => { not: { rule: 'test-rule-2', resourceType: 'test-resource', - params: { - foo: 'c', - }, }, }, }, @@ -360,9 +331,6 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-2', resourceType: 'test-resource', - params: { - foo: 'd', - }, }, ], }, @@ -626,13 +594,7 @@ describe('createPermissionIntegrationRouter', () => { paramsSchema: { $schema: 'http://json-schema.org/draft-07/schema#', additionalProperties: false, - properties: { - foo: { - description: 'foo', - type: 'string', - }, - }, - required: ['foo'], + properties: {}, type: 'object', }, }, diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index c2d99ac4a4..52f34d7e6a 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -47,7 +47,7 @@ const permissionCriteriaSchema: z.ZodSchema< z.object({ rule: z.string(), resourceType: z.string(), - params: z.record(z.any()), + params: z.record(z.any()).optional(), }), ]), ); @@ -127,13 +127,13 @@ const applyConditions = ( } const rule = getRule(criteria.rule); - const result = rule.paramsSchema.safeParse(criteria.params); + const result = rule.paramsSchema?.safeParse(criteria.params); - if (rule.paramsSchema && !result.success) { + if (result && !result.success) { throw new InputError(`Parameters to rule are invalid`, result.error); } - return rule.apply(resource, criteria.params); + return rule.apply(resource, criteria.params ?? {}); }; /** @@ -195,7 +195,7 @@ export const createPermissionIntegrationRouter = < name: rule.name, description: rule.description, resourceType: rule.resourceType, - paramsSchema: zodToJsonSchema(rule.paramsSchema), + paramsSchema: zodToJsonSchema(rule.paramsSchema ?? z.object({})), })); return res.json({ permissions, rules: serializableRules }); diff --git a/plugins/permission-node/src/integration/createPermissionRule.ts b/plugins/permission-node/src/integration/createPermissionRule.ts index ea2e37a671..44187e58a6 100644 --- a/plugins/permission-node/src/integration/createPermissionRule.ts +++ b/plugins/permission-node/src/integration/createPermissionRule.ts @@ -26,7 +26,7 @@ export const createPermissionRule = < TResource, TQuery, TResourceType extends string, - TParams extends PermissionRuleParams = PermissionRuleParams, + TParams extends PermissionRuleParams = undefined, >( rule: PermissionRule, ) => rule; @@ -41,7 +41,7 @@ export const createPermissionRule = < */ export const makeCreatePermissionRule = () => - ( + ( 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 b1961ae532..6c1c270953 100644 --- a/plugins/permission-node/src/integration/util.test.ts +++ b/plugins/permission-node/src/integration/util.test.ts @@ -14,7 +14,6 @@ * limitations under the License. */ -import { z } from 'zod'; import { createPermissionRule } from './createPermissionRule'; import { createGetRule, @@ -31,7 +30,6 @@ describe('permission integration utils', () => { name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', - paramsSchema: z.object({}), apply: jest.fn(), toQuery: jest.fn(), }); @@ -40,7 +38,6 @@ describe('permission integration utils', () => { name: 'test-rule-2', description: 'Test rule 2', resourceType: 'test-resource', - paramsSchema: z.object({}), apply: jest.fn(), toQuery: jest.fn(), }); diff --git a/plugins/permission-node/src/types.ts b/plugins/permission-node/src/types.ts index df21889fb9..6b15df407e 100644 --- a/plugins/permission-node/src/types.ts +++ b/plugins/permission-node/src/types.ts @@ -64,9 +64,9 @@ export type PermissionRule< resourceType: TResourceType; /** - * A ZodSchema that documents the parameters that this rule accepts. + * A ZodSchema that reflects the structure of the parameters that are passed to */ - paramsSchema: PermissionRuleSchema; + paramsSchema?: PermissionRuleSchema; /** * Apply this rule to a resource already loaded from a backing data source. The params are diff --git a/plugins/playlist-backend/api-report.md b/plugins/playlist-backend/api-report.md index faae97a734..6b51f316c4 100644 --- a/plugins/playlist-backend/api-report.md +++ b/plugins/playlist-backend/api-report.md @@ -79,7 +79,7 @@ export const playlistConditions: Conditions<{ PlaylistMetadata, ListPlaylistsFilter, 'playlist-list', - PermissionRuleParams + undefined >; }>; diff --git a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts index 3d761bfd35..17ee4a840a 100644 --- a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts +++ b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts @@ -70,7 +70,7 @@ describe('DefaultPlaylistPermissionPolicy', () => { playlistConditions.isOwner({ owners: ['user:default/me', 'group:default/owner'], }), - playlistConditions.isPublic({}), + playlistConditions.isPublic(), ], }, }); @@ -91,7 +91,7 @@ describe('DefaultPlaylistPermissionPolicy', () => { playlistConditions.isOwner({ owners: ['user:default/me', 'group:default/owner'], }), - playlistConditions.isPublic({}), + playlistConditions.isPublic(), ], }, }); diff --git a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts index 317f5da9c8..b4556fbb4f 100644 --- a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts +++ b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts @@ -71,7 +71,7 @@ export class DefaultPlaylistPermissionPolicy implements PermissionPolicy { playlistConditions.isOwner({ owners: user?.identity.ownershipEntityRefs ?? [], }), - playlistConditions.isPublic({}), + playlistConditions.isPublic(), ], }); } diff --git a/plugins/playlist-backend/src/permissions/rules.ts b/plugins/playlist-backend/src/permissions/rules.ts index cd6711befb..99f4725972 100644 --- a/plugins/playlist-backend/src/permissions/rules.ts +++ b/plugins/playlist-backend/src/permissions/rules.ts @@ -47,7 +47,6 @@ const isPublic = createPlaylistPermissionRule({ name: 'IS_PUBLIC', description: 'Should allow only if the playlist is public', resourceType: PLAYLIST_LIST_RESOURCE_TYPE, - paramsSchema: z.object({}), apply: (list: PlaylistMetadata) => list.public, toQuery: () => ({ key: 'public', values: [true] }), }); From 78e7698e4b70e1f07f031228ec5f754ea5dbdd5d Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Fri, 7 Oct 2022 12:08:00 +0100 Subject: [PATCH 23/33] Removed unnecessary tupling of params Signed-off-by: Harry Hogg --- .../permission-node/src/integration/createConditionFactory.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/permission-node/src/integration/createConditionFactory.ts b/plugins/permission-node/src/integration/createConditionFactory.ts index 3a31e07dac..7f8cb1ced8 100644 --- a/plugins/permission-node/src/integration/createConditionFactory.ts +++ b/plugins/permission-node/src/integration/createConditionFactory.ts @@ -42,11 +42,11 @@ export const createConditionFactory = < >( rule: PermissionRule, ) => { - return (...args: [TParams]): PermissionCondition => { + return (params: TParams): PermissionCondition => { return { rule: rule.name, resourceType: rule.resourceType, - params: args[0], + params, }; }; }; From eb25f7e12d99fbe175ac97d3b46e791c3508232a Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Fri, 7 Oct 2022 15:19:09 +0100 Subject: [PATCH 24/33] Broke up changesets to reflect the package changes Signed-off-by: Harry Hogg --- .changeset/kind-bees-suffer.md | 2 -- .changeset/quick-meals-talk.md | 19 ++++++++++++++++++ .changeset/seven-panthers-chew.md | 33 +++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 2 deletions(-) create mode 100644 .changeset/quick-meals-talk.md create mode 100644 .changeset/seven-panthers-chew.md diff --git a/.changeset/kind-bees-suffer.md b/.changeset/kind-bees-suffer.md index 18e8afa947..bdd6d1f594 100644 --- a/.changeset/kind-bees-suffer.md +++ b/.changeset/kind-bees-suffer.md @@ -1,8 +1,6 @@ --- -'@backstage/plugin-catalog-backend': minor '@backstage/plugin-permission-common': minor '@backstage/plugin-permission-node': minor -'@backstage/plugin-playlist-backend': minor --- **BREAKING**: When defining permission rules, it's now necessary to provide a ZodSchema that specifies the parameters the rule expects. This has been added to help better describe the parameters in the response of the metadata endpoint and to validate the parameters before a rule is executed. diff --git a/.changeset/quick-meals-talk.md b/.changeset/quick-meals-talk.md new file mode 100644 index 0000000000..bde70d25fd --- /dev/null +++ b/.changeset/quick-meals-talk.md @@ -0,0 +1,19 @@ +--- +'@backstage/plugin-playlist-backend': minor +--- + +**BREAKING** Due to the changes made in the Permission framework. The playlist backends permission rules have change to reflect this. + +As an example the `playlistConditions.isOwner` API has changed from + +```ts +playlistConditions.isOwner(['user:default/me', 'group:default/owner']); +``` + +to the new API + +```ts +playlistConditions.isOwner({ + owners: ['user:default/me', 'group:default/owner'], +}); +``` diff --git a/.changeset/seven-panthers-chew.md b/.changeset/seven-panthers-chew.md new file mode 100644 index 0000000000..7b44f2f633 --- /dev/null +++ b/.changeset/seven-panthers-chew.md @@ -0,0 +1,33 @@ +--- +'@backstage/plugin-catalog-backend': minor +--- + +**BREAKING** Due to the changes made in the Permission framework. The catalogs permission rules and the API of `createCatalogPermissionRule` have been changed to reflect the change from individual function parameters to a single object parameter and the addition of the `paramsSchema`. + +As an example for the `hasLabel` rule. The API before the change was + +```ts +hasLabel.apply(entity, 'backstage.io/testLabel'); +hasLabel.toQuery('backstage.io/testLabel'); +``` + +and the API after the change now is + +```ts +hasLabel.apply(entity, { + label: 'backstage.io/testLabel', +}); + +hasLabel.toQuery({ + label: 'backstage.io/testLabel', +}); +``` + +This applies to all of the permission rules exported by the catalog backend. + +- `hasAnnotation` +- `hasLabel` +- `hasMetadata` +- `hasSpec` +- `isEntityKind` +- `isEntityOwner` From f6aa16fb7a3788f61919a959086f63853d580a49 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Fri, 7 Oct 2022 16:16:22 +0100 Subject: [PATCH 25/33] Updated API reports Signed-off-by: Harry Hogg --- plugins/permission-node/api-report.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 4bc654cd73..3895b4b3cb 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -100,7 +100,7 @@ export const createConditionFactory: < TParams extends PermissionRuleParams = PermissionRuleParams, >( rule: PermissionRule, -) => (args_0: TParams) => PermissionCondition; +) => (params: TParams) => PermissionCondition; // @public export const createConditionTransformer: < From a72ae031901ee2a49a1c40ec01ffb9108498b2f1 Mon Sep 17 00:00:00 2001 From: Harrison Hogg <7130591+HHogg@users.noreply.github.com> Date: Tue, 11 Oct 2022 10:18:49 +0100 Subject: [PATCH 26/33] Update .changeset/quick-meals-talk.md Co-authored-by: MT Lewis Signed-off-by: Harrison Hogg <7130591+HHogg@users.noreply.github.com> --- .changeset/quick-meals-talk.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/quick-meals-talk.md b/.changeset/quick-meals-talk.md index bde70d25fd..b9ce0189f0 100644 --- a/.changeset/quick-meals-talk.md +++ b/.changeset/quick-meals-talk.md @@ -2,7 +2,7 @@ '@backstage/plugin-playlist-backend': minor --- -**BREAKING** Due to the changes made in the Permission framework. The playlist backends permission rules have change to reflect this. +**BREAKING** The exported permission rules have changed to reflect the breaking changes made to the PermissionRule type. As an example the `playlistConditions.isOwner` API has changed from From 9db6099633e1fe73770079789f90b2ca240fb928 Mon Sep 17 00:00:00 2001 From: Harrison Hogg <7130591+HHogg@users.noreply.github.com> Date: Tue, 11 Oct 2022 10:19:01 +0100 Subject: [PATCH 27/33] Update .changeset/quick-meals-talk.md Co-authored-by: MT Lewis Signed-off-by: Harrison Hogg <7130591+HHogg@users.noreply.github.com> --- .changeset/quick-meals-talk.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/quick-meals-talk.md b/.changeset/quick-meals-talk.md index b9ce0189f0..b27034d684 100644 --- a/.changeset/quick-meals-talk.md +++ b/.changeset/quick-meals-talk.md @@ -4,7 +4,7 @@ **BREAKING** The exported permission rules have changed to reflect the breaking changes made to the PermissionRule type. -As an example the `playlistConditions.isOwner` API has changed from +For example, the `playlistConditions.isOwner` API has changed from: ```ts playlistConditions.isOwner(['user:default/me', 'group:default/owner']); From ddf3986add991869fb67cba9ec2f903f0558665f Mon Sep 17 00:00:00 2001 From: Harrison Hogg <7130591+HHogg@users.noreply.github.com> Date: Tue, 11 Oct 2022 10:19:11 +0100 Subject: [PATCH 28/33] Update .changeset/quick-meals-talk.md Co-authored-by: MT Lewis Signed-off-by: Harrison Hogg <7130591+HHogg@users.noreply.github.com> --- .changeset/quick-meals-talk.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/quick-meals-talk.md b/.changeset/quick-meals-talk.md index b27034d684..5834928e77 100644 --- a/.changeset/quick-meals-talk.md +++ b/.changeset/quick-meals-talk.md @@ -10,7 +10,7 @@ For example, the `playlistConditions.isOwner` API has changed from: playlistConditions.isOwner(['user:default/me', 'group:default/owner']); ``` -to the new API +to: ```ts playlistConditions.isOwner({ From bbbe968e10c6f08beef90a84102f13e218e706f1 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 11 Oct 2022 11:46:28 +0100 Subject: [PATCH 29/33] Fixed allowing optional params outside of the toQuery and apply Signed-off-by: Harry Hogg Co-authored-by: Mike Lewis --- .../createConditionExports.test.ts | 8 +++--- plugins/permission-node/src/types.ts | 27 +++---------------- 2 files changed, 6 insertions(+), 29 deletions(-) diff --git a/plugins/permission-node/src/integration/createConditionExports.test.ts b/plugins/permission-node/src/integration/createConditionExports.test.ts index 424b3c3c24..dad57e2b8b 100644 --- a/plugins/permission-node/src/integration/createConditionExports.test.ts +++ b/plugins/permission-node/src/integration/createConditionExports.test.ts @@ -46,7 +46,7 @@ const testIntegration = () => description: 'Test rule 2', resourceType: 'test-resource', paramsSchema: z.object({ - foo: z.string(), + foo: z.string().optional(), }), apply: (_resource: any) => false, toQuery: params => ({ @@ -76,12 +76,10 @@ describe('createConditionExports', () => { }, }); - expect(conditions.testRule2({ foo: 'baz' })).toEqual({ + expect(conditions.testRule2({})).toEqual({ rule: 'testRule2', resourceType: 'test-resource', - params: { - foo: 'baz', - }, + params: {}, }); }); }); diff --git a/plugins/permission-node/src/types.ts b/plugins/permission-node/src/types.ts index 6b15df407e..36e30714f6 100644 --- a/plugins/permission-node/src/types.ts +++ b/plugins/permission-node/src/types.ts @@ -21,22 +21,6 @@ import type { import { z } from 'zod'; import { NoInfer } from './integration/util'; -/** - * A ZodSchema that reflects the structure of the parameters that are passed to - * into a {@link PermissionRule}. - * - * @public - */ -export type PermissionRuleSchema = z.ZodObject<{ - // Parameters can be optional, however we we want to make sure that the - // parameters are always present in the schema, even if they are undefined. - // We remove the optional flag from the schema, and then add it back in - // with an optional zod type. - [P in keyof TParams]-?: TParams[P] extends undefined - ? z.ZodOptionalType> - : z.ZodType; -}>; - /** * A conditional rule that can be provided in an * {@link @backstage/permission-common#AuthorizeDecision} response to an authorization request. @@ -66,24 +50,19 @@ export type PermissionRule< /** * A ZodSchema that reflects the structure of the parameters that are passed to */ - paramsSchema?: PermissionRuleSchema; + paramsSchema?: z.ZodSchema; /** * Apply this rule to a resource already loaded from a backing data source. The params are * arguments supplied for the rule; for example, a rule could be `isOwner` with entityRefs as the * params. */ - apply( - resource: TResource, - params: NoInfer>>, - ): boolean; + apply(resource: TResource, params: NoInfer): boolean; /** * Translate this rule to criteria suitable for use in querying a backing data store. The criteria * can be used for loading a collection of resources efficiently with conditional criteria already * applied. */ - toQuery( - params: NoInfer>>, - ): PermissionCriteria; + toQuery(params: NoInfer): PermissionCriteria; }; From 30592654f9148eae63978cf39938bd7128369d8f Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 11 Oct 2022 11:51:43 +0100 Subject: [PATCH 30/33] Added link to Zod in changelog Signed-off-by: Harry Hogg --- .changeset/kind-bees-suffer.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/kind-bees-suffer.md b/.changeset/kind-bees-suffer.md index bdd6d1f594..79e9c0a027 100644 --- a/.changeset/kind-bees-suffer.md +++ b/.changeset/kind-bees-suffer.md @@ -3,7 +3,7 @@ '@backstage/plugin-permission-node': minor --- -**BREAKING**: When defining permission rules, it's now necessary to provide a ZodSchema that specifies the parameters the rule expects. This has been added to help better describe the parameters in the response of the metadata endpoint and to validate the parameters before a rule is executed. +**BREAKING**: When defining permission rules, it's now necessary to provide a [ZodSchema](https://github.com/colinhacks/zod) that specifies the parameters the rule expects. This has been added to help better describe the parameters in the response of the metadata endpoint and to validate the parameters before a rule is executed. To help with this, we have also made a change to the API of permission rules. Before, the permission rules `toQuery` and `apply` signature expected parameters to be separate arguments, like so... From 3f25d863b0adb9b2036339cdfe031559cc1a6235 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 11 Oct 2022 11:55:40 +0100 Subject: [PATCH 31/33] Updated changelog for plugin-catalog-backend to be more focused on createCatalogConditionalDecision Signed-off-by: Harry Hogg --- .changeset/seven-panthers-chew.md | 30 +----------------------------- 1 file changed, 1 insertion(+), 29 deletions(-) diff --git a/.changeset/seven-panthers-chew.md b/.changeset/seven-panthers-chew.md index 7b44f2f633..33c9166d41 100644 --- a/.changeset/seven-panthers-chew.md +++ b/.changeset/seven-panthers-chew.md @@ -2,32 +2,4 @@ '@backstage/plugin-catalog-backend': minor --- -**BREAKING** Due to the changes made in the Permission framework. The catalogs permission rules and the API of `createCatalogPermissionRule` have been changed to reflect the change from individual function parameters to a single object parameter and the addition of the `paramsSchema`. - -As an example for the `hasLabel` rule. The API before the change was - -```ts -hasLabel.apply(entity, 'backstage.io/testLabel'); -hasLabel.toQuery('backstage.io/testLabel'); -``` - -and the API after the change now is - -```ts -hasLabel.apply(entity, { - label: 'backstage.io/testLabel', -}); - -hasLabel.toQuery({ - label: 'backstage.io/testLabel', -}); -``` - -This applies to all of the permission rules exported by the catalog backend. - -- `hasAnnotation` -- `hasLabel` -- `hasMetadata` -- `hasSpec` -- `isEntityKind` -- `isEntityOwner` +**BREAKING** The exported permission rules and the API of `createCatalogConditionalDecision` have changed to reflect the breaking changes made to the PermissionRule type. From 04db0e8afbfd84157fc7a4ffc848d431e7d75679 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 11 Oct 2022 12:32:26 +0100 Subject: [PATCH 32/33] Updated API Reports Signed-off-by: Harry Hogg --- plugins/catalog-backend/api-report.md | 12 ++++++------ plugins/permission-node/api-report.md | 18 +++--------------- 2 files changed, 9 insertions(+), 21 deletions(-) diff --git a/plugins/catalog-backend/api-report.md b/plugins/catalog-backend/api-report.md index 6935243cf5..c9402945c1 100644 --- a/plugins/catalog-backend/api-report.md +++ b/plugins/catalog-backend/api-report.md @@ -179,8 +179,8 @@ export const catalogConditions: Conditions<{ EntitiesSearchFilter, 'catalog-entity', { + value?: string | undefined; annotation: string; - value: string | undefined; } >; hasLabel: PermissionRule< @@ -196,8 +196,8 @@ export const catalogConditions: Conditions<{ EntitiesSearchFilter, 'catalog-entity', { + value?: string | undefined; key: string; - value: string | undefined; } >; hasSpec: PermissionRule< @@ -205,8 +205,8 @@ export const catalogConditions: Conditions<{ EntitiesSearchFilter, 'catalog-entity', { + value?: string | undefined; key: string; - value: string | undefined; } >; isEntityKind: PermissionRule< @@ -468,8 +468,8 @@ export const permissionRules: { EntitiesSearchFilter, 'catalog-entity', { + value?: string | undefined; annotation: string; - value: string | undefined; } >; hasLabel: PermissionRule< @@ -485,8 +485,8 @@ export const permissionRules: { EntitiesSearchFilter, 'catalog-entity', { + value?: string | undefined; key: string; - value: string | undefined; } >; hasSpec: PermissionRule< @@ -494,8 +494,8 @@ export const permissionRules: { EntitiesSearchFilter, 'catalog-entity', { + value?: string | undefined; key: string; - value: string | undefined; } >; isEntityKind: PermissionRule< diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 3895b4b3cb..7feb20d7aa 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -179,23 +179,11 @@ export type PermissionRule< name: string; description: string; resourceType: TResourceType; - paramsSchema?: PermissionRuleSchema; - apply( - resource: TResource, - params: NoInfer>>, - ): boolean; - toQuery( - params: NoInfer>>, - ): PermissionCriteria; + paramsSchema?: z.ZodSchema; + apply(resource: TResource, params: NoInfer): boolean; + toQuery(params: NoInfer): PermissionCriteria; }; -// @public -export type PermissionRuleSchema = z.ZodObject<{ - [P in keyof TParams]-?: TParams[P] extends undefined - ? z.ZodOptionalType> - : z.ZodType; -}>; - // @public export type PolicyQuery = { permission: Permission; From cd6c66ef9c37933d32d6aa49e0e3f90e9172f0c8 Mon Sep 17 00:00:00 2001 From: Harrison Hogg <7130591+HHogg@users.noreply.github.com> Date: Tue, 11 Oct 2022 14:11:12 +0100 Subject: [PATCH 33/33] Update .changeset/seven-panthers-chew.md Co-authored-by: Patrik Oldsberg Signed-off-by: Harrison Hogg <7130591+HHogg@users.noreply.github.com> --- .changeset/seven-panthers-chew.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/seven-panthers-chew.md b/.changeset/seven-panthers-chew.md index 33c9166d41..110df1d389 100644 --- a/.changeset/seven-panthers-chew.md +++ b/.changeset/seven-panthers-chew.md @@ -2,4 +2,4 @@ '@backstage/plugin-catalog-backend': minor --- -**BREAKING** The exported permission rules and the API of `createCatalogConditionalDecision` have changed to reflect the breaking changes made to the PermissionRule type. +The exported permission rules and the API of `createCatalogConditionalDecision` have changed to reflect the breaking changes made to the `PermissionRule` type. Note that all involved types are exported from `@backstage/plugin-catalog-backend/alpha`