fix(auth): reopen a pre-dated approval instead of auto-granting it

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

@@ -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.
 

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}
 

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()

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})