mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
fix: Tighten OpenID role-sync tenant scoping and config validation
Addresses the third Codex review cycle: - Constrain base-user role lookups to base roles (P2): findRolesByNames now filters to roles with an unset tenantId when no tenant ALS context is active, so a base user cannot match — and be assigned — a role that only exists within a tenant. Tenant-scoped lookups remain controlled by the isolation plugin. - Re-enforce tenant login policy after role sync (P2): when role sync changes a tenant user's role, the OpenID strategy re-resolves the tenant appConfig and re-checks allowedDomains, so a token cannot complete login under the previous role's looser policy. - Skip role-sync-specific validation when disabled (P3): getOpenIdRoleSyncOptions returns disabled options before validating role-sync settings, so a stale or mistyped value no longer breaks OpenID login while the feature is off. Adds unit and strategy-level coverage for all three.
This commit is contained in:
parent
80885e6161
commit
0188f41348
6 changed files with 113 additions and 12 deletions
|
|
@ -750,6 +750,7 @@ async function processOpenIDAuth(tokenset, existingUsersOnly = false) {
|
|||
}
|
||||
|
||||
if (!adminRoleGranted) {
|
||||
const roleBeforeSync = user.role;
|
||||
await applyOpenIdRoleSync({
|
||||
user,
|
||||
username,
|
||||
|
|
@ -758,6 +759,21 @@ async function processOpenIDAuth(tokenset, existingUsersOnly = false) {
|
|||
userinfo,
|
||||
resolvedOverageGroups,
|
||||
});
|
||||
/**
|
||||
* The earlier login-policy check ran with the pre-sync role. If role sync moved a
|
||||
* tenant user into a different role, re-resolve the tenant config and re-enforce
|
||||
* `allowedDomains` so role-scoped overrides for the new role are honored and a token
|
||||
* cannot complete login under the previous role's looser policy.
|
||||
*/
|
||||
if (user?.tenantId && user.role !== roleBeforeSync) {
|
||||
const postSyncConfig = await resolveAppConfigForUser(getAppConfig, user);
|
||||
if (!isEmailDomainAllowed(email, postSyncConfig?.registration?.allowedDomains)) {
|
||||
logger.error(
|
||||
`[OpenID Strategy] Authentication blocked after role sync - email domain not allowed [Identifier: ${email}]`,
|
||||
);
|
||||
throw new Error('Email domain not allowed');
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (!!userinfo && userinfo.picture && !user.avatar?.includes('manual=true')) {
|
||||
|
|
|
|||
|
|
@ -1767,6 +1767,41 @@ describe('setupOpenId', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('re-enforces tenant login policy after role sync changes the role', async () => {
|
||||
const existingUser = {
|
||||
_id: 'existingTenantUserId',
|
||||
provider: 'openid',
|
||||
email: tokenset.claims().email,
|
||||
openidId: tokenset.claims().sub,
|
||||
username: 'tenantuser',
|
||||
name: 'Tenant User',
|
||||
tenantId: 'tenant-a',
|
||||
role: 'USER',
|
||||
};
|
||||
const { isEmailDomainAllowed } = require('@librechat/api');
|
||||
|
||||
findUser.mockImplementation(async (query) => {
|
||||
if (query.openidId === tokenset.claims().sub || query.email === tokenset.claims().email) {
|
||||
return existingUser;
|
||||
}
|
||||
return null;
|
||||
});
|
||||
jwtDecode.mockReturnValue({
|
||||
roles: ['requiredRole', 'BASIC-USER'],
|
||||
permissions: ['not-admin'],
|
||||
});
|
||||
// Pre-sync domain check passes; the post-sync re-resolved config rejects the domain.
|
||||
isEmailDomainAllowed.mockReturnValueOnce(true).mockReturnValueOnce(false);
|
||||
resolveAppConfigForUser.mockResolvedValue({
|
||||
registration: { allowedDomains: ['restricted.com'] },
|
||||
});
|
||||
|
||||
const { user, details } = await validate(tokenset);
|
||||
|
||||
expect(user).toBe(false);
|
||||
expect(details).toEqual({ message: 'Email domain not allowed' });
|
||||
});
|
||||
|
||||
it('reuses required-role overage groups for role sync', async () => {
|
||||
process.env.OPENID_REQUIRED_ROLE = 'group-required';
|
||||
process.env.OPENID_REQUIRED_ROLE_PARAMETER_PATH = 'groups';
|
||||
|
|
|
|||
|
|
@ -44,14 +44,26 @@ describe('getOpenIdRoleSyncOptions', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('rejects invalid claim sources', () => {
|
||||
it('rejects invalid claim sources when enabled', () => {
|
||||
expect(() =>
|
||||
getOpenIdRoleSyncOptions({
|
||||
OPENID_ROLE_SYNC_ENABLED: 'true',
|
||||
OPENID_ROLE_SYNC_CLAIM: 'roles',
|
||||
OPENID_ROLE_SYNC_SOURCE: 'profile',
|
||||
}),
|
||||
).toThrow('OPENID_ROLE_SYNC_SOURCE must be one of');
|
||||
});
|
||||
|
||||
it('ignores role-sync-specific settings when the feature is disabled', () => {
|
||||
expect(
|
||||
getOpenIdRoleSyncOptions({
|
||||
OPENID_ROLE_SYNC_SOURCE: 'profile',
|
||||
OPENID_ROLE_SYNC_ROLE_PRIORITY: SystemRoles.ADMIN,
|
||||
OPENID_ROLE_SYNC_FALLBACK_ROLE: SystemRoles.ADMIN,
|
||||
}),
|
||||
).toMatchObject({ enabled: false, apiEnabled: false });
|
||||
});
|
||||
|
||||
it('rejects API role sync when global role sync is disabled', () => {
|
||||
expect(() =>
|
||||
getOpenIdRoleSyncOptions({
|
||||
|
|
@ -60,15 +72,19 @@ describe('getOpenIdRoleSyncOptions', () => {
|
|||
).toThrow('OPENID_ROLE_SYNC_API_ENABLED requires OPENID_ROLE_SYNC_ENABLED=true');
|
||||
});
|
||||
|
||||
it('rejects ADMIN in role priority and fallback role', () => {
|
||||
it('rejects ADMIN in role priority and fallback role when enabled', () => {
|
||||
expect(() =>
|
||||
getOpenIdRoleSyncOptions({
|
||||
OPENID_ROLE_SYNC_ENABLED: 'true',
|
||||
OPENID_ROLE_SYNC_CLAIM: 'roles',
|
||||
OPENID_ROLE_SYNC_ROLE_PRIORITY: `STANDARD-USER,${SystemRoles.ADMIN}`,
|
||||
}),
|
||||
).toThrow('OPENID_ROLE_SYNC_ROLE_PRIORITY cannot include ADMIN');
|
||||
|
||||
expect(() =>
|
||||
getOpenIdRoleSyncOptions({
|
||||
OPENID_ROLE_SYNC_ENABLED: 'true',
|
||||
OPENID_ROLE_SYNC_CLAIM: 'roles',
|
||||
OPENID_ROLE_SYNC_FALLBACK_ROLE: SystemRoles.ADMIN,
|
||||
}),
|
||||
).toThrow('OPENID_ROLE_SYNC_FALLBACK_ROLE cannot be ADMIN');
|
||||
|
|
|
|||
|
|
@ -70,24 +70,33 @@ export function getOpenIdRoleSyncOptions(
|
|||
.filter(Boolean) ?? [];
|
||||
const fallbackRole = env.OPENID_ROLE_SYNC_FALLBACK_ROLE?.trim() || undefined;
|
||||
|
||||
if (apiEnabled && !enabled) {
|
||||
throw new Error(
|
||||
'[openidRoleSync] OPENID_ROLE_SYNC_API_ENABLED requires OPENID_ROLE_SYNC_ENABLED=true',
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Only validate role-sync-specific settings once the feature is enabled. A
|
||||
* disabled deployment must not fail OpenID login just because a stale or
|
||||
* mistyped OPENID_ROLE_SYNC_* value is left in the environment.
|
||||
*/
|
||||
if (!enabled) {
|
||||
return { enabled, apiEnabled, claimSource, claim, rolePriority, fallbackRole };
|
||||
}
|
||||
|
||||
if (!['access', 'id', 'userinfo'].includes(claimSource)) {
|
||||
throw new Error(
|
||||
`[openidRoleSync] OPENID_ROLE_SYNC_SOURCE must be one of: access, id, userinfo`,
|
||||
);
|
||||
}
|
||||
|
||||
if (enabled && !claim) {
|
||||
if (!claim) {
|
||||
throw new Error(
|
||||
'[openidRoleSync] OPENID_ROLE_SYNC_CLAIM is required when role sync is enabled',
|
||||
);
|
||||
}
|
||||
|
||||
if (apiEnabled && !enabled) {
|
||||
throw new Error(
|
||||
'[openidRoleSync] OPENID_ROLE_SYNC_API_ENABLED requires OPENID_ROLE_SYNC_ENABLED=true',
|
||||
);
|
||||
}
|
||||
|
||||
if (rolePriority.some((role) => role.toLowerCase() === SystemRoles.ADMIN.toLowerCase())) {
|
||||
throw new Error('[openidRoleSync] OPENID_ROLE_SYNC_ROLE_PRIORITY cannot include ADMIN');
|
||||
}
|
||||
|
|
|
|||
|
|
@ -125,6 +125,21 @@ describe('findRolesByNames', () => {
|
|||
expect.objectContaining({ name: 'TENANT-ROLE', tenantId: 'tenant-b' }),
|
||||
]);
|
||||
});
|
||||
|
||||
it('matches only base roles when no tenant context is active', async () => {
|
||||
await Role.create({ name: 'SCOPED-ROLE', permissions: {} });
|
||||
await tenantStorage.run({ tenantId: 'tenant-a' }, async () => {
|
||||
await Role.create({ name: 'TENANT-ONLY-ROLE', permissions: {} });
|
||||
});
|
||||
|
||||
const baseMatches = await findRolesByNames(
|
||||
['SCOPED-ROLE', 'TENANT-ONLY-ROLE'],
|
||||
'name tenantId',
|
||||
);
|
||||
|
||||
expect(baseMatches).toEqual([expect.objectContaining({ name: 'SCOPED-ROLE' })]);
|
||||
expect(baseMatches).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe('updateAccessPermissions', () => {
|
||||
|
|
|
|||
|
|
@ -7,7 +7,7 @@ import {
|
|||
} from 'librechat-data-provider';
|
||||
import type { Model } from 'mongoose';
|
||||
import type { IRole, IUser } from '~/types';
|
||||
import { scopedCacheKey } from '~/config/tenantContext';
|
||||
import { scopedCacheKey, getTenantId, SYSTEM_TENANT_ID } from '~/config/tenantContext';
|
||||
import { escapeRegExp } from '~/utils/string';
|
||||
import logger from '~/config/winston';
|
||||
|
||||
|
|
@ -127,6 +127,11 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol
|
|||
/**
|
||||
* Find roles by name without using or populating the shared role-name cache.
|
||||
* Use this for tenant-scoped authorization lookups where the active ALS tenant context must control the query.
|
||||
*
|
||||
* When a non-system tenant context is active, the tenant-isolation plugin scopes the
|
||||
* query to that tenant. When no tenant context is active (base/global users), the lookup
|
||||
* is constrained to base roles (`tenantId` unset) so a base user cannot match — and be
|
||||
* assigned — a role that only exists within some tenant.
|
||||
*/
|
||||
async function findRolesByNames(
|
||||
roleNames: string[],
|
||||
|
|
@ -141,11 +146,16 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol
|
|||
}
|
||||
|
||||
const Role = mongoose.models.Role;
|
||||
let query = Role.find({
|
||||
const filter: Record<string, unknown> = {
|
||||
$or: uniqueRoleNames.map((roleName) => ({
|
||||
name: new RegExp(`^${escapeRegExp(roleName)}$`, 'i'),
|
||||
})),
|
||||
});
|
||||
};
|
||||
const tenantId = getTenantId();
|
||||
if (!tenantId || tenantId === SYSTEM_TENANT_ID) {
|
||||
filter.tenantId = { $in: [null, undefined] };
|
||||
}
|
||||
let query = Role.find(filter);
|
||||
if (fieldsToSelect) {
|
||||
query = query.select(fieldsToSelect);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue