-
Notifications
You must be signed in to change notification settings - Fork 965
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
base: main
Are you sure you want to change the base?
feature(auth): Allow delegating OAuth authorization to existing app-level implementations #485
Conversation
3352fce
to
96f19fc
Compare
b0e2654
to
1bf3a74
Compare
7758221
to
02f8659
Compare
59b8e7f
to
e544126
Compare
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 ;-) |
b8b1a68
to
96d2b31
Compare
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. |
aa7a7b1
to
2be7d47
Compare
👋 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! |
2be7d47
to
861ea8c
Compare
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.
👋 hi @m-paternostro thanks for the PR, and sorry for the delay.
Would you be able to provide an example of how you'd use this in practice? Some code snippets would be great. I'm having difficulty contextualizing this, e.g.:
- what would you use the metadata for in the function?
- would it be better to delegate the whole
auth
function and not discover any metadata first? e.g. leave it up to the implementor to decide if they need metadata - what parts of the existing flow break down for you?
- is the auth flow you're supporting still OAuth or something else?
If there are specific parts of the OAuthProvider that you can override instead of the whole auth flow, I'd prefer that, or if it makes sense to override at a higher level that's something to consider. The control flow in the auth
function is fairly complex as it is, and this adds another branch to consider that I'd like to do only if absolutely necessary.
Hi @pcarleton, thanks for reaching out and for the thoughtful questions! I will try to answer all of them, sharing context on why this change has been so valuable in our case and potentially in other environments too. Let me know if I missed something or if you’d like to explore alternative designs.
To be honest, my current implementation doesn’t use the metadata. I included it in the patch because it’s readily available at that point where I'm injecting the delegate call into the flow, and I figured others might benefit from it. It could be useful for:
That’s definitely simpler to implement, but I leaned toward a more integrated design to allow delegation with full context. This also helps preserve compatibility with RFC 8707 (resource indicators), future metadata-driven decisions, or scenarios where delegation might only apply for certain resources or servers. That said, I’m 100% okay if there is a preference for a leaner version that calls
The flow itself is solid - the issue is more about not duplicating it and/or existing implementations. In my case, the host app (a VS Code extension, now targeting Cursor) already has:
So instead of trying to reimplement all of that again inside the MCP auth abstraction, it made more sense to delegate - basically, share tokens, trap a 401, do a refresh using our existing code, then try again. I did try to wire our system into the SDK’s OAuth machinery directly, but it was hard to do without forking or patching significant parts our OAuth flow implementation - so, at this moment, our code ships with a modified copy of the Streamable and SSE transports that are aware the proposed
It’s absolutely still OAuth - fully standards-compliant - but orchestrated by the host app (the extension in my case). We use an authorization server, token endpoints, and scopes, and we persist tokens in our own session model (which uses VS Code APIs to write to the machine's keychain). With the proposed change, the SDK just needs to ask for tokens and react to 401s - the rest is handled by our host environment. Also, our tokens embed extra fields (e.g. user/org identifiers) that are JWT encoded - we extract those post-auth for custom logic and, afaik, other companies do something similar. These kinds of extensions are hard to model generically, and honestly, I wouldn’t expect the SDK to try (at least at this moment). And that’s another reason I think having a lean “take control” hook is valuable: it lets people plug in existing solutions.
Indeed! And, honestly, kudos to the team for implementing it. My first idea was to try to inject the hooks into the currently flow but it quickly became a very complicated mess, with way more code that I thought is necessary for this feature, and, even then, would probably not cover all possible corners that everybody out there need. Perhaps naively, I assume that taking control over the flow probably applies to more than just my case, being useful for mobile apps, thin clients, or any environment that already manages sessions and just needs to connect to MCP with minimal overhead. In my case, the extension runs inside Cursor, which, like other environments, doesn’t currently expose any way for extensions to plug into auth-related flows directly. In environments like this, SDK-level delegation isn’t just a convenience - it is arguably the only path that does not require huge rewrites or design compromises.
I tried to keep the implementation minimal and low-risk and this was one the reasons to fork the default flow right at the beginning instead of intermixing hooks all over the place, which would make hard to implement future changes. Also, if That said, if there’s a cleaner or more idiomatic injection point I’ve overlooked, I’d be happy to adapt! I have to confess that, as of SDK 1.13, this was the most self-contained way I found to implement the feature with minimal changes to the code base - that's one of the reasons I didn't go for changes on the transport classes. Thanks again for taking the time to look through this! I am happy to adjust the approach if there’s a simpler way forward. Just hoping we can find a design that supports host-authenticated environments without requiring them to reinvent the entire stack. Let me know what you think! |
861ea8c
to
dd35f24
Compare
An optional method that clients can use whenever the authorization should be delegated to an existing implementation.
dd35f24
to
9d0539e
Compare
An optional method that clients can use whenever the authorization should be delegated to an existing implementation.
This PR introduces a new optional method
delegateAuthorization
to theOAuthClientProvider
interface. It allows clients to short-circuit the standard OAuth flow when they already manage authorization through another mechanism (e.g. platform tokens, ambient credentials, preexisting identity systems). When implemented, this method gives control back to the host application to determine whether it considers the session authorized - if so, the SDK skips its internal flow entirely.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/orSSEClientTransport
and overriding enough methods to reimplement_authThenStart
. However, because the relevant methods are private and deeply interwoven (e.g.send
,_startOrAuthSse
), doing so required replicating a significant amount of transport code - leading to maintenance burden and fragile overrides.This change introduces a clean, focused mechanism for opting into externally-managed auth without needing to reimplement large portions of the transport logic.
How Has This Been Tested?
The design was validated by subclassing SSEClientTransport and making the necessary changes to use this new hook.
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
Checklist
Additional context
Notes about the changes:
auth
function insrc/client/auth.ts
now checks fordelegateAuthorization
(if provided) before entering the standard flow.