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/03/19 11:54:34 UTC
(superset) 05/09: fix: check if guest user modified query (#27484)
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.1
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 51f904c42cc1573059976c964a2d5bf945e2912b
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Tue Mar 12 21:28:06 2024 -0400
fix: check if guest user modified query (#27484)
(cherry picked from commit 735b895dd5e409bfc95406e847a82fd786d93a1d)
---
superset/security/manager.py | 85 +++++++++++++++++------
tests/unit_tests/security/manager_test.py | 112 +++++++++++++++++++++++++-----
2 files changed, 156 insertions(+), 41 deletions(-)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index e6eb77e645..38f4e4bdfa 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -143,6 +143,56 @@ RoleModelView.edit_columns = ["name", "permissions", "user"]
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 @@ 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,
- )
+ # 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 7d2b9153a3..5a06013a68 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.exceptions import SupersetSecurityException
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,