This is an automated email from the ASF dual-hosted git repository.
beto pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 67df4e3ce3 fix: prevent guest users from changing columns (#29530)
67df4e3ce3 is described below
commit 67df4e3ce3b2d2d3aacad5db93a67483e5db58e7
Author: Beto Dealmeida <[email protected]>
AuthorDate: Wed Jul 10 12:26:51 2024 -0400
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 d03da75079..53fc9aa232 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -67,7 +67,6 @@ from superset.security.guest_token import (
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 @@ RoleModelView.edit_columns = ["name", "permissions",
"user"]
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 e35513f2ff..2d3e7250be 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,24 @@ def stored_metrics() -> list[AdhocMetric]:
]
[email protected]
+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,
