From b66704db18a51cc9d4e908981b4bcae068204ad8 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 20 Dec 2021 14:12:26 +0000 Subject: [PATCH] permission-node: accept batched requests in /apply-conditions Signed-off-by: MT Lewis --- plugins/permission-backend/package.json | 3 + .../PermissionIntegrationClient.test.ts | 359 ++++++++++++++---- .../service/PermissionIntegrationClient.ts | 85 +++-- .../src/service/router.test.ts | 85 ++++- .../permission-backend/src/service/router.ts | 100 +++-- plugins/permission-node/api-report.md | 28 +- plugins/permission-node/package.json | 2 + .../createPermissionIntegrationRouter.test.ts | 175 ++++++--- .../createPermissionIntegrationRouter.ts | 159 +++++--- plugins/permission-node/src/policy/index.ts | 1 + plugins/permission-node/src/policy/types.ts | 15 +- 11 files changed, 726 insertions(+), 286 deletions(-) diff --git a/plugins/permission-backend/package.json b/plugins/permission-backend/package.json index 890495f646..f628984013 100644 --- a/plugins/permission-backend/package.json +++ b/plugins/permission-backend/package.json @@ -21,12 +21,14 @@ "dependencies": { "@backstage/backend-common": "^0.10.1", "@backstage/config": "^0.1.11", + "@backstage/errors": "^0.1.5", "@backstage/plugin-auth-backend": "^0.6.0", "@backstage/plugin-permission-common": "^0.3.0", "@backstage/plugin-permission-node": "^0.2.3", "@types/express": "*", "express": "^4.17.1", "express-promise-router": "^4.1.0", + "lodash": "^4.17.21", "node-fetch": "^2.6.1", "winston": "^3.2.1", "yn": "^4.0.0", @@ -34,6 +36,7 @@ }, "devDependencies": { "@backstage/cli": "^0.10.4", + "@types/lodash": "^4.14.151", "@types/supertest": "^2.0.8", "supertest": "^6.1.6", "msw": "^0.35.0" diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts index 7f46490ce2..79f604e948 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts @@ -16,7 +16,7 @@ import { AddressInfo } from 'net'; import { Server } from 'http'; -import express, { Router } from 'express'; +import express, { Router, RequestHandler } from 'express'; import { RestContext, rest } from 'msw'; import { setupServer, SetupServerApi } from 'msw/node'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; @@ -39,14 +39,14 @@ describe('PermissionIntegrationClient', () => { const mockApplyConditionsHandler = jest.fn( (_req, res, { json }: RestContext) => { - return res(json({ result: AuthorizeResult.ALLOW })); + return res(json([{ id: '123', result: AuthorizeResult.ALLOW }])); }, ); - const mockBaseUrl = 'http://backstage:9191/i-am-a-mock-base'; + const mockBaseUrl = 'http://backstage:9191'; const discovery: PluginEndpointDiscovery = { - async getBaseUrl() { - return mockBaseUrl; + async getBaseUrl(pluginId) { + return `${mockBaseUrl}/${pluginId}`; }, async getExternalBaseUrl() { throw new Error('Not implemented.'); @@ -64,7 +64,7 @@ describe('PermissionIntegrationClient', () => { server.listen({ onUnhandledRequest: 'error' }); server.use( rest.post( - `${mockBaseUrl}/.well-known/backstage/permissions/apply-conditions`, + `${mockBaseUrl}/plugin-1/.well-known/backstage/permissions/apply-conditions`, mockApplyConditionsHandler, ), ); @@ -77,31 +77,42 @@ describe('PermissionIntegrationClient', () => { }); it('should make a POST request to the correct endpoint', async () => { - await client.applyConditions({ - pluginId: 'test-plugin', - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: mockConditions, - }); + await client.applyConditions([ + { + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ]); expect(mockApplyConditionsHandler).toHaveBeenCalled(); }); it('should include a request body', async () => { - await client.applyConditions({ - pluginId: 'test-plugin', - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: mockConditions, - }); + await client.applyConditions([ + { + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ]); expect(mockApplyConditionsHandler).toHaveBeenCalledWith( expect.objectContaining({ - body: { - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: mockConditions, - }, + body: [ + { + id: '123', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ], }), expect.anything(), expect.anything(), @@ -109,25 +120,33 @@ describe('PermissionIntegrationClient', () => { }); it('should return the response from the fetch request', async () => { - const response = await client.applyConditions({ - pluginId: 'test-plugin', - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: mockConditions, - }); + const response = await client.applyConditions([ + { + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ]); expect(response).toEqual( - expect.objectContaining({ result: AuthorizeResult.ALLOW }), + expect.objectContaining([{ id: '123', result: AuthorizeResult.ALLOW }]), ); }); it('should not include authorization headers if no token is supplied', async () => { - await client.applyConditions({ - pluginId: 'test-plugin', - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: mockConditions, - }); + await client.applyConditions([ + { + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ]); const request = mockApplyConditionsHandler.mock.calls[0][0]; expect(request.headers.has('authorization')).toEqual(false); @@ -135,12 +154,16 @@ describe('PermissionIntegrationClient', () => { it('should include correctly-constructed authorization header if token is supplied', async () => { await client.applyConditions( - { - pluginId: 'test-plugin', - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: mockConditions, - }, + [ + { + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ], 'Bearer fake-token', ); @@ -156,36 +179,95 @@ describe('PermissionIntegrationClient', () => { ); await expect( - client.applyConditions({ - pluginId: 'test-plugin', - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: mockConditions, - }), + client.applyConditions([ + { + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ]), ).rejects.toThrowError(/401/i); }); it('should reject invalid responses', async () => { mockApplyConditionsHandler.mockImplementationOnce( (_req, res, { json }: RestContext) => { - return res(json({ outcome: AuthorizeResult.ALLOW })); + return res(json([{ id: '123', outcome: AuthorizeResult.ALLOW }])); }, ); await expect( - client.applyConditions({ - pluginId: 'test-plugin', - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: mockConditions, - }), + client.applyConditions([ + { + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ]), ).rejects.toThrowError(/invalid input/i); }); + + it('should batch requests to plugin backends', async () => { + mockApplyConditionsHandler.mockImplementationOnce( + (_req, res, { json }: RestContext) => { + return res( + json([ + { id: '123', result: AuthorizeResult.ALLOW }, + { id: '456', result: AuthorizeResult.DENY }, + { id: '789', result: AuthorizeResult.ALLOW }, + ]), + ); + }, + ); + + await expect( + client.applyConditions([ + { + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + { + id: '456', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + { + id: '789', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ]), + ).resolves.toEqual([ + { id: '123', result: AuthorizeResult.ALLOW }, + { id: '456', result: AuthorizeResult.DENY }, + { id: '789', result: AuthorizeResult.ALLOW }, + ]); + + expect(mockApplyConditionsHandler).toHaveBeenCalledTimes(1); + }); }); describe('integration with @backstage/plugin-permission-node', () => { let server: Server; let client: PermissionIntegrationClient; + let plugin1Router: RequestHandler; + let plugin2Router: RequestHandler; beforeAll(async () => { const router = Router(); @@ -217,7 +299,11 @@ describe('PermissionIntegrationClient', () => { const app = express(); - app.use('/test-plugin', router); + plugin1Router = jest.fn(router); + plugin2Router = jest.fn(router); + + app.use('/plugin-1', plugin1Router); + app.use('/plugin-2', plugin2Router); await new Promise(resolve => { server = app.listen(resolve); @@ -252,41 +338,148 @@ describe('PermissionIntegrationClient', () => { it('works for simple conditions', async () => { await expect( - client.applyConditions({ - pluginId: 'test-plugin', - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: { rule: 'RULE_1', params: ['no'] }, - }), - ).resolves.toEqual({ result: AuthorizeResult.DENY }); + client.applyConditions([ + { + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { rule: 'RULE_1', params: ['no'] }, + }, + ]), + ).resolves.toEqual([{ id: '123', result: AuthorizeResult.DENY }]); }); it('works for complex criteria', async () => { await expect( - client.applyConditions({ - pluginId: 'test-plugin', - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: { - allOf: [ - { - allOf: [ - { rule: 'RULE_1', params: ['yes'] }, - { not: { rule: 'RULE_2', params: ['no'] } }, - ], - }, - { - not: { + client.applyConditions([ + { + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { + allOf: [ + { allOf: [ - { rule: 'RULE_1', params: ['no'] }, - { rule: 'RULE_2', params: ['yes'] }, + { rule: 'RULE_1', params: ['yes'] }, + { not: { rule: 'RULE_2', params: ['no'] } }, ], }, - }, - ], + { + not: { + allOf: [ + { rule: 'RULE_1', params: ['no'] }, + { rule: 'RULE_2', params: ['yes'] }, + ], + }, + }, + ], + }, }, - }), - ).resolves.toEqual({ result: AuthorizeResult.ALLOW }); + ]), + ).resolves.toEqual([{ id: '123', result: AuthorizeResult.ALLOW }]); + }); + + it('makes separate batched requests to multiple plugin backends', async () => { + await expect( + client.applyConditions([ + { + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { rule: 'RULE_1', params: ['yes'] }, + }, + { + id: '234', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { rule: 'RULE_1', params: ['no'] }, + }, + { + id: '345', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { rule: 'RULE_1', params: ['no'] }, + }, + { + id: '456', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { rule: 'RULE_1', params: ['yes'] }, + }, + ]), + ).resolves.toEqual([ + { id: '123', result: AuthorizeResult.ALLOW }, + { id: '234', result: AuthorizeResult.DENY }, + { id: '345', result: AuthorizeResult.DENY }, + { id: '456', result: AuthorizeResult.ALLOW }, + ]); + + expect(plugin1Router).toHaveBeenCalledTimes(1); + expect(plugin2Router).toHaveBeenCalledTimes(1); + }); + + it('leaves definitive results unchanged', async () => { + await expect( + client.applyConditions([ + { + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { rule: 'RULE_1', params: ['yes'] }, + }, + { + id: '234', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { rule: 'RULE_1', params: ['no'] }, + }, + { + id: '345', + result: AuthorizeResult.ALLOW, + }, + { + id: '456', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { rule: 'RULE_1', params: ['no'] }, + }, + { + id: '567', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { rule: 'RULE_1', params: ['yes'] }, + }, + ]), + ).resolves.toEqual([ + { id: '123', result: AuthorizeResult.ALLOW }, + { id: '234', result: AuthorizeResult.DENY }, + { id: '345', result: AuthorizeResult.ALLOW }, + { id: '456', result: AuthorizeResult.DENY }, + { id: '567', result: AuthorizeResult.ALLOW }, + ]); + + expect(plugin1Router).toHaveBeenCalledTimes(1); + expect(plugin2Router).toHaveBeenCalledTimes(1); }); }); }); diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.ts index de42a7f194..644cac1b38 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.ts @@ -14,22 +14,37 @@ * limitations under the License. */ +import { groupBy, keyBy } from 'lodash'; import fetch from 'node-fetch'; import { z } from 'zod'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { + AuthorizeResponse, AuthorizeResult, - PermissionCondition, - PermissionCriteria, + Identified, } from '@backstage/plugin-permission-common'; import { - ApplyConditionsRequest, ApplyConditionsResponse, + ConditionalPolicyDecision, + PolicyDecision, } from '@backstage/plugin-permission-node'; -const responseSchema = z.object({ - result: z.literal(AuthorizeResult.ALLOW).or(z.literal(AuthorizeResult.DENY)), -}); +const responseSchema = z.array( + z.object({ + id: z.string(), + result: z + .literal(AuthorizeResult.ALLOW) + .or(z.literal(AuthorizeResult.DENY)), + }), +); + +export type ResourcePolicyDecision = Identified< + PolicyDecision & { resourceRef?: string } +>; + +type ConditionalResourcePolicyDecision = Identified< + ConditionalPolicyDecision & { resourceRef: string } +>; export class PermissionIntegrationClient { private readonly discovery: PluginEndpointDiscovery; @@ -39,32 +54,39 @@ export class PermissionIntegrationClient { } async applyConditions( - { - pluginId, - resourceRef, - resourceType, - conditions, - }: { - resourceRef: string; - pluginId: string; - resourceType: string; - conditions: PermissionCriteria; - }, + decisions: ResourcePolicyDecision[], + authHeader?: string, + ): Promise[]> { + const responses = await Promise.all( + this.groupRequestsByPluginId(decisions).map(([pluginId, requests]) => + this.makeRequest(pluginId, requests, authHeader), + ), + ); + + const responseIndex = keyBy(responses.flat(), 'id'); + + return decisions.map(decision => responseIndex[decision.id] ?? decision); + } + + private async makeRequest( + pluginId: string, + decisions: ConditionalResourcePolicyDecision[], authHeader?: string, ): Promise { const endpoint = `${await this.discovery.getBaseUrl( pluginId, )}/.well-known/backstage/permissions/apply-conditions`; - const request: ApplyConditionsRequest = { - resourceRef, - resourceType, - conditions, - }; - const response = await fetch(endpoint, { method: 'POST', - body: JSON.stringify(request), + body: JSON.stringify( + decisions.map(({ id, resourceRef, resourceType, conditions }) => ({ + id, + resourceRef, + resourceType, + conditions, + })), + ), headers: { ...(authHeader ? { authorization: authHeader } : {}), 'content-type': 'application/json', @@ -79,4 +101,19 @@ export class PermissionIntegrationClient { return responseSchema.parse(await response.json()); } + + private groupRequestsByPluginId( + decisions: ResourcePolicyDecision[], + ): [string, ConditionalResourcePolicyDecision[]][] { + return Object.entries( + groupBy( + decisions.filter( + (decision): decision is ConditionalResourcePolicyDecision => + decision.result === AuthorizeResult.CONDITIONAL && + !!decision.resourceRef, + ), + 'pluginId', + ), + ); + } } diff --git a/plugins/permission-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index 26fa7ef8a6..ef165b2ac5 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -19,14 +19,34 @@ import request from 'supertest'; import { getVoidLogger } from '@backstage/backend-common'; import { IdentityClient } from '@backstage/plugin-auth-backend'; import { AuthorizeResult } from '@backstage/plugin-permission-common'; -import { ApplyConditionsResponse } from '@backstage/plugin-permission-node'; +import { ApplyConditionsResponseEntry } from '@backstage/plugin-permission-node'; import { PermissionIntegrationClient } from './PermissionIntegrationClient'; import { createRouter } from './router'; const mockApplyConditions: jest.MockedFunction< InstanceType['applyConditions'] -> = jest.fn(); +> = jest.fn(async decisions => + decisions.map(decision => { + if ( + decision.result === AuthorizeResult.CONDITIONAL && + decision.resourceRef + ) { + return { id: decision.id, result: AuthorizeResult.DENY as const }; + } + + if (decision.result === AuthorizeResult.CONDITIONAL) { + return { + id: decision.id, + result: decision.result, + conditions: decision.conditions, + }; + } + + return decision; + }), +); + jest.mock('./PermissionIntegrationClient', () => ({ PermissionIntegrationClient: jest.fn(() => ({ applyConditions: mockApplyConditions, @@ -34,7 +54,7 @@ jest.mock('./PermissionIntegrationClient', () => ({ })); const policy = { - handle: jest.fn().mockImplementation((_req, identity) => { + handle: jest.fn().mockImplementation(async (_req, identity) => { if (identity) { return { result: AuthorizeResult.ALLOW }; } @@ -159,7 +179,7 @@ describe('createRouter', () => { describe('conditional policy result', () => { beforeEach(() => { - policy.handle.mockReturnValueOnce({ + policy.handle.mockResolvedValue({ result: AuthorizeResult.CONDITIONAL, pluginId: 'test-plugin', resourceType: 'test-resource-1', @@ -193,15 +213,22 @@ describe('createRouter', () => { ]); }); - it.each([ + it.each([ AuthorizeResult.ALLOW, AuthorizeResult.DENY, ])( 'applies conditions and returns %s if resourceRef is supplied', async result => { - mockApplyConditions.mockResolvedValueOnce({ - result, - }); + mockApplyConditions.mockResolvedValueOnce([ + { + id: '123', + result, + }, + { + id: '234', + result, + }, + ]); const response = await request(app) .post('/authorize') @@ -216,15 +243,35 @@ describe('createRouter', () => { attributes: {}, }, }, + { + id: '234', + resourceRef: 'test/resource', + permission: { + name: 'test.permission', + resourceType: 'test-resource-1', + attributes: {}, + }, + }, ]); expect(mockApplyConditions).toHaveBeenCalledWith( - { - pluginId: 'test-plugin', - resourceType: 'test-resource-1', - resourceRef: 'test/resource', - conditions: { anyOf: [{ rule: 'test-rule', params: ['abc'] }] }, - }, + [ + expect.objectContaining({ + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'test-plugin', + resourceType: 'test-resource-1', + resourceRef: 'test/resource', + conditions: { anyOf: [{ rule: 'test-rule', params: ['abc'] }] }, + }), + expect.objectContaining({ + id: '234', + pluginId: 'test-plugin', + resourceType: 'test-resource-1', + resourceRef: 'test/resource', + conditions: { anyOf: [{ rule: 'test-rule', params: ['abc'] }] }, + }), + ], 'Bearer test-token', ); @@ -234,6 +281,10 @@ describe('createRouter', () => { id: '123', result, }, + { + id: '234', + result, + }, ]); }, ); @@ -247,10 +298,10 @@ describe('createRouter', () => { [{ id: '123' }], [{ id: '123', permission: { name: 'test.permission' } }], [{ id: '123', permission: { attributes: { invalid: 'attribute' } } }], - ])('returns a 500 error for invalid request %#', async requestBody => { + ])('returns a 400 error for invalid request %#', async requestBody => { const response = await request(app).post('/authorize').send(requestBody); - expect(response.status).toEqual(500); + expect(response.status).toEqual(400); expect(response.body).toEqual( expect.objectContaining({ error: expect.objectContaining({ @@ -261,7 +312,7 @@ describe('createRouter', () => { }); it('returns a 500 error if the policy returns a different resourceType', async () => { - policy.handle.mockReturnValueOnce({ + policy.handle.mockResolvedValueOnce({ result: AuthorizeResult.CONDITIONAL, pluginId: 'test-plugin', resourceType: 'test-resource-2', diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index 41791b8fc8..0e877bc816 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -22,6 +22,7 @@ import { errorHandler, PluginEndpointDiscovery, } from '@backstage/backend-common'; +import { InputError } from '@backstage/errors'; import { BackstageIdentityResponse, IdentityClient, @@ -32,7 +33,10 @@ import { AuthorizeRequest, Identified, } from '@backstage/plugin-permission-common'; -import { PermissionPolicy } from '@backstage/plugin-permission-node'; +import { + PermissionPolicy, + PolicyDecision, +} from '@backstage/plugin-permission-node'; import { PermissionIntegrationClient } from './PermissionIntegrationClient'; const requestSchema: z.ZodSchema[]> = z.array( @@ -69,46 +73,55 @@ export interface RouterOptions { identity: IdentityClient; } -const handleRequest = async ( - { id, resourceRef, ...request }: Identified, - user: BackstageIdentityResponse | undefined, +const applyPolicy = async ( policy: PermissionPolicy, - permissionIntegrationClient: PermissionIntegrationClient, - authHeader?: string, -): Promise> => { - const response = await policy.handle(request, user); + requests: Identified[], + user: BackstageIdentityResponse | undefined, +): Promise[]> => { + return Promise.all( + requests.map(({ id, resourceRef, ...authorizeRequest }) => + policy.handle(authorizeRequest, user).then(decision => ({ + id, + ...(decision.result === AuthorizeResult.CONDITIONAL + ? { resourceRef } + : {}), + ...decision, + })), + ), + ); +}; + +const assertMatchingResourceTypes = ( + requests: AuthorizeRequest[], + decisions: PolicyDecision[], +) => { + requests.forEach((request, index) => { + const decision = decisions[index]; - if (response.result === AuthorizeResult.CONDITIONAL) { // Sanity check that any resource provided matches the one expected by the permission - if (request.permission.resourceType !== response.resourceType) { + if ( + decision.result === AuthorizeResult.CONDITIONAL && + decision.resourceType !== request.permission.resourceType + ) { throw new Error( `Invalid resource conditions returned from permission policy for permission ${request.permission.name}`, ); } + }); +}; - if (resourceRef) { - return { - id, - ...(await permissionIntegrationClient.applyConditions( - { - resourceRef, - pluginId: response.pluginId, - resourceType: response.resourceType, - conditions: response.conditions, - }, - authHeader, - )), - }; - } +const handleRequest = async ( + requests: Identified[], + user: BackstageIdentityResponse | undefined, + policy: PermissionPolicy, + permissionIntegrationClient: PermissionIntegrationClient, + authHeader?: string, +): Promise[]> => { + const decisions = await applyPolicy(policy, requests, user); - return { - id, - result: AuthorizeResult.CONDITIONAL, - conditions: response.conditions, - }; - } + assertMatchingResourceTypes(requests, decisions); - return { id, ...response }; + return permissionIntegrationClient.applyConditions(decisions, authHeader); }; /** @@ -142,24 +155,27 @@ export async function createRouter( const token = IdentityClient.getBearerToken(req.header('authorization')); const user = token ? await identity.authenticate(token) : undefined; - const body = requestSchema.parse(req.body); + const parseResult = requestSchema.safeParse(req.body); + + if (!parseResult.success) { + throw new InputError(parseResult.error.toString()); + } + + const body = parseResult.data; res.json( - await Promise.all( - body.map(request => - handleRequest( - request, - user, - policy, - permissionIntegrationClient, - req.header('authorization'), - ), - ), + await handleRequest( + body, + user, + policy, + permissionIntegrationClient, + req.header('authorization'), ), ); }, ); router.use(errorHandler()); + return router; } diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index e743f18311..9e0d521cd3 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -9,24 +9,29 @@ import { AuthorizeResponse } from '@backstage/plugin-permission-common'; import { AuthorizeResult } from '@backstage/plugin-permission-common'; import { BackstageIdentityResponse } from '@backstage/plugin-auth-backend'; import { Config } from '@backstage/config'; +import express from 'express'; +import { Identified } from '@backstage/plugin-permission-common'; import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; import { PermissionCondition } from '@backstage/plugin-permission-common'; import { PermissionCriteria } from '@backstage/plugin-permission-common'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; -import { Router } from 'express'; import { TokenManager } from '@backstage/backend-common'; // @public -export type ApplyConditionsRequest = { +export type ApplyConditionsRequest = ApplyConditionsRequestEntry[]; + +// @public +export type ApplyConditionsRequestEntry = Identified<{ resourceRef: string; resourceType: string; conditions: PermissionCriteria; -}; +}>; // @public -export type ApplyConditionsResponse = { - result: AuthorizeResult.ALLOW | AuthorizeResult.DENY; -}; +export type ApplyConditionsResponse = ApplyConditionsResponseEntry[]; + +// @public +export type ApplyConditionsResponseEntry = Identified; // @public export type Condition = TRule extends PermissionRule< @@ -93,7 +98,12 @@ export const createPermissionIntegrationRouter: (options: { resourceType: string; rules: PermissionRule[]; getResource: (resourceRef: string) => Promise; -}) => Router; +}) => express.Router; + +// @public +export type DefinitivePolicyDecision = { + result: AuthorizeResult.ALLOW | AuthorizeResult.DENY; +}; // @public export const createPermissionRule: < @@ -137,9 +147,7 @@ export type PolicyAuthorizeRequest = Omit; // @public export type PolicyDecision = - | { - result: AuthorizeResult.ALLOW | AuthorizeResult.DENY; - } + | DefinitivePolicyDecision | ConditionalPolicyDecision; // @public diff --git a/plugins/permission-node/package.json b/plugins/permission-node/package.json index 55f955110e..a5b32e2507 100644 --- a/plugins/permission-node/package.json +++ b/plugins/permission-node/package.json @@ -31,10 +31,12 @@ "dependencies": { "@backstage/backend-common": "^0.10.1", "@backstage/config": "^0.1.11", + "@backstage/errors": "^0.1.5", "@backstage/plugin-auth-backend": "^0.6.0", "@backstage/plugin-permission-common": "^0.3.0", "@types/express": "^4.17.6", "express": "^4.17.1", + "express-promise-router": "^4.1.0", "zod": "^3.11.6" }, "devDependencies": { diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 15c9cf4003..c7c48ae9fc 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -16,16 +16,12 @@ import { AuthorizeResult } from '@backstage/plugin-permission-common'; import express, { Express, Router } from 'express'; -import request from 'supertest'; +import request, { Response } from 'supertest'; import { createPermissionIntegrationRouter } from './createPermissionIntegrationRouter'; const mockGetResource: jest.MockedFunction< - (resourceRef: string) => Promise -> = jest.fn((resourceRef: string) => - Promise.resolve({ - resourceRef, - }), -); + Parameters[0]['getResource'] +> = jest.fn(async resourceRef => ({ id: resourceRef })); const testRule1 = { name: 'test-rule-1', @@ -47,7 +43,7 @@ describe('createPermissionIntegrationRouter', () => { let app: Express; let router: Router; - beforeEach(() => { + beforeAll(() => { router = createPermissionIntegrationRouter({ resourceType: 'test-resource', getResource: mockGetResource, @@ -57,6 +53,10 @@ describe('createPermissionIntegrationRouter', () => { app = express().use(router); }); + afterEach(() => { + jest.clearAllMocks(); + }); + it('works', async () => { expect(router).toBeDefined(); }); @@ -70,7 +70,6 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-2', params: [{}] }, ], }, - { not: { rule: 'test-rule-2', params: [{}] }, }, @@ -95,14 +94,22 @@ describe('createPermissionIntegrationRouter', () => { ])('returns 200/ALLOW when criteria match (case %#)', async conditions => { const response = await request(app) .post('/.well-known/backstage/permissions/apply-conditions') - .send({ - resourceRef: 'default:test/resource', - resourceType: 'test-resource', - conditions, - }); + .send([ + { + id: '123', + resourceRef: 'default:test/resource', + resourceType: 'test-resource', + conditions, + }, + ]); expect(response.status).toEqual(200); - expect(response.body).toEqual({ result: AuthorizeResult.ALLOW }); + expect(response.body).toEqual([ + { + id: '123', + result: AuthorizeResult.ALLOW, + }, + ]); }); it.each([ @@ -136,27 +143,92 @@ describe('createPermissionIntegrationRouter', () => { async conditions => { const response = await request(app) .post('/.well-known/backstage/permissions/apply-conditions') - .send({ - resourceRef: 'default:test/resource', - resourceType: 'test-resource', - conditions, - }); + .send([ + { + id: '123', + resourceRef: 'default:test/resource', + resourceType: 'test-resource', + conditions, + }, + ]); expect(response.status).toEqual(200); - expect(response.body).toEqual({ result: AuthorizeResult.DENY }); + expect(response.body).toEqual([ + { id: '123', result: AuthorizeResult.DENY }, + ]); }, ); + describe('batched requests', () => { + let response: Response; + + beforeEach(async () => { + response = await request(app) + .post('/.well-known/backstage/permissions/apply-conditions') + .send([ + { + id: '123', + resourceRef: 'default:test/resource-1', + resourceType: 'test-resource', + conditions: { rule: 'test-rule-1', params: [] }, + }, + { + id: '234', + resourceRef: 'default:test/resource-1', + resourceType: 'test-resource', + conditions: { rule: 'test-rule-2', params: [] }, + }, + { + id: '345', + resourceRef: 'default:test/resource-2', + resourceType: 'test-resource', + conditions: { not: { rule: 'test-rule-1', params: [] } }, + }, + { + id: '456', + resourceRef: 'default:test/resource-3', + resourceType: 'test-resource', + conditions: { not: { rule: 'test-rule-2', params: [] } }, + }, + { + id: '567', + resourceRef: 'default:test/resource-4', + resourceType: 'test-resource', + conditions: { + anyOf: [ + { rule: 'test-rule-1', params: [] }, + { rule: 'test-rule-2', params: [] }, + ], + }, + }, + ]); + }); + + it('processes batched requests', () => { + expect(response.status).toEqual(200); + expect(response.body).toEqual([ + { id: '123', result: AuthorizeResult.ALLOW }, + { id: '234', result: AuthorizeResult.DENY }, + { id: '345', result: AuthorizeResult.DENY }, + { id: '456', result: AuthorizeResult.ALLOW }, + { id: '567', result: AuthorizeResult.ALLOW }, + ]); + }); + }); + it('returns 400 when called with incorrect resource type', async () => { const response = await request(app) .post('/.well-known/backstage/permissions/apply-conditions') - .send({ - resourceRef: 'default:test/resource', - resourceType: 'test-incorrect-resource', - conditions: { - anyOf: [], + .send([ + { + id: '123', + resourceRef: 'default:test/resource', + resourceType: 'test-incorrect-resource', + conditions: { + anyOf: [], + }, }, - }); + ]); expect(response.status).toEqual(400); expect(response.error && response.error.text).toMatch( @@ -164,47 +236,50 @@ describe('createPermissionIntegrationRouter', () => { ); }); - it('returns 400 when resource is not found', async () => { + it('returns 200/DENY when resource is not found', async () => { mockGetResource.mockReturnValueOnce(Promise.resolve(undefined)); const response = await request(app) .post('/.well-known/backstage/permissions/apply-conditions') - .send({ - resourceRef: 'default:test/resource', - resourceType: 'test-resource', - conditions: { - not: { - rule: 'testRule1', - params: ['a', 1], - }, + .send([ + { + id: '123', + resourceRef: 'default:test/resource', + resourceType: 'test-resource', + conditions: { rule: 'testRule1', params: [] }, }, - }); + ]); - expect(response.status).toEqual(400); - expect(response.error && response.error.text).toMatch( - /resource for ref default:test\/resource not found/i, - ); + expect(response.status).toEqual(200); + expect(response.body).toEqual([ + { + id: '123', + result: AuthorizeResult.DENY, + }, + ]); }); it.each([ undefined, + '', {}, { resourceType: 'test-resource-type' }, - { resourceRef: 'test/resource-ref' }, - { - resourceType: 'test-resource-type', - resourceRef: 'test/resource-ref', - }, - { conditions: { anyOf: [] } }, + [{ resourceType: 'test-resource-type' }], + [{ resourceRef: 'test/resource-ref' }], + [ + { + resourceType: 'test-resource-type', + resourceRef: 'test/resource-ref', + }, + ], + [{ conditions: { anyOf: [] } }], ])(`returns 400 for invalid input %#`, async input => { const response = await request(app) .post('/.well-known/backstage/permissions/apply-conditions') .send(input); expect(response.status).toEqual(400); - expect(response.error && response.error.text).toMatch( - /invalid request body/i, - ); + expect(response.error && response.error.text).toMatch(/invalid/i); }); }); }); diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index b8d3e02a81..919af47c02 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -14,10 +14,14 @@ * limitations under the License. */ -import express, { Response, Router } from 'express'; +import express, { Response } from 'express'; +import Router from 'express-promise-router'; import { z } from 'zod'; +import { InputError } from '@backstage/errors'; +import { errorHandler } from '@backstage/backend-common'; import { AuthorizeResult, + Identified, PermissionCondition, PermissionCriteria, } from '@backstage/plugin-permission-common'; @@ -28,6 +32,7 @@ import { isNotCriteria, isOrCriteria, } from './util'; +import { DefinitivePolicyDecision } from '../policy/types'; const permissionCriteriaSchema: z.ZodSchema< PermissionCriteria @@ -43,11 +48,14 @@ const permissionCriteriaSchema: z.ZodSchema< ]), ); -const applyConditionsRequestSchema = z.object({ - resourceRef: z.string(), - resourceType: z.string(), - conditions: permissionCriteriaSchema, -}); +const applyConditionsRequestSchema = z.array( + z.object({ + id: z.string(), + resourceRef: z.string(), + resourceType: z.string(), + conditions: permissionCriteriaSchema, + }), +); /** * A request to load the referenced resource and apply conditions in order to @@ -55,11 +63,18 @@ const applyConditionsRequestSchema = z.object({ * * @public */ -export type ApplyConditionsRequest = { +export type ApplyConditionsRequestEntry = Identified<{ resourceRef: string; resourceType: string; conditions: PermissionCriteria; -}; +}>; + +/** + * A batch of {@link ApplyConditionsRequestEntry} objects. + * + * @public + */ +export type ApplyConditionsRequest = ApplyConditionsRequestEntry[]; /** * The result of applying the conditions, expressed as a definitive authorize @@ -67,15 +82,27 @@ export type ApplyConditionsRequest = { * * @public */ -export type ApplyConditionsResponse = { - result: AuthorizeResult.ALLOW | AuthorizeResult.DENY; -}; +export type ApplyConditionsResponseEntry = Identified; + +/** + * A batch of {@link ApplyConditionsResponseEntry} objects. + * + * @public + */ +export type ApplyConditionsResponse = ApplyConditionsResponseEntry[]; const applyConditions = ( criteria: PermissionCriteria, resource: TResource, getRule: (name: string) => PermissionRule, ): boolean => { + // If resource was not found, deny. This avoids leaking information from the + // apply-conditions API which would allow a user to differentiate between + // non-existent resources and resources to which they do not have access. + if (typeof resource === 'undefined') { + return false; + } + if (isAndCriteria(criteria)) { return criteria.allOf.every(child => applyConditions(child, resource, getRule), @@ -92,30 +119,37 @@ const applyConditions = ( }; /** - * Create an express Router which provides an authorization route to allow integration between the - * permission backend and other Backstage backend plugins. Plugin owners that wish to support - * conditional authorization for their resources should add the router created by this function - * to their express app inside their `createRouter` implementation. + * Create an express Router which provides an authorization route to allow + * integration between the permission backend and other Backstage backend + * plugins. Plugin owners that wish to support conditional authorization for + * their resources should add the router created by this function to their + * express app inside their `createRouter` implementation. * * @remarks * - * To make this concrete, we can use the Backstage software catalog as an example. The catalog has - * conditional rules around access to specific _entities_ in the catalog. The _type_ of resource is - * captured here as `resourceType`, a string identifier (`catalog-entity` in this example) that can - * be provided with permission definitions. This is merely a _type_ to verify that conditions in an - * authorization policy are constructed correctly, not a reference to a specific resource. + * To make this concrete, we can use the Backstage software catalog as an + * example. The catalog has conditional rules around access to specific + * _entities_ in the catalog. The _type_ of resource is captured here as + * `resourceType`, a string identifier (`catalog-entity` in this example) that + * can be provided with permission definitions. This is merely a _type_ to + * verify that conditions in an authorization policy are constructed correctly, + * not a reference to a specific resource. * - * The `rules` parameter is an array of {@link PermissionRule}s that introduce conditional - * filtering logic for resources; for the catalog, these are things like `isEntityOwner` or - * `hasAnnotation`. Rules describe how to filter a list of resources, and the `conditions` returned - * allow these rules to be applied with specific parameters (such as 'group:default/team-a', or + * The `rules` parameter is an array of {@link PermissionRule}s that introduce + * conditional filtering logic for resources; for the catalog, these are things + * like `isEntityOwner` or `hasAnnotation`. Rules describe how to filter a list + * of resources, and the `conditions` returned allow these rules to be applied + * with specific parameters (such as 'group:default/team-a', or * 'backstage.io/edit-url'). * - * The `getResource` argument should load a resource by reference. For the catalog, this is an - * {@link @backstage/catalog-model#EntityRef}. For other plugins, this can be any serialized format. - * This is used to construct the `createPermissionIntegrationRouter`, a function to add an - * authorization route to your backend plugin. This route will be called by the `permission-backend` - * when authorization conditions relating to this plugin need to be evaluated. + * The `getResources` argument should load resources based on a reference + * identifier. For the catalog, this is an + * {@link @backstage/catalog-model#EntityRef}. For other plugins, this can be + * any serialized format. This is used to construct the + * `createPermissionIntegrationRouter`, a function to add an authorization route + * to your backend plugin. This function will be called by the + * `permission-backend` when authorization conditions relating to this plugin + * need to be evaluated. * * @public */ @@ -123,53 +157,60 @@ export const createPermissionIntegrationRouter = (options: { resourceType: string; rules: PermissionRule[]; getResource: (resourceRef: string) => Promise; -}): Router => { +}): express.Router => { const { resourceType, rules, getResource } = options; const router = Router(); const getRule = createGetRule(rules); + const assertValidResourceTypes = (requests: ApplyConditionsRequest) => { + const invalidResourceType = requests.find( + request => request.resourceType !== resourceType, + )?.resourceType; + + if (invalidResourceType) { + throw new InputError(`Unexpected resource type: ${invalidResourceType}.`); + } + }; + + router.use(express.json()); + router.post( '/.well-known/backstage/permissions/apply-conditions', - express.json(), - async ( - req, - res: Response< - | { - result: Omit; - } - | string - >, - ) => { + async (req, res: Response) => { const parseResult = applyConditionsRequestSchema.safeParse(req.body); if (!parseResult.success) { - return res.status(400).send(`Invalid request body.`); + throw new InputError(parseResult.error.toString()); } - const { data: body } = parseResult; + const body = parseResult.data; - if (body.resourceType !== resourceType) { - return res - .status(400) - .send(`Unexpected resource type: ${body.resourceType}.`); + assertValidResourceTypes(body); + + const resources = {} as Record; + for (const { resourceRef } of body) { + if (!resources[resourceRef]) { + resources[resourceRef] = await getResource(resourceRef); + } } - const resource = await getResource(body.resourceRef); - - if (!resource) { - return res - .status(400) - .send(`Resource for ref ${body.resourceRef} not found.`); - } - - return res.status(200).json({ - result: applyConditions(body.conditions, resource, getRule) - ? AuthorizeResult.ALLOW - : AuthorizeResult.DENY, - }); + return res.status(200).json( + body.map(request => ({ + id: request.id, + result: applyConditions( + request.conditions, + resources[request.resourceRef], + getRule, + ) + ? AuthorizeResult.ALLOW + : AuthorizeResult.DENY, + })), + ); }, ); + router.use(errorHandler()); + return router; }; diff --git a/plugins/permission-node/src/policy/index.ts b/plugins/permission-node/src/policy/index.ts index c8216989a1..1b05f240d3 100644 --- a/plugins/permission-node/src/policy/index.ts +++ b/plugins/permission-node/src/policy/index.ts @@ -16,6 +16,7 @@ export type { ConditionalPolicyDecision, + DefinitivePolicyDecision, PermissionPolicy, PolicyAuthorizeRequest, PolicyDecision, diff --git a/plugins/permission-node/src/policy/types.ts b/plugins/permission-node/src/policy/types.ts index d122ad8d8d..4c6a033e11 100644 --- a/plugins/permission-node/src/policy/types.ts +++ b/plugins/permission-node/src/policy/types.ts @@ -35,6 +35,19 @@ import { BackstageIdentityResponse } from '@backstage/plugin-auth-backend'; */ export type PolicyAuthorizeRequest = Omit; +/** + * A definitive result to an authorization request, returned by the {@link PermissionPolicy}. + * + * @remarks + * + * This indicates that the policy unconditionally allows (or denies) the request. + * + * @public + */ +export type DefinitivePolicyDecision = { + result: AuthorizeResult.ALLOW | AuthorizeResult.DENY; +}; + /** * A conditional result to an authorization request, returned by the {@link PermissionPolicy}. * @@ -61,7 +74,7 @@ export type ConditionalPolicyDecision = { * @public */ export type PolicyDecision = - | { result: AuthorizeResult.ALLOW | AuthorizeResult.DENY } + | DefinitivePolicyDecision | ConditionalPolicyDecision; /**