From 1437caa025ec43f3f817b33f3fae4ad5c2f53841 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Fri, 25 Sep 2026 09:47:22 +0200 Subject: [PATCH 1/3] fix(auth): provision an organization on OAuth signup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OAuth (Google/Apple) signup resolved a user but never called AuthOrganizationService.handleSignupOrganization — only local signup and verifyEmail did, so an OAuth user landed on the org-required page with no workspace, unlike email signup. oauthCallback's passport callback is now async: after the !user guard, it calls handleSignupOrganization (best-effort, same pattern as verifyEmail) whenever the resolved user has no currentOrganization — covers new OAuth signups and any account left orphaned by this bug (self-heals, no backfill needed), and is a no-op on a normal login that already has a workspace. Wrapped the whole callback body in a try/catch: passport.authenticate() invokes this callback fire-and-forget and never awaits its returned promise, so any other throw here would otherwise become a silent unhandled rejection instead of the existing error redirect. Refs #3762, #3765, #3680 Claude-Session: https://claude.ai/code/session_01TTK9g6SFCfjfuWvB3MLFr3 --- ERRORS.md | 1 + modules/auth/controllers/auth.controller.js | 65 +++- .../tests/auth.oauth.signup-org.unit.tests.js | 300 ++++++++++++++++++ 3 files changed, 350 insertions(+), 16 deletions(-) create mode 100644 modules/auth/tests/auth.oauth.signup-org.unit.tests.js diff --git a/ERRORS.md b/ERRORS.md index 1587fa895..c969463e4 100644 --- a/ERRORS.md +++ b/ERRORS.md @@ -47,3 +47,4 @@ Use this file as a compact memory of recurring AI mistakes. - [2026-09-24] billing: `billing.meter.service.js unitsFromCosts` documented `ratios.default` as the per-key fallback (`billing.config.zod.js` JSDoc) but the code never read it, hardcoding `1` for any cost key absent from the plan's ratio map -> a plan configuring `ratios: { default: 2 }` silently billed every unlisted feature at `1`, not `2`; latent because the shipped example used `default: 1`, which happened to match the hardcoded fallback. Fix: compute the fallback once as `ratios.default` when it is a number `>= 0`, else `1`, and use it in place of the literal `1`; see pierreb-devkit/Node#4025 - [2026-09-25] billing/extras: the expiry sweep (`addExpirationEntries`) removed a pack's FULL amount even when part or all of it was already consumed or refunded -> phantom debt the next pack paid twice. Now the ledger is replayed in array order, debits (and a pack's own refunds, matched by `stripeSessionId`) are attributed to live credits earliest-expiry-first (no-expiry credits last, uncovered debt repaid by the next credit), and an expiring pack removes only its own remainder at its `expiresAt`; a fully spent pack gets a zero-amount `expiration` marker (schemas allow 0 for that kind only, hidden from `listLedgerPage`) so the `expire-` guard still holds. The write is one `findOneAndUpdate` guarded by the snapshot's ledger `$size` (append-only ⇒ unchanged length = unchanged ledger), retried on a concurrent write. Legacy full-amount entries are left as is; see pierreb-devkit/Node#4120 - [2026-09-25] billing/stripe: `billing.plans.service.js fetchPlansFromStripe` fell back to the raw Stripe product id (`product.metadata?.planId || product.id`) when a product carried no `planId` metadata -> ANY active Stripe product (a one-time pack, a recurring product sold outside the plans catalogue via a Payment Link) advertised itself as a public plan via `GET /api/billing/plans`, often with a null price id; fix = filter to `product.metadata?.planId` truthy BEFORE mapping, drop the id fallback entirely — a product now needs `metadata.planId` to be listed. Left every OTHER `metadata?.planId ||` fallback untouched (`billing.planResolver.js`, `billing.webhook.service.js`, `billing.admin.service.js`) since those resolve an EXISTING subscription's plan, not the public catalogue, and must keep working for a recurring product sold outside it; see pierreb-devkit/Node#4113 +- [2026-09-25] auth: `AuthOrganizationService.handleSignupOrganization` was wired into local signup and `verifyEmail` only, never into `oauthCallback`'s passport callback -> every OAuth (Google/Apple) signup skipped org provisioning entirely and landed on the org-required page, even though the OAuth email already counts as verified at that point (the provider vouches for it). Fix: made the passport callback `async`, and after the `!user` guard call `await AuthOrganizationService.handleSignupOrganization(user)` in its own try/catch (best-effort, same pattern as `verifyEmail`) whenever the resolved user has no `currentOrganization` — covers new signups and any account left orphaned by this bug (self-heals, no backfill migration), and is a no-op on a normal login that already has one. Trap: passport.authenticate() invokes this callback fire-and-forget and never awaits/observes its returned promise, so making it `async` without an OUTER try/catch around the whole callback body would turn any other throw (schema/JWT/etc.) into a silent unhandled rejection instead of the existing `oauthErrorRedirect` 302 — wrap the entire callback, not just the new provisioning line; see pierreb-devkit/Node#4115 diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js index 776f53433..5233b6799 100644 --- a/modules/auth/controllers/auth.controller.js +++ b/modules/auth/controllers/auth.controller.js @@ -579,26 +579,59 @@ const oauthCallback = async (req, res, next) => { // function as the 2nd/3rd arg) never calls req.logIn() itself — session // establishment is entirely the caller's responsibility below (JWT + cookie, // no express-session) — so the `session` option has nothing to act on. - return passport.authenticate(strategy, (err, user) => { - if (err) { - logger.error( - { err: { message: err?.message, code: err?.code, stack: err?.stack }, strategy }, - 'OAuth callback failed', - ); - return oauthErrorRedirect(res, err, 'oAuth error'); - } - if (!user) { + // The callback below is async (org provisioning needs to `await`), but + // passport.authenticate() invokes it fire-and-forget — it never awaits or + // otherwise observes the promise this callback returns. Without the outer + // try/catch, any throw past the org-provisioning branch (which owns its own + // best-effort catch) would become an unhandled rejection instead of the + // client-facing error redirect every other failure in this callback gets. + return passport.authenticate(strategy, async (err, user) => { + try { + if (err) { + logger.error( + { err: { message: err?.message, code: err?.code, stack: err?.stack }, strategy }, + 'OAuth callback failed', + ); + return oauthErrorRedirect(res, err, 'oAuth error'); + } + if (!user) { + logger.error( + { err: { message: err?.message, code: err?.code, stack: err?.stack }, strategy }, + 'OAuth callback failed', + ); + return oauthErrorRedirect(res, null, 'Could not define user in oAuth'); + } + // Org provisioning parity with local signup/verifyEmail (issue #4115): an + // OAuth signup never went through either path, so it never provisioned a + // workspace and the user landed on the org-required page. Only when the + // resolved user has no active org — covers new OAuth signups and any + // account left orphaned by this bug (self-heals, no backfill migration + // needed) — on every other resolution (existing/linked user already + // carrying a currentOrganization) this is a no-op, zero extra queries or + // events on a normal login. Best-effort, same pattern as `verifyEmail` + // above (#3762/#3765): a provisioning failure must never break the redirect. + if (!user.currentOrganization) { + try { + await AuthOrganizationService.handleSignupOrganization(user); + } catch (orgErr) { + logger.warn('[auth.oauthCallback] org provisioning failed (non-fatal)', { + userId: user.id, + error: orgErr?.message, + }); + } + } + const token = jwt.sign({ userId: user.id }, config.jwt.secret, { + expiresIn: config.jwt.expiresIn, + }); + res.cookie('TOKEN', token, tokenCookieOptions); + return res.redirect(302, `${getBaseUrl()}/token`); + } catch (callbackErr) { logger.error( - { err: { message: err?.message, code: err?.code, stack: err?.stack }, strategy }, + { err: { message: callbackErr?.message, code: callbackErr?.code, stack: callbackErr?.stack }, strategy }, 'OAuth callback failed', ); - return oauthErrorRedirect(res, null, 'Could not define user in oAuth'); + return oauthErrorRedirect(res, callbackErr, 'oAuth error'); } - const token = jwt.sign({ userId: user.id }, config.jwt.secret, { - expiresIn: config.jwt.expiresIn, - }); - res.cookie('TOKEN', token, tokenCookieOptions); - return res.redirect(302, `${getBaseUrl()}/token`); })(req, res, next); }; diff --git a/modules/auth/tests/auth.oauth.signup-org.unit.tests.js b/modules/auth/tests/auth.oauth.signup-org.unit.tests.js new file mode 100644 index 000000000..d0272d1c3 --- /dev/null +++ b/modules/auth/tests/auth.oauth.signup-org.unit.tests.js @@ -0,0 +1,300 @@ +/** + * Module dependencies. + */ +import { jest, describe, test, expect, beforeEach } from '@jest/globals'; + +/** + * Unit tests for auth.controller oauthCallback() — handleSignupOrganization + * wiring (issue #4115). Mirrors auth.verifyEmail.signup-org.unit.tests.js. + * + * Verifies that: + * 1. oauthCallback calls handleSignupOrganization when the resolved user has + * no currentOrganization (new OAuth signup, or an account orphaned by + * this bug before the fix). + * 2. oauthCallback does NOT call handleSignupOrganization when the resolved + * user already has a currentOrganization (normal login — no extra + * queries/events). + * 3. oauthCallback does NOT call handleSignupOrganization on the err/!user + * failure paths. + * 4. A provisioning rejection is best-effort: the TOKEN cookie is still set + * and the response still redirects to /token. + */ +describe('auth.controller oauthCallback — handleSignupOrganization wiring:', () => { + let handleSignupOrganizationMock; + let mockPassport; + + beforeEach(() => { + jest.resetModules(); + + handleSignupOrganizationMock = jest.fn().mockResolvedValue({ _id: 'org_001' }); + + mockPassport = { + authenticate: jest.fn(), + _strategy: jest.fn().mockReturnValue({ name: 'google' }), + }; + + jest.unstable_mockModule('passport', () => ({ + default: mockPassport, + })); + jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { warn: jest.fn(), error: jest.fn(), info: jest.fn() }, + })); + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { + sign: { up: true, in: true }, + jwt: { secret: 's', expiresIn: 3600 }, + cookie: { secure: true, sameSite: 'lax' }, + organizations: { enabled: true }, + app: { title: 'Test', contact: 'a@b.com' }, + }, + })); + jest.unstable_mockModule('../../../modules/users/services/users.service.js', () => ({ + default: { create: jest.fn(), getBrut: jest.fn(), update: jest.fn(), remove: jest.fn(), search: jest.fn(), count: jest.fn().mockResolvedValue(0) }, + })); + jest.unstable_mockModule('../../../modules/auth/services/auth.eligibility.js', () => ({ + default: { + registerSignupEligibility: jest.fn(), + assertSignupEligible: jest.fn().mockResolvedValue(undefined), + _reset: jest.fn(), + }, + })); + jest.unstable_mockModule('../../../modules/auth/services/auth.signupCapacity.js', () => ({ + computeSignupCapacity: jest.fn().mockResolvedValue({ cap: null, remaining: null }), + })); + jest.unstable_mockModule('../../../modules/users/repositories/users.repository.js', () => ({ + default: { update: jest.fn() }, + })); + jest.unstable_mockModule('../../../modules/organizations/services/organizations.service.js', () => ({ + default: { handleSignupOrganization: handleSignupOrganizationMock }, + })); + jest.unstable_mockModule('../../../modules/organizations/services/organizations.crud.service.js', () => ({ + default: { autoSetCurrentOrganization: jest.fn() }, + })); + jest.unstable_mockModule('../../../modules/organizations/services/organizations.membership.service.js', () => ({ + default: { findByUserAndOrganization: jest.fn(), listPendingByUser: jest.fn().mockResolvedValue([]) }, + })); + jest.unstable_mockModule('../../../modules/users/models/users.schema.js', () => ({ + default: { User: {} }, + })); + jest.unstable_mockModule('../../../lib/middlewares/model.js', () => ({ + default: { getResultFromZod: jest.fn(), checkError: jest.fn() }, + })); + jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({ + default: { defineAbilityFor: jest.fn().mockResolvedValue({}) }, + })); + jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ + default: { isConfigured: jest.fn().mockReturnValue(false), sendMail: jest.fn() }, + })); + jest.unstable_mockModule('../../../lib/helpers/responses.js', () => ({ + default: { + success: jest.fn().mockReturnValue(jest.fn()), + error: jest.fn().mockReturnValue(jest.fn()), + }, + })); + jest.unstable_mockModule('../../../lib/helpers/errors.js', () => ({ + default: { getMessage: jest.fn().mockReturnValue('error') }, + })); + jest.unstable_mockModule('../../../lib/helpers/AppError.js', () => ({ + default: class AppError extends Error { + constructor(msg, opts) { + super(msg); + this.status = opts?.status; + this.code = opts?.code; + this.details = opts?.details; + } + }, + })); + jest.unstable_mockModule('../../../lib/helpers/abilities.js', () => ({ + default: jest.fn().mockReturnValue([]), + })); + jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ + default: jest.fn().mockReturnValue('http://localhost:3000'), + })); + jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({ + default: { identify: jest.fn(), capture: jest.fn(), groupIdentify: jest.fn() }, + })); + }); + + /** + * Build a mock Express res that records cookie/redirect calls, mirroring + * the pattern used in auth.integration.tests.js's oauthCallback suite. + * @returns {{res: Object, cookies: Object, redirectCalls: Array}} + */ + const buildRes = () => { + const cookies = {}; + const redirectCalls = []; + const res = { + cookie(name, val, opts) { cookies[name] = { val, opts }; return this; }, + redirect(code, url) { redirectCalls.push({ code, url }); }, + }; + return { res, cookies, redirectCalls }; + }; + + test('calls handleSignupOrganization when the resolved user has no currentOrganization', async () => { + const user = { id: 'user_001', currentOrganization: null }; + mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, user)); + + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + const req = { params: { strategy: 'google' }, body: {} }; + const { res, cookies, redirectCalls } = buildRes(); + + await AuthController.oauthCallback(req, res, () => {}); + + expect(handleSignupOrganizationMock).toHaveBeenCalledTimes(1); + expect(handleSignupOrganizationMock).toHaveBeenCalledWith(user); + expect(cookies.TOKEN).toBeDefined(); + expect(redirectCalls[0]).toMatchObject({ code: 302 }); + expect(redirectCalls[0].url).toMatch(/\/token$/); + }); + + test('does not call handleSignupOrganization when the resolved user already has a currentOrganization', async () => { + const user = { id: 'user_002', currentOrganization: 'org_existing' }; + mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, user)); + + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + const req = { params: { strategy: 'google' }, body: {} }; + const { res, cookies, redirectCalls } = buildRes(); + + await AuthController.oauthCallback(req, res, () => {}); + + expect(handleSignupOrganizationMock).not.toHaveBeenCalled(); + expect(cookies.TOKEN).toBeDefined(); + expect(redirectCalls[0]).toMatchObject({ code: 302 }); + }); + + test('does not call handleSignupOrganization on the err path', async () => { + mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(new Error('token exchange failed'), null)); + + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + const req = { params: { strategy: 'google' }, body: {} }; + const { res, redirectCalls } = buildRes(); + + await AuthController.oauthCallback(req, res, () => {}); + + expect(handleSignupOrganizationMock).not.toHaveBeenCalled(); + expect(redirectCalls[0]).toMatchObject({ code: 302 }); + }); + + test('does not call handleSignupOrganization on the !user path', async () => { + mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, null)); + + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + const req = { params: { strategy: 'google' }, body: {} }; + const { res, redirectCalls } = buildRes(); + + await AuthController.oauthCallback(req, res, () => {}); + + expect(handleSignupOrganizationMock).not.toHaveBeenCalled(); + expect(redirectCalls[0]).toMatchObject({ code: 302 }); + }); + + test('a provisioning rejection is best-effort: cookie is still set and it still redirects to /token', async () => { + handleSignupOrganizationMock.mockRejectedValue(new Error('org boom')); + const user = { id: 'user_003', currentOrganization: null }; + mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, user)); + + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + const req = { params: { strategy: 'google' }, body: {} }; + const { res, cookies, redirectCalls } = buildRes(); + + // Must not throw / must not reject the caller's await. + await AuthController.oauthCallback(req, res, () => {}); + + expect(handleSignupOrganizationMock).toHaveBeenCalledWith(user); + expect(cookies.TOKEN).toBeDefined(); + expect(redirectCalls[0]).toMatchObject({ code: 302 }); + expect(redirectCalls[0].url).toMatch(/\/token$/); + }); + + test('a throw past the org-provisioning branch (e.g. jwt.sign failing) is caught by the outer handler and redirects with the canonical error envelope, not an unhandled rejection', async () => { + // Real `jsonwebtoken` (not mocked in this suite) throws synchronously when + // handed no secret — exercises the outer try/catch this fix adds around + // the whole passport callback, past the org-provisioning best-effort branch. + jest.resetModules(); + jest.unstable_mockModule('passport', () => ({ default: mockPassport })); + jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { warn: jest.fn(), error: jest.fn(), info: jest.fn() }, + })); + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { + sign: { up: true, in: true }, + jwt: { secret: undefined, expiresIn: 3600 }, + cookie: { secure: true, sameSite: 'lax' }, + organizations: { enabled: true }, + app: { title: 'Test', contact: 'a@b.com' }, + }, + })); + jest.unstable_mockModule('../../../modules/users/services/users.service.js', () => ({ + default: { create: jest.fn(), getBrut: jest.fn(), update: jest.fn(), remove: jest.fn(), search: jest.fn(), count: jest.fn().mockResolvedValue(0) }, + })); + jest.unstable_mockModule('../../../modules/auth/services/auth.eligibility.js', () => ({ + default: { registerSignupEligibility: jest.fn(), assertSignupEligible: jest.fn().mockResolvedValue(undefined), _reset: jest.fn() }, + })); + jest.unstable_mockModule('../../../modules/auth/services/auth.signupCapacity.js', () => ({ + computeSignupCapacity: jest.fn().mockResolvedValue({ cap: null, remaining: null }), + })); + jest.unstable_mockModule('../../../modules/users/repositories/users.repository.js', () => ({ + default: { update: jest.fn() }, + })); + jest.unstable_mockModule('../../../modules/organizations/services/organizations.service.js', () => ({ + default: { handleSignupOrganization: jest.fn().mockResolvedValue({ _id: 'org_001' }) }, + })); + jest.unstable_mockModule('../../../modules/organizations/services/organizations.crud.service.js', () => ({ + default: { autoSetCurrentOrganization: jest.fn() }, + })); + jest.unstable_mockModule('../../../modules/organizations/services/organizations.membership.service.js', () => ({ + default: { findByUserAndOrganization: jest.fn(), listPendingByUser: jest.fn().mockResolvedValue([]) }, + })); + jest.unstable_mockModule('../../../modules/users/models/users.schema.js', () => ({ + default: { User: {} }, + })); + jest.unstable_mockModule('../../../lib/middlewares/model.js', () => ({ + default: { getResultFromZod: jest.fn(), checkError: jest.fn() }, + })); + jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({ + default: { defineAbilityFor: jest.fn().mockResolvedValue({}) }, + })); + jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ + default: { isConfigured: jest.fn().mockReturnValue(false), sendMail: jest.fn() }, + })); + jest.unstable_mockModule('../../../lib/helpers/responses.js', () => ({ + default: { success: jest.fn().mockReturnValue(jest.fn()), error: jest.fn().mockReturnValue(jest.fn()) }, + })); + jest.unstable_mockModule('../../../lib/helpers/errors.js', () => ({ + default: { getMessage: jest.fn().mockReturnValue('error') }, + })); + jest.unstable_mockModule('../../../lib/helpers/AppError.js', () => ({ + default: class AppError extends Error { + constructor(msg, opts) { + super(msg); + this.status = opts?.status; + this.code = opts?.code; + this.details = opts?.details; + } + }, + })); + jest.unstable_mockModule('../../../lib/helpers/abilities.js', () => ({ + default: jest.fn().mockReturnValue([]), + })); + jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ + default: jest.fn().mockReturnValue('http://localhost:3000'), + })); + jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({ + default: { identify: jest.fn(), capture: jest.fn(), groupIdentify: jest.fn() }, + })); + + const user = { id: 'user_004', currentOrganization: 'org_existing' }; + mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, user)); + + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + const req = { params: { strategy: 'google' }, body: {} }; + const { res, cookies, redirectCalls } = buildRes(); + + // Must not throw / must not reject the caller's await — the outer catch owns it. + await AuthController.oauthCallback(req, res, () => {}); + + expect(cookies.TOKEN).toBeUndefined(); + expect(redirectCalls[0]).toMatchObject({ code: 302 }); + expect(redirectCalls[0].url).toMatch(/\/token/); + }); +}); From 07a5a71c655993cfa70a01bc45b11ea8dd6b9485 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Fri, 25 Sep 2026 09:54:13 +0200 Subject: [PATCH 2/3] refactor(simplify): dedup oauthCallback error logging and test mock setup - auth.controller.js: extract logOAuthCallbackFailure() so the three OAuth-callback failure branches (passport err, !user, outer catch-all) share one logger.error call instead of repeating it verbatim. - auth.oauth.signup-org.unit.tests.js: extract the shared jest mock registration into one registerMocks() helper reused by beforeEach and the jwt-throw test, instead of duplicating ~80 lines of module mocks. Claude-Session: https://claude.ai/code/session_01TTK9g6SFCfjfuWvB3MLFr3 --- modules/auth/controllers/auth.controller.js | 30 +++-- .../tests/auth.oauth.signup-org.unit.tests.js | 105 +++++------------- 2 files changed, 47 insertions(+), 88 deletions(-) diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js index 5233b6799..ad522b74a 100644 --- a/modules/auth/controllers/auth.controller.js +++ b/modules/auth/controllers/auth.controller.js @@ -559,6 +559,21 @@ const oauthErrorRedirect = (res, err, fallbackTitle) => { res.redirect(302, target.toString()); }; +/** + * @desc Log an OAuth callback failure with a consistent shape. Shared by every + * failure branch in `oauthCallback` (passport error, no user, and the outer + * catch-all) so the three sites can't drift on what gets logged. + * @param {string} strategy - OAuth strategy name (req.params.strategy) + * @param {Object|null} errArg - the error to log (may be null for the !user case) + * @returns {void} + */ +const logOAuthCallbackFailure = (strategy, errArg) => { + logger.error( + { err: { message: errArg?.message, code: errArg?.code, stack: errArg?.stack }, strategy }, + 'OAuth callback failed', + ); +}; + /** * @desc Endpoint for oautCallCallBack * @param {Object} req - Express request object @@ -588,17 +603,11 @@ const oauthCallback = async (req, res, next) => { return passport.authenticate(strategy, async (err, user) => { try { if (err) { - logger.error( - { err: { message: err?.message, code: err?.code, stack: err?.stack }, strategy }, - 'OAuth callback failed', - ); + logOAuthCallbackFailure(strategy, err); return oauthErrorRedirect(res, err, 'oAuth error'); } if (!user) { - logger.error( - { err: { message: err?.message, code: err?.code, stack: err?.stack }, strategy }, - 'OAuth callback failed', - ); + logOAuthCallbackFailure(strategy, null); return oauthErrorRedirect(res, null, 'Could not define user in oAuth'); } // Org provisioning parity with local signup/verifyEmail (issue #4115): an @@ -626,10 +635,7 @@ const oauthCallback = async (req, res, next) => { res.cookie('TOKEN', token, tokenCookieOptions); return res.redirect(302, `${getBaseUrl()}/token`); } catch (callbackErr) { - logger.error( - { err: { message: callbackErr?.message, code: callbackErr?.code, stack: callbackErr?.stack }, strategy }, - 'OAuth callback failed', - ); + logOAuthCallbackFailure(strategy, callbackErr); return oauthErrorRedirect(res, callbackErr, 'oAuth error'); } })(req, res, next); diff --git a/modules/auth/tests/auth.oauth.signup-org.unit.tests.js b/modules/auth/tests/auth.oauth.signup-org.unit.tests.js index d0272d1c3..187395411 100644 --- a/modules/auth/tests/auth.oauth.signup-org.unit.tests.js +++ b/modules/auth/tests/auth.oauth.signup-org.unit.tests.js @@ -18,15 +18,31 @@ import { jest, describe, test, expect, beforeEach } from '@jest/globals'; * failure paths. * 4. A provisioning rejection is best-effort: the TOKEN cookie is still set * and the response still redirects to /token. + * 5. A throw past the org-provisioning branch (anything else in the + * callback) is caught by the outer handler, not left as an unhandled + * rejection (passport.authenticate() never awaits this callback). */ describe('auth.controller oauthCallback — handleSignupOrganization wiring:', () => { - let handleSignupOrganizationMock; let mockPassport; - beforeEach(() => { + /** + * Register every mock auth.controller's dependency graph needs for this + * suite, mirroring `auth-controller.mock-setup.js`'s shared fixture (kept + * local here since it also needs the organizations.service + jwt secret + * knobs the shared fixture doesn't expose). Call from `beforeEach` (default + * jwt secret) or a single test that needs a broken secret to force + * `jwt.sign` to throw synchronously. + * @param {Object} [opts] + * @param {string|undefined} [opts.jwtSecret] - `config.jwt.secret`; omit for the default `'s'`, + * pass `undefined` explicitly (via `{ jwtSecret: undefined }`) to make `jsonwebtoken` throw — + * NOT a default-parameter value, since `{ jwtSecret = 's' }` would silently mask that case. + * @returns {{handleSignupOrganizationMock: import('@jest/globals').Mock}} + */ + const registerMocks = (opts = {}) => { + const jwtSecret = Object.hasOwn(opts, 'jwtSecret') ? opts.jwtSecret : 's'; jest.resetModules(); - handleSignupOrganizationMock = jest.fn().mockResolvedValue({ _id: 'org_001' }); + const handleSignupOrganizationMock = jest.fn().mockResolvedValue({ _id: 'org_001' }); mockPassport = { authenticate: jest.fn(), @@ -42,7 +58,7 @@ describe('auth.controller oauthCallback — handleSignupOrganization wiring:', ( jest.unstable_mockModule('../../../config/index.js', () => ({ default: { sign: { up: true, in: true }, - jwt: { secret: 's', expiresIn: 3600 }, + jwt: { secret: jwtSecret, expiresIn: 3600 }, cookie: { secure: true, sameSite: 'lax' }, organizations: { enabled: true }, app: { title: 'Test', contact: 'a@b.com' }, @@ -113,6 +129,14 @@ describe('auth.controller oauthCallback — handleSignupOrganization wiring:', ( jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({ default: { identify: jest.fn(), capture: jest.fn(), groupIdentify: jest.fn() }, })); + + return { handleSignupOrganizationMock }; + }; + + let handleSignupOrganizationMock; + + beforeEach(() => { + ({ handleSignupOrganizationMock } = registerMocks()); }); /** @@ -210,78 +234,7 @@ describe('auth.controller oauthCallback — handleSignupOrganization wiring:', ( // Real `jsonwebtoken` (not mocked in this suite) throws synchronously when // handed no secret — exercises the outer try/catch this fix adds around // the whole passport callback, past the org-provisioning best-effort branch. - jest.resetModules(); - jest.unstable_mockModule('passport', () => ({ default: mockPassport })); - jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ - default: { warn: jest.fn(), error: jest.fn(), info: jest.fn() }, - })); - jest.unstable_mockModule('../../../config/index.js', () => ({ - default: { - sign: { up: true, in: true }, - jwt: { secret: undefined, expiresIn: 3600 }, - cookie: { secure: true, sameSite: 'lax' }, - organizations: { enabled: true }, - app: { title: 'Test', contact: 'a@b.com' }, - }, - })); - jest.unstable_mockModule('../../../modules/users/services/users.service.js', () => ({ - default: { create: jest.fn(), getBrut: jest.fn(), update: jest.fn(), remove: jest.fn(), search: jest.fn(), count: jest.fn().mockResolvedValue(0) }, - })); - jest.unstable_mockModule('../../../modules/auth/services/auth.eligibility.js', () => ({ - default: { registerSignupEligibility: jest.fn(), assertSignupEligible: jest.fn().mockResolvedValue(undefined), _reset: jest.fn() }, - })); - jest.unstable_mockModule('../../../modules/auth/services/auth.signupCapacity.js', () => ({ - computeSignupCapacity: jest.fn().mockResolvedValue({ cap: null, remaining: null }), - })); - jest.unstable_mockModule('../../../modules/users/repositories/users.repository.js', () => ({ - default: { update: jest.fn() }, - })); - jest.unstable_mockModule('../../../modules/organizations/services/organizations.service.js', () => ({ - default: { handleSignupOrganization: jest.fn().mockResolvedValue({ _id: 'org_001' }) }, - })); - jest.unstable_mockModule('../../../modules/organizations/services/organizations.crud.service.js', () => ({ - default: { autoSetCurrentOrganization: jest.fn() }, - })); - jest.unstable_mockModule('../../../modules/organizations/services/organizations.membership.service.js', () => ({ - default: { findByUserAndOrganization: jest.fn(), listPendingByUser: jest.fn().mockResolvedValue([]) }, - })); - jest.unstable_mockModule('../../../modules/users/models/users.schema.js', () => ({ - default: { User: {} }, - })); - jest.unstable_mockModule('../../../lib/middlewares/model.js', () => ({ - default: { getResultFromZod: jest.fn(), checkError: jest.fn() }, - })); - jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({ - default: { defineAbilityFor: jest.fn().mockResolvedValue({}) }, - })); - jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ - default: { isConfigured: jest.fn().mockReturnValue(false), sendMail: jest.fn() }, - })); - jest.unstable_mockModule('../../../lib/helpers/responses.js', () => ({ - default: { success: jest.fn().mockReturnValue(jest.fn()), error: jest.fn().mockReturnValue(jest.fn()) }, - })); - jest.unstable_mockModule('../../../lib/helpers/errors.js', () => ({ - default: { getMessage: jest.fn().mockReturnValue('error') }, - })); - jest.unstable_mockModule('../../../lib/helpers/AppError.js', () => ({ - default: class AppError extends Error { - constructor(msg, opts) { - super(msg); - this.status = opts?.status; - this.code = opts?.code; - this.details = opts?.details; - } - }, - })); - jest.unstable_mockModule('../../../lib/helpers/abilities.js', () => ({ - default: jest.fn().mockReturnValue([]), - })); - jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ - default: jest.fn().mockReturnValue('http://localhost:3000'), - })); - jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({ - default: { identify: jest.fn(), capture: jest.fn(), groupIdentify: jest.fn() }, - })); + registerMocks({ jwtSecret: undefined }); const user = { id: 'user_004', currentOrganization: 'org_existing' }; mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, user)); From bb02343d55d6241087090690331a77ae655a13fe Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Fri, 25 Sep 2026 10:27:30 +0200 Subject: [PATCH 3/3] fix(auth): provision only OAuth-created users Gating org provisioning on `!user.currentOrganization` re-provisioned any existing org-less user (removed from org, org deleted, pending join) on every OAuth login. Gate on `info.created` instead, set by checkOAuthUserProfile's create branch and relayed through passport's verify-callback info argument to oauthCallback. Also guards the outer-catch fallback redirect with res.headersSent, so a throw after the success response starts writing can't attempt a second, conflicting redirect. Claude-Session: https://claude.ai/code/session_01TTK9g6SFCfjfuWvB3MLFr3 --- ERRORS.md | 2 +- modules/auth/controllers/auth.controller.js | 42 ++++++--- modules/auth/strategies/local/apple.js | 5 +- modules/auth/strategies/local/google.js | 5 +- .../tests/auth.oauth.signup-org.unit.tests.js | 92 ++++++++++++++++--- .../auth.oauth.signup.analytics.unit.tests.js | 8 ++ 6 files changed, 126 insertions(+), 28 deletions(-) diff --git a/ERRORS.md b/ERRORS.md index c969463e4..0bf024ae7 100644 --- a/ERRORS.md +++ b/ERRORS.md @@ -47,4 +47,4 @@ Use this file as a compact memory of recurring AI mistakes. - [2026-09-24] billing: `billing.meter.service.js unitsFromCosts` documented `ratios.default` as the per-key fallback (`billing.config.zod.js` JSDoc) but the code never read it, hardcoding `1` for any cost key absent from the plan's ratio map -> a plan configuring `ratios: { default: 2 }` silently billed every unlisted feature at `1`, not `2`; latent because the shipped example used `default: 1`, which happened to match the hardcoded fallback. Fix: compute the fallback once as `ratios.default` when it is a number `>= 0`, else `1`, and use it in place of the literal `1`; see pierreb-devkit/Node#4025 - [2026-09-25] billing/extras: the expiry sweep (`addExpirationEntries`) removed a pack's FULL amount even when part or all of it was already consumed or refunded -> phantom debt the next pack paid twice. Now the ledger is replayed in array order, debits (and a pack's own refunds, matched by `stripeSessionId`) are attributed to live credits earliest-expiry-first (no-expiry credits last, uncovered debt repaid by the next credit), and an expiring pack removes only its own remainder at its `expiresAt`; a fully spent pack gets a zero-amount `expiration` marker (schemas allow 0 for that kind only, hidden from `listLedgerPage`) so the `expire-` guard still holds. The write is one `findOneAndUpdate` guarded by the snapshot's ledger `$size` (append-only ⇒ unchanged length = unchanged ledger), retried on a concurrent write. Legacy full-amount entries are left as is; see pierreb-devkit/Node#4120 - [2026-09-25] billing/stripe: `billing.plans.service.js fetchPlansFromStripe` fell back to the raw Stripe product id (`product.metadata?.planId || product.id`) when a product carried no `planId` metadata -> ANY active Stripe product (a one-time pack, a recurring product sold outside the plans catalogue via a Payment Link) advertised itself as a public plan via `GET /api/billing/plans`, often with a null price id; fix = filter to `product.metadata?.planId` truthy BEFORE mapping, drop the id fallback entirely — a product now needs `metadata.planId` to be listed. Left every OTHER `metadata?.planId ||` fallback untouched (`billing.planResolver.js`, `billing.webhook.service.js`, `billing.admin.service.js`) since those resolve an EXISTING subscription's plan, not the public catalogue, and must keep working for a recurring product sold outside it; see pierreb-devkit/Node#4113 -- [2026-09-25] auth: `AuthOrganizationService.handleSignupOrganization` was wired into local signup and `verifyEmail` only, never into `oauthCallback`'s passport callback -> every OAuth (Google/Apple) signup skipped org provisioning entirely and landed on the org-required page, even though the OAuth email already counts as verified at that point (the provider vouches for it). Fix: made the passport callback `async`, and after the `!user` guard call `await AuthOrganizationService.handleSignupOrganization(user)` in its own try/catch (best-effort, same pattern as `verifyEmail`) whenever the resolved user has no `currentOrganization` — covers new signups and any account left orphaned by this bug (self-heals, no backfill migration), and is a no-op on a normal login that already has one. Trap: passport.authenticate() invokes this callback fire-and-forget and never awaits/observes its returned promise, so making it `async` without an OUTER try/catch around the whole callback body would turn any other throw (schema/JWT/etc.) into a silent unhandled rejection instead of the existing `oauthErrorRedirect` 302 — wrap the entire callback, not just the new provisioning line; see pierreb-devkit/Node#4115 +- [2026-09-25] auth: `oauthCallback` never provisioned an org, and review of the first fix caught that gating on `!user.currentOrganization` would also fire for an EXISTING org-less user (removed from org, pending join) on every login -> gate on `info.created` instead (set by `checkOAuthUserProfile`'s create branch, relayed via passport's verify-callback `info`), so only a genuine new signup provisions. `oauthCallback` also needed an outer try/catch (passport invokes it fire-and-forget) and a `headersSent` guard before any fallback redirect; see pierreb-devkit/Node#4115 diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js index ad522b74a..2b775ae21 100644 --- a/modules/auth/controllers/auth.controller.js +++ b/modules/auth/controllers/auth.controller.js @@ -484,6 +484,13 @@ const checkOAuthUserProfile = async (profil, key, provider) => { }); } } + // Marks THIS resolution as a brand-new account (branch 4 only — never set + // on branches 1-3, which resolve to an existing/linked user). Non-enumerable + // so it never leaks into a JSON response or a DB write; the OAuth strategy + // wrappers (google.js/apple.js) read it to decide whether to report + // `created: true` to passport's verify callback (issue #4115 follow-up — + // org provisioning must fire only for a genuine new signup, see oauthCallback). + Object.defineProperty(createdUser, '_isOAuthSignup', { value: true, enumerable: false, configurable: true }); return createdUser; } catch (err) { if (err instanceof AppError) throw err; @@ -600,7 +607,7 @@ const oauthCallback = async (req, res, next) => { // try/catch, any throw past the org-provisioning branch (which owns its own // best-effort catch) would become an unhandled rejection instead of the // client-facing error redirect every other failure in this callback gets. - return passport.authenticate(strategy, async (err, user) => { + return passport.authenticate(strategy, async (err, user, info) => { try { if (err) { logOAuthCallbackFailure(strategy, err); @@ -610,16 +617,21 @@ const oauthCallback = async (req, res, next) => { logOAuthCallbackFailure(strategy, null); return oauthErrorRedirect(res, null, 'Could not define user in oAuth'); } - // Org provisioning parity with local signup/verifyEmail (issue #4115): an - // OAuth signup never went through either path, so it never provisioned a - // workspace and the user landed on the org-required page. Only when the - // resolved user has no active org — covers new OAuth signups and any - // account left orphaned by this bug (self-heals, no backfill migration - // needed) — on every other resolution (existing/linked user already - // carrying a currentOrganization) this is a no-op, zero extra queries or - // events on a normal login. Best-effort, same pattern as `verifyEmail` - // above (#3762/#3765): a provisioning failure must never break the redirect. - if (!user.currentOrganization) { + // Org provisioning parity with local signup/verifyEmail (issue #4115): a + // brand-new OAuth signup never went through either path, so it never + // provisioned a workspace and the user landed on the org-required page. + // Gated on `info.created` (set by checkOAuthUserProfile's create branch, + // relayed through the strategy's verify callback — passport-oauth2 forwards + // this `info` object all the way to this custom-callback's 3rd argument) + // rather than "no currentOrganization": an EXISTING user who currently has + // no org (removed from their org, org deleted, a pending join request — + // local signin already treats this as valid and never provisions) must not + // be silently handed a fresh workspace on every OAuth login. Only a genuine + // new signup provisions; every other resolution (existing/linked user, with + // or without a current org) is a no-op, zero extra queries or events. Best- + // effort, same pattern as `verifyEmail` above (#3762/#3765): a provisioning + // failure must never break the redirect. + if (info?.created) { try { await AuthOrganizationService.handleSignupOrganization(user); } catch (orgErr) { @@ -636,6 +648,14 @@ const oauthCallback = async (req, res, next) => { return res.redirect(302, `${getBaseUrl()}/token`); } catch (callbackErr) { logOAuthCallbackFailure(strategy, callbackErr); + // If a throw happens after the success redirect already started writing + // (e.g. a future statement added between res.cookie and res.redirect), + // headers may already be sent — a second oauthErrorRedirect() would either + // throw again (ERR_HTTP_HEADERS_SENT, re-creating the exact unhandled- + // rejection risk the outer try/catch exists to prevent) or send garbage + // after the real response. Log only in that case; the client already got + // its redirect. + if (res.headersSent) return; return oauthErrorRedirect(res, callbackErr, 'oAuth error'); } })(req, res, next); diff --git a/modules/auth/strategies/local/apple.js b/modules/auth/strategies/local/apple.js index bff5ba68e..3ffbad291 100644 --- a/modules/auth/strategies/local/apple.js +++ b/modules/auth/strategies/local/apple.js @@ -18,7 +18,8 @@ const callbackURL = `${config.api.protocol}://${config.api.host}${config.api.por * @param {string} refreshToken - Apple refresh token * @param {Object} decodedIdToken - Decoded Apple ID token * @param {Object} profile - Apple profile (may be empty on repeat sign-ins) - * @param {Function} cb - Passport callback (err, user) + * @param {Function} cb - Passport callback (err, user, info) — `info.created` tells + * oauthCallback whether this resolution is a brand-new signup (see auth.controller.js) * @returns {Promise} */ const prepare = async (req, accessToken, refreshToken, decodedIdToken, profile, cb) => { @@ -42,7 +43,7 @@ const prepare = async (req, accessToken, refreshToken, decodedIdToken, profile, // Save the user OAuth profile try { const user = await auth.checkOAuthUserProfile(_profile, 'sub', 'apple'); - return cb(null, user); + return cb(null, user, { created: !!user._isOAuthSignup }); } catch (err) { return cb(err); } diff --git a/modules/auth/strategies/local/google.js b/modules/auth/strategies/local/google.js index 7ce3b24ca..009bb3ab0 100644 --- a/modules/auth/strategies/local/google.js +++ b/modules/auth/strategies/local/google.js @@ -16,7 +16,8 @@ const callbackURL = `${config.api.protocol}://${config.api.host}${config.api.por * @param {string} accessToken - Google access token * @param {string} refreshToken - Google refresh token * @param {Object} profile - Google profile object - * @param {Function} cb - Passport callback (err, user) + * @param {Function} cb - Passport callback (err, user, info) — `info.created` tells + * oauthCallback whether this resolution is a brand-new signup (see auth.controller.js) * @returns {Promise} */ const prepare = async (accessToken, refreshToken, profile, cb) => { @@ -37,7 +38,7 @@ const prepare = async (accessToken, refreshToken, profile, cb) => { // Save the user OAuth profile try { const user = await auth.checkOAuthUserProfile(_profile, 'sub', 'google'); - return cb(null, user); + return cb(null, user, { created: !!user._isOAuthSignup }); } catch (err) { return cb(err); } diff --git a/modules/auth/tests/auth.oauth.signup-org.unit.tests.js b/modules/auth/tests/auth.oauth.signup-org.unit.tests.js index 187395411..e0e398b3d 100644 --- a/modules/auth/tests/auth.oauth.signup-org.unit.tests.js +++ b/modules/auth/tests/auth.oauth.signup-org.unit.tests.js @@ -8,19 +8,24 @@ import { jest, describe, test, expect, beforeEach } from '@jest/globals'; * wiring (issue #4115). Mirrors auth.verifyEmail.signup-org.unit.tests.js. * * Verifies that: - * 1. oauthCallback calls handleSignupOrganization when the resolved user has - * no currentOrganization (new OAuth signup, or an account orphaned by - * this bug before the fix). - * 2. oauthCallback does NOT call handleSignupOrganization when the resolved - * user already has a currentOrganization (normal login — no extra - * queries/events). - * 3. oauthCallback does NOT call handleSignupOrganization on the err/!user + * 1. oauthCallback calls handleSignupOrganization when passport's `info` + * reports a brand-new signup (`info.created`, set by checkOAuthUserProfile's + * create branch and relayed by the strategy — see google.js/apple.js). + * 2. oauthCallback does NOT call handleSignupOrganization for an EXISTING + * user, even with no currentOrganization (e.g. removed from their org, or + * a pending join request — local signin already treats this as valid and + * never provisions; this must not silently hand them a fresh workspace). + * 3. oauthCallback does NOT call handleSignupOrganization for an existing + * user who already has a currentOrganization (normal login). + * 4. oauthCallback does NOT call handleSignupOrganization on the err/!user * failure paths. - * 4. A provisioning rejection is best-effort: the TOKEN cookie is still set + * 5. A provisioning rejection is best-effort: the TOKEN cookie is still set * and the response still redirects to /token. - * 5. A throw past the org-provisioning branch (anything else in the + * 6. A throw past the org-provisioning branch (anything else in the * callback) is caught by the outer handler, not left as an unhandled * rejection (passport.authenticate() never awaits this callback). + * 7. A throw that happens AFTER the success redirect already sent headers + * does not attempt a second redirect (headersSent guard). */ describe('auth.controller oauthCallback — handleSignupOrganization wiring:', () => { let mockPassport; @@ -154,9 +159,9 @@ describe('auth.controller oauthCallback — handleSignupOrganization wiring:', ( return { res, cookies, redirectCalls }; }; - test('calls handleSignupOrganization when the resolved user has no currentOrganization', async () => { + test('calls handleSignupOrganization when passport reports a newly created user (info.created)', async () => { const user = { id: 'user_001', currentOrganization: null }; - mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, user)); + mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, user, { created: true })); const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); const req = { params: { strategy: 'google' }, body: {} }; @@ -171,8 +176,43 @@ describe('auth.controller oauthCallback — handleSignupOrganization wiring:', ( expect(redirectCalls[0].url).toMatch(/\/token$/); }); + test('does not call handleSignupOrganization for an EXISTING user with no currentOrganization (e.g. a pending join request)', async () => { + // info.created is false/absent — checkOAuthUserProfile resolved to an + // existing/linked user (branches 1-3), not a new signup — even though + // this user happens to have no currentOrganization right now. + const user = { id: 'user_001b', currentOrganization: null }; + mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, user, { created: false })); + + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + const req = { params: { strategy: 'google' }, body: {} }; + const { res, cookies, redirectCalls } = buildRes(); + + await AuthController.oauthCallback(req, res, () => {}); + + expect(handleSignupOrganizationMock).not.toHaveBeenCalled(); + expect(cookies.TOKEN).toBeDefined(); + expect(redirectCalls[0]).toMatchObject({ code: 302 }); + }); + test('does not call handleSignupOrganization when the resolved user already has a currentOrganization', async () => { const user = { id: 'user_002', currentOrganization: 'org_existing' }; + mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, user, { created: false })); + + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + const req = { params: { strategy: 'google' }, body: {} }; + const { res, cookies, redirectCalls } = buildRes(); + + await AuthController.oauthCallback(req, res, () => {}); + + expect(handleSignupOrganizationMock).not.toHaveBeenCalled(); + expect(cookies.TOKEN).toBeDefined(); + expect(redirectCalls[0]).toMatchObject({ code: 302 }); + }); + + test('does not call handleSignupOrganization when passport supplies no info object at all', async () => { + // Defensive: a future/alternate strategy wiring that omits the 3rd `info` + // argument entirely must fail closed (no provisioning), not throw. + const user = { id: 'user_002b', currentOrganization: null }; mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, user)); const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); @@ -215,7 +255,7 @@ describe('auth.controller oauthCallback — handleSignupOrganization wiring:', ( test('a provisioning rejection is best-effort: cookie is still set and it still redirects to /token', async () => { handleSignupOrganizationMock.mockRejectedValue(new Error('org boom')); const user = { id: 'user_003', currentOrganization: null }; - mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, user)); + mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, user, { created: true })); const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); const req = { params: { strategy: 'google' }, body: {} }; @@ -250,4 +290,32 @@ describe('auth.controller oauthCallback — handleSignupOrganization wiring:', ( expect(redirectCalls[0]).toMatchObject({ code: 302 }); expect(redirectCalls[0].url).toMatch(/\/token/); }); + + test('a throw AFTER the success redirect already sent headers does not attempt a second redirect (headersSent guard)', async () => { + const user = { id: 'user_005', currentOrganization: 'org_existing' }; + mockPassport.authenticate.mockImplementation((strategy, callback) => () => callback(null, user, { created: false })); + + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + const req = { params: { strategy: 'google' }, body: {} }; + const cookies = {}; + const redirectCalls = []; + const res = { + headersSent: false, + cookie(name, val, opts) { cookies[name] = { val, opts }; return this; }, + // Simulates the success res.redirect() itself starting to write the + // response (headersSent flips true) before throwing — an edge case a + // future statement between res.cookie and res.redirect could introduce. + redirect(code, url) { + this.headersSent = true; + redirectCalls.push({ code, url }); + throw new Error('simulated failure after headers were already sent'); + }, + }; + + // Must not throw / must not attempt a second, conflicting redirect. + await AuthController.oauthCallback(req, res, () => {}); + + expect(handleSignupOrganizationMock).not.toHaveBeenCalled(); + expect(redirectCalls).toHaveLength(1); // only the original attempt — no error-envelope redirect appended + }); }); diff --git a/modules/auth/tests/auth.oauth.signup.analytics.unit.tests.js b/modules/auth/tests/auth.oauth.signup.analytics.unit.tests.js index de5b5e41b..f6cb56b25 100644 --- a/modules/auth/tests/auth.oauth.signup.analytics.unit.tests.js +++ b/modules/auth/tests/auth.oauth.signup.analytics.unit.tests.js @@ -160,6 +160,11 @@ describe('auth.controller checkOAuthUserProfile analytics (#4002/#4003):', () => expect(mockCreate).toHaveBeenCalledTimes(1); expect(result.id).toBe('u9'); + // (#4115 follow-up) branch 4 marks its result so the OAuth strategy + // wrappers can report `created: true` to oauthCallback via passport's + // `info` — non-enumerable so it never leaks into a JSON response. + expect(result._isOAuthSignup).toBe(true); + expect(Object.keys(result)).not.toContain('_isOAuthSignup'); expect(mockIdentify).toHaveBeenCalledWith('u9', expect.objectContaining({ email: 'newoauth@test.com', provider: 'google', @@ -195,6 +200,7 @@ describe('auth.controller checkOAuthUserProfile analytics (#4002/#4003):', () => expect(mockCreate).not.toHaveBeenCalled(); expect(mockIdentify).not.toHaveBeenCalled(); expect(mockCapture).not.toHaveBeenCalled(); + expect(result._isOAuthSignup).toBeUndefined(); }); test('branch 2 (linked identity match): does NOT fire analytics', async () => { @@ -213,6 +219,7 @@ describe('auth.controller checkOAuthUserProfile analytics (#4002/#4003):', () => expect(mockCreate).not.toHaveBeenCalled(); expect(mockIdentify).not.toHaveBeenCalled(); expect(mockCapture).not.toHaveBeenCalled(); + expect(result._isOAuthSignup).toBeUndefined(); }); test('branch 3 (link on verified email to an existing local account): does NOT fire analytics', async () => { @@ -237,5 +244,6 @@ describe('auth.controller checkOAuthUserProfile analytics (#4002/#4003):', () => expect(mockCreate).not.toHaveBeenCalled(); expect(mockIdentify).not.toHaveBeenCalled(); expect(mockCapture).not.toHaveBeenCalled(); + expect(result._isOAuthSignup).toBeUndefined(); }); });