fix: subclass AccessToken with custom fields - #406
Conversation
|
Looks like this will conflict with #383 when it is merged but can fix when that's in. I'm going to test them together now in a separate branch. |
I'm not convinced that is the right approached to what he's describing but don't use external mode myself so I don't have an environment to properly put it through its paces. |
|
@taylorwilsdon looking into it a bit deeper, you're right. There's an issue explaining the leak here. Looks like the proper fix is to re-use services created with build() as much as possible and dispose of them by calling close(). |
There was a problem hiding this comment.
Pull request overview
This PR fixes an authentication error where SimpleNamespace objects were being used instead of proper AccessToken instances, causing AttributeError: 'types.SimpleNamespace' object has no attribute 'model_dump' when using external OAuth authentication.
Changes:
- Introduced a new
WorkspaceAccessTokenclass that extends FastMCP'sAccessTokenwith workspace-specific fields (session_id, sub, email) - Replaced all
SimpleNamespaceinstantiations withWorkspaceAccessTokento ensure proper type compatibility - Added missing
claimsfield assignments in token creation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| auth/oauth_types.py | Defines the new WorkspaceAccessToken class extending FastMCP's AccessToken |
| auth/external_oauth_provider.py | Replaces SimpleNamespace with WorkspaceAccessToken for token creation |
| auth/auth_info_middleware.py | Replaces SimpleNamespace with WorkspaceAccessToken in two token creation locations and adds claims field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """AccessToken extended with workspace-specific fields.""" | ||
|
|
There was a problem hiding this comment.
The class is missing documentation for its additional fields. Consider adding docstring descriptions for session_id, sub, and email to clarify their purpose and expected values.
| """AccessToken extended with workspace-specific fields.""" | |
| """ | |
| AccessToken extended with workspace-specific fields for workspace identity. | |
| Attributes: | |
| session_id: Optional identifier for the current authenticated session | |
| associated with this access token. May be ``None`` if the token is | |
| not bound to a specific session. | |
| sub: Optional subject identifier for the authenticated user, typically | |
| taken from the identity provider's ``sub`` claim. Intended to be a | |
| stable, opaque user identifier when available. | |
| email: Optional email address associated with the authenticated user, | |
| if provided by the identity provider or upstream authentication | |
| system. | |
| """ |
| claims=getattr(verified_auth, "claims", {}) | ||
| or {}, |
There was a problem hiding this comment.
The or {} fallback is redundant since getattr(verified_auth, 'claims', {}) already provides an empty dict as the default. This creates unnecessary complexity.
| claims=getattr(verified_auth, "claims", {}) | |
| or {}, | |
| claims=getattr(verified_auth, "claims", {}), |
…into fix/external-auth-token
Description
Using external auth, I got this error:
My initial tests worked ok via
mcp-inspectorbut later got this error when sending requests from another app.Type of Change
Testing
Checklist
Additional Notes
Add any other context about the pull request here.
To enable this setting: