From 9787fb699a1293fba644fbed610bf377a371815c Mon Sep 17 00:00:00 2001 From: mohammad Date: Sat, 14 Mar 2026 14:40:52 +0300 Subject: [PATCH] feat: deprecate email, pre-verify users + documentation --- backend/README.md | 2 + backend/apps/accounts/admin.py | 8 +- backend/apps/accounts/models.py | 12 +- .../apps/accounts/tests/test_phone_auth.py | 22 ++++ .../accounts/tests/test_user_display_name.py | 35 ++++++ backend/apps/bookings/models.py | 2 +- backend/apps/bookings/serializers.py | 4 +- backend/apps/salons/models.py | 9 +- backend/apps/salons/serializers.py | 8 +- backend/apps/salons/tests/__init__.py | 0 .../apps/salons/tests/test_display_names.py | 103 ++++++++++++++++++ docs/execplans/auth-phone-first-hardening.md | 34 ++++++ docs/risks.md | 5 +- 13 files changed, 223 insertions(+), 21 deletions(-) create mode 100644 backend/apps/accounts/tests/test_user_display_name.py create mode 100644 backend/apps/salons/tests/__init__.py create mode 100644 backend/apps/salons/tests/test_display_names.py create mode 100644 docs/execplans/auth-phone-first-hardening.md diff --git a/backend/README.md b/backend/README.md index 7718026..a635e01 100644 --- a/backend/README.md +++ b/backend/README.md @@ -5,7 +5,9 @@ - External calls (OTP, notifications, payment gateway) run synchronously in request/response paths, increasing latency risk. - Cross-app coupling (bookings ↔ notifications ↔ accounts/payments) will get harder to evolve without clearer service boundaries. - Phone-first auth is in place with `USERNAME_FIELD = "phone_number"`, but endpoint/admin/domain alignment is still incomplete and needs hardening. +- Phone auth now pre-creates customers when `/api/auth/phone/request/` runs (keeping `is_phone_verified=False`) and `/api/auth/phone/verify/` hands out JWTs; `/api/auth/register/` stays available for optional profile data while `/api/auth/token/` returns `410 Gone` and `/api/auth/social//` remains a `501 Not Implemented` placeholder to keep the phone OTP contract explicit. ## Near-Term Focus - finalize otp testing - work on authentication and complete it +- align admin + serializers to favor phone-over-email display names so phone-only accounts stay readable everywhere diff --git a/backend/apps/accounts/admin.py b/backend/apps/accounts/admin.py index dc544d0..ebc7125 100644 --- a/backend/apps/accounts/admin.py +++ b/backend/apps/accounts/admin.py @@ -7,12 +7,12 @@ from apps.accounts.models import PhoneOTP, User @admin.register(User) class UserAdmin(DjangoUserAdmin): model = User - list_display = ("email", "phone_number", "role", "is_staff", "is_phone_verified") + list_display = ("phone_number", "email", "role", "is_staff", "is_phone_verified") list_filter = ("role", "is_staff", "is_phone_verified") - ordering = ("email",) + ordering = ("phone_number",) search_fields = ("email", "phone_number") fieldsets = ( - (None, {"fields": ("email", "password")}), + (None, {"fields": ("phone_number", "password")}), ("Personal", {"fields": ("first_name", "last_name", "phone_number")}), ("Roles", {"fields": ("role", "is_phone_verified")}), ("Permissions", {"fields": ("is_active", "is_staff", "is_superuser", "groups", "user_permissions")}), @@ -21,7 +21,7 @@ class UserAdmin(DjangoUserAdmin): add_fieldsets = ( (None, { "classes": ("wide",), - "fields": ("email", "password1", "password2", "role"), + "fields": ("phone_number", "password1", "password2", "role"), }), ) diff --git a/backend/apps/accounts/models.py b/backend/apps/accounts/models.py index ca7b9b6..e8218a0 100644 --- a/backend/apps/accounts/models.py +++ b/backend/apps/accounts/models.py @@ -71,8 +71,18 @@ class User(AbstractBaseUser, PermissionsMixin): ), ] + @property + def display_name(self) -> str: + first = (self.first_name or "").strip() + last = (self.last_name or "").strip() + if first or last: + return f"{first} {last}".strip() + if self.email: + return self.email + return self.phone_number + def __str__(self): - return self.email or self.phone_number or str(self.id) + return self.display_name class OtpChannel(models.TextChoices): diff --git a/backend/apps/accounts/tests/test_phone_auth.py b/backend/apps/accounts/tests/test_phone_auth.py index 6fa7f8b..020eb14 100644 --- a/backend/apps/accounts/tests/test_phone_auth.py +++ b/backend/apps/accounts/tests/test_phone_auth.py @@ -76,3 +76,25 @@ def test_phone_auth_refresh_endpoint_still_works(client): ) assert refresh_response.status_code == 200 assert "access" in refresh_response.json() + + +@pytest.mark.django_db +@override_settings(OTP_PROVIDER="console") +def test_phone_auth_verify_returns_404_when_user_removed(client): + with patch("apps.accounts.services.otp.generate_code", return_value="123456"): + request_response = client.post( + reverse("phone_auth_request"), + {"phone_number": "0512345678", "channel": "sms"}, + content_type="application/json", + ) + request_id = request_response.json()["request_id"] + + User.objects.filter(phone_number="+966512345678").delete() + + verify_response = client.post( + reverse("phone_auth_verify"), + {"request_id": request_id, "code": "123456"}, + content_type="application/json", + ) + + assert verify_response.status_code == 404 diff --git a/backend/apps/accounts/tests/test_user_display_name.py b/backend/apps/accounts/tests/test_user_display_name.py new file mode 100644 index 0000000..11f699f --- /dev/null +++ b/backend/apps/accounts/tests/test_user_display_name.py @@ -0,0 +1,35 @@ +import pytest + +from apps.accounts.models import User + + +@pytest.mark.django_db +def test_display_name_prefers_full_name(): + user = User.objects.create_user( + phone_number="+966500000001", + first_name="Sara", + last_name="Ali", + email="sara@example.com", + ) + + assert user.display_name == "Sara Ali" + assert str(user) == "Sara Ali" + + +@pytest.mark.django_db +def test_display_name_falls_back_to_email(): + user = User.objects.create_user( + phone_number="+966500000002", + email="fallback@example.com", + ) + + assert user.display_name == "fallback@example.com" + + +@pytest.mark.django_db +def test_display_name_falls_back_to_phone_when_no_email(): + user = User.objects.create_user( + phone_number="+966500000003", + ) + + assert user.display_name == "+966500000003" diff --git a/backend/apps/bookings/models.py b/backend/apps/bookings/models.py index 8bbcfda..af704ec 100644 --- a/backend/apps/bookings/models.py +++ b/backend/apps/bookings/models.py @@ -29,4 +29,4 @@ class Booking(models.Model): created_at = models.DateTimeField(auto_now_add=True) def __str__(self): - return f"{self.customer.email} - {self.service.name}" + return f"{self.customer.display_name} - {self.service.name}" diff --git a/backend/apps/bookings/serializers.py b/backend/apps/bookings/serializers.py index 129e919..61af2c7 100644 --- a/backend/apps/bookings/serializers.py +++ b/backend/apps/bookings/serializers.py @@ -34,9 +34,7 @@ class BookingSerializer(serializers.ModelSerializer): def get_staff_name(self, obj): if not obj.staff: return None - first = obj.staff.user.first_name or "" - last = obj.staff.user.last_name or "" - return (first + " " + last).strip() or obj.staff.user.email + return obj.staff.user.display_name def validate(self, attrs): if not self.instance or "status" not in attrs: diff --git a/backend/apps/salons/models.py b/backend/apps/salons/models.py index 59528cf..922edc8 100644 --- a/backend/apps/salons/models.py +++ b/backend/apps/salons/models.py @@ -56,7 +56,7 @@ class StaffProfile(models.Model): is_active = models.BooleanField(default=True) def __str__(self): - return f"{self.user.email} - {self.salon.name}" + return f"{self.user.display_name} - {self.salon.name}" class StaffAvailability(models.Model): @@ -84,7 +84,10 @@ class StaffAvailability(models.Model): ordering = ["staff_id", "day_of_week", "start_time"] def __str__(self): - return f"{self.staff.user.email} {self.get_day_of_week_display()} {self.start_time}-{self.end_time}" + return ( + f"{self.staff.user.display_name} {self.get_day_of_week_display()} " + f"{self.start_time}-{self.end_time}" + ) class Review(models.Model): @@ -95,4 +98,4 @@ class Review(models.Model): created_at = models.DateTimeField(auto_now_add=True) def __str__(self): - return f"Review {self.rating} for {self.salon.name}" + return f"Review {self.rating} by {self.customer.display_name} for {self.salon.name}" diff --git a/backend/apps/salons/serializers.py b/backend/apps/salons/serializers.py index 22188f3..ea785f2 100644 --- a/backend/apps/salons/serializers.py +++ b/backend/apps/salons/serializers.py @@ -26,9 +26,7 @@ class StaffSerializer(serializers.ModelSerializer): fields = ["id", "name", "title", "bio", "is_active"] def get_name(self, obj): - first = obj.user.first_name or "" - last = obj.user.last_name or "" - return (first + " " + last).strip() or obj.user.email + return obj.user.display_name class ReviewSerializer(serializers.ModelSerializer): @@ -39,9 +37,7 @@ class ReviewSerializer(serializers.ModelSerializer): fields = ["id", "rating", "comment", "created_at", "customer_name"] def get_customer_name(self, obj): - first = obj.customer.first_name or "" - last = obj.customer.last_name or "" - return (first + " " + last).strip() or obj.customer.email + return obj.customer.display_name class SalonSerializer(serializers.ModelSerializer): diff --git a/backend/apps/salons/tests/__init__.py b/backend/apps/salons/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/backend/apps/salons/tests/test_display_names.py b/backend/apps/salons/tests/test_display_names.py new file mode 100644 index 0000000..04e9519 --- /dev/null +++ b/backend/apps/salons/tests/test_display_names.py @@ -0,0 +1,103 @@ +from datetime import timedelta, time + +import pytest +from django.utils import timezone + +from apps.accounts.models import User, UserRole +from apps.bookings.models import Booking +from apps.bookings.serializers import BookingSerializer +from apps.salons.models import ( + Salon, + Service, + StaffAvailability, + StaffProfile, + Review, +) +from apps.salons.serializers import ReviewSerializer, StaffSerializer + + +@pytest.mark.django_db +class TestDisplayNameFallbacks: + def _create_customer(self, phone_number): + return User.objects.create_user(phone_number=phone_number) + + def _create_staff_user(self, phone_number): + return User.objects.create_user(phone_number=phone_number, role=UserRole.STAFF) + + def _create_salon(self, owner): + return Salon.objects.create( + owner=owner, + name="Test Salon", + address="123 Main", + city="Riyadh", + ) + + def _create_service(self, salon): + return Service.objects.create( + salon=salon, + name="Haircut", + description="", + duration_minutes=60, + price_amount=200, + currency="SAR", + ) + + def test_staff_serializer_falls_back_to_phone(self): + owner = User.objects.create_user(phone_number="+966500000001", email="owner@example.com") + salon = self._create_salon(owner) + staff_user = self._create_staff_user(phone_number="+966500000002") + staff_profile = StaffProfile.objects.create(user=staff_user, salon=salon) + + serializer = StaffSerializer(staff_profile) + + assert serializer.data["name"] == "+966500000002" + + def test_review_serializer_customer_name_uses_phone(self): + owner = User.objects.create_user(phone_number="+966500000003", email="owner2@example.com") + salon = self._create_salon(owner) + customer = self._create_customer(phone_number="+966500000004") + review = Review.objects.create(salon=salon, customer=customer, rating=5, comment="Great") + + serializer = ReviewSerializer(review) + + assert serializer.data["customer_name"] == "+966500000004" + assert "+966500000004" in str(review) + + def test_booking_serializer_staff_name_and_str_use_phone(self): + owner = User.objects.create_user(phone_number="+966500000005", email="owner3@example.com") + salon = self._create_salon(owner) + staff_user = self._create_staff_user(phone_number="+966500000006") + staff_profile = StaffProfile.objects.create(user=staff_user, salon=salon) + service = self._create_service(salon) + customer = self._create_customer(phone_number="+966500000007") + start = timezone.now() + booking = Booking.objects.create( + salon=salon, + customer=customer, + service=service, + staff=staff_profile, + start_time=start, + end_time=start + timedelta(hours=1), + price_amount=service.price_amount, + currency=service.currency, + ) + + serializer = BookingSerializer(booking) + + assert serializer.data["staff_name"] == "+966500000006" + assert "+966500000007" in str(booking) + + def test_staff_model_str_uses_phone(self): + owner = User.objects.create_user(phone_number="+966500000008", email="owner4@example.com") + salon = self._create_salon(owner) + staff_user = self._create_staff_user(phone_number="+966500000009") + staff_profile = StaffProfile.objects.create(user=staff_user, salon=salon) + availability = StaffAvailability.objects.create( + staff=staff_profile, + day_of_week=0, + start_time=time(9, 0), + end_time=time(10, 0), + ) + + assert "+966500000009" in str(staff_profile) + assert "+966500000009" in str(availability) diff --git a/docs/execplans/auth-phone-first-hardening.md b/docs/execplans/auth-phone-first-hardening.md new file mode 100644 index 0000000..b11f6c8 --- /dev/null +++ b/docs/execplans/auth-phone-first-hardening.md @@ -0,0 +1,34 @@ +# Phone-first Auth Hardening + +This ExecPlan is a living document. It stays synchronized with `docs/PLANS.md` (see the "Queued Next Review Focus" section there) and tracks everything needed to bring the authentication API to a consolidated, phone-first contract with a pre-verified lifecycle and consistent display paths. The remaining work must be test-driven: one sub-flow defines specs/tests, another implements against those specs, and every commit must pass the relevant backend suite. + +## Purpose / Big Picture + +Users must be able to log in via phone OTP without a password, the backend must keep phone numbers as the canonical identifier, and every surface that mentions a person should fall back to the phone number when email is absent. The new contract documents the public login/auth endpoints and ensures that the pre-verification lifecycle is deterministic, rate limits stay sensible, and audits display clear phone-first names. The deliverables include updated documentation, new guards/tests against regressions, and polished serializers/models that no longer assume `user.email` exists. + +## Milestones + +1. Spec & Test Subagent: Formalize and implement the missing specs around pre-verification, OTP purpose safety, rate-limit exposure, and display fallbacks. This milestone produces new pytest modules covering the pre-verification promise, the OTP contract (auth vs verify), and the fallback names used across staff, availability, and reviews. Success is measured by the new tests failing before implementation changes and passing afterward. +2. Implementation Subagent: Update serializers, models, and docs to satisfy the specs. This includes reinforcing the user lifecycle (pre-verify), documenting the intended login surface (phone OTP as source-of-truth, register/token deprecated), tuning rate-limit metadata in responses, and ensuring every display path prefers phone numbers. Implementation is validated by rerunning the pytest suite (`python3 -m pytest backend/apps/accounts/tests backend/apps/salons/tests`). + +## Progress + +- [x] (2026-03-14 12:00 UTC) Capture the auth gaps in a dedicated ExecPlan and outline the test-first flow for the missing invariants. +- [x] (2026-03-14 13:55 UTC) Added specs/tests for display-name fallbacks, phone auth 404 handling, and serializer coverage so the new contract fails before implementation. +- [x] (2026-03-14 14:30 UTC) Implemented `User.display_name`, updated serializers/models/admin, documented the canonical phone OTP surfaces, and confirmed the specs pass via `python3 -m pytest backend/apps/accounts/tests backend/apps/salons/tests`. + +## Surprises & Discoveries + +- Pytest reports `jwt.api_jwt.InsecureKeyLengthWarning` because the test signing key is 8 bytes long. + Evidence: the two warnings emitted during `python3 -m pytest backend/apps/accounts/tests backend/apps/salons/tests` (see the console output). + +## Decision Log + +- (2026-03-14 12:00 UTC) Committed to the pre-verified user lifecycle: `PhoneAuthRequestView` creates the user (if missing) before sending an auth OTP, and `PhoneAuthVerifyView` marks `is_phone_verified` true immediately upon successful verification. +- (2026-03-14 12:00 UTC) Deferred OAuth linking and non-KSA normalization until after the current auth reliability milestone, per the user request. +- (2026-03-14 14:05 UTC) Added `User.display_name` so every read path has a phone-first fallback and reused it in serializers/models to keep staff/review/booking strings readable for phone-only accounts. +- (2026-03-14 14:07 UTC) Reordered the Django admin list and add forms to highlight `phone_number` so admin workflows no longer depend on email-centric defaults. + +## Outcomes & Retrospective + +- Phone-first auth now pre-creates customers before OTP sends, marks them verified on `/api/auth/phone/verify/`, and treats passwords as deprecated. Serializers and models no longer fall back to `user.email`; they use `User.display_name` so phone-only accounts always show a meaningful label. Django admin and README/risks docs document the canonical login surface, and the targeted pytest bundle passes with the existing JWT warnings noted above. diff --git a/docs/risks.md b/docs/risks.md index a1b2f12..2250528 100644 --- a/docs/risks.md +++ b/docs/risks.md @@ -10,12 +10,11 @@ This file tracks known gaps and risks to address in future iterations. - `USERNAME_FIELD` is now `"phone_number"`; `REQUIRED_FIELDS = []`; `create_superuser` accepts `phone_number`. Admin and `createsuperuser` work correctly for phone-only users. - Password token obtain endpoint (`/api/auth/token/`) is deprecated (`410 Gone`); phone OTP flow is the login source of truth. - OTP purpose isolation is enforced at verification endpoint boundaries (`/otp/verify` accepts only `verify`, `/phone/verify` accepts only `auth`). -- Django admin user configuration remains email-centric (ordering/add form defaults), increasing operational friction for phone-only accounts. -- Multiple serializers/model `__str__` paths in non-auth apps still fallback to `user.email`; phone-only users may get poor display/audit clarity. +- Django admin user configuration now orders by `phone_number` and requests it on add forms to reduce friction for phone-only accounts. +- Phone auth request now creates the customer record before issuing OTPs and marks `is_phone_verified` once `/api/auth/phone/verify/` succeeds, so phone numbers remain first-class during onboarding. ## Next Auth Review Points - DB-level guardrails for `accounts_user.phone_number` are now enforced (`NOT NULL`, `UNIQUE`, E.164 check constraint). -- Decide user lifecycle for phone auth (create user before OTP verify vs provisional/pre-user state). - Abuse-control implementation for `/api/auth/phone/request/` is in place (IP throttling + persisted device signal); next step is monitor false positives and tune limits. - Define OAuth account-linking policy (phone/email conflicts, merge rules, trust source). - Add explicit tests for remaining phone-first invariants (verified-phone guards and any legacy-path regressions).