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.
This commit is contained in:
+28
-15
@@ -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
|
||||
|
||||
|
||||
@@ -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")
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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"
|
||||
|
||||
Reference in New Issue
Block a user