mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
fix: Harden OpenID role-sync claim and fallback handling
Addresses the second Codex review cycle (P2 findings): - Apply fallback when the claim is absent: getOpenIdRolesForOpenIdSync now returns an empty list (not undefined) when the token source exists but has no usable claim value, so callers still run selection and assign the configured fallback instead of leaving a stale elevated role. A truly unavailable source still returns undefined and skips sync. - Resolve group overage for access tokens too: the _claim_names/_claim_sources overage path previously only ran for claimSource 'id'; Entra also moves an oversized groups claim into access tokens, so 'access'+'groups' (the only source supported by remote-agent API sync) now resolves overage as well. - Allow system fallback roles for tenant users: getLibreChatRolesForOpenIdSync treats SystemRoles (e.g. USER) as always-available canonical names, since they are provisioned globally at startup and a tenant-scoped lookup may not return them — preventing a spurious 'configured roles do not exist: USER'. Adds unit and strategy-level coverage for all three.
This commit is contained in:
parent
c3cc330a19
commit
80885e6161
3 changed files with 91 additions and 3 deletions
|
|
@ -1670,6 +1670,23 @@ describe('setupOpenId', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('applies fallback when the role claim is absent from the token', async () => {
|
||||
// Required-role gate reads the same `roles` claim; disable it to model an IdP
|
||||
// that authenticates the user but stops emitting the role claim entirely.
|
||||
delete process.env.OPENID_REQUIRED_ROLE;
|
||||
jwtDecode.mockReturnValue({
|
||||
permissions: ['not-admin'],
|
||||
});
|
||||
|
||||
const { user } = await validate(tokenset);
|
||||
|
||||
expect(user.role).toBe('USER');
|
||||
expect(updateUser).toHaveBeenCalledWith(
|
||||
'newUserId',
|
||||
expect.objectContaining({ role: 'USER' }),
|
||||
);
|
||||
});
|
||||
|
||||
it('rejects login when configured sync roles do not exist', async () => {
|
||||
findRolesByNames.mockImplementation(async (roleNames) =>
|
||||
roleNames
|
||||
|
|
|
|||
|
|
@ -94,13 +94,22 @@ describe('getOpenIdRolesForOpenIdSync', () => {
|
|||
).resolves.toEqual(['STANDARD-USER']);
|
||||
});
|
||||
|
||||
it('returns undefined for missing or invalid claim values', async () => {
|
||||
it('returns an empty list when the source lacks a usable claim value (applies fallback)', async () => {
|
||||
await expect(
|
||||
getOpenIdRolesForOpenIdSync({
|
||||
options,
|
||||
idToken: 'id-token',
|
||||
decodeToken: () => ({ roles: { STANDARD_USER: true } }),
|
||||
}),
|
||||
).resolves.toEqual([]);
|
||||
});
|
||||
|
||||
it('returns undefined when no token source is available (skips sync)', async () => {
|
||||
await expect(
|
||||
getOpenIdRolesForOpenIdSync({
|
||||
options,
|
||||
decodeToken: () => ({ roles: ['STANDARD-USER'] }),
|
||||
}),
|
||||
).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
|
|
@ -125,10 +134,24 @@ describe('getOpenIdRolesForOpenIdSync', () => {
|
|||
idToken: 'id-token',
|
||||
decodeToken,
|
||||
}),
|
||||
).resolves.toBeUndefined();
|
||||
).resolves.toEqual([]);
|
||||
expect(decodeToken).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('resolves group overage from an access token source (not just id)', async () => {
|
||||
await expect(
|
||||
getOpenIdRolesForOpenIdSync({
|
||||
options: { ...options, claimSource: 'access', claim: 'groups' },
|
||||
accessClaims: {
|
||||
_claim_names: { groups: 'src1' },
|
||||
_claim_sources: { src1: { endpoint: 'https://graph' } },
|
||||
},
|
||||
decodeToken: () => ({}),
|
||||
resolveGroupOverage: async () => ['group-a'],
|
||||
}),
|
||||
).resolves.toEqual(['group-a']);
|
||||
});
|
||||
|
||||
it('uses claims for the id source when no id token is available', async () => {
|
||||
const decodeToken = jest.fn();
|
||||
const claims = { roles: ['STANDARD-USER'] };
|
||||
|
|
@ -193,6 +216,27 @@ describe('getLibreChatRolesForOpenIdSync', () => {
|
|||
}),
|
||||
).rejects.toThrow('[openidStrategy] OpenID role sync configured roles do not exist: MISSING');
|
||||
});
|
||||
|
||||
it('accepts a system fallback role even when a tenant-scoped lookup omits it', async () => {
|
||||
// Tenant-scoped getRolesByNames returns only the tenant's own roles, not the
|
||||
// globally-provisioned system USER role.
|
||||
const getRolesByNames = jest.fn(async (roleNames: string[]) =>
|
||||
roleNames
|
||||
.filter((roleName) => roleName === 'STANDARD-USER')
|
||||
.map((roleName) => ({ name: roleName })),
|
||||
);
|
||||
|
||||
await expect(
|
||||
getLibreChatRolesForOpenIdSync({
|
||||
getRolesByNames,
|
||||
rolePriority: ['STANDARD-USER'],
|
||||
fallbackRole: SystemRoles.USER,
|
||||
}),
|
||||
).resolves.toEqual({
|
||||
rolePriority: ['STANDARD-USER'],
|
||||
fallbackRole: SystemRoles.USER,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('selectOpenIdRole', () => {
|
||||
|
|
|
|||
|
|
@ -131,7 +131,15 @@ export async function getOpenIdRolesForOpenIdSync({
|
|||
return;
|
||||
}
|
||||
|
||||
if (options.claimSource === 'id' && options.claim === 'groups') {
|
||||
/**
|
||||
* Azure AD/Entra moves an oversized `groups` claim into `_claim_names`/`_claim_sources`
|
||||
* for both ID and access tokens, so overage resolution must cover both sources — not
|
||||
* just the ID token — or `access`-sourced syncs silently see no groups for those users.
|
||||
*/
|
||||
const supportsGroupOverage =
|
||||
options.claim === 'groups' &&
|
||||
(options.claimSource === 'id' || options.claimSource === 'access');
|
||||
if (supportsGroupOverage) {
|
||||
const claimsData = source as {
|
||||
hasgroups?: unknown;
|
||||
_claim_names?: { groups?: string };
|
||||
|
|
@ -151,6 +159,13 @@ export async function getOpenIdRolesForOpenIdSync({
|
|||
if (Array.isArray(openIdRoleValues) || typeof openIdRoleValues === 'string') {
|
||||
return openIdRoleValues;
|
||||
}
|
||||
/**
|
||||
* The source is available but carries no usable value for the configured claim
|
||||
* (absent, null, or a non-string/array type). Return an empty list rather than
|
||||
* `undefined` so callers still run selection and apply the configured fallback,
|
||||
* instead of leaving a stale elevated role in place.
|
||||
*/
|
||||
return [];
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -184,6 +199,18 @@ export async function getLibreChatRolesForOpenIdSync(
|
|||
.filter((role): role is { name: string } => typeof role?.name === 'string')
|
||||
.map((role) => [role.name.trim().toLowerCase(), role.name.trim()]),
|
||||
);
|
||||
/**
|
||||
* System roles (e.g. USER) are provisioned globally at startup without a tenant
|
||||
* context, so a tenant-scoped lookup may not return them even though they exist.
|
||||
* Treat them as always-available canonical names so a documented system fallback
|
||||
* role does not fail validation for tenant users.
|
||||
*/
|
||||
for (const systemRole of Object.values(SystemRoles)) {
|
||||
const key = systemRole.toLowerCase();
|
||||
if (!existingRoleNames.has(key)) {
|
||||
existingRoleNames.set(key, systemRole);
|
||||
}
|
||||
}
|
||||
const missingRoleNames = uniqueRoleNames.filter(
|
||||
(roleName) => !existingRoleNames.has(roleName.toLowerCase()),
|
||||
);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue