ENG-2264 Add oauth method that uses async session#7206
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Summary
This PR adds async OAuth verification functions (verify_oauth_client_async and extract_token_and_load_client_async) to support async database sessions, which will be used in fidesplus.
Key Changes
- New
verify_oauth_client_async()function using AsyncSession dependency - New
extract_token_and_load_client_async()function for async token extraction and client loading - Comprehensive test coverage with 16 new async test cases covering various authentication scenarios
Issues Found
Critical Issue: Lazy-Loading in Async Context (Lines 519-524)
The implementation queries non-root clients without eager loading the user relationship:
result = await db.execute(
select(ClientDetail).where(ClientDetail.id == client_id)
)
client = result.scalars().first()When is_token_invalidated() is subsequently called, it attempts to access client.user.password_reset_at. In async SQLAlchemy contexts, lazy-loading is not allowed and will raise a greenlet error. While the exception is caught and logged, this silently bypasses password reset token validation for async-loaded clients - a potential security issue. The sync version using ClientDetail.get() doesn't have this problem because it operates synchronously.
Recommendation: Use eager loading with selectinload(ClientDetail.user) to match sync behavior and ensure password reset validation works correctly in async contexts.
Positive Aspects
- Good test coverage mirroring sync version tests
- Proper use of async/await patterns
- Import organization follows existing conventions
- Root client handling (in-memory client without DB query) is correct
Confidence Score: 2/5
- This PR has a critical security issue with password reset token validation being silently bypassed in async contexts due to lazy-loading of relationships
- The PR introduces a security vulnerability where password reset token validation fails silently in async contexts due to lazy-loaded relationships not being accessible in async SQLAlchemy. While the exception is caught, this bypasses an important security check (password reset invalidation). The code will still function (returning False from is_token_invalidated), but it silently allows tokens to remain valid even after password resets in async code paths. This requires fixing the eager-loading strategy before merge.
- src/fides/api/oauth/utils.py requires attention: the async client loading (lines 519-524) needs eager loading of the user relationship to prevent lazy-loading errors and ensure password reset validation works correctly
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/fides/api/oauth/utils.py | 3/5 | Added two new async functions: verify_oauth_client_async() and extract_token_and_load_client_async(). The implementation mirrors the sync versions but has a critical issue with lazy-loading of the user relationship in async contexts. When non-root clients are queried from the database, the relationship is not eagerly loaded, which will cause a greenlet error when is_token_invalidated() tries to access client.user.password_reset_at. While the exception is caught, this bypasses password reset token validation silently for async clients. |
| tests/lib/test_oauth_util.py | 4/5 | Added comprehensive test coverage for the new async functions with two test classes: TestVerifyOauthClientAsync and TestVerifyOauthClientAsyncRoles. Tests cover malformed tokens, missing/expired issued_at, missing client_id, missing client, wrong scopes, and role-based scenarios. Tests are well-structured and mirror the sync version's test cases appropriately. |
|
@greptile re-review |
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Added async versions of OAuth client verification functions (verify_oauth_client_async and extract_token_and_load_client_async) that use AsyncSession instead of synchronous Session. The implementation properly uses eager loading with selectinload(ClientDetail.user) to avoid lazy-loading issues in async context when checking password reset timestamps.
Key Changes:
- New
verify_oauth_client_async()function mirrors sync version but acceptsAsyncSession - New
extract_token_and_load_client_async()with async database queries and eager loading - Comprehensive test coverage mirroring existing sync tests
- Root client handling optimized to skip DB query for in-memory client
Minor Issues:
- Test assertion patterns inconsistent between sync/async versions (object equality vs ID comparison)
Confidence Score: 4/5
- This PR is safe to merge with minor style inconsistencies
- The async implementation correctly mirrors the sync version with proper eager loading to prevent lazy-loading issues. Test coverage is comprehensive, covering all major scenarios. The only issues found are minor style inconsistencies in test assertions that don't affect functionality.
- No files require special attention - the implementation is solid
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/fides/api/oauth/utils.py | 5/5 | Added async versions of verify_oauth_client and extract_token_and_load_client with proper eager loading for async context |
| tests/lib/test_oauth_util.py | 4/5 | Added comprehensive test coverage for async oauth functions, but has minor inconsistencies in assertion patterns compared to sync version |
| changelog/7206.yaml | 5/5 | Standard changelog entry documenting the addition of async oauth verification |
Ticket ENG-2264
Description Of Changes
Adds a new async method
verify_oauth_client_asyncthat uses an async session instead of a sync one. Will be used in fidesplus.Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works