Spec Doc for Email OTP Verification#226
Conversation
zandre-eng
requested review from
ajeety4,
calellowitz,
hemant10yadav,
mkangia and
pxwxnvermx
| 1. **Email uniqueness:** Should a verified email address be unique across active users (i.e. prevent two users from verifying the same address)? A unique constraint would add integrity but could cause friction if a user re-registers with the same email. **Recommendation:** leave uncontrained at MVP, revisit if abuse is observed. | ||
| 2. **Email re-verification:** Can a user verify a different email address and overwrite their existing verified email? | ||
| 3. **Test email bypass:** The design skips email delivery for users whose phone starts with `TEST_NUMBER_PREFIX`. Should a hardcoded test OTP (e.g. `"123456"`) be returned for test users, similar to any existing test-number shortcuts? | ||
| 4. **Rate limit response code:** Should `send_email_otp` return `HTTP 429 Too Many Requests` with a `Retry-After` header when backoff is active, or a `400` with an error code? `429` is more semantically correct and aligns with RFC 6585. |
There was a problem hiding this comment.
@shubham1g5 I'd like to get mobile's thoughts here on what you'd prefer as a response code?
There was a problem hiding this comment.
429 would be best for any rate limiting.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR introduces a comprehensive technical specification document for Email OTP verification. The spec outlines two new API endpoints (send_email_otp and verify_email_otp), model additions (email_verified field on ConnectUser, verified_email on ConfigurationSession, and a new EmailOTPDevice model), authentication flows for sign-up and post-registration contexts, feature-flag gating, SES-backed email delivery via AnyMail, Celery-driven async email sending with exponential backoff, monitoring and alerting strategy, deployment requirements, QA test workflows, and detailed API definitions and implementation tickets. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/plans/2026-05-06-email-otp-verification-spec.md`:
- Line 211: API auth entries for the endpoints incorrectly list only OAuth2;
update the two endpoint auth blocks (the ones showing "Authorization: Bearer
<personal_id_token>") to enumerate both SessionTokenAuthentication (used in the
sign-up/verification flow) and OAuth2Authentication (used post-registration) so
clients know to accept either method; specifically replace the single OAuth2
auth declaration with a dual-auth declaration referencing
SessionTokenAuthentication and OAuth2Authentication in those endpoint
definitions.
- Line 98: The spec uses an inconsistent endpoint name: change the modified view
heading and any occurrences of start_device_configuration to the canonical
endpoint name start_configuration to match the rest of the document; update the
header "**Modified view — `start_device_configuration`**" and any references
within the `start_configuration` section so implementation and QA use the same
endpoint name.
- Around line 112-116: The fenced code block containing the email template
starting with "Subject: Your CommCare Connect verification code" should include
a language tag to satisfy MD040; change the opening fence from ``` to ```text so
the block becomes ```text ... ``` and preserves the template (including the
"{token}" placeholder) while passing the markdown lint check.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 630bbc75-429f-4b6a-b781-e264d5cb0a4e
📒 Files selected for processing (1)
docs/plans/2026-05-06-email-otp-verification-spec.md
| - **Sign-up flow** — the user optionally verifies an email during onboarding, before a `ConnectUser` account exists. Authentication uses a `ConfigurationSession` Bearer token (the same token returned by `start_configuration` and used by `complete_profile`), via `SessionTokenAuthentication`. | ||
| - **Post-registration flow** — an already-registered user verifies or updates their email. Authentication uses an OAuth2 Bearer token, via `OAuth2Authentication` (already the default in `REST_FRAMEWORK` settings). | ||
|
|
||
| Both flows use identical request/response payloads. The feature is gated behind a django-waffle flag (`email_otp_verification`) to allow a phased rollout. |
There was a problem hiding this comment.
Clarifying that this is same as toggles that are sent in response for /start_configuration ?
There was a problem hiding this comment.
That is correct. This will be a global switch for PersonalID that we can use to enable/disable this feature entirely on the platform.
| | 200 | `{}` | OTP email sent (or queued if test user) | | ||
| | 400 | `{"error_code": "MISSING_DATA"}` | Missing or invalid email | | ||
| | 404 | — | Feature flag disabled | | ||
| | 429 | `{"error_code": "RATE_LIMITED", "retry_after_seconds": N}` | Backoff window not elapsed | |
There was a problem hiding this comment.
If possible, can we name it EMAIL_RATE_LIMITED so that mobile code has more readability while handling it?
There was a problem hiding this comment.
That should be fine to do. @Jignesh-dimagi Would mobile also want MISSING_DATA to be EMAIL_MISSING_DATA for consistency?
There was a problem hiding this comment.
@Jignesh-dimagi I would vote for keeping them generic instead, if we are making a request to email API think it's quite obvious that the rate limiting error is around email and we should not need to append EMAIL_ to all these error codes.
3 participants
Technical Summary
Link to ticket here.
This is a release path 3 feature.
This PR contains a spec doc for implementing email OTP verification in PersonalID. Specifically, it will allow users to add an email address to their account which can be used in the future for verification purposes via email.
Please note, this PR will not be merged and exists solely for review purposes.
Logging and monitoring
N/A
Safety Assurance
Safety story
Just a spec document, this is not planned to be merged.
Automated test coverage
N/A
QA Plan
N/A
Labels & Review