Skip to content

Commit 5e166d7

Browse files
feat(organizations): always-create workspace + suggestedJoin hint (drop onboarding limbo) (#3680)
* feat(organizations): canonical email-domain normalization helper * feat(organizations): always-create workspace + suggestedJoin hint, drop autoCreate footgun Spec D5 — every signup provisions an active workspace. autoCreate config flag is a deprecated no-op; no path returns organizationSetupRequired or pendingJoin. - handleSignupOrganization: delete autoCreate:false footgun branch entirely; all enabled paths call createOrganizationForUser returning {organization, membership}. - Domain-match branch: drop createJoinRequest+pendingJoin; return suggestedJoin: {orgId,orgName} name-only hint alongside the newly created workspace. - Use A1 isPublicDomain for the public-domain gate (hardcoded list + config overrides). - Naming: domainMatching+corporate domain → domain-based name; all other cases (domainMatching off, public domain) → "{firstName}'s organization". - signupGrant: credited inside createOrganizationForUser, once per real new org, never double-credited. Preserved exactly from pre-A2. - Auth controller: no change needed — uses ||false/||null fallbacks on removed keys. - Test updates (required 0-failure): emailVerification, domainJoin E2E, auth signup integration + auth E2E updated to assert new always-create contract. - New unit test: organizations.service.signup.unit.tests.js — all config combos, suggestedJoin shape (name-only), signupGrant credited once, createJoinRequest never. * fix(organizations): remove dead imports + harden signup test mock isolation - I1: remove unused MembershipService import and dead sanitizeOrg helper (both orphaned by A2); file is now lint-clean (no-unused-vars: 0) - I2: replace clearAllMocks() with resetAllMocks() + explicit defaults in top-level beforeEach; prevents mockReturnValue leaking across describe blocks - m1: hoist triple `orgConfig.domainMatching && !domainIsPublic` into const isCorporateDomain; used at all 3 call sites, behavior identical - m2: reword misleading e2e comment on suggestedOrganization:null to state actual controller contract and reference A2b for wiring * fix(organizations): normalize domain on write + exact-match read via canonical normalizeEmailDomain replace extractDomain(user.email) with normalizeEmailDomain (A1) at the two signup sites in handleSignupOrganization: the repository list query (read) and the domain passed to createOrganizationForUser (write). both now use the single canonical lowercased/trimmed/null-safe path from organizations.domain.js. guard isCorporateDomain against null domain (malformed email → normalizeEmailDomain returns null → treated as non-corporate, falls through to personal workspace). extractDomain kept as exported public API for non-signup callers; not modified. tests (A3): mixed-case write normalization, case-insensitive match, subdomain non-match (eu.acme.com ≠ acme.com), public-domain gate with mixed-case email. * refactor(organizations): drop dead extractDomain + simplify public-domain guard Remove extractDomain (def + JSDoc + export entry) — fully superseded by normalizeEmailDomain since A3; 0 callers in repo. Drop stale "kept for non-signup callers" comment (was factually wrong). Simplify domainIsPublic to `isPublicDomain(domain) || publicDomains.includes(domain ?? '')` — normalizeEmailDomain guarantees a lowercased string or null, so the redundant ?. chain is gone. Fix stale RED label in A3 test to describe the canonical-normalization contract it actually asserts. * fix(organizations): idempotent retry-safe signup provisioning (spec C1 / A4) Add convergence guard in handleSignupOrganization: if the user already holds an active membership (partial-failure retry scenario), return the existing org+membership without calling createOrganizationForUser — preventing duplicate workspaces and double-crediting signupGrant on retried signups. Uses MembershipRepository.findOne({ userId, status: ACTIVE }) — same canonical read as autoSetCurrentOrganization. Guard fires after the email-verification early-return and before Case 1/Case 2 always-create paths. signupGrant stays once-only because it lives inside createOrganizationForUser, which is skipped. * refactor(organizations): dedupe signup result builder + lock email-verif ordering test * feat(auth): forward suggestedJoin through signup HTTP response (A2b) Controller now serializes `suggestedJoin: { orgId, orgName } | null` from orgResult into the signup response body alongside the existing keys. `suggestedOrganization` kept as always-null with deprecation comment (additive — breaking removal deferred to next release). TDD: two integration tests (shape assert + null path), auth 1579 / orgs 1469 green. * fix(organizations): address CodeRabbit + Copilot review findings - domain: reject multiple-@ malformed emails in normalizeEmailDomain - service: JSDoc for buildResult @param/@returns (CR+Copilot) - service: update @returns to reflect null org/membership on mailer path - e2e: add explicit suggestedJoin assertion on domain-match signup path - e2e: fix stale A2b comment (now shipped in this PR) - unit: remove stale "RED" test-driven header comment - unit: cover multiple-@ rejection in domain tests
1 parent 77acdad commit 5e166d7

9 files changed

Lines changed: 842 additions & 160 deletions

modules/auth/controllers/auth.controller.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,9 @@ const signup = async (req, res) => {
144144
abilities: orgResult.abilities || [],
145145
organizationSetupRequired: orgResult.organizationSetupRequired || false,
146146
emailVerificationRequired: orgResult.emailVerificationRequired || false,
147+
// deprecated: always null since always-create (A2); superseded by suggestedJoin — remove next release
147148
suggestedOrganization: orgResult.suggestedOrganization || null,
149+
suggestedJoin: orgResult.suggestedJoin || null,
148150
type: 'success',
149151
message: 'Sign up',
150152
});

