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 4eb982314b..9c60e70707 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 @@ -66,6 +66,12 @@ export class LegacyTokenHandler implements TokenHandler { throw new Error('Illegal secret, must be a valid base64 string'); } + if (this.#entries.some(e => e.key === key)) { + throw new Error( + 'Legacy externalAccess token was declared more than once', + ); + } + this.#entries.push({ key, result: { diff --git a/plugins/permission-node/src/ServerPermissionClient.test.ts b/plugins/permission-node/src/ServerPermissionClient.test.ts index 9becad4d7b..8273606845 100644 --- a/plugins/permission-node/src/ServerPermissionClient.test.ts +++ b/plugins/permission-node/src/ServerPermissionClient.test.ts @@ -366,63 +366,242 @@ 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(), - }); - - // 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'], + it.each([{ enabled: true }, { enabled: false }])( + 'short circuits the response when using a service principal, applying the relevant access restrictions if present, when permissions %p', + async permissionConfig => { + const client = ServerPermissionClient.fromConfig( + new ConfigReader({ + permission: permissionConfig, }), - }), - ).resolves.toEqual([{ result: AuthorizeResult.ALLOW }]); + { + discovery, + tokenManager: mockServices.tokenManager(), + auth: mockServices.auth(), + }, + ); - // matching attributes - await expect( - client.authorize([{ permission: testBasicPermission }], { - credentials: mockCredentials.service('foo', { - permissionAttributes: { - action: [testBasicPermission.attributes.action!, 'other' as any], + // no restrictions for the given plugin + await expect( + client.authorize( + [ + { + permission: createPermission({ + name: 'test.permission', + attributes: { + action: 'create', + }, + }), + }, + ], + { + credentials: mockCredentials.service('foo', {}), }, - }), - }), - ).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.ALLOW }]); + await expect( + client.authorizeConditional( + [ + { + resourceRef: undefined as any, + permission: createPermission({ + resourceType: 'test', + name: 'test.permission', + attributes: { + action: 'create', + }, + }), + }, + ], + { + credentials: mockCredentials.service('foo', {}), }, - }), - }), - ).resolves.toEqual([{ result: AuthorizeResult.DENY }]); + ), + ).resolves.toEqual([{ result: AuthorizeResult.ALLOW }]); - // matching attributes but not permission name - await expect( - client.authorize([{ permission: testBasicPermission }], { - credentials: mockCredentials.service('foo', { - permissionNames: ['wrong-name'], - permissionAttributes: { - action: [testBasicPermission.attributes.action!], + // matching permission name + await expect( + client.authorize( + [ + { + permission: createPermission({ + name: 'test.permission', + attributes: { + action: 'create', + }, + }), + }, + ], + { + credentials: mockCredentials.service('foo', { + permissionNames: ['test.permission', 'other'], + }), }, - }), - }), - ).resolves.toEqual([{ result: AuthorizeResult.DENY }]); - }); + ), + ).resolves.toEqual([{ result: AuthorizeResult.ALLOW }]); + await expect( + client.authorizeConditional( + [ + { + resourceRef: undefined as any, + permission: createPermission({ + resourceType: 'test', + name: 'test.permission', + attributes: { + action: 'create', + }, + }), + }, + ], + { + credentials: mockCredentials.service('foo', { + permissionNames: ['test.permission', 'other'], + }), + }, + ), + ).resolves.toEqual([{ result: AuthorizeResult.ALLOW }]); + + // matching attributes + await expect( + client.authorize( + [ + { + permission: createPermission({ + name: 'test.permission', + attributes: { + action: 'create', + }, + }), + }, + ], + { + credentials: mockCredentials.service('foo', { + permissionAttributes: { + action: ['create', 'other' as any], + }, + }), + }, + ), + ).resolves.toEqual([{ result: AuthorizeResult.ALLOW }]); + await expect( + client.authorizeConditional( + [ + { + resourceRef: undefined as any, + permission: createPermission({ + resourceType: 'test', + name: 'test.permission', + attributes: { + action: 'create', + }, + }), + }, + ], + { + credentials: mockCredentials.service('foo', { + permissionAttributes: { + action: ['create', 'other' as any], + }, + }), + }, + ), + ).resolves.toEqual([{ result: AuthorizeResult.ALLOW }]); + + // matching permission name but not attributes + await expect( + client.authorize( + [ + { + permission: createPermission({ + name: 'test.permission', + attributes: { + action: 'create', + }, + }), + }, + ], + { + credentials: mockCredentials.service('foo', { + permissionNames: ['test.permission'], + permissionAttributes: { + action: ['other' as any], + }, + }), + }, + ), + ).resolves.toEqual([{ result: AuthorizeResult.DENY }]); + await expect( + client.authorizeConditional( + [ + { + resourceRef: undefined as any, + permission: createPermission({ + resourceType: 'test', + name: 'test.permission', + attributes: { + action: 'create', + }, + }), + }, + ], + { + credentials: mockCredentials.service('foo', { + permissionNames: ['test.permission'], + permissionAttributes: { + action: ['other' as any], + }, + }), + }, + ), + ).resolves.toEqual([{ result: AuthorizeResult.DENY }]); + + // matching attributes but not permission name + await expect( + client.authorize( + [ + { + permission: createPermission({ + name: 'test.permission', + attributes: { + action: 'create', + }, + }), + }, + ], + { + credentials: mockCredentials.service('foo', { + permissionNames: ['wrong-name'], + permissionAttributes: { + action: ['create'], + }, + }), + }, + ), + ).resolves.toEqual([{ result: AuthorizeResult.DENY }]); + await expect( + client.authorizeConditional( + [ + { + resourceRef: undefined as any, + permission: createPermission({ + resourceType: 'test', + name: 'test.permission', + attributes: { + action: 'create', + }, + }), + }, + ], + { + credentials: mockCredentials.service('foo', { + permissionNames: ['wrong-name'], + permissionAttributes: { + action: ['create'], + }, + }), + }, + ), + ).resolves.toEqual([{ result: AuthorizeResult.DENY }]); + }, + ); }); }); diff --git a/plugins/permission-node/src/ServerPermissionClient.ts b/plugins/permission-node/src/ServerPermissionClient.ts index 6e3901d65b..cb39f310bd 100644 --- a/plugins/permission-node/src/ServerPermissionClient.ts +++ b/plugins/permission-node/src/ServerPermissionClient.ts @@ -21,26 +21,27 @@ import { import { AuthService, BackstageCredentials, + BackstageServicePrincipal, DiscoveryService, PermissionsService, PermissionsServiceRequestOptions, } from '@backstage/backend-plugin-api'; import { Config } from '@backstage/config'; import { - AuthorizeResult, - PermissionClient, AuthorizePermissionRequest, AuthorizePermissionResponse, + AuthorizeResult, + DefinitivePolicyDecision, + Permission, + PermissionClient, PolicyDecision, QueryPermissionRequest, - DefinitivePolicyDecision, } from '@backstage/plugin-permission-common'; /** * A thin wrapper around - * {@link @backstage/plugin-permission-common#PermissionClient} that ensures the - * proper short-circuit handling of service principals. - * + * {@link @backstage/plugin-permission-common#PermissionClient} that allows all + * service-to-service requests. * @public */ export class ServerPermissionClient implements PermissionsService { @@ -93,47 +94,39 @@ export class ServerPermissionClient implements PermissionsService { queries: QueryPermissionRequest[], options?: PermissionsServiceRequestOptions, ): Promise { - const maybeResponse = this.#decideBasedOnPrincipalAccessRestrictions( + const credentials = await this.#getIncomingCredentials(options); + if (credentials && this.#auth.isPrincipal(credentials, 'service')) { + return this.#servicePrincipalDecision(queries, credentials); + } else if (!this.#permissionEnabled) { + return queries.map(_ => ({ result: AuthorizeResult.ALLOW })); + } + + return this.#permissionClient.authorizeConditional( queries, - options, + await this.#getRequestOptions(options), ); - if (maybeResponse) { - return maybeResponse; - } - - if (await this.#shouldPermissionsBeApplied(options)) { - return this.#permissionClient.authorizeConditional( - queries, - await this.#getRequestOptions(options), - ); - } - - return queries.map(_ => ({ result: AuthorizeResult.ALLOW })); } async authorize( requests: AuthorizePermissionRequest[], options?: PermissionsServiceRequestOptions, ): Promise { - const maybeResponse = this.#decideBasedOnPrincipalAccessRestrictions( + const credentials = await this.#getIncomingCredentials(options); + if (credentials && this.#auth.isPrincipal(credentials, 'service')) { + return this.#servicePrincipalDecision(requests, credentials); + } else if (!this.#permissionEnabled) { + return requests.map(_ => ({ result: AuthorizeResult.ALLOW })); + } + + return this.#permissionClient.authorize( requests, - options, + await this.#getRequestOptions(options), ); - if (maybeResponse) { - return maybeResponse; - } - - if (await this.#shouldPermissionsBeApplied(options)) { - return this.#permissionClient.authorize( - requests, - await this.#getRequestOptions(options), - ); - } - - return requests.map(_ => ({ result: AuthorizeResult.ALLOW })); } - async #getRequestOptions(options?: PermissionsServiceRequestOptions) { + async #getRequestOptions( + options?: PermissionsServiceRequestOptions, + ): Promise<{ token?: string } | undefined> { if (options && 'credentials' in options) { if (this.#auth.isPrincipal(options.credentials, 'none')) { return {}; @@ -148,66 +141,48 @@ export class ServerPermissionClient implements PermissionsService { return options; } - #decideBasedOnPrincipalAccessRestrictions( - requests: Array, + async #getIncomingCredentials( options?: PermissionsServiceRequestOptions, - ): DefinitivePolicyDecision[] | undefined { - if (!options || !('credentials' in options)) { - return undefined; + ): Promise { + if (options && 'credentials' in options) { + return options.credentials; } - // Bail out to the old behavior if - // - the principal is not a service - // - the principal was apparently unrestricted - const credentials = options.credentials; - if ( - !this.#auth.isPrincipal(credentials, 'service') || - !credentials.principal.accessRestrictions - ) { - return undefined; + if (options?.token) { + try { + return await this.#auth.authenticate(options.token); + } catch { + // ignore + } } + return undefined; + } + + /** + * For service principals, we can always make an immediate definitive decision + * based on their associated access restrictions (if any). + */ + #servicePrincipalDecision( + input: { permission: Permission }[], + credentials: BackstageCredentials, + ): DefinitivePolicyDecision[] { const { permissionNames, permissionAttributes } = - credentials.principal.accessRestrictions; + credentials.principal.accessRestrictions ?? {}; - return requests.map(query => { - if (permissionNames && !permissionNames.includes(query.permission.name)) { + return input.map(item => { + if (permissionNames && !permissionNames.includes(item.permission.name)) { return { result: AuthorizeResult.DENY }; } + if (permissionAttributes?.action) { - const action = query.permission.attributes?.action; + const action = item.permission.attributes?.action; if (!action || !permissionAttributes.action.includes(action)) { return { result: AuthorizeResult.DENY }; } } + return { result: AuthorizeResult.ALLOW }; }); } - - async #shouldPermissionsBeApplied( - options?: PermissionsServiceRequestOptions, - ) { - if (!this.#permissionEnabled) { - return false; - } - - let credentials: BackstageCredentials; - if (options && 'credentials' in options) { - credentials = options.credentials; - } else { - if (!options?.token) { - return true; - } - try { - credentials = await this.#auth.authenticate(options.token); - } catch { - return true; - } - } - - if (this.#auth.isPrincipal(credentials, 'service')) { - return false; - } - return true; - } }