diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 676e24cd..e5f9382d 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -99,10 +99,16 @@ def test_reset_confirm_is_single_use_and_kills_existing_sessions(client, make_us 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). + # unverified (SEC-10, account pre-emption). It was dropped because it destroyed the password a + # legitimate registrant had just chosen, in what is the NORMAL ordering — the admin decides + # before the user reads their mail — making "your password was cleared for security" the default + # experience. + # + # S1 ALSO argued the revoke was redundant, because password_login refuses an unverified account + # before it reads is_active. The epic's closing review refuted that: the gate holds only while + # the address is unverified, and S6's approval mail exists to get the real owner to end that + # state. Do not restore that reasoning from this comment — the residual is SEC-16, and round 2 + # of the review tried and reverted a guard for it (see test_review_round.py). from app.routes.admin import _activate_user_by_email user = make_user( @@ -122,8 +128,10 @@ def test_approving_an_unverified_account_keeps_its_password(db, make_user): 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. + # The gate that stops a pre-emptor BEFORE the address is confirmed, whatever the admin decided. + # It is not the whole story — confirming the address opens it, which is SEC-16 — but it is the + # invariant this test owns, and a change that let an unverified account sign in would be a + # straightforwardly worse bug than the residual. make_user( email="notyet@example.com", password="attacker-chosen", diff --git a/backend/tests/test_review_round.py b/backend/tests/test_review_round.py index 9d7c81a0..5f1e6f39 100644 --- a/backend/tests/test_review_round.py +++ b/backend/tests/test_review_round.py @@ -122,6 +122,9 @@ def test_a_later_registration_leaves_a_link_already_in_flight_alive(db, client, db.expire_all() user = db.execute(select(User).where(User.email == "victim@example.com")).scalar_one() first = db.execute(select(AuthToken.id).where(AuthToken.user_id == user.id)).scalars().all() + # Not just a subset check below: `set() <= set()` is vacuously true, so without this the test + # would go green while asserting nothing the day registration stops issuing a token. + assert len(first) == 1 client.post( "/auth/register", json={"email": "victim@example.com", "password": "the second password"} @@ -130,7 +133,7 @@ def test_a_later_registration_leaves_a_link_already_in_flight_alive(db, client, db.expire_all() live = db.execute(select(AuthToken.id).where(AuthToken.user_id == user.id)).scalars().all() # The re-send adds a link; it must not invalidate the one the owner may already be holding. - assert set(first) <= set(live) + assert set(first) <= set(live) and len(live) == len(first) + 1 def test_a_stripped_account_keeps_its_recovery_stamp(db, client, smtp_on, make_user): @@ -244,11 +247,14 @@ def test_deleting_one_of_two_accounts_leaves_the_other_one_allowed(db, make_user # --- a button an admin pressed must not report a send that did not happen ------------------------- -def test_a_failed_invitation_is_not_recorded_as_written( +def test_a_failed_invitation_changes_nothing_the_admin_was_told_about( db, make_user, as_user, smtp_on, monkeypatch ): monkeypatch.setattr(email_mod, "send_access_invitation", lambda *a, **kw: False) admin = make_user(email="admin@example.com", role="admin") + stranger = make_user( + email="stranger@example.com", password=_PW, email_verified=False, is_active=False + ) inv = Invite(email="stranger@example.com", status="approved") db.add(inv) db.commit() @@ -260,6 +266,9 @@ def test_a_failed_invitation_is_not_recorded_as_written( # The column exists to answer "did I already write to them?", and the UI dims the button and # warns about a second mail on the strength of it. A dropped send must not make it say yes. assert db.get(Invite, inv.id).invited_at is None + # And a request the UI reports as a failure must not have let somebody in behind the admin's + # back — the activation belongs after a mail that actually left. + assert db.get(User, stranger.id).is_active is False def test_a_failed_resend_says_so_instead_of_reporting_ok(