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")
[email protected]_access(
- [
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG_RUN),
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
- ],
-)
[email protected]_access_dag("GET", DagAccessEntity.RUN)
[email protected]_access_dag("GET", DagAccessEntity.TASK_INSTANCE)
Review Comment:
Looking at the comments/code I looked at 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")
[email protected]_access(
- [
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG_RUN),
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
- ],
-)
[email protected]_access_dag("GET", DagAccessEntity.RUN)
[email protected]_access_dag("GET", DagAccessEntity.TASK_INSTANCE)
Review Comment:
Looking at the comments/code I looked at below I have some doubts and
different idea.
I think the original permissions 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
