From dc4f4f5143c5d0282e4b6196cbad9d73d7808098 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 20 Oct 2026 04:08:11 +0200 Subject: [PATCH 01/11] test(e2e): a plain member's shell is held end to end, and a page it cannot open lands on the feed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suite ran only as an admin, so nothing end to end held what a member sees. A second seeded account, a member with its own session, now walks every rail page (no admin entry, no admin call, no error state) and follows links to the admin pages. It found one: a member's link to an admin page showed the feed with no rail row current, and the address bar kept naming the admin page — only the content fell back. The navigation state now derives the page on screen through each module's gate (reachablePage), so the rail, the content, the tab title and the address bar agree, on a link, a Back, and a role taken away mid-session. --- frontend/e2e/README.md | 10 +- frontend/e2e/global.setup.ts | 80 +++-- frontend/e2e/helpers.ts | 3 + frontend/e2e/member.spec.ts | 78 +++++ frontend/playwright.config.ts | 7 +- frontend/src/App.tsx | 18 +- frontend/src/components/BootGate.dom.test.tsx | 5 +- .../NavigationProvider.dom.test.tsx | 40 ++- .../src/components/NavigationProvider.tsx | 10 +- frontend/src/lib/modules.ts | 8 + scripts/e2e_seed.py | 282 +++++++++--------- 11 files changed, 351 insertions(+), 190 deletions(-) create mode 100644 frontend/e2e/member.spec.ts diff --git a/frontend/e2e/README.md b/frontend/e2e/README.md index 6f8116fc..6e47b248 100644 --- a/frontend/e2e/README.md +++ b/frontend/e2e/README.md @@ -57,9 +57,13 @@ admin so the suite can reach the admin-only pages / render branches, matching th user), reset to a deterministic baseline every run by [`scripts/e2e_seed.py`](../../scripts/e2e_seed.py) (run in-container so it reuses the app's models + argon2 hasher): the top-3 organic channels subscribed, -one in-progress + one watched + one saved video, prefs pinned to `language: en`. Credentials come -from `E2E_EMAIL` (default `e2e@siftlode.local`) and `E2E_PASSWORD` — **required, no default**: run -the suite through the ops wrapper, which exports it from the secret store. Override the container name with +one in-progress + one watched + one saved video, prefs pinned to `language: en`. A second account, +the plain **member** `e2e-member@siftlode.local`, gets the same baseline and its own session +(`e2e/.auth/member.json`); only `member.spec.ts` runs as it, holding what a non-admin sees (no admin +entry, no admin call, an admin link lands on the feed). Credentials come from `E2E_EMAIL` (default +`e2e@siftlode.local`), `E2E_MEMBER_EMAIL` (default `e2e-member@siftlode.local`) and `E2E_PASSWORD` +(both accounts) — **required, no default**: run the suite through the ops wrapper, which exports it +from the secret store. Override the container name with `E2E_API_CONTAINER`; skip the in-container seed with `E2E_SKIP_SEED=1`. ## Selectors diff --git a/frontend/e2e/global.setup.ts b/frontend/e2e/global.setup.ts index 5c706ecf..4a292d5e 100644 --- a/frontend/e2e/global.setup.ts +++ b/frontend/e2e/global.setup.ts @@ -1,20 +1,23 @@ -import { test as setup, expect } from "@playwright/test"; +import { test as setup, expect, type Page } from "@playwright/test"; import { execFileSync } from "node:child_process"; import { mkdirSync, readFileSync } from "node:fs"; import { dirname, resolve } from "node:path"; import { fileURLToPath } from "node:url"; +import { MEMBER_STORAGE } from "./helpers"; import { retryConnect } from "./retryConnect"; /** * One-time setup for the characterization net (the `setup` project the tests depend on): * 1. preflight — the localdev :8080 stack must be up (owned by `siftlode dev`); - * 2. seed — reset the dedicated E2E account to a known baseline, in-container (idempotent); - * 3. authenticate ONCE via password-login and persist the session + `en` locale to - * storageState, so the real specs never re-auth (auth endpoints are rate-limited). + * 2. seed — reset the dedicated E2E accounts to a known baseline, in-container (idempotent); + * 3. authenticate each ONCE via password-login and persist its session + `en` locale to its own + * storageState, so the real specs never re-auth (auth endpoints are rate-limited). The suite + * runs as the admin; `member.spec.ts` runs as the plain member, from MEMBER_STORAGE. */ const STORAGE = "e2e/.auth/state.json"; const EMAIL = process.env.E2E_EMAIL || "e2e@siftlode.local"; +const MEMBER_EMAIL = process.env.E2E_MEMBER_EMAIL || "e2e-member@siftlode.local"; // No default: a password committed to a public repo is not a credential. The ops wrapper // (`siftlode e2e` / `check-full`) exports it from the secret store. const PASSWORD = process.env.E2E_PASSWORD ?? ""; @@ -27,14 +30,43 @@ const SEED_SCRIPT = resolve(HERE, "../../scripts/e2e_seed.py"); // The in-container seed only makes sense when the target is the local stack. const isLocalTarget = /\/\/(127\.0\.0\.1|localhost)[:/]/.test(BASE_URL); -setup("seed + authenticate", async ({ page }) => { +/** Sign `email` in on `page`'s context and save that context as `path`. */ +async function signIn(page: Page, email: string, path: string): Promise { + // These requests run in Node and share the page's cookie jar — so the `session` cookie the login + // receives is what storageState saves below — and a localhost connect the machine dropped is + // tried again (retryConnect). + const api = retryConnect(page.request); + const login = await api.post("/auth/password-login", { data: { email, password: PASSWORD } }); + expect( + login.ok(), + `password-login failed for ${email} — is the E2E account seeded?`, + ).toBeTruthy(); + + const me = await (await api.get("/api/me")).json(); + expect(me.authenticated, `session not established after login (${email})`).toBeTruthy(); + + // Pin the UI locale, suppress the first-login onboarding wizard (a full-screen overlay that + // would intercept every click — the test accounts have no YouTube connection, so it auto-opens), + // and opt the account out of starting on Home. A bare URL lands on Home, and the feed, channel + // and player flows this net locks all open "/" expecting the feed; the opt-out is the one + // Settings → Appearance → Startup writes. Both flags are per-account: + // `siftlode.onboarding.dismissed.`, `siftlode.startOnHome.`. + await page.goto("/"); + await page.evaluate((id: number) => { + localStorage.setItem("siftlode.lang", "en"); + localStorage.setItem(`siftlode.onboarding.dismissed.${id}`, "1"); + localStorage.setItem(`siftlode.startOnHome.${id}`, "0"); + }, me.id); + mkdirSync(dirname(path), { recursive: true }); + await page.context().storageState({ path }); +} + +setup("seed + authenticate", async ({ page, browser }) => { expect( PASSWORD, "E2E_PASSWORD is not set — run the suite through `siftlode e2e` (it reads the secret store)", ).not.toBe(""); - // These requests run in Node (sharing the page's cookie jar), so a localhost connect the machine - // dropped is tried again (retryConnect). const api = retryConnect(page.request); // 1. Preflight: fail fast (with a useful message) if the stack isn't running. @@ -55,6 +87,8 @@ setup("seed + authenticate", async ({ page }) => { "-e", `E2E_EMAIL=${EMAIL}`, "-e", + `E2E_MEMBER_EMAIL=${MEMBER_EMAIL}`, + "-e", `E2E_PASSWORD=${PASSWORD}`, CONTAINER, "python", @@ -69,28 +103,12 @@ setup("seed + authenticate", async ({ page }) => { } } - // 3. Authenticate. page.request shares the browser context's cookie jar, so the `session` - // cookie it receives is captured by storageState below. - const login = await api.post("/auth/password-login", { - data: { email: EMAIL, password: PASSWORD }, - }); - expect(login.ok(), "password-login failed — is the E2E account seeded?").toBeTruthy(); - - const me = await (await api.get("/api/me")).json(); - expect(me.authenticated, "session not established after login").toBeTruthy(); - - // 4. Pin the UI locale, suppress the first-login onboarding wizard (a full-screen overlay - // that would intercept every click — the test account has no YouTube connection, so it - // auto-opens), and opt the account out of starting on Home. A bare URL lands on - // Home, and the feed, channel and player flows this net locks all open "/" expecting the - // feed; the opt-out is the one Settings → Appearance → Startup writes. Both flags are - // per-account: `siftlode.onboarding.dismissed.`, `siftlode.startOnHome.`. - await page.goto("/"); - await page.evaluate((id: number) => { - localStorage.setItem("siftlode.lang", "en"); - localStorage.setItem(`siftlode.onboarding.dismissed.${id}`, "1"); - localStorage.setItem(`siftlode.startOnHome.${id}`, "0"); - }, me.id); - mkdirSync(dirname(STORAGE), { recursive: true }); - await page.context().storageState({ path: STORAGE }); + // 3. Authenticate each account in its own context: one cookie jar holds one session. + await signIn(page, EMAIL, STORAGE); + const memberContext = await browser.newContext({ storageState: { cookies: [], origins: [] } }); + try { + await signIn(await memberContext.newPage(), MEMBER_EMAIL, MEMBER_STORAGE); + } finally { + await memberContext.close(); + } }); diff --git a/frontend/e2e/helpers.ts b/frontend/e2e/helpers.ts index d0681f04..3e126291 100644 --- a/frontend/e2e/helpers.ts +++ b/frontend/e2e/helpers.ts @@ -1,5 +1,8 @@ import { expect, type Page } from "@playwright/test"; +/** The plain member's saved session (global.setup.ts signs it in); `member.spec.ts` runs from it. */ +export const MEMBER_STORAGE = "e2e/.auth/member.json"; + /** Open the app and wait for the authenticated shell (the nav rail) to be ready. */ export async function openApp(page: Page, path = "/"): Promise { await page.goto(path); diff --git a/frontend/e2e/member.spec.ts b/frontend/e2e/member.spec.ts new file mode 100644 index 00000000..c4dd40d6 --- /dev/null +++ b/frontend/e2e/member.spec.ts @@ -0,0 +1,78 @@ +import { test, expect, type Page, type Request, type Response } from "@playwright/test"; +import { MEMBER_STORAGE, openApp } from "./helpers"; + +// What a plain member (not an admin) sees. The rest of the suite runs as the admin, so without this +// spec nothing end to end holds the member's shell: the backend refuses the admin routes (pytest), +// but a member shown an admin entry, sent to an admin page or met by a refused request would pass. +test.use({ storageState: MEMBER_STORAGE }); + +const ADMIN_PAGES = ["scheduler", "config", "users", "activity"]; +// The pages every member has; Plex joins them only where the instance enables it. +const CORE = [ + "home", + "feed", + "channels", + "playlists", + "notifications", + "files", + "stats", + "settings", +]; + +/** Collect every admin call and every refused response the page makes while `run` goes. */ +async function watchRequests(page: Page, run: () => Promise): Promise { + const bad: string[] = []; + const onRequest = (r: Request) => { + if (new URL(r.url()).pathname.startsWith("/api/admin")) bad.push(`admin call ${r.url()}`); + }; + const onResponse = (r: Response) => { + if (r.status() === 401 || r.status() === 403) bad.push(`${r.status()} ${r.url()}`); + }; + page.on("request", onRequest); + page.on("response", onResponse); + try { + await run(); + } finally { + page.off("request", onRequest); + page.off("response", onResponse); + } + return bad; +} + +test.describe("a member", () => { + test("gets no admin entry, and every page of the rail opens without an error", async ({ + page, + }) => { + const bad = await watchRequests(page, async () => { + await openApp(page); + await expect(page.getByTestId("nav-admin")).toHaveCount(0); + + const rail = page + .getByRole("navigation", { name: "Primary navigation" }) + .locator('[data-testid^="nav-"]'); + const ids = (await rail.evaluateAll((els) => els.map((el) => el.getAttribute("data-testid")))) + .filter((id): id is string => id !== null) + .map((id) => id.replace(/^nav-/, "")); + expect(ids).toEqual(expect.arrayContaining(CORE)); + expect(ids.filter((id) => ADMIN_PAGES.includes(id))).toEqual([]); + + for (const id of ids) { + await page.getByTestId(`nav-${id}`).click(); + await expect(page.getByTestId(`nav-${id}`)).toHaveAttribute("aria-current", "page"); + // An error state is a `role=alert` StateMessage; a page that failed to load shows one. + await expect(page.getByRole("alert"), `error state on ${id}`).toHaveCount(0); + } + }); + expect(bad).toEqual([]); + }); + + test("a link to an admin page opens no admin page", async ({ page }) => { + for (const id of ADMIN_PAGES) { + const bad = await watchRequests(page, () => openApp(page, `/?page=${id}`)); + expect(bad, id).toEqual([]); + await expect(page.getByTestId("nav-admin")).toHaveCount(0); + // The member lands on one of their own pages instead. + await expect(page.locator('[data-testid^="nav-"][aria-current="page"]')).toHaveCount(1); + } + }); +}); diff --git a/frontend/playwright.config.ts b/frontend/playwright.config.ts index 0acd2290..70358c41 100644 --- a/frontend/playwright.config.ts +++ b/frontend/playwright.config.ts @@ -17,9 +17,10 @@ import { defineConfig, devices } from "@playwright/test"; const BASE_URL = process.env.PW_BASE_URL || "http://127.0.0.1:8080"; const STORAGE_STATE = "e2e/.auth/state.json"; -// The axe scan reads the DOM and the accessibility tree, which the engines build alike: one engine -// answers for all, so the other projects leave it out instead of tripling its time. -const CHROMIUM_ONLY = /a11y\.spec\.ts/; +// The axe scan reads the DOM and the accessibility tree, which the engines build alike, and the +// member spec checks which pages and requests a role gets, which no engine changes: one engine +// answers for all, so the other projects leave them out instead of tripling their time. +const CHROMIUM_ONLY = /(a11y|member)\.spec\.ts/; export default defineConfig({ testDir: "./e2e", diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 548faf26..02d6f1c9 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -7,7 +7,6 @@ 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 type { Page } from "./lib/urlState"; import { PREF_KEY, saveLayoutLocal, @@ -22,7 +21,7 @@ import { useVerifyEntry } from "./lib/useVerifyEntry"; import { useNavigation, useNavigationActions } from "./components/navigationContext"; import { useFeedFilters, useFeedFiltersActions } from "./components/feedFiltersContext"; import { useFeedView, useFeedViewActions } from "./components/feedViewContext"; -import { moduleDef, moduleOrder } from "./lib/modules"; +import { moduleOrder } from "./lib/modules"; import { PAGE_CONTENT, PAGE_RAIL, preloadPages } from "./components/moduleRegistry"; import { lazyWithPreload } from "./lib/lazyWithPreload"; import { LoadingState } from "./components/ui/QueryState"; @@ -373,14 +372,11 @@ export default function App() { ? `yt:${ytSearch}` : `feed:${JSON.stringify({ ...filters, q: "" })}`; - // The module registry drives the render from `page` alone — no per-page chrome conditionals. A - // page the account can't reach (a gated module via stale nav / a deep link) falls back to the feed, - // exactly as the old ternary's fallthrough did. A channel page overlays the content column, so no - // side rail shows behind it. - const pageGate = moduleDef(page).gate; - const activePage: Page = !pageGate || pageGate(me!) ? page : "feed"; - const { Component: PageContent, maxWidth } = PAGE_CONTENT[activePage]; - const railDef = channelView ? undefined : PAGE_RAIL[activePage]; + // The module registry drives the render from `page` alone — no per-page chrome conditionals. A page + // the account can't reach never gets here: the navigation state already moved it to the feed + // (`reachablePage`). A channel page overlays the content column, so no side rail shows behind it. + const { Component: PageContent, maxWidth } = PAGE_CONTENT[page]; + const railDef = channelView ? undefined : PAGE_RAIL[page]; return (
@@ -444,7 +440,7 @@ export default function App() { ) : ( } diff --git a/frontend/src/components/BootGate.dom.test.tsx b/frontend/src/components/BootGate.dom.test.tsx index e281c054..76452d45 100644 --- a/frontend/src/components/BootGate.dom.test.tsx +++ b/frontend/src/components/BootGate.dom.test.tsx @@ -47,7 +47,10 @@ function mount() { ); return qc; } -const account = (over: Partial = {}) => ({ id: 5, preferences: {}, ...over }) as Me; +// On a Plex-enabled instance: the remembered page these cases restore is the Plex module, which only +// an instance with Plex lets an account open. +const account = (over: Partial = {}) => + ({ id: 5, preferences: {}, plex_enabled: true, ...over }) as Me; let savePrefs: MockInstance; const retryButton = () => screen.findByRole("button", { name: i18n.t("common.retry") }); diff --git a/frontend/src/components/NavigationProvider.dom.test.tsx b/frontend/src/components/NavigationProvider.dom.test.tsx index 630c2a61..888eefb9 100644 --- a/frontend/src/components/NavigationProvider.dom.test.tsx +++ b/frontend/src/components/NavigationProvider.dom.test.tsx @@ -79,7 +79,8 @@ async function openOverlay() { beforeEach(() => { qc = new QueryClient(); - qc.setQueryData(qk.me(), { id: 1 }); + // An admin on a Plex-enabled instance: every module is open to it, so no gate moves a page here. + qc.setQueryData(qk.me(), { id: 1, role: "admin", plex_enabled: true }); localStorage.clear(); sessionStorage.setItem("siftlode.activeAccount", "1"); // account-scoped page memory window.history.replaceState({ sfPage: "feed" }, "", "/"); @@ -295,6 +296,43 @@ describe("NavigationProvider · Back", () => { }); }); +describe("NavigationProvider · a page the account cannot open", () => { + const member = { id: 1, role: "user", plex_enabled: true }; + + // A member following a link to an admin page saw the feed with no rail row current: the page + // stayed "scheduler" while only the content fell back. + it("lands on the feed, and the address bar says so", () => { + qc.setQueryData(qk.me(), member); + window.history.replaceState(null, "", "/?page=scheduler&keep=1"); + const { result } = render(); + expect(result.current.page).toBe("feed"); + expect(window.location.search).toBe("?keep=1"); + expect(window.history.state).toMatchObject({ sfPage: "feed" }); + }); + + it("lands on the feed when Back reaches an entry naming one", () => { + qc.setQueryData(qk.me(), member); + const { result } = render(); + act(() => result.current.setPage("files")); + act(() => { + window.history.replaceState({ sfPage: "config" }, "", "/?page=config"); + window.dispatchEvent(new PopStateEvent("popstate", { state: { sfPage: "config" } })); + }); + expect(result.current.page).toBe("feed"); + }); + + it("leaves the page it is on once the account loses it", async () => { + window.history.replaceState(null, "", "/?page=users"); + const { result } = render(); + expect(result.current.page).toBe("users"); + act(() => { + qc.setQueryData(qk.me(), member); + }); + await settle(); + expect(result.current.page).toBe("feed"); + }); +}); + describe("NavigationProvider · leaving a sub-view whose entry is the oldest of this document", () => { // A full tab can have dropped the entry under the one the channel page or the search pushed (see // lib/history's backStaysInDocument): leaving clears it in place instead of stepping out. diff --git a/frontend/src/components/NavigationProvider.tsx b/frontend/src/components/NavigationProvider.tsx index 8e61101f..6602fdfa 100644 --- a/frontend/src/components/NavigationProvider.tsx +++ b/frontend/src/components/NavigationProvider.tsx @@ -14,6 +14,7 @@ import { notifyNav, rememberNavPosition, } from "../lib/history"; +import { reachablePage } from "../lib/modules"; import { channelViewFrom, loadInitialPage, PAGE_KEY, type ChannelView } from "./navigationInit"; import { NavigationActionsContext, @@ -31,7 +32,12 @@ import { export function NavigationProvider({ children }: { children: ReactNode }) { const { t, i18n } = useTranslation(); - const [page, setPageState] = useState(loadInitialPage); + const [chosenPage, setPageState] = useState(loadInitialPage); + // The page on screen: the chosen one when this account may open it, else the feed — derived, so a + // link, a Back and a role taken away mid-session all land the same way, and the rail, the content + // and the address bar (stamped from it below) agree. + const me = useMe(); + const page = reachablePage(chosenPage, me); // 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 @@ -171,7 +177,7 @@ export function NavigationProvider({ children }: { children: ReactNode }) { // title, a playlist — `PageHeading` reports it), so a channel opened from a link is named the same // as one opened in the app. Signed out, the page on screen is the public landing whatever `page` // says — the tab read "What should I watch?" over the sign-in form (prod UAT 2026-09-19). - const signedIn = useMe() != null; + const signedIn = me != null; const objectTitle = useDocumentObjectTitle(); useEffect(() => { const parts = signedIn ? [objectTitle, t(pageTitleKey(page))] : []; diff --git a/frontend/src/lib/modules.ts b/frontend/src/lib/modules.ts index ae4fb848..2e1d43de 100644 --- a/frontend/src/lib/modules.ts +++ b/frontend/src/lib/modules.ts @@ -183,3 +183,11 @@ export const SYSTEM_PAGES: readonly Page[] = MODULES.filter((m) => m.system).map export function moduleOrder(me: Me): Page[] { return MODULES.filter((m) => !m.gate || m.gate(me)).map((m) => m.id); } + +/** The page this account is shown for `page`: itself when its gate lets the account in, else the + * feed (a link to an admin page, a Back onto one, an admin role taken away mid-session). Signed + * out, no page renders, so nothing is moved. */ +export function reachablePage(page: Page, me: Me | null | undefined): Page { + const gate = moduleDef(page).gate; + return !me || !gate || gate(me) ? page : "feed"; +} diff --git a/scripts/e2e_seed.py b/scripts/e2e_seed.py index c1f6e470..5d545baf 100644 --- a/scripts/e2e_seed.py +++ b/scripts/e2e_seed.py @@ -4,14 +4,15 @@ Run INSIDE the api container — it needs the app's models + the argon2 hasher: docker exec -i siftlode-api-1 python - < scripts/e2e_seed.py -Resets a dedicated test account to a known baseline every run, so the Playwright -golden-master net is deterministic and never touches a real account. Self-healing: a crashed +Resets two dedicated test accounts to a known baseline every run, so the Playwright +golden-master net is deterministic and never touches a real account: the admin the suite runs as, +and a plain member (`member.spec.ts`) that holds what a non-admin sees. Self-healing: a crashed spec that left the account dirty is cleaned on the next run. Prints one JSON line describing what it seeded (the wrapper / global-setup reads it). -Credentials come from env: E2E_EMAIL (default e2e@siftlode.local) and E2E_PASSWORD (REQUIRED, no -default — the ops wrapper exports it from the secret store); the account -lives only in the localdev DB. +Credentials come from env: E2E_EMAIL (default e2e@siftlode.local), E2E_MEMBER_EMAIL (default +e2e-member@siftlode.local) and E2E_PASSWORD (REQUIRED, no default — the ops wrapper exports it from +the secret store; both accounts share it); the accounts live only in the localdev DB. """ import json @@ -31,6 +32,7 @@ from app.reader.models import Playlist, PlaylistItem, VideoState from app.security import hash_password EMAIL = os.environ.get("E2E_EMAIL", "e2e@siftlode.local") +MEMBER_EMAIL = os.environ.get("E2E_MEMBER_EMAIL", "e2e-member@siftlode.local") PASSWORD = os.environ.get("E2E_PASSWORD", "") if not PASSWORD: sys.exit( @@ -42,145 +44,149 @@ N_SUBS = 3 def main() -> None: db = SessionLocal() try: - # 0. Mark the instance configured so the first-run setup wizard is skipped. A prod→dev - # reseed (siftlode seed-dev-from-prod) TRUNCATEs app_state — it's excluded from the - # data allow-list, being local/operational state — which would otherwise drop the box - # back into "Setup needed" and block login/UAT. Since e2e_seed always leaves a loginable - # admin below, the instance is legitimately set up. Idempotent (mirrors state._row + - # state.mark_configured). The running api process caches `configured` one-way from False, - # so it re-reads the DB and promotes on the next request — no restart needed. - app_state = db.get(AppState, 1) - if app_state is None: - app_state = AppState(id=1, sync_paused=False) - db.add(app_state) - app_state.configured = True - app_state.setup_token_hash = None - db.commit() - - # 1. Upsert the account and always reset it to a clean, loginable baseline - # (email_verified + is_active + not suspended + password set). - user = db.scalar(select(User).where(User.email == EMAIL)) - if user is None: - user = User(email=EMAIL) - db.add(user) - user.password_hash = hash_password(PASSWORD) - user.email_verified = True - user.is_active = True - user.is_suspended = False - user.google_sub = None + _mark_configured(db) # Admin so the E2E can reach the admin-only pages (scheduler / config / users / audit) and # thus the render ternary's admin branches — the primary real user (npeter83) is admin too. - user.role = "admin" - user.display_name = "E2E Tester" - user.preferences = {"language": "en"} - db.commit() - - # 2. Subscriptions: the top-N organic channels by catalog size — deterministic, and - # >1 so a channel filter visibly narrows the feed. Reset each run. - top = db.execute( - select(Video.channel_id, func.count().label("n")) - .join(Channel, Channel.id == Video.channel_id) - .where(Channel.from_search.is_(False), Channel.from_explore.is_(False)) - .group_by(Video.channel_id) - .order_by(func.count().desc()) - .limit(N_SUBS) - ).all() - channel_ids = [row[0] for row in top] - - db.execute(delete(Subscription).where(Subscription.user_id == user.id)) - for cid in channel_ids: - db.add( - Subscription( - user_id=user.id, channel_id=cid, subscribed_at=datetime.now(timezone.utc) - ) - ) - db.commit() - - # 3. Deterministic watch state (reset each run): - # - one in-progress video (drives the resume bar + "in progress" filter) from the - # first channel — status stays "new" so it still counts as unwatched; - # - one WATCHED video from the second channel, so the "watched" show-filter is a - # real, non-empty narrowing of the feed (and the "all" vs "unwatched" split exists). - db.execute(delete(VideoState).where(VideoState.user_id == user.id)) - - def _newest(channel_id: str, exclude: set[str]) -> Video | None: - conds = [ - Video.channel_id == channel_id, - Video.duration_seconds.is_not(None), - Video.live_status == "none", - ] - if exclude: - conds.append(Video.id.not_in(exclude)) - return db.scalar( - select(Video).where(*conds).order_by(Video.published_at.desc()).limit(1) - ) - - used: set[str] = set() - in_progress = None - watched = None - if channel_ids: - vid = _newest(channel_ids[0], used) - if vid is not None: - db.add( - VideoState( - user_id=user.id, - video_id=vid.id, - status="new", - position_seconds=42, - progress_updated_at=datetime.now(timezone.utc), - ) - ) - in_progress = vid.id - used.add(vid.id) - if len(channel_ids) > 1: - wv = _newest(channel_ids[1], used) - if wv is not None: - db.add( - VideoState( - user_id=user.id, - video_id=wv.id, - status="watched", - watched_at=datetime.now(timezone.utc), - ) - ) - watched = wv.id - used.add(wv.id) - - # 4. One "saved" video (Watch-later membership drives the saved marker) from the last - # subscribed channel. Reset the built-in playlist's items each run. - wl = db.scalar( - select(Playlist).where(Playlist.user_id == user.id, Playlist.kind == "watch_later") - ) - if wl is None: - wl = Playlist(user_id=user.id, name="Watch later", kind="watch_later") - db.add(wl) - db.commit() - db.execute(delete(PlaylistItem).where(PlaylistItem.playlist_id == wl.id)) - saved = None - if channel_ids: - svid = _newest(channel_ids[-1], used) - if svid is not None: - db.add(PlaylistItem(playlist_id=wl.id, video_id=svid.id, position=0)) - saved = svid.id - db.commit() - + admin = _seed_account(db, EMAIL, "admin", "E2E Tester") + member = _seed_account(db, MEMBER_EMAIL, "user", "E2E Member") # T201: this script's stdout IS its interface — the e2e runner parses this JSON. - print( # noqa: T201 - json.dumps( - { - "ok": True, - "user_id": user.id, - "email": EMAIL, - "channels": channel_ids, - "in_progress": in_progress, - "watched": watched, - "saved": saved, - } - ) - ) + print(json.dumps({"ok": True, **admin, "member": member})) # noqa: T201 finally: db.close() +def _mark_configured(db) -> None: + # 0. Mark the instance configured so the first-run setup wizard is skipped. A prod→dev + # reseed (siftlode seed-dev-from-prod) TRUNCATEs app_state — it's excluded from the + # data allow-list, being local/operational state — which would otherwise drop the box + # back into "Setup needed" and block login/UAT. Since e2e_seed always leaves a loginable + # admin below, the instance is legitimately set up. Idempotent (mirrors state._row + + # state.mark_configured). The running api process caches `configured` one-way from False, + # so it re-reads the DB and promotes on the next request — no restart needed. + app_state = db.get(AppState, 1) + if app_state is None: + app_state = AppState(id=1, sync_paused=False) + db.add(app_state) + app_state.configured = True + app_state.setup_token_hash = None + db.commit() + + +def _seed_account(db, email: str, role: str, display_name: str) -> dict: + # 1. Upsert the account and always reset it to a clean, loginable baseline + # (email_verified + is_active + not suspended + password set). + user = db.scalar(select(User).where(User.email == email)) + if user is None: + user = User(email=email) + db.add(user) + user.password_hash = hash_password(PASSWORD) + user.email_verified = True + user.is_active = True + user.is_suspended = False + user.google_sub = None + user.role = role + user.display_name = display_name + user.preferences = {"language": "en"} + db.commit() + + # 2. Subscriptions: the top-N organic channels by catalog size — deterministic, and + # >1 so a channel filter visibly narrows the feed. Reset each run. + top = db.execute( + select(Video.channel_id, func.count().label("n")) + .join(Channel, Channel.id == Video.channel_id) + .where(Channel.from_search.is_(False), Channel.from_explore.is_(False)) + .group_by(Video.channel_id) + .order_by(func.count().desc()) + .limit(N_SUBS) + ).all() + channel_ids = [row[0] for row in top] + + db.execute(delete(Subscription).where(Subscription.user_id == user.id)) + for cid in channel_ids: + db.add( + Subscription( + user_id=user.id, + channel_id=cid, + subscribed_at=datetime.now(timezone.utc), + ) + ) + db.commit() + + # 3. Deterministic watch state (reset each run): + # - one in-progress video (drives the resume bar + "in progress" filter) from the + # first channel — status stays "new" so it still counts as unwatched; + # - one WATCHED video from the second channel, so the "watched" show-filter is a + # real, non-empty narrowing of the feed (and the "all" vs "unwatched" split exists). + db.execute(delete(VideoState).where(VideoState.user_id == user.id)) + + def _newest(channel_id: str, exclude: set[str]) -> Video | None: + conds = [ + Video.channel_id == channel_id, + Video.duration_seconds.is_not(None), + Video.live_status == "none", + ] + if exclude: + conds.append(Video.id.not_in(exclude)) + return db.scalar(select(Video).where(*conds).order_by(Video.published_at.desc()).limit(1)) + + used: set[str] = set() + in_progress = None + watched = None + if channel_ids: + vid = _newest(channel_ids[0], used) + if vid is not None: + db.add( + VideoState( + user_id=user.id, + video_id=vid.id, + status="new", + position_seconds=42, + progress_updated_at=datetime.now(timezone.utc), + ) + ) + in_progress = vid.id + used.add(vid.id) + if len(channel_ids) > 1: + wv = _newest(channel_ids[1], used) + if wv is not None: + db.add( + VideoState( + user_id=user.id, + video_id=wv.id, + status="watched", + watched_at=datetime.now(timezone.utc), + ) + ) + watched = wv.id + used.add(wv.id) + + # 4. One "saved" video (Watch-later membership drives the saved marker) from the last + # subscribed channel. Reset the built-in playlist's items each run. + wl = db.scalar( + select(Playlist).where(Playlist.user_id == user.id, Playlist.kind == "watch_later") + ) + if wl is None: + wl = Playlist(user_id=user.id, name="Watch later", kind="watch_later") + db.add(wl) + db.commit() + db.execute(delete(PlaylistItem).where(PlaylistItem.playlist_id == wl.id)) + saved = None + if channel_ids: + svid = _newest(channel_ids[-1], used) + if svid is not None: + db.add(PlaylistItem(playlist_id=wl.id, video_id=svid.id, position=0)) + saved = svid.id + db.commit() + + return { + "user_id": user.id, + "email": email, + "channels": channel_ids, + "in_progress": in_progress, + "watched": watched, + "saved": saved, + } + + if __name__ == "__main__": main() From 59ec4a28bae375665b4a98af5362f42602b857e5 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 20 Oct 2026 04:36:26 +0200 Subject: [PATCH 02/11] refactor(auth): one account per browser session The session held a wallet of up to six accounts, and a request picked one by a header or an ?account= parameter. It now holds one: user_id and its epoch. A sign-in replaces whoever was signed in, sign-out and account deletion end the session, and an account refused mid-session (revoked, suspended, deactivated) clears it. /api/me/accounts and /api/me/switch are gone, and with them their refusal codes and the UI copy for them. A cookie the multi-account build wrote carries no single epoch and is refused: its holder signs in once more. A Google link finishing in a browser now signed in as someone else is refused, as before for an account outside the wallet. A test holds that no app code reads the wallet keys or the account header again. --- backend/app/auth.py | 117 ++++------------ backend/app/routes/auth.py | 71 +++------- backend/app/routes/me.py | 83 +----------- backend/app/routes/plex/playback.py | 4 +- backend/tests/test_auth_session.py | 162 ++++++++++++++--------- backend/tests/test_plex_image_sizes.py | 2 +- backend/tests/test_plex_subtitle_auth.py | 2 +- frontend/src/i18n/locales/en/errors.json | 5 - frontend/src/i18n/locales/hu/errors.json | 5 - 9 files changed, 151 insertions(+), 300 deletions(-) diff --git a/backend/app/auth.py b/backend/app/auth.py index c908f924..bf765b07 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -25,8 +25,6 @@ def operator_contact() -> str | None: return next(iter(sorted(settings.admin_email_set)), None) -MAX_SESSION_ACCOUNTS = 6 - # OAuth scope vocabulary — the atoms + the request bundles, kept together as core. has_*_scope check # the atoms against a user's stored grant; the *_SCOPES bundles are what the login/callback endpoints # request (imported by app/routes/auth.py). @@ -253,94 +251,37 @@ def purge_user(db: Session, user: User, background: BackgroundTasks) -> None: background.add_task(email_mod.send_account_deleted, email, lang, operator_contact()) -ACTIVE_ACCOUNT_HEADER = "x-siftlode-account" +def session_user_id(conn: HTTPConnection) -> int | None: + """The account this browser session signed in as, read from the signed cookie alone (no DB). + One account per session: signing in as another replaces it (`routes.auth._establish_session`).""" + return conn.session.get("user_id") -def resolved_user_id(conn: HTTPConnection, *, strict: bool = True) -> tuple[int | None, bool]: - """Which account this connection acts as, and whether that's the session's default account. - Takes an HTTPConnection rather than a Request: it reads only the session and the headers. - - The signed cookie holds the *wallet* (`account_ids`, every account signed into this browser) - plus a default `user_id`. A caller may name its account with the X-Siftlode-Account header (or - `?account=`), but ONLY an account in the wallet — so a tab can't impersonate an account that - never authenticated here. A connection that names no account acts as the cookie default. - - A connection naming an account NOT in the wallet acts as nobody (`None`) — typically a tab still - pinned to an account that was signed out in another tab. It must not fall back to the default: - that tab would go on acting as an account it never showed, its player checkpoints and pref saves - landing there and its Sign out signing THAT account out. Only the bootstrap /api/me probe passes - `strict=False` and gets the default, which is how such a tab learns who it now is (the frontend - re-pins it and reloads). An account whose session ended while it stayed in the wallet (deleted, - suspended, revoked) leaves the wallet when it is refused (`_session_user`), so a tab pinned to it - takes the same way back. - """ - default_id = conn.session.get("user_id") - wallet = conn.session.get("account_ids") or [] - # Header for normal XHR; ?account= for a context that can't set headers (a plain file - # download). Both are wallet-gated below. - hdr = conn.headers.get(ACTIVE_ACCOUNT_HEADER) or conn.query_params.get("account") - if hdr: - try: - hid = int(hdr) - except ValueError: - # Under `strict` this becomes a 401 and the tab authenticates as nobody, which reads - # in the browser exactly like a session that expired. The value itself stays out — - # it came from a request, and `wallet` says which accounts were actually on offer. - log.debug( - "the active-account header could not be read as an id", - extra=logs.event("auth.account_header.invalid", accounts=len(wallet)), - ) - hid = None - if hid is not None and hid in wallet: - return hid, hid == default_id - if strict: - return None, False - return default_id, True - - -def drop_from_wallet(session: dict, user_id: int) -> None: - """Take one account out of this browser's wallet (`account_ids`); the session default is untouched.""" - session["account_ids"] = [a for a in (session.get("account_ids") or []) if a != user_id] - - -def _end_account_session(request: Request, user_id: int, is_default: bool) -> None: - """An account whose session here is no longer valid. Only nuke the whole session when it is the - SESSION DEFAULT; a per-tab account just leaves the wallet (other tabs' default may still be valid). - Leaving is what lets a tab pinned to it recover: its next /api/me names an account no longer signed - in here and gets the default. Left in, it answered "signed out" — the landing page, with the default - still signed in, and no way off it but closing the tab.""" - if is_default: - request.session.clear() - else: - drop_from_wallet(request.session, user_id) +def stamp_session(request: Request, user: User) -> None: + """Record the account's current `session_epoch` in the signed cookie, so `current_user` can later + reject the cookie once the epoch moves (password reset/change, or "log out everywhere").""" + request.session["epoch"] = user.session_epoch or 0 def current_user(request: Request, db: Session = Depends(get_db)) -> User: - return _session_user(request, db, *resolved_user_id(request)) + return _session_user(request, db, session_user_id(request)) -def _session_user(request: Request, db: Session, user_id: int | None, is_default: bool) -> User: +def _session_user(request: Request, db: Session, user_id: int | None) -> User: if not user_id: raise refused(401, "not_authenticated") user = db.get(User, user_id) 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. - _end_account_session(request, user_id, is_default) + request.session.clear() raise refused(401, "not_authenticated") - # SA4: reject a cookie whose recorded epoch is behind the account's current one (revoked by a - # password reset/change or "log out everywhere"). A cookie with no recorded epoch (pre-SA4, or an - # account signed in before this shipped) is treated as epoch 0 — so the FIRST bump revokes it too. - epochs = request.session.get("epochs") or {} - if int(epochs.get(str(user_id), 0)) != (user.session_epoch or 0): - _end_account_session(request, user_id, is_default) + # 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") - # Always keep the session default in the switchable list — covers sessions created before - # multi-session existed (their account_ids was never seeded), so the switcher isn't empty. - default_id = request.session.get("user_id") - accounts = request.session.get("account_ids") or [] - if default_id and default_id not in accounts: - accounts.append(default_id) - request.session["account_ids"] = accounts[-MAX_SESSION_ACCOUNTS:] # 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 # session middleware and this task's context. `request.state` is shared across middleware layers @@ -358,25 +299,17 @@ def _session_user(request: Request, db: Session, user_id: int | None, is_default def optional_current_user(request: Request, db: Session = Depends(get_db)) -> User | None: """Like `current_user` but returns None instead of raising 401 when there's no valid session. Used by the bootstrap /api/me probe so the logged-out landing doesn't log a failed request to - the browser console (a non-2xx fetch is a console error no matter how the app handles it). - The one lenient resolution (`strict=False`): a tab naming an account no longer signed in here gets - the default account, which is how it learns who it now is — see `resolved_user_id`. That includes an - account refused right here, which `_session_user` has just taken out of the wallet.""" - user_id, is_default = resolved_user_id(request, strict=False) + the browser console (a non-2xx fetch is a console error no matter how the app handles it).""" + user_id = session_user_id(request) + if user_id is None: + return None try: - return _session_user(request, db, user_id, is_default) - # silent-ok: this is the anonymous case as much as the refused one — every landing page hit - # comes through here. The second attempt below is the handling, and it reports. + return _session_user(request, db, user_id) except HTTPException: - if is_default: - return None - try: - return _session_user(request, db, *resolved_user_id(request, strict=False)) - except HTTPException: - # Both the pinned account AND the default one were refused, so the tab is signed out - # without being told why — and it got here from a session that named a real account. + # A session that named an account, refused: the browser is signed out without being told + # why, so say it here. log.info( - "a session was resolved to nobody: neither the named nor the default account held", + "a session was refused: its account no longer holds", extra=logs.event("auth.session.probe_rejected", user_id=user_id), ) return None diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 336d665e..ff659b09 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -25,7 +25,6 @@ from app import sysconfig from app.auth import ( _YOUTUBE_SCOPES, BASE_SCOPES, - MAX_SESSION_ACCOUNTS, PASSWORD_MIN_LEN, READ_SCOPES, WRITE_SCOPES, @@ -37,7 +36,8 @@ from app.auth import ( is_allowed, log, operator_contact, - resolved_user_id, + session_user_id, + stamp_session, ) from app.config import settings from app.db import SessionLocal, get_db @@ -361,13 +361,13 @@ async def callback( token = await client.authorize_access_token(request) except OAuthError as exc: # The user cancelled or denied consent at Google — land back with a friendly notice instead - # of a raw 400 JSON error page. An already-signed-in user (adding another account via - # /auth/login, or linking/upgrading via /auth/link|upgrade) 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 the add-account path (no marker) is covered too. + # of a raw 400 JSON error page. An already-signed-in user (signing in again via /auth/login, + # or linking/upgrading via /auth/link|upgrade) 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. log.info("Google OAuth cancelled/failed: %s", exc.error) - if request.session.get("user_id") is not None: + if session_user_id(request) is not None: return RedirectResponse(url="/?oauth=cancelled") return RedirectResponse(url="/?login=oauth_cancelled") @@ -584,12 +584,9 @@ 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 upgrade — the caller pops it up front so an aborted flow leaves nothing behind.""" - # SEC-11: the marker must name an account signed into THIS browser — the session default OR any - # account in the wallet — not only the default. A tab acting as a secondary account links Google to - # ITSELF (the marker was set from current_user, which resolves the ?account= it navigated with), so - # accepting wallet membership stops the identity/token landing on the default account A instead of B. - accounts = request.session.get("account_ids") or [] - if link_uid != request.session.get("user_id") and link_uid not in accounts: + # 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: @@ -619,38 +616,17 @@ def _complete_link( @router.post("/logout") async def logout(request: Request): - """Sign the requesting tab's active account out of this browser (removing it from the wallet). - Per-tab aware: the account is taken from the X-Siftlode-Account header when present, so logging - out in one tab doesn't disturb another tab running a different account. If the account being - removed was the session default, promote the most recent remaining account (or clear the - session when none remain).""" - # A tab still pinned to an account already signed out in another tab resolves to nobody, so it - # signs nobody out — it used to fall back to the default and sign THAT account out in its place. - active, is_default = resolved_user_id(request) - remaining = [a for a in (request.session.get("account_ids") or []) if a != active] - request.session["account_ids"] = remaining - if is_default: - if remaining: - request.session["user_id"] = remaining[-1] - return JSONResponse({"ok": True, "switched": True}) - request.session.clear() - return JSONResponse({"ok": True, "switched": False}) - # A non-default (per-tab) account signed out: it's gone from the wallet and the session default - # is untouched. The tab drops its own override and falls back to the default on reload. - return JSONResponse({"ok": True, "switched": False}) + """Sign this browser out: the session holds one account, and it ends.""" + request.session.clear() + return JSONResponse({"ok": True}) def _establish_session(request: Request, user: User) -> None: """Sign `user` in for this browser session — the ONE sign-in path, shared by the Google callback - and password login, so both join the same account switcher and both stamp `last_login_at` (the - caller commits). Multi-session: every account that authenticated in this browser is remembered so - the user can switch between them; only accounts that actually proved themselves HERE land in that - wallet (it is the proof they may be switched to).""" - accounts = [a for a in (request.session.get("account_ids") or []) if a != user.id] - accounts.append(user.id) - request.session["account_ids"] = accounts[-MAX_SESSION_ACCOUNTS:] + and password login, so both stamp `last_login_at` (the caller commits). One account per session: + the sign-in replaces whoever was signed in here before.""" request.session["user_id"] = user.id - _remember_epoch(request, user) + stamp_session(request, user) user.last_login_at = datetime.now(timezone.utc) @@ -1135,15 +1111,6 @@ def password_reset_confirm(body: TokenPasswordIn, db: Session = Depends(get_db)) return {"ok": True} -def _remember_epoch(request: Request, user: User) -> None: - """Record this account's current `session_epoch` in the signed cookie (SA4), so `current_user` - can later reject the cookie if the account's epoch was bumped (password reset/change, or - "log out everywhere"). Per-account so one account's revocation doesn't evict the others.""" - epochs = dict(request.session.get("epochs") or {}) - epochs[str(user.id)] = user.session_epoch - request.session["epochs"] = epochs - - def bump_session_epoch(db: Session, user: User) -> None: """Invalidate every EXISTING signed cookie for this account by advancing its epoch (SA4). Cookies minted before the bump carry a now-stale epoch and are rejected by `current_user`. The account's @@ -1220,7 +1187,7 @@ def logout_others( # session at a newer epoch than the DB (see set_password). bump_session_epoch(db, user) db.commit() - _remember_epoch(request, user) + stamp_session(request, user) log.info("Logout-others: uid=%s (epoch=%s)", user.id, user.session_epoch) return {"ok": True} @@ -1272,7 +1239,7 @@ def set_password( # can't leave the cookie at a newer epoch than the DB (which would wrongly 401 THIS session). bump_session_epoch(db, user) db.commit() - _remember_epoch(request, user) + stamp_session(request, user) log.info("Password set/changed: uid=%s", user.id) return {"ok": True} diff --git a/backend/app/routes/me.py b/backend/app/routes/me.py index e6b79e35..d3a3d5cd 100644 --- a/backend/app/routes/me.py +++ b/backend/app/routes/me.py @@ -9,11 +9,9 @@ from app.auth import ( count_admins, count_pending_activations, current_user, - drop_from_wallet, google_enabled, has_read_scope, has_write_scope, - is_allowed, optional_current_user, purge_user, ) @@ -26,79 +24,10 @@ from app.db import get_db from app.models import Invite, User from app.utils import iso from app.refusal import refused -from app.routes._common import not_found router = APIRouter(prefix="/api/me", tags=["me"]) -@router.get("/accounts") -def my_accounts( - request: Request, - user: User = Depends(current_user), - db: Session = Depends(get_db), -) -> list[dict]: - """The accounts that have authenticated in this browser session (switchable without a - new Google sign-in). The active one is flagged.""" - ids = request.session.get("account_ids") or [user.id] - rows = {u.id: u for u in db.execute(select(User).where(User.id.in_(ids))).scalars()} - out = [] - for uid in ids: # preserve recency order from the session - u = rows.get(uid) - if u is not None: - out.append( - { - "id": u.id, - "email": u.email, - "display_name": u.display_name, - "avatar_url": u.avatar_url, - "active": u.id == user.id, - } - ) - return out - - -class SwitchAccountIn(BaseModel): - user_id: int - - -@router.post("/switch") -def switch_account( - body: SwitchAccountIn, - request: Request, - user: User = Depends(current_user), - db: Session = Depends(get_db), -) -> dict: - """Switch the active account to another one already authenticated in this browser. No - Google round-trip — but only accounts present in the session list (proof they signed in - here) are allowed, and access is re-checked in case the invite was revoked since.""" - target = body.user_id - if target not in (request.session.get("account_ids") or []): - raise refused(403, "not_your_account") - # An account `_session_user` would refuse — gone, deactivated, suspended, or its session revoked — is - # refused here too, and dropped from the switch list the way `_session_user` drops it. Made the session - # default instead, the next request's current_user would refuse it as the DEFAULT and clear the whole - # wallet session, logging out every account here. (B-core-9) - u = db.get(User, target) - if u is None: - drop_from_wallet(request.session, target) - raise not_found("account") - if not is_allowed(db, u.email): - raise refused(403, "account_no_access") - if not u.is_active: - drop_from_wallet(request.session, target) - raise refused(403, "account_no_access") - if u.is_suspended: - drop_from_wallet(request.session, target) - raise refused(403, "account_suspended") - # Revoked: a password reset/change or "log out everywhere" bumped the account's epoch. - epochs = request.session.get("epochs") or {} - if int(epochs.get(str(target), 0)) != (u.session_epoch or 0): - drop_from_wallet(request.session, target) - raise refused(403, "account_signed_out") - request.session["user_id"] = target - return {"ok": True, "user_id": target} - - @router.get("") def get_me( user: User | None = Depends(optional_current_user), db: Session = Depends(get_db) @@ -187,17 +116,11 @@ def delete_my_account( surfaces).""" if user.role == "admin" and count_admins(db) <= 1: raise refused(400, "only_admin_delete") - user_id = user.id # Full erasure (cascades) + access-request cleanup + Google-grant revocation; shared with the - # admin delete path. Capture the id first — `user` is detached once purge_user deletes the row. + # admin delete path. purge_user(db, user, background) - # Drop this account from the browser session; switch to another signed-in account if any. - remaining = [a for a in (request.session.get("account_ids") or []) if a != user_id] - if remaining: - request.session["account_ids"] = remaining - request.session["user_id"] = remaining[-1] - else: - request.session.clear() + # The session's one account is gone: so is the session. + request.session.clear() return {"deleted": True} diff --git a/backend/app/routes/plex/playback.py b/backend/app/routes/plex/playback.py index d8cea012..77b7c231 100644 --- a/backend/app/routes/plex/playback.py +++ b/backend/app/routes/plex/playback.py @@ -19,7 +19,7 @@ from fastapi.responses import FileResponse from pydantic import BaseModel from sqlalchemy.orm import Session -from app.auth import current_user, resolved_user_id +from app.auth import current_user, session_user_id from app.db import SessionLocal, get_db from app.models import User from app.plex import imgcache, queries, service, subtitles @@ -42,7 +42,7 @@ def _require_session(request: Request) -> int: """Lightweight auth for the high-fan-out image proxies: validate the signed session cookie WITHOUT a DB user-load, so these endpoints never hold a pool connection for the auth. Poster images are low-sensitivity; a valid session is enough.""" - uid, _ = resolved_user_id(request) + uid = session_user_id(request) if not uid: raise refused(401, "not_authenticated") return uid diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index f1ec9bd2..611f22fc 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -1,21 +1,25 @@ """Auth / session security core (TEST-1), DB-lane. -The wallet impersonation gate, epoch-based session revocation, single-use tokens and the -enumeration-safe uniform 401 are the security mag, and had zero tests (only `_oauth_redirect_uri` was -covered). Starlette's SessionMiddleware round-trips the signed cookie under TestClient, so the whole -browser-less flow is exercisable. Uses the real auth stack via the `client` fixture (NO current_user -override) so login, the wallet gate and epoch revoke are the code under test. +One account per browser session, epoch-based session revocation, single-use tokens and the +enumeration-safe uniform 401 are the security mag. Starlette's SessionMiddleware round-trips the +signed cookie under TestClient, so the whole browser-less flow is exercisable. Uses the real auth +stack via the `client` fixture (NO current_user override) so login, the session and epoch revoke are +the code under test. Note: `/api/me` is the bootstrap probe — it returns 200 `{authenticated: False}` when logged out, not a 401. A revoked session is therefore checked on a PROTECTED endpoint (`/api/feed`), which does 401. """ +import base64 +import json from datetime import timedelta +from pathlib import Path import pytest from fastapi.testclient import TestClient +from itsdangerous import TimestampSigner -from app.auth import ACTIVE_ACCOUNT_HEADER +from app.config import settings from app.main import app _PW = "correct horse battery" @@ -46,74 +50,48 @@ def test_wrong_password_and_unknown_email_give_a_byte_identical_401(client, make assert wrong_pw.json() == unknown.json() -def test_a_non_wallet_account_header_falls_back_to_the_default(client, make_user): - # The X-Siftlode-Account header may only re-target an account already signed into THIS browser - # (in the session wallet). A header naming someone else must be ignored, not honored — otherwise - # it's an impersonation gate hole. - make_user(email="alice2@example.com", password=_PW) - mallory = make_user(email="mallory@example.com", password=_PW) - assert _login(client, "alice2@example.com", _PW).status_code == 200 - me = client.get("/api/me", headers={ACTIVE_ACCOUNT_HEADER: str(mallory.id)}) - assert me.status_code == 200 - assert me.json()["email"] == "alice2@example.com" # still Alice, not Mallory +def _cookie(payload: dict) -> str: + """A session cookie signed the way SessionMiddleware signs one — for a cookie an older build wrote.""" + data = base64.b64encode(json.dumps(payload).encode()) + return TimestampSigner(str(settings.secret_key)).sign(data).decode() -def test_a_tab_pinned_to_an_account_signed_out_elsewhere_acts_as_nobody(client, make_user): - # Two accounts in one browser: a tab pinned to Alice, Bob the default. Alice signs out in another - # tab. The pinned tab used to go on acting as the default, Bob — its player checkpoints and pref - # saves landed in his account, and its own Sign out signed HIM out. - alice = make_user(email="alice3@example.com", password=_PW) +def test_signing_in_as_another_account_replaces_the_first(client, make_user): + # One account per browser session: a second sign-in is a fresh session, not a second account + # beside the first — so signing out leaves nobody signed in here. + make_user(email="alice3@example.com", password=_PW) make_user(email="bob3@example.com", password=_PW) assert _login(client, "alice3@example.com", _PW).status_code == 200 - assert _login(client, "bob3@example.com", _PW).status_code == 200 # Bob is now the default - tab = {ACTIVE_ACCOUNT_HEADER: str(alice.id)} - assert client.get("/api/feed", headers=tab).status_code == 200 - assert client.post("/auth/logout", headers=tab).status_code == 200 # Alice signs out + assert _login(client, "bob3@example.com", _PW).status_code == 200 + assert client.get("/api/me").json()["email"] == "bob3@example.com" - assert client.get("/api/feed", headers=tab).status_code == 401 - assert client.post("/auth/logout", headers=tab).status_code == 200 - assert client.get("/api/feed").status_code == 200 # Bob is still signed in, the session intact - # The bootstrap probe still answers as the default: how the stale tab learns who it is now. - assert client.get("/api/me", headers=tab).json()["email"] == "bob3@example.com" + assert client.post("/auth/logout").status_code == 200 + assert client.get("/api/feed").status_code == 401 + assert client.get("/api/me").json() == {"authenticated": False} -@pytest.mark.parametrize("ended", ["revoked", "suspended"]) -def test_a_tab_pinned_to_an_account_whose_session_ended_comes_back_as_the_default( - client, make_user, db, ended -): - # A tab pinned to Alice, Bob the default. Alice's session then ends while she is still in this - # browser's wallet ("log out everywhere" on another device, or an admin suspends her). The tab's - # bootstrap probe used to answer "signed out": the landing page with Bob still signed in, and no way - # off it but closing the tab (signing in as anyone else there kept the tab pinned to Alice). +def test_no_header_or_parameter_picks_another_account(client, make_user): + # Alice signed in here before Bob: nothing a request carries makes it act as her any more. alice = make_user(email="alice4@example.com", password=_PW) make_user(email="bob4@example.com", password=_PW) assert _login(client, "alice4@example.com", _PW).status_code == 200 assert _login(client, "bob4@example.com", _PW).status_code == 200 - tab = {ACTIVE_ACCOUNT_HEADER: str(alice.id)} - assert client.get("/api/feed", headers=tab).status_code == 200 - if ended == "revoked": - alice.session_epoch = (alice.session_epoch or 0) + 1 - else: - alice.is_suspended = True - db.commit() + me = client.get(f"/api/me?account={alice.id}", headers={"X-Siftlode-Account": str(alice.id)}) + assert me.json()["email"] == "bob4@example.com" - assert client.get("/api/me", headers=tab).json()["email"] == "bob4@example.com" - assert client.get("/api/feed", headers=tab).status_code == 401 # still nobody, never Bob - assert client.get("/api/feed").status_code == 200 # Bob's session is intact + +@pytest.mark.parametrize("path", ["/api/me/accounts", "/api/me/switch"]) +def test_there_is_no_account_list_or_switch(client, make_user, path): + make_user(email="alice5@example.com", password=_PW) + assert _login(client, "alice5@example.com", _PW).status_code == 200 + r = client.get(path) if path.endswith("accounts") else client.post(path, json={"user_id": 1}) + assert r.status_code in (404, 405) @pytest.mark.parametrize("ended", ["revoked", "suspended", "deactivated"]) -def test_switching_to_an_account_whose_session_ended_refuses_it_and_drops_it_from_the_list( - client, make_user, db, monkeypatch, ended -): - # The account switcher lists this browser's wallet. A revoked account was dropped from it on a refused - # switch, but a suspended one stayed, refused on every click; a deactivated one was not refused at all — - # made the default, the next request found it inactive and signed every account here out. - monkeypatch.setattr("app.routes.me.is_allowed", lambda _db, _email: True) # both hold an invite - alice = make_user(email="alice5@example.com", password=_PW) - 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 +def test_a_session_whose_account_ended_signs_the_browser_out(client, make_user, db, ended): + alice = make_user(email="alice6@example.com", password=_PW) + assert _login(client, "alice6@example.com", _PW).status_code == 200 if ended == "revoked": alice.session_epoch = (alice.session_epoch or 0) + 1 elif ended == "suspended": @@ -121,10 +99,70 @@ def test_switching_to_an_account_whose_session_ended_refuses_it_and_drops_it_fro else: alice.is_active = False db.commit() + assert client.get("/api/feed").status_code == 401 + assert "user_id" not in _session(client) # the refusal ended the session, not just this request + assert client.get("/api/me").json() == {"authenticated": False} - assert client.post("/api/me/switch", json={"user_id": alice.id}).status_code == 403 - assert [a["email"] for a in client.get("/api/me/accounts").json()] == ["bob5@example.com"] - assert client.get("/api/feed").status_code == 200 # Bob is still signed in + +def test_a_cookie_from_the_multi_account_build_signs_in_again(client, make_user): + # The old session held a wallet and one epoch per account. It is not read: whoever holds one + # signs in once more. + alice = make_user(email="alice7@example.com", password=_PW) + old = {"user_id": alice.id, "account_ids": [alice.id], "epochs": {str(alice.id): 0}} + client.cookies.set("session", _cookie(old)) + assert client.get("/api/feed").status_code == 401 + assert client.get("/api/me").json() == {"authenticated": False} + + +def test_deleting_the_account_signs_the_browser_out(client, make_user): + make_user(email="alice8@example.com", password=_PW) + assert _login(client, "alice8@example.com", _PW).status_code == 200 + assert client.delete("/api/me/account").status_code == 200 + # The cookie names nobody now: the probe answers signed out without looking the account up. + assert "user_id" not in _session(client) + + +def _session(client) -> dict: + """The decoded payload of the client's session cookie.""" + raw = client.cookies.get("session") + if not raw: + return {} + data = TimestampSigner(str(settings.secret_key)).unsign(raw.encode()) + return json.loads(base64.b64decode(data)) + + +@pytest.mark.parametrize("signed_in_as", ["starter", "someone_else"]) +def test_a_google_link_lands_only_on_the_account_still_signed_in(db, make_user, signed_in_as): + # The link marker names who started it; a sign-in as someone else before the callback must not + # get that Google identity attached to the account that started it from the other person's session. + from starlette.requests import Request + + 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} + request = Request({"type": "http", "session": session, "headers": []}) + resp = _complete_link(request, db, starter.id, True, "sub-123", {"email": ""}, {"scope": ""}) + db.refresh(starter) + if signed_in_as == "starter": + assert resp.headers["location"] == "/?link=ok" + assert starter.google_sub == "sub-123" + else: + assert resp.headers["location"] == "/?link=error" + assert starter.google_sub is None + + +def test_no_app_code_reads_a_second_account(): + # The multi-account machinery is gone; a module reading its session keys or its header again + # would bring back a session that names more than one account. + app_dir = Path(__file__).parent.parent / "app" + hits = [ + f"{path.relative_to(app_dir)}: {word}" + for path in app_dir.rglob("*.py") + for word in ("account_ids", '"epochs"', "x-siftlode-account", 'query_params.get("account")') + if word in path.read_text(encoding="utf-8").lower() + ] + assert hits == [] def test_logout_others_revokes_the_other_session_but_keeps_the_caller(client, make_user): diff --git a/backend/tests/test_plex_image_sizes.py b/backend/tests/test_plex_image_sizes.py index de996f5a..0f617bb1 100644 --- a/backend/tests/test_plex_image_sizes.py +++ b/backend/tests/test_plex_image_sizes.py @@ -48,7 +48,7 @@ def _setup(db, make_user, monkeypatch, tmp_path) -> TestClient: ) db.commit() monkeypatch.setattr(app_state, "_configured_cache", True) - monkeypatch.setattr(playback, "resolved_user_id", lambda request: (user.id, True)) + monkeypatch.setattr(playback, "session_user_id", lambda request: user.id) monkeypatch.setattr(playback, "PlexClient", _FakePlex) monkeypatch.setattr(imgcache, "CACHE_DIR", tmp_path) _FakePlex.calls = [] diff --git a/backend/tests/test_plex_subtitle_auth.py b/backend/tests/test_plex_subtitle_auth.py index c9beb4f8..906ba125 100644 --- a/backend/tests/test_plex_subtitle_auth.py +++ b/backend/tests/test_plex_subtitle_auth.py @@ -32,7 +32,7 @@ def test_a_suspended_account_cannot_drive_the_subtitle_extractor( monkeypatch.setattr(app_state, "_configured_cache", True) # A validly SIGNED cookie for that account — which is all the old gate looked at. for mod in (auth, playback): - monkeypatch.setattr(mod, "resolved_user_id", lambda request: (user.id, True)) + monkeypatch.setattr(mod, "session_user_id", lambda request: user.id) monkeypatch.setattr(subtitles, "CACHE_DIR", tmp_path) pool = _NoPool() monkeypatch.setattr(subtitles, "_WIN_POOL", pool) diff --git a/frontend/src/i18n/locales/en/errors.json b/frontend/src/i18n/locales/en/errors.json index be730977..cb21200e 100644 --- a/frontend/src/i18n/locales/en/errors.json +++ b/frontend/src/i18n/locales/en/errors.json @@ -24,7 +24,6 @@ "screenshot": "This screenshot doesn't exist any more", "link": "This share link doesn't exist any more", "notification": "This notification doesn't exist any more", - "account": "That account no longer exists", "collection": "This collection doesn't exist any more", "config_key": "There's no such setting", "file": "The file is no longer available", @@ -135,9 +134,6 @@ "gone": "This playlist no longer exists on YouTube" }, "refused": { - "account_no_access": "That account no longer has access", - "account_signed_out": "That account was signed out elsewhere — sign in again", - "account_suspended": "That account is suspended", "address_has_account": "That address already has an account — delete the account instead", "admin_first": "Create the admin account first", "admin_only": "Only an admin can do that", @@ -174,7 +170,6 @@ "not_authenticated": "You're not signed in", "not_ready": "Not available yet", "not_redownloadable": "This download can't be downloaded again", - "not_your_account": "That's not an account you've signed into here", "only_admin_delete": "You're the only admin — make someone else admin before deleting your account", "own_delete": "Delete your own account in Settings → Account", "own_role": "You can't change your own role", diff --git a/frontend/src/i18n/locales/hu/errors.json b/frontend/src/i18n/locales/hu/errors.json index 3c70c9c6..5e024fe3 100644 --- a/frontend/src/i18n/locales/hu/errors.json +++ b/frontend/src/i18n/locales/hu/errors.json @@ -24,7 +24,6 @@ "screenshot": "Ez a képernyőkép már nem létezik", "link": "Ez a megosztási link már nem létezik", "notification": "Ez az értesítés már nem létezik", - "account": "Ez a fiók már nem létezik", "collection": "Ez a kollekció már nem létezik", "config_key": "Nincs ilyen beállítás", "file": "A fájl már nem érhető el", @@ -135,9 +134,6 @@ "gone": "Ez a lejátszási lista már nem létezik a YouTube-on" }, "refused": { - "account_no_access": "Ennek a fióknak már nincs hozzáférése", - "account_signed_out": "Ezt a fiókot máshol kijelentkeztették — lépj be újra", - "account_suspended": "Ez a fiók fel van függesztve", "address_has_account": "Ehhez a címhez már tartozik fiók — azt töröld helyette", "admin_first": "Előbb hozd létre az admin fiókot", "admin_only": "Ezt csak admin teheti meg", @@ -174,7 +170,6 @@ "not_authenticated": "Nem vagy bejelentkezve", "not_ready": "Még nem érhető el", "not_redownloadable": "Ez a letöltés nem tölthető le újra", - "not_your_account": "Ebbe a fiókba itt nem léptél be", "only_admin_delete": "Te vagy az egyetlen admin — a fiókod törlése előtt tegyél meg valaki mást adminnak", "own_delete": "A saját fiókodat a Beállítások → Fiók alatt törölheted", "own_role": "A saját szerepkörödet nem módosíthatod", From 6645caa157bdbb539882680c09c540656bb8e81c Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 20 Oct 2026 12:49:11 +0200 Subject: [PATCH 03/11] refactor(auth): one Google connect route for a sign-in and for YouTube access /auth/link and /auth/upgrade become /auth/google/connect?access=identity|read|write: one handler, one table of what each access asks Google for, the same callback markers. An unknown access is refused (422) instead of quietly meaning read, and an instance without Google answers ?link=error for a grant too, not a silent return to the app. The frontend builds the URL in lib/googleConnect, and a contract test holds it to the backend route and its access values. --- backend/app/routes/auth.py | 74 +++++++--------- backend/tests/test_google_callback.py | 85 ++++++++++++++++++- frontend/src/components/SettingsPanel.tsx | 20 ++--- .../src/lib/googleConnect.contract.test.ts | 21 +++++ frontend/src/lib/googleConnect.ts | 16 ++++ frontend/src/lib/onboarding.ts | 4 +- 6 files changed, 156 insertions(+), 64 deletions(-) create mode 100644 frontend/src/lib/googleConnect.contract.test.ts create mode 100644 frontend/src/lib/googleConnect.ts diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index ff659b09..eff76ea6 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -5,7 +5,7 @@ helpers) stays in app/auth.py, imported below.""" import secrets from datetime import datetime, timedelta, timezone -from typing import NamedTuple +from typing import Literal, NamedTuple from urllib.parse import urlencode from authlib.integrations.starlette_client import OAuth, OAuthError @@ -350,8 +350,8 @@ 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/link and - # /auth/upgrade; when present we attach to the current user instead of identifying by sub. + # 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. Popping up front means an aborted link flow leaves no residue in the session. @@ -362,7 +362,7 @@ async def callback( except OAuthError as exc: # The user cancelled or denied consent at Google — land back with a friendly notice instead # of a raw 400 JSON error page. An already-signed-in user (signing in again via /auth/login, - # or linking/upgrading via /auth/link|upgrade) never sees the pre-auth login banner, so + # 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. @@ -580,10 +580,10 @@ def _complete_link( token: dict, ): """Attach the just-authorized Google identity to the already-signed-in account that started - the link/upgrade. No is_allowed gate — they're already an active user. Refuses to hijack a + 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 upgrade — the caller pops it up front so an aborted flow leaves nothing behind.""" + 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): @@ -609,7 +609,7 @@ def _complete_link( _store_token(db, user, token) db.commit() log.info("Google linked: uid=%s scopes=%s", user.id, token.get("scope")) - # An explicit "Connect Google" lands on Settings with a confirmation; a YouTube upgrade + # An explicit "Connect Google" lands on Settings with a confirmation; a YouTube grant # (wizard / Settings access rows) returns to the app so the onboarding flow resumes as before. return RedirectResponse(url="/?link=ok" if explicit else "/") @@ -1192,30 +1192,6 @@ def logout_others( return {"ok": True} -@router.get("/link") -async def link_google( - request: Request, - user: User = Depends(current_user), - db: Session = Depends(get_db), -): - """Start linking a Google account to the signed-in (e.g. password) account. Requests only - the identity scopes — YouTube access is granted separately via /auth/upgrade. The callback - sees `oauth_link_uid` and attaches the identity instead of creating a new account.""" - client = google_oauth(db) - if client is None: - return RedirectResponse(url="/?link=error") - request.session["oauth_link_uid"] = user.id - request.session["oauth_link_explicit"] = True - return await client.authorize_redirect( - request, - _oauth_redirect_uri(request), - access_type="offline", - prompt="select_account", - include_granted_scopes="true", - scope=BASE_SCOPES, - ) - - @router.post("/set-password") def set_password( body: SetPasswordIn, @@ -1244,30 +1220,40 @@ def set_password( return {"ok": True} -@router.get("/upgrade") -async def upgrade( +# What each Google connect asks Google for. `identity` attaches a Google sign-in to the account +# (Settings → Connect Google) and lets the person pick which Google account; `read` and `write` grant +# YouTube access (the onboarding wizard, the Settings access rows) and always show the consent screen, +# so a scope granted earlier and since revoked at Google is asked for again. +_CONNECT: dict[str, tuple[str, str]] = { + "identity": (BASE_SCOPES, "select_account"), + "read": (READ_SCOPES, "consent"), + "write": (WRITE_SCOPES, "consent"), +} + + +@router.get("/google/connect") +async def google_connect( request: Request, - access: str = "read", + access: Literal["identity", "read", "write"], user: User = Depends(current_user), db: Session = Depends(get_db), ): - """Incremental consent for the onboarding wizard. `access=read` grants YouTube - read-only (enough to build the feed); `access=write` additionally grants management - (unsubscribe). The shared callback stores whatever Google returns, so `can_read` / - `can_write` flip on once granted. Anything other than an explicit `write` defaults to - the least-privileged read scope.""" + """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.""" client = google_oauth(db) if client is None: - return RedirectResponse(url="/") - # Attach the grant to THIS account in the callback. Without it a password account (no - # google_sub) would get a brand-new, separate Google user created instead of being linked. + return RedirectResponse(url="/?link=error") + scope, prompt = _CONNECT[access] request.session["oauth_link_uid"] = user.id - scope = WRITE_SCOPES if access == "write" else READ_SCOPES + request.session["oauth_link_explicit"] = access == "identity" return await client.authorize_redirect( request, _oauth_redirect_uri(request), access_type="offline", - prompt="consent", + prompt=prompt, include_granted_scopes="true", scope=scope, ) diff --git a/backend/tests/test_google_callback.py b/backend/tests/test_google_callback.py index bb863a5e..75cce7e8 100644 --- a/backend/tests/test_google_callback.py +++ b/backend/tests/test_google_callback.py @@ -23,8 +23,10 @@ class _FakeGoogle: def __init__(self, userinfo: dict): self.userinfo = userinfo + self.asked: dict = {} - async def authorize_redirect(self, _request, _redirect_uri, **_kw): + async def authorize_redirect(self, _request, _redirect_uri, **kw): + self.asked = kw return RedirectResponse(url="/fake-google") async def authorize_access_token(self, _request): @@ -218,12 +220,20 @@ def test_a_refused_sign_in_records_a_request_in_the_reported_language( assert _rows(db, User, User.email, "new@example.com") == [] -def _start_link(client, email: str): +def _password_login(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 _connect(client, access: str): + return client.get(f"/auth/google/connect?access={access}", follow_redirects=False) + + +def _start_link(client, email: str): + _password_login(client, email) + assert _connect(client, "identity").headers["location"] == "/fake-google" def test_linking_an_identity_another_account_holds_is_a_conflict(client, db, google, make_user): @@ -258,3 +268,72 @@ def test_linking_a_free_identity_attaches_it(client, db, google, make_user): assert r.headers["location"] == "/?link=ok" db.refresh(me) assert (me.google_sub, me.google_email) == ("sub-free", "me3.google@example.com") + + +@pytest.mark.parametrize( + ("access", "scope", "prompt"), + [ + ("identity", "openid email profile", "select_account"), + ("read", auth_routes.READ_SCOPES, "consent"), + ("write", auth_routes.WRITE_SCOPES, "consent"), + ], +) +def test_one_connect_route_asks_google_for_what_the_access_needs( + client, google, make_user, access, scope, prompt +): + make_user(email=f"ask-{access}@example.com") + _password_login(client, f"ask-{access}@example.com") + + assert _connect(client, access).headers["location"] == "/fake-google" + + assert (google.asked["scope"], google.asked["prompt"]) == (scope, prompt) + assert google.asked["access_type"] == "offline" + + +@pytest.mark.parametrize("access", ["read", "write"]) +def test_a_youtube_grant_attaches_to_the_password_account_and_returns_to_the_app( + client, db, google, make_user, access +): + me = make_user(email=f"grant-{access}@example.com") + _password_login(client, f"grant-{access}@example.com") + _connect(client, access) + + r = _sign_in(client, google, sub=f"sub-grant-{access}", email=f"g-{access}@example.com") + + # Back to the app, not the "linked" notice: the onboarding wizard picks up where it was. + assert r.headers["location"] == "/" + db.refresh(me) + assert me.google_sub == f"sub-grant-{access}" + assert _rows(db, User, User.google_sub, f"sub-grant-{access}") == [me] + + +@pytest.mark.parametrize("access", ["", "admin", "READ"]) +def test_an_unknown_access_starts_no_connect(client, google, make_user, access): + make_user(email="odd@example.com") + _password_login(client, "odd@example.com") + + assert _connect(client, access).status_code == 422 + assert google.asked == {} + + +def test_a_connect_needs_a_signed_in_account(client, google): + assert _connect(client, "identity").status_code == 401 + assert google.asked == {} + + +@pytest.mark.parametrize("access", ["identity", "read", "write"]) +def test_without_google_configured_a_connect_says_it_failed(client, make_user, monkeypatch, access): + monkeypatch.setattr(auth_routes, "google_oauth", lambda _db: None) + make_user(email=f"nog-{access}@example.com") + _password_login(client, f"nog-{access}@example.com") + + assert _connect(client, access).headers["location"] == "/?link=error" + + +@pytest.mark.parametrize("path", ["/auth/link", "/auth/upgrade", "/auth/upgrade?access=write"]) +def test_the_old_connect_routes_are_gone(client, google, make_user, path): + make_user(email="old@example.com") + _password_login(client, "old@example.com") + + assert client.get(path, follow_redirects=False).status_code == 404 + assert google.asked == {} diff --git a/frontend/src/components/SettingsPanel.tsx b/frontend/src/components/SettingsPanel.tsx index e2995d8d..a0ab887c 100644 --- a/frontend/src/components/SettingsPanel.tsx +++ b/frontend/src/components/SettingsPanel.tsx @@ -1,5 +1,5 @@ import { useState } from "react"; -import { withActiveAccount } from "../lib/activeAccount"; +import { connectGoogle } from "../lib/googleConnect"; import { qk } from "../lib/queryKeys"; import { formatDate, formatDateTime, formatUnit } from "../lib/format"; import { useTranslation } from "react-i18next"; @@ -359,7 +359,7 @@ function AccessRow({ // Sign-in methods: link a Google account to a password account (or vice-versa set a password), // so either method can reach the same account. Google connect is a full-page OAuth round-trip -// (auth.py attaches the identity to the current session via /auth/link); the password form posts +// (auth.py attaches the identity to the signed-in account via /auth/google/connect); the password form posts // directly with inline errors. export function SignInMethods({ me }: { me: Me }) { const { t } = useTranslation(); @@ -451,13 +451,7 @@ export function SignInMethods({ me }: { me: Me }) { {me.has_google ? ( {t("settings.account.googleLink.connected")} ) : ( - )} @@ -877,18 +871,14 @@ function Account({ me, onOpenWizard }: { me: Me; onOpenWizard: () => void }) { granted={me.can_read} grantedHint={t("settings.account.readGrantedHint")} enableHint={t("settings.account.readEnableHint")} - onEnable={() => { - window.location.href = withActiveAccount("/auth/upgrade?access=read"); - }} + onEnable={() => connectGoogle("read")} /> { - window.location.href = withActiveAccount("/auth/upgrade?access=write"); - }} + onEnable={() => connectGoogle("write")} />