From 96865ec71ec7f5b5bd561cd4216747d91a7f786e Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 8 Apr 2025 21:21:20 +0200 Subject: [PATCH 01/18] permission: improve validation Signed-off-by: Vincenzo Scamporlino --- .../permission-backend/src/service/router.ts | 80 ++++++++++++++----- 1 file changed, 60 insertions(+), 20 deletions(-) diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index c624947375..bcb066c129 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -21,6 +21,7 @@ import { createLegacyAuthAdapters } from '@backstage/backend-common'; import { InputError } from '@backstage/errors'; import { IdentityApi } from '@backstage/plugin-auth-node'; import { + AuthorizePermissionRequest, AuthorizeResult, EvaluatePermissionRequest, EvaluatePermissionRequestBatch, @@ -29,6 +30,7 @@ import { IdentifiedPermissionMessage, isResourcePermission, PermissionAttributes, + PermissionMessageBatch, } from '@backstage/plugin-permission-common'; import { ApplyConditionsRequestEntry, @@ -62,31 +64,46 @@ const attributesSchema: z.ZodSchema = z.object({ .optional(), }); -const permissionSchema = z.union([ - z.object({ - type: z.literal('basic'), - name: z.string(), - attributes: attributesSchema, - }), - z.object({ - type: z.literal('resource'), - name: z.string(), - attributes: attributesSchema, - resourceType: z.string(), - }), -]); +const basicPermissionSchema = z.object({ + type: z.literal('basic'), + name: z.string(), + attributes: attributesSchema, +}); -const evaluatePermissionRequestSchema: z.ZodSchema< - IdentifiedPermissionMessage +const resourcePermissionSchema = z.object({ + type: z.literal('resource'), + name: z.string(), + attributes: attributesSchema, + resourceType: z.string(), +}); + +const authorizePermissionRequestBatchSchema: z.ZodSchema< + PermissionMessageBatch > = z.object({ - id: z.string(), - resourceRef: z.string().optional(), - permission: permissionSchema, + items: z.array( + z.union([ + z.object({ + id: z.string(), + permission: basicPermissionSchema, + }), + z.object({ + id: z.string(), + resourceRef: z.string(), + permission: resourcePermissionSchema, + }), + ]), + ), }); const evaluatePermissionRequestBatchSchema: z.ZodSchema = z.object({ - items: z.array(evaluatePermissionRequestSchema), + items: z.array( + z.object({ + id: z.string(), + resourceRef: z.string().optional(), + permission: resourcePermissionSchema, + }), + ), }); /** @@ -225,7 +242,30 @@ export async function createRouter( allow: ['user', 'none'], }); - const parseResult = evaluatePermissionRequestBatchSchema.safeParse( + // TODO(vinzscam): make some magic + const isServicePrincipal = false; + + if (isServicePrincipal) { + const parsedResult = evaluatePermissionRequestBatchSchema.safeParse( + req.body, + ); + + if (parsedResult.success) { + res.json({ + items: await handleRequest( + parsedResult.data.items, + policy, + permissionIntegrationClient, + credentials, + auth, + userInfo, + ), + }); + return; + } + } + + const parseResult = authorizePermissionRequestBatchSchema.safeParse( req.body, ); From 86d5f4a07ca9a2545e26e4fcb170ef0d9a4ca08b Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 9 Apr 2025 11:52:48 +0200 Subject: [PATCH 02/18] backend-defaults: add via to credentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Co-authored-by: Patrik Oldsberg Signed-off-by: Vincenzo Scamporlino --- .../backend-defaults/src/entrypoints/auth/DefaultAuthService.ts | 1 + packages/backend-defaults/src/entrypoints/auth/helpers.ts | 2 ++ .../backend-plugin-api/src/services/definitions/AuthService.ts | 2 ++ 3 files changed, 5 insertions(+) diff --git a/packages/backend-defaults/src/entrypoints/auth/DefaultAuthService.ts b/packages/backend-defaults/src/entrypoints/auth/DefaultAuthService.ts index a81a99fec7..189976ed7d 100644 --- a/packages/backend-defaults/src/entrypoints/auth/DefaultAuthService.ts +++ b/packages/backend-defaults/src/entrypoints/auth/DefaultAuthService.ts @@ -68,6 +68,7 @@ export class DefaultAuthService implements AuthService { userResult.userEntityRef, pluginResult.limitedUserToken, this.#getJwtExpiration(pluginResult.limitedUserToken), + pluginResult.subject, ); } return createCredentialsWithServicePrincipal(pluginResult.subject); diff --git a/packages/backend-defaults/src/entrypoints/auth/helpers.ts b/packages/backend-defaults/src/entrypoints/auth/helpers.ts index dad36670b9..266d3e86b8 100644 --- a/packages/backend-defaults/src/entrypoints/auth/helpers.ts +++ b/packages/backend-defaults/src/entrypoints/auth/helpers.ts @@ -51,6 +51,7 @@ export function createCredentialsWithUserPrincipal( sub: string, token: string, expiresAt?: Date, + viaSubject?: string, ): InternalBackstageCredentials { return Object.defineProperty( { @@ -60,6 +61,7 @@ export function createCredentialsWithUserPrincipal( principal: { type: 'user', userEntityRef: sub, + ...(viaSubject && { via: { type: 'service', subject: viaSubject } }), }, }, 'token', diff --git a/packages/backend-plugin-api/src/services/definitions/AuthService.ts b/packages/backend-plugin-api/src/services/definitions/AuthService.ts index 1e0cf6e56b..eeac3ea924 100644 --- a/packages/backend-plugin-api/src/services/definitions/AuthService.ts +++ b/packages/backend-plugin-api/src/services/definitions/AuthService.ts @@ -35,6 +35,8 @@ export type BackstageUserPrincipal = { * The entity ref of the user entity that this principal represents. */ userEntityRef: string; + + via?: BackstageServicePrincipal; }; /** From 42b29f826ee6a1c1754dd55140de17d9bc5f741e Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 9 Apr 2025 15:17:07 +0200 Subject: [PATCH 03/18] backend-defaults: rename viaSubject to issuedBy Signed-off-by: Vincenzo Scamporlino --- .../src/entrypoints/auth/DefaultAuthService.ts | 1 + .../backend-defaults/src/entrypoints/auth/helpers.ts | 6 ++++-- .../src/services/definitions/AuthService.ts | 11 ++++++++++- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/packages/backend-defaults/src/entrypoints/auth/DefaultAuthService.ts b/packages/backend-defaults/src/entrypoints/auth/DefaultAuthService.ts index 189976ed7d..511c11ce40 100644 --- a/packages/backend-defaults/src/entrypoints/auth/DefaultAuthService.ts +++ b/packages/backend-defaults/src/entrypoints/auth/DefaultAuthService.ts @@ -87,6 +87,7 @@ export class DefaultAuthService implements AuthService { userResult.userEntityRef, token, this.#getJwtExpiration(token), + this.pluginId, ); } diff --git a/packages/backend-defaults/src/entrypoints/auth/helpers.ts b/packages/backend-defaults/src/entrypoints/auth/helpers.ts index 266d3e86b8..2023cb9c2c 100644 --- a/packages/backend-defaults/src/entrypoints/auth/helpers.ts +++ b/packages/backend-defaults/src/entrypoints/auth/helpers.ts @@ -51,7 +51,7 @@ export function createCredentialsWithUserPrincipal( sub: string, token: string, expiresAt?: Date, - viaSubject?: string, + issuedBySubject?: string, ): InternalBackstageCredentials { return Object.defineProperty( { @@ -61,7 +61,9 @@ export function createCredentialsWithUserPrincipal( principal: { type: 'user', userEntityRef: sub, - ...(viaSubject && { via: { type: 'service', subject: viaSubject } }), + ...(issuedBySubject && { + issuedBy: { type: 'service', subject: issuedBySubject }, + }), }, }, 'token', diff --git a/packages/backend-plugin-api/src/services/definitions/AuthService.ts b/packages/backend-plugin-api/src/services/definitions/AuthService.ts index eeac3ea924..976e5ed1a5 100644 --- a/packages/backend-plugin-api/src/services/definitions/AuthService.ts +++ b/packages/backend-plugin-api/src/services/definitions/AuthService.ts @@ -36,7 +36,16 @@ export type BackstageUserPrincipal = { */ userEntityRef: string; - via?: BackstageServicePrincipal; + /** + * The service principal that issued the token on behalf of the user. + * + * @remarks + * + * This field is present in scenarios where a backend service acts on behalf + * of a user. It provides context about the intermediary service that + * facilitated the authentication. + */ + issuedBy?: BackstageServicePrincipal; }; /** From ab89da245161515d394086358976be85c03cc55c Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 9 Apr 2025 15:18:05 +0200 Subject: [PATCH 04/18] test-utils: add issuedBy to mockCredentials Signed-off-by: Vincenzo Scamporlino --- .../src/next/services/MockAuthService.ts | 4 ++-- .../src/next/services/mockCredentials.ts | 18 ++++++++++++++++-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/packages/backend-test-utils/src/next/services/MockAuthService.ts b/packages/backend-test-utils/src/next/services/MockAuthService.ts index 4bce87bd2a..b259f7d182 100644 --- a/packages/backend-test-utils/src/next/services/MockAuthService.ts +++ b/packages/backend-test-utils/src/next/services/MockAuthService.ts @@ -73,11 +73,11 @@ export class MockAuthService implements AuthService { } if (token.startsWith(MOCK_USER_TOKEN_PREFIX)) { - const { sub: userEntityRef }: UserTokenPayload = JSON.parse( + const { sub: userEntityRef, issuedBy }: UserTokenPayload = JSON.parse( token.slice(MOCK_USER_TOKEN_PREFIX.length), ); - return mockCredentials.user(userEntityRef); + return mockCredentials.user(userEntityRef, { issuedBySubject: issuedBy }); } if (token.startsWith(MOCK_USER_LIMITED_TOKEN_PREFIX)) { diff --git a/packages/backend-test-utils/src/next/services/mockCredentials.ts b/packages/backend-test-utils/src/next/services/mockCredentials.ts index 4214320142..1f1c6263a9 100644 --- a/packages/backend-test-utils/src/next/services/mockCredentials.ts +++ b/packages/backend-test-utils/src/next/services/mockCredentials.ts @@ -55,6 +55,7 @@ function validateUserEntityRef(ref: string) { */ export type UserTokenPayload = { sub?: string; + issuedBy?: string; }; /** @@ -107,11 +108,18 @@ export namespace mockCredentials { */ export function user( userEntityRef: string = DEFAULT_MOCK_USER_ENTITY_REF, + options?: { issuedBySubject?: string }, ): BackstageCredentials { validateUserEntityRef(userEntityRef); return { $$type: '@backstage/BackstageCredentials', - principal: { type: 'user', userEntityRef }, + principal: { + type: 'user', + userEntityRef, + ...(options?.issuedBySubject && { + issuedBy: { type: 'service', subject: options.issuedBySubject }, + }), + }, }; } @@ -124,11 +132,17 @@ export namespace mockCredentials { * into the token and forwarded to the credentials object when authenticated * by the mock auth service. */ - export function token(userEntityRef?: string): string { + export function token( + userEntityRef?: string, + options?: { issuedBySubject?: string }, + ): string { if (userEntityRef) { validateUserEntityRef(userEntityRef); return `${MOCK_USER_TOKEN_PREFIX}${JSON.stringify({ sub: userEntityRef, + ...(options?.issuedBySubject && { + issuedBy: options.issuedBySubject, + }), } satisfies UserTokenPayload)}`; } return MOCK_USER_TOKEN; From c772f1c54df4642a77890e29a621c74c28226a72 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 9 Apr 2025 15:19:31 +0200 Subject: [PATCH 05/18] permission-backend: add tests for credentials issued by services Signed-off-by: Vincenzo Scamporlino --- .../src/service/router.test.ts | 56 ++++++++++++++++++- 1 file changed, 53 insertions(+), 3 deletions(-) diff --git a/plugins/permission-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index e3fb5418cf..9d95193fed 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -217,6 +217,9 @@ describe('createRouter', () => { const response = await request(app) .post('/authorize') + .auth(userTokenIssuedByService(), { + type: 'bearer', + }) .send({ items: [ { @@ -545,7 +548,7 @@ describe('createRouter', () => { const response = await request(app) .post('/authorize') - .auth(mockCredentials.user.token(), { type: 'bearer' }) + .auth(userTokenIssuedByService(), { type: 'bearer' }) .send({ items: [ { @@ -592,7 +595,9 @@ describe('createRouter', () => { expect(mockApplyConditions).toHaveBeenCalledWith( 'plugin-1', - mockCredentials.user(), + mockCredentials.user('user:default/spiderman', { + issuedBySubject: 'some-service', + }), [ expect.objectContaining({ id: '123', @@ -605,7 +610,9 @@ describe('createRouter', () => { expect(mockApplyConditions).toHaveBeenCalledWith( 'plugin-2', - mockCredentials.user(), + mockCredentials.user('user:default/spiderman', { + issuedBySubject: 'some-service', + }), [ expect.objectContaining({ id: '234', @@ -719,6 +726,12 @@ describe('createRouter', () => { }); }, ); + + function userTokenIssuedByService() { + return mockCredentials.user.token('user:default/spiderman', { + issuedBySubject: 'some-service', + }); + } }); it.each([ @@ -794,6 +807,7 @@ describe('createRouter', () => { resourceType: 'test-resource-1', attributes: {}, }, + resourceRef: 'resource:1', }, ], }); @@ -807,5 +821,41 @@ describe('createRouter', () => { }), ); }); + + it(`returns a 400 error if the request doesn't contain resourceRef for credentials not issued by a service`, async () => { + policy.handle.mockResolvedValueOnce({ + result: AuthorizeResult.CONDITIONAL, + pluginId: 'test-plugin', + resourceType: 'test-resource-2', + conditions: {}, + }); + + const response = await request(app) + .post('/authorize') + .send({ + items: [ + { + id: '123', + permission: { + type: 'resource', + name: 'test.permission', + resourceType: 'test-resource-1', + attributes: {}, + }, + }, + ], + }); + + expect(response.status).toEqual(400); + expect(response.body).toEqual( + expect.objectContaining({ + error: expect.objectContaining({ + message: expect.stringMatching( + /Resource permissions require a resourceRef to be set/i, + ), + }), + }), + ); + }); }); }); From 3542052a4b81c97fce03a5c0352f123a0497aa8d Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 9 Apr 2025 15:20:27 +0200 Subject: [PATCH 06/18] permission-backend: rollback parser Signed-off-by: Vincenzo Scamporlino --- .../permission-backend/src/service/router.ts | 80 +++++-------------- 1 file changed, 20 insertions(+), 60 deletions(-) diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index bcb066c129..c624947375 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -21,7 +21,6 @@ import { createLegacyAuthAdapters } from '@backstage/backend-common'; import { InputError } from '@backstage/errors'; import { IdentityApi } from '@backstage/plugin-auth-node'; import { - AuthorizePermissionRequest, AuthorizeResult, EvaluatePermissionRequest, EvaluatePermissionRequestBatch, @@ -30,7 +29,6 @@ import { IdentifiedPermissionMessage, isResourcePermission, PermissionAttributes, - PermissionMessageBatch, } from '@backstage/plugin-permission-common'; import { ApplyConditionsRequestEntry, @@ -64,46 +62,31 @@ const attributesSchema: z.ZodSchema = z.object({ .optional(), }); -const basicPermissionSchema = z.object({ - type: z.literal('basic'), - name: z.string(), - attributes: attributesSchema, -}); +const permissionSchema = z.union([ + z.object({ + type: z.literal('basic'), + name: z.string(), + attributes: attributesSchema, + }), + z.object({ + type: z.literal('resource'), + name: z.string(), + attributes: attributesSchema, + resourceType: z.string(), + }), +]); -const resourcePermissionSchema = z.object({ - type: z.literal('resource'), - name: z.string(), - attributes: attributesSchema, - resourceType: z.string(), -}); - -const authorizePermissionRequestBatchSchema: z.ZodSchema< - PermissionMessageBatch +const evaluatePermissionRequestSchema: z.ZodSchema< + IdentifiedPermissionMessage > = z.object({ - items: z.array( - z.union([ - z.object({ - id: z.string(), - permission: basicPermissionSchema, - }), - z.object({ - id: z.string(), - resourceRef: z.string(), - permission: resourcePermissionSchema, - }), - ]), - ), + id: z.string(), + resourceRef: z.string().optional(), + permission: permissionSchema, }); const evaluatePermissionRequestBatchSchema: z.ZodSchema = z.object({ - items: z.array( - z.object({ - id: z.string(), - resourceRef: z.string().optional(), - permission: resourcePermissionSchema, - }), - ), + items: z.array(evaluatePermissionRequestSchema), }); /** @@ -242,30 +225,7 @@ export async function createRouter( allow: ['user', 'none'], }); - // TODO(vinzscam): make some magic - const isServicePrincipal = false; - - if (isServicePrincipal) { - const parsedResult = evaluatePermissionRequestBatchSchema.safeParse( - req.body, - ); - - if (parsedResult.success) { - res.json({ - items: await handleRequest( - parsedResult.data.items, - policy, - permissionIntegrationClient, - credentials, - auth, - userInfo, - ), - }); - return; - } - } - - const parseResult = authorizePermissionRequestBatchSchema.safeParse( + const parseResult = evaluatePermissionRequestBatchSchema.safeParse( req.body, ); From dcc5f2b3ccf1807165f9fa34c436426bbdd75638 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 9 Apr 2025 15:21:11 +0200 Subject: [PATCH 07/18] permission-backend: validate issueBy from credentials Signed-off-by: Vincenzo Scamporlino --- plugins/permission-backend/src/service/router.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index c624947375..dd152d66aa 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -235,6 +235,18 @@ export async function createRouter( const body = parseResult.data; + if ( + !(credentials.principal as BackstageUserPrincipal).issuedBy && + body.items.some( + r => + isResourcePermission(r.permission) && r.resourceRef === undefined, + ) + ) { + throw new InputError( + 'Resource permissions require a resourceRef to be set', + ); + } + res.json({ items: await handleRequest( body.items, From 7a88f85f2f4e1e62fec46f9569b5d3b845fe189c Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 9 Apr 2025 15:58:20 +0200 Subject: [PATCH 08/18] auth: rename issuedBy to actor Signed-off-by: Vincenzo Scamporlino --- .../src/entrypoints/auth/DefaultAuthService.ts | 1 - .../src/entrypoints/auth/helpers.ts | 6 +++--- .../src/services/definitions/AuthService.ts | 2 +- .../src/next/services/MockAuthService.ts | 4 ++-- .../src/next/services/mockCredentials.ts | 14 +++++++------- .../permission-backend/src/service/router.test.ts | 6 +++--- 6 files changed, 16 insertions(+), 17 deletions(-) diff --git a/packages/backend-defaults/src/entrypoints/auth/DefaultAuthService.ts b/packages/backend-defaults/src/entrypoints/auth/DefaultAuthService.ts index 511c11ce40..189976ed7d 100644 --- a/packages/backend-defaults/src/entrypoints/auth/DefaultAuthService.ts +++ b/packages/backend-defaults/src/entrypoints/auth/DefaultAuthService.ts @@ -87,7 +87,6 @@ export class DefaultAuthService implements AuthService { userResult.userEntityRef, token, this.#getJwtExpiration(token), - this.pluginId, ); } diff --git a/packages/backend-defaults/src/entrypoints/auth/helpers.ts b/packages/backend-defaults/src/entrypoints/auth/helpers.ts index 2023cb9c2c..58e2bcff74 100644 --- a/packages/backend-defaults/src/entrypoints/auth/helpers.ts +++ b/packages/backend-defaults/src/entrypoints/auth/helpers.ts @@ -51,7 +51,7 @@ export function createCredentialsWithUserPrincipal( sub: string, token: string, expiresAt?: Date, - issuedBySubject?: string, + actor?: string, ): InternalBackstageCredentials { return Object.defineProperty( { @@ -61,8 +61,8 @@ export function createCredentialsWithUserPrincipal( principal: { type: 'user', userEntityRef: sub, - ...(issuedBySubject && { - issuedBy: { type: 'service', subject: issuedBySubject }, + ...(actor && { + actor: { type: 'service', subject: actor }, }), }, }, diff --git a/packages/backend-plugin-api/src/services/definitions/AuthService.ts b/packages/backend-plugin-api/src/services/definitions/AuthService.ts index 976e5ed1a5..c4e03bb4b7 100644 --- a/packages/backend-plugin-api/src/services/definitions/AuthService.ts +++ b/packages/backend-plugin-api/src/services/definitions/AuthService.ts @@ -45,7 +45,7 @@ export type BackstageUserPrincipal = { * of a user. It provides context about the intermediary service that * facilitated the authentication. */ - issuedBy?: BackstageServicePrincipal; + actor?: BackstageServicePrincipal; }; /** diff --git a/packages/backend-test-utils/src/next/services/MockAuthService.ts b/packages/backend-test-utils/src/next/services/MockAuthService.ts index b259f7d182..660ff61a57 100644 --- a/packages/backend-test-utils/src/next/services/MockAuthService.ts +++ b/packages/backend-test-utils/src/next/services/MockAuthService.ts @@ -73,11 +73,11 @@ export class MockAuthService implements AuthService { } if (token.startsWith(MOCK_USER_TOKEN_PREFIX)) { - const { sub: userEntityRef, issuedBy }: UserTokenPayload = JSON.parse( + const { sub: userEntityRef, actor }: UserTokenPayload = JSON.parse( token.slice(MOCK_USER_TOKEN_PREFIX.length), ); - return mockCredentials.user(userEntityRef, { issuedBySubject: issuedBy }); + return mockCredentials.user(userEntityRef, { actor }); } if (token.startsWith(MOCK_USER_LIMITED_TOKEN_PREFIX)) { diff --git a/packages/backend-test-utils/src/next/services/mockCredentials.ts b/packages/backend-test-utils/src/next/services/mockCredentials.ts index 1f1c6263a9..99456e574b 100644 --- a/packages/backend-test-utils/src/next/services/mockCredentials.ts +++ b/packages/backend-test-utils/src/next/services/mockCredentials.ts @@ -55,7 +55,7 @@ function validateUserEntityRef(ref: string) { */ export type UserTokenPayload = { sub?: string; - issuedBy?: string; + actor?: { subject: string }; }; /** @@ -108,7 +108,7 @@ export namespace mockCredentials { */ export function user( userEntityRef: string = DEFAULT_MOCK_USER_ENTITY_REF, - options?: { issuedBySubject?: string }, + options?: { actor?: string }, ): BackstageCredentials { validateUserEntityRef(userEntityRef); return { @@ -116,8 +116,8 @@ export namespace mockCredentials { principal: { type: 'user', userEntityRef, - ...(options?.issuedBySubject && { - issuedBy: { type: 'service', subject: options.issuedBySubject }, + ...(options?.actor && { + actor: { type: 'service', subject: options.actor.subject }, }), }, }; @@ -134,14 +134,14 @@ export namespace mockCredentials { */ export function token( userEntityRef?: string, - options?: { issuedBySubject?: string }, + options?: { actor?: string }, ): string { if (userEntityRef) { validateUserEntityRef(userEntityRef); return `${MOCK_USER_TOKEN_PREFIX}${JSON.stringify({ sub: userEntityRef, - ...(options?.issuedBySubject && { - issuedBy: options.issuedBySubject, + ...(options?.actor && { + actor: options.actor, }), } satisfies UserTokenPayload)}`; } diff --git a/plugins/permission-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index 9d95193fed..264a19ccc3 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -596,7 +596,7 @@ describe('createRouter', () => { expect(mockApplyConditions).toHaveBeenCalledWith( 'plugin-1', mockCredentials.user('user:default/spiderman', { - issuedBySubject: 'some-service', + actor: 'some-service', }), [ expect.objectContaining({ @@ -611,7 +611,7 @@ describe('createRouter', () => { expect(mockApplyConditions).toHaveBeenCalledWith( 'plugin-2', mockCredentials.user('user:default/spiderman', { - issuedBySubject: 'some-service', + actor: 'some-service', }), [ expect.objectContaining({ @@ -729,7 +729,7 @@ describe('createRouter', () => { function userTokenIssuedByService() { return mockCredentials.user.token('user:default/spiderman', { - issuedBySubject: 'some-service', + actor: 'some-service', }); } }); From ebe9b13b0afd6db18968af558357ca2c6880edb0 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 9 Apr 2025 16:01:42 +0200 Subject: [PATCH 09/18] permission: clarify resourceRef error Signed-off-by: Vincenzo Scamporlino --- .../permission-backend/src/service/router.ts | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index dd152d66aa..20cfaff285 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -236,15 +236,19 @@ export async function createRouter( const body = parseResult.data; if ( - !(credentials.principal as BackstageUserPrincipal).issuedBy && - body.items.some( - r => - isResourcePermission(r.permission) && r.resourceRef === undefined, - ) + auth.isPrincipal(credentials, 'none') || + (auth.isPrincipal(credentials, 'user') && !credentials.principal.actor) ) { - throw new InputError( - 'Resource permissions require a resourceRef to be set', - ); + if ( + body.items.some( + r => + isResourcePermission(r.permission) && r.resourceRef === undefined, + ) + ) { + throw new InputError( + 'Resource permissions require a resourceRef to be set. Direct user requests without a resourceRef are not allowed.', + ); + } } res.json({ From dea4fb1708bdd6cbae4ca9af38539ecb0e63b154 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 9 Apr 2025 16:04:04 +0200 Subject: [PATCH 10/18] test-utils: make optional actor object Signed-off-by: Vincenzo Scamporlino --- .../backend-test-utils/src/next/services/mockCredentials.ts | 6 +++--- plugins/permission-backend/src/service/router.test.ts | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/backend-test-utils/src/next/services/mockCredentials.ts b/packages/backend-test-utils/src/next/services/mockCredentials.ts index 99456e574b..231c108ac4 100644 --- a/packages/backend-test-utils/src/next/services/mockCredentials.ts +++ b/packages/backend-test-utils/src/next/services/mockCredentials.ts @@ -108,7 +108,7 @@ export namespace mockCredentials { */ export function user( userEntityRef: string = DEFAULT_MOCK_USER_ENTITY_REF, - options?: { actor?: string }, + options?: { actor?: { subject: string } }, ): BackstageCredentials { validateUserEntityRef(userEntityRef); return { @@ -134,14 +134,14 @@ export namespace mockCredentials { */ export function token( userEntityRef?: string, - options?: { actor?: string }, + options?: { actor?: { subject: string } }, ): string { if (userEntityRef) { validateUserEntityRef(userEntityRef); return `${MOCK_USER_TOKEN_PREFIX}${JSON.stringify({ sub: userEntityRef, ...(options?.actor && { - actor: options.actor, + actor: { subject: options.actor.subject }, }), } satisfies UserTokenPayload)}`; } diff --git a/plugins/permission-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index 264a19ccc3..80308d7ca7 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -596,7 +596,7 @@ describe('createRouter', () => { expect(mockApplyConditions).toHaveBeenCalledWith( 'plugin-1', mockCredentials.user('user:default/spiderman', { - actor: 'some-service', + actor: { subject: 'some-service' }, }), [ expect.objectContaining({ @@ -611,7 +611,7 @@ describe('createRouter', () => { expect(mockApplyConditions).toHaveBeenCalledWith( 'plugin-2', mockCredentials.user('user:default/spiderman', { - actor: 'some-service', + actor: { subject: 'some-service' }, }), [ expect.objectContaining({ @@ -729,7 +729,7 @@ describe('createRouter', () => { function userTokenIssuedByService() { return mockCredentials.user.token('user:default/spiderman', { - actor: 'some-service', + actor: { subject: 'some-service' }, }); } }); From df133cc3ccf7bfa33db1823be540039138bae01f Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 10 Apr 2025 13:05:25 +0200 Subject: [PATCH 11/18] permission-backend: throw if resourceRef is passed along with a basic permission Signed-off-by: Vincenzo Scamporlino --- .../src/service/router.test.ts | 29 +++++++++++++ .../permission-backend/src/service/router.ts | 42 +++++++++++-------- 2 files changed, 53 insertions(+), 18 deletions(-) diff --git a/plugins/permission-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index 80308d7ca7..9c8fa4ad5f 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -774,6 +774,35 @@ describe('createRouter', () => { { id: '123', permission: { attributes: { invalid: 'attribute' } } }, ], }, + { + items: [ + { + id: '123', + // basic permission can't have resourceRef + resourceRef: 'resource:1', + permission: { + type: 'basic', + name: 'test.permission', + attributes: {}, + }, + }, + ], + }, + { + items: [ + { + id: '123', + // resource ref should be a string + resourceRef: ['resource:1'], + permission: { + type: 'resource', + name: 'test.permission', + attributes: {}, + resourceType: 'test-resource-1', + }, + }, + ], + }, ])('returns a 400 error for invalid request %#', async requestBody => { const response = await request(app).post('/authorize').send(requestBody); diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index 20cfaff285..8ba8658397 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -62,27 +62,33 @@ const attributesSchema: z.ZodSchema = z.object({ .optional(), }); -const permissionSchema = z.union([ - z.object({ - type: z.literal('basic'), - name: z.string(), - attributes: attributesSchema, - }), - z.object({ - type: z.literal('resource'), - name: z.string(), - attributes: attributesSchema, - resourceType: z.string(), - }), -]); +const basicPermissionSchema = z.object({ + type: z.literal('basic'), + name: z.string(), + attributes: attributesSchema, +}); + +const resourcePermissionSchema = z.object({ + type: z.literal('resource'), + name: z.string(), + attributes: attributesSchema, + resourceType: z.string(), +}); const evaluatePermissionRequestSchema: z.ZodSchema< IdentifiedPermissionMessage -> = z.object({ - id: z.string(), - resourceRef: z.string().optional(), - permission: permissionSchema, -}); +> = z.union([ + z.object({ + id: z.string(), + resourceRef: z.undefined().optional(), + permission: basicPermissionSchema, + }), + z.object({ + id: z.string(), + resourceRef: z.string().optional(), + permission: resourcePermissionSchema, + }), +]); const evaluatePermissionRequestBatchSchema: z.ZodSchema = z.object({ From 78eaa50f76a2c25f6b6a8cd69978de2a0949cbad Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 10 Apr 2025 13:26:51 +0200 Subject: [PATCH 12/18] permission-backend: validation changeset Signed-off-by: Vincenzo Scamporlino --- .changeset/cold-rocks-laugh.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/cold-rocks-laugh.md diff --git a/.changeset/cold-rocks-laugh.md b/.changeset/cold-rocks-laugh.md new file mode 100644 index 0000000000..5686e94707 --- /dev/null +++ b/.changeset/cold-rocks-laugh.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-permission-backend': minor +--- + +Improved validation for the `/authorize` endpoint when a `resourceRef` is provided alongside a basic permission. Additionally, introduced a clearer error message for cases where users attempt to directly evaluate conditional permissions. From cf4eb13cee82998065e8bf4a269f2e73e004723b Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 10 Apr 2025 13:27:09 +0200 Subject: [PATCH 13/18] backend-defaults: actor changeset Signed-off-by: Vincenzo Scamporlino --- .changeset/pink-geese-teach.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/pink-geese-teach.md diff --git a/.changeset/pink-geese-teach.md b/.changeset/pink-geese-teach.md new file mode 100644 index 0000000000..2c33721967 --- /dev/null +++ b/.changeset/pink-geese-teach.md @@ -0,0 +1,6 @@ +--- +'@backstage/backend-defaults': minor +'@backstage/backend-test-utils': minor +--- + +Added `actor` property to `BackstageUserPrincipal` containing the subject of the last service (if any) who performed authentication on behalf of the user. From f8de73808863e43c29f8b2bec1334a51d06cb2cf Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 15 Apr 2025 14:24:24 +0200 Subject: [PATCH 14/18] permission: validate actor when applying conditions Signed-off-by: Vincenzo Scamporlino --- .../permissionsRegistryServiceFactory.ts | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/packages/backend-defaults/src/entrypoints/permissionsRegistry/permissionsRegistryServiceFactory.ts b/packages/backend-defaults/src/entrypoints/permissionsRegistry/permissionsRegistryServiceFactory.ts index 7ff6eabf03..f599c4906a 100644 --- a/packages/backend-defaults/src/entrypoints/permissionsRegistry/permissionsRegistryServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/permissionsRegistry/permissionsRegistryServiceFactory.ts @@ -23,6 +23,8 @@ import { PermissionResourceRef, createPermissionIntegrationRouter, } from '@backstage/plugin-permission-node'; +import { NotAllowedError } from '@backstage/errors'; +import Router from 'express-promise-router'; function assertRefPluginId(ref: PermissionResourceRef, pluginId: string) { if (ref.pluginId !== pluginId) { @@ -44,14 +46,34 @@ function assertRefPluginId(ref: PermissionResourceRef, pluginId: string) { export const permissionsRegistryServiceFactory = createServiceFactory({ service: coreServices.permissionsRegistry, deps: { + auth: coreServices.auth, + httpAuth: coreServices.httpAuth, lifecycle: coreServices.lifecycle, httpRouter: coreServices.httpRouter, pluginMetadata: coreServices.pluginMetadata, }, - async factory({ httpRouter, lifecycle, pluginMetadata }) { + async factory({ auth, httpAuth, httpRouter, lifecycle, pluginMetadata }) { const router = createPermissionIntegrationRouter(); + const pluginId = pluginMetadata.getId(); + const applyConditionMiddleware = Router(); + applyConditionMiddleware.use( + '/.well-known/backstage/permissions/apply-conditions', + async (req, _res, next) => { + const credentials = await httpAuth.credentials(req, { + allow: ['user', 'service'], + }); + if ( + auth.isPrincipal(credentials, 'user') && + !credentials.principal.actor + ) { + throw new NotAllowedError(); + } + next(); + }, + ); + httpRouter.use(applyConditionMiddleware); httpRouter.use(router); let started = false; From 63f706e67fb52e71769759bc6f3ffae6548ccba9 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 15 Apr 2025 14:40:47 +0200 Subject: [PATCH 15/18] backend-test-utils: mockCredentials api-reports Signed-off-by: Vincenzo Scamporlino --- packages/backend-test-utils/report.api.md | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/backend-test-utils/report.api.md b/packages/backend-test-utils/report.api.md index 89035cbaec..5497497068 100644 --- a/packages/backend-test-utils/report.api.md +++ b/packages/backend-test-utils/report.api.md @@ -88,6 +88,11 @@ export namespace mockCredentials { } export function user( userEntityRef?: string, + options?: { + actor?: { + subject: string; + }; + }, ): BackstageCredentials; export namespace user { export function header(userEntityRef?: string): string; @@ -95,7 +100,14 @@ export namespace mockCredentials { export function invalidHeader(): string; // (undocumented) export function invalidToken(): string; - export function token(userEntityRef?: string): string; + export function token( + userEntityRef?: string, + options?: { + actor?: { + subject: string; + }; + }, + ): string; } } From 1f905849f7bbd2fb4d2c265ffcb68a0239566913 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 15 Apr 2025 14:41:14 +0200 Subject: [PATCH 16/18] backend-plugin-api: add actor api-report Signed-off-by: Vincenzo Scamporlino --- packages/backend-plugin-api/report.api.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/backend-plugin-api/report.api.md b/packages/backend-plugin-api/report.api.md index 30678f124f..52843dec96 100644 --- a/packages/backend-plugin-api/report.api.md +++ b/packages/backend-plugin-api/report.api.md @@ -177,6 +177,7 @@ export interface BackstageUserInfo { export type BackstageUserPrincipal = { type: 'user'; userEntityRef: string; + actor?: BackstageServicePrincipal; }; // @public From 8a0e34e1b1b5090d9a99560c380fb65d63d8455d Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 15 Apr 2025 15:00:10 +0200 Subject: [PATCH 17/18] backend-plugin-api: add actor changeset Signed-off-by: Vincenzo Scamporlino --- .changeset/pink-geese-teach.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.changeset/pink-geese-teach.md b/.changeset/pink-geese-teach.md index 2c33721967..0264fe1ec6 100644 --- a/.changeset/pink-geese-teach.md +++ b/.changeset/pink-geese-teach.md @@ -1,5 +1,6 @@ --- '@backstage/backend-defaults': minor +'@backstage/backend-plugin-api': minor '@backstage/backend-test-utils': minor --- From 02d9f566de10eb625a323e658e53f98dacb4c047 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 15 Apr 2025 16:03:58 +0200 Subject: [PATCH 18/18] permission-backend: fix for disabled auth policy Signed-off-by: Vincenzo Scamporlino --- plugins/permission-backend/src/service/router.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index 8ba8658397..83dc90a363 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -209,6 +209,11 @@ export async function createRouter( ); } + const disabledDefaultAuthPolicy = + config.getOptionalBoolean( + 'backend.auth.dangerouslyDisableDefaultAuthPolicy', + ) ?? false; + const permissionIntegrationClient = new PermissionIntegrationClient({ discovery, auth, @@ -242,7 +247,7 @@ export async function createRouter( const body = parseResult.data; if ( - auth.isPrincipal(credentials, 'none') || + (auth.isPrincipal(credentials, 'none') && !disabledDefaultAuthPolicy) || (auth.isPrincipal(credentials, 'user') && !credentials.principal.actor) ) { if (