diff --git a/api/server/controllers/AuthController.js b/api/server/controllers/AuthController.js index e1d0977f1b..249817610e 100644 --- a/api/server/controllers/AuthController.js +++ b/api/server/controllers/AuthController.js @@ -1,8 +1,8 @@ const cookies = require('cookie'); const jwt = require('jsonwebtoken'); const openIdClient = require('openid-client'); -const { isEnabled } = require('@librechat/api'); const { logger } = require('@librechat/data-schemas'); +const { isEnabled, findOpenIDUser } = require('@librechat/api'); const { requestPasswordReset, setOpenIDAuthTokens, @@ -72,8 +72,14 @@ const refreshController = async (req, res) => { const openIdConfig = getOpenIdConfig(); const tokenset = await openIdClient.refreshTokenGrant(openIdConfig, refreshToken); const claims = tokenset.claims(); - const user = await findUser({ email: claims.email }); - if (!user) { + const { user, error } = await findOpenIDUser({ + findUser, + email: claims.email, + openidId: claims.sub, + idOnTheSource: claims.oid, + strategyName: 'refreshController', + }); + if (error || !user) { return res.status(401).redirect('/login'); } const token = setOpenIDAuthTokens(tokenset, res, user._id.toString()); @@ -126,7 +132,7 @@ const refreshController = async (req, res) => { res.status(401).send('Refresh token expired or not found for this user'); } } catch (err) { - logger.error(`[refreshController] Refresh token: ${refreshToken}`, err); + logger.error(`[refreshController] Invalid refresh token:`, err); res.status(403).send('Invalid refresh token'); } }; diff --git a/api/strategies/openIdJwtStrategy.js b/api/strategies/openIdJwtStrategy.js index 841b645936..94685fc86c 100644 --- a/api/strategies/openIdJwtStrategy.js +++ b/api/strategies/openIdJwtStrategy.js @@ -41,13 +41,18 @@ const openIdJwtLogin = (openIdConfig) => { jwtFromRequest: ExtractJwt.fromAuthHeaderAsBearerToken(), secretOrKeyProvider: jwksRsa.passportJwtSecret(jwksRsaOptions), }, + /** + * @param {import('openid-client').IDToken} payload + * @param {import('passport-jwt').VerifyCallback} done + */ async (payload, done) => { try { const { user, error, migration } = await findOpenIDUser({ - openidId: payload?.sub, - email: payload?.email, - strategyName: 'openIdJwtLogin', findUser, + email: payload?.email, + openidId: payload?.sub, + idOnTheSource: payload?.oid, + strategyName: 'openIdJwtLogin', }); if (error) { diff --git a/api/strategies/openidStrategy.js b/api/strategies/openidStrategy.js index 6a4e783d4a..0a396211cf 100644 --- a/api/strategies/openidStrategy.js +++ b/api/strategies/openidStrategy.js @@ -337,6 +337,10 @@ async function setupOpenId() { clockTolerance: process.env.OPENID_CLOCK_TOLERANCE || 300, usePKCE, }, + /** + * @param {import('openid-client').TokenEndpointResponseHelpers} tokenset + * @param {import('passport-jwt').VerifyCallback} done + */ async (tokenset, done) => { try { const claims = tokenset.claims(); @@ -354,10 +358,11 @@ async function setupOpenId() { } const result = await findOpenIDUser({ - openidId: claims.sub, - email: claims.email, - strategyName: 'openidStrategy', findUser, + email: claims.email, + openidId: claims.sub, + idOnTheSource: claims.oid, + strategyName: 'openidStrategy', }); let user = result.user; const error = result.error; @@ -436,6 +441,10 @@ async function setupOpenId() { user.username = username; user.name = fullName; user.idOnTheSource = userinfo.oid; + if (userinfo.email && userinfo.email !== user.email) { + user.email = userinfo.email; + user.emailVerified = userinfo.email_verified || false; + } } if (!!userinfo && userinfo.picture && !user.avatar?.includes('manual=true')) { diff --git a/api/strategies/openidStrategy.spec.js b/api/strategies/openidStrategy.spec.js index 35841e3783..e668e078de 100644 --- a/api/strategies/openidStrategy.spec.js +++ b/api/strategies/openidStrategy.spec.js @@ -274,10 +274,7 @@ describe('setupOpenId', () => { name: '', }; findUser.mockImplementation(async (query) => { - if ( - query.openidId === tokenset.claims().sub || - (query.email === tokenset.claims().email && query.provider === 'openid') - ) { + if (query.openidId === tokenset.claims().sub || query.email === tokenset.claims().email) { return existingUser; } return null; diff --git a/packages/api/src/auth/openid.spec.ts b/packages/api/src/auth/openid.spec.ts new file mode 100644 index 0000000000..032c62e580 --- /dev/null +++ b/packages/api/src/auth/openid.spec.ts @@ -0,0 +1,408 @@ +import { ErrorTypes } from 'librechat-data-provider'; +import { logger } from '@librechat/data-schemas'; +import type { IUser, UserMethods } from '@librechat/data-schemas'; +import { findOpenIDUser } from './openid'; + +jest.mock('@librechat/data-schemas', () => ({ + ...jest.requireActual('@librechat/data-schemas'), + logger: { + warn: jest.fn(), + info: jest.fn(), + }, +})); + +describe('findOpenIDUser', () => { + let mockFindUser: jest.MockedFunction; + + beforeEach(() => { + mockFindUser = jest.fn(); + jest.clearAllMocks(); + (logger.warn as jest.Mock).mockClear(); + (logger.info as jest.Mock).mockClear(); + }); + + describe('Primary condition searches', () => { + it('should find user by openidId', async () => { + const mockUser: IUser = { + _id: 'user123', + provider: 'openid', + openidId: 'openid_123', + email: 'user@example.com', + username: 'testuser', + } as IUser; + + mockFindUser.mockResolvedValueOnce(mockUser); + + const result = await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + email: 'user@example.com', + }); + + expect(mockFindUser).toHaveBeenCalledWith({ + $or: [{ openidId: 'openid_123' }], + }); + expect(result).toEqual({ + user: mockUser, + error: null, + migration: false, + }); + }); + + it('should find user by idOnTheSource', async () => { + const mockUser: IUser = { + _id: 'user123', + provider: 'openid', + idOnTheSource: 'source_123', + email: 'user@example.com', + username: 'testuser', + } as IUser; + + mockFindUser.mockResolvedValueOnce(mockUser); + + const result = await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + idOnTheSource: 'source_123', + }); + + expect(mockFindUser).toHaveBeenCalledWith({ + $or: [{ openidId: 'openid_123' }, { idOnTheSource: 'source_123' }], + }); + expect(result).toEqual({ + user: mockUser, + error: null, + migration: false, + }); + }); + + it('should find user by both openidId and idOnTheSource', async () => { + const mockUser: IUser = { + _id: 'user123', + provider: 'openid', + openidId: 'openid_123', + idOnTheSource: 'source_123', + email: 'user@example.com', + username: 'testuser', + } as IUser; + + mockFindUser.mockResolvedValueOnce(mockUser); + + const result = await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + idOnTheSource: 'source_123', + email: 'user@example.com', + }); + + expect(mockFindUser).toHaveBeenCalledWith({ + $or: [{ openidId: 'openid_123' }, { idOnTheSource: 'source_123' }], + }); + expect(result).toEqual({ + user: mockUser, + error: null, + migration: false, + }); + }); + }); + + describe('Email-based searches', () => { + it('should find user by email when primary conditions fail', async () => { + const mockUser: IUser = { + _id: 'user123', + provider: 'openid', + openidId: 'openid_456', + email: 'user@example.com', + username: 'testuser', + } as IUser; + + mockFindUser + .mockResolvedValueOnce(null) // Primary condition fails + .mockResolvedValueOnce(mockUser); // Email search succeeds + + const result = await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + email: 'user@example.com', + }); + + expect(mockFindUser).toHaveBeenNthCalledWith(1, { + $or: [{ openidId: 'openid_123' }], + }); + expect(mockFindUser).toHaveBeenNthCalledWith(2, { email: 'user@example.com' }); + expect(result).toEqual({ + user: mockUser, + error: null, + migration: false, + }); + }); + + it('should return null user when email is not found', async () => { + mockFindUser + .mockResolvedValueOnce(null) // Primary condition fails + .mockResolvedValueOnce(null); // Email search fails + + const result = await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + email: 'user@example.com', + }); + + expect(mockFindUser).toHaveBeenCalledTimes(2); + expect(result).toEqual({ + user: null, + error: null, + migration: false, + }); + }); + + it('should not search by email if not provided', async () => { + mockFindUser.mockResolvedValueOnce(null); + + const result = await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + }); + + expect(mockFindUser).toHaveBeenCalledTimes(1); + expect(mockFindUser).toHaveBeenCalledWith({ + $or: [{ openidId: 'openid_123' }], + }); + expect(result).toEqual({ + user: null, + error: null, + migration: false, + }); + }); + }); + + describe('Provider conflict handling', () => { + it('should return error when user has different provider', async () => { + const mockUser: IUser = { + _id: 'user123', + provider: 'google', + email: 'user@example.com', + username: 'testuser', + } as IUser; + + mockFindUser + .mockResolvedValueOnce(null) // Primary condition fails + .mockResolvedValueOnce(mockUser); // Email search finds user with different provider + + const result = await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + email: 'user@example.com', + }); + + expect(result).toEqual({ + user: null, + error: ErrorTypes.AUTH_FAILED, + migration: false, + }); + }); + + it('should allow login when user has openid provider', async () => { + const mockUser: IUser = { + _id: 'user123', + provider: 'openid', + openidId: 'openid_456', + email: 'user@example.com', + username: 'testuser', + } as IUser; + + mockFindUser + .mockResolvedValueOnce(null) // Primary condition fails + .mockResolvedValueOnce(mockUser); // Email search finds user with openid provider + + const result = await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + email: 'user@example.com', + }); + + expect(result).toEqual({ + user: mockUser, + error: null, + migration: false, + }); + }); + }); + + describe('User migration scenarios', () => { + it('should prepare user for migration when email exists without openidId', async () => { + const mockUser: IUser = { + _id: 'user123', + email: 'user@example.com', + username: 'testuser', + // No provider and no openidId - needs migration + } as IUser; + + mockFindUser + .mockResolvedValueOnce(null) // Primary condition fails + .mockResolvedValueOnce(mockUser); // Email search finds user without openidId + + const result = await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + email: 'user@example.com', + }); + + expect(result).toEqual({ + user: { + ...mockUser, + provider: 'openid', + openidId: 'openid_123', + }, + error: null, + migration: true, + }); + }); + + it('should not migrate user who already has openidId', async () => { + const mockUser: IUser = { + _id: 'user123', + provider: 'openid', + openidId: 'existing_openid', + email: 'user@example.com', + username: 'testuser', + } as IUser; + + mockFindUser + .mockResolvedValueOnce(null) // Primary condition fails + .mockResolvedValueOnce(mockUser); // Email search finds user with existing openidId + + const result = await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + email: 'user@example.com', + }); + + expect(result).toEqual({ + user: mockUser, + error: null, + migration: false, + }); + }); + + it('should handle user with no provider but existing openidId', async () => { + const mockUser: IUser = { + _id: 'user123', + openidId: 'existing_openid', + email: 'user@example.com', + username: 'testuser', + // No provider field + } as IUser; + + mockFindUser + .mockResolvedValueOnce(null) // Primary condition fails + .mockResolvedValueOnce(mockUser); // Email search finds user + + const result = await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + email: 'user@example.com', + }); + + expect(result).toEqual({ + user: mockUser, + error: null, + migration: false, + }); + }); + }); + + describe('Custom strategy names', () => { + it('should use custom strategy name in logs', async () => { + const loggerWarn = logger.warn as jest.Mock; + loggerWarn.mockClear(); + + mockFindUser.mockResolvedValueOnce(null).mockResolvedValueOnce(null); + + await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + email: 'user@example.com', + strategyName: 'customStrategy', + }); + + expect(loggerWarn).toHaveBeenCalledWith(expect.stringContaining('[customStrategy]')); + }); + + it('should default to openid strategy name', async () => { + const loggerWarn = logger.warn as jest.Mock; + loggerWarn.mockClear(); + + mockFindUser.mockResolvedValueOnce(null).mockResolvedValueOnce(null); + + await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + email: 'user@example.com', + }); + + expect(loggerWarn).toHaveBeenCalledWith(expect.stringContaining('[openid]')); + }); + }); + + describe('Edge cases', () => { + it('should handle empty string openidId', async () => { + mockFindUser.mockResolvedValueOnce(null); + + const result = await findOpenIDUser({ + openidId: '', + findUser: mockFindUser, + }); + + expect(mockFindUser).not.toHaveBeenCalled(); + expect(result).toEqual({ + user: null, + error: null, + migration: false, + }); + }); + + it('should handle empty string idOnTheSource', async () => { + mockFindUser.mockResolvedValueOnce(null); + + const result = await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + idOnTheSource: '', + }); + + expect(mockFindUser).toHaveBeenCalledWith({ + $or: [{ openidId: 'openid_123' }], + }); + expect(result).toEqual({ + user: null, + error: null, + migration: false, + }); + }); + + it('should handle both openidId and idOnTheSource as empty strings', async () => { + await findOpenIDUser({ + openidId: '', + findUser: mockFindUser, + idOnTheSource: '', + email: 'user@example.com', + }); + + // Should skip primary search and go directly to email search + expect(mockFindUser).toHaveBeenCalledTimes(1); + expect(mockFindUser).toHaveBeenCalledWith({ email: 'user@example.com' }); + }); + + it('should handle findUser throwing an error', async () => { + mockFindUser.mockRejectedValueOnce(new Error('Database error')); + + await expect( + findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + }), + ).rejects.toThrow('Database error'); + }); + }); +}); diff --git a/packages/api/src/auth/openid.ts b/packages/api/src/auth/openid.ts index 0c5e0d0a4f..a7079ccd16 100644 --- a/packages/api/src/auth/openid.ts +++ b/packages/api/src/auth/openid.ts @@ -1,4 +1,5 @@ import { logger } from '@librechat/data-schemas'; +import { ErrorTypes } from 'librechat-data-provider'; import type { IUser, UserMethods } from '@librechat/data-schemas'; /** @@ -7,16 +8,31 @@ import type { IUser, UserMethods } from '@librechat/data-schemas'; */ export async function findOpenIDUser({ openidId, - email, findUser, + email, + idOnTheSource, strategyName = 'openid', }: { openidId: string; findUser: UserMethods['findUser']; email?: string; + idOnTheSource?: string; strategyName?: string; }): Promise<{ user: IUser | null; error: string | null; migration: boolean }> { - let user = await findUser({ openidId }); + const primaryConditions = []; + + if (openidId && typeof openidId === 'string') { + primaryConditions.push({ openidId }); + } + + if (idOnTheSource && typeof idOnTheSource === 'string') { + primaryConditions.push({ idOnTheSource }); + } + + let user = null; + if (primaryConditions.length > 0) { + user = await findUser({ $or: primaryConditions }); + } if (!user && email) { user = await findUser({ email }); logger.warn( @@ -28,7 +44,7 @@ export async function findOpenIDUser({ logger.warn( `[${strategyName}] Attempted OpenID login by user ${user.email}, was registered with "${user.provider}" provider`, ); - return { user: null, error: 'AUTH_FAILED', migration: false }; + return { user: null, error: ErrorTypes.AUTH_FAILED, migration: false }; } // If user found by email but doesn't have openidId, prepare for migration