From 79b0b1f3d3dd4e84b787a64890d4e7d3a28209e3 Mon Sep 17 00:00:00 2001 From: Phil Kuang Date: Fri, 7 Jun 2024 16:26:02 -0400 Subject: [PATCH] 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);