From bec9b580bcf55ddc4682af302b9ce697c29a5514 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Fri, 4 Sep 2026 13:50:06 +0200 Subject: [PATCH 1/4] refactor(auth): extract signup service out of the auth controller Move the account-signup business logic (capacity gate, invite eligibility, mass-assignment scrubbing, user creation, email verification, organization provisioning, analytics, invite finalize/release) out of auth.controller.signup into a new auth.signup.service.js. The controller keeps only the response block (token/cookie/JSON) and error mapping. isMailerConfigured/sendVerificationEmail move with it (both had callers outside signup - getConfig and resendVerification - which now re-import them from the service). The capacity/eligibility gate rejection is signaled via an AppError with code SIGNUP_DISABLED so the controller reconstructs the exact original 404 response instead of the generic 422 fallback. Pure refactor - behaviour unchanged. Verified against a baseline run of the unmodified auth suites (14/82 unit, 5/122 integration, 1/4 e2e); the refactored branch reproduces the same counts. Refs #3995 Claude-Session: https://claude.ai/code/session_0185ELiCjZaBJx8PH4xoSsZb --- modules/auth/controllers/auth.controller.js | 307 +---------------- modules/auth/services/auth.signup.service.js | 345 +++++++++++++++++++ 2 files changed, 360 insertions(+), 292 deletions(-) create mode 100644 modules/auth/services/auth.signup.service.js diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js index fe174f94e..0389e7d41 100644 --- a/modules/auth/controllers/auth.controller.js +++ b/modules/auth/controllers/auth.controller.js @@ -8,9 +8,9 @@ import jwt from 'jsonwebtoken'; import UserService from '../../users/services/users.service.js'; import Eligibility from '../services/auth.eligibility.js'; import { computeSignupCapacity } from '../services/auth.signupCapacity.js'; +import SignupService, { isMailerConfigured, sendVerificationEmail } from '../services/auth.signup.service.js'; import config from '../../../config/index.js'; import model from '../../../lib/middlewares/model.js'; -import mails from '../../../lib/helpers/mailer/index.js'; import responses from '../../../lib/helpers/responses.js'; import errors from '../../../lib/helpers/errors.js'; import AppError from '../../../lib/helpers/AppError.js'; @@ -22,6 +22,7 @@ import OrganizationCrudService from '../../organizations/services/organizations. import MembershipService from '../../organizations/services/organizations.membership.service.js'; import AnalyticsService from '../../../lib/services/analytics.js'; import logger from '../../../lib/services/logger.js'; +import getBaseUrl from '../../../lib/helpers/getBaseUrl.js'; const tokenCookieOptions = { httpOnly: true, @@ -30,302 +31,15 @@ const tokenCookieOptions = { }; /** - * @desc Check whether the mailer is configured with a real sender address. - * Delegates to the centralized helper in lib/helpers/mailer. - * @returns {boolean} true when SMTP mail sending is available - */ -const isMailerConfigured = () => mails.isConfigured(); - -import getBaseUrl from '../../../lib/helpers/getBaseUrl.js'; - -/** - * @desc Send a verification email to the user with a signed token link - * @param {Object} user - User object (must have email, firstName, lastName) - * @param {string} verificationToken - The email verification token - * @returns {Promise} nodemailer send result - */ -const sendVerificationEmail = async (user, verificationToken) => { - const mail = await mails.sendMail({ - template: 'verify-email', - to: user.email, - subject: 'Verify your email address', - params: { - displayName: [user.firstName, user.lastName].filter(Boolean).join(' '), - url: `${getBaseUrl()}/verify-email?token=${verificationToken}`, - appName: config.app.title, - appContact: config.app.contact, - }, - }); - return mail; -}; - -/** - * @desc Flatten a persisted `attribution` subdocument into PostHog-style - * snake_case event properties. Only present keys are included (absent - * attribution, or an absent individual field, contributes nothing) — mirrors - * the "only keys that are present" contract for the `user_signed_up` event. - * @param {Object|undefined} attribution - persisted attribution subdocument - * @returns {Object} flattened snake_case properties, possibly empty - */ -const attributionEventProperties = (attribution) => { - if (!attribution || typeof attribution !== 'object') return {}; - const map = { - referrer: 'referrer', - landingPath: 'landing_path', - utmSource: 'utm_source', - utmMedium: 'utm_medium', - utmCampaign: 'utm_campaign', - utmTerm: 'utm_term', - utmContent: 'utm_content', - }; - const properties = {}; - for (const [camelKey, snakeKey] of Object.entries(map)) { - if (attribution[camelKey] !== undefined) properties[snakeKey] = attribution[camelKey]; - } - return properties; -}; - -/** - * @desc Endpoint to ask the service to create a user + * @desc Endpoint to ask the service to create a user. Business logic lives in + * auth.signup.service.js — this handler only invokes it and shapes the HTTP + * response (token + cookie + JSON body) or maps a thrown error to a status. * @param {Object} req - Express request object * @param {Object} res - Express response object */ const signup = async (req, res) => { try { - // Two AND-ed gates: (1) capacity — a hard ceiling on total accounts, - // invited users included; (2) eligibility — public signup open OR a valid - // invite token. The eligibility check is supplied by optional modules via the - // generic registry (auth never imports invitation code). The invitations - // checker resolves the email-pinned invite, atomically CLAIMS it when required - // (closed signup) or opted into (open signup + `invitations.userFacing`, #3981; - // a lost claim race there downgrades to unclaimed instead of blocking signup — - // see invitations.init.js), and RETURNS `{ invite, claimed, finalize, release }`, - // which auth relays back here verbatim (opaque result) so this controller can - // canonicalize the account email + finalize/release the invite below. - // E4: cap is computed by computeSignupCapacity (single source of truth shared - // with getConfig) — a BLANK cap ('') means UNCAPPED. The old inline Number('')→0 - // hard-rejected everyone while getConfig advertised the deployment as open. - // remaining<=0 (and cap>0) ⇒ the ceiling is reached. Accepted TOCTOU: cap is - // checked then the user created non-atomically, so a burst of concurrent signups - // can overshoot the cap by a few accounts (a small beta overshoot, tolerated). - const { cap, remaining } = await computeSignupCapacity(config.sign?.cap, UserService.count); - // Note the `cap > 0` guard: a `cap:0` deployment is NOT `capReached` here. cap:0 - // ({cap:0, remaining:0} per computeSignupCapacity) means "closed to the public", - // and its rejection rides the `!sign.up && !invite` arm below — invites intentionally - // bypass a zero cap (computeSignupCapacity short-circuits the count for cap<=0), so an - // invited signup under cap:0 still passes the gate. capReached is only for a real - // positive ceiling that filled up (which DOES reject invites — they count in the cap). - const capReached = cap != null && cap > 0 && remaining <= 0; - // `signupOpen` tells the checker whether the invite is REQUIRED to open the gate. - // When public signup is open a token may be PRESENTED but is not required — by - // default (`invitations.userFacing: false`) the checker resolves WITHOUT claiming - // (E2), so an open-signup signup never burns / locks a presented token (preserves - // the P2 `!config.sign.up` gating). With `userFacing: true` the checker DOES claim - // it (#3981), but open signup's own invariant — a presented token must never be - // able to fail an otherwise-valid signup — still holds: a lost claim race there - // downgrades to unclaimed rather than throwing (see invitations.init.js). - const eligibility = await Eligibility.assertSignupEligible({ email: req.body.email, body: req.body, req, signupOpen: !!config.sign.up }); - // null when no optional module opened the gate (registry empty or no valid invite). - const invite = eligibility?.invite || null; - // #3981: whether the invite needs finalize on success / release on failure is - // decided by `eligibility.claimed` — relayed verbatim from the checker that - // actually called claim() (invitations.init.js), the single source of truth for - // "was this atomically claimed". Reading it here rather than re-deriving the - // closed-signup / userFacing condition a second time from config avoids the two - // sides ever drifting out of lockstep (a duplicated condition could finalize an - // invite that was never claimed, or leave a claimed one stuck) — auth stays - // import-free of invitation code either way, since `claimed` is just a boolean on - // the opaque relayed result, not a call into invitations. - const inviteHonored = !!eligibility?.claimed; - if (capReached || (!config.sign.up && !invite)) { - // On the closed-signup (or userFacing open-signup) path the eligibility checker - // CLAIMED the invite (E2) before the cap was found exhausted — release it so a - // cap bump later does not leave it stuck mid-claim (the lazy sweep would also - // recover it; this is immediate). Gate the release on `inviteHonored` to avoid a - // no-op release (+ misleading log) on a presented-but-never-claimed token. - if (inviteHonored) { - try { - await eligibility?.release?.(); - } catch (releaseErr) { - // Best-effort — the sweep recovers; log it so a burst of "signup blocked" - // reports is diagnosable (a silently stuck claim looks like a dead invite). - logger.warn('[signup] invite release failed on capacity gate (left to the sweep)', { - err: releaseErr?.message, - stack: releaseErr?.stack, - }); - } - } - return responses.error(res, 404, 'Signup error', 'Registration is currently deactivated')(); - } - // Force default role on public signup — clients must not self-assign admin. - // Defense-in-depth against mass assignment: the SignupUser route schema already - // rejects server-owned keys (.strict()), but UserService.create does NO whitelist - // filtering, so we ALSO scrub the body here. Force roles + emailVerified, and delete - // every server-owned field a client could otherwise seed (provider identity, reset / - // verification tokens, lockout counters). emailVerified:true would self-verify the - // account and defeat the OAuth-annexation guard (linkProviderByEmail matches on - // emailVerified:true); a pre-seeded providerData enables identity hijack. - // `referredBy` is accepted by SignupUser (preventing a .strict() 422 on invite paths - // that may send it) but ALWAYS deleted here — the server sets it via the invite - // finalize seam so a client can never self-assign a referrer. - const safeBody = { ...req.body, roles: ['user'], emailVerified: false }; - for (const serverOwned of [ - 'providerData', - 'additionalProvidersData', - 'resetPasswordToken', - 'resetPasswordExpires', - 'emailVerificationToken', - 'emailVerificationExpires', - 'failedLoginAttempts', - 'lockUntil', - 'lastLoginAt', - 'currentOrganization', - 'referredBy', - ]) delete safeBody[serverOwned]; - // First-touch attribution (#4002/#4003) is a legitimate client-provided field - // (unlike the server-owned list above), but the feature is inert unless the - // PostHog client actually initialized — nothing would ever read it back, so - // strip it before create rather than persist dead data. Gate on - // AnalyticsService.isConfigured() (client !== null) rather than - // config.analytics.posthog.enabled directly: `enabled:true` with no `key` set - // never initializes the client (see lib/services/analytics.js#init), so the - // config flag alone would silently persist attribution nobody ever reads. - // When configured, attribution flows into UserService.create untouched - // (already validated + trimmed + length-capped by SignupUser's `.strict()` - // Attribution shape) and is flattened onto the `user_signed_up` capture event - // below. - if (!AnalyticsService.isConfigured()) delete safeBody.attribution; - // Invite-gated signup: canonicalize the account email to the invite's pinned - // (lowercased) email. Enforces the pin exactly AND makes the case-insensitive - // unique-email index (email_ci_unique, collation strength-2) a reliable single-use backstop — concurrent case-variant - // signups on the same invite collide on the index instead of creating two accounts. - if (invite) safeBody.email = invite.email; - // E2: the invite was atomically CLAIMED (consumingAt stamped) before we got here, - // so a throw FROM create itself must release the claim too — otherwise the invite - // stays locked until the 15-min sweep. The most realistic throw is an E11000 from - // the case-insensitive unique-email index (email_ci_unique) when two case-variant signups race the same - // invited email (validation/transient errors land here as well). Mirror the three - // release sites below + the same `inviteHonored` gating (only a claimed invite — - // closed signup, or userFacing open signup — needs releasing). Best-effort: a - // release failure must not mask the create error. - let user; - try { - user = await UserService.create(safeBody); - } catch (createErr) { - if (inviteHonored) { try { await eligibility?.release?.(); } catch (_releaseErr) { /* best-effort */ } } - throw createErr; - } - - // Handle email verification — rollback user on failure to avoid orphaned accounts - try { - if (isMailerConfigured()) { - // Generate verification token and persist it - const verificationToken = crypto.randomBytes(20).toString('hex'); - const brutUser = await UserService.getBrut({ id: user.id }); - await UserService.update(brutUser, { - emailVerificationToken: verificationToken, - emailVerificationExpires: Date.now() + 24 * 3600000, // 24 hours - }, 'recover'); - // Send verification email (best-effort, do not block signup) - sendVerificationEmail(user, verificationToken).catch((err) => logger.warn('auth.signup: verification email failed', { message: err?.message, stack: err?.stack })); - } else if (!invite) { - // No mailer configured — auto-verify so dev/test are not blocked. - // E6: do NOT auto-verify an INVITE-created account even with the mailer off. - // The token proves the INVITER knew the address, not that the SIGNER controls - // it — an invited account must follow the normal verification path (it just - // won't receive the email when the mailer is off, same as any account). - const brutUser = await UserService.getBrut({ id: user.id }); - await UserService.update(brutUser, { emailVerified: true }, 'recover'); - user.emailVerified = true; - } - } catch (verifyErr) { - try { await UserService.remove(user); } catch (_cleanupErr) { /* best-effort */ } - // E2: a claimed invite must be released on a pre-response failure so the token - // is reusable (it was only claimed, never finalized). Gate the release on - // `inviteHonored` — only a claimed invite (closed signup, or userFacing open - // signup) needs releasing. - if (inviteHonored) { try { await eligibility?.release?.(); } catch (_releaseErr) { /* best-effort */ } } - throw verifyErr; - } - - // Handle organization provisioning based on config - // If org creation fails, rollback the just-created user - let orgResult; - try { - orgResult = await AuthOrganizationService.handleSignupOrganization(user); - } catch (orgErr) { - // Manual rollback: delete the user we just created - try { - await UserService.remove(user); - } catch (_cleanupErr) { - // Best-effort cleanup; log but don't mask original error - } - // E2: release the claimed invite on org-provisioning failure so it can retry - // (gate on `inviteHonored` — only a claimed invite needs releasing). - if (inviteHonored) { try { await eligibility?.release?.(); } catch (_releaseErr) { /* best-effort */ } } - throw orgErr; - } - - // Analytics — fire-and-forget, never break signup flow - try { - AnalyticsService.identify(String(user.id), { - email: user.email, - firstName: user.firstName, - lastName: user.lastName, - provider: user.provider, - }); - AnalyticsService.capture({ - distinctId: String(user.id), - event: 'user_signed_up', - properties: { - email: user.email, - plan: user.plan, - createdAt: user.createdAt, - // #3945: carry invite/referral attribution on the signup event so the - // referral funnel is measurable. `invite` is the resolved (opaque) result - // from the eligibility registry — already in scope, no invitations import. - invited: Boolean(invite), - invitationId: invite ? String(invite.id) : null, - invitedBy: invite?.invitedBy ? String(invite.invitedBy) : null, - // #4002/#4003: first-touch attribution, flattened PostHog-style. Read - // from `safeBody` (the object actually handed to UserService.create), - // NOT the sanitized `user` response — `attribution` is deliberately - // absent from `config.whitelists.users.default`, so `UserService.create`'s - // `removeSensitive()` return would always strip it regardless of whether - // it was actually persisted. Empty when analytics was disabled at create - // time (stripped from safeBody above) or when none was submitted. - ...attributionEventProperties(safeBody.attribution), - }, - }); - } catch (_) { /* analytics must not break auth */ } - - // E2 single-use: FINALIZE only when the invite was actually CLAIMED — closed - // signup (the invite was required), or open signup with `invitations.userFacing` - // on (#3981: a presented token still converts even though it wasn't required — - // closes the open-signup hole documented in the invitations README). Otherwise a - // token can be presented but is not required, and the checker never claimed it, - // so there is nothing to finalize. finalize burns single-use (usedAt + - // status:'accepted') and records the user; it runs through the closure returned - // by the eligibility checker (invitations module owns it; auth never imports - // invitation code). This is the last pre-response step, and every earlier failure - // path (create-throw, verify-failure, org-failure) already released the claim - // under the same `inviteHonored` condition, so reaching finalize means the claim - // is still ours to burn. finalize itself is best-effort (see catch below). - if (inviteHonored) { - try { - await eligibility?.finalize?.(user._id || user.id); - } catch (finalizeErr) { - // Best-effort: the account exists and the response is about to succeed — - // a finalize DB hiccup must not convert a created account into a 422. - // The claim stays stamped and the 15-min lazy sweep releases it; the - // invite is reconcilable from its pending state + the account's email. - logger.warn('[signup] invite finalize failed post-create (left to the sweep)', { - userId: String(user._id || user.id), - err: finalizeErr?.message, - stack: finalizeErr?.stack, - }); - } - } + const { user, orgResult } = await SignupService.signup(req); const token = jwt.sign({ userId: user.id }, config.jwt.secret, { expiresIn: config.jwt.expiresIn, @@ -356,6 +70,15 @@ const signup = async (req, res) => { message: 'Sign up', }); } catch (err) { + // The capacity/eligibility gate (auth.signup.service.js) signals via this code + // so the ORIGINAL 404 response is reproduced byte-for-byte — status, title, + // description, and no `err` argument to responses.error (matches the inline + // call this replaced) — instead of falling through to the generic 422 mapping + // below. Every distinct error status this flow can produce keeps its own + // status/title/message; none are collapsed into a shared helper. + if (err?.code === 'SIGNUP_DISABLED') { + return responses.error(res, 404, 'Signup error', 'Registration is currently deactivated')(); + } responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err); } }; diff --git a/modules/auth/services/auth.signup.service.js b/modules/auth/services/auth.signup.service.js new file mode 100644 index 000000000..f1c2ddef7 --- /dev/null +++ b/modules/auth/services/auth.signup.service.js @@ -0,0 +1,345 @@ +/** + * Module dependencies + */ +import crypto from 'crypto'; + +import UserService from '../../users/services/users.service.js'; +import Eligibility from './auth.eligibility.js'; +import { computeSignupCapacity } from './auth.signupCapacity.js'; +import config from '../../../config/index.js'; +import mails from '../../../lib/helpers/mailer/index.js'; +import AppError from '../../../lib/helpers/AppError.js'; +import AuthOrganizationService from '../../organizations/services/organizations.service.js'; +import AnalyticsService from '../../../lib/services/analytics.js'; +import logger from '../../../lib/services/logger.js'; +import getBaseUrl from '../../../lib/helpers/getBaseUrl.js'; + +/** + * @desc Check whether the mailer is configured with a real sender address. + * Delegates to the centralized helper in lib/helpers/mailer. Controller-local + * helper (not a mailer-lib export) — auth.controller re-imports this for its + * own `getConfig` and `resendVerification` endpoints, which call it directly. + * @returns {boolean} true when SMTP mail sending is available + */ +export const isMailerConfigured = () => mails.isConfigured(); + +/** + * @desc Send a verification email to the user with a signed token link. + * Controller-local helper (not a mailer-lib export) — auth.controller + * re-imports this for its own `resendVerification` endpoint, which calls it + * directly. + * @param {Object} user - User object (must have email, firstName, lastName) + * @param {string} verificationToken - The email verification token + * @returns {Promise} nodemailer send result + */ +export const sendVerificationEmail = async (user, verificationToken) => { + const mail = await mails.sendMail({ + template: 'verify-email', + to: user.email, + subject: 'Verify your email address', + params: { + displayName: [user.firstName, user.lastName].filter(Boolean).join(' '), + url: `${getBaseUrl()}/verify-email?token=${verificationToken}`, + appName: config.app.title, + appContact: config.app.contact, + }, + }); + return mail; +}; + +/** + * @desc Flatten a persisted `attribution` subdocument into PostHog-style + * snake_case event properties. Only present keys are included (absent + * attribution, or an absent individual field, contributes nothing) — mirrors + * the "only keys that are present" contract for the `user_signed_up` event. + * @param {Object|undefined} attribution - persisted attribution subdocument + * @returns {Object} flattened snake_case properties, possibly empty + */ +const attributionEventProperties = (attribution) => { + if (!attribution || typeof attribution !== 'object') return {}; + const map = { + referrer: 'referrer', + landingPath: 'landing_path', + utmSource: 'utm_source', + utmMedium: 'utm_medium', + utmCampaign: 'utm_campaign', + utmTerm: 'utm_term', + utmContent: 'utm_content', + }; + const properties = {}; + for (const [camelKey, snakeKey] of Object.entries(map)) { + if (attribution[camelKey] !== undefined) properties[snakeKey] = attribution[camelKey]; + } + return properties; +}; + +/** + * @desc Run the full account-signup flow: capacity + invite-eligibility + * gating, mass-assignment scrubbing, user creation, email verification, + * organization provisioning, analytics and invite finalize/release. + * + * HTTP-agnostic: thrown errors propagate to the caller (auth.controller's + * `signup`) for status mapping. The capacity/eligibility gate rejection is + * signaled via an AppError carrying `code: 'SIGNUP_DISABLED'` rather than + * writing a response directly, so the caller can reconstruct the exact + * original 404 response (status, title, description — no `err` argument) + * instead of falling through to the generic 422 mapping. Every distinct + * error status this flow can produce keeps its own status/title/message; + * none are collapsed into a shared helper. + * @param {Object} req - Express request object. `req.body` is read verbatim + * (email, and every signup field); the whole object is also relayed + * opaquely as `ctx.req` to the eligibility registry (an invite checker may + * read `req.query`/`req.body` for a presented token — see + * modules/invitations/invitations.init.js). + * @returns {Promise<{user: Object, orgResult: Object}>} the created user + * (post-verification, sanitized by UserService.create) and the + * organization-provisioning result — together, everything + * auth.controller.signup's response block reads. + */ +const signup = async (req) => { + // Two AND-ed gates: (1) capacity — a hard ceiling on total accounts, + // invited users included; (2) eligibility — public signup open OR a valid + // invite token. The eligibility check is supplied by optional modules via the + // generic registry (auth never imports invitation code). The invitations + // checker resolves the email-pinned invite, atomically CLAIMS it when required + // (closed signup) or opted into (open signup + `invitations.userFacing`, #3981; + // a lost claim race there downgrades to unclaimed instead of blocking signup — + // see invitations.init.js), and RETURNS `{ invite, claimed, finalize, release }`, + // which auth relays back here verbatim (opaque result) so this service can + // canonicalize the account email + finalize/release the invite below. + // E4: cap is computed by computeSignupCapacity (single source of truth shared + // with getConfig) — a BLANK cap ('') means UNCAPPED. The old inline Number('')→0 + // hard-rejected everyone while getConfig advertised the deployment as open. + // remaining<=0 (and cap>0) ⇒ the ceiling is reached. Accepted TOCTOU: cap is + // checked then the user created non-atomically, so a burst of concurrent signups + // can overshoot the cap by a few accounts (a small beta overshoot, tolerated). + const { cap, remaining } = await computeSignupCapacity(config.sign?.cap, UserService.count); + // Note the `cap > 0` guard: a `cap:0` deployment is NOT `capReached` here. cap:0 + // ({cap:0, remaining:0} per computeSignupCapacity) means "closed to the public", + // and its rejection rides the `!sign.up && !invite` arm below — invites intentionally + // bypass a zero cap (computeSignupCapacity short-circuits the count for cap<=0), so an + // invited signup under cap:0 still passes the gate. capReached is only for a real + // positive ceiling that filled up (which DOES reject invites — they count in the cap). + const capReached = cap != null && cap > 0 && remaining <= 0; + // `signupOpen` tells the checker whether the invite is REQUIRED to open the gate. + // When public signup is open a token may be PRESENTED but is not required — by + // default (`invitations.userFacing: false`) the checker resolves WITHOUT claiming + // (E2), so an open-signup signup never burns / locks a presented token (preserves + // the P2 `!config.sign.up` gating). With `userFacing: true` the checker DOES claim + // it (#3981), but open signup's own invariant — a presented token must never be + // able to fail an otherwise-valid signup — still holds: a lost claim race there + // downgrades to unclaimed rather than throwing (see invitations.init.js). + const eligibility = await Eligibility.assertSignupEligible({ email: req.body.email, body: req.body, req, signupOpen: !!config.sign.up }); + // null when no optional module opened the gate (registry empty or no valid invite). + const invite = eligibility?.invite || null; + // #3981: whether the invite needs finalize on success / release on failure is + // decided by `eligibility.claimed` — relayed verbatim from the checker that + // actually called claim() (invitations.init.js), the single source of truth for + // "was this atomically claimed". Reading it here rather than re-deriving the + // closed-signup / userFacing condition a second time from config avoids the two + // sides ever drifting out of lockstep (a duplicated condition could finalize an + // invite that was never claimed, or leave a claimed one stuck) — auth stays + // import-free of invitation code either way, since `claimed` is just a boolean on + // the opaque relayed result, not a call into invitations. + const inviteHonored = !!eligibility?.claimed; + if (capReached || (!config.sign.up && !invite)) { + // On the closed-signup (or userFacing open-signup) path the eligibility checker + // CLAIMED the invite (E2) before the cap was found exhausted — release it so a + // cap bump later does not leave it stuck mid-claim (the lazy sweep would also + // recover it; this is immediate). Gate the release on `inviteHonored` to avoid a + // no-op release (+ misleading log) on a presented-but-never-claimed token. + if (inviteHonored) { + try { + await eligibility?.release?.(); + } catch (releaseErr) { + // Best-effort — the sweep recovers; log it so a burst of "signup blocked" + // reports is diagnosable (a silently stuck claim looks like a dead invite). + logger.warn('[signup] invite release failed on capacity gate (left to the sweep)', { + err: releaseErr?.message, + stack: releaseErr?.stack, + }); + } + } + // See this function's own doc comment: the caller (auth.controller.signup) + // matches on `code: 'SIGNUP_DISABLED'` to reconstruct the exact original 404 + // response rather than the generic 422 fallback. + throw new AppError('Signup error', { + status: 404, + code: 'SIGNUP_DISABLED', + details: { message: 'Registration is currently deactivated' }, + }); + } + // Force default role on public signup — clients must not self-assign admin. + // Defense-in-depth against mass assignment: the SignupUser route schema already + // rejects server-owned keys (.strict()), but UserService.create does NO whitelist + // filtering, so we ALSO scrub the body here. Force roles + emailVerified, and delete + // every server-owned field a client could otherwise seed (provider identity, reset / + // verification tokens, lockout counters). emailVerified:true would self-verify the + // account and defeat the OAuth-annexation guard (linkProviderByEmail matches on + // emailVerified:true); a pre-seeded providerData enables identity hijack. + // `referredBy` is accepted by SignupUser (preventing a .strict() 422 on invite paths + // that may send it) but ALWAYS deleted here — the server sets it via the invite + // finalize seam so a client can never self-assign a referrer. + const safeBody = { ...req.body, roles: ['user'], emailVerified: false }; + for (const serverOwned of [ + 'providerData', + 'additionalProvidersData', + 'resetPasswordToken', + 'resetPasswordExpires', + 'emailVerificationToken', + 'emailVerificationExpires', + 'failedLoginAttempts', + 'lockUntil', + 'lastLoginAt', + 'currentOrganization', + 'referredBy', + ]) delete safeBody[serverOwned]; + // First-touch attribution (#4002/#4003) is a legitimate client-provided field + // (unlike the server-owned list above), but the feature is inert unless the + // PostHog client actually initialized — nothing would ever read it back, so + // strip it before create rather than persist dead data. Gate on + // AnalyticsService.isConfigured() (client !== null) rather than + // config.analytics.posthog.enabled directly: `enabled:true` with no `key` set + // never initializes the client (see lib/services/analytics.js#init), so the + // config flag alone would silently persist attribution nobody ever reads. + // When configured, attribution flows into UserService.create untouched + // (already validated + trimmed + length-capped by SignupUser's `.strict()` + // Attribution shape) and is flattened onto the `user_signed_up` capture event + // below. + if (!AnalyticsService.isConfigured()) delete safeBody.attribution; + // Invite-gated signup: canonicalize the account email to the invite's pinned + // (lowercased) email. Enforces the pin exactly AND makes the case-insensitive + // unique-email index (email_ci_unique, collation strength-2) a reliable single-use backstop — concurrent case-variant + // signups on the same invite collide on the index instead of creating two accounts. + if (invite) safeBody.email = invite.email; + // E2: the invite was atomically CLAIMED (consumingAt stamped) before we got here, + // so a throw FROM create itself must release the claim too — otherwise the invite + // stays locked until the 15-min sweep. The most realistic throw is an E11000 from + // the case-insensitive unique-email index (email_ci_unique) when two case-variant signups race the same + // invited email (validation/transient errors land here as well). Mirror the three + // release sites below + the same `inviteHonored` gating (only a claimed invite — + // closed signup, or userFacing open signup — needs releasing). Best-effort: a + // release failure must not mask the create error. + let user; + try { + user = await UserService.create(safeBody); + } catch (createErr) { + if (inviteHonored) { try { await eligibility?.release?.(); } catch (_releaseErr) { /* best-effort */ } } + throw createErr; + } + + // Handle email verification — rollback user on failure to avoid orphaned accounts + try { + if (isMailerConfigured()) { + // Generate verification token and persist it + const verificationToken = crypto.randomBytes(20).toString('hex'); + const brutUser = await UserService.getBrut({ id: user.id }); + await UserService.update(brutUser, { + emailVerificationToken: verificationToken, + emailVerificationExpires: Date.now() + 24 * 3600000, // 24 hours + }, 'recover'); + // Send verification email (best-effort, do not block signup) + sendVerificationEmail(user, verificationToken).catch((err) => logger.warn('auth.signup: verification email failed', { message: err?.message, stack: err?.stack })); + } else if (!invite) { + // No mailer configured — auto-verify so dev/test are not blocked. + // E6: do NOT auto-verify an INVITE-created account even with the mailer off. + // The token proves the INVITER knew the address, not that the SIGNER controls + // it — an invited account must follow the normal verification path (it just + // won't receive the email when the mailer is off, same as any account). + const brutUser = await UserService.getBrut({ id: user.id }); + await UserService.update(brutUser, { emailVerified: true }, 'recover'); + user.emailVerified = true; + } + } catch (verifyErr) { + try { await UserService.remove(user); } catch (_cleanupErr) { /* best-effort */ } + // E2: a claimed invite must be released on a pre-response failure so the token + // is reusable (it was only claimed, never finalized). Gate the release on + // `inviteHonored` — only a claimed invite (closed signup, or userFacing open + // signup) needs releasing. + if (inviteHonored) { try { await eligibility?.release?.(); } catch (_releaseErr) { /* best-effort */ } } + throw verifyErr; + } + + // Handle organization provisioning based on config + // If org creation fails, rollback the just-created user + let orgResult; + try { + orgResult = await AuthOrganizationService.handleSignupOrganization(user); + } catch (orgErr) { + // Manual rollback: delete the user we just created + try { + await UserService.remove(user); + } catch (_cleanupErr) { + // Best-effort cleanup; log but don't mask original error + } + // E2: release the claimed invite on org-provisioning failure so it can retry + // (gate on `inviteHonored` — only a claimed invite needs releasing). + if (inviteHonored) { try { await eligibility?.release?.(); } catch (_releaseErr) { /* best-effort */ } } + throw orgErr; + } + + // Analytics — fire-and-forget, never break signup flow + try { + AnalyticsService.identify(String(user.id), { + email: user.email, + firstName: user.firstName, + lastName: user.lastName, + provider: user.provider, + }); + AnalyticsService.capture({ + distinctId: String(user.id), + event: 'user_signed_up', + properties: { + email: user.email, + plan: user.plan, + createdAt: user.createdAt, + // #3945: carry invite/referral attribution on the signup event so the + // referral funnel is measurable. `invite` is the resolved (opaque) result + // from the eligibility registry — already in scope, no invitations import. + invited: Boolean(invite), + invitationId: invite ? String(invite.id) : null, + invitedBy: invite?.invitedBy ? String(invite.invitedBy) : null, + // #4002/#4003: first-touch attribution, flattened PostHog-style. Read + // from `safeBody` (the object actually handed to UserService.create), + // NOT the sanitized `user` response — `attribution` is deliberately + // absent from `config.whitelists.users.default`, so `UserService.create`'s + // `removeSensitive()` return would always strip it regardless of whether + // it was actually persisted. Empty when analytics was disabled at create + // time (stripped from safeBody above) or when none was submitted. + ...attributionEventProperties(safeBody.attribution), + }, + }); + } catch (_) { /* analytics must not break auth */ } + + // E2 single-use: FINALIZE only when the invite was actually CLAIMED — closed + // signup (the invite was required), or open signup with `invitations.userFacing` + // on (#3981: a presented token still converts even though it wasn't required — + // closes the open-signup hole documented in the invitations README). Otherwise a + // token can be presented but is not required, and the checker never claimed it, + // so there is nothing to finalize. finalize burns single-use (usedAt + + // status:'accepted') and records the user; it runs through the closure returned + // by the eligibility checker (invitations module owns it; auth never imports + // invitation code). This is the last pre-response step, and every earlier failure + // path (create-throw, verify-failure, org-failure) already released the claim + // under the same `inviteHonored` condition, so reaching finalize means the claim + // is still ours to burn. finalize itself is best-effort (see catch below). + if (inviteHonored) { + try { + await eligibility?.finalize?.(user._id || user.id); + } catch (finalizeErr) { + // Best-effort: the account exists and the response is about to succeed — + // a finalize DB hiccup must not convert a created account into a 422. + // The claim stays stamped and the 15-min lazy sweep releases it; the + // invite is reconcilable from its pending state + the account's email. + logger.warn('[signup] invite finalize failed post-create (left to the sweep)', { + userId: String(user._id || user.id), + err: finalizeErr?.message, + stack: finalizeErr?.stack, + }); + } + } + + return { user, orgResult }; +}; + +export default { signup, isMailerConfigured, sendVerificationEmail }; From fd379637674f31a91910d483c1de0af4b9f10927 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Fri, 4 Sep 2026 13:54:05 +0200 Subject: [PATCH 2/4] test(auth): direct unit coverage for auth.signup.service gates Add auth.signup.service.unit.tests.js covering the gates that previously needed an HTTP round-trip: capacity rejection, invite release on the capacity gate (claimed and unclaimed), invite release + user rollback on organization-provisioning failure, no auto-verify for an invited account when the mailer is off (plus a control for the plain-signup case that still auto-verifies), and finalize ordering (strictly after organization provisioning). Each test verified red-before/green-after by temporarily breaking the behaviour it covers (disabling the capacity gate, skipping the org-failure rollback, dropping the invite guard on auto-verify, and reordering finalize before organization provisioning) and restoring afterward. Refs #3995 Claude-Session: https://claude.ai/code/session_0185ELiCjZaBJx8PH4xoSsZb --- .../tests/auth.signup.service.unit.tests.js | 279 ++++++++++++++++++ 1 file changed, 279 insertions(+) create mode 100644 modules/auth/tests/auth.signup.service.unit.tests.js diff --git a/modules/auth/tests/auth.signup.service.unit.tests.js b/modules/auth/tests/auth.signup.service.unit.tests.js new file mode 100644 index 000000000..c622f9015 --- /dev/null +++ b/modules/auth/tests/auth.signup.service.unit.tests.js @@ -0,0 +1,279 @@ +/** + * Module dependencies. + */ +import { jest, describe, test, expect } from '@jest/globals'; + +/** + * Unit tests — auth.signup.service.js (issue #3995). + * + * Direct unit coverage for gates that, before the extraction, could only be + * exercised through an HTTP round-trip via auth.controller.signup: + * - capacity rejection + * - invite release on the capacity gate + * - invite release on organization-provisioning failure + * - no auto-verify for an invited account when the mailer is off + * - finalize ordering (after organization provisioning, last pre-response step) + * + * No DB, no HTTP — every leaf dependency (UserService, the eligibility + * registry, AuthOrganizationService, mailer, analytics, logger) is mocked. + * computeSignupCapacity is left real (pure function, no external deps) so + * only UserService.count needs to be controlled per scenario. + */ + +/** + * @desc Register every auth.signup.service dependency EXCEPT config/eligibility, + * which are supplied per test so each scenario can vary sign.up/cap/invite + * outcome. Must run before the dynamic import of auth.signup.service.js in + * each test (jest.resetModules() + jest.unstable_mockModule are call-order + * sensitive). + * @param {Object} args + * @param {Object} args.config - the mocked config module default export + * @param {Object} [args.eligibility] - the mocked auth.eligibility default export + * @param {Function} [args.create] - UserService.create mock implementation + * @param {Function} [args.update] - UserService.update mock implementation + * @param {Function} [args.remove] - UserService.remove mock implementation + * @param {Function} [args.count] - UserService.count mock implementation + * @param {Function} [args.handleSignupOrganization] - AuthOrganizationService.handleSignupOrganization mock implementation + * @param {boolean} [args.mailerConfigured=false] - mails.isConfigured() return value + * @returns {void} + */ +function mockSignupServiceDeps({ + config, + eligibility, + create, + update, + remove, + count, + handleSignupOrganization, + mailerConfigured = false, +}) { + jest.resetModules(); + + jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { warn: jest.fn(), error: jest.fn(), info: jest.fn() }, + })); + + jest.unstable_mockModule('../../../config/index.js', () => ({ default: config })); + + jest.unstable_mockModule('../../users/services/users.service.js', () => ({ + default: { + create: create || jest.fn().mockResolvedValue({ + id: 'u1', email: 'x@y.com', firstName: 'A', lastName: 'B', provider: 'local', emailVerified: false, + }), + getBrut: jest.fn().mockResolvedValue({ id: 'u1' }), + update: update || jest.fn().mockResolvedValue({}), + remove: remove || jest.fn().mockResolvedValue({}), + count: count || jest.fn().mockResolvedValue(0), + }, + })); + + jest.unstable_mockModule('../services/auth.eligibility.js', () => ({ + default: eligibility || { + assertSignupEligible: jest.fn().mockResolvedValue(undefined), + }, + })); + + jest.unstable_mockModule('../../organizations/services/organizations.service.js', () => ({ + default: { + handleSignupOrganization: handleSignupOrganization || jest.fn().mockResolvedValue({ + organization: null, abilities: [], emailVerificationRequired: false, + }), + }, + })); + + jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ + default: { + isConfigured: jest.fn().mockReturnValue(mailerConfigured), + sendMail: jest.fn().mockResolvedValue({ accepted: ['x@y.com'] }), + }, + })); + + jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({ + default: { + identify: jest.fn(), + capture: jest.fn(), + isConfigured: jest.fn().mockReturnValue(false), + }, + })); + + jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ + default: jest.fn().mockReturnValue('http://localhost:3000'), + })); +} + +const baseConfig = (overrides = {}) => ({ + sign: { up: true, ...overrides.sign }, + app: { title: 'Test', contact: 'test@test.com' }, +}); + +/** + * @desc Build a mocked eligibility default export carrying a resolved invite, + * the `claimed` flag, and spy-able finalize/release closures — mirrors the + * shape the invitations checker returns (see auth.signup.inviteHonored.unit.tests.js). + * @param {Object} [invite] - resolved invite doc + * @param {Boolean} [claimed] - whether the checker actually claimed this invite + * @returns {{ eligibility: Object, finalize: jest.Mock, release: jest.Mock }} + */ +function mockEligibilityWithInvite(invite = { id: 'inv1', email: 'invitee@test.com', invitedBy: 'inviter1' }, claimed = true) { + const finalize = jest.fn().mockResolvedValue({ id: invite.id, status: 'accepted' }); + const release = jest.fn().mockResolvedValue({ id: invite.id }); + const eligibility = { + assertSignupEligible: jest.fn().mockResolvedValue({ invite, claimed, finalize, release }), + }; + return { eligibility, finalize, release }; +} + +describe('auth.signup.service — capacity gate (#3995)', () => { + test('rejects with SIGNUP_DISABLED when the cap is reached, and never calls UserService.create', async () => { + const create = jest.fn(); + mockSignupServiceDeps({ + config: baseConfig({ sign: { up: true, cap: 1 } }), + count: jest.fn().mockResolvedValue(1), // remaining = cap(1) - count(1) = 0 ⇒ capReached + create, + }); + + const { default: SignupService } = await import('../services/auth.signup.service.js'); + const req = { body: { email: 'new@test.com', firstName: 'A', lastName: 'B', password: 'P@ss1234!' }, query: {} }; + + await expect(SignupService.signup(req)).rejects.toMatchObject({ + code: 'SIGNUP_DISABLED', + status: 404, + details: { message: 'Registration is currently deactivated' }, + }); + expect(create).not.toHaveBeenCalled(); + }); + + test('a claimed invite is RELEASED when the capacity gate rejects the signup', async () => { + const { eligibility, finalize, release } = mockEligibilityWithInvite(); + const create = jest.fn(); + mockSignupServiceDeps({ + config: baseConfig({ sign: { up: true, cap: 1 } }), + count: jest.fn().mockResolvedValue(1), + eligibility, + create, + }); + + const { default: SignupService } = await import('../services/auth.signup.service.js'); + const req = { body: { email: 'invitee@test.com', firstName: 'A', lastName: 'B', password: 'P@ss1234!' }, query: { inviteToken: 'tok' } }; + + await expect(SignupService.signup(req)).rejects.toMatchObject({ code: 'SIGNUP_DISABLED' }); + expect(release).toHaveBeenCalledTimes(1); + expect(finalize).not.toHaveBeenCalled(); + expect(create).not.toHaveBeenCalled(); + }); + + test('an UNCLAIMED (presented-only) invite is NOT released when the capacity gate rejects the signup', async () => { + const { eligibility, finalize, release } = mockEligibilityWithInvite(undefined, false); + mockSignupServiceDeps({ + config: baseConfig({ sign: { up: true, cap: 1 } }), + count: jest.fn().mockResolvedValue(1), + eligibility, + }); + + const { default: SignupService } = await import('../services/auth.signup.service.js'); + const req = { body: { email: 'invitee@test.com', firstName: 'A', lastName: 'B', password: 'P@ss1234!' }, query: { inviteToken: 'tok' } }; + + await expect(SignupService.signup(req)).rejects.toMatchObject({ code: 'SIGNUP_DISABLED' }); + expect(release).not.toHaveBeenCalled(); + expect(finalize).not.toHaveBeenCalled(); + }); +}); + +describe('auth.signup.service — organization-provisioning failure (#3995)', () => { + test('releases a claimed invite AND rolls back the created user when handleSignupOrganization throws', async () => { + const { eligibility, finalize, release } = mockEligibilityWithInvite(); + const remove = jest.fn().mockResolvedValue({}); + const orgErr = new Error('org provisioning DB error'); + mockSignupServiceDeps({ + config: baseConfig({ sign: { up: false } }), + eligibility, + remove, + handleSignupOrganization: jest.fn().mockRejectedValue(orgErr), + }); + + const { default: SignupService } = await import('../services/auth.signup.service.js'); + const req = { body: { email: 'invitee@test.com', firstName: 'A', lastName: 'B', password: 'P@ss1234!' }, query: { inviteToken: 'tok' } }; + + await expect(SignupService.signup(req)).rejects.toBe(orgErr); + expect(remove).toHaveBeenCalledTimes(1); + expect(release).toHaveBeenCalledTimes(1); + expect(finalize).not.toHaveBeenCalled(); + }); +}); + +describe('auth.signup.service — invited signup + mailer off does NOT auto-verify (#3995)', () => { + test('an invited account is NOT auto-verified when the mailer is unconfigured (unlike a plain signup)', async () => { + const createdUser = { + id: 'u1', email: 'invitee@test.com', firstName: 'A', lastName: 'B', provider: 'local', emailVerified: false, + }; + const update = jest.fn().mockResolvedValue({}); + const { eligibility } = mockEligibilityWithInvite(); + mockSignupServiceDeps({ + config: baseConfig({ sign: { up: false } }), + eligibility, + create: jest.fn().mockResolvedValue(createdUser), + update, + mailerConfigured: false, + }); + + const { default: SignupService } = await import('../services/auth.signup.service.js'); + const req = { body: { email: 'invitee@test.com', firstName: 'A', lastName: 'B', password: 'P@ss1234!' }, query: { inviteToken: 'tok' } }; + + const { user } = await SignupService.signup(req); + + // No emailVerified:true persist for an invited account — the token proves the + // INVITER knew the address, not that the SIGNER controls it (E6). + expect(update).not.toHaveBeenCalled(); + expect(user.emailVerified).toBe(false); + }); + + test('control: a NON-invited (plain) signup DOES auto-verify when the mailer is unconfigured', async () => { + const createdUser = { + id: 'u2', email: 'self@test.com', firstName: 'C', lastName: 'D', provider: 'local', emailVerified: false, + }; + const update = jest.fn().mockResolvedValue({}); + mockSignupServiceDeps({ + config: baseConfig({ sign: { up: true } }), + create: jest.fn().mockResolvedValue(createdUser), + update, + mailerConfigured: false, + }); + + const { default: SignupService } = await import('../services/auth.signup.service.js'); + const req = { body: { email: 'self@test.com', firstName: 'C', lastName: 'D', password: 'P@ss1234!' }, query: {} }; + + const { user } = await SignupService.signup(req); + + expect(update).toHaveBeenCalledWith(expect.objectContaining({ id: 'u1' } /* getBrut mock */), { emailVerified: true }, 'recover'); + expect(user.emailVerified).toBe(true); + }); +}); + +describe('auth.signup.service — invite finalize ordering (#3995)', () => { + test('finalize runs strictly AFTER organization provisioning and is the last pre-response step', async () => { + const order = []; + const { eligibility, finalize } = mockEligibilityWithInvite(); + finalize.mockImplementation(async () => { + order.push('finalize'); + return { id: 'inv1', status: 'accepted' }; + }); + const handleSignupOrganization = jest.fn().mockImplementation(async () => { + order.push('organization'); + return { organization: { id: 'org1' }, abilities: [], emailVerificationRequired: false }; + }); + mockSignupServiceDeps({ + config: baseConfig({ sign: { up: false } }), + eligibility, + handleSignupOrganization, + }); + + const { default: SignupService } = await import('../services/auth.signup.service.js'); + const req = { body: { email: 'invitee@test.com', firstName: 'A', lastName: 'B', password: 'P@ss1234!' }, query: { inviteToken: 'tok' } }; + + const { orgResult } = await SignupService.signup(req); + + expect(order).toEqual(['organization', 'finalize']); + // The response block reads the organization result — not just {user, invite}. + expect(orgResult.organization).toEqual({ id: 'org1' }); + }); +}); From 83dd4aa6ec2eb573c9530a8878e58f9802c96332 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Fri, 4 Sep 2026 15:14:44 +0200 Subject: [PATCH 3/4] test(auth): strengthen signup-service rollback and auto-verify assertions A reviewer ran 10 mutations against auth.signup.service.js; 6 stayed green because the tests only checked call counts or a mock's fixed return value. Strengthens the capacity-gate/org-failure release-and-rollback assertions to check the actual call target, the call order (remove before release), and that each cleanup call is genuinely awaited (a dropped await here becomes an unhandledRejection that crashes the process on Node 24). Also asserts on getBrut's call argument instead of its mocked return value, so an auto-verify lookup of the wrong user is no longer invisible. Verified each of the 6 previously-green mutations now fails the suite, and that the 4 already-red mutations still do. Claude-Session: https://claude.ai/code/session_0185ELiCjZaBJx8PH4xoSsZb --- .../tests/auth.signup.service.unit.tests.js | 129 ++++++++++++++++-- 1 file changed, 119 insertions(+), 10 deletions(-) diff --git a/modules/auth/tests/auth.signup.service.unit.tests.js b/modules/auth/tests/auth.signup.service.unit.tests.js index c622f9015..db8f378e6 100644 --- a/modules/auth/tests/auth.signup.service.unit.tests.js +++ b/modules/auth/tests/auth.signup.service.unit.tests.js @@ -30,6 +30,7 @@ import { jest, describe, test, expect } from '@jest/globals'; * @param {Object} args.config - the mocked config module default export * @param {Object} [args.eligibility] - the mocked auth.eligibility default export * @param {Function} [args.create] - UserService.create mock implementation + * @param {Function} [args.getBrut] - UserService.getBrut mock implementation * @param {Function} [args.update] - UserService.update mock implementation * @param {Function} [args.remove] - UserService.remove mock implementation * @param {Function} [args.count] - UserService.count mock implementation @@ -41,6 +42,7 @@ function mockSignupServiceDeps({ config, eligibility, create, + getBrut, update, remove, count, @@ -60,7 +62,7 @@ function mockSignupServiceDeps({ create: create || jest.fn().mockResolvedValue({ id: 'u1', email: 'x@y.com', firstName: 'A', lastName: 'B', provider: 'local', emailVerified: false, }), - getBrut: jest.fn().mockResolvedValue({ id: 'u1' }), + getBrut: getBrut || jest.fn().mockResolvedValue({ id: 'u1' }), update: update || jest.fn().mockResolvedValue({}), remove: remove || jest.fn().mockResolvedValue({}), count: count || jest.fn().mockResolvedValue(0), @@ -143,8 +145,31 @@ describe('auth.signup.service — capacity gate (#3995)', () => { expect(create).not.toHaveBeenCalled(); }); - test('a claimed invite is RELEASED when the capacity gate rejects the signup', async () => { - const { eligibility, finalize, release } = mockEligibilityWithInvite(); + test('a claimed invite release on the capacity gate is AWAITED before the rejection is returned, and a release failure is swallowed', async () => { + // A call-count assertion alone can't see a dropped `await` — the release() + // call still happens, it's just not waited on. Make the mock settle on a + // LATER tick (and reject) so a missing `await` shows up as a timing gap: + // with the await in place, `releaseSettled` must already be true by the + // time signup() rejects. Rejecting with `orgErr`'s sibling here — SIGNUP_ + // DISABLED, not the release() error — also proves the failure is swallowed + // rather than escaping (mirrors billing's own `setImmediate` tick-flush + // idiom, see modules/billing/tests/billing.init.ops-listeners.unit.tests.js). + let releaseSettled = false; + const release = jest.fn().mockImplementation(() => new Promise((_resolve, reject) => { + setImmediate(() => { + releaseSettled = true; + reject(new Error('release failed')); + }); + })); + const finalize = jest.fn(); + const eligibility = { + assertSignupEligible: jest.fn().mockResolvedValue({ + invite: { id: 'inv1', email: 'invitee@test.com', invitedBy: 'inviter1' }, + claimed: true, + finalize, + release, + }), + }; const create = jest.fn(); mockSignupServiceDeps({ config: baseConfig({ sign: { up: true, cap: 1 } }), @@ -157,6 +182,13 @@ describe('auth.signup.service — capacity gate (#3995)', () => { const req = { body: { email: 'invitee@test.com', firstName: 'A', lastName: 'B', password: 'P@ss1234!' }, query: { inviteToken: 'tok' } }; await expect(SignupService.signup(req)).rejects.toMatchObject({ code: 'SIGNUP_DISABLED' }); + // Snapshot BEFORE flushing — a mutant's dangling (unawaited) release() + // promise would otherwise settle during the flush below and paper over + // the missing await. + const releaseSettledAtThrow = releaseSettled; + await new Promise((resolve) => setImmediate(resolve)); + + expect(releaseSettledAtThrow).toBe(true); expect(release).toHaveBeenCalledTimes(1); expect(finalize).not.toHaveBeenCalled(); expect(create).not.toHaveBeenCalled(); @@ -180,13 +212,77 @@ describe('auth.signup.service — capacity gate (#3995)', () => { }); describe('auth.signup.service — organization-provisioning failure (#3995)', () => { - test('releases a claimed invite AND rolls back the created user when handleSignupOrganization throws', async () => { - const { eligibility, finalize, release } = mockEligibilityWithInvite(); - const remove = jest.fn().mockResolvedValue({}); + test('a NON-invited signup awaits UserService.remove() on rollback (no invite path to mask a missing await), and removes the exact created user', async () => { + // Isolation matters here: with no invite in play there is nothing else to + // await after remove() before the throw. In the invited scenario below, + // the SUBSEQUENT `await release()` would pump the event loop long enough + // for an earlier, unawaited remove() to also settle — masking a missing + // `await` on remove() specifically. This scenario is the only one that + // isolates it. + const createdUser = { + id: 'org-fail-user-42', email: 'self@test.com', firstName: 'A', lastName: 'B', provider: 'local', emailVerified: false, + }; + const create = jest.fn().mockResolvedValue(createdUser); const orgErr = new Error('org provisioning DB error'); + let removeSettled = false; + const remove = jest.fn().mockImplementation(() => new Promise((_resolve, reject) => { + setImmediate(() => { + removeSettled = true; + reject(new Error('remove failed')); + }); + })); + mockSignupServiceDeps({ + config: baseConfig({ sign: { up: true } }), // public signup, no invite + create, + remove, + handleSignupOrganization: jest.fn().mockRejectedValue(orgErr), + }); + + const { default: SignupService } = await import('../services/auth.signup.service.js'); + const req = { body: { email: 'self@test.com', firstName: 'A', lastName: 'B', password: 'P@ss1234!' }, query: {} }; + + // Rejecting with `orgErr` (not remove()'s own error) proves the rollback + // failure is swallowed rather than escaping. + await expect(SignupService.signup(req)).rejects.toBe(orgErr); + const removeSettledAtThrow = removeSettled; + await new Promise((resolve) => setImmediate(resolve)); + + expect(removeSettledAtThrow).toBe(true); + expect(remove).toHaveBeenCalledWith(expect.objectContaining({ id: 'org-fail-user-42' })); + expect(remove).toHaveBeenCalledTimes(1); + }); + + test('an invited signup rolls back in order — remove() strictly BEFORE release(), both awaited — and targets the created user', async () => { + const createdUser = { + id: 'org-fail-invited-user-7', email: 'invitee@test.com', firstName: 'A', lastName: 'B', provider: 'local', emailVerified: false, + }; + const create = jest.fn().mockResolvedValue(createdUser); + const orgErr = new Error('org provisioning DB error'); + const order = []; + const remove = jest.fn().mockImplementation(() => new Promise((resolve) => { + setImmediate(() => { order.push('remove'); resolve({}); }); + })); + let releaseSettled = false; + const release = jest.fn().mockImplementation(() => new Promise((_resolve, reject) => { + setImmediate(() => { + order.push('release'); + releaseSettled = true; + reject(new Error('release failed')); + }); + })); + const finalize = jest.fn(); + const eligibility = { + assertSignupEligible: jest.fn().mockResolvedValue({ + invite: { id: 'inv1', email: 'invitee@test.com', invitedBy: 'inviter1' }, + claimed: true, + finalize, + release, + }), + }; mockSignupServiceDeps({ config: baseConfig({ sign: { up: false } }), eligibility, + create, remove, handleSignupOrganization: jest.fn().mockRejectedValue(orgErr), }); @@ -195,8 +291,14 @@ describe('auth.signup.service — organization-provisioning failure (#3995)', () const req = { body: { email: 'invitee@test.com', firstName: 'A', lastName: 'B', password: 'P@ss1234!' }, query: { inviteToken: 'tok' } }; await expect(SignupService.signup(req)).rejects.toBe(orgErr); - expect(remove).toHaveBeenCalledTimes(1); - expect(release).toHaveBeenCalledTimes(1); + // Snapshot BEFORE flushing (see the capacity-gate test above for why). + const orderAtThrow = [...order]; + const releaseSettledAtThrow = releaseSettled; + await new Promise((resolve) => setImmediate(resolve)); + + expect(orderAtThrow).toEqual(['remove', 'release']); + expect(releaseSettledAtThrow).toBe(true); + expect(remove).toHaveBeenCalledWith(expect.objectContaining({ id: 'org-fail-invited-user-7' })); expect(finalize).not.toHaveBeenCalled(); }); }); @@ -227,14 +329,20 @@ describe('auth.signup.service — invited signup + mailer off does NOT auto-veri expect(user.emailVerified).toBe(false); }); - test('control: a NON-invited (plain) signup DOES auto-verify when the mailer is unconfigured', async () => { + test('control: a NON-invited (plain) signup DOES auto-verify when the mailer is unconfigured, looking up the ACTUAL created user', async () => { const createdUser = { id: 'u2', email: 'self@test.com', firstName: 'C', lastName: 'D', provider: 'local', emailVerified: false, }; + // A getBrut mock that returns a fixed value for ANY argument hides a + // lookup of the wrong user — assert on the call ARGUMENT instead, which + // is what actually identifies which account gets auto-verified. + const brutUser = { id: 'u2', _brut: true }; + const getBrut = jest.fn().mockResolvedValue(brutUser); const update = jest.fn().mockResolvedValue({}); mockSignupServiceDeps({ config: baseConfig({ sign: { up: true } }), create: jest.fn().mockResolvedValue(createdUser), + getBrut, update, mailerConfigured: false, }); @@ -244,7 +352,8 @@ describe('auth.signup.service — invited signup + mailer off does NOT auto-veri const { user } = await SignupService.signup(req); - expect(update).toHaveBeenCalledWith(expect.objectContaining({ id: 'u1' } /* getBrut mock */), { emailVerified: true }, 'recover'); + expect(getBrut).toHaveBeenCalledWith({ id: 'u2' }); + expect(update).toHaveBeenCalledWith(brutUser, { emailVerified: true }, 'recover'); expect(user.emailVerified).toBe(true); }); }); From 3156ef947932873fb2353aea829def27a1fb904c Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Fri, 4 Sep 2026 16:19:27 +0200 Subject: [PATCH 4/4] test(auth): assert finalize/handleSignupOrganization identity, add missing JSDoc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit PR #4058: the finalize-ordering test proved call order but never checked which account the honored invite gets recorded against — a wrong identifier there would link the invite to the wrong account and the suite would still pass. Asserts eligibility.finalize's argument, plus the same gap found on handleSignupOrganization's argument in the same test (order-only, no identity check). Verified both mutations (wrong finalize id, wrong handleSignupOrganization user) turn the test red, and that reverting turns it green again — alongside the six rollback/auto-verify mutations hardened in the previous commit, all still red. Also adds the @returns JSDoc this repo's guideline requires on auth.signup (documents the actual three return shapes — Express res, responses.error()'s result object, or undefined on the unguarded 422 path — rather than the flattened "Express response" wording CodeRabbit proposed) and a header on the baseConfig test helper to match its two documented siblings. Claude-Session: https://claude.ai/code/session_0185ELiCjZaBJx8PH4xoSsZb --- modules/auth/controllers/auth.controller.js | 6 +++++ .../tests/auth.signup.service.unit.tests.js | 22 ++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js index 0389e7d41..bb9263fef 100644 --- a/modules/auth/controllers/auth.controller.js +++ b/modules/auth/controllers/auth.controller.js @@ -36,6 +36,12 @@ const tokenCookieOptions = { * response (token + cookie + JSON body) or maps a thrown error to a status. * @param {Object} req - Express request object * @param {Object} res - Express response object + * @returns {Promise} on the success path, the Express + * response object (chained from res.json()); on the SIGNUP_DISABLED gate + * path, the plain result object returned by responses.error() ({type, + * message, code, status, errorCode, description} — not `res` itself); + * undefined on the generic 422 fallback, which has no explicit return + * (the error response is still sent as a side effect either way) */ const signup = async (req, res) => { try { diff --git a/modules/auth/tests/auth.signup.service.unit.tests.js b/modules/auth/tests/auth.signup.service.unit.tests.js index db8f378e6..fde4005d5 100644 --- a/modules/auth/tests/auth.signup.service.unit.tests.js +++ b/modules/auth/tests/auth.signup.service.unit.tests.js @@ -103,6 +103,17 @@ function mockSignupServiceDeps({ })); } +/** + * @desc Build the mocked config default export auth.signup.service.js reads. + * Only `sign` is overridable — `overrides.sign` is shallow-merged over the + * default `{ up: true }` so a scenario can set `up`/`cap` without restating + * both. `app` is always the fixed test default; `overrides.app` is never + * read, so passing one is silently ignored. + * @param {Object} [overrides] - partial config overrides + * @param {Object} [overrides.sign] - shallow-merged onto `{ up: true }` + * @returns {Object} config object exposing `sign` and `app`, shaped like the + * real config module's sections that this service consumes + */ const baseConfig = (overrides = {}) => ({ sign: { up: true, ...overrides.sign }, app: { title: 'Test', contact: 'test@test.com' }, @@ -379,10 +390,19 @@ describe('auth.signup.service — invite finalize ordering (#3995)', () => { const { default: SignupService } = await import('../services/auth.signup.service.js'); const req = { body: { email: 'invitee@test.com', firstName: 'A', lastName: 'B', password: 'P@ss1234!' }, query: { inviteToken: 'tok' } }; - const { orgResult } = await SignupService.signup(req); + const { user, orgResult } = await SignupService.signup(req); expect(order).toEqual(['organization', 'finalize']); + // Which account the invite is recorded against — a fixed-return finalize + // mock would otherwise hide a wrong-user link (same class of gap as the + // getBrut fix in the mailer-off/auto-verify block above). + expect(finalize).toHaveBeenCalledWith('u1'); + // Same gap, same fix: handleSignupOrganization is also only tracked by + // `order` above — a wrong/undefined user reaching org provisioning would + // still pass without this. + expect(handleSignupOrganization).toHaveBeenCalledWith(expect.objectContaining({ id: 'u1' })); // The response block reads the organization result — not just {user, invite}. expect(orgResult.organization).toEqual({ id: 'org1' }); + expect(user.id).toBe('u1'); }); });