diff --git a/VERSION b/VERSION index 1458c369..ba4cbeab 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.78.4 \ 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..b41f51d1 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,28 +16,88 @@ import ssl 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 log = logging.getLogger("siftlode.email") - -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"), - } +# 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 email_enabled() -> bool: - c = _smtp() +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: + 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: + # 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( + 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: + _alert_throttle.reset() + log.exception("Could not record the undelivered-email notice (%r)", subject) + + +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 _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"]) @@ -47,6 +112,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 +133,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/ratelimit.py b/backend/app/ratelimit.py index 4ec3a2b9..d88e22da 100644 --- a/backend/app/ratelimit.py +++ b/backend/app/ratelimit.py @@ -18,6 +18,13 @@ class RateLimiter: self._hits: dict[str, list[float]] = {} self._lock = threading.Lock() + def reset(self) -> None: + """Forget every recorded attempt. Each limiter is a module-level singleton, so a test suite + sharing one process would otherwise pool all of its attempts into a single window and + eventually trip an unrelated test with a 429.""" + with self._lock: + self._hits.clear() + def allow(self, key: str) -> bool: """Record an attempt for ``key`` and return whether it is within the limit. diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index e4fcf8c5..70ee72f6 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 @@ -18,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 ( @@ -31,7 +33,26 @@ 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 _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, "email": inv.email, @@ -40,6 +61,11 @@ 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, + # …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), } @@ -56,7 +82,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 +134,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 +148,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 +177,49 @@ 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). 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 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") + if _accounts_for(db, [inv.email]): + raise HTTPException( + status_code=409, + detail="That address already has an account — delete the account instead.", + ) + still_allowed_by_env = _env_allowed(inv.email) + 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, "still_allowed_by_env": still_allowed_by_env} # --- Users & roles -------------------------------------------------------------------- diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index c0361fed..58760f3b 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -636,12 +636,25 @@ def _register_account(email: str, password: str) -> None: # no-op as the pre-check hit — roll back and stop. db.rollback() return - upsert_pending_invite(db, email) # admin-approval gate (commits) + upsert_pending_invite(db, email) # admin-approval gate + # Persist the account ourselves. upsert_pending_invite commits only when it actually writes an + # invite row — not when this address already has a pending or approved one — and _issue_token + # below only runs with SMTP on. So for an address the admin had already approved (or one whose + # request was still pending), on an instance with no working SMTP, NOTHING along this path + # committed and the flushed user was silently rolled back when the session closed: the + # would-be user got the uniform "ok" and no account, and re-registering did the same again. + # 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}") 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) diff --git a/backend/app/routes/me.py b/backend/app/routes/me.py index 595930d7..4761a6f6 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(db) # this request's session; no second connect 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 8a75d48b..0dde097b 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -189,3 +189,24 @@ def as_user(db, override_dep, _configured): return TestClient(app, headers={"x-test-user": str(user.id)}) return _as + + +@pytest.fixture(autouse=True) +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. + + 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 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..2786aa4e --- /dev/null +++ b/backend/tests/test_admin_invites.py @@ -0,0 +1,170 @@ +"""Deleting a whitelist / access-request entry (admin). + +Deny records an auditable decision and keeps the row; delete forgets the address outright, for an +approval added by mistake or one that is no longer wanted. Neither stops the person asking again — +`upsert_pending_invite` reopens a denied row as pending, and a deleted one comes back as a fresh +request. The one hard rule is that an entry backed by an account is NOT deletable here: this row is +that account's approval gate, and orphaning it would strand someone who could then neither sign in +nor be approved. +""" + +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_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 + + # 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/{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 — "drop this without recording a decision" is a different + # choice from Deny, which stores a decision (status, decided_by) and mails the requester. + 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 r.json()["still_allowed_by_env"] is False # nothing in the env allow-lists + 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_auth_session.py b/backend/tests/test_auth_session.py index 150f6ad9..b4e8adc0 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -127,3 +127,217 @@ def test_activating_a_verified_account_keeps_its_password(db, make_user): _activate_user_by_email(db, "legit@example.com") assert user.is_active is True assert user.password_hash is not None + + +def _register_and_fetch(db, email: str): + """Run the /register background worker, then read the users/invites rows back through a FRESH + query. The worker owns its own session, so anything it failed to commit is simply not there.""" + from sqlalchemy import select as _select + + from app.models import Invite, User + from app.routes.auth import _register_account + + _register_account(email, _PW) + db.expire_all() + user = db.execute(_select(User).where(User.email == email)).scalar_one_or_none() + invite = db.execute(_select(Invite).where(Invite.email == email)).scalar_one_or_none() + return user, invite + + +def _no_smtp(monkeypatch): + """Put the worker on the no-SMTP path and capture the admin notice instead of sending it.""" + from app.routes import auth as auth_mod + + 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 + ) + return notified + + +def test_registration_of_a_pre_approved_email_persists_the_account_without_smtp( + db, make_user, monkeypatch +): + # THE regression. An admin whitelists an address, the person registers, and the account silently + # never existed: upsert_pending_invite leaves an already-approved row alone and returns WITHOUT + # committing, and the only other commit on this path (_issue_token) is gated on SMTP — so with no + # working mail the flushed user was rolled back at session close. The sign-up answered "ok", + # created nothing, and re-registering did the same again. + from app.models import Invite + + notified = _no_smtp(monkeypatch) + make_user(email="admin@example.com", role="admin") + db.add(Invite(email="roland@example.com", status="approved", decided_by="admin@example.com")) + db.commit() + + user, invite = _register_and_fetch(db, "roland@example.com") + + assert user is not None, "pre-approved registration was rolled back — the account is gone" + assert user.password_hash is not None + assert user.email_verified is True # no SMTP => ownership can't be proven by link + assert ( + user.is_active is False + ) # approval is bound to the account by an admin, not by signing up + # The admin's standing decision is left exactly as it was — is_allowed() still reads "approved", + # so their Google sign-in keeps working and an anonymous sign-up cannot revoke access. + assert invite.status == "approved" + assert invite.decided_by == "admin@example.com" + # The notify path still fires for an already-approved address — nothing lands in the queue, so + # it is the only announcement there is. (Whether it is DELIVERED is separate: on a real no-SMTP + # instance `_send` drops it, which is the open half of the problem, roadmap UX-25.) + assert notified == ["roland@example.com"] + + +def test_registration_with_an_already_pending_request_persists_the_account( + db, make_user, monkeypatch +): + # The same rollback, reached the other way: someone uses Request access, then registers before + # the admin has decided. upsert_pending_invite leaves the pending row alone and does not commit. + from app.models import Invite + + notified = _no_smtp(monkeypatch) + make_user(email="admin@example.com", role="admin") + db.add(Invite(email="pia@example.com", status="pending")) + db.commit() + + user, invite = _register_and_fetch(db, "pia@example.com") + + assert user is not None, "registration was rolled back — the account is gone" + assert user.is_active is False + assert invite.status == "pending" + assert notified == ["pia@example.com"] + + +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. + from app.models import Invite + from app.routes.admin import _activate_user_by_email + + _no_smtp(monkeypatch) + db.add(Invite(email="roland@example.com", status="approved", decided_by="admin@example.com")) + db.commit() + user, _ = _register_and_fetch(db, "roland@example.com") + + _activate_user_by_email(db, "roland@example.com") + + db.expire_all() + assert user.is_active is True + assert user.password_hash is not None # a verified account keeps its own password + + +def test_registration_of_an_unknown_email_still_persists_account_and_pending_invite( + db, make_user, monkeypatch +): + # Guard, not a regression test: this branch always worked (upsert INSERTs the invite and commits + # the same transaction). It must keep working — and it is the case that DOES notify the admin. + notified = _no_smtp(monkeypatch) + make_user(email="admin@example.com", role="admin") + + user, invite = _register_and_fetch(db, "stranger@example.com") + + assert user is not None + assert user.is_active is False + assert invite is not None and invite.status == "pending" + assert notified == ["stranger@example.com"] + + +def test_registration_of_a_previously_denied_email_revives_the_request(db, make_user, monkeypatch): + # Guard: a denied person may ask again — the invite flips back to pending (and commits) so the + # admin sees a fresh request. The stale decision must not leak into the new account's state. + from app.models import Invite + + notified = _no_smtp(monkeypatch) + make_user(email="admin@example.com", role="admin") + db.add(Invite(email="denis@example.com", status="denied", decided_by="admin@example.com")) + db.commit() + + user, invite = _register_and_fetch(db, "denis@example.com") + + assert user is not None + assert user.is_active is False + assert invite.status == "pending" + assert invite.decided_at is None and invite.decided_by is None + assert notified == ["denis@example.com"] + + +def test_registration_of_a_pre_approved_email_persists_the_account_with_smtp(db, monkeypatch): + # Guard, not a regression test: with SMTP on, _issue_token's own commit already persisted the + # user, so this branch was never broken and must stay unbroken. + from app.models import Invite + from app.routes import auth as auth_mod + + 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_admin_new_request", lambda admins, to: True) + db.add(Invite(email="rita@example.com", status="approved", decided_by="admin@example.com")) + db.commit() + + user, invite = _register_and_fetch(db, "rita@example.com") + + assert user is not None + assert user.email_verified is False # must confirm the address + assert user.is_active is False + assert invite.status == "approved" + assert sent == ["rita@example.com"] + + +def test_verifying_an_email_never_grants_access_on_its_own(client, db, make_user): + # Verification attests the address; it does not decide access. The two gates stay separate. + from app.models import Invite + from app.routes.auth import _issue_token + + user = make_user(email="rita@example.com", password=_PW, email_verified=False, is_active=False) + db.add(Invite(email="rita@example.com", status="approved", decided_by="admin@example.com")) + db.commit() + raw = _issue_token(db, user, "verify", timedelta(hours=1)) + + assert client.post("/auth/verify", json={"token": raw}).status_code == 200 + + db.expire_all() + assert user.email_verified is True + assert user.is_active is False + assert _login(client, "rita@example.com", _PW).status_code == 403 + + +def test_register_route_creates_the_account_end_to_end(client, db, monkeypatch): + # The tests above call _register_account directly, so nothing covered the ROUTE: its + # allow_registration gate, its rate limiter and the background.add_task wiring that makes the + # whole flow reachable at all. TestClient runs BackgroundTasks after the response, so a single + # POST exercises the real path end to end. + from sqlalchemy import select as _select + + from app.models import User + + _no_smtp(monkeypatch) + + r = client.post("/auth/register", json={"email": "newbie@example.com", "password": _PW}) + + assert r.status_code == 200 and r.json() == {"status": "ok"} + db.expire_all() + user = db.execute(_select(User).where(User.email == "newbie@example.com")).scalar_one_or_none() + assert user is not None, "POST /auth/register answered ok but created nothing" + assert user.password_hash is not None + + +def test_register_route_is_refused_when_registration_is_closed(client, db, monkeypatch): + # The gate is an explicit 403 with nothing created — not a uniform "ok" that silently drops it. + from sqlalchemy import select as _select + + from app import sysconfig + from app.models import User + + _no_smtp(monkeypatch) + sysconfig.set_value(db, "allow_registration", False) + + r = client.post("/auth/register", json={"email": "closed@example.com", "password": _PW}) + + assert r.status_code == 403 + db.expire_all() + assert ( + db.execute(_select(User).where(User.email == "closed@example.com")).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..f8b66c94 --- /dev/null +++ b/backend/tests/test_email_alerts.py @@ -0,0 +1,149 @@ +"""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_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 + admin = make_user(email="admin@example.com", role="admin") + + assert email_mod.send_test("someone@example.com") is False + + db.expire_all() + assert len(_notifications(db, admin.id)) == 1 + + +def test_a_failed_alert_does_not_burn_the_hour_either(db, make_user, monkeypatch): + admin = make_user(email="admin@example.com", role="admin") + boom = {"on": True} + + def _maybe_boom(*a, **kw): + if boom["on"]: + raise RuntimeError("notification store is down") + from app.notifications import create_notification as real + + 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 + boom["on"] = False + + assert email_mod.send_test("someone@example.com") is False + + db.expire_all() + assert len(_notifications(db, admin.id)) == 1 + + +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/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/backend/tests/test_ratelimit.py b/backend/tests/test_ratelimit.py new file mode 100644 index 00000000..5102318e --- /dev/null +++ b/backend/tests/test_ratelimit.py @@ -0,0 +1,65 @@ +"""The sliding-window limiter behind /auth/register, password-login, password-reset and demo. + +Pure logic, no DB. `reset()` in particular is only ever called by a test fixture (conftest clears +the module-level limiters between tests), so without a test here a refactor could turn it into a +no-op and nothing would notice until an unrelated test started failing with a 429. +""" + +from app import ratelimit +from app.ratelimit import RateLimiter + + +def test_allow_trips_at_the_cap_and_stays_tripped(): + lim = RateLimiter(max_events=2, window_seconds=300) + + assert lim.allow("ip") is True + assert lim.allow("ip") is True + assert lim.allow("ip") is False + assert lim.allow("ip") is False + + +def test_hammering_while_blocked_does_not_push_the_window_forward(monkeypatch): + # A blocked attempt must not be recorded, otherwise someone who keeps retrying holds their own + # window open forever — an indefinite lockout for anyone sharing that IP. Only observable across + # the window boundary, so drive the clock: two attempts fill the cap, ten blocked retries land + # just before it expires, and the caller must still be let back in on the far side. + clock = {"t": 1000.0} + monkeypatch.setattr(ratelimit.time, "monotonic", lambda: clock["t"]) + lim = RateLimiter(max_events=2, window_seconds=60) + + assert lim.allow("ip") is True + assert lim.allow("ip") is True + for _ in range(10): + clock["t"] += 5 # still inside the window; every one of these is refused + assert lim.allow("ip") is False + + clock["t"] = 1000.0 + 61 # past the window of the two attempts that actually counted + assert lim.allow("ip") is True + + +def test_keys_are_independent(): + lim = RateLimiter(max_events=1, window_seconds=300) + + assert lim.allow("a") is True + assert lim.allow("a") is False + assert lim.allow("b") is True + + +def test_reset_clears_every_key(): + lim = RateLimiter(max_events=1, window_seconds=300) + lim.allow("a") + lim.allow("b") + assert lim.allow("a") is False + + lim.reset() + + assert lim.allow("a") is True + assert lim.allow("b") is True + + +def test_expired_attempts_fall_out_of_the_window(): + # A zero-length window means every previous attempt is already outside it. + lim = RateLimiter(max_events=1, window_seconds=0) + + assert lim.allow("ip") is True + assert lim.allow("ip") is True diff --git a/frontend/src/components/AdminUsers.tsx b/frontend/src/components/AdminUsers.tsx index 445dea8a..1cbb78c7 100644 --- a/frontend/src/components/AdminUsers.tsx +++ b/frontend/src/components/AdminUsers.tsx @@ -3,10 +3,11 @@ 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"; +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,45 @@ function AdminInvites() { }); if (ok) deny.mutate(i.id); }; + const remove = useMutation({ + mutationFn: (id: number) => api.deleteInvite(id), + onSuccess: (d) => { + refresh(); + // 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(); + }, + }); + const onDelete = async (i: Invite) => { + const ok = await confirm({ + title: t("settings.invites.deleteTitle"), + // 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, + }); + if (ok) remove.mutate(i.id); + }; const add = useMutation({ mutationFn: (email: string) => api.addInvite(email), onSuccess: (_d, email) => { @@ -313,6 +354,9 @@ function AdminInvites() { return (
+ {!me.email_configured && ( + {t("settings.invites.emailOff")} + )} {invites.isError && !invites.data ? ( invites.refetch()}> {t("common.loadError")} @@ -329,7 +373,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 +409,38 @@ 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 ? ( + // `disabled` would drop it from the tab order AND suppress the title, so the + // reason reached nobody — same call as the Playlists drag handle. + + ) : ( + + + + )}
))}
@@ -506,11 +578,13 @@ function InviteRow({ inv, onApprove, onDeny, + onDelete, busy, }: { inv: Invite; onApprove: () => void; onDeny: () => void; + onDelete: () => void; busy: boolean; }) { const { t } = useTranslation(); @@ -546,6 +620,34 @@ function InviteRow({ + {/* 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 ? ( + // aria-disabled, not `disabled`: the reason has to stay reachable by keyboard. + + ) : ( + + + + )}
); } diff --git a/frontend/src/components/ConfigPanel.tsx b/frontend/src/components/ConfigPanel.tsx index 8f3cff33..f16f1631 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; @@ -100,6 +103,10 @@ export default function ConfigPanel() { } } await qc.invalidateQueries({ queryKey: qk.adminConfig() }); + // The email-not-configured notice reads me.email_configured, which is otherwise only + // refetched on its own 60s interval — without this it keeps warning about fields the admin + // has just filled in, which reads as the save not having taken. + await qc.invalidateQueries({ queryKey: qk.me() }); setReseedToken((n) => n + 1); // re-seed the draft from the saved server state setSaveState("saved"); setTimeout(() => setSaveState((s) => (s === "saved" ? "idle" : s)), 2500); @@ -204,7 +211,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")}}