Copilot commented on code in PR #64288:
URL: https://github.com/apache/airflow/pull/64288#discussion_r3025348528
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/task_instances.py:
##########
@@ -229,6 +233,23 @@ def _categorize_entities(
)
continue
+ if dag_id not in dag_authorization_cache:
+ team_name = DagModel.get_team_name(dag_id,
session=self.session)
+ dag_authorization_cache[dag_id] =
get_auth_manager().is_authorized_dag(
+ method="PUT",
+ access_entity=DagAccessEntity.TASK_INSTANCE,
+ details=DagDetails(id=dag_id, team_name=team_name),
+ user=self.user,
+ )
Review Comment:
`_categorize_entities()` hardcodes `method="PUT"` for the per-DAG
authorization check. This function is also used by `handle_bulk_delete()`, so
delete actions will be authorized using PUT semantics, which can lead to
incorrect allow/deny decisions in auth managers that differentiate methods.
Consider passing the HTTP/resource method (or action type) into
`_categorize_entities()` and using `method="DELETE"` for bulk delete flows.
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_task_instances.py:
##########
@@ -5501,6 +5506,16 @@ class TestBulkTaskInstances(TestTaskInstanceEndpoint):
BASH_DAG_ID = "example_bash_operator"
BASH_TASK_ID = "also_run_this"
WILDCARD_ENDPOINT = "/dags/~/dagRuns/~/taskInstances"
+ RESTRICTED_BUNDLE_NAME = "restricted-bundle"
+ RESTRICTED_TEAM_NAME = "restricted-team"
+
+ @pytest.fixture(autouse=True)
+ def clean_db(self, session):
+ clear_db_runs()
+ clear_db_teams()
+ yield
+ clear_db_teams()
+ clear_db_runs()
Review Comment:
The new `clean_db` fixture clears runs and teams, but this test class also
creates a `DagBundleModel` (`restricted_bundle`) which isn’t cleaned up here.
Since `dag_bundle` rows can persist across tests, this can cause test order
dependence. Consider also clearing dag bundles (e.g. `clear_db_dag_bundles()`)
in this fixture, or avoid persisting a bundle by using existing
fixtures/helpers that create teams/bundles with proper cleanup.
##########
airflow-core/tests/unit/api_fastapi/core_api/services/public/test_task_instances.py:
##########
@@ -393,9 +407,18 @@ def test_categorize_entities(
)
results = BulkActionResponse()
- specific_map_index_task_keys, all_map_index_task_keys =
service._categorize_entities(
- entities, results
- )
+ with (
+ mock.patch.object(DagModel, "get_team_name", return_value="team1"),
+ mock.patch(
+
"airflow.api_fastapi.core_api.services.public.task_instances.get_auth_manager"
+ ) as mock_get_auth_manager,
+ ):
+ auth_manager = mock.Mock(spec=["is_authorized_dag"])
+ auth_manager.is_authorized_dag.return_value = True
+ mock_get_auth_manager.return_value = auth_manager
Review Comment:
The unit test creates `mock.Mock(spec=["is_authorized_dag"])` rather than
using an autospecced mock. In Airflow tests, prefer
`mock.create_autospec(...)`/`patch(..., autospec=True)` so attribute/type
mismatches are caught early and the mock matches the real callable signature.
--
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]
