You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by be...@apache.org on 2024/03/12 21:14:56 UTC
(superset) branch sc-79188 updated (8ab585aba1 -> bf879a8516)
This is an automated email from the ASF dual-hosted git repository.
beto pushed a change to branch sc-79188
in repository https://gitbox.apache.org/repos/asf/superset.git
omit 8ab585aba1 fix: check if guest user modified query
new bf879a8516 fix: check if guest user modified query
This update added new revisions after undoing existing revisions.
That is to say, some revisions that were in the old version of the
branch are not in the new version. This situation occurs
when a user --force pushes a change and generates a repository
containing something like this:
* -- * -- B -- O -- O -- O (8ab585aba1)
\
N -- N -- N refs/heads/sc-79188 (bf879a8516)
You should already have received notification emails for all of the O
revisions, and so the following emails describe only the N revisions
from the common base, B.
Any revisions marked "omit" are not gone; other references still
refer to them. Any revisions marked "discard" are gone forever.
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
superset/security/manager.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
(superset) 01/01: fix: check if guest user modified query
Posted by be...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
beto pushed a commit to branch sc-79188
in repository https://gitbox.apache.org/repos/asf/superset.git
commit bf879a85167b9863e184a7cbcb34a93988e63b28
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Tue Mar 12 12:38:40 2024 -0400
fix: check if guest user modified query
---
superset/security/manager.py | 64 +++++++++++++----
tests/unit_tests/security/manager_test.py | 112 +++++++++++++++++++++++++-----
2 files changed, 144 insertions(+), 32 deletions(-)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 83e12fb2dc..69d42993a4 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 <= 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 <= stored_metrics
+
+
class SupersetSecurityManager( # pylint: disable=too-many-public-methods
SecurityManager
):
@@ -1944,19 +1994,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
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
- )
- ):
+ if query_context_modified(query_context):
raise SupersetSecurityException(
SupersetError(
error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR,
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,