python-sdk icon indicating copy to clipboard operation
python-sdk copied to clipboard

OAuth Update: Adding the Client Credentials & Token Exchange Grant Types

Open SoldierSacha opened this issue 8 months ago • 16 comments

Motivation and Context

https://github.com/modelcontextprotocol/python-sdk/issues/881

In addition to implementing the Client Credentials grant (as referenced in the issue linked above), I have also integrated support for the Token Exchange grant.

Reasoning for Token Exchange: While the Client Credentials grant is suitable 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)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)
  • [ ] Documentation update

Checklist

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

Additional context

No

SoldierSacha avatar Jun 04 '25 00:06 SoldierSacha

@simonw @sheffler @comfuture @shimizukawa @ihrpr

SoldierSacha avatar Jun 08 '25 21:06 SoldierSacha

@Kludex @pcarleton

SoldierSacha avatar Jun 10 '25 06:06 SoldierSacha

Went ahead and updated the PR to also support the Token Exchange grant type (in addition to Client Credentials).

This is ready to be merged!! This PR is a huge win for this community.

@dsp-ant @jspahrsummers @calclavia @nick-merrill @jerome3o-anthropic

SoldierSacha avatar Jun 10 '25 23:06 SoldierSacha

Please review this for merge... tired of keeping it in sync with current repo!!!

@Kludex @pcarleton @simonw @sheffler @comfuture @shimizukawa @ihrpr @dsp-ant @jspahrsummers @calclavia @nick-merrill @jerome3o-anthropic

SoldierSacha avatar Jun 13 '25 22:06 SoldierSacha

I have some questions:

  1. First of all, thanks a lot for the contribution and for preparing the MR. From my perspective, this is one of the most important features of MCP Python SDK. It's actually a key reason why a significant number (if not the majority) of companies are still not using MCP. Exposing business data without proper authentication/authorization is simply not an option in most cases.
  2. Is there an ETA for when the review will be done and the code can be merged? This would be critical for our project - we’re eagerly waiting for these changes to be available
  3. Is there a simple example in submitted changes showing how to configure the client credentials flow on the server side (e.g., using popular SSO providers like Azure or Okta)?

patrykks avatar Jun 14 '25 07:06 patrykks

I have some questions:

  1. First of all, thanks a lot for the contribution and for preparing the MR. From my perspective, this is one of the most important features of MCP Python SDK. It's actually a key reason why a significant number (if not the majority) of companies are still not using MCP. Exposing business data without proper authentication/authorization is simply not an option in most cases.
  2. Is there an ETA for when the review will be done and the code can be merged? This would be critical for our project - we’re eagerly waiting for these changes to be available
  3. Is there a simple example in submitted changes showing how to configure the client credentials flow on the server side (e.g., using popular SSO providers like Azure or Okta)?
  1. No problem! I needed these features myself too, so I decided to add them.

  2. If you want to use these features in the meantime while the admins merge this PR, I've made a fork: https://github.com/sacha-development-stuff/mcp-python-sdk/releases/tag/v1.9.4%2Bfork

  3. Regarding examples, everything can be found in /examples.

SoldierSacha avatar Jun 14 '25 23:06 SoldierSacha

@Kludex @ihrpr Any news on when this will be merged? It looks like it solves some big issues with the current sdk.

ChrisBugliosi avatar Jun 17 '25 00:06 ChrisBugliosi

Hello,

I wanted to kindly ask if there has been any progress on this issue/pull request.

I’m noticing the author has had to rebase multiple times, yet the review has still not started. In my view, the change introduced in this PR is the most important and fundamental update in the library. Without it, the SDK is practically unusable for the majority of business use cases (such as AI chatbots). I’m struggling to understand why it appears to be receiving no/low priority. Could someone from the library’s maintainers please take a look? Thank you so much.

patrykks avatar Jun 18 '25 12:06 patrykks

I would also like to see this review happen in a timely fashion. What can we do to help get 👀 on this?

jessesanford avatar Jun 25 '25 19:06 jessesanford

@ihrpr, I see you made some substantial changes to auth (your PR's regarding separating MCP servers into AS + RS using token verifiers & using resource indicators for the new auth spec).

I've gone ahead and synced this fork up to work with the remote branch. Kindly waiting for this PR to be reviewed.

SoldierSacha avatar Jun 27 '25 00:06 SoldierSacha

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.

LucaButBoring avatar Aug 05 '25 17:08 LucaButBoring

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.

SoldierSacha avatar Aug 15 '25 22:08 SoldierSacha

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!

SoldierSacha avatar Sep 17 '25 17:09 SoldierSacha

Converting this to a draft for now as the SEP is still being discussed - once accepted feel free to re-publish for review.

felixweinberger avatar Sep 26 '25 13:09 felixweinberger

Hi @felixweinberger!

I see thathttps://github.com/modelcontextprotocol/modelcontextprotocol/pull/1047 was closed in favor of https://github.com/modelcontextprotocol/ext-auth/pull/3, which was recently accepted. Would you like me to now change this as 'Ready for Review?'

SoldierSacha avatar Oct 18 '25 23:10 SoldierSacha

Hi @felixweinberger!

I see thathttps://github.com/modelcontextprotocol/modelcontextprotocol/pull/1047 was closed in favor of modelcontextprotocol/ext-auth#3. Would you like me to now change this as 'Ready for Review?'

Thanks, I marked it as ready for review.

felixweinberger avatar Oct 21 '25 17:10 felixweinberger

Closing in favor of https://github.com/modelcontextprotocol/python-sdk/pull/1663

felixweinberger avatar Dec 02 '25 15:12 felixweinberger