modules/auth/tests/auth.e2e.tests.js

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,9 @@ describe('Auth E2E tests:', () => {
118118
secondUser = null;
119119
});
120120

121-
test('should create pending join request when email domain matches existing org', async () => {
121+
test('spec D5: second user with same domain gets own workspace + suggestedJoin (no pending join request)', async () => {
122+
// Spec D5 always-create: domain-match no longer creates a pending join request.
123+
// Both users get their own active workspace.
122124
config.organizations = { enabled: true, autoCreate: true, domainMatching: true };
123125

124126
try {
@@ -139,7 +141,7 @@ describe('Auth E2E tests:', () => {
139141
expect(firstOrg).toBeDefined();
140142
expect(firstOrg.domain).toBe('e2etest.com');
141143

142-
// Step 2: signup second user with same domain
144+
// Step 2: signup second user with same domain — gets own workspace (spec D5)
143145
const result2 = await agent
144146
.post('/api/auth/signup')
145147
.send({
@@ -153,26 +155,29 @@ describe('Auth E2E tests:', () => {
153155

154156
secondUser = result2.body.user;
155157

156-
// Verify org is returned with pending flag
158+
// Second user gets their OWN active workspace (not a join on firstOrg)
157159
expect(result2.body.organization).toBeDefined();
158-
expect(result2.body.organization._id).toBe(firstOrg._id);
159-
expect(result2.body.pendingJoin).toBe(true);
160+
expect(result2.body.organization).not.toBeNull();
161+
expect(result2.body.organization._id).not.toBe(firstOrg._id);
162+
// pendingJoin is always false (spec D5)
163+
expect(result2.body.pendingJoin).toBeFalsy();
160164

161-
// Verify second user has a PENDING membership (not active)
162-
const pendingMemberships = await MembershipRepository.list({
165+
// Second user has an ACTIVE membership on their new org (not pending on firstOrg)
166+
const activeMemberships = await MembershipRepository.list({
163167
userId: secondUser.id,
164-
organizationId: firstOrg._id,
165-
status: 'pending',
168+
organizationId: result2.body.organization._id,
169+
status: 'active',
166170
});
167-
expect(pendingMemberships).toHaveLength(1);
171+
expect(activeMemberships).toHaveLength(1);
172+
expect(activeMemberships[0].role).toBe('owner');
168173

169-
// Verify NO active membership
170-
const activeMemberships = await MembershipRepository.list({
174+
// NO pending membership on firstOrg (spec D5: no join request created)
175+
const pendingOnFirst = await MembershipRepository.list({
171176
userId: secondUser.id,
172177
organizationId: firstOrg._id,
173-
status: 'active',
178+
status: 'pending',
174179
});
175-
expect(activeMemberships).toHaveLength(0);
180+
expect(pendingOnFirst).toHaveLength(0);
176181
} catch (err) {
177182
console.log(err);
178183
expect(err).toBeFalsy();

modules/auth/tests/auth.signup.organization.integration.tests.js

Lines changed: 42 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -116,13 +116,13 @@ describe('Auth signup organization integration tests:', () => {
116116
});
117117

118118
describe('Signup with organizations enabled + autoCreate + domainMatching', () => {
119-
test('should create pending join request when domain matches existing org', async () => {
119+
test('spec D5: second user with same domain gets own workspace + suggestedJoin hint (no pending request)', async () => {
120+
// Spec D5 always-create: domain-match no longer creates a pending join request.
121+
// Both users get their own active workspace; the second receives a suggestedJoin hint.
120122
config.organizations = { enabled: true, autoCreate: true, domainMatching: true };
121123

122-
// First, create an existing organization with a specific domain
123124
let firstUser;
124125
let secondUser;
125-
// clean up stale users from previous runs on shared databases
126126
await purgeUser('first@matchdomain.com');
127127
await purgeUser('second@matchdomain.com');
128128
try {
@@ -143,7 +143,7 @@ describe('Auth signup organization integration tests:', () => {
143143
expect(firstOrg).toBeDefined();
144144
expect(firstOrg.domain).toBe('matchdomain.com');
145145

146-
// Sign up second user with same domain — should get pending join request
146+
// Sign up second user with same domain — spec D5: gets own workspace, not a join request
147147
const result2 = await agent
148148
.post('/api/auth/signup')
149149
.send({
@@ -157,15 +157,29 @@ describe('Auth signup organization integration tests:', () => {
157157

158158
secondUser = result2.body.user;
159159

160-
// Should reference the same org but with pending flag
160+
// Second user gets their OWN active workspace, not a reference to firstOrg
161161
expect(result2.body.organization).toBeDefined();
162-
expect(result2.body.organization._id).toBe(firstOrg._id);
163-
expect(result2.body.pendingJoin).toBe(true);
164-
expect(result2.body.organizationSetupRequired).toBe(false);
162+
expect(result2.body.organization).not.toBeNull();
163+
expect(result2.body.organization._id).not.toBe(firstOrg._id);
164+
expect(result2.body.pendingJoin).toBeFalsy();
165+
expect(result2.body.organizationSetupRequired).toBeFalsy();
165166

166-
// Abilities should be present (without org context)
167+
// Abilities should be present (with org context — user is owner of their own org)
167168
expect(result2.body.abilities).toBeDefined();
168169
expect(result2.body.abilities).toBeInstanceOf(Array);
170+
171+
// A2b: suggestedJoin must be forwarded from the service through the controller response.
172+
// Shape is name-only { orgId: string, orgName: string } — no extra keys (domain, size, etc.)
173+
expect(result2.body.suggestedJoin).toBeDefined();
174+
expect(result2.body.suggestedJoin).not.toBeNull();
175+
expect(typeof result2.body.suggestedJoin.orgId).toBe('string');
176+
expect(typeof result2.body.suggestedJoin.orgName).toBe('string');
177+
expect(result2.body.suggestedJoin.orgId).toBe(String(firstOrg._id));
178+
// name-only: no leaked fields
179+
expect(result2.body.suggestedJoin).not.toHaveProperty('domain');
180+
expect(result2.body.suggestedJoin).not.toHaveProperty('memberCount');
181+
expect(result2.body.suggestedJoin).not.toHaveProperty('membership');
182+
expect(result2.body.suggestedJoin).not.toHaveProperty('plan');
169183
} catch (err) {
170184
console.log(err);
171185
expect(err).toBeFalsy();
@@ -215,11 +229,10 @@ describe('Auth signup organization integration tests:', () => {
215229
});
216230

217231
describe('Signup with organizations enabled + autoCreate + no domainMatching', () => {
218-
test('should create a personal organization for the user', async () => {
232+
test('should create a personal organization for the user (domainMatching off = personal name)', async () => {
219233
config.organizations = { enabled: true, autoCreate: true, domainMatching: false };
220234

221235
let user;
222-
// clean up stale users from previous runs on shared databases
223236
await purgeUser('personal@somecompany.com');
224237
try {
225238
const result = await agent
@@ -235,12 +248,12 @@ describe('Auth signup organization integration tests:', () => {
235248

236249
user = result.body.user;
237250

238-
// A personal organization should be created (no domain matching)
251+
// domainMatching off → personal workspace name regardless of domain
239252
expect(result.body.organization).toBeDefined();
240253
expect(result.body.organization).not.toBeNull();
241254
expect(result.body.organization.name).toBe("Personal's organization");
242255
expect(result.body.organization.domain).toBe('');
243-
expect(result.body.organizationSetupRequired).toBe(false);
256+
expect(result.body.organizationSetupRequired).toBeFalsy();
244257

245258
// Abilities should be present
246259
expect(result.body.abilities).toBeDefined();
@@ -254,12 +267,13 @@ describe('Auth signup organization integration tests:', () => {
254267
});
255268
});
256269

257-
describe('Signup with organizations enabled + no autoCreate', () => {
258-
test('should not create an organization and flag setup as required', async () => {
270+
describe('Signup with organizations enabled + no autoCreate (spec D5: autoCreate is deprecated no-op)', () => {
271+
test('spec D5: autoCreate:false is a no-op — workspace always provisioned', async () => {
272+
// Spec D5: config.organizations.autoCreate is a deprecated no-op.
273+
// Signup ALWAYS provisions a workspace; organizationSetupRequired is never returned.
259274
config.organizations = { enabled: true, autoCreate: false, domainMatching: true };
260275

261276
let user;
262-
// clean up stale users from previous runs on shared databases
263277
await purgeUser('manual@setup.com');
264278
try {
265279
const result = await agent
@@ -275,11 +289,13 @@ describe('Auth signup organization integration tests:', () => {
275289

276290
user = result.body.user;
277291

278-
// No organization should be created
279-
expect(result.body.organization).toBeNull();
280-
expect(result.body.organizationSetupRequired).toBe(true);
292+
// Workspace is always created now (spec D5)
293+
expect(result.body.organization).not.toBeNull();
294+
expect(result.body.organization).toBeDefined();
295+
// organizationSetupRequired defaults to false (auth controller || false)
296+
expect(result.body.organizationSetupRequired).toBeFalsy();
281297

282-
// Abilities should still be present (guest-level for org context)
298+
// Abilities should be present (user is owner of their new workspace)
283299
expect(result.body.abilities).toBeDefined();
284300
expect(result.body.abilities).toBeInstanceOf(Array);
285301
} catch (err) {
@@ -312,14 +328,18 @@ describe('Auth signup organization integration tests:', () => {
312328

313329
user = result.body.user;
314330

315-
// Verify response structure includes the new fields
331+
// Verify response structure includes the expected fields
316332
expect(result.body).toHaveProperty('user');
317333
expect(result.body).toHaveProperty('organization');
318334
expect(result.body).toHaveProperty('abilities');
319-
expect(result.body).toHaveProperty('organizationSetupRequired');
335+
// organizationSetupRequired is always false (auth controller || false default)
336+
expect(result.body.organizationSetupRequired).toBeFalsy();
320337
expect(result.body).toHaveProperty('tokenExpiresIn');
321338
expect(result.body.type).toBe('success');
322339
expect(result.body.message).toBe('Sign up');
340+
// A2b: no domain match / orgs disabled → suggestedJoin is present-as-null (not absent)
341+
expect(result.body).toHaveProperty('suggestedJoin');
342+
expect(result.body.suggestedJoin).toBeNull();
323343
} catch (err) {
324344
console.log(err);
325345
expect(err).toBeFalsy();
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/**
2+
* Email-domain normalization helpers.
3+
*
4+
* Maintained constant. Not auto-refreshed (spec H2 — freshness out of scope).
5+
*/
6+
7+
export const PUBLIC_DOMAINS = new Set([
8+
'gmail.com', 'googlemail.com', 'outlook.com', 'hotmail.com', 'live.com',
9+
'yahoo.com', 'icloud.com', 'me.com', 'proton.me', 'protonmail.com',
10+
'aol.com', 'gmx.com', 'mail.com', 'yandex.com', 'zoho.com',
11+
]);
12+
13+
/**
14+
* Extract and normalize the domain portion of an email address.
15+
* @param {*} email - Raw value to parse.
16+
* @returns {string|null} Lowercased, trimmed domain, or null on malformed input.
17+
*/
18+
export function normalizeEmailDomain(email) {
19+
if (typeof email !== 'string') return null;
20+
const at = email.indexOf('@');
21+
if (at === -1 || at !== email.lastIndexOf('@')) return null;
22+
const domain = email.slice(at + 1).trim().toLowerCase();
23+
return domain.length > 0 && domain.includes('.') ? domain : null;
24+
}
25+
26+
/**
27+
* Return true when the domain belongs to a known public e-mail provider.
28+
* @param {string} domain - Normalized or raw domain string.
29+
* @returns {boolean}
30+
*/
31+
export function isPublicDomain(domain) {
32+
return PUBLIC_DOMAINS.has(String(domain ?? '').trim().toLowerCase());
33+
}

0 commit comments

Comments
 (0)