diff --git a/.changeset/shiny-bugs-beam.md b/.changeset/shiny-bugs-beam.md new file mode 100644 index 0000000000..b16c8d6729 --- /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 an array of the corresponding resources. 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 }], + }); + }); +}); diff --git a/plugins/permission-backend/package.json b/plugins/permission-backend/package.json index 890495f646..8a9bd61445 100644 --- a/plugins/permission-backend/package.json +++ b/plugins/permission-backend/package.json @@ -21,12 +21,15 @@ "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": "*", + "dataloader": "^2.0.0", "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 +37,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..4be1b55379 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,16 @@ describe('PermissionIntegrationClient', () => { const mockApplyConditionsHandler = jest.fn( (_req, res, { json }: RestContext) => { - return res(json({ result: AuthorizeResult.ALLOW })); + return res( + json({ items: [{ 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 +66,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,30 +79,39 @@ 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('plugin-1', [ + { + id: '123', + 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('plugin-1', [ + { + id: '123', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ]); expect(mockApplyConditionsHandler).toHaveBeenCalledWith( expect.objectContaining({ body: { - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: mockConditions, + items: [ + { + id: '123', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ], }, }), expect.anything(), @@ -109,25 +120,29 @@ 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('plugin-1', [ + { + id: '123', + 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('plugin-1', [ + { + id: '123', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ]); const request = mockApplyConditionsHandler.mock.calls[0][0]; expect(request.headers.has('authorization')).toEqual(false); @@ -135,12 +150,15 @@ 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, - }, + 'plugin-1', + [ + { + id: '123', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + ], 'Bearer fake-token', ); @@ -156,36 +174,88 @@ describe('PermissionIntegrationClient', () => { ); await expect( - client.applyConditions({ - pluginId: 'test-plugin', - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: mockConditions, - }), + client.applyConditions('plugin-1', [ + { + id: '123', + 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({ items: [{ id: '123', outcome: AuthorizeResult.ALLOW }] }), + ); }, ); await expect( - client.applyConditions({ - pluginId: 'test-plugin', - resourceRef: 'testResource1', - resourceType: 'test-resource', - conditions: mockConditions, - }), + client.applyConditions('plugin-1', [ + { + id: '123', + 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({ + items: [ + { id: '123', result: AuthorizeResult.ALLOW }, + { id: '456', result: AuthorizeResult.DENY }, + { id: '789', result: AuthorizeResult.ALLOW }, + ], + }), + ); + }, + ); + + await expect( + client.applyConditions('plugin-1', [ + { + id: '123', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + { + id: '456', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: mockConditions, + }, + { + id: '789', + 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 routerSpy: RequestHandler; beforeAll(async () => { const router = Router(); @@ -193,7 +263,10 @@ describe('PermissionIntegrationClient', () => { router.use( createPermissionIntegrationRouter({ resourceType: 'test-resource', - getResource: async resourceRef => ({ id: resourceRef }), + getResources: async resourceRefs => + resourceRefs.map(resourceRef => ({ + id: resourceRef, + })), rules: [ { name: 'RULE_1', @@ -217,7 +290,9 @@ describe('PermissionIntegrationClient', () => { const app = express(); - app.use('/test-plugin', router); + routerSpy = jest.fn(router); + + app.use('/plugin-1', routerSpy); await new Promise(resolve => { server = app.listen(resolve); @@ -252,41 +327,45 @@ 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('plugin-1', [ + { + id: '123', + 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('plugin-1', [ + { + id: '123', + 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 }]); }); }); }); diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.ts index de42a7f194..2b2161ee71 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.ts @@ -17,20 +17,28 @@ import fetch from 'node-fetch'; import { z } from 'zod'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; +import { AuthorizeResult } from '@backstage/plugin-permission-common'; import { - AuthorizeResult, - PermissionCondition, - PermissionCriteria, -} from '@backstage/plugin-permission-common'; -import { - ApplyConditionsRequest, - ApplyConditionsResponse, + ApplyConditionsRequestEntry, + ApplyConditionsResponseEntry, + ConditionalPolicyDecision, } from '@backstage/plugin-permission-node'; const responseSchema = z.object({ - result: z.literal(AuthorizeResult.ALLOW).or(z.literal(AuthorizeResult.DENY)), + items: z.array( + z.object({ + id: z.string(), + result: z + .literal(AuthorizeResult.ALLOW) + .or(z.literal(AuthorizeResult.DENY)), + }), + ), }); +export type ResourcePolicyDecision = ConditionalPolicyDecision & { + resourceRef: string; +}; + export class PermissionIntegrationClient { private readonly discovery: PluginEndpointDiscovery; @@ -39,32 +47,26 @@ export class PermissionIntegrationClient { } async applyConditions( - { - pluginId, - resourceRef, - resourceType, - conditions, - }: { - resourceRef: string; - pluginId: string; - resourceType: string; - conditions: PermissionCriteria; - }, + pluginId: string, + decisions: readonly ApplyConditionsRequestEntry[], authHeader?: string, - ): Promise { + ): 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({ + items: decisions.map( + ({ id, resourceRef, resourceType, conditions }) => ({ + id, + resourceRef, + resourceType, + conditions, + }), + ), + }), headers: { ...(authHeader ? { authorization: authHeader } : {}), 'content-type': 'application/json', @@ -77,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-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index 26fa7ef8a6..971cb6a223 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -19,14 +19,30 @@ 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 { + ApplyConditionsRequestEntry, + 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 ( + _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', () => ({ PermissionIntegrationClient: jest.fn(() => ({ applyConditions: mockApplyConditions, @@ -34,7 +50,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 }; } @@ -70,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'); @@ -158,18 +178,14 @@ describe('createRouter', () => { }); describe('conditional policy result', () => { - beforeEach(() => { - policy.handle.mockReturnValueOnce({ + 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([ @@ -188,21 +204,399 @@ 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 => { - mockApplyConditions.mockResolvedValueOnce({ - 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', + result, + }, + { + id: '234', + result, + }, + ]); + const response = await request(app) .post('/authorize') .auth('test-token', { type: 'bearer' }) @@ -216,15 +610,33 @@ 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'] }] }, - }, + 'test-plugin', + [ + expect.objectContaining({ + id: '123', + resourceType: 'test-resource-1', + resourceRef: 'test/resource', + conditions: { rule: 'test-rule', params }, + }), + expect.objectContaining({ + id: '234', + resourceType: 'test-resource-1', + resourceRef: 'test/resource', + conditions: { rule: 'test-rule', params }, + }), + ], 'Bearer test-token', ); @@ -234,6 +646,10 @@ describe('createRouter', () => { id: '123', result, }, + { + id: '234', + result, + }, ]); }, ); @@ -247,10 +663,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 +677,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..58438afbb5 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,8 +33,14 @@ import { AuthorizeRequest, Identified, } from '@backstage/plugin-permission-common'; -import { PermissionPolicy } from '@backstage/plugin-permission-node'; +import { + ApplyConditionsRequestEntry, + ApplyConditionsResponseEntry, + PermissionPolicy, +} 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({ @@ -70,45 +77,52 @@ export interface RouterOptions { } const handleRequest = async ( - { id, resourceRef, ...request }: Identified, + requests: Identified[], user: BackstageIdentityResponse | undefined, policy: PermissionPolicy, permissionIntegrationClient: PermissionIntegrationClient, authHeader?: string, -): Promise> => { - const response = await policy.handle(request, user); +): Promise[]> => { + const applyConditionsLoaderFor = memoize((pluginId: string) => { + return new DataLoader< + ApplyConditionsRequestEntry, + ApplyConditionsResponseEntry + >(batch => + permissionIntegrationClient.applyConditions(pluginId, batch, authHeader), + ); + }); - if (response.result === AuthorizeResult.CONDITIONAL) { - // Sanity check that any resource provided matches the one expected by the permission - if (request.permission.resourceType !== response.resourceType) { - throw new Error( - `Invalid resource conditions returned from permission policy for permission ${request.permission.name}`, - ); - } + return Promise.all( + requests.map(({ id, resourceRef, ...request }) => + policy.handle(request, user).then(decision => { + if (decision.result !== AuthorizeResult.CONDITIONAL) { + return { + id, + ...decision, + }; + } - if (resourceRef) { - return { - id, - ...(await permissionIntegrationClient.applyConditions( - { - resourceRef, - pluginId: response.pluginId, - resourceType: response.resourceType, - conditions: response.conditions, - }, - authHeader, - )), - }; - } + if (decision.resourceType !== request.permission.resourceType) { + throw new Error( + `Invalid resource conditions returned from permission policy for permission ${request.permission.name}`, + ); + } - return { - id, - result: AuthorizeResult.CONDITIONAL, - conditions: response.conditions, - }; - } + if (!resourceRef) { + return { + id, + ...decision, + }; + } - return { id, ...response }; + return applyConditionsLoaderFor(decision.pluginId).load({ + id, + resourceRef, + ...decision, + }); + }), + ), + ); }; /** @@ -142,24 +156,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..71685c59b1 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -9,25 +9,34 @@ 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 = { - resourceRef: string; - resourceType: string; - conditions: PermissionCriteria; + items: ApplyConditionsRequestEntry[]; }; +// @public +export type ApplyConditionsRequestEntry = Identified<{ + resourceRef: string; + resourceType: string; + conditions: PermissionCriteria; +}>; + // @public export type ApplyConditionsResponse = { - result: AuthorizeResult.ALLOW | AuthorizeResult.DENY; + items: ApplyConditionsResponseEntry[]; }; +// @public +export type ApplyConditionsResponseEntry = Identified; + // @public export type Condition = TRule extends PermissionRule< any, @@ -92,8 +101,8 @@ export const createConditionTransformer: < export const createPermissionIntegrationRouter: (options: { resourceType: string; rules: PermissionRule[]; - getResource: (resourceRef: string) => Promise; -}) => Router; + getResources: (resourceRefs: string[]) => Promise<(TResource | undefined)[]>; +}) => express.Router; // @public export const createPermissionRule: < @@ -104,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[], @@ -137,9 +151,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..065c30a978 100644 --- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts +++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts @@ -16,15 +16,13 @@ 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, - }), +const mockGetResources: jest.MockedFunction< + Parameters[0]['getResources'] +> = jest.fn(async resourceRefs => + resourceRefs.map(resourceRef => ({ id: resourceRef })), ); const testRule1 = { @@ -47,16 +45,20 @@ describe('createPermissionIntegrationRouter', () => { let app: Express; let router: Router; - beforeEach(() => { + beforeAll(() => { router = createPermissionIntegrationRouter({ resourceType: 'test-resource', - getResource: mockGetResource, + getResources: mockGetResources, rules: [testRule1, testRule2], }); app = express().use(router); }); + afterEach(() => { + jest.clearAllMocks(); + }); + it('works', async () => { expect(router).toBeDefined(); }); @@ -70,7 +72,6 @@ describe('createPermissionIntegrationRouter', () => { { rule: 'test-rule-2', params: [{}] }, ], }, - { not: { rule: 'test-rule-2', params: [{}] }, }, @@ -96,13 +97,25 @@ describe('createPermissionIntegrationRouter', () => { const response = await request(app) .post('/.well-known/backstage/permissions/apply-conditions') .send({ - resourceRef: 'default:test/resource', - resourceType: 'test-resource', - conditions, + items: [ + { + 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({ + items: [ + { + id: '123', + result: AuthorizeResult.ALLOW, + }, + ], + }); }); it.each([ @@ -137,74 +150,237 @@ describe('createPermissionIntegrationRouter', () => { const response = await request(app) .post('/.well-known/backstage/permissions/apply-conditions') .send({ - resourceRef: 'default:test/resource', - resourceType: 'test-resource', - conditions, + items: [ + { + 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({ + items: [{ 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({ + 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({ + 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', () => { + 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 () => { const response = await request(app) .post('/.well-known/backstage/permissions/apply-conditions') .send({ - resourceRef: 'default:test/resource', - resourceType: 'test-incorrect-resource', - conditions: { - anyOf: [], - }, + items: [ + { + id: '123', + 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: [], + }, + }, + ], }); 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, ); }); - it('returns 400 when resource is not found', async () => { - mockGetResource.mockReturnValueOnce(Promise.resolve(undefined)); + it('returns 200/DENY when resource is not found', async () => { + mockGetResources.mockImplementationOnce(async resourceRefs => + resourceRefs.map(() => 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], + items: [ + { + id: '123', + resourceRef: 'default:test/resource', + resourceType: 'test-resource', + conditions: { rule: 'test-rule-1', 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({ + items: [ + { + id: '123', + result: AuthorizeResult.DENY, + }, + ], + }); + }); + + 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, + '', {}, { resourceType: 'test-resource-type' }, - { resourceRef: 'test/resource-ref' }, + [{ resourceType: 'test-resource-type' }], + { items: [{ resourceType: 'test-resource-type' }] }, + { items: [{ resourceRef: 'test/resource-ref' }] }, { - resourceType: 'test-resource-type', - resourceRef: 'test/resource-ref', + items: [ + { + resourceType: 'test-resource-type', + resourceRef: 'test/resource-ref', + }, + ], }, - { conditions: { anyOf: [] } }, + { items: [{ 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..1b01660fb0 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 @@ -44,9 +49,14 @@ const permissionCriteriaSchema: z.ZodSchema< ); const applyConditionsRequestSchema = z.object({ - resourceRef: z.string(), - resourceType: z.string(), - conditions: permissionCriteriaSchema, + items: z.array( + z.object({ + id: z.string(), + resourceRef: z.string(), + resourceType: z.string(), + conditions: permissionCriteriaSchema, + }), + ), }); /** @@ -55,10 +65,19 @@ 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 = { + items: ApplyConditionsRequestEntry[]; }; /** @@ -67,15 +86,29 @@ export type ApplyConditionsRequest = { * * @public */ +export type ApplyConditionsResponseEntry = Identified; + +/** + * A batch of {@link ApplyConditionsResponseEntry} objects. + * + * @public + */ export type ApplyConditionsResponse = { - result: AuthorizeResult.ALLOW | AuthorizeResult.DENY; + items: 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 (resource === undefined) { + return false; + } + if (isAndCriteria(criteria)) { return criteria.allOf.every(child => applyConditions(child, resource, getRule), @@ -92,84 +125,107 @@ 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 */ export const createPermissionIntegrationRouter = (options: { resourceType: string; rules: PermissionRule[]; - getResource: (resourceRef: string) => Promise; -}): Router => { - const { resourceType, rules, getResource } = options; + getResources: ( + resourceRefs: string[], + ) => Promise>; +}): express.Router => { + const { resourceType, rules, getResources } = options; const router = Router(); const getRule = createGetRule(rules); + const assertValidResourceTypes = ( + requests: ApplyConditionsRequestEntry[], + ) => { + const invalidResourceTypes = requests + .filter(request => request.resourceType !== resourceType) + .map(request => request.resourceType); + + if (invalidResourceTypes.length) { + throw new InputError( + `Unexpected resource types: ${invalidResourceTypes.join(', ')}.`, + ); + } + }; + + router.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.items); - const resource = await getResource(body.resourceRef); + const resourceRefs = Array.from( + new Set(body.items.map(({ resourceRef }) => resourceRef)), + ); + const resourceArray = await getResources(resourceRefs); + const resources = resourceRefs.reduce((acc, resourceRef, index) => { + acc[resourceRef] = resourceArray[index]; - if (!resource) { - return res - .status(400) - .send(`Resource for ref ${body.resourceRef} not found.`); - } + return acc; + }, {} as Record); return res.status(200).json({ - result: applyConditions(body.conditions, resource, getRule) - ? AuthorizeResult.ALLOW - : AuthorizeResult.DENY, + items: body.items.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; /** diff --git a/yarn.lock b/yarn.lock index 84a36dbb12..ca58fc8ea3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -12661,7 +12661,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==