Return users to the page they asked for after OIDC login

README.md

@@ -197,6 +197,14 @@ OIDC_OVERWRITE_REDIRECT_URI=https://<YOUR_ACCESS_DEPLOYMENT_DOMAIN_NAME>/oidc/au
 ALLOWED_HOSTS=<YOUR_ACCESS_DEPLOYMENT_DOMAIN_NAME>
 ```
 
+Signed-out users who follow a link into Access — say a group page you sent
+someone so they can request membership — are sent through the IdP and returned
+to the page they asked for, query string and all, rather than dropped on the
+home page. The same holds when a session expires while the app is open: the
+browser is handed to the IdP and comes back to whatever page the user was on.
+Only same-origin paths are accepted as a return target, so the flow can't be
+used to bounce anyone to a third-party host.
+
 #### Cloudflare Access
 
 To use Cloudflare Access authentication, set up a

api/auth/dependencies.py

@@ -57,6 +57,17 @@ def __init__(self, next_path: str) -> None:
         super().__init__(f"OIDC login required (next={next_path!r})")
 
 
+def _requested_path(request: Request) -> str:
+    """The path the caller asked for, query string included.
+
+    The SPA keeps list filters, search terms, sort order and pagination in the
+    query string, so those are the shareable part of a deep link — dropping
+    them lands the user on an unfiltered page after login.
+    """
+    query = request.url.query
+    return f"{request.url.path}?{query}" if query else request.url.path
+
+
 async def get_current_user_id(request: Request, db: DbSession) -> str:
     """Resolve the current user id, raising 403 if unauthenticated."""
     if settings.ENV in ("development", "test"):
@@ -94,9 +105,11 @@ async def get_current_user_id(request: Request, db: DbSession) -> str:
     if settings.OIDC_CLIENT_SECRETS:
         userinfo = request.session.get("userinfo") if hasattr(request, "session") else None
         if not userinfo or "email" not in userinfo:
-            # Browser flow: the SPA should follow the 307 to the OIDC login
-            # endpoint, which kicks off the authorization-code redirect.
-            raise OIDCRedirectRequired(next_path=request.url.path)
+            # Browser navigation gets a 307 to the OIDC login endpoint, which
+            # kicks off the authorization-code redirect and returns the user to
+            # `next_path` afterwards. `/api/*` callers get a 401 instead — see
+            # `oidc_redirect_handler`.
+            raise OIDCRedirectRequired(next_path=_requested_path(request))
         user = await _lookup_user_by_email(db, userinfo["email"])
         request.state.current_user_id = user.id
         if settings.FASTAPI_SENTRY_DSN:

api/auth/oidc.py

@@ -67,13 +67,19 @@ def register_oidc(app: FastAPI) -> None:
 
 def _is_safe_next(next_url: Optional[str]) -> bool:
     # Reject absolute URLs and protocol-relative paths so `next` cannot bounce
-    # the post-auth redirect to a third-party host.
+    # the post-auth redirect to a third-party host. A query string and fragment
+    # are fine — the SPA keeps deep-link state (filters, search, pagination)
+    # there, so they have to survive the round trip.
     if not next_url or not next_url.startswith("/"):
         return False
     if next_url.startswith("//") or next_url.startswith("/\\"):
         return False
     parsed = urlparse(next_url)
-    return not parsed.scheme and not parsed.netloc
+    if parsed.scheme or parsed.netloc:
+        return False
+    # Landing back on an auth endpoint is never what the user meant: `/logout`
+    # would undo the login they just completed and `/login` would loop.
+    return not parsed.path.startswith(_router.prefix + "/")
 
 
 @_router.get("/login", name="oidc_login")

api/exception_handlers.py

@@ -2,9 +2,10 @@
 
 All HTTP errors cross the wire as
 `{"type": "about:blank", "title": "<reason>", "status": <code>, "detail": "<message>"}`
-with `Content-Type: application/problem+json`. Validation errors include a
-non-standard `errors:` extension with the full FastAPI/Pydantic error list
-for clients that want it.
+with `Content-Type: application/problem+json`. Two RFC 9457 extension members
+are in use: validation errors carry `errors:` (the full FastAPI/Pydantic error
+list) and the OIDC 401 carries `login_url:` (where the client should send the
+browser to sign in).
 
 `PluginNotFoundError` is the lone outlier: it emits `{"error": ...}` because
 the React plugin-admin page reads `error` (not `detail`). Migrating that page
@@ -27,6 +28,7 @@
 from pydantic import ValidationError
 from starlette.exceptions import HTTPException as StarletteHTTPException
 from starlette.requests import Request
+from starlette.responses import Response
 
 from api.auth.dependencies import OIDCRedirectRequired
 from api.exceptions import AccessException
@@ -39,6 +41,9 @@
 
 PROBLEM_JSON = "application/problem+json"
 
+# `api.auth.oidc` mounts the login endpoint here.
+LOGIN_PATH = "/oidc/login"
+
 
 def _is_api(request: Request) -> bool:
     return request.url.path.startswith("/api/") or request.url.path == "/api"
@@ -156,9 +161,34 @@ async def pydantic_validation_handler(request: Request, exc: ValidationError) ->
     )
 
 
-async def oidc_redirect_handler(request: Request, exc: OIDCRedirectRequired) -> RedirectResponse:
+def _is_document_navigation(request: Request) -> bool:
+    """Whether this request is a browser navigating to a page.
+
+    Only a top-level navigation can carry a user through an interactive IdP
+    round trip. `fetch`/XHR cannot: it would follow the redirect into the IdP's
+    cross-origin HTML, fail CORS, and surface as a generic network error rather
+    than a login prompt.
+    """
+    # Every browser that would follow the redirect sends Sec-Fetch-Mode; the
+    # Accept sniff is the fallback for clients that omit it.
+    sec_fetch_mode = request.headers.get("sec-fetch-mode")
+    if sec_fetch_mode is not None:
+        return sec_fetch_mode == "navigate"
+    return "text/html" in request.headers.get("accept", "")
+
+
+async def oidc_redirect_handler(request: Request, exc: OIDCRedirectRequired) -> Response:
+    if not _is_document_navigation(request):
+        # Hand XHR callers a 401 carrying the login endpoint instead, so the SPA
+        # can navigate the whole window there with its own location as `next`
+        # — see `redirectToLogin` in `src/api/apiFetcher.ts`.
+        return _problem(
+            status_code=401,
+            detail="Authentication required",
+            extras={"login_url": LOGIN_PATH},
+        )
     query = urlencode({"next": exc.next_path})
-    return RedirectResponse(url=f"/oidc/login?{query}", status_code=307)
+    return RedirectResponse(url=f"{LOGIN_PATH}?{query}", status_code=307)
 
 
 async def plugin_not_found_handler(request: Request, exc: PluginNotFoundError) -> JSONResponse:

src/api/apiFetcher.test.ts

@@ -0,0 +1,59 @@
+import {describe, it, expect, beforeEach, afterEach, vi} from 'vitest';
+import {apiFetch} from './apiFetcher';
+
+const assign = vi.fn();
+
+const setLocation = (pathname: string, search = '', hash = '') => {
+  Object.defineProperty(window, 'location', {
+    configurable: true,
+    value: {pathname, search, hash, assign},
+  });
+};
+
+const respondWith = (status: number, body: unknown) => {
+  vi.stubGlobal(
+    'fetch',
+    vi.fn(async () => new Response(JSON.stringify(body), {status, headers: {'Content-Type': 'application/json'}})),
+  );
+};
+
+// `apiFetch` deliberately never settles once it starts a login navigation, so
+// race it against a timer rather than awaiting it.
+const settles = (promise: Promise<unknown>) =>
+  Promise.race([promise.then(() => true).catch(() => true), new Promise((r) => setTimeout(() => r(false), 20))]);
+
+describe('apiFetch on 401', () => {
+  beforeEach(() => {
+    assign.mockClear();
+    setLocation('/groups/acme-painter');
+  });
+  afterEach(() => {
+    vi.unstubAllGlobals();
+  });
+
+  it('sends the browser to the login endpoint from the problem body', async () => {
+    respondWith(401, {status: 401, detail: 'Authentication required', login_url: '/oidc/login'});
+    const pending = apiFetch({url: '/api/users/@me', method: 'get'});
+    expect(await settles(pending)).toBe(false);
+    expect(assign).toHaveBeenCalledWith('/oidc/login?next=%2Fgroups%2Facme-painter');
+  });
+
+  it('asks to return to the current page, query string and fragment included', async () => {
+    setLocation('/roles', '?q=painter&page=2', '#members');
+    respondWith(401, {status: 401, detail: 'Authentication required', login_url: '/oidc/login'});
+    expect(await settles(apiFetch({url: '/api/roles', method: 'get'}))).toBe(false);
+    expect(assign).toHaveBeenCalledWith('/oidc/login?next=%2Froles%3Fq%3Dpainter%26page%3D2%23members');
+  });
+
+  it('falls back to the default login path when the body omits one', async () => {
+    respondWith(401, {status: 401, detail: 'Authentication required'});
+    expect(await settles(apiFetch({url: '/api/users/@me', method: 'get'}))).toBe(false);
+    expect(assign).toHaveBeenCalledWith('/oidc/login?next=%2Fgroups%2Facme-painter');
+  });
+
+  it('leaves other error statuses to the caller', async () => {
+    respondWith(403, {status: 403, detail: 'Forbidden'});
+    await expect(apiFetch({url: '/api/users/@me', method: 'get'})).rejects.toMatchObject({payload: 'Forbidden'});
+    expect(assign).not.toHaveBeenCalled();
+  });
+});

src/api/apiFetcher.ts

@@ -13,6 +13,9 @@ export type ErrorMessage = {
   title?: string;
   status?: number;
   detail?: string;
+  // Non-standard extension on 401s from an OIDC deployment: the endpoint that
+  // starts the login flow.
+  login_url?: string;
   errors?: Array<{
     type?: string;
     loc?: Array<string | number>;
@@ -21,6 +24,24 @@ export type ErrorMessage = {
   }>;
 };
 
+const DEFAULT_LOGIN_PATH = '/oidc/login';
+
+/**
+ * Hand the whole window to the login endpoint, asking to be returned to the
+ * page the user is currently on.
+ *
+ * The backend answers an unauthenticated `/api/*` request with a 401 rather
+ * than the 307-to-the-IdP a document navigation gets, because `fetch` can't
+ * complete an interactive login: it would follow the redirect to the IdP's
+ * cross-origin HTML and fail CORS. So the session-expiry case has to be
+ * escalated to a real navigation here, otherwise the user is stuck looking at
+ * an error on a page they can't reload their way out of.
+ */
+export const redirectToLogin = (loginUrl: string = DEFAULT_LOGIN_PATH) => {
+  const {pathname, search, hash} = window.location;
+  window.location.assign(`${loginUrl}?next=${encodeURIComponent(`${pathname}${search}${hash}`)}`);
+};
+
 export type ApiFetcherOptions<TBody, THeaders, TQueryParams, TPathParams> = {
   url: string;
   method: string;
@@ -74,12 +95,20 @@ export async function apiFetch<
       // React client renders directly via `error.payload`. `detail` is the
       // human-readable summary; fall back to `title`.
       let payload: string;
+      let problem: ErrorMessage = {};
       try {
-        const problem = (await response.json()) as ErrorMessage;
+        problem = (await response.json()) as ErrorMessage;
         payload = problem.detail ?? problem.title ?? 'Unexpected error';
       } catch (e) {
         payload = e instanceof Error ? `Unexpected error (${e.message})` : 'Unexpected error';
       }
+      if (response.status === 401) {
+        // The OIDC session expired or was never established. Log back in and
+        // come back to this page; the returned promise never settles because
+        // the navigation is already underway.
+        redirectToLogin(problem.login_url);
+        return await new Promise<TData>(() => {});
+      }
       throw {status: 'unknown' as const, payload};
     }