-
Notifications
You must be signed in to change notification settings - Fork 342
WIP: Authorization Support (Using ASP.NET Core Native AuthN/AuthZ Integration) #377
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?
Conversation
src/ModelContextProtocol.AspNetCore/Auth/McpAuthorizationExtensions.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.AspNetCore/Auth/McpAuthorizationExtensions.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.AspNetCore/Auth/McpAuthorizationExtensions.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.AspNetCore/Auth/McpAuthorizationExtensions.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.AspNetCore/Auth/McpAuthenticationResponseMiddlewareExtensions.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Halter <halter73@gmail.com>
Co-authored-by: Stephen Halter <halter73@gmail.com>
…sions.cs Co-authored-by: Stephen Halter <halter73@gmail.com>
src/ModelContextProtocol.AspNetCore/Auth/McpAuthorizationExtensions.cs
Outdated
Show resolved
Hide resolved
var response = await _httpClient.GetAsync(new Uri(baseUrl + path), cancellationToken); | ||
if (response.IsSuccessStatusCode) | ||
{ | ||
var json = await response.Content.ReadAsStringAsync(cancellationToken); |
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.
Using ReadAsStream and then Deserializing the stream will save allocating the string because the Stream contents are not buffered.
var response = await _httpClient.SendAsync(request, cancellationToken); | ||
if (response.IsSuccessStatusCode) | ||
{ | ||
var json = await response.Content.ReadAsStringAsync(cancellationToken); |
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.
Same as above. Use ReadAsStream
/// <summary> | ||
/// Default values used by MCP authentication. | ||
/// </summary> | ||
public static class McpAuthenticationDefaults |
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.
Does this need to be public? Who consumes this? Are these documented in the MCP spec?
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 is a convention for ASP.NET Core authentication handlers. It's similar to CookieAuthenticationDefaults, JwtBearerDefaults, etc. Scheme names are used to identify authentication handlers since there can be more than one. They're not part of any spec. The ProtectedMcpServer sample demonstrates how it's used.
src/ModelContextProtocol.AspNetCore/Authentication/McpAuthenticationHandler.cs
Outdated
Show resolved
Hide resolved
…cationHandler.cs Co-authored-by: Stephen Toub <stoub@microsoft.com>
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.
Let me know if you need help with anything. I can take over applying the feedback I suggested once you think the PR is good otherwise.
|
||
# Misc project metadata | ||
.specs/ |
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.
Out of curiosity, what's this for? If it's some sort of dev tool, it might be better to call out what it is.
/// <summary> | ||
/// Default values used by MCP authentication. | ||
/// </summary> | ||
public static class McpAuthenticationDefaults |
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 is a convention for ASP.NET Core authentication handlers. It's similar to CookieAuthenticationDefaults, JwtBearerDefaults, etc. Scheme names are used to identify authentication handlers since there can be more than one. They're not part of any spec. The ProtectedMcpServer sample demonstrates how it's used.
src/ModelContextProtocol.AspNetCore/Authentication/McpAuthenticationEvents.cs
Show resolved
Hide resolved
src/ModelContextProtocol.AspNetCore/Authentication/McpAuthenticationExceptions.cs
Outdated
Show resolved
Hide resolved
/// allowing dynamic customization based on the caller or other contextual information. | ||
/// This takes precedence over the static <see cref="ResourceMetadata"/> property. | ||
/// </remarks> | ||
public Func<HttpContext, ProtectedResourceMetadata>? ProtectedResourceMetadataProvider |
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.
Can we make this an McpAuthenticationEvent? This might have gotten missed from my last review.
Thanks. The main reason for suggesting adding McpAuthenticationEvents now is because it would be technically breaking to add later. But looking at the current state, I think that McpAuthenticationOptions.UseDynamicResourceMetadata(Func<HttpContext, ProtectedResourceMetadata> provider) andFunc<HttpContext, ProtectedResourceMetadata> McpAuthenticationOptions.ProtectedResourceMetadataProvider { get; set; } should be replaced by a Func<RetrieveResourceMetadataContext, Task> McpAuthenticationEvents.RetrieveResourceMetadata { get; set; } event similar to what we have for JwtBearerEvents.OnChallenge.
I think the "Use" in UseDynamicResourceMetadata wrongly implies that it's configuring a pipeline rather than being another way for overriding a Func property. All the other callbacks I can think that you can configure for any other auth handlers are always on WhateverAuthHandlerOptions.Events.Whatever, and not directly on the options type.
I know that unlike JWT's OnChallenge event, this gets called for both the challenge and the /.well-known/oauth-protected resource callback, but I think that's fine. We can also continue to just use the _resourceMetadata field by default when the event doesn't configure RetrieveResourceMetadataContext.ResourceMetadataAddress. We'll just document it. The RetreiveResourceMetadataContext would derive from BaseContext like JwtBearerChallengeContext does, and that exposes the HttpContext plus more like the current scheme name.
The other thing you'll notice about the events pattern that types like JwtBearerEvents follow is that that also have virtual methods you can override to implement events as an alternative to setting the property. We should also do that if we make the McpAuthenticationHandler an unsealed public type, since we should design it to be inherited from if that's the case. The alternative is to make the handler internal and seal that plus the still-public options and event types. This would make the AuthenticationBuilder.AddMcp extension method the only way to add the handler, but I think that's okay. We did just that with the AuthenticationBuilder.AddBearerToken method in the internal BearerTokenHandler (not to be confused with the JwtBearerHandler).
I also wonder if it'd be more consistent to make the ResourceMetadataUri Uri a ResourceMetadataAddress string instead for consistency with the type JwtBearerOptions.MetadataAddress and JwtBearerOptions.Authority. I'm aware these are not precisely the same thing. I'm just talking about it in terms of string vs Uri and consistency.
src/ModelContextProtocol.AspNetCore/Authentication/McpAuthenticationOptions.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.Core/Authentication/GenericOAuthProvider.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.Core/Authentication/GenericOAuthProvider.cs
Outdated
Show resolved
Hide resolved
/// <summary> | ||
/// A delegating handler that adds authentication tokens to requests and handles 401 responses. | ||
/// </summary> | ||
public class AuthorizationDelegatingHandler : DelegatingHandler |
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.
I'm wondering if you saw this comment from earlier and whether you agree with it.
I don't think we should put this retry logic in the DelegatingHandler. And even if we keep the AuthorizationDelegatingHandler, we should make it internal so we can change the implementation later.
Instead, we should instead surface the 401 Unauthorized HttpResponseMessages to the SseClientTransport and then have it call into the IMcpAuthorizationProvider and update the Authorization header before resending the request. That way we don't have to worry about the DelegatingHandler ever getting used with non-replayable request bodies.
I don't care that if ExtractServerSupportedSchemes and the like literally live in SseClientTransport.cs, but it should not be part of a DelegatingHandler. We should try to factor the code to make it clear we are only replaying JsonRpcMessage request bodies that we created ourselves.
Co-authored-by: Stephen Halter <halter73@gmail.com>
…cationExceptions.cs Co-authored-by: Stephen Halter <halter73@gmail.com>
…cationOptions.cs Co-authored-by: Stephen Halter <halter73@gmail.com>
…der.cs Co-authored-by: Stephen Halter <halter73@gmail.com>
…der.cs Co-authored-by: Stephen Halter <halter73@gmail.com>
…xtprotocol/csharp-sdk into localden/experimental
Implements the authorization flow for clients and servers, per specification. Instead of re-implementing everything from scratch, this follows the suggestions from #349 and uses the native ASP.NET Core constructs to handle post-discovery steps server-side.
Developer experience
Server
HTTP context in tools
.AddHttpContextAccessor
is used to ensure that tools can access the HTTP context (such as the authorization header contents).Tools that want to use the HTTP context will need to amend their signatures to include a reference to
IHttpContextAccessor
, like this:Client