diff --git a/modules/auth/tests/auth.oauthCall.realPassportGuard.unit.tests.js b/modules/auth/tests/auth.oauthCall.realPassportGuard.unit.tests.js new file mode 100644 index 000000000..e6797b882 --- /dev/null +++ b/modules/auth/tests/auth.oauthCall.realPassportGuard.unit.tests.js @@ -0,0 +1,60 @@ +/** + * Module dependencies. + */ +import { jest, describe, test, expect } from '@jest/globals'; +import { setupAuthControllerMocks } from './fixtures/auth-controller.mock-setup.js'; + +/** + * Regression guard (issue #3954): every test in auth.oauthCall.controller.unit.tests.js + * replaces `passport` wholesale via `jest.unstable_mockModule` and stubs `_strategy` as + * a `jest.fn()` — so if a future passport upgrade renames or removes the private + * `_strategy` API that isEnabledOAuthProvider() relies on (see auth.controller.js), none + * of those mocks would notice; production would silently 404 every OAuth login while + * that whole suite kept passing green. + * + * This test uses the REAL `passport` package, registers a trivial strategy via the real + * `passport.use()`, and asserts the guard recognizes it through the real `_strategy` + * lookup (only `passport.authenticate` itself is stubbed, since the trivial strategy's + * `authenticate()` is never meant to run a full handshake). + * + * Deliberately kept in its OWN file rather than added to + * auth.oauthCall.controller.unit.tests.js: `jest.unstable_mockModule('passport', ...)` + * mock-factory registrations are sticky for the lifetime of a test FILE — they survive + * `jest.resetModules()` — so a real-passport test placed after any mocked-passport test + * in the same file would still resolve `import('passport')` to the stale mock. A + * dedicated file gives this test a clean module registry with no prior passport mock. + */ +describe('auth.controller oauthCall — isEnabledOAuthProvider against a REAL passport strategy:', () => { + test('a strategy registered via the real passport.use() is recognized by the guard and delegated to', async () => { + setupAuthControllerMocks({ realPassport: true }); + + const { default: passport } = await import('passport'); + + class TrivialStrategy { + constructor(name) { + this.name = name; + } + + // Never actually invoked in this test — passport.authenticate() is stubbed + // below, so the guard-focused assertions never reach a real handshake. + authenticate() { + this.fail({ message: 'trivial strategy stub — not exercised by this guard test' }); + } + } + passport.use(new TrivialStrategy('google')); + const authenticateMiddleware = jest.fn(); + jest.spyOn(passport, 'authenticate').mockReturnValue(authenticateMiddleware); + + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + + const req = { params: { strategy: 'google' } }; + const res = {}; + const next = jest.fn(); + + AuthController.oauthCall(req, res, next); + + expect(passport.authenticate).toHaveBeenCalledWith('google', { session: false }); + expect(authenticateMiddleware).toHaveBeenCalledWith(req, res, next); + expect(next).not.toHaveBeenCalled(); + }); +}); diff --git a/modules/auth/tests/fixtures/auth-controller.mock-setup.js b/modules/auth/tests/fixtures/auth-controller.mock-setup.js index 7c5938614..58e7f357b 100644 --- a/modules/auth/tests/fixtures/auth-controller.mock-setup.js +++ b/modules/auth/tests/fixtures/auth-controller.mock-setup.js @@ -13,18 +13,29 @@ import { jest } from '@jest/globals'; * dependency graph needs, mirroring what each test's dynamic import expects. * Call this from `beforeEach` (not `beforeAll`) so the mocks are registered * before that test's `await import('.../auth.controller.js')`. - * @returns {{authenticate: import('@jest/globals').Mock, _strategy: import('@jest/globals').Mock}} mockPassport - the passport mock, so callers can further customize per-test behavior (e.g. `mockPassport._strategy.mockReturnValue(...)`) + * @param {Object} [opts] + * @param {boolean} [opts.realPassport=false] - When true, skip mocking `passport` entirely + * so the caller's own `await import('passport')` resolves the REAL package (issue #3954: + * a regression test registers an actual strategy via `passport.use()` and asserts + * `isEnabledOAuthProvider` sees it through the real `_strategy` API, so a passport + * upgrade renaming that private API fails here instead of silently 404-ing OAuth + * logins in prod — every other test in this suite mocks `_strategy` away and can't + * catch that). + * @returns {{authenticate: import('@jest/globals').Mock, _strategy: import('@jest/globals').Mock}|undefined} mockPassport - the passport mock, so callers can further customize per-test behavior (e.g. `mockPassport._strategy.mockReturnValue(...)`). `undefined` when `realPassport` is true. */ -export function setupAuthControllerMocks() { +export function setupAuthControllerMocks({ realPassport = false } = {}) { jest.resetModules(); - const mockPassport = { - authenticate: jest.fn().mockReturnValue(jest.fn()), - _strategy: jest.fn().mockReturnValue(undefined), - }; - jest.unstable_mockModule('passport', () => ({ - default: mockPassport, - })); + let mockPassport; + if (!realPassport) { + mockPassport = { + authenticate: jest.fn().mockReturnValue(jest.fn()), + _strategy: jest.fn().mockReturnValue(undefined), + }; + jest.unstable_mockModule('passport', () => ({ + default: mockPassport, + })); + } jest.unstable_mockModule('../../../../lib/services/logger.js', () => ({ default: { warn: jest.fn(), error: jest.fn(), info: jest.fn() }, diff --git a/modules/billing/migrations/20260707100000-backfill-missing-signup-grant-credits.js b/modules/billing/migrations/20260707100000-backfill-missing-signup-grant-credits.js index 7c261c07f..94c16749b 100644 --- a/modules/billing/migrations/20260707100000-backfill-missing-signup-grant-credits.js +++ b/modules/billing/migrations/20260707100000-backfill-missing-signup-grant-credits.js @@ -34,6 +34,7 @@ import mongoose from 'mongoose'; import config from '../../../config/index.js'; import BillingSignupGrantService from '../services/billing.signupGrant.service.js'; +import BillingPlanService from '../services/billing.plan.service.js'; /** * @returns {Promise} @@ -57,9 +58,22 @@ export async function up() { let granted = 0; let skipped = 0; - let failed = 0; + let failedPlanNotFound = 0; + let failedGuardStripped = 0; + let failedError = 0; for await (const org of cursor) { + // grantOnSignup swallows ALL failure modes to a bare `null` by design + // (best-effort — never throws), so plan-not-found, co-presence-guard-stripped, + // and a genuine runtime error are otherwise indistinguishable from the outside. + // Pre-classify with the same read-only, config-static lookup grantOnSignup uses + // internally BEFORE calling it, purely for the completion-log breakdown below — + // this never short-circuits or skips the real call, so behavior is unchanged. + const plan = BillingPlanService.getActivePlan(org.plan); + let preclassifiedFailure = null; + if (!plan) preclassifiedFailure = 'planNotFound'; + else if (!plan.signupGrant) preclassifiedFailure = 'guardStripped'; + // grantOnSignup is idempotent (refId) and never throws — reuses the runtime // path so ObjectId casting + validation + co-presence guard all apply. const result = await BillingSignupGrantService.grantOnSignup({ @@ -68,8 +82,11 @@ export async function up() { }); if (result == null) { - // Plan has no exposed grant (co-presence guard) or a swallowed error — logged by the service. - failed += 1; + if (preclassifiedFailure === 'planNotFound') failedPlanNotFound += 1; + else if (preclassifiedFailure === 'guardStripped') failedGuardStripped += 1; + // Plan found + grant configured, yet still null: the repository call itself + // threw and grantOnSignup's catch swallowed it — a genuine runtime error. + else failedError += 1; } else if (result.applied === false) { // Idempotent no-op — org already had a signup_grant entry. skipped += 1; @@ -78,7 +95,11 @@ export async function up() { } } - console.info(`[migration] backfill-signup-grant: complete — granted=${granted} skipped=${skipped} failed=${failed}`); + const failed = failedPlanNotFound + failedGuardStripped + failedError; + console.info( + `[migration] backfill-signup-grant: complete — granted=${granted} skipped=${skipped} failed=${failed} ` + + `(planNotFound=${failedPlanNotFound} guardStripped=${failedGuardStripped} error=${failedError})`, + ); } /** diff --git a/modules/billing/tests/billing.migration.backfill-signup-grant.integration.tests.js b/modules/billing/tests/billing.migration.backfill-signup-grant.integration.tests.js index 0c35a487d..d7bb74a80 100644 --- a/modules/billing/tests/billing.migration.backfill-signup-grant.integration.tests.js +++ b/modules/billing/tests/billing.migration.backfill-signup-grant.integration.tests.js @@ -2,7 +2,7 @@ * Module dependencies. */ import mongoose from 'mongoose'; -import { describe, beforeAll, afterEach, afterAll, test, expect } from '@jest/globals'; +import { describe, beforeAll, afterEach, afterAll, test, expect, jest } from '@jest/globals'; import mongooseService from '../../../lib/services/mongoose.js'; import config from '../../../config/index.js'; @@ -15,6 +15,15 @@ import { up as backfillSignupGrants } from '../migrations/20260707100000-backfil * is Schema.ObjectId, so the migration MUST credit orgs through a path that casts to * ObjectId (it delegates to grantOnSignup → the Mongoose repository) — a raw-driver * write keyed on a string would create orphan, app-invisible ghost documents. + * + * The migration's `up()` takes no arguments — it scans the ENTIRE shared + * `organizations` collection filtered only by plan. Run unscoped against the real + * test DB, it would also silently credit any leftover grant-plan orgs left behind + * by other integration test files (crashed runs, ordering quirks), mutating state + * outside this test's ownership. `runScopedMigration` patches the `organizations` + * collection's `find()` for the duration of a single `up()` call so it only ever + * sees this test's own seeded fixture ids — the real migration logic (plan filter, + * grantOnSignup delegation, idempotency) is untouched, only its blast radius is. */ describe('backfill-missing-signup-grant migration (integration):', () => { let organizations; @@ -35,6 +44,39 @@ describe('backfill-missing-signup-grant migration (integration):', () => { return _id; }; + /** + * Run the real migration `up()` scoped to only the given org ids — intersects the + * migration's own `{ plan: { $in: grantPlanIds } }` query with `_id: { $in: orgIds }` + * for the duration of the call, so it can never reach an org outside this test's + * fixtures, then restores the unpatched collection. + * @param {import('mongoose').Types.ObjectId[]} orgIds - Fixture ids owned by this test. + * @returns {Promise} + */ + const runScopedMigration = async (orgIds) => { + const realCollection = mongoose.connection.db.collection.bind(mongoose.connection.db); + const spy = jest.spyOn(mongoose.connection.db, 'collection').mockImplementation((name) => { + const coll = realCollection(name); + if (name !== 'organizations') return coll; + // Proxy (not a duck-typed `{ find }` object) so every other method/property + // on the real Collection instance stays reachable — only `find()` is + // intercepted to intersect in the fixture id scope. + return new Proxy(coll, { + get(target, prop) { + if (prop === 'find') { + return (filter, options) => target.find({ ...filter, _id: { $in: orgIds } }, options); + } + const val = target[prop]; + return typeof val === 'function' ? val.bind(target) : val; + }, + }); + }); + try { + await backfillSignupGrants(); + } finally { + spy.mockRestore(); + } + }; + beforeAll(async () => { await mongooseService.loadModels(); await mongooseService.connect(); @@ -62,7 +104,7 @@ describe('backfill-missing-signup-grant migration (integration):', () => { test('credits the configured grant to an ObjectId-keyed org missing it, readable by ObjectId', async () => { const orgId = await seedOrg(grantPlanId); - await backfillSignupGrants(); + await runScopedMigration(createdOrgIds); // Stored + readable keyed on the ObjectId — the critical-fix assertion. const eb = await extraBalances.findOne({ organization: orgId }); @@ -79,8 +121,8 @@ describe('backfill-missing-signup-grant migration (integration):', () => { test('is idempotent — a second run does not double-credit', async () => { const orgId = await seedOrg(grantPlanId); - await backfillSignupGrants(); - await backfillSignupGrants(); + await runScopedMigration(createdOrgIds); + await runScopedMigration(createdOrgIds); const docs = await extraBalances.find({ organization: orgId }).toArray(); expect(docs).toHaveLength(1); @@ -94,7 +136,7 @@ describe('backfill-missing-signup-grant migration (integration):', () => { const repo = (await import('../repositories/billing.extraBalance.repository.js')).default; await repo.creditGrant(orgId.toString(), grantAmount, 'signup_grant'); - await backfillSignupGrants(); + await runScopedMigration(createdOrgIds); const docs = await extraBalances.find({ organization: orgId }).toArray(); expect(docs).toHaveLength(1); diff --git a/modules/organizations/services/organizations.crud.service.js b/modules/organizations/services/organizations.crud.service.js index a68143331..663fc4a92 100644 --- a/modules/organizations/services/organizations.crud.service.js +++ b/modules/organizations/services/organizations.crud.service.js @@ -128,7 +128,11 @@ const create = async (body, user) => { // idempotence (refId signup_grant-) and never throws. Fire-and-forget, synchronous // emit — the try/catch here only guards a SYNCHRONOUS listener throw (see ../lib/events.js). try { - organizationEvents.emit('organization.created', { orgId: result._id.toString(), planId: result.plan || 'free' }); + // planId is a literal 'free' (not `result.plan`), matching organizations.service.js:: + // createOrganizationForUser exactly — `plan: 'free'` is set unconditionally a few lines + // above, so `result.plan` is always 'free' here too; a `|| 'free'` fallback would be + // unreachable dead code (#3954 audit finding). + organizationEvents.emit('organization.created', { orgId: result._id.toString(), planId: 'free' }); } catch (err) { logger.warn('organizations: organization.created listener threw', { message: err?.message }); } diff --git a/modules/organizations/tests/organizations.crud.grant.unit.tests.js b/modules/organizations/tests/organizations.crud.grant.unit.tests.js index 763b45bde..fd395d04e 100644 --- a/modules/organizations/tests/organizations.crud.grant.unit.tests.js +++ b/modules/organizations/tests/organizations.crud.grant.unit.tests.js @@ -81,14 +81,6 @@ describe('organizations.crud.service.create organization.created emit:', () => { expect(mockEmit).toHaveBeenCalledWith('organization.created', { orgId: 'org1', planId: 'free' }); }); - test('defaults planId to free when the created org has no plan field', async () => { - mockOrgCreate.mockResolvedValueOnce({ _id: 'org2' }); - const user = { id: 'u1', _id: 'u1', email: 'a@b.com', emailVerified: true }; - await OrgCrudService.create({ name: 'No Plan Org' }, user); - - expect(mockEmit).toHaveBeenCalledWith('organization.created', { orgId: 'org2', planId: 'free' }); - }); - test('does not emit when membership creation fails (rollback exits before the emit)', async () => { mockMembershipCreate.mockRejectedValueOnce(new Error('membership boom')); const user = { id: 'u1', _id: 'u1', email: 'a@b.com', emailVerified: true };