🚪 fix: Let Concurrent First OpenID Logins Continue as the New User

api/strategies/openidStrategy.js

@@ -13,9 +13,11 @@ const {
   findOpenIDUser,
   getOpenIdEmail,
   getOpenIdIssuer,
+  createOpenIDUser,
   getBalanceConfig,
   selectOpenIdRole,
   getTokenCacheTtlMs,
+  applyOpenIDProfile,
   getAvatarSaveParams,
   isEmailDomainAllowed,
   getAvatarFileStrategy,
@@ -28,7 +30,13 @@ const {
 } = require('@librechat/api');
 const { getStrategyFunctions } = require('~/server/services/Files/strategies');
 const { resizeAvatar } = require('~/server/services/Files/images/avatar');
-const { findUser, createUser, updateUser, findRolesByNames } = require('~/models');
+const {
+  findUser,
+  updateUser,
+  findRolesByNames,
+  findBalanceByUser,
+  createUserIfAbsent,
+} = require('~/models');
 const { getAppConfig } = require('~/server/services/Config');
 const getLogStores = require('~/cache/getLogStores');
 
@@ -580,22 +588,23 @@ async function processOpenIDAuth(tokenset, existingUsersOnly = false) {
     throw new Error('Email domain not allowed');
   }
 
-  const result = await findOpenIDUser({
+  const lookup = {
     findUser,
     email: email,
     openidId: claims.sub || userinfo.sub,
     openidIssuer,
     idOnTheSource: claims.oid || userinfo.oid,
     strategyName: 'openidStrategy',
-  });
+  };
+  const result = await findOpenIDUser(lookup);
   let user = result.user;
   const error = result.error;
 
   if (error) {
     throw new Error(ErrorTypes.AUTH_FAILED);
   }
 
-  const appConfig = user?.tenantId ? await resolveAppConfigForUser(getAppConfig, user) : baseConfig;
+  let appConfig = user?.tenantId ? await resolveAppConfigForUser(getAppConfig, user) : baseConfig;
 
   if (!isEmailDomainAllowed(email, appConfig?.registration?.allowedDomains)) {
     logger.error(
@@ -676,33 +685,28 @@ async function processOpenIDAuth(tokenset, existingUsersOnly = false) {
     throw new Error('User does not exist');
   }
 
-  if (!user) {
-    user = {
-      provider: 'openid',
-      openidId: userinfo.sub,
-      username,
-      email: email || '',
-      emailVerified: userinfo.email_verified || false,
-      name: fullName,
-      idOnTheSource: userinfo.oid,
-      openidIssuer,
-    };
+  const profile = {
+    openidId: userinfo.sub,
+    openidIssuer,
+    username,
+    name: fullName,
+    email,
+    emailVerified: userinfo.email_verified || false,
+    idOnTheSource: userinfo.oid,
+  };
 
-    const balanceConfig = getBalanceConfig(appConfig);
-    user = await createUser(user, balanceConfig, true, true);
+  if (!user) {
+    ({ user, appConfig } = await createOpenIDUser({
+      lookup,
+      profile,
+      appConfig,
+      getAppConfig,
+      getBalanceConfig,
+      createUserIfAbsent,
+      findBalanceByUser,
+    }));
   } else {
-    user.provider = 'openid';
-    user.openidId = userinfo.sub;
-    if (openidIssuer) {
-      user.openidIssuer = openidIssuer;
-    }
-    user.username = username;
-    user.name = fullName;
-    user.idOnTheSource = userinfo.oid;
-    if (email && email !== user.email) {
-      user.email = email;
-      user.emailVerified = userinfo.email_verified || false;
-    }
+    user = applyOpenIDProfile(user, profile);
   }
 
   const adminRole = process.env.OPENID_ADMIN_ROLE;

api/strategies/openidStrategy.spec.js

@@ -2,7 +2,7 @@ const undici = require('undici');
 const fetch = require('node-fetch');
 const jwtDecode = require('jsonwebtoken/decode');
 const { ErrorTypes, FileSources } = require('librechat-data-provider');
-const { findUser, createUser, updateUser, findRolesByNames } = require('~/models');
+const { findUser, updateUser, findRolesByNames, createUserIfAbsent } = require('~/models');
 const {
   getOpenIdProxyDispatcher,
   resolveAppConfigForUser,
@@ -97,9 +97,10 @@ jest.mock('@librechat/api', () => {
 });
 jest.mock('~/models', () => ({
   findUser: jest.fn(),
-  createUser: jest.fn(),
+  createUserIfAbsent: jest.fn(),
   updateUser: jest.fn(),
   findRolesByNames: jest.fn(),
+  findBalanceByUser: jest.fn(),
 }));
 jest.mock('@librechat/data-schemas', () => ({
   ...jest.requireActual('@librechat/api'),
@@ -264,11 +265,11 @@ describe('setupOpenId', () => {
       permissions: ['admin'],
     });
 
-    // By default, assume that no user is found, so createUser will be called
+    // By default, assume that no user is found, so createUserIfAbsent will be called
     findUser.mockResolvedValue(null);
-    createUser.mockImplementation(async (userData) => {
+    createUserIfAbsent.mockImplementation(async (userData) => {
       // simulate created user with an _id property
-      return { _id: 'newUserId', ...userData };
+      return { ok: true, value: { _id: 'newUserId', ...userData } };
     });
     updateUser.mockImplementation(async (id, userData) => {
       return { _id: id, ...userData };
@@ -420,7 +421,7 @@ describe('setupOpenId', () => {
     expect(getOpenIdIssuer.mock.calls[0][1]).toEqual(
       expect.objectContaining({ issuer: 'https://fake-issuer.com' }),
     );
-    expect(createUser).toHaveBeenCalledWith(
+    expect(createUserIfAbsent).toHaveBeenCalledWith(
       expect.objectContaining({
         provider: 'openid',
         openidId: userinfo.sub,
@@ -430,8 +431,6 @@ describe('setupOpenId', () => {
         name: `${userinfo.given_name} ${userinfo.family_name}`,
       }),
       { enabled: false },
-      true,
-      true,
     );
   });
 
@@ -447,11 +446,9 @@ describe('setupOpenId', () => {
 
     // Assert
     expect(user.username).toBe(expectUsername);
-    expect(createUser).toHaveBeenCalledWith(
+    expect(createUserIfAbsent).toHaveBeenCalledWith(
       expect.objectContaining({ username: expectUsername }),
       { enabled: false },
-      true,
-      true,
     );
   });
 
@@ -467,11 +464,9 @@ describe('setupOpenId', () => {
 
     // Assert
     expect(user.username).toBe(expectUsername);
-    expect(createUser).toHaveBeenCalledWith(
+    expect(createUserIfAbsent).toHaveBeenCalledWith(
       expect.objectContaining({ username: expectUsername }),
       { enabled: false },
-      true,
-      true,
     );
   });
 
@@ -485,11 +480,9 @@ describe('setupOpenId', () => {
 
     // Assert – username should equal the sub (converted as-is)
     expect(user.username).toBe(userinfo.sub);
-    expect(createUser).toHaveBeenCalledWith(
+    expect(createUserIfAbsent).toHaveBeenCalledWith(
       expect.objectContaining({ username: userinfo.sub }),
       { enabled: false },
-      true,
-      true,
     );
   });
 
@@ -575,7 +568,7 @@ describe('setupOpenId', () => {
     // Assert – verify that the strategy rejects login
     expect(result.user).toBe(false);
     expect(result.details.message).toBe(ErrorTypes.AUTH_FAILED);
-    expect(createUser).not.toHaveBeenCalled();
+    expect(createUserIfAbsent).not.toHaveBeenCalled();
     expect(updateUser).not.toHaveBeenCalled();
   });
 
@@ -602,10 +595,109 @@ describe('setupOpenId', () => {
 
     expect(result.user).toBe(false);
     expect(result.details.message).toBe(ErrorTypes.AUTH_FAILED);
-    expect(createUser).not.toHaveBeenCalled();
+    expect(createUserIfAbsent).not.toHaveBeenCalled();
     expect(updateUser).not.toHaveBeenCalled();
   });
 
+  describe('concurrent first login', () => {
+    const raceCreateWith = (existingUser) => {
+      let created = false;
+      const matches = (query) =>
+        Object.entries(query).every(([field, value]) => existingUser[field] === value);
+      findUser.mockImplementation(async (query) =>
+        created && matches(query) ? existingUser : null,
+      );
+      createUserIfAbsent.mockImplementation(async () => {
+        created = true;
+        return { ok: false, error: { code: 'user_exists' } };
+      });
+    };
+
+    it('continues as the user the other request created', async () => {
+      raceCreateWith({
+        _id: 'winnerUserId',
+        provider: 'openid',
+        openidId: tokenset.claims().sub,
+        openidIssuer: 'https://fake-issuer.com',
+        email: tokenset.claims().email,
+        username: 'testusername',
+        name: 'First Last',
+      });
+
+      const { user } = await validate(tokenset);
+
+      expect(createUserIfAbsent).toHaveBeenCalledTimes(1);
+      expect(updateUser).toHaveBeenCalledWith(
+        'winnerUserId',
+        expect.objectContaining({ openidId: tokenset.claims().sub }),
+      );
+      expect(user).toEqual(
+        expect.objectContaining({ _id: 'winnerUserId', email: tokenset.claims().email }),
+      );
+    });
+
+    it("writes this callback's claims over the ones the other request stored", async () => {
+      raceCreateWith({
+        _id: 'winnerUserId',
+        provider: 'openid',
+        openidId: tokenset.claims().sub,
+        openidIssuer: 'https://fake-issuer.com',
+        email: 'old@example.com',
+        emailVerified: false,
+        username: 'old-username',
+        name: 'Old Name',
+      });
+
+      await validate(tokenset);
+
+      expect(updateUser).toHaveBeenCalledWith(
+        'winnerUserId',
+        expect.objectContaining({
+          email: tokenset.claims().email,
+          emailVerified: true,
+          username: tokenset.claims().preferred_username,
+          name: `${tokenset.claims().given_name} ${tokenset.claims().family_name}`,
+        }),
+      );
+    });
+
+    it('fails the login while the account it found has no start balance yet', async () => {
+      const { findBalanceByUser } = require('~/models');
+      const { getBalanceConfig } = require('@librechat/api');
+      getBalanceConfig.mockReturnValue({ enabled: true, startBalance: 500 });
+      findBalanceByUser.mockResolvedValue(null);
+      raceCreateWith({
+        _id: 'winnerUserId',
+        provider: 'openid',
+        openidId: tokenset.claims().sub,
+        openidIssuer: 'https://fake-issuer.com',
+        email: tokenset.claims().email,
+      });
+
+      const result = await validate(tokenset);
+
+      expect(findBalanceByUser).toHaveBeenCalledWith('winnerUserId');
+      expect(result.user).toBe(false);
+      expect(result.details.message).toBe(ErrorTypes.AUTH_FAILED);
+      expect(updateUser).not.toHaveBeenCalled();
+      getBalanceConfig.mockReturnValue({ enabled: false });
+    });
+
+    it('fails the login when another provider took the email in the meantime', async () => {
+      raceCreateWith({
+        _id: 'localUserId',
+        provider: 'local',
+        email: tokenset.claims().email,
+      });
+
+      const result = await validate(tokenset);
+
+      expect(result.user).toBe(false);
+      expect(result.details.message).toBe(ErrorTypes.AUTH_FAILED);
+      expect(updateUser).not.toHaveBeenCalled();
+    });
+  });
+
   it('should enforce the required role and reject login if missing', async () => {
     // Arrange – simulate a token without the required role.
     jwtDecode.mockReturnValue({
@@ -650,7 +742,7 @@ describe('setupOpenId', () => {
 
     // Assert – login succeeds when required role is present after splitting
     expect(user).toBeTruthy();
-    expect(createUser).toHaveBeenCalled();
+    expect(createUserIfAbsent).toHaveBeenCalled();
   });
 
   it('should allow login when roles claim is a comma-separated string containing the required role', async () => {
@@ -664,7 +756,7 @@ describe('setupOpenId', () => {
 
     // Assert – login succeeds when required role is present after splitting
     expect(user).toBeTruthy();
-    expect(createUser).toHaveBeenCalled();
+    expect(createUserIfAbsent).toHaveBeenCalled();
   });
 
   it('should allow login when roles claim is a mixed comma-and-space-separated string containing the required role', async () => {
@@ -678,7 +770,7 @@ describe('setupOpenId', () => {
 
     // Assert – login succeeds when required role is present after splitting
     expect(user).toBeTruthy();
-    expect(createUser).toHaveBeenCalled();
+    expect(createUserIfAbsent).toHaveBeenCalled();
   });
 
   it('should reject login when roles claim is a space-separated string that does not contain the required role', async () => {
@@ -710,7 +802,7 @@ describe('setupOpenId', () => {
     expect(user).toBeTruthy();
     expect(user.email).toBe(tokenset.claims().email);
     expect(user.username).toBe(tokenset.claims().preferred_username);
-    expect(createUser).toHaveBeenCalled();
+    expect(createUserIfAbsent).toHaveBeenCalled();
   });
 
   it('should allow login when required role is found in userinfo claims', async () => {
@@ -2422,11 +2514,9 @@ describe('setupOpenId', () => {
       const { user } = await validate({ ...tokenset, claims: () => userinfo });
 
       expect(user.email).toBe('user@corp.example.com');
-      expect(createUser).toHaveBeenCalledWith(
+      expect(createUserIfAbsent).toHaveBeenCalledWith(
         expect.objectContaining({ email: 'user@corp.example.com' }),
         expect.anything(),
-        true,
-        true,
       );
     });