no-mistakes(review): Normalize legacy emails to prevent case-based login lockout
This commit is contained in:
@@ -15,6 +15,7 @@ from app.core.config import settings
|
|||||||
from app.core.database import Base, async_session_factory, engine
|
from app.core.database import Base, async_session_factory, engine
|
||||||
from app.routers import auth, health, pages, tickets, whatsapp
|
from app.routers import auth, health, pages, tickets, whatsapp
|
||||||
from app.services.seed import (
|
from app.services.seed import (
|
||||||
|
normalize_legacy_user_emails,
|
||||||
normalize_legacy_user_roles,
|
normalize_legacy_user_roles,
|
||||||
seed_categories,
|
seed_categories,
|
||||||
seed_units,
|
seed_units,
|
||||||
@@ -75,6 +76,7 @@ async def lifespan(app: FastAPI):
|
|||||||
# P0 role-model unification: converge legacy nickname roles (e.g.
|
# P0 role-model unification: converge legacy nickname roles (e.g.
|
||||||
# ``technician``/``cs``/``fm``) onto the canonical taxonomy at startup.
|
# ``technician``/``cs``/``fm``) onto the canonical taxonomy at startup.
|
||||||
await normalize_legacy_user_roles(session)
|
await normalize_legacy_user_roles(session)
|
||||||
|
await normalize_legacy_user_emails(session)
|
||||||
await session.commit()
|
await session.commit()
|
||||||
await seed_units(session, json_path=str(settings.BASE_DIR / "apartment_mapping.json"))
|
await seed_units(session, json_path=str(settings.BASE_DIR / "apartment_mapping.json"))
|
||||||
await session.commit()
|
await session.commit()
|
||||||
|
|||||||
@@ -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
|
Fails closed (401) for bad credentials, inactive accounts, and any user
|
||||||
whose stored role is not part of the unified role model.
|
whose stored role is not part of the unified role model.
|
||||||
"""
|
"""
|
||||||
result = await db.execute(select(User).where(User.email == email.strip().lower()))
|
result = await db.execute(
|
||||||
user = result.scalar_one_or_none()
|
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):
|
if user is None or not verify_password(password, user.password_hash):
|
||||||
raise HTTPException(status_code=_UNAUTHORIZED, detail="Invalid email or password")
|
raise HTTPException(status_code=_UNAUTHORIZED, detail="Invalid email or password")
|
||||||
if not user.active:
|
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:
|
async def create_user(db: AsyncSession, body: AdminCreateUserRequest) -> User:
|
||||||
"""Admin-created user with an explicit, canonical role. 409 on duplicate email."""
|
"""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():
|
if result.scalar_one_or_none():
|
||||||
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail="Email already registered")
|
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")
|
raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Unknown role")
|
||||||
|
|
||||||
user = User(
|
user = User(
|
||||||
email=body.email,
|
email=email,
|
||||||
password_hash=hash_password(body.password),
|
password_hash=hash_password(body.password),
|
||||||
full_name=body.full_name.strip(),
|
full_name=body.full_name.strip(),
|
||||||
phone=body.phone,
|
phone=body.phone,
|
||||||
|
|||||||
@@ -73,6 +73,57 @@ async def normalize_legacy_user_roles(db: AsyncSession) -> int:
|
|||||||
return changed
|
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]:
|
async def seed_users(db: AsyncSession, default_password: str = "denya123") -> list[User]:
|
||||||
"""Insert seed users if they don't already exist."""
|
"""Insert seed users if they don't already exist."""
|
||||||
hashed = hash_password(default_password)
|
hashed = hash_password(default_password)
|
||||||
|
|||||||
@@ -14,7 +14,7 @@ from app.core.config import settings
|
|||||||
from app.core.database import async_session_factory
|
from app.core.database import async_session_factory
|
||||||
from app.core.security import hash_password
|
from app.core.security import hash_password
|
||||||
from app.models.user import User
|
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
|
pytestmark = pytest.mark.asyncio
|
||||||
|
|
||||||
@@ -52,6 +52,12 @@ async def _normalize_roles() -> None:
|
|||||||
await session.commit()
|
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:
|
async def _create_ticket(client, token: str, **overrides) -> dict:
|
||||||
payload = {
|
payload = {
|
||||||
"unit_id": 2,
|
"unit_id": 2,
|
||||||
@@ -302,6 +308,69 @@ async def test_normalize_does_not_map_ambiguous_admin_alias(client: AsyncClient)
|
|||||||
assert user.role == "admin"
|
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):
|
async def test_admin_only_rbac_gate(client: AsyncClient):
|
||||||
"""Canonical admins pass /api/auth/admin-only; everyone else 403."""
|
"""Canonical admins pass /api/auth/admin-only; everyone else 403."""
|
||||||
wahab = await _login(client)
|
wahab = await _login(client)
|
||||||
|
|||||||
Reference in New Issue
Block a user