From aff7edaea1d511d0cb0ef3ef91303118a51005d8 Mon Sep 17 00:00:00 2001 From: praffq Date: Mon, 4 May 2026 13:54:32 +0530 Subject: [PATCH 01/13] added more control over otps and protect from ddos attacks --- care/emr/api/otp_viewsets/login.py | 65 ++++-- care/emr/tests/test_otp_login.py | 218 ++++++++++++++++++ .../0485_patientmobileotp_failed_attempts.py | 17 ++ care/facility/models/patient.py | 1 + config/settings/base.py | 12 +- 5 files changed, 297 insertions(+), 16 deletions(-) create mode 100644 care/emr/tests/test_otp_login.py create mode 100644 care/facility/migrations/0485_patientmobileotp_failed_attempts.py diff --git a/care/emr/api/otp_viewsets/login.py b/care/emr/api/otp_viewsets/login.py index a442621b72..e03248beb1 100644 --- a/care/emr/api/otp_viewsets/login.py +++ b/care/emr/api/otp_viewsets/login.py @@ -4,11 +4,13 @@ from datetime import timedelta from django.conf import settings +from django.db import transaction +from django.db.models import Sum from django.utils import timezone from drf_spectacular.utils import extend_schema from pydantic import BaseModel, Field, field_validator from rest_framework.decorators import action -from rest_framework.exceptions import ValidationError +from rest_framework.exceptions import Throttled, ValidationError from rest_framework.response import Response from care.emr.api.viewsets.base import EMRBaseViewSet @@ -47,18 +49,33 @@ class OTPLoginView(EMRBaseViewSet): authentication_classes = [] permission_classes = [] + def failure_count(self, phone_number: str) -> int: + since = timezone.now() - timedelta(minutes=settings.OTP_LOCKOUT_MINUTES) + total = PatientMobileOTP.objects.filter( + phone_number=phone_number, + modified_date__gte=since, + failed_attempts__gt=0, + ).aggregate(total=Sum("failed_attempts"))["total"] + return total or 0 + @extend_schema( request=OTPLoginRequestSpec, ) @action(detail=False, methods=["POST"]) def send(self, request): data = OTPLoginRequestSpec(**request.data) + + if self.failure_count(data.phone_number) >= settings.OTP_MAX_FAILURES: + raise Throttled(detail="Too many failed login attempts. Try again later.") + sent_otps = PatientMobileOTP.objects.filter( - created_date__gte=(timezone.now() - timedelta(settings.OTP_REPEAT_WINDOW)), + created_date__gte=( + timezone.now() - timedelta(minutes=settings.OTP_SEND_WINDOW_MINUTES) + ), is_used=False, phone_number=data.phone_number, ) - if sent_otps.count() >= settings.OTP_MAX_REPEATS_WINDOW: + if sent_otps.count() >= settings.OTP_MAX_SENDS_PER_WINDOW: raise ValidationError({"phone_number": "Max Retries has exceeded"}) random_otp = "" if settings.USE_SMS: @@ -91,16 +108,36 @@ def send(self, request): @action(detail=False, methods=["POST"]) def login(self, request): data = OTPLoginSpec(**request.data) - otp_object = PatientMobileOTP.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() - - token = PatientToken() - token["phone_number"] = data.phone_number + if self.failure_count(data.phone_number) >= settings.OTP_MAX_FAILURES: + raise Throttled(detail="Too many failed login attempts. Try again later.") + + expired = False + with transaction.atomic(): + otp_object = ( + PatientMobileOTP.objects.select_for_update() + .filter(phone_number=data.phone_number, is_used=False) + .order_by("-created_date") + .first() + ) + + if otp_object: + if otp_object.otp == data.otp: + otp_object.is_used = True + otp_object.save(update_fields=["is_used", "modified_date"]) + token = PatientToken() + token["phone_number"] = data.phone_number + return Response({"access": str(token)}) + otp_object.failed_attempts += 1 + if otp_object.failed_attempts >= settings.OTP_MAX_VERIFY_ATTEMPTS: + otp_object.is_used = True + expired = True + otp_object.save( + update_fields=["failed_attempts", "is_used", "modified_date"] + ) - return Response({"access": str(token)}) + if expired: + raise ValidationError( + {"otp": "Too many wrong attempts. Please request a new OTP."} + ) + raise ValidationError({"otp": "Invalid OTP"}) diff --git a/care/emr/tests/test_otp_login.py b/care/emr/tests/test_otp_login.py new file mode 100644 index 0000000000..80c6984b34 --- /dev/null +++ b/care/emr/tests/test_otp_login.py @@ -0,0 +1,218 @@ +from datetime import timedelta + +from django.test import override_settings +from django.utils import timezone +from rest_framework import status +from rest_framework.test import APITestCase + +from care.facility.models.patient import PatientMobileOTP + + +@override_settings( + USE_SMS=False, + IS_PRODUCTION=False, + OTP_MAX_VERIFY_ATTEMPTS=3, + OTP_MAX_FAILURES=5, + OTP_LOCKOUT_MINUTES=30, + OTP_SEND_WINDOW_MINUTES=60, + OTP_MAX_SENDS_PER_WINDOW=10, +) +class OTPLoginFlowTests(APITestCase): + SEND_URL = "/api/v1/otp/send/" + LOGIN_URL = "/api/v1/otp/login/" + PHONE = "+919999999999" + DEV_OTP = "45612" # value hard-coded in send() for non-prod, non-SMS path + WRONG_OTP = "00000" + + def _send(self, phone=None): + return self.client.post( + self.SEND_URL, + {"phone_number": phone or self.PHONE}, + format="json", + ) + + def _login(self, otp, phone=None): + return self.client.post( + self.LOGIN_URL, + {"phone_number": phone or self.PHONE, "otp": otp}, + format="json", + ) + + # ------------------------------------------------------------------ happy path + + def test_send_creates_otp_row(self): + resp = self._send() + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertEqual( + PatientMobileOTP.objects.filter(phone_number=self.PHONE).count(), 1 + ) + + def test_login_with_correct_otp_returns_token(self): + self._send() + resp = self._login(self.DEV_OTP) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertIn("access", resp.data) + self.assertTrue(PatientMobileOTP.objects.get(phone_number=self.PHONE).is_used) + + def test_used_otp_cannot_be_reused(self): + self._send() + self.assertEqual(self._login(self.DEV_OTP).status_code, 200) + # second login with the same code must fail; row is is_used=True so no + # latest unused OTP exists -> "Invalid OTP", failure NOT counted + resp = self._login(self.DEV_OTP) + self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) + + # ------------------------------------------------------------------ per-OTP cap + + def test_third_wrong_attempt_invalidates_otp(self): + self._send() + self.assertEqual(self._login(self.WRONG_OTP).status_code, 400) + self.assertEqual(self._login(self.WRONG_OTP).status_code, 400) + + resp = self._login(self.WRONG_OTP) + self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn("request a new OTP", str(resp.data)) + + otp_row = PatientMobileOTP.objects.get(phone_number=self.PHONE) + self.assertTrue(otp_row.is_used) + self.assertEqual(otp_row.failed_attempts, 3) + + # even the correct code now fails — OTP is dead + resp = self._login(self.DEV_OTP) + self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) + self.assertNotIn("request a new OTP", str(resp.data)) + + def test_correct_otp_works_after_partial_failures(self): + self._send() + self._login(self.WRONG_OTP) + self._login(self.WRONG_OTP) + self.assertEqual(self._login(self.DEV_OTP).status_code, 200) + + def test_new_otp_works_after_previous_was_killed(self): + self._send() + for _ in range(3): + self._login(self.WRONG_OTP) + # OTP #1 killed, but daily failure count = 3 < 5 → can still request + self.assertEqual(self._send().status_code, 200) + self.assertEqual(self._login(self.DEV_OTP).status_code, 200) + + @override_settings(OTP_MAX_VERIFY_ATTEMPTS=1) + def test_setting_override_kills_otp_on_first_wrong_attempt(self): + self._send() + resp = self._login(self.WRONG_OTP) + self.assertEqual(resp.status_code, 400) + self.assertIn("request a new OTP", str(resp.data)) + + # ------------------------------------------------------------------ phone lockout + + def test_phone_locked_out_after_max_failures_blocks_login(self): + # 5 failures across two OTPs (3 + 2) + self._send() + for _ in range(3): + self._login(self.WRONG_OTP) + self._send() + for _ in range(2): + self._login(self.WRONG_OTP) + + # 6th attempt -> lockout + resp = self._login(self.WRONG_OTP) + self.assertEqual(resp.status_code, status.HTTP_429_TOO_MANY_REQUESTS) + + def test_phone_locked_out_blocks_send_too(self): + self._send() + for _ in range(3): + self._login(self.WRONG_OTP) + self._send() + for _ in range(2): + self._login(self.WRONG_OTP) + + resp = self._send() + self.assertEqual(resp.status_code, status.HTTP_429_TOO_MANY_REQUESTS) + + def test_lockout_does_not_affect_other_phone_numbers(self): + # lock out PHONE + self._send() + for _ in range(3): + self._login(self.WRONG_OTP) + self._send() + for _ in range(2): + self._login(self.WRONG_OTP) + + other = "+918888888888" + self.assertEqual(self._send(phone=other).status_code, 200) + self.assertEqual(self._login(self.DEV_OTP, phone=other).status_code, 200) + + def test_lockout_window_slides_over_time(self): + self._send() + for _ in range(3): + self._login(self.WRONG_OTP) + self._send() + for _ in range(2): + self._login(self.WRONG_OTP) + self.assertEqual(self._send().status_code, 429) + + # age all existing failures past the lockout window + old = timezone.now() - timedelta(minutes=31) + PatientMobileOTP.objects.filter(phone_number=self.PHONE).update( + modified_date=old + ) + + # window has slid; the phone is unlocked again + self.assertEqual(self._send().status_code, 200) + + # ------------------------------------------------------------------ send rate limit + + def test_send_rate_limit_enforced_within_window(self): + for _ in range(10): + self.assertEqual(self._send().status_code, 200) + resp = self._send() + self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) + + def test_send_rate_limit_window_excludes_old_otps(self): + for _ in range(10): + self._send() + # age them past the send window + old = timezone.now() - timedelta(minutes=61) + PatientMobileOTP.objects.filter(phone_number=self.PHONE).update( + created_date=old + ) + self.assertEqual(self._send().status_code, 200) + + # ------------------------------------------------------------------ multi-OTP behavior + + def test_only_latest_unused_otp_is_validated(self): + # First OTP issued and "leaked" to attacker (we know it's 45612 in dev) + self._send() + first_id = PatientMobileOTP.objects.latest("created_date").id + + # User requests a new OTP. Older one should no longer log in. + self._send() + + # Attacker tries the older known code against the latest row. + # In dev mode both rows have value "45612", so login *will* succeed + # against the latest — this just confirms we never read the older row. + resp = self._login(self.DEV_OTP) + self.assertEqual(resp.status_code, 200) + + # The older (first) OTP row was not touched by the login + first_row = PatientMobileOTP.objects.get(id=first_id) + self.assertFalse(first_row.is_used) + + def test_failure_bumps_only_latest_otp_row(self): + self._send() + first_id = PatientMobileOTP.objects.latest("created_date").id + self._send() + second_id = PatientMobileOTP.objects.latest("created_date").id + self.assertNotEqual(first_id, second_id) + + self._login(self.WRONG_OTP) + + self.assertEqual(PatientMobileOTP.objects.get(id=first_id).failed_attempts, 0) + self.assertEqual(PatientMobileOTP.objects.get(id=second_id).failed_attempts, 1) + + # ------------------------------------------------------------------ no-OTP edge + + def test_login_with_no_active_otp_returns_400_without_bumping(self): + resp = self._login(self.WRONG_OTP) + self.assertEqual(resp.status_code, 400) + self.assertEqual(PatientMobileOTP.objects.count(), 0) diff --git a/care/facility/migrations/0485_patientmobileotp_failed_attempts.py b/care/facility/migrations/0485_patientmobileotp_failed_attempts.py new file mode 100644 index 0000000000..ac12236c17 --- /dev/null +++ b/care/facility/migrations/0485_patientmobileotp_failed_attempts.py @@ -0,0 +1,17 @@ +# Generated by Django 6.0 on 2026-05-03 04:33 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("facility", "0484_remove_facility_discount_codes_and_more"), + ] + + operations = [ + migrations.AddField( + model_name="patientmobileotp", + name="failed_attempts", + field=models.PositiveSmallIntegerField(default=0), + ), + ] diff --git a/care/facility/models/patient.py b/care/facility/models/patient.py index b0af7e32e2..708a498b9f 100644 --- a/care/facility/models/patient.py +++ b/care/facility/models/patient.py @@ -10,3 +10,4 @@ class PatientMobileOTP(BaseModel): max_length=14, validators=[mobile_or_landline_number_validator] ) otp = models.CharField(max_length=10) + failed_attempts = models.PositiveSmallIntegerField(default=0) diff --git a/config/settings/base.py b/config/settings/base.py index 75d37c5c2d..6f35a537fd 100644 --- a/config/settings/base.py +++ b/config/settings/base.py @@ -502,10 +502,18 @@ # OTP # ------------------------------------------------------------------------------ -OTP_REPEAT_WINDOW = 6 # OTPs will only be valid for 6 hours to login -OTP_MAX_REPEATS_WINDOW = 10 # times OTPs can be sent within OTP_REPEAT_WINDOW OTP_LENGTH = 5 +OTP_SEND_WINDOW_MINUTES = env.int("OTP_SEND_WINDOW_MINUTES", default=60) +# max no. of otp that can be generated in (OTP_SEND_WINDOW_MINUTES)mins +OTP_MAX_SENDS_PER_WINDOW = env.int("OTP_MAX_SENDS_PER_WINDOW", default=10) +# no. of attempts after which the otp is blocked +OTP_MAX_VERIFY_ATTEMPTS = env.int("OTP_MAX_VERIFY_ATTEMPTS", default=3) +# max no. of attempts after which the phone number is blocked +OTP_MAX_FAILURES = env.int("OTP_MAX_FAILURES", default=5) +# minutes it stays lock when reached OTP_MAX_FAILURES +OTP_LOCKOUT_MINUTES = env.int("OTP_LOCKOUT_MINUTES", default=30) + # Rate Limiting # ------------------------------------------------------------------------------ DISABLE_RATELIMIT = env.bool("DISABLE_RATELIMIT", default=False) From 45b86b9e7200502b40797c5471e4038e721b2825 Mon Sep 17 00:00:00 2001 From: praffq Date: Mon, 4 May 2026 14:36:50 +0530 Subject: [PATCH 02/13] code rabbit review --- care/emr/api/otp_viewsets/login.py | 1 - 1 file changed, 1 deletion(-) diff --git a/care/emr/api/otp_viewsets/login.py b/care/emr/api/otp_viewsets/login.py index e03248beb1..4088623bab 100644 --- a/care/emr/api/otp_viewsets/login.py +++ b/care/emr/api/otp_viewsets/login.py @@ -72,7 +72,6 @@ def send(self, request): created_date__gte=( timezone.now() - timedelta(minutes=settings.OTP_SEND_WINDOW_MINUTES) ), - is_used=False, phone_number=data.phone_number, ) if sent_otps.count() >= settings.OTP_MAX_SENDS_PER_WINDOW: From c8eb2e32e13aa56942e100c9be7736b7b9cfa534 Mon Sep 17 00:00:00 2001 From: praffq Date: Wed, 6 May 2026 12:27:56 +0530 Subject: [PATCH 03/13] code rabbit review v2 --- care/emr/api/otp_viewsets/login.py | 71 +++++++++++-------- care/emr/locks/otp.py | 9 +++ care/emr/tests/test_otp_login.py | 39 +++++++--- .../0485_patientmobileotp_failed_attempts.py | 17 ----- ...tientmobileotp_failed_attempts_and_more.py | 26 +++++++ care/facility/models/patient.py | 12 ++++ 6 files changed, 118 insertions(+), 56 deletions(-) create mode 100644 care/emr/locks/otp.py delete mode 100644 care/facility/migrations/0485_patientmobileotp_failed_attempts.py create mode 100644 care/facility/migrations/0485_patientmobileotp_failed_attempts_and_more.py diff --git a/care/emr/api/otp_viewsets/login.py b/care/emr/api/otp_viewsets/login.py index 4088623bab..2d4facce26 100644 --- a/care/emr/api/otp_viewsets/login.py +++ b/care/emr/api/otp_viewsets/login.py @@ -14,6 +14,7 @@ from rest_framework.response import Response from care.emr.api.viewsets.base import EMRBaseViewSet +from care.emr.locks.otp import OTPSendLock from care.facility.models.patient import PatientMobileOTP from care.utils import sms from care.utils.models.validators import mobile_validator @@ -68,37 +69,45 @@ def send(self, request): if self.failure_count(data.phone_number) >= settings.OTP_MAX_FAILURES: raise Throttled(detail="Too many failed login attempts. Try again later.") - sent_otps = PatientMobileOTP.objects.filter( - created_date__gte=( - timezone.now() - timedelta(minutes=settings.OTP_SEND_WINDOW_MINUTES) - ), - phone_number=data.phone_number, - ) - if sent_otps.count() >= settings.OTP_MAX_SENDS_PER_WINDOW: - raise ValidationError({"phone_number": "Max Retries has exceeded"}) - random_otp = "" - if settings.USE_SMS: - random_otp = rand_pass(settings.OTP_LENGTH) - try: - content = get_sms_content( - settings.OTP_SMS_TEMPLATE_PATH, {"random_otp": random_otp} - ) - sms.send_text_message( - content=content, - recipients=[data.phone_number], - ) - except Exception as e: - logger.error(e) - return Response( - {"error": "Error while sending OTP. Contact admin."}, status=400 - ) - elif settings.IS_PRODUCTION: - random_otp = rand_pass(settings.OTP_LENGTH) - else: - random_otp = "45612" - - otp_obj = PatientMobileOTP(phone_number=data.phone_number, otp=random_otp) - otp_obj.save() + with OTPSendLock(data.phone_number): + sent_otps = PatientMobileOTP.objects.filter( + created_date__gte=( + timezone.now() - timedelta(minutes=settings.OTP_SEND_WINDOW_MINUTES) + ), + phone_number=data.phone_number, + ) + if sent_otps.count() >= settings.OTP_MAX_SENDS_PER_WINDOW: + raise ValidationError({"phone_number": "Max Retries has exceeded"}) + + random_otp = "" + if settings.USE_SMS: + random_otp = rand_pass(settings.OTP_LENGTH) + try: + content = get_sms_content( + settings.OTP_SMS_TEMPLATE_PATH, {"random_otp": random_otp} + ) + sms.send_text_message( + content=content, + recipients=[data.phone_number], + ) + except Exception as e: + logger.error(e) + return Response( + {"error": "Error while sending OTP. Contact admin."}, + status=400, + ) + elif settings.IS_PRODUCTION: + random_otp = rand_pass(settings.OTP_LENGTH) + else: + random_otp = "45612" + + # disable all other existing otp before creating a new one + PatientMobileOTP.objects.filter( + phone_number=data.phone_number, is_used=False + ).update(is_used=True) + PatientMobileOTP.objects.create( + phone_number=data.phone_number, otp=random_otp + ) return Response({"otp": "generated"}) @extend_schema( diff --git a/care/emr/locks/otp.py b/care/emr/locks/otp.py new file mode 100644 index 0000000000..dc5fbefe9c --- /dev/null +++ b/care/emr/locks/otp.py @@ -0,0 +1,9 @@ +from django.conf import settings + +from care.utils.lock import Lock + + +class OTPSendLock(Lock): + def __init__(self, phone_number, timeout=settings.LOCK_TIMEOUT): + self.key = f"lock:otp_send:{phone_number}" + self.timeout = timeout diff --git a/care/emr/tests/test_otp_login.py b/care/emr/tests/test_otp_login.py index 80c6984b34..62aa3178b0 100644 --- a/care/emr/tests/test_otp_login.py +++ b/care/emr/tests/test_otp_login.py @@ -178,25 +178,48 @@ def test_send_rate_limit_window_excludes_old_otps(self): ) self.assertEqual(self._send().status_code, 200) + def test_send_rate_limit_counts_used_otps(self): + """Send cap counts every OTP issued in the window, not just unused ones — + otherwise a client that consumes each OTP could bypass the cap entirely.""" + for _ in range(10): + self.assertEqual(self._send().status_code, 200) + # consume the OTP so only used rows remain + self._login(self.DEV_OTP) + resp = self._send() + self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) + # ------------------------------------------------------------------ multi-OTP behavior def test_only_latest_unused_otp_is_validated(self): - # First OTP issued and "leaked" to attacker (we know it's 45612 in dev) + # First OTP issued self._send() first_id = PatientMobileOTP.objects.latest("created_date").id - # User requests a new OTP. Older one should no longer log in. + # Issuing a new OTP must invalidate the older one (no two unused OTPs alive at once) self._send() + first_row = PatientMobileOTP.objects.get(id=first_id) + self.assertTrue(first_row.is_used) - # Attacker tries the older known code against the latest row. - # In dev mode both rows have value "45612", so login *will* succeed - # against the latest — this just confirms we never read the older row. + # Login uses only the newest unused row resp = self._login(self.DEV_OTP) self.assertEqual(resp.status_code, 200) - # The older (first) OTP row was not touched by the login - first_row = PatientMobileOTP.objects.get(id=first_id) - self.assertFalse(first_row.is_used) + def test_old_otp_cannot_login_after_new_one_issued(self): + """Even if an attacker knows an old OTP code, it must not work after a re-send.""" + self._send() + # User re-requests; old row becomes is_used=True + self._send() + # Consume the new OTP + self.assertEqual(self._login(self.DEV_OTP).status_code, 200) + # No unused OTP rows for this phone now + self.assertFalse( + PatientMobileOTP.objects.filter( + phone_number=self.PHONE, is_used=False + ).exists() + ) + # Replaying the (same-value) old code must fail — no unused row to match against + resp = self._login(self.DEV_OTP) + self.assertEqual(resp.status_code, 400) def test_failure_bumps_only_latest_otp_row(self): self._send() diff --git a/care/facility/migrations/0485_patientmobileotp_failed_attempts.py b/care/facility/migrations/0485_patientmobileotp_failed_attempts.py deleted file mode 100644 index ac12236c17..0000000000 --- a/care/facility/migrations/0485_patientmobileotp_failed_attempts.py +++ /dev/null @@ -1,17 +0,0 @@ -# Generated by Django 6.0 on 2026-05-03 04:33 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("facility", "0484_remove_facility_discount_codes_and_more"), - ] - - operations = [ - migrations.AddField( - model_name="patientmobileotp", - name="failed_attempts", - field=models.PositiveSmallIntegerField(default=0), - ), - ] diff --git a/care/facility/migrations/0485_patientmobileotp_failed_attempts_and_more.py b/care/facility/migrations/0485_patientmobileotp_failed_attempts_and_more.py new file mode 100644 index 0000000000..1dfed889db --- /dev/null +++ b/care/facility/migrations/0485_patientmobileotp_failed_attempts_and_more.py @@ -0,0 +1,26 @@ +# Generated by Django 6.0 on 2026-05-06 05:18 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('facility', '0484_remove_facility_discount_codes_and_more'), + ] + + operations = [ + migrations.AddField( + model_name='patientmobileotp', + name='failed_attempts', + field=models.PositiveSmallIntegerField(default=0), + ), + migrations.AddIndex( + model_name='patientmobileotp', + index=models.Index(fields=['phone_number', '-created_date'], name='pmo_phone_created_idx'), + ), + migrations.AddIndex( + model_name='patientmobileotp', + index=models.Index(fields=['phone_number', '-modified_date'], name='pmo_phone_modified_idx'), + ), + ] diff --git a/care/facility/models/patient.py b/care/facility/models/patient.py index 708a498b9f..234bd205ab 100644 --- a/care/facility/models/patient.py +++ b/care/facility/models/patient.py @@ -11,3 +11,15 @@ class PatientMobileOTP(BaseModel): ) otp = models.CharField(max_length=10) failed_attempts = models.PositiveSmallIntegerField(default=0) + + class Meta: + indexes = [ + models.Index( + fields=["phone_number", "-created_date"], + name="pmo_phone_created_idx", + ), + models.Index( + fields=["phone_number", "-modified_date"], + name="pmo_phone_modified_idx", + ), + ] From c77cf11bb0895a0ad13e88f3590ef6b9596450b0 Mon Sep 17 00:00:00 2001 From: praffq Date: Wed, 6 May 2026 22:53:53 +0530 Subject: [PATCH 04/13] reviews from aakash --- .env.example | 28 +++++++++++++++++ care/emr/api/otp_viewsets/login.py | 25 +++++++++------ care/emr/tasks/__init__.py | 7 +++++ care/emr/tasks/cleanup_expired_otps.py | 22 +++++++++++++ care/emr/tests/test_otp_login.py | 31 +++++++++++++++++-- ...tientmobileotp_failed_attempts_and_more.py | 6 ++-- care/facility/models/patient.py | 7 +++-- config/settings/base.py | 18 ++++++++--- 8 files changed, 121 insertions(+), 23 deletions(-) create mode 100644 care/emr/tasks/cleanup_expired_otps.py diff --git a/.env.example b/.env.example index 641baf4477..b797efba87 100644 --- a/.env.example +++ b/.env.example @@ -311,6 +311,34 @@ FACILITY_S3_BUCKET=facility-bucket # Default: "sms/otp_sms.txt" # OTP_SMS_TEMPLATE=sms/otp_sms.txt +# ============================================================================ +# OTP RATE LIMITING +# ============================================================================ + +# Time window (in minutes) for tracking OTP request limits +# Default: 60 +# OTP_SEND_WINDOW_MINUTES=60 + +# Maximum number of OTPs that can be generated within OTP_SEND_WINDOW_MINUTES +# Default: 10 +# OTP_MAX_SENDS_PER_WINDOW=10 + +# Number of failed verification attempts allowed before an OTP is invalidated +# Default: 3 +# OTP_MAX_VERIFY_ATTEMPTS=3 + +# Maximum total failures allowed before the phone number is restricted +# Default: 5 +# OTP_MAX_FAILURES=5 + +# Duration (in minutes) the account remains locked after reaching maximum failures +# Default: 60 +# OTP_LOCKOUT_MINUTES=60 + +# Duration (in minutes) an OTP remains valid for verification after it is generated +# Default: 10 +# OTP_VALIDITY_MINUTES=10 + # ============================================================================ # EMAIL TEMPLATES # ============================================================================ diff --git a/care/emr/api/otp_viewsets/login.py b/care/emr/api/otp_viewsets/login.py index 2d4facce26..5e70c58193 100644 --- a/care/emr/api/otp_viewsets/login.py +++ b/care/emr/api/otp_viewsets/login.py @@ -6,11 +6,10 @@ from django.conf import settings from django.db import transaction from django.db.models import Sum -from django.utils import timezone from drf_spectacular.utils import extend_schema from pydantic import BaseModel, Field, field_validator from rest_framework.decorators import action -from rest_framework.exceptions import Throttled, ValidationError +from rest_framework.exceptions import APIException, Throttled, ValidationError from rest_framework.response import Response from care.emr.api.viewsets.base import EMRBaseViewSet @@ -19,12 +18,13 @@ from care.utils import sms from care.utils.models.validators import mobile_validator from care.utils.sms.utils import get_sms_content +from care.utils.time_util import care_now from config.patient_otp_token import PatientToken logger = logging.getLogger(__name__) -def rand_pass(size): +def generate_otp(size): return "".join(secrets.choice(string.digits) for _ in range(size)) @@ -51,7 +51,7 @@ class OTPLoginView(EMRBaseViewSet): permission_classes = [] def failure_count(self, phone_number: str) -> int: - since = timezone.now() - timedelta(minutes=settings.OTP_LOCKOUT_MINUTES) + since = care_now() - timedelta(minutes=settings.OTP_LOCKOUT_MINUTES) total = PatientMobileOTP.objects.filter( phone_number=phone_number, modified_date__gte=since, @@ -72,7 +72,7 @@ def send(self, request): with OTPSendLock(data.phone_number): sent_otps = PatientMobileOTP.objects.filter( created_date__gte=( - timezone.now() - timedelta(minutes=settings.OTP_SEND_WINDOW_MINUTES) + care_now() - timedelta(minutes=settings.OTP_SEND_WINDOW_MINUTES) ), phone_number=data.phone_number, ) @@ -81,7 +81,7 @@ def send(self, request): random_otp = "" if settings.USE_SMS: - random_otp = rand_pass(settings.OTP_LENGTH) + random_otp = generate_otp(settings.OTP_LENGTH) try: content = get_sms_content( settings.OTP_SMS_TEMPLATE_PATH, {"random_otp": random_otp} @@ -96,10 +96,10 @@ def send(self, request): {"error": "Error while sending OTP. Contact admin."}, status=400, ) - elif settings.IS_PRODUCTION: - random_otp = rand_pass(settings.OTP_LENGTH) - else: + elif not settings.IS_PRODUCTION: random_otp = "45612" + else: + raise APIException("SMS Backend not configured") # disable all other existing otp before creating a new one PatientMobileOTP.objects.filter( @@ -124,7 +124,12 @@ def login(self, request): with transaction.atomic(): otp_object = ( PatientMobileOTP.objects.select_for_update() - .filter(phone_number=data.phone_number, is_used=False) + .filter( + phone_number=data.phone_number, + is_used=False, + created_date__gte=care_now() + - timedelta(minutes=settings.OTP_VALIDITY_MINUTES), + ) .order_by("-created_date") .first() ) diff --git a/care/emr/tasks/__init__.py b/care/emr/tasks/__init__.py index 49cc884de4..626996c0e2 100644 --- a/care/emr/tasks/__init__.py +++ b/care/emr/tasks/__init__.py @@ -2,6 +2,7 @@ from celery.schedules import crontab from django.conf import settings +from care.emr.tasks.cleanup_expired_otps import cleanup_expired_otps from care.emr.tasks.cleanup_expired_token_slots import cleanup_expired_token_slots from care.emr.tasks.cleanup_incomplete_file_uploads import ( cleanup_incomplete_file_uploads, @@ -16,6 +17,12 @@ def setup_periodic_tasks(sender: Celery, **kwargs): name="cleanup_expired_token_slots", ) + sender.add_periodic_task( + crontab(hour="0", minute="0"), + cleanup_expired_otps.s(), + name="cleanup_expired_otps", + ) + if cleanup_file_upload_hours := settings.FILE_UPLOAD_EXPIRY_HOURS: sender.add_periodic_task( cleanup_file_upload_hours * 3600, # convert hours to seconds diff --git a/care/emr/tasks/cleanup_expired_otps.py b/care/emr/tasks/cleanup_expired_otps.py new file mode 100644 index 0000000000..c630b9f122 --- /dev/null +++ b/care/emr/tasks/cleanup_expired_otps.py @@ -0,0 +1,22 @@ +from datetime import timedelta +from logging import Logger + +from celery import shared_task +from celery.utils.log import get_task_logger + +from care.facility.models.patient import PatientMobileOTP +from care.utils.time_util import care_now + +logger: Logger = get_task_logger(__name__) + + +@shared_task +def cleanup_expired_otps(): + """ + Soft-deletes PatientMobileOTP rows older than 24 hours + """ + cutoff = care_now() - timedelta(hours=24) + count = PatientMobileOTP.objects.filter(created_date__lt=cutoff).update( + deleted=True + ) + logger.info("Soft-deleted %d expired OTP rows", count) diff --git a/care/emr/tests/test_otp_login.py b/care/emr/tests/test_otp_login.py index 62aa3178b0..b99e8dd92a 100644 --- a/care/emr/tests/test_otp_login.py +++ b/care/emr/tests/test_otp_login.py @@ -1,11 +1,11 @@ from datetime import timedelta from django.test import override_settings -from django.utils import timezone from rest_framework import status from rest_framework.test import APITestCase from care.facility.models.patient import PatientMobileOTP +from care.utils.time_util import care_now @override_settings( @@ -16,6 +16,7 @@ OTP_LOCKOUT_MINUTES=30, OTP_SEND_WINDOW_MINUTES=60, OTP_MAX_SENDS_PER_WINDOW=10, + OTP_VALIDITY_MINUTES=10, ) class OTPLoginFlowTests(APITestCase): SEND_URL = "/api/v1/otp/send/" @@ -152,7 +153,7 @@ def test_lockout_window_slides_over_time(self): self.assertEqual(self._send().status_code, 429) # age all existing failures past the lockout window - old = timezone.now() - timedelta(minutes=31) + old = care_now() - timedelta(minutes=31) PatientMobileOTP.objects.filter(phone_number=self.PHONE).update( modified_date=old ) @@ -172,7 +173,7 @@ def test_send_rate_limit_window_excludes_old_otps(self): for _ in range(10): self._send() # age them past the send window - old = timezone.now() - timedelta(minutes=61) + old = care_now() - timedelta(minutes=61) PatientMobileOTP.objects.filter(phone_number=self.PHONE).update( created_date=old ) @@ -239,3 +240,27 @@ def test_login_with_no_active_otp_returns_400_without_bumping(self): resp = self._login(self.WRONG_OTP) self.assertEqual(resp.status_code, 400) self.assertEqual(PatientMobileOTP.objects.count(), 0) + + # ------------------------------------------------------------------ otp validity + + def test_expired_otp_cannot_login(self): + self._send() + # age the OTP past its validity window + old = care_now() - timedelta(minutes=11) + PatientMobileOTP.objects.filter(phone_number=self.PHONE).update( + created_date=old + ) + resp = self._login(self.DEV_OTP) + self.assertEqual(resp.status_code, 400) + # row remains unused since it was never picked up by the verify query + self.assertFalse(PatientMobileOTP.objects.get(phone_number=self.PHONE).is_used) + + def test_otp_within_validity_window_works(self): + self._send() + # age the OTP to just inside the validity window + recent = care_now() - timedelta(minutes=9) + PatientMobileOTP.objects.filter(phone_number=self.PHONE).update( + created_date=recent + ) + resp = self._login(self.DEV_OTP) + self.assertEqual(resp.status_code, 200) diff --git a/care/facility/migrations/0485_patientmobileotp_failed_attempts_and_more.py b/care/facility/migrations/0485_patientmobileotp_failed_attempts_and_more.py index 1dfed889db..1b6021f19e 100644 --- a/care/facility/migrations/0485_patientmobileotp_failed_attempts_and_more.py +++ b/care/facility/migrations/0485_patientmobileotp_failed_attempts_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 6.0 on 2026-05-06 05:18 +# Generated by Django 6.0 on 2026-05-06 13:12 from django.db import migrations, models @@ -17,10 +17,10 @@ class Migration(migrations.Migration): ), migrations.AddIndex( model_name='patientmobileotp', - index=models.Index(fields=['phone_number', '-created_date'], name='pmo_phone_created_idx'), + index=models.Index(condition=models.Q(('deleted', False)), fields=['phone_number', '-created_date'], name='pmo_phone_created_active_idx'), ), migrations.AddIndex( model_name='patientmobileotp', - index=models.Index(fields=['phone_number', '-modified_date'], name='pmo_phone_modified_idx'), + index=models.Index(condition=models.Q(('deleted', False)), fields=['phone_number', '-modified_date'], name='pmo_phone_modified_active_idx'), ), ] diff --git a/care/facility/models/patient.py b/care/facility/models/patient.py index 234bd205ab..4600827af3 100644 --- a/care/facility/models/patient.py +++ b/care/facility/models/patient.py @@ -1,4 +1,5 @@ from django.db import models +from django.db.models import Q from care.utils.models.base import BaseModel from care.utils.models.validators import mobile_or_landline_number_validator @@ -16,10 +17,12 @@ class Meta: indexes = [ models.Index( fields=["phone_number", "-created_date"], - name="pmo_phone_created_idx", + name="pmo_phone_created_active_idx", + condition=Q(deleted=False), ), models.Index( fields=["phone_number", "-modified_date"], - name="pmo_phone_modified_idx", + name="pmo_phone_modified_active_idx", + condition=Q(deleted=False), ), ] diff --git a/config/settings/base.py b/config/settings/base.py index 6f35a537fd..7f30f9b7f0 100644 --- a/config/settings/base.py +++ b/config/settings/base.py @@ -504,15 +504,23 @@ # ------------------------------------------------------------------------------ OTP_LENGTH = 5 +# The time window (in minutes) for tracking OTP request limits OTP_SEND_WINDOW_MINUTES = env.int("OTP_SEND_WINDOW_MINUTES", default=60) -# max no. of otp that can be generated in (OTP_SEND_WINDOW_MINUTES)mins + +# Maximum number of OTPs that can be generated within the window defined above OTP_MAX_SENDS_PER_WINDOW = env.int("OTP_MAX_SENDS_PER_WINDOW", default=10) -# no. of attempts after which the otp is blocked + +# Number of failed verification attempts allowed before an OTP is invalidated OTP_MAX_VERIFY_ATTEMPTS = env.int("OTP_MAX_VERIFY_ATTEMPTS", default=3) -# max no. of attempts after which the phone number is blocked + +# Maximum total failures allowed before the phone number is restricted OTP_MAX_FAILURES = env.int("OTP_MAX_FAILURES", default=5) -# minutes it stays lock when reached OTP_MAX_FAILURES -OTP_LOCKOUT_MINUTES = env.int("OTP_LOCKOUT_MINUTES", default=30) + +# Duration (in minutes) the account remains locked after reaching maximum failures +OTP_LOCKOUT_MINUTES = env.int("OTP_LOCKOUT_MINUTES", default=60) + +# Duration (in minutes) an OTP remains valid for verification after it is generated +OTP_VALIDITY_MINUTES = env.int("OTP_VALIDITY_MINUTES", default=10) # Rate Limiting # ------------------------------------------------------------------------------ From 3fe211c0696798e0c0beb626b332b99be1ae3c6e Mon Sep 17 00:00:00 2001 From: praffq Date: Wed, 6 May 2026 23:26:48 +0530 Subject: [PATCH 05/13] updated clean up time cutoff --- care/emr/tasks/cleanup_expired_otps.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/care/emr/tasks/cleanup_expired_otps.py b/care/emr/tasks/cleanup_expired_otps.py index c630b9f122..25f781410a 100644 --- a/care/emr/tasks/cleanup_expired_otps.py +++ b/care/emr/tasks/cleanup_expired_otps.py @@ -3,6 +3,7 @@ from celery import shared_task from celery.utils.log import get_task_logger +from django.conf import settings from care.facility.models.patient import PatientMobileOTP from care.utils.time_util import care_now @@ -13,9 +14,9 @@ @shared_task def cleanup_expired_otps(): """ - Soft-deletes PatientMobileOTP rows older than 24 hours + Soft-deletes PatientMobileOTP rows older than the lockout window """ - cutoff = care_now() - timedelta(hours=24) + cutoff = care_now() - timedelta(minutes=settings.OTP_LOCKOUT_MINUTES) count = PatientMobileOTP.objects.filter(created_date__lt=cutoff).update( deleted=True ) From b497806047ef9593a83698741e1c8f782d61aeb9 Mon Sep 17 00:00:00 2001 From: praffq Date: Mon, 8 Jun 2026 02:02:31 +0530 Subject: [PATCH 06/13] clean up --- docker-compose.local.yaml | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/docker-compose.local.yaml b/docker-compose.local.yaml index 621b2b1d46..ba3d56a444 100644 --- a/docker-compose.local.yaml +++ b/docker-compose.local.yaml @@ -8,17 +8,9 @@ services: ADDITIONAL_PLUGS: ${ADDITIONAL_PLUGS:-} env_file: - ./docker/.local.env - environment: - ADDITIONAL_PLUGS: '[{"name":"booking_notifications","package_name":"booking_notifications","version":""}]' volumes: - .:/app - - ../care_booking_notifications_be:/plugs/care_booking_notifications_be - entrypoint: - - bash - - -c - - | - pip install -e /plugs/care_booking_notifications_be --quiet - exec bash scripts/start-dev.sh + entrypoint: ["bash", "scripts/start-dev.sh"] ports: - "9000:9000" - "9876:9876" #debugpy @@ -35,18 +27,10 @@ services: image: care_local env_file: - ./docker/.local.env - environment: - ADDITIONAL_PLUGS: '[{"name":"booking_notifications","package_name":"booking_notifications","version":""}]' - volumes: - - .:/app - - ../care_booking_notifications_be:/plugs/care_booking_notifications_be - entrypoint: - - bash - - -c - - | - pip install -e /plugs/care_booking_notifications_be --quiet - exec bash scripts/celery-dev.sh + entrypoint: ["bash", "scripts/celery-dev.sh"] restart: unless-stopped depends_on: - db - redis + volumes: + - .:/app From fbd837ed6a6f1ee5800dff992b59ab8bfab6cd60 Mon Sep 17 00:00:00 2001 From: praffq Date: Mon, 8 Jun 2026 02:13:03 +0530 Subject: [PATCH 07/13] migation ordering --- ...leotp.py => 0485_rename_patientmobileotp_mobileotp.py} | 2 +- ...more.py => 0486_mobileotp_failed_attempts_and_more.py} | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) rename care/facility/migrations/{0486_rename_patientmobileotp_mobileotp.py => 0485_rename_patientmobileotp_mobileotp.py} (80%) rename care/facility/migrations/{0485_patientmobileotp_failed_attempts_and_more.py => 0486_mobileotp_failed_attempts_and_more.py} (78%) diff --git a/care/facility/migrations/0486_rename_patientmobileotp_mobileotp.py b/care/facility/migrations/0485_rename_patientmobileotp_mobileotp.py similarity index 80% rename from care/facility/migrations/0486_rename_patientmobileotp_mobileotp.py rename to care/facility/migrations/0485_rename_patientmobileotp_mobileotp.py index 4fc2ade3ff..29400ff759 100644 --- a/care/facility/migrations/0486_rename_patientmobileotp_mobileotp.py +++ b/care/facility/migrations/0485_rename_patientmobileotp_mobileotp.py @@ -6,7 +6,7 @@ class Migration(migrations.Migration): dependencies = [ - ('facility', '0485_patientmobileotp_failed_attempts_and_more'), + ('facility', '0484_remove_facility_discount_codes_and_more'), ] operations = [ diff --git a/care/facility/migrations/0485_patientmobileotp_failed_attempts_and_more.py b/care/facility/migrations/0486_mobileotp_failed_attempts_and_more.py similarity index 78% rename from care/facility/migrations/0485_patientmobileotp_failed_attempts_and_more.py rename to care/facility/migrations/0486_mobileotp_failed_attempts_and_more.py index 1b6021f19e..f6fa85079c 100644 --- a/care/facility/migrations/0485_patientmobileotp_failed_attempts_and_more.py +++ b/care/facility/migrations/0486_mobileotp_failed_attempts_and_more.py @@ -6,21 +6,21 @@ class Migration(migrations.Migration): dependencies = [ - ('facility', '0484_remove_facility_discount_codes_and_more'), + ('facility', '0485_rename_patientmobileotp_mobileotp'), ] operations = [ migrations.AddField( - model_name='patientmobileotp', + model_name='mobileotp', name='failed_attempts', field=models.PositiveSmallIntegerField(default=0), ), migrations.AddIndex( - model_name='patientmobileotp', + model_name='mobileotp', index=models.Index(condition=models.Q(('deleted', False)), fields=['phone_number', '-created_date'], name='pmo_phone_created_active_idx'), ), migrations.AddIndex( - model_name='patientmobileotp', + model_name='mobileotp', index=models.Index(condition=models.Q(('deleted', False)), fields=['phone_number', '-modified_date'], name='pmo_phone_modified_active_idx'), ), ] From 62b719ad0b3be42766cde160bcd2bc2877b1a8a8 Mon Sep 17 00:00:00 2001 From: praffq Date: Mon, 8 Jun 2026 02:30:43 +0530 Subject: [PATCH 08/13] code review --- care/emr/api/otp_viewsets/login.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/care/emr/api/otp_viewsets/login.py b/care/emr/api/otp_viewsets/login.py index 4b6c16da1d..8c6446667c 100644 --- a/care/emr/api/otp_viewsets/login.py +++ b/care/emr/api/otp_viewsets/login.py @@ -42,7 +42,9 @@ def render_content(cls, otp: str) -> str: def send_otp(phone_number, otp_type: BaseOTPType): 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, ) From 4a5fafc9d9bc51913e2ed1a10d189a50d040372d Mon Sep 17 00:00:00 2001 From: praffq Date: Tue, 9 Jun 2026 01:11:00 +0530 Subject: [PATCH 09/13] making use of helper functions --- care/emr/api/otp_viewsets/login.py | 142 ++++++++---------- care/emr/tests/test_otp_reset_password_api.py | 3 +- care/users/api/otp_viewset/reset_password.py | 15 +- 3 files changed, 75 insertions(+), 85 deletions(-) diff --git a/care/emr/api/otp_viewsets/login.py b/care/emr/api/otp_viewsets/login.py index 8c6446667c..d85b7f492e 100644 --- a/care/emr/api/otp_viewsets/login.py +++ b/care/emr/api/otp_viewsets/login.py @@ -6,7 +6,6 @@ from django.conf import settings from django.db import transaction from django.db.models import Sum -from django.utils import timezone from drf_spectacular.utils import extend_schema from pydantic import BaseModel, Field, field_validator from rest_framework.decorators import action @@ -33,41 +32,27 @@ class BaseOTPType: def render_content(cls, otp: str) -> str: raise NotImplementedError + @classmethod + def send_window(cls) -> timedelta: + raise NotImplementedError + + @classmethod + def max_sends(cls) -> int: + raise NotImplementedError + class LoginOTP(BaseOTPType): @classmethod def render_content(cls, otp: str) -> str: return settings.OTP_SMS_LOGIN_CONTENT.format(otp=otp) + @classmethod + def send_window(cls) -> timedelta: + return timedelta(minutes=settings.OTP_SEND_WINDOW_MINUTES) -def send_otp(phone_number, otp_type: BaseOTPType): - sent_otps = MobileOTP.objects.filter( - created_date__gte=( - timezone.now() - timedelta(hours=settings.OTP_REPEAT_WINDOW) - ), - is_used=False, - phone_number=phone_number, - ) - if sent_otps.count() >= settings.OTP_MAX_REPEATS_WINDOW: - raise ValueError("Max Retries has exceeded") - - random_otp = "" - if settings.USE_SMS: - random_otp = generate_otp(settings.OTP_LENGTH) - try: - content = otp_type.render_content(random_otp) - sms.send_text_message( - content=content, - recipients=[phone_number], - ) - except Exception as e: - raise Exception("Error while sending OTP. Contact admin.") from e - elif settings.IS_PRODUCTION: - random_otp = generate_otp(settings.OTP_LENGTH) - else: - random_otp = "45612" - - MobileOTP.objects.create(phone_number=phone_number, otp=random_otp) + @classmethod + def max_sends(cls) -> int: + return settings.OTP_MAX_SENDS_PER_WINDOW class OTPRequestBaseSpec(BaseModel): @@ -88,64 +73,65 @@ class OTPLoginSpec(OTPRequestBaseSpec): otp: str = Field(min_length=settings.OTP_LENGTH, max_length=settings.OTP_LENGTH) +def failure_count(phone_number: str) -> int: + since = care_now() - timedelta(minutes=settings.OTP_LOCKOUT_MINUTES) + total = MobileOTP.objects.filter( + phone_number=phone_number, + modified_date__gte=since, + failed_attempts__gt=0, + ).aggregate(total=Sum("failed_attempts"))["total"] + return total or 0 + + +def send_otp(phone_number, otp_type: type[BaseOTPType]): + if failure_count(phone_number) >= settings.OTP_MAX_FAILURES: + raise Throttled(detail="Too many failed login attempts. Try again later.") + + with OTPSendLock(phone_number): + sent_otps = MobileOTP.objects.filter( + created_date__gte=care_now() - otp_type.send_window(), + phone_number=phone_number, + ) + if sent_otps.count() >= otp_type.max_sends(): + raise ValidationError({"phone_number": "Max Retries has exceeded"}) + + random_otp = "" + if settings.USE_SMS: + random_otp = generate_otp(settings.OTP_LENGTH) + try: + content = otp_type.render_content(random_otp) + sms.send_text_message( + content=content, + recipients=[phone_number], + ) + except Exception as e: + logger.error(e) + raise ValidationError( + {"error": "Error while sending OTP. Contact admin."} + ) from e + elif not settings.IS_PRODUCTION: + random_otp = "45612" + else: + raise APIException("SMS Backend not configured") + + # disable all other existing otp before creating a new one + MobileOTP.objects.filter(phone_number=phone_number, is_used=False).update( + is_used=True + ) + MobileOTP.objects.create(phone_number=phone_number, otp=random_otp) + + class OTPLoginView(EMRBaseViewSet): authentication_classes = [] permission_classes = [] - def failure_count(self, phone_number: str) -> int: - since = care_now() - timedelta(minutes=settings.OTP_LOCKOUT_MINUTES) - total = MobileOTP.objects.filter( - phone_number=phone_number, - modified_date__gte=since, - failed_attempts__gt=0, - ).aggregate(total=Sum("failed_attempts"))["total"] - return total or 0 - @extend_schema( request=OTPRequestBaseSpec, ) @action(detail=False, methods=["POST"]) def send(self, request): data = OTPRequestBaseSpec(**request.data) - - if self.failure_count(data.phone_number) >= settings.OTP_MAX_FAILURES: - raise Throttled(detail="Too many failed login attempts. Try again later.") - - with OTPSendLock(data.phone_number): - sent_otps = MobileOTP.objects.filter( - created_date__gte=( - care_now() - timedelta(minutes=settings.OTP_SEND_WINDOW_MINUTES) - ), - phone_number=data.phone_number, - ) - if sent_otps.count() >= settings.OTP_MAX_SENDS_PER_WINDOW: - raise ValidationError({"phone_number": "Max Retries has exceeded"}) - - random_otp = "" - if settings.USE_SMS: - random_otp = generate_otp(settings.OTP_LENGTH) - try: - content = LoginOTP.render_content(random_otp) - sms.send_text_message( - content=content, - recipients=[data.phone_number], - ) - except Exception as e: - logger.error(e) - return Response( - {"error": "Error while sending OTP. Contact admin."}, - status=400, - ) - elif not settings.IS_PRODUCTION: - random_otp = "45612" - else: - raise APIException("SMS Backend not configured") - - # disable all other existing otp before creating a new one - MobileOTP.objects.filter( - phone_number=data.phone_number, is_used=False - ).update(is_used=True) - MobileOTP.objects.create(phone_number=data.phone_number, otp=random_otp) + send_otp(data.phone_number, otp_type=LoginOTP) return Response({"otp": "generated"}) @extend_schema( @@ -155,7 +141,7 @@ def send(self, request): def login(self, request): data = OTPLoginSpec(**request.data) - if self.failure_count(data.phone_number) >= settings.OTP_MAX_FAILURES: + if failure_count(data.phone_number) >= settings.OTP_MAX_FAILURES: raise Throttled(detail="Too many failed login attempts. Try again later.") expired = False diff --git a/care/emr/tests/test_otp_reset_password_api.py b/care/emr/tests/test_otp_reset_password_api.py index 8616f45a54..1d4e544814 100644 --- a/care/emr/tests/test_otp_reset_password_api.py +++ b/care/emr/tests/test_otp_reset_password_api.py @@ -82,7 +82,8 @@ def test_send_opt_request_exceeding_limit(self): response = self._send() self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) self.assertIn( - "Unable to send OTP", response.data["errors"][0]["msg"]["phone_number"] + "Max Retries has exceeded", + response.data["errors"][0]["msg"]["phone_number"], ) def test_confirm_otp_with_valid_otp(self): diff --git a/care/users/api/otp_viewset/reset_password.py b/care/users/api/otp_viewset/reset_password.py index 7acc03badf..dae05cbf20 100644 --- a/care/users/api/otp_viewset/reset_password.py +++ b/care/users/api/otp_viewset/reset_password.py @@ -27,6 +27,14 @@ class ResetPasswordOTP(BaseOTPType): def render_content(cls, otp: str) -> str: return settings.OTP_SMS_RESET_PASSWORD_CONTENT.format(otp=otp) + @classmethod + def send_window(cls) -> timedelta: + return timedelta(hours=settings.OTP_REPEAT_WINDOW) + + @classmethod + def max_sends(cls) -> int: + return settings.OTP_MAX_REPEATS_WINDOW + class OTPResetConfirmSpec(OTPRequestBaseSpec): otp: str = Field(min_length=settings.OTP_LENGTH, max_length=settings.OTP_LENGTH) @@ -46,12 +54,7 @@ def send(self, request): if not User.objects.filter(phone_number=data.phone_number).exists(): return Response({"otp": "generated"}) - try: - send_otp(data.phone_number, otp_type=ResetPasswordOTP) - except ValueError as e: - raise ValidationError({"phone_number": "Unable to send OTP"}) from e - except Exception: - return Response({"error": "Unable to send OTP"}, status=400) + send_otp(data.phone_number, otp_type=ResetPasswordOTP) return Response({"otp": "generated"}) @action(detail=False, methods=["POST"]) From f21c324b7db69f59160ced4077b3e0d6f709ff12 Mon Sep 17 00:00:00 2001 From: praffq Date: Tue, 16 Jun 2026 11:09:08 +0530 Subject: [PATCH 10/13] reviews --- care/emr/api/otp_viewsets/login.py | 8 +++----- care/emr/locks/otp.py | 6 ++++++ care/emr/tasks/cleanup_expired_otps.py | 6 +++--- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/care/emr/api/otp_viewsets/login.py b/care/emr/api/otp_viewsets/login.py index d85b7f492e..c721fd4f6c 100644 --- a/care/emr/api/otp_viewsets/login.py +++ b/care/emr/api/otp_viewsets/login.py @@ -4,7 +4,6 @@ from datetime import timedelta from django.conf import settings -from django.db import transaction from django.db.models import Sum from drf_spectacular.utils import extend_schema from pydantic import BaseModel, Field, field_validator @@ -13,7 +12,7 @@ from rest_framework.response import Response from care.emr.api.viewsets.base import EMRBaseViewSet -from care.emr.locks.otp import OTPSendLock +from care.emr.locks.otp import OTPSendLock, OTPVerifyLock from care.facility.models.patient import MobileOTP from care.utils import sms from care.utils.models.validators import mobile_validator @@ -145,10 +144,9 @@ def login(self, request): raise Throttled(detail="Too many failed login attempts. Try again later.") expired = False - with transaction.atomic(): + with OTPVerifyLock(data.phone_number): otp_object = ( - MobileOTP.objects.select_for_update() - .filter( + MobileOTP.objects.filter( phone_number=data.phone_number, is_used=False, created_date__gte=care_now() diff --git a/care/emr/locks/otp.py b/care/emr/locks/otp.py index dc5fbefe9c..5f216abe3c 100644 --- a/care/emr/locks/otp.py +++ b/care/emr/locks/otp.py @@ -7,3 +7,9 @@ class OTPSendLock(Lock): def __init__(self, phone_number, timeout=settings.LOCK_TIMEOUT): self.key = f"lock:otp_send:{phone_number}" self.timeout = timeout + + +class OTPVerifyLock(Lock): + def __init__(self, phone_number, timeout=settings.LOCK_TIMEOUT): + self.key = f"lock:otp_verify:{phone_number}" + self.timeout = timeout diff --git a/care/emr/tasks/cleanup_expired_otps.py b/care/emr/tasks/cleanup_expired_otps.py index b229280265..f2a89463b6 100644 --- a/care/emr/tasks/cleanup_expired_otps.py +++ b/care/emr/tasks/cleanup_expired_otps.py @@ -14,8 +14,8 @@ @shared_task def cleanup_expired_otps(): """ - Soft-deletes MobileOTP rows older than the lockout window + Hard-deletes MobileOTP rows older than the lockout window """ cutoff = care_now() - timedelta(minutes=settings.OTP_LOCKOUT_MINUTES) - count = MobileOTP.objects.filter(created_date__lt=cutoff).update(deleted=True) - logger.info("Soft-deleted %d expired OTP rows", count) + count, _ = MobileOTP.objects.filter(created_date__lt=cutoff).delete() + logger.info("Deleted %d expired OTP rows", count) From 93d844c3bcaa422d2dcabc34bd455e3338009440 Mon Sep 17 00:00:00 2001 From: praffq Date: Tue, 16 Jun 2026 11:28:18 +0530 Subject: [PATCH 11/13] greptile reviews --- care/emr/api/otp_viewsets/login.py | 14 ++++++++------ care/emr/tasks/cleanup_expired_otps.py | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/care/emr/api/otp_viewsets/login.py b/care/emr/api/otp_viewsets/login.py index c721fd4f6c..fdc6d4ee0d 100644 --- a/care/emr/api/otp_viewsets/login.py +++ b/care/emr/api/otp_viewsets/login.py @@ -83,10 +83,10 @@ def failure_count(phone_number: str) -> int: def send_otp(phone_number, otp_type: type[BaseOTPType]): - if failure_count(phone_number) >= settings.OTP_MAX_FAILURES: - raise Throttled(detail="Too many failed login attempts. Try again later.") - with OTPSendLock(phone_number): + if failure_count(phone_number) >= settings.OTP_MAX_FAILURES: + raise Throttled(detail="Too many failed login attempts. Try again later.") + sent_otps = MobileOTP.objects.filter( created_date__gte=care_now() - otp_type.send_window(), phone_number=phone_number, @@ -140,11 +140,13 @@ def send(self, request): def login(self, request): data = OTPLoginSpec(**request.data) - if failure_count(data.phone_number) >= settings.OTP_MAX_FAILURES: - raise Throttled(detail="Too many failed login attempts. Try again later.") - expired = False with OTPVerifyLock(data.phone_number): + if failure_count(data.phone_number) >= settings.OTP_MAX_FAILURES: + raise Throttled( + detail="Too many failed login attempts. Try again later." + ) + otp_object = ( MobileOTP.objects.filter( phone_number=data.phone_number, diff --git a/care/emr/tasks/cleanup_expired_otps.py b/care/emr/tasks/cleanup_expired_otps.py index f2a89463b6..9205ab258c 100644 --- a/care/emr/tasks/cleanup_expired_otps.py +++ b/care/emr/tasks/cleanup_expired_otps.py @@ -17,5 +17,5 @@ def cleanup_expired_otps(): Hard-deletes MobileOTP rows older than the lockout window """ cutoff = care_now() - timedelta(minutes=settings.OTP_LOCKOUT_MINUTES) - count, _ = MobileOTP.objects.filter(created_date__lt=cutoff).delete() + count, _ = MobileOTP.objects.filter(modified_date__lt=cutoff).delete() logger.info("Deleted %d expired OTP rows", count) From 451a508034ab1b461c4833351979c90b9417ac64 Mon Sep 17 00:00:00 2001 From: praffq Date: Thu, 18 Jun 2026 02:47:17 +0530 Subject: [PATCH 12/13] nandkishor reviews --- .env.example | 4 ++++ care/emr/api/otp_viewsets/login.py | 13 ++++++------- config/settings/deployment.py | 1 + 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.env.example b/.env.example index b797efba87..93f2546878 100644 --- a/.env.example +++ b/.env.example @@ -291,6 +291,10 @@ FACILITY_S3_BUCKET=facility-bucket # Default: False (True in staging/production) # USE_SMS=False +# SMS backend dotted path (string) +# Default: "care.utils.sms.backend.sns.SnsBackend" +# SMS_BACKEND=care.utils.sms.backend.sns.SnsBackend + # AWS SNS access key for SMS (string) # Default: "" # SNS_ACCESS_KEY= diff --git a/care/emr/api/otp_viewsets/login.py b/care/emr/api/otp_viewsets/login.py index fdc6d4ee0d..3174a18f11 100644 --- a/care/emr/api/otp_viewsets/login.py +++ b/care/emr/api/otp_viewsets/login.py @@ -94,11 +94,12 @@ def send_otp(phone_number, otp_type: type[BaseOTPType]): if sent_otps.count() >= otp_type.max_sends(): raise ValidationError({"phone_number": "Max Retries has exceeded"}) - random_otp = "" + otp_value = ( + generate_otp(settings.OTP_LENGTH) if settings.IS_PRODUCTION else "45612" + ) if settings.USE_SMS: - random_otp = generate_otp(settings.OTP_LENGTH) try: - content = otp_type.render_content(random_otp) + content = otp_type.render_content(otp_value) sms.send_text_message( content=content, recipients=[phone_number], @@ -108,16 +109,14 @@ def send_otp(phone_number, otp_type: type[BaseOTPType]): raise ValidationError( {"error": "Error while sending OTP. Contact admin."} ) from e - elif not settings.IS_PRODUCTION: - random_otp = "45612" - else: + elif settings.IS_PRODUCTION: raise APIException("SMS Backend not configured") # disable all other existing otp before creating a new one MobileOTP.objects.filter(phone_number=phone_number, is_used=False).update( is_used=True ) - MobileOTP.objects.create(phone_number=phone_number, otp=random_otp) + MobileOTP.objects.create(phone_number=phone_number, otp=otp_value) class OTPLoginView(EMRBaseViewSet): diff --git a/config/settings/deployment.py b/config/settings/deployment.py index 855adc8801..e75ce6d31a 100644 --- a/config/settings/deployment.py +++ b/config/settings/deployment.py @@ -120,6 +120,7 @@ ignore_logger("django.security.DisallowedHost") # SMS API KEYS +SMS_BACKEND = env("SMS_BACKEND", default="care.utils.sms.backend.sns.SnsBackend") SNS_ACCESS_KEY = env("SNS_ACCESS_KEY", default="") SNS_SECRET_KEY = env("SNS_SECRET_KEY", default="") SNS_REGION = env("SNS_REGION", default="ap-south-1") From 0a01e9769524a0d0a3cfedc2e168830525a24d47 Mon Sep 17 00:00:00 2001 From: praffq Date: Mon, 22 Jun 2026 00:30:22 +0530 Subject: [PATCH 13/13] adding validation to reset password --- care/emr/tests/test_otp_reset_password_api.py | 43 ++++++++++++++ care/users/api/otp_viewset/reset_password.py | 56 ++++++++++++++----- 2 files changed, 84 insertions(+), 15 deletions(-) diff --git a/care/emr/tests/test_otp_reset_password_api.py b/care/emr/tests/test_otp_reset_password_api.py index 1d4e544814..806c54765b 100644 --- a/care/emr/tests/test_otp_reset_password_api.py +++ b/care/emr/tests/test_otp_reset_password_api.py @@ -170,3 +170,46 @@ def test_confirm_otp_with_multiple_users_linked_to_phone_number_and_with_invalid "No User with this username linked to this phone number", response.data["errors"][0]["msg"]["error"], ) + + @override_settings(OTP_MAX_VERIFY_ATTEMPTS=2) + def test_confirm_otp_attempts_are_capped(self): + """ + Reset confirm shares login's brute-force protection: after + OTP_MAX_VERIFY_ATTEMPTS wrong guesses the OTP is burned and even the + correct OTP is rejected afterwards. + """ + self._send() + first = self._confirm(otp="00000") + self.assertEqual(first.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(str(first.data["errors"][0]["msg"]["otp"]), "Invalid OTP") + + capped = self._confirm(otp="00001") + self.assertEqual(capped.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn( + "Too many wrong attempts", str(capped.data["errors"][0]["msg"]["otp"]) + ) + + # OTP is now burned, so the correct value no longer works. + correct = self._confirm(otp=self.otp) + self.assertEqual(correct.status_code, status.HTTP_400_BAD_REQUEST) + self.user.refresh_from_db() + self.assertFalse(self.user.check_password(self.new_password)) + + def test_confirm_otp_survives_multiple_user_conflict_for_retry(self): + """ + A 409 for multiple users must not consume the OTP: the same OTP can be + retried with a username. + """ + self.create_user( + username="testuser2", + phone_number=self.phone_number, + password="AnotherPass123!", + ) + self._send() + conflict = self._confirm(otp=self.otp) + self.assertEqual(conflict.status_code, status.HTTP_409_CONFLICT) + + retry = self._confirm(otp=self.otp, username="testuser") + self.assertEqual(retry.status_code, status.HTTP_200_OK) + self.user.refresh_from_db() + self.assertTrue(self.user.check_password(self.new_password)) diff --git a/care/users/api/otp_viewset/reset_password.py b/care/users/api/otp_viewset/reset_password.py index dae05cbf20..2e28d5b773 100644 --- a/care/users/api/otp_viewset/reset_password.py +++ b/care/users/api/otp_viewset/reset_password.py @@ -5,17 +5,23 @@ get_password_validators, validate_password, ) -from django.utils import timezone from drf_spectacular.utils import extend_schema from pydantic import Field from rest_framework.decorators import action -from rest_framework.exceptions import ValidationError +from rest_framework.exceptions import Throttled, ValidationError from rest_framework.response import Response -from care.emr.api.otp_viewsets.login import BaseOTPType, OTPRequestBaseSpec, send_otp +from care.emr.api.otp_viewsets.login import ( + BaseOTPType, + OTPRequestBaseSpec, + failure_count, + send_otp, +) from care.emr.api.viewsets.base import EMRBaseViewSet +from care.emr.locks.otp import OTPVerifyLock from care.facility.models.patient import MobileOTP from care.users.models import User +from care.utils.time_util import care_now class OTPResetSendSpec(OTPRequestBaseSpec): @@ -61,19 +67,37 @@ def send(self, request): @extend_schema(request=OTPResetConfirmSpec) def confirm(self, request): data = OTPResetConfirmSpec(**request.data) - otp_obj = ( - MobileOTP.objects.filter( - phone_number=data.phone_number, - is_used=False, - created_date__gte=( - timezone.now() - timedelta(hours=settings.OTP_REPEAT_WINDOW) - ), + + expired = False + with OTPVerifyLock(data.phone_number): + if failure_count(data.phone_number) >= settings.OTP_MAX_FAILURES: + raise Throttled(detail="Too many failed attempts. Try again later.") + + otp_obj = ( + MobileOTP.objects.filter( + phone_number=data.phone_number, + is_used=False, + created_date__gte=care_now() + - timedelta(hours=settings.OTP_REPEAT_WINDOW), + ) + .order_by("-created_date") + .first() ) - .order_by("-created_date") - .first() - ) - if not otp_obj or otp_obj.otp != data.otp: - raise ValidationError({"otp": "Invalid OTP"}) + + if not otp_obj or otp_obj.otp != data.otp: + if otp_obj: + otp_obj.failed_attempts += 1 + if otp_obj.failed_attempts >= settings.OTP_MAX_VERIFY_ATTEMPTS: + otp_obj.is_used = True + expired = True + otp_obj.save( + update_fields=["failed_attempts", "is_used", "modified_date"] + ) + if expired: + raise ValidationError( + {"otp": "Too many wrong attempts. Please request a new OTP."} + ) + raise ValidationError({"otp": "Invalid OTP"}) users = User.objects.filter(phone_number=data.phone_number) user_count = users.count() @@ -92,6 +116,8 @@ def confirm(self, request): status=409, ) user = users.first() + if user is None: + raise ValidationError({"otp": "Invalid OTP"}) validate_password( data.password,