Skip to content

[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

Merged
merged 21 commits into from
Jun 4, 2025

Conversation

pcarleton
Copy link
Contributor

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:

  • Upgrade typescript sdk to v0.12.0
  • Auth debugger tweaks
    • Got rid of the loading parameter in the auth debugger that wasn't useful
    • Started saving debugger state to SessionStorage so that the Quick Flow can persist state through redirecting to the Authorization server
      • this also allowed having the Quick Flow continue to completion post-redirect rather than waiting for user interaction
    • Added a new empty version of AuthDebuggerState to avoid having to update many places any time there's a new field
    • Added several fields to AuthDebuggerState to support the new metadata endpoint
    • got rid of defunct "nextStep" field in the Oauth state machine

Motivation 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:
CleanShot 2025-05-29 at 15 29 29@2x

CleanShot 2025-05-29 at 15 30 12@2x

Breaking Changes

Non-breaking, if the endpoint isn't present, we'll fallback to the existing flow.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@pcarleton pcarleton requested review from olaservo and ihrpr May 29, 2025 14:31
Copy link
Member

@olaservo olaservo left a 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?

Copy link
Contributor Author

@pcarleton pcarleton left a 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.

@cliffhall
Copy link
Contributor

cliffhall commented Jun 2, 2025

@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

@cliffhall cliffhall added the waiting on submitter Waiting for the submitter to provide more info label Jun 2, 2025
@thedadams
Copy link

thedadams commented Jun 3, 2025

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 supported_scopes from the oauth-authorization-server metadata. However, my understanding is that the auth server's metadata typically does not contain a complete list of all the supported scopes. In this case, it would be better if the client was registered and the authorization URL was constructed based on the supported_scopes from the oauth-protected-resource metadata served by the MCP server.

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 oauth-authoriation-server metadata:

scopes_supported
      RECOMMENDED.  JSON array containing a list of the OAuth 2.0
      [[RFC6749](https://datatracker.ietf.org/doc/html/rfc6749)] "scope" values that this authorization server supports.
      Servers MAY choose not to advertise some supported scope values
      even when this parameter is used.

@pcarleton
Copy link
Contributor Author

pcarleton commented Jun 3, 2025

@cliffhall thanks for testing! for an example that supports it, the typescript sdk server does here if you run it with --oauth:
https://github.com/modelcontextprotocol/typescript-sdk/blob/main/src/examples/README.md#streamable-http-transport

npx tsx src/examples/server/simpleStreamableHttp.ts --oauth

@thedadams thanks for that. In practice, I expect folks to have the same value for supported scopes in both places, but I agree we should prefer the one in the protected resource metadata if it's available. I've changed it to prefer the scopes from that metadata.

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

@pcarleton pcarleton requested a review from cliffhall June 3, 2025 10:09
@thedadams
Copy link

Awesome, thanks! Does it make sense to use the scopes from the protected-resource-metadata for the authorization URL step as well?

@cliffhall
Copy link
Contributor

cliffhall commented Jun 3, 2025

@cliffhall thanks for testing! for an example that supports it, the typescript sdk server does here if you run it with --oauth: https://github.com/modelcontextprotocol/typescript-sdk/blob/main/src/examples/README.md#streamable-http-transport

npx tsx src/examples/server/simpleStreamableHttp.ts --oauth

@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:

Screenshot 2025-06-03 at 11 43 53 AM Screenshot 2025-06-03 at 11 44 15 AM

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.
Screenshot 2025-06-03 at 11 48 39 AM

I cleared browser cache for the Inspector, restarted it and the AS/RS, and then I was able to make it through
Screenshot 2025-06-03 at 11 50 47 AM

Copy link
Contributor

@cliffhall cliffhall left a 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.

@pcarleton pcarleton added Needs Review authorization Issues related to authentication and removed waiting on submitter Waiting for the submitter to provide more info labels Jun 4, 2025
Copy link
Contributor

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@cliffhall cliffhall merged commit 3772ff3 into main Jun 4, 2025
5 checks passed
@cliffhall cliffhall deleted the pcarleton/auth-debugger-draft branch June 4, 2025 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authorization Issues related to authentication Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants