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.
Unleash · Jan 17, 2024 · ceaaf3d · ceaaf3d
1 parent 6a5ce1f
commit ceaaf3d
Show file tree

Hide file tree

Showing 11 changed files with 231 additions and 30 deletions.
diff --git 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
@@ -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
@@ -1,18 +1,20 @@
 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,
 } from '../types';
 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
@@ -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,15 +158,20 @@ 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,
                 environment: token.environment,
                 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<IApiToken> {
         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<IApiTokenCreate, 'secret'>,
-        createdBy: string = SYSTEM_USER.username,
-        createdByUserId: number = SYSTEM_USER.id,
+        createdBy?: string | IApiUser | IUser,
+        createdByUserId?: number,
+    ): Promise<IApiToken> {
+        // 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<IApiTokenCreate, 'secret'>,
+        createdBy: string,
+        createdByUserId: number,
     ): Promise<IApiToken> {
         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
@@ -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
@@ -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,15 +82,60 @@ 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<void> {
         return this.storeEvents([event]);
     }
 
+    /**
+     * @deprecated use storeUserEvents instead
+     */
     async storeEvents(events: IBaseEvent[]): Promise<void> {
         let enhancedEvents = events;
         for (const enhancer of [this.enhanceEventsWithTags.bind(this)]) {
             enhancedEvents = await enhancer(enhancedEvents);
         }
         return this.eventStore.batchStore(enhancedEvents);
     }
+
+    async storeUserEvent(event: IUserEvent): Promise<void> {
+        return this.storeUserEvents([event]);
+    }
+
+    async storeUserEvents(events: IUserEvent[]): Promise<void> {
+        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
@@ -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
@@ -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
@@ -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
@@ -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),
 });