From ef99f7b5c778616fd91744755ff74e20d80fc276 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 21 Oct 2026 02:05:33 +0200 Subject: [PATCH] fix(auth): a Google connect names its tab's account too, and a refused tab says why - /auth/google/connect is a navigation and carries no headers: the tab names the account it was built for in `expect`, checked by the same comparison as the request header (built_for_another_account); a mismatch lands on /?account=changed - the link marker names the OAuth state of its own flow: an older flow coming back no longer spends the marker of the connect still open (which then returned as a plain sign-in) - a 401 account_changed reloads the tab with a one-time flag, and the reloaded tab says it now shows the account signed in - a refused switch to a suspended or not-yet-approved account says so in the signed-in app's words, naming the address a request was recorded for - the prefs merge locks FOR NO KEY UPDATE, so a transaction that only added a row referring to the account does not hold the save up; a test pins the lock - giving up a lost page closes a channel page open over it - e2e README names the chromium-only specs --- backend/app/auth.py | 22 ++-- backend/app/routes/auth.py | 80 +++++++++---- backend/app/routes/me.py | 5 +- backend/tests/test_google_callback.py | 60 +++++++++- backend/tests/test_me_preferences.py | 32 ++++- frontend/e2e/README.md | 3 +- frontend/src/App.tsx | 26 ++-- .../NavigationProvider.dom.test.tsx | 14 +++ .../src/components/NavigationProvider.tsx | 28 +++-- frontend/src/i18n/locales/en/header.json | 3 + frontend/src/i18n/locales/hu/header.json | 3 + .../src/lib/accountSwitchNotice.dom.test.ts | 111 ++++++++++++++++++ frontend/src/lib/api/core.ts | 11 +- .../lib/api/expectedAccount.contract.test.ts | 16 +++ frontend/src/lib/entryUrl.ts | 61 ++++++++-- frontend/src/lib/googleConnect.ts | 12 +- .../src/lib/refusedSwitchNotice.dom.test.ts | 37 ------ .../lib/useAccountSwitchNotice.dom.test.ts | 65 ++++++++++ frontend/src/lib/useAccountSwitchNotice.ts | 19 +++ 19 files changed, 487 insertions(+), 121 deletions(-) create mode 100644 frontend/src/lib/accountSwitchNotice.dom.test.ts delete mode 100644 frontend/src/lib/refusedSwitchNotice.dom.test.ts create mode 100644 frontend/src/lib/useAccountSwitchNotice.dom.test.ts create mode 100644 frontend/src/lib/useAccountSwitchNotice.ts diff --git a/backend/app/auth.py b/backend/app/auth.py index 627bbdf3..c21de036 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -272,15 +272,23 @@ def stamp_session(request: Request, user: User) -> None: EXPECTED_ACCOUNT_HEADER = "x-siftlode-expect-account" +def built_for_another_account(user_id: int, expected: str | None) -> bool: + """Whether a tab naming `expected` is showing another account than the one signed in. No name is + no claim (a caller from before this check, a script): only a mismatch is refused, and the + session stays — it is the other tab's sign-in, and it holds. The one comparison for every way a + tab names its account: this header on a request, `expect` on a navigation.""" + if expected is None or expected == str(user_id): + return False + log.info( + "a request from a tab built for another account was refused", + extra=logs.event("auth.session.account_changed", user_id=user_id), + ) + return True + + def current_user(request: Request, db: Session = Depends(get_db)) -> User: 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), - ) + if built_for_another_account(user.id, request.headers.get(EXPECTED_ACCOUNT_HEADER)): raise refused(401, "account_changed") return user diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 4ccd8a54..a36f4584 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -8,6 +8,7 @@ from datetime import datetime, timedelta, timezone from typing import Literal, NamedTuple from urllib.parse import urlencode +from authlib.common.security import generate_token from authlib.integrations.starlette_client import OAuth, OAuthError from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request from fastapi.responses import JSONResponse, RedirectResponse @@ -29,6 +30,7 @@ from app.auth import ( PASSWORD_MIN_LEN, READ_SCOPES, WRITE_SCOPES, + built_for_another_account, current_user, env_admin_canonicals, env_allowed_canonicals, @@ -322,6 +324,25 @@ class SetPasswordIn(BaseModel): current_password: str | None = None +# A connect in flight, in the session: `{state, uid, explicit}`. It names the OAuth state of the flow +# that set it, because the session is the browser's, not the flow's: a callback coming back with +# another state (an older flow, which authlib then refuses) must not spend the marker of the connect +# still open, or that connect would return as a plain sign-in — signing someone else in, or storing +# a YouTube grant on whichever account the Google identity already belongs to. +_LINK_MARKER = "oauth_link" + +# Where a navigation from a tab showing another account than the one signed in lands: the app, which +# boots as the account signed in now and says why (the frontend's `ACCOUNT_CHANGED_FLAG`). +ACCOUNT_CHANGED_LANDING = "/?account=changed" + + +def _take_link_marker(request: Request) -> dict | None: + marker = request.session.get(_LINK_MARKER) + if not isinstance(marker, dict) or marker.get("state") != request.query_params.get("state"): + return None + return request.session.pop(_LINK_MARKER) + + @router.get("/login") async def login(request: Request, db: Session = Depends(get_db)): # access_type=offline ensures a refresh_token on first authorization. We avoid @@ -332,11 +353,10 @@ 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) + # A sign-in, never a link. Starting it retires every earlier flow's state (authlib keeps only the + # newest), so a connect's marker left from before — abandoned at Google, or still open in + # another tab — names a flow that can no longer finish. + request.session.pop(_LINK_MARKER, None) return await client.authorize_redirect( request, _oauth_redirect_uri(request), @@ -357,13 +377,12 @@ async def callback( if client is None: return RedirectResponse(url="/") # Linking mode: an already-authenticated account is attaching this Google identity (a - # password account enabling YouTube or connecting SSO). The markers are set by - # /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. 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) + # password account enabling YouTube or connecting SSO). The marker is set by + # /auth/google/connect; when it is this flow's we attach to the current user instead of + # identifying by sub. Taken 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. A connect that never comes back here at all is cleared by /auth/login. + link = _take_link_marker(request) try: token = await client.authorize_access_token(request) except OAuthError as exc: @@ -385,8 +404,8 @@ async def callback( sub = userinfo["sub"] email = (userinfo.get("email") or "").lower() - if link_uid is not None: - return _complete_link(request, db, link_uid, link_explicit, sub, userinfo, token) + if link is not None: + return _complete_link(request, db, link["uid"], link["explicit"], sub, userinfo, token) if not is_allowed(db, email): # No uid to name — having no account here is the branch. And the address is left out on @@ -589,8 +608,8 @@ def _complete_link( """Attach the just-authorized Google identity to the already-signed-in account that started the connect. No is_allowed gate — they're already an active user. Refuses to hijack a 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.""" + `explicit` (from the link marker) distinguishes a "Connect Google" click from a YouTube grant — + the caller takes the marker up front so an aborted flow leaves nothing behind.""" # 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 @@ -626,9 +645,10 @@ async def logout(request: Request): """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): + if signed_in is not None and built_for_another_account( + signed_in, request.headers.get(EXPECTED_ACCOUNT_HEADER) + ): raise refused(401, "account_changed") request.session.clear() return JSONResponse({"ok": True}) @@ -1248,23 +1268,35 @@ _CONNECT: dict[str, tuple[str, str]] = { async def google_connect( request: Request, access: Literal["identity", "read", "write"], + expect: str | None = None, user: User = Depends(current_user), db: Session = Depends(get_db), ): """Attach Google to the signed-in account: its identity, or YouTube read/write access on top. - The shared callback sees the `oauth_link_*` markers and attaches what Google returns to THIS - account — without them a password account (no google_sub) would get a brand-new, separate Google - user created instead. It stores whatever scopes Google grants, so `can_read` / `can_write` flip - on once granted.""" + The shared callback sees the link marker and attaches what Google returns to THIS account — + without it a password account (no google_sub) would get a brand-new, separate Google user + created instead. It stores whatever scopes Google grants, so `can_read` / `can_write` flip on + once granted. + + `expect` is the account the tab was built for. A navigation carries no headers, so the check + every request gets (`EXPECTED_ACCOUNT_HEADER`) rides the query here: a tab still showing the + account signed in before would otherwise attach a Google identity to the one signed in now.""" + if built_for_another_account(user.id, expect): + return RedirectResponse(url=ACCOUNT_CHANGED_LANDING) client = google_oauth(db) if client is None: return RedirectResponse(url="/?link=error") scope, prompt = _CONNECT[access] - request.session["oauth_link_uid"] = user.id - request.session["oauth_link_explicit"] = access == "identity" + state = generate_token(48) + request.session[_LINK_MARKER] = { + "state": state, + "uid": user.id, + "explicit": access == "identity", + } return await client.authorize_redirect( request, _oauth_redirect_uri(request), + state=state, access_type="offline", prompt=prompt, include_granted_scopes="true", diff --git a/backend/app/routes/me.py b/backend/app/routes/me.py index 612b307e..0c5101cd 100644 --- a/backend/app/routes/me.py +++ b/backend/app/routes/me.py @@ -139,7 +139,10 @@ def update_preferences( # 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) + # FOR NO KEY UPDATE, as bump_session_epoch: it still orders two merges, and a transaction that + # only added a row referring to the account (a playlist pull, then minutes of YouTube) does not + # hold the save up. + db.refresh(user, with_for_update={"key_share": True}) merged = dict(user.preferences or {}) merged.update(preferences.root) user.preferences = merged diff --git a/backend/tests/test_google_callback.py b/backend/tests/test_google_callback.py index 26daf0ad..4390d403 100644 --- a/backend/tests/test_google_callback.py +++ b/backend/tests/test_google_callback.py @@ -7,6 +7,7 @@ the refused sign-in that doubles as an access request, and the link flow's confl """ import pytest +from authlib.integrations.starlette_client import OAuthError from sqlalchemy import select from starlette.responses import RedirectResponse @@ -19,7 +20,8 @@ _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.""" + the token exchange when Google redirects back. Like authlib, it honours only the newest flow's + state: starting a flow retires every earlier one.""" def __init__(self, userinfo: dict): self.userinfo = userinfo @@ -29,7 +31,9 @@ class _FakeGoogle: self.asked = kw return RedirectResponse(url="/fake-google") - async def authorize_access_token(self, _request): + async def authorize_access_token(self, request): + if request.query_params.get("state") != self.asked.get("state"): + raise OAuthError(error="mismatching_state") return {"userinfo": self.userinfo, "access_token": "at", "scope": "openid email profile"} @@ -46,9 +50,13 @@ def google(monkeypatch): return fake -def _sign_in(client, google, **userinfo): +def _sign_in(client, google, state: str | None = None, **userinfo): + """Google redirecting back: by default with the state of the flow started last.""" google.userinfo = {"email_verified": True, **userinfo} - return client.get("/auth/callback", follow_redirects=False) + state = state or google.asked.get("state") + return client.get( + "/auth/callback", params={"state": state} if state else {}, follow_redirects=False + ) def _rows(db, model, column, value): @@ -305,6 +313,50 @@ def test_a_link_whose_session_was_revoked_meanwhile_does_not_finish(client, db, assert me.google_sub is None +def test_an_older_flow_coming_back_leaves_the_connect_in_flight_a_link( + client, db, google, make_user +): + # Two round-trips from one browser: an older one (another tab's, or Back to Google's screen) + # returns after a connect started. Its state is refused, and it must not spend the connect's + # marker: the connect itself, coming back, would then land as a plain sign-in. + me = make_user(email="me6@example.com") + _start_link(client, "me6@example.com") + + older = _sign_in(client, google, state="an-older-flow", sub="sub-6", email="g6@example.com") + assert older.headers["location"] == "/?oauth=cancelled" + r = _sign_in(client, google, sub="sub-6", email="g6@example.com") + + assert r.headers["location"] == "/?link=ok" + db.refresh(me) + assert me.google_sub == "sub-6" + + +def test_a_connect_from_a_tab_showing_another_account_starts_nothing(client, google, make_user): + # A navigation carries no headers, so the tab names the account it was built for in the query. + # One still showing the account signed in before must not start a link onto the one signed in + # now: it lands on the app, which boots as that one and says why. + me = make_user(email="me7@example.com") + _password_login(client, "me7@example.com") + + r = client.get( + f"/auth/google/connect?access=identity&expect={me.id + 1}", follow_redirects=False + ) + + assert r.headers["location"] == auth_routes.ACCOUNT_CHANGED_LANDING == "/?account=changed" + assert google.asked == {} + assert client.get("/api/me").json()["email"] == "me7@example.com" + + +def test_a_connect_naming_the_signed_in_account_goes_to_google(client, google, make_user): + me = make_user(email="me8@example.com") + _password_login(client, "me8@example.com") + + r = client.get(f"/auth/google/connect?access=identity&expect={me.id}", follow_redirects=False) + + assert r.headers["location"] == "/fake-google" + assert google.asked["state"] + + @pytest.mark.parametrize( ("access", "scope", "prompt"), [ diff --git a/backend/tests/test_me_preferences.py b/backend/tests/test_me_preferences.py index ae157261..cf284d9a 100644 --- a/backend/tests/test_me_preferences.py +++ b/backend/tests/test_me_preferences.py @@ -6,7 +6,8 @@ the request first read it — two merges over the same earlier read keep only th """ from fastapi import Depends, Request -from sqlalchemy import update +from sqlalchemy import event, update +from sqlalchemy.engine import Engine from sqlalchemy.orm import Session from app.auth import current_user @@ -42,3 +43,32 @@ def test_a_save_keeps_a_key_another_save_stored_after_this_one_read_the_account( assert r.status_code == 200 db.expire_all() assert db.get(User, user.id).preferences == {"seenVersion": "1.2.3", "startOnHome": False} + + +def test_the_merge_reads_the_account_under_a_lock_that_orders_merges_and_nothing_else( + client, make_user +): + # The re-read above is half of it: two merges that both read before either commits would still + # keep only the last, unless the read takes a lock the other merge waits for. FOR NO KEY UPDATE + # is that lock, and the weakest one: a plain FOR UPDATE would also wait behind any transaction + # that only added a row referring to the account (a playlist pull, then minutes of YouTube). + make_user(email="prefs-lock@example.com", password=_PW) + assert ( + client.post( + "/auth/password-login", json={"email": "prefs-lock@example.com", "password": _PW} + ) + ).status_code == 200 + sent: list[str] = [] + + def record(_conn, _cursor, statement, *_rest): + sent.append(statement) + + event.listen(Engine, "before_cursor_execute", record) + try: + assert client.put("/api/me/preferences", json={"startOnHome": False}).status_code == 200 + finally: + event.remove(Engine, "before_cursor_execute", record) + + locks = [s for s in sent if "FROM users" in s and " FOR " in s] + assert locks, sent + assert all(s.rstrip().endswith("FOR NO KEY UPDATE") for s in locks), locks diff --git a/frontend/e2e/README.md b/frontend/e2e/README.md index ab6799d3..8ea9c7c9 100644 --- a/frontend/e2e/README.md +++ b/frontend/e2e/README.md @@ -25,7 +25,8 @@ that one error again and passes every other failure straight on. Don't run two e same machine at once. **Browsers** — every flow runs in three projects: `chromium` (Desktop Chrome), `webkit` (Desktop -Safari) and `ipad` (iPad gen 7 landscape: 1080×810, touch, mobile). Install the engines once per +Safari) and `ipad` (iPad gen 7 landscape: 1080×810, touch, mobile) — except `a11y`, `member` and +`devices`, which run on chromium only (`CHROMIUM_ONLY`; why is in the `docs/gates.md` row). Install the engines once per machine with `npx playwright install chromium webkit`; run one project with `npx playwright test --project=ipad`. WebKit catches what every iOS browser inherits from the engine (JS, CSS/layout, event order). It does **not** reproduce iPad media refusals: Playwright's WebKit diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index eef58eec..c0d3709c 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -7,7 +7,8 @@ import { api, setUnauthorizedHandler, type Me } from "./lib/api"; import { notifyActionError } from "./lib/actionErrors"; import { setLanguage, type LangCode } from "./i18n"; import { clearParams } from "./lib/urlState"; -import { refusedSwitchNotice } from "./lib/entryUrl"; +import { reloadAfterRefusal } from "./lib/entryUrl"; +import { useAccountSwitchNotice } from "./lib/useAccountSwitchNotice"; import { PREF_KEY, saveLayoutLocal, @@ -231,26 +232,19 @@ export default function App() { if (signedIn) clearParams("access", "verify", "reset"); }, [signedIn]); - // A sign-in as someone else that the server refused (an account not approved, or suspended) came - // back here as the account still signed in — say so, once. - const switchAnswered = useRef(false); - useEffect(() => { - if (!signedIn || switchAnswered.current) return; - switchAnswered.current = true; - const notice = refusedSwitchNotice(); - if (notice) notify({ level: notice.level, message: t(notice.key) }); - }, [signedIn, t]); + // A sign-in as someone else that the server refused, or a reload as another account: say so, once. + useAccountSwitchNotice(signedIn); // Any request that 401s means the session ended server-side. If we were signed in (cached // `me` is a user object), reload to the login page at once (also covers any click that hits // the API). Guarded on a cached user so a stray 401 while logged out (cached `me` is null) - // can't loop the public landing. + // can't loop the public landing. A tab showing another account than the one signed in now + // reloads as that one instead, and says why (`reloadAfterRefusal`). useEffect(() => { - setUnauthorizedHandler(() => { - if (queryClient.getQueryData(qk.me())) { - queryClient.clear(); - window.location.reload(); - } + setUnauthorizedHandler((err) => { + if (!queryClient.getQueryData(qk.me())) return; + queryClient.clear(); + reloadAfterRefusal(err); }); return () => setUnauthorizedHandler(null); }, [queryClient]); diff --git a/frontend/src/components/NavigationProvider.dom.test.tsx b/frontend/src/components/NavigationProvider.dom.test.tsx index a52d814a..61b6b5ea 100644 --- a/frontend/src/components/NavigationProvider.dom.test.tsx +++ b/frontend/src/components/NavigationProvider.dom.test.tsx @@ -370,6 +370,20 @@ describe("NavigationProvider · a page the account cannot open", () => { await settle(); expect(result.current.page).toBe("feed"); }); + + it("closes a channel page open over the lost page, on screen and in the entry", async () => { + window.history.replaceState({ sfPage: "users" }, "", "/?page=users"); + const { result } = render(); + act(() => result.current.openChannel("UC1", "One")); + expect(window.location.search).toContain("chan=UC1"); + act(() => { + qc.setQueryData(qk.me(), member); + }); + await settle(); + expect(result.current.channelView).toBeNull(); + expect(window.location.search).toBe(""); + expect(window.history.state?._chan).toBeUndefined(); + }); }); describe("NavigationProvider · leaving a sub-view whose entry is the oldest of this document", () => { diff --git a/frontend/src/components/NavigationProvider.tsx b/frontend/src/components/NavigationProvider.tsx index aa224198..8075b0ab 100644 --- a/frontend/src/components/NavigationProvider.tsx +++ b/frontend/src/components/NavigationProvider.tsx @@ -67,16 +67,6 @@ export function NavigationProvider({ children }: { children: ReactNode }) { // and the address bar (stamped from it below) agree. const me = useMe(); const page = reachablePage(chosenPage, me); - // A page this account can no longer open (a role taken away, a module switched off mid-session) - // is given up, not kept behind the feed: kept, the address bar would go on naming it, a click on - // Feed would be a no-op (setPage's same-page guard sees the feed already on screen), and a later - // grant would switch the page by itself. Adjusted during render; the history entry and the - // remembered page follow in the effect below. - const [givenUp, setGivenUp] = useState(0); - if (page !== chosenPage) { - setPageState(page); - setGivenUp((n) => n + 1); - } // Ephemeral: not persisted and not in the URL, so a reload returns to the normal feed (a search // spends quota, so we never auto-replay it) — UNLESS it was zero-quota scrape-sourced, which rides // in history.state and is free to re-fetch. The popstate handler derives it from history.state so @@ -92,6 +82,18 @@ export function NavigationProvider({ children }: { children: ReactNode }) { const [channelView, setChannelView] = useState(() => channelViewFrom(window.history.state, window.location.search), ); + // A page this account can no longer open (a role taken away, a module switched off mid-session) + // is given up, not kept behind the feed: kept, the address bar would go on naming it, a click on + // Feed would be a no-op (setPage's same-page guard sees the feed already on screen), and a later + // grant would switch the page by itself. A channel page open over it closes with it, as on any + // page switch. Adjusted during render; the history entry and the remembered page follow in the + // effect below. + const [givenUp, setGivenUp] = useState(0); + if (page !== chosenPage) { + setPageState(page); + setChannelView(null); + setGivenUp((n) => n + 1); + } // Latest-value refs so the STABLE action callbacks can read the latest state without depending on // it — a dep would recreate them on every navigation and re-render every actions consumer (e.g. @@ -211,12 +213,14 @@ export function NavigationProvider({ children }: { children: ReactNode }) { }, [signedIn, page, objectTitle, i18n.language, t]); // The page given up above is the feed, and the current entry says so (replaced, not pushed: no - // step was taken). Its own keys go with it, as on any page switch. + // step was taken). Its own keys go with it, and the channel page closed with it, as on any page + // switch — left in the entry, a reload would reopen what the screen no longer shows. useEffect(() => { if (!givenUp) return; setAccountRaw(PAGE_KEY, "feed"); + const { _chan: _closedChan, _chanName: _closedName, ...rest } = window.history.state ?? {}; window.history.replaceState( - { ...window.history.state, sfPage: "feed" }, + { ...rest, sfPage: "feed" }, "", urlWithParams(pageRootParams("feed")), ); diff --git a/frontend/src/i18n/locales/en/header.json b/frontend/src/i18n/locales/en/header.json index f441943a..3c9e775d 100644 --- a/frontend/src/i18n/locales/en/header.json +++ b/frontend/src/i18n/locales/en/header.json @@ -32,6 +32,9 @@ "signOut": "Sign out", "signInAsOther": "Sign in as someone else", "switchNotApproved": "That Google account isn't approved for this instance yet, so you're still signed in as before.", + "switchSuspended": "That account is suspended, so you're still signed in as before.", + "switchRequested": "We recorded an access request for {{email}}. Until an admin approves it, you're still signed in as before.", + "tabSwitched": "This tab was showing another account, so it reloaded as the one signed in now.", "googleAddress": "Google account: {{email}}", "activity": "Activity" }, diff --git a/frontend/src/i18n/locales/hu/header.json b/frontend/src/i18n/locales/hu/header.json index 17bd49ed..348a9016 100644 --- a/frontend/src/i18n/locales/hu/header.json +++ b/frontend/src/i18n/locales/hu/header.json @@ -32,6 +32,9 @@ "signOut": "Kijelentkezés", "signInAsOther": "Belépés más fiókkal", "switchNotApproved": "Ez a Google-fiók még nincs jóváhagyva ezen a példányon, ezért továbbra is az eddigi fiókkal vagy belépve.", + "switchSuspended": "Az a fiók fel van függesztve, ezért továbbra is az eddigi fiókkal vagy belépve.", + "switchRequested": "Rögzítettük a hozzáférési kérelmet ehhez a címhez: {{email}}. Amíg egy admin jóvá nem hagyja, továbbra is az eddigi fiókkal vagy belépve.", + "tabSwitched": "Ez a fül egy másik fiókot mutatott, ezért újratöltött az éppen bejelentkezett fiókkal.", "googleAddress": "Google-fiók: {{email}}", "activity": "Napló" }, diff --git a/frontend/src/lib/accountSwitchNotice.dom.test.ts b/frontend/src/lib/accountSwitchNotice.dom.test.ts new file mode 100644 index 00000000..39803f89 --- /dev/null +++ b/frontend/src/lib/accountSwitchNotice.dom.test.ts @@ -0,0 +1,111 @@ +// @vitest-environment jsdom +// +// "Sign in as someone else" with a Google account the server will not sign in (not approved, or +// suspended) comes back to the account still signed in here, with the flags the sign-in page reads; +// a tab the server refused because another account signed in under it reloads with a flag of its +// own. Each says so in the signed-in app's words, not the sign-in page's. +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + ACCOUNT_CHANGED_FLAG, + accountChangedUrl, + accountSwitchNotice, + clearEntryUrl, + reloadAfterRefusal, +} from "./entryUrl"; + +const notice = (search: string, hash = "") => + accountSwitchNotice(new URLSearchParams(search), new URLSearchParams(hash)); + +afterEach(() => { + window.history.replaceState(null, "", "/"); + vi.unstubAllGlobals(); +}); + +describe("a switch of account, told to the account signed in", () => { + it("names the address a request was recorded for", () => { + expect(notice("", "access=requested&email=b%2Btag%40example.com")).toEqual({ + level: "info", + key: "header.account.switchRequested", + values: { email: "b+tag@example.com" }, + }); + }); + + it("names the address the request lives under when that is another spelling", () => { + expect(notice("", "access=requested&email=bob%40x.com&linked=b.ob%40x.com")?.values).toEqual({ + email: "b.ob@x.com", + }); + }); + + it("says the account is not approved, and nothing changed here", () => { + expect(notice("", "access=denied")).toEqual({ + level: "warning", + key: "header.account.switchNotApproved", + }); + }); + + it("says that account is suspended, not this one", () => { + expect(notice("?login=suspended")).toEqual({ + level: "warning", + key: "header.account.switchSuspended", + }); + }); + + it("says the tab reloaded as the account signed in now", () => { + expect(notice(`?page=stats&${ACCOUNT_CHANGED_FLAG}`)).toEqual({ + level: "info", + key: "header.account.tabSwitched", + }); + }); + + it("says nothing on an ordinary load", () => { + expect(notice("?page=stats", "")).toBeNull(); + }); +}); + +describe("the reload of a tab another account signed in under", () => { + it("stays on its page and carries the flag", () => { + window.history.replaceState(null, "", "/?page=stats&chan=UC1#x=1"); + const url = new URL(accountChangedUrl(), "http://h"); + expect(url.pathname).toBe("/"); + expect(Object.fromEntries(url.searchParams)).toEqual({ + page: "stats", + chan: "UC1", + account: "changed", + }); + expect(url.hash).toBe("#x=1"); + }); + + it("drops the flag once read, so a later reload does not say it again", () => { + window.history.replaceState(null, "", `/?page=stats&${ACCOUNT_CHANGED_FLAG}`); + clearEntryUrl(); + expect(window.location.search).toBe("?page=stats"); + }); +}); + +describe("a tab whose session the server refused", () => { + const stubLocation = () => { + const loc = { + pathname: "/", + search: "?page=stats", + hash: "", + replace: vi.fn(), + reload: vi.fn(), + }; + vi.stubGlobal("location", loc); + return loc; + }; + + it("reloads as the account signed in now, saying why, when it showed another", () => { + const loc = stubLocation(); + reloadAfterRefusal({ detailData: { code: "refused", reason: "account_changed" } }); + expect(loc.replace).toHaveBeenCalledWith(`/?page=stats&${ACCOUNT_CHANGED_FLAG}`); + expect(loc.reload).not.toHaveBeenCalled(); + }); + + it("reloads plainly when the session ended", () => { + const loc = stubLocation(); + reloadAfterRefusal({ detailData: { code: "refused", reason: "not_authenticated" } }); + expect(loc.reload).toHaveBeenCalled(); + expect(loc.replace).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index dd11c681..62f39ec8 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -223,10 +223,11 @@ function reportFailure(err: HttpError, quiet: boolean): void { if (RETRIABLE_GATEWAY.has(err.status)) { markConnectivityLost(); } else if (err.status === 401) { - // Session ended server-side (e.g. the account was suspended or deleted mid-visit). Let the app - // shell decide what to do (drop to the login page if we were signed in). Not a modal — but + // Session ended server-side (e.g. the account was suspended or deleted mid-visit), or this tab + // shows another account than the one signed in now. Let the app shell decide what to do (drop to + // the login page, or reload as the account signed in, if we were signed in). Not a modal — but // handled all the same, so not the caller's to toast either. - onUnauthorized?.(); + onUnauthorized?.(err); reported.add(err); } else if (quiet) { /* caller handles the error inline — no global modal */ @@ -256,8 +257,8 @@ export interface ReqConfig { // page. The handler itself guards against firing when we were never signed in, so it's safe to // call for every 401. (The bootstrap /api/me probe no longer 401s — it returns 200 with a null // user when logged out — but other protected endpoints still do.) -let onUnauthorized: (() => void) | null = null; -export function setUnauthorizedHandler(fn: (() => void) | null): void { +let onUnauthorized: ((err: HttpError) => void) | null = null; +export function setUnauthorizedHandler(fn: ((err: HttpError) => void) | null): void { onUnauthorized = fn; } diff --git a/frontend/src/lib/api/expectedAccount.contract.test.ts b/frontend/src/lib/api/expectedAccount.contract.test.ts index d6b15626..9cfce156 100644 --- a/frontend/src/lib/api/expectedAccount.contract.test.ts +++ b/frontend/src/lib/api/expectedAccount.contract.test.ts @@ -7,7 +7,10 @@ import { describe, expect, it } from "vitest"; // one without a word. const read = (path: string) => readFileSync(new URL(path, import.meta.url), "utf-8"); const AUTH = read("../../../../backend/app/auth.py"); +const ROUTES = read("../../../../backend/app/routes/auth.py"); const CORE = read("./core.ts"); +const CONNECT = read("../googleConnect.ts"); +const ENTRY = read("../entryUrl.ts"); describe("the account a tab was built for", () => { it("is the header the server checks", () => { @@ -16,4 +19,17 @@ describe("the account a tab was built for", () => { expect(backend).toBeTruthy(); expect(backend).toBe(frontend?.toLowerCase()); }); + + it("rides a Google connect as the query parameter the route reads", () => { + // A navigation carries no headers; a rename on one side would leave the connect unchecked. + expect(CONNECT).toMatch(/params\.set\("expect", account\)/); + expect(ROUTES).toMatch(/^ {4}expect: str \| None = None,$/m); + }); + + it("lands a refused navigation where the reloaded tab says why", () => { + const landing = /^ACCOUNT_CHANGED_LANDING = "\/\?([^"]+)"$/m.exec(ROUTES)?.[1]; + const flag = /^export const ACCOUNT_CHANGED_FLAG = "([^"]+)";$/m.exec(ENTRY)?.[1]; + expect(landing).toBeTruthy(); + expect(landing).toBe(flag); + }); }); diff --git a/frontend/src/lib/entryUrl.ts b/frontend/src/lib/entryUrl.ts index 0f21f878..24a7e19e 100644 --- a/frontend/src/lib/entryUrl.ts +++ b/frontend/src/lib/entryUrl.ts @@ -12,7 +12,7 @@ // Secret tokens (verify, reset) ride the FRAGMENT so they never reach the server, a proxy log or a // Referer header (SB3); so does the Google access-request notice (`access`, and the addresses it // names) — an email in a query string lands in every reverse-proxy access log. Plain status flags -// with nothing personal in them (login, deleted) ride the query string. +// with nothing personal in them (login, deleted, account) ride the query string. import type { NotifLevel } from "./notifications"; @@ -24,7 +24,31 @@ export const entryHash = new URLSearchParams(window.location.hash.replace(/^#/, * in its old shape — it rides the fragment now). Deliberately NOT `link`, * `oauth`, `admin` or the setup `token` — those have their own readers, and some of them (the setup * wizard, the lazily-mounted module views) do not run until several commits later. */ -const ONE_TIME_QUERY = ["login", "deleted", "access"] as const; +const ONE_TIME_QUERY = ["login", "deleted", "access", "account"] as const; + +/** The flag a tab reloads with when it was showing another account than the one signed in now (the + * server refused it: `account_changed`), and where a navigation the server refused for that lands + * (backend `ACCOUNT_CHANGED_LANDING`). A reload wipes any toast, so the reason rides the URL. */ +export const ACCOUNT_CHANGED_FLAG = "account=changed"; + +/** This page again, carrying `ACCOUNT_CHANGED_FLAG`: the tab boots as the account signed in now and + * says why, on the page it was on. */ +export function accountChangedUrl(): string { + const params = new URLSearchParams(window.location.search); + const [key, value] = ACCOUNT_CHANGED_FLAG.split("=") as [string, string]; + params.set(key, value); + return `${window.location.pathname}?${params.toString()}${window.location.hash}`; +} + +/** Reload a tab whose session the server refused (a 401). Showing another account than the one + * signed in now (`account_changed`: a sign-in as someone else in another tab), it boots as that one + * and says why — a Sign out that silently left someone else signed in would read as broken. Any + * other refusal means the session ended: a plain reload lands on the sign-in page. */ +export function reloadAfterRefusal(err: { detailData?: unknown }): void { + if ((err.detailData as { reason?: string } | undefined)?.reason === "account_changed") + window.location.replace(accountChangedUrl()); + else window.location.reload(); +} /** Strip the one-time entry state from the address bar, so a token doesn't linger in history or get * re-sent on a reload. ONE owner calls this (useVerifyEntry, which App mounts unconditionally) — a @@ -61,18 +85,33 @@ export function clearEntryUrl(): void { ); } -/** What a sign-in as someone else that did not happen says to the account still signed in here. The - * callback answers a Google account that is not approved, or is suspended, without replacing the - * session, and with the flags the sign-in page reads; the signed-in app read none of them, so the - * person came back as before without a word. A request recorded on the way says so. */ -export function refusedSwitchNotice( +export interface AccountNotice { + level: NotifLevel; + key: string; + values?: Record; +} + +/** What the entry URL says to the account signed in here about a switch of account. A sign-in as + * someone else that did not happen: the callback answers a Google account that is not approved, or + * is suspended, without replacing the session, and with the flags the sign-in page reads — the + * signed-in app read none of them, so the person came back as before without a word. Its own copy, + * not the sign-in page's: beside this account's avatar, "this account is suspended" would read as + * this one. A request recorded on the way names the address it lives under (`linked`, when that is + * not the one Google returned). And a tab that reloaded because another account signed in under it + * says that it did. */ +export function accountSwitchNotice( search: URLSearchParams = entrySearch, hash: URLSearchParams = entryHash, -): { level: NotifLevel; key: string } | null { +): AccountNotice | null { + if (search.get("account") === "changed") + return { level: "info", key: "header.account.tabSwitched" }; if (search.get("login") === "suspended") - return { level: "warning", key: "welcome.auth.suspended" }; + return { level: "warning", key: "header.account.switchSuspended" }; const access = hash.get("access"); - if (access === "requested") return { level: "info", key: "welcome.auth.accessRequested" }; - if (access === "denied") return { level: "warning", key: "header.account.switchNotApproved" }; + const email = hash.get("linked") || hash.get("email"); + if (access === "requested" && email) + return { level: "info", key: "header.account.switchRequested", values: { email } }; + if (access === "requested" || access === "denied") + return { level: "warning", key: "header.account.switchNotApproved" }; return null; } diff --git a/frontend/src/lib/googleConnect.ts b/frontend/src/lib/googleConnect.ts index 01e73635..3c70f102 100644 --- a/frontend/src/lib/googleConnect.ts +++ b/frontend/src/lib/googleConnect.ts @@ -1,13 +1,21 @@ // Every in-app Google round-trip starts at one backend route (auth.py `google_connect`): attaching a // Google sign-in to the account (`identity`), or granting YouTube access on top (`read`, `write`). // The page navigates away to Google and comes back through the shared callback, which lands on -// `?link=…` (App.tsx) for `identity` and on the app itself for a grant. +// `?link=…` (App.tsx) for `identity` and on the app itself for a grant. A navigation carries no +// headers, so it names the account the tab was built for in the query (`expect`) — the check every +// request gets (`identityHeaders`): a tab still showing the account signed in before would otherwise +// attach Google to the one signed in now. + +import { claimedAccount } from "./storage"; export const GOOGLE_ACCESS = ["identity", "read", "write"] as const; export type GoogleAccess = (typeof GOOGLE_ACCESS)[number]; export function googleConnectUrl(access: GoogleAccess): string { - return `/auth/google/connect?access=${access}`; + const params = new URLSearchParams({ access }); + const account = claimedAccount(); + if (account) params.set("expect", account); + return `/auth/google/connect?${params.toString()}`; } /** Leave for Google's sign-in or consent screen. */ diff --git a/frontend/src/lib/refusedSwitchNotice.dom.test.ts b/frontend/src/lib/refusedSwitchNotice.dom.test.ts deleted file mode 100644 index 1cb46ac3..00000000 --- a/frontend/src/lib/refusedSwitchNotice.dom.test.ts +++ /dev/null @@ -1,37 +0,0 @@ -// @vitest-environment jsdom -// -// "Sign in as someone else" with a Google account the server will not sign in (not approved, or -// suspended) comes back to the account still signed in here, with the flags the sign-in page reads. -// The signed-in app read none of them, so the person was back as before without a word. -import { describe, expect, it } from "vitest"; -import { refusedSwitchNotice } from "./entryUrl"; - -const notice = (search: string, hash = "") => - refusedSwitchNotice(new URLSearchParams(search), new URLSearchParams(hash)); - -describe("a sign-in as someone else the server refused", () => { - it("says a request was recorded for an address not approved yet", () => { - expect(notice("", "access=requested&email=b%40example.com")).toEqual({ - level: "info", - key: "welcome.auth.accessRequested", - }); - }); - - it("says the account is not approved, and nothing changed here", () => { - expect(notice("", "access=denied")).toEqual({ - level: "warning", - key: "header.account.switchNotApproved", - }); - }); - - it("says the account is suspended", () => { - expect(notice("?login=suspended")).toEqual({ - level: "warning", - key: "welcome.auth.suspended", - }); - }); - - it("says nothing on an ordinary load", () => { - expect(notice("?page=stats", "")).toBeNull(); - }); -}); diff --git a/frontend/src/lib/useAccountSwitchNotice.dom.test.ts b/frontend/src/lib/useAccountSwitchNotice.dom.test.ts new file mode 100644 index 00000000..ea83cdb5 --- /dev/null +++ b/frontend/src/lib/useAccountSwitchNotice.dom.test.ts @@ -0,0 +1,65 @@ +// @vitest-environment jsdom +// +// The app says what its entry URL tells the account signed in about a switch of account, once, and +// only once that account is known. Without the hook in the shell, a refused switch or a tab that +// reloaded as another account comes back without a word. +import { afterEach, describe, expect, it, vi } from "vitest"; +import { renderHook } from "@testing-library/react"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ + t: (key: string, values?: Record) => + values ? `${key} ${JSON.stringify(values)}` : key, + i18n: { language: "en" }, + }), + }; +}); +const notifyMock = vi.fn(); +vi.mock("./notifications", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, notify: (...a: unknown[]) => notifyMock(...a) }; +}); +const notice = vi.fn(); +vi.mock("./entryUrl", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, accountSwitchNotice: () => notice() }; +}); +const { useAccountSwitchNotice } = await import("./useAccountSwitchNotice"); + +afterEach(() => { + notifyMock.mockClear(); + notice.mockReset(); +}); + +describe("the shell's word on a switch of account", () => { + it("waits for the account, then says it once", () => { + notice.mockReturnValue({ + level: "info", + key: "header.account.switchRequested", + values: { email: "b@example.com" }, + }); + const { rerender } = renderHook(({ signedIn }) => useAccountSwitchNotice(signedIn), { + initialProps: { signedIn: false }, + }); + expect(notifyMock).not.toHaveBeenCalled(); + + rerender({ signedIn: true }); + rerender({ signedIn: false }); + rerender({ signedIn: true }); + + expect(notifyMock).toHaveBeenCalledTimes(1); + expect(notifyMock).toHaveBeenCalledWith({ + level: "info", + message: 'header.account.switchRequested {"email":"b@example.com"}', + }); + }); + + it("says nothing when the entry URL has nothing to say", () => { + notice.mockReturnValue(null); + renderHook(() => useAccountSwitchNotice(true)); + expect(notifyMock).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/lib/useAccountSwitchNotice.ts b/frontend/src/lib/useAccountSwitchNotice.ts new file mode 100644 index 00000000..77e72de2 --- /dev/null +++ b/frontend/src/lib/useAccountSwitchNotice.ts @@ -0,0 +1,19 @@ +import { useEffect, useRef } from "react"; +import { useTranslation } from "react-i18next"; +import { accountSwitchNotice } from "./entryUrl"; +import { notify } from "./notifications"; + +/** Say, once per load, what the entry URL tells the account signed in here about a switch of + * account (`accountSwitchNotice`): a sign-in as someone else the server refused, or a tab that + * reloaded because another account signed in under it. It waits for the account — until it is + * known there is no one signed in to say it to. */ +export function useAccountSwitchNotice(signedIn: boolean): void { + const { t } = useTranslation(); + const said = useRef(false); + useEffect(() => { + if (!signedIn || said.current) return; + said.current = true; + const notice = accountSwitchNotice(); + if (notice) notify({ level: notice.level, message: t(notice.key, notice.values) }); + }, [signedIn, t]); +}