[Eng-32] feat :create otp based reset for password#3630
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR centralizes OTP generation/sending with purpose-specific renderers, renames the OTP model to MobileOTP (with migration), adapts login to use MobileOTP, adds an OTP-based password-reset send/confirm API, updates SMS message settings, and registers the new route. ChangesOTP Authentication Refactoring and Password Reset
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
care/users/models.py (1)
271-276: Add an index for the OTP lookup query, and please pick a story foris_used.A couple of things worth tightening up:
- Missing index on the hot path.
OTPResetSendViewandOTPResetConfirmViewboth filter on(phone_number, is_used, created_date). Without an index, this scans the table on every send/confirm. A composite index will scale much better.is_usedis never set toTrue. The confirm view just.delete()s all OTPs for the phone number. Either keep the flag and mark it consumed (preferred — preserves an audit trail and avoids deleting unrelated rows), or drop the field entirely. Right now it's dead weight that pretends to do something.- Consider an explicit
expires_at(or rely strictly onOTP_REPEAT_WINDOWeverywhere), and aMeta.verbose_namefor admin niceties.♻️ Proposed refactor
class UserMobileOTP(BaseModel): is_used = models.BooleanField(default=False) phone_number = models.CharField( max_length=14, validators=[mobile_or_landline_number_validator] ) otp = models.CharField(max_length=10) + + class Meta: + verbose_name = "User Mobile OTP" + indexes = [ + models.Index( + fields=["phone_number", "is_used", "-created_date"], + name="user_mobile_otp_lookup_idx", + ), + ]As per coding guidelines: "Implement database indexing and query optimization techniques for better performance."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/users/models.py` around lines 271 - 276, Add a composite DB index on UserMobileOTP for (phone_number, is_used, created_date) and either use or remove the is_used flag: update the model class UserMobileOTP to include a Meta.indexes entry for that composite index and add Meta.verbose_name; then modify OTPResetConfirmView (and OTPResetSendView lookups) to mark matching OTP rows as consumed by setting is_used=True (instead of deleting all rows) and/or set an explicit expires_at timestamp on creation so queries can filter on expiry rather than relying solely on OTP_REPEAT_WINDOW. Ensure all references to created_date filtering in OTPResetSendView and OTPResetConfirmView are updated to use the new index and expires_at where applicable.care/users/api/otp_viewset/reset_password.py (2)
119-125: Recommended: invalidate active sessions/tokens and minimize the save.Two follow-ups on the post-reset bookkeeping:
- After resetting a password, existing JWT refresh tokens and Django sessions remain valid. A common expectation for password-reset flows is that all other sessions are killed. Consider blacklisting outstanding refresh tokens for the user (
simplejwt'sOutstandingToken/BlacklistedToken) and bumping apassword_changed_atfield if you have one.user.save()writes every field; passupdate_fields=["password"]to avoid spurious side-effects from any customsaveoverrides.- Per the earlier comment on
UserMobileOTP.is_used, prefer marking the consumed row asis_used=Trueinstead of deleting all rows for the number — useful for audit/forensics.♻️ Suggested patch
- UserMobileOTP.objects.filter( - phone_number=data.phone_number, - ).delete() - - user.set_password(data.password) - user.save() + otp_obj.is_used = True + otp_obj.save(update_fields=["is_used"]) + + user.set_password(data.password) + user.save(update_fields=["password"]) + # TODO: invalidate outstanding refresh tokens / sessions for this user. return Response({"message": "Password reset successful"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/users/api/otp_viewset/reset_password.py` around lines 119 - 125, Replace the destructive delete and plain save with explicit post-reset bookkeeping: mark the consumed UserMobileOTP row(s) using UserMobileOTP.is_used=True (rather than deleting) for auditability, call user.set_password(data.password) and persist only the password change via user.save(update_fields=["password"]), and invalidate existing sessions/tokens by blacklisting the user's outstanding JWT refresh tokens (use simplejwt's OutstandingToken/BlacklistedToken) and/or updating a password_changed_at timestamp on the user record to force session revocation. Ensure you reference UserMobileOTP, user.set_password, user.save(update_fields=["password"]), OutstandingToken/BlacklistedToken and password_changed_at when implementing these changes.
76-89: Narrow the exception and don't break atomicity between SMS and DB record.A few smaller items here:
except Exception(RuffBLE001) hides programming bugs. Catch the specific exception(s) raised byget_sms_content/sms.send_text_messageinstead.logger.error(e)doesn't include the traceback —logger.exception("Failed to send reset OTP")is more useful and avoids stringifying the exception.- The DB record is created after a successful SMS send. If
UserMobileOTP.objects.createthen fails (rare but possible), you've SMS'd an OTP that nobody can verify. Consider creating the record first inside a transaction, then sending the SMS, and rolling back / marking failed on send error. Or at least swap tologger.exceptionand revisit ordering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/users/api/otp_viewset/reset_password.py` around lines 76 - 89, The try/except currently catches Exception and logs without traceback and sends SMS before creating the DB record, which risks losing atomicity; instead, wrap the creation of UserMobileOTP.objects.create in a transaction.atomic block, create the OTP record first, then call get_sms_content and sms.send_text_message; narrow the except to the specific exceptions those functions raise (e.g., template/rendering errors and your SMS client error) and use logger.exception("Failed to send reset OTP") to log tracebacks; if sending fails, roll back or delete/mark the created UserMobileOTP record so an unsync'd SMS can't be verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@care/users/api/otp_viewset/reset_password.py`:
- Around line 22-26: rand_pass currently returns a hard-coded OTP when
settings.USE_SMS is False, allowing account takeover; instead always generate a
secure random OTP in rand_pass (use secrets.choice over string.digits for the
requested size) and only gate delivery on settings.USE_SMS (i.e., do not send
SMS when disabled), and for deterministic tests override/mock rand_pass in test
fixtures rather than relying on USE_SMS; if you must keep a deterministic
fallback, restrict that code path to settings.DEBUG and an explicit test-mode
flag (or when running under the test runner) and add a conspicuous warning,
referencing rand_pass and the OTPResetConfirmView delivery logic so generation
and delivery are separated.
- Around line 47-49: The OTPResetConfirmSpec currently allows any 4-char
password and bypasses Django validators; in OTPResetConfirmView.post, after you
locate the user, call
django.contrib.auth.password_validation.validate_password(data.password,
user=user) and catch django.core.exceptions.ValidationError to re-raise as
rest_framework.exceptions.ValidationError({"password":
validation_error.messages}) so validation failures are returned on the
"password" field; also consider increasing OTPResetConfirmSpec.password Field
min_length to at least 8 to match MinimumLengthValidator.
- Around line 60-72: The current flow leaks user existence because the sent_otps
count check runs before the user existence check (sent_otps, User,
settings.OTP_MAX_REPEATS_WINDOW) and is racy; reorder the logic so you first
check whether the phone_number exists (User.objects.filter(...).exists()) and
always return the same silent response for non-existent numbers, then apply the
repeat-window check only for existing users. To fix the TOCTOU race on
sent_otps, wrap the rate check+creation in a DB transaction
(transaction.atomic()) and use a locking read (select_for_update() on the
UserMobileOTP queryset) or replace with an atomic Redis counter; also add/stack
a coarser IP-based rate limit via django_ratelimit for the reset_password
endpoint to mitigate attacker rotation.
- Line 75: Remove the credential leak by deleting or disabling the
logger.info(random_otp) call; do not log OTP values. If a local debug aid is
required, guard it with settings.DEBUG and use logger.debug(...) (and log only a
one-way hash or an OTP id, not the OTP itself). Also revisit the rand_pass
fallback ("45612") in the same module and replace it with a secure, non-constant
fallback or raise an error so secrets are not predictable.
- Around line 32-40: The validator OTPBaseSpec.validate_phone_number currently
calls mobile_validator and catches Exception broadly, which mismatches
User.phone_number and UserMobileOTP.phone_number (which use
mobile_or_landline_number_validator) and hides intent; update
validate_phone_number to either call mobile_or_landline_number_validator if
landlines should be accepted, or keep mobile_validator but add an inline comment
stating landline-only numbers are intentionally rejected for OTP, and replace
the broad "except Exception" with catching the specific ValidationError from
PhoneNumberValidator so only validation failures are handled and original
exceptions propagate.
- Around line 100-117: In the post method of the OTP reset flow
(OTPResetConfirmSpec input handling in post), replace the explicit "No account
found" ValidationError with the same generic OTP error used for invalid OTPs so
phone enumeration is not possible; add tracking on UserMobileOTP (e.g., a
failed_attempts integer and last_failed_at) and increment it on each mismatch,
invalidating or marking is_used=True when failed_attempts reaches a constant
(e.g., OTP_MAX_FAILED_ATTEMPTS = 5) to prevent brute-force, and perform the OTP
comparison using secrets.compare_digest(otp_obj.otp, data.otp) for
timing-safety; optionally add/annotate a rate-limit decorator (django_ratelimit)
keyed by phone_number+IP to provide a second layer of protection.
---
Nitpick comments:
In `@care/users/api/otp_viewset/reset_password.py`:
- Around line 119-125: Replace the destructive delete and plain save with
explicit post-reset bookkeeping: mark the consumed UserMobileOTP row(s) using
UserMobileOTP.is_used=True (rather than deleting) for auditability, call
user.set_password(data.password) and persist only the password change via
user.save(update_fields=["password"]), and invalidate existing sessions/tokens
by blacklisting the user's outstanding JWT refresh tokens (use simplejwt's
OutstandingToken/BlacklistedToken) and/or updating a password_changed_at
timestamp on the user record to force session revocation. Ensure you reference
UserMobileOTP, user.set_password, user.save(update_fields=["password"]),
OutstandingToken/BlacklistedToken and password_changed_at when implementing
these changes.
- Around line 76-89: The try/except currently catches Exception and logs without
traceback and sends SMS before creating the DB record, which risks losing
atomicity; instead, wrap the creation of UserMobileOTP.objects.create in a
transaction.atomic block, create the OTP record first, then call get_sms_content
and sms.send_text_message; narrow the except to the specific exceptions those
functions raise (e.g., template/rendering errors and your SMS client error) and
use logger.exception("Failed to send reset OTP") to log tracebacks; if sending
fails, roll back or delete/mark the created UserMobileOTP record so an unsync'd
SMS can't be verified.
In `@care/users/models.py`:
- Around line 271-276: Add a composite DB index on UserMobileOTP for
(phone_number, is_used, created_date) and either use or remove the is_used flag:
update the model class UserMobileOTP to include a Meta.indexes entry for that
composite index and add Meta.verbose_name; then modify OTPResetConfirmView (and
OTPResetSendView lookups) to mark matching OTP rows as consumed by setting
is_used=True (instead of deleting all rows) and/or set an explicit expires_at
timestamp on creation so queries can filter on expiry rather than relying solely
on OTP_REPEAT_WINDOW. Ensure all references to created_date filtering in
OTPResetSendView and OTPResetConfirmView are updated to use the new index and
expires_at where applicable.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e4176c30-f594-496e-88f8-b595ce32a95b
📒 Files selected for processing (7)
care/templates/sms/otp_reset_sms.txtcare/users/api/otp_viewset/__init__.pycare/users/api/otp_viewset/reset_password.pycare/users/migrations/0028_usermobileotp.pycare/users/models.pyconfig/settings/base.pyconfig/urls.py
There was a problem hiding this comment.
Pull request overview
Adds OTP-by-SMS based password reset endpoints for users, including a new OTP persistence model and an SMS template/config setting.
Changes:
- Added unauthenticated APIs to send a password-reset OTP and confirm OTP + set a new password.
- Introduced
UserMobileOTPmodel + migration to store OTPs for password reset. - Added reset-password OTP SMS template + settings for selecting the template and new URL routes.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| config/urls.py | Registers new OTP password reset endpoints under /api/v1/otp/password_reset/.... |
| config/settings/base.py | Adds setting/env var for reset-password OTP SMS template path. |
| care/users/models.py | Adds UserMobileOTP model to persist OTPs for password reset. |
| care/users/migrations/0028_usermobileotp.py | Creates DB table for UserMobileOTP. |
| care/users/api/otp_viewset/reset_password.py | Implements OTP send/confirm API views and request specs. |
| care/templates/sms/otp_reset_sms.txt | Adds SMS template content for reset-password OTP message. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3630 +/- ##
===========================================
- Coverage 75.86% 75.85% -0.02%
===========================================
Files 479 479
Lines 23031 23044 +13
Branches 2378 2378
===========================================
+ Hits 17472 17479 +7
- Misses 4986 4992 +6
Partials 573 573 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nandkishorr
changed the title
vigneshhari
requested changes
Apr 29, 2026
vigneshhari
left a comment
vigneshhari
left a comment
Member
There was a problem hiding this comment.
Rewrite this whole MR only using existing models and existing code. No changes more than 150 lines allowed.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
care/users/api/otp_viewset/reset_password.py (6)
128-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConfirm endpoint still enumerates users
Line 129 returns a user-existence-specific error (
"No User linked..."). This leaks registration status and undercuts the send endpoint’s generic behavior.🤖 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 `@care/users/api/otp_viewset/reset_password.py` around lines 128 - 129, The confirm endpoint currently raises a user-specific ValidationError ("No User linked to this phone number"), which leaks registration status; update the raise in the reset_password confirm flow (the raise ValidationError call) to return the same generic, non-enumerating error message used by the send endpoint (e.g. a generic "If this phone number is registered, you will receive an OTP" or the existing send-endpoint message) so both paths are indistinguishable; locate the raise ValidationError in reset_password.py (the confirm handler) and replace the specific message with the shared generic message constant/string used by the send endpoint.
76-107:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRate-limit check and OTP creation are racy (TOCTOU)
The count-then-create flow is non-atomic; concurrent requests can exceed
OTP_MAX_REPEATS_WINDOW. Wrap the check+create in a transaction with locking (or move to an atomic external counter).🤖 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 `@care/users/api/otp_viewset/reset_password.py` around lines 76 - 107, The current count-then-create for UserMobileOTP is vulnerable to TOCTOU races; wrap the logic that queries recent OTPs and creates a new UserMobileOTP in a single atomic DB transaction and acquire a lock on the relevant rows to serialize concurrent requests. Concretely, enclose the check + UserMobileOTP.objects.create(...) inside transaction.atomic() and use UserMobileOTP.objects.select_for_update().filter(phone_number=data.phone_number, created_date__gte=(timezone.now() - timedelta(hours=settings.OTP_REPEAT_WINDOW)), is_used=False) to lock the matching rows before re-counting and deciding whether to raise the OTP_MAX_REPEATS_WINDOW ValidationError, then create the OTP only after the locked check passes.
28-32:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: deterministic OTP fallback enables trivial account takeover
On Line 29-30, returning
"45612"when SMS is disabled makes OTPs predictable in non-SMS environments. That’s a direct auth bypass, and yes, it’s as bad as it sounds.Suggested fix
def rand_pass(size): - if not settings.USE_SMS: - return "45612" - return "".join(secrets.choice(string.digits) for _ in range(size))🤖 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 `@care/users/api/otp_viewset/reset_password.py` around lines 28 - 32, The rand_pass function currently returns a hardcoded "45612" when settings.USE_SMS is false, creating a predictable OTP; change rand_pass to never return a constant fallback—use a secure random generator (e.g., secrets.choice over string.digits) to build a numeric OTP of the requested size even when settings.USE_SMS is false (or alternatively raise an explicit exception if non-SMS flows should not generate OTPs). Update the rand_pass implementation (referencing the function name rand_pass and the settings.USE_SMS check and the size parameter) so it uses secrets to generate a size-length digit string instead of the deterministic "45612".
41-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNarrow exception handling in phone validator
Catching
Exceptionon Line 43 is too broad and can mask unrelated failures. Catch the specific validation exception instead.Suggested fix
+from django.core.exceptions import ValidationError as DjangoValidationError @@ - except Exception as e: + except DjangoValidationError as e: msg = "Invalid phone number" raise ValueError(msg) from e🤖 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 `@care/users/api/otp_viewset/reset_password.py` around lines 41 - 45, The code currently catches all Exceptions around mobile_validator(value) and re-raises ValueError; narrow this by catching the specific exception type the validator raises (e.g., ValidationError or the validator's custom exception) instead of Exception. Update the except clause for the mobile_validator call to import and catch that exact exception (for example except ValidationError as e:) and then raise ValueError(msg) from e so only validation failures are handled while other errors bubble up.
130-143:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftNo failed-attempt cap on OTP verification
There’s still no per-phone/per-OTP failed-attempt lockout in confirm flow. With a short numeric OTP, this leaves a practical brute-force window.
🤖 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 `@care/users/api/otp_viewset/reset_password.py` around lines 130 - 143, The confirm-flow currently directly compares OTPs and lacks a per-phone/OTP failed-attempt lockout; modify the logic around UserMobileOTP in reset_password.py to enforce a max-failed-attempts and temporary lockout: add fields to the UserMobileOTP model (e.g., failed_attempts integer and lockout_until datetime) or reuse existing equivalents, check lockout_until (timezone.now()) before validating, and if locked raise a ValidationError like {"otp":"Too many attempts, try later"}; on failed match increment failed_attempts atomically (use F() or select_for_update) and if failed_attempts reaches settings.OTP_MAX_FAILED_ATTEMPTS set lockout_until = timezone.now() + timedelta(minutes=settings.OTP_LOCKOUT_WINDOW) (and optionally mark is_used), while on success reset failed_attempts and clear lockout_until and proceed as before. Ensure you reference UserMobileOTP, the OTP-check block in reset_password.py, and use settings keys (OTP_MAX_FAILED_ATTEMPTS, OTP_LOCKOUT_WINDOW) and timezone.now() for time comparisons.
76-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUser enumeration still leaks via rate-limit branch ordering
Line 83-86 can return a distinct “max OTP requests exceeded” response before the non-existent-user path at Line 87-88. Attackers can still differentiate registered numbers over repeated probes.
Suggested fix
- sent_otps = UserMobileOTP.objects.filter( + if not User.objects.filter(phone_number=data.phone_number).exists(): + return Response({"otp": "generated"}) + + sent_otps = UserMobileOTP.objects.filter( created_date__gte=( timezone.now() - timedelta(hours=settings.OTP_REPEAT_WINDOW) ), is_used=False, phone_number=data.phone_number, ) if sent_otps.count() >= settings.OTP_MAX_REPEATS_WINDOW: - raise ValidationError( - {"error": "Max OTP requests exceeded. Try again later."} - ) - if not User.objects.filter(phone_number=data.phone_number).exists(): return Response({"otp": "generated"})🤖 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 `@care/users/api/otp_viewset/reset_password.py` around lines 76 - 88, Reorder the checks to avoid user enumeration: first verify whether the account exists via User.objects.filter(phone_number=data.phone_number).exists() and if the user does not exist return the same generic response ("otp": "generated") as used for other branches; only for existing users proceed to evaluate sent_otps = UserMobileOTP.objects.filter(...), compare against settings.OTP_MAX_REPEATS_WINDOW and raise the rate-limit ValidationError if exceeded. This ensures the UserMobileOTP/User existence logic (sent_otps, User, settings.OTP_MAX_REPEATS_WINDOW) cannot be probed to distinguish registered vs unregistered numbers.
🧹 Nitpick comments (1)
care/users/api/otp_viewset/reset_password.py (1)
59-60: ⚡ Quick winUse immutable class attributes for DRF auth/permission lists
Using mutable lists for
authentication_classesandpermission_classestriggers RuffRUF012and is avoidable. Tuples are the safer/default choice.Suggested fix
- authentication_classes = [] - permission_classes = [] + authentication_classes = () + permission_classes = () @@ - authentication_classes = [] - permission_classes = [] + authentication_classes = () + permission_classes = ()As per coding guidelines, "
**/*.py: Use Ruff for linting and formatting Python code ... After code changes, run 'ruff check --fix .' and 'ruff format .'".Also applies to: 112-113
🤖 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 `@care/users/api/otp_viewset/reset_password.py` around lines 59 - 60, The class-level attributes authentication_classes and permission_classes are defined as mutable lists which triggers RUF012; change them to immutable tuples (e.g., authentication_classes = () and permission_classes = ()) wherever they appear (including in reset_password.py and the other occurrence around lines 112-113) and then run the project linter/formatter (ruff check --fix . and ruff format .) to apply/verify fixes.
🤖 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.
Duplicate comments:
In `@care/users/api/otp_viewset/reset_password.py`:
- Around line 128-129: The confirm endpoint currently raises a user-specific
ValidationError ("No User linked to this phone number"), which leaks
registration status; update the raise in the reset_password confirm flow (the
raise ValidationError call) to return the same generic, non-enumerating error
message used by the send endpoint (e.g. a generic "If this phone number is
registered, you will receive an OTP" or the existing send-endpoint message) so
both paths are indistinguishable; locate the raise ValidationError in
reset_password.py (the confirm handler) and replace the specific message with
the shared generic message constant/string used by the send endpoint.
- Around line 76-107: The current count-then-create for UserMobileOTP is
vulnerable to TOCTOU races; wrap the logic that queries recent OTPs and creates
a new UserMobileOTP in a single atomic DB transaction and acquire a lock on the
relevant rows to serialize concurrent requests. Concretely, enclose the check +
UserMobileOTP.objects.create(...) inside transaction.atomic() and use
UserMobileOTP.objects.select_for_update().filter(phone_number=data.phone_number,
created_date__gte=(timezone.now() -
timedelta(hours=settings.OTP_REPEAT_WINDOW)), is_used=False) to lock the
matching rows before re-counting and deciding whether to raise the
OTP_MAX_REPEATS_WINDOW ValidationError, then create the OTP only after the
locked check passes.
- Around line 28-32: The rand_pass function currently returns a hardcoded
"45612" when settings.USE_SMS is false, creating a predictable OTP; change
rand_pass to never return a constant fallback—use a secure random generator
(e.g., secrets.choice over string.digits) to build a numeric OTP of the
requested size even when settings.USE_SMS is false (or alternatively raise an
explicit exception if non-SMS flows should not generate OTPs). Update the
rand_pass implementation (referencing the function name rand_pass and the
settings.USE_SMS check and the size parameter) so it uses secrets to generate a
size-length digit string instead of the deterministic "45612".
- Around line 41-45: The code currently catches all Exceptions around
mobile_validator(value) and re-raises ValueError; narrow this by catching the
specific exception type the validator raises (e.g., ValidationError or the
validator's custom exception) instead of Exception. Update the except clause for
the mobile_validator call to import and catch that exact exception (for example
except ValidationError as e:) and then raise ValueError(msg) from e so only
validation failures are handled while other errors bubble up.
- Around line 130-143: The confirm-flow currently directly compares OTPs and
lacks a per-phone/OTP failed-attempt lockout; modify the logic around
UserMobileOTP in reset_password.py to enforce a max-failed-attempts and
temporary lockout: add fields to the UserMobileOTP model (e.g., failed_attempts
integer and lockout_until datetime) or reuse existing equivalents, check
lockout_until (timezone.now()) before validating, and if locked raise a
ValidationError like {"otp":"Too many attempts, try later"}; on failed match
increment failed_attempts atomically (use F() or select_for_update) and if
failed_attempts reaches settings.OTP_MAX_FAILED_ATTEMPTS set lockout_until =
timezone.now() + timedelta(minutes=settings.OTP_LOCKOUT_WINDOW) (and optionally
mark is_used), while on success reset failed_attempts and clear lockout_until
and proceed as before. Ensure you reference UserMobileOTP, the OTP-check block
in reset_password.py, and use settings keys (OTP_MAX_FAILED_ATTEMPTS,
OTP_LOCKOUT_WINDOW) and timezone.now() for time comparisons.
- Around line 76-88: Reorder the checks to avoid user enumeration: first verify
whether the account exists via
User.objects.filter(phone_number=data.phone_number).exists() and if the user
does not exist return the same generic response ("otp": "generated") as used for
other branches; only for existing users proceed to evaluate sent_otps =
UserMobileOTP.objects.filter(...), compare against
settings.OTP_MAX_REPEATS_WINDOW and raise the rate-limit ValidationError if
exceeded. This ensures the UserMobileOTP/User existence logic (sent_otps, User,
settings.OTP_MAX_REPEATS_WINDOW) cannot be probed to distinguish registered vs
unregistered numbers.
---
Nitpick comments:
In `@care/users/api/otp_viewset/reset_password.py`:
- Around line 59-60: The class-level attributes authentication_classes and
permission_classes are defined as mutable lists which triggers RUF012; change
them to immutable tuples (e.g., authentication_classes = () and
permission_classes = ()) wherever they appear (including in reset_password.py
and the other occurrence around lines 112-113) and then run the project
linter/formatter (ruff check --fix . and ruff format .) to apply/verify fixes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9b11e806-cb03-451b-b10d-af6b115ef9e6
📒 Files selected for processing (1)
care/users/api/otp_viewset/reset_password.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@care/emr/api/otp_viewsets/login.py`:
- Around line 34-73: The MobileOTP rows lack a stored purpose so different flows
can consume each other's OTPs; modify the MobileOTP model to add a purpose field
(enum/char matching OTPType) and an index on (phone_number, purpose, otp,
is_used) via a migration, then update send_otp to include purpose when creating
MobileOTP and to filter sent_otps by purpose, and update OTPLoginView.login and
OTPResetPasswordView.confirm (and any cleanup/delete logic) to include purpose
in their queryset filters and deletes so each flow only reads/deletes OTPs with
the matching purpose.
- Around line 35-41: The rate-limit window uses
timedelta(settings.OTP_REPEAT_WINDOW) which treats the value as days; update the
calculation so the window is built with hours—e.g. replace the timedelta usage
in the sent_otps query (the MobileOTP filter where created_date__gte is
computed) to use timedelta(hours=settings.OTP_REPEAT_WINDOW) so the
OTP_REPEAT_WINDOW setting is applied as hours when enforcing
OTP_MAX_REPEATS_WINDOW.
- Around line 115-117: The current lookup for MobileOTP (using
MobileOTP.objects.filter(...).first()) doesn't enforce the OTP validity window;
change the query to also filter created_date >= timezone.now() -
OTP_REPEAT_WINDOW (use Django timezone) so only recent OTPs are accepted, and
immediately expire the OTP upon successful validation by setting
otp_object.is_used = True and saving (or perform an atomic update like
MobileOTP.objects.filter(pk=otp_object.pk, is_used=False).update(is_used=True)
to avoid races). Reference MobileOTP, OTP_REPEAT_WINDOW, otp_object, and the
data.phone_number/data.otp fields when applying these changes.
In `@config/exception_handler.py`:
- Around line 12-13: The Pydantic error conversion is leaking submitted values
via exc.errors(); update the conversion where PydanticValidationError is mapped
to DRFValidationError (the block that currently does exc =
DRFValidationError(detail=exc.errors())) to filter or remap the error list and
remove any "input" keys or nested submitted values (specifically strip
password/code fields originating from PasswordVerifyRequest and
TOTPVerifyRequest) before constructing the DRFValidationError; ensure you
iterate exc.errors(), remove or redact any "input" entries (or replace them with
safe placeholders), and pass the sanitized list into
DRFValidationError(detail=...).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b6380c58-61ee-4c34-87fc-aef2fdeaa6c4
📒 Files selected for processing (6)
care/emr/api/otp_viewsets/login.pycare/facility/migrations/0485_rename_patientmobileotp_mobileotp.pycare/facility/models/patient.pycare/users/api/otp_viewset/reset_password.pyconfig/api_router.pyconfig/exception_handler.py
Comment on lines
+115
to
117
| otp_object = MobileOTP.objects.filter( | ||
| phone_number=data.phone_number, otp=data.otp, is_used=False | ||
| ).first() |
Contributor
There was a problem hiding this comment.
Expire login OTPs when validating them.
Lines 115-117 accept any unused row matching the phone number and OTP, with no created_date bound. That makes stale OTPs valid indefinitely until someone happens to use them, despite OTP_REPEAT_WINDOW being defined as the validity window.
Suggested fix
- otp_object = MobileOTP.objects.filter(
- phone_number=data.phone_number, otp=data.otp, is_used=False
- ).first()
+ otp_object = (
+ MobileOTP.objects.filter(
+ phone_number=data.phone_number,
+ otp=data.otp,
+ is_used=False,
+ created_date__gte=(
+ timezone.now() - timedelta(hours=settings.OTP_REPEAT_WINDOW)
+ ),
+ )
+ .order_by("-created_date")
+ .first()
+ )🤖 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 `@care/emr/api/otp_viewsets/login.py` around lines 115 - 117, The current
lookup for MobileOTP (using MobileOTP.objects.filter(...).first()) doesn't
enforce the OTP validity window; change the query to also filter created_date >=
timezone.now() - OTP_REPEAT_WINDOW (use Django timezone) so only recent OTPs are
accepted, and immediately expire the OTP upon successful validation by setting
otp_object.is_used = True and saving (or perform an atomic update like
MobileOTP.objects.filter(pk=otp_object.pk, is_used=False).update(is_used=True)
to avoid races). Reference MobileOTP, OTP_REPEAT_WINDOW, otp_object, and the
data.phone_number/data.otp fields when applying these changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@care/users/api/otp_viewset/reset_password.py`:
- Around line 74-99: Make the OTP consumption transactional: wrap the OTP
read-and-claim in a transaction.atomic() and use select_for_update() to lock the
matching OTP row before validating it, then mark it consumed (is_used=True and
save) before performing validate_password, user.set_password and user.save.
Locate the MobileOTP lookup (the variable otp_obj) and replace the non-atomic
read with
MobileOTP.objects.select_for_update().filter(phone_number=data.phone_number,
is_used=False, created_date__gte=(timezone.now() -
timedelta(hours=settings.OTP_REPEAT_WINDOW))).order_by("-created_date").first()
inside a transaction.atomic() block, check otp_obj and otp match, set
otp_obj.is_used=True and save immediately to atomically claim the OTP, then
continue with password validation and user.save.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a4ed46b0-4aeb-466b-8d1e-1b8baf23d5a7
📒 Files selected for processing (3)
care/emr/api/otp_viewsets/login.pycare/users/api/otp_viewset/reset_password.pyconfig/settings/base.py
vigneshhari
requested changes
May 20, 2026
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
care/emr/api/otp_viewsets/login.py (1)
107-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClaim the OTP atomically before issuing the token.
The current read-then-save flow lets two concurrent requests reuse the same OTP if they both pass the
is_used=Falsecheck before Line 113 flips it.🛠️ Suggested fix
otp_object = MobileOTP.objects.filter( phone_number=data.phone_number, otp=data.otp, is_used=False ).first() if not otp_object: raise ValidationError({"otp": "Invalid OTP"}) - otp_object.is_used = True - otp_object.save() + claimed = MobileOTP.objects.filter(pk=otp_object.pk, is_used=False).update( + is_used=True + ) + if not claimed: + raise ValidationError({"otp": "Invalid OTP"})🤖 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 `@care/emr/api/otp_viewsets/login.py` around lines 107 - 114, The code performs a non-atomic read-then-write on MobileOTP (otp_object = MobileOTP.objects.filter(...).first(); otp_object.is_used = True; otp_object.save()) which allows race conditions; wrap the claim in a DB transaction and atomically claim the OTP (use either an UPDATE filter that sets is_used=True where phone_number=data.phone_number, otp=data.otp, is_used=False and check the affected row count, or perform a transaction.atomic with MobileOTP.objects.select_for_update(...) to lock the row before marking is_used=True). If the atomic update affects 0 rows, raise the same ValidationError({"otp":"Invalid OTP"}); otherwise proceed to issue the token. Ensure you reference MobileOTP, otp_object, and the view/function handling the OTP claim.
♻️ Duplicate comments (2)
care/emr/api/otp_viewsets/login.py (2)
38-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
hours=when building the resend window.
timedelta(settings.OTP_REPEAT_WINDOW)treats6as days, so once the cap is hit the user gets blocked for six days, not six hours.🛠️ Suggested fix
sent_otps = MobileOTP.objects.filter( - created_date__gte=(timezone.now() - timedelta(settings.OTP_REPEAT_WINDOW)), + created_date__gte=( + timezone.now() - timedelta(hours=settings.OTP_REPEAT_WINDOW) + ), is_used=False, phone_number=phone_number, )🤖 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 `@care/emr/api/otp_viewsets/login.py` around lines 38 - 44, The code builds the resend window incorrectly using timedelta(settings.OTP_REPEAT_WINDOW) which interprets the value as days; change it to use an hours keyword so the window is hours, e.g., use timedelta(hours=settings.OTP_REPEAT_WINDOW) when computing created_date__gte in the MobileOTP.objects.filter call (the logic around sent_otps and the OTP_REPEAT_WINDOW / OTP_MAX_REPEATS_WINDOW checks should remain the same).
107-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpire OTPs during login validation.
This lookup accepts any unused matching row, so an old OTP stays valid until someone eventually uses it. That quietly ignores
OTP_REPEAT_WINDOW.🛠️ Suggested fix
otp_object = MobileOTP.objects.filter( - phone_number=data.phone_number, otp=data.otp, is_used=False - ).first() + phone_number=data.phone_number, + otp=data.otp, + is_used=False, + created_date__gte=( + timezone.now() - timedelta(hours=settings.OTP_REPEAT_WINDOW) + ), + ).order_by("-created_date").first()🤖 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 `@care/emr/api/otp_viewsets/login.py` around lines 107 - 109, The login lookup currently picks any unused MobileOTP row and ignores OTP_REPEAT_WINDOW; modify the logic around the otp_object assignment so old OTPs are expired before/when validating: either filter the queryset to include created_at__gte=timezone.now() - OTP_REPEAT_WINDOW (using Django timezone) when building MobileOTP.objects.filter(phone_number=..., otp=..., is_used=False) or proactively mark stale rows as used with MobileOTP.objects.filter(phone_number=data.phone_number, is_used=False, created_at__lt=timezone.now() - OTP_REPEAT_WINDOW).update(is_used=True) and then perform the existing lookup; reference MobileOTP, OTP_REPEAT_WINDOW, created_at and the otp_object assignment to locate the change.
🧹 Nitpick comments (1)
config/settings/base.py (1)
693-700: ⚡ Quick winValidate the
{otp}placeholder at startup.These settings are now part of the deployment contract, so an override without
{otp}will either send a useless SMS or fail only when this path is hit. Catching that during startup is a lot less exciting.🧩 Suggested fix
+from django.core.exceptions import ImproperlyConfigured + ... OTP_SMS_LOGIN_CONTENT = env( "OTP_SMS_LOGIN_CONTENT", default="Care OTP for login is {otp}. Please do not share this with anyone.", ) +if "{otp}" not in OTP_SMS_LOGIN_CONTENT: + raise ImproperlyConfigured( + "OTP_SMS_LOGIN_CONTENT must include the '{otp}' placeholder" + ) OTP_SMS_RESET_PASSWORD_CONTENT = env( "OTP_SMS_RESET_PASSWORD_CONTENT", default="Care OTP for password reset is {otp}. Please do not share this with anyone.", ) +if "{otp}" not in OTP_SMS_RESET_PASSWORD_CONTENT: + raise ImproperlyConfigured( + "OTP_SMS_RESET_PASSWORD_CONTENT must include the '{otp}' placeholder" + )🤖 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 `@config/settings/base.py` around lines 693 - 700, Validate that the OTP SMS templates OTP_SMS_LOGIN_CONTENT and OTP_SMS_RESET_PASSWORD_CONTENT contain the "{otp}" placeholder at startup by checking the loaded values immediately after they are read from env and raising a clear configuration error (e.g., ValueError or Django's ImproperlyConfigured) if "{otp}" is missing; add this check in the same module (base.py) so the process fails fast during import rather than at runtime when sending SMS.
🤖 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 `@care/emr/api/otp_viewsets/login.py`:
- Around line 93-99: The endpoint currently returns raw exception text and maps
unexpected errors to HTTP 400; change the except Exception block in the login
OTP flow (around send_otp(...) / LoginOTP()) to log the full exception
internally (e.g., logger.exception or process_logger.exception) and return a
generic error Response with a 500 status and no internal details (e.g.,
{"error": "internal server error"}), while preserving the existing ValueError ->
ValidationError behavior for invalid phone input.
---
Outside diff comments:
In `@care/emr/api/otp_viewsets/login.py`:
- Around line 107-114: The code performs a non-atomic read-then-write on
MobileOTP (otp_object = MobileOTP.objects.filter(...).first();
otp_object.is_used = True; otp_object.save()) which allows race conditions; wrap
the claim in a DB transaction and atomically claim the OTP (use either an UPDATE
filter that sets is_used=True where phone_number=data.phone_number,
otp=data.otp, is_used=False and check the affected row count, or perform a
transaction.atomic with MobileOTP.objects.select_for_update(...) to lock the row
before marking is_used=True). If the atomic update affects 0 rows, raise the
same ValidationError({"otp":"Invalid OTP"}); otherwise proceed to issue the
token. Ensure you reference MobileOTP, otp_object, and the view/function
handling the OTP claim.
---
Duplicate comments:
In `@care/emr/api/otp_viewsets/login.py`:
- Around line 38-44: The code builds the resend window incorrectly using
timedelta(settings.OTP_REPEAT_WINDOW) which interprets the value as days; change
it to use an hours keyword so the window is hours, e.g., use
timedelta(hours=settings.OTP_REPEAT_WINDOW) when computing created_date__gte in
the MobileOTP.objects.filter call (the logic around sent_otps and the
OTP_REPEAT_WINDOW / OTP_MAX_REPEATS_WINDOW checks should remain the same).
- Around line 107-109: The login lookup currently picks any unused MobileOTP row
and ignores OTP_REPEAT_WINDOW; modify the logic around the otp_object assignment
so old OTPs are expired before/when validating: either filter the queryset to
include created_at__gte=timezone.now() - OTP_REPEAT_WINDOW (using Django
timezone) when building MobileOTP.objects.filter(phone_number=..., otp=...,
is_used=False) or proactively mark stale rows as used with
MobileOTP.objects.filter(phone_number=data.phone_number, is_used=False,
created_at__lt=timezone.now() - OTP_REPEAT_WINDOW).update(is_used=True) and then
perform the existing lookup; reference MobileOTP, OTP_REPEAT_WINDOW, created_at
and the otp_object assignment to locate the change.
---
Nitpick comments:
In `@config/settings/base.py`:
- Around line 693-700: Validate that the OTP SMS templates OTP_SMS_LOGIN_CONTENT
and OTP_SMS_RESET_PASSWORD_CONTENT contain the "{otp}" placeholder at startup by
checking the loaded values immediately after they are read from env and raising
a clear configuration error (e.g., ValueError or Django's ImproperlyConfigured)
if "{otp}" is missing; add this check in the same module (base.py) so the
process fails fast during import rather than at runtime when sending SMS.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dd06d766-a027-4425-b9c6-3e0a1e50f042
📒 Files selected for processing (3)
care/emr/api/otp_viewsets/login.pycare/users/api/otp_viewset/reset_password.pyconfig/settings/base.py
vigneshhari
requested changes
May 25, 2026
vigneshhari
left a comment
vigneshhari
left a comment
Member
There was a problem hiding this comment.
Address all the comments and vulnerabilities commented by CodeRabbit as well.
|
|
||
|
|
||
| class PatientMobileOTP(BaseModel): | ||
| class MobileOTP(BaseModel): |
Member
There was a problem hiding this comment.
We need a periodic job to delete all OTP's that are older than the period they are active for.
Member
|
I also cant find any tests on this feature, all lines need to be tested with the OTP sending mechanism mocked. |
nandkishorr
force-pushed
the
ENG-32-create-otp-based-reset-for-password
branch
from
d74176e to
8e03058
Compare
vigneshhari
approved these changes
Jun 5, 2026
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.
Proposed Changes
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
New Features
Improvements