From 2574dc52c5a88f596bbe2be23bfaf642c4c5c479 Mon Sep 17 00:00:00 2001 From: Codex Date: Fri, 24 Apr 2026 09:28:58 -0500 Subject: [PATCH] Fix TeamSnap callback stalls --- backend/app/routes/auth.py | 37 ++++++++++++-- backend/tests/test_api.py | 83 +++++++++++++++++++++++++++++++ frontend/src/pages/SignInPage.tsx | 8 +-- 3 files changed, 122 insertions(+), 6 deletions(-) diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 6fa0ab8..afa8f29 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -2,6 +2,7 @@ from __future__ import annotations import secrets import time +from urllib.parse import urlencode from fastapi import APIRouter, Depends, HTTPException, Query, Request, Response, status from fastapi.responses import JSONResponse, RedirectResponse @@ -42,6 +43,10 @@ def normalize_return_to(return_to: str | None) -> str: return return_to +def build_signin_error_redirect_url(message: str) -> str: + return f"/signin?{urlencode({'error': message})}" + + @router.get("/teamsnap/start") def teamsnap_start(return_to: str | None = Query(default="/")) -> Response: if not settings.teamsnap_client_id: @@ -63,14 +68,31 @@ def teamsnap_start(return_to: str | None = Query(default="/")) -> Response: @router.get("/teamsnap/callback") async def teamsnap_callback( request: Request, - code: str = Query(...), + code: str | None = Query(default=None), + error: str | None = Query(default=None), db: Session = Depends(get_db), ) -> Response: + if error: + redirect = RedirectResponse( + url=build_signin_error_redirect_url(f"TeamSnap sign-in failed: {error}"), + status_code=status.HTTP_303_SEE_OTHER, + ) + set_no_store(redirect) + redirect.delete_cookie(settings.auth_return_cookie_name) + return redirect + + if not code: + redirect = RedirectResponse( + url=build_signin_error_redirect_url("TeamSnap sign-in did not return an authorization code."), + status_code=status.HTTP_303_SEE_OTHER, + ) + set_no_store(redirect) + redirect.delete_cookie(settings.auth_return_cookie_name) + return redirect + token_payload = await exchange_code_for_token(code) session = UserSession(session_token=create_session_token(), provider="teamsnap") update_session_tokens(session, token_payload) - if session.access_token: - session.external_user_id = await fetch_teamsnap_user_id(session.access_token) db.add(session) db.commit() redirect_target = normalize_return_to(request.cookies.get(settings.auth_return_cookie_name)) @@ -113,12 +135,21 @@ async def teamsnap_token( if not session.access_token: raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Missing TeamSnap access token") + session_updated = False + + if not session.external_user_id: + session.external_user_id = await fetch_teamsnap_user_id(session.access_token) + session_updated = session.external_user_id is not None + expires_soon = session.token_expires_at is None or session.token_expires_at.timestamp() <= (time.time() + 60) if expires_soon and session.refresh_token: token_payload = await refresh_access_token(session.refresh_token) update_session_tokens(session, token_payload) if not session.external_user_id and session.access_token: session.external_user_id = await fetch_teamsnap_user_id(session.access_token) + session_updated = True + + if session_updated or expires_soon: db.add(session) db.commit() db.refresh(session) diff --git a/backend/tests/test_api.py b/backend/tests/test_api.py index 12dfc6f..26143a0 100644 --- a/backend/tests/test_api.py +++ b/backend/tests/test_api.py @@ -111,6 +111,89 @@ def test_teamsnap_token_returns_proxy_api_root() -> None: assert response.headers["cache-control"] == "no-store" +def test_teamsnap_callback_redirects_without_waiting_for_user_lookup(monkeypatch: pytest.MonkeyPatch) -> None: + from app.routes import auth as auth_routes + + async def fake_exchange_code_for_token(code: str) -> dict: + assert code == "test-code" + return { + "access_token": "callback-access-token", + "refresh_token": "callback-refresh-token", + "expires_in": 3600, + } + + async def fail_fetch_teamsnap_user_id(_: str) -> str | None: + raise AssertionError("callback should not fetch the TeamSnap user profile before redirecting") + + monkeypatch.setattr(auth_routes, "exchange_code_for_token", fake_exchange_code_for_token) + monkeypatch.setattr(auth_routes, "fetch_teamsnap_user_id", fail_fetch_teamsnap_user_id) + + client.cookies.set(settings.auth_return_cookie_name, "/library") + response = client.get("/auth/teamsnap/callback", params={"code": "test-code"}, follow_redirects=False) + + assert response.status_code == 303 + assert response.headers["location"] == "/library" + assert response.headers["cache-control"] == "no-store" + assert settings.session_cookie_name in response.cookies + + db = SessionLocal() + session = db.query(UserSession).filter_by(session_token=response.cookies[settings.session_cookie_name]).one() + assert session.provider == "teamsnap" + assert session.access_token == "callback-access-token" + assert session.refresh_token == "callback-refresh-token" + assert session.external_user_id is None + db.close() + + +def test_teamsnap_callback_redirects_to_signin_when_code_is_blank() -> None: + client.cookies.set(settings.auth_return_cookie_name, "/library") + + response = client.get("/auth/teamsnap/callback", params={"code": ""}, follow_redirects=False) + + assert response.status_code == 303 + assert response.headers["location"] == "/signin?error=TeamSnap+sign-in+did+not+return+an+authorization+code." + assert settings.auth_return_cookie_name not in response.cookies + + +def test_teamsnap_callback_redirects_to_signin_when_teamsnap_returns_error() -> None: + client.cookies.set(settings.auth_return_cookie_name, "/library") + + response = client.get("/auth/teamsnap/callback", params={"error": "access_denied"}, follow_redirects=False) + + assert response.status_code == 303 + assert response.headers["location"] == "/signin?error=TeamSnap+sign-in+failed%3A+access_denied" + assert settings.auth_return_cookie_name not in response.cookies + + +def test_teamsnap_token_backfills_external_user_id(monkeypatch: pytest.MonkeyPatch) -> None: + from app.routes import auth as auth_routes + + async def fake_fetch_teamsnap_user_id(access_token: str) -> str | None: + assert access_token == "token-value" + return "user-42" + + monkeypatch.setattr(auth_routes, "fetch_teamsnap_user_id", fake_fetch_teamsnap_user_id) + + db = SessionLocal() + session = UserSession(session_token="teamsnap-session", provider="teamsnap", access_token="token-value") + db.add(session) + db.commit() + db.close() + + client.cookies.set(settings.session_cookie_name, "teamsnap-session") + response = client.post( + "/auth/teamsnap/token", + headers={"host": "kif.local.ascorrea.com", "x-forwarded-proto": "https"}, + ) + + assert response.status_code == 200 + + db = SessionLocal() + refreshed_session = db.query(UserSession).filter_by(session_token="teamsnap-session").one() + assert refreshed_session.external_user_id == "user-42" + db.close() + + def test_session_and_clip_reads_use_cache_validators() -> None: login = client.post("/auth/admin/login", json={"username": "admin", "password": "admin"}) assert login.status_code == 200 diff --git a/frontend/src/pages/SignInPage.tsx b/frontend/src/pages/SignInPage.tsx index c06ffc2..6ebc3e0 100644 --- a/frontend/src/pages/SignInPage.tsx +++ b/frontend/src/pages/SignInPage.tsx @@ -1,12 +1,14 @@ import { useState } from "react"; -import { useLocation, useNavigate } from "react-router-dom"; +import { useLocation, useNavigate, useSearchParams } from "react-router-dom"; import { api } from "../api/client"; export function SignInPage() { const navigate = useNavigate(); const location = useLocation(); + const [searchParams] = useSearchParams(); const [error, setError] = useState(null); + const callbackError = searchParams.get("error"); async function handleTeamSnapStart() { try { @@ -46,10 +48,10 @@ export function SignInPage() { - {error ? ( + {error || callbackError ? (
- {error} + {error ?? callbackError}
) : null}