-
Notifications
You must be signed in to change notification settings - Fork 149
Add: support for oauth 2.1 authentification for Remote MCP Server #13166
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
Conversation
29a6ad4
to
1ecb25c
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.
Found several coding rule violations that need to be addressed.
a7ac312
to
55f5730
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.
Found several coding rule violations that need to be addressed.
55f5730
to
f4be444
Compare
That's a pretty big decision as it will require us to rebase adapt etc... which is toil for future us. How likely do you think you'll be able to merge upstream? Where's the diff of that fork? Needs to be reviewed as well here 👍 |
I agree.
Quite likely.
It's ultra light : dust-tt/typescript-sdk@bca26e4 |
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.
Made a first batch of comments on the oauth flow. I think we can avoid adding a new MCPOAuthExtraConfig
which is really not desirable in oauth/client/setup.ts since the only non string fields used is grant_type_supported
which we know the constraints of as soon as we get it and can error early.
f4be444
to
9f46a7b
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.
LGTM modulo name change of the endpoint. Ideally of course we have a more principled approach to discovering metadata that does not involve throwing an error 👍 But can move forward as is.
Of course feedback here is that the PR is incredibly long. I reckon it's very hard to split but it was really really f**ing hard to review :) |
9f46a7b
to
b646ac2
Compare
aabdbcc
to
10486e1
Compare
Description
Add support for the new Oauth flow when connecting RemoteMCPServer as
workspace
connection.Note:
New code
MCPOAuthProvider
to handle the oauth callback from the MCP client (more about it below)Refactored
PROVIDER_STRATEGIES
to use file per provider (similar to what we do inoauth
).How it works
The official MCP sdk can do most of the flow and requires a class implementing
OAuthClientProvider
(source) with callbacks to retrieve / save the information & tokens.However, we want the flow to be handled by our own oauth service so it works naturally with everything else.
I didn't want to re-implement the discovery of the metadata because it quite intricated with the MCP specificities and I prefer to get any fix they do there (such as this one 2 weeks ago) for "free".
So what I did is that in our implementation of
OAuthClientProvider
:In the front code, I use the new endpoint to check if an mcp server (via it's url) requires oauth and if so, get the said metadata. Then It's similar to other providers except all metadata of the oauth server (tokens endpoint, auth endpoint..) are passed via the connection's metdata.
Tests
A lot of local test with unprotected mcp servers, api token protected mcp server (stripe), multilple oauth protected mcp servers (atlassian, linear, intercom, a nice list is here) as well as checking that gmail & salesforce were still working.
Risk
Medium, with the refactoring of the provider in front, I could have broke something (but I double-checked that the code was exactly the same).
Deploy Plan
Deploy
oauth
andfront