potiuk commented on code in PR #34317: URL: https://github.com/apache/airflow/pull/34317#discussion_r1355917769
########## 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: Looking at the comments I looked up below I have some doubts and different idea. I think the original permission for task endpoints were just wrong: ```python @security.requires_access( [ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG), (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE), ], ) ``` It really should be: ```python @security.requires_access( [ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG), (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK), ], ) ``` I know we do not have "RESOURCE_TASK" - but this should be really how it should be done inititially. Those endpoints do not access task instance nor dag_run access - they just add `task` definition. I think we wrongly had only one `task_instance` resource where we should have two different resources for those. IMHO - we have the opportunity to fix it (and that will allow us to avoid adding tuple handling from my above proposal. Why don't we simplify it all by * `DagAccessEntity.DAG` * `DagAccessEntity.TASK` * `DagAccessEntity.RUN` * `DagAccessEntity.TASK_INSTANCE` We should only need to specify TASK_INSTANCE entity. In FAB implementation we could map: * DagEntity.TASK => resource.TASK_INSTANCE and * DagEntity.TASK_INSTANCE -> resource.DAG_RUN + resource.TASK_INSTANCE The fact that sometimes we specified "read" for DAG_RUN and "write" to "TASK_INSTANCE" resources was I think a mistake. I can't imagine situation where we would like to limit the user from writing to DAG_RUN where the user should be able to update TASK_INSTANCE. So this would automatically fix the bad permissions we had originally for those endpoints. ########## 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: Looking at the comments/code I looked up below I have some doubts and different idea. I think the original permission for task endpoints were just wrong: ```python @security.requires_access( [ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG), (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE), ], ) ``` It really should be: ```python @security.requires_access( [ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG), (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK), ], ) ``` I know we do not have "RESOURCE_TASK" - but this should be really how it should be done inititially. Those endpoints do not access task instance nor dag_run access - they just add `task` definition. I think we wrongly had only one `task_instance` resource where we should have two different resources for those. IMHO - we have the opportunity to fix it (and that will allow us to avoid adding tuple handling from my above proposal. Why don't we simplify it all by * `DagAccessEntity.DAG` * `DagAccessEntity.TASK` * `DagAccessEntity.RUN` * `DagAccessEntity.TASK_INSTANCE` We should only need to specify TASK_INSTANCE entity. In FAB implementation we could map: * DagEntity.TASK => resource.TASK_INSTANCE and * DagEntity.TASK_INSTANCE -> resource.DAG_RUN + resource.TASK_INSTANCE The fact that sometimes we specified "read" for DAG_RUN and "write" to "TASK_INSTANCE" resources was I think a mistake. I can't imagine situation where we would like to limit the user from writing to DAG_RUN where the user should be able to update TASK_INSTANCE. So this would automatically fix the bad permissions we had originally for those endpoints. -- 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