fix(auth): address second-round review findings
- OAuth-cancel feedback now keys on the live session, not link_uid: ANY signed-in cancel (add-another-account via /auth/login, link, or upgrade) redirects to /?oauth=cancelled and gets a neutral "Google sign-in was cancelled" toast — the add-account path set no marker and was silent, and the shared ?link= message wrongly called an upgrade "account linking" - drop 127.0.0.1 from the dev OAuth host allowlist: only the localhost callbacks are registered in the Google console, so a 127.0.0.1 origin now falls back to the fixed callback instead of a redirect_uri_mismatch - Welcome: extract the shared goResend handler (banner CTA + sign-in link) and merge the two adjacent signin-only blocks
This commit is contained in:
+12
-9
@@ -249,10 +249,11 @@ def upsert_pending_invite(db: Session, email: str) -> Invite | None:
|
|||||||
# Local-dev origins allowed to drive the OAuth redirect_uri from the request Host. The Vite dev
|
# Local-dev origins allowed to drive the OAuth redirect_uri from the request Host. The Vite dev
|
||||||
# server (:5173) proxies /auth to the backend (:8080) preserving the Host header, and the session
|
# server (:5173) proxies /auth to the backend (:8080) preserving the Host header, and the session
|
||||||
# cookie is port-agnostic — so a flow started on :5173 can come back to :5173 instead of always
|
# cookie is port-agnostic — so a flow started on :5173 can come back to :5173 instead of always
|
||||||
# landing on the fixed :8080 callback. Both must also be registered in the Google console.
|
# landing on the fixed :8080 callback. Only the localhost variants are listed: they must ALSO be
|
||||||
_DEV_OAUTH_HOSTS = frozenset(
|
# registered as redirect URIs in the Google console, and Google rejects an unregistered one with
|
||||||
{"localhost:5173", "localhost:8080", "127.0.0.1:5173", "127.0.0.1:8080"}
|
# redirect_uri_mismatch. A 127.0.0.1 origin isn't registered, so it deliberately falls through to
|
||||||
)
|
# the fixed configured callback (which is registered) rather than producing a mismatch.
|
||||||
|
_DEV_OAUTH_HOSTS = frozenset({"localhost:5173", "localhost:8080"})
|
||||||
|
|
||||||
|
|
||||||
def _oauth_redirect_uri(request: Request) -> str:
|
def _oauth_redirect_uri(request: Request) -> str:
|
||||||
@@ -309,12 +310,14 @@ async def callback(
|
|||||||
token = await client.authorize_access_token(request)
|
token = await client.authorize_access_token(request)
|
||||||
except OAuthError as exc:
|
except OAuthError as exc:
|
||||||
# The user cancelled or denied consent at Google — land back with a friendly notice instead
|
# The user cancelled or denied consent at Google — land back with a friendly notice instead
|
||||||
# of a raw 400 JSON error page. A link/upgrade cancel is an ALREADY-signed-in user who never
|
# of a raw 400 JSON error page. An already-signed-in user (adding another account via
|
||||||
# sees the pre-auth login banner, so route it through the ?link= channel App.tsx surfaces as
|
# /auth/login, or linking/upgrading via /auth/link|upgrade) never sees the pre-auth login
|
||||||
# a toast; a plain sign-in cancel lands on the login page with the oauth_cancelled banner.
|
# 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.
|
||||||
log.info("Google OAuth cancelled/failed: %s", exc.error)
|
log.info("Google OAuth cancelled/failed: %s", exc.error)
|
||||||
if link_uid is not None:
|
if request.session.get("user_id") is not None:
|
||||||
return RedirectResponse(url="/?link=cancelled")
|
return RedirectResponse(url="/?oauth=cancelled")
|
||||||
return RedirectResponse(url="/?login=oauth_cancelled")
|
return RedirectResponse(url="/?login=oauth_cancelled")
|
||||||
|
|
||||||
userinfo = token.get("userinfo")
|
userinfo = token.get("userinfo")
|
||||||
|
|||||||
@@ -30,7 +30,6 @@ def test_dev_known_host_returns_to_that_origin(monkeypatch):
|
|||||||
monkeypatch.setattr(auth, "settings", _dev_settings())
|
monkeypatch.setattr(auth, "settings", _dev_settings())
|
||||||
assert auth._oauth_redirect_uri(_req("localhost:5173")) == "http://localhost:5173/auth/callback"
|
assert auth._oauth_redirect_uri(_req("localhost:5173")) == "http://localhost:5173/auth/callback"
|
||||||
assert auth._oauth_redirect_uri(_req("localhost:8080")) == "http://localhost:8080/auth/callback"
|
assert auth._oauth_redirect_uri(_req("localhost:8080")) == "http://localhost:8080/auth/callback"
|
||||||
assert auth._oauth_redirect_uri(_req("127.0.0.1:5173")) == "http://127.0.0.1:5173/auth/callback"
|
|
||||||
|
|
||||||
|
|
||||||
def test_dev_host_is_case_insensitive(monkeypatch):
|
def test_dev_host_is_case_insensitive(monkeypatch):
|
||||||
@@ -41,6 +40,9 @@ def test_dev_host_is_case_insensitive(monkeypatch):
|
|||||||
def test_dev_unknown_or_missing_host_falls_back_to_configured(monkeypatch):
|
def test_dev_unknown_or_missing_host_falls_back_to_configured(monkeypatch):
|
||||||
monkeypatch.setattr(auth, "settings", _dev_settings())
|
monkeypatch.setattr(auth, "settings", _dev_settings())
|
||||||
# An origin not on the allowlist (or a request with no Host) must not drive the redirect.
|
# An origin not on the allowlist (or a request with no Host) must not drive the redirect.
|
||||||
|
# 127.0.0.1 is deliberately NOT allowlisted (not registered in the Google console), so it falls
|
||||||
|
# back to the fixed configured callback rather than producing a redirect_uri_mismatch.
|
||||||
|
assert auth._oauth_redirect_uri(_req("127.0.0.1:5173")) == "http://localhost:8080/auth/callback"
|
||||||
assert auth._oauth_redirect_uri(_req("evil.example.com")) == "http://localhost:8080/auth/callback"
|
assert auth._oauth_redirect_uri(_req("evil.example.com")) == "http://localhost:8080/auth/callback"
|
||||||
assert auth._oauth_redirect_uri(_req(None)) == "http://localhost:8080/auth/callback"
|
assert auth._oauth_redirect_uri(_req(None)) == "http://localhost:8080/auth/callback"
|
||||||
|
|
||||||
|
|||||||
@@ -192,9 +192,6 @@ export default function App() {
|
|||||||
void meQuery.refetch();
|
void meQuery.refetch();
|
||||||
setAccountRaw(LS.settingsTab, "account");
|
setAccountRaw(LS.settingsTab, "account");
|
||||||
setPage("settings");
|
setPage("settings");
|
||||||
} else if (result === "cancelled") {
|
|
||||||
// The user backed out at Google's consent screen — nothing changed, just acknowledge it.
|
|
||||||
notify({ level: "info", message: t("settings.account.googleLink.cancelled") });
|
|
||||||
} else {
|
} else {
|
||||||
const key = result === "conflict" || result === "mismatch" ? result : "error";
|
const key = result === "conflict" || result === "mismatch" ? result : "error";
|
||||||
notify({ level: "error", message: t(`settings.account.googleLink.${key}`) });
|
notify({ level: "error", message: t(`settings.account.googleLink.${key}`) });
|
||||||
@@ -202,6 +199,15 @@ export default function App() {
|
|||||||
stripUrlParams();
|
stripUrlParams();
|
||||||
}, []); // eslint-disable-line react-hooks/exhaustive-deps
|
}, []); // eslint-disable-line react-hooks/exhaustive-deps
|
||||||
|
|
||||||
|
// A Google OAuth flow started while already signed in (add-another-account, link, or upgrade) was
|
||||||
|
// cancelled at Google's consent screen. The backend only emits this for a live session, so a
|
||||||
|
// neutral toast is the right feedback — the pre-auth login banner would never render here.
|
||||||
|
useEffect(() => {
|
||||||
|
if (new URLSearchParams(window.location.search).get("oauth") !== "cancelled") return;
|
||||||
|
notify({ level: "info", message: t("settings.account.googleCancelled") });
|
||||||
|
stripUrlParams();
|
||||||
|
}, []); // eslint-disable-line react-hooks/exhaustive-deps
|
||||||
|
|
||||||
// Deep-link from the "new access request" admin email (?admin=access-requests) → jump straight to
|
// Deep-link from the "new access request" admin email (?admin=access-requests) → jump straight to
|
||||||
// the admin Users → Access requests view. Snapshot the param so the pre-login URL strip doesn't
|
// the admin Users → Access requests view. Snapshot the param so the pre-login URL strip doesn't
|
||||||
// drop it; act once `me` is known, and only for an admin (others just land on their default page).
|
// drop it; act once `me` is known, and only for an admin (others just land on their default page).
|
||||||
|
|||||||
@@ -346,6 +346,12 @@ function AuthCard() {
|
|||||||
setLogin(null);
|
setLogin(null);
|
||||||
};
|
};
|
||||||
|
|
||||||
|
// Shared by the expired-link banner CTA and the sign-in "didn't get the email?" link.
|
||||||
|
const goResend = () => {
|
||||||
|
reset();
|
||||||
|
setMode("resend");
|
||||||
|
};
|
||||||
|
|
||||||
async function onSubmit(e: React.FormEvent) {
|
async function onSubmit(e: React.FormEvent) {
|
||||||
e.preventDefault();
|
e.preventDefault();
|
||||||
reset();
|
reset();
|
||||||
@@ -423,13 +429,7 @@ function AuthCard() {
|
|||||||
{verify === "invalid" && (
|
{verify === "invalid" && (
|
||||||
<Banner tone="warn">
|
<Banner tone="warn">
|
||||||
{t("welcome.auth.verifyInvalid")}{" "}
|
{t("welcome.auth.verifyInvalid")}{" "}
|
||||||
<button
|
<button onClick={goResend} className="font-semibold text-accent hover:underline">
|
||||||
onClick={() => {
|
|
||||||
reset(); // also clears the verify banner (see reset())
|
|
||||||
setMode("resend");
|
|
||||||
}}
|
|
||||||
className="font-semibold text-accent hover:underline"
|
|
||||||
>
|
|
||||||
{t("welcome.auth.resendCta")}
|
{t("welcome.auth.resendCta")}
|
||||||
</button>
|
</button>
|
||||||
</Banner>
|
</Banner>
|
||||||
@@ -530,22 +530,16 @@ function AuthCard() {
|
|||||||
)}
|
)}
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
{/* A verification email can be lost or never arrive, so offer a resend path from sign-in
|
|
||||||
too — not only from the expired-link banner (which needs a broken link to appear). */}
|
|
||||||
{mode === "signin" && (
|
|
||||||
<button
|
|
||||||
onClick={() => {
|
|
||||||
reset();
|
|
||||||
setMode("resend");
|
|
||||||
}}
|
|
||||||
className="mt-2 block w-full text-center text-xs text-muted hover:text-fg"
|
|
||||||
>
|
|
||||||
{t("welcome.auth.resendLink")}
|
|
||||||
</button>
|
|
||||||
)}
|
|
||||||
|
|
||||||
{mode === "signin" && (
|
{mode === "signin" && (
|
||||||
<>
|
<>
|
||||||
|
{/* A verification email can be lost or never arrive, so offer a resend path from
|
||||||
|
sign-in too — not only from the expired-link banner (needs a broken link to show). */}
|
||||||
|
<button
|
||||||
|
onClick={goResend}
|
||||||
|
className="mt-2 block w-full text-center text-xs text-muted hover:text-fg"
|
||||||
|
>
|
||||||
|
{t("welcome.auth.resendLink")}
|
||||||
|
</button>
|
||||||
<div className="flex items-center gap-3 my-4 text-[11px] uppercase tracking-wide text-muted">
|
<div className="flex items-center gap-3 my-4 text-[11px] uppercase tracking-wide text-muted">
|
||||||
<span className="h-px flex-1 bg-border" />
|
<span className="h-px flex-1 bg-border" />
|
||||||
{t("welcome.auth.or")}
|
{t("welcome.auth.or")}
|
||||||
|
|||||||
@@ -78,9 +78,9 @@
|
|||||||
"linked": "Google account linked.",
|
"linked": "Google account linked.",
|
||||||
"conflict": "That Google account is already linked to a different Siftlode account.",
|
"conflict": "That Google account is already linked to a different Siftlode account.",
|
||||||
"mismatch": "That's a different Google account than the one already linked here. Sign in with the linked one.",
|
"mismatch": "That's a different Google account than the one already linked here. Sign in with the linked one.",
|
||||||
"error": "Couldn't link the Google account. Please try again.",
|
"error": "Couldn't link the Google account. Please try again."
|
||||||
"cancelled": "Google account linking was cancelled."
|
|
||||||
},
|
},
|
||||||
|
"googleCancelled": "Google sign-in was cancelled.",
|
||||||
"password": {
|
"password": {
|
||||||
"title": "Password",
|
"title": "Password",
|
||||||
"setHint": "A password is set. You can sign in with your email and password.",
|
"setHint": "A password is set. You can sign in with your email and password.",
|
||||||
|
|||||||
@@ -78,9 +78,9 @@
|
|||||||
"linked": "Google-fiók összekapcsolva.",
|
"linked": "Google-fiók összekapcsolva.",
|
||||||
"conflict": "Ez a Google-fiók már egy másik Siftlode-fiókhoz van kapcsolva.",
|
"conflict": "Ez a Google-fiók már egy másik Siftlode-fiókhoz van kapcsolva.",
|
||||||
"mismatch": "Ez nem az a Google-fiók, amely ide már össze van kapcsolva. Az összekapcsolttal jelentkezz be.",
|
"mismatch": "Ez nem az a Google-fiók, amely ide már össze van kapcsolva. Az összekapcsolttal jelentkezz be.",
|
||||||
"error": "Nem sikerült összekapcsolni a Google-fiókot. Próbáld újra.",
|
"error": "Nem sikerült összekapcsolni a Google-fiókot. Próbáld újra."
|
||||||
"cancelled": "A Google-fiók összekapcsolása megszakadt."
|
|
||||||
},
|
},
|
||||||
|
"googleCancelled": "A Google-belépés megszakadt.",
|
||||||
"password": {
|
"password": {
|
||||||
"title": "Jelszó",
|
"title": "Jelszó",
|
||||||
"setHint": "Van beállított jelszó. Bejelentkezhetsz e-mail-címmel és jelszóval.",
|
"setHint": "Van beállított jelszó. Bejelentkezhetsz e-mail-címmel és jelszóval.",
|
||||||
|
|||||||
Reference in New Issue
Block a user