diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 7348f935..7a4ba873 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -446,11 +446,7 @@ 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, - becomes_admin=user.role != "admin" - and canonical_email(email) in env_admin_canonicals(), - ) + revoke_untrusted_preset_password(user) # 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 @@ -481,7 +477,16 @@ async def callback( # default for a brand-new account (its role attribute is still unset before flush). # Canonical, like every other read of the env lists: a dotted or `+tag`ged ADMIN_EMAILS entry # would otherwise never bootstrap its admin, because Google answers with the plain primary. + # + # Becoming admin this way drops any password the account holds, on whichever sign-in it + # happens. Nothing here can tell who chose that password: without SMTP registration marks the + # address verified with no proof (and SMTP configured later never revisits it), and an admin's + # Activate vouches for access, not for the password. So a stranger who registered the address + # first, and was let in, would otherwise see their own live session become an admin one. The + # real owner, signed in through Google right now, sets a new password in Settings. if canonical_email(email) in env_admin_canonicals(): + if user.role != "admin" and user.password_hash is not None: + _revoke_password(user) user.role = "admin" elif not user.role: user.role = "user" @@ -1144,24 +1149,18 @@ def bump_session_epoch(user: User) -> None: user.session_epoch = (user.session_epoch or 0) + 1 -def revoke_untrusted_preset_password(user: User, *, becomes_admin: bool = False) -> None: +def revoke_untrusted_preset_password(user: User) -> 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. - 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. + A NEVER-ACTIVATED account is untrusted too, verified or not: without SMTP, registration marks the + address verified with no proof at all. 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 + with their own password still working. (An account the callback makes ADMIN loses its password + whatever its state — see the ADMIN_EMAILS block there.) 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 @@ -1183,14 +1182,16 @@ def revoke_untrusted_preset_password(user: User, *, becomes_admin: bool = False) 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) - 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) + if not user.email_verified or (not user.is_active and user.password_hash is not None): + _revoke_password(user) + + +def _revoke_password(user: User) -> None: + """Drop the account's password, stamp why (so password-reset still accepts it) and end every + session it signed in.""" + user.password_hash = None + user.password_revoked_at = datetime.now(timezone.utc) + bump_session_epoch(user) @router.post("/logout-others") diff --git a/backend/tests/test_google_callback.py b/backend/tests/test_google_callback.py index 5de5b57c..bb863a5e 100644 --- a/backend/tests/test_google_callback.py +++ b/backend/tests/test_google_callback.py @@ -59,7 +59,7 @@ def test_a_no_smtp_squat_of_an_admin_address_loses_its_password(client, db, goog # does. Without SMTP the row is marked verified, and an env-allowed address gets no invite row # to gate it — so the owner's first Google sign-in activates THIS row as admin. The squatter's # password must not survive that. - monkeypatch.setattr(auth_routes.email_mod, "email_enabled", lambda: False) + monkeypatch.setattr(auth_routes.email_mod, "email_enabled", lambda db=None: False) monkeypatch.setattr(auth_routes, "notify_admins_of_request", lambda *_a, **_k: None) monkeypatch.setattr(settings, "admin_emails", "bob@gmail.com") auth_routes._register_account("b.ob@gmail.com", "the squatter's own") @@ -87,7 +87,7 @@ def test_an_activated_squat_loses_its_password_when_the_sign_in_makes_it_admin( # 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(auth_routes.email_mod, "email_enabled", lambda db=None: 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) @@ -107,18 +107,46 @@ def test_an_activated_squat_loses_its_password_when_the_sign_in_makes_it_admin( assert squatter.get("/api/me").json() == {"authenticated": False} -def test_with_smtp_a_verified_account_made_admin_keeps_its_password( +def test_with_smtp_too_an_account_made_admin_loses_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) + # SMTP configured now says nothing about a row verified before it was: the rule does not ask. + monkeypatch.setattr(auth_routes.email_mod, "email_enabled", lambda db=None: 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 + assert user.role == "admin" and user.password_hash is None + # …and the owner, signed in through Google, is not thrown out by the epoch bump. + assert client.get("/api/me").json()["id"] == user.id + + +def test_a_later_sign_in_that_makes_the_account_admin_drops_its_password( + client, db, google, make_user, monkeypatch +): + # Adopted while the address was only allowed (the password kept: nothing promoted it), then + # added to ADMIN_EMAILS: the promotion happens on a sign-in matched by Google identity. + user = make_user(email="later@example.com", password=_PW, is_active=True, google_sub="sub-l") + monkeypatch.setattr(settings, "admin_emails", "later@example.com") + + _sign_in(client, google, sub="sub-l", email="later@example.com") + + db.refresh(user) + assert user.role == "admin" + assert user.password_hash is None and user.password_revoked_at is not None + + +def test_an_admin_sign_in_keeps_an_admin_password(client, db, google, make_user, monkeypatch): + # Only the promotion drops it: an account that is already admin signs in as before. + user = make_user(email="boss@example.com", password=_PW, role="admin", google_sub="sub-b") + monkeypatch.setattr(settings, "admin_emails", "boss@example.com") + + _sign_in(client, google, sub="sub-b", email="boss@example.com") + + db.refresh(user) + assert user.password_hash is not None def test_adopting_an_account_already_let_in_keeps_its_password( @@ -126,7 +154,7 @@ def test_adopting_an_account_already_let_in_keeps_its_password( ): # 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) + monkeypatch.setattr(auth_routes.email_mod, "email_enabled", lambda db=None: False) user = make_user(email="kept@example.com", password=_PW, is_active=True) monkeypatch.setattr(settings, "allowed_emails", "kept@example.com")