chore: audit polish — migration test scoping, dead fallback, OAuth guard test, failure counters (#3960)

* chore: audit polish — migration test scoping, dead fallback, OAuth guard test, failure counters

Batches 4 small audit findings: scope the signup-grant backfill migration
integration test to its own seeded fixture ids (was crediting any leftover
grant-plan org in the shared collection), drop the unreachable
`result.plan || 'free'` fallback plus the unit test that forced it via an
impossible repository return shape, add a regression test that registers a
REAL passport strategy so a passport upgrade renaming the private `_strategy`
API fails CI instead of silently 404-ing OAuth logins, and differentiate the
migration's `failed` counter into planNotFound/guardStripped/error buckets.

Closes #3954

Claude-Session: https://claude.ai/code/session_01WfNC8bt1TgL4AsiYgCEGup

* test(billing): use a Proxy for the scoped-migration collection mock

Preserve every method/property of the real Mongoose Collection instance
instead of a duck-typed { find } object — only find() is intercepted to
intersect in the fixture id scope. Addresses CodeRabbit nit on #3960
(PR #3960 / issue #3954).

Author: PierreBrisorgueil

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();
+  });
+});

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() },

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<void>}
@@ -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})`,
+  );
 }
 
 /**

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<void>}
+   */
+  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);

modules/organizations/services/organizations.crud.service.js

@@ -128,7 +128,11 @@ const create = async (body, user) => {
   // idempotence (refId signup_grant-<orgId>) 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 });
   }

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 };