From aee69d31520e6624890541f0673e06ba9dea996f Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sat, 13 Jun 2026 08:33:00 +0200 Subject: [PATCH 1/4] fix(users): reset email verification on email change Changing the account email via the self or admin update paths kept emailVerified:true on the brand-new address, letting the OAuth link-by-email flow ({ email, emailVerified: true }) attach a provider identity to an address the account never proved it owns. On email change (mailer configured), reset emailVerified, mint a fresh verification token (24h expiry, same expressions as signup) and fire-and-forget a verify-email mail to the new address. The recover path (internal verification writer) and mailer-less deployments (signup auto-verifies by design) are exempt. Re-sends are covered by the existing resend-verification endpoint. refs #3825 --- modules/users/services/users.service.js | 48 +++ .../users.email.change.integration.tests.js | 284 ++++++++++++++++++ 2 files changed, 332 insertions(+) create mode 100644 modules/users/tests/users.email.change.integration.tests.js diff --git a/modules/users/services/users.service.js b/modules/users/services/users.service.js index 2bfa167dc..447243a90 100644 --- a/modules/users/services/users.service.js +++ b/modules/users/services/users.service.js @@ -1,9 +1,13 @@ /** * Module dependencies */ +import crypto from 'crypto'; import _ from 'lodash'; import config from '../../../config/index.js'; +import logger from '../../../lib/services/logger.js'; +import getBaseUrl from '../../../lib/helpers/getBaseUrl.js'; +import mailer from '../../../lib/helpers/mailer/index.js'; import AuthService from '../../auth/services/auth.service.js'; import UserRepository from '../repositories/users.repository.js'; import MembershipService from '../../organizations/services/organizations.membership.service.js'; @@ -12,6 +16,10 @@ import MembershipRepository from '../../organizations/repositories/organizations import { MEMBERSHIP_ROLES, MEMBERSHIP_STATUSES } from '../../organizations/lib/constants.js'; import { removeSensitive } from '../utils/sanitizeUser.js'; +// Same normalization the repository applies on email lookups (module-local there, +// duplicated here as a one-liner to keep the layering untouched). +const normalizeEmail = (email) => (typeof email === 'string' ? email.toLowerCase().trim() : null); + /** * @desc Function to get all users in db * @param {String} search @@ -85,11 +93,51 @@ const getBrut = async (user) => { * @returns {Promise} updated user (sanitized) */ const update = async (user, body, option) => { + const previousEmail = user.email; if (!option) user = _.assignIn(user, removeSensitive(body, config.whitelists.users.update)); else if (option === 'admin') user = _.assignIn(user, removeSensitive(body, config.whitelists.users.updateAdmin)); else if (option === 'recover') user = _.assignIn(user, removeSensitive(body, config.whitelists.users.recover)); + // #3825 — an email change through the self ('update') or admin ('updateAdmin') + // whitelists must invalidate the verified state: a stale emailVerified:true on a + // brand-new address lets linkProviderByEmail ({ email, emailVerified: true }) + // attach an OAuth identity to an address the account never proved it owns. The + // 'recover' option is exempt — it is the internal writer that SETS verification + // state (verifyEmail, signup). Mailer-less deployments are exempt too: signup + // auto-verifies there by design (trust-any-email), and resetting with no way to + // send a new verification mail would break OAuth linking with no recovery path. + // The whitelist only filters the BODY, so the service can set these fields itself. + let verificationToken = null; + if (option !== 'recover' && mailer.isConfigured()) { + const normalizedNew = normalizeEmail(user.email); + const normalizedPrevious = normalizeEmail(previousEmail); + if (normalizedNew && normalizedNew !== normalizedPrevious) { + verificationToken = crypto.randomBytes(20).toString('hex'); + user.emailVerified = false; + user.emailVerificationToken = verificationToken; + user.emailVerificationExpires = Date.now() + 24 * 3600000; // 24 hours + } + } + const result = await UserRepository.update(user); + + // Fire-and-forget the re-verification mail to the NEW address (same template and + // params shape as the signup verification mail). Never blocks or fails the update; + // re-sends are covered by POST /api/auth/resend-verification. + if (verificationToken) { + mailer.sendMail({ + template: 'verify-email', + to: result.email, + subject: 'Verify your email address', + params: { + displayName: [result.firstName, result.lastName].filter(Boolean).join(' '), + url: `${getBaseUrl()}/verify-email?token=${verificationToken}`, + appName: config.app.title, + appContact: config.app.contact, + }, + }).catch((err) => logger.warn('users.update: email-change verification email failed', { message: err?.message, stack: err?.stack })); + } + return removeSensitive(result); }; diff --git a/modules/users/tests/users.email.change.integration.tests.js b/modules/users/tests/users.email.change.integration.tests.js new file mode 100644 index 000000000..46e72281c --- /dev/null +++ b/modules/users/tests/users.email.change.integration.tests.js @@ -0,0 +1,284 @@ +/** + * Module dependencies. + */ +import request from 'supertest'; +import path from 'path'; +import { jest } from '@jest/globals'; + +import { bootstrap } from '../../../lib/app.js'; +import mongooseService from '../../../lib/services/mongoose.js'; +import mailer from '../../../lib/helpers/mailer/index.js'; + +/** + * Email change must invalidate the verified state (#3825). + * + * Changing the account email through PUT /api/users (self) or + * PUT /api/admin/users/:userId (admin) while keeping emailVerified:true lets + * linkProviderByEmail ({ email, emailVerified: true }) attach an OAuth identity + * to an address the account never proved it owns. On email change the service + * must reset emailVerified, mint a new verification token, and send a + * re-verification mail — but ONLY when the mailer is configured (mailer-less + * deployments auto-verify at signup by design; resetting there would break + * OAuth linking with no recovery path). + * + * The test-env mailer is unconfigured, so signup auto-verifies — the exact + * starting state these tests need. Mailer-configured behavior is simulated + * with jest.spyOn(mailer, 'isConfigured') applied AFTER signup, and + * mailer.sendMail is stubbed so no template read / real send happens. + */ +describe('Email change re-verification integration tests:', () => { + let UserService = null; + let app; + let agent; + let credentials; + let user; + let _user; + + // init + beforeAll(async () => { + try { + const init = await bootstrap(); + UserService = (await import(path.resolve('./modules/users/services/users.service.js'))).default; + app = init.app; + agent = request.agent(app); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + + describe('Logged', () => { + beforeEach(async () => { + credentials = { + email: 'email-change@test.com', + password: 'W@os.jsI$Aw3$0m3', + }; + _user = { + firstName: 'First', + lastName: 'Last', + email: credentials.email, + password: credentials.password, + provider: 'local', + }; + + try { + const result = await agent.post('/api/auth/signup').send(_user).expect(200); + user = result.body.user; + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + + test('should reset emailVerified and mint a new token on email change when mailer is configured', async () => { + const isConfiguredSpy = jest.spyOn(mailer, 'isConfigured').mockReturnValue(true); + const sendMailSpy = jest.spyOn(mailer, 'sendMail').mockResolvedValue(null); + try { + // precondition — signup auto-verified (mailer was off at signup time) + const before = await UserService.getBrut({ id: user.id }); + expect(before.emailVerified).toBe(true); + + await agent.put('/api/users').send({ email: 'email-change-new@test.com' }).expect(200); + + const after = await UserService.getBrut({ id: user.id }); + expect(after.email).toBe('email-change-new@test.com'); + expect(after.emailVerified).toBe(false); + expect(after.emailVerificationToken).toMatch(/^[0-9a-f]{40}$/); + expect(Number(after.emailVerificationExpires)).toBeGreaterThan(Date.now()); + + // re-verification mail fired to the NEW address with the new token + expect(sendMailSpy).toHaveBeenCalledTimes(1); + const mail = sendMailSpy.mock.calls[0][0]; + expect(mail.template).toBe('verify-email'); + expect(mail.to).toBe('email-change-new@test.com'); + expect(mail.params.url).toContain(after.emailVerificationToken); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } finally { + isConfiguredSpy.mockRestore(); + sendMailSpy.mockRestore(); + } + }); + + test('should NOT touch emailVerified on an update without email change', async () => { + const isConfiguredSpy = jest.spyOn(mailer, 'isConfigured').mockReturnValue(true); + const sendMailSpy = jest.spyOn(mailer, 'sendMail').mockResolvedValue(null); + try { + await agent.put('/api/users').send({ firstName: 'StillVerified' }).expect(200); + + const after = await UserService.getBrut({ id: user.id }); + expect(after.firstName).toBe('StillVerified'); + expect(after.emailVerified).toBe(true); + expect(after.emailVerificationToken == null).toBe(true); + expect(sendMailSpy).not.toHaveBeenCalled(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } finally { + isConfiguredSpy.mockRestore(); + sendMailSpy.mockRestore(); + } + }); + + test('should NOT reset when the new email is a case variant of the current one', async () => { + const isConfiguredSpy = jest.spyOn(mailer, 'isConfigured').mockReturnValue(true); + const sendMailSpy = jest.spyOn(mailer, 'sendMail').mockResolvedValue(null); + try { + await agent.put('/api/users').send({ email: 'EMAIL-CHANGE@TEST.COM' }).expect(200); + + const after = await UserService.getBrut({ id: user.id }); + expect(after.email.toLowerCase()).toBe('email-change@test.com'); + expect(after.emailVerified).toBe(true); + expect(after.emailVerificationToken == null).toBe(true); + expect(sendMailSpy).not.toHaveBeenCalled(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } finally { + isConfiguredSpy.mockRestore(); + sendMailSpy.mockRestore(); + } + }); + + test('should keep emailVerified true on email change when mailer is NOT configured', async () => { + // No spies — the test-env mailer is genuinely unconfigured (trust-any-email mode). + try { + await agent.put('/api/users').send({ email: 'email-change-trusted@test.com' }).expect(200); + + const after = await UserService.getBrut({ id: user.id }); + expect(after.email).toBe('email-change-trusted@test.com'); + expect(after.emailVerified).toBe(true); + expect(after.emailVerificationToken == null).toBe(true); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + + test('should stop linkProviderByEmail matching after an email change', async () => { + const isConfiguredSpy = jest.spyOn(mailer, 'isConfigured').mockReturnValue(true); + const sendMailSpy = jest.spyOn(mailer, 'sendMail').mockResolvedValue(null); + try { + // positive control — the verified account on its current email links + const linkedBefore = await UserService.linkProviderByEmail(_user.email, 'google', { id: 'g-before' }); + expect(linkedBefore).toBeTruthy(); + + await agent.put('/api/users').send({ email: 'email-change-annex@test.com' }).expect(200); + + // the changed (now unverified) email must no longer link… + const linkedAfter = await UserService.linkProviderByEmail('email-change-annex@test.com', 'google', { id: 'g-after' }); + expect(linkedAfter).toBeNull(); + // …and the old address no longer exists in db + const linkedOld = await UserService.linkProviderByEmail(_user.email, 'google', { id: 'g-old' }); + expect(linkedOld).toBeNull(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } finally { + isConfiguredSpy.mockRestore(); + sendMailSpy.mockRestore(); + } + }); + + test('should reset emailVerified on the admin update path too', async () => { + // Promote the signed-in user to admin server-side (roles are stripped from + // signup for security — same pattern as user.admin.integration.tests.js). + try { + const brut = await UserService.getBrut({ id: user.id }); + await UserService.update(brut, { roles: ['user', 'admin'] }, 'admin'); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + + const isConfiguredSpy = jest.spyOn(mailer, 'isConfigured').mockReturnValue(true); + const sendMailSpy = jest.spyOn(mailer, 'sendMail').mockResolvedValue(null); + try { + await agent.put(`/api/admin/users/${user.id}`).send({ email: 'email-change-admin@test.com' }).expect(200); + + const after = await UserService.getBrut({ id: user.id }); + expect(after.email).toBe('email-change-admin@test.com'); + expect(after.emailVerified).toBe(false); + expect(after.emailVerificationToken).toMatch(/^[0-9a-f]{40}$/); + expect(sendMailSpy).toHaveBeenCalledTimes(1); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } finally { + isConfiguredSpy.mockRestore(); + sendMailSpy.mockRestore(); + } + }); + + test('should re-verify end-to-end with the new token', async () => { + let token; + const isConfiguredSpy = jest.spyOn(mailer, 'isConfigured').mockReturnValue(true); + const sendMailSpy = jest.spyOn(mailer, 'sendMail').mockResolvedValue(null); + try { + await agent.put('/api/users').send({ email: 'email-change-everify@test.com' }).expect(200); + + const brut = await UserService.getBrut({ id: user.id }); + expect(brut.emailVerified).toBe(false); + token = brut.emailVerificationToken; + expect(token).toMatch(/^[0-9a-f]{40}$/); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } finally { + isConfiguredSpy.mockRestore(); + sendMailSpy.mockRestore(); + } + + try { + const result = await agent.post(`/api/auth/verify-email/${token}`).expect(200); + expect(result.body.type).toBe('success'); + expect(result.body.data.emailVerified).toBe(true); + + const after = await UserService.getBrut({ id: user.id }); + expect(after.emailVerified).toBe(true); + expect(after.emailVerificationToken).toBeNull(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + + test('should not fail the update when the verification mail send rejects', async () => { + const isConfiguredSpy = jest.spyOn(mailer, 'isConfigured').mockReturnValue(true); + const sendMailSpy = jest.spyOn(mailer, 'sendMail').mockRejectedValue(new Error('SMTP down')); + try { + await agent.put('/api/users').send({ email: 'email-change-smtp@test.com' }).expect(200); + + const after = await UserService.getBrut({ id: user.id }); + expect(after.emailVerified).toBe(false); + expect(sendMailSpy).toHaveBeenCalledTimes(1); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } finally { + isConfiguredSpy.mockRestore(); + sendMailSpy.mockRestore(); + } + }); + + afterEach(async () => { + // del user (removal is by id, so it works whatever email the test left behind) + try { + await UserService.remove(user); + } catch (err) { + console.log(err); + } + }); + }); + + // Mongoose disconnect + afterAll(async () => { + try { + await mongooseService.disconnect(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); +}); From 235984c89df914cbbceecb8141ae70e3dfac462d Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sat, 13 Jun 2026 09:11:54 +0200 Subject: [PATCH 2/4] test(users): mock logger/mailer/getBaseUrl in count unit test users.service.js now imports logger (+ mailer + getBaseUrl) for the email-change reset path. The existing count unit test uses a minimal config mock (no `log` key); logger.js calls setupFileLogger() at module-evaluation time, which crashed on config.log.fileLogger. Add jest.unstable_mockModule stubs for the three new deps so the minimal-config test keeps running in isolation. --- .../users/tests/users.service.count.unit.tests.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/modules/users/tests/users.service.count.unit.tests.js b/modules/users/tests/users.service.count.unit.tests.js index 37e7d5c32..5a8777f95 100644 --- a/modules/users/tests/users.service.count.unit.tests.js +++ b/modules/users/tests/users.service.count.unit.tests.js @@ -83,6 +83,20 @@ jest.unstable_mockModule('../../../config/index.js', () => ({ default: { app: {}, db: {} }, })); +// Prevent logger from calling setupFileLogger() with the minimal config mock above +// (logger.js calls setupFileLogger at module-evaluation time; config.log is undefined here). +jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { info: jest.fn(), warn: jest.fn(), error: jest.fn(), debug: jest.fn() }, +})); + +jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ + default: jest.fn(() => 'http://localhost:3000'), +})); + +jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ + default: { sendMail: jest.fn() }, +})); + const { default: UserService } = await import('../services/users.service.js'); /** From 28781ed808329a3f777fa4ee31d680b707efc153 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sat, 13 Jun 2026 09:39:31 +0200 Subject: [PATCH 3/4] test(users): recover-exemption test + count-mock isConfigured Pin that the recover update path never resets emailVerified or sends a re-verification mail (verifyEmail invariant); complete the count unit-test mailer mock with isConfigured. refs #3825 --- .../users.email.change.integration.tests.js | 23 +++++++++++++++++++ .../tests/users.service.count.unit.tests.js | 2 +- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/modules/users/tests/users.email.change.integration.tests.js b/modules/users/tests/users.email.change.integration.tests.js index 46e72281c..624208ade 100644 --- a/modules/users/tests/users.email.change.integration.tests.js +++ b/modules/users/tests/users.email.change.integration.tests.js @@ -262,6 +262,29 @@ describe('Email change re-verification integration tests:', () => { } }); + test('should NOT reset emailVerified or send mail on the recover update path', async () => { + // 'recover' is the internal verification writer (verifyEmail / password reset) — it + // SETS verification state, so the email-change guard is exempt for it (option !== 'recover'). + // Even an email in the recover body must not trigger a reset or a re-verification mail, + // else verifyEmail would clobber the very emailVerified:true it just wrote. + const isConfiguredSpy = jest.spyOn(mailer, 'isConfigured').mockReturnValue(true); + const sendMailSpy = jest.spyOn(mailer, 'sendMail').mockResolvedValue(null); + try { + const brut = await UserService.getBrut({ id: user.id }); + await UserService.update(brut, { email: 'recover-change@test.com', emailVerified: true }, 'recover'); + + const after = await UserService.getBrut({ id: user.id }); + expect(after.emailVerified).toBe(true); + expect(sendMailSpy).not.toHaveBeenCalled(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } finally { + isConfiguredSpy.mockRestore(); + sendMailSpy.mockRestore(); + } + }); + afterEach(async () => { // del user (removal is by id, so it works whatever email the test left behind) try { diff --git a/modules/users/tests/users.service.count.unit.tests.js b/modules/users/tests/users.service.count.unit.tests.js index 5a8777f95..2965096ac 100644 --- a/modules/users/tests/users.service.count.unit.tests.js +++ b/modules/users/tests/users.service.count.unit.tests.js @@ -94,7 +94,7 @@ jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ })); jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ - default: { sendMail: jest.fn() }, + default: { sendMail: jest.fn(), isConfigured: jest.fn().mockReturnValue(false) }, })); const { default: UserService } = await import('../services/users.service.js'); From 7b9e45b10f76121b70482d20c2986b7e274512aa Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sat, 13 Jun 2026 09:55:15 +0200 Subject: [PATCH 4/4] docs(users): document normalizeEmail contract + repo-sync coupling Adds a JSDoc for the helper (nullability) and the deliberate duplication of the repository-layer normalization. Addresses review on #3854. refs #3825 --- modules/users/services/users.service.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/modules/users/services/users.service.js b/modules/users/services/users.service.js index 447243a90..50d758e2d 100644 --- a/modules/users/services/users.service.js +++ b/modules/users/services/users.service.js @@ -16,8 +16,16 @@ import MembershipRepository from '../../organizations/repositories/organizations import { MEMBERSHIP_ROLES, MEMBERSHIP_STATUSES } from '../../organizations/lib/constants.js'; import { removeSensitive } from '../utils/sanitizeUser.js'; -// Same normalization the repository applies on email lookups (module-local there, -// duplicated here as a one-liner to keep the layering untouched). +/** + * @function normalizeEmail + * @desc Lowercase + trim an email for case-insensitive comparison. MUST stay in sync + * with the repository-layer normalization (UserRepository, module-local there) — + * duplicated here as a one-liner to keep the layering untouched; if the repository + * normalization ever changes (e.g. Unicode fold), update both so the email-change + * guard's previous-vs-new comparison cannot drift. + * @param {string} email - raw email value + * @returns {string|null} normalized email, or null for a non-string input + */ const normalizeEmail = (email) => (typeof email === 'string' ? email.toLowerCase().trim() : null); /**