feat(organizations): require email verification before domain-based org features#3234
Merged
PierreBrisorgueil merged 6 commits intomasterfrom Mar 15, 2026
Merged
Conversation
…rg features When mailer is configured, users must verify their email before: - Organization auto-provisioning during signup - Explicit organization creation - Join request creation - Domain-based organization search Without mailer configured, the flow remains unchanged (dev-friendly). Closes #3232
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThe PR introduces email verification requirements for domain-based organization operations to prevent domain squatting. It centralizes base URL resolution via a new helper module, replaces inline Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds email-verification gating (when the mailer is configured) to prevent unverified accounts from using domain-based organization behaviors, addressing the domain-squatting risk described in #3232.
Changes:
- Added a shared
assertEmailVerifiedhelper and applied it to org creation and join-request flows. - Blocked signup-time org provisioning and domain-based org search for unverified users when mailer is configured.
- Added unit tests covering the new gating behavior across signup provisioning, org creation, join requests, and search.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/organizations/tests/organizations.emailVerification.unit.tests.js | Adds unit coverage for email-verification gates across org flows. |
| modules/organizations/services/organizations.service.js | Skips signup org provisioning when mailer is configured and user email isn’t verified. |
| modules/organizations/services/organizations.membership.service.js | Enforces email verification before creating join requests. |
| modules/organizations/services/organizations.crud.service.js | Enforces email verification before explicit org creation. |
| modules/organizations/controllers/organizations.controller.js | Returns empty search results for unverified users when mailer is configured. |
| lib/helpers/emailVerification.js | Introduces shared assertEmailVerified helper for mailer-configured environments. |
…flag, update JSDoc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…lVerified false - Extract getBaseUrl() into shared lib/helpers/getBaseUrl.js - Replace all config.cors.origin[0] and config.app.front with getBaseUrl() - Remove unused app.front config - Fix backslash in verify-email template - Fix displayName trailing space when lastName is empty - Keep emailVerified false at signup even without mailer (gates enforce only when mailer configured) - Update consecutive_zero threshold to 3 in pull-request skill
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/auth/controllers/auth.controller.js (1)
58-63: 🛠️ Refactor suggestion | 🟠 MajorAdd
@returnsto the modified async handlers.
signupandoauthCallbackwere both changed in this PR, but their JSDoc blocks still omit the resolved return type required by the repo rules.📝 Minimal JSDoc fix
/** * `@desc` Endpoint to ask the service to create a user * `@param` {Object} req - Express request object * `@param` {Object} res - Express response object + * `@returns` {Promise<void>} Resolves after sending the signup response */ const signup = async (req, res) => {/** * `@desc` Endpoint for oautCallCallBack * `@param` {Object} req - Express request object * `@param` {Object} res - Express response object * `@param` {Function} next - Express next middleware function + * `@returns` {Promise<void>} Resolves after redirecting or sending the OAuth response */ const oauthCallback = async (req, res, next) => {As per coding guidelines,
**/*.js: Every new or modified function must have a JSDoc header: one-line description,@paramfor each argument,@returnsfor any non-void return value (always include@returnsfor async functions to document the resolved value).Also applies to: 315-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/auth/controllers/auth.controller.js` around lines 58 - 63, The JSDoc for the modified async handlers is missing `@returns`; update the comment blocks for the async functions signup and oauthCallback to include a `@returns` that documents the resolved return value (e.g., Express response or Promise<void> / Promise<Express.Response> as appropriate), keeping the existing one-line description and `@param` entries and ensuring the async resolved type is clearly specified for each handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/helpers/emailVerification.js`:
- Around line 10-12: The function assertEmailVerified currently dereferences
user.emailVerified without checking user; update assertEmailVerified to first
guard that user is non-null/undefined (e.g., if (!user || !user.emailVerified)
or separate checks) before accessing user.emailVerified, and when the guard
fails throw the same AppError used currently (with code 'FORBIDDEN' and status
403) so callers receive a controlled error; reference mailer.isConfigured(),
assertEmailVerified, user.emailVerified, and AppError when making the change.
- Around line 4-9: The JSDoc for the new helper is missing a `@returns` tag:
update the JSDoc for the helper function (assertEmailVerified) to include
"@returns {void}" so the function's return shape is explicitly documented; keep
the existing `@param` and description intact and place the `@returns` line with the
other tags in the comment block above the assertEmailVerified function.
In `@lib/helpers/getBaseUrl.js`:
- Around line 11-15: getBaseUrl currently returns origin values as-is which can
include trailing slashes causing double-slash URLs; update the getBaseUrl
function to trim whitespace and remove any trailing slash(es) from the resolved
origin (both when origin is an array element and when it's a string) before
returning it (e.g. normalize origin or origin[0] via a trailing-slash strip
using a regex or string method); ensure empty or undefined origin still returns
an empty string.
In `@modules/auth/controllers/auth.password.controller.js`:
- Around line 50-52: Add missing `@returns` JSDoc tags to the modified forgot and
reset handler headers in auth.password.controller.js: update the JSDoc for the
forgot function and the reset function to include a `@returns` line that documents
the return value and its type (for example a Promise resolving to the Express
response or void as appropriate) and a short description of what the handler
returns (e.g., Promise<void> or Promise<Response> / JSON result). Ensure both
handlers' JSDoc blocks include `@param` entries already present and now also an
explicit `@returns` description matching the actual returned value/type in the
forgot and reset functions.
In `@modules/organizations/services/organizations.membership.service.js`:
- Around line 136-138: The code calls UserService.getBrut({ id: String(userId)
}) and then immediately assertEmailVerified(user) without checking that user is
not null; add an existence check after UserService.getBrut (e.g., if (!user)
throw a NotFound/BadRequest error or return a clear failure) before calling
assertEmailVerified and before any pending membership creation logic so you
never pass a null user into assertEmailVerified or createPendingMembership;
update the flow around UserService.getBrut and assertEmailVerified to handle the
missing-user case explicitly.
In `@modules/organizations/services/organizations.service.js`:
- Around line 133-149: The early return in handleSignupOrganization omits the
suggestedOrganization key, violating the documented return shape; update the
mailer verification branch return to include suggestedOrganization: null
(alongside organization, membership, abilities, organizationSetupRequired,
emailVerificationRequired, pendingJoin) so consumers receive a consistent object
shape.
In `@modules/organizations/tests/organizations.emailVerification.unit.tests.js`:
- Around line 77-79: Update the tests to assert call flow rather than only HTTP
200: where the controller uses OrganizationsCrudService.searchByDomain(),
explicitly expect the mock to have been called (or not) depending on the gate
branch; add assertions for the mailer-enforced path to verify that the allow
branch calls OrganizationsCrudService.searchByDomain() and the blocked branch
does not call it. Also add a new test case exercising the configured+verified
scenario so the "allow" path under mailer enforcement is covered, and ensure you
reset mocks in beforeEach (jest.clearAllMocks()) so call counts are reliable.
In `@modules/users/controllers/users.data.controller.js`:
- Line 69: The JSDoc for the getMail function is missing a `@returns` tag; update
the JSDoc block above the getMail function (function name: getMail) to include a
`@returns` annotation that describes the returned value (e.g., the mail object or
response type), matching the existing `@param` style and project conventions so
every function header documents both `@param` and `@returns`.
---
Outside diff comments:
In `@modules/auth/controllers/auth.controller.js`:
- Around line 58-63: The JSDoc for the modified async handlers is missing
`@returns`; update the comment blocks for the async functions signup and
oauthCallback to include a `@returns` that documents the resolved return value
(e.g., Express response or Promise<void> / Promise<Express.Response> as
appropriate), keeping the existing one-line description and `@param` entries and
ensuring the async resolved type is clearly specified for each handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b1cbb30-e2cf-4708-82e2-8f5969052962
📒 Files selected for processing (12)
config/defaults/development.config.jsconfig/templates/verify-email.htmllib/helpers/emailVerification.jslib/helpers/getBaseUrl.jsmodules/auth/controllers/auth.controller.jsmodules/auth/controllers/auth.password.controller.jsmodules/organizations/controllers/organizations.controller.jsmodules/organizations/services/organizations.crud.service.jsmodules/organizations/services/organizations.membership.service.jsmodules/organizations/services/organizations.service.jsmodules/organizations/tests/organizations.emailVerification.unit.tests.jsmodules/users/controllers/users.data.controller.js
💤 Files with no reviewable changes (1)
- config/defaults/development.config.js
modules/organizations/tests/organizations.emailVerification.unit.tests.js
Show resolved
Hide resolved
Contributor
Author
|
re-triggering CI |
PierreBrisorgueil
force-pushed
the
feat/email-verification-gate
branch
from
37a0493 to
e15a1d6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
emailVerifiedwhen mailer is configuredassertEmailVerifiedhelper to avoid duplicationCloses #3232
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests