-
Notifications
You must be signed in to change notification settings - Fork 880
use client info scope #564
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
62dbbca
to
5a8dd80
Compare
I believe this would be a breaking change – IMO important to highlight as related to auth. See also my comment in the linked inspector PR: modelcontextprotocol/inspector#455 (comment) |
@leblancfg can you explain why it's a breaking change? I guess I don't consider adding a fallback to be one. In addition, I was under the impression that client registration supports scopes on the input and that is somehow validated against the supported scopes and then returned in the client information upon successful registration. |
Thanks for this change. I've gone back and forth on this, and after digging around a bit, I think we should not add this, Scopes come up in the MCP SDK’s in several different places:
In general, omitting There are a few cases I'm considering:
(4) was a motivator for originally automatically threading scopes through. The better fix for this issue was on the server, and if that wasn't available, fixing the scope passing outside of this function would be better, while leaving the default behavior to be omitting scope. (2) seems useful to support to me, which is impossible with this change. A server can have a client that's capable of requesting many scopes, but the default auth flow will give it only the default scopes. If it needs more scopes, it can request them via the scope parameter. Note: re the springboot issue, that was specifically about piping a I'll follow up separately on modelcontextprotocol/inspector#455 I'm going to close this for now, but I'm open to reconsidering if we see that (1) and (4) being very common in practice. |
Caught up with @dr3s, and thought about this a bit more, so want to share the update on that.
(4) is going to be common, and as I quoted it's valid to fail and return A nicer way to get a more specific scope is to get it via the |
Fallback to client information scope when it is provided during client registration.
Motivation and Context
The supported scopes are being passed into DCR but not being used yet in the authorization flow. I believe this is the root cause of modelcontextprotocol/inspector#454
How Has This Been Tested?
Unit test and validated that it fixes the inspector oauth flow when connecting.
Breaking Changes
Types of changes
Checklist
Additional context