Skip to content

Commit 9c3daec

Browse files
btiernaybtiernay
committed
feat: unify OAuth scope discovery between automatic and manual flows
- Add shared discoverScopes function in auth.ts with resource metadata preference - Update automatic flow (handleAuthError) to use dynamic scope discovery - Update manual flow (OAuth state machine) to use shared scope discovery logic - Add comprehensive test suite for discoverScopes function (11 test cases) - Fix empty array handling to return undefined instead of empty string - Maintain backward compatibility with existing oauthScope parameter - All quality gates pass: TypeScript, linting, formatting, and tests Resolves OAuth scope discovery inconsistency where automatic flow used static scopes while manual flow used metadata scopes. Both flows now use identical shared logic with proper resource metadata preference.
1 parent 618370d commit 9c3daec

File tree

6 files changed

+449
-20
lines changed

6 files changed

+449
-20
lines changed

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

Lines changed: 65 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,40 @@ 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(),
81+
saveClientInformation: jest.fn(),
82+
saveTokens: jest.fn(),
83+
redirectToAuthorization: jest.fn(),
84+
saveCodeVerifier: jest.fn(),
85+
codeVerifier: jest.fn(),
86+
saveServerMetadata: jest.fn(),
87+
getServerMetadata: jest.fn(),
88+
})),
89+
discoverScopes: jest.fn().mockResolvedValue("read write" as never),
90+
}));
91+
92+
import { discoverScopes } from "@/lib/auth";
93+
5994
// Type the mocked functions properly
6095
const mockDiscoverAuthorizationServerMetadata =
6196
discoverAuthorizationServerMetadata as jest.MockedFunction<
@@ -75,6 +110,9 @@ const mockDiscoverOAuthProtectedResourceMetadata =
75110
discoverOAuthProtectedResourceMetadata as jest.MockedFunction<
76111
typeof discoverOAuthProtectedResourceMetadata
77112
>;
113+
const mockDiscoverScopes = discoverScopes as jest.MockedFunction<
114+
typeof discoverScopes
115+
>;
78116

79117
const sessionStorageMock = {
80118
getItem: jest.fn(),
@@ -103,9 +141,15 @@ describe("AuthDebugger", () => {
103141
// Suppress console errors in tests to avoid JSDOM navigation noise
104142
jest.spyOn(console, "error").mockImplementation(() => {});
105143

106-
mockDiscoverAuthorizationServerMetadata.mockResolvedValue(
107-
mockOAuthMetadata,
108-
);
144+
// Set default mock behaviors with complete OAuth metadata
145+
mockDiscoverAuthorizationServerMetadata.mockResolvedValue({
146+
issuer: "https://oauth.example.com",
147+
authorization_endpoint: "https://oauth.example.com/authorize",
148+
token_endpoint: "https://oauth.example.com/token",
149+
response_types_supported: ["code"],
150+
grant_types_supported: ["authorization_code"],
151+
scopes_supported: ["read", "write"],
152+
});
109153
mockRegisterClient.mockResolvedValue(mockOAuthClientInfo);
110154
mockDiscoverOAuthProtectedResourceMetadata.mockRejectedValue(
111155
new Error("No protected resource metadata found"),
@@ -427,7 +471,24 @@ describe("AuthDebugger", () => {
427471
});
428472
});
429473

430-
it("should not include scope in authorization URL when scopes_supported is not present", async () => {
474+
it("should include scope in authorization URL when scopes_supported is not present", async () => {
475+
const updateAuthState =
476+
await setupAuthorizationUrlTest(mockOAuthMetadata);
477+
478+
// Wait for the updateAuthState to be called
479+
await waitFor(() => {
480+
expect(updateAuthState).toHaveBeenCalledWith(
481+
expect.objectContaining({
482+
authorizationUrl: expect.stringContaining("scope="),
483+
}),
484+
);
485+
});
486+
});
487+
488+
it("should omit scope from authorization URL when discoverScopes returns undefined", async () => {
489+
// Mock discoverScopes to return undefined (no scopes available)
490+
mockDiscoverScopes.mockResolvedValueOnce(undefined);
491+
431492
const updateAuthState =
432493
await setupAuthorizationUrlTest(mockOAuthMetadata);
433494

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

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
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+
const { scopes_supported, ...rest } = baseMetadata;
105+
return rest;
106+
})(),
107+
serverUrl: "https://example.com",
108+
},
109+
{
110+
name: "returns undefined with resource metadata but OAuth fails",
111+
mockRejects: new Error("No OAuth metadata"),
112+
serverUrl: "https://example.com",
113+
resourceMetadata: {
114+
resource: "https://example.com",
115+
scopes_supported: ["read", "write"],
116+
},
117+
},
118+
];
119+
120+
test.each(testCases)(
121+
"$name",
122+
async ({
123+
mockResolves,
124+
serverUrl,
125+
resourceMetadata,
126+
expected,
127+
expectedCallUrl,
128+
}) => {
129+
mockDiscoverAuth.mockResolvedValue(mockResolves);
130+
131+
const result = await discoverScopes(serverUrl, resourceMetadata);
132+
133+
expect(result).toBe(expected);
134+
if (expectedCallUrl) {
135+
expect(mockDiscoverAuth).toHaveBeenCalledWith(new URL(expectedCallUrl));
136+
}
137+
},
138+
);
139+
140+
test.each(undefinedCases)(
141+
"$name",
142+
async ({ mockResolves, mockRejects, serverUrl, resourceMetadata }) => {
143+
if (mockRejects) {
144+
mockDiscoverAuth.mockRejectedValue(mockRejects);
145+
} else {
146+
mockDiscoverAuth.mockResolvedValue(mockResolves);
147+
}
148+
149+
const result = await discoverScopes(serverUrl, resourceMetadata);
150+
151+
expect(result).toBeUndefined();
152+
},
153+
);
154+
});

‎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)