fix(auth): stop discarding a legitimate password on admin approval (E9.5 S1)

SEC-10 revoked any password set while an account's email was unverified, on both the Google-adoption and the admin-approval path. On the approval path that was redundant: password_login refuses an unverified account before it ever reads is_active, so approval hands a pre-emptor's password nothing it could use. All the revoke achieved there was destroying the password a legitimate registrant had just chosen -- and since the admin normally decides before the user reads their mail, 'your password was cleared for security' was the DEFAULT experience, not an edge case. The guard stays on the Google-adoption path, where the real owner does attest ownership. password_reset_required and the ok_reset banner stay: accounts stripped on prod before this change have no other way back in. Two existing tests pinned the old rule, so they are rewritten rather than deleted, and the SEC-10 recovery pack now builds the revoked state through the surviving producer.
2026-09-03 23:32:34 +02:00
parent 7339a0b84b
commit 243869401a
6 changed files with 128 additions and 55 deletions
@@ -97,28 +97,73 @@ def test_reset_confirm_is_single_use_and_kills_existing_sessions(client, make_us
    assert replay.status_code == 400


-def test_activating_an_unverified_account_drops_its_preset_password(db, make_user):
-    # SEC-10 (account pre-emption): an attacker registers victim@x with a password before the approved
-    # owner arrives. The address was never verified, so admin approval must NOT hand that pre-set
-    # password a working login — it is dropped and any session it minted is revoked.
+def test_approving_an_unverified_account_keeps_its_password(db, make_user):
+    # E9.5 S1, inverting an earlier rule. Approval used to drop a password set while the email was
+    # unverified (SEC-10, account pre-emption). That was redundant: password_login refuses an
+    # unverified account BEFORE it reads is_active, so approval hands a pre-emptor's password nothing
+    # — see the pair of tests below. All it did was destroy the password a legitimate registrant had
+    # just chosen, in what is the NORMAL ordering (the admin decides before the user reads their mail).
    from app.routes.admin import _activate_user_by_email

-    victim = make_user(
-        email="victim@example.com",
-        password="attacker-chosen",
+    user = make_user(
+        email="approved-first@example.com",
+        password="chosen at registration",
        email_verified=False,
        is_active=False,
    )
-    epoch0 = victim.session_epoch or 0
-    _activate_user_by_email(db, "victim@example.com")
-    assert victim.is_active is True
-    assert victim.password_hash is None
-    assert (victim.session_epoch or 0) == epoch0 + 1
+    epoch0 = user.session_epoch or 0
+
+    _activate_user_by_email(db, "approved-first@example.com")
+
+    assert user.is_active is True
+    assert user.password_hash is not None
+    assert user.password_revoked_at is None
+    assert (user.session_epoch or 0) == epoch0  # nothing to revoke, so no epoch bump either
+
+
+def test_an_approved_but_unverified_account_still_cannot_sign_in(client, make_user):
+    # Why dropping the approval-path revoke is safe: this is the gate that actually stops a
+    # pre-emptor, and it runs whatever the admin decided.
+    make_user(
+        email="notyet@example.com",
+        password="attacker-chosen",
+        email_verified=False,
+        is_active=True,
+    )
+
+    r = _login(client, "notyet@example.com", "attacker-chosen")
+
+    assert r.status_code == 403
+    assert "verify" in r.json()["detail"].lower()
+
+
+def test_verifying_afterwards_lets_the_original_password_in(client, db, make_user):
+    # The payoff for the legitimate registrant: the password chosen at registration still works once
+    # the mailbox is proven — no "your password was cleared for security" detour, no reset round-trip.
+    from app.routes.auth import _issue_token
+
+    user = make_user(
+        email="ordering@example.com",
+        password="chosen at registration",
+        email_verified=False,
+        is_active=False,
+    )
+    from app.routes.admin import _activate_user_by_email
+
+    _activate_user_by_email(db, "ordering@example.com")  # admin decides FIRST — the common ordering
+    raw = _issue_token(db, user, "verify", timedelta(hours=1))
+    db.commit()
+
+    verified = client.post("/auth/verify", json={"token": raw})
+
+    assert verified.status_code == 200
+    assert verified.json() == {"ok": True, "password_reset_required": False}
+    assert _login(client, "ordering@example.com", "chosen at registration").status_code == 200


 def test_activating_a_verified_account_keeps_its_password(db, make_user):
-    # A legit account whose email IS verified (e.g. a no-SMTP registration) keeps its password on
-    # approval — the guard must not break the normal flow.
+    # The no-SMTP shape: registration marks such an account verified up front, so it was never in
+    # scope for the SEC-10 guard even before E9.5 S1. Kept as the other half of the pair.
    from app.routes.admin import _activate_user_by_email

    user = make_user(
@@ -212,7 +257,7 @@ def test_registration_with_an_already_pending_request_persists_the_account(
 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.
+    # _activate_user_by_email.
    from app.models import Invite
    from app.routes.admin import _activate_user_by_email

@@ -225,7 +270,7 @@ def test_an_admin_can_activate_a_pre_approved_registrant(db, monkeypatch):

    db.expire_all()
    assert user.is_active is True
-    assert user.password_hash is not None  # a verified account keeps its own password
+    assert user.password_hash is not None  # approval never touches the password (E9.5 S1)


 def test_registration_of_an_unknown_email_still_persists_account_and_pending_invite(
@@ -346,11 +391,24 @@ def test_register_route_is_refused_when_registration_is_closed(client, db, monke
 # --- SEC-10 lockout: a revoked preset password must stay RECOVERABLE ---------------------------
 #
 # Regression pack for the prod incident of 2026-09-02. An account registered with a password, then
-# approved by an admin BEFORE its email was verified, ended up with password_hash = NULL. That is
-# also what a Google-only account looks like, and reset eligibility keyed off `password_hash` alone —
-# so the account could neither sign in (no password) nor obtain a reset link, and every response on
-# the way was a deliberately uniform "ok"/401. `password_revoked_at` is the trace that tells the two
-# apart; these pin each step of the way back in.
+# stripped of it by the SEC-10 guard, ended up with password_hash = NULL. That is also what a
+# Google-only account looks like, and reset eligibility keyed off `password_hash` alone — so the
+# account could neither sign in (no password) nor obtain a reset link, and every response on the way
+# was a deliberately uniform "ok"/401. `password_revoked_at` is the trace that tells the two apart;
+# these pin each step of the way back in.
+#
+# E9.5 S1 removed the admin-approval caller, so Google adoption is the only path that still produces
+# this state — plus every account stripped on prod before that change, which is why the whole recovery
+# route below must keep working. These tests manufacture the state through the surviving producer.
+
+
+def _revoke_as_google_adoption(db, user) -> None:
+    """Put an account into the SEC-10-revoked state the way the ONLY remaining producer does: the
+    Google callback adopting an account whose email was never verified."""
+    from app.routes.auth import revoke_preset_password_if_unverified
+
+    revoke_preset_password_if_unverified(user)
+    db.commit()


 def _capture_reset_links(monkeypatch) -> list[str]:
@@ -362,9 +420,9 @@ def _capture_reset_links(monkeypatch) -> list[str]:
    return sent


-def test_the_approval_revoke_stamps_password_revoked_at(db, make_user):
-    from app.routes.admin import _activate_user_by_email
-
+def test_the_google_adoption_revoke_stamps_password_revoked_at(db, make_user):
+    # The guard still fires where it earns its keep: the real owner has just proven ownership through
+    # Google, so a password set on the address while it was unverified must not survive.
    victim = make_user(
        email="stamped@example.com",
        password="attacker-chosen",
@@ -372,27 +430,28 @@ def test_the_approval_revoke_stamps_password_revoked_at(db, make_user):
        is_active=False,
    )
    assert victim.password_revoked_at is None
+    epoch0 = victim.session_epoch or 0

-    _activate_user_by_email(db, "stamped@example.com")
+    _revoke_as_google_adoption(db, victim)

    assert victim.password_hash is None
    assert victim.password_revoked_at is not None  # …so the reset flow can tell it HAD a password
+    assert (victim.session_epoch or 0) == epoch0 + 1  # any session that password minted is killed


 def test_an_account_whose_password_was_revoked_can_still_get_a_reset_link(
    db, make_user, monkeypatch
 ):
    # THE lockout bug: without the stamp this sent nothing, and the account was unrecoverable.
-    from app.routes.admin import _activate_user_by_email
    from app.routes.auth import _send_reset_if_eligible

-    make_user(
+    victim = make_user(
        email="lockedout@example.com",
        password="chosen at registration",
        email_verified=False,
        is_active=False,
    )
-    _activate_user_by_email(db, "lockedout@example.com")
+    _revoke_as_google_adoption(db, victim)
    sent = _capture_reset_links(monkeypatch)

    _send_reset_if_eligible("lockedout@example.com")
@@ -414,16 +473,15 @@ def test_a_google_only_account_is_still_not_reset_eligible(db, make_user, monkey


 def test_completing_a_reset_clears_the_revocation_stamp(client, db, make_user):
-    from app.routes.admin import _activate_user_by_email
    from app.routes.auth import _issue_token

    user = make_user(
        email="recovered@example.com",
        password="chosen at registration",
        email_verified=False,
-        is_active=False,
+        is_active=True,  # already approved; reset-confirm verifies the email but never activates
    )
-    _activate_user_by_email(db, "recovered@example.com")
+    _revoke_as_google_adoption(db, user)
    raw = _issue_token(db, user, "reset", timedelta(hours=1))
    db.commit()

@@ -442,7 +500,6 @@ def test_verify_tells_a_revoked_account_that_it_needs_a_new_password(client, db,
    # The banner's input. Saying so leaks nothing: the caller just proved mailbox control by holding a
    # valid single-use verify token. Without it the user meets only the uniform "Invalid email or
    # password" and has no way to learn why the password they chose stopped working.
-    from app.routes.admin import _activate_user_by_email
    from app.routes.auth import _issue_token

    user = make_user(
@@ -451,7 +508,7 @@ def test_verify_tells_a_revoked_account_that_it_needs_a_new_password(client, db,
        email_verified=False,
        is_active=False,
    )
-    _activate_user_by_email(db, "needspw@example.com")
+    _revoke_as_google_adoption(db, user)
    raw = _issue_token(db, user, "verify", timedelta(hours=1))
    db.commit()