Skip to content

Commit ee66f01

Browse files
cliffhallcliffhall
authored
Merge pull request #1153 from BobDickinson/pr-1133-v1.5
OAuth: use authorization_servers URL from resource metadata
2 parents 632e967 + 5128c60 commit ee66f01

File tree

4 files changed

+202
-13
lines changed

4 files changed

+202
-13
lines changed

‎core/__tests__/auth/discovery.test.ts‎

Lines changed: 139 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
import { describe, it, expect, vi, beforeEach } from "vitest";
2-
import { discoverScopes } from "../../auth/discovery.js";
2+
import {
3+
discoverScopes,
4+
getAuthorizationServerUrl,
5+
} from "../../auth/discovery.js";
36
import type { OAuthProtectedResourceMetadata } from "@modelcontextprotocol/sdk/shared/auth.js";
47

58
// Mock SDK functions
@@ -176,4 +179,139 @@ describe("OAuth Scope Discovery", () => {
176179
{ fetchFn: mockFetchFn },
177180
);
178181
});
182+
183+
it("should use authorization_servers URL from resource metadata for discovery (different domain)", async () => {
184+
const { discoverAuthorizationServerMetadata } =
185+
await import("@modelcontextprotocol/sdk/client/auth.js");
186+
vi.mocked(discoverAuthorizationServerMetadata).mockResolvedValue({
187+
issuer: "https://auth-server.com",
188+
authorization_endpoint: "https://auth-server.com/authorize",
189+
token_endpoint: "https://auth-server.com/token",
190+
response_types_supported: ["code"],
191+
scopes_supported: ["read", "write"],
192+
});
193+
194+
const resourceMetadata: OAuthProtectedResourceMetadata = {
195+
resource: "https://mcp-server.com",
196+
authorization_servers: ["https://auth-server.com/"],
197+
scopes_supported: ["read", "write"],
198+
};
199+
200+
const scopes = await discoverScopes(
201+
"https://mcp-server.com",
202+
resourceMetadata,
203+
);
204+
205+
expect(scopes).toBe("read write");
206+
expect(discoverAuthorizationServerMetadata).toHaveBeenCalledWith(
207+
new URL("https://auth-server.com/"),
208+
{ fetchFn: undefined },
209+
);
210+
});
211+
212+
it("should preserve full path in authorization_servers URL", async () => {
213+
const { discoverAuthorizationServerMetadata } =
214+
await import("@modelcontextprotocol/sdk/client/auth.js");
215+
vi.mocked(discoverAuthorizationServerMetadata).mockResolvedValue({
216+
issuer: "https://auth-server.com/realms/my-realm",
217+
authorization_endpoint:
218+
"https://auth-server.com/realms/my-realm/authorize",
219+
token_endpoint: "https://auth-server.com/realms/my-realm/token",
220+
response_types_supported: ["code"],
221+
scopes_supported: ["read", "write"],
222+
});
223+
224+
const resourceMetadata: OAuthProtectedResourceMetadata = {
225+
resource: "https://mcp-server.com",
226+
authorization_servers: ["https://auth-server.com/realms/my-realm/"],
227+
scopes_supported: ["read", "write"],
228+
};
229+
230+
const scopes = await discoverScopes(
231+
"https://mcp-server.com",
232+
resourceMetadata,
233+
);
234+
235+
expect(scopes).toBe("read write");
236+
expect(discoverAuthorizationServerMetadata).toHaveBeenCalledWith(
237+
new URL("https://auth-server.com/realms/my-realm/"),
238+
{ fetchFn: undefined },
239+
);
240+
});
241+
242+
it("should fall back to serverUrl when authorization_servers is empty", async () => {
243+
const { discoverAuthorizationServerMetadata } =
244+
await import("@modelcontextprotocol/sdk/client/auth.js");
245+
vi.mocked(discoverAuthorizationServerMetadata).mockResolvedValue({
246+
issuer: "https://mcp-server.com",
247+
authorization_endpoint: "https://mcp-server.com/authorize",
248+
token_endpoint: "https://mcp-server.com/token",
249+
response_types_supported: ["code"],
250+
scopes_supported: ["read", "write"],
251+
});
252+
253+
const resourceMetadata: OAuthProtectedResourceMetadata = {
254+
resource: "https://mcp-server.com",
255+
authorization_servers: [],
256+
scopes_supported: ["read", "write"],
257+
};
258+
259+
const scopes = await discoverScopes(
260+
"https://mcp-server.com",
261+
resourceMetadata,
262+
);
263+
264+
expect(scopes).toBe("read write");
265+
expect(discoverAuthorizationServerMetadata).toHaveBeenCalledWith(
266+
new URL("/", "https://mcp-server.com"),
267+
{ fetchFn: undefined },
268+
);
269+
});
270+
});
271+
272+
describe("getAuthorizationServerUrl", () => {
273+
const serverUrl = "https://mcp.example.com";
274+
275+
it("returns server URL when resourceMetadata is null", () => {
276+
expect(getAuthorizationServerUrl(serverUrl, null)).toEqual(
277+
new URL("/", serverUrl),
278+
);
279+
});
280+
281+
it("returns server URL when resourceMetadata is undefined", () => {
282+
expect(getAuthorizationServerUrl(serverUrl)).toEqual(
283+
new URL("/", serverUrl),
284+
);
285+
});
286+
287+
it("returns server URL when authorization_servers is empty array", () => {
288+
const resourceMetadata: OAuthProtectedResourceMetadata = {
289+
resource: serverUrl,
290+
authorization_servers: [],
291+
};
292+
expect(getAuthorizationServerUrl(serverUrl, resourceMetadata)).toEqual(
293+
new URL("/", serverUrl),
294+
);
295+
});
296+
297+
it("falls back to server URL when authorization_servers[0] is empty string", () => {
298+
const resourceMetadata: OAuthProtectedResourceMetadata = {
299+
resource: serverUrl,
300+
authorization_servers: [""],
301+
};
302+
expect(getAuthorizationServerUrl(serverUrl, resourceMetadata)).toEqual(
303+
new URL("/", serverUrl),
304+
);
305+
});
306+
307+
it("returns authorization_servers[0] when present and truthy", () => {
308+
const authUrl = "https://auth.example.com/";
309+
const resourceMetadata: OAuthProtectedResourceMetadata = {
310+
resource: serverUrl,
311+
authorization_servers: [authUrl],
312+
};
313+
expect(getAuthorizationServerUrl(serverUrl, resourceMetadata)).toEqual(
314+
new URL(authUrl),
315+
);
316+
});
179317
});

