Add application_type to the Dynamic Client Registration request#1613
Conversation
application_type to the Dynamic Client Registration requestapplication_type to the Dynamic Client Registration request
5063493 to
cd9ba60
Compare
|
@mikekistler - can you please help reviewing this PR |
mikekistler
left a comment
There was a problem hiding this comment.
Looks good! 👍
Thank you for this contribution!
|
@mikekistler |
jeffhandley
left a comment
There was a problem hiding this comment.
Thanks for getting this together, @jayaraman-venkatesan.
Before we merge, SEP-837's normative language ("MCP clients MUST be prepared to handle registration failures…", "SHOULD surface a meaningful error", "MAY retry with adjusted application_type or redirect URIs") is aimed at MCP clients. As the C# SDK those clients are built on, we don't have to implement the surfacing or retry behavior ourselves, but we do need to make sure a developer using the SDK is able to do those things, and we should prove it with tests in this PR before we close out #1545.
Two test scenarios I'd like added here:
- DCR failure is recoverable from the consumer's perspective. When an OIDC AS rejects the DCR call (e.g. 400
invalid_redirect_uri), the failure should propagate to the SDK consumer with enough context: the HTTP status, the AS error body, and ideally theapplication_type/redirect_urithe SDK actually sent; so the consumer can produce a meaningful error. Please add a negative test that mocks an OIDC AS rejecting registration and asserts the thrown exception (or returned result) carries that information. - A consumer can retry registration with adjusted parameters. The SDK's resolved
ApplicationTypeis fixed in theClientOAuthProviderconstructor today, so a retry implies the consumer constructing a new provider with a differentApplicationType(or different redirect URI). Please add a test that does exactly that: first DCR attempt rejected, second attempt succeeds after swappingApplicationType; to (a) prove the surface supports the MAY behavior and (b) catch any subtle state-carryover bugs.
One related point worth thinking about while you're in here: the synchronous ArgumentException on conflict (e.g. "web" + localhost) is a nice guardrail, but it also blocks a consumer from retrying with a combination the AS happens to require; for example, if an AS demanded "web" paired with a localhost redirect, the SDK would refuse to even attempt that registration. Probably fine to leave the behavior as-is, but worth a sentence in the xmldoc noting that consumers who need that flexibility must set ApplicationType explicitly and that the inference will not override it. Up to you.
…ble and retryable and enrich the DCR-failure McpException with the application_type and redirect_uri the SDK sent
|
Thanks @jeffhandley, pushed 2643b0b addressing both test scenarios
The failure is build (windows-latest, Release), I suppose is pre-existing flake in McpServerBuilderExtensionsMessageFilterTests, a re-run should clear it, please help kick one off when you get a chance. Thanks! |
Fixes #1545
Implements SEP-837 — adds
application_typeto the Dynamic Client Registration request when the C# SDK registers against an OIDC-flavored authorization server.Summary
Without
application_type, an OIDC AS applies its spec-mandated default. This PR makes the SDK supply the correct value.What happens in the changes
When the caller does set
DynamicClientRegistrationOptions.ApplicationTypeexplicitly, the value is cross-checked against the inferred type atClientOAuthProviderconstruction time. A conflict (for example"web"paired with a localhost redirect URI) throwsArgumentExceptionsynchronously, before any HTTP request is sentWhat changed
src/ModelContextProtocol.Core/Authentication/DynamicClientRegistrationRequest.cs— adds theapplication_typefield on the wire DTO.DynamicClientRegistrationOptions.cs— adds theApplicationTypesetter so callers can opt out of inference and force a specific value.ClientOAuthProvider.cs— resolution happens once, in the constructor:InferApplicationType(Uri redirectUri)helper (mirrors Go sdk's inferApplicationType`).ArgumentExceptionon conflict; otherwise stores the resolved value and writes it back ontooptions.DynamicClientRegistration.ApplicationTypeso callers can observe what was selected.tests/ModelContextProtocol.Tests/Authentication/ClientOAuthProviderApplicationTypeTests.cs(new)127.0.0.1,[::1], custom scheme, https remote.(Parameter 'options').Contract changes
DynamicClientRegistrationOptions:ApplicationType { get; set; }. Backwards-compatible — existing callers default tonulland now get an inferred value where they previously got nothing on the wire.ClientOAuthProvider(constructed viaHttpClientTransport) now throwsArgumentExceptionwhen an explicitApplicationTypeconflicts with the redirect URI shape.ApplicationTypeback tooptions.DynamicClientRegistration.ApplicationType. This is documented in the xmldoc and mirrors Go's behavior.Test plan
dotnet build src/ModelContextProtocol.Core— 0 warnings, 0 errors.dotnet test tests/ModelContextProtocol.Tests(net10.0) — 1969 passed, 0 failed, 5 skippedClientOAuthProviderApplicationTypeTests— 9/9 pass, including exact-message assertion on the conflict throw.