-
Notifications
You must be signed in to change notification settings - Fork 811
Feat: Separate authorization server and resource server on client auth flow #416
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
authorization server url to avoid fetching Protected Resource Metadata every time
add function to discover protected resource metadata
Looking quite good. Thanks for the blog post on this. I'm going to test this out |
Code also looks good |
Thank you! The spec seems to be still under construction, especially the toke validation parts. I am willing to add more if necessary but probably at a different branch would be better? |
@0Itsuki0 I built a mcp server with the latest draft of the spec - maybe we can try the client against this? I've been testing with my postman collection so far |
thank you for the server! I will be happy to try it out and let you know how it goes! |
@0Itsuki0 Let me know if I can help with anything. I'm taking a look at the server aspect of the new auth spec |
@QuantGeekDev |
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.
Thanks for sending this! Apologies for the slow response.
I added a few comments. The most substantial was around including resource
in the authorization url, which isn't in the spec (but might have been in an earlier draft).
other things were:
- some extra log lines
- RFC number fix
- an import path
It looks like it also needs some merge conflict resolution.
I'm attempting to test this with the server supporting the new spec I've implemented in: #503 , but it will require an example client impl. I'll see about stacking a PR with the example on it
Co-authored-by: Paul Carleton <[email protected]>
Co-authored-by: Paul Carleton <[email protected]>
Co-authored-by: Paul Carleton <[email protected]>
Co-authored-by: Paul Carleton <[email protected]>
Co-authored-by: Paul Carleton <[email protected]>
Co-authored-by: Paul Carleton <[email protected]>
Co-authored-by: Paul Carleton <[email protected]>
Co-authored-by: Paul Carleton <[email protected]>
Co-authored-by: Paul Carleton <[email protected]>
Co-authored-by: Paul Carleton <[email protected]>
Co-authored-by: Paul Carleton <[email protected]>
Co-authored-by: Paul Carleton <[email protected]>
Co-authored-by: Paul Carleton <[email protected]>
Co-authored-by: Paul Carleton <[email protected]>
@pcarleton |
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.
thanks for the changes! one more thing
authorizationCode?: string; | ||
scope?: string; | ||
}): Promise<AuthResult> { | ||
const metadata = await discoverOAuthMetadata(serverUrl); |
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.
hey sorry missed this on the first go through, but we'll want to change this to be backwards compatible. Not all servers will support the new metadata endpoint immediately, so we need to try the new metadata path, and if that doesn't bear fruit, fall back to calling discoverOAuthMetadata(serverUrl)
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.
Got it!
Thank you for pointing it out!
const resourceMetadata = await discoverOAuthProtectedResourceMetadata( | ||
resourceMetadataUrl || serverUrl); | ||
|
||
if (resourceMetadata.authorization_servers && resourceMetadata.authorization_servers.length > 0) { |
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.
It feels like we could possibly expose a better interface in this scenario - perhaps a function on the provider that would be called when there is more than one authorization server returned from the protected resource metadata that would pass in the array of servers and utilize the one returned from the function. Happy to PR this in if you feel like it makes sense!
Conform to the new authentication protocol by separating authorization server and resource server.
Motivation and Context
Ensure the sdk functionality conforms to the protocol specification.
How Has This Been Tested?
npm test
Breaking Changes
Yes.
Types of changes
Checklist
Additional context
None.