This is an automated email from the ASF dual-hosted git repository.
kaxilnaik pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/main by this push:
new 87ea8acd0c Fix inconsitencies in checking edit permissions for a DAG
(#20346)
87ea8acd0c is described below
commit 87ea8acd0c8e7339249bec51908b35e5e3d70129
Author: Kaxil Naik <kaxiln...@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.
---
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 bd4c34c9bd..6b2561292a 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 d2928ade17..8618a61fa6 100644
--- a/airflow/www/views.py
+++ b/airflow/www/views.py
@@ -769,17 +769,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(
@@ -789,16 +783,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 ac42bb6019..31e6ca3b46 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)
