History

Deterministic DCR slugs; thread issuer through probe and connect paths

Rhys Sullivan committed 4 months ago

Commit 8155ab21 parent a5d2cd26 files · +262 −8Browse files

Unified · Split

6 files
  1. packages/core/sdk/src/oauth-register-dynamic.test.ts +225 −3
  2. packages/react/src/components/add-account-modal.test.ts +5 −0
  3. packages/react/src/components/add-account-modal.tsx +8 −2
  4. packages/react/src/components/oauth-client-form.tsx +7 −3
  5. packages/react/src/plugins/use-effective-oauth-client.test.ts +10 −0
  6. packages/react/src/plugins/use-effective-oauth-client.tsx +7 −0
packages/core/sdk/src/oauth-register-dynamic.test.ts+225−3
@@ -25,6 +25,18 @@ const TEMPLATE = AuthTemplateSlug.make("oauth"); · 24 unmodified lines
2525 const CLIENT = OAuthClientSlug.make("acme-dcr");
2626 const FLOW_REDIRECT_URI = "https://localhost:5394/api/oauth/callback";
2727
28+const dcrSlugFor = (issuerUrl: string): OAuthClientSlug =>
29+ OAuthClientSlug.make(
30+ `dcr-${new URL(issuerUrl).host
31+ .toLowerCase()
32+ .replace(/[^a-z0-9]+/g, "-")
33+ .replace(/^-+|-+$/g, "")}`,
34+ );
35+
36+const registerRequestCount = (
37+ requests: readonly { readonly path: string; readonly method: string }[],
38+): number => requests.filter((r) => r.path === "/register" && r.method === "POST").length;
39+
2840 const oauthPlugin = definePlugin(() => ({
2941 id: "acme" as const,
3042 storage: () => ({}),
@@ -56,6 +68,7 @@ describe("oauth.registerDynamicClient", () => { · 25 unmodified lines
5668 // Probe surfaces the registration endpoint + advertised auth methods so
5769 // the caller knows a public client is allowed.
5870 const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl });
71+ expect(probe.issuer).toBe(server.issuerUrl);
5972 expect(probe.registrationEndpoint).toBe(server.registrationEndpoint);
6073 expect(probe.tokenEndpointAuthMethodsSupported).toContain("none");
6174 expect(probe.resource).toBe(server.mcpResourceUrl);
@@ -64,6 +77,7 @@ describe("oauth.registerDynamicClient", () => { · 2 unmodified lines
6477 const slug = yield* executor.oauth.registerDynamicClient({
6578 owner: "org",
6679 slug: CLIENT,
80+ issuer: probe.issuer,
6781 registrationEndpoint: probe.registrationEndpoint!,
6882 authorizationUrl: probe.authorizationUrl,
6983 tokenUrl: probe.tokenUrl,
@@ -74,12 +88,13 @@ describe("oauth.registerDynamicClient", () => { · 4 unmodified lines
7488 redirectUri: FLOW_REDIRECT_URI,
7589 originIntegration: INTEG,
7690 });
77− expect(String(slug)).toBe(String(CLIENT));
91+ const expectedSlug = dcrSlugFor(server.issuerUrl);
92+ expect(String(slug)).toBe(String(expectedSlug));
7893
7994 // The minted client appears in listClients with a server-issued
8095 // client_id and NO secret ever projected.
8196 const clients = yield* executor.oauth.listClients();
82− const minted = clients.find((c) => String(c.slug) === String(CLIENT));
97+ const minted = clients.find((c) => String(c.slug) === String(expectedSlug));
8398 expect(minted).toBeDefined();
8499 expect(minted!.owner).toBe("org");
85100 expect(minted!.grant).toBe("authorization_code");
@@ -98,7 +113,7 @@ describe("oauth.registerDynamicClient", () => { · 12 unmodified lines
98113 // PKCE flow with NO client_secret on the token exchange.
99114 const started = yield* executor.oauth.start({
100115 owner: "org",
101− client: CLIENT,
116+ client: expectedSlug,
102117 clientOwner: "org",
103118 name: ConnectionName.make("main"),
104119 integration: INTEG,
@@ -145,6 +160,213 @@ describe("oauth.registerDynamicClient", () => { · 40 unmodified lines
145160 ),
146161 );
147162
163+ it.effect("reuses an existing DCR client for the same owner and authorization server", () =>
164+ Effect.scoped(
165+ Effect.gen(function* () {
166+ const server = yield* serveOAuthTestServer({ scopes: ["read"] });
167+ const { executor } = yield* makeTestWorkspaceHarness({ plugins });
168+ yield* executor.acme.seed();
169+ const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl });
170+
171+ const first = yield* executor.oauth.registerDynamicClient({
172+ owner: "org",
173+ slug: OAuthClientSlug.make("first-attempt"),
174+ issuer: probe.issuer,
175+ registrationEndpoint: probe.registrationEndpoint!,
176+ authorizationUrl: probe.authorizationUrl,
177+ tokenUrl: probe.tokenUrl,
178+ resource: probe.resource,
179+ scopes: ["read"],
180+ tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
181+ clientName: "Acme DCR",
182+ redirectUri: FLOW_REDIRECT_URI,
183+ originIntegration: INTEG,
184+ });
185+ yield* server.clearRequests;
186+
187+ const second = yield* executor.oauth.registerDynamicClient({
188+ owner: "org",
189+ slug: OAuthClientSlug.make("second-attempt"),
190+ issuer: probe.issuer,
191+ registrationEndpoint: probe.registrationEndpoint!,
192+ authorizationUrl: probe.authorizationUrl,
193+ tokenUrl: probe.tokenUrl,
194+ resource: probe.resource,
195+ scopes: ["read"],
196+ tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
197+ clientName: "Other integration DCR",
198+ redirectUri: FLOW_REDIRECT_URI,
199+ originIntegration: IntegrationSlug.make("other"),
200+ });
201+
202+ expect(second).toBe(first);
203+ const requests = yield* server.requests;
204+ expect(registerRequestCount(requests)).toBe(0);
205+ }),
206+ ),
207+ );
208+
209+ it.effect("does not reuse a DCR client across owners", () =>
210+ Effect.scoped(
211+ Effect.gen(function* () {
212+ const server = yield* serveOAuthTestServer({ scopes: ["read"] });
213+ const { executor } = yield* makeTestWorkspaceHarness({ plugins });
214+ yield* executor.acme.seed();
215+ const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl });
216+
217+ const orgSlug = yield* executor.oauth.registerDynamicClient({
218+ owner: "org",
219+ slug: OAuthClientSlug.make("org-attempt"),
220+ issuer: probe.issuer,
221+ registrationEndpoint: probe.registrationEndpoint!,
222+ authorizationUrl: probe.authorizationUrl,
223+ tokenUrl: probe.tokenUrl,
224+ resource: probe.resource,
225+ scopes: ["read"],
226+ tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
227+ clientName: "Org DCR",
228+ redirectUri: FLOW_REDIRECT_URI,
229+ originIntegration: INTEG,
230+ });
231+ yield* server.clearRequests;
232+
233+ const userSlug = yield* executor.oauth.registerDynamicClient({
234+ owner: "user",
235+ slug: OAuthClientSlug.make("user-attempt"),
236+ issuer: probe.issuer,
237+ registrationEndpoint: probe.registrationEndpoint!,
238+ authorizationUrl: probe.authorizationUrl,
239+ tokenUrl: probe.tokenUrl,
240+ resource: probe.resource,
241+ scopes: ["read"],
242+ tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
243+ clientName: "User DCR",
244+ redirectUri: FLOW_REDIRECT_URI,
245+ originIntegration: INTEG,
246+ });
247+
248+ expect(String(userSlug)).toBe(String(orgSlug));
249+ const requests = yield* server.requests;
250+ expect(registerRequestCount(requests)).toBe(1);
251+ }),
252+ ),
253+ );
254+
255+ it.effect("reuses legacy DCR-looking rows with no stored issuer by token host", () =>
256+ Effect.scoped(
257+ Effect.gen(function* () {
258+ const server = yield* serveOAuthTestServer({ scopes: ["read"] });
259+ const { config, executor } = yield* makeTestWorkspaceHarness({ plugins });
260+ yield* executor.acme.seed();
261+ const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl });
262+ const legacySlug = OAuthClientSlug.make("cloudflare-mcp");
263+
264+ yield* executor.oauth.createClient({
265+ owner: "org",
266+ slug: legacySlug,
267+ authorizationUrl: probe.authorizationUrl,
268+ tokenUrl: probe.tokenUrl,
269+ resource: server.mcpResourceUrl,
270+ grant: "authorization_code",
271+ clientId: "legacy-dcr-client",
272+ clientSecret: "",
273+ });
274+ yield* Effect.promise(() =>
275+ config.db.updateMany("oauth_client", {
276+ where: (b) => b("slug", "=", String(legacySlug)),
277+ set: { origin_kind: null, origin_integration: null, origin_issuer: null },
278+ }),
279+ );
280+ yield* server.clearRequests;
281+
282+ const reused = yield* executor.oauth.registerDynamicClient({
283+ owner: "org",
284+ slug: OAuthClientSlug.make("new-attempt"),
285+ issuer: probe.issuer,
286+ registrationEndpoint: probe.registrationEndpoint!,
287+ authorizationUrl: probe.authorizationUrl,
288+ tokenUrl: probe.tokenUrl,
289+ resource: server.mcpResourceUrl,
290+ scopes: ["read"],
291+ tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
292+ clientName: "Acme DCR",
293+ redirectUri: FLOW_REDIRECT_URI,
294+ originIntegration: INTEG,
295+ });
296+
297+ expect(reused).toBe(legacySlug);
298+ const requests = yield* server.requests;
299+ expect(registerRequestCount(requests)).toBe(0);
300+ }),
301+ ),
302+ );
303+
304+ it.effect("uses resource to distinguish DCR clients only after an issuer already differs", () =>
305+ Effect.scoped(
306+ Effect.gen(function* () {
307+ const server = yield* serveOAuthTestServer({ scopes: ["read"] });
308+ const { executor } = yield* makeTestWorkspaceHarness({ plugins });
309+ yield* executor.acme.seed();
310+ const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl });
311+ const resourceA = `${server.issuerUrl}/mcp/a`;
312+ const resourceB = `${server.issuerUrl}/mcp/b`;
313+
314+ const first = yield* executor.oauth.registerDynamicClient({
315+ owner: "org",
316+ slug: OAuthClientSlug.make("first-resource"),
317+ issuer: probe.issuer,
318+ registrationEndpoint: probe.registrationEndpoint!,
319+ authorizationUrl: probe.authorizationUrl,
320+ tokenUrl: probe.tokenUrl,
321+ resource: resourceA,
322+ scopes: ["read"],
323+ tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
324+ clientName: "Resource A",
325+ redirectUri: FLOW_REDIRECT_URI,
326+ originIntegration: INTEG,
327+ });
328+ expect(String(first)).toBe(String(dcrSlugFor(server.issuerUrl)));
329+
330+ const second = yield* executor.oauth.registerDynamicClient({
331+ owner: "org",
332+ slug: OAuthClientSlug.make("second-resource"),
333+ issuer: probe.issuer,
334+ registrationEndpoint: probe.registrationEndpoint!,
335+ authorizationUrl: probe.authorizationUrl,
336+ tokenUrl: probe.tokenUrl,
337+ resource: resourceB,
338+ scopes: ["read"],
339+ tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
340+ clientName: "Resource B",
341+ redirectUri: FLOW_REDIRECT_URI,
342+ originIntegration: INTEG,
343+ });
344+ expect(String(second)).not.toBe(String(first));
345+ expect(String(second)).toContain(String(first));
346+ yield* server.clearRequests;
347+
348+ const third = yield* executor.oauth.registerDynamicClient({
349+ owner: "org",
350+ slug: OAuthClientSlug.make("third-resource"),
351+ issuer: probe.issuer,
352+ registrationEndpoint: probe.registrationEndpoint!,
353+ authorizationUrl: probe.authorizationUrl,
354+ tokenUrl: probe.tokenUrl,
355+ resource: resourceB,
356+ scopes: ["read"],
357+ tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
358+ clientName: "Resource B again",
359+ redirectUri: FLOW_REDIRECT_URI,
360+ originIntegration: IntegrationSlug.make("other"),
361+ });
362+
363+ expect(third).toBe(second);
364+ const requests = yield* server.requests;
365+ expect(registerRequestCount(requests)).toBe(0);
366+ }),
367+ ),
368+ );
369+
148370 // Regression: issue #770. Vercel (and other RFC 8252-strict servers) only
149371 // approve loopback redirect URIs for anonymous DCR, so a hosted/tailnet/LAN
150372 // origin trips `invalid_redirect_uri`. The failure must explain the loopback
packages/react/src/components/add-account-modal.test.ts+5−0
@@ -31,6 +31,7 @@ const apiKeyMethod = (id: string, source: "spec" | "custom", template = id): Aut · 30 unmodified lines
3131 });
3232
3333 type ProbeResult = {
34+ readonly issuer?: string | null;
3435 readonly authorizationUrl: string;
3536 readonly tokenUrl: string;
3637 readonly resource?: string | null;
@@ -42,6 +43,7 @@ type ProbeResult = { · 5 unmodified lines
4243 type RegisterArgs = {
4344 readonly owner: Owner;
4445 readonly slug: OAuthClientSlug;
46+ readonly issuer?: string | null;
4547 readonly registrationEndpoint: string;
4648 readonly authorizationUrl: string;
4749 readonly tokenUrl: string;
@@ -306,6 +308,7 @@ describe("runDcrConnect", () => { · 258 unmodified lines
306308 const probe = (_url: string): Promise<ProbeResult | null> => {
307309 calls.push("probe");
308310 return Promise.resolve({
311+ issuer: "https://auth.example.com",
309312 authorizationUrl: "https://auth.example.com/authorize",
310313 tokenUrl: "https://auth.example.com/token",
311314 resource: "https://mcp.example.com/mcp",
@@ -342,6 +345,8 @@ describe("runDcrConnect", () => { · 30 unmodified lines
342345 // DCR always mints an authorization-code/PKCE client; callers do not choose
343346 // a grant here.
344347 expect(registerArgs).not.toBeNull();
348+ expect(String(registerArgs!.slug)).toBe("dcr-auth-example-com");
349+ expect(registerArgs!.issuer).toBe("https://auth.example.com");
345350 expect(registerArgs!.registrationEndpoint).toBe("https://auth.example.com/register");
346351 expect(registerArgs!.authorizationUrl).toBe("https://auth.example.com/authorize");
347352 expect(registerArgs!.tokenUrl).toBe("https://auth.example.com/token");
packages/react/src/components/add-account-modal.tsx+8−2
@@ -47,6 +47,7 @@ import { · 46 unmodified lines
4747 import {
4848 clientDisplayName,
4949 clientHost,
50+ dcrClientSlug,
5051 uniqueClientSlug,
5152 useOAuthClientsForIntegration,
5253 type OAuthClientOption,
@@ -613,6 +614,7 @@ export async function runCimdConnect( · 560 unmodified lines
613614
614615 /** Discovery result from the probe step (subset of the `probeOAuth` response). */
615616 type DcrProbeResult = {
617+ readonly issuer?: string | null;
616618 readonly authorizationUrl: string;
617619 readonly tokenUrl: string;
618620 readonly resource?: string | null;
@@ -624,6 +626,7 @@ type DcrProbeResult = { · 5 unmodified lines
624626 type DcrRegisterArgs = {
625627 readonly owner: Owner;
626628 readonly slug: OAuthClientSlug;
629+ readonly issuer?: string | null;
627630 readonly registrationEndpoint: string;
628631 readonly authorizationUrl: string;
629632 readonly tokenUrl: string;
@@ -684,7 +687,7 @@ type RunDcrConnectInput = { · 54 unmodified lines
684687 readonly resourceFallback?: string;
685688 readonly owner: Owner;
686689 readonly integrationName: string;
687− /** The owner's existing client slugs, so the minted slug stays unique. */
690+ /** Kept for callers that still pass picker state. DCR slugs are server keyed. */
688691 readonly existingSlugs: readonly string[];
689692 /** Scopes declared by the integration's method (override the probed ones). */
690693 readonly declaredScopes?: readonly string[];
@@ -718,11 +721,12 @@ export async function runDcrConnect( · 27 unmodified lines
718721 const registrationEndpoint = probe.registrationEndpoint;
719722 if (!registrationEndpoint) return { kind: "fallback", reason: "no-registration-endpoint", probe };
720723
721− const slug = uniqueClientSlug(input.integrationName, input.existingSlugs);
724+ const slug = dcrClientSlug(probe.issuer ?? registrationEndpoint);
722725 const scopes = registrationScopes(input.declaredScopes ?? [], probe.scopesSupported ?? []);
723726 const minted = await deps.register({
724727 owner: input.owner,
725728 slug,
729+ issuer: probe.issuer ?? null,
726730 registrationEndpoint,
727731 authorizationUrl: probe.authorizationUrl,
728732 tokenUrl: probe.tokenUrl,
@@ -1444,6 +1448,7 @@ function AddAccountModalView(props: AddAccountModalProps) { · 715 unmodified lines
14441448 payload: {
14451449 owner: args.owner,
14461450 slug: args.slug,
1451+ issuer: args.issuer ?? null,
14471452 registrationEndpoint: args.registrationEndpoint,
14481453 authorizationUrl: args.authorizationUrl,
14491454 tokenUrl: args.tokenUrl,
@@ -1602,6 +1607,7 @@ function AddAccountModalView(props: AddAccountModalProps) { · 152 unmodified lines
16021607 null,
16031608 scopes: method.oauth?.scopes,
16041609 discoveredScopes: oauthFallbackProbe?.scopesSupported,
1610+ issuer: oauthFallbackProbe?.issuer ?? null,
16051611 registrationEndpoint:
16061612 method.oauth?.registrationEndpoint ??
16071613 oauthFallbackProbe?.registrationEndpoint ??
packages/react/src/components/oauth-client-form.tsx+7−3
@@ -9,7 +9,7 @@ import { ownerLabelForHost } from "../api/owner-display"; · 8 unmodified lines
99 import { trackEvent } from "../api/analytics";
1010 import { useOrganizationId } from "../api/organization-context";
1111 import { oauthClientWriteKeys } from "../api/reactivity-keys";
12−import { uniqueClientSlug } from "../plugins/use-effective-oauth-client";
12+import { dcrClientSlug, uniqueClientSlug } from "../plugins/use-effective-oauth-client";
1313 import { oauthCallbackUrl } from "../plugins/oauth-sign-in";
1414 import {
1515 ConnectionOwnerDropdown,
@@ -36,6 +36,7 @@ import { RadioGroup, RadioGroupItem } from "./radio-group"; · 20 unmodified lines
3636 // ---------------------------------------------------------------------------
3737
3838 export interface OAuthClientFormPrefill {
39+ readonly issuer?: string | null;
3940 readonly authorizationUrl?: string;
4041 readonly tokenUrl?: string;
4142 readonly resource?: string | null;
@@ -147,6 +148,7 @@ export function OAuthClientForm(props: { · 105 unmodified lines
147148 const [clientId, setClientId] = useState(prefill?.clientId ?? "");
148149 const [clientSecret, setClientSecret] = useState("");
149150 const [issuerUrl, setIssuerUrl] = useState("");
151+ const [discoveredIssuer, setDiscoveredIssuer] = useState<string | null>(prefill?.issuer ?? null);
150152 const [authorizationUrl, setAuthorizationUrl] = useState(prefill?.authorizationUrl ?? "");
151153 const [tokenUrl, setTokenUrl] = useState(prefill?.tokenUrl ?? "");
152154 const [resource, setResource] = useState(prefill?.resource ?? null);
@@ -226,6 +228,7 @@ export function OAuthClientForm(props: { · 73 unmodified lines
226228 return;
227229 }
228230 const result = exit.value;
231+ setDiscoveredIssuer(result.issuer ?? null);
229232 setAuthorizationUrl(result.authorizationUrl);
230233 setTokenUrl(result.tokenUrl);
231234 setResource(result.resource ?? null);
@@ -247,11 +250,12 @@ export function OAuthClientForm(props: { · 15 unmodified lines
247250 const handleRegisterDynamic = async () => {
248251 if (!canRegisterDynamic || registering) return;
249252 setRegistering(true);
250− const slug = fixedSlug ?? uniqueClientSlug(name, existingSlugs);
253+ const slug = fixedSlug ?? dcrClientSlug(discoveredIssuer ?? registrationEndpoint.trim());
251254 const exit = await doRegisterDynamic({
252255 payload: {
253256 owner,
254257 slug,
258+ issuer: discoveredIssuer,
255259 registrationEndpoint: registrationEndpoint.trim(),
256260 authorizationUrl: authorizationUrl.trim(),
257261 tokenUrl: tokenUrl.trim(),
@@ -273,7 +277,7 @@ export function OAuthClientForm(props: { · 15 unmodified lines
273277 }
274278 trackEvent("oauth_client_registered", { owner, grant, via_dcr: true, success: true });
275279 toast.success(`Registered ${integrationName} OAuth app`);
276− onCreated({ owner, slug });
280+ onCreated({ owner, slug: exit.value.client });
277281 };
278282
279283 const handleSubmit = async () => {
packages/react/src/plugins/use-effective-oauth-client.test.ts+10−0
@@ -2,6 +2,7 @@ import { describe, expect, it } from "@effect/vitest"; · 1 unmodified line
22 import { OAuthClientSlug, type Owner } from "@executor-js/sdk/shared";
33
44 import {
5+ dcrClientSlug,
56 selectClientsForEndpoints,
67 uniqueClientSlug,
78 type OAuthClientOption,
@@ -144,3 +145,12 @@ describe("uniqueClientSlug", () => { · 136 unmodified lines
144145 );
145146 });
146147 });
148+
149+describe("dcrClientSlug", () => {
150+ it("derives a deterministic DCR slug from the authorization server host", () => {
151+ expect(String(dcrClientSlug("https://mcp.cloudflare.com/register"))).toBe(
152+ "dcr-mcp-cloudflare-com",
153+ );
154+ expect(String(dcrClientSlug("http://127.0.0.1:8787/register"))).toBe("dcr-127-0-0-1-8787");
155+ });
156+});
packages/react/src/plugins/use-effective-oauth-client.tsx+7−0
@@ -199,6 +199,13 @@ export function uniqueClientSlug(name: string, existing: readonly string[]): OAu · 198 unmodified lines
199199 return OAuthClientSlug.make(`${base}-${suffix}`);
200200 }
201201
202+/** Deterministic DCR client slug derived from the authorization server host. */
203+export function dcrClientSlug(issuerOrEndpoint: string): OAuthClientSlug {
204+ const host = hostOf(issuerOrEndpoint);
205+ const base = host === undefined ? "" : slugifyName(host);
206+ return OAuthClientSlug.make(`dcr-${base || "authorization-server"}`);
207+}
208+
202209 /** Humanize a client slug for display ("spotify-prod" → "Spotify prod"). */
203210 export function clientDisplayName(slug: string): string {
204211 const text = slug.replace(/[-_]/g, " ").trim();