From 2b182489b923f0ae1599488c4e3e2e769a8d12ae Mon Sep 17 00:00:00 2001 From: bouhamza abderrahmane Date: Sun, 31 May 2026 08:54:31 +0100 Subject: [PATCH 01/11] Validate mobile auth required fields --- app/core/config.py | 5 + app/schema/request/mobile/auth.py | 42 +++- ...1-mobile-auth-required-field-validation.md | 201 ++++++++++++++++++ docs/problem-fixes/README.md | 15 ++ pyproject.toml | 3 + ...test_mobile_auth_request_validation_e2e.py | 51 +++++ .../test_mobile_auth_request_validation.py | 120 +++++++++++ 7 files changed, 433 insertions(+), 4 deletions(-) create mode 100644 docs/problem-fixes/001-mobile-auth-required-field-validation.md create mode 100644 docs/problem-fixes/README.md create mode 100644 tests/e2e/test_mobile_auth_request_validation_e2e.py create mode 100644 tests/unit/test_mobile_auth_request_validation.py diff --git a/app/core/config.py b/app/core/config.py index 9b72a6c..22eda29 100644 --- a/app/core/config.py +++ b/app/core/config.py @@ -36,6 +36,11 @@ class Settings(BaseSettings): MOBILE_SESSION_LIMIT: int = 3 MOBILE_SESSION_TTL_SECONDS: int = 180 MOBILE_SESSION_DAYS: int = 7 + # Mobile auth validation defaults + MOBILE_AUTH_PASSWORD_MIN_LEN: int = 8 + MOBILE_AUTH_PASSWORD_MAX_LEN: int = 128 + MOBILE_AUTH_DEVICE_NAME_MAX_LEN: int = 64 + MOBILE_AUTH_DEVICE_TYPE_MAX_LEN: int = 32 # Admin list defaults ADMIN_USERS_DEFAULT_LIMIT: int = 20 ADMIN_USERS_MAX_LIMIT: int = 100 diff --git a/app/schema/request/mobile/auth.py b/app/schema/request/mobile/auth.py index fcbf1c5..5fdc611 100644 --- a/app/schema/request/mobile/auth.py +++ b/app/schema/request/mobile/auth.py @@ -1,14 +1,48 @@ -from pydantic import BaseModel, EmailStr +from pydantic import BaseModel, EmailStr, Field, ValidationInfo, field_validator from uuid import UUID +from app.core.config import settings class MobileAuthRequest(BaseModel): email: EmailStr - password: str - device_name: str - device_type: str + password: str = Field( + ..., + min_length=settings.MOBILE_AUTH_PASSWORD_MIN_LEN, + max_length=settings.MOBILE_AUTH_PASSWORD_MAX_LEN, + ) + device_name: str = Field( + ..., + min_length=1, + max_length=settings.MOBILE_AUTH_DEVICE_NAME_MAX_LEN, + ) + device_type: str = Field( + ..., + min_length=1, + max_length=settings.MOBILE_AUTH_DEVICE_TYPE_MAX_LEN, + ) device_id: UUID + @field_validator("email", mode="before") + @classmethod + def _normalize_email(cls, value: object) -> object: + if not isinstance(value, str): + return value + return value.strip().lower() + + @field_validator("password", "device_name", "device_type", mode="before") + @classmethod + def _strip_required_text(cls, value: object, info: ValidationInfo) -> object: + if not isinstance(value, str): + return value + stripped = value.strip() + if not stripped: + raise ValueError(f"{info.field_name} must not be empty") + if info.field_name == "password": + return stripped + if info.field_name == "device_type": + return stripped.lower() + return stripped + diff --git a/docs/problem-fixes/001-mobile-auth-required-field-validation.md b/docs/problem-fixes/001-mobile-auth-required-field-validation.md new file mode 100644 index 0000000..0e6a2b2 --- /dev/null +++ b/docs/problem-fixes/001-mobile-auth-required-field-validation.md @@ -0,0 +1,201 @@ +# 001 - Mobile Auth Accepted Empty Or Unnormalized Input Fields + +## Status + +Fixed in `app/schema/request/mobile/auth.py` and covered by endpoint and e2e tests. + +## Problem + +`POST /user/auth/register-login` accepted weak request payloads because `MobileAuthRequest` used plain `str` fields for `password`, `device_name`, and `device_type`. + +The risky payloads were: + +- empty password: `""` +- whitespace-only password: `" "` +- empty or whitespace-only device name +- empty or whitespace-only device type +- device names and types with accidental surrounding whitespace +- mixed-case device types that would fragment downstream analytics +- oversized password/device strings because no upper bound existed + +This created both correctness and data-quality problems. A mobile account could be registered with an empty password, and device records could be created with blank or inconsistent device metadata. + +## Root Cause + +The request model did not put validation at the boundary where the API payload enters the system. + +Before the fix, the schema shape was effectively: + +```python +class MobileAuthRequest(BaseModel): + email: EmailStr + password: str + device_name: str + device_type: str + device_id: UUID +``` + +Plain `str` accepts `""`, `" "`, and arbitrarily large values. Since `app/service/users.py` trusts `MobileAuthRequest`, bad values could flow directly into password hashing, user creation, and device creation. + +## Fix Location + +The fix is in [app/schema/request/mobile/auth.py](../../app/schema/request/mobile/auth.py). + +- `app/schema/request/mobile/auth.py:8-22` adds `Field(...)` constraints: + - password min/max length + - device name min/max length + - device type min/max length + +- `app/schema/request/mobile/auth.py:25-30` normalizes email input: + - trims surrounding whitespace + - lowercases casing before `EmailStr` validation + +- `app/schema/request/mobile/auth.py:32-44` validates required text fields: + - rejects empty or whitespace-only values + - trims password before length validation + - trims `device_name` + - trims and lowercases `device_type` + +The config-backed limits are in [app/core/config.py](../../app/core/config.py): + +- `app/core/config.py:39-43` + - `MOBILE_AUTH_PASSWORD_MIN_LEN = 8` + - `MOBILE_AUTH_PASSWORD_MAX_LEN = 128` + - `MOBILE_AUTH_DEVICE_NAME_MAX_LEN = 64` + - `MOBILE_AUTH_DEVICE_TYPE_MAX_LEN = 32` + +The executable regression tests are in [tests/unit/test_mobile_auth_request_validation.py](../../tests/unit/test_mobile_auth_request_validation.py). They use FastAPI `TestClient` against the real `app.main.app` and the real `/user/auth/register-login` route. + +The only dependency override is `get_container`, because FastAPI resolves that dependency before returning request-body validation errors. Without the override, the test tries to open a real Postgres connection before it can assert `422`. + +- `tests/unit/test_mobile_auth_request_validation.py:50-56` creates a `TestClient` from the real app and overrides only the infrastructure container. +- `tests/unit/test_mobile_auth_request_validation.py:69-83` posts empty and whitespace-only values to the real endpoint and verifies the service is not called. +- `tests/unit/test_mobile_auth_request_validation.py:86-106` posts a valid payload and verifies the service receives normalized values. +- `tests/unit/test_mobile_auth_request_validation.py:109-119` posts a padded one-character password and verifies it is rejected before the service is called. + +The live e2e tests are in [tests/e2e/test_mobile_auth_request_validation_e2e.py](../../tests/e2e/test_mobile_auth_request_validation_e2e.py): + +- `tests/e2e/test_mobile_auth_request_validation_e2e.py:8-18` makes the test opt-in and points it at a real running backend. +- `tests/e2e/test_mobile_auth_request_validation_e2e.py:31-42` sends real HTTP requests for empty and whitespace-only fields. +- `tests/e2e/test_mobile_auth_request_validation_e2e.py:45-51` sends a real HTTP request for a padded short password. + +## Fix Behavior + +After the fix: + +- `password=""` is rejected +- `password=" "` is rejected +- `device_name=""` and `device_name=" "` are rejected +- `device_type=""` and `device_type=" "` are rejected +- `" Pixel 8 "` becomes `"Pixel 8"` +- `" ANDROID "` becomes `"android"` +- `" USER@Example.COM "` becomes `"user@example.com"` +- `" a"` is rejected because trimming happens before password length validation + +## Test Showcase + +These are the core endpoint tests that demonstrate the bug and the fix. + +```python +@pytest.mark.parametrize("field", ["password", "device_name", "device_type"]) +@pytest.mark.parametrize("value", ["", " "]) +def test_register_login_rejects_empty_required_text_fields( + client, + fake_container, + field, + value, +): + payload = _valid_payload() + payload[field] = value + + response = client.post("/user/auth/register-login", json=payload) + + assert response.status_code == 422 + assert fake_container.auth_service.received_request is None +``` + +Before the fix, those payloads were accepted by the endpoint because the fields were plain `str`. The second assertion makes the test real at the route boundary: invalid payloads must fail before they can reach `AuthService.mobile_register_login()`. + +The request body is still written in the test because the test has to define the scenario. What makes this a real endpoint simulation is that the payload goes through FastAPI HTTP request handling, dependency solving, request-body parsing, Pydantic validation, and the actual route path instead of directly calling `MobileAuthRequest(...)`. + +```python +def test_register_login_passes_normalized_input_to_service(client, fake_container): + payload = _valid_payload() + payload.update( + { + "email": " USER@Example.COM ", + "device_name": " Pixel 8 ", + "device_type": " ANDROID ", + } + ) + + response = client.post("/user/auth/register-login", json=payload) + + assert response.status_code == 200 + req = fake_container.auth_service.received_request + assert req is not None + assert req.email == "user@example.com" + assert req.device_name == "Pixel 8" + assert req.device_type == "android" +``` + +Before the fix, device fields kept surrounding whitespace and mixed casing. + +```python +def test_register_login_password_length_is_checked_after_trimming( + client, + fake_container, +): + payload = _valid_payload() + payload["password"] = " a" + + response = client.post("/user/auth/register-login", json=payload) + + assert response.status_code == 422 + assert fake_container.auth_service.received_request is None +``` + +This protects the password minimum-length rule from being bypassed by padding a one-character password with spaces. + +## Verification + +Endpoint test command: + +```bash +.venv\Scripts\python.exe -m pytest tests\unit\test_mobile_auth_request_validation.py -q +``` + +Result: + +```text +8 passed, 2 warnings in 4.07s +``` + +E2e skipped-mode command: + +```bash +.venv\Scripts\python.exe -m pytest tests\e2e\test_mobile_auth_request_validation_e2e.py -q +``` + +Result: + +```text +7 skipped in 0.16s +``` + +Live e2e command, after starting the backend and infrastructure: + +```bash +$env:MULTAI_RUN_E2E = "1" +$env:MULTAI_E2E_BASE_URL = "http://localhost:8000" +.venv\Scripts\python.exe -m pytest tests\e2e\test_mobile_auth_request_validation_e2e.py -q +``` + +## Remaining Nearby Risks + +This fix covers the empty-field and basic normalization problem. The workshop audit still lists separate follow-up issues: + +- the combined register/login endpoint can still create a new account from an email typo +- plaintext email is still logged during auth attempts +- bcrypt still truncates passwords at 72 bytes unless the password policy prevents it +- `device_type` is normalized but still not restricted to an allowed enum diff --git a/docs/problem-fixes/README.md b/docs/problem-fixes/README.md new file mode 100644 index 0000000..6344e52 --- /dev/null +++ b/docs/problem-fixes/README.md @@ -0,0 +1,15 @@ +# Problem Fix Documentation + +This folder records production/debugging problems as one file per problem. + +Each problem note should include: + +- what broke +- why it broke +- where the fix lives, with code line numbers +- how the fix works +- tests or test snippets that demonstrate the bug and the expected fixed behavior + +## Entries + +- [001 - Mobile auth accepted empty or unnormalized input fields](002-mobile-auth-required-field-validation.md) diff --git a/pyproject.toml b/pyproject.toml index 8646b74..cb5cd01 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -73,3 +73,6 @@ dev = [ [tool.pytest.ini_options] pythonpath = ["."] asyncio_mode = "auto" +markers = [ + "e2e: tests that require a running backend and infrastructure services", +] diff --git a/tests/e2e/test_mobile_auth_request_validation_e2e.py b/tests/e2e/test_mobile_auth_request_validation_e2e.py new file mode 100644 index 0000000..deeced7 --- /dev/null +++ b/tests/e2e/test_mobile_auth_request_validation_e2e.py @@ -0,0 +1,51 @@ +import os +import uuid + +import httpx +import pytest + + +pytestmark = [ + pytest.mark.e2e, + pytest.mark.skipif( + os.getenv("MULTAI_RUN_E2E") != "1", + reason="set MULTAI_RUN_E2E=1 to run live e2e tests", + ), +] + + +BASE_URL = os.getenv("MULTAI_E2E_BASE_URL", "http://localhost:8000").rstrip("/") +REGISTER_LOGIN_URL = f"{BASE_URL}/user/auth/register-login" + + +def _valid_payload() -> dict[str, object]: + return { + "email": f"e2e-{uuid.uuid4()}@example.com", + "password": "validpass123", + "device_name": "Pixel 8", + "device_type": "android", + "device_id": str(uuid.uuid4()), + } + + +@pytest.mark.parametrize("field", ["password", "device_name", "device_type"]) +@pytest.mark.parametrize("value", ["", " "]) +def test_live_register_login_rejects_empty_required_text_fields( + field: str, + value: str, +) -> None: + payload = _valid_payload() + payload[field] = value + + response = httpx.post(REGISTER_LOGIN_URL, json=payload, timeout=10.0) + + assert response.status_code == 422 + + +def test_live_register_login_rejects_padded_short_password() -> None: + payload = _valid_payload() + payload["password"] = " a" + + response = httpx.post(REGISTER_LOGIN_URL, json=payload, timeout=10.0) + + assert response.status_code == 422 diff --git a/tests/unit/test_mobile_auth_request_validation.py b/tests/unit/test_mobile_auth_request_validation.py new file mode 100644 index 0000000..2fd48a6 --- /dev/null +++ b/tests/unit/test_mobile_auth_request_validation.py @@ -0,0 +1,120 @@ +import uuid +from collections.abc import Iterator +from typing import Any + +import pytest +from fastapi.testclient import TestClient + +from app.container import get_container +from app.main import app +from app.schema.request.mobile.auth import MobileAuthRequest +from app.schema.response.mobile.auth import MobileAuthResponse + + +class FakeAuthService: + def __init__(self) -> None: + self.received_request: MobileAuthRequest | None = None + + async def mobile_register_login( + self, + redis: object, + req: MobileAuthRequest, + ) -> MobileAuthResponse: + self.received_request = req + return MobileAuthResponse( + access_token="access", + refresh_token="refresh", + session_id=str(uuid.uuid4()), + expires_in=3600, + user_id=uuid.uuid4(), + is_new_user=True, + ) + + +class FakeAuditService: + async def create_record(self, **kwargs: Any) -> None: + return None + + +class FakeContainer: + def __init__(self) -> None: + self.redis = object() + self.auth_service = FakeAuthService() + self.audit_service = FakeAuditService() + + +@pytest.fixture +def fake_container() -> FakeContainer: + return FakeContainer() + + +@pytest.fixture +def client(fake_container: FakeContainer) -> Iterator[TestClient]: + app.dependency_overrides[get_container] = lambda: fake_container + try: + yield TestClient(app) + finally: + app.dependency_overrides.clear() + + +def _valid_payload() -> dict[str, object]: + return { + "email": "USER@Example.COM", + "password": "validpass123", + "device_name": "Pixel 8", + "device_type": "android", + "device_id": str(uuid.uuid4()), + } + + +@pytest.mark.parametrize("field", ["password", "device_name", "device_type"]) +@pytest.mark.parametrize("value", ["", " "]) +def test_register_login_rejects_empty_required_text_fields( + client: TestClient, + fake_container: FakeContainer, + field: str, + value: str, +) -> None: + payload = _valid_payload() + payload[field] = value + + response = client.post("/user/auth/register-login", json=payload) + + assert response.status_code == 422 + assert fake_container.auth_service.received_request is None + + +def test_register_login_passes_normalized_input_to_service( + client: TestClient, + fake_container: FakeContainer, +) -> None: + payload = _valid_payload() + payload.update( + { + "email": " USER@Example.COM ", + "device_name": " Pixel 8 ", + "device_type": " ANDROID ", + } + ) + + response = client.post("/user/auth/register-login", json=payload) + + assert response.status_code == 200 + req = fake_container.auth_service.received_request + assert req is not None + assert req.email == "user@example.com" + assert req.device_name == "Pixel 8" + assert req.device_type == "android" + + +def test_register_login_password_length_is_checked_after_trimming( + client: TestClient, + fake_container: FakeContainer, +) -> None: + payload = _valid_payload() + payload["password"] = " a" + + response = client.post("/user/auth/register-login", json=payload) + + assert response.status_code == 422 + assert fake_container.auth_service.received_request is None From 992e8f1cbc2d3a55ed4a92183ff06f5049a29b94 Mon Sep 17 00:00:00 2001 From: bouhamza abderrahmane Date: Mon, 1 Jun 2026 13:10:30 +0100 Subject: [PATCH 02/11] fix(auth): split register/login endpoints and fix stale e2e test - Replace ambiguous /user/auth/register-login with explicit /register and /login - Add MobileRegisterRequest and MobileLoginRequest schemas (shared base) - Add AuthService.mobile_register() (409 on existing email) and mobile_login() (401 on unknown email) - Update e2e validation test to target both new endpoints instead of removed register-login - Add unit + e2e tests for intent validation - Add problem-fix documentation (002) --- app/router/mobile/auth.py | 30 +- app/schema/request/mobile/auth.py | 10 +- app/service/users.py | 84 +++-- ...le-auth-ambiguous-register-login-intent.md | 125 +++++++ tests/e2e/test_mobile_auth_intent_e2e.py | 143 ++++++++ ...test_mobile_auth_request_validation_e2e.py | 12 +- .../test_mobile_auth_intent_validation.py | 335 ++++++++++++++++++ .../test_mobile_auth_request_validation.py | 88 +++-- 8 files changed, 757 insertions(+), 70 deletions(-) create mode 100644 docs/problem-fixes/002-mobile-auth-ambiguous-register-login-intent.md create mode 100644 tests/e2e/test_mobile_auth_intent_e2e.py create mode 100644 tests/unit/test_mobile_auth_intent_validation.py diff --git a/app/router/mobile/auth.py b/app/router/mobile/auth.py index cc7d1e1..2af4ca8 100644 --- a/app/router/mobile/auth.py +++ b/app/router/mobile/auth.py @@ -8,7 +8,8 @@ from app.deps.token_auth import MobileUserSchema, get_current_mobile_user from app.schema.request.mobile.auth import ( - MobileAuthRequest, + MobileLoginRequest, + MobileRegisterRequest, RefreshTokenRequest, UpdateDeviceTokenRequest, InactivateDeviceRequest, @@ -18,17 +19,30 @@ router = APIRouter(prefix="/auth") -@router.post("/register-login", response_model=MobileAuthResponse) -async def mobile_register_login( - req: MobileAuthRequest, +@router.post("/register", response_model=MobileAuthResponse) +async def mobile_register( + req: MobileRegisterRequest, container: Container = Depends(get_container), ) -> MobileAuthResponse: - result = await container.auth_service.mobile_register_login(container.redis, req) - event_type = AuditEventType.USER_SIGNUP if result.is_new_user else AuditEventType.USER_LOGIN + result = await container.auth_service.mobile_register(container.redis, req) await container.audit_service.create_record( - event_type=event_type, + event_type=AuditEventType.USER_SIGNUP, user_id=result.user_id, - metadata={"email": req.email}, + metadata={"endpoint": "register"}, + ) + return result + + +@router.post("/login", response_model=MobileAuthResponse) +async def mobile_login( + req: MobileLoginRequest, + container: Container = Depends(get_container), +) -> MobileAuthResponse: + result = await container.auth_service.mobile_login(container.redis, req) + await container.audit_service.create_record( + event_type=AuditEventType.USER_LOGIN, + user_id=result.user_id, + metadata={"endpoint": "login"}, ) return result diff --git a/app/schema/request/mobile/auth.py b/app/schema/request/mobile/auth.py index 5fdc611..a15c683 100644 --- a/app/schema/request/mobile/auth.py +++ b/app/schema/request/mobile/auth.py @@ -3,7 +3,7 @@ from app.core.config import settings -class MobileAuthRequest(BaseModel): +class MobileAuthBaseRequest(BaseModel): email: EmailStr password: str = Field( ..., @@ -44,6 +44,14 @@ def _strip_required_text(cls, value: object, info: ValidationInfo) -> object: return stripped +class MobileRegisterRequest(MobileAuthBaseRequest): + pass + + +class MobileLoginRequest(MobileAuthBaseRequest): + pass + + diff --git a/app/service/users.py b/app/service/users.py index 9750734..40f2bde 100644 --- a/app/service/users.py +++ b/app/service/users.py @@ -14,7 +14,11 @@ from app.core.config import settings from app.infra.redis import RedisClient -from app.schema.request.mobile.auth import MobileAuthRequest +from app.schema.request.mobile.auth import ( + MobileAuthBaseRequest, + MobileLoginRequest, + MobileRegisterRequest, +) from app.schema.response.mobile.auth import MobileAuthResponse from db.generated import user as user_queries from db.generated import devices as device_queries @@ -48,7 +52,7 @@ def __init__( async def _ensure_device_for_login( self, user_id: uuid.UUID, - req: MobileAuthRequest, + req: MobileAuthBaseRequest, ) -> UserDevice: existing_device = await self.device_querier.get_device_by_id(id=req.device_id) @@ -76,35 +80,61 @@ async def _ensure_device_for_login( raise AppException.internal_error("Failed to create device") return device - async def mobile_register_login( + async def mobile_login( self, redis: RedisClient, - req: MobileAuthRequest, + req: MobileLoginRequest, ) -> MobileAuthResponse: - logger.info("mobile register/login attempt for %s", req.email) + logger.info("mobile_login attempt") existing_user = await self.user_querier.get_user_by_email(email=req.email) - user: User | None = None + if existing_user is None: + logger.warning("login attempt: user_not_found") + raise AppException.unauthorized("User not found; consider registering instead") + if existing_user.blocked: + logger.warning("login attempt: user_blocked user_id=%s", existing_user.id) + raise AppException.forbidden("User is blocked") + if not verify_password(req.password, existing_user.hashed_password or ""): + logger.warning("login attempt: invalid_credentials user_id=%s", existing_user.id) + raise AppException.unauthorized("Invalid credentials") + logger.info("login success user_id=%s", existing_user.id) + return await self._create_mobile_session( + redis=redis, + user=existing_user, + req=req, + is_new_user=False, + ) - is_new_user = False + async def mobile_register( + self, + redis: RedisClient, + req: MobileRegisterRequest, + ) -> MobileAuthResponse: + logger.info("mobile_register attempt") + existing_user = await self.user_querier.get_user_by_email(email=req.email) if existing_user is not None: - if existing_user.blocked: - raise AppException.forbidden("User is blocked") - if not verify_password(req.password, existing_user.hashed_password or ""): - raise AppException.unauthorized("Invalid credentials") - user = existing_user - logger.info("existing user login: %s", req.email) - else: - is_new_user = True - hashed = hash_password(req.password) - logger.info("creating new user for %s", req.email) - user = await self.user_querier.create_user( - email=req.email, hashed_password=hashed - ) - if not user: - raise AppException.internal_error("Failed to create user") - - assert user is not None + logger.warning("register attempt: email_already_in_use") + raise AppException.conflict("Email already in use; please login instead") + hashed = hash_password(req.password) + logger.info("register attempt: creating_new_user") + user = await self.user_querier.create_user(email=req.email, hashed_password=hashed) + if not user: + raise AppException.internal_error("Failed to create user") + logger.info("register success user_id=%s", user.id) + return await self._create_mobile_session( + redis=redis, + user=user, + req=req, + is_new_user=True, + ) + async def _create_mobile_session( + self, + *, + redis: RedisClient, + user: User, + req: MobileAuthBaseRequest, + is_new_user: bool, + ) -> MobileAuthResponse: user_id: uuid.UUID = user.id session_key = constant.RedisKey.UserSessionByUser.value.format(user_id=user_id) @@ -112,8 +142,8 @@ async def mobile_register_login( session_count = await self.session_querier.count_user_sessions(user_id=user_id) if session_count and session_count >= AuthService.SESSION_LIMIT: logger.warning( - "user %s reached session limit %s", - req.email, + "session_limit_reached user_id=%s limit=%s", + user_id, AuthService.SESSION_LIMIT, ) raise AppException.forbidden("Maximum session limit reached") @@ -141,7 +171,7 @@ async def mobile_register_login( access_token = create_acces_mobile_token(str(session.id)) refresh_token = create_refresh_mobile_token(str(session.id)) expiry = Get_expiry_time() - logger.info("created session %s for user %s", session.id, user_id) + logger.info("session_created session_id=%s user_id=%s", session.id, user_id) # Populate Redis auth cache for fast-path validation await SessionService.cache_session_for_auth( diff --git a/docs/problem-fixes/002-mobile-auth-ambiguous-register-login-intent.md b/docs/problem-fixes/002-mobile-auth-ambiguous-register-login-intent.md new file mode 100644 index 0000000..4bb19be --- /dev/null +++ b/docs/problem-fixes/002-mobile-auth-ambiguous-register-login-intent.md @@ -0,0 +1,125 @@ +# 002 - Mobile Auth: Split Register/Login Endpoints + +## Status + +Implemented. The ambiguous `/user/auth/register-login` endpoint is replaced with explicit `/user/auth/register` and `/user/auth/login` endpoints. + +## Problem + +`POST /user/auth/register-login` combined registration and login without an explicit intent. When a client attempted to login with an unknown email, the service silently created an account instead of rejecting the login attempt. + +### Scenario: Email Typo + +1. User A exists with email `user@example.com`. +2. User A attempts to login but typos their email as `usera@example.com`. +3. The system does not find a user with `usera@example.com`. +4. Instead of failing the login, the system creates a new account for `usera@example.com`. +5. User A is now confused: they have two accounts, the login failed mysteriously, and they may think they lost access to their original account. + +### Risky Behaviors + +- **User enumeration via account creation**: An attacker can infer whether an email is registered by observing the response behavior. +- **Accidental duplicate accounts**: Typos in login lead to new account creation, fragmenting user data and confusing users. +- **No UX clarity**: Clients cannot distinguish between "registration succeeded, account created" and "login succeeded, existing account accessed." + +## Root Cause + +The endpoint contained both registration and login logic in a single route and automatically created users when `get_user_by_email` returned `None`. Without explicit endpoints, there was no strict boundary between the two flows. + +## Fix Location + +- `app/schema/request/mobile/auth.py`: introduce `MobileRegisterRequest` and `MobileLoginRequest` (shared base validation). +- `app/service/users.py`: split logic into `AuthService.mobile_register()` and `AuthService.mobile_login()`. +- `app/router/mobile/auth.py`: replace `/user/auth/register-login` with `/user/auth/register` and `/user/auth/login`. +- Tests: + - `tests/unit/test_mobile_auth_request_validation.py` + - `tests/unit/test_mobile_auth_intent_validation.py` + - `tests/e2e/test_mobile_auth_intent_e2e.py` + +## Fix Behavior + +After the fix: + +- `POST /user/auth/login` with unknown email returns `401 Unauthorized` ("User not found; consider registering instead"). +- `POST /user/auth/register` with existing email returns `409 Conflict` ("Email already in use; please login instead"). +- `POST /user/auth/login` with correct credentials succeeds. +- `POST /user/auth/register` with a new email succeeds (`is_new_user = True`). + +## Test Showcase + +Example endpoint tests (in `tests/unit/test_mobile_auth_request_validation.py`): + +```python +@pytest.mark.parametrize("field", ["password", "device_name", "device_type"]) +@pytest.mark.parametrize("value", ["", " "]) +def test_register_login_rejects_empty_required_text_fields( + client: TestClient, + fake_container: FakeContainer, + field: str, + value: str, +) -> None: + for endpoint in ("/user/auth/register", "/user/auth/login"): + payload = _valid_payload() + payload[field] = value + + response = client.post(endpoint, json=payload) + + assert response.status_code == 422 +``` + +Example service tests (in `tests/unit/test_mobile_auth_intent_validation.py`): + +```python +def test_login_with_unknown_email_is_rejected() -> None: + req = MobileLoginRequest( + email="unknown@example.com", + password="validpass123", + device_name="Pixel 8", + device_type="android", + device_id=uuid.uuid4(), + ) + with pytest.raises(HTTPException): + asyncio.run(service.mobile_login(FakeRedis(), req)) + +def test_register_with_existing_email_is_rejected() -> None: + req = MobileRegisterRequest( + email="user@example.com", + password="validpass123", + device_name="Pixel 8", + device_type="android", + device_id=uuid.uuid4(), + ) + with pytest.raises(HTTPException): + asyncio.run(service.mobile_register(FakeRedis(), req)) +``` + +Example e2e test (in `tests/e2e/test_mobile_auth_intent_e2e.py`): + +```python +@pytest.mark.skipif(not os.getenv("MULTAI_RUN_E2E"), reason="E2E disabled") +def test_register_then_login_succeeds_e2e() -> None: + base_url = os.getenv("MULTAI_E2E_BASE_URL", "http://localhost:8000") + email = f"user_{uuid.uuid4()}@example.com" + password = "validpass123" + device_id = str(uuid.uuid4()) + + register_payload = { + "email": email, + "password": password, + "device_name": "TestDevice", + "device_type": "android", + "device_id": device_id, + } + register_response = requests.post(f"{base_url}/user/auth/register", json=register_payload) + assert register_response.status_code == 200 + + login_payload = { + "email": email, + "password": password, + "device_name": "TestDevice", + "device_type": "android", + "device_id": device_id, + } + login_response = requests.post(f"{base_url}/user/auth/login", json=login_payload) + assert login_response.status_code == 200 +``` diff --git a/tests/e2e/test_mobile_auth_intent_e2e.py b/tests/e2e/test_mobile_auth_intent_e2e.py new file mode 100644 index 0000000..2bcf4b6 --- /dev/null +++ b/tests/e2e/test_mobile_auth_intent_e2e.py @@ -0,0 +1,143 @@ +import os +import uuid +import requests +import pytest + + +@pytest.mark.skipif( + not os.getenv("MULTAI_RUN_E2E"), + reason="E2E disabled (set MULTAI_RUN_E2E=1 to run)", +) +class TestMobileAuthEndpointsE2E: + """End-to-end tests for mobile auth register/login endpoints. + + These tests run against a live API. Set MULTAI_RUN_E2E=1 and + MULTAI_E2E_BASE_URL=http://localhost:8000 to enable. + """ + + @pytest.fixture(autouse=True) + def setup(self): + self.base_url = os.getenv("MULTAI_E2E_BASE_URL", "http://localhost:8000") + + def test_login_with_unknown_email_fails(self) -> None: + """Test that login with unknown email returns 401.""" + payload = { + "email": f"nonexistent_{uuid.uuid4()}@example.com", + "password": "anypassword", + "device_name": "TestDevice", + "device_type": "android", + "device_id": str(uuid.uuid4()), + } + response = requests.post(f"{self.base_url}/user/auth/login", json=payload) + assert response.status_code == 401 + assert "not found" in response.json()["detail"].lower() + + def test_register_with_existing_email_fails(self) -> None: + """Test that registration with existing email returns 409.""" + email = f"testuser_{uuid.uuid4()}@example.com" + device_id = str(uuid.uuid4()) + + # First registration succeeds + register_payload = { + "email": email, + "password": "validpass123", + "device_name": "TestDevice", + "device_type": "android", + "device_id": device_id, + } + response1 = requests.post(f"{self.base_url}/user/auth/register", json=register_payload) + assert response1.status_code == 200 + assert response1.json()["is_new_user"] is True + + # Second registration with same email fails + response2 = requests.post(f"{self.base_url}/user/auth/register", json=register_payload) + assert response2.status_code == 409 + assert "already" in response2.json()["detail"].lower() + + def test_register_then_login_succeeds(self) -> None: + """Test full flow: register then login.""" + email = f"user_{uuid.uuid4()}@example.com" + password = "validpass123" + device_id = str(uuid.uuid4()) + + # Register + register_payload = { + "email": email, + "password": password, + "device_name": "TestDevice", + "device_type": "android", + "device_id": device_id, + } + register_response = requests.post(f"{self.base_url}/user/auth/register", json=register_payload) + assert register_response.status_code == 200 + assert register_response.json()["is_new_user"] is True + register_token = register_response.json()["access_token"] + + # Login with same credentials + login_payload = { + "email": email, + "password": password, + "device_name": "TestDevice", + "device_type": "android", + "device_id": device_id, + } + login_response = requests.post(f"{self.base_url}/user/auth/login", json=login_payload) + assert login_response.status_code == 200 + assert login_response.json()["is_new_user"] is False + login_token = login_response.json()["access_token"] + + # Both tokens should work + assert register_token + assert login_token + + def test_login_with_wrong_password_fails(self) -> None: + """Test that login with wrong password returns 401.""" + email = f"user_{uuid.uuid4()}@example.com" + password = "correctpass123" + device_id = str(uuid.uuid4()) + + # Register first + register_payload = { + "email": email, + "password": password, + "device_name": "TestDevice", + "device_type": "android", + "device_id": device_id, + } + requests.post(f"{self.base_url}/user/auth/register", json=register_payload) + + # Try to login with wrong password + login_payload = { + "email": email, + "password": "wrongpassword", + "device_name": "TestDevice", + "device_type": "android", + "device_id": device_id, + } + response = requests.post(f"{self.base_url}/user/auth/login", json=login_payload) + assert response.status_code == 401 + assert "invalid" in response.json()["detail"].lower() + + def test_register_requires_password(self) -> None: + """Test that register requires a password.""" + payload = { + "email": "user@example.com", + "device_name": "TestDevice", + "device_type": "android", + "device_id": str(uuid.uuid4()), + # Missing password + } + response = requests.post(f"{self.base_url}/user/auth/register", json=payload) + assert response.status_code == 422 + + def test_login_requires_device_type(self) -> None: + """Test that login requires device_type.""" + payload = { + "email": "user@example.com", + "password": "validpass123", + "device_name": "TestDevice", + "device_id": str(uuid.uuid4()), + # Missing device_type + } + response = requests.post(f"{self.base_url}/user/auth/login", json=payload) + assert response.status_code == 422 diff --git a/tests/e2e/test_mobile_auth_request_validation_e2e.py b/tests/e2e/test_mobile_auth_request_validation_e2e.py index deeced7..08b67a0 100644 --- a/tests/e2e/test_mobile_auth_request_validation_e2e.py +++ b/tests/e2e/test_mobile_auth_request_validation_e2e.py @@ -15,7 +15,8 @@ BASE_URL = os.getenv("MULTAI_E2E_BASE_URL", "http://localhost:8000").rstrip("/") -REGISTER_LOGIN_URL = f"{BASE_URL}/user/auth/register-login" +REGISTER_URL = f"{BASE_URL}/user/auth/register" +LOGIN_URL = f"{BASE_URL}/user/auth/login" def _valid_payload() -> dict[str, object]: @@ -30,22 +31,25 @@ def _valid_payload() -> dict[str, object]: @pytest.mark.parametrize("field", ["password", "device_name", "device_type"]) @pytest.mark.parametrize("value", ["", " "]) +@pytest.mark.parametrize("url", [REGISTER_URL, LOGIN_URL]) def test_live_register_login_rejects_empty_required_text_fields( field: str, value: str, + url: str, ) -> None: payload = _valid_payload() payload[field] = value - response = httpx.post(REGISTER_LOGIN_URL, json=payload, timeout=10.0) + response = httpx.post(url, json=payload, timeout=10.0) assert response.status_code == 422 -def test_live_register_login_rejects_padded_short_password() -> None: +@pytest.mark.parametrize("url", [REGISTER_URL, LOGIN_URL]) +def test_live_register_login_rejects_padded_short_password(url: str) -> None: payload = _valid_payload() payload["password"] = " a" - response = httpx.post(REGISTER_LOGIN_URL, json=payload, timeout=10.0) + response = httpx.post(url, json=payload, timeout=10.0) assert response.status_code == 422 diff --git a/tests/unit/test_mobile_auth_intent_validation.py b/tests/unit/test_mobile_auth_intent_validation.py new file mode 100644 index 0000000..ab7f497 --- /dev/null +++ b/tests/unit/test_mobile_auth_intent_validation.py @@ -0,0 +1,335 @@ +import asyncio +import logging +import uuid +from datetime import datetime, timezone + +import pytest +from fastapi import HTTPException + +import app.service.users as users_module +from app.core.securite import hash_password +from app.schema.request.mobile.auth import MobileLoginRequest, MobileRegisterRequest +from app.service.session import SessionService +from app.service.users import AuthService + + +class FakeUser: + def __init__(self, email: str, exists: bool = True, password: str = "password123") -> None: + self.id = uuid.uuid4() + self.email = email + self.blocked = False + self.hashed_password = hash_password(password) + self.exists = exists + + +class FakeDevice: + is_invalid_token = False + is_active = True + + +class FakeSession: + def __init__(self) -> None: + self.id = uuid.uuid4() + self.expires_at = datetime.now(timezone.utc) + + +class FakeUserQuerier: + def __init__(self, user: FakeUser) -> None: + self._user = user + self._created_users = {} + + async def get_user_by_email(self, email: str) -> FakeUser | None: + if self._user.exists and self._user.email == email: + return self._user + if email in self._created_users: + return self._created_users[email] + return None + + async def get_user_by_id(self, id: uuid.UUID) -> FakeUser | None: + if self._user.id == id: + return self._user + return None + + async def create_user(self, *, email: str, hashed_password: str) -> FakeUser: + new_user = FakeUser(email=email, exists=True) + new_user.hashed_password = hashed_password + self._created_users[email] = new_user + return new_user + + +class FakeDeviceQuerier: + async def get_device_by_id(self, id: uuid.UUID) -> FakeDevice | None: + return None + + async def create_device(self, arg: object) -> FakeDevice: + return FakeDevice() + + async def activate_device(self, id: uuid.UUID, user_id: uuid.UUID) -> None: + return None + + +class FakeSessionQuerier: + def __init__(self, session: FakeSession) -> None: + self._session = session + + async def count_user_sessions(self, user_id: uuid.UUID) -> int: + return 0 + + async def get_session_by_id(self, id: uuid.UUID) -> FakeSession | None: + return self._session + + async def upsert_session( + self, + *, + user_id: uuid.UUID, + device_id: uuid.UUID, + expires_at: datetime, + ) -> FakeSession: + self._session.expires_at = expires_at + return self._session + + +class FakeRedis: + async def set(self, key: str, value: str, expire: int) -> None: + return None + + +class FakeFaceEmbeddingService: + pass + + +def test_login_with_unknown_email_is_rejected() -> None: + """Test that login with unknown email fails.""" + user = FakeUser(email="user@example.com", exists=True) + session = FakeSession() + service = AuthService( + user_querier=FakeUserQuerier(user), + device_querier=FakeDeviceQuerier(), + session_querier=FakeSessionQuerier(session), + face_embedding_service=FakeFaceEmbeddingService(), + ) + + req = MobileLoginRequest( + email="unknown@example.com", + password="validpass123", + device_name="Pixel 8", + device_type="android", + device_id=uuid.uuid4(), + ) + + with pytest.raises(HTTPException) as exc_info: + asyncio.run(service.mobile_login(FakeRedis(), req)) + assert exc_info.value.status_code == 401 + assert "not found" in exc_info.value.detail.lower() + + +def test_register_with_existing_email_is_rejected() -> None: + """Test that registration with existing email fails.""" + user = FakeUser(email="user@example.com", exists=True) + session = FakeSession() + service = AuthService( + user_querier=FakeUserQuerier(user), + device_querier=FakeDeviceQuerier(), + session_querier=FakeSessionQuerier(session), + face_embedding_service=FakeFaceEmbeddingService(), + ) + + req = MobileRegisterRequest( + email="user@example.com", + password="validpass123", + device_name="Pixel 8", + device_type="android", + device_id=uuid.uuid4(), + ) + + with pytest.raises(HTTPException) as exc_info: + asyncio.run(service.mobile_register(FakeRedis(), req)) + assert exc_info.value.status_code == 409 + assert "already" in exc_info.value.detail.lower() + + +def test_login_with_correct_credentials_succeeds( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Test that login with correct credentials succeeds.""" + user = FakeUser(email="user@example.com", exists=True) + session = FakeSession() + service = AuthService( + user_querier=FakeUserQuerier(user), + device_querier=FakeDeviceQuerier(), + session_querier=FakeSessionQuerier(session), + face_embedding_service=FakeFaceEmbeddingService(), + ) + + req = MobileLoginRequest( + email="user@example.com", + password="password123", + device_name="Pixel 8", + device_type="android", + device_id=uuid.uuid4(), + ) + + async def _noop_cache_session_for_auth(**_: object) -> None: + return None + + monkeypatch.setattr(SessionService, "cache_session_for_auth", _noop_cache_session_for_auth) + monkeypatch.setattr(users_module, "create_acces_mobile_token", lambda _: "access") + monkeypatch.setattr(users_module, "create_refresh_mobile_token", lambda _: "refresh") + monkeypatch.setattr(users_module, "Get_expiry_time", lambda: 3600) + + result = asyncio.run(service.mobile_login(FakeRedis(), req)) + assert result.access_token == "access" + assert result.refresh_token == "refresh" + assert result.is_new_user is False + + +def test_register_with_new_email_succeeds( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Test that registration with new email succeeds.""" + user = FakeUser(email="user@example.com", exists=False) + session = FakeSession() + service = AuthService( + user_querier=FakeUserQuerier(user), + device_querier=FakeDeviceQuerier(), + session_querier=FakeSessionQuerier(session), + face_embedding_service=FakeFaceEmbeddingService(), + ) + + req = MobileRegisterRequest( + email="newuser@example.com", + password="validpass123", + device_name="Pixel 8", + device_type="android", + device_id=uuid.uuid4(), + ) + + async def _noop_cache_session_for_auth(**_: object) -> None: + return None + + monkeypatch.setattr(SessionService, "cache_session_for_auth", _noop_cache_session_for_auth) + monkeypatch.setattr(users_module, "create_acces_mobile_token", lambda _: "access") + monkeypatch.setattr(users_module, "create_refresh_mobile_token", lambda _: "refresh") + monkeypatch.setattr(users_module, "Get_expiry_time", lambda: 3600) + + result = asyncio.run(service.mobile_register(FakeRedis(), req)) + assert result.access_token == "access" + assert result.refresh_token == "refresh" + assert result.is_new_user is True + + +def test_register_then_login_same_device_succeeds( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Test full flow: register then login with same device.""" + user = FakeUser(email="newuser@example.com", exists=False) + session = FakeSession() + service = AuthService( + user_querier=FakeUserQuerier(user), + device_querier=FakeDeviceQuerier(), + session_querier=FakeSessionQuerier(session), + face_embedding_service=FakeFaceEmbeddingService(), + ) + + async def _noop_cache_session_for_auth(**_: object) -> None: + return None + + monkeypatch.setattr(SessionService, "cache_session_for_auth", _noop_cache_session_for_auth) + monkeypatch.setattr(users_module, "create_acces_mobile_token", lambda _: "access") + monkeypatch.setattr(users_module, "create_refresh_mobile_token", lambda _: "refresh") + monkeypatch.setattr(users_module, "Get_expiry_time", lambda: 3600) + + device_id = uuid.uuid4() + password = "validpass123" + + # Register + register_req = MobileRegisterRequest( + email="newuser@example.com", + password=password, + device_name="TestDevice", + device_type="android", + device_id=device_id, + ) + result1 = asyncio.run(service.mobile_register(FakeRedis(), register_req)) + assert result1.is_new_user is True + + # Try to register again (should fail) + with pytest.raises(HTTPException) as exc_info: + asyncio.run(service.mobile_register(FakeRedis(), register_req)) + assert exc_info.value.status_code == 409 + + # Now login + login_req = MobileLoginRequest( + email="newuser@example.com", + password=password, + device_name="TestDevice", + device_type="android", + device_id=device_id, + ) + result2 = asyncio.run(service.mobile_login(FakeRedis(), login_req)) + assert result2.is_new_user is False + + +def test_login_with_wrong_password_fails() -> None: + """Test that login with wrong password fails.""" + user = FakeUser(email="user@example.com", exists=True) + session = FakeSession() + service = AuthService( + user_querier=FakeUserQuerier(user), + device_querier=FakeDeviceQuerier(), + session_querier=FakeSessionQuerier(session), + face_embedding_service=FakeFaceEmbeddingService(), + ) + + req = MobileLoginRequest( + email="user@example.com", + password="wrongpassword", + device_name="Pixel 8", + device_type="android", + device_id=uuid.uuid4(), + ) + + with pytest.raises(HTTPException) as exc_info: + asyncio.run(service.mobile_login(FakeRedis(), req)) + assert exc_info.value.status_code == 401 + assert "invalid" in exc_info.value.detail.lower() + + +def test_login_logs_correctly( + caplog: pytest.LogCaptureFixture, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Test that login emits audit-friendly logs without email.""" + caplog.set_level(logging.INFO, logger="multAI") + + user = FakeUser(email="user@example.com", exists=True) + session = FakeSession() + service = AuthService( + user_querier=FakeUserQuerier(user), + device_querier=FakeDeviceQuerier(), + session_querier=FakeSessionQuerier(session), + face_embedding_service=FakeFaceEmbeddingService(), + ) + + req = MobileLoginRequest( + email="USER@Example.COM", + password="password123", + device_name="Pixel 8", + device_type="android", + device_id=uuid.uuid4(), + ) + + async def _noop_cache_session_for_auth(**_: object) -> None: + return None + + monkeypatch.setattr(SessionService, "cache_session_for_auth", _noop_cache_session_for_auth) + monkeypatch.setattr(users_module, "create_acces_mobile_token", lambda _: "access") + monkeypatch.setattr(users_module, "create_refresh_mobile_token", lambda _: "refresh") + monkeypatch.setattr(users_module, "Get_expiry_time", lambda: 3600) + + asyncio.run(service.mobile_login(FakeRedis(), req)) + + assert "mobile_login attempt" in caplog.text + assert "login success user_id=" in caplog.text + assert "session_id=" in caplog.text + assert "user@example.com" not in caplog.text diff --git a/tests/unit/test_mobile_auth_request_validation.py b/tests/unit/test_mobile_auth_request_validation.py index 2fd48a6..6660d8a 100644 --- a/tests/unit/test_mobile_auth_request_validation.py +++ b/tests/unit/test_mobile_auth_request_validation.py @@ -7,20 +7,21 @@ from app.container import get_container from app.main import app -from app.schema.request.mobile.auth import MobileAuthRequest +from app.schema.request.mobile.auth import MobileLoginRequest, MobileRegisterRequest from app.schema.response.mobile.auth import MobileAuthResponse class FakeAuthService: def __init__(self) -> None: - self.received_request: MobileAuthRequest | None = None + self.register_request: MobileRegisterRequest | None = None + self.login_request: MobileLoginRequest | None = None - async def mobile_register_login( + async def mobile_register( self, redis: object, - req: MobileAuthRequest, + req: MobileRegisterRequest, ) -> MobileAuthResponse: - self.received_request = req + self.register_request = req return MobileAuthResponse( access_token="access", refresh_token="refresh", @@ -30,6 +31,21 @@ async def mobile_register_login( is_new_user=True, ) + async def mobile_login( + self, + redis: object, + req: MobileLoginRequest, + ) -> MobileAuthResponse: + self.login_request = req + return MobileAuthResponse( + access_token="access", + refresh_token="refresh", + session_id=str(uuid.uuid4()), + expires_in=3600, + user_id=uuid.uuid4(), + is_new_user=False, + ) + class FakeAuditService: async def create_record(self, **kwargs: Any) -> None: @@ -75,46 +91,58 @@ def test_register_login_rejects_empty_required_text_fields( field: str, value: str, ) -> None: - payload = _valid_payload() - payload[field] = value + for endpoint, attr in ( + ("/user/auth/register", "register_request"), + ("/user/auth/login", "login_request"), + ): + payload = _valid_payload() + payload[field] = value - response = client.post("/user/auth/register-login", json=payload) + response = client.post(endpoint, json=payload) - assert response.status_code == 422 - assert fake_container.auth_service.received_request is None + assert response.status_code == 422 + assert getattr(fake_container.auth_service, attr) is None def test_register_login_passes_normalized_input_to_service( client: TestClient, fake_container: FakeContainer, ) -> None: - payload = _valid_payload() - payload.update( - { - "email": " USER@Example.COM ", - "device_name": " Pixel 8 ", - "device_type": " ANDROID ", - } - ) + for endpoint, attr in ( + ("/user/auth/register", "register_request"), + ("/user/auth/login", "login_request"), + ): + payload = _valid_payload() + payload.update( + { + "email": " USER@Example.COM ", + "device_name": " Pixel 8 ", + "device_type": " ANDROID ", + } + ) - response = client.post("/user/auth/register-login", json=payload) + response = client.post(endpoint, json=payload) - assert response.status_code == 200 - req = fake_container.auth_service.received_request - assert req is not None - assert req.email == "user@example.com" - assert req.device_name == "Pixel 8" - assert req.device_type == "android" + assert response.status_code == 200 + req = getattr(fake_container.auth_service, attr) + assert req is not None + assert req.email == "user@example.com" + assert req.device_name == "Pixel 8" + assert req.device_type == "android" def test_register_login_password_length_is_checked_after_trimming( client: TestClient, fake_container: FakeContainer, ) -> None: - payload = _valid_payload() - payload["password"] = " a" + for endpoint, attr in ( + ("/user/auth/register", "register_request"), + ("/user/auth/login", "login_request"), + ): + payload = _valid_payload() + payload["password"] = " a" - response = client.post("/user/auth/register-login", json=payload) + response = client.post(endpoint, json=payload) - assert response.status_code == 422 - assert fake_container.auth_service.received_request is None + assert response.status_code == 422 + assert getattr(fake_container.auth_service, attr) is None From b54e2c12beb279e2e2921580392241ad3b40b7d9 Mon Sep 17 00:00:00 2001 From: bouhamza abderrahmane Date: Mon, 1 Jun 2026 13:20:07 +0100 Subject: [PATCH 03/11] fix(mobile-auth): bound email length to 255 characters at schema level --- app/schema/request/mobile/auth.py | 2 +- .../test_mobile_auth_request_validation.py | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/app/schema/request/mobile/auth.py b/app/schema/request/mobile/auth.py index a15c683..2204311 100644 --- a/app/schema/request/mobile/auth.py +++ b/app/schema/request/mobile/auth.py @@ -4,7 +4,7 @@ class MobileAuthBaseRequest(BaseModel): - email: EmailStr + email: EmailStr = Field(..., max_length=255) password: str = Field( ..., min_length=settings.MOBILE_AUTH_PASSWORD_MIN_LEN, diff --git a/tests/unit/test_mobile_auth_request_validation.py b/tests/unit/test_mobile_auth_request_validation.py index 6660d8a..4e8f3f6 100644 --- a/tests/unit/test_mobile_auth_request_validation.py +++ b/tests/unit/test_mobile_auth_request_validation.py @@ -146,3 +146,23 @@ def test_register_login_password_length_is_checked_after_trimming( assert response.status_code == 422 assert getattr(fake_container.auth_service, attr) is None + + +def test_register_login_rejects_oversized_email( + client: TestClient, + fake_container: FakeContainer, +) -> None: + for endpoint, attr in ( + ("/user/auth/register", "register_request"), + ("/user/auth/login", "login_request"), + ): + payload = _valid_payload() + # Create an email address with a length > 255 chars + # username part is 250 'a's, domain is "@example.com" + payload["email"] = "a" * 250 + "@example.com" + + response = client.post(endpoint, json=payload) + + assert response.status_code == 422 + assert getattr(fake_container.auth_service, attr) is None + From 916aa2bea612ace362ecefaf90194ea52c765540 Mon Sep 17 00:00:00 2001 From: bouhamza abderrahmane Date: Mon, 1 Jun 2026 13:22:39 +0100 Subject: [PATCH 04/11] fix(mobile-auth): handle concurrent register race condition and return 409 --- app/core/exceptions.py | 8 +++- app/service/users.py | 10 ++-- .../test_mobile_auth_intent_validation.py | 46 +++++++++++++++++++ 3 files changed, 60 insertions(+), 4 deletions(-) diff --git a/app/core/exceptions.py b/app/core/exceptions.py index ddf0d36..41b03e3 100644 --- a/app/core/exceptions.py +++ b/app/core/exceptions.py @@ -104,11 +104,17 @@ class DBExceptionImpl(DBException): @staticmethod def handle_unique_violation(exc: Exception) -> HTTPException: constraint = getattr(exc, "constraint_name", None) - if constraint == "staff_users_email_key": + err_msg = str(exc).lower() + if constraint == "staff_users_email_key" or "staff_users_email_key" in err_msg: return HTTPException( status_code=409, detail="Staff user with this email already exists" ) + if constraint in ("users_email_key", "idx_users_email") or "idx_users_email" in err_msg or "users_email_key" in err_msg: + return HTTPException( + status_code=409, + detail="Email already in use; please login instead" + ) return HTTPException(status_code=409, detail="Resource already exists") @staticmethod diff --git a/app/service/users.py b/app/service/users.py index 40f2bde..90652a6 100644 --- a/app/service/users.py +++ b/app/service/users.py @@ -116,9 +116,13 @@ async def mobile_register( raise AppException.conflict("Email already in use; please login instead") hashed = hash_password(req.password) logger.info("register attempt: creating_new_user") - user = await self.user_querier.create_user(email=req.email, hashed_password=hashed) - if not user: - raise AppException.internal_error("Failed to create user") + try: + user = await self.user_querier.create_user(email=req.email, hashed_password=hashed) + if not user: + raise AppException.internal_error("Failed to create user") + except Exception as exc: + logger.error("Failed to create user: %s", exc) + raise DBException.handle(exc) logger.info("register success user_id=%s", user.id) return await self._create_mobile_session( redis=redis, diff --git a/tests/unit/test_mobile_auth_intent_validation.py b/tests/unit/test_mobile_auth_intent_validation.py index ab7f497..ed59f9b 100644 --- a/tests/unit/test_mobile_auth_intent_validation.py +++ b/tests/unit/test_mobile_auth_intent_validation.py @@ -333,3 +333,49 @@ async def _noop_cache_session_for_auth(**_: object) -> None: assert "login success user_id=" in caplog.text assert "session_id=" in caplog.text assert "user@example.com" not in caplog.text + + +def test_register_concurrent_signup_integrity_error() -> None: + """Test that concurrent signup IntegrityError is caught and raised as 409.""" + from typing import Any + from sqlalchemy.exc import IntegrityError + + class FakeOrigException(Exception): + sqlstate = "23505" + constraint_name = "idx_users_email" + + user = FakeUser(email="user@example.com", exists=False) + session = FakeSession() + + async def _raise_integrity_error(*args: Any, **kwargs: Any) -> Any: + raise IntegrityError( + statement="INSERT INTO users", + params={}, + orig=FakeOrigException("duplicate key value violates unique constraint idx_users_email") + ) + + user_querier = FakeUserQuerier(user) + # Stub create_user to raise IntegrityError + user_querier.create_user = _raise_integrity_error # type: ignore + + service = AuthService( + user_querier=user_querier, + device_querier=FakeDeviceQuerier(), + session_querier=FakeSessionQuerier(session), + face_embedding_service=FakeFaceEmbeddingService(), + ) + + req = MobileRegisterRequest( + email="newuser@example.com", + password="validpass123", + device_name="Pixel 8", + device_type="android", + device_id=uuid.uuid4(), + ) + + with pytest.raises(HTTPException) as exc_info: + asyncio.run(service.mobile_register(FakeRedis(), req)) + + assert exc_info.value.status_code == 409 + assert "already in use" in exc_info.value.detail.lower() + From be306b261bbcca6ba3e6fa1c5e17690899e68f15 Mon Sep 17 00:00:00 2001 From: bouhamza abderrahmane Date: Mon, 1 Jun 2026 13:33:58 +0100 Subject: [PATCH 05/11] fix(security): prevent Bcrypt 72-byte truncation using SHA-256 pre-hashing --- app/core/securite.py | 14 +++++++++----- tests/unit/test_bcrypt_truncation.py | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) create mode 100644 tests/unit/test_bcrypt_truncation.py diff --git a/app/core/securite.py b/app/core/securite.py index af9db00..002ae27 100644 --- a/app/core/securite.py +++ b/app/core/securite.py @@ -1,3 +1,5 @@ +import base64 +import hashlib from datetime import datetime, timedelta, timezone from typing import Any, Literal import jwt @@ -18,14 +20,16 @@ def _normalize_password(password: str) -> bytes: def hash_password(password: str) -> str: - normalized = _normalize_password(password) - logger.debug("hashing password (normalized %s bytes)", len(normalized)) - return pwd_context.hash(normalized) + # Use SHA-256 pre-hashing to overcome bcrypt's 72-byte limit + pre_hashed = base64.b64encode(hashlib.sha256(password.encode("utf-8")).digest()) + logger.debug("hashing password (pre-hashed %s bytes)", len(pre_hashed)) + return pwd_context.hash(pre_hashed) def verify_password(password: str, hashed: str) -> bool: - normalized = _normalize_password(password) - result = pwd_context.verify(normalized, hashed) + # Verify using the SHA-256 pre-hashed format + pre_hashed = base64.b64encode(hashlib.sha256(password.encode("utf-8")).digest()) + result = pwd_context.verify(pre_hashed, hashed) logger.debug("password verification result: %s", result) return result diff --git a/tests/unit/test_bcrypt_truncation.py b/tests/unit/test_bcrypt_truncation.py new file mode 100644 index 0000000..2aaaffc --- /dev/null +++ b/tests/unit/test_bcrypt_truncation.py @@ -0,0 +1,21 @@ +from passlib.context import CryptContext +from app.core.securite import hash_password, verify_password + + +def test_bcrypt_72_byte_truncation_is_prevented() -> None: + # Create two passwords that are longer than 72 bytes and share the same 72-byte prefix + prefix = "a" * 72 + pass1 = prefix + "X" + pass2 = prefix + "Y" + + # Hash both passwords + hash1 = hash_password(pass1) + hash2 = hash_password(pass2) + + # They must produce different hashes (or at least pass2 must not verify with hash1) + assert not verify_password(pass2, hash1) + assert not verify_password(pass1, hash2) + + # Each must verify with its own hash + assert verify_password(pass1, hash1) + assert verify_password(pass2, hash2) From 0c67f16d178228c089c20a7d8b811833bf3047c9 Mon Sep 17 00:00:00 2001 From: bouhamza abderrahmane Date: Mon, 1 Jun 2026 13:36:11 +0100 Subject: [PATCH 06/11] fix(security): enforce password complexity rules at signup level --- app/schema/request/mobile/auth.py | 13 +- tests/unit/test_mobile_auth_email_logging.py | 121 ++++++++++++++++++ .../test_mobile_auth_intent_validation.py | 16 +-- .../test_mobile_auth_request_validation.py | 42 +++++- 4 files changed, 182 insertions(+), 10 deletions(-) create mode 100644 tests/unit/test_mobile_auth_email_logging.py diff --git a/app/schema/request/mobile/auth.py b/app/schema/request/mobile/auth.py index 2204311..b272f2b 100644 --- a/app/schema/request/mobile/auth.py +++ b/app/schema/request/mobile/auth.py @@ -45,7 +45,18 @@ def _strip_required_text(cls, value: object, info: ValidationInfo) -> object: class MobileRegisterRequest(MobileAuthBaseRequest): - pass + @field_validator("password") + @classmethod + def _validate_password_complexity(cls, value: str) -> str: + if not any(c.isupper() for c in value): + raise ValueError("Password must contain at least one uppercase letter") + if not any(c.islower() for c in value): + raise ValueError("Password must contain at least one lowercase letter") + if not any(c.isdigit() for c in value): + raise ValueError("Password must contain at least one digit") + if not any(not c.isalnum() for c in value): + raise ValueError("Password must contain at least one special character") + return value class MobileLoginRequest(MobileAuthBaseRequest): diff --git a/tests/unit/test_mobile_auth_email_logging.py b/tests/unit/test_mobile_auth_email_logging.py new file mode 100644 index 0000000..3dbe421 --- /dev/null +++ b/tests/unit/test_mobile_auth_email_logging.py @@ -0,0 +1,121 @@ +import asyncio +import logging +import uuid +from datetime import datetime, timezone + +import pytest + +import app.service.users as users_module +from app.schema.request.mobile.auth import MobileRegisterRequest +from app.service.session import SessionService +from app.service.users import AuthService + + +class FakeUser: + def __init__(self, email: str) -> None: + self.id = uuid.uuid4() + self.email = email + self.blocked = False + self.hashed_password = "hashed" + + +class FakeDevice: + is_invalid_token = False + is_active = True + + +class FakeSession: + def __init__(self) -> None: + self.id = uuid.uuid4() + self.expires_at = datetime.now(timezone.utc) + + +class FakeUserQuerier: + def __init__(self, user: FakeUser) -> None: + self._user = user + + async def get_user_by_email(self, email: str) -> FakeUser | None: + return None + + async def create_user(self, *, email: str, hashed_password: str) -> FakeUser: + self._user.email = email + self._user.hashed_password = hashed_password + return self._user + + +class FakeDeviceQuerier: + async def get_device_by_id(self, id: uuid.UUID) -> FakeDevice | None: + return None + + async def create_device(self, arg: object) -> FakeDevice: + return FakeDevice() + + +class FakeSessionQuerier: + def __init__(self, session: FakeSession) -> None: + self._session = session + + async def count_user_sessions(self, user_id: uuid.UUID) -> int: + return 0 + + async def upsert_session( + self, + *, + user_id: uuid.UUID, + device_id: uuid.UUID, + expires_at: datetime, + ) -> FakeSession: + self._session.expires_at = expires_at + return self._session + + +class FakeRedis: + async def set(self, key: str, value: str, expire: int) -> None: + return None + + +class FakeFaceEmbeddingService: + pass + + +def test_mobile_register_logs_without_plaintext_email( + caplog: pytest.LogCaptureFixture, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Verify that plaintext email is never logged; use user_id instead.""" + caplog.set_level(logging.INFO, logger="multAI") + + user = FakeUser(email="user@example.com") + session = FakeSession() + service = AuthService( + user_querier=FakeUserQuerier(user), + device_querier=FakeDeviceQuerier(), + session_querier=FakeSessionQuerier(session), + face_embedding_service=FakeFaceEmbeddingService(), + ) + + req = MobileRegisterRequest( + email="USER@Example.COM", + password="ValidPass@123", + device_name="Pixel 8", + device_type="android", + device_id=uuid.uuid4(), + ) + + async def _noop_cache_session_for_auth(**_: object) -> None: + return None + + monkeypatch.setattr(SessionService, "cache_session_for_auth", _noop_cache_session_for_auth) + monkeypatch.setattr(users_module, "create_acces_mobile_token", lambda _: "access") + monkeypatch.setattr(users_module, "create_refresh_mobile_token", lambda _: "refresh") + monkeypatch.setattr(users_module, "Get_expiry_time", lambda: 3600) + + asyncio.run(service.mobile_register(FakeRedis(), req)) + + # Verify no plaintext email in logs + assert req.email not in caplog.text + assert "user@example.com" not in caplog.text + # Verify user_id is logged instead + assert "user_id=" in caplog.text + assert "session_id=" in caplog.text + diff --git a/tests/unit/test_mobile_auth_intent_validation.py b/tests/unit/test_mobile_auth_intent_validation.py index ed59f9b..563df05 100644 --- a/tests/unit/test_mobile_auth_intent_validation.py +++ b/tests/unit/test_mobile_auth_intent_validation.py @@ -14,7 +14,7 @@ class FakeUser: - def __init__(self, email: str, exists: bool = True, password: str = "password123") -> None: + def __init__(self, email: str, exists: bool = True, password: str = "ValidPass@123") -> None: self.id = uuid.uuid4() self.email = email self.blocked = False @@ -111,7 +111,7 @@ def test_login_with_unknown_email_is_rejected() -> None: req = MobileLoginRequest( email="unknown@example.com", - password="validpass123", + password="ValidPass@123", device_name="Pixel 8", device_type="android", device_id=uuid.uuid4(), @@ -136,7 +136,7 @@ def test_register_with_existing_email_is_rejected() -> None: req = MobileRegisterRequest( email="user@example.com", - password="validpass123", + password="ValidPass@123", device_name="Pixel 8", device_type="android", device_id=uuid.uuid4(), @@ -163,7 +163,7 @@ def test_login_with_correct_credentials_succeeds( req = MobileLoginRequest( email="user@example.com", - password="password123", + password="ValidPass@123", device_name="Pixel 8", device_type="android", device_id=uuid.uuid4(), @@ -198,7 +198,7 @@ def test_register_with_new_email_succeeds( req = MobileRegisterRequest( email="newuser@example.com", - password="validpass123", + password="ValidPass@123", device_name="Pixel 8", device_type="android", device_id=uuid.uuid4(), @@ -240,7 +240,7 @@ async def _noop_cache_session_for_auth(**_: object) -> None: monkeypatch.setattr(users_module, "Get_expiry_time", lambda: 3600) device_id = uuid.uuid4() - password = "validpass123" + password = "ValidPass@123" # Register register_req = MobileRegisterRequest( @@ -313,7 +313,7 @@ def test_login_logs_correctly( req = MobileLoginRequest( email="USER@Example.COM", - password="password123", + password="ValidPass@123", device_name="Pixel 8", device_type="android", device_id=uuid.uuid4(), @@ -367,7 +367,7 @@ async def _raise_integrity_error(*args: Any, **kwargs: Any) -> Any: req = MobileRegisterRequest( email="newuser@example.com", - password="validpass123", + password="ValidPass@123", device_name="Pixel 8", device_type="android", device_id=uuid.uuid4(), diff --git a/tests/unit/test_mobile_auth_request_validation.py b/tests/unit/test_mobile_auth_request_validation.py index 4e8f3f6..b9bf117 100644 --- a/tests/unit/test_mobile_auth_request_validation.py +++ b/tests/unit/test_mobile_auth_request_validation.py @@ -76,7 +76,7 @@ def client(fake_container: FakeContainer) -> Iterator[TestClient]: def _valid_payload() -> dict[str, object]: return { "email": "USER@Example.COM", - "password": "validpass123", + "password": "ValidPass@123", "device_name": "Pixel 8", "device_type": "android", "device_id": str(uuid.uuid4()), @@ -166,3 +166,43 @@ def test_register_login_rejects_oversized_email( assert response.status_code == 422 assert getattr(fake_container.auth_service, attr) is None + +def test_register_enforces_password_complexity( + client: TestClient, + fake_container: FakeContainer, +) -> None: + # Valid complex password + payload = _valid_payload() + payload["password"] = "P@ssword123" + response = client.post("/user/auth/register", json=payload) + assert response.status_code == 200 + assert fake_container.auth_service.register_request is not None + + # Invalid passwords lacking different criteria + invalid_passwords = [ + "p@ssword123", # missing uppercase + "P@SSWORD123", # missing lowercase + "P@sswordabc", # missing digit + "Password123", # missing special char + ] + for pw in invalid_passwords: + fake_container.auth_service.register_request = None + payload = _valid_payload() + payload["password"] = pw + response = client.post("/user/auth/register", json=payload) + assert response.status_code == 422 + assert fake_container.auth_service.register_request is None + + +def test_login_does_not_enforce_password_complexity( + client: TestClient, + fake_container: FakeContainer, +) -> None: + # Simple password should still be allowed to attempt log in + payload = _valid_payload() + payload["password"] = "Password123" # missing special character + response = client.post("/user/auth/login", json=payload) + assert response.status_code == 200 + assert fake_container.auth_service.login_request is not None + + From c5183a25d8f7de0272ec6db0f6fb0e6ac543e7e7 Mon Sep 17 00:00:00 2001 From: bouhamza abderrahmane Date: Mon, 1 Jun 2026 14:31:04 +0100 Subject: [PATCH 07/11] Fix mobile auth validation and rate limiting --- app/core/config.py | 4 + app/core/exceptions.py | 8 ++ app/core/securite.py | 14 +-- app/infra/redis.py | 4 + app/router/mobile/auth.py | 24 ++++- app/schema/response/web/auth.py | 7 +- app/service/users.py | 64 ++++++++++++- app/worker/audit/settings.py | 6 +- app/worker/photo_worker/settings.py | 7 +- ...1-mobile-auth-required-field-validation.md | 76 ++++++++------- .../003-mobile-auth-email-length-bound.md | 36 +++++++ .../004-mobile-auth-concurrent-signup-race.md | 37 ++++++++ .../005-mobile-auth-bcrypt-truncation.md | 40 ++++++++ ...-mobile-auth-password-complexity-policy.md | 42 +++++++++ ...07-mobile-auth-rate-limiting-bruteforce.md | 42 +++++++++ docs/problem-fixes/README.md | 8 +- tests/e2e/test_mobile_auth_intent_e2e.py | 68 +++++++++++--- ...test_mobile_auth_request_validation_e2e.py | 2 +- tests/unit/test_mobile_auth_email_logging.py | 14 ++- .../test_mobile_auth_intent_validation.py | 13 +++ tests/unit/test_mobile_auth_rate_limiting.py | 94 +++++++++++++++++++ .../test_mobile_auth_request_validation.py | 22 +++++ 22 files changed, 556 insertions(+), 76 deletions(-) create mode 100644 docs/problem-fixes/003-mobile-auth-email-length-bound.md create mode 100644 docs/problem-fixes/004-mobile-auth-concurrent-signup-race.md create mode 100644 docs/problem-fixes/005-mobile-auth-bcrypt-truncation.md create mode 100644 docs/problem-fixes/006-mobile-auth-password-complexity-policy.md create mode 100644 docs/problem-fixes/007-mobile-auth-rate-limiting-bruteforce.md create mode 100644 tests/unit/test_mobile_auth_rate_limiting.py diff --git a/app/core/config.py b/app/core/config.py index 22eda29..2f9087f 100644 --- a/app/core/config.py +++ b/app/core/config.py @@ -41,6 +41,10 @@ class Settings(BaseSettings): MOBILE_AUTH_PASSWORD_MAX_LEN: int = 128 MOBILE_AUTH_DEVICE_NAME_MAX_LEN: int = 64 MOBILE_AUTH_DEVICE_TYPE_MAX_LEN: int = 32 + # Rate Limit Settings + RATE_LIMIT_LOGIN_MAX_ATTEMPTS: int = 5 + RATE_LIMIT_LOGIN_WINDOW_SECONDS: int = 60 + TRUST_PROXY_HEADERS: bool = True # Admin list defaults ADMIN_USERS_DEFAULT_LIMIT: int = 20 ADMIN_USERS_MAX_LIMIT: int = 100 diff --git a/app/core/exceptions.py b/app/core/exceptions.py index 41b03e3..1546568 100644 --- a/app/core/exceptions.py +++ b/app/core/exceptions.py @@ -34,6 +34,14 @@ def internal_error(detail: str = "Internal server error") -> HTTPException: def conflict(detail: str = "Conflict") -> HTTPException: return HTTPException(status_code=409, detail=detail) + @staticmethod + def too_many_requests( + detail: str = "Too many requests. Please try again later.", + retry_after: int | None = None, + ) -> HTTPException: + headers = {"Retry-After": str(retry_after)} if retry_after is not None else None + return HTTPException(status_code=429, detail=detail, headers=headers) + @staticmethod def storage_error(detail: str = "Storage operation failed") -> HTTPException: return HTTPException(status_code=500, detail=detail) diff --git a/app/core/securite.py b/app/core/securite.py index 002ae27..77342a3 100644 --- a/app/core/securite.py +++ b/app/core/securite.py @@ -4,19 +4,13 @@ from typing import Any, Literal import jwt from passlib.context import CryptContext -from pydantic import BaseModel +from pydantic import BaseModel, ConfigDict import pyotp from app.core.config import settings from app.core.exceptions import AppException from app.core.logger import logger - pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto") -_BCRYPT_MAX_LEN = 72 - - -def _normalize_password(password: str) -> bytes: - return password.encode("utf-8")[:_BCRYPT_MAX_LEN] def hash_password(password: str) -> str: @@ -132,15 +126,13 @@ def generate_Acces_token_stuff(user_id: str, role: str) -> str: class StaffJWTPayload(BaseModel): + model_config = ConfigDict(frozen=True) + sub: str role: str type: Literal["access", "refresh"] exp: int - class Config: - frozen = True - # immutable class - def create_access_staff_token(staff_id: str, role: str) -> str: """ diff --git a/app/infra/redis.py b/app/infra/redis.py index 0ce5e68..acfaa28 100644 --- a/app/infra/redis.py +++ b/app/infra/redis.py @@ -58,6 +58,10 @@ async def expire(self, key: RedisKey | str, seconds: int) -> bool: result = await self._client.expire(key, seconds) return int(cast(int, result)) == 1 + async def incr(self, key: RedisKey | str) -> int: + result = await self._client.incr(key) + return int(cast(int, result)) + async def sadd(self, key: RedisKey | str, *values: str) -> int: result = self._client.sadd(key, *values) diff --git a/app/router/mobile/auth.py b/app/router/mobile/auth.py index 2af4ca8..03fa0fc 100644 --- a/app/router/mobile/auth.py +++ b/app/router/mobile/auth.py @@ -1,9 +1,10 @@ from typing import Optional -from fastapi import APIRouter, Depends +from fastapi import APIRouter, Depends, Request from uuid import UUID from app.container import get_container, Container +from app.core.config import settings from app.core.constant import AuditEventType from app.deps.token_auth import MobileUserSchema, get_current_mobile_user @@ -19,12 +20,27 @@ router = APIRouter(prefix="/auth") +def _get_client_ip(request: Request) -> str | None: + if settings.TRUST_PROXY_HEADERS: + forwarded_for = request.headers.get("x-forwarded-for") + if forwarded_for: + return forwarded_for.split(",", maxsplit=1)[0].strip() or None + + real_ip = request.headers.get("x-real-ip") + if real_ip: + return real_ip.strip() or None + + return request.client.host if request.client else None + + @router.post("/register", response_model=MobileAuthResponse) async def mobile_register( req: MobileRegisterRequest, + request: Request, container: Container = Depends(get_container), ) -> MobileAuthResponse: - result = await container.auth_service.mobile_register(container.redis, req) + client_ip = _get_client_ip(request) + result = await container.auth_service.mobile_register(container.redis, req, client_ip=client_ip) await container.audit_service.create_record( event_type=AuditEventType.USER_SIGNUP, user_id=result.user_id, @@ -36,9 +52,11 @@ async def mobile_register( @router.post("/login", response_model=MobileAuthResponse) async def mobile_login( req: MobileLoginRequest, + request: Request, container: Container = Depends(get_container), ) -> MobileAuthResponse: - result = await container.auth_service.mobile_login(container.redis, req) + client_ip = _get_client_ip(request) + result = await container.auth_service.mobile_login(container.redis, req, client_ip=client_ip) await container.audit_service.create_record( event_type=AuditEventType.USER_LOGIN, user_id=result.user_id, diff --git a/app/schema/response/web/auth.py b/app/schema/response/web/auth.py index f1fefed..cac08e7 100644 --- a/app/schema/response/web/auth.py +++ b/app/schema/response/web/auth.py @@ -1,15 +1,14 @@ import uuid -from pydantic import BaseModel +from pydantic import BaseModel, ConfigDict class WebAuthResponse(BaseModel): + model_config = ConfigDict(from_attributes=True) + access_token: str user_id: uuid.UUID role: str - class Config: - from_attributes = True - diff --git a/app/service/users.py b/app/service/users.py index 90652a6..2237d8c 100644 --- a/app/service/users.py +++ b/app/service/users.py @@ -1,5 +1,8 @@ from datetime import datetime, timedelta, timezone import uuid +from typing import Optional + +from sqlalchemy.exc import SQLAlchemyError from app.core.exceptions import AppException, DBException from app.core.securite import ( @@ -84,8 +87,26 @@ async def mobile_login( self, redis: RedisClient, req: MobileLoginRequest, + client_ip: Optional[str] = None, ) -> MobileAuthResponse: logger.info("mobile_login attempt") + max_attempts = settings.RATE_LIMIT_LOGIN_MAX_ATTEMPTS + window = settings.RATE_LIMIT_LOGIN_WINDOW_SECONDS + + if client_ip: + await self.check_rate_limit( + redis, + f"rate:ip:{client_ip}", + max_attempts, + window, + ) + await self.check_rate_limit( + redis, + f"rate:email:{req.email}", + max_attempts, + window, + ) + existing_user = await self.user_querier.get_user_by_email(email=req.email) if existing_user is None: logger.warning("login attempt: user_not_found") @@ -108,8 +129,26 @@ async def mobile_register( self, redis: RedisClient, req: MobileRegisterRequest, + client_ip: Optional[str] = None, ) -> MobileAuthResponse: logger.info("mobile_register attempt") + max_attempts = settings.RATE_LIMIT_LOGIN_MAX_ATTEMPTS + window = settings.RATE_LIMIT_LOGIN_WINDOW_SECONDS + + if client_ip: + await self.check_rate_limit( + redis, + f"rate:ip:{client_ip}", + max_attempts, + window, + ) + await self.check_rate_limit( + redis, + f"rate:email:{req.email}", + max_attempts, + window, + ) + existing_user = await self.user_querier.get_user_by_email(email=req.email) if existing_user is not None: logger.warning("register attempt: email_already_in_use") @@ -120,7 +159,7 @@ async def mobile_register( user = await self.user_querier.create_user(email=req.email, hashed_password=hashed) if not user: raise AppException.internal_error("Failed to create user") - except Exception as exc: + except SQLAlchemyError as exc: logger.error("Failed to create user: %s", exc) raise DBException.handle(exc) logger.info("register success user_id=%s", user.id) @@ -425,3 +464,26 @@ async def find_closest_user(self, *, embedding_literal: str) -> ClosestUserMatch if row is None or row.distance is None: return None return ClosestUserMatch(user_id=row.id, distance=float(row.distance)) + + async def check_rate_limit( + self, + redis: RedisClient, + key: str, + max_requests: int, + window_seconds: int, + ) -> None: + """Enforce rate limiting using Redis INCR + EXPIRE. + + Increments a counter for ``key``. On the first increment the key + is given a TTL of ``window_seconds`` so the window resets + automatically. If the counter exceeds ``max_requests`` a 429 + response is raised with a ``Retry-After`` header. + """ + current_count = await redis.incr(key) + if current_count == 1: + await redis.expire(key, window_seconds) + if current_count > max_requests: + raise AppException.too_many_requests( + "Too many requests. Please try again later.", + retry_after=window_seconds, + ) diff --git a/app/worker/audit/settings.py b/app/worker/audit/settings.py index 6d081c8..c732ca5 100644 --- a/app/worker/audit/settings.py +++ b/app/worker/audit/settings.py @@ -1,12 +1,10 @@ from __future__ import annotations -from pydantic_settings import BaseSettings +from pydantic_settings import BaseSettings, SettingsConfigDict class AuditWorkerSettings(BaseSettings): - - class Config: - env_prefix = "AUDIT_" + model_config = SettingsConfigDict(env_prefix="AUDIT_") settings = AuditWorkerSettings() # type: ignore diff --git a/app/worker/photo_worker/settings.py b/app/worker/photo_worker/settings.py index b632fc9..7f09f28 100644 --- a/app/worker/photo_worker/settings.py +++ b/app/worker/photo_worker/settings.py @@ -1,16 +1,15 @@ from __future__ import annotations from pydantic import Field -from pydantic_settings import BaseSettings +from pydantic_settings import BaseSettings, SettingsConfigDict class PhotoWorkerSettings(BaseSettings): + model_config = SettingsConfigDict(env_prefix="PHOTO_WORKER_") + stream_name: str = Field("photo_processing") durable_name: str = Field("photo_processing_worker") similarity_threshold: float = Field(0.5) - class Config: - env_prefix = "PHOTO_WORKER_" - settings = PhotoWorkerSettings() # type: ignore diff --git a/docs/problem-fixes/001-mobile-auth-required-field-validation.md b/docs/problem-fixes/001-mobile-auth-required-field-validation.md index 0e6a2b2..32830cf 100644 --- a/docs/problem-fixes/001-mobile-auth-required-field-validation.md +++ b/docs/problem-fixes/001-mobile-auth-required-field-validation.md @@ -6,7 +6,7 @@ Fixed in `app/schema/request/mobile/auth.py` and covered by endpoint and e2e tes ## Problem -`POST /user/auth/register-login` accepted weak request payloads because `MobileAuthRequest` used plain `str` fields for `password`, `device_name`, and `device_type`. +`POST /user/auth/register` and `POST /user/auth/login` accepted weak request payloads because the shared mobile auth request model used plain `str` fields for `password`, `device_name`, and `device_type`. The risky payloads were: @@ -27,7 +27,7 @@ The request model did not put validation at the boundary where the API payload e Before the fix, the schema shape was effectively: ```python -class MobileAuthRequest(BaseModel): +class MobileAuthBaseRequest(BaseModel): email: EmailStr password: str device_name: str @@ -35,7 +35,7 @@ class MobileAuthRequest(BaseModel): device_id: UUID ``` -Plain `str` accepts `""`, `" "`, and arbitrarily large values. Since `app/service/users.py` trusts `MobileAuthRequest`, bad values could flow directly into password hashing, user creation, and device creation. +Plain `str` accepts `""`, `" "`, and arbitrarily large values. Since `app/service/users.py` trusts `MobileRegisterRequest` and `MobileLoginRequest`, bad values could flow directly into password hashing, user creation, and device creation. ## Fix Location @@ -105,38 +105,46 @@ def test_register_login_rejects_empty_required_text_fields( field, value, ): - payload = _valid_payload() - payload[field] = value + for endpoint, attr in ( + ("/user/auth/register", "register_request"), + ("/user/auth/login", "login_request"), + ): + payload = _valid_payload() + payload[field] = value - response = client.post("/user/auth/register-login", json=payload) + response = client.post(endpoint, json=payload) - assert response.status_code == 422 - assert fake_container.auth_service.received_request is None + assert response.status_code == 422 + assert getattr(fake_container.auth_service, attr) is None ``` -Before the fix, those payloads were accepted by the endpoint because the fields were plain `str`. The second assertion makes the test real at the route boundary: invalid payloads must fail before they can reach `AuthService.mobile_register_login()`. +Before the fix, those payloads were accepted by the endpoint because the fields were plain `str`. The second assertion makes the test real at the route boundary: invalid payloads must fail before they can reach `AuthService.mobile_register()` or `AuthService.mobile_login()`. -The request body is still written in the test because the test has to define the scenario. What makes this a real endpoint simulation is that the payload goes through FastAPI HTTP request handling, dependency solving, request-body parsing, Pydantic validation, and the actual route path instead of directly calling `MobileAuthRequest(...)`. +The request body is still written in the test because the test has to define the scenario. What makes this a real endpoint simulation is that the payload goes through FastAPI HTTP request handling, dependency solving, request-body parsing, Pydantic validation, and the actual route path instead of directly calling `MobileRegisterRequest(...)` or `MobileLoginRequest(...)`. ```python def test_register_login_passes_normalized_input_to_service(client, fake_container): - payload = _valid_payload() - payload.update( - { - "email": " USER@Example.COM ", - "device_name": " Pixel 8 ", - "device_type": " ANDROID ", - } - ) - - response = client.post("/user/auth/register-login", json=payload) - - assert response.status_code == 200 - req = fake_container.auth_service.received_request - assert req is not None - assert req.email == "user@example.com" - assert req.device_name == "Pixel 8" - assert req.device_type == "android" + for endpoint, attr in ( + ("/user/auth/register", "register_request"), + ("/user/auth/login", "login_request"), + ): + payload = _valid_payload() + payload.update( + { + "email": " USER@Example.COM ", + "device_name": " Pixel 8 ", + "device_type": " ANDROID ", + } + ) + + response = client.post(endpoint, json=payload) + + assert response.status_code == 200 + req = getattr(fake_container.auth_service, attr) + assert req is not None + assert req.email == "user@example.com" + assert req.device_name == "Pixel 8" + assert req.device_type == "android" ``` Before the fix, device fields kept surrounding whitespace and mixed casing. @@ -146,13 +154,17 @@ def test_register_login_password_length_is_checked_after_trimming( client, fake_container, ): - payload = _valid_payload() - payload["password"] = " a" + for endpoint, attr in ( + ("/user/auth/register", "register_request"), + ("/user/auth/login", "login_request"), + ): + payload = _valid_payload() + payload["password"] = " a" - response = client.post("/user/auth/register-login", json=payload) + response = client.post(endpoint, json=payload) - assert response.status_code == 422 - assert fake_container.auth_service.received_request is None + assert response.status_code == 422 + assert getattr(fake_container.auth_service, attr) is None ``` This protects the password minimum-length rule from being bypassed by padding a one-character password with spaces. diff --git a/docs/problem-fixes/003-mobile-auth-email-length-bound.md b/docs/problem-fixes/003-mobile-auth-email-length-bound.md new file mode 100644 index 0000000..c5c9702 --- /dev/null +++ b/docs/problem-fixes/003-mobile-auth-email-length-bound.md @@ -0,0 +1,36 @@ +# 003 - Mobile Auth Email Length Bound + +## Status + +Fixed. Mobile auth emails are now bounded at the API request boundary before they can reach the database. + +## Problem + +`EmailStr` validates email syntax, but it does not by itself guarantee that the accepted value fits the `users.email` database column. A payload with a syntactically valid email longer than 255 characters could pass request validation and then fail later when inserted or queried against the database. + +That creates avoidable late failures and makes the API response depend on database behavior instead of the request contract. + +## Root Cause + +The mobile auth request schema previously relied on `EmailStr` alone for the `email` field. The database column is constrained to 255 characters, so the schema needed to mirror that limit. + +## Fix Location + +- `app/schema/request/mobile/auth.py`: `MobileAuthBaseRequest.email` is declared as `EmailStr = Field(..., max_length=255)`. +- `app/schema/request/mobile/auth.py`: `_normalize_email()` trims and lowercases string input before `EmailStr` validation. + +## Fix Behavior + +After the fix: + +- oversized email strings are rejected with `422 Unprocessable Entity`; +- normalized email values are what reach the auth service; +- database insert/update code no longer has to be the first layer that discovers overlong mobile-auth emails. + +## Tests + +Covered by `tests/unit/test_mobile_auth_request_validation.py`: + +- `test_register_login_rejects_oversized_email()` posts an email longer than 255 characters to both `/user/auth/register` and `/user/auth/login`. +- The expected result is `422`. +- The fake auth service is not called, proving rejection happens at request validation time. diff --git a/docs/problem-fixes/004-mobile-auth-concurrent-signup-race.md b/docs/problem-fixes/004-mobile-auth-concurrent-signup-race.md new file mode 100644 index 0000000..e701cb4 --- /dev/null +++ b/docs/problem-fixes/004-mobile-auth-concurrent-signup-race.md @@ -0,0 +1,37 @@ +# 004 - Mobile Auth Concurrent Signup Race + +## Status + +Fixed. Duplicate-user races during registration are caught and converted to a conflict response. + +## Problem + +`AuthService.mobile_register()` first checks whether an email exists, then creates a user if no row is found. Two simultaneous requests for the same email can both pass the existence check before either insert commits. + +Without handling the database uniqueness failure, the losing request can bubble up an `IntegrityError` as a generic server error instead of returning a clear duplicate-email response. + +## Root Cause + +The service relied on the pre-insert lookup as if it were enough to guarantee uniqueness. The real source of truth is the database unique constraint, so the registration path also needed to handle insert-time uniqueness errors. + +## Fix Location + +- `app/service/users.py`: `AuthService.mobile_register()` wraps `user_querier.create_user(...)` in a `try` block. +- `app/service/users.py`: `except SQLAlchemyError as exc` passes the database error to `DBException.handle(exc)`. +- `app/core/exceptions.py`: duplicate-key handling maps unique-constraint violations to a conflict response. + +## Fix Behavior + +After the fix: + +- normal first registration succeeds; +- pre-existing email registration returns `409 Conflict`; +- race-condition duplicate insert errors are also returned as `409 Conflict` instead of `500 Internal Server Error`. + +## Tests + +Covered by `tests/unit/test_mobile_auth_intent_validation.py`: + +- `test_register_concurrent_signup_integrity_error()` stubs `create_user()` to raise a SQLAlchemy `IntegrityError` with duplicate-key metadata. +- The test asserts that `mobile_register()` raises `HTTPException` with status `409`. +- The detail includes language indicating the email is already in use. diff --git a/docs/problem-fixes/005-mobile-auth-bcrypt-truncation.md b/docs/problem-fixes/005-mobile-auth-bcrypt-truncation.md new file mode 100644 index 0000000..17455f1 --- /dev/null +++ b/docs/problem-fixes/005-mobile-auth-bcrypt-truncation.md @@ -0,0 +1,40 @@ +# 005 - Mobile Auth Bcrypt 72-Byte Truncation + +## Status + +Fixed. Password hashing now avoids bcrypt's 72-byte input truncation by pre-hashing passwords before passing them to bcrypt. + +## Problem + +Bcrypt only uses the first 72 bytes of its password input. If the application passes raw user passwords directly to bcrypt, two long passwords that share the same first 72 bytes can verify against the same hash. + +That is especially risky for long passphrases or generated passwords, where differences after byte 72 should still matter. + +## Root Cause + +The password hashing layer needed a stable input format whose length is safely below bcrypt's truncation boundary while still representing the full original password. + +## Fix Location + +- `app/core/securite.py`: `hash_password()` hashes the UTF-8 password with SHA-256, base64-encodes the digest, then passes that fixed-size value to bcrypt. +- `app/core/securite.py`: `verify_password()` applies the same SHA-256/base64 transform before bcrypt verification. + +## Fix Behavior + +After the fix: + +- every byte of the original password contributes to the SHA-256 digest; +- bcrypt receives a fixed-length pre-hashed value, so its 72-byte truncation does not discard user-password suffixes; +- two long passwords with the same 72-byte prefix no longer verify against each other's hashes. + +## Tests + +Covered by `tests/unit/test_bcrypt_truncation.py`: + +- `test_bcrypt_72_byte_truncation_is_prevented()` builds two passwords with an identical 72-byte prefix and different suffixes. +- Each password verifies with its own hash. +- Neither password verifies against the other password's hash. + +## Note + +This fix does not reject passwords over 72 bytes. It preserves support for long passwords by pre-hashing them safely before bcrypt. diff --git a/docs/problem-fixes/006-mobile-auth-password-complexity-policy.md b/docs/problem-fixes/006-mobile-auth-password-complexity-policy.md new file mode 100644 index 0000000..d0da9de --- /dev/null +++ b/docs/problem-fixes/006-mobile-auth-password-complexity-policy.md @@ -0,0 +1,42 @@ +# 006 - Mobile Auth Password Complexity Policy + +## Status + +Fixed. Registration enforces a password complexity policy while login remains permissive enough to verify existing credentials. + +## Problem + +The mobile registration flow only had length validation. A user could register with a password that met the minimum length but lacked mixed case, digits, or special characters. + +That created a weaker baseline for new accounts and made the validation policy unclear to clients. + +## Root Cause + +Password validation lived mostly in generic field constraints. The registration schema needed explicit semantic validation for password composition. + +## Fix Location + +- `app/schema/request/mobile/auth.py`: `MobileAuthBaseRequest.password` has config-backed min/max length limits. +- `app/schema/request/mobile/auth.py`: `MobileRegisterRequest._validate_password_complexity()` requires: + - at least one uppercase letter; + - at least one lowercase letter; + - at least one digit; + - at least one special character. +- `app/schema/request/mobile/auth.py`: `MobileLoginRequest` intentionally inherits only the shared base validation and does not enforce registration complexity on login attempts. + +## Fix Behavior + +After the fix: + +- weak new-registration passwords fail with `422`; +- valid complex registration passwords pass; +- login still accepts syntactically valid password strings even if they do not meet the current registration complexity policy, so older credentials can still be checked by `verify_password()`. + +## Tests + +Covered by `tests/unit/test_mobile_auth_request_validation.py`: + +- `test_register_enforces_password_complexity()` verifies valid complex passwords pass and missing uppercase/lowercase/digit/special-character cases fail. +- `test_login_does_not_enforce_password_complexity()` verifies login can still attempt verification with a password that lacks the registration-only complexity requirement. + +The live e2e fixtures in `tests/e2e/test_mobile_auth_intent_e2e.py` and `tests/e2e/test_mobile_auth_request_validation_e2e.py` use complex passwords such as `ValidPass@123` so registration tests exercise the current policy. diff --git a/docs/problem-fixes/007-mobile-auth-rate-limiting-bruteforce.md b/docs/problem-fixes/007-mobile-auth-rate-limiting-bruteforce.md new file mode 100644 index 0000000..60380bb --- /dev/null +++ b/docs/problem-fixes/007-mobile-auth-rate-limiting-bruteforce.md @@ -0,0 +1,42 @@ +# 007 - Mobile Auth Rate Limiting And Brute-Force Protection + +## Status + +Fixed. Mobile login and registration now apply Redis-backed rate limits by email and client IP, and the test doubles have the required Redis methods. + +## Problem + +Mobile auth endpoints did not have brute-force protection. Attackers could repeatedly try passwords or enumerate registration/login behavior without a request counter at the service boundary. + +An initial implementation added rate limiting, but tests failed because fake Redis/test doubles did not implement the new `incr()` and `expire()` methods, and live e2e tests shared one client IP bucket. + +## Root Cause + +The auth service needed a small atomic counter abstraction from Redis and tests needed to evolve with that new contract. The router also needed to pass a stable client identity into the service so IP-based throttling could work. + +## Fix Location + +- `app/core/config.py`: adds `RATE_LIMIT_LOGIN_MAX_ATTEMPTS`, `RATE_LIMIT_LOGIN_WINDOW_SECONDS`, and `TRUST_PROXY_HEADERS`. +- `app/infra/redis.py`: `RedisClient.incr()` and `RedisClient.expire()` expose the Redis operations needed by rate limiting. +- `app/router/mobile/auth.py`: `_get_client_ip()` derives client identity from `X-Forwarded-For`, `X-Real-IP`, or the socket client, depending on `TRUST_PROXY_HEADERS`. +- `app/router/mobile/auth.py`: `mobile_register()` and `mobile_login()` pass `client_ip` to the auth service. +- `app/service/users.py`: `AuthService.mobile_register()` and `AuthService.mobile_login()` check `rate:ip:{client_ip}` and `rate:email:{email}` before credential or signup work. +- `app/service/users.py`: `AuthService.check_rate_limit()` increments the Redis counter, sets the expiry on first use, and raises `429 Too Many Requests` after the configured limit. + +## Fix Behavior + +After the fix: + +- repeated attempts from the same email are throttled; +- repeated attempts from the same client IP are throttled when a client IP is available; +- counters expire after the configured window; +- `429` responses include the service-level too-many-requests error path; +- live e2e tests isolate client IP buckets with `X-Forwarded-For` so one test does not accidentally consume another test's limit. + +## Tests + +Covered by: + +- `tests/unit/test_mobile_auth_rate_limiting.py`: `test_rate_limiting_triggered_after_max_attempts()` verifies the first five login attempts pass and the sixth returns `429`. +- `tests/unit/test_mobile_auth_request_validation.py`: `test_mobile_auth_uses_forwarded_ip_for_rate_limit_identity()` verifies the router forwards the first `X-Forwarded-For` address into the auth service. +- `tests/e2e/test_mobile_auth_intent_e2e.py`: live e2e requests include a per-test `X-Forwarded-For` header, preventing shared-IP rate-limit collisions while still exercising real HTTP requests. diff --git a/docs/problem-fixes/README.md b/docs/problem-fixes/README.md index 6344e52..737dcbf 100644 --- a/docs/problem-fixes/README.md +++ b/docs/problem-fixes/README.md @@ -12,4 +12,10 @@ Each problem note should include: ## Entries -- [001 - Mobile auth accepted empty or unnormalized input fields](002-mobile-auth-required-field-validation.md) +- [001 - Mobile auth accepted empty or unnormalized input fields](001-mobile-auth-required-field-validation.md) +- [002 - Mobile auth split register/login endpoints](002-mobile-auth-ambiguous-register-login-intent.md) +- [003 - Mobile auth email length bound](003-mobile-auth-email-length-bound.md) +- [004 - Mobile auth concurrent signup race](004-mobile-auth-concurrent-signup-race.md) +- [005 - Mobile auth bcrypt 72-byte truncation](005-mobile-auth-bcrypt-truncation.md) +- [006 - Mobile auth password complexity policy](006-mobile-auth-password-complexity-policy.md) +- [007 - Mobile auth rate limiting and brute-force protection](007-mobile-auth-rate-limiting-bruteforce.md) diff --git a/tests/e2e/test_mobile_auth_intent_e2e.py b/tests/e2e/test_mobile_auth_intent_e2e.py index 2bcf4b6..e19e5e2 100644 --- a/tests/e2e/test_mobile_auth_intent_e2e.py +++ b/tests/e2e/test_mobile_auth_intent_e2e.py @@ -18,6 +18,9 @@ class TestMobileAuthEndpointsE2E: @pytest.fixture(autouse=True) def setup(self): self.base_url = os.getenv("MULTAI_E2E_BASE_URL", "http://localhost:8000") + self.headers = { + "X-Forwarded-For": f"203.0.113.{uuid.uuid4().int % 250 + 1}", + } def test_login_with_unknown_email_fails(self) -> None: """Test that login with unknown email returns 401.""" @@ -28,7 +31,11 @@ def test_login_with_unknown_email_fails(self) -> None: "device_type": "android", "device_id": str(uuid.uuid4()), } - response = requests.post(f"{self.base_url}/user/auth/login", json=payload) + response = requests.post( + f"{self.base_url}/user/auth/login", + json=payload, + headers=self.headers, + ) assert response.status_code == 401 assert "not found" in response.json()["detail"].lower() @@ -40,24 +47,32 @@ def test_register_with_existing_email_fails(self) -> None: # First registration succeeds register_payload = { "email": email, - "password": "validpass123", + "password": "ValidPass@123", "device_name": "TestDevice", "device_type": "android", "device_id": device_id, } - response1 = requests.post(f"{self.base_url}/user/auth/register", json=register_payload) + response1 = requests.post( + f"{self.base_url}/user/auth/register", + json=register_payload, + headers=self.headers, + ) assert response1.status_code == 200 assert response1.json()["is_new_user"] is True # Second registration with same email fails - response2 = requests.post(f"{self.base_url}/user/auth/register", json=register_payload) + response2 = requests.post( + f"{self.base_url}/user/auth/register", + json=register_payload, + headers=self.headers, + ) assert response2.status_code == 409 assert "already" in response2.json()["detail"].lower() def test_register_then_login_succeeds(self) -> None: """Test full flow: register then login.""" email = f"user_{uuid.uuid4()}@example.com" - password = "validpass123" + password = "ValidPass@123" device_id = str(uuid.uuid4()) # Register @@ -68,7 +83,11 @@ def test_register_then_login_succeeds(self) -> None: "device_type": "android", "device_id": device_id, } - register_response = requests.post(f"{self.base_url}/user/auth/register", json=register_payload) + register_response = requests.post( + f"{self.base_url}/user/auth/register", + json=register_payload, + headers=self.headers, + ) assert register_response.status_code == 200 assert register_response.json()["is_new_user"] is True register_token = register_response.json()["access_token"] @@ -81,7 +100,11 @@ def test_register_then_login_succeeds(self) -> None: "device_type": "android", "device_id": device_id, } - login_response = requests.post(f"{self.base_url}/user/auth/login", json=login_payload) + login_response = requests.post( + f"{self.base_url}/user/auth/login", + json=login_payload, + headers=self.headers, + ) assert login_response.status_code == 200 assert login_response.json()["is_new_user"] is False login_token = login_response.json()["access_token"] @@ -93,7 +116,7 @@ def test_register_then_login_succeeds(self) -> None: def test_login_with_wrong_password_fails(self) -> None: """Test that login with wrong password returns 401.""" email = f"user_{uuid.uuid4()}@example.com" - password = "correctpass123" + password = "CorrectPass@123" device_id = str(uuid.uuid4()) # Register first @@ -104,17 +127,26 @@ def test_login_with_wrong_password_fails(self) -> None: "device_type": "android", "device_id": device_id, } - requests.post(f"{self.base_url}/user/auth/register", json=register_payload) + register_response = requests.post( + f"{self.base_url}/user/auth/register", + json=register_payload, + headers=self.headers, + ) + assert register_response.status_code == 200 # Try to login with wrong password login_payload = { "email": email, - "password": "wrongpassword", + "password": "WrongPass@123", "device_name": "TestDevice", "device_type": "android", "device_id": device_id, } - response = requests.post(f"{self.base_url}/user/auth/login", json=login_payload) + response = requests.post( + f"{self.base_url}/user/auth/login", + json=login_payload, + headers=self.headers, + ) assert response.status_code == 401 assert "invalid" in response.json()["detail"].lower() @@ -127,17 +159,25 @@ def test_register_requires_password(self) -> None: "device_id": str(uuid.uuid4()), # Missing password } - response = requests.post(f"{self.base_url}/user/auth/register", json=payload) + response = requests.post( + f"{self.base_url}/user/auth/register", + json=payload, + headers=self.headers, + ) assert response.status_code == 422 def test_login_requires_device_type(self) -> None: """Test that login requires device_type.""" payload = { "email": "user@example.com", - "password": "validpass123", + "password": "ValidPass@123", "device_name": "TestDevice", "device_id": str(uuid.uuid4()), # Missing device_type } - response = requests.post(f"{self.base_url}/user/auth/login", json=payload) + response = requests.post( + f"{self.base_url}/user/auth/login", + json=payload, + headers=self.headers, + ) assert response.status_code == 422 diff --git a/tests/e2e/test_mobile_auth_request_validation_e2e.py b/tests/e2e/test_mobile_auth_request_validation_e2e.py index 08b67a0..79907c0 100644 --- a/tests/e2e/test_mobile_auth_request_validation_e2e.py +++ b/tests/e2e/test_mobile_auth_request_validation_e2e.py @@ -22,7 +22,7 @@ def _valid_payload() -> dict[str, object]: return { "email": f"e2e-{uuid.uuid4()}@example.com", - "password": "validpass123", + "password": "ValidPass@123", "device_name": "Pixel 8", "device_type": "android", "device_id": str(uuid.uuid4()), diff --git a/tests/unit/test_mobile_auth_email_logging.py b/tests/unit/test_mobile_auth_email_logging.py index 3dbe421..f96073a 100644 --- a/tests/unit/test_mobile_auth_email_logging.py +++ b/tests/unit/test_mobile_auth_email_logging.py @@ -70,10 +70,22 @@ async def upsert_session( class FakeRedis: + def __init__(self): + self._store = {} + + async def incr(self, key: str) -> int: + self._store[key] = self._store.get(key, 0) + 1 + return self._store[key] + + async def expire(self, key: str, seconds: int) -> None: + pass + + async def ttl(self, key: str) -> int: + return -1 + async def set(self, key: str, value: str, expire: int) -> None: return None - class FakeFaceEmbeddingService: pass diff --git a/tests/unit/test_mobile_auth_intent_validation.py b/tests/unit/test_mobile_auth_intent_validation.py index 563df05..4d3117d 100644 --- a/tests/unit/test_mobile_auth_intent_validation.py +++ b/tests/unit/test_mobile_auth_intent_validation.py @@ -90,6 +90,19 @@ async def upsert_session( class FakeRedis: + def __init__(self): + self._store = {} + + async def incr(self, key: str) -> int: + self._store[key] = self._store.get(key, 0) + 1 + return self._store[key] + + async def expire(self, key: str, seconds: int) -> None: + pass + + async def ttl(self, key: str) -> int: + return -1 + async def set(self, key: str, value: str, expire: int) -> None: return None diff --git a/tests/unit/test_mobile_auth_rate_limiting.py b/tests/unit/test_mobile_auth_rate_limiting.py new file mode 100644 index 0000000..feb5675 --- /dev/null +++ b/tests/unit/test_mobile_auth_rate_limiting.py @@ -0,0 +1,94 @@ +import asyncio +import uuid +import pytest +from fastapi import HTTPException +from app.service.users import AuthService +from app.schema.request.mobile.auth import MobileLoginRequest, MobileRegisterRequest + + +class MockRedis: + def __init__(self) -> None: + self.data = {} + self.ttls = {} + + async def incr(self, key: str) -> int: + self.data[key] = self.data.get(key, 0) + 1 + return self.data[key] + + async def expire(self, key: str, seconds: int) -> bool: + self.ttls[key] = seconds + return True + + +class FakeUser: + def __init__(self) -> None: + self.id = uuid.uuid4() + self.email = "test@example.com" + from app.core.securite import hash_password + self.hashed_password = hash_password("ValidPass@123") + self.blocked = False + + +class FakeUserQuerier: + async def get_user_by_email(self, email: str) -> FakeUser: + return FakeUser() + + +class FakeDeviceQuerier: + pass + + +class FakeSessionQuerier: + pass + + +class FakeFaceEmbeddingService: + pass + + +def test_rate_limiting_triggered_after_max_attempts() -> None: + # Set up mocks + redis = MockRedis() + service = AuthService( + user_querier=FakeUserQuerier(), + device_querier=FakeDeviceQuerier(), + session_querier=FakeSessionQuerier(), + face_embedding_service=FakeFaceEmbeddingService(), + ) + + # Stub session creation to avoid database / redis dependencies + async def _dummy_create_session(*args: object, **kwargs: object) -> object: + from app.schema.response.mobile.auth import MobileAuthResponse + return MobileAuthResponse( + access_token="access", + refresh_token="refresh", + session_id=str(uuid.uuid4()), + expires_in=3600, + user_id=uuid.uuid4(), + is_new_user=False, + ) + service._create_mobile_session = _dummy_create_session # type: ignore + + req = MobileLoginRequest( + email="test@example.com", + password="ValidPass@123", + device_name="Pixel 8", + device_type="android", + device_id=uuid.uuid4(), + ) + + # Call mobile_login 5 times (which is the default max limit in settings) + # The first 5 should succeed without raising exceptions + for i in range(5): + res = asyncio.run(service.mobile_login(redis, req, client_ip="127.0.0.1")) + assert res.access_token == "access" + + # The 6th call must trigger the rate limit and raise 429! + with pytest.raises(HTTPException) as exc_info: + asyncio.run(service.mobile_login(redis, req, client_ip="127.0.0.1")) + assert exc_info.value.status_code == 429 + assert "too many requests" in exc_info.value.detail.lower() + + # The ttls for the keys should have been set + assert redis.ttls["rate:ip:127.0.0.1"] == 60 + assert redis.ttls["rate:email:test@example.com"] == 60 diff --git a/tests/unit/test_mobile_auth_request_validation.py b/tests/unit/test_mobile_auth_request_validation.py index b9bf117..eb61902 100644 --- a/tests/unit/test_mobile_auth_request_validation.py +++ b/tests/unit/test_mobile_auth_request_validation.py @@ -15,13 +15,17 @@ class FakeAuthService: def __init__(self) -> None: self.register_request: MobileRegisterRequest | None = None self.login_request: MobileLoginRequest | None = None + self.register_client_ip: object = None + self.login_client_ip: object = None async def mobile_register( self, redis: object, req: MobileRegisterRequest, + client_ip: object = None, ) -> MobileAuthResponse: self.register_request = req + self.register_client_ip = client_ip return MobileAuthResponse( access_token="access", refresh_token="refresh", @@ -35,8 +39,10 @@ async def mobile_login( self, redis: object, req: MobileLoginRequest, + client_ip: object = None, ) -> MobileAuthResponse: self.login_request = req + self.login_client_ip = client_ip return MobileAuthResponse( access_token="access", refresh_token="refresh", @@ -206,3 +212,19 @@ def test_login_does_not_enforce_password_complexity( assert fake_container.auth_service.login_request is not None +def test_mobile_auth_uses_forwarded_ip_for_rate_limit_identity( + client: TestClient, + fake_container: FakeContainer, +) -> None: + payload = _valid_payload() + + response = client.post( + "/user/auth/login", + json=payload, + headers={"X-Forwarded-For": "203.0.113.10, 10.0.0.1"}, + ) + + assert response.status_code == 200 + assert fake_container.auth_service.login_client_ip == "203.0.113.10" + + From 7fe865efc6058e33cb11a487aeb91b43a2e4131f Mon Sep 17 00:00:00 2001 From: bouhamza abderrahmane Date: Mon, 1 Jun 2026 14:45:15 +0100 Subject: [PATCH 08/11] fix mypy errors --- tests/e2e/test_mobile_auth_intent_e2e.py | 4 ++-- tests/unit/test_bcrypt_truncation.py | 1 - tests/unit/test_mobile_auth_email_logging.py | 11 +++++++++-- tests/unit/test_mobile_auth_intent_validation.py | 14 ++++++++++---- tests/unit/test_mobile_auth_rate_limiting.py | 15 +++++++++++---- 5 files changed, 32 insertions(+), 13 deletions(-) diff --git a/tests/e2e/test_mobile_auth_intent_e2e.py b/tests/e2e/test_mobile_auth_intent_e2e.py index e19e5e2..4d56978 100644 --- a/tests/e2e/test_mobile_auth_intent_e2e.py +++ b/tests/e2e/test_mobile_auth_intent_e2e.py @@ -1,6 +1,6 @@ import os import uuid -import requests +import requests # type: ignore[import-untyped] import pytest @@ -16,7 +16,7 @@ class TestMobileAuthEndpointsE2E: """ @pytest.fixture(autouse=True) - def setup(self): + def setup(self) -> None: self.base_url = os.getenv("MULTAI_E2E_BASE_URL", "http://localhost:8000") self.headers = { "X-Forwarded-For": f"203.0.113.{uuid.uuid4().int % 250 + 1}", diff --git a/tests/unit/test_bcrypt_truncation.py b/tests/unit/test_bcrypt_truncation.py index 2aaaffc..d0e994c 100644 --- a/tests/unit/test_bcrypt_truncation.py +++ b/tests/unit/test_bcrypt_truncation.py @@ -1,4 +1,3 @@ -from passlib.context import CryptContext from app.core.securite import hash_password, verify_password diff --git a/tests/unit/test_mobile_auth_email_logging.py b/tests/unit/test_mobile_auth_email_logging.py index f96073a..f61048c 100644 --- a/tests/unit/test_mobile_auth_email_logging.py +++ b/tests/unit/test_mobile_auth_email_logging.py @@ -1,3 +1,10 @@ +from typing import Any + +# Test doubles intentionally implement only the AuthService methods exercised here. +# They do not subclass the generated queriers, so mypy would otherwise flag each +# constructor injection as an arg-type mismatch. +# mypy: disable-error-code=arg-type + import asyncio import logging import uuid @@ -70,8 +77,8 @@ async def upsert_session( class FakeRedis: - def __init__(self): - self._store = {} + def __init__(self) -> None: + self._store: dict[str, int] = {} async def incr(self, key: str) -> int: self._store[key] = self._store.get(key, 0) + 1 diff --git a/tests/unit/test_mobile_auth_intent_validation.py b/tests/unit/test_mobile_auth_intent_validation.py index 4d3117d..0f81e97 100644 --- a/tests/unit/test_mobile_auth_intent_validation.py +++ b/tests/unit/test_mobile_auth_intent_validation.py @@ -1,3 +1,10 @@ +from typing import Any + +# Test doubles intentionally implement only the AuthService methods exercised here. +# They do not subclass the generated queriers, so mypy would otherwise flag each +# constructor injection as an arg-type mismatch. +# mypy: disable-error-code=arg-type + import asyncio import logging import uuid @@ -36,7 +43,7 @@ def __init__(self) -> None: class FakeUserQuerier: def __init__(self, user: FakeUser) -> None: self._user = user - self._created_users = {} + self._created_users: dict[str, FakeUser] = {} async def get_user_by_email(self, email: str) -> FakeUser | None: if self._user.exists and self._user.email == email: @@ -90,8 +97,8 @@ async def upsert_session( class FakeRedis: - def __init__(self): - self._store = {} + def __init__(self) -> None: + self._store: dict[str, int] = {} async def incr(self, key: str) -> int: self._store[key] = self._store.get(key, 0) + 1 @@ -350,7 +357,6 @@ async def _noop_cache_session_for_auth(**_: object) -> None: def test_register_concurrent_signup_integrity_error() -> None: """Test that concurrent signup IntegrityError is caught and raised as 409.""" - from typing import Any from sqlalchemy.exc import IntegrityError class FakeOrigException(Exception): diff --git a/tests/unit/test_mobile_auth_rate_limiting.py b/tests/unit/test_mobile_auth_rate_limiting.py index feb5675..e5f227e 100644 --- a/tests/unit/test_mobile_auth_rate_limiting.py +++ b/tests/unit/test_mobile_auth_rate_limiting.py @@ -1,15 +1,22 @@ import asyncio import uuid +from typing import Any + import pytest from fastapi import HTTPException from app.service.users import AuthService -from app.schema.request.mobile.auth import MobileLoginRequest, MobileRegisterRequest +from app.schema.request.mobile.auth import MobileLoginRequest + +# Test doubles intentionally implement only the AuthService methods exercised here. +# They do not subclass the generated queriers, so mypy would otherwise flag each +# constructor injection as an arg-type mismatch. +# mypy: disable-error-code=arg-type class MockRedis: def __init__(self) -> None: - self.data = {} - self.ttls = {} + self.data: dict[str, int] = {} + self.ttls: dict[str, int] = {} async def incr(self, key: str) -> int: self.data[key] = self.data.get(key, 0) + 1 @@ -57,7 +64,7 @@ def test_rate_limiting_triggered_after_max_attempts() -> None: ) # Stub session creation to avoid database / redis dependencies - async def _dummy_create_session(*args: object, **kwargs: object) -> object: + async def _dummy_create_session(*args: object, **kwargs: object) -> Any: from app.schema.response.mobile.auth import MobileAuthResponse return MobileAuthResponse( access_token="access", From 983a14f4cb01a0a55bf69f0cef2e7604c9543ff8 Mon Sep 17 00:00:00 2001 From: bouhamza abderrahmane Date: Mon, 1 Jun 2026 14:47:36 +0100 Subject: [PATCH 09/11] fix ruff errors --- tests/unit/test_mobile_auth_email_logging.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/test_mobile_auth_email_logging.py b/tests/unit/test_mobile_auth_email_logging.py index f61048c..696e095 100644 --- a/tests/unit/test_mobile_auth_email_logging.py +++ b/tests/unit/test_mobile_auth_email_logging.py @@ -1,4 +1,3 @@ -from typing import Any # Test doubles intentionally implement only the AuthService methods exercised here. # They do not subclass the generated queriers, so mypy would otherwise flag each From 8a272d3c3f8d26faabe3311172c870f31368b939 Mon Sep 17 00:00:00 2001 From: ademboukabes Date: Mon, 1 Jun 2026 15:51:36 +0100 Subject: [PATCH 10/11] fix(db): resolve asyncpg AmbiguousParameterError by explicitly casting enum types in processing_jobs queries --- db/generated/processing_jobs.py | 4 ++-- db/queries/processing_jobs.sql | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/db/generated/processing_jobs.py b/db/generated/processing_jobs.py index d2d4d80..0cde67b 100644 --- a/db/generated/processing_jobs.py +++ b/db/generated/processing_jobs.py @@ -26,11 +26,11 @@ """ -UPDATE_PROCESSING_JOB_STATUS = """-- name: update_processing_job_status \\:one +UPDATE_PROCESSING_JOB_STATUS = """-- name: update_processing_job_status \:one UPDATE processing_jobs SET status = :p2, attempts = attempts + 1, - completed_at = CASE WHEN :p2 IN ('completed', 'failed') THEN now() ELSE completed_at END + completed_at = CASE WHEN :p2 IN ('completed'::processing_job_status, 'failed'::processing_job_status) THEN now() ELSE completed_at END WHERE id = :p1 RETURNING id, photo_id, job_type, status, attempts, created_at, completed_at """ diff --git a/db/queries/processing_jobs.sql b/db/queries/processing_jobs.sql index dfc7d45..e3f0d45 100644 --- a/db/queries/processing_jobs.sql +++ b/db/queries/processing_jobs.sql @@ -7,7 +7,7 @@ RETURNING *; UPDATE processing_jobs SET status = $2, attempts = attempts + 1, - completed_at = CASE WHEN $2 IN ('completed', 'failed') THEN now() ELSE completed_at END + completed_at = CASE WHEN $2 IN ('completed'::processing_job_status, 'failed'::processing_job_status) THEN now() ELSE completed_at END WHERE id = $1 RETURNING *; From 59c26d8694ce86ea773d91997c6f923b2fad2ef7 Mon Sep 17 00:00:00 2001 From: ademboukabes Date: Tue, 2 Jun 2026 22:31:17 +0100 Subject: [PATCH 11/11] fix(security): resolve privacy bug in photo approval and ineffective device revocation --- app/router/mobile/auth.py | 11 +++++++++++ app/service/photo_approval.py | 8 ++++---- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/app/router/mobile/auth.py b/app/router/mobile/auth.py index 03fa0fc..8a9a1a0 100644 --- a/app/router/mobile/auth.py +++ b/app/router/mobile/auth.py @@ -96,6 +96,17 @@ async def revoke_device( container: Container = Depends(get_container), current_user: MobileUserSchema = Depends(get_current_mobile_user), ) -> dict[str, str]: + from app.core.constant import RedisKey + + session = await container.session_service.session_querier.get_session_by_device( + device_id=device_id + ) + if session: + await container.session_service.delete_session_cache(container.redis, session.id) + + user_session_key = RedisKey.UserSessionByUser.value.format(user_id=current_user.user_id) + await container.redis.delete(user_session_key) + await container.device_service.revoke_device( device_id=device_id, user_id=current_user.user_id, diff --git a/app/service/photo_approval.py b/app/service/photo_approval.py index b6cb870..dd3f68e 100644 --- a/app/service/photo_approval.py +++ b/app/service/photo_approval.py @@ -51,16 +51,16 @@ async def decide( async for a in self._approval_querier.get_photo_approvals_by_photo_id(photo_id=photo_id): approvals.append(a) - pending = [a for a in approvals if a.decision == "pending"] - if pending: - return "pending" - rejected = [a for a in approvals if a.decision == "rejected"] if rejected: await self._photo_querier.update_photo_status(id=photo_id, status="rejected") await self._delete_photo_storage(photo_id) return "rejected" + pending = [a for a in approvals if a.decision == "pending"] + if pending: + return "pending" + await self._photo_querier.update_photo_status(id=photo_id, status="approved") return "approved"