diff --git a/.changeset/connection-oauth-cancel.md b/.changeset/connection-oauth-cancel.md new file mode 100644 index 0000000000..59028ef5a1 --- /dev/null +++ b/.changeset/connection-oauth-cancel.md @@ -0,0 +1,5 @@ +--- +"executor": patch +--- + +Let users cancel sign-in from the connection form and prevent cancelled OAuth requests from disrupting a retry. diff --git a/e2e/scenarios/connection-setup-ux.test.ts b/e2e/scenarios/connection-setup-ux.test.ts index fa87ea93d6..4900b302db 100644 --- a/e2e/scenarios/connection-setup-ux.test.ts +++ b/e2e/scenarios/connection-setup-ux.test.ts @@ -122,6 +122,11 @@ scenario( await step("Sign in with a provider account without naming the connection", async () => { await visit(page, `/integrations/${slug}?addAccount=1`); await page.getByRole("tab", { name: "OAuth2", exact: true }).click(); + // Attached previews can contain other compatible apps. Use this + // scenario's provider instance for the full callback and token exchange. + await page + .getByRole("radio", { name: new RegExp(slug.replace("setup-", ""), "i") }) + .check(); const opened = page.waitForEvent("popup"); await page.getByRole("button", { name: "Connect with OAuth", exact: true }).click(); const popup = await opened; @@ -288,3 +293,148 @@ scenario( }), ), ); + +scenario( + "Connection setup · an interrupted OAuth popup can be cancelled and retried", + {}, + Effect.scoped( + Effect.gen(function* () { + const { browser, identity, slug, client, emulator } = yield* fixture; + yield* browser.session(identity, async ({ page, step }) => { + await step("Start provider sign-in without entering a name", async () => { + await visit(page, `/integrations/${slug}?addAccount=1`); + await page.getByRole("tab", { name: "OAuth2", exact: true }).click(); + // Attached previews can contain other compatible apps. Use this + // scenario's provider instance for the full callback and token exchange. + await page + .getByRole("radio", { name: new RegExp(slug.replace("setup-", ""), "i") }) + .check(); + const opened = page.waitForEvent("popup"); + await page.getByRole("button", { name: "Connect with OAuth", exact: true }).click(); + const popup = await opened; + await popup.waitForURL(/oauth\/v2\/authorize/); + await popup.close(); + }); + await step("Cancel the waiting sign-in and retry", async () => { + const cancel = page.getByRole("button", { name: "Cancel sign-in", exact: true }); + expect( + await cancel.isVisible(), + "an interrupted provider window must not strand Connecting forever", + ).toBe(true); + expect(await cancel.isEnabled()).toBe(true); + await page.getByText("Continue in the sign-in window", { exact: true }).waitFor(); + expect( + await page.getByRole("tab", { name: "OAuth2", exact: true }).isVisible(), + "the authentication form stays visible during sign-in", + ).toBe(true); + expect(await page.getByRole("textbox", { name: /Display name/ }).isVisible()).toBe(true); + expect( + await page.getByRole("button", { name: "Connecting…", exact: true }).count(), + "waiting for provider consent must not leave a dead Connecting button", + ).toBe(0); + await cancel.click(); + expect( + await page.getByRole("button", { name: "Connect with OAuth", exact: true }).isEnabled(), + ).toBe(true); + const opened = page.waitForEvent("popup"); + await page.getByRole("button", { name: "Connect with OAuth", exact: true }).click(); + const popup = await opened; + await popup.waitForURL(/oauth\/v2\/authorize/); + await popup.route("https://emulators.dev/oauth/v2/authorize/callback", (route) => + route.continue({ url: `${emulator.baseUrl}/oauth/v2/authorize/callback` }), + ); + await popup.getByRole("button", { name: /admin/ }).click(); + await page.getByRole("dialog").waitFor({ state: "hidden", timeout: 30_000 }); + }); + }); + const connections = yield* client.connections.list({ query: { integration: slug } }); + expect(connections, "retry finishes and persists exactly one connection").toHaveLength(1); + }), + ), +); + +scenario( + "Connection setup · cancelling a pending OAuth start ignores its late response", + {}, + Effect.scoped( + Effect.gen(function* () { + const { browser, identity, slug, client, emulator } = yield* fixture; + yield* browser.session(identity, async ({ page, step }) => { + const released = Promise.withResolvers(); + const started = Promise.withResolvers(); + const completed = Promise.withResolvers(); + await page.route( + "**/api/oauth/start", + async (route) => { + const response = await route.fetch(); + const body: unknown = await response.json(); + if ( + typeof body !== "object" || + body === null || + !("state" in body) || + typeof body.state !== "string" + ) { + expect.fail("the held start response must contain its OAuth session state"); + } + started.resolve(body.state); + await released.promise; + await route.fulfill({ response }); + completed.resolve(); + }, + { times: 1 }, + ); + try { + await step("Cancel while the authorization response is still in flight", async () => { + await visit(page, `/integrations/${slug}?addAccount=1`); + await page + .getByRole("radio", { name: new RegExp(slug.replace("setup-", ""), "i") }) + .check(); + const opened = page.waitForEvent("popup"); + await page.getByRole("button", { name: "Connect with OAuth", exact: true }).click(); + const popup = await opened; + const oldState = await started.promise; + expect( + await page.getByRole("textbox", { name: /Display name/ }).isVisible(), + "the connection form stays visible while sign-in is being prepared", + ).toBe(true); + await page.getByRole("button", { name: "Cancel sign-in", exact: true }).click(); + await expect.poll(() => popup.isClosed()).toBe(true); + const cancelled = page.waitForResponse((response) => + response.url().endsWith("/api/oauth/cancel"), + ); + const retryOpened = page.waitForEvent("popup"); + await page.getByRole("button", { name: "Connect with OAuth", exact: true }).click(); + const retryPopup = await retryOpened; + await retryPopup.waitForURL(/oauth\/v2\/authorize/); + released.resolve(); + await completed.promise; + const cancellation = await cancelled; + expect( + cancellation.request().postDataJSON(), + "only the old session is cancelled", + ).toEqual({ state: oldState }); + expect( + retryPopup.isClosed(), + "the old response cannot close the new sign-in window", + ).toBe(false); + expect( + page.context().pages(), + "only the replacement sign-in window remains", + ).toHaveLength(2); + await page.getByText("Continue in the sign-in window", { exact: true }).waitFor(); + await retryPopup.route("https://emulators.dev/oauth/v2/authorize/callback", (route) => + route.continue({ url: `${emulator.baseUrl}/oauth/v2/authorize/callback` }), + ); + await retryPopup.getByRole("button", { name: /admin/ }).click(); + await page.getByRole("dialog").waitFor({ state: "hidden", timeout: 30_000 }); + }); + } finally { + released.resolve(); + await page.unrouteAll({ behavior: "wait" }); + } + }); + const connections = yield* client.connections.list({ query: { integration: slug } }); + expect(connections, "the newer attempt survives and completes consent").toHaveLength(1); + }), + ), +); diff --git a/packages/react/src/components/add-account-modal.test.ts b/packages/react/src/components/add-account-modal.test.ts index 90558590e6..8be53d3d2c 100644 --- a/packages/react/src/components/add-account-modal.test.ts +++ b/packages/react/src/components/add-account-modal.test.ts @@ -366,6 +366,7 @@ describe("runCimdConnect", () => { const outcome = await runCimdConnect( { + isActive: () => true, reserve: (): OAuthPopupReservation => RESERVED, release: (): void => {}, createClient: (args: CimdCreateArgs): Promise => { @@ -410,6 +411,7 @@ describe("runCimdConnect", () => { const outcome = await runCimdConnect( { + isActive: () => true, reserve: (): OAuthPopupReservation => RESERVED, release: (): void => {}, createClient: (): Promise => { @@ -1076,6 +1078,7 @@ describe("runCimdConnect popup reservation", () => { const popup = popupSpy(); const outcome = await runCimdConnect( { + isActive: () => true, ...popup, createClient: (args: CimdCreateArgs): Promise => { popup.calls.push("createClient"); @@ -1096,6 +1099,7 @@ describe("runCimdConnect popup reservation", () => { const popup = popupSpy(); const outcome = await runCimdConnect( { + isActive: () => true, ...popup, createClient: (): Promise => Promise.resolve(null), start: (): void => {}, @@ -1107,10 +1111,32 @@ describe("runCimdConnect popup reservation", () => { expect(popup.calls).toEqual(["reserve", "release"]); }); + it("does not start sign-in after cancellation during client setup", async () => { + const created = Promise.withResolvers(); + const popup = popupSpy(); + let active = true; + const connecting = runCimdConnect( + { + ...popup, + isActive: () => active, + createClient: () => created.promise, + start: () => { + popup.calls.push("start"); + }, + }, + cimdInput, + ); + active = false; + created.resolve(OAuthClientSlug.make("cancelled-client")); + expect(await connecting).toEqual({ kind: "aborted" }); + 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( { + isActive: () => true, ...popup, createClient: (): Promise => Promise.resolve(null), start: (): void => {}, diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 222dd0f682..9f93803071 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -741,6 +741,7 @@ type CimdCreateClientArgs = { }; type RunCimdConnectDeps = { + readonly isActive: () => boolean; readonly createClient: (args: CimdCreateClientArgs) => Promise; readonly start: (args: CimdStartArgs) => void; /** Claim the sign-in window before any await. See `useOAuthPopupFlow.reserve`. */ @@ -767,6 +768,7 @@ type RunCimdConnectInput = { type CimdOutcome = | { readonly kind: "started"; readonly client: OAuthClientSlug; readonly reused: boolean } + | { readonly kind: "aborted" } | { readonly kind: "popup-blocked" } | { readonly kind: "failed"; readonly reason: "missing-endpoints" | "create-failed" }; @@ -830,6 +832,10 @@ export async function runCimdConnect( if (reservation.kind === "blocked") return { kind: "popup-blocked" }; const resolved = await resolveCimdClient(deps, input); + if (!deps.isActive()) { + deps.release(); + return { kind: "aborted" }; + } if (resolved.kind === "failed") { deps.release(); return resolved; @@ -1866,7 +1872,14 @@ function AddAccountModalView(props: AddAccountModalProps) { const oauthBusy = ccBusy || oauthPopup.busy; const cimdConnecting = cimdBusy || oauthPopup.busy; const dcrConnecting = dcrBusy || oauthPopup.busy; - const automaticOAuthConnecting = cimdConnecting || dcrConnecting; + const signInPending = cimdBusy || dcrBusy || oauthPopup.busy; + const automaticAttemptRef = useRef(0); + const cancelSignIn = () => { + automaticAttemptRef.current += 1; + oauthPopup.cancel(); + setCimdBusy(false); + setDcrBusy(false); + }; // "Connection saved to" for a PICKED BYO OAuth app. Cloud: a Workspace (`org`) // app can mint Personal or Workspace connections; a Personal (`user`) app can @@ -2359,11 +2372,16 @@ function AddAccountModalView(props: AddAccountModalProps) { const cimdOwner = owner; const connectionName = previewConnectionName(label, cimdOwner); const identityLabel = typedIdentityLabel(label); + const attempt = ++automaticAttemptRef.current; + const isActive = () => viewMountedRef.current && attempt === automaticAttemptRef.current; setCimdBusy(true); const outcome = await runCimdConnect( { reserve: oauthPopup.reserve, - release: oauthPopup.releaseReservation, + release: () => { + if (attempt === automaticAttemptRef.current) oauthPopup.releaseReservation(); + }, + isActive, createClient: createCimdClient, start: (args: CimdStartArgs): void => { void oauthPopup.start({ @@ -2398,6 +2416,7 @@ function AddAccountModalView(props: AddAccountModalProps) { existingClients: clientSummaries, }, ); + if (!isActive()) return; setCimdBusy(false); trackEvent("connection_oauth_started", { integration_slug: String(integration), @@ -2442,16 +2461,18 @@ function AddAccountModalView(props: AddAccountModalProps) { setDcrFailed(true); return; } + const attempt = ++automaticAttemptRef.current; + const isActive = () => viewMountedRef.current && attempt === automaticAttemptRef.current; setDcrBusy(true); const outcome = await runAutomaticOAuthConnect( { reserve: oauthPopup.reserve, - release: oauthPopup.releaseReservation, - // Closing the modal genuinely unmounts this view (see - // `AddAccountModal`), so "still mounted" is exactly "still open". - // The sequence checks it between round trips: a close mid-flight - // must not register a client or launch the popup afterwards. - isActive: () => viewMountedRef.current, + release: () => { + if (attempt === automaticAttemptRef.current) oauthPopup.releaseReservation(); + }, + // A closed modal or cancelled attempt cannot register a client or + // launch sign-in after the next round trip. A retry owns a new attempt. + isActive, probe: async (url: string): Promise => { const exit = await doProbe({ payload: { url }, reactivityKeys: [] }); if (Exit.isFailure(exit)) return null; @@ -2560,7 +2581,7 @@ function AddAccountModalView(props: AddAccountModalProps) { // The modal closed mid-flight: this view is unmounted, so no state may // be written at all — not the fallback below, and not even the busy // flag, which belongs to the surface that is gone. - if (outcome.kind === "aborted") return; + if (!isActive() || outcome.kind === "aborted") return; setDcrBusy(false); // `connection_oauth_started` measures the connect funnel; a reconnect // reports through `connection_reconnected` on the popup callbacks above, @@ -3371,15 +3392,20 @@ function AddAccountModalView(props: AddAccountModalProps) {

) : null} - {/* Footer action, in precedence order: + - pending sign-in: cancel, or wait for an authorized connection to save; - transparent CIMD (no app registration): create/reuse a public metadata-document client and start OAuth; - transparent DCR (no picker): a single Connect that runs @@ -3387,7 +3413,17 @@ function AddAccountModalView(props: AddAccountModalProps) { - registering a BYO app: the form owns its own submit, no footer; - picked BYO OAuth app: Connect with OAuth / Connect (client creds); - credential/no-auth method: Add connection. */} - {cimdActive ? ( + {signInPending ? ( + oauthPopup.phase === "saving" ? ( + + ) : ( + + ) + ) : cimdActive ? (