History

Fix OAuth popup completion and DCR reuse (#852)

Rhys Sullivan committed 5 months ago

Commit 6d177071 parent bc70f845 files · +201 −12Browse files

Unified · Split

5 files
  1. packages/core/sdk/src/executor.ts +1 −0
  2. packages/core/sdk/src/oauth-service.ts +122 −0
  3. packages/core/sdk/src/testing.test.ts +72 −0
  4. packages/react/src/api/oauth-popup.test.ts +3 −3
  5. packages/react/src/api/oauth-popup.ts +3 −9
packages/core/sdk/src/executor.ts+1−0
@@ -3021,6 +3021,7 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 3020 unmodified lines
30213021 ),
30223022 secretsSet: (input) => secretsSet(input),
30233023 connectionsCreate: (input) => connectionsCreate(input),
3024+ connectionsGet: (id) => connectionsGet(id),
30243025 httpClientLayer: config.httpClientLayer,
30253026 endpointUrlPolicy: config.oauthEndpointUrlPolicy,
30263027 });
packages/core/sdk/src/oauth-service.ts+122−0
@@ -76,6 +76,10 @@ import { · 75 unmodified lines
7676 beginDynamicAuthorization,
7777 discoverAuthorizationServerMetadata,
7878 discoverProtectedResourceMetadata,
79+ type BeginDynamicAuthorizationInput,
80+ type OAuthAuthorizationServerMetadata,
81+ type OAuthClientInformation,
82+ type OAuthProtectedResourceMetadata,
7983 } from "./oauth-discovery";
8084 import {
8185 buildAuthorizationUrl,
@@ -111,6 +115,14 @@ const DynamicDcrSessionPayload = Schema.Struct({ · 29 unmodified lines
111115 resource: Schema.NullOr(Schema.String).pipe(Schema.withDecodingDefaultType(Effect.succeed(null))),
112116 });
113117
118+const PendingDynamicDcrSessionRows = Schema.Array(
119+ Schema.Struct({
120+ payload: Schema.Unknown,
121+ expires_at: Schema.Union([Schema.Number, Schema.BigInt, Schema.String]),
122+ created_at: Schema.Union([Schema.Date, Schema.String, Schema.Number]),
123+ }),
124+);
125+
114126 const AuthorizationCodeSessionPayload = Schema.Struct({
115127 kind: Schema.Literal("authorization-code"),
116128 identityLabel: Schema.NullOr(Schema.String),
@@ -141,14 +153,17 @@ const OAuthSessionPayload = Schema.Union([ · 24 unmodified lines
141153 AuthorizationCodeSessionPayload,
142154 ]);
143155 type OAuthSessionPayload = typeof OAuthSessionPayload.Type;
156+type PreviousDynamicAuthorizationState = BeginDynamicAuthorizationInput["previousState"];
144157
145158 const decodeSessionPayload = Schema.decodeUnknownSync(OAuthSessionPayload);
146159 const encodeSessionPayload = Schema.encodeSync(OAuthSessionPayload);
160+const isPendingDynamicDcrSessionRows = Schema.is(PendingDynamicDcrSessionRows);
147161
148162 const UnknownFromJsonString = Schema.fromJsonString(Schema.Unknown);
149163 const decodeUnknownJsonOption = Schema.decodeUnknownOption(UnknownFromJsonString);
150164
151165 const decodeProviderStateSync = Schema.decodeUnknownSync(OAuthProviderStateSchema);
166+const decodeProviderStateOption = Schema.decodeUnknownOption(OAuthProviderStateSchema);
152167 const encodeProviderStateSync = Schema.encodeSync(OAuthProviderStateSchema);
153168
154169 const coerceJson = (value: unknown): unknown => {
@@ -187,6 +202,10 @@ export interface OAuthServiceDeps { · 32 unmodified lines
187202 readonly connectionsCreate: (
188203 input: CreateConnectionInput,
189204 ) => Effect.Effect<ConnectionRef, ConnectionProviderNotRegisteredError | StorageFailure>;
205+ /** Reads an existing Connection so dynamic-DCR retries can reuse the
206+ * registered OAuth client instead of registering a new client every
207+ * time the user restarts a browser flow. */
208+ readonly connectionsGet?: (id: string) => Effect.Effect<ConnectionRef | null, StorageFailure>;
190209 /** Random session id generator. Tests override to make outputs
191210 * deterministic. */
192211 readonly newSessionId?: () => string;
@@ -239,6 +258,7 @@ export const makeOAuth2Service = ( · 46 unmodified lines
239258 const newSessionId = deps.newSessionId ?? defaultSessionId;
240259 const httpClientLayer = deps.httpClientLayer;
241260 const endpointUrlPolicy = deps.endpointUrlPolicy;
261+ const connectionsGet = deps.connectionsGet ?? (() => Effect.succeed(null));
242262 const secretsGetResolved =
243263 deps.secretsGetResolved ??
244264 ((id: string) =>
@@ -372,17 +392,119 @@ export const makeOAuth2Service = ( · 127 unmodified lines
372392 // -------------------------------------------------------------------
373393 // start — branches on strategy.kind
374394 // -------------------------------------------------------------------
395+
396+ const dynamicClientAuthMethod = (
397+ state: Extract<OAuthProviderState, { kind: "dynamic-dcr" }>,
398+ ): "none" | "client_secret_basic" | "client_secret_post" =>
399+ state.clientSecretSecretId
400+ ? state.clientAuth === "basic"
401+ ? "client_secret_basic"
402+ : "client_secret_post"
403+ : "none";
404+
405+ const timestampMillis = (value: unknown): number => {
406+ if (value instanceof Date) return value.getTime();
407+ if (typeof value === "string" || typeof value === "number") return new Date(value).getTime();
408+ return 0;
409+ };
410+
411+ const previousDynamicStateFromConnection = (
412+ connectionId: string,
413+ ): Effect.Effect<PreviousDynamicAuthorizationState | undefined, StorageFailure> =>
414+ Effect.gen(function* () {
415+ const existing = yield* connectionsGet(connectionId);
416+ const state = existing?.providerState
417+ ? Option.getOrNull(decodeProviderStateOption(coerceJson(existing.providerState)))
418+ : null;
419+ if (!state || state.kind !== "dynamic-dcr") return undefined;
420+
421+ const clientSecret =
422+ state.clientSecretSecretId !== null
423+ ? yield* getSecretFromRecordedScope({
424+ secretId: state.clientSecretSecretId,
425+ scopeId: state.clientSecretSecretScopeId ?? null,
426+ })
427+ : null;
428+ if (state.clientSecretSecretId !== null && !clientSecret) return undefined;
429+
430+ return {
431+ authorizationServerUrl: state.authorizationServerUrl ?? null,
432+ authorizationServerMetadataUrl: state.authorizationServerMetadataUrl,
433+ clientInformation: {
434+ client_id: state.clientId,
435+ token_endpoint_auth_method: dynamicClientAuthMethod(state),
436+ ...(clientSecret ? { client_secret: clientSecret } : {}),
437+ },
438+ };
439+ });
440+
441+ const previousDynamicStateFromPendingSession = (input: {
442+ readonly connectionId: string;
443+ readonly tokenScope: string;
444+ }): Effect.Effect<PreviousDynamicAuthorizationState | undefined, StorageFailure> =>
445+ Effect.gen(function* () {
446+ const rowsRaw = yield* deps.fuma.use("oauth2_session.findReusableDynamicDcr", (db) =>
447+ db.findMany("oauth2_session", {
448+ where: (b) =>
449+ b.and(
450+ b("connection_id", "=", input.connectionId),
451+ b("token_scope", "=", input.tokenScope),
452+ b("strategy", "=", "dynamic-dcr"),
453+ ),
454+ }),
455+ );
456+ const rows = isPendingDynamicDcrSessionRows(rowsRaw) ? rowsRaw : [];
457+
458+ const reusable = rows
459+ .filter((row) => Number(row.expires_at) > now())
460+ .sort((a, b) => {
461+ const aTime = timestampMillis(a.created_at);
462+ const bTime = timestampMillis(b.created_at);
463+ return bTime - aTime;
464+ });
465+
466+ for (const row of reusable) {
467+ const payload = decodeSessionPayload(row.payload);
468+ if (payload.kind !== "dynamic-dcr") continue;
469+ return {
470+ authorizationServerUrl: payload.authorizationServerUrl,
471+ authorizationServerMetadataUrl: payload.authorizationServerMetadataUrl,
472+ authorizationServerMetadata:
473+ payload.authorizationServerMetadata as OAuthAuthorizationServerMetadata,
474+ resourceMetadata: payload.resourceMetadata as OAuthProtectedResourceMetadata | null,
475+ resourceMetadataUrl: payload.resourceMetadataUrl,
476+ clientInformation: payload.clientInformation as OAuthClientInformation,
477+ };
478+ }
479+ return undefined;
480+ });
481+
482+ const previousDynamicState = (input: {
483+ readonly connectionId: string;
484+ readonly tokenScope: string;
485+ }) =>
486+ previousDynamicStateFromPendingSession(input).pipe(
487+ Effect.flatMap((pending) =>
488+ pending ? Effect.succeed(pending) : previousDynamicStateFromConnection(input.connectionId),
489+ ),
490+ );
491+
375492 const startDynamicDcr = (
376493 input: OAuthStartInput,
377494 strategy: OAuthDynamicDcrStrategy,
378495 ): Effect.Effect<OAuthStartResult, OAuthStartError | StorageFailure> =>
379496 Effect.gen(function* () {
497+ const previousState = yield* previousDynamicState({
498+ connectionId: input.connectionId,
499+ tokenScope: input.tokenScope,
500+ });
380501 const started = yield* beginDynamicAuthorization(
381502 {
382503 endpoint: input.endpoint,
383504 redirectUrl: input.redirectUrl,
384505 state: "",
385506 scopes: strategy.scopes,
507+ previousState,
386508 },
387509 {
388510 httpClientLayer,
packages/core/sdk/src/testing.test.ts+72−0
@@ -115,6 +115,78 @@ layer(TestLayer, { timeout: "15 seconds" })("testing fixtures", (it) => { · 114 unmodified lines
115115 }),
116116 );
117117
118+ it.effect("dynamic client registration is reused across OAuth start retries", () =>
119+ Effect.gen(function* () {
120+ const workspace = yield* TestWorkspace.current<typeof plugins>();
121+ const oauth = yield* OAuthTestServer;
122+ const scope = workspace.scopes[0]!;
123+ yield* oauth.clearRequests;
124+
125+ const start = () =>
126+ workspace.executor.oauth.start({
127+ endpoint: oauth.mcpResourceUrl,
128+ connectionId: "test-oauth-dcr-retry",
129+ tokenScope: String(scope.id),
130+ redirectUrl: "http://127.0.0.1/callback",
131+ pluginId: "test",
132+ identityLabel: "MCP OAuth Test",
133+ strategy: { kind: "dynamic-dcr", scopes: ["read"] },
134+ });
135+
136+ const startedA = yield* start();
137+ const startedB = yield* start();
138+
139+ expect(startedA.authorizationUrl).not.toBeNull();
140+ expect(startedB.authorizationUrl).not.toBeNull();
141+ expect(
142+ (yield* oauth.requests).filter((request) => request.path === "/register"),
143+ ).toHaveLength(1);
144+ expect(new URL(startedB.authorizationUrl ?? "").searchParams.get("client_id")).toBe(
145+ new URL(startedA.authorizationUrl ?? "").searchParams.get("client_id"),
146+ );
147+ }),
148+ );
149+
150+ it.effect("dynamic client registration is reused after a completed connection reconnect", () =>
151+ Effect.gen(function* () {
152+ const workspace = yield* TestWorkspace.current<typeof plugins>();
153+ const oauth = yield* OAuthTestServer;
154+ const scope = workspace.scopes[0]!;
155+ yield* oauth.clearRequests;
156+
157+ const start = () =>
158+ workspace.executor.oauth.start({
159+ endpoint: oauth.mcpResourceUrl,
160+ connectionId: "test-oauth-dcr-reconnect",
161+ tokenScope: String(scope.id),
162+ redirectUrl: "http://127.0.0.1/callback",
163+ pluginId: "test",
164+ identityLabel: "MCP OAuth Test",
165+ strategy: { kind: "dynamic-dcr", scopes: ["read"] },
166+ });
167+
168+ const startedA = yield* start();
169+ const callback = yield* oauth.completeAuthorizationCodeFlow({
170+ authorizationUrl: startedA.authorizationUrl ?? "",
171+ });
172+ yield* workspace.executor.oauth.complete({
173+ state: callback.state,
174+ code: callback.code,
175+ tokenScope: String(scope.id),
176+ });
177+
178+ const startedB = yield* start();
179+
180+ expect(startedB.authorizationUrl).not.toBeNull();
181+ expect(
182+ (yield* oauth.requests).filter((request) => request.path === "/register"),
183+ ).toHaveLength(1);
184+ expect(new URL(startedB.authorizationUrl ?? "").searchParams.get("client_id")).toBe(
185+ new URL(startedA.authorizationUrl ?? "").searchParams.get("client_id"),
186+ );
187+ }),
188+ );
189+
118190 it.effect(
119191 "OAuthTestServer can mint a bearer token through the full authorization-code flow",
120192 () =>
packages/react/src/api/oauth-popup.test.ts+3−3
@@ -42,7 +42,7 @@ describe("openOAuthPopup", () => { · 41 unmodified lines
4242 expect(openFailed).toBe(true);
4343 });
4444
45− it("opens supported OAuth URLs through a reserved popup", () => {
45+ it("opens supported OAuth URLs through a reserved popup with opener available", () => {
4646 let features = "";
4747 let opened = "";
4848 const popup: FakePopup = { closed: false, close: () => {}, location: { href: "" } };
@@ -81,7 +81,7 @@ describe("openOAuthPopup", () => { · 32 unmodified lines
8181 writable: true,
8282 });
8383 expect(opened).toBe("about:blank");
84− expect(popup.opener).toBe(null);
84+ expect(popup.opener).toBeUndefined();
8585 expect(popup.location.href).toBe("https://auth.example/authorize");
8686 expect(features).toContain("popup=1");
8787 expect(features).not.toContain("noopener");
@@ -128,7 +128,7 @@ describe("openOAuthPopup", () => { · 40 unmodified lines
128128 });
129129 expect(opened).toBe("about:blank");
130130 expect(reservedPopup).not.toBeNull();
131− expect(popup.opener).toBe(null);
131+ expect(popup.opener).toBeUndefined();
132132 expect(popup.location.href).toBe("https://auth.example/authorize");
133133 });
134134
packages/react/src/api/oauth-popup.ts+3−9
@@ -72,15 +72,9 @@ export const reserveOAuthPopup = (input: { · 71 unmodified lines
7272 }): ReservedOAuthPopup | null => {
7373 const popup = window.open("about:blank", input.popupName, oauthPopupFeatures(input));
7474 if (!popup) return null;
75− // The app keeps a WindowProxy for navigation/closed polling, but the
76− // provider should not receive opener access after the reserved window
77− // is navigated cross-origin. The callback uses BroadcastChannel.
78− // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: popup opener access can throw in browser-specific states
79− try {
80− popup.opener = null;
81− } catch {
82− // Best-effort hardening; the popup handle is still usable for the flow.
83− }
75+ // Keep opener available for the same-origin callback page. Browser
76+ // BroadcastChannel delivery can be partitioned in popup flows, so the
77+ // callback's postMessage path is the primary completion signal.
8478 return { popup };
8579 };
8680