vincbeck commented on code in PR #34317:
URL: https://github.com/apache/airflow/pull/34317#discussion_r1340310444
##########
airflow/api_connexion/endpoints/dag_endpoint.py:
##########
@@ -69,7 +69,7 @@ def get_dag_details(*, dag_id: str) -> APIResponse:
return dag_detail_schema.dump(dag)
[email protected]_access([(permissions.ACTION_CAN_READ,
permissions.RESOURCE_DAG)])
+@requires_authentication
Review Comment:
Glad you asked :) Basically this method returns all the DAGs you have access
to. If you look at line 99, there is `readable_dags =
get_airflow_app().appbuilder.sm.get_permitted_dag_ids(g.user)` which return the
list of DAG ids the user has read access to. Setting
`@security.requires_access_dag("GET")` would check if the user has access to
all DAGs but here this is not what we want to check.
So I figure we could remove the authorization check since if the user has no
access to DAGs, `get_permitted_dag_ids` will return an empty list.
I dont think this is perfect so if you think something is off here, please
call it out :).
While writing this comment I am wondering if
`@security.requires_access_dag("GET")` should mean "does the user have access
to one DAG" instead of "does the user have access to all DAGs". Thoughts? I
dont think checking whether the user has access to all DAGs in Airflow makes
sense.
--
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]
