diff --git a/api/strategies/appleStrategy.test.js b/api/strategies/appleStrategy.test.js index d8ba4616f2..d142d27eac 100644 --- a/api/strategies/appleStrategy.test.js +++ b/api/strategies/appleStrategy.test.js @@ -304,6 +304,7 @@ describe('Apple Login Strategy', () => { fileStrategy: 'local', balance: { enabled: false }, }), + 'jane.doe@example.com', ); }); diff --git a/api/strategies/process.js b/api/strategies/process.js index 8f70cd86ce..c1e0ad0bbc 100644 --- a/api/strategies/process.js +++ b/api/strategies/process.js @@ -5,22 +5,25 @@ const { resizeAvatar } = require('~/server/services/Files/images/avatar'); const { updateUser, createUser, getUserById } = require('~/models'); /** - * Updates the avatar URL of an existing user. If the user's avatar URL does not include the query parameter + * Updates the avatar URL and email of an existing user. If the user's avatar URL does not include the query parameter * '?manual=true', it updates the user's avatar with the provided URL. For local file storage, it directly updates * the avatar URL, while for other storage types, it processes the avatar URL using the specified file strategy. + * Also updates the email if it has changed (e.g., when a Google Workspace email is updated). * * @param {IUser} oldUser - The existing user object that needs to be updated. * @param {string} avatarUrl - The new avatar URL to be set for the user. * @param {AppConfig} appConfig - The application configuration object. + * @param {string} [email] - Optional. The new email address to update if it has changed. * * @returns {Promise} - * The function updates the user's avatar and saves the user object. It does not return any value. + * The function updates the user's avatar and/or email and saves the user object. It does not return any value. * * @throws {Error} Throws an error if there's an issue saving the updated user object. */ -const handleExistingUser = async (oldUser, avatarUrl, appConfig) => { +const handleExistingUser = async (oldUser, avatarUrl, appConfig, email) => { const fileStrategy = appConfig?.fileStrategy ?? process.env.CDN_PROVIDER; const isLocal = fileStrategy === FileSources.local; + const updates = {}; let updatedAvatar = false; const hasManualFlag = @@ -39,7 +42,16 @@ const handleExistingUser = async (oldUser, avatarUrl, appConfig) => { } if (updatedAvatar) { - await updateUser(oldUser._id, { avatar: updatedAvatar }); + updates.avatar = updatedAvatar; + } + + /** Update email if it has changed */ + if (email && email.trim() !== oldUser.email) { + updates.email = email.trim(); + } + + if (Object.keys(updates).length > 0) { + await updateUser(oldUser._id, updates); } }; diff --git a/api/strategies/process.test.js b/api/strategies/process.test.js index ceb7d21a64..ab5fdb651f 100644 --- a/api/strategies/process.test.js +++ b/api/strategies/process.test.js @@ -167,4 +167,76 @@ describe('handleExistingUser', () => { // This should throw an error when trying to access oldUser._id await expect(handleExistingUser(null, avatarUrl)).rejects.toThrow(); }); + + it('should update email when it has changed', async () => { + const oldUser = { + _id: 'user123', + email: 'old@example.com', + avatar: 'https://example.com/avatar.png?manual=true', + }; + const avatarUrl = 'https://example.com/avatar.png'; + const newEmail = 'new@example.com'; + + await handleExistingUser(oldUser, avatarUrl, {}, newEmail); + + expect(updateUser).toHaveBeenCalledWith('user123', { email: 'new@example.com' }); + }); + + it('should update both avatar and email when both have changed', async () => { + const oldUser = { + _id: 'user123', + email: 'old@example.com', + avatar: null, + }; + const avatarUrl = 'https://example.com/new-avatar.png'; + const newEmail = 'new@example.com'; + + await handleExistingUser(oldUser, avatarUrl, {}, newEmail); + + expect(updateUser).toHaveBeenCalledWith('user123', { + avatar: avatarUrl, + email: 'new@example.com', + }); + }); + + it('should not update email when it has not changed', async () => { + const oldUser = { + _id: 'user123', + email: 'same@example.com', + avatar: 'https://example.com/avatar.png?manual=true', + }; + const avatarUrl = 'https://example.com/avatar.png'; + const sameEmail = 'same@example.com'; + + await handleExistingUser(oldUser, avatarUrl, {}, sameEmail); + + expect(updateUser).not.toHaveBeenCalled(); + }); + + it('should trim email before comparison and update', async () => { + const oldUser = { + _id: 'user123', + email: 'test@example.com', + avatar: 'https://example.com/avatar.png?manual=true', + }; + const avatarUrl = 'https://example.com/avatar.png'; + const newEmailWithSpaces = ' newemail@example.com '; + + await handleExistingUser(oldUser, avatarUrl, {}, newEmailWithSpaces); + + expect(updateUser).toHaveBeenCalledWith('user123', { email: 'newemail@example.com' }); + }); + + it('should not update when email parameter is not provided', async () => { + const oldUser = { + _id: 'user123', + email: 'test@example.com', + avatar: 'https://example.com/avatar.png?manual=true', + }; + const avatarUrl = 'https://example.com/avatar.png'; + + await handleExistingUser(oldUser, avatarUrl, {}); + + expect(updateUser).not.toHaveBeenCalled(); + }); }); diff --git a/api/strategies/socialLogin.js b/api/strategies/socialLogin.js index bad70cc040..88fb347042 100644 --- a/api/strategies/socialLogin.js +++ b/api/strategies/socialLogin.js @@ -25,10 +25,24 @@ const socialLogin = return cb(error); } - const existingUser = await findUser({ email: email.trim() }); + 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}`); + } + } if (existingUser?.provider === provider) { - await handleExistingUser(existingUser, avatarUrl, appConfig); + await handleExistingUser(existingUser, avatarUrl, appConfig, email); return cb(null, existingUser); } else if (existingUser) { logger.info( diff --git a/api/strategies/socialLogin.test.js b/api/strategies/socialLogin.test.js new file mode 100644 index 0000000000..11ada17975 --- /dev/null +++ b/api/strategies/socialLogin.test.js @@ -0,0 +1,276 @@ +const { logger } = require('@librechat/data-schemas'); +const { ErrorTypes } = require('librechat-data-provider'); +const { createSocialUser, handleExistingUser } = require('./process'); +const socialLogin = require('./socialLogin'); +const { findUser } = require('~/models'); + +jest.mock('@librechat/data-schemas', () => { + const actualModule = jest.requireActual('@librechat/data-schemas'); + return { + ...actualModule, + logger: { + error: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + }, + }; +}); + +jest.mock('./process', () => ({ + createSocialUser: jest.fn(), + handleExistingUser: jest.fn(), +})); + +jest.mock('@librechat/api', () => ({ + ...jest.requireActual('@librechat/api'), + isEnabled: jest.fn().mockReturnValue(true), + isEmailDomainAllowed: jest.fn().mockReturnValue(true), +})); + +jest.mock('~/models', () => ({ + findUser: jest.fn(), +})); + +jest.mock('~/server/services/Config', () => ({ + getAppConfig: jest.fn().mockResolvedValue({ + fileStrategy: 'local', + balance: { enabled: false }, + }), +})); + +describe('socialLogin', () => { + const mockGetProfileDetails = ({ profile }) => ({ + email: profile.emails[0].value, + id: profile.id, + avatarUrl: profile.photos?.[0]?.value || null, + username: profile.name?.givenName || 'user', + name: `${profile.name?.givenName || ''} ${profile.name?.familyName || ''}`.trim(), + emailVerified: profile.emails[0].verified || false, + }); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('Finding users by provider ID', () => { + it('should find user by provider ID (googleId) when email has changed', async () => { + const provider = 'google'; + const googleId = 'google-user-123'; + const oldEmail = 'old@example.com'; + const newEmail = 'new@example.com'; + + const existingUser = { + _id: 'user123', + email: oldEmail, + provider: 'google', + googleId: googleId, + }; + + /** Mock findUser to return user on first call (by googleId), null on second call */ + findUser + .mockResolvedValueOnce(existingUser) // First call: finds by googleId + .mockResolvedValueOnce(null); // Second call would be by email, but won't be reached + + const mockProfile = { + id: googleId, + emails: [{ value: newEmail, verified: true }], + photos: [{ value: 'https://example.com/avatar.png' }], + name: { givenName: 'John', familyName: 'Doe' }, + }; + + const loginFn = socialLogin(provider, mockGetProfileDetails); + const callback = jest.fn(); + + await loginFn(null, null, null, mockProfile, callback); + + /** Verify it searched by googleId first */ + expect(findUser).toHaveBeenNthCalledWith(1, { googleId: googleId }); + + /** Verify it did NOT search by email (because it found user by googleId) */ + expect(findUser).toHaveBeenCalledTimes(1); + + /** Verify handleExistingUser was called with the new email */ + expect(handleExistingUser).toHaveBeenCalledWith( + existingUser, + 'https://example.com/avatar.png', + expect.any(Object), + newEmail, + ); + + /** Verify callback was called with success */ + expect(callback).toHaveBeenCalledWith(null, existingUser); + }); + + it('should find user by provider ID (facebookId) when using Facebook', async () => { + const provider = 'facebook'; + const facebookId = 'fb-user-456'; + const email = 'user@example.com'; + + const existingUser = { + _id: 'user456', + email: email, + provider: 'facebook', + facebookId: facebookId, + }; + + findUser.mockResolvedValue(existingUser); // Always returns user + + const mockProfile = { + id: facebookId, + emails: [{ value: email, verified: true }], + photos: [{ value: 'https://example.com/fb-avatar.png' }], + name: { givenName: 'Jane', familyName: 'Smith' }, + }; + + const loginFn = socialLogin(provider, mockGetProfileDetails); + const callback = jest.fn(); + + await loginFn(null, null, null, mockProfile, callback); + + /** Verify it searched by facebookId first */ + expect(findUser).toHaveBeenCalledWith({ facebookId: facebookId }); + expect(findUser.mock.calls[0]).toEqual([{ facebookId: facebookId }]); + + expect(handleExistingUser).toHaveBeenCalledWith( + existingUser, + 'https://example.com/fb-avatar.png', + expect.any(Object), + email, + ); + + expect(callback).toHaveBeenCalledWith(null, existingUser); + }); + + it('should fallback to finding user by email if not found by provider ID', async () => { + const provider = 'google'; + const googleId = 'google-user-789'; + const email = 'user@example.com'; + + const existingUser = { + _id: 'user789', + email: email, + provider: 'google', + googleId: 'old-google-id', // Different googleId (edge case) + }; + + /** First call (by googleId) returns null, second call (by email) returns user */ + findUser + .mockResolvedValueOnce(null) // By googleId + .mockResolvedValueOnce(existingUser); // By email + + const mockProfile = { + id: googleId, + emails: [{ value: email, verified: true }], + photos: [{ value: 'https://example.com/avatar.png' }], + name: { givenName: 'Bob', familyName: 'Johnson' }, + }; + + const loginFn = socialLogin(provider, mockGetProfileDetails); + const callback = jest.fn(); + + await loginFn(null, null, null, mockProfile, callback); + + /** Verify both searches happened */ + expect(findUser).toHaveBeenNthCalledWith(1, { googleId: googleId }); + expect(findUser).toHaveBeenNthCalledWith(2, { email: email }); + expect(findUser).toHaveBeenCalledTimes(2); + + /** Verify warning log */ + expect(logger.warn).toHaveBeenCalledWith( + `[${provider}Login] User found by email: ${email} but not by ${provider}Id`, + ); + + expect(handleExistingUser).toHaveBeenCalled(); + expect(callback).toHaveBeenCalledWith(null, existingUser); + }); + + it('should create new user if not found by provider ID or email', async () => { + const provider = 'google'; + const googleId = 'google-new-user'; + const email = 'newuser@example.com'; + + const newUser = { + _id: 'newuser123', + email: email, + provider: 'google', + googleId: googleId, + }; + + /** Both searches return null */ + findUser.mockResolvedValue(null); + createSocialUser.mockResolvedValue(newUser); + + const mockProfile = { + id: googleId, + emails: [{ value: email, verified: true }], + photos: [{ value: 'https://example.com/avatar.png' }], + name: { givenName: 'New', familyName: 'User' }, + }; + + const loginFn = socialLogin(provider, mockGetProfileDetails); + const callback = jest.fn(); + + await loginFn(null, null, null, mockProfile, callback); + + /** Verify both searches happened */ + expect(findUser).toHaveBeenCalledTimes(2); + + /** Verify createSocialUser was called */ + expect(createSocialUser).toHaveBeenCalledWith({ + email: email, + avatarUrl: 'https://example.com/avatar.png', + provider: provider, + providerKey: 'googleId', + providerId: googleId, + username: 'New', + name: 'New User', + emailVerified: true, + appConfig: expect.any(Object), + }); + + expect(callback).toHaveBeenCalledWith(null, newUser); + }); + }); + + describe('Error handling', () => { + it('should return error if user exists with different provider', async () => { + const provider = 'google'; + const googleId = 'google-user-123'; + const email = 'user@example.com'; + + const existingUser = { + _id: 'user123', + email: email, + provider: 'local', // Different provider + }; + + findUser + .mockResolvedValueOnce(null) // By googleId + .mockResolvedValueOnce(existingUser); // By email + + const mockProfile = { + id: googleId, + emails: [{ value: email, verified: true }], + photos: [{ value: 'https://example.com/avatar.png' }], + name: { givenName: 'John', familyName: 'Doe' }, + }; + + const loginFn = socialLogin(provider, mockGetProfileDetails); + const callback = jest.fn(); + + await loginFn(null, null, null, mockProfile, callback); + + /** Verify error callback */ + expect(callback).toHaveBeenCalledWith( + expect.objectContaining({ + code: ErrorTypes.AUTH_FAILED, + provider: 'local', + }), + ); + + expect(logger.info).toHaveBeenCalledWith( + `[${provider}Login] User ${email} already exists with provider local`, + ); + }); + }); +});