Skip to content

Commit 2207602

Browse files
PierreBrisorgueilPierreBrisorgueil
committed
fix(users): repair co-member currentOrganization on sole-owner deletion (#3709)
Replace the bare OrganizationsCrudService.removeById call with an inline cascade that properly repairs co-member currentOrganization to their next available org (or null) instead of clearing it uniformly to null.
1 parent 8a50c8e commit 2207602

2 files changed

Lines changed: 189 additions & 6 deletions

File tree

‎modules/users/services/users.service.js‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@ import config from '../../../config/index.js';
77
import AuthService from '../../auth/services/auth.service.js';
88
import UserRepository from '../repositories/users.repository.js';
99
import MembershipService from '../../organizations/services/organizations.membership.service.js';
10-
import OrganizationsCrudService from '../../organizations/services/organizations.crud.service.js';
10+
import OrganizationsRepository from '../../organizations/repositories/organizations.repository.js';
11+
import MembershipRepository from '../../organizations/repositories/organizations.membership.repository.js';
1112
import { MEMBERSHIP_ROLES, MEMBERSHIP_STATUSES } from '../../organizations/lib/constants.js';
1213
import { removeSensitive } from '../utils/sanitizeUser.js';
1314

@@ -119,12 +120,20 @@ const remove = async (user) => {
119120
// Check if this user is the only owner of the org
120121
const ownerCount = await MembershipService.count({ organizationId: orgId, role: MEMBERSHIP_ROLES.OWNER, status: MEMBERSHIP_STATUSES.ACTIVE });
121122
if (ownerCount <= 1) {
122-
// Sole owner — delete the entire org and its memberships
123-
// Clear currentOrganization for affected users
124-
await UserRepository.updateMany({ currentOrganization: orgId }, { currentOrganization: null });
123+
// Sole owner — delete org and cascade: repair co-member currentOrganization
124+
// Step 1: Collect co-members whose currentOrganization points to this org
125+
const affectedUsers = await UserRepository.findWithFilter({ currentOrganization: orgId }, '_id');
126+
// Step 2: Delete all memberships for this org (including this user's and all co-members')
125127
await MembershipService.deleteMany({ organizationId: orgId });
126-
await OrganizationsCrudService.removeById(orgId);
127-
continue; // membership already deleted above
128+
// Step 3: For each affected co-member, switch to their next available org or set null
129+
await Promise.all(affectedUsers.map(async (u) => {
130+
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;
132+
await UserRepository.updateById(u._id, { currentOrganization: nextOrg });
133+
}));
134+
// Step 4: Delete the org (bare remove — org-scoped tasks are intentionally not deleted here)
135+
await OrganizationsRepository.remove({ _id: orgId });
136+
continue; // memberships for this org already cleaned up
128137
}
129138
}
130139
// Delete this user's membership

‎modules/users/tests/users.remove.cascade.e2e.tests.js‎

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
/**
2+
* E2E test: user deletion cascades org cleanup so co-members can still sign in.
3+
* Repro from issue #3709: User A (sole owner) deleted → Org X removed.
4+
* User B (active member of Org X, currentOrganization=X) must be able to sign in
5+
* without 500, with currentOrganization cleared to null.
6+
*/
7+
import request from 'supertest';
8+
import path from 'path';
9+
import { bootstrap } from '../../../lib/app.js';
10+
import mongooseService from '../../../lib/services/mongoose.js';
11+
import config from '../../../config/index.js';
12+
13+
describe('users.service.remove cascade (#3709):', () => {
14+
let UserService;
15+
let OrganizationsRepository;
16+
let MembershipRepository;
17+
let agent;
18+
19+
const originalOrganizations = { ...config.organizations };
20+
21+
beforeAll(async () => {
22+
try {
23+
const init = await bootstrap();
24+
UserService = (await import(path.resolve('./modules/users/services/users.service.js'))).default;
25+
OrganizationsRepository = (await import(path.resolve('./modules/organizations/repositories/organizations.repository.js'))).default;
26+
MembershipRepository = (await import(path.resolve('./modules/organizations/repositories/organizations.membership.repository.js'))).default;
27+
agent = request.agent(init.app);
28+
} catch (err) {
29+
console.log(err);
30+
expect(err).toBeFalsy();
31+
}
32+
});
33+
34+
afterAll(async () => {
35+
config.organizations = { ...originalOrganizations };
36+
try {
37+
await mongooseService.disconnect();
38+
} catch (err) {
39+
console.log(err);
40+
expect(err).toBeFalsy();
41+
}
42+
});
43+
44+
describe('User B can sign in after User A (sole owner) is deleted', () => {
45+
let userA;
46+
let userB;
47+
let orgX;
48+
let agentA;
49+
let agentB;
50+
51+
beforeAll(async () => {
52+
config.organizations = { enabled: true, autoCreate: true, domainMatching: false };
53+
agentA = request.agent(agent.app);
54+
agentB = request.agent(agent.app);
55+
});
56+
57+
afterAll(async () => {
58+
// Best-effort cleanup
59+
try {
60+
if (userB) await UserService.remove(userB);
61+
} catch (_) { /* cleanup */ }
62+
try {
63+
if (orgX) {
64+
await MembershipRepository.deleteMany({ organizationId: orgX._id });
65+
await OrganizationsRepository.deleteMany({ _id: orgX._id });
66+
}
67+
} catch (_) { /* cleanup */ }
68+
});
69+
70+
test('should not 500 on signin when currentOrganization points to a deleted org (issue #3709 repro)', async () => {
71+
// Step 1: User A signs up — auto-creates Org X
72+
try {
73+
const resA = await agentA
74+
.post('/api/auth/signup')
75+
.send({
76+
firstName: 'CascadeA',
77+
lastName: 'User',
78+
email: 'cascade-a-3709@test.com',
79+
password: 'W@os.jsI$Aw3$0m3',
80+
provider: 'local',
81+
})
82+
.expect(200);
83+
userA = resA.body.user;
84+
orgX = resA.body.organization;
85+
expect(orgX).toBeDefined();
86+
expect(orgX).not.toBeNull();
87+
} catch (err) {
88+
console.log(err);
89+
expect(err).toBeFalsy();
90+
}
91+
92+
// Step 2: User B signs up separately
93+
try {
94+
const resB = await agentB
95+
.post('/api/auth/signup')
96+
.send({
97+
firstName: 'CascadeB',
98+
lastName: 'User',
99+
email: 'cascade-b-3709@test.com',
100+
password: 'W@os.jsI$Aw3$0m3',
101+
provider: 'local',
102+
})
103+
.expect(200);
104+
userB = resB.body.user;
105+
} catch (err) {
106+
console.log(err);
107+
expect(err).toBeFalsy();
108+
}
109+
110+
// Step 3: Directly create an ACTIVE membership for User B on Org X + set currentOrganization
111+
// (bypassing invite flow for test speed)
112+
try {
113+
const MembershipService = (await import(path.resolve('./modules/organizations/services/organizations.membership.service.js'))).default;
114+
await MembershipService.create({
115+
userId: userB._id || userB.id,
116+
organizationId: orgX._id,
117+
role: 'member',
118+
status: 'active',
119+
});
120+
// Set User B's currentOrganization to Org X directly
121+
await UserService.updateById(userB._id || userB.id, { currentOrganization: orgX._id });
122+
} catch (err) {
123+
console.log(err);
124+
expect(err).toBeFalsy();
125+
}
126+
127+
// Step 4: Delete User A (sole owner of Org X) — this should cascade-delete Org X + all memberships
128+
try {
129+
const brutUserA = await UserService.getBrut({ id: userA.id });
130+
await UserService.remove(brutUserA);
131+
userA = null; // Mark as cleaned
132+
} catch (err) {
133+
console.log(err);
134+
expect(err).toBeFalsy();
135+
}
136+
137+
// Step 5: Verify Org X no longer exists
138+
try {
139+
const org = await OrganizationsRepository.get(orgX._id);
140+
expect(org).toBeNull();
141+
} catch (err) {
142+
console.log(err);
143+
expect(err).toBeFalsy();
144+
}
145+
146+
// Step 6: Verify User B's currentOrganization has been cleared (not left dangling)
147+
try {
148+
const brutUserB = await UserService.getBrut({ id: userB.id });
149+
// currentOrganization must be null or undefined — not the deleted org ID
150+
const currentOrgId = brutUserB.currentOrganization?._id || brutUserB.currentOrganization;
151+
expect(currentOrgId == null || String(currentOrgId) !== String(orgX._id)).toBe(true);
152+
} catch (err) {
153+
console.log(err);
154+
expect(err).toBeFalsy();
155+
}
156+
157+
// Step 7: User B signs in — must NOT 500 (was: "Cannot read properties of null (reading '_id')")
158+
try {
159+
const signinRes = await agentB
160+
.post('/api/auth/signin')
161+
.send({ email: 'cascade-b-3709@test.com', password: 'W@os.jsI$Aw3$0m3' })
162+
.expect(200); // Must be 200, not 500
163+
164+
expect(signinRes.body.type).toBe('success');
165+
// currentOrganization must NOT be the deleted Org X (could be null or User B's own org)
166+
const signedInOrgId = signinRes.body.user.currentOrganization?._id || signinRes.body.user.currentOrganization;
167+
expect(signedInOrgId == null || String(signedInOrgId) !== String(orgX._id)).toBe(true);
168+
} catch (err) {
169+
console.log(err);
170+
expect(err).toBeFalsy();
171+
}
172+
});
173+
});
174+
});

0 commit comments

Comments
 (0)