vincbeck commented on code in PR #34317:
URL: https://github.com/apache/airflow/pull/34317#discussion_r1357045707
##########
airflow/api_connexion/endpoints/task_instance_endpoint.py:
##########
@@ -61,13 +61,8 @@
T = TypeVar("T")
-@security.requires_access(
- [
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG_RUN),
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
- ],
-)
+@security.requires_access_dag("GET", DagAccessEntity.RUN)
+@security.requires_access_dag("GET", DagAccessEntity.TASK_INSTANCE)
Review Comment:
Strongly agree on this one. I tried to achieve this and as you said, just
declare the action the user is trying to do, then the auth manager is
responsible of checking the permissions needed to perform such action. The new
resource type `TASK` makes and simplify it. I dont like having to check
`DAG_RUN` and `TASK_INSTANCE` at the same time.
> Why don't we simplify it all by
>
> DagAccessEntity.DAG
> DagAccessEntity.TASK
> DagAccessEntity.RUN
> DagAccessEntity.TASK_INSTANCE
My approach is, if no `DagAccessEntity` is defined then it is on the Dag
level. My thinking is `DagAccessEntity` is here to give more information on
which entity/information of a DAG the user is trying to access. If none is
specified, then it is on the DAG itself. You rather being explicit here?
Also, just to confirm. We still need the other values in `DagAccessEntity`
right? Such as `AUDIT_LOG`, `TASK_LOGS`, ...
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
