Skip to content

feature(auth): Allow delegating OAuth authorization to existing app-level implementations #485

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

m-paternostro
Copy link
Contributor

@m-paternostro m-paternostro commented May 13, 2025

An optional provider that clients can use whenever the authentication should be delegated to an existing implementation.

This PR introduces a new optional DelegatedAuthClientProvider interface. It allows clients to delegate authentication to existing systems when they already manage authorization through another mechanism (e.g. platform tokens, ambient credentials, preexisting identity systems). When provided, this provider takes precedence over the standard OAuth flow and gives control to the host application for both header injection and authentication handling.

Motivation and Context

Some applications embedding the MCP SDK already have fully functional authorization systems. In such cases, the SDK’s built-in OAuth flow can be redundant or even problematic - especially when the app simply needs to know when authorization is required, not how to perform it.

Prior to this change, the only way to hook into the authorization process was by subclassing StreamableHTTPClientTransport and/or SSEClientTransport and overriding enough methods to reimplement the auth flow. However, because the relevant methods are private and deeply interwoven, doing so required replicating a significant amount of transport code - leading to maintenance burden and fragile overrides.

This change introduces a clean, focused interface for delegating authentication to external systems without needing to reimplement transport logic. The DelegatedAuthClientProvider is implemented directly in both transport classes and takes precedence over OAuthClientProvider when both are provided.

How Has This Been Tested?

The implementation was validated with comprehensive unit tests covering both StreamableHTTPClientTransport and SSEClientTransport. Tests verify header injection, precedence over OAuth providers, 401 handling with successful reauth, and error handling when authentication fails.

Breaking Changes

No: the new method is purely opt-in, backward-compatible, and safely ignored if unimplemented. It’s designed to be as simple and low-friction as possible while avoiding the need to subclass transports or bypass internal behavior.

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

Notes about the changes:

  • A new DelegatedAuthClientProvider interface is defined in src/client/auth.ts with headers() and authorize() methods.
  • Both StreamableHTTPClientTransport and SSEClientTransport support the optional delegatedAuthProvider option.
  • When provided, delegatedAuthProvider takes precedence over authProvider for all authentication operations.
  • Comprehensive unit tests were added to verify:
    • Header injection from the delegated provider.
    • Precedence over OAuth providers when both are configured.
    • 401 handling with successful and failed reauth scenarios.
    • Context information (serverUrl, resourceMetadataUrl) is passed to the authorize method.
  • The implementation follows existing conventions and coexists seamlessly with the OAuth provider system.

@m-paternostro m-paternostro force-pushed the mp/delegatedauth branch 5 times, most recently from 3352fce to 96f19fc Compare May 15, 2025 13:03
@m-paternostro m-paternostro changed the title feature(auth): OAuthClientProvider.delegateAuthorization feature(auth): Allow delegating OAuth authorization to existing app-level implementations May 15, 2025
@m-paternostro m-paternostro force-pushed the mp/delegatedauth branch 4 times, most recently from b0e2654 to 1bf3a74 Compare May 21, 2025 03:02
@ihrpr ihrpr added this to the HPR milestone May 21, 2025
@m-paternostro m-paternostro force-pushed the mp/delegatedauth branch 4 times, most recently from 7758221 to 02f8659 Compare May 27, 2025 14:06
@m-paternostro m-paternostro force-pushed the mp/delegatedauth branch 2 times, most recently from 59b8e7f to e544126 Compare May 30, 2025 11:54
@m-paternostro
Copy link
Contributor Author

Hi @ihrpr ,

Sorry for the direct tag. I really appreciate that this is already on the HPR list, and I completely understand you have a lot on your plate.

This is just a gentle nudge on the PR - a decision here would really help me move forward on my side, especially if the change is accepted.

Thanks again for all the amazing work on the SDK! It’s been a real pleasure working with it over the past three months ;-)

@m-paternostro m-paternostro force-pushed the mp/delegatedauth branch 2 times, most recently from b8b1a68 to 96d2b31 Compare June 10, 2025 19:29
@m-paternostro
Copy link
Contributor Author

One additional detail worth mentioning: the OAuth implementation used by my company (and others as well 😉) includes JWT tokens in the final authentication response. These tokens encode valuable metadata such as user identity, organization context, and more.

This is yet another reason to allow client implementations to fully control the authentication flow - they may want to extract and act on this information in ways that are specific to their environment.

@m-paternostro m-paternostro force-pushed the mp/delegatedauth branch 7 times, most recently from aa7a7b1 to 2be7d47 Compare June 20, 2025 12:19
@m-paternostro
Copy link
Contributor Author

👋 Hello,

I just wanted to follow up on this PR. I’ve been keeping it up to date over the past month, including adapting to changes like the recent addition of protected resource support (RFC 8707), which this PR now explicitly handles.

I really appreciate that it was marked as an HPR and I took that as a sign that it might be reviewed soon. I also sent a (hopefully gentle) nudge to @ihrpr at the time, just to make sure it was on the radar.

