potiuk commented on code in PR #71113:
URL: https://github.com/apache/airflow/pull/71113#discussion_r3776400160
##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
@@ -396,6 +396,30 @@ def depends_readable_event_logs_filter(
# does; see the comment there for why any divergence is a cross-Dag
authorization bypass.
_BACKFILL_ID_ADAPTER: TypeAdapter[NonNegativeInt] = TypeAdapter(NonNegativeInt)
+_BACKFILL_NOT_FOUND = "Backfill not found"
+
+
+def _authorize_backfill_in_path(method: ResourceMethod, dag_id: str | None,
user: BaseUser) -> None:
+ """Authorize a backfill named by the request path against that backfill's
Dag alone."""
+ # ``dag_id`` is None when the id matched no row. Answering 404 there while
a backfill on a Dag
+ # the caller may not read answers 403 would tell them which backfill ids
exist across Dags.
+ if dag_id is None:
+ raise HTTPException(status.HTTP_404_NOT_FOUND, _BACKFILL_NOT_FOUND)
Review Comment:
Because this fires before the handler, the 404 *detail* an authorized caller
sees for an unknown id also changed: `/dag_runs` went from `Backfill with id
999 not found` and pause/unpause/cancel from `Could not find backfill with id
999`, both now `Backfill not found`. Only `get_backfill` already used this
wording.
Worth a line in the newsfragment — a client matching on `detail` breaks
silently otherwise. It also leaves the four `raise HTTPException(404, …)`
branches in `backfills.py` reachable only via a TOCTOU delete between this
lookup and the handler's; pointing them at `_BACKFILL_NOT_FOUND` too would
collapse three strings for one condition into one.
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_backfills.py:
##########
@@ -80,6 +82,21 @@ def clean_db():
_clean_db()
[email protected]
+def dag_reader_test_client(test_client):
Review Comment:
This duplicates `unauthorized_test_client`
(`airflow-core/tests/unit/api_fastapi/conftest.py:174`) with `role="viewer"`
instead of `role=None`, token signer and `RevokedToken.is_revoked` patch
included. Three client fixtures now differ only by role — a `role_test_client`
factory in `conftest.py` would fold all three. Fine as a local fixture for this
PR; flagging the pattern.
##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
@@ -396,6 +396,30 @@ def depends_readable_event_logs_filter(
# does; see the comment there for why any divergence is a cross-Dag
authorization bypass.
_BACKFILL_ID_ADAPTER: TypeAdapter[NonNegativeInt] = TypeAdapter(NonNegativeInt)
+_BACKFILL_NOT_FOUND = "Backfill not found"
+
+
+def _authorize_backfill_in_path(method: ResourceMethod, dag_id: str | None,
user: BaseUser) -> None:
+ """Authorize a backfill named by the request path against that backfill's
Dag alone."""
+ # ``dag_id`` is None when the id matched no row. Answering 404 there while
a backfill on a Dag
+ # the caller may not read answers 403 would tell them which backfill ids
exist across Dags.
+ if dag_id is None:
+ raise HTTPException(status.HTTP_404_NOT_FOUND, _BACKFILL_NOT_FOUND)
+
+ details = DagDetails(id=dag_id, team_name=DagModel.get_team_name(dag_id))
+ auth_manager = get_auth_manager()
+ if auth_manager.is_authorized_dag(
+ method=method, access_entity=DagAccessEntity.RUN, details=details,
user=user
+ ):
+ return
+ # A caller who may read the Dag can already list its backfills, so the id
is no secret from
+ # them: hiding it would only cost them the reason their request was
refused.
+ if method != "GET" and auth_manager.is_authorized_dag(
Review Comment:
This is a second auth-manager round-trip on the denied non-GET path.
`batch_is_authorized_dag` covers the "method + GET" pair in one call — that's
what `requires_access_dag_from_file_token` uses. Bounded cost since it's only
the refused path, but a non-caching auth manager pays two backend calls per
refused write.
--
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]
