Skip to content

Commit 4b970e6

Browse files
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 7132647 commit 4b970e6

5 files changed

Lines changed: 22 additions & 6 deletions

File tree

modules/organizations/services/organizations.domain.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ export const PUBLIC_DOMAINS = new Set([
1818
export function normalizeEmailDomain(email) {
1919
if (typeof email !== 'string') return null;
2020
const at = email.indexOf('@');
21-
if (at === -1) return null;
21+
if (at === -1 || at !== email.lastIndexOf('@')) return null;
2222
const domain = email.slice(at + 1).trim().toLowerCase();
2323
return domain.length > 0 && domain.includes('.') ? domain : null;
2424
}

modules/organizations/services/organizations.service.js

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,8 +127,9 @@ const createOrganizationForUser = async ({ name, slug, domain, user, slugGenerat
127127
* exactly once per real new org — no double-credit possible.
128128
*
129129
* @param {Object} user - The user object returned by UserService.create (with id, email, firstName, lastName).
130-
* @returns {Promise<{organization: Object, membership: Object, abilities: Array, emailVerificationRequired?: boolean, suggestedJoin?: {orgId: string, orgName: string}}>}
130+
* @returns {Promise<{organization: Object|null, membership: Object|null, abilities: Array, emailVerificationRequired?: boolean, suggestedJoin?: {orgId: string, orgName: string}}>}
131131
* An object containing the organization context for the signup response.
132+
* `organization` and `membership` are null when `emailVerificationRequired` is true (mailer path).
132133
* NEVER returns `organizationSetupRequired` or `pendingJoin`.
133134
*/
134135
const handleSignupOrganization = async (user) => {
@@ -155,6 +156,12 @@ const handleSignupOrganization = async (user) => {
155156
// MembershipRepository.defaultPopulate includes organizationId, so membership.organizationId
156157
// is the full org document after the query.
157158
// Shared result builder — all three signup exit-paths return the same {organization,membership,abilities} shape.
159+
/**
160+
* Build the signup organization payload with serialized abilities.
161+
* @param {Object} organization - Organization document.
162+
* @param {Object} membership - Membership document.
163+
* @returns {Promise<{organization: Object, membership: Object, abilities: Array}>}
164+
*/
158165
const buildResult = async (organization, membership) => ({
159166
organization,
160167
membership,

modules/organizations/tests/organizations.domain.unit.tests.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ describe('organizations.domain', () => {
1515
expect(normalizeEmailDomain('')).toBeNull();
1616
expect(normalizeEmailDomain(null)).toBeNull();
1717
});
18+
it('returns null for multiple-@ addresses', () => {
19+
expect(normalizeEmailDomain('a@b@acme.com')).toBeNull();
20+
});
1821
});
1922
describe('isPublicDomain', () => {
2023
it('flags common public providers', () => {

modules/organizations/tests/organizations.domainJoin.e2e.tests.js

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -152,10 +152,16 @@ describe('Organizations domain join E2E tests:', () => {
152152
expect(activeMemberships).toHaveLength(1);
153153
expect(activeMemberships[0].role).toBe('owner');
154154

155-
// suggestedJoin hint returned (name-only) pointing to owner's org
156-
// controller still emits suggestedOrganization (always null post-footgun-removal);
157-
// suggestedJoin is wired to the response in a follow-up (A2b).
155+
// suggestedJoin hint (name-only shape) pointing to owner's org — wired in A2b
156+
// controller still emits suggestedOrganization (always null post-footgun-removal)
158157
expect(result.body.suggestedOrganization).toBeNull();
158+
expect(result.body.suggestedJoin).toBeDefined();
159+
expect(result.body.suggestedJoin).toEqual(
160+
expect.objectContaining({
161+
orgId: String(org._id),
162+
orgName: org.name,
163+
}),
164+
);
159165
} catch (err) {
160166
console.log(err);
161167
expect(err).toBeFalsy();

modules/organizations/tests/organizations.service.signup.unit.tests.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
* - signupGrant credited exactly once per real new org creation (via BillingSignupGrantService.grantOnSignup);
1111
* not double-credited on any path.
1212
*
13-
* RED: written before A2 implementation — current code fails these assertions.
13+
* All assertions pass with the A2 implementation shipped in this PR.
1414
*/
1515
import mongoose from 'mongoose';
1616
import { jest, describe, test, expect, beforeEach } from '@jest/globals';

0 commit comments

Comments
 (0)