feat email verification flow#382
Open
saurabhhhcodes wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an end-to-end email verification flow: a new ChangesEmail Verification Flow
Sequence Diagram(s)sequenceDiagram
actor User
participant register.html
participant api.js
participant AuthViewSet
participant EmailProvider
participant email-verification.html
participant verify_email_endpoint
User->>register.html: Submit registration form
register.html->>api.js: register(email, password, ...)
api.js->>AuthViewSet: POST /api/v1/auth/register/
AuthViewSet->>AuthViewSet: Create User(is_email_verified=False)
AuthViewSet->>EmailProvider: send_mail(verification_url with signed token)
AuthViewSet-->>api.js: 201 {message, verification_url}
api.js-->>register.html: response JSON
register.html->>email-verification.html: redirect ?email=<encoded>
User->>email-verification.html: Clicks link from email (?token=<signed>)
email-verification.html->>api.js: verifyEmail(token)
api.js->>verify_email_endpoint: GET /api/v1/auth/verify-email/<token>/
alt valid token
verify_email_endpoint->>verify_email_endpoint: user.is_email_verified = True
verify_email_endpoint-->>api.js: 200 {detail, user}
api.js-->>email-verification.html: success result
email-verification.html-->>User: Show success alert + login link
else invalid/expired token
verify_email_endpoint-->>api.js: 400 {detail}
api.js-->>email-verification.html: throws error
email-verification.html-->>User: Show failure alert + register link
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
backend/users/tests.py (1)
47-54: ⚡ Quick winAssert the verification token is not returned in registration JSON.
This test guards “no JWTs,” but the stronger ownership-proof invariant is that the verification URL/token appears only in the email body. Add an assertion so the regression in the registration response cannot come back.
🧪 Proposed test assertion
self.assertIn('message', response.data) self.assertNotIn('access', response.data) self.assertNotIn('refresh', response.data) + self.assertNotIn('verification_url', response.data) user = User.objects.get(email='new@example.com')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/tests.py` around lines 47 - 54, Add an assertion to verify that the verification token or URL is not exposed in the registration response JSON. After checking that the token appears in the email body via mock_send_mail.call_args, add an assertNotIn statement to ensure the token or verification URL pattern (such as the email-verification token string) does not appear in response.data, similar to how the test already checks that 'access' and 'refresh' tokens are not in the response data.backend/users/serializers.py (1)
14-14: Makeis_email_verifiedread-only inUserSerializer.This field is security-sensitive and should never be writable at the serializer level. While
UserSerializeris currently only used for responses, marking it read-only prevents accidental future misuse and hardens the trust boundary around email verification state.🔒 Proposed serializer hardening
class UserSerializer(serializers.ModelSerializer): organization = OrganizationSerializer(read_only=True) class Meta: model = User - fields = ['id', 'email', 'role', 'organization', 'is_active', 'is_email_verified'] + fields = ('id', 'email', 'role', 'organization', 'is_active', 'is_email_verified') + read_only_fields = ('is_email_verified',)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/serializers.py` at line 14, In the `UserSerializer` class, the `is_email_verified` field is listed in the fields tuple but is not marked as read-only, which could allow it to be writable. Add `is_email_verified` to a `read_only_fields` tuple within the Meta class of `UserSerializer` to ensure this security-sensitive field cannot be modified through the serializer, preventing accidental future misuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/users/authentication.py`:
- Around line 8-9: The authentication check in the conditional at line 8-9 that
validates `is_email_verified` will reject all existing users since the new field
defaults to False and was not backfilled. Create a Django data migration that
updates all existing User records to set `is_email_verified = True` before
deploying this authentication guard. This ensures that existing users are not
locked out when the email verification check is enabled globally. The migration
should run before the authentication code is deployed to production, or
alternatively disable this check temporarily until the migration and any email
verification flow are ready.
In `@backend/users/migrations/0002_user_is_email_verified.py`:
- Around line 11-15: The AddField migration in the migrations file adds
is_email_verified with default=False, which will mark all existing users as
unverified and lock them out of authentication. Add a data migration function to
backfill all existing User records by setting is_email_verified=True for
pre-existing accounts before the AddField operation takes effect. This ensures
current authenticated users retain access when EmailVerifiedJWTAuthentication is
enforced.
In `@backend/users/views.py`:
- Around line 41-45: The send_mail() call failure is being caught and logged but
then the function continues to return verification_url as if the email was
successfully sent. Instead of swallowing the exception, you need to handle email
delivery failures explicitly by either rolling back the user registration,
raising an exception to return a failure response to the client, or queuing the
email for asynchronous retry. Additionally, remove the user.email parameter from
the logger.exception() call to avoid logging sensitive information in the
failure path.
- Around line 73-76: The verify_email endpoint in the UserViewSet currently
exposes sensitive user data via the UserSerializer in the response, which is a
security concern since this is an AllowAny endpoint with only a bearer secret
for protection. Remove the 'user': UserSerializer(user).data line from the
Response in verify_email and return only the 'detail' message confirming
successful verification, as the client only needs a success or failure status
from this public endpoint.
- Around line 53-58: Remove the verification_url from the Response dictionary in
the registration endpoint. The send_verification_email function should continue
to be called to deliver the link via email, but the Response object should only
include the user data and success message without exposing the verification_url,
as returning the signed token in the API response defeats the purpose of
email-based verification and creates a security risk where users could verify
email addresses they do not own.
In `@frontend/api.js`:
- Around line 149-150: The registration response is exposing the
verification_url field which contains a signed verification token, allowing
users to verify their account without accessing their email. Although this code
in frontend/api.js at line 149 is just returning the backend response as-is, the
actual fix must be implemented on the backend server: modify the registration
endpoint to exclude the verification_url field from public registration
responses, or alternatively gate this field to non-production test-only paths
only. Do not return the verification_url in production registration API
responses.
- Around line 148-149: The registration error handler at the if (!res.ok) check
throws a generic "Registration failed" error message instead of propagating the
actual backend validation errors. Instead of throwing a fixed error message,
parse the response body to extract field-level validation errors returned by the
backend, and throw an error object that includes these details so that
register.html can display specific validation feedback for each field. This
requires awaiting the response JSON before throwing the error to include the
actual error information in the thrown error.
In `@frontend/register.html`:
- Line 195: The email address is being exposed as PII in the URL query parameter
on the window.location.href assignment. Instead of appending the email to the
query string, store the email in sessionStorage before navigation, then redirect
to the email-verification page without the email parameter. Update the
email-verification page to retrieve the email from sessionStorage rather than
parsing it from the URL query parameters. This prevents the sensitive
information from being exposed in browser history, logs, and screenshots.
---
Nitpick comments:
In `@backend/users/serializers.py`:
- Line 14: In the `UserSerializer` class, the `is_email_verified` field is
listed in the fields tuple but is not marked as read-only, which could allow it
to be writable. Add `is_email_verified` to a `read_only_fields` tuple within the
Meta class of `UserSerializer` to ensure this security-sensitive field cannot be
modified through the serializer, preventing accidental future misuse.
In `@backend/users/tests.py`:
- Around line 47-54: Add an assertion to verify that the verification token or
URL is not exposed in the registration response JSON. After checking that the
token appears in the email body via mock_send_mail.call_args, add an assertNotIn
statement to ensure the token or verification URL pattern (such as the
email-verification token string) does not appear in response.data, similar to
how the test already checks that 'access' and 'refresh' tokens are not in the
response data.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86a475d5-45b4-4eb1-a715-e8556270f9fe
📒 Files selected for processing (14)
backend/backend/settings.pybackend/campaigns/migrations/0010_merge_0009_campaign_cached_counters_0009_campaignlead_bounce_metadata.pybackend/users/authentication.pybackend/users/jwt.pybackend/users/migrations/0002_user_is_email_verified.pybackend/users/migrations/0003_merge_0002_user_is_email_verified_0002_user_member_role_default.pybackend/users/models.pybackend/users/permissions.pybackend/users/serializers.pybackend/users/tests.pybackend/users/views.pyfrontend/api.jsfrontend/email-verification.htmlfrontend/register.html
Comment on lines
+8
to
+9
| if not getattr(user, 'is_email_verified', False): | ||
| raise AuthenticationFailed('Please verify your email before accessing the API.') |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect user migrations for an is_email_verified backfill for existing rows.
fd '\.py$' backend/users/migrations -x sh -c 'echo "### $1"; sed -n "1,220p" "$1"' sh {}Repository: Kuldeeep18/LeadOrbit
Length of output: 4952
Backfill existing users before enforcing email verification.
With is_email_verified defaulting to False and this authentication class configured globally, existing users will be rejected from JWT-protected API endpoints. The migration that adds is_email_verified does not backfill existing rows—it only applies the default to newly created users. Add a data migration to mark existing users as verified, or implement and ship a resend/verification flow before enabling this guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/users/authentication.py` around lines 8 - 9, The authentication check
in the conditional at line 8-9 that validates `is_email_verified` will reject
all existing users since the new field defaults to False and was not backfilled.
Create a Django data migration that updates all existing User records to set
`is_email_verified = True` before deploying this authentication guard. This
ensures that existing users are not locked out when the email verification check
is enabled globally. The migration should run before the authentication code is
deployed to production, or alternatively disable this check temporarily until
the migration and any email verification flow are ready.
Comment on lines
+11
to
+15
| migrations.AddField( | ||
| model_name='user', | ||
| name='is_email_verified', | ||
| field=models.BooleanField(default=False), | ||
| ), |
Contributor
There was a problem hiding this comment.
Backfill existing users before enforcing email-verified auth.
Adding is_email_verified with default=False without a data backfill will mark pre-existing accounts unverified, and they will be denied auth once the default authentication class is switched to EmailVerifiedJWTAuthentication (backend/backend/settings.py Line 135).
💡 Suggested migration adjustment
from django.db import migrations, models
+def mark_existing_users_verified(apps, schema_editor):
+ User = apps.get_model('users', 'User')
+ User.objects.filter(is_email_verified=False).update(is_email_verified=True)
+
+
class Migration(migrations.Migration):
@@
operations = [
migrations.AddField(
model_name='user',
name='is_email_verified',
field=models.BooleanField(default=False),
),
+ migrations.RunPython(
+ mark_existing_users_verified,
+ migrations.RunPython.noop,
+ ),
]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/users/migrations/0002_user_is_email_verified.py` around lines 11 -
15, The AddField migration in the migrations file adds is_email_verified with
default=False, which will mark all existing users as unverified and lock them
out of authentication. Add a data migration function to backfill all existing
User records by setting is_email_verified=True for pre-existing accounts before
the AddField operation takes effect. This ensures current authenticated users
retain access when EmailVerifiedJWTAuthentication is enforced.
Comment on lines
+41
to
+45
| try: | ||
| send_mail(subject, message, getattr(settings, 'DEFAULT_FROM_EMAIL', 'noreply@leadorbit.local'), [user.email], fail_silently=False) | ||
| except Exception: | ||
| logger.exception('Failed to send verification email for user %s', user.email) | ||
| return verification_url |
Contributor
There was a problem hiding this comment.
Do not report success after email delivery fails.
send_mail() failures are logged and swallowed, so registration still returns 201 even when no verification email was delivered. Handle this with rollback, durable queuing plus retry/resend, or an explicit failure response; also avoid logging the raw email address in the failure path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/users/views.py` around lines 41 - 45, The send_mail() call failure is
being caught and logged but then the function continues to return
verification_url as if the email was successfully sent. Instead of swallowing
the exception, you need to handle email delivery failures explicitly by either
rolling back the user registration, raising an exception to return a failure
response to the client, or queuing the email for asynchronous retry.
Additionally, remove the user.email parameter from the logger.exception() call
to avoid logging sensitive information in the failure path.
Comment on lines
+53
to
58
| verification_url = send_verification_email(user) | ||
| return Response({ | ||
| 'user': UserSerializer(user).data, | ||
| 'refresh': str(refresh), | ||
| 'access': str(refresh.access_token), | ||
| 'message': 'Verification email sent. Please check your inbox to activate your account.', | ||
| 'verification_url': verification_url, | ||
| }, status=status.HTTP_201_CREATED) |
Contributor
There was a problem hiding this comment.
Do not return the verification URL from registration.
Returning verification_url gives the requester the signed token immediately, so they can verify an address they do not own without opening that mailbox. The verification link must be delivered only through email.
🔒 Keep the verification token email-only
- verification_url = send_verification_email(user)
+ send_verification_email(user)
return Response({
'user': UserSerializer(user).data,
'message': 'Verification email sent. Please check your inbox to activate your account.',
- 'verification_url': verification_url,
}, status=status.HTTP_201_CREATED)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/users/views.py` around lines 53 - 58, Remove the verification_url
from the Response dictionary in the registration endpoint. The
send_verification_email function should continue to be called to deliver the
link via email, but the Response object should only include the user data and
success message without exposing the verification_url, as returning the signed
token in the API response defeats the purpose of email-based verification and
creates a security risk where users could verify email addresses they do not
own.
Comment on lines
+73
to
+76
| return Response({ | ||
| 'detail': 'Email verified successfully.', | ||
| 'user': UserSerializer(user).data, | ||
| }) |
Contributor
There was a problem hiding this comment.
Return only verification status from this public endpoint.
verify_email is AllowAny, and the verification link is a bearer secret; anyone with the link can receive UserSerializer data. The client only needs success/failure here, so avoid exposing user and organization metadata.
🔒 Minimize the public verification response
return Response({
'detail': 'Email verified successfully.',
- 'user': UserSerializer(user).data,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return Response({ | |
| 'detail': 'Email verified successfully.', | |
| 'user': UserSerializer(user).data, | |
| }) | |
| return Response({ | |
| 'detail': 'Email verified successfully.', | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/users/views.py` around lines 73 - 76, The verify_email endpoint in
the UserViewSet currently exposes sensitive user data via the UserSerializer in
the response, which is a security concern since this is an AllowAny endpoint
with only a bearer secret for protection. Remove the 'user':
UserSerializer(user).data line from the Response in verify_email and return only
the 'detail' message confirming successful verification, as the client only
needs a success or failure status from this public endpoint.
Comment on lines
148
to
+149
| if (!res.ok) throw new Error("Registration failed"); | ||
| const data = await res.json(); | ||
| setTokens(data.access, data.refresh); | ||
| return await res.json(); |
Contributor
There was a problem hiding this comment.
Propagate backend validation errors during registration.
Line 148 throws a fixed message, so the updated register.html alert path cannot show field-level serializer errors from /auth/register/ responses.
Suggested patch
export const register = async (userData) => {
const res = await fetch(`${API_BASE}/auth/register/`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(userData)
});
- if (!res.ok) throw new Error("Registration failed");
- return await res.json();
+ const data = await res.json().catch(() => ({}));
+ if (!res.ok) {
+ const firstFieldError = Object.values(data)?.[0]?.[0];
+ throw new Error(data.detail || firstFieldError || 'Registration failed. Please try again.');
+ }
+ return data;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!res.ok) throw new Error("Registration failed"); | |
| const data = await res.json(); | |
| setTokens(data.access, data.refresh); | |
| return await res.json(); | |
| export const register = async (userData) => { | |
| const res = await fetch(`${API_BASE}/auth/register/`, { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify(userData) | |
| }); | |
| const data = await res.json().catch(() => ({})); | |
| if (!res.ok) { | |
| const firstFieldError = Object.values(data)?.[0]?.[0]; | |
| throw new Error(data.detail || firstFieldError || 'Registration failed. Please try again.'); | |
| } | |
| return data; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/api.js` around lines 148 - 149, The registration error handler at
the if (!res.ok) check throws a generic "Registration failed" error message
instead of propagating the actual backend validation errors. Instead of throwing
a fixed error message, parse the response body to extract field-level validation
errors returned by the backend, and throw an error object that includes these
details so that register.html can display specific validation feedback for each
field. This requires awaiting the response JSON before throwing the error to
include the actual error information in the thrown error.
Comment on lines
+149
to
+150
| return await res.json(); | ||
| }; |
Contributor
There was a problem hiding this comment.
Remove verification tokens from registration responses (email verification bypass).
Line 149 returns backend payload as-is, and the backend contract currently includes verification_url. That exposes the signed verification token to the same client that just registered, allowing account verification without inbox access. This defeats the control this PR is introducing.
Fix must be server-side: do not return verification_url on public registration responses (or gate it to non-production test-only paths).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/api.js` around lines 149 - 150, The registration response is
exposing the verification_url field which contains a signed verification token,
allowing users to verify their account without accessing their email. Although
this code in frontend/api.js at line 149 is just returning the backend response
as-is, the actual fix must be implemented on the backend server: modify the
registration endpoint to exclude the verification_url field from public
registration responses, or alternatively gate this field to non-production
test-only paths only. Do not return the verification_url in production
registration API responses.
| }); | ||
|
|
||
| window.location.href = '/dashboard.html'; | ||
| window.location.href = `/email-verification.html?email=${encodeURIComponent(email)}`; |
Contributor
There was a problem hiding this comment.
Avoid putting user email addresses in query parameters.
Line 195 appends the raw email into the URL. That creates unnecessary PII exposure via browser history, logs, and shared screenshots. Pass this through sessionStorage (or render a generic message without echoing the email) instead of query params.
Suggested patch
- window.location.href = `/email-verification.html?email=${encodeURIComponent(email)}`;
+ sessionStorage.setItem('pending_verification_email', email);
+ window.location.href = '/email-verification.html';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/register.html` at line 195, The email address is being exposed as
PII in the URL query parameter on the window.location.href assignment. Instead
of appending the email to the query string, store the email in sessionStorage
before navigation, then redirect to the email-verification page without the
email parameter. Update the email-verification page to retrieve the email from
sessionStorage rather than parsing it from the URL query parameters. This
prevents the sensitive information from being exposed in browser history, logs,
and screenshots.
Owner
|
Hi @saurabhhhcodes 👋 LeadOrbit Bot here 🤖 A repository star ⭐ is required for PR review and merge. We noticed that you haven't starred the repository yet. Please star the repo and reply once completed so we can continue with the review process. Thanks for contributing to LeadOrbit! 🚀 |
Owner
|
Hi @saurabhhhcodes 👋 LeadOrbit Bot here 🤖 We noticed you've opened a Pull Request but haven't starred the repository yet. ⭐ Starring the repository is mandatory for PR review and merge. Please:
Once you've done that, the bot will continue processing your PR. Note: PRs from contributors who haven't starred the repository will remain pending until this requirement is completed. Thanks for contributing to LeadOrbit! 🚀 |
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
2 participants
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.
What changed
is_email_verifiedto the user model and blocked JWT auth until the email is verified./auth/verify-email/<token>/endpoint and a frontend verification page.Why
Validation
python3 backend/manage.py test users -v 2python3 -m py_compile backend/users/models.py backend/users/views.py backend/users/serializers.py backend/users/jwt.py backend/users/authentication.py backend/users/permissions.py backend/users/tests.py backend/backend/settings.pygit diff --checkCloses #335
Summary by CodeRabbit
New Features
Tests
Chores