diff --git a/.changeset/connection-oauth-default.md b/.changeset/connection-oauth-default.md new file mode 100644 index 0000000000..35cfce58c2 --- /dev/null +++ b/.changeset/connection-oauth-default.md @@ -0,0 +1,5 @@ +--- +"executor": patch +--- + +Prefer browser sign-in when a matching OAuth client is available, while preserving a user’s chosen method when clients finish loading. diff --git a/e2e/scenarios/connection-setup-ux.test.ts b/e2e/scenarios/connection-setup-ux.test.ts index 8158a9edbd..fa87ea93d6 100644 --- a/e2e/scenarios/connection-setup-ux.test.ts +++ b/e2e/scenarios/connection-setup-ux.test.ts @@ -9,95 +9,108 @@ import { variable } from "@executor-js/sdk/http-auth"; import { createEmulatorInstance } from "../src/emulator-instance"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; -import { visit } from "../src/surfaces/browser"; +import { hydrated, visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); // Each journey has its own real provider state, OAuth app, user and integration. -const fixture = Effect.gen(function* () { - const target = yield* Target; - const browser = yield* Browser; - const { client: makeClient } = yield* Api; - const identity = yield* target.newIdentity(); - const client = yield* makeClient(api, identity); - const slug = IntegrationSlug.make(`setup-${randomBytes(4).toString("hex")}`); - const app = OAuthClientSlug.make(`${slug}-app`); - const baseUrl = yield* createEmulatorInstance("slack", "connection-setup"); - const emulator = yield* Effect.promise(() => connectEmulator({ baseUrl })); - const credential = yield* Effect.promise(() => - emulator.credentials.mint({ - type: "oauth-authorization-code", - redirect_uris: [new URL("/api/oauth/callback", target.baseUrl).toString()], - }), - ); - const { - client_id: clientId, - client_secret: clientSecret, - authorization_url: authorizationUrl, - token_url: tokenUrl, - } = credential; - if (!clientId || !clientSecret || !authorizationUrl || !tokenUrl) { - return yield* Effect.die("Slack emulator did not mint an OAuth app"); - } - yield* Effect.addFinalizer(() => - Effect.gen(function* () { - const connections = yield* client.connections.list({ query: { integration: slug } }); - for (const connection of connections) { - yield* client.connections - .remove({ - params: { - owner: connection.owner, - integration: slug, - name: connection.name, - }, - }) +const connectionFixture = (registerClient: boolean) => + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const slug = IntegrationSlug.make(`setup-${randomBytes(4).toString("hex")}`); + const app = OAuthClientSlug.make(`${slug}-app`); + const baseUrl = yield* createEmulatorInstance("slack", "connection-setup"); + const emulator = yield* Effect.promise(() => connectEmulator({ baseUrl })); + const credential = yield* Effect.promise(() => + emulator.credentials.mint({ + type: "oauth-authorization-code", + redirect_uris: [new URL("/api/oauth/callback", target.baseUrl).toString()], + }), + ); + const { + client_id: clientId, + client_secret: clientSecret, + authorization_url: authorizationUrl, + token_url: tokenUrl, + } = credential; + if (!clientId || !clientSecret || !authorizationUrl || !tokenUrl) { + return yield* Effect.die("Slack emulator did not mint an OAuth app"); + } + yield* Effect.addFinalizer(() => + Effect.gen(function* () { + const connections = yield* client.connections.list({ query: { integration: slug } }); + for (const connection of connections) { + yield* client.connections + .remove({ + params: { + owner: connection.owner, + integration: slug, + name: connection.name, + }, + }) + .pipe(Effect.ignore); + } + yield* client.oauth + .removeClient({ params: { slug: app }, payload: { owner: "org" } }) .pipe(Effect.ignore); - } - yield* client.oauth - .removeClient({ params: { slug: app }, payload: { owner: "org" } }) - .pipe(Effect.ignore); - yield* client.openapi.removeSpec({ params: { slug } }).pipe(Effect.ignore); - }).pipe(Effect.ignore), - ); - yield* client.openapi.addSpec({ - payload: { - slug, - name: "Team chat", - baseUrl: "https://slack.com", - displayDomain: "slack.com", - spec: { - kind: "blob", - value: JSON.stringify({ - openapi: "3.0.3", - info: { title: "Team chat", version: "1" }, - // No API operations: only the isolated emulator receives OAuth traffic. - servers: [{ url: "https://slack.com" }], - paths: {}, - }), + yield* client.openapi.removeSpec({ params: { slug } }).pipe(Effect.ignore); + }).pipe(Effect.ignore), + ); + yield* client.openapi.addSpec({ + payload: { + slug, + name: "Team chat", + baseUrl: "https://slack.com", + displayDomain: "slack.com", + spec: { + kind: "blob", + value: JSON.stringify({ + openapi: "3.0.3", + info: { title: "Team chat", version: "1" }, + // No API operations: only the isolated emulator receives OAuth traffic. + servers: [{ url: "https://slack.com" }], + paths: {}, + }), + }, + authenticationTemplate: [ + { + slug: "token", + type: "apiKey", + headers: { Authorization: ["Bearer ", variable("token")] }, + }, + { + slug: "oauth", + kind: "oauth2", + // The unconfigured case tests metadata only, without contacting a + // provider. A unique reserved host cannot match another test's app. + authorizationUrl: registerClient + ? authorizationUrl + : `https://${slug}.invalid/authorize`, + tokenUrl: registerClient ? tokenUrl : `https://${slug}.invalid/token`, + scopes: ["users:read"], + }, + ], }, - authenticationTemplate: [ - { - slug: "token", - type: "apiKey", - headers: { Authorization: ["Bearer ", variable("token")] }, + }); + if (registerClient) + yield* client.oauth.createClient({ + payload: { + slug: app, + owner: "org", + grant: "authorization_code", + clientId, + clientSecret, + authorizationUrl, + tokenUrl, + originIntegration: slug, }, - { slug: "oauth", kind: "oauth2", authorizationUrl, tokenUrl, scopes: ["users:read"] }, - ], - }, - }); - yield* client.oauth.createClient({ - payload: { - slug: app, - owner: "org", - grant: "authorization_code", - clientId, - clientSecret, - authorizationUrl, - tokenUrl, - originIntegration: slug, - }, + }); + return { target, browser, identity, client, slug, emulator }; }); - return { target, browser, identity, client, slug, emulator }; -}); +const fixture = connectionFixture(true); scenario( "Slack OAuth · provider consent saves a connection", @@ -135,3 +148,143 @@ scenario( }), ), ); + +scenario( + "Connection setup · without a matching client the API key stays the default", + {}, + Effect.scoped( + Effect.gen(function* () { + const { browser, identity, slug } = yield* connectionFixture(false); + yield* browser.session(identity, async ({ page, step }) => { + await step("Open an integration whose OAuth app has not been configured", async () => { + await visit(page, `/integrations/${slug}?addAccount=1`); + expect( + await page + .getByRole("tab", { name: "API key (Authorization)", exact: true }) + .getAttribute("aria-selected"), + "declaring OAuth without a usable client must not replace the key form", + ).toBe("true"); + await page.getByRole("tab", { name: "OAuth2", exact: true }).click(); + await page.getByRole("button", { name: "Register app", exact: true }).waitFor(); + }); + }); + }), + ), +); + +for (const { interaction, expectedOAuth, expectedKeys } of [ + { interaction: "untouched", expectedOAuth: "true", expectedKeys: [] }, + { interaction: "select API key", expectedOAuth: "false", expectedKeys: [""] }, + { + interaction: "enter API key", + expectedOAuth: "false", + expectedKeys: ["synthetic-key-in-progress"], + }, +] as const) { + scenario( + `Connection setup · client list arrives with the form ${interaction}`, + {}, + Effect.scoped( + Effect.gen(function* () { + const { browser, identity, slug } = yield* fixture; + yield* browser.session(identity, async ({ page, step }) => { + const started = Promise.withResolvers(); + const released = Promise.withResolvers(); + const completed = Promise.withResolvers(); + await page.route(/\/api\/oauth\/clients(?:\?|$)/, async (route) => { + const response = await route.fetch(); + started.resolve(); + await released.promise; + await route.fulfill({ response }); + completed.resolve(); + }); + try { + await step("Open the dialog while the real OAuth client list is held", async () => { + await page.goto(`/integrations/${slug}?addAccount=1`, { + waitUntil: "domcontentloaded", + }); + await hydrated(page); + await started.promise; + const keyTab = page.getByRole("tab", { + name: "API key (Authorization)", + exact: true, + }); + await keyTab.waitFor(); + expect(await keyTab.getAttribute("aria-selected")).toBe("true"); + }); + await step( + "Resolve client availability without replacing a user's choice", + async () => { + const keyTab = page.getByRole("tab", { + name: "API key (Authorization)", + exact: true, + }); + const keyInput = page.getByRole("textbox", { name: "Authorization", exact: true }); + if (interaction === "select API key") await keyTab.click(); + if (interaction === "enter API key") + await keyInput.fill("synthetic-key-in-progress"); + released.resolve(); + await completed.promise; + await page.waitForLoadState("networkidle"); + expect( + await page + .getByRole("tab", { name: "OAuth2", exact: true }) + .getAttribute("aria-selected"), + "only an untouched form should adopt the available OAuth client", + ).toBe(expectedOAuth); + expect( + await keyTab.getAttribute("aria-selected"), + "late data must preserve the chosen key form", + ).toBe(String(expectedOAuth === "false")); + expect( + await Promise.all((await keyInput.all()).map((input) => input.inputValue())), + "any key already entered must remain unchanged", + ).toEqual(expectedKeys); + }, + ); + } finally { + released.resolve(); + await page.unrouteAll({ behavior: "wait" }); + } + }); + }), + ), + ); +} + +scenario( + "Connection setup · a ready OAuth app is the default", + {}, + Effect.scoped( + Effect.gen(function* () { + const { browser, identity, slug } = yield* fixture; + yield* browser.session(identity, async ({ page, step }) => { + await step("Add a connection with both a token and a registered sign-in app", async () => { + await visit(page, `/integrations/${slug}?addAccount=1`); + await page.getByRole("tab", { name: "OAuth2", exact: true }).waitFor(); + expect( + await page + .getByRole("tab", { name: "OAuth2", exact: true }) + .getAttribute("aria-selected"), + "sign-in is selected without first switching away from API token", + ).toBe("true"); + }); + await step("Choose an API key instead of browser sign-in", async () => { + const tokenTab = page.getByRole("tab", { name: "API key (Authorization)", exact: true }); + await tokenTab.click(); + expect(await tokenTab.getAttribute("aria-selected")).toBe("true"); + await page.getByRole("button", { name: "Cancel", exact: true }).click(); + await page.getByRole("heading", { name: /Add connection/ }).waitFor({ state: "hidden" }); + }); + await step("Open a new connection on browser sign-in again", async () => { + await page.getByRole("button", { name: "Add connection", exact: true }).click(); + expect( + await page + .getByRole("tab", { name: "OAuth2", exact: true }) + .getAttribute("aria-selected"), + ).toBe("true"); + }); + }); + }), + ), +); diff --git a/packages/react/src/components/add-account-modal.test.ts b/packages/react/src/components/add-account-modal.test.ts index 0fdedf4983..90558590e6 100644 --- a/packages/react/src/components/add-account-modal.test.ts +++ b/packages/react/src/components/add-account-modal.test.ts @@ -11,6 +11,7 @@ import { import type { AuthMethod } from "../lib/auth-placements"; import type { OAuthPopupReservation } from "../plugins/oauth-sign-in"; +import type { OAuthClientOption } from "../plugins/use-effective-oauth-client"; import { connectionNameFrom, connectionLabel, @@ -19,6 +20,7 @@ import { DEFAULT_CONNECTION_OWNER, hasDcr, mergeCustomMethods, + preferredMethodId, oauthIdentityLabelFromHealth, runAutomaticOAuthConnect, runCimdConnect, @@ -1438,3 +1440,68 @@ describe("runDcrConnect", () => { expect(String(registerArgs!.slug)).toBe("dcr-auth-example-com"); }); }); + +describe("preferredMethodId", () => { + const integration = IntegrationSlug.make("team-chat"); + const token = apiKeyMethod("token", "spec"); + const oauth: AuthMethod = { + id: "oauth", + label: "OAuth", + kind: "oauth", + source: "spec", + template: AuthTemplateSlug.make("oauth"), + placements: [], + oauth: { tokenUrl: "https://auth.example.com/token", scopes: ["read"] }, + }; + const client: OAuthClientOption = { + owner: "org", + slug: OAuthClientSlug.make("team-chat-app"), + grant: "authorization_code", + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + clientId: "synthetic-client", + origin: { kind: "manual", integration: null }, + }; + + it("prefers OAuth only with a client matched to that method", () => { + expect(preferredMethodId([token, oauth], [client], integration)).toBe("oauth"); + expect(preferredMethodId([oauth, token], [], integration)).toBe("token"); + expect( + preferredMethodId( + [token, oauth], + [{ ...client, tokenUrl: "https://other.example.com/token" }], + integration, + ), + ).toBe("token"); + }); + + it("selects the OAuth method with a client rather than the first OAuth method", () => { + const unconfigured = { + ...oauth, + id: "other-oauth", + oauth: { tokenUrl: "https://unregistered.example.net/token" }, + }; + expect(preferredMethodId([token, unconfigured, oauth], [client], integration)).toBe("oauth"); + }); + + it("uses matching built-in clients only when they allow the requested scopes", () => { + const builtIn: OAuthClientOption = { + ...client, + origin: { kind: "first_party", allowedScopes: ["read"] }, + }; + expect(preferredMethodId([token, oauth], [builtIn], integration)).toBe("oauth"); + expect( + preferredMethodId( + [token, { ...oauth, oauth: { ...oauth.oauth, scopes: ["write"] } }], + [builtIn], + integration, + ), + ).toBe("token"); + }); + + it("keeps single-method and empty integrations usable", () => { + expect(preferredMethodId([token], [], integration)).toBe("token"); + expect(preferredMethodId([oauth], [], integration)).toBe("oauth"); + expect(preferredMethodId([], [], integration)).toBe(""); + }); +}); diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 5e10fd509f..222dd0f682 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -73,6 +73,7 @@ import { clientDisplayName, clientHost, optimisticDcrClientSlug, + selectClientsForEndpoints, selectDcrClientsForIntegration, uniqueClientSlug, useOAuthClientsForIntegration, @@ -628,13 +629,29 @@ export const connectionExistsMessage = (label: string): string => * explicit choice. Personal: a connection is most often a personal credential. */ export const DEFAULT_CONNECTION_OWNER: Owner = "user"; -/** The method the modal opens on. OAuth needs a registered app (or a DCR - * round-trip) before "Connect" does anything; a key is one paste. When an - * integration declares both, starting on OAuth greets most users with - * "Register app" — a dead end — while the working method sits one tab over. - * Prefer the first non-OAuth method; OAuth stays one click away. */ -export const preferredMethodId = (methods: readonly AuthMethod[]): string => - (methods.find((method) => method.kind !== "oauth") ?? methods[0])?.id ?? ""; +/** Prefer OAuth only when its picker has a matching, usable client. Otherwise + * prefer a credential method; OAuth-only integrations still expose setup. */ +export const preferredMethodId = ( + methods: readonly AuthMethod[], + clients: readonly OAuthClientOption[], + integration: IntegrationSlug, +): string => + ( + methods.find( + (method) => + method.kind === "oauth" && + selectClientsForEndpoints(clients, { + integration, + tokenUrl: method.oauth?.tokenUrl, + authorizationUrl: method.oauth?.authorizationUrl, + scopes: method.oauth?.scopes, + discoversScopes: hasDcr(method), + requireEndpointMatch: true, + }).matched.length > 0, + ) ?? + methods.find((method) => method.kind !== "oauth") ?? + methods[0] + )?.id ?? ""; const authMethodKey = (method: AuthMethod): string => method.source === "custom" ? `custom:${String(method.template)}` : `declared:${method.id}`; @@ -1443,7 +1460,9 @@ function AddAccountModalView(props: AddAccountModalProps) { ); const [addingMethod, setAddingMethod] = useState(false); - const [methodId, setMethodId] = useState(preferredMethodId(methods)); + // An untouched form follows client availability. User interaction or a + // handoff pins a method so a late clients response cannot replace their form. + const [selectedMethodId, setMethodId] = useState(null); // One value per distinct credential input (`variable → pasted value`). A // single-secret method has just `{ token }`; a method with two distinct inputs // (e.g. Datadog's two keys) collects one value per variable. @@ -1548,6 +1567,23 @@ function AddAccountModalView(props: AddAccountModalProps) { () => (AsyncResult.isSuccess(allClientsResult) ? allClientsResult.value : []), [allClientsResult], ); + const defaultMethodId = useMemo( + () => + preferredMethodId( + allMethods, + clientSummaries.flatMap((client) => + client.grant === "authorization_code" || client.grant === "client_credentials" + ? [{ ...client, grant: client.grant }] + : [], + ), + integration, + ), + [allMethods, clientSummaries, integration], + ); + const methodId = + selectedMethodId !== null && allMethods.some((method) => method.id === selectedMethodId) + ? selectedMethodId + : defaultMethodId; const usage = useMemo( () => buildUsageMap(AsyncResult.isSuccess(connectionsResult) ? connectionsResult.value : []), [connectionsResult], @@ -1582,15 +1618,6 @@ function AddAccountModalView(props: AddAccountModalProps) { [allMethods, methodId], ); - useEffect(() => { - if (allMethods.length === 0) { - if (methodId !== "") setMethodId(""); - return; - } - if (allMethods.some((m: AuthMethod) => m.id === methodId)) return; - setMethodId(allMethods[0]!.id); - }, [allMethods, methodId]); - // Apply the handoff prefill ONCE per handoff key (tracked by ref). The // effect's deps include `allMethods`, which gets a new identity whenever the // integration refetches — and the wizard itself triggers a refetch mid-flow @@ -1622,18 +1649,6 @@ function AddAccountModalView(props: AddAccountModalProps) { setDcrFallbackMessage(null); }, [initialState, allMethods, defaultOwner, ownerOptions]); - useEffect(() => { - if (allMethods.length === 0) return; - if (allMethods.some((m: AuthMethod) => m.id === methodId)) return; - const initialMethod = initialState?.template - ? allMethods.find( - (m: AuthMethod) => - m.id === initialState.template || String(m.template) === initialState.template, - ) - : undefined; - setMethodId(initialMethod?.id ?? preferredMethodId(allMethods)); - }, [allMethods, initialState?.template, methodId]); - // Non-secret prefill carried by an `oauth.clients.createHandoff` deep link. // The agent fills in the endpoints/grant/client id it discovered; the client // secret is deliberately absent and is typed by the human in the form below. @@ -2695,6 +2710,9 @@ function AddAccountModalView(props: AddAccountModalProps) { setMethodId(methodId)} + onKeyDownCapture={() => setMethodId(methodId)} + onInput={() => setMethodId(methodId)} className={cn( "max-h-[85vh] overflow-x-hidden overflow-y-auto", (addingMethod && createCustomMethod) || oauthRegistering || oauthEditing