diff --git a/.changeset/oauth-popup-ownership.md b/.changeset/oauth-popup-ownership.md new file mode 100644 index 000000000..c8514511a --- /dev/null +++ b/.changeset/oauth-popup-ownership.md @@ -0,0 +1,14 @@ +--- +"@crossmint/client-sdk-react-ui": patch +--- + +Fixes the OAuth popup flow losing logins when a second provider is clicked. + +All providers share one named popup (`PopupWindow.initEmpty` opens `window.open(..., "popupWindow")`), so a second click wraps the window a first flow is still using in a client of its own. The flow resumes at three points after handing that popup over: the URL await, the auth-material refresh await, and the 2.5s closure poller. Each flow now claims the popup and re-checks the claim at every one of them, so a superseded flow can no longer close the popup, publish its error, or clear the loading state belonging to the flow that replaced it. + +Two teardown-ordering fixes come with it: + +- Listener teardown happens when a new flow takes the popup over, not at the top of the call. Doing it earlier left a still-live popup with nothing listening across the URL await, dropping a login that had already completed. +- The closure poller no longer removes listeners. The callback page posts its one-time secret and then closes itself, so a tick landing in that gap dropped the material. Takeover and unmount both remove them, so at most one pair is ever alive. + +Unmounting mid-flow now closes the popup it opened, instead of leaving a window on screen that nothing will ever close. diff --git a/packages/client/ui/react-ui/src/hooks/use-oauth-window-listener.test.ts b/packages/client/ui/react-ui/src/hooks/use-oauth-window-listener.test.ts index 56a9a5aff..daf815fa6 100644 --- a/packages/client/ui/react-ui/src/hooks/use-oauth-window-listener.test.ts +++ b/packages/client/ui/react-ui/src/hooks/use-oauth-window-listener.test.ts @@ -1,44 +1,78 @@ -import { act, renderHook } from "@testing-library/react"; +import { act, cleanup, renderHook, waitFor } from "@testing-library/react"; import { afterEach, describe, expect, test, vi } from "vitest"; import type { OAuthProvider } from "@crossmint/common-sdk-auth"; +const getOAuthUrl = vi.fn(); const handleRefreshAuthMaterial = vi.fn(); vi.mock("@/hooks", () => ({ - useCrossmintAuth: () => ({ crossmintAuth: { handleRefreshAuthMaterial } }), + useCrossmintAuth: () => ({ crossmintAuth: { getOAuthUrl, handleRefreshAuthMaterial } }), })); import { useOAuthWindowListener } from "./useOAuthWindowListener"; -const OAUTH_URL_MAP = { google: "https://www.crossmint.com/auth/oauth/google" } as Record; +// No prefetched URLs, so every click awaits getOAuthUrl. +const NO_PREFETCHED_URLS = { google: "", twitter: "" } as Record; +const PREFETCHED_URLS = { + google: "https://oauth.example/google", + twitter: "https://oauth.example/twitter", +} as Record; + +// Kept outside the render callback so a superseded flow's writes are observable, and so the +// hook's useCallback sees the stable identity OAuthFlowProvider gives it in production. +const setError = vi.fn(); function fakeWindow(): Window { - return { postMessage: vi.fn(), close: vi.fn(), closed: false, location: { href: "" } } as unknown as Window; + return { + postMessage: vi.fn(), + close: vi.fn(), + closed: false, + location: { href: "about:blank" }, + } as unknown as Window; +} + +// PopupWindow.initEmpty opens a window by name, so every flow wraps the same popup. +function openPopup(): Window { + const popup = fakeWindow(); + vi.spyOn(window, "open").mockReturnValue(popup); + return popup; +} + +function renderOAuthHook(urls = NO_PREFETCHED_URLS) { + return renderHook(() => useOAuthWindowListener(urls, setError)); } -function deliverAuthMaterial(source: Window) { - const event = new MessageEvent("message", { - data: { event: "authMaterialFromPopupCallback", data: { oneTimeSecret: "one-time-secret" } }, +function deliver(source: Window, event: string, data: unknown) { + const messageEvent = new MessageEvent("message", { + data: { event, data }, origin: "https://www.crossmint.com", }); // jsdom's MessageEvent only accepts a real WindowProxy as `source`, so install the stub directly. - Object.defineProperty(event, "source", { value: source }); - window.dispatchEvent(event); + Object.defineProperty(messageEvent, "source", { value: source }); + window.dispatchEvent(messageEvent); +} + +function deliverAuthMaterial(source: Window, oneTimeSecret = "one-time-secret") { + deliver(source, "authMaterialFromPopupCallback", { oneTimeSecret }); } describe("useOAuthWindowListener", () => { afterEach(() => { + // Before clearing the spies, not after: unmounting tears a live flow down, and testing + // library's auto-cleanup runs late enough that those calls would land in the next test. + cleanup(); vi.restoreAllMocks(); + getOAuthUrl.mockReset(); handleRefreshAuthMaterial.mockClear(); + setError.mockClear(); }); describe("when another window on the page sends the callback", () => { test("takes auth material only from the popup it opened", async () => { - const popup = fakeWindow(); - vi.spyOn(window, "open").mockReturnValue(popup); + const popup = openPopup(); - const { result } = renderHook(() => useOAuthWindowListener(OAUTH_URL_MAP, vi.fn())); + const { result } = renderOAuthHook(PREFETCHED_URLS); await act(async () => { await result.current.createPopupAndSetupListeners("google" as OAuthProvider); }); @@ -54,4 +88,136 @@ describe("useOAuthWindowListener", () => { expect(handleRefreshAuthMaterial).toHaveBeenCalledWith("one-time-secret"); }); }); + + describe("when a second provider is clicked while the first URL is still resolving", () => { + test("leaves one listener pair on the shared popup, so the secret is redeemed once", async () => { + const popup = openPopup(); + const resolvers: Array<(url: string) => void> = []; + getOAuthUrl.mockImplementation(() => new Promise((resolve) => resolvers.push(resolve))); + + const { result } = renderOAuthHook(); + + await act(async () => { + result.current.createPopupAndSetupListeners("google" as OAuthProvider); + result.current.createPopupAndSetupListeners("twitter" as OAuthProvider); + await waitFor(() => expect(resolvers).toHaveLength(2)); + resolvers[0]("https://oauth.example/google"); + resolvers[1]("https://oauth.example/twitter"); + }); + + await waitFor(() => expect(popup.location.href).toBe("https://oauth.example/twitter")); + + await act(async () => { + deliverAuthMaterial(popup); + }); + expect(handleRefreshAuthMaterial).toHaveBeenCalledTimes(1); + }); + + test("leaves the popup, the error and the loading state to the flow that took over", async () => { + const popup = openPopup(); + const resolvers: Array<(url: string) => void> = []; + const rejecters: Array<(error: Error) => void> = []; + getOAuthUrl.mockImplementation( + () => + new Promise((resolve, reject) => { + resolvers.push(resolve); + rejecters.push(reject); + }) + ); + + const { result } = renderOAuthHook(); + + await act(async () => { + result.current.createPopupAndSetupListeners("google" as OAuthProvider); + result.current.createPopupAndSetupListeners("twitter" as OAuthProvider); + await waitFor(() => expect(resolvers).toHaveLength(2)); + resolvers[1]("https://oauth.example/twitter"); + }); + await act(async () => { + rejecters[0](new Error("google 500")); + }); + + expect(popup.location.href).toBe("https://oauth.example/twitter"); + expect(popup.close).not.toHaveBeenCalled(); + expect(setError).not.toHaveBeenCalledWith("google 500"); + expect(result.current.activeOAuthProvider).toBe("twitter"); + }); + }); + + describe("when a second provider is clicked after the first flow is fully set up", () => { + test("drops the first flow's listeners, so the secret is redeemed once", async () => { + const popup = openPopup(); + const { result } = renderOAuthHook(PREFETCHED_URLS); + + await act(async () => { + await result.current.createPopupAndSetupListeners("google" as OAuthProvider); + }); + await act(async () => { + await result.current.createPopupAndSetupListeners("twitter" as OAuthProvider); + }); + + await act(async () => { + deliverAuthMaterial(popup); + }); + + expect(handleRefreshAuthMaterial).toHaveBeenCalledTimes(1); + expect(popup.location.href).toBe("https://oauth.example/twitter"); + }); + }); + + describe("when the popup reports auth material", () => { + test("redeems the secret, then closes the popup and clears the loading state", async () => { + const popup = openPopup(); + const { result } = renderOAuthHook(PREFETCHED_URLS); + + await act(async () => { + await result.current.createPopupAndSetupListeners("google" as OAuthProvider); + }); + await act(async () => { + deliverAuthMaterial(popup, "secret"); + }); + + expect(handleRefreshAuthMaterial).toHaveBeenCalledWith("secret"); + expect(popup.close).toHaveBeenCalled(); + expect(result.current.activeOAuthProvider).toBeNull(); + }); + }); + + describe("when the provider unmounts mid-flow", () => { + test("closes the popup it opened and stops redeeming what arrives after", async () => { + const popup = openPopup(); + const { result, unmount } = renderOAuthHook(PREFETCHED_URLS); + + await act(async () => { + await result.current.createPopupAndSetupListeners("google" as OAuthProvider); + }); + unmount(); + + expect(popup.close).toHaveBeenCalled(); + + await act(async () => { + deliverAuthMaterial(popup); + }); + expect(handleRefreshAuthMaterial).not.toHaveBeenCalled(); + }); + + test("does not publish an error for the flow it abandoned", async () => { + openPopup(); + const rejecters: Array<(error: Error) => void> = []; + getOAuthUrl.mockImplementation(() => new Promise((_, reject) => rejecters.push(reject))); + + const { result, unmount } = renderOAuthHook(); + + await act(async () => { + result.current.createPopupAndSetupListeners("google" as OAuthProvider); + await waitFor(() => expect(rejecters).toHaveLength(1)); + }); + unmount(); + await act(async () => { + rejecters[0](new Error("google 500")); + }); + + expect(setError).not.toHaveBeenCalledWith("google 500"); + }); + }); }); diff --git a/packages/client/ui/react-ui/src/hooks/useOAuthWindowListener.ts b/packages/client/ui/react-ui/src/hooks/useOAuthWindowListener.ts index f0ebdd982..9aa809091 100644 --- a/packages/client/ui/react-ui/src/hooks/useOAuthWindowListener.ts +++ b/packages/client/ui/react-ui/src/hooks/useOAuthWindowListener.ts @@ -1,4 +1,4 @@ -import { useState, useCallback } from "react"; +import { useEffect, useRef, useState, useCallback } from "react"; import type { OAuthProvider } from "@crossmint/common-sdk-auth"; import { PopupWindow } from "@crossmint/client-sdk-window"; import { useCrossmintAuth } from "@/hooks"; @@ -10,9 +10,33 @@ export const useOAuthWindowListener = (oauthUrlMap: OAuthUrlMap, setError: (erro const { crossmintAuth } = useCrossmintAuth(); // Track which OAuth provider's window is currently being interacted with const [activeOAuthProvider, setActiveOAuthProvider] = useState(null); + const cleanupRef = useRef<(() => void) | null>(null); + const popupRef = useRef | null>(null); + // Every click claims the next id. Because all flows share one named popup, each resumption + // point below has to check it still holds the claim before touching the popup or the state. + const flowIdRef = useRef(0); + const mountedRef = useRef(true); + + useEffect(() => { + mountedRef.current = true; + + return () => { + mountedRef.current = false; + cleanupRef.current?.(); + cleanupRef.current = null; + // Nothing is left to adopt this popup, so abort the flow instead of leaving a window + // on screen that no longer has a listener to close it. + popupRef.current?.window?.close(); + popupRef.current = null; + }; + }, []); const createPopupAndSetupListeners = useCallback( async (provider: OAuthProvider, providerLoginHint?: string) => { + // Claim the flow before the first await. + const flowId = ++flowIdRef.current; + const ownsPopup = () => flowIdRef.current === flowId && mountedRef.current; + setActiveOAuthProvider(provider); setError(null); @@ -27,6 +51,7 @@ export const useOAuthWindowListener = (oauthUrlMap: OAuthUrlMap, setError: (erro height: 700, incomingEvents, }); + popupRef.current = popup; const prefetchedUrl = oauthUrlMap[provider]; const resolvedUrl = prefetchedUrl || (await crossmintAuth?.getOAuthUrl(provider)); @@ -35,9 +60,17 @@ export const useOAuthWindowListener = (oauthUrlMap: OAuthUrlMap, setError: (erro } baseUrl = new URL(resolvedUrl); } catch (e) { - popup?.window?.close(); - setActiveOAuthProvider(null); - setError(e instanceof Error ? e.message : "Failed to start OAuth login"); + if (ownsPopup()) { + popup?.window?.close(); + setActiveOAuthProvider(null); + setError(e instanceof Error ? e.message : "Failed to start OAuth login"); + } + return; + } + + // PopupWindow.initEmpty opens a named window, so the later click reused this + // popup. Leave it to that flow rather than closing it or navigating it again. + if (!ownsPopup()) { return; } @@ -59,26 +92,33 @@ export const useOAuthWindowListener = (oauthUrlMap: OAuthUrlMap, setError: (erro }); } + // Drop the previous flow's listeners only now that this one is taking the popup over. + // Doing it before the await above would leave its still-live popup with nothing listening. + cleanupRef.current?.(); + if (popup.window != null) { popup.window.location.href = baseUrl.toString(); } // Listen on the popup itself: it is the window that sends these events, and its - // transport drops anything from another sender. - let stopListening = () => { - // reassigned below, once there is something to unsubscribe - }; - + // transport drops anything from another sender. Every flow wraps the same reused + // window in its own client, so the takeover above is what keeps one pair alive. const handleAuthMaterial = async (data: { oneTimeSecret: string }) => { await crossmintAuth?.handleRefreshAuthMaterial(data.oneTimeSecret); - stopListening(); + if (!ownsPopup()) { + return; + } + cleanup(); popup.window?.close(); setActiveOAuthProvider(null); }; const handleError = (data: { error: string }) => { + if (!ownsPopup()) { + return; + } setError(data.error); - stopListening(); + cleanup(); popup.window?.close(); setActiveOAuthProvider(null); }; @@ -87,18 +127,24 @@ export const useOAuthWindowListener = (oauthUrlMap: OAuthUrlMap, setError: (erro const errorListenerId = popup.on("errorFromPopupCallback", handleError); // Add a check for manual window closure // Ideally we should find a more explicit way of doing this, but I think this is fine for now. + // The listeners deliberately stay registered: a callback page posts its secret and then closes + // itself, so tearing them down here would drop material that is already in flight. Takeover + // and unmount both remove them, so at most one pair is ever alive. const checkWindowClosure = setInterval(() => { if (popup.window?.closed) { - stopListening(); - setActiveOAuthProvider(null); + clearInterval(checkWindowClosure); + if (ownsPopup()) { + setActiveOAuthProvider(null); + } } }, 2500); // Check every 2.5 seconds - stopListening = () => { + const cleanup = () => { + clearInterval(checkWindowClosure); popup.off(authMaterialListenerId); popup.off(errorListenerId); - clearInterval(checkWindowClosure); }; + cleanupRef.current = cleanup; }, [oauthUrlMap, crossmintAuth, setError] );