mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-02-28 05:14:08 +01:00
🪪 feat: Add OPENID_EMAIL_CLAIM for Configurable OpenID User Identifier (#11699)
* Allow setting the claim field to be used when OpenID login is configured * fix(openid): harden getOpenIdEmail and expand test coverage Guard against non-string claim values in getOpenIdEmail to prevent a TypeError crash in isEmailDomainAllowed when domain restrictions are configured. Improve warning messages to name the fallback chain explicitly and distinguish missing vs. non-string claim values. Fix the domain-block error log to record the resolved identifier rather than userinfo.email, which was misleading when OPENID_EMAIL_CLAIM resolved to a different field (e.g. upn). Fix a latent test defect in openIdJwtStrategy.spec.js where the ~/server/services/Config mock exported getCustomConfig instead of getAppConfig, the symbol actually consumed by openidStrategy.js. Add refreshController tests covering the OPENID_EMAIL_CLAIM paths, which were previously untested despite being a stated fix target. Expand JWT strategy tests with null-payload, empty/whitespace OPENID_EMAIL_CLAIM, migration-via-preferred_username, and call-order assertions for the findUser lookup sequence. * test(auth): enhance AuthController and openIdJwtStrategy tests for openidId updates Added a new test in AuthController to verify that the openidId is updated correctly when a migration is triggered during the refresh process. Expanded the openIdJwtStrategy tests to include assertions for the updateUser function, ensuring that the correct parameters are passed when a user is found with a legacy email. This improves test coverage for OpenID-related functionality. --------- Co-authored-by: Danny Avila <danny@librechat.ai>
This commit is contained in:
parent
e978a934fc
commit
13df8ed67c
8 changed files with 447 additions and 13 deletions
|
|
@ -18,7 +18,7 @@ const {
|
|||
findUser,
|
||||
} = require('~/models');
|
||||
const { getGraphApiToken } = require('~/server/services/GraphTokenService');
|
||||
const { getOpenIdConfig } = require('~/strategies');
|
||||
const { getOpenIdConfig, getOpenIdEmail } = require('~/strategies');
|
||||
|
||||
const registrationController = async (req, res) => {
|
||||
try {
|
||||
|
|
@ -87,7 +87,7 @@ const refreshController = async (req, res) => {
|
|||
const claims = tokenset.claims();
|
||||
const { user, error, migration } = await findOpenIDUser({
|
||||
findUser,
|
||||
email: claims.email,
|
||||
email: getOpenIdEmail(claims),
|
||||
openidId: claims.sub,
|
||||
idOnTheSource: claims.oid,
|
||||
strategyName: 'refreshController',
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
jest.mock('@librechat/data-schemas', () => ({
|
||||
logger: { error: jest.fn(), debug: jest.fn(), warn: jest.fn() },
|
||||
logger: { error: jest.fn(), debug: jest.fn(), warn: jest.fn(), info: jest.fn() },
|
||||
}));
|
||||
jest.mock('~/server/services/GraphTokenService', () => ({
|
||||
getGraphApiToken: jest.fn(),
|
||||
|
|
@ -11,7 +11,8 @@ jest.mock('~/server/services/AuthService', () => ({
|
|||
setAuthTokens: jest.fn(),
|
||||
registerUser: jest.fn(),
|
||||
}));
|
||||
jest.mock('~/strategies', () => ({ getOpenIdConfig: jest.fn() }));
|
||||
jest.mock('~/strategies', () => ({ getOpenIdConfig: jest.fn(), getOpenIdEmail: jest.fn() }));
|
||||
jest.mock('openid-client', () => ({ refreshTokenGrant: jest.fn() }));
|
||||
jest.mock('~/models', () => ({
|
||||
deleteAllUserSessions: jest.fn(),
|
||||
getUserById: jest.fn(),
|
||||
|
|
@ -24,9 +25,13 @@ jest.mock('@librechat/api', () => ({
|
|||
findOpenIDUser: jest.fn(),
|
||||
}));
|
||||
|
||||
const { isEnabled } = require('@librechat/api');
|
||||
const openIdClient = require('openid-client');
|
||||
const { isEnabled, findOpenIDUser } = require('@librechat/api');
|
||||
const { graphTokenController, refreshController } = require('./AuthController');
|
||||
const { getGraphApiToken } = require('~/server/services/GraphTokenService');
|
||||
const { graphTokenController } = require('./AuthController');
|
||||
const { setOpenIDAuthTokens } = require('~/server/services/AuthService');
|
||||
const { getOpenIdConfig, getOpenIdEmail } = require('~/strategies');
|
||||
const { updateUser } = require('~/models');
|
||||
|
||||
describe('graphTokenController', () => {
|
||||
let req, res;
|
||||
|
|
@ -142,3 +147,156 @@ describe('graphTokenController', () => {
|
|||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('refreshController – OpenID path', () => {
|
||||
const mockTokenset = {
|
||||
claims: jest.fn(),
|
||||
access_token: 'new-access',
|
||||
id_token: 'new-id',
|
||||
refresh_token: 'new-refresh',
|
||||
};
|
||||
|
||||
const baseClaims = {
|
||||
sub: 'oidc-sub-123',
|
||||
oid: 'oid-456',
|
||||
email: 'user@example.com',
|
||||
exp: 9999999999,
|
||||
};
|
||||
|
||||
let req, res;
|
||||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
|
||||
isEnabled.mockReturnValue(true);
|
||||
getOpenIdConfig.mockReturnValue({ some: 'config' });
|
||||
openIdClient.refreshTokenGrant.mockResolvedValue(mockTokenset);
|
||||
mockTokenset.claims.mockReturnValue(baseClaims);
|
||||
getOpenIdEmail.mockReturnValue(baseClaims.email);
|
||||
setOpenIDAuthTokens.mockReturnValue('new-app-token');
|
||||
updateUser.mockResolvedValue({});
|
||||
|
||||
req = {
|
||||
headers: { cookie: 'token_provider=openid; refreshToken=stored-refresh' },
|
||||
session: {},
|
||||
};
|
||||
|
||||
res = {
|
||||
status: jest.fn().mockReturnThis(),
|
||||
send: jest.fn().mockReturnThis(),
|
||||
redirect: jest.fn(),
|
||||
};
|
||||
});
|
||||
|
||||
it('should call getOpenIdEmail with token claims and use result for findOpenIDUser', async () => {
|
||||
const user = {
|
||||
_id: 'user-db-id',
|
||||
email: baseClaims.email,
|
||||
openidId: baseClaims.sub,
|
||||
};
|
||||
findOpenIDUser.mockResolvedValue({ user, error: null, migration: false });
|
||||
|
||||
await refreshController(req, res);
|
||||
|
||||
expect(getOpenIdEmail).toHaveBeenCalledWith(baseClaims);
|
||||
expect(findOpenIDUser).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ email: baseClaims.email }),
|
||||
);
|
||||
expect(res.status).toHaveBeenCalledWith(200);
|
||||
});
|
||||
|
||||
it('should use OPENID_EMAIL_CLAIM-resolved value when claim is present in token', async () => {
|
||||
const claimsWithUpn = { ...baseClaims, upn: 'user@corp.example.com' };
|
||||
mockTokenset.claims.mockReturnValue(claimsWithUpn);
|
||||
getOpenIdEmail.mockReturnValue('user@corp.example.com');
|
||||
|
||||
const user = {
|
||||
_id: 'user-db-id',
|
||||
email: 'user@corp.example.com',
|
||||
openidId: baseClaims.sub,
|
||||
};
|
||||
findOpenIDUser.mockResolvedValue({ user, error: null, migration: false });
|
||||
|
||||
await refreshController(req, res);
|
||||
|
||||
expect(getOpenIdEmail).toHaveBeenCalledWith(claimsWithUpn);
|
||||
expect(findOpenIDUser).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ email: 'user@corp.example.com' }),
|
||||
);
|
||||
expect(res.status).toHaveBeenCalledWith(200);
|
||||
});
|
||||
|
||||
it('should fall back to claims.email when configured claim is absent from token claims', async () => {
|
||||
getOpenIdEmail.mockReturnValue(baseClaims.email);
|
||||
|
||||
const user = {
|
||||
_id: 'user-db-id',
|
||||
email: baseClaims.email,
|
||||
openidId: baseClaims.sub,
|
||||
};
|
||||
findOpenIDUser.mockResolvedValue({ user, error: null, migration: false });
|
||||
|
||||
await refreshController(req, res);
|
||||
|
||||
expect(findOpenIDUser).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ email: baseClaims.email }),
|
||||
);
|
||||
});
|
||||
|
||||
it('should update openidId when migration is triggered on refresh', async () => {
|
||||
const user = { _id: 'user-db-id', email: baseClaims.email, openidId: null };
|
||||
findOpenIDUser.mockResolvedValue({ user, error: null, migration: true });
|
||||
|
||||
await refreshController(req, res);
|
||||
|
||||
expect(updateUser).toHaveBeenCalledWith(
|
||||
'user-db-id',
|
||||
expect.objectContaining({ provider: 'openid', openidId: baseClaims.sub }),
|
||||
);
|
||||
expect(res.status).toHaveBeenCalledWith(200);
|
||||
});
|
||||
|
||||
it('should return 401 and redirect to /login when findOpenIDUser returns no user', async () => {
|
||||
findOpenIDUser.mockResolvedValue({ user: null, error: null, migration: false });
|
||||
|
||||
await refreshController(req, res);
|
||||
|
||||
expect(res.status).toHaveBeenCalledWith(401);
|
||||
expect(res.redirect).toHaveBeenCalledWith('/login');
|
||||
});
|
||||
|
||||
it('should return 401 and redirect when findOpenIDUser returns an error', async () => {
|
||||
findOpenIDUser.mockResolvedValue({ user: null, error: 'AUTH_FAILED', migration: false });
|
||||
|
||||
await refreshController(req, res);
|
||||
|
||||
expect(res.status).toHaveBeenCalledWith(401);
|
||||
expect(res.redirect).toHaveBeenCalledWith('/login');
|
||||
});
|
||||
|
||||
it('should skip OpenID path when token_provider is not openid', async () => {
|
||||
req.headers.cookie = 'token_provider=local; refreshToken=some-token';
|
||||
|
||||
await refreshController(req, res);
|
||||
|
||||
expect(openIdClient.refreshTokenGrant).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should skip OpenID path when OPENID_REUSE_TOKENS is disabled', async () => {
|
||||
isEnabled.mockReturnValue(false);
|
||||
|
||||
await refreshController(req, res);
|
||||
|
||||
expect(openIdClient.refreshTokenGrant).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should return 200 with token not provided when refresh token is absent', async () => {
|
||||
req.headers.cookie = 'token_provider=openid';
|
||||
req.session = {};
|
||||
|
||||
await refreshController(req, res);
|
||||
|
||||
expect(res.status).toHaveBeenCalledWith(200);
|
||||
expect(res.send).toHaveBeenCalledWith('Refresh token not provided');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue