From f1b68dd1b75fe4c916732c27db61cef3d2428bd7 Mon Sep 17 00:00:00 2001 From: root Date: Tue, 8 Sep 2026 12:33:01 +0000 Subject: [PATCH] no-mistakes(review): Normalize legacy emails to prevent case-based login lockout --- app/main.py | 2 + app/services/auth.py | 13 ++++-- app/services/seed.py | 51 ++++++++++++++++++++++ tests/test_p0_auth_admin_batch.py | 71 ++++++++++++++++++++++++++++++- 4 files changed, 132 insertions(+), 5 deletions(-) diff --git a/app/main.py b/app/main.py index 92dfa25..d8003d9 100644 --- a/app/main.py +++ b/app/main.py @@ -15,6 +15,7 @@ from app.core.config import settings from app.core.database import Base, async_session_factory, engine from app.routers import auth, health, pages, tickets, whatsapp from app.services.seed import ( + normalize_legacy_user_emails, normalize_legacy_user_roles, seed_categories, seed_units, @@ -75,6 +76,7 @@ async def lifespan(app: FastAPI): # P0 role-model unification: converge legacy nickname roles (e.g. # ``technician``/``cs``/``fm``) onto the canonical taxonomy at startup. await normalize_legacy_user_roles(session) + await normalize_legacy_user_emails(session) await session.commit() await seed_units(session, json_path=str(settings.BASE_DIR / "apartment_mapping.json")) await session.commit() diff --git a/app/services/auth.py b/app/services/auth.py index b228485..a96a8b2 100644 --- a/app/services/auth.py +++ b/app/services/auth.py @@ -32,8 +32,12 @@ async def login(db: AsyncSession, email: str, password: str) -> tuple[str, str, Fails closed (401) for bad credentials, inactive accounts, and any user whose stored role is not part of the unified role model. """ - result = await db.execute(select(User).where(User.email == email.strip().lower())) - user = result.scalar_one_or_none() + result = await db.execute( + select(User) + .where(func.lower(User.email) == email.strip().lower()) + .order_by(User.id) + ) + user = result.scalars().first() if user is None or not verify_password(password, user.password_hash): raise HTTPException(status_code=_UNAUTHORIZED, detail="Invalid email or password") if not user.active: @@ -86,7 +90,8 @@ async def _get_user_or_404(db: AsyncSession, user_id: int) -> User: async def create_user(db: AsyncSession, body: AdminCreateUserRequest) -> User: """Admin-created user with an explicit, canonical role. 409 on duplicate email.""" - result = await db.execute(select(User).where(User.email == body.email)) + email = body.email.strip().lower() + result = await db.execute(select(User).where(func.lower(User.email) == email)) if result.scalar_one_or_none(): raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail="Email already registered") @@ -96,7 +101,7 @@ async def create_user(db: AsyncSession, body: AdminCreateUserRequest) -> User: raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Unknown role") user = User( - email=body.email, + email=email, password_hash=hash_password(body.password), full_name=body.full_name.strip(), phone=body.phone, diff --git a/app/services/seed.py b/app/services/seed.py index 22b16c1..1f27f68 100644 --- a/app/services/seed.py +++ b/app/services/seed.py @@ -73,6 +73,57 @@ async def normalize_legacy_user_roles(db: AsyncSession) -> int: return changed +async def normalize_legacy_user_emails(db: AsyncSession) -> int: + """Lowercase stored user emails to match the normalized login lookup. + + Databases built before the P0 batch can hold mixed-case emails (the old + open self-registration stored them verbatim) while login now compares on + the lowercase form, so such rows would otherwise be silently locked out. + Rewrites each stored email to its stripped/lowercase form. When two rows + share an email that differs only in case, only the lowest-id row becomes + canonical (any others keep their stored value and are logged as a warning) + so the unique constraint is never violated. Idempotent; returns the + number of rows rewritten. + """ + result = await db.execute(select(User).order_by(User.id)) + users = list(result.scalars().all()) + groups: dict[str, list[User]] = {} + for user in users: + groups.setdefault(user.email.strip().lower(), []).append(user) + + changed = 0 + for normalized, members in groups.items(): + if len(members) == 1: + user = members[0] + if user.email != normalized: + logger.info( + "Normalizing legacy email %r → %r for user %d", user.email, normalized, user.id + ) + user.email = normalized + changed += 1 + continue + if any(user.email == normalized for user in members): + losers = [u for u in members if u.email != normalized] + else: + winner = members[0] + logger.info( + "Normalizing legacy email %r → %r for user %d", winner.email, normalized, winner.id + ) + winner.email = normalized + changed += 1 + losers = members[1:] + for loser in losers: + logger.warning( + "Cannot normalize email %r for user %d: another account already holds " + "that normalized email; keeping the stored value", + normalized, + loser.id, + ) + if changed: + await db.flush() + return changed + + async def seed_users(db: AsyncSession, default_password: str = "denya123") -> list[User]: """Insert seed users if they don't already exist.""" hashed = hash_password(default_password) diff --git a/tests/test_p0_auth_admin_batch.py b/tests/test_p0_auth_admin_batch.py index 9da11aa..c46b20e 100644 --- a/tests/test_p0_auth_admin_batch.py +++ b/tests/test_p0_auth_admin_batch.py @@ -14,7 +14,7 @@ from app.core.config import settings from app.core.database import async_session_factory from app.core.security import hash_password from app.models.user import User -from app.services.seed import normalize_legacy_user_roles +from app.services.seed import normalize_legacy_user_emails, normalize_legacy_user_roles pytestmark = pytest.mark.asyncio @@ -52,6 +52,12 @@ async def _normalize_roles() -> None: await session.commit() +async def _normalize_emails() -> None: + async with async_session_factory() as session: + await normalize_legacy_user_emails(session) + await session.commit() + + async def _create_ticket(client, token: str, **overrides) -> dict: payload = { "unit_id": 2, @@ -302,6 +308,69 @@ async def test_normalize_does_not_map_ambiguous_admin_alias(client: AsyncClient) assert user.role == "admin" +# ── P0 email normalization (legacy mixed-case rows) ─────────────────── +async def test_legacy_mixed_case_email_migrated_and_authenticates(client: AsyncClient): + """A legacy row whose email was stored verbatim in mixed case (the old open + register) is lowercased by the startup self-heal and still authenticates.""" + await _insert_user("DemoUser@Example.com", "Tech") + await _normalize_emails() # what lifespan does each boot + + async with async_session_factory() as session: + from sqlalchemy import select + user = ( + await session.execute(select(User).where(User.email == "demouser@example.com")) + ).scalar_one() + assert user.email == "demouser@example.com" + + for variant in ("demouser@example.com", "DemoUser@Example.com"): + resp = await client.post( + "/api/auth/login", json={"email": variant, "password": "denya123"} + ) + assert resp.status_code == 200, resp.text + + +async def test_login_matches_legacy_mixed_case_email_before_migration(client: AsyncClient): + """Login compares on the normalized form, so an un-migrated mixed-case row + is still matched by its lowercase login (no hard dependency on the + self-heal having run).""" + await _insert_user("DemoUser@Example.com", "Tech") + resp = await client.post( + "/api/auth/login", json={"email": "demouser@example.com", "password": "denya123"} + ) + assert resp.status_code == 200, resp.text + + +async def test_legacy_email_normalization_is_idempotent(client: AsyncClient): + """The startup self-heal rewrites once and no-ops on subsequent boots.""" + await _insert_user("DemoUser@Example.com", "Tech") + async with async_session_factory() as session: + first = await normalize_legacy_user_emails(session) + await session.commit() + async with async_session_factory() as session: + second = await normalize_legacy_user_emails(session) + await session.commit() + assert first == 1 + assert second == 0 + + +async def test_create_user_rejects_case_variant_of_legacy_email(client: AsyncClient): + """The admin create-user duplicate check compares on the normalized form: + creating a case-variant of a legacy mixed-case row returns 409, not 201.""" + await _insert_user("DemoUser@Example.com", "Tech") + token = await _login(client) + resp = await client.post( + "/api/auth/users", + json={ + "email": "demouser@example.com", + "password": "password1", + "full_name": "X", + "role": "Tech", + }, + headers=_auth(token), + ) + assert resp.status_code == 409, resp.text + + async def test_admin_only_rbac_gate(client: AsyncClient): """Canonical admins pass /api/auth/admin-only; everyone else 403.""" wahab = await _login(client)