fix(auth): drop the invite reopen — keep the fix to the lost account alone

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.
This commit is contained in:
2026-09-01 21:34:38 +02:00
parent b5a1002412
commit b64c3f514d
4 changed files with 116 additions and 78 deletions
+14 -24
View File
@@ -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)
+50 -47
View File
@@ -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))
+46
View File
@@ -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
+6 -7
View File
@@ -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.",
],
},
{