Copilot commented on code in PR #64822:
URL: https://github.com/apache/airflow/pull/64822#discussion_r3066473706
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_dags.py:
##########
@@ -235,6 +237,29 @@ def test_should_response_403(self,
unauthorized_test_client):
response = unauthorized_test_client.get("/dags", params={})
assert response.status_code == 403
+ @pytest.mark.parametrize(
+ "denied_entity",
+ [
+ DagAccessEntity.RUN,
+ DagAccessEntity.HITL_DETAIL,
+ DagAccessEntity.TASK_INSTANCE,
+ ],
+ )
+ def test_should_response_403_on_missing_entity_permission(self,
test_client, denied_entity):
+ """Users lacking any sub-entity permission should receive 403 on the
DAG list."""
+ original_is_authorized = SimpleAuthManager.is_authorized_dag
+
+ def restricted_is_authorized_dag(self, *, method, user,
access_entity=None, details=None):
+ if access_entity == denied_entity:
+ return False
+ return original_is_authorized(
Review Comment:
`original_is_authorized` is assigned, but the function returns
`original_is_authorized(...)` (a different name), which will raise `NameError`
and make the test fail. Rename the call to use the variable that was actually
captured (or rename the captured variable to match).
```suggestion
original_is_authorized_dag = SimpleAuthManager.is_authorized_dag
def restricted_is_authorized_dag(self, *, method, user,
access_entity=None, details=None):
if access_entity == denied_entity:
return False
return original_is_authorized_dag(
```
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/dags.py:
##########
@@ -79,6 +79,9 @@
response_model_exclude_none=True,
dependencies=[
Depends(requires_access_dag(method="GET")),
+ Depends(requires_access_dag(method="GET",
access_entity=DagAccessEntity.RUN)),
+ Depends(requires_access_dag(method="GET",
access_entity=DagAccessEntity.HITL_DETAIL)),
+ Depends(requires_access_dag(method="GET",
access_entity=DagAccessEntity.TASK_INSTANCE)),
],
Review Comment:
Repeating `requires_access_dag(method="GET", ...)` three times duplicates
the access-check wiring and makes future additions/removals easy to miss (and
may also add avoidable overhead if each dependency performs non-trivial work).
Consider consolidating into a single dependency that can validate multiple
`DagAccessEntity` values (e.g., accept a list/set or provide a small wrapper
dependency that iterates entities and fails fast).
##########
airflow-core/newsfragments/64822.significant.rst:
##########
@@ -0,0 +1 @@
+Users who only have read access to DAGs will no longer be able to fetch data
from the ``/dags`` endpoint, as it now requires additional permissions
(``DagAccessEntity.RUN``, ``DagAccessEntity.HITL_DETAIL``, and
``DagAccessEntity.TASK_INSTANCE``). This change was made because the endpoint
returns aggregated data from these multiple entities. Please update your custom
user roles to include read access for DAG Runs, Task Instances, and HITL
Details if those users should still have access to the ``/dags`` endpoint.
Review Comment:
This note may be ambiguous because Airflow has multiple “/dags” surfaces (UI
vs API) and this change is specifically in the FastAPI UI route
(`get_dags_ui`). Consider clarifying that this applies to the UI FastAPI
`/dags` endpoint (and/or naming it as shown in the route) to reduce confusion
for operators updating roles.
```suggestion
Users who only have read access to DAGs will no longer be able to fetch data
from the FastAPI UI ``/dags`` endpoint, as it now requires additional
permissions (``DagAccessEntity.RUN``, ``DagAccessEntity.HITL_DETAIL``, and
``DagAccessEntity.TASK_INSTANCE``). This change was made because the endpoint
returns aggregated data from these multiple entities. Please update your custom
user roles to include read access for DAG Runs, Task Instances, and HITL
Details if those users should still have access to the FastAPI UI ``/dags``
endpoint.
```
--
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]
