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.
This commit is contained in:
+30
-44
@@ -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,
|
||||
)
|
||||
|
||||
@@ -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 == {}
|
||||
|
||||
@@ -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 ? (
|
||||
<Badge tone="accent">{t("settings.account.googleLink.connected")}</Badge>
|
||||
) : (
|
||||
<Button
|
||||
variant="glass"
|
||||
size="sm"
|
||||
onClick={() => {
|
||||
window.location.href = withActiveAccount("/auth/link");
|
||||
}}
|
||||
>
|
||||
<Button variant="glass" size="sm" onClick={() => connectGoogle("identity")}>
|
||||
{t("settings.account.googleLink.connect")}
|
||||
</Button>
|
||||
)}
|
||||
@@ -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")}
|
||||
/>
|
||||
<AccessRow
|
||||
title={t("settings.account.writeTitle")}
|
||||
granted={me.can_write}
|
||||
grantedHint={t("settings.account.writeGrantedHint")}
|
||||
enableHint={t("settings.account.writeEnableHint")}
|
||||
onEnable={() => {
|
||||
window.location.href = withActiveAccount("/auth/upgrade?access=write");
|
||||
}}
|
||||
onEnable={() => connectGoogle("write")}
|
||||
/>
|
||||
</div>
|
||||
<Button variant="link" className="mt-2 text-sm" onClick={onOpenWizard}>
|
||||
|
||||
@@ -0,0 +1,21 @@
|
||||
import { readFileSync } from "node:fs";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { GOOGLE_ACCESS, googleConnectUrl } from "./googleConnect";
|
||||
|
||||
// The frontend builds the Google connect URL by hand, so nothing but this test notices when the
|
||||
// backend route moves or its `access` values change: the click would land on a 404 or a 422 page.
|
||||
const AUTH = readFileSync(new URL("../../../backend/app/routes/auth.py", import.meta.url), "utf-8");
|
||||
|
||||
describe("the Google connect URL", () => {
|
||||
it("names the backend route", () => {
|
||||
const prefix = /APIRouter\(prefix="([^"]+)"/.exec(AUTH)?.[1];
|
||||
const path = /@router\.get\("([^"]+)"\)\nasync def google_connect\(/.exec(AUTH)?.[1];
|
||||
expect(googleConnectUrl("read").split("?")[0]).toBe(`${prefix}${path}`);
|
||||
});
|
||||
|
||||
it("offers exactly the access values the backend accepts", () => {
|
||||
const literal = /access: Literal\[([^\]]+)\]/.exec(AUTH)?.[1] ?? "";
|
||||
const accepted = [...literal.matchAll(/"([^"]+)"/g)].map((m) => m[1]);
|
||||
expect(accepted).toEqual([...GOOGLE_ACCESS]);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,16 @@
|
||||
// Every in-app Google round-trip starts at one backend route (auth.py `google_connect`): attaching a
|
||||
// Google sign-in to the account (`identity`), or granting YouTube access on top (`read`, `write`).
|
||||
// The page navigates away to Google and comes back through the shared callback, which lands on
|
||||
// `?link=…` (App.tsx) for `identity` and on the app itself for a grant.
|
||||
|
||||
export const GOOGLE_ACCESS = ["identity", "read", "write"] as const;
|
||||
export type GoogleAccess = (typeof GOOGLE_ACCESS)[number];
|
||||
|
||||
export function googleConnectUrl(access: GoogleAccess): string {
|
||||
return `/auth/google/connect?access=${access}`;
|
||||
}
|
||||
|
||||
/** Leave for Google's sign-in or consent screen. */
|
||||
export function connectGoogle(access: GoogleAccess): void {
|
||||
window.location.href = googleConnectUrl(access);
|
||||
}
|
||||
@@ -12,7 +12,7 @@
|
||||
// suppresses the auto-popup on future logins (they
|
||||
// can still reopen it from Settings → Account).
|
||||
|
||||
import { withActiveAccount } from "./activeAccount";
|
||||
import { connectGoogle } from "./googleConnect";
|
||||
import { getAccountRaw, LS, setAccountRaw } from "./storage";
|
||||
|
||||
const ONBOARD_ACTIVE = "siftlode.onboarding.active"; // sessionStorage (not in LS, which is localStorage)
|
||||
@@ -34,7 +34,7 @@ export function shouldAutoOpenOnboarding(me: {
|
||||
/** Mark a grant as in progress, then send the user to Google's consent screen. */
|
||||
export function beginGrant(access: "read" | "write"): void {
|
||||
sessionStorage.setItem(ONBOARD_ACTIVE, "1");
|
||||
window.location.href = withActiveAccount(`/auth/upgrade?access=${access}`);
|
||||
connectGoogle(access);
|
||||
}
|
||||
|
||||
/** Leave the wizard. `dismiss` suppresses the future auto-popup (used when read is skipped). */
|
||||
|
||||
Reference in New Issue
Block a user