diff --git a/src/__tests__/controllers/UserController.test.ts b/src/__tests__/controllers/UserController.test.ts index 6ef6b2a..5caee0e 100644 --- a/src/__tests__/controllers/UserController.test.ts +++ b/src/__tests__/controllers/UserController.test.ts @@ -99,3 +99,88 @@ describe('UserController.secondOnboarding', () => { expect(res.json).toHaveBeenCalledWith({}); }); }); + +describe('UserController.initLogin', () => { + let userService: any; + let controller: UserController; + + beforeEach(() => { + userService = { + verifyToken: jest.fn(), + getAuthMethodByExternalId: jest.fn(), + getUser: jest.fn(), + addNewUser: jest.fn(), + addAuthMethod: jest.fn(), + addNewDevice: jest.fn(), + tokenUser: jest.fn<() => Promise>().mockResolvedValue('jwt'), + }; + controller = new UserController(userService, {} as any); + }); + + const appleLogin = () => + controller.initLogin({ body: { token_id: 'apple-token' }, headers: {} } as any, makeRes()); + + it("links Apple to an account whose email differs in case, and the token carries the account's email", async () => { + // The account was created by a passkey sign-up typed lowercase; Apple keeps the casing + userService.verifyToken.mockResolvedValue({ email: 'Jane.Doe@Example.com', sub: 'apple-sub' }); + userService.getAuthMethodByExternalId.mockResolvedValue(null); + userService.getUser.mockResolvedValue({ + id_user: 7, + email: 'jane.doe@example.com', + external_id: 'uuid-7', + }); + + await appleLogin(); + + expect(userService.addNewUser).not.toHaveBeenCalled(); + expect(userService.addAuthMethod).toHaveBeenCalledWith( + expect.objectContaining({ user_id: 7, auth_type: 'apple', is_primary: false }), + ); + // An email-prefixed account's files live under its own spelling + expect(userService.tokenUser).toHaveBeenCalledWith( + expect.objectContaining({ id_user: 7, email: 'jane.doe@example.com' }), + ); + }); + + it("keeps the account's email in the token on later sign-ins with the linked credential", async () => { + userService.verifyToken.mockResolvedValue({ email: 'Jane.Doe@Example.com', sub: 'apple-sub' }); + userService.getAuthMethodByExternalId.mockResolvedValue({ + user_id: 7, + id_auth_method: 1, + email: 'jane.doe@example.com', + }); + userService.getUser.mockResolvedValue({ + id_user: 7, + email: 'jane.doe@example.com', + external_id: 'uuid-7', + session: 'apple-sub', + }); + + await appleLogin(); + + expect(userService.tokenUser).toHaveBeenCalledWith( + expect.objectContaining({ id_user: 7, email: 'jane.doe@example.com' }), + ); + }); + + it("keeps the provider's email in the token when it changed altogether", async () => { + userService.verifyToken.mockResolvedValue({ email: 'new@example.com', sub: 'apple-sub' }); + userService.getAuthMethodByExternalId.mockResolvedValue({ + user_id: 7, + id_auth_method: 1, + email: 'old@example.com', + }); + userService.getUser.mockResolvedValue({ + id_user: 7, + email: 'old@example.com', + external_id: 'uuid-7', + session: 'apple-sub', + }); + + await appleLogin(); + + expect(userService.tokenUser).toHaveBeenCalledWith( + expect.objectContaining({ id_user: 7, email: 'new@example.com' }), + ); + }); +}); diff --git a/src/__tests__/database/usersLowerEmailUnique.test.ts b/src/__tests__/database/usersLowerEmailUnique.test.ts new file mode 100644 index 0000000..c3e7a3a --- /dev/null +++ b/src/__tests__/database/usersLowerEmailUnique.test.ts @@ -0,0 +1,27 @@ +import { describe, it, expect } from '@jest/globals'; +import { getTestTransaction, createTestUser } from '../setup'; + +// Migration 20261013120000: account lookups ignore the email's case, so only +// one active account may hold an email, whatever its case. +describe('users_lower_email_active_unique', () => { + it('rejects a second active account whose email differs only in case', async () => { + const trx = getTestTransaction(); + await createTestUser(trx, { email: 'Jane.Doe@Example.com' }); + + // A savepoint, so the failed insert doesn't abort the test's transaction + await expect( + trx.transaction((savepoint) => + createTestUser(savepoint, { email: 'jane.doe@example.com' }), + ), + ).rejects.toThrow(/users_lower_email_active_unique/); + }); + + it('allows it once the other account is inactive', async () => { + const trx = getTestTransaction(); + await createTestUser(trx, { email: 'Gone.Doe@Example.com', active: false }); + + const user = await createTestUser(trx, { email: 'gone.doe@example.com' }); + + expect(user.email).toBe('gone.doe@example.com'); + }); +}); diff --git a/src/__tests__/services/EmailVerificationService.test.ts b/src/__tests__/services/EmailVerificationService.test.ts index 7604ef4..9777d1d 100644 --- a/src/__tests__/services/EmailVerificationService.test.ts +++ b/src/__tests__/services/EmailVerificationService.test.ts @@ -1,10 +1,12 @@ import { describe, it, expect, beforeEach } from '@jest/globals'; import { EmailVerificationService } from '../../services/EmailVerificationService'; +import { UserServices } from '../../services/UserServices'; import { getTestTransaction, mockEmailService, mockLoggerService, mockUserService, + createTestUser, createTestVerificationCode, } from '../setup'; import moment from 'moment'; @@ -116,6 +118,23 @@ describe('EmailVerificationService', () => { expect(mockEmailService.sendEmail).not.toHaveBeenCalled(); }); + it('should refuse an email an account already has in another case', async () => { + const trx = getTestTransaction(); + // An Apple sign-in stored the address with its casing + await createTestUser(trx, { email: 'Jane.Doe@Example.com' }); + const userService = new UserServices(); + (userService as any)._userDB.db = trx; + (userService as any)._userDB._logger = mockLoggerService; + (service as any)._userService = userService; + + // A passkey sign-up on the other app, typed lowercase + const result = await service.sendVerificationCode('jane.doe@example.com'); + + expect(result.success).toBe(false); + expect(result.error_code).toBe('EMAIL_ALREADY_REGISTERED'); + expect(mockEmailService.sendEmail).not.toHaveBeenCalled(); + }); + it('should allow sending after rate limit window expires', async () => { const trx = getTestTransaction(); const email = 'test@example.com'; diff --git a/src/__tests__/services/PasskeyService.test.ts b/src/__tests__/services/PasskeyService.test.ts index 5686b05..08d080c 100644 --- a/src/__tests__/services/PasskeyService.test.ts +++ b/src/__tests__/services/PasskeyService.test.ts @@ -162,6 +162,17 @@ describe('PasskeyService', () => { expect(result!.external_id).toBe(user.external_id); }); + it('should find the user whatever the email casing', async () => { + const trx = getTestTransaction(); + const user = await createTestUser(trx, { email: 'Jane.Doe@Example.com' }); + + const result = await service.getUserByEmail('jane.doe@example.com'); + + expect(result).not.toBeNull(); + expect(result!.id_user).toBe(user.id_user); + expect(result!.email).toBe('Jane.Doe@Example.com'); + }); + it('should return null for non-existent email', async () => { const result = await service.getUserByEmail('nonexistent@example.com'); expect(result).toBeNull(); diff --git a/src/__tests__/services/UserServices.test.ts b/src/__tests__/services/UserServices.test.ts index a75c6cb..4c4dc03 100644 --- a/src/__tests__/services/UserServices.test.ts +++ b/src/__tests__/services/UserServices.test.ts @@ -19,6 +19,31 @@ describe('UserServices - Duplicate Prevention', () => { mockLoggerService.log.mockClear(); }); + describe('GetUser', () => { + it('should find the account whatever the email casing', async () => { + const trx = getTestTransaction(); + + // Apple keeps the casing of the address; the other app's user types it lowercase + const user = await createTestUser(trx, { email: 'Jane.Doe@Example.com' }); + + const result = await service.getUser({ email: 'jane.doe@example.com' }); + + expect(result).toBeDefined(); + expect(result.id_user).toBe(user.id_user); + expect(result.email).toBe('Jane.Doe@Example.com'); + }); + + it('should not return an inactive account', async () => { + const trx = getTestTransaction(); + + await createTestUser(trx, { email: 'Gone@Example.com', active: false }); + + const result = await service.getUser({ email: 'gone@example.com' }); + + expect(result).toBeUndefined(); + }); + }); + describe('GetAuthMethodByExternalId', () => { it('should return auth method and user info when found', async () => { const trx = getTestTransaction(); diff --git a/src/controllers/UserController.ts b/src/controllers/UserController.ts index e79af9b..02647be 100644 --- a/src/controllers/UserController.ts +++ b/src/controllers/UserController.ts @@ -143,7 +143,14 @@ export class UserController { const token = await this._userService.tokenUser({ id_user: user.id_user, - email: authData.email, + // This claim is the S3 prefix of an account that predates external_id + // storage. Accounts are found by email whatever its case, so the + // provider may spell it differently than the account: carry the + // account's. A provider email that changed altogether is kept, as before. + email: + authData.email.toLowerCase() === user.email.toLowerCase() + ? user.email + : authData.email, external_id: user.external_id, session: authData.external_id, }); diff --git a/src/database/migrations/20261013120000_users_lower_email_active_unique.ts b/src/database/migrations/20261013120000_users_lower_email_active_unique.ts new file mode 100644 index 0000000..ab26d21 --- /dev/null +++ b/src/database/migrations/20261013120000_users_lower_email_active_unique.ts @@ -0,0 +1,25 @@ +import type { Knex } from 'knex'; + +// CREATE INDEX CONCURRENTLY can't run inside a transaction, and it keeps +// `users` writable (sign-ups) while the index builds. +export const config = { transaction: false }; + +// One active account per email, whatever its case. Account lookups match +// `lower(email)`, which `users_email_active_unique` can't serve, and an exact +// match had split people into two accounts (`Jane@` from Apple, `jane@` typed +// at a passkey sign-up). Those pairs were merged before this ran; if one is +// left, the build fails, and the migration can run again once it's merged. +export async function up(knex: Knex): Promise { + // A concurrent build that fails leaves an INVALID index the planner never + // uses; IF NOT EXISTS would then skip it for good. Drop any leftover first. + await knex.raw('DROP INDEX CONCURRENTLY IF EXISTS users_lower_email_active_unique'); + await knex.raw(` + CREATE UNIQUE INDEX CONCURRENTLY users_lower_email_active_unique + ON users (lower(email)) + WHERE active = true + `); +} + +export async function down(knex: Knex): Promise { + await knex.raw('DROP INDEX CONCURRENTLY IF EXISTS users_lower_email_active_unique'); +} diff --git a/src/services/db/PasskeyDB.ts b/src/services/db/PasskeyDB.ts index 99b2611..c8bf28f 100644 --- a/src/services/db/PasskeyDB.ts +++ b/src/services/db/PasskeyDB.ts @@ -305,8 +305,10 @@ export class PasskeyDB { ): Promise { try { const db = trx || this.db; + // Case-insensitive: see UserDB.getUser. const user = await db('users') - .where({ email, active: true }) + .whereRaw('lower(email) = lower(?)', [email]) + .where('active', true) .select('id_user', 'email', 'external_id', 'active') .first(); return user || null; diff --git a/src/services/db/UserDB.ts b/src/services/db/UserDB.ts index c9a18a6..df970f7 100644 --- a/src/services/db/UserDB.ts +++ b/src/services/db/UserDB.ts @@ -29,10 +29,11 @@ export class UserDB { db.raw('?', [session || '']), ); }) - .where({ - email, - 'usr.active': true, - }) + // Apple keeps the address's casing (`Jane@example.com`) while a typed + // one is often lowercase; an exact match split one person into two + // accounts. users_lower_email_active_unique keeps it to one. + .whereRaw('lower(usr.email) = lower(?)', [email]) + .where('usr.active', true) .first(); return user; } catch (err) {