fix(organizations): replace full-doc save with $set sparse patch to eliminate race clobber (#3607)

* fix(organizations): replace full-doc save with \$set sparse patch to eliminate race clobber (#3605)

OrganizationsRepository.update() used organization.save() — a full-document
overwrite. Concurrent setPlan(orgId, 'pro') from a Stripe webhook could fire
between the fetch and the save, silently reverting the plan field.

Fix:
- Rename repository method to updateById(orgId, fields) using
  findByIdAndUpdate with \$set + { new: true, runValidators: true, populate }
- Service builds a sparse patch from the Zod-validated body, stripping
  billing-owned fields (plan, stripeCustomerId, stripeSubscriptionId)
  defensively so they can never flow through the settings endpoint
- Add race-condition unit tests (6 tests): repository contract + service
  invariant that concurrent setPlan write is never overwritten

Closes #3605

* refactor(organizations): drop dead billing-strip loop + modernize findByIdAndUpdate option

- Remove BILLING_FIELDS delete loop in update service: the patch is built via an
  explicit allow-list (name, description, domain, slug), so billing fields can
  never be present and the loop is unreachable. Codacy + CodeRabbit both
  flagged it as dead code triggering object-injection lint warnings.
- Switch findByIdAndUpdate option from deprecated `new: true` to canonical
  `returnDocument: 'after'` for consistency with setPlan and Mongoose v6+ docs.
- Update race-invariant test to assert allow-list construction (no plan key
  in patch + only allowed keys present) rather than per-field stripping.

Addresses CodeRabbit nit + Codacy ACTION_REQUIRED on PR #3607.

Author: PierreBrisorgueil

modules/organizations/repositories/organizations.repository.js

@@ -44,12 +44,20 @@ const get = (id) => {
 };
 
 /**
- * @function update
- * @description Data access operation to update an existing organization in the database.
- * @param {Object} organization - The organization object containing the updated details.
- * @returns {Promise<Object>} The updated organization.
+ * @function updateById
+ * @description Atomically update an organization by ID using a sparse field patch ($set).
+ *   Replaces the old full-document `save()` path to eliminate the race window where a
+ *   concurrent `setPlan` write (Stripe webhook) could be silently overwritten.
+ * @param {String} orgId - The organization ObjectId (string).
+ * @param {Object} fields - Sparse patch object with only the fields to update.
+ * @returns {Promise<Object|null>} The updated organization with defaultPopulate, or null.
  */
-const update = (organization) => organization.save().then((doc) => doc.populate(defaultPopulate));
+const updateById = (orgId, fields) => {
+  if (!mongoose.Types.ObjectId.isValid(orgId)) return Promise.resolve(null);
+  return Organization.findByIdAndUpdate(orgId, { $set: fields }, { returnDocument: 'after', runValidators: true })
+    .populate(defaultPopulate)
+    .exec();
+};
 
 /**
  * @function remove
@@ -103,7 +111,7 @@ export default {
   list,
   create,
   get,
-  update,
+  updateById,
   remove,
   deleteMany,
   findOne,

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

@@ -135,27 +135,36 @@ const get = async (id) => {
 
 /**
  * @function update
- * @description Service to update an existing organization.
- * @param {Object} organization - The existing organization object.
- * @param {Object} body - The object containing updated organization details.
+ * @description Service to update an existing organization using a sparse $set patch.
+ *   The patch is constructed via an explicit allow-list so billing-owned fields (plan)
+ *   are never included — they are written exclusively by the Stripe webhook path
+ *   (setPlan / billing crons) and must never flow through this settings endpoint.
+ * @param {Object} organization - The existing organization Mongoose document (for slug validation).
+ * @param {Object} body - Zod-validated request body (already parsed by model.isValid middleware).
  * @returns {Promise<Object>} A promise resolving to the updated organization.
  */
 const update = async (organization, body) => {
-  if (body.name !== undefined) organization.name = body.name;
-  if (body.description !== undefined) organization.description = body.description;
+  // Build sparse patch from validated body via explicit allow-list.
+  // Billing-owned fields (plan) are intentionally absent — they are written exclusively
+  // by the Stripe webhook path (setPlan / billing crons).
+  const patch = {};
+  if (body.name !== undefined) patch.name = body.name;
+  if (body.description !== undefined) patch.description = body.description;
+  if (body.domain !== undefined) patch.domain = normalizeDomain(body.domain);
+
+  // Slug: validate uniqueness before adding to patch.
   if (body.slug !== undefined) {
     if (body.slug !== organization.slug) {
       const existing = await OrganizationsRepository.findOne({ slug: body.slug });
       if (existing) {
         throw new AppError('An organization with this slug already exists.', { code: 'CONFLICT' });
       }
     }
-    organization.slug = body.slug;
+    patch.slug = body.slug;
   }
-  if (body.domain !== undefined) organization.domain = normalizeDomain(body.domain);
-  if (body.plan !== undefined) organization.plan = body.plan;
 
-  const result = await OrganizationsRepository.update(organization);
+  const orgId = organization._id || organization.id;
+  const result = await OrganizationsRepository.updateById(String(orgId), patch);
   return result;
 };
 

modules/organizations/tests/organizations.updateById.race.unit.tests.js

@@ -0,0 +1,213 @@
+/**
+ * Module dependencies.
+ */
+import { jest, describe, test, beforeEach, afterEach, expect } from '@jest/globals';
+
+/**
+ * Unit tests for OrganizationsRepository.updateById and OrganizationsCrudService.update
+ * (#3605 — race-condition fix: replace full-doc save with $set sparse patch)
+ *
+ * Race scenario validated:
+ *   1. Fetch org doc (plan = 'free')
+ *   2. Mid-flight: Stripe webhook calls setPlan(orgId, 'pro') → raw findByIdAndUpdate
+ *   3. Service calls updateById with { name: 'New name' } patch
+ *   4. Asserts: doc has BOTH plan='pro' AND name='New name' — no clobber
+ */
+
+// ─── Repository unit tests ───────────────────────────────────────────────────
+
+describe('OrganizationsRepository.updateById (#3605):', () => {
+  let OrganizationsRepository;
+  let mockModel;
+
+  const orgId = '507f1f77bcf86cd799439011';
+
+  beforeEach(async () => {
+    jest.resetModules();
+
+    const mockPopulate = jest.fn().mockReturnThis();
+    const mockExec = jest.fn();
+
+    mockModel = {
+      findByIdAndUpdate: jest.fn().mockReturnValue({
+        populate: mockPopulate,
+        exec: mockExec,
+      }),
+    };
+    // Store refs so individual tests can override exec
+    mockModel._mockPopulate = mockPopulate;
+    mockModel._mockExec = mockExec;
+
+    jest.unstable_mockModule('mongoose', () => ({
+      default: {
+        Types: { ObjectId: { isValid: (id) => /^[a-f\d]{24}$/i.test(id) } },
+        model: jest.fn(() => mockModel),
+      },
+    }));
+
+    const mod = await import('../repositories/organizations.repository.js');
+    OrganizationsRepository = mod.default;
+  });
+
+  afterEach(() => {
+    jest.restoreAllMocks();
+  });
+
+  test('calls findByIdAndUpdate with $set and returns populated doc', async () => {
+    const updatedOrg = { _id: orgId, name: 'New name', plan: 'pro' };
+    mockModel._mockExec.mockResolvedValue(updatedOrg);
+
+    const result = await OrganizationsRepository.updateById(orgId, { name: 'New name' });
+
+    expect(mockModel.findByIdAndUpdate).toHaveBeenCalledWith(
+      orgId,
+      { $set: { name: 'New name' } },
+      expect.objectContaining({ returnDocument: 'after', runValidators: true }),
+    );
+    expect(result).toEqual(updatedOrg);
+  });
+
+  test('returns null for invalid orgId without querying DB', async () => {
+    const result = await OrganizationsRepository.updateById('not-valid', { name: 'X' });
+
+    expect(result).toBeNull();
+    expect(mockModel.findByIdAndUpdate).not.toHaveBeenCalled();
+  });
+
+  test('returns null for empty orgId without querying DB', async () => {
+    const result = await OrganizationsRepository.updateById('', { name: 'X' });
+
+    expect(result).toBeNull();
+    expect(mockModel.findByIdAndUpdate).not.toHaveBeenCalled();
+  });
+
+  test('propagates DB errors to the caller', async () => {
+    const dbErr = new Error('timeout');
+    mockModel._mockExec.mockRejectedValue(dbErr);
+
+    await expect(OrganizationsRepository.updateById(orgId, { name: 'X' })).rejects.toThrow('timeout');
+  });
+});
+
+// ─── Service race-condition test ──────────────────────────────────────────────
+
+describe('OrganizationsCrudService.update — race-condition invariant (#3605):', () => {
+  const orgId = '507f1f77bcf86cd799439011';
+
+  // Simulated in-memory "DB state" tracking concurrent writes
+  let dbState;
+
+  // Mock updateById: applies patch to dbState and returns merged doc
+  const mockUpdateById = jest.fn().mockImplementation(async (_id, patch) => {
+    // Simulate atomic $set: only patch fields are written, others survive
+    dbState = { ...dbState, ...patch };
+    return { ...dbState };
+  });
+
+  // Mock setPlan (Stripe webhook path): raw findByIdAndUpdate on plan only
+  const mockFindByIdAndUpdate = jest.fn().mockImplementation(async (_id, planPatch) => {
+    dbState = { ...dbState, ...planPatch };
+    return { ...dbState };
+  });
+
+  beforeEach(async () => {
+    jest.resetModules();
+
+    dbState = { _id: orgId, name: 'Old name', plan: 'free', slug: 'old-org', domain: '', description: '' };
+
+    jest.unstable_mockModule('../repositories/organizations.repository.js', () => ({
+      default: {
+        updateById: mockUpdateById,
+        findOne: jest.fn().mockResolvedValue(null),
+      },
+    }));
+
+    jest.unstable_mockModule('../../users/services/users.service.js', () => ({
+      default: { updateById: jest.fn() },
+    }));
+
+    jest.unstable_mockModule('../repositories/organizations.membership.repository.js', () => ({
+      default: {
+        list: jest.fn().mockResolvedValue([]),
+        create: jest.fn().mockResolvedValue({ _id: 'm1' }),
+        deleteMany: jest.fn(),
+        findOne: jest.fn().mockResolvedValue(null),
+      },
+    }));
+
+    jest.unstable_mockModule('../../../lib/helpers/AppError.js', () => ({
+      default: class AppError extends Error {
+        constructor(msg, opts) { super(msg); this.code = opts?.code; }
+      },
+    }));
+
+    jest.unstable_mockModule('../../../lib/helpers/emailVerification.js', () => ({
+      assertEmailVerified: jest.fn(),
+    }));
+
+    jest.unstable_mockModule('../helpers/organizations.slug.js', () => ({
+      slugify: jest.fn().mockReturnValue('old-org'),
+    }));
+
+    jest.unstable_mockModule('../../../lib/services/logger.js', () => ({
+      default: { error: jest.fn(), warn: jest.fn(), info: jest.fn() },
+    }));
+
+    jest.unstable_mockModule('../../../config/index.js', () => ({
+      default: { organizations: { publicDomains: [] } },
+    }));
+  });
+
+  afterEach(() => {
+    jest.restoreAllMocks();
+    mockUpdateById.mockClear();
+    mockFindByIdAndUpdate.mockClear();
+  });
+
+  test(
+    'concurrent setPlan write is preserved — $set patch never overwrites plan (#3605 race fix)',
+    async () => {
+      const { default: OrgCrudService } = await import('../services/organizations.crud.service.js');
+
+      // Org doc as fetched before the webhook fires (plan = 'free')
+      const organization = { _id: orgId, id: orgId, name: 'Old name', plan: 'free', slug: 'old-org', domain: '' };
+
+      // Mid-flight: Stripe webhook sets plan = 'pro' directly in the DB
+      await mockFindByIdAndUpdate(orgId, { plan: 'pro' });
+      // DB now: { name: 'Old name', plan: 'pro' }
+
+      // Service update: only name patch, plan field absent from body (billing-owned)
+      const result = await OrgCrudService.update(organization, { name: 'New name' });
+
+      // updateById must have been called with a patch that does NOT contain 'plan'
+      const [, patch] = mockUpdateById.mock.calls[0];
+      expect(patch).not.toHaveProperty('plan');
+
+      // The returned doc must have BOTH the webhook's plan AND the service's name change
+      expect(result.name).toBe('New name');
+      expect(result.plan).toBe('pro'); // webhook write survives — no clobber
+    },
+  );
+
+  test(
+    'plan is excluded from patch by allow-list construction — billing fields never flow through update (#3605)',
+    async () => {
+      const { default: OrgCrudService } = await import('../services/organizations.crud.service.js');
+
+      const organization = { _id: orgId, id: orgId, name: 'Old name', plan: 'free', slug: 'old-org', domain: '' };
+
+      // Even when body contains plan, the allow-list in update() only copies known
+      // settable fields (name, description, domain, slug) — plan is structurally absent.
+      await OrgCrudService.update(organization, { name: 'New name', plan: 'enterprise' });
+
+      const [, patch] = mockUpdateById.mock.calls[0];
+      // plan must not appear in the patch — it is excluded by allow-list construction
+      expect(patch).not.toHaveProperty('plan');
+      // Only allow-listed fields may be present
+      const allowedKeys = new Set(['name', 'description', 'domain', 'slug']);
+      for (const key of Object.keys(patch)) {
+        expect(allowedKeys.has(key)).toBe(true);
+      }
+    },
+  );
+});