Auth redesign #172
Auth redesign #172
Conversation
…ger, and ProviderAdapters Implement phases 1-3 of auth redesign plan, introducing a cleaner SDK-first architecture: AuthService (Phase 1): - Single entry point for all authentication flows - Factory methods from_config() and from_provider_config() - Unified route registration and middleware building - FastMCPAuthProvider adapter for FastMCP integration Session & Token Infrastructure (Phase 2): - SessionManager for session lifecycle, OAuth state, and auth codes - TokenStore protocol with SqliteTokenStore implementation - Fernet encryption for provider tokens at rest - One-way hashing for MXCP tokens Provider Adapters (Phase 3): - ProviderAdapter protocol replacing ExternalOAuthHandler - Thin adapters for Google, Keycloak, GitHub, Atlassian, Salesforce - Adapters handle only IdP communication; state managed by SessionManager - Single unified callback route in AuthService Cleanup: - Removed legacy GeneralOAuthAuthorizationServer - Removed legacy providers/ handlers - Removed unused storage operations (auth codes, clients, tokens) - Simplified TokenStore to session operations only BREAKING: Internal auth APIs changed; external behavior preserved.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
bgaidioz
left a comment
bgaidioz
left a comment
There was a problem hiding this comment.
Some comments. I believe bugs to fix.
| self._auth_codes.pop(authorization_code.code, None) | ||
|
|
||
| # Create a session | ||
| session = await self.session_manager.create_session( |
There was a problem hiding this comment.
This is creating a new session instead of looking up the existing one. SessionManager seems to have an implementation to recover a session from the code (consume_auth_code which sits on top of an internal mapping called _auth_codes). Instead FastMCPAuthProvider maintains its own list of auth codes in _auth_codes.
| session = await self.session_manager.create_session( | ||
| client_id=client.client_id, | ||
| scopes=authorization_code.scopes, | ||
| expires_in=3600, # 1 hour default |
There was a problem hiding this comment.
I believe the expiry of the session and all its details shouldn't be changing, it was created during the initial callback. All we can do now is return a token which exposes the session's details (e.g. report an expiry based on now - expiry_at of the matching session).
| # In-memory storage for auth codes and clients | ||
| # (auth codes are short-lived, clients are configured) | ||
| self._auth_codes: dict[str, MXCPAuthCode] = {} | ||
| self._clients: dict[str, MXCPClient] = {} |
There was a problem hiding this comment.
Shouldn't this be protected by a lock?
| session: The session to delete. | ||
| """ | ||
| # Remove from memory cache | ||
| self._sessions.pop(session.mxcp_token_hash, None) |
| ] | ||
|
|
||
| for token_hash in expired_hashes: | ||
| session = self._sessions.pop(token_hash, None) |
There was a problem hiding this comment.
This is duplicating the individual deletion code. I imagine we could factorize the bulk deletion and use it from the "delete_session" and "delete_expired_sessions".
| return None | ||
|
|
||
| # Update last accessed | ||
| session.touch() |
| """SQLite-based token storage with encryption support. | ||
|
|
||
| Provider tokens are encrypted using Fernet symmetric encryption. | ||
| MXCP tokens are stored as SHA-256 hashes for secure lookup. |
There was a problem hiding this comment.
MXCP tokens aren't persisted?
|
|
||
| return Session( | ||
| session_id=row["session_id"], | ||
| mxcp_token="", # Token is not stored, only hash |
There was a problem hiding this comment.
I believe that won't work if we never return the MXCP token itself. Especially in sessions.py, there's that kind of code:
session = await self.token_store.load_session_by_token_hash(token_hash)
...
self._token_to_hash[session.mxcp_token] = token_hashWe use mxcp_token as returned by the storage.
| callback_url = str(params.redirect_uri) if params.redirect_uri else "" | ||
| oauth_state = self.session_manager.create_oauth_state( | ||
| client_id=client.client_id, | ||
| redirect_uri=callback_url, |
There was a problem hiding this comment.
redirect_url and callback_url are set to the same URL. That seems to be the client's URL (the URL to which we'll send the MXCP token?). callback_url should probably be our callback (which providers will call to pass the OAuth code/state).
| def _sync_initialize(self) -> None: | ||
| """Synchronous database initialization.""" | ||
| self.db_path.parent.mkdir(parents=True, exist_ok=True) | ||
| self.conn = sqlite3.connect(str(self.db_path), check_same_thread=False) |
There was a problem hiding this comment.
With max_workers=1, the lock is redundant. Only one task runs at a time in the executor, so the lock doesn’t add safety. If setting max_workers>1 but keeping a single shared connection plus the lock, everything is still serialized by that lock, so we gain no parallelism. check_same_thread=False is only needed if the connection might be used from multiple threads.
So I'd say we could just drop the lock and leave SQLite checking the thread is the same?
| """Load a session by MXCP token hash.""" | ||
| ... | ||
|
|
||
| async def load_session_by_id(self, session_id: str) -> Session | None: |
There was a problem hiding this comment.
I think load_session_by_id isn't needed. And we miss load_session_by_code.
Description
WIP
Type of Change
Testing
uv run pytestuv run ruff check .uv run black --check .uv run mypy .Security Considerations
Breaking Changes
If this is a breaking change, describe what users need to do to migrate:
Additional Notes
Any additional context or screenshots.