From 9e63318311be4a267adcb7b4b566f69357df73cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Tue, 7 May 2024 08:30:04 +0200 Subject: [PATCH] Implement the scope feature of external access service tokens, as per BEP-0007 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/friendly-keys-fold.md | 6 + .changeset/great-cougars-guess.md | 5 + .changeset/neat-rivers-share.md | 5 + packages/backend-app-api/config.d.ts | 80 ++++++++ .../auth/DefaultAuthService.ts | 22 ++- .../auth/authServiceFactory.test.ts | 60 +++++- .../auth/external/ExternalTokenHandler.ts | 12 +- .../auth/external/helpers.test.ts | 181 ++++++++++++++++++ .../implementations/auth/external/helpers.ts | 144 ++++++++++++++ .../auth/external/legacy.test.ts | 150 +++++++++++---- .../implementations/auth/external/legacy.ts | 39 +++- .../auth/external/static.test.ts | 86 ++++++--- .../implementations/auth/external/static.ts | 28 ++- .../implementations/auth/external/types.ts | 14 +- .../services/implementations/auth/helpers.ts | 3 + .../permissions/permissionsServiceFactory.ts | 4 +- packages/backend-plugin-api/api-report.md | 10 + .../src/services/definitions/AuthService.ts | 49 +++++ .../src/services/definitions/index.ts | 1 + packages/backend-test-utils/api-report.md | 2 + .../src/next/services/mockCredentials.test.ts | 10 + .../src/next/services/mockCredentials.ts | 10 +- plugins/permission-node/api-report.md | 1 + .../src/ServerPermissionClient.test.ts | 66 ++++++- .../src/ServerPermissionClient.ts | 65 ++++++- 25 files changed, 968 insertions(+), 85 deletions(-) create mode 100644 .changeset/friendly-keys-fold.md create mode 100644 .changeset/great-cougars-guess.md create mode 100644 .changeset/neat-rivers-share.md create mode 100644 packages/backend-app-api/src/services/implementations/auth/external/helpers.test.ts create mode 100644 packages/backend-app-api/src/services/implementations/auth/external/helpers.ts diff --git a/.changeset/friendly-keys-fold.md b/.changeset/friendly-keys-fold.md new file mode 100644 index 0000000000..218abe4efe --- /dev/null +++ b/.changeset/friendly-keys-fold.md @@ -0,0 +1,6 @@ +--- +'@backstage/backend-plugin-api': patch +'@backstage/backend-app-api': patch +--- + +Added an optional `accessRestrictions` to external access service tokens and service principals in general, such that you can limit their access to certain plugins or permissions. diff --git a/.changeset/great-cougars-guess.md b/.changeset/great-cougars-guess.md new file mode 100644 index 0000000000..af1de3e8ce --- /dev/null +++ b/.changeset/great-cougars-guess.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-test-utils': patch +--- + +Made it possible to give access restrictions to `mockCredentials.service` diff --git a/.changeset/neat-rivers-share.md b/.changeset/neat-rivers-share.md new file mode 100644 index 0000000000..42649de977 --- /dev/null +++ b/.changeset/neat-rivers-share.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-permission-node': patch +--- + +Ensure that service token access restrictions, when present, are taken into account diff --git a/packages/backend-app-api/config.d.ts b/packages/backend-app-api/config.d.ts index 5517af4f98..5b72fef54c 100644 --- a/packages/backend-app-api/config.d.ts +++ b/packages/backend-app-api/config.d.ts @@ -88,6 +88,46 @@ export interface Config { */ subject: string; }; + /** + * Restricts what types of access that are permitted for this access + * method. If no access restrictions are given, it'll have unlimited + * access. This access restriction applies for the framework level; + * individual plugins may have their own access control mechanisms + * on top of this. + */ + accessRestrictions?: Array<{ + /** + * Permit access to make requests to this plugin. + * + * Can be further refined by setting additional fields below. + */ + plugin: string; + /** + * If given, this method is limited to only performing actions + * with these named permissions in this plugin. + * + * Note that this only applies where permissions checks are + * enabled in the first place. Endpoints that are not protected by + * the permissions system at all, are not affected by this + * setting. + */ + permission?: string | Array; + /** + * If given, this method is limited to only performing actions + * whose permissions have these attributes. + * + * Note that this only applies where permissions checks are + * enabled in the first place. Endpoints that are not protected by + * the permissions system at all, are not affected by this + * setting. + */ + permissionAttribute?: { + /** + * One of more of 'create', 'read', 'update', or 'delete'. + */ + action?: string | Array; + }; + }>; } | { /** @@ -130,6 +170,46 @@ export interface Config { */ subject: string; }; + /** + * Restricts what types of access that are permitted for this access + * method. If no access restrictions are given, it'll have unlimited + * access. This access restriction applies for the framework level; + * individual plugins may have their own access control mechanisms + * on top of this. + */ + accessRestrictions?: Array<{ + /** + * Permit access to make requests to this plugin. + * + * Can be further refined by setting additional fields below. + */ + plugin: string; + /** + * If given, this method is limited to only performing actions + * with these named permissions in this plugin. + * + * Note that this only applies where permissions checks are + * enabled in the first place. Endpoints that are not protected by + * the permissions system at all, are not affected by this + * setting. + */ + permission?: string | Array; + /** + * If given, this method is limited to only performing actions + * whose permissions have these attributes. + * + * Note that this only applies where permissions checks are + * enabled in the first place. Endpoints that are not protected by + * the permissions system at all, are not affected by this + * setting. + */ + permissionAttribute?: { + /** + * One of more of 'create', 'read', 'update', or 'delete'. + */ + action?: string | Array; + }; + }>; } | { /** diff --git a/packages/backend-app-api/src/services/implementations/auth/DefaultAuthService.ts b/packages/backend-app-api/src/services/implementations/auth/DefaultAuthService.ts index dc76a8761b..d0c11a4eee 100644 --- a/packages/backend-app-api/src/services/implementations/auth/DefaultAuthService.ts +++ b/packages/backend-app-api/src/services/implementations/auth/DefaultAuthService.ts @@ -23,7 +23,11 @@ import { BackstageServicePrincipal, BackstageUserPrincipal, } from '@backstage/backend-plugin-api'; -import { AuthenticationError, ForwardedError } from '@backstage/errors'; +import { + AuthenticationError, + ForwardedError, + NotAllowedError, +} from '@backstage/errors'; import { JsonObject } from '@backstage/types'; import { decodeJwt } from 'jose'; import { ExternalTokenHandler } from './external/ExternalTokenHandler'; @@ -82,7 +86,21 @@ export class DefaultAuthService implements AuthService { const externalResult = await this.externalTokenHandler.verifyToken(token); if (externalResult) { - return createCredentialsWithServicePrincipal(externalResult.subject); + const restrictions = externalResult.accessRestrictions; + if (restrictions) { + if (!restrictions.has(this.pluginId)) { + const valid = [...restrictions.keys()].map(k => `'${k}'`).join(', '); + throw new NotAllowedError( + `This token's access is restricted to plugin(s) ${valid}`, + ); + } + } + + return createCredentialsWithServicePrincipal( + externalResult.subject, + undefined, + restrictions?.get(this.pluginId), + ); } throw new AuthenticationError('Illegal token'); diff --git a/packages/backend-app-api/src/services/implementations/auth/authServiceFactory.test.ts b/packages/backend-app-api/src/services/implementations/auth/authServiceFactory.test.ts index 96d4ce4dc9..71fbe64963 100644 --- a/packages/backend-app-api/src/services/implementations/auth/authServiceFactory.test.ts +++ b/packages/backend-app-api/src/services/implementations/auth/authServiceFactory.test.ts @@ -42,7 +42,26 @@ const mockDeps = [ data: { backend: { baseUrl: 'http://localhost', - auth: { keys: [{ secret: 'abc' }] }, + auth: { + keys: [{ secret: 'abc' }], + externalAccess: [ + { + type: 'static', + options: { + token: 'limited-static-token', + subject: 'limited-static-subject', + }, + accessRestrictions: [{ plugin: 'catalog', permission: 'do.it' }], + }, + { + type: 'static', + options: { + token: 'unlimited-static-token', + subject: 'unlimited-static-subject', + }, + }, + ], + }, }, }, }), @@ -385,4 +404,43 @@ describe('authServiceFactory', () => { "Unable to call 'kubernetes' plugin on behalf of user, because the target plugin does not support on-behalf-of tokens or the plugin doesn't exist", ); }); + + it('should eagerly reject access to external access tokens based on plugin id', async () => { + const tester = ServiceFactoryTester.from(authServiceFactory, { + dependencies: mockDeps, + }); + + const catalogAuth = await tester.get('catalog'); + + await expect( + catalogAuth.authenticate('limited-static-token'), + ).resolves.toMatchObject({ + principal: { + subject: 'limited-static-subject', + accessRestrictions: { permissionNames: ['do.it'] }, + }, + }); + + await expect( + catalogAuth.authenticate('unlimited-static-token'), + ).resolves.toMatchObject({ + principal: { + subject: 'unlimited-static-subject', + }, + }); + + const scaffolderAuth = await tester.get('scaffolder'); + + await expect( + scaffolderAuth.authenticate('limited-static-token'), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"This token's access is restricted to plugin(s) 'catalog'"`, + ); + + await expect( + scaffolderAuth.authenticate('unlimited-static-token'), + ).resolves.toMatchObject({ + principal: { subject: 'unlimited-static-subject' }, + }); + }); }); diff --git a/packages/backend-app-api/src/services/implementations/auth/external/ExternalTokenHandler.ts b/packages/backend-app-api/src/services/implementations/auth/external/ExternalTokenHandler.ts index 79ad8dd3c4..82603953d0 100644 --- a/packages/backend-app-api/src/services/implementations/auth/external/ExternalTokenHandler.ts +++ b/packages/backend-app-api/src/services/implementations/auth/external/ExternalTokenHandler.ts @@ -20,8 +20,8 @@ import { } from '@backstage/backend-plugin-api'; import { LegacyTokenHandler } from './legacy'; import { StaticTokenHandler } from './static'; -import { TokenHandler } from './types'; import { JWKSHandler } from './jwks'; +import { AccessRestriptionsMap, TokenHandler } from './types'; const NEW_CONFIG_KEY = 'backend.auth.externalAccess'; const OLD_CONFIG_KEY = 'backend.auth.keys'; @@ -61,7 +61,7 @@ export class ExternalTokenHandler { `Unknown type '${type}' in ${NEW_CONFIG_KEY}, expected one of ${valid}`, ); } - handler.add(handlerConfig.getConfig('options')); + handler.add(handlerConfig); } // Load the old keys too @@ -80,7 +80,13 @@ export class ExternalTokenHandler { constructor(private readonly handlers: TokenHandler[]) {} - async verifyToken(token: string): Promise<{ subject: string } | undefined> { + async verifyToken(token: string): Promise< + | { + subject: string; + accessRestrictions?: AccessRestriptionsMap; + } + | undefined + > { for (const handler of this.handlers) { const result = await handler.verifyToken(token); if (result) { diff --git a/packages/backend-app-api/src/services/implementations/auth/external/helpers.test.ts b/packages/backend-app-api/src/services/implementations/auth/external/helpers.test.ts new file mode 100644 index 0000000000..ee0dc3cb87 --- /dev/null +++ b/packages/backend-app-api/src/services/implementations/auth/external/helpers.test.ts @@ -0,0 +1,181 @@ +/* + * Copyright 2024 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ConfigReader } from '@backstage/config'; +import { readAccessRestrictionsFromConfig } from './helpers'; +import { JsonObject } from '@backstage/types'; + +describe('readAccessRestrictionsFromConfig', () => { + function r(config: JsonObject) { + return readAccessRestrictionsFromConfig(new ConfigReader(config)); + } + + it('handles empty / missing restrictions', () => { + expect(r({})).toBeUndefined(); + expect(r({ accessRestrictions: [] })).toBeUndefined(); + }); + + it('handles type errors', () => { + expect(() => + r({ accessRestrictions: 7 }), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'accessRestrictions' in 'mock-config', got number, wanted object-array"`, + ); + expect(() => + r({ accessRestrictions: ['hello'] }), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'accessRestrictions[0]' in 'mock-config', got string, wanted object-array"`, + ); + expect(() => + r({ accessRestrictions: [{ unknown: {} }] }), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid key 'unknown' in 'accessRestrictions' config, expected one of 'plugin', 'permission', 'permissionAttribute'"`, + ); + expect(() => + r({ accessRestrictions: [{ plugin: 7 }] }), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'accessRestrictions[0].plugin' in 'mock-config', got number, wanted string"`, + ); + expect(() => + r({ accessRestrictions: [{ plugin: 'valid', permission: 7 }] }), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'accessRestrictions[0].permission' in 'mock-config', got number, wanted string"`, + ); + expect(() => + r({ accessRestrictions: [{ plugin: 'valid', permission: [7] }] }), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'accessRestrictions[0].permission[0]' in 'mock-config', got number, wanted string-array"`, + ); + expect(() => + r({ accessRestrictions: [{ plugin: 'valid', permissionAttribute: 7 }] }), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'accessRestrictions[0].permissionAttribute' in 'mock-config', got number, wanted object"`, + ); + expect(() => + r({ + accessRestrictions: [ + { plugin: 'valid', permissionAttribute: { a: [] } }, + ], + }), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid key 'a' in 'permissionAttribute' config, expected 'action'"`, + ); + expect(() => + r({ + accessRestrictions: [ + { plugin: 'valid', permissionAttribute: { action: 7 } }, + ], + }), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'accessRestrictions[0].permissionAttribute.action' in 'mock-config', got number, wanted string"`, + ); + expect(() => + r({ + accessRestrictions: [ + { plugin: 'valid', permissionAttribute: { action: 'wrong' } }, + ], + }), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid value 'wrong' at 'action' in 'permissionAttributes' config, valid values are 'create', 'read', 'update', 'delete'"`, + ); + }); + + it('parses valid access restrictions', () => { + expect( + r({ + accessRestrictions: [ + { + plugin: 'a', + }, + ], + }), + ).toEqual( + new Map( + Object.entries({ + a: {}, + }), + ), + ); + + expect( + r({ + accessRestrictions: [ + { + plugin: 'a', + permission: 'a, b a', + }, + ], + }), + ).toEqual( + new Map( + Object.entries({ + a: { permissionNames: ['a', 'b'] }, + }), + ), + ); + + expect( + r({ + accessRestrictions: [ + { + plugin: 'a', + permission: ['a', 'b', 'a'], + }, + ], + }), + ).toEqual( + new Map( + Object.entries({ + a: { permissionNames: ['a', 'b'] }, + }), + ), + ); + + expect( + r({ + accessRestrictions: [ + { + plugin: 'a', + permissionAttribute: { action: 'read, update read' }, + }, + ], + }), + ).toEqual( + new Map( + Object.entries({ + a: { permissionAttributes: { action: ['read', 'update'] } }, + }), + ), + ); + + expect( + r({ + accessRestrictions: [ + { + plugin: 'a', + permissionAttribute: { action: ['read', 'update', 'read'] }, + }, + ], + }), + ).toEqual( + new Map( + Object.entries({ + a: { permissionAttributes: { action: ['read', 'update'] } }, + }), + ), + ); + }); +}); diff --git a/packages/backend-app-api/src/services/implementations/auth/external/helpers.ts b/packages/backend-app-api/src/services/implementations/auth/external/helpers.ts new file mode 100644 index 0000000000..5d199a5067 --- /dev/null +++ b/packages/backend-app-api/src/services/implementations/auth/external/helpers.ts @@ -0,0 +1,144 @@ +/* + * Copyright 2024 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Config } from '@backstage/config'; +import { AccessRestriptionsMap } from './types'; + +/** + * Parses and returns the `accessRestrictions` configuration from an + * `externalAccess` entry, or undefined if there wasn't one. + * + * @internal + */ +export function readAccessRestrictionsFromConfig( + externalAccessEntryConfig: Config, +): AccessRestriptionsMap | undefined { + const configs = + externalAccessEntryConfig.getOptionalConfigArray('accessRestrictions') ?? + []; + + const result: AccessRestriptionsMap = new Map(); + for (const config of configs) { + const validKeys = ['plugin', 'permission', 'permissionAttribute']; + for (const key of config.keys()) { + if (!validKeys.includes(key)) { + const valid = validKeys.map(k => `'${k}'`).join(', '); + throw new Error( + `Invalid key '${key}' in 'accessRestrictions' config, expected one of ${valid}`, + ); + } + } + + const pluginId = config.getString('plugin'); + const permissionNames = readPermissionNames(config); + const permissionAttributes = readPermissionAttributes(config); + + if (result.has(pluginId)) { + throw new Error( + `Attempted to declare 'accessRestrictions' twice for plugin '${pluginId}', which is not permitted`, + ); + } + + result.set(pluginId, { + ...(permissionNames ? { permissionNames } : {}), + ...(permissionAttributes ? { permissionAttributes } : {}), + }); + } + + return result.size ? result : undefined; +} + +/** + * Reads a config value as a string or an array of strings, and deduplicates and + * splits by comma/space into a string array. Can also validate against a known + * set of values. Returns undefined if the key didn't exist or if the array + * would have ended up being empty. + */ +function stringOrStringArray( + root: Config, + key: string, + validValues?: readonly T[], +): T[] | undefined { + if (!root.has(key)) { + return undefined; + } + + const rawValues = Array.isArray(root.get(key)) + ? root.getStringArray(key) + : [root.getString(key)]; + + const values = [ + ...new Set( + rawValues + .map(v => v.split(/[ ,]/)) + .flat() + .filter(Boolean), + ), + ]; + + if (!values.length) { + return undefined; + } + + if (validValues?.length) { + for (const value of values) { + if (!validValues.includes(value as T)) { + const valid = validValues.map(k => `'${k}'`).join(', '); + throw new Error( + `Invalid value '${value}' at '${key}' in 'permissionAttributes' config, valid values are ${valid}`, + ); + } + } + } + + return values as T[]; +} + +function readPermissionNames(externalAccessEntryConfig: Config) { + return stringOrStringArray(externalAccessEntryConfig, 'permission'); +} + +function readPermissionAttributes(externalAccessEntryConfig: Config) { + const config = externalAccessEntryConfig.getOptionalConfig( + 'permissionAttribute', + ); + if (!config) { + return undefined; + } + + const validKeys = ['action']; + for (const key of config.keys()) { + if (!validKeys.includes(key)) { + const valid = validKeys.map(k => `'${k}'`).join(', '); + throw new Error( + `Invalid key '${key}' in 'permissionAttribute' config, expected ${valid}`, + ); + } + } + + const action = stringOrStringArray(config, 'action', [ + 'create', + 'read', + 'update', + 'delete', + ]); + + const result = { + ...(action ? { action } : {}), + }; + + return Object.keys(result).length ? result : undefined; +} diff --git a/packages/backend-app-api/src/services/implementations/auth/external/legacy.test.ts b/packages/backend-app-api/src/services/implementations/auth/external/legacy.test.ts index 8b2469f6b7..503157acc7 100644 --- a/packages/backend-app-api/src/services/implementations/auth/external/legacy.test.ts +++ b/packages/backend-app-api/src/services/implementations/auth/external/legacy.test.ts @@ -25,17 +25,35 @@ describe('LegacyTokenHandler', () => { const key1 = randomBytes(24); const key2 = randomBytes(24); const key3 = randomBytes(24); + const accessRestrictions1 = new Map( + Object.entries({ + scaffolder: {}, + }), + ); + const accessRestrictions2 = new Map( + Object.entries({ + catalog: { permissionNames: ['catalog.entity.read'] }, + }), + ); tokenHandler.add( new ConfigReader({ - secret: key1.toString('base64'), - subject: 'key1', + options: { + secret: key1.toString('base64'), + subject: 'key1', + }, + accessRestrictions: [{ plugin: 'scaffolder' }], }), ); tokenHandler.add( new ConfigReader({ - secret: key2.toString('base64'), - subject: 'key2', + options: { + secret: key2.toString('base64'), + subject: 'key2', + }, + accessRestrictions: [ + { plugin: 'catalog', permission: 'catalog.entity.read' }, + ], }), ); tokenHandler.addOld( @@ -54,6 +72,7 @@ describe('LegacyTokenHandler', () => { await expect(tokenHandler.verifyToken(token1)).resolves.toEqual({ subject: 'key1', + accessRestrictions: accessRestrictions1, }); const token2 = await new SignJWT({ @@ -65,6 +84,7 @@ describe('LegacyTokenHandler', () => { await expect(tokenHandler.verifyToken(token2)).resolves.toEqual({ subject: 'key2', + accessRestrictions: accessRestrictions2, }); const token3 = await new SignJWT({ @@ -147,39 +167,93 @@ describe('LegacyTokenHandler', () => { // new style add, bad secrets expect(() => - handler.add(new ConfigReader({ _missingsecret: true, subject: 'ok' })), - ).toThrow(/secret/); + handler.add( + new ConfigReader({ options: { _missingsecret: true, subject: 'ok' } }), + ), + ).toThrowErrorMatchingInlineSnapshot( + `"Missing required config value at 'options.secret' in 'mock-config'"`, + ); expect(() => - handler.add(new ConfigReader({ secret: '', subject: 'ok' })), - ).toThrow(/secret/); + handler.add(new ConfigReader({ options: { secret: '', subject: 'ok' } })), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'options.secret' in 'mock-config', got empty-string, wanted string"`, + ); expect(() => - handler.add(new ConfigReader({ secret: 'has spaces', subject: 'ok' })), - ).toThrow(/secret/); + handler.add( + new ConfigReader({ options: { secret: 'has spaces', subject: 'ok' } }), + ), + ).toThrowErrorMatchingInlineSnapshot( + `"Illegal secret, must be a valid base64 string"`, + ); expect(() => - handler.add(new ConfigReader({ secret: 'hasnewline\n', subject: 'ok' })), - ).toThrow(/secret/); + handler.add( + new ConfigReader({ + options: { secret: 'hasnewline\n', subject: 'ok' }, + }), + ), + ).toThrowErrorMatchingInlineSnapshot( + `"Illegal secret, must be a valid base64 string"`, + ); expect(() => - handler.add(new ConfigReader({ secret: 3, subject: 'ok' })), - ).toThrow(/secret/); + handler.add(new ConfigReader({ options: { secret: 3, subject: 'ok' } })), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'options.secret' in 'mock-config', got number, wanted string"`, + ); // new style add, bad subjects expect(() => - handler.add(new ConfigReader({ secret: 'b2s=', _missingsubject: true })), - ).toThrow(/subject/); - expect(() => - handler.add(new ConfigReader({ secret: 'b2s=', subject: '' })), - ).toThrow(/subject/); - expect(() => - handler.add(new ConfigReader({ secret: 'b2s=', subject: 'has spaces' })), - ).toThrow(/subject/); + handler.add( + new ConfigReader({ + options: { secret: 'b2s=', _missingsubject: true }, + }), + ), + ).toThrowErrorMatchingInlineSnapshot( + `"Missing required config value at 'options.subject' in 'mock-config'"`, + ); expect(() => handler.add( - new ConfigReader({ secret: 'b2s=', subject: 'hasnewline\n' }), + new ConfigReader({ options: { secret: 'b2s=', subject: '' } }), ), - ).toThrow(/subject/); + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'options.subject' in 'mock-config', got empty-string, wanted string"`, + ); expect(() => - handler.add(new ConfigReader({ secret: 'b2s=', subject: 3 })), - ).toThrow(/subject/); + handler.add( + new ConfigReader({ + options: { secret: 'b2s=', subject: 'has spaces' }, + }), + ), + ).toThrowErrorMatchingInlineSnapshot( + `"Illegal subject, must be a set of non-space characters"`, + ); + expect(() => + handler.add( + new ConfigReader({ + options: { secret: 'b2s=', subject: 'hasnewline\n' }, + }), + ), + ).toThrowErrorMatchingInlineSnapshot( + `"Illegal subject, must be a set of non-space characters"`, + ); + expect(() => + handler.add( + new ConfigReader({ options: { secret: 'b2s=', subject: 3 } }), + ), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'options.subject' in 'mock-config', got number, wanted string"`, + ); + + // new style add, bad access restrictions + expect(() => + handler.add( + new ConfigReader({ + options: { secret: 'b2s=', subject: 'subject' }, + accessRestrictions: [{ plugin: ['a'] }], + }), + ), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'accessRestrictions[0].plugin' in 'mock-config', got array, wanted string"`, + ); // old style add expect(() => @@ -187,18 +261,28 @@ describe('LegacyTokenHandler', () => { ).not.toThrow(); expect(() => handler.addOld(new ConfigReader({ _missingsecret: true })), - ).toThrow(/secret/); - expect(() => handler.addOld(new ConfigReader({ secret: '' }))).toThrow( - /secret/, + ).toThrowErrorMatchingInlineSnapshot( + `"Missing required config value at 'secret' in 'mock-config'"`, + ); + expect(() => + handler.addOld(new ConfigReader({ secret: '' })), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'secret' in 'mock-config', got empty-string, wanted string"`, ); expect(() => handler.addOld(new ConfigReader({ secret: 'has spaces' })), - ).toThrow(/secret/); + ).toThrowErrorMatchingInlineSnapshot( + `"Illegal secret, must be a valid base64 string"`, + ); expect(() => handler.addOld(new ConfigReader({ secret: 'hasnewline\n' })), - ).toThrow(/secret/); - expect(() => handler.addOld(new ConfigReader({ secret: 3 }))).toThrow( - /secret/, + ).toThrowErrorMatchingInlineSnapshot( + `"Illegal secret, must be a valid base64 string"`, + ); + expect(() => + handler.addOld(new ConfigReader({ secret: 3 })), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'secret' in 'mock-config', got number, wanted string"`, ); }); }); diff --git a/packages/backend-app-api/src/services/implementations/auth/external/legacy.ts b/packages/backend-app-api/src/services/implementations/auth/external/legacy.ts index 8448f295a6..ba56d3a213 100644 --- a/packages/backend-app-api/src/services/implementations/auth/external/legacy.ts +++ b/packages/backend-app-api/src/services/implementations/auth/external/legacy.ts @@ -16,7 +16,8 @@ import { Config } from '@backstage/config'; import { base64url, decodeJwt, decodeProtectedHeader, jwtVerify } from 'jose'; -import { TokenHandler } from './types'; +import { readAccessRestrictionsFromConfig } from './helpers'; +import { AccessRestriptionsMap, TokenHandler } from './types'; /** * Handles `type: legacy` access. @@ -24,19 +25,32 @@ import { TokenHandler } from './types'; * @internal */ export class LegacyTokenHandler implements TokenHandler { - #entries: Array<{ key: Uint8Array; subject: string }> = []; + #entries = new Array<{ + key: Uint8Array; + subject: string; + accessRestrictions?: AccessRestriptionsMap; + }>(); - add(options: Config) { - this.#doAdd(options.getString('secret'), options.getString('subject')); + add(config: Config) { + const accessRestrictions = readAccessRestrictionsFromConfig(config); + this.#doAdd( + config.getString('options.secret'), + config.getString('options.subject'), + accessRestrictions, + ); } // used only for the old backend.auth.keys array - addOld(options: Config) { + addOld(config: Config) { // This choice of subject is for compatibility reasons - this.#doAdd(options.getString('secret'), 'external:backstage-plugin'); + this.#doAdd(config.getString('secret'), 'external:backstage-plugin'); } - #doAdd(secret: string, subject: string) { + #doAdd( + secret: string, + subject: string, + accessRestrictions?: AccessRestriptionsMap, + ) { if (!secret.match(/^\S+$/)) { throw new Error('Illegal secret, must be a valid base64 string'); } @@ -52,7 +66,11 @@ export class LegacyTokenHandler implements TokenHandler { throw new Error('Illegal subject, must be a set of non-space characters'); } - this.#entries.push({ key, subject }); + this.#entries.push({ + key, + subject, + accessRestrictions, + }); } async verifyToken(token: string) { @@ -79,7 +97,10 @@ export class LegacyTokenHandler implements TokenHandler { for (const entry of this.#entries) { try { await jwtVerify(token, entry.key); - return { subject: entry.subject }; + return { + subject: entry.subject, + accessRestrictions: entry.accessRestrictions, + }; } catch (e) { if (e.code !== 'ERR_JWS_SIGNATURE_VERIFICATION_FAILED') { throw e; diff --git a/packages/backend-app-api/src/services/implementations/auth/external/static.test.ts b/packages/backend-app-api/src/services/implementations/auth/external/static.test.ts index 86bdf0bdee..458c3f06f3 100644 --- a/packages/backend-app-api/src/services/implementations/auth/external/static.test.ts +++ b/packages/backend-app-api/src/services/implementations/auth/external/static.test.ts @@ -20,14 +20,36 @@ import { StaticTokenHandler } from './static'; describe('StaticTokenHandler', () => { it('accepts any of the added list of tokens', async () => { const handler = new StaticTokenHandler(); - handler.add(new ConfigReader({ token: 'abcabcabc', subject: 'one' })); - handler.add(new ConfigReader({ token: 'defdefdef', subject: 'two' })); + handler.add( + new ConfigReader({ + options: { token: 'abcabcabc', subject: 'one' }, + accessRestrictions: [{ plugin: 'scaffolder' }], + }), + ); + handler.add( + new ConfigReader({ + options: { token: 'defdefdef', subject: 'two' }, + accessRestrictions: [ + { plugin: 'catalog', permission: 'catalog.entity.read' }, + ], + }), + ); + const accessRestrictionsOne = new Map(Object.entries({ scaffolder: {} })); + const accessRestrictionsTwo = new Map( + Object.entries({ + catalog: { + permissionNames: ['catalog.entity.read'], + }, + }), + ); await expect(handler.verifyToken('abcabcabc')).resolves.toEqual({ subject: 'one', + accessRestrictions: accessRestrictionsOne, }); await expect(handler.verifyToken('defdefdef')).resolves.toEqual({ subject: 'two', + accessRestrictions: accessRestrictionsTwo, }); await expect(handler.verifyToken('ghighighi')).resolves.toBeUndefined(); }); @@ -41,71 +63,89 @@ describe('StaticTokenHandler', () => { const handler = new StaticTokenHandler(); expect(() => - handler.add(new ConfigReader({ _missingtoken: true, subject: 'ok' })), + handler.add( + new ConfigReader({ options: { _missingtoken: true, subject: 'ok' } }), + ), ).toThrowErrorMatchingInlineSnapshot( - `"Missing required config value at 'token' in 'mock-config'"`, + `"Missing required config value at 'options.token' in 'mock-config'"`, ); expect(() => - handler.add(new ConfigReader({ token: '', subject: 'ok' })), + handler.add(new ConfigReader({ options: { token: '', subject: 'ok' } })), ).toThrowErrorMatchingInlineSnapshot( - `"Invalid type in config for key 'token' in 'mock-config', got empty-string, wanted string"`, + `"Invalid type in config for key 'options.token' in 'mock-config', got empty-string, wanted string"`, ); expect(() => - handler.add(new ConfigReader({ token: 'has spaces', subject: 'ok' })), + handler.add( + new ConfigReader({ options: { token: 'has spaces', subject: 'ok' } }), + ), ).toThrowErrorMatchingInlineSnapshot( `"Illegal token, must be a set of non-space characters"`, ); expect(() => handler.add( new ConfigReader({ - token: 'hasnewlinebutislongenough\n', - subject: 'ok', + options: { + token: 'hasnewlinebutislongenough\n', + subject: 'ok', + }, }), ), ).toThrowErrorMatchingInlineSnapshot( `"Illegal token, must be a set of non-space characters"`, ); expect(() => - handler.add(new ConfigReader({ token: 'short', subject: 'ok' })), + handler.add( + new ConfigReader({ options: { token: 'short', subject: 'ok' } }), + ), ).toThrowErrorMatchingInlineSnapshot( `"Illegal token, must be at least 8 characters length"`, ); expect(() => - handler.add(new ConfigReader({ token: 3, subject: 'ok' })), + handler.add(new ConfigReader({ options: { token: 3, subject: 'ok' } })), ).toThrowErrorMatchingInlineSnapshot( - `"Invalid type in config for key 'token' in 'mock-config', got number, wanted string"`, + `"Invalid type in config for key 'options.token' in 'mock-config', got number, wanted string"`, ); expect(() => handler.add( - new ConfigReader({ token: 'validtoken', _missingsubject: true }), + new ConfigReader({ + options: { token: 'validtoken', _missingsubject: true }, + }), ), ).toThrowErrorMatchingInlineSnapshot( - `"Missing required config value at 'subject' in 'mock-config'"`, - ); - expect(() => - handler.add(new ConfigReader({ token: 'validtoken', subject: '' })), - ).toThrowErrorMatchingInlineSnapshot( - `"Invalid type in config for key 'subject' in 'mock-config', got empty-string, wanted string"`, + `"Missing required config value at 'options.subject' in 'mock-config'"`, ); expect(() => handler.add( - new ConfigReader({ token: 'validtoken', subject: 'has spaces' }), + new ConfigReader({ options: { token: 'validtoken', subject: '' } }), + ), + ).toThrowErrorMatchingInlineSnapshot( + `"Invalid type in config for key 'options.subject' in 'mock-config', got empty-string, wanted string"`, + ); + expect(() => + handler.add( + new ConfigReader({ + options: { token: 'validtoken', subject: 'has spaces' }, + }), ), ).toThrowErrorMatchingInlineSnapshot( `"Illegal subject, must be a set of non-space characters"`, ); expect(() => handler.add( - new ConfigReader({ token: 'validtoken', subject: 'hasnewline\n' }), + new ConfigReader({ + options: { token: 'validtoken', subject: 'hasnewline\n' }, + }), ), ).toThrowErrorMatchingInlineSnapshot( `"Illegal subject, must be a set of non-space characters"`, ); expect(() => - handler.add(new ConfigReader({ token: 'validtoken', subject: 3 })), + handler.add( + new ConfigReader({ options: { token: 'validtoken', subject: 3 } }), + ), ).toThrowErrorMatchingInlineSnapshot( - `"Invalid type in config for key 'subject' in 'mock-config', got number, wanted string"`, + `"Invalid type in config for key 'options.subject' in 'mock-config', got number, wanted string"`, ); }); }); diff --git a/packages/backend-app-api/src/services/implementations/auth/external/static.ts b/packages/backend-app-api/src/services/implementations/auth/external/static.ts index bae8e05f2b..8242b87dc0 100644 --- a/packages/backend-app-api/src/services/implementations/auth/external/static.ts +++ b/packages/backend-app-api/src/services/implementations/auth/external/static.ts @@ -15,7 +15,8 @@ */ import { Config } from '@backstage/config'; -import { TokenHandler } from './types'; +import { readAccessRestrictionsFromConfig } from './helpers'; +import { AccessRestriptionsMap, TokenHandler } from './types'; const MIN_TOKEN_LENGTH = 8; @@ -25,10 +26,14 @@ const MIN_TOKEN_LENGTH = 8; * @internal */ export class StaticTokenHandler implements TokenHandler { - #entries: Array<{ token: string; subject: string }> = []; + #entries = new Array<{ + token: string; + subject: string; + accessRestrictions?: AccessRestriptionsMap; + }>(); - add(options: Config) { - const token = options.getString('token'); + add(config: Config) { + const token = config.getString('options.token'); if (!token.match(/^\S+$/)) { throw new Error('Illegal token, must be a set of non-space characters'); } @@ -38,12 +43,18 @@ export class StaticTokenHandler implements TokenHandler { ); } - const subject = options.getString('subject'); + const subject = config.getString('options.subject'); if (!subject.match(/^\S+$/)) { throw new Error('Illegal subject, must be a set of non-space characters'); } - this.#entries.push({ token, subject }); + const accessRestrictions = readAccessRestrictionsFromConfig(config); + + this.#entries.push({ + token, + subject, + accessRestrictions, + }); } async verifyToken(token: string) { @@ -52,6 +63,9 @@ export class StaticTokenHandler implements TokenHandler { return undefined; } - return { subject: entry.subject }; + return { + subject: entry.subject, + accessRestrictions: entry.accessRestrictions, + }; } } diff --git a/packages/backend-app-api/src/services/implementations/auth/external/types.ts b/packages/backend-app-api/src/services/implementations/auth/external/types.ts index 5d33f09a22..54e51c1ef6 100644 --- a/packages/backend-app-api/src/services/implementations/auth/external/types.ts +++ b/packages/backend-app-api/src/services/implementations/auth/external/types.ts @@ -14,9 +14,21 @@ * limitations under the License. */ +import { BackstagePrincipalAccessRestrictions } from '@backstage/backend-plugin-api'; import { Config } from '@backstage/config'; +export type AccessRestriptionsMap = Map< + string, // plugin ID + BackstagePrincipalAccessRestrictions +>; + export interface TokenHandler { add(options: Config): void; - verifyToken(token: string): Promise<{ subject: string } | undefined>; + verifyToken(token: string): Promise< + | { + subject: string; + accessRestrictions?: AccessRestriptionsMap; + } + | undefined + >; } diff --git a/packages/backend-app-api/src/services/implementations/auth/helpers.ts b/packages/backend-app-api/src/services/implementations/auth/helpers.ts index 01fb9537a1..eebe45eb76 100644 --- a/packages/backend-app-api/src/services/implementations/auth/helpers.ts +++ b/packages/backend-app-api/src/services/implementations/auth/helpers.ts @@ -17,6 +17,7 @@ import { BackstageCredentials, BackstageNonePrincipal, + BackstagePrincipalAccessRestrictions, BackstageServicePrincipal, BackstageUserPrincipal, } from '@backstage/backend-plugin-api'; @@ -25,6 +26,7 @@ import { InternalBackstageCredentials } from './types'; export function createCredentialsWithServicePrincipal( sub: string, token?: string, + accessRestrictions?: BackstagePrincipalAccessRestrictions, ): InternalBackstageCredentials { return { $$type: '@backstage/BackstageCredentials', @@ -33,6 +35,7 @@ export function createCredentialsWithServicePrincipal( principal: { type: 'service', subject: sub, + accessRestrictions, }, }; } diff --git a/packages/backend-app-api/src/services/implementations/permissions/permissionsServiceFactory.ts b/packages/backend-app-api/src/services/implementations/permissions/permissionsServiceFactory.ts index c8fa0e7bbf..ecdaa1e938 100644 --- a/packages/backend-app-api/src/services/implementations/permissions/permissionsServiceFactory.ts +++ b/packages/backend-app-api/src/services/implementations/permissions/permissionsServiceFactory.ts @@ -31,12 +31,14 @@ export const permissionsServiceFactory = createServiceFactory({ config: coreServices.rootConfig, discovery: coreServices.discovery, tokenManager: coreServices.tokenManager, + pluginMetadata: coreServices.pluginMetadata, }, - async factory({ auth, config, discovery, tokenManager }) { + async factory({ auth, config, discovery, tokenManager, pluginMetadata }) { return ServerPermissionClient.fromConfig(config, { auth, discovery, tokenManager, + pluginId: pluginMetadata.getId(), }); }, }); diff --git a/packages/backend-plugin-api/api-report.md b/packages/backend-plugin-api/api-report.md index c65fc5cf3e..134e4b61da 100644 --- a/packages/backend-plugin-api/api-report.md +++ b/packages/backend-plugin-api/api-report.md @@ -16,6 +16,7 @@ import { isChildPath } from '@backstage/cli-common'; import { JsonObject } from '@backstage/types'; import { JsonValue } from '@backstage/types'; import { Knex } from 'knex'; +import { PermissionAttributes } from '@backstage/plugin-permission-common'; import { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { QueryPermissionRequest } from '@backstage/plugin-permission-common'; import { QueryPermissionResponse } from '@backstage/plugin-permission-common'; @@ -136,6 +137,14 @@ export type BackstageNonePrincipal = { type: 'none'; }; +// @public +export type BackstagePrincipalAccessRestrictions = { + permissionNames?: string[]; + permissionAttributes?: { + action?: Array['action']>; + }; +}; + // @public (undocumented) export type BackstagePrincipalTypes = { user: BackstageUserPrincipal; @@ -148,6 +157,7 @@ export type BackstagePrincipalTypes = { export type BackstageServicePrincipal = { type: 'service'; subject: string; + accessRestrictions?: BackstagePrincipalAccessRestrictions; }; // @public (undocumented) diff --git a/packages/backend-plugin-api/src/services/definitions/AuthService.ts b/packages/backend-plugin-api/src/services/definitions/AuthService.ts index 827b4122cd..000ce8bbb1 100644 --- a/packages/backend-plugin-api/src/services/definitions/AuthService.ts +++ b/packages/backend-plugin-api/src/services/definitions/AuthService.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { PermissionAttributes } from '@backstage/plugin-permission-common'; import { JsonObject } from '@backstage/types'; /** @@ -40,6 +41,54 @@ export type BackstageServicePrincipal = { // Exact format TBD, possibly 'plugin:' or 'external:' subject: string; + + /** + * The access restrictions that apply to this principal. + * + * @remarks + * + * If no access restrictions are provided the principal is assumed to have + * unlimited access, at a framework level. The permissions system and + * individual plugins may or may not still apply additional access controls on + * top of this. + */ + accessRestrictions?: BackstagePrincipalAccessRestrictions; +}; + +/** + * The access restrictions that apply to a given principal. + * + * @public + */ +export type BackstagePrincipalAccessRestrictions = { + /** + * If given, the principal is limited to only performing actions with these + * named permissions. + * + * Note that this only applies where permissions checks are enabled in the + * first place. Endpoints that are not protected by the permissions system at + * all, are not affected by this setting. + * + * This array always has at least one element, or is missing entirely. + */ + permissionNames?: string[]; + /** + * If given, the principal is limited to only performing actions whose + * permissions have these attributes. + * + * Note that this only applies where permissions checks are enabled in the + * first place. Endpoints that are not protected by the permissions system at + * all, are not affected by this setting. + * + * This object always has at least one key, or is missing entirely. + */ + permissionAttributes?: { + /** + * Match any of these action values. This array always has at least one + * element, or is missing entirely. + */ + action?: Array['action']>; + }; }; /** diff --git a/packages/backend-plugin-api/src/services/definitions/index.ts b/packages/backend-plugin-api/src/services/definitions/index.ts index 5739add90c..f22213a285 100644 --- a/packages/backend-plugin-api/src/services/definitions/index.ts +++ b/packages/backend-plugin-api/src/services/definitions/index.ts @@ -20,6 +20,7 @@ export type { BackstageCredentials, BackstageUserPrincipal, BackstageServicePrincipal, + BackstagePrincipalAccessRestrictions, BackstagePrincipalTypes, BackstageNonePrincipal, } from './AuthService'; diff --git a/packages/backend-test-utils/api-report.md b/packages/backend-test-utils/api-report.md index 4f6eb47243..5a192cb61f 100644 --- a/packages/backend-test-utils/api-report.md +++ b/packages/backend-test-utils/api-report.md @@ -11,6 +11,7 @@ import { Backend } from '@backstage/backend-app-api'; import { BackendFeature } from '@backstage/backend-plugin-api'; import { BackstageCredentials } from '@backstage/backend-plugin-api'; import { BackstageNonePrincipal } from '@backstage/backend-plugin-api'; +import { BackstagePrincipalAccessRestrictions } from '@backstage/backend-plugin-api'; import { BackstageServicePrincipal } from '@backstage/backend-plugin-api'; import { BackstageUserInfo } from '@backstage/backend-plugin-api'; import { BackstageUserPrincipal } from '@backstage/backend-plugin-api'; @@ -68,6 +69,7 @@ export namespace mockCredentials { } export function service( subject?: string, + accessRestrictions?: BackstagePrincipalAccessRestrictions, ): BackstageCredentials; export namespace service { export function header(options?: TokenOptions): string; diff --git a/packages/backend-test-utils/src/next/services/mockCredentials.test.ts b/packages/backend-test-utils/src/next/services/mockCredentials.test.ts index ed0071d4b8..343d6d6d20 100644 --- a/packages/backend-test-utils/src/next/services/mockCredentials.test.ts +++ b/packages/backend-test-utils/src/next/services/mockCredentials.test.ts @@ -134,6 +134,16 @@ describe('mockCredentials', () => { expect(mockCredentials.service.invalidHeader()).toBe( 'Bearer mock-invalid-service-token', ); + expect( + mockCredentials.service('test', { permissionNames: ['do.it'] }), + ).toEqual({ + $$type: '@backstage/BackstageCredentials', + principal: { + type: 'service', + subject: 'test', + accessRestrictions: { permissionNames: ['do.it'] }, + }, + }); }); it('should throw on invalid user entity refs', () => { diff --git a/packages/backend-test-utils/src/next/services/mockCredentials.ts b/packages/backend-test-utils/src/next/services/mockCredentials.ts index 16d2381c73..4214320142 100644 --- a/packages/backend-test-utils/src/next/services/mockCredentials.ts +++ b/packages/backend-test-utils/src/next/services/mockCredentials.ts @@ -17,6 +17,7 @@ import { BackstageCredentials, BackstageNonePrincipal, + BackstagePrincipalAccessRestrictions, BackstageServicePrincipal, BackstageUserPrincipal, } from '@backstage/backend-plugin-api'; @@ -202,14 +203,19 @@ export namespace mockCredentials { /** * Creates a mocked credentials object for a service principal. * - * The default subject is 'external:test-service'. + * The default subject is 'external:test-service', and no access restrictions. */ export function service( subject: string = DEFAULT_MOCK_SERVICE_SUBJECT, + accessRestrictions?: BackstagePrincipalAccessRestrictions, ): BackstageCredentials { return { $$type: '@backstage/BackstageCredentials', - principal: { type: 'service', subject }, + principal: { + type: 'service', + subject, + ...(accessRestrictions ? { accessRestrictions } : {}), + }, }; } diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 7d2910d391..fab3bc7742 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -291,6 +291,7 @@ export class ServerPermissionClient implements PermissionsService { discovery: DiscoveryService; tokenManager: TokenManager; auth?: AuthService; + pluginId?: string; }, ): ServerPermissionClient; } diff --git a/plugins/permission-node/src/ServerPermissionClient.test.ts b/plugins/permission-node/src/ServerPermissionClient.test.ts index 215f624fe9..68c675745a 100644 --- a/plugins/permission-node/src/ServerPermissionClient.test.ts +++ b/plugins/permission-node/src/ServerPermissionClient.test.ts @@ -48,7 +48,9 @@ const discovery: PluginEndpointDiscovery = { }; const testBasicPermission = createPermission({ name: 'test.permission', - attributes: {}, + attributes: { + action: 'create', + }, }); const testResourcePermission = createPermission({ @@ -362,4 +364,66 @@ describe('ServerPermissionClient', () => { }); }); }); + + describe('with access restrictions', () => { + it('short circuits the response when relevant access restrictions are present', async () => { + const client = ServerPermissionClient.fromConfig(config, { + discovery, + tokenManager: mockServices.tokenManager(), + auth: mockServices.auth(), + pluginId: 'test', + }); + + // no restrictions for the given plugin + await expect( + client.authorize([{ permission: testBasicPermission }], { + credentials: mockCredentials.service('foo', {}), + }), + ).resolves.toEqual([{ result: AuthorizeResult.ALLOW }]); + + // matching permission name + await expect( + client.authorize([{ permission: testBasicPermission }], { + credentials: mockCredentials.service('foo', { + permissionNames: [testBasicPermission.name, 'other'], + }), + }), + ).resolves.toEqual([{ result: AuthorizeResult.ALLOW }]); + + // matching attributes + await expect( + client.authorize([{ permission: testBasicPermission }], { + credentials: mockCredentials.service('foo', { + permissionAttributes: { + action: [testBasicPermission.attributes.action!, 'other' as any], + }, + }), + }), + ).resolves.toEqual([{ result: AuthorizeResult.ALLOW }]); + + // matching permission name but not attributes + await expect( + client.authorize([{ permission: testBasicPermission }], { + credentials: mockCredentials.service('foo', { + permissionNames: [testBasicPermission.name], + permissionAttributes: { + action: ['other' as any], + }, + }), + }), + ).resolves.toEqual([{ result: AuthorizeResult.DENY }]); + + // matching attributes but not permission name + await expect( + client.authorize([{ permission: testBasicPermission }], { + credentials: mockCredentials.service('foo', { + permissionNames: ['wrong-name'], + permissionAttributes: { + action: [testBasicPermission.attributes.action!], + }, + }), + }), + ).resolves.toEqual([{ result: AuthorizeResult.DENY }]); + }); + }); }); diff --git a/plugins/permission-node/src/ServerPermissionClient.ts b/plugins/permission-node/src/ServerPermissionClient.ts index f8570fee74..2c2ccefbf4 100644 --- a/plugins/permission-node/src/ServerPermissionClient.ts +++ b/plugins/permission-node/src/ServerPermissionClient.ts @@ -33,18 +33,21 @@ import { AuthorizePermissionResponse, PolicyDecision, QueryPermissionRequest, + DefinitivePolicyDecision, } from '@backstage/plugin-permission-common'; /** * A thin wrapper around - * {@link @backstage/plugin-permission-common#PermissionClient} that allows all - * service-to-service requests. + * {@link @backstage/plugin-permission-common#PermissionClient} that ensures the + * proper short-circuit handling of service principals. + * * @public */ export class ServerPermissionClient implements PermissionsService { readonly #auth: AuthService; readonly #permissionClient: PermissionClient; readonly #permissionEnabled: boolean; + readonly #pluginId?: string; static fromConfig( config: Config, @@ -52,6 +55,7 @@ export class ServerPermissionClient implements PermissionsService { discovery: DiscoveryService; tokenManager: TokenManager; auth?: AuthService; + pluginId?: string; }, ) { const { discovery, tokenManager } = options; @@ -74,6 +78,7 @@ export class ServerPermissionClient implements PermissionsService { auth, permissionClient, permissionEnabled, + pluginId: options.pluginId, }); } @@ -81,16 +86,26 @@ export class ServerPermissionClient implements PermissionsService { auth: AuthService; permissionClient: PermissionClient; permissionEnabled: boolean; + pluginId?: string; }) { this.#auth = options.auth; this.#permissionClient = options.permissionClient; this.#permissionEnabled = options.permissionEnabled; + this.#pluginId = options.pluginId; } async authorizeConditional( queries: QueryPermissionRequest[], options?: PermissionsServiceRequestOptions, ): Promise { + const maybeResponse = this.#decideBasedOnPrincipalAccessRestrictions( + queries, + options, + ); + if (maybeResponse) { + return maybeResponse; + } + if (await this.#shouldPermissionsBeApplied(options)) { return this.#permissionClient.authorizeConditional( queries, @@ -105,6 +120,14 @@ export class ServerPermissionClient implements PermissionsService { requests: AuthorizePermissionRequest[], options?: PermissionsServiceRequestOptions, ): Promise { + const maybeResponse = this.#decideBasedOnPrincipalAccessRestrictions( + requests, + options, + ); + if (maybeResponse) { + return maybeResponse; + } + if (await this.#shouldPermissionsBeApplied(options)) { return this.#permissionClient.authorize( requests, @@ -130,6 +153,44 @@ export class ServerPermissionClient implements PermissionsService { return options; } + #decideBasedOnPrincipalAccessRestrictions( + requests: Array, + options?: PermissionsServiceRequestOptions, + ): DefinitivePolicyDecision[] | undefined { + if (!options || !('credentials' in options)) { + return undefined; + } + + // Bail out to the old behavior if + // - the principal is not a service + // - the principal was apparently unrestricted + // - we are in legacy mode because nobody passed in a plugin ID + const credentials = options.credentials; + if ( + !this.#auth.isPrincipal(credentials, 'service') || + !credentials.principal.accessRestrictions || + !this.#pluginId + ) { + return undefined; + } + + const { permissionNames, permissionAttributes } = + credentials.principal.accessRestrictions; + + return requests.map(query => { + if (permissionNames && !permissionNames.includes(query.permission.name)) { + return { result: AuthorizeResult.DENY }; + } + if (permissionAttributes?.action) { + const action = query.permission.attributes?.action; + if (!action || !permissionAttributes.action.includes(action)) { + return { result: AuthorizeResult.DENY }; + } + } + return { result: AuthorizeResult.ALLOW }; + }); + } + async #shouldPermissionsBeApplied( options?: PermissionsServiceRequestOptions, ) {