diff --git a/backend/app/models.py b/backend/app/models.py index 62d9c9ba..6fd47d67 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -109,7 +109,7 @@ class User(Base, TimestampMixin, CanonicalEmailMixin): # argon2id hash for email+password login; NULL for Google-only accounts. password_hash: Mapped[str | None] = mapped_column(String(255)) # When SEC-10 dropped a preset password on Google adoption (see the single caller of - # auth.revoke_preset_password_if_unverified; admin approval no longer revokes). Without + # auth.revoke_untrusted_preset_password; admin approval no longer revokes). Without # this trace a revoked account was indistinguishable from a Google-only one, so it failed the # password-reset eligibility check and could never be recovered — it could neither sign in nor # reset. Set = "had a password, lost it to the guard" -> reset-eligible. Cleared whenever a diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index d083a959..331a1fdc 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -151,7 +151,7 @@ def _activate_user_by_email(db: Session, email: str) -> None: attacker can always register last — and it handed anonymous callers a write onto a real account's credential. The owner's recourse is password-reset, which proves the mailbox, rewrites the hash and bumps the session epoch. The residual is logged as SEC-16. The revoke also stays on - the Google-adoption path (auth.revoke_preset_password_if_unverified), where ownership is + the Google-adoption path (auth.revoke_untrusted_preset_password), where ownership is actually attested.""" # Canonical, with the literal spelling preferred when both exist: approving # `npeter83.temu@gmail.com` must activate the account that registered as `npeter83temu@gmail.com`, diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 0c8d587f..44eecf9f 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -442,9 +442,10 @@ async def callback( return RedirectResponse(url=_access_notice_url("denied")) user = existing user.google_sub = sub - # SEC-10: drop any password pre-set on this account while its email was still unverified, - # now that Google attests ownership (bumps the epoch to revoke any session it minted). - revoke_preset_password_if_unverified(user) + # 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) # 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 @@ -1138,12 +1139,18 @@ def bump_session_epoch(user: User) -> None: user.session_epoch = (user.session_epoch or 0) + 1 -def revoke_preset_password_if_unverified(user: User) -> 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. No-op for an already-verified account, - so a legit no-SMTP registration is unaffected. + 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. + + 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. 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 @@ -1165,7 +1172,7 @@ def revoke_preset_password_if_unverified(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: + if not user.email_verified or (not user.is_active and user.password_hash is not None): user.password_hash = None user.password_revoked_at = datetime.now(timezone.utc) bump_session_epoch(user) diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 25f8aec0..cc91d987 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -491,9 +491,9 @@ def test_register_route_is_refused_when_registration_is_closed(client, db, monke 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 + from app.routes.auth import revoke_untrusted_preset_password - revoke_preset_password_if_unverified(user) + revoke_untrusted_preset_password(user) db.commit() diff --git a/backend/tests/test_google_callback.py b/backend/tests/test_google_callback.py new file mode 100644 index 00000000..82ee1995 --- /dev/null +++ b/backend/tests/test_google_callback.py @@ -0,0 +1,191 @@ +"""The Google callback's account-matching branches, through the real route, DB-lane. + +Each `select()` here was rewritten from a `.query()` without a test that drove it, and one branch — +adopting an existing row — is where a no-SMTP squat of an env-allowed address turned into an admin +account with the squatter's password. These pin the adoption (and what it revokes), its two refusals, +the refused sign-in that doubles as an access request, and the link flow's conflict answers. +""" + +import pytest +from sqlalchemy import select +from starlette.responses import RedirectResponse + +from app.config import settings +from app.models import Invite, User +from app.routes import auth as auth_routes + +_PW = "correct horse battery" + + +class _FakeGoogle: + """authlib's client, reduced to the two calls the routes make: the redirect out to Google and + the token exchange when Google redirects back.""" + + def __init__(self, userinfo: dict): + self.userinfo = userinfo + + async def authorize_redirect(self, _request, _redirect_uri, **_kw): + return RedirectResponse(url="/fake-google") + + async def authorize_access_token(self, _request): + return {"userinfo": self.userinfo, "access_token": "at", "scope": "openid email profile"} + + +@pytest.fixture +def google(monkeypatch): + from cryptography.fernet import Fernet + + from app import security + + # The callback stores the OAuth grant encrypted; a throwaway key stands in for TOKEN_ENCRYPTION_KEY. + monkeypatch.setattr(security, "_fernet", Fernet(Fernet.generate_key())) + fake = _FakeGoogle({}) + monkeypatch.setattr(auth_routes, "google_oauth", lambda _db: fake) + return fake + + +def _sign_in(client, google, **userinfo): + google.userinfo = {"email_verified": True, **userinfo} + return client.get("/auth/callback", follow_redirects=False) + + +def _rows(db, model, column, value): + db.expire_all() + return db.execute(select(model).where(column == value)).scalars().all() + + +def test_a_no_smtp_squat_of_an_admin_address_loses_its_password(client, db, google, monkeypatch): + # A stranger registers the ADMIN_EMAILS address (a sibling spelling will do) before its owner + # 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, "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") + [squat] = _rows(db, User, User.email, "b.ob@gmail.com") + assert squat.email_verified is True and squat.is_active is False + + r = _sign_in(client, google, sub="sub-bob", email="bob@gmail.com") + + assert r.headers["location"] == "/" + [owner] = _rows(db, User, User.email_canonical, squat.email_canonical) + assert (owner.id, owner.role, owner.is_active) == (squat.id, "admin", True) + assert owner.password_hash is None and owner.password_revoked_at is not None + assert ( + client.post( + "/auth/password-login", + json={"email": "b.ob@gmail.com", "password": "the squatter's own"}, + ).status_code + == 401 + ) + + +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. + user = make_user(email="kept@example.com", password=_PW, is_active=True) + monkeypatch.setattr(settings, "admin_emails", "kept@example.com") + + _sign_in(client, google, sub="sub-kept", email="kept@example.com") + + db.refresh(user) + assert user.google_sub == "sub-kept" + assert user.password_hash is not None and user.password_revoked_at is None + + +def test_adoption_matches_the_mailbox_not_the_spelling(client, db, google, make_user, monkeypatch): + user = make_user(email="pair.one@gmail.com", is_active=True) + monkeypatch.setattr(settings, "admin_emails", "pairone@gmail.com") + + _sign_in(client, google, sub="sub-pair", email="pairone@gmail.com") + + assert [u.id for u in _rows(db, User, User.email_canonical, user.email_canonical)] == [user.id] + db.refresh(user) + assert user.google_sub == "sub-pair" + + +def test_a_different_google_identity_on_the_address_is_refused( + client, db, google, make_user, monkeypatch +): + user = make_user(email="held@example.com", google_sub="sub-first", is_active=True) + monkeypatch.setattr(settings, "admin_emails", "held@example.com") + + r = _sign_in(client, google, sub="sub-second", email="held@example.com") + + assert "access=denied" in r.headers["location"] + db.refresh(user) + assert user.google_sub == "sub-first" + + +def test_an_unverified_google_address_does_not_adopt(client, db, google, make_user, monkeypatch): + user = make_user(email="pending@example.com", is_active=False, email_verified=False) + monkeypatch.setattr(settings, "admin_emails", "pending@example.com") + + google.userinfo = {"sub": "sub-unv", "email": "pending@example.com", "email_verified": False} + r = client.get("/auth/callback", follow_redirects=False) + + assert "access=denied" in r.headers["location"] + db.refresh(user) + assert (user.google_sub, user.is_active) == (None, False) + + +def test_a_refused_sign_in_records_a_request_in_the_reported_language( + client, db, google, monkeypatch +): + notified: list[str] = [] + monkeypatch.setattr( + auth_routes, "notify_admins_of_request", lambda _db, _bg, email: notified.append(email) + ) + + r = _sign_in(client, google, sub="sub-new", email="new@example.com", locale="hu-HU") + + assert "access=requested" in r.headers["location"] + [invite] = _rows(db, Invite, Invite.email, "new@example.com") + assert (invite.status, invite.lang) == ("pending", "hu") + assert notified == ["new@example.com"] + assert _rows(db, User, User.email, "new@example.com") == [] + + +def _start_link(client, email: str): + assert ( + client.post("/auth/password-login", json={"email": email, "password": _PW}).status_code + == 200 + ) + assert client.get("/auth/link", follow_redirects=False).headers["location"] == "/fake-google" + + +def test_linking_an_identity_another_account_holds_is_a_conflict(client, db, google, make_user): + me = make_user(email="me@example.com") + make_user(email="them@example.com", google_sub="sub-theirs") + _start_link(client, "me@example.com") + + r = _sign_in(client, google, sub="sub-theirs", email="them@example.com") + + assert r.headers["location"] == "/?link=conflict" + db.refresh(me) + assert me.google_sub is None + + +def test_linking_a_second_identity_is_a_mismatch(client, db, google, make_user): + me = make_user(email="me2@example.com", google_sub="sub-mine") + _start_link(client, "me2@example.com") + + r = _sign_in(client, google, sub="sub-other", email="other@example.com") + + assert r.headers["location"] == "/?link=mismatch" + db.refresh(me) + assert me.google_sub == "sub-mine" + + +def test_linking_a_free_identity_attaches_it(client, db, google, make_user): + me = make_user(email="me3@example.com") + _start_link(client, "me3@example.com") + + r = _sign_in(client, google, sub="sub-free", email="me3.google@example.com") + + assert r.headers["location"] == "/?link=ok" + db.refresh(me) + assert (me.google_sub, me.google_email) == ("sub-free", "me3.google@example.com")