From 48f31ac0ceedc8c33781e8ef7177906e087e3786 Mon Sep 17 00:00:00 2001 From: Kevin Yang Date: Fri, 1 Aug 2025 21:39:15 -0400 Subject: [PATCH 1/5] hitl permission draft --- .../auth/managers/base_auth_manager.py | 20 +++++++++++++++++++ .../auth/managers/models/resource_details.py | 8 ++++++++ .../core_api/routes/public/hitl.py | 12 +++++------ .../airflow/api_fastapi/core_api/security.py | 19 ++++++++++++++++++ .../fab/auth_manager/fab_auth_manager.py | 15 ++++++++++++++ 5 files changed, 68 insertions(+), 6 deletions(-) diff --git a/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py b/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py index 8e7ef8573f2dc..c2a2ab0adae19 100644 --- a/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py +++ b/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py @@ -52,6 +52,7 @@ AccessView, AssetAliasDetails, AssetDetails, + HITLDetails, ConfigurationDetails, ConnectionDetails, DagAccessEntity, @@ -240,6 +241,25 @@ def is_authorized_asset_alias( :param details: optional details about the asset alias """ + @abstractmethod + def is_authorized_hitl_detail( + self, + *, + method: ResourceMethod, + user: T, + access_entity: DagAccessEntity | None = None, + details: HITLDetails | None = None, + ) -> bool: + """ + Return whether the user is authorized to perform a given action on an HITL operator. + + :param method: the method to perform + :param user: the user to perform the action + :param access_entity: the kind of DAG information the authorization request is about. + If not provided, the authorization request is about the DAG itself + :param details: optional details about the HITL operator + """ + @abstractmethod def is_authorized_pool( self, diff --git a/airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py b/airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py index c89d599a60ba2..57228fd274d2a 100644 --- a/airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py +++ b/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 + + @dataclass class PoolDetails: """Represents the details of a pool.""" @@ -98,6 +105,7 @@ class DagAccessEntity(Enum): AUDIT_LOG = "AUDIT_LOG" CODE = "CODE" DEPENDENCIES = "DEPENDENCIES" + HITL_DETAIL = "HITL_DETAIL" RUN = "RUN" TASK = "TASK" TASK_INSTANCE = "TASK_INSTANCE" diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/hitl.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/hitl.py index 9b9cdfaf0a9c8..02efbc5347dcb 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/hitl.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/hitl.py @@ -47,7 +47,7 @@ UpdateHITLDetailPayload, ) from airflow.api_fastapi.core_api.openapi.exceptions import create_openapi_http_exception_doc -from airflow.api_fastapi.core_api.security import GetUserDep, ReadableTIFilterDep, requires_access_dag +from airflow.api_fastapi.core_api.security import GetUserDep, ReadableTIFilterDep, requires_access_hitl_detail from airflow.models.hitl import HITLDetail as HITLDetailModel from airflow.models.taskinstance import TaskInstance as TI @@ -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))], ) def update_hitl_detail( dag_id: str, @@ -203,7 +203,7 @@ def update_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))], ) def update_mapped_ti_hitl_detail( dag_id: str, @@ -230,7 +230,7 @@ def update_mapped_ti_hitl_detail( "/{dag_id}/{dag_run_id}/{task_id}", status_code=status.HTTP_200_OK, responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), - 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))], ) def get_hitl_detail( dag_id: str, @@ -252,7 +252,7 @@ def get_hitl_detail( "/{dag_id}/{dag_run_id}/{task_id}/{map_index}", status_code=status.HTTP_200_OK, responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), - 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))], ) def get_mapped_ti_hitl_detail( dag_id: str, @@ -274,7 +274,7 @@ def get_mapped_ti_hitl_detail( @hitl_router.get( "/", status_code=status.HTTP_200_OK, - 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))], ) def get_hitl_details( limit: QueryLimit, diff --git a/airflow-core/src/airflow/api_fastapi/core_api/security.py b/airflow-core/src/airflow/api_fastapi/core_api/security.py index d4f6e11e7e919..bcc0b1e3d1548 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/security.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/security.py @@ -32,6 +32,7 @@ AccessView, AssetAliasDetails, AssetDetails, + HITLDetails, BackfillDetails, ConfigurationDetails, ConnectionDetails, @@ -294,6 +295,24 @@ def inner( return inner +def requires_access_hitl_detail( + method: ResourceMethod, access_entity: DagAccessEntity | None = None +) -> Callable[[Request, BaseUser], None]: + def inner( + request: Request, + user: GetUserDep, + ) -> None: + ti_id_str: str = str(request.path_params.get("task_id")) + + _requires_access( + is_authorized_callback=lambda: get_auth_manager().is_authorized_hitl_detail( + method=method, access_entity=access_entity, details=HITLDetails(ti_id=ti_id_str), user=user + ), + ) + + return inner + + def requires_access_view(access_view: AccessView) -> Callable[[Request, BaseUser], None]: def inner( request: Request, diff --git a/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py b/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py index b560205298a99..50400cc4387f9 100644 --- a/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py +++ b/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py @@ -43,6 +43,7 @@ from airflow.api_fastapi.auth.managers.models.resource_details import ( AccessView, BackfillDetails, + HITLDetails, ConfigurationDetails, ConnectionDetails, DagAccessEntity, @@ -86,6 +87,7 @@ RESOURCE_DAG_VERSION, RESOURCE_DAG_WARNING, RESOURCE_DOCS, + RESOURCE_HITL_DETAIL, RESOURCE_IMPORT_ERROR, RESOURCE_JOB, RESOURCE_PLUGIN, @@ -131,6 +133,7 @@ DagAccessEntity.AUDIT_LOG: (RESOURCE_AUDIT_LOG,), DagAccessEntity.CODE: (RESOURCE_DAG_CODE,), DagAccessEntity.DEPENDENCIES: (RESOURCE_DAG_DEPENDENCIES,), + DagAccessEntity.HITL_DETAIL: (RESOURCE_HITL_DETAIL,), DagAccessEntity.RUN: (RESOURCE_DAG_RUN,), # RESOURCE_TASK_INSTANCE has been originally misused. RESOURCE_TASK_INSTANCE referred to task definition # AND task instances without making the difference @@ -380,6 +383,18 @@ def is_authorized_asset_alias( ) -> bool: return self._is_authorized(method=method, resource_type=RESOURCE_ASSET_ALIAS, user=user) + def is_authorized_hitl_detail( + self, + *, + method: ResourceMethod, + user: User, + details: HITLDetails | None = None, + ) -> bool: + if packaging.version.parse( + packaging.version.parse(airflow_version).base_version + ) >= packaging.version.parse("3.1.0"): + pass + def is_authorized_pool( self, *, method: ResourceMethod, user: User, details: PoolDetails | None = None ) -> bool: From 1a0df75087f77922f81d5ba3e9e58f87f94c6e2a Mon Sep 17 00:00:00 2001 From: Kevin Yang Date: Sat, 2 Aug 2025 00:08:25 -0400 Subject: [PATCH 2/5] add RESOURCE_HITL_DETAIL and make it a DagAccessEntity --- .../auth/managers/base_auth_manager.py | 19 --------- .../auth/managers/models/resource_details.py | 7 ---- .../core_api/routes/public/hitl.py | 12 +++--- .../airflow/api_fastapi/core_api/security.py | 19 --------- .../fab/auth_manager/fab_auth_manager.py | 41 ++++++++++--------- 5 files changed, 28 insertions(+), 70 deletions(-) diff --git a/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py b/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py index c2a2ab0adae19..4bad2153dbd19 100644 --- a/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py +++ b/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py @@ -52,7 +52,6 @@ AccessView, AssetAliasDetails, AssetDetails, - HITLDetails, ConfigurationDetails, ConnectionDetails, DagAccessEntity, @@ -241,24 +240,6 @@ def is_authorized_asset_alias( :param details: optional details about the asset alias """ - @abstractmethod - def is_authorized_hitl_detail( - self, - *, - method: ResourceMethod, - user: T, - access_entity: DagAccessEntity | None = None, - details: HITLDetails | None = None, - ) -> bool: - """ - Return whether the user is authorized to perform a given action on an HITL operator. - - :param method: the method to perform - :param user: the user to perform the action - :param access_entity: the kind of DAG information the authorization request is about. - If not provided, the authorization request is about the DAG itself - :param details: optional details about the HITL operator - """ @abstractmethod def is_authorized_pool( diff --git a/airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py b/airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py index 57228fd274d2a..038f82b30672f 100644 --- a/airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py +++ b/airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py @@ -65,13 +65,6 @@ class AssetAliasDetails: id: str | None = None -@dataclass -class HITLDetails: - """Represents the details of an HITL operator""" - - ti_id: str | None = None - - @dataclass class PoolDetails: """Represents the details of a pool.""" diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/hitl.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/hitl.py index 02efbc5347dcb..63a5f575b2d85 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/hitl.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/hitl.py @@ -47,7 +47,7 @@ UpdateHITLDetailPayload, ) from airflow.api_fastapi.core_api.openapi.exceptions import create_openapi_http_exception_doc -from airflow.api_fastapi.core_api.security import GetUserDep, ReadableTIFilterDep, requires_access_hitl_detail +from airflow.api_fastapi.core_api.security import GetUserDep, ReadableTIFilterDep, requires_access_dag from airflow.models.hitl import HITLDetail as HITLDetailModel from airflow.models.taskinstance import TaskInstance as TI @@ -173,7 +173,7 @@ def _get_hitl_detail( status.HTTP_409_CONFLICT, ] ), - dependencies=[Depends(requires_access_hitl_detail(method="GET", access_entity=DagAccessEntity.HITL_DETAIL))], + dependencies=[Depends(requires_access_dag(method="PUT", access_entity=DagAccessEntity.HITL_DETAIL))], ) def update_hitl_detail( dag_id: str, @@ -203,7 +203,7 @@ def update_hitl_detail( status.HTTP_409_CONFLICT, ] ), - dependencies=[Depends(requires_access_hitl_detail(method="GET", access_entity=DagAccessEntity.HITL_DETAIL))], + dependencies=[Depends(requires_access_dag(method="PUT", access_entity=DagAccessEntity.HITL_DETAIL))], ) def update_mapped_ti_hitl_detail( dag_id: str, @@ -230,7 +230,7 @@ def update_mapped_ti_hitl_detail( "/{dag_id}/{dag_run_id}/{task_id}", status_code=status.HTTP_200_OK, responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), - dependencies=[Depends(requires_access_hitl_detail(method="GET", access_entity=DagAccessEntity.HITL_DETAIL))], + dependencies=[Depends(requires_access_dag(method="GET", access_entity=DagAccessEntity.HITL_DETAIL))], ) def get_hitl_detail( dag_id: str, @@ -252,7 +252,7 @@ def get_hitl_detail( "/{dag_id}/{dag_run_id}/{task_id}/{map_index}", status_code=status.HTTP_200_OK, responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), - dependencies=[Depends(requires_access_hitl_detail(method="GET", access_entity=DagAccessEntity.HITL_DETAIL))], + dependencies=[Depends(requires_access_dag(method="GET", access_entity=DagAccessEntity.HITL_DETAIL))], ) def get_mapped_ti_hitl_detail( dag_id: str, @@ -274,7 +274,7 @@ def get_mapped_ti_hitl_detail( @hitl_router.get( "/", status_code=status.HTTP_200_OK, - dependencies=[Depends(requires_access_hitl_detail(method="GET", access_entity=DagAccessEntity.HITL_DETAIL))], + dependencies=[Depends(requires_access_dag(method="GET", access_entity=DagAccessEntity.HITL_DETAIL))], ) def get_hitl_details( limit: QueryLimit, diff --git a/airflow-core/src/airflow/api_fastapi/core_api/security.py b/airflow-core/src/airflow/api_fastapi/core_api/security.py index bcc0b1e3d1548..d4f6e11e7e919 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/security.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/security.py @@ -32,7 +32,6 @@ AccessView, AssetAliasDetails, AssetDetails, - HITLDetails, BackfillDetails, ConfigurationDetails, ConnectionDetails, @@ -295,24 +294,6 @@ def inner( return inner -def requires_access_hitl_detail( - method: ResourceMethod, access_entity: DagAccessEntity | None = None -) -> Callable[[Request, BaseUser], None]: - def inner( - request: Request, - user: GetUserDep, - ) -> None: - ti_id_str: str = str(request.path_params.get("task_id")) - - _requires_access( - is_authorized_callback=lambda: get_auth_manager().is_authorized_hitl_detail( - method=method, access_entity=access_entity, details=HITLDetails(ti_id=ti_id_str), user=user - ), - ) - - return inner - - def requires_access_view(access_view: AccessView) -> Callable[[Request, BaseUser], None]: def inner( request: Request, diff --git a/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py b/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py index 50400cc4387f9..5fcb3d1732beb 100644 --- a/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py +++ b/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py @@ -43,7 +43,6 @@ from airflow.api_fastapi.auth.managers.models.resource_details import ( AccessView, BackfillDetails, - HITLDetails, ConfigurationDetails, ConnectionDetails, DagAccessEntity, @@ -350,14 +349,30 @@ def is_authorized_dag( method=dag_method, details=details, user=user ): return False - + + # return all( + # ( + # self._is_authorized(method=method, resource_type=resource_type, user=user) + # if resource_type != RESOURCE_DAG_RUN or not hasattr(permissions, "resource_name") + # else self._is_authorized_dag_run(method=method, details=details, user=user) + # ) + # for resource_type in resource_types + # ) + + # if Airflow version is less than 3.1.0 and the resource type is RESOURCE_HITL_DETAIL, skip. return all( ( - self._is_authorized(method=method, resource_type=resource_type, user=user) - if resource_type != RESOURCE_DAG_RUN or not hasattr(permissions, "resource_name") - else self._is_authorized_dag_run(method=method, details=details, user=user) - ) - for resource_type in resource_types + True + if ( + resource_type == RESOURCE_HITL_DETAIL + and packaging.version.parse(packaging.version.parse(airflow_version).base_version) < packaging.version.parse("3.1.0") + ) + else ( + self._is_authorized(method=method, resource_type=resource_type, user=user) + if resource_type != RESOURCE_DAG_RUN or not hasattr(permissions, "resource_name") + else self._is_authorized_dag_run(method=method, details=details, user=user) + ) + ) for resource_type in resource_types ) def is_authorized_backfill( @@ -383,18 +398,6 @@ def is_authorized_asset_alias( ) -> bool: return self._is_authorized(method=method, resource_type=RESOURCE_ASSET_ALIAS, user=user) - def is_authorized_hitl_detail( - self, - *, - method: ResourceMethod, - user: User, - details: HITLDetails | None = None, - ) -> bool: - if packaging.version.parse( - packaging.version.parse(airflow_version).base_version - ) >= packaging.version.parse("3.1.0"): - pass - def is_authorized_pool( self, *, method: ResourceMethod, user: User, details: PoolDetails | None = None ) -> bool: From 2cf61406f661a667e4bce9ee82998fcd025cc4a2 Mon Sep 17 00:00:00 2001 From: Kevin Yang Date: Sat, 2 Aug 2025 22:50:41 -0400 Subject: [PATCH 3/5] configure permissions --- .../auth/managers/base_auth_manager.py | 1 - .../fab/auth_manager/fab_auth_manager.py | 28 +++-------- .../auth_manager/security_manager/override.py | 3 ++ .../fab/auth_manager/test_fab_auth_manager.py | 49 +++++++++++++++++++ .../unit/fab/auth_manager/test_security.py | 2 + 5 files changed, 60 insertions(+), 23 deletions(-) diff --git a/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py b/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py index 4bad2153dbd19..8e7ef8573f2dc 100644 --- a/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py +++ b/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py @@ -240,7 +240,6 @@ def is_authorized_asset_alias( :param details: optional details about the asset alias """ - @abstractmethod def is_authorized_pool( self, diff --git a/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py b/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py index 5fcb3d1732beb..51ba5c743e115 100644 --- a/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py +++ b/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py @@ -349,30 +349,14 @@ def is_authorized_dag( method=dag_method, details=details, user=user ): return False - - # return all( - # ( - # self._is_authorized(method=method, resource_type=resource_type, user=user) - # if resource_type != RESOURCE_DAG_RUN or not hasattr(permissions, "resource_name") - # else self._is_authorized_dag_run(method=method, details=details, user=user) - # ) - # for resource_type in resource_types - # ) - - # if Airflow version is less than 3.1.0 and the resource type is RESOURCE_HITL_DETAIL, skip. + return all( ( - True - if ( - resource_type == RESOURCE_HITL_DETAIL - and packaging.version.parse(packaging.version.parse(airflow_version).base_version) < packaging.version.parse("3.1.0") - ) - else ( - self._is_authorized(method=method, resource_type=resource_type, user=user) - if resource_type != RESOURCE_DAG_RUN or not hasattr(permissions, "resource_name") - else self._is_authorized_dag_run(method=method, details=details, user=user) - ) - ) for resource_type in resource_types + self._is_authorized(method=method, resource_type=resource_type, user=user) + if resource_type != RESOURCE_DAG_RUN or not hasattr(permissions, "resource_name") + else self._is_authorized_dag_run(method=method, details=details, user=user) + ) + for resource_type in resource_types ) def is_authorized_backfill( diff --git a/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py b/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py index 1366f45dee42e..100fdc6b898c9 100644 --- a/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py +++ b/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py @@ -235,6 +235,7 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2): (permissions.ACTION_CAN_READ, permissions.RESOURCE_XCOM), (permissions.ACTION_CAN_READ, permissions.RESOURCE_HITL_DETAIL), (permissions.ACTION_CAN_READ, permissions.RESOURCE_WEBSITE), + (permissions.ACTION_CAN_READ, permissions.RESOURCE_HITL_DETAIL), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_BROWSE_MENU), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DAG), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DAG_DEPENDENCIES), @@ -246,6 +247,7 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2): (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_JOB), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_SLA_MISS), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_TASK_INSTANCE), + (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_HITL_DETAIL), ] # [END security_viewer_perms] @@ -295,6 +297,7 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2): (permissions.ACTION_CAN_CREATE, RESOURCE_BACKFILL), (permissions.ACTION_CAN_EDIT, RESOURCE_BACKFILL), (permissions.ACTION_CAN_DELETE, RESOURCE_BACKFILL), + (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_HITL_DETAIL), ] # [END security_op_perms] diff --git a/providers/fab/tests/unit/fab/auth_manager/test_fab_auth_manager.py b/providers/fab/tests/unit/fab/auth_manager/test_fab_auth_manager.py index 2e18a9327a25d..319b3f0db6082 100644 --- a/providers/fab/tests/unit/fab/auth_manager/test_fab_auth_manager.py +++ b/providers/fab/tests/unit/fab/auth_manager/test_fab_auth_manager.py @@ -65,6 +65,7 @@ RESOURCE_DAG, RESOURCE_DAG_RUN, RESOURCE_DOCS, + RESOURCE_HITL_DETAIL, RESOURCE_JOB, RESOURCE_PLUGIN, RESOURCE_PROVIDER, @@ -443,6 +444,54 @@ def test_is_authorized(self, api_name, method, user_permissions, expected_result [(ACTION_CAN_READ, "DAG:test_dag_id"), (ACTION_CAN_READ, RESOURCE_DAG_RUN)], True, ), + # With global permissions on Dags, but no permission on HITL Detail + ( + "GET", + DagAccessEntity.HITL_DETAIL, + None, + [(ACTION_CAN_READ, RESOURCE_DAG)], + False, + ), + # With global permissions on Dags, but no permission on HITL Detail + ( + "PUT", + DagAccessEntity.HITL_DETAIL, + None, + [(ACTION_CAN_READ, RESOURCE_DAG)], + False, + ), + # With global permissions on Dags, with read permission on HITL Detail + ( + "GET", + DagAccessEntity.HITL_DETAIL, + None, + [(ACTION_CAN_READ, RESOURCE_DAG), (ACTION_CAN_READ, RESOURCE_HITL_DETAIL)], + True, + ), + # With global permissions on Dags, with read permission on HITL Detail, but wrong method + ( + "PUT", + DagAccessEntity.HITL_DETAIL, + None, + [(ACTION_CAN_READ, RESOURCE_DAG), (ACTION_CAN_READ, RESOURCE_HITL_DETAIL)], + False, + ), + # With global permissions on Dags, with write permission on HITL Detail, but wrong method + ( + "GET", + DagAccessEntity.HITL_DETAIL, + None, + [(ACTION_CAN_READ, RESOURCE_DAG), (ACTION_CAN_EDIT, RESOURCE_HITL_DETAIL)], + False, + ), + # With global permissions on Dags, with edit permission on HITL Detail + ( + "PUT", + DagAccessEntity.HITL_DETAIL, + None, + [(ACTION_CAN_READ, RESOURCE_DAG), (ACTION_CAN_EDIT, RESOURCE_HITL_DETAIL)], + True, + ), ], ) @mock.patch.object(FabAuthManager, "get_authorized_dag_ids") diff --git a/providers/fab/tests/unit/fab/auth_manager/test_security.py b/providers/fab/tests/unit/fab/auth_manager/test_security.py index adc94b4b7bddc..19a04ea6839b1 100644 --- a/providers/fab/tests/unit/fab/auth_manager/test_security.py +++ b/providers/fab/tests/unit/fab/auth_manager/test_security.py @@ -454,6 +454,7 @@ def test_get_user_roles_for_anonymous_user(app, security_manager): (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_MY_PASSWORD), (permissions.ACTION_CAN_READ, permissions.RESOURCE_MY_PROFILE), (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_MY_PROFILE), + (permissions.ACTION_CAN_READ, permissions.RESOURCE_HITL_DETAIL), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_BROWSE_MENU), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DAG), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DAG_DEPENDENCIES), @@ -465,6 +466,7 @@ def test_get_user_roles_for_anonymous_user(app, security_manager): (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_TASK_INSTANCE), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DOCS_MENU), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DOCS), + (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_HITL_DETAIL), } app.config["AUTH_ROLE_PUBLIC"] = "Viewer" From aed33cf063fdd25b5f612e12b791f534aeb1fbe3 Mon Sep 17 00:00:00 2001 From: Kevin Yang Date: Mon, 4 Aug 2025 16:54:49 -0400 Subject: [PATCH 4/5] import and register RESOURCE_HITL_DETAIL only after 3.1.0, and remove redundant configuration for permission in override --- .../src/airflow/providers/fab/auth_manager/fab_auth_manager.py | 2 +- .../providers/fab/auth_manager/security_manager/override.py | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py b/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py index 51ba5c743e115..c7f372cdeafd5 100644 --- a/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py +++ b/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py @@ -132,7 +132,6 @@ DagAccessEntity.AUDIT_LOG: (RESOURCE_AUDIT_LOG,), DagAccessEntity.CODE: (RESOURCE_DAG_CODE,), DagAccessEntity.DEPENDENCIES: (RESOURCE_DAG_DEPENDENCIES,), - DagAccessEntity.HITL_DETAIL: (RESOURCE_HITL_DETAIL,), DagAccessEntity.RUN: (RESOURCE_DAG_RUN,), # RESOURCE_TASK_INSTANCE has been originally misused. RESOURCE_TASK_INSTANCE referred to task definition # AND task instances without making the difference @@ -177,6 +176,7 @@ from airflow.providers.fab.www.security.permissions import RESOURCE_HITL_DETAIL _MAP_MENU_ITEM_TO_FAB_RESOURCE_TYPE[MenuItem.REQUIRED_ACTIONS] = RESOURCE_HITL_DETAIL + _MAP_DAG_ACCESS_ENTITY_TO_FAB_RESOURCE_TYPE[DagAccessEntity.HITL_DETAIL] = (RESOURCE_HITL_DETAIL,) class FabAuthManager(BaseAuthManager[User]): diff --git a/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py b/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py index 100fdc6b898c9..1366f45dee42e 100644 --- a/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py +++ b/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py @@ -235,7 +235,6 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2): (permissions.ACTION_CAN_READ, permissions.RESOURCE_XCOM), (permissions.ACTION_CAN_READ, permissions.RESOURCE_HITL_DETAIL), (permissions.ACTION_CAN_READ, permissions.RESOURCE_WEBSITE), - (permissions.ACTION_CAN_READ, permissions.RESOURCE_HITL_DETAIL), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_BROWSE_MENU), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DAG), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DAG_DEPENDENCIES), @@ -247,7 +246,6 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2): (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_JOB), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_SLA_MISS), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_TASK_INSTANCE), - (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_HITL_DETAIL), ] # [END security_viewer_perms] @@ -297,7 +295,6 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2): (permissions.ACTION_CAN_CREATE, RESOURCE_BACKFILL), (permissions.ACTION_CAN_EDIT, RESOURCE_BACKFILL), (permissions.ACTION_CAN_DELETE, RESOURCE_BACKFILL), - (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_HITL_DETAIL), ] # [END security_op_perms] From fffaeea3e7a03f8ac45667580e51fe7f5b88cc4d Mon Sep 17 00:00:00 2001 From: Kevin Yang Date: Mon, 4 Aug 2025 18:25:25 -0400 Subject: [PATCH 5/5] make and fab auth manager import and test file import version compatible regarding RESOURCE_HITL_DETAIL, remove redundant read RESOURCE_HILT_DETAIL from test_security --- .../fab/auth_manager/fab_auth_manager.py | 1 - .../fab/auth_manager/test_fab_auth_manager.py | 135 +++++++++++------- .../unit/fab/auth_manager/test_security.py | 2 - 3 files changed, 86 insertions(+), 52 deletions(-) diff --git a/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py b/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py index c7f372cdeafd5..d17219c699fbd 100644 --- a/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py +++ b/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py @@ -86,7 +86,6 @@ RESOURCE_DAG_VERSION, RESOURCE_DAG_WARNING, RESOURCE_DOCS, - RESOURCE_HITL_DETAIL, RESOURCE_IMPORT_ERROR, RESOURCE_JOB, RESOURCE_PLUGIN, diff --git a/providers/fab/tests/unit/fab/auth_manager/test_fab_auth_manager.py b/providers/fab/tests/unit/fab/auth_manager/test_fab_auth_manager.py index 319b3f0db6082..bdcb0fde48a0a 100644 --- a/providers/fab/tests/unit/fab/auth_manager/test_fab_auth_manager.py +++ b/providers/fab/tests/unit/fab/auth_manager/test_fab_auth_manager.py @@ -65,7 +65,6 @@ RESOURCE_DAG, RESOURCE_DAG_RUN, RESOURCE_DOCS, - RESOURCE_HITL_DETAIL, RESOURCE_JOB, RESOURCE_PLUGIN, RESOURCE_PROVIDER, @@ -75,6 +74,62 @@ RESOURCE_WEBSITE, ) +from tests_common.test_utils.version_compat import AIRFLOW_V_3_1_PLUS + +if AIRFLOW_V_3_1_PLUS: + from airflow.providers.fab.www.security.permissions import RESOURCE_HITL_DETAIL + + HITL_ENDPOINT_TESTS = [ + # With global permissions on Dags, but no permission on HITL Detail + ( + "GET", + DagAccessEntity.HITL_DETAIL, + None, + [(ACTION_CAN_READ, RESOURCE_DAG)], + False, + ), + # With global permissions on Dags, but no permission on HITL Detail + ( + "PUT", + DagAccessEntity.HITL_DETAIL, + None, + [(ACTION_CAN_READ, RESOURCE_DAG)], + False, + ), + # With global permissions on Dags, with read permission on HITL Detail + ( + "GET", + DagAccessEntity.HITL_DETAIL, + None, + [(ACTION_CAN_READ, RESOURCE_DAG), (ACTION_CAN_READ, RESOURCE_HITL_DETAIL)], + True, + ), + # With global permissions on Dags, with read permission on HITL Detail, but wrong method + ( + "PUT", + DagAccessEntity.HITL_DETAIL, + None, + [(ACTION_CAN_READ, RESOURCE_DAG), (ACTION_CAN_READ, RESOURCE_HITL_DETAIL)], + False, + ), + # With global permissions on Dags, with write permission on HITL Detail, but wrong method + ( + "GET", + DagAccessEntity.HITL_DETAIL, + None, + [(ACTION_CAN_READ, RESOURCE_DAG), (ACTION_CAN_EDIT, RESOURCE_HITL_DETAIL)], + False, + ), + # With global permissions on Dags, with edit permission on HITL Detail + ( + "PUT", + DagAccessEntity.HITL_DETAIL, + None, + [(ACTION_CAN_READ, RESOURCE_DAG), (ACTION_CAN_EDIT, RESOURCE_HITL_DETAIL)], + True, + ), + ] + if TYPE_CHECKING: from airflow.api_fastapi.auth.managers.base_auth_manager import ResourceMethod @@ -444,54 +499,6 @@ def test_is_authorized(self, api_name, method, user_permissions, expected_result [(ACTION_CAN_READ, "DAG:test_dag_id"), (ACTION_CAN_READ, RESOURCE_DAG_RUN)], True, ), - # With global permissions on Dags, but no permission on HITL Detail - ( - "GET", - DagAccessEntity.HITL_DETAIL, - None, - [(ACTION_CAN_READ, RESOURCE_DAG)], - False, - ), - # With global permissions on Dags, but no permission on HITL Detail - ( - "PUT", - DagAccessEntity.HITL_DETAIL, - None, - [(ACTION_CAN_READ, RESOURCE_DAG)], - False, - ), - # With global permissions on Dags, with read permission on HITL Detail - ( - "GET", - DagAccessEntity.HITL_DETAIL, - None, - [(ACTION_CAN_READ, RESOURCE_DAG), (ACTION_CAN_READ, RESOURCE_HITL_DETAIL)], - True, - ), - # With global permissions on Dags, with read permission on HITL Detail, but wrong method - ( - "PUT", - DagAccessEntity.HITL_DETAIL, - None, - [(ACTION_CAN_READ, RESOURCE_DAG), (ACTION_CAN_READ, RESOURCE_HITL_DETAIL)], - False, - ), - # With global permissions on Dags, with write permission on HITL Detail, but wrong method - ( - "GET", - DagAccessEntity.HITL_DETAIL, - None, - [(ACTION_CAN_READ, RESOURCE_DAG), (ACTION_CAN_EDIT, RESOURCE_HITL_DETAIL)], - False, - ), - # With global permissions on Dags, with edit permission on HITL Detail - ( - "PUT", - DagAccessEntity.HITL_DETAIL, - None, - [(ACTION_CAN_READ, RESOURCE_DAG), (ACTION_CAN_EDIT, RESOURCE_HITL_DETAIL)], - True, - ), ], ) @mock.patch.object(FabAuthManager, "get_authorized_dag_ids") @@ -517,6 +524,36 @@ def test_is_authorized_dag( ) assert result == expected_result + @pytest.mark.skipif( + AIRFLOW_V_3_1_PLUS is not True, reason="HITL test will be skipped if Airflow version < 3.1.0" + ) + @pytest.mark.parametrize( + "method, dag_access_entity, dag_details, user_permissions, expected_result", + HITL_ENDPOINT_TESTS if AIRFLOW_V_3_1_PLUS else [], + ) + @mock.patch.object(FabAuthManager, "get_authorized_dag_ids") + def test_is_authorized_dag_hitl_detail( + self, + mock_get_authorized_dag_ids, + method, + dag_access_entity, + dag_details, + user_permissions, + expected_result, + auth_manager_with_appbuilder, + ): + dag_permissions = [perm[1] for perm in user_permissions if perm[1].startswith("DAG:")] + dag_ids = {perm.replace("DAG:", "") for perm in dag_permissions} + mock_get_authorized_dag_ids.return_value = dag_ids + + user = Mock() + user.perms = user_permissions + user.id = 1 + result = auth_manager_with_appbuilder.is_authorized_dag( + method=method, access_entity=dag_access_entity, details=dag_details, user=user + ) + assert result == expected_result + @pytest.mark.parametrize( "access_view, user_permissions, expected_result", [ diff --git a/providers/fab/tests/unit/fab/auth_manager/test_security.py b/providers/fab/tests/unit/fab/auth_manager/test_security.py index 19a04ea6839b1..adc94b4b7bddc 100644 --- a/providers/fab/tests/unit/fab/auth_manager/test_security.py +++ b/providers/fab/tests/unit/fab/auth_manager/test_security.py @@ -454,7 +454,6 @@ def test_get_user_roles_for_anonymous_user(app, security_manager): (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_MY_PASSWORD), (permissions.ACTION_CAN_READ, permissions.RESOURCE_MY_PROFILE), (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_MY_PROFILE), - (permissions.ACTION_CAN_READ, permissions.RESOURCE_HITL_DETAIL), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_BROWSE_MENU), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DAG), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DAG_DEPENDENCIES), @@ -466,7 +465,6 @@ def test_get_user_roles_for_anonymous_user(app, security_manager): (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_TASK_INSTANCE), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DOCS_MENU), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DOCS), - (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_HITL_DETAIL), } app.config["AUTH_ROLE_PUBLIC"] = "Viewer"