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")} />