‎core/__tests__/auth/state-machine.test.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,44 @@ describe("OAuthStateMachine", () => {
167167
);
168168
});
169169

170+
it("should use authorization_servers URL from resource metadata for auth server discovery", async () => {
171+
const authServerUrl = "https://auth-server.com/";
172+
const resourceMetaDifferentAuth: OAuthProtectedResourceMetadata = {
173+
resource: serverUrl,
174+
authorization_servers: [authServerUrl],
175+
scopes_supported: ["read", "write"],
176+
};
177+
const selectedResource = new URL(serverUrl);
178+
const {
179+
discoverOAuthProtectedResourceMetadata,
180+
discoverAuthorizationServerMetadata,
181+
selectResourceURL,
182+
} = await import("@modelcontextprotocol/sdk/client/auth.js");
183+
vi.mocked(discoverOAuthProtectedResourceMetadata).mockResolvedValue(
184+
resourceMetaDifferentAuth,
185+
);
186+
vi.mocked(selectResourceURL).mockResolvedValue(selectedResource);
187+
188+
const stateMachine = new OAuthStateMachine(
189+
serverUrl,
190+
mockProvider,
191+
updateState,
192+
);
193+
await stateMachine.executeStep(state);
194+
195+
expect(discoverAuthorizationServerMetadata).toHaveBeenCalledWith(
196+
new URL(authServerUrl),
197+
expect.any(Object),
198+
);
199+
expect(updateState).toHaveBeenCalledWith(
200+
expect.objectContaining({
201+
resourceMetadata: resourceMetaDifferentAuth,
202+
authServerUrl: new URL(authServerUrl),
203+
oauthStep: "client_registration",
204+
}),
205+
);
206+
});
207+
170208
it("should call selectResourceURL only when resource metadata is present", async () => {
171209
const { discoverOAuthProtectedResourceMetadata, selectResourceURL } =
172210
await import("@modelcontextprotocol/sdk/client/auth.js");

‎core/auth/discovery.ts‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,19 @@
11
import { discoverAuthorizationServerMetadata } from "@modelcontextprotocol/sdk/client/auth.js";
22
import type { OAuthProtectedResourceMetadata } from "@modelcontextprotocol/sdk/shared/auth.js";
33

4+
/**
5+
* Returns the URL to use for OAuth authorization server metadata discovery.
6+
* Uses resource metadata's authorization_servers[0] when present, otherwise the MCP server URL.
7+
*/
8+
export function getAuthorizationServerUrl(
9+
serverUrl: string,
10+
resourceMetadata?: OAuthProtectedResourceMetadata | null,
11+
): URL {
12+
const first = resourceMetadata?.authorization_servers?.[0];
13+
// Use truthy check to match original state-machine: empty string falls back to serverUrl
14+
return first ? new URL(first) : new URL("/", serverUrl);
15+
}
16+
417
/**
518
* Discovers OAuth scopes from server metadata, with preference for resource metadata scopes
619
* @param serverUrl - The MCP server URL
@@ -14,10 +27,13 @@ export const discoverScopes = async (
1427
fetchFn?: typeof fetch,
1528
): Promise<string | undefined> => {
1629
try {
17-
const metadata = await discoverAuthorizationServerMetadata(
18-
new URL("/", serverUrl),
19-
{ fetchFn },
30+
const authServerUrl = getAuthorizationServerUrl(
31+
serverUrl,
32+
resourceMetadata,
2033
);
34+
const metadata = await discoverAuthorizationServerMetadata(authServerUrl, {
35+
fetchFn,
36+
});
2137

2238
// Prefer resource metadata scopes, but fall back to OAuth metadata if empty
2339
const resourceScopes = resourceMetadata?.scopes_supported;

‎core/auth/state-machine.ts‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { OAuthStep, AuthGuidedState } from "./types.js";
22
import type { BaseOAuthClientProvider } from "./providers.js";
3-
import { discoverScopes } from "./discovery.js";
3+
import { discoverScopes, getAuthorizationServerUrl } from "./discovery.js";
44
import {
55
discoverAuthorizationServerMetadata,
66
registerClient,
@@ -32,20 +32,12 @@ export const oauthTransitions: Record<OAuthStep, StateTransition> = {
3232
metadata_discovery: {
3333
canTransition: async () => true,
3434
execute: async (context) => {
35-
// Default to discovering from the server's URL
36-
let authServerUrl: URL = new URL("/", context.serverUrl);
3735
let resourceMetadata: OAuthProtectedResourceMetadata | null = null;
3836
let resourceMetadataError: Error | null = null;
3937
try {
4038
resourceMetadata = await discoverOAuthProtectedResourceMetadata(
4139
context.serverUrl as string | URL,
4240
);
43-
if (resourceMetadata?.authorization_servers?.length) {
44-
const firstServer = resourceMetadata.authorization_servers[0];
45-
if (firstServer) {
46-
authServerUrl = new URL(firstServer);
47-
}
48-
}
4941
} catch (e) {
5042
if (e instanceof Error) {
5143
resourceMetadataError = e;
@@ -54,6 +46,11 @@ export const oauthTransitions: Record<OAuthStep, StateTransition> = {
5446
}
5547
}
5648

49+
const authServerUrl = getAuthorizationServerUrl(
50+
context.serverUrl,
51+
resourceMetadata,
52+
);
53+
5754
const resource: URL | undefined = resourceMetadata
5855
? await selectResourceURL(
5956
context.serverUrl,

0 commit comments

Comments
 (0)