From 1e60bfffa1313cfd9b0b713a0093d419ee727963 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 9 Apr 2025 22:24:18 +0200 Subject: [PATCH 01/13] permission-common: batching of permissions Signed-off-by: Vincenzo Scamporlino --- plugins/permission-common/config.d.ts | 5 + .../src/PermissionClient.test.ts | 282 +++++++++++++++++- .../permission-common/src/PermissionClient.ts | 77 ++++- plugins/permission-common/src/types/api.ts | 1 + 4 files changed, 358 insertions(+), 7 deletions(-) diff --git a/plugins/permission-common/config.d.ts b/plugins/permission-common/config.d.ts index 2378fa4a71..d43fc96586 100644 --- a/plugins/permission-common/config.d.ts +++ b/plugins/permission-common/config.d.ts @@ -23,5 +23,10 @@ export interface Config { * @visibility frontend */ enabled?: boolean; + + /** + * @visibility frontend + */ + EXPERIMENTAL_enableBatchedRequests?: boolean; }; } diff --git a/plugins/permission-common/src/PermissionClient.test.ts b/plugins/permission-common/src/PermissionClient.test.ts index 72707bf98a..b32c5f148e 100644 --- a/plugins/permission-common/src/PermissionClient.test.ts +++ b/plugins/permission-common/src/PermissionClient.test.ts @@ -17,7 +17,10 @@ import { RestContext, rest } from 'msw'; import { setupServer } from 'msw/node'; import { ConfigReader } from '@backstage/config'; -import { PermissionClient } from './PermissionClient'; +import { + BatchedAuthorizePermissionRequest, + PermissionClient, +} from './PermissionClient'; import { EvaluatePermissionRequest, AuthorizeResult, @@ -36,10 +39,7 @@ const discovery: DiscoveryApi = { return mockBaseUrl; }, }; -const client: PermissionClient = new PermissionClient({ - discovery, - config: new ConfigReader({ permission: { enabled: true } }), -}); +let client: PermissionClient; const mockPermission = createPermission({ name: 'test.permission', @@ -53,6 +53,13 @@ describe('PermissionClient', () => { afterEach(() => server.resetHandlers()); describe('authorize', () => { + beforeAll(() => { + client = new PermissionClient({ + discovery, + config: new ConfigReader({ permission: { enabled: true } }), + }); + }); + const mockAuthorizeConditional = { permission: mockPermission, resourceRef: 'foo:bar', @@ -211,7 +218,270 @@ describe('PermissionClient', () => { }); }); + describe('authorize (batched)', () => { + beforeAll(() => { + client = new PermissionClient({ + discovery, + config: new ConfigReader({ + permission: { + enabled: true, + EXPERIMENTAL_enableBatchedRequests: true, + }, + }), + }); + }); + + const mockAuthorizeConditional = { + permission: mockPermission, + resourceRef: 'foo:bar', + }; + + const mockAuthorizeHandler = jest.fn(); + + beforeEach(() => { + mockAuthorizeHandler.mockReset(); + server.use(rest.post(`${mockBaseUrl}/authorize`, mockAuthorizeHandler)); + + mockAuthorizeHandler.mockImplementation( + (req, res, { json }: RestContext) => { + const responses = req.body.items.map( + (a: BatchedAuthorizePermissionRequest) => ({ + id: a.id, + result: [AuthorizeResult.ALLOW], + }), + ); + + return res(json({ items: responses })); + }, + ); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should fetch entities from correct endpoint', async () => { + await client.authorize([mockAuthorizeConditional]); + expect(mockAuthorizeHandler).toHaveBeenCalled(); + }); + + it('should include a request body', async () => { + await client.authorize([mockAuthorizeConditional]); + + const request = mockAuthorizeHandler.mock.calls[0][0]; + + expect(request.body).toEqual({ + items: [ + expect.objectContaining({ + permission: mockPermission, + resourceRefs: ['foo:bar'], + }), + ], + }); + }); + + it('should return the response from the fetch request', async () => { + const response = await client.authorize([mockAuthorizeConditional]); + expect(response[0]).toEqual( + expect.objectContaining({ result: AuthorizeResult.ALLOW }), + ); + }); + + it('should not include authorization headers if no token is supplied', async () => { + await client.authorize([mockAuthorizeConditional]); + + const request = mockAuthorizeHandler.mock.calls[0][0]; + expect(request.headers.has('authorization')).toEqual(false); + }); + + it('should include correctly-constructed authorization header if token is supplied', async () => { + await client.authorize([mockAuthorizeConditional], { token }); + + const request = mockAuthorizeHandler.mock.calls[0][0]; + expect(request.headers.get('authorization')).toEqual('Bearer fake-token'); + }); + + it('should forward response errors', async () => { + mockAuthorizeHandler.mockImplementationOnce( + (_req, res, { status }: RestContext) => { + return res(status(401)); + }, + ); + await expect( + client.authorize([mockAuthorizeConditional], { token }), + ).rejects.toThrow(/request failed with 401/i); + }); + + it('should reject responses with missing ids', async () => { + mockAuthorizeHandler.mockImplementationOnce( + (_req, res, { json }: RestContext) => { + return res( + json({ + items: [{ id: 'wrong-id', result: [AuthorizeResult.ALLOW] }], + }), + ); + }, + ); + await expect( + client.authorize([mockAuthorizeConditional], { token }), + ).rejects.toThrow(/items in response do not match request/i); + }); + + it('should reject invalid responses', async () => { + mockAuthorizeHandler.mockImplementationOnce( + (req, res, { json }: RestContext) => { + const responses = req.body.items.map( + (a: IdentifiedPermissionMessage) => ({ + id: a.id, + outcome: AuthorizeResult.ALLOW, + }), + ); + + return res(json({ items: responses })); + }, + ); + await expect( + client.authorize([mockAuthorizeConditional], { token }), + ).rejects.toThrow(/invalid_type/i); + }); + + it('should allow all when permission.enabled is false', async () => { + const disabled = new PermissionClient({ + discovery, + config: new ConfigReader({ permission: { enabled: false } }), + }); + const response = await disabled.authorize([mockAuthorizeConditional]); + expect(response[0]).toEqual( + expect.objectContaining({ result: AuthorizeResult.ALLOW }), + ); + expect(mockAuthorizeHandler).not.toHaveBeenCalled(); + }); + + it('should allow all when permission.enabled is not configured', async () => { + const disabled = new PermissionClient({ + discovery, + config: new ConfigReader({}), + }); + const response = await disabled.authorize([mockAuthorizeConditional]); + expect(response[0]).toEqual( + expect.objectContaining({ result: AuthorizeResult.ALLOW }), + ); + expect(mockAuthorizeHandler).not.toHaveBeenCalled(); + }); + + it('should properly map the permissions', async () => { + const mockPermission2 = createPermission({ + name: 'test.permission2', + attributes: {}, + resourceType: 'foo', + }); + + const mockPermission3 = createPermission({ + name: 'test.permission3', + attributes: {}, + }); + + mockAuthorizeHandler.mockImplementationOnce( + (req, res, { json }: RestContext) => { + return res( + json({ + items: [ + { + id: req.body.items[0].id, + result: [AuthorizeResult.ALLOW, AuthorizeResult.DENY], + }, + { + id: req.body.items[1].id, + result: [AuthorizeResult.DENY], + }, + { + id: req.body.items[2].id, + result: [AuthorizeResult.DENY, AuthorizeResult.ALLOW], + }, + ], + }), + ); + }, + ); + + const response = await client.authorize([ + { + permission: mockPermission, + resourceRef: 'foo:bar', // allow + }, + { + permission: mockPermission3, // deny + }, + { + permission: mockPermission, + resourceRef: 'foo:car', // deny + }, + { + permission: mockPermission3, // deny + }, + { + permission: mockPermission2, + resourceRef: 'r2', // deny + }, + { + permission: mockPermission2, + resourceRef: 'r1', // allow + }, + ]); + + expect(mockAuthorizeHandler.mock.calls[0][0].body).toEqual({ + items: [ + { + permission: { + type: 'resource', + name: 'test.permission', + attributes: {}, + resourceType: 'foo', + }, + resourceRefs: ['foo:bar', 'foo:car'], + id: expect.any(String), + }, + { + permission: { + type: 'basic', + name: 'test.permission3', + attributes: {}, + }, + resourceRefs: [], + id: expect.any(String), + }, + { + permission: { + type: 'resource', + name: 'test.permission2', + attributes: {}, + resourceType: 'foo', + }, + resourceRefs: ['r2', 'r1'], + id: expect.any(String), + }, + ], + }); + + expect(response).toEqual([ + { result: 'ALLOW' }, + { result: 'DENY' }, + { result: 'DENY' }, + { result: 'DENY' }, + { result: 'DENY' }, + { result: 'ALLOW' }, + ]); + }); + }); + describe('authorizeConditional', () => { + beforeAll(() => { + client = new PermissionClient({ + discovery, + config: new ConfigReader({ permission: { enabled: true } }), + }); + }); + const mockResourceAuthorizeConditional = { permission: mockPermission, }; @@ -420,7 +690,7 @@ describe('PermissionClient', () => { }), ); - return res(json(responses)); + return res(json({ items: responses })); }, ); const disabled = new PermissionClient({ diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 2414774775..04810ec66f 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -29,9 +29,10 @@ import { AuthorizePermissionRequest, AuthorizePermissionResponse, QueryPermissionResponse, + IdentifiedPermissionMessage, } from './types/api'; import { DiscoveryApi } from './types/discovery'; -import { AuthorizeRequestOptions } from './types/permission'; +import { AuthorizeRequestOptions, Permission } from './types/permission'; const permissionCriteriaSchema: z.ZodSchema< PermissionCriteria @@ -108,11 +109,17 @@ export type PermissionClientRequestOptions = { export class PermissionClient implements PermissionEvaluator { private readonly enabled: boolean; private readonly discovery: DiscoveryApi; + private readonly enableBatchedRequests: boolean; constructor(options: { discovery: DiscoveryApi; config: Config }) { this.discovery = options.discovery; this.enabled = options.config.getOptionalBoolean('permission.enabled') ?? false; + + this.enableBatchedRequests = + options.config.getOptionalBoolean( + 'permission.EXPERIMENTAL_enableBatchedRequests', + ) ?? false; } /** @@ -122,6 +129,10 @@ export class PermissionClient implements PermissionEvaluator { requests: AuthorizePermissionRequest[], options?: PermissionClientRequestOptions, ): Promise { + if (this.enableBatchedRequests) { + return this.makeBatchedRequest(requests, options); + } + return this.makeRequest( requests, authorizePermissionResponseSchema, @@ -183,7 +194,71 @@ export class PermissionClient implements PermissionEvaluator { return request.items.map(query => responsesById[query.id]); } + private async makeBatchedRequest( + queries: AuthorizePermissionRequest[], + options?: AuthorizeRequestOptions, + ) { + if (!this.enabled) { + return queries.map(_ => ({ result: AuthorizeResult.ALLOW as const })); + } + + const request: Record = {}; + + for (const query of queries) { + const { permission, resourceRef } = query; + + request[permission.name] ||= { + permission, + resourceRefs: [], + id: uuid.v4(), + }; + + if (resourceRef) { + request[permission.name].resourceRefs.push(resourceRef); + } + } + + const rawRequest = { items: Object.values(request) }; + const permissionApi = await this.discovery.getBaseUrl('permission'); + const response = await fetch(`${permissionApi}/authorize`, { + method: 'POST', + body: JSON.stringify(rawRequest), + headers: { + ...this.getAuthorizationHeader(options?.token), + 'content-type': 'application/json', + }, + }); + if (!response.ok) { + throw await ResponseError.fromResponse(response); + } + + const responseBody = await response.json(); + + const parsedResponse = responseSchema( + z.object({ + result: z.array( + z.literal(AuthorizeResult.ALLOW).or(z.literal(AuthorizeResult.DENY)), + ), + }), + new Set(rawRequest.items.map(({ id }) => id)), + ).parse(responseBody); + + return queries.map(query => { + const { id } = request[query.permission.name]; + + const item = parsedResponse.items.find(i => i.id === id)!; + return { + result: query.resourceRef ? item.result.shift()! : item.result[0], + }; + }); + } + private getAuthorizationHeader(token?: string): Record { return token ? { Authorization: `Bearer ${token}` } : {}; } } + +export type BatchedAuthorizePermissionRequest = IdentifiedPermissionMessage<{ + permission: Permission; + resourceRefs: string[]; +}>; diff --git a/plugins/permission-common/src/types/api.ts b/plugins/permission-common/src/types/api.ts index 072615989b..f70bd2d890 100644 --- a/plugins/permission-common/src/types/api.ts +++ b/plugins/permission-common/src/types/api.ts @@ -176,6 +176,7 @@ export type EvaluatePermissionRequest = { /** * A batch of requests sent to the permission backend. * @public + * @deprecated This type is not used and it will be removed in the future */ export type EvaluatePermissionRequestBatch = PermissionMessageBatch; From 1ffe31cf694ea3ae507937e47da107fb7cda4e29 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 9 Apr 2025 22:26:14 +0200 Subject: [PATCH 02/13] permission-backend: accept batched permissions Signed-off-by: Vincenzo Scamporlino --- .../permission-backend/src/service/router.ts | 130 +++++++++++------- 1 file changed, 83 insertions(+), 47 deletions(-) diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index 0759f6df3a..f89a83ed51 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -21,13 +21,12 @@ import { InputError } from '@backstage/errors'; import { IdentityApi } from '@backstage/plugin-auth-node'; import { AuthorizeResult, - EvaluatePermissionRequest, - EvaluatePermissionRequestBatch, EvaluatePermissionResponse, - EvaluatePermissionResponseBatch, IdentifiedPermissionMessage, isResourcePermission, PermissionAttributes, + PermissionMessageBatch, + PolicyDecision, } from '@backstage/plugin-permission-common'; import { ApplyConditionsRequestEntry, @@ -74,25 +73,30 @@ const resourcePermissionSchema = z.object({ resourceType: z.string(), }); -const evaluatePermissionRequestSchema: z.ZodSchema< - IdentifiedPermissionMessage -> = z.union([ +const evaluatePermissionRequestSchema = z.union([ z.object({ id: z.string(), resourceRef: z.undefined().optional(), + resourceRefs: z.undefined().optional(), permission: basicPermissionSchema, }), z.object({ id: z.string(), resourceRef: z.string().optional(), + resourceRefs: z.undefined().optional(), + permission: resourcePermissionSchema, + }), + z.object({ + id: z.string(), + resourceRef: z.undefined().optional(), + resourceRefs: z.array(z.string()), permission: resourcePermissionSchema, }), ]); -const evaluatePermissionRequestBatchSchema: z.ZodSchema = - z.object({ - items: z.array(evaluatePermissionRequestSchema), - }); +const evaluatePermissionRequestBatchSchema = z.object({ + items: z.array(evaluatePermissionRequestSchema), +}); /** * Options required when constructing a new {@link express#Router} using @@ -112,7 +116,7 @@ export interface RouterOptions { } const handleRequest = async ( - requests: IdentifiedPermissionMessage[], + requests: z.infer['items'], policy: PermissionPolicy, permissionIntegrationClient: PermissionIntegrationClient, credentials: BackstageCredentials< @@ -120,7 +124,11 @@ const handleRequest = async ( >, auth: AuthService, userInfo: UserInfoService, -): Promise[]> => { +): Promise< + IdentifiedPermissionMessage< + EvaluatePermissionResponse | BulkDefinitivePolicyDecision + >[] +> => { const applyConditionsLoaderFor = memoize((pluginId: string) => { return new DataLoader< ApplyConditionsRequestEntry, @@ -150,40 +158,59 @@ const handleRequest = async ( } return Promise.all( - requests.map(({ id, resourceRef, ...request }) => - policy.handle(request, user).then(decision => { - if (decision.result !== AuthorizeResult.CONDITIONAL) { - return { - id, + requests.map(request => + policy + .handle({ permission: request.permission }, user) + .then(async decision => { + if (decision.result !== AuthorizeResult.CONDITIONAL) { + return { + id: request.id, + ...decision, + }; + } + + if (!isResourcePermission(request.permission)) { + throw new Error( + `Conditional decision returned from permission policy for non-resource permission ${request.permission.name}`, + ); + } + + if (decision.resourceType !== request.permission.resourceType) { + throw new Error( + `Invalid resource conditions returned from permission policy for permission ${request.permission.name}`, + ); + } + + if (request.resourceRefs) { + const results = await Promise.all( + request.resourceRefs.map(resourceRef => + applyConditionsLoaderFor(decision.pluginId).load({ + id: request.id, + resourceRef, + ...decision, + }), + ), + ); + + return { + id: request.id, + result: results.map(({ result }) => result), + }; + } + + if (!request.resourceRef) { + return { + id: request.id, + ...decision, + }; + } + + return applyConditionsLoaderFor(decision.pluginId).load({ + id: request.id, + resourceRef: request.resourceRef, ...decision, - }; - } - - if (!isResourcePermission(request.permission)) { - throw new Error( - `Conditional decision returned from permission policy for non-resource permission ${request.permission.name}`, - ); - } - - if (decision.resourceType !== request.permission.resourceType) { - throw new Error( - `Invalid resource conditions returned from permission policy for permission ${request.permission.name}`, - ); - } - - if (!resourceRef) { - return { - id, - ...decision, - }; - } - - return applyConditionsLoaderFor(decision.pluginId).load({ - id, - resourceRef, - ...decision, - }); - }), + }); + }), ), ); }; @@ -226,8 +253,10 @@ export async function createRouter( router.post( '/authorize', async ( - req: Request, - res: Response, + req: Request, + res: Response< + PermissionMessageBatch + >, ) => { const credentials = await httpAuth.credentials(req, { allow: ['user', 'none'], @@ -274,3 +303,10 @@ export async function createRouter( return router; } + +/** + * @internal + */ +type BulkDefinitivePolicyDecision = { + result: Array; +}; From 9692c85d7ec22a31e82fe4a1b592c83fd36aa71a Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 10 Apr 2025 12:47:06 +0200 Subject: [PATCH 03/13] permission-backend: add tests for bulk resourceRefs Signed-off-by: Vincenzo Scamporlino --- .../src/service/router.test.ts | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/plugins/permission-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index 9c8fa4ad5f..fd5d8f67bd 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -154,6 +154,84 @@ describe('createRouter', () => { }); }); + it('calls the permission policy with batched resourceRefs', async () => { + policy.handle.mockResolvedValueOnce({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'test-plugin', + resourceType: 'test-resource-1', + conditions: { rule: 'test-rule', params: ['abc'] }, + }); + + mockApplyConditions.mockResolvedValueOnce([ + { + id: '123', + result: AuthorizeResult.ALLOW, + }, + { + id: '123', + result: AuthorizeResult.DENY, + }, + ]); + + const response = await request(app) + .post('/authorize') + .send({ + items: [ + { + id: '123', + permission: { + type: 'resource', + name: 'test.permission1', + attributes: {}, + resourceType: 'test-resource-1', + }, + resourceRefs: ['resource:1', 'resource:2'], + }, + { + id: '234', + permission: { + type: 'basic', + name: 'test.permission2', + attributes: {}, + }, + }, + ], + }); + + expect(response.status).toEqual(200); + + expect(policy.handle).toHaveBeenCalledWith( + { + permission: { + type: 'resource', + name: 'test.permission1', + attributes: {}, + resourceType: 'test-resource-1', + }, + }, + undefined, + ); + expect(policy.handle).toHaveBeenCalledWith( + { + permission: { + type: 'basic', + name: 'test.permission2', + attributes: {}, + }, + }, + undefined, + ); + + expect(policy.handle).toHaveBeenCalledTimes(2); + + expect(response.body).toEqual({ + items: [ + { id: '123', result: [AuthorizeResult.ALLOW, AuthorizeResult.DENY] }, + { id: '234', result: AuthorizeResult.DENY }, + ], + }); + }); + it('resolves identity from the Authorization header', async () => { const response = await request(app) .post('/authorize') From a5656fd532b822fe931011be573f3883a31e8cca Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 10 Apr 2025 12:50:09 +0200 Subject: [PATCH 04/13] permission-backend: improve request validation Signed-off-by: Vincenzo Scamporlino --- .../src/service/router.test.ts | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/plugins/permission-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index fd5d8f67bd..68ae5d8ff5 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -881,6 +881,75 @@ describe('createRouter', () => { }, ], }, + { + items: [ + { + id: '123', + resourceRef: ['resource:1'], + permission: { + type: 'resource', + name: 'test.permission', + attributes: {}, + resourceType: 'test-resource-1', + }, + }, + ], + }, + { + items: [ + { + id: '123', + resourceRefs: 'resource:1', + permission: { + type: 'resource', + name: 'test.permission', + attributes: {}, + resourceType: 'test-resource-1', + }, + }, + ], + }, + { + items: [ + { + id: '123', + resourceRefs: ['resource:1'], + resourceRef: 'resource:1', + permission: { + type: 'resource', + name: 'test.permission', + attributes: {}, + resourceType: 'test-resource-1', + }, + }, + ], + }, + { + items: [ + { + id: '123', + resourceRefs: ['resource:1'], + permission: { + type: 'basic', + name: 'test.permission', + attributes: {}, + }, + }, + ], + }, + { + items: [ + { + id: '123', + resourceRef: 'resource:1', + permission: { + type: 'basic', + name: 'test.permission', + attributes: {}, + }, + }, + ], + }, ])('returns a 400 error for invalid request %#', async requestBody => { const response = await request(app).post('/authorize').send(requestBody); From ec42d827c2ba14c9c491746fc9ff74fb30529818 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 10 Apr 2025 16:05:59 +0200 Subject: [PATCH 05/13] permission-common: refactor batched requests Signed-off-by: Vincenzo Scamporlino --- .../permission-common/src/PermissionClient.ts | 99 +++++++++---------- 1 file changed, 49 insertions(+), 50 deletions(-) diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 04810ec66f..83773d4cd0 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -55,6 +55,12 @@ const authorizePermissionResponseSchema: z.ZodSchema = z.union([ z.object({ @@ -129,6 +135,10 @@ export class PermissionClient implements PermissionEvaluator { requests: AuthorizePermissionRequest[], options?: PermissionClientRequestOptions, ): Promise { + if (!this.enabled) { + return requests.map(_ => ({ result: AuthorizeResult.ALLOW as const })); + } + if (this.enableBatchedRequests) { return this.makeBatchedRequest(requests, options); } @@ -147,6 +157,10 @@ export class PermissionClient implements PermissionEvaluator { queries: QueryPermissionRequest[], options?: PermissionClientRequestOptions, ): Promise { + if (!this.enabled) { + return queries.map(_ => ({ result: AuthorizeResult.ALLOW as const })); + } + return this.makeRequest(queries, queryPermissionResponseSchema, options); } @@ -155,10 +169,6 @@ export class PermissionClient implements PermissionEvaluator { itemSchema: z.ZodSchema, options?: AuthorizeRequestOptions, ) { - if (!this.enabled) { - return queries.map(_ => ({ result: AuthorizeResult.ALLOW as const })); - } - const request: PermissionMessageBatch = { items: queries.map(query => ({ id: uuid.v4(), @@ -166,25 +176,11 @@ export class PermissionClient implements PermissionEvaluator { })), }; - const permissionApi = await this.discovery.getBaseUrl('permission'); - const response = await fetch(`${permissionApi}/authorize`, { - method: 'POST', - body: JSON.stringify(request), - headers: { - ...this.getAuthorizationHeader(options?.token), - 'content-type': 'application/json', - }, - }); - if (!response.ok) { - throw await ResponseError.fromResponse(response); - } - - const responseBody = await response.json(); - - const parsedResponse = responseSchema( + const parsedResponse = await this.makeRawRequest( + request, itemSchema, - new Set(request.items.map(({ id }) => id)), - ).parse(responseBody); + options, + ); const responsesById = parsedResponse.items.reduce((acc, r) => { acc[r.id] = r; @@ -198,10 +194,6 @@ export class PermissionClient implements PermissionEvaluator { queries: AuthorizePermissionRequest[], options?: AuthorizeRequestOptions, ) { - if (!this.enabled) { - return queries.map(_ => ({ result: AuthorizeResult.ALLOW as const })); - } - const request: Record = {}; for (const query of queries) { @@ -218,30 +210,11 @@ export class PermissionClient implements PermissionEvaluator { } } - const rawRequest = { items: Object.values(request) }; - const permissionApi = await this.discovery.getBaseUrl('permission'); - const response = await fetch(`${permissionApi}/authorize`, { - method: 'POST', - body: JSON.stringify(rawRequest), - headers: { - ...this.getAuthorizationHeader(options?.token), - 'content-type': 'application/json', - }, - }); - if (!response.ok) { - throw await ResponseError.fromResponse(response); - } - - const responseBody = await response.json(); - - const parsedResponse = responseSchema( - z.object({ - result: z.array( - z.literal(AuthorizeResult.ALLOW).or(z.literal(AuthorizeResult.DENY)), - ), - }), - new Set(rawRequest.items.map(({ id }) => id)), - ).parse(responseBody); + const parsedResponse = await this.makeRawRequest( + { items: Object.values(request) }, + authorizePermissionResponseBatchSchema, + options, + ); return queries.map(query => { const { id } = request[query.permission.name]; @@ -253,6 +226,32 @@ export class PermissionClient implements PermissionEvaluator { }); } + private async makeRawRequest( + request: PermissionMessageBatch, + itemSchema: z.ZodSchema, + options?: AuthorizeRequestOptions, + ) { + const permissionApi = await this.discovery.getBaseUrl('permission'); + const response = await fetch(`${permissionApi}/authorize`, { + method: 'POST', + body: JSON.stringify(request), + headers: { + ...this.getAuthorizationHeader(options?.token), + 'content-type': 'application/json', + }, + }); + if (!response.ok) { + throw await ResponseError.fromResponse(response); + } + + const responseBody = await response.json(); + + return responseSchema( + itemSchema, + new Set(request.items.map(({ id }) => id)), + ).parse(responseBody); + } + private getAuthorizationHeader(token?: string): Record { return token ? { Authorization: `Bearer ${token}` } : {}; } From 4d4ec58d6b99e35faa20734e313ccc3d10fa5735 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Fri, 11 Apr 2025 10:34:21 +0200 Subject: [PATCH 06/13] permission: batch apply conditions payload Signed-off-by: Vincenzo Scamporlino --- .../service/PermissionIntegrationClient.ts | 25 ++--- .../src/service/router.test.ts | 21 +++- .../permission-backend/src/service/router.ts | 34 +++---- .../createPermissionIntegrationRouter.test.ts | 95 +++++++++++++++++++ .../createPermissionIntegrationRouter.ts | 88 ++++++++++++----- 5 files changed, 202 insertions(+), 61 deletions(-) diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.ts index 21d2f66887..610eb8b34b 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.ts @@ -34,9 +34,16 @@ const responseSchema = z.object({ items: z.array( z.object({ id: z.string(), - result: z - .literal(AuthorizeResult.ALLOW) - .or(z.literal(AuthorizeResult.DENY)), + result: z.union([ + z.literal(AuthorizeResult.ALLOW), + z.literal(AuthorizeResult.DENY), + z.array( + z.union([ + z.literal(AuthorizeResult.ALLOW), + z.literal(AuthorizeResult.DENY), + ]), + ), + ]), }), ), }); @@ -45,6 +52,9 @@ export type ResourcePolicyDecision = ConditionalPolicyDecision & { resourceRef: string; }; +/** + * @internal + */ export class PermissionIntegrationClient { private readonly discovery: DiscoveryService; private readonly auth: AuthService; @@ -74,14 +84,7 @@ export class PermissionIntegrationClient { const response = await fetch(endpoint, { method: 'POST', body: JSON.stringify({ - items: decisions.map( - ({ id, resourceRef, resourceType, conditions }) => ({ - id, - resourceRef, - resourceType, - conditions, - }), - ), + items: decisions, }), headers: { ...(token ? { authorization: `Bearer ${token}` } : {}), diff --git a/plugins/permission-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index 68ae5d8ff5..d1f25c1be0 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -165,11 +165,7 @@ describe('createRouter', () => { mockApplyConditions.mockResolvedValueOnce([ { id: '123', - result: AuthorizeResult.ALLOW, - }, - { - id: '123', - result: AuthorizeResult.DENY, + result: [AuthorizeResult.ALLOW, AuthorizeResult.DENY], }, ]); @@ -198,6 +194,21 @@ describe('createRouter', () => { ], }); + expect(mockApplyConditions).toHaveBeenCalledWith( + 'test-plugin', + expect.any(Object), + [ + { + conditions: { params: ['abc'], rule: 'test-rule' }, + id: '123', + pluginId: 'test-plugin', + resourceRefs: ['resource:1', 'resource:2'], + resourceType: 'test-resource-1', + result: 'CONDITIONAL', + }, + ], + ); + expect(response.status).toEqual(200); expect(policy.handle).toHaveBeenCalledWith( diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index f89a83ed51..1d6f1b36d8 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -26,7 +26,6 @@ import { isResourcePermission, PermissionAttributes, PermissionMessageBatch, - PolicyDecision, } from '@backstage/plugin-permission-common'; import { ApplyConditionsRequestEntry, @@ -125,9 +124,7 @@ const handleRequest = async ( auth: AuthService, userInfo: UserInfoService, ): Promise< - IdentifiedPermissionMessage< - EvaluatePermissionResponse | BulkDefinitivePolicyDecision - >[] + IdentifiedPermissionMessage[] > => { const applyConditionsLoaderFor = memoize((pluginId: string) => { return new DataLoader< @@ -182,20 +179,11 @@ const handleRequest = async ( } if (request.resourceRefs) { - const results = await Promise.all( - request.resourceRefs.map(resourceRef => - applyConditionsLoaderFor(decision.pluginId).load({ - id: request.id, - resourceRef, - ...decision, - }), - ), - ); - - return { + return applyConditionsLoaderFor(decision.pluginId).load({ id: request.id, - result: results.map(({ result }) => result), - }; + resourceRefs: request.resourceRefs, + ...decision, + }); } if (!request.resourceRef) { @@ -254,9 +242,7 @@ export async function createRouter( '/authorize', async ( req: Request, - res: Response< - PermissionMessageBatch - >, + res: Response>, ) => { const credentials = await httpAuth.credentials(req, { allow: ['user', 'none'], @@ -307,6 +293,8 @@ export async function createRouter( /** * @internal */ -type BulkDefinitivePolicyDecision = { - result: Array; -}; +type InternalEvaluatePermissionResponse = + | EvaluatePermissionResponse + | { + result: Array; + }; diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index dbb5647157..973fb39d45 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -567,6 +567,101 @@ describe('createPermissionIntegrationRouter', () => { }); }); + describe('batched requests with resourceRefs', () => { + let response: Response; + + beforeEach(async () => { + const app = express().use( + createPermissionIntegrationRouter(mockedOptionResources).use( + middleware.error(), + ), + ); + + mockTestRule1Apply.mockReturnValueOnce(true); + mockTestRule1Apply.mockReturnValueOnce(false); + mockTestRule1Apply.mockReturnValueOnce(false); + + response = await request(app) + .post('/.well-known/backstage/permissions/apply-conditions') + .send({ + items: [ + { + id: '123', + resourceRefs: [ + 'default:test/resource-1', + 'default:test/resource-2', + ], + resourceType: 'test-resource', + conditions: { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: { + foo: 'a', + bar: 1, + }, + }, + }, + { + id: '234', + resourceRef: 'default:test/resource-3', + resourceType: 'test-resource', + conditions: { + rule: 'test-rule-1', + resourceType: 'test-resource', + params: { + foo: 'a', + bar: 1, + }, + }, + }, + { + id: '345', + resourceRef: 'default:test/resource-2', + resourceType: 'test-resource-2', + conditions: { + not: { + rule: 'test-rule-1', + resourceType: 'test-resource-2', + params: { + foo: 'a', + bar: 1, + }, + }, + }, + }, + ], + }); + }); + + it('processes batched requests', () => { + expect(response.status).toEqual(200); + expect(response.body).toEqual({ + items: [ + { + id: '123', + result: [AuthorizeResult.ALLOW, AuthorizeResult.DENY], + }, + { + id: '234', + result: AuthorizeResult.DENY, + }, + { id: '345', result: AuthorizeResult.ALLOW }, + ], + }); + }); + + it('calls getResources for all required resources at once', () => { + expect(defaultMockedGetResources1).toHaveBeenCalledWith([ + 'default:test/resource-1', + 'default:test/resource-2', + 'default:test/resource-3', + ]); + expect(defaultMockedGetResources2).toHaveBeenCalledWith([ + 'default:test/resource-2', + ]); + }); + }); + it('returns 400 when called with incorrect resource type', async () => { const response = await request(createApp()) .post('/.well-known/backstage/permissions/apply-conditions') diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index cf68af3a3e..0d74f8eca5 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -58,12 +58,22 @@ const permissionCriteriaSchema: z.ZodSchema< const applyConditionsRequestSchema = z.object({ items: z.array( - z.object({ - id: z.string(), - resourceRef: z.string(), - resourceType: z.string(), - conditions: permissionCriteriaSchema, - }), + z.union([ + z.object({ + id: z.string(), + resourceRef: z.string(), + resourceRefs: z.undefined().optional(), + resourceType: z.string(), + conditions: permissionCriteriaSchema, + }), + z.object({ + id: z.string(), + resourceRef: z.undefined().optional(), + resourceRefs: z.array(z.string()), + resourceType: z.string(), + conditions: permissionCriteriaSchema, + }), + ]), ), }); @@ -73,11 +83,20 @@ const applyConditionsRequestSchema = z.object({ * * @public */ -export type ApplyConditionsRequestEntry = IdentifiedPermissionMessage<{ - resourceRef: string; - resourceType: string; - conditions: PermissionCriteria; -}>; +export type ApplyConditionsRequestEntry = IdentifiedPermissionMessage< + | { + resourceRef: string; + resourceRefs?: undefined; + resourceType: string; + conditions: PermissionCriteria; + } + | { + resourceRef?: undefined; + resourceRefs: string[]; + resourceType: string; + conditions: PermissionCriteria; + } +>; /** * A batch of {@link ApplyConditionsRequestEntry} objects. @@ -94,8 +113,12 @@ export type ApplyConditionsRequest = { * * @public */ -export type ApplyConditionsResponseEntry = - IdentifiedPermissionMessage; +export type ApplyConditionsResponseEntry = IdentifiedPermissionMessage< + | DefinitivePolicyDecision + | { + result: Array; + } +>; /** * A batch of {@link ApplyConditionsResponseEntry} objects. @@ -158,6 +181,16 @@ const applyConditions = ( return rule.apply(resource, criteria.params ?? {}); }; +function authorizeResult( + criteria: PermissionCriteria>, + resource: TResource | undefined, + getRule: (name: string) => PermissionRule, +) { + return applyConditions(criteria, resource, getRule) + ? AuthorizeResult.ALLOW + : AuthorizeResult.DENY; +} + /** * Takes some permission conditions and returns a definitive authorization result * on the resource to which they apply. @@ -478,7 +511,7 @@ export function createPermissionIntegrationRouter< router.post( '/.well-known/backstage/permissions/apply-conditions', - async (req, res: Response) => { + async (req, res: Response) => { const parseResult = applyConditionsRequestSchema.safeParse(req.body); if (!parseResult.success) { throw new InputError(parseResult.error.toString()); @@ -503,20 +536,31 @@ export function createPermissionIntegrationRouter< requestedType, requests .filter(r => r.resourceType === requestedType) - .map(i => i.resourceRef), + .map(i => + typeof i.resourceRef === 'string' + ? [i.resourceRef] + : i.resourceRefs, + ) + .flat(), ); } res.json({ items: requests.map(request => ({ id: request.id, - result: applyConditions( - request.conditions, - resourcesByType[request.resourceType][request.resourceRef], - store.getRuleMapper(request.resourceType), - ) - ? AuthorizeResult.ALLOW - : AuthorizeResult.DENY, + result: request.resourceRefs + ? request.resourceRefs.map(resourceRef => + authorizeResult( + request.conditions, + resourcesByType[request.resourceType][resourceRef], + store.getRuleMapper(request.resourceType), + ), + ) + : authorizeResult( + request.conditions, + resourcesByType[request.resourceType][request.resourceRef], + store.getRuleMapper(request.resourceType), + ), })), }); }, From a62afa182ef1bef1a8e5d600325a616d760d7a37 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Fri, 11 Apr 2025 10:55:29 +0200 Subject: [PATCH 07/13] permission: do not forward resourceRefs for basic permissions Signed-off-by: Vincenzo Scamporlino --- .../src/PermissionClient.test.ts | 24 +++++++++--- .../permission-common/src/PermissionClient.ts | 38 +++++++++++++------ 2 files changed, 46 insertions(+), 16 deletions(-) diff --git a/plugins/permission-common/src/PermissionClient.test.ts b/plugins/permission-common/src/PermissionClient.test.ts index b32c5f148e..34b295778a 100644 --- a/plugins/permission-common/src/PermissionClient.test.ts +++ b/plugins/permission-common/src/PermissionClient.test.ts @@ -266,16 +266,31 @@ describe('PermissionClient', () => { }); it('should include a request body', async () => { - await client.authorize([mockAuthorizeConditional]); + const basicPermission = createPermission({ + name: 'test.permission-basic', + attributes: {}, + }); + + await client.authorize([ + { permission: mockPermission, resourceRef: 'foo:bar' }, + { permission: mockPermission, resourceRef: 'foo:car' }, + { permission: mockPermission, resourceRef: 'foo:baz' }, + { permission: basicPermission }, + ]); const request = mockAuthorizeHandler.mock.calls[0][0]; expect(request.body).toEqual({ items: [ - expect.objectContaining({ + { + id: expect.any(String), permission: mockPermission, - resourceRefs: ['foo:bar'], - }), + resourceRefs: ['foo:bar', 'foo:car', 'foo:baz'], + }, + { + id: expect.any(String), + permission: basicPermission, + }, ], }); }); @@ -447,7 +462,6 @@ describe('PermissionClient', () => { name: 'test.permission3', attributes: {}, }, - resourceRefs: [], id: expect.any(String), }, { diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 83773d4cd0..6fd0047253 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -32,7 +32,13 @@ import { IdentifiedPermissionMessage, } from './types/api'; import { DiscoveryApi } from './types/discovery'; -import { AuthorizeRequestOptions, Permission } from './types/permission'; +import { + AuthorizeRequestOptions, + BasicPermission, + Permission, + ResourcePermission, +} from './types/permission'; +import { isResourcePermission } from './permissions'; const permissionCriteriaSchema: z.ZodSchema< PermissionCriteria @@ -199,14 +205,21 @@ export class PermissionClient implements PermissionEvaluator { for (const query of queries) { const { permission, resourceRef } = query; - request[permission.name] ||= { - permission, - resourceRefs: [], - id: uuid.v4(), - }; + if (isResourcePermission(permission)) { + request[permission.name] ||= { + permission, + resourceRefs: [], + id: uuid.v4(), + }; + } else { + request[permission.name] ||= { + permission, + id: uuid.v4(), + }; + } if (resourceRef) { - request[permission.name].resourceRefs.push(resourceRef); + request[permission.name].resourceRefs?.push(resourceRef); } } @@ -257,7 +270,10 @@ export class PermissionClient implements PermissionEvaluator { } } -export type BatchedAuthorizePermissionRequest = IdentifiedPermissionMessage<{ - permission: Permission; - resourceRefs: string[]; -}>; +export type BatchedAuthorizePermissionRequest = IdentifiedPermissionMessage< + | { + permission: BasicPermission; + resourceRefs?: undefined; + } + | { permission: ResourcePermission; resourceRefs: string[] } +>; From 4da29658236bb90f85c6af093fae0767d44d00ec Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Fri, 11 Apr 2025 11:28:14 +0200 Subject: [PATCH 08/13] permission: changeset batched requests Signed-off-by: Vincenzo Scamporlino --- .changeset/lazy-tires-show.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/lazy-tires-show.md diff --git a/.changeset/lazy-tires-show.md b/.changeset/lazy-tires-show.md new file mode 100644 index 0000000000..3ccf5f155e --- /dev/null +++ b/.changeset/lazy-tires-show.md @@ -0,0 +1,7 @@ +--- +'@backstage/plugin-permission-backend': minor +'@backstage/plugin-permission-common': minor +'@backstage/plugin-permission-node': minor +--- + +Fixed an issue causing the `PermissionClient` to exhaust the request body size limit too quickly when making many requests. From 059bb019f8364987a8900623b250a622741c1b5b Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Fri, 11 Apr 2025 14:00:36 +0200 Subject: [PATCH 09/13] permission: api-reports Signed-off-by: Vincenzo Scamporlino --- plugins/permission-common/report.api.md | 2 +- .../permission-common/src/PermissionClient.ts | 4 ++- plugins/permission-common/src/index.ts | 5 +++- plugins/permission-node/report.api.md | 28 ++++++++++++++----- 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/plugins/permission-common/report.api.md b/plugins/permission-common/report.api.md index 465c43846f..ff3c03efb5 100644 --- a/plugins/permission-common/report.api.md +++ b/plugins/permission-common/report.api.md @@ -83,7 +83,7 @@ export type EvaluatePermissionRequest = { resourceRef?: string; }; -// @public +// @public @deprecated export type EvaluatePermissionRequestBatch = PermissionMessageBatch; diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 6fd0047253..edc5093ae8 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -35,7 +35,6 @@ import { DiscoveryApi } from './types/discovery'; import { AuthorizeRequestOptions, BasicPermission, - Permission, ResourcePermission, } from './types/permission'; import { isResourcePermission } from './permissions'; @@ -270,6 +269,9 @@ export class PermissionClient implements PermissionEvaluator { } } +/** + * @internal + */ export type BatchedAuthorizePermissionRequest = IdentifiedPermissionMessage< | { permission: BasicPermission; diff --git a/plugins/permission-common/src/index.ts b/plugins/permission-common/src/index.ts index 7c3f6dd613..3c28921a39 100644 --- a/plugins/permission-common/src/index.ts +++ b/plugins/permission-common/src/index.ts @@ -21,4 +21,7 @@ */ export * from './types'; export * from './permissions'; -export * from './PermissionClient'; +export { + PermissionClient, + type PermissionClientRequestOptions, +} from './PermissionClient'; diff --git a/plugins/permission-node/report.api.md b/plugins/permission-node/report.api.md index 8c743a0f6c..af8c4f6455 100644 --- a/plugins/permission-node/report.api.md +++ b/plugins/permission-node/report.api.md @@ -7,6 +7,7 @@ import { AllOfCriteria } from '@backstage/plugin-permission-common'; import { AnyOfCriteria } from '@backstage/plugin-permission-common'; import { AuthorizePermissionRequest } from '@backstage/plugin-permission-common'; import { AuthorizePermissionResponse } from '@backstage/plugin-permission-common'; +import { AuthorizeResult } from '@backstage/plugin-permission-common'; import { AuthService } from '@backstage/backend-plugin-api'; import { BackstageCredentials } from '@backstage/backend-plugin-api'; import { BackstageUserIdentity } from '@backstage/plugin-auth-node'; @@ -37,11 +38,20 @@ export type ApplyConditionsRequest = { }; // @public -export type ApplyConditionsRequestEntry = IdentifiedPermissionMessage<{ - resourceRef: string; - resourceType: string; - conditions: PermissionCriteria; -}>; +export type ApplyConditionsRequestEntry = IdentifiedPermissionMessage< + | { + resourceRef: string; + resourceRefs?: undefined; + resourceType: string; + conditions: PermissionCriteria; + } + | { + resourceRef?: undefined; + resourceRefs: string[]; + resourceType: string; + conditions: PermissionCriteria; + } +>; // @public export type ApplyConditionsResponse = { @@ -49,8 +59,12 @@ export type ApplyConditionsResponse = { }; // @public -export type ApplyConditionsResponseEntry = - IdentifiedPermissionMessage; +export type ApplyConditionsResponseEntry = IdentifiedPermissionMessage< + | DefinitivePolicyDecision + | { + result: Array; + } +>; // @public export type Condition = TRule extends PermissionRule< From f2cd66a162d6fa7ff8311645f86b8fb457226301 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Fri, 11 Apr 2025 14:04:53 +0200 Subject: [PATCH 10/13] permission: minor tweaks Signed-off-by: Vincenzo Scamporlino --- plugins/permission-common/src/PermissionClient.ts | 5 ++++- .../src/integration/createPermissionIntegrationRouter.ts | 6 +----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index edc5093ae8..830c5c6b32 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -62,7 +62,10 @@ const authorizePermissionResponseSchema: z.ZodSchema r.resourceType === requestedType) - .map(i => - typeof i.resourceRef === 'string' - ? [i.resourceRef] - : i.resourceRefs, - ) + .map(i => i.resourceRefs ?? [i.resourceRef]) .flat(), ); } From 1b4cba98e0b8b83e4e383a4563288ff07d8e3e2d Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 23 Apr 2025 14:51:07 +0200 Subject: [PATCH 11/13] permission-backend: validate resourceRefs Signed-off-by: Vincenzo Scamporlino --- .../src/service/router.test.ts | 24 ++++++++++++------- .../permission-backend/src/service/router.ts | 6 +++-- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/plugins/permission-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index d1f25c1be0..db9c915fae 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -920,6 +920,20 @@ describe('createRouter', () => { }, ], }, + { + items: [ + { + id: '123', + resourceRefs: [], + permission: { + type: 'resource', + name: 'test.permission', + attributes: {}, + resourceType: 'test-resource-1', + }, + }, + ], + }, { items: [ { @@ -965,13 +979,7 @@ describe('createRouter', () => { const response = await request(app).post('/authorize').send(requestBody); expect(response.status).toEqual(400); - expect(response.body).toEqual( - expect.objectContaining({ - error: expect.objectContaining({ - message: expect.stringMatching(/invalid/i), - }), - }), - ); + expect(response.body.error.name).toEqual('InputError'); }); it('returns a 500 error if the policy returns a different resourceType', async () => { @@ -1009,7 +1017,7 @@ describe('createRouter', () => { ); }); - it(`returns a 400 error if the request doesn't contain resourceRef for credentials not issued by a service`, async () => { + it(`returns a 400 error if the request doesn't contain resourceRef or resourceRefs for credentials not issued by a service`, async () => { policy.handle.mockResolvedValueOnce({ result: AuthorizeResult.CONDITIONAL, pluginId: 'test-plugin', diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index 1d6f1b36d8..08bebcf8e3 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -88,7 +88,7 @@ const evaluatePermissionRequestSchema = z.union([ z.object({ id: z.string(), resourceRef: z.undefined().optional(), - resourceRefs: z.array(z.string()), + resourceRefs: z.array(z.string()).nonempty().optional(), permission: resourcePermissionSchema, }), ]); @@ -265,7 +265,9 @@ export async function createRouter( if ( body.items.some( r => - isResourcePermission(r.permission) && r.resourceRef === undefined, + isResourcePermission(r.permission) && + r.resourceRef === undefined && + r.resourceRefs === undefined, ) ) { throw new InputError( From 669038aab857378ed39dfd3885c0bf8ebda876c0 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 23 Apr 2025 15:03:33 +0200 Subject: [PATCH 12/13] permission-common: handle resourceRef only for resource permissions Signed-off-by: Vincenzo Scamporlino --- plugins/permission-common/src/PermissionClient.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 830c5c6b32..7fdb1c239f 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -213,16 +213,16 @@ export class PermissionClient implements PermissionEvaluator { resourceRefs: [], id: uuid.v4(), }; + + if (resourceRef) { + request[permission.name].resourceRefs?.push(resourceRef); + } } else { request[permission.name] ||= { permission, id: uuid.v4(), }; } - - if (resourceRef) { - request[permission.name].resourceRefs?.push(resourceRef); - } } const parsedResponse = await this.makeRawRequest( From 7853dac010e4b20a2b9a239b98ec1ffcaabe64fa Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 29 Apr 2025 10:46:10 +0200 Subject: [PATCH 13/13] permission-backend: accept resourceRef as array Signed-off-by: Vincenzo Scamporlino --- .../src/service/router.test.ts | 89 ++----------------- .../permission-backend/src/service/router.ts | 24 +---- .../src/PermissionClient.test.ts | 6 +- .../permission-common/src/PermissionClient.ts | 15 ++-- plugins/permission-node/report.api.md | 19 ++-- .../createPermissionIntegrationRouter.test.ts | 4 +- .../createPermissionIntegrationRouter.ts | 47 +++------- 7 files changed, 47 insertions(+), 157 deletions(-) diff --git a/plugins/permission-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index db9c915fae..386ae694ed 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -154,7 +154,7 @@ describe('createRouter', () => { }); }); - it('calls the permission policy with batched resourceRefs', async () => { + it('calls the permission policy with batched resourceRef as an array', async () => { policy.handle.mockResolvedValueOnce({ result: AuthorizeResult.CONDITIONAL, pluginId: 'test-plugin', @@ -181,7 +181,7 @@ describe('createRouter', () => { attributes: {}, resourceType: 'test-resource-1', }, - resourceRefs: ['resource:1', 'resource:2'], + resourceRef: ['resource:1', 'resource:2'], }, { id: '234', @@ -202,7 +202,7 @@ describe('createRouter', () => { conditions: { params: ['abc'], rule: 'test-rule' }, id: '123', pluginId: 'test-plugin', - resourceRefs: ['resource:1', 'resource:2'], + resourceRef: ['resource:1', 'resource:2'], resourceType: 'test-resource-1', result: 'CONDITIONAL', }, @@ -611,7 +611,7 @@ describe('createRouter', () => { }); }); - it('leaves conditional results without resourceRefs unchanged', async () => { + it('leaves conditional results without resourceRef unchanged', async () => { policy.handle .mockResolvedValueOnce({ result: AuthorizeResult.CONDITIONAL, @@ -881,79 +881,7 @@ describe('createRouter', () => { items: [ { id: '123', - // resource ref should be a string resourceRef: ['resource:1'], - permission: { - type: 'resource', - name: 'test.permission', - attributes: {}, - resourceType: 'test-resource-1', - }, - }, - ], - }, - { - items: [ - { - id: '123', - resourceRef: ['resource:1'], - permission: { - type: 'resource', - name: 'test.permission', - attributes: {}, - resourceType: 'test-resource-1', - }, - }, - ], - }, - { - items: [ - { - id: '123', - resourceRefs: 'resource:1', - permission: { - type: 'resource', - name: 'test.permission', - attributes: {}, - resourceType: 'test-resource-1', - }, - }, - ], - }, - { - items: [ - { - id: '123', - resourceRefs: [], - permission: { - type: 'resource', - name: 'test.permission', - attributes: {}, - resourceType: 'test-resource-1', - }, - }, - ], - }, - { - items: [ - { - id: '123', - resourceRefs: ['resource:1'], - resourceRef: 'resource:1', - permission: { - type: 'resource', - name: 'test.permission', - attributes: {}, - resourceType: 'test-resource-1', - }, - }, - ], - }, - { - items: [ - { - id: '123', - resourceRefs: ['resource:1'], permission: { type: 'basic', name: 'test.permission', @@ -966,16 +894,17 @@ describe('createRouter', () => { items: [ { id: '123', - resourceRef: 'resource:1', + resourceRef: [], permission: { - type: 'basic', + type: 'resource', name: 'test.permission', attributes: {}, + resourceType: 'test-resource-1', }, }, ], }, - ])('returns a 400 error for invalid request %#', async requestBody => { + ])('returns a 400 error for invalid request %o', async requestBody => { const response = await request(app).post('/authorize').send(requestBody); expect(response.status).toEqual(400); @@ -1017,7 +946,7 @@ describe('createRouter', () => { ); }); - it(`returns a 400 error if the request doesn't contain resourceRef or resourceRefs for credentials not issued by a service`, async () => { + it(`returns a 400 error if the request doesn't contain resourceRef for credentials not issued by a service`, async () => { policy.handle.mockResolvedValueOnce({ result: AuthorizeResult.CONDITIONAL, pluginId: 'test-plugin', diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index 08bebcf8e3..b6f570562b 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -76,19 +76,13 @@ const evaluatePermissionRequestSchema = z.union([ z.object({ id: z.string(), resourceRef: z.undefined().optional(), - resourceRefs: z.undefined().optional(), permission: basicPermissionSchema, }), z.object({ id: z.string(), - resourceRef: z.string().optional(), - resourceRefs: z.undefined().optional(), - permission: resourcePermissionSchema, - }), - z.object({ - id: z.string(), - resourceRef: z.undefined().optional(), - resourceRefs: z.array(z.string()).nonempty().optional(), + resourceRef: z + .union([z.string(), z.array(z.string()).nonempty()]) + .optional(), permission: resourcePermissionSchema, }), ]); @@ -178,14 +172,6 @@ const handleRequest = async ( ); } - if (request.resourceRefs) { - return applyConditionsLoaderFor(decision.pluginId).load({ - id: request.id, - resourceRefs: request.resourceRefs, - ...decision, - }); - } - if (!request.resourceRef) { return { id: request.id, @@ -265,9 +251,7 @@ export async function createRouter( if ( body.items.some( r => - isResourcePermission(r.permission) && - r.resourceRef === undefined && - r.resourceRefs === undefined, + isResourcePermission(r.permission) && r.resourceRef === undefined, ) ) { throw new InputError( diff --git a/plugins/permission-common/src/PermissionClient.test.ts b/plugins/permission-common/src/PermissionClient.test.ts index 34b295778a..b55e5da94d 100644 --- a/plugins/permission-common/src/PermissionClient.test.ts +++ b/plugins/permission-common/src/PermissionClient.test.ts @@ -285,7 +285,7 @@ describe('PermissionClient', () => { { id: expect.any(String), permission: mockPermission, - resourceRefs: ['foo:bar', 'foo:car', 'foo:baz'], + resourceRef: ['foo:bar', 'foo:car', 'foo:baz'], }, { id: expect.any(String), @@ -453,7 +453,7 @@ describe('PermissionClient', () => { attributes: {}, resourceType: 'foo', }, - resourceRefs: ['foo:bar', 'foo:car'], + resourceRef: ['foo:bar', 'foo:car'], id: expect.any(String), }, { @@ -471,7 +471,7 @@ describe('PermissionClient', () => { attributes: {}, resourceType: 'foo', }, - resourceRefs: ['r2', 'r1'], + resourceRef: ['r2', 'r1'], id: expect.any(String), }, ], diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 7fdb1c239f..93c3d5152f 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -210,12 +210,12 @@ export class PermissionClient implements PermissionEvaluator { if (isResourcePermission(permission)) { request[permission.name] ||= { permission, - resourceRefs: [], + resourceRef: [], id: uuid.v4(), }; if (resourceRef) { - request[permission.name].resourceRefs?.push(resourceRef); + request[permission.name].resourceRef?.push(resourceRef); } } else { request[permission.name] ||= { @@ -231,10 +231,15 @@ export class PermissionClient implements PermissionEvaluator { options, ); + const responsesById = parsedResponse.items.reduce((acc, r) => { + acc[r.id] = r; + return acc; + }, {} as Record); + return queries.map(query => { const { id } = request[query.permission.name]; - const item = parsedResponse.items.find(i => i.id === id)!; + const item = responsesById[id]; return { result: query.resourceRef ? item.result.shift()! : item.result[0], }; @@ -278,7 +283,7 @@ export class PermissionClient implements PermissionEvaluator { export type BatchedAuthorizePermissionRequest = IdentifiedPermissionMessage< | { permission: BasicPermission; - resourceRefs?: undefined; + resourceRef?: undefined; } - | { permission: ResourcePermission; resourceRefs: string[] } + | { permission: ResourcePermission; resourceRef: string[] } >; diff --git a/plugins/permission-node/report.api.md b/plugins/permission-node/report.api.md index af8c4f6455..6345861c93 100644 --- a/plugins/permission-node/report.api.md +++ b/plugins/permission-node/report.api.md @@ -38,20 +38,11 @@ export type ApplyConditionsRequest = { }; // @public -export type ApplyConditionsRequestEntry = IdentifiedPermissionMessage< - | { - resourceRef: string; - resourceRefs?: undefined; - resourceType: string; - conditions: PermissionCriteria; - } - | { - resourceRef?: undefined; - resourceRefs: string[]; - resourceType: string; - conditions: PermissionCriteria; - } ->; +export type ApplyConditionsRequestEntry = IdentifiedPermissionMessage<{ + resourceRef: string | string[]; + resourceType: string; + conditions: PermissionCriteria; +}>; // @public export type ApplyConditionsResponse = { diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 973fb39d45..0d28ef710f 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -567,7 +567,7 @@ describe('createPermissionIntegrationRouter', () => { }); }); - describe('batched requests with resourceRefs', () => { + describe('batched requests with resourceRef as an array', () => { let response: Response; beforeEach(async () => { @@ -587,7 +587,7 @@ describe('createPermissionIntegrationRouter', () => { items: [ { id: '123', - resourceRefs: [ + resourceRef: [ 'default:test/resource-1', 'default:test/resource-2', ], diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index 78cf619206..4ee937866b 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -58,22 +58,12 @@ const permissionCriteriaSchema: z.ZodSchema< const applyConditionsRequestSchema = z.object({ items: z.array( - z.union([ - z.object({ - id: z.string(), - resourceRef: z.string(), - resourceRefs: z.undefined().optional(), - resourceType: z.string(), - conditions: permissionCriteriaSchema, - }), - z.object({ - id: z.string(), - resourceRef: z.undefined().optional(), - resourceRefs: z.array(z.string()), - resourceType: z.string(), - conditions: permissionCriteriaSchema, - }), - ]), + z.object({ + id: z.string(), + resourceRef: z.union([z.string(), z.array(z.string()).nonempty()]), + resourceType: z.string(), + conditions: permissionCriteriaSchema, + }), ), }); @@ -83,20 +73,11 @@ const applyConditionsRequestSchema = z.object({ * * @public */ -export type ApplyConditionsRequestEntry = IdentifiedPermissionMessage< - | { - resourceRef: string; - resourceRefs?: undefined; - resourceType: string; - conditions: PermissionCriteria; - } - | { - resourceRef?: undefined; - resourceRefs: string[]; - resourceType: string; - conditions: PermissionCriteria; - } ->; +export type ApplyConditionsRequestEntry = IdentifiedPermissionMessage<{ + resourceRef: string | string[]; + resourceType: string; + conditions: PermissionCriteria; +}>; /** * A batch of {@link ApplyConditionsRequestEntry} objects. @@ -536,7 +517,7 @@ export function createPermissionIntegrationRouter< requestedType, requests .filter(r => r.resourceType === requestedType) - .map(i => i.resourceRefs ?? [i.resourceRef]) + .map(i => i.resourceRef) .flat(), ); } @@ -544,8 +525,8 @@ export function createPermissionIntegrationRouter< res.json({ items: requests.map(request => ({ id: request.id, - result: request.resourceRefs - ? request.resourceRefs.map(resourceRef => + result: Array.isArray(request.resourceRef) + ? request.resourceRef.map(resourceRef => authorizeResult( request.conditions, resourcesByType[request.resourceType][resourceRef],