From 331de07e3945603333482b2439109613be8a28a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Thu, 17 Apr 2025 09:22:35 +0200 Subject: [PATCH] fix: double token initialization (#9783) ## About the changes Initially at Unleash we started using `process.nextTick` inside constructors to delay initialization of services. Later we stared using a pattern where we instantiate services multiple times. The problem is the first pattern implies we have singleton services, while the second pattern breaks the singleton. There are reasons for both patterns, but we've decided that `process.nextTick` inside constructors is not something we want to keep as it creates side effects from creating objects. Instead this PR proposes a more explicit approach. Fixes #9775 --- src/lib/create-config.test.ts | 36 ++++++++++++++++++++++ src/lib/create-config.ts | 6 ++++ src/lib/server-impl.test.ts | 2 ++ src/lib/server-impl.ts | 14 +++++++++ src/lib/services/api-token-service.test.ts | 5 +-- src/lib/services/api-token-service.ts | 18 ++++++----- src/lib/services/user-service.ts | 11 +------ src/test/e2e/helpers/test-helper.ts | 3 ++ 8 files changed, 76 insertions(+), 19 deletions(-) diff --git a/src/lib/create-config.test.ts b/src/lib/create-config.test.ts index 940eedd1e3..3a89474c63 100644 --- a/src/lib/create-config.test.ts +++ b/src/lib/create-config.test.ts @@ -499,6 +499,42 @@ test('Config with enterpriseVersion set and not pro environment should set isEnt expect(config.isEnterprise).toBe(true); }); +test('create config should be idempotent in terms of tokens', async () => { + // two admin tokens + process.env.INIT_ADMIN_API_TOKENS = '*:*.some-token1, *:*.some-token2'; + process.env.INIT_CLIENT_API_TOKENS = 'default:development.some-token1'; + process.env.INIT_FRONTEND_API_TOKENS = 'frontend:development.some-token1'; + const token = { + environment: '*', + project: '*', + secret: '*:*.some-random-string', + type: ApiTokenType.ADMIN, + tokenName: 'admin', + }; + const config = createConfig({ + db: { + host: 'localhost', + port: 4242, + user: 'unleash', + password: 'password', + database: 'unleash_db', + }, + server: { + port: 4242, + }, + authentication: { + initApiTokens: [token], + }, + }); + expect(config.authentication.initApiTokens.length).toStrictEqual( + createConfig(config).authentication.initApiTokens.length, + ); + expect(config.authentication.initApiTokens).toHaveLength(5); + delete process.env.INIT_ADMIN_API_TOKENS; + delete process.env.INIT_CLIENT_API_TOKENS; + delete process.env.INIT_FRONTEND_API_TOKENS; +}); + describe('isOSS', () => { test('Config with pro environment should set isOss to false regardless of pro casing', async () => { const isOss = resolveIsOss(false, false, 'Pro'); diff --git a/src/lib/create-config.ts b/src/lib/create-config.ts index 730724b47d..ffd9f942da 100644 --- a/src/lib/create-config.ts +++ b/src/lib/create-config.ts @@ -574,6 +574,12 @@ export function createConfig(options: IUnleashOptions): IUnleashConfig { : options.authentication) || {}, { initApiTokens: initApiTokens }, ]); + // make sure init tokens appear only once + authentication.initApiTokens = [ + ...new Map( + authentication.initApiTokens.map((token) => [token.secret, token]), + ).values(), + ]; const environmentEnableOverrides = loadEnvironmentEnableOverrides(); diff --git a/src/lib/server-impl.test.ts b/src/lib/server-impl.test.ts index 36effa95f8..8f96abf7bc 100644 --- a/src/lib/server-impl.test.ts +++ b/src/lib/server-impl.test.ts @@ -38,6 +38,8 @@ jest.mock('./services', () => ({ featureLifecycleService: { listen() {} }, schedulerService: { stop() {}, start() {} }, addonService: { destroy() {} }, + userService: { initAdminUser() {} }, + apiTokenService: { initApiTokens() {} }, }; }, })); diff --git a/src/lib/server-impl.ts b/src/lib/server-impl.ts index 5ef4d9706c..39c27c2745 100644 --- a/src/lib/server-impl.ts +++ b/src/lib/server-impl.ts @@ -36,6 +36,19 @@ import { defaultLockKey, defaultTimeout, withDbLock } from './util/db-lock'; import { scheduleServices } from './features/scheduler/schedule-services'; import { compareAndLogPostgresVersion } from './util/postgres-version-checker'; +export async function initialServiceSetup( + { authentication }: Pick, + { + userService, + apiTokenService, + }: Pick, +) { + await userService.initAdminUser(authentication); + if (authentication.initApiTokens.length > 0) { + await apiTokenService.initApiTokens(authentication.initApiTokens); + } +} + async function createApp( config: IUnleashConfig, startApp: boolean, @@ -47,6 +60,7 @@ async function createApp( const stores = createStores(config, db); await compareAndLogPostgresVersion(config, stores.settingStore); const services = createServices(stores, config, db); + await initialServiceSetup(config, services); if (!config.disableScheduler) { await scheduleServices(services, config); diff --git a/src/lib/services/api-token-service.test.ts b/src/lib/services/api-token-service.test.ts index 132593c386..5f83f430d4 100644 --- a/src/lib/services/api-token-service.test.ts +++ b/src/lib/services/api-token-service.test.ts @@ -32,11 +32,12 @@ test('Should init api token', async () => { }, }, }); - const { apiTokenStore } = createFakeApiTokenService(config); + const { apiTokenService, apiTokenStore } = + createFakeApiTokenService(config); const insertCalled = new Promise((resolve) => { apiTokenStore.on('insert', resolve); }); - + apiTokenService.initApiTokens([token]); await insertCalled; const tokens = await apiTokenStore.getAll(); diff --git a/src/lib/services/api-token-service.ts b/src/lib/services/api-token-service.ts index f58368ea82..d64b00b50a 100644 --- a/src/lib/services/api-token-service.ts +++ b/src/lib/services/api-token-service.ts @@ -102,11 +102,6 @@ export class ApiTokenService { this.fetchActiveTokens(); } this.updateLastSeen(); - if (config.authentication.initApiTokens.length > 0) { - process.nextTick(async () => - this.initApiTokens(config.authentication.initApiTokens), - ); - } this.timer = (functionName: string) => metricsHelper.wrapTimer(config.eventBus, FUNCTION_TIME, { className: 'ApiTokenService', @@ -199,9 +194,12 @@ export class ApiTokenService { return this.store.getAll(); } - private async initApiTokens(tokens: ILegacyApiTokenCreate[]) { + async initApiTokens(tokens: ILegacyApiTokenCreate[]) { const tokenCount = await this.store.count(); if (tokenCount > 0) { + this.logger.debug( + 'Not creating initial API tokens because tokens exist in the database', + ); return; } try { @@ -209,8 +207,14 @@ export class ApiTokenService { .map(mapLegacyTokenWithSecret) .map((t) => this.insertNewApiToken(t, SYSTEM_USER_AUDIT)); await Promise.all(createAll); + this.logger.info( + `Created initial API tokens: ${tokens.map((t) => `(name: ${t.tokenName}, type: ${t.type})`).join(', ')}`, + ); } catch (e) { - this.logger.error('Unable to create initial Admin API tokens'); + this.logger.warn( + `Unable to create initial API tokens from: ${tokens.map((t) => `(name: ${t.tokenName}, type: ${t.type})`).join(', ')}`, + e, + ); } } diff --git a/src/lib/services/user-service.ts b/src/lib/services/user-service.ts index 32255aabd0..62289199f6 100644 --- a/src/lib/services/user-service.ts +++ b/src/lib/services/user-service.ts @@ -106,18 +106,12 @@ class UserService { { server, getLogger, - authentication, eventBus, flagResolver, session, }: Pick< IUnleashConfig, - | 'getLogger' - | 'authentication' - | 'server' - | 'eventBus' - | 'flagResolver' - | 'session' + 'getLogger' | 'server' | 'eventBus' | 'flagResolver' | 'session' >, services: { accessService: AccessService; @@ -139,9 +133,6 @@ class UserService { this.settingService = services.settingService; this.flagResolver = flagResolver; this.maxParallelSessions = session.maxParallelSessions; - - process.nextTick(() => this.initAdminUser(authentication)); - this.baseUriPath = server.baseUriPath || ''; this.unleashUrl = server.unleashUrl; } diff --git a/src/test/e2e/helpers/test-helper.ts b/src/test/e2e/helpers/test-helper.ts index 52ede02c03..ea68e47ae0 100644 --- a/src/test/e2e/helpers/test-helper.ts +++ b/src/test/e2e/helpers/test-helper.ts @@ -24,6 +24,7 @@ import type { Knex } from 'knex'; import type TestAgent from 'supertest/lib/agent'; import type Test from 'supertest/lib/test'; import type { Server } from 'node:http'; +import { initialServiceSetup } from '../../../lib/server-impl'; process.env.NODE_ENV = 'test'; export interface IUnleashTest extends IUnleashHttpAPI { @@ -357,6 +358,7 @@ async function createApp( }, }); const services = createServices(stores, config, db); + await initialServiceSetup(config, services); // @ts-expect-error We don't have a database for sessions here. const unleashSession = sessionDb(config, undefined); const app = await getApp(config, stores, services, unleashSession, db); @@ -412,6 +414,7 @@ export async function setupAppWithoutSupertest( }, }); const services = createServices(stores, config, db); + await initialServiceSetup(config, services); // @ts-expect-error we don't have a db for the session here const unleashSession = sessionDb(config, undefined); const app = await getApp(config, stores, services, unleashSession, db);