🔁 fix: Recover Concurrent First SAML, LDAP and Social Logins

api/strategies/ldapStrategy.js

@@ -1,14 +1,22 @@
 const fs = require('fs');
 const LdapStrategy = require('passport-ldapauth');
 const { logger } = require('@librechat/data-schemas');
-const { SystemRoles, ErrorTypes } = require('librechat-data-provider');
+const { ErrorTypes } = require('librechat-data-provider');
 const {
   isEnabled,
+  findLdapUser,
   getBalanceConfig,
+  provisionLdapUser,
   isEmailDomainAllowed,
   resolveAppConfigForUser,
 } = require('@librechat/api');
-const { createUser, findUser, updateUser, countUsers } = require('~/models');
+const {
+  findUser,
+  updateUser,
+  countUsers,
+  findBalanceByUser,
+  createUserIfAbsent,
+} = require('~/models');
 const { getAppConfig } = require('~/server/services/Config');
 
 const {
@@ -128,18 +136,13 @@ const ldapLogin = new LdapStrategy(ldapOptions, async (userinfo, done) => {
       return done(null, false, { message: 'Email domain not allowed' });
     }
 
-    let user = await findUser({ ldapId });
-    if (user && user.provider !== 'ldap') {
-      logger.info(
-        `[ldapStrategy] User ${user.email} already exists with provider ${user.provider}`,
-      );
-      return done(null, false, {
-        message: ErrorTypes.AUTH_FAILED,
-      });
+    const found = await findLdapUser({ findUser, ldapId });
+    if (found.error) {
+      return done(null, false, { message: ErrorTypes.AUTH_FAILED });
     }
 
-    const appConfig = user?.tenantId
-      ? await resolveAppConfigForUser(getAppConfig, user)
+    const appConfig = found.user?.tenantId
+      ? await resolveAppConfigForUser(getAppConfig, found.user)
       : baseConfig;
 
     if (!isEmailDomainAllowed(mail, appConfig?.registration?.allowedDomains)) {
@@ -149,33 +152,25 @@ const ldapLogin = new LdapStrategy(ldapOptions, async (userinfo, done) => {
       return done(null, false, { message: 'Email domain not allowed' });
     }
 
-    if (!user) {
-      const isFirstRegisteredUser = (await countUsers()) === 0;
-      const role = isFirstRegisteredUser ? SystemRoles.ADMIN : SystemRoles.USER;
-
-      user = {
-        provider: 'ldap',
-        ldapId,
-        username,
-        email: mail,
-        emailVerified: true, // The ldap server administrator should verify the email
-        name: fullName,
-        role,
-      };
-      const balanceConfig = getBalanceConfig(appConfig);
-      const userId = await createUser(user, balanceConfig);
-      user._id = userId;
-    } else {
-      // Users registered in LDAP are assumed to have their user information managed in LDAP,
-      // so update the user information with the values registered in LDAP
-      user.provider = 'ldap';
-      user.ldapId = ldapId;
-      user.email = mail;
-      user.username = username;
-      user.name = fullName;
+    const provisioned = await provisionLdapUser({
+      user: found.user,
+      ldapId,
+      email: mail,
+      username,
+      name: fullName,
+      appConfig,
+      getBalanceConfig,
+      getAppConfig,
+      findUser,
+      countUsers,
+      createUserIfAbsent,
+      findBalanceByUser,
+    });
+    if (provisioned.error) {
+      return done(null, false, { message: provisioned.error });
     }
 
-    user = await updateUser(user._id, user);
+    const user = await updateUser(provisioned.user._id, provisioned.user);
     done(null, user);
   } catch (err) {
     logger.error('[ldapStrategy]', err);

api/strategies/ldapStrategy.spec.js

@@ -6,9 +6,12 @@ jest.mock('@librechat/data-schemas', () => ({
     debug: jest.fn(),
     error: jest.fn(),
   },
+  tenantStorage: { run: (_store, fn) => fn() },
 }));
 
 jest.mock('@librechat/api', () => ({
+  findLdapUser: jest.requireActual('@librechat/api').findLdapUser,
+  provisionLdapUser: jest.requireActual('@librechat/api').provisionLdapUser,
   isEnabled: jest.fn(() => false),
   isEmailDomainAllowed: jest.fn(() => true),
   getBalanceConfig: jest.fn(() => ({ enabled: false })),
@@ -17,9 +20,10 @@ jest.mock('@librechat/api', () => ({
 
 jest.mock('~/models', () => ({
   findUser: jest.fn(),
-  createUser: jest.fn(),
+  createUserIfAbsent: jest.fn(),
   updateUser: jest.fn(),
   countUsers: jest.fn(),
+  findBalanceByUser: jest.fn(),
 }));
 
 jest.mock('~/server/services/Config', () => ({
@@ -37,7 +41,7 @@ jest.mock('passport-ldapauth', () => {
 
 const { ErrorTypes } = require('librechat-data-provider');
 const { isEmailDomainAllowed, resolveAppConfigForUser } = require('@librechat/api');
-const { findUser, createUser, updateUser, countUsers } = require('~/models');
+const { findUser, updateUser, countUsers, createUserIfAbsent } = require('~/models');
 const { getAppConfig } = require('~/server/services/Config');
 
 // Helper to call the verify callback and wrap in a Promise for convenience
@@ -68,7 +72,9 @@ describe('ldapStrategy', () => {
 
     // Default model/domain mocks
     findUser.mockReset().mockResolvedValue(null);
-    createUser.mockReset().mockResolvedValue('newUserId');
+    createUserIfAbsent
+      .mockReset()
+      .mockImplementation(async (data) => ({ ok: true, value: { _id: 'newUserId', ...data } }));
     updateUser.mockReset().mockImplementation(async (id, user) => ({ _id: id, ...user }));
     countUsers.mockReset().mockResolvedValue(0);
     isEmailDomainAllowed.mockReset().mockReturnValue(true);
@@ -90,7 +96,7 @@ describe('ldapStrategy', () => {
     const { user } = await callVerify(userinfo);
 
     expect(user.email).toBe('first@example.com');
-    expect(createUser).toHaveBeenCalledWith(
+    expect(createUserIfAbsent).toHaveBeenCalledWith(
       expect.objectContaining({
         provider: 'ldap',
         ldapId: 'uid123',
@@ -117,7 +123,7 @@ describe('ldapStrategy', () => {
 
     expect(user).toBe(false);
     expect(info).toEqual({ message: ErrorTypes.AUTH_FAILED });
-    expect(createUser).not.toHaveBeenCalled();
+    expect(createUserIfAbsent).not.toHaveBeenCalled();
     expect(resolveAppConfigForUser).not.toHaveBeenCalled();
   });
 
@@ -141,7 +147,7 @@ describe('ldapStrategy', () => {
 
     const { user } = await callVerify(userinfo);
 
-    expect(createUser).not.toHaveBeenCalled();
+    expect(createUserIfAbsent).not.toHaveBeenCalled();
     expect(updateUser).toHaveBeenCalledWith(
       'u2',
       expect.objectContaining({
@@ -243,4 +249,80 @@ describe('ldapStrategy', () => {
     expect(user).toBe(false);
     expect(info).toEqual({ message: 'Email domain not allowed' });
   });
+
+  describe('concurrent first login', () => {
+    const userinfo = {
+      uid: 'uid-race',
+      mail: 'race@example.com',
+      givenName: 'Race',
+      cn: 'Race User',
+    };
+
+    const raceCreateWith = (existingUser) => {
+      let created = false;
+      findUser.mockImplementation(async (query) =>
+        created && query.ldapId === existingUser.ldapId ? existingUser : null,
+      );
+      createUserIfAbsent.mockImplementation(async () => {
+        created = true;
+        return { ok: false, error: { code: 'user_exists' } };
+      });
+    };
+
+    it("continues as the other request's account with this login's LDAP values", async () => {
+      raceCreateWith({
+        _id: 'winner-id',
+        provider: 'ldap',
+        ldapId: 'uid-race',
+        email: 'old@example.com',
+        username: 'old-username',
+        name: 'Old Name',
+        role: 'USER',
+      });
+
+      const { user } = await callVerify(userinfo);
+
+      expect(createUserIfAbsent).toHaveBeenCalledTimes(1);
+      expect(updateUser).toHaveBeenCalledWith(
+        'winner-id',
+        expect.objectContaining({
+          ldapId: 'uid-race',
+          email: 'race@example.com',
+          username: 'Race',
+          name: 'Race User',
+          role: 'USER',
+        }),
+      );
+      expect(user).toEqual(expect.objectContaining({ _id: 'winner-id' }));
+    });
+
+    it("fails the login when the recovered tenant account's policy rejects the email", async () => {
+      getAppConfig.mockImplementation(async (options) =>
+        options?.tenantId ? { registration: { allowedDomains: ['other.example'] } } : {},
+      );
+      raceCreateWith({
+        _id: 'tenant-id',
+        provider: 'ldap',
+        ldapId: 'uid-race',
+        tenantId: 'tenant-a',
+      });
+
+      const { user, info } = await callVerify(userinfo);
+
+      expect(user).toBe(false);
+      expect(info).toEqual({ message: 'Email domain not allowed' });
+      expect(updateUser).not.toHaveBeenCalled();
+      getAppConfig.mockResolvedValue({});
+    });
+
+    it('fails the login when the recovered account belongs to another provider', async () => {
+      raceCreateWith({ _id: 'google-id', provider: 'google', ldapId: 'uid-race' });
+
+      const { user, info } = await callVerify(userinfo);
+
+      expect(user).toBe(false);
+      expect(info).toEqual({ message: ErrorTypes.AUTH_FAILED });
+      expect(updateUser).not.toHaveBeenCalled();
+    });
+  });
 });

api/strategies/process.js

@@ -1,8 +1,9 @@
-const { getBalanceConfig } = require('@librechat/api');
 const { FileSources } = require('librechat-data-provider');
+const { getBalanceConfig, provisionSocialUser } = require('@librechat/api');
 const { getStrategyFunctions } = require('~/server/services/Files/strategies');
 const { resizeAvatar } = require('~/server/services/Files/images/avatar');
-const { updateUser, createUser, getUserById } = require('~/models');
+const { getAppConfig } = require('~/server/services/Config');
+const { updateUser, getUserById, findBalanceByUser, createUserIfAbsent } = require('~/models');
 
 /**
  * Updates the avatar URL and email of an existing user. If the user's avatar URL does not include the query parameter
@@ -55,10 +56,41 @@ const handleExistingUser = async (oldUser, avatarUrl, appConfig, email) => {
   }
 };
 
+/**
+ * Processes a newly created social user's avatar through the file strategy (a local strategy keeps
+ * the provider URL) and returns the stored user.
+ *
+ * @param {string} newUserId - The new user's id.
+ * @param {string} avatarUrl - The provider's avatar URL.
+ * @param {AppConfig} appConfig - The application configuration object.
+ * @returns {Promise<User>}
+ */
+const finishNewSocialUser = async (newUserId, avatarUrl, appConfig) => {
+  const fileStrategy = appConfig?.fileStrategy ?? process.env.CDN_PROVIDER;
+  const isLocal = fileStrategy === FileSources.local;
+
+  if (avatarUrl && !isLocal) {
+    const resizedBuffer = await resizeAvatar({
+      userId: newUserId,
+      input: avatarUrl,
+    });
+    const { processAvatar } = getStrategyFunctions(fileStrategy);
+    const avatar = await processAvatar({
+      buffer: resizedBuffer,
+      userId: newUserId,
+      manual: 'false',
+    });
+    await updateUser(newUserId, { avatar });
+  }
+
+  return await getUserById(newUserId);
+};
+
 /**
  * Creates a new user with the provided user details. If the file strategy is not local, the avatar URL is
  * processed using the specified file strategy. The new user is saved to the database with the processed or
- * original avatar URL.
+ * original avatar URL. When a concurrent first login created the account first, that account is handled as
+ * an existing user instead.
  *
  * @param {Object} params - The parameters object for user creation.
  * @param {string} params.email - The email of the new user.
@@ -70,6 +102,7 @@ const handleExistingUser = async (oldUser, avatarUrl, appConfig, email) => {
  * @param {string} params.name - The name of the new user.
  * @param {AppConfig} appConfig - The application configuration object.
  * @param {boolean} [params.emailVerified=false] - Optional. Indicates whether the user's email is verified. Defaults to false.
+ * @param {SocialUserLookup} params.lookup - The login's lookup, repeated when the insert loses a concurrent first login.
  *
  * @returns {Promise<User>}
  *          A promise that resolves to the newly created user object.
@@ -86,38 +119,28 @@ const createSocialUser = async ({
   name,
   appConfig,
   emailVerified,
-}) => {
-  const update = {
-    email,
-    avatar: avatarUrl,
-    provider,
-    [providerKey]: providerId,
-    username,
-    name,
-    emailVerified,
-  };
-
-  const balanceConfig = getBalanceConfig(appConfig);
-  const newUserId = await createUser(update, balanceConfig);
-  const fileStrategy = appConfig?.fileStrategy ?? process.env.CDN_PROVIDER;
-  const isLocal = fileStrategy === FileSources.local;
-
-  if (avatarUrl && !isLocal) {
-    const resizedBuffer = await resizeAvatar({
-      userId: newUserId,
-      input: avatarUrl,
-    });
-    const { processAvatar } = getStrategyFunctions(fileStrategy);
-    const avatar = await processAvatar({
-      buffer: resizedBuffer,
-      userId: newUserId,
-      manual: 'false',
-    });
-    await updateUser(newUserId, { avatar });
-  }
-
-  return await getUserById(newUserId);
-};
+  lookup,
+}) =>
+  provisionSocialUser({
+    lookup,
+    newUser: {
+      email,
+      avatar: avatarUrl,
+      provider,
+      [providerKey]: providerId,
+      username,
+      name,
+      emailVerified,
+    },
+    appConfig,
+    getBalanceConfig,
+    getAppConfig,
+    createUserIfAbsent,
+    findBalanceByUser,
+    finishNewUser: (user) => finishNewSocialUser(user._id, avatarUrl, appConfig),
+    refreshExistingUser: (user, accountConfig) =>
+      handleExistingUser(user, avatarUrl, accountConfig, email),
+  });
 
 module.exports = {
   handleExistingUser,