Fixed 304 tests

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,

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

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

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,

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 &&

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 = [
-
-const shutdownHooks: (() => Promise<void>)[] = [];
-
-describe.each([
     {
-        name: 'disabled',
+        tokenName: `client-dev-token`,
-        enabled: false,
+        permissions: [CLIENT],
-        feature_enabled: false,
+        projects: ['*'],
+        environment: 'development',
+        type: ApiTokenType.CLIENT,
+        secret: '*:development.client',
     },
     {
-        name: 'v2',
+        tokenName: `client-prod-token`,
-        enabled: true,
+        permissions: [CLIENT],
-        feature_enabled: true,
+        projects: ['*'],
+        environment: 'production',
+        type: ApiTokenType.CLIENT,
+        secret: '*:production.client',
     },
-])('feature 304 api client (etag variant = %s)', (etagVariant) => {
+];
-    let app: IUnleashTest;
+const devTokenSecret = validTokens[0].secret;
-    let db: ITestDb;
+const prodTokenSecret = validTokens[1].secret;
-    const apendix = etagVariant.feature_enabled
+async function setup({
-        ? `${etagVariant.name}`
+    etagVariantName,
-        : 'etag_variant_off';
+    enabled,
-    beforeAll(async () => {
+}: {
-        db = await dbInit(`feature_304_api_client_${apendix}`, getLogger);
+    etagVariantName: string;
-        app = await setupAppWithCustomConfig(
+    enabled: boolean;
+}): Promise<{ app: IUnleashTest; db: ITestDb }> {
+    const db = await dbInit(`ignored`, getLogger);
+
+    // 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: etagVariant,
+                    etagVariant: {
+                        name: etagVariantName,
+                        enabled,
+                        feature_enabled: enabled,
+                    },
                 },
             },
         },
         db.rawDatabase,
     );
 
-        await app.services.featureToggleService.createFeatureToggle(
+    return { app, db };
-            '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(
+async function initialize({ app, db }: { app: IUnleashTest; db: ITestDb }) {
-            `featureArchivedX${apendix}`,
+    const allEnvs = await app.services.environmentService.getAll();
-            testUser,
+    const nonDefaultEnv = allEnvs.find((env) => env.name !== DEFAULT_ENV)!.name;
-            TEST_AUDIT_USER,
-        );
 
-        await app.services.featureToggleService.createFeatureToggle(
+    await app.createFeature('X');
-            'default',
+    await app.createFeature('Y');
-            {
+    await app.archiveFeature('Y');
-                name: `featureArchivedY${apendix}`,
+    await app.createFeature('Z');
-                description: 'soon to be the #1 feature',
+    await app.enableFeature('Z', DEFAULT_ENV);
-            },
+    await app.enableFeature('Z', nonDefaultEnv);
-            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,
+        createdBy: 'some@example.com',
-            createdByUserId: testUser.id,
+        createdByUserId: 123,
         ip: '127.0.0.1',
-            featureName: `ch-on-feature-${apendix}`,
+        featureName: `X`,
     });
 
-        shutdownHooks.push(async () => {
+    /**
+     * This helps reason about the etag, which is formed by <query-hash>:<event-id>
+     * 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,
+    },
+    {
+        name: 'v2',
+        enabled: true,
+    },
+])('feature 304 api client (etag variant = $name)', ({ name, enabled }) => {
+    let app: IUnleashTest;
+    let db: ITestDb;
+    beforeAll(async () => {
+        ({ app, db } = await setup({
+            etagVariantName: name,
+            enabled,
+        }));
+        await initialize({ app, db });
+    });
+
+    afterAll(async () => {
         await app.destroy();
         await db.destroy();
     });
-    });
 
-    test('returns calculated hash', async () => {
+    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) {
+        if (enabled) {
-            expect(res.headers.etag).toBe(`"76d8bb0e:16:${etagVariant.name}"`);
+            expect(res.headers.etag).toBe(`"76d8bb0e:12:${name}"`);
-            expect(res.body.meta.etag).toBe(
+            expect(res.body.meta.etag).toBe(`"76d8bb0e:12:${name}"`);
-                `"76d8bb0e:16:${etagVariant.name}"`,
-            );
         } else {
-            expect(res.headers.etag).toBe('"76d8bb0e:16"');
+            expect(res.headers.etag).toBe('"76d8bb0e:12"');
-            expect(res.body.meta.etag).toBe('"76d8bb0e:16"');
+            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"')
+            .set('Authorization', devTokenSecret)
-            .expect(etagVariant.feature_enabled ? 200 : 304);
+            .set('if-none-match', '"76d8bb0e:12"')
+            .expect(enabled ? 200 : 304);
 
-        if (etagVariant.feature_enabled) {
+        if (enabled) {
-            expect(res.headers.etag).toBe(`"76d8bb0e:16:${etagVariant.name}"`);
+            expect(res.headers.etag).toBe(`"76d8bb0e:12:${name}"`);
-            expect(res.body.meta.etag).toBe(
+            expect(res.body.meta.etag).toBe(`"76d8bb0e:12:${name}"`);
-                `"76d8bb0e:16:${etagVariant.name}"`,
-            );
         }
     });
 
-    test('returns 200 when content updates and hash does not match anymore', async () => {
+    test('creating a new feature does not modify etag', async () => {
-        await app.services.featureToggleService.createFeatureToggle(
+        await app.createFeature('new');
-            'default',
-            {
-                name: `featureNew304${apendix}`,
-                description: 'the #1 feature',
-            },
-            TEST_AUDIT_USER,
-        );
         await app.services.configurationRevisionService.updateMaxRevisionId();
 
-        const res = await app.request
+        await app.request
             .get('/api/client/features')
-            .set('if-none-match', 'ae443048:16')
+            .set('Authorization', devTokenSecret)
-            .expect(200);
+            .set('if-none-match', `"76d8bb0e:12${enabled ? `:${name}` : ''}"`)
+            .expect(304);
+    });
 
-        if (etagVariant.feature_enabled) {
+    test('production environment gets a different etag than development', async () => {
-            expect(res.headers.etag).toBe(`"76d8bb0e:16:${etagVariant.name}"`);
+        const { headers: prodHeaders } = await app.request
-            expect(res.body.meta.etag).toBe(
+            .get('/api/client/features?bla=1')
-                `"76d8bb0e:16:${etagVariant.name}"`,
+            .set('Authorization', prodTokenSecret)
-            );
+            .expect(200);
+        if (enabled) {
+            expect(prodHeaders.etag).toEqual('"67e24428:14:v2"');
         } else {
-            expect(res.headers.etag).toBe('"76d8bb0e:16"');
+            expect(prodHeaders.etag).toEqual('"67e24428:14"');
-            expect(res.body.meta.etag).toBe('"76d8bb0e:16"');
+        }
+
+        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
+    test('modifying dev environment should only invalidate dev tokens', async () => {
-// this workaround is to run the afterAll outside the describe block
+        const currentDevEtag = `"76d8bb0e:12${enabled ? `:${name}` : ''}"`;
-afterAll(async () => {
+        const currentProdEtag = `"67e24428:14${enabled ? `:${name}` : ''}"`;
-    await Promise.all(shutdownHooks.map((hook) => hook()));
+        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}` : ''}"`,
+        );
+    });
 });