-
Notifications
You must be signed in to change notification settings - Fork 495
Add middleware and authz support for server-side handlers #733
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
43deac5
to
58dc73a
Compare
- filters.md cleanup
@halter73 Looks fine, basic idea is the same. Like the pipeline building / middleware concept. Had to think a bit if having filters on the option class is the right choice ( i think that typical aspnet core patterns is to have it registered with DI directly and resolving IEnumerable<IFilter...> however i have no knowledge that otherwise indicate if thats better/worse or more correct way to do it. From user perspectiv i think the examples given is clean and understanable. This would be a fine way to do so. Should also be possible to do my sample from #703 adding all kind of filterings ontop using this approach. So to me the thing i would consider is if developers would like to be able to just do services.AddMCPFilter() and have it implement an interface, where thats not directly possibel right now as its handlers being added to the filter array on the options. But for me this works and solve the usecase i was going for. |
#### Individual Operations (CallTool, GetPrompt, ReadResource) | ||
For individual operations, the filters return authorization errors when access is denied: | ||
|
||
- **Tools**: Returns a `CallToolResult` with `IsError = true` and an error message |
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 am not sure about returning an authorization error for unlisted primitives. This allows sniffing out the names of primitives that one is not authorized to access. The MCP spec isn't really clear on whether it is allowed to call a tool or fetch a resource not present in the list.
In practice it shouldn't happen, as an LLM should never call a tool that isn't in the list. However many clients cache tool lists, so it probably will happen in cases where the list of available tools can shrink.
From an MCP client host developer perspective I'd rather handle authorization errors at the application layer than the model layer. Especially as many models tend to try to work around errors ("let me try to call the tool in a different way.."). Changing tools to do this would also create symmetry with prompts and resources.
So I think throwing an exception is better than a CallToolResult. This allows the client host application to do step up authorization and similar - which isn't really possible if the error is handled at the model layer.
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.
Changing tools to do this would also create symmetry with prompts and resources.
Agreed. I almost did that to begin with, but I went with a CallToolResult since it did have an IsError concept unlike prompt and resource results. However, upon further reflection, I like the consistency of throwing, so the client doesn't have to special case tool authz errors vs resource and prompts.
That wouldn't fix the issue you describe about sniffing out names of primitives that require authorization, but that doesn't seem any worse to me than a typical HTTP endpoint where you could sniff for a 401/403 vs a 404.
I really like the way the filters are done from a developer experience perspective. Easy to use and highly customizable. Only thing is that it's not really limited to filtering - you can freely modify, act, and/or add in the middleware. So I'm not sure the name should be "filter". Developers will figure it out soon enough if it is, but I think there's an argument for picking a name that reflects that this can be used for anything really. But this is a great improvement in terms of convenient handler flexibility. |
The nice thing about options is this can be reconfigured per-session in a This is why the middleware pipelines in ASP.NET Core is typically on some sort of builder or options type rather than registered directly as services. For example, the main middleware pipeline is typically stored in ApplicationBuilder._components, Endpoint filters are stored in EndpointBuilder._filterFactories, SignalR Hub filters are stored in HubOptions.HubFilters, and Kestrel connection middleware is stored in ListenOptions._middleware. |
{ | ||
// Custom call tool logic | ||
return await next(context, 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.
This codeblock invites the question: "Do I need AddCallToolFilter for [Authorize] to work?"
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.
The answer is no, but I can remove it.
I think I'm going to change the behavior, so you have to call AddAuthorizationFiliters()
for authorization to work rather than just doing it implicitly inside of WithHttpTransport()
. However, WithHttpTransport()
will add filters in IPostConfigureOptions
that will throw if the MatchedPrimitive
(or one of the MatchPrimitive
instances in the collection) has any relevant metadata and the authorization filters haven't been added.
The benefit is that it way more explicit which filters run before and after auth
message.RelatedTransport = transport; | ||
if (message.Context is not null) | ||
{ | ||
throw new ArgumentException("Only transports can provide a JsonRpcMessageContext."); |
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.
would be better to not need this. If we could change the types to avoid this check, that would be best. Perhaps making the setter only accessible via an internal cast.
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 agree it'd be nice to leverage the type system to make a runtime ArgumentException impossible, but internal doesn't work because of the multiple projects.
I'm not making things worse than before where someone could have injected an IMcpServer into their handler and then sent a JsonRpcMessage with the RelatedTransport set, but it's more honest to throw an ArgumentException in this case rather pretending this could never happen just because it'd be weird usage.
We could update ITransport.MessageReader to be a ChannelReader<JsonRpcMessageWithContext>
instead of ChannelReader<JsonRpcMessage>
which is something @eiriktsarpalis suggested in the PR that added RelatedTransport in the first place. I'm willing to do that as part of this if @eiriktsarpalis and others still think this would be a good idea. Now's the time for breaking changes, since it's better to do them all at once if we're going to do it at all. (See #723)
/// Tools from both sources will be combined when returning results to clients. | ||
/// </para> | ||
/// </remarks> | ||
public List<Func<Func<RequestContext<ListToolsRequestParams>, CancellationToken, ValueTask<ListToolsResult>>, Func<RequestContext<ListToolsRequestParams>, CancellationToken, ValueTask<ListToolsResult>>>> ListToolsFilters { get; } = new(); |
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.
The amount of generics nesting in this is going to trigger some codebases that have CA rules to prevent it. Would be better to have declarative delegate types that are well named.
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 started with delegate types, but I removed them. In general, I lean against delegate types because it can force unnecessary conversions and reminds me of older pre Func/Action APIs like WaitCallback. I also doubt that the generic nesting is going to trigger CA rules considering you're usually passing in a lambda or a method group to an Add method.
These are extraordinarily long signatures though, so I'll see what it looks like if I add the delegates back and use them in McpServerHandlers as well. The McpServerHandlers change would technically be breaking, but it won't be source breaking in a lot of cases.
MCP Server Handler Filters
For each handler type in the MCP Server, there are corresponding
AddXXXFilter
methods inMcpServerBuilderExtensions.cs
that allow you to add filters to the handler pipeline. The filters are stored inMcpServerOptions.Filters
and applied during server configuration.Available Filter Methods
The following filter methods are available:
AddListResourceTemplatesFilter
- Filter for list resource templates handlersAddListToolsFilter
- Filter for list tools handlersAddCallToolFilter
- Filter for call tool handlersAddListPromptsFilter
- Filter for list prompts handlersAddGetPromptFilter
- Filter for get prompt handlersAddListResourcesFilter
- Filter for list resources handlersAddReadResourceFilter
- Filter for read resource handlersAddCompleteFilter
- Filter for completion handlersAddSubscribeToResourcesFilter
- Filter for resource subscription handlersAddUnsubscribeFromResourcesFilter
- Filter for resource unsubscription handlersAddSetLoggingLevelFilter
- Filter for logging level handlersUsage
Filters are functions that take a handler and return a new handler, allowing you to wrap the original handler with additional functionality:
Filter Execution Order
Execution flow:
filter1 -> filter2 -> filter3 -> baseHandler -> filter3 -> filter2 -> filter1
[Authorize] attribute support
When using the ASP.NET Core integration (
ModelContextProtocol.AspNetCore
), authorization filters are automatically configured byWithHttpTransport()
that support[Authorize]
and[AllowAnonymous]
attributes on MCP server tools, prompts, and resources. Some of the attributes the MCP server automatically respects after this change include:[Authorize]
- Requires authentication for access[Authorize(Roles = "RoleName")]
- Requires specific roles[Authorize(Policy = "PolicyName")]
- Requires specific authorization policies[AllowAnonymous]
- Explicitly allows anonymous access (overrides[Authorize]
)Tool Authorization
Tools can be decorated with authorization attributes to control access:
Class-Level Authorization
You can apply authorization at the class level, which affects all tools in the class:
How Authorization Filters Work
The authorization filters work differently for list operations versus individual operations:
List Operations (ListTools, ListPrompts, ListResources)
For list operations, the filters automatically remove unauthorized items from the results. Users only see tools, prompts, or resources they have permission to access.
Individual Operations (CallTool, GetPrompt, ReadResource)
For individual operations, the filters return authorization errors when access is denied:
CallToolResult
withIsError = true
and an error messageMcpException
with "Access forbidden" messageMcpException
with "Access forbidden" messageSetup Requirements
To use authorization features, you must configure authentication and authorization in your ASP.NET Core application:
Custom Authorization Filters
You can also create custom authorization filters using the filter methods:
RequestContext
Within filters, you have access to:
context.User
- The current user'sClaimsPrincipal
context.Services
- The request's service provider for resolving authorization servicescontext.MatchedPrimitive
- The matched tool/prompt/resource with its metadata including authorization attributes