From 4c0270112662bd5e2d82508ab14d9d41cb8b0185 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Wed, 10 Jul 2024 12:26:51 -0400 Subject: [PATCH] fix: prevent guest users from changing columns (#29530) --- superset/security/manager.py | 58 ++++---- tests/unit_tests/security/manager_test.py | 158 +++++++++++++++++++++- 2 files changed, 186 insertions(+), 30 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index d03da750796a3..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, @@ -145,11 +144,11 @@ def __init__(self, **kwargs: Any) -> None: RoleModelView.related_views = [] -def freeze_metric(metric: Metric) -> str: +def freeze_value(value: Any) -> str: """ - Used to compare metric sets. + 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: @@ -170,32 +169,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", "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 [] + } + 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..2d3e7250be533 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,77 @@ 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_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, stored_metrics: list[AdhocMetric], @@ -189,6 +305,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,