Fix/remove settings cache (#2694)

In this PR we remove the general SettingService cache, as it will not work across multiple horizontal unleash instances, events are not published across. We also fix the CORS origin to: - Access-Control-Allow-Origin set to "*" if no Origin is configured - Access-Control-Allow-Origin set to "*" if any Origin is configured to "*" - - Access-Control-Allow-Origin set to array and have the "cors" middleware to return an exact match on the user provided Origin. Co-authored-by: Fredrik Oseberg <fredrik.no@gmail.com>
2025-01-20 00:08:02 +01:00 · 2022-12-14 17:35:22 +01:00 · 2022-12-14 17:35:22 +01:00 · 09c87c755f
commit 09c87c755f
parent db7b39af2d
11 changed files with 218 additions and 91 deletions
--- a/frontend/src/component/admin/cors/CorsForm.tsx
+++ b/frontend/src/component/admin/cors/CorsForm.tsx
@ -6,6 +6,7 @@ import { useUiConfigApi } from 'hooks/api/actions/useUiConfigApi/useUiConfigApi'
 import useToast from 'hooks/useToast';
 import { formatUnknownError } from 'utils/formatUnknownError';
 import { useId } from 'hooks/useId';
+import { fontSize } from '@mui/system';

 interface ICorsFormProps {
    frontendApiOrigins: string[] | undefined;
@ -35,8 +36,23 @@ export const CorsForm = ({ frontendApiOrigins }: ICorsFormProps) => {
            <Box sx={{ display: 'grid', gap: 1 }}>
                <label htmlFor={inputFieldId}>
                    Which origins should be allowed to call the Frontend API?
-                    Add only one origin per line.
+                    Add only one origin per line. The CORS specification does
+                    not support wildcard for subdomains, it needs to be a fully
+                    qualified domain, including the protocol.
+                    <br />
+                    <br />
+                    If you specify "*" it will be the chosen origin.
+                    <br />
+                    <br />
+                    Example:
                </label>
+
+                <code style={{ fontSize: '0.7em' }}>
+                    https://www.example.com
+                    <br />
+                    https://www.example2.com
+                </code>
+
                <TextField
                    id={inputFieldId}
                    aria-describedby={helpTextId}
--- a/frontend/src/component/admin/cors/CorsHelpAlert.tsx
+++ b/frontend/src/component/admin/cors/CorsHelpAlert.tsx
@ -17,6 +17,15 @@ export const CorsHelpAlert = () => {
                An asterisk (<code>*</code>) may be used to allow API calls from
                any origin.
            </p>
+            <br />
+            <p>
+                Be aware that changes here will take up to two minutes to be
+                updated. In addition, there is a maxAge on the
+                Access-Control-Allow-Origin header that will instruct browsers
+                to cache this header for some time. The cache period is set to
+                the maxium that the browser allows (2h for Chrome, 24h for
+                Firefox).
+            </p>
        </Alert>
    );
 };
--- a/src/lib/middleware/cors-origin-middleware.test.ts
+++ b/src/lib/middleware/cors-origin-middleware.test.ts
@ -1,86 +1,152 @@
-import { allowRequestOrigin } from './cors-origin-middleware';
+import { resolveOrigin } from './cors-origin-middleware';
 import FakeSettingStore from '../../test/fixtures/fake-setting-store';
-import SettingService from '../services/setting-service';
 import { createTestConfig } from '../../test/config/test-config';
 import FakeEventStore from '../../test/fixtures/fake-event-store';
 import { randomId } from '../util/random-id';
-import { frontendSettingsKey } from '../types/settings/frontend-settings';
+import FakeProjectStore from '../../test/fixtures/fake-project-store';
+import { ProxyService, SettingService } from '../../lib/services';
+import { ISettingStore } from '../../lib/types';
+import { frontendSettingsKey } from '../../lib/types/settings/frontend-settings';
+import { minutesToMilliseconds } from 'date-fns';

-const createSettingService = (frontendApiOrigins: string[]): SettingService => {
+const createSettingService = (
+    frontendApiOrigins: string[],
+): { proxyService: ProxyService; settingStore: ISettingStore } => {
    const config = createTestConfig({ frontendApiOrigins });

    const stores = {
        settingStore: new FakeSettingStore(),
        eventStore: new FakeEventStore(),
+        projectStore: new FakeProjectStore(),
    };

-    return new SettingService(stores, config);
+    const services = {
+        settingService: new SettingService(stores, config),
    };

-test('allowRequestOrigin', () => {
+    return {
+        //@ts-ignore
+        proxyService: new ProxyService(config, stores, services),
+        settingStore: stores.settingStore,
+    };
+};
+
+test('resolveOrigin', () => {
    const dotCom = 'https://example.com';
    const dotOrg = 'https://example.org';

-    expect(allowRequestOrigin('', [])).toEqual(false);
-    expect(allowRequestOrigin(dotCom, [])).toEqual(false);
-    expect(allowRequestOrigin(dotCom, [dotOrg])).toEqual(false);
-
-    expect(allowRequestOrigin(dotCom, [dotCom, dotOrg])).toEqual(true);
-    expect(allowRequestOrigin(dotCom, [dotOrg, dotCom])).toEqual(true);
-    expect(allowRequestOrigin(dotCom, [dotCom, dotCom])).toEqual(true);
-
-    expect(allowRequestOrigin(dotCom, ['*'])).toEqual(true);
-    expect(allowRequestOrigin(dotCom, [dotOrg, '*'])).toEqual(true);
-    expect(allowRequestOrigin(dotCom, [dotCom, dotOrg, '*'])).toEqual(true);
+    expect(resolveOrigin([])).toEqual('*');
+    expect(resolveOrigin(['*'])).toEqual('*');
+    expect(resolveOrigin([dotOrg])).toEqual([dotOrg]);
+    expect(resolveOrigin([dotCom, dotOrg])).toEqual([dotCom, dotOrg]);
+    expect(resolveOrigin([dotOrg, '*'])).toEqual('*');
 });

 test('corsOriginMiddleware origin validation', async () => {
-    const service = createSettingService([]);
+    const { proxyService } = createSettingService([]);
    const userName = randomId();
    await expect(() =>
-        service.setFrontendSettings({ frontendApiOrigins: ['a'] }, userName),
+        proxyService.setFrontendSettings(
+            { frontendApiOrigins: ['a'] },
+            userName,
+        ),
    ).rejects.toThrow('Invalid origin: a');
+    proxyService.destroy();
 });

 test('corsOriginMiddleware without config', async () => {
-    const service = createSettingService([]);
+    const { proxyService, settingStore } = createSettingService([]);
    const userName = randomId();
-    expect(await service.getFrontendSettings()).toEqual({
+    expect(await proxyService.getFrontendSettings(false)).toEqual({
        frontendApiOrigins: [],
    });
-    await service.setFrontendSettings({ frontendApiOrigins: [] }, userName);
-    expect(await service.getFrontendSettings()).toEqual({
+    await proxyService.setFrontendSettings(
+        { frontendApiOrigins: [] },
+        userName,
+    );
+    expect(await proxyService.getFrontendSettings(false)).toEqual({
        frontendApiOrigins: [],
    });
-    await service.setFrontendSettings({ frontendApiOrigins: ['*'] }, userName);
-    expect(await service.getFrontendSettings()).toEqual({
+    await proxyService.setFrontendSettings(
+        { frontendApiOrigins: ['*'] },
+        userName,
+    );
+    expect(await proxyService.getFrontendSettings(false)).toEqual({
        frontendApiOrigins: ['*'],
    });
-    await service.delete(frontendSettingsKey, userName);
-    expect(await service.getFrontendSettings()).toEqual({
+    await settingStore.delete(frontendSettingsKey);
+    expect(await proxyService.getFrontendSettings(false)).toEqual({
        frontendApiOrigins: [],
    });
+    proxyService.destroy();
 });

 test('corsOriginMiddleware with config', async () => {
-    const service = createSettingService(['*']);
+    const { proxyService, settingStore } = createSettingService(['*']);
    const userName = randomId();
-    expect(await service.getFrontendSettings()).toEqual({
+    expect(await proxyService.getFrontendSettings(false)).toEqual({
        frontendApiOrigins: ['*'],
    });
-    await service.setFrontendSettings({ frontendApiOrigins: [] }, userName);
-    expect(await service.getFrontendSettings()).toEqual({
+    await proxyService.setFrontendSettings(
+        { frontendApiOrigins: [] },
+        userName,
+    );
+    expect(await proxyService.getFrontendSettings(false)).toEqual({
        frontendApiOrigins: [],
    });
-    await service.setFrontendSettings(
+    await proxyService.setFrontendSettings(
        { frontendApiOrigins: ['https://example.com', 'https://example.org'] },
        userName,
    );
-    expect(await service.getFrontendSettings()).toEqual({
+    expect(await proxyService.getFrontendSettings(false)).toEqual({
        frontendApiOrigins: ['https://example.com', 'https://example.org'],
    });
-    await service.delete(frontendSettingsKey, userName);
-    expect(await service.getFrontendSettings()).toEqual({
+    await settingStore.delete(frontendSettingsKey);
+    expect(await proxyService.getFrontendSettings(false)).toEqual({
        frontendApiOrigins: ['*'],
    });
+    proxyService.destroy();
+});
+
+test('corsOriginMiddleware with caching enabled', async () => {
+    jest.useFakeTimers();
+
+    const { proxyService } = createSettingService([]);
+
+    const userName = randomId();
+    expect(await proxyService.getFrontendSettings()).toEqual({
+        frontendApiOrigins: [],
+    });
+
+    //setting
+    await proxyService.setFrontendSettings(
+        { frontendApiOrigins: ['*'] },
+        userName,
+    );
+
+    //still get cached value
+    expect(await proxyService.getFrontendSettings()).toEqual({
+        frontendApiOrigins: [],
+    });
+
+    jest.advanceTimersByTime(minutesToMilliseconds(2));
+
+    jest.useRealTimers();
+
+    /*
+    This is needed because it is not enough to fake time to test the
+    updated cache, we also need to make sure that all promises are 
+    executed and completed, in the right order. 
+    */
+    await new Promise<void>((resolve) =>
+        process.nextTick(async () => {
+            const settings = await proxyService.getFrontendSettings();
+
+            expect(settings).toEqual({
+                frontendApiOrigins: ['*'],
+            });
+            resolve();
+        }),
+    );
+    proxyService.destroy();
 });
--- a/src/lib/middleware/cors-origin-middleware.ts
+++ b/src/lib/middleware/cors-origin-middleware.ts
@ -2,32 +2,32 @@ import { RequestHandler } from 'express';
 import cors from 'cors';
 import { IUnleashConfig, IUnleashServices } from '../types';

-export const allowRequestOrigin = (
-    requestOrigin: string,
-    allowedOrigins: string[],
-): boolean => {
-    return allowedOrigins.some((allowedOrigin) => {
-        return allowedOrigin === requestOrigin || allowedOrigin === '*';
-    });
+export const resolveOrigin = (allowedOrigins: string[]): string | string[] => {
+    if (allowedOrigins.length === 0) {
+        return '*';
+    }
+    if (allowedOrigins.some((origin: string) => origin === '*')) {
+        return '*';
+    } else {
+        return allowedOrigins;
+    }
 };

 // Check the request's Origin header against a list of allowed origins.
 // The list may include '*', which `cors` does not support natively.
 export const corsOriginMiddleware = (
-    { settingService }: Pick<IUnleashServices, 'settingService'>,
+    { proxyService }: Pick<IUnleashServices, 'proxyService'>,
    config: IUnleashConfig,
 ): RequestHandler => {
    return cors(async (req, callback) => {
        try {
            const { frontendApiOrigins = [] } =
-                await settingService.getFrontendSettings();
+                await proxyService.getFrontendSettings();
            callback(null, {
-                origin: allowRequestOrigin(
-                    req.header('Origin'),
-                    frontendApiOrigins,
-                ),
+                origin: resolveOrigin(frontendApiOrigins),
                maxAge: config.accessControlMaxAge,
                exposedHeaders: 'ETag',
+                credentials: true,
            });
        } catch (error) {
            callback(error);
--- a/src/lib/routes/admin-api/config.ts
+++ b/src/lib/routes/admin-api/config.ts
@ -24,12 +24,15 @@ import { extractUsername } from '../../util/extract-user';
 import NotFoundError from '../../error/notfound-error';
 import { SetUiConfigSchema } from '../../openapi/spec/set-ui-config-schema';
 import { createRequestSchema } from '../../openapi/util/create-request-schema';
+import { ProxyService } from 'lib/services';

 class ConfigController extends Controller {
    private versionService: VersionService;

    private settingService: SettingService;

+    private proxyService: ProxyService;
+
    private emailService: EmailService;

    private readonly openApiService: OpenApiService;
@ -41,12 +44,14 @@ class ConfigController extends Controller {
            settingService,
            emailService,
            openApiService,
+            proxyService,
        }: Pick<
            IUnleashServices,
            | 'versionService'
            | 'settingService'
            | 'emailService'
            | 'openApiService'
+            | 'proxyService'
        >,
    ) {
        super(config);
@ -54,6 +59,7 @@ class ConfigController extends Controller {
        this.settingService = settingService;
        this.emailService = emailService;
        this.openApiService = openApiService;
+        this.proxyService = proxyService;

        this.route({
            method: 'get',
@ -92,7 +98,7 @@ class ConfigController extends Controller {
        res: Response<UiConfigSchema>,
    ): Promise<void> {
        const [frontendSettings, simpleAuthSettings] = await Promise.all([
-            this.settingService.getFrontendSettings(),
+            this.proxyService.getFrontendSettings(false),
            this.settingService.get<SimpleAuthSettings>(simpleAuthSettingsKey),
        ]);

@ -133,7 +139,7 @@ class ConfigController extends Controller {
        res: Response<string>,
    ): Promise<void> {
        if (req.body.frontendSettings) {
-            await this.settingService.setFrontendSettings(
+            await this.proxyService.setFrontendSettings(
                req.body.frontendSettings,
                extractUsername(req),
            );
--- a/src/lib/server-impl.ts
+++ b/src/lib/server-impl.ts
@ -55,6 +55,7 @@ async function createApp(
        metricsMonitor.stopMonitoring();
        stores.clientInstanceStore.destroy();
        services.clientMetricsServiceV2.destroy();
+        services.proxyService.destroy();
        await db.destroy();
    };

--- a/src/lib/services/index.ts
+++ b/src/lib/services/index.ts
@ -109,6 +109,7 @@ export const createServices = (
        featureToggleServiceV2,
        clientMetricsServiceV2,
        segmentService,
+        settingService,
    });

    const edgeService = new EdgeService(stores, config);
--- a/src/lib/services/proxy-service.ts
+++ b/src/lib/services/proxy-service.ts
@ -10,16 +10,29 @@ import {
    UnleashEvents,
 } from 'unleash-client';
 import { ProxyRepository } from '../proxy';
-import assert from 'assert';
 import { ApiTokenType } from '../types/models/api-token';
+import {
+    FrontendSettings,
+    frontendSettingsKey,
+} from '../types/settings/frontend-settings';
+import { validateOrigins } from '../util';
+import { BadDataError } from '../error';
+import assert from 'assert';
+import { minutesToMilliseconds } from 'date-fns';

-type Config = Pick<IUnleashConfig, 'getLogger' | 'frontendApi'>;
+type Config = Pick<
+    IUnleashConfig,
+    'getLogger' | 'frontendApi' | 'frontendApiOrigins'
+>;

 type Stores = Pick<IUnleashStores, 'projectStore' | 'eventStore'>;

 type Services = Pick<
    IUnleashServices,
-    'featureToggleServiceV2' | 'segmentService' | 'clientMetricsServiceV2'
+    | 'featureToggleServiceV2'
+    | 'segmentService'
+    | 'clientMetricsServiceV2'
+    | 'settingService'
 >;

 export class ProxyService {
@ -33,11 +46,20 @@ export class ProxyService {

    private readonly clients: Map<ApiUser['secret'], Unleash> = new Map();

+    private cachedFrontendSettings?: FrontendSettings;
+
+    private timer: NodeJS.Timeout;
+
    constructor(config: Config, stores: Stores, services: Services) {
        this.config = config;
        this.logger = config.getLogger('services/proxy-service.ts');
        this.stores = stores;
        this.services = services;
+
+        this.timer = setInterval(
+            () => this.fetchFrontendSettings(),
+            minutesToMilliseconds(2),
+        ).unref();
    }

    async getProxyFeatures(
@ -138,4 +160,43 @@ export class ProxyService {
    private static assertExpectedTokenType({ type }: ApiUser) {
        assert(type === ApiTokenType.FRONTEND || type === ApiTokenType.ADMIN);
    }
+
+    async setFrontendSettings(
+        value: FrontendSettings,
+        createdBy: string,
+    ): Promise<void> {
+        const error = validateOrigins(value.frontendApiOrigins);
+        if (error) {
+            throw new BadDataError(error);
+        }
+        await this.services.settingService.insert(
+            frontendSettingsKey,
+            value,
+            createdBy,
+        );
+    }
+
+    private async fetchFrontendSettings(): Promise<FrontendSettings> {
+        this.cachedFrontendSettings = await this.services.settingService.get(
+            frontendSettingsKey,
+            {
+                frontendApiOrigins: this.config.frontendApiOrigins,
+            },
+        );
+        return this.cachedFrontendSettings;
+    }
+
+    async getFrontendSettings(
+        useCache: boolean = true,
+    ): Promise<FrontendSettings> {
+        if (useCache && this.cachedFrontendSettings) {
+            return this.cachedFrontendSettings;
+        }
+        return this.fetchFrontendSettings();
+    }
+
+    destroy(): void {
+        clearInterval(this.timer);
+        this.timer = null;
+    }
 }
--- a/src/lib/services/setting-service.ts
+++ b/src/lib/services/setting-service.ts
@ -8,12 +8,6 @@ import {
    SettingDeletedEvent,
    SettingUpdatedEvent,
 } from '../types/events';
-import { validateOrigins } from '../util/validateOrigin';
-import {
-    FrontendSettings,
-    frontendSettingsKey,
-} from '../types/settings/frontend-settings';
-import BadDataError from '../error/bad-data-error';

 export default class SettingService {
    private config: IUnleashConfig;
@ -24,10 +18,6 @@ export default class SettingService {

    private eventStore: IEventStore;

-    // SettingService.getFrontendSettings is called on every request to the
-    // frontend API. Keep fetched settings in a cache for fewer DB queries.
-    private cache = new Map<string, unknown>();
-
    constructor(
        {
            settingStore,
@ -42,14 +32,11 @@ export default class SettingService {
    }

    async get<T>(id: string, defaultValue?: T): Promise<T> {
-        if (!this.cache.has(id)) {
-            this.cache.set(id, await this.settingStore.get(id));
-        }
-        return (this.cache.get(id) as T) || defaultValue;
+        const value = await this.settingStore.get(id);
+        return value || defaultValue;
    }

    async insert(id: string, value: object, createdBy: string): Promise<void> {
-        this.cache.delete(id);
        const exists = await this.settingStore.exists(id);
        if (exists) {
            await this.settingStore.updateRow(id, value);
@ -71,7 +58,6 @@ export default class SettingService {
    }

    async delete(id: string, createdBy: string): Promise<void> {
-        this.cache.delete(id);
        await this.settingStore.delete(id);
        await this.eventStore.store(
            new SettingDeletedEvent({
@ -84,26 +70,6 @@ export default class SettingService {
    }

    async deleteAll(): Promise<void> {
-        this.cache.clear();
        await this.settingStore.deleteAll();
    }
-
-    async setFrontendSettings(
-        value: FrontendSettings,
-        createdBy: string,
-    ): Promise<void> {
-        const error = validateOrigins(value.frontendApiOrigins);
-        if (error) {
-            throw new BadDataError(error);
 }
-        await this.insert(frontendSettingsKey, value, createdBy);
-    }
-
-    async getFrontendSettings(): Promise<FrontendSettings> {
-        return this.get(frontendSettingsKey, {
-            frontendApiOrigins: this.config.frontendApiOrigins,
-        });
-    }
-}
-
-module.exports = SettingService;
--- a/src/test/e2e/api/admin/config.e2e.test.ts
+++ b/src/test/e2e/api/admin/config.e2e.test.ts
@ -44,7 +44,7 @@ test('gets ui config with disablePasswordAuth', async () => {

 test('gets ui config with frontendSettings', async () => {
    const frontendApiOrigins = ['https://example.net'];
-    await app.services.settingService.setFrontendSettings(
+    await app.services.proxyService.setFrontendSettings(
        { frontendApiOrigins },
        randomId(),
    );
--- a/src/test/e2e/helpers/test-helper.ts
+++ b/src/test/e2e/helpers/test-helper.ts
@ -47,6 +47,7 @@ async function createApp(
        services.clientInstanceService.destroy();
        services.clientMetricsServiceV2.destroy();
        services.apiTokenService.destroy();
+        services.proxyService.destroy();
    };

    // TODO: use create from server-impl instead?