-
Notifications
You must be signed in to change notification settings - Fork 721
OAuth state parameter for InspectorOAuthClientProvider via reusable generateOAuthState helper #698
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
OAuth state parameter for InspectorOAuthClientProvider via reusable generateOAuthState helper #698
Conversation
…Provider and in oauth-state-machine, add test
Also added a simple test & renamed a test file that was ending in .ts instead of .test.ts |
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.
@KKonstantinov Could you please add a more detailed description of the changes and their intent?
I can see that it appears you've refactor/extracted the generateOAuthState
function from oauth-state-machine.ts
and put it into oauthUtils.ts
where it is also now unit tested and accessed by auth.ts
in a new state method. But I don't see any calls to that method, so this PR seems like no net change with regard to behavior.
It would be good to understand what the goal is with this and if it is supposed to be fixing a bug (in which case I suspect there's still some code to add) or just a new feature (the state method in auth.ts
that could be called by some future code.
client/src/lib/auth.ts
Outdated
@@ -75,6 +76,10 @@ export class InspectorOAuthClientProvider implements OAuthClientProvider { | |||
return window.location.origin + "/oauth/callback"; | |||
} | |||
|
|||
state(): string | Promise<string> { |
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.
Should this be get state()
? If not, why wedge it between two explicit getters? Who is calling this?
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.
No, it's not a get. The location wasn't picked for any particular reason. Have moved it now.
Re. the rationale of the method. No one from this repository is calling it.
The intent of this addition is for the state parameter to be added to the Authorize URL when the user is redirected to the Authorization Server. Have tested this.
Now to the rationale on how it works to deliver what it's intended to deliver:
- Observe the lines in useConnection.ts, specifically:
inspector/client/src/lib/hooks/useConnection.ts
Lines 318 to 327 in 113b612
const serverAuthProvider = new InspectorOAuthClientProvider(sseUrl); | |
const result = await auth(serverAuthProvider, { | |
serverUrl: sseUrl, | |
scope: oauthScope, | |
}); | |
return result === "AUTHORIZED"; | |
} | |
return false; |
We have implemented an OAuthClientProvider as defined in the @modelcontextprotocol/sdk client.
Then, in useConnection we are calling the auth method exported from the SDK - implementation here. (https://github.com/modelcontextprotocol/typescript-sdk/blob/4d0977bc9169965233120e823c8024e210132ad9/src/client/auth.ts#L33)
The line in the @modelcontextprotocol/sdk client where it specifically checks if our OAuthClientProvider implementation has defined the "state" method and then calls it is:
The outcome is that the state param is included in the authorization URL whenever the OAuthClientProvider has defined such a method.
Hope this clarifies it, let me know if any more detail is needed.
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.
TL;DR it's a feature and it is being called by the @modelcontextprotocol/sdk client right now and not in some future code.
Linking to issue: #682 |
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, looks good to me 👍
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! 👍
Motivation and Context
An extension to #615, which adds the
state
parameter to the newly introduced flow in theInspectorOAuthClientProvider
class.How Has This Been Tested?
Tested via a real MCP server, added unit tests.
Breaking Changes
None
Types of changes
Checklist