From 3e823d3e14e26b06e64b3631dfaa23f212e6bbf1 Mon Sep 17 00:00:00 2001 From: Phil Kuang Date: Fri, 10 May 2024 18:21:48 -0400 Subject: [PATCH 1/3] feat(userInfo): implement persisting user info to support limited tokens Signed-off-by: Phil Kuang --- .changeset/cold-comics-rush.md | 6 + .../auth/DefaultAuthService.ts | 15 ++- .../auth/authServiceFactory.test.ts | 35 ++++-- .../auth/user/UserTokenHandler.test.ts | 2 - .../auth/user/UserTokenHandler.ts | 1 - .../userInfo/userInfoServiceFactory.ts | 48 ++++++-- .../migrations/20240510120825_user_info.js | 44 +++++++ .../src/identity/TokenFactory.test.ts | 18 ++- .../auth-backend/src/identity/TokenFactory.ts | 19 +-- .../identity/UserInfoDatabaseHandler.test.ts | 109 ++++++++++++++++++ .../src/identity/UserInfoDatabaseHandler.ts | 60 ++++++++++ plugins/auth-backend/src/identity/index.ts | 1 + .../auth-backend/src/identity/router.test.ts | 65 ++++++++++- plugins/auth-backend/src/identity/router.ts | 29 +++-- plugins/auth-backend/src/migrations.test.ts | 26 +++++ plugins/auth-backend/src/service/router.ts | 13 ++- 16 files changed, 449 insertions(+), 42 deletions(-) create mode 100644 .changeset/cold-comics-rush.md create mode 100644 plugins/auth-backend/migrations/20240510120825_user_info.js create mode 100644 plugins/auth-backend/src/identity/UserInfoDatabaseHandler.test.ts create mode 100644 plugins/auth-backend/src/identity/UserInfoDatabaseHandler.ts diff --git a/.changeset/cold-comics-rush.md b/.changeset/cold-comics-rush.md new file mode 100644 index 0000000000..edb481fd83 --- /dev/null +++ b/.changeset/cold-comics-rush.md @@ -0,0 +1,6 @@ +--- +'@backstage/backend-app-api': patch +'@backstage/plugin-auth-backend': patch +--- + +Limited user tokens will no longer include the `ent` field in its payload. Ownership claims will now be fetched from the user info service. 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..9d9fb58bfb 100644 --- a/packages/backend-app-api/src/services/implementations/auth/DefaultAuthService.ts +++ b/packages/backend-app-api/src/services/implementations/auth/DefaultAuthService.ts @@ -49,8 +49,12 @@ export class DefaultAuthService implements AuthService { private readonly publicKeyStore: KeyStore, ) {} - // allowLimitedAccess is currently ignored, since we currently always use the full user tokens - async authenticate(token: string): Promise { + async authenticate( + token: string, + options?: { + allowLimitedAccess?: boolean; + }, + ): Promise { const pluginResult = await this.pluginTokenHandler.verifyToken(token); if (pluginResult) { if (pluginResult.limitedUserToken) { @@ -73,6 +77,13 @@ export class DefaultAuthService implements AuthService { const userResult = await this.userTokenHandler.verifyToken(token); if (userResult) { + if ( + !options?.allowLimitedAccess && + this.userTokenHandler.isLimitedUserToken(token) + ) { + throw new AuthenticationError('Illegal limited user token'); + } + return createCredentialsWithUserPrincipal( userResult.userEntityRef, 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..f8628c001b 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 @@ -199,6 +199,17 @@ describe('authServiceFactory', () => { }); it('should issue limited user tokens', async () => { + /* Corresponding private key in case this test needs to be updated in the future: + { + kty: 'EC', + x: 'c9cPvv_S7zETBKDlAa3oOjr7RvyUueIYIak0TRph7mg', + y: 'bKaxDRAWgmEJ9Ix8e85blH_IsnbQxX31x0oQTVwLZ2c', + crv: 'P-256', + d: '2eJlhCDdGx9fxKDL1D9BnY3CCTEKxL60Bkms0hmubmY', + kid: '8d01c3db-56f9-45f0-86dd-05b3c835b3d3', + alg: 'ES256' + } + */ server.use( rest.get( 'http://localhost:7007/api/auth/.well-known/jwks.json', @@ -208,8 +219,8 @@ describe('authServiceFactory', () => { keys: [ { kty: 'EC', - x: '78-Ei1H3nKM23ZpGMMzte2mVoYCcnfnSiLTm1P7vZM0', - y: 'Z9-PjG_EU598tLLUc2f8sCqxT7bjs8WpoV-lHm9GJHY', + x: 'c9cPvv_S7zETBKDlAa3oOjr7RvyUueIYIak0TRph7mg', + y: 'bKaxDRAWgmEJ9Ix8e85blH_IsnbQxX31x0oQTVwLZ2c', crv: 'P-256', kid: '8d01c3db-56f9-45f0-86dd-05b3c835b3d3', alg: 'ES256', @@ -234,7 +245,7 @@ describe('authServiceFactory', () => { const catalogAuth = await tester.get('catalog'); const fullToken = - 'eyJ0eXAiOiJ2bmQuYmFja3N0YWdlLnVzZXIiLCJhbGciOiJFUzI1NiIsImtpZCI6IjhkMDFjM2RiLTU2ZjktNDVmMC04NmRkLTA1YjNjODM1YjNkMyJ9.eyJpc3MiOiJodHRwOi8vbG9jYWxob3N0OjcwMDcvYXBpL2F1dGgiLCJzdWIiOiJ1c2VyOmRldmVsb3BtZW50L2d1ZXN0IiwiZW50IjpbInVzZXI6ZGV2ZWxvcG1lbnQvZ3Vlc3QiLCJncm91cDpkZWZhdWx0L3RlYW0tYSJdLCJhdWQiOiJiYWNrc3RhZ2UiLCJpYXQiOjE3MTIwNzE3MTQsImV4cCI6MTcxMjA3NTMxNCwidWlwIjoiMDFBUUJfSWpHTXRWc2gyWmgzZEg1NXhOX29pSVlhQ1F3ODJjeDZ5M1BQMXlpTjM4eGMzMVpMS2U0YVNDQlJTTy10cjFzZFUzT29ELUxJYV8tNV9RVUEifQ.mjIrZGqbZ2t68fS4U3crlGw-bYJZnMlhMHf-YL7q_u1HfaLr4NMTcHkxdnNS2wfJxCmUBxRfUS8b3nSAKsxcHA'; + 'eyJ0eXAiOiJ2bmQuYmFja3N0YWdlLnVzZXIiLCJhbGciOiJFUzI1NiIsImtpZCI6IjhkMDFjM2RiLTU2ZjktNDVmMC04NmRkLTA1YjNjODM1YjNkMyJ9.eyJpc3MiOiJodHRwOi8vbG9jYWxob3N0OjcwMDcvYXBpL2F1dGgiLCJzdWIiOiJ1c2VyOmRldmVsb3BtZW50L2d1ZXN0IiwiZW50IjpbInVzZXI6ZGV2ZWxvcG1lbnQvZ3Vlc3QiLCJncm91cDpkZWZhdWx0L3RlYW0tYSJdLCJhdWQiOiJiYWNrc3RhZ2UiLCJpYXQiOjE3MTIwNzE3MTQsImV4cCI6MTcxMjA3NTMxNCwidWlwIjoiSmwxVEpycG9VUjR1NENjUE9nalJMeHpEMi1FMGZPR3ptSm81UWI2eS1aN19meG5oVVBEdWVWRE1CS0l6WF9pc0lvSDhlZm9EUFA5bG9aQnpPblB5Z2cifQ.1gVMq1ofO8PzRctu72D6c4IMqXuIabT79WdGEhW6vIrBRs_qhuWAa94Wvz_KYKpBTb2nxgzXJ5OeddeoYApMyQ'; const credentials = await catalogAuth.authenticate(fullToken); if (!catalogAuth.isPrincipal(credentials, 'user')) { @@ -256,7 +267,6 @@ describe('authServiceFactory', () => { const expectedTokenPayload = base64url.encode( JSON.stringify({ sub: 'user:development/guest', - ent: ['user:development/guest', 'group:default/team-a'], iat: expectedIssuedAt, exp: expectedExpiresAt, }), @@ -293,6 +303,17 @@ describe('authServiceFactory', () => { const catalogAuth = await tester.get('catalog'); const permissionAuth = await tester.get('permission'); + /* Corresponding private key in case this test needs to be updated in the future: + { + kty: 'EC', + x: 'c9cPvv_S7zETBKDlAa3oOjr7RvyUueIYIak0TRph7mg', + y: 'bKaxDRAWgmEJ9Ix8e85blH_IsnbQxX31x0oQTVwLZ2c', + crv: 'P-256', + d: '2eJlhCDdGx9fxKDL1D9BnY3CCTEKxL60Bkms0hmubmY', + kid: '8d01c3db-56f9-45f0-86dd-05b3c835b3d3', + alg: 'ES256' + } + */ server.use( rest.get( 'http://localhost:7007/api/auth/.well-known/jwks.json', @@ -302,8 +323,8 @@ describe('authServiceFactory', () => { keys: [ { kty: 'EC', - x: '78-Ei1H3nKM23ZpGMMzte2mVoYCcnfnSiLTm1P7vZM0', - y: 'Z9-PjG_EU598tLLUc2f8sCqxT7bjs8WpoV-lHm9GJHY', + x: 'c9cPvv_S7zETBKDlAa3oOjr7RvyUueIYIak0TRph7mg', + y: 'bKaxDRAWgmEJ9Ix8e85blH_IsnbQxX31x0oQTVwLZ2c', crv: 'P-256', kid: '8d01c3db-56f9-45f0-86dd-05b3c835b3d3', alg: 'ES256', @@ -341,7 +362,7 @@ describe('authServiceFactory', () => { }); const fullToken = - 'eyJ0eXAiOiJ2bmQuYmFja3N0YWdlLnVzZXIiLCJhbGciOiJFUzI1NiIsImtpZCI6IjhkMDFjM2RiLTU2ZjktNDVmMC04NmRkLTA1YjNjODM1YjNkMyJ9.eyJpc3MiOiJodHRwOi8vbG9jYWxob3N0OjcwMDcvYXBpL2F1dGgiLCJzdWIiOiJ1c2VyOmRldmVsb3BtZW50L2d1ZXN0IiwiZW50IjpbInVzZXI6ZGV2ZWxvcG1lbnQvZ3Vlc3QiLCJncm91cDpkZWZhdWx0L3RlYW0tYSJdLCJhdWQiOiJiYWNrc3RhZ2UiLCJpYXQiOjE3MTIwNzE3MTQsImV4cCI6MTcxMjA3NTMxNCwidWlwIjoiMDFBUUJfSWpHTXRWc2gyWmgzZEg1NXhOX29pSVlhQ1F3ODJjeDZ5M1BQMXlpTjM4eGMzMVpMS2U0YVNDQlJTTy10cjFzZFUzT29ELUxJYV8tNV9RVUEifQ.mjIrZGqbZ2t68fS4U3crlGw-bYJZnMlhMHf-YL7q_u1HfaLr4NMTcHkxdnNS2wfJxCmUBxRfUS8b3nSAKsxcHA'; + 'eyJ0eXAiOiJ2bmQuYmFja3N0YWdlLnVzZXIiLCJhbGciOiJFUzI1NiIsImtpZCI6IjhkMDFjM2RiLTU2ZjktNDVmMC04NmRkLTA1YjNjODM1YjNkMyJ9.eyJpc3MiOiJodHRwOi8vbG9jYWxob3N0OjcwMDcvYXBpL2F1dGgiLCJzdWIiOiJ1c2VyOmRldmVsb3BtZW50L2d1ZXN0IiwiZW50IjpbInVzZXI6ZGV2ZWxvcG1lbnQvZ3Vlc3QiLCJncm91cDpkZWZhdWx0L3RlYW0tYSJdLCJhdWQiOiJiYWNrc3RhZ2UiLCJpYXQiOjE3MTIwNzE3MTQsImV4cCI6MTcxMjA3NTMxNCwidWlwIjoiSmwxVEpycG9VUjR1NENjUE9nalJMeHpEMi1FMGZPR3ptSm81UWI2eS1aN19meG5oVVBEdWVWRE1CS0l6WF9pc0lvSDhlZm9EUFA5bG9aQnpPblB5Z2cifQ.1gVMq1ofO8PzRctu72D6c4IMqXuIabT79WdGEhW6vIrBRs_qhuWAa94Wvz_KYKpBTb2nxgzXJ5OeddeoYApMyQ'; const credentials = await searchAuth.authenticate(fullToken); if (!searchAuth.isPrincipal(credentials, 'user')) { diff --git a/packages/backend-app-api/src/services/implementations/auth/user/UserTokenHandler.test.ts b/packages/backend-app-api/src/services/implementations/auth/user/UserTokenHandler.test.ts index b0e8418655..76bafb2a8f 100644 --- a/packages/backend-app-api/src/services/implementations/auth/user/UserTokenHandler.test.ts +++ b/packages/backend-app-api/src/services/implementations/auth/user/UserTokenHandler.test.ts @@ -346,7 +346,6 @@ describe('UserTokenHandler', () => { header: { typ: 'vnd.backstage.limited-user', alg: 'ES256' }, payload: { sub: 'mock', - ent: ['mock'], iat: 1, exp: 2, }, @@ -384,7 +383,6 @@ describe('UserTokenHandler', () => { new TextEncoder().encode( JSON.stringify({ sub: parts.payload.sub, - ent: parts.payload.ent, iat: parts.payload.iat, exp: parts.payload.exp, }), diff --git a/packages/backend-app-api/src/services/implementations/auth/user/UserTokenHandler.ts b/packages/backend-app-api/src/services/implementations/auth/user/UserTokenHandler.ts index bd73d8701f..4dd1874843 100644 --- a/packages/backend-app-api/src/services/implementations/auth/user/UserTokenHandler.ts +++ b/packages/backend-app-api/src/services/implementations/auth/user/UserTokenHandler.ts @@ -137,7 +137,6 @@ export class UserTokenHandler { base64url.encode( JSON.stringify({ sub: payload.sub, - ent: payload.ent, iat: payload.iat, exp: payload.exp, }), diff --git a/packages/backend-app-api/src/services/implementations/userInfo/userInfoServiceFactory.ts b/packages/backend-app-api/src/services/implementations/userInfo/userInfoServiceFactory.ts index e88acaf4c2..a745285843 100644 --- a/packages/backend-app-api/src/services/implementations/userInfo/userInfoServiceFactory.ts +++ b/packages/backend-app-api/src/services/implementations/userInfo/userInfoServiceFactory.ts @@ -19,13 +19,24 @@ import { BackstageUserInfo, coreServices, createServiceFactory, + DiscoveryService, BackstageCredentials, } from '@backstage/backend-plugin-api'; +import { ResponseError } from '@backstage/errors'; import { decodeJwt } from 'jose'; import { toInternalBackstageCredentials } from '../auth/helpers'; -// TODO: The intention is for this to eventually be replaced by a call to the auth-backend +type Options = { + discovery: DiscoveryService; +}; + export class DefaultUserInfoService implements UserInfoService { + private readonly discovery: DiscoveryService; + + constructor(options: Options) { + this.discovery = options.discovery; + } + async getUserInfo( credentials: BackstageCredentials, ): Promise { @@ -36,29 +47,48 @@ export class DefaultUserInfoService implements UserInfoService { if (!internalCredentials.token) { throw new Error('User credentials is unexpectedly missing token'); } - const { sub: userEntityRef, ent: ownershipEntityRefs = [] } = decodeJwt( + const { sub: userEntityRef, ent: ownershipEntityRefs } = decodeJwt( internalCredentials.token, ); if (typeof userEntityRef !== 'string') { throw new Error('User entity ref must be a string'); } + + // Return user info if it's already available in the token (ie. it is a full token) if ( - !Array.isArray(ownershipEntityRefs) || - ownershipEntityRefs.some(ref => typeof ref !== 'string') + Array.isArray(ownershipEntityRefs) && + ownershipEntityRefs.every(ref => typeof ref === 'string') ) { - throw new Error('Ownership entity refs must be an array of strings'); + return { userEntityRef, ownershipEntityRefs }; } - return { userEntityRef, ownershipEntityRefs }; + const userInfoResp = await fetch( + `${await this.discovery.getBaseUrl('auth')}/v1/userinfo`, + { + headers: { + Authorization: `Bearer ${internalCredentials.token}`, + }, + }, + ); + + if (!userInfoResp.ok) { + throw await ResponseError.fromResponse(userInfoResp); + } + + const { sub, ent } = await userInfoResp.json(); + + return { userEntityRef: sub, ownershipEntityRefs: ent }; } } /** @public */ export const userInfoServiceFactory = createServiceFactory({ service: coreServices.userInfo, - deps: {}, - async factory() { - return new DefaultUserInfoService(); + deps: { + discovery: coreServices.discovery, + }, + async factory({ discovery }) { + return new DefaultUserInfoService({ discovery }); }, }); diff --git a/plugins/auth-backend/migrations/20240510120825_user_info.js b/plugins/auth-backend/migrations/20240510120825_user_info.js new file mode 100644 index 0000000000..ed667a2b7a --- /dev/null +++ b/plugins/auth-backend/migrations/20240510120825_user_info.js @@ -0,0 +1,44 @@ +/* + * 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. + */ + +// @ts-check + +/** + * @param {import('knex').Knex} knex + */ +exports.up = async function up(knex) { + await knex.schema.createTable('user_info', table => { + table.comment('User information'); + + table + .string('user_entity_ref') + .primary() + .notNullable() + .comment('User entity reference'); + + table + .text('user_info', 'longtext') + .notNullable() + .comment('User info blob, JSON serialized'); + }); +}; + +/** + * @param {import('knex').Knex} knex + */ +exports.down = async function down(knex) { + await knex.schema.dropTable('user_info'); +}; diff --git a/plugins/auth-backend/src/identity/TokenFactory.test.ts b/plugins/auth-backend/src/identity/TokenFactory.test.ts index 0994e44f68..becf821db7 100644 --- a/plugins/auth-backend/src/identity/TokenFactory.test.ts +++ b/plugins/auth-backend/src/identity/TokenFactory.test.ts @@ -24,6 +24,7 @@ import { } from 'jose'; import { MemoryKeyStore } from './MemoryKeyStore'; import { TokenFactory } from './TokenFactory'; +import { UserInfoDatabaseHandler } from './UserInfoDatabaseHandler'; import { tokenTypes } from '@backstage/plugin-auth-node'; const logger = getVoidLogger(); @@ -43,6 +44,10 @@ const entityRef = stringifyEntityRef({ }); describe('TokenFactory', () => { + const mockUserInfoDatabaseHandler = { + addUserInfo: jest.fn().mockResolvedValue(undefined), + } as unknown as UserInfoDatabaseHandler; + it('should issue valid tokens signed by a listed key', async () => { const keyDurationSeconds = 5; const factory = new TokenFactory({ @@ -50,6 +55,7 @@ describe('TokenFactory', () => { keyStore: new MemoryKeyStore(), keyDurationSeconds, logger, + userInfoDatabaseHandler: mockUserInfoDatabaseHandler, }); await expect(factory.listPublicKeys()).resolves.toEqual({ keys: [] }); @@ -81,6 +87,11 @@ describe('TokenFactory', () => { verifyResult.payload.iat! + keyDurationSeconds, ); + expect(mockUserInfoDatabaseHandler.addUserInfo).toHaveBeenCalledWith({ + userEntityRef: entityRef, + ownershipEntityRefs: [entityRef], + }); + // Emulate the reconstruction of a limited user token const limitedUserToken = [ base64url.encode( @@ -93,7 +104,6 @@ describe('TokenFactory', () => { base64url.encode( JSON.stringify({ sub: verifyResult.payload.sub, - ent: verifyResult.payload.ent, iat: verifyResult.payload.iat, exp: verifyResult.payload.exp, }), @@ -107,7 +117,6 @@ describe('TokenFactory', () => { ); expect(verifyProofResult.payload).toEqual({ sub: entityRef, - ent: [entityRef], iat: expect.any(Number), exp: expect.any(Number), }); @@ -125,6 +134,7 @@ describe('TokenFactory', () => { keyStore: new MemoryKeyStore(), keyDurationSeconds: 5, logger, + userInfoDatabaseHandler: mockUserInfoDatabaseHandler, }); const token1 = await factory.issueToken({ @@ -170,6 +180,7 @@ describe('TokenFactory', () => { keyStore: new MemoryKeyStore(), keyDurationSeconds, logger, + userInfoDatabaseHandler: mockUserInfoDatabaseHandler, }); await expect(() => { @@ -187,6 +198,7 @@ describe('TokenFactory', () => { keyDurationSeconds, logger, algorithm: '', + userInfoDatabaseHandler: mockUserInfoDatabaseHandler, }); await expect(() => { @@ -202,6 +214,7 @@ describe('TokenFactory', () => { keyStore: new MemoryKeyStore(), keyDurationSeconds: 5, logger, + userInfoDatabaseHandler: mockUserInfoDatabaseHandler, }); await expect(() => { @@ -220,6 +233,7 @@ describe('TokenFactory', () => { keyStore: new MemoryKeyStore(), keyDurationSeconds, logger, + userInfoDatabaseHandler: mockUserInfoDatabaseHandler, }); const token = await factory.issueToken({ diff --git a/plugins/auth-backend/src/identity/TokenFactory.ts b/plugins/auth-backend/src/identity/TokenFactory.ts index 9277361679..4c7753d998 100644 --- a/plugins/auth-backend/src/identity/TokenFactory.ts +++ b/plugins/auth-backend/src/identity/TokenFactory.ts @@ -31,6 +31,7 @@ import { LoggerService } from '@backstage/backend-plugin-api'; import { TokenParams, tokenTypes } from '@backstage/plugin-auth-node'; import { AnyJWK, KeyStore, TokenIssuer } from './types'; import { JsonValue } from '@backstage/types'; +import { UserInfoDatabaseHandler } from './UserInfoDatabaseHandler'; const MS_IN_S = 1000; const MAX_TOKEN_LENGTH = 32768; // At 64 bytes per entity ref this still leaves room for about 500 entities @@ -93,11 +94,6 @@ interface BackstageUserIdentityProofPayload { */ sub: string; - /** - * The ownership entity refs of the user - */ - ent?: string[]; - /** * Standard expiry in epoch seconds */ @@ -124,6 +120,7 @@ type Options = { * If not, add a knex migration file in the migrations folder. * More info on supported algorithms: https://github.com/panva/jose */ algorithm?: string; + userInfoDatabaseHandler: UserInfoDatabaseHandler; }; /** @@ -146,6 +143,7 @@ export class TokenFactory implements TokenIssuer { private readonly keyStore: KeyStore; private readonly keyDurationSeconds: number; private readonly algorithm: string; + private readonly userInfoDatabaseHandler: UserInfoDatabaseHandler; private keyExpiry?: Date; private privateKeyPromise?: Promise; @@ -156,6 +154,7 @@ export class TokenFactory implements TokenIssuer { this.keyStore = options.keyStore; this.keyDurationSeconds = options.keyDurationSeconds; this.algorithm = options.algorithm ?? 'ES256'; + this.userInfoDatabaseHandler = options.userInfoDatabaseHandler; } async issueToken(params: TokenParams): Promise { @@ -190,7 +189,7 @@ export class TokenFactory implements TokenIssuer { alg: key.alg, kid: key.kid, }, - payload: { sub, ent, iat, exp }, + payload: { sub, iat, exp }, key: signingKey, }); @@ -221,6 +220,13 @@ export class TokenFactory implements TokenIssuer { ); } + // Store the user info in the database upon successful token + // issuance so that it can be retrieved later by limited user tokens + await this.userInfoDatabaseHandler.addUserInfo({ + userEntityRef: sub, + ownershipEntityRefs: ent, + }); + return token; } @@ -342,7 +348,6 @@ export class TokenFactory implements TokenIssuer { const payload = { sub: options.payload.sub, - ent: options.payload.ent, iat: options.payload.iat, exp: options.payload.exp, }; diff --git a/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.test.ts b/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.test.ts new file mode 100644 index 0000000000..4c18e1ff2a --- /dev/null +++ b/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.test.ts @@ -0,0 +1,109 @@ +/* + * 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 { resolvePackagePath } from '@backstage/backend-common'; +import { TestDatabaseId, TestDatabases } from '@backstage/backend-test-utils'; +import { Knex } from 'knex'; +import { UserInfoDatabaseHandler } from './UserInfoDatabaseHandler'; + +const migrationsDir = resolvePackagePath( + '@backstage/plugin-auth-backend', + 'migrations', +); + +describe('UserInfoDatabaseHandler', () => { + const databases = TestDatabases.create(); + + async function createDatabaseHandler(databaseId: TestDatabaseId) { + const knex = await databases.init(databaseId); + + await knex.migrate.latest({ + directory: migrationsDir, + }); + + return { + knex, + dbHandler: new UserInfoDatabaseHandler(knex), + }; + } + + describe.each(databases.eachSupportedId())( + '%p', + databaseId => { + let knex: Knex; + let dbHandler: UserInfoDatabaseHandler; + + beforeEach(async () => { + ({ knex, dbHandler } = await createDatabaseHandler(databaseId)); + }, 30000); + + it('addUserInfo', async () => { + const userInfo = { + userEntityRef: 'user:default/foo', + ownershipEntityRefs: ['group:default/foo-group', 'group:default/bar'], + }; + + await dbHandler.addUserInfo(userInfo); + + const savedUserInfo = await knex('user_info') + .where('user_entity_ref', 'user:default/foo') + .first(); + expect(savedUserInfo).toEqual({ + user_entity_ref: userInfo.userEntityRef, + user_info: JSON.stringify({ + ownershipEntityRefs: userInfo.ownershipEntityRefs, + }), + }); + + userInfo.ownershipEntityRefs = [ + 'group:default/group1', + 'group:default/group2', + ]; + await dbHandler.addUserInfo(userInfo); + + const updatedUserInfo = await knex('user_info') + .where('user_entity_ref', 'user:default/foo') + .first(); + expect(updatedUserInfo).toEqual({ + user_entity_ref: userInfo.userEntityRef, + user_info: JSON.stringify({ + ownershipEntityRefs: userInfo.ownershipEntityRefs, + }), + }); + }); + + it('getUserInfo', async () => { + const userInfo = { + userEntityRef: 'user:default/backstage-user', + ownershipEntityRefs: ['group:default/group1', 'group:default/group2'], + }; + + await knex('user_info').insert({ + user_entity_ref: userInfo.userEntityRef, + user_info: JSON.stringify({ + ownershipEntityRefs: userInfo.ownershipEntityRefs, + }), + }); + + const savedUserInfo = await dbHandler.getUserInfo( + userInfo.userEntityRef, + ); + expect(savedUserInfo).toEqual(userInfo); + }); + }, + 60000, + ); +}); diff --git a/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.ts b/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.ts new file mode 100644 index 0000000000..f7dea52236 --- /dev/null +++ b/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.ts @@ -0,0 +1,60 @@ +/* + * 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 { BackstageUserInfo } from '@backstage/backend-plugin-api'; +import { Knex } from 'knex'; + +const TABLE = 'user_info'; + +type Row = { + user_entity_ref: string; + user_info: string; +}; + +// TODO: How do we prune stale users? +export class UserInfoDatabaseHandler { + constructor(private readonly client: Knex) {} + + async addUserInfo(userInfo: BackstageUserInfo): Promise { + await this.client(TABLE) + .insert({ + user_entity_ref: userInfo.userEntityRef, + user_info: JSON.stringify({ + ownershipEntityRefs: userInfo.ownershipEntityRefs, + }), + }) + .onConflict('user_entity_ref') + .merge(); + } + + async getUserInfo( + userEntityRef: string, + ): Promise { + const info = await this.client(TABLE) + .where({ user_entity_ref: userEntityRef }) + .first(); + + if (!info) { + return undefined; + } + + const { ownershipEntityRefs } = JSON.parse(info.user_info); + return { + userEntityRef: info.user_entity_ref, + ownershipEntityRefs, + }; + } +} diff --git a/plugins/auth-backend/src/identity/index.ts b/plugins/auth-backend/src/identity/index.ts index 0492836723..6ea025aae9 100644 --- a/plugins/auth-backend/src/identity/index.ts +++ b/plugins/auth-backend/src/identity/index.ts @@ -21,3 +21,4 @@ export { MemoryKeyStore } from './MemoryKeyStore'; export { FirestoreKeyStore } from './FirestoreKeyStore'; export { KeyStores } from './KeyStores'; export type { KeyStore, TokenParams } from './types'; +export { UserInfoDatabaseHandler } from './UserInfoDatabaseHandler'; diff --git a/plugins/auth-backend/src/identity/router.test.ts b/plugins/auth-backend/src/identity/router.test.ts index 9880d2931c..1ed9071578 100644 --- a/plugins/auth-backend/src/identity/router.test.ts +++ b/plugins/auth-backend/src/identity/router.test.ts @@ -22,10 +22,15 @@ import { mockServices, startTestBackend } from '@backstage/backend-test-utils'; import Router from 'express-promise-router'; import request from 'supertest'; import { bindOidcRouter } from './router'; +import { UserInfoDatabaseHandler } from './UserInfoDatabaseHandler'; describe('bindOidcRouter', () => { - it('should return user info', async () => { + it('should return user info for full tokens', async () => { const auth = mockServices.auth.mock(); + const mockUserInfoDatabaseHandler = { + getUserInfo: jest.fn().mockResolvedValue(undefined), + } as unknown as UserInfoDatabaseHandler; + const { server } = await startTestBackend({ features: [ createBackendPlugin({ @@ -39,6 +44,7 @@ describe('bindOidcRouter', () => { baseUrl: 'http://localhost:7000', auth, tokenIssuer: {} as any, + userInfoDatabaseHandler: mockUserInfoDatabaseHandler, }); httpRouter.use(router); httpRouter.addAuthPolicy({ @@ -68,6 +74,61 @@ describe('bindOidcRouter', () => { ent: ['k/ns:a', 'k/ns:b'], }); - expect('test').toBe('test'); + expect(mockUserInfoDatabaseHandler.getUserInfo).not.toHaveBeenCalled(); + }); + + it('should return user info for limited tokens', async () => { + const auth = mockServices.auth.mock(); + const mockUserInfoDatabaseHandler = { + getUserInfo: jest.fn().mockResolvedValue({ + userEntityRef: 'k/ns:n', + ownershipEntityRefs: ['k/ns:a', 'k/ns:b'], + }), + } as unknown as UserInfoDatabaseHandler; + + const { server } = await startTestBackend({ + features: [ + createBackendPlugin({ + pluginId: 'auth', + register(reg) { + reg.registerInit({ + deps: { httpRouter: coreServices.httpRouter }, + async init({ httpRouter }) { + const router = Router(); + bindOidcRouter(router, { + baseUrl: 'http://localhost:7000', + auth, + tokenIssuer: {} as any, + userInfoDatabaseHandler: mockUserInfoDatabaseHandler, + }); + httpRouter.use(router); + httpRouter.addAuthPolicy({ + path: '/', + allow: 'unauthenticated', + }); + }, + }); + }, + }), + ], + }); + + auth.authenticate.mockResolvedValueOnce({} as any); + auth.isPrincipal.mockReturnValueOnce(true); + + await request(server) + .get('/api/auth/v1/userinfo') + .set( + 'Authorization', + `Bearer h.${btoa(JSON.stringify({ sub: 'k/ns:n' }))}.s`, + ) + .expect(200, { + sub: 'k/ns:n', + ent: ['k/ns:a', 'k/ns:b'], + }); + + expect(mockUserInfoDatabaseHandler.getUserInfo).toHaveBeenCalledWith( + 'k/ns:n', + ); }); }); diff --git a/plugins/auth-backend/src/identity/router.ts b/plugins/auth-backend/src/identity/router.ts index 3936298c72..be9345338f 100644 --- a/plugins/auth-backend/src/identity/router.ts +++ b/plugins/auth-backend/src/identity/router.ts @@ -20,6 +20,7 @@ import { TokenIssuer } from './types'; import { AuthService } from '@backstage/backend-plugin-api'; import { decodeJwt } from 'jose'; import { AuthenticationError, InputError } from '@backstage/errors'; +import { UserInfoDatabaseHandler } from './UserInfoDatabaseHandler'; export function bindOidcRouter( targetRouter: express.Router, @@ -27,9 +28,10 @@ export function bindOidcRouter( baseUrl: string; auth: AuthService; tokenIssuer: TokenIssuer; + userInfoDatabaseHandler: UserInfoDatabaseHandler; }, ) { - const { baseUrl, auth, tokenIssuer } = options; + const { baseUrl, auth, tokenIssuer, userInfoDatabaseHandler } = options; const router = Router(); targetRouter.use(router); @@ -91,21 +93,30 @@ export function bindOidcRouter( ); } - const { sub: userEntityRef, ent: ownershipEntityRefs = [] } = - decodeJwt(token); + const { sub: userEntityRef, ent: ownershipEntityRefs } = decodeJwt(token); if (typeof userEntityRef !== 'string') { throw new Error('Invalid user token, user entity ref must be a string'); } + + // Return user info if it's already available in the token (ie. it is a full token) if ( - !Array.isArray(ownershipEntityRefs) || - ownershipEntityRefs.some(ref => typeof ref !== 'string') + Array.isArray(ownershipEntityRefs) && + ownershipEntityRefs.every(ref => typeof ref === 'string') ) { - throw new Error( - 'Invalid user token, ownership entity refs must be an array of strings', - ); + res.json({ sub: userEntityRef, ent: ownershipEntityRefs }); + return; } - res.json({ sub: userEntityRef, ent: ownershipEntityRefs }); + const userInfo = await userInfoDatabaseHandler.getUserInfo(userEntityRef); + if (!userInfo) { + res.status(404).send('User info not found'); + return; + } + + res.json({ + sub: userInfo.userEntityRef, + ent: userInfo.ownershipEntityRefs, + }); }); } diff --git a/plugins/auth-backend/src/migrations.test.ts b/plugins/auth-backend/src/migrations.test.ts index f80b729975..eeb56785c8 100644 --- a/plugins/auth-backend/src/migrations.test.ts +++ b/plugins/auth-backend/src/migrations.test.ts @@ -71,4 +71,30 @@ describe('migrations', () => { await knex.destroy(); }, ); + + it.each(databases.eachSupportedId())( + '20240510120825_user_info.js, %p', + async databaseId => { + const knex = await databases.init(databaseId); + + await migrateUntilBefore(knex, '20240510120825_user_info.js'); + await migrateUpOnce(knex); + + const user_info = JSON.stringify({ + ownershipEntityRefs: ['group:default/group1', 'group:default/group2'], + }); + + await knex + .insert({ user_entity_ref: 'user:default/backstage-user', user_info }) + .into('user_info'); + + await expect(knex('user_info')).resolves.toEqual([ + { user_entity_ref: 'user:default/backstage-user', user_info }, + ]); + + await migrateDownOnce(knex); + + await knex.destroy(); + }, + ); }); diff --git a/plugins/auth-backend/src/service/router.ts b/plugins/auth-backend/src/service/router.ts index 11c150aa33..4bb42c3abd 100644 --- a/plugins/auth-backend/src/service/router.ts +++ b/plugins/auth-backend/src/service/router.ts @@ -32,7 +32,12 @@ import { } from '@backstage/backend-common'; import { NotFoundError } from '@backstage/errors'; import { CatalogApi } from '@backstage/catalog-client'; -import { bindOidcRouter, KeyStores, TokenFactory } from '../identity'; +import { + bindOidcRouter, + KeyStores, + TokenFactory, + UserInfoDatabaseHandler, +} from '../identity'; import session from 'express-session'; import connectSessionKnex from 'connect-session-knex'; import passport from 'passport'; @@ -87,6 +92,10 @@ export async function createRouter( database: authDb, }); + const userInfoDatabaseHandler = new UserInfoDatabaseHandler( + await authDb.get(), + ); + let tokenIssuer: TokenIssuer; if (keyStore instanceof StaticKeyStore) { tokenIssuer = new StaticTokenIssuer( @@ -106,6 +115,7 @@ export async function createRouter( algorithm: tokenFactoryAlgorithm ?? config.getOptionalString('auth.identityTokenAlgorithm'), + userInfoDatabaseHandler, }); } @@ -156,6 +166,7 @@ export async function createRouter( auth, tokenIssuer, baseUrl: authUrl, + userInfoDatabaseHandler, }); // Gives a more helpful error message than a plain 404 From 79b0b1f3d3dd4e84b787a64890d4e7d3a28209e3 Mon Sep 17 00:00:00 2001 From: Phil Kuang Date: Fri, 7 Jun 2024 16:26:02 -0400 Subject: [PATCH 2/3] refactor(userInfo): store full claims Signed-off-by: Phil Kuang --- .../userInfo/userInfoServiceFactory.ts | 46 +++++++++++-------- .../migrations/20240510120825_user_info.js | 5 ++ .../src/identity/TokenFactory.test.ts | 3 +- .../auth-backend/src/identity/TokenFactory.ts | 9 +--- .../identity/UserInfoDatabaseHandler.test.ts | 42 ++++++++--------- .../src/identity/UserInfoDatabaseHandler.ts | 32 +++++++------ .../auth-backend/src/identity/router.test.ts | 29 ++++++++---- plugins/auth-backend/src/identity/router.ts | 16 +------ plugins/auth-backend/src/migrations.test.ts | 16 +++++-- 9 files changed, 109 insertions(+), 89 deletions(-) diff --git a/packages/backend-app-api/src/services/implementations/userInfo/userInfoServiceFactory.ts b/packages/backend-app-api/src/services/implementations/userInfo/userInfoServiceFactory.ts index a745285843..5362ff83b0 100644 --- a/packages/backend-app-api/src/services/implementations/userInfo/userInfoServiceFactory.ts +++ b/packages/backend-app-api/src/services/implementations/userInfo/userInfoServiceFactory.ts @@ -47,7 +47,7 @@ export class DefaultUserInfoService implements UserInfoService { if (!internalCredentials.token) { throw new Error('User credentials is unexpectedly missing token'); } - const { sub: userEntityRef, ent: ownershipEntityRefs } = decodeJwt( + const { sub: userEntityRef, ent: tokenEnt } = decodeJwt( internalCredentials.token, ); @@ -55,30 +55,38 @@ export class DefaultUserInfoService implements UserInfoService { throw new Error('User entity ref must be a string'); } - // Return user info if it's already available in the token (ie. it is a full token) - if ( - Array.isArray(ownershipEntityRefs) && - ownershipEntityRefs.every(ref => typeof ref === 'string') - ) { - return { userEntityRef, ownershipEntityRefs }; - } + let ownershipEntityRefs = tokenEnt; - const userInfoResp = await fetch( - `${await this.discovery.getBaseUrl('auth')}/v1/userinfo`, - { - headers: { - Authorization: `Bearer ${internalCredentials.token}`, + if (!ownershipEntityRefs) { + const userInfoResp = await fetch( + `${await this.discovery.getBaseUrl('auth')}/v1/userinfo`, + { + headers: { + Authorization: `Bearer ${internalCredentials.token}`, + }, }, - }, - ); + ); - if (!userInfoResp.ok) { - throw await ResponseError.fromResponse(userInfoResp); + if (!userInfoResp.ok) { + throw await ResponseError.fromResponse(userInfoResp); + } + + const { + claims: { ent }, + } = await userInfoResp.json(); + ownershipEntityRefs = ent; } - const { sub, ent } = await userInfoResp.json(); + if (!ownershipEntityRefs) { + throw new Error('Ownership entity refs can not be determined'); + } else if ( + !Array.isArray(ownershipEntityRefs) || + ownershipEntityRefs.some(ref => typeof ref !== 'string') + ) { + throw new Error('Ownership entity refs must be an array of strings'); + } - return { userEntityRef: sub, ownershipEntityRefs: ent }; + return { userEntityRef, ownershipEntityRefs }; } } diff --git a/plugins/auth-backend/migrations/20240510120825_user_info.js b/plugins/auth-backend/migrations/20240510120825_user_info.js index ed667a2b7a..63d061a4e7 100644 --- a/plugins/auth-backend/migrations/20240510120825_user_info.js +++ b/plugins/auth-backend/migrations/20240510120825_user_info.js @@ -33,6 +33,11 @@ exports.up = async function up(knex) { .text('user_info', 'longtext') .notNullable() .comment('User info blob, JSON serialized'); + + table + .timestamp('exp') + .notNullable() + .comment('Expiration timestamp of the user info'); }); }; diff --git a/plugins/auth-backend/src/identity/TokenFactory.test.ts b/plugins/auth-backend/src/identity/TokenFactory.test.ts index becf821db7..d4221ed389 100644 --- a/plugins/auth-backend/src/identity/TokenFactory.test.ts +++ b/plugins/auth-backend/src/identity/TokenFactory.test.ts @@ -88,8 +88,7 @@ describe('TokenFactory', () => { ); expect(mockUserInfoDatabaseHandler.addUserInfo).toHaveBeenCalledWith({ - userEntityRef: entityRef, - ownershipEntityRefs: [entityRef], + claims: verifyResult.payload, }); // Emulate the reconstruction of a limited user token diff --git a/plugins/auth-backend/src/identity/TokenFactory.ts b/plugins/auth-backend/src/identity/TokenFactory.ts index 4c7753d998..12798a4d79 100644 --- a/plugins/auth-backend/src/identity/TokenFactory.ts +++ b/plugins/auth-backend/src/identity/TokenFactory.ts @@ -38,10 +38,8 @@ const MAX_TOKEN_LENGTH = 32768; // At 64 bytes per entity ref this still leaves /** * The payload contents of a valid Backstage JWT token - * - * @internal */ -interface BackstageTokenPayload { +export interface BackstageTokenPayload { /** * The issuer of the token, currently the discovery URL of the auth backend */ @@ -222,10 +220,7 @@ export class TokenFactory implements TokenIssuer { // Store the user info in the database upon successful token // issuance so that it can be retrieved later by limited user tokens - await this.userInfoDatabaseHandler.addUserInfo({ - userEntityRef: sub, - ownershipEntityRefs: ent, - }); + await this.userInfoDatabaseHandler.addUserInfo({ claims }); return token; } diff --git a/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.test.ts b/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.test.ts index 4c18e1ff2a..b9f4d84bd5 100644 --- a/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.test.ts +++ b/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.test.ts @@ -52,8 +52,11 @@ describe('UserInfoDatabaseHandler', () => { it('addUserInfo', async () => { const userInfo = { - userEntityRef: 'user:default/foo', - ownershipEntityRefs: ['group:default/foo-group', 'group:default/bar'], + claims: { + sub: 'user:default/foo', + ent: ['group:default/foo-group', 'group:default/bar'], + exp: 1234567890, + }, }; await dbHandler.addUserInfo(userInfo); @@ -62,44 +65,41 @@ describe('UserInfoDatabaseHandler', () => { .where('user_entity_ref', 'user:default/foo') .first(); expect(savedUserInfo).toEqual({ - user_entity_ref: userInfo.userEntityRef, - user_info: JSON.stringify({ - ownershipEntityRefs: userInfo.ownershipEntityRefs, - }), + user_entity_ref: 'user:default/foo', + user_info: JSON.stringify(userInfo), + exp: expect.anything(), }); - userInfo.ownershipEntityRefs = [ - 'group:default/group1', - 'group:default/group2', - ]; + userInfo.claims.ent = ['group:default/group1', 'group:default/group2']; await dbHandler.addUserInfo(userInfo); const updatedUserInfo = await knex('user_info') .where('user_entity_ref', 'user:default/foo') .first(); expect(updatedUserInfo).toEqual({ - user_entity_ref: userInfo.userEntityRef, - user_info: JSON.stringify({ - ownershipEntityRefs: userInfo.ownershipEntityRefs, - }), + user_entity_ref: 'user:default/foo', + user_info: JSON.stringify(userInfo), + exp: expect.anything(), }); }); it('getUserInfo', async () => { const userInfo = { - userEntityRef: 'user:default/backstage-user', - ownershipEntityRefs: ['group:default/group1', 'group:default/group2'], + claims: { + sub: 'user:default/backstage-user', + ent: ['group:default/group1', 'group:default/group2'], + exp: 1234567890, + }, }; await knex('user_info').insert({ - user_entity_ref: userInfo.userEntityRef, - user_info: JSON.stringify({ - ownershipEntityRefs: userInfo.ownershipEntityRefs, - }), + user_entity_ref: 'user:default/backstage-user', + user_info: JSON.stringify(userInfo), + exp: knex.fn.now(), }); const savedUserInfo = await dbHandler.getUserInfo( - userInfo.userEntityRef, + 'user:default/backstage-user', ); expect(savedUserInfo).toEqual(userInfo); }); diff --git a/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.ts b/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.ts index f7dea52236..c0b31de8e4 100644 --- a/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.ts +++ b/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.ts @@ -14,35 +14,40 @@ * limitations under the License. */ -import { BackstageUserInfo } from '@backstage/backend-plugin-api'; +import { DateTime } from 'luxon'; import { Knex } from 'knex'; +import { BackstageTokenPayload } from './TokenFactory'; + const TABLE = 'user_info'; type Row = { user_entity_ref: string; user_info: string; + exp: string; +}; + +type UserInfo = { + claims: Omit; }; -// TODO: How do we prune stale users? export class UserInfoDatabaseHandler { constructor(private readonly client: Knex) {} - async addUserInfo(userInfo: BackstageUserInfo): Promise { + async addUserInfo(userInfo: UserInfo): Promise { await this.client(TABLE) .insert({ - user_entity_ref: userInfo.userEntityRef, - user_info: JSON.stringify({ - ownershipEntityRefs: userInfo.ownershipEntityRefs, - }), + user_entity_ref: userInfo.claims.sub as string, + user_info: JSON.stringify(userInfo), + exp: DateTime.fromSeconds(userInfo.claims.exp as number, { + zone: 'utc', + }).toSQL({ includeOffset: false }), }) .onConflict('user_entity_ref') .merge(); } - async getUserInfo( - userEntityRef: string, - ): Promise { + async getUserInfo(userEntityRef: string): Promise { const info = await this.client(TABLE) .where({ user_entity_ref: userEntityRef }) .first(); @@ -51,10 +56,7 @@ export class UserInfoDatabaseHandler { return undefined; } - const { ownershipEntityRefs } = JSON.parse(info.user_info); - return { - userEntityRef: info.user_entity_ref, - ownershipEntityRefs, - }; + const userInfo = JSON.parse(info.user_info); + return userInfo; } } diff --git a/plugins/auth-backend/src/identity/router.test.ts b/plugins/auth-backend/src/identity/router.test.ts index 1ed9071578..9ede44f195 100644 --- a/plugins/auth-backend/src/identity/router.test.ts +++ b/plugins/auth-backend/src/identity/router.test.ts @@ -28,7 +28,12 @@ describe('bindOidcRouter', () => { it('should return user info for full tokens', async () => { const auth = mockServices.auth.mock(); const mockUserInfoDatabaseHandler = { - getUserInfo: jest.fn().mockResolvedValue(undefined), + getUserInfo: jest.fn().mockResolvedValue({ + claims: { + sub: 'k/ns:n', + ent: ['k/ns:a', 'k/ns:b'], + }, + }), } as unknown as UserInfoDatabaseHandler; const { server } = await startTestBackend({ @@ -70,19 +75,25 @@ describe('bindOidcRouter', () => { )}.s`, ) .expect(200, { - sub: 'k/ns:n', - ent: ['k/ns:a', 'k/ns:b'], + claims: { + sub: 'k/ns:n', + ent: ['k/ns:a', 'k/ns:b'], + }, }); - expect(mockUserInfoDatabaseHandler.getUserInfo).not.toHaveBeenCalled(); + expect(mockUserInfoDatabaseHandler.getUserInfo).toHaveBeenCalledWith( + 'k/ns:n', + ); }); it('should return user info for limited tokens', async () => { const auth = mockServices.auth.mock(); const mockUserInfoDatabaseHandler = { getUserInfo: jest.fn().mockResolvedValue({ - userEntityRef: 'k/ns:n', - ownershipEntityRefs: ['k/ns:a', 'k/ns:b'], + claims: { + sub: 'k/ns:n', + ent: ['k/ns:a', 'k/ns:b'], + }, }), } as unknown as UserInfoDatabaseHandler; @@ -123,8 +134,10 @@ describe('bindOidcRouter', () => { `Bearer h.${btoa(JSON.stringify({ sub: 'k/ns:n' }))}.s`, ) .expect(200, { - sub: 'k/ns:n', - ent: ['k/ns:a', 'k/ns:b'], + claims: { + sub: 'k/ns:n', + ent: ['k/ns:a', 'k/ns:b'], + }, }); expect(mockUserInfoDatabaseHandler.getUserInfo).toHaveBeenCalledWith( diff --git a/plugins/auth-backend/src/identity/router.ts b/plugins/auth-backend/src/identity/router.ts index be9345338f..3952ec64b5 100644 --- a/plugins/auth-backend/src/identity/router.ts +++ b/plugins/auth-backend/src/identity/router.ts @@ -93,30 +93,18 @@ export function bindOidcRouter( ); } - const { sub: userEntityRef, ent: ownershipEntityRefs } = decodeJwt(token); + const { sub: userEntityRef } = decodeJwt(token); if (typeof userEntityRef !== 'string') { throw new Error('Invalid user token, user entity ref must be a string'); } - // Return user info if it's already available in the token (ie. it is a full token) - if ( - Array.isArray(ownershipEntityRefs) && - ownershipEntityRefs.every(ref => typeof ref === 'string') - ) { - res.json({ sub: userEntityRef, ent: ownershipEntityRefs }); - return; - } - const userInfo = await userInfoDatabaseHandler.getUserInfo(userEntityRef); if (!userInfo) { res.status(404).send('User info not found'); return; } - res.json({ - sub: userInfo.userEntityRef, - ent: userInfo.ownershipEntityRefs, - }); + res.json(userInfo); }); } diff --git a/plugins/auth-backend/src/migrations.test.ts b/plugins/auth-backend/src/migrations.test.ts index eeb56785c8..b4a6572a86 100644 --- a/plugins/auth-backend/src/migrations.test.ts +++ b/plugins/auth-backend/src/migrations.test.ts @@ -81,15 +81,25 @@ describe('migrations', () => { await migrateUpOnce(knex); const user_info = JSON.stringify({ - ownershipEntityRefs: ['group:default/group1', 'group:default/group2'], + claims: { + ent: ['group:default/group1', 'group:default/group2'], + }, }); await knex - .insert({ user_entity_ref: 'user:default/backstage-user', user_info }) + .insert({ + user_entity_ref: 'user:default/backstage-user', + user_info, + exp: knex.fn.now(), + }) .into('user_info'); await expect(knex('user_info')).resolves.toEqual([ - { user_entity_ref: 'user:default/backstage-user', user_info }, + { + user_entity_ref: 'user:default/backstage-user', + user_info, + exp: expect.anything(), + }, ]); await migrateDownOnce(knex); From 4a50a8736466fcf1b90d59be311a668255b62d0c Mon Sep 17 00:00:00 2001 From: Phil Kuang Date: Sun, 16 Jun 2024 10:52:30 -0400 Subject: [PATCH 3/3] refactor(userInfo): minor updates Signed-off-by: Phil Kuang --- .changeset/cold-comics-rush.md | 2 ++ packages/backend-app-api/package.json | 1 + .../implementations/userInfo/userInfoServiceFactory.ts | 1 + plugins/auth-backend/src/identity/TokenFactory.test.ts | 3 ++- plugins/auth-backend/src/identity/TokenFactory.ts | 5 ++++- .../src/identity/UserInfoDatabaseHandler.test.ts | 7 ++++--- yarn.lock | 1 + 7 files changed, 15 insertions(+), 5 deletions(-) diff --git a/.changeset/cold-comics-rush.md b/.changeset/cold-comics-rush.md index edb481fd83..4bd9a4de22 100644 --- a/.changeset/cold-comics-rush.md +++ b/.changeset/cold-comics-rush.md @@ -4,3 +4,5 @@ --- Limited user tokens will no longer include the `ent` field in its payload. Ownership claims will now be fetched from the user info service. + +NOTE: Limited tokens issued prior to this change will no longer be valid. Users may have to clear their browser cookies in order to refresh their auth tokens. diff --git a/packages/backend-app-api/package.json b/packages/backend-app-api/package.json index f188b4aa51..9ede813a3a 100644 --- a/packages/backend-app-api/package.json +++ b/packages/backend-app-api/package.json @@ -79,6 +79,7 @@ "minimatch": "^9.0.0", "minimist": "^1.2.5", "morgan": "^1.10.0", + "node-fetch": "^2.6.7", "node-forge": "^1.3.1", "path-to-regexp": "^6.2.1", "selfsigned": "^2.0.0", diff --git a/packages/backend-app-api/src/services/implementations/userInfo/userInfoServiceFactory.ts b/packages/backend-app-api/src/services/implementations/userInfo/userInfoServiceFactory.ts index 5362ff83b0..db550bf9e8 100644 --- a/packages/backend-app-api/src/services/implementations/userInfo/userInfoServiceFactory.ts +++ b/packages/backend-app-api/src/services/implementations/userInfo/userInfoServiceFactory.ts @@ -24,6 +24,7 @@ import { } from '@backstage/backend-plugin-api'; import { ResponseError } from '@backstage/errors'; import { decodeJwt } from 'jose'; +import fetch from 'node-fetch'; import { toInternalBackstageCredentials } from '../auth/helpers'; type Options = { diff --git a/plugins/auth-backend/src/identity/TokenFactory.test.ts b/plugins/auth-backend/src/identity/TokenFactory.test.ts index d4221ed389..34e04aa858 100644 --- a/plugins/auth-backend/src/identity/TokenFactory.test.ts +++ b/plugins/auth-backend/src/identity/TokenFactory.test.ts @@ -22,6 +22,7 @@ import { decodeProtectedHeader, jwtVerify, } from 'jose'; +import { omit } from 'lodash'; import { MemoryKeyStore } from './MemoryKeyStore'; import { TokenFactory } from './TokenFactory'; import { UserInfoDatabaseHandler } from './UserInfoDatabaseHandler'; @@ -88,7 +89,7 @@ describe('TokenFactory', () => { ); expect(mockUserInfoDatabaseHandler.addUserInfo).toHaveBeenCalledWith({ - claims: verifyResult.payload, + claims: omit(verifyResult.payload, ['aud', 'iat', 'iss', 'uip']), }); // Emulate the reconstruction of a limited user token diff --git a/plugins/auth-backend/src/identity/TokenFactory.ts b/plugins/auth-backend/src/identity/TokenFactory.ts index 12798a4d79..a1643cdbe3 100644 --- a/plugins/auth-backend/src/identity/TokenFactory.ts +++ b/plugins/auth-backend/src/identity/TokenFactory.ts @@ -25,6 +25,7 @@ import { GeneralSign, KeyLike, } from 'jose'; +import { omit } from 'lodash'; import { DateTime } from 'luxon'; import { v4 as uuid } from 'uuid'; import { LoggerService } from '@backstage/backend-plugin-api'; @@ -220,7 +221,9 @@ export class TokenFactory implements TokenIssuer { // Store the user info in the database upon successful token // issuance so that it can be retrieved later by limited user tokens - await this.userInfoDatabaseHandler.addUserInfo({ claims }); + await this.userInfoDatabaseHandler.addUserInfo({ + claims: omit(claims, ['aud', 'iat', 'iss', 'uip']), + }); return token; } diff --git a/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.test.ts b/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.test.ts index b9f4d84bd5..ef3d0f6c8b 100644 --- a/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.test.ts +++ b/plugins/auth-backend/src/identity/UserInfoDatabaseHandler.test.ts @@ -24,6 +24,8 @@ const migrationsDir = resolvePackagePath( 'migrations', ); +jest.setTimeout(60_000); + describe('UserInfoDatabaseHandler', () => { const databases = TestDatabases.create(); @@ -41,14 +43,14 @@ describe('UserInfoDatabaseHandler', () => { } describe.each(databases.eachSupportedId())( - '%p', + 'should support database %p', databaseId => { let knex: Knex; let dbHandler: UserInfoDatabaseHandler; beforeEach(async () => { ({ knex, dbHandler } = await createDatabaseHandler(databaseId)); - }, 30000); + }); it('addUserInfo', async () => { const userInfo = { @@ -104,6 +106,5 @@ describe('UserInfoDatabaseHandler', () => { expect(savedUserInfo).toEqual(userInfo); }); }, - 60000, ); }); diff --git a/yarn.lock b/yarn.lock index 977e2b724f..5fb392ee4d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3325,6 +3325,7 @@ __metadata: minimist: ^1.2.5 morgan: ^1.10.0 msw: ^1.0.0 + node-fetch: ^2.6.7 node-forge: ^1.3.1 path-to-regexp: ^6.2.1 selfsigned: ^2.0.0