From 0d0d7d05bbd95d09f1bb331be89f36f088861456 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 27 Aug 2026 06:45:41 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=AC=20fix:=20Harden=20SAML=20Identity?= =?UTF-8?q?=20Binding=20(#15264)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .env.example | 7 + api/strategies/samlStrategy.js | 87 ++++---- api/strategies/samlStrategy.spec.js | 200 ++++++++++++++++-- packages/api/src/auth/index.ts | 1 + packages/api/src/auth/saml.spec.ts | 41 ++++ packages/api/src/auth/saml.ts | 35 +++ .../src/methods/user.methods.spec.ts | 134 ++++++++++++ packages/data-schemas/src/methods/user.ts | 31 +++ 8 files changed, 473 insertions(+), 63 deletions(-) create mode 100644 packages/api/src/auth/saml.spec.ts create mode 100644 packages/api/src/auth/saml.ts diff --git a/.env.example b/.env.example index e7320aa3a4..9e66dbeb10 100644 --- a/.env.example +++ b/.env.example @@ -974,6 +974,13 @@ SAML_CERT= SAML_CALLBACK_URL=/oauth/saml/callback SAML_SESSION_SECRET= +# Stable NameID format requested from the IdP. Transient identifiers are rejected. +# Persistent identifiers are recommended for account binding. +# SAML_NAME_ID_FORMAT=urn:oasis:names:tc:SAML:2.0:nameid-format:persistent + +# Expected IdP entity ID. When set, assertions from a different or missing issuer are rejected. +SAML_IDP_ISSUER= + # Attribute mappings (optional) SAML_EMAIL_CLAIM= SAML_USERNAME_CLAIM= diff --git a/api/strategies/samlStrategy.js b/api/strategies/samlStrategy.js index ced6c26148..11ac1c06e1 100644 --- a/api/strategies/samlStrategy.js +++ b/api/strategies/samlStrategy.js @@ -10,10 +10,12 @@ const { getAvatarFileStrategy, getAvatarSaveParams, resolveAppConfigForUser, + resolveSamlSubject, + TRANSIENT_SAML_NAME_ID_FORMAT, } = require('@librechat/api'); const { getStrategyFunctions } = require('~/server/services/Files/strategies'); const { resizeAvatar } = require('~/server/services/Files/images/avatar'); -const { findUser, createUser, updateUser } = require('~/models'); +const { findUser, createUser, updateUser, claimSamlIdentity } = require('~/models'); const { getAppConfig } = require('~/server/services/Config'); const paths = require('~/config/paths'); @@ -118,9 +120,7 @@ const resizeIdentityProviderAvatar = async (url, userId) => { try { return await resizeAvatar({ userId, input: url }); } catch (error) { - logger.error( - `[samlStrategy] resizeIdentityProviderAvatar: Error processing avatar at URL "${url}": ${error}`, - ); + logger.error('[samlStrategy] Failed to process identity-provider avatar', error); return null; } }; @@ -133,9 +133,7 @@ const resizeIdentityProviderAvatar = async (url, userId) => { */ function getFullName(profile) { if (process.env.SAML_NAME_CLAIM) { - logger.info( - `[samlStrategy] Using SAML_NAME_CLAIM: ${process.env.SAML_NAME_CLAIM}, profile: ${profile[process.env.SAML_NAME_CLAIM]}`, - ); + logger.debug(`[samlStrategy] Using SAML_NAME_CLAIM: ${process.env.SAML_NAME_CLAIM}`); return profile[process.env.SAML_NAME_CLAIM]; } @@ -184,42 +182,41 @@ function convertToUsername(input, defaultValue = '') { function createSamlCallback(existingUsersOnly = false) { return async (profile, done) => { try { - logger.info(`[samlStrategy] SAML authentication received for NameID: ${profile.nameID}`); - logger.debug('[samlStrategy] SAML profile:', profile); + const subject = resolveSamlSubject(profile, process.env.SAML_IDP_ISSUER); + if (subject.error) { + logger.warn(`[samlStrategy] Rejected SAML subject: ${subject.error}`); + return done(null, false, { message: ErrorTypes.AUTH_FAILED }); + } + const { nameID } = subject; + logger.info('[samlStrategy] SAML authentication received'); const userEmail = getEmail(profile) || ''; const baseConfig = await getAppConfig({ baseOnly: true }); if (!isEmailDomainAllowed(userEmail, baseConfig?.registration?.allowedDomains)) { logger.error( - `[SAML Strategy] Authentication blocked - email domain not allowed [Email: ${userEmail}]`, + '[samlStrategy] Authentication blocked because the email domain is not allowed', ); return done(null, false, { message: 'Email domain not allowed' }); } - let user = await findUser({ samlId: profile.nameID }); - logger.info( - `[samlStrategy] User ${user ? 'found' : 'not found'} with SAML ID: ${profile.nameID}`, - ); + 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'} with email: ${userEmail}`); + logger.info(`[samlStrategy] User ${user ? 'found' : 'not found'} by SAML email claim`); } if (user && user.provider !== 'saml') { - logger.info( - `[samlStrategy] User ${user.email} already exists with provider ${user.provider}`, - ); + logger.info(`[samlStrategy] SAML login conflicts with existing provider: ${user.provider}`); return done(null, false, { message: ErrorTypes.AUTH_FAILED, }); } - if (user?.samlId && user.samlId !== profile.nameID) { - logger.warn( - `[samlStrategy] Refused SAML login with a different NameID for user: ${user.email}`, - ); + if (user?.samlId && user.samlId !== nameID) { + logger.warn('[samlStrategy] Refused SAML login with a different NameID'); return done(null, false, { message: ErrorTypes.AUTH_FAILED, }); @@ -230,9 +227,7 @@ function createSamlCallback(existingUsersOnly = false) { : baseConfig; if (!isEmailDomainAllowed(userEmail, appConfig?.registration?.allowedDomains)) { - logger.error( - `[SAML Strategy] Authentication blocked - email domain not allowed [Email: ${userEmail}]`, - ); + logger.error('[samlStrategy] Authentication blocked by the tenant email-domain policy'); return done(null, false, { message: 'Email domain not allowed' }); } @@ -244,15 +239,13 @@ function createSamlCallback(existingUsersOnly = false) { if (!user) { if (existingUsersOnly) { - logger.error( - `[samlStrategy] Admin auth blocked - user does not exist [Email: ${userEmail}]`, - ); + 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: profile.nameID, + samlId: nameID, username, email: userEmail, emailVerified: true, @@ -261,10 +254,14 @@ function createSamlCallback(existingUsersOnly = false) { const balanceConfig = getBalanceConfig(appConfig); user = await createUser(user, balanceConfig, true, true); } else { - user.provider = 'saml'; - user.samlId = profile.nameID; - user.username = username; - user.name = fullName; + 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 picture = getPicture(profile); @@ -274,9 +271,9 @@ function createSamlCallback(existingUsersOnly = false) { if (imageBuffer) { let fileName; if (crypto) { - fileName = (await hashToken(profile.nameID)) + '.png'; + fileName = (await hashToken(nameID)) + '.png'; } else { - fileName = profile.nameID + '.png'; + fileName = userId + '.png'; } const fileStrategy = getAvatarFileStrategy(appConfig, process.env.CDN_PROVIDER); @@ -290,22 +287,11 @@ function createSamlCallback(existingUsersOnly = false) { }), ); user.avatar = imagePath ?? ''; + user = await updateUser(user._id, user); } } - user = await updateUser(user._id, user); - - logger.info( - `[samlStrategy] Login success SAML ID: ${user.samlId} | email: ${user.email} | username: ${user.username}`, - { - user: { - samlId: user.samlId, - username: user.username, - email: user.email, - name: user.name, - }, - }, - ); + logger.info(`[samlStrategy] Login success for user: ${user._id}`); done(null, user); } catch (err) { @@ -320,12 +306,17 @@ function createSamlCallback(existingUsersOnly = false) { * @returns {object} The SAML configuration object. */ function getBaseSamlConfig() { + const identifierFormat = process.env.SAML_NAME_ID_FORMAT?.trim(); + if (identifierFormat === TRANSIENT_SAML_NAME_ID_FORMAT) { + throw new Error('SAML_NAME_ID_FORMAT must provide a stable, non-transient identifier'); + } return { entryPoint: process.env.SAML_ENTRY_POINT, issuer: process.env.SAML_ISSUER, idpCert: getCertificateContent(process.env.SAML_CERT), wantAssertionsSigned: process.env.SAML_USE_AUTHN_RESPONSE_SIGNED === 'true' ? false : true, wantAuthnResponseSigned: process.env.SAML_USE_AUTHN_RESPONSE_SIGNED === 'true' ? true : false, + ...(identifierFormat ? { identifierFormat } : {}), }; } diff --git a/api/strategies/samlStrategy.spec.js b/api/strategies/samlStrategy.spec.js index 353ab1df7a..34e06e78c8 100644 --- a/api/strategies/samlStrategy.spec.js +++ b/api/strategies/samlStrategy.spec.js @@ -16,6 +16,7 @@ jest.mock('~/models', () => ({ findUser: jest.fn(), createUser: jest.fn(), updateUser: jest.fn(), + claimSamlIdentity: jest.fn(), })); jest.mock('~/server/services/Config', () => ({ config: { @@ -45,6 +46,8 @@ jest.mock('@librechat/api', () => ({ : params; }), resolveAppConfigForUser: jest.fn(async (_getAppConfig, _user) => ({})), + resolveSamlSubject: jest.fn((profile) => ({ nameID: profile.nameID })), + TRANSIENT_SAML_NAME_ID_FORMAT: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient', })); jest.mock('~/server/services/Config/EndpointService', () => ({ config: {}, @@ -77,15 +80,11 @@ jest.mocked(fs).existsSync = jest.fn(); jest.mocked(fs).statSync = jest.fn(); jest.mocked(fs).readFileSync = jest.fn(); -// To capture the verify callback from the strategy, we grab it from the mock constructor. -// setupSaml() registers both 'saml' (regular) and 'samlAdmin' strategies, so we capture -// only the first callback per setupSaml() call (the regular one). -let verifyCallback; +const verifyCallbacks = new Map(); SamlStrategy.mockImplementation((options, verify) => { - if (!verifyCallback) { - verifyCallback = verify; - } - return { name: 'saml', options, verify }; + const strategyName = options.callbackUrl?.includes('/api/admin/') ? 'samlAdmin' : 'saml'; + verifyCallbacks.set(strategyName, verify); + return { name: strategyName, options, verify }; }); describe('getCertificateContent', () => { @@ -218,9 +217,9 @@ u7wlOSk+oFzDIO/UILIA describe('setupSaml', () => { // Helper to wrap the verify callback in a promise - const validate = (profile) => + const validate = (profile, strategyName = 'saml') => new Promise((resolve, reject) => { - verifyCallback(profile, (err, user, details) => { + verifyCallbacks.get(strategyName)(profile, (err, user, details) => { if (err) { reject(err); } else { @@ -242,11 +241,10 @@ describe('setupSaml', () => { beforeEach(async () => { jest.clearAllMocks(); - // Reset so the mock captures the regular (non-admin) callback on next setupSaml() call - verifyCallback = null; + verifyCallbacks.clear(); // Configure mocks - const { findUser, createUser, updateUser } = require('~/models'); + const { findUser, createUser, updateUser, claimSamlIdentity } = require('~/models'); findUser.mockResolvedValue(null); createUser.mockImplementation(async (userData) => ({ _id: 'mock-user-id', @@ -256,6 +254,11 @@ describe('setupSaml', () => { _id: id, ...userData, })); + claimSamlIdentity.mockImplementation(async (id, samlId, userData) => { + const result = findUser.mock.results[findUser.mock.results.length - 1]; + const existingUser = result ? await result.value : {}; + return { ...existingUser, _id: id, ...userData, samlId }; + }); const cert = ` -----BEGIN CERTIFICATE----- @@ -292,6 +295,8 @@ u7wlOSk+oFzDIO/UILIA delete process.env.SAML_FAMILY_NAME_CLAIM; delete process.env.SAML_PICTURE_CLAIM; delete process.env.SAML_NAME_CLAIM; + delete process.env.SAML_NAME_ID_FORMAT; + delete process.env.SAML_IDP_ISSUER; resizeAvatar.mockResolvedValue(Buffer.from('safe avatar')); @@ -427,8 +432,108 @@ u7wlOSk+oFzDIO/UILIA expect(user.email).toBe(baseProfile.email); }); + it('should preserve a matching NameID binding', async () => { + const { findUser, claimSamlIdentity } = require('~/models'); + const existingUser = { + _id: 'existing-user-id', + provider: 'saml', + email: baseProfile.email, + samlId: baseProfile.nameID, + }; + findUser.mockResolvedValueOnce(existingUser); + + const { user } = await validate(baseProfile); + + expect(user.samlId).toBe(baseProfile.nameID); + expect(claimSamlIdentity).toHaveBeenCalledWith( + existingUser._id, + baseProfile.nameID, + expect.objectContaining({ username: baseProfile.username }), + ); + }); + + it('should atomically bind a legacy SAML account found by email', async () => { + const { findUser, claimSamlIdentity } = require('~/models'); + const existingUser = { + _id: 'legacy-user-id', + provider: 'saml', + email: baseProfile.email, + }; + findUser.mockResolvedValueOnce(null).mockResolvedValueOnce(existingUser); + + const { user } = await validate(baseProfile); + + expect(user.samlId).toBe(baseProfile.nameID); + expect(claimSamlIdentity).toHaveBeenCalledWith( + existingUser._id, + baseProfile.nameID, + expect.objectContaining({ username: baseProfile.username }), + ); + }); + + it('should reject a concurrent first-time binding that loses the atomic claim', async () => { + const { findUser, updateUser, claimSamlIdentity } = require('~/models'); + const existingUser = { + _id: 'legacy-user-id', + provider: 'saml', + email: baseProfile.email, + }; + findUser.mockResolvedValueOnce(null).mockResolvedValueOnce(existingUser); + claimSamlIdentity.mockResolvedValueOnce(null); + + const result = await validate(baseProfile); + + expect(result.user).toBe(false); + expect(result.details.message).toBe(require('librechat-data-provider').ErrorTypes.AUTH_FAILED); + 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'); + resolveSamlSubject.mockReturnValueOnce({ error: 'missing_name_id' }); + + const result = await validate({ ...baseProfile, nameID }); + + expect(result.user).toBe(false); + expect(result.details.message).toBe(require('librechat-data-provider').ErrorTypes.AUTH_FAILED); + expect(findUser).not.toHaveBeenCalled(); + expect(claimSamlIdentity).not.toHaveBeenCalled(); + }); + + it('should reject a transient NameID', async () => { + const { findUser } = require('~/models'); + const { resolveSamlSubject } = require('@librechat/api'); + resolveSamlSubject.mockReturnValueOnce({ error: 'transient_name_id' }); + const result = await validate({ + ...baseProfile, + nameIDFormat: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient', + }); + + expect(result.user).toBe(false); + expect(result.details.message).toBe(require('librechat-data-provider').ErrorTypes.AUTH_FAILED); + expect(findUser).not.toHaveBeenCalled(); + }); + + it('should reject an assertion from a different IdP issuer when configured', async () => { + const { findUser } = require('~/models'); + const { resolveSamlSubject } = require('@librechat/api'); + resolveSamlSubject.mockReturnValueOnce({ error: 'issuer_mismatch' }); + process.env.SAML_IDP_ISSUER = 'https://idp.example.com'; + + const result = await validate({ ...baseProfile, issuer: 'https://other-idp.example.com' }); + + expect(result.user).toBe(false); + expect(result.details.message).toBe(require('librechat-data-provider').ErrorTypes.AUTH_FAILED); + expect(findUser).not.toHaveBeenCalled(); + expect(resolveSamlSubject).toHaveBeenCalledWith( + expect.objectContaining({ issuer: 'https://other-idp.example.com' }), + 'https://idp.example.com', + ); + }); + it('should reject an email match bound to a different NameID', async () => { - const { findUser, updateUser } = require('~/models'); + const { findUser, updateUser, claimSamlIdentity } = require('~/models'); const existingUser = { _id: 'existing-user-id', provider: 'saml', @@ -442,6 +547,24 @@ u7wlOSk+oFzDIO/UILIA expect(result.user).toBe(false); expect(result.details.message).toBe(require('librechat-data-provider').ErrorTypes.AUTH_FAILED); expect(updateUser).not.toHaveBeenCalled(); + expect(claimSamlIdentity).not.toHaveBeenCalled(); + }); + + it('should enforce the NameID binding for the admin SAML callback', async () => { + const { findUser, claimSamlIdentity } = require('~/models'); + const existingUser = { + _id: 'existing-admin-id', + provider: 'saml', + email: baseProfile.email, + samlId: 'original-name-id', + }; + findUser.mockResolvedValueOnce(null).mockResolvedValueOnce(existingUser); + + const result = await validate(baseProfile, 'samlAdmin'); + + expect(result.user).toBe(false); + expect(result.details.message).toBe(require('librechat-data-provider').ErrorTypes.AUTH_FAILED); + expect(claimSamlIdentity).not.toHaveBeenCalled(); }); it('should block login when email exists with different provider', async () => { @@ -504,7 +627,6 @@ u7wlOSk+oFzDIO/UILIA it('uses the configured SAML picture claim for shared avatar processing', async () => { process.env.SAML_PICTURE_CLAIM = 'avatar_url'; - verifyCallback = null; await setupSaml(); const profile = { @@ -522,6 +644,54 @@ u7wlOSk+oFzDIO/UILIA expect(fetch).not.toHaveBeenCalled(); }); + it('should pass the configured NameID format to both SAML strategies', async () => { + process.env.SAML_NAME_ID_FORMAT = 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent'; + process.env.SAML_IDP_ISSUER = 'https://idp.example.com'; + + await setupSaml(); + + const calls = SamlStrategy.mock.calls.slice(-2); + for (const [options] of calls) { + expect(options).toEqual( + expect.objectContaining({ + identifierFormat: 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent', + }), + ); + } + }); + + it('should refuse to configure a transient NameID format', async () => { + const { logger } = require('@librechat/data-schemas'); + process.env.SAML_NAME_ID_FORMAT = 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient'; + const callCount = SamlStrategy.mock.calls.length; + + await setupSaml(); + + expect(SamlStrategy).toHaveBeenCalledTimes(callCount); + expect(logger.error).toHaveBeenCalledWith( + '[samlStrategy]', + expect.objectContaining({ + message: 'SAML_NAME_ID_FORMAT must provide a stable, non-transient identifier', + }), + ); + }); + + it('should not log raw NameID or profile attributes', async () => { + const { logger } = require('@librechat/data-schemas'); + const sensitiveValue = 'sensitive-profile-attribute'; + + await validate({ ...baseProfile, sensitiveAttribute: sensitiveValue }); + + const logOutput = JSON.stringify([ + ...logger.info.mock.calls, + ...logger.debug.mock.calls, + ...logger.warn.mock.calls, + ...logger.error.mock.calls, + ]); + expect(logOutput).not.toContain(baseProfile.nameID); + expect(logOutput).not.toContain(sensitiveValue); + }); + it('should save CloudFront SAML avatars under the shared avatar prefix', async () => { const { getStrategyFunctions } = require('~/server/services/Files/strategies'); getAppConfig.mockResolvedValueOnce({ fileStrategies: { avatar: FileSources.cloudfront } }); diff --git a/packages/api/src/auth/index.ts b/packages/api/src/auth/index.ts index f2b4b2adde..5e1da8e1c5 100644 --- a/packages/api/src/auth/index.ts +++ b/packages/api/src/auth/index.ts @@ -1,5 +1,6 @@ export * from './domain'; export * from './openid'; +export * from './saml'; export * from './proxy'; export * from './exchange'; export * from './refresh'; diff --git a/packages/api/src/auth/saml.spec.ts b/packages/api/src/auth/saml.spec.ts new file mode 100644 index 0000000000..65ecfd22d8 --- /dev/null +++ b/packages/api/src/auth/saml.spec.ts @@ -0,0 +1,41 @@ +import { resolveSamlSubject, TRANSIENT_SAML_NAME_ID_FORMAT, type SamlSubjectProfile } from './saml'; + +describe('resolveSamlSubject', () => { + test.each([ + undefined, + null, + {}, + { nameID: '' }, + { nameID: ' ' }, + ])('rejects a missing or blank NameID: %p', (profile) => { + expect(resolveSamlSubject(profile)).toEqual({ error: 'missing_name_id' }); + }); + + test('rejects a transient NameID', () => { + expect( + resolveSamlSubject({ nameID: 'temporary-id', nameIDFormat: TRANSIENT_SAML_NAME_ID_FORMAT }), + ).toEqual({ error: 'transient_name_id' }); + }); + + test('preserves an opaque NameID exactly', () => { + expect(resolveSamlSubject({ nameID: ' opaque-id ' })).toEqual({ nameID: ' opaque-id ' }); + }); + + test('accepts the configured IdP issuer', () => { + expect( + resolveSamlSubject( + { nameID: 'persistent-id', issuer: 'https://idp.example.com' }, + 'https://idp.example.com', + ), + ).toEqual({ nameID: 'persistent-id' }); + }); + + test.each([undefined, '', 'https://other-idp.example.com'])( + 'rejects a missing or different IdP issuer: %p', + (issuer) => { + expect( + resolveSamlSubject({ nameID: 'persistent-id', issuer }, 'https://idp.example.com'), + ).toEqual({ error: 'issuer_mismatch' }); + }, + ); +}); diff --git a/packages/api/src/auth/saml.ts b/packages/api/src/auth/saml.ts new file mode 100644 index 0000000000..f51a611ae8 --- /dev/null +++ b/packages/api/src/auth/saml.ts @@ -0,0 +1,35 @@ +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'; + +export interface SamlSubjectProfile { + nameID?: string; + nameIDFormat?: string; + issuer?: string; +} + +export type SamlSubjectResolution = + | { nameID: string; error?: never } + | { nameID?: never; error: SamlSubjectError }; + +export function resolveSamlSubject( + profile: SamlSubjectProfile | null | undefined, + expectedIssuer?: string, +): SamlSubjectResolution { + const nameID = profile?.nameID; + if (typeof nameID !== 'string' || nameID.trim().length === 0) { + return { error: 'missing_name_id' }; + } + + if (profile.nameIDFormat === TRANSIENT_SAML_NAME_ID_FORMAT) { + return { error: 'transient_name_id' }; + } + + const normalizedExpectedIssuer = expectedIssuer?.trim(); + const issuer = typeof profile.issuer === 'string' ? profile.issuer.trim() : ''; + if (normalizedExpectedIssuer && issuer !== normalizedExpectedIssuer) { + return { error: 'issuer_mismatch' }; + } + + return { nameID }; +} diff --git a/packages/data-schemas/src/methods/user.methods.spec.ts b/packages/data-schemas/src/methods/user.methods.spec.ts index d7be36e814..bc122b0239 100644 --- a/packages/data-schemas/src/methods/user.methods.spec.ts +++ b/packages/data-schemas/src/methods/user.methods.spec.ts @@ -425,6 +425,140 @@ describe('User Methods - Database Tests', () => { }); }); + describe('claimSamlIdentity', () => { + test('should atomically bind an unbound SAML user', async () => { + const user = await User.create({ + name: 'Legacy SAML User', + email: 'legacy-saml@example.com', + provider: 'saml', + }); + + const updated = await methods.claimSamlIdentity(user._id?.toString() ?? '', 'saml-123', { + name: 'Current SAML User', + }); + + expect(updated).toMatchObject({ + name: 'Current SAML User', + samlId: 'saml-123', + }); + }); + + test('should preserve an existing different SAML binding', async () => { + const user = await User.create({ + email: 'bound-saml@example.com', + provider: 'saml', + samlId: 'original-saml-id', + }); + + const updated = await methods.claimSamlIdentity(user._id?.toString() ?? '', 'new-saml-id', { + name: 'Untrusted Name', + }); + const stored = await User.findById(user._id).lean(); + + expect(updated).toBeNull(); + expect(stored).toMatchObject({ samlId: 'original-saml-id' }); + expect(stored?.name).not.toBe('Untrusted Name'); + }); + + test('should update a user when the SAML binding already matches', async () => { + const user = await User.create({ + email: 'matching-saml@example.com', + provider: 'saml', + samlId: 'saml-123', + }); + + const updated = await methods.claimSamlIdentity(user._id?.toString() ?? '', 'saml-123', { + name: 'Updated Name', + }); + + expect(updated).toMatchObject({ + name: 'Updated Name', + samlId: 'saml-123', + }); + }); + + test('should not convert a user registered with a different provider', async () => { + const user = await User.create({ + email: 'local-user@example.com', + provider: 'local', + }); + + const updated = await methods.claimSamlIdentity(user._id?.toString() ?? '', 'saml-123', {}); + const stored = await User.findById(user._id).lean(); + + expect(updated).toBeNull(); + expect(stored).toMatchObject({ provider: 'local' }); + expect(stored?.samlId).toBeUndefined(); + }); + + test('should allow only one concurrent first-time SAML binding', async () => { + const user = await User.create({ + email: 'concurrent-saml@example.com', + provider: 'saml', + }); + const userId = user._id?.toString() ?? ''; + + const results = await Promise.all([ + methods.claimSamlIdentity(userId, 'saml-a', { name: 'Identity A' }), + methods.claimSamlIdentity(userId, 'saml-b', { name: 'Identity B' }), + ]); + const stored = await User.findById(user._id).lean(); + + expect(results.filter(Boolean)).toHaveLength(1); + expect(['saml-a', 'saml-b']).toContain(stored?.samlId); + expect(results.find(Boolean)?.samlId).toBe(stored?.samlId); + }); + + test('should invalidate cached auth documents after a successful claim', async () => { + enableAuthUserDocCache(); + const user = await User.create({ + email: 'cached-saml@example.com', + provider: 'saml', + }); + const userId = user._id?.toString() ?? ''; + const indexKey = `${AUTH_USER_DOC_BY_ID_PREFIX}:${userId}`; + const cache = { + get: jest.fn().mockResolvedValue(['auth-cache-key']), + delete: jest.fn().mockResolvedValue(true), + }; + const methodsWithCache = createUserMethods(mongoose, { + getCache: jest.fn().mockReturnValue(cache), + }); + + await methodsWithCache.claimSamlIdentity(userId, 'saml-123', {}); + + expect(cache.get).toHaveBeenCalledWith(indexKey); + expect(cache.delete).toHaveBeenCalledWith('auth-cache-key'); + expect(cache.delete).toHaveBeenCalledWith(indexKey); + }); + + test('should not invalidate cached auth documents after a rejected claim', async () => { + enableAuthUserDocCache(); + const user = await User.create({ + email: 'cached-bound-saml@example.com', + provider: 'saml', + samlId: 'original-saml-id', + }); + const cache = { + get: jest.fn(), + delete: jest.fn(), + }; + const getCache = jest.fn().mockReturnValue(cache); + const methodsWithCache = createUserMethods(mongoose, { getCache }); + + const updated = await methodsWithCache.claimSamlIdentity( + user._id?.toString() ?? '', + 'different-saml-id', + {}, + ); + + expect(updated).toBeNull(); + expect(getCache).not.toHaveBeenCalled(); + expect(cache.get).not.toHaveBeenCalled(); + expect(cache.delete).not.toHaveBeenCalled(); + }); + }); + describe('getUserById', () => { test('should get user by ID', async () => { const user = await User.create({ diff --git a/packages/data-schemas/src/methods/user.ts b/packages/data-schemas/src/methods/user.ts index 2840c458b3..d1c4d20b03 100644 --- a/packages/data-schemas/src/methods/user.ts +++ b/packages/data-schemas/src/methods/user.ts @@ -47,6 +47,11 @@ export function createUserMethods( returnUser?: boolean, ) => Promise>; updateUser: (userId: string, updateData: Partial) => Promise; + claimSamlIdentity: ( + userId: string, + samlId: string, + profileData: Pick, 'username' | 'name'>, + ) => Promise; acceptTerms: (userId: string) => Promise; searchUsers: ({ searchPattern, @@ -296,6 +301,31 @@ export function createUserMethods( return updated; } + /** Atomically updates a SAML user only when the incoming identity can claim the document. */ + async function claimSamlIdentity( + userId: string, + samlId: string, + profileData: Pick, 'username' | 'name'>, + ): Promise { + const User = mongoose.models.User; + const updated = await User.findOneAndUpdate( + { + _id: userId, + provider: 'saml', + $or: [{ samlId }, { samlId: { $exists: false } }, { samlId: null }, { samlId: '' }], + }, + { + $set: { ...profileData, samlId }, + $unset: { expiresAt: '' }, + }, + { new: true, runValidators: true }, + ).lean(); + if (updated) { + await invalidateAuthUserDocCache(userId); + } + return updated; + } + async function invalidateAuthUserDocCache(userId: string): Promise { if (!isAuthUserDocCacheEnabled()) { return; @@ -800,6 +830,7 @@ export function createUserMethods( countUsers, createUser, updateUser, + claimSamlIdentity, acceptTerms, searchUsers, getUserById,