(superset) 01/01: fix: check if guest user modified query

[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 f74908e8aeb9a707f6c531ba526aa990ce3d40cb
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 | 52 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/superset/security/manager.py b/superset/security/manager.py
index 83e12fb2dc..66bf4c623a 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -67,6 +67,7 @@ from superset.security.guest_token import (
     GuestTokenUser,
     GuestUser,
 )
+from superset.superset_typing import AdhocMetric
 from superset.utils.core import (
     DatasourceName,
     DatasourceType,
@@ -143,6 +144,43 @@ 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 = set(form_data.get("metrics") or [])
+    stored_metrics = set(stored_chart.params_dict.get("metrics") or [])
+    if requested_metrics != stored_metrics:
+        return True
+
+    # compare queries in query_context
+    queries_metrics: set[Union[AdhocMetric, str]] = set()
+    for query in query_context.queries:
+        queries_metrics.update(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(query.get("metrics") or [])  # type: ignore
+
+    return queries_metrics != stored_metrics
+
+
 class SupersetSecurityManager(  # pylint: disable=too-many-public-methods
     SecurityManager
 ):
@@ -1944,19 +1982,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,