fix(auth): address CodeRabbit/Copilot review findings on invite gate

PierreBrisorgueil · PierreBrisorgueil · commit 4fd1edc5ada1 · 2026-05-28T19:33:23.000+02:00
- email template: add non-empty <title> to signup-invite.html - auth.controller: short-circuit UserService.count() when cap is null - auth.controller: coerce sign.cap to Number + guard non-finite at gate - auth.controller: only consume invite when it actually opened the gate (sign.up=true = invite not required, burning it would be wrong) - auth.invitation.controller: add full JSDoc (@param/@returns) to all controller functions per project standard - auth.invitations.yml: add InvitationListItem schema (token omitted); admin list endpoint now references it instead of Invitation - auth.invitation.model.mongoose.js: add JSDoc to addID virtual getter - auth.invitation.integration.tests.js: add @returns to test helpers; fix cap test to create admin before setting cap (was order-dependent) - auth.signout.controller.unit.tests.js: complete InvitationService mock - auth.silent.catch.unit.tests.js: complete InvitationService mock
diff --git a/config/templates/signup-invite.html b/config/templates/signup-invite.html
@@ -1,7 +1,7 @@
 <!DOCTYPE html>
 <html lang="en" xmlns="http://www.w3.org/1999/xhtml">
   <head>
-    <title></title>
+    <title>You've been invited — complete your signup</title>
   </head>
   <body>
     <p>Hello,</p>
diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js
@@ -68,8 +68,10 @@ const signup = async (req, res) => {
     // 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 (read from query: model.isValid strips unknown body keys).
-    const total = await UserService.count();
-    const capReached = config.sign.cap != null && total >= config.sign.cap;
+    // Short-circuit count() when cap is not set (null = unlimited) to avoid a
+    // collection-wide count on every signup request for uncapped deployments.
+    const cap = config.sign.cap != null ? Number(config.sign.cap) : null;
+    const capReached = cap != null && Number.isFinite(cap) && (await UserService.count()) >= cap;
     let invite = null;
     if (req.query?.inviteToken) {
       invite = await InvitationService.findValid(req.query.inviteToken, req.body.email);
@@ -138,8 +140,10 @@ const signup = async (req, res) => {
       });
     } catch (_) { /* analytics must not break auth */ }
 
