refactor(modules): decoupling — constants, removeSensitive, audit req, last owner (#3403)

* chore(organizations): rename middleware/ to middlewares/ for consistency (#3402)

* chore(organizations): fix test import path after middleware/ → middlewares/ rename

* refactor(organizations): add constants.js for membership status and role magic strings (#3400)

* refactor(membership): extract validateLastOwnerProtection private helper (#3399)

* refactor(users): move removeSensitive() from AuthService to UsersService (#3401)

* fix(audit): remove req dependency from AuditService.log() (#3398)

* fix(modules): address reviewer feedback — constants, removeSensitive util, test precision, JSDoc

* fix(organizations): fail-closed RBAC guards, active-owner count filter

* test(organizations): add membership controller RBAC unit tests

* fix(organizations): neutral error message, MEMBERSHIP_ROLES constants in tests

Author: PierreBrisorgueil

modules/audit/middlewares/audit.middleware.js

@@ -3,6 +3,7 @@
  */
 import AuditService from '../services/audit.service.js';
 import logger from '../../../lib/services/logger.js';
+import config from '../../../config/index.js';
 
 /**
  * Default route prefixes to skip when auto-capturing audit events.
@@ -124,7 +125,10 @@ const createAuditMiddleware = (options = {}) => {
 
       AuditService.log({
         action,
-        req,
+        userId: req.user?._id || req.user?.id,
+        organizationId: req.organization?._id || req.organization?.id,
+        ip: config.audit?.captureIp !== false ? (req.ip || req.connection?.remoteAddress || '') : undefined,
+        userAgent: config.audit?.captureUserAgent !== false ? (req.headers?.['user-agent'] || '') : undefined,
         targetType,
         targetId,
       }).catch((err) => logger.error('audit.middleware: audit log write failed', { message: err?.message, stack: err?.stack }));

modules/audit/services/audit.service.js

@@ -10,13 +10,16 @@ import AuditRepository from '../repositories/audit.repository.js';
  * @description Record an audit log entry. No-op when audit is disabled.
  * @param {Object} params
  * @param {string} params.action - Action identifier (e.g. 'auth.login', 'billing.subscribe')
- * @param {Object} [params.req] - Express request object (extracts userId, orgId, ip, userAgent)
+ * @param {string} [params.userId] - ID of the acting user
+ * @param {string} [params.organizationId] - ID of the organization context
+ * @param {string} [params.ip] - IP address of the request
+ * @param {string} [params.userAgent] - User-agent of the request
  * @param {string} [params.targetType] - Type of the target entity
  * @param {string} [params.targetId] - ID of the target entity
  * @param {Object} [params.metadata] - Additional metadata
  * @returns {Promise<Object|null>} The created audit log entry or null if disabled
  */
-const log = async ({ action, req, targetType, targetId, metadata } = {}) => {
+const log = async ({ action, userId, organizationId, ip, userAgent, targetType, targetId, metadata } = {}) => {
   if (!config.audit?.enabled) return null;
   if (!action) return null;
 
@@ -27,15 +30,10 @@ const log = async ({ action, req, targetType, targetId, metadata } = {}) => {
     metadata: metadata || {},
   };
 
-  // Extract context from request if available (coerce ObjectIds to strings)
-  if (req) {
-    const uid = req.user?._id || req.user?.id;
-    const oid = req.organization?._id || req.organization?.id;
-    if (uid) entry.userId = String(uid);
-    if (oid) entry.orgId = String(oid);
-    entry.ip = config.audit?.captureIp !== false ? (req.ip || req.connection?.remoteAddress || '') : '';
-    entry.userAgent = config.audit?.captureUserAgent !== false ? (req.headers?.['user-agent'] || '') : '';
-  }
+  if (userId) entry.userId = String(userId);
+  if (organizationId) entry.orgId = String(organizationId);
+  if (ip !== undefined) entry.ip = ip || '';
+  if (userAgent !== undefined) entry.userAgent = userAgent || '';
 
   try {
     return await AuditRepository.create(entry);

modules/audit/tests/audit.integration.tests.js

@@ -68,13 +68,15 @@ describe('Audit integration tests:', () => {
       // Create some audit entries
       await AuditService.log({
         action: 'auth.login',
-        req: { user: { _id: adminUser.id }, headers: { 'user-agent': 'test' } },
+        userId: adminUser.id,
+        userAgent: 'test',
         targetType: 'User',
         targetId: adminUser.id,
       });
       await AuditService.log({
         action: 'auth.signup',
-        req: { user: { _id: adminUser.id }, headers: { 'user-agent': 'test' } },
+        userId: adminUser.id,
+        userAgent: 'test',
         targetType: 'User',
         targetId: adminUser.id,
       });

modules/audit/tests/audit.middleware.unit.tests.js

@@ -148,7 +148,11 @@ describe('Audit middleware unit tests:', () => {
     const call = mockLog.mock.calls[0][0];
     expect(call.action).toBe('auth.signin');
     expect(call.targetType).toBe('User');
-    expect(call.req).toBe(req);
+    expect(call.req).toBeUndefined();
+    expect(call.userId).toBe('507f1f77bcf86cd799439011');
+    expect(call.organizationId).toBe('507f1f77bcf86cd799439012');
+    expect(call.ip).toBe('127.0.0.1');
+    expect(call.userAgent).toBe('TestAgent/1.0');
   });
 
   test('should log PUT mutations', () => {

modules/audit/tests/audit.service.unit.tests.js

@@ -54,16 +54,13 @@ describe('AuditService unit tests:', () => {
   test('should log an audit entry with request context', async () => {
     const userId = '507f1f77bcf86cd799439011';
     const orgId = '507f1f77bcf86cd799439012';
-    const req = {
-      user: { _id: userId },
-      organization: { _id: orgId },
-      ip: '127.0.0.1',
-      headers: { 'user-agent': 'TestAgent/1.0' },
-    };
 
     await AuditService.log({
       action: 'auth.login',
-      req,
+      userId,
+      organizationId: orgId,
+      ip: '127.0.0.1',
+      userAgent: 'TestAgent/1.0',
       targetType: 'User',
       targetId: userId,
       metadata: { foo: 'bar' },
@@ -115,47 +112,37 @@ describe('AuditService unit tests:', () => {
     expect(mockList).toHaveBeenCalledWith({ userId }, 1, 10);
   });
 
-  // GDPR config flag tests
-  test('should capture IP when captureIp is true (default)', async () => {
-    mockConfig.audit = { enabled: true, ttlDays: 90, captureIp: true, captureUserAgent: true };
-    const req = { ip: '10.0.0.1', headers: { 'user-agent': 'Bot/1.0' } };
-    await AuditService.log({ action: 'test.ip', req });
+  // GDPR config flag tests — ip/userAgent are now extracted by the caller
+  test('should store ip when provided', async () => {
+    await AuditService.log({ action: 'test.ip', ip: '10.0.0.1', userAgent: 'Bot/1.0' });
     expect(mockCreate).toHaveBeenCalledTimes(1);
     const arg = mockCreate.mock.calls[0][0];
     expect(arg.ip).toBe('10.0.0.1');
   });
 
-  test('should set IP to empty string when captureIp is false', async () => {
-    mockConfig.audit = { enabled: true, ttlDays: 90, captureIp: false, captureUserAgent: true };
-    const req = { ip: '10.0.0.1', headers: { 'user-agent': 'Bot/1.0' } };
-    await AuditService.log({ action: 'test.ip', req });
+  test('should omit ip when undefined is passed', async () => {
+    await AuditService.log({ action: 'test.ip', ip: undefined, userAgent: 'Bot/1.0' });
     expect(mockCreate).toHaveBeenCalledTimes(1);
     const arg = mockCreate.mock.calls[0][0];
-    expect(arg.ip).toBe('');
+    expect(arg.ip).toBeUndefined();
   });
 
-  test('should capture User-Agent when captureUserAgent is true (default)', async () => {
-    mockConfig.audit = { enabled: true, ttlDays: 90, captureIp: true, captureUserAgent: true };
-    const req = { ip: '10.0.0.1', headers: { 'user-agent': 'Bot/1.0' } };
-    await AuditService.log({ action: 'test.ua', req });
+  test('should store userAgent when provided', async () => {
+    await AuditService.log({ action: 'test.ua', ip: '10.0.0.1', userAgent: 'Bot/1.0' });
     expect(mockCreate).toHaveBeenCalledTimes(1);
     const arg = mockCreate.mock.calls[0][0];
     expect(arg.userAgent).toBe('Bot/1.0');
   });
 
-  test('should set User-Agent to empty string when captureUserAgent is false', async () => {
-    mockConfig.audit = { enabled: true, ttlDays: 90, captureIp: true, captureUserAgent: false };
-    const req = { ip: '10.0.0.1', headers: { 'user-agent': 'Bot/1.0' } };
-    await AuditService.log({ action: 'test.ua', req });
+  test('should omit userAgent when undefined is passed', async () => {
+    await AuditService.log({ action: 'test.ua', ip: '10.0.0.1', userAgent: undefined });
     expect(mockCreate).toHaveBeenCalledTimes(1);
     const arg = mockCreate.mock.calls[0][0];
-    expect(arg.userAgent).toBe('');
+    expect(arg.userAgent).toBeUndefined();
   });
 
-  test('should default to capturing IP and User-Agent when config keys are undefined', async () => {
-    mockConfig.audit = { enabled: true, ttlDays: 90 };
-    const req = { ip: '192.168.1.1', headers: { 'user-agent': 'DefaultBot/2.0' } };
-    await AuditService.log({ action: 'test.defaults', req });
+  test('should store ip and userAgent when provided', async () => {
+    await AuditService.log({ action: 'test.defaults', ip: '192.168.1.1', userAgent: 'DefaultBot/2.0' });
     expect(mockCreate).toHaveBeenCalledTimes(1);
     const arg = mockCreate.mock.calls[0][0];
     expect(arg.ip).toBe('192.168.1.1');

modules/billing/routes/billing.routes.js

@@ -5,7 +5,7 @@ import passport from 'passport';
 
 import model from '../../../lib/middlewares/model.js';
 import policy from '../../../lib/middlewares/policy.js';
-import organization from '../../organizations/middleware/organizations.middleware.js';
+import organization from '../../organizations/middlewares/organizations.middleware.js';
 import billingSchema from '../models/billing.subscription.schema.js';
 import billingPlans from '../controllers/billing.plans.controller.js';
 import billing from '../controllers/billing.controller.js';

modules/home/services/home.service.js

@@ -8,10 +8,10 @@ import { Base64 } from 'js-base64';
 import { promises as fs } from 'fs';
 import mongoose from 'mongoose';
 
-import AuthService from '../../auth/services/auth.service.js';
 import config from '../../../config/index.js';
 import mailer from '../../../lib/helpers/mailer/index.js';
 import HomeRepository from '../repositories/home.repository.js';
+import { removeSensitive } from '../../users/utils/sanitizeUser.js';
 
 /**
  * @desc Check whether a config value is meaningfully set (non-empty, not a DEVKIT placeholder).
@@ -21,19 +21,20 @@ import HomeRepository from '../repositories/home.repository.js';
 const isSet = (value) => !!(value && typeof value === 'string' && value.trim() !== '' && !value.startsWith('DEVKIT_NODE_'));
 
 /**
- * @desc Function to get all admin users in db
- * @return {Promise} All users
+ * @desc Function to get page content from markdown file
+ * @param {string} name - The name of the markdown file
+ * @returns {Promise<Array>} Page content array
  */
 const page = async (name) => {
   const markdown = await fs.readFile(path.resolve(`./config/markdown/${name}.md`), 'utf8');
   const test = await fs.stat(path.resolve(`./config/markdown/${name}.md`));
-  return Promise.resolve([
+  return [
     {
       title: _.startCase(name),
       updatedAt: test.mtime,
       markdown,
     },
-  ]);
+  ];
 };
 
 /**
@@ -87,11 +88,11 @@ const changelogs = async () => {
 
 /**
  * @desc Function to get all admin users in db
- * @return {Promise} All users
+ * @returns {Promise<Array>} All users (sanitized)
  */
 const team = async () => {
   const result = await HomeRepository.team();
-  return Promise.resolve(result.map((user) => AuthService.removeSensitive(user)));
+  return result.map((user) => removeSensitive(user));
 };
 
 /**

modules/organizations/controllers/organizations.membership.controller.js

@@ -4,6 +4,7 @@
 import errors from '../../../lib/helpers/errors.js';
 import responses from '../../../lib/helpers/responses.js';
 import MembershipService from '../services/organizations.membership.service.js';
+import { MEMBERSHIP_ROLES } from '../lib/constants.js';
 
 /**
  * @function list
@@ -34,7 +35,7 @@ const list = async (req, res) => {
 const updateRole = async (req, res) => {
   try {
     // Belt-and-suspenders: only owners can change roles (CASL blocks admins via no 'update Membership')
-    if (req.membership && req.membership.role !== 'owner') {
+    if (!req.membership || req.membership.role !== MEMBERSHIP_ROLES.OWNER) {
       return responses.error(res, 403, 'Forbidden', 'Only owners can change member roles')();
     }
     const membership = await MembershipService.updateRole(req.membershipDoc, req.body.role);
@@ -53,9 +54,13 @@ const updateRole = async (req, res) => {
  */
 const remove = async (req, res) => {
   try {
-    // Admins can only remove members, not other admins or owners
-    if (req.membership && req.membership.role !== 'owner' && req.membershipDoc.role !== 'member') {
-      return responses.error(res, 403, 'Forbidden', 'Only owners can remove admins or other owners')();
+    // Only owners can remove anyone; admins can only remove members
+    const actorRole = req.membership?.role;
+    const targetRole = req.membershipDoc.role;
+    const canRemove = actorRole === MEMBERSHIP_ROLES.OWNER
+      || (actorRole === MEMBERSHIP_ROLES.ADMIN && targetRole === MEMBERSHIP_ROLES.MEMBER);
+    if (!canRemove) {
+      return responses.error(res, 403, 'Forbidden', 'Insufficient permissions to remove this member')();
     }
     const result = await MembershipService.remove(req.membershipDoc);
     responses.success(res, 'membership deleted')({ id: req.membershipDoc.id, ...result });

modules/organizations/controllers/organizations.membershipRequest.controller.js

@@ -4,6 +4,7 @@
 import errors from '../../../lib/helpers/errors.js';
 import responses from '../../../lib/helpers/responses.js';
 import MembershipService from '../services/organizations.membership.service.js';
+import { MEMBERSHIP_ROLES, MEMBERSHIP_STATUSES } from '../lib/constants.js';
 
 /**
  * @function create
@@ -33,7 +34,7 @@ const create = async (req, res) => {
  */
 const listPending = async (req, res) => {
   try {
-    if (!req.membership || req.membership.role === 'member') {
+    if (!req.membership || req.membership.role === MEMBERSHIP_ROLES.MEMBER) {
       return responses.success(res, 'membership request list')([]);
     }
     const requests = await MembershipService.listPending(req.organization._id || req.organization.id);
@@ -165,7 +166,7 @@ const requestByID = async (req, res, next, id) => {
     const membership = await MembershipService.get(id);
     const organizationId = String(req.organization._id || req.organization.id);
     const membershipOrgId = String(membership?.organizationId?._id || membership?.organizationId);
-    if (!membership || membership.status !== 'pending' || membershipOrgId !== organizationId) {
+    if (!membership || membership.status !== MEMBERSHIP_STATUSES.PENDING || membershipOrgId !== organizationId) {
       return responses.error(res, 404, 'Not Found', 'No pending request with that identifier has been found')();
     }
     req.membershipRequest = membership;

modules/organizations/lib/constants.js

@@ -0,0 +1,12 @@
+export const MEMBERSHIP_STATUSES = {
+  ACTIVE: 'active',
+  PENDING: 'pending',
+  INVITED: 'invited',
+  REJECTED: 'rejected',
+};
+
+export const MEMBERSHIP_ROLES = {
+  OWNER: 'owner',
+  ADMIN: 'admin',
+  MEMBER: 'member',
+};