-
Notifications
You must be signed in to change notification settings - Fork 497
[auth] Support new auth metadata endpoint from draft spec #469
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
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.
Besides the build errors, main question is more related to the other PR that might cause conflicts, and whether it will handle session state correctly once there are multiple connections. Thinking this PR should go first since its an update to match the spec vs. a general improvement?
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 @olaservo !
my bad on the failed build 😬 , I added a npm run lint
to make it harder to miss next time.
re: session ID's, this all happens pre-MCP session initialization, so Cliff's PR should be independent.
@pcarleton I tested locally with the asana endpoint and the step where it handles metadata discovery looks good. A server that supports it would be nice to test against. That could be difficult to find at the moment, I realize. Another thing I'm concerned about is the amount of errors now showing up in the test output. Is there a way we can get a handle on this output? Suppress it like this perhaps: beforeEach(() => {
jest.spyOn(console, 'error').mockImplementation(() => {});
});
afterEach(() => {
console.error.mockRestore();
});
test('my negative test', () => {
// Your test code here, which might produce errors
}); Screen.Recording.2025-06-02.at.5.17.47.PM.mov |
Hello! And thank you for this change. I am using this branch to create MCP and auth server implementations that conform to the draft spec. Everything is working great except for one thing. I wanted to provide feedback in case it could be helpful. The current implementation of the auth flow in this branch will register the client and construct the authorization URL using the In my mind, this makes sense because the MCP server would be able to indicate which scopes it needs to perform its duties. Of course, please let me know if my understanding is out of line or my feedback is not helpful. EDIT: From RFC8414 describing the
|
@cliffhall thanks for testing! for an example that supports it, the typescript sdk server does here if you run it with
@thedadams thanks for that. edit: this discussion in springboot highlights another reason for preferring the protected-resource-metadata: the Authorization Server may list scopes that aren't valid for the resource since it may be shared across several resources. spring-projects/spring-security#8514 |
Awesome, thanks! Does it make sense to use the scopes from the protected-resource-metadata for the authorization URL step as well? |
@pcarleton nice one! And much gratitude for the error output suppression in your tests! I made sure my fork of the SDK was up to date and gave your example a try. It stopped at the token exchange step: ![]() ![]() Interestingly, it seems like there may be some kind of looping hiccup because when I first tried to connect to the server, the url bar flashed for 5 seconds or so before settling down, like it was trying a url over and over. When I got the above issue, the console showed a rate limit error. I cleared browser cache for the Inspector, restarted it and the AS/RS, and then I was able to make it through |
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.
Looking good except for the hook warning.
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.
LGTM! 👍
The main change in this PR support for the protected resource metadata endpoint. Screenshots are below for a server with the endpoint and a server without it.
Some smaller changes related:
loading
parameter in the auth debugger that wasn't usefulMotivation and Context
This is to support the new auth step in the draft version of the spec.
How Has This Been Tested?
I added tests, and here are screenshots:

Breaking Changes
Non-breaking, if the endpoint isn't present, we'll fallback to the existing flow.
Types of changes
Checklist
Additional context