You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2024/02/01 16:43:10 UTC
(superset) 03/09: fix: prevent guest user from modifying metrics (#26749)
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit e453059413ee14c8eb1200c3855ffa9a2722b040
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Mon Jan 29 12:59:33 2024 -0500
fix: prevent guest user from modifying metrics (#26749)
(cherry picked from commit fade4806ceebde32a775c04d86a46c7e93bc371f)
---
superset/common/query_object.py | 2 +-
superset/security/manager.py | 26 +++++++-
.../security/guest_token_security_tests.py | 15 +++++
tests/unit_tests/security/manager_test.py | 76 ++++++++++++++++++++++
4 files changed, 117 insertions(+), 2 deletions(-)
diff --git a/superset/common/query_object.py b/superset/common/query_object.py
index 1e826761ec..ea56a9072b 100644
--- a/superset/common/query_object.py
+++ b/superset/common/query_object.py
@@ -125,7 +125,7 @@ class QueryObject: # pylint: disable=too-many-instance-attributes
order_desc: bool = True,
orderby: list[OrderBy] | None = None,
post_processing: list[dict[str, Any] | None] | None = None,
- row_limit: int | None,
+ row_limit: int | None = None,
row_offset: int | None = None,
series_columns: list[Column] | None = None,
series_limit: int = 0,
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 5c0833fdf9..f7f8662b5d 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -1797,7 +1797,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
return []
def raise_for_access(
- # pylint: disable=too-many-arguments,too-many-branches,too-many-locals
+ # pylint: disable=too-many-arguments,too-many-branches,too-many-locals,too-many-statements
self,
dashboard: Optional["Dashboard"] = None,
database: Optional["Database"] = None,
@@ -1887,6 +1887,30 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
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,
+ )
+ )
+
if datasource or query_context or viz:
form_data = None
diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py
index 4cd8a77f76..b812929433 100644
--- a/tests/integration_tests/security/guest_token_security_tests.py
+++ b/tests/integration_tests/security/guest_token_security_tests.py
@@ -288,7 +288,10 @@ class TestGuestUserDatasourceAccess(SupersetTestCase):
form_data={
"dashboardId": self.dash.id,
"slice_id": self.chart.id,
+ "metrics": self.chart.params_dict["metrics"],
},
+ slice_=self.chart,
+ queries=[],
)
}
)
@@ -304,7 +307,11 @@ class TestGuestUserDatasourceAccess(SupersetTestCase):
"dashboardId": self.dash.id,
"native_filter_id": "NATIVE_FILTER-ABCDEFGH",
"type": "NATIVE_FILTER",
+ "slice_id": self.chart.id,
+ "metrics": self.chart.params_dict["metrics"],
},
+ slice_=self.chart,
+ queries=[],
)
}
)
@@ -382,7 +389,11 @@ class TestGuestUserDatasourceAccess(SupersetTestCase):
form_data={
"dashboardId": self.dash.id,
"type": "NATIVE_FILTER",
+ "slice_id": self.chart.id,
+ "metrics": self.chart.params_dict["metrics"],
},
+ slice_=self.chart,
+ queries=[],
)
}
)
@@ -399,7 +410,11 @@ class TestGuestUserDatasourceAccess(SupersetTestCase):
"dashboardId": self.dash.id,
"native_filter_id": "NATIVE_FILTER-ABCDEFGH",
"type": "NATIVE_FILTER",
+ "slice_id": self.chart.id,
+ "metrics": self.chart.params_dict["metrics"],
},
+ slice_=self.chart,
+ queries=[],
)
}
)
diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py
index 1843e7261c..ad6e53e993 100644
--- a/tests/unit_tests/security/manager_test.py
+++ b/tests/unit_tests/security/manager_test.py
@@ -18,6 +18,7 @@
import pytest
from pytest_mock import MockFixture
+from superset.common.query_object import QueryObject
from superset.exceptions import SupersetSecurityException
from superset.extensions import appbuilder
from superset.security.manager import SupersetSecurityManager
@@ -31,6 +32,81 @@ def test_security_manager(app_context: None) -> None:
assert sm
+def test_raise_for_access_guest_user(
+ mocker: MockFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that guest user can't modify chart payload.
+ """
+ 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
+ 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_.params_dict = {
+ "metrics": stored_metrics,
+ }
+
+ # normal request
+ query_context.form_data = {
+ "slice_id": 42,
+ "metrics": stored_metrics,
+ }
+ query_context.queries = [QueryObject(metrics=stored_metrics)] # type: ignore
+ sm.raise_for_access(query_context=query_context)
+
+ # tampered requests
+ query_context.form_data = {
+ "slice_id": 43,
+ "metrics": stored_metrics,
+ }
+ query_context.queries = [QueryObject(metrics=stored_metrics)] # type: ignore
+ with pytest.raises(SupersetSecurityException):
+ sm.raise_for_access(query_context=query_context)
+
+ tampered_metrics = [
+ {
+ "aggregate": None,
+ "column": None,
+ "datasourceWarning": False,
+ "expressionType": "SQL",
+ "hasCustomLabel": False,
+ "label": "COUNT(*) + 2",
+ "optionName": "metric_ssa1gwimio_cxpyjc7vj3s",
+ "sqlExpression": "COUNT(*) + 2",
+ }
+ ]
+
+ query_context.form_data = {
+ "slice_id": 42,
+ "metrics": tampered_metrics,
+ }
+ with pytest.raises(SupersetSecurityException):
+ sm.raise_for_access(query_context=query_context)
+
+ query_context.form_data = {
+ "slice_id": 42,
+ "metrics": stored_metrics,
+ }
+ query_context.queries = [QueryObject(metrics=tampered_metrics)] # type: ignore
+ with pytest.raises(SupersetSecurityException):
+ sm.raise_for_access(query_context=query_context)
+
+
def test_raise_for_access_query_default_schema(
mocker: MockFixture,
app_context: None,