From dd6c47cb7896bb9cc3b40d93bec895b196277d9f Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:17:03 -0700 Subject: [PATCH] Reserve the OAuth sign-in window on the click, not after discovery The transparent DCR/CIMD connects and OAuth reconnect opened the popup only after their setup round trips answered. window.open needs transient user activation, which expires a few seconds after the click, so a slow API meant the browser refused the window and the connect ended silently. Claim the window on the click and navigate it when the authorization URL arrives; close it on the paths that end without signing in, and on cancel and unmount. Report a refused window instead of swallowing it, and render the sign-in error above the footer where the automatic flows can show it. --- .changeset/oauth-popup-reserve-on-click.md | 9 + e2e/selfhost/mcp-oauth-slow-connect.test.ts | 132 +++++++++ .../react/src/components/accounts-section.tsx | 8 + .../src/components/add-account-modal.test.ts | 279 +++++++++++++++++- .../src/components/add-account-modal.tsx | 106 +++++-- packages/react/src/plugins/oauth-sign-in.tsx | 110 +++++-- 6 files changed, 593 insertions(+), 51 deletions(-) create mode 100644 .changeset/oauth-popup-reserve-on-click.md create mode 100644 e2e/selfhost/mcp-oauth-slow-connect.test.ts diff --git a/.changeset/oauth-popup-reserve-on-click.md b/.changeset/oauth-popup-reserve-on-click.md new file mode 100644 index 0000000000..cf8d9430a5 --- /dev/null +++ b/.changeset/oauth-popup-reserve-on-click.md @@ -0,0 +1,9 @@ +--- +"@executor-js/react": patch +--- + +**A slow OAuth discovery no longer kills the connect with no popup and no error** + +The transparent connect flows opened the sign-in window only after their setup round trips had answered: DCR after probe and dynamic registration, CIMD after minting the client, reconnect after starting the session. `window.open` needs transient user activation, which browsers expire a few seconds after the click, so once the API was slow enough the browser refused the window and the connect ended with nothing on screen but the button returning to "Connect". Every MCP integration takes that path. + +The window is now claimed on the click itself and navigated when the authorization URL arrives, however long that takes, and it is closed again on the paths that end without signing in (failed probe, no registration endpoint, rejected registration, failed client mint) as well as on cancel and unmount. A window the browser does refuse is now reported instead of swallowed: the flows stop before their round trips, and the sign-in error renders above the dialog footer, where the automatic flows can actually show it, rather than inside a method tab panel they never render. diff --git a/e2e/selfhost/mcp-oauth-slow-connect.test.ts b/e2e/selfhost/mcp-oauth-slow-connect.test.ts new file mode 100644 index 0000000000..6a7b7fd9d5 --- /dev/null +++ b/e2e/selfhost/mcp-oauth-slow-connect.test.ts @@ -0,0 +1,132 @@ +// Selfhost browser coverage for the transparent DCR connect when OAuth +// discovery is slow, the shape behind the report: "When I hit connect, it loads +// for a second, but then theres no user feedback and nothing happens." +// +// One click runs probe -> register -> start. The first two are network round +// trips, and `window.open` needs transient user activation, which a real +// browser expires a few seconds after the click. The shipped code opened the +// sign-in window AFTER both round trips, so once the API was slow the browser +// refused it and the connect died silently. The window is now claimed on the +// click and navigated later, so discovery latency no longer decides whether the +// user can connect. +// +// What this scenario guards: the whole slow path still works end to end, with +// the reservation threaded from the click through registration to a window that +// really lands on the discovered authorization server. +// +// What it CANNOT guard, and why: Playwright drives Chromium in automation mode, +// which never enforces the activation rule for `window.open` (verified headed +// and headless, and with `--disable-popup-blocking` removed via +// ignoreDefaultArgs). So a browser here opens the window no matter how stale +// the click is, and this scenario passes against the pre-fix ordering too. The +// ordering itself is pinned by unit tests over `runDcrConnect` in +// packages/react/src/components/add-account-modal.test.ts, which do fail when +// the reservation moves back after the round trips. See LEARNINGS.md. +import { randomBytes } from "node:crypto"; + +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { deriveMcpNamespace } from "@executor-js/plugin-mcp"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; +import { makeGreetingMcpServer, serveMcpServerWithOAuth } from "@executor-js/plugin-mcp/testing"; +import { IntegrationSlug } from "@executor-js/sdk/shared"; +import { OAuthTestServer } from "@executor-js/sdk/testing"; + +import { scenario } from "../src/scenario"; +import { Api, Browser, Target } from "../src/services"; + +const api = composePluginApi([mcpHttpPlugin()] as const); + +// Comfortably past Chromium's ~5s transient user activation once both land on +// the same click, and well under the step timeouts below. +const STALL_MS = 3_500; + +const isDiscoveryCall = (url: string): boolean => + url.includes("/api/oauth/probe") || url.includes("/api/oauth/clients/register-dynamic"); + +scenario( + "MCP OAuth · a slow discovery round trip still opens the sign-in window", + { timeout: 240_000 }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const { client: makeApiClient } = yield* Api; + const oauth = yield* OAuthTestServer; + const server = yield* serveMcpServerWithOAuth( + () => makeGreetingMcpServer({ name: "slow-connect-mcp" }), + { path: "/mcp" }, + ); + const identity = yield* target.newIdentity(); + const client = yield* makeApiClient(api, identity); + const displayName = `Slow MCP ${randomBytes(3).toString("hex")}`; + const slug = IntegrationSlug.make(deriveMcpNamespace({ name: displayName })); + + yield* Effect.gen(function* () { + yield* browser.session(identity, async ({ page, step }) => { + await step("Add an OAuth-protected MCP integration", async () => { + const addUrl = new URL("/integrations/add/mcp", target.baseUrl); + addUrl.searchParams.set("url", server.endpoint); + await page.goto(addUrl.toString(), { waitUntil: "networkidle" }); + await page.getByText("How does this server authenticate?").waitFor({ timeout: 30_000 }); + await page.getByPlaceholder("e.g. Linear").fill(displayName); + await page.getByRole("button", { name: "Add integration" }).click(); + await page.waitForURL(/\/integrations\/(?!add\b)[^/?]+$/, { timeout: 30_000 }); + await page.getByText("Connections").first().waitFor(); + }); + + await step("Make OAuth discovery slow, the way a degraded API is", async () => { + // Delay the responses rather than the requests, so the app sees a + // genuinely slow API and not a stalled network stack. + await page.route( + (url) => isDiscoveryCall(url.href), + async (route) => { + await new Promise((resolve) => setTimeout(resolve, STALL_MS)); + await route.continue(); + }, + ); + }); + + await step("Connect, and wait out the slow discovery", async () => { + await page.getByRole("button", { name: "Add connection" }).first().click(); + await page.getByRole("heading", { name: /Add connection/ }).waitFor(); + await page.getByRole("tab", { name: "OAuth" }).waitFor(); + + const popupPromise = page.waitForEvent("popup", { timeout: 60_000 }); + await page.getByRole("button", { name: "Connect", exact: true }).click(); + + // In a real browser the two stalls outlast the click's user + // activation, so this popup only exists because it was reserved on + // the click. Automation-mode Chromium would open it either way; + // the ordering is pinned by the unit tests named above. + const popup = await popupPromise; + await popup.waitForURL((url) => url.origin === new URL(oauth.issuerUrl).origin, { + timeout: 60_000, + }); + await popup.waitForLoadState("domcontentloaded", { timeout: 30_000 }); + expect( + new URL(popup.url()).origin, + "the reserved window reached the discovered authorization host", + ).toBe(new URL(oauth.authorizationEndpoint).origin); + await popup.close(); + }); + }); + + const oauthRequests = yield* oauth.requests; + expect( + oauthRequests.some( + (request) => request.method === "POST" && request.path === "/register", + ), + "the slow connect still dynamically registered its OAuth client", + ).toBe(true); + expect( + oauthRequests.some( + (request) => request.method === "GET" && request.path === "/authorize", + ), + "the slow connect still reached the authorize endpoint", + ).toBe(true); + }).pipe(Effect.ensuring(client.mcp.removeServer({ params: { slug } }).pipe(Effect.ignore))); + }), + ).pipe(Effect.provide(OAuthTestServer.layer())), +); diff --git a/packages/react/src/components/accounts-section.tsx b/packages/react/src/components/accounts-section.tsx index dddbfbe805..f0ec361dab 100644 --- a/packages/react/src/components/accounts-section.tsx +++ b/packages/react/src/components/accounts-section.tsx @@ -279,6 +279,11 @@ function OwnerAccounts(props: { } const payload = oauthReconnectPayload(connection); if (payload === null) return; + // Claim the sign-in window on the click: `oauth.start` below is a network + // round trip, and the browser's user activation can expire before it + // answers, which would leave Reconnect silently doing nothing. + const reservation = oauthPopup.reserve(); + if (reservation.kind === "blocked") return; // `oauth.start` discriminates the grant: client_credentials mints inline // (`status: "connected"`, no authorization URL) while authorization_code // returns a redirect the popup must complete. The popup hook only handles @@ -291,6 +296,7 @@ function OwnerAccounts(props: { reactivityKeys: connectionWriteKeys, }); if (Exit.isFailure(startExit)) { + oauthPopup.releaseReservation(); toast.error(messageFromExit(startExit, "Failed to reconnect")); trackEvent("connection_reconnected", { integration_slug: String(connection.integration), @@ -301,6 +307,7 @@ function OwnerAccounts(props: { } const started = startExit.value; if (started.status === "connected") { + oauthPopup.releaseReservation(); toast.success("Reconnected"); trackEvent("connection_reconnected", { integration_slug: String(connection.integration), @@ -311,6 +318,7 @@ function OwnerAccounts(props: { } void oauthPopup.openAuthorization({ owner: payload.owner, + reservation, run: () => Promise.resolve({ state: started.state, diff --git a/packages/react/src/components/add-account-modal.test.ts b/packages/react/src/components/add-account-modal.test.ts index acc5952691..b95a3b2aa4 100644 --- a/packages/react/src/components/add-account-modal.test.ts +++ b/packages/react/src/components/add-account-modal.test.ts @@ -9,6 +9,7 @@ import { } from "@executor-js/sdk/shared"; import type { AuthMethod } from "../lib/auth-placements"; +import type { OAuthPopupReservation } from "../plugins/oauth-sign-in"; import { connectionNameFrom, connectionLabel, @@ -57,7 +58,35 @@ type RegisterArgs = { readonly originIntegration?: IntegrationSlug; }; -type StartArgs = { readonly client: OAuthClientSlug; readonly owner: Owner }; +type StartArgs = { + readonly client: OAuthClientSlug; + readonly owner: Owner; + readonly reservation: OAuthPopupReservation; +}; + +// A reservation the browser honoured. The desktop shape is the one that carries +// no `Window`, so the orchestrators' ordering is testable without a DOM. +const RESERVED: OAuthPopupReservation = { + kind: "desktop", + bridge: { openExternal: (): Promise => Promise.resolve() }, +}; + +/** Records the reserve/release calls alongside the network steps, so a test can + * assert the window is claimed BEFORE the first round trip and closed again + * whenever the flow ends without signing in. */ +const popupSpy = (reservation: OAuthPopupReservation = RESERVED) => { + const calls: string[] = []; + return { + calls, + reserve: (): OAuthPopupReservation => { + calls.push("reserve"); + return reservation; + }, + release: (): void => { + calls.push("release"); + }, + }; +}; type CimdCreateArgs = { readonly owner: Owner; @@ -70,7 +99,11 @@ type CimdCreateArgs = { readonly clientSecret: ""; }; -type CimdStartArgs = { readonly client: OAuthClientSlug; readonly owner: Owner }; +type CimdStartArgs = { + readonly client: OAuthClientSlug; + readonly owner: Owner; + readonly reservation: OAuthPopupReservation; +}; const TEST_INTEGRATION = IntegrationSlug.make("linear_mcp"); @@ -310,6 +343,8 @@ describe("runCimdConnect", () => { const outcome = await runCimdConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, createClient: (args: CimdCreateArgs): Promise => { createArgs = args; return Promise.resolve(args.slug); @@ -352,6 +387,8 @@ describe("runCimdConnect", () => { const outcome = await runCimdConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, createClient: (): Promise => { created = true; return Promise.resolve(OAuthClientSlug.make("new-client")); @@ -383,7 +420,221 @@ describe("runCimdConnect", () => { expect(outcome).toEqual({ kind: "started", client: existingSlug, reused: true }); expect(created).toBe(false); - expect(startArgs).toEqual({ client: existingSlug, owner: "user" }); + expect(startArgs).toEqual({ client: existingSlug, owner: "user", reservation: RESERVED }); + }); +}); + +describe("runDcrConnect popup reservation", () => { + const probeOk = (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + registrationEndpoint: "https://auth.example.com/register", + }); + const dcrInput = { + discoveryUrl: "https://mcp.example.com/mcp", + owner: "user" as Owner, + integration: TEST_INTEGRATION, + }; + + // The regression: probe and register are two round trips, and a browser stops + // honouring `window.open` about five seconds after the click. Opening the + // window after them is refused, so the connect ends with no popup, no error, + // and the button back on "Connect". The window has to be claimed up front. + it("claims the sign-in window BEFORE the first network call", async () => { + const popup = popupSpy(); + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => { + popup.calls.push("probe"); + return probeOk(); + }, + register: (): Promise => { + popup.calls.push("register"); + return Promise.resolve(OAuthClientSlug.make("mcp-app")); + }, + start: (): void => { + popup.calls.push("start"); + }, + }, + dcrInput, + ); + + expect(outcome).toEqual({ kind: "started" }); + expect(popup.calls).toEqual(["reserve", "probe", "register", "start"]); + }); + + it("hands the reserved window to start, so the flow navigates it rather than opening a new one", async () => { + let startArgs: StartArgs | null = null; + const outcome = await runDcrConnect( + { + ...popupSpy(), + probe: probeOk, + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (args: StartArgs): void => { + startArgs = args; + }, + }, + dcrInput, + ); + + expect(outcome).toEqual({ kind: "started" }); + expect(startArgs!.reservation).toBe(RESERVED); + }); + + it("gives up before any network call when the browser refuses the window", async () => { + const popup = popupSpy({ kind: "blocked" }); + let probed = false; + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => { + probed = true; + return probeOk(); + }, + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (): void => {}, + }, + dcrInput, + ); + + // No BYO fallback: registering an app by hand cannot open a window either. + expect(outcome).toEqual({ kind: "popup-blocked" }); + expect(probed).toBe(false); + expect(popup.calls).toEqual(["reserve"]); + }); + + // Reserving early means a flow that ends without signing in is holding an + // about:blank window the user never asked for. + it("closes the reserved window when the probe fails", async () => { + const popup = popupSpy(); + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => Promise.resolve(null), + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (): void => {}, + }, + dcrInput, + ); + + expect(outcome).toEqual({ kind: "fallback", reason: "probe-failed" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + it("closes the reserved window when the server advertises no registration endpoint", async () => { + const popup = popupSpy(); + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + }), + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (): void => {}, + }, + dcrInput, + ); + + expect(outcome.kind).toBe("fallback"); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + it("closes the reserved window when registration is rejected", async () => { + const popup = popupSpy(); + const outcome = await runDcrConnect( + { + ...popup, + probe: probeOk, + register: (): Promise<{ readonly error: string }> => + Promise.resolve({ error: "redirect_uri not allowed" }), + start: (): void => {}, + }, + dcrInput, + ); + + expect(outcome).toMatchObject({ reason: "registration-failed" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + it("keeps the reserved window open on the path that signs in", async () => { + const popup = popupSpy(); + await runDcrConnect( + { + ...popup, + probe: probeOk, + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (): void => {}, + }, + dcrInput, + ); + + expect(popup.calls).not.toContain("release"); + }); +}); + +describe("runCimdConnect popup reservation", () => { + const cimdInput = { + owner: "user" as Owner, + integrationName: "PostHog API", + authorizationUrl: "https://us.posthog.com/oauth/authorize/", + tokenUrl: "https://us.posthog.com/oauth/token/", + resource: null, + clientIdMetadataDocumentUrl: "https://executor.example/api/oauth/client-id-metadata.json", + existingClients: [], + }; + + it("claims the sign-in window before minting the client", async () => { + const popup = popupSpy(); + const outcome = await runCimdConnect( + { + ...popup, + createClient: (args: CimdCreateArgs): Promise => { + popup.calls.push("createClient"); + return Promise.resolve(args.slug); + }, + start: (): void => { + popup.calls.push("start"); + }, + }, + cimdInput, + ); + + expect(outcome.kind).toBe("started"); + expect(popup.calls).toEqual(["reserve", "createClient", "start"]); + }); + + it("closes the reserved window when minting the client fails", async () => { + const popup = popupSpy(); + const outcome = await runCimdConnect( + { + ...popup, + createClient: (): Promise => Promise.resolve(null), + start: (): void => {}, + }, + cimdInput, + ); + + expect(outcome).toEqual({ kind: "failed", reason: "create-failed" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + it("never claims a window when the method is missing its endpoints", async () => { + const popup = popupSpy(); + const outcome = await runCimdConnect( + { + ...popup, + createClient: (): Promise => Promise.resolve(null), + start: (): void => {}, + }, + { ...cimdInput, tokenUrl: " " }, + ); + + expect(outcome).toEqual({ kind: "failed", reason: "missing-endpoints" }); + expect(popup.calls).toEqual([]); }); }); @@ -416,7 +667,13 @@ describe("runDcrConnect", () => { }; const outcome = await runDcrConnect( - { probe, register, start }, + { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, + probe, + register, + start, + }, { discoveryUrl: "https://mcp.example.com/mcp", owner: "user", @@ -454,6 +711,8 @@ describe("runDcrConnect", () => { let registerArgs: RegisterArgs | null = null; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ authorizationUrl: "https://auth.example.com/authorize", @@ -490,6 +749,8 @@ describe("runDcrConnect", () => { let registerArgs: RegisterArgs | null = null; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ authorizationUrl: "https://auth.example.com/authorize", @@ -517,6 +778,8 @@ describe("runDcrConnect", () => { const calls: string[] = []; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => { calls.push("probe"); return Promise.resolve({ @@ -554,6 +817,8 @@ describe("runDcrConnect", () => { const calls: string[] = []; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => { calls.push("probe"); return Promise.resolve(null); @@ -580,6 +845,8 @@ describe("runDcrConnect", () => { const calls: string[] = []; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ authorizationUrl: "https://auth.example.com/authorize", @@ -620,6 +887,8 @@ describe("runDcrConnect", () => { let started = false; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ authorizationUrl: "https://auth.example.com/authorize", @@ -659,6 +928,8 @@ describe("runDcrConnect", () => { let registerArgs: RegisterArgs | null = null; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ issuer: "https://auth.example.com", diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 7376a39890..c2a7a5ac98 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -62,6 +62,7 @@ import { oauthClientIdMetadataDocumentUrl, useOAuthPopupFlow, type OAuthCompletionPayload, + type OAuthPopupReservation, } from "../plugins/oauth-sign-in"; import { clientDisplayName, @@ -638,7 +639,17 @@ type CimdCreateClientArgs = { type RunCimdConnectDeps = { readonly createClient: (args: CimdCreateClientArgs) => Promise; - readonly start: (args: { readonly client: OAuthClientSlug; readonly owner: Owner }) => void; + readonly start: (args: CimdStartArgs) => void; + /** Claim the sign-in window before any await. See `useOAuthPopupFlow.reserve`. */ + readonly reserve: () => OAuthPopupReservation; + /** Close a claimed window this flow turned out not to need. */ + readonly release: () => void; +}; + +type CimdStartArgs = { + readonly client: OAuthClientSlug; + readonly owner: Owner; + readonly reservation: OAuthPopupReservation; }; type RunCimdConnectInput = { @@ -653,6 +664,7 @@ type RunCimdConnectInput = { type CimdOutcome = | { readonly kind: "started"; readonly client: OAuthClientSlug; readonly reused: boolean } + | { readonly kind: "popup-blocked" } | { readonly kind: "failed"; readonly reason: "missing-endpoints" | "create-failed" }; export async function runCimdConnect( @@ -663,6 +675,12 @@ export async function runCimdConnect( return { kind: "failed", reason: "missing-endpoints" }; } + // Claim the window while the click still counts. Minting the client is a + // round trip, and the browser stops honouring `window.open` once the + // activation from the click expires. + const reservation = deps.reserve(); + if (reservation.kind === "blocked") return { kind: "popup-blocked" }; + const resource = input.resource ?? null; const existing = input.existingClients.find( (client) => @@ -675,7 +693,7 @@ export async function runCimdConnect( ); if (existing) { - deps.start({ client: existing.slug, owner: input.owner }); + deps.start({ client: existing.slug, owner: input.owner, reservation }); return { kind: "started", client: existing.slug, reused: true }; } @@ -693,8 +711,11 @@ export async function runCimdConnect( clientId: input.clientIdMetadataDocumentUrl, clientSecret: "", }); - if (created === null) return { kind: "failed", reason: "create-failed" }; - deps.start({ client: created, owner: input.owner }); + if (created === null) { + deps.release(); + return { kind: "failed", reason: "create-failed" }; + } + deps.start({ client: created, owner: input.owner, reservation }); return { kind: "started", client: created, reused: false }; } @@ -741,16 +762,20 @@ type DcrRegisterArgs = { type DcrStartArgs = { readonly client: OAuthClientSlug; readonly owner: Owner; + readonly reservation: OAuthPopupReservation; }; /** Outcome of the DCR orchestration. `"started"` means the OAuth flow handed - * off (the popup/inline start ran); `"fallback"` means we could not auto-set-up - * — a failed probe, no registration endpoint, or a failed registration — and - * the caller should fall back to the bring-your-own-app picker. A failed probe + * off (the popup/inline start ran); `"popup-blocked"` means the browser refused + * the sign-in window, which no app picker can fix, so the caller reports it + * rather than falling back; `"fallback"` means we could not auto-set-up (a + * failed probe, no registration endpoint, or a failed registration) and the + * caller should fall back to the bring-your-own-app picker. A failed probe * carries no probe result; the other two reasons always carry the probe that * seeds the picker. */ type DcrOutcome = | { readonly kind: "started" } + | { readonly kind: "popup-blocked" } | { readonly kind: "fallback"; readonly reason: "probe-failed" } | { readonly kind: "fallback"; @@ -774,6 +799,10 @@ type RunDcrConnectDeps = { ) => Promise; /** Start the OAuth flow with the minted client (popup / inline). */ readonly start: (args: DcrStartArgs) => void; + /** Claim the sign-in window before any await. See `useOAuthPopupFlow.reserve`. */ + readonly reserve: () => OAuthPopupReservation; + /** Close a claimed window this flow turned out not to need. */ + readonly release: () => void; }; type RunDcrConnectInput = { @@ -804,8 +833,9 @@ type RunDcrConnectInput = { const DCR_CLIENT_NAME = "Executor"; /** - * Run the transparent DCR connect sequence: probe → register → start. + * Run the transparent DCR connect sequence: reserve → probe → register → start. * + * - Popup refused → `{ kind: "popup-blocked" }` before any network call. * - Probe failure → `{ kind: "fallback", reason: "probe-failed" }` (caller shows BYO). * - No registration endpoint → `{ kind: "fallback", reason: "no-registration-endpoint", probe }`. * - Register rejected with a message → `{ kind: "fallback", reason: "registration-failed", probe, message }` @@ -817,10 +847,23 @@ export async function runDcrConnect( deps: RunDcrConnectDeps, input: RunDcrConnectInput, ): Promise { + // Claim the sign-in window FIRST, on the click that got us here. Probe and + // register are two network round trips; the browser's user activation expires + // well before they finish, so a window opened after them is refused and the + // connect dies with nothing on screen. + const reservation = deps.reserve(); + if (reservation.kind === "blocked") return { kind: "popup-blocked" }; + const probe = await deps.probe(input.discoveryUrl); - if (probe === null) return { kind: "fallback", reason: "probe-failed" }; + if (probe === null) { + deps.release(); + return { kind: "fallback", reason: "probe-failed" }; + } const registrationEndpoint = probe.registrationEndpoint; - if (!registrationEndpoint) return { kind: "fallback", reason: "no-registration-endpoint", probe }; + if (!registrationEndpoint) { + deps.release(); + return { kind: "fallback", reason: "no-registration-endpoint", probe }; + } const slug = optimisticDcrClientSlug(probe.issuer ?? registrationEndpoint); const scopes = registrationScopes(input.declaredScopes ?? [], probe.scopesSupported ?? []); @@ -838,13 +881,17 @@ export async function runDcrConnect( redirectUri: input.redirectUri, originIntegration: input.integration, }); - if (minted === null) return { kind: "fallback", reason: "registration-failed", probe }; + if (minted === null) { + deps.release(); + return { kind: "fallback", reason: "registration-failed", probe }; + } // OAuthClientSlug is a branded string; an object return carries the failure // message (e.g. the server rejected the redirect URI) for the BYO fallback. if (typeof minted === "object") { + deps.release(); return { kind: "fallback", reason: "registration-failed", probe, message: minted.error }; } - deps.start({ client: minted, owner: input.owner }); + deps.start({ client: minted, owner: input.owner, reservation }); return { kind: "started" }; } @@ -2083,6 +2130,8 @@ function AddAccountModalView(props: AddAccountModalProps) { setCimdBusy(true); const outcome = await runCimdConnect( { + reserve: oauthPopup.reserve, + release: oauthPopup.releaseReservation, createClient: async (args: CimdCreateClientArgs): Promise => { const exit = await doCreateOAuthClient({ payload: { @@ -2100,8 +2149,9 @@ function AddAccountModalView(props: AddAccountModalProps) { if (Exit.isFailure(exit)) return null; return exit.value.client; }, - start: (args: { readonly client: OAuthClientSlug; readonly owner: Owner }): void => { + start: (args: CimdStartArgs): void => { void oauthPopup.start({ + reservation: args.reservation, payload: { client: args.client, // CIMD creates the public client under the connection owner, so @@ -2160,6 +2210,8 @@ function AddAccountModalView(props: AddAccountModalProps) { setDcrBusy(true); const outcome = await runDcrConnect( { + reserve: oauthPopup.reserve, + release: oauthPopup.releaseReservation, probe: async (url: string): Promise => { const exit = await doProbe({ payload: { url }, reactivityKeys: [] }); if (Exit.isFailure(exit)) return null; @@ -2194,6 +2246,7 @@ function AddAccountModalView(props: AddAccountModalProps) { }, start: (args: DcrStartArgs): void => { void oauthPopup.start({ + reservation: args.reservation, payload: { client: args.client, // DCR registers the client under the connection owner, so the app @@ -2236,6 +2289,10 @@ function AddAccountModalView(props: AddAccountModalProps) { success: outcome.kind === "started", ...(outcome.kind === "fallback" ? { dcr_fallback: true } : {}), }); + // Deliberately absent: a "popup-blocked" branch. Registering an app by hand + // does not make the browser open a window, so dropping to the BYO picker + // would send the user down a path that cannot succeed either. `reserve` + // already put the reason in `oauthPopup.error`, which the footer renders. if (outcome.kind === "fallback") { setOAuthFallbackProbe("probe" in outcome ? outcome.probe : null); setDcrFailed(true); @@ -2497,17 +2554,6 @@ function AddAccountModalView(props: AddAccountModalProps) { : `${integrationName} supports Client ID Metadata Document OAuth. We'll use this Executor host's public client metadata document and sign you in.`}

- ) : dcrActive ? ( - // Transparent DCR: no picker. We register an app for you and run - // the OAuth flow with a single Connect click. -
-

No app to choose

-

- {dcrConnecting - ? `Connecting to ${integrationName}…` - : `${integrationName} supports automatic setup. We register an app for you and sign you in — no client ID or app to pick.`} -

-
) : oauthLoading ? (

Loading OAuth apps…

) : ( @@ -2739,9 +2785,6 @@ function AddAccountModalView(props: AddAccountModalProps) { /> )} - {isOAuth && oauthPopup.error ? ( -

{oauthPopup.error}

- ) : null} )} @@ -2909,6 +2952,15 @@ function AddAccountModalView(props: AddAccountModalProps) { {continueError}

) : null} + {/* Above the footer, not inside the method tab: the automatic + (CIMD/DCR) flows render no tab panel at all, and putting the + sign-in error in there left a blocked popup with nothing on + screen but the button returning to "Connect". */} + {isOAuth && oauthPopup.error ? ( +

+ {oauthPopup.error} +

+ ) : null}