From e0296897eb8cfce05b1f0ba1525d1033ad4c3b4a Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 5 Oct 2026 11:01:39 -0400 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=94=81=20fix:=20Recover=20Concurrent?= =?UTF-8?q?=20First=20SAML,=20LDAP=20and=20Social=20Logins?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SAML, LDAP and social logins looked the user up and then inserted it with nothing in between, so two first logins for one new identity both inserted and the unique index failed the second with E11000. Every strategy now provisions first-login users in packages/api: createUserOnce inserts through createUserIfAbsent and, on user_exists, repeats the strategy's own lookup and resolves to the account that won, once its creator finished provisioning it (its start balance exists when one is configured), admitted exactly as a found account (a tenant account resolves its tenant config and that config's email-domain policy applies). provisionSamlUser, provisionLdapUser and provisionSocialUser own the create / recover / refresh decision, so a recovered account gets the same refresh as any account the login finds (claimSamlIdentity, the LDAP directory overwrite, handleExistingUser), and the CJS strategies only pass their models and services and map the result to passport. The lookups take a plain FindUserByFields and the contracts carry UserRecord and NewUserData. createOpenIDUser now goes through createUserOnce. Related to #16766 --- api/strategies/ldapStrategy.js | 69 +++--- api/strategies/ldapStrategy.spec.js | 94 +++++++- api/strategies/process.js | 93 +++++--- api/strategies/process.test.js | 101 +++++++- api/strategies/samlStrategy.js | 81 +++---- api/strategies/samlStrategy.spec.js | 105 ++++++++- api/strategies/socialLogin.js | 24 +- api/strategies/socialLogin.test.js | 1 + packages/api/src/auth/domain.ts | 3 + packages/api/src/auth/index.ts | 3 + packages/api/src/auth/ldap.ts | 91 ++++++++ packages/api/src/auth/openid.ts | 109 +++------ packages/api/src/auth/provision.spec.ts | 293 ++++++++++++++++++++++++ packages/api/src/auth/provision.ts | 121 ++++++++++ packages/api/src/auth/saml.ts | 114 +++++++++ packages/api/src/auth/social.ts | 99 ++++++++ 16 files changed, 1176 insertions(+), 225 deletions(-) create mode 100644 packages/api/src/auth/ldap.ts create mode 100644 packages/api/src/auth/provision.spec.ts create mode 100644 packages/api/src/auth/provision.ts create mode 100644 packages/api/src/auth/social.ts diff --git a/api/strategies/ldapStrategy.js b/api/strategies/ldapStrategy.js index 9253f541963..64bc776bf96 100644 --- a/api/strategies/ldapStrategy.js +++ b/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); diff --git a/api/strategies/ldapStrategy.spec.js b/api/strategies/ldapStrategy.spec.js index 876d70f8450..7e93823fdc2 100644 --- a/api/strategies/ldapStrategy.spec.js +++ b/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(); + }); + }); }); diff --git a/api/strategies/process.js b/api/strategies/process.js index a911a3b447c..d8a9c02ba10 100644 --- a/api/strategies/process.js +++ b/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} + */ +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} * 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, diff --git a/api/strategies/process.test.js b/api/strategies/process.test.js index dd953d67dd1..63fdad6734f 100644 --- a/api/strategies/process.test.js +++ b/api/strategies/process.test.js @@ -1,4 +1,4 @@ -const { FileSources } = require('librechat-data-provider'); +const { FileSources, ErrorTypes } = require('librechat-data-provider'); const { handleExistingUser, createSocialUser } = require('./process'); jest.mock('~/server/services/Files/strategies', () => ({ @@ -11,8 +11,9 @@ jest.mock('~/server/services/Files/images/avatar', () => ({ jest.mock('~/models', () => ({ updateUser: jest.fn(), - createUser: jest.fn(), + createUserIfAbsent: jest.fn(), getUserById: jest.fn(), + findBalanceByUser: jest.fn(), })); jest.mock('~/server/services/Config', () => ({ @@ -20,6 +21,7 @@ jest.mock('~/server/services/Config', () => ({ })); jest.mock('@librechat/api', () => ({ + provisionSocialUser: jest.requireActual('@librechat/api').provisionSocialUser, getBalanceConfig: jest.fn(() => ({ enabled: false, })), @@ -27,7 +29,7 @@ jest.mock('@librechat/api', () => ({ const { getStrategyFunctions } = require('~/server/services/Files/strategies'); const { resizeAvatar } = require('~/server/services/Files/images/avatar'); -const { updateUser, createUser, getUserById } = require('~/models'); +const { updateUser, getUserById, createUserIfAbsent } = require('~/models'); describe('handleExistingUser', () => { beforeEach(() => { @@ -284,7 +286,7 @@ describe('createSocialUser', () => { beforeEach(() => { jest.clearAllMocks(); process.env.CDN_PROVIDER = FileSources.s3; - createUser.mockResolvedValue('newUser123'); + createUserIfAbsent.mockResolvedValue({ ok: true, value: { _id: 'newUser123' } }); getUserById.mockResolvedValue({ _id: 'newUser123' }); }); @@ -298,10 +300,101 @@ describe('createSocialUser', () => { username: 'user', name: 'User', emailVerified: true, + lookup: { + findUser: jest.fn(), + provider: 'apple', + providerId: 'apple-sub-123', + email: 'user@privaterelay.appleid.com', + }, }); expect(resizeAvatar).not.toHaveBeenCalled(); expect(updateUser).not.toHaveBeenCalled(); expect(getUserById).toHaveBeenCalledWith('newUser123'); }); + + describe('concurrent first login', () => { + const params = { + email: 'user@example.com', + avatarUrl: null, + provider: 'google', + providerKey: 'googleId', + providerId: 'google-sub-123', + username: 'user', + name: 'User', + emailVerified: true, + }; + + beforeEach(() => { + createUserIfAbsent.mockResolvedValue({ ok: false, error: { code: 'user_exists' } }); + }); + + it('handles the account the other request created as an existing user', async () => { + const winner = { _id: 'winner-id', provider: 'google', email: 'old@example.com' }; + + const findUser = jest.fn(async (query) => + query.googleId === params.providerId ? winner : null, + ); + + const user = await createSocialUser({ + ...params, + lookup: { + findUser, + provider: 'google', + providerId: params.providerId, + email: params.email, + }, + }); + + expect(user).toBe(winner); + expect(updateUser).toHaveBeenCalledWith('winner-id', { email: 'user@example.com' }); + expect(getUserById).not.toHaveBeenCalled(); + }); + + it("fails the login when the recovered tenant account's policy rejects the email", async () => { + const { getAppConfig } = require('~/server/services/Config'); + getAppConfig.mockImplementation(async (options) => + options?.tenantId ? { registration: { allowedDomains: ['other.example'] } } : {}, + ); + const winner = { _id: 'tenant-id', provider: 'google', email: params.email, tenantId: 't' }; + const findUser = jest.fn(async (query) => + query.googleId === params.providerId ? winner : null, + ); + + const login = createSocialUser({ + ...params, + lookup: { + findUser, + provider: 'google', + providerId: params.providerId, + email: params.email, + }, + }); + + await expect(login).rejects.toMatchObject({ + code: ErrorTypes.AUTH_FAILED, + message: 'Email domain not allowed', + }); + expect(updateUser).not.toHaveBeenCalled(); + getAppConfig.mockResolvedValue({}); + }); + + it('fails the login when another provider took the email in the meantime', async () => { + const localUser = { _id: 'local-id', provider: 'local', email: params.email }; + const findUser = jest.fn(async (query) => (query.email === params.email ? localUser : null)); + + const login = createSocialUser({ + ...params, + lookup: { + findUser, + provider: 'google', + providerId: params.providerId, + email: params.email, + }, + }); + + await expect(login).rejects.toMatchObject({ code: ErrorTypes.AUTH_FAILED }); + expect(updateUser).not.toHaveBeenCalled(); + }); + }); }); diff --git a/api/strategies/samlStrategy.js b/api/strategies/samlStrategy.js index 11ac1c06e1c..bdfc060ec15 100644 --- a/api/strategies/samlStrategy.js +++ b/api/strategies/samlStrategy.js @@ -5,7 +5,9 @@ const { ErrorTypes } = require('librechat-data-provider'); const { hashToken, logger } = require('@librechat/data-schemas'); const { Strategy: SamlStrategy } = require('@node-saml/passport-saml'); const { + findSamlUser, getBalanceConfig, + provisionSamlUser, isEmailDomainAllowed, getAvatarFileStrategy, getAvatarSaveParams, @@ -15,7 +17,13 @@ const { } = require('@librechat/api'); const { getStrategyFunctions } = require('~/server/services/Files/strategies'); const { resizeAvatar } = require('~/server/services/Files/images/avatar'); -const { findUser, createUser, updateUser, claimSamlIdentity } = require('~/models'); +const { + findUser, + updateUser, + findBalanceByUser, + claimSamlIdentity, + createUserIfAbsent, +} = require('~/models'); const { getAppConfig } = require('~/server/services/Config'); const paths = require('~/config/paths'); @@ -200,29 +208,13 @@ function createSamlCallback(existingUsersOnly = false) { return done(null, false, { message: 'Email domain not allowed' }); } - let user = await findUser({ samlId: nameID }); - logger.info(`[samlStrategy] User ${user ? 'found' : 'not found'} by SAML identity`); - - if (!user) { - user = await findUser({ email: userEmail }); - logger.info(`[samlStrategy] User ${user ? 'found' : 'not found'} by SAML email claim`); - } - - if (user && user.provider !== 'saml') { - logger.info(`[samlStrategy] SAML login conflicts with existing provider: ${user.provider}`); - return done(null, false, { - message: ErrorTypes.AUTH_FAILED, - }); - } - - if (user?.samlId && user.samlId !== nameID) { - logger.warn('[samlStrategy] Refused SAML login with a different NameID'); - return done(null, false, { - message: ErrorTypes.AUTH_FAILED, - }); + const found = await findSamlUser({ findUser, nameID, email: userEmail }); + if (found.error) { + return done(null, false, { message: ErrorTypes.AUTH_FAILED }); } + let user = found.user; - const appConfig = user?.tenantId + let appConfig = user?.tenantId ? await resolveAppConfigForUser(getAppConfig, user) : baseConfig; @@ -237,32 +229,29 @@ function createSamlCallback(existingUsersOnly = false) { getUserName(profile) || getGivenName(profile) || getEmail(profile), ); - if (!user) { - if (existingUsersOnly) { - logger.error('[samlStrategy] Admin auth blocked because the user does not exist'); - return done(null, false, { message: 'User does not exist' }); - } + if (!user && existingUsersOnly) { + logger.error('[samlStrategy] Admin auth blocked because the user does not exist'); + return done(null, false, { message: 'User does not exist' }); + } - user = { - provider: 'saml', - samlId: nameID, - username, - email: userEmail, - emailVerified: true, - name: fullName, - }; - const balanceConfig = getBalanceConfig(appConfig); - user = await createUser(user, balanceConfig, true, true); - } else { - user = await claimSamlIdentity(user._id, nameID, { - username, - name: fullName, - }); - if (!user) { - logger.warn('[samlStrategy] Refused a concurrent SAML identity binding'); - return done(null, false, { message: ErrorTypes.AUTH_FAILED }); - } + const provisioned = await provisionSamlUser({ + user, + nameID, + email: userEmail, + username, + name: fullName, + appConfig, + getBalanceConfig, + getAppConfig, + findUser, + createUserIfAbsent, + findBalanceByUser, + claimSamlIdentity, + }); + if (provisioned.error) { + return done(null, false, { message: provisioned.error }); } + ({ user, appConfig } = provisioned); const picture = getPicture(profile); if (picture && !user.avatar?.includes('manual=true')) { diff --git a/api/strategies/samlStrategy.spec.js b/api/strategies/samlStrategy.spec.js index 34e06e78c87..0e6af7853aa 100644 --- a/api/strategies/samlStrategy.spec.js +++ b/api/strategies/samlStrategy.spec.js @@ -1,6 +1,22 @@ // --- Mocks --- -jest.mock('fs'); -jest.mock('path'); +jest.mock('fs', () => { + const actual = jest.requireActual('fs'); + return { + ...actual, + existsSync: jest.fn(actual.existsSync), + statSync: jest.fn(actual.statSync), + readFileSync: jest.fn(actual.readFileSync), + }; +}); +jest.mock('path', () => { + const actual = jest.requireActual('path'); + return { + ...actual, + isAbsolute: jest.fn(actual.isAbsolute), + join: jest.fn(actual.join), + normalize: jest.fn(actual.normalize), + }; +}); jest.mock('node-fetch'); jest.mock('@node-saml/passport-saml'); jest.mock('@librechat/data-schemas', () => ({ @@ -11,12 +27,14 @@ jest.mock('@librechat/data-schemas', () => ({ error: jest.fn(), }, hashToken: jest.fn().mockResolvedValue('hashed-token'), + tenantStorage: { run: (_store, fn) => fn() }, })); jest.mock('~/models', () => ({ findUser: jest.fn(), - createUser: jest.fn(), + createUserIfAbsent: jest.fn(), updateUser: jest.fn(), claimSamlIdentity: jest.fn(), + findBalanceByUser: jest.fn(), })); jest.mock('~/server/services/Config', () => ({ config: { @@ -27,6 +45,8 @@ jest.mock('~/server/services/Config', () => ({ getAppConfig: jest.fn().mockResolvedValue({}), })); jest.mock('@librechat/api', () => ({ + findSamlUser: jest.requireActual('@librechat/api').findSamlUser, + provisionSamlUser: jest.requireActual('@librechat/api').provisionSamlUser, isEmailDomainAllowed: jest.fn(() => true), getBalanceConfig: jest.fn(() => ({ tokenCredits: 1000, @@ -244,11 +264,11 @@ describe('setupSaml', () => { verifyCallbacks.clear(); // Configure mocks - const { findUser, createUser, updateUser, claimSamlIdentity } = require('~/models'); + const { findUser, createUserIfAbsent, updateUser, claimSamlIdentity } = require('~/models'); findUser.mockResolvedValue(null); - createUser.mockImplementation(async (userData) => ({ - _id: 'mock-user-id', - ...userData, + createUserIfAbsent.mockImplementation(async (userData) => ({ + ok: true, + value: { _id: 'mock-user-id', ...userData }, })); updateUser.mockImplementation(async (id, userData) => ({ _id: id, @@ -488,6 +508,77 @@ u7wlOSk+oFzDIO/UILIA expect(updateUser).not.toHaveBeenCalled(); }); + describe('concurrent first login', () => { + const raceCreateWith = (existingUser) => { + const { findUser, createUserIfAbsent } = require('~/models'); + 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("claims the account the other request created with this login's profile", async () => { + const { createUserIfAbsent, claimSamlIdentity } = require('~/models'); + raceCreateWith({ + _id: 'winner-user-id', + provider: 'saml', + samlId: baseProfile.nameID, + email: baseProfile.email, + username: 'old-username', + name: 'Old Name', + }); + + const { user } = await validate(baseProfile); + + expect(createUserIfAbsent).toHaveBeenCalledTimes(1); + expect(claimSamlIdentity).toHaveBeenCalledWith('winner-user-id', baseProfile.nameID, { + username: baseProfile.username, + name: `${baseProfile.given_name} ${baseProfile.family_name}`, + }); + expect(user).toEqual(expect.objectContaining({ _id: 'winner-user-id' })); + }); + + it("fails the login when the recovered tenant account's policy rejects the email", async () => { + const { getAppConfig } = require('~/server/services/Config'); + getAppConfig.mockImplementation(async (options) => + options?.tenantId ? { registration: { allowedDomains: ['other.example'] } } : {}, + ); + raceCreateWith({ + _id: 'tenant-user-id', + provider: 'saml', + samlId: baseProfile.nameID, + email: baseProfile.email, + tenantId: 'tenant-a', + }); + + const result = await validate(baseProfile); + + expect(result.user).toBe(false); + expect(result.details.message).toBe('Email domain not allowed'); + getAppConfig.mockResolvedValue({}); + }); + + it('fails the login when another provider took the email in the meantime', async () => { + const { claimSamlIdentity, updateUser } = require('~/models'); + raceCreateWith({ _id: 'local-user-id', provider: 'local', email: baseProfile.email }); + + const result = await validate(baseProfile); + + expect(result.user).toBe(false); + expect(result.details.message).toBe( + require('librechat-data-provider').ErrorTypes.AUTH_FAILED, + ); + expect(claimSamlIdentity).not.toHaveBeenCalled(); + expect(updateUser).not.toHaveBeenCalled(); + }); + }); + it.each([undefined, '', ' '])('should reject an invalid NameID value: %p', async (nameID) => { const { findUser, claimSamlIdentity } = require('~/models'); const { resolveSamlSubject } = require('@librechat/api'); diff --git a/api/strategies/socialLogin.js b/api/strategies/socialLogin.js index da751d0c1c0..e6dfd622ac4 100644 --- a/api/strategies/socialLogin.js +++ b/api/strategies/socialLogin.js @@ -1,6 +1,11 @@ const { logger } = require('@librechat/data-schemas'); const { ErrorTypes } = require('librechat-data-provider'); -const { isEnabled, isEmailDomainAllowed, resolveAppConfigForUser } = require('@librechat/api'); +const { + isEnabled, + findSocialUser, + isEmailDomainAllowed, + resolveAppConfigForUser, +} = require('@librechat/api'); const { createSocialUser, handleExistingUser } = require('./process'); const { getAppConfig } = require('~/server/services/Config'); const { findUser, updateUser } = require('~/models'); @@ -26,20 +31,8 @@ const socialLogin = } const providerKey = `${provider}Id`; - let existingUser = null; - - /** First try to find user by provider ID (e.g., googleId, facebookId) */ - if (id && typeof id === 'string') { - existingUser = await findUser({ [providerKey]: id }); - } - - /** If not found by provider ID, try finding by email */ - if (!existingUser) { - existingUser = await findUser({ email: email?.trim() }); - if (existingUser) { - logger.warn(`[${provider}Login] User found by email: ${email} but not by ${providerKey}`); - } - } + const lookup = { findUser, provider, providerId: id, email }; + const existingUser = await findSocialUser(lookup); const appConfig = existingUser?.tenantId ? await resolveAppConfigForUser(getAppConfig, existingUser) @@ -133,6 +126,7 @@ const socialLogin = name, emailVerified, appConfig, + lookup, }); return passResult(newUser); } catch (err) { diff --git a/api/strategies/socialLogin.test.js b/api/strategies/socialLogin.test.js index 01b8c6f2109..e5a8cbf9876 100644 --- a/api/strategies/socialLogin.test.js +++ b/api/strategies/socialLogin.test.js @@ -350,6 +350,7 @@ describe('socialLogin', () => { name: 'New User', emailVerified: true, appConfig: expect.any(Object), + lookup: { findUser, provider, providerId: googleId, email }, }); expect(callback).toHaveBeenCalledWith(null, newUser); diff --git a/packages/api/src/auth/domain.ts b/packages/api/src/auth/domain.ts index 8b8d2b27464..57be3f927de 100644 --- a/packages/api/src/auth/domain.ts +++ b/packages/api/src/auth/domain.ts @@ -9,6 +9,9 @@ import { isPrivateIP } from './ip'; /** Re-exported here for backward compatibility; canonical location is `./ip`. */ export { isPrivateIP }; +/** The failure message login strategies report when the email-domain policy rejects a login. */ +export const EMAIL_DOMAIN_NOT_ALLOWED = 'Email domain not allowed'; + /** * @param email * @param allowedDomains diff --git a/packages/api/src/auth/index.ts b/packages/api/src/auth/index.ts index 3c57d149a32..5c03a45acf6 100644 --- a/packages/api/src/auth/index.ts +++ b/packages/api/src/auth/index.ts @@ -1,6 +1,9 @@ export * from './domain'; export * from './openid'; export * from './saml'; +export * from './ldap'; +export * from './social'; +export * from './provision'; export * from './proxy'; export * from './exchange'; export * from './refresh'; diff --git a/packages/api/src/auth/ldap.ts b/packages/api/src/auth/ldap.ts new file mode 100644 index 00000000000..e44a2358323 --- /dev/null +++ b/packages/api/src/auth/ldap.ts @@ -0,0 +1,91 @@ +import { logger } from '@librechat/data-schemas'; +import { ErrorTypes, SystemRoles } from 'librechat-data-provider'; +import type { AppConfig, UserRecord } from '@librechat/data-schemas'; +import type { + GetAppConfig, + GetBalanceConfig, + ProvisionedUser, + FindUserByFields, + FindBalanceByUser, + CreateUserIfAbsent, + LoginUserResolution, +} from './provision'; +import { createUserOnce } from './provision'; + +/** Finds the account an LDAP login continues as; one owned by another provider fails the login. */ +export async function findLdapUser({ + findUser, + ldapId, +}: { + findUser: FindUserByFields; + ldapId: string; +}): Promise { + const user = await findUser({ ldapId }); + if (user && user.provider !== 'ldap') { + logger.info(`[ldapStrategy] User ${user.email} already exists with provider ${user.provider}`); + return { user: null, error: ErrorTypes.AUTH_FAILED }; + } + return { user, error: null }; +} + +/** + * Resolves the account an LDAP login continues as, carrying the directory's values: LDAP manages + * these users' identity, so provider, `ldapId`, email, username and name are overwritten on every + * login. Without an account, the user is created through `createUserOnce` (the deployment's first + * user as ADMIN), and an account a concurrent first login created is admitted like a found account + * and refreshed the same way. Returns the account with the config the login continues under. + */ +export async function provisionLdapUser({ + user, + ldapId, + email, + username, + name, + appConfig, + getAppConfig, + getBalanceConfig, + findUser, + countUsers, + createUserIfAbsent, + findBalanceByUser, +}: { + user: UserRecord | null; + ldapId: string; + email: string; + username: string; + name: string; + appConfig: AppConfig; + getAppConfig: GetAppConfig; + getBalanceConfig: GetBalanceConfig; + findUser: FindUserByFields; + countUsers: () => Promise; + createUserIfAbsent: CreateUserIfAbsent; + findBalanceByUser: FindBalanceByUser; +}): Promise { + let account = user; + let accountConfig = appConfig; + if (!account) { + const role = (await countUsers()) === 0 ? SystemRoles.ADMIN : SystemRoles.USER; + const result = await createUserOnce({ + newUser: { provider: 'ldap', ldapId, username, email, emailVerified: true, name, role }, + email, + appConfig, + getAppConfig, + getBalanceConfig, + strategyName: 'ldapStrategy', + lookup: () => findLdapUser({ findUser, ldapId }), + createUserIfAbsent, + findBalanceByUser, + }); + if (result.error !== null) return { user: null, error: result.error }; + account = result.user; + accountConfig = result.appConfig; + } + + const directoryValues = { provider: 'ldap', ldapId, email, username, name }; + return { + user: Object.assign({}, account, directoryValues), + appConfig: accountConfig, + error: null, + }; +} diff --git a/packages/api/src/auth/openid.ts b/packages/api/src/auth/openid.ts index e121f484948..91a966f6a7e 100644 --- a/packages/api/src/auth/openid.ts +++ b/packages/api/src/auth/openid.ts @@ -1,19 +1,16 @@ import { logger } from '@librechat/data-schemas'; import { ErrorTypes } from 'librechat-data-provider'; -import type { - IUser, - UserMethods, - UserRecord, - NewUserData, - BalanceConfig, - CreateUserIfAbsentResult, - AppConfig, -} from '@librechat/data-schemas'; +import type { IUser, AppConfig, UserMethods, UserRecord } from '@librechat/data-schemas'; import type { FilterQuery } from 'mongoose'; +import type { + GetAppConfig, + GetBalanceConfig, + FindBalanceByUser, + CreateUserIfAbsent, +} from './provision'; import type { OpenIDUserLookupResult } from '~/app/metrics'; import { isMetricsConfigured, recordOpenIDUserLookup } from '~/app/metrics'; -import { resolveAppConfigForUser } from '~/app/resolve'; -import { isEmailDomainAllowed } from './domain'; +import { createUserOnce } from './provision'; export type OpenIdEmailClaims = { email?: unknown; @@ -339,27 +336,13 @@ export function applyOpenIDProfile(user: T, profile: OpenI return updated; } -/** Whether new users start with a balance record (the condition data-schemas credits it under). */ -function hasStartBalance(balanceConfig?: BalanceConfig | null): boolean { - return Boolean(balanceConfig?.enabled && balanceConfig.startBalance); -} - /** - * Creates a first-login OpenID user. Concurrent first logins for one identity all miss - * `findOpenIDUser`, and `createUserIfAbsent` reports `user_exists` for every insert but the - * first; a rejected request repeats the lookup, with its provider and issuer checks, and - * continues as the account that won, admitted and refreshed exactly as if its first lookup had - * found it: a tenant account resolves its tenant config and that config's email-domain policy - * applies, and the account carries this callback's claims. Returns the account with the config - * the login continues under (`appConfig` for a user this request created). - * - * It continues only once the winner finished provisioning that account. When the config new - * users are created under (`appConfig`) sets a start balance, the balance must exist: - * `createUserIfAbsent` writes it before the user, but a winner on an earlier release adds it with - * `$inc` after its insert, and login balance sync must not initialize it first; such a login fails - * as it did before recovery existed. A start balance only the account's tenant config sets is - * initialized by login balance sync with an insert-only write that cannot be added on top, so it - * does not hold the login back. A conflict the lookup cannot account for throws. + * Creates a first-login OpenID user through `createUserOnce`, which recovers a concurrent first + * login by repeating `findOpenIDUser`, with its provider and issuer checks, once the winner + * finished provisioning the account, and admits that account exactly as a found one (tenant + * config and email-domain policy). The recovered account is refreshed with this callback's + * claims like any account the lookup finds. Returns the account with the config the login + * continues under (`appConfig` for a user this request created). */ export async function createOpenIDUser({ lookup, @@ -373,16 +356,14 @@ export async function createOpenIDUser({ lookup: Parameters[0]; profile: OpenIDProfile; appConfig: AppConfig; - getAppConfig: Parameters[0]; - getBalanceConfig: (appConfig: AppConfig) => BalanceConfig | null | undefined; - createUserIfAbsent: ( - data: NewUserData, - balanceConfig?: BalanceConfig, - ) => Promise; - findBalanceByUser: (userId: string) => Promise; + getAppConfig: GetAppConfig; + getBalanceConfig: GetBalanceConfig; + createUserIfAbsent: CreateUserIfAbsent; + findBalanceByUser: FindBalanceByUser; }): Promise<{ user: UserRecord; appConfig: AppConfig }> { - const created = await createUserIfAbsent( - { + const result = await createUserOnce({ + strategyName: lookup.strategyName ?? 'openid', + newUser: { provider: 'openid', openidId: profile.openidId, username: profile.username, @@ -392,39 +373,17 @@ export async function createOpenIDUser({ idOnTheSource: profile.idOnTheSource, openidIssuer: profile.openidIssuer, }, - getBalanceConfig(appConfig) ?? undefined, - ); - if (created.ok) return { user: created.value, appConfig }; - - const strategyName = lookup.strategyName ?? 'openid'; - const resolution = await findOpenIDUser(lookup); - if (resolution.error) throw new Error(ErrorTypes.AUTH_FAILED); - if (!resolution.user) { - throw new Error( - `[${strategyName}] New user conflicts with an account the lookup cannot resolve`, - ); - } - - const userId = resolution.user._id.toString(); - const accountConfig = resolution.user.tenantId - ? await resolveAppConfigForUser(getAppConfig, resolution.user) - : appConfig; - if (!isEmailDomainAllowed(profile.email ?? '', accountConfig?.registration?.allowedDomains)) { - logger.error( - `[${strategyName}] Authentication blocked - email domain not allowed for the recovered account [Identifier: ${profile.email}]`, - ); - throw new Error('Email domain not allowed'); - } - - if (hasStartBalance(getBalanceConfig(appConfig)) && !(await findBalanceByUser(userId))) { - logger.warn( - `[${strategyName}] Concurrent first login found user ${userId} before its start balance; failing this login`, - ); - throw new Error(ErrorTypes.AUTH_FAILED); - } - - logger.info( - `[${strategyName}] Concurrent first login for user ${userId}; continuing as that user`, - ); - return { user: applyOpenIDProfile(resolution.user, profile), appConfig: accountConfig }; + email: profile.email ?? '', + appConfig, + getAppConfig, + getBalanceConfig, + lookup: () => findOpenIDUser(lookup), + createUserIfAbsent, + findBalanceByUser, + }); + if (result.error !== null) throw new Error(result.error); + return { + user: result.created ? result.user : applyOpenIDProfile(result.user, profile), + appConfig: result.appConfig, + }; } diff --git a/packages/api/src/auth/provision.spec.ts b/packages/api/src/auth/provision.spec.ts new file mode 100644 index 00000000000..9c5e3bc5d4f --- /dev/null +++ b/packages/api/src/auth/provision.spec.ts @@ -0,0 +1,293 @@ +import mongoose from 'mongoose'; +import { MongoMemoryServer } from 'mongodb-memory-server'; +import { ErrorTypes, SystemRoles } from 'librechat-data-provider'; +import { tenantStorage, createMethods, createModels } from '@librechat/data-schemas'; +import type { IUser, AppConfig, UserRecord } from '@librechat/data-schemas'; +import type { LoginUserResolution } from './provision'; +import { provisionSocialUser } from './social'; +import { createUserOnce } from './provision'; +import { provisionSamlUser } from './saml'; +import { provisionLdapUser } from './ldap'; + +let mongoServer: MongoMemoryServer; +let User: mongoose.Model; +let methods: ReturnType; + +beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + await mongoose.connect(mongoServer.getUri(), { autoIndex: false }); + createModels(mongoose); + User = mongoose.models.User as mongoose.Model; + methods = createMethods(mongoose); +}); + +afterAll(async () => { + await mongoose.disconnect(); + await mongoServer.stop(); +}); + +beforeEach(async () => { + await mongoose.connection.dropDatabase(); + await User.syncIndexes(); +}); + +function findBalanceByUser(userId: string) { + return methods.findBalanceByUser(userId); +} + +function appConfigWith( + allowedDomains?: string[], + balance?: { enabled: boolean; startBalance: number }, +): AppConfig { + return { balance, registration: { allowedDomains } } as Partial as AppConfig; +} + +const baseConfig = appConfigWith(); +const getAppConfig = jest.fn(async (): Promise => baseConfig); +const getBalanceConfig = (config: AppConfig) => config.balance ?? null; +const admission = { appConfig: baseConfig, getAppConfig, getBalanceConfig }; + +describe('createUserOnce', () => { + const email = 'first-login@example.com'; + const newUser = { provider: 'saml', samlId: 'first-login-name-id', email, username: 'first' }; + + function lookupByEmail() { + return methods.findUser({ email }).then((user) => ({ user, error: null })); + } + + function firstLogin( + lookup: () => Promise = lookupByEmail, + overrides: Partial[0]> = {}, + ) { + return createUserOnce({ + newUser, + email, + ...admission, + lookup, + strategyName: 'test', + createUserIfAbsent: methods.createUserIfAbsent, + findBalanceByUser, + ...overrides, + }); + } + + it('creates once and resolves the concurrent login to the created account', async () => { + const results = await Promise.all([firstLogin(), firstLogin()]); + + expect(await User.countDocuments({ email })).toBe(1); + expect(results.map((result) => result.created).sort()).toEqual([false, true]); + expect(results[0].user?._id.toString()).toBe(results[1].user?._id.toString()); + }); + + it("returns the lookup's error when it rejects the account that won", async () => { + await firstLogin(); + + const result = await firstLogin(async () => ({ user: null, error: 'auth_failed' })); + + expect(result).toEqual({ user: null, error: 'auth_failed', created: false }); + }); + + it('fails the recovered login while the winner has not credited its start balance yet', async () => { + await User.create(newUser); + + const result = await firstLogin(lookupByEmail, { + appConfig: appConfigWith(undefined, { enabled: true, startBalance: 500 }), + }); + + expect(result).toEqual({ user: null, error: ErrorTypes.AUTH_FAILED, created: false }); + }); + + it('recovers once the winner has credited its start balance', async () => { + const appConfig = appConfigWith(undefined, { enabled: true, startBalance: 500 }); + const winner = await firstLogin(lookupByEmail, { appConfig }); + + const loser = await firstLogin(lookupByEmail, { appConfig }); + + expect(loser.created).toBe(false); + expect(loser.user?._id.toString()).toBe(winner.user?._id.toString()); + }); + + it("applies the recovered tenant account's email-domain policy", async () => { + const tenantId = 'tenant-a'; + await tenantStorage.run({ tenantId }, () => User.create(newUser)); + getAppConfig.mockResolvedValueOnce(appConfigWith(['other.example'])); + + const result = await tenantStorage.run({ tenantId }, () => firstLogin()); + + expect(result).toEqual({ user: null, error: 'Email domain not allowed', created: false }); + expect(getAppConfig).toHaveBeenCalledWith(expect.objectContaining({ tenantId })); + }); + + it("continues when only the recovered tenant account's config sets a start balance", async () => { + const tenantId = 'tenant-a'; + await tenantStorage.run({ tenantId }, () => User.create(newUser)); + getAppConfig.mockResolvedValueOnce( + appConfigWith(undefined, { enabled: true, startBalance: 500 }), + ); + + const result = await tenantStorage.run({ tenantId }, () => firstLogin()); + + expect(result).toEqual(expect.objectContaining({ created: false, error: null })); + }); + + it("continues under the recovered tenant account's config when it admits the email", async () => { + const tenantId = 'tenant-a'; + await tenantStorage.run({ tenantId }, () => User.create(newUser)); + const tenantConfig = appConfigWith(['example.com']); + getAppConfig.mockResolvedValueOnce(tenantConfig); + + const result = await tenantStorage.run({ tenantId }, () => firstLogin()); + + expect(result).toEqual( + expect.objectContaining({ created: false, error: null, appConfig: tenantConfig }), + ); + }); + + it('throws when the lookup cannot account for the existing account', async () => { + await firstLogin(); + + const login = firstLogin(async () => ({ user: null, error: null })); + + await expect(login).rejects.toThrow('conflicts with an account the lookup cannot resolve'); + }); + + it('rethrows other create failures without repeating the lookup', async () => { + const lookup = jest.fn(lookupByEmail); + + const login = firstLogin(lookup, { newUser: { ...newUser, email: 'not-an-email' } }); + + await expect(login).rejects.toMatchObject({ name: 'ValidationError' }); + expect(lookup).not.toHaveBeenCalled(); + }); +}); + +describe('provisionSamlUser', () => { + const nameID = 'saml-name-id'; + const email = 'saml@example.com'; + + function samlLogin( + user: UserRecord | null, + profile = { username: 'saml-user', name: 'Saml User' }, + ) { + return provisionSamlUser({ + user, + nameID, + email, + ...profile, + ...admission, + findUser: methods.findUser, + createUserIfAbsent: methods.createUserIfAbsent, + findBalanceByUser, + claimSamlIdentity: methods.claimSamlIdentity, + }); + } + + it("claims the account a concurrent first login created with this login's profile", async () => { + const winner = await samlLogin(null); + + const loser = await samlLogin(null, { username: 'renamed', name: 'Renamed User' }); + + expect(await User.countDocuments()).toBe(1); + expect(loser.user?._id.toString()).toBe(winner.user?._id.toString()); + expect(loser.user).toEqual( + expect.objectContaining({ samlId: nameID, username: 'renamed', name: 'Renamed User' }), + ); + }); + + it('fails the login when another provider took the email in the meantime', async () => { + await User.create({ email, provider: 'local', username: 'local-user' }); + + const result = await samlLogin(null); + + expect(result).toEqual({ user: null, error: ErrorTypes.AUTH_FAILED }); + }); +}); + +describe('provisionLdapUser', () => { + const ldapId = 'ldap-uid'; + + function ldapLogin( + user: UserRecord | null, + values = { email: 'ldap@example.com', username: 'ldap' }, + ) { + return provisionLdapUser({ + user, + ldapId, + name: 'Ldap User', + ...values, + ...admission, + findUser: methods.findUser, + countUsers: () => methods.countUsers(), + createUserIfAbsent: methods.createUserIfAbsent, + findBalanceByUser, + }); + } + + it('creates the first deployment user as ADMIN', async () => { + const result = await ldapLogin(null); + + expect(result.user).toEqual(expect.objectContaining({ ldapId, role: SystemRoles.ADMIN })); + }); + + it("carries this login's directory values onto the account a concurrent first login created", async () => { + const winner = await ldapLogin(null); + + const loser = await ldapLogin(null, { email: 'moved@example.com', username: 'moved' }); + + expect(await User.countDocuments()).toBe(1); + expect(loser.user?._id.toString()).toBe(winner.user?._id.toString()); + expect(loser.user).toEqual( + expect.objectContaining({ + provider: 'ldap', + email: 'moved@example.com', + username: 'moved', + role: SystemRoles.ADMIN, + }), + ); + }); +}); + +describe('provisionSocialUser', () => { + const email = 'social@example.com'; + const newUser = { email, provider: 'google', googleId: 'google-sub', username: 'social' }; + + function socialLogin() { + const finishNewUser = jest.fn(async (user: UserRecord) => user); + const refreshExistingUser = jest.fn(async () => undefined); + const login = provisionSocialUser({ + lookup: { findUser: methods.findUser, provider: 'google', providerId: 'google-sub', email }, + newUser, + ...admission, + createUserIfAbsent: methods.createUserIfAbsent, + findBalanceByUser, + finishNewUser, + refreshExistingUser, + }); + return { login, finishNewUser, refreshExistingUser }; + } + + it('refreshes the account a concurrent first login created instead of finishing it again', async () => { + const winner = socialLogin(); + const created = await winner.login; + + const loser = socialLogin(); + const recovered = await loser.login; + + expect(recovered._id.toString()).toBe(created._id.toString()); + expect(winner.finishNewUser).toHaveBeenCalledTimes(1); + expect(loser.finishNewUser).not.toHaveBeenCalled(); + expect(loser.refreshExistingUser).toHaveBeenCalledWith( + expect.objectContaining({ _id: created._id }), + baseConfig, + ); + }); + + it('throws the coded AUTH_FAILED error when another provider took the email', async () => { + await User.create({ email, provider: 'local', username: 'local-user' }); + + const { login, refreshExistingUser } = socialLogin(); + + await expect(login).rejects.toMatchObject({ code: ErrorTypes.AUTH_FAILED }); + expect(refreshExistingUser).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/api/src/auth/provision.ts b/packages/api/src/auth/provision.ts new file mode 100644 index 00000000000..48bde611b02 --- /dev/null +++ b/packages/api/src/auth/provision.ts @@ -0,0 +1,121 @@ +import { logger } from '@librechat/data-schemas'; +import { ErrorTypes } from 'librechat-data-provider'; +import type { + AppConfig, + UserRecord, + NewUserData, + BalanceConfig, + CreateUserIfAbsentResult, +} from '@librechat/data-schemas'; +import { EMAIL_DOMAIN_NOT_ALLOWED, isEmailDomainAllowed } from './domain'; +import { resolveAppConfigForUser } from '~/app/resolve'; + +/** A login lookup's verdict: the account to continue as (or none yet), or the code the login fails with. */ +export type LoginUserResolution = { user: UserRecord | null; error: string | null }; + +/** A provisioned login account with the config the login continues under, or the code it fails with. */ +export type ProvisionedUser = + | { user: UserRecord; appConfig: AppConfig; error: null } + | { user: null; error: string }; + +/** Finds one user by field equality; the caller binds it to its storage (data-schemas `findUser`). */ +export type FindUserByFields = ( + fields: Record, +) => Promise; + +/** Inserts a first-login user, reporting `user_exists` on a lost race (data-schemas `createUserIfAbsent`). */ +export type CreateUserIfAbsent = ( + data: NewUserData, + balanceConfig?: BalanceConfig, +) => Promise; + +/** Finds a user's balance record, or `null` before it is initialized (data-schemas `findBalanceByUser`). */ +export type FindBalanceByUser = (userId: string) => Promise; + +/** Resolves an app config (the strategies' `getAppConfig`). */ +export type GetAppConfig = Parameters[0]; + +/** Reads the balance settings an app config applies (`@librechat/api` `getBalanceConfig`). */ +export type GetBalanceConfig = (appConfig: AppConfig) => BalanceConfig | null | undefined; + +export type FirstLoginResult = + | { user: UserRecord; appConfig: AppConfig; created: boolean; error: null } + | { user: null; error: string; created: false }; + +/** Whether new users start with a balance record (the condition data-schemas credits it under). */ +function hasStartBalance(balanceConfig?: BalanceConfig | null): boolean { + return Boolean(balanceConfig?.enabled && balanceConfig.startBalance); +} + +/** + * Inserts a first-login user. Concurrent first logins for one identity all miss the strategy's + * lookup, and `createUserIfAbsent` reports `user_exists` for every insert but the first. A + * rejected request repeats `lookup`, with the strategy's provider and identity checks, and + * resolves to the account that won (`created: false`) admitted exactly as a found account: a + * tenant account resolves its tenant config and that config's email-domain policy applies. The + * caller then refreshes it like any account its lookup finds. A user this request created keeps + * `appConfig`. A conflict the lookup cannot account for throws. + * + * It resolves to the winner's account only once the winner finished provisioning it. When the + * config new users are created under (`appConfig`) sets a start balance, the balance must + * exist: `createUserIfAbsent` writes it before the user, but a winner on an earlier release adds + * it with `$inc` after its insert, and login balance sync must not initialize it first; such a + * login fails as it did before recovery existed. A start balance only the account's tenant + * config sets is initialized by login balance sync with an insert-only write that cannot be + * added on top, so it does not hold the login back. + */ +export async function createUserOnce({ + newUser, + email, + appConfig, + getAppConfig, + getBalanceConfig, + lookup, + strategyName, + createUserIfAbsent, + findBalanceByUser, +}: { + newUser: NewUserData; + email: string; + appConfig: AppConfig; + getAppConfig: GetAppConfig; + getBalanceConfig: GetBalanceConfig; + lookup: () => Promise; + strategyName: string; + createUserIfAbsent: CreateUserIfAbsent; + findBalanceByUser: FindBalanceByUser; +}): Promise { + const created = await createUserIfAbsent(newUser, getBalanceConfig(appConfig) ?? undefined); + if (created.ok) return { user: created.value, appConfig, created: true, error: null }; + + const resolution = await lookup(); + if (resolution.error) return { user: null, error: resolution.error, created: false }; + if (!resolution.user) { + throw new Error( + `[${strategyName}] New user conflicts with an account the lookup cannot resolve`, + ); + } + + const userId = resolution.user._id.toString(); + const accountConfig = resolution.user.tenantId + ? await resolveAppConfigForUser(getAppConfig, resolution.user) + : appConfig; + if (!isEmailDomainAllowed(email, accountConfig?.registration?.allowedDomains)) { + logger.error( + `[${strategyName}] Authentication blocked - email domain not allowed for recovered user ${userId}`, + ); + return { user: null, error: EMAIL_DOMAIN_NOT_ALLOWED, created: false }; + } + + if (hasStartBalance(getBalanceConfig(appConfig)) && !(await findBalanceByUser(userId))) { + logger.warn( + `[${strategyName}] Concurrent first login found user ${userId} before its start balance; failing this login`, + ); + return { user: null, error: ErrorTypes.AUTH_FAILED, created: false }; + } + + logger.info( + `[${strategyName}] Concurrent first login for user ${userId}; continuing as that user`, + ); + return { user: resolution.user, appConfig: accountConfig, created: false, error: null }; +} diff --git a/packages/api/src/auth/saml.ts b/packages/api/src/auth/saml.ts index 72227831574..ffa8c5e928a 100644 --- a/packages/api/src/auth/saml.ts +++ b/packages/api/src/auth/saml.ts @@ -1,3 +1,17 @@ +import { logger } from '@librechat/data-schemas'; +import { ErrorTypes } from 'librechat-data-provider'; +import type { AppConfig, UserRecord } from '@librechat/data-schemas'; +import type { + GetAppConfig, + GetBalanceConfig, + ProvisionedUser, + FindUserByFields, + FindBalanceByUser, + CreateUserIfAbsent, + LoginUserResolution, +} from './provision'; +import { createUserOnce } from './provision'; + export const TRANSIENT_SAML_NAME_ID_FORMAT = 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient'; export type SamlSubjectError = 'missing_name_id' | 'transient_name_id' | 'issuer_mismatch'; @@ -33,3 +47,103 @@ export function resolveSamlSubject( return { nameID }; } + +/** + * Finds the account a SAML login continues as: by NameID, then by email. An account owned by + * another provider or bound to a different NameID fails the login. + */ +export async function findSamlUser({ + findUser, + nameID, + email, +}: { + findUser: FindUserByFields; + nameID: string; + email: string; +}): Promise { + let user = await findUser({ samlId: nameID }); + logger.info(`[samlStrategy] User ${user ? 'found' : 'not found'} by SAML identity`); + + if (!user) { + user = await findUser({ email }); + logger.info(`[samlStrategy] User ${user ? 'found' : 'not found'} by SAML email claim`); + } + + if (user && user.provider !== 'saml') { + logger.info(`[samlStrategy] SAML login conflicts with existing provider: ${user.provider}`); + return { user: null, error: ErrorTypes.AUTH_FAILED }; + } + + if (user?.samlId && user.samlId !== nameID) { + logger.warn('[samlStrategy] Refused SAML login with a different NameID'); + return { user: null, error: ErrorTypes.AUTH_FAILED }; + } + + return { user, error: null }; +} + +/** + * Resolves the account a SAML login continues as once the strategy's policy checks pass. A + * found account claims this login's NameID and profile atomically; without one, the user is + * created through `createUserOnce`, and an account a concurrent first login created is admitted + * like a found account and claimed the same way. Returns the account with the config the login + * continues under. + */ +export async function provisionSamlUser({ + user, + nameID, + email, + username, + name, + appConfig, + getAppConfig, + getBalanceConfig, + findUser, + createUserIfAbsent, + findBalanceByUser, + claimSamlIdentity, +}: { + user: UserRecord | null; + nameID: string; + email: string; + username: string; + name: string; + appConfig: AppConfig; + getAppConfig: GetAppConfig; + getBalanceConfig: GetBalanceConfig; + findUser: FindUserByFields; + createUserIfAbsent: CreateUserIfAbsent; + findBalanceByUser: FindBalanceByUser; + claimSamlIdentity: ( + userId: string, + samlId: string, + profile: { username: string; name: string }, + ) => Promise; +}): Promise { + let account = user; + let accountConfig = appConfig; + if (!account) { + const result = await createUserOnce({ + newUser: { provider: 'saml', samlId: nameID, username, email, emailVerified: true, name }, + email, + appConfig, + getAppConfig, + getBalanceConfig, + strategyName: 'samlStrategy', + lookup: () => findSamlUser({ findUser, nameID, email }), + createUserIfAbsent, + findBalanceByUser, + }); + if (result.error !== null) return { user: null, error: result.error }; + if (result.created) return { user: result.user, appConfig: result.appConfig, error: null }; + account = result.user; + accountConfig = result.appConfig; + } + + const claimed = await claimSamlIdentity(account._id.toString(), nameID, { username, name }); + if (!claimed) { + logger.warn('[samlStrategy] Refused a concurrent SAML identity binding'); + return { user: null, error: ErrorTypes.AUTH_FAILED }; + } + return { user: claimed, appConfig: accountConfig, error: null }; +} diff --git a/packages/api/src/auth/social.ts b/packages/api/src/auth/social.ts new file mode 100644 index 00000000000..4fb7e174913 --- /dev/null +++ b/packages/api/src/auth/social.ts @@ -0,0 +1,99 @@ +import { logger } from '@librechat/data-schemas'; +import { ErrorTypes } from 'librechat-data-provider'; +import type { AppConfig, UserRecord, NewUserData } from '@librechat/data-schemas'; +import type { + GetAppConfig, + GetBalanceConfig, + FindUserByFields, + FindBalanceByUser, + CreateUserIfAbsent, + LoginUserResolution, +} from './provision'; +import { createUserOnce } from './provision'; + +export type SocialUserLookup = { + findUser: FindUserByFields; + provider: string; + providerId?: string; + email?: string; +}; + +/** Finds a social login's account by provider id, then by email, whichever provider owns it. */ +export async function findSocialUser({ + findUser, + provider, + providerId, + email, +}: SocialUserLookup): Promise { + const providerKey = `${provider}Id`; + if (providerId && typeof providerId === 'string') { + const user = await findUser({ [providerKey]: providerId }); + if (user) return user; + } + + const user = await findUser({ email: email?.trim() }); + if (user) { + logger.warn(`[${provider}Login] User found by email: ${email} but not by ${providerKey}`); + } + return user; +} + +/** The account a social first login recovers: an account owned by another provider fails it. */ +export async function resolveSocialUser(lookup: SocialUserLookup): Promise { + const user = await findSocialUser(lookup); + if (!user || user.provider === lookup.provider) return { user, error: null }; + + logger.info( + `[${lookup.provider}Login] User ${lookup.email} already exists with provider ${user.provider}`, + ); + return { user: null, error: ErrorTypes.AUTH_FAILED }; +} + +/** + * Creates a social first-login user through `createUserOnce`. A new account goes through + * `finishNewUser`; an account a concurrent first login created is admitted like a found account + * and goes through `refreshExistingUser` with the config its login continues under, as any + * account the login finds does. A failure throws the coded `AUTH_FAILED` error `socialLogin` + * hands to its passport callback, with the reason (`AUTH_FAILED` or the email-domain policy) + * as its message. + */ +export async function provisionSocialUser({ + lookup, + newUser, + appConfig, + getAppConfig, + getBalanceConfig, + createUserIfAbsent, + findBalanceByUser, + finishNewUser, + refreshExistingUser, +}: { + lookup: SocialUserLookup; + newUser: NewUserData; + appConfig: AppConfig; + getAppConfig: GetAppConfig; + getBalanceConfig: GetBalanceConfig; + createUserIfAbsent: CreateUserIfAbsent; + findBalanceByUser: FindBalanceByUser; + finishNewUser: (user: UserRecord) => Promise; + refreshExistingUser: (user: UserRecord, appConfig: AppConfig) => Promise; +}): Promise { + const result = await createUserOnce({ + newUser, + email: newUser.email, + appConfig, + getAppConfig, + getBalanceConfig, + strategyName: `${lookup.provider}Login`, + lookup: () => resolveSocialUser(lookup), + createUserIfAbsent, + findBalanceByUser, + }); + if (result.error !== null) { + throw Object.assign(new Error(result.error), { code: ErrorTypes.AUTH_FAILED }); + } + if (result.created) return finishNewUser(result.user); + + await refreshExistingUser(result.user, result.appConfig); + return result.user; +} From eaa1d8d7d97f07eb18afe60be891836fc8cebd9c Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 5 Oct 2026 12:32:44 -0400 Subject: [PATCH 2/2] =?UTF-8?q?=E2=9A=A1=20perf:=20Read=20a=20Recovered=20?= =?UTF-8?q?Account's=20Config=20and=20Balance=20Together?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After losing the insert race, createUserOnce resolved the tenant config and only then read the start balance, although the balance check now depends only on the creation config. Both reads start together. --- packages/api/src/auth/provision.spec.ts | 26 +++++++++++++++++++++++++ packages/api/src/auth/provision.ts | 10 ++++++---- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/packages/api/src/auth/provision.spec.ts b/packages/api/src/auth/provision.spec.ts index 9c5e3bc5d4f..6ac83c8c7c8 100644 --- a/packages/api/src/auth/provision.spec.ts +++ b/packages/api/src/auth/provision.spec.ts @@ -130,6 +130,32 @@ describe('createUserOnce', () => { expect(result).toEqual(expect.objectContaining({ created: false, error: null })); }); + it('reads the tenant config and the start balance together', async () => { + const tenantId = 'tenant-a'; + const appConfig = appConfigWith(undefined, { enabled: true, startBalance: 500 }); + await tenantStorage.run({ tenantId }, () => firstLogin(lookupByEmail, { appConfig })); + let configResolved = false; + let balanceReadBeforeConfig = false; + getAppConfig.mockImplementationOnce(async () => { + await new Promise((resolve) => setTimeout(resolve, 20)); + configResolved = true; + return baseConfig; + }); + + const result = await tenantStorage.run({ tenantId }, () => + firstLogin(lookupByEmail, { + appConfig, + findBalanceByUser: (userId) => { + balanceReadBeforeConfig = !configResolved; + return findBalanceByUser(userId); + }, + }), + ); + + expect(result).toEqual(expect.objectContaining({ created: false, error: null })); + expect(balanceReadBeforeConfig).toBe(true); + }); + it("continues under the recovered tenant account's config when it admits the email", async () => { const tenantId = 'tenant-a'; await tenantStorage.run({ tenantId }, () => User.create(newUser)); diff --git a/packages/api/src/auth/provision.ts b/packages/api/src/auth/provision.ts index 48bde611b02..e55299e47e6 100644 --- a/packages/api/src/auth/provision.ts +++ b/packages/api/src/auth/provision.ts @@ -97,9 +97,11 @@ export async function createUserOnce({ } const userId = resolution.user._id.toString(); - const accountConfig = resolution.user.tenantId - ? await resolveAppConfigForUser(getAppConfig, resolution.user) - : appConfig; + const requiresBalance = hasStartBalance(getBalanceConfig(appConfig)); + const [accountConfig, balance] = await Promise.all([ + resolution.user.tenantId ? resolveAppConfigForUser(getAppConfig, resolution.user) : appConfig, + requiresBalance ? findBalanceByUser(userId) : null, + ]); if (!isEmailDomainAllowed(email, accountConfig?.registration?.allowedDomains)) { logger.error( `[${strategyName}] Authentication blocked - email domain not allowed for recovered user ${userId}`, @@ -107,7 +109,7 @@ export async function createUserOnce({ return { user: null, error: EMAIL_DOMAIN_NOT_ALLOWED, created: false }; } - if (hasStartBalance(getBalanceConfig(appConfig)) && !(await findBalanceByUser(userId))) { + if (requiresBalance && !balance) { logger.warn( `[${strategyName}] Concurrent first login found user ${userId} before its start balance; failing this login`, );