(superset) 01/01: feat: prevent adhoc metrics from guest user

[be...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

beto pushed a commit to branch supersetsec-41
in repository https://gitbox.apache.org/repos/asf/superset.git

commit 7bdc483c17685f9d886ec57f4e61ba4604a55e19
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Thu Jan 18 15:33:13 2024 -0500

    feat: prevent adhoc metrics from guest user
---
 superset/common/query_object.py               | 17 ++++++-
 tests/unit_tests/queries/query_object_test.py | 73 ++++++++++++++++++++++++++-
 2 files changed, 87 insertions(+), 3 deletions(-)

diff --git a/superset/common/query_object.py b/superset/common/query_object.py
index 989df5775b..226d81debb 100644
--- a/superset/common/query_object.py
+++ b/superset/common/query_object.py
@@ -27,12 +27,14 @@ from flask import g
 from flask_babel import gettext as _
 from pandas import DataFrame
 
-from superset import feature_flag_manager
+from superset import feature_flag_manager, security_manager
 from superset.common.chart_data import ChartDataResultType
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
 from superset.exceptions import (
     InvalidPostProcessingError,
     QueryClauseValidationException,
     QueryObjectValidationError,
+    SupersetSecurityException,
 )
 from superset.sql_parse import sanitize_clause
 from superset.superset_typing import Column, Metric, OrderBy
@@ -189,6 +191,19 @@ class QueryObject:  # pylint: disable=too-many-instance-attributes
         def is_str_or_adhoc(metric: Metric) -> bool:
             return isinstance(metric, str) or is_adhoc_metric(metric)
 
+        # When connecting with a guest token users should only be allowed to use
+        # predefined metrics.
+        if security_manager.is_guest_user() and any(
+            is_adhoc_metric(metric) for metric in metrics or []
+        ):
+            raise SupersetSecurityException(
+                SupersetError(
+                    error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR,
+                    message="Guest users can only use predefined metrics",
+                    level=ErrorLevel.ERROR,
+                ),
+            )
+
         self.metrics = metrics and [
             x if is_str_or_adhoc(x) else x["label"] for x in metrics  # type: ignore
         ]
diff --git a/tests/unit_tests/queries/query_object_test.py b/tests/unit_tests/queries/query_object_test.py
index 81a654653f..be12e795a2 100644
--- a/tests/unit_tests/queries/query_object_test.py
+++ b/tests/unit_tests/queries/query_object_test.py
@@ -16,13 +16,17 @@
 # under the License.
 from unittest.mock import call, patch
 
+import pytest
 from flask_appbuilder.security.sqla.models import User
-from pytest_mock import MockFixture
+from pytest_mock import MockerFixture
 
 from superset.common.query_object import QueryObject
 from superset.connectors.sqla.models import SqlaTable
+from superset.errors import SupersetErrorType
+from superset.exceptions import SupersetSecurityException
 from superset.models.core import Database
-from superset.utils.core import override_user
+from superset.utils.core import AdhocMetric, override_user
+from tests.unit_tests.conftest import with_feature_flags
 
 
 def cache_impersonation_flag_side_effect(feature=None):
@@ -343,3 +347,68 @@ def test_cache_key_cache_impersonation_on_with_different_user_and_db_impersonati
             ),
         ]
     )
+
+
+def test_query_object_metrics():
+    """
+    Tests that metrics are correctly parsed from the query object.
+    """
+    query_object = QueryObject(row_limit=1, metrics=["sum__SP_POP_TOTL"])
+    assert query_object.metrics == ["sum__SP_POP_TOTL"]
+
+    query_object = QueryObject(row_limit=1, metrics=[{"label": "sum__SP_POP_TOTL"}])
+    assert query_object.metrics == ["sum__SP_POP_TOTL"]
+
+    query_object = QueryObject(
+        row_limit=1,
+        metrics=[
+            {
+                "aggregate": "SUM",
+                "column": None,
+                "expressionType": "SQL",
+                "hasCustomLabel": False,
+                "label": 'sum("SP_POP_TOTL")',
+                "sqlExpression": 'sum("SP_POP_TOTL")',
+            }
+        ],
+    )
+    assert query_object.metrics == [
+        {
+            "aggregate": "SUM",
+            "column": None,
+            "expressionType": "SQL",
+            "hasCustomLabel": False,
+            "label": 'sum("SP_POP_TOTL")',
+            "sqlExpression": 'sum("SP_POP_TOTL")',
+        },
+    ]
+
+
+@with_feature_flags(EMBEDDED_SUPERSET=True)
+def test_query_object_embedded(mocker: MockerFixture):
+    """
+    Test that only predefined metrics are allowed in embedded dashboards.
+    """
+    g = mocker.patch("superset.security.manager.g")
+    g.user.is_guest_user = True
+
+    query_object = QueryObject(row_limit=1, metrics=["sum__SP_POP_TOTL"])
+    assert query_object.metrics == ["sum__SP_POP_TOTL"]
+
+    query_object = QueryObject(row_limit=1, metrics=[{"label": "sum__SP_POP_TOTL"}])
+    assert query_object.metrics == ["sum__SP_POP_TOTL"]
+
+    adhoc_metric: AdhocMetric = {
+        "aggregate": "SUM",
+        "column": None,
+        "expressionType": "SQL",
+        "hasCustomLabel": False,
+        "label": 'sum("SP_POP_TOTL")',
+        "sqlExpression": 'sum("SP_POP_TOTL")',
+    }
+    with pytest.raises(SupersetSecurityException) as excinfo:
+        QueryObject(row_limit=1, metrics=[adhoc_metric])
+    assert (
+        excinfo.value.error.error_type
+        == SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR
+    )