fix(auth): a tab acts only as the account it was built for, and the review's other findings
A tab names the account it was claimed for on every request, beacon and upload (X-Siftlode-Expect-Account); current_user and /auth/logout refuse a mismatch with 401 account_changed and keep the session, so a tab left showing the account signed in before reloads instead of writing to the new one. /auth/login clears a connect marker left by an abandoned connect, and a link completes only on a session that still holds. The prefs merge reads the row under a lock. A tab's sessionStorage records its own owner. A page the account loses is given up, address bar and remembered page included. The panel layouts are preferences; the storage contract also refuses a key handed to the Storage API or to usePref outside its kind. A refused sign-in as someone else and a failed sign-out say so. The chromium-only e2e specs are in the gate register, the member walk waits for its requests, and devices.spec checks the save itself.
This commit is contained in:
+19
-1
@@ -263,8 +263,26 @@ def stamp_session(request: Request, user: User) -> None:
|
||||
request.session["epoch"] = user.session_epoch or 0
|
||||
|
||||
|
||||
# The account a tab was built for, sent with every request it makes (`identityHeaders`, the
|
||||
# frontend). The cookie alone says who is asking, and one browser holds one account: a sign-in as
|
||||
# someone else in another tab replaces it under every open tab. A tab still showing the previous
|
||||
# account would otherwise act as the new one — its player checkpoints, pref saves, Sign out and
|
||||
# Delete account landing there — until it noticed. It never selects an account; a mismatch is
|
||||
# refused, and the tab reloads as whoever is signed in now.
|
||||
EXPECTED_ACCOUNT_HEADER = "x-siftlode-expect-account"
|
||||
|
||||
|
||||
def current_user(request: Request, db: Session = Depends(get_db)) -> User:
|
||||
return _session_user(request, db, session_user_id(request))
|
||||
user = _session_user(request, db, session_user_id(request))
|
||||
expected = request.headers.get(EXPECTED_ACCOUNT_HEADER)
|
||||
if expected is not None and expected != str(user.id):
|
||||
# The session stays: it is the other tab's sign-in, and it holds.
|
||||
log.info(
|
||||
"a request from a tab built for another account was refused",
|
||||
extra=logs.event("auth.session.account_changed", user_id=user.id),
|
||||
)
|
||||
raise refused(401, "account_changed")
|
||||
return user
|
||||
|
||||
|
||||
def _session_user(request: Request, db: Session, user_id: int | None) -> User:
|
||||
|
||||
@@ -25,6 +25,7 @@ from app import sysconfig
|
||||
from app.auth import (
|
||||
_YOUTUBE_SCOPES,
|
||||
BASE_SCOPES,
|
||||
EXPECTED_ACCOUNT_HEADER,
|
||||
PASSWORD_MIN_LEN,
|
||||
READ_SCOPES,
|
||||
WRITE_SCOPES,
|
||||
@@ -36,6 +37,7 @@ from app.auth import (
|
||||
is_allowed,
|
||||
log,
|
||||
operator_contact,
|
||||
optional_current_user,
|
||||
session_user_id,
|
||||
stamp_session,
|
||||
)
|
||||
@@ -330,6 +332,11 @@ async def login(request: Request, db: Session = Depends(get_db)):
|
||||
# Google sign-in isn't configured on this instance — land back on the login page (the UI
|
||||
# hides the button, so this is just a safety net for a stale/direct hit).
|
||||
return RedirectResponse(url="/")
|
||||
# A sign-in, never a link: a connect abandoned at Google (Back, a closed tab) never reached the
|
||||
# callback that pops its markers, and left behind they would attach the identity signing in
|
||||
# here to the account still signed into this browser.
|
||||
request.session.pop("oauth_link_uid", None)
|
||||
request.session.pop("oauth_link_explicit", None)
|
||||
return await client.authorize_redirect(
|
||||
request,
|
||||
_oauth_redirect_uri(request),
|
||||
@@ -354,7 +361,7 @@ async def callback(
|
||||
# /auth/google/connect; when present we attach to the current user instead of identifying by sub.
|
||||
# Pop them BEFORE the token exchange: if the user aborts Google's consent screen the exchange
|
||||
# throws below, and a marker left behind would silently turn the NEXT clean sign-in into a link
|
||||
# attempt. Popping up front means an aborted link flow leaves no residue in the session.
|
||||
# attempt. A connect that never comes back here at all is cleared by /auth/login instead.
|
||||
link_uid = request.session.pop("oauth_link_uid", None)
|
||||
link_explicit = request.session.pop("oauth_link_explicit", False)
|
||||
try:
|
||||
@@ -584,12 +591,12 @@ def _complete_link(
|
||||
Google identity owned by another account, or to silently swap a different linked identity.
|
||||
`explicit` (the popped oauth_link_explicit marker) distinguishes a "Connect Google" click from
|
||||
a YouTube grant — the caller pops it up front so an aborted flow leaves nothing behind."""
|
||||
# The marker must name the account still signed into THIS browser: a sign-in as someone else
|
||||
# between the start and the callback replaced it, and the identity must not land on the newcomer.
|
||||
if link_uid != session_user_id(request):
|
||||
return RedirectResponse(url="/?link=error")
|
||||
user = db.get(User, link_uid)
|
||||
if user is None:
|
||||
# The marker must name the account still signed into THIS browser, and that session must still
|
||||
# hold: a sign-in as someone else between the start and the callback replaced it (the identity
|
||||
# must not land on the newcomer), and a session revoked meanwhile ("log out everywhere", a
|
||||
# password change, a suspension) must not finish what it started.
|
||||
user = optional_current_user(request, db)
|
||||
if user is None or user.id != link_uid:
|
||||
return RedirectResponse(url="/?link=error")
|
||||
other = db.execute(
|
||||
select(User).where(User.google_sub == sub, User.id != user.id)
|
||||
@@ -616,7 +623,13 @@ def _complete_link(
|
||||
|
||||
@router.post("/logout")
|
||||
async def logout(request: Request):
|
||||
"""Sign this browser out: the session holds one account, and it ends."""
|
||||
"""Sign this browser out: the session holds one account, and it ends — unless the tab asking was
|
||||
built for another account than the one signed in now (a sign-in as someone else in another tab):
|
||||
its Sign out would end THAT person's session (see `EXPECTED_ACCOUNT_HEADER`)."""
|
||||
expected = request.headers.get(EXPECTED_ACCOUNT_HEADER)
|
||||
signed_in = session_user_id(request)
|
||||
if expected is not None and signed_in is not None and expected != str(signed_in):
|
||||
raise refused(401, "account_changed")
|
||||
request.session.clear()
|
||||
return JSONResponse({"ok": True})
|
||||
|
||||
|
||||
@@ -136,7 +136,10 @@ def update_preferences(
|
||||
user: User = Depends(current_user),
|
||||
db: Session = Depends(get_db),
|
||||
) -> dict:
|
||||
# Merge so partial updates don't wipe other keys.
|
||||
# Merge so partial updates don't wipe other keys — over the stored row read again under a row
|
||||
# lock: a client saves keys one at a time (several at once on a boot that moves this browser's
|
||||
# copies to the account), and two merges over the same earlier read would keep only the last.
|
||||
db.refresh(user, with_for_update=True)
|
||||
merged = dict(user.preferences or {})
|
||||
merged.update(preferences.root)
|
||||
user.preferences = merged
|
||||
|
||||
@@ -231,7 +231,7 @@ def _configured(monkeypatch):
|
||||
@pytest.fixture
|
||||
def client(db, override_dep, _configured):
|
||||
"""A TestClient wired to the test DB that goes through the REAL auth/session stack — no
|
||||
`current_user` override — so login, the wallet gate and epoch revoke are exercised end to end.
|
||||
`current_user` override — so login, the one-account session and epoch revoke are exercised end to end.
|
||||
Starlette's SessionMiddleware round-trips the signed cookie, so a login in one call authenticates
|
||||
the next on this same instance. No lifespan (the scheduler / HLS sweep stay off)."""
|
||||
override_dep(get_db, db.info["request"])
|
||||
|
||||
@@ -140,7 +140,7 @@ def test_a_google_link_lands_only_on_the_account_still_signed_in(db, make_user,
|
||||
from app.routes.auth import _complete_link
|
||||
|
||||
starter, other = make_user(), make_user()
|
||||
session = {"user_id": starter.id if signed_in_as == "starter" else other.id}
|
||||
session = {"user_id": starter.id if signed_in_as == "starter" else other.id, "epoch": 0}
|
||||
request = Request({"type": "http", "session": session, "headers": []})
|
||||
resp = _complete_link(request, db, starter.id, True, "sub-123", {"email": ""}, {"scope": ""})
|
||||
db.refresh(starter)
|
||||
@@ -165,6 +165,47 @@ def test_no_app_code_reads_a_second_account():
|
||||
assert hits == []
|
||||
|
||||
|
||||
def test_a_tab_built_for_the_account_signed_in_before_is_refused(client, make_user):
|
||||
# Two tabs of one browser: one signed in as someone else, so the cookie is Bob's under both. The
|
||||
# tab still showing Alice says so with every request; acting as Bob there would put her player
|
||||
# positions, pref saves and Sign out on his account. Refused, and the session stays Bob's.
|
||||
alice = make_user(email="alice5@example.com", password=_PW)
|
||||
bob = make_user(email="bob5@example.com", password=_PW)
|
||||
assert _login(client, "alice5@example.com", _PW).status_code == 200
|
||||
assert _login(client, "bob5@example.com", _PW).status_code == 200
|
||||
|
||||
stale = client.get("/api/feed", headers={"X-Siftlode-Expect-Account": str(alice.id)})
|
||||
assert stale.status_code == 401
|
||||
assert stale.json()["detail"]["reason"] == "account_changed"
|
||||
assert (
|
||||
client.get("/api/feed", headers={"X-Siftlode-Expect-Account": str(bob.id)}).status_code
|
||||
== 200
|
||||
)
|
||||
assert client.get("/api/feed").status_code == 200
|
||||
# Nor may its Sign out end Bob's session.
|
||||
assert (
|
||||
client.post(
|
||||
"/auth/logout", headers={"X-Siftlode-Expect-Account": str(alice.id)}
|
||||
).status_code
|
||||
== 401
|
||||
)
|
||||
# The probe answers who is signed in now, so the stale tab can reload as Bob.
|
||||
probe = client.get("/api/me", headers={"X-Siftlode-Expect-Account": str(alice.id)})
|
||||
assert probe.json()["email"] == "bob5@example.com"
|
||||
|
||||
|
||||
def test_setting_a_password_keeps_the_caller_signed_in(client, make_user):
|
||||
# The change ends every OTHER session (the epoch moves); the caller is stamped with the new one.
|
||||
make_user(email="dave@example.com", password=_PW)
|
||||
assert _login(client, "dave@example.com", _PW).status_code == 200
|
||||
r = client.post(
|
||||
"/auth/set-password",
|
||||
json={"password": "a brand new passphrase", "current_password": _PW},
|
||||
)
|
||||
assert r.status_code == 200
|
||||
assert client.get("/api/feed").status_code == 200
|
||||
|
||||
|
||||
def test_logout_others_revokes_the_other_session_but_keeps_the_caller(client, make_user):
|
||||
# Two independent cookie jars (browsers) signed into the same account. `logout-others` from one
|
||||
# bumps the account epoch so the OTHER's cookie is rejected on its next request, while the caller
|
||||
|
||||
@@ -270,6 +270,41 @@ def test_linking_a_free_identity_attaches_it(client, db, google, make_user):
|
||||
assert (me.google_sub, me.google_email) == ("sub-free", "me3.google@example.com")
|
||||
|
||||
|
||||
def test_a_connect_abandoned_at_google_leaves_the_next_sign_in_a_sign_in(
|
||||
client, db, google, make_user, monkeypatch
|
||||
):
|
||||
# A connect left at Google's screen (Back, a closed tab) never reaches the callback that pops
|
||||
# its marker. "Sign in as someone else" afterwards must sign that person in, not link their
|
||||
# Google identity onto the account still signed in here.
|
||||
me = make_user(email="me4@example.com")
|
||||
make_user(email="them4@example.com", google_sub="sub-them4", password=None)
|
||||
monkeypatch.setattr(settings, "allowed_emails", "them4@example.com")
|
||||
_start_link(client, "me4@example.com")
|
||||
assert client.get("/auth/login", follow_redirects=False).headers["location"] == "/fake-google"
|
||||
|
||||
r = _sign_in(client, google, sub="sub-them4", email="them4@example.com")
|
||||
|
||||
assert r.headers["location"] == "/"
|
||||
assert client.get("/api/me").json()["email"] == "them4@example.com"
|
||||
db.refresh(me)
|
||||
assert me.google_sub is None
|
||||
|
||||
|
||||
def test_a_link_whose_session_was_revoked_meanwhile_does_not_finish(client, db, google, make_user):
|
||||
# "Log out everywhere" (or a password change) between the start and the callback: the cookie
|
||||
# still names the account, but its session no longer holds, so it may not attach an identity.
|
||||
me = make_user(email="me5@example.com")
|
||||
_start_link(client, "me5@example.com")
|
||||
me.session_epoch = (me.session_epoch or 0) + 1
|
||||
db.commit()
|
||||
|
||||
r = _sign_in(client, google, sub="sub-late", email="late@example.com")
|
||||
|
||||
assert r.headers["location"] == "/?link=error"
|
||||
db.refresh(me)
|
||||
assert me.google_sub is None
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("access", "scope", "prompt"),
|
||||
[
|
||||
|
||||
@@ -0,0 +1,44 @@
|
||||
"""PUT /api/me/preferences merges the keys it is sent into the stored ones, DB-lane.
|
||||
|
||||
A client saves its preferences a key at a time, and several at once on the boot that moves this
|
||||
browser's copies to the account. Each merge must start from the row as stored when it runs, not as
|
||||
the request first read it — two merges over the same earlier read keep only the last one's keys.
|
||||
"""
|
||||
|
||||
from fastapi import Depends, Request
|
||||
from sqlalchemy import update
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
from app.auth import current_user
|
||||
from app.db import SessionLocal, get_db
|
||||
from app.models import User
|
||||
|
||||
_PW = "correct horse battery"
|
||||
|
||||
|
||||
def test_a_save_keeps_a_key_another_save_stored_after_this_one_read_the_account(
|
||||
client, db, make_user, override_dep
|
||||
):
|
||||
user = make_user(email="prefs@example.com", password=_PW)
|
||||
assert (
|
||||
client.post("/auth/password-login", json={"email": "prefs@example.com", "password": _PW})
|
||||
).status_code == 200
|
||||
|
||||
def read_then_another_save(request: Request, session: Session = Depends(get_db)) -> User:
|
||||
# The account as this request reads it, then a second save committing in the meantime.
|
||||
caller = current_user(request, session)
|
||||
with SessionLocal() as other:
|
||||
other.execute(
|
||||
update(User)
|
||||
.where(User.id == caller.id)
|
||||
.values(preferences={"seenVersion": "1.2.3"})
|
||||
)
|
||||
other.commit()
|
||||
return caller
|
||||
|
||||
override_dep(current_user, read_then_another_save)
|
||||
r = client.put("/api/me/preferences", json={"startOnHome": False})
|
||||
|
||||
assert r.status_code == 200
|
||||
db.expire_all()
|
||||
assert db.get(User, user.id).preferences == {"seenVersion": "1.2.3", "startOnHome": False}
|
||||
Reference in New Issue
Block a user