o-nikolas commented on code in PR #34317:
URL: https://github.com/apache/airflow/pull/34317#discussion_r1340496770
##########
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:
> I dont think this is perfect so if you think something is off here, please
call it out :).
I don't think I have enough context to call out whether something is off
here or not. But your explanation does make sense!
> @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.
Is this what the decorator is doing for something like `get_dag(...)` above?
In that case we only want one specific dag, but we're checking if the user has
access to all DAGs?
> 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.
I _think_ this makes more sense. And is more of the common case. But surely
there are cases where an action is being done on multiple dags? (but maybe not,
you would know better than I).
--
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]
