Skip to content

Commit 19f69c6

Browse files
fix(users): guard null org in deletion-cascade nextOrg (#3709)
Co-member cascade in `users.service.remove` was dereferencing `remaining[0].organizationId._id` without filtering out null-populated memberships (hard-deleted org), mirroring the same crash fixed in `autoSetCurrentOrganization`. Apply the identical `liveMemberships` filter before picking the next current org. Add unit tests covering the null-only and mixed cases.
1 parent 7c02787 commit 19f69c6

2 files changed

Lines changed: 138 additions & 1 deletion

File tree

modules/users/services/users.service.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,8 @@ const remove = async (user) => {
128128
// Step 3: For each affected co-member, switch to their next available org or set null
129129
await Promise.all(affectedUsers.map(async (u) => {
130130
const remaining = await MembershipRepository.list({ userId: u._id, status: MEMBERSHIP_STATUSES.ACTIVE });
131-
const nextOrg = remaining.length > 0 ? (remaining[0].organizationId._id || remaining[0].organizationId) : null;
131+
const liveMemberships = remaining.filter((m) => m.organizationId != null);
132+
const nextOrg = liveMemberships.length > 0 ? (liveMemberships[0].organizationId._id || liveMemberships[0].organizationId) : null;
132133
await UserRepository.updateById(u._id, { currentOrganization: nextOrg });
133134
}));
134135
// Step 4: Delete the org (bare remove — org-scoped tasks are intentionally not deleted here)
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
/**
2+
* Unit tests — users.service.remove deletion-cascade must not crash when
3+
* a co-member's membership.organizationId is null after populate (deleted org,
4+
* dangling ref). Same pattern guarded in autoSetCurrentOrganization. Issue #3709.
5+
*/
6+
import { jest, describe, test, expect } from '@jest/globals';
7+
8+
const mockMembershipServiceListByUser = jest.fn();
9+
const mockMembershipServiceCount = jest.fn();
10+
const mockMembershipServiceDeleteMany = jest.fn();
11+
const mockMembershipRepositoryList = jest.fn();
12+
const mockUserRepositoryFindWithFilter = jest.fn();
13+
const mockUserRepositoryUpdateById = jest.fn();
14+
const mockUserRepositoryRemove = jest.fn();
15+
const mockOrgRepositoryRemove = jest.fn();
16+
17+
jest.unstable_mockModule('../../../lib/services/logger.js', () => ({
18+
default: { error: jest.fn(), warn: jest.fn(), info: jest.fn() },
19+
}));
20+
21+
jest.unstable_mockModule('../repositories/users.repository.js', () => ({
22+
default: {
23+
list: jest.fn(),
24+
create: jest.fn(),
25+
get: jest.fn(),
26+
update: jest.fn(),
27+
remove: mockUserRepositoryRemove,
28+
stats: jest.fn(),
29+
updateById: mockUserRepositoryUpdateById,
30+
findWithFilter: mockUserRepositoryFindWithFilter,
31+
findByIdAndUpdatePopulated: jest.fn(),
32+
searchByNameOrEmail: jest.fn(),
33+
search: jest.fn(),
34+
},
35+
}));
36+
37+
jest.unstable_mockModule('../../organizations/services/organizations.membership.service.js', () => ({
38+
default: {
39+
listByUser: mockMembershipServiceListByUser,
40+
count: mockMembershipServiceCount,
41+
deleteMany: mockMembershipServiceDeleteMany,
42+
},
43+
}));
44+
45+
jest.unstable_mockModule('../../organizations/repositories/organizations.repository.js', () => ({
46+
default: {
47+
remove: mockOrgRepositoryRemove,
48+
findOne: jest.fn().mockResolvedValue(null),
49+
list: jest.fn().mockResolvedValue([]),
50+
create: jest.fn(),
51+
update: jest.fn(),
52+
get: jest.fn(),
53+
exists: jest.fn().mockResolvedValue(false),
54+
updateById: jest.fn(),
55+
},
56+
}));
57+
58+
jest.unstable_mockModule('../../organizations/repositories/organizations.membership.repository.js', () => ({
59+
default: {
60+
list: mockMembershipRepositoryList,
61+
findOne: jest.fn(),
62+
create: jest.fn(),
63+
deleteMany: jest.fn(),
64+
update: jest.fn(),
65+
remove: jest.fn(),
66+
count: jest.fn(),
67+
},
68+
}));
69+
70+
jest.unstable_mockModule('../../auth/services/auth.service.js', () => ({
71+
default: { signOut: jest.fn() },
72+
}));
73+
74+
jest.unstable_mockModule('../../../config/index.js', () => ({
75+
default: { organizations: { enabled: true } },
76+
}));
77+
78+
jest.unstable_mockModule('../utils/sanitizeUser.js', () => ({
79+
removeSensitive: jest.fn((u) => u),
80+
}));
81+
82+
jest.unstable_mockModule('lodash', () => ({
83+
default: { pick: jest.fn((o) => o) },
84+
}));
85+
86+
const { default: UsersService } = await import('../services/users.service.js');
87+
88+
describe('users.service.remove deletion-cascade — dangling org ref (#3709):', () => {
89+
const ownerUser = { _id: 'ownerUid', id: 'ownerUid', currentOrganization: { _id: 'orgX' } };
90+
const orgId = 'orgX';
91+
92+
beforeEach(() => {
93+
jest.clearAllMocks();
94+
mockUserRepositoryRemove.mockResolvedValue({ deletedCount: 1 });
95+
mockMembershipServiceDeleteMany.mockResolvedValue({});
96+
mockOrgRepositoryRemove.mockResolvedValue({});
97+
mockMembershipServiceCount.mockResolvedValue(1); // sole owner
98+
});
99+
100+
test('should not throw when co-member remaining membership.organizationId is null (deleted org)', async () => {
101+
// Owner user has one OWNER membership
102+
mockMembershipServiceListByUser.mockResolvedValue([
103+
{ _id: 'm1', role: 'owner', organizationId: { _id: orgId }, status: 'active' },
104+
]);
105+
mockMembershipServiceCount.mockResolvedValue(1); // sole owner → org deleted
106+
// One co-member (user B) whose currentOrganization = orgX
107+
mockUserRepositoryFindWithFilter.mockResolvedValue([{ _id: 'coUid' }]);
108+
// User B's remaining memberships: all point to deleted org (null populate)
109+
mockMembershipRepositoryList.mockResolvedValue([
110+
{ _id: 'm2', organizationId: null, status: 'active' },
111+
]);
112+
mockUserRepositoryUpdateById.mockResolvedValue({});
113+
114+
// Must not throw; co-member's currentOrganization must be set to null
115+
await expect(UsersService.remove(ownerUser)).resolves.not.toThrow();
116+
expect(mockUserRepositoryUpdateById).toHaveBeenCalledWith('coUid', { currentOrganization: null });
117+
});
118+
119+
test('should skip null-org memberships and pick first live org for co-member when mixed', async () => {
120+
mockMembershipServiceListByUser.mockResolvedValue([
121+
{ _id: 'm1', role: 'owner', organizationId: { _id: orgId }, status: 'active' },
122+
]);
123+
mockMembershipServiceCount.mockResolvedValue(1);
124+
mockUserRepositoryFindWithFilter.mockResolvedValue([{ _id: 'coUid' }]);
125+
// Co-member has two memberships: first null (deleted org), second live org
126+
mockMembershipRepositoryList.mockResolvedValue([
127+
{ _id: 'm2', organizationId: null, status: 'active' },
128+
{ _id: 'm3', organizationId: { _id: 'orgY' }, status: 'active' },
129+
]);
130+
mockUserRepositoryUpdateById.mockResolvedValue({});
131+
132+
await expect(UsersService.remove(ownerUser)).resolves.not.toThrow();
133+
// Must pick the live org (orgY), not crash on null._id
134+
expect(mockUserRepositoryUpdateById).toHaveBeenCalledWith('coUid', { currentOrganization: 'orgY' });
135+
});
136+
});

0 commit comments

Comments
 (0)