diff --git a/frontend/app/auth/callback/page.test.tsx b/frontend/app/auth/callback/page.test.tsx new file mode 100644 index 000000000..516371468 --- /dev/null +++ b/frontend/app/auth/callback/page.test.tsx @@ -0,0 +1,153 @@ +import { screen, waitFor } from "@testing-library/react"; +import { HttpResponse, http } from "msw"; +import { beforeEach, describe, expect, it } from "vitest"; +import { renderWithProviders } from "@/test-utils/render"; +import { mockRouter, setMockLocation } from "@/test-utils/router"; +import AuthCallbackPage from "./page"; + +let callbackRequests = 0; +const callbackHandler = http.post("/api/auth/callback", () => { + callbackRequests += 1; + return HttpResponse.json({ purpose: "app_auth" }); +}); + +function setCallbackLocation(code: string) { + setMockLocation({ + pathname: "/auth/callback", + searchParams: { code, state: "test-state" }, + }); +} + +describe("AuthCallbackPage", () => { + beforeEach(() => { + callbackRequests = 0; + localStorage.setItem("auth_purpose", "app_auth"); + }); + + // A inside the provider wrapper doesn't double-invoke effects. + it("finishes sign-in under StrictMode's effect double-invoke", async () => { + setCallbackLocation(crypto.randomUUID()); + + renderWithProviders(, { + providers: ["auth"], + handlers: [callbackHandler], + reactStrictMode: true, + }); + + expect(await screen.findByText("Welcome to OpenRAG!")).toBeInTheDocument(); + await waitFor(() => expect(mockRouter.push).toHaveBeenCalledWith("/chat"), { + timeout: 3000, + }); + expect(callbackRequests).toBe(1); + }); + + it("reuses a settled exchange when the page remounts with the same code", async () => { + setCallbackLocation(crypto.randomUUID()); + + const first = renderWithProviders(, { + providers: ["auth"], + handlers: [callbackHandler], + }); + expect(await screen.findByText("Welcome to OpenRAG!")).toBeInTheDocument(); + first.unmount(); + + renderWithProviders(, { + providers: ["auth"], + handlers: [callbackHandler], + }); + expect(await screen.findByText("Welcome to OpenRAG!")).toBeInTheDocument(); + expect(callbackRequests).toBe(1); + }); + + it("redirects to auth_redirect_to after sign-in", async () => { + setCallbackLocation(crypto.randomUUID()); + localStorage.setItem("auth_redirect_to", "/knowledge"); + + renderWithProviders(, { + providers: ["auth"], + handlers: [callbackHandler], + reactStrictMode: true, + }); + + await waitFor( + () => expect(mockRouter.push).toHaveBeenCalledWith("/knowledge"), + { timeout: 3000 }, + ); + expect(localStorage.getItem("auth_redirect_to")).toBeNull(); + }); + + it("shows the backend's error and allows a retry when the exchange fails", async () => { + const code = crypto.randomUUID(); + setCallbackLocation(code); + + renderWithProviders(, { + providers: ["auth"], + handlers: [ + http.post("/api/auth/callback", () => + HttpResponse.json({ error: "invalid_grant" }, { status: 400 }), + ), + ], + reactStrictMode: true, + }); + + expect(await screen.findByText("Sign In Failed")).toBeInTheDocument(); + expect(screen.getByText("invalid_grant")).toBeInTheDocument(); + expect(sessionStorage.getItem(`callback_processed_${code}`)).toBeNull(); + }); + + it("clears the dedupe state when the exchange fails after unmount", async () => { + const code = crypto.randomUUID(); + setCallbackLocation(code); + let failRequest!: () => void; + const requestFailed = new Promise((resolve) => { + failRequest = resolve; + }); + + const view = renderWithProviders(, { + providers: ["auth"], + handlers: [ + http.post("/api/auth/callback", async () => { + callbackRequests += 1; + await requestFailed; + return HttpResponse.json({ error: "invalid_grant" }, { status: 400 }); + }), + ], + }); + await waitFor(() => expect(callbackRequests).toBe(1)); + view.unmount(); + failRequest(); + + await waitFor(() => + expect(sessionStorage.getItem(`callback_processed_${code}`)).toBeNull(), + ); + + // The rejected exchange isn't reused: a remount reports the backend's error, + // not "already used". + renderWithProviders(, { + providers: ["auth"], + handlers: [ + http.post("/api/auth/callback", () => + HttpResponse.json({ error: "invalid_grant" }, { status: 400 }), + ), + ], + }); + expect(await screen.findByText("invalid_grant")).toBeInTheDocument(); + }); + + it("shows an error instead of spinning when the code was already used", async () => { + const code = crypto.randomUUID(); + setCallbackLocation(code); + sessionStorage.setItem(`callback_processed_${code}`, "true"); + + renderWithProviders(, { + providers: ["auth"], + handlers: [callbackHandler], + }); + + expect(await screen.findByText("Sign In Failed")).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: /back to login/i }), + ).toBeInTheDocument(); + expect(callbackRequests).toBe(0); + }); +}); diff --git a/frontend/app/auth/callback/page.tsx b/frontend/app/auth/callback/page.tsx index 15209186d..dee80b598 100644 --- a/frontend/app/auth/callback/page.tsx +++ b/frontend/app/auth/callback/page.tsx @@ -1,6 +1,6 @@ "use client"; -import { useMutation, useQueryClient } from "@tanstack/react-query"; +import { useQueryClient } from "@tanstack/react-query"; import { ArrowLeft, CheckCircle, Loader2, XCircle } from "lucide-react"; import { useRouter, useSearchParams } from "next/navigation"; import { @@ -31,6 +31,14 @@ function cleanupOAuthStorage() { localStorage.removeItem("auth_redirect_to"); } +type CallbackResult = { purpose?: string }; + +// One exchange per single-use code, kept after settling so re-runs and remounts +// reuse it instead of POSTing the spent code again. +const exchanges = new Map>(); + +const callbackKeyFor = (code: string) => `callback_processed_${code}`; + function AuthCallbackContent() { const router = useRouter(); const searchParams = useSearchParams(); @@ -69,34 +77,35 @@ function AuthCallbackContent() { ? localStorage.getItem("connecting_connector_type") : null, ); + // Read once: cleanupOAuthStorage() clears auth_redirect_to. + const [appAuthRedirectTo] = useState( + () => + (typeof window !== "undefined" && + localStorage.getItem("auth_redirect_to")) || + searchParams.get("redirect") || + "/chat", + ); + + // Reload / back button: an earlier load spent the code and nothing is in flight. + const [codeAlreadyUsed] = useState( + () => + typeof window !== "undefined" && + !!code && + !exchanges.has(code) && + !!sessionStorage.getItem(callbackKeyFor(code)), + ); const validationError = errorParam ? `OAuth error: ${errorParam}` : !code || !state || !finalConnectorId ? "Missing required parameters for OAuth callback" - : null; + : codeAlreadyUsed + ? "This sign-in link has already been used. Please try again." + : null; - const { mutate: exchangeCode, ...callbackMutation } = useMutation({ - mutationFn: async (params: { - connectionId: string; - code: string; - state: string; - }) => { - const response = await apiClient.post("/auth/callback", { - connection_id: params.connectionId, - authorization_code: params.code, - state: params.state, - }); - const result = response.data; - if (response.status < 200 || response.status >= 300) { - throw new Error(result.error || "Authentication failed"); - } - return result as { purpose?: string }; - }, - onSettled: () => { - queryClient.invalidateQueries({ queryKey: ["connectors"], exact: false }); - }, - }); + const [exchangeResult, setExchangeResult] = useState< + { status: "success" } | { status: "error"; error: string } | null + >(null); useEffect(() => { if (validationError) { @@ -104,10 +113,8 @@ function AuthCallbackContent() { return; } - const callbackKey = `callback_processed_${code}`; - - if (sessionStorage.getItem(callbackKey)) return; - sessionStorage.setItem(callbackKey, "true"); + const callbackKey = callbackKeyFor(code!); + let active = true; let parsedConnectionId = finalConnectorId!; @@ -135,79 +142,109 @@ function AuthCallbackContent() { } } - exchangeCode( - { connectionId: parsedConnectionId, code: code!, state: state! }, - { - onSuccess: async (result) => { - if (result.purpose === "app_auth" || purpose === "app_auth") { - await refreshAuth(); + let exchange = exchanges.get(code!); + if (!exchange) { + sessionStorage.setItem(callbackKey, "true"); + exchange = apiClient + .post("/auth/callback", { + connection_id: parsedConnectionId, + authorization_code: code, + state, + }) + .then((response) => { + const result = response.data; + if (response.status < 200 || response.status >= 300) { + throw new Error(result.error || "Authentication failed"); + } + return result as CallbackResult; + }) + .catch((error: unknown) => { + // Runs even if every page instance has unmounted, so a failed code + // isn't later reported as "already used". + exchanges.delete(code!); + sessionStorage.removeItem(callbackKey); + throw error; + }) + .finally(() => { + queryClient.invalidateQueries({ + queryKey: ["connectors"], + exact: false, + }); + }); + exchanges.set(code!, exchange); + } - const redirectTo = - localStorage.getItem("auth_redirect_to") || - searchParams.get("redirect") || - "/chat"; + exchange.then( + async (result) => { + if (!active) return; + setExchangeResult({ status: "success" }); + if (result.purpose === "app_auth" || purpose === "app_auth") { + await refreshAuth(); + if (!active) return; - cleanupOAuthStorage(); - redirectTimeoutRef.current = setTimeout( - () => router.push(redirectTo), - 2000, - ); - } else if (result.purpose === "test" || purpose === "test") { - // Test Connection — token exchange succeeded, credentials work. No - // connection was persisted (see _handle_test_auth); send the admin - // back to Connector Settings with a success indicator to toast. - cleanupOAuthStorage(); - localStorage.removeItem("test_connection_return_tab"); - redirectTimeoutRef.current = setTimeout( - () => - router.push( - `/settings/connector-access?oauth_test=success&credential=${encodeURIComponent(storedConnectorType || "")}`, - ), - 1000, - ); - } else { - cleanupOAuthStorage(); - redirectTimeoutRef.current = setTimeout( - () => router.push("/settings?oauth_success=true"), - 2000, - ); - } - }, - onError: () => { + cleanupOAuthStorage(); + redirectTimeoutRef.current = setTimeout( + () => router.push(appAuthRedirectTo), + 2000, + ); + } else if (result.purpose === "test" || purpose === "test") { + // Test Connection — token exchange succeeded, credentials work. No + // connection was persisted (see _handle_test_auth); send the admin + // back to Connector Settings with a success indicator to toast. cleanupOAuthStorage(); localStorage.removeItem("test_connection_return_tab"); - sessionStorage.removeItem(callbackKey); - }, + redirectTimeoutRef.current = setTimeout( + () => + router.push( + `/settings/connector-access?oauth_test=success&credential=${encodeURIComponent(storedConnectorType || "")}`, + ), + 1000, + ); + } else { + cleanupOAuthStorage(); + redirectTimeoutRef.current = setTimeout( + () => router.push("/settings?oauth_success=true"), + 2000, + ); + } + }, + (error: unknown) => { + if (!active) return; + setExchangeResult({ + status: "error", + error: + error instanceof Error ? error.message : "Authentication failed", + }); + cleanupOAuthStorage(); + localStorage.removeItem("test_connection_return_tab"); }, ); - return () => clearTimeout(redirectTimeoutRef.current); + return () => { + active = false; + clearTimeout(redirectTimeoutRef.current); + }; }, [ code, state, finalConnectorId, storedConnectorType, validationError, - searchParams, + appAuthRedirectTo, isIbmAuthMode, - exchangeCode, + queryClient, refreshAuth, purpose, router, ]); - const status: "processing" | "success" | "error" = - validationError || callbackMutation.isError - ? "error" - : callbackMutation.isSuccess - ? "success" - : "processing"; + const status: "processing" | "success" | "error" = validationError + ? "error" + : (exchangeResult?.status ?? "processing"); const error = validationError || - (callbackMutation.error instanceof Error - ? callbackMutation.error.message - : null); + (exchangeResult?.status === "error" ? exchangeResult.error : null); const isAppAuth = mounted && purpose === "app_auth"; const isTest = mounted && purpose === "test";