potiuk commented on code in PR #66504:
URL: https://github.com/apache/airflow/pull/66504#discussion_r3266058397
##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
@@ -757,8 +757,12 @@ async def _collect_teams_to_check(
if method != "POST":
teams.add(get_existing_team(resource_id) if resource_id else None)
if method in ("POST", "PUT"):
- with suppress(JSONDecodeError):
+ try:
raw = (await request.json()).get("team_name")
+ except JSONDecodeError:
+ # Fail closed: ensure auth callback still runs when team can't be
determined.
+ teams.add(None)
Review Comment:
Yep — went with the stricter "raise 400" approach. In `f0d28ed81c`,
`_collect_teams_to_check` now does:
```python
try:
body = await request.json()
except JSONDecodeError:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Request body is not valid JSON",
)
```
No more `suppress`, no chance of the auth callback running with an empty
set. Note the analogous `with suppress(JSONDecodeError):` in
`requires_access_backfill` is pre-existing and intentionally left alone in this
PR — happy to tighten it in a follow-up if you'd like a consistent treatment
across both helpers.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
airflow-core/tests/unit/api_fastapi/core_api/test_security.py:
##########
@@ -587,6 +587,37 @@ async def
test_requires_access_connection_post_invalid_team_returns_400(
assert exc_info.value.status_code == 400
assert "nonexistent" in exc_info.value.detail
+ @pytest.mark.db_test
+ @pytest.mark.parametrize("method", ["POST", "PUT"])
+ @patch.object(Team, "get_name_if_exists")
+ @patch.object(Connection, "get_team_name")
+ @patch("airflow.api_fastapi.core_api.security.get_auth_manager")
+ async def test_requires_access_connection_body_parse_failure_fails_closed(
+ self, mock_get_auth_manager, mock_get_team_name,
mock_get_name_if_exists, method
+ ):
+ """If the request body cannot be parsed, the auth callback must still
run with team=None."""
+ from json import JSONDecodeError
Review Comment:
Done in `f0d28ed81c` — `from json import JSONDecodeError` is now at module
top (`test_security.py:19`); the inline import inside the test body was removed
when the test was renamed to
`test_requires_access_connection_body_parse_failure_returns_400`.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
--
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]
