diff --git a/ERRORS.md b/ERRORS.md index 664f91777..fc334866d 100644 --- a/ERRORS.md +++ b/ERRORS.md @@ -32,3 +32,4 @@ Use this file as a compact memory of recurring AI mistakes. - [2026-06-15] deps/audit: leaving `npm audit` advisories unaddressed on the assumption they need a major bump -> run `npm audit fix` (never `--force`) first; the runtime-tree DoS/ReDoS items (`qs`, `path-to-regexp`, `brace-expansion`) all fixed via in-range bumps, no residual. These are DoS-class but NOT attacker-reachable in this stack: Express route patterns are static (no user-controlled `path-to-regexp` input) and `qs`/`brace-expansion` only parse server-side query strings under fixed code paths — still bump them to keep the tree clean and avoid scanner noise. - [2026-07-16] security: `users.repository.js findByIdAndUpdatePopulated()` does `.populate()` with no `.select()`, so `organizations.controller.js switchOrganization` serialized the raw doc (password hash + OAuth tokens + reset/verification tokens) straight to the client; `users.account.controller.js me()` separately forwarded `providerData` (OAuth tokens) verbatim -> any endpoint returning a Mongoose user doc must go through `UserService.removeSensitive()` (whitelist, `modules/users/utils/sanitizeUser.js`) at the response boundary, never serialize `req.user`/a populated doc directly; a local-signup fixture's `providerData` defaults to `{}` so a naive falsy check won't catch this — seed a fake OAuth token in tests to prove the leak is actually closed; see pierreb-devkit/Node#3963 - [2026-07-16] billing/stripe: the #3742 priceId-map fix (`resolvePlan`/`buildPriceIdToPlanMap`) was applied only to the webhook handler; `billing.admin.service.js` `resolveStripePlan` (admin force-sync, a DB WRITE path) and `billing.reconcile.service.js` `resolveStripePlan` (LOG-ONLY divergence check) kept their own copies reading only `metadata.planId` -> a paid org's Stripe subscription (which never carries `metadata.planId`) resolved to `'free'` on admin sync (silently downgrading a paying org) and produced a false `planMismatch` alert on every reconcile run; fix = one shared resolver (`modules/billing/lib/billing.planResolver.js`) used by all three call sites, and a null-unresolved sentinel (never guess `'free'`) so the admin write path ABORTS instead of downgrading and the reconcile log path skips the comparison instead of alerting; see pierreb-devkit/Node#3964 +- [2026-07-16] architecture: `users.service.js remove()`'s sole-owner cascade called `OrganizationsRepository.remove()` directly instead of `organizations.crud.service.js#remove()`, silently skipping `runOrganizationRemovedHandlers` (the `onOrganizationRemoved` seam `modules/tasks/tasks.init.js` registers for org-scoped task cleanup) -> deleting an org must always route through the owning module's SERVICE method, never straight to its Repository, even for a "bare" cascade delete from another module — a direct-repository shortcut silently bypasses any registered removal hook; a stale cross-test fixture in `tasks.integration.tests.js` (an orphaned-but-still-existing task reused across unrelated test cases) was inadvertently relying on this bug and needed a properly isolated fixture once the cascade actually cleaned up; see pierreb-devkit/Node#3965 diff --git a/modules/organizations/lib/orgRemoval.registry.js b/modules/organizations/lib/orgRemoval.registry.js index afb265fd5..365d8aaf6 100644 --- a/modules/organizations/lib/orgRemoval.registry.js +++ b/modules/organizations/lib/orgRemoval.registry.js @@ -2,9 +2,17 @@ * @module organizations/lib/orgRemoval.registry * @description Subscriber registry for organization-removal cleanup. * Optional modules register a handler at load time via `onOrganizationRemoved`; - * the organization crud service runs them sequentially on org delete and - * propagates their errors (no silent swallow). Config-free, import-safe leaf — - * it must not import organization/tasks services (avoids an import cycle). + * `runOrganizationRemovedHandlers` runs every handler sequentially, ISOLATING each — + * one handler throwing never prevents the remaining handlers from running (so a bug + * in one module's cleanup can't orphan another module's org-scoped rows). Failures are + * collected and, after the full run, re-raised together as a single AggregateError to + * ITS caller (no silent swallow inside this module). The current sole caller, + * organizations.crud.service.js#remove, calls this AFTER the org doc and its + * memberships are already gone, and deliberately catches + logs each aggregated error + * there (best-effort) so a handler bug can never leave a zombie org or abort an + * unrelated caller's own work (#3965) — see that function's docblock. Config-free, + * import-safe leaf — it must not import organization/tasks services (avoids an + * import cycle). */ const handlers = new Set(); @@ -24,13 +32,24 @@ export const onOrganizationRemoved = (fn) => { /** * @function runOrganizationRemovedHandlers - * @description Run every registered handler sequentially. Errors propagate (not swallowed). + * @description Run every registered handler sequentially, isolating each: a handler + * throwing is caught so every later handler still runs, then all failures are re-raised + * together as a single AggregateError once the full run completes (not swallowed). * @param {Object} payload - { organizationId, organization }. * @returns {Promise} + * @throws {AggregateError} If one or more handlers threw — carries every handler error. */ export const runOrganizationRemovedHandlers = async (payload) => { + const errors = []; for (const fn of handlers) { - await fn(payload); // sequential: each handler must resolve before the next runs + try { + await fn(payload); // sequential: each handler must resolve before the next runs + } catch (err) { + errors.push(err); // isolate: a failing handler must not block the remaining ones + } + } + if (errors.length > 0) { + throw new AggregateError(errors, `${errors.length} organization-removed handler(s) failed`); } }; diff --git a/modules/organizations/services/organizations.crud.service.js b/modules/organizations/services/organizations.crud.service.js index 663fc4a92..3fb2a6974 100644 --- a/modules/organizations/services/organizations.crud.service.js +++ b/modules/organizations/services/organizations.crud.service.js @@ -188,11 +188,22 @@ const update = async (organization, body) => { /** * @function remove - * @description Service to delete an organization, all its memberships, and any - * data owned by optional modules that registered an org-removal handler via - * `onOrganizationRemoved`. Handlers run sequentially after membership cleanup; - * any handler error propagates and aborts the delete before the repository - * removal (no silent swallow). + * @description Service to delete an organization and all its memberships. Removal is + * atomic-by-ordering: membership cleanup, affected-user reassignment, and the org + * repository delete itself all happen BEFORE any onOrganizationRemoved handler runs — + * once removal starts, the org always ends fully removed (no zombie org doc, no + * memberships left pointing at it), regardless of what a handler does (#3965). + * Handlers (data owned by optional modules, e.g. tasks) then run sequentially and + * ISOLATED — one throwing never skips the rest — BEST-EFFORT: every handler error is + * logged here for manual reconciliation of that module's leftover org-scoped rows, but + * never re-thrown. This is deliberate — a + * handler failure must not resurrect/block a removal that has already committed, and + * must not propagate into an unrelated caller (e.g. users.service.js#remove's + * sole-owner cascade, which deletes the user right after this call and must not have + * that deletion aborted by a downstream task-cleanup bug). + * A STRUCTURAL failure — the membership wipe, the reassignment loop, or the org + * repository delete itself throwing — is NOT caught here and propagates to the + * caller, so a genuinely broken teardown is never reported as success. * @param {Object} organization - The organization to delete. * @returns {Promise} A promise resolving to a confirmation of the deletion. */ @@ -205,20 +216,41 @@ const remove = async (organization) => { // Remove all memberships for this organization await MembershipRepository.deleteMany({ organizationId: orgId }); - // For each affected user, switch to their next available org or set null + // For each affected user, switch to their next available org or set null. + // Guard against a dangling membership whose populated organizationId is null (ref + // to another, already-deleted org) — otherwise `.organizationId._id` throws (#3709, + // centralized here from users.service.js's own cascade by #3965). await Promise.all(affectedUsers.map(async (u) => { const remaining = await MembershipRepository.list({ userId: u._id, status: MEMBERSHIP_STATUSES.ACTIVE }); - const nextOrg = remaining.length > 0 - ? (remaining[0].organizationId._id || remaining[0].organizationId) + const liveMemberships = remaining.filter((m) => m.organizationId != null); + const nextOrg = liveMemberships.length > 0 + ? (liveMemberships[0].organizationId._id || liveMemberships[0].organizationId) : null; await UserService.updateById(u._id, { currentOrganization: nextOrg }); })); - // Run org-removal cleanup handlers registered by optional modules (e.g. tasks). - // Errors propagate and abort the delete before the repository removal. - await runOrganizationRemovedHandlers({ organizationId: orgId, organization }); - + // Structural teardown is complete — memberships gone, affected users reassigned. + // Remove the org doc itself BEFORE running any optional cleanup handler, so the org + // can never be left half-removed by a handler throwing (#3965). const result = await OrganizationsRepository.remove(organization); + + // Run org-removal cleanup handlers registered by optional modules (e.g. tasks) AFTER + // the org is structurally gone. Best-effort: log and continue on failure (see docblock). + try { + await runOrganizationRemovedHandlers({ organizationId: orgId, organization }); + } catch (err) { + // The registry isolates each handler and re-raises every failure as one AggregateError, + // so later handlers already ran. Log each failure individually for reconciliation. + const failures = err instanceof AggregateError ? err.errors : [err]; + failures.forEach((failure) => { + logger.error('organizations.crud.remove: org-removal cleanup handler failed after the org was removed (needs reconciliation)', { + organizationId: String(orgId), + message: failure?.message, + stack: failure?.stack, + }); + }); + } + return result; }; diff --git a/modules/organizations/tests/organizations.crud.orgRemoval.unit.tests.js b/modules/organizations/tests/organizations.crud.orgRemoval.unit.tests.js index 054699794..36f1f1760 100644 --- a/modules/organizations/tests/organizations.crud.orgRemoval.unit.tests.js +++ b/modules/organizations/tests/organizations.crud.orgRemoval.unit.tests.js @@ -1,5 +1,7 @@ /** - * Unit tests — OrgCrudService.remove() fires the org-removal registry and propagates handler errors. + * Unit tests — OrgCrudService.remove() fires the org-removal registry and is atomic: + * the org doc + memberships are always gone together before any handler runs, and a + * handler error is caught + logged (best-effort), never re-thrown (#3965). */ import { jest, describe, test, expect, beforeEach } from '@jest/globals'; @@ -8,9 +10,10 @@ const mockMembershipDeleteMany = jest.fn().mockResolvedValue({ deletedCount: 0 } const mockMembershipList = jest.fn().mockResolvedValue([]); const mockUpdateById = jest.fn().mockResolvedValue({}); const mockFindWithFilter = jest.fn().mockResolvedValue([]); +const mockLoggerError = jest.fn(); jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ - default: { error: jest.fn(), warn: jest.fn(), info: jest.fn() }, + default: { error: mockLoggerError, warn: jest.fn(), info: jest.fn() }, })); jest.unstable_mockModule('../../../config/index.js', () => ({ @@ -61,17 +64,103 @@ describe('OrgCrudService.remove() — org-removal registry', () => { expect(mockOrgRemove).toHaveBeenCalledWith(organization); }); - test('propagates a handler error and aborts before the repository remove', async () => { + test('a handler error is best-effort — logged, never re-thrown, and the org is still fully removed (#3965)', async () => { onOrganizationRemoved(async () => { throw new Error('tasks cleanup failed'); }); - await expect(OrgCrudService.remove(organization)).rejects.toThrow('tasks cleanup failed'); - expect(mockOrgRemove).not.toHaveBeenCalled(); + await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true }); + // The org repository delete ran BEFORE the handler — removal is atomic-by-ordering. + expect(mockOrgRemove).toHaveBeenCalledWith(organization); + expect(mockLoggerError).toHaveBeenCalledWith( + 'organizations.crud.remove: org-removal cleanup handler failed after the org was removed (needs reconciliation)', + expect.objectContaining({ organizationId: 'org-1', message: 'tasks cleanup failed' }), + ); + }); + + test('isolates handler failures — a failing handler does not skip a later one, and every failure is logged (#3965)', async () => { + const failingFirst = jest.fn().mockRejectedValue(new Error('tasks cleanup failed')); + const succeedingSecond = jest.fn().mockResolvedValue(undefined); + onOrganizationRemoved(failingFirst); + onOrganizationRemoved(succeedingSecond); + + await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true }); + + // The second handler still ran despite the first throwing (per-handler isolation). + expect(failingFirst).toHaveBeenCalledTimes(1); + expect(succeedingSecond).toHaveBeenCalledTimes(1); + expect(succeedingSecond).toHaveBeenCalledWith({ organizationId: 'org-1', organization }); + // The single failure is logged for reconciliation; the org is still fully removed. + expect(mockOrgRemove).toHaveBeenCalledWith(organization); + expect(mockLoggerError).toHaveBeenCalledTimes(1); + expect(mockLoggerError).toHaveBeenCalledWith( + 'organizations.crud.remove: org-removal cleanup handler failed after the org was removed (needs reconciliation)', + expect.objectContaining({ organizationId: 'org-1', message: 'tasks cleanup failed' }), + ); + }); + + test('logs each failure when multiple handlers throw, and still runs every handler (#3965)', async () => { + const failingA = jest.fn().mockRejectedValue(new Error('tasks cleanup failed')); + const failingB = jest.fn().mockRejectedValue(new Error('files cleanup failed')); + const succeeding = jest.fn().mockResolvedValue(undefined); + onOrganizationRemoved(failingA); + onOrganizationRemoved(failingB); + onOrganizationRemoved(succeeding); + + await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true }); + + expect(failingA).toHaveBeenCalledTimes(1); + expect(failingB).toHaveBeenCalledTimes(1); + expect(succeeding).toHaveBeenCalledTimes(1); + // One log line per failing handler (both aggregated failures surfaced individually). + expect(mockLoggerError).toHaveBeenCalledTimes(2); + expect(mockLoggerError).toHaveBeenCalledWith( + 'organizations.crud.remove: org-removal cleanup handler failed after the org was removed (needs reconciliation)', + expect.objectContaining({ organizationId: 'org-1', message: 'tasks cleanup failed' }), + ); + expect(mockLoggerError).toHaveBeenCalledWith( + 'organizations.crud.remove: org-removal cleanup handler failed after the org was removed (needs reconciliation)', + expect.objectContaining({ organizationId: 'org-1', message: 'files cleanup failed' }), + ); + }); + + test('a STRUCTURAL failure (org repository delete itself throws) propagates and is NOT swallowed', async () => { + mockOrgRemove.mockRejectedValueOnce(new Error('db delete failed')); + + await expect(OrgCrudService.remove(organization)).rejects.toThrow('db delete failed'); }); test('with zero handlers registered, removes the organization without throwing', async () => { await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true }); expect(mockOrgRemove).toHaveBeenCalledWith(organization); }); + + test('reassigns an affected user without throwing when their remaining membership.organizationId is null (dangling ref, #3709)', async () => { + mockFindWithFilter.mockResolvedValueOnce([{ _id: 'coUid' }]); + mockMembershipList.mockResolvedValueOnce([{ _id: 'm2', organizationId: null, status: 'active' }]); + + await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true }); + expect(mockUpdateById).toHaveBeenCalledWith('coUid', { currentOrganization: null }); + }); + + test('skips a null-org membership and picks the first live org when reassigning an affected user (mixed, #3709)', async () => { + mockFindWithFilter.mockResolvedValueOnce([{ _id: 'coUid' }]); + mockMembershipList.mockResolvedValueOnce([ + { _id: 'm2', organizationId: null, status: 'active' }, + { _id: 'm3', organizationId: { _id: 'orgY' }, status: 'active' }, + ]); + + await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true }); + expect(mockUpdateById).toHaveBeenCalledWith('coUid', { currentOrganization: 'orgY' }); + }); + + test('reassigns to a non-populated (raw id) organizationId when the membership was not populated', async () => { + mockFindWithFilter.mockResolvedValueOnce([{ _id: 'coUid' }]); + mockMembershipList.mockResolvedValueOnce([ + { _id: 'm4', organizationId: 'orgZ', status: 'active' }, + ]); + + await expect(OrgCrudService.remove(organization)).resolves.toEqual({ acknowledged: true }); + expect(mockUpdateById).toHaveBeenCalledWith('coUid', { currentOrganization: 'orgZ' }); + }); }); diff --git a/modules/organizations/tests/organizations.orgRemoval.registry.unit.tests.js b/modules/organizations/tests/organizations.orgRemoval.registry.unit.tests.js index 8bfd3e396..470e1e4ad 100644 --- a/modules/organizations/tests/organizations.orgRemoval.registry.unit.tests.js +++ b/modules/organizations/tests/organizations.orgRemoval.registry.unit.tests.js @@ -35,7 +35,7 @@ describe('orgRemoval.registry', () => { expect(order).toEqual(['first', 'second']); }); - test('propagates a handler error (does not swallow) and aborts the remaining handlers', async () => { + test('isolates a handler error — the remaining handlers still run, then failures re-raise as one AggregateError', async () => { const boom = new Error('cleanup failed'); const after = jest.fn().mockResolvedValue(undefined); onOrganizationRemoved(async () => { @@ -43,8 +43,27 @@ describe('orgRemoval.registry', () => { }); onOrganizationRemoved(after); - await expect(runOrganizationRemovedHandlers({ organizationId: 'org-1' })).rejects.toThrow('cleanup failed'); - expect(after).not.toHaveBeenCalled(); + await expect(runOrganizationRemovedHandlers({ organizationId: 'org-1' })).rejects.toThrow(AggregateError); + // A failing handler must not block a later one (avoids orphaning another module's rows). + expect(after).toHaveBeenCalledTimes(1); + }); + + test('aggregates every handler failure — each error is carried on the AggregateError', async () => { + const boomA = new Error('tasks cleanup failed'); + const boomB = new Error('files cleanup failed'); + const middle = jest.fn().mockResolvedValue(undefined); + onOrganizationRemoved(async () => { + throw boomA; + }); + onOrganizationRemoved(middle); + onOrganizationRemoved(async () => { + throw boomB; + }); + + await expect(runOrganizationRemovedHandlers({ organizationId: 'org-1' })).rejects.toMatchObject({ + errors: [boomA, boomB], + }); + expect(middle).toHaveBeenCalledTimes(1); }); test('runs zero handlers without throwing when none are registered', async () => { diff --git a/modules/tasks/tests/tasks.integration.tests.js b/modules/tasks/tests/tasks.integration.tests.js index 5a267529d..d08984e97 100644 --- a/modules/tasks/tests/tasks.integration.tests.js +++ b/modules/tasks/tests/tasks.integration.tests.js @@ -23,7 +23,6 @@ describe('Tasks integration tests:', () => { let _user; let _tasks; let task1; - let task2; // init beforeAll(async () => { @@ -43,6 +42,49 @@ describe('Tasks integration tests:', () => { }); describe('Logged', () => { + // A task belonging to a completely separate user/org, used to test cross-user + // authorization ("not a user's task"). Created ONCE via its own agent/session + // (not in the per-test beforeEach) so it survives this block's per-test afterEach + // (`UserService.remove(user)`) — which, since #3965, correctly cascades to delete + // a sole-owner's org-scoped tasks and would otherwise leave this fixture deleted + // partway through the suite (previously masked by the org-removal-seam bug: an + // orphaned-but-still-existing task used to linger regardless of its owner's fate). + let foreignAgent; + let foreignUser; + let foreignTask; + + beforeAll(async () => { + foreignAgent = request.agent(app); + try { + const result = await foreignAgent.post('/api/auth/signup').send({ + firstName: 'Foreign', + lastName: 'User', + email: 'task-foreign@test.com', + password: 'W@os.jsI$Aw3$0m3', + provider: 'local', + }).expect(200); + foreignUser = result.body.user; + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + try { + const result = await foreignAgent.post('/api/tasks').send({ title: 'foreign-task', description: 'belongs to a different user' }).expect(200); + foreignTask = result.body.data; + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + + afterAll(async () => { + try { + await UserService.remove(foreignUser); + } catch (err) { + console.log(err); + } + }); + beforeEach(async () => { // user _user = { @@ -92,7 +134,6 @@ describe('Tasks integration tests:', () => { // add task try { const result = await agent.post('/api/tasks').send(_tasks[1]).expect(200); - task2 = result.body.data; expect(result.body.type).toBe('success'); expect(result.body.message).toBe('task created'); expect(result.body.data.title).toBe(_tasks[1].title); @@ -192,7 +233,7 @@ describe('Tasks integration tests:', () => { test('should not be able to update a task if it is not a user task', async () => { // edit task try { - const result = await agent.put(`/api/tasks/${task2.id}`).send(_tasks[0]).expect(403); + const result = await agent.put(`/api/tasks/${foreignTask.id}`).send(_tasks[0]).expect(403); expect(result.body.type).toBe('error'); expect(result.body.message).toBe('Unauthorized'); expect(result.body.description).toBe('User is not authorized'); @@ -218,7 +259,7 @@ describe('Tasks integration tests:', () => { test('should not be able to remove a task if it is not a user task', async () => { // edit task try { - const result = await agent.delete(`/api/tasks/${task2.id}`).send(_tasks[0]).expect(403); + const result = await agent.delete(`/api/tasks/${foreignTask.id}`).send(_tasks[0]).expect(403); expect(result.body.type).toBe('error'); expect(result.body.message).toBe('Unauthorized'); expect(result.body.description).toBe('User is not authorized'); diff --git a/modules/users/services/users.service.js b/modules/users/services/users.service.js index 92155e52f..49564f653 100644 --- a/modules/users/services/users.service.js +++ b/modules/users/services/users.service.js @@ -11,7 +11,6 @@ import mailer from '../../../lib/helpers/mailer/index.js'; import passwordHelper from '../../../lib/helpers/password.js'; import UserRepository from '../repositories/users.repository.js'; import MembershipService from '../../organizations/services/organizations.membership.service.js'; -import OrganizationsRepository from '../../organizations/repositories/organizations.repository.js'; import MembershipRepository from '../../organizations/repositories/organizations.membership.repository.js'; import { MEMBERSHIP_ROLES, MEMBERSHIP_STATUSES } from '../../organizations/lib/constants.js'; import { removeSensitive } from '../utils/sanitizeUser.js'; @@ -176,21 +175,28 @@ const remove = async (user) => { // Check if this user is the only owner of the org const ownerCount = await MembershipService.count({ organizationId: orgId, role: MEMBERSHIP_ROLES.OWNER, status: MEMBERSHIP_STATUSES.ACTIVE }); if (ownerCount <= 1) { - // Sole owner — delete org and cascade: repair co-member currentOrganization - // Step 1: Collect co-members whose currentOrganization points to this org - const affectedUsers = await UserRepository.findWithFilter({ currentOrganization: orgId }, '_id'); - // Step 2: Delete all memberships for this org (including this user's and all co-members') - await MembershipService.deleteMany({ organizationId: orgId }); - // Step 3: For each affected co-member, switch to their next available org or set null - await Promise.all(affectedUsers.map(async (u) => { - const remaining = await MembershipRepository.list({ userId: u._id, status: MEMBERSHIP_STATUSES.ACTIVE }); - const liveMemberships = remaining.filter((m) => m.organizationId != null); - const nextOrg = liveMemberships.length > 0 ? (liveMemberships[0].organizationId._id || liveMemberships[0].organizationId) : null; - await UserRepository.updateById(u._id, { currentOrganization: nextOrg }); - })); - // Step 4: Delete the org (bare remove — org-scoped tasks are intentionally not deleted here) - await OrganizationsRepository.remove({ _id: orgId }); - continue; // memberships for this org already cleaned up + // Sole owner — delete the org through the canonical removal seam rather than + // duplicating its cascade here. organizations.crud.service.js#remove() owns the + // full contract: membership cleanup, co-member currentOrganization reassignment, + // and the org repository delete ALL happen first (atomic-by-ordering — the org + // is never left half-removed), and only THEN do registered onOrganizationRemoved + // handlers (e.g. task cleanup) run, best-effort — a handler failure is caught and + // logged inside that service, never re-thrown (#3965). + // + // No try/catch here on purpose: because handler failures are already swallowed + // (logged) inside the seam, anything that DOES escape this call is a STRUCTURAL + // failure (membership wipe / reassignment / the org repository delete itself + // throwing) — and that must propagate and abort this entire user deletion, same + // as the pre-#3965 behavior, so a user is never deleted on top of an org whose + // teardown genuinely broke. + // + // Lazy import: organizations.crud.service.js statically imports this module + // (UserService), so a static import here would create a cycle. Matches the + // lazy-import pattern already used for the same reason in billing.init.js / + // billing.referral.service.js. + const { default: OrganizationsCrudService } = await import('../../organizations/services/organizations.crud.service.js'); + await OrganizationsCrudService.remove({ _id: orgId }); + continue; // memberships for this org already cleaned up by the removal seam above } } // Delete this user's membership diff --git a/modules/users/tests/users.service.remove.orgRemovalSeam.unit.tests.js b/modules/users/tests/users.service.remove.orgRemovalSeam.unit.tests.js new file mode 100644 index 000000000..1ce8f6ed1 --- /dev/null +++ b/modules/users/tests/users.service.remove.orgRemovalSeam.unit.tests.js @@ -0,0 +1,172 @@ +/** + * Unit tests — deleting the sole-owner user of an organization must route the org + * deletion through the canonical org-removal seam (organizations.crud.service.js#remove, + * which calls runOrganizationRemovedHandlers) rather than the repository directly. + * Otherwise modules that register an onOrganizationRemoved handler (e.g. tasks — + * see modules/tasks/tasks.init.js) never get to clean up org-scoped data. Issue #3965. + * + * Also asserts the atomicity contract added to close a Phase-0 BLOCK finding: the org + * removal seam is atomic-by-ordering (org doc + memberships always gone together, + * handlers run best-effort AFTER), so a throwing onOrganizationRemoved handler can + * never leave a zombie org, and a STRUCTURAL failure in the seam propagates and aborts + * the whole user deletion (the user is never deleted on top of an inconsistent org). + * + * Uses the REAL org-removal registry (not mocked) so the assertion exercises the actual + * seam, mirroring the pattern in organizations.crud.orgRemoval.unit.tests.js. + */ +import { jest, describe, test, expect, beforeEach, afterEach } from '@jest/globals'; + +const mockMembershipServiceListByUser = jest.fn(); +const mockMembershipServiceCount = jest.fn(); +const mockMembershipServiceDeleteMany = jest.fn(); +const mockMembershipRepositoryList = jest.fn(); +const mockMembershipRepositoryDeleteMany = jest.fn(); +const mockUserRepositoryFindWithFilter = jest.fn(); +const mockUserRepositoryUpdateById = jest.fn(); +const mockUserRepositoryRemove = jest.fn(); +const mockOrgRepositoryRemove = jest.fn(); + +jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { error: jest.fn(), warn: jest.fn(), info: jest.fn() }, +})); + +jest.unstable_mockModule('../repositories/users.repository.js', () => ({ + default: { + list: jest.fn(), + create: jest.fn(), + get: jest.fn(), + update: jest.fn(), + remove: mockUserRepositoryRemove, + stats: jest.fn(), + updateById: mockUserRepositoryUpdateById, + findWithFilter: mockUserRepositoryFindWithFilter, + findByIdAndUpdatePopulated: jest.fn(), + searchByNameOrEmail: jest.fn(), + search: jest.fn(), + }, +})); + +jest.unstable_mockModule('../../organizations/services/organizations.membership.service.js', () => ({ + default: { + listByUser: mockMembershipServiceListByUser, + count: mockMembershipServiceCount, + deleteMany: mockMembershipServiceDeleteMany, + }, +})); + +jest.unstable_mockModule('../../organizations/repositories/organizations.repository.js', () => ({ + default: { + remove: mockOrgRepositoryRemove, + findOne: jest.fn().mockResolvedValue(null), + list: jest.fn().mockResolvedValue([]), + create: jest.fn(), + update: jest.fn(), + get: jest.fn(), + exists: jest.fn().mockResolvedValue(false), + updateById: jest.fn(), + }, +})); + +jest.unstable_mockModule('../../organizations/repositories/organizations.membership.repository.js', () => ({ + default: { + list: mockMembershipRepositoryList, + findOne: jest.fn(), + create: jest.fn(), + deleteMany: mockMembershipRepositoryDeleteMany, + update: jest.fn(), + remove: jest.fn(), + count: jest.fn(), + }, +})); + +jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { organizations: { enabled: true } }, +})); + +jest.unstable_mockModule('../utils/sanitizeUser.js', () => ({ + removeSensitive: jest.fn((u) => u), +})); + +jest.unstable_mockModule('lodash', () => ({ + default: { pick: jest.fn((o) => o) }, +})); + +const { default: UsersService } = await import('../services/users.service.js'); +const { onOrganizationRemoved, _reset } = await import('../../organizations/lib/orgRemoval.registry.js'); + +describe('users.service.remove — sole-owner org deletion routes through the org-removal seam (#3965):', () => { + const orgId = 'orgX'; + const ownerUser = { _id: 'ownerUid', id: 'ownerUid', currentOrganization: { _id: orgId } }; + + beforeEach(() => { + jest.clearAllMocks(); + _reset(); + mockUserRepositoryRemove.mockResolvedValue({ deletedCount: 1 }); + mockMembershipServiceDeleteMany.mockResolvedValue({}); + mockMembershipRepositoryDeleteMany.mockResolvedValue({}); + mockOrgRepositoryRemove.mockResolvedValue({}); + mockMembershipServiceCount.mockResolvedValue(1); // sole owner + mockUserRepositoryFindWithFilter.mockResolvedValue([]); // no co-members to reassign + mockMembershipRepositoryList.mockResolvedValue([]); + mockMembershipServiceListByUser.mockResolvedValue([ + { _id: 'm1', role: 'owner', organizationId: { _id: orgId }, status: 'active' }, + ]); + }); + + afterEach(() => { + _reset(); + }); + + test('fires the registered onOrganizationRemoved handler for the sole-owned org (task cleanup would run)', async () => { + const handler = jest.fn().mockResolvedValue(undefined); + onOrganizationRemoved(handler); + + await UsersService.remove(ownerUser); + + expect(handler).toHaveBeenCalledTimes(1); + expect(handler).toHaveBeenCalledWith({ organizationId: orgId, organization: { _id: orgId } }); + // The org itself must still be removed after the handler runs + expect(mockOrgRepositoryRemove).toHaveBeenCalledWith({ _id: orgId }); + }); + + test('still deletes the user AND fully removes the org when a registered handler throws (no zombie org, no aborted user deletion)', async () => { + const failingHandler = jest.fn().mockRejectedValue(new Error('task cleanup failed')); + onOrganizationRemoved(failingHandler); + + await expect(UsersService.remove(ownerUser)).resolves.not.toThrow(); + + expect(failingHandler).toHaveBeenCalledTimes(1); + // The org doc must still be removed despite the handler failure — a handler error + // is best-effort (logged) inside the removal seam, never a reason to leave a + // zombie org doc with its memberships already wiped. + expect(mockOrgRepositoryRemove).toHaveBeenCalledWith({ _id: orgId }); + // User deletion must still complete despite the downstream handler failure + expect(mockUserRepositoryRemove).toHaveBeenCalledWith(ownerUser); + }); + + test('with zero handlers registered, still removes the org (no regression for orgs with no cleanup consumers)', async () => { + await expect(UsersService.remove(ownerUser)).resolves.not.toThrow(); + expect(mockOrgRepositoryRemove).toHaveBeenCalledWith({ _id: orgId }); + expect(mockUserRepositoryRemove).toHaveBeenCalledWith(ownerUser); + }); + + test('aborts the user deletion when the org removal seam hits a STRUCTURAL failure (org repository delete throws) — user is NOT removed', async () => { + const structuralError = new Error('org repository delete failed (simulated DB error)'); + mockOrgRepositoryRemove.mockRejectedValueOnce(structuralError); + + await expect(UsersService.remove(ownerUser)).rejects.toThrow('org repository delete failed'); + + // The user must never be deleted on top of a teardown that genuinely broke. + expect(mockUserRepositoryRemove).not.toHaveBeenCalled(); + }); + + test('aborts the user deletion when the org removal seam hits a STRUCTURAL failure (membership wipe throws) — user is NOT removed', async () => { + const structuralError = new Error('membership wipe failed (simulated DB error)'); + mockMembershipRepositoryDeleteMany.mockRejectedValueOnce(structuralError); + + await expect(UsersService.remove(ownerUser)).rejects.toThrow('membership wipe failed'); + + expect(mockOrgRepositoryRemove).not.toHaveBeenCalled(); + expect(mockUserRepositoryRemove).not.toHaveBeenCalled(); + }); +});