-
Notifications
You must be signed in to change notification settings - Fork 541
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
Include Authorization Info in Tool Calls (and request handlers generally) #166
base: main
Are you sure you want to change the base?
Include Authorization Info in Tool Calls (and request handlers generally) #166
Conversation
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 all looks good, @allenzhou101. Could you add some unit tests?
Sure, added! |
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 all looks good, builds, and tests run. It seems like a straightforward way to pass the AuthInfo to the individual tool requests. But I couldn't find any discussions or specs about this particular auth-related wrinkle. I would just like one or more of the core team to weigh in on the approach.
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.
Thanks! I think this broadly makes sense and seems useful, but I'd like to tidy up the interface a little bit.
FWIW the motivation behind my comments is basically: we will have to maintain support for whatever API we land upon. Object types are better for this purpose than positional parameters (they can be extended arbitrarily later), and as a bonus, they are also more self-explanatory and clearer in usage.
Please also update from main
and resolve any conflicts.
Co-authored-by: Justin Spahr-Summers <[email protected]>
Co-authored-by: Justin Spahr-Summers <[email protected]>
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 think this still needs a few tweaks with regard to the extra
object.
@cliffhall Ah yes sorry, haven't had the chance to make the changes yet. Will lyk when done. |
Hi, following this PR because I need something like this for the server I'm working on. To understand how we might use this feature once merged, could you clarify these two points:
Thank you! |
This PR simply adds the ability for the |
Oh, I had not seen the bearer middleware. I have aligned with this now, thank you! Eager for this PR to be merged, currently I have to do a weird mapping to identify the user in the tools/dataset. |
Includes the
req.auth
AuthInfo that's set by the MCP Server bearer auth middleware in the server request handler via SSE Transport, allowing for distinguishing of users in requests (eg. tool use).Motivation and Context
When an MCP Server makes a tool call it may need to validate that a particular user has the authorization necessary to access a particular resource. For instance, the MCP Client could give any email it would like to the MCP Server, say to fetch audit logs of a project, but the Server should verify that the user has access to that particular project, which has to happen at the tool call level in the server.
This PR allows the developer to access the auth info not only in the initial
/sse
or/message
route handlers but in the server request handler (eg. tool call) itself.How Has This Been Tested?
Running an authenticated MCP Client session with a Server and validating that the
extra
param appears in the tool call with auth info.This allows auth info to be easily used in a tool call:
Breaking Changes
None.
Types of changes
Checklist
Additional context
This allows for simpler access of auth data than simply including the session id in the transport since you'd have to handle mapping in that case: #158
Fixes #171