potiuk commented on code in PR #70889:
URL: https://github.com/apache/airflow/pull/70889#discussion_r3707316963
##########
airflow-core/tests/unit/api_fastapi/core_api/test_security.py:
##########
@@ -455,6 +458,48 @@ async def
test_requires_access_backfill_backfill_not_found_falls_back_to_body(
user=user,
)
+ @pytest.mark.db_test
+ @pytest.mark.asyncio
+ @pytest.mark.parametrize("backfill_id", ["42", "42.0", "42.00"])
+ @patch.object(DagModel, "get_team_name")
+ @patch("airflow.api_fastapi.core_api.security.get_auth_manager")
+ async def
test_requires_access_backfill_authorizes_the_backfill_the_handler_will_act_on(
+ self, mock_get_auth_manager, mock_get_team_name, backfill_id
+ ):
+ """The dependency must resolve the same backfill the handler does, for
every spelling.
+
+ The endpoints declare ``backfill_id: NonNegativeInt``, and pydantic's
lax mode coerces
+ ``"42.0"`` and ``"42.00"`` to ``42`` -- both are spellings the handler
accepts and serves
+ against backfill 42. Parsing with ``int()`` here rejected them and
left ``dag_id``
+ unresolved, so the two disagreed about which Dag the request concerned.
+ """
+ auth_manager = Mock()
+ auth_manager.is_authorized_dag.return_value = True
+ mock_get_auth_manager.return_value = auth_manager
+ mock_get_team_name.return_value = "team1"
+
+ backfill = Mock()
+ backfill.dag_id = "backfill_dag"
+ session = Mock()
+ session.scalars.return_value.one_or_none.return_value = backfill
+
+ request = Mock()
+ request.path_params = {"backfill_id": backfill_id}
+ request.query_params = {"dag_id": "some_other_dag"}
+ request.json = AsyncMock(return_value={"dag_id": "some_other_dag"})
+
+ user = Mock()
Review Comment:
Done — the mocks in this test now carry `spec=`: `Request`, `Session`,
`Backfill`, `BaseAuthManager` and `BaseUser`. Without it an unspecced `Mock`
accepts any attribute, so the test would keep passing even if the dependency
started reading something the real objects do not have.
Pushed together with a rebase onto `main`.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
Review Comment:
I went and checked whether this works, and it does not — it would
reintroduce the bug in a worse form.
`requires_access_backfill` is not attached only to `/{backfill_id}` routes.
It is also on `GET /backfills`, `POST /backfills` and `POST /backfills/dry_run`
(`backfills.py` lines 87, 285 and 355), where there is no `backfill_id` path
parameter at all and the Dag is taken from the request body instead. If the
dependency declares `backfill_id` as a typed parameter, FastAPI resolves it as
a **query** parameter on exactly those three routes:
- declared bare, it is mandatory, so those three routes 422 on every request;
- declared `NonNegativeInt | None = None`, it becomes an optional query
parameter that appears in the public OpenAPI schema — and lets a caller pass
`?backfill_id=<other>` on `POST /backfills` to steer the authorization decision
at a backfill the handler will never touch.
That second one is the same dependency-versus-handler divergence this PR
exists to close, just entered from the other side.
On the "two independently constructed validators" point: it is the same
`NonNegativeInt` the routes declare, which is what the comment above the
adapter is pointing at — not a second type that happens to agree today. If the
worry is future drift, the tighter fix is a named type alias that both the
routes and this module import, so the declaration has one home. Happy to do
that instead if you would prefer it, but it belongs in the same change as
updating the route signatures, and I would rather keep this 3.3.1 backport to
the bug.
---
Drafted-by: Claude Code (Opus 5); 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]
