From 08833b6eddf7e9f4149623373c303d20d2b45916 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Tue, 9 Jul 2024 16:22:45 -0400 Subject: [PATCH 1/4] fix: prevent guest users from changing columns --- superset/security/manager.py | 55 +++++----- tests/unit_tests/security/manager_test.py | 123 +++++++++++++++++++++- 2 files changed, 150 insertions(+), 28 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index d03da750796a3..04e2b737a76bc 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -145,9 +145,9 @@ def __init__(self, **kwargs: Any) -> None: RoleModelView.related_views = [] -def freeze_metric(metric: Metric) -> str: +def freeze_value(metric: Metric) -> str: """ - Used to compare metric sets. + Used to compare column and metric sets. """ return json.dumps(metric, sort_keys=True) @@ -170,32 +170,37 @@ def query_context_modified(query_context: "QueryContext") -> bool: if form_data.get("slice_id") != stored_chart.id: return True - # compare form_data - requested_metrics = { - freeze_metric(metric) for metric in form_data.get("metrics") or [] - } - stored_metrics = { - freeze_metric(metric) - for metric in stored_chart.params_dict.get("metrics") or [] - } - if not requested_metrics.issubset(stored_metrics): - return True + stored_query_context = ( + json.loads(cast(str, stored_chart.query_context)) + if stored_chart.query_context + else None + ) - # compare queries in query_context - queries_metrics = { - freeze_metric(metric) - for query in query_context.queries - for metric in query.metrics or [] - } + # compare columns and metrics in form_data with stored values + for key in ["metrics", "columns"]: + requested_values = {freeze_value(value) for value in form_data.get(key) or []} + stored_values = { + freeze_value(value) for value in stored_chart.params_dict.get(key) or [] + } + if not requested_values.issubset(stored_values): + return True - if stored_chart.query_context: - stored_query_context = json.loads(cast(str, stored_chart.query_context)) - for query in stored_query_context.get("queries") or []: - stored_metrics.update( - {freeze_metric(metric) for metric in query.get("metrics") or []} - ) + # compare queries in query_context + queries_values = { + freeze_value(value) + for query in query_context.queries + for value in getattr(query, key) or [] + } + if stored_query_context: + for query in stored_query_context.get("queries") or []: + stored_values.update( + {freeze_value(value) for value in query.get(key) or []} + ) + + if not queries_values.issubset(stored_values): + return True - return not queries_metrics.issubset(stored_metrics) + return False class SupersetSecurityManager( # pylint: disable=too-many-public-methods diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index e35513f2ff8a1..42a63aba789a1 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -31,7 +31,7 @@ SupersetSecurityManager, ) from superset.sql_parse import Table -from superset.superset_typing import AdhocMetric +from superset.superset_typing import AdhocColumn, AdhocMetric from superset.utils.core import override_user @@ -59,10 +59,24 @@ def stored_metrics() -> list[AdhocMetric]: ] +@pytest.fixture +def stored_columns() -> list[AdhocColumn]: + """ + Return a list of columns. + """ + return [ + { + "label": "My column", + "sqlExpression": "UPPER(name)", + }, + ] + + def test_raise_for_access_guest_user_ok( mocker: MockerFixture, app_context: None, stored_metrics: list[AdhocMetric], + stored_columns: list[AdhocColumn], ) -> None: """ Test that guest user can submit an unmodified chart payload. @@ -76,11 +90,43 @@ def test_raise_for_access_guest_user_ok( query_context.slice_.query_context = None query_context.slice_.params_dict = { "metrics": stored_metrics, + "columns": stored_columns, } query_context.form_data = { "slice_id": 42, "metrics": stored_metrics, + "columns": stored_columns, + } + query_context.queries = [QueryObject(metrics=stored_metrics)] # type: ignore + sm.raise_for_access(query_context=query_context) + + +def test_raise_for_access_guest_user_ok_subset( + mocker: MockerFixture, + app_context: None, + stored_metrics: list[AdhocMetric], + stored_columns: list[AdhocColumn], +) -> None: + """ + Test that guest user can submit a request of a subset of the metrics/columns. + """ + sm = SupersetSecurityManager(appbuilder) + mocker.patch.object(sm, "is_guest_user", return_value=True) + mocker.patch.object(sm, "can_access", return_value=True) + + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "metrics": stored_metrics, + "columns": stored_columns, + } + + query_context.form_data = { + "slice_id": 42, + "metrics": [], + "columns": [], } query_context.queries = [QueryObject(metrics=stored_metrics)] # type: ignore sm.raise_for_access(query_context=query_context) @@ -114,7 +160,7 @@ def test_raise_for_access_guest_user_tampered_id( sm.raise_for_access(query_context=query_context) -def test_raise_for_access_guest_user_tampered_form_data( +def test_raise_for_access_guest_user_tampered_form_data_metrics( mocker: MockerFixture, app_context: None, stored_metrics: list[AdhocMetric], @@ -151,7 +197,42 @@ def test_raise_for_access_guest_user_tampered_form_data( sm.raise_for_access(query_context=query_context) -def test_raise_for_access_guest_user_tampered_queries( +def test_raise_for_access_guest_user_tampered_form_data_columns( + mocker: MockerFixture, + app_context: None, + stored_columns: list[AdhocColumn], +) -> None: + """ + Test that guest user cannot modify columns in the form data. + """ + sm = SupersetSecurityManager(appbuilder) + mocker.patch.object(sm, "is_guest_user", return_value=True) + mocker.patch.object(sm, "can_access", return_value=True) + + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": stored_columns, + } + + tampered_columns = [ + { + "label": "My column", + "sqlExpression": "list_secret()", + "expressionType": "SQL", + }, + ] + + query_context.form_data = { + "slice_id": 42, + "columns": tampered_columns, + } + with pytest.raises(SupersetSecurityException): + sm.raise_for_access(query_context=query_context) + + +def test_raise_for_access_guest_user_tampered_queries_metrics( mocker: MockerFixture, app_context: None, stored_metrics: list[AdhocMetric], @@ -189,6 +270,42 @@ def test_raise_for_access_guest_user_tampered_queries( sm.raise_for_access(query_context=query_context) +def test_raise_for_access_guest_user_tampered_queries_columns( + mocker: MockerFixture, + app_context: None, + stored_columns: list[AdhocColumn], +) -> None: + """ + Test that guest user cannot modify columns in the queries. + """ + sm = SupersetSecurityManager(appbuilder) + mocker.patch.object(sm, "is_guest_user", return_value=True) + mocker.patch.object(sm, "can_access", return_value=True) + + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": stored_columns, + } + + tampered_columns = [ + { + "label": "My column", + "sqlExpression": "list_secret()", + "expressionType": "SQL", + } + ] + + query_context.form_data = { + "slice_id": 42, + "columns": stored_columns, + } + query_context.queries = [QueryObject(metrics=tampered_columns)] # type: ignore + with pytest.raises(SupersetSecurityException): + sm.raise_for_access(query_context=query_context) + + def test_raise_for_access_query_default_schema( mocker: MockerFixture, app_context: None, From 229fa1e7e973f84e3332941c541d7233104d79f7 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Wed, 10 Jul 2024 10:20:38 -0400 Subject: [PATCH 2/4] Consider groupby as well --- superset/security/manager.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 04e2b737a76bc..e0ef5c8cd042f 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -177,7 +177,7 @@ def query_context_modified(query_context: "QueryContext") -> bool: ) # compare columns and metrics in form_data with stored values - for key in ["metrics", "columns"]: + for key in ["metrics", "columns", "groupby"]: requested_values = {freeze_value(value) for value in form_data.get(key) or []} stored_values = { freeze_value(value) for value in stored_chart.params_dict.get(key) or [] @@ -189,7 +189,7 @@ def query_context_modified(query_context: "QueryContext") -> bool: queries_values = { freeze_value(value) for query in query_context.queries - for value in getattr(query, key) or [] + for value in getattr(query, key, []) or [] } if stored_query_context: for query in stored_query_context.get("queries") or []: From bee3895216c8167330db3d0701b83fed9cb1edfa Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Wed, 10 Jul 2024 10:31:59 -0400 Subject: [PATCH 3/4] Rename parameters --- superset/security/manager.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index e0ef5c8cd042f..cea33ccc0cb69 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -145,11 +145,11 @@ def __init__(self, **kwargs: Any) -> None: RoleModelView.related_views = [] -def freeze_value(metric: Metric) -> str: +def freeze_value(value: Any) -> str: """ Used to compare column and metric sets. """ - return json.dumps(metric, sort_keys=True) + return json.dumps(value, sort_keys=True) def query_context_modified(query_context: "QueryContext") -> bool: From 8b6e116b4f6db1ba1e30abb4c66ba3602c26ba69 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Wed, 10 Jul 2024 11:25:46 -0400 Subject: [PATCH 4/4] Add one more test --- superset/security/manager.py | 1 - tests/unit_tests/security/manager_test.py | 35 +++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index cea33ccc0cb69..53fc9aa232d84 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -67,7 +67,6 @@ GuestUser, ) from superset.sql_parse import extract_tables_from_jinja_sql, Table -from superset.superset_typing import Metric from superset.utils import json from superset.utils.core import ( DatasourceName, diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index 42a63aba789a1..2d3e7250be533 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -232,6 +232,41 @@ def test_raise_for_access_guest_user_tampered_form_data_columns( sm.raise_for_access(query_context=query_context) +def test_raise_for_access_guest_user_tampered_form_data_groupby( + mocker: MockerFixture, + app_context: None, + stored_columns: list[AdhocColumn], +) -> None: + """ + Test that guest user cannot modify groupby in the form data. + """ + sm = SupersetSecurityManager(appbuilder) + mocker.patch.object(sm, "is_guest_user", return_value=True) + mocker.patch.object(sm, "can_access", return_value=True) + + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "groupby": stored_columns, + } + + tampered_columns = [ + { + "label": "My column", + "sqlExpression": "list_secret()", + "expressionType": "SQL", + }, + ] + + query_context.form_data = { + "slice_id": 42, + "columns": tampered_columns, + } + with pytest.raises(SupersetSecurityException): + sm.raise_for_access(query_context=query_context) + + def test_raise_for_access_guest_user_tampered_queries_metrics( mocker: MockerFixture, app_context: None,