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.