fix: chart import validation (apache#26993)

superset/commands/chart/importers/v1/utils.py

@@ -20,15 +20,14 @@
 from inspect import isclass
 from typing import Any
 
-from flask import g
 from sqlalchemy.orm import Session
 
 from superset import security_manager
 from superset.commands.exceptions import ImportFailedError
 from superset.migrations.shared.migrate_viz import processors
 from superset.migrations.shared.migrate_viz.base import MigrateViz
 from superset.models.slice import Slice
-from superset.utils.core import AnnotationType
+from superset.utils.core import AnnotationType, get_user
 
 
 def filter_chart_annotations(chart_config: dict[str, Any]) -> None:
@@ -55,6 +54,12 @@ def import_chart(
     can_write = ignore_permissions or security_manager.can_access("can_write", "Chart")
     existing = session.query(Slice).filter_by(uuid=config["uuid"]).first()
     if existing:
+        if overwrite and can_write and get_user():
+            if not security_manager.can_access_chart(existing):
+                raise ImportFailedError(
+                    "A chart already exists and user doesn't "
+                    "have permissions to overwrite it"
+                )
         if not overwrite or not can_write:
             return existing
         config["id"] = existing.id
@@ -77,8 +82,8 @@ def import_chart(
     if chart.id is None:
         session.flush()
 
-    if hasattr(g, "user") and g.user:
-        chart.owners.append(g.user)
+    if user := get_user():
+        chart.owners.append(user)
 
     return chart
 

superset/commands/dashboard/importers/v1/utils.py

@@ -19,12 +19,12 @@
 import logging
 from typing import Any
 
-from flask import g
 from sqlalchemy.orm import Session
 
 from superset import security_manager
 from superset.commands.exceptions import ImportFailedError
 from superset.models.dashboard import Dashboard
+from superset.utils.core import get_user
 
 logger = logging.getLogger(__name__)
 
@@ -157,7 +157,7 @@ def import_dashboard(
     )
     existing = session.query(Dashboard).filter_by(uuid=config["uuid"]).first()
     if existing:
-        if overwrite and can_write and hasattr(g, "user") and g.user:
+        if overwrite and can_write and get_user():
             if not security_manager.can_access_dashboard(existing):
                 raise ImportFailedError(
                     "A dashboard already exists and user doesn't "
@@ -191,7 +191,7 @@ def import_dashboard(
     if dashboard.id is None:
         session.flush()
 
-    if hasattr(g, "user") and g.user:
-        dashboard.owners.append(g.user)
+    if user := get_user():
+        dashboard.owners.append(user)
 
     return dashboard

superset/commands/dataset/importers/v1/utils.py

@@ -22,7 +22,7 @@
 from urllib import request
 
 import pandas as pd
-from flask import current_app, g
+from flask import current_app
 from sqlalchemy import BigInteger, Boolean, Date, DateTime, Float, String, Text
 from sqlalchemy.exc import MultipleResultsFound
 from sqlalchemy.orm import Session
@@ -33,6 +33,7 @@
 from superset.commands.exceptions import ImportFailedError
 from superset.connectors.sqla.models import SqlaTable
 from superset.models.core import Database
+from superset.utils.core import get_user
 
 logger = logging.getLogger(__name__)
 
@@ -176,8 +177,8 @@ def import_dataset(
     if data_uri and (not table_exists or force_data):
         load_data(data_uri, dataset, dataset.database, session)
 
-    if hasattr(g, "user") and g.user:
-        dataset.owners.append(g.user)
+    if user := get_user():
+        dataset.owners.append(user)
 
     return dataset
 

superset/errors.py

@@ -66,6 +66,7 @@ class SupersetErrorType(StrEnum):
     MISSING_OWNERSHIP_ERROR = "MISSING_OWNERSHIP_ERROR"
     USER_ACTIVITY_SECURITY_ACCESS_ERROR = "USER_ACTIVITY_SECURITY_ACCESS_ERROR"
     DASHBOARD_SECURITY_ACCESS_ERROR = "DASHBOARD_SECURITY_ACCESS_ERROR"
+    CHART_SECURITY_ACCESS_ERROR = "CHART_SECURITY_ACCESS_ERROR"
 
     # Other errors
     BACKEND_TIMEOUT_ERROR = "BACKEND_TIMEOUT_ERROR"

superset/jinja_context.py

@@ -36,7 +36,7 @@
 from superset.extensions import feature_flag_manager
 from superset.utils.core import (
     convert_legacy_filters_into_adhoc,
-    get_user_id,
+    get_user,
     merge_extra_filters,
 )
 
@@ -110,11 +110,10 @@ def current_user_id(self, add_to_cache_keys: bool = True) -> Optional[int]:
         :returns: The user ID
         """
 
-        if hasattr(g, "user") and g.user:
-            id_ = get_user_id()
+        if user := get_user():
             if add_to_cache_keys:
-                self.cache_key_wrapper(id_)
-            return id_
+                self.cache_key_wrapper(user.id)
+            return user.id
         return None
 
     def current_username(self, add_to_cache_keys: bool = True) -> Optional[str]:

superset/security/manager.py

@@ -85,6 +85,7 @@
     )
     from superset.models.core import Database
     from superset.models.dashboard import Dashboard
+    from superset.models.slice import Slice
     from superset.models.sql_lab import Query
     from superset.sql_parse import Table
     from superset.viz import BaseViz
@@ -422,6 +423,19 @@ def can_access_dashboard(self, dashboard: "Dashboard") -> bool:
 
         return True
 
+    def can_access_chart(self, chart: "Slice") -> bool:
+        """
+        Return True if the user can access the specified chart, False otherwise.
+        :param chart: The chart
+        :return: Whether the user can access the chart
+        """
+        try:
+            self.raise_for_access(chart=chart)
+        except SupersetSecurityException:
+            return False
+
+        return True
+
     def get_dashboard_access_error_object(  # pylint: disable=invalid-name
         self,
         dashboard: "Dashboard",  # pylint: disable=unused-argument
@@ -439,6 +453,23 @@ def get_dashboard_access_error_object(  # pylint: disable=invalid-name
             level=ErrorLevel.ERROR,
         )
 
+    def get_chart_access_error_object(  # pylint: disable=invalid-name
+        self,
+        dashboard: "Dashboard",  # pylint: disable=unused-argument
+    ) -> SupersetError:
+        """
+        Return the error object for the denied Superset dashboard.
+
+        :param dashboard: The denied Superset dashboard
+        :returns: The error object
+        """
+
+        return SupersetError(
+            error_type=SupersetErrorType.CHART_SECURITY_ACCESS_ERROR,
+            message="You don't have access to this chart.",
+            level=ErrorLevel.ERROR,
+        )
+
     @staticmethod
     def get_datasource_access_error_msg(datasource: "BaseDatasource") -> str:
         """
@@ -1822,6 +1853,7 @@ def raise_for_access(
         # pylint: disable=too-many-arguments,too-many-branches,too-many-locals,too-many-statements
         self,
         dashboard: Optional["Dashboard"] = None,
+        chart: Optional["Slice"] = None,
         database: Optional["Database"] = None,
         datasource: Optional["BaseDatasource"] = None,
         query: Optional["Query"] = None,
@@ -2044,6 +2076,15 @@ def raise_for_access(
                 self.get_dashboard_access_error_object(dashboard)
             )
 
+        if chart:
+            if self.is_admin() or self.is_owner(chart):
+                return
+
+            if chart.datasource and self.can_access_datasource(chart.datasource):
+                return
+
+            raise SupersetSecurityException(self.get_chart_access_error_object(chart))
+
     def get_user_by_username(self, username: str) -> Optional[User]:
         """
         Retrieves a user by it's username case sensitive. Optional session parameter

superset/utils/core.py

@@ -1344,6 +1344,15 @@ def split_adhoc_filters_into_base_filters(  # pylint: disable=invalid-name
         form_data["filters"] = simple_where_filters
 
 
+def get_user() -> User | None:
+    """
+    Get the current user (if defined).
+
+    :returns: The current user
+    """
+    return g.user if hasattr(g, "user") else None
+
+
 def get_username() -> str | None:
     """
     Get username (if defined) associated with the current user.

tests/integration_tests/charts/commands_tests.py

@@ -171,7 +171,7 @@ def test_export_chart_command_no_related(self, mock_g):
 
 
 class TestImportChartsCommand(SupersetTestCase):
-    @patch("superset.commands.chart.importers.v1.utils.g")
+    @patch("superset.utils.core.g")
     @patch("superset.security.manager.g")
     def test_import_v1_chart(self, sm_g, utils_g):
         """Test that we can import a chart"""

tests/integration_tests/dashboards/commands_tests.py

@@ -486,7 +486,7 @@ def test_import_v0_dashboard_cli_export(self):
         db.session.delete(dataset)
         db.session.commit()
 
-    @patch("superset.commands.dashboard.importers.v1.utils.g")
+    @patch("superset.utils.core.g")
     @patch("superset.security.manager.g")
     def test_import_v1_dashboard(self, sm_g, utils_g):
         """Test that we can import a dashboard"""

tests/integration_tests/datasets/commands_tests.py

@@ -339,7 +339,7 @@ def test_import_v0_dataset_ui_export(self):
         db.session.delete(dataset)
         db.session.commit()
 
-    @patch("superset.commands.dataset.importers.v1.utils.g")
+    @patch("superset.utils.core.g")
     @patch("superset.security.manager.g")
     @pytest.mark.usefixtures("load_energy_table_with_slice")
     def test_import_v1_dataset(self, sm_g, utils_g):