From ceaaf3d0f317c7fb0b3821f9710e8957cbcb63a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Wed, 17 Jan 2024 16:55:59 +0100 Subject: [PATCH] feat: admin token calls get an admin token user (#5924) ## About the changes Whenever we get a call from an admin token we want to associate it with the [admin token user](https://github.com/Unleash/unleash/blob/4d42093a07a86dcff1766425c8429b20dade8e0d/src/lib/types/core.ts#L34-L41). This should give us the needed audit for this type of calls that currently were lacking a user id (we only stored a string with the token name in the event log). We consciously decided not to use `id` as the property to prevent any unforeseen side effects. The reason is that only `IUser` type has an id and adding an id to `IApiUser` might lead to confusion. --- src/lib/routes/admin-api/api-token.ts | 3 +- src/lib/routes/admin-api/project/api-token.ts | 3 +- src/lib/services/api-token-service.test.ts | 40 +++++++++++--- src/lib/services/api-token-service.ts | 52 ++++++++++++++++--- src/lib/services/event-service.test.ts | 20 +++++++ src/lib/services/event-service.ts | 50 +++++++++++++++++- src/lib/types/api-user.ts | 1 + src/lib/types/events.ts | 15 ++++++ src/lib/util/extract-user.test.ts | 13 +++-- src/lib/util/extract-user.ts | 19 ++++--- .../e2e/api/admin/api-token.auth.e2e.test.ts | 45 +++++++++++++++- 11 files changed, 231 insertions(+), 30 deletions(-) create mode 100644 src/lib/services/event-service.test.ts diff --git a/src/lib/routes/admin-api/api-token.ts b/src/lib/routes/admin-api/api-token.ts index 0fcfbc3484..c35c2c7c07 100644 --- a/src/lib/routes/admin-api/api-token.ts +++ b/src/lib/routes/admin-api/api-token.ts @@ -42,7 +42,7 @@ import { getStandardResponses, } from '../../openapi/util/standard-responses'; import { ProxyService } from '../../services/proxy-service'; -import { extractUsername } from '../../util'; +import { extractUserId, extractUsername } from '../../util'; import { OperationDeniedError } from '../../error'; interface TokenParam { @@ -323,6 +323,7 @@ export class ApiTokenController extends Controller { const token = await this.apiTokenService.createApiToken( createToken, extractUsername(req), + extractUserId(req), ); this.openApiService.respondWithValidation( 201, diff --git a/src/lib/routes/admin-api/project/api-token.ts b/src/lib/routes/admin-api/project/api-token.ts index 3a1631794f..2eb4b23d4e 100644 --- a/src/lib/routes/admin-api/project/api-token.ts +++ b/src/lib/routes/admin-api/project/api-token.ts @@ -27,7 +27,7 @@ import { ProjectService, ProxyService, } from '../../../services'; -import { extractUsername } from '../../../util'; +import { extractUserId, extractUsername } from '../../../util'; import { IAuthRequest } from '../../unleash-types'; import Controller from '../../controller'; import { Logger } from '../../../logger'; @@ -190,6 +190,7 @@ export class ProjectApiTokenController extends Controller { const token = await this.apiTokenService.createApiToken( createToken, extractUsername(req), + extractUserId(req), ); this.openApiService.respondWithValidation( 201, diff --git a/src/lib/services/api-token-service.test.ts b/src/lib/services/api-token-service.test.ts index 49c8af45f7..25aad84b7d 100644 --- a/src/lib/services/api-token-service.test.ts +++ b/src/lib/services/api-token-service.test.ts @@ -1,11 +1,12 @@ import { ApiTokenService } from './api-token-service'; import { createTestConfig } from '../../test/config/test-config'; -import { IUnleashConfig } from '../server-impl'; +import { IUnleashConfig, IUser } from '../server-impl'; import { ApiTokenType, IApiTokenCreate } from '../types/models/api-token'; import FakeApiTokenStore from '../../test/fixtures/fake-api-token-store'; import FakeEnvironmentStore from '../features/project-environments/fake-environment-store'; import FakeEventStore from '../../test/fixtures/fake-event-store'; import { + ADMIN_TOKEN_USER, API_TOKEN_CREATED, API_TOKEN_DELETED, API_TOKEN_UPDATED, @@ -13,6 +14,7 @@ import { import { addDays } from 'date-fns'; import EventService from './event-service'; import FakeFeatureTagStore from '../../test/fixtures/fake-feature-tag-store'; +import { createFakeEventsService } from '../../lib/features'; test('Should init api token', async () => { const token = { @@ -34,13 +36,7 @@ test('Should init api token', async () => { apiTokenStore.on('insert', resolve); }); - const eventService = new EventService( - { - eventStore: new FakeEventStore(), - featureTagStore: new FakeFeatureTagStore(), - }, - config, - ); + const eventService = createFakeEventsService(config); new ApiTokenService( { apiTokenStore, environmentStore }, @@ -161,3 +157,31 @@ test('Api token operations should all have events attached', async () => { expect(deletedApiTokenEvents[0].preData).toBeDefined(); expect(deletedApiTokenEvents[0].preData.secret).toBeUndefined(); }); + +test('getUserForToken should get a user with admin token user id and token name', async () => { + const config = createTestConfig(); + const apiTokenStore = new FakeApiTokenStore(); + const environmentStore = new FakeEnvironmentStore(); + + const eventService = createFakeEventsService(config); + + const tokenService = new ApiTokenService( + { apiTokenStore, environmentStore }, + config, + eventService, + ); + const token = await tokenService.createApiTokenWithProjects( + { + environment: '*', + projects: ['*'], + type: ApiTokenType.ADMIN, + tokenName: 'admin.token', + }, + ADMIN_TOKEN_USER as IUser, + ); + + const user = tokenService.getUserForToken(token.secret); + expect(user).toBeDefined(); + expect(user!.username).toBe(token.tokenName); + expect(user!.internalAdminTokenUserId).toBe(ADMIN_TOKEN_USER.id); +}); diff --git a/src/lib/services/api-token-service.ts b/src/lib/services/api-token-service.ts index 5072c91756..49eef45f54 100644 --- a/src/lib/services/api-token-service.ts +++ b/src/lib/services/api-token-service.ts @@ -20,13 +20,19 @@ import BadDataError from '../error/bad-data-error'; import { IEnvironmentStore } from '../features/project-environments/environment-store-type'; import { constantTimeCompare } from '../util/constantTimeCompare'; import { + ADMIN_TOKEN_USER, ApiTokenCreatedEvent, ApiTokenDeletedEvent, ApiTokenUpdatedEvent, + IUser, SYSTEM_USER, SYSTEM_USER_ID, } from '../types'; -import { omitKeys } from '../util'; +import { + extractUserIdFromUser, + extractUsernameFromUser, + omitKeys, +} from '../util'; import EventService from './event-service'; const resolveTokenPermissions = (tokenType: string) => { @@ -152,8 +158,7 @@ export class ApiTokenService { if (token) { this.lastSeenSecrets.add(token.secret); - - return new ApiUser({ + const apiUser: IApiUser = new ApiUser({ tokenName: token.tokenName, permissions: resolveTokenPermissions(token.type), projects: token.projects, @@ -161,6 +166,12 @@ export class ApiTokenService { type: token.type, secret: token.secret, }); + + apiUser.internalAdminTokenUserId = + token.type === ApiTokenType.ADMIN + ? ADMIN_TOKEN_USER.id + : undefined; + return apiUser; } return undefined; @@ -212,17 +223,46 @@ export class ApiTokenService { createdByUserId: number = SYSTEM_USER.id, ): Promise { const token = mapLegacyToken(newToken); - return this.createApiTokenWithProjects( + return this.internalCreateApiTokenWithProjects( token, createdBy, createdByUserId, ); } + /** + * @param newToken + * @param createdBy should be IApiUser or IUser. Still supports optional or string for backward compatibility + * @param createdByUserId still supported for backward compatibility + */ public async createApiTokenWithProjects( newToken: Omit, - createdBy: string = SYSTEM_USER.username, - createdByUserId: number = SYSTEM_USER.id, + createdBy?: string | IApiUser | IUser, + createdByUserId?: number, + ): Promise { + // if statement to support old method signature + if ( + createdBy === undefined || + typeof createdBy === 'string' || + createdByUserId + ) { + return this.internalCreateApiTokenWithProjects( + newToken, + (createdBy as string) || SYSTEM_USER.username, + createdByUserId || SYSTEM_USER.id, + ); + } + return this.internalCreateApiTokenWithProjects( + newToken, + extractUsernameFromUser(createdBy), + extractUserIdFromUser(createdBy), + ); + } + + private async internalCreateApiTokenWithProjects( + newToken: Omit, + createdBy: string, + createdByUserId: number, ): Promise { validateApiToken(newToken); const environments = await this.environmentStore.getAll(); diff --git a/src/lib/services/event-service.test.ts b/src/lib/services/event-service.test.ts new file mode 100644 index 0000000000..bd74a86251 --- /dev/null +++ b/src/lib/services/event-service.test.ts @@ -0,0 +1,20 @@ +import { ADMIN_TOKEN_USER, IApiUser } from '../types'; +import { createTestConfig } from '../../test/config/test-config'; +import { createFakeEventsService } from '../../lib/features'; +import { ApiTokenType } from '../../lib/types/models/api-token'; + +test('when using an admin token should get the username of the token and the id from internalAdminTokenUserId', async () => { + const adminToken: IApiUser = { + projects: ['*'], + environment: '*', + type: ApiTokenType.ADMIN, + secret: '', + username: 'admin-token-username', + permissions: [], + internalAdminTokenUserId: ADMIN_TOKEN_USER.id, + }; + const eventService = createFakeEventsService(createTestConfig()); + const userDetails = eventService.getUserDetails(adminToken); + expect(userDetails.createdBy).toBe('admin-token-username'); + expect(userDetails.createdByUserId).toBe(ADMIN_TOKEN_USER.id); +}); diff --git a/src/lib/services/event-service.ts b/src/lib/services/event-service.ts index 35df38206c..01223b23e0 100644 --- a/src/lib/services/event-service.ts +++ b/src/lib/services/event-service.ts @@ -2,10 +2,11 @@ import { IUnleashConfig } from '../types/option'; import { IFeatureTagStore, IUnleashStores } from '../types/stores'; import { Logger } from '../logger'; import { IEventStore } from '../types/stores/event-store'; -import { IBaseEvent, IEventList } from '../types/events'; +import { IBaseEvent, IEventList, IUserEvent } from '../types/events'; import { SearchEventsSchema } from '../openapi/spec/search-events-schema'; import EventEmitter from 'events'; -import { ITag } from '../types'; +import { ADMIN_TOKEN_USER, IApiUser, ITag, IUser, SYSTEM_USER } from '../types'; +import { ApiTokenType } from '../../lib/types/models/api-token'; export default class EventService { private logger: Logger; @@ -81,10 +82,38 @@ export default class EventService { return events; } + isAdminToken(user: IUser | IApiUser): boolean { + return (user as IApiUser)?.type === ApiTokenType.ADMIN; + } + + getUserDetails(user: IUser | IApiUser): { + createdBy: string; + createdByUserId: number; + } { + return { + createdBy: + (user as IUser)?.email || + user?.username || + (this.isAdminToken(user) + ? ADMIN_TOKEN_USER.username + : SYSTEM_USER.username), + createdByUserId: + (user as IUser)?.id || + (user as IApiUser)?.internalAdminTokenUserId || + SYSTEM_USER.id, + }; + } + + /** + * @deprecated use storeUserEvent instead + */ async storeEvent(event: IBaseEvent): Promise { return this.storeEvents([event]); } + /** + * @deprecated use storeUserEvents instead + */ async storeEvents(events: IBaseEvent[]): Promise { let enhancedEvents = events; for (const enhancer of [this.enhanceEventsWithTags.bind(this)]) { @@ -92,4 +121,21 @@ export default class EventService { } return this.eventStore.batchStore(enhancedEvents); } + + async storeUserEvent(event: IUserEvent): Promise { + return this.storeUserEvents([event]); + } + + async storeUserEvents(events: IUserEvent[]): Promise { + let enhancedEvents = events.map(({ byUser, ...event }) => { + return { + ...event, + ...this.getUserDetails(byUser), + }; + }); + for (const enhancer of [this.enhanceEventsWithTags.bind(this)]) { + enhancedEvents = await enhancer(enhancedEvents); + } + return this.eventStore.batchStore(enhancedEvents); + } } diff --git a/src/lib/types/api-user.ts b/src/lib/types/api-user.ts index a62c6b0a80..f875589cfc 100644 --- a/src/lib/types/api-user.ts +++ b/src/lib/types/api-user.ts @@ -14,6 +14,7 @@ export interface IApiUserData { } export interface IApiUser { + internalAdminTokenUserId?: number; // user associated to an admin token username: string; permissions: string[]; projects: string[]; diff --git a/src/lib/types/events.ts b/src/lib/types/events.ts index 0b545ebbaf..5e9e6d95d0 100644 --- a/src/lib/types/events.ts +++ b/src/lib/types/events.ts @@ -1,4 +1,5 @@ import { extractUsernameFromUser } from '../util'; +import { IApiUser } from './api-user'; import { FeatureToggle, IStrategyConfig, ITag, IVariant } from './model'; import { IApiToken } from './models/api-token'; import { IUser, IUserWithRootRole } from './user'; @@ -336,6 +337,9 @@ export const IEventTypes = [ ] as const; export type IEventType = (typeof IEventTypes)[number]; +/** + * This type should only be used in the store layer but deprecated elsewhere + */ export interface IBaseEvent { type: IEventType; createdBy: string; @@ -348,6 +352,17 @@ export interface IBaseEvent { tags?: ITag[]; } +export interface IUserEvent { + type: IEventType; + byUser: IUser | IApiUser; + project?: string; + environment?: string; + featureName?: string; + data?: any; + preData?: any; + tags?: ITag[]; +} + export interface IEvent extends IBaseEvent { id: number; createdAt: Date; diff --git a/src/lib/util/extract-user.test.ts b/src/lib/util/extract-user.test.ts index 85494876e1..322f88a25e 100644 --- a/src/lib/util/extract-user.test.ts +++ b/src/lib/util/extract-user.test.ts @@ -1,5 +1,6 @@ +import { SYSTEM_USER } from '../../lib/types'; import { IUser } from '../server-impl'; -import { extractUsernameFromUser } from './extract-user'; +import { extractUserIdFromUser, extractUsernameFromUser } from './extract-user'; describe('extractUsernameFromUser', () => { test('Should return the email if it exists', () => { @@ -19,14 +20,16 @@ describe('extractUsernameFromUser', () => { expect(extractUsernameFromUser(user)).toBe(user.username); }); - test('Should return "unknown" if neither email nor username exists', () => { + test('Should return the system user if neither email nor username exists', () => { const user = {} as IUser; - expect(extractUsernameFromUser(user)).toBe('unknown'); + expect(extractUsernameFromUser(user)).toBe(SYSTEM_USER.username); + expect(extractUserIdFromUser(user)).toBe(SYSTEM_USER.id); }); - test('Should return "unknown" if user is null', () => { + test('Should return the system user if user is null', () => { const user = null as unknown as IUser; - expect(extractUsernameFromUser(user)).toBe('unknown'); + expect(extractUsernameFromUser(user)).toBe(SYSTEM_USER.username); + expect(extractUserIdFromUser(user)).toBe(SYSTEM_USER.id); }); }); diff --git a/src/lib/util/extract-user.ts b/src/lib/util/extract-user.ts index d58e8d8946..d753c0c8c7 100644 --- a/src/lib/util/extract-user.ts +++ b/src/lib/util/extract-user.ts @@ -1,16 +1,23 @@ -import { IAuthRequest, IUser } from '../server-impl'; +import { SYSTEM_USER } from '../../lib/types'; +import { IApiRequest, IApiUser, IAuthRequest, IUser } from '../server-impl'; -export function extractUsernameFromUser(user: IUser): string { - return user?.email || user?.username || 'unknown'; +export function extractUsernameFromUser(user: IUser | IApiUser): string { + return (user as IUser)?.email || user?.username || SYSTEM_USER.username; } -export function extractUsername(req: IAuthRequest): string { +export function extractUsername(req: IAuthRequest | IApiRequest): string { return extractUsernameFromUser(req.user); } -export const extractUserId = (req: IAuthRequest) => req.user.id; +export const extractUserIdFromUser = (user: IUser | IApiUser) => + (user as IUser)?.id || + (user as IApiUser)?.internalAdminTokenUserId || + SYSTEM_USER.id; -export const extractUserInfo = (req: IAuthRequest) => ({ +export const extractUserId = (req: IAuthRequest | IApiRequest) => + extractUserIdFromUser(req.user); + +export const extractUserInfo = (req: IAuthRequest | IApiRequest) => ({ id: extractUserId(req), username: extractUsername(req), }); diff --git a/src/test/e2e/api/admin/api-token.auth.e2e.test.ts b/src/test/e2e/api/admin/api-token.auth.e2e.test.ts index 0fd0b5dfd3..1a1f03fb71 100644 --- a/src/test/e2e/api/admin/api-token.auth.e2e.test.ts +++ b/src/test/e2e/api/admin/api-token.auth.e2e.test.ts @@ -1,9 +1,13 @@ -import { setupAppWithCustomAuth } from '../../helpers/test-helper'; +import { + setupAppWithAuth, + setupAppWithCustomAuth, +} from '../../helpers/test-helper'; import dbInit, { ITestDb } from '../../helpers/database-init'; import getLogger from '../../../fixtures/no-logger'; import { ApiTokenType } from '../../../../lib/types/models/api-token'; import { RoleName } from '../../../../lib/types/model'; import { + ADMIN_TOKEN_USER, CREATE_CLIENT_API_TOKEN, CREATE_PROJECT_API_TOKEN, DELETE_CLIENT_API_TOKEN, @@ -34,6 +38,16 @@ afterEach(async () => { await stores.apiTokenStore.deleteAll(); }); +const getLastEvent = async () => { + const events = await db.stores.eventStore.getEvents(); + return events.reduce((last, current) => { + if (current.id > last.id) { + return current; + } + return last; + }); +}; + test('editor users should only get client or frontend tokens', async () => { expect.assertions(3); @@ -190,6 +204,35 @@ test('Token-admin should be allowed to create token', async () => { await destroy(); }); +test('An admin token should be allowed to create a token', async () => { + expect.assertions(2); + + const adminToken = await db.stores.apiTokenStore.insert({ + type: ApiTokenType.ADMIN, + secret: '12345', + environment: '', + projects: [], + tokenName: 'default-admin', + }); + + const { request, destroy } = await setupAppWithAuth(stores); + + await request + .post('/api/admin/api-tokens') + .send({ + username: 'default-admin', + type: 'admin', + }) + .set('Authorization', adminToken.secret) + .set('Content-Type', 'application/json') + .expect(201); + + const event = await getLastEvent(); + expect(event.createdBy).toBe(adminToken.tokenName); + expect(event.createdByUserId).toBe(ADMIN_TOKEN_USER.id); + await destroy(); +}); + test('A role with only CREATE_PROJECT_API_TOKEN can create project tokens', async () => { expect.assertions(0);