-
Notifications
You must be signed in to change notification settings - Fork 536
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
Client implementation of MCP auth #144
Conversation
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.
this looks great! - just a couple of non-blocking questions
const metadata = { | ||
...validMetadata, | ||
response_types_supported: ["code"], | ||
code_challenge_methods_supported: ["plain"], // Does not support 'S256' |
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.
[q] Do we only support S256?
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.
Yes, OAuth 2.1 basically* requires this
throw error; | ||
} | ||
|
||
if (result !== "AUTHORIZED") { |
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.
What's the intended behaviour when we need to redirect? should the caller just catch this error?
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.
my understanding is:
- the provider will initiate the redirect on a 401
- once the user is redirected back we will call
auth()
again but with the authorization code - that will populate the provider's tokens
- and then we init a new transport with that provider
I could be missing something though - is that how we intend to get the authorization code in here?
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.
Yes, that's right. This is a good reminder to add more docs, though—and I also wanted to add a convenience method that makes it easier to finish the auth flow via the SSEClientTransport
class.
|
||
if (req.url === "/token" && req.method === "POST") { | ||
// Handle token refresh request - always fail | ||
res.writeHead(400).end(); |
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.
[q] should this be 401
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.
The spec actually says 400: https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-11#name-error-response
…/justin/client-auth Client implementation of MCP auth
This implements the client side only of the MCP auth draft spec.
Example usage
A real implementation needs to save OAuth token state, code verifiers, etc. to a persistent store, but this sample at least demonstrates the bits that are necessary: