mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-02-13 13:04:24 +01:00
🏢 fix: Handle Group Overage for Azure Entra Authentication (#11557)
small fix add tests reorder Update api/strategies/openidStrategy.spec.js Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Update api/strategies/openidStrategy.js Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> some fixes and fix fix more fixes fix
This commit is contained in:
parent
924be3b647
commit
417405a974
2 changed files with 403 additions and 1 deletions
|
|
@ -287,6 +287,77 @@ function convertToUsername(input, defaultValue = '') {
|
|||
return defaultValue;
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve Azure AD groups when group overage is in effect (groups moved to _claim_names/_claim_sources).
|
||||
*
|
||||
* NOTE: Microsoft recommends treating _claim_names/_claim_sources as a signal only and using Microsoft Graph
|
||||
* to resolve group membership instead of calling the endpoint in _claim_sources directly.
|
||||
*
|
||||
* @param {string} accessToken - Access token with Microsoft Graph permissions
|
||||
* @returns {Promise<string[] | null>} Resolved group IDs or null on failure
|
||||
* @see https://learn.microsoft.com/en-us/entra/identity-platform/access-token-claims-reference#groups-overage-claim
|
||||
* @see https://learn.microsoft.com/en-us/graph/api/directoryobject-getmemberobjects
|
||||
*/
|
||||
async function resolveGroupsFromOverage(accessToken) {
|
||||
try {
|
||||
if (!accessToken) {
|
||||
logger.error('[openidStrategy] Access token missing; cannot resolve group overage');
|
||||
return null;
|
||||
}
|
||||
|
||||
// Use /me/getMemberObjects so least-privileged delegated permission User.Read is sufficient
|
||||
// when resolving the signed-in user's group membership.
|
||||
const url = 'https://graph.microsoft.com/v1.0/me/getMemberObjects';
|
||||
|
||||
logger.debug(
|
||||
`[openidStrategy] Detected group overage, resolving groups via Microsoft Graph getMemberObjects: ${url}`,
|
||||
);
|
||||
|
||||
const fetchOptions = {
|
||||
method: 'POST',
|
||||
headers: {
|
||||
Authorization: `Bearer ${accessToken}`,
|
||||
'Content-Type': 'application/json',
|
||||
},
|
||||
body: JSON.stringify({ securityEnabledOnly: false }),
|
||||
};
|
||||
|
||||
if (process.env.PROXY) {
|
||||
const { ProxyAgent } = undici;
|
||||
fetchOptions.dispatcher = new ProxyAgent(process.env.PROXY);
|
||||
}
|
||||
|
||||
const response = await undici.fetch(url, fetchOptions);
|
||||
if (!response.ok) {
|
||||
logger.error(
|
||||
`[openidStrategy] Failed to resolve groups via Microsoft Graph getMemberObjects: HTTP ${response.status} ${response.statusText}`,
|
||||
);
|
||||
return null;
|
||||
}
|
||||
|
||||
const data = await response.json();
|
||||
const values = Array.isArray(data?.value) ? data.value : null;
|
||||
if (!values) {
|
||||
logger.error(
|
||||
'[openidStrategy] Unexpected response format when resolving groups via Microsoft Graph getMemberObjects',
|
||||
);
|
||||
return null;
|
||||
}
|
||||
const groupIds = values.filter((id) => typeof id === 'string');
|
||||
|
||||
logger.debug(
|
||||
`[openidStrategy] Successfully resolved ${groupIds.length} groups via Microsoft Graph getMemberObjects`,
|
||||
);
|
||||
return groupIds;
|
||||
} catch (err) {
|
||||
logger.error(
|
||||
'[openidStrategy] Error resolving groups via Microsoft Graph getMemberObjects:',
|
||||
err,
|
||||
);
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Process OpenID authentication tokenset and userinfo
|
||||
* This is the core logic extracted from the passport strategy callback
|
||||
|
|
@ -350,6 +421,25 @@ async function processOpenIDAuth(tokenset, existingUsersOnly = false) {
|
|||
}
|
||||
|
||||
let roles = get(decodedToken, requiredRoleParameterPath);
|
||||
|
||||
// Handle Azure AD group overage for ID token groups: when hasgroups or _claim_* indicate overage,
|
||||
// resolve groups via Microsoft Graph instead of relying on token group values.
|
||||
if (
|
||||
!Array.isArray(roles) &&
|
||||
typeof roles !== 'string' &&
|
||||
requiredRoleTokenKind === 'id' &&
|
||||
requiredRoleParameterPath === 'groups' &&
|
||||
decodedToken &&
|
||||
(decodedToken.hasgroups ||
|
||||
(decodedToken._claim_names?.groups &&
|
||||
decodedToken._claim_sources?.[decodedToken._claim_names.groups]))
|
||||
) {
|
||||
const overageGroups = await resolveGroupsFromOverage(tokenset.access_token);
|
||||
if (overageGroups) {
|
||||
roles = overageGroups;
|
||||
}
|
||||
}
|
||||
|
||||
if (!roles || (!Array.isArray(roles) && typeof roles !== 'string')) {
|
||||
logger.error(
|
||||
`[openidStrategy] Key '${requiredRoleParameterPath}' not found in ${requiredRoleTokenKind} token!`,
|
||||
|
|
@ -361,7 +451,9 @@ async function processOpenIDAuth(tokenset, existingUsersOnly = false) {
|
|||
throw new Error(`You must have ${rolesList} role to log in.`);
|
||||
}
|
||||
|
||||
if (!requiredRoles.some((role) => roles.includes(role))) {
|
||||
const roleValues = Array.isArray(roles) ? roles : [roles];
|
||||
|
||||
if (!requiredRoles.some((role) => roleValues.includes(role))) {
|
||||
const rolesList =
|
||||
requiredRoles.length === 1
|
||||
? `"${requiredRoles[0]}"`
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
const fetch = require('node-fetch');
|
||||
const jwtDecode = require('jsonwebtoken/decode');
|
||||
const undici = require('undici');
|
||||
const { ErrorTypes } = require('librechat-data-provider');
|
||||
const { findUser, createUser, updateUser } = require('~/models');
|
||||
const { setupOpenId } = require('./openidStrategy');
|
||||
|
|
@ -7,6 +8,10 @@ const { setupOpenId } = require('./openidStrategy');
|
|||
// --- Mocks ---
|
||||
jest.mock('node-fetch');
|
||||
jest.mock('jsonwebtoken/decode');
|
||||
jest.mock('undici', () => ({
|
||||
fetch: jest.fn(),
|
||||
ProxyAgent: jest.fn(),
|
||||
}));
|
||||
jest.mock('~/server/services/Files/strategies', () => ({
|
||||
getStrategyFunctions: jest.fn(() => ({
|
||||
saveBuffer: jest.fn().mockResolvedValue('/fake/path/to/avatar.png'),
|
||||
|
|
@ -360,6 +365,25 @@ describe('setupOpenId', () => {
|
|||
expect(details.message).toBe('You must have "requiredRole" role to log in.');
|
||||
});
|
||||
|
||||
it('should not treat substring matches in string roles as satisfying required role', async () => {
|
||||
// Arrange – override required role to "read" then re-setup
|
||||
process.env.OPENID_REQUIRED_ROLE = 'read';
|
||||
await setupOpenId();
|
||||
verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid');
|
||||
|
||||
// Token contains "bread" which *contains* "read" as a substring
|
||||
jwtDecode.mockReturnValue({
|
||||
roles: 'bread',
|
||||
});
|
||||
|
||||
// Act
|
||||
const { user, details } = await validate(tokenset);
|
||||
|
||||
// Assert – verify that substring match does not grant access
|
||||
expect(user).toBe(false);
|
||||
expect(details.message).toBe('You must have "read" role to log in.');
|
||||
});
|
||||
|
||||
it('should allow login when single required role is present (backward compatibility)', async () => {
|
||||
// Arrange – ensure single role configuration (as set in beforeEach)
|
||||
// OPENID_REQUIRED_ROLE = 'requiredRole'
|
||||
|
|
@ -378,6 +402,292 @@ describe('setupOpenId', () => {
|
|||
expect(createUser).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
describe('group overage and groups handling', () => {
|
||||
it.each([
|
||||
['groups array contains required group', ['group-required', 'other-group'], true, undefined],
|
||||
[
|
||||
'groups array missing required group',
|
||||
['other-group'],
|
||||
false,
|
||||
'You must have "group-required" role to log in.',
|
||||
],
|
||||
['groups string equals required group', 'group-required', true, undefined],
|
||||
[
|
||||
'groups string is other group',
|
||||
'other-group',
|
||||
false,
|
||||
'You must have "group-required" role to log in.',
|
||||
],
|
||||
])(
|
||||
'uses groups claim directly when %s (no overage)',
|
||||
async (_label, groupsClaim, expectedAllowed, expectedMessage) => {
|
||||
process.env.OPENID_REQUIRED_ROLE = 'group-required';
|
||||
process.env.OPENID_REQUIRED_ROLE_PARAMETER_PATH = 'groups';
|
||||
process.env.OPENID_REQUIRED_ROLE_TOKEN_KIND = 'id';
|
||||
|
||||
jwtDecode.mockReturnValue({
|
||||
groups: groupsClaim,
|
||||
permissions: ['admin'],
|
||||
});
|
||||
|
||||
await setupOpenId();
|
||||
verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid');
|
||||
|
||||
const { user, details } = await validate(tokenset);
|
||||
|
||||
expect(undici.fetch).not.toHaveBeenCalled();
|
||||
expect(Boolean(user)).toBe(expectedAllowed);
|
||||
expect(details?.message).toBe(expectedMessage);
|
||||
},
|
||||
);
|
||||
|
||||
it.each([
|
||||
['token kind is not id', { kind: 'access', path: 'groups', decoded: { hasgroups: true } }],
|
||||
['parameter path is not groups', { kind: 'id', path: 'roles', decoded: { hasgroups: true } }],
|
||||
['decoded token is falsy', { kind: 'id', path: 'groups', decoded: null }],
|
||||
[
|
||||
'no overage indicators in decoded token',
|
||||
{
|
||||
kind: 'id',
|
||||
path: 'groups',
|
||||
decoded: {
|
||||
permissions: ['admin'],
|
||||
},
|
||||
},
|
||||
],
|
||||
[
|
||||
'only _claim_names present (no _claim_sources)',
|
||||
{
|
||||
kind: 'id',
|
||||
path: 'groups',
|
||||
decoded: {
|
||||
_claim_names: { groups: 'src1' },
|
||||
permissions: ['admin'],
|
||||
},
|
||||
},
|
||||
],
|
||||
[
|
||||
'only _claim_sources present (no _claim_names)',
|
||||
{
|
||||
kind: 'id',
|
||||
path: 'groups',
|
||||
decoded: {
|
||||
_claim_sources: { src1: { endpoint: 'https://graph.windows.net/ignored' } },
|
||||
permissions: ['admin'],
|
||||
},
|
||||
},
|
||||
],
|
||||
])('does not attempt overage resolution when %s', async (_label, cfg) => {
|
||||
process.env.OPENID_REQUIRED_ROLE = 'group-required';
|
||||
process.env.OPENID_REQUIRED_ROLE_PARAMETER_PATH = cfg.path;
|
||||
process.env.OPENID_REQUIRED_ROLE_TOKEN_KIND = cfg.kind;
|
||||
|
||||
jwtDecode.mockReturnValue(cfg.decoded);
|
||||
|
||||
await setupOpenId();
|
||||
verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid');
|
||||
|
||||
const { user, details } = await validate(tokenset);
|
||||
|
||||
expect(undici.fetch).not.toHaveBeenCalled();
|
||||
expect(user).toBe(false);
|
||||
expect(details.message).toBe('You must have "group-required" role to log in.');
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
const expectedTokenKind = cfg.kind === 'access' ? 'access token' : 'id token';
|
||||
expect(logger.error).toHaveBeenCalledWith(
|
||||
expect.stringContaining(`Key '${cfg.path}' not found in ${expectedTokenKind}!`),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('resolving groups via Microsoft Graph', () => {
|
||||
it('denies login and does not call Graph when access token is missing', async () => {
|
||||
process.env.OPENID_REQUIRED_ROLE = 'group-required';
|
||||
process.env.OPENID_REQUIRED_ROLE_PARAMETER_PATH = 'groups';
|
||||
process.env.OPENID_REQUIRED_ROLE_TOKEN_KIND = 'id';
|
||||
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
|
||||
jwtDecode.mockReturnValue({
|
||||
hasgroups: true,
|
||||
permissions: ['admin'],
|
||||
});
|
||||
|
||||
await setupOpenId();
|
||||
verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid');
|
||||
|
||||
const tokensetWithoutAccess = {
|
||||
...tokenset,
|
||||
access_token: undefined,
|
||||
};
|
||||
|
||||
const { user, details } = await validate(tokensetWithoutAccess);
|
||||
|
||||
expect(user).toBe(false);
|
||||
expect(details.message).toBe('You must have "group-required" role to log in.');
|
||||
|
||||
expect(undici.fetch).not.toHaveBeenCalled();
|
||||
expect(logger.error).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Access token missing; cannot resolve group overage'),
|
||||
);
|
||||
});
|
||||
|
||||
it.each([
|
||||
[
|
||||
'Graph returns HTTP error',
|
||||
async () => ({
|
||||
ok: false,
|
||||
status: 403,
|
||||
statusText: 'Forbidden',
|
||||
json: async () => ({}),
|
||||
}),
|
||||
[
|
||||
'[openidStrategy] Failed to resolve groups via Microsoft Graph getMemberObjects: HTTP 403 Forbidden',
|
||||
],
|
||||
],
|
||||
[
|
||||
'Graph network error',
|
||||
async () => {
|
||||
throw new Error('network error');
|
||||
},
|
||||
[
|
||||
'[openidStrategy] Error resolving groups via Microsoft Graph getMemberObjects:',
|
||||
expect.any(Error),
|
||||
],
|
||||
],
|
||||
[
|
||||
'Graph returns unexpected shape (no value)',
|
||||
async () => ({
|
||||
ok: true,
|
||||
status: 200,
|
||||
statusText: 'OK',
|
||||
json: async () => ({}),
|
||||
}),
|
||||
[
|
||||
'[openidStrategy] Unexpected response format when resolving groups via Microsoft Graph getMemberObjects',
|
||||
],
|
||||
],
|
||||
[
|
||||
'Graph returns invalid value type',
|
||||
async () => ({
|
||||
ok: true,
|
||||
status: 200,
|
||||
statusText: 'OK',
|
||||
json: async () => ({ value: 'not-an-array' }),
|
||||
}),
|
||||
[
|
||||
'[openidStrategy] Unexpected response format when resolving groups via Microsoft Graph getMemberObjects',
|
||||
],
|
||||
],
|
||||
])(
|
||||
'denies login when overage resolution fails because %s',
|
||||
async (_label, setupFetch, expectedErrorArgs) => {
|
||||
process.env.OPENID_REQUIRED_ROLE = 'group-required';
|
||||
process.env.OPENID_REQUIRED_ROLE_PARAMETER_PATH = 'groups';
|
||||
process.env.OPENID_REQUIRED_ROLE_TOKEN_KIND = 'id';
|
||||
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
|
||||
jwtDecode.mockReturnValue({
|
||||
hasgroups: true,
|
||||
permissions: ['admin'],
|
||||
});
|
||||
|
||||
await setupOpenId();
|
||||
verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid');
|
||||
|
||||
undici.fetch.mockImplementation(setupFetch);
|
||||
|
||||
const { user, details } = await validate(tokenset);
|
||||
|
||||
expect(undici.fetch).toHaveBeenCalled();
|
||||
expect(user).toBe(false);
|
||||
expect(details.message).toBe('You must have "group-required" role to log in.');
|
||||
|
||||
expect(logger.error).toHaveBeenCalledWith(...expectedErrorArgs);
|
||||
},
|
||||
);
|
||||
|
||||
it.each([
|
||||
[
|
||||
'hasgroups overage and Graph contains required group',
|
||||
{
|
||||
hasgroups: true,
|
||||
},
|
||||
['group-required', 'some-other-group'],
|
||||
true,
|
||||
],
|
||||
[
|
||||
'_claim_* overage and Graph contains required group',
|
||||
{
|
||||
_claim_names: { groups: 'src1' },
|
||||
_claim_sources: { src1: { endpoint: 'https://graph.windows.net/ignored' } },
|
||||
},
|
||||
['group-required', 'some-other-group'],
|
||||
true,
|
||||
],
|
||||
[
|
||||
'hasgroups overage and Graph does NOT contain required group',
|
||||
{
|
||||
hasgroups: true,
|
||||
},
|
||||
['some-other-group'],
|
||||
false,
|
||||
],
|
||||
[
|
||||
'_claim_* overage and Graph does NOT contain required group',
|
||||
{
|
||||
_claim_names: { groups: 'src1' },
|
||||
_claim_sources: { src1: { endpoint: 'https://graph.windows.net/ignored' } },
|
||||
},
|
||||
['some-other-group'],
|
||||
false,
|
||||
],
|
||||
])(
|
||||
'resolves groups via Microsoft Graph when %s',
|
||||
async (_label, decodedTokenValue, graphGroups, expectedAllowed) => {
|
||||
process.env.OPENID_REQUIRED_ROLE = 'group-required';
|
||||
process.env.OPENID_REQUIRED_ROLE_PARAMETER_PATH = 'groups';
|
||||
process.env.OPENID_REQUIRED_ROLE_TOKEN_KIND = 'id';
|
||||
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
|
||||
jwtDecode.mockReturnValue(decodedTokenValue);
|
||||
|
||||
await setupOpenId();
|
||||
verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid');
|
||||
|
||||
undici.fetch.mockResolvedValue({
|
||||
ok: true,
|
||||
status: 200,
|
||||
statusText: 'OK',
|
||||
json: async () => ({
|
||||
value: graphGroups,
|
||||
}),
|
||||
});
|
||||
|
||||
const { user } = await validate(tokenset);
|
||||
|
||||
expect(undici.fetch).toHaveBeenCalledWith(
|
||||
'https://graph.microsoft.com/v1.0/me/getMemberObjects',
|
||||
expect.objectContaining({
|
||||
method: 'POST',
|
||||
headers: expect.objectContaining({
|
||||
Authorization: `Bearer ${tokenset.access_token}`,
|
||||
}),
|
||||
}),
|
||||
);
|
||||
expect(Boolean(user)).toBe(expectedAllowed);
|
||||
|
||||
expect(logger.debug).toHaveBeenCalledWith(
|
||||
expect.stringContaining(
|
||||
`Successfully resolved ${graphGroups.length} groups via Microsoft Graph getMemberObjects`,
|
||||
),
|
||||
);
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
it('should attempt to download and save the avatar if picture is provided', async () => {
|
||||
// Act
|
||||
const { user } = await validate(tokenset);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue