diff --git a/.changeset/kind-bees-suffer.md b/.changeset/kind-bees-suffer.md new file mode 100644 index 0000000000..79e9c0a027 --- /dev/null +++ b/.changeset/kind-bees-suffer.md @@ -0,0 +1,30 @@ +--- +'@backstage/plugin-permission-common': minor +'@backstage/plugin-permission-node': minor +--- + +**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... + +```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({ + paramSchema: 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 }) => {}, +}); +``` + +One final change made is to limit the possible values for a parameter to primitives and arrays of primitives. diff --git a/.changeset/quick-meals-talk.md b/.changeset/quick-meals-talk.md new file mode 100644 index 0000000000..5834928e77 --- /dev/null +++ b/.changeset/quick-meals-talk.md @@ -0,0 +1,19 @@ +--- +'@backstage/plugin-playlist-backend': minor +--- + +**BREAKING** The exported permission rules have changed to reflect the breaking changes made to the PermissionRule type. + +For example, the `playlistConditions.isOwner` API has changed from: + +```ts +playlistConditions.isOwner(['user:default/me', 'group:default/owner']); +``` + +to: + +```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..110df1d389 --- /dev/null +++ b/.changeset/seven-panthers-chew.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': minor +--- + +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` diff --git a/plugins/catalog-backend/api-report.md b/plugins/catalog-backend/api-report.md index d23c6f7519..10b9afc291 100644 --- a/plugins/catalog-backend/api-report.md +++ b/plugins/catalog-backend/api-report.md @@ -45,6 +45,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'; @@ -171,37 +172,52 @@ export const catalogConditions: Conditions<{ Entity, EntitiesSearchFilter, 'catalog-entity', - [annotation: string, value?: string | undefined] + { + value?: string | undefined; + annotation: string; + } >; hasLabel: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [label: string] + { + label: string; + } >; hasMetadata: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [key: string, value?: string | undefined] + { + value?: string | undefined; + key: string; + } >; hasSpec: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [key: string, value?: string | undefined] + { + value?: string | undefined; + key: string; + } >; isEntityKind: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [kinds: string[]] + { + kinds: string[]; + } >; isEntityOwner: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [claims: string[]] + { + claims: string[]; + } >; }>; @@ -215,8 +231,9 @@ export type CatalogEnvironment = { }; // @alpha -export type CatalogPermissionRule = - PermissionRule; +export type CatalogPermissionRule< + TParams extends PermissionRuleParams = PermissionRuleParams, +> = PermissionRule; // @alpha export const catalogPlugin: (options?: undefined) => BackendFeature; @@ -274,12 +291,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 = undefined, +>( rule: PermissionRule, ) => PermissionRule; @@ -442,37 +461,52 @@ export const permissionRules: { Entity, EntitiesSearchFilter, 'catalog-entity', - [annotation: string, value?: string | undefined] + { + value?: string | undefined; + annotation: string; + } >; hasLabel: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [label: string] + { + label: string; + } >; hasMetadata: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [key: string, value?: string | undefined] + { + value?: string | undefined; + key: string; + } >; hasSpec: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [key: string, value?: string | undefined] + { + value?: string | undefined; + key: string; + } >; isEntityKind: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [kinds: string[]] + { + kinds: string[]; + } >; isEntityOwner: PermissionRule< Entity, EntitiesSearchFilter, 'catalog-entity', - [claims: string[]] + { + claims: string[]; + } >; }; 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 57de5eb922..19a4423846 100644 --- a/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts +++ b/plugins/catalog-backend/src/permissions/rules/createPropertyRule.ts @@ -15,16 +15,25 @@ */ 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, - apply: (resource: Entity, key: string, value?: string) => { + paramsSchema: z.object({ + key: z + .string() + .describe(`Property within the entities ${propertyType} to match on`), + value: z + .string() + .optional() + .describe(`Value of the given property to match on`), + }), + apply: (resource, { key, value }) => { const foundValue = get(resource[propertyType], key); if (Array.isArray(foundValue)) { @@ -38,7 +47,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 22dbd307a6..c24620f41c 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasAnnotation.ts @@ -14,8 +14,8 @@ * 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'; /** @@ -31,12 +31,19 @@ export const hasAnnotation = createCatalogPermissionRule({ description: 'Allow entities which are annotated with the specified annotation', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - apply: (resource: Entity, annotation: string, value?: string) => + paramsSchema: z.object({ + annotation: z.string().describe('Name of the annotation to match on'), + value: z + .string() + .optional() + .describe('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 9f37cbfb59..376534ca6f 100644 --- a/plugins/catalog-backend/src/permissions/rules/hasLabel.ts +++ b/plugins/catalog-backend/src/permissions/rules/hasLabel.ts @@ -14,8 +14,8 @@ * 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'; /** @@ -27,9 +27,12 @@ export const hasLabel = createCatalogPermissionRule({ name: 'HAS_LABEL', description: 'Allow entities which have the specified label metadata.', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - apply: (resource: Entity, label: string) => + paramsSchema: z.object({ + label: z.string().describe('Name of the label to match one'), + }), + 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 3bd600e8fe..eee7a790b2 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityKind.ts @@ -13,8 +13,8 @@ * 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'; import { createCatalogPermissionRule } from './util'; @@ -27,11 +27,16 @@ export const isEntityKind = createCatalogPermissionRule({ name: 'IS_ENTITY_KIND', description: 'Allow entities with the specified kind', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - apply(resource: Entity, kinds: string[]) { + paramsSchema: z.object({ + 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'); 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 5403da2310..c3bfff5c2e 100644 --- a/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts +++ b/plugins/catalog-backend/src/permissions/rules/isEntityOwner.ts @@ -14,8 +14,9 @@ * 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'; /** @@ -28,7 +29,14 @@ export const isEntityOwner = createCatalogPermissionRule({ name: 'IS_ENTITY_OWNER', description: 'Allow entities owned by the current user', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - apply: (resource: Entity, claims: string[]) => { + paramsSchema: z.object({ + 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) { return false; } @@ -37,7 +45,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..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, @@ -29,8 +30,9 @@ import { EntitiesSearchFilter } from '../../catalog/types'; * * @alpha */ -export type CatalogPermissionRule = - PermissionRule; +export type CatalogPermissionRule< + TParams extends PermissionRuleParams = PermissionRuleParams, +> = 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 16a36eb8c4..f383df7364 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,9 @@ describe('NextRouter permissioning', () => { name: 'FAKE_RULE', description: 'fake rule', resourceType: RESOURCE_TYPE_CATALOG_ENTITY, + paramsSchema: z.object({ + foo: z.string(), + }), apply: () => true, toQuery: () => ({ key: '', values: [] }), }); @@ -758,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 4c50e1646f..80782848e7 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', () => { @@ -38,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' }, + }, ], }, }; @@ -279,7 +284,10 @@ describe('PermissionIntegrationClient', () => { name: 'RULE_1', description: 'Test rule 1', resourceType: 'test-resource', - apply: (_resource: any, input: 'yes' | 'no') => input === 'yes', + paramsSchema: z.object({ + input: z.enum(['yes', 'no']), + }), + apply: (_resource, { input }) => input === 'yes', toQuery: () => { throw new Error('Not implemented'); }, @@ -288,7 +296,11 @@ describe('PermissionIntegrationClient', () => { name: 'RULE_2', description: 'Test rule 2', resourceType: 'test-resource', - apply: (_resource: any, input: 'yes' | 'no') => input === 'yes', + + paramsSchema: z.object({ + input: z.enum(['yes', 'no']), + }), + apply: (_resource, { input }) => input === 'yes', toQuery: () => { throw new Error('Not implemented'); }, @@ -344,7 +356,9 @@ describe('PermissionIntegrationClient', () => { conditions: { rule: 'RULE_1', resourceType: 'test-resource', - params: ['no'], + params: { + input: 'no', + }, }, }, ]), @@ -365,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', + }, }, }, ], @@ -382,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/api-report.md b/plugins/permission-common/api-report.md index c0c351d6cd..1c2476d20d 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,11 +173,11 @@ 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; - params: TParams; + params?: TParams; }; // @public @@ -203,6 +204,14 @@ export type PermissionMessageBatch = { items: IdentifiedPermissionMessage[]; }; +// @public +export type PermissionRuleParam = undefined | JsonPrimitive | JsonPrimitive[]; + +// @public +export type PermissionRuleParams = + | undefined + | Record; + // @public export type PolicyDecision = | DefinitivePolicyDecision diff --git a/plugins/permission-common/package.json b/plugins/permission-common/package.json index a8fe1e7947..f9f9ce6f94 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.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..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.array(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 f29a639d18..5e7a2bb349 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,11 +102,11 @@ export type PolicyDecision = */ export type PermissionCondition< TResourceType extends string = string, - TParams extends unknown[] = unknown[], + TParams extends PermissionRuleParams = PermissionRuleParams, > = { resourceType: TResourceType; rule: string; - params: TParams; + params?: TParams; }; /** @@ -148,6 +149,22 @@ export type PermissionCriteria = | NotCriteria | TQuery; +/** + * 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 = + | undefined + | 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/api-report.md b/plugins/permission-node/api-report.md index 702d363137..7feb20d7aa 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -19,11 +19,13 @@ 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'; import { ResourcePermission } from '@backstage/plugin-permission-common'; import { TokenManager } from '@backstage/backend-common'; +import { z } from 'zod'; // @public export type ApplyConditionsRequest = { @@ -53,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 @@ -74,7 +78,7 @@ export const createConditionExports: < TResource, TRules extends Record< string, - PermissionRule + PermissionRule >, >(options: { pluginId: string; @@ -85,7 +89,7 @@ export const createConditionExports: < createConditionalDecision: ( permission: ResourcePermission, conditions: PermissionCriteria< - PermissionCondition + PermissionCondition >, ) => ConditionalPolicyDecision; }; @@ -93,15 +97,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; @@ -113,7 +117,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; @@ -122,7 +131,7 @@ export const createPermissionRule: < TResource, TQuery, TResourceType extends string, - TParams extends unknown[], + TParams extends PermissionRuleParams = undefined, >( rule: PermissionRule, ) => PermissionRule; @@ -147,7 +156,7 @@ export const makeCreatePermissionRule: < TResource, TQuery, TResourceType extends string, ->() => ( +>() => ( rule: PermissionRule, ) => PermissionRule; @@ -165,13 +174,14 @@ export type PermissionRule< TResource, TQuery, TResourceType extends string, - TParams extends unknown[] = unknown[], + TParams extends PermissionRuleParams = PermissionRuleParams, > = { name: string; description: string; resourceType: TResourceType; - apply(resource: TResource, ...params: TParams): boolean; - toQuery(...params: TParams): PermissionCriteria; + paramsSchema?: z.ZodSchema; + apply(resource: TResource, params: NoInfer): boolean; + toQuery(params: NoInfer): PermissionCriteria; }; // @public diff --git a/plugins/permission-node/package.json b/plugins/permission-node/package.json index db2479dc27..588c0aafe4 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/createConditionExports.test.ts b/plugins/permission-node/src/integration/createConditionExports.test.ts index 54d5652996..dad57e2b8b 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,23 +31,28 @@ const testIntegration = () => name: 'testRule1', description: 'Test rule 1', resourceType: 'test-resource', - apply: jest.fn( - (_resource: any, _firstParam: string, _secondParam: number) => true, - ), - toQuery: jest.fn((firstParam: string, secondParam: number) => ({ + paramsSchema: 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', - apply: jest.fn((_resource: any, _firstParam: object) => false), - toQuery: jest.fn((firstParam: object) => ({ + paramsSchema: z.object({ + foo: z.string().optional(), + }), + apply: (_resource: any) => false, + toQuery: params => ({ query: 'testRule2', - params: [firstParam], - })), + params, + }), }), }, }); @@ -56,16 +62,24 @@ 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({})).toEqual({ rule: 'testRule2', resourceType: 'test-resource', - params: [{ baz: 'quux' }], + params: {}, }); }); }); @@ -85,7 +99,10 @@ describe('createConditionExports', () => { { rule: 'testRule1', resourceType: 'test-resource', - params: ['a', 1], + params: { + foo: 'a', + bar: 1, + }, }, ], }), @@ -98,7 +115,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..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 1941b5ac0b..520fcc01f9 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,8 +23,11 @@ describe('createConditionFactory', () => { name: 'test-rule', description: 'test-description', resourceType: 'test-resource', - apply: jest.fn(), - toQuery: jest.fn(), + paramsSchema: z.object({ + foo: z.string(), + }), + apply: (_resource, _params) => true, + toQuery: _params => ({}), }); it('returns a function', () => { @@ -33,10 +37,17 @@ 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..7f8cb1ced8 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'; /** @@ -33,12 +36,17 @@ import { PermissionRule } from '../types'; * * @public */ -export const createConditionFactory = - ( - 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 (params: TParams): PermissionCondition => { + return { + 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 917449561d..a5507f0b44 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,20 +27,22 @@ const transformConditions = createConditionTransformer([ name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', + paramsSchema: 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', + paramsSchema: z.object({ + foo: z.string(), + }), apply: jest.fn(), - toQuery: jest.fn( - (firstParam: object) => `test-rule-2:${JSON.stringify(firstParam)}`, - ), + toQuery: jest.fn(({ foo }) => `test-rule-2:${foo}`), }), ]); @@ -52,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', }, @@ -60,9 +66,11 @@ describe('createConditionTransformer', () => { conditions: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{ foo: 0 }], + params: { + foo: '0', + }, }, - expectedResult: 'test-rule-2:{"foo":0}', + expectedResult: 'test-rule-2:0', }, { conditions: { @@ -70,13 +78,22 @@ 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: 'b', + }, }, - { rule: 'test-rule-2', resourceType: 'test-resource', params: [{}] }, ], }, expectedResult: { - anyOf: ['test-rule-1:a/1', 'test-rule-2:{}'], + anyOf: ['test-rule-1:a/1', 'test-rule-2:b'], }, }, { @@ -85,13 +102,22 @@ 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: 'b', + }, }, - { rule: 'test-rule-2', resourceType: 'test-resource', params: [{}] }, ], }, expectedResult: { - allOf: ['test-rule-1:a/1', 'test-rule-2:{}'], + allOf: ['test-rule-1:a/1', 'test-rule-2:b'], }, }, { @@ -99,11 +125,13 @@ describe('createConditionTransformer', () => { not: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{}], + params: { + foo: 'a', + }, }, }, expectedResult: { - not: 'test-rule-2:{}', + not: 'test-rule-2:a', }, }, { @@ -114,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: 'b', + }, }, ], }, @@ -129,61 +162,16 @@ 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 }], - }, - ], - }, - }, - ], - }, - expectedResult: { - allOf: [ - { - anyOf: ['test-rule-1:a/1', 'test-rule-2:{}'], - }, - { - not: { - allOf: ['test-rule-1:b/2', 'test-rule-2:{"c":3}'], - }, - }, - ], - }, - }, - { - conditions: { - allOf: [ - { - anyOf: [ - { - rule: 'test-rule-1', - resourceType: 'test-resource', - params: ['a', 1], - }, - { - rule: 'test-rule-2', - resourceType: 'test-resource', - params: [{ b: 2 }], - }, - ], - }, - { - not: { - allOf: [ - { - rule: 'test-rule-1', - resourceType: 'test-resource', - params: ['c', 3], - }, - { - not: { - rule: 'test-rule-2', - resourceType: 'test-resource', - params: [{ d: 4 }], + params: { + foo: 'c', }, }, ], @@ -194,11 +182,71 @@ 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:b/2', 'test-rule-2:c'], + }, + }, + ], + }, + }, + { + conditions: { + allOf: [ + { + anyOf: [ + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: { + foo: 'a', + bar: 1, + }, + }, + { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: { + foo: 'b', + }, + }, + ], + }, + { + not: { + allOf: [ + { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: { + foo: 'c', + bar: 3, + }, + }, + { + not: { + rule: 'test-rule-2', + resourceType: 'test-resource', + params: { + foo: 'd', + }, + }, + }, + ], + }, + }, + ], + }, + expectedResult: { + allOf: [ + { + anyOf: ['test-rule-1:a/1', 'test-rule-2:b'], + }, + { + not: { + allOf: ['test-rule-1:c/3', { not: 'test-rule-2:d' }], }, }, ], diff --git a/plugins/permission-node/src/integration/createConditionTransformer.ts b/plugins/permission-node/src/integration/createConditionTransformer.ts index 112bb30d01..4b6f933f79 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.paramsSchema?.safeParse(criteria.params); + + if (result && !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 ac4005c487..32d814465f 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,16 +40,20 @@ const testRule1 = createPermissionRule({ name: 'test-rule-1', description: 'Test rule 1', resourceType: 'test-resource', - apply: (_resource: any, _firstParam: string, _secondParam: number) => true, - toQuery: (_firstParam: string, _secondParam: number) => ({}), + paramsSchema: 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', - apply: (_resource: any, _firstParam: object) => false, - toQuery: (_firstParam: object) => ({}), + apply: (_resource: any) => false, + toQuery: () => ({}), }); describe('createPermissionIntegrationRouter', () => { @@ -79,23 +84,31 @@ 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', }, - { rule: 'test-rule-2', resourceType: 'test-resource', params: [{}] }, ], }, { not: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{}], }, }, { @@ -105,12 +118,14 @@ 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: [{}], }, ], }, @@ -120,12 +135,14 @@ 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 }], }, ], }, @@ -161,16 +178,21 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-2', resourceType: 'test-resource', - params: [{ 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', }, - { rule: 'test-rule-2', resourceType: 'test-resource', params: [{}] }, ], }, { @@ -180,12 +202,14 @@ 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 }], }, ], }, @@ -195,13 +219,15 @@ 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 }], }, }, ], @@ -247,7 +273,10 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: { + foo: 'a', + bar: 1, + }, }, }, { @@ -257,7 +286,6 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [], }, }, { @@ -268,7 +296,10 @@ describe('createPermissionIntegrationRouter', () => { not: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: { + foo: 'a', + bar: 1, + }, }, }, }, @@ -280,7 +311,6 @@ describe('createPermissionIntegrationRouter', () => { not: { rule: 'test-rule-2', resourceType: 'test-resource', - params: [], }, }, }, @@ -293,12 +323,14 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: { + foo: 'a', + bar: 1, + }, }, { rule: 'test-rule-2', resourceType: 'test-resource', - params: [], }, ], }, @@ -342,7 +374,9 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-incorrect-resource-1', - params: [{}], + params: { + foo: {}, + }, }, }, { @@ -352,7 +386,9 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [{}], + params: { + foo: {}, + }, }, }, { @@ -362,7 +398,9 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-incorrect-resource-2', - params: [{}], + params: { + foo: {}, + }, }, }, ], @@ -390,7 +428,7 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: {}, }, }, ], @@ -427,7 +465,10 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: { + foo: 'a', + bar: 1, + }, }, }, { @@ -437,7 +478,10 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: { + foo: 'a', + bar: 1, + }, }, }, { @@ -447,7 +491,10 @@ describe('createPermissionIntegrationRouter', () => { conditions: { rule: 'test-rule-1', resourceType: 'test-resource', - params: [], + params: { + foo: 'a', + bar: 1, + }, }, }, ], @@ -524,13 +571,32 @@ describe('createPermissionIntegrationRouter', () => { name: testRule1.name, description: testRule1.description, resourceType: testRule1.resourceType, - parameters: { count: 2 }, + paramsSchema: { + $schema: 'http://json-schema.org/draft-07/schema#', + additionalProperties: false, + properties: { + foo: { + type: 'string', + }, + bar: { + description: 'bar', + type: 'number', + }, + }, + required: ['foo', 'bar'], + type: 'object', + }, }, { name: testRule2.name, description: testRule2.description, resourceType: testRule2.resourceType, - parameters: { count: 1 }, + paramsSchema: { + $schema: 'http://json-schema.org/draft-07/schema#', + additionalProperties: false, + properties: {}, + type: 'object', + }, }, ], }); diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index 28abc2f48d..52f34d7e6a 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 { @@ -29,6 +30,7 @@ import { } from '@backstage/plugin-permission-common'; import { PermissionRule } from '../types'; import { + NoInfer, createGetRule, isAndCriteria, isNotCriteria, @@ -45,7 +47,7 @@ const permissionCriteriaSchema: z.ZodSchema< z.object({ rule: z.string(), resourceType: z.string(), - params: z.array(z.unknown()), + params: z.record(z.any()).optional(), }), ]), ); @@ -124,16 +126,15 @@ const applyConditions = ( return !applyConditions(criteria.not, resource, getRule); } - return getRule(criteria.rule).apply(resource, ...criteria.params); -}; + const rule = getRule(criteria.rule); + const result = rule.paramsSchema?.safeParse(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; + if (result && !result.success) { + throw new InputError(`Parameters to rule are invalid`, result.error); + } + + return rule.apply(resource, criteria.params ?? {}); +}; /** * Create an express Router which provides an authorization route to allow @@ -194,9 +195,7 @@ export const createPermissionIntegrationRouter = < name: rule.name, description: rule.description, resourceType: rule.resourceType, - parameters: { - count: rule.toQuery.length, - }, + 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 6673fe76de..44187e58a6 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 unknown[], + TParams extends PermissionRuleParams = undefined, >( rule: PermissionRule, ) => rule; @@ -40,7 +41,7 @@ export const createPermissionRule = < */ export const makeCreatePermissionRule = () => - ( + ( rule: PermissionRule, ) => createPermissionRule(rule); 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 66b4a7b22b..36e30714f6 100644 --- a/plugins/permission-node/src/types.ts +++ b/plugins/permission-node/src/types.ts @@ -14,7 +14,12 @@ * 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'; +import { NoInfer } from './integration/util'; /** * A conditional rule that can be provided in an @@ -36,23 +41,28 @@ export type PermissionRule< TResource, TQuery, TResourceType extends string, - TParams extends unknown[] = unknown[], + TParams extends PermissionRuleParams = PermissionRuleParams, > = { name: string; description: string; resourceType: TResourceType; + /** + * A ZodSchema that reflects the structure of the parameters that are passed to + */ + 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: 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; }; diff --git a/plugins/playlist-backend/api-report.md b/plugins/playlist-backend/api-report.md index c7109efbfb..6b51f316c4 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', - [] + undefined >; }>; diff --git a/plugins/playlist-backend/package.json b/plugins/playlist-backend/package.json index d9790a7d48..136d2a2108 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/DefaultPlaylistPermissionPolicy.test.ts b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts index 179b0d7f9b..17ee4a840a 100644 --- a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts +++ b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts @@ -67,10 +67,9 @@ describe('DefaultPlaylistPermissionPolicy', () => { resourceType: PLAYLIST_LIST_RESOURCE_TYPE, conditions: { anyOf: [ - playlistConditions.isOwner([ - 'user:default/me', - 'group:default/owner', - ]), + playlistConditions.isOwner({ + owners: ['user:default/me', 'group:default/owner'], + }), playlistConditions.isPublic(), ], }, @@ -89,10 +88,9 @@ describe('DefaultPlaylistPermissionPolicy', () => { resourceType: PLAYLIST_LIST_RESOURCE_TYPE, conditions: { anyOf: [ - playlistConditions.isOwner([ - 'user:default/me', - 'group:default/owner', - ]), + 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..b4556fbb4f 100644 --- a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts +++ b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts @@ -68,7 +68,9 @@ export class DefaultPlaylistPermissionPolicy implements PermissionPolicy { ) { return createPlaylistConditionalDecision(request.permission, { anyOf: [ - playlistConditions.isOwner(user?.identity.ownershipEntityRefs ?? []), + 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 cbf81bee65..99f4725972 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,11 +33,13 @@ const isOwner = createPlaylistPermissionRule({ name: 'IS_OWNER', description: 'Should allow only if the playlist belongs to the user', resourceType: PLAYLIST_LIST_RESOURCE_TYPE, - apply: (list: PlaylistMetadata, userOwnershipRefs: string[]) => - userOwnershipRefs.includes(list.owner), - toQuery: (userOwnershipRefs: string[]) => ({ + paramsSchema: 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, }), }); diff --git a/yarn.lock b/yarn.lock index 9af65cfa61..7c369f3d91 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6346,6 +6346,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 @@ -6371,6 +6372,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 @@ -6439,6 +6441,7 @@ __metadata: uuid: ^8.2.0 winston: ^3.2.1 yn: ^4.0.0 + zod: ^3.11.6 languageName: unknown linkType: soft @@ -40189,6 +40192,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"