Copilot commented on code in PR #64418:
URL: https://github.com/apache/airflow/pull/64418#discussion_r3025335300
##########
providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/routes/login.py:
##########
@@ -71,7 +80,7 @@ def create_token_cli(request: Request, body: dict[str, Any] =
Body(...)) -> Logi
)
def logout(request: Request) -> RedirectResponse:
"""Generate a new API token."""
Review Comment:
`logout()`’s docstring says "Generate a new API token.", but the handler
actually performs a redirect and deletes cookies. Updating the docstring would
avoid confusion when reading the routes or generated docs.
```suggestion
"""Log out the current user by clearing auth cookies and redirecting to
the login page."""
```
##########
providers/fab/tests/unit/fab/auth_manager/api_fastapi/routes/test_login.py:
##########
@@ -54,18 +58,22 @@ def test_create_token_cli(self,
mock_fab_auth_manager_login, test_client):
assert response.status_code == 201
assert response.json()["access_token"] == self.dummy_token.access_token
- def test_logout(self, test_client):
+
@patch("airflow.providers.fab.auth_manager.api_fastapi.routes.login._get_flask_app")
+ def test_logout(self, mock_get_flask_app, test_client):
+
mock_get_flask_app.return_value.app_context.return_value.__enter__.return_value
= None
response = test_client.get("/logout", follow_redirects=False)
assert response.status_code == 307
assert response.headers["location"] == "/auth/login"
cookies = response.headers.get_list("set-cookie")
assert any("session=" in c for c in cookies)
assert any(f"{COOKIE_NAME_JWT_TOKEN}=" in c for c in cookies)
+
@patch("airflow.providers.fab.auth_manager.api_fastapi.routes.login._get_flask_app")
@patch(
"airflow.providers.fab.auth_manager.api_fastapi.routes.login.get_cookie_path",
return_value=SUBPATH
)
- def test_logout_cookie_uses_subpath(self, mock_cookie_path, test_client):
+ def test_logout_cookie_uses_subpath(self, mock_cookie_path,
mock_get_flask_app, test_client):
+
mock_get_flask_app.return_value.app_context.return_value.__enter__.return_value
= None
"""Cookies on logout must be scoped to the configured subpath."""
Review Comment:
In `test_logout_cookie_uses_subpath`, the string literal """Cookies on
logout must be scoped to the configured subpath.""" is no longer a docstring
because it’s preceded by executable statements (mock setup). This becomes a
no-op expression statement and won’t show up in test reports/docs. Move it to
be the first statement in the function (right after `def ...:`) or convert it
to a comment.
```suggestion
"""Cookies on logout must be scoped to the configured subpath."""
mock_get_flask_app.return_value.app_context.return_value.__enter__.return_value
= None
```
##########
providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/routes/login.py:
##########
@@ -37,6 +39,13 @@
get_cookie_path = lambda: "/"
+def _get_flask_app():
+ auth_manager = cast("FabAuthManager", get_auth_manager())
+ if not auth_manager.flask_app:
+ raise RuntimeError("Flask app must be initialized")
+ return auth_manager.flask_app
Review Comment:
`_get_flask_app()` currently `cast`s the result of `get_auth_manager()` to
`FabAuthManager`, which can mask misconfiguration and lead to an
`AttributeError` when accessing `.flask_app` (e.g. when the FAB FastAPI sub-app
is instantiated for OpenAPI generation while a different auth manager is
configured). Consider using the existing `get_fab_auth_manager()` helper (or an
`isinstance` check) and raising a clear exception (ideally an HTTPException
with a 500 and actionable detail) if the auth manager is not `FabAuthManager`
or the Flask app has not been initialized.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
