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>

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}

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

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),
+    };
+
+    return {
+        //@ts-ignore
+        proxyService: new ProxyService(config, stores, services),
+        settingStore: stores.settingStore,
+    };
 };
 
-test('allowRequestOrigin', () => {
+test('resolveOrigin', () => {
     const dotCom = 'https://example.com';
     const dotOrg = 'https://example.org';
 
-    expect(allowRequestOrigin('', [])).toEqual(false);
+    expect(resolveOrigin([])).toEqual('*');
-    expect(allowRequestOrigin(dotCom, [])).toEqual(false);
+    expect(resolveOrigin(['*'])).toEqual('*');
-    expect(allowRequestOrigin(dotCom, [dotOrg])).toEqual(false);
+    expect(resolveOrigin([dotOrg])).toEqual([dotOrg]);
-
+    expect(resolveOrigin([dotCom, dotOrg])).toEqual([dotCom, dotOrg]);
-    expect(allowRequestOrigin(dotCom, [dotCom, dotOrg])).toEqual(true);
+    expect(resolveOrigin([dotOrg, '*'])).toEqual('*');
-    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);
 });
 
 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);
+    await proxyService.setFrontendSettings(
-    expect(await service.getFrontendSettings()).toEqual({
+        { frontendApiOrigins: [] },
+        userName,
+    );
+    expect(await proxyService.getFrontendSettings(false)).toEqual({
         frontendApiOrigins: [],
     });
-    await service.setFrontendSettings({ frontendApiOrigins: ['*'] }, userName);
+    await proxyService.setFrontendSettings(
-    expect(await service.getFrontendSettings()).toEqual({
+        { frontendApiOrigins: ['*'] },
+        userName,
+    );
+    expect(await proxyService.getFrontendSettings(false)).toEqual({
         frontendApiOrigins: ['*'],
     });
-    await service.delete(frontendSettingsKey, userName);
+    await settingStore.delete(frontendSettingsKey);
-    expect(await service.getFrontendSettings()).toEqual({
+    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);
+    await proxyService.setFrontendSettings(
-    expect(await service.getFrontendSettings()).toEqual({
+        { 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);
+    await settingStore.delete(frontendSettingsKey);
-    expect(await service.getFrontendSettings()).toEqual({
+    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();
 });

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 = (
+export const resolveOrigin = (allowedOrigins: string[]): string | string[] => {
-    requestOrigin: string,
+    if (allowedOrigins.length === 0) {
-    allowedOrigins: string[],
+        return '*';
-): boolean => {
+    }
-    return allowedOrigins.some((allowedOrigin) => {
+    if (allowedOrigins.some((origin: string) => origin === '*')) {
-        return allowedOrigin === requestOrigin || allowedOrigin === '*';
+        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(
+                origin: resolveOrigin(frontendApiOrigins),
-                    req.header('Origin'),
-                    frontendApiOrigins,
-                ),
                 maxAge: config.accessControlMaxAge,
                 exposedHeaders: 'ETag',
+                credentials: true,
             });
         } catch (error) {
             callback(error);

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

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

src/lib/services/index.ts

@@ -109,6 +109,7 @@ export const createServices = (
         featureToggleServiceV2,
         clientMetricsServiceV2,
         segmentService,
+        settingService,
     });
 
     const edgeService = new EdgeService(stores, config);

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

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)) {
+        const value = await this.settingStore.get(id);
-            this.cache.set(id, await this.settingStore.get(id));
+        return value || defaultValue;
-        }
-        return (this.cache.get(id) as T) || 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;

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

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?