fix: add IBM user to the session#1300
fix: add IBM user to the session#1300edwinjosechittilappilly merged 3 commits intorelease-saas-0.1from
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the FastAPI authentication dependencies so IBM AMS cookie-authenticated users are persisted in the in-memory SessionManager.users registry, aligning IBM auth behavior with other auth modes that rely on session_manager lookups.
Changes:
- Store IBM-authenticated users in
session_manager.usersduringget_current_user. - Store IBM-authenticated users in
session_manager.usersduringget_optional_user.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/dependencies.py
Outdated
| logger.debug("[IBM Auth] IBM auth mode enabled, getting current user") | ||
| return await _get_ibm_user(request, required=True) | ||
| user = await _get_ibm_user(request, required=True) | ||
| if user and user.user_id not in session_manager.users: |
There was a problem hiding this comment.
In the IBM auth branch, _get_ibm_user(required=True) can still return None when an IBM session cookie is present but the JWT is missing the required sub claim (see _get_ibm_user which logs a warning and returns None). get_current_user would then return None despite being a required-auth dependency, potentially letting requests proceed unauthenticated or causing downstream AttributeErrors. Consider raising HTTPException(401, ...) here when user is falsy (or when user.user_id is missing).
| if user and user.user_id not in session_manager.users: | |
| # `_get_ibm_user` may still return None (for example, when JWT is missing | |
| # a required claim). In required-auth contexts, treat this as unauthenticated. | |
| if not user or not getattr(user, "user_id", None): | |
| logger.warning( | |
| "[IBM Auth] Failed to obtain authenticated user from IBM session; " | |
| "returning HTTP 401" | |
| ) | |
| raise HTTPException(status_code=401, detail="Authentication required") | |
| if user.user_id not in session_manager.users: |
| logger.debug("[IBM Auth] IBM auth mode enabled, getting optional user") | ||
| return await _get_ibm_user(request, required=False) | ||
| user = await _get_ibm_user(request, required=False) | ||
| if user and user.user_id and user.user_id not in session_manager.users: |
There was a problem hiding this comment.
_get_ibm_user constructs a fresh User each request and SessionManager.get_user_opensearch_client notes IBM credentials may rotate per-request. With the current if ... not in session_manager.users guard, the cached session_manager.users[user_id] entry will never be refreshed, so any downstream code that reads users from the session manager can see stale jwt_token/credentials or last_login. Consider updating/replacing the existing entry (or at least refreshing the rotating fields) on every successful IBM-authenticated request.
| if user and user.user_id and user.user_id not in session_manager.users: | |
| if user and user.user_id: | |
| # Always refresh cached IBM user to keep rotating credentials/jwt/last_login current |
mpawlow
left a comment
There was a problem hiding this comment.
Code Review 1
- See PR comments: (a) to (c)
src/dependencies.py
Outdated
| logger.debug("[IBM Auth] IBM auth mode enabled, getting current user") | ||
| return await _get_ibm_user(request, required=True) | ||
| user = await _get_ibm_user(request, required=True) | ||
| if user and user.user_id not in session_manager.users: |
There was a problem hiding this comment.
(b) [Normal] Stale IBM credentials never refreshed in session cache
Problem
- IBM credentials can rotate on every request
- Traefik injects fresh X-IBM-LH-Credentials on each forwarded call.
- The new code only populates session_manager.users when the user_id is not already present
- Once a user is cached, subsequent requests with rotated credentials do not update the stored User object.
- Any component that calls session_manager.get_user(user_id) will receive the original (potentially stale) User instance
- For OpenSearch client construction, IBM mode already bypasses the cache (if IBM_AUTH_ENABLED: return clients.create_user_opensearch_client(jwt_token)) so that specific path is safe.
- But the pattern is fragile and any new call-site that reads credentials from the cached user would silently use stale data.
- Also affects: src/dependencies.py:301–303 (get_optional_user)
Solution
- Always overwrite the entry, regardless of whether it already exists. IBM users have no concept of a "login session" — each request is the authoritative source of truth:
if user and user.user_id:
session_manager.users[user.user_id] = userThere was a problem hiding this comment.
it is done by the FE and the IBM APi key is stored locally in json
src/dependencies.py
Outdated
| logger.debug("[IBM Auth] IBM auth mode enabled, getting current user") | ||
| return await _get_ibm_user(request, required=True) | ||
| user = await _get_ibm_user(request, required=True) | ||
| if user and user.user_id not in session_manager.users: |
There was a problem hiding this comment.
(c) [Minor] Unbounded growth of session_manager.users in IBM multi-user deployments
- This is Minor severity. Please feel free to ignore or optionally implement
Problem
- session_manager.users is an in-memory plain dict with no eviction policy.
- For IBM deployments with many unique users, this dict will grow indefinitely for the lifetime of the process.
- Note: This is a pre-existing structural issue; however, this PR extends the pattern to IBM auth, which is the primary auth mode in multi-tenant IBM deployments where user churn may be higher.
- Also affects src/dependencies.py:301–303
Solution
- Introduce a bounded LRU cache or TTL-based eviction in SessionManager
- e.g., use cachetools.TTLCache or cachetools.LRUCache as a drop-in replacement for the users dict.
Add a truthiness check for user.user_id in get_current_user's IBM auth path before inserting into session_manager.users. This prevents adding users with a None/empty user_id as a dict key, avoiding potential errors or incorrect session map entries when IBM auth is enabled.
mpawlow
left a comment
There was a problem hiding this comment.
Code Review 2
- ✅ LGTM / Approved
This pull request updates the user authentication flow to ensure that users authenticated via IBM AMS cookie are properly tracked in the
session_manager. Now, after a user is authenticated with IBM AMS, the user is added to thesession_manager.usersdictionary if they are not already present.Session management improvements:
get_current_user, the user is added tosession_manager.usersif not already tracked.get_optional_user, the user is added tosession_manager.usersif they have auser_idand are not already present.