From 243869401a8586d69e7d54a8f503334fe849bb0e Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 3 Sep 2026 23:32:34 +0200 Subject: [PATCH 01/16] fix(auth): stop discarding a legitimate password on admin approval (E9.5 S1) SEC-10 revoked any password set while an account's email was unverified, on both the Google-adoption and the admin-approval path. On the approval path that was redundant: password_login refuses an unverified account before it ever reads is_active, so approval hands a pre-emptor's password nothing it could use. All the revoke achieved there was destroying the password a legitimate registrant had just chosen -- and since the admin normally decides before the user reads their mail, 'your password was cleared for security' was the DEFAULT experience, not an edge case. The guard stays on the Google-adoption path, where the real owner does attest ownership. password_reset_required and the ok_reset banner stay: accounts stripped on prod before this change have no other way back in. Two existing tests pinned the old rule, so they are rewritten rather than deleted, and the SEC-10 recovery pack now builds the revoked state through the surviving producer. --- backend/app/models.py | 3 +- backend/app/routes/admin.py | 16 ++-- backend/app/routes/auth.py | 27 ++++-- backend/tests/test_auth_session.py | 123 ++++++++++++++++++++-------- frontend/src/components/Welcome.tsx | 7 +- frontend/src/lib/api/auth.ts | 7 +- 6 files changed, 128 insertions(+), 55 deletions(-) diff --git a/backend/app/models.py b/backend/app/models.py index 2544111b..d25911cd 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -47,7 +47,8 @@ class User(Base, TimestampMixin): email: Mapped[str] = mapped_column(String(320), unique=True, index=True) # argon2id hash for email+password login; NULL for Google-only / demo accounts. password_hash: Mapped[str | None] = mapped_column(String(255)) - # When SEC-10 dropped a preset password (see auth.revoke_preset_password_if_unverified). Without + # When SEC-10 dropped a preset password on Google adoption (see the single caller of + # auth.revoke_preset_password_if_unverified; admin approval no longer revokes — E9.5 S1). Without # this trace a revoked account was indistinguishable from a Google-only one, so it failed the # password-reset eligibility check and could never be recovered — it could neither sign in nor # reset. Set = "had a password, lost it to the guard" -> reset-eligible. Cleared whenever a diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index 70ee72f6..24f724fb 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -27,7 +27,6 @@ from app.models import ( Invite, User, ) -from app.routes.auth import revoke_preset_password_if_unverified from app.utils import iso, valid_email router = APIRouter(prefix="/api/admin", tags=["admin"]) @@ -88,13 +87,18 @@ def list_invites( 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).""" + 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.""" user = db.execute(select(User).where(User.email == email.lower())).scalar_one_or_none() if user is not None and not user.is_active: - # SEC-10: same guard as the Google-adoption path — approving an account whose email was never - # verified must not hand a working login to a password set on it by whoever registered the - # address (a pre-emption). Shared single-source helper so the two paths can't drift. - revoke_preset_password_if_unverified(user) user.is_active = True db.commit() diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 8cc9c316..c6ff8a91 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -664,11 +664,13 @@ def verify_email_confirm(body: TokenIn, db: Session = Depends(get_db)) -> dict: """Confirm an email-verification token the SPA read from the URL fragment (SB3). Uniform 400 on any invalid/expired/used token. - `password_reset_required` tells the SPA to send the user straight to the reset flow: an admin who - approved this account BEFORE it was verified tripped SEC-10, so the password chosen at registration - is gone and sign-in would answer with the deliberately uniform "Invalid email or password" — true, - but unreadable to the person who just proved they own the mailbox. Saying so leaks nothing: holding - a valid single-use verify token for this account already proves that much.""" + `password_reset_required` tells the SPA to send the user straight to the reset flow: SEC-10 had + dropped the password this account registered with, so sign-in would answer with the deliberately + uniform "Invalid email or password" — true, but unreadable to the person who just proved they own + the mailbox. Saying so leaks nothing: holding a valid single-use verify token for this account + already proves that much. Since E9.5 S1 the common source of that state is gone (admin approval no + longer revokes), leaving accounts stripped before that change and the Google-adoption path, so this + flag is rare now — but it must stay, because those users have no other way back in.""" user = _consume_token(db, body.token, "verify") if user is None: raise HTTPException( @@ -859,10 +861,17 @@ def bump_session_epoch(user: User) -> None: def revoke_preset_password_if_unverified(user: User) -> None: """SEC-10 (account pre-emption): a password set on an account whose email was NEVER verified is untrusted — whoever registered the address (to pre-empt the real owner) could have chosen it. When - ownership is finally attested (Google adoption, or admin approval), drop that password and bump the - epoch to revoke any session it minted; the owner re-sets one via password-reset. No-op for an - already-verified account, so a legit no-SMTP registration is unaffected. The single source for this - rule — both the Google-callback and admin-activation paths call it so they can't drift. + the real owner attests ownership through Google, drop that password and bump the epoch to revoke any + session it minted; the owner re-sets one via password-reset. No-op for an already-verified account, + 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. `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_auth_session.py b/backend/tests/test_auth_session.py index fe63b04f..35ef8143 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -97,28 +97,73 @@ def test_reset_confirm_is_single_use_and_kills_existing_sessions(client, make_us assert replay.status_code == 400 -def test_activating_an_unverified_account_drops_its_preset_password(db, make_user): - # SEC-10 (account pre-emption): an attacker registers victim@x with a password before the approved - # owner arrives. The address was never verified, so admin approval must NOT hand that pre-set - # password a working login — it is dropped and any session it minted is revoked. +def test_approving_an_unverified_account_keeps_its_password(db, make_user): + # E9.5 S1, inverting an earlier rule. Approval used to drop a password set while the email was + # unverified (SEC-10, account pre-emption). That was redundant: password_login refuses an + # unverified account BEFORE it reads is_active, so approval hands a pre-emptor's password nothing + # — see the pair of tests below. All it did was destroy the password a legitimate registrant had + # just chosen, in what is the NORMAL ordering (the admin decides before the user reads their mail). from app.routes.admin import _activate_user_by_email - victim = make_user( - email="victim@example.com", - password="attacker-chosen", + user = make_user( + email="approved-first@example.com", + password="chosen at registration", email_verified=False, is_active=False, ) - epoch0 = victim.session_epoch or 0 - _activate_user_by_email(db, "victim@example.com") - assert victim.is_active is True - assert victim.password_hash is None - assert (victim.session_epoch or 0) == epoch0 + 1 + epoch0 = user.session_epoch or 0 + + _activate_user_by_email(db, "approved-first@example.com") + + assert user.is_active is True + assert user.password_hash is not None + assert user.password_revoked_at is None + assert (user.session_epoch or 0) == epoch0 # nothing to revoke, so no epoch bump either + + +def test_an_approved_but_unverified_account_still_cannot_sign_in(client, make_user): + # Why dropping the approval-path revoke is safe: this is the gate that actually stops a + # pre-emptor, and it runs whatever the admin decided. + make_user( + email="notyet@example.com", + password="attacker-chosen", + email_verified=False, + is_active=True, + ) + + r = _login(client, "notyet@example.com", "attacker-chosen") + + assert r.status_code == 403 + assert "verify" in r.json()["detail"].lower() + + +def test_verifying_afterwards_lets_the_original_password_in(client, db, make_user): + # The payoff for the legitimate registrant: the password chosen at registration still works once + # the mailbox is proven — no "your password was cleared for security" detour, no reset round-trip. + from app.routes.auth import _issue_token + + user = make_user( + email="ordering@example.com", + password="chosen at registration", + email_verified=False, + is_active=False, + ) + from app.routes.admin import _activate_user_by_email + + _activate_user_by_email(db, "ordering@example.com") # admin decides FIRST — the common ordering + raw = _issue_token(db, user, "verify", timedelta(hours=1)) + db.commit() + + verified = client.post("/auth/verify", json={"token": raw}) + + assert verified.status_code == 200 + assert verified.json() == {"ok": True, "password_reset_required": False} + assert _login(client, "ordering@example.com", "chosen at registration").status_code == 200 def test_activating_a_verified_account_keeps_its_password(db, make_user): - # A legit account whose email IS verified (e.g. a no-SMTP registration) keeps its password on - # approval — the guard must not break the normal flow. + # The no-SMTP shape: registration marks such an account verified up front, so it was never in + # scope for the SEC-10 guard even before E9.5 S1. Kept as the other half of the pair. from app.routes.admin import _activate_user_by_email user = make_user( @@ -212,7 +257,7 @@ def test_registration_with_an_already_pending_request_persists_the_account( def test_an_admin_can_activate_a_pre_approved_registrant(db, monkeypatch): # The recovery path for the account above, pinned so it cannot quietly break: re-entering the # address in the Access-requests whitelist box (POST /api/admin/invites) runs - # _activate_user_by_email, which is also what keeps the SEC-10 preset-password guard in play. + # _activate_user_by_email. from app.models import Invite from app.routes.admin import _activate_user_by_email @@ -225,7 +270,7 @@ def test_an_admin_can_activate_a_pre_approved_registrant(db, monkeypatch): db.expire_all() assert user.is_active is True - assert user.password_hash is not None # a verified account keeps its own password + assert user.password_hash is not None # approval never touches the password (E9.5 S1) def test_registration_of_an_unknown_email_still_persists_account_and_pending_invite( @@ -346,11 +391,24 @@ def test_register_route_is_refused_when_registration_is_closed(client, db, monke # --- SEC-10 lockout: a revoked preset password must stay RECOVERABLE --------------------------- # # Regression pack for the prod incident of 2026-09-02. An account registered with a password, then -# approved by an admin BEFORE its email was verified, ended up with password_hash = NULL. That is -# also what a Google-only account looks like, and reset eligibility keyed off `password_hash` alone — -# so the account could neither sign in (no password) nor obtain a reset link, and every response on -# the way was a deliberately uniform "ok"/401. `password_revoked_at` is the trace that tells the two -# apart; these pin each step of the way back in. +# stripped of it by the SEC-10 guard, ended up with password_hash = NULL. That is also what a +# Google-only account looks like, and reset eligibility keyed off `password_hash` alone — so the +# account could neither sign in (no password) nor obtain a reset link, and every response on the way +# was a deliberately uniform "ok"/401. `password_revoked_at` is the trace that tells the two apart; +# these pin each step of the way back in. +# +# E9.5 S1 removed the admin-approval caller, so Google adoption is the only path that still produces +# this state — plus every account stripped on prod before that change, which is why the whole recovery +# route below must keep working. These tests manufacture the state through the surviving producer. + + +def _revoke_as_google_adoption(db, user) -> None: + """Put an account into the SEC-10-revoked state the way the ONLY remaining producer does: the + Google callback adopting an account whose email was never verified.""" + from app.routes.auth import revoke_preset_password_if_unverified + + revoke_preset_password_if_unverified(user) + db.commit() def _capture_reset_links(monkeypatch) -> list[str]: @@ -362,9 +420,9 @@ def _capture_reset_links(monkeypatch) -> list[str]: return sent -def test_the_approval_revoke_stamps_password_revoked_at(db, make_user): - from app.routes.admin import _activate_user_by_email - +def test_the_google_adoption_revoke_stamps_password_revoked_at(db, make_user): + # The guard still fires where it earns its keep: the real owner has just proven ownership through + # Google, so a password set on the address while it was unverified must not survive. victim = make_user( email="stamped@example.com", password="attacker-chosen", @@ -372,27 +430,28 @@ def test_the_approval_revoke_stamps_password_revoked_at(db, make_user): is_active=False, ) assert victim.password_revoked_at is None + epoch0 = victim.session_epoch or 0 - _activate_user_by_email(db, "stamped@example.com") + _revoke_as_google_adoption(db, victim) assert victim.password_hash is None assert victim.password_revoked_at is not None # …so the reset flow can tell it HAD a password + assert (victim.session_epoch or 0) == epoch0 + 1 # any session that password minted is killed def test_an_account_whose_password_was_revoked_can_still_get_a_reset_link( db, make_user, monkeypatch ): # THE lockout bug: without the stamp this sent nothing, and the account was unrecoverable. - from app.routes.admin import _activate_user_by_email from app.routes.auth import _send_reset_if_eligible - make_user( + victim = make_user( email="lockedout@example.com", password="chosen at registration", email_verified=False, is_active=False, ) - _activate_user_by_email(db, "lockedout@example.com") + _revoke_as_google_adoption(db, victim) sent = _capture_reset_links(monkeypatch) _send_reset_if_eligible("lockedout@example.com") @@ -414,16 +473,15 @@ def test_a_google_only_account_is_still_not_reset_eligible(db, make_user, monkey def test_completing_a_reset_clears_the_revocation_stamp(client, db, make_user): - from app.routes.admin import _activate_user_by_email from app.routes.auth import _issue_token user = make_user( email="recovered@example.com", password="chosen at registration", email_verified=False, - is_active=False, + is_active=True, # already approved; reset-confirm verifies the email but never activates ) - _activate_user_by_email(db, "recovered@example.com") + _revoke_as_google_adoption(db, user) raw = _issue_token(db, user, "reset", timedelta(hours=1)) db.commit() @@ -442,7 +500,6 @@ def test_verify_tells_a_revoked_account_that_it_needs_a_new_password(client, db, # The banner's input. Saying so leaks nothing: the caller just proved mailbox control by holding a # valid single-use verify token. Without it the user meets only the uniform "Invalid email or # password" and has no way to learn why the password they chose stopped working. - from app.routes.admin import _activate_user_by_email from app.routes.auth import _issue_token user = make_user( @@ -451,7 +508,7 @@ def test_verify_tells_a_revoked_account_that_it_needs_a_new_password(client, db, email_verified=False, is_active=False, ) - _activate_user_by_email(db, "needspw@example.com") + _revoke_as_google_adoption(db, user) raw = _issue_token(db, user, "verify", timedelta(hours=1)) db.commit() diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx index 563181db..a016d015 100644 --- a/frontend/src/components/Welcome.tsx +++ b/frontend/src/components/Welcome.tsx @@ -339,9 +339,10 @@ function AuthCard() { verifyFired.current = true; api .verifyEmail(verifyToken) - // "ok_reset": verified, but SEC-10 had dropped the password this account registered with (an - // admin approved it before it was verified). Sign-in would answer with the uniform credentials - // error, so point the user at the reset flow instead. Same value the legacy ?verify= redirect uses. + // "ok_reset": verified, but SEC-10 had dropped the password this account registered with. Sign-in + // would answer with the uniform credentials error, so point the user at the reset flow instead. + // Same value the legacy ?verify= redirect uses. Rare since E9.5 S1 (admin approval no longer + // revokes) — it still covers accounts stripped before that and the Google-adoption path. .then((r) => setVerify(r.password_reset_required ? "ok_reset" : "ok")) .catch(() => setVerify("invalid")); }, [verifyToken]); diff --git a/frontend/src/lib/api/auth.ts b/frontend/src/lib/api/auth.ts index 66cd7416..2f8ec006 100644 --- a/frontend/src/lib/api/auth.ts +++ b/frontend/src/lib/api/auth.ts @@ -37,9 +37,10 @@ export const authApi = { { quiet: true }, ), // Confirm an email-verification token the SPA read from the URL fragment (SB3). - // `password_reset_required` = SEC-10 dropped the password this account registered with (an admin - // approved it before it was verified), so signing in would fail with the uniform credentials error. - // The card sends the user to the reset flow instead of leaving them to guess. + // `password_reset_required` = SEC-10 dropped the password this account registered with, so signing + // in would fail with the uniform credentials error. The card sends the user to the reset flow instead + // of leaving them to guess. Rare since E9.5 S1: only accounts stripped before that change, and the + // Google-adoption path, still reach it. verifyEmail: (token: string): Promise<{ ok: boolean; password_reset_required?: boolean }> => req("/auth/verify", { method: "POST", body: JSON.stringify({ token }) }, { quiet: true }), // Re-issue a verification link (the first one expired, or was lost). Uniform response — never From c134b514c1cd6219d3098ff1a983d1a4ce7c9c32 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 3 Sep 2026 23:45:31 +0200 Subject: [PATCH 02/16] fix(auth): confirm an emailed verification link wherever it is opened (E9.5 S2) The token was only ever consumed inside Welcome.tsx, which App renders ONLY when there is no session. Opening the emailed link in a browser that already held a Siftlode session posted nothing, the fragment was then stripped, and the user got no signal at all -- the link simply did nothing. Incognito worked, which is why it survived. Confirmation now happens in App, above that branch: signed out the outcome still drives the auth card's banners, signed in it drives a top-of-app bar. The entry URL is snapshotted once at import time (lib/entryUrl) instead of per component, so no future URL writer can strip a token before whoever needs it has read it -- that ordering race is what broke this in the first place. POST /auth/verify now names the address it confirmed, which matters when the browser holds a session for a different account; the same reasoning as password_reset_required already covers the disclosure. Also here, from the 2026-09-02 review tail: the dead GET /auth/verify goes (both senders emit the fragment form, and the last query-token link expired 24h after the 0.78.7 deploy), a spent #verify=/#reset= fragment is cleared while signed in, and the auth-card banners get role=status. The verifyOkReset copy no longer blames admin approval, which E9.5 S1 made false. --- backend/app/routes/auth.py | 21 ++--- backend/tests/test_auth_session.py | 18 +++- frontend/src/App.tsx | 10 ++- frontend/src/components/Banner.tsx | 10 ++- frontend/src/components/VerifyBanner.tsx | 41 +++++++++ frontend/src/components/Welcome.tsx | 76 ++++++---------- .../welcomeMode.render.dom.test.tsx | 47 +++++++++- frontend/src/i18n/locales/en/header.json | 6 ++ frontend/src/i18n/locales/en/welcome.json | 2 +- frontend/src/i18n/locales/hu/header.json | 6 ++ frontend/src/i18n/locales/hu/welcome.json | 2 +- frontend/src/lib/api/auth.ts | 6 +- frontend/src/lib/entryUrl.ts | 26 ++++++ frontend/src/lib/useVerifyEntry.ts | 52 +++++++++++ frontend/src/lib/verifyEntry.dom.test.tsx | 87 +++++++++++++++++++ 15 files changed, 337 insertions(+), 73 deletions(-) create mode 100644 frontend/src/components/VerifyBanner.tsx create mode 100644 frontend/src/lib/entryUrl.ts create mode 100644 frontend/src/lib/useVerifyEntry.ts create mode 100644 frontend/src/lib/verifyEntry.dom.test.tsx diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index c6ff8a91..f60b469c 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -678,8 +678,13 @@ def verify_email_confirm(body: TokenIn, db: Session = Depends(get_db)) -> dict: ) user.email_verified = True needs_password = user.password_hash is None and user.password_revoked_at is not None + email = user.email db.commit() - return {"ok": True, "password_reset_required": needs_password} + # Naming the address lets the SPA say WHICH mailbox it just confirmed — the same reasoning as + # password_reset_required above: holding a valid single-use verify token for this account already + # proves that much, so echoing it back leaks nothing. It matters in a browser that holds a session + # for a different account, where an unqualified "email confirmed" would be actively misleading. + return {"ok": True, "password_reset_required": needs_password, "email": email} @router.post("/verify/resend") @@ -715,20 +720,6 @@ def _resend_verify_if_eligible(email: str) -> None: email_mod.send_verify_email(email, f"{_app_base()}/#verify={raw}") -@router.get("/verify") -def verify_email(token: str, db: Session = Depends(get_db)): - """Legacy query-token verification link (pre-SB3 emails still in flight). New emails use the - fragment + POST flow above; this bounces to a friendly status for older links until they expire.""" - user = _consume_token(db, token, "verify") - if user is None: - return RedirectResponse(url="/?verify=invalid") - user.email_verified = True - needs_password = user.password_hash is None and user.password_revoked_at is not None - db.commit() - # Same two outcomes as the POST flow above, so an old link in flight lands on the same guidance. - return RedirectResponse(url="/?verify=" + ("ok_reset" if needs_password else "ok")) - - @router.post("/password-login") def password_login( body: EmailPasswordIn, diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 35ef8143..b3a3fcd3 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -157,7 +157,11 @@ def test_verifying_afterwards_lets_the_original_password_in(client, db, make_use verified = client.post("/auth/verify", json={"token": raw}) assert verified.status_code == 200 - assert verified.json() == {"ok": True, "password_reset_required": False} + assert verified.json() == { + "ok": True, + "password_reset_required": False, + "email": "ordering@example.com", + } assert _login(client, "ordering@example.com", "chosen at registration").status_code == 200 @@ -515,7 +519,11 @@ def test_verify_tells_a_revoked_account_that_it_needs_a_new_password(client, db, r = client.post("/auth/verify", json={"token": raw}) assert r.status_code == 200 - assert r.json() == {"ok": True, "password_reset_required": True} + assert r.json() == { + "ok": True, + "password_reset_required": True, + "email": "needspw@example.com", + } def test_verify_reports_no_reset_needed_for_an_intact_account(client, db, make_user): @@ -528,4 +536,8 @@ def test_verify_reports_no_reset_needed_for_an_intact_account(client, db, make_u r = client.post("/auth/verify", json={"token": raw}) assert r.status_code == 200 - assert r.json() == {"ok": True, "password_reset_required": False} + assert r.json() == { + "ok": True, + "password_reset_required": False, + "email": "intact@example.com", + } diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 9ec6a00d..5d4975d2 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -17,6 +17,7 @@ import { import { notify, removeByMetaKind } from "./lib/notifications"; import { LS, setAccountRaw } from "./lib/storage"; import { usePref } from "./lib/usePref"; +import { useVerifyEntry } from "./lib/useVerifyEntry"; import { useNavigation, useNavigationActions } from "./components/NavigationProvider"; import { useFeedFilters, useFeedFiltersActions } from "./components/FeedFiltersProvider"; import { useChannelLayout } from "./components/ChannelsProvider"; @@ -37,6 +38,7 @@ import { StateMessage } from "./components/QueryState"; import ErrorDialog from "./components/ErrorDialog"; import VersionBanner from "./components/VersionBanner"; import DemoBanner from "./components/DemoBanner"; +import VerifyBanner from "./components/VerifyBanner"; import { focusAccessRequestsTab } from "./lib/adminUsersTab"; // The built app version, NOT imported from releaseNotes — importing anything from that module // pulls its whole ~900-line changelog into the eager App chunk and defeats the lazy ReleaseNotes @@ -54,6 +56,11 @@ export default function App() { const { t, i18n } = useTranslation(); const { page, channelView, ytSearch } = useNavigation(); const { setPage, enterYtSearch } = useNavigationActions(); + // An emailed verification link is confirmed HERE, above the signed-in/signed-out branch below, so + // it works in a browser that already holds a session. It used to be handled inside Welcome, which + // that branch never renders when signed in — the token was silently dropped. Signed out, the + // outcome goes to the auth card's banners; signed in, to the top-of-app bar. + const verifyEntry = useVerifyEntry(); // The global feed filters live in FeedFiltersProvider (per-account persistence + the account-load // effect + share-link capture all moved there). App reads them to hand to and to the // handlers that mutate the feed view (onShowInFeed, tag-click, the feed scrollKey). @@ -288,7 +295,7 @@ export default function App() { ); - if (!meQuery.data) return ; + if (!meQuery.data) return ; // Shown briefly while a lazy page/module chunk loads (first visit to that page only). const pageFallback = ( @@ -386,6 +393,7 @@ export default function App() { header={
} banners={ <> + {meQuery.data!.is_demo && } openReleaseNotes(FRONTEND_VERSION)} /> diff --git a/frontend/src/components/Banner.tsx b/frontend/src/components/Banner.tsx index ecdc536d..e175ab9b 100644 --- a/frontend/src/components/Banner.tsx +++ b/frontend/src/components/Banner.tsx @@ -26,6 +26,7 @@ export default function Banner({ action, onDismiss, dismissTitle, + role, }: { tone: BannerTone; icon: LucideIcon; @@ -33,10 +34,17 @@ export default function Banner({ action?: { label: string; onClick: () => void }; onDismiss?: () => void; dismissTitle?: string; + /** `"status"` for a bar that APPEARS in response to something the user just did, so a screen + * reader announces it. Left unset for the permanent bars (demo, version): those are part of the + * page when it loads, and announcing them on every visit is noise, not information. */ + role?: "status"; }) { const c = TONES[tone]; return ( -
+
{children} {action && ( diff --git a/frontend/src/components/VerifyBanner.tsx b/frontend/src/components/VerifyBanner.tsx new file mode 100644 index 00000000..0d99bab2 --- /dev/null +++ b/frontend/src/components/VerifyBanner.tsx @@ -0,0 +1,41 @@ +import { useTranslation } from "react-i18next"; +import { CheckCircle2, AlertTriangle } from "lucide-react"; +import Banner from "./Banner"; +import type { VerifyEntry } from "../lib/useVerifyEntry"; + +// The top-of-app outcome of an emailed verification link opened in a browser that ALREADY holds a +// session. That case used to be silent: the token was only ever consumed inside Welcome.tsx, which a +// signed-in visit never reaches, so the link opened a tab, the fragment was stripped, and nothing +// happened. Signed OUT, the same outcome is rendered by the auth card's own banners instead — this +// bar is only for the signed-in half. +// +// No "send a new link" action here, unlike the auth card: this viewer is signed in, so the sign-in +// screen (where resending lives) is not where they are, and the message is a report, not a task. A +// wrong-account case is why the address is named. +export default function VerifyBanner({ entry }: { entry: VerifyEntry }) { + const { t } = useTranslation(); + if (!entry.outcome) return null; + + const ok = entry.outcome !== "invalid"; + const text = + entry.outcome === "invalid" + ? t("header.verify.invalid") + : entry.email + ? t("header.verify.confirmedNamed", { email: entry.email }) + : t("header.verify.confirmed"); + + return ( + + {text} + {entry.outcome === "ok_reset" && ( + {t("header.verify.reset")} + )} + + ); +} diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx index a016d015..bc20ef52 100644 --- a/frontend/src/components/Welcome.tsx +++ b/frontend/src/components/Welcome.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useState } from "react"; import { useHistorySubview, type SubviewUrl } from "../lib/history"; import { useTranslation } from "react-i18next"; import { @@ -24,6 +24,8 @@ import { HttpError } from "../lib/api"; import { setLanguage, type LangCode } from "../i18n"; import LanguageSwitcher from "./LanguageSwitcher"; import { inputCls } from "./ui/form"; +import { entryHash, entrySearch } from "../lib/entryUrl"; +import type { VerifyEntry } from "../lib/useVerifyEntry"; const EMAIL_RE = /^[^@\s]+@[^@\s]+\.[^@\s]+$/; const PASSWORD_MIN = 10; @@ -63,7 +65,7 @@ const SUBMIT_KEY: Record = { // password, Google, explicit demo) over a short marketing pitch with feature cards. Shown // whenever the user isn't signed in (see App.tsx). The auth forms talk to /auth/* (see the // backend); errors are surfaced inline (the api calls use the quiet flag). -export default function Welcome() { +export default function Welcome({ verify }: { verify: VerifyEntry }) { const { t, i18n } = useTranslation(); // Which screenshot (if any) is open in the full-size lightbox. const [zoomed, setZoomed] = useState<{ src: string; label: string } | null>(null); @@ -87,7 +89,7 @@ export default function Welcome() {

{t("welcome.hero.subtitle")}

{t("welcome.hero.trust")}

- + {/* App preview */} @@ -301,51 +303,24 @@ function Lightbox({ src, label, onClose }: { src: string; label: string; onClose ); } -function AuthCard() { +function AuthCard({ verify }: { verify: VerifyEntry }) { const { t } = useTranslation(); - // Snapshot the pre-login params ONCE (query + fragment) so we can clean the address bar below - // without dismissing the banners/reset-mode they drive. Secret tokens (reset, verify) ride the - // URL FRAGMENT so they never reach the server / proxy logs / Referer (SB3); plain status flags - // stay in the query string. - const [params] = useState(() => new URLSearchParams(window.location.search)); - const [hashParams] = useState(() => new URLSearchParams(window.location.hash.replace(/^#/, ""))); + // The entry URL is snapshotted once at import time (lib/entryUrl) rather than here, so nothing can + // rewrite the address bar out from under this card before it mounts — and so the App-level hook and + // this card read the SAME values regardless of which mounts first. Clearing the address bar is that + // hook's job now, since it is the one thing that runs on every visit, signed in or not. + // // State, not a const: once the token has been spent the reset view must stop being the card's // root, or a Back from the "password changed" screen lands on a form whose token is already used. - const [resetToken, setResetToken] = useState(() => hashParams.get("reset")); - const verifyToken = hashParams.get("verify"); // raw token → POST to confirm (new flow) - const bounced = params.get("access"); // Google denied → "requested" | "denied" - const deleted = params.get("deleted"); // self-service account deletion just completed + const [resetToken, setResetToken] = useState(() => entryHash.get("reset")); + const bounced = entrySearch.get("access"); // Google denied → "requested" | "denied" + const deleted = entrySearch.get("deleted"); // self-service account deletion just completed // Google login outcome → "suspended" | "oauth_cancelled". State (not a const) so it clears on the // first deliberate action, like the verify banner — otherwise it lingers stalely on later screens. - const [login, setLogin] = useState(() => params.get("login")); - // Verification outcome: from POSTing the fragment token (new flow), or the legacy - // ?verify=ok|invalid redirect (pre-SB3 emails still in flight). - const [verify, setVerify] = useState(() => params.get("verify")); - - // Strip BOTH the query string and the fragment so the address bar stays clean and the token - // doesn't linger in history. - useEffect(() => { - if (window.location.search || window.location.hash) { - window.history.replaceState(null, "", window.location.pathname); - } - }, []); - - // Confirm a fragment verification token by POSTing it (new SB3 flow). Fire ONCE: the token is - // single-use, so a double POST (StrictMode remount / any re-render) would 400 on the second call - // and wrongly flip the banner to "invalid". - const verifyFired = useRef(false); - useEffect(() => { - if (!verifyToken || verifyFired.current) return; - verifyFired.current = true; - api - .verifyEmail(verifyToken) - // "ok_reset": verified, but SEC-10 had dropped the password this account registered with. Sign-in - // would answer with the uniform credentials error, so point the user at the reset flow instead. - // Same value the legacy ?verify= redirect uses. Rare since E9.5 S1 (admin approval no longer - // revokes) — it still covers accounts stripped before that and the Google-adoption path. - .then((r) => setVerify(r.password_reset_required ? "ok_reset" : "ok")) - .catch(() => setVerify("invalid")); - }, [verifyToken]); + const [login, setLogin] = useState(() => entrySearch.get("login")); + // The verification outcome is confirmed in App (above the signed-in branch) and handed down, so + // one emailed link behaves the same whether or not this browser already holds a session. + const verifyOutcome = verify.outcome; const { view: mode, open: setMode } = useHistorySubview( resetToken ? "reset" : "signin", @@ -386,7 +361,7 @@ function AuthCard() { setDone(""); // The verify/login banners are one-time entry notices — dismiss once the user takes any // deliberate action (mode switch, CTA, submit) so they don't linger stalely on later screens. - setVerify(null); + verify.dismiss(); setLogin(null); }; @@ -475,8 +450,8 @@ function AuthCard() {

{t(titleKey)}

{/* One-time status banners from a redirect (Google-denied, email verification). */} - {verify === "ok" && {t("welcome.auth.verifyOk")}} - {verify === "ok_reset" && ( + {verifyOutcome === "ok" && {t("welcome.auth.verifyOk")}} + {verifyOutcome === "ok_reset" && ( {t("welcome.auth.verifyOkReset")}{" "} )} - {verify === "invalid" && ( + {verifyOutcome === "invalid" && ( {t("welcome.auth.verifyInvalid")}{" "}
{me.role === "admin" && ( diff --git a/frontend/src/components/SettingsPanel.tsx b/frontend/src/components/SettingsPanel.tsx index c11d7cf4..8645b5cb 100644 --- a/frontend/src/components/SettingsPanel.tsx +++ b/frontend/src/components/SettingsPanel.tsx @@ -413,6 +413,15 @@ function SignInMethods({ me }: { me: Me }) { ? t("settings.account.googleLink.connectedHint") : t("settings.account.googleLink.connectHint")}

+ {/* Which address Google actually answers with — sent only when it differs from the + account's own (see /api/me). "Connected" alone left that unanswerable, and it is + exactly what someone who registered with a `+tag` needs to know. */} + {me.google_email && ( +

+ {t("settings.account.googleLink.signedInAs")}{" "} + {me.google_email} +

+ )} {me.has_google ? ( diff --git a/frontend/src/i18n/locales/en/header.json b/frontend/src/i18n/locales/en/header.json index 31044b73..b5f7eb63 100644 --- a/frontend/src/i18n/locales/en/header.json +++ b/frontend/src/i18n/locales/en/header.json @@ -34,7 +34,8 @@ "signOut": "Sign out", "addAccount": "Add another account", "switchTo": "Switch to {{name}}", - "audit": "Audit log" + "audit": "Audit log", + "googleAddress": "Google account: {{email}}" }, "sync": { "yours": "yours", diff --git a/frontend/src/i18n/locales/en/settings.json b/frontend/src/i18n/locales/en/settings.json index 96d12b5d..8bfc564e 100644 --- a/frontend/src/i18n/locales/en/settings.json +++ b/frontend/src/i18n/locales/en/settings.json @@ -78,7 +78,8 @@ "linked": "Google account linked.", "conflict": "That Google account is already linked to a different Siftlode account.", "mismatch": "That's a different Google account than the one already linked here. Sign in with the linked one.", - "error": "Couldn't link the Google account. Please try again." + "error": "Couldn't link the Google account. Please try again.", + "signedInAs": "Signed in with Google as" }, "googleCancelled": "Google sign-in was cancelled.", "password": { diff --git a/frontend/src/i18n/locales/hu/header.json b/frontend/src/i18n/locales/hu/header.json index 05bb5db1..cb3b9333 100644 --- a/frontend/src/i18n/locales/hu/header.json +++ b/frontend/src/i18n/locales/hu/header.json @@ -34,7 +34,8 @@ "signOut": "Kijelentkezés", "addAccount": "Másik fiók hozzáadása", "switchTo": "Váltás erre: {{name}}", - "audit": "Naplózás" + "audit": "Naplózás", + "googleAddress": "Google-fiók: {{email}}" }, "sync": { "yours": "tiéd", diff --git a/frontend/src/i18n/locales/hu/settings.json b/frontend/src/i18n/locales/hu/settings.json index 999d59e0..72adf40e 100644 --- a/frontend/src/i18n/locales/hu/settings.json +++ b/frontend/src/i18n/locales/hu/settings.json @@ -78,7 +78,8 @@ "linked": "Google-fiók összekapcsolva.", "conflict": "Ez a Google-fiók már egy másik Siftlode-fiókhoz van kapcsolva.", "mismatch": "Ez nem az a Google-fiók, amely ide már össze van kapcsolva. Az összekapcsolttal jelentkezz be.", - "error": "Nem sikerült összekapcsolni a Google-fiókot. Próbáld újra." + "error": "Nem sikerült összekapcsolni a Google-fiókot. Próbáld újra.", + "signedInAs": "Google-belépés ezzel a címmel:" }, "googleCancelled": "A Google-belépés megszakadt.", "password": { diff --git a/frontend/src/lib/api/account.ts b/frontend/src/lib/api/account.ts index 18f6cfa2..865d62eb 100644 --- a/frontend/src/lib/api/account.ts +++ b/frontend/src/lib/api/account.ts @@ -10,6 +10,10 @@ export interface Me { role: string; is_demo: boolean; has_google: boolean; + /** The address Google answers with, ONLY when it differs from `email` — the backend does that + * comparison, so a surface can render this whenever it is present. Someone who registered as + * `me+siftlode@gmail.com` keeps that address; this is the other one that also signs them in. */ + google_email: string | null; has_password: boolean; google_enabled: boolean; plex_enabled: boolean; From ed91c83dd7d549bea8053b191daf506dc71d0a0c Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 4 Sep 2026 01:56:55 +0200 Subject: [PATCH 07/16] fix(auth): name the state and the address behind a refused sign-in (E9.5 S4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Your account is awaiting admin approval." was true and useless. It never said WHICH mailbox the account was on, and on 2026-09-03 that silence hid a Gmail alias mismatch for a day: a registration as np.temu@gmail.com met a Google claim of nptemu@gmail.com, and every screen in the funnel agreed to say nothing about it. Every refusal password-login can answer with now uses the codebase's structured {code, reason} detail, built in one helper. The three that arrive AFTER the password is proven carry the address on the account plus both gate flags; the two that answer before it (invalid, rate_limited) carry nothing, so a wrong password and an unknown account stay byte-identical. The frontend renders the first three as a named screen in place of the form — there is nothing to retype once the password is correct — with an icon, the two-gate checklist, the address in monospace, and a resend CTA where one helps. Reporting each gate from its own flag is the payoff of S1: approved-but-unverified is a real state now, and a screen that assumed a sequence would send that user off to wait for an admin who has already decided. The Google access-request notice names the address too, and says so when the request was already on file under ANOTHER spelling of the same mailbox — the honest version of "we didn't open a second one", stated only when it applies. It moved from the query string to the fragment: the notice now carries an address, and a query string lands in every reverse-proxy access log. Same reasoning that put the verify/reset tokens there, and the same owner already clears it. urlencode, not an f-string, because URLSearchParams reads a bare + in a fragment as a space and would mangle exactly the +tag addresses this epic exists to protect. upsert_pending_invite returns the row it settled on beside the is-new flag, since the row's stored address is what the notice has to name and what mail will go to. Deviation from the PendingScreen mockup, deliberately: no "Sign out" button and no separate "Not your address?" link. The mockup drew the panel in the app shell; the accepted decision (a) keeps the pending user session-less, so there is no session to end, and the address is named directly above a "Back to sign in" that answers the same question. UAT on localdev, both languages: all three refusals with the typed spelling deliberately differing from the stored one (the panel named the stored one), the resend CTA carrying the typed address into the resend form, and both access-request banners with a +tag address surviving the fragment round trip. Throwaway accounts and the language switch reverted. --- backend/app/routes/auth.py | 113 ++++++++--- backend/tests/test_auth_session.py | 2 +- backend/tests/test_email_canonical.py | 5 +- backend/tests/test_login_states.py | 107 +++++++++++ frontend/src/components/Welcome.tsx | 165 +++++++++++++++- .../components/authCardStates.dom.test.tsx | 177 ++++++++++++++++++ frontend/src/i18n/locales/en/welcome.json | 35 ++++ frontend/src/i18n/locales/hu/welcome.json | 35 ++++ frontend/src/lib/api.ts | 1 + frontend/src/lib/api/auth.ts | 42 +++++ frontend/src/lib/api/core.ts | 6 + frontend/src/lib/entryUrl.ts | 4 +- 12 files changed, 656 insertions(+), 36 deletions(-) create mode 100644 backend/tests/test_login_states.py create mode 100644 frontend/src/components/authCardStates.dom.test.tsx diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 214813ff..c98d434d 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -6,6 +6,8 @@ helpers) stays in app/auth.py, imported below.""" import ipaddress import secrets from datetime import datetime, timedelta, timezone +from typing import NamedTuple +from urllib.parse import urlencode from authlib.integrations.starlette_client import OAuth, OAuthError from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request @@ -148,9 +150,22 @@ def _client_ip(request: Request) -> str: return peer -def upsert_pending_invite(db: Session, email: str) -> Invite | None: - """Idempotently record an access request. Returns the Invite if a *new* pending row - was created (so the caller can notify admins), else None for already-known emails. +class InviteRequest(NamedTuple): + """What an access request settled on. + + `invite` is the row the request now lives under — which is NOT always the address that was just + typed: one Gmail mailbox spelled two ways settles on whichever row already exists, and that row's + address is where mail will go. Callers that tell the user what happened need it (E9.5 S4). + `is_new` is the old boolean return in a name: true only when a row was created or a denied one + reopened, i.e. exactly when admins should hear about it. A repeat request must not re-spam them. + """ + + invite: Invite | None + is_new: bool + + +def upsert_pending_invite(db: Session, email: str) -> InviteRequest: + """Idempotently record an access request. "Already known" is decided on the CANONICAL form: `npeter83.temu@` and `npeter83temu@` are one Gmail mailbox, and treating them as two is what put a second, silent access request in the admin @@ -160,7 +175,7 @@ def upsert_pending_invite(db: Session, email: str) -> Invite | None: """ email = (email or "").lower() if not valid_email(email): - return None + return InviteRequest(None, False) 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. @@ -173,12 +188,37 @@ def upsert_pending_invite(db: Session, email: str) -> Invite | None: inv.decided_at = None inv.decided_by = None db.commit() - return inv - return None + return InviteRequest(inv, True) + return InviteRequest(inv, False) inv = Invite(email=email, status="pending") db.add(inv) db.commit() - return inv + return InviteRequest(inv, True) + + +def _access_notice_url(kind: str, email: str = "", inv: Invite | None = None) -> str: + """The landing URL for a Google sign-in that did not reach an account: `requested` (we recorded + the request) or `denied` (nothing to record). + + It rides the FRAGMENT, not the query string, for the same reason the verify/reset tokens do + (SB3): a fragment never reaches the server, a proxy access log or a Referer header — and this + notice now carries the user's address. Naming it is the point of E9.5 S4: on 2026-09-03 a + registration as `npeter83.temu@` met an OAuth claim of `npeter83temu@`, and a screen that said + "request recorded" without saying FOR WHAT hid the mismatch for a day. + + `linked` names the address the request actually lives under, and ONLY when that differs from the + one Google just returned — the honest version of "we didn't open a second request". Same + spelling twice is just a repeat, and saying anything there would be noise. + + urlencode (not an f-string): a `+tag` address must arrive as %2B, or the SPA's URLSearchParams + would read it back as a space and mangle exactly the address this epic exists to protect. + """ + params = {"access": kind} + if email: + params["email"] = email + if inv is not None and inv.email != email: + params["linked"] = inv.email + return "/#" + urlencode(params) _DEV_OAUTH_HOSTS = frozenset( @@ -294,12 +334,12 @@ async def callback( # A denied Google login doubles as an access request: record it for the admin and # land the user on a friendly "request received" screen instead of a raw 403. if email: - inv = upsert_pending_invite(db, email) + req = upsert_pending_invite(db, email) admins = admin_notify_emails(db) - if inv is not None and admins: + if req.is_new and admins: background.add_task(email_mod.send_admin_new_request, admins, email) - return RedirectResponse(url="/?access=requested") - return RedirectResponse(url="/?access=denied") + return RedirectResponse(url=_access_notice_url("requested", email, req.invite)) + return RedirectResponse(url=_access_notice_url("denied")) user = db.query(User).filter(User.google_sub == sub).one_or_none() if user is None: @@ -323,7 +363,7 @@ async def callback( # A different Google identity is already bound to this email — refuse rather than # hijack it. (Near-impossible with real Google accounts: email↔sub is stable.) log.warning("Google login email collision (different sub): %s", email) - return RedirectResponse(url="/?access=denied") + return RedirectResponse(url=_access_notice_url("denied")) if not userinfo.get("email_verified", False): # Adopting (and activating) a pre-existing password account requires Google to # actually attest the address — otherwise an unverified Google email that merely @@ -332,7 +372,7 @@ async def callback( "Google login: unverified email, refusing to adopt account: %s", email, ) - return RedirectResponse(url="/?access=denied") + return RedirectResponse(url=_access_notice_url("denied")) user = existing user.google_sub = sub # SEC-10: drop any password pre-set on this account while its email was still unverified, @@ -538,9 +578,9 @@ async def request_access( return {"status": "pending"} if is_allowed(db, email): return {"status": "pending"} # already allowed — don't reveal it - inv = upsert_pending_invite(db, email) + req = upsert_pending_invite(db, email) admins = admin_notify_emails(db) - if inv is not None and admins: + if req.is_new and admins: background.add_task(email_mod.send_admin_new_request, admins, email) return {"status": "pending"} @@ -776,6 +816,33 @@ def _resend_verify_if_eligible(email: str) -> None: email_mod.send_verify_email(user.email, f"{_app_base()}/#verify={raw}") +def _login_refused(status: int, reason: str, user: User | None = None, **extra) -> HTTPException: + """Every refusal password_login can answer with, in the codebase's structured `{code, reason}` + shape so the SPA can say it in the user's own language (the old bare English strings were + unreadable under a bilingual UI, and unusable as anything but a one-line error). + + The account-shaped reasons carry the address ON THE ACCOUNT plus both gate flags. That is what + turns a flat 403 into an honest screen — it names the mailbox and says which of the two gates is + still shut. It is not an enumeration leak: this branch is only reachable once the password has + been proven, so the caller already knows the account exists. The two reasons that answer BEFORE + that proof (`invalid`, `rate_limited`) carry nothing at all, so they stay byte-identical for a + real account and an unknown one. + + Since E9.5 S1 the two gates are order-independent, so `approved` really can be true while + `email_verified` is false, and vice versa — the SPA renders whichever is actually outstanding + rather than assuming a sequence. + """ + detail: dict = {"code": "login", "reason": reason} + if user is not None: + detail |= { + "email": user.email, + "email_verified": user.email_verified, + "approved": user.is_active, + } + detail |= {k: v for k, v in extra.items() if v is not None} + return HTTPException(status_code=status, detail=detail) + + @router.post("/password-login") def password_login( body: EmailPasswordIn, @@ -787,7 +854,7 @@ def password_login( Once the password is proven correct, the owner gets a specific reason if their account is still pending or suspended — that's not an enumeration leak (they already hold the password).""" if not _login_limiter.allow(_client_ip(request)): - raise HTTPException(status_code=429, detail="Too many attempts. Try again shortly.") + raise _login_refused(429, "rate_limited") email = (body.email or "").strip().lower() password = body.password or "" user = find_user_by_email(db, email) @@ -796,20 +863,14 @@ def password_login( candidate_hash = user.password_hash if (user and not user.is_demo) else None password_ok = verify_password(password, candidate_hash or _DECOY_PASSWORD_HASH) if user is None or user.is_demo or not password_ok: - raise HTTPException(status_code=401, detail="Invalid email or password.") + raise _login_refused(401, "invalid") if user.is_suspended: _notify_suspended(background, user.email) - op = operator_contact() - detail = "Your account has been suspended." - if op: - detail += f" If you have questions, contact the operator at {op}." - raise HTTPException(status_code=403, detail=detail) + raise _login_refused(403, "suspended", user, operator=operator_contact() or None) if not user.email_verified: - raise HTTPException( - status_code=403, detail="Please verify your email first (check your inbox)." - ) + raise _login_refused(403, "email_unverified", user) if not user.is_active: - raise HTTPException(status_code=403, detail="Your account is awaiting admin approval.") + raise _login_refused(403, "pending_approval", user) _establish_session(request, user) log.info("Password login: %s (id=%s, role=%s)", email, user.id, user.role) return {"ok": True} diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index b3a3fcd3..905709eb 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -134,7 +134,7 @@ def test_an_approved_but_unverified_account_still_cannot_sign_in(client, make_us r = _login(client, "notyet@example.com", "attacker-chosen") assert r.status_code == 403 - assert "verify" in r.json()["detail"].lower() + assert r.json()["detail"]["reason"] == "email_unverified" def test_verifying_afterwards_lets_the_original_password_in(client, db, make_user): diff --git a/backend/tests/test_email_canonical.py b/backend/tests/test_email_canonical.py index 483230a9..f0934f0f 100644 --- a/backend/tests/test_email_canonical.py +++ b/backend/tests/test_email_canonical.py @@ -137,9 +137,10 @@ def test_a_second_spelling_does_not_open_a_second_access_request(db): db.add(Invite(email="already.approved@gmail.com", status="approved")) db.commit() - created = upsert_pending_invite(db, "alreadyapproved@gmail.com") + req = upsert_pending_invite(db, "alreadyapproved@gmail.com") - assert created is None # nothing new to notify an admin about + assert req.is_new is False # nothing new to notify an admin about + assert req.invite is not None and req.invite.email == "already.approved@gmail.com" assert len(db.execute(select(Invite)).scalars().all()) == 1 diff --git a/backend/tests/test_login_states.py b/backend/tests/test_login_states.py new file mode 100644 index 00000000..9b243853 --- /dev/null +++ b/backend/tests/test_login_states.py @@ -0,0 +1,107 @@ +"""What a refused sign-in SAYS (E9.5 S4), DB-lane. + +The states behind a 403 were true and useless: "Your account is awaiting admin approval." never +revealed WHICH mailbox the account was on. On 2026-09-03 a registration as `npeter83.temu@gmail.com` +met a Google claim of `npeter83temu@gmail.com`, and that missing address hid the mismatch for a day. +So these pin the DETAIL, not just the status code — the address, and both gates, since E9.5 S1 made +the two order-independent and the screen must report whichever is actually outstanding. + +The two refusals that answer BEFORE the password is proven carry nothing, and that is the security +half of the same contract: `invalid` must stay byte-identical for a real account and an unknown one. +""" + +from app.config import settings +from app.models import Invite +from app.routes.auth import _access_notice_url, upsert_pending_invite + +_PW = "correct horse battery" + + +def _login(client, email, password=_PW): + return client.post("/auth/password-login", json={"email": email, "password": password}) + + +def test_pending_approval_names_the_address_the_account_is_on(client, make_user): + # The typed spelling and the stored one are the same mailbox; the screen must show the STORED + # one, because that is the mismatch a user cannot otherwise see. + make_user(email="np.temu@gmail.com", password=_PW, is_active=False) + + detail = _login(client, "nptemu@gmail.com").json()["detail"] + + assert detail["reason"] == "pending_approval" + assert detail["email"] == "np.temu@gmail.com" + assert detail["email_verified"] is True and detail["approved"] is False + + +def test_an_already_approved_account_still_reports_its_missing_verification(client, make_user): + # S1 made the gates order-independent, so this really happens: approved, not yet verified. A + # screen that assumed a sequence would tell this user to wait for an admin who already decided. + make_user( + email="approved.first@example.com", password=_PW, email_verified=False, is_active=True + ) + + detail = _login(client, "approved.first@example.com").json()["detail"] + + assert detail["reason"] == "email_unverified" + assert detail["email_verified"] is False and detail["approved"] is True + + +def test_a_suspended_account_is_told_who_to_contact(client, make_user, monkeypatch): + monkeypatch.setattr(settings, "admin_emails", "ops@example.com") + make_user(email="banned@example.com", password=_PW, is_suspended=True) + + r = _login(client, "banned@example.com") + + assert r.status_code == 403 + assert r.json()["detail"]["reason"] == "suspended" + assert r.json()["detail"]["operator"] == "ops@example.com" + + +def test_a_refusal_before_the_password_is_proven_carries_no_account_detail(client, make_user): + make_user(email="real@example.com", password=_PW, is_active=False) + + wrong_pw = _login(client, "real@example.com", "not the password").json()["detail"] + unknown = _login(client, "nobody@example.com").json()["detail"] + + # No address, no flags — the pending state above must not leak through the wrong-password door. + assert wrong_pw == unknown == {"code": "login", "reason": "invalid"} + + +# --- the access-request notice ------------------------------------------------------------------- + + +def test_the_access_notice_names_the_address_and_rides_the_fragment(): + url = _access_notice_url("requested", "someone@example.com") + + # A fragment never reaches a proxy access log or a Referer header — the same reasoning that put + # the verify/reset tokens there (SB3), applied to an address. + assert url == "/#access=requested&email=someone%40example.com" + + +def test_a_plus_tagged_address_survives_the_round_trip(): + # URLSearchParams reads a bare `+` in a fragment as a SPACE, so the encoder must emit %2B — or + # the very address family this epic exists to protect arrives mangled. + url = _access_notice_url("requested", "me+siftlode@gmail.com") + + assert "me%2Bsiftlode%40gmail.com" in url and "+siftlode" not in url + + +def test_a_request_already_on_file_under_another_spelling_says_so(db): + db.add(Invite(email="hartmann.peter@gmail.com", status="pending")) + db.commit() + + req = upsert_pending_invite(db, "hartmannpeter@gmail.com") + url = _access_notice_url("requested", "hartmannpeter@gmail.com", req.invite) + + assert req.is_new is False # one mailbox, one request — no second row for the admin queue + assert "linked=hartmann.peter%40gmail.com" in url + + +def test_the_same_spelling_twice_is_just_a_repeat_and_says_nothing_extra(db): + db.add(Invite(email="same@example.com", status="pending")) + db.commit() + + req = upsert_pending_invite(db, "same@example.com") + + # Nothing to explain: telling this user we "linked" their request to itself would be noise. + assert "linked" not in _access_notice_url("requested", "same@example.com", req.invite) diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx index bc20ef52..a37aecb9 100644 --- a/frontend/src/components/Welcome.tsx +++ b/frontend/src/components/Welcome.tsx @@ -3,7 +3,9 @@ import { useHistorySubview, type SubviewUrl } from "../lib/history"; import { useTranslation } from "react-i18next"; import { ArrowRight, + Check, Clapperboard, + Clock, Code2, Download, Expand, @@ -11,15 +13,17 @@ import { Inbox, ListVideo, Lock, + MailWarning, PlayCircle, Search, + ShieldAlert, X, type LucideIcon, } from "lucide-react"; // Siftlode is open source; the landing footer links to the public repository. const REPO_URL = "https://git.b1fr0st.eu/peter/siftlode"; -import { api } from "../lib/api"; +import { api, loginBlocked, type LoginBlocked } from "../lib/api"; import { HttpError } from "../lib/api"; import { setLanguage, type LangCode } from "../i18n"; import LanguageSwitcher from "./LanguageSwitcher"; @@ -313,7 +317,13 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { // State, not a const: once the token has been spent the reset view must stop being the card's // root, or a Back from the "password changed" screen lands on a form whose token is already used. const [resetToken, setResetToken] = useState(() => entryHash.get("reset")); - const bounced = entrySearch.get("access"); // Google denied → "requested" | "denied" + // The Google access-request notice, off the FRAGMENT (see lib/entryUrl): it names the address now, + // and an address in the query string would land in every reverse-proxy access log. `linked` is set + // only when the request already existed under ANOTHER spelling of the same mailbox — the case worth + // explaining; the same spelling twice is just a repeat. + const bounced = entryHash.get("access"); // Google denied → "requested" | "denied" + const bouncedEmail = entryHash.get("email"); + const bouncedLinked = entryHash.get("linked"); const deleted = entrySearch.get("deleted"); // self-service account deletion just completed // Google login outcome → "suspended" | "oauth_cancelled". State (not a const) so it clears on the // first deliberate action, like the verify banner — otherwise it lingers stalely on later screens. @@ -341,12 +351,17 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { .catch(() => {}); }, []); const [done, setDone] = useState(""); // success/info message replacing the form + // Why password sign-in refused, when the server named a state worth a screen rather than a red line + // (E9.5 S4). It REPLACES the form: in all three of those states the password was already proven + // correct, so there is nothing to retype — leaving the form up would only invite a pointless retry. + const [blocked, setBlocked] = useState(null); // `done` and `error` belong to the view that produced them, and the render is `done ? msg : form` // — without this, Back out of a submitted form would show the old success message under the // previous view's title. Clearing on view change covers both Back/Forward and the mode links. useEffect(() => { setDone(""); setError(""); + setBlocked(null); }, [mode]); // Explicit demo entry (replaces the old hidden probe): a labelled email field, still gated @@ -359,6 +374,7 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { const reset = () => { setError(""); setDone(""); + setBlocked(null); // The verify/login banners are one-time entry notices — dismiss once the user takes any // deliberate action (mode switch, CTA, submit) so they don't linger stalely on later screens. verify.dismiss(); @@ -415,6 +431,13 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { setDone(t("welcome.auth.resetDone")); } } catch (err) { + // An account-shaped refusal becomes a named screen; `invalid` / `rate_limited` stay a line + // under the form, because there the form IS the next step. + const refusal = err instanceof HttpError ? loginBlocked(err.detailData) : null; + if (refusal && PANEL_REASONS.has(refusal.reason)) { + setBlocked(refusal); + return; + } setError( err instanceof HttpError && err.detail ? err.detail : t("welcome.auth.genericError"), ); @@ -443,11 +466,19 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { } } - const titleKey = TITLE_KEY[mode]; + const titleKey = blocked ? `welcome.auth.blocked.${blocked.reason}.title` : TITLE_KEY[mode]; + const BlockedIcon = blocked ? BLOCKED_ICON[blocked.reason] : null; return (
-

{t(titleKey)}

+

+ {BlockedIcon && ( + + + + )} + {t(titleKey)} +

{/* One-time status banners from a redirect (Google-denied, email verification). */} {verifyOutcome === "ok" && {t("welcome.auth.verifyOk")}} @@ -467,7 +498,26 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { )} - {bounced === "requested" && {t("welcome.auth.accessRequested")}} + {bounced === "requested" && + (bouncedEmail ? ( + + {t("welcome.auth.accessRequestedFor")} +
+ {t("welcome.auth.accessRequestedReview")} + + ) : ( + {t("welcome.auth.accessRequested")} + ))} + {/* The alias insight, stated only when it applies: Google answered with one spelling of a + mailbox whose request is already on file under another. Silence here is what let the + 2026-09-03 registrant believe nothing had been recorded at all. */} + {bounced === "requested" && bouncedLinked && ( + + {t("welcome.auth.accessLinked")} +
+ {t("welcome.auth.accessLinkedTail")} + + )} {bounced === "denied" && {t("welcome.auth.accessDenied")}} {login === "suspended" && {t("welcome.auth.suspended")}} {login === "oauth_cancelled" && ( @@ -475,7 +525,9 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { )} {deleted && {t("welcome.auth.deleted")}} - {done ? ( + {blocked ? ( + setBlocked(null)} /> + ) : done ? (

{done}

+ {reason === "email_unverified" && ( + + )} +
+
+ ); +} + +// One row of the two-gate checklist. Both states get their own full sentence rather than a shared +// label plus a status word: Hungarian puts them together differently, and gluing translated +// fragments is how a bilingual UI ends up reading like neither language. +function Gate({ done, k }: { done: boolean; k: "email" | "approval" }) { + const { t } = useTranslation(); + const Icon = done ? Check : Clock; + return ( + + + {t(`welcome.auth.blocked.gate.${k}${done ? "Done" : "Pending"}`)} + + ); +} + // `role="status"` because these appear in response to something that just happened (a verification // link, a denied Google sign-in, a completed deletion) rather than being part of the card at rest — // without it a screen-reader user gets no announcement that anything changed. diff --git a/frontend/src/components/authCardStates.dom.test.tsx b/frontend/src/components/authCardStates.dom.test.tsx new file mode 100644 index 00000000..a713e02e --- /dev/null +++ b/frontend/src/components/authCardStates.dom.test.tsx @@ -0,0 +1,177 @@ +// @vitest-environment jsdom +// +// The honest states of the sign-in card (E9.5 S4). Two things are under test and they share a root: +// a screen that does not name the address it is talking about hides an alias mismatch. On 2026-09-03 +// a registration as `np.temu@gmail.com` met a Google claim of `nptemu@gmail.com` and every screen in +// the funnel stayed silent about it. +// +// entryUrl snapshots window.location at IMPORT time, so the access-notice cases reset the module +// registry and set the URL before importing — that is the behaviour under test, not a workaround. +import { describe, it, expect, afterEach, vi } from "vitest"; +import { render, screen, fireEvent, cleanup, act } from "@testing-library/react"; +import type { VerifyEntry } from "../lib/useVerifyEntry"; + +const noVerify: VerifyEntry = { outcome: null, email: null, dismiss: () => {} }; + +afterEach(() => { + cleanup(); + vi.resetModules(); + vi.restoreAllMocks(); + window.history.replaceState(null, "", "/"); +}); + +/** Mount the real card at a given entry URL, with i18n as a key-passthrough and /auth/* stubbed. */ +async function mountCard(url: string, apiOverrides: Record = {}) { + window.history.replaceState(null, "", url); + vi.resetModules(); + vi.doMock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ + t: (key: string, opts?: Record) => + opts && typeof opts.contact === "string" ? `${key}:${opts.contact}` : key, + i18n: { language: "en" }, + }), + }; + }); + vi.doMock("../lib/api", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + api: { + ...actual.api, + authConfig: () => Promise.resolve({ google_enabled: false }), + ...apiOverrides, + }, + }; + }); + const { HttpError } = await import("../lib/api"); + const { default: Welcome } = await import("./Welcome"); + await act(async () => { + render(); + }); + return { HttpError }; +} + +/** Fill the sign-in form and submit it. */ +async function signIn(email = "someone@example.com") { + fireEvent.change(screen.getByPlaceholderText("welcome.auth.email"), { target: { value: email } }); + fireEvent.change(screen.getByPlaceholderText("welcome.auth.password"), { + target: { value: "correct horse battery" }, + }); + await act(async () => { + fireEvent.click(screen.getByText("welcome.auth.signinButton")); + }); +} + +/** A password-login refusal carrying the server's structured detail. */ +function refusal( + HttpError: typeof import("../lib/api").HttpError, + detail: Record, +) { + return () => Promise.reject(new HttpError(403, "ignored", { code: "login", ...detail })); +} + +describe("a refused sign-in becomes a named state", () => { + it("names the address the account is on, not the one that was typed", async () => { + const passwordLogin = vi.fn(); + const { HttpError } = await mountCard("/", { passwordLogin }); + passwordLogin.mockImplementation( + refusal(HttpError, { + reason: "pending_approval", + email: "np.temu@gmail.com", + email_verified: true, + approved: false, + }), + ); + + await signIn("nptemu@gmail.com"); + + expect(screen.getByText("welcome.auth.blocked.pending_approval.title")).toBeTruthy(); + expect(screen.getByText("np.temu@gmail.com")).toBeTruthy(); + // The password was already proven correct, so the form is not the next step and must be gone — + // leaving it up would only invite a retry that cannot succeed. + expect(screen.queryByPlaceholderText("welcome.auth.password")).toBeNull(); + }); + + it("reports each gate from its own flag, so an approved-but-unverified account is told the truth", async () => { + const passwordLogin = vi.fn(); + const { HttpError } = await mountCard("/", { passwordLogin }); + passwordLogin.mockImplementation( + refusal(HttpError, { + reason: "email_unverified", + email: "approved.first@example.com", + email_verified: false, + approved: true, + }), + ); + + await signIn("approved.first@example.com"); + + // E9.5 S1 made the two gates order-independent; a screen that assumed a sequence would send this + // user off to wait for an admin who has already decided. + expect(screen.getByText("welcome.auth.blocked.gate.emailPending")).toBeTruthy(); + expect(screen.getByText("welcome.auth.blocked.gate.approvalDone")).toBeTruthy(); + expect(screen.getByText("welcome.auth.resendCta")).toBeTruthy(); + }); + + it("shows no checklist for a suspension, which is not a gate that opens on its own", async () => { + const passwordLogin = vi.fn(); + const { HttpError } = await mountCard("/", { passwordLogin }); + passwordLogin.mockImplementation( + refusal(HttpError, { + reason: "suspended", + email: "banned@example.com", + operator: "ops@example.com", + }), + ); + + await signIn("banned@example.com"); + + expect(screen.getByText("welcome.auth.blocked.suspended.title")).toBeTruthy(); + expect(screen.getByText("welcome.auth.blocked.operator:ops@example.com")).toBeTruthy(); + expect(screen.queryByText("welcome.auth.blocked.gate.emailDone")).toBeNull(); + }); + + it("leaves a wrong password as a line under the form, where the form IS the next step", async () => { + const passwordLogin = vi.fn(); + const { HttpError } = await mountCard("/", { passwordLogin }); + passwordLogin.mockImplementation(() => + Promise.reject(new HttpError(401, "localized invalid", { code: "login", reason: "invalid" })), + ); + + await signIn(); + + expect(screen.getByText("localized invalid")).toBeTruthy(); + expect(screen.getByPlaceholderText("welcome.auth.password")).toBeTruthy(); + }); +}); + +describe("the access-request notice from a denied Google sign-in", () => { + it("names the address the request was recorded for", async () => { + await mountCard("/#access=requested&email=nptemu%40gmail.com"); + + expect(screen.getByText("welcome.auth.accessRequestedFor")).toBeTruthy(); + expect(screen.getByText("nptemu@gmail.com")).toBeTruthy(); + expect(screen.queryByText("welcome.auth.accessLinked")).toBeNull(); + }); + + it("explains the alias when the request is already on file under another spelling", async () => { + await mountCard( + "/#access=requested&email=nptemu%40gmail.com&linked=np.temu%2Bsiftlode%40gmail.com", + ); + + expect(screen.getByText("welcome.auth.accessLinked")).toBeTruthy(); + // %2B, not a bare `+`: URLSearchParams reads the latter as a space and would mangle exactly the + // address family this epic exists to protect. + expect(screen.getByText("np.temu+siftlode@gmail.com")).toBeTruthy(); + }); + + it("falls back to the unnamed notice when the server sent no address", async () => { + await mountCard("/#access=requested"); + + expect(screen.getByText("welcome.auth.accessRequested")).toBeTruthy(); + expect(screen.queryByText("welcome.auth.accessRequestedFor")).toBeNull(); + }); +}); diff --git a/frontend/src/i18n/locales/en/welcome.json b/frontend/src/i18n/locales/en/welcome.json index fa33d127..7338cb94 100644 --- a/frontend/src/i18n/locales/en/welcome.json +++ b/frontend/src/i18n/locales/en/welcome.json @@ -93,6 +93,41 @@ "resendDone": "If that email has an unverified account, we've sent a new verification link. Check your inbox.", "oauthCancelled": "Google sign-in was cancelled. You can try again or use email and password.", "accessRequested": "Thanks — we recorded your request. An admin will review it.", + "accessRequestedFor": "We've recorded your access request for:", + "accessRequestedReview": "An admin will review it, and we'll email you either way.", + "accessLinked": "You already had a request on file for this mailbox, under a different spelling:", + "accessLinkedTail": "Google returns one fixed spelling of a Gmail address, so we linked this sign-in to that request instead of opening a second one. Any mail goes to the address above.", + "blocked": { + "addressLabel": "Address on file", + "operator": "If you think this is a mistake, contact the operator at {{contact}}.", + "gate": { + "emailDone": "Email address confirmed", + "emailPending": "Email address — not confirmed yet", + "approvalDone": "Admin approval — granted", + "approvalPending": "Admin approval — pending" + }, + "email_unverified": { + "title": "Confirm your email address", + "body": "Your password is correct — we just can't let you in until the address on this account is confirmed. Open the link we emailed you, or ask for a new one.", + "short": "Confirm your email address first — check your inbox." + }, + "pending_approval": { + "title": "Waiting for approval", + "body": "Your password is correct and your address is confirmed. An admin still has to let you in — we'll email you as soon as they decide.", + "short": "Your account is waiting for admin approval." + }, + "suspended": { + "title": "Account suspended", + "body": "This account has been suspended, so it can't sign in by any method.", + "short": "This account has been suspended." + }, + "invalid": { + "short": "Invalid email or password." + }, + "rate_limited": { + "short": "Too many attempts. Try again shortly." + } + }, "accessDenied": "That account isn't approved for this instance yet — register below or ask the admin for access.", "suspended": "This account has been suspended and can't sign in. If you think this is a mistake, contact the operator (check your email for details).", "deleted": "Your account and all its data were permanently deleted. We've emailed you a confirmation." diff --git a/frontend/src/i18n/locales/hu/welcome.json b/frontend/src/i18n/locales/hu/welcome.json index 6b49f788..af1b78e5 100644 --- a/frontend/src/i18n/locales/hu/welcome.json +++ b/frontend/src/i18n/locales/hu/welcome.json @@ -93,6 +93,41 @@ "resendDone": "Ha ehhez az e-mailhez tartozik megerősítetlen fiók, elküldtünk egy új megerősítő linket. Nézd meg a postafiókod.", "oauthCancelled": "A Google-belépés megszakadt. Próbáld újra, vagy lépj be e-maillel és jelszóval.", "accessRequested": "Köszönjük — rögzítettük a kérésed. Egy admin felülvizsgálja.", + "accessRequestedFor": "Rögzítettük a hozzáférési kérelmedet erre a címre:", + "accessRequestedReview": "Egy admin átnézi, és e-mailben szólunk a döntésről.", + "accessLinked": "Ehhez a postafiókhoz már volt kérelmed, csak más írásmóddal:", + "accessLinkedTail": "A Google egy rögzített írásmódot ad vissza a Gmail-címekből, ezért ezt a belépést a meglévő kérelemhez kötöttük, nem indítottunk újat. A levelek a fenti címre mennek.", + "blocked": { + "addressLabel": "Rögzített cím", + "operator": "Ha tévedésnek gondolod, keresd az üzemeltetőt: {{contact}}.", + "gate": { + "emailDone": "E-mail-cím megerősítve", + "emailPending": "E-mail-cím — még nincs megerősítve", + "approvalDone": "Admin jóváhagyás — megvan", + "approvalPending": "Admin jóváhagyás — folyamatban" + }, + "email_unverified": { + "title": "Erősítsd meg az e-mail-címed", + "body": "A jelszavad jó — csak addig nem engedhetünk be, amíg a fiókon lévő cím nincs megerősítve. Nyisd meg a kiküldött linket, vagy kérj újat.", + "short": "Előbb erősítsd meg az e-mail-címed — nézd meg a postafiókod." + }, + "pending_approval": { + "title": "Jóváhagyásra vár", + "body": "A jelszavad jó, a címed megerősítve. Már csak arra várunk, hogy egy admin beengedjen — szólunk e-mailben, amint döntött.", + "short": "A fiókod admin jóváhagyásra vár." + }, + "suspended": { + "title": "Felfüggesztett fiók", + "body": "Ez a fiók fel van függesztve, így semmilyen módon nem tud belépni.", + "short": "Ez a fiók fel van függesztve." + }, + "invalid": { + "short": "Hibás e-mail-cím vagy jelszó." + }, + "rate_limited": { + "short": "Túl sok próbálkozás. Próbáld újra kicsit később." + } + }, "accessDenied": "Ez a fiók még nincs jóváhagyva ehhez a példányhoz — regisztrálj lent, vagy kérj hozzáférést az admintól.", "suspended": "Ez a fiók fel van függesztve, nem tud belépni. Ha tévedésnek gondolod, keresd az üzemeltetőt (a részletek az e-mailedben).", "deleted": "A fiókod és minden adata véglegesen törölve lett. A megerősítést e-mailben is elküldtük." diff --git a/frontend/src/lib/api.ts b/frontend/src/lib/api.ts index b3e91cfd..fd9349ff 100644 --- a/frontend/src/lib/api.ts +++ b/frontend/src/lib/api.ts @@ -27,6 +27,7 @@ import { accountApi } from "./api/account"; export type * from "./api/account"; import { authApi } from "./api/auth"; export type * from "./api/auth"; +export { loginBlocked } from "./api/auth"; import { setupApi } from "./api/setup"; export type * from "./api/setup"; import { feedApi } from "./api/feed"; diff --git a/frontend/src/lib/api/auth.ts b/frontend/src/lib/api/auth.ts index 545f4aaa..f3df502f 100644 --- a/frontend/src/lib/api/auth.ts +++ b/frontend/src/lib/api/auth.ts @@ -3,6 +3,47 @@ // are surfaced inline (quiet) rather than via the global modal. Only touches core's req. import { req } from "./core"; +/** Why password sign-in refused. The three account-shaped reasons name the mailbox and both gates, + * so the card can render an honest state instead of a red line; `invalid` and `rate_limited` answer + * BEFORE the password is proven and therefore carry nothing (they must stay identical for a real + * account and an unknown one). Since E9.5 S1 the gates are order-independent — read both flags, never + * infer one from the other. */ +export type LoginBlocked = { + reason: "email_unverified" | "pending_approval" | "suspended" | "invalid" | "rate_limited"; + /** The address ON THE ACCOUNT — not the spelling that was typed. Absent for the two pre-proof + * reasons. Naming it is what would have exposed the 2026-09-03 Gmail-alias mismatch in a second. */ + email?: string; + emailVerified?: boolean; + approved?: boolean; + /** Operator contact, when the instance publishes one (suspended accounts only). */ + operator?: string; +}; + +const LOGIN_REASONS = new Set([ + "email_unverified", + "pending_approval", + "suspended", + "invalid", + "rate_limited", +]); + +/** Read a `{code:"login", …}` error detail into LoginBlocked, or null for anything else — including + * a reason a NEWER server invented, which the card must fall back to a plain message for rather than + * render as a blank panel. */ +export function loginBlocked(detail: unknown): LoginBlocked | null { + const d = detail as Record | null | undefined; + if (!d || d.code !== "login") return null; + const reason = d.reason as LoginBlocked["reason"]; + if (!LOGIN_REASONS.has(reason)) return null; + return { + reason, + email: typeof d.email === "string" ? d.email : undefined, + emailVerified: typeof d.email_verified === "boolean" ? d.email_verified : undefined, + approved: typeof d.approved === "boolean" ? d.approved : undefined, + operator: typeof d.operator === "string" ? d.operator : undefined, + }; +} + export const authApi = { // Public: which sign-in options this instance offers (e.g. hide Google when not configured). authConfig: (): Promise<{ @@ -18,6 +59,7 @@ export const authApi = { { method: "POST", body: JSON.stringify({ email, password }) }, { quiet: true }, ), + // A refusal carries a structured `{code:"login", reason, …}` detail — see LoginBlocked above. passwordLogin: (email: string, password: string): Promise<{ ok: boolean }> => req( "/auth/password-login", diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index c3bf4e0a..952229c6 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -46,6 +46,12 @@ function localizeDetail(d: unknown): string { return t(`downloads.errors.edit.${o.reason}`, t("downloads.errors.edit.generic")); if (o?.code === "cookies") return t(`downloads.errors.cookies.${o.reason}`, t("downloads.errors.cookies.generic")); + // Password sign-in refusals (E9.5 S4). The card renders the account-shaped ones as a full panel + // off `detailData`; this is the one-line form every other consumer (and the card's own fallback) + // gets. A reason this build doesn't know falls through to the generic message rather than showing + // a raw key, which is what an i18n miss would otherwise put on screen. + if (o?.code === "login") + return t(`welcome.auth.blocked.${o.reason}.short`, t("welcome.auth.genericError")); // Local-upload ingest (E7 S5): the per-file cap carries the limit in bytes; empty is a bare code. if (o?.code === "upload_too_large") return t("downloads.errors.upload.tooLarge", { size: formatBytes(o.limit) }); diff --git a/frontend/src/lib/entryUrl.ts b/frontend/src/lib/entryUrl.ts index f84a5993..04d1954f 100644 --- a/frontend/src/lib/entryUrl.ts +++ b/frontend/src/lib/entryUrl.ts @@ -10,7 +10,9 @@ // cannot lose the race no matter who mounts first or which chunk loads when. // // Secret tokens (verify, reset) ride the FRAGMENT so they never reach the server, a proxy log or a -// Referer header (SB3); plain status flags ride the query string. +// Referer header (SB3); so does the Google access-request notice (`access`, and the addresses it +// names) — an email in a query string lands in every reverse-proxy access log. Plain status flags +// with nothing personal in them (login, deleted) ride the query string. export const entrySearch = new URLSearchParams(window.location.search); export const entryHash = new URLSearchParams(window.location.hash.replace(/^#/, "")); From b1086e05dc94fe595ecf9dec88ad53832f5a0e95 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 4 Sep 2026 02:38:01 +0200 Subject: [PATCH 08/16] fix(auth): stop drawing a reset form the link can no longer fill (E9.5 S4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported from UAT: delete an account, then open its password-reset link. The card drew the full "set a new password" form — because a `#reset=` fragment was present, and nothing more — and only after the user had typed a password did the server answer 400. Measured first: no security hole. A user's tokens cascade away with the row, so nothing can be set on a deleted or a re-created account. What was left is a dead end of exactly the kind this epic exists to remove, and it is not rare: a reset link lives one hour, mail is slow or lands in spam, and whoever got two mails clicks the older one. password-reset/check answers whether a link is still usable WITHOUT spending it, so asking can never cost the user their one real attempt — the property the tests pin hardest. It gets its own limiter, because opening a link must not eat the budget for requesting one, and a throttled caller is told "ok", never "dead": this check may only ever save someone a wasted form, so when it cannot answer it degrades to the old behaviour instead of declaring a live link finished. The confirm endpoint stays the authority, and its failure raises the same named screen, since the token can die between the two calls or the probe itself may have failed. A too-short password is deliberately NOT that screen. The length check runs before the token is spent, so a typo costs the link nothing, and the card tells the two 400s apart by `reason` so only one of them takes the form down. Both email-link confirmations (verify, password-reset/confirm) now answer in the {code, reason} shape S4 introduced. They were the last refusals a user meets by simply clicking an old link while still being answered in hardcoded English under a bilingual UI. The seven remaining bare strings in this module (register, request-access, the demo guards, set-password) sit behind client-side validation the UI does first; they are logged as a follow-up rather than swept in silently here. One bug found by the UAT itself, in the new code: the check went out FOUR times per opened link — StrictMode double-invokes effects and the shell remounts the card as the session probe settles. Harmless per request, but against a per-IP limit it would exhaust the budget in two reloads and then silently fall back to drawing the form unchecked: nothing errors, the feature just stops working. Deduped in a module-level map holding the in-flight promise (a ref dies with the remount that causes it); a failed probe drops itself so a blip is never cached as "dead". The test renders the card twice, because with one render the guard would rot undetected. UAT on localdev: the reported scenario end to end (delete the account, open the link) shows the named screen and no form; a live link still opens the form and completes; the CTA lands on the request-a- link form; one probe per opened link. Throwaway accounts removed. --- backend/app/routes/auth.py | 53 +++++- backend/tests/test_reset_link_states.py | 90 ++++++++++ frontend/src/components/Welcome.tsx | 155 +++++++++++++++--- .../components/authCardStates.dom.test.tsx | 92 ++++++++++- frontend/src/i18n/locales/en/welcome.json | 6 + frontend/src/i18n/locales/hu/welcome.json | 6 + frontend/src/lib/api.ts | 2 +- frontend/src/lib/api/auth.ts | 17 ++ frontend/src/lib/api/core.ts | 12 +- 9 files changed, 400 insertions(+), 33 deletions(-) create mode 100644 backend/tests/test_reset_link_states.py diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index c98d434d..a78dfb15 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -50,6 +50,8 @@ _DECOY_PASSWORD_HASH = hash_password(secrets.token_urlsafe(16)) _register_limiter = RateLimiter(max_events=5, window_seconds=300) _login_limiter = RateLimiter(max_events=10, window_seconds=300) _reset_limiter = RateLimiter(max_events=5, window_seconds=300) +# Separate from _reset_limiter on purpose: opening a link must not eat the budget for REQUESTING one. +_reset_check_limiter = RateLimiter(max_events=10, window_seconds=300) _verify_resend_limiter = RateLimiter(max_events=5, window_seconds=300) _request_access_limiter = RateLimiter(max_events=5, window_seconds=300) _suspend_email_limiter = RateLimiter(max_events=1, window_seconds=3600) @@ -638,6 +640,29 @@ def _issue_token(db: Session, user: User, kind: str, ttl: timedelta) -> str: return raw +def _peek_token(db: Session, raw: str | None, kind: str) -> User | None: + """Is this token still usable — WITHOUT spending it. Same four conditions as _consume_token, and + the same "does its user still exist" answer, so the two can never disagree about a given token. + + Deliberately NOT the read half of a read-check-write: _consume_token stays one atomic UPDATE, and + this never feeds it. A peek is only ever an early warning — the confirm endpoint remains the + authority, because a token can expire, be spent, or lose its account between the two calls. + """ + if not raw: + return None + row = db.execute( + select(AuthToken).where( + AuthToken.token_hash == hash_token(raw), + AuthToken.kind == kind, + AuthToken.used_at.is_(None), + AuthToken.expires_at >= datetime.now(timezone.utc), + ) + ).scalar_one_or_none() + # db.get, exactly as _consume_token does: a deleted account cascades its tokens away, so this is + # belt-and-braces — but the two must agree, and agreeing by construction beats agreeing by luck. + return db.get(User, row.user_id) if row is not None else None + + def _consume_token(db: Session, raw: str | None, kind: str) -> User | None: """Validate + burn a token atomically. Returns its user, or None if missing/expired/used/wrong kind. The burn is a single conditional UPDATE (…WHERE used_at IS NULL…RETURNING) rather than a @@ -766,9 +791,7 @@ def verify_email_confirm(body: TokenIn, db: Session = Depends(get_db)) -> dict: flag is rare now — but it must stay, because those users have no other way back in.""" user = _consume_token(db, body.token, "verify") if user is None: - raise HTTPException( - status_code=400, detail="This verification link is invalid or has expired." - ) + raise HTTPException(status_code=400, detail={"code": "verify", "reason": "invalid"}) user.email_verified = True needs_password = user.password_hash is None and user.password_revoked_at is not None email = user.email @@ -916,6 +939,26 @@ def _send_reset_if_eligible(email: str) -> None: email_mod.send_password_reset(user.email, f"{_app_base()}/#reset={raw}") +@router.post("/password-reset/check") +def password_reset_check(body: TokenIn, request: Request, db: Session = Depends(get_db)) -> dict: + """Is the reset link the SPA just opened still good? Answers without spending the token. + + Why this exists: the card used to render "set a new password" purely because a `#reset=` fragment + was present, never asking whether it could still succeed. Reported on 2026-09-05 after deleting an + account and then opening its reset link — the user types a new password into a form that answers + 400. Opening a dead reset link is not an edge case: the token lives one hour, mail is slow or lands + in spam, and whoever got two mails usually clicks the older one. + + Not an enumeration oracle: probing requires already holding a 43-character random, and the answer + is exactly what submitting it would reveal. Throttled per IP anyway — and a throttled caller is + told `true`, never `false`: this check may only ever save a user a wasted form, so when it cannot + answer it must degrade to the old behaviour instead of declaring a live link dead. + """ + if not _reset_check_limiter.allow(_client_ip(request)): + return {"ok": True} + return {"ok": _peek_token(db, body.token, "reset") is not None} + + @router.post("/password-reset/confirm") def password_reset_confirm(body: TokenPasswordIn, db: Session = Depends(get_db)) -> dict: """Set a new password from a valid reset token, then invalidate any other reset tokens.""" @@ -924,11 +967,11 @@ def password_reset_confirm(body: TokenPasswordIn, db: Session = Depends(get_db)) if len(new_password) < PASSWORD_MIN_LEN: raise HTTPException( status_code=400, - detail=f"Password must be at least {PASSWORD_MIN_LEN} characters.", + detail={"code": "reset", "reason": "weak_password", "min": PASSWORD_MIN_LEN}, ) user = _consume_token(db, token, "reset") if user is None: - raise HTTPException(status_code=400, detail="This reset link is invalid or has expired.") + raise HTTPException(status_code=400, detail={"code": "reset", "reason": "invalid"}) user.password_hash = hash_password(new_password) user.password_revoked_at = None # the account has a password again — SEC-10's trace is spent # A valid reset link proves control of the mailbox, so completing a reset also verifies the diff --git a/backend/tests/test_reset_link_states.py b/backend/tests/test_reset_link_states.py new file mode 100644 index 00000000..a11c03d4 --- /dev/null +++ b/backend/tests/test_reset_link_states.py @@ -0,0 +1,90 @@ +"""What a dead email link says, and what it costs to ask (E9.5 S4 follow-up), DB-lane. + +Reported 2026-09-05: delete an account, then open its password-reset link. The card drew a full +"set a new password" form — because a `#reset=` fragment was present, nothing more — and only after +the user typed a password did the server answer 400. No security hole (the account's tokens cascade +away with it, so nothing can be set on a deleted or a re-created account), but a dead end of exactly +the kind this epic exists to remove. + +The fix is a check that does NOT spend the token, so the pre-flight can never cost the user their +one real attempt. That is the property most worth pinning here. +""" + +from app.routes.auth import RESET_TTL, _issue_token + +_PW = "correct horse battery" +_NEW = "brand new password" + + +def _issue(db, user): + raw = _issue_token(db, user, "reset", RESET_TTL) + db.commit() + return raw + + +def test_checking_a_live_link_does_not_spend_it(client, db, make_user): + raw = _issue(db, make_user(email="live@example.com", password=_PW)) + + assert client.post("/auth/password-reset/check", json={"token": raw}).json() == {"ok": True} + assert client.post("/auth/password-reset/check", json={"token": raw}).json() == {"ok": True} + + # The whole point: after any number of checks the one real attempt is still there. + assert client.post( + "/auth/password-reset/confirm", json={"token": raw, "password": _NEW} + ).json() == {"ok": True} + + +def test_a_link_whose_account_was_deleted_is_reported_dead(client, db, make_user): + user = make_user(email="gone@example.com", password=_PW) + raw = _issue(db, user) + db.delete(user) + db.commit() + + assert client.post("/auth/password-reset/check", json={"token": raw}).json() == {"ok": False} + + # And the confirm agrees, in the structured shape the card renders as a named screen — the two + # must never disagree about one token. + r = client.post("/auth/password-reset/confirm", json={"token": raw, "password": _NEW}) + assert r.status_code == 400 + assert r.json()["detail"] == {"code": "reset", "reason": "invalid"} + + +def test_a_spent_link_is_reported_dead(client, db, make_user): + raw = _issue(db, make_user(email="spent@example.com", password=_PW)) + client.post("/auth/password-reset/confirm", json={"token": raw, "password": _NEW}) + + assert client.post("/auth/password-reset/check", json={"token": raw}).json() == {"ok": False} + + +def test_a_too_short_password_does_not_burn_the_link(client, db, make_user): + raw = _issue(db, make_user(email="short@example.com", password=_PW)) + + r = client.post("/auth/password-reset/confirm", json={"token": raw, "password": "short"}) + + assert r.status_code == 400 + assert r.json()["detail"]["reason"] == "weak_password" + # A mistyped password must not cost the link — the length check runs BEFORE the token is spent, + # and the card tells the two failures apart by `reason` so only one of them takes the form down. + assert client.post( + "/auth/password-reset/confirm", json={"token": raw, "password": _NEW} + ).json() == {"ok": True} + + +def test_a_dead_verification_link_answers_in_the_same_structured_shape(client): + r = client.post("/auth/verify", json={"token": "no-such-token"}) + + assert r.status_code == 400 + assert r.json()["detail"] == {"code": "verify", "reason": "invalid"} + + +def test_the_check_never_reports_a_live_link_dead_when_throttled( + client, db, make_user, monkeypatch +): + from app.routes import auth as auth_routes + + raw = _issue(db, make_user(email="throttled@example.com", password=_PW)) + monkeypatch.setattr(auth_routes._reset_check_limiter, "allow", lambda _ip: False) + + # Fail OPEN. This check may only ever save a wasted form; a throttled probe that answered "dead" + # would lock someone out of a link that works. + assert client.post("/auth/password-reset/check", json={"token": raw}).json() == {"ok": True} diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx index a37aecb9..203ab53c 100644 --- a/frontend/src/components/Welcome.tsx +++ b/frontend/src/components/Welcome.tsx @@ -11,6 +11,7 @@ import { Expand, Filter, Inbox, + Link2Off, ListVideo, Lock, MailWarning, @@ -23,7 +24,7 @@ import { // Siftlode is open source; the landing footer links to the public repository. const REPO_URL = "https://git.b1fr0st.eu/peter/siftlode"; -import { api, loginBlocked, type LoginBlocked } from "../lib/api"; +import { api, deadResetLink, loginBlocked, type LoginBlocked } from "../lib/api"; import { HttpError } from "../lib/api"; import { setLanguage, type LangCode } from "../i18n"; import LanguageSwitcher from "./LanguageSwitcher"; @@ -307,6 +308,30 @@ function Lightbox({ src, label, onClose }: { src: string; label: string; onClose ); } +// One answer per reset token, however many times React mounts this card. StrictMode double-invokes +// effects in dev and the shell remounts the card as the session probe settles — measured in UAT on +// 2026-09-05: FOUR probes went out for one opened link. That would burn the endpoint's per-IP budget +// after a couple of reloads and silently drop back to drawing the form unchecked. Holding the +// in-flight promise means a remount joins the request already running instead of starting another. +const resetTokenChecks = new Map>(); + +function checkResetToken(token: string): Promise { + let pending = resetTokenChecks.get(token); + if (!pending) { + pending = api + .checkPasswordReset(token) + .then((r) => r.ok) + // A probe that FAILED says nothing about the token, so it answers "go ahead" and forgets + // itself: a network blip must never be cached as "this link is dead". + .catch(() => { + resetTokenChecks.delete(token); + return true; + }); + resetTokenChecks.set(token, pending); + } + return pending; +} + function AuthCard({ verify }: { verify: VerifyEntry }) { const { t } = useTranslation(); // The entry URL is snapshotted once at import time (lib/entryUrl) rather than here, so nothing can @@ -355,6 +380,31 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { // (E9.5 S4). It REPLACES the form: in all three of those states the password was already proven // correct, so there is nothing to retype — leaving the form up would only invite a pointless retry. const [blocked, setBlocked] = useState(null); + // The emailed reset link turned out to be spent, expired, or to belong to an account that no longer + // exists. Same reasoning as `blocked`: the form cannot succeed, so it must not be on screen. + const [deadReset, setDeadReset] = useState(false); + // Ask whether the `#reset=` token is still usable BEFORE drawing the form, and hold the card blank + // until the answer lands. Reported 2026-09-05: deleting an account and then opening its reset link + // rendered a full "set a new password" form that could only ever answer 400 — the user finds out + // after typing. This is advisory (the confirm below stays the authority); a check that errors is + // treated as "go ahead", never as "dead", so a broken probe cannot lock anyone out of a live link. + const [checkingReset, setCheckingReset] = useState(() => entryHash.get("reset") !== null); + useEffect(() => { + const token = entryHash.get("reset"); + if (!token) return; + let live = true; + void checkResetToken(token).then((ok) => { + if (!live) return; + if (!ok) { + setDeadReset(true); + setResetToken(null); // and stop the reset view being the card's root, so Back can't return + } + setCheckingReset(false); + }); + return () => { + live = false; + }; + }, []); // `done` and `error` belong to the view that produced them, and the render is `done ? msg : form` // — without this, Back out of a submitted form would show the old success message under the // previous view's title. Clearing on view change covers both Back/Forward and the mode links. @@ -362,6 +412,7 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { setDone(""); setError(""); setBlocked(null); + setDeadReset(false); }, [mode]); // Explicit demo entry (replaces the old hidden probe): a labelled email field, still gated @@ -375,6 +426,7 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { setError(""); setDone(""); setBlocked(null); + setDeadReset(false); // The verify/login banners are one-time entry notices — dismiss once the user takes any // deliberate action (mode switch, CTA, submit) so they don't linger stalely on later screens. verify.dismiss(); @@ -438,6 +490,13 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { setBlocked(refusal); return; } + // The token died between the pre-flight check and this submit — or was never checked, because + // the probe itself failed. Either way the answer is the same screen, not a red line. + if (mode === "reset" && err instanceof HttpError && deadResetLink(err.detailData)) { + setDeadReset(true); + setResetToken(null); + return; + } setError( err instanceof HttpError && err.detail ? err.detail : t("welcome.auth.genericError"), ); @@ -466,15 +525,21 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { } } - const titleKey = blocked ? `welcome.auth.blocked.${blocked.reason}.title` : TITLE_KEY[mode]; - const BlockedIcon = blocked ? BLOCKED_ICON[blocked.reason] : null; + // A named state owns the card's heading while it is up, so it reads as the card's subject rather + // than a box sitting inside a heading that still says "Set a new password". + const titleKey = blocked + ? `welcome.auth.blocked.${blocked.reason}.title` + : deadReset + ? "welcome.auth.deadLink.title" + : TITLE_KEY[mode]; + const StateIcon = blocked ? BLOCKED_ICON[blocked.reason] : deadReset ? Link2Off : null; return (

- {BlockedIcon && ( + {StateIcon && ( - + )} {t(titleKey)} @@ -527,6 +592,19 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { {blocked ? ( setBlocked(null)} /> + ) : deadReset ? ( + { + reset(); + setMode("signin"); + }} + /> + ) : checkingReset ? ( + // Blank rather than the form: showing a form we may be about to withdraw is the flicker this + // whole check exists to remove. +

{t("welcome.auth.working")}

) : done ? (

{done}

@@ -698,6 +776,43 @@ const BLOCKED_ICON: Partial> = { suspended: ShieldAlert, }; +// The frame every named state in this card shares: a sentence, whatever detail that state can give, +// then the way out. The icon and title sit in the card's own h2 (see titleKey/StateIcon) so a state +// reads as the card's subject. One frame, because two states that mean "this form cannot succeed" +// should not drift apart in shape. +function StatePanel({ + body, + cta, + onBack, + children, +}: { + body: string; + cta?: { label: string; onClick: () => void }; + onBack: () => void; + children?: React.ReactNode; +}) { + const { t } = useTranslation(); + return ( +
+

{body}

+ {children} +
+ + {cta && ( + + )} +
+
+ ); +} + // The named state behind a 403, in place of the sign-in form (E9.5 S4). It says WHICH mailbox the // account is on and WHICH of the two gates is still shut, because the flat "awaiting admin approval" // string it replaces was true and useless: it never revealed that the address the account carried was @@ -716,11 +831,15 @@ function BlockedPanel({ // Suspension is not a gate that will open on its own, so the checklist would be a lie there. const gated = reason !== "suspended"; return ( -
-

- {t(`welcome.auth.blocked.${reason}.body`)} -

- + {gated && (
@@ -744,21 +863,7 @@ function BlockedPanel({ {t("welcome.auth.blocked.operator", { contact: operator })}

)} - -
- - {reason === "email_unverified" && ( - - )} -
-
+ ); } diff --git a/frontend/src/components/authCardStates.dom.test.tsx b/frontend/src/components/authCardStates.dom.test.tsx index a713e02e..9c03bdc4 100644 --- a/frontend/src/components/authCardStates.dom.test.tsx +++ b/frontend/src/components/authCardStates.dom.test.tsx @@ -51,7 +51,7 @@ async function mountCard(url: string, apiOverrides: Record = {} await act(async () => { render(); }); - return { HttpError }; + return { HttpError, Welcome }; } /** Fill the sign-in form and submit it. */ @@ -148,6 +148,96 @@ describe("a refused sign-in becomes a named state", () => { }); }); +// Reported 2026-09-05: delete an account, then open its reset link. The card drew the full "set a new +// password" form purely because a `#reset=` fragment was present, and only told the user after they +// had typed one. Opening a dead reset link is ordinary — the token lives an hour, mail is slow, and +// whoever got two mails clicks the older one. +describe("an emailed reset link that is already finished", () => { + it("refuses to draw a form that cannot succeed", async () => { + await mountCard("/#reset=dead-token", { + checkPasswordReset: () => Promise.resolve({ ok: false }), + }); + + expect(screen.getByText("welcome.auth.deadLink.title")).toBeTruthy(); + expect(screen.getByText("welcome.auth.deadLink.cta")).toBeTruthy(); + expect(screen.queryByPlaceholderText("welcome.auth.newPassword")).toBeNull(); + }); + + it("draws the form when the link is still good", async () => { + await mountCard("/#reset=live-token", { + checkPasswordReset: () => Promise.resolve({ ok: true }), + }); + + expect(screen.getByPlaceholderText("welcome.auth.newPassword")).toBeTruthy(); + expect(screen.queryByText("welcome.auth.deadLink.title")).toBeNull(); + }); + + it("takes the form down if the link dies between the check and the submit", async () => { + const confirmPasswordReset = vi.fn(); + const { HttpError } = await mountCard("/#reset=racing-token", { + checkPasswordReset: () => Promise.resolve({ ok: true }), + confirmPasswordReset, + }); + confirmPasswordReset.mockImplementation(() => + Promise.reject(new HttpError(400, "ignored", { code: "reset", reason: "invalid" })), + ); + + fireEvent.change(screen.getByPlaceholderText("welcome.auth.newPassword"), { + target: { value: "brand new password" }, + }); + await act(async () => { + fireEvent.click(screen.getByText("welcome.auth.resetButton")); + }); + + expect(screen.getByText("welcome.auth.deadLink.title")).toBeTruthy(); + }); + + it("keeps the form up for a too-short password, which costs the link nothing", async () => { + const confirmPasswordReset = vi.fn(); + const { HttpError } = await mountCard("/#reset=live-token", { + checkPasswordReset: () => Promise.resolve({ ok: true }), + confirmPasswordReset, + }); + confirmPasswordReset.mockImplementation(() => + Promise.reject( + new HttpError(400, "too short", { code: "reset", reason: "weak_password", min: 10 }), + ), + ); + + fireEvent.change(screen.getByPlaceholderText("welcome.auth.newPassword"), { + target: { value: "0123456789ab" }, + }); + await act(async () => { + fireEvent.click(screen.getByText("welcome.auth.resetButton")); + }); + + expect(screen.getByPlaceholderText("welcome.auth.newPassword")).toBeTruthy(); + expect(screen.queryByText("welcome.auth.deadLink.title")).toBeNull(); + }); + + it("probes a token once, however many times the card mounts", async () => { + const checkPasswordReset = vi.fn().mockResolvedValue({ ok: true }); + const { Welcome } = await mountCard("/#reset=counted-token", { checkPasswordReset }); + + await act(async () => { + render(); + }); + + // StrictMode double-invokes effects and the shell remounts this card as the session probe + // settles; four probes per opened link would exhaust the endpoint's per-IP budget in two reloads + // and silently drop back to drawing the form unchecked. + expect(checkPasswordReset).toHaveBeenCalledTimes(1); + }); + + it("shows the form when the check itself fails, so a broken probe locks nobody out", async () => { + await mountCard("/#reset=live-token", { + checkPasswordReset: () => Promise.reject(new Error("network")), + }); + + expect(screen.getByPlaceholderText("welcome.auth.newPassword")).toBeTruthy(); + }); +}); + describe("the access-request notice from a denied Google sign-in", () => { it("names the address the request was recorded for", async () => { await mountCard("/#access=requested&email=nptemu%40gmail.com"); diff --git a/frontend/src/i18n/locales/en/welcome.json b/frontend/src/i18n/locales/en/welcome.json index 7338cb94..15c3686f 100644 --- a/frontend/src/i18n/locales/en/welcome.json +++ b/frontend/src/i18n/locales/en/welcome.json @@ -85,6 +85,12 @@ "registerDone": "Almost there — check your email for a verification link. After you confirm it, an admin approves your account before you can sign in.", "forgotDone": "If that email has an account, we've sent a reset link. Check your inbox.", "resetDone": "Your password has been set — you can sign in now.", + "deadLink": { + "title": "This link is no longer valid", + "body": "A password reset link lasts one hour and works once. This one has expired, has already been used, or belongs to an account that no longer exists — so there is nothing to set a password on. Ask for a fresh one and we'll email it.", + "short": "This reset link is no longer valid.", + "cta": "Request a new link" + }, "verifyOk": "Email confirmed. Once an admin approves your account, you can sign in.", "verifyOkReset": "Email confirmed. The password this account registered with had been cleared for security — choose a new one to sign in.", "verifyOkResetCta": "Set a new password", diff --git a/frontend/src/i18n/locales/hu/welcome.json b/frontend/src/i18n/locales/hu/welcome.json index af1b78e5..fd3cd8e1 100644 --- a/frontend/src/i18n/locales/hu/welcome.json +++ b/frontend/src/i18n/locales/hu/welcome.json @@ -85,6 +85,12 @@ "registerDone": "Majdnem kész — nézd meg a postafiókod a megerősítő linkért. Miután megerősítetted, egy admin jóváhagyja a fiókodat, mielőtt beléphetnél.", "forgotDone": "Ha ehhez az e-mailhez tartozik fiók, elküldtük a visszaállító linket. Nézd meg a postafiókod.", "resetDone": "A jelszavad be van állítva — most már beléphetsz.", + "deadLink": { + "title": "Ez a link már nem érvényes", + "body": "A jelszó-visszaállító link egy óráig él, és egyszer használható. Ez lejárt, már felhasználták, vagy olyan fiókhoz tartozik, ami már nem létezik — nincs mire jelszót állítani. Kérj újat, és elküldjük.", + "short": "Ez a visszaállító link már nem érvényes.", + "cta": "Új link kérése" + }, "verifyOk": "E-mail megerősítve. Amint egy admin jóváhagyja a fiókodat, beléphetsz.", "verifyOkReset": "E-mail megerősítve. A fiók regisztrációkori jelszavát biztonsági okból korábban töröltük — válassz újat a belépéshez.", "verifyOkResetCta": "Új jelszó beállítása", diff --git a/frontend/src/lib/api.ts b/frontend/src/lib/api.ts index fd9349ff..afcc11e8 100644 --- a/frontend/src/lib/api.ts +++ b/frontend/src/lib/api.ts @@ -27,7 +27,7 @@ import { accountApi } from "./api/account"; export type * from "./api/account"; import { authApi } from "./api/auth"; export type * from "./api/auth"; -export { loginBlocked } from "./api/auth"; +export { loginBlocked, deadResetLink } from "./api/auth"; import { setupApi } from "./api/setup"; export type * from "./api/setup"; import { feedApi } from "./api/feed"; diff --git a/frontend/src/lib/api/auth.ts b/frontend/src/lib/api/auth.ts index f3df502f..946bb54b 100644 --- a/frontend/src/lib/api/auth.ts +++ b/frontend/src/lib/api/auth.ts @@ -19,6 +19,14 @@ export type LoginBlocked = { operator?: string; }; +/** Did a password-reset call fail because the LINK itself is finished — spent, expired, or belonging + * to an account that no longer exists — rather than because the new password was too short? The two + * arrive as the same 400, and only the first one means the form should come down. */ +export function deadResetLink(detail: unknown): boolean { + const d = detail as { code?: string; reason?: string } | null | undefined; + return d?.code === "reset" && d.reason === "invalid"; +} + const LOGIN_REASONS = new Set([ "email_unverified", "pending_approval", @@ -72,6 +80,15 @@ export const authApi = { { method: "POST", body: JSON.stringify({ email }) }, { quiet: true }, ), + // Is a `#reset=` token still usable? Answers WITHOUT spending it, so the card can refuse to draw a + // form that cannot succeed (a deleted account, an expired or already-used link). Advisory only — + // confirmPasswordReset stays the authority, since the token can die between the two calls. + checkPasswordReset: (token: string): Promise<{ ok: boolean }> => + req( + "/auth/password-reset/check", + { method: "POST", body: JSON.stringify({ token }) }, + { quiet: true }, + ), confirmPasswordReset: (token: string, password: string): Promise<{ ok: boolean }> => req( "/auth/password-reset/confirm", diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index 952229c6..819d5cb8 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -31,7 +31,10 @@ function localizeDetail(d: unknown): string { // `loc` field names are English, snake_case internal tokens — surface a clean, fully localized // "check your input" message rather than leaking them into a translated sentence. if (Array.isArray(d)) return t("errors.validation"); - const o = d as { code?: string; reason?: string; limit?: number } | null | undefined; + const o = d as + | { code?: string; reason?: string; limit?: number; min?: number } + | null + | undefined; if (o?.code === "quota") { if (o.reason === "max_bytes") { const gb = new Intl.NumberFormat(i18n.language, { maximumFractionDigits: 1 }).format( @@ -52,6 +55,13 @@ function localizeDetail(d: unknown): string { // a raw key, which is what an i18n miss would otherwise put on screen. if (o?.code === "login") return t(`welcome.auth.blocked.${o.reason}.short`, t("welcome.auth.genericError")); + // The two email-link confirmations. Their failure is one a user meets by simply clicking an old + // link, so it is the last place that may answer in hardcoded English under a bilingual UI. + if (o?.code === "verify") return t("welcome.auth.verifyInvalid"); + if (o?.code === "reset") + return o.reason === "weak_password" + ? t("welcome.auth.weakPassword", { min: o.limit ?? o.min }) + : t("welcome.auth.deadLink.short"); // Local-upload ingest (E7 S5): the per-file cap carries the limit in bytes; empty is a bare code. if (o?.code === "upload_too_large") return t("downloads.errors.upload.tooLarge", { size: formatBytes(o.limit) }); From 7acdf5d5777fc1e86a614a226ad9546343244db3 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 4 Sep 2026 03:27:48 +0200 Subject: [PATCH 09/16] feat(admin): surface the sign-ups that are stuck outside, and let an admin finish letting them in (E9.5 S5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Approve an address BEFORE the person registers and their sign-up produces no queue row at all — `_decide` only takes a still-pending invite — so the Access-requests screen sat empty, the tab badge read zero, and the only working move was re-typing the address into the whitelist box. Nobody would guess that, and on 2026-09-03 nobody did. This is roadmap UX-25, where two earlier attempts were reverted after review. Both moved AUTHORITY: auto-activating on registration turns a standing whitelist entry into a bearer grant for whoever registers the address first, and reopening the approved invite to `pending` lets an anonymous sign-up REVOKE a standing approval. What was actually missing was visibility, so this changes nothing about what an unauthenticated caller can cause — the admin could already do exactly this by re-entering the address; they just had no way to know they had to. The section lists every account that cannot sign in yet and says WHICH gate is shut, because the two dead ends look the same from the admin's chair: waiting for activation, or waiting on a confirmation mail. Naming the unconfirmed ones is not decoration — on a no-SMTP instance that is the difference between the owner and whoever registered the address first (SEC-15), and an admin about to let someone in should see it. A person the queue above already covers is deliberately NOT listed here; two buttons for one person is worse than one. Activate is gated on `is_allowed` — the same function the sign-in paths consult, in core, so the button that appears and the badge that counts cannot disagree. Without that gate it would manufacture an inconsistent account: password sign-in only checks `is_active` and would let them in, while their Google sign-in would be turned away and silently reopened as a fresh access request. Letting in someone with no approved invite stays the whitelist box's job, which says so out loud. The badge counts activations SEPARATELY from pending invites rather than widening an existing field's meaning, and only counts what genuinely waits on the admin: an unconfirmed account waits on its owner, so it is listed with a re-send but does not nag. The admin-side resend says plainly what happened, including refusing when the instance has no working SMTP — mail is fail-soft everywhere else, and that silence is exactly what left the 0.78.5 sign-ups with nothing to fall back on. `send_verification_link` is now one function with three callers (registration, the public resend, this one), so the link's shape and TTL cannot drift by who triggered it. UAT on localdev, both languages: the UX-25 shape reproduced end to end (approve the address, then register), the badge counting it, Activate letting them in and dropping the badge, the row losing its Activate but keeping the re-send, a fresh token issued by the re-send, and both actions in the audit log. Test data removed. --- backend/app/audit.py | 4 + backend/app/auth.py | 39 ++++ backend/app/routes/admin.py | 133 +++++++++++- backend/app/routes/auth.py | 25 ++- backend/app/routes/me.py | 9 + backend/tests/test_admin_pending_access.py | 186 ++++++++++++++++ frontend/src/App.tsx | 7 +- frontend/src/components/AdminUsers.tsx | 198 +++++++++++++++++- .../src/components/adminInvites.dom.test.tsx | 11 +- .../adminPendingAccess.dom.test.tsx | 186 ++++++++++++++++ .../src/i18n/locales/en/auditActions.json | 2 + frontend/src/i18n/locales/en/users.json | 17 ++ .../src/i18n/locales/hu/auditActions.json | 2 + frontend/src/i18n/locales/hu/users.json | 17 ++ frontend/src/lib/api/account.ts | 5 + frontend/src/lib/api/admin.ts | 20 ++ frontend/src/lib/queryKeys.ts | 1 + 17 files changed, 846 insertions(+), 16 deletions(-) create mode 100644 backend/tests/test_admin_pending_access.py create mode 100644 frontend/src/components/adminPendingAccess.dom.test.tsx diff --git a/backend/app/audit.py b/backend/app/audit.py index 52690925..95d24515 100644 --- a/backend/app/audit.py +++ b/backend/app/audit.py @@ -27,6 +27,10 @@ class AuditAction: USER_SUSPEND = "user_suspend" USER_UNSUSPEND = "user_unsuspend" USER_DELETE = "user_delete" + # E9.5 S5: an admin let a stuck registrant in, or re-sent their confirmation link. Both are + # recorded because both are the admin acting ON someone's access, not merely reading a list. + USER_ACTIVATE = "user_activate" + USER_VERIFY_RESEND = "user_verify_resend" INVITE_APPROVE = "invite_approve" INVITE_DENY = "invite_deny" INVITE_ADD = "invite_add" diff --git a/backend/app/auth.py b/backend/app/auth.py index 29f664af..17a4e9e1 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -92,6 +92,45 @@ def is_allowed(db: Session, email: str) -> bool: return email in settings.allowed_email_set or email in settings.admin_email_set +def can_activate(db: Session, user: User) -> bool: + """Is this account merely waiting for an admin to finish letting it in? + + True when it is inactive but its address is ALREADY allowed — the shape roadmap UX-25 describes: + the admin approved the address, the person signed up afterwards, and nothing connects the two + (the Access-requests queue only offers Approve on a still-pending invite). + + Gated on `is_allowed`, the same function the sign-in paths consult, because activating an address + that is NOT allowed manufactures an inconsistent account: password sign-in only checks + `is_active` and would let them in, while their Google sign-in would be turned away and silently + reopened as a fresh access request. Letting in someone with no approved invite is the whitelist + box's job — that action says so out loud. + + Lives in core so the admin surface and the /api/me badge count answer from ONE definition; a + button that appears and a number that counts must never disagree about who is stuck. + """ + if user.is_demo or user.is_suspended or user.is_active: + return False + return is_allowed(db, user.email) + + +def count_pending_activations(db: Session) -> int: + """How many sign-ups are waiting on an admin click (see can_activate). Drives the admin badge. + + Deliberately NOT "everyone who cannot sign in": an account that has simply not confirmed its + address yet is waiting on its owner, not on the admin, and badging that would nag about something + the admin did not cause. It is still listed on the admin screen — with a re-send — because the + mail may never have arrived. + """ + rows = db.execute( + select(User).where( + User.is_demo.is_(False), + User.is_suspended.is_(False), + User.is_active.is_(False), + ) + ).scalars() + return sum(1 for u in rows if is_allowed(db, u.email)) + + def find_user_by_email(db: Session, email: str) -> User | None: """The account someone means when they type an address at sign-in, password-reset or verify-resend. None when there isn't exactly one answer. diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index 220327ca..bf867d76 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -6,7 +6,7 @@ from datetime import datetime, timezone from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException from pydantic import BaseModel -from sqlalchemy import select +from sqlalchemy import or_, select from sqlalchemy.orm import Session from app import audit @@ -14,8 +14,10 @@ from app import email as email_mod from app.audit import AuditAction from app.auth import ( admin_user, + can_activate, count_admins, get_or_create_demo_user, + is_allowed, operator_contact, purge_user, ) @@ -27,6 +29,8 @@ from app.models import ( Invite, User, ) +from app.routes._common import get_or_404 +from app.routes.auth import send_verification_link from app.utils import canonical_email, iso, valid_email router = APIRouter(prefix="/api/admin", tags=["admin"]) @@ -414,6 +418,133 @@ def admin_delete_user( # --- Demo account: email whitelist + state reset ------------------------------------- +# --- Stuck sign-ups: the admin lifeline (E9.5 S5, roadmap UX-25) --------------------------------- + + +def _pending_access_rows(db: Session) -> list[User]: + """Accounts that have signed up but still cannot sign in, newest first. + + Two different dead ends share this list, because from the admin's chair they look the same — a + person who registered and is still outside: + · `is_active=False` — approved BEFORE they signed up, so nothing ever landed in the + Access-requests queue and its Approve button doesn't apply (`_decide` only takes a pending + row). This is roadmap UX-25, and it is why the list exists. + · `email_verified=False` — the confirmation mail hasn't been acted on. Not the admin's to + decide, but theirs to help with: on 2026-09-03 the first mail to a fresh Gmail landed in + Spam, so "send it again" is a real move. + A suspended account is deliberately excluded: it cannot sign in either, but that is a decision + already made, not a person stuck waiting on one. + """ + return list( + db.execute( + select(User) + .where( + User.is_demo.is_(False), + User.is_suspended.is_(False), + or_(User.is_active.is_(False), User.email_verified.is_(False)), + ) + .order_by(User.created_at.desc()) + ) + .scalars() + .all() + ) + + +@router.get("/pending-access") +def list_pending_access(_: User = Depends(admin_user), db: Session = Depends(get_db)) -> list[dict]: + """The sign-ups that are still outside, with which gate is shut for each. `can_activate` cannot + be derived on the client — it needs the invite/allow-list lookup — so it is answered here. + + One `is_allowed` query per row rather than a batched re-implementation of it: the row count is + the admin's own backlog of stuck sign-ups (single digits on any real instance), and a second, + subtly different copy of the "may this address sign in" rule is exactly the drift this codebase + keeps getting bitten by.""" + rows = [(u, can_activate(db, u)) for u in _pending_access_rows(db)] + # An account that is inactive, confirmed and merely waiting on a still-pending invite is ALREADY + # an actionable row in the Access-requests queue above, with Approve on it. Listing it here too + # would make the admin choose between two buttons for one person. + return [ + _serialize_user(u) | {"can_activate": ca} for u, ca in rows if ca or not u.email_verified + ] + + +@router.post("/users/{user_id}/activate") +def activate_user( + user_id: int, + admin: User = Depends(admin_user), + db: Session = Depends(get_db), +) -> dict: + """Let in a registrant whose address was approved before they signed up (roadmap UX-25). + + Admin-side ONLY, and that is the whole design. Two earlier attempts at this were reverted after + review, and both moved authority rather than visibility: auto-activating on registration turns a + standing whitelist entry into a bearer grant for whoever registers the address first, and + reopening the approved invite to `pending` lets an anonymous sign-up REVOKE a standing approval. + This endpoint changes nothing about what an unauthenticated caller can cause — the admin could + already do exactly this by re-entering the address in the whitelist box (`add_invite` → + `_activate_user_by_email`). What was missing was any way to know they had to.""" + target = get_or_404(db, User, user_id, detail="No such user") + if target.is_demo: + raise HTTPException(status_code=400, detail="The demo account can't be activated.") + if target.is_active: + return _serialize_user(target) # idempotent: a double click is not an error + if not is_allowed(db, target.email): + raise HTTPException( + status_code=409, + detail={"code": "activate", "reason": "not_allowed", "email": target.email}, + ) + audit.record( + db, + admin.id, + AuditAction.USER_ACTIVATE, + target_type="user", + target_id=target.id, + summary=target.email, + before={"is_active": False}, + after={"is_active": True}, + ) + target.is_active = True + db.commit() + return _serialize_user(target) + + +@router.post("/users/{user_id}/resend-verification") +def resend_user_verification( + user_id: int, + admin: User = Depends(admin_user), + db: Session = Depends(get_db), +) -> dict: + """Re-send an account's confirmation link from the admin side. + + The public /auth/verify/resend answers uniformly and reveals nothing, which is right for an + anonymous caller but leaves the admin unable to tell a delivered mail from a dropped one. Here + the caller is already authenticated as an admin, so this one says plainly what happened. + + Not rate-limited: an admin is not a threat model to enumerate against, and the deliberate silence + of the public endpoint is exactly what this exists to escape.""" + target = get_or_404(db, User, user_id, detail="No such user") + if target.is_demo or target.email_verified: + raise HTTPException( + status_code=409, + detail={"code": "resend", "reason": "not_applicable", "email": target.email}, + ) + if not email_mod.email_enabled(db): + # 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) + audit.record( + db, + admin.id, + AuditAction.USER_VERIFY_RESEND, + target_type="user", + target_id=target.id, + summary=target.email, + ) + db.commit() + return {"ok": True, "email": target.email} + + def _serialize_demo(row: DemoWhitelist) -> dict: return { "id": row.id, diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index a78dfb15..1f8a743d 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -764,9 +764,7 @@ def _register_account(email: str, password: str) -> None: # Commit unconditionally; it is a no-op when upsert already committed the same transaction. db.commit() if email_ok: - raw = _issue_token(db, user, "verify", VERIFY_TTL) - # Fragment (#), not a query token: keeps the token out of proxy/access logs + Referer (SB3). - email_mod.send_verify_email(email, f"{_app_base()}/#verify={raw}") + send_verification_link(db, user) admins = admin_notify_emails(db) # Fired unconditionally, unlike the other two callers which gate on a fresh invite row: for # an address the admin already approved, nothing lands in the Access-requests queue, so this @@ -777,6 +775,21 @@ def _register_account(email: str, password: str) -> None: email_mod.send_admin_new_request(admins, email) +def send_verification_link(db: Session, user: User) -> None: + """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 + address is the one its owner chose to be reachable at, and a `+tag` there is usually deliberate. + The token rides the URL FRAGMENT (#) so it can never reach a proxy/access log or a Referer (SB3). + + 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. + """ + raw = _issue_token(db, user, "verify", VERIFY_TTL) + email_mod.send_verify_email(user.email, f"{_app_base()}/#verify={raw}") + + @router.post("/verify") def verify_email_confirm(body: TokenIn, db: Session = Depends(get_db)) -> dict: """Confirm an email-verification token the SPA read from the URL fragment (SB3). Uniform 400 @@ -832,11 +845,7 @@ def _resend_verify_if_eligible(email: str) -> None: with SessionLocal() as db: user = find_user_by_email(db, email) if user is not None and not user.email_verified and not user.is_demo: - raw = _issue_token(db, user, "verify", VERIFY_TTL) - # To the address ON THE ACCOUNT, not the one just typed: the account's address is the one - # its owner chose to be reachable at, and a `+tag` there is usually deliberate (it tells - # them which site leaked, if one ever does). Same mailbox either way for a Gmail alias. - email_mod.send_verify_email(user.email, f"{_app_base()}/#verify={raw}") + send_verification_link(db, user) def _login_refused(status: int, reason: str, user: User | None = None, **extra) -> HTTPException: diff --git a/backend/app/routes/me.py b/backend/app/routes/me.py index ea7fd425..6de8d72d 100644 --- a/backend/app/routes/me.py +++ b/backend/app/routes/me.py @@ -5,6 +5,7 @@ from sqlalchemy.orm import Session from app.auth import ( count_admins, + count_pending_activations, current_user, google_enabled, has_read_scope, @@ -99,6 +100,7 @@ def get_me( if user is None: return {"authenticated": False} pending_invites = 0 + pending_activations = 0 # Admin-only, because it drives an operator warning: email is fail-soft, so a half-filled SMTP # config costs verification links, password resets and access-request notices with nothing # visible anywhere. Surfacing it here puts the warning on the screens where onboarding happens. @@ -108,6 +110,10 @@ def get_me( db.scalar(select(func.count()).select_from(Invite).where(Invite.status == "pending")) or 0 ) + # Counted separately from pending_invites rather than folded into it: the two are different + # facts (a request awaiting a decision vs a decided address whose sign-up never got let in), + # and quietly widening one number would make an existing field mean something new. + pending_activations = count_pending_activations(db) email_configured = email_mod.email_enabled(db) # this request's session; no second connect return { "authenticated": True, @@ -133,6 +139,9 @@ def get_me( "can_read": has_read_scope(user), "can_write": has_write_scope(user), "pending_invites": pending_invites, + # Sign-ups an admin still has to let in (roadmap UX-25): approved BEFORE they registered, so + # nothing ever reached the Access-requests queue and the badge used to read zero. + "pending_activations": pending_activations, "email_configured": email_configured, "preferences": user.preferences or {}, } diff --git a/backend/tests/test_admin_pending_access.py b/backend/tests/test_admin_pending_access.py new file mode 100644 index 00000000..615b33be --- /dev/null +++ b/backend/tests/test_admin_pending_access.py @@ -0,0 +1,186 @@ +"""The admin lifeline for a sign-up that is stuck outside (E9.5 S5, roadmap UX-25), DB-lane. + +Approve an address BEFORE the person registers and their sign-up produces no queue row at all — +`_decide` only takes a still-pending invite — so the Access-requests screen stayed empty while they +sat locked out, and the badge read zero. The only working move was re-typing the address into the +whitelist box, which nobody would guess. + +Two earlier attempts at this were reverted after review, and both moved AUTHORITY rather than +visibility: auto-activating on registration, and reopening the approved invite to pending. So the +tests here pin the boundary as much as the feature — an admin click, gated on an address that is +already allowed, and nothing an unauthenticated caller can reach. +""" + +from sqlalchemy import select + +from app.models import AuditLog, AuthToken, Invite, User + +_PW = "correct horse battery" + + +def _pending_access(client): + r = client.get("/api/admin/pending-access") + assert r.status_code == 200 + return {u["email"]: u for u in r.json()} + + +def _stuck_registrant(db, make_user, admin): + """The UX-25 shape: the admin approved the address first, the account arrived afterwards.""" + db.add(Invite(email="later@example.com", status="approved", decided_by=admin.email)) + db.commit() + return make_user(email="later@example.com", password=_PW, is_active=False) + + +def test_a_pre_approved_registrant_is_listed_and_offers_activation(db, make_user, as_user): + admin = make_user(email="admin@example.com", role="admin") + _stuck_registrant(db, make_user, admin) + + row = _pending_access(as_user(admin))["later@example.com"] + + assert row["is_active"] is False + assert ( + row["can_activate"] is True + ) # the address is already allowed — the click just finishes it + + +def test_an_ordinary_registrant_awaiting_a_decision_is_not_listed_twice(db, make_user, as_user): + # They already have a pending invite with Approve on it in the queue above. Two buttons for one + # person is worse than one. + admin = make_user(email="admin@example.com", role="admin") + db.add(Invite(email="normal@example.com", status="pending")) + db.commit() + make_user(email="normal@example.com", password=_PW, is_active=False) + + assert "normal@example.com" not in _pending_access(as_user(admin)) + + +def test_activating_lets_them_in_and_is_recorded(db, make_user, as_user): + admin = make_user(email="admin@example.com", role="admin") + stuck = _stuck_registrant(db, make_user, admin) + + r = as_user(admin).post(f"/api/admin/users/{stuck.id}/activate") + + assert r.status_code == 200 and r.json()["is_active"] is True + db.expire_all() + assert db.get(User, stuck.id).is_active is True + actions = db.execute(select(AuditLog.action)).scalars().all() + assert ( + "user_activate" in actions + ) # the admin acted on someone's access; it is not a silent read + + +def test_activating_an_address_that_is_not_allowed_is_refused(db, make_user, as_user): + # The guard that keeps this button from manufacturing an inconsistent account: password sign-in + # only checks is_active and would let them in, while their Google sign-in would be turned away + # and silently reopened as a fresh access request. Whitelisting is the move that says "let them + # in"; this one only finishes a decision already made. + admin = make_user(email="admin@example.com", role="admin") + nobody = make_user(email="nobody@example.com", password=_PW, is_active=False) + + r = as_user(admin).post(f"/api/admin/users/{nobody.id}/activate") + + assert r.status_code == 409 + assert r.json()["detail"]["reason"] == "not_allowed" + db.expire_all() + assert db.get(User, nobody.id).is_active is False + + +def test_activating_twice_is_not_an_error(db, make_user, as_user): + admin = make_user(email="admin@example.com", role="admin") + stuck = _stuck_registrant(db, make_user, admin) + client = as_user(admin) + + assert client.post(f"/api/admin/users/{stuck.id}/activate").status_code == 200 + assert client.post(f"/api/admin/users/{stuck.id}/activate").status_code == 200 + + +def test_a_suspended_account_is_not_offered_as_stuck(db, make_user, as_user): + # It cannot sign in either, but that is a decision already made — not a person waiting on one. + admin = make_user(email="admin@example.com", role="admin") + db.add(Invite(email="banned@example.com", status="approved", decided_by=admin.email)) + db.commit() + make_user(email="banned@example.com", password=_PW, is_active=False, is_suspended=True) + + assert "banned@example.com" not in _pending_access(as_user(admin)) + + +# --- the confirmation resend --------------------------------------------------------------------- + + +def test_an_unconfirmed_account_is_listed_and_can_be_sent_a_fresh_link( + db, make_user, as_user, monkeypatch +): + from app.routes import admin as admin_routes + from app.routes import auth as auth_routes + + 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, link: sent.append((to, link)) + ) + 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 + # not the admin's to decide, but the mail may simply never have arrived (2026-09-03: the first + # message to a fresh Gmail landed in Spam), so it is theirs to help with. + u = make_user(email="unconfirmed@example.com", password=_PW, email_verified=False) + + assert "unconfirmed@example.com" in _pending_access(as_user(admin)) + r = as_user(admin).post(f"/api/admin/users/{u.id}/resend-verification") + + assert r.status_code == 200 and r.json()["email"] == "unconfirmed@example.com" + assert [to for to, _ in sent] == ["unconfirmed@example.com"] + tokens = db.execute(select(AuthToken).where(AuthToken.user_id == u.id)).scalars().all() + assert [t.kind for t in tokens] == ["verify"] + + +def test_the_resend_says_so_when_the_instance_cannot_send_mail(db, make_user, as_user, monkeypatch): + from app.routes import admin as admin_routes + + monkeypatch.setattr(admin_routes.email_mod, "email_enabled", lambda db=None: False) + admin = make_user(email="admin@example.com", role="admin") + u = make_user(email="unconfirmed@example.com", password=_PW, email_verified=False) + + r = as_user(admin).post(f"/api/admin/users/{u.id}/resend-verification") + + # Mail is fail-soft everywhere else, which is exactly what left the 0.78.5 sign-ups with nothing + # to fall back on. An admin pressing a button gets the failure, not a success toast. + assert r.status_code == 409 and r.json()["detail"]["reason"] == "email_off" + + +def test_resending_to_an_already_confirmed_account_is_refused(db, make_user, as_user): + admin = make_user(email="admin@example.com", role="admin") + u = make_user(email="fine@example.com", password=_PW) + + r = as_user(admin).post(f"/api/admin/users/{u.id}/resend-verification") + + assert r.status_code == 409 and r.json()["detail"]["reason"] == "not_applicable" + + +# --- the badge ----------------------------------------------------------------------------------- + + +def test_the_badge_counts_stuck_sign_ups_but_not_ones_the_queue_already_shows( + db, make_user, client +): + # Signed in for real, not via the as_user override: /api/me resolves its user through + # optional_current_user, which that override does not cover (see test_me_email_flag). + admin = make_user(email="admin@example.com", password=_PW, role="admin") + _stuck_registrant(db, make_user, admin) # approved first → counts + db.add(Invite(email="normal@example.com", status="pending")) # already in the queue → doesn't + db.commit() + make_user(email="normal@example.com", password=_PW, is_active=False) + make_user(email="unconfirmed@example.com", password=_PW, email_verified=False) # not ours + + assert ( + client.post( + "/auth/password-login", json={"email": admin.email, "password": _PW} + ).status_code + == 200 + ) + me = client.get("/api/me").json() + assert me["authenticated"] is True + + # An unconfirmed account waits on its OWNER, not on the admin — listing it is help, badging it + # would be nagging about something the admin did not cause. + assert me["pending_activations"] == 1 + assert me["pending_invites"] == 1 # unchanged meaning: still just the undecided requests diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 5d4975d2..4534ef55 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -247,7 +247,10 @@ export default function App() { // Keep a single, current nudge: drop any prior one (persisted copies reload as dismissed // history, so they'd otherwise pile up one-per-reload) before re-issuing the live notice. removeByMetaKind("access-requests"); - const pending = meQuery.data.pending_invites ?? 0; + // Both kinds of "someone is waiting on you": a request needing a decision, and a sign-up whose + // address was approved before they registered. The second used to produce NO signal at all — + // nothing lands in the queue for it — which is how the 2026-09-03 registrant went unnoticed. + const pending = (meQuery.data.pending_invites ?? 0) + (meQuery.data.pending_activations ?? 0); if (pending > 0) { notify({ level: "warning", @@ -266,7 +269,7 @@ export default function App() { }); } // eslint-disable-next-line react-hooks/exhaustive-deps - }, [meQuery.data?.role, meQuery.data?.pending_invites]); + }, [meQuery.data?.role, meQuery.data?.pending_invites, meQuery.data?.pending_activations]); // Language is edited outside the Settings page (sidebar/login), so it stays instant. function changeLanguage(code: LangCode) { diff --git a/frontend/src/components/AdminUsers.tsx b/frontend/src/components/AdminUsers.tsx index 1cbb78c7..0b42f9ff 100644 --- a/frontend/src/components/AdminUsers.tsx +++ b/frontend/src/components/AdminUsers.tsx @@ -2,8 +2,26 @@ import { useState } from "react"; import { qk } from "../lib/queryKeys"; import { useTranslation } from "react-i18next"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; -import { Ban, Check, RotateCcw, Shield, Trash2, UserPlus, X } from "lucide-react"; -import { api, HttpError, type AdminUserRow, type Invite, type Me } from "../lib/api"; +import { + Ban, + Check, + Clock, + MailWarning, + RotateCcw, + Send, + Shield, + Trash2, + UserPlus, + X, +} from "lucide-react"; +import { + api, + HttpError, + type AdminUserRow, + type Invite, + type Me, + type PendingAccessRow, +} from "../lib/api"; import { notify } from "../lib/notifications"; import { ADMIN_USERS_TAB_KEY } from "../lib/adminUsersTab"; import Tooltip from "./Tooltip"; @@ -27,7 +45,14 @@ export default function AdminUsers() { const [tab, setTab] = usePersistedTab(ADMIN_USERS_TAB_KEY, "roles"); const tabs = [ { id: "roles", label: t("users.tabs.roles") }, - { id: "access", label: t("users.tabs.access"), badge: me.pending_invites }, + // Both kinds of "waiting on the admin": a request needing a decision, and a sign-up whose + // address was approved BEFORE they registered — the second never reaches the invite queue, + // so the badge used to read zero while somebody sat locked out (roadmap UX-25). + { + id: "access", + label: t("users.tabs.access"), + badge: me.pending_invites + me.pending_activations, + }, { id: "demo", label: t("users.tabs.demo") }, ]; const active = tabs.some((x) => x.id === tab) ? tab : "roles"; @@ -41,7 +66,12 @@ export default function AdminUsers() {
{active === "roles" && } - {active === "access" && } + {active === "access" && ( + <> + + + + )} {active === "demo" && }
@@ -261,6 +291,166 @@ function UsersRoles({ me }: { me: Me }) { ); } +// --- Sign-ups that are still outside (roadmap UX-25) -------------------------------------------- +// +// The gap this closes: approve an address BEFORE the person registers and their sign-up produces no +// queue row at all — `_decide` only takes a pending invite — so the Access-requests screen above was +// empty while they sat locked out. The only working move was re-typing the address into the +// whitelist box, which nobody would guess. This section makes the state visible and the fix one +// click, WITHOUT changing who may cause what: the button is admin-authenticated and explicit. Two +// earlier attempts moved authority instead (auto-activating on registration, reopening the approved +// invite) and were reverted after review. +function PendingAccess() { + const { t } = useTranslation(); + const qc = useQueryClient(); + const rows = useQuery({ + queryKey: qk.adminPendingAccess(), + queryFn: () => api.adminPendingAccess(), + }); + // Activating changes /api/me's badge count too, so refresh both or the number lies until reload. + const refresh = () => { + qc.invalidateQueries({ queryKey: qk.adminPendingAccess() }); + qc.invalidateQueries({ queryKey: qk.me() }); + qc.invalidateQueries({ queryKey: qk.adminUsers() }); + }; + + const activate = useMutation({ + mutationFn: (u: PendingAccessRow) => api.activateUser(u.id), + onSuccess: (_r, u) => { + refresh(); + notify({ level: "success", message: t("users.pendingAccess.activated", { email: u.email }) }); + }, + onError: () => notify({ level: "error", message: t("users.pendingAccess.activateFailed") }), + }); + + const resend = useMutation({ + mutationFn: (u: PendingAccessRow) => api.resendUserVerification(u.id), + onSuccess: (_r, u) => { + refresh(); + notify({ level: "success", message: t("users.pendingAccess.resent", { email: u.email }) }); + }, + // The public resend is deliberately silent; this one is not, so say WHY it failed — an instance + // with no working SMTP would otherwise show a success toast for a mail that was never sent. + onError: (err) => + notify({ + level: "error", + message: + err instanceof HttpError && isEmailOff(err) + ? t("users.pendingAccess.resendEmailOff") + : t("users.pendingAccess.resendFailed"), + }), + }); + + const list = rows.data ?? []; + const busy = activate.isPending || resend.isPending; + if (rows.isSuccess && list.length === 0) return null; // nothing stuck: no empty box to explain + + return ( +
+

{t("users.pendingAccess.body")}

+ {rows.isError && !rows.data ? ( + rows.refetch()}> + {t("common.loadError")} + + ) : rows.isLoading && !rows.data ? ( + + ) : ( +
+ {list.map((u) => ( + activate.mutate(u)} + onResend={() => resend.mutate(u)} + /> + ))} +
+ )} +
+ ); +} + +/** Did the resend fail because this instance has no working SMTP? */ +function isEmailOff(err: HttpError): boolean { + const d = err.detailData as { code?: string; reason?: string } | null | undefined; + return d?.code === "resend" && d.reason === "email_off"; +} + +// One stuck sign-up. The row names WHICH gate is shut rather than just offering buttons — the same +// reasoning as the sign-in card's blocked panel, from the admin's side: an admin about to let someone +// in should see that the address has not been confirmed yet, because on a no-SMTP instance that is +// the difference between the owner and whoever registered the address first (roadmap SEC-15). +function PendingAccessRowView({ + row, + busy, + onActivate, + onResend, +}: { + row: PendingAccessRow; + busy: boolean; + onActivate: () => void; + onResend: () => void; +}) { + const { t } = useTranslation(); + return ( +
+
+
{row.email}
+
+ {row.created_at && ( + + {t("users.pendingAccess.signedUp", { + time: new Date(row.created_at).toLocaleString(), + })} + + )} + {row.email_verified ? ( + + + {t("users.pendingAccess.emailConfirmed")} + + ) : ( + + + {t("users.pendingAccess.emailUnconfirmed")} + + )} + {!row.is_active && ( + + + {t("users.pendingAccess.inactive")} + + )} +
+
+ {!row.email_verified && ( + + + + )} + {row.can_activate && ( + + + + )} +
+ ); +} + // --- Access requests (the Invite whitelist) — moved verbatim from Settings → Account ------- function AdminInvites() { diff --git a/frontend/src/components/adminInvites.dom.test.tsx b/frontend/src/components/adminInvites.dom.test.tsx index 1e3cda1a..ebc7c9e8 100644 --- a/frontend/src/components/adminInvites.dom.test.tsx +++ b/frontend/src/components/adminInvites.dom.test.tsx @@ -37,7 +37,13 @@ vi.mock("../lib/notifications", async (importOriginal) => { return { ...actual, notify: (...a: unknown[]) => notifyMock(...a) }; }); -const meMock = { pending_invites: 0, email_configured: true, id: 1, role: "admin" }; +const meMock = { + pending_invites: 0, + pending_activations: 0, + email_configured: true, + id: 1, + role: "admin", +}; vi.mock("../lib/useMe", () => ({ useMe: () => meMock })); // The tab strip is rendered into an app-level toolbar slot that does not exist here, so pin the @@ -57,6 +63,9 @@ vi.mock("../lib/api", async (importOriginal) => { ...actual.api, adminInvites: () => listMock(), deleteInvite: (id: number) => deleteMock(id), + // The access tab also renders the stuck-sign-ups section (E9.5 S5); nothing stuck here, so it + // renders nothing and this file stays about the delete affordance. + adminPendingAccess: () => Promise.resolve([]), }, }; }); diff --git a/frontend/src/components/adminPendingAccess.dom.test.tsx b/frontend/src/components/adminPendingAccess.dom.test.tsx new file mode 100644 index 00000000..bfedaa10 --- /dev/null +++ b/frontend/src/components/adminPendingAccess.dom.test.tsx @@ -0,0 +1,186 @@ +// @vitest-environment jsdom +// +// The admin lifeline for a sign-up that is stuck outside (E9.5 S5, roadmap UX-25). Approve an address +// BEFORE the person registers and their sign-up produces no queue row at all, so the Access-requests +// screen was empty while they sat locked out and the only working move — re-typing the address into +// the whitelist box — was undiscoverable. These pin what the section shows and, just as importantly, +// what it does NOT show: the section is a discovery surface, and a person the queue above already +// covers must not appear twice. +// +// i18n is a key-passthrough so assertions read stable keys; the API and the notify singleton are +// stubbed so the panel can be driven without a server. +import { describe, it, expect, afterEach, beforeEach, vi } from "vitest"; +import { render, screen, cleanup, waitFor, fireEvent } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ + t: (key: string, vars?: Record) => + vars?.email ? `${key}:${String(vars.email)}` : key, + i18n: { language: "en" }, + }), + }; +}); + +vi.mock("./ConfirmProvider", () => ({ + useConfirm: () => vi.fn(async () => true), + ConfirmProvider: ({ children }: { children: React.ReactNode }) => children, +})); + +const notifyMock = vi.fn(); +vi.mock("../lib/notifications", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, notify: (...a: unknown[]) => notifyMock(...a) }; +}); + +const meMock = { + pending_invites: 0, + pending_activations: 0, + email_configured: true, + id: 1, + role: "admin", +}; +vi.mock("../lib/useMe", () => ({ useMe: () => meMock })); + +// The tab strip renders into an app-level toolbar slot that does not exist here; pin the tab. +vi.mock("./Tabs", () => ({ + default: () => null, + usePersistedTab: () => ["access", () => {}], +})); + +const pendingMock = vi.fn(); +const activateMock = vi.fn(); +const resendMock = vi.fn(); +vi.mock("../lib/api", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + api: { + ...actual.api, + adminInvites: () => Promise.resolve([]), + adminPendingAccess: () => pendingMock(), + activateUser: (id: number) => activateMock(id), + resendUserVerification: (id: number) => resendMock(id), + }, + }; +}); + +import AdminUsers from "./AdminUsers"; + +/** A row as /api/admin/pending-access serves it. */ +const row = (over: Record = {}) => ({ + id: 7, + email: "later@example.com", + display_name: null, + role: "user", + is_active: false, + is_suspended: false, + email_verified: true, + is_demo: false, + has_password: true, + has_google: false, + created_at: "2026-09-01T10:00:00Z", + can_activate: true, + ...over, +}); + +function mount() { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + render( + + + , + ); +} + +beforeEach(() => { + pendingMock.mockReset(); + activateMock.mockReset().mockResolvedValue({}); + resendMock.mockReset().mockResolvedValue({ ok: true, email: "later@example.com" }); + notifyMock.mockReset(); +}); +afterEach(cleanup); + +describe("the stuck sign-ups section", () => { + it("offers Activate for a registrant whose address was already approved", async () => { + pendingMock.mockResolvedValue([row()]); + mount(); + + const button = await screen.findByText("users.pendingAccess.activate"); + fireEvent.click(button); + + await waitFor(() => expect(activateMock).toHaveBeenCalledWith(7)); + expect(notifyMock).toHaveBeenCalledWith( + expect.objectContaining({ message: "users.pendingAccess.activated:later@example.com" }), + ); + }); + + it("shows no Activate when the address is not allowed, since that would be a different decision", async () => { + // Activating an address with no approved invite makes an account that signs in with a password + // but is turned away by Google — the whitelist box is the honest move for that. + pendingMock.mockResolvedValue([row({ can_activate: false, email_verified: false })]); + mount(); + + await screen.findByText("users.pendingAccess.emailUnconfirmed"); + expect(screen.queryByText("users.pendingAccess.activate")).toBeNull(); + }); + + it("names which gate is shut, so an admin sees an unconfirmed address before letting it in", async () => { + pendingMock.mockResolvedValue([row({ email_verified: false })]); + mount(); + + // On a no-SMTP instance this is the difference between the owner and whoever registered the + // address first (roadmap SEC-15), so it belongs on the row, not in a tooltip. + expect(await screen.findByText("users.pendingAccess.emailUnconfirmed")).toBeTruthy(); + expect(screen.getByText("users.pendingAccess.inactive")).toBeTruthy(); + }); + + it("re-sends the confirmation link for an unconfirmed account", async () => { + pendingMock.mockResolvedValue([row({ email_verified: false })]); + mount(); + + fireEvent.click(await screen.findByLabelText("users.pendingAccess.resend")); + + await waitFor(() => expect(resendMock).toHaveBeenCalledWith(7)); + }); + + it("offers no resend once the address is confirmed", async () => { + pendingMock.mockResolvedValue([row()]); + mount(); + + await screen.findByText("users.pendingAccess.activate"); + expect(screen.queryByLabelText("users.pendingAccess.resend")).toBeNull(); + }); + + it("says the mail was never sent when the instance has no SMTP", async () => { + const { HttpError } = await import("../lib/api"); + pendingMock.mockResolvedValue([row({ email_verified: false })]); + resendMock.mockRejectedValue( + new HttpError(409, "ignored", { code: "resend", reason: "email_off" }), + ); + mount(); + + fireEvent.click(await screen.findByLabelText("users.pendingAccess.resend")); + + // Mail is fail-soft everywhere else, which is what left the 0.78.5 sign-ups with nothing to fall + // back on. A button the admin pressed must not answer with a success toast. + await waitFor(() => + expect(notifyMock).toHaveBeenCalledWith( + expect.objectContaining({ message: "users.pendingAccess.resendEmailOff" }), + ), + ); + }); + + it("renders nothing at all when nobody is stuck", async () => { + pendingMock.mockResolvedValue([]); + mount(); + + // The section withdraws itself rather than leaving an empty box to explain — so wait for it to + // GO, not merely for the request to have been made. + await waitFor(() => expect(screen.queryByText("users.pendingAccess.title")).toBeNull()); + expect(pendingMock).toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/i18n/locales/en/auditActions.json b/frontend/src/i18n/locales/en/auditActions.json index 09eaa33b..942ca170 100644 --- a/frontend/src/i18n/locales/en/auditActions.json +++ b/frontend/src/i18n/locales/en/auditActions.json @@ -3,6 +3,8 @@ "user_suspend": "User suspended", "user_unsuspend": "User reinstated", "user_delete": "User deleted", + "user_activate": "Account activated", + "user_verify_resend": "Confirmation link re-sent", "invite_approve": "Access request approved", "invite_deny": "Access request denied", "invite_add": "Email whitelisted", diff --git a/frontend/src/i18n/locales/en/users.json b/frontend/src/i18n/locales/en/users.json index a5f955d6..103f8c37 100644 --- a/frontend/src/i18n/locales/en/users.json +++ b/frontend/src/i18n/locales/en/users.json @@ -4,6 +4,23 @@ "access": "Access requests", "demo": "Demo" }, + "pendingAccess": { + "title": "Signed up, but still outside", + "body": "These people created an account that can't sign in yet. If you approved their address before they registered, no request reached the queue above — let them in here.", + "signedUp": "Signed up {{time}}", + "emailConfirmed": "email confirmed", + "emailUnconfirmed": "email not confirmed", + "inactive": "waiting for activation", + "activate": "Activate", + "activateHint": "Let this account in. Their address is already approved — this only finishes it.", + "activated": "{{email}} can sign in now.", + "activateFailed": "Couldn't activate that account.", + "resend": "Send the confirmation link again", + "resendHint": "Send the confirmation link again, to the address on the account.", + "resent": "Confirmation link sent to {{email}}.", + "resendFailed": "Couldn't send the confirmation link.", + "resendEmailOff": "No mail was sent: this instance has no working SMTP setup. Check Configuration → Email." + }, "roles": { "title": "Users & roles", "intro": "Everyone who can sign in. Promote a user to admin, or back to a regular user.", diff --git a/frontend/src/i18n/locales/hu/auditActions.json b/frontend/src/i18n/locales/hu/auditActions.json index d57ab117..f6c8eb62 100644 --- a/frontend/src/i18n/locales/hu/auditActions.json +++ b/frontend/src/i18n/locales/hu/auditActions.json @@ -3,6 +3,8 @@ "user_suspend": "Felhasználó felfüggesztve", "user_unsuspend": "Felhasználó visszaállítva", "user_delete": "Felhasználó törölve", + "user_activate": "Fiók aktiválva", + "user_verify_resend": "Megerősítő link újraküldve", "invite_approve": "Hozzáférési kérelem jóváhagyva", "invite_deny": "Hozzáférési kérelem elutasítva", "invite_add": "Email fehérlistázva", diff --git a/frontend/src/i18n/locales/hu/users.json b/frontend/src/i18n/locales/hu/users.json index d65554d6..207fbf87 100644 --- a/frontend/src/i18n/locales/hu/users.json +++ b/frontend/src/i18n/locales/hu/users.json @@ -4,6 +4,23 @@ "access": "Hozzáférési kérések", "demo": "Demo" }, + "pendingAccess": { + "title": "Regisztrált, de még kívül van", + "body": "Ezek a fiókok léteznek, de még nem tudnak belépni. Ha a címüket a regisztrációjuk előtt hagytad jóvá, a fenti sorba semmi nem került — itt engedheted be őket.", + "signedUp": "Regisztrált: {{time}}", + "emailConfirmed": "cím megerősítve", + "emailUnconfirmed": "cím nincs megerősítve", + "inactive": "aktiválásra vár", + "activate": "Aktiválás", + "activateHint": "Beengedi ezt a fiókot. A címe már jóvá van hagyva — ez csak befejezi.", + "activated": "{{email}} mostantól be tud lépni.", + "activateFailed": "Nem sikerült aktiválni a fiókot.", + "resend": "Megerősítő link újraküldése", + "resendHint": "Újraküldi a megerősítő linket, a fiókon szereplő címre.", + "resent": "Megerősítő link elküldve ide: {{email}}.", + "resendFailed": "Nem sikerült elküldeni a megerősítő linket.", + "resendEmailOff": "Nem ment ki levél: ezen a példányon nincs működő SMTP. Nézd meg a Beállítások → E-mail részt." + }, "roles": { "title": "Felhasználók és szerepek", "intro": "Mindenki, aki be tud lépni. Léptess egy felhasználót adminná, vagy vissza sima felhasználóvá.", diff --git a/frontend/src/lib/api/account.ts b/frontend/src/lib/api/account.ts index 865d62eb..89473148 100644 --- a/frontend/src/lib/api/account.ts +++ b/frontend/src/lib/api/account.ts @@ -20,6 +20,11 @@ export interface Me { can_read: boolean; can_write: boolean; pending_invites: number; + /** Admin-only: sign-ups an admin still has to let in — approved BEFORE they registered, so nothing + * ever reached the Access-requests queue and its badge read zero (roadmap UX-25). Counted apart + * from pending_invites because they are different facts, and widening one number silently would + * change what an existing field means. */ + pending_activations: number; /** Admin-only: false when SMTP is incomplete, so every outbound message is silently skipped. */ email_configured: boolean; preferences: Record; diff --git a/frontend/src/lib/api/admin.ts b/frontend/src/lib/api/admin.ts index 2c10790a..c2282737 100644 --- a/frontend/src/lib/api/admin.ts +++ b/frontend/src/lib/api/admin.ts @@ -27,6 +27,16 @@ export interface Invite { still_allowed_by_env: boolean; } +/** A sign-up that is still outside, as the admin screen shows it. Same shape as a user row plus the + * one thing the client cannot work out for itself. */ +export interface PendingAccessRow extends AdminUserRow { + /** The address is already allowed and the account is merely inactive, so Activate finishes a + * decision the admin has already made. False when there is no approved invite behind it — + * activating then would make an account that signs in with a password but is turned away by + * Google, so the whitelist box is the honest move instead. */ + can_activate: boolean; +} + // Admin Configuration page: one DB-overridable setting (registry entry on the backend). export interface ConfigItem { key: string; @@ -169,6 +179,16 @@ export const adminApi = { resetDemo: (): Promise<{ reset: boolean; playlists_seeded: number }> => req("/api/admin/demo/reset", { method: "POST" }), // --- access invites --- + // Sign-ups that registered but still cannot get in — the admin lifeline for roadmap UX-25. + adminPendingAccess: (): Promise => req("/api/admin/pending-access"), + // Let one in. Admin-only and explicit: it changes discoverability, not authority — the same thing + // was always possible by re-entering the address in the whitelist box. + activateUser: (id: number): Promise => + req(`/api/admin/users/${id}/activate`, { method: "POST" }, { quiet: true }), + // Re-send someone's confirmation link. Unlike the public resend this one says what happened, + // because the caller is already an authenticated admin. + resendUserVerification: (id: number): Promise<{ ok: boolean; email: string }> => + req(`/api/admin/users/${id}/resend-verification`, { method: "POST" }, { quiet: true }), adminInvites: (status?: string): Promise => req(`/api/admin/invites${status ? `?status=${status}` : ""}`), approveInvite: (id: number) => req(`/api/admin/invites/${id}/approve`, { method: "POST" }), diff --git a/frontend/src/lib/queryKeys.ts b/frontend/src/lib/queryKeys.ts index 58adb1d7..fa635c03 100644 --- a/frontend/src/lib/queryKeys.ts +++ b/frontend/src/lib/queryKeys.ts @@ -86,6 +86,7 @@ export const qk = { // --- admin --- adminUsers: () => ["admin-users"] as const, adminInvites: () => ["admin-invites"] as const, + adminPendingAccess: () => ["admin-pending-access"] as const, adminConfig: () => ["admin-config"] as const, adminAudit: () => ["admin-audit"] as const, adminStorage: () => ["admin-storage"] as const, From a76bab9b340e058940783f3dd425732d52217b87 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 4 Sep 2026 04:05:28 +0200 Subject: [PATCH 10/16] =?UTF-8?q?feat(auth):=20one=20mail,=20one=20click?= =?UTF-8?q?=20=E2=80=94=20the=20approval=20mail=20carries=20the=20confirma?= =?UTF-8?q?tion=20link=20(E9.5=20S6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The funnel's common ordering made a person open TWO messages: one saying their access was approved, another asking them to confirm their address. The approval mail now branches on what they actually still have to do — carry the confirmation link, point at sign-in, or say a confirmation mail follows once they register — so the usual case is one mail and one click. A mail that guesses wrong is worse than a plain one: telling someone to sign in while their address is unconfirmed contradicts the screen that will refuse them. That embedded link lives 7 days rather than the usual 24 hours. Nobody sits waiting for an approval — it can easily be read on Monday after a weekend, and an expired link spends the one-mail promise and puts them back on two. The risk is small: the token only ever confirms an address; it signs nobody in and sets no password. The most likely root cause of the 2026-09-03 report that a confirmation mail "never arrived" is here too, and it was never a delivery failure: re-registering an address that already had an unconfirmed account returned in SILENCE — same cheerful "check your inbox", no mail sent at all. It now re-sends the link. Nothing new leaks: the response and its timing are unchanged, and the public verify/resend already does exactly this for the same input. Whitelisting stays silent, and that is a decision, not an omission. The operator opens a door as a possibility — usually already agreed with the person — and a row can sit approved for weeks with nobody registering; a mail fired automatically there would be nudging someone who never asked. Telling them is its own act on its own button, `invited_at` records it so a second click is a decision rather than an accident, and the button only appears where nobody is behind the address yet: "the door is open, come in" is meaningless to someone already inside. Two things UAT caught that no test would have. Every fresh registration appeared in BOTH admin lists at once, because the stuck-sign-ups filter keyed on "unconfirmed" — the rule is now "the address may sign in, and this account still can't", which leaves an undecided request with the queue that owns it. And the invitation button was offered on every approved row, half of which are existing users. Also: the confirmation mail finally says what its link's rules are (once, 24 hours) — the reset mail always has, while this one left the reader to discover both by meeting a dead link. The card's "didn't get the email?" moves up under the form, and the three waiting messages name the spam folder, which is where a first message to a fresh recipient sometimes lands. Registration now records the language the sign-up page was in. Nothing reads it yet — the bilingual templates are S7 — but it is the only moment that choice exists anywhere the server can see, and a preference nobody recorded cannot be backfilled later. UAT on localdev: re-registering re-sent the link (one token became two, one account); approval issued a 7-day token and moved the account into the stuck list with only a re-send; whitelisting mailed nobody; the invitation recorded itself and the row then warned that sending again would write twice. Test data removed. --- .../versions/0081_invite_invited_at.py | 34 +++ backend/app/audit.py | 3 + backend/app/auth.py | 20 +- backend/app/email.py | 67 ++++- backend/app/models.py | 8 + backend/app/routes/admin.py | 97 ++++++- backend/app/routes/auth.py | 50 +++- backend/tests/test_admin_pending_access.py | 25 +- backend/tests/test_auth_session.py | 4 +- backend/tests/test_welcome_mail.py | 249 ++++++++++++++++++ frontend/src/components/AdminUsers.tsx | 56 ++++ frontend/src/components/Welcome.tsx | 20 +- .../src/components/adminInvites.dom.test.tsx | 63 +++++ .../src/i18n/locales/en/auditActions.json | 3 +- frontend/src/i18n/locales/en/settings.json | 6 + frontend/src/i18n/locales/en/welcome.json | 6 +- .../src/i18n/locales/hu/auditActions.json | 3 +- frontend/src/i18n/locales/hu/settings.json | 6 + frontend/src/i18n/locales/hu/welcome.json | 6 +- frontend/src/lib/api/admin.ts | 8 + 20 files changed, 687 insertions(+), 47 deletions(-) create mode 100644 backend/alembic/versions/0081_invite_invited_at.py create mode 100644 backend/tests/test_welcome_mail.py diff --git a/backend/alembic/versions/0081_invite_invited_at.py b/backend/alembic/versions/0081_invite_invited_at.py new file mode 100644 index 00000000..c9230499 --- /dev/null +++ b/backend/alembic/versions/0081_invite_invited_at.py @@ -0,0 +1,34 @@ +"""invites.invited_at — when the admin actually told this person the door was open + +Revision ID: 0081_invite_invited_at +Revises: 0080_google_email +Create Date: 2026-09-05 + +Whitelisting an address stays SILENT by design: the operator is opening a door as a possibility, +usually after agreeing it with the person, and a row can sit approved for weeks without anyone +registering. Telling them is therefore a separate, deliberate act — a send button on the row. + +Which raises the one question the admin cannot otherwise answer: "did I already write to them?" +Without a record the only failure mode is writing to a real person twice, which is exactly the +nudging the silent-by-default design exists to avoid. The audit log holds the event, but not where +the decision is made. + +NULL means "never invited from here", which is true of every existing row: no backfill is possible +or wanted, since the historic approval mails answered a REQUEST rather than inviting anyone. +""" + +import sqlalchemy as sa +from alembic import op + +revision: str = "0081_invite_invited_at" +down_revision: str | None = "0080_google_email" +branch_labels: str | None = None +depends_on: str | None = None + + +def upgrade() -> None: + op.add_column("invites", sa.Column("invited_at", sa.DateTime(timezone=True), nullable=True)) + + +def downgrade() -> None: + op.drop_column("invites", "invited_at") diff --git a/backend/app/audit.py b/backend/app/audit.py index 95d24515..dae44955 100644 --- a/backend/app/audit.py +++ b/backend/app/audit.py @@ -35,6 +35,9 @@ class AuditAction: INVITE_DENY = "invite_deny" INVITE_ADD = "invite_add" INVITE_DELETE = "invite_delete" + # E9.5 S6: the admin chose to tell someone their address is whitelisted. Adding the address is + # silent, so this is the act that actually reaches a person — and the one worth a record. + INVITE_SEND = "invite_send" DEMO_ADD = "demo_add" DEMO_REMOVE = "demo_remove" DEMO_RESET = "demo_reset" diff --git a/backend/app/auth.py b/backend/app/auth.py index 17a4e9e1..f2283b1f 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -121,14 +121,20 @@ def count_pending_activations(db: Session) -> int: the admin did not cause. It is still listed on the admin screen — with a re-send — because the mail may never have arrived. """ - rows = db.execute( - select(User).where( - User.is_demo.is_(False), - User.is_suspended.is_(False), - User.is_active.is_(False), + # Narrowed in SQL, then decided by can_activate itself — so the number the badge shows and the + # button the admin sees cannot drift apart by one of them re-deriving the rule. + rows = ( + db.execute( + select(User).where( + User.is_demo.is_(False), + User.is_suspended.is_(False), + User.is_active.is_(False), + ) ) - ).scalars() - return sum(1 for u in rows if is_allowed(db, u.email)) + .scalars() + .all() + ) + return sum(1 for u in rows if can_activate(db, u)) def find_user_by_email(db: Session, email: str) -> User | None: diff --git a/backend/app/email.py b/backend/app/email.py index b0e9d451..18062b3e 100644 --- a/backend/app/email.py +++ b/backend/app/email.py @@ -184,19 +184,74 @@ def _send(to: list[str], subject: str, body: str, reply_to: str | None = None) - return False -def send_access_approved(email: str) -> bool: +def _next_step(verify_link: str | None, has_account: bool, hours: int) -> str: + """What this person actually has to do next, in the one paragraph that carries the whole flow. + + Three states, because a mail that guesses wrong sends someone looking for a link that isn't + coming — or worse, tells them to sign in when their address is still unconfirmed and sign-in will + refuse them (E9.5 S4 named that state on screen; the mail must not contradict it). + + The link branch is the point of the C option: in the common ordering — approve first, the person + signs up later, or signs up and waits — they would otherwise have to find and open TWO separate + messages. One mail, one click. + """ + if verify_link: + return ( + "You already started an account, so there is just one step left. Confirm your email " + "address and you're in:\n\n" + f"{verify_link}\n\n" + f"The link works once and expires in {hours} hours. If it does, you can ask for a fresh " + "one from the sign-in page." + ) + if has_account: + return f"Your account is ready — just sign in:\n\n{settings.app_base}" + return ( + "Create your account whenever you feel like it:\n\n" + f"{settings.app_base}\n\n" + "We'll send you a confirmation link to finish, and then you're in." + ) + + +def send_access_approved( + email: str, verify_link: str | None = None, has_account: bool = False, hours: int = 24 +) -> bool: + """Answer someone who ASKED for access. Sent automatically on approval, because a person who + made a request is owed a reply — unlike the admin-initiated invitation below.""" admin = _admin_contact() body = ( "Hi,\n\n" "Your request to use Siftlode has been approved — welcome aboard.\n\n" - "Open Siftlode and sign in, and your YouTube subscriptions feed will be ready:\n\n" - f"{settings.app_base}\n\n" + f"{_next_step(verify_link, has_account, hours)}\n\n" "If you weren't expecting this, you can ignore the message.\n\n" "— Siftlode" + (f"\n\nQuestions? Just reply to this email ({admin})." if admin else "") ) return _send([email], "You're in — your Siftlode access is approved", body, reply_to=admin) +def send_access_invitation( + email: str, verify_link: str | None = None, has_account: bool = False, hours: int = 24 +) -> bool: + """Invite someone the admin let in WITHOUT them asking — sent only when the admin presses send. + + Whitelisting an address is opening a door, not knocking on one: the operator has usually already + agreed it with the person, and an address can sit approved for weeks without anyone registering. + So adding to the whitelist stays silent and this is a separate, deliberate act. That is also why + the closing line offers no deadline — nudging is the one thing this mail must not do. + """ + admin = _admin_contact() + body = ( + "Hi,\n\n" + "Someone has opened up Siftlode for you — a quiet, self-hosted reader for your YouTube " + "subscriptions. Every upload from the channels you follow lands in one feed you can " + "actually filter and sort, with no algorithm deciding what you see, and no Shorts or " + "livestreams unless you want them.\n\n" + f"{_next_step(verify_link, has_account, hours)}\n\n" + "No rush — the door stays open.\n\n" + "— Siftlode" + (f"\n\nQuestions? Just reply to this email ({admin})." if admin else "") + ) + return _send([email], "You're invited to Siftlode", body, reply_to=admin) + + def send_access_denied(email: str) -> bool: admin = _admin_contact() body = ( @@ -277,11 +332,15 @@ def send_admin_new_request(admins: list[str], requester: str) -> bool: return _send(admins, f"Siftlode access request from {requester}", body, reply_to=requester) -def send_verify_email(to: str, link: str) -> bool: +def send_verify_email(to: str, link: str, hours: int = 24) -> bool: body = ( "Hi,\n\n" "Confirm your email to finish creating your Siftlode account:\n\n" f"{link}\n\n" + # Say 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. + f"The link works once and expires in {hours} hours. 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" ) diff --git a/backend/app/models.py b/backend/app/models.py index c9601577..2b3e5fca 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -167,6 +167,14 @@ class Invite(Base, CanonicalEmailMixin): ) decided_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) decided_by: Mapped[str | None] = mapped_column(String(320)) # admin email + # When the admin last SENT this person an invitation from the whitelist row. Adding an + # address stays silent (it opens a door, it does not knock), so telling them is a separate + # act — and this is what stops it happening twice to a real person. + invited_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) + # When the admin last SENT this person an invitation from the whitelist row. Adding an address + # stays silent — it opens a door, it does not knock — so telling them is a separate, deliberate + # act, and this is what stops it happening twice to a real person. + invited_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) class DemoWhitelist(Base, TimestampMixin): diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index bf867d76..da807d67 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -16,6 +16,7 @@ from app.auth import ( admin_user, can_activate, count_admins, + find_user_by_email, get_or_create_demo_user, is_allowed, operator_contact, @@ -30,7 +31,7 @@ from app.models import ( User, ) from app.routes._common import get_or_404 -from app.routes.auth import send_verification_link +from app.routes.auth import APPROVAL_VERIFY_TTL, issue_verification_link, send_verification_link from app.utils import canonical_email, iso, valid_email router = APIRouter(prefix="/api/admin", tags=["admin"]) @@ -71,6 +72,10 @@ def _serialize(inv: Invite, *, has_account: bool = False) -> dict: "requested_at": iso(inv.requested_at), "decided_at": iso(inv.decided_at), "decided_by": inv.decided_by, + # When (if ever) the admin sent this person an invitation. Adding an address is silent, so + # without this the admin has no way to answer "did I already write to them?" — and the only + # failure mode is writing to a real person twice. + "invited_at": iso(inv.invited_at), # Drives the delete affordance: an entry backed by an account is not removable here. "has_account": has_account, # …and whether removing it would actually take access away, or leave the env allow-list @@ -96,6 +101,30 @@ def list_invites( return [_serialize(i, has_account=canonical_email(i.email) in accounts) for i in rows] +def _welcome_mail_parts(db: Session, email: str) -> tuple[str | None, bool, int]: + """What a welcome or invitation mail should tell this address to do next: a confirmation link (if + there is an unconfirmed account behind it), whether an account exists at all, and how long the + link lasts. + + This is the C option's whole mechanic. In the common ordering — the address is approved and the + person signs up either side of that — they would otherwise have to find and open TWO messages: + one saying they are in, another asking them to confirm. Carrying the link inside the mail that is + already going out makes it one mail and one click. + + Resolved through `find_user_by_email`, so an account registered under another spelling of the + same Gmail mailbox is still recognised — and, deliberately, so several candidates resolve to + "no single answer" rather than a guess: an invitation that carried the wrong account's link would + be worse than one that simply pointed at the sign-up page. + """ + hours = int(APPROVAL_VERIFY_TTL.total_seconds() // 3600) + user = find_user_by_email(db, email) + if user is None or user.is_demo: + return None, False, hours + if user.email_verified or not email_mod.email_enabled(db): + return None, True, hours + return issue_verification_link(db, user, APPROVAL_VERIFY_TTL), True, hours + + 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). @@ -159,7 +188,11 @@ def approve_invite( ) -> dict: inv = _decide(db, invite_id, admin, approved=True) _activate_user_by_email(db, inv.email) - background.add_task(email_mod.send_access_approved, inv.email) + # Computed here, not in the background task: issuing the token needs this request's session, and + # the task must not outlive it. Approval answers a request the person MADE, so unlike the + # invitation below it is sent automatically — a reply is owed, not a nudge. + link, has_account, hours = _welcome_mail_parts(db, inv.email) + background.add_task(email_mod.send_access_approved, inv.email, link, has_account, hours) return _serialize(inv, has_account=bool(_accounts_for(db, [inv.email]))) @@ -211,6 +244,49 @@ def add_invite( return _serialize(inv, has_account=bool(_accounts_for(db, [email]))) +@router.post("/invites/{invite_id}/invite") +def send_invitation( + invite_id: int, + background: BackgroundTasks, + admin: User = Depends(admin_user), + db: Session = Depends(get_db), +) -> dict: + """Tell someone their address is whitelisted — only when the admin asks for it. + + Whitelisting stays SILENT on purpose. The operator opens a door as a possibility, usually having + already agreed it with the person, and an address can sit approved for weeks with nobody + registering; a mail fired automatically would be nudging someone who never asked to be nudged. + So the send is its own act, on its own button, and `invited_at` records it so the admin can see + they have already written rather than writing twice. + + Only an APPROVED row can be invited: there is nothing to invite someone to while their request is + undecided, and inviting a denied address would contradict the decision.""" + inv = get_or_404(db, Invite, invite_id, detail="No such invite") + if inv.status != "approved": + raise HTTPException( + status_code=409, + detail={"code": "invite_send", "reason": "not_approved", "email": inv.email}, + ) + if not email_mod.email_enabled(db): + # Same reasoning as the admin-side verification resend: mail is fail-soft everywhere else, + # 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"}) + link, has_account, hours = _welcome_mail_parts(db, inv.email) + inv.invited_at = datetime.now(timezone.utc) + audit.record( + db, + admin.id, + AuditAction.INVITE_SEND, + target_type="invite", + target_id=inv.id, + summary=inv.email, + ) + db.commit() + background.add_task(email_mod.send_access_invitation, inv.email, link, has_account, hours) + return _serialize(inv, has_account=has_account) + + @router.delete("/invites/{invite_id}") def delete_invite( invite_id: int, @@ -459,13 +535,16 @@ def list_pending_access(_: User = Depends(admin_user), db: Session = Depends(get the admin's own backlog of stuck sign-ups (single digits on any real instance), and a second, subtly different copy of the "may this address sign in" rule is exactly the drift this codebase keeps getting bitten by.""" - rows = [(u, can_activate(db, u)) for u in _pending_access_rows(db)] - # An account that is inactive, confirmed and merely waiting on a still-pending invite is ALREADY - # an actionable row in the Access-requests queue above, with Approve on it. Listing it here too - # would make the admin choose between two buttons for one person. - return [ - _serialize_user(u) | {"can_activate": ca} for u, ca in rows if ca or not u.email_verified - ] + out = [] + for u in _pending_access_rows(db): + # The whole rule in one line: this address MAY sign in, and this account still can't. An + # undecided request fails it (nothing is allowed yet) and so does a denied one — both belong + # to the queue above, which owns that decision. Without this, every fresh registration + # appeared in both lists at once, which UAT caught on 2026-09-05. + if not is_allowed(db, u.email): + continue + out.append(_serialize_user(u) | {"can_activate": can_activate(db, u)}) + return out @router.post("/users/{user_id}/activate") diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 1f8a743d..85234d01 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -43,6 +43,12 @@ from app.security import encrypt, hash_password, hash_token, verify_password from app.utils import canonical_email, valid_email VERIFY_TTL = timedelta(hours=24) +# A confirmation link that rides an APPROVAL or an invitation lives longer than one the person is +# waiting for. Nobody sits refreshing their inbox for an approval — it can easily be read on Monday +# after a weekend, and if it has expired by then the one-mail-one-click promise is spent and they are +# back to two messages. The risk is small: this token only ever confirms an address. It signs nobody +# in and sets no password. +APPROVAL_VERIFY_TTL = timedelta(days=7) RESET_TTL = timedelta(hours=1) _DECOY_PASSWORD_HASH = hash_password(secrets.token_urlsafe(16)) @@ -254,6 +260,12 @@ class EmailIn(BaseModel): class EmailPasswordIn(BaseModel): email: str | None = None password: str | None = None + # The language the sign-up page was in. Captured at registration because it is the ONLY moment + # the choice exists anywhere the server can see: it lives in the browser until an account does, + # and a preference nobody recorded cannot be backfilled later (the lesson `google_email` taught + # in E9.5 S3 — those rows still read NULL until each user signs in again). Nothing reads it yet; + # the bilingual mail templates are E9.5 S7. + lang: str | None = None class TokenIn(BaseModel): @@ -718,11 +730,11 @@ def register( # session). The create path hashes the password + writes several rows + schedules emails; doing it # inline would make a NEW email respond measurably slower than an already-registered one (which # skips all that) — a timing oracle for enumeration. Off-path, any valid email responds the same. - background.add_task(_register_account, email, password) + background.add_task(_register_account, email, password, body.lang) return {"status": "ok"} -def _register_account(email: str, password: str) -> None: +def _register_account(email: str, password: str, lang: str | None = None) -> None: """Background worker for /register: create the pending account (+ verification email + admin notice) for a genuinely new email; no-op for an already-registered one. Runs after the response with its own DB session, so the request's timing never reveals whether the email already exists.""" @@ -734,7 +746,20 @@ def _register_account(email: str, password: str) -> None: select(User).where(User.email_canonical == canonical_email(email)) ).first() if existing is not None: - return # already registered — nothing to do (the uniform "ok" was already returned) + # 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": + # nothing was ever SENT. The person re-registered, got the same cheerful "check your + # inbox", and waited for a message that did not exist. + # + # Re-sending the link instead leaks nothing new. The response and its timing are + # unchanged (uniform "ok", decided off the response path), the mail goes to the address + # 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) + 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 # the flow still completes on a no-SMTP self-host. See email.email_enabled(). @@ -744,6 +769,8 @@ def _register_account(email: str, password: str) -> None: password_hash=hash_password(password), 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 {}, ) db.add(user) try: @@ -775,6 +802,16 @@ def _register_account(email: str, password: str) -> None: email_mod.send_admin_new_request(admins, email) +def issue_verification_link(db: Session, user: User, ttl: timedelta | None = None) -> str: + """A fresh confirmation link for `user`, returned rather than sent — for a mail that carries the + link inside a larger message (the branching approval/invitation mails, E9.5 S6). + + Same token machinery and the same fragment (SB3) as the standalone confirmation mail, so the two + kinds of link cannot drift apart in shape or in how they are consumed.""" + raw = _issue_token(db, user, "verify", ttl or VERIFY_TTL) + return f"{_app_base()}/#verify={raw}" + + def send_verification_link(db: Session, user: User) -> None: """Issue a fresh verification token for `user` and mail them the link. @@ -786,8 +823,11 @@ def send_verification_link(db: Session, user: User) -> None: (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. """ - raw = _issue_token(db, user, "verify", VERIFY_TTL) - email_mod.send_verify_email(user.email, f"{_app_base()}/#verify={raw}") + email_mod.send_verify_email( + user.email, + issue_verification_link(db, user), + hours=int(VERIFY_TTL.total_seconds() // 3600), + ) @router.post("/verify") diff --git a/backend/tests/test_admin_pending_access.py b/backend/tests/test_admin_pending_access.py index 615b33be..3f286657 100644 --- a/backend/tests/test_admin_pending_access.py +++ b/backend/tests/test_admin_pending_access.py @@ -44,8 +44,9 @@ def test_a_pre_approved_registrant_is_listed_and_offers_activation(db, make_user def test_an_ordinary_registrant_awaiting_a_decision_is_not_listed_twice(db, make_user, as_user): - # They already have a pending invite with Approve on it in the queue above. Two buttons for one - # person is worse than one. + # They already have a pending invite with Approve on it in the queue above, and their address is + # not allowed YET — the decision is the queue's to own. Caught in UAT on 2026-09-05: an earlier + # filter keyed on "unconfirmed" put every fresh registration in both lists at once. admin = make_user(email="admin@example.com", role="admin") db.add(Invite(email="normal@example.com", status="pending")) db.commit() @@ -54,6 +55,17 @@ def test_an_ordinary_registrant_awaiting_a_decision_is_not_listed_twice(db, make assert "normal@example.com" not in _pending_access(as_user(admin)) +def test_a_fresh_registration_stays_in_the_queue_and_out_of_this_list(db, make_user, as_user): + # The realistic version of the case above: a brand-new sign-up is inactive AND unconfirmed. It + # still belongs only to the queue — the admin's next act is the decision, not a re-send. + admin = make_user(email="admin@example.com", role="admin") + db.add(Invite(email="fresh@example.com", status="pending")) + db.commit() + make_user(email="fresh@example.com", password=_PW, is_active=False, email_verified=False) + + assert "fresh@example.com" not in _pending_access(as_user(admin)) + + def test_activating_lets_them_in_and_is_recorded(db, make_user, as_user): admin = make_user(email="admin@example.com", role="admin") stuck = _stuck_registrant(db, make_user, admin) @@ -116,12 +128,15 @@ 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, link: sent.append((to, link)) + auth_routes.email_mod, "send_verify_email", lambda to, link, **kw: sent.append((to, link)) ) 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 - # not the admin's to decide, but the mail may simply never have arrived (2026-09-03: the first - # message to a fresh Gmail landed in Spam), so it is theirs to help with. + # not the admin's to decide, but the mail may simply never have arrived, so it is theirs to help + # with. The approved invite is what puts them on this screen at all: the list is "the address may + # sign in, this account still can't", so an undecided request stays with the queue that owns it. + db.add(Invite(email="unconfirmed@example.com", status="approved", decided_by=admin.email)) + db.commit() u = make_user(email="unconfirmed@example.com", password=_PW, email_verified=False) assert "unconfirmed@example.com" in _pending_access(as_user(admin)) diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 905709eb..68a42286 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -320,7 +320,9 @@ def test_registration_of_a_pre_approved_email_persists_the_account_with_smtp(db, sent: list[str] = [] monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: True) - monkeypatch.setattr(auth_mod.email_mod, "send_verify_email", lambda to, link: sent.append(to)) + monkeypatch.setattr( + auth_mod.email_mod, "send_verify_email", lambda to, link, **kw: sent.append(to) + ) monkeypatch.setattr(auth_mod.email_mod, "send_admin_new_request", lambda admins, to: True) db.add(Invite(email="rita@example.com", status="approved", decided_by="admin@example.com")) db.commit() diff --git a/backend/tests/test_welcome_mail.py b/backend/tests/test_welcome_mail.py new file mode 100644 index 00000000..ca5af938 --- /dev/null +++ b/backend/tests/test_welcome_mail.py @@ -0,0 +1,249 @@ +"""One mail, one click — the branching welcome/invitation mail (E9.5 S6), DB-lane. + +The funnel's common ordering makes a person open TWO messages: one saying their access is approved, +another asking them to confirm their address. The C option carries the confirmation link inside the +mail that is already going out, so the usual case is one mail and one click. + +Also here: the silent no-op that is the most likely explanation for the 2026-09-03 report that a +confirmation mail "never arrived". Re-registering an address that already had an unconfirmed account +sent nothing at all, while answering with the same cheerful "check your inbox". +""" + +from datetime import datetime, timedelta, timezone + +from sqlalchemy import select + +from app.models import AuditLog, AuthToken, Invite, User + +_PW = "correct horse battery" + + +def _capture(monkeypatch, name): + """Record the arguments a given email_mod.send_* was called with.""" + from app import email as email_mod + + calls: list[tuple] = [] + monkeypatch.setattr(email_mod, name, lambda *a, **kw: calls.append((a, kw)) or True) + return calls + + +def _smtp_on(monkeypatch): + from app import email as email_mod + + monkeypatch.setattr(email_mod, "email_enabled", lambda db=None: True) + + +def _approve(client, invite_id): + r = client.post(f"/api/admin/invites/{invite_id}/approve") + assert r.status_code == 200 + return r + + +# --- the approval mail branches ------------------------------------------------------------------ + + +def test_an_unconfirmed_account_gets_the_confirmation_link_in_the_approval_mail( + db, make_user, as_user, monkeypatch +): + _smtp_on(monkeypatch) + sent = _capture(monkeypatch, "send_access_approved") + admin = make_user(email="admin@example.com", role="admin") + inv = Invite(email="waiting@example.com", status="pending") + db.add(inv) + db.commit() + make_user(email="waiting@example.com", password=_PW, email_verified=False, is_active=False) + + _approve(as_user(admin), inv.id) + + (email, link, has_account, hours), _ = sent[0] + assert email == "waiting@example.com" and has_account is True + assert link and "#verify=" in link + # The whole point: this is the ONLY message they now have to open. + assert hours == 24 * 7 + + +def test_the_link_in_that_mail_actually_confirms_the_address(db, make_user, as_user, monkeypatch): + _smtp_on(monkeypatch) + sent = _capture(monkeypatch, "send_access_approved") + admin = make_user(email="admin@example.com", role="admin") + inv = Invite(email="waiting@example.com", status="pending") + db.add(inv) + db.commit() + user = make_user( + email="waiting@example.com", password=_PW, email_verified=False, is_active=False + ) + _approve(as_user(admin), inv.id) + token = sent[0][0][1].split("#verify=")[1] + + r = as_user(admin).post("/auth/verify", json={"token": token}) + + assert r.status_code == 200 + db.expire_all() + assert db.get(User, user.id).email_verified is True + + +def test_that_link_outlives_a_weekend(db, make_user, as_user, monkeypatch): + # An approval is not a mail anyone sits waiting for; the standard 24h would routinely expire + # before it is read, and the one-mail promise with it. + _smtp_on(monkeypatch) + _capture(monkeypatch, "send_access_approved") + admin = make_user(email="admin@example.com", role="admin") + inv = Invite(email="waiting@example.com", status="pending") + db.add(inv) + db.commit() + user = make_user( + email="waiting@example.com", password=_PW, email_verified=False, is_active=False + ) + + _approve(as_user(admin), inv.id) + + tok = db.execute(select(AuthToken).where(AuthToken.user_id == user.id)).scalars().all()[-1] + assert tok.expires_at > datetime.now(timezone.utc) + timedelta(days=6) + + +def test_an_already_confirmed_account_is_just_told_to_sign_in(db, make_user, as_user, monkeypatch): + _smtp_on(monkeypatch) + sent = _capture(monkeypatch, "send_access_approved") + admin = make_user(email="admin@example.com", role="admin") + inv = Invite(email="ready@example.com", status="pending") + db.add(inv) + db.commit() + make_user(email="ready@example.com", password=_PW, is_active=False) + + _approve(as_user(admin), inv.id) + + (_, link, has_account, _), _ = sent[0] + assert link is None and has_account is True # no link to offer, and none needed + + +def test_someone_with_no_account_yet_is_told_a_second_mail_is_coming( + db, make_user, as_user, monkeypatch +): + _smtp_on(monkeypatch) + sent = _capture(monkeypatch, "send_access_approved") + admin = make_user(email="admin@example.com", role="admin") + inv = Invite(email="stranger@example.com", status="pending") + db.add(inv) + db.commit() + + _approve(as_user(admin), inv.id) + + (_, link, has_account, _), _ = sent[0] + # Nothing to link to yet. Saying a confirmation mail follows is what makes its ABSENCE + # noticeable — the failure that went unnoticed on 2026-09-03. + assert link is None and has_account is False + + +# --- the deliberate invitation -------------------------------------------------------------------- + + +def test_adding_an_address_to_the_whitelist_mails_nobody(db, make_user, as_user, monkeypatch): + _smtp_on(monkeypatch) + approved = _capture(monkeypatch, "send_access_approved") + invited = _capture(monkeypatch, "send_access_invitation") + admin = make_user(email="admin@example.com", role="admin") + + r = as_user(admin).post("/api/admin/invites", json={"email": "maybe@example.com"}) + + # Whitelisting opens a door as a possibility — often already agreed in person — and an address + # can sit approved for weeks. A mail fired automatically here would be nudging someone who never + # asked to be nudged. + assert r.status_code == 200 and r.json()["invited_at"] is None + assert approved == [] and invited == [] + + +def test_the_admin_can_then_invite_them_on_purpose(db, make_user, as_user, monkeypatch): + _smtp_on(monkeypatch) + invited = _capture(monkeypatch, "send_access_invitation") + admin = make_user(email="admin@example.com", role="admin") + inv = Invite(email="maybe@example.com", status="approved", decided_by=admin.email) + db.add(inv) + db.commit() + + r = as_user(admin).post(f"/api/admin/invites/{inv.id}/invite") + + assert r.status_code == 200 and r.json()["invited_at"] is not None + assert invited[0][0][0] == "maybe@example.com" + # Recorded on the row, so "did I already write to them?" has an answer where the decision is + # made — the wrong answer means mailing a real person twice. + db.expire_all() + assert db.get(Invite, inv.id).invited_at is not None + assert "invite_send" in db.execute(select(AuditLog.action)).scalars().all() + + +def test_an_undecided_request_cannot_be_invited(db, make_user, as_user, monkeypatch): + _smtp_on(monkeypatch) + admin = make_user(email="admin@example.com", role="admin") + inv = Invite(email="asking@example.com", status="pending") + db.add(inv) + db.commit() + + r = as_user(admin).post(f"/api/admin/invites/{inv.id}/invite") + + assert r.status_code == 409 and r.json()["detail"]["reason"] == "not_approved" + + +def test_the_invitation_says_so_when_the_instance_cannot_send_mail( + db, make_user, as_user, monkeypatch +): + from app import email as email_mod + + monkeypatch.setattr(email_mod, "email_enabled", lambda db=None: False) + admin = make_user(email="admin@example.com", role="admin") + inv = Invite(email="maybe@example.com", status="approved", decided_by=admin.email) + db.add(inv) + db.commit() + + r = as_user(admin).post(f"/api/admin/invites/{inv.id}/invite") + + assert r.status_code == 409 and r.json()["detail"]["reason"] == "email_off" + db.expire_all() + assert db.get(Invite, inv.id).invited_at is None # nothing sent, nothing recorded + + +# --- the silent register no-op -------------------------------------------------------------------- + + +def test_registering_again_re_sends_the_link_instead_of_sending_nothing(db, client, monkeypatch): + _smtp_on(monkeypatch) + sent = _capture(monkeypatch, "send_verify_email") + assert ( + client.post( + "/auth/register", json={"email": "twice@example.com", "password": _PW} + ).status_code + == 200 + ) + assert len(sent) == 1 + + r = client.post("/auth/register", json={"email": "twice@example.com", "password": _PW}) + + # The old behaviour was to return in silence: same cheerful "check your inbox", no mail. That is + # the most likely explanation for the 2026-09-03 confirmation mail that "never arrived" — none + # was ever sent. The response stays uniform; only the dead end is gone. + assert r.status_code == 200 and r.json() == {"status": "ok"} + assert len(sent) == 2 + assert sent[1][0][0] == "twice@example.com" + + +def test_registering_again_does_not_create_a_second_account(db, client, monkeypatch): + _smtp_on(monkeypatch) + _capture(monkeypatch, "send_verify_email") + for _ in range(2): + client.post("/auth/register", json={"email": "twice@example.com", "password": _PW}) + + db.expire_all() + rows = db.execute(select(User).where(User.email == "twice@example.com")).scalars().all() + assert len(rows) == 1 + + +def test_a_confirmed_account_still_gets_nothing(db, client, make_user, monkeypatch): + _smtp_on(monkeypatch) + sent = _capture(monkeypatch, "send_verify_email") + make_user(email="done@example.com", password=_PW) # already verified + + r = client.post("/auth/register", json={"email": "done@example.com", "password": _PW}) + + # There is nothing to confirm, so there is nothing to send — and mailing "confirm your address" + # to someone who already did would be a lie. + assert r.status_code == 200 + assert sent == [] diff --git a/frontend/src/components/AdminUsers.tsx b/frontend/src/components/AdminUsers.tsx index 0b42f9ff..d02c0fc1 100644 --- a/frontend/src/components/AdminUsers.tsx +++ b/frontend/src/components/AdminUsers.tsx @@ -7,6 +7,7 @@ import { Check, Clock, MailWarning, + Mails, RotateCcw, Send, Shield, @@ -291,6 +292,12 @@ function UsersRoles({ me }: { me: Me }) { ); } +/** Did the invitation fail because this instance has no working SMTP? */ +function inviteEmailOff(err: HttpError): boolean { + const d = err.detailData as { code?: string; reason?: string } | null | undefined; + return d?.code === "invite_send" && d.reason === "email_off"; +} + // --- Sign-ups that are still outside (roadmap UX-25) -------------------------------------------- // // The gap this closes: approve an address BEFORE the person registers and their sign-up produces no @@ -528,6 +535,26 @@ function AdminInvites() { }); if (ok) remove.mutate(i.id); }; + // Sending is its OWN act, never a side effect of approving an address: the operator opens a door + // as a possibility (often already agreed in person), and a mail fired automatically would be + // nudging someone who never asked. `invited_at` comes back on the row so a second click is a + // decision rather than an accident. + const invite = useMutation({ + mutationFn: (i: Invite) => api.sendInvitation(i.id), + onSuccess: (_r, i) => { + refresh(); + notify({ level: "success", message: t("settings.invites.invited", { email: i.email }) }); + }, + onError: (err) => + notify({ + level: "error", + message: + err instanceof HttpError && inviteEmailOff(err) + ? t("settings.invites.inviteEmailOff") + : t("settings.invites.inviteFailed"), + }), + }); + const add = useMutation({ mutationFn: (email: string) => api.addInvite(email), onSuccess: (_d, email) => { @@ -604,6 +631,35 @@ function AdminInvites() { {i.status} + {/* Only an approved row with NOBODY behind it yet: this button means "the door is + open, come in", which is meaningless to someone already inside — and half the + whitelist is existing users. An account that exists but is still stuck has its + own surface above, with a re-send. The label carries the answer to "did I already + write to them?", the question whose wrong answer means mailing a person twice. */} + {i.status === "approved" && !i.has_account && ( + + + + )} {/* An entry with an account behind it stays read-only: deleting it would revoke that user's access, since is_allowed reads this row. Delete the user instead. */} {i.has_account ? ( diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx index 203ab53c..dcbef1ca 100644 --- a/frontend/src/components/Welcome.tsx +++ b/frontend/src/components/Welcome.tsx @@ -657,6 +657,18 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { + {/* A confirmation mail can be slow, filtered, or — before E9.5 S6 — never sent at all for a + repeat sign-up. Offered right under the form rather than below the mode switches: it is + the second thing a stuck person needs, after the password they just tried. */} + {mode === "signin" && ( + + )} + {/* Mode switches */}
{mode === "signin" ? ( @@ -695,14 +707,6 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { {mode === "signin" && ( <> - {/* A verification email can be lost or never arrive, so offer a resend path from - sign-in too — not only from the expired-link banner (needs a broken link to show). */} -
{t("welcome.auth.or")} diff --git a/frontend/src/components/adminInvites.dom.test.tsx b/frontend/src/components/adminInvites.dom.test.tsx index ebc7c9e8..71e009ee 100644 --- a/frontend/src/components/adminInvites.dom.test.tsx +++ b/frontend/src/components/adminInvites.dom.test.tsx @@ -55,6 +55,7 @@ vi.mock("./Tabs", () => ({ const listMock = vi.fn(); const deleteMock = vi.fn(); +const inviteMock = vi.fn(); vi.mock("../lib/api", async (importOriginal) => { const actual = await importOriginal(); return { @@ -63,6 +64,7 @@ vi.mock("../lib/api", async (importOriginal) => { ...actual.api, adminInvites: () => listMock(), deleteInvite: (id: number) => deleteMock(id), + sendInvitation: (id: number) => inviteMock(id), // The access tab also renders the stuck-sign-ups section (E9.5 S5); nothing stuck here, so it // renders nothing and this file stays about the delete affordance. adminPendingAccess: () => Promise.resolve([]), @@ -83,6 +85,7 @@ const invite = (over: Partial> = {}) => ({ decided_by: null, has_account: false, still_allowed_by_env: false, + invited_at: null, ...over, }); @@ -100,6 +103,9 @@ beforeEach(() => { notifyMock.mockClear(); deleteMock.mockReset().mockResolvedValue({ deleted: 1, still_allowed_by_env: false }); listMock.mockReset().mockResolvedValue([invite()]); + inviteMock + .mockReset() + .mockResolvedValue(invite({ status: "approved", invited_at: "2026-09-05T09:00:00Z" })); meMock.email_configured = true; }); afterEach(cleanup); @@ -223,3 +229,60 @@ describe("Access requests — the delete affordance", () => { expect(await screen.findByText("settings.invites.emailOff")).toBeTruthy(); }); }); + +// Whitelisting an address is opening a door as a possibility, not knocking on one — the operator has +// usually already agreed it with the person, and a row can sit approved for weeks. So adding stays +// silent and telling them is its own act, on its own button (E9.5 S6). +describe("inviting someone the admin whitelisted", () => { + // The decided rows live behind a
; await the summary rather than the section title, or + // the click races the invite query that populates it. + const open = async () => { + renderPanel(); + fireEvent.click(await screen.findByText(/settings.invites.decided/)); + }; + + it("offers a send button on an approved row", async () => { + listMock.mockResolvedValue([invite({ status: "approved" })]); + await open(); + + fireEvent.click(screen.getByLabelText("settings.invites.inviteLabel:asker@example.com")); + + await waitFor(() => expect(inviteMock).toHaveBeenCalledWith(1)); + expect(notifyMock).toHaveBeenCalledWith( + expect.objectContaining({ message: "settings.invites.invited:asker@example.com" }), + ); + }); + + it("offers none on a denied row, which would contradict the decision", async () => { + listMock.mockResolvedValue([invite({ status: "denied" })]); + await open(); + + expect(screen.queryByLabelText("settings.invites.inviteLabel:asker@example.com")).toBeNull(); + }); + + it("offers none once somebody is already behind that address", async () => { + // "The door is open, come in" is meaningless to someone already inside, and most of a mature + // whitelist is existing users. An account that exists but is stuck has its own row above. + listMock.mockResolvedValue([invite({ status: "approved", has_account: true })]); + await open(); + + expect(screen.queryByLabelText("settings.invites.inviteLabel:asker@example.com")).toBeNull(); + }); + + it("says the mail was never sent when the instance has no SMTP", async () => { + listMock.mockResolvedValue([invite({ status: "approved" })]); + inviteMock.mockRejectedValue( + new HttpError(409, "ignored", { code: "invite_send", reason: "email_off" }), + ); + await open(); + + fireEvent.click(screen.getByLabelText("settings.invites.inviteLabel:asker@example.com")); + + // A button the admin pressed must not answer with a success toast for a mail nobody received. + await waitFor(() => + expect(notifyMock).toHaveBeenCalledWith( + expect.objectContaining({ message: "settings.invites.inviteEmailOff" }), + ), + ); + }); +}); diff --git a/frontend/src/i18n/locales/en/auditActions.json b/frontend/src/i18n/locales/en/auditActions.json index 942ca170..c569c299 100644 --- a/frontend/src/i18n/locales/en/auditActions.json +++ b/frontend/src/i18n/locales/en/auditActions.json @@ -22,5 +22,6 @@ "audit_clear": "Audit log cleared", "download_credential_set": "Download cookies set", "download_credential_delete": "Download cookies removed", - "invite_delete": "Whitelist entry deleted" + "invite_delete": "Whitelist entry deleted", + "invite_send": "Invitation sent" } diff --git a/frontend/src/i18n/locales/en/settings.json b/frontend/src/i18n/locales/en/settings.json index 8bfc564e..797332f2 100644 --- a/frontend/src/i18n/locales/en/settings.json +++ b/frontend/src/i18n/locales/en/settings.json @@ -157,6 +157,12 @@ "approveFailed": "Approve failed", "denyFailed": "Deny failed", "addedToWhitelist": "{{email}} added to the whitelist", + "inviteHint": "Send them an invitation. Adding an address never mails anyone — this does.", + "invitedAt": "Invited {{time}}. Sending again will mail them a second time.", + "inviteLabel": "Send an invitation to {{email}}", + "invited": "Invitation sent to {{email}}.", + "inviteFailed": "Couldn't send that invitation.", + "inviteEmailOff": "No mail was sent: this instance has no working SMTP setup. Check Configuration → Email.", "addFailed": "Couldn't add that email", "requested": "requested {{time}}", "approveHint": "Approve — whitelist this email and email them they're in.", diff --git a/frontend/src/i18n/locales/en/welcome.json b/frontend/src/i18n/locales/en/welcome.json index 15c3686f..0dfecd5b 100644 --- a/frontend/src/i18n/locales/en/welcome.json +++ b/frontend/src/i18n/locales/en/welcome.json @@ -82,8 +82,8 @@ "demoEmail": "Demo email", "demoEnter": "Enter", "demoDenied": "That email isn't enabled for the demo. Ask the admin to add it.", - "registerDone": "Almost there — check your email for a verification link. After you confirm it, an admin approves your account before you can sign in.", - "forgotDone": "If that email has an account, we've sent a reset link. Check your inbox.", + "registerDone": "Almost there — check your email for a confirmation link, and your spam folder if it isn't there in a minute. After you confirm it, an admin approves your account before you can sign in.", + "forgotDone": "If that email has an account, we've sent a reset link. Check your inbox, and your spam folder if it hasn't arrived in a minute.", "resetDone": "Your password has been set — you can sign in now.", "deadLink": { "title": "This link is no longer valid", @@ -96,7 +96,7 @@ "verifyOkResetCta": "Set a new password", "verifyInvalid": "That verification link is invalid or has expired.", "resendCta": "Send a new link", - "resendDone": "If that email has an unverified account, we've sent a new verification link. Check your inbox.", + "resendDone": "If that email has an unconfirmed account, we've sent a new confirmation link. Check your inbox, and your spam folder if it hasn't arrived in a minute.", "oauthCancelled": "Google sign-in was cancelled. You can try again or use email and password.", "accessRequested": "Thanks — we recorded your request. An admin will review it.", "accessRequestedFor": "We've recorded your access request for:", diff --git a/frontend/src/i18n/locales/hu/auditActions.json b/frontend/src/i18n/locales/hu/auditActions.json index f6c8eb62..636e8a53 100644 --- a/frontend/src/i18n/locales/hu/auditActions.json +++ b/frontend/src/i18n/locales/hu/auditActions.json @@ -22,5 +22,6 @@ "audit_clear": "Napló ürítve", "download_credential_set": "Letöltési cookie-k beállítva", "download_credential_delete": "Letöltési cookie-k eltávolítva", - "invite_delete": "Whitelista-bejegyzés törölve" + "invite_delete": "Whitelista-bejegyzés törölve", + "invite_send": "Meghívó elküldve" } diff --git a/frontend/src/i18n/locales/hu/settings.json b/frontend/src/i18n/locales/hu/settings.json index 72adf40e..eab22b25 100644 --- a/frontend/src/i18n/locales/hu/settings.json +++ b/frontend/src/i18n/locales/hu/settings.json @@ -157,6 +157,12 @@ "approveFailed": "A jóváhagyás sikertelen", "denyFailed": "Az elutasítás sikertelen", "addedToWhitelist": "{{email}} hozzáadva a fehérlistához", + "inviteHint": "Küldj neki meghívót. A cím hozzáadása soha nem küld levelet — ez igen.", + "invitedAt": "Meghívva: {{time}}. Az újraküldés másodszor is levelet küld neki.", + "inviteLabel": "Meghívó küldése ide: {{email}}", + "invited": "Meghívó elküldve ide: {{email}}.", + "inviteFailed": "Nem sikerült elküldeni a meghívót.", + "inviteEmailOff": "Nem ment ki levél: ezen a példányon nincs működő SMTP. Nézd meg a Beállítások → E-mail részt.", "addFailed": "Nem sikerült hozzáadni ezt az e-mailt", "requested": "kérve: {{time}}", "approveHint": "Jóváhagyás — fehérlistára teszi ezt az e-mailt és e-mailben értesíti őket, hogy bekerültek.", diff --git a/frontend/src/i18n/locales/hu/welcome.json b/frontend/src/i18n/locales/hu/welcome.json index fd3cd8e1..2bb1f998 100644 --- a/frontend/src/i18n/locales/hu/welcome.json +++ b/frontend/src/i18n/locales/hu/welcome.json @@ -82,8 +82,8 @@ "demoEmail": "Demó e-mail", "demoEnter": "Belépés", "demoDenied": "Ez az e-mail nincs engedélyezve a demóhoz. Kérd az admint, hogy adja hozzá.", - "registerDone": "Majdnem kész — nézd meg a postafiókod a megerősítő linkért. Miután megerősítetted, egy admin jóváhagyja a fiókodat, mielőtt beléphetnél.", - "forgotDone": "Ha ehhez az e-mailhez tartozik fiók, elküldtük a visszaállító linket. Nézd meg a postafiókod.", + "registerDone": "Majdnem kész — nézd meg a postafiókod a megerősítő linkért, és a Spam mappát is, ha egy percen belül nem jön meg. Miután megerősítetted, egy admin jóváhagyja a fiókodat, mielőtt beléphetnél.", + "forgotDone": "Ha ehhez az e-mailhez tartozik fiók, elküldtük a visszaállító linket. Nézd meg a postafiókod, és a Spam mappát is, ha egy percen belül nem érkezik meg.", "resetDone": "A jelszavad be van állítva — most már beléphetsz.", "deadLink": { "title": "Ez a link már nem érvényes", @@ -96,7 +96,7 @@ "verifyOkResetCta": "Új jelszó beállítása", "verifyInvalid": "Ez a megerősítő link érvénytelen vagy lejárt.", "resendCta": "Új link küldése", - "resendDone": "Ha ehhez az e-mailhez tartozik megerősítetlen fiók, elküldtünk egy új megerősítő linket. Nézd meg a postafiókod.", + "resendDone": "Ha ehhez az e-mailhez tartozik megerősítetlen fiók, elküldtünk egy új megerősítő linket. Nézd meg a postafiókod, és a Spam mappát is, ha egy percen belül nem érkezik meg.", "oauthCancelled": "A Google-belépés megszakadt. Próbáld újra, vagy lépj be e-maillel és jelszóval.", "accessRequested": "Köszönjük — rögzítettük a kérésed. Egy admin felülvizsgálja.", "accessRequestedFor": "Rögzítettük a hozzáférési kérelmedet erre a címre:", diff --git a/frontend/src/lib/api/admin.ts b/frontend/src/lib/api/admin.ts index c2282737..a4dfd5a2 100644 --- a/frontend/src/lib/api/admin.ts +++ b/frontend/src/lib/api/admin.ts @@ -25,6 +25,10 @@ export interface Invite { /** The address stays allow-listed by the server's env ALLOWED_EMAILS / ADMIN_EMAILS, so removing * this row would NOT take access away. The delete confirmation says so. */ still_allowed_by_env: boolean; + /** When the admin last invited this person, or null if never. Adding an address to the whitelist + * is deliberately silent — it opens a door rather than knocking — so telling them is a separate + * act, and this is how the admin sees they have already done it. */ + invited_at: string | null; } /** A sign-up that is still outside, as the admin screen shows it. Same shape as a user row plus the @@ -193,6 +197,10 @@ export const adminApi = { req(`/api/admin/invites${status ? `?status=${status}` : ""}`), approveInvite: (id: number) => req(`/api/admin/invites/${id}/approve`, { method: "POST" }), denyInvite: (id: number) => req(`/api/admin/invites/${id}/deny`, { method: "POST" }), + // Tell someone their address is whitelisted. Never automatic: adding the address stays silent, so + // this is the deliberate act that actually reaches a person. + sendInvitation: (id: number): Promise => + req(`/api/admin/invites/${id}/invite`, { method: "POST" }, { quiet: true }), addInvite: (email: string) => req("/api/admin/invites", { method: "POST", body: JSON.stringify({ email }) }), /** `still_allowed_by_env` is true when the address stays allow-listed by the server's env From 10c0ca4cf73d396aefddeeaced042d616c5c34a2 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 4 Sep 2026 20:29:33 +0200 Subject: [PATCH 11/16] feat(email): write to people in their own language (E9.5 S7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Twelve mails carried hardcoded English while the app itself has enforced HU/EN parity since E5. A mail is the one user-facing surface nobody can re-render by switching the UI language: it arrives once, in whatever language we picked, and stays that way. That made the epic's own headline mail — "one mail, one click" — English for a Hungarian reader. The text moved to a catalog (`mailtext.py`) with a `text(key, lang, **vars)` renderer, and the parity rule is now a test rather than a habit: every key must carry both languages AND the same `{placeholders}`. A translation that drops `{link}` sends a mail with no link in it; one that invents a placeholder raises inside a background task nobody is watching. Both are silent in production. Every `send_*` takes the language as its second argument, resolved by the REQUEST handler and never inside the mail — these all run as background tasks, after the request's session is gone, which is the same reason the approval mail computes its link in the handler. Where the language comes from, in order: the account's own preference; failing that, the language the person asked us in; failing that, English. The middle one is new — `invites.lang`, migration `0082` — because the mails an access request produces go to an address that usually has no account yet, and the signal does exist: the page their form was submitted from, or the locale Google reported for a sign-in we refused. Captured rather than derived, for the third time in this epic (`google_email` in S3, `preferences.language` in S6 taught the same lesson): a signal that exists only in one moment cannot be backfilled later. So the admin is asked in exactly one place — the invitation to an address they typed in themselves, where nobody has ever told us anything. That send is now a dialog rather than a bare click, which is also where the "you already wrote to them" warning belongs — it used to live only in a tooltip the admin may never hover, guarding the one action whose failure mode is mailing a real person twice. S6's registration `lang` field turned out to be dead on arrival: the backend accepted it, the frontend never sent it, so every account has been storing nothing. The request builders now stamp the UI language themselves — a signal that has to be passed in by every caller is one every caller can forget, which is exactly how it shipped unset. Admins are notified one mail per language group instead of one blast to everyone. They are people with their own UI language; a single message would have to pick one of them and be wrong for the rest. Also removes a duplicated `invited_at` column declaration on `Invite` (two identical definitions landed in S6). --- backend/alembic/versions/0082_invite_lang.py | 34 ++ backend/app/auth.py | 6 +- backend/app/email.py | 225 +++++------ backend/app/mailtext.py | 359 ++++++++++++++++++ backend/app/models.py | 11 +- backend/app/routes/admin.py | 45 ++- backend/app/routes/auth.py | 122 ++++-- backend/app/routes/config.py | 3 +- backend/app/routes/setup.py | 6 +- backend/tests/test_admin_pending_access.py | 2 +- backend/tests/test_auth_session.py | 8 +- backend/tests/test_email_alerts.py | 18 +- backend/tests/test_email_canonical.py | 2 +- backend/tests/test_email_html_part.py | 4 +- backend/tests/test_mail_language.py | 263 +++++++++++++ backend/tests/test_welcome_mail.py | 8 +- frontend/src/components/AdminUsers.tsx | 68 +++- .../src/components/adminInvites.dom.test.tsx | 65 +++- frontend/src/i18n/locales/en/settings.json | 5 + frontend/src/i18n/locales/hu/settings.json | 5 + frontend/src/lib/api/admin.ts | 12 +- frontend/src/lib/api/auth.ts | 14 +- frontend/src/lib/api/core.ts | 6 + frontend/src/lib/api/setup.ts | 8 +- 24 files changed, 1088 insertions(+), 211 deletions(-) create mode 100644 backend/alembic/versions/0082_invite_lang.py create mode 100644 backend/app/mailtext.py create mode 100644 backend/tests/test_mail_language.py diff --git a/backend/alembic/versions/0082_invite_lang.py b/backend/alembic/versions/0082_invite_lang.py new file mode 100644 index 00000000..1d818c62 --- /dev/null +++ b/backend/alembic/versions/0082_invite_lang.py @@ -0,0 +1,34 @@ +"""invites.lang — the language this person asked us in + +Revision ID: 0082_invite_lang +Revises: 0081_invite_invited_at +Create Date: 2026-09-05 + +The mails an access request produces — approved, denied, invited — go to an address that often has +no account yet, so there is no `preferences.language` to read. The signal does exist, though, and +only at the moment of the request: the language of the page the form was submitted from, or the +locale Google reported for a sign-in we refused. Recording it there is what lets the decision mail +answer in the requester's own language instead of an admin's guess about a stranger. + +Captured rather than derived, for the third time in this epic: `preferences.language` (S6) and +`google_email` (S3) both taught that a signal which exists only in one moment cannot be backfilled +later. So no backfill here either — NULL means "we were never told", which is the honest state for +every historic row, and for every address an admin whitelists by hand. Those fall back to English +unless the admin picks otherwise on the invitation send. +""" + +import sqlalchemy as sa +from alembic import op + +revision: str = "0082_invite_lang" +down_revision: str | None = "0081_invite_invited_at" +branch_labels: str | None = None +depends_on: str | None = None + + +def upgrade() -> None: + op.add_column("invites", sa.Column("lang", sa.String(length=8), nullable=True)) + + +def downgrade() -> None: + op.drop_column("invites", "lang") diff --git a/backend/app/auth.py b/backend/app/auth.py index f2283b1f..bfe2fdb6 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -7,6 +7,7 @@ from sqlalchemy.orm import Session from starlette.requests import HTTPConnection from app import email as email_mod +from app import mailtext from app import sysconfig from app.config import settings from app.db import get_db @@ -211,6 +212,9 @@ def purge_user(db: Session, user: User, background: BackgroundTasks) -> None: touch any browser session — each caller handles its own.""" email = user.email.lower() user_id = user.id + # Read the language off the row while it still exists — the confirmation below is queued as a + # background task that runs after this session (and this user) are gone. + lang = mailtext.lang_for_user(user) tok = user.token # Both are stored encrypted — decrypt for the revoke call (the access-token fallback was passing # ciphertext, so a token row with no refresh token silently failed to revoke). decrypt_optional so a @@ -231,7 +235,7 @@ def purge_user(db: Session, user: User, background: BackgroundTasks) -> None: background.add_task(revoke_google_token, google_token) # Confirm the erasure to the (now former) user — GDPR good practice, and covers both the # self-service and admin deletion paths since both funnel through here. - background.add_task(email_mod.send_account_deleted, email, operator_contact()) + background.add_task(email_mod.send_account_deleted, email, lang, operator_contact()) ACTIVE_ACCOUNT_HEADER = "x-siftlode-account" diff --git a/backend/app/email.py b/backend/app/email.py index 18062b3e..57013360 100644 --- a/backend/app/email.py +++ b/backend/app/email.py @@ -21,7 +21,7 @@ from email.utils import formatdate from sqlalchemy import select from sqlalchemy.orm import Session -from app import sysconfig +from app import mailtext, sysconfig from app.config import settings from app.db import SessionLocal from app.models import User @@ -184,7 +184,7 @@ def _send(to: list[str], subject: str, body: str, reply_to: str | None = None) - return False -def _next_step(verify_link: str | None, has_account: bool, hours: int) -> str: +def _next_step(verify_link: str | None, has_account: bool, hours: int, lang: str) -> str: """What this person actually has to do next, in the one paragraph that carries the whole flow. Three states, because a mail that guesses wrong sends someone looking for a link that isn't @@ -196,40 +196,44 @@ def _next_step(verify_link: str | None, has_account: bool, hours: int) -> str: messages. One mail, one click. """ if verify_link: - return ( - "You already started an account, so there is just one step left. Confirm your email " - "address and you're in:\n\n" - f"{verify_link}\n\n" - f"The link works once and expires in {hours} hours. If it does, you can ask for a fresh " - "one from the sign-in page." - ) + return mailtext.text("next_step.verify", lang, link=verify_link, hours=hours) if has_account: - return f"Your account is ready — just sign in:\n\n{settings.app_base}" - return ( - "Create your account whenever you feel like it:\n\n" - f"{settings.app_base}\n\n" - "We'll send you a confirmation link to finish, and then you're in." - ) + return mailtext.text("next_step.sign_in", lang, app=settings.app_base) + return mailtext.text("next_step.register", lang, app=settings.app_base) + + +def _reply_contact(lang: str) -> tuple[str | None, str]: + """The admin address to answer to, and the closing line that names it — empty when no admin + contact is configured, so the mail never promises a reply nobody will read.""" + admin = _admin_contact() + return admin, (mailtext.text("contact.reply", lang, admin=admin) if admin else "") def send_access_approved( - email: str, verify_link: str | None = None, has_account: bool = False, hours: int = 24 + email: str, + lang: str, + verify_link: str | None = None, + has_account: bool = False, + hours: int = 24, ) -> bool: """Answer someone who ASKED for access. Sent automatically on approval, because a person who made a request is owed a reply — unlike the admin-initiated invitation below.""" - admin = _admin_contact() - body = ( - "Hi,\n\n" - "Your request to use Siftlode has been approved — welcome aboard.\n\n" - f"{_next_step(verify_link, has_account, hours)}\n\n" - "If you weren't expecting this, you can ignore the message.\n\n" - "— Siftlode" + (f"\n\nQuestions? Just reply to this email ({admin})." if admin else "") + admin, contact = _reply_contact(lang) + body = mailtext.text( + "access_approved.body", + lang, + next_step=_next_step(verify_link, has_account, hours, lang), + contact=contact, ) - return _send([email], "You're in — your Siftlode access is approved", body, reply_to=admin) + return _send([email], mailtext.text("access_approved.subject", lang), body, reply_to=admin) def send_access_invitation( - email: str, verify_link: str | None = None, has_account: bool = False, hours: int = 24 + email: str, + lang: str, + verify_link: str | None = None, + has_account: bool = False, + hours: int = 24, ) -> bool: """Invite someone the admin let in WITHOUT them asking — sent only when the admin presses send. @@ -237,131 +241,88 @@ def send_access_invitation( agreed it with the person, and an address can sit approved for weeks without anyone registering. So adding to the whitelist stays silent and this is a separate, deliberate act. That is also why the closing line offers no deadline — nudging is the one thing this mail must not do. + + Its language is the one place the app has no signal to read: the admin typed this address in, so + nobody has ever told us how to address its owner. That is why the send button asks (E9.5 S7). """ - admin = _admin_contact() - body = ( - "Hi,\n\n" - "Someone has opened up Siftlode for you — a quiet, self-hosted reader for your YouTube " - "subscriptions. Every upload from the channels you follow lands in one feed you can " - "actually filter and sort, with no algorithm deciding what you see, and no Shorts or " - "livestreams unless you want them.\n\n" - f"{_next_step(verify_link, has_account, hours)}\n\n" - "No rush — the door stays open.\n\n" - "— Siftlode" + (f"\n\nQuestions? Just reply to this email ({admin})." if admin else "") + admin, contact = _reply_contact(lang) + body = mailtext.text( + "access_invitation.body", + lang, + next_step=_next_step(verify_link, has_account, hours, lang), + contact=contact, ) - return _send([email], "You're invited to Siftlode", body, reply_to=admin) + return _send([email], mailtext.text("access_invitation.subject", lang), body, reply_to=admin) -def send_access_denied(email: str) -> bool: - admin = _admin_contact() - body = ( - "Hi,\n\n" - "Thanks for your interest in Siftlode. After review, your access request wasn't approved " - "this time.\n\n" - "If you think this was a mistake, you're welcome to reply and let us know.\n\n" - "— Siftlode" + (f"\n\nQuestions? Just reply to this email ({admin})." if admin else "") +def send_access_denied(email: str, lang: str) -> bool: + admin, contact = _reply_contact(lang) + body = mailtext.text("access_denied.body", lang, contact=contact) + return _send([email], mailtext.text("access_denied.subject", lang), body, reply_to=admin) + + +def send_role_changed(to: str, lang: str, role: str, operator: str | None) -> bool: + line = mailtext.text( + "role_changed.granted" if role == "admin" else "role_changed.revoked", lang ) - return _send([email], "About your Siftlode access request", body, reply_to=admin) + contact = mailtext.text("contact.operator", lang, operator=operator) if operator else "" + body = mailtext.text("role_changed.body", lang, line=line, contact=contact) + return _send([to], mailtext.text("role_changed.subject", lang), body, reply_to=operator) -def send_role_changed(to: str, role: str, operator: str | None) -> bool: - line = ( - "You've been granted admin access on Siftlode — you can now manage settings, the " - "scheduler and users." - if role == "admin" - else "Your admin access on Siftlode has been removed — you're now a regular user." - ) - contact = f"\n\nQuestions? Contact the operator at {operator}." if operator else "" - body = "Hi,\n\n" + line + contact + "\n\n— Siftlode" - return _send([to], "Your Siftlode role has changed", body, reply_to=operator) +def send_account_unsuspended(to: str, lang: str, operator: str | None) -> bool: + contact = mailtext.text("contact.operator", lang, operator=operator) if operator else "" + body = mailtext.text("account_unsuspended.body", lang, contact=contact) + return _send([to], mailtext.text("account_unsuspended.subject", lang), body, reply_to=operator) -def send_account_unsuspended(to: str, operator: str | None) -> bool: - contact = f"\n\nQuestions? Contact the operator at {operator}." if operator else "" - body = ( - "Hi,\n\n" - "Your Siftlode account has been reinstated — the suspension was lifted and you can sign " - "in again." + contact + "\n\n— Siftlode" - ) - return _send([to], "Your Siftlode account has been reinstated", body, reply_to=operator) +def send_account_deleted(to: str, lang: str, operator: str | None) -> bool: + contact = mailtext.text("contact.deleted", lang, operator=operator) if operator else "" + body = mailtext.text("account_deleted.body", lang, contact=contact) + return _send([to], mailtext.text("account_deleted.subject", lang), body, reply_to=operator) -def send_account_deleted(to: str, operator: str | None) -> bool: +def send_account_suspended(to: str, lang: str, operator: str | None) -> bool: contact = ( - f"\n\nIf you didn't request this, contact the Siftlode operator at {operator}." + mailtext.text("contact.suspended", lang, operator=operator) if operator - else "" + else mailtext.text("contact.suspended_unknown", lang) ) - body = ( - "Hi,\n\n" - "Your Siftlode account and all associated data — subscriptions, tags, watch history, " - "playlists and settings — have been permanently deleted. Nothing is retained.\n\n" - "Thanks for having used Siftlode." + contact + "\n\n— Siftlode" + body = mailtext.text("account_suspended.body", lang, contact=contact) + return _send([to], mailtext.text("account_suspended.subject", lang), body, reply_to=operator) + + +def send_admin_new_request(admins: list[str], lang: str, requester: str) -> bool: + """Tell the admins someone is waiting. One call per language group (see + `routes.auth.notify_admins_of_request`) — admins are people with their own UI language, and a + single blast to all of them would have to pick one and be wrong for the rest. + + The link deep-links straight to the admin Access-requests page (the SPA reads ?admin=…). The + requester address is spelled out (mail clients auto-link both the URL and the address), and the + reply note is accurate: _send sets Reply-To to the requester, so a reply reaches THEM — not the + sending mailbox. (The old copy said "Settings → Account", which was the wrong menu.) + """ + body = mailtext.text( + "admin_new_request.body", + lang, + requester=requester, + link=f"{settings.app_base}/?admin=access-requests", ) - return _send([to], "Your Siftlode account has been deleted", body, reply_to=operator) + subject = mailtext.text("admin_new_request.subject", lang, requester=requester) + return _send(admins, subject, body, reply_to=requester) -def send_account_suspended(to: str, operator: str | None) -> bool: - contact = ( - f"If you think this is a mistake or have questions, contact the Siftlode operator " - f"at {operator}.\n\n" - if operator - else "If you think this is a mistake, please contact the Siftlode operator.\n\n" - ) - body = ( - "Hi,\n\n" - "A sign-in to your Siftlode account was just attempted, but the account is currently " - "suspended, so access was denied.\n\n" + contact + "— Siftlode" - ) - return _send([to], "Your Siftlode account is suspended", body, reply_to=operator) +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) + return _send([to], mailtext.text("verify_email.subject", lang), body) -def send_admin_new_request(admins: list[str], requester: str) -> bool: - # The link deep-links straight to the admin Access-requests page (the SPA reads ?admin=…). The - # requester address is spelled out (mail clients auto-link both the URL and the address), and the - # reply note is accurate: _send sets Reply-To to the requester, so a reply reaches THEM — not the - # sending mailbox. (The old copy said "Settings → Account", which was the wrong menu.) - approve_url = f"{settings.app_base}/?admin=access-requests" - body = ( - f"{requester} just requested access to Siftlode.\n\n" - f"Approve or deny it here:\n{approve_url}\n" - f"(or in Siftlode: open Users → Access requests)\n\n" - f"Requester's email: {requester}\n" - f"Replying to this message goes straight to the requester (it's the Reply-To address).\n" - ) - return _send(admins, f"Siftlode access request from {requester}", body, reply_to=requester) +def send_password_reset(to: str, lang: str, link: str) -> bool: + body = mailtext.text("password_reset.body", lang, link=link) + return _send([to], mailtext.text("password_reset.subject", lang), body) -def send_verify_email(to: str, link: str, hours: int = 24) -> bool: - body = ( - "Hi,\n\n" - "Confirm your email to finish creating your Siftlode account:\n\n" - f"{link}\n\n" - # Say 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. - f"The link works once and expires in {hours} hours. 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" - ) - return _send([to], "Confirm your Siftlode email", body) - - -def send_password_reset(to: str, link: str) -> bool: - body = ( - "Hi,\n\n" - "Use this link to set a new Siftlode password (it expires in an hour):\n\n" - f"{link}\n\n" - "If you didn't request this, you can ignore this message — your password is unchanged.\n\n" - "— Siftlode" - ) - return _send([to], "Reset your Siftlode password", body) - - -def send_test(to: str) -> bool: - body = ( - "This is a test email from Siftlode.\n\n" - "If you're reading this, your SMTP settings work.\n\n" - "— Siftlode" - ) - return _send([to], "Siftlode SMTP test", body) +def send_test(to: str, lang: str) -> bool: + return _send([to], mailtext.text("test.subject", lang), mailtext.text("test.body", lang)) diff --git a/backend/app/mailtext.py b/backend/app/mailtext.py new file mode 100644 index 00000000..5573c991 --- /dev/null +++ b/backend/app/mailtext.py @@ -0,0 +1,359 @@ +"""Bilingual text for every outbound mail, and the one resolver that decides which language a +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` +enforces that every key carries both languages *and* the same `{placeholders}`, so a half-translated +mail fails the gate instead of reaching a person. + +The language is resolved by the REQUEST handler and handed to `send_*` as a plain code. It is never +resolved inside the mail: every send runs as a `BackgroundTasks` job, after the request's session is +gone — the same reason `_welcome_mail_parts` computes its link in the handler and not in the task. +""" + +from sqlalchemy import select +from sqlalchemy.orm import Session + +from app.models import Invite, User +from app.utils import canonical_email + +SUPPORTED: tuple[str, ...] = ("en", "hu") +DEFAULT = "en" + + +def normalize(lang: str | None) -> str: + """Coerce anything to a language we can actually render. Storage is not a guarantee: a + `preferences.language` written by an older build (or a locale we no longer ship) must degrade to + English rather than raise inside a background task nobody is watching.""" + return lang if lang in SUPPORTED else DEFAULT + + +def text(key: str, lang: str | None, **values: object) -> str: + """Render one catalog entry. Unknown languages fall back to English (see `normalize`); an + unknown KEY raises, because that is a programming error the tests must surface, not something to + paper over with an empty line in someone's inbox.""" + return MAIL_TEXT[key][normalize(lang)].format(**values) + + +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 {} + return normalize(prefs.get("language")) + + +def lang_for_email(db: Session, email: str) -> str: + """The language for an address that may not have an account yet: their own account preference, + else the language they filled our access-request form in, else English. + + Matched CANONICALLY like every other identity lookup in this epic, and it walks past rows that + carry no usable code instead of stopping at the first one — prod really does hold two accounts + for a single mailbox, and only one of them may have ever picked a language. + """ + canon = canonical_email(email) + for prefs in ( + db.execute(select(User.preferences).where(User.email_canonical == canon).order_by(User.id)) + .scalars() + .all() + ): + code = (prefs or {}).get("language") + if code in SUPPORTED: + return code + for code in ( + db.execute(select(Invite.lang).where(Invite.email_canonical == canon).order_by(Invite.id)) + .scalars() + .all() + ): + if code in SUPPORTED: + return code + return DEFAULT + + +# 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]] = { + # ── Shared closing fragments ──────────────────────────────────────────────────────────────── + "contact.reply": { + "en": "\n\nQuestions? Just reply to this email ({admin}).", + "hu": "\n\nKérdésed van? Csak válaszolj erre a levélre ({admin}).", + }, + "contact.operator": { + "en": "\n\nQuestions? Contact the operator at {operator}.", + "hu": "\n\nKérdésed van? Írj az üzemeltetőnek: {operator}.", + }, + "contact.deleted": { + "en": "\n\nIf you didn't request this, contact the Siftlode operator at {operator}.", + "hu": "\n\nHa nem te kérted, írj a Siftlode üzemeltetőjének: {operator}.", + }, + "contact.suspended": { + "en": ( + "If you think this is a mistake or have questions, contact the Siftlode operator " + "at {operator}.\n\n" + ), + "hu": ( + "Ha szerinted ez tévedés, vagy kérdésed van, írj a Siftlode üzemeltetőjének: " + "{operator}.\n\n" + ), + }, + "contact.suspended_unknown": { + "en": "If you think this is a mistake, please contact the Siftlode operator.\n\n", + "hu": "Ha szerinted ez tévedés, keresd a Siftlode üzemeltetőjét.\n\n", + }, + # ── The one paragraph that carries the whole onboarding flow (see email._next_step) ────────── + "next_step.verify": { + "en": ( + "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 " + "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 " + "kérhetsz újat." + ), + }, + "next_step.sign_in": { + "en": "Your account is ready — just sign in:\n\n{app}", + "hu": "A fiókod készen áll — csak lépj be:\n\n{app}", + }, + "next_step.register": { + "en": ( + "Create your account whenever you feel like it:\n\n" + "{app}\n\n" + "We'll send you a confirmation link to finish, and then you're in." + ), + "hu": ( + "Hozd létre a fiókod, amikor kedved tartja:\n\n" + "{app}\n\n" + "Küldünk egy megerősítő linket a befejezéshez, és már bent is vagy." + ), + }, + # ── Access decision ───────────────────────────────────────────────────────────────────────── + "access_approved.subject": { + "en": "You're in — your Siftlode access is approved", + "hu": "Bent vagy — jóváhagytuk a Siftlode-hozzáférésed", + }, + "access_approved.body": { + "en": ( + "Hi,\n\n" + "Your request to use Siftlode has been approved — welcome aboard.\n\n" + "{next_step}\n\n" + "If you weren't expecting this, you can ignore the message.\n\n" + "— Siftlode{contact}" + ), + "hu": ( + "Szia,\n\n" + "Jóváhagytuk a Siftlode-hozzáférési kérelmed — üdv a fedélzeten.\n\n" + "{next_step}\n\n" + "Ha nem számítottál erre, nyugodtan hagyd figyelmen kívül ezt a levelet.\n\n" + "— Siftlode{contact}" + ), + }, + "access_invitation.subject": { + "en": "You're invited to Siftlode", + "hu": "Meghívtak a Siftlode-ra", + }, + "access_invitation.body": { + "en": ( + "Hi,\n\n" + "Someone has opened up Siftlode for you — a quiet, self-hosted reader for your YouTube " + "subscriptions. Every upload from the channels you follow lands in one feed you can " + "actually filter and sort, with no algorithm deciding what you see, and no Shorts or " + "livestreams unless you want them.\n\n" + "{next_step}\n\n" + "No rush — the door stays open.\n\n" + "— Siftlode{contact}" + ), + "hu": ( + "Szia,\n\n" + "Valaki megnyitotta előtted a Siftlode-ot — ez egy csendes, saját üzemeltetésű olvasó " + "a YouTube-feliratkozásaidhoz. A követett csatornák minden feltöltése egyetlen " + "hírfolyamba érkezik, amit tényleg tudsz szűrni és rendezni; nincs algoritmus, ami " + "eldönti, mit látsz, és nincs Shorts vagy élő adás, hacsak nem kéred.\n\n" + "{next_step}\n\n" + "Nem sürget semmi — az ajtó nyitva marad.\n\n" + "— Siftlode{contact}" + ), + }, + "access_denied.subject": { + "en": "About your Siftlode access request", + "hu": "A Siftlode-hozzáférési kérelmedről", + }, + "access_denied.body": { + "en": ( + "Hi,\n\n" + "Thanks for your interest in Siftlode. After review, your access request wasn't " + "approved this time.\n\n" + "If you think this was a mistake, you're welcome to reply and let us know.\n\n" + "— Siftlode{contact}" + ), + "hu": ( + "Szia,\n\n" + "Köszönjük az érdeklődésed a Siftlode iránt. Átnéztük a kérelmed, de most nem hagytuk " + "jóvá.\n\n" + "Ha szerinted ez tévedés, válaszolj nyugodtan erre a levélre.\n\n" + "— Siftlode{contact}" + ), + }, + # ── Account lifecycle ─────────────────────────────────────────────────────────────────────── + "role_changed.subject": { + "en": "Your Siftlode role has changed", + "hu": "Megváltozott a szereped a Siftlode-on", + }, + "role_changed.granted": { + "en": ( + "You've been granted admin access on Siftlode — you can now manage settings, the " + "scheduler and users." + ), + "hu": ( + "Admin jogot kaptál a Siftlode-on — mostantól kezelheted a beállításokat, az ütemezőt " + "és a felhasználókat." + ), + }, + "role_changed.revoked": { + "en": "Your admin access on Siftlode has been removed — you're now a regular user.", + "hu": "Elvettük az admin jogod a Siftlode-on — mostantól sima felhasználó vagy.", + }, + "role_changed.body": { + "en": "Hi,\n\n{line}{contact}\n\n— Siftlode", + "hu": "Szia,\n\n{line}{contact}\n\n— Siftlode", + }, + "account_unsuspended.subject": { + "en": "Your Siftlode account has been reinstated", + "hu": "Visszaállítottuk a Siftlode-fiókod", + }, + "account_unsuspended.body": { + "en": ( + "Hi,\n\n" + "Your Siftlode account has been reinstated — the suspension was lifted and you can " + "sign in again.{contact}\n\n— Siftlode" + ), + "hu": ( + "Szia,\n\n" + "Visszaállítottuk a Siftlode-fiókod — a felfüggesztést feloldottuk, újra be tudsz " + "lépni.{contact}\n\n— Siftlode" + ), + }, + "account_deleted.subject": { + "en": "Your Siftlode account has been deleted", + "hu": "Töröltük a Siftlode-fiókod", + }, + "account_deleted.body": { + "en": ( + "Hi,\n\n" + "Your Siftlode account and all associated data — subscriptions, tags, watch history, " + "playlists and settings — have been permanently deleted. Nothing is retained.\n\n" + "Thanks for having used Siftlode.{contact}\n\n— Siftlode" + ), + "hu": ( + "Szia,\n\n" + "A Siftlode-fiókod és minden hozzá tartozó adat — feliratkozások, címkék, megtekintési " + "előzmények, lejátszási listák és beállítások — véglegesen törölve. Semmit nem őrzünk " + "meg.\n\n" + "Köszönjük, hogy használtad a Siftlode-ot.{contact}\n\n— Siftlode" + ), + }, + "account_suspended.subject": { + "en": "Your Siftlode account is suspended", + "hu": "A Siftlode-fiókod fel van függesztve", + }, + "account_suspended.body": { + "en": ( + "Hi,\n\n" + "A sign-in to your Siftlode account was just attempted, but the account is currently " + "suspended, so access was denied.\n\n{contact}— Siftlode" + ), + "hu": ( + "Szia,\n\n" + "Valaki most próbált belépni a Siftlode-fiókodba, de a fiók jelenleg fel van " + "függesztve, ezért a belépést elutasítottuk.\n\n{contact}— Siftlode" + ), + }, + # ── To the admins ─────────────────────────────────────────────────────────────────────────── + "admin_new_request.subject": { + "en": "Siftlode access request from {requester}", + "hu": "Siftlode hozzáférési kérelem: {requester}", + }, + "admin_new_request.body": { + "en": ( + "{requester} just requested access to Siftlode.\n\n" + "Approve or deny it here:\n{link}\n" + "(or in Siftlode: open Users → Access requests)\n\n" + "Requester's email: {requester}\n" + "Replying to this message goes straight to the requester (it's the Reply-To address).\n" + ), + "hu": ( + "{requester} most kért hozzáférést a Siftlode-hoz.\n\n" + "Itt hagyhatod jóvá vagy utasíthatod el:\n{link}\n" + "(vagy a Siftlode-ban: Felhasználók → Hozzáférési kérések)\n\n" + "A kérelmező e-mail-címe: {requester}\n" + "Ha erre a levélre válaszolsz, az egyenesen a kérelmezőhöz megy (ő a Reply-To cím).\n" + ), + }, + # ── Link mails ────────────────────────────────────────────────────────────────────────────── + "verify_email.subject": { + "en": "Confirm your Siftlode email", + "hu": "Erősítsd meg a Siftlode e-mail-címed", + }, + "verify_email.body": { + "en": ( + "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 " + "one from the sign-in page.\n\n" + "If you didn't sign up, you can ignore this message.\n\n" + "— Siftlode" + ), + "hu": ( + "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 " + "kérhetsz újat.\n\n" + "Ha nem te regisztráltál, nyugodtan hagyd figyelmen kívül ezt a levelet.\n\n" + "— Siftlode" + ), + }, + "password_reset.subject": { + "en": "Reset your Siftlode password", + "hu": "Állíts be új Siftlode-jelszót", + }, + "password_reset.body": { + "en": ( + "Hi,\n\n" + "Use this link to set a new Siftlode password (it expires in an hour):\n\n" + "{link}\n\n" + "If you didn't request this, you can ignore this message — your password is " + "unchanged.\n\n" + "— Siftlode" + ), + "hu": ( + "Szia,\n\n" + "Ezzel a linkkel állíthatsz be új Siftlode-jelszót (egy óra múlva lejár):\n\n" + "{link}\n\n" + "Ha nem te kérted, hagyd figyelmen kívül ezt a levelet — a jelszavad változatlan.\n\n" + "— Siftlode" + ), + }, + "test.subject": { + "en": "Siftlode SMTP test", + "hu": "Siftlode SMTP-teszt", + }, + "test.body": { + "en": ( + "This is a test email from Siftlode.\n\n" + "If you're reading this, your SMTP settings work.\n\n" + "— Siftlode" + ), + "hu": ( + "Ez egy teszt e-mail a Siftlode-tól.\n\n" + "Ha ezt olvasod, az SMTP-beállításaid működnek.\n\n" + "— Siftlode" + ), + }, +} diff --git a/backend/app/models.py b/backend/app/models.py index 2b3e5fca..8d16706f 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -167,14 +167,17 @@ class Invite(Base, CanonicalEmailMixin): ) decided_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) decided_by: Mapped[str | None] = mapped_column(String(320)) # admin email - # When the admin last SENT this person an invitation from the whitelist row. Adding an - # address stays silent (it opens a door, it does not knock), so telling them is a separate - # act — and this is what stops it happening twice to a real person. - invited_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) # When the admin last SENT this person an invitation from the whitelist row. Adding an address # stays silent — it opens a door, it does not knock — so telling them is a separate, deliberate # act, and this is what stops it happening twice to a real person. invited_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) + # The language this person asked us in ("en"/"hu"), captured when the request was made: the page + # their form was on, or the locale Google reported for a refused sign-in. NULL for a row the + # admin whitelisted by hand — nobody has told us anything about that mailbox, which is exactly + # why the invitation send asks (E9.5 S7). Captured rather than derived for the same reason + # `preferences.language` and `google_email` are: the signal exists only in that one moment and + # cannot be backfilled afterwards. + lang: Mapped[str | None] = mapped_column(String(8)) class DemoWhitelist(Base, TimestampMixin): diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index da807d67..b10a90aa 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -11,6 +11,7 @@ from sqlalchemy.orm import Session from app import audit from app import email as email_mod +from app import mailtext from app.audit import AuditAction from app.auth import ( admin_user, @@ -81,6 +82,10 @@ def _serialize(inv: Invite, *, has_account: bool = False) -> dict: # …and whether removing it would actually take access away, or leave the env allow-list # granting it anyway (for an ADMIN_EMAILS address, as an admin). "still_allowed_by_env": _env_allowed(inv.email), + # The language this person asked us in, or null when nobody ever told us (a hand-added + # whitelist row). The invitation dialog preselects it, and falls back to the admin's own UI + # language — a guess, but a visible one the admin can correct before sending. + "lang": inv.lang, } @@ -192,7 +197,8 @@ def approve_invite( # the task must not outlive it. Approval answers a request the person MADE, so unlike the # invitation below it is sent automatically — a reply is owed, not a nudge. link, has_account, hours = _welcome_mail_parts(db, inv.email) - background.add_task(email_mod.send_access_approved, inv.email, link, has_account, hours) + lang = mailtext.lang_for_email(db, inv.email) + background.add_task(email_mod.send_access_approved, inv.email, lang, link, has_account, hours) return _serialize(inv, has_account=bool(_accounts_for(db, [inv.email]))) @@ -206,7 +212,9 @@ def deny_invite( # Symmetry with approve: the requester hears back either way, instead of a silent limbo. No # account is activated, so this is an email only (the requester has no in-app inbox yet). inv = _decide(db, invite_id, admin, approved=False) - background.add_task(email_mod.send_access_denied, inv.email) + background.add_task( + email_mod.send_access_denied, inv.email, mailtext.lang_for_email(db, inv.email) + ) return _serialize(inv, has_account=bool(_accounts_for(db, [inv.email]))) @@ -244,10 +252,18 @@ def add_invite( return _serialize(inv, has_account=bool(_accounts_for(db, [email]))) +class InviteSendIn(BaseModel): + # Which language to write this person in. Optional so an existing caller (and a test) can post + # an empty body, but the admin UI always sends it: this is the one recipient the app has no + # signal about, because the admin typed the address in themselves. + lang: str | None = None + + @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), ) -> dict: @@ -273,6 +289,14 @@ def send_invitation( # never sent. raise HTTPException(status_code=409, detail={"code": "invite_send", "reason": "email_off"}) 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 + # that means "what this person told us" into "what we last guessed". + picked = body.lang if body else None + if picked in mailtext.SUPPORTED: + lang = inv.lang = picked + else: + lang = mailtext.lang_for_email(db, inv.email) inv.invited_at = datetime.now(timezone.utc) audit.record( db, @@ -283,7 +307,7 @@ def send_invitation( summary=inv.email, ) db.commit() - background.add_task(email_mod.send_access_invitation, inv.email, link, has_account, hours) + background.add_task(email_mod.send_access_invitation, inv.email, lang, link, has_account, hours) return _serialize(inv, has_account=has_account) @@ -400,7 +424,13 @@ def set_user_role( target.role = role db.commit() if changed: - background.add_task(email_mod.send_role_changed, target.email, role, operator_contact()) + background.add_task( + email_mod.send_role_changed, + target.email, + mailtext.lang_for_user(target), + role, + operator_contact(), + ) return _serialize_user(target) @@ -449,7 +479,12 @@ def set_user_suspended( target.is_suspended = suspended db.commit() if was_suspended and not suspended: - background.add_task(email_mod.send_account_unsuspended, target.email, operator_contact()) + background.add_task( + email_mod.send_account_unsuspended, + target.email, + mailtext.lang_for_user(target), + operator_contact(), + ) return _serialize_user(target) diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 85234d01..712a20ef 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -18,6 +18,7 @@ from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session from app import email as email_mod +from app import mailtext from app import sysconfig from app.auth import ( _YOUTUBE_SCOPES, @@ -63,12 +64,20 @@ _request_access_limiter = RateLimiter(max_events=5, window_seconds=300) _suspend_email_limiter = RateLimiter(max_events=1, window_seconds=3600) -def _notify_suspended(background: BackgroundTasks, email: str) -> None: +def _notify_suspended(background: BackgroundTasks, user: User) -> None: """Schedule the one-per-hour 'account suspended' email to a user whose (otherwise valid) sign-in was just blocked. Gated by verified credentials at the call site, so this can't be - triggered by someone who doesn't control the account.""" - if _suspend_email_limiter.allow(email): - background.add_task(email_mod.send_account_suspended, email, operator_contact()) + triggered by someone who doesn't control the account. + + Takes the user rather than the address because the mail needs their language, and the send runs + as a background task — after this request's session, and the row, are out of reach.""" + if _suspend_email_limiter.allow(user.email): + background.add_task( + email_mod.send_account_suspended, + user.email, + mailtext.lang_for_user(user), + operator_contact(), + ) _demo_limiter = RateLimiter(max_events=5, window_seconds=60) @@ -101,18 +110,43 @@ def google_oauth(db: Session): return _oauth.google -def admin_notify_emails(db: Session) -> list[str]: - """Who to email about admin events (e.g. a new access request). The ACTUAL admins — every active, - non-suspended `role=admin` user — UNIONED with the env ADMIN_EMAILS bootstrap list. Using the DB - roles means an admin promoted through the UI is notified too (env alone would miss them, yet they - CAN approve requests — the approve UI is role-gated, so the notify set must match); the env union - keeps a freshly-seeded instance working before any DB admin exists.""" - db_admins = db.execute( - select(User.email).where( +def admin_notify_groups(db: Session) -> dict[str, list[str]]: + """Who to email about admin events (e.g. a new access request), grouped by the language each of + them reads. The ACTUAL admins — every active, non-suspended `role=admin` user — UNIONED with the + env ADMIN_EMAILS bootstrap list. Using the DB roles means an admin promoted through the UI is + notified too (env alone would miss them, yet they CAN approve requests — the approve UI is + role-gated, so the notify set must match); the env union keeps a freshly-seeded instance working + before any DB admin exists. + + Grouped rather than one blast to everyone, because these recipients are people with their own UI + language: a single message would have to pick one of them and be wrong for the rest. An env + address with no admin account still gets looked up, so an operator listed in ADMIN_EMAILS whose + account is (say) suspended is still written to in the language they chose.""" + rows = db.execute( + select(User.email, User.preferences).where( User.role == "admin", User.is_active.is_(True), User.is_suspended.is_(False) ) - ).scalars() - return sorted({e.lower() for e in db_admins if e} | settings.admin_email_set) + ).all() + by_email = {e.lower(): mailtext.normalize((p or {}).get("language")) for e, p in rows if e} + for email in settings.admin_email_set: + if email not in by_email: + by_email[email] = mailtext.lang_for_email(db, email) + groups: dict[str, list[str]] = {} + for email in sorted(by_email): + groups.setdefault(by_email[email], []).append(email) + return groups + + +def notify_admins_of_request( + db: Session, background: BackgroundTasks | None, requester: str +) -> None: + """Tell every admin that someone is waiting — one mail per language group. Sends inline when + there is no BackgroundTasks to hand (the registration path already runs outside a response).""" + for lang, recipients in admin_notify_groups(db).items(): + if background is None: + email_mod.send_admin_new_request(recipients, lang, requester) + else: + background.add_task(email_mod.send_admin_new_request, recipients, lang, requester) def is_demo_allowed(db: Session, email: str) -> bool: @@ -172,23 +206,36 @@ class InviteRequest(NamedTuple): is_new: bool -def upsert_pending_invite(db: Session, email: str) -> InviteRequest: - """Idempotently record an access request. +def upsert_pending_invite(db: Session, email: str, lang: str | None = None) -> InviteRequest: + """Idempotently record an access request, and the language it was made in. "Already known" is decided on the CANONICAL form: `npeter83.temu@` and `npeter83temu@` are one Gmail mailbox, and treating them as two is what put a second, silent access request in the admin queue on 2026-09-03 for an already-approved user. An approved sibling wins — re-requesting must not reopen a decision that has already been made for that mailbox. The row is still STORED under the address as typed, because that is where mail has to go. + + `lang` is the language of the surface the request came from — the sign-in page, or the locale + Google reported for a refused sign-in. The latest one wins: someone who asks again from a + Hungarian page is telling us something newer than last time. Anything unsupported is ignored + rather than stored, so a forged body cannot put junk in the column. + + The commit contract is unchanged: this still commits ONLY when it writes something. Callers rely + on that — `_register_account` persists its own flushed user precisely because a no-op here + commits nothing (E9.5 S6). """ email = (email or "").lower() if not valid_email(email): 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) if inv is not None: + relearned = lang is not None and inv.lang != lang + if relearned: + inv.lang = lang # Let a previously-denied person ask again; don't disturb approved/pending rows. if inv.status == "denied": inv.status = "pending" @@ -197,8 +244,10 @@ def upsert_pending_invite(db: Session, email: str) -> InviteRequest: inv.decided_by = None db.commit() return InviteRequest(inv, True) + if relearned: + db.commit() return InviteRequest(inv, False) - inv = Invite(email=email, status="pending") + inv = Invite(email=email, status="pending", lang=lang) db.add(inv) db.commit() return InviteRequest(inv, True) @@ -348,10 +397,11 @@ async def callback( # A denied Google login doubles as an access request: record it for the admin and # land the user on a friendly "request received" screen instead of a raw 403. if email: - req = upsert_pending_invite(db, email) - admins = admin_notify_emails(db) - if req.is_new and admins: - background.add_task(email_mod.send_admin_new_request, admins, email) + # Google just told us this person's locale — the only language signal that will ever + # exist for an address with no account, and the decision mail is written from it. + req = upsert_pending_invite(db, email, (userinfo.get("locale") or "").split("-")[0]) + if req.is_new: + notify_admins_of_request(db, background, email) return RedirectResponse(url=_access_notice_url("requested", email, req.invite)) return RedirectResponse(url=_access_notice_url("denied")) @@ -403,7 +453,7 @@ async def callback( # tell the (verified) owner why — a successful Google auth proves they control this account. if user.is_suspended: log.info("Suspended account blocked at Google login: %s", email) - _notify_suspended(background, user.email) + _notify_suspended(background, user) return RedirectResponse(url="/?login=suspended") # Record what Google actually answers with, beside the address the user registered with. The two # are the same string for most people; where they differ (a deliberate `+tag`, a different dot @@ -569,9 +619,17 @@ async def logout(request: Request): return JSONResponse({"ok": True, "switched": False}) +class AccessRequestIn(EmailIn): + # The language of the page this request was made from. Same reasoning as registration's `lang` + # (and the same shape): it is the ONLY moment this preference exists anywhere the server can + # see, since the requester has no account to carry it, and it decides what language the approval + # or denial mail is written in. + lang: str | None = None + + @router.post("/request-access") async def request_access( - body: EmailIn, + body: AccessRequestIn, request: Request, background: BackgroundTasks, db: Session = Depends(get_db), @@ -592,10 +650,9 @@ async def request_access( return {"status": "pending"} if is_allowed(db, email): return {"status": "pending"} # already allowed — don't reveal it - req = upsert_pending_invite(db, email) - admins = admin_notify_emails(db) - if req.is_new and admins: - background.add_task(email_mod.send_admin_new_request, admins, email) + req = upsert_pending_invite(db, email, body.lang) + if req.is_new: + notify_admins_of_request(db, background, email) return {"status": "pending"} @@ -792,14 +849,12 @@ def _register_account(email: str, password: str, lang: str | None = None) -> Non db.commit() if email_ok: send_verification_link(db, user) - admins = admin_notify_emails(db) # Fired unconditionally, unlike the other two callers which gate on a fresh invite row: for # an address the admin already approved, nothing lands in the Access-requests queue, so this # is the only place the account's existence is announced at all. Delivery is a separate # matter — with no working SMTP `_send` drops it, which is why the pre-approved registrant # still needs the admin to notice them (roadmap UX-25). - if admins: - email_mod.send_admin_new_request(admins, email) + notify_admins_of_request(db, None, email) def issue_verification_link(db: Session, user: User, ttl: timedelta | None = None) -> str: @@ -825,6 +880,7 @@ def send_verification_link(db: Session, user: User) -> None: """ email_mod.send_verify_email( user.email, + mailtext.lang_for_user(user), issue_verification_link(db, user), hours=int(VERIFY_TTL.total_seconds() // 3600), ) @@ -937,7 +993,7 @@ def password_login( if user is None or user.is_demo or not password_ok: raise _login_refused(401, "invalid") if user.is_suspended: - _notify_suspended(background, user.email) + _notify_suspended(background, user) raise _login_refused(403, "suspended", user, operator=operator_contact() or None) if not user.email_verified: raise _login_refused(403, "email_unverified", user) @@ -985,7 +1041,9 @@ def _send_reset_if_eligible(email: str) -> None: if user.password_hash or user.password_revoked_at: raw = _issue_token(db, user, "reset", RESET_TTL) # The account's own address, as above — never the spelling that was typed into the form. - email_mod.send_password_reset(user.email, f"{_app_base()}/#reset={raw}") + email_mod.send_password_reset( + user.email, mailtext.lang_for_user(user), f"{_app_base()}/#reset={raw}" + ) @router.post("/password-reset/check") diff --git a/backend/app/routes/config.py b/backend/app/routes/config.py index 2086ae54..59f2afd3 100644 --- a/backend/app/routes/config.py +++ b/backend/app/routes/config.py @@ -10,6 +10,7 @@ from sqlalchemy.orm import Session from app import audit from app import email as email_mod +from app import mailtext from app import sysconfig from app.audit import AuditAction from app.auth import admin_user @@ -115,7 +116,7 @@ def send_test_email( lets the admin verify the settings actually work.""" if not email_mod.email_enabled(): raise HTTPException(status_code=400, detail="SMTP is not configured.") - sent = email_mod.send_test(user.email) + sent = email_mod.send_test(user.email, mailtext.lang_for_user(user)) if not sent: raise HTTPException(status_code=422, detail="SMTP send failed — check the settings.") return {"sent": True, "to": user.email} diff --git a/backend/app/routes/setup.py b/backend/app/routes/setup.py index a033629d..1c1b5b58 100644 --- a/backend/app/routes/setup.py +++ b/backend/app/routes/setup.py @@ -14,6 +14,7 @@ from sqlalchemy import select from sqlalchemy.orm import Session from app import email as email_mod +from app import mailtext from app import state, sysconfig from app.auth import PASSWORD_MIN_LEN from app.db import get_db @@ -103,6 +104,9 @@ def setup_config(settings_map: dict[str, Any], db: Session = Depends(require_set class SetupTestEmailIn(BaseModel): to: str = "" + # The wizard's page language. No account exists yet on a fresh instance, so this is the only + # thing that can say what language to write in; anything unsupported falls back to English. + lang: str | None = None @router.post("/test-email") @@ -111,7 +115,7 @@ def setup_test_email(body: SetupTestEmailIn, db: Session = Depends(require_setup to = body.to.strip() if "@" not in to: raise HTTPException(status_code=400, detail="Enter a valid email address.") - if not email_mod.send_test(to): + if not email_mod.send_test(to, mailtext.normalize(body.lang)): raise HTTPException(status_code=400, detail="Couldn't send — check the SMTP settings.") return {"ok": True} diff --git a/backend/tests/test_admin_pending_access.py b/backend/tests/test_admin_pending_access.py index 3f286657..d337050a 100644 --- a/backend/tests/test_admin_pending_access.py +++ b/backend/tests/test_admin_pending_access.py @@ -128,7 +128,7 @@ 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, link, **kw: sent.append((to, link)) + auth_routes.email_mod, "send_verify_email", lambda to, lang, link, **kw: sent.append((to, link)) ) 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 68a42286..b2c47cc4 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -200,7 +200,7 @@ 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, 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 @@ -321,9 +321,9 @@ def test_registration_of_a_pre_approved_email_persists_the_account_with_smtp(db, sent: list[str] = [] monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: True) monkeypatch.setattr( - auth_mod.email_mod, "send_verify_email", lambda to, link, **kw: sent.append(to) + auth_mod.email_mod, "send_verify_email", lambda to, lang, link, **kw: sent.append(to) ) - monkeypatch.setattr(auth_mod.email_mod, "send_admin_new_request", lambda admins, to: True) + monkeypatch.setattr(auth_mod.email_mod, "send_admin_new_request", lambda admins, lang, to: True) db.add(Invite(email="rita@example.com", status="approved", decided_by="admin@example.com")) db.commit() @@ -422,7 +422,7 @@ 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, 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_alerts.py b/backend/tests/test_email_alerts.py index f8b66c94..0cb1679c 100644 --- a/backend/tests/test_email_alerts.py +++ b/backend/tests/test_email_alerts.py @@ -42,7 +42,7 @@ def test_a_skipped_email_notifies_every_admin(db, make_user): admin_b = make_user(email="admin-b@example.com", role="admin") plain = make_user(email="plain@example.com") - assert email_mod.send_test("someone@example.com") is False # fail-soft, as before + assert email_mod.send_test("someone@example.com", "en") is False # fail-soft, as before db.expire_all() for admin in (admin_a, admin_b): @@ -57,7 +57,7 @@ def test_the_notice_is_throttled_so_a_dead_relay_cannot_flood_the_inbox(db, make admin = make_user(email="admin@example.com", role="admin") for _ in range(5): - email_mod.send_test("someone@example.com") + email_mod.send_test("someone@example.com", "en") db.expire_all() assert len(_notifications(db, admin.id)) == 1 @@ -84,7 +84,7 @@ def test_a_send_failure_is_reported_separately_from_a_missing_config(db, make_us monkeypatch.setattr(email_mod.smtplib, "SMTP", _boom) - assert email_mod.send_test("someone@example.com") is False + assert email_mod.send_test("someone@example.com", "en") is False db.expire_all() (n,) = _notifications(db, admin.id) @@ -96,7 +96,7 @@ def test_no_admins_means_no_notification_and_no_error(db, make_user): # rather than blowing up inside the fail-soft path it is supposed to make visible. plain = make_user(email="plain@example.com") - assert email_mod.send_test("someone@example.com") is False + assert email_mod.send_test("someone@example.com", "en") is False db.expire_all() assert _notifications(db, plain.id) == [] @@ -106,10 +106,10 @@ def test_a_drop_with_nobody_to_tell_does_not_burn_the_hour(db, make_user): # The throttle window must only be spent on a notice that was actually written. Otherwise a drop # during setup — before any admin exists — silences the next hour for the admin created a minute # later, which is exactly the blind spot this alert exists to remove. - email_mod.send_test("someone@example.com") # no admins yet + email_mod.send_test("someone@example.com", "en") # no admins yet admin = make_user(email="admin@example.com", role="admin") - assert email_mod.send_test("someone@example.com") is False + assert email_mod.send_test("someone@example.com", "en") is False db.expire_all() assert len(_notifications(db, admin.id)) == 1 @@ -127,10 +127,10 @@ def test_a_failed_alert_does_not_burn_the_hour_either(db, make_user, monkeypatch return real(*a, **kw) monkeypatch.setattr(email_mod, "create_notification", _maybe_boom) - email_mod.send_test("someone@example.com") # the write blows up; nothing recorded + email_mod.send_test("someone@example.com", "en") # the write blows up; nothing recorded boom["on"] = False - assert email_mod.send_test("someone@example.com") is False + assert email_mod.send_test("someone@example.com", "en") is False db.expire_all() assert len(_notifications(db, admin.id)) == 1 @@ -146,4 +146,4 @@ def test_a_broken_alert_never_breaks_the_send(db, make_user, monkeypatch): monkeypatch.setattr(email_mod, "create_notification", _boom) - assert email_mod.send_test("someone@example.com") is False + assert email_mod.send_test("someone@example.com", "en") is False diff --git a/backend/tests/test_email_canonical.py b/backend/tests/test_email_canonical.py index f0934f0f..144d20e1 100644 --- a/backend/tests/test_email_canonical.py +++ b/backend/tests/test_email_canonical.py @@ -166,7 +166,7 @@ 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, 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_html_part.py b/backend/tests/test_email_html_part.py index 91f4ae85..dfe75479 100644 --- a/backend/tests/test_email_html_part.py +++ b/backend/tests/test_email_html_part.py @@ -103,7 +103,9 @@ def test_the_sent_message_is_multipart_with_the_plain_body_intact(monkeypatch): monkeypatch.setattr(email_mod.smtplib, "SMTP", _FakeSMTP) assert ( - email_mod.send_verify_email("someone@example.com", "https://siftlode.example/#verify=tok") + email_mod.send_verify_email( + "someone@example.com", "en", "https://siftlode.example/#verify=tok" + ) is True ) diff --git a/backend/tests/test_mail_language.py b/backend/tests/test_mail_language.py new file mode 100644 index 00000000..0f60ae86 --- /dev/null +++ b/backend/tests/test_mail_language.py @@ -0,0 +1,263 @@ +"""E9.5 S7 — every outbound mail is written in the reader's language. + +Two halves. The first is the catalog invariant: HU/EN parity with identical placeholders, enforced +here because a mail is the one user-facing surface nobody can re-render by switching the UI language +— it arrives once, in whatever language we picked. The second is the resolution chain: which +language a given recipient actually gets, and where that signal was captured, since for an address +with no account it exists only at the moment they asked us. +""" + +import string + +import pytest +from sqlalchemy import select + +from app import mailtext +from app.models import Invite, User +from app.routes import auth as auth_routes + +_PW = "correct horse battery" + + +def _placeholders(template: str) -> set[str]: + return {name for _, name, _, _ in string.Formatter().parse(template) if name} + + +# --- the catalog itself --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("key", sorted(mailtext.MAIL_TEXT)) +def test_every_mail_string_exists_in_both_languages(key): + assert set(mailtext.MAIL_TEXT[key]) == set(mailtext.SUPPORTED) + + +@pytest.mark.parametrize("key", sorted(mailtext.MAIL_TEXT)) +def test_every_translation_takes_the_same_placeholders(key): + # The parity that actually bites: a translation that drops `{link}` renders a mail with no link + # in it, and a translation that invents one raises KeyError inside a background task nobody is + # watching. Both are silent in production and loud here. + variants = mailtext.MAIL_TEXT[key] + expected = _placeholders(variants[mailtext.DEFAULT]) + for lang, template in variants.items(): + 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.""" + 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 == [] + + +def test_an_unknown_language_falls_back_to_english_instead_of_raising(): + # `preferences.language` is free-form JSON written by older builds and by locales we may stop + # shipping. A send that raised on one would take out a background task with no user in sight. + assert mailtext.text("test.subject", "de") == mailtext.text("test.subject", "en") + assert mailtext.text("test.subject", None) == mailtext.text("test.subject", "en") + + +# --- who gets which language ---------------------------------------------------------------------- + + +def test_a_users_own_preference_decides_their_language(make_user): + hu = make_user(email="hu@example.com", preferences={"language": "hu"}) + en = make_user(email="en@example.com", preferences={"language": "en"}) + none = make_user(email="none@example.com", preferences=None) + + assert mailtext.lang_for_user(hu) == "hu" + assert mailtext.lang_for_user(en) == "en" + assert mailtext.lang_for_user(none) == "en" + + +def test_an_address_with_no_account_falls_back_to_the_language_it_asked_in(db): + db.add(Invite(email="asked@example.com", status="pending", lang="hu")) + db.commit() + + assert mailtext.lang_for_email(db, "asked@example.com") == "hu" + + +def test_the_account_wins_over_the_invite_row(db, make_user): + make_user(email="both@example.com", preferences={"language": "en"}) + db.add(Invite(email="both@example.com", status="approved", lang="hu")) + db.commit() + + # The invite records how they asked once; the account records what they have since chosen. + assert mailtext.lang_for_email(db, "both@example.com") == "en" + + +def test_a_row_that_never_recorded_a_language_is_walked_past(db, make_user): + # Prod carries TWO accounts for one Gmail mailbox (the 2026-09-03 pair). Stopping at the first + # row would answer English for a person whose other account says Hungarian. + make_user(email="dup.one@gmail.com", preferences={}) + make_user(email="dupone@gmail.com", preferences={"language": "hu"}) + + assert mailtext.lang_for_email(db, "dup.one@gmail.com") == "hu" + + +def test_the_lookup_is_canonical_like_every_other_identity_match(db, make_user): + make_user(email="canon.match@gmail.com", preferences={"language": "hu"}) + + assert mailtext.lang_for_email(db, "canonmatch+tag@gmail.com") == "hu" + + +# --- capturing the signal at the one moment it exists --------------------------------------------- + + +def test_registration_records_the_language_of_the_sign_up_page(db, client): + r = client.post( + "/auth/register", json={"email": "signup@example.com", "password": _PW, "lang": "hu"} + ) + + assert r.status_code == 200 + user = db.execute(select(User).where(User.email == "signup@example.com")).scalar_one() + assert (user.preferences or {}).get("language") == "hu" + + +def test_an_access_request_records_the_language_it_was_made_in(db, client): + assert ( + client.post( + "/auth/request-access", json={"email": "asking@example.com", "lang": "hu"} + ).status_code + == 200 + ) + + inv = db.execute(select(Invite).where(Invite.email == "asking@example.com")).scalar_one() + assert inv.lang == "hu" + + +def test_asking_again_from_another_language_updates_the_row(db, client): + client.post("/auth/request-access", json={"email": "asking@example.com", "lang": "en"}) + client.post("/auth/request-access", json={"email": "asking@example.com", "lang": "hu"}) + + db.expire_all() + inv = db.execute(select(Invite).where(Invite.email == "asking@example.com")).scalar_one() + # The newest thing they told us wins — and there is still exactly one row (S3's canonical upsert). + assert inv.lang == "hu" + assert db.execute(select(Invite)).scalars().all() == [inv] + + +def test_an_unsupported_language_is_ignored_rather_than_stored(db, client): + client.post("/auth/request-access", json={"email": "asking@example.com", "lang": "../../etc"}) + + inv = db.execute(select(Invite).where(Invite.email == "asking@example.com")).scalar_one() + assert inv.lang is None + + +# --- the mails themselves ------------------------------------------------------------------------- + + +def _bodies(monkeypatch): + """Capture what `_send` would have put on the wire, rather than the arguments a caller passed — + the language only matters if it reaches the body.""" + from app import email as email_mod + + out: list[tuple[list[str], str, str]] = [] + monkeypatch.setattr( + email_mod, "_send", lambda to, subject, body, reply_to=None: out.append((to, subject, body)) + ) + return out + + +def test_the_confirmation_mail_is_written_in_the_accounts_language(db, client, monkeypatch): + from app import email as email_mod + + monkeypatch.setattr(email_mod, "email_enabled", lambda db=None: True) + sent = _bodies(monkeypatch) + + client.post("/auth/register", json={"email": "hu@example.com", "password": _PW, "lang": "hu"}) + + (_, subject, body) = sent[0] + assert subject == mailtext.MAIL_TEXT["verify_email.subject"]["hu"] + assert "#verify=" in body + + +def test_the_approval_mail_answers_in_the_language_the_request_was_made_in( + db, client, make_user, as_user, monkeypatch +): + from app import email as email_mod + + monkeypatch.setattr(email_mod, "email_enabled", lambda db=None: True) + sent = _bodies(monkeypatch) + admin = make_user(email="admin@example.com", role="admin") + client.post("/auth/request-access", json={"email": "asking@example.com", "lang": "hu"}) + inv = db.execute(select(Invite).where(Invite.email == "asking@example.com")).scalar_one() + sent.clear() # drop the admin notification; this test is about the reply to the requester + + r = as_user(admin).post(f"/api/admin/invites/{inv.id}/approve") + + assert r.status_code == 200 + (to, subject, _) = sent[0] + assert to == ["asking@example.com"] + assert subject == mailtext.MAIL_TEXT["access_approved.subject"]["hu"] + + +def test_a_denial_answers_in_that_language_too(db, client, make_user, as_user, monkeypatch): + from app import email as email_mod + + monkeypatch.setattr(email_mod, "email_enabled", lambda db=None: True) + sent = _bodies(monkeypatch) + admin = make_user(email="admin@example.com", role="admin") + client.post("/auth/request-access", json={"email": "asking@example.com", "lang": "hu"}) + inv = db.execute(select(Invite).where(Invite.email == "asking@example.com")).scalar_one() + sent.clear() + + as_user(admin).post(f"/api/admin/invites/{inv.id}/deny") + + assert sent[0][1] == mailtext.MAIL_TEXT["access_denied.subject"]["hu"] + + +def test_the_admins_pick_decides_an_invitation_and_is_remembered( + db, make_user, as_user, monkeypatch +): + from app import email as email_mod + + monkeypatch.setattr(email_mod, "email_enabled", lambda db=None: True) + sent = _bodies(monkeypatch) + admin = make_user(email="admin@example.com", role="admin") + # Hand-whitelisted: nobody has ever told us anything about this mailbox, which is exactly why + # the send button asks. + 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": "hu"}) + + assert r.status_code == 200 + assert sent[0][1] == mailtext.MAIL_TEXT["access_invitation.subject"]["hu"] + db.expire_all() + # Recorded, so a later mail to this address matches the one the admin already sent. + assert db.get(Invite, inv.id).lang == "hu" + + +def test_an_invitation_without_a_pick_still_goes_out(db, make_user, as_user, monkeypatch): + from app import email as email_mod + + monkeypatch.setattr(email_mod, "email_enabled", lambda db=None: True) + sent = _bodies(monkeypatch) + 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") + + assert r.status_code == 200 + assert sent[0][1] == mailtext.MAIL_TEXT["access_invitation.subject"]["en"] + db.expire_all() + # Nothing was picked, so nothing is remembered: the column means "what this person told us", + # not "what we last defaulted to". + assert db.get(Invite, inv.id).lang is None + + +def test_admins_are_notified_in_their_own_languages(db, make_user): + make_user(email="hu.admin@example.com", role="admin", preferences={"language": "hu"}) + make_user(email="en.admin@example.com", role="admin", preferences={"language": "en"}) + + groups = auth_routes.admin_notify_groups(db) + + # One mail per language, not one blast that has to be wrong for somebody. + assert groups == {"hu": ["hu.admin@example.com"], "en": ["en.admin@example.com"]} diff --git a/backend/tests/test_welcome_mail.py b/backend/tests/test_welcome_mail.py index ca5af938..2984cb1e 100644 --- a/backend/tests/test_welcome_mail.py +++ b/backend/tests/test_welcome_mail.py @@ -55,7 +55,7 @@ def test_an_unconfirmed_account_gets_the_confirmation_link_in_the_approval_mail( _approve(as_user(admin), inv.id) - (email, link, has_account, hours), _ = sent[0] + (email, _lang, link, has_account, hours), _ = sent[0] assert email == "waiting@example.com" and has_account is True assert link and "#verify=" in link # The whole point: this is the ONLY message they now have to open. @@ -73,7 +73,7 @@ def test_the_link_in_that_mail_actually_confirms_the_address(db, make_user, as_u email="waiting@example.com", password=_PW, email_verified=False, is_active=False ) _approve(as_user(admin), inv.id) - token = sent[0][0][1].split("#verify=")[1] + token = sent[0][0][2].split("#verify=")[1] r = as_user(admin).post("/auth/verify", json={"token": token}) @@ -112,7 +112,7 @@ def test_an_already_confirmed_account_is_just_told_to_sign_in(db, make_user, as_ _approve(as_user(admin), inv.id) - (_, link, has_account, _), _ = sent[0] + (_, _lang, link, has_account, _), _ = sent[0] assert link is None and has_account is True # no link to offer, and none needed @@ -128,7 +128,7 @@ def test_someone_with_no_account_yet_is_told_a_second_mail_is_coming( _approve(as_user(admin), inv.id) - (_, link, has_account, _), _ = sent[0] + (_, _lang, link, has_account, _), _ = sent[0] # Nothing to link to yet. Saying a confirmation mail follows is what makes its ABSENCE # noticeable — the failure that went unnoticed on 2026-09-03. assert link is None and has_account is False diff --git a/frontend/src/components/AdminUsers.tsx b/frontend/src/components/AdminUsers.tsx index d02c0fc1..38a7aa68 100644 --- a/frontend/src/components/AdminUsers.tsx +++ b/frontend/src/components/AdminUsers.tsx @@ -28,6 +28,8 @@ import { ADMIN_USERS_TAB_KEY } from "../lib/adminUsersTab"; import Tooltip from "./Tooltip"; import InlineNotice from "./InlineNotice"; import { Section } from "./ui/form"; +import SegmentedToggle from "./SegmentedToggle"; +import { LANGUAGES, type LangCode } from "../i18n"; import { LoadingState, StateMessage } from "./QueryState"; import { useConfirm } from "./ConfirmProvider"; import Tabs, { usePersistedTab } from "./Tabs"; @@ -458,10 +460,39 @@ function PendingAccessRowView({ ); } +/** The mail language for one invitation. Owns its own display state and reports every change up + * through `onPick`, because the dialog that hosts it resolves to a plain boolean — the choice has + * to live in the caller's closure, not in this component. Endonyms come from the app's own + * language list, so a third locale would appear here for free. */ +function InviteLangPicker({ + initial, + onPick, +}: { + initial: LangCode; + onPick: (lang: LangCode) => void; +}) { + const { t } = useTranslation(); + const [lang, setLang] = useState(initial); + return ( + ({ value: l.code, label: l.label }))} + value={lang} + onChange={(v) => { + setLang(v); + onPick(v); + }} + /> + ); +} + // --- Access requests (the Invite whitelist) — moved verbatim from Settings → Account ------- function AdminInvites() { - const { t } = useTranslation(); + const { t, i18n } = useTranslation(); const qc = useQueryClient(); const confirm = useConfirm(); const me = useMe(); @@ -540,10 +571,10 @@ function AdminInvites() { // nudging someone who never asked. `invited_at` comes back on the row so a second click is a // decision rather than an accident. const invite = useMutation({ - mutationFn: (i: Invite) => api.sendInvitation(i.id), - onSuccess: (_r, i) => { + mutationFn: (v: { inv: Invite; lang: LangCode }) => api.sendInvitation(v.inv.id, v.lang), + onSuccess: (_r, v) => { refresh(); - notify({ level: "success", message: t("settings.invites.invited", { email: i.email }) }); + notify({ level: "success", message: t("settings.invites.invited", { email: v.inv.email }) }); }, onError: (err) => notify({ @@ -554,6 +585,33 @@ function AdminInvites() { : t("settings.invites.inviteFailed"), }), }); + // The one recipient the app has no language signal about: the admin typed this address in, so + // nobody ever told us how to write to its owner. The dialog is where that gets decided — and it + // is also where the "you already wrote to them" warning belongs, next to the button that would + // do it a second time, rather than only in a tooltip the admin may never hover. + const onInvite = async (inv: Invite) => { + let lang: LangCode = LANGUAGES.some((l) => l.code === inv.lang) + ? (inv.lang as LangCode) + : ((i18n.language.split("-")[0] === "hu" ? "hu" : "en") as LangCode); + const ok = await confirm({ + title: t("settings.invites.inviteTitle"), + message: ( + <> +

+ {t( + inv.invited_at + ? "settings.invites.inviteConfirmAgain" + : "settings.invites.inviteConfirm", + { email: inv.email, time: new Date(inv.invited_at ?? 0).toLocaleString() }, + )} +

+ (lang = v)} /> + + ), + confirmLabel: t("settings.invites.inviteSend"), + }); + if (ok) invite.mutate({ inv, lang }); + }; const add = useMutation({ mutationFn: (email: string) => api.addInvite(email), @@ -647,7 +705,7 @@ function AdminInvites() { } >