-    // Single-use: consume only after the account is fully provisioned (best-effort)
-    if (invite) await InvitationService.consume(invite.id);
+    // Single-use: consume only when the invite actually opened the gate (signup
+    // was closed, so the invite was required). When signup is open, a token can
+    // be presented but is not required — consuming it would silently burn it.
+    if (invite && !config.sign.up) await InvitationService.consume(invite.id);
 
     const token = jwt.sign({ userId: user.id }, config.jwt.secret, {
       expiresIn: config.jwt.expiresIn,
@@ -425,8 +429,9 @@ const checkOAuthUserProfile = async (profil, key, provider) => {
   try {
     // Same two gates as local signup. OAuth can't carry an invite token through
     // the redirect, so the invite is matched on the provider's verified email.
-    const total = await UserService.count();
-    const capReached = config.sign.cap != null && total >= config.sign.cap;
+    // Short-circuit count() when cap is not set (null = unlimited).
+    const oauthCap = config.sign.cap != null ? Number(config.sign.cap) : null;
+    const capReached = oauthCap != null && Number.isFinite(oauthCap) && (await UserService.count()) >= oauthCap;
     const oauthInvite = config.sign.up ? null : await InvitationService.findValidByEmail(profil.email);
     if (capReached || (!config.sign.up && !oauthInvite)) {
       // Mirror the local signup endpoint's error shape so clients see the same
diff --git a/modules/auth/controllers/auth.invitation.controller.js b/modules/auth/controllers/auth.invitation.controller.js
@@ -5,7 +5,14 @@ import errors from '../../../lib/helpers/errors.js';
 import responses from '../../../lib/helpers/responses.js';
 import InvitationService from '../services/auth.invitation.service.js';
 
-/** @function create — Admin: create + email a signup invitation. */
+/**
+ * @desc Admin: create + email a signup invitation
+ * @param {Object} req - Express request object
+ * @param {string} req.body.email - Email address to invite
+ * @param {Object} req.user - Authenticated admin user
+ * @param {Object} res - Express response object
+ * @returns {Promise<void>} Sends HTTP 200 with created invitation or 422 on error
+ */
 const create = async (req, res) => {
   try {
     const invitation = await InvitationService.create(req.body.email, req.user);
@@ -15,7 +22,12 @@ const create = async (req, res) => {
   }
 };
 
-/** @function list — Admin: list all signup invitations. */
+/**
+ * @desc Admin: list all signup invitations
+ * @param {Object} req - Express request object
+ * @param {Object} res - Express response object
+ * @returns {Promise<void>} Sends HTTP 200 with invitation array or 422 on error
+ */
 const list = async (req, res) => {
   try {
     const invitations = await InvitationService.list();
@@ -25,7 +37,14 @@ const list = async (req, res) => {
   }
 };
 
-/** @function remove — Admin: revoke an invitation. */
+/**
+ * @desc Admin: revoke an invitation
+ * @param {Object} req - Express request object
+ * @param {Object} req.invitation - Loaded invitation document (set by invitationByID middleware)
+ * @param {string} req.invitation.id - Invitation id
+ * @param {Object} res - Express response object
+ * @returns {Promise<void>} Sends HTTP 200 with deleted id or 422 on error
+ */
 const remove = async (req, res) => {
   try {
     await InvitationService.revoke(req.invitation.id);
@@ -35,7 +54,13 @@ const remove = async (req, res) => {
   }
 };
 
-/** @function verify — Public: report whether a token is a valid invite (+ email). */
+/**
+ * @desc Public: report whether a token is a valid invite (+ prefill email)
+ * @param {Object} req - Express request object
+ * @param {string} req.params.token - Invitation token to verify
+ * @param {Object} res - Express response object
+ * @returns {Promise<void>} Sends HTTP 200 with { valid, email } or 422 on error
+ */
 const verify = async (req, res) => {
   try {
     const invite = await InvitationService.findValid(req.params.token);
@@ -45,7 +70,14 @@ const verify = async (req, res) => {
   }
 };
 
-/** @function invitationByID — Middleware to load an invitation into req.invitation. */
+/**
+ * @desc Middleware to load an invitation into req.invitation by id param
+ * @param {Object} req - Express request object
+ * @param {Object} res - Express response object
+ * @param {Function} next - Express next middleware function
+ * @param {string} id - Invitation id from URL param
+ * @returns {Promise<void>} Calls next() with invitation on req, or sends 404
+ */
 const invitationByID = async (req, res, next, id) => {
   try {
     const invitation = await InvitationService.get(id);
diff --git a/modules/auth/doc/auth.invitations.yml b/modules/auth/doc/auth.invitations.yml
@@ -21,7 +21,7 @@ paths:
                       data:
                         type: array
                         items:
-                          $ref: '#/components/schemas/Invitation'
+                          $ref: '#/components/schemas/InvitationListItem'
         '401':
           description: Not authenticated
         '403':
@@ -115,6 +115,28 @@ paths:
 
 components:
   schemas:
+    InvitationListItem:
+      type: object
+      description: Invitation as returned by the admin list endpoint (token omitted)
+      properties:
+        id:
+          type: string
+        email:
+          type: string
+          format: email
+        expiresAt:
+          type: string
+          format: date-time
+        usedAt:
+          type: string
+          format: date-time
+          nullable: true
+        invitedBy:
+          type: string
+          nullable: true
+        createdAt:
+          type: string
+          format: date-time
     Invitation:
       type: object
       properties:
diff --git a/modules/auth/models/auth.invitation.model.mongoose.js b/modules/auth/models/auth.invitation.model.mongoose.js
@@ -23,6 +23,11 @@ const InvitationMongoose = new Schema(
 InvitationMongoose.index({ token: 1 }, { unique: true });
 InvitationMongoose.index({ email: 1 });
 
+/**
+ * @desc Return document id as hex string
+ * @this {Object} Mongoose Invitation document
+ * @returns {String} hex string id
+ */
 function addID() {
   return this._id.toHexString();
 }
diff --git a/modules/auth/tests/auth.invitation.integration.tests.js b/modules/auth/tests/auth.invitation.integration.tests.js
@@ -41,6 +41,7 @@ describe('Signup invitations:', () => {
    *   1. signup via /api/auth/signup
    *   2. promote to admin via UserService.update (roles stripped on signup)
    *   3. signin so the agent cookie jar holds the fresh JWT reflecting the new role
+   * @returns {Promise<import('supertest').SuperAgentTest>} Supertest agent with admin JWT cookie
    */
   async function createAdminAndSignin() {
     const email = 'inv-admin@test.com';
@@ -75,6 +76,7 @@ describe('Signup invitations:', () => {
 
   /**
    * Helper: create a regular user and return a bound supertest agent with the auth cookie.
+   * @returns {Promise<import('supertest').SuperAgentTest>} Supertest agent with user JWT cookie
    */
   async function createUserAndSignin() {
     const email = 'inv-user@test.com';
@@ -190,16 +192,19 @@ describe('Signup invitations:', () => {
     });
 
     test('cap reached → 404 even with a valid invite (invites count in the cap)', async () => {
-      config.sign.up = true;
-      const total = await UserService.count();
-      config.sign.cap = total; // total >= cap → blocked
+      // Create admin and issue invite BEFORE setting cap so the helper signup
+      // is not blocked. Then snapshot the user count and set cap = total.
+      config.sign.up = true; config.sign.cap = null;
       const adminAgent = await createAdminAndSignin();
       const created = await adminAgent.post('/api/auth/invitations').send({ email: 'capped@example.com' });
       const { token } = created.body.data;
+      // Freeze the cap at the current total — next signup must be blocked
+      config.sign.cap = await UserService.count(); // total >= cap → blocked
       const res = await request(app)
         .post(`/api/auth/signup?inviteToken=${token}`)
         .send({ email: 'capped@example.com', password: 'Sup3rStr0ng!' });
       expect(res.status).toBe(404);
+      config.sign.cap = null;
     });
 
     test('under cap + signup open → normal signup works', async () => {
diff --git a/modules/auth/tests/auth.signout.controller.unit.tests.js b/modules/auth/tests/auth.signout.controller.unit.tests.js
@@ -23,7 +23,15 @@ describe('auth.controller signout:', () => {
       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.invitation.service.js', () => ({
-      default: { findValid: jest.fn().mockResolvedValue(null), consume: jest.fn().mockResolvedValue(null) },
+      default: {
+        findValid: jest.fn().mockResolvedValue(null),
+        findValidByEmail: jest.fn().mockResolvedValue(null),
+        consume: jest.fn().mockResolvedValue(null),
+        create: jest.fn(),
+        list: jest.fn(),
+        get: jest.fn(),
+        revoke: jest.fn(),
+      },
     }));
     jest.unstable_mockModule('../../../modules/users/repositories/users.repository.js', () => ({
       default: { update: jest.fn() },
diff --git a/modules/auth/tests/auth.silent.catch.unit.tests.js b/modules/auth/tests/auth.silent.catch.unit.tests.js
@@ -40,7 +40,15 @@ describe('auth.controller silent-catch error logging:', () => {
       }));
 
       jest.unstable_mockModule('../../../modules/auth/services/auth.invitation.service.js', () => ({
-        default: { findValid: jest.fn().mockResolvedValue(null), consume: jest.fn().mockResolvedValue(null) },
+        default: {
+          findValid: jest.fn().mockResolvedValue(null),
+          findValidByEmail: jest.fn().mockResolvedValue(null),
+          consume: jest.fn().mockResolvedValue(null),
+          create: jest.fn(),
+          list: jest.fn(),
+          get: jest.fn(),
+          revoke: jest.fn(),
+        },
       }));
 
       jest.unstable_mockModule('../../../modules/organizations/services/organizations.service.js', () => ({
