1
0
mirror of https://github.com/Unleash/unleash.git synced 2025-07-26 13:48:33 +02:00

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
This commit is contained in:
Gastón Fournier 2025-04-17 09:22:35 +02:00 committed by GitHub
parent 61c98a9994
commit 331de07e39
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 76 additions and 19 deletions

View File

@ -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');

View File

@ -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();

View File

@ -38,6 +38,8 @@ jest.mock('./services', () => ({
featureLifecycleService: { listen() {} },
schedulerService: { stop() {}, start() {} },
addonService: { destroy() {} },
userService: { initAdminUser() {} },
apiTokenService: { initApiTokens() {} },
};
},
}));

View File

@ -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<IUnleashConfig, 'authentication'>,
{
userService,
apiTokenService,
}: Pick<IUnleashServices, 'userService' | 'apiTokenService'>,
) {
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);

View File

@ -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();

View File

@ -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,
);
}
}

View File

@ -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;
}

View File

@ -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);