-
Notifications
You must be signed in to change notification settings - Fork 954
feat: support oidc discovery in client sdk #652
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: support oidc discovery in client sdk #652
Conversation
Hi @pcarleton , thanks for the heads-up on the spec change! I've now implemented the corresponding OIDC discovery support in the SDK. Would you mind taking a look when you get a chance? Thanks~ |
0b3f1bc
to
cf64a97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OICD and OAuth 2.0 AS metadata are no the same. They share a common base. We need to represent this in the type system appropriately so that clients can distinguish what to call.
This implement should have a common base interface that an OAuthMetadataSchema
and an OICDMetadataSchema
that inherit from it and discoverOAuthMetadata
should return a Promise<OAuthMetadataSchema | OICDMetadataSchema | undefined>
, that clients can discriminate on or downcast to the AuthserverBaseSchema
.
const fetchWithCorsFallback = async (url: URL, protocolVersion: string) => { | ||
try { | ||
return await fetch(url, { | ||
headers: { | ||
"MCP-Protocol-Version": protocolVersion | ||
} | ||
}) | ||
} catch (error) { | ||
if (error instanceof TypeError) { | ||
// CORS errors come back as TypeError, try again without protocol version header | ||
return await fetch(url); | ||
} | ||
throw error; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an async function fetchWithCorsFallback()
and not const fetchWithCorsFallback =
, to stay in line with everything else here and ensure that we define a proper return type.
/** | ||
* The `OAuthMetadataSchema` is compatible with both OIDC and OAuth2 | ||
* discovery responses. Because OIDC's metadata is a superset, `zod` will | ||
* correctly parse the fields defined in our schema and simply ignore any | ||
* additional OIDC-specific fields. | ||
*/ | ||
return OAuthMetadataSchema.parse(await response.json()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OICD is not a superset. They have a common base, but they do have both fields that they dont share, AS Metadata has introspection_endpoint
, revocation_endpoint
, introspection_endpoint_auth_methods_supported
and OICD has userinfo_endpoint
, subject_types_supported
, id_token_signing_alg_values_supported
for example.
tried adding more explicit typing here: main...feat-support-oidc-discovery-in-client-sdk |
Hi @dsp-ant , thank you so much for the detailed review. |
Hi @pcarleton , thanks for your input and for highlighting the relevant code. Feel free to push directly to this branch if you'd like to collaborate on the changes. No worries if not, I'm happy to handle the refactoring myself once the spec is ready. |
This PR adds support for OpenID Connect (OIDC) Discovery to the client SDK. The client will now attempt discovery from both the
/.well-known/oauth-authorization-server
and/.well-known/openid-configuration
endpoints.Motivation and Context
This change is required to align the SDK with the updated MCP specification, as requested in modelcontextprotocol/modelcontextprotocol#677. It improves compatibility with modern identity providers (e.g., Keycloak, Auth0, Logto) that use OIDC discovery.
How Has This Been Tested?
Unit tests have been updated to cover successful discovery from both OIDC and OAuth2 endpoints, as well as the case where both fail.
Breaking Changes
None. This change is fully backward-compatible.
Types of changes
Checklist