From 85043013f013a4eb263a26b2e4fa3df064a2d670 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 1 Sep 2026 23:31:49 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20address=20the=20feature=20review=20?= =?UTF-8?q?=E2=80=94=20accurate=20copy,=20reachable=20controls,=20real=20c?= =?UTF-8?q?overage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 on the feature diff found 15; the substantive ones: - The delete control VANISHED on any invite whose address had registered, which is the ordinary sign-up shape (register writes the User and the pending invite together). The guard stays broad — that row is the account's approval gate, and orphaning it strands someone who can then neither sign in nor be approved — but the control is now a disabled button carrying the reason, on both lists, so an admin never sees a row silently missing a button its neighbours have. - Deleting an entry is not always a revoke: is_allowed falls back to the env ALLOWED_EMAILS / ADMIN_EMAILS lists once no row exists, and an ADMIN_EMAILS address is force-promoted on its next Google sign-in. The route now reports still_allowed_by_env and the UI says so instead of claiming access was removed. - "Deny keeps the row so they can't immediately re-ask" was simply false — upsert_pending_invite reopens a denied row. Corrected everywhere it was stated. - The Configuration warning reads me.email_configured, which save() did not invalidate, so it kept warning about fields the admin had just filled in. - The read-only variant was an aria-hidden span explained only by a Tooltip that the hints toggle can switch off; now a disabled button with title + aria-label. - The delete error path threw away the 409's reason (the stale-list case) and left the stale row on screen. - The throttle window was spent even when no notice was written, silencing the next hour on the path whose job is to stop silent drops. - email_enabled() opened a second session and decrypted the SMTP password on every /api/me poll; it now takes the caller's session. - welcomeMode.dom.test.ts never rendered the card, so reverting the history wiring to useState left it green. A render test now drives the real links and fails on that revert (verified). Plus /api/me flag tests, the registration-shaped pending case, the env-allowlist case, and a release note that matches what the code does. --- backend/app/email.py | 42 ++++++---- backend/app/routes/admin.py | 22 ++++-- backend/app/routes/me.py | 2 +- backend/tests/test_admin_invites.py | 47 ++++++++++- backend/tests/test_me_email_flag.py | 58 ++++++++++++++ frontend/src/components/AdminUsers.tsx | 55 ++++++++++--- frontend/src/components/ConfigPanel.tsx | 4 + .../welcomeMode.render.dom.test.tsx | 78 +++++++++++++++++++ frontend/src/i18n/locales/en/inbox.json | 6 +- frontend/src/i18n/locales/en/settings.json | 9 ++- frontend/src/i18n/locales/hu/inbox.json | 6 +- frontend/src/i18n/locales/hu/settings.json | 9 ++- frontend/src/lib/api/admin.ts | 5 +- frontend/src/lib/releaseNotes.ts | 10 +-- 14 files changed, 298 insertions(+), 55 deletions(-) create mode 100644 backend/tests/test_me_email_flag.py create mode 100644 frontend/src/components/welcomeMode.render.dom.test.tsx diff --git a/backend/app/email.py b/backend/app/email.py index 5bff78a6..b41f51d1 100644 --- a/backend/app/email.py +++ b/backend/app/email.py @@ -17,10 +17,12 @@ from email.message import EmailMessage from email.utils import formatdate from sqlalchemy import select +from sqlalchemy.orm import Session from app import sysconfig from app.config import settings from app.db import SessionLocal +from app.models import User from app.notifications import create_notification from app.ratelimit import RateLimiter @@ -39,8 +41,6 @@ def _alert_admins(reason: str, subject: str, recipients: list[str]) -> None: if not _alert_throttle.allow(reason): return try: - from app.models import User # local: app.models imports settle before this ever runs - with SessionLocal() as db: admins = ( db.execute( @@ -54,6 +54,9 @@ def _alert_admins(reason: str, subject: str, recipients: list[str]) -> None: .all() ) if not admins: + # Nothing was recorded, so don't let this cost the next hour of alerts on the very + # path whose job is to stop drops going unseen. Same on the failure branch below. + _alert_throttle.reset() return for uid in admins: create_notification( @@ -68,24 +71,33 @@ def _alert_admins(reason: str, subject: str, recipients: list[str]) -> None: ) db.commit() except Exception: + _alert_throttle.reset() log.exception("Could not record the undelivered-email notice (%r)", subject) -def _smtp() -> dict: - """Effective SMTP config (admin DB override over env defaults). Opens its own short - session because email is often sent from a BackgroundTask, outside a request's db.""" - with SessionLocal() as db: - return { - "host": sysconfig.get_str(db, "smtp_host"), - "port": sysconfig.get_int(db, "smtp_port"), - "user": sysconfig.get_str(db, "smtp_user"), - "password": sysconfig.get_str(db, "smtp_password"), - "from": sysconfig.get_str(db, "smtp_from"), - } +def _read_smtp(db: Session) -> dict: + return { + "host": sysconfig.get_str(db, "smtp_host"), + "port": sysconfig.get_int(db, "smtp_port"), + "user": sysconfig.get_str(db, "smtp_user"), + "password": sysconfig.get_str(db, "smtp_password"), + "from": sysconfig.get_str(db, "smtp_from"), + } -def email_enabled() -> bool: - c = _smtp() +def _smtp(db: Session | None = None) -> dict: + """Effective SMTP config (admin DB override over env defaults). Opens its own short session when + the caller has none, because email is usually sent from a BackgroundTask outside a request's db. + A caller that IS inside a request should pass its session: reading the password decrypts a Fernet + value, and /api/me polls this for every admin tab.""" + if db is not None: + return _read_smtp(db) + with SessionLocal() as own: + return _read_smtp(own) + + +def email_enabled(db: Session | None = None) -> bool: + c = _smtp(db) return bool(c["host"] and c["user"] and c["password"]) diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index fbdbd878..8ef7cc84 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -19,6 +19,7 @@ from app.auth import ( operator_contact, purge_user, ) +from app.config import settings from app.db import get_db from app.demo import reset_demo_state from app.models import ( @@ -175,12 +176,19 @@ def delete_invite( db: Session = Depends(get_db), ) -> dict: """Remove a whitelist/request entry outright — the counterpart to add_invite, for an address that - never became an account (an approval added by mistake, or one that is no longer wanted). Denying - a request keeps the row so the person cannot immediately re-ask; deleting it forgets them, and a - fresh request from that address starts over as pending. + never became an account (an approval added by mistake, or one that is no longer wanted). Deny + records a decision and keeps the row for the audit trail; delete forgets the address. Neither + stops the person asking again: `upsert_pending_invite` reopens a denied row, and a deleted one + simply comes back as a fresh pending request. - Refused once an account exists: `is_allowed` reads this row, so removing it would silently revoke - a live user's access. Delete the USER instead — purge_user removes the invite along with it.""" + Refused once an account exists for the address, whatever its state: this row is the account's + approval gate, and orphaning the account from it would leave a person who can neither sign in nor + be approved. Delete the USER instead — purge_user removes the invite along with it. + + Reports `still_allowed_by_env`, because deleting the row is NOT always a revoke: `is_allowed` + falls back to the env ALLOWED_EMAILS / ADMIN_EMAILS bootstrap lists once no invite exists, so for + an address on those the deletion leaves access intact (and an ADMIN_EMAILS address is still + promoted to admin on its next Google sign-in). The admin has to hear that, not guess it.""" inv = db.get(Invite, invite_id) if inv is None: raise HTTPException(status_code=404, detail="No such invite") @@ -189,6 +197,8 @@ def delete_invite( status_code=409, detail="That address already has an account — delete the account instead.", ) + email = (inv.email or "").lower() + still_allowed_by_env = email in settings.allowed_email_set or email in settings.admin_email_set audit.record( db, admin.id, @@ -199,7 +209,7 @@ def delete_invite( ) db.delete(inv) db.commit() - return {"deleted": invite_id} + return {"deleted": invite_id, "still_allowed_by_env": still_allowed_by_env} # --- Users & roles -------------------------------------------------------------------- diff --git a/backend/app/routes/me.py b/backend/app/routes/me.py index ea7cbf0c..4761a6f6 100644 --- a/backend/app/routes/me.py +++ b/backend/app/routes/me.py @@ -108,7 +108,7 @@ def get_me( db.scalar(select(func.count()).select_from(Invite).where(Invite.status == "pending")) or 0 ) - email_configured = email_mod.email_enabled() + email_configured = email_mod.email_enabled(db) # this request's session; no second connect return { "authenticated": True, "id": user.id, diff --git a/backend/tests/test_admin_invites.py b/backend/tests/test_admin_invites.py index 16620b86..0cc31906 100644 --- a/backend/tests/test_admin_invites.py +++ b/backend/tests/test_admin_invites.py @@ -27,7 +27,7 @@ def test_admin_deletes_a_whitelist_entry_that_never_became_an_account(db, make_u r = client.delete(f"/api/admin/invites/{listed['id']}") - assert r.status_code == 200 and r.json() == {"deleted": listed["id"]} + assert r.status_code == 200 and r.json()["deleted"] == listed["id"] db.expire_all() assert ( db.execute(select(Invite).where(Invite.email == "mistake@example.com")).scalar_one_or_none() @@ -56,6 +56,50 @@ def test_deleting_an_entry_that_has_an_account_is_refused(db, make_user, as_user ) +def test_a_pending_request_from_a_registration_is_not_deletable(db, make_user, as_user): + # The shape the ordinary sign-up produces: _register_account writes the User (inactive) AND the + # pending Invite together. The invite is that account's approval gate, so deleting it would + # strand the person — they could neither sign in nor be approved, and re-registering is a silent + # no-op. Managed from Users & roles instead, which is what the UI and the 409 both say. + admin = make_user(email="admin@example.com", role="admin") + make_user(email="signup@example.com", is_active=False, email_verified=False) + db.add(Invite(email="signup@example.com", status="pending")) + db.commit() + client = as_user(admin) + listed = _invites(client)["signup@example.com"] + assert listed["has_account"] is True # the UI shows a disabled control, not a missing one + + r = client.delete(f"/api/admin/invites/{listed['id']}") + + assert r.status_code == 409 + db.expire_all() + assert ( + db.execute(select(Invite).where(Invite.email == "signup@example.com")).scalar_one_or_none() + is not None + ) + + +def test_deleting_an_env_allow_listed_address_reports_that_it_kept_access( + db, make_user, as_user, monkeypatch +): + # Removing the row is NOT always a revoke: is_allowed falls back to the env bootstrap lists once + # no invite exists. The admin has to be told, or they think they removed access and did not. + from app.config import settings + + monkeypatch.setattr( + type(settings), "allowed_email_set", property(lambda self: {"ops@example.com"}) + ) + admin = make_user(email="admin@example.com", role="admin") + db.add(Invite(email="ops@example.com", status="approved", decided_by=admin.email)) + db.commit() + client = as_user(admin) + + r = client.delete(f"/api/admin/invites/{_invites(client)['ops@example.com']['id']}") + + assert r.status_code == 200 + assert r.json()["still_allowed_by_env"] is True + + def test_deleting_a_pending_request_forgets_it(db, make_user, as_user): # A pending row is deletable too — "not now, and don't keep the record" is a different decision # from Deny, which keeps the row precisely so the person cannot immediately re-ask. @@ -67,6 +111,7 @@ def test_deleting_a_pending_request_forgets_it(db, make_user, as_user): r = client.delete(f"/api/admin/invites/{_invites(client)['asker@example.com']['id']}") assert r.status_code == 200 + assert r.json()["still_allowed_by_env"] is False # nothing in the env allow-lists assert "asker@example.com" not in _invites(client) diff --git a/backend/tests/test_me_email_flag.py b/backend/tests/test_me_email_flag.py new file mode 100644 index 00000000..a63a267c --- /dev/null +++ b/backend/tests/test_me_email_flag.py @@ -0,0 +1,58 @@ +"""/api/me carries whether this instance can actually send email. + +It drives the operator warnings on the Configuration and Access-requests screens. Admin-only, +because it is an operator concern and a regular user has nothing to do with it. +""" + +from app import email as email_mod + +_PW = "correct horse battery" + + +def _signed_in(client, make_user, **kw): + """/api/me resolves its user through optional_current_user, which the as_user override does not + cover — so sign in for real and read the bootstrap payload the SPA actually gets.""" + user = make_user(password=_PW, **kw) + assert ( + client.post("/auth/password-login", json={"email": user.email, "password": _PW}).status_code + == 200 + ) + r = client.get("/api/me") + assert r.status_code == 200 and r.json()["authenticated"] is True + return r.json() + + +def test_an_admin_is_told_when_email_is_not_configured(client, make_user, monkeypatch): + monkeypatch.setattr(email_mod, "email_enabled", lambda db=None: False) + + me = _signed_in(client, make_user, email="admin@example.com", role="admin") + + assert me["email_configured"] is False + + +def test_an_admin_sees_nothing_wrong_when_email_works(client, make_user, monkeypatch): + monkeypatch.setattr(email_mod, "email_enabled", lambda db=None: True) + + me = _signed_in(client, make_user, email="admin@example.com", role="admin") + + assert me["email_configured"] is True + + +def test_a_regular_user_is_never_warned_about_the_mail_setup(client, make_user, monkeypatch): + # Not their problem, and the flag must not become a way to probe the instance's configuration. + monkeypatch.setattr(email_mod, "email_enabled", lambda db=None: False) + + me = _signed_in(client, make_user, email="plain@example.com") + + assert me["email_configured"] is True + + +def test_the_flag_reuses_the_request_session(client, make_user, monkeypatch): + # /api/me is polled once a minute per open tab; reading the SMTP settings decrypts a Fernet + # value, so it must not also check out a second pooled connection each time. + seen: list[object] = [] + monkeypatch.setattr(email_mod, "email_enabled", lambda db=None: seen.append(db) or True) + + _signed_in(client, make_user, email="admin@example.com", role="admin") + + assert seen and seen[0] is not None diff --git a/frontend/src/components/AdminUsers.tsx b/frontend/src/components/AdminUsers.tsx index 3e811fa7..807341cb 100644 --- a/frontend/src/components/AdminUsers.tsx +++ b/frontend/src/components/AdminUsers.tsx @@ -3,7 +3,7 @@ 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, type AdminUserRow, type Invite, type Me } from "../lib/api"; +import { api, HttpError, type AdminUserRow, type Invite, type Me } from "../lib/api"; import { notify } from "../lib/notifications"; import { ADMIN_USERS_TAB_KEY } from "../lib/adminUsersTab"; import Tooltip from "./Tooltip"; @@ -301,11 +301,26 @@ function AdminInvites() { }; const remove = useMutation({ mutationFn: (id: number) => api.deleteInvite(id), - onSuccess: (_d, _id) => { + onSuccess: (d) => { refresh(); - notify({ level: "success", message: t("settings.invites.deleted") }); + // Removing the row is not always a revoke: is_allowed falls back to the env allow-lists, so + // for an address on those the person keeps access. Saying "removed" alone would mislead. + if (d.still_allowed_by_env) + notify({ level: "warning", message: t("settings.invites.deletedStillAllowed") }); + else notify({ level: "success", message: t("settings.invites.deleted") }); + }, + onError: (e) => { + // A 409 means the list was stale — someone registered on that address since it loaded. Say + // what to do instead of a generic failure, and refetch so the row re-renders as read-only. + const conflict = e instanceof HttpError && e.status === 409; + notify({ + level: "error", + message: t( + conflict ? "settings.invites.deleteHasAccount" : "settings.invites.deleteFailed", + ), + }); + if (conflict) refresh(); }, - onError: () => notify({ level: "error", message: t("settings.invites.deleteFailed") }), }); const onDelete = async (i: { id: number; email: string }) => { const ok = await confirm({ @@ -395,11 +410,15 @@ function AdminInvites() { {/* 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 ? ( - - - - - + ) : ( - {/* Deny keeps the row (so the person can't immediately re-ask); delete forgets the address - entirely. Withheld once an account exists — that would revoke a live user's access. */} - {!inv.has_account && ( + {/* Deny records a decision and keeps the row; delete forgets the address. Once an account + exists for it the row is that account's approval gate, so it is managed from Users & roles + instead — shown as a disabled control rather than omitted, or the admin sees a row that is + silently missing a button their other rows have. */} + {inv.has_account ? ( + + ) : (