From 7a1487f554f6b478395f3c049d41b5941a0855be Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 14 Nov 2026 20:07:13 +0100 Subject: [PATCH] fix(auth): a cancel at Google lands where the browser is; the probe says why it refused A refused session keeps its cookie, so the cancel branch, keyed on the cookie's account, sent a signed-out browser to the in-app toast the login page never shows: it now asks whether the session holds. The probe's log line named every refusal 'its account no longer holds'; it now carries the reason. The pruning docstring says again that a renewal may land a request's time after the revoke, and the test pins the day of margin. The test clock clamps in the zone asked for. --- backend/app/auth.py | 43 +++++++++++++++++---------- backend/app/routes/auth.py | 5 ++-- backend/tests/conftest.py | 4 ++- backend/tests/test_auth_session.py | 19 +++++++++++- backend/tests/test_google_callback.py | 23 ++++++++++++++ backend/tests/test_test_clock.py | 23 ++++++++++++++ 6 files changed, 98 insertions(+), 19 deletions(-) diff --git a/backend/app/auth.py b/backend/app/auth.py index b9df7e51..5a61618b 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -309,8 +309,9 @@ def new_session_id() -> str: def revoke_session(db: Session, request: Request) -> None: """Revoke this browser's session id, so a cookie carrying it that lands later is refused. Not committed — the caller commits. A row is dropped once no cookie with its id can still be valid: - a refused session is never renewed, so it expires `SESSION_MAX_AGE` after its `renewed` stamp, - which is no later than the revoke (a day of margin).""" + a refused session is never renewed, so it expires `SESSION_MAX_AGE` after its `renewed` stamp — + and that stamp is at most a request's time after the revoke (a request past its revoked-id check + can still renew), which the day of margin covers.""" sid = request.session.get("sid") if not isinstance(sid, str): return @@ -350,6 +351,12 @@ def current_user(request: Request, db: Session = Depends(get_db)) -> User: return user +def _refuse(request: Request, reason: str) -> HTTPException: + """The one answer to a refused session, its reason kept for the probe's log line.""" + request.state.session_refused = reason + return refused(401, "not_authenticated") + + def _session_user(request: Request, db: Session, user_id: int | None) -> User: if not user_id: raise refused(401, "not_authenticated") @@ -357,27 +364,29 @@ def _session_user(request: Request, db: Session, user_id: int | None) -> User: if user is None or not user.is_active or user.is_suspended: # Suspended/deactivated mid-session → end it so the block takes effect at once. request.session.clear() - raise refused(401, "not_authenticated") + raise _refuse(request, "account") # A cookie whose epoch is behind the account's (revoked by a password reset/change or "log out # everywhere") is refused — and so is one with no epoch at all, which only a build before the # one-account session wrote: its holder signs in once more. epoch = request.session.get("epoch") if epoch is None or int(epoch) != (user.session_epoch or 0): request.session.clear() - raise refused(401, "not_authenticated") + raise _refuse(request, "epoch") # A session signed out (or replaced by a sign-in as someone else) whose cookie landed late; one # with no id, which only a build before session ids wrote (its holder signs in once more); or one # not renewed for its whole life. Not cleared: this request may have left before a sign-in as # someone else answered, and a cookie deletion landing after that sign-in's cookie would sign the - # browser out of it. Such a session is refused on every request and never renewed, so leaving - # its cookie costs nothing. + # browser out of it. Such a session is refused here on every request and never renewed. What + # reads the cookie alone (`session_user_id`) still sees its account: the poster proxy, by design + # (a signed cookie is enough for a poster); anything that must know the browser is signed in + # asks `optional_current_user`. sid = request.session.get("sid") - if ( - not isinstance(sid, str) - or _session_expired(request) - or db.get(RevokedSession, sid) is not None - ): - raise refused(401, "not_authenticated") + if not isinstance(sid, str): + raise _refuse(request, "no_session_id") + if _session_expired(request): + raise _refuse(request, "expired") + if db.get(RevokedSession, sid) is not None: + raise _refuse(request, "revoked") _renew_session(request) # Attribute this request's log lines to the account. Here, not in the request middleware: # this is the one place a session becomes an account, and the middleware runs outside both the @@ -404,10 +413,14 @@ def optional_current_user(request: Request, db: Session = Depends(get_db)) -> Us return _session_user(request, db, user_id) except HTTPException: # A session that named an account, refused: the browser is signed out without being told - # why, so say it here. + # why, so say it here — and why, since only some reasons mean the account itself changed. log.info( - "a session was refused: its account no longer holds", - extra=logs.event("auth.session.probe_rejected", user_id=user_id), + "a session was refused", + extra=logs.event( + "auth.session.probe_rejected", + user_id=user_id, + reason=getattr(request.state, "session_refused", None), + ), ) return None diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index b440af61..355db046 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -392,9 +392,10 @@ async def callback( # or connecting via /auth/google/connect) never sees the pre-auth login banner, so # surface a neutral toast via ?oauth=cancelled; a fresh sign-in cancel lands on the login page # with the oauth_cancelled banner instead. Keyed on the live session, not on link_uid, so a - # sign-in started while signed in (no marker) is covered too. + # sign-in started while signed in (no marker) is covered too — a session that still HOLDS: + # a refused one keeps its cookie (`auth._session_user`), and its browser is on the login page. log.info("Google OAuth cancelled/failed: %s", exc.error) - if session_user_id(request) is not None: + if optional_current_user(request, db) is not None: return RedirectResponse(url="/?oauth=cancelled") return RedirectResponse(url="/?login=oauth_cancelled") diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 0aca6420..8f7bd8e8 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -332,7 +332,9 @@ class _NeverBack(_dt.datetime): t = cls.clock.now(tz) last = cls.last if last is not None and (last.tzinfo is None) == (t.tzinfo is None) and t <= last: - t = last + _dt.timedelta(microseconds=1) + # In the zone asked for: a Pacific reading clamped to a UTC one gave the UTC date. + nudged = last + _dt.timedelta(microseconds=1) + t = nudged.astimezone(t.tzinfo) if t.tzinfo is not None else nudged cls.last = t return t diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index a4657432..956bd81b 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -223,6 +223,20 @@ def test_a_cookie_from_before_a_sign_in_as_another_account_is_refused(client, ma assert client.get("/api/me").json()["email"] == "late2b@example.com" +def test_the_probe_says_why_a_session_was_refused(client, make_user, caplog): + # Only some refusals mean the account changed; a cookie with no session id is not one of them. + make_user(email="why@example.com", password=_PW) + assert _login(client, "why@example.com", _PW).status_code == 200 + _hold(client, _cookie({k: v for k, v in _session(client).items() if k != "sid"})) + with caplog.at_level("INFO", logger="siftlode.auth"): + assert client.get("/api/me").json() == {"authenticated": False} + [line] = [ + r for r in caplog.records if getattr(r, "event", None) == "auth.session.probe_rejected" + ] + assert line.getMessage() == "a session was refused" + assert line.attrs["reason"] == "no_session_id" + + def test_a_cookie_from_before_session_ids_signs_in_once_more(client, make_user): # Its sign-out could revoke nothing, so a request of its still in flight signed the browser back # in. Every such cookie would have run out within 14 days anyway: its holder signs in once more. @@ -250,11 +264,14 @@ def test_a_revoke_drops_only_the_revocations_no_cookie_can_outlive(db): life = timedelta(seconds=SESSION_MAX_AGE) db.add(RevokedSession(sid="outlived", revoked_at=now - life - timedelta(days=2))) db.add(RevokedSession(sid="still-live", revoked_at=now - life + timedelta(hours=1))) + # Past the cookie's life but inside the day of margin: a renewal a request's time after the + # revoke may still be valid. + db.add(RevokedSession(sid="in-the-margin", revoked_at=now - life - timedelta(hours=12))) db.commit() revoke_session(db, SimpleNamespace(session={"user_id": 1, "sid": "this-one"}, cookies={})) db.commit() left = set(db.scalars(select(RevokedSession.sid)).all()) - assert left == {"still-live", "this-one"} + assert left == {"still-live", "in-the-margin", "this-one"} def _session(client) -> dict: diff --git a/backend/tests/test_google_callback.py b/backend/tests/test_google_callback.py index 4390d403..bfcaa63d 100644 --- a/backend/tests/test_google_callback.py +++ b/backend/tests/test_google_callback.py @@ -59,6 +59,29 @@ def _sign_in(client, google, state: str | None = None, **userinfo): ) +@pytest.mark.parametrize("session", ["holds", "refused"]) +def test_a_cancel_at_google_lands_where_the_browser_is(client, google, make_user, session): + # Signed in, the cancel is a toast in the app; signed out, the login page's banner. A refused + # session keeps its cookie and its account id, and its browser is on the login page. + import base64 + import json + + from itsdangerous import TimestampSigner + + make_user(email="cancel@example.com", password=_PW) + _password_login(client, "cancel@example.com") + if session == "refused": + signer = TimestampSigner(str(settings.secret_key)) + payload = json.loads(base64.b64decode(signer.unsign(client.cookies["session"]))) + payload.pop("sid") # a cookie from before session ids + refused = signer.sign(base64.b64encode(json.dumps(payload).encode())).decode() + client.cookies.clear() + client.cookies.set("session", refused) + r = _sign_in(client, google, state="not-this-flow") + expected = "/?oauth=cancelled" if session == "holds" else "/?login=oauth_cancelled" + assert r.headers["location"] == expected + + def _rows(db, model, column, value): db.expire_all() return db.execute(select(model).where(column == value)).scalars().all() diff --git a/backend/tests/test_test_clock.py b/backend/tests/test_test_clock.py index 71a599fa..0008be5b 100644 --- a/backend/tests/test_test_clock.py +++ b/backend/tests/test_test_clock.py @@ -49,3 +49,26 @@ def test_a_test_reads_the_clock_the_app_stamps_with(monkeypatch): monkeypatch.setattr(conftest._NeverBack, "last", None) stamped = diagnostics.datetime.now(timezone.utc) assert datetime.now(timezone.utc) >= stamped + + +def test_a_clamped_reading_is_in_the_zone_asked_for(monkeypatch): + # The Pacific quota day read through a clamp came back in UTC: its date was the next day's. + from zoneinfo import ZoneInfo + + import conftest + + pacific = ZoneInfo("America/Los_Angeles") + utc_reading = datetime(2099, 1, 2, 3, 0, 0, tzinfo=timezone.utc) # 1 January 19:00 in Pacific + stepped_back = datetime(2099, 1, 1, 18, 59, 59, tzinfo=pacific) # a second earlier + + class Stepping(datetime): + @classmethod + def now(cls, tz=None): + return utc_reading if tz is timezone.utc else stepped_back + + monkeypatch.setattr(conftest._NeverBack, "clock", Stepping) + monkeypatch.setattr(conftest._NeverBack, "last", None) + conftest._NeverBack.now(timezone.utc) + clamped = conftest._NeverBack.now(pacific) + assert clamped.tzinfo is pacific + assert clamped.date().isoformat() == "2099-01-01"