refactor resource selection to not include resource if PRM is not present
This commit is contained in:
Paul Carleton
parent
ebf8e2d029
commit
15a2277c89
2 changed files with 241 additions and 17 deletions
|
|
@ -954,10 +954,19 @@ describe("OAuth Authorization", () => {
|
|||
});
|
||||
|
||||
it("passes resource parameter through authorization flow", async () => {
|
||||
// Mock successful metadata discovery
|
||||
// Mock successful metadata discovery - need to include protected resource metadata
|
||||
mockFetch.mockImplementation((url) => {
|
||||
const urlString = url.toString();
|
||||
if (urlString.includes("/.well-known/oauth-authorization-server")) {
|
||||
if (urlString.includes("/.well-known/oauth-protected-resource")) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
json: async () => ({
|
||||
resource: "https://api.example.com/mcp-server",
|
||||
authorization_servers: ["https://auth.example.com"],
|
||||
}),
|
||||
});
|
||||
} else if (urlString.includes("/.well-known/oauth-authorization-server")) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
|
|
@ -1002,11 +1011,20 @@ describe("OAuth Authorization", () => {
|
|||
});
|
||||
|
||||
it("includes resource in token exchange when authorization code is provided", async () => {
|
||||
// Mock successful metadata discovery and token exchange
|
||||
// Mock successful metadata discovery and token exchange - need protected resource metadata
|
||||
mockFetch.mockImplementation((url) => {
|
||||
const urlString = url.toString();
|
||||
|
||||
if (urlString.includes("/.well-known/oauth-authorization-server")) {
|
||||
if (urlString.includes("/.well-known/oauth-protected-resource")) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
json: async () => ({
|
||||
resource: "https://api.example.com/mcp-server",
|
||||
authorization_servers: ["https://auth.example.com"],
|
||||
}),
|
||||
});
|
||||
} else if (urlString.includes("/.well-known/oauth-authorization-server")) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
|
|
@ -1062,11 +1080,20 @@ describe("OAuth Authorization", () => {
|
|||
});
|
||||
|
||||
it("includes resource in token refresh", async () => {
|
||||
// Mock successful metadata discovery and token refresh
|
||||
// Mock successful metadata discovery and token refresh - need protected resource metadata
|
||||
mockFetch.mockImplementation((url) => {
|
||||
const urlString = url.toString();
|
||||
|
||||
if (urlString.includes("/.well-known/oauth-authorization-server")) {
|
||||
if (urlString.includes("/.well-known/oauth-protected-resource")) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
json: async () => ({
|
||||
resource: "https://api.example.com/mcp-server",
|
||||
authorization_servers: ["https://auth.example.com"],
|
||||
}),
|
||||
});
|
||||
} else if (urlString.includes("/.well-known/oauth-authorization-server")) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
|
|
@ -1244,5 +1271,197 @@ describe("OAuth Authorization", () => {
|
|||
// Should use the PRM's resource value, not the full requested URL
|
||||
expect(authUrl.searchParams.get("resource")).toBe("https://api.example.com/");
|
||||
});
|
||||
|
||||
it("excludes resource parameter when Protected Resource Metadata is not present", async () => {
|
||||
// Mock metadata discovery where protected resource metadata is not available (404)
|
||||
// but authorization server metadata is available
|
||||
mockFetch.mockImplementation((url) => {
|
||||
const urlString = url.toString();
|
||||
|
||||
if (urlString.includes("/.well-known/oauth-protected-resource")) {
|
||||
// Protected resource metadata not available
|
||||
return Promise.resolve({
|
||||
ok: false,
|
||||
status: 404,
|
||||
});
|
||||
} else if (urlString.includes("/.well-known/oauth-authorization-server")) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
json: async () => ({
|
||||
issuer: "https://auth.example.com",
|
||||
authorization_endpoint: "https://auth.example.com/authorize",
|
||||
token_endpoint: "https://auth.example.com/token",
|
||||
response_types_supported: ["code"],
|
||||
code_challenge_methods_supported: ["S256"],
|
||||
}),
|
||||
});
|
||||
}
|
||||
|
||||
return Promise.resolve({ ok: false, status: 404 });
|
||||
});
|
||||
|
||||
// Mock provider methods
|
||||
(mockProvider.clientInformation as jest.Mock).mockResolvedValue({
|
||||
client_id: "test-client",
|
||||
client_secret: "test-secret",
|
||||
});
|
||||
(mockProvider.tokens as jest.Mock).mockResolvedValue(undefined);
|
||||
(mockProvider.saveCodeVerifier as jest.Mock).mockResolvedValue(undefined);
|
||||
(mockProvider.redirectToAuthorization as jest.Mock).mockResolvedValue(undefined);
|
||||
|
||||
// Call auth - should not include resource parameter
|
||||
const result = await auth(mockProvider, {
|
||||
serverUrl: "https://api.example.com/mcp-server",
|
||||
});
|
||||
|
||||
expect(result).toBe("REDIRECT");
|
||||
|
||||
// Verify the authorization URL does NOT include the resource parameter
|
||||
expect(mockProvider.redirectToAuthorization).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
searchParams: expect.any(URLSearchParams),
|
||||
})
|
||||
);
|
||||
|
||||
const redirectCall = (mockProvider.redirectToAuthorization as jest.Mock).mock.calls[0];
|
||||
const authUrl: URL = redirectCall[0];
|
||||
// Resource parameter should not be present when PRM is not available
|
||||
expect(authUrl.searchParams.has("resource")).toBe(false);
|
||||
});
|
||||
|
||||
it("excludes resource parameter in token exchange when Protected Resource Metadata is not present", async () => {
|
||||
// Mock metadata discovery - no protected resource metadata, but auth server metadata available
|
||||
mockFetch.mockImplementation((url) => {
|
||||
const urlString = url.toString();
|
||||
|
||||
if (urlString.includes("/.well-known/oauth-protected-resource")) {
|
||||
return Promise.resolve({
|
||||
ok: false,
|
||||
status: 404,
|
||||
});
|
||||
} else if (urlString.includes("/.well-known/oauth-authorization-server")) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
json: async () => ({
|
||||
issuer: "https://auth.example.com",
|
||||
authorization_endpoint: "https://auth.example.com/authorize",
|
||||
token_endpoint: "https://auth.example.com/token",
|
||||
response_types_supported: ["code"],
|
||||
code_challenge_methods_supported: ["S256"],
|
||||
}),
|
||||
});
|
||||
} else if (urlString.includes("/token")) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
json: async () => ({
|
||||
access_token: "access123",
|
||||
token_type: "Bearer",
|
||||
expires_in: 3600,
|
||||
refresh_token: "refresh123",
|
||||
}),
|
||||
});
|
||||
}
|
||||
|
||||
return Promise.resolve({ ok: false, status: 404 });
|
||||
});
|
||||
|
||||
// Mock provider methods for token exchange
|
||||
(mockProvider.clientInformation as jest.Mock).mockResolvedValue({
|
||||
client_id: "test-client",
|
||||
client_secret: "test-secret",
|
||||
});
|
||||
(mockProvider.codeVerifier as jest.Mock).mockResolvedValue("test-verifier");
|
||||
(mockProvider.saveTokens as jest.Mock).mockResolvedValue(undefined);
|
||||
|
||||
// Call auth with authorization code
|
||||
const result = await auth(mockProvider, {
|
||||
serverUrl: "https://api.example.com/mcp-server",
|
||||
authorizationCode: "auth-code-123",
|
||||
});
|
||||
|
||||
expect(result).toBe("AUTHORIZED");
|
||||
|
||||
// Find the token exchange call
|
||||
const tokenCall = mockFetch.mock.calls.find(call =>
|
||||
call[0].toString().includes("/token")
|
||||
);
|
||||
expect(tokenCall).toBeDefined();
|
||||
|
||||
const body = tokenCall![1].body as URLSearchParams;
|
||||
// Resource parameter should not be present when PRM is not available
|
||||
expect(body.has("resource")).toBe(false);
|
||||
expect(body.get("code")).toBe("auth-code-123");
|
||||
});
|
||||
|
||||
it("excludes resource parameter in token refresh when Protected Resource Metadata is not present", async () => {
|
||||
// Mock metadata discovery - no protected resource metadata, but auth server metadata available
|
||||
mockFetch.mockImplementation((url) => {
|
||||
const urlString = url.toString();
|
||||
|
||||
if (urlString.includes("/.well-known/oauth-protected-resource")) {
|
||||
return Promise.resolve({
|
||||
ok: false,
|
||||
status: 404,
|
||||
});
|
||||
} else if (urlString.includes("/.well-known/oauth-authorization-server")) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
json: async () => ({
|
||||
issuer: "https://auth.example.com",
|
||||
authorization_endpoint: "https://auth.example.com/authorize",
|
||||
token_endpoint: "https://auth.example.com/token",
|
||||
response_types_supported: ["code"],
|
||||
code_challenge_methods_supported: ["S256"],
|
||||
}),
|
||||
});
|
||||
} else if (urlString.includes("/token")) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
json: async () => ({
|
||||
access_token: "new-access123",
|
||||
token_type: "Bearer",
|
||||
expires_in: 3600,
|
||||
}),
|
||||
});
|
||||
}
|
||||
|
||||
return Promise.resolve({ ok: false, status: 404 });
|
||||
});
|
||||
|
||||
// Mock provider methods for token refresh
|
||||
(mockProvider.clientInformation as jest.Mock).mockResolvedValue({
|
||||
client_id: "test-client",
|
||||
client_secret: "test-secret",
|
||||
});
|
||||
(mockProvider.tokens as jest.Mock).mockResolvedValue({
|
||||
access_token: "old-access",
|
||||
refresh_token: "refresh123",
|
||||
});
|
||||
(mockProvider.saveTokens as jest.Mock).mockResolvedValue(undefined);
|
||||
|
||||
// Call auth with existing tokens (should trigger refresh)
|
||||
const result = await auth(mockProvider, {
|
||||
serverUrl: "https://api.example.com/mcp-server",
|
||||
});
|
||||
|
||||
expect(result).toBe("AUTHORIZED");
|
||||
|
||||
// Find the token refresh call
|
||||
const tokenCall = mockFetch.mock.calls.find(call =>
|
||||
call[0].toString().includes("/token")
|
||||
);
|
||||
expect(tokenCall).toBeDefined();
|
||||
|
||||
const body = tokenCall![1].body as URLSearchParams;
|
||||
// Resource parameter should not be present when PRM is not available
|
||||
expect(body.has("resource")).toBe(false);
|
||||
expect(body.get("grant_type")).toBe("refresh_token");
|
||||
expect(body.get("refresh_token")).toBe("refresh123");
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -198,19 +198,24 @@ export async function auth(
|
|||
}
|
||||
|
||||
export async function selectResourceURL(serverUrl: string| URL, provider: OAuthClientProvider, resourceMetadata?: OAuthProtectedResourceMetadata): Promise<URL | undefined> {
|
||||
let resource = resourceUrlFromServerUrl(serverUrl);
|
||||
const defaultResource = resourceUrlFromServerUrl(serverUrl);
|
||||
|
||||
// If provider has custom validation, delegate to it
|
||||
if (provider.validateResourceURL) {
|
||||
return await provider.validateResourceURL(resource, resourceMetadata?.resource);
|
||||
} else if (resourceMetadata) {
|
||||
if (checkResourceAllowed({ requestedResource: resource, configuredResource: resourceMetadata.resource })) {
|
||||
// If the resource mentioned in metadata is valid, prefer it since it is what the server is telling us to request.
|
||||
resource = new URL(resourceMetadata.resource);
|
||||
} else {
|
||||
throw new Error(`Protected resource ${resourceMetadata.resource} does not match expected ${resource} (or origin)`);
|
||||
}
|
||||
return await provider.validateResourceURL(defaultResource, resourceMetadata?.resource);
|
||||
}
|
||||
|
||||
return resource;
|
||||
// Only include resource parameter when Protected Resource Metadata is present
|
||||
if (!resourceMetadata) {
|
||||
return undefined;
|
||||
}
|
||||
|
||||
// Validate that the metadata's resource is compatible with our request
|
||||
if (!checkResourceAllowed({ requestedResource: defaultResource, configuredResource: resourceMetadata.resource })) {
|
||||
throw new Error(`Protected resource ${resourceMetadata.resource} does not match expected ${defaultResource} (or origin)`);
|
||||
}
|
||||
// Prefer the resource from metadata since it's what the server is telling us to request
|
||||
return new URL(resourceMetadata.resource);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -360,7 +365,7 @@ export async function discoverOAuthMetadata(
|
|||
try {
|
||||
const rootUrl = new URL("/.well-known/oauth-authorization-server", issuer);
|
||||
response = await tryMetadataDiscovery(rootUrl, protocolVersion);
|
||||
|
||||
|
||||
if (response.status === 404) {
|
||||
return undefined;
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue