mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-04-03 06:17:21 +02:00
⚖️ refactor: Split Config Route into Unauthenticated and Authenticated Paths (#12490)
* refactor: split /api/config into unauthenticated and authenticated response paths
- Replace preAuthTenantMiddleware with optionalJwtAuth on the /api/config
route so the handler can detect whether the request is authenticated
- When unauthenticated: call getAppConfig({ baseOnly: true }) for zero DB
queries, return only login-relevant fields (social logins, turnstile,
privacy policy / terms of service from interface config)
- When authenticated: call getAppConfig({ role, userId, tenantId }) to
resolve per-user DB overrides (USER + ROLE + GROUP + PUBLIC principals),
return full payload including modelSpecs, balance, webSearch, etc.
- Extract buildSharedPayload() and addWebSearchConfig() helpers to avoid
duplication between the two code paths
- Fixes per-user balance overrides not appearing in the frontend because
userId was never passed to getAppConfig (follow-up to #12474)
* test: rewrite config route tests for unauthenticated vs authenticated paths
- Replace the previously-skipped supertest tests with proper mocked tests
- Cover unauthenticated path: baseOnly config call, minimal payload,
interface subset (privacyPolicy/termsOfService only), exclusion of
authenticated-only fields
- Cover authenticated path: getAppConfig called with userId, full payload
including modelSpecs/balance/webSearch, per-user balance override merging
* fix: address review findings — restore multi-tenant support, improve tests
- Chain preAuthTenantMiddleware back before optionalJwtAuth on /api/config
so unauthenticated requests in multi-tenant deployments still get
tenant-scoped config via X-Tenant-Id header (Finding #1)
- Use getAppConfig({ tenantId }) instead of getAppConfig({ baseOnly: true })
when a tenant context is present; fall back to baseOnly for single-tenant
- Fix @type annotation: unauthenticated payload is Partial<TStartupConfig>
- Refactor addWebSearchConfig into pure buildWebSearchConfig that returns a
value instead of mutating the payload argument
- Hoist isBirthday() to module level
- Remove inline narration comments
- Assert tenantId propagation in tests, including getTenantId fallback and
user.tenantId preference
- Add error-path tests for both unauthenticated and authenticated branches
- Expand afterEach env var cleanup for proper test isolation
* test: fix mock isolation and add tenant-scoped response test
- Replace jest.clearAllMocks() with jest.resetAllMocks() so
mockReturnValue implementations don't leak between tests
- Add test verifying tenant-scoped socialLogins and turnstile are
correctly mapped in the unauthenticated response
* fix: add optionalJwtAuth to /api/config in experimental.js
Without this middleware, req.user is never populated in the experimental
cluster entrypoint, so authenticated users always receive the minimal
unauthenticated config payload.
This commit is contained in:
Danny Avila
GitHub
parent
7181174c3b
commit
2e706ebcb3
4 changed files with 390 additions and 165 deletions
|
|
@ -1,25 +1,73 @@
|
|||
jest.mock('~/cache/getLogStores');
|
||||
|
||||
const mockGetAppConfig = jest.fn();
|
||||
jest.mock('~/server/services/Config/app', () => ({
|
||||
getAppConfig: (...args) => mockGetAppConfig(...args),
|
||||
}));
|
||||
|
||||
jest.mock('~/server/services/Config/ldap', () => ({
|
||||
getLdapConfig: jest.fn(() => null),
|
||||
}));
|
||||
|
||||
const mockGetTenantId = jest.fn(() => undefined);
|
||||
jest.mock('@librechat/data-schemas', () => ({
|
||||
...jest.requireActual('@librechat/data-schemas'),
|
||||
getTenantId: (...args) => mockGetTenantId(...args),
|
||||
}));
|
||||
|
||||
const request = require('supertest');
|
||||
const express = require('express');
|
||||
const configRoute = require('../config');
|
||||
// file deepcode ignore UseCsurfForExpress/test: test
|
||||
const app = express();
|
||||
app.disable('x-powered-by');
|
||||
app.use('/api/config', configRoute);
|
||||
|
||||
function createApp(user) {
|
||||
const app = express();
|
||||
app.disable('x-powered-by');
|
||||
if (user) {
|
||||
app.use((req, _res, next) => {
|
||||
req.user = user;
|
||||
next();
|
||||
});
|
||||
}
|
||||
app.use('/api/config', configRoute);
|
||||
return app;
|
||||
}
|
||||
|
||||
const baseAppConfig = {
|
||||
registration: { socialLogins: ['google', 'github'] },
|
||||
interfaceConfig: {
|
||||
privacyPolicy: { externalUrl: 'https://example.com/privacy' },
|
||||
termsOfService: { externalUrl: 'https://example.com/tos' },
|
||||
modelSelect: true,
|
||||
},
|
||||
turnstileConfig: { siteKey: 'test-key' },
|
||||
modelSpecs: { list: [{ name: 'test-spec' }] },
|
||||
webSearch: { searchProvider: 'tavily' },
|
||||
};
|
||||
|
||||
const mockUser = {
|
||||
id: 'user123',
|
||||
role: 'USER',
|
||||
tenantId: undefined,
|
||||
};
|
||||
|
||||
afterEach(() => {
|
||||
jest.resetAllMocks();
|
||||
delete process.env.APP_TITLE;
|
||||
delete process.env.CHECK_BALANCE;
|
||||
delete process.env.START_BALANCE;
|
||||
delete process.env.SANDPACK_BUNDLER_URL;
|
||||
delete process.env.SANDPACK_STATIC_BUNDLER_URL;
|
||||
delete process.env.CONVERSATION_IMPORT_MAX_FILE_SIZE_BYTES;
|
||||
delete process.env.ALLOW_REGISTRATION;
|
||||
delete process.env.ALLOW_SOCIAL_LOGIN;
|
||||
delete process.env.ALLOW_PASSWORD_RESET;
|
||||
delete process.env.DOMAIN_SERVER;
|
||||
delete process.env.GOOGLE_CLIENT_ID;
|
||||
delete process.env.GOOGLE_CLIENT_SECRET;
|
||||
delete process.env.FACEBOOK_CLIENT_ID;
|
||||
delete process.env.FACEBOOK_CLIENT_SECRET;
|
||||
delete process.env.OPENID_CLIENT_ID;
|
||||
delete process.env.OPENID_CLIENT_SECRET;
|
||||
delete process.env.OPENID_ISSUER;
|
||||
delete process.env.OPENID_SESSION_SECRET;
|
||||
delete process.env.OPENID_BUTTON_LABEL;
|
||||
delete process.env.OPENID_AUTO_REDIRECT;
|
||||
delete process.env.OPENID_AUTH_URL;
|
||||
delete process.env.GITHUB_CLIENT_ID;
|
||||
delete process.env.GITHUB_CLIENT_SECRET;
|
||||
delete process.env.DISCORD_CLIENT_ID;
|
||||
|
|
@ -28,78 +76,215 @@ afterEach(() => {
|
|||
delete process.env.SAML_ISSUER;
|
||||
delete process.env.SAML_CERT;
|
||||
delete process.env.SAML_SESSION_SECRET;
|
||||
delete process.env.SAML_BUTTON_LABEL;
|
||||
delete process.env.SAML_IMAGE_URL;
|
||||
delete process.env.DOMAIN_SERVER;
|
||||
delete process.env.ALLOW_REGISTRATION;
|
||||
delete process.env.ALLOW_SOCIAL_LOGIN;
|
||||
delete process.env.ALLOW_PASSWORD_RESET;
|
||||
delete process.env.LDAP_URL;
|
||||
delete process.env.LDAP_BIND_DN;
|
||||
delete process.env.LDAP_BIND_CREDENTIALS;
|
||||
delete process.env.LDAP_USER_SEARCH_BASE;
|
||||
delete process.env.LDAP_SEARCH_FILTER;
|
||||
});
|
||||
|
||||
//TODO: This works/passes locally but http request tests fail with 404 in CI. Need to figure out why.
|
||||
describe('GET /api/config', () => {
|
||||
describe('unauthenticated (no req.user)', () => {
|
||||
it('should call getAppConfig with baseOnly when no tenant context', async () => {
|
||||
mockGetAppConfig.mockResolvedValue(baseAppConfig);
|
||||
mockGetTenantId.mockReturnValue(undefined);
|
||||
const app = createApp(null);
|
||||
|
||||
describe.skip('GET /', () => {
|
||||
it('should return 200 and the correct body', async () => {
|
||||
process.env.APP_TITLE = 'Test Title';
|
||||
process.env.GOOGLE_CLIENT_ID = 'Test Google Client Id';
|
||||
process.env.GOOGLE_CLIENT_SECRET = 'Test Google Client Secret';
|
||||
process.env.FACEBOOK_CLIENT_ID = 'Test Facebook Client Id';
|
||||
process.env.FACEBOOK_CLIENT_SECRET = 'Test Facebook Client Secret';
|
||||
process.env.OPENID_CLIENT_ID = 'Test OpenID Id';
|
||||
process.env.OPENID_CLIENT_SECRET = 'Test OpenID Secret';
|
||||
process.env.OPENID_ISSUER = 'Test OpenID Issuer';
|
||||
process.env.OPENID_SESSION_SECRET = 'Test Secret';
|
||||
process.env.OPENID_BUTTON_LABEL = 'Test OpenID';
|
||||
process.env.OPENID_AUTH_URL = 'http://test-server.com';
|
||||
process.env.GITHUB_CLIENT_ID = 'Test Github client Id';
|
||||
process.env.GITHUB_CLIENT_SECRET = 'Test Github client Secret';
|
||||
process.env.DISCORD_CLIENT_ID = 'Test Discord client Id';
|
||||
process.env.DISCORD_CLIENT_SECRET = 'Test Discord client Secret';
|
||||
process.env.SAML_ENTRY_POINT = 'http://test-server.com';
|
||||
process.env.SAML_ISSUER = 'Test SAML Issuer';
|
||||
process.env.SAML_CERT = 'saml.pem';
|
||||
process.env.SAML_SESSION_SECRET = 'Test Secret';
|
||||
process.env.SAML_BUTTON_LABEL = 'Test SAML';
|
||||
process.env.SAML_IMAGE_URL = 'http://test-server.com';
|
||||
process.env.DOMAIN_SERVER = 'http://test-server.com';
|
||||
process.env.ALLOW_REGISTRATION = 'true';
|
||||
process.env.ALLOW_SOCIAL_LOGIN = 'true';
|
||||
process.env.ALLOW_PASSWORD_RESET = 'true';
|
||||
process.env.LDAP_URL = 'Test LDAP URL';
|
||||
process.env.LDAP_BIND_DN = 'Test LDAP Bind DN';
|
||||
process.env.LDAP_BIND_CREDENTIALS = 'Test LDAP Bind Credentials';
|
||||
process.env.LDAP_USER_SEARCH_BASE = 'Test LDAP User Search Base';
|
||||
process.env.LDAP_SEARCH_FILTER = 'Test LDAP Search Filter';
|
||||
await request(app).get('/api/config');
|
||||
|
||||
const response = await request(app).get('/');
|
||||
expect(mockGetAppConfig).toHaveBeenCalledWith({ baseOnly: true });
|
||||
});
|
||||
|
||||
expect(response.statusCode).toBe(200);
|
||||
expect(response.body).toEqual({
|
||||
appTitle: 'Test Title',
|
||||
socialLogins: ['google', 'facebook', 'openid', 'github', 'discord', 'saml'],
|
||||
discordLoginEnabled: true,
|
||||
facebookLoginEnabled: true,
|
||||
githubLoginEnabled: true,
|
||||
googleLoginEnabled: true,
|
||||
openidLoginEnabled: true,
|
||||
openidLabel: 'Test OpenID',
|
||||
openidImageUrl: 'http://test-server.com',
|
||||
samlLoginEnabled: true,
|
||||
samlLabel: 'Test SAML',
|
||||
samlImageUrl: 'http://test-server.com',
|
||||
ldap: {
|
||||
enabled: true,
|
||||
},
|
||||
serverDomain: 'http://test-server.com',
|
||||
emailLoginEnabled: 'true',
|
||||
registrationEnabled: 'true',
|
||||
passwordResetEnabled: 'true',
|
||||
socialLoginEnabled: 'true',
|
||||
it('should call getAppConfig with tenantId when tenant context is present', async () => {
|
||||
mockGetAppConfig.mockResolvedValue(baseAppConfig);
|
||||
mockGetTenantId.mockReturnValue('tenant-abc');
|
||||
const app = createApp(null);
|
||||
|
||||
await request(app).get('/api/config');
|
||||
|
||||
expect(mockGetAppConfig).toHaveBeenCalledWith({ tenantId: 'tenant-abc' });
|
||||
});
|
||||
|
||||
it('should map tenant-scoped config fields in unauthenticated response', async () => {
|
||||
const tenantConfig = {
|
||||
...baseAppConfig,
|
||||
registration: { socialLogins: ['saml'] },
|
||||
turnstileConfig: { siteKey: 'tenant-key' },
|
||||
};
|
||||
mockGetAppConfig.mockResolvedValue(tenantConfig);
|
||||
mockGetTenantId.mockReturnValue('tenant-abc');
|
||||
const app = createApp(null);
|
||||
|
||||
const response = await request(app).get('/api/config');
|
||||
|
||||
expect(response.statusCode).toBe(200);
|
||||
expect(response.body.socialLogins).toEqual(['saml']);
|
||||
expect(response.body.turnstile).toEqual({ siteKey: 'tenant-key' });
|
||||
expect(response.body).not.toHaveProperty('modelSpecs');
|
||||
});
|
||||
|
||||
it('should return minimal payload without authenticated-only fields', async () => {
|
||||
mockGetAppConfig.mockResolvedValue(baseAppConfig);
|
||||
const app = createApp(null);
|
||||
|
||||
const response = await request(app).get('/api/config');
|
||||
|
||||
expect(response.statusCode).toBe(200);
|
||||
expect(response.body).not.toHaveProperty('modelSpecs');
|
||||
expect(response.body).not.toHaveProperty('balance');
|
||||
expect(response.body).not.toHaveProperty('webSearch');
|
||||
expect(response.body).not.toHaveProperty('bundlerURL');
|
||||
expect(response.body).not.toHaveProperty('staticBundlerURL');
|
||||
expect(response.body).not.toHaveProperty('sharePointFilePickerEnabled');
|
||||
expect(response.body).not.toHaveProperty('conversationImportMaxFileSize');
|
||||
});
|
||||
|
||||
it('should include socialLogins and turnstile from base config', async () => {
|
||||
mockGetAppConfig.mockResolvedValue(baseAppConfig);
|
||||
const app = createApp(null);
|
||||
|
||||
const response = await request(app).get('/api/config');
|
||||
|
||||
expect(response.body.socialLogins).toEqual(['google', 'github']);
|
||||
expect(response.body.turnstile).toEqual({ siteKey: 'test-key' });
|
||||
});
|
||||
|
||||
it('should include only privacyPolicy and termsOfService from interface config', async () => {
|
||||
mockGetAppConfig.mockResolvedValue(baseAppConfig);
|
||||
const app = createApp(null);
|
||||
|
||||
const response = await request(app).get('/api/config');
|
||||
|
||||
expect(response.body.interface).toEqual({
|
||||
privacyPolicy: { externalUrl: 'https://example.com/privacy' },
|
||||
termsOfService: { externalUrl: 'https://example.com/tos' },
|
||||
});
|
||||
expect(response.body.interface).not.toHaveProperty('modelSelect');
|
||||
});
|
||||
|
||||
it('should not include interface if no privacyPolicy or termsOfService', async () => {
|
||||
mockGetAppConfig.mockResolvedValue({
|
||||
...baseAppConfig,
|
||||
interfaceConfig: { modelSelect: true },
|
||||
});
|
||||
const app = createApp(null);
|
||||
|
||||
const response = await request(app).get('/api/config');
|
||||
|
||||
expect(response.body).not.toHaveProperty('interface');
|
||||
});
|
||||
|
||||
it('should include shared env var fields', async () => {
|
||||
mockGetAppConfig.mockResolvedValue(baseAppConfig);
|
||||
process.env.APP_TITLE = 'Test App';
|
||||
const app = createApp(null);
|
||||
|
||||
const response = await request(app).get('/api/config');
|
||||
|
||||
expect(response.body.appTitle).toBe('Test App');
|
||||
expect(response.body).toHaveProperty('emailLoginEnabled');
|
||||
expect(response.body).toHaveProperty('serverDomain');
|
||||
});
|
||||
|
||||
it('should return 500 when getAppConfig throws', async () => {
|
||||
mockGetAppConfig.mockRejectedValue(new Error('Config service failure'));
|
||||
const app = createApp(null);
|
||||
|
||||
const response = await request(app).get('/api/config');
|
||||
|
||||
expect(response.statusCode).toBe(500);
|
||||
expect(response.body).toHaveProperty('error');
|
||||
});
|
||||
});
|
||||
|
||||
describe('authenticated (req.user exists)', () => {
|
||||
it('should call getAppConfig with role, userId, and tenantId', async () => {
|
||||
mockGetAppConfig.mockResolvedValue(baseAppConfig);
|
||||
mockGetTenantId.mockReturnValue('fallback-tenant');
|
||||
const app = createApp(mockUser);
|
||||
|
||||
await request(app).get('/api/config');
|
||||
|
||||
expect(mockGetAppConfig).toHaveBeenCalledWith({
|
||||
role: 'USER',
|
||||
userId: 'user123',
|
||||
tenantId: 'fallback-tenant',
|
||||
});
|
||||
});
|
||||
|
||||
it('should prefer user tenantId over getTenantId fallback', async () => {
|
||||
mockGetAppConfig.mockResolvedValue(baseAppConfig);
|
||||
mockGetTenantId.mockReturnValue('fallback-tenant');
|
||||
const app = createApp({ ...mockUser, tenantId: 'user-tenant' });
|
||||
|
||||
await request(app).get('/api/config');
|
||||
|
||||
expect(mockGetAppConfig).toHaveBeenCalledWith({
|
||||
role: 'USER',
|
||||
userId: 'user123',
|
||||
tenantId: 'user-tenant',
|
||||
});
|
||||
});
|
||||
|
||||
it('should include modelSpecs, balance, and webSearch', async () => {
|
||||
mockGetAppConfig.mockResolvedValue(baseAppConfig);
|
||||
process.env.CHECK_BALANCE = 'true';
|
||||
process.env.START_BALANCE = '10000';
|
||||
const app = createApp(mockUser);
|
||||
|
||||
const response = await request(app).get('/api/config');
|
||||
|
||||
expect(response.body.modelSpecs).toEqual({ list: [{ name: 'test-spec' }] });
|
||||
expect(response.body.balance).toEqual({ enabled: true, startBalance: 10000 });
|
||||
expect(response.body.webSearch).toEqual({ searchProvider: 'tavily' });
|
||||
});
|
||||
|
||||
it('should include full interface config', async () => {
|
||||
mockGetAppConfig.mockResolvedValue(baseAppConfig);
|
||||
const app = createApp(mockUser);
|
||||
|
||||
const response = await request(app).get('/api/config');
|
||||
|
||||
expect(response.body.interface).toEqual(baseAppConfig.interfaceConfig);
|
||||
});
|
||||
|
||||
it('should include authenticated-only env var fields', async () => {
|
||||
mockGetAppConfig.mockResolvedValue(baseAppConfig);
|
||||
process.env.SANDPACK_BUNDLER_URL = 'https://bundler.test';
|
||||
process.env.SANDPACK_STATIC_BUNDLER_URL = 'https://static-bundler.test';
|
||||
process.env.CONVERSATION_IMPORT_MAX_FILE_SIZE_BYTES = '5000000';
|
||||
const app = createApp(mockUser);
|
||||
|
||||
const response = await request(app).get('/api/config');
|
||||
|
||||
expect(response.body.bundlerURL).toBe('https://bundler.test');
|
||||
expect(response.body.staticBundlerURL).toBe('https://static-bundler.test');
|
||||
expect(response.body.conversationImportMaxFileSize).toBe(5000000);
|
||||
});
|
||||
|
||||
it('should merge per-user balance override into config', async () => {
|
||||
mockGetAppConfig.mockResolvedValue({
|
||||
...baseAppConfig,
|
||||
balance: {
|
||||
enabled: true,
|
||||
startBalance: 50000,
|
||||
},
|
||||
});
|
||||
const app = createApp(mockUser);
|
||||
|
||||
const response = await request(app).get('/api/config');
|
||||
|
||||
expect(response.body.balance).toEqual(
|
||||
expect.objectContaining({
|
||||
enabled: true,
|
||||
startBalance: 50000,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('should return 500 when getAppConfig throws', async () => {
|
||||
mockGetAppConfig.mockRejectedValue(new Error('Config service failure'));
|
||||
const app = createApp(mockUser);
|
||||
|
||||
const response = await request(app).get('/api/config');
|
||||
|
||||
expect(response.statusCode).toBe(500);
|
||||
expect(response.body).toHaveProperty('error');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue