diff --git a/superset/security/manager.py b/superset/security/manager.py index 83e12fb2dcde1..d56c0ad688c1a 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -143,6 +143,56 @@ def __init__(self, **kwargs: Any) -> None: RoleModelView.related_views = [] +def query_context_modified(query_context: "QueryContext") -> bool: + """ + Check if a query context has been modified. + + This is used to ensure guest users don't modify the payload and fetch data + different from what was shared with them in dashboards. + """ + form_data = query_context.form_data + stored_chart = query_context.slice_ + + # sanity checks + if form_data is None or stored_chart is None: + return True + + # cannot request a different chart + if form_data.get("slice_id") != stored_chart.id: + return True + + # compare form_data + requested_metrics = { + frozenset(metric.items()) if isinstance(metric, dict) else metric + for metric in form_data.get("metrics") or [] + } + stored_metrics = { + frozenset(metric.items()) if isinstance(metric, dict) else metric + for metric in stored_chart.params_dict.get("metrics") or [] + } + if not requested_metrics.issubset(stored_metrics): + return True + + # compare queries in query_context + queries_metrics = { + frozenset(metric.items()) if isinstance(metric, dict) else metric + for query in query_context.queries + for metric in query.metrics or [] + } + + 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( + { + frozenset(metric.items()) if isinstance(metric, dict) else metric + for metric in query.get("metrics") or [] + } + ) + + return not queries_metrics.issubset(stored_metrics) + + class SupersetSecurityManager( # pylint: disable=too-many-public-methods SecurityManager ): @@ -1941,29 +1991,20 @@ def raise_for_access( self.get_table_access_error_object(denied) ) - if self.is_guest_user() and query_context: - # Guest users MUST not modify the payload so it's requesting a different - # chart or different ad-hoc metrics from what's saved. - form_data = query_context.form_data - stored_chart = query_context.slice_ - - if ( - form_data is None - or stored_chart is None - or form_data.get("slice_id") != stored_chart.id - or form_data.get("metrics", []) != stored_chart.params_dict["metrics"] - or any( - query.metrics != stored_chart.params_dict["metrics"] - for query in query_context.queries - ) - ): - raise SupersetSecurityException( - SupersetError( - error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR, - message=_("Guest user cannot modify chart payload"), - level=ErrorLevel.ERROR, - ) + # Guest users MUST not modify the payload so it's requesting a + # different chart or different ad-hoc metrics from what's saved. + if ( + query_context + and self.is_guest_user() + and query_context_modified(query_context) + ): + raise SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR, + message=_("Guest user cannot modify chart payload"), + level=ErrorLevel.ERROR, ) + ) if datasource or query_context or viz: form_data = None diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index 7d2b9153a392d..5a06013a683bb 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -15,6 +15,8 @@ # specific language governing permissions and limitations # under the License. +# pylint: disable=invalid-name, unused-argument, redefined-outer-name + import pytest from flask_appbuilder.security.sqla.models import Role, User from pytest_mock import MockFixture @@ -25,6 +27,7 @@ from superset.extensions import appbuilder from superset.models.slice import Slice from superset.security.manager import SupersetSecurityManager +from superset.superset_typing import AdhocMetric from superset.utils.core import override_user @@ -36,12 +39,29 @@ def test_security_manager(app_context: None) -> None: assert sm -def test_raise_for_access_guest_user( +@pytest.fixture +def stored_metrics() -> list[AdhocMetric]: + """ + Return a list of metrics. + """ + return [ + { + "column": None, + "expressionType": "SQL", + "hasCustomLabel": False, + "label": "COUNT(*) + 1", + "sqlExpression": "COUNT(*) + 1", + }, + ] + + +def test_raise_for_access_guest_user_ok( mocker: MockFixture, app_context: None, + stored_metrics: list[AdhocMetric], ) -> None: """ - Test that guest user can't modify chart payload. + Test that guest user can submit an unmodified chart payload. """ sm = SupersetSecurityManager(appbuilder) mocker.patch.object(sm, "is_guest_user", return_value=True) @@ -49,23 +69,11 @@ def test_raise_for_access_guest_user( query_context = mocker.MagicMock() query_context.slice_.id = 42 - stored_metrics = [ - { - "aggregate": None, - "column": None, - "datasourceWarning": False, - "expressionType": "SQL", - "hasCustomLabel": False, - "label": "COUNT(*) + 1", - "optionName": "metric_ssa1gwimio_cxpyjc7vj3s", - "sqlExpression": "COUNT(*) + 1", - } - ] + query_context.slice_.query_context = None query_context.slice_.params_dict = { "metrics": stored_metrics, } - # normal request query_context.form_data = { "slice_id": 42, "metrics": stored_metrics, @@ -73,7 +81,26 @@ def test_raise_for_access_guest_user( query_context.queries = [QueryObject(metrics=stored_metrics)] # type: ignore sm.raise_for_access(query_context=query_context) - # tampered requests + +def test_raise_for_access_guest_user_tampered_id( + mocker: MockFixture, + app_context: None, + stored_metrics: list[AdhocMetric], +) -> None: + """ + Test that guest user cannot modify the chart ID. + """ + 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, + } + query_context.form_data = { "slice_id": 43, "metrics": stored_metrics, @@ -82,15 +109,32 @@ def test_raise_for_access_guest_user( with pytest.raises(SupersetSecurityException): sm.raise_for_access(query_context=query_context) + +def test_raise_for_access_guest_user_tampered_form_data( + mocker: MockFixture, + app_context: None, + stored_metrics: list[AdhocMetric], +) -> None: + """ + Test that guest user cannot modify metrics 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 = { + "metrics": stored_metrics, + } + tampered_metrics = [ { - "aggregate": None, "column": None, - "datasourceWarning": False, "expressionType": "SQL", "hasCustomLabel": False, "label": "COUNT(*) + 2", - "optionName": "metric_ssa1gwimio_cxpyjc7vj3s", "sqlExpression": "COUNT(*) + 2", } ] @@ -102,6 +146,36 @@ def test_raise_for_access_guest_user( with pytest.raises(SupersetSecurityException): sm.raise_for_access(query_context=query_context) + +def test_raise_for_access_guest_user_tampered_queries( + mocker: MockFixture, + app_context: None, + stored_metrics: list[AdhocMetric], +) -> None: + """ + Test that guest user cannot modify metrics 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 = { + "metrics": stored_metrics, + } + + tampered_metrics = [ + { + "column": None, + "expressionType": "SQL", + "hasCustomLabel": False, + "label": "COUNT(*) + 2", + "sqlExpression": "COUNT(*) + 2", + } + ] + query_context.form_data = { "slice_id": 42, "metrics": stored_metrics,