sjyangkevin commented on code in PR #54043:
URL: https://github.com/apache/airflow/pull/54043#discussion_r2249074536
##########
airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py:
##########
@@ -240,6 +241,25 @@ def is_authorized_asset_alias(
:param details: optional details about the asset alias
"""
+ @abstractmethod
+ def is_authorized_hitl_detail(
Review Comment:
create this abstract method for verifying HITL specific permission. I am
wondering if we need the `access_entity` here, and trying to understand more
about what is actually an "access_entity". I think only `is_authorized_dag` is
using this as parameter, and use it to refer a sub-entity, such as dag run
(i.e., `RUN`) and task instance (i.e., `TASK_INSTANCE`).
From UI perspective, It looks like "components" shown in the DAG UI such as
XComs, Code, DAG Run, Logs, etc. Trying to figure out what is the definition
and what is it from the point of view of data model or api endpoint resources.
HITL operator probably is also an entity, like Logs, XComs, Code,...
thinking from another perspective, I think it may be more suitable for using
`is_authorized_dag` instead of creating a new `is_authorized_hitl_detail`
method. Then, we can create a new `DagAccessEntity.HITL_DETAIL` and use it in
the following way.
Not sure which method is the right direction.
```python
dependencies=[Depends(requires_access_dag(method="GET",
access_entity=DagAccessEntity.HITL_DETAIL))]
```
Furthermore, I noticed some `PATCH` endpoints, should we adjust the method
from `GET` to `PATCH` when checking for access?
##########
airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py:
##########
@@ -65,6 +65,13 @@ class AssetAliasDetails:
id: str | None = None
+@dataclass
+class HITLDetails:
+ """Represents the details of an HITL operator"""
+
+ ti_id: str | None = None
Review Comment:
Here, I think it is a container for some data being sent to the method that
verify the access. `ti_id` here refers to the HITL task instance id.
##########
airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py:
##########
@@ -98,6 +105,7 @@ class DagAccessEntity(Enum):
AUDIT_LOG = "AUDIT_LOG"
CODE = "CODE"
DEPENDENCIES = "DEPENDENCIES"
+ HITL_DETAIL = "HITL_DETAIL"
Review Comment:
As mentioned above, `HITL_DETAIL` is considered as an access entity.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/hitl.py:
##########
@@ -173,7 +173,7 @@ def _get_hitl_detail(
status.HTTP_409_CONFLICT,
]
),
- dependencies=[Depends(requires_access_dag(method="GET",
access_entity=DagAccessEntity.TASK_INSTANCE))],
+ dependencies=[Depends(requires_access_hitl_detail(method="GET",
access_entity=DagAccessEntity.HITL_DETAIL))],
Review Comment:
The comment applies to all below, probably the right direction is as below?
```python
dependencies=[Depends(requires_access_dag(method="GET",
access_entity=DagAccessEntity.HITL_DETAIL))]
```
--
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]
