This is an automated email from the ASF dual-hosted git repository. beto pushed a commit to branch prevent-column-modifications in repository https://gitbox.apache.org/repos/asf/superset.git
commit 15467b8bf5cef9862423c57718aa7e4dce30550d Author: Beto Dealmeida <[email protected]> AuthorDate: Tue Jul 9 16:22:45 2024 -0400 fix: prevent guest users from changing columns --- superset/security/manager.py | 55 +++++++------ tests/unit_tests/security/manager_test.py | 124 +++++++++++++++++++++++++++++- 2 files changed, 151 insertions(+), 28 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index d03da75079..04e2b737a7 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -145,9 +145,9 @@ RoleModelView.edit_columns = ["name", "permissions", "user"] 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 e35513f2ff..0c11bebb02 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -31,7 +31,7 @@ from superset.security.manager import ( 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,25 @@ def stored_metrics() -> list[AdhocMetric]: ] [email protected] +def stored_columns() -> list[AdhocColumn]: + """ + Return a list of columns. + """ + return [ + { + "label": "My column", + "sqlExpression": "UPPER(name)", + "expressionType": "SQL", + }, + ] + + 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 +91,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 +161,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 +198,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 +271,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,
