Refactor AuthManager from app.state to FastAPI dependency #57665
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jason810496
requested review from
bugraoz93,
ephraimbuddy,
pierrejeambrun,
rawwar and
shubhamraj-git
as code owners
- Create new auth_manager.py module in common with AuthManagerDep - Update security.py to use AuthManagerDep instead of request.app.state.auth_manager - Update auth.py routes to use AuthManagerDep - Remove Request parameter where no longer needed - Follow the same dependency injection pattern as DagBag Co-authored-by: jason810496 <68415893+jason810496@users.noreply.github.com> Add unit tests for auth_manager dependency injection - Create test_auth_manager.py to test the new dependency - Verify auth_manager_from_app correctly retrieves from app.state - Test integration with existing test client fixture Co-authored-by: jason810496 <68415893+jason810496@users.noreply.github.com> Fix linting issues with ruff - Move BaseAuthManager import out of TYPE_CHECKING block - Fix import ordering in security.py - Remove unused pytest import from test - Remove trailing whitespace - Format code with ruff format Co-authored-by: jason810496 <68415893+jason810496@users.noreply.github.com> Move auth_manager dependency to security.py module - Move auth_manager_from_app and AuthManagerDep from common/auth_manager.py to core_api/security.py - Update import in routes/public/auth.py to use security module - Move tests from common/test_auth_manager.py to core_api/test_security.py - Delete now-unused common/auth_manager.py and common/test_auth_manager.py - Import BaseAuthManager directly in security.py (not in TYPE_CHECKING) Co-authored-by: jason810496 <68415893+jason810496@users.noreply.github.com>
jason810496
force-pushed
the
feature/AIP-84/refactor-auth-manager-dependency
branch
from
9998a38 to
16b534a
Compare
potiuk
approved these changes
Nov 3, 2025
Member
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
LGTM - but I am not a FastAPI expert (yet :). ) It des make sense though following all what I've learned about dependencies in FastAPI - looks like breaking out with "old/classic ways of doing things in Flask".
Member
|
So - second pair of eyes would be great :) |
pierrejeambrun
added
the
backport-to-v3-1-test
Backport failed to create: v3-1-test. View the failure log Run details
You can attempt to backport this manually by running: cherry_picker 1242e07 v3-1-testThis should apply the commit to the v3-1-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continue |
jason810496
added a commit
to jason810496/airflow
that referenced
this pull request
Nov 4, 2025
- Create new auth_manager.py module in common with AuthManagerDep - Update security.py to use AuthManagerDep instead of request.app.state.auth_manager - Update auth.py routes to use AuthManagerDep - Remove Request parameter where no longer needed - Follow the same dependency injection pattern as DagBag Add unit tests for auth_manager dependency injection - Create test_auth_manager.py to test the new dependency - Verify auth_manager_from_app correctly retrieves from app.state - Test integration with existing test client fixture Fix linting issues with ruff - Move BaseAuthManager import out of TYPE_CHECKING block - Fix import ordering in security.py - Remove unused pytest import from test - Remove trailing whitespace - Format code with ruff format Move auth_manager dependency to security.py module - Move auth_manager_from_app and AuthManagerDep from common/auth_manager.py to core_api/security.py - Update import in routes/public/auth.py to use security module - Move tests from common/test_auth_manager.py to core_api/test_security.py - Delete now-unused common/auth_manager.py and common/test_auth_manager.py - Import BaseAuthManager directly in security.py (not in TYPE_CHECKING) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> (cherry picked from commit 1242e07)
Contributor
|
Should it be backported? It is not a bug fix but more an optimization no? I am not against putting it in 3.1.X but the more we put things in patch releases, the more likely we can also add regressions |
Member
Author
Yeah, it's more like a optimization. I will close the backport PR #57814, we can reopen it again if anyone suggest to backport. |
Member
|
Sounds good to me, updated the milestone to If that happens too often we can reconsider, maybe that will just be marginal. |
Copilot AI
added a commit
to jason810496/airflow
that referenced
this pull request
Dec 5, 2025
- Create new auth_manager.py module in common with AuthManagerDep - Update security.py to use AuthManagerDep instead of request.app.state.auth_manager - Update auth.py routes to use AuthManagerDep - Remove Request parameter where no longer needed - Follow the same dependency injection pattern as DagBag Add unit tests for auth_manager dependency injection - Create test_auth_manager.py to test the new dependency - Verify auth_manager_from_app correctly retrieves from app.state - Test integration with existing test client fixture Fix linting issues with ruff - Move BaseAuthManager import out of TYPE_CHECKING block - Fix import ordering in security.py - Remove unused pytest import from test - Remove trailing whitespace - Format code with ruff format Move auth_manager dependency to security.py module - Move auth_manager_from_app and AuthManagerDep from common/auth_manager.py to core_api/security.py - Update import in routes/public/auth.py to use security module - Move tests from common/test_auth_manager.py to core_api/test_security.py - Delete now-unused common/auth_manager.py and common/test_auth_manager.py - Import BaseAuthManager directly in security.py (not in TYPE_CHECKING) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
related: #50372
related comment: #50372 (comment)
Why
Replace
request.app.state.auth_managerwithAuthManagerDepfor better dependency tracking in Core API.