From 0188f413482447232efb102068446c58c122a95d Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 1 Jun 2026 23:04:45 -0400 Subject: [PATCH] fix: Tighten OpenID role-sync tenant scoping and config validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- api/strategies/openidStrategy.js | 16 +++++++++ api/strategies/openidStrategy.spec.js | 35 +++++++++++++++++++ packages/api/src/auth/openidRoleSync.spec.ts | 20 +++++++++-- packages/api/src/auth/openidRoleSync.ts | 23 ++++++++---- .../src/methods/role.methods.spec.ts | 15 ++++++++ packages/data-schemas/src/methods/role.ts | 16 +++++++-- 6 files changed, 113 insertions(+), 12 deletions(-) diff --git a/api/strategies/openidStrategy.js b/api/strategies/openidStrategy.js index 2d7957639d..67da4bf007 100644 --- a/api/strategies/openidStrategy.js +++ b/api/strategies/openidStrategy.js @@ -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')) { diff --git a/api/strategies/openidStrategy.spec.js b/api/strategies/openidStrategy.spec.js index 7322811a07..b1d6081fe8 100644 --- a/api/strategies/openidStrategy.spec.js +++ b/api/strategies/openidStrategy.spec.js @@ -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'; diff --git a/packages/api/src/auth/openidRoleSync.spec.ts b/packages/api/src/auth/openidRoleSync.spec.ts index f80e57d965..4451d31f76 100644 --- a/packages/api/src/auth/openidRoleSync.spec.ts +++ b/packages/api/src/auth/openidRoleSync.spec.ts @@ -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'); diff --git a/packages/api/src/auth/openidRoleSync.ts b/packages/api/src/auth/openidRoleSync.ts index 266a7c0cc6..3f028213ee 100644 --- a/packages/api/src/auth/openidRoleSync.ts +++ b/packages/api/src/auth/openidRoleSync.ts @@ -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'); } diff --git a/packages/data-schemas/src/methods/role.methods.spec.ts b/packages/data-schemas/src/methods/role.methods.spec.ts index a803674869..a3169ef05a 100644 --- a/packages/data-schemas/src/methods/role.methods.spec.ts +++ b/packages/data-schemas/src/methods/role.methods.spec.ts @@ -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', () => { diff --git a/packages/data-schemas/src/methods/role.ts b/packages/data-schemas/src/methods/role.ts index 8e593704c9..2af8135a25 100644 --- a/packages/data-schemas/src/methods/role.ts +++ b/packages/data-schemas/src/methods/role.ts @@ -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 = { $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); }