From d954e71a770b5698c00887bb5c767c56ae1787c9 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 1 Sep 2026 16:25:05 +0200 Subject: [PATCH 01/12] fix(auth): commit the account on registration so a pre-approved sign-up isn't lost MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _register_account() flushed the new User but relied on someone else to commit: upsert_pending_invite() commits only when it actually writes an invite row, and _issue_token() runs only when SMTP is enabled. For an email an admin had ALREADY approved, on an instance where email is unavailable, neither fired — the session closed and the account was rolled back. The would-be user got the uniform "ok" and no account, could not sign in, and re-registering was a silent no-op. Commit explicitly after the invite upsert (a no-op when upsert already committed the same transaction). Three DB-lane regression tests cover the pre-approved no-SMTP path (fails without the fix), the unknown-email path that always worked, and the pre-approved path with SMTP on. --- VERSION | 2 +- backend/app/routes/auth.py | 9 +++- backend/tests/test_auth_session.py | 69 ++++++++++++++++++++++++++++++ frontend/src/lib/releaseNotes.ts | 13 ++++++ 4 files changed, 91 insertions(+), 2 deletions(-) diff --git a/VERSION b/VERSION index 1458c369..357b91b2 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.78.4 \ No newline at end of file +0.78.5 \ No newline at end of file diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index c0361fed..294a19ac 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -636,7 +636,14 @@ 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 (commits only when it writes a row) + # Persist the account ourselves. upsert_pending_invite commits only when it actually inserts + # (or revives) an invite row, and _issue_token below only runs with SMTP on — so for an email + # the admin had ALREADY approved, on an instance with no SMTP, nothing along this path + # committed and the flushed user was silently rolled back when the session closed. The + # would-be user got a uniform "ok" and no account. 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). diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 150f6ad9..652682aa 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -127,3 +127,72 @@ 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 test_registration_of_a_pre_approved_email_persists_the_account_without_smtp(db, monkeypatch): + # The regression: an admin pre-approves an address, the person then registers, and the account + # silently never existed. upsert_pending_invite returns early WITHOUT committing for an + # already-approved email, and the only other commit on this path (_issue_token) is gated on + # SMTP — so on a no-SMTP instance the flushed user was rolled back at session close. + from app.models import Invite + from app.routes import auth as auth_mod + + monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False) + 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.is_active is False # admin approval still gates sign-in + assert user.password_hash is not None + assert user.email_verified is True # no SMTP => ownership can't be proven by link + assert invite.status == "approved" # an existing approval is never disturbed + + +def test_registration_of_an_unknown_email_still_persists_account_and_pending_invite( + db, monkeypatch +): + # The branch that always worked (upsert INSERTs the invite and commits) must keep working. + from app.routes import auth as auth_mod + + monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False) + + user, invite = _register_and_fetch(db, "stranger@example.com") + + assert user is not None + assert invite is not None and invite.status == "pending" + + +def test_registration_of_a_pre_approved_email_persists_the_account_with_smtp(db, monkeypatch): + # Same pre-approved path with SMTP on: the account must persist AND get a verification link. + 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, _ = _register_and_fetch(db, "rita@example.com") + + assert user is not None + assert user.email_verified is False # must confirm the address + assert sent == ["rita@example.com"] diff --git a/frontend/src/lib/releaseNotes.ts b/frontend/src/lib/releaseNotes.ts index b09d95a6..d122adff 100644 --- a/frontend/src/lib/releaseNotes.ts +++ b/frontend/src/lib/releaseNotes.ts @@ -14,6 +14,19 @@ export interface ReleaseEntry { } export const RELEASE_NOTES: ReleaseEntry[] = [ + { + version: "0.78.5", + date: "2026-09-01", + summary: + "Account emails are being delivered again — and a sign-up by someone the admin had already approved no longer vanishes without a trace.", + fixes: [ + "Email is going out again. Verification links, password-reset links, approval and denial notices, and the “new access request” alert to the admin were all being silently dropped because the mail settings were incomplete — the app skipped each message instead of sending it. Check Configuration → Email and use “Send test email” if yours has ever gone quiet.", + "Signing up with an address an admin had already approved now actually creates the account. The sign-up reported success but the account was discarded on the spot, so the person could neither sign in nor ask for access again — they simply had no account at all.", + ], + chores: [ + "Registration now commits the new account explicitly. For an already-approved email, upsert_pending_invite() returns without committing, and the only other commit on that path is gated on working SMTP — so with mail unavailable the flushed user row was rolled back when the background task's session closed. Covered by three DB-lane regression tests: pre-approved without SMTP, unknown email, and pre-approved with SMTP.", + ], + }, { version: "0.78.4", date: "2026-08-30", From ea931dca308bcb572da0c8a3f6d97bf0d7119e12 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 1 Sep 2026 20:56:43 +0200 Subject: [PATCH 02/12] fix(auth): honour an approval granted before sign-up instead of parking the account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the commit fix, from the deep-review. Persisting the account was only half the story: when the admin had approved the address BEFORE the person registered, the account landed is_active=False against an invite that is no longer pending — absent from the Access-requests queue, and approve_invite 409s on an already-decided invite. The sign-up worked and still could not sign in. Derive activation from is_allowed() at the moment ownership is attested: - verify_email_confirm activates a pre-approved account when the emailed link is confirmed (only the mailbox owner can follow it), so SEC-10 stays intact; - _register_account activates only when there is no SMTP to verify with, where the admin whitelist is by design the sole gate — the same one add_invite already activates on. Tests: the pre-approved no-SMTP case and the verify-activates case are real regression tests (both fail without this change); guards cover the unknown-email and denied->pending branches, verification of an unapproved account, and the first end-to-end coverage of the /auth/register route and its allow_registration gate. Release notes rewritten — the previous entry miscounted its regression tests and advertised an email fix this release has no code for. --- backend/app/routes/auth.py | 15 ++++ backend/tests/test_auth_session.py | 120 +++++++++++++++++++++++++++-- frontend/src/lib/releaseNotes.ts | 9 ++- 3 files changed, 135 insertions(+), 9 deletions(-) diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 294a19ac..6a5da2a8 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -637,6 +637,15 @@ def _register_account(email: str, password: str) -> None: db.rollback() return upsert_pending_invite(db, email) # admin-approval gate (commits only when it writes a row) + # Honour an approval the admin had ALREADY given for this address. Without this the account + # lands is_active=False against a non-pending invite: it never shows up in the Access-requests + # queue (which lists pending only) and approve_invite 409s on an already-decided invite, so + # the admin has no obvious way to let the person in. Attestation still gates it — with SMTP on + # the address is unverified at this point, so activation waits for the verify link (see + # verify_email_confirm). With no SMTP there is no attestation to wait for and the admin's + # whitelist is by design the only gate, the very one add_invite already activates on. + if not email_ok and is_allowed(db, email): + user.is_active = True # Persist the account ourselves. upsert_pending_invite commits only when it actually inserts # (or revives) an invite row, and _issue_token below only runs with SMTP on — so for an email # the admin had ALREADY approved, on an instance with no SMTP, nothing along this path @@ -663,6 +672,12 @@ def verify_email_confirm(body: TokenIn, db: Session = Depends(get_db)) -> dict: status_code=400, detail="This verification link is invalid or has expired." ) user.email_verified = True + # Ownership is attested now (only the mailbox owner can follow this link), so an approval the + # admin granted BEFORE the person signed up can finally be honoured. Otherwise they sit at + # is_active=False behind a request no admin screen shows — the invite is already decided, so it + # is absent from the pending queue and approve_invite would 409 on it. + if not user.is_active and is_allowed(db, user.email): + user.is_active = True db.commit() return {"ok": True} diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 652682aa..2ef161e3 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -145,8 +145,9 @@ def _register_and_fetch(db, email: str): def test_registration_of_a_pre_approved_email_persists_the_account_without_smtp(db, monkeypatch): - # The regression: an admin pre-approves an address, the person then registers, and the account - # silently never existed. upsert_pending_invite returns early WITHOUT committing for an + # THE regression (the only one of these that fails without the explicit commit in + # _register_account): an admin pre-approves an address, the person then registers, and the + # account silently never existed. upsert_pending_invite returns early WITHOUT committing for an # already-approved email, and the only other commit on this path (_issue_token) is gated on # SMTP — so on a no-SMTP instance the flushed user was rolled back at session close. from app.models import Invite @@ -159,16 +160,21 @@ def test_registration_of_a_pre_approved_email_persists_the_account_without_smtp( 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.is_active is False # admin approval still gates sign-in assert user.password_hash is not None assert user.email_verified is True # no SMTP => ownership can't be proven by link assert invite.status == "approved" # an existing approval is never disturbed + # With no SMTP there is no attestation to wait for, so the admin's standing approval IS the gate + # and the account is usable at once. Otherwise it would sit inactive behind an invite that is no + # longer pending — invisible in the Access-requests queue, and approve_invite 409s on it. + assert user.is_active is True def test_registration_of_an_unknown_email_still_persists_account_and_pending_invite( db, monkeypatch ): - # The branch that always worked (upsert INSERTs the invite and commits) must keep working. + # Guard, not a regression test: this branch always worked (upsert INSERTs the invite and commits + # the same transaction). It must keep working — and the account must stay inactive, since nobody + # has approved this address. from app.routes import auth as auth_mod monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False) @@ -176,11 +182,33 @@ def test_registration_of_an_unknown_email_still_persists_account_and_pending_inv 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" +def test_registration_of_a_previously_denied_email_revives_the_request(db, monkeypatch): + # Guard: a denied person may ask again — upsert_pending_invite flips the invite back to pending + # (and commits) so the admin sees a fresh request. The stale decision must not leak into the new + # account's state, and re-registering must not be a dead end. + from app.models import Invite + from app.routes import auth as auth_mod + + monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False) + 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 + + def test_registration_of_a_pre_approved_email_persists_the_account_with_smtp(db, monkeypatch): - # Same pre-approved path with SMTP on: the account must persist AND get a verification link. + # 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. Here the account waits for the + # verify link (see the activation test below), so it is inactive and unverified. from app.models import Invite from app.routes import auth as auth_mod @@ -195,4 +223,86 @@ def test_registration_of_a_pre_approved_email_persists_the_account_with_smtp(db, assert user is not None assert user.email_verified is False # must confirm the address + assert user.is_active is False assert sent == ["rita@example.com"] + + +def test_verifying_an_email_activates_a_pre_approved_account(client, db, make_user): + # Confirming the emailed link attests ownership, so an approval the admin granted before the + # person signed up is honoured right there. Without this the SMTP-on pre-approved registrant is + # stuck: their invite is already decided, so it never reaches the Access-requests queue and + # approve_invite 409s on it. + 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 True + assert _login(client, "rita@example.com", _PW).status_code == 200 + + +def test_verifying_an_email_does_not_activate_an_unapproved_account(client, db, make_user): + # The mirror guard: verification proves the address, it does not grant access. With no approved + # invite the account stays inactive and sign-in is still refused. + from app.routes.auth import _issue_token + + user = make_user( + email="nobody@example.com", password=_PW, email_verified=False, is_active=False + ) + 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, "nobody@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 + from app.routes import auth as auth_mod + + monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False) + + 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 + from app.routes import auth as auth_mod + + monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False) + 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/frontend/src/lib/releaseNotes.ts b/frontend/src/lib/releaseNotes.ts index d122adff..4d28c766 100644 --- a/frontend/src/lib/releaseNotes.ts +++ b/frontend/src/lib/releaseNotes.ts @@ -18,13 +18,14 @@ export const RELEASE_NOTES: ReleaseEntry[] = [ version: "0.78.5", date: "2026-09-01", summary: - "Account emails are being delivered again — and a sign-up by someone the admin had already approved no longer vanishes without a trace.", + "Signing up with an address the admin had already approved now works end to end — the account is created, and confirming your email lets you straight in instead of parking you behind an approval that already happened.", fixes: [ - "Email is going out again. Verification links, password-reset links, approval and denial notices, and the “new access request” alert to the admin were all being silently dropped because the mail settings were incomplete — the app skipped each message instead of sending it. Check Configuration → Email and use “Send test email” if yours has ever gone quiet.", - "Signing up with an address an admin had already approved now actually creates the account. The sign-up reported success but the account was discarded on the spot, so the person could neither sign in nor ask for access again — they simply had no account at all.", + "Signing up with an address an admin had already approved now actually creates the account. Before, the sign-up reported success and the account was discarded on the spot, so the person could neither sign in nor ask for access again — they simply had no account at all.", + "Such an account no longer waits for an approval it already has. Confirming the emailed link now activates it straight away, and on an instance that sends no email the admin's whitelist alone is enough — previously the sign-up landed in a state the Access-requests screen never showed, because the request had already been decided.", ], chores: [ - "Registration now commits the new account explicitly. For an already-approved email, upsert_pending_invite() returns without committing, and the only other commit on that path is gated on working SMTP — so with mail unavailable the flushed user row was rolled back when the background task's session closed. Covered by three DB-lane regression tests: pre-approved without SMTP, unknown email, and pre-approved with SMTP.", + "Registration commits the new account explicitly: for an already-approved email, upsert_pending_invite() returns without committing and the only other commit on that path is gated on working SMTP, so with mail unavailable the flushed user row was rolled back when the background task's session closed. Activation is now derived from is_allowed() at the moment ownership is attested — at email verification, or at registration when the instance has no email to verify with. Covered by one regression test plus guards for every other branch of the worker, and the first end-to-end tests of the /auth/register route itself.", + "Operator note: an incomplete SMTP setting makes Siftlode skip every message silently by design. If your instance has never delivered mail, check Configuration → Email and use “Send test email”.", ], }, { From b5a1002412b0a9dbb3b4472321383413b0673d89 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 1 Sep 2026 21:18:14 +0200 Subject: [PATCH 03/12] fix(auth): reopen a pre-dated approval instead of auto-granting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the activation added in ea931dc, which round 2 of the review showed was a real weakening: on a no-SMTP instance it made a standing whitelist entry a bearer grant — the first anonymous party to register an approved-but-unclaimed address got an immediately usable account under that identity, with no attestation, no admin action and no signal anywhere. The verify-time activation had the same shape one step removed, and neither site ran the SEC-10 preset- password guard or left an audit trail. Keep a human in the loop instead. An approval granted before any account existed was a decision about the ADDRESS, so registration reopens the invite (upsert_pending_invite gains revive_approved, used only by the register worker). The request lands in the Access-requests queue, approving it runs _activate_user_by_email — and with it revoke_preset_password_if_unverified — and the account activates. One mechanism, already audited, no new grant path, and the "awaiting admin approval" copy is honest again. The explicit commit from the previous commit still matters: with an invite already pending, upsert returns without committing and _issue_token is gated on SMTP, so a no-SMTP registration was still being rolled back. Also from round 2: gate the admin notice on a fresh decision (matching the two other upsert callers) so a retry no longer re-pings every admin, and reset the module-level rate limiters between tests — the suite pooled every login into one 300s window and was two logins short of failing an unrelated test with a 429. --- backend/app/ratelimit.py | 7 ++ backend/app/routes/auth.py | 53 ++++++----- backend/tests/conftest.py | 17 ++++ backend/tests/test_auth_session.py | 142 +++++++++++++++++------------ frontend/src/lib/releaseNotes.ts | 10 +- 5 files changed, 142 insertions(+), 87 deletions(-) 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/auth.py b/backend/app/routes/auth.py index 6a5da2a8..3508cdd4 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -147,16 +147,25 @@ def _client_ip(request: Request) -> str: return peer -def upsert_pending_invite(db: Session, email: str) -> Invite | None: +def upsert_pending_invite( + db: Session, email: str, *, revive_approved: bool = False +) -> Invite | None: """Idempotently record an access request. Returns the Invite if a *new* pending row - was created (so the caller can notify admins), else None for already-known emails.""" + was created (so the caller can notify admins), else None for already-known emails. + + `revive_approved` is for the registration path only. An approval granted BEFORE any account + existed was a decision about the ADDRESS, not about whoever has now set a password on it, so the + request goes back in front of a human rather than becoming an automatic grant. It also un-strands + the registrant: an approved invite never appears in the Access-requests queue and `_decide` 409s + on an already-decided one, so nothing in the admin UI could otherwise activate the account.""" email = (email or "").lower() if not valid_email(email): return None inv = db.execute(select(Invite).where(Invite.email == email)).scalar_one_or_none() if inv is not None: - # Let a previously-denied person ask again; don't disturb approved/pending rows. - if inv.status == "denied": + # Let a previously-denied person ask again; don't disturb a pending row (the admin already + # has that request in front of them). + if inv.status == "denied" or (revive_approved and inv.status == "approved"): inv.status = "pending" inv.requested_at = datetime.now(timezone.utc) inv.decided_at = None @@ -636,29 +645,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 only when it writes a row) - # Honour an approval the admin had ALREADY given for this address. Without this the account - # lands is_active=False against a non-pending invite: it never shows up in the Access-requests - # queue (which lists pending only) and approve_invite 409s on an already-decided invite, so - # the admin has no obvious way to let the person in. Attestation still gates it — with SMTP on - # the address is unverified at this point, so activation waits for the verify link (see - # verify_email_confirm). With no SMTP there is no attestation to wait for and the admin's - # whitelist is by design the only gate, the very one add_invite already activates on. - if not email_ok and is_allowed(db, email): - user.is_active = True - # Persist the account ourselves. upsert_pending_invite commits only when it actually inserts - # (or revives) an invite row, and _issue_token below only runs with SMTP on — so for an email - # the admin had ALREADY approved, on an instance with no SMTP, nothing along this path - # committed and the flushed user was silently rolled back when the session closed. The - # would-be user got a uniform "ok" and no account. Commit unconditionally; it is a no-op - # when upsert already committed the same transaction. + # admin-approval gate. revive_approved: an approval that predates the account is re-opened so + # a human binds it to THIS registration — that keeps the SEC-10 preset-password guard in the + # loop (approve_invite runs it) and puts the request somewhere the admin can actually see. + inv = upsert_pending_invite(db, email, revive_approved=True) + # Persist the account ourselves. upsert_pending_invite commits only when it actually writes an + # invite row, and _issue_token below only runs with SMTP on — so when a request for this + # address was already pending, on an instance with no SMTP, nothing along this path committed + # and the flushed user was silently rolled back when the session closed. The would-be user got + # a uniform "ok" and no account. 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) - if admins: + # Only when there is a fresh decision to make, matching the other two call sites. Without the + # guard an address whose request was already pending re-pinged every admin on each retry. + if inv is not None and admins: email_mod.send_admin_new_request(admins, email) @@ -672,12 +677,6 @@ def verify_email_confirm(body: TokenIn, db: Session = Depends(get_db)) -> dict: status_code=400, detail="This verification link is invalid or has expired." ) user.email_verified = True - # Ownership is attested now (only the mailbox owner can follow this link), so an approval the - # admin granted BEFORE the person signed up can finally be honoured. Otherwise they sit at - # is_active=False behind a request no admin screen shows — the invite is already decided, so it - # is absent from the pending queue and approve_invite would 409 on it. - if not user.is_active and is_allowed(db, user.email): - user.is_active = True db.commit() return {"ok": True} diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 8a75d48b..54b967ff 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -189,3 +189,20 @@ 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.""" + import app.routes.auth as _auth + import app.routes.messages as _messages + import app.routes.public as _public + from app.ratelimit import RateLimiter + + for mod in (_auth, _messages, _public): + for obj in vars(mod).values(): + if isinstance(obj, RateLimiter): + obj.reset() diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 2ef161e3..16849323 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -144,56 +144,106 @@ def _register_and_fetch(db, email: str): return user, invite -def test_registration_of_a_pre_approved_email_persists_the_account_without_smtp(db, monkeypatch): - # THE regression (the only one of these that fails without the explicit commit in - # _register_account): an admin pre-approves an address, the person then registers, and the - # account silently never existed. upsert_pending_invite returns early WITHOUT committing for an - # already-approved email, and the only other commit on this path (_issue_token) is gated on - # SMTP — so on a no-SMTP instance the flushed user was rolled back at session close. - from app.models import 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_with_an_already_pending_request_persists_the_account( + db, make_user, monkeypatch +): + # THE regression: someone asks for access, then registers before the admin has decided. + # upsert_pending_invite leaves the pending row alone and returns WITHOUT committing, and the only + # other commit on this path (_issue_token) is gated on SMTP — so on a no-SMTP instance the flushed + # user was rolled back at session close. The sign-up answered "ok" and created nothing. + 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.password_hash is not None + assert user.is_active is False # the admin has not decided yet + assert invite.status == "pending" + assert notified == [] # the admin already has this request; don't re-ping on every retry + + +def test_registration_of_a_pre_approved_email_reopens_the_request_for_the_admin( + db, make_user, monkeypatch +): + # An approval granted before any account existed was a decision about the ADDRESS. Binding it to + # whoever has now set a password on that address is a human's call, so the invite goes back to + # pending — which is also the only way the admin can reach it: an approved invite is absent from + # the Access-requests queue and _decide 409s on an already-decided one. + 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 is not None + assert user.is_active is False # no automatic grant — approving it is what activates assert user.email_verified is True # no SMTP => ownership can't be proven by link - assert invite.status == "approved" # an existing approval is never disturbed - # With no SMTP there is no attestation to wait for, so the admin's standing approval IS the gate - # and the account is usable at once. Otherwise it would sit inactive behind an invite that is no - # longer pending — invisible in the Access-requests queue, and approve_invite 409s on it. + assert invite.status == "pending" + assert invite.decided_at is None and invite.decided_by is None + assert notified == ["roland@example.com"] + + +def test_approving_the_reopened_request_activates_the_account(db, make_user, monkeypatch): + # The other half of the loop: the admin clicks Approve and the person can sign in. This is what + # keeps the SEC-10 preset-password guard in the path — _activate_user_by_email runs it. + 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 # verified account keeps its own password def test_registration_of_an_unknown_email_still_persists_account_and_pending_invite( db, monkeypatch ): # Guard, not a regression test: this branch always worked (upsert INSERTs the invite and commits - # the same transaction). It must keep working — and the account must stay inactive, since nobody - # has approved this address. - from app.routes import auth as auth_mod - - monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False) + # the same transaction). It must keep working — and the account stays inactive, nobody has + # approved this address. + notified = _no_smtp(monkeypatch) 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 == [] # no admin in this DB, so nothing to notify -def test_registration_of_a_previously_denied_email_revives_the_request(db, monkeypatch): - # Guard: a denied person may ask again — upsert_pending_invite flips the invite back to pending - # (and commits) so the admin sees a fresh request. The stale decision must not leak into the new - # account's state, and re-registering must not be a dead end. +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 so the admin sees a + # fresh request. The stale decision must not leak into the new account's state. from app.models import Invite - from app.routes import auth as auth_mod - monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False) + 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() @@ -203,12 +253,13 @@ def test_registration_of_a_previously_denied_email_revives_the_request(db, monke 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. Here the account waits for the - # verify link (see the activation test below), so it is inactive and unverified. + # user, so this branch was never broken and must stay unbroken. The reopened request and the + # verification link are independent gates — the account clears both before it can sign in. from app.models import Invite from app.routes import auth as auth_mod @@ -219,51 +270,32 @@ def test_registration_of_a_pre_approved_email_persists_the_account_with_smtp(db, db.add(Invite(email="rita@example.com", status="approved", decided_by="admin@example.com")) db.commit() - user, _ = _register_and_fetch(db, "rita@example.com") + 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 == "pending" assert sent == ["rita@example.com"] -def test_verifying_an_email_activates_a_pre_approved_account(client, db, make_user): - # Confirming the emailed link attests ownership, so an approval the admin granted before the - # person signed up is honoured right there. Without this the SMTP-on pre-approved registrant is - # stuck: their invite is already decided, so it never reaches the Access-requests queue and - # approve_invite 409s on it. +def test_verifying_an_email_never_grants_access_on_its_own(client, db, make_user): + # Verification attests the address; it does not decide access. An account whose request is still + # pending stays inactive and cannot sign in — the two gates are deliberately 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.add(Invite(email="rita@example.com", status="pending")) 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 True - assert _login(client, "rita@example.com", _PW).status_code == 200 - - -def test_verifying_an_email_does_not_activate_an_unapproved_account(client, db, make_user): - # The mirror guard: verification proves the address, it does not grant access. With no approved - # invite the account stays inactive and sign-in is still refused. - from app.routes.auth import _issue_token - - user = make_user( - email="nobody@example.com", password=_PW, email_verified=False, is_active=False - ) - 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, "nobody@example.com", _PW).status_code == 403 + assert _login(client, "rita@example.com", _PW).status_code == 403 def test_register_route_creates_the_account_end_to_end(client, db, monkeypatch): @@ -274,9 +306,8 @@ def test_register_route_creates_the_account_end_to_end(client, db, monkeypatch): from sqlalchemy import select as _select from app.models import User - from app.routes import auth as auth_mod - monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False) + _no_smtp(monkeypatch) r = client.post("/auth/register", json={"email": "newbie@example.com", "password": _PW}) @@ -293,9 +324,8 @@ def test_register_route_is_refused_when_registration_is_closed(client, db, monke from app import sysconfig from app.models import User - from app.routes import auth as auth_mod - monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False) + _no_smtp(monkeypatch) sysconfig.set_value(db, "allow_registration", False) r = client.post("/auth/register", json={"email": "closed@example.com", "password": _PW}) diff --git a/frontend/src/lib/releaseNotes.ts b/frontend/src/lib/releaseNotes.ts index 4d28c766..715af96a 100644 --- a/frontend/src/lib/releaseNotes.ts +++ b/frontend/src/lib/releaseNotes.ts @@ -18,13 +18,15 @@ export const RELEASE_NOTES: ReleaseEntry[] = [ version: "0.78.5", date: "2026-09-01", summary: - "Signing up with an address the admin had already approved now works end to end — the account is created, and confirming your email lets you straight in instead of parking you behind an approval that already happened.", + "Two ways a sign-up could quietly go nowhere are fixed: on an instance that sends no email the account was discarded outright, and an address the admin had whitelisted in advance landed in a request no admin screen ever showed.", fixes: [ - "Signing up with an address an admin had already approved now actually creates the account. Before, the sign-up reported success and the account was discarded on the spot, so the person could neither sign in nor ask for access again — they simply had no account at all.", - "Such an account no longer waits for an approval it already has. Confirming the emailed link now activates it straight away, and on an instance that sends no email the admin's whitelist alone is enough — previously the sign-up landed in a state the Access-requests screen never showed, because the request had already been decided.", + "On an instance that sends no email, signing up with an address that already had a request on file created nothing at all. The sign-up reported success, no account existed, and trying again did the same — so the person could neither sign in nor get back into the queue.", + "Whitelisting someone before they sign up now works the way it looks like it should. Their sign-up reopens the request so it appears under Users → Access requests, where approving it lets them in; previously it landed against a decision already made, which the queue does not list and the Approve action refuses, leaving nobody able to complete it.", + "Retrying a sign-up while a request is already pending no longer emails every admin again.", ], chores: [ - "Registration commits the new account explicitly: for an already-approved email, upsert_pending_invite() returns without committing and the only other commit on that path is gated on working SMTP, so with mail unavailable the flushed user row was rolled back when the background task's session closed. Activation is now derived from is_allowed() at the moment ownership is attested — at email verification, or at registration when the instance has no email to verify with. Covered by one regression test plus guards for every other branch of the worker, and the first end-to-end tests of the /auth/register route itself.", + "Registration commits the new account explicitly — upsert_pending_invite() commits only when it actually writes an invite row, and the only other commit on that path is gated on working SMTP, so with mail unavailable the flushed user row was rolled back when the background task's session closed. An approval that predates the account now reopens the invite instead of being treated as a standing grant, which keeps a human (and with them the SEC-10 preset-password guard in _activate_user_by_email) between a whitelisted address and whoever registers it. The admin notice is gated on a fresh decision, matching the other two callers.", + "Two regression tests (no-SMTP with a request already pending, and the reopen-then-approve loop) plus guards for the unknown-email, previously-denied and SMTP-on branches, the first end-to-end tests of the /auth/register route and its allow_registration gate, and an autouse fixture that resets the module-level rate limiters between tests so their windows stop pooling across the suite.", "Operator note: an incomplete SMTP setting makes Siftlode skip every message silently by design. If your instance has never delivered mail, check Configuration → Email and use “Send test email”.", ], }, From b64c3f514ded8135c73fc3580f7cf5ae4e92772f Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 1 Sep 2026 21:34:38 +0200 Subject: [PATCH 04/12] =?UTF-8?q?fix(auth):=20drop=20the=20invite=20reopen?= =?UTF-8?q?=20=E2=80=94=20keep=20the=20fix=20to=20the=20lost=20account=20a?= =?UTF-8?q?lone?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 of the review killed the reopen from b5a1002: flipping an approved invite back to pending let an unauthenticated POST /auth/register REVOKE a standing approval for any whitelisted-but-unclaimed address, which then fails is_allowed() and locks the real owner out of Google sign-in — the primary way people get in here. It also re-armed the pre-emption window it was meant to close, because with no SMTP email_verified is set True and the SEC-10 guard the design leaned on never fires. Two attempts at fixing the stranded-account UX in the same change both came back worse than the wart. What remains is the narrow, real bug: the account was silently rolled back and is now committed. An admin still binds the approval to the account, exactly as before this branch. UX-25 in the roadmap records the follow-up and both dead ends so they are not retried. Also from round 3: the upsert docstring no longer claims the admin UI cannot reach these accounts (re-entering the address in the same panel's whitelist box does, and a test now pins it), the release notes are scoped to what the diff actually changes with the operator note moved where the dialog renders it, and RateLimiter has unit tests so reset() is not production code only a fixture calls. --- backend/app/routes/auth.py | 38 +++++------- backend/tests/test_auth_session.py | 97 +++++++++++++++--------------- backend/tests/test_ratelimit.py | 46 ++++++++++++++ frontend/src/lib/releaseNotes.ts | 13 ++-- 4 files changed, 116 insertions(+), 78 deletions(-) create mode 100644 backend/tests/test_ratelimit.py diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 3508cdd4..b7863313 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -147,25 +147,16 @@ def _client_ip(request: Request) -> str: return peer -def upsert_pending_invite( - db: Session, email: str, *, revive_approved: bool = False -) -> Invite | None: +def upsert_pending_invite(db: Session, email: str) -> Invite | None: """Idempotently record an access request. Returns the Invite if a *new* pending row - was created (so the caller can notify admins), else None for already-known emails. - - `revive_approved` is for the registration path only. An approval granted BEFORE any account - existed was a decision about the ADDRESS, not about whoever has now set a password on it, so the - request goes back in front of a human rather than becoming an automatic grant. It also un-strands - the registrant: an approved invite never appears in the Access-requests queue and `_decide` 409s - on an already-decided one, so nothing in the admin UI could otherwise activate the account.""" + was created (so the caller can notify admins), else None for already-known emails.""" email = (email or "").lower() if not valid_email(email): return None inv = db.execute(select(Invite).where(Invite.email == email)).scalar_one_or_none() if inv is not None: - # Let a previously-denied person ask again; don't disturb a pending row (the admin already - # has that request in front of them). - if inv.status == "denied" or (revive_approved and inv.status == "approved"): + # Let a previously-denied person ask again; don't disturb approved/pending rows. + if inv.status == "denied": inv.status = "pending" inv.requested_at = datetime.now(timezone.utc) inv.decided_at = None @@ -645,24 +636,23 @@ def _register_account(email: str, password: str) -> None: # no-op as the pre-check hit — roll back and stop. db.rollback() return - # admin-approval gate. revive_approved: an approval that predates the account is re-opened so - # a human binds it to THIS registration — that keeps the SEC-10 preset-password guard in the - # loop (approve_invite runs it) and puts the request somewhere the admin can actually see. - inv = upsert_pending_invite(db, email, revive_approved=True) + inv = upsert_pending_invite(db, email) # admin-approval gate # Persist the account ourselves. upsert_pending_invite commits only when it actually writes an - # invite row, and _issue_token below only runs with SMTP on — so when a request for this - # address was already pending, on an instance with no SMTP, nothing along this path committed - # and the flushed user was silently rolled back when the session closed. The would-be user got - # a uniform "ok" and no account. Commit unconditionally; it is a no-op when upsert already - # committed the same transaction. + # 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) - # Only when there is a fresh decision to make, matching the other two call sites. Without the - # guard an address whose request was already pending re-pinged every admin on each retry. + # Only when there is a fresh decision to make, matching the other two call sites. Otherwise + # someone who used Request access and then registered sent admins a second copy of the same + # request, and an already-approved address produced a notice pointing at an empty queue. if inv is not None and admins: email_mod.send_admin_new_request(admins, email) diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 16849323..0f41b613 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -156,13 +156,42 @@ def _no_smtp(monkeypatch): 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" + # …and no "new access request" notice, because there is no fresh decision to make. + assert notified == [] + + def test_registration_with_an_already_pending_request_persists_the_account( db, make_user, monkeypatch ): - # THE regression: someone asks for access, then registers before the admin has decided. - # upsert_pending_invite leaves the pending row alone and returns WITHOUT committing, and the only - # other commit on this path (_issue_token) is gated on SMTP — so on a no-SMTP instance the flushed - # user was rolled back at session close. The sign-up answered "ok" and created nothing. + # 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) @@ -173,39 +202,15 @@ def test_registration_with_an_already_pending_request_persists_the_account( user, invite = _register_and_fetch(db, "pia@example.com") assert user is not None, "registration was rolled back — the account is gone" - assert user.password_hash is not None - assert user.is_active is False # the admin has not decided yet + assert user.is_active is False assert invite.status == "pending" - assert notified == [] # the admin already has this request; don't re-ping on every retry + assert notified == [] # the admin already has this request; don't send a second copy -def test_registration_of_a_pre_approved_email_reopens_the_request_for_the_admin( - db, make_user, monkeypatch -): - # An approval granted before any account existed was a decision about the ADDRESS. Binding it to - # whoever has now set a password on that address is a human's call, so the invite goes back to - # pending — which is also the only way the admin can reach it: an approved invite is absent from - # the Access-requests queue and _decide 409s on an already-decided one. - 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 - assert user.is_active is False # no automatic grant — approving it is what activates - assert user.email_verified is True # no SMTP => ownership can't be proven by link - assert invite.status == "pending" - assert invite.decided_at is None and invite.decided_by is None - assert notified == ["roland@example.com"] - - -def test_approving_the_reopened_request_activates_the_account(db, make_user, monkeypatch): - # The other half of the loop: the admin clicks Approve and the person can sign in. This is what - # keeps the SEC-10 preset-password guard in the path — _activate_user_by_email runs it. +def test_an_admin_can_activate_a_pre_approved_registrant(db, make_user, 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 @@ -218,28 +223,28 @@ def test_approving_the_reopened_request_activates_the_account(db, make_user, mon db.expire_all() assert user.is_active is True - assert user.password_hash is not None # verified account keeps its own password + 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, monkeypatch + 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 the account stays inactive, nobody has - # approved this address. + # 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 == [] # no admin in this DB, so nothing to notify + 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 so the admin sees a - # fresh request. The stale decision must not leak into the new account's state. + # 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) @@ -258,8 +263,7 @@ def test_registration_of_a_previously_denied_email_revives_the_request(db, make_ 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. The reopened request and the - # verification link are independent gates — the account clears both before it can sign in. + # user, so this branch was never broken and must stay unbroken. from app.models import Invite from app.routes import auth as auth_mod @@ -275,18 +279,17 @@ def test_registration_of_a_pre_approved_email_persists_the_account_with_smtp(db, assert user is not None assert user.email_verified is False # must confirm the address assert user.is_active is False - assert invite.status == "pending" + 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. An account whose request is still - # pending stays inactive and cannot sign in — the two gates are deliberately separate. + # 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="pending")) + 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)) diff --git a/backend/tests/test_ratelimit.py b/backend/tests/test_ratelimit.py new file mode 100644 index 00000000..30a1ebb5 --- /dev/null +++ b/backend/tests/test_ratelimit.py @@ -0,0 +1,46 @@ +"""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.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 + # A blocked attempt is not counted, so a caller cannot push the window forward by hammering. + assert lim.allow("ip") is False + + +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/lib/releaseNotes.ts b/frontend/src/lib/releaseNotes.ts index 715af96a..abae450c 100644 --- a/frontend/src/lib/releaseNotes.ts +++ b/frontend/src/lib/releaseNotes.ts @@ -18,16 +18,15 @@ export const RELEASE_NOTES: ReleaseEntry[] = [ version: "0.78.5", date: "2026-09-01", summary: - "Two ways a sign-up could quietly go nowhere are fixed: on an instance that sends no email the account was discarded outright, and an address the admin had whitelisted in advance landed in a request no admin screen ever showed.", + "On an instance that cannot send email, signing up with an address that had already been whitelisted or had already requested access created no account at all. The sign-up said it worked, nothing existed, and trying again did the same.", fixes: [ - "On an instance that sends no email, signing up with an address that already had a request on file created nothing at all. The sign-up reported success, no account existed, and trying again did the same — so the person could neither sign in nor get back into the queue.", - "Whitelisting someone before they sign up now works the way it looks like it should. Their sign-up reopens the request so it appears under Users → Access requests, where approving it lets them in; previously it landed against a decision already made, which the queue does not list and the Approve action refuses, leaving nobody able to complete it.", - "Retrying a sign-up while a request is already pending no longer emails every admin again.", + "Such a sign-up now creates the account. Previously the person could neither sign in nor get back into the access queue — they had no account to approve, and every retry silently repeated the same nothing. Only a brand-new address was unaffected. As before, the account starts inactive and an admin's approval is what lets it sign in.", + "Signing up with an address that had already asked for access no longer sends admins a second copy of the same request, and one that was already whitelisted no longer produces a notice pointing at a queue with nothing in it.", + "Operator note: an incomplete SMTP setting makes Siftlode skip every message silently by design — that is what left the sign-ups above with nothing to fall back on. If your instance has never delivered mail, check Configuration → Email and use “Send test email”.", ], chores: [ - "Registration commits the new account explicitly — upsert_pending_invite() commits only when it actually writes an invite row, and the only other commit on that path is gated on working SMTP, so with mail unavailable the flushed user row was rolled back when the background task's session closed. An approval that predates the account now reopens the invite instead of being treated as a standing grant, which keeps a human (and with them the SEC-10 preset-password guard in _activate_user_by_email) between a whitelisted address and whoever registers it. The admin notice is gated on a fresh decision, matching the other two callers.", - "Two regression tests (no-SMTP with a request already pending, and the reopen-then-approve loop) plus guards for the unknown-email, previously-denied and SMTP-on branches, the first end-to-end tests of the /auth/register route and its allow_registration gate, and an autouse fixture that resets the module-level rate limiters between tests so their windows stop pooling across the suite.", - "Operator note: an incomplete SMTP setting makes Siftlode skip every message silently by design. If your instance has never delivered mail, check Configuration → Email and use “Send test email”.", + "Registration commits the new account explicitly. upsert_pending_invite() commits only when it actually writes an invite row — not when the address already has a pending or approved one — and the only other commit on that path is gated on working SMTP, so with mail unavailable the flushed user row was rolled back when the background task's session closed. The admin notice is now gated on that same fresh-decision signal, matching the two other callers.", + "Three DB-lane regression tests (pre-approved and already-pending sign-ups on a no-SMTP instance, plus the admin-activation recovery that depends on the account existing), guards for the unknown-email, previously-denied and SMTP-on branches, the first end-to-end tests of the /auth/register route and its allow_registration gate, unit tests for the rate limiter, and an autouse fixture resetting the module-level limiters between tests so their windows stop pooling across the suite.", ], }, { From f6e1d1e69faf73b11de2d704ec31631b485543f1 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 1 Sep 2026 21:48:40 +0200 Subject: [PATCH 05/12] fix(auth): keep the admin notice unconditional on the register path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 showed the `inv is not None` gate added in b5a1002 removed a signal that matters: for an address the admin had already approved, that email is their only notice that someone has now created an account on it — the invite is not pending so nothing lands in the Access-requests queue either. Suppressing it left the registrant at "awaiting admin approval" with the admin unaware they existed. The duplicate it avoided (Request access, then sign up) is mild noise by comparison. The production change is now one db.commit(), which is all the reported bug ever needed. Release notes corrected again: a previously-denied address was also unaffected, the pending-request case never lost its queue entry (only the account behind it), and the test inventory was missing a guard. --- backend/app/routes/auth.py | 10 +++++----- backend/tests/test_auth_session.py | 7 ++++--- frontend/src/lib/releaseNotes.ts | 7 +++---- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index b7863313..10c6c123 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -636,7 +636,7 @@ def _register_account(email: str, password: str) -> None: # no-op as the pre-check hit — roll back and stop. db.rollback() return - inv = upsert_pending_invite(db, email) # admin-approval gate + 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 @@ -650,10 +650,10 @@ def _register_account(email: str, password: str) -> None: # 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) - # Only when there is a fresh decision to make, matching the other two call sites. Otherwise - # someone who used Request access and then registered sent admins a second copy of the same - # request, and an already-approved address produced a notice pointing at an empty queue. - if inv is not None and admins: + # Unconditional, unlike the other two callers which gate on a fresh invite row: for an + # address the admin already approved this notice is the ONLY signal that someone has now + # created an account on it, and that account is inactive until the admin acts. + if admins: email_mod.send_admin_new_request(admins, email) diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 0f41b613..2e77a0dd 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -183,8 +183,9 @@ def test_registration_of_a_pre_approved_email_persists_the_account_without_smtp( # 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" - # …and no "new access request" notice, because there is no fresh decision to make. - assert notified == [] + # The admin is still told: for an already-approved address this notice is their only signal + # that an account now exists on it, and it is inactive until they act. + assert notified == ["roland@example.com"] def test_registration_with_an_already_pending_request_persists_the_account( @@ -204,7 +205,7 @@ def test_registration_with_an_already_pending_request_persists_the_account( 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 == [] # the admin already has this request; don't send a second copy + assert notified == ["pia@example.com"] def test_an_admin_can_activate_a_pre_approved_registrant(db, make_user, monkeypatch): diff --git a/frontend/src/lib/releaseNotes.ts b/frontend/src/lib/releaseNotes.ts index abae450c..aac775bf 100644 --- a/frontend/src/lib/releaseNotes.ts +++ b/frontend/src/lib/releaseNotes.ts @@ -20,13 +20,12 @@ export const RELEASE_NOTES: ReleaseEntry[] = [ summary: "On an instance that cannot send email, signing up with an address that had already been whitelisted or had already requested access created no account at all. The sign-up said it worked, nothing existed, and trying again did the same.", fixes: [ - "Such a sign-up now creates the account. Previously the person could neither sign in nor get back into the access queue — they had no account to approve, and every retry silently repeated the same nothing. Only a brand-new address was unaffected. As before, the account starts inactive and an admin's approval is what lets it sign in.", - "Signing up with an address that had already asked for access no longer sends admins a second copy of the same request, and one that was already whitelisted no longer produces a notice pointing at a queue with nothing in it.", + "Such a sign-up now creates the account. Before, there was nothing behind the request for an admin to approve, and every retry silently repeated the same nothing. Other sign-ups were unaffected — a brand-new address, one that had been denied and was asking again, and every sign-up on an instance with working email all created their account as expected. As before, a new account starts inactive and an admin's approval is what lets it sign in.", "Operator note: an incomplete SMTP setting makes Siftlode skip every message silently by design — that is what left the sign-ups above with nothing to fall back on. If your instance has never delivered mail, check Configuration → Email and use “Send test email”.", ], chores: [ - "Registration commits the new account explicitly. upsert_pending_invite() commits only when it actually writes an invite row — not when the address already has a pending or approved one — and the only other commit on that path is gated on working SMTP, so with mail unavailable the flushed user row was rolled back when the background task's session closed. The admin notice is now gated on that same fresh-decision signal, matching the two other callers.", - "Three DB-lane regression tests (pre-approved and already-pending sign-ups on a no-SMTP instance, plus the admin-activation recovery that depends on the account existing), guards for the unknown-email, previously-denied and SMTP-on branches, the first end-to-end tests of the /auth/register route and its allow_registration gate, unit tests for the rate limiter, and an autouse fixture resetting the module-level limiters between tests so their windows stop pooling across the suite.", + "Registration commits the new account explicitly. upsert_pending_invite() commits only when it actually writes an invite row — not when the address already has a pending or approved one — and the only other commit on that path is gated on working SMTP, so with mail unavailable the flushed user row was rolled back when the background task's session closed. That one line is the whole production change; two attempts to also fix the pre-approved account's activation UX were reverted after review (roadmap UX-25 records why).", + "Tests: three that fail without the fix (pre-approved and already-pending sign-ups on a no-SMTP instance, and the admin-activation recovery that depends on the account existing), guards for the unknown-email, previously-denied and SMTP-on branches and for verification not granting access on its own, the first end-to-end tests of the /auth/register route and its allow_registration gate, unit tests for the rate limiter, and an autouse fixture resetting the module-level limiters between tests so their windows stop pooling across the suite.", ], }, { From cb1f4f00b0e673602cf2ccf133cbc7e13de431ec Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 1 Sep 2026 22:02:43 +0200 Subject: [PATCH 06/12] docs(auth): correct the last round of over-claiming comments and notes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5's correctness/security pass came back clean; what was left was text that said more than the code does. - The register-path notify comment (and its test) claimed the admin "is still told". The call fires, but on a no-SMTP instance _send drops it — which is the open half of the problem, not something this branch fixed. Both now say so. - The release notes tell admins what to actually do: an account whose address was whitelisted in advance has no pending request to approve, so the whitelist box is what activates it. That instruction only existed in the roadmap and in chores, which the dialog does not render. - "That one line is the whole production change" now accounts for the test-only RateLimiter.reset() that also ships. - The rate-limiter test claimed to pin that blocked attempts are not counted, on an assertion that held either way. Replaced with one that drives the clock across the window boundary and fails if hammering extends it. --- backend/app/routes/auth.py | 8 +++++--- backend/tests/test_auth_session.py | 5 +++-- backend/tests/test_ratelimit.py | 21 ++++++++++++++++++++- frontend/src/lib/releaseNotes.ts | 5 +++-- 4 files changed, 31 insertions(+), 8 deletions(-) diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 10c6c123..58760f3b 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -650,9 +650,11 @@ def _register_account(email: str, password: str) -> None: # 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) - # Unconditional, unlike the other two callers which gate on a fresh invite row: for an - # address the admin already approved this notice is the ONLY signal that someone has now - # created an account on it, and that account is inactive until the admin acts. + # 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/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 2e77a0dd..861bbeb2 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -183,8 +183,9 @@ def test_registration_of_a_pre_approved_email_persists_the_account_without_smtp( # 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 admin is still told: for an already-approved address this notice is their only signal - # that an account now exists on it, and it is inactive until they act. + # 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"] diff --git a/backend/tests/test_ratelimit.py b/backend/tests/test_ratelimit.py index 30a1ebb5..5102318e 100644 --- a/backend/tests/test_ratelimit.py +++ b/backend/tests/test_ratelimit.py @@ -5,6 +5,7 @@ the module-level limiters between tests), so without a test here a refactor coul no-op and nothing would notice until an unrelated test started failing with a 429. """ +from app import ratelimit from app.ratelimit import RateLimiter @@ -14,10 +15,28 @@ def test_allow_trips_at_the_cap_and_stays_tripped(): assert lim.allow("ip") is True assert lim.allow("ip") is True assert lim.allow("ip") is False - # A blocked attempt is not counted, so a caller cannot push the window forward by hammering. 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) diff --git a/frontend/src/lib/releaseNotes.ts b/frontend/src/lib/releaseNotes.ts index aac775bf..661c0145 100644 --- a/frontend/src/lib/releaseNotes.ts +++ b/frontend/src/lib/releaseNotes.ts @@ -20,11 +20,12 @@ export const RELEASE_NOTES: ReleaseEntry[] = [ summary: "On an instance that cannot send email, signing up with an address that had already been whitelisted or had already requested access created no account at all. The sign-up said it worked, nothing existed, and trying again did the same.", fixes: [ - "Such a sign-up now creates the account. Before, there was nothing behind the request for an admin to approve, and every retry silently repeated the same nothing. Other sign-ups were unaffected — a brand-new address, one that had been denied and was asking again, and every sign-up on an instance with working email all created their account as expected. As before, a new account starts inactive and an admin's approval is what lets it sign in.", + "Such a sign-up now creates the account. Before, there was nothing behind the request for an admin to approve, and every retry silently repeated the same nothing. Other sign-ups were unaffected — a brand-new address, one that had been denied and was asking again, and every sign-up on an instance with working email all created their account as expected.", + "Admins: a new account still starts inactive. If the person had merely requested access, approve their request as usual. If you had already whitelisted their address, there is no pending request to approve — re-enter the address in the whitelist box under Users → Access requests and that activates their account.", "Operator note: an incomplete SMTP setting makes Siftlode skip every message silently by design — that is what left the sign-ups above with nothing to fall back on. If your instance has never delivered mail, check Configuration → Email and use “Send test email”.", ], chores: [ - "Registration commits the new account explicitly. upsert_pending_invite() commits only when it actually writes an invite row — not when the address already has a pending or approved one — and the only other commit on that path is gated on working SMTP, so with mail unavailable the flushed user row was rolled back when the background task's session closed. That one line is the whole production change; two attempts to also fix the pre-approved account's activation UX were reverted after review (roadmap UX-25 records why).", + "Registration commits the new account explicitly. upsert_pending_invite() commits only when it actually writes an invite row — not when the address already has a pending or approved one — and the only other commit on that path is gated on working SMTP, so with mail unavailable the flushed user row was rolled back when the background task's session closed. That one line is the whole behavioural change in production; the only other shipped edit is a test-only RateLimiter.reset() helper. Two attempts to also fix the pre-approved account's activation UX were reverted after review — roadmap UX-25 records both dead ends and the safe shape.", "Tests: three that fail without the fix (pre-approved and already-pending sign-ups on a no-SMTP instance, and the admin-activation recovery that depends on the account existing), guards for the unknown-email, previously-denied and SMTP-on branches and for verification not granting access on its own, the first end-to-end tests of the /auth/register route and its allow_registration gate, unit tests for the rate limiter, and an autouse fixture resetting the module-level limiters between tests so their windows stop pooling across the suite.", ], }, From 24c93cbb8376fec6993c1013203dd24ab38b7129 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 1 Sep 2026 22:15:56 +0200 Subject: [PATCH 07/12] docs: name the Configuration tab as it is rendered, drop an unused fixture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6's two remaining nits. The operator note said "Configuration → Email" but the group renders as "Email / SMTP" in both locales, and test_an_admin_can_activate_a_pre_approved_registrant requested make_user without using it — a leftover from the reverted rounds that made the test look like it seeded an admin when it did not. --- backend/tests/test_auth_session.py | 2 +- frontend/src/lib/releaseNotes.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 861bbeb2..b4e8adc0 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -209,7 +209,7 @@ def test_registration_with_an_already_pending_request_persists_the_account( assert notified == ["pia@example.com"] -def test_an_admin_can_activate_a_pre_approved_registrant(db, make_user, monkeypatch): +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. diff --git a/frontend/src/lib/releaseNotes.ts b/frontend/src/lib/releaseNotes.ts index 661c0145..75ac6380 100644 --- a/frontend/src/lib/releaseNotes.ts +++ b/frontend/src/lib/releaseNotes.ts @@ -22,7 +22,7 @@ export const RELEASE_NOTES: ReleaseEntry[] = [ fixes: [ "Such a sign-up now creates the account. Before, there was nothing behind the request for an admin to approve, and every retry silently repeated the same nothing. Other sign-ups were unaffected — a brand-new address, one that had been denied and was asking again, and every sign-up on an instance with working email all created their account as expected.", "Admins: a new account still starts inactive. If the person had merely requested access, approve their request as usual. If you had already whitelisted their address, there is no pending request to approve — re-enter the address in the whitelist box under Users → Access requests and that activates their account.", - "Operator note: an incomplete SMTP setting makes Siftlode skip every message silently by design — that is what left the sign-ups above with nothing to fall back on. If your instance has never delivered mail, check Configuration → Email and use “Send test email”.", + "Operator note: an incomplete SMTP setting makes Siftlode skip every message silently by design — that is what left the sign-ups above with nothing to fall back on. If your instance has never delivered mail, check Configuration → Email / SMTP and use “Send test email”.", ], chores: [ "Registration commits the new account explicitly. upsert_pending_invite() commits only when it actually writes an invite row — not when the address already has a pending or approved one — and the only other commit on that path is gated on working SMTP, so with mail unavailable the flushed user row was rolled back when the background task's session closed. That one line is the whole behavioural change in production; the only other shipped edit is a test-only RateLimiter.reset() helper. Two attempts to also fix the pre-approved account's activation UX were reverted after review — roadmap UX-25 records both dead ends and the safe shape.", From 44f626f7320ce6166d0cf2a1c9e3f1e81fa2f915 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 1 Sep 2026 23:09:25 +0200 Subject: [PATCH 08/12] feat(admin): delete a whitelist entry, and stop hiding a broken mail setup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three operator gaps, all found the same way — a person could not sign up and the app showed nobody anything. - DELETE /api/admin/invites/{id} with a delete control on both the pending requests and the previously-decided list. Deny keeps the row so the person cannot immediately re-ask; delete forgets the address. Refused with 409 while a User row exists for it, because is_allowed reads that row and removing it would revoke a live user's access with no other sign — the list ships has_account so the UI withholds the button instead of offering one that fails. - Email is fail-soft by design, which had come to mean invisible. _send() now records an email_undelivered notification for every admin when a message is skipped or rejected, naming the message that was lost, throttled to one an hour. /api/me carries email_configured, driving a standing warning on Configuration → Email / SMTP and above the access-request list. - The sign-in card's Create account / Forgot password / Didn't get the email views are history entries (useHistorySubview), so Back returns to the previous view instead of only moving the address bar. Also: the per-test rate-limiter reset sweeps every loaded app.* module rather than a hand-kept list of three — this change added a limiter outside it. --- VERSION | 2 +- backend/app/audit.py | 1 + backend/app/email.py | 55 +++++++++ backend/app/routes/admin.py | 59 ++++++++- backend/app/routes/me.py | 7 ++ backend/tests/conftest.py | 14 ++- backend/tests/test_admin_invites.py | 106 ++++++++++++++++ backend/tests/test_email_alerts.py | 115 ++++++++++++++++++ frontend/src/components/AdminUsers.tsx | 65 +++++++++- frontend/src/components/ConfigPanel.tsx | 9 +- frontend/src/components/InlineNotice.tsx | 26 ++++ .../src/components/NotificationsPanel.tsx | 12 ++ frontend/src/components/Welcome.tsx | 27 +++- .../src/components/welcomeMode.dom.test.ts | 93 ++++++++++++++ .../src/i18n/locales/en/auditActions.json | 3 +- frontend/src/i18n/locales/en/config.json | 3 +- frontend/src/i18n/locales/en/inbox.json | 5 + frontend/src/i18n/locales/en/settings.json | 11 +- .../src/i18n/locales/hu/auditActions.json | 3 +- frontend/src/i18n/locales/hu/config.json | 3 +- frontend/src/i18n/locales/hu/inbox.json | 5 + frontend/src/i18n/locales/hu/settings.json | 11 +- frontend/src/lib/api/account.ts | 2 + frontend/src/lib/api/admin.ts | 4 + frontend/src/lib/releaseNotes.ts | 18 +++ 25 files changed, 636 insertions(+), 23 deletions(-) create mode 100644 backend/tests/test_admin_invites.py create mode 100644 backend/tests/test_email_alerts.py create mode 100644 frontend/src/components/InlineNotice.tsx create mode 100644 frontend/src/components/welcomeMode.dom.test.ts 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")}} ) : ( - {/* 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 ? ( + + ) : (