diff --git a/api/strategies/openIdJwtStrategy.spec.js b/api/strategies/openIdJwtStrategy.spec.js index 79af848046..fd710f1ebd 100644 --- a/api/strategies/openIdJwtStrategy.spec.js +++ b/api/strategies/openIdJwtStrategy.spec.js @@ -271,6 +271,32 @@ describe('openIdJwtStrategy – OPENID_EMAIL_CLAIM', () => { expect(user).toBe(false); }); + it('should reject login when email fallback finds user with mismatched openidId', async () => { + const emailMatchWithDifferentSub = { + _id: 'user-id-2', + provider: 'openid', + openidId: 'different-sub', + email: payload.email, + role: SystemRoles.USER, + }; + + findUser.mockImplementation(async (query) => { + if (query.$or) { + return null; + } + if (query.email === payload.email) { + return emailMatchWithDifferentSub; + } + return null; + }); + + const req = { headers: { authorization: 'Bearer tok' }, session: {} }; + const { user, info } = await invokeVerify(req, payload); + + expect(user).toBe(false); + expect(info).toEqual({ message: 'auth_failed' }); + }); + it('should trim whitespace from OPENID_EMAIL_CLAIM', async () => { process.env.OPENID_EMAIL_CLAIM = ' upn '; findUser.mockResolvedValue(null); diff --git a/api/strategies/openidStrategy.spec.js b/api/strategies/openidStrategy.spec.js index 16fa548a59..4436fab672 100644 --- a/api/strategies/openidStrategy.spec.js +++ b/api/strategies/openidStrategy.spec.js @@ -356,6 +356,33 @@ describe('setupOpenId', () => { expect(updateUser).not.toHaveBeenCalled(); }); + it('should block login when email fallback finds user with mismatched openidId', async () => { + const existingUser = { + _id: 'existingUserId', + provider: 'openid', + openidId: 'different-sub-claim', + email: tokenset.claims().email, + username: 'existinguser', + name: 'Existing User', + }; + findUser.mockImplementation(async (query) => { + if (query.$or) { + return null; + } + if (query.email === tokenset.claims().email) { + return existingUser; + } + return null; + }); + + const result = await validate(tokenset); + + expect(result.user).toBe(false); + expect(result.details.message).toBe(ErrorTypes.AUTH_FAILED); + expect(createUser).not.toHaveBeenCalled(); + expect(updateUser).not.toHaveBeenCalled(); + }); + it('should enforce the required role and reject login if missing', async () => { // Arrange – simulate a token without the required role. jwtDecode.mockReturnValue({ diff --git a/packages/api/src/auth/openid.spec.ts b/packages/api/src/auth/openid.spec.ts index 7349508ce1..0761a24e85 100644 --- a/packages/api/src/auth/openid.spec.ts +++ b/packages/api/src/auth/openid.spec.ts @@ -107,18 +107,18 @@ describe('findOpenIDUser', () => { }); describe('Email-based searches', () => { - it('should find user by email when primary conditions fail', async () => { + it('should find user by email when primary conditions fail and openidId matches', async () => { const mockUser: IUser = { _id: 'user123', provider: 'openid', - openidId: 'openid_456', + openidId: 'openid_123', email: 'user@example.com', username: 'testuser', } as IUser; mockFindUser - .mockResolvedValueOnce(null) // Primary condition fails - .mockResolvedValueOnce(mockUser); // Email search succeeds + .mockResolvedValueOnce(null) + .mockResolvedValueOnce(mockUser); const result = await findOpenIDUser({ openidId: 'openid_123', @@ -202,7 +202,7 @@ describe('findOpenIDUser', () => { }); }); - it('should allow login when user has openid provider', async () => { + it('should reject email fallback when existing openidId does not match token sub', async () => { const mockUser: IUser = { _id: 'user123', provider: 'openid', @@ -212,8 +212,34 @@ describe('findOpenIDUser', () => { } as IUser; mockFindUser - .mockResolvedValueOnce(null) // Primary condition fails - .mockResolvedValueOnce(mockUser); // Email search finds user with openid provider + .mockResolvedValueOnce(null) + .mockResolvedValueOnce(mockUser); + + const result = await findOpenIDUser({ + openidId: 'openid_123', + findUser: mockFindUser, + email: 'user@example.com', + }); + + expect(result).toEqual({ + user: null, + error: ErrorTypes.AUTH_FAILED, + migration: false, + }); + }); + + it('should allow email fallback when existing openidId matches token sub', async () => { + const mockUser: IUser = { + _id: 'user123', + provider: 'openid', + openidId: 'openid_123', + email: 'user@example.com', + username: 'testuser', + } as IUser; + + mockFindUser + .mockResolvedValueOnce(null) + .mockResolvedValueOnce(mockUser); const result = await findOpenIDUser({ openidId: 'openid_123', @@ -259,7 +285,7 @@ describe('findOpenIDUser', () => { }); }); - it('should not migrate user who already has openidId', async () => { + it('should reject when user already has a different openidId', async () => { const mockUser: IUser = { _id: 'user123', provider: 'openid', @@ -269,8 +295,8 @@ describe('findOpenIDUser', () => { } as IUser; mockFindUser - .mockResolvedValueOnce(null) // Primary condition fails - .mockResolvedValueOnce(mockUser); // Email search finds user with existing openidId + .mockResolvedValueOnce(null) + .mockResolvedValueOnce(mockUser); const result = await findOpenIDUser({ openidId: 'openid_123', @@ -279,24 +305,24 @@ describe('findOpenIDUser', () => { }); expect(result).toEqual({ - user: mockUser, - error: null, + user: null, + error: ErrorTypes.AUTH_FAILED, migration: false, }); }); - it('should handle user with no provider but existing openidId', async () => { + it('should reject when user has no provider but a different openidId', async () => { const mockUser: IUser = { _id: 'user123', openidId: 'existing_openid', email: 'user@example.com', username: 'testuser', - // No provider field + // No provider field — tests a different branch than openid-provider mismatch } as IUser; mockFindUser - .mockResolvedValueOnce(null) // Primary condition fails - .mockResolvedValueOnce(mockUser); // Email search finds user + .mockResolvedValueOnce(null) + .mockResolvedValueOnce(mockUser); const result = await findOpenIDUser({ openidId: 'openid_123', @@ -305,8 +331,8 @@ describe('findOpenIDUser', () => { }); expect(result).toEqual({ - user: mockUser, - error: null, + user: null, + error: ErrorTypes.AUTH_FAILED, migration: false, }); }); @@ -398,14 +424,14 @@ describe('findOpenIDUser', () => { const mockUser: IUser = { _id: 'user123', provider: 'openid', - openidId: 'openid_456', + openidId: 'openid_123', email: 'user@example.com', username: 'testuser', } as IUser; mockFindUser - .mockResolvedValueOnce(null) // Primary condition fails - .mockResolvedValueOnce(mockUser); // Email search succeeds + .mockResolvedValueOnce(null) + .mockResolvedValueOnce(mockUser); const result = await findOpenIDUser({ openidId: 'openid_123', @@ -413,7 +439,6 @@ describe('findOpenIDUser', () => { email: 'User@Example.COM', }); - /** Email is passed as-is; findUser implementation handles normalization */ expect(mockFindUser).toHaveBeenNthCalledWith(2, { email: 'User@Example.COM' }); expect(result).toEqual({ user: mockUser, @@ -432,5 +457,31 @@ describe('findOpenIDUser', () => { }), ).rejects.toThrow('Database error'); }); + + it('should reject email fallback when openidId is empty and user has a stored openidId', async () => { + const mockUser: IUser = { + _id: 'user123', + provider: 'openid', + openidId: 'existing-real-id', + email: 'user@example.com', + username: 'testuser', + } as IUser; + + mockFindUser.mockResolvedValueOnce(mockUser); + + const result = await findOpenIDUser({ + openidId: '', + findUser: mockFindUser, + email: 'user@example.com', + }); + + expect(mockFindUser).toHaveBeenCalledTimes(1); + expect(mockFindUser).toHaveBeenCalledWith({ email: 'user@example.com' }); + expect(result).toEqual({ + user: null, + error: ErrorTypes.AUTH_FAILED, + migration: false, + }); + }); }); }); diff --git a/packages/api/src/auth/openid.ts b/packages/api/src/auth/openid.ts index a7079ccd16..12ff48b2a9 100644 --- a/packages/api/src/auth/openid.ts +++ b/packages/api/src/auth/openid.ts @@ -47,7 +47,13 @@ export async function findOpenIDUser({ return { user: null, error: ErrorTypes.AUTH_FAILED, migration: false }; } - // If user found by email but doesn't have openidId, prepare for migration + if (user?.openidId && user.openidId !== openidId) { + logger.warn( + `[${strategyName}] Rejected email fallback for ${user.email}: stored openidId does not match token sub`, + ); + return { user: null, error: ErrorTypes.AUTH_FAILED, migration: false }; + } + if (user && !user.openidId) { logger.info( `[${strategyName}] Preparing user ${user.email} for migration to OpenID with sub: ${openidId}`,