From 27a103ca07bdebaa34b6165d4bd2345178bfac96 Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 14 Feb 2023 10:23:58 +0000 Subject: [PATCH 01/12] Changed the createPermissionIntegrationRouter API to allow getResources to be optional Signed-off-by: Harry Hogg --- .changeset/clean-planes-join.md | 5 + plugins/permission-node/api-report.md | 7 +- .../createPermissionIntegrationRouter.test.ts | 94 +++++++++++-------- .../createPermissionIntegrationRouter.ts | 20 +++- 4 files changed, 81 insertions(+), 45 deletions(-) create mode 100644 .changeset/clean-planes-join.md diff --git a/.changeset/clean-planes-join.md b/.changeset/clean-planes-join.md new file mode 100644 index 0000000000..2ad009c133 --- /dev/null +++ b/.changeset/clean-planes-join.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-permission-node': patch +--- + +Changed the createPermissionIntegrationRouter API to allow getResources to be optional diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 3f712fe362..3019812749 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -124,7 +124,7 @@ export const createPermissionIntegrationRouter: < NoInfer, PermissionRuleParams >[]; - getResources: (resourceRefs: string[]) => Promise<(TResource | undefined)[]>; + getResources?: GetResourcesFn | undefined; }) => express.Router; // @public @@ -137,6 +137,11 @@ export const createPermissionRule: < rule: PermissionRule, ) => PermissionRule; +// @public +export type GetResourcesFn = ( + resourceRefs: string[], +) => Promise>; + // @alpha export const isAndCriteria: ( criteria: PermissionCriteria, diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 32d814465f..33b12f4fc2 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -19,18 +19,15 @@ import { createPermission, Permission, } from '@backstage/plugin-permission-common'; -import express, { Express, Router } from 'express'; +import express from 'express'; import request, { Response } from 'supertest'; import { z } from 'zod'; -import { createPermissionIntegrationRouter } from './createPermissionIntegrationRouter'; +import { + createPermissionIntegrationRouter, + GetResourcesFn, +} from './createPermissionIntegrationRouter'; import { createPermissionRule } from './createPermissionRule'; -const mockGetResources: jest.MockedFunction< - Parameters[0]['getResources'] -> = jest.fn(async resourceRefs => - resourceRefs.map(resourceRef => ({ id: resourceRef })), -); - const testPermission: Permission = createPermission({ name: 'test.permission', attributes: {}, @@ -56,29 +53,30 @@ const testRule2 = createPermissionRule({ toQuery: () => ({}), }); -describe('createPermissionIntegrationRouter', () => { - let app: Express; - let router: Router; +const defaultMockedGetResources: GetResourcesFn<{ id: string }> = jest.fn( + async resourceRefs => resourceRefs.map(resourceRef => ({ id: resourceRef })), +); - beforeAll(() => { - router = createPermissionIntegrationRouter({ - resourceType: 'test-resource', - permissions: [testPermission], - getResources: mockGetResources, - rules: [testRule1, testRule2], - }); - - app = express().use(router); +const createApp = ( + mockedGetResources: + | typeof defaultMockedGetResources + | null = defaultMockedGetResources, +) => { + const router = createPermissionIntegrationRouter({ + resourceType: 'test-resource', + permissions: [testPermission], + getResources: mockedGetResources || undefined, + rules: [testRule1, testRule2], }); + return express().use(router); +}; + +describe('createPermissionIntegrationRouter', () => { afterEach(() => { jest.clearAllMocks(); }); - it('works', async () => { - expect(router).toBeDefined(); - }); - describe('POST /.well-known/backstage/permissions/apply-conditions', () => { it.each([ { @@ -150,7 +148,7 @@ describe('createPermissionIntegrationRouter', () => { ], }, ])('returns 200/ALLOW when criteria match (case %#)', async conditions => { - const response = await request(app) + const response = await request(createApp()) .post('/.well-known/backstage/permissions/apply-conditions') .send({ items: [ @@ -238,7 +236,7 @@ describe('createPermissionIntegrationRouter', () => { ])( 'returns 200/DENY when criteria do not match (case %#)', async conditions => { - const response = await request(app) + const response = await request(createApp()) .post('/.well-known/backstage/permissions/apply-conditions') .send({ items: [ @@ -262,7 +260,7 @@ describe('createPermissionIntegrationRouter', () => { let response: Response; beforeEach(async () => { - response = await request(app) + response = await request(createApp()) .post('/.well-known/backstage/permissions/apply-conditions') .send({ items: [ @@ -353,7 +351,7 @@ describe('createPermissionIntegrationRouter', () => { }); it('calls getResources for all required resources at once', () => { - expect(mockGetResources).toHaveBeenCalledWith([ + expect(defaultMockedGetResources).toHaveBeenCalledWith([ 'default:test/resource-1', 'default:test/resource-2', 'default:test/resource-3', @@ -363,7 +361,7 @@ describe('createPermissionIntegrationRouter', () => { }); it('returns 400 when called with incorrect resource type', async () => { - const response = await request(app) + const response = await request(createApp()) .post('/.well-known/backstage/permissions/apply-conditions') .send({ items: [ @@ -413,11 +411,11 @@ describe('createPermissionIntegrationRouter', () => { }); it('returns 200/DENY when resource is not found', async () => { - mockGetResources.mockImplementationOnce(async resourceRefs => - resourceRefs.map(() => undefined), + const mockedGetResources: GetResourcesFn<{ id: string }> = jest.fn( + async resourceRefs => resourceRefs.map(() => undefined), ); - const response = await request(app) + const response = await request(createApp(mockedGetResources)) .post('/.well-known/backstage/permissions/apply-conditions') .send({ items: [ @@ -446,15 +444,16 @@ describe('createPermissionIntegrationRouter', () => { }); it('interleaves responses for present and missing resources', async () => { - mockGetResources.mockImplementationOnce(async resourceRefs => - resourceRefs.map(resourceRef => - resourceRef === 'default:test/missing-resource' - ? undefined - : { id: resourceRef }, - ), + const mockedGetResources: GetResourcesFn<{ id: string }> = jest.fn( + async resourceRefs => + resourceRefs.map(resourceRef => + resourceRef === 'default:test/missing-resource' + ? undefined + : { id: resourceRef }, + ), ); - const response = await request(app) + const response = await request(createApp(mockedGetResources)) .post('/.well-known/backstage/permissions/apply-conditions') .send({ items: [ @@ -548,18 +547,31 @@ describe('createPermissionIntegrationRouter', () => { ], }, ])(`returns 400 for invalid input %#`, async input => { - const response = await request(app) + const response = await request(createApp()) .post('/.well-known/backstage/permissions/apply-conditions') .send(input); expect(response.status).toEqual(400); expect(response.error && response.error.text).toMatch(/invalid/i); }); + + it('returns 400 with no getResources implementation', async () => { + const response = await request(createApp(null)) + .post('/.well-known/backstage/permissions/apply-conditions') + .send({ + items: [], + }); + + expect(response.status).toEqual(400); + expect(response.body.error.message).toEqual( + 'This plugin does not support the apply-conditions API.', + ); + }); }); describe('GET /.well-known/backstage/permissions/metadata', () => { it('returns a list of permissions and rules used by a given backend', async () => { - const response = await request(app).get( + const response = await request(createApp()).get( '/.well-known/backstage/permissions/metadata', ); diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index 23b59c9c43..49a89f0394 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -125,6 +125,16 @@ export type MetadataResponse = { rules: MetadataResponseSerializedRule[]; }; +/** + * Function type for returning an array of resources + * matching the given resourceRefs. + * + * @public + */ +export type GetResourcesFn = ( + resourceRefs: string[], +) => Promise>; + const applyConditions = ( criteria: PermissionCriteria>, resource: TResource | undefined, @@ -205,9 +215,7 @@ export const createPermissionIntegrationRouter = < // consider any rules whose resource type does not match // to be an error. rules: PermissionRule>[]; - getResources: ( - resourceRefs: string[], - ) => Promise>; + getResources?: GetResourcesFn; }): express.Router => { const { resourceType, permissions, rules, getResources } = options; const router = Router(); @@ -250,6 +258,12 @@ export const createPermissionIntegrationRouter = < router.post( '/.well-known/backstage/permissions/apply-conditions', async (req, res: Response) => { + if (!getResources) { + throw new InputError( + 'This plugin does not support the apply-conditions API.', + ); + } + const parseResult = applyConditionsRequestSchema.safeParse(req.body); if (!parseResult.success) { From 417ae9bb0867e559376a05e04454784f2757954d Mon Sep 17 00:00:00 2001 From: Harry Hogg Date: Tue, 14 Feb 2023 10:35:40 +0000 Subject: [PATCH 02/12] Backticks around changeset words to pass spelling check Signed-off-by: Harry Hogg --- .changeset/clean-planes-join.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/clean-planes-join.md b/.changeset/clean-planes-join.md index 2ad009c133..346f81f1b8 100644 --- a/.changeset/clean-planes-join.md +++ b/.changeset/clean-planes-join.md @@ -2,4 +2,4 @@ '@backstage/plugin-permission-node': patch --- -Changed the createPermissionIntegrationRouter API to allow getResources to be optional +Changed the `createPermissionIntegrationRouter` API to allow `getResources` to be optional From 3bf83a2aabf0ee578f51c12ac549aec6ae1a8aa2 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 14 Feb 2023 17:07:57 +0100 Subject: [PATCH 03/12] errors: add NotImplementedError Co-authored-by: Harry Hogg Signed-off-by: Vincenzo Scamporlino --- .changeset/polite-wombats-smash.md | 6 ++++++ packages/backend-app-api/src/http/MiddlewareFactory.ts | 3 +++ packages/errors/src/errors/common.ts | 7 +++++++ packages/errors/src/errors/index.ts | 1 + 4 files changed, 17 insertions(+) create mode 100644 .changeset/polite-wombats-smash.md diff --git a/.changeset/polite-wombats-smash.md b/.changeset/polite-wombats-smash.md new file mode 100644 index 0000000000..250ca215f5 --- /dev/null +++ b/.changeset/polite-wombats-smash.md @@ -0,0 +1,6 @@ +--- +'@backstage/backend-app-api': patch +'@backstage/errors': patch +--- + +Add NotImplementedError diff --git a/packages/backend-app-api/src/http/MiddlewareFactory.ts b/packages/backend-app-api/src/http/MiddlewareFactory.ts index dc9fe9aa5f..833bdba540 100644 --- a/packages/backend-app-api/src/http/MiddlewareFactory.ts +++ b/packages/backend-app-api/src/http/MiddlewareFactory.ts @@ -38,6 +38,7 @@ import { NotModifiedError, serializeError, } from '@backstage/errors'; +import { NotImplementedError } from '@backstage/errors'; /** * Options used to create a {@link MiddlewareFactory}. @@ -257,6 +258,8 @@ function getStatusCode(error: Error): number { return 404; case ConflictError.name: return 409; + case NotImplementedError.name: + return 501; default: break; } diff --git a/packages/errors/src/errors/common.ts b/packages/errors/src/errors/common.ts index 80a3d84d82..e1a292d7b5 100644 --- a/packages/errors/src/errors/common.ts +++ b/packages/errors/src/errors/common.ts @@ -74,6 +74,13 @@ export class ConflictError extends CustomErrorBase {} */ export class NotModifiedError extends CustomErrorBase {} +/** + * The server does not support the functionality required to fulfill the request. + * + * @public + */ +export class NotImplementedError extends CustomErrorBase {} + /** * An error that forwards an underlying cause with additional context in the message. * diff --git a/packages/errors/src/errors/index.ts b/packages/errors/src/errors/index.ts index 9e1c2408af..efd3257fbf 100644 --- a/packages/errors/src/errors/index.ts +++ b/packages/errors/src/errors/index.ts @@ -24,6 +24,7 @@ export { NotAllowedError, NotFoundError, NotModifiedError, + NotImplementedError, } from './common'; export { CustomErrorBase } from './CustomErrorBase'; export { ResponseError } from './ResponseError'; From 85194da56cc6fee618a0b37f63a11e49777c80e1 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 14 Feb 2023 17:12:28 +0100 Subject: [PATCH 04/12] permission-node: make resources and rules optional in createPermissionIntegrationRouter Co-authored-by: Harry Hogg Signed-off-by: Vincenzo Scamporlino --- .../createPermissionIntegrationRouter.test.ts | 22 +++-- .../createPermissionIntegrationRouter.ts | 86 +++++++++++++------ 2 files changed, 75 insertions(+), 33 deletions(-) diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 33b12f4fc2..af764b01e2 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -62,12 +62,16 @@ const createApp = ( | typeof defaultMockedGetResources | null = defaultMockedGetResources, ) => { - const router = createPermissionIntegrationRouter({ - resourceType: 'test-resource', - permissions: [testPermission], - getResources: mockedGetResources || undefined, - rules: [testRule1, testRule2], - }); + const router = createPermissionIntegrationRouter( + mockedGetResources + ? { + resourceType: 'test-resource', + permissions: [testPermission], + getResources: mockedGetResources, + rules: [testRule1, testRule2], + } + : { permissions: [testPermission] }, + ); return express().use(router); }; @@ -555,16 +559,16 @@ describe('createPermissionIntegrationRouter', () => { expect(response.error && response.error.text).toMatch(/invalid/i); }); - it('returns 400 with no getResources implementation', async () => { + it('returns 501 with no getResources implementation', async () => { const response = await request(createApp(null)) .post('/.well-known/backstage/permissions/apply-conditions') .send({ items: [], }); - expect(response.status).toEqual(400); + expect(response.status).toEqual(501); expect(response.body.error.message).toEqual( - 'This plugin does not support the apply-conditions API.', + 'This plugin does not support the apply-conditions API', ); }); }); diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index 49a89f0394..d35bf06b24 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -36,6 +36,7 @@ import { isNotCriteria, isOrCriteria, } from './util'; +import { NotImplementedError } from '@backstage/errors'; const permissionCriteriaSchema: z.ZodSchema< PermissionCriteria @@ -204,10 +205,11 @@ const applyConditions = ( * * @public */ -export const createPermissionIntegrationRouter = < + +type CreatePermissionIntegrationRouterResourceOptions< TResourceType extends string, TResource, ->(options: { +> = { resourceType: TResourceType; permissions?: Array; // Do not infer value of TResourceType from supplied rules. @@ -215,12 +217,28 @@ export const createPermissionIntegrationRouter = < // consider any rules whose resource type does not match // to be an error. rules: PermissionRule>[]; - getResources?: GetResourcesFn; -}): express.Router => { - const { resourceType, permissions, rules, getResources } = options; + getResources: GetResourcesFn; +}; + +type CreatePermissionIntegrationRouterOptions< + TResourceType extends string, + TResource, +> = + | { + permissions: Array; + } + | CreatePermissionIntegrationRouterResourceOptions; + +export const createPermissionIntegrationRouter = < + TResourceType extends string, + TResource, +>( + options: CreatePermissionIntegrationRouterOptions, +): express.Router => { const router = Router(); router.use(express.json()); + const { permissions = [], rules = [] } = { rules: [], ...options }; router.get('/.well-known/backstage/permissions/metadata', (_, res) => { const serializedRules: MetadataResponseSerializedRule[] = rules.map( rule => ({ @@ -239,30 +257,31 @@ export const createPermissionIntegrationRouter = < return res.json(responseJson); }); - const getRule = createGetRule(rules); - - const assertValidResourceTypes = ( - requests: ApplyConditionsRequestEntry[], - ) => { - const invalidResourceTypes = requests - .filter(request => request.resourceType !== resourceType) - .map(request => request.resourceType); - - if (invalidResourceTypes.length) { - throw new InputError( - `Unexpected resource types: ${invalidResourceTypes.join(', ')}.`, - ); - } - }; - router.post( '/.well-known/backstage/permissions/apply-conditions', async (req, res: Response) => { - if (!getResources) { - throw new InputError( - 'This plugin does not support the apply-conditions API.', + if (!isCreatePermissionIntegrationRouterResourceOptions(options)) { + throw new NotImplementedError( + 'This plugin does not support the apply-conditions API', ); } + const { resourceType, getResources } = options; + + const getRule = createGetRule(rules); + + const assertValidResourceTypes = ( + requests: ApplyConditionsRequestEntry[], + ) => { + const invalidResourceTypes = requests + .filter(request => request.resourceType !== resourceType) + .map(request => request.resourceType); + + if (invalidResourceTypes.length) { + throw new InputError( + `Unexpected resource types: ${invalidResourceTypes.join(', ')}.`, + ); + } + }; const parseResult = applyConditionsRequestSchema.safeParse(req.body); @@ -303,3 +322,22 @@ export const createPermissionIntegrationRouter = < return router; }; + +function isCreatePermissionIntegrationRouterResourceOptions< + TResourceType extends string, + TResource, +>( + options: CreatePermissionIntegrationRouterOptions, +): options is CreatePermissionIntegrationRouterResourceOptions< + TResourceType, + TResource +> { + return ( + ( + options as CreatePermissionIntegrationRouterResourceOptions< + TResourceType, + TResource + > + ).resourceType !== undefined + ); +} From afdb225025e3375ef67f83a4e7c6c65587926802 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 14 Feb 2023 17:15:41 +0100 Subject: [PATCH 05/12] Improve changeset Co-authored-by: Harry Hogg Signed-off-by: Vincenzo Scamporlino --- .changeset/clean-planes-join.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/clean-planes-join.md b/.changeset/clean-planes-join.md index 346f81f1b8..dd8280f553 100644 --- a/.changeset/clean-planes-join.md +++ b/.changeset/clean-planes-join.md @@ -2,4 +2,4 @@ '@backstage/plugin-permission-node': patch --- -Changed the `createPermissionIntegrationRouter` API to allow `getResources` to be optional +Changed the `createPermissionIntegrationRouter` API to allow `getResources`, `resourceType` and `rules` to be optional From dbf36da3eb96c686a6aab23391e4cf516bcae0ae Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 14 Feb 2023 17:22:17 +0100 Subject: [PATCH 06/12] permission-node: improve api report for createPermissionIntegrationRouter Co-authored-by: Harry Hogg Signed-off-by: Vincenzo Scamporlino --- plugins/permission-node/api-report.md | 12 ++-- .../createPermissionIntegrationRouter.ts | 58 +++++++++++-------- 2 files changed, 40 insertions(+), 30 deletions(-) diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 3019812749..b8d337819c 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -111,6 +111,11 @@ export const createConditionTransformer: < permissionRules: [...TRules], ) => ConditionTransformer; +// @public +export const createIsAuthorized: ( + rules: PermissionRule[], +) => (decision: PolicyDecision, resource: TResource | undefined) => boolean; + // @public export const createPermissionIntegrationRouter: < TResourceType extends string, @@ -124,7 +129,7 @@ export const createPermissionIntegrationRouter: < NoInfer, PermissionRuleParams >[]; - getResources?: GetResourcesFn | undefined; + getResources: (resourceRefs: string[]) => Promise<(TResource | undefined)[]>; }) => express.Router; // @public @@ -137,11 +142,6 @@ export const createPermissionRule: < rule: PermissionRule, ) => PermissionRule; -// @public -export type GetResourcesFn = ( - resourceRefs: string[], -) => Promise>; - // @alpha export const isAndCriteria: ( criteria: PermissionCriteria, diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index d35bf06b24..c8bd94f68e 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -170,6 +170,40 @@ const applyConditions = ( return rule.apply(resource, criteria.params ?? {}); }; +/** + * Options for creating a permission integration router specific + * for a particular resource type. + * + * @public + */ +export type CreatePermissionIntegrationRouterResourceOptions< + TResourceType extends string, + TResource, +> = { + resourceType: TResourceType; + permissions?: Array; + // Do not infer value of TResourceType from supplied rules. + // instead only consider the resourceType parameter, and + // consider any rules whose resource type does not match + // to be an error. + rules: PermissionRule>[]; + getResources: GetResourcesFn; +}; + +/** + * Options for creating a permission integration router. + * + * @public + */ +export type CreatePermissionIntegrationRouterOptions< + TResourceType extends string, + TResource, +> = + | { + permissions: Array; + } + | CreatePermissionIntegrationRouterResourceOptions; + /** * Create an express Router which provides an authorization route to allow * integration between the permission backend and other Backstage backend @@ -205,30 +239,6 @@ const applyConditions = ( * * @public */ - -type CreatePermissionIntegrationRouterResourceOptions< - TResourceType extends string, - TResource, -> = { - resourceType: TResourceType; - permissions?: Array; - // Do not infer value of TResourceType from supplied rules. - // instead only consider the resourceType parameter, and - // consider any rules whose resource type does not match - // to be an error. - rules: PermissionRule>[]; - getResources: GetResourcesFn; -}; - -type CreatePermissionIntegrationRouterOptions< - TResourceType extends string, - TResource, -> = - | { - permissions: Array; - } - | CreatePermissionIntegrationRouterResourceOptions; - export const createPermissionIntegrationRouter = < TResourceType extends string, TResource, From 5632097f9258eb268d6387dfb2ff587826b87a48 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Fri, 17 Feb 2023 10:50:38 +0100 Subject: [PATCH 07/12] permission-node: make getResources optional Signed-off-by: Vincenzo Scamporlino --- .../src/integration/createPermissionIntegrationRouter.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index c8bd94f68e..f78fea6ea6 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -187,7 +187,7 @@ export type CreatePermissionIntegrationRouterResourceOptions< // consider any rules whose resource type does not match // to be an error. rules: PermissionRule>[]; - getResources: GetResourcesFn; + getResources?: GetResourcesFn; }; /** @@ -270,9 +270,12 @@ export const createPermissionIntegrationRouter = < router.post( '/.well-known/backstage/permissions/apply-conditions', async (req, res: Response) => { - if (!isCreatePermissionIntegrationRouterResourceOptions(options)) { + if ( + !isCreatePermissionIntegrationRouterResourceOptions(options) || + options.getResources === undefined + ) { throw new NotImplementedError( - 'This plugin does not support the apply-conditions API', + `This plugin does not expose any permission rule or can't evaluate conditional decisions`, ); } const { resourceType, getResources } = options; From d7ef962073699be1182c20ff270cf4f9b3d1e38d Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Fri, 17 Feb 2023 10:51:02 +0100 Subject: [PATCH 08/12] api reports Signed-off-by: Vincenzo Scamporlino --- packages/errors/api-report.md | 3 ++ plugins/permission-node/api-report.md | 43 +++++++++++++++++---------- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/packages/errors/api-report.md b/packages/errors/api-report.md index 1a65ae2315..650a826a30 100644 --- a/packages/errors/api-report.md +++ b/packages/errors/api-report.md @@ -84,6 +84,9 @@ export class NotAllowedError extends CustomErrorBase {} // @public export class NotFoundError extends CustomErrorBase {} +// @public +export class NotImplementedError extends CustomErrorBase {} + // @public export class NotModifiedError extends CustomErrorBase {} diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index b8d337819c..9fda5cfd96 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -111,26 +111,34 @@ export const createConditionTransformer: < permissionRules: [...TRules], ) => ConditionTransformer; -// @public -export const createIsAuthorized: ( - rules: PermissionRule[], -) => (decision: PolicyDecision, resource: TResource | undefined) => boolean; - // @public export const createPermissionIntegrationRouter: < TResourceType extends string, TResource, ->(options: { +>( + options: CreatePermissionIntegrationRouterOptions, +) => express.Router; + +// @public +export type CreatePermissionIntegrationRouterOptions< + TResourceType extends string, + TResource, +> = + | { + permissions: Array; + } + | CreatePermissionIntegrationRouterResourceOptions; + +// @public +export type CreatePermissionIntegrationRouterResourceOptions< + TResourceType extends string, + TResource, +> = { resourceType: TResourceType; - permissions?: Permission[] | undefined; - rules: PermissionRule< - TResource, - any, - NoInfer, - PermissionRuleParams - >[]; - getResources: (resourceRefs: string[]) => Promise<(TResource | undefined)[]>; -}) => express.Router; + permissions?: Array; + rules: PermissionRule>[]; + getResources?: GetResourcesFn; +}; // @public export const createPermissionRule: < @@ -142,6 +150,11 @@ export const createPermissionRule: < rule: PermissionRule, ) => PermissionRule; +// @public +export type GetResourcesFn = ( + resourceRefs: string[], +) => Promise>; + // @alpha export const isAndCriteria: ( criteria: PermissionCriteria, From 36e90ecdf1dd81b7142ff4a9571bc0c69ab1437f Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Fri, 17 Feb 2023 13:39:03 +0100 Subject: [PATCH 09/12] permission-node: fix error message Signed-off-by: Vincenzo Scamporlino --- .../src/integration/createPermissionIntegrationRouter.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index af764b01e2..577267b161 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -568,7 +568,7 @@ describe('createPermissionIntegrationRouter', () => { expect(response.status).toEqual(501); expect(response.body.error.message).toEqual( - 'This plugin does not support the apply-conditions API', + `This plugin does not expose any permission rule or can't evaluate conditional decisions`, ); }); }); From 915e46622cf4bb809ad81011e05af3ba6af7f8b8 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 27 Feb 2023 11:24:32 +0100 Subject: [PATCH 10/12] split changesets Signed-off-by: Vincenzo Scamporlino --- .changeset/polite-wombats-smash.md | 3 +-- .changeset/what-is-going-on-babe.md | 5 +++++ 2 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 .changeset/what-is-going-on-babe.md diff --git a/.changeset/polite-wombats-smash.md b/.changeset/polite-wombats-smash.md index 250ca215f5..98569e1799 100644 --- a/.changeset/polite-wombats-smash.md +++ b/.changeset/polite-wombats-smash.md @@ -1,6 +1,5 @@ --- -'@backstage/backend-app-api': patch '@backstage/errors': patch --- -Add NotImplementedError +Added `NotImplementedError`, which can be used when the server does not recognize the request method and is incapable of supporting it for any resource. diff --git a/.changeset/what-is-going-on-babe.md b/.changeset/what-is-going-on-babe.md new file mode 100644 index 0000000000..c22d69d1de --- /dev/null +++ b/.changeset/what-is-going-on-babe.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-app-api': patch +--- + +Add support for `NotImplementedError`, properly returning 501 as status code. From e837143bc9a3d9a112ab6df5ac10db8ef6d0d711 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 27 Feb 2023 11:30:05 +0100 Subject: [PATCH 11/12] permission-node: simplify api report Signed-off-by: Vincenzo Scamporlino --- plugins/permission-node/api-report.md | 9 ++---- .../createPermissionIntegrationRouter.test.ts | 32 ++++++++++++------- .../createPermissionIntegrationRouter.ts | 14 ++------ 3 files changed, 26 insertions(+), 29 deletions(-) diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 9fda5cfd96..5d2966ec73 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -137,7 +137,9 @@ export type CreatePermissionIntegrationRouterResourceOptions< resourceType: TResourceType; permissions?: Array; rules: PermissionRule>[]; - getResources?: GetResourcesFn; + getResources?: ( + resourceRefs: string[], + ) => Promise>; }; // @public @@ -150,11 +152,6 @@ export const createPermissionRule: < rule: PermissionRule, ) => PermissionRule; -// @public -export type GetResourcesFn = ( - resourceRefs: string[], -) => Promise>; - // @alpha export const isAndCriteria: ( criteria: PermissionCriteria, diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 577267b161..8b918cabb2 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -24,7 +24,7 @@ import request, { Response } from 'supertest'; import { z } from 'zod'; import { createPermissionIntegrationRouter, - GetResourcesFn, + CreatePermissionIntegrationRouterResourceOptions, } from './createPermissionIntegrationRouter'; import { createPermissionRule } from './createPermissionRule'; @@ -53,8 +53,11 @@ const testRule2 = createPermissionRule({ toQuery: () => ({}), }); -const defaultMockedGetResources: GetResourcesFn<{ id: string }> = jest.fn( - async resourceRefs => resourceRefs.map(resourceRef => ({ id: resourceRef })), +const defaultMockedGetResources: CreatePermissionIntegrationRouterResourceOptions< + string, + { id: string } +>['getResources'] = jest.fn(async resourceRefs => + resourceRefs.map(resourceRef => ({ id: resourceRef })), ); const createApp = ( @@ -415,8 +418,11 @@ describe('createPermissionIntegrationRouter', () => { }); it('returns 200/DENY when resource is not found', async () => { - const mockedGetResources: GetResourcesFn<{ id: string }> = jest.fn( - async resourceRefs => resourceRefs.map(() => undefined), + const mockedGetResources: CreatePermissionIntegrationRouterResourceOptions< + string, + { id: string } + >['getResources'] = jest.fn(async resourceRefs => + resourceRefs.map(() => undefined), ); const response = await request(createApp(mockedGetResources)) @@ -448,13 +454,15 @@ describe('createPermissionIntegrationRouter', () => { }); it('interleaves responses for present and missing resources', async () => { - const mockedGetResources: GetResourcesFn<{ id: string }> = jest.fn( - async resourceRefs => - resourceRefs.map(resourceRef => - resourceRef === 'default:test/missing-resource' - ? undefined - : { id: resourceRef }, - ), + const mockedGetResources: CreatePermissionIntegrationRouterResourceOptions< + string, + { id: string } + >['getResources'] = jest.fn(async resourceRefs => + resourceRefs.map(resourceRef => + resourceRef === 'default:test/missing-resource' + ? undefined + : { id: resourceRef }, + ), ); const response = await request(createApp(mockedGetResources)) diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index f78fea6ea6..715b1ec574 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -126,16 +126,6 @@ export type MetadataResponse = { rules: MetadataResponseSerializedRule[]; }; -/** - * Function type for returning an array of resources - * matching the given resourceRefs. - * - * @public - */ -export type GetResourcesFn = ( - resourceRefs: string[], -) => Promise>; - const applyConditions = ( criteria: PermissionCriteria>, resource: TResource | undefined, @@ -187,7 +177,9 @@ export type CreatePermissionIntegrationRouterResourceOptions< // consider any rules whose resource type does not match // to be an error. rules: PermissionRule>[]; - getResources?: GetResourcesFn; + getResources?: ( + resourceRefs: string[], + ) => Promise>; }; /** From 4c0ba1cfc77df577e4fff9fc0de4773f7462a27f Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 27 Feb 2023 15:20:08 +0100 Subject: [PATCH 12/12] permission-node: improve createPermissionIntegrationRouter docs Signed-off-by: Vincenzo Scamporlino --- plugins/permission-node/api-report.md | 20 +++---- .../createPermissionIntegrationRouter.test.ts | 18 +++--- .../createPermissionIntegrationRouter.ts | 60 +++++++++++++------ 3 files changed, 58 insertions(+), 40 deletions(-) diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 5d2966ec73..52e666996a 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -112,22 +112,20 @@ export const createConditionTransformer: < ) => ConditionTransformer; // @public -export const createPermissionIntegrationRouter: < +export function createPermissionIntegrationRouter< TResourceType extends string, TResource, >( - options: CreatePermissionIntegrationRouterOptions, -) => express.Router; + options: CreatePermissionIntegrationRouterResourceOptions< + TResourceType, + TResource + >, +): express.Router; // @public -export type CreatePermissionIntegrationRouterOptions< - TResourceType extends string, - TResource, -> = - | { - permissions: Array; - } - | CreatePermissionIntegrationRouterResourceOptions; +export function createPermissionIntegrationRouter(options: { + permissions: Array; +}): express.Router; // @public export type CreatePermissionIntegrationRouterResourceOptions< diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 8b918cabb2..1a7462665e 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -65,16 +65,14 @@ const createApp = ( | typeof defaultMockedGetResources | null = defaultMockedGetResources, ) => { - const router = createPermissionIntegrationRouter( - mockedGetResources - ? { - resourceType: 'test-resource', - permissions: [testPermission], - getResources: mockedGetResources, - rules: [testRule1, testRule2], - } - : { permissions: [testPermission] }, - ); + const router = mockedGetResources + ? createPermissionIntegrationRouter({ + resourceType: 'test-resource', + permissions: [testPermission], + getResources: mockedGetResources, + rules: [testRule1, testRule2], + }) + : createPermissionIntegrationRouter({ permissions: [testPermission] }); return express().use(router); }; diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index 715b1ec574..087de23198 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -182,20 +182,6 @@ export type CreatePermissionIntegrationRouterResourceOptions< ) => Promise>; }; -/** - * Options for creating a permission integration router. - * - * @public - */ -export type CreatePermissionIntegrationRouterOptions< - TResourceType extends string, - TResource, -> = - | { - permissions: Array; - } - | CreatePermissionIntegrationRouterResourceOptions; - /** * Create an express Router which provides an authorization route to allow * integration between the permission backend and other Backstage backend @@ -203,6 +189,9 @@ export type CreatePermissionIntegrationRouterOptions< * their resources should add the router created by this function to their * express app inside their `createRouter` implementation. * + * In case the `permissions` option is provided, the router also + * provides a route that exposes permissions and routes of a plugin. + * * @remarks * * To make this concrete, we can use the Backstage software catalog as an @@ -231,12 +220,40 @@ export type CreatePermissionIntegrationRouterOptions< * * @public */ -export const createPermissionIntegrationRouter = < +export function createPermissionIntegrationRouter< TResourceType extends string, TResource, >( - options: CreatePermissionIntegrationRouterOptions, -): express.Router => { + options: CreatePermissionIntegrationRouterResourceOptions< + TResourceType, + TResource + >, +): express.Router; + +/** + * + * Create an express Router which provides a route that exposes + * permissions and routes of a plugin. + * @public + */ +export function createPermissionIntegrationRouter(options: { + permissions: Array; +}): express.Router; + +/** + * @public + */ +export function createPermissionIntegrationRouter< + TResourceType extends string, + TResource, +>( + options: + | { permissions: Array } + | CreatePermissionIntegrationRouterResourceOptions< + TResourceType, + TResource + >, +): express.Router { const router = Router(); router.use(express.json()); @@ -326,13 +343,18 @@ export const createPermissionIntegrationRouter = < router.use(errorHandler()); return router; -}; +} function isCreatePermissionIntegrationRouterResourceOptions< TResourceType extends string, TResource, >( - options: CreatePermissionIntegrationRouterOptions, + options: + | { permissions: Array } + | CreatePermissionIntegrationRouterResourceOptions< + TResourceType, + TResource + >, ): options is CreatePermissionIntegrationRouterResourceOptions< TResourceType, TResource