diff --git a/modules/auth/tests/auth.e2e.tests.js b/modules/auth/tests/auth.e2e.tests.js index b2971a33e..0438b8783 100644 --- a/modules/auth/tests/auth.e2e.tests.js +++ b/modules/auth/tests/auth.e2e.tests.js @@ -237,6 +237,42 @@ describe('Auth E2E tests:', () => { expect(err).toBeFalsy(); } }); + + test('should return 200 on signin even when autoSetCurrentOrganization would encounter a data-integrity anomaly', async () => { + config.organizations = { enabled: true, autoCreate: true, domainMatching: false }; + + // Signup user, then directly corrupt their currentOrganization to a non-existent ObjectId + // to simulate a pre-existing dangling ref in prod + try { + const signupRes = await agent + .post('/api/auth/signup') + .send({ + firstName: 'Danglingref', + lastName: 'User', + email: 'e2e-dangling-ref-3709@test.com', + password: 'W@os.jsI$Aw3$0m3', + provider: 'local', + }) + .expect(200); + + user = signupRes.body.user; + // Directly write a bogus ObjectId as currentOrganization (simulates pre-existing corruption) + const mongoose = (await import('mongoose')).default; + const User = mongoose.model('User'); + await User.updateOne({ _id: user.id }, { currentOrganization: new mongoose.Types.ObjectId() }); + + // Signin must NOT 500 + const signinRes = await agent + .post('/api/auth/signin') + .send({ email: 'e2e-dangling-ref-3709@test.com', password: 'W@os.jsI$Aw3$0m3' }) + .expect(200); + + expect(signinRes.body.type).toBe('success'); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); }); // Mongoose disconnect diff --git a/modules/organizations/services/organizations.crud.service.js b/modules/organizations/services/organizations.crud.service.js index 6071ddc64..be563056d 100644 --- a/modules/organizations/services/organizations.crud.service.js +++ b/modules/organizations/services/organizations.crud.service.js @@ -253,13 +253,16 @@ const autoSetCurrentOrganization = async (user) => { organizationId: user.currentOrganization._id || user.currentOrganization, status: MEMBERSHIP_STATUSES.ACTIVE, }); - if (stillActive) return user; - // Membership gone — clear stale reference and fall through to find another + // Guard: populated organizationId may be null when the org was hard-deleted + if (stillActive && stillActive.organizationId != null) return user; + // Membership gone or org deleted — clear stale reference and fall through to find another user.currentOrganization = null; } const memberships = await MembershipRepository.list({ userId: user._id || user.id, status: MEMBERSHIP_STATUSES.ACTIVE }); - if (memberships.length > 0) { - const orgId = memberships[0].organizationId._id || memberships[0].organizationId; + // Filter out memberships whose org was deleted (Mongoose populate sets organizationId to null) + const liveMemberships = memberships.filter((m) => m.organizationId != null); + if (liveMemberships.length > 0) { + const orgId = liveMemberships[0].organizationId._id || liveMemberships[0].organizationId; await UserService.updateById(user._id || user.id, { currentOrganization: orgId }); user.currentOrganization = orgId; } else { diff --git a/modules/organizations/tests/organizations.crud.dangling-org.unit.tests.js b/modules/organizations/tests/organizations.crud.dangling-org.unit.tests.js new file mode 100644 index 000000000..804673c01 --- /dev/null +++ b/modules/organizations/tests/organizations.crud.dangling-org.unit.tests.js @@ -0,0 +1,115 @@ +/** + * Unit tests — autoSetCurrentOrganization must not crash when membership.organizationId + * is null after populate (deleted org, dangling ref). Issue #3709. + */ +import { jest, describe, test, expect } from '@jest/globals'; + +const mockMembershipFindOne = jest.fn(); +const mockMembershipList = jest.fn(); +const mockUpdateById = jest.fn(); +const mockOrgRemove = jest.fn(); + +jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { error: jest.fn(), warn: jest.fn(), info: jest.fn() }, +})); + +jest.unstable_mockModule('../repositories/organizations.repository.js', () => ({ + default: { + create: jest.fn(), + findOne: jest.fn().mockResolvedValue(null), + remove: mockOrgRemove, + list: jest.fn().mockResolvedValue([]), + update: jest.fn(), + get: jest.fn(), + exists: jest.fn().mockResolvedValue(false), + updateById: jest.fn(), + }, +})); + +jest.unstable_mockModule('../repositories/organizations.membership.repository.js', () => ({ + default: { + create: jest.fn(), + deleteMany: jest.fn(), + list: mockMembershipList, + findOne: mockMembershipFindOne, + update: jest.fn(), + remove: jest.fn(), + count: jest.fn(), + }, +})); + +jest.unstable_mockModule('../../users/services/users.service.js', () => ({ + default: { + updateById: mockUpdateById, + findWithFilter: jest.fn().mockResolvedValue([]), + getBrut: jest.fn(), + }, +})); + +jest.unstable_mockModule('../../../lib/helpers/emailVerification.js', () => ({ + assertEmailVerified: jest.fn(), +})); + +jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { organizations: { enabled: true } }, +})); + +const { default: OrgCrudService } = await import('../services/organizations.crud.service.js'); + +describe('autoSetCurrentOrganization — dangling org ref (#3709):', () => { + const user = { _id: 'uid1', id: 'uid1', currentOrganization: null }; + + beforeEach(() => { + jest.clearAllMocks(); + mockUpdateById.mockResolvedValue({}); + }); + + test('should not throw when membership.organizationId is null after populate', async () => { + // Simulate: user has an active membership but the org is deleted (populate yields null) + mockMembershipList.mockResolvedValue([ + { _id: 'm1', organizationId: null, status: 'active' }, // null = deleted org + ]); + + // Must not throw; must set currentOrganization to null (no live org) + const result = await OrgCrudService.autoSetCurrentOrganization({ ...user }); + expect(result.currentOrganization).toBeNull(); + expect(mockUpdateById).toHaveBeenCalledWith('uid1', { currentOrganization: null }); + }); + + test('should skip null-org memberships and pick first live org when mixed', async () => { + // Two memberships: first has deleted org (null populate), second has live org + mockMembershipList.mockResolvedValue([ + { _id: 'm1', organizationId: null, status: 'active' }, // deleted org + { _id: 'm2', organizationId: { _id: 'org2' }, status: 'active' }, // live org + ]); + + const result = await OrgCrudService.autoSetCurrentOrganization({ ...user }); + expect(result.currentOrganization).toBe('org2'); + expect(mockUpdateById).toHaveBeenCalledWith('uid1', { currentOrganization: 'org2' }); + }); + + test('should return user unchanged when currentOrganization is set and membership is live with a live org', async () => { + const userWithOrg = { _id: 'uid1', id: 'uid1', currentOrganization: { _id: 'org1' } }; + mockMembershipFindOne.mockResolvedValue({ _id: 'm1', organizationId: { _id: 'org1' } }); + + const result = await OrgCrudService.autoSetCurrentOrganization(userWithOrg); + // Early return — no updateById called + expect(mockUpdateById).not.toHaveBeenCalled(); + expect(result.currentOrganization).toEqual({ _id: 'org1' }); + }); + + test('should fall through and clear currentOrganization when membership exists but org is null-populated', async () => { + // Membership still exists (findOne returns it) but org is deleted (organizationId = null) + const userWithOrg = { _id: 'uid1', id: 'uid1', currentOrganization: { _id: 'org1' } }; + // findOne called in early branch — membership exists (stillActive truthy) but org deleted + mockMembershipFindOne.mockResolvedValue({ _id: 'm1', organizationId: null }); + // list called in fallback branch — also returns the same dangling membership + mockMembershipList.mockResolvedValue([ + { _id: 'm1', organizationId: null, status: 'active' }, + ]); + + const result = await OrgCrudService.autoSetCurrentOrganization(userWithOrg); + expect(result.currentOrganization).toBeNull(); + expect(mockUpdateById).toHaveBeenCalledWith('uid1', { currentOrganization: null }); + }); +}); diff --git a/modules/users/services/users.service.js b/modules/users/services/users.service.js index 5287e216d..c3893400c 100644 --- a/modules/users/services/users.service.js +++ b/modules/users/services/users.service.js @@ -7,7 +7,8 @@ import config from '../../../config/index.js'; import AuthService from '../../auth/services/auth.service.js'; import UserRepository from '../repositories/users.repository.js'; import MembershipService from '../../organizations/services/organizations.membership.service.js'; -import OrganizationsCrudService from '../../organizations/services/organizations.crud.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'; @@ -119,12 +120,21 @@ 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 the entire org and its memberships - // Clear currentOrganization for affected users - await UserRepository.updateMany({ currentOrganization: orgId }, { currentOrganization: null }); + // 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 }); - await OrganizationsCrudService.removeById(orgId); - continue; // membership already deleted above + // 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 } } // Delete this user's membership diff --git a/modules/users/tests/users.remove.cascade.e2e.tests.js b/modules/users/tests/users.remove.cascade.e2e.tests.js new file mode 100644 index 000000000..bcf1182bd --- /dev/null +++ b/modules/users/tests/users.remove.cascade.e2e.tests.js @@ -0,0 +1,174 @@ +/** + * E2E test: user deletion cascades org cleanup so co-members can still sign in. + * Repro from issue #3709: User A (sole owner) deleted → Org X removed. + * User B (active member of Org X, currentOrganization=X) must be able to sign in + * without 500, with currentOrganization cleared to null. + */ +import request from 'supertest'; +import path from 'path'; +import { bootstrap } from '../../../lib/app.js'; +import mongooseService from '../../../lib/services/mongoose.js'; +import config from '../../../config/index.js'; + +describe('users.service.remove cascade (#3709):', () => { + let UserService; + let OrganizationsRepository; + let MembershipRepository; + let agent; + + const originalOrganizations = { ...config.organizations }; + + beforeAll(async () => { + try { + const init = await bootstrap(); + UserService = (await import(path.resolve('./modules/users/services/users.service.js'))).default; + OrganizationsRepository = (await import(path.resolve('./modules/organizations/repositories/organizations.repository.js'))).default; + MembershipRepository = (await import(path.resolve('./modules/organizations/repositories/organizations.membership.repository.js'))).default; + agent = request.agent(init.app); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + + afterAll(async () => { + config.organizations = { ...originalOrganizations }; + try { + await mongooseService.disconnect(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + + describe('User B can sign in after User A (sole owner) is deleted', () => { + let userA; + let userB; + let orgX; + let agentA; + let agentB; + + beforeAll(async () => { + config.organizations = { enabled: true, autoCreate: true, domainMatching: false }; + agentA = request.agent(agent.app); + agentB = request.agent(agent.app); + }); + + afterAll(async () => { + // Best-effort cleanup + try { + if (userB) await UserService.remove(userB); + } catch (_) { /* cleanup */ } + try { + if (orgX) { + await MembershipRepository.deleteMany({ organizationId: orgX._id }); + await OrganizationsRepository.deleteMany({ _id: orgX._id }); + } + } catch (_) { /* cleanup */ } + }); + + test('should not 500 on signin when currentOrganization points to a deleted org (issue #3709 repro)', async () => { + // Step 1: User A signs up — auto-creates Org X + try { + const resA = await agentA + .post('/api/auth/signup') + .send({ + firstName: 'CascadeA', + lastName: 'User', + email: 'cascade-a-3709@test.com', + password: 'W@os.jsI$Aw3$0m3', + provider: 'local', + }) + .expect(200); + userA = resA.body.user; + orgX = resA.body.organization; + expect(orgX).toBeDefined(); + expect(orgX).not.toBeNull(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + + // Step 2: User B signs up separately + try { + const resB = await agentB + .post('/api/auth/signup') + .send({ + firstName: 'CascadeB', + lastName: 'User', + email: 'cascade-b-3709@test.com', + password: 'W@os.jsI$Aw3$0m3', + provider: 'local', + }) + .expect(200); + userB = resB.body.user; + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + + // Step 3: Directly create an ACTIVE membership for User B on Org X + set currentOrganization + // (bypassing invite flow for test speed) + try { + const MembershipService = (await import(path.resolve('./modules/organizations/services/organizations.membership.service.js'))).default; + await MembershipService.create({ + userId: userB._id || userB.id, + organizationId: orgX._id, + role: 'member', + status: 'active', + }); + // Set User B's currentOrganization to Org X directly + await UserService.updateById(userB._id || userB.id, { currentOrganization: orgX._id }); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + + // Step 4: Delete User A (sole owner of Org X) — this should cascade-delete Org X + all memberships + try { + const brutUserA = await UserService.getBrut({ id: userA.id }); + await UserService.remove(brutUserA); + userA = null; // Mark as cleaned + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + + // Step 5: Verify Org X no longer exists + try { + const org = await OrganizationsRepository.get(orgX._id); + expect(org).toBeNull(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + + // Step 6: Verify User B's currentOrganization has been cleared (not left dangling) + try { + const brutUserB = await UserService.getBrut({ id: userB.id }); + // currentOrganization must be null or undefined — not the deleted org ID + const currentOrgId = brutUserB.currentOrganization?._id || brutUserB.currentOrganization; + expect(currentOrgId == null || String(currentOrgId) !== String(orgX._id)).toBe(true); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + + // Step 7: User B signs in — must NOT 500 (was: "Cannot read properties of null (reading '_id')") + try { + const signinRes = await agentB + .post('/api/auth/signin') + .send({ email: 'cascade-b-3709@test.com', password: 'W@os.jsI$Aw3$0m3' }) + .expect(200); // Must be 200, not 500 + + expect(signinRes.body.type).toBe('success'); + // currentOrganization must NOT be the deleted Org X (could be null or User B's own org) + const signedInOrgId = signinRes.body.user.currentOrganization?._id || signinRes.body.user.currentOrganization; + expect(signedInOrgId == null || String(signedInOrgId) !== String(orgX._id)).toBe(true); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + }); +}); diff --git a/modules/users/tests/users.service.remove.cascade.unit.tests.js b/modules/users/tests/users.service.remove.cascade.unit.tests.js new file mode 100644 index 000000000..b49ad36bb --- /dev/null +++ b/modules/users/tests/users.service.remove.cascade.unit.tests.js @@ -0,0 +1,136 @@ +/** + * Unit tests — users.service.remove deletion-cascade must not crash when + * a co-member's membership.organizationId is null after populate (deleted org, + * dangling ref). Same pattern guarded in autoSetCurrentOrganization. Issue #3709. + */ +import { jest, describe, test, expect } from '@jest/globals'; + +const mockMembershipServiceListByUser = jest.fn(); +const mockMembershipServiceCount = jest.fn(); +const mockMembershipServiceDeleteMany = jest.fn(); +const mockMembershipRepositoryList = 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: jest.fn(), + update: jest.fn(), + remove: jest.fn(), + count: jest.fn(), + }, +})); + +jest.unstable_mockModule('../../auth/services/auth.service.js', () => ({ + default: { signOut: 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'); + +describe('users.service.remove deletion-cascade — dangling org ref (#3709):', () => { + const ownerUser = { _id: 'ownerUid', id: 'ownerUid', currentOrganization: { _id: 'orgX' } }; + const orgId = 'orgX'; + + beforeEach(() => { + jest.clearAllMocks(); + mockUserRepositoryRemove.mockResolvedValue({ deletedCount: 1 }); + mockMembershipServiceDeleteMany.mockResolvedValue({}); + mockOrgRepositoryRemove.mockResolvedValue({}); + mockMembershipServiceCount.mockResolvedValue(1); // sole owner + }); + + test('should not throw when co-member remaining membership.organizationId is null (deleted org)', async () => { + // Owner user has one OWNER membership + mockMembershipServiceListByUser.mockResolvedValue([ + { _id: 'm1', role: 'owner', organizationId: { _id: orgId }, status: 'active' }, + ]); + mockMembershipServiceCount.mockResolvedValue(1); // sole owner → org deleted + // One co-member (user B) whose currentOrganization = orgX + mockUserRepositoryFindWithFilter.mockResolvedValue([{ _id: 'coUid' }]); + // User B's remaining memberships: all point to deleted org (null populate) + mockMembershipRepositoryList.mockResolvedValue([ + { _id: 'm2', organizationId: null, status: 'active' }, + ]); + mockUserRepositoryUpdateById.mockResolvedValue({}); + + // Must not throw; co-member's currentOrganization must be set to null + await expect(UsersService.remove(ownerUser)).resolves.not.toThrow(); + expect(mockUserRepositoryUpdateById).toHaveBeenCalledWith('coUid', { currentOrganization: null }); + }); + + test('should skip null-org memberships and pick first live org for co-member when mixed', async () => { + mockMembershipServiceListByUser.mockResolvedValue([ + { _id: 'm1', role: 'owner', organizationId: { _id: orgId }, status: 'active' }, + ]); + mockMembershipServiceCount.mockResolvedValue(1); + mockUserRepositoryFindWithFilter.mockResolvedValue([{ _id: 'coUid' }]); + // Co-member has two memberships: first null (deleted org), second live org + mockMembershipRepositoryList.mockResolvedValue([ + { _id: 'm2', organizationId: null, status: 'active' }, + { _id: 'm3', organizationId: { _id: 'orgY' }, status: 'active' }, + ]); + mockUserRepositoryUpdateById.mockResolvedValue({}); + + await expect(UsersService.remove(ownerUser)).resolves.not.toThrow(); + // Must pick the live org (orgY), not crash on null._id + expect(mockUserRepositoryUpdateById).toHaveBeenCalledWith('coUid', { currentOrganization: 'orgY' }); + }); +});