You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by pi...@apache.org on 2023/03/06 18:29:25 UTC
[airflow] branch v2-5-test updated: Fix inconsitencies in checking edit permissions for a DAG (#20346)
This is an automated email from the ASF dual-hosted git repository.
pierrejeambrun pushed a commit to branch v2-5-test
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/v2-5-test by this push:
new 5c0aabb290 Fix inconsitencies in checking edit permissions for a DAG (#20346)
5c0aabb290 is described below
commit 5c0aabb29089132484cdb3cde20d8c2a87c55545
Author: Kaxil Naik <ka...@apache.org>
AuthorDate: Sat Jan 14 21:21:06 2023 +0000
Fix inconsitencies in checking edit permissions for a DAG (#20346)
We were short-circuting permission in the views instead of letting the security manager handle that. A user will find it inconsistent as the Graph and other views check "per-dag" permissions via https://github.com/apache/airflow/blob/174681911f96f17d41a4f560ca08d5e200944f7f/airflow/www/views.py#L579
so if someone uses Custom Security Manager that will end up with user not being able to "pause" DAG from individual dag page but would be able to do so from Homepage. This PR fixes this inconsistency and gives back this responsibility of permissions to security manager instead if Views.
(cherry picked from commit 87ea8acd0c8e7339249bec51908b35e5e3d70129)
---
airflow/www/security.py | 16 +++++++--
airflow/www/views.py | 17 ++-------
tests/www/test_security.py | 71 +++++++++++++++++++-------------------
tests/www/views/test_views_home.py | 36 +++++++++++++++++++
4 files changed, 88 insertions(+), 52 deletions(-)
diff --git a/airflow/www/security.py b/airflow/www/security.py
index ce75970b14..1237aa7862 100644
--- a/airflow/www/security.py
+++ b/airflow/www/security.py
@@ -322,6 +322,10 @@ class AirflowSecurityManager(SecurityManager, LoggingMixin):
if user.is_anonymous:
roles = user.roles
else:
+ if (permissions.ACTION_CAN_EDIT in user_actions and self.can_edit_all_dags(user)) or (
+ permissions.ACTION_CAN_READ in user_actions and self.can_read_all_dags(user)
+ ):
+ return {dag.dag_id for dag in session.query(DagModel.dag_id)}
user_query = (
session.query(User)
.options(
@@ -437,10 +441,18 @@ class AirflowSecurityManager(SecurityManager, LoggingMixin):
user = g.user
return (
self._has_role(["Admin", "Viewer", "Op", "User"], user)
- or self.has_access(permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG, user)
- or self.has_access(permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG, user)
+ or self.can_read_all_dags(user)
+ or self.can_edit_all_dags(user)
)
+ def can_edit_all_dags(self, user=None):
+ """Has can_edit action on DAG resource"""
+ return self.has_access(permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG, user)
+
+ def can_read_all_dags(self, user=None):
+ """Has can_read action on DAG resource"""
+ return self.has_access(permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG, user)
+
def clean_perms(self):
"""FAB leaves faulty permissions that need to be cleaned up"""
self.log.debug("Cleaning faulty perms")
diff --git a/airflow/www/views.py b/airflow/www/views.py
index 14a57b5b4f..7e8abd02a4 100644
--- a/airflow/www/views.py
+++ b/airflow/www/views.py
@@ -700,17 +700,11 @@ class Airflow(AirflowBaseView):
dags = current_dags.options(joinedload(DagModel.tags)).offset(start).limit(dags_per_page).all()
user_permissions = g.user.perms
- all_dags_editable = (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG) in user_permissions
can_create_dag_run = (
permissions.ACTION_CAN_CREATE,
permissions.RESOURCE_DAG_RUN,
) in user_permissions
- all_dags_deletable = (
- permissions.ACTION_CAN_DELETE,
- permissions.RESOURCE_DAG,
- ) in user_permissions
-
dataset_triggered_dag_ids = {dag.dag_id for dag in dags if dag.schedule_interval == "Dataset"}
if dataset_triggered_dag_ids:
dataset_triggered_next_run_info = get_dataset_triggered_next_run_info(
@@ -720,16 +714,9 @@ class Airflow(AirflowBaseView):
dataset_triggered_next_run_info = {}
for dag in dags:
- dag_resource_name = permissions.RESOURCE_DAG_PREFIX + dag.dag_id
- if all_dags_editable:
- dag.can_edit = True
- else:
- dag.can_edit = (permissions.ACTION_CAN_EDIT, dag_resource_name) in user_permissions
+ dag.can_edit = get_airflow_app().appbuilder.sm.can_edit_dag(dag.dag_id, g.user)
dag.can_trigger = dag.can_edit and can_create_dag_run
- if all_dags_deletable:
- dag.can_delete = True
- else:
- dag.can_delete = (permissions.ACTION_CAN_DELETE, dag_resource_name) in user_permissions
+ dag.can_delete = get_airflow_app().appbuilder.sm.can_delete_dag(dag.dag_id, g.user)
dagtags = session.query(func.distinct(DagTag.name)).all()
tags = [
diff --git a/tests/www/test_security.py b/tests/www/test_security.py
index 05d41cf291..b34842a92d 100644
--- a/tests/www/test_security.py
+++ b/tests/www/test_security.py
@@ -453,25 +453,26 @@ def test_get_accessible_dag_ids(app, security_manager, session):
dag_id = "dag_id"
username = "ElUser"
- with create_user_scope(
- app,
- username=username,
- role_name=role_name,
- permissions=[
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
- ],
- ) as user:
-
- dag_model = DagModel(dag_id=dag_id, fileloc="/tmp/dag_.py", schedule_interval="2 2 * * *")
- session.add(dag_model)
- session.commit()
-
- security_manager.sync_perm_for_dag( # type: ignore
- dag_id, access_control={role_name: permission_action}
- )
+ with app.app_context():
+ with create_user_scope(
+ app,
+ username=username,
+ role_name=role_name,
+ permissions=[
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ ],
+ ) as user:
+
+ dag_model = DagModel(dag_id=dag_id, fileloc="/tmp/dag_.py", schedule_interval="2 2 * * *")
+ session.add(dag_model)
+ session.commit()
- assert security_manager.get_accessible_dag_ids(user) == {"dag_id"}
+ security_manager.sync_perm_for_dag( # type: ignore
+ dag_id, access_control={role_name: permission_action}
+ )
+
+ assert security_manager.get_accessible_dag_ids(user) == {"dag_id"}
def test_dont_get_inaccessible_dag_ids_for_dag_resource_permission(app, security_manager, session):
@@ -481,25 +482,25 @@ def test_dont_get_inaccessible_dag_ids_for_dag_resource_permission(app, security
role_name = "MyRole1"
permission_action = [permissions.ACTION_CAN_EDIT]
dag_id = "dag_id"
+ with app.app_context():
+ with create_user_scope(
+ app,
+ username=username,
+ role_name=role_name,
+ permissions=[
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG),
+ ],
+ ) as user:
- with create_user_scope(
- app,
- username=username,
- role_name=role_name,
- permissions=[
- (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG),
- ],
- ) as user:
-
- dag_model = DagModel(dag_id=dag_id, fileloc="/tmp/dag_.py", schedule_interval="2 2 * * *")
- session.add(dag_model)
- session.commit()
-
- security_manager.sync_perm_for_dag( # type: ignore
- dag_id, access_control={role_name: permission_action}
- )
+ dag_model = DagModel(dag_id=dag_id, fileloc="/tmp/dag_.py", schedule_interval="2 2 * * *")
+ session.add(dag_model)
+ session.commit()
+
+ security_manager.sync_perm_for_dag( # type: ignore
+ dag_id, access_control={role_name: permission_action}
+ )
- assert security_manager.get_readable_dag_ids(user) == set()
+ assert security_manager.get_readable_dag_ids(user) == set()
def test_has_access(security_manager):
diff --git a/tests/www/views/test_views_home.py b/tests/www/views/test_views_home.py
index 4f6e354a7b..6619bcdf71 100644
--- a/tests/www/views/test_views_home.py
+++ b/tests/www/views/test_views_home.py
@@ -109,6 +109,31 @@ def client_single_dag(app, user_single_dag):
)
+@pytest.fixture(scope="module")
+def user_single_dag_edit(app):
+ """Create User that can edit DAG resource only a single DAG"""
+ return create_user(
+ app,
+ username="user_single_dag_edit",
+ role_name="role_single_dag",
+ permissions=[
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_WEBSITE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.resource_name_for_dag("filter_test_1")),
+ ],
+ )
+
+
+@pytest.fixture()
+def client_single_dag_edit(app, user_single_dag_edit):
+ """Client for User that can only edit the first DAG from TEST_FILTER_DAG_IDS"""
+ return client_with_login(
+ app,
+ username="user_single_dag_edit",
+ password="user_single_dag_edit",
+ )
+
+
TEST_FILTER_DAG_IDS = ["filter_test_1", "filter_test_2", "a_first_dag_id_asc", "filter.test"]
TEST_TAGS = ["example", "test", "team", "group"]
@@ -194,6 +219,17 @@ def test_home_dag_list_search(working_dags, user_client):
check_content_not_in_response("dag_id=a_first_dag_id_asc", resp)
+def test_home_dag_edit_permissions(capture_templates, working_dags, client_single_dag_edit):
+ with capture_templates() as templates:
+ client_single_dag_edit.get("home", follow_redirects=True)
+
+ dags = templates[0].local_context["dags"]
+ assert len(dags) > 0
+ dag_edit_perm_tuple = [(dag.dag_id, dag.can_edit) for dag in dags]
+ assert ("filter_test_1", True) in dag_edit_perm_tuple
+ assert ("filter_test_2", False) in dag_edit_perm_tuple
+
+
def test_home_robots_header_in_response(user_client):
# Responses should include X-Robots-Tag header
resp = user_client.get("home", follow_redirects=True)