From e15ce5c16e5ed4837151d7d96cfbee93a6098fa1 Mon Sep 17 00:00:00 2001 From: Joon Park Date: Mon, 17 Jan 2022 12:22:24 +0000 Subject: [PATCH] Integrate authorization into catalog-backend delete entities endpoint (#8805) * Integrate authorization into catalog-backend delete entities endpoint Signed-off-by: Joon Park * Remove entity ref from request and handle conditional response Signed-off-by: Joon Park --- .changeset/bright-candles-call.md | 5 ++ plugins/catalog-backend/api-report.md | 7 +- plugins/catalog-backend/src/catalog/types.ts | 5 +- .../service/AuthorizedEntitiesCatalog.test.ts | 83 +++++++++++++++++++ .../src/service/AuthorizedEntitiesCatalog.ts | 35 +++++++- .../src/service/NextRouter.test.ts | 24 ++++-- .../catalog-backend/src/service/NextRouter.ts | 4 +- 7 files changed, 151 insertions(+), 12 deletions(-) create mode 100644 .changeset/bright-candles-call.md diff --git a/.changeset/bright-candles-call.md b/.changeset/bright-candles-call.md new file mode 100644 index 0000000000..23f7f483fd --- /dev/null +++ b/.changeset/bright-candles-call.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Integrate authorization into the delete entities endpoint diff --git a/plugins/catalog-backend/api-report.md b/plugins/catalog-backend/api-report.md index c503d303ba..25924341e0 100644 --- a/plugins/catalog-backend/api-report.md +++ b/plugins/catalog-backend/api-report.md @@ -830,7 +830,12 @@ export function durationText(startTimestamp: [number, number]): string; // @public (undocumented) export type EntitiesCatalog = { entities(request?: EntitiesRequest): Promise; - removeEntityByUid(uid: string): Promise; + removeEntityByUid( + uid: string, + options?: { + authorizationToken?: string; + }, + ): Promise; batchAddOrUpdateEntities?( requests: EntityUpsertRequest[], options?: { diff --git a/plugins/catalog-backend/src/catalog/types.ts b/plugins/catalog-backend/src/catalog/types.ts index f71d7fb7d9..de78e4ac84 100644 --- a/plugins/catalog-backend/src/catalog/types.ts +++ b/plugins/catalog-backend/src/catalog/types.ts @@ -113,7 +113,10 @@ export type EntitiesCatalog = { * * @param uid - The metadata.uid of the entity */ - removeEntityByUid(uid: string): Promise; + removeEntityByUid( + uid: string, + options?: { authorizationToken?: string }, + ): Promise; /** * Writes a number of entities efficiently to storage. diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts index 768e3a690b..34e8b6c569 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { NotAllowedError } from '@backstage/errors'; import { AuthorizeResult } from '@backstage/plugin-permission-common'; import { createConditionTransformer } from '@backstage/plugin-permission-node'; import { isEntityKind } from '../permissions/rules/isEntityKind'; @@ -93,4 +94,86 @@ describe('AuthorizedEntitiesCatalog', () => { }); }); }); + + describe('removeEntityByUid', () => { + it('throws error on DENY', async () => { + fakeCatalog.entities.mockResolvedValue({ + entities: [ + { kind: 'component', namespace: 'default', name: 'my-component' }, + ], + }); + fakePermissionApi.authorize.mockResolvedValue([ + { result: AuthorizeResult.DENY }, + ]); + const catalog = new AuthorizedEntitiesCatalog( + fakeCatalog, + fakePermissionApi, + createConditionTransformer([]), + ); + + await expect(() => + catalog.removeEntityByUid('uid', { authorizationToken: 'abcd' }), + ).rejects.toThrowError(NotAllowedError); + }); + + it('throws error on CONDITIONAL authorization that evaluates to 0 entities', async () => { + fakePermissionApi.authorize.mockResolvedValue([ + { + result: AuthorizeResult.CONDITIONAL, + conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, + }, + ]); + fakeCatalog.entities.mockResolvedValue({ entities: [] }); + const catalog = new AuthorizedEntitiesCatalog( + fakeCatalog, + fakePermissionApi, + createConditionTransformer([isEntityKind]), + ); + + await expect(() => + catalog.removeEntityByUid('uid', { authorizationToken: 'abcd' }), + ).rejects.toThrowError(NotAllowedError); + }); + + it('calls underlying catalog method on CONDITIONAL authorization that evaluates to nonzero entities', async () => { + fakePermissionApi.authorize.mockResolvedValue([ + { + result: AuthorizeResult.CONDITIONAL, + conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, + }, + ]); + fakeCatalog.entities.mockResolvedValue({ + entities: [{ kind: 'b', namespace: 'default', name: 'my-component' }], + }); + const catalog = new AuthorizedEntitiesCatalog( + fakeCatalog, + fakePermissionApi, + createConditionTransformer([isEntityKind]), + ); + + await catalog.removeEntityByUid('uid', { authorizationToken: 'abcd' }); + + expect(fakeCatalog.removeEntityByUid).toHaveBeenCalledWith('uid'); + }); + + it('calls underlying catalog method on ALLOW', async () => { + fakeCatalog.entities.mockResolvedValue({ + entities: [ + { kind: 'component', namespace: 'default', name: 'my-component' }, + ], + }); + fakePermissionApi.authorize.mockResolvedValue([ + { result: AuthorizeResult.ALLOW }, + ]); + const catalog = new AuthorizedEntitiesCatalog( + fakeCatalog, + fakePermissionApi, + createConditionTransformer([]), + ); + + await catalog.removeEntityByUid('uid', { authorizationToken: 'abcd' }); + + expect(fakeCatalog.removeEntityByUid).toHaveBeenCalledWith('uid'); + }); + }); }); diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts index 270eb64a40..412c6bd326 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts @@ -14,7 +14,11 @@ * limitations under the License. */ -import { catalogEntityReadPermission } from '@backstage/plugin-catalog-common'; +import { NotAllowedError } from '@backstage/errors'; +import { + catalogEntityDeletePermission, + catalogEntityReadPermission, +} from '@backstage/plugin-catalog-common'; import { AuthorizeResult, PermissionAuthorizer, @@ -27,6 +31,7 @@ import { EntityAncestryResponse, EntityFilter, } from '../catalog/types'; +import { basicEntityFilter } from './request/basicEntityFilter'; export class AuthorizedEntitiesCatalog implements EntitiesCatalog { constructor( @@ -65,8 +70,32 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { return this.entitiesCatalog.entities(request); } - removeEntityByUid(uid: string): Promise { - // TODO: Implement permissioning + async removeEntityByUid( + uid: string, + options?: { authorizationToken?: string }, + ): Promise { + const authorizeResponse = ( + await this.permissionApi.authorize( + [{ permission: catalogEntityDeletePermission }], + { token: options?.authorizationToken }, + ) + )[0]; + if (authorizeResponse.result === AuthorizeResult.DENY) { + throw new NotAllowedError(); + } + if (authorizeResponse.result === AuthorizeResult.CONDITIONAL) { + const permissionFilter: EntityFilter = this.transformConditions( + authorizeResponse.conditions, + ); + const { entities } = await this.entitiesCatalog.entities({ + filter: { + allOf: [permissionFilter, basicEntityFilter({ 'metadata.uid': uid })], + }, + }); + if (entities.length === 0) { + throw new NotAllowedError(); + } + } return this.entitiesCatalog.removeEntityByUid(uid); } diff --git a/plugins/catalog-backend/src/service/NextRouter.test.ts b/plugins/catalog-backend/src/service/NextRouter.test.ts index b79fba4a56..1ef8eb594f 100644 --- a/plugins/catalog-backend/src/service/NextRouter.test.ts +++ b/plugins/catalog-backend/src/service/NextRouter.test.ts @@ -216,10 +216,14 @@ describe('createNextRouter readonly disabled', () => { it('can remove', async () => { entitiesCatalog.removeEntityByUid.mockResolvedValue(undefined); - const response = await request(app).delete('/entities/by-uid/apa'); + const response = await request(app) + .delete('/entities/by-uid/apa') + .set('authorization', 'Bearer someauthtoken'); expect(entitiesCatalog.removeEntityByUid).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.removeEntityByUid).toHaveBeenCalledWith('apa'); + expect(entitiesCatalog.removeEntityByUid).toHaveBeenCalledWith('apa', { + authorizationToken: 'someauthtoken', + }); expect(response.status).toEqual(204); }); @@ -228,10 +232,14 @@ describe('createNextRouter readonly disabled', () => { new NotFoundError('nope'), ); - const response = await request(app).delete('/entities/by-uid/apa'); + const response = await request(app) + .delete('/entities/by-uid/apa') + .set('authorization', 'Bearer someauthtoken'); expect(entitiesCatalog.removeEntityByUid).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.removeEntityByUid).toHaveBeenCalledWith('apa'); + expect(entitiesCatalog.removeEntityByUid).toHaveBeenCalledWith('apa', { + authorizationToken: 'someauthtoken', + }); expect(response.status).toEqual(404); }); }); @@ -372,10 +380,14 @@ describe('createNextRouter readonly enabled', () => { describe('DELETE /entities/by-uid/:uid', () => { // this delete is allowed as there is no other way to remove entities it('is allowed', async () => { - const response = await request(app).delete('/entities/by-uid/apa'); + const response = await request(app) + .delete('/entities/by-uid/apa') + .set('authorization', 'Bearer someauthtoken'); expect(entitiesCatalog.removeEntityByUid).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.removeEntityByUid).toHaveBeenCalledWith('apa'); + expect(entitiesCatalog.removeEntityByUid).toHaveBeenCalledWith('apa', { + authorizationToken: 'someauthtoken', + }); expect(response.status).toEqual(204); }); }); diff --git a/plugins/catalog-backend/src/service/NextRouter.ts b/plugins/catalog-backend/src/service/NextRouter.ts index 78550e1791..7298c2c32c 100644 --- a/plugins/catalog-backend/src/service/NextRouter.ts +++ b/plugins/catalog-backend/src/service/NextRouter.ts @@ -119,7 +119,9 @@ export async function createNextRouter( }) .delete('/entities/by-uid/:uid', async (req, res) => { const { uid } = req.params; - await entitiesCatalog.removeEntityByUid(uid); + await entitiesCatalog.removeEntityByUid(uid, { + authorizationToken: getBearerToken(req.header('authorization')), + }); res.status(204).end(); }) .get('/entities/by-name/:kind/:namespace/:name', async (req, res) => {