| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I'm having a similar issue so I am happy to see this PR. However, one concern is that OAuth 2.0 doesnt require the response to contain an expires_in.. In that case, if a 401 is returned but we do have refresh token available, should we try that first rather than doing the whole client flow? https://github.com/modelcontextprotocol/python-sdk/blob/main/src/mcp/client/auth/oauth2.py#L505 |
Sorry, something went wrong.
|
Hey @keurcien — I ran into the same bug against HubSpot's MCP Auth App and opened #2492 which fixes the related oauth_metadata-not-restored side of #1318 (your PR tackles the token_expiry_time side). Your 4-line fix + tests here look clean and the CI is green apart from one flaky Windows run. If you're swamped and want me to rebase it onto current main + ping a maintainer for review, happy to do that. Otherwise I'm also fine waiting for your timeline — both fixes are needed and I don't want to step on your work. Let me know what works best for you. |
Sorry, something went wrong.
Hi @mangibaud33! Yes I don't have much time now so yeah please go ahead :) PS: I also use HubSpot's official MCP server, so I'm looking forward to using your fix (had to subclass the current token storage in my custom implementation). |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Partial fix for #1318.
Motivation and Context
When storing OAuth tokens in a persistent storage, the OAuthClientProvider context would never refresh tokens, because it would consider tokens valid (by is_token_valid()) and proceed with the expired access_token until it meets a 401 response from the MCP server, forcing users to start a new OAuth flow.
The proposed change is small and only solve part of the problem. It's mainly inspired by the discussion from #1318 and the approach taken in the FastMCP library: https://github.com/jlowin/fastmcp/blob/main/src/fastmcp/client/auth/oauth.py
This fix works in the case where token.expires_in returned by the TokenStorage is well calculated (which is not the case if we just simply store the OAuthToken as is, cf. below), and when the token endpoint is the default MCP_SERVER_URL/token.
This PR could also be considered a draft, as it raises other questions about how client_metadata (in case where token endpoint is not obvious) and how or where token.expires_at values should be stored.
How Has This Been Tested?
Here's a sample script to test the proposed change (tested with Notion MCP and Linear MCP). It uses a StoredToken class to hold an extra expires_at value, to return a correct token.expires_in value in get_tokens (cf. PrefectHQ/fastmcp@f73b7b5)
Steps:
Breaking Changes
No breaking changes.
Types of changes
Checklist
Additional context
The added tests complement the existing test_token_validity_check as it assumes the token_expiry_time gets set at initialization.