diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 36e65ab7..7348f935 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -446,7 +446,11 @@ async def callback( # SEC-10: drop any password nobody proved the mailbox for, now that Google attests # ownership (bumps the epoch to revoke any session it minted). Runs before the # activation below, which is what it reads. - revoke_untrusted_preset_password(user) + revoke_untrusted_preset_password( + user, + becomes_admin=user.role != "admin" + and canonical_email(email) in env_admin_canonicals(), + ) # Google verified the email and is_allowed granted access, so finish activating a # previously pending password registration (otherwise current_user would bounce it). user.email_verified = True @@ -1140,18 +1144,24 @@ def bump_session_epoch(user: User) -> None: user.session_epoch = (user.session_epoch or 0) + 1 -def revoke_untrusted_preset_password(user: User) -> None: +def revoke_untrusted_preset_password(user: User, *, becomes_admin: bool = False) -> None: """SEC-10 (account pre-emption): a password set on an account whose email was NEVER verified is untrusted — whoever registered the address (to pre-empt the real owner) could have chosen it. When the real owner attests ownership through Google, drop that password and bump the epoch to revoke any session it minted; the owner re-sets one via password-reset, or from Settings while the Google - session lasts. No-op for a verified account that was already let in. + session lasts. - A NEVER-ACTIVATED account is untrusted too, verified or not. Without SMTP, registration marks - the address verified with no proof at all, so "verified" says nothing about who chose the - password; activation is what an admin (or this very sign-in) grants. For an ADMIN_EMAILS / - ALLOWED_EMAILS address no invite row gates the registration, so a stranger who registered it - first would otherwise be activated here — as admin — with their own password still working. + Two more cases are untrusted, because without SMTP registration marks the address verified with + no proof at all, so "verified" says nothing about who chose the password: + - A NEVER-ACTIVATED account. For an ADMIN_EMAILS / ALLOWED_EMAILS address no invite row gates the + registration, so a stranger who registered it first would otherwise be activated here — as + admin — with their own password still working. + - An account this sign-in is about to make ADMIN (`becomes_admin`: ADMIN_EMAILS names it) on an + instance without SMTP, even one an admin already activated. Activation is the admin vouching + for access, not for who chose the password, and ADMIN_EMAILS would otherwise hand the admin + role to a stranger's live session. With SMTP the verified flag is a proven mailbox, and an + account that is neither of these keeps its password: clearing it for everyone is the default + detour rejected below. The Google-adoption callback is the ONLY caller. Admin approval used to call this too, and dropping it was right for the reason given — the revoke destroyed the password a legitimate @@ -1173,7 +1183,11 @@ def revoke_untrusted_preset_password(user: User) -> None: revoke leaves `password_hash IS NULL`, which is also what a Google-only account looks like — and _send_reset_if_eligible keyed off exactly that, so a revoked account silently got no reset link and was locked out for good (it could not sign in either). The stamp separates the two cases.""" - if not user.email_verified or (not user.is_active and user.password_hash is not None): + if ( + not user.email_verified + or (not user.is_active and user.password_hash is not None) + or (becomes_admin and user.password_hash is not None and not email_mod.email_enabled()) + ): user.password_hash = None user.password_revoked_at = datetime.now(timezone.utc) bump_session_epoch(user) diff --git a/backend/tests/test_google_callback.py b/backend/tests/test_google_callback.py index 82ee1995..5de5b57c 100644 --- a/backend/tests/test_google_callback.py +++ b/backend/tests/test_google_callback.py @@ -81,13 +81,54 @@ def test_a_no_smtp_squat_of_an_admin_address_loses_its_password(client, db, goog ) +def test_an_activated_squat_loses_its_password_when_the_sign_in_makes_it_admin( + client, db, google, make_user, monkeypatch +): + # The same squat after an admin clicked Activate on it: activation vouches for access, not for + # who chose the password, and without SMTP nothing else ever proved the mailbox. ADMIN_EMAILS + # must not raise the squatter's live session to admin. + monkeypatch.setattr(auth_routes.email_mod, "email_enabled", lambda: False) + monkeypatch.setattr(settings, "admin_emails", "bob@gmail.com") + squat = make_user(email="b.ob@gmail.com", password="the squatter's own", is_active=True) + squatter = client.__class__(client.app) + assert ( + squatter.post( + "/auth/password-login", + json={"email": "b.ob@gmail.com", "password": "the squatter's own"}, + ).status_code + == 200 + ) + + _sign_in(client, google, sub="sub-bob", email="bob@gmail.com") + + db.refresh(squat) + assert squat.role == "admin" + assert squat.password_hash is None and squat.password_revoked_at is not None + assert squatter.get("/api/me").json() == {"authenticated": False} + + +def test_with_smtp_a_verified_account_made_admin_keeps_its_password( + client, db, google, make_user, monkeypatch +): + # With SMTP the verified flag is a proven mailbox, so the promotion takes nothing away. + monkeypatch.setattr(auth_routes.email_mod, "email_enabled", lambda: True) + monkeypatch.setattr(settings, "admin_emails", "own@example.com") + user = make_user(email="own@example.com", password=_PW, is_active=True) + + _sign_in(client, google, sub="sub-own", email="own@example.com") + + db.refresh(user) + assert user.role == "admin" and user.password_hash is not None + + def test_adopting_an_account_already_let_in_keeps_its_password( client, db, google, make_user, monkeypatch ): - # The revoke is for passwords nobody vouched for; an active, verified account's own password - # is not one of them. + # The revoke is for passwords nobody vouched for; an active, verified account that this sign-in + # does not make admin keeps its own. + monkeypatch.setattr(auth_routes.email_mod, "email_enabled", lambda: False) user = make_user(email="kept@example.com", password=_PW, is_active=True) - monkeypatch.setattr(settings, "admin_emails", "kept@example.com") + monkeypatch.setattr(settings, "allowed_emails", "kept@example.com") _sign_in(client, google, sub="sub-kept", email="kept@example.com")