fix: permission checks on import (#23200)

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

@@ -21,17 +21,24 @@
 from flask import g
 from sqlalchemy.orm import Session
 
+from superset import security_manager
+from superset.commands.exceptions import ImportFailedError
 from superset.models.slice import Slice
 
 
 def import_chart(
     session: Session, config: Dict[str, Any], overwrite: bool = False
 ) -> Slice:
+    can_write = security_manager.can_access("can_write", "Chart")
     existing = session.query(Slice).filter_by(uuid=config["uuid"]).first()
     if existing:
-        if not overwrite:
+        if not overwrite or not can_write:
             return existing
         config["id"] = existing.id
+    elif not can_write:
+        raise ImportFailedError(
+            "Chart doesn't exist and user doesn't have permission to create charts"
+        )
 
     # TODO (betodealmeida): move this logic to import_from_dict
     config["params"] = json.dumps(config["params"])

superset/commands/importers/v1/examples.py

@@ -21,7 +21,7 @@
 from sqlalchemy.orm.exc import MultipleResultsFound
 from sqlalchemy.sql import select
 
-from superset import db
+from superset import db, security_manager
 from superset.charts.commands.importers.v1 import ImportChartsCommand
 from superset.charts.commands.importers.v1.utils import import_chart
 from superset.charts.schemas import ImportV1ChartSchema
@@ -42,7 +42,7 @@
 from superset.datasets.commands.importers.v1.utils import import_dataset
 from superset.datasets.schemas import ImportV1DatasetSchema
 from superset.models.dashboard import dashboard_slices
-from superset.utils.core import get_example_default_schema
+from superset.utils.core import get_example_default_schema, override_user
 from superset.utils.database import get_example_database
 
 
@@ -69,7 +69,13 @@ def run(self) -> None:
 
         # rollback to prevent partial imports
         try:
-            self._import(db.session, self._configs, self.overwrite, self.force_data)
+            with override_user(security_manager.find_user(username="admin")):
+                self._import(
+                    db.session,
+                    self._configs,
+                    self.overwrite,
+                    self.force_data,
+                )
             db.session.commit()
         except Exception as ex:
             db.session.rollback()
@@ -119,13 +125,12 @@ def _import(  # pylint: disable=arguments-differ, too-many-locals, too-many-bran
                 if config["schema"] is None:
                     config["schema"] = get_example_default_schema()
 
-                dataset = import_dataset(
-                    session, config, overwrite=overwrite, force_data=force_data
-                )
-
                 try:
                     dataset = import_dataset(
-                        session, config, overwrite=overwrite, force_data=force_data
+                        session,
+                        config,
+                        overwrite=overwrite,
+                        force_data=force_data,
                     )
                 except MultipleResultsFound:
                     # Multiple result can be found for datasets. There was a bug in

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

@@ -22,6 +22,8 @@
 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
 
 logger = logging.getLogger(__name__)
@@ -146,11 +148,17 @@ def update_id_refs(  # pylint: disable=too-many-locals
 def import_dashboard(
     session: Session, config: Dict[str, Any], overwrite: bool = False
 ) -> Dashboard:
+    can_write = security_manager.can_access("can_write", "Dashboard")
     existing = session.query(Dashboard).filter_by(uuid=config["uuid"]).first()
     if existing:
-        if not overwrite:
+        if not overwrite or not can_write:
             return existing
         config["id"] = existing.id
+    elif not can_write:
+        raise ImportFailedError(
+            "Dashboard doesn't exist and user doesn't "
+            "have permission to create dashboards"
+        )
 
     # TODO (betodealmeida): move this logic to import_from_dict
     config = config.copy()

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

@@ -20,18 +20,27 @@
 
 from sqlalchemy.orm import Session
 
+from superset import security_manager
+from superset.commands.exceptions import ImportFailedError
 from superset.databases.ssh_tunnel.models import SSHTunnel
 from superset.models.core import Database
 
 
 def import_database(
-    session: Session, config: Dict[str, Any], overwrite: bool = False
+    session: Session,
+    config: Dict[str, Any],
+    overwrite: bool = False,
 ) -> Database:
+    can_write = security_manager.can_access("can_write", "Database")
     existing = session.query(Database).filter_by(uuid=config["uuid"]).first()
     if existing:
-        if not overwrite:
+        if not overwrite or not can_write:
             return existing
         config["id"] = existing.id
+    elif not can_write:
+        raise ImportFailedError(
+            "Database doesn't exist and user doesn't have permission to create databases"
+        )
 
     # https://github.com/apache/superset/pull/16756 renamed ``csv`` to ``file``.
     config["allow_file_upload"] = config.pop("allow_csv_upload")

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

@@ -28,6 +28,8 @@
 from sqlalchemy.orm.exc import MultipleResultsFound
 from sqlalchemy.sql.visitors import VisitableType
 
+from superset import security_manager
+from superset.commands.exceptions import ImportFailedError
 from superset.connectors.sqla.models import SqlaTable
 from superset.datasets.commands.exceptions import DatasetForbiddenDataURI
 from superset.models.core import Database
@@ -104,11 +106,16 @@ def import_dataset(
     overwrite: bool = False,
     force_data: bool = False,
 ) -> SqlaTable:
+    can_write = security_manager.can_access("can_write", "Dataset")
     existing = session.query(SqlaTable).filter_by(uuid=config["uuid"]).first()
     if existing:
-        if not overwrite:
+        if not overwrite or not can_write:
             return existing
         config["id"] = existing.id
+    elif not can_write:
+        raise ImportFailedError(
+            "Dataset doesn't exist and user doesn't have permission to create datasets"
+        )
 
     # TODO (betodealmeida): move this logic to import_from_dict
     config = config.copy()

superset/examples/utils.py

@@ -92,7 +92,9 @@ def load_configs_from_directory(
     contents[METADATA_FILE_NAME] = yaml.dump(metadata)
 
     command = ImportExamplesCommand(
-        contents, overwrite=overwrite, force_data=force_data
+        contents,
+        overwrite=overwrite,
+        force_data=force_data,
     )
     try:
         command.run()

tests/integration_tests/charts/commands_tests.py

@@ -164,9 +164,10 @@ def test_export_chart_command_no_related(self, mock_g):
 
 class TestImportChartsCommand(SupersetTestCase):
     @patch("superset.charts.commands.importers.v1.utils.g")
-    def test_import_v1_chart(self, mock_g):
+    @patch("superset.security.manager.g")
+    def test_import_v1_chart(self, sm_g, utils_g):
         """Test that we can import a chart"""
-        mock_g.user = security_manager.find_user("admin")
+        admin = sm_g.user = utils_g.user = security_manager.find_user("admin")
         contents = {
             "metadata.yaml": yaml.safe_dump(chart_metadata_config),
             "databases/imported_database.yaml": yaml.safe_dump(database_config),
@@ -227,7 +228,7 @@ def test_import_v1_chart(self, mock_g):
         assert database.database_name == "imported_database"
         assert chart.table.database == database
 
-        assert chart.owners == [mock_g.user]
+        assert chart.owners == [admin]
 
         chart.owners = []
         dataset.owners = []
@@ -237,8 +238,10 @@ def test_import_v1_chart(self, mock_g):
         db.session.delete(database)
         db.session.commit()
 
-    def test_import_v1_chart_multiple(self):
+    @patch("superset.security.manager.g")
+    def test_import_v1_chart_multiple(self, sm_g):
         """Test that a chart can be imported multiple times"""
+        sm_g.user = security_manager.find_user("admin")
         contents = {
             "metadata.yaml": yaml.safe_dump(chart_metadata_config),
             "databases/imported_database.yaml": yaml.safe_dump(database_config),

tests/integration_tests/dashboards/commands_tests.py

@@ -485,9 +485,10 @@ def test_import_v0_dashboard_cli_export(self):
         db.session.commit()
 
     @patch("superset.dashboards.commands.importers.v1.utils.g")
-    def test_import_v1_dashboard(self, mock_g):
+    @patch("superset.security.manager.g")
+    def test_import_v1_dashboard(self, sm_g, utils_g):
         """Test that we can import a dashboard"""
-        mock_g.user = security_manager.find_user("admin")
+        admin = sm_g.user = utils_g.user = security_manager.find_user("admin")
         contents = {
             "metadata.yaml": yaml.safe_dump(dashboard_metadata_config),
             "databases/imported_database.yaml": yaml.safe_dump(database_config),
@@ -567,7 +568,7 @@ def test_import_v1_dashboard(self, mock_g):
         database = dataset.database
         assert str(database.uuid) == database_config["uuid"]
 
-        assert dashboard.owners == [mock_g.user]
+        assert dashboard.owners == [admin]
 
         dashboard.owners = []
         chart.owners = []
@@ -579,8 +580,11 @@ def test_import_v1_dashboard(self, mock_g):
         db.session.delete(database)
         db.session.commit()
 
-    def test_import_v1_dashboard_multiple(self):
+    @patch("superset.security.manager.g")
+    def test_import_v1_dashboard_multiple(self, mock_g):
         """Test that a dashboard can be imported multiple times"""
+        mock_g.user = security_manager.find_user("admin")
+
         num_dashboards = db.session.query(Dashboard).count()
 
         contents = {