diff --git a/.changeset/connection-saved-mcp-app.md b/.changeset/connection-saved-mcp-app.md new file mode 100644 index 0000000000..941fd81038 --- /dev/null +++ b/.changeset/connection-saved-mcp-app.md @@ -0,0 +1,5 @@ +--- +"executor": patch +--- + +Reuse saved OAuth apps for MCP connections without requiring another registration or click. diff --git a/e2e/scenarios/connection-setup-ux.test.ts b/e2e/scenarios/connection-setup-ux.test.ts index fa87ea93d6..d2bcb566b1 100644 --- a/e2e/scenarios/connection-setup-ux.test.ts +++ b/e2e/scenarios/connection-setup-ux.test.ts @@ -1,8 +1,9 @@ import { randomBytes } from "node:crypto"; import { expect } from "@effect/vitest"; -import { Effect } from "effect"; +import { Effect, Schema } from "effect"; import { composePluginApi } from "@executor-js/api/server"; import { connectEmulator } from "@executor-js/emulate"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api"; import { IntegrationSlug, OAuthClientSlug } from "@executor-js/sdk/shared"; import { variable } from "@executor-js/sdk/http-auth"; @@ -12,6 +13,10 @@ import { Api, Browser, Target } from "../src/services"; import { hydrated, visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); +const decodeRegisteredClient = Schema.decodeUnknownSync( + Schema.Struct({ client_id: Schema.String, client_secret: Schema.String }), +); + // Each journey has its own real provider state, OAuth app, user and integration. const connectionFixture = (registerClient: boolean) => Effect.gen(function* () { @@ -262,6 +267,7 @@ scenario( 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(); + await page.getByRole("tab", { name: "OAuth2", exact: true, selected: true }).waitFor(); expect( await page .getByRole("tab", { name: "OAuth2", exact: true }) @@ -288,3 +294,126 @@ scenario( }), ), ); +for (const origin of ["integration", "workspace"] as const) { + scenario( + origin === "integration" + ? "Connection setup · a saved MCP app opens sign-in on the first click" + : "Connection setup · discovery reuses a workspace OAuth app without another click", + {}, + Effect.scoped( + 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(composePluginApi([mcpHttpPlugin()] as const), identity); + const base = yield* createEmulatorInstance("mcp", "saved-app"); + const emulator = yield* Effect.promise(() => connectEmulator({ baseUrl: base })); + const slug = IntegrationSlug.make(`saved-app-${randomBytes(4).toString("hex")}`); + const app = OAuthClientSlug.make(`${slug}-client`); + const registered = yield* Effect.promise(async () => { + const response = await fetch(`${base}/register`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + client_name: "Saved app", + redirect_uris: [new URL("/api/oauth/callback", target.baseUrl).toString()], + grant_types: ["authorization_code", "refresh_token"], + response_types: ["code"], + token_endpoint_auth_method: "client_secret_post", + }), + }); + expect(response.status).toBe(201); + return decodeRegisteredClient(await response.json()); + }); + 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.mcp.removeServer({ params: { slug } }).pipe(Effect.ignore); + yield* client.oauth + .removeClient({ params: { slug: app }, payload: { owner: "org" } }) + .pipe(Effect.ignore); + }).pipe(Effect.ignore), + ); + yield* client.mcp.addServer({ + payload: { + transport: "remote", + slug, + name: "Team MCP", + endpoint: `${base}/mcp`, + authenticationTemplate: [{ kind: "oauth2" }], + }, + }); + yield* client.oauth.createClient({ + payload: { + owner: "org", + slug: app, + grant: "authorization_code", + clientId: registered.client_id, + clientSecret: registered.client_secret, + authorizationUrl: `${base}/authorize`, + tokenUrl: `${base}/token`, + resource: base, + ...(origin === "integration" ? { originIntegration: slug } : {}), + }, + }); + yield* Effect.promise(() => emulator.ledger.clear()); + yield* browser.session(identity, async ({ page, step }) => { + await step("Open an integration that already has a saved OAuth app", async () => { + await visit(page, `/integrations/${slug}?addAccount=1`); + await page.getByRole("tab", { name: "OAuth", exact: true }).waitFor(); + }); + await step("Connect once using the saved app", async () => { + const opened = page.waitForEvent("popup"); + await page.getByRole("button", { name: /^Connect(?: with OAuth)?$/ }).click(); + const popup = await opened; + await popup.waitForURL(/\/authorize/); + expect( + new URL(popup.url()).searchParams.get("client_id"), + "the existing app is reused without another registration step", + ).toBe(registered.client_id); + }); + await step("Approve provider sign-in and save the connection", async () => { + const popup = page + .context() + .pages() + .find((candidate) => candidate !== page); + if (!popup) throw new Error("Provider sign-in window was not open"); + // The published MCP consent form omits its selected user's login. + // Keep the real provider exchange; forward the identity clicked below. + await popup.route(`${base}/authorize/approve`, (route) => { + const body = new URLSearchParams(route.request().postData() ?? ""); + body.set("login", "admin"); + return route.continue({ postData: body.toString() }); + }); + await popup.getByRole("button", { name: /admin/ }).click(); + await page + .getByRole("heading", { name: /Add connection/ }) + .waitFor({ state: "hidden", timeout: 30_000 }); + }); + }); + const connections = yield* client.connections.list({ query: { integration: slug } }); + expect(connections, "provider consent saves the connection").toHaveLength(1); + expect(connections[0]?.owner, "a shared app keeps the connection personal").toBe("user"); + const savedClients = yield* client.oauth.listClients({}); + expect(savedClients.find((saved) => saved.slug === app)?.origin).toEqual( + origin === "integration" + ? { kind: "manual", integration: slug } + : { kind: "manual", integration: null }, + ); + const ledger = yield* Effect.promise(() => emulator.ledger.list()); + expect( + ledger.filter((entry) => entry.method === "POST" && entry.path === "/register"), + "connecting must reuse the saved app", + ).toHaveLength(0); + }), + ), + ); +} diff --git a/e2e/selfhost/oauth-resource-indicator-clear.test.ts b/e2e/selfhost/oauth-resource-indicator-clear.test.ts index e494436773..38876be4a0 100644 --- a/e2e/selfhost/oauth-resource-indicator-clear.test.ts +++ b/e2e/selfhost/oauth-resource-indicator-clear.test.ts @@ -212,15 +212,13 @@ scenario( expect(saved, "the browser-registered app is in the catalog").toBeDefined(); expect(saved?.resource ?? null, "a cleared resource persists as absent").toBeNull(); - // Reopening the form: the cleared field STAYS empty. A DCR-capable method - // only shows the app picker after automatic setup falls back, so take the - // same path a returning user would. + // Reopening the form: the cleared field STAYS empty. The saved app is + // available immediately, without repeating automatic registration. yield* browser.session(identity, async ({ page, step }) => { - await step("Reach the app picker again through the failed automatic setup", async () => { + await step("Reopen the saved app without repeating automatic registration", async () => { await visit(page, `/integrations/${String(slug)}`); await page.getByRole("button", { name: "Add connection" }).first().click(); await page.getByRole("heading", { name: /Add connection/ }).waitFor(); - await page.getByRole("button", { name: "Connect", exact: true }).click(); await page .getByRole("button", { name: `Actions for ${appName}` }) .waitFor({ timeout: 30_000 }); diff --git a/packages/core/api/src/oauth/api.ts b/packages/core/api/src/oauth/api.ts index 5d0eee3d41..1389683d98 100644 --- a/packages/core/api/src/oauth/api.ts +++ b/packages/core/api/src/oauth/api.ts @@ -121,7 +121,10 @@ const OAuthClientSummaryResponse = Schema.Struct({ clientId: Schema.String, tokenEndpointAuthMethod: Schema.optional(TokenEndpointAuthMethodSchema), origin: Schema.Union([ - Schema.Struct({ kind: Schema.Literal("manual") }), + Schema.Struct({ + kind: Schema.Literal("manual"), + integration: Schema.optional(Schema.NullOr(IntegrationSlug)), + }), Schema.Struct({ kind: Schema.Literal("dynamic_client_registration"), integration: Schema.optional(Schema.NullOr(IntegrationSlug)), diff --git a/packages/react/src/components/add-account-modal.test.ts b/packages/react/src/components/add-account-modal.test.ts index 90558590e6..385532cf75 100644 --- a/packages/react/src/components/add-account-modal.test.ts +++ b/packages/react/src/components/add-account-modal.test.ts @@ -1505,3 +1505,67 @@ describe("preferredMethodId", () => { expect(preferredMethodId([], [], integration)).toBe(""); }); }); + +describe("discovered saved OAuth apps", () => { + const cases = [ + { + variation: "shared", + outcome: { kind: "started", flow: "byo" }, + calls: [{ client: "saved", owner: "org", reservation: RESERVED }], + }, + { variation: "other-owner", outcome: { kind: "fallback" }, calls: [] }, + { variation: "other-endpoint", outcome: { kind: "fallback" }, calls: [] }, + { variation: "other-resource", outcome: { kind: "fallback" }, calls: [] }, + ] as const; + for (const { variation, outcome: expectedOutcome, calls } of cases) { + it(`handles a ${variation} app without changing the connection owner`, async () => { + const started: StartArgs[] = []; + const outcome = await runAutomaticOAuthConnect( + { + ...popupSpy(), + isActive: () => true, + probe: async () => ({ + authorizationUrl: "https://auth.example/authorize", + tokenUrl: "https://auth.example/token", + resource: "https://api.example/mcp", + }), + createCimdClient: async () => null, + register: async () => null, + start: (args) => { + started.push(args); + }, + }, + { + owner: variation === "other-owner" ? "org" : "user", + integration: TEST_INTEGRATION, + discoveryUrl: "https://api.example/mcp", + registeredClients: [ + { + owner: variation === "other-owner" ? "user" : "org", + slug: OAuthClientSlug.make("saved"), + grant: "authorization_code", + clientId: "client", + origin: { kind: "manual", integration: null }, + authorizationUrl: "https://auth.example/authorize", + tokenUrl: + variation === "other-endpoint" + ? "https://other.example/token" + : "https://auth.example/token", + resource: + variation === "other-resource" + ? "https://other.example/mcp" + : "https://api.example/mcp", + }, + ], + cimd: { + integrationName: "MCP", + clientIdMetadataDocumentUrl: "https://app.example/client.json", + existingClients: [], + }, + }, + ); + expect(outcome).toMatchObject(expectedOutcome); + expect(started).toEqual(calls); + }); + } +}); diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 222dd0f682..e5228cbd68 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -882,7 +882,7 @@ type DcrStartArgs = { * carries no probe result; the other two reasons always carry the probe that * seeds the picker. */ type AutomaticOAuthOutcome = - | { readonly kind: "started"; readonly flow: "cimd" | "dcr" } + | { readonly kind: "started"; readonly flow: "cimd" | "dcr" | "byo" } | { readonly kind: "popup-blocked" } /** The owning surface went away mid-flight (`isActive` turned false): the * sequence stopped before its next side effect and released the window. @@ -927,6 +927,8 @@ type RunAutomaticOAuthConnectDeps = { }; type RunAutomaticOAuthConnectInput = { + /** Saved apps available for a fresh connection; omitted for reconnects. */ + readonly registeredClients?: readonly OAuthClientSummary[]; readonly discoveryUrl: string; /** The integration's genuine protected-resource URL (the MCP discovery URL), * used as the RFC 8707 resource indicator when the server's PRM names no @@ -1022,6 +1024,22 @@ export async function runAutomaticOAuthConnect( ? null : (probe.resource ?? input.storedResource) : (probe.resource ?? input.resourceFallback ?? null); + // A workspace app may have been registered outside this dialog. Once + // discovery establishes the exact endpoints, reuse it without a second + // click. Never select a near match or another owner's personal app. + const savedClient = input.registeredClients?.find( + (client) => + client.origin.kind === "manual" && + client.grant === "authorization_code" && + (client.owner === input.owner || client.owner === "org") && + client.authorizationUrl === probe.authorizationUrl && + client.tokenUrl === probe.tokenUrl && + (client.resource == null || client.resource === resource), + ); + if (savedClient) { + deps.start({ client: savedClient.slug, owner: savedClient.owner, reservation }); + return { kind: "started", flow: "byo" }; + } if (probe.clientIdMetadataDocumentSupported === true) { const resolved = await resolveCimdClient( { createClient: deps.createCimdClient }, @@ -1788,7 +1806,15 @@ function AddAccountModalView(props: AddAccountModalProps) { // DCR-capable (see `hasDcr`). When DCR-capable and not yet fallen back, we // skip the app picker entirely (Option A). const isDcr = !cimdActive && hasDcr(method); - const dcrActive = isDcr && !dcrFailed; + // Reuse an app explicitly registered for this integration. Discovery must + // not send its users through automatic registration again on every connect. + const hasSavedOAuthApp = clientSummaries.some( + (client) => + client.grant === "authorization_code" && + client.origin.kind === "manual" && + client.origin.integration === integration, + ); + const dcrActive = isDcr && !dcrFailed && !hasSavedOAuthApp; const automaticOAuthActive = cimdActive || dcrActive; // OAuth apps usable for this integration (user-owned first). Hooks run @@ -2493,8 +2519,8 @@ function AddAccountModalView(props: AddAccountModalProps) { reservation: args.reservation, payload: { client: args.client, - // DCR/CIMD mints the client under the connection owner, so the - // app and connection share one owner. + // Discovery may reuse a shared app for a Personal connection. + // Keep the app owner separate from the requested connection owner. clientOwner: args.owner, owner: dcrOwner, name: request.connectionName, @@ -2542,11 +2568,10 @@ function AddAccountModalView(props: AddAccountModalProps) { // not, so pass the un-collapsed method value here. resourceFallback: requestMethod.oauth?.discoveryUrl, owner: dcrOwner, - // DCR slugs are server-keyed (Part A): the connect path no longer depends - // on the picker's app list, so it need not be threaded here. declaredScopes: requestMethod.oauth?.scopes, redirectUri: oauthCallbackUrl(), integration, + ...(reconnect ? {} : { registeredClients: clientSummaries }), cimd: { integrationName, clientIdMetadataDocumentUrl: oauthClientIdMetadataDocumentUrl(),