From 8d7aaad9a8cf0a141eae42975749a6c74fa90469 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 1 Sep 2026 23:50:29 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20warn=20before=20the=20delete,=20not=20af?= =?UTF-8?q?ter=20=E2=80=94=20plus=20round-2=20copy=20and=20a11y=20correcti?= =?UTF-8?q?ons?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 on the feature diff (15 → 12): - still_allowed_by_env now rides the invite LIST as well as the delete response, so the confirmation warns while the admin can still change their mind. It mattered most for a DENIED row on an env-allow-listed address: deleting it silently converted a refusal into a grant, and the only warning arrived after the irreversible click. - Both inbox bodies claimed to name "the most recent" dropped message. The throttle records the FIRST of each hour and never updates it, so they now say "one of them", and the not-configured body finally names where to fix it. - settings.invites.emailOff told the admin they "get no notice when someone signs up" — rendered directly above the queue that shows exactly that. Only the email is lost, and it now says so. - The disabled delete buttons put their reason out of reach: `disabled` drops a control from the tab order and suppresses its title. Switched to aria-disabled + preventDefault, the pattern Playlists.tsx already established for the same reason. - deleteInvite is quiet, so the 409 the UI handles with a translated toast no longer also raises the server's English detail as a modal. - After a completed password reset, Back landed on the reset form again with a consumed token — the history change made that state reachable. The token is cleared on success so the reset view stops being the card's root. - The hu deleteHasAccount named a tab that renders under a different label. Tests: a dom test for the Access-requests panel covering both has_account variants and all three delete outcomes, /api/me flag cases, an ADMIN_EMAILS case, and the env-allowlist test now patches the raw setting so the parsing stays under test. --- backend/app/routes/admin.py | 14 +- backend/tests/test_admin_invites.py | 25 ++- frontend/src/components/AdminUsers.tsx | 20 +- frontend/src/components/Welcome.tsx | 5 +- .../src/components/adminInvites.dom.test.tsx | 192 ++++++++++++++++++ frontend/src/i18n/locales/en/inbox.json | 4 +- frontend/src/i18n/locales/en/settings.json | 5 +- frontend/src/i18n/locales/hu/inbox.json | 4 +- frontend/src/i18n/locales/hu/settings.json | 7 +- frontend/src/lib/api/admin.ts | 7 +- frontend/src/lib/releaseNotes.ts | 4 +- 11 files changed, 264 insertions(+), 23 deletions(-) create mode 100644 frontend/src/components/adminInvites.dom.test.tsx diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index 8ef7cc84..70ee72f6 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -44,6 +44,14 @@ def _accounts_for(db: Session, emails: Iterable[str]) -> set[str]: return {e.lower() for e in rows if e} +def _env_allowed(email: str) -> bool: + """Whether the env bootstrap lists still let this address in. `is_allowed` consults them only + when no Invite row exists, so this is exactly what deleting the row would fall back to — and the + admin has to know that BEFORE they confirm, not from a toast afterwards.""" + email = (email or "").lower() + return email in settings.allowed_email_set or email in settings.admin_email_set + + def _serialize(inv: Invite, *, has_account: bool = False) -> dict: return { "id": inv.id, @@ -55,6 +63,9 @@ def _serialize(inv: Invite, *, has_account: bool = False) -> dict: "decided_by": inv.decided_by, # 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 + # granting it anyway (for an ADMIN_EMAILS address, as an admin). + "still_allowed_by_env": _env_allowed(inv.email), } @@ -197,8 +208,7 @@ 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 + still_allowed_by_env = _env_allowed(inv.email) audit.record( db, admin.id, diff --git a/backend/tests/test_admin_invites.py b/backend/tests/test_admin_invites.py index 0cc31906..96d35173 100644 --- a/backend/tests/test_admin_invites.py +++ b/backend/tests/test_admin_invites.py @@ -86,20 +86,37 @@ def test_deleting_an_env_allow_listed_address_reports_that_it_kept_access( # 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"}) - ) + # Patch the raw env field, not the derived property: that keeps the split/strip/lower parsing + # under test, and matches how the rest of the suite stubs settings. + monkeypatch.setattr(settings, "allowed_emails", " OPS@Example.com , someone@else.test ") 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) + listed = _invites(client)["ops@example.com"] + # The LIST carries it too, so the confirm dialog can warn before the delete happens. + assert listed["still_allowed_by_env"] is True - r = client.delete(f"/api/admin/invites/{_invites(client)['ops@example.com']['id']}") + r = client.delete(f"/api/admin/invites/{listed['id']}") assert r.status_code == 200 assert r.json()["still_allowed_by_env"] is True +def test_an_admin_emails_address_is_also_reported(db, make_user, as_user, monkeypatch): + # ADMIN_EMAILS is the sharper case: deleting the row not only keeps access, the next Google + # sign-in re-promotes the address to admin. + from app.config import settings + + monkeypatch.setattr(settings, "admin_emails", "boss@example.com") + admin = make_user(email="admin@example.com", role="admin") + db.add(Invite(email="boss@example.com", status="approved", decided_by=admin.email)) + db.commit() + client = as_user(admin) + + assert _invites(client)["boss@example.com"]["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. diff --git a/frontend/src/components/AdminUsers.tsx b/frontend/src/components/AdminUsers.tsx index 807341cb..1cbb78c7 100644 --- a/frontend/src/components/AdminUsers.tsx +++ b/frontend/src/components/AdminUsers.tsx @@ -322,10 +322,17 @@ function AdminInvites() { if (conflict) refresh(); }, }); - const onDelete = async (i: { id: number; email: string }) => { + const onDelete = async (i: Invite) => { const ok = await confirm({ title: t("settings.invites.deleteTitle"), - message: t("settings.invites.deleteConfirm", { email: i.email }), + // Removing the row is not a revoke when the address is also in the server's env allow-lists — + // say so while the admin can still change their mind, not in a toast after the fact. + message: t( + i.still_allowed_by_env + ? "settings.invites.deleteConfirmStillAllowed" + : "settings.invites.deleteConfirm", + { email: i.email }, + ), confirmLabel: t("settings.invites.delete"), danger: true, }); @@ -410,9 +417,12 @@ 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 ? ( + // `disabled` would drop it from the tab order AND suppress the title, so the + // reason reached nobody — same call as the Playlists drag handle.