diff --git a/VERSION b/VERSION index 357b91b2..ba4cbeab 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.78.5 \ No newline at end of file +0.78.6 \ No newline at end of file diff --git a/backend/app/audit.py b/backend/app/audit.py index 1578a656..52690925 100644 --- a/backend/app/audit.py +++ b/backend/app/audit.py @@ -30,6 +30,7 @@ class AuditAction: INVITE_APPROVE = "invite_approve" INVITE_DENY = "invite_deny" INVITE_ADD = "invite_add" + INVITE_DELETE = "invite_delete" DEMO_ADD = "demo_add" DEMO_REMOVE = "demo_remove" DEMO_RESET = "demo_reset" diff --git a/backend/app/email.py b/backend/app/email.py index 219c4116..5bff78a6 100644 --- a/backend/app/email.py +++ b/backend/app/email.py @@ -3,6 +3,11 @@ Gmail SMTP + App Password by default. Deliberately fail-soft: if SMTP isn't configured or a send errors, we log and return False so the caller can fall back to in-app notifications — email is never load-bearing for the app's core flow. + +Fail-soft must not mean invisible, though: every drop also leaves an in-app notice in the admins' +inbox (see `_alert_admins`). An instance whose SMTP was never filled in silently swallows +verification links, password resets and access-request notices, and the operator's first clue used +to be a person telling them nothing arrived. """ import logging @@ -11,12 +16,60 @@ import ssl from email.message import EmailMessage from email.utils import formatdate +from sqlalchemy import select + from app import sysconfig from app.config import settings from app.db import SessionLocal +from app.notifications import create_notification +from app.ratelimit import RateLimiter log = logging.getLogger("siftlode.email") +# One admin notice an hour per reason. A relay that is down fails every send, and the inbox must +# stay readable; the log keeps the full record either way. +_alert_throttle = RateLimiter(max_events=1, window_seconds=3600) + + +def _alert_admins(reason: str, subject: str, recipients: list[str]) -> None: + """Record an in-app notice that a message did NOT go out. Best effort in every direction: it + opens its own session (callers are often background tasks), swallows its own errors, and never + reports failure to the caller — this is the fallback path, it must not become a new way to + break a send.""" + 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( + select(User.id).where( + User.role == "admin", + User.is_active.is_(True), + User.is_suspended.is_(False), + ) + ) + .scalars() + .all() + ) + if not admins: + return + for uid in admins: + create_notification( + db, + user_id=uid, + type="email_undelivered", + # English fallback; the inbox renders a translated version from `data`. + title="Email could not be sent", + body=subject, + data={"reason": reason, "subject": subject, "recipients": recipients}, + commit=False, + ) + db.commit() + except Exception: + 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 @@ -47,6 +100,7 @@ def _send(to: list[str], subject: str, body: str, reply_to: str | None = None) - c = _smtp() if not (c["host"] and c["user"] and c["password"]): log.info("SMTP not configured; skipping email %r to %s", subject, recipients) + _alert_admins("not_configured", subject, recipients) return False msg = EmailMessage() msg["Subject"] = subject @@ -67,6 +121,7 @@ def _send(to: list[str], subject: str, body: str, reply_to: str | None = None) - return True except Exception: log.exception("SMTP send failed (%r to %s)", subject, recipients) + _alert_admins("send_failed", subject, recipients) return False diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index e4fcf8c5..fbdbd878 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -1,6 +1,7 @@ """Admin-only onboarding: review access requests (Invites), approve/deny, manual add. Also the demo-account admin surface: the demo email whitelist + a manual state reset.""" +from collections.abc import Iterable from datetime import datetime, timezone from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException @@ -31,7 +32,18 @@ from app.utils import iso, valid_email router = APIRouter(prefix="/api/admin", tags=["admin"]) -def _serialize(inv: Invite) -> dict: +def _accounts_for(db: Session, emails: Iterable[str]) -> set[str]: + """Which of these addresses already have an account. Batched, so listing the invites costs one + extra query. An invite whose address has an account must not be deletable: `is_allowed` reads + this row, so removing it would silently revoke a live user's access.""" + wanted = {e.lower() for e in emails if e} + if not wanted: + return set() + rows = db.execute(select(User.email).where(User.email.in_(wanted))).scalars() + return {e.lower() for e in rows if e} + + +def _serialize(inv: Invite, *, has_account: bool = False) -> dict: return { "id": inv.id, "email": inv.email, @@ -40,6 +52,8 @@ def _serialize(inv: Invite) -> dict: "requested_at": iso(inv.requested_at), "decided_at": iso(inv.decided_at), "decided_by": inv.decided_by, + # Drives the delete affordance: an entry backed by an account is not removable here. + "has_account": has_account, } @@ -56,7 +70,8 @@ def list_invites( rows = db.execute(stmt.order_by(Invite.requested_at.desc())).scalars().all() order = {"pending": 0, "approved": 1, "denied": 2} rows.sort(key=lambda i: order.get(i.status, 9)) - return [_serialize(i) for i in rows] + accounts = _accounts_for(db, (i.email for i in rows)) + return [_serialize(i, has_account=(i.email or "").lower() in accounts) for i in rows] def _activate_user_by_email(db: Session, email: str) -> None: @@ -107,7 +122,7 @@ def approve_invite( 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) - return _serialize(inv) + return _serialize(inv, has_account=bool(_accounts_for(db, [inv.email]))) @router.post("/invites/{invite_id}/deny") @@ -121,7 +136,7 @@ def deny_invite( # 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) - return _serialize(inv) + return _serialize(inv, has_account=bool(_accounts_for(db, [inv.email]))) class AddInviteIn(BaseModel): @@ -150,7 +165,41 @@ def add_invite( ) db.commit() _activate_user_by_email(db, email) - return _serialize(inv) + return _serialize(inv, has_account=bool(_accounts_for(db, [email]))) + + +@router.delete("/invites/{invite_id}") +def delete_invite( + invite_id: int, + admin: User = Depends(admin_user), + 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. + + 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.""" + inv = db.get(Invite, invite_id) + if inv is None: + raise HTTPException(status_code=404, detail="No such invite") + if _accounts_for(db, [inv.email]): + raise HTTPException( + status_code=409, + detail="That address already has an account — delete the account instead.", + ) + audit.record( + db, + admin.id, + AuditAction.INVITE_DELETE, + target_type="invite", + target_id=inv.id, + summary=inv.email, + ) + db.delete(inv) + db.commit() + return {"deleted": invite_id} # --- Users & roles -------------------------------------------------------------------- diff --git a/backend/app/routes/me.py b/backend/app/routes/me.py index 595930d7..ea7cbf0c 100644 --- a/backend/app/routes/me.py +++ b/backend/app/routes/me.py @@ -13,6 +13,7 @@ from app.auth import ( optional_current_user, purge_user, ) +from app import email as email_mod from app import sysconfig from app.db import get_db from app.models import Invite, User @@ -98,11 +99,16 @@ def get_me( if user is None: return {"authenticated": False} pending_invites = 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. + email_configured = True if user.role == "admin": pending_invites = ( db.scalar(select(func.count()).select_from(Invite).where(Invite.status == "pending")) or 0 ) + email_configured = email_mod.email_enabled() return { "authenticated": True, "id": user.id, @@ -121,6 +127,7 @@ def get_me( "can_read": has_read_scope(user), "can_write": has_write_scope(user), "pending_invites": pending_invites, + "email_configured": email_configured, "preferences": user.preferences or {}, } diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 54b967ff..0dde097b 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -196,13 +196,17 @@ def _reset_rate_limiters(): """Every RateLimiter is a module-level singleton keyed by client IP, and TestClient always presents the same host — so without this the whole suite's login/register attempts pool into one 300s window and, past the cap, some unrelated test starts failing with a 429 depending on order. - Cleared before each test; a test that wants to exercise throttling can still exhaust one itself.""" - import app.routes.auth as _auth - import app.routes.messages as _messages - import app.routes.public as _public + Cleared before each test; a test that wants to exercise throttling can still exhaust one itself. + + Sweeps every loaded `app.*` module rather than a hand-kept list, so a limiter added in a new + module is covered the day it lands instead of silently pooling until something fails.""" + import sys + from app.ratelimit import RateLimiter - for mod in (_auth, _messages, _public): + for name, mod in list(sys.modules.items()): + if name != "app" and not name.startswith("app."): + continue for obj in vars(mod).values(): if isinstance(obj, RateLimiter): obj.reset() diff --git a/backend/tests/test_admin_invites.py b/backend/tests/test_admin_invites.py new file mode 100644 index 00000000..16620b86 --- /dev/null +++ b/backend/tests/test_admin_invites.py @@ -0,0 +1,106 @@ +"""Deleting a whitelist / access-request entry (admin). + +Deny keeps the row so a refused person cannot immediately re-ask; delete forgets the address +outright, for an approval added by mistake or one that is no longer wanted. The one hard rule is +that an entry backed by an account is NOT deletable here: `is_allowed` reads this row, so removing +it would silently revoke a live user's access. +""" + +from sqlalchemy import select + +from app.models import Invite, User + + +def _invites(client): + r = client.get("/api/admin/invites") + assert r.status_code == 200 + return {i["email"]: i for i in r.json()} + + +def test_admin_deletes_a_whitelist_entry_that_never_became_an_account(db, make_user, as_user): + admin = make_user(email="admin@example.com", role="admin") + db.add(Invite(email="mistake@example.com", status="approved", decided_by=admin.email)) + db.commit() + client = as_user(admin) + listed = _invites(client)["mistake@example.com"] + assert listed["has_account"] is False # drives the delete affordance in the UI + + r = client.delete(f"/api/admin/invites/{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() + is None + ) + + +def test_deleting_an_entry_that_has_an_account_is_refused(db, make_user, as_user): + # The guard that matters: this row is what is_allowed() reads, so deleting it would revoke the + # user's access without touching the account or saying so. + admin = make_user(email="admin@example.com", role="admin") + member = make_user(email="member@example.com") + db.add(Invite(email=member.email, status="approved", decided_by=admin.email)) + db.commit() + client = as_user(admin) + listed = _invites(client)[member.email] + assert listed["has_account"] is True + + 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 == member.email)).scalar_one_or_none() + is not None + ) + + +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. + admin = make_user(email="admin@example.com", role="admin") + db.add(Invite(email="asker@example.com", status="pending")) + db.commit() + client = as_user(admin) + + r = client.delete(f"/api/admin/invites/{_invites(client)['asker@example.com']['id']}") + + assert r.status_code == 200 + assert "asker@example.com" not in _invites(client) + + +def test_deleting_a_missing_invite_is_404(db, make_user, as_user): + admin = make_user(email="admin@example.com", role="admin") + assert as_user(admin).delete("/api/admin/invites/9999").status_code == 404 + + +def test_a_regular_user_cannot_delete_an_invite(db, make_user, as_user): + make_user(email="admin@example.com", role="admin") + plain = make_user(email="plain@example.com") + db.add(Invite(email="mistake@example.com", status="approved")) + db.commit() + inv_id = db.execute(select(Invite.id).where(Invite.email == "mistake@example.com")).scalar_one() + + assert as_user(plain).delete(f"/api/admin/invites/{inv_id}").status_code == 403 + + db.expire_all() + assert db.get(Invite, inv_id) is not None + + +def test_deleting_the_user_also_clears_the_way_to_re_invite(db, make_user, as_user): + # The refusal above tells the admin to delete the account instead; purge_user takes the invite + # with it, so the address really is forgotten and a fresh request starts over as pending. + admin = make_user(email="admin@example.com", role="admin") + member = make_user(email="member@example.com") + db.add(Invite(email=member.email, status="approved", decided_by=admin.email)) + db.commit() + client = as_user(admin) + + assert client.delete(f"/api/admin/users/{member.id}").status_code == 200 + + db.expire_all() + assert db.execute(select(User).where(User.email == member.email)).scalar_one_or_none() is None + assert ( + db.execute(select(Invite).where(Invite.email == member.email)).scalar_one_or_none() is None + ) diff --git a/backend/tests/test_email_alerts.py b/backend/tests/test_email_alerts.py new file mode 100644 index 00000000..3eddb99a --- /dev/null +++ b/backend/tests/test_email_alerts.py @@ -0,0 +1,115 @@ +"""An undelivered email leaves a trace in the admins' inbox. + +Sending is deliberately fail-soft — a missing SMTP setting or a rejecting relay logs and returns +False so nothing user-facing breaks. The cost of that used to be total silence: an instance whose +SMTP was never filled in swallowed verification links, password resets and access-request notices, +and the operator's first clue was somebody saying nothing arrived. +""" + +import smtplib + +import pytest +from sqlalchemy import select + +from app import email as email_mod +from app.models import Notification + + +@pytest.fixture(autouse=True) +def _no_smtp_config(monkeypatch): + """Every test here drives the real `_send`, so make sure it never reaches a socket.""" + monkeypatch.setattr( + email_mod, + "_smtp", + lambda: {"host": "", "port": 587, "user": "", "password": "", "from": ""}, + ) + + +def _notifications(db, user_id: int) -> list[Notification]: + return list( + db.execute( + select(Notification) + .where(Notification.user_id == user_id, Notification.type == "email_undelivered") + .order_by(Notification.id) + ) + .scalars() + .all() + ) + + +def test_a_skipped_email_notifies_every_admin(db, make_user): + admin_a = make_user(email="admin-a@example.com", role="admin") + 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 + + db.expire_all() + for admin in (admin_a, admin_b): + (n,) = _notifications(db, admin.id) + assert n.data["reason"] == "not_configured" + assert n.data["subject"] == "Siftlode SMTP test" + assert n.data["recipients"] == ["someone@example.com"] + assert _notifications(db, plain.id) == [] # not an operator's problem to see + + +def test_the_notice_is_throttled_so_a_dead_relay_cannot_flood_the_inbox(db, make_user): + admin = make_user(email="admin@example.com", role="admin") + + for _ in range(5): + email_mod.send_test("someone@example.com") + + db.expire_all() + assert len(_notifications(db, admin.id)) == 1 + + +def test_a_send_failure_is_reported_separately_from_a_missing_config(db, make_user, monkeypatch): + # A configured-but-rejecting relay is a different operator problem from an empty config, and the + # inbox copy differs — so the reason has to survive to the notification. + admin = make_user(email="admin@example.com", role="admin") + monkeypatch.setattr( + email_mod, + "_smtp", + lambda: { + "host": "smtp.example.com", + "port": 587, + "user": "u", + "password": "p", + "from": "Siftlode ", + }, + ) + + def _boom(*a, **kw): + raise smtplib.SMTPException("nope") + + monkeypatch.setattr(email_mod.smtplib, "SMTP", _boom) + + assert email_mod.send_test("someone@example.com") is False + + db.expire_all() + (n,) = _notifications(db, admin.id) + assert n.data["reason"] == "send_failed" + + +def test_no_admins_means_no_notification_and_no_error(db, make_user): + # A fresh instance can have no admin yet (the wizard has not run). The alert must stay silent + # 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 + + db.expire_all() + assert _notifications(db, plain.id) == [] + + +def test_a_broken_alert_never_breaks_the_send(db, make_user, monkeypatch): + # The alert is the fallback path; if IT fails, the caller must still get the ordinary fail-soft + # False rather than an exception surfacing from a background task. + make_user(email="admin@example.com", role="admin") + + def _boom(*a, **kw): + raise RuntimeError("notification store is down") + + monkeypatch.setattr(email_mod, "create_notification", _boom) + + assert email_mod.send_test("someone@example.com") is False diff --git a/frontend/src/components/AdminUsers.tsx b/frontend/src/components/AdminUsers.tsx index 445dea8a..3e811fa7 100644 --- a/frontend/src/components/AdminUsers.tsx +++ b/frontend/src/components/AdminUsers.tsx @@ -7,6 +7,7 @@ import { api, 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"; +import InlineNotice from "./InlineNotice"; import { Section } from "./ui/form"; import { LoadingState, StateMessage } from "./QueryState"; import { useConfirm } from "./ConfirmProvider"; @@ -266,6 +267,7 @@ function AdminInvites() { const { t } = useTranslation(); const qc = useQueryClient(); const confirm = useConfirm(); + const me = useMe(); const invites = useQuery({ queryKey: qk.adminInvites(), queryFn: () => api.adminInvites() }); const [newEmail, setNewEmail] = useState(""); const refresh = () => { @@ -297,6 +299,23 @@ function AdminInvites() { }); if (ok) deny.mutate(i.id); }; + const remove = useMutation({ + mutationFn: (id: number) => api.deleteInvite(id), + onSuccess: (_d, _id) => { + refresh(); + notify({ level: "success", message: t("settings.invites.deleted") }); + }, + onError: () => notify({ level: "error", message: t("settings.invites.deleteFailed") }), + }); + const onDelete = async (i: { id: number; email: string }) => { + const ok = await confirm({ + title: t("settings.invites.deleteTitle"), + message: t("settings.invites.deleteConfirm", { email: i.email }), + confirmLabel: t("settings.invites.delete"), + danger: true, + }); + if (ok) remove.mutate(i.id); + }; const add = useMutation({ mutationFn: (email: string) => api.addInvite(email), onSuccess: (_d, email) => { @@ -313,6 +332,9 @@ function AdminInvites() { return (
+ {!me.email_configured && ( + {t("settings.invites.emailOff")} + )} {invites.isError && !invites.data ? ( invites.refetch()}> {t("common.loadError")} @@ -329,7 +351,8 @@ function AdminInvites() { inv={i} onApprove={() => approve.mutate({ id: i.id, email: i.email })} onDeny={() => onDeny(i)} - busy={approve.isPending || deny.isPending} + onDelete={() => onDelete(i)} + busy={approve.isPending || deny.isPending || remove.isPending} /> ))} @@ -364,11 +387,31 @@ function AdminInvites() {
{decided.map((i) => ( -
- {i.email} +
+ {i.email} {i.status} + {/* 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 ? ( + + + + + + ) : ( + + + + )}
))}
@@ -506,11 +549,13 @@ function InviteRow({ inv, onApprove, onDeny, + onDelete, busy, }: { inv: Invite; onApprove: () => void; onDeny: () => void; + onDelete: () => void; busy: boolean; }) { const { t } = useTranslation(); @@ -546,6 +591,20 @@ function InviteRow({ + {/* 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 && ( + + + + )}
); } diff --git a/frontend/src/components/ConfigPanel.tsx b/frontend/src/components/ConfigPanel.tsx index 8f3cff33..3c344c9c 100644 --- a/frontend/src/components/ConfigPanel.tsx +++ b/frontend/src/components/ConfigPanel.tsx @@ -6,6 +6,8 @@ import { Plug, RotateCcw, Send } from "lucide-react"; import { api, type ConfigItem, type PlexTestResult } from "../lib/api"; import { notify } from "../lib/notifications"; import Tooltip from "./Tooltip"; +import InlineNotice from "./InlineNotice"; +import { useMe } from "../lib/useMe"; import Tabs, { usePersistedTab } from "./Tabs"; import { LoadingState, StateMessage } from "./QueryState"; import { PageToolbar } from "./PageShell"; @@ -35,6 +37,7 @@ const GROUP_ORDER = [ export default function ConfigPanel() { const { t } = useTranslation(); const qc = useQueryClient(); + const me = useMe(); const q = useQuery({ queryKey: qk.adminConfig(), queryFn: api.adminConfig }); const data = q.data; @@ -204,7 +207,11 @@ export default function ConfigPanel() { {activeGroup === "email" && ( -
+
+ {/* The one place an operator can see that mail is off. Sending is fail-soft, so a + half-filled config costs verification links, resets and access-request notices + with no error anywhere — say so next to the fields that fix it. */} + {!me.email_configured && {t("config.emailOff")}}