mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
📬 fix: Honor Admin-Panel allowedDomains Override at Registration (#13204)
* fix: honor admin-panel allowedDomains override at registration
registerUser called getAppConfig({ baseOnly: true }), which short-
circuits before any DB override merge. As a result, admin-panel edits to
registration.allowedDomains were silently ignored at signup, even though
they correctly apply to SSO callbacks via checkDomainAllowed (which
calls getAppConfig() with the full resolution).
The admin panel writes registration.allowedDomains to the __base__
principal in the configs collection. That principal is unconditionally
injected by getApplicableConfigs (no user identity required), so a
fully-resolved getAppConfig call picks up the override even before any
user exists. This aligns native signup with the SSO paths and lets
admins tighten or relax the allowed list without a backend restart.
Per review feedback: pass the ALS tenantId explicitly. /api/auth runs
through preAuthTenantMiddleware, which puts a tenantId into
AsyncLocalStorage. Mongoose queries inside getApplicableConfigs are
ALS-scoped, but the per-principal merged-config cache key uses the
*explicit* tenantId parameter (see overrideCacheKey in
packages/api/src/app/service.ts). If we leave tenantId undefined while
ALS holds tenant A, the merged result caches at `__default__` — and a
later request from tenant B would hit that entry, leaking tenant A's
allowedDomains (and balance) across tenants. Reading getTenantId() and
forwarding it makes the cache key match the DB scope, so __base__
overrides apply per-tenant correctly.
Behavior when no admin override exists is unchanged (the merged config
equals the YAML config; optional chaining handles missing fields).
Tests in AuthService.spec.js:
- Regression guard that getAppConfig is called with `{}` (no baseOnly)
when ALS has no tenant — protects against reintroduction of the
short-circuit.
- New tenant-context test verifying getAppConfig({ tenantId }) when
getTenantId() returns a tenant ID — protects against cross-tenant
cache bleed.
- Behavioral test confirming a disallowed domain returns 403 before any
DB user lookup.
* test: remove unused registerSchema import after merge resolution
---------
Co-authored-by: Danny Avila <danny@librechat.ai>
This commit is contained in:
parent
3487672a32
commit
6a04fb89e2
2 changed files with 64 additions and 2 deletions
|
|
@ -3,6 +3,7 @@ const jwt = require('jsonwebtoken');
|
|||
const { webcrypto } = require('node:crypto');
|
||||
const {
|
||||
logger,
|
||||
getTenantId,
|
||||
DEFAULT_SESSION_EXPIRY,
|
||||
DEFAULT_REFRESH_TOKEN_EXPIRY,
|
||||
} = require('@librechat/data-schemas');
|
||||
|
|
@ -212,7 +213,8 @@ const registerUser = async (user, additionalData = {}) => {
|
|||
|
||||
let newUserId;
|
||||
try {
|
||||
const appConfig = await getAppConfig({ baseOnly: true });
|
||||
const tenantId = getTenantId();
|
||||
const appConfig = await getAppConfig(tenantId ? { tenantId } : {});
|
||||
if (!isEmailDomainAllowed(email, appConfig?.registration?.allowedDomains)) {
|
||||
const errorMessage =
|
||||
'The email address provided cannot be used. Please use a different email address.';
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
jest.mock('@librechat/data-schemas', () => ({
|
||||
logger: { info: jest.fn(), warn: jest.fn(), debug: jest.fn(), error: jest.fn() },
|
||||
getTenantId: jest.fn(() => undefined),
|
||||
DEFAULT_SESSION_EXPIRY: 900000,
|
||||
DEFAULT_REFRESH_TOKEN_EXPIRY: 604800000,
|
||||
}));
|
||||
|
|
@ -69,7 +70,7 @@ const {
|
|||
parseCloudFrontCookieScope,
|
||||
} = require('@librechat/api');
|
||||
const jwt = require('jsonwebtoken');
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
const { logger, getTenantId } = require('@librechat/data-schemas');
|
||||
const {
|
||||
findUser,
|
||||
createUser,
|
||||
|
|
@ -887,3 +888,62 @@ describe('CloudFront cookie integration', () => {
|
|||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('registerUser - allowedDomains admin-panel override', () => {
|
||||
const validUser = {
|
||||
email: 'new-user@example.com',
|
||||
password: 'a-secure-password',
|
||||
name: 'New User',
|
||||
username: 'new-user',
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
getTenantId.mockReturnValue(undefined);
|
||||
isEmailDomainAllowed.mockReturnValue(true);
|
||||
getAppConfig.mockResolvedValue({
|
||||
registration: { allowedDomains: ['example.com'] },
|
||||
balance: undefined,
|
||||
});
|
||||
findUser.mockResolvedValue(null);
|
||||
countUsers.mockResolvedValue(0);
|
||||
});
|
||||
|
||||
it('should resolve the full app config so admin-panel overrides on the __base__ principal apply', async () => {
|
||||
// Regression guard for getAppConfig({ baseOnly: true }): that option short-circuits
|
||||
// before the DB override merge, which silently ignores any admin-panel edits to
|
||||
// registration.allowedDomains (the admin panel writes overrides to the __base__
|
||||
// principal in the configs collection). registerUser must request the merged config
|
||||
// so the global __base__ override is honored, same as it is for SSO callbacks via
|
||||
// checkDomainAllowed.
|
||||
await registerUser(validUser);
|
||||
|
||||
expect(getAppConfig).toHaveBeenCalledTimes(1);
|
||||
expect(getAppConfig).toHaveBeenCalledWith({});
|
||||
expect(getAppConfig).not.toHaveBeenCalledWith(expect.objectContaining({ baseOnly: true }));
|
||||
});
|
||||
|
||||
it('should pass tenantId from ALS so the merged-config cache key matches tenant-scoped DB queries', async () => {
|
||||
// /api/auth runs through preAuthTenantMiddleware, which puts a tenantId into
|
||||
// AsyncLocalStorage. Mongoose queries inside getApplicableConfigs are scoped by ALS,
|
||||
// but the per-principal merged-config cache key uses the explicit tenantId param.
|
||||
// If we don't forward the ALS tenantId, tenant A's request caches at `__default__`
|
||||
// and a later tenant B request can hit that entry — leaking config across tenants.
|
||||
getTenantId.mockReturnValue('tenant-x');
|
||||
|
||||
await registerUser(validUser);
|
||||
|
||||
expect(getAppConfig).toHaveBeenCalledWith({ tenantId: 'tenant-x' });
|
||||
});
|
||||
|
||||
it('should block registration when the resolved allowedDomains rejects the email', async () => {
|
||||
isEmailDomainAllowed.mockReturnValue(false);
|
||||
|
||||
const result = await registerUser({ ...validUser, email: 'blocked@evil.com' });
|
||||
|
||||
expect(result.status).toBe(403);
|
||||
expect(result.message).toMatch(/cannot be used/i);
|
||||
// Domain check must happen before any DB user lookup.
|
||||
expect(findUser).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue