diff --git a/backend/app/auth.py b/backend/app/auth.py index bfe2fdb6..8a6859bb 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -65,6 +65,26 @@ def has_write_scope(user: User) -> bool: return WRITE_SCOPE in tok.scopes.split() +def env_allowed_canonicals() -> set[str]: + """The env bootstrap allow-list, in CANONICAL form — the only shape it may be compared in. + + S3 canonicalised the Invite branch of `is_allowed` and left this one matching the literal string, + which is the 2026-09-03 lockout with a different source of truth: an operator who writes + `first.last@gmail.com` into ALLOWED_EMAILS is refused when Google answers with the undotted + primary, and a `+tag`ged entry can never match at all, because a Google account cannot contain a + `+`. The addresses themselves stay literal wherever we MAIL them (`admin_notify_groups`); only + the comparison is canonical. + """ + return env_admin_canonicals() | {canonical_email(e) for e in settings.allowed_email_set} + + +def env_admin_canonicals() -> set[str]: + """The env ADMIN_EMAILS bootstrap list, canonical — for the role check only. Kept separate from + `env_allowed_canonicals` because being listed as an admin also grants access, but being allowed + does not make anyone an admin.""" + return {canonical_email(e) for e in settings.admin_email_set} + + def is_allowed(db: Session, email: str) -> bool: """May this email sign in? An approved Invite is the source of truth; the env ALLOWED_EMAILS/ADMIN_EMAILS remain a bootstrap fallback (e.g. a fresh DB). @@ -89,8 +109,7 @@ def is_allowed(db: Session, email: str) -> bool: ) if statuses: return "approved" in statuses - email = email.lower() - return email in settings.allowed_email_set or email in settings.admin_email_set + return canon in env_allowed_canonicals() def can_activate(db: Session, user: User) -> bool: @@ -226,10 +245,19 @@ def purge_user(db: Session, user: User, background: BackgroundTasks) -> None: ) # Raw delete so Postgres applies ON DELETE CASCADE / SET NULL on every dependent table. db.execute(delete(User).where(User.id == user_id)) - # Every canonically-equal invite goes, not just the literal spelling: is_allowed now matches on - # the canonical form, so leaving a sibling row behind would keep granting access to a mailbox - # whose account was just erased — and prod really does carry approved pairs for one mailbox. - db.execute(delete(Invite).where(Invite.email_canonical == canonical_email(email))) + # Every canonically-equal invite goes, not just the literal spelling: is_allowed matches on the + # canonical form, so leaving a sibling row behind would keep granting access to a mailbox whose + # account was just erased — and prod really does carry approved pairs for one mailbox. + # + # Unless ANOTHER account still lives on that mailbox — prod carries that too (two user rows for + # one Gmail mailbox). Then NO invite goes, not even the literal one: is_allowed matches + # canonically, so every row here is what keeps the survivor allowed, and removing any of them + # would refuse their next Google sign-in and reopen it as a fresh access request — this epic's + # founding bug, caused by its own cleanup. `delete_invite` refuses for exactly this reason; the + # user row is already gone by now, so this query sees only the others. + canon = canonical_email(email) + if db.execute(select(User.id).where(User.email_canonical == canon)).first() is None: + db.execute(delete(Invite).where(Invite.email_canonical == canon)) db.commit() if google_token: background.add_task(revoke_google_token, google_token) diff --git a/backend/app/email.py b/backend/app/email.py index 57013360..1c08e0e4 100644 --- a/backend/app/email.py +++ b/backend/app/email.py @@ -196,7 +196,9 @@ def _next_step(verify_link: str | None, has_account: bool, hours: int, lang: str messages. One mail, one click. """ if verify_link: - return mailtext.text("next_step.verify", lang, link=verify_link, hours=hours) + return mailtext.text( + "next_step.verify", lang, link=verify_link, expiry=mailtext.link_expiry(hours, lang) + ) if has_account: return mailtext.text("next_step.sign_in", lang, app=settings.app_base) return mailtext.text("next_step.register", lang, app=settings.app_base) @@ -315,7 +317,9 @@ def send_admin_new_request(admins: list[str], lang: str, requester: str) -> bool def send_verify_email(to: str, lang: str, link: str, hours: int = 24) -> bool: # The body says what the link's rules are. The reset mail always has; this one left the reader to # discover both by meeting a dead link, which is precisely the silence this epic removes. - body = mailtext.text("verify_email.body", lang, link=link, hours=hours) + body = mailtext.text( + "verify_email.body", lang, link=link, expiry=mailtext.link_expiry(hours, lang) + ) return _send([to], mailtext.text("verify_email.subject", lang), body) diff --git a/backend/app/mailtext.py b/backend/app/mailtext.py index 5573c991..bf172d5c 100644 --- a/backend/app/mailtext.py +++ b/backend/app/mailtext.py @@ -3,7 +3,7 @@ recipient gets (E9.5 S7). Why a catalog instead of literals in `email.py`: HU/EN parity is a house rule for every user-facing string, and a mail is the one user-facing surface the reader CANNOT re-render by switching the UI -language — it arrives once, in whatever language we picked, and stays that way. `test_mail_text.py` +language — it arrives once, in whatever language we picked, and stays that way. `test_mail_language.py` enforces that every key carries both languages *and* the same `{placeholders}`, so a half-translated mail fails the gate instead of reaching a person. @@ -36,6 +36,15 @@ def text(key: str, lang: str | None, **values: object) -> str: return MAIL_TEXT[key][normalize(lang)].format(**values) +def link_expiry(hours: int, lang: str) -> str: + """How long a link lasts, in a unit a reader converts on sight. A whole number of days is + said in days — the approval mail's token lives seven of them, and "168 hours" is not an answer + to "how long have I got". A single day stays "24 hours", which is how everyone writes it.""" + if hours >= 48 and hours % 24 == 0: + return text("duration.days", lang, n=hours // 24) + return text("duration.hours", lang, n=hours) + + def lang_for_user(user: User | None) -> str: """The language a signed-up recipient gets: the one they picked in the app.""" prefs = (user.preferences or {}) if user is not None else {} @@ -72,6 +81,11 @@ def lang_for_email(db: Session, email: str) -> str: # The brand line ("— Siftlode") and the product name are deliberately NOT catalog entries: they are # a name, not a translated string, and giving them keys would invite someone to "translate" them. MAIL_TEXT: dict[str, dict[str, str]] = { + # ── How long a link lasts ─────────────────────────────────────────────────────────────── + # Rendered, not interpolated as a raw hour count: the approval mail's token lives seven days + # (E9.5 S6), and "expires in 168 hours" is a unit no reader converts. + "duration.hours": {"en": "{n} hours", "hu": "{n} óra"}, + "duration.days": {"en": "{n} days", "hu": "{n} nap"}, # ── Shared closing fragments ──────────────────────────────────────────────────────────────── "contact.reply": { "en": "\n\nQuestions? Just reply to this email ({admin}).", @@ -105,14 +119,14 @@ MAIL_TEXT: dict[str, dict[str, str]] = { "You already started an account, so there is just one step left. Confirm your email " "address and you're in:\n\n" "{link}\n\n" - "The link works once and expires in {hours} hours. If it does, you can ask for a fresh " + "The link works once and expires in {expiry}. If it does, you can ask for a fresh " "one from the sign-in page." ), "hu": ( "A fiókodat már elkezdted, úgyhogy egyetlen lépés maradt. Erősítsd meg az " "e-mail-címed, és bent is vagy:\n\n" "{link}\n\n" - "A link egyszer használható, és {hours} óra múlva lejár. Ha lejárt, a belépési oldalon " + "A link egyszer használható, és {expiry} múlva lejár. Ha lejárt, a belépési oldalon " "kérhetsz újat." ), }, @@ -304,7 +318,7 @@ MAIL_TEXT: dict[str, dict[str, str]] = { "Hi,\n\n" "Confirm your email to finish creating your Siftlode account:\n\n" "{link}\n\n" - "The link works once and expires in {hours} hours. If it does, you can ask for a fresh " + "The link works once and expires in {expiry}. If it does, you can ask for a fresh " "one from the sign-in page.\n\n" "If you didn't sign up, you can ignore this message.\n\n" "— Siftlode" @@ -313,7 +327,7 @@ MAIL_TEXT: dict[str, dict[str, str]] = { "Szia,\n\n" "Erősítsd meg az e-mail-címed, hogy elkészüljön a Siftlode-fiókod:\n\n" "{link}\n\n" - "A link egyszer használható, és {hours} óra múlva lejár. Ha lejárt, a belépési oldalon " + "A link egyszer használható, és {expiry} múlva lejár. Ha lejárt, a belépési oldalon " "kérhetsz újat.\n\n" "Ha nem te regisztráltál, nyugodtan hagyd figyelmen kívül ezt a levelet.\n\n" "— Siftlode" diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index b10a90aa..c198bce0 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -17,13 +17,13 @@ from app.auth import ( admin_user, can_activate, count_admins, + env_allowed_canonicals, find_user_by_email, get_or_create_demo_user, is_allowed, operator_contact, purge_user, ) -from app.config import settings from app.db import get_db from app.demo import reset_demo_state from app.models import ( @@ -59,9 +59,11 @@ def _accounts_for(db: Session, emails: Iterable[str]) -> set[str]: def _env_allowed(email: str) -> bool: """Whether the env bootstrap lists still let this address in. `is_allowed` consults them only when no Invite row exists, so this is exactly what deleting the row would fall back to — and the - admin has to know that BEFORE they confirm, not from a toast afterwards.""" - email = (email or "").lower() - return email in settings.allowed_email_set or email in settings.admin_email_set + admin has to know that BEFORE they confirm, not from a toast afterwards. + + Canonical, because `is_allowed` is: a warning that disagrees with the gate it is warning about is + worse than no warning.""" + return canonical_email(email or "") in env_allowed_canonicals() def _serialize(inv: Invite, *, has_account: bool = False) -> dict: @@ -134,14 +136,20 @@ def _activate_user_by_email(db: Session, email: str) -> None: """Approving access activates a matching (e.g. pending password-registration) account so it can sign in. No-op if no such user exists yet (a Google user is created at first login). - E9.5 S1: this path deliberately does NOT revoke a preset password. Approval is not an ownership - attestation, and it does not need to be one — `password_login` refuses an unverified account - BEFORE it looks at `is_active`, so a pre-emptor who set a password on someone else's address - still cannot sign in with it, approved or not. Revoking here only ever destroyed the password a - legitimate registrant had just chosen, and made "your password was cleared for security" the - DEFAULT experience, because approve-before-verify is the normal ordering. The revoke stays on - the Google-adoption path (auth.revoke_preset_password_if_unverified), where the real owner does - attest ownership and the preset password must not survive.""" + E9.5 S1: this path deliberately does NOT revoke a preset password. Revoking here destroyed the + password a legitimate registrant had just chosen and made "your password was cleared for + security" the DEFAULT experience, because approve-before-verify is the normal ordering. + + S1 justified that by saying `password_login` refuses an unverified account BEFORE it looks at + `is_active`, so a pre-emptor could not sign in "approved or not". A review of the finished epic + showed that argument only holds while the address stays unverified — and S6's approval mail + hands the real owner a confirmation link precisely so they will end that state. What actually + guards this now is `_register_account`: while an address is unverified the NEWEST registration + replaces the password and kills the outstanding verify tokens, so the real owner takes the + account back by registering, and a pre-emptor's password is only ever as good as the owner's + silence. The residual (an owner who confirms without ever registering or resetting) is logged + for E12 beside SEC-15. The revoke also stays on the Google-adoption path + (auth.revoke_preset_password_if_unverified), where ownership is actually attested.""" # Canonical, with the literal spelling preferred when both exist: approving # `npeter83.temu@gmail.com` must activate the account that registered as `npeter83temu@gmail.com`, # since Gmail delivers both to one mailbox and the admin decided about the mailbox. @@ -236,7 +244,11 @@ def add_invite( # exactly that pair for one person from 2026-06-16 (hartmann.peter1986@ / hartmannpeter1986@), # which is how an admin ends up approving the same human twice and still seeing them bounce. canon = canonical_email(email) - rows = db.execute(select(Invite).where(Invite.email_canonical == canon)).scalars().all() + rows = ( + db.execute(select(Invite).where(Invite.email_canonical == canon).order_by(Invite.id)) + .scalars() + .all() + ) inv = next((r for r in rows if r.email == email), None) or next(iter(rows), None) if inv is None: inv = Invite(email=email) @@ -262,7 +274,6 @@ class InviteSendIn(BaseModel): @router.post("/invites/{invite_id}/invite") def send_invitation( invite_id: int, - background: BackgroundTasks, body: InviteSendIn | None = None, admin: User = Depends(admin_user), db: Session = Depends(get_db), @@ -288,6 +299,10 @@ def send_invitation( # but a button the admin pressed must not answer with a success toast for a mail that was # never sent. raise HTTPException(status_code=409, detail={"code": "invite_send", "reason": "email_off"}) + # Same as the other two paths that decide for an address (`approve_invite`, `add_invite`): an + # approved row means this mailbox may come in, so finish it. Without this the mail can promise + # "your account is ready — just sign in" to someone `password_login` will refuse for `is_active`. + _activate_user_by_email(db, inv.email) link, has_account, hours = _welcome_mail_parts(db, inv.email) # An explicit pick from the dialog is recorded, so a later mail to this address matches the one # the admin already sent. Only the explicit pick — writing the fallback back would turn a column @@ -297,6 +312,15 @@ def send_invitation( lang = inv.lang = picked else: lang = mailtext.lang_for_email(db, inv.email) + # Sent INLINE, so `invited_at` records the send rather than the intent. Every other mail here is + # fail-soft in the background, which is right when nobody is waiting on it — but this column + # exists to answer "did I already write to them?", and the UI dims the button and warns about a + # second mail on the strength of it. Stamping it before a send that then failed makes the one + # question it answers a lie, and talks the admin out of the retry that would fix it. + if not email_mod.send_access_invitation(inv.email, lang, link, has_account, hours): + raise HTTPException( + status_code=422, detail={"code": "invite_send", "reason": "send_failed"} + ) inv.invited_at = datetime.now(timezone.utc) audit.record( db, @@ -307,7 +331,6 @@ def send_invitation( summary=inv.email, ) db.commit() - background.add_task(email_mod.send_access_invitation, inv.email, lang, link, has_account, hours) return _serialize(inv, has_account=has_account) @@ -646,7 +669,11 @@ def resend_user_verification( # Mail is fail-soft everywhere else, which is what left the 0.78.5 sign-ups with nothing to # fall back on. An admin pressing a button deserves the failure, not a success toast. raise HTTPException(status_code=409, detail={"code": "resend", "reason": "email_off"}) - send_verification_link(db, target) + if not send_verification_link(db, target): + # Same rule as the invitation above: a button an admin pressed must not answer with a + # success for a mail that was dropped. Mail is fail-soft everywhere else because nobody is + # waiting on the answer; here somebody is. + raise HTTPException(status_code=422, detail={"code": "resend", "reason": "send_failed"}) audit.record( db, admin.id, diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 712a20ef..528f2c93 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -13,7 +13,7 @@ from authlib.integrations.starlette_client import OAuth, OAuthError from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request from fastapi.responses import JSONResponse, RedirectResponse from pydantic import BaseModel -from sqlalchemy import select, update +from sqlalchemy import delete, select, update from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session @@ -28,6 +28,7 @@ from app.auth import ( READ_SCOPES, WRITE_SCOPES, current_user, + env_admin_canonicals, find_user_by_email, get_or_create_demo_user, google_enabled, @@ -229,9 +230,20 @@ def upsert_pending_invite(db: Session, email: str, lang: str | None = None) -> I return InviteRequest(None, False) lang = lang if lang in mailtext.SUPPORTED else None canon = canonical_email(email) - rows = db.execute(select(Invite).where(Invite.email_canonical == canon)).scalars().all() - # Prefer a decided row over a pending one, so a denied duplicate can't mask an approval. - inv = next((r for r in rows if r.status == "approved"), None) or next(iter(rows), None) + rows = ( + db.execute(select(Invite).where(Invite.email_canonical == canon).order_by(Invite.id)) + .scalars() + .all() + ) + # An approved row wins, so a denied duplicate can't mask an approval; then the exact spelling; + # then the oldest. Without the middle rule an unordered scan could hand back a DENIED sibling + # while a pending row already existed, and the denied branch below would reopen it — re-notifying + # every admin and putting one person in the queue twice. + inv = ( + next((r for r in rows if r.status == "approved"), None) + or next((r for r in rows if r.email == email), None) + or next(iter(rows), None) + ) if inv is not None: relearned = lang is not None and inv.lang != lang if relearned: @@ -467,7 +479,9 @@ async def callback( # be locked out. Everyone else's role is managed in the admin UI and stored in the DB, so a # routine login must NOT reset it (that would clobber a UI promotion/demotion). Only seed a # default for a brand-new account (its role attribute is still unset before flush). - if email in settings.admin_email_set: + # Canonical, like every other read of the env lists: a dotted or `+tag`ged ADMIN_EMAILS entry + # would otherwise never bootstrap its admin, because Google answers with the plain primary. + if canonical_email(email) in env_admin_canonicals(): user.role = "admin" elif not user.role: user.role = "user" @@ -479,8 +493,7 @@ async def callback( # picked one yet); unsupported locales fall back to English. prefs = dict(user.preferences or {}) if not prefs.get("language"): - loc = (userinfo.get("locale") or "").split("-")[0].lower() - prefs["language"] = loc if loc in {"en", "hu"} else "en" + prefs["language"] = mailtext.normalize((userinfo.get("locale") or "").split("-")[0].lower()) user.preferences = prefs db.flush() @@ -635,7 +648,7 @@ async def request_access( db: Session = Depends(get_db), ) -> dict: """Public: ask for access. Idempotent — repeat requests for the same email don't - duplicate or re-spam admins (upsert returns None for an already-pending row). + duplicate or re-spam admins (upsert answers `is_new=False` for an already-pending row). Answers a uniform ``"pending"`` for EVERY outcome — already-allowed, newly-pending, already-pending, and rate-limited alike. The old ``"approved"`` reply for an allow-listed @@ -799,9 +812,20 @@ def _register_account(email: str, password: str, lang: str | None = None) -> Non # Canonical, so a second spelling of one Gmail mailbox cannot open a SECOND account. Prod # already carries such a pair (user ids 13 and 14, both for npeter83temu@gmail.com); this is # the end that stops new ones appearing, while the Google callback copes with the existing. - existing = db.execute( - select(User).where(User.email_canonical == canonical_email(email)) - ).first() + candidates = ( + db.execute( + select(User).where(User.email_canonical == canonical_email(email)).order_by(User.id) + ) + .scalars() + .all() + ) + # Exact spelling wins when a mailbox carries two rows, same as every other canonical lookup + # in this epic (`find_user_by_email`, `_activate_user_by_email`, the Google callback). Without + # it `.first()` handed back whichever row Postgres felt like, so a re-registration of the + # dotted spelling could resolve to the undotted, already-confirmed twin and send nothing. + existing = next((u for u in candidates if u.email == email), None) or next( + iter(candidates), None + ) if existing is not None: # Already registered. This used to return in silence, and that silence is the most # likely explanation for the 2026-09-03 report that a confirmation mail "never arrived": @@ -813,9 +837,29 @@ def _register_account(email: str, password: str, lang: str | None = None) -> Non # ON THE ACCOUNT, and this is exactly what the public /auth/verify/resend already does # for the same input — so it is not a new capability, just the end of a dead end. An # account that is already confirmed still gets nothing: there is nothing to confirm. - user = existing[0] - if not user.email_verified and not user.is_demo and email_mod.email_enabled(): - send_verification_link(db, user) + user = existing + if not user.email_verified and not user.is_demo: + # While the address is UNVERIFIED, the newest registration owns the account. + # + # Anyone can post any address here, so the first registration for a stranger's + # mailbox used to own it permanently: the password stayed, approval activated the + # account (E9.5 S1 stopped revoking it), and the confirmation link the approval mail + # carries goes to the real owner — whose click would open sign-in for the squatter's + # password. Letting the later registration replace the hash makes that pre-emption + # worthless the moment the real owner registers, and it costs a legitimate registrant + # nothing: re-registering an address you already registered is you, choosing a + # password again. Every verify token issued to the previous holder dies with it, so a + # link already in flight cannot confirm the account for them either. + user.password_hash = hash_password(password) + user.password_revoked_at = None + db.execute( + delete(AuthToken).where( + AuthToken.user_id == user.id, AuthToken.kind == "verify" + ) + ) + db.commit() + if email_mod.email_enabled(): + send_verification_link(db, user) return # Without working SMTP we can't deliver a verification link, so email ownership can't gate # sign-in. Admin approval stays the real gate (is_active=False); mark the account verified so @@ -827,7 +871,7 @@ def _register_account(email: str, password: str, lang: str | None = None) -> Non is_active=False, email_verified=not email_ok, # Only a supported code is stored, so a forged body can't put junk in preferences. - preferences={"language": lang} if lang in ("en", "hu") else {}, + preferences={"language": lang} if lang in mailtext.SUPPORTED else {}, ) db.add(user) try: @@ -867,7 +911,7 @@ def issue_verification_link(db: Session, user: User, ttl: timedelta | None = Non return f"{_app_base()}/#verify={raw}" -def send_verification_link(db: Session, user: User) -> None: +def send_verification_link(db: Session, user: User) -> bool: """Issue a fresh verification token for `user` and mail them the link. Always to the address ON THE ACCOUNT, never a spelling typed into some form: the account's @@ -877,8 +921,11 @@ def send_verification_link(db: Session, user: User) -> None: One definition, three callers — registration, the self-service resend, and the admin-side resend (E9.5 S5). The link's shape and TTL must not drift between them; a verification mail that differs by who triggered it is a bug waiting to be discovered by exactly one user. + + Returns whether the mail actually went out, so a caller with a human waiting on the answer (the + admin-side resend) can say so instead of reporting success for a message that was dropped. """ - email_mod.send_verify_email( + return email_mod.send_verify_email( user.email, mailtext.lang_for_user(user), issue_verification_link(db, user), @@ -1125,12 +1172,18 @@ def revoke_preset_password_if_unverified(user: User) -> None: so a legit no-SMTP registration is unaffected. The Google-adoption callback is the ONLY caller (E9.5 S1). Admin approval used to call this too, and - that was redundant: `password_login` refuses an unverified account before it even reads `is_active`, - so approval hands a pre-emptor's password nothing it could use. All the admin-path revoke achieved - was destroying the password a legitimate registrant had just chosen — and since the admin normally - decides before the user reads their mail, that made the "your password was cleared" detour the - DEFAULT path rather than an edge case. Keep it that way: if a second caller ever looks justified, - check first whether the gate it is guarding already runs earlier. + dropping it was right for the reason S1 gave — the revoke destroyed the password a legitimate + registrant had just chosen, and since the admin normally decides before the user reads their mail, + that made the "your password was cleared" detour the DEFAULT path rather than an edge case. + + S1 also claimed the revoke was REDUNDANT there, because `password_login` refuses an unverified + account before it reads `is_active`. That part was wrong, and the epic's own review caught it: the + gate holds only while the address is unverified, and S6's approval mail exists to get the real + owner to end that state. The guard that actually replaced it lives in `_register_account` — while + an address is unverified, the newest registration replaces the password and deletes the + outstanding verify tokens, so the owner reclaims the account by registering. Do not re-add a + caller here without re-reading that: two revokes racing would put "your password was cleared" + back in front of every legitimate registrant. `password_revoked_at` is what makes "the owner re-sets one via password-reset" actually true. The revoke leaves `password_hash IS NULL`, which is also what a Google-only account looks like — and diff --git a/backend/tests/test_admin_pending_access.py b/backend/tests/test_admin_pending_access.py index d337050a..c4cedd87 100644 --- a/backend/tests/test_admin_pending_access.py +++ b/backend/tests/test_admin_pending_access.py @@ -128,7 +128,9 @@ def test_an_unconfirmed_account_is_listed_and_can_be_sent_a_fresh_link( monkeypatch.setattr(admin_routes.email_mod, "email_enabled", lambda db=None: True) sent: list[tuple[str, str]] = [] monkeypatch.setattr( - auth_routes.email_mod, "send_verify_email", lambda to, lang, link, **kw: sent.append((to, link)) + auth_routes.email_mod, + "send_verify_email", + lambda to, lang, link, **kw: sent.append((to, link)) is None, ) admin = make_user(email="admin@example.com", role="admin") # Active but unconfirmed — a real state since E9.5 S1 made the two gates order-independent. It is diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index b2c47cc4..676e24cd 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -200,7 +200,9 @@ def _no_smtp(monkeypatch): notified: list[str] = [] monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False) monkeypatch.setattr( - auth_mod.email_mod, "send_admin_new_request", lambda admins, lang, to: notified.append(to) or True + auth_mod.email_mod, + "send_admin_new_request", + lambda admins, lang, to: notified.append(to) or True, ) return notified @@ -422,7 +424,9 @@ def _capture_reset_links(monkeypatch) -> list[str]: from app.routes import auth as auth_mod sent: list[str] = [] - monkeypatch.setattr(auth_mod.email_mod, "send_password_reset", lambda to, lang, link: sent.append(to)) + monkeypatch.setattr( + auth_mod.email_mod, "send_password_reset", lambda to, lang, link: sent.append(to) + ) return sent diff --git a/backend/tests/test_email_canonical.py b/backend/tests/test_email_canonical.py index 144d20e1..c5eca415 100644 --- a/backend/tests/test_email_canonical.py +++ b/backend/tests/test_email_canonical.py @@ -166,7 +166,9 @@ def _capture_reset_links(monkeypatch) -> list[str]: from app.routes import auth as auth_mod sent: list[str] = [] - monkeypatch.setattr(auth_mod.email_mod, "send_password_reset", lambda to, lang, link: sent.append(to)) + monkeypatch.setattr( + auth_mod.email_mod, "send_password_reset", lambda to, lang, link: sent.append(to) + ) return sent diff --git a/backend/tests/test_mail_language.py b/backend/tests/test_mail_language.py index 0f60ae86..ce1c38df 100644 --- a/backend/tests/test_mail_language.py +++ b/backend/tests/test_mail_language.py @@ -7,6 +7,7 @@ language a given recipient actually gets, and where that signal was captured, si with no account it exists only at the moment they asked us. """ +import re import string import pytest @@ -42,15 +43,38 @@ def test_every_translation_takes_the_same_placeholders(key): assert _placeholders(template) == expected, f"{key} [{lang}]" -def test_no_catalog_entry_is_orphaned(): - """Every key is referenced from the app, so a mail that stopped being sent can't leave its text - behind to be 'kept up to date' by the next person who greps for it.""" +def _app_sources_without_the_catalog() -> str: + """Every app source, with the catalog LITERAL cut out of mailtext.py. + + Cutting it is the whole point: the dict spells each key as a quoted literal, so a scan that + included it reported every key as used no matter what the catalog held. Cutting the whole FILE + is too much, though — `link_expiry` legitimately asks for `duration.*`, and those keys would + then look orphaned.""" from pathlib import Path app_dir = Path(__file__).resolve().parents[1] / "app" - sources = "\n".join(p.read_text(encoding="utf-8") for p in app_dir.rglob("*.py")) - unused = [k for k in mailtext.MAIL_TEXT if f'"{k}"' not in sources] - assert unused == [] + out = [] + for f in app_dir.rglob("*.py"): + src = f.read_text(encoding="utf-8") + if f.name == "mailtext.py": + src = src[: src.index("MAIL_TEXT: dict")] + out.append(src) + return "\n".join(out) + + +def test_no_catalog_entry_is_orphaned(): + """Every key is referenced from the app, so a mail that stopped being sent can't leave its text + behind to be 'kept up to date' by the next person who greps for it.""" + sources = _app_sources_without_the_catalog() + assert [k for k in mailtext.MAIL_TEXT if f'"{k}"' not in sources] == [] + + +def test_every_key_the_app_asks_for_exists(): + """The other direction, and the one that raises in production: `text()` looks the key up in a + dict, so a renamed or mistyped key is a KeyError inside a background task with no user in sight.""" + asked = set(re.findall(r'text\(\s*"([a-z_]+\.[a-z_]+)"', _app_sources_without_the_catalog())) + assert asked, "the scan found no mailtext.text(...) call sites — the pattern has rotted" + assert sorted(asked - set(mailtext.MAIL_TEXT)) == [] def test_an_unknown_language_falls_back_to_english_instead_of_raising(): @@ -157,7 +181,9 @@ def _bodies(monkeypatch): out: list[tuple[list[str], str, str]] = [] monkeypatch.setattr( - email_mod, "_send", lambda to, subject, body, reply_to=None: out.append((to, subject, body)) + email_mod, + "_send", + lambda to, subject, body, reply_to=None: out.append((to, subject, body)) is None, ) return out diff --git a/backend/tests/test_review_round.py b/backend/tests/test_review_round.py new file mode 100644 index 00000000..97cc9320 --- /dev/null +++ b/backend/tests/test_review_round.py @@ -0,0 +1,317 @@ +"""E9.5 review round — the invariants the epic's own review found unpinned. + +Every test here corresponds to a defect a reviewer could still introduce with the whole suite green: +a mail whose body never renders in any test, an activation that only works for the exact spelling, a +"we already wrote to them" stamp for a mail that was dropped, an env allow-list that answers a +different question than the gate it backs, and the address pre-emption that E9.5 S1's argument +covered only while the address stayed unverified. +""" + +from datetime import datetime, timedelta, timezone + +import pytest +from sqlalchemy import select + +from app import auth as auth_core +from app import email as email_mod +from app import mailtext +from app.config import settings +from app.models import AuthToken, Invite, User +from app.routes import auth as auth_routes + +_PW = "correct horse battery" + + +@pytest.fixture +def wire(monkeypatch): + """Capture what `_send` would have put on the wire. Mails are the one surface where a body that + never renders in a test is a body that first renders in someone's inbox.""" + out: list[tuple[list[str], str, str]] = [] + monkeypatch.setattr( + email_mod, + "_send", + lambda to, subject, body, reply_to=None: out.append((to, subject, body)) is None, + ) + return out + + +@pytest.fixture +def smtp_on(monkeypatch): + monkeypatch.setattr(email_mod, "email_enabled", lambda db=None: True) + + +# --- every mail actually renders ------------------------------------------------------------------ + +# One call per `send_*`, with arguments of the right shape. The parity test compares HU against EN +# and can therefore never catch a template whose placeholders disagree with its CALLER — that +# mismatch is a KeyError inside a background task, i.e. a mail that silently never goes out. Three +# mailers this epic rewrote (`password_reset`, `role_changed`, `account_unsuspended`) had no test +# that executed their `.format()` at all. +_EVERY_MAIL = [ + ("send_access_approved", ("to@example.com", "LANG", "https://x/#verify=t", True, 168)), + ("send_access_approved", ("to@example.com", "LANG", None, True, 24)), + ("send_access_approved", ("to@example.com", "LANG", None, False, 24)), + ("send_access_invitation", ("to@example.com", "LANG", "https://x/#verify=t", True, 168)), + ("send_access_invitation", ("to@example.com", "LANG", None, False, 24)), + ("send_access_denied", ("to@example.com", "LANG")), + ("send_role_changed", ("to@example.com", "LANG", "admin", "op@example.com")), + ("send_role_changed", ("to@example.com", "LANG", "user", None)), + ("send_account_unsuspended", ("to@example.com", "LANG", "op@example.com")), + ("send_account_deleted", ("to@example.com", "LANG", None)), + ("send_account_suspended", ("to@example.com", "LANG", "op@example.com")), + ("send_account_suspended", ("to@example.com", "LANG", None)), + ("send_admin_new_request", (["a@example.com"], "LANG", "who@example.com")), + ("send_verify_email", ("to@example.com", "LANG", "https://x/#verify=t", 24)), + ("send_password_reset", ("to@example.com", "LANG", "https://x/#reset=t")), + ("send_test", ("to@example.com", "LANG")), +] + + +@pytest.mark.parametrize("lang", mailtext.SUPPORTED) +@pytest.mark.parametrize("name,args", _EVERY_MAIL, ids=[f"{n}{a[2:3]}" for n, a in _EVERY_MAIL]) +def test_every_mail_renders_in_every_language(wire, name, args, lang): + getattr(email_mod, name)(*[lang if a == "LANG" else a for a in args]) + + (_, subject, body) = wire[0] + assert subject and body + # A template that lost its interpolation renders the brace text verbatim. + assert "{" not in subject and "{" not in body + + +def test_a_weeks_link_is_described_in_days_not_hours(wire): + # The S6 approval token lives seven days, and the mail used to say "expires in 168 hours". + email_mod.send_access_approved("to@example.com", "en", "https://x/#verify=t", True, 168) + assert "7 days" in wire[0][2] and "168" not in wire[0][2] + + email_mod.send_verify_email("to@example.com", "hu", "https://x/#verify=t", 24) + assert "24 óra" in wire[1][2] + + +# --- the pre-emption guard: while unverified, the newest registration owns the account ------------- + + +def test_registering_again_takes_the_account_back_from_a_pre_emptor(db, client, smtp_on, wire): + """The hole E9.5 S1's argument left open, and the guard that replaces it. + + S1 stopped admin approval revoking a preset password, reasoning that `password_login` refuses an + unverified account before it reads `is_active`. True — but S6's approval mail exists to get the + real owner to confirm the address, and that click opens the gate for whatever password is on the + row. So the owner must be able to take the row back, and re-registering is how. + """ + client.post("/auth/register", json={"email": "victim@example.com", "password": "pre-emptor pw"}) + db.expire_all() + user = db.execute(select(User).where(User.email == "victim@example.com")).scalar_one() + squatted = user.password_hash + assert squatted is not None + + client.post( + "/auth/register", json={"email": "victim@example.com", "password": "the owner's pw"} + ) + + db.expire_all() + user = db.execute(select(User).where(User.email == "victim@example.com")).scalar_one() + assert user.password_hash != squatted + # One account, not two — the canonical pre-check still holds. + assert ( + len(db.execute(select(User).where(User.email_canonical == "victim@example.com")).all()) == 1 + ) + + +def test_the_pre_emptors_verify_link_dies_with_their_password(db, client, smtp_on, wire): + client.post("/auth/register", json={"email": "victim@example.com", "password": "pre-emptor pw"}) + db.expire_all() + user = db.execute(select(User).where(User.email == "victim@example.com")).scalar_one() + first = db.execute(select(AuthToken.id).where(AuthToken.user_id == user.id)).scalars().all() + assert len(first) == 1 + + client.post( + "/auth/register", json={"email": "victim@example.com", "password": "the owner's pw"} + ) + + db.expire_all() + live = db.execute(select(AuthToken).where(AuthToken.user_id == user.id)).scalars().all() + # Exactly one token, and not the one already in the pre-emptor's hands: otherwise they could + # confirm the address themselves and re-open the gate for a password that is no longer theirs. + assert len(live) == 1 and live[0].id != first[0] + + +def test_a_confirmed_account_is_not_re_opened_by_a_later_registration( + db, client, smtp_on, make_user +): + # The guard is scoped to the UNVERIFIED window. Once the address is confirmed the account is its + # owner's, and a stranger posting the form must not touch the password. + user = make_user(email="done@example.com", password=_PW) # already verified + before = user.password_hash + + client.post("/auth/register", json={"email": "done@example.com", "password": "not yours"}) + + db.expire_all() + assert db.get(User, user.id).password_hash == before + + +# --- the env bootstrap answers the same question as the gate -------------------------------------- + + +def test_the_env_allow_list_is_matched_canonically(db, monkeypatch): + # An operator writing the dotted spelling into ALLOWED_EMAILS was refused when Google answered + # with the undotted primary — the 2026-09-03 lockout, on the bootstrap path. + monkeypatch.setattr(settings, "allowed_emails", "first.last@gmail.com") + + assert auth_core.is_allowed(db, "firstlast@gmail.com") is True + assert auth_core.is_allowed(db, "first.last+tag@gmail.com") is True + assert auth_core.is_allowed(db, "someone.else@gmail.com") is False + + +def test_an_env_granted_address_cannot_be_shadowed_by_a_stranger(db, client, monkeypatch): + # `is_allowed` short-circuits on ANY invite row for the canonical form, so before this an + # unauthenticated caller could request access under a sibling spelling and permanently shadow an + # env grant. With the env list matched canonically the request never opens a row. + monkeypatch.setattr(settings, "allowed_emails", "bob@gmail.com") + + client.post("/auth/request-access", json={"email": "b.ob@gmail.com"}) + + assert db.execute(select(Invite)).scalars().all() == [] + assert auth_core.is_allowed(db, "bob@gmail.com") is True + + +def test_a_dotted_admin_emails_entry_still_bootstraps_its_admin(monkeypatch): + monkeypatch.setattr(settings, "admin_emails", "the.operator+siftlode@gmail.com") + + assert "theoperator@gmail.com" in auth_core.env_admin_canonicals() + + +# --- approving decides about a MAILBOX, not a spelling -------------------------------------------- + + +def test_approving_one_spelling_activates_the_account_registered_under_another( + db, make_user, as_user, smtp_on, wire +): + # The 2026-09-03 incident in one test: the admin approves the address as typed into the request, + # and the account exists under the other Gmail spelling of the same mailbox. + admin = make_user(email="admin@example.com", role="admin") + user = make_user(email="dotless@gmail.com", password=_PW, is_active=False) + user.email = "dotless@gmail.com" + inv = Invite(email="dot.less@gmail.com", status="pending") + db.add(inv) + db.commit() + + r = as_user(admin).post(f"/api/admin/invites/{inv.id}/approve") + + assert r.status_code == 200 + db.expire_all() + assert db.get(User, user.id).is_active is True + + +def test_deleting_one_of_two_accounts_leaves_the_other_one_allowed(db, make_user, as_user): + # Prod carries two accounts for one Gmail mailbox. A canonical invite sweep on delete took the + # survivor's grant with it, refusing their next sign-in — this epic's founding bug, self-inflicted. + admin = make_user(email="admin@example.com", role="admin") + doomed = make_user(email="pair.one@gmail.com", password=_PW) + make_user(email="pairone@gmail.com", password=_PW) + db.add_all([Invite(email="pair.one@gmail.com", status="approved")]) + db.commit() + + r = as_user(admin).delete(f"/api/admin/users/{doomed.id}") + + assert r.status_code == 200 + assert auth_core.is_allowed(db, "pairone@gmail.com") is True + + +# --- a button an admin pressed must not report a send that did not happen ------------------------- + + +def test_a_failed_invitation_is_not_recorded_as_written( + db, make_user, as_user, smtp_on, monkeypatch +): + monkeypatch.setattr(email_mod, "send_access_invitation", lambda *a, **kw: False) + admin = make_user(email="admin@example.com", role="admin") + inv = Invite(email="stranger@example.com", status="approved") + db.add(inv) + db.commit() + + r = as_user(admin).post(f"/api/admin/invites/{inv.id}/invite", json={"lang": "en"}) + + assert r.status_code == 422 and r.json()["detail"]["reason"] == "send_failed" + db.expire_all() + # The column exists to answer "did I already write to them?", and the UI dims the button and + # warns about a second mail on the strength of it. A dropped send must not make it say yes. + assert db.get(Invite, inv.id).invited_at is None + + +def test_a_failed_resend_says_so_instead_of_reporting_ok( + db, make_user, as_user, smtp_on, monkeypatch +): + monkeypatch.setattr(email_mod, "send_verify_email", lambda *a, **kw: False) + admin = make_user(email="admin@example.com", role="admin") + stuck = make_user(email="stuck@example.com", password=_PW, email_verified=False) + + r = as_user(admin).post(f"/api/admin/users/{stuck.id}/resend-verification") + + assert r.status_code == 422 and r.json()["detail"]["reason"] == "send_failed" + + +# --- refusals and links that are already dead ------------------------------------------------------ + + +def test_an_expired_reset_link_is_reported_dead_without_being_spent(db, client, make_user): + # The endpoint exists because a spent or stale link used to draw the full "set a new password" + # form. Expiry is the reason its docstring gives, and it was the one case no test covered. + user = make_user(email="late@example.com", password=_PW) + raw = auth_routes._issue_token(db, user, "reset", timedelta(hours=1)) + token = db.execute(select(AuthToken).where(AuthToken.user_id == user.id)).scalar_one() + token.expires_at = datetime.now(timezone.utc) - timedelta(minutes=1) + db.commit() + + assert client.post("/auth/password-reset/check", json={"token": raw}).json() == {"ok": False} + r = client.post( + "/auth/password-reset/confirm", json={"token": raw, "password": "a new one 123"} + ) + assert r.status_code == 400 + + +def test_a_verify_token_cannot_pass_as_a_reset_link(db, client, make_user): + user = make_user(email="mixed@example.com", password=_PW) + raw = auth_routes._issue_token(db, user, "verify", timedelta(hours=1)) + + assert client.post("/auth/password-reset/check", json={"token": raw}).json() == {"ok": False} + + +def test_a_throttled_login_says_nothing_about_the_account(db, client, make_user, monkeypatch): + # The two refusals that answer BEFORE the password is proven must stay byte-identical for a real + # account and an unknown one; only `invalid` was pinned. + make_user(email="real@example.com", password=_PW) + monkeypatch.setattr(auth_routes._login_limiter, "allow", lambda key: False) + + real = client.post("/auth/password-login", json={"email": "real@example.com", "password": _PW}) + unknown = client.post( + "/auth/password-login", json={"email": "nobody@example.com", "password": _PW} + ) + + assert real.status_code == unknown.status_code == 429 + assert real.json() == unknown.json() == {"detail": {"code": "login", "reason": "rate_limited"}} + + +# --- admins hear about a request in their own language -------------------------------------------- + + +def test_two_admins_two_languages_two_mails(db, client, make_user, smtp_on, wire): + make_user(email="hu.admin@example.com", role="admin", preferences={"language": "hu"}) + make_user(email="en.admin@example.com", role="admin", preferences={"language": "en"}) + + client.post("/auth/request-access", json={"email": "asking@example.com", "lang": "en"}) + + notices = [w for w in wire if "asking@example.com" in w[1]] + assert len(notices) == 2 + by_recipient = {w[0][0]: w[1] for w in notices} + assert by_recipient["hu.admin@example.com"] == mailtext.MAIL_TEXT["admin_new_request.subject"][ + "hu" + ].format(requester="asking@example.com") + assert by_recipient["en.admin@example.com"] == mailtext.MAIL_TEXT["admin_new_request.subject"][ + "en" + ].format(requester="asking@example.com") + + +def test_an_env_admin_without_an_account_is_written_to_in_the_default(db, monkeypatch): + monkeypatch.setattr(settings, "admin_emails", "bootstrap@example.com") + + assert auth_routes.admin_notify_groups(db) == {mailtext.DEFAULT: ["bootstrap@example.com"]} diff --git a/frontend/src/components/AdminUsers.tsx b/frontend/src/components/AdminUsers.tsx index 38a7aa68..64d094a0 100644 --- a/frontend/src/components/AdminUsers.tsx +++ b/frontend/src/components/AdminUsers.tsx @@ -501,6 +501,10 @@ function AdminInvites() { const refresh = () => { qc.invalidateQueries({ queryKey: qk.adminInvites() }); qc.invalidateQueries({ queryKey: qk.me() }); // updates the pending badge + // The stuck-sign-ups section below describes the same backlog and changes with every decision + // here: approving an address moves a registrant INTO it (active, still unverified), whitelisting + // one takes a row out. Leaving it stale hides the resend button this epic added for them. + qc.invalidateQueries({ queryKey: qk.adminPendingAccess() }); }; const approve = useMutation({ mutationFn: ({ id }: { id: number; email: string }) => api.approveInvite(id), diff --git a/frontend/src/components/ConfirmProvider.tsx b/frontend/src/components/ConfirmProvider.tsx index 91b5f331..47cc9dd3 100644 --- a/frontend/src/components/ConfirmProvider.tsx +++ b/frontend/src/components/ConfirmProvider.tsx @@ -94,7 +94,7 @@ export function ConfirmProvider({ children }: { children: ReactNode }) { onClose={() => close(null)} maxWidth={choices.length > 1 ? "max-w-md" : "max-w-sm"} > -

{state.opts.message}

+
{state.opts.message}
{/* One action: the familiar Cancel/Confirm row. Two or more: a column, because side by side they wrap raggedly and a wrapped button reads as a second, lesser group — and these are peers. Ordered least to most destructive, top to bottom. */} diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx index dcbef1ca..b9811cda 100644 --- a/frontend/src/components/Welcome.tsx +++ b/frontend/src/components/Welcome.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { useHistorySubview, type SubviewUrl } from "../lib/history"; import { useTranslation } from "react-i18next"; import { @@ -796,9 +796,22 @@ function StatePanel({ children?: React.ReactNode; }) { const { t } = useTranslation(); + // Announced and focused, because this panel REPLACES the form the user just submitted: without a + // live region a screen reader reports nothing at all, and without the focus move the button they + // were standing on is unmounted, dropping focus to so Tab restarts at the top of the page. + // The Banner in this same file got `role="status"` for the same reason; the panel is the louder + // half — it is the whole answer to a submit — so it announces assertively. + const heading = useRef(null); + useEffect(() => heading.current?.focus(), []); return ( -
-

{body}

+
+

+ {body} +

{children}