From 554f36265e19d61d0e06b4f76ce233a979367eba Mon Sep 17 00:00:00 2001 From: callumalpass Date: Sun, 11 Oct 2026 09:48:36 +1100 Subject: [PATCH] Close the owned Connect popup after consent completes --- apps/reader/src/NativeSignIn.tsx | 19 +++++- apps/reader/src/connect-popup.test.ts | 86 ++++++++++++++++++++++++ apps/reader/src/connect-popup.ts | 67 ++++++++++++++++++ apps/reader/src/native-signin-ui.test.ts | 22 ++++-- apps/reader/src/native-signin-ui.ts | 47 ++++++------- docs/new-sdk.md | 11 ++- 6 files changed, 214 insertions(+), 38 deletions(-) create mode 100644 apps/reader/src/connect-popup.test.ts create mode 100644 apps/reader/src/connect-popup.ts diff --git a/apps/reader/src/NativeSignIn.tsx b/apps/reader/src/NativeSignIn.tsx index 67404d4a..e9654b4b 100644 --- a/apps/reader/src/NativeSignIn.tsx +++ b/apps/reader/src/NativeSignIn.tsx @@ -48,6 +48,15 @@ function observeForeground( }; } +const portalCloseStatuses = new Set([ + "unselected", + "opening", + "setup_required", + "ready", + "blocked", + "closed", +]); + function NativeSignInView({ binding, onSignedOut }: NativeSignInProps): JSX.Element { const { session, portal } = binding; const snapshot = useSyncExternalStore( @@ -70,9 +79,17 @@ function NativeSignInView({ binding, onSignedOut }: NativeSignInProps): JSX.Elem return () => { mounted.current = false; stop(); + portal.cancel(); // Borrowed SDK session: its actual owner handles unmount/HMR shutdown. }; - }, [session]); + }, [session, portal]); + + useEffect(() => { + // An already paired account may still be authorizing additional consent. + if (portalCloseStatuses.has(snapshot.status)) { + portal.cancel(); + } + }, [portal, snapshot.status]); const act = async (action: () => Promise): Promise => { if (busy.current) { diff --git a/apps/reader/src/connect-popup.test.ts b/apps/reader/src/connect-popup.test.ts new file mode 100644 index 00000000..bb233f53 --- /dev/null +++ b/apps/reader/src/connect-popup.test.ts @@ -0,0 +1,86 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { ConnectPopupBlocked, openConnectPopup } from "./connect-popup.js"; +import { createSignInPortalUi } from "./native-signin-ui.js"; + +afterEach(() => vi.unstubAllGlobals()); + +function handle(): { + opener: object | null; + closed: boolean; + location: { href: string }; + close: ReturnType; +} { + return { opener: {}, closed: false, location: { href: "" }, close: vi.fn() }; +} + +describe("UI-owned popup handles, not native authentication", () => { + it("uses a detached fallback tab if the popup throws", () => { + const tab = handle(); + const open = vi + .fn() + .mockImplementationOnce(() => { + throw new Error("blocked"); + }) + .mockReturnValue(tab); + vi.stubGlobal("window", { open }); + const portal = openConnectPopup(); + expect(open).toHaveBeenLastCalledWith("", "_blank"); + expect(tab.opener).toBeNull(); + portal.navigate("https://mdbase.test/approve"); + expect(tab.location.href).toBe("https://mdbase.test/approve"); + }); + it("reports an actually blocked popup and fallback", () => { + vi.stubGlobal("window", { open: () => null }); + expect(openConnectPopup).toThrow(ConnectPopupBlocked); + }); + it("closes idempotently and refuses late navigation of a disposed handle", () => { + const popup = handle(); + vi.stubGlobal("window", { open: () => popup }); + const portal = openConnectPopup(); + portal.close(); + portal.close(); + expect(popup.close).toHaveBeenCalledOnce(); + expect(() => portal.navigate("https://mdbase.test/late")).toThrow("Sign-in window closed."); + }); + it("keeps the original handle after SDK handoff and reuses it for reopen", () => { + const popup = handle(); + const open = vi.fn(() => popup); + vi.stubGlobal("window", { open }); + const ui = createSignInPortalUi(); + ui.reserve(); + const original = ui.open(); + ui.reserve(); + expect(ui.open()).toBe(original); + expect(open).toHaveBeenCalledOnce(); + ui.cancel(); + ui.cancel(); + expect(popup.close).toHaveBeenCalledOnce(); + }); + it("reopens a manually closed popup without sharing a retired owner's name", () => { + const first = handle(); + const second = handle(); + const open = vi.fn().mockReturnValueOnce(first).mockReturnValue(second); + vi.stubGlobal("window", { open }); + const ui = createSignInPortalUi(); + ui.reserve(); + const original = ui.open(); + first.closed = true; + ui.reserve(); + expect(ui.open()).not.toBe(original); + createSignInPortalUi().reserve(); + expect(open.mock.calls[0]?.[1]).toBe(open.mock.calls[1]?.[1]); + expect(open.mock.calls[2]?.[1]).not.toBe(open.mock.calls[0]?.[1]); + }); + it("does not turn display cleanup failure into an approval error", () => { + const popup = handle(); + popup.close.mockImplementation(() => { + throw new Error("display cleanup"); + }); + vi.stubGlobal("window", { open: () => popup }); + const portal = openConnectPopup(); + expect(() => portal.close()).not.toThrow(); + portal.close(); + expect(popup.close).toHaveBeenCalledOnce(); + }); +}); diff --git a/apps/reader/src/connect-popup.ts b/apps/reader/src/connect-popup.ts new file mode 100644 index 00000000..3e2d6270 --- /dev/null +++ b/apps/reader/src/connect-popup.ts @@ -0,0 +1,67 @@ +import type { AppWebSignInPortal } from "@mdbase-dev/sdk/app-host"; + +export class ConnectPopupBlocked extends Error { + constructor() { + super("Your browser blocked the sign-in tab."); + } +} + +export interface ConnectPopup extends AppWebSignInPortal { + readonly closed: boolean; +} + +/** UI-only capability: callers pass only the SDK-validated Connect URI. + * Keep the production popup opener so its parent can close it across origins. + * The tab fallback detaches its opener before navigation. */ +export function openConnectPopup(name = "mdbase-connect-authorization"): ConnectPopup { + let popup: Window | null = null; + try { + popup = window.open("", name, "popup,width=620,height=760"); + } catch { + /* Try a tab. */ + } + if (!popup) { + try { + popup = window.open("", "_blank"); + // A literal noopener feature returns no usable blank handle. Detach + // immediately instead, before the SDK supplies the navigation URI. + if (popup) { + popup.opener = null; + } + } catch { + try { + popup?.close(); + } catch { + /* Display cleanup only. */ + } + popup = null; + } + } + if (!popup) { + throw new ConnectPopupBlocked(); + } + const original = popup; + let closed = false; + return { + get closed(): boolean { + return closed || original.closed; + }, + navigate(uri: string): void { + if (closed || original.closed) { + throw new Error("Sign-in window closed."); + } + original.location.href = uri; + }, + close(): void { + if (closed) { + return; + } + closed = true; + try { + original.close(); + } catch { + /* Display cleanup does not undo approval. */ + } + }, + }; +} diff --git a/apps/reader/src/native-signin-ui.test.ts b/apps/reader/src/native-signin-ui.test.ts index c86a7332..6074169d 100644 --- a/apps/reader/src/native-signin-ui.test.ts +++ b/apps/reader/src/native-signin-ui.test.ts @@ -114,17 +114,25 @@ describe("native sign-in presentation, not an authentication engine", () => { expect(view.message).toBe("Your browser blocked the sign-in tab."); expect(view.action).toBe("Open sign-in"); }); - it("opens a tab in the UI gesture, detaches the opener, and delegates only navigation", () => { - const tab = { opener: {}, location: { replace: vi.fn() }, close: vi.fn() }; - vi.stubGlobal("window", { open: vi.fn(() => tab) }); +}); + +describe("public UI popup and unavailable collection boundary", () => { + it("opens the named production popup synchronously with its trusted opener", () => { + const opener = {}; + const popup = { opener, closed: false, location: { href: "" }, close: vi.fn() }; + vi.stubGlobal("window", { open: vi.fn(() => popup) }); try { const portal = openSignInPortal(); - expect(window.open).toHaveBeenCalledWith("about:blank", "_blank"); - expect(tab.opener).toBeNull(); + expect(window.open).toHaveBeenCalledWith( + "", + "mdbase-connect-authorization", + "popup,width=620,height=760", + ); + expect(popup.opener).toBe(opener); portal.navigate("https://mdbase.test/approve"); - expect(tab.location.replace).toHaveBeenCalledWith("https://mdbase.test/approve"); + expect(popup.location.href).toBe("https://mdbase.test/approve"); portal.close(); - expect(tab.close).toHaveBeenCalledOnce(); + expect(popup.close).toHaveBeenCalledOnce(); } finally { vi.unstubAllGlobals(); } diff --git a/apps/reader/src/native-signin-ui.ts b/apps/reader/src/native-signin-ui.ts index a12882a6..b4bd942b 100644 --- a/apps/reader/src/native-signin-ui.ts +++ b/apps/reader/src/native-signin-ui.ts @@ -1,10 +1,13 @@ import { AppWebSignInSession } from "@mdbase-dev/sdk/app-host"; +import { openConnectPopup, type ConnectPopup } from "./connect-popup.js"; + import type { AppWebSignInSessionOptions, AppWebSignInSnapshot, AppWebSignInPortal, } from "@mdbase-dev/sdk/app-host"; +export { ConnectPopupBlocked as SignInTabBlocked } from "./connect-popup.js"; export type NativeSignInSession = Pick< AppWebSignInSession, @@ -27,47 +30,35 @@ export const signInCopy = Object.freeze({ signOut: "Sign out", unavailable: "Opening collections isn't available in this build yet.", }); -export class SignInTabBlocked extends Error { - constructor() { - super(signInCopy.tabBlocked); - } -} - /** UI portal only. Navigation URI comes from the SDK. */ -export function openSignInPortal(): AppWebSignInPortal { - const tab = window.open("about:blank", "_blank"); - if (!tab) { - throw new SignInTabBlocked(); - } - tab.opener = null; - return { - navigate(uri): void { - tab.location.replace(uri); - }, - close(): void { - tab.close(); - }, - }; -} +export const openSignInPortal = openConnectPopup; + +// Popup names are UI ownership identities, never native IDs or namespaces. +let nextPortalKey = 0; export interface SignInPortalUi { reserve: () => void; open: () => AppWebSignInPortal; cancel: () => void; } export function createSignInPortalUi(): SignInPortalUi { - let reserved: AppWebSignInPortal | null = null; + const name = `mdbase-reader-connect-${String(++nextPortalKey)}`; + let active: ConnectPopup | null = null; return { reserve(): void { - reserved ??= openSignInPortal(); + if (!active || active.closed) { + active = openSignInPortal(name); + } }, open(): AppWebSignInPortal { - const portal = reserved ?? openSignInPortal(); - reserved = null; - return portal; + if (!active || active.closed) { + active = openSignInPortal(name); + } + return active; }, cancel(): void { - reserved?.close(); - reserved = null; + const original = active; + active = null; + original?.close(); }, }; } diff --git a/docs/new-sdk.md b/docs/new-sdk.md index c9279f6c..b0571363 100644 --- a/docs/new-sdk.md +++ b/docs/new-sdk.md @@ -82,8 +82,15 @@ SDK's actual `AppWebSignInSession`. `createNativeSignIn(options)` requires expli owner-provided release, app identity, origins, runtime, locks and original signal; it does not guess them or replace the default Connect/extension runtimes. -Sign in reserves the tab in the user gesture before asynchronous SDK start or -expiry renewal. The SDK owns approval detection and protected state. The UI uses +Sign in synchronously reserves a named 620×760 Connect popup before SDK start +or expiry renewal. The trusted popup keeps its production opener; the tab +fallback detaches its opener before SDK-validated navigation. No secrets, +postMessage approval or storage callback use the opener. The UI retains its +original handle after handoff and closes on completed/terminal public statuses, +never authorizing additional consent even when already paired. View teardown +closes only its owned popup. A detached fallback tab is not claimed parent-closeable +across origins. This follows TaskNotes' app-owned pattern without an SDK repin. +The SDK owns approval detection and protected state. The UI uses canonical status/email, reopen-tab, popup-blocked and interrupted wording. Sign-in and secondary actions reuse Reader's existing shared control styles, including usable touch targets at 390px. It