* fix(oauth): route DCR reconnect through registration flow * Route DCR reconnect through the registration flow A dynamically registered OAuth client is bound to the redirect URI it registered with, so once the app's callback origin changed (127.0.0.1 to localhost) Reconnect kept starting the flow against the stored client and the authorization server rejected it, leaving no way to repair the connection. Reconnect now takes the same probe -> CIMD-or-register -> start route as the initial connect, sharing one runner rather than a second copy of the orchestration. Methods with a fixed, hand-registered app are unchanged. Fixes #1542 * Add an e2e repro for DCR reconnect after callback-origin drift * Route reconnect by stored client binding, reuse drift-recovery DCR clients, abort automatic connect on modal close * Route reconnect by binding origin, reconcile stored resource with probe, abort before failure fallbacks * Route reconnect by stale client summaries on refresh failure, surface and retry data-less failures * Remove stale-data routing from Reconnect clients view * Fix stale routing comments ---------
Unified · Split
| @@ -0,0 +1,9 @@ | ||
| 1 | +--- | |
| 2 | +"@executor-js/react": patch | |
| 3 | +--- | |
| 4 | + | |
| 5 | +**Reconnecting a DCR connection now re-registers instead of reusing a stranded client** | |
| 6 | + | |
| 7 | +A dynamically registered OAuth client is bound to the redirect URI it registered with. Once the app's callback origin changed (127.0.0.1 to localhost), Reconnect still started the flow against the stored client, and the authorization server rejected it — leaving no way to repair the connection. | |
| 8 | + | |
| 9 | +Reconnect now takes the same probe → CIMD-or-register → start route as the initial connect, so the registration gateway replaces the stranded client against the current redirect URI. Methods with a fixed, hand-registered app are unaffected and keep using their stored client. | |
| @@ -0,0 +1,410 @@ | ||
| 1 | +// Selfhost repro for #1542: a dynamically registered (RFC 7591) OAuth client is | |
| 2 | +// bound to the exact callback URL it registered with the authorization server. | |
| 3 | +// Executor's callback origin is not stable for the life of a connection (a | |
| 4 | +// desktop sidecar on 127.0.0.1 vs a CLI daemon on localhost, or a self-hosted | |
| 5 | +// instance that moves domain), so a DCR client can outlive the origin it was | |
| 6 | +// registered for. Reconnect used to re-send that stranded client on every | |
| 7 | +// attempt — the authorization server answered `invalid_request: redirect_uri | |
| 8 | +// is not registered` forever, and the only exit was deleting the OAuth app by | |
| 9 | +// hand. Add connection recovered (probe → register-dynamic → start reaches the | |
| 10 | +// #1443 reuse gate); Reconnect did not. | |
| 11 | +// | |
| 12 | +// The contract under test: Reconnect takes the same probe → register-dynamic → | |
| 13 | +// start route as the initial connect, so the registration gate declines the | |
| 14 | +// redirect-mismatched client, mints a fresh one bound to the CURRENT callback, | |
| 15 | +// and re-minting rebinds the SAME connection row — no orphaned grant state. | |
| 16 | +import { randomBytes } from "node:crypto"; | |
| 17 | + | |
| 18 | +import { expect } from "@effect/vitest"; | |
| 19 | +import { Effect } from "effect"; | |
| 20 | +import type { Page } from "playwright"; | |
| 21 | +import { composePluginApi } from "@executor-js/api/server"; | |
| 22 | +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; | |
| 23 | +import { | |
| 24 | + AuthTemplateSlug, | |
| 25 | + ConnectionName, | |
| 26 | + IntegrationSlug, | |
| 27 | + OAuthClientSlug, | |
| 28 | +} from "@executor-js/sdk/shared"; | |
| 29 | +import { serveOAuthTestServer } from "@executor-js/sdk/testing"; | |
| 30 | + | |
| 31 | +import { scenario } from "../src/scenario"; | |
| 32 | +import { Api, Browser, Target } from "../src/services"; | |
| 33 | +import { visit } from "../src/surfaces/browser"; | |
| 34 | + | |
| 35 | +const api = composePluginApi([mcpHttpPlugin()] as const); | |
| 36 | + | |
| 37 | +const name = ConnectionName.make("main"); | |
| 38 | +const template = AuthTemplateSlug.make("oauth2"); | |
| 39 | + | |
| 40 | +/** The callback the DCR client was registered under — an origin the app no | |
| 41 | + * longer serves. Only its string identity matters: seeding completes the | |
| 42 | + * authorization out of band, so nothing ever listens here (mirrors the | |
| 43 | + * desktop-sidecar origin from the report). */ | |
| 44 | +const STALE_REDIRECT_URI = "http://127.0.0.1:64999/api/oauth/callback"; | |
| 45 | + | |
| 46 | +/** Client the authorization server knows UP FRONT (`clients` option below) — | |
| 47 | + * the bring-your-own-app case. Reconnect must keep using it directly: routing | |
| 48 | + * it through probe/registration would rebind the connection to an auto-minted | |
| 49 | + * client behind the user's back. */ | |
| 50 | +const STATIC_CLIENT_ID = "static-byo-client"; | |
| 51 | +const STATIC_CLIENT_SECRET = "static-byo-secret"; | |
| 52 | + | |
| 53 | +const connectionsSection = (page: Page) => | |
| 54 | + page.locator("section").filter({ | |
| 55 | + has: page.getByRole("heading", { level: 3, name: "Connections" }), | |
| 56 | + }); | |
| 57 | + | |
| 58 | +const connectionRow = (page: Page, label: string) => | |
| 59 | + connectionsSection(page) | |
| 60 | + .locator('[data-slot="card-stack-entry"]') | |
| 61 | + .filter({ has: page.getByText(label, { exact: true }) }); | |
| 62 | + | |
| 63 | +const requiredRedirect = (response: Response, from: string): string => { | |
| 64 | + const location = response.headers.get("location"); | |
| 65 | + if (!location) { | |
| 66 | + throw new Error(`Expected redirect from ${from}, got HTTP ${response.status}`); | |
| 67 | + } | |
| 68 | + return new URL(location, from).toString(); | |
| 69 | +}; | |
| 70 | + | |
| 71 | +/** The test server's login page is plain text with Basic-auth POST — nothing a | |
| 72 | + * browser can click. Complete it out of band and hand back the callback URL. */ | |
| 73 | +const submitProviderLogin = async (loginUrl: string): Promise<string> => { | |
| 74 | + const credentials = Buffer.from("alice:password").toString("base64"); | |
| 75 | + const response = await fetch(loginUrl, { | |
| 76 | + method: "POST", | |
| 77 | + redirect: "manual", | |
| 78 | + headers: { authorization: `Basic ${credentials}` }, | |
| 79 | + }); | |
| 80 | + const location = response.headers.get("location"); | |
| 81 | + if (response.status !== 302 || !location) { | |
| 82 | + throw new Error(`provider login did not redirect (${response.status})`); | |
| 83 | + } | |
| 84 | + return new URL(location, loginUrl).toString(); | |
| 85 | +}; | |
| 86 | + | |
| 87 | +const completeAuthorization = (authorizationUrl: string) => | |
| 88 | + Effect.promise(async () => { | |
| 89 | + const login = await fetch(authorizationUrl, { redirect: "manual" }); | |
| 90 | + const loginUrl = requiredRedirect(login, authorizationUrl); | |
| 91 | + const callbackUrl = await submitProviderLogin(loginUrl); | |
| 92 | + const parsed = new URL(callbackUrl); | |
| 93 | + const code = parsed.searchParams.get("code"); | |
| 94 | + if (!code) throw new Error(`OAuth callback did not include a code: ${callbackUrl}`); | |
| 95 | + return { code }; | |
| 96 | + }); | |
| 97 | + | |
| 98 | +scenario( | |
| 99 | + "MCP OAuth · reconnect recovers a DCR connection whose callback origin changed", | |
| 100 | + { timeout: 420_000 }, | |
| 101 | + Effect.scoped( | |
| 102 | + Effect.gen(function* () { | |
| 103 | + const target = yield* Target; | |
| 104 | + const browser = yield* Browser; | |
| 105 | + const { client: makeApiClient } = yield* Api; | |
| 106 | + const identity = yield* target.newIdentity(); | |
| 107 | + const client = yield* makeApiClient(api, identity); | |
| 108 | + | |
| 109 | + const oauth = yield* serveOAuthTestServer({ | |
| 110 | + scopes: ["channels:history", "users:read"], | |
| 111 | + clients: { [STATIC_CLIENT_ID]: STATIC_CLIENT_SECRET }, | |
| 112 | + }); | |
| 113 | + | |
| 114 | + const slug = IntegrationSlug.make(`mcp-origin-drift-${randomBytes(4).toString("hex")}`); | |
| 115 | + const clientSlug = OAuthClientSlug.make(`origin-drift-${randomBytes(4).toString("hex")}`); | |
| 116 | + | |
| 117 | + yield* client.mcp.addServer({ | |
| 118 | + payload: { | |
| 119 | + transport: "remote", | |
| 120 | + name: `Origin drift repro ${String(slug)}`, | |
| 121 | + endpoint: oauth.mcpResourceUrl, | |
| 122 | + slug: String(slug), | |
| 123 | + authenticationTemplate: [{ kind: "oauth2" }], | |
| 124 | + }, | |
| 125 | + }); | |
| 126 | + yield* Effect.addFinalizer(() => | |
| 127 | + client.mcp.removeServer({ params: { slug } }).pipe(Effect.ignore), | |
| 128 | + ); | |
| 129 | + // The recovery mints a fresh server-slugged client mid-scenario; the org | |
| 130 | + // is scenario-fresh, so reap every client it accumulated rather than | |
| 131 | + // guessing derived slugs. | |
| 132 | + yield* Effect.addFinalizer(() => | |
| 133 | + client.oauth.listClients().pipe( | |
| 134 | + Effect.flatMap((clients) => | |
| 135 | + Effect.forEach(clients, (candidate) => | |
| 136 | + client.oauth | |
| 137 | + .removeClient({ | |
| 138 | + params: { slug: candidate.slug }, | |
| 139 | + payload: { owner: candidate.owner }, | |
| 140 | + }) | |
| 141 | + .pipe(Effect.ignore), | |
| 142 | + ), | |
| 143 | + ), | |
| 144 | + Effect.ignore, | |
| 145 | + ), | |
| 146 | + ); | |
| 147 | + | |
| 148 | + // Seed the stranded state from the report: the DCR client registers the | |
| 149 | + // STALE callback with the authorization server (as if minted weeks ago | |
| 150 | + // under the old origin), and the connection completes against it out of | |
| 151 | + // band — so the connection is live while its client is bound to a | |
| 152 | + // callback the app no longer serves. | |
| 153 | + const probe = yield* client.oauth.probe({ payload: { url: oauth.mcpResourceUrl } }); | |
| 154 | + if (!probe.registrationEndpoint) { | |
| 155 | + return yield* Effect.die("OAuth probe did not discover a DCR registration endpoint"); | |
| 156 | + } | |
| 157 | + const registered = yield* client.oauth.registerDynamic({ | |
| 158 | + payload: { | |
| 159 | + owner: "org", | |
| 160 | + slug: clientSlug, | |
| 161 | + issuer: probe.issuer ?? null, | |
| 162 | + registrationEndpoint: probe.registrationEndpoint, | |
| 163 | + authorizationUrl: probe.authorizationUrl, | |
| 164 | + tokenUrl: probe.tokenUrl, | |
| 165 | + resource: probe.resource ?? oauth.mcpResourceUrl, | |
| 166 | + scopes: probe.scopesSupported ?? [], | |
| 167 | + tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, | |
| 168 | + clientName: "Executor e2e origin drift repro", | |
| 169 | + redirectUri: STALE_REDIRECT_URI, | |
| 170 | + originIntegration: slug, | |
| 171 | + }, | |
| 172 | + }); | |
| 173 | + const started = yield* client.oauth.start({ | |
| 174 | + payload: { | |
| 175 | + owner: "org", | |
| 176 | + client: registered.client, | |
| 177 | + clientOwner: "org", | |
| 178 | + name, | |
| 179 | + integration: slug, | |
| 180 | + template, | |
| 181 | + redirectUri: STALE_REDIRECT_URI, | |
| 182 | + }, | |
| 183 | + }); | |
| 184 | + expect(started.status, "seeding starts an authorization-code redirect").toBe("redirect"); | |
| 185 | + if (started.status !== "redirect") return yield* Effect.die("OAuth start did not redirect"); | |
| 186 | + const callback = yield* completeAuthorization(started.authorizationUrl); | |
| 187 | + yield* client.oauth.complete({ payload: { state: started.state, code: callback.code } }); | |
| 188 | + yield* Effect.addFinalizer(() => | |
| 189 | + client.connections | |
| 190 | + .remove({ params: { owner: "org", integration: slug, name } }) | |
| 191 | + .pipe(Effect.ignore), | |
| 192 | + ); | |
| 193 | + | |
| 194 | + // Wire truth for the seeded trap: the AS holds exactly one registration, | |
| 195 | + // and it is bound to the stale callback. | |
| 196 | + const seedRequests = yield* oauth.requests; | |
| 197 | + const seedRegistrations = seedRequests.filter( | |
| 198 | + (request) => request.method === "POST" && request.path === "/register", | |
| 199 | + ); | |
| 200 | + expect(seedRegistrations, "seeding registered exactly one client").toHaveLength(1); | |
| 201 | + expect( | |
| 202 | + seedRegistrations[0]?.body ?? "", | |
| 203 | + "the seeded client is bound to the stale callback", | |
| 204 | + ).toContain(STALE_REDIRECT_URI); | |
| 205 | + | |
| 206 | + // A second connection on the SAME discovery-capable integration, backed | |
| 207 | + // by a STATIC (BYO) client the authorization server already knows. Its | |
| 208 | + // seeding start omits `redirectUri`, so it binds to the callback the app | |
| 209 | + // serves NOW — nothing about it needs recovery, and reconnect must leave | |
| 210 | + // its client binding alone. | |
| 211 | + const staticName = ConnectionName.make("static"); | |
| 212 | + const staticSlug = OAuthClientSlug.make(`static-byo-${randomBytes(4).toString("hex")}`); | |
| 213 | + yield* client.oauth.createClient({ | |
| 214 | + payload: { | |
| 215 | + owner: "org", | |
| 216 | + slug: staticSlug, | |
| 217 | + authorizationUrl: probe.authorizationUrl, | |
| 218 | + tokenUrl: probe.tokenUrl, | |
| 219 | + grant: "authorization_code", | |
| 220 | + clientId: STATIC_CLIENT_ID, | |
| 221 | + clientSecret: STATIC_CLIENT_SECRET, | |
| 222 | + originIntegration: slug, | |
| 223 | + }, | |
| 224 | + }); | |
| 225 | + const staticStarted = yield* client.oauth.start({ | |
| 226 | + payload: { | |
| 227 | + owner: "org", | |
| 228 | + client: staticSlug, | |
| 229 | + clientOwner: "org", | |
| 230 | + name: staticName, | |
| 231 | + integration: slug, | |
| 232 | + template, | |
| 233 | + }, | |
| 234 | + }); | |
| 235 | + expect(staticStarted.status, "static seeding starts an authorization-code redirect").toBe( | |
| 236 | + "redirect", | |
| 237 | + ); | |
| 238 | + if (staticStarted.status !== "redirect") { | |
| 239 | + return yield* Effect.die("static OAuth start did not redirect"); | |
| 240 | + } | |
| 241 | + const staticCallback = yield* completeAuthorization(staticStarted.authorizationUrl); | |
| 242 | + yield* client.oauth.complete({ | |
| 243 | + payload: { state: staticStarted.state, code: staticCallback.code }, | |
| 244 | + }); | |
| 245 | + yield* Effect.addFinalizer(() => | |
| 246 | + client.connections | |
| 247 | + .remove({ params: { owner: "org", integration: slug, name: staticName } }) | |
| 248 | + .pipe(Effect.ignore), | |
| 249 | + ); | |
| 250 | + yield* oauth.clearRequests; | |
| 251 | + | |
| 252 | + // One Reconnect journey: open the row's menu, complete the provider | |
| 253 | + // login in the popup, and wait for the success toast. Visits the | |
| 254 | + // integration page first so a toast from an earlier reconnect can never | |
| 255 | + // satisfy this one's wait. | |
| 256 | + const reconnectThroughUi = (label: string, description: string) => | |
| 257 | + browser.session(identity, async ({ page, step }) => { | |
| 258 | + const connections = connectionsSection(page); | |
| 259 | + await step(`Open the MCP integration and find the ${label} connection`, async () => { | |
| 260 | + await visit(page, `/integrations/${String(slug)}`); | |
| 261 | + await connections.getByText(label, { exact: true }).waitFor({ timeout: 30_000 }); | |
| 262 | + }); | |
| 263 | + | |
| 264 | + await step(description, async () => { | |
| 265 | + const popupPromise = page.waitForEvent("popup", { timeout: 30_000 }); | |
| 266 | + await connectionRow(page, label).locator('button[aria-haspopup="menu"]').click(); | |
| 267 | + await page.getByRole("menuitem", { name: "Reconnect" }).click(); | |
| 268 | + const popup = await popupPromise; | |
| 269 | + | |
| 270 | + // #1542's dead end lived here: reconnect re-sent the stranded client, | |
| 271 | + // so the popup landed on the authorization server's `invalid_request: | |
| 272 | + // redirect_uri is not registered` JSON and never reached the login | |
| 273 | + // page. Surface that page instead of a bare timeout when it regresses. | |
| 274 | + try { | |
| 275 | + await popup.waitForURL(/\/login\?/, { timeout: 30_000 }); | |
| 276 | + } catch (cause) { | |
| 277 | + const body = await popup | |
| 278 | + .locator("body") | |
| 279 | + .innerText() | |
| 280 | + .catch(() => "<unreadable>"); | |
| 281 | + throw new Error( | |
| 282 | + `Reconnect dead-ended before the provider login page. ` + | |
| 283 | + `Popup URL: ${popup.url()}; body: ${body}`, | |
| 284 | + { cause }, | |
| 285 | + ); | |
| 286 | + } | |
| 287 | + // The test AS login page is plain text driven by Basic-auth POST, so | |
| 288 | + // complete it out of band and drive the popup to the callback — the | |
| 289 | + // same journey a user's click-through consent takes. | |
| 290 | + const callbackUrl = await submitProviderLogin(popup.url()); | |
| 291 | + await popup.goto(callbackUrl); | |
| 292 | + await page.getByText("Reconnected", { exact: true }).waitFor({ timeout: 30_000 }); | |
| 293 | + }); | |
| 294 | + }); | |
| 295 | + | |
| 296 | + const registrationsIn = <R extends { readonly method: string; readonly path: string }>( | |
| 297 | + requests: readonly R[], | |
| 298 | + ): readonly R[] => | |
| 299 | + requests.filter((request) => request.method === "POST" && request.path === "/register"); | |
| 300 | + const dcrClientCount = Effect.map( | |
| 301 | + client.oauth.listClients(), | |
| 302 | + (clients) => | |
| 303 | + clients.filter((candidate) => candidate.origin.kind === "dynamic_client_registration") | |
| 304 | + .length, | |
| 305 | + ); | |
| 306 | + const mainConnection = client.connections.get({ | |
| 307 | + params: { owner: "org", integration: slug, name }, | |
| 308 | + }); | |
| 309 | + | |
| 310 | + yield* reconnectThroughUi("main", "Reconnect the drifted connection and complete OAuth"); | |
| 311 | + | |
| 312 | + // Wire truth for the recovery: reconnect re-registered EXACTLY ONE client | |
| 313 | + // for the callback the app serves NOW instead of re-sending the stranded | |
| 314 | + // one, and the authorization it started used that fresh registration. | |
| 315 | + const recoveryRequests = yield* oauth.requests; | |
| 316 | + const recoveryRegistrations = registrationsIn(recoveryRequests); | |
| 317 | + expect( | |
| 318 | + recoveryRegistrations, | |
| 319 | + "the drift reconnect registers exactly one fresh client", | |
| 320 | + ).toHaveLength(1); | |
| 321 | + expect( | |
| 322 | + recoveryRegistrations[0]?.body ?? "", | |
| 323 | + "the fresh registration is bound to the current callback, not the stale one", | |
| 324 | + ).not.toContain(STALE_REDIRECT_URI); | |
| 325 | + expect( | |
| 326 | + recoveryRegistrations[0]?.body ?? "", | |
| 327 | + "the fresh registration carries the app's callback path", | |
| 328 | + ).toContain("/api/oauth/callback"); | |
| 329 | + const authorize = recoveryRequests.find( | |
| 330 | + (request) => request.method === "GET" && request.path === "/authorize", | |
| 331 | + ); | |
| 332 | + expect(authorize, "the popup reached the authorize endpoint").toBeDefined(); | |
| 333 | + // The stranded client row deliberately survives (it stays valid for | |
| 334 | + // refresh, and `createClient` must never clobber it) — the recovery adds | |
| 335 | + // exactly ONE freshly bound client rather than editing the stranded one. | |
| 336 | + expect( | |
| 337 | + yield* dcrClientCount, | |
| 338 | + "the stranded DCR client survives alongside exactly one fresh registration", | |
| 339 | + ).toBe(2); | |
| 340 | + const mainAfterRecovery = yield* mainConnection; | |
| 341 | + expect( | |
| 342 | + String(mainAfterRecovery.oauthClient), | |
| 343 | + "the connection is rebound off the stranded client", | |
| 344 | + ).not.toBe(String(clientSlug)); | |
| 345 | + yield* oauth.clearRequests; | |
| 346 | + | |
| 347 | + // A SECOND reconnect at the SAME origin must reuse the recovery client: | |
| 348 | + // zero additional registrations, zero additional client rows, and the | |
| 349 | + // connection keeps the binding the recovery minted. Registering again on | |
| 350 | + // every reconnect after the first drift is the regression class here. | |
| 351 | + yield* reconnectThroughUi("main", "Reconnect again at the same origin"); | |
| 352 | + const repeatRequests = yield* oauth.requests; | |
| 353 | + expect( | |
| 354 | + registrationsIn(repeatRequests), | |
| 355 | + "a reconnect at an unchanged origin registers nothing", | |
| 356 | + ).toHaveLength(0); | |
| 357 | + expect( | |
| 358 | + repeatRequests.filter( | |
| 359 | + (request) => request.method === "GET" && request.path === "/authorize", | |
| 360 | + ), | |
| 361 | + "the repeat reconnect still ran a real authorization", | |
| 362 | + ).toHaveLength(1); | |
| 363 | + expect(yield* dcrClientCount, "no additional DCR client row is minted").toBe(2); | |
| 364 | + const mainAfterRepeat = yield* mainConnection; | |
| 365 | + expect( | |
| 366 | + String(mainAfterRepeat.oauthClient), | |
| 367 | + "the repeat reconnect reuses the recovery client binding", | |
| 368 | + ).toBe(String(mainAfterRecovery.oauthClient)); | |
| 369 | + yield* oauth.clearRequests; | |
| 370 | + | |
| 371 | + // The static (BYO) connection reconnects DIRECTLY through its stored | |
| 372 | + // client: no probe-driven registration, and the binding is untouched — | |
| 373 | + // a discovery-capable integration must not hijack a static binding into | |
| 374 | + // the automatic flow. | |
| 375 | + yield* reconnectThroughUi("static", "Reconnect the static-client connection"); | |
| 376 | + const staticRequests = yield* oauth.requests; | |
| 377 | + expect( | |
| 378 | + registrationsIn(staticRequests), | |
| 379 | + "a static-client reconnect never touches the registration endpoint", | |
| 380 | + ).toHaveLength(0); | |
| 381 | + expect( | |
| 382 | + staticRequests.filter( | |
| 383 | + (request) => request.method === "GET" && request.path === "/authorize", | |
| 384 | + ), | |
| 385 | + "the static reconnect ran a real authorization", | |
| 386 | + ).toHaveLength(1); | |
| 387 | + expect(yield* dcrClientCount, "the static reconnect mints no DCR client").toBe(2); | |
| 388 | + const staticConnection = yield* client.connections.get({ | |
| 389 | + params: { owner: "org", integration: slug, name: staticName }, | |
| 390 | + }); | |
| 391 | + expect( | |
| 392 | + String(staticConnection.oauthClient), | |
| 393 | + "the static connection keeps its BYO client binding", | |
| 394 | + ).toBe(String(staticSlug)); | |
| 395 | + | |
| 396 | + // No orphaned grant state: the SAME connection rows are rebound and both | |
| 397 | + // grants are healthy end to end. | |
| 398 | + const health = yield* client.connections.checkHealth({ | |
| 399 | + params: { owner: "org", integration: slug, name }, | |
| 400 | + query: {}, | |
| 401 | + }); | |
| 402 | + expect(health.status, "the reconnected grant is healthy").toBe("healthy"); | |
| 403 | + const staticHealth = yield* client.connections.checkHealth({ | |
| 404 | + params: { owner: "org", integration: slug, name: staticName }, | |
| 405 | + query: {}, | |
| 406 | + }); | |
| 407 | + expect(staticHealth.status, "the static grant is healthy after reconnect").toBe("healthy"); | |
| 408 | + }), | |
| 409 | + ), | |
| 410 | +); | |
| @@ -168,7 +168,18 @@ export interface IntegrationAccountHandoff { · 167 unmodified lines | ||
| 168 | 168 | readonly clientId?: string; |
| 169 | 169 | readonly authorizationUrl?: string; |
| 170 | 170 | readonly tokenUrl?: string; |
| 171 | − readonly resource?: string; | |
| 171 | + /** RFC 8707 resource indicator. On a reconnect handoff this is the STORED | |
| 172 | + * client's value, and an EXPLICIT null means the stored client was | |
| 173 | + * registered WITHOUT a resource indicator — that absence must survive a | |
| 174 | + * re-registration (some servers reject any `resource` parameter). | |
| 175 | + * Undefined means no stored value was carried. */ | |
| 176 | + readonly resource?: string | null; | |
| 177 | + /** Reconnect only: the stored client binding is an auto-minted DCR client | |
| 178 | + * (or its row is gone), so the modal may re-run the automatic | |
| 179 | + * probe/registration flow. Absent or false pins the reconnect to the | |
| 180 | + * stored client — a static/BYO or first-party binding must never be | |
| 181 | + * silently rebound to an automatic client. */ | |
| 182 | + readonly dynamicRegistration?: boolean; | |
| 172 | 183 | }; |
| 173 | 184 | } |
| 174 | 185 | |
| @@ -656,6 +656,56 @@ describe("oauth.registerDynamicClient", () => { · 655 unmodified lines | ||
| 656 | 656 | ), |
| 657 | 657 | ); |
| 658 | 658 | |
| 659 | + // After the A→B drift recovery above, the owner holds TWO matching-resource | |
| 660 | + // clients: the stale one (bound to redirect A, oldest) and the recovery one | |
| 661 | + // (bound to redirect B). The reuse decision must prefer a candidate matching | |
| 662 | + // resource AND the current redirect across ALL candidates — taking only the | |
| 663 | + // OLDEST matching-resource candidate and then checking its redirect would | |
| 664 | + // mint yet another client on EVERY reconnect after the first drift. | |
| 665 | + it.effect( | |
| 666 | + "reuses the drift-recovery client on later reconnects instead of registering again", | |
| 667 | + () => | |
| 668 | + Effect.scoped( | |
| 669 | + Effect.gen(function* () { | |
| 670 | + const server = yield* serveOAuthTestServer({ scopes: ["read"] }); | |
| 671 | + const { executor } = yield* makeTestWorkspaceHarness({ plugins }); | |
| 672 | + yield* executor.acme.seed(); | |
| 673 | + const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl }); | |
| 674 | + | |
| 675 | + const registerAt = (slug: string, redirectUri: string) => | |
| 676 | + executor.oauth.registerDynamicClient({ | |
| 677 | + owner: "org", | |
| 678 | + slug: OAuthClientSlug.make(slug), | |
| 679 | + issuer: probe.issuer, | |
| 680 | + registrationEndpoint: probe.registrationEndpoint!, | |
| 681 | + authorizationUrl: probe.authorizationUrl, | |
| 682 | + tokenUrl: probe.tokenUrl, | |
| 683 | + resource: probe.resource, | |
| 684 | + scopes: ["read"], | |
| 685 | + tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, | |
| 686 | + clientName: "Acme DCR", | |
| 687 | + redirectUri, | |
| 688 | + originIntegration: INTEG, | |
| 689 | + }); | |
| 690 | + | |
| 691 | + // Original sandbox at redirect A, then the drift recovery at redirect B. | |
| 692 | + yield* registerAt("original-sandbox", FLOW_REDIRECT_URI); | |
| 693 | + const driftedRedirectUri = "https://localhost:6410/api/oauth/callback"; | |
| 694 | + const recovered = yield* registerAt("recreated-sandbox", driftedRedirectUri); | |
| 695 | + yield* server.clearRequests; | |
| 696 | + | |
| 697 | + // A later reconnect at the SAME (current) redirect B: the recovery | |
| 698 | + // client already matches resource + redirect, so it is reused — no | |
| 699 | + // third registration, no third row. | |
| 700 | + const reused = yield* registerAt("later-reconnect", driftedRedirectUri); | |
| 701 | + expect(registerRequestCount(yield* server.requests)).toBe(0); | |
| 702 | + expect(String(reused)).toBe(String(recovered)); | |
| 703 | + const clients = yield* executor.oauth.listClients(); | |
| 704 | + expect(clients).toHaveLength(2); | |
| 705 | + }), | |
| 706 | + ), | |
| 707 | + ); | |
| 708 | + | |
| 659 | 709 | // Regression: Mercury's authorization server vets `client_name` and rejects |
| 660 | 710 | // any value containing its own brand with `invalid_client_metadata`, which |
| 661 | 711 | // the old auto-generated "Executor for Mercury MCP" always tripped. The UI |
| @@ -1144,9 +1144,22 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 1143 unmodified lines | ||
| 1144 | 1144 | // token grant doesn't involve the redirect URI). |
| 1145 | 1145 | const takenSlugs = new Set(candidates.map((client) => String(client.slug))); |
| 1146 | 1146 | if (resource !== null) { |
| 1147 | − const matchingResource = candidates.find((client) => client.resource === resource); | |
| 1148 | − if (matchingResource && redirectMatches(matchingResource)) { | |
| 1149 | − return { existingSlug: matchingResource.slug, registrationSlug: matchingResource.slug }; | |
| 1147 | + // Prefer a candidate matching resource AND the current redirect across | |
| 1148 | + // ALL candidates (mirroring the resource-less branch below). Candidates | |
| 1149 | + // are oldest-first, so after an origin drift the oldest matching- | |
| 1150 | + // resource row is the STRANDED one — but the first drift recovery | |
| 1151 | + // already minted a client bound to the CURRENT callback, and later | |
| 1152 | + // reconnects must reuse that instead of registering another duplicate | |
| 1153 | + // each time. Known limitation: the legacy null-redirect rule in | |
| 1154 | + // `redirectMatches` (a legacy row with no stored redirect matches any | |
| 1155 | + // flow redirect) still lets such a row win over a later, exactly- | |
| 1156 | + // matching one; kept deliberately so upgrades don't re-register every | |
| 1157 | + // client whose callback never changed. | |
| 1158 | + const reusable = candidates.find( | |
| 1159 | + (client) => client.resource === resource && redirectMatches(client), | |
| 1160 | + ); | |
| 1161 | + if (reusable) { | |
| 1162 | + return { existingSlug: reusable.slug, registrationSlug: reusable.slug }; | |
| 1150 | 1163 | } |
| 1151 | 1164 | const slug = uniqueDcrSlug( |
| 1152 | 1165 | dcrClientSlug(issuer, candidates.length > 0 ? resource : null, input.slug), |
| @@ -1,14 +1,20 @@ | ||
| 1 | 1 | import { useEffect, useMemo, useState } from "react"; |
| 2 | −import { useAtomValue, useAtomSet } from "@effect/atom-react"; | |
| 2 | +import { useAtomValue, useAtomRefresh, useAtomSet } from "@effect/atom-react"; | |
| 3 | 3 | import * as AsyncResult from "effect/unstable/reactivity/AsyncResult"; |
| 4 | 4 | import * as Exit from "effect/Exit"; |
| 5 | −import { IntegrationSlug, type Connection, type Owner } from "@executor-js/sdk/shared"; | |
| 5 | +import { | |
| 6 | + IntegrationSlug, | |
| 7 | + type Connection, | |
| 8 | + type OAuthClientSummary, | |
| 9 | + type Owner, | |
| 10 | +} from "@executor-js/sdk/shared"; | |
| 6 | 11 | import type { IntegrationAccountHandoff } from "@executor-js/sdk/client"; |
| 7 | 12 | import { toast } from "sonner"; |
| 8 | 13 | |
| 9 | 14 | import { |
| 10 | 15 | addConnectionOptimistic, |
| 11 | 16 | connectionsForIntegrationAtom, |
| 17 | + oauthClientsOptimisticAtom, | |
| 12 | 18 | refreshConnection, |
| 13 | 19 | removeConnectionOptimistic, |
| 14 | 20 | startOAuth, |
| @@ -23,11 +29,14 @@ import type { AuthMethod } from "../lib/auth-placements"; · 8 unmodified lines | ||
| 23 | 29 | import { |
| 24 | 30 | connectionNeedsReconsent, |
| 25 | 31 | oauthReconnectPayload, |
| 32 | + reconnectClientsView, | |
| 26 | 33 | reconnectMode, |
| 34 | + reconnectRoute, | |
| 27 | 35 | reconsentRequiredScopes, |
| 36 | + retryReconnectClientsOnMenuOpen, | |
| 28 | 37 | } from "../plugins/oauth-reconnect"; |
| 29 | 38 | import { useOAuthPopupFlow } from "../plugins/oauth-sign-in"; |
| 30 | −import { AddAccountModal } from "./add-account-modal"; | |
| 39 | +import { AddAccountModal, hasDcr } from "./add-account-modal"; | |
| 31 | 40 | import { ConnectionEditSheet } from "./metadata-edit-sheet"; |
| 32 | 41 | import type { CreateCustomMethod } from "./add-custom-method-modal"; |
| 33 | 42 | import { |
| @@ -117,6 +126,19 @@ function AccountRow(props: { · 83 unmodified lines | ||
| 117 | 126 | readonly showOwnerLabel: boolean; |
| 118 | 127 | readonly onEdit: () => void; |
| 119 | 128 | readonly onReconnect: () => void; |
| 129 | + /** Reconnect routing needs the stored client binding; while the client | |
| 130 | + * summaries carry no data the route is unknown, so the action is | |
| 131 | + * disabled rather than guessed (same idiom as "Check now" above). */ | |
| 132 | + readonly reconnectDisabled: boolean; | |
| 133 | + /** The summaries query failed: the Reconnect item stays disabled but says | |
| 134 | + * so (never a silently dead action). Retained stale data never routes — a | |
| 135 | + * binding changed since the snapshot could misroute. Opening the menu | |
| 136 | + * retries the query via `onMenuOpenChange`, so the hint reflects a retry | |
| 137 | + * that just failed, not a permanently stuck state. */ | |
| 138 | + readonly reconnectFailed: boolean; | |
| 139 | + /** Forwarded to the row menu; the owner uses the OPEN transition to retry | |
| 140 | + * a failed client-summaries query. */ | |
| 141 | + readonly onMenuOpenChange: (open: boolean) => void; | |
| 120 | 142 | readonly onRemove: () => void; |
| 121 | 143 | }) { |
| 122 | 144 | const { connection, needsReconsent } = props; |
| @@ -245,7 +267,7 @@ function AccountRow(props: { · 122 unmodified lines | ||
| 245 | 267 | {props.showOwnerLabel ? ( |
| 246 | 268 | <Badge variant="outline">{ownerLabel(connection.owner)}</Badge> |
| 247 | 269 | ) : null} |
| 248 | − <DropdownMenu> | |
| 270 | + <DropdownMenu onOpenChange={props.onMenuOpenChange}> | |
| 249 | 271 | <DropdownMenuTrigger asChild> |
| 250 | 272 | <Button |
| 251 | 273 | variant="ghost" |
| @@ -270,8 +292,17 @@ function AccountRow(props: { · 18 unmodified lines | ||
| 270 | 292 | <DropdownMenuItem className="text-sm" onClick={props.onEdit}> |
| 271 | 293 | Edit |
| 272 | 294 | </DropdownMenuItem> |
| 273 | − <DropdownMenuItem className="text-sm" onClick={props.onReconnect}> | |
| 295 | + <DropdownMenuItem | |
| 296 | + className="text-sm" | |
| 297 | + disabled={props.reconnectDisabled} | |
| 298 | + onClick={props.onReconnect} | |
| 299 | + > | |
| 274 | 300 | Reconnect |
| 301 | + {props.reconnectFailed ? ( | |
| 302 | + // Same failed-query voice as the modal's picker errors; the | |
| 303 | + // trailing placement mirrors DropdownMenuShortcut. | |
| 304 | + <span className="ml-auto text-xs text-destructive">Failed to load</span> | |
| 305 | + ) : null} | |
| 275 | 306 | </DropdownMenuItem> |
| 276 | 307 | <DropdownMenuItem variant="destructive" className="text-sm" onClick={props.onRemove}> |
| 277 | 308 | Remove |
| @@ -289,13 +320,29 @@ function OwnerAccounts(props: { · 11 unmodified lines | ||
| 289 | 320 | readonly showOwnerLabels: boolean; |
| 290 | 321 | readonly methods: readonly AuthMethod[]; |
| 291 | 322 | readonly onEdit: (connection: Connection) => void; |
| 292 | − readonly onDcrReconnect: (connection: Connection) => void; | |
| 323 | + /** Hand the connection to the modal's automatic reconnect flow. Only called | |
| 324 | + * once the stored binding was vetted as auto-minted DCR (or gone); | |
| 325 | + * `storedClient` is that binding's summary — undefined when the row is | |
| 326 | + * gone — so the handoff can carry its resource. */ | |
| 327 | + readonly onDcrReconnect: ( | |
| 328 | + connection: Connection, | |
| 329 | + storedClient: OAuthClientSummary | undefined, | |
| 330 | + ) => void; | |
| 293 | 331 | /** The integration's declared oauth scopes — compared against each connection's |
| 294 | 332 | * granted `oauthScope` to flag connections that must reconnect for new access. */ |
| 295 | 333 | readonly declaredScopes: readonly string[] | undefined; |
| 296 | 334 | }) { |
| 297 | 335 | const { integration, owner } = props; |
| 298 | 336 | const connections = useAtomValue(connectionsForIntegrationAtom({ integration, owner })); |
| 337 | + // Registered-app summaries: the Reconnect routing below inspects the STORED | |
| 338 | + // client binding (its origin kind and resource), not just the method's | |
| 339 | + // capability, before sending a connection into the automatic flow. Routing | |
| 340 | + // only ever reads a current successful load — never stale data — and a | |
| 341 | + // failed query marks the action failed (recoverable — opening the row menu | |
| 342 | + // retries the query). | |
| 343 | + const allClients = useAtomValue(oauthClientsOptimisticAtom); | |
| 344 | + const refreshClients = useAtomRefresh(oauthClientsOptimisticAtom); | |
| 345 | + const clientsView = reconnectClientsView(allClients); | |
| 299 | 346 | // Removal confirms in a dialog. State lives here (not in the row) because the |
| 300 | 347 | // Remove menu item closes its dropdown on click, which would unmount a dialog |
| 301 | 348 | // nested inside it — so the row only nominates the connection to remove. |
| @@ -327,11 +374,26 @@ function OwnerAccounts(props: { · 25 unmodified lines | ||
| 327 | 374 | (candidate: AuthMethod) => |
| 328 | 375 | candidate.kind === "oauth" && String(candidate.template) === String(connection.template), |
| 329 | 376 | ); |
| 330 | − if ( | |
| 331 | − method?.oauth?.supportsDynamicRegistration === true || | |
| 332 | − method?.oauth?.discoveryUrl != null | |
| 333 | − ) { | |
| 334 | − props.onDcrReconnect(connection); | |
| 377 | + // Route by the STORED binding's origin (`reconnectRoute`): an | |
| 378 | + // auto-minted DCR binding re-runs the automatic probe/registration flow | |
| 379 | + // (direct reuse dead-ends once the callback origin drifts, #1542); a | |
| 380 | + // static/BYO or first-party binding takes the direct path below even on | |
| 381 | + // a discovery-capable integration — re-registering would silently | |
| 382 | + // rebind the connection to an automatic client. Unless the client list | |
| 383 | + // is a CURRENT success the binding is UNKNOWN and no route may be | |
| 384 | + // chosen: the menu item is disabled until then, and this guard | |
| 385 | + // backstops a race — a permanent wrong choice on a guess is never | |
| 386 | + // acceptable. A failed refresh never routes, even when stale data is | |
| 387 | + // retained: a binding changed since the snapshot could repeat the | |
| 388 | + // origin-drift dead end or silently rebind to an automatic client. | |
| 389 | + const route = reconnectRoute( | |
| 390 | + clientsView.kind === "ready" ? clientsView.clients : undefined, | |
| 391 | + connection, | |
| 392 | + hasDcr(method), | |
| 393 | + ); | |
| 394 | + if (route.kind === "unknown") return; | |
| 395 | + if (route.kind === "automatic") { | |
| 396 | + props.onDcrReconnect(connection, route.stored); | |
| 335 | 397 | return; |
| 336 | 398 | } |
| 337 | 399 | const payload = oauthReconnectPayload(connection); |
| @@ -451,6 +513,19 @@ function OwnerAccounts(props: { · 113 unmodified lines | ||
| 451 | 513 | showOwnerLabel={props.showOwnerLabels} |
| 452 | 514 | onEdit={() => props.onEdit(connection)} |
| 453 | 515 | onReconnect={() => void handleReconnect(connection)} |
| 516 | + // An OAuth Reconnect routes by the stored client binding; without | |
| 517 | + // a current successful load (loading or failed) the route is | |
| 518 | + // unknown, so the action waits. Static-credential rows refresh | |
| 519 | + // without the binding. | |
| 520 | + reconnectDisabled={ | |
| 521 | + reconnectMode(connection) === "oauth" && clientsView.kind !== "ready" | |
| 522 | + } | |
| 523 | + // A failed load is surfaced on the item (not silently disabled) | |
| 524 | + // and recovers on menu open, which retries the query. | |
| 525 | + reconnectFailed={reconnectMode(connection) === "oauth" && clientsView.kind === "failed"} | |
| 526 | + onMenuOpenChange={(open: boolean) => | |
| 527 | + retryReconnectClientsOnMenuOpen(open, clientsView, refreshClients) | |
| 528 | + } | |
| 454 | 529 | onRemove={() => setRemovingConnection(connection)} |
| 455 | 530 | /> |
| 456 | 531 | ))} |
| @@ -632,7 +707,10 @@ export function AccountsSection(props: { · 175 unmodified lines | ||
| 632 | 707 | showOwnerLabels={ownerDisplay.showOwnerLabels} |
| 633 | 708 | methods={methods} |
| 634 | 709 | onEdit={setEditingConnection} |
| 635 | − onDcrReconnect={(connection: Connection) => { | |
| 710 | + onDcrReconnect={( | |
| 711 | + connection: Connection, | |
| 712 | + storedClient: OAuthClientSummary | undefined, | |
| 713 | + ) => { | |
| 636 | 714 | if (connection.oauthClient == null) return; |
| 637 | 715 | setReconnectHandoff({ |
| 638 | 716 | key: `reconnect:${connection.owner}:${String(connection.integration)}:${String( |
| @@ -648,6 +726,19 @@ export function AccountsSection(props: { · 9 unmodified lines | ||
| 648 | 726 | action: "reconnect", |
| 649 | 727 | slug: String(connection.oauthClient), |
| 650 | 728 | owner: connection.oauthClientOwner ?? connection.owner, |
| 729 | + // Vetted by the routing in `handleReconnect`: the stored | |
| 730 | + // binding is auto-minted DCR (or its row is gone), so the | |
| 731 | + // modal may re-run the automatic flow. | |
| 732 | + dynamicRegistration: true, | |
| 733 | + // The stored client's RFC 8707 resource — an EXPLICIT null | |
| 734 | + // for a client registered WITHOUT a resource indicator | |
| 735 | + // (that absence always survives re-registration); a stored | |
| 736 | + // value is reconciled against the probe downstream, so a | |
| 737 | + // migrated resource follows the server. Omitted when the | |
| 738 | + // row is gone. | |
| 739 | + ...(storedClient !== undefined | |
| 740 | + ? { resource: storedClient.resource ?? null } | |
| 741 | + : {}), | |
| 651 | 742 | }, |
| 652 | 743 | }); |
| 653 | 744 | }} |
| @@ -17,6 +17,7 @@ import { · 16 unmodified lines | ||
| 17 | 17 | connectionLabelForHost, |
| 18 | 18 | createCredentialPayloadOrigin, |
| 19 | 19 | DEFAULT_CONNECTION_OWNER, |
| 20 | + hasDcr, | |
| 20 | 21 | mergeCustomMethods, |
| 21 | 22 | oauthIdentityLabelFromHealth, |
| 22 | 23 | runAutomaticOAuthConnect, |
| @@ -85,13 +86,16 @@ type AutomaticOAuthDeps = Parameters<typeof runAutomaticOAuthConnect>[0]; · 62 unmodified lines | ||
| 85 | 86 | type AutomaticOAuthInput = Parameters<typeof runAutomaticOAuthConnect>[1]; |
| 86 | 87 | |
| 87 | 88 | /** DCR-focused tests use defaults for the CIMD branch they intentionally do |
| 88 | − * not exercise. Discovery-level tests call the orchestrator directly. */ | |
| 89 | + * not exercise (and an always-mounted surface unless a test closes it). | |
| 90 | + * Discovery-level tests call the orchestrator directly. */ | |
| 89 | 91 | const runDcrConnect = ( |
| 90 | − deps: Omit<AutomaticOAuthDeps, "createCimdClient">, | |
| 92 | + deps: Omit<AutomaticOAuthDeps, "createCimdClient" | "isActive"> & | |
| 93 | + Partial<Pick<AutomaticOAuthDeps, "isActive">>, | |
| 91 | 94 | input: Omit<AutomaticOAuthInput, "cimd">, |
| 92 | 95 | ) => |
| 93 | 96 | runAutomaticOAuthConnect( |
| 94 | 97 | { |
| 98 | + isActive: (): boolean => true, | |
| 95 | 99 | ...deps, |
| 96 | 100 | createCimdClient: (): Promise<OAuthClientSlug | null> => Promise.resolve(null), |
| 97 | 101 | }, |
| @@ -451,6 +455,7 @@ describe("runAutomaticOAuthConnect", () => { · 353 unmodified lines | ||
| 451 | 455 | const outcome = await runAutomaticOAuthConnect( |
| 452 | 456 | { |
| 453 | 457 | ...popup, |
| 458 | + isActive: (): boolean => true, | |
| 454 | 459 | probe: (): Promise<ProbeResult> => { |
| 455 | 460 | calls.push("probe"); |
| 456 | 461 | return Promise.resolve({ |
| @@ -499,6 +504,409 @@ describe("runAutomaticOAuthConnect", () => { · 42 unmodified lines | ||
| 499 | 504 | }); |
| 500 | 505 | }); |
| 501 | 506 | |
| 507 | +// --------------------------------------------------------------------------- | |
| 508 | +// Reconnect (issue #1542). A DCR client is bound to the redirect URI it | |
| 509 | +// registered with, so when the app's callback origin moves (127.0.0.1 -> | |
| 510 | +// localhost) re-authorizing against the STORED client is rejected by the | |
| 511 | +// authorization server and the connection can never be repaired. Reconnect | |
| 512 | +// therefore takes the same probe -> (CIMD | register) -> start route as the | |
| 513 | +// initial connect, which re-registers against the CURRENT redirect URI. | |
| 514 | +// | |
| 515 | +// `hasDcr` is the routing decision the modal's reconnect handoff makes; the | |
| 516 | +// orchestrator run below is what that decision buys. | |
| 517 | +// --------------------------------------------------------------------------- | |
| 518 | +describe("hasDcr (which methods reconnect through the automatic path)", () => { | |
| 519 | + const oauthMethod = (oauth: NonNullable<AuthMethod["oauth"]>): AuthMethod => ({ | |
| 520 | + id: "oauth", | |
| 521 | + label: "OAuth", | |
| 522 | + kind: "oauth", | |
| 523 | + source: "spec", | |
| 524 | + template: AuthTemplateSlug.make("oauth"), | |
| 525 | + placements: [{ carrier: "header", name: "Authorization", prefix: "Bearer " }], | |
| 526 | + oauth, | |
| 527 | + }); | |
| 528 | + | |
| 529 | + it("routes a method advertising dynamic registration", () => { | |
| 530 | + expect(hasDcr(oauthMethod({ supportsDynamicRegistration: true }))).toBe(true); | |
| 531 | + }); | |
| 532 | + | |
| 533 | + it("routes a method carrying a discovery URL we can probe at connect time", () => { | |
| 534 | + expect(hasDcr(oauthMethod({ discoveryUrl: "https://mcp.example.com/mcp" }))).toBe(true); | |
| 535 | + }); | |
| 536 | + | |
| 537 | + // A fixed, hand-registered app has no stranded-client problem: its redirect | |
| 538 | + // URI is whatever the human entered, so reconnect keeps using it directly. | |
| 539 | + it("leaves a plain registered-app OAuth method on the stored-client path", () => { | |
| 540 | + expect(hasDcr(oauthMethod({ authorizationUrl: "https://auth.example.com/authorize" }))).toBe( | |
| 541 | + false, | |
| 542 | + ); | |
| 543 | + expect(hasDcr(oauthMethod({ supportsDynamicRegistration: false }))).toBe(false); | |
| 544 | + }); | |
| 545 | + | |
| 546 | + it("never routes a non-OAuth or absent method", () => { | |
| 547 | + expect(hasDcr(apiKeyMethod("api", "spec"))).toBe(false); | |
| 548 | + expect(hasDcr(undefined)).toBe(false); | |
| 549 | + expect(hasDcr(null)).toBe(false); | |
| 550 | + }); | |
| 551 | +}); | |
| 552 | + | |
| 553 | +describe("runAutomaticOAuthConnect (reconnect)", () => { | |
| 554 | + // The fix for #1542: reconnect must MINT a client against the redirect URI in | |
| 555 | + // force now, not reuse the one the connection was originally bound to. | |
| 556 | + it("re-registers against the current redirect URI and starts on the fresh client", async () => { | |
| 557 | + const popup = popupSpy(); | |
| 558 | + let registerArgs: RegisterArgs | null = null; | |
| 559 | + let startArgs: StartArgs | null = null; | |
| 560 | + | |
| 561 | + const outcome = await runDcrConnect( | |
| 562 | + { | |
| 563 | + ...popup, | |
| 564 | + probe: (): Promise<ProbeResult> => | |
| 565 | + Promise.resolve({ | |
| 566 | + authorizationUrl: "https://auth.example.com/authorize", | |
| 567 | + tokenUrl: "https://auth.example.com/token", | |
| 568 | + registrationEndpoint: "https://auth.example.com/register", | |
| 569 | + }), | |
| 570 | + register: (args: RegisterArgs): Promise<OAuthClientSlug> => { | |
| 571 | + registerArgs = args; | |
| 572 | + return Promise.resolve(OAuthClientSlug.make("reconnected-app")); | |
| 573 | + }, | |
| 574 | + start: (args: StartArgs): void => { | |
| 575 | + startArgs = args; | |
| 576 | + }, | |
| 577 | + }, | |
| 578 | + { | |
| 579 | + discoveryUrl: "https://mcp.example.com/mcp", | |
| 580 | + // The connection was registered under the old origin; this is the one | |
| 581 | + // the app serves its callback on now. | |
| 582 | + redirectUri: "http://localhost:4788/api/oauth/callback", | |
| 583 | + owner: "user" as Owner, | |
| 584 | + integration: TEST_INTEGRATION, | |
| 585 | + }, | |
| 586 | + ); | |
| 587 | + | |
| 588 | + expect(outcome).toEqual({ kind: "started", flow: "dcr" }); | |
| 589 | + expect(registerArgs!.redirectUri).toBe("http://localhost:4788/api/oauth/callback"); | |
| 590 | + // The stranded client is replaced, not reused. | |
| 591 | + expect(startArgs!.client).toBe(OAuthClientSlug.make("reconnected-app")); | |
| 592 | + expect(startArgs!.owner).toBe("user"); | |
| 593 | + }); | |
| 594 | + | |
| 595 | + // Reconnect keeps the BYO picker as its recovery path, exactly as connect | |
| 596 | + // does, rather than dead-ending on a server that cannot self-register. | |
| 597 | + it("falls back with the probe when the server advertises no registration endpoint", async () => { | |
| 598 | + const popup = popupSpy(); | |
| 599 | + const outcome = await runDcrConnect( | |
| 600 | + { | |
| 601 | + ...popup, | |
| 602 | + probe: (): Promise<ProbeResult> => | |
| 603 | + Promise.resolve({ | |
| 604 | + authorizationUrl: "https://auth.example.com/authorize", | |
| 605 | + tokenUrl: "https://auth.example.com/token", | |
| 606 | + }), | |
| 607 | + register: (): Promise<OAuthClientSlug> => | |
| 608 | + Promise.resolve(OAuthClientSlug.make("unexpected")), | |
| 609 | + start: (): void => {}, | |
| 610 | + }, | |
| 611 | + { | |
| 612 | + discoveryUrl: "https://mcp.example.com/mcp", | |
| 613 | + redirectUri: "http://localhost:4788/api/oauth/callback", | |
| 614 | + owner: "user" as Owner, | |
| 615 | + integration: TEST_INTEGRATION, | |
| 616 | + }, | |
| 617 | + ); | |
| 618 | + | |
| 619 | + expect(outcome).toMatchObject({ kind: "fallback", reason: "no-registration-endpoint" }); | |
| 620 | + expect(popup.calls).toEqual(["reserve", "release"]); | |
| 621 | + }); | |
| 622 | + | |
| 623 | + // #1822: a stored client registered WITHOUT a resource indicator (explicit | |
| 624 | + // null) must stay resource-less through a reconnect — the probe's advertised | |
| 625 | + // resource (or the discovery-URL fallback) must not resurrect the parameter | |
| 626 | + // some servers reject. | |
| 627 | + it("preserves the stored client's explicit resource absence through re-registration", async () => { | |
| 628 | + let registerArgs: RegisterArgs | null = null; | |
| 629 | + const outcome = await runDcrConnect( | |
| 630 | + { | |
| 631 | + ...popupSpy(), | |
| 632 | + probe: (): Promise<ProbeResult> => | |
| 633 | + Promise.resolve({ | |
| 634 | + authorizationUrl: "https://auth.example.com/authorize", | |
| 635 | + tokenUrl: "https://auth.example.com/token", | |
| 636 | + registrationEndpoint: "https://auth.example.com/register", | |
| 637 | + resource: "https://mcp.example.com/mcp", | |
| 638 | + }), | |
| 639 | + register: (args: RegisterArgs): Promise<OAuthClientSlug> => { | |
| 640 | + registerArgs = args; | |
| 641 | + return Promise.resolve(OAuthClientSlug.make("reconnected-app")); | |
| 642 | + }, | |
| 643 | + start: (): void => {}, | |
| 644 | + }, | |
| 645 | + { | |
| 646 | + discoveryUrl: "https://mcp.example.com/mcp", | |
| 647 | + resourceFallback: "https://mcp.example.com/mcp", | |
| 648 | + redirectUri: "http://localhost:4788/api/oauth/callback", | |
| 649 | + owner: "user" as Owner, | |
| 650 | + integration: TEST_INTEGRATION, | |
| 651 | + storedResource: null, | |
| 652 | + }, | |
| 653 | + ); | |
| 654 | + | |
| 655 | + expect(outcome).toEqual({ kind: "started", flow: "dcr" }); | |
| 656 | + expect(registerArgs!.resource).toBeNull(); | |
| 657 | + }); | |
| 658 | + | |
| 659 | + // The server MIGRATED its protected resource (R1 → R2) since the client was | |
| 660 | + // stored: the probe's freshly advertised value is the truth, so the stored | |
| 661 | + // one must not pin the reconnect to the old resource forever. | |
| 662 | + it("follows a migrated resource: the probe's advertised value beats the stored one", async () => { | |
| 663 | + let registerArgs: RegisterArgs | null = null; | |
| 664 | + await runDcrConnect( | |
| 665 | + { | |
| 666 | + ...popupSpy(), | |
| 667 | + probe: (): Promise<ProbeResult> => | |
| 668 | + Promise.resolve({ | |
| 669 | + authorizationUrl: "https://auth.example.com/authorize", | |
| 670 | + tokenUrl: "https://auth.example.com/token", | |
| 671 | + registrationEndpoint: "https://auth.example.com/register", | |
| 672 | + resource: "https://probed.example.com/mcp", | |
| 673 | + }), | |
| 674 | + register: (args: RegisterArgs): Promise<OAuthClientSlug> => { | |
| 675 | + registerArgs = args; | |
| 676 | + return Promise.resolve(OAuthClientSlug.make("reconnected-app")); | |
| 677 | + }, | |
| 678 | + start: (): void => {}, | |
| 679 | + }, | |
| 680 | + { | |
| 681 | + discoveryUrl: "https://mcp.example.com/mcp", | |
| 682 | + redirectUri: "http://localhost:4788/api/oauth/callback", | |
| 683 | + owner: "user" as Owner, | |
| 684 | + integration: TEST_INTEGRATION, | |
| 685 | + storedResource: "https://stored.example.com/mcp", | |
| 686 | + }, | |
| 687 | + ); | |
| 688 | + | |
| 689 | + expect(registerArgs!.resource).toBe("https://probed.example.com/mcp"); | |
| 690 | + }); | |
| 691 | + | |
| 692 | + // When the probe advertises NO resource, the stored one stands — including | |
| 693 | + // over the discovery-URL fallback, which is our own guess, not the server's. | |
| 694 | + it("keeps the stored resource when the probe advertises none", async () => { | |
| 695 | + let registerArgs: RegisterArgs | null = null; | |
| 696 | + await runDcrConnect( | |
| 697 | + { | |
| 698 | + ...popupSpy(), | |
| 699 | + probe: (): Promise<ProbeResult> => | |
| 700 | + Promise.resolve({ | |
| 701 | + authorizationUrl: "https://auth.example.com/authorize", | |
| 702 | + tokenUrl: "https://auth.example.com/token", | |
| 703 | + registrationEndpoint: "https://auth.example.com/register", | |
| 704 | + }), | |
| 705 | + register: (args: RegisterArgs): Promise<OAuthClientSlug> => { | |
| 706 | + registerArgs = args; | |
| 707 | + return Promise.resolve(OAuthClientSlug.make("reconnected-app")); | |
| 708 | + }, | |
| 709 | + start: (): void => {}, | |
| 710 | + }, | |
| 711 | + { | |
| 712 | + discoveryUrl: "https://mcp.example.com/mcp", | |
| 713 | + resourceFallback: "https://mcp.example.com/mcp", | |
| 714 | + redirectUri: "http://localhost:4788/api/oauth/callback", | |
| 715 | + owner: "user" as Owner, | |
| 716 | + integration: TEST_INTEGRATION, | |
| 717 | + storedResource: "https://stored.example.com/mcp", | |
| 718 | + }, | |
| 719 | + ); | |
| 720 | + | |
| 721 | + expect(registerArgs!.resource).toBe("https://stored.example.com/mcp"); | |
| 722 | + }); | |
| 723 | +}); | |
| 724 | + | |
| 725 | +describe("runAutomaticOAuthConnect (modal closed mid-flight)", () => { | |
| 726 | + const probeOk = (): Promise<ProbeResult> => | |
| 727 | + Promise.resolve({ | |
| 728 | + authorizationUrl: "https://auth.example.com/authorize", | |
| 729 | + tokenUrl: "https://auth.example.com/token", | |
| 730 | + registrationEndpoint: "https://auth.example.com/register", | |
| 731 | + }); | |
| 732 | + const input = { | |
| 733 | + discoveryUrl: "https://mcp.example.com/mcp", | |
| 734 | + redirectUri: "http://localhost:4788/api/oauth/callback", | |
| 735 | + owner: "user" as Owner, | |
| 736 | + integration: TEST_INTEGRATION, | |
| 737 | + }; | |
| 738 | + | |
| 739 | + // Closing the modal unmounts the view, but the awaited sequence keeps | |
| 740 | + // running. It must stop at the next checkpoint: nothing registered, nothing | |
| 741 | + // launched, the claimed window given back. | |
| 742 | + it("stops after the probe: no client is registered and no popup launches", async () => { | |
| 743 | + const popup = popupSpy(); | |
| 744 | + let active = true; | |
| 745 | + let registered = 0; | |
| 746 | + const outcome = await runDcrConnect( | |
| 747 | + { | |
| 748 | + ...popup, | |
| 749 | + isActive: (): boolean => active, | |
| 750 | + probe: (): Promise<ProbeResult> => { | |
| 751 | + // The modal closes while the probe is in flight. | |
| 752 | + active = false; | |
| 753 | + return probeOk(); | |
| 754 | + }, | |
| 755 | + register: (): Promise<OAuthClientSlug> => { | |
| 756 | + registered += 1; | |
| 757 | + return Promise.resolve(OAuthClientSlug.make("mcp-app")); | |
| 758 | + }, | |
| 759 | + start: (): void => { | |
| 760 | + popup.calls.push("start"); | |
| 761 | + }, | |
| 762 | + }, | |
| 763 | + input, | |
| 764 | + ); | |
| 765 | + | |
| 766 | + expect(outcome).toEqual({ kind: "aborted" }); | |
| 767 | + expect(registered).toBe(0); | |
| 768 | + expect(popup.calls).toEqual(["reserve", "release"]); | |
| 769 | + }); | |
| 770 | + | |
| 771 | + // A close racing the registration itself cannot unmint the client (no | |
| 772 | + // server-side cancel exists; the row is inert, reusable DCR plumbing), but | |
| 773 | + // the sign-in popup must never launch after the modal is gone. | |
| 774 | + it("never launches the popup when the close raced the registration", async () => { | |
| 775 | + const popup = popupSpy(); | |
| 776 | + let active = true; | |
| 777 | + const outcome = await runDcrConnect( | |
| 778 | + { | |
| 779 | + ...popup, | |
| 780 | + isActive: (): boolean => active, | |
| 781 | + probe: probeOk, | |
| 782 | + register: (): Promise<OAuthClientSlug> => { | |
| 783 | + // The modal closes while the registration is in flight. | |
| 784 | + active = false; | |
| 785 | + return Promise.resolve(OAuthClientSlug.make("mcp-app")); | |
| 786 | + }, | |
| 787 | + start: (): void => { | |
| 788 | + popup.calls.push("start"); | |
| 789 | + }, | |
| 790 | + }, | |
| 791 | + input, | |
| 792 | + ); | |
| 793 | + | |
| 794 | + expect(outcome).toEqual({ kind: "aborted" }); | |
| 795 | + expect(popup.calls).toEqual(["reserve", "release"]); | |
| 796 | + }); | |
| 797 | + | |
| 798 | + it("never starts a CIMD flow after the close", async () => { | |
| 799 | + const popup = popupSpy(); | |
| 800 | + let active = true; | |
| 801 | + const outcome = await runAutomaticOAuthConnect( | |
| 802 | + { | |
| 803 | + ...popup, | |
| 804 | + isActive: (): boolean => active, | |
| 805 | + probe: (): Promise<ProbeResult> => | |
| 806 | + Promise.resolve({ | |
| 807 | + authorizationUrl: "https://auth.example.com/authorize", | |
| 808 | + tokenUrl: "https://auth.example.com/token", | |
| 809 | + clientIdMetadataDocumentSupported: true, | |
| 810 | + }), | |
| 811 | + createCimdClient: (args: CimdCreateArgs): Promise<OAuthClientSlug> => { | |
| 812 | + // The modal closes while the local CIMD client is being minted. | |
| 813 | + active = false; | |
| 814 | + return Promise.resolve(args.slug); | |
| 815 | + }, | |
| 816 | + register: (): Promise<OAuthClientSlug> => | |
| 817 | + Promise.resolve(OAuthClientSlug.make("unexpected")), | |
| 818 | + start: (): void => { | |
| 819 | + popup.calls.push("start"); | |
| 820 | + }, | |
| 821 | + }, | |
| 822 | + { | |
| 823 | + discoveryUrl: "https://mcp.example.com/mcp", | |
| 824 | + owner: "user" as Owner, | |
| 825 | + integration: TEST_INTEGRATION, | |
| 826 | + cimd: { | |
| 827 | + integrationName: "Test MCP", | |
| 828 | + clientIdMetadataDocumentUrl: "https://executor.example/api/oauth/client-id-metadata.json", | |
| 829 | + existingClients: [], | |
| 830 | + }, | |
| 831 | + }, | |
| 832 | + ); | |
| 833 | + | |
| 834 | + expect(outcome).toEqual({ kind: "aborted" }); | |
| 835 | + expect(popup.calls).toEqual(["reserve", "release"]); | |
| 836 | + }); | |
| 837 | + | |
| 838 | + // A close racing a FAILED registration must still abort: a "fallback" | |
| 839 | + // outcome would make the caller write recovery state into a modal that no | |
| 840 | + // longer exists. | |
| 841 | + it("aborts (not fallback) when the close races a failed registration", async () => { | |
| 842 | + const popup = popupSpy(); | |
| 843 | + let active = true; | |
| 844 | + const outcome = await runDcrConnect( | |
| 845 | + { | |
| 846 | + ...popup, | |
| 847 | + isActive: (): boolean => active, | |
| 848 | + probe: probeOk, | |
| 849 | + register: (): Promise<OAuthClientSlug | null> => { | |
| 850 | + // The modal closes while the registration is in flight, AND the | |
| 851 | + // server rejects it. | |
| 852 | + active = false; | |
| 853 | + return Promise.resolve(null); | |
| 854 | + }, | |
| 855 | + start: (): void => { | |
| 856 | + popup.calls.push("start"); | |
| 857 | + }, | |
| 858 | + }, | |
| 859 | + input, | |
| 860 | + ); | |
| 861 | + | |
| 862 | + expect(outcome).toEqual({ kind: "aborted" }); | |
| 863 | + expect(popup.calls).toEqual(["reserve", "release"]); | |
| 864 | + }); | |
| 865 | + | |
| 866 | + // Same for the CIMD branch: a failed mint racing the close is an abort, not | |
| 867 | + // a client-metadata-failed fallback for the unmounted modal to render. | |
| 868 | + it("aborts (not fallback) when the close races a failed CIMD mint", async () => { | |
| 869 | + const popup = popupSpy(); | |
| 870 | + let active = true; | |
| 871 | + const outcome = await runAutomaticOAuthConnect( | |
| 872 | + { | |
| 873 | + ...popup, | |
| 874 | + isActive: (): boolean => active, | |
| 875 | + probe: (): Promise<ProbeResult> => | |
| 876 | + Promise.resolve({ | |
| 877 | + authorizationUrl: "https://auth.example.com/authorize", | |
| 878 | + tokenUrl: "https://auth.example.com/token", | |
| 879 | + clientIdMetadataDocumentSupported: true, | |
| 880 | + }), | |
| 881 | + createCimdClient: (): Promise<OAuthClientSlug | null> => { | |
| 882 | + // The modal closes while the local CIMD client is being minted, AND | |
| 883 | + // the mint fails. | |
| 884 | + active = false; | |
| 885 | + return Promise.resolve(null); | |
| 886 | + }, | |
| 887 | + register: (): Promise<OAuthClientSlug> => | |
| 888 | + Promise.resolve(OAuthClientSlug.make("unexpected")), | |
| 889 | + start: (): void => { | |
| 890 | + popup.calls.push("start"); | |
| 891 | + }, | |
| 892 | + }, | |
| 893 | + { | |
| 894 | + discoveryUrl: "https://mcp.example.com/mcp", | |
| 895 | + owner: "user" as Owner, | |
| 896 | + integration: TEST_INTEGRATION, | |
| 897 | + cimd: { | |
| 898 | + integrationName: "Test MCP", | |
| 899 | + clientIdMetadataDocumentUrl: "https://executor.example/api/oauth/client-id-metadata.json", | |
| 900 | + existingClients: [], | |
| 901 | + }, | |
| 902 | + }, | |
| 903 | + ); | |
| 904 | + | |
| 905 | + expect(outcome).toEqual({ kind: "aborted" }); | |
| 906 | + expect(popup.calls).toEqual(["reserve", "release"]); | |
| 907 | + }); | |
| 908 | +}); | |
| 909 | + | |
| 502 | 910 | describe("runDcrConnect popup reservation", () => { |
| 503 | 911 | const probeOk = (): Promise<ProbeResult> => |
| 504 | 912 | Promise.resolve({ |
| @@ -856,6 +856,10 @@ type DcrStartArgs = { · 855 unmodified lines | ||
| 856 | 856 | type AutomaticOAuthOutcome = |
| 857 | 857 | | { readonly kind: "started"; readonly flow: "cimd" | "dcr" } |
| 858 | 858 | | { readonly kind: "popup-blocked" } |
| 859 | + /** The owning surface went away mid-flight (`isActive` turned false): the | |
| 860 | + * sequence stopped before its next side effect and released the window. | |
| 861 | + * Nothing to report — the modal that would show it is gone. */ | |
| 862 | + | { readonly kind: "aborted" } | |
| 859 | 863 | | { readonly kind: "fallback"; readonly reason: "probe-failed" } |
| 860 | 864 | | { |
| 861 | 865 | readonly kind: "fallback"; |
| @@ -888,6 +892,10 @@ type RunAutomaticOAuthConnectDeps = { · 26 unmodified lines | ||
| 888 | 892 | readonly reserve: () => OAuthPopupReservation; |
| 889 | 893 | /** Close a claimed window this flow turned out not to need. */ |
| 890 | 894 | readonly release: () => void; |
| 895 | + /** Whether the surface that started this connect still exists. Checked | |
| 896 | + * after every awaited round trip: closing the modal mid-flight must not | |
| 897 | + * register a client or launch the popup afterwards. */ | |
| 898 | + readonly isActive: () => boolean; | |
| 891 | 899 | }; |
| 892 | 900 | |
| 893 | 901 | type RunAutomaticOAuthConnectInput = { |
| @@ -912,6 +920,15 @@ type RunAutomaticOAuthConnectInput = { · 18 unmodified lines | ||
| 912 | 920 | RunCimdConnectInput, |
| 913 | 921 | "integrationName" | "clientIdMetadataDocumentUrl" | "existingClients" |
| 914 | 922 | >; |
| 923 | + /** A reconnect carries the STORED client's RFC 8707 resource — including an | |
| 924 | + * EXPLICIT null for a client registered WITHOUT a resource indicator | |
| 925 | + * (#1822: some servers reject any `resource` parameter). The flow | |
| 926 | + * RECONCILES it with the probe: an explicit null always wins (the | |
| 927 | + * deliberate absence is preserved), a probed resource beats a stored one | |
| 928 | + * (the server migrated), and the stored one stands when the probe | |
| 929 | + * advertises none. Undefined (a fresh connect, or the stored row is gone) | |
| 930 | + * keeps the probed behavior. */ | |
| 931 | + readonly storedResource?: string | null; | |
| 915 | 932 | }; |
| 916 | 933 | |
| 917 | 934 | /** RFC 7591 `client_name` sent for every dynamic registration. Deliberately |
| @@ -933,6 +950,11 @@ const DCR_CLIENT_NAME = "Executor"; · 15 unmodified lines | ||
| 933 | 950 | * - Register rejected with a message → `{ kind: "fallback", reason: "registration-failed", probe, message }` |
| 934 | 951 | * so the caller can show why (e.g. a redirect-URI rejection) over the generic copy. |
| 935 | 952 | * - Register failed without detail (null) → `{ kind: "fallback", reason: "registration-failed", probe }`. |
| 953 | + * - Surface gone after any await (`isActive` false) → `{ kind: "aborted" }`, | |
| 954 | + * window released, popup never launched. Checked BEFORE inspecting that | |
| 955 | + * await's result, so aborted wins even when the round trip failed — a | |
| 956 | + * failure outcome would have the caller write fallback state for a modal | |
| 957 | + * that no longer exists. | |
| 936 | 958 | * - Success → calls `start` and reports which automatic flow was used. |
| 937 | 959 | */ |
| 938 | 960 | export async function runAutomaticOAuthConnect( |
| @@ -947,10 +969,31 @@ export async function runAutomaticOAuthConnect( · 8 unmodified lines | ||
| 947 | 969 | if (reservation.kind === "blocked") return { kind: "popup-blocked" }; |
| 948 | 970 | |
| 949 | 971 | const probe = await deps.probe(input.discoveryUrl); |
| 972 | + // The modal may have closed while the probe was in flight. Stop BEFORE the | |
| 973 | + // next side effect (registration persists a client; start launches the | |
| 974 | + // popup) and give the claimed window back. | |
| 975 | + if (!deps.isActive()) { | |
| 976 | + deps.release(); | |
| 977 | + return { kind: "aborted" }; | |
| 978 | + } | |
| 950 | 979 | if (probe === null) { |
| 951 | 980 | deps.release(); |
| 952 | 981 | return { kind: "fallback", reason: "probe-failed" }; |
| 953 | 982 | } |
| 983 | + // The flow's RFC 8707 resource indicator. A reconnect RECONCILES the stored | |
| 984 | + // value with the probe rather than pinning it: a stored EXPLICIT null always | |
| 985 | + // wins (#1822 — the deliberate absence some servers require), a probed | |
| 986 | + // resource beats a stored one (the server migrated its protected resource, | |
| 987 | + // and the probe is the fresh truth), and a stored resource stands when the | |
| 988 | + // probe advertises none — including over the discovery-URL fallback, which | |
| 989 | + // is our own guess. A fresh connect keeps the probed value with the | |
| 990 | + // discovery-URL fallback. | |
| 991 | + const resource = | |
| 992 | + input.storedResource !== undefined | |
| 993 | + ? input.storedResource === null | |
| 994 | + ? null | |
| 995 | + : (probe.resource ?? input.storedResource) | |
| 996 | + : (probe.resource ?? input.resourceFallback ?? null); | |
| 954 | 997 | if (probe.clientIdMetadataDocumentSupported === true) { |
| 955 | 998 | const resolved = await resolveCimdClient( |
| 956 | 999 | { createClient: deps.createCimdClient }, |
| @@ -959,11 +1002,17 @@ export async function runAutomaticOAuthConnect( · 2 unmodified lines | ||
| 959 | 1002 | integrationName: input.cimd.integrationName, |
| 960 | 1003 | authorizationUrl: probe.authorizationUrl, |
| 961 | 1004 | tokenUrl: probe.tokenUrl, |
| 962 | − resource: probe.resource ?? input.resourceFallback ?? null, | |
| 1005 | + resource, | |
| 963 | 1006 | clientIdMetadataDocumentUrl: input.cimd.clientIdMetadataDocumentUrl, |
| 964 | 1007 | existingClients: input.cimd.existingClients, |
| 965 | 1008 | }, |
| 966 | 1009 | ); |
| 1010 | + // Aborted wins over failure: a "fallback" outcome makes the caller write | |
| 1011 | + // recovery state, and the modal that would render it is gone. | |
| 1012 | + if (!deps.isActive()) { | |
| 1013 | + deps.release(); | |
| 1014 | + return { kind: "aborted" }; | |
| 1015 | + } | |
| 967 | 1016 | if (resolved.kind === "failed") { |
| 968 | 1017 | deps.release(); |
| 969 | 1018 | return { kind: "fallback", reason: "client-metadata-failed", probe }; |
| @@ -986,13 +1035,21 @@ export async function runAutomaticOAuthConnect( · 16 unmodified lines | ||
| 986 | 1035 | registrationEndpoint, |
| 987 | 1036 | authorizationUrl: probe.authorizationUrl, |
| 988 | 1037 | tokenUrl: probe.tokenUrl, |
| 989 | − resource: probe.resource ?? input.resourceFallback ?? null, | |
| 1038 | + resource, | |
| 990 | 1039 | scopes, |
| 991 | 1040 | tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, |
| 992 | 1041 | clientName: DCR_CLIENT_NAME, |
| 993 | 1042 | redirectUri: input.redirectUri, |
| 994 | 1043 | originIntegration: input.integration, |
| 995 | 1044 | }); |
| 1045 | + // A close that raced the registration itself cannot unmint the client (there | |
| 1046 | + // is no server-side cancel) — the row is inert, reusable DCR plumbing — but | |
| 1047 | + // nothing may land on the closed surface afterwards: not the sign-in popup, | |
| 1048 | + // and not a failure fallback either, so aborted wins over the mint result. | |
| 1049 | + if (!deps.isActive()) { | |
| 1050 | + deps.release(); | |
| 1051 | + return { kind: "aborted" }; | |
| 1052 | + } | |
| 996 | 1053 | if (minted === null) { |
| 997 | 1054 | deps.release(); |
| 998 | 1055 | return { kind: "fallback", reason: "registration-failed", probe }; |
| @@ -1007,6 +1064,39 @@ export async function runAutomaticOAuthConnect( · 8 unmodified lines | ||
| 1007 | 1064 | return { kind: "started", flow: "dcr" }; |
| 1008 | 1065 | } |
| 1009 | 1066 | |
| 1067 | +/** | |
| 1068 | + * Can this method go through {@link runAutomaticOAuthConnect} at all? | |
| 1069 | + * | |
| 1070 | + * True when the integration advertises dynamic registration (MCP oauth2) OR | |
| 1071 | + * carries a discovery URL we can probe at connect time — the probe decides | |
| 1072 | + * between CIMD and DCR from there. This is a METHOD capability only: a | |
| 1073 | + * reconnect routes by the STORED client binding instead (`reconnectRoute`), | |
| 1074 | + * so a static/BYO or first-party binding is never silently rebound, and an | |
| 1075 | + * auto-minted DCR binding re-registers even when the method declares no | |
| 1076 | + * capability (the probe falls back to the token URL). | |
| 1077 | + */ | |
| 1078 | +export const hasDcr = (method: AuthMethod | undefined | null): boolean => | |
| 1079 | + method?.kind === "oauth" && | |
| 1080 | + (method.oauth?.supportsDynamicRegistration === true || method.oauth?.discoveryUrl != null); | |
| 1081 | + | |
| 1082 | +/** What a caller of the modal's `startAutomaticOAuthConnect` decides for itself. */ | |
| 1083 | +type AutomaticOAuthConnectRequest = { | |
| 1084 | + readonly method: AuthMethod; | |
| 1085 | + readonly owner: Owner; | |
| 1086 | + readonly connectionName: ConnectionName; | |
| 1087 | + /** Stored on the connection; undefined leaves it untouched. */ | |
| 1088 | + readonly identityLabel: string | undefined; | |
| 1089 | + /** What the user typed, used to auto-name a NEW connection after connect. */ | |
| 1090 | + readonly typedLabel: string; | |
| 1091 | + /** A reconnect re-authorizes a connection that already exists; a connect | |
| 1092 | + * creates one. The only behavioral difference between the two paths. */ | |
| 1093 | + readonly mode: "connect" | "reconnect"; | |
| 1094 | + /** Reconnect only: the stored client's RFC 8707 resource, an EXPLICIT null | |
| 1095 | + * when it was registered WITHOUT one. See | |
| 1096 | + * `RunAutomaticOAuthConnectInput.storedResource`. */ | |
| 1097 | + readonly storedResource?: string | null; | |
| 1098 | +}; | |
| 1099 | + | |
| 1010 | 1100 | // --------------------------------------------------------------------------- |
| 1011 | 1101 | // One row in the OAuth app picker: a radio-select Label plus an actions menu |
| 1012 | 1102 | // (Edit / Remove) so the registered app can be managed inline. The page that |
| @@ -1654,13 +1744,9 @@ function AddAccountModalView(props: AddAccountModalProps) { · 641 unmodified lines | ||
| 1654 | 1744 | : `?${placement.name || "api_key"}=`; |
| 1655 | 1745 | return `${lead}${placement.prefix ?? ""}`; |
| 1656 | 1746 | }, [method, singleInput, isEnvMethod]); |
| 1657 | − // DCR-capable: the integration advertises dynamic registration (MCP oauth2), | |
| 1658 | − // OR carries a discovery URL we can probe at connect time. When DCR-capable | |
| 1659 | − // and not yet fallen back, we skip the app picker entirely (Option A). | |
| 1660 | − const isDcr = | |
| 1661 | − !cimdActive && | |
| 1662 | − isOAuth && | |
| 1663 | − (method?.oauth?.supportsDynamicRegistration === true || method?.oauth?.discoveryUrl != null); | |
| 1747 | + // DCR-capable (see `hasDcr`). When DCR-capable and not yet fallen back, we | |
| 1748 | + // skip the app picker entirely (Option A). | |
| 1749 | + const isDcr = !cimdActive && hasDcr(method); | |
| 1664 | 1750 | const dcrActive = isDcr && !dcrFailed; |
| 1665 | 1751 | const automaticOAuthActive = cimdActive || dcrActive; |
| 1666 | 1752 | |
| @@ -1861,90 +1947,42 @@ function AddAccountModalView(props: AddAccountModalProps) { · 194 unmodified lines | ||
| 1861 | 1947 | // OAuth popup flow's busy state die with this instance. |
| 1862 | 1948 | const close = useCallback(() => onOpenChange(false), [onOpenChange]); |
| 1863 | 1949 | |
| 1864 | − useEffect(() => { | |
| 1865 | − const handoff = initialState; | |
| 1866 | − const oauthClient = handoff?.oauthClient; | |
| 1867 | − if (!handoff || oauthClient?.action !== "reconnect") return; | |
| 1868 | − if (oauthReconnectOpenedKey.current === handoff.key) return; | |
| 1869 | − const client = oauthClient.slug; | |
| 1870 | − const clientOwner = oauthClient.owner ?? handoff.owner; | |
| 1871 | − const connectionOwner = handoff.owner; | |
| 1872 | − const connectionName = handoff.label; | |
| 1873 | − const oauthMethod = handoff.template | |
| 1874 | − ? allMethods.find( | |
| 1875 | − (m: AuthMethod) => | |
| 1876 | − m.kind === "oauth" && | |
| 1877 | − (m.id === handoff.template || String(m.template) === handoff.template), | |
| 1878 | − ) | |
| 1879 | − : allMethods.find((m: AuthMethod) => m.kind === "oauth"); | |
| 1880 | − if (!client || !clientOwner || !connectionOwner || !connectionName || !oauthMethod) return; | |
| 1881 | − | |
| 1882 | − oauthReconnectOpenedKey.current = handoff.key; | |
| 1883 | − setMethodId(oauthMethod.id); | |
| 1884 | − void oauthPopup.start({ | |
| 1885 | − payload: { | |
| 1886 | − client: OAuthClientSlug.make(client), | |
| 1887 | − clientOwner, | |
| 1888 | − owner: connectionOwner, | |
| 1889 | − name: ConnectionName.make(connectionName), | |
| 1890 | − integration, | |
| 1891 | − template: oauthMethod.template, | |
| 1892 | − ...(handoff.identityLabel !== undefined ? { identityLabel: handoff.identityLabel } : {}), | |
| 1893 | − }, | |
| 1894 | − onAuthorizationStarted: () => { | |
| 1895 | − trackEvent("connection_reconnected", { | |
| 1896 | − integration_slug: String(integration), | |
| 1897 | − owner: connectionOwner, | |
| 1898 | − success: true, | |
| 1899 | − }); | |
| 1900 | − }, | |
| 1901 | − onError: () => { | |
| 1902 | − trackEvent("connection_reconnected", { | |
| 1903 | − integration_slug: String(integration), | |
| 1904 | − owner: connectionOwner, | |
| 1905 | − success: false, | |
| 1906 | − }); | |
| 1907 | − }, | |
| 1908 | − onSuccess: () => { | |
| 1909 | − toast.success("Reconnected"); | |
| 1910 | − close(); | |
| 1911 | − }, | |
| 1912 | − }); | |
| 1913 | − }, [initialState, allMethods, integration, oauthPopup, close]); | |
| 1914 | − | |
| 1915 | − const probeAndAutoNameOAuthConnection = async ( | |
| 1916 | − connection: OAuthCompletionPayload, | |
| 1917 | − typedLabel: string, | |
| 1918 | − ): Promise<void> => { | |
| 1919 | − const check = await doCheckConnectionHealth({ | |
| 1920 | − params: { | |
| 1921 | − owner: connection.owner, | |
| 1922 | − integration: connection.integration, | |
| 1923 | − name: connection.name, | |
| 1924 | − }, | |
| 1925 | − query: {}, | |
| 1926 | − reactivityKeys: connectionCheckKeys, | |
| 1927 | − }); | |
| 1928 | − if (Exit.isFailure(check)) return; | |
| 1929 | − const nextIdentityLabel = oauthIdentityLabelFromHealth({ | |
| 1930 | − result: check.value, | |
| 1931 | − typedLabel, | |
| 1932 | − storedIdentityLabel: connection.identityLabel, | |
| 1933 | − }); | |
| 1934 | − if (nextIdentityLabel === null) return; | |
| 1935 | − const updated = await doUpdateConnection({ | |
| 1936 | − params: { | |
| 1937 | − owner: connection.owner, | |
| 1938 | − integration: connection.integration, | |
| 1939 | − name: connection.name, | |
| 1940 | − }, | |
| 1941 | − payload: { identityLabel: nextIdentityLabel }, | |
| 1942 | − reactivityKeys: connectionWriteKeys, | |
| 1943 | − }); | |
| 1944 | − if (Exit.isFailure(updated)) { | |
| 1945 | − toast.error(messageFromExit(updated, "Couldn't update connection name")); | |
| 1946 | − } | |
| 1947 | − }; | |
| 1950 | + // Stable identity: the reconnect effect below reaches this through | |
| 1951 | + // `startAutomaticOAuthConnect`, so an identity that changed every render | |
| 1952 | + // would re-run that effect on every keystroke. | |
| 1953 | + const probeAndAutoNameOAuthConnection = useCallback( | |
| 1954 | + async (connection: OAuthCompletionPayload, typedLabel: string): Promise<void> => { | |
| 1955 | + const check = await doCheckConnectionHealth({ | |
| 1956 | + params: { | |
| 1957 | + owner: connection.owner, | |
| 1958 | + integration: connection.integration, | |
| 1959 | + name: connection.name, | |
| 1960 | + }, | |
| 1961 | + query: {}, | |
| 1962 | + reactivityKeys: connectionCheckKeys, | |
| 1963 | + }); | |
| 1964 | + if (Exit.isFailure(check)) return; | |
| 1965 | + const nextIdentityLabel = oauthIdentityLabelFromHealth({ | |
| 1966 | + result: check.value, | |
| 1967 | + typedLabel, | |
| 1968 | + storedIdentityLabel: connection.identityLabel, | |
| 1969 | + }); | |
| 1970 | + if (nextIdentityLabel === null) return; | |
| 1971 | + const updated = await doUpdateConnection({ | |
| 1972 | + params: { | |
| 1973 | + owner: connection.owner, | |
| 1974 | + integration: connection.integration, | |
| 1975 | + name: connection.name, | |
| 1976 | + }, | |
| 1977 | + payload: { identityLabel: nextIdentityLabel }, | |
| 1978 | + reactivityKeys: connectionWriteKeys, | |
| 1979 | + }); | |
| 1980 | + if (Exit.isFailure(updated)) { | |
| 1981 | + toast.error(messageFromExit(updated, "Couldn't update connection name")); | |
| 1982 | + } | |
| 1983 | + }, | |
| 1984 | + [doCheckConnectionHealth, doUpdateConnection], | |
| 1985 | + ); | |
| 1948 | 1986 | |
| 1949 | 1987 | const credentialPayloadOrigin = createCredentialPayloadOrigin({ |
| 1950 | 1988 | origin: credentialOrigin, |
| @@ -2245,23 +2283,27 @@ function AddAccountModalView(props: AddAccountModalProps) { · 294 unmodified lines | ||
| 2245 | 2283 | }); |
| 2246 | 2284 | }; |
| 2247 | 2285 | |
| 2248 | − const createCimdClient = async (args: CimdCreateClientArgs): Promise<OAuthClientSlug | null> => { | |
| 2249 | − const exit = await doCreateOAuthClient({ | |
| 2250 | − payload: { | |
| 2251 | − owner: args.owner, | |
| 2252 | − slug: args.slug, | |
| 2253 | − authorizationUrl: args.authorizationUrl, | |
| 2254 | − tokenUrl: args.tokenUrl, | |
| 2255 | − resource: args.resource ?? null, | |
| 2256 | − grant: args.grant, | |
| 2257 | − clientId: args.clientId, | |
| 2258 | − clientSecret: args.clientSecret, | |
| 2259 | − }, | |
| 2260 | − reactivityKeys: oauthClientWriteKeys, | |
| 2261 | − }); | |
| 2262 | − if (Exit.isFailure(exit)) return null; | |
| 2263 | − return exit.value.client; | |
| 2264 | − }; | |
| 2286 | + // Stable identity for the same reason as probeAndAutoNameOAuthConnection. | |
| 2287 | + const createCimdClient = useCallback( | |
| 2288 | + async (args: CimdCreateClientArgs): Promise<OAuthClientSlug | null> => { | |
| 2289 | + const exit = await doCreateOAuthClient({ | |
| 2290 | + payload: { | |
| 2291 | + owner: args.owner, | |
| 2292 | + slug: args.slug, | |
| 2293 | + authorizationUrl: args.authorizationUrl, | |
| 2294 | + tokenUrl: args.tokenUrl, | |
| 2295 | + resource: args.resource ?? null, | |
| 2296 | + grant: args.grant, | |
| 2297 | + clientId: args.clientId, | |
| 2298 | + clientSecret: args.clientSecret, | |
| 2299 | + }, | |
| 2300 | + reactivityKeys: oauthClientWriteKeys, | |
| 2301 | + }); | |
| 2302 | + if (Exit.isFailure(exit)) return null; | |
| 2303 | + return exit.value.client; | |
| 2304 | + }, | |
| 2305 | + [doCreateOAuthClient], | |
| 2306 | + ); | |
| 2265 | 2307 | |
| 2266 | 2308 | const handleCimdConnect = async () => { |
| 2267 | 2309 | const authorizationUrl = method?.oauth?.authorizationUrl; |
| @@ -2324,124 +2366,295 @@ function AddAccountModalView(props: AddAccountModalProps) { · 56 unmodified lines | ||
| 2324 | 2366 | } |
| 2325 | 2367 | }; |
| 2326 | 2368 | |
| 2327 | − // Automatic discovered OAuth connect: probe once, then prefer CIMD or use DCR | |
| 2369 | + // Whether this view is still mounted. Closing the modal unmounts it (see | |
| 2370 | + // `AddAccountModal`), and the automatic connect sequence below polls this | |
| 2371 | + // between its awaited round trips so a close mid-flight aborts instead of | |
| 2372 | + // registering a client / launching the popup into a dead surface. The ref is | |
| 2373 | + // re-armed in the effect body so a StrictMode remount stays active. | |
| 2374 | + const viewMountedRef = useRef(true); | |
| 2375 | + useEffect(() => { | |
| 2376 | + viewMountedRef.current = true; | |
| 2377 | + return () => { | |
| 2378 | + viewMountedRef.current = false; | |
| 2379 | + }; | |
| 2380 | + }, []); | |
| 2381 | + | |
| 2382 | + // Automatic discovered OAuth: probe once, then prefer CIMD or use DCR | |
| 2328 | 2383 | // according to the authorization server's advertised metadata. On failure we |
| 2329 | 2384 | // flip `dcrFailed` so the bring-your-own-app picker remains the recovery path. |
| 2385 | + // | |
| 2386 | + // Reconnect runs through here too, and must (issue #1542): a DCR client is | |
| 2387 | + // bound to the redirect URI it registered with, so once the app's callback | |
| 2388 | + // origin moves (127.0.0.1 -> localhost) re-authorizing against the STORED | |
| 2389 | + // client fails at the authorization server. Re-probing and re-registering is | |
| 2390 | + // what replaces that stranded client, and it is exactly what the connect path | |
| 2391 | + // already does — so both take one route rather than two that drift. | |
| 2392 | + const startAutomaticOAuthConnect = useCallback( | |
| 2393 | + async (request: AutomaticOAuthConnectRequest): Promise<void> => { | |
| 2394 | + const { method: requestMethod, owner: dcrOwner, mode } = request; | |
| 2395 | + const reconnect = mode === "reconnect"; | |
| 2396 | + const discoveryUrl = requestMethod.oauth?.discoveryUrl ?? requestMethod.oauth?.tokenUrl; | |
| 2397 | + if (!discoveryUrl) { | |
| 2398 | + setDcrFailed(true); | |
| 2399 | + return; | |
| 2400 | + } | |
| 2401 | + setDcrBusy(true); | |
| 2402 | + const outcome = await runAutomaticOAuthConnect( | |
| 2403 | + { | |
| 2404 | + reserve: oauthPopup.reserve, | |
| 2405 | + release: oauthPopup.releaseReservation, | |
| 2406 | + // Closing the modal genuinely unmounts this view (see | |
| 2407 | + // `AddAccountModal`), so "still mounted" is exactly "still open". | |
| 2408 | + // The sequence checks it between round trips: a close mid-flight | |
| 2409 | + // must not register a client or launch the popup afterwards. | |
| 2410 | + isActive: () => viewMountedRef.current, | |
| 2411 | + probe: async (url: string): Promise<OAuthProbeResult | null> => { | |
| 2412 | + const exit = await doProbe({ payload: { url }, reactivityKeys: [] }); | |
| 2413 | + if (Exit.isFailure(exit)) return null; | |
| 2414 | + return exit.value; | |
| 2415 | + }, | |
| 2416 | + createCimdClient, | |
| 2417 | + register: async ( | |
| 2418 | + args: DcrRegisterArgs, | |
| 2419 | + ): Promise<OAuthClientSlug | { readonly error: string } | null> => { | |
| 2420 | + const exit = await doRegisterDynamic({ | |
| 2421 | + payload: { | |
| 2422 | + owner: args.owner, | |
| 2423 | + slug: args.slug, | |
| 2424 | + issuer: args.issuer ?? null, | |
| 2425 | + registrationEndpoint: args.registrationEndpoint, | |
| 2426 | + authorizationUrl: args.authorizationUrl, | |
| 2427 | + tokenUrl: args.tokenUrl, | |
| 2428 | + resource: args.resource ?? null, | |
| 2429 | + scopes: args.scopes, | |
| 2430 | + tokenEndpointAuthMethodsSupported: args.tokenEndpointAuthMethodsSupported, | |
| 2431 | + clientName: args.clientName, | |
| 2432 | + redirectUri: args.redirectUri, | |
| 2433 | + originIntegration: args.originIntegration, | |
| 2434 | + }, | |
| 2435 | + reactivityKeys: oauthClientWriteKeys, | |
| 2436 | + }); | |
| 2437 | + if (Exit.isFailure(exit)) { | |
| 2438 | + return { | |
| 2439 | + error: messageFromExit( | |
| 2440 | + exit, | |
| 2441 | + "Automatic setup unavailable. Register an app instead.", | |
| 2442 | + ), | |
| 2443 | + }; | |
| 2444 | + } | |
| 2445 | + return exit.value.client; | |
| 2446 | + }, | |
| 2447 | + start: (args: DcrStartArgs): void => { | |
| 2448 | + void oauthPopup.start({ | |
| 2449 | + reservation: args.reservation, | |
| 2450 | + payload: { | |
| 2451 | + client: args.client, | |
| 2452 | + // DCR/CIMD mints the client under the connection owner, so the | |
| 2453 | + // app and connection share one owner. | |
| 2454 | + clientOwner: args.owner, | |
| 2455 | + owner: dcrOwner, | |
| 2456 | + name: request.connectionName, | |
| 2457 | + integration, | |
| 2458 | + template: requestMethod.template, | |
| 2459 | + ...(reconnect ? {} : { newConnection: true }), | |
| 2460 | + ...(request.identityLabel !== undefined | |
| 2461 | + ? { identityLabel: request.identityLabel } | |
| 2462 | + : {}), | |
| 2463 | + }, | |
| 2464 | + ...(reconnect | |
| 2465 | + ? { | |
| 2466 | + onAuthorizationStarted: () => { | |
| 2467 | + trackEvent("connection_reconnected", { | |
| 2468 | + integration_slug: String(integration), | |
| 2469 | + owner: dcrOwner, | |
| 2470 | + success: true, | |
| 2471 | + }); | |
| 2472 | + }, | |
| 2473 | + onError: () => { | |
| 2474 | + trackEvent("connection_reconnected", { | |
| 2475 | + integration_slug: String(integration), | |
| 2476 | + owner: dcrOwner, | |
| 2477 | + success: false, | |
| 2478 | + }); | |
| 2479 | + }, | |
| 2480 | + } | |
| 2481 | + : {}), | |
| 2482 | + onSuccess: async (connection: OAuthCompletionPayload) => { | |
| 2483 | + // A reconnect keeps the connection's existing name; only a new | |
| 2484 | + // connection gets auto-named from what was probed. | |
| 2485 | + if (!reconnect) { | |
| 2486 | + await probeAndAutoNameOAuthConnection(connection, request.typedLabel); | |
| 2487 | + } | |
| 2488 | + toast.success(reconnect ? "Reconnected" : "Connection added"); | |
| 2489 | + close(); | |
| 2490 | + }, | |
| 2491 | + }); | |
| 2492 | + }, | |
| 2493 | + }, | |
| 2494 | + { | |
| 2495 | + discoveryUrl, | |
| 2496 | + // Only a genuine discovery URL (MCP) seeds the RFC 8707 resource | |
| 2497 | + // indicator; the token-endpoint fallback baked into `discoveryUrl` must | |
| 2498 | + // not, so pass the un-collapsed method value here. | |
| 2499 | + resourceFallback: requestMethod.oauth?.discoveryUrl, | |
| 2500 | + owner: dcrOwner, | |
| 2501 | + // DCR slugs are server-keyed (Part A): the connect path no longer depends | |
| 2502 | + // on the picker's app list, so it need not be threaded here. | |
| 2503 | + declaredScopes: requestMethod.oauth?.scopes, | |
| 2504 | + redirectUri: oauthCallbackUrl(), | |
| 2505 | + integration, | |
| 2506 | + cimd: { | |
| 2507 | + integrationName, | |
| 2508 | + clientIdMetadataDocumentUrl: oauthClientIdMetadataDocumentUrl(), | |
| 2509 | + existingClients: clientSummaries, | |
| 2510 | + }, | |
| 2511 | + ...(request.storedResource !== undefined | |
| 2512 | + ? { storedResource: request.storedResource } | |
| 2513 | + : {}), | |
| 2514 | + }, | |
| 2515 | + ); | |
| 2516 | + // The modal closed mid-flight: this view is unmounted, so no state may | |
| 2517 | + // be written at all — not the fallback below, and not even the busy | |
| 2518 | + // flag, which belongs to the surface that is gone. | |
| 2519 | + if (outcome.kind === "aborted") return; | |
| 2520 | + setDcrBusy(false); | |
| 2521 | + // `connection_oauth_started` measures the connect funnel; a reconnect | |
| 2522 | + // reports through `connection_reconnected` on the popup callbacks above, | |
| 2523 | + // so it must not also land here. | |
| 2524 | + if (!reconnect) { | |
| 2525 | + trackEvent("connection_oauth_started", { | |
| 2526 | + integration_slug: String(integration), | |
| 2527 | + owner: dcrOwner, | |
| 2528 | + flow: | |
| 2529 | + outcome.kind === "started" | |
| 2530 | + ? outcome.flow | |
| 2531 | + : "probe" in outcome && outcome.probe.clientIdMetadataDocumentSupported === true | |
| 2532 | + ? "cimd" | |
| 2533 | + : "dcr", | |
| 2534 | + success: outcome.kind === "started", | |
| 2535 | + ...(outcome.kind === "fallback" ? { dcr_fallback: true } : {}), | |
| 2536 | + }); | |
| 2537 | + } | |
| 2538 | + // Deliberately absent: a "popup-blocked" branch. Registering an app by hand | |
| 2539 | + // does not make the browser open a window, so dropping to the BYO picker | |
| 2540 | + // would send the user down a path that cannot succeed either. `reserve` | |
| 2541 | + // already put the reason in `oauthPopup.error`, which the footer renders. | |
| 2542 | + if (outcome.kind === "fallback") { | |
| 2543 | + setOAuthFallbackProbe("probe" in outcome ? outcome.probe : null); | |
| 2544 | + setDcrFailed(true); | |
| 2545 | + // Surface the server's actionable rejection reason on the recovery view as | |
| 2546 | + // an inline error card. Generic fallbacks (no message) fall through to the | |
| 2547 | + // "register an app" empty state, which already guides the user. | |
| 2548 | + setDcrFallbackMessage("message" in outcome ? (outcome.message ?? null) : null); | |
| 2549 | + } | |
| 2550 | + }, | |
| 2551 | + [ | |
| 2552 | + close, | |
| 2553 | + clientSummaries, | |
| 2554 | + createCimdClient, | |
| 2555 | + doProbe, | |
| 2556 | + doRegisterDynamic, | |
| 2557 | + integration, | |
| 2558 | + integrationName, | |
| 2559 | + oauthPopup, | |
| 2560 | + probeAndAutoNameOAuthConnection, | |
| 2561 | + ], | |
| 2562 | + ); | |
| 2563 | + | |
| 2330 | 2564 | const handleAutomaticOAuthConnect = async () => { |
| 2331 | − const discoveryUrl = method?.oauth?.discoveryUrl ?? method?.oauth?.tokenUrl; | |
| 2332 | − if (!method || !discoveryUrl) { | |
| 2565 | + if (!method) { | |
| 2333 | 2566 | setDcrFailed(true); |
| 2334 | 2567 | return; |
| 2335 | 2568 | } |
| 2336 | − const dcrOwner = owner; | |
| 2337 | − const connectionName = previewConnectionName(label, dcrOwner); | |
| 2338 | − const identityLabel = typedIdentityLabel(label); | |
| 2339 | − setDcrBusy(true); | |
| 2340 | − const outcome = await runAutomaticOAuthConnect( | |
| 2341 | − { | |
| 2342 | − reserve: oauthPopup.reserve, | |
| 2343 | − release: oauthPopup.releaseReservation, | |
| 2344 | − probe: async (url: string): Promise<OAuthProbeResult | null> => { | |
| 2345 | − const exit = await doProbe({ payload: { url }, reactivityKeys: [] }); | |
| 2346 | − if (Exit.isFailure(exit)) return null; | |
| 2347 | − return exit.value; | |
| 2348 | − }, | |
| 2349 | − createCimdClient, | |
| 2350 | − register: async ( | |
| 2351 | − args: DcrRegisterArgs, | |
| 2352 | − ): Promise<OAuthClientSlug | { readonly error: string } | null> => { | |
| 2353 | − const exit = await doRegisterDynamic({ | |
| 2354 | − payload: { | |
| 2355 | − owner: args.owner, | |
| 2356 | − slug: args.slug, | |
| 2357 | − issuer: args.issuer ?? null, | |
| 2358 | − registrationEndpoint: args.registrationEndpoint, | |
| 2359 | − authorizationUrl: args.authorizationUrl, | |
| 2360 | − tokenUrl: args.tokenUrl, | |
| 2361 | − resource: args.resource ?? null, | |
| 2362 | − scopes: args.scopes, | |
| 2363 | − tokenEndpointAuthMethodsSupported: args.tokenEndpointAuthMethodsSupported, | |
| 2364 | − clientName: args.clientName, | |
| 2365 | − redirectUri: args.redirectUri, | |
| 2366 | − originIntegration: args.originIntegration, | |
| 2367 | − }, | |
| 2368 | − reactivityKeys: oauthClientWriteKeys, | |
| 2369 | − }); | |
| 2370 | − if (Exit.isFailure(exit)) { | |
| 2371 | − return { | |
| 2372 | − error: messageFromExit(exit, "Automatic setup unavailable. Register an app instead."), | |
| 2373 | − }; | |
| 2374 | − } | |
| 2375 | − return exit.value.client; | |
| 2376 | − }, | |
| 2377 | − start: (args: DcrStartArgs): void => { | |
| 2378 | − void oauthPopup.start({ | |
| 2379 | − reservation: args.reservation, | |
| 2380 | − payload: { | |
| 2381 | − client: args.client, | |
| 2382 | − // DCR registers the client under the connection owner, so the app | |
| 2383 | − // and connection share one owner. | |
| 2384 | − clientOwner: args.owner, | |
| 2385 | − owner: args.owner, | |
| 2386 | − name: connectionName, | |
| 2387 | − integration, | |
| 2388 | − template: method.template, | |
| 2389 | − newConnection: true, | |
| 2390 | − ...(identityLabel !== undefined ? { identityLabel } : {}), | |
| 2391 | − }, | |
| 2392 | − onSuccess: async (connection: OAuthCompletionPayload) => { | |
| 2393 | − await probeAndAutoNameOAuthConnection(connection, label); | |
| 2394 | − toast.success("Connection added"); | |
| 2395 | − close(); | |
| 2396 | − }, | |
| 2397 | − }); | |
| 2398 | − }, | |
| 2399 | − }, | |
| 2400 | − { | |
| 2401 | − discoveryUrl, | |
| 2402 | − // Only a genuine discovery URL (MCP) seeds the RFC 8707 resource | |
| 2403 | − // indicator; the token-endpoint fallback baked into `discoveryUrl` must | |
| 2404 | − // not, so pass the un-collapsed method value here. | |
| 2405 | − resourceFallback: method.oauth?.discoveryUrl, | |
| 2406 | − owner: dcrOwner, | |
| 2407 | − // DCR slugs are server-keyed (Part A): the connect path no longer depends | |
| 2408 | − // on the picker's app list, so it need not be threaded here. | |
| 2409 | − declaredScopes: method.oauth?.scopes, | |
| 2410 | − redirectUri: oauthCallbackUrl(), | |
| 2569 | + await startAutomaticOAuthConnect({ | |
| 2570 | + method, | |
| 2571 | + owner, | |
| 2572 | + connectionName: previewConnectionName(label, owner), | |
| 2573 | + identityLabel: typedIdentityLabel(label), | |
| 2574 | + typedLabel: label, | |
| 2575 | + mode: "connect", | |
| 2576 | + }); | |
| 2577 | + }; | |
| 2578 | + | |
| 2579 | + // The reconnect handoff: a connection asked to be re-authorized, so open its | |
| 2580 | + // OAuth flow immediately. Fires once per handoff key (tracked by ref), which | |
| 2581 | + // is also what makes a re-render mid-flight harmless. | |
| 2582 | + // | |
| 2583 | + // Routing follows the STORED binding, not just the method: only a handoff | |
| 2584 | + // whose stored client is auto-minted DCR (vetted by the accounts section, | |
| 2585 | + // `dynamicRegistration: true`) re-runs the automatic path — see | |
| 2586 | + // `startAutomaticOAuthConnect`. Everything else (static/BYO, first-party) | |
| 2587 | + // has a fixed, registered app, so it starts the popup against that client | |
| 2588 | + // directly and is never rebound to an automatic one. | |
| 2589 | + useEffect(() => { | |
| 2590 | + const handoff = initialState; | |
| 2591 | + const oauthClient = handoff?.oauthClient; | |
| 2592 | + if (!handoff || oauthClient?.action !== "reconnect") return; | |
| 2593 | + if (oauthReconnectOpenedKey.current === handoff.key) return; | |
| 2594 | + const client = oauthClient.slug; | |
| 2595 | + const clientOwner = oauthClient.owner ?? handoff.owner; | |
| 2596 | + const connectionOwner = handoff.owner; | |
| 2597 | + const connectionName = handoff.label; | |
| 2598 | + const oauthMethod = handoff.template | |
| 2599 | + ? allMethods.find( | |
| 2600 | + (m: AuthMethod) => | |
| 2601 | + m.kind === "oauth" && | |
| 2602 | + (m.id === handoff.template || String(m.template) === handoff.template), | |
| 2603 | + ) | |
| 2604 | + : allMethods.find((m: AuthMethod) => m.kind === "oauth"); | |
| 2605 | + if (!client || !clientOwner || !connectionOwner || !connectionName || !oauthMethod) return; | |
| 2606 | + | |
| 2607 | + oauthReconnectOpenedKey.current = handoff.key; | |
| 2608 | + setMethodId(oauthMethod.id); | |
| 2609 | + | |
| 2610 | + // The accounts section vetted the STORED binding as auto-minted DCR | |
| 2611 | + // (`dynamicRegistration: true`), and that alone routes: the automatic | |
| 2612 | + // flow probes the method's token URL even when it declares no | |
| 2613 | + // discovery/DCR capability, whereas the direct path below would dead-end | |
| 2614 | + // an origin-drifted DCR client (#1542). | |
| 2615 | + if (oauthClient.dynamicRegistration === true) { | |
| 2616 | + void startAutomaticOAuthConnect({ | |
| 2617 | + method: oauthMethod, | |
| 2618 | + owner: connectionOwner, | |
| 2619 | + connectionName: ConnectionName.make(connectionName), | |
| 2620 | + identityLabel: handoff.identityLabel, | |
| 2621 | + typedLabel: connectionName, | |
| 2622 | + mode: "reconnect", | |
| 2623 | + ...(oauthClient.resource !== undefined ? { storedResource: oauthClient.resource } : {}), | |
| 2624 | + }); | |
| 2625 | + return; | |
| 2626 | + } | |
| 2627 | + | |
| 2628 | + void oauthPopup.start({ | |
| 2629 | + payload: { | |
| 2630 | + client: OAuthClientSlug.make(client), | |
| 2631 | + clientOwner, | |
| 2632 | + owner: connectionOwner, | |
| 2633 | + name: ConnectionName.make(connectionName), | |
| 2411 | 2634 | integration, |
| 2412 | − cimd: { | |
| 2413 | − integrationName, | |
| 2414 | − clientIdMetadataDocumentUrl: oauthClientIdMetadataDocumentUrl(), | |
| 2415 | − existingClients: clientSummaries, | |
| 2416 | − }, | |
| 2635 | + template: oauthMethod.template, | |
| 2636 | + ...(handoff.identityLabel !== undefined ? { identityLabel: handoff.identityLabel } : {}), | |
| 2637 | + }, | |
| 2638 | + onAuthorizationStarted: () => { | |
| 2639 | + trackEvent("connection_reconnected", { | |
| 2640 | + integration_slug: String(integration), | |
| 2641 | + owner: connectionOwner, | |
| 2642 | + success: true, | |
| 2643 | + }); | |
| 2644 | + }, | |
| 2645 | + onError: () => { | |
| 2646 | + trackEvent("connection_reconnected", { | |
| 2647 | + integration_slug: String(integration), | |
| 2648 | + owner: connectionOwner, | |
| 2649 | + success: false, | |
| 2650 | + }); | |
| 2651 | + }, | |
| 2652 | + onSuccess: () => { | |
| 2653 | + toast.success("Reconnected"); | |
| 2654 | + close(); | |
| 2417 | 2655 | }, |
| 2418 | − ); | |
| 2419 | − setDcrBusy(false); | |
| 2420 | − trackEvent("connection_oauth_started", { | |
| 2421 | − integration_slug: String(integration), | |
| 2422 | − owner: dcrOwner, | |
| 2423 | − flow: | |
| 2424 | − outcome.kind === "started" | |
| 2425 | − ? outcome.flow | |
| 2426 | − : "probe" in outcome && outcome.probe.clientIdMetadataDocumentSupported === true | |
| 2427 | − ? "cimd" | |
| 2428 | − : "dcr", | |
| 2429 | − success: outcome.kind === "started", | |
| 2430 | − ...(outcome.kind === "fallback" ? { dcr_fallback: true } : {}), | |
| 2431 | 2656 | }); |
| 2432 | − // Deliberately absent: a "popup-blocked" branch. Registering an app by hand | |
| 2433 | − // does not make the browser open a window, so dropping to the BYO picker | |
| 2434 | − // would send the user down a path that cannot succeed either. `reserve` | |
| 2435 | − // already put the reason in `oauthPopup.error`, which the footer renders. | |
| 2436 | − if (outcome.kind === "fallback") { | |
| 2437 | − setOAuthFallbackProbe("probe" in outcome ? outcome.probe : null); | |
| 2438 | − setDcrFailed(true); | |
| 2439 | − // Surface the server's actionable rejection reason on the recovery view as | |
| 2440 | − // an inline error card. Generic fallbacks (no message) fall through to the | |
| 2441 | − // "register an app" empty state, which already guides the user. | |
| 2442 | − setDcrFallbackMessage("message" in outcome ? (outcome.message ?? null) : null); | |
| 2443 | − } | |
| 2444 | − }; | |
| 2657 | + }, [initialState, allMethods, integration, oauthPopup, close, startAutomaticOAuthConnect]); | |
| 2445 | 2658 | |
| 2446 | 2659 | return ( |
| 2447 | 2660 | // Non-modal for the same reason as the health-check editor sheet: a modal |
| @@ -1,4 +1,6 @@ | ||
| 1 | 1 | import { describe, expect, it } from "@effect/vitest"; |
| 2 | +import * as AsyncResult from "effect/unstable/reactivity/AsyncResult"; | |
| 3 | +import * as Option from "effect/Option"; | |
| 2 | 4 | import { |
| 3 | 5 | AuthTemplateSlug, |
| 4 | 6 | ConnectionAddress, |
| @@ -7,13 +9,18 @@ import { · 2 unmodified lines | ||
| 7 | 9 | OAuthClientSlug, |
| 8 | 10 | ProviderKey, |
| 9 | 11 | type Connection, |
| 12 | + type OAuthClientSummary, | |
| 10 | 13 | } from "@executor-js/sdk/shared"; |
| 11 | 14 | |
| 12 | 15 | import { |
| 13 | 16 | missingScopes, |
| 14 | 17 | oauthReconnectPayload, |
| 18 | + reconnectClientsView, | |
| 15 | 19 | reconnectMode, |
| 20 | + reconnectRoute, | |
| 21 | + reconnectStoredClient, | |
| 16 | 22 | reconsentRequiredScopes, |
| 23 | + retryReconnectClientsOnMenuOpen, | |
| 17 | 24 | } from "./oauth-reconnect"; |
| 18 | 25 | |
| 19 | 26 | const connection = (overrides: Partial<Connection> = {}): Connection => ({ |
| @@ -70,6 +77,181 @@ describe("oauthReconnectPayload (re-mint the SAME connection)", () => { · 50 unmodified lines | ||
| 70 | 77 | }); |
| 71 | 78 | }); |
| 72 | 79 | |
| 80 | +const clientSummary = (overrides: Partial<OAuthClientSummary> = {}): OAuthClientSummary => ({ | |
| 81 | + owner: "user", | |
| 82 | + slug: OAuthClientSlug.make("github-app"), | |
| 83 | + grant: "authorization_code", | |
| 84 | + authorizationUrl: "https://auth.example.com/authorize", | |
| 85 | + tokenUrl: "https://auth.example.com/token", | |
| 86 | + resource: null, | |
| 87 | + clientId: "client-123", | |
| 88 | + origin: { kind: "manual", integration: null }, | |
| 89 | + ...overrides, | |
| 90 | +}); | |
| 91 | + | |
| 92 | +describe("reconnectStoredClient (resolve a connection's stored app)", () => { | |
| 93 | + it("finds the stored row by slug and the app's stored owner", () => { | |
| 94 | + const stored = clientSummary(); | |
| 95 | + expect(reconnectStoredClient([clientSummary({ owner: "org" }), stored], connection())).toBe( | |
| 96 | + stored, | |
| 97 | + ); | |
| 98 | + }); | |
| 99 | + | |
| 100 | + it("matches against oauthClientOwner when the app is shared (org app, user connection)", () => { | |
| 101 | + const shared = clientSummary({ owner: "org" }); | |
| 102 | + expect(reconnectStoredClient([shared], connection({ oauthClientOwner: "org" }))).toBe(shared); | |
| 103 | + // Without the stored app owner, the connection's own owner is the key. | |
| 104 | + expect(reconnectStoredClient([shared], connection())).toBeUndefined(); | |
| 105 | + }); | |
| 106 | + | |
| 107 | + it("matches a first-party app on slug alone (config-declared, deployment-scoped)", () => { | |
| 108 | + const firstParty = clientSummary({ | |
| 109 | + owner: "org", | |
| 110 | + slug: OAuthClientSlug.make("first-party:github"), | |
| 111 | + origin: { kind: "first_party" }, | |
| 112 | + }); | |
| 113 | + expect( | |
| 114 | + reconnectStoredClient( | |
| 115 | + [firstParty], | |
| 116 | + connection({ oauthClient: OAuthClientSlug.make("first-party:github") }), | |
| 117 | + ), | |
| 118 | + ).toBe(firstParty); | |
| 119 | + }); | |
| 120 | + | |
| 121 | + it("is undefined for a non-OAuth connection and for a binding whose row is gone", () => { | |
| 122 | + expect( | |
| 123 | + reconnectStoredClient([clientSummary()], connection({ oauthClient: null })), | |
| 124 | + ).toBeUndefined(); | |
| 125 | + expect(reconnectStoredClient([], connection())).toBeUndefined(); | |
| 126 | + }); | |
| 127 | +}); | |
| 128 | + | |
| 129 | +describe("reconnectRoute (where an OAuth Reconnect goes)", () => { | |
| 130 | + const dcrBinding = clientSummary({ | |
| 131 | + origin: { kind: "dynamic_client_registration", integration: null }, | |
| 132 | + }); | |
| 133 | + | |
| 134 | + // The summaries have not loaded → the stored binding is UNKNOWN, and no | |
| 135 | + // route may be chosen. The old routing chose "direct" here, permanently | |
| 136 | + // dead-ending an origin-drifted DCR client exactly as #1542 described. | |
| 137 | + it("waits (never chooses direct) while the client summaries are loading", () => { | |
| 138 | + expect(reconnectRoute(undefined, connection(), true)).toEqual({ kind: "unknown" }); | |
| 139 | + expect(reconnectRoute(undefined, connection(), false)).toEqual({ kind: "unknown" }); | |
| 140 | + }); | |
| 141 | + | |
| 142 | + // The BINDING's origin routes, not the method's capability flags: the | |
| 143 | + // automatic flow can probe the token URL even when the method declares no | |
| 144 | + // discovery/DCR support. | |
| 145 | + it("routes a DCR-origin binding to the automatic path regardless of method capability", () => { | |
| 146 | + expect(reconnectRoute([dcrBinding], connection(), false)).toEqual({ | |
| 147 | + kind: "automatic", | |
| 148 | + stored: dcrBinding, | |
| 149 | + }); | |
| 150 | + expect(reconnectRoute([dcrBinding], connection(), true)).toEqual({ | |
| 151 | + kind: "automatic", | |
| 152 | + stored: dcrBinding, | |
| 153 | + }); | |
| 154 | + }); | |
| 155 | + | |
| 156 | + // A manual (static/BYO) or first-party binding must keep the direct | |
| 157 | + // stored-client path — re-registering would silently rebind the connection. | |
| 158 | + it("keeps a manual (static/BYO) binding on the direct path even on a capable method", () => { | |
| 159 | + expect(reconnectRoute([clientSummary()], connection(), true)).toEqual({ kind: "direct" }); | |
| 160 | + }); | |
| 161 | + | |
| 162 | + it("keeps a first-party binding on the direct path", () => { | |
| 163 | + expect( | |
| 164 | + reconnectRoute([clientSummary({ origin: { kind: "first_party" } })], connection(), true), | |
| 165 | + ).toEqual({ kind: "direct" }); | |
| 166 | + }); | |
| 167 | + | |
| 168 | + // A binding whose row is GONE has nothing to start directly against, so | |
| 169 | + // re-registration is its only recovery — when the method supports the | |
| 170 | + // automatic flow at all. | |
| 171 | + it("re-registers a gone binding when the method supports the automatic flow", () => { | |
| 172 | + expect(reconnectRoute([], connection(), true)).toEqual({ | |
| 173 | + kind: "automatic", | |
| 174 | + stored: undefined, | |
| 175 | + }); | |
| 176 | + expect(reconnectRoute([], connection(), false)).toEqual({ kind: "direct" }); | |
| 177 | + }); | |
| 178 | +}); | |
| 179 | + | |
| 180 | +describe("reconnectClientsView (Reconnect availability from the summaries query)", () => { | |
| 181 | + const dcrBinding = clientSummary({ | |
| 182 | + origin: { kind: "dynamic_client_registration", integration: null }, | |
| 183 | + }); | |
| 184 | + const loaded = AsyncResult.success<readonly OAuthClientSummary[], string>([dcrBinding]); | |
| 185 | + | |
| 186 | + it("is ready with the loaded summaries on success", () => { | |
| 187 | + expect(reconnectClientsView(loaded)).toEqual({ kind: "ready", clients: [dcrBinding] }); | |
| 188 | + }); | |
| 189 | + | |
| 190 | + // Stale data NEVER routes: a binding changed since the retained snapshot can | |
| 191 | + // misroute (repeat the origin-drift dead end, or treat a client absent from | |
| 192 | + // the snapshot as vanished and rebind it). A failed refresh is failed even | |
| 193 | + // with previousSuccess — disabled, surfaced, and retried on menu open like | |
| 194 | + // any other failure. | |
| 195 | + it("is failed when a refresh fails, even with previous successful data", () => { | |
| 196 | + const failedRefresh = AsyncResult.fail("listClients failed", { | |
| 197 | + previousSuccess: Option.some(loaded), | |
| 198 | + }); | |
| 199 | + const view = reconnectClientsView(failedRefresh); | |
| 200 | + expect(view).toEqual({ kind: "failed" }); | |
| 201 | + // The failed view recovers like any other: opening the menu retries. | |
| 202 | + let calls = 0; | |
| 203 | + retryReconnectClientsOnMenuOpen(true, view, () => calls++); | |
| 204 | + expect(calls).toBe(1); | |
| 205 | + }); | |
| 206 | + | |
| 207 | + it("is loading while the first load is in flight", () => { | |
| 208 | + expect(reconnectClientsView(AsyncResult.initial())).toEqual({ kind: "loading" }); | |
| 209 | + expect(reconnectClientsView(AsyncResult.initial(true))).toEqual({ kind: "loading" }); | |
| 210 | + }); | |
| 211 | + | |
| 212 | + it("is failed for a first-load failure (never produced data)", () => { | |
| 213 | + expect(reconnectClientsView(AsyncResult.fail("listClients failed"))).toEqual({ | |
| 214 | + kind: "failed", | |
| 215 | + }); | |
| 216 | + }); | |
| 217 | + | |
| 218 | + it("reads a failure that is retrying as loading, not failed", () => { | |
| 219 | + expect(reconnectClientsView(AsyncResult.fail("listClients failed", { waiting: true }))).toEqual( | |
| 220 | + { kind: "loading" }, | |
| 221 | + ); | |
| 222 | + // A retry in flight is loading even when previous data is retained. | |
| 223 | + expect( | |
| 224 | + reconnectClientsView( | |
| 225 | + AsyncResult.fail("listClients failed", { | |
| 226 | + waiting: true, | |
| 227 | + previousSuccess: Option.some(loaded), | |
| 228 | + }), | |
| 229 | + ), | |
| 230 | + ).toEqual({ kind: "loading" }); | |
| 231 | + }); | |
| 232 | +}); | |
| 233 | + | |
| 234 | +describe("retryReconnectClientsOnMenuOpen (recovery for a failed summaries query)", () => { | |
| 235 | + const counter = () => { | |
| 236 | + let calls = 0; | |
| 237 | + return { retry: () => calls++, calls: () => calls }; | |
| 238 | + }; | |
| 239 | + | |
| 240 | + it("refetches when the menu OPENS on a failed view", () => { | |
| 241 | + const spy = counter(); | |
| 242 | + retryReconnectClientsOnMenuOpen(true, { kind: "failed" }, spy.retry); | |
| 243 | + expect(spy.calls()).toBe(1); | |
| 244 | + }); | |
| 245 | + | |
| 246 | + it("does not refetch on close, while loading, or with data", () => { | |
| 247 | + const spy = counter(); | |
| 248 | + retryReconnectClientsOnMenuOpen(false, { kind: "failed" }, spy.retry); | |
| 249 | + retryReconnectClientsOnMenuOpen(true, { kind: "loading" }, spy.retry); | |
| 250 | + retryReconnectClientsOnMenuOpen(true, { kind: "ready", clients: [] }, spy.retry); | |
| 251 | + expect(spy.calls()).toBe(0); | |
| 252 | + }); | |
| 253 | +}); | |
| 254 | + | |
| 73 | 255 | describe("missingScopes (Part 2 informational subset warning)", () => { |
| 74 | 256 | // The app's scopes are a STRICT subset of the integration's → list what's |
| 75 | 257 | // missing, in the integration's declared order. |
| @@ -1,4 +1,5 @@ | ||
| 1 | −import type { Connection } from "@executor-js/sdk/shared"; | |
| 1 | +import * as AsyncResult from "effect/unstable/reactivity/AsyncResult"; | |
| 2 | +import type { Connection, OAuthClientSummary } from "@executor-js/sdk/shared"; | |
| 2 | 3 | |
| 3 | 4 | import type { OAuthStartPayload } from "./oauth-sign-in"; |
| 4 | 5 | |
| @@ -43,6 +44,108 @@ export function oauthReconnectPayload(connection: Connection): OAuthStartPayload · 38 unmodified lines | ||
| 43 | 44 | }; |
| 44 | 45 | } |
| 45 | 46 | |
| 47 | +/** The stored OAuth app backing a connection, resolved from the loaded client | |
| 48 | + * summaries. First-party apps are config-declared and deployment-scoped, so | |
| 49 | + * they match on slug alone; stored rows are owner-scoped, matched against the | |
| 50 | + * app's stored owner (a Personal connection may be backed by a shared | |
| 51 | + * Workspace app). Undefined when the connection is not OAuth or its client | |
| 52 | + * row is gone (e.g. removed by hand). */ | |
| 53 | +export function reconnectStoredClient( | |
| 54 | + clients: readonly OAuthClientSummary[], | |
| 55 | + connection: Connection, | |
| 56 | +): OAuthClientSummary | undefined { | |
| 57 | + if (connection.oauthClient == null) return undefined; | |
| 58 | + const slug = String(connection.oauthClient); | |
| 59 | + const owner = connection.oauthClientOwner ?? connection.owner; | |
| 60 | + return clients.find( | |
| 61 | + (client) => | |
| 62 | + String(client.slug) === slug && | |
| 63 | + (client.origin.kind === "first_party" || client.owner === owner), | |
| 64 | + ); | |
| 65 | +} | |
| 66 | + | |
| 67 | +/** Where an OAuth connection's Reconnect goes. */ | |
| 68 | +export type ReconnectRoute = | |
| 69 | + /** The client summaries have not loaded, so the stored binding is unknown. | |
| 70 | + * NO route may be chosen yet: guessing "direct" dead-ends an origin-drifted | |
| 71 | + * DCR client (#1542), and guessing "automatic" would rebind a manual app. | |
| 72 | + * The caller keeps the action unavailable until the summaries resolve. */ | |
| 73 | + | { readonly kind: "unknown" } | |
| 74 | + /** Re-run the automatic probe/CIMD/DCR flow. `stored` is the binding's | |
| 75 | + * summary — undefined when its row is gone — so the handoff can carry its | |
| 76 | + * resource. */ | |
| 77 | + | { readonly kind: "automatic"; readonly stored: OAuthClientSummary | undefined } | |
| 78 | + /** Start the OAuth flow directly against the stored client. */ | |
| 79 | + | { readonly kind: "direct" }; | |
| 80 | + | |
| 81 | +/** Decide the Reconnect route from the STORED client binding. | |
| 82 | + * | |
| 83 | + * The binding's ORIGIN is what routes, not the method's capability flags: | |
| 84 | + * - An auto-minted DCR binding re-registers (its whole lifecycle is | |
| 85 | + * automatic, and the automatic flow can probe the token URL even when the | |
| 86 | + * method declares no discovery/DCR support). Reusing it directly dead-ends | |
| 87 | + * once the callback origin drifts (#1542). | |
| 88 | + * - A manual (static/BYO) or first-party binding keeps the direct | |
| 89 | + * stored-client path — re-registering would silently rebind the connection | |
| 90 | + * to an automatic client. | |
| 91 | + * - A binding whose row is GONE has nothing to start directly against, so it | |
| 92 | + * re-registers when the method supports the automatic flow at all. | |
| 93 | + * Pass `clients: undefined` while the summaries are loading: the decision is | |
| 94 | + * then `"unknown"`, never a guess. */ | |
| 95 | +export function reconnectRoute( | |
| 96 | + clients: readonly OAuthClientSummary[] | undefined, | |
| 97 | + connection: Connection, | |
| 98 | + methodSupportsAutomatic: boolean, | |
| 99 | +): ReconnectRoute { | |
| 100 | + if (clients === undefined) return { kind: "unknown" }; | |
| 101 | + const stored = reconnectStoredClient(clients, connection); | |
| 102 | + if (stored !== undefined) { | |
| 103 | + return stored.origin.kind === "dynamic_client_registration" | |
| 104 | + ? { kind: "automatic", stored } | |
| 105 | + : { kind: "direct" }; | |
| 106 | + } | |
| 107 | + return methodSupportsAutomatic ? { kind: "automatic", stored: undefined } : { kind: "direct" }; | |
| 108 | +} | |
| 109 | + | |
| 110 | +/** Reconnect's view of the client-summaries query: | |
| 111 | + * - `"ready"` — a current successful load; routing reads these summaries. | |
| 112 | + * - `"loading"` — the query is in flight (first load, or a retry after a | |
| 113 | + * failure); the action waits, exactly as before. | |
| 114 | + * - `"failed"` — the query failed. Stale data from an earlier success NEVER | |
| 115 | + * routes: a binding changed since that snapshot can misroute (repeat the | |
| 116 | + * origin-drift dead end, or treat a client absent from the snapshot as | |
| 117 | + * vanished and rebind it). The menu item surfaces the failure instead of | |
| 118 | + * sitting silently disabled, and opening the menu retries the query (see | |
| 119 | + * `retryReconnectClientsOnMenuOpen`). */ | |
| 120 | +export type ReconnectClientsView = | |
| 121 | + | { readonly kind: "ready"; readonly clients: readonly OAuthClientSummary[] } | |
| 122 | + | { readonly kind: "loading" } | |
| 123 | + | { readonly kind: "failed" }; | |
| 124 | + | |
| 125 | +/** Fold the summaries query into Reconnect's availability. Only a current | |
| 126 | + * Success is ready — a Failure reads as failed even when it retains a | |
| 127 | + * `previousSuccess`. A failure that is `waiting` is a retry in flight, so it | |
| 128 | + * reads as loading, not failed. */ | |
| 129 | +export function reconnectClientsView( | |
| 130 | + result: AsyncResult.AsyncResult<readonly OAuthClientSummary[], unknown>, | |
| 131 | +): ReconnectClientsView { | |
| 132 | + if (AsyncResult.isSuccess(result)) return { kind: "ready", clients: result.value }; | |
| 133 | + if (AsyncResult.isFailure(result) && !result.waiting) return { kind: "failed" }; | |
| 134 | + return { kind: "loading" }; | |
| 135 | +} | |
| 136 | + | |
| 137 | +/** Recovery for the failed view: OPENING the row menu retries the query, so a | |
| 138 | + * transient listClients error never leaves Reconnect dead for the mounted | |
| 139 | + * lifetime. Only the failed view refetches — ready and loading views must | |
| 140 | + * not restart a request that already has data or is already running. */ | |
| 141 | +export function retryReconnectClientsOnMenuOpen( | |
| 142 | + open: boolean, | |
| 143 | + view: ReconnectClientsView, | |
| 144 | + retry: () => void, | |
| 145 | +): void { | |
| 146 | + if (open && view.kind === "failed") retry(); | |
| 147 | +} | |
| 148 | + | |
| 46 | 149 | // --------------------------------------------------------------------------- |
| 47 | 150 | // Subset-scope warning (Part 2). At connect, when the chosen OAuth app's |
| 48 | 151 | // DECLARED scopes are a STRICT subset of the integration's declared scopes, the |