My main reason for commenting now is to ask for a bit of clarity: I’m more than happy to continue monitoring and updating this PR for as long as there’s a reasonable chance it might be reviewed and potentially merged. I sincerely believe it improves the SDK’s OAuth2 support and brings tangible value to the community.

That said, I totally understand if this PR isn’t likely to be accepted either for technical or strategic reasons. However, in this case, I’d rather step back than keep chasing changes unnecessarily.

Regardless, thanks again for all the work you do on this project!

@m-paternostro m-paternostro force-pushed the mp/delegatedauth branch 2 times, most recently from dd35f24 to 9d0539e Compare June 25, 2025 21:46
@pcarleton pcarleton self-assigned this Jun 26, 2025
@pcarleton
Copy link
Contributor

Hi @m-paternostro thanks for providing more context.

Are you able to share a snippet of what your: delegateAuthorization implementation looks like in your client? I believe you that the current API's and available overrides don't suit your needs, but I'm still not getting the specifics.

Also, is this a client targeting a single MCP server? or is it meant to be used for any user provided MCP server?

The overall message I'm getting is "I've already got a function that does the auth flow i need, please let me uses it, and don't make me re-write it / re-test it etc. just to fit the more generic flow that I don't need." Is that right?

Some priorities in my mind are:

  • Compatibility
  • Security
  • API surface maintainability

In general we want MCP clients and servers to be compatible with each other as much as is reasonable. For security, standardized auth flows means its easier to get fixes deployed more quickly if there are fewer implementations of the same critical code. And for API surface maintainability, we want to be able to quickly reason about whether a given change is breaking or not.

Weighing all those together, and if my understanding above is correct (pending the snippet etc.) I'd prefer we delegate at a higher level. That makes this function easier to reason about, and introduces fewer cases where odd interactions might happen. If we add a parameter, or change metadata discovery, I don't want to have to decide whether that comes above or below the delegation call. Similarly, it should be clear it's fully up to the delegating implementer to implement the security best practices, it shouldn't be a mix of responsibility.

In terms of a higher level, this could mean:

  • (simplest, but messy) Move the delegate call above the metadata discovery
    • What I don't love about this is that you've got this OAuthProvider with all these functions required on it, but you're overriding the whole flow that uses it, so you don't actually need any of those functions aside from tokens() to be called in the transport.
  • Add a "401-handler" optional parameter to the transports, so you can intercept there, and have the whole response object available

I'm leaning towards 401 interception as being a more complete solution to this issue, wdyt?

@m-paternostro
Copy link
Contributor Author

m-paternostro commented Jun 26, 2025

thanks for providing more context.

Really my pleasure. Thank you for your time!

Are you able to share a snippet of what your: delegateAuthorization implementation looks like in your client?

I forgot you asked this on your first comment. Sorry.

Here you are:

const authProvider = createDelegatedOAuthClientProvider(
  // tokens
  () => {
    const scope = this.checkScope();

    const accessToken = scope.accessToken();
    if (accessToken) {
      scope.logger.i`injecting access token into the backend transport`;
      return { token_type: 'Bearer', access_token: accessToken };
    }

    scope.logger.d`no access token found to inject into the backend transport`;
    return undefined;
  },

  // delegateAuthorization
  async () => {
    const scope = this.checkScope();

    scope.logger.i`authenticating for the backend transport`;
    const response = await scope
      .tryFetchToRefreshToken()
      .then(() => true)
      .catch((error: unknown) => {
        scope.logger.args(error).e`backend authentication failed: ${error}`;
        return false;
      });
    return response ? 'AUTHORIZED' : undefined;
  },
);

is this a client targeting a single MCP server?

This is being used when targeting MCP Servers that are available on an already authenticated backend.

is it meant to be used for any user provided MCP server?

Not at the moment - for now we only do this for very specific servers.

The overall message I'm getting is... Is that right?

Pretty much ;-)

I would just rephrase the "re-write it" to emphasize the goal of not duplicating it, and ending up with "similar but slightly different" implementations, one for the extension and another for MCP server.

Btw, besides the code duplication, a more pressing issue for us would be to ensure that both flows are always in sync - without going into too many details, our users need to switch between "organizations" and that's imply managing different live OAuth sessions.

Some priorities in my mind are: ...

Nice list.

I'd prefer we delegate at a higher level.
it should be clear it's fully up to the delegating implementer to implement the security best practices, it shouldn't be a mix of responsibility.

All points make total sense.

In terms of a higher level, this could mean:
(simplest, but messy)...

I certainly agree with moving the call to delegateAuthorization to the top.

Some ideas regarding the other concerns...

  • About the metadata computation, perhaps the delegateAuthorization method could take the exact arguments passed to auth (minus the provider). That reinforces the idea of total delegation.

  • About the need to provide an implementation for the required methods in OAuthProvider:

    • If you like the idea that the delegation can "choose" to let the normal flow happen, I'd suggest keeping it as is.
    • If you prefer the approach in which the delegation is a "no return" choice, perhaps the type of the provider ultimately passed to the auth method could be OAuthClientProvider | DelegatedOAuthClientProvider, with the latter exposing just the tokens and the refresh method. This would require changing the transports.

