🆔 fix: Prioritize Immutable Sub Claim for OIDC User ID (#9788)

* add use of immutable claims to identify user object

* fix semicolons

* update email attribute on change

* replace ternary expressions

* fix semicolon

* chore: add typing

* chore: reorder fields in `findOpenIDUser`

* refactor: optimize user lookup logic in `findOpenIDUser` function to minimize database roundtrips

* refactor: integrate findOpenIDUser for improved user retrieval in refreshController

* refactor: improve error logging for invalid refresh tokens in refreshController

* ci: mock findUser correctly in openidStrategy tests

* test: add unit tests for findOpenIDUser function to enhance user retrieval logic

---------

Co-authored-by: Joachim Keltsch <joachim.keltsch@daimlertruck.com>

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');
   }
 };

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) {

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')) {

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;

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<UserMethods['findUser']>;
+
+  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');
+    });
+  });
+});

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