From ae4dbab3cc297d0ee5d117e7abace3e142bc9e27 Mon Sep 17 00:00:00 2001 From: javad Date: Fri, 9 Oct 2026 10:32:27 +0330 Subject: [PATCH] fix bug sms --- apps/jwt_auth/services.py | 13 ++++++-- apps/jwt_auth/test_customer_login.py | 50 +++++++++++++++++++++++++++- apps/jwt_auth/test_regressions.py | 2 +- apps/jwt_auth/views.py | 6 ++-- utils/services/sms.py | 35 ++++++++++++++++--- 5 files changed, 93 insertions(+), 13 deletions(-) diff --git a/apps/jwt_auth/services.py b/apps/jwt_auth/services.py index 6a83cf2..ca8aa7a 100644 --- a/apps/jwt_auth/services.py +++ b/apps/jwt_auth/services.py @@ -94,11 +94,18 @@ def login_by_mobile(mobile: int): from math import ceil raise SmsRequestThrottled(ceil(remaining)) code = secrets.randbelow(900000) + 100000 - if not DefaultSmsProvider().send_sms(str(user.mobile), f'کد ورود شما: {code}'): + delivery = DefaultSmsProvider().send_sms(str(user.mobile), f'کد ورود شما: {code}') + if delivery is False: raise SmsDeliveryUnavailable() - SmsOpt.objects.filter(mobile=user.mobile).delete() + if delivery is True: + SmsOpt.objects.filter(mobile=user.mobile).delete() + else: + # Keep any previous unexpired code too: either SMS may have arrived. + SmsOpt.objects.filter(mobile=user.mobile, expire__lte=timezone.now()).delete() SmsOpt.objects.create(mobile=user.mobile, opt=code, expire=timezone.now() + timedelta(minutes=3)) - return user.mobile + return {'mobile': user.mobile, 'delivery_status': 'accepted' if delivery is True else 'unconfirmed', + 'message': 'opt code send' if delivery is True else 'وضعیت ارسال پیامک تأیید نشد. اگر کد را دریافت کرده‌اید، آن را وارد کنید.', + 'retry_after': SMS_RESEND_INTERVAL} @transaction.atomic diff --git a/apps/jwt_auth/test_customer_login.py b/apps/jwt_auth/test_customer_login.py index d08e606..e224b9e 100644 --- a/apps/jwt_auth/test_customer_login.py +++ b/apps/jwt_auth/test_customer_login.py @@ -1,14 +1,62 @@ """Customer OTP journey, repeat submits, and recoverable failure contracts.""" from datetime import timedelta -from unittest.mock import patch +from unittest.mock import Mock, patch +from django.test import override_settings from django.utils import timezone +import requests from apps.cuser.models import SmsOpt from apps.jwt_auth.test_regressions import RegressionFixtures class CustomerLoginTests(RegressionFixtures): + def test_unrecognized_provider_reply_keeps_delivered_code_usable(self): + for body in ('1', '654321987', 'accepted', ''): + with self.subTest(body=body): + SmsOpt.objects.all().delete() + with patch('utils.services.sms.requests.get', return_value=Mock(text=body)): + sent = self.post('login/mobile/', {'mobile': self.user.mobile}) + self.assertEqual(sent.status_code, 202) + self.assertEqual(sent.data['result']['delivery_status'], 'unconfirmed') + code = SmsOpt.objects.get(mobile=self.user.mobile).opt + verified = self.post('login/mobile/verify/', {'mobile': self.user.mobile, 'opt': code}) + self.assertEqual(verified.status_code, 200) + self.assertEqual(self.post('token/', {'uuid_opt': verified.data[0]['uuid']}).status_code, 200) + + def test_lost_provider_reply_keeps_code_and_still_throttles_repeat_sms(self): + for error in (requests.Timeout, requests.ConnectionError): + with self.subTest(error=error): + SmsOpt.objects.all().delete() + with patch('utils.services.sms.requests.get', side_effect=error) as send: + sent = self.post('login/mobile/', {'mobile': self.user.mobile}) + self.assertEqual(sent.status_code, 202) + self.assertEqual(self.post('login/mobile/', {'mobile': self.user.mobile}).status_code, 429) + send.assert_called_once() + code = SmsOpt.objects.get().opt + self.assertEqual(self.post('login/mobile/verify/', {'mobile': self.user.mobile, 'opt': code}).status_code, 200) + + def test_uncertain_resend_retains_previous_delivered_code(self): + original = self.otp() + SmsOpt.objects.filter(pk=original.pk).update(created_at=timezone.now() - timedelta(seconds=61)) + with patch('utils.services.sms.requests.get', return_value=Mock(text='unexpected')): + self.assertEqual(self.post('login/mobile/', {'mobile': self.user.mobile}).status_code, 202) + self.assertEqual(SmsOpt.objects.filter(mobile=self.user.mobile).count(), 2) + self.assertEqual(self.post('login/mobile/verify/', {'mobile': self.user.mobile, 'opt': original.opt}).status_code, 200) + self.assertFalse(SmsOpt.objects.filter(mobile=self.user.mobile).exists()) + + @override_settings(SMS_SUCCESS_CODES='0,1', SMS_FAILURE_CODES='error,-1') + def test_configured_response_codes_and_explicit_rejection(self): + with patch('utils.services.sms.requests.get', return_value=Mock(text='\ufeff1\r\n')): + sent = self.post('login/mobile/', {'mobile': self.user.mobile}) + self.assertEqual(sent.status_code, 200) + self.assertEqual(sent.data['result']['delivery_status'], 'accepted') + original = SmsOpt.objects.get() + SmsOpt.objects.filter(pk=original.pk).update(created_at=timezone.now() - timedelta(seconds=61)) + with patch('utils.services.sms.requests.get', return_value=Mock(text='-1')): + self.assertEqual(self.post('login/mobile/', {'mobile': self.user.mobile}).status_code, 503) + self.assertEqual(SmsOpt.objects.get().pk, original.pk) + def test_complete_customer_journey_login_refresh_and_logout(self): with patch('apps.jwt_auth.services.DefaultSmsProvider.send_sms', return_value=True) as send: sent = self.post('login/mobile/', {'mobile': '۰۹۱۲۳۴۵۶۷۸۹'}) diff --git a/apps/jwt_auth/test_regressions.py b/apps/jwt_auth/test_regressions.py index 0defed5..9398ba4 100644 --- a/apps/jwt_auth/test_regressions.py +++ b/apps/jwt_auth/test_regressions.py @@ -249,7 +249,7 @@ class AuthRegressionTests(RegressionFixtures): with patch('utils.services.sms.requests.get', return_value=Mock(text='error')): self.assertFalse(DosefrNavadHashtSMS().send_sms(str(self.user.mobile), 'Test')) with patch('utils.services.sms.requests.get', side_effect=requests.Timeout): - self.assertFalse(DosefrNavadHashtSMS().send_sms(str(self.user.mobile), 'Test')) + self.assertIsNone(DosefrNavadHashtSMS().send_sms(str(self.user.mobile), 'Test')) def test_iranian_mobile_prefix_is_not_accidentally_removed(self): self.assertEqual(normalize_mobile('9891234567'), 9891234567) diff --git a/apps/jwt_auth/views.py b/apps/jwt_auth/views.py index 29344bb..29a34e9 100644 --- a/apps/jwt_auth/views.py +++ b/apps/jwt_auth/views.py @@ -9,7 +9,7 @@ from rest_framework_simplejwt.tokens import RefreshToken from apps.cuser.models import CUser, CuserRole, RoleType from apps.jwt_auth.models import JwtToken, CUserRoleLoginOpt -from apps.jwt_auth.services import login_by_username, login_by_mobile, verify_mobile_and_get_token, SMS_RESEND_INTERVAL +from apps.jwt_auth.services import login_by_username, login_by_mobile, verify_mobile_and_get_token from apps.jwt_auth.session import issue_tokens, validate_session_claims from apps.workspace.models import Workspace @@ -69,8 +69,8 @@ class LoginByMobileView(PublicAuthView): def post(self, request): serializer = self.InputSerializer(data=request.data) serializer.is_valid(raise_exception=True) - mobile = login_by_mobile(**serializer.validated_data) - return Response({'result': {'mobile': mobile, 'message': 'opt code send', 'retry_after': SMS_RESEND_INTERVAL}}) + result = login_by_mobile(**serializer.validated_data) + return Response({'result': result}, status=202 if result['delivery_status'] == 'unconfirmed' else 200) class LoginByUserView(PublicAuthView): diff --git a/utils/services/sms.py b/utils/services/sms.py index 291202e..811c979 100644 --- a/utils/services/sms.py +++ b/utils/services/sms.py @@ -1,22 +1,25 @@ from abc import ABC, abstractmethod import requests +import logging from django.conf import settings +logger = logging.getLogger(__name__) + class SmsProviderABC(ABC): @abstractmethod - def send_sms(self, mobile: str, message: str) -> bool: + def send_sms(self, mobile: str, message: str) -> bool | None: pass class DefaultSmsProvider(SmsProviderABC): - def send_sms(self, mobile: str, message: str) -> bool: + def send_sms(self, mobile: str, message: str) -> bool | None: default_provider = DosefrNavadHashtSMS() return default_provider.send_sms(mobile, message) class DosefrNavadHashtSMS(SmsProviderABC): - def send_sms(self, mobile: str, message: str) -> bool: + def send_sms(self, mobile: str, message: str) -> bool | None: payload = { 'USERNAME': 'vsms7937', 'PASSWORD': '335577775533', @@ -27,9 +30,31 @@ class DosefrNavadHashtSMS(SmsProviderABC): try: r = requests.get('http://www.0098sms.com/sendsmslink.aspx', params=payload, timeout=10) r.raise_for_status() + except (requests.Timeout, requests.ConnectionError): + # The provider may have dispatched the SMS before its reply was lost. + logger.warning('SMS provider reply unavailable; dispatch is unconfirmed') + return None + except requests.HTTPError as error: + if error.response is not None and error.response.status_code >= 500: + logger.warning('SMS provider server error; dispatch is unconfirmed') + return None + return False except requests.RequestException: return False - # An HTTP 200 alone does not mean the SMS provider accepted the message. + # Unknown bodies are NOT evidence of rejection. In particular, never + # discard a delivered OTP just because the response contract differs. success_codes = getattr(settings, 'SMS_SUCCESS_CODES', ('0',)) - return r.text.strip() in success_codes + failure_codes = getattr(settings, 'SMS_FAILURE_CODES', ('error',)) + body = r.text.strip().lstrip('\ufeff').strip() + def codes(values): + if isinstance(values, str): + values = values.split(',') + return {str(value).strip() for value in values} + if body in codes(success_codes): + return True + if body in codes(failure_codes): + return False + # Do not log the reply, URL, credentials, mobile, or OTP. + logger.warning('SMS provider returned an unrecognized reply; dispatch is unconfirmed') + return None