Skip to content

Commit 24ef319

Browse files
cliffhallcliffhall
authored
Merge pull request #701 from btiernay/feat/oauth-scope-discovery-consistency
feat: unify OAuth scope discovery between automatic and manual flows
2 parents c5047ca + 739e0aa commit 24ef319

File tree

6 files changed

+469
-20
lines changed

6 files changed

+469
-20
lines changed

‎client/src/components/__tests__/AuthDebugger.test.tsx

Lines changed: 82 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ const mockOAuthMetadata = {
2525
token_endpoint: "https://oauth.example.com/token",
2626
response_types_supported: ["code"],
2727
grant_types_supported: ["authorization_code"],
28+
scopes_supported: ["read", "write"],
2829
};
2930

3031
const mockOAuthClientInfo = {
@@ -56,6 +57,57 @@ import {
5657
import { OAuthMetadata } from "@modelcontextprotocol/sdk/shared/auth.js";
5758
import { EMPTY_DEBUGGER_STATE } from "@/lib/auth-types";
5859

60+
// Mock local auth module
61+
jest.mock("@/lib/auth", () => ({
62+
DebugInspectorOAuthClientProvider: jest.fn().mockImplementation(() => ({
63+
tokens: jest.fn().mockImplementation(() => Promise.resolve(undefined)),
64+
clear: jest.fn().mockImplementation(() => {
65+
// Mock the real clear() behavior which removes items from sessionStorage
66+
sessionStorage.removeItem("[https://example.com/mcp] mcp_tokens");
67+
sessionStorage.removeItem("[https://example.com/mcp] mcp_client_info");
68+
sessionStorage.removeItem(
69+
"[https://example.com/mcp] mcp_server_metadata",
70+
);
71+
}),
72+
redirectUrl: "http://localhost:3000/oauth/callback/debug",
73+
clientMetadata: {
74+
redirect_uris: ["http://localhost:3000/oauth/callback/debug"],
75+
token_endpoint_auth_method: "none",
76+
grant_types: ["authorization_code", "refresh_token"],
77+
response_types: ["code"],
78+
client_name: "MCP Inspector",
79+
},
80+
clientInformation: jest.fn().mockImplementation(async () => {
81+
const serverUrl = "https://example.com/mcp";
82+
const preregisteredKey = `[${serverUrl}] ${SESSION_KEYS.PREREGISTERED_CLIENT_INFORMATION}`;
83+
const preregisteredData = sessionStorage.getItem(preregisteredKey);
84+
if (preregisteredData) {
85+
return JSON.parse(preregisteredData);
86+
}
87+
const dynamicKey = `[${serverUrl}] ${SESSION_KEYS.CLIENT_INFORMATION}`;
88+
const dynamicData = sessionStorage.getItem(dynamicKey);
89+
if (dynamicData) {
90+
return JSON.parse(dynamicData);
91+
}
92+
return undefined;
93+
}),
94+
saveClientInformation: jest.fn().mockImplementation((clientInfo) => {
95+
const serverUrl = "https://example.com/mcp";
96+
const key = `[${serverUrl}] ${SESSION_KEYS.CLIENT_INFORMATION}`;
97+
sessionStorage.setItem(key, JSON.stringify(clientInfo));
98+
}),
99+
saveTokens: jest.fn(),
100+
redirectToAuthorization: jest.fn(),
101+
saveCodeVerifier: jest.fn(),
102+
codeVerifier: jest.fn(),
103+
saveServerMetadata: jest.fn(),
104+
getServerMetadata: jest.fn(),
105+
})),
106+
discoverScopes: jest.fn().mockResolvedValue("read write" as never),
107+
}));
108+
109+
import { discoverScopes } from "@/lib/auth";
110+
59111
// Type the mocked functions properly
60112
const mockDiscoverAuthorizationServerMetadata =
61113
discoverAuthorizationServerMetadata as jest.MockedFunction<
@@ -75,6 +127,9 @@ const mockDiscoverOAuthProtectedResourceMetadata =
75127
discoverOAuthProtectedResourceMetadata as jest.MockedFunction<
76128
typeof discoverOAuthProtectedResourceMetadata
77129
>;
130+
const mockDiscoverScopes = discoverScopes as jest.MockedFunction<
131+
typeof discoverScopes
132+
>;
78133

79134
const sessionStorageMock = {
80135
getItem: jest.fn(),
@@ -103,9 +158,15 @@ describe("AuthDebugger", () => {
103158
// Suppress console errors in tests to avoid JSDOM navigation noise
104159
jest.spyOn(console, "error").mockImplementation(() => {});
105160

106-
mockDiscoverAuthorizationServerMetadata.mockResolvedValue(
107-
mockOAuthMetadata,
108-
);
161+
// Set default mock behaviors with complete OAuth metadata
162+
mockDiscoverAuthorizationServerMetadata.mockResolvedValue({
163+
issuer: "https://oauth.example.com",
164+
authorization_endpoint: "https://oauth.example.com/authorize",
165+
token_endpoint: "https://oauth.example.com/token",
166+
response_types_supported: ["code"],
167+
grant_types_supported: ["authorization_code"],
168+
scopes_supported: ["read", "write"],
169+
});
109170
mockRegisterClient.mockResolvedValue(mockOAuthClientInfo);
110171
mockDiscoverOAuthProtectedResourceMetadata.mockRejectedValue(
111172
new Error("No protected resource metadata found"),
@@ -427,7 +488,24 @@ describe("AuthDebugger", () => {
427488
});
428489
});
429490

430-
it("should not include scope in authorization URL when scopes_supported is not present", async () => {
491+
it("should include scope in authorization URL when scopes_supported is not present", async () => {
492+
const updateAuthState =
493+
await setupAuthorizationUrlTest(mockOAuthMetadata);
494+
495+
// Wait for the updateAuthState to be called
496+
await waitFor(() => {
497+
expect(updateAuthState).toHaveBeenCalledWith(
498+
expect.objectContaining({
499+
authorizationUrl: expect.stringContaining("scope="),
500+
}),
501+
);
502+
});
503+
});
504+
505+
it("should omit scope from authorization URL when discoverScopes returns undefined", async () => {
506+
// Mock discoverScopes to return undefined (no scopes available)
507+
mockDiscoverScopes.mockResolvedValueOnce(undefined);
508+
431509
const updateAuthState =
432510
await setupAuthorizationUrlTest(mockOAuthMetadata);
433511

‎client/src/lib/__tests__/auth.test.ts

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
import { discoverScopes } from "../auth";
2+
import { discoverAuthorizationServerMetadata } from "@modelcontextprotocol/sdk/client/auth.js";
3+
4+
jest.mock("@modelcontextprotocol/sdk/client/auth.js", () => ({
5+
discoverAuthorizationServerMetadata: jest.fn(),
6+
}));
7+
8+
const mockDiscoverAuth =
9+
discoverAuthorizationServerMetadata as jest.MockedFunction<
10+
typeof discoverAuthorizationServerMetadata
11+
>;
12+
13+
const baseMetadata = {
14+
issuer: "https://test.com",
15+
authorization_endpoint: "https://test.com/authorize",
16+
token_endpoint: "https://test.com/token",
17+
response_types_supported: ["code"],
18+
grant_types_supported: ["authorization_code"],
19+
scopes_supported: ["read", "write"],
20+
};
21+
22+
describe("discoverScopes", () => {
23+
beforeEach(() => {
24+
jest.clearAllMocks();
25+
});
26+
27+
const testCases = [
28+
{
29+
name: "returns joined scopes from OAuth metadata",
30+
mockResolves: baseMetadata,
31+
serverUrl: "https://example.com",
32+
expected: "read write",
33+
expectedCallUrl: "https://example.com/",
34+
},
35+
{
36+
name: "prefers resource metadata over OAuth metadata",
37+
mockResolves: baseMetadata,
38+
serverUrl: "https://example.com",
39+
resourceMetadata: {
40+
resource: "https://example.com",
41+
scopes_supported: ["admin", "full"],
42+
},
43+
expected: "admin full",
44+
},
45+
{
46+
name: "falls back to OAuth when resource has empty scopes",
47+
mockResolves: baseMetadata,
48+
serverUrl: "https://example.com",
49+
resourceMetadata: {
50+
resource: "https://example.com",
51+
scopes_supported: [],
52+
},
53+
expected: "read write",
54+
},
55+
{
56+
name: "normalizes URL with port and path",
57+
mockResolves: baseMetadata,
58+
serverUrl: "https://example.com:8080/some/path",
59+
expected: "read write",
60+
expectedCallUrl: "https://example.com:8080/",
61+
},
62+
{
63+
name: "normalizes URL with trailing slash",
64+
mockResolves: baseMetadata,
65+
serverUrl: "https://example.com/",
66+
expected: "read write",
67+
expectedCallUrl: "https://example.com/",
68+
},
69+
{
70+
name: "handles single scope",
71+
mockResolves: { ...baseMetadata, scopes_supported: ["admin"] },
72+
serverUrl: "https://example.com",
73+
expected: "admin",
74+
},
75+
{
76+
name: "prefers resource metadata even with fewer scopes",
77+
mockResolves: {
78+
...baseMetadata,
79+
scopes_supported: ["read", "write", "admin", "full"],
80+
},
81+
serverUrl: "https://example.com",
82+
resourceMetadata: {
83+
resource: "https://example.com",
84+
scopes_supported: ["read"],
85+
},
86+
expected: "read",
87+
},
88+
];
89+
90+
const undefinedCases = [
91+
{
92+
name: "returns undefined when OAuth discovery fails",
93+
mockRejects: new Error("Discovery failed"),
94+
serverUrl: "https://example.com",
95+
},
96+
{
97+
name: "returns undefined when OAuth has no scopes",
98+
mockResolves: { ...baseMetadata, scopes_supported: [] },
99+
serverUrl: "https://example.com",
100+
},
101+
{
102+
name: "returns undefined when scopes_supported missing",
103+
mockResolves: (() => {
104+
// eslint-disable-next-line @typescript-eslint/no-unused-vars
105+
const { scopes_supported, ...rest } = baseMetadata;
106+
return rest;
107+
})(),
108+
serverUrl: "https://example.com",
109+
},
110+
{
111+
name: "returns undefined with resource metadata but OAuth fails",
112+
mockRejects: new Error("No OAuth metadata"),
113+
serverUrl: "https://example.com",
114+
resourceMetadata: {
115+
resource: "https://example.com",
116+
scopes_supported: ["read", "write"],
117+
},
118+
},
119+
];
120+
121+
test.each(testCases)(
122+
"$name",
123+
async ({
124+
mockResolves,
125+
serverUrl,
126+
resourceMetadata,
127+
expected,
128+
expectedCallUrl,
129+
}) => {
130+
mockDiscoverAuth.mockResolvedValue(mockResolves);
131+
132+
const result = await discoverScopes(serverUrl, resourceMetadata);
133+
134+
expect(result).toBe(expected);
135+
if (expectedCallUrl) {
136+
expect(mockDiscoverAuth).toHaveBeenCalledWith(new URL(expectedCallUrl));
137+
}
138+
},
139+
);
140+
141+
test.each(undefinedCases)(
142+
"$name",
143+
async ({ mockResolves, mockRejects, serverUrl, resourceMetadata }) => {
144+
if (mockRejects) {
145+
mockDiscoverAuth.mockRejectedValue(mockRejects);
146+
} else {
147+
mockDiscoverAuth.mockResolvedValue(mockResolves);
148+
}
149+
150+
const result = await discoverScopes(serverUrl, resourceMetadata);
151+
152+
expect(result).toBeUndefined();
153+
},
154+
);
155+
});

‎client/src/lib/auth.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,45 @@ import {
66
OAuthTokensSchema,
77
OAuthClientMetadata,
88
OAuthMetadata,
9+
OAuthProtectedResourceMetadata,
910
} from "@modelcontextprotocol/sdk/shared/auth.js";
11+
import { discoverAuthorizationServerMetadata } from "@modelcontextprotocol/sdk/client/auth.js";
1012
import { SESSION_KEYS, getServerSpecificKey } from "./constants";
1113
import { generateOAuthState } from "@/utils/oauthUtils";
1214

15+
/**
16+
* Discovers OAuth scopes from server metadata, with preference for resource metadata scopes
17+
* @param serverUrl - The MCP server URL
18+
* @param resourceMetadata - Optional resource metadata containing preferred scopes
19+
* @returns Promise resolving to space-separated scope string or undefined
20+
*/
21+
export const discoverScopes = async (
22+
serverUrl: string,
23+
resourceMetadata?: OAuthProtectedResourceMetadata,
24+
): Promise<string | undefined> => {
25+
try {
26+
const metadata = await discoverAuthorizationServerMetadata(
27+
new URL("/", serverUrl),
28+
);
29+
30+
// Prefer resource metadata scopes, but fall back to OAuth metadata if empty
31+
const resourceScopes = resourceMetadata?.scopes_supported;
32+
const oauthScopes = metadata?.scopes_supported;
33+
34+
const scopesSupported =
35+
resourceScopes && resourceScopes.length > 0
36+
? resourceScopes
37+
: oauthScopes;
38+
39+
return scopesSupported && scopesSupported.length > 0
40+
? scopesSupported.join(" ")
41+
: undefined;
42+
} catch (error) {
43+
console.debug("OAuth scope discovery failed:", error);
44+
return undefined;
45+
}
46+
};
47+
1348
export const getClientInformationFromSessionStorage = async ({
1449
serverUrl,
1550
isPreregistered,

0 commit comments

Comments
 (0)