From b66704db18a51cc9d4e908981b4bcae068204ad8 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 20 Dec 2021 14:12:26 +0000 Subject: [PATCH 01/13] 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; /** From 706b6c29e9a69afa03bef3c0be5f964236705804 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 20 Dec 2021 14:37:45 +0000 Subject: [PATCH 02/13] permission-node: allow batch retrieval of resources in /apply-conditions Signed-off-by: MT Lewis --- .../PermissionIntegrationClient.test.ts | 6 ++++- plugins/permission-node/api-report.md | 2 +- .../createPermissionIntegrationRouter.test.ts | 24 +++++++++++++++---- .../createPermissionIntegrationRouter.ts | 13 ++++------ 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts index 79f604e948..57beda9c1f 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts @@ -275,7 +275,11 @@ describe('PermissionIntegrationClient', () => { router.use( createPermissionIntegrationRouter({ resourceType: 'test-resource', - getResource: async resourceRef => ({ id: resourceRef }), + getResources: async resourceRefs => + resourceRefs.reduce((acc, ref) => { + acc[ref] = { id: ref }; + return acc; + }, {} as Record), rules: [ { name: 'RULE_1', diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 9e0d521cd3..e90a1f2677 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -97,7 +97,7 @@ export const createConditionTransformer: < export const createPermissionIntegrationRouter: (options: { resourceType: string; rules: PermissionRule[]; - getResource: (resourceRef: string) => Promise; + getResources: (resourceRefs: string[]) => Promise>; }) => express.Router; // @public diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index c7c48ae9fc..37495ae76e 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -19,9 +19,14 @@ import express, { Express, Router } from 'express'; import request, { Response } from 'supertest'; import { createPermissionIntegrationRouter } from './createPermissionIntegrationRouter'; -const mockGetResource: jest.MockedFunction< - Parameters[0]['getResource'] -> = jest.fn(async resourceRef => ({ id: resourceRef })); +const mockGetResources: jest.MockedFunction< + Parameters[0]['getResources'] +> = jest.fn(async resourceRefs => + resourceRefs.reduce( + (acc, resourceRef) => ({ ...acc, [resourceRef]: { id: resourceRef } }), + {}, + ), +); const testRule1 = { name: 'test-rule-1', @@ -46,7 +51,7 @@ describe('createPermissionIntegrationRouter', () => { beforeAll(() => { router = createPermissionIntegrationRouter({ resourceType: 'test-resource', - getResource: mockGetResource, + getResources: mockGetResources, rules: [testRule1, testRule2], }); @@ -214,6 +219,15 @@ describe('createPermissionIntegrationRouter', () => { { id: '567', result: AuthorizeResult.ALLOW }, ]); }); + + it('calls getResources for all required resources at once', () => { + expect(mockGetResources).toHaveBeenCalledWith([ + 'default:test/resource-1', + 'default:test/resource-2', + 'default:test/resource-3', + 'default:test/resource-4', + ]); + }); }); it('returns 400 when called with incorrect resource type', async () => { @@ -237,7 +251,7 @@ describe('createPermissionIntegrationRouter', () => { }); it('returns 200/DENY when resource is not found', async () => { - mockGetResource.mockReturnValueOnce(Promise.resolve(undefined)); + mockGetResources.mockReturnValueOnce(Promise.resolve({})); const response = await request(app) .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 919af47c02..c15f3cc80b 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -156,9 +156,9 @@ const applyConditions = ( export const createPermissionIntegrationRouter = (options: { resourceType: string; rules: PermissionRule[]; - getResource: (resourceRef: string) => Promise; + getResources: (resourceRefs: string[]) => Promise>; }): express.Router => { - const { resourceType, rules, getResource } = options; + const { resourceType, rules, getResources } = options; const router = Router(); const getRule = createGetRule(rules); @@ -188,12 +188,9 @@ export const createPermissionIntegrationRouter = (options: { assertValidResourceTypes(body); - const resources = {} as Record; - for (const { resourceRef } of body) { - if (!resources[resourceRef]) { - resources[resourceRef] = await getResource(resourceRef); - } - } + const resources = await getResources( + Array.from(new Set(body.map(({ resourceRef }) => resourceRef))), + ); return res.status(200).json( body.map(request => ({ From 419ca637c075ae4765a8e9674080642a96521c08 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 20 Dec 2021 15:14:38 +0000 Subject: [PATCH 03/13] permissions: add changeset for optimizations Signed-off-by: MT Lewis --- .changeset/shiny-bugs-beam.md | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 .changeset/shiny-bugs-beam.md diff --git a/.changeset/shiny-bugs-beam.md b/.changeset/shiny-bugs-beam.md new file mode 100644 index 0000000000..bc50486526 --- /dev/null +++ b/.changeset/shiny-bugs-beam.md @@ -0,0 +1,11 @@ +--- +'@backstage/plugin-permission-backend': minor +'@backstage/plugin-permission-node': minor +--- + +Optimizations to the integration between the permission backend and plugin-backends using createPermissionIntegrationRouter: + +- The permission backend already supported batched requests to authorize, but would make calls to plugin backend to apply conditions serially. Now, after applying the policy for each authorization request, the permission backend makes a single batched /apply-conditions request to each plugin backend referenced in policy decisions. +- The `getResource` method accepted by `createPermissionIntegrationRouter` has been replaced with `getResources`, to allow consumers to make batch requests to upstream data stores. When /apply-conditions is called with a batch of requests, all required resources are requested in a single invocation of `getResources`. + +Plugin owners consuming `createPermissionIntegrationRouter` should replace the `getResource` method in the options with a `getResources` method, accepting an array of resourceRefs, and returning a record object mapping those resourceRefs to resources (if present). From 8e72b573aa0b7b9e6fbd43de137b48614aac4bf7 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 20 Dec 2021 17:17:20 +0000 Subject: [PATCH 04/13] permission-node: switch to array for getResources return value Signed-off-by: MT Lewis --- .changeset/shiny-bugs-beam.md | 2 +- .../service/PermissionIntegrationClient.test.ts | 7 +++---- plugins/permission-node/api-report.md | 2 +- .../createPermissionIntegrationRouter.test.ts | 9 ++++----- .../createPermissionIntegrationRouter.ts | 14 +++++++++++--- 5 files changed, 20 insertions(+), 14 deletions(-) diff --git a/.changeset/shiny-bugs-beam.md b/.changeset/shiny-bugs-beam.md index bc50486526..b16c8d6729 100644 --- a/.changeset/shiny-bugs-beam.md +++ b/.changeset/shiny-bugs-beam.md @@ -8,4 +8,4 @@ Optimizations to the integration between the permission backend and plugin-backe - The permission backend already supported batched requests to authorize, but would make calls to plugin backend to apply conditions serially. Now, after applying the policy for each authorization request, the permission backend makes a single batched /apply-conditions request to each plugin backend referenced in policy decisions. - The `getResource` method accepted by `createPermissionIntegrationRouter` has been replaced with `getResources`, to allow consumers to make batch requests to upstream data stores. When /apply-conditions is called with a batch of requests, all required resources are requested in a single invocation of `getResources`. -Plugin owners consuming `createPermissionIntegrationRouter` should replace the `getResource` method in the options with a `getResources` method, accepting an array of resourceRefs, and returning a record object mapping those resourceRefs to resources (if present). +Plugin owners consuming `createPermissionIntegrationRouter` should replace the `getResource` method in the options with a `getResources` method, accepting an array of resourceRefs, and returning an array of the corresponding resources. diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts index 57beda9c1f..4697277377 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts @@ -276,10 +276,9 @@ describe('PermissionIntegrationClient', () => { createPermissionIntegrationRouter({ resourceType: 'test-resource', getResources: async resourceRefs => - resourceRefs.reduce((acc, ref) => { - acc[ref] = { id: ref }; - return acc; - }, {} as Record), + resourceRefs.map(resourceRef => ({ + id: resourceRef, + })), rules: [ { name: 'RULE_1', diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index e90a1f2677..7023369306 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -97,7 +97,7 @@ export const createConditionTransformer: < export const createPermissionIntegrationRouter: (options: { resourceType: string; rules: PermissionRule[]; - getResources: (resourceRefs: string[]) => Promise>; + getResources: (resourceRefs: string[]) => Promise<(TResource | undefined)[]>; }) => express.Router; // @public diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 37495ae76e..2e911b7cae 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -22,10 +22,7 @@ import { createPermissionIntegrationRouter } from './createPermissionIntegration const mockGetResources: jest.MockedFunction< Parameters[0]['getResources'] > = jest.fn(async resourceRefs => - resourceRefs.reduce( - (acc, resourceRef) => ({ ...acc, [resourceRef]: { id: resourceRef } }), - {}, - ), + resourceRefs.map(resourceRef => ({ id: resourceRef })), ); const testRule1 = { @@ -251,7 +248,9 @@ describe('createPermissionIntegrationRouter', () => { }); it('returns 200/DENY when resource is not found', async () => { - mockGetResources.mockReturnValueOnce(Promise.resolve({})); + mockGetResources.mockImplementationOnce(async resourceRefs => + resourceRefs.map(() => undefined), + ); const response = await request(app) .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 c15f3cc80b..0fdbab7a9f 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -156,7 +156,9 @@ const applyConditions = ( export const createPermissionIntegrationRouter = (options: { resourceType: string; rules: PermissionRule[]; - getResources: (resourceRefs: string[]) => Promise>; + getResources: ( + resourceRefs: string[], + ) => Promise>; }): express.Router => { const { resourceType, rules, getResources } = options; const router = Router(); @@ -188,9 +190,15 @@ export const createPermissionIntegrationRouter = (options: { assertValidResourceTypes(body); - const resources = await getResources( - Array.from(new Set(body.map(({ resourceRef }) => resourceRef))), + const resourceRefs = Array.from( + new Set(body.map(({ resourceRef }) => resourceRef)), ); + const resourceArray = await getResources(resourceRefs); + const resources = resourceRefs.reduce((acc, resourceRef, index) => { + acc[resourceRef] = resourceArray[index]; + + return acc; + }, {} as Record); return res.status(200).json( body.map(request => ({ From cbb85e07f0687121c490a7b63df1594b108293ae Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Wed, 22 Dec 2021 10:17:21 +0000 Subject: [PATCH 05/13] permission-node: simplify undefined check and fix applyConditions signature Signed-off-by: MT Lewis --- .../src/integration/createPermissionIntegrationRouter.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index 0fdbab7a9f..747e47b87d 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -93,13 +93,13 @@ export type ApplyConditionsResponse = ApplyConditionsResponseEntry[]; const applyConditions = ( criteria: PermissionCriteria, - resource: TResource, + resource: TResource | undefined, 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') { + if (resource === undefined) { return false; } From e4c2ee4ba7f0b1b5b2e224488e220c3e9e0519b7 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Wed, 5 Jan 2022 12:34:46 +0000 Subject: [PATCH 06/13] build(deps): add dependency on dataloader Signed-off-by: MT Lewis --- plugins/permission-backend/package.json | 1 + yarn.lock | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/plugins/permission-backend/package.json b/plugins/permission-backend/package.json index f628984013..8a9bd61445 100644 --- a/plugins/permission-backend/package.json +++ b/plugins/permission-backend/package.json @@ -26,6 +26,7 @@ "@backstage/plugin-permission-common": "^0.3.0", "@backstage/plugin-permission-node": "^0.2.3", "@types/express": "*", + "dataloader": "^2.0.0", "express": "^4.17.1", "express-promise-router": "^4.1.0", "lodash": "^4.17.21", diff --git a/yarn.lock b/yarn.lock index c96d4cd39c..88afd4080a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -12834,7 +12834,7 @@ data-urls@^2.0.0: whatwg-mimetype "^2.3.0" whatwg-url "^8.0.0" -dataloader@2.0.0: +dataloader@2.0.0, dataloader@^2.0.0: version "2.0.0" resolved "https://registry.npmjs.org/dataloader/-/dataloader-2.0.0.tgz#41eaf123db115987e21ca93c005cd7753c55fe6f" integrity sha512-YzhyDAwA4TaQIhM5go+vCLmU0UikghC/t9DTQYZR2M/UvZ1MdOhPezSDZcjj9uqQJOMqjLcpWtyW2iNINdlatQ== From 85b9e1ae608faaa202104bb9b59a5e7441442fe6 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Wed, 5 Jan 2022 12:35:31 +0000 Subject: [PATCH 07/13] permission-backend: use dataloader in PermissionIntegrationClient Signed-off-by: MT Lewis --- .../PermissionIntegrationClient.test.ts | 76 +++++++++++++++++++ .../service/PermissionIntegrationClient.ts | 56 +++++++------- 2 files changed, 102 insertions(+), 30 deletions(-) diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts index 4697277377..cc1de05127 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts @@ -484,5 +484,81 @@ describe('PermissionIntegrationClient', () => { expect(plugin1Router).toHaveBeenCalledTimes(1); expect(plugin2Router).toHaveBeenCalledTimes(1); }); + + it('leaves conditional results without resourceRefs 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'] }, + }, + { + id: '789', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceType: 'test-resource', + conditions: { + anyOf: [ + { rule: 'RULE_1', params: ['yes'] }, + { rule: 'RULE_2', 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 }, + { + id: '789', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceType: 'test-resource', + conditions: { + anyOf: [ + { rule: 'RULE_1', params: ['yes'] }, + { rule: 'RULE_2', params: ['yes'] }, + ], + }, + }, + ]); + + 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 644cac1b38..520e8b19ae 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.ts @@ -14,9 +14,10 @@ * limitations under the License. */ -import { groupBy, keyBy } from 'lodash'; +import { memoize } from 'lodash'; import fetch from 'node-fetch'; import { z } from 'zod'; +import DataLoader from 'dataloader'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { AuthorizeResponse, @@ -25,6 +26,7 @@ import { } from '@backstage/plugin-permission-common'; import { ApplyConditionsResponse, + ApplyConditionsResponseEntry, ConditionalPolicyDecision, PolicyDecision, } from '@backstage/plugin-permission-node'; @@ -38,12 +40,10 @@ const responseSchema = z.array( }), ); -export type ResourcePolicyDecision = Identified< - PolicyDecision & { resourceRef?: string } ->; - -type ConditionalResourcePolicyDecision = Identified< - ConditionalPolicyDecision & { resourceRef: string } +type ResourceDecision = Identified< + T & { + resourceRef?: string; + } >; export class PermissionIntegrationClient { @@ -54,23 +54,34 @@ export class PermissionIntegrationClient { } async applyConditions( - decisions: ResourcePolicyDecision[], + decisions: ResourceDecision[], authHeader?: string, ): Promise[]> { - const responses = await Promise.all( - this.groupRequestsByPluginId(decisions).map(([pluginId, requests]) => - this.makeRequest(pluginId, requests, authHeader), - ), + const loaderFor = memoize( + (pluginId: string) => + new DataLoader< + Identified, + ApplyConditionsResponseEntry + >(requests => this.makeRequest(pluginId, requests, authHeader)), ); - const responseIndex = keyBy(responses.flat(), 'id'); + return Promise.all( + decisions.map(decision => { + if ( + decision.result !== AuthorizeResult.CONDITIONAL || + !decision.resourceRef + ) { + return decision; + } - return decisions.map(decision => responseIndex[decision.id] ?? decision); + return loaderFor(decision.pluginId).load(decision); + }), + ); } private async makeRequest( pluginId: string, - decisions: ConditionalResourcePolicyDecision[], + decisions: readonly ResourceDecision[], authHeader?: string, ): Promise { const endpoint = `${await this.discovery.getBaseUrl( @@ -101,19 +112,4 @@ 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', - ), - ); - } } From ef291ff9856e87eeb21ea09289925a983d48a587 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Wed, 5 Jan 2022 18:34:48 +0000 Subject: [PATCH 08/13] permission-backend: move dataloader to router Signed-off-by: MT Lewis --- .../PermissionIntegrationClient.test.ts | 227 +-------- .../service/PermissionIntegrationClient.ts | 47 +- .../src/service/router.test.ts | 439 ++++++++++++++++-- .../permission-backend/src/service/router.ts | 83 ++-- 4 files changed, 463 insertions(+), 333 deletions(-) diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts index cc1de05127..814ccd3f38 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts @@ -77,11 +77,9 @@ describe('PermissionIntegrationClient', () => { }); it('should make a POST request to the correct endpoint', async () => { - await client.applyConditions([ + await client.applyConditions('plugin-1', [ { id: '123', - result: AuthorizeResult.CONDITIONAL, - pluginId: 'plugin-1', resourceRef: 'testResource1', resourceType: 'test-resource', conditions: mockConditions, @@ -92,11 +90,9 @@ describe('PermissionIntegrationClient', () => { }); it('should include a request body', async () => { - await client.applyConditions([ + await client.applyConditions('plugin-1', [ { id: '123', - result: AuthorizeResult.CONDITIONAL, - pluginId: 'plugin-1', resourceRef: 'testResource1', resourceType: 'test-resource', conditions: mockConditions, @@ -120,11 +116,9 @@ describe('PermissionIntegrationClient', () => { }); it('should return the response from the fetch request', async () => { - const response = await client.applyConditions([ + const response = await client.applyConditions('plugin-1', [ { id: '123', - result: AuthorizeResult.CONDITIONAL, - pluginId: 'plugin-1', resourceRef: 'testResource1', resourceType: 'test-resource', conditions: mockConditions, @@ -137,11 +131,9 @@ describe('PermissionIntegrationClient', () => { }); it('should not include authorization headers if no token is supplied', async () => { - await client.applyConditions([ + await client.applyConditions('plugin-1', [ { id: '123', - result: AuthorizeResult.CONDITIONAL, - pluginId: 'plugin-1', resourceRef: 'testResource1', resourceType: 'test-resource', conditions: mockConditions, @@ -154,11 +146,10 @@ describe('PermissionIntegrationClient', () => { it('should include correctly-constructed authorization header if token is supplied', async () => { await client.applyConditions( + 'plugin-1', [ { id: '123', - result: AuthorizeResult.CONDITIONAL, - pluginId: 'plugin-1', resourceRef: 'testResource1', resourceType: 'test-resource', conditions: mockConditions, @@ -179,11 +170,9 @@ describe('PermissionIntegrationClient', () => { ); await expect( - client.applyConditions([ + client.applyConditions('plugin-1', [ { id: '123', - result: AuthorizeResult.CONDITIONAL, - pluginId: 'plugin-1', resourceRef: 'testResource1', resourceType: 'test-resource', conditions: mockConditions, @@ -200,11 +189,9 @@ describe('PermissionIntegrationClient', () => { ); await expect( - client.applyConditions([ + client.applyConditions('plugin-1', [ { id: '123', - result: AuthorizeResult.CONDITIONAL, - pluginId: 'plugin-1', resourceRef: 'testResource1', resourceType: 'test-resource', conditions: mockConditions, @@ -227,27 +214,21 @@ describe('PermissionIntegrationClient', () => { ); await expect( - client.applyConditions([ + client.applyConditions('plugin-1', [ { 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, @@ -266,8 +247,7 @@ describe('PermissionIntegrationClient', () => { describe('integration with @backstage/plugin-permission-node', () => { let server: Server; let client: PermissionIntegrationClient; - let plugin1Router: RequestHandler; - let plugin2Router: RequestHandler; + let routerSpy: RequestHandler; beforeAll(async () => { const router = Router(); @@ -302,11 +282,9 @@ describe('PermissionIntegrationClient', () => { const app = express(); - plugin1Router = jest.fn(router); - plugin2Router = jest.fn(router); + routerSpy = jest.fn(router); - app.use('/plugin-1', plugin1Router); - app.use('/plugin-2', plugin2Router); + app.use('/plugin-1', routerSpy); await new Promise(resolve => { server = app.listen(resolve); @@ -341,11 +319,9 @@ describe('PermissionIntegrationClient', () => { it('works for simple conditions', async () => { await expect( - client.applyConditions([ + client.applyConditions('plugin-1', [ { id: '123', - result: AuthorizeResult.CONDITIONAL, - pluginId: 'plugin-1', resourceRef: 'testResource1', resourceType: 'test-resource', conditions: { rule: 'RULE_1', params: ['no'] }, @@ -356,11 +332,9 @@ describe('PermissionIntegrationClient', () => { it('works for complex criteria', async () => { await expect( - client.applyConditions([ + client.applyConditions('plugin-1', [ { id: '123', - result: AuthorizeResult.CONDITIONAL, - pluginId: 'plugin-1', resourceRef: 'testResource1', resourceType: 'test-resource', conditions: { @@ -385,180 +359,5 @@ describe('PermissionIntegrationClient', () => { ]), ).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); - }); - - it('leaves conditional results without resourceRefs 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'] }, - }, - { - id: '789', - result: AuthorizeResult.CONDITIONAL, - pluginId: 'plugin-2', - resourceType: 'test-resource', - conditions: { - anyOf: [ - { rule: 'RULE_1', params: ['yes'] }, - { rule: 'RULE_2', 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 }, - { - id: '789', - result: AuthorizeResult.CONDITIONAL, - pluginId: 'plugin-2', - resourceType: 'test-resource', - conditions: { - anyOf: [ - { rule: 'RULE_1', params: ['yes'] }, - { rule: 'RULE_2', params: ['yes'] }, - ], - }, - }, - ]); - - 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 520e8b19ae..f4fdb6cbfc 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.ts @@ -14,21 +14,14 @@ * limitations under the License. */ -import { memoize } from 'lodash'; import fetch from 'node-fetch'; import { z } from 'zod'; -import DataLoader from 'dataloader'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; +import { AuthorizeResult } from '@backstage/plugin-permission-common'; import { - AuthorizeResponse, - AuthorizeResult, - Identified, -} from '@backstage/plugin-permission-common'; -import { + ApplyConditionsRequestEntry, ApplyConditionsResponse, - ApplyConditionsResponseEntry, ConditionalPolicyDecision, - PolicyDecision, } from '@backstage/plugin-permission-node'; const responseSchema = z.array( @@ -40,11 +33,9 @@ const responseSchema = z.array( }), ); -type ResourceDecision = Identified< - T & { - resourceRef?: string; - } ->; +export type ResourcePolicyDecision = ConditionalPolicyDecision & { + resourceRef: string; +}; export class PermissionIntegrationClient { private readonly discovery: PluginEndpointDiscovery; @@ -54,34 +45,8 @@ export class PermissionIntegrationClient { } async applyConditions( - decisions: ResourceDecision[], - authHeader?: string, - ): Promise[]> { - const loaderFor = memoize( - (pluginId: string) => - new DataLoader< - Identified, - ApplyConditionsResponseEntry - >(requests => this.makeRequest(pluginId, requests, authHeader)), - ); - - return Promise.all( - decisions.map(decision => { - if ( - decision.result !== AuthorizeResult.CONDITIONAL || - !decision.resourceRef - ) { - return decision; - } - - return loaderFor(decision.pluginId).load(decision); - }), - ); - } - - private async makeRequest( pluginId: string, - decisions: readonly ResourceDecision[], + decisions: readonly ApplyConditionsRequestEntry[], authHeader?: string, ): Promise { const endpoint = `${await this.discovery.getBaseUrl( diff --git a/plugins/permission-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index ef165b2ac5..971cb6a223 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -19,32 +19,28 @@ 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 { ApplyConditionsResponseEntry } from '@backstage/plugin-permission-node'; +import { + ApplyConditionsRequestEntry, + ApplyConditionsResponseEntry, +} from '@backstage/plugin-permission-node'; import { PermissionIntegrationClient } from './PermissionIntegrationClient'; import { createRouter } from './router'; const mockApplyConditions: jest.MockedFunction< InstanceType['applyConditions'] -> = 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.fn( + async ( + _pluginId: string, + decisions: readonly ApplyConditionsRequestEntry[], + ) => + decisions.map(decision => ({ + id: decision.id, + result: + (decision.conditions as any).params[0] === 'yes' + ? (AuthorizeResult.ALLOW as const) + : (AuthorizeResult.DENY as const), + })), ); jest.mock('./PermissionIntegrationClient', () => ({ @@ -90,6 +86,10 @@ describe('createRouter', () => { app = express().use(router); }); + afterEach(() => { + jest.clearAllMocks(); + }); + describe('GET /health', () => { it('returns ok', async () => { const response = await request(app).get('/health'); @@ -178,18 +178,14 @@ describe('createRouter', () => { }); describe('conditional policy result', () => { - beforeEach(() => { - policy.handle.mockResolvedValue({ + it('returns conditions if no resourceRef is supplied', async () => { + policy.handle.mockResolvedValueOnce({ result: AuthorizeResult.CONDITIONAL, pluginId: 'test-plugin', resourceType: 'test-resource-1', - conditions: { - anyOf: [{ rule: 'test-rule', params: ['abc'] }], - }, + conditions: { rule: 'test-rule', params: ['abc'] }, }); - }); - it('returns conditions if no resourceRef is supplied', async () => { const response = await request(app) .post('/authorize') .send([ @@ -208,17 +204,388 @@ describe('createRouter', () => { { id: '123', result: AuthorizeResult.CONDITIONAL, - conditions: { anyOf: [{ rule: 'test-rule', params: ['abc'] }] }, + pluginId: 'test-plugin', + resourceType: 'test-resource-1', + conditions: { rule: 'test-rule', params: ['abc'] }, }, ]); }); - it.each([ - AuthorizeResult.ALLOW, - AuthorizeResult.DENY, + it('makes separate batched requests to multiple plugin backends', async () => { + policy.handle + .mockResolvedValueOnce({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceType: 'test-resource-1', + conditions: { rule: 'test-rule', params: ['yes'] }, + }) + .mockResolvedValueOnce({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceType: 'test-resource-2', + conditions: { rule: 'test-rule', params: ['yes'] }, + }) + .mockResolvedValueOnce({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceType: 'test-resource-1', + conditions: { rule: 'test-rule', params: ['no'] }, + }) + .mockResolvedValueOnce({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceType: 'test-resource-2', + conditions: { rule: 'test-rule', params: ['no'] }, + }); + + const response = await request(app) + .post('/authorize') + .auth('test-token', { type: 'bearer' }) + .send([ + { + id: '123', + permission: { + name: 'test.permission.1', + resourceType: 'test-resource-1', + attributes: {}, + }, + resourceRef: 'resource:1', + }, + { + id: '234', + permission: { + name: 'test.permission.2', + resourceType: 'test-resource-2', + attributes: {}, + }, + resourceRef: 'resource:2', + }, + { + id: '345', + permission: { + name: 'test.permission.3', + resourceType: 'test-resource-1', + attributes: {}, + }, + resourceRef: 'resource:3', + }, + { + id: '456', + permission: { + name: 'test.permission.4', + resourceType: 'test-resource-2', + attributes: {}, + }, + resourceRef: 'resource:4', + }, + ]); + + expect(mockApplyConditions).toHaveBeenCalledWith( + 'plugin-1', + [ + expect.objectContaining({ + id: '123', + resourceType: 'test-resource-1', + resourceRef: 'resource:1', + conditions: { rule: 'test-rule', params: ['yes'] }, + }), + expect.objectContaining({ + id: '345', + resourceType: 'test-resource-1', + resourceRef: 'resource:3', + conditions: { rule: 'test-rule', params: ['no'] }, + }), + ], + 'Bearer test-token', + ); + + expect(mockApplyConditions).toHaveBeenCalledWith( + 'plugin-2', + [ + expect.objectContaining({ + id: '234', + resourceType: 'test-resource-2', + resourceRef: 'resource:2', + conditions: { rule: 'test-rule', params: ['yes'] }, + }), + expect.objectContaining({ + id: '456', + resourceType: 'test-resource-2', + resourceRef: 'resource:4', + conditions: { rule: 'test-rule', params: ['no'] }, + }), + ], + 'Bearer test-token', + ); + + expect(response.status).toEqual(200); + expect(response.body).toEqual([ + { id: '123', result: AuthorizeResult.ALLOW }, + { id: '234', result: AuthorizeResult.ALLOW }, + { id: '345', result: AuthorizeResult.DENY }, + { id: '456', result: AuthorizeResult.DENY }, + ]); + }); + + it('leaves definitive results unchanged', async () => { + policy.handle + .mockResolvedValueOnce({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceType: 'test-resource-1', + conditions: { rule: 'test-rule', params: ['no'] }, + }) + .mockResolvedValueOnce({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceType: 'test-resource-2', + conditions: { rule: 'test-rule', params: ['no'] }, + }) + .mockResolvedValueOnce({ + result: AuthorizeResult.ALLOW, + }) + .mockResolvedValueOnce({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceType: 'test-resource-1', + conditions: { rule: 'test-rule', params: ['yes'] }, + }) + .mockResolvedValueOnce({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceType: 'test-resource-2', + conditions: { rule: 'test-rule', params: ['yes'] }, + }) + .mockResolvedValueOnce({ + result: AuthorizeResult.DENY, + }); + + const response = await request(app) + .post('/authorize') + .auth('test-token', { type: 'bearer' }) + .send([ + { + id: '123', + permission: { + name: 'test.permission.1', + resourceType: 'test-resource-1', + attributes: {}, + }, + resourceRef: 'resource:1', + }, + { + id: '234', + permission: { + name: 'test.permission.2', + resourceType: 'test-resource-2', + attributes: {}, + }, + resourceRef: 'resource:2', + }, + { + id: '345', + permission: { + name: 'test.permission.3', + resourceType: 'test-resource-1', + attributes: {}, + }, + resourceRef: 'resource:3', + }, + { + id: '456', + permission: { + name: 'test.permission.4', + resourceType: 'test-resource-1', + attributes: {}, + }, + resourceRef: 'resource:4', + }, + { + id: '567', + permission: { + name: 'test.permission.5', + resourceType: 'test-resource-2', + attributes: {}, + }, + resourceRef: 'resource:5', + }, + { + id: '678', + permission: { + name: 'test.permission.6', + attributes: {}, + }, + }, + ]); + + expect(mockApplyConditions).toHaveBeenCalledWith( + 'plugin-1', + [ + expect.objectContaining({ + id: '123', + resourceType: 'test-resource-1', + resourceRef: 'resource:1', + conditions: { rule: 'test-rule', params: ['no'] }, + }), + expect.objectContaining({ + id: '456', + resourceType: 'test-resource-1', + resourceRef: 'resource:4', + conditions: { rule: 'test-rule', params: ['yes'] }, + }), + ], + 'Bearer test-token', + ); + + expect(mockApplyConditions).toHaveBeenCalledWith( + 'plugin-2', + [ + expect.objectContaining({ + id: '234', + resourceType: 'test-resource-2', + resourceRef: 'resource:2', + conditions: { rule: 'test-rule', params: ['no'] }, + }), + expect.objectContaining({ + id: '567', + resourceType: 'test-resource-2', + resourceRef: 'resource:5', + conditions: { rule: 'test-rule', params: ['yes'] }, + }), + ], + 'Bearer test-token', + ); + + expect(response.status).toEqual(200); + expect(response.body).toEqual([ + { id: '123', result: AuthorizeResult.DENY }, + { id: '234', result: AuthorizeResult.DENY }, + { id: '345', result: AuthorizeResult.ALLOW }, + { id: '456', result: AuthorizeResult.ALLOW }, + { id: '567', result: AuthorizeResult.ALLOW }, + { id: '678', result: AuthorizeResult.DENY }, + ]); + }); + + it('leaves conditional results without resourceRefs unchanged', async () => { + policy.handle + .mockResolvedValueOnce({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceType: 'test-resource-1', + conditions: { rule: 'test-rule', params: ['yes'] }, + }) + .mockResolvedValueOnce({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceType: 'test-resource-2', + conditions: { rule: 'test-rule', params: ['yes'] }, + }) + .mockResolvedValueOnce({ + result: AuthorizeResult.ALLOW, + }) + .mockResolvedValueOnce({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceType: 'test-resource-1', + conditions: { rule: 'test-rule', params: ['abc'] }, + }); + + const response = await request(app) + .post('/authorize') + .auth('test-token', { type: 'bearer' }) + .send([ + { + id: '123', + permission: { + name: 'test.permission.1', + resourceType: 'test-resource-1', + attributes: {}, + }, + resourceRef: 'resource:1', + }, + { + id: '234', + permission: { + name: 'test.permission.2', + resourceType: 'test-resource-2', + attributes: {}, + }, + resourceRef: 'resource:2', + }, + { + id: '345', + permission: { + name: 'test.permission.3', + resourceType: 'test-resource-1', + attributes: {}, + }, + resourceRef: 'resource:3', + }, + { + id: '456', + permission: { + name: 'test.permission.4', + resourceType: 'test-resource-1', + attributes: {}, + }, + }, + ]); + + expect(mockApplyConditions).toHaveBeenCalledWith( + 'plugin-1', + [ + expect.objectContaining({ + id: '123', + resourceType: 'test-resource-1', + resourceRef: 'resource:1', + conditions: { rule: 'test-rule', params: ['yes'] }, + }), + ], + 'Bearer test-token', + ); + + expect(mockApplyConditions).toHaveBeenCalledWith( + 'plugin-2', + [ + expect.objectContaining({ + id: '234', + resourceType: 'test-resource-2', + resourceRef: 'resource:2', + conditions: { rule: 'test-rule', params: ['yes'] }, + }), + ], + 'Bearer test-token', + ); + + expect(response.status).toEqual(200); + expect(response.body).toEqual([ + { id: '123', result: AuthorizeResult.ALLOW }, + { id: '234', result: AuthorizeResult.ALLOW }, + { id: '345', result: AuthorizeResult.ALLOW }, + { + id: '456', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceType: 'test-resource-1', + conditions: { rule: 'test-rule', params: ['abc'] }, + }, + ]); + }); + + it.each<[ApplyConditionsResponseEntry['result'], string]>([ + [AuthorizeResult.ALLOW, 'yes'], + [AuthorizeResult.DENY, 'no'], ])( 'applies conditions and returns %s if resourceRef is supplied', - async result => { + async (result, params) => { + policy.handle.mockResolvedValue({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'test-plugin', + resourceType: 'test-resource-1', + conditions: { rule: 'test-rule', params }, + }); + mockApplyConditions.mockResolvedValueOnce([ { id: '123', @@ -255,21 +622,19 @@ describe('createRouter', () => { ]); expect(mockApplyConditions).toHaveBeenCalledWith( + 'test-plugin', [ expect.objectContaining({ id: '123', - result: AuthorizeResult.CONDITIONAL, - pluginId: 'test-plugin', resourceType: 'test-resource-1', resourceRef: 'test/resource', - conditions: { anyOf: [{ rule: 'test-rule', params: ['abc'] }] }, + conditions: { rule: 'test-rule', params }, }), expect.objectContaining({ id: '234', - pluginId: 'test-plugin', resourceType: 'test-resource-1', resourceRef: 'test/resource', - conditions: { anyOf: [{ rule: 'test-rule', params: ['abc'] }] }, + conditions: { rule: 'test-rule', params }, }), ], 'Bearer test-token', diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index 0e877bc816..58438afbb5 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -34,10 +34,13 @@ import { Identified, } from '@backstage/plugin-permission-common'; import { + ApplyConditionsRequestEntry, + ApplyConditionsResponseEntry, PermissionPolicy, - PolicyDecision, } from '@backstage/plugin-permission-node'; import { PermissionIntegrationClient } from './PermissionIntegrationClient'; +import { memoize } from 'lodash'; +import DataLoader from 'dataloader'; const requestSchema: z.ZodSchema[]> = z.array( z.object({ @@ -73,43 +76,6 @@ export interface RouterOptions { identity: IdentityClient; } -const applyPolicy = async ( - policy: PermissionPolicy, - 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]; - - // Sanity check that any resource provided matches the one expected by the permission - 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}`, - ); - } - }); -}; - const handleRequest = async ( requests: Identified[], user: BackstageIdentityResponse | undefined, @@ -117,11 +83,46 @@ const handleRequest = async ( permissionIntegrationClient: PermissionIntegrationClient, authHeader?: string, ): Promise[]> => { - const decisions = await applyPolicy(policy, requests, user); + const applyConditionsLoaderFor = memoize((pluginId: string) => { + return new DataLoader< + ApplyConditionsRequestEntry, + ApplyConditionsResponseEntry + >(batch => + permissionIntegrationClient.applyConditions(pluginId, batch, authHeader), + ); + }); - assertMatchingResourceTypes(requests, decisions); + return Promise.all( + requests.map(({ id, resourceRef, ...request }) => + policy.handle(request, user).then(decision => { + if (decision.result !== AuthorizeResult.CONDITIONAL) { + return { + id, + ...decision, + }; + } - return permissionIntegrationClient.applyConditions(decisions, authHeader); + 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, + }); + }), + ), + ); }; /** From 1fb2e0e0b4e2ab5bd91be82310a641b7d8a2b61d Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Thu, 6 Jan 2022 12:18:01 +0000 Subject: [PATCH 09/13] permission-node: wrap request and response arrays in object Signed-off-by: MT Lewis --- .../PermissionIntegrationClient.test.ts | 38 +-- .../service/PermissionIntegrationClient.ts | 44 ++-- plugins/permission-node/api-report.md | 8 +- .../createPermissionIntegrationRouter.test.ts | 217 ++++++++++-------- .../createPermissionIntegrationRouter.ts | 40 ++-- 5 files changed, 196 insertions(+), 151 deletions(-) diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts index 814ccd3f38..4be1b55379 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts @@ -39,7 +39,9 @@ describe('PermissionIntegrationClient', () => { const mockApplyConditionsHandler = jest.fn( (_req, res, { json }: RestContext) => { - return res(json([{ id: '123', result: AuthorizeResult.ALLOW }])); + return res( + json({ items: [{ id: '123', result: AuthorizeResult.ALLOW }] }), + ); }, ); @@ -101,14 +103,16 @@ describe('PermissionIntegrationClient', () => { expect(mockApplyConditionsHandler).toHaveBeenCalledWith( expect.objectContaining({ - body: [ - { - id: '123', - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: mockConditions, - }, - ], + body: { + items: [ + { + id: '123', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ], + }, }), expect.anything(), expect.anything(), @@ -184,7 +188,9 @@ describe('PermissionIntegrationClient', () => { it('should reject invalid responses', async () => { mockApplyConditionsHandler.mockImplementationOnce( (_req, res, { json }: RestContext) => { - return res(json([{ id: '123', outcome: AuthorizeResult.ALLOW }])); + return res( + json({ items: [{ id: '123', outcome: AuthorizeResult.ALLOW }] }), + ); }, ); @@ -204,11 +210,13 @@ describe('PermissionIntegrationClient', () => { mockApplyConditionsHandler.mockImplementationOnce( (_req, res, { json }: RestContext) => { return res( - json([ - { id: '123', result: AuthorizeResult.ALLOW }, - { id: '456', result: AuthorizeResult.DENY }, - { id: '789', result: AuthorizeResult.ALLOW }, - ]), + json({ + items: [ + { id: '123', result: AuthorizeResult.ALLOW }, + { id: '456', result: AuthorizeResult.DENY }, + { id: '789', result: AuthorizeResult.ALLOW }, + ], + }), ); }, ); diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.ts index f4fdb6cbfc..2b2161ee71 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.ts @@ -20,18 +20,20 @@ import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { AuthorizeResult } from '@backstage/plugin-permission-common'; import { ApplyConditionsRequestEntry, - ApplyConditionsResponse, + ApplyConditionsResponseEntry, ConditionalPolicyDecision, } from '@backstage/plugin-permission-node'; -const responseSchema = z.array( - z.object({ - id: z.string(), - result: z - .literal(AuthorizeResult.ALLOW) - .or(z.literal(AuthorizeResult.DENY)), - }), -); +const responseSchema = z.object({ + items: z.array( + z.object({ + id: z.string(), + result: z + .literal(AuthorizeResult.ALLOW) + .or(z.literal(AuthorizeResult.DENY)), + }), + ), +}); export type ResourcePolicyDecision = ConditionalPolicyDecision & { resourceRef: string; @@ -48,21 +50,23 @@ export class PermissionIntegrationClient { pluginId: string, decisions: readonly ApplyConditionsRequestEntry[], authHeader?: string, - ): Promise { + ): Promise { const endpoint = `${await this.discovery.getBaseUrl( pluginId, )}/.well-known/backstage/permissions/apply-conditions`; const response = await fetch(endpoint, { method: 'POST', - body: JSON.stringify( - decisions.map(({ id, resourceRef, resourceType, conditions }) => ({ - id, - resourceRef, - resourceType, - conditions, - })), - ), + body: JSON.stringify({ + items: decisions.map( + ({ id, resourceRef, resourceType, conditions }) => ({ + id, + resourceRef, + resourceType, + conditions, + }), + ), + }), headers: { ...(authHeader ? { authorization: authHeader } : {}), 'content-type': 'application/json', @@ -75,6 +79,8 @@ export class PermissionIntegrationClient { ); } - return responseSchema.parse(await response.json()); + const result = responseSchema.parse(await response.json()); + + return result.items; } } diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 7023369306..d9ea64483e 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -18,7 +18,9 @@ import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { TokenManager } from '@backstage/backend-common'; // @public -export type ApplyConditionsRequest = ApplyConditionsRequestEntry[]; +export type ApplyConditionsRequest = { + items: ApplyConditionsRequestEntry[]; +}; // @public export type ApplyConditionsRequestEntry = Identified<{ @@ -28,7 +30,9 @@ export type ApplyConditionsRequestEntry = Identified<{ }>; // @public -export type ApplyConditionsResponse = ApplyConditionsResponseEntry[]; +export type ApplyConditionsResponse = { + items: ApplyConditionsResponseEntry[]; +}; // @public export type ApplyConditionsResponseEntry = Identified; diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 2e911b7cae..79bfcb3b0b 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -96,22 +96,26 @@ describe('createPermissionIntegrationRouter', () => { ])('returns 200/ALLOW when criteria match (case %#)', async conditions => { const response = await request(app) .post('/.well-known/backstage/permissions/apply-conditions') - .send([ - { - id: '123', - resourceRef: 'default:test/resource', - resourceType: 'test-resource', - conditions, - }, - ]); + .send({ + items: [ + { + id: '123', + resourceRef: 'default:test/resource', + resourceType: 'test-resource', + conditions, + }, + ], + }); expect(response.status).toEqual(200); - expect(response.body).toEqual([ - { - id: '123', - result: AuthorizeResult.ALLOW, - }, - ]); + expect(response.body).toEqual({ + items: [ + { + id: '123', + result: AuthorizeResult.ALLOW, + }, + ], + }); }); it.each([ @@ -145,19 +149,21 @@ describe('createPermissionIntegrationRouter', () => { async conditions => { const response = await request(app) .post('/.well-known/backstage/permissions/apply-conditions') - .send([ - { - id: '123', - resourceRef: 'default:test/resource', - resourceType: 'test-resource', - conditions, - }, - ]); + .send({ + items: [ + { + id: '123', + resourceRef: 'default:test/resource', + resourceType: 'test-resource', + conditions, + }, + ], + }); expect(response.status).toEqual(200); - expect(response.body).toEqual([ - { id: '123', result: AuthorizeResult.DENY }, - ]); + expect(response.body).toEqual({ + items: [{ id: '123', result: AuthorizeResult.DENY }], + }); }, ); @@ -167,54 +173,58 @@ describe('createPermissionIntegrationRouter', () => { 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: [] }, - ], + .send({ + items: [ + { + 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 }, - ]); + expect(response.body).toEqual({ + items: [ + { 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('calls getResources for all required resources at once', () => { @@ -230,16 +240,18 @@ describe('createPermissionIntegrationRouter', () => { it('returns 400 when called with incorrect resource type', async () => { const response = await request(app) .post('/.well-known/backstage/permissions/apply-conditions') - .send([ - { - id: '123', - resourceRef: 'default:test/resource', - resourceType: 'test-incorrect-resource', - conditions: { - anyOf: [], + .send({ + items: [ + { + id: '123', + resourceRef: 'default:test/resource', + resourceType: 'test-incorrect-resource', + conditions: { + anyOf: [], + }, }, - }, - ]); + ], + }); expect(response.status).toEqual(400); expect(response.error && response.error.text).toMatch( @@ -254,22 +266,26 @@ describe('createPermissionIntegrationRouter', () => { const response = await request(app) .post('/.well-known/backstage/permissions/apply-conditions') - .send([ - { - id: '123', - resourceRef: 'default:test/resource', - resourceType: 'test-resource', - conditions: { rule: 'testRule1', params: [] }, - }, - ]); + .send({ + items: [ + { + id: '123', + resourceRef: 'default:test/resource', + resourceType: 'test-resource', + conditions: { rule: 'test-rule-1', params: [] }, + }, + ], + }); expect(response.status).toEqual(200); - expect(response.body).toEqual([ - { - id: '123', - result: AuthorizeResult.DENY, - }, - ]); + expect(response.body).toEqual({ + items: [ + { + id: '123', + result: AuthorizeResult.DENY, + }, + ], + }); }); it.each([ @@ -278,14 +294,17 @@ describe('createPermissionIntegrationRouter', () => { {}, { resourceType: 'test-resource-type' }, [{ resourceType: 'test-resource-type' }], - [{ resourceRef: 'test/resource-ref' }], - [ - { - resourceType: 'test-resource-type', - resourceRef: 'test/resource-ref', - }, - ], - [{ conditions: { anyOf: [] } }], + { items: [{ resourceType: 'test-resource-type' }] }, + { items: [{ resourceRef: 'test/resource-ref' }] }, + { + items: [ + { + resourceType: 'test-resource-type', + resourceRef: 'test/resource-ref', + }, + ], + }, + { items: [{ conditions: { anyOf: [] } }] }, ])(`returns 400 for invalid input %#`, async input => { const response = await request(app) .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 747e47b87d..4968db6100 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -48,14 +48,16 @@ const permissionCriteriaSchema: z.ZodSchema< ]), ); -const applyConditionsRequestSchema = z.array( - z.object({ - id: z.string(), - resourceRef: z.string(), - resourceType: z.string(), - conditions: permissionCriteriaSchema, - }), -); +const applyConditionsRequestSchema = z.object({ + items: 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 @@ -74,7 +76,9 @@ export type ApplyConditionsRequestEntry = Identified<{ * * @public */ -export type ApplyConditionsRequest = ApplyConditionsRequestEntry[]; +export type ApplyConditionsRequest = { + items: ApplyConditionsRequestEntry[]; +}; /** * The result of applying the conditions, expressed as a definitive authorize @@ -89,7 +93,9 @@ export type ApplyConditionsResponseEntry = Identified; * * @public */ -export type ApplyConditionsResponse = ApplyConditionsResponseEntry[]; +export type ApplyConditionsResponse = { + items: ApplyConditionsResponseEntry[]; +}; const applyConditions = ( criteria: PermissionCriteria, @@ -165,7 +171,9 @@ export const createPermissionIntegrationRouter = (options: { const getRule = createGetRule(rules); - const assertValidResourceTypes = (requests: ApplyConditionsRequest) => { + const assertValidResourceTypes = ( + requests: ApplyConditionsRequestEntry[], + ) => { const invalidResourceType = requests.find( request => request.resourceType !== resourceType, )?.resourceType; @@ -188,10 +196,10 @@ export const createPermissionIntegrationRouter = (options: { const body = parseResult.data; - assertValidResourceTypes(body); + assertValidResourceTypes(body.items); const resourceRefs = Array.from( - new Set(body.map(({ resourceRef }) => resourceRef)), + new Set(body.items.map(({ resourceRef }) => resourceRef)), ); const resourceArray = await getResources(resourceRefs); const resources = resourceRefs.reduce((acc, resourceRef, index) => { @@ -200,8 +208,8 @@ export const createPermissionIntegrationRouter = (options: { return acc; }, {} as Record); - return res.status(200).json( - body.map(request => ({ + return res.status(200).json({ + items: body.items.map(request => ({ id: request.id, result: applyConditions( request.conditions, @@ -211,7 +219,7 @@ export const createPermissionIntegrationRouter = (options: { ? AuthorizeResult.ALLOW : AuthorizeResult.DENY, })), - ); + }); }, ); From 34a4be296f5b3dd7d3185ead2ef35d258cd00950 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Thu, 13 Jan 2022 10:52:05 +0000 Subject: [PATCH 10/13] permission-node: list all incorrect resource types in apply-conditions handler Signed-off-by: MT Lewis --- .../createPermissionIntegrationRouter.test.ts | 22 ++++++++++++++++--- .../createPermissionIntegrationRouter.ts | 12 +++++----- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index 79bfcb3b0b..be422c87d2 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -244,8 +244,24 @@ describe('createPermissionIntegrationRouter', () => { items: [ { id: '123', - resourceRef: 'default:test/resource', - resourceType: 'test-incorrect-resource', + resourceRef: 'default:test/resource-1', + resourceType: 'test-incorrect-resource-1', + conditions: { + anyOf: [], + }, + }, + { + id: '234', + resourceRef: 'default:test/resource-2', + resourceType: 'test-resource', + conditions: { + anyOf: [], + }, + }, + { + id: '345', + resourceRef: 'default:test/resource-3', + resourceType: 'test-incorrect-resource-2', conditions: { anyOf: [], }, @@ -255,7 +271,7 @@ describe('createPermissionIntegrationRouter', () => { expect(response.status).toEqual(400); expect(response.error && response.error.text).toMatch( - /unexpected resource type: test-incorrect-resource/i, + /unexpected resource types: test-incorrect-resource-1, test-incorrect-resource-2/i, ); }); diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts index 4968db6100..1b01660fb0 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts @@ -174,12 +174,14 @@ export const createPermissionIntegrationRouter = (options: { const assertValidResourceTypes = ( requests: ApplyConditionsRequestEntry[], ) => { - const invalidResourceType = requests.find( - request => request.resourceType !== resourceType, - )?.resourceType; + const invalidResourceTypes = requests + .filter(request => request.resourceType !== resourceType) + .map(request => request.resourceType); - if (invalidResourceType) { - throw new InputError(`Unexpected resource type: ${invalidResourceType}.`); + if (invalidResourceTypes.length) { + throw new InputError( + `Unexpected resource types: ${invalidResourceTypes.join(', ')}.`, + ); } }; From 3bb0afb54c3865ea843aa0091dfbe1f4645d825d Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Thu, 13 Jan 2022 13:04:30 +0000 Subject: [PATCH 11/13] permission-node: add test for apply conditions router Signed-off-by: MT Lewis --- .../createPermissionIntegrationRouter.test.ts | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts index be422c87d2..065c30a978 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -304,6 +304,59 @@ describe('createPermissionIntegrationRouter', () => { }); }); + it('interleaves responses for present and missing resources', async () => { + mockGetResources.mockImplementationOnce(async resourceRefs => + resourceRefs.map(resourceRef => + resourceRef === 'default:test/missing-resource' + ? undefined + : { id: resourceRef }, + ), + ); + + const response = await request(app) + .post('/.well-known/backstage/permissions/apply-conditions') + .send({ + items: [ + { + id: '123', + resourceRef: 'default:test/resource-1', + resourceType: 'test-resource', + conditions: { rule: 'test-rule-1', params: [] }, + }, + { + id: '234', + resourceRef: 'default:test/missing-resource', + resourceType: 'test-resource', + conditions: { rule: 'test-rule-1', params: [] }, + }, + { + id: '345', + resourceRef: 'default:test/resource-2', + resourceType: 'test-resource', + conditions: { rule: 'test-rule-1', params: [] }, + }, + ], + }); + + expect(response.status).toEqual(200); + expect(response.body).toEqual({ + items: [ + { + id: '123', + result: AuthorizeResult.ALLOW, + }, + { + id: '234', + result: AuthorizeResult.DENY, + }, + { + id: '345', + result: AuthorizeResult.ALLOW, + }, + ], + }); + }); + it.each([ undefined, '', From 74967cf50d48f19bb9520df9659bd3209bf34f62 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Thu, 13 Jan 2022 13:04:47 +0000 Subject: [PATCH 12/13] catalog-backend: update permission integration to support batching Signed-off-by: MT Lewis --- .../src/service/NextCatalogBuilder.ts | 30 ++++--- .../src/service/NextRouter.test.ts | 87 +++++++++++++++++++ 2 files changed, 107 insertions(+), 10 deletions(-) diff --git a/plugins/catalog-backend/src/service/NextCatalogBuilder.ts b/plugins/catalog-backend/src/service/NextCatalogBuilder.ts index 391c89d83d..be75b4dfe7 100644 --- a/plugins/catalog-backend/src/service/NextCatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/NextCatalogBuilder.ts @@ -25,6 +25,7 @@ import { NoForeignRootFieldsEntityPolicy, parseEntityRef, SchemaValidEntityPolicy, + stringifyEntityRef, Validators, } from '@backstage/catalog-model'; import { @@ -34,7 +35,7 @@ import { } from '@backstage/integration'; import { createHash } from 'crypto'; import { Router } from 'express'; -import lodash from 'lodash'; +import lodash, { keyBy } from 'lodash'; import { EntitiesCatalog, EntitiesSearchFilter } from '../catalog'; import { DatabaseLocationsCatalog, @@ -415,18 +416,27 @@ export class NextCatalogBuilder { ); const permissionIntegrationRouter = createPermissionIntegrationRouter({ resourceType: RESOURCE_TYPE_CATALOG_ENTITY, - getResource: async (resourceRef: string) => { - const parsed = parseEntityRef(resourceRef); + getResources: async (resourceRefs: string[]) => { + const { entities } = await entitiesCatalog.entities({ + filter: { + anyOf: resourceRefs.map(resourceRef => { + const { kind, namespace, name } = parseEntityRef(resourceRef); - const { entities } = await unauthorizedEntitiesCatalog.entities({ - filter: basicEntityFilter({ - kind: parsed.kind, - 'metadata.namespace': parsed.namespace, - 'metadata.name': parsed.name, - }), + return basicEntityFilter({ + kind, + 'metadata.namespace': namespace, + 'metadata.name': name, + }); + }), + }, }); - return entities[0]; + const entitiesByRef = keyBy(entities, stringifyEntityRef); + + return resourceRefs.map( + resourceRef => + entitiesByRef[stringifyEntityRef(parseEntityRef(resourceRef))], + ); }, rules: this.permissionRules, }); diff --git a/plugins/catalog-backend/src/service/NextRouter.test.ts b/plugins/catalog-backend/src/service/NextRouter.test.ts index 84580d0ca4..b79fba4a56 100644 --- a/plugins/catalog-backend/src/service/NextRouter.test.ts +++ b/plugins/catalog-backend/src/service/NextRouter.test.ts @@ -24,6 +24,9 @@ import { EntitiesCatalog } from '../catalog'; import { LocationService, RefreshService } from './types'; import { basicEntityFilter } from './request'; import { createNextRouter } from './NextRouter'; +import { AuthorizeResult } from '@backstage/plugin-permission-common'; +import { createPermissionIntegrationRouter } from '@backstage/plugin-permission-node'; +import { RESOURCE_TYPE_CATALOG_ENTITY } from '@backstage/plugin-catalog-common'; describe('createNextRouter readonly disabled', () => { let entitiesCatalog: jest.Mocked; @@ -433,3 +436,87 @@ describe('createNextRouter readonly enabled', () => { }); }); }); + +describe('NextRouter permissioning', () => { + let entitiesCatalog: jest.Mocked; + let locationService: jest.Mocked; + let app: express.Express; + let refreshService: RefreshService; + + const fakeRule = { + name: 'FAKE_RULE', + description: 'fake rule', + apply: () => true, + toQuery: () => ({ key: '', values: [] }), + }; + + beforeAll(async () => { + entitiesCatalog = { + entities: jest.fn(), + removeEntityByUid: jest.fn(), + batchAddOrUpdateEntities: jest.fn(), + entityAncestry: jest.fn(), + }; + locationService = { + getLocation: jest.fn(), + createLocation: jest.fn(), + listLocations: jest.fn(), + deleteLocation: jest.fn(), + }; + refreshService = { refresh: jest.fn() }; + const router = await createNextRouter({ + entitiesCatalog, + locationService, + logger: getVoidLogger(), + refreshService, + config: new ConfigReader(undefined), + permissionIntegrationRouter: createPermissionIntegrationRouter({ + resourceType: RESOURCE_TYPE_CATALOG_ENTITY, + rules: [fakeRule], + getResources: jest.fn((resourceRefs: string[]) => + Promise.resolve( + resourceRefs.map(resourceRef => ({ id: resourceRef })), + ), + ), + }), + }); + app = express().use(router); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + it('accepts and evaluates conditions at the apply-conditions endpoint', async () => { + const spideySense: Entity = { + apiVersion: 'a', + kind: 'component', + metadata: { + name: 'spidey-sense', + }, + }; + entitiesCatalog.entities.mockResolvedValueOnce({ + entities: [spideySense], + pageInfo: { hasNextPage: false }, + }); + + const requestBody = { + items: [ + { + id: '123', + resourceType: 'catalog-entity', + resourceRef: 'component:default/spidey-sense', + conditions: { rule: 'FAKE_RULE', params: ['user:default/spiderman'] }, + }, + ], + }; + const response = await request(app) + .post('/.well-known/backstage/permissions/apply-conditions') + .send(requestBody); + + expect(response.status).toBe(200); + expect(response.body).toEqual({ + items: [{ id: '123', result: AuthorizeResult.ALLOW }], + }); + }); +}); From ba2dbe47d736b43c174098df4f9b6a71a2737d41 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Thu, 13 Jan 2022 13:28:51 +0000 Subject: [PATCH 13/13] permission-node: regenerate api-report Signed-off-by: MT Lewis --- plugins/permission-node/api-report.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index d9ea64483e..71685c59b1 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -104,11 +104,6 @@ export const createPermissionIntegrationRouter: (options: { getResources: (resourceRefs: string[]) => Promise<(TResource | undefined)[]>; }) => express.Router; -// @public -export type DefinitivePolicyDecision = { - result: AuthorizeResult.ALLOW | AuthorizeResult.DENY; -}; - // @public export const createPermissionRule: < TResource, @@ -118,6 +113,11 @@ export const createPermissionRule: < rule: PermissionRule, ) => PermissionRule; +// @public +export type DefinitivePolicyDecision = { + result: AuthorizeResult.ALLOW | AuthorizeResult.DENY; +}; + // @public export const makeCreatePermissionRule: () => < TParams extends unknown[],