Skip to content

Conversation

SoldierSacha
Copy link

@SoldierSacha SoldierSacha commented Jun 4, 2025

Motivation and Context

#881

In addition to adding the Client Credentials grant (from the issue linked above), I've also gone on to add the Token Exchange grant.

Reasoning for Token Exchange: Since the client credentials grant is for machine-to-machine authorization, I realized that there are times where the client machine (acting as an MCP Client) might have to make requests on behalf of an end-user to the MCP Server. With that being said, in the current implementation, this did not exist because there was no way to securely identify the end-user.

Now it does through Token Exchange.

How Has This Been Tested?

Added test cases (all pass), and also currently using in my own mcp server and client. Everything is working as intended.

Breaking Changes

None

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

No

@SoldierSacha
Copy link
Author

@SoldierSacha
Copy link
Author

@Kludex @pcarleton

@LucaButBoring
Copy link
Contributor

LucaButBoring commented Aug 5, 2025

Since #1020 was closed, I took a look at the code here, leaving a few comments:

  1. We should avoid duplicating dynamic client registration code and metadata discovery.
  2. Client credentials has client_secret_post and client_secret_basic variants which should both be handled (this PR currently assumes client_secret_post without checking).
  3. The authorization_code, client_credentials, and token_exchange flows should probably all extend a new base class, instead of making ClientCredentialsProvider extend httpx.Auth and making TokenExchangeProvider extend ClientCredentialsProvider.

@SoldierSacha
Copy link
Author

Since #1020 was closed, I took a look at the code here, leaving a few comments:

  1. We should avoid duplicating dynamic client registration code and metadata discovery.
  2. Client credentials has client_secret_post and client_secret_basic variants which should both be handled (this PR currently assumes client_secret_post without checking).
  3. The authorization_code, client_credentials, and token_exchange flows should probably all extend a new base class, instead of making ClientCredentialsProvider extend httpx.Auth and making TokenExchangeProvider extend ClientCredentialsProvider.

Thank you for your comments! In my recent commits, here's what I changed to address your concerns.

  1. I went ahead and implemented a shared BaseOAuthProvider that centralizes OAuth server discovery and dynamic client registration logic. This BaseOAuthProvider provides reusable helpers for all grant types.

  2. I added _apply_client_auth to support both client_secret_post and client_secret_basic when authenticating to the token endpoint.

  3. The token exchange is now refactored to derive from the new base class (BaseOAuthProvider) and default the resource parameter using the server URL.

@felixweinberger felixweinberger requested review from pcarleton and removed request for ochafik September 17, 2025 14:49
@felixweinberger felixweinberger added needs more eyes Needs alignment among maintainers whether this is something we want to add auth Issues and PRs related to Authentication / OAuth pending SEP approval When a PR is attached as an implementation detail to a SEP, we mark it as such for triage. and removed needs more eyes Needs alignment among maintainers whether this is something we want to add labels Sep 17, 2025
Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

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

Hi @SoldierSacha thanks for this contribution! And apologies for the time it took to get back to this.

I checked with @pcarleton and it looks like this change is still pending SEP-1046: modelcontextprotocol/modelcontextprotocol#1047

I'm going to request changes for now as there will still be merge conflict resolution and potentially minor changes needed once that SEP is accepted.

@SoldierSacha
Copy link
Author

SoldierSacha commented Sep 17, 2025

Hi @SoldierSacha thanks for this contribution! And apologies for the time it took to get back to this.

I checked with @pcarleton and it looks like this change is still pending SEP-1046: modelcontextprotocol/modelcontextprotocol#1047

I'm going to request changes for now as there will still be merge conflict resolution and potentially minor changes needed once that SEP is accepted.

Thanks for the update, @felixweinberger!

Understood — I’ll keep an eye on SEP-1046. Once that’s accepted and merged, I’ll rebase again and make any necessary adjustments.

For now, I’ve gone ahead and updated this branch with the latest changes from main to keep it current and reduce future conflicts.

Appreciate the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth Issues and PRs related to Authentication / OAuth pending SEP approval When a PR is attached as an implementation detail to a SEP, we mark it as such for triage.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants