From 09c87c755facbf645a29d913bc84e4867b8ac6cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivar=20Conradi=20=C3=98sthus?= Date: Wed, 14 Dec 2022 17:35:22 +0100 Subject: [PATCH] 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 --- .../src/component/admin/cors/CorsForm.tsx | 18 ++- .../component/admin/cors/CorsHelpAlert.tsx | 9 ++ .../middleware/cors-origin-middleware.test.ts | 136 +++++++++++++----- src/lib/middleware/cors-origin-middleware.ts | 26 ++-- src/lib/routes/admin-api/config.ts | 10 +- src/lib/server-impl.ts | 1 + src/lib/services/index.ts | 1 + src/lib/services/proxy-service.ts | 67 ++++++++- src/lib/services/setting-service.ts | 38 +---- src/test/e2e/api/admin/config.e2e.test.ts | 2 +- src/test/e2e/helpers/test-helper.ts | 1 + 11 files changed, 218 insertions(+), 91 deletions(-) diff --git a/frontend/src/component/admin/cors/CorsForm.tsx b/frontend/src/component/admin/cors/CorsForm.tsx index cc9b124eaf..eebea9671f 100644 --- 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) => { + + + https://www.example.com +
+ https://www.example2.com + + { An asterisk (*) may be used to allow API calls from any origin.

+
+

+ 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). +

); }; diff --git a/src/lib/middleware/cors-origin-middleware.test.ts b/src/lib/middleware/cors-origin-middleware.test.ts index 2696c01bc8..95206b4f87 100644 --- 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), + }; + + 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(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((resolve) => + process.nextTick(async () => { + const settings = await proxyService.getFrontendSettings(); + + expect(settings).toEqual({ + frontendApiOrigins: ['*'], + }); + resolve(); + }), + ); + proxyService.destroy(); }); diff --git a/src/lib/middleware/cors-origin-middleware.ts b/src/lib/middleware/cors-origin-middleware.ts index 2fc971cdf9..6645c93e8b 100644 --- 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, + { proxyService }: Pick, 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); diff --git a/src/lib/routes/admin-api/config.ts b/src/lib/routes/admin-api/config.ts index 3604909b59..cf38d9098c 100644 --- 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, ): Promise { const [frontendSettings, simpleAuthSettings] = await Promise.all([ - this.settingService.getFrontendSettings(), + this.proxyService.getFrontendSettings(false), this.settingService.get(simpleAuthSettingsKey), ]); @@ -133,7 +139,7 @@ class ConfigController extends Controller { res: Response, ): Promise { if (req.body.frontendSettings) { - await this.settingService.setFrontendSettings( + await this.proxyService.setFrontendSettings( req.body.frontendSettings, extractUsername(req), ); diff --git a/src/lib/server-impl.ts b/src/lib/server-impl.ts index d44fcd6c3d..8815984d9b 100644 --- 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(); }; diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index f0a310ef53..ddfd38201a 100644 --- 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); diff --git a/src/lib/services/proxy-service.ts b/src/lib/services/proxy-service.ts index c858c2790e..9041846cde 100644 --- 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; +type Config = Pick< + IUnleashConfig, + 'getLogger' | 'frontendApi' | 'frontendApiOrigins' +>; type Stores = Pick; 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 = 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 { + const error = validateOrigins(value.frontendApiOrigins); + if (error) { + throw new BadDataError(error); + } + await this.services.settingService.insert( + frontendSettingsKey, + value, + createdBy, + ); + } + + private async fetchFrontendSettings(): Promise { + this.cachedFrontendSettings = await this.services.settingService.get( + frontendSettingsKey, + { + frontendApiOrigins: this.config.frontendApiOrigins, + }, + ); + return this.cachedFrontendSettings; + } + + async getFrontendSettings( + useCache: boolean = true, + ): Promise { + if (useCache && this.cachedFrontendSettings) { + return this.cachedFrontendSettings; + } + return this.fetchFrontendSettings(); + } + + destroy(): void { + clearInterval(this.timer); + this.timer = null; + } } diff --git a/src/lib/services/setting-service.ts b/src/lib/services/setting-service.ts index 77ad7c54a2..0f2f64703b 100644 --- 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(); - constructor( { settingStore, @@ -42,14 +32,11 @@ export default class SettingService { } async get(id: string, defaultValue?: T): Promise { - 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 { - 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 { - 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 { - this.cache.clear(); await this.settingStore.deleteAll(); } - - async setFrontendSettings( - value: FrontendSettings, - createdBy: string, - ): Promise { - const error = validateOrigins(value.frontendApiOrigins); - if (error) { - throw new BadDataError(error); - } - await this.insert(frontendSettingsKey, value, createdBy); - } - - async getFrontendSettings(): Promise { - return this.get(frontendSettingsKey, { - frontendApiOrigins: this.config.frontendApiOrigins, - }); - } } - -module.exports = SettingService; diff --git a/src/test/e2e/api/admin/config.e2e.test.ts b/src/test/e2e/api/admin/config.e2e.test.ts index 4bf36e8812..c6e30be67e 100644 --- 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(), ); diff --git a/src/test/e2e/helpers/test-helper.ts b/src/test/e2e/helpers/test-helper.ts index 172be1cfe4..ad265d0db0 100644 --- 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?