🛂 feat: Social Login by Provider ID First then Email (#10358)

api/strategies/appleStrategy.test.js

@@ -304,6 +304,7 @@ describe('Apple Login Strategy', () => {
           fileStrategy: 'local',
           balance: { enabled: false },
         }),
+        'jane.doe@example.com',
       );
     });
 

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<void>}
- *          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);
   }
 };
 

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();
+  });
 });

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(

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`,
+      );
+    });
+  });
+});