In terms of a higher level, this could mean:
Add a "401-handler"...

Hmmm... Not a bad call although I see 3 potential issues:

  • The access token would still need to be injected. Maybe the handler should be an object with 2 methods (one for the 401 and another for the tokens)

  • The hook would need to be added to every transport (present and future).

  • The code for both Streamable and SSE transports have multiple places checking for 401. The hook would probably need to be invoked in all (or most) of them, which may complicate the overall flow.


Let me know your thoughts. I can update the code based on whatever is decided here.

@pcarleton
Copy link
Contributor

pcarleton commented Jul 1, 2025

The access token would still need to be injected. Maybe the handler should be an object with 2 methods (one for the 401 and another for the tokens)

Good callout, you're right, it'd also need to be able to provide tokens.

The hook would need to be added to every transport (present and future).

I think is fine, it can be the standard way we respond to headers. As in, we'll need to handle unauthorized anyway, so seems fine to do it in a standard "hook" way that's applicable across http-based transports (this is http header handling anyway)

The code for both Streamable and SSE transports have multiple places checking for 401. The hook would probably need to be invoked in all (or most) of them, which may complicate the overall flow.

this is a plus in my mind since it would mean we have consistent handling across both SSE and streamable, and would be harder to miss spots. I think we currently miss at least one spot where we should be setting resource_metadata_url, and the intention is to also handle scope in there, so would be useful to have 1 place to do that and have it apply across the board.

imo the higher level override is the more flexible option to allow full customization for folks that want the underlying jsonrpc handling, but need full control over auth. if you're up for coding that'd up, would be happy to review.

@m-paternostro
Copy link
Contributor Author

if you're up for coding that'd up, would be happy to review.

This all make great sense. I will implement it - I should have the PR ready for review soon (no later than Thursday)

@m-paternostro
Copy link
Contributor Author

👋 Hello,

I've updated to code based on the ideas we've discussed.

I've also rewrote the PR description to reflect the changes: the first paragraph and the 'Additional context' section should be enough to convey what is going on - the rest of the text is more about the "why" ;-)

@pcarleton let me know what you think!

@m-paternostro m-paternostro force-pushed the mp/delegatedauth branch 2 times, most recently from eb43d42 to 48d00de Compare July 4, 2025 01:48
@pcarleton
Copy link
Contributor

hey thanks for this. I'll be away for a week, and will take a closer look once I'm back.

@m-paternostro m-paternostro force-pushed the mp/delegatedauth branch 6 times, most recently from 5c574af to af6fe5c Compare July 16, 2025 02:52
@m-paternostro
Copy link
Contributor Author

👋 Hello @pcarleton,

Hope you had a great week off! We all need to recharge now and then - I’ll actually be off for the next two weeks starting Monday.

If you get a chance to take a look at the changes before Friday and spot anything we should tweak, I’ll do my best to fit it in before I go. Let me know!

@m-paternostro m-paternostro force-pushed the mp/delegatedauth branch 2 times, most recently from 063bbec to 1cab5a6 Compare July 17, 2025 20:14
An optional provider that can be passed to the SSE and StreamableHttp client transports in order to completely delegate the authentication to an external system.
@m-paternostro m-paternostro requested a review from a team as a code owner July 18, 2025 17:02
@m-paternostro m-paternostro requested review from a team, ochafik and ihrpr July 18, 2025 17:02
@pcarleton
Copy link
Contributor

Hey @m-paternostro sorry for the delay here, this is on my list, just didn't get to it this week.

@pcarleton
Copy link
Contributor

hey @m-paternostro thanks for the patience here. I had a long chat with some other folks (@ihrpr , @dsp-ant, @ochafik ) and we're thinking that an even higher level approach would be better here, and I wanted to see if it'd work for you and if you'd be up for adjusting this PR to tackle it.

It'd look something like this:

  // Simple auth wrapper type
  type FetchWrapper = (fetch: FetchLike) => FetchLike;

  // The default implementation
  const withOAuth = (provider: OAuthProvider): FetchWrapper =>
    (fetch) => async (input, init) => {
      const headers = new Headers(init?.headers);
      const tokens = await provider.tokens();
      if (tokens) {
        headers.set('Authorization', `Bearer ${tokens.access_token}`);
      }

      const response = await fetch(input, { ...init, headers });
      if (response.status === 401) {
          const resourceMetadataUrl = extractResourceMetadataUrl(response);

          const result = await auth(provider, { serverUrl: this._url, resourceMetadataUrl, fetchFn: fetch });
          if (result !== "AUTHORIZED") {
            throw new UnauthorizedError();
          }

          const tokens = await provider.tokens();
          if (tokens) {
            headers.set('Authorization', `Bearer ${tokens.access_token}`);
          }

          return await fetch(input, { ...init, headers });
      }

      return response;
    };


  // Adding a custom wrapper:
  const transport = new StreamableHttpClientTransport(url, {
    fetch: withCustomOAuth(oauthProvider)(withLogging(fetch))
  });

The idea is then we could support any type of custom auth there, or other request/response munging.

Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants