From d9fb624c390c59ad3533385ba35401f55c19c7f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Thu, 21 Aug 2025 20:16:09 +0200 Subject: [PATCH] Fixed 304 tests --- src/lib/app.ts | 16 +- .../configuration-revision-service.ts | 3 +- .../feature-toggle/feature-toggle-service.ts | 5 +- src/lib/middleware/api-token-middleware.ts | 3 +- src/lib/middleware/rbac-middleware.ts | 1 + .../api/client/feature.optimal304.e2e.test.ts | 411 ++++++++++-------- 6 files changed, 255 insertions(+), 184 deletions(-) diff --git a/src/lib/app.ts b/src/lib/app.ts index 0037c0d88b..dd8ac6cf62 100644 --- a/src/lib/app.ts +++ b/src/lib/app.ts @@ -9,7 +9,7 @@ import { corsOriginMiddleware, } from './middleware/index.js'; import rbacMiddleware from './middleware/rbac-middleware.js'; -import apiTokenMiddleware from './middleware/api-token-middleware.js'; +import { apiAccessMiddleware } from './middleware/api-token-middleware.js'; import type { IUnleashServices } from './services/index.js'; import { IAuthType, type IUnleashConfig } from './types/option.js'; import type { IUnleashStores } from './types/index.js'; @@ -116,26 +116,26 @@ export default async function getApp( switch (config.authentication.type) { case IAuthType.OPEN_SOURCE: { - app.use(baseUriPath, apiTokenMiddleware(config, services)); + app.use(baseUriPath, apiAccessMiddleware(config, services)); ossAuthentication(app, config.getLogger, config.server.baseUriPath); break; } case IAuthType.ENTERPRISE: { - app.use(baseUriPath, apiTokenMiddleware(config, services)); + app.use(baseUriPath, apiAccessMiddleware(config, services)); if (config.authentication.customAuthHandler) { config.authentication.customAuthHandler(app, config, services); } break; } case IAuthType.HOSTED: { - app.use(baseUriPath, apiTokenMiddleware(config, services)); + app.use(baseUriPath, apiAccessMiddleware(config, services)); if (config.authentication.customAuthHandler) { config.authentication.customAuthHandler(app, config, services); } break; } case IAuthType.DEMO: { - app.use(baseUriPath, apiTokenMiddleware(config, services)); + app.use(baseUriPath, apiAccessMiddleware(config, services)); demoAuthentication( app, config.server.baseUriPath, @@ -145,7 +145,7 @@ export default async function getApp( break; } case IAuthType.CUSTOM: { - app.use(baseUriPath, apiTokenMiddleware(config, services)); + app.use(baseUriPath, apiAccessMiddleware(config, services)); if (config.authentication.customAuthHandler) { config.authentication.customAuthHandler(app, config, services); } @@ -156,12 +156,12 @@ export default async function getApp( 'The AuthType=none option for Unleash is no longer recommended and will be removed in version 6.', ); noApiToken(baseUriPath, app); - app.use(baseUriPath, apiTokenMiddleware(config, services)); + app.use(baseUriPath, apiAccessMiddleware(config, services)); noAuthentication(baseUriPath, app); break; } default: { - app.use(baseUriPath, apiTokenMiddleware(config, services)); + app.use(baseUriPath, apiAccessMiddleware(config, services)); demoAuthentication( app, config.server.baseUriPath, diff --git a/src/lib/features/feature-toggle/configuration-revision-service.ts b/src/lib/features/feature-toggle/configuration-revision-service.ts index 85897a7b1e..d46e7544e7 100644 --- a/src/lib/features/feature-toggle/configuration-revision-service.ts +++ b/src/lib/features/feature-toggle/configuration-revision-service.ts @@ -10,7 +10,7 @@ import EventEmitter from 'events'; export const UPDATE_REVISION = 'UPDATE_REVISION'; export default class ConfigurationRevisionService extends EventEmitter { - private static instance: ConfigurationRevisionService; + private static instance: ConfigurationRevisionService | undefined; private logger: Logger; @@ -111,5 +111,6 @@ export default class ConfigurationRevisionService extends EventEmitter { destroy(): void { ConfigurationRevisionService.instance?.removeAllListeners(); + ConfigurationRevisionService.instance = undefined; } } diff --git a/src/lib/features/feature-toggle/feature-toggle-service.ts b/src/lib/features/feature-toggle/feature-toggle-service.ts index 293ae8a6cc..658aa72926 100644 --- a/src/lib/features/feature-toggle/feature-toggle-service.ts +++ b/src/lib/features/feature-toggle/feature-toggle-service.ts @@ -2538,7 +2538,10 @@ export class FeatureToggleService { environment, )); if (!canAddStrategies) { - throw new PermissionError(CREATE_FEATURE_STRATEGY); + throw new PermissionError( + CREATE_FEATURE_STRATEGY, + environment, + ); } } } diff --git a/src/lib/middleware/api-token-middleware.ts b/src/lib/middleware/api-token-middleware.ts index 1c17cdc4a3..d1d0d53c5f 100644 --- a/src/lib/middleware/api-token-middleware.ts +++ b/src/lib/middleware/api-token-middleware.ts @@ -31,7 +31,8 @@ export const TOKEN_TYPE_ERROR_MESSAGE = export const NO_TOKEN_WHERE_TOKEN_WAS_REQUIRED = 'This endpoint requires an API token. Please add an authorization header to your request with a valid token'; -const apiAccessMiddleware = ( + +export const apiAccessMiddleware = ( { getLogger, authentication, diff --git a/src/lib/middleware/rbac-middleware.ts b/src/lib/middleware/rbac-middleware.ts index 982e74aa90..0a4c07e94c 100644 --- a/src/lib/middleware/rbac-middleware.ts +++ b/src/lib/middleware/rbac-middleware.ts @@ -87,6 +87,7 @@ const rbacMiddleware = ( ) ) { const { featureName } = params; + // TODO track if this deprecated path is still in use projectId = await featureToggleStore.getProjectId(featureName); } else if ( projectId === undefined && diff --git a/src/test/e2e/api/client/feature.optimal304.e2e.test.ts b/src/test/e2e/api/client/feature.optimal304.e2e.test.ts index c90a40ddad..9b29c3fd93 100644 --- a/src/test/e2e/api/client/feature.optimal304.e2e.test.ts +++ b/src/test/e2e/api/client/feature.optimal304.e2e.test.ts @@ -1,220 +1,285 @@ import { type IUnleashTest, - setupAppWithCustomConfig, + setupAppWithAuth, } from '../../helpers/test-helper.js'; import dbInit, { type ITestDb } from '../../helpers/database-init.js'; import getLogger from '../../../fixtures/no-logger.js'; -import type User from '../../../../lib/types/user.js'; -import { TEST_AUDIT_USER } from '../../../../lib/types/index.js'; import { CHANGE_REQUEST_CREATED } from '../../../../lib/events/index.js'; -// import { DEFAULT_ENV } from '../../../../lib/util/constants'; +import { CLIENT, DEFAULT_ENV } from '../../../../lib/server-impl.js'; +import { ApiTokenType } from '../../../../lib/types/model.js'; -const testUser = { name: 'test', id: -9999 } as User; +const validTokens = [ + { + tokenName: `client-dev-token`, + permissions: [CLIENT], + projects: ['*'], + environment: 'development', + type: ApiTokenType.CLIENT, + secret: '*:development.client', + }, + { + tokenName: `client-prod-token`, + permissions: [CLIENT], + projects: ['*'], + environment: 'production', + type: ApiTokenType.CLIENT, + secret: '*:production.client', + }, +]; +const devTokenSecret = validTokens[0].secret; +const prodTokenSecret = validTokens[1].secret; +async function setup({ + etagVariantName, + enabled, +}: { + etagVariantName: string; + enabled: boolean; +}): Promise<{ app: IUnleashTest; db: ITestDb }> { + const db = await dbInit(`ignored`, getLogger); -const shutdownHooks: (() => Promise)[] = []; + // Create per-environment client tokens so we can request specific environment snapshots + const app = await setupAppWithAuth( + db.stores, + { + authentication: { + enableApiToken: true, + initApiTokens: validTokens, + }, + experimental: { + flags: { + strictSchemaValidation: true, + etagVariant: { + name: etagVariantName, + enabled, + feature_enabled: enabled, + }, + }, + }, + }, + db.rawDatabase, + ); + + return { app, db }; +} + +async function initialize({ app, db }: { app: IUnleashTest; db: ITestDb }) { + const allEnvs = await app.services.environmentService.getAll(); + const nonDefaultEnv = allEnvs.find((env) => env.name !== DEFAULT_ENV)!.name; + + await app.createFeature('X'); + await app.createFeature('Y'); + await app.archiveFeature('Y'); + await app.createFeature('Z'); + await app.enableFeature('Z', DEFAULT_ENV); + await app.enableFeature('Z', nonDefaultEnv); + + await app.services.eventService.storeEvent({ + type: CHANGE_REQUEST_CREATED, + createdBy: 'some@example.com', + createdByUserId: 123, + ip: '127.0.0.1', + featureName: `X`, + }); + + /** + * This helps reason about the etag, which is formed by : + * To see the output you need to run this test with --silent=false + * You can see the expected output in the expect statement below + */ + const { events } = await app.services.eventService.getEvents(); + // NOTE: events could be processed in different order resulting in a flaky test + const actualEvents = events + .reverse() + .map(({ id, environment, featureName, type }) => ({ + id, + environment, + featureName, + type, + })); + const expectedEvents = [ + { + id: 7, + featureName: 'X', + type: 'feature-created', + }, + { + id: 8, + featureName: 'Y', + type: 'feature-created', + }, + { + id: 9, + featureName: 'Y', + type: 'feature-archived', + }, + { + id: 10, + featureName: 'Z', + type: 'feature-created', + }, + { + id: 11, + environment: 'development', + featureName: 'Z', + type: 'feature-strategy-add', + }, + { + id: 12, + environment: 'development', + featureName: 'Z', + type: 'feature-environment-enabled', + }, + { + id: 13, + environment: 'production', + featureName: 'Z', + type: 'feature-strategy-add', + }, + { + id: 14, + environment: 'production', + featureName: 'Z', + type: 'feature-environment-enabled', + }, + { + id: 15, + featureName: 'X', + type: 'change-request-created', + }, + ]; + // We only require that all expectedEvents exist within actualEvents, matching + // only on the properties explicitly specified in each expected object. + // This lets us omit properties (like id) from some expected entries that might + // arrive in different order, without breaking the test. + for (const expectedEvent of expectedEvents) { + expect(actualEvents).toContainEqual( + expect.objectContaining(expectedEvent), + ); + } +} describe.each([ { name: 'disabled', enabled: false, - feature_enabled: false, }, { name: 'v2', enabled: true, - feature_enabled: true, }, -])('feature 304 api client (etag variant = %s)', (etagVariant) => { +])('feature 304 api client (etag variant = $name)', ({ name, enabled }) => { let app: IUnleashTest; let db: ITestDb; - const apendix = etagVariant.feature_enabled - ? `${etagVariant.name}` - : 'etag_variant_off'; beforeAll(async () => { - db = await dbInit(`feature_304_api_client_${apendix}`, getLogger); - app = await setupAppWithCustomConfig( - db.stores, - { - experimental: { - flags: { - strictSchemaValidation: true, - etagVariant: etagVariant, - }, - }, - }, - db.rawDatabase, - ); - - await app.services.featureToggleService.createFeatureToggle( - 'default', - { - name: `featureX${apendix}`, - description: 'the #1 feature', - impressionData: true, - }, - TEST_AUDIT_USER, - ); - await app.services.featureToggleService.createFeatureToggle( - 'default', - { - name: `featureY${apendix}`, - description: 'soon to be the #1 feature', - }, - TEST_AUDIT_USER, - ); - await app.services.featureToggleService.createFeatureToggle( - 'default', - { - name: `featureZ${apendix}`, - description: 'terrible feature', - }, - TEST_AUDIT_USER, - ); - await app.services.featureToggleService.createFeatureToggle( - 'default', - { - name: `featureArchivedX${apendix}`, - description: 'the #1 feature', - }, - TEST_AUDIT_USER, - ); - - await app.services.featureToggleService.archiveToggle( - `featureArchivedX${apendix}`, - testUser, - TEST_AUDIT_USER, - ); - - await app.services.featureToggleService.createFeatureToggle( - 'default', - { - name: `featureArchivedY${apendix}`, - description: 'soon to be the #1 feature', - }, - TEST_AUDIT_USER, - ); - - await app.services.featureToggleService.archiveToggle( - `featureArchivedY${apendix}`, - testUser, - TEST_AUDIT_USER, - ); - await app.services.featureToggleService.createFeatureToggle( - 'default', - { - name: `featureArchivedZ${apendix}`, - description: 'terrible feature', - }, - TEST_AUDIT_USER, - ); - await app.services.featureToggleService.archiveToggle( - `featureArchivedZ${apendix}`, - testUser, - TEST_AUDIT_USER, - ); - await app.services.featureToggleService.createFeatureToggle( - 'default', - { - name: `feature.with.variants${apendix}`, - description: 'A feature flag with variants', - }, - TEST_AUDIT_USER, - ); - await app.services.featureToggleService.saveVariants( - `feature.with.variants${apendix}`, - 'default', - [ - { - name: 'control', - weight: 50, - weightType: 'fix', - stickiness: 'default', - }, - { - name: 'new', - weight: 50, - weightType: 'variable', - stickiness: 'default', - }, - ], - TEST_AUDIT_USER, - ); - - await app.services.eventService.storeEvent({ - type: CHANGE_REQUEST_CREATED, - createdBy: testUser.email, - createdByUserId: testUser.id, - ip: '127.0.0.1', - featureName: `ch-on-feature-${apendix}`, - }); - - shutdownHooks.push(async () => { - await app.destroy(); - await db.destroy(); - }); + ({ app, db } = await setup({ + etagVariantName: name, + enabled, + })); + await initialize({ app, db }); }); - test('returns calculated hash', async () => { + afterAll(async () => { + await app.destroy(); + await db.destroy(); + }); + + test('returns calculated hash without if-none-match header (dev env token)', async () => { const res = await app.request .get('/api/client/features') + .set('Authorization', devTokenSecret) .expect('Content-Type', /json/) .expect(200); - if (etagVariant.feature_enabled) { - expect(res.headers.etag).toBe(`"76d8bb0e:16:${etagVariant.name}"`); - expect(res.body.meta.etag).toBe( - `"76d8bb0e:16:${etagVariant.name}"`, - ); + if (enabled) { + expect(res.headers.etag).toBe(`"76d8bb0e:12:${name}"`); + expect(res.body.meta.etag).toBe(`"76d8bb0e:12:${name}"`); } else { - expect(res.headers.etag).toBe('"76d8bb0e:16"'); - expect(res.body.meta.etag).toBe('"76d8bb0e:16"'); + expect(res.headers.etag).toBe('"76d8bb0e:12"'); + expect(res.body.meta.etag).toBe('"76d8bb0e:12"'); } }); - test(`returns ${etagVariant.feature_enabled ? 200 : 304} for pre-calculated hash${etagVariant.feature_enabled ? ' because hash changed' : ''}`, async () => { + test(`returns ${enabled ? 200 : 304} for pre-calculated hash${enabled ? ' because hash changed' : ''} (dev env token)`, async () => { const res = await app.request .get('/api/client/features') - .set('if-none-match', '"76d8bb0e:16"') - .expect(etagVariant.feature_enabled ? 200 : 304); + .set('Authorization', devTokenSecret) + .set('if-none-match', '"76d8bb0e:12"') + .expect(enabled ? 200 : 304); - if (etagVariant.feature_enabled) { - expect(res.headers.etag).toBe(`"76d8bb0e:16:${etagVariant.name}"`); - expect(res.body.meta.etag).toBe( - `"76d8bb0e:16:${etagVariant.name}"`, - ); + if (enabled) { + expect(res.headers.etag).toBe(`"76d8bb0e:12:${name}"`); + expect(res.body.meta.etag).toBe(`"76d8bb0e:12:${name}"`); } }); - test('returns 200 when content updates and hash does not match anymore', async () => { - await app.services.featureToggleService.createFeatureToggle( - 'default', - { - name: `featureNew304${apendix}`, - description: 'the #1 feature', - }, - TEST_AUDIT_USER, - ); + test('creating a new feature does not modify etag', async () => { + await app.createFeature('new'); await app.services.configurationRevisionService.updateMaxRevisionId(); - const res = await app.request + await app.request .get('/api/client/features') - .set('if-none-match', 'ae443048:16') - .expect(200); + .set('Authorization', devTokenSecret) + .set('if-none-match', `"76d8bb0e:12${enabled ? `:${name}` : ''}"`) + .expect(304); + }); - if (etagVariant.feature_enabled) { - expect(res.headers.etag).toBe(`"76d8bb0e:16:${etagVariant.name}"`); - expect(res.body.meta.etag).toBe( - `"76d8bb0e:16:${etagVariant.name}"`, - ); + test('production environment gets a different etag than development', async () => { + const { headers: prodHeaders } = await app.request + .get('/api/client/features?bla=1') + .set('Authorization', prodTokenSecret) + .expect(200); + if (enabled) { + expect(prodHeaders.etag).toEqual('"67e24428:14:v2"'); } else { - expect(res.headers.etag).toBe('"76d8bb0e:16"'); - expect(res.body.meta.etag).toBe('"76d8bb0e:16"'); + expect(prodHeaders.etag).toEqual('"67e24428:14"'); + } + + const { headers: devHeaders } = await app.request + .get('/api/client/features') + .set('Authorization', devTokenSecret) + .expect(200); + if (enabled) { + expect(devHeaders.etag).toEqual('"76d8bb0e:12:v2"'); + } else { + expect(devHeaders.etag).toEqual('"76d8bb0e:12"'); } }); -}); -// running after all inside describe block, causes some of the queries to fail to acquire a connection -// this workaround is to run the afterAll outside the describe block -afterAll(async () => { - await Promise.all(shutdownHooks.map((hook) => hook())); + test('modifying dev environment should only invalidate dev tokens', async () => { + const currentDevEtag = `"76d8bb0e:12${enabled ? `:${name}` : ''}"`; + const currentProdEtag = `"67e24428:14${enabled ? `:${name}` : ''}"`; + await app.request + .get('/api/client/features') + .set('if-none-match', currentProdEtag) + .set('Authorization', prodTokenSecret) + .expect(304); + + await app.request + .get('/api/client/features') + .set('Authorization', devTokenSecret) + .set('if-none-match', currentDevEtag) + .expect(304); + + await app.enableFeature('X', DEFAULT_ENV); + await app.services.configurationRevisionService.updateMaxRevisionId(); + + await app.request + .get('/api/client/features') + .set('Authorization', prodTokenSecret) + .set('if-none-match', currentProdEtag) + .expect(304); + + const { headers: devHeaders } = await app.request + .get('/api/client/features') + .set('Authorization', devTokenSecret) + .set('if-none-match', currentDevEtag) + .expect(200); + + // Note: this test yields a different result if run in isolation + // this is because the id 18 depends on a previous test adding a feature + // otherwise the id will be 17 + expect(devHeaders.etag).toEqual( + `"76d8bb0e:18${enabled ? `:${name}` : ''}"`, + ); + }); });