refactor: Short-Circuit Config Override Resolution (#12553)

This commit is contained in:
Danny Avila 2026-04-07 22:38:08 -04:00
parent 8ed0bcf5ca
commit 15fc27950d
No known key found for this signature in database
GPG key ID: BF31EEB2C5CA0956
3 changed files with 153 additions and 13 deletions

View file

@ -17,7 +17,7 @@ const configMiddleware = async (req, res, next) => {
});
try {
req.config = await getAppConfig();
req.config = await getAppConfig({ tenantId: req.user?.tenantId });
next();
} catch (fallbackError) {
logger.error('Fallback config middleware error:', fallbackError);

View file

@ -1,5 +1,5 @@
import type { AppConfig } from '@librechat/data-schemas';
import { createAppConfigService } from './service';
import { createAppConfigService, _resetOverrideStrictCache } from './service';
/** Extends AppConfig with mock fields used by merge behavior tests. */
interface TestConfig extends AppConfig {
@ -229,6 +229,124 @@ describe('createAppConfigService', () => {
expect((config as TestConfig).x).toBe('admin-only');
});
it('passes empty principals to getApplicableConfigs when buildPrincipals returns empty', async () => {
const deps = createDeps({
getUserPrincipals: jest.fn().mockResolvedValue([]),
});
const { getAppConfig } = createAppConfigService(deps);
const config = await getAppConfig({ userId: 'uid1', role: 'USER' });
expect(deps.getUserPrincipals).toHaveBeenCalledWith({ userId: 'uid1', role: 'USER' });
expect(deps.getApplicableConfigs).toHaveBeenCalledWith([]);
expect(config).toEqual(deps._baseConfig);
});
describe('strict mode (TENANT_ISOLATION_STRICT=true)', () => {
beforeEach(() => {
process.env.TENANT_ISOLATION_STRICT = 'true';
_resetOverrideStrictCache();
});
afterEach(() => {
delete process.env.TENANT_ISOLATION_STRICT;
_resetOverrideStrictCache();
});
it('skips DB query for empty principals without tenantId and does not cache', async () => {
const deps = createDeps();
const { getAppConfig } = createAppConfigService(deps);
const config = await getAppConfig();
expect(deps.getApplicableConfigs).not.toHaveBeenCalled();
expect(config).toEqual(deps._baseConfig);
const setCalls = deps._cache.set.mock.calls.filter(
([key]: [string, unknown]) => key !== '_BASE_',
);
expect(setCalls).toHaveLength(0);
});
it('queries DB when tenantId is present', async () => {
const deps = createDeps();
const { getAppConfig } = createAppConfigService(deps);
await getAppConfig({ tenantId: 'tenant-a' });
expect(deps.getApplicableConfigs).toHaveBeenCalledWith([]);
});
it('warns once when non-empty principals proceed without tenantId', async () => {
const { logger } = jest.requireActual('@librechat/data-schemas');
const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
const deps = createDeps();
const { getAppConfig } = createAppConfigService(deps);
await getAppConfig({ role: 'USER' });
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('No tenantId in strict mode'));
const warnCount = warnSpy.mock.calls.length;
await getAppConfig({ role: 'ADMIN' });
expect(warnSpy).toHaveBeenCalledTimes(warnCount);
warnSpy.mockRestore();
});
it('falls through to getApplicableConfigs when ALS has tenant context despite no tenantId param', async () => {
const { tenantStorage } = jest.requireActual('@librechat/data-schemas');
const deps = createDeps({
getApplicableConfigs: jest
.fn()
.mockResolvedValue([{ priority: 5, overrides: { restricted: true }, isActive: true }]),
});
const { getAppConfig } = createAppConfigService(deps);
const config = await tenantStorage.run({ tenantId: 'tenant-a' }, async () =>
getAppConfig(),
);
expect(deps.getApplicableConfigs).toHaveBeenCalledWith([]);
expect((config as TestConfig).restricted).toBe(true);
});
});
describe('non-strict mode (TENANT_ISOLATION_STRICT unset)', () => {
beforeEach(() => {
delete process.env.TENANT_ISOLATION_STRICT;
_resetOverrideStrictCache();
});
afterEach(() => {
_resetOverrideStrictCache();
});
it('passes empty principals through to getApplicableConfigs', async () => {
const deps = createDeps();
const { getAppConfig } = createAppConfigService(deps);
await getAppConfig();
expect(deps.getApplicableConfigs).toHaveBeenCalledWith([]);
});
});
it('does not cache on buildPrincipals error — retries on next request', async () => {
const deps = createDeps({
getUserPrincipals: jest
.fn()
.mockRejectedValueOnce(new Error('transient'))
.mockResolvedValue([{ principalType: 'role', principalId: 'USER' }]),
});
const { getAppConfig } = createAppConfigService(deps);
const first = await getAppConfig({ userId: 'uid1', role: 'USER' });
expect(first).toEqual(deps._baseConfig);
expect(deps.getApplicableConfigs).not.toHaveBeenCalled();
await getAppConfig({ userId: 'uid1', role: 'USER' });
expect(deps.getUserPrincipals).toHaveBeenCalledTimes(2);
expect(deps.getApplicableConfigs).toHaveBeenCalledTimes(1);
});
it('falls back to base config on getApplicableConfigs error', async () => {
const deps = createDeps({
getApplicableConfigs: jest.fn().mockRejectedValue(new Error('DB down')),

View file

@ -1,11 +1,16 @@
import { PrincipalType } from 'librechat-data-provider';
import { logger, mergeConfigOverrides, BASE_CONFIG_PRINCIPAL_ID } from '@librechat/data-schemas';
import {
logger,
getTenantId,
mergeConfigOverrides,
BASE_CONFIG_PRINCIPAL_ID,
} from '@librechat/data-schemas';
import type { Types } from 'mongoose';
import type { AppConfig, IConfig } from '@librechat/data-schemas';
const BASE_CONFIG_KEY = '_BASE_';
const DEFAULT_OVERRIDE_CACHE_TTL = 60_000;
export const DEFAULT_OVERRIDE_CACHE_TTL = 60_000;
// ── Types ────────────────────────────────────────────────────────────
@ -50,9 +55,9 @@ function isStrictOverrideMode(): boolean {
return (_strictOverride ??= process.env.TENANT_ISOLATION_STRICT === 'true');
}
/** @internal Resets the cached strict-override flag. Exposed for test teardown only. */
let _warnedNoTenantInStrictMode = false;
/** @internal Resets the memoized strict-override flag and one-time no-tenantId warning gate. Exposed for test teardown only. */
export function _resetOverrideStrictCache(): void {
_strictOverride = undefined;
_warnedNoTenantInStrictMode = false;
@ -60,13 +65,6 @@ export function _resetOverrideStrictCache(): void {
function overrideCacheKey(role?: string, userId?: string, tenantId?: string): string {
const tenant = tenantId || '__default__';
if (!tenantId && isStrictOverrideMode() && !_warnedNoTenantInStrictMode) {
_warnedNoTenantInStrictMode = true;
logger.warn(
'[overrideCacheKey] No tenantId in strict mode — falling back to __default__. ' +
'This likely indicates a code path that bypasses the tenant context middleware.',
);
}
if (userId && role) {
return `_OVERRIDE_:${tenant}:${role}:${userId}`;
}
@ -168,8 +166,32 @@ export function createAppConfigService(deps: AppConfigServiceDeps) {
}
}
const principals = await buildPrincipals(role, userId).catch((error: unknown) => {
logger.error('[getAppConfig] Error building principals, falling back to base:', error);
return null;
});
if (principals === null) {
return baseConfig;
}
// Strict-isolation + no tenant (param or ALS) = pathological path (middleware bypass or
// unauthenticated startup). Pre-tenant calls use baseOnly:true; admin calls carry tenantId.
// If ALS has a tenant, Mongoose scopes queries to that tenant's overrides — must fall through.
// Not cached: the cache key doesn't include ALS context, so a cached __default__ entry would
// be served to later ALS-scoped calls that share the same param-derived key.
if (principals.length === 0 && !tenantId && !getTenantId() && isStrictOverrideMode()) {
return baseConfig;
}
if (!tenantId && isStrictOverrideMode() && !_warnedNoTenantInStrictMode) {
_warnedNoTenantInStrictMode = true;
logger.warn(
'[getAppConfig] No tenantId in strict mode — falling back to __default__. ' +
'This likely indicates a code path that bypasses the tenant context middleware.',
);
}
try {
const principals = await buildPrincipals(role, userId);
const configs = await getApplicableConfigs(principals);
if (configs.length === 0) {