fix: dashboard, chart and dataset import validation (#32500)

Author: dpgaspar

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

@@ -50,9 +50,12 @@ def import_chart(
 ) -> Slice:
     can_write = ignore_permissions or security_manager.can_access("can_write", "Chart")
     existing = db.session.query(Slice).filter_by(uuid=config["uuid"]).first()
+    user = get_user()
     if existing:
-        if overwrite and can_write and get_user():
-            if not security_manager.can_access_chart(existing):
+        if overwrite and can_write and user:
+            if not security_manager.can_access_chart(existing) or (
+                user not in existing.owners and not security_manager.is_admin()
+            ):
                 raise ImportFailedError(
                     "A chart already exists and user doesn't "
                     "have permissions to overwrite it"

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

@@ -153,9 +153,12 @@ def import_dashboard(  # noqa: C901
         "Dashboard",
     )
     existing = db.session.query(Dashboard).filter_by(uuid=config["uuid"]).first()
+    user = get_user()
     if existing:
-        if overwrite and can_write and get_user():
-            if not security_manager.can_access_dashboard(existing):
+        if overwrite and can_write and user:
+            if not security_manager.can_access_dashboard(existing) or (
+                user not in existing.owners and not security_manager.is_admin()
+            ):
                 raise ImportFailedError(
                     "A dashboard already exists and user doesn't "
                     "have permissions to overwrite it"

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

@@ -113,10 +113,18 @@ def import_dataset(  # noqa: C901
         "Dataset",
     )
     existing = db.session.query(SqlaTable).filter_by(uuid=config["uuid"]).first()
+    user = get_user()
     if existing:
+        if overwrite and can_write and user:
+            if user not in existing.owners and not security_manager.is_admin():
+                raise ImportFailedError(
+                    "A dataset already exists and user doesn't "
+                    "have permissions to overwrite it"
+                )
         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"

tests/unit_tests/charts/commands/importers/v1/import_test.py

@@ -181,7 +181,56 @@ def test_import_existing_chart_without_permission(
         .one_or_none()
     )
 
-    with override_user("admin"):
+    user = User(
+        first_name="Alice",
+        last_name="Doe",
+        email="[email protected]",
+        username="admin",
+        roles=[Role(name="Admin")],
+    )
+
+    with override_user(user):
+        with pytest.raises(ImportFailedError) as excinfo:
+            import_chart(chart_config, overwrite=True)
+        assert (
+            str(excinfo.value)
+            == "A chart already exists and user doesn't have permissions to overwrite it"  # noqa: E501
+        )
+
+    # Assert that the can write to chart was checked
+    mock_can_access.assert_called_once_with("can_write", "Chart")
+    mock_can_access_chart.assert_called_once_with(slice)
+
+
+def test_import_existing_chart_without_owner_permission(
+    mocker: MockerFixture,
+    session_with_data: Session,
+) -> None:
+    """
+    Test importing a chart when a user doesn't have permissions to modify.
+    """
+    mock_can_access = mocker.patch.object(
+        security_manager, "can_access", return_value=True
+    )
+    mock_can_access_chart = mocker.patch.object(
+        security_manager, "can_access_chart", return_value=True
+    )
+
+    slice = (
+        session_with_data.query(Slice)
+        .filter(Slice.uuid == chart_config["uuid"])
+        .one_or_none()
+    )
+
+    user = User(
+        first_name="Alice",
+        last_name="Doe",
+        email="[email protected]",
+        username="admin",
+        roles=[Role(name="Gamma")],
+    )
+
+    with override_user(user):
         with pytest.raises(ImportFailedError) as excinfo:
             import_chart(chart_config, overwrite=True)
         assert (

tests/unit_tests/dashboards/commands/importers/v1/import_test.py

@@ -122,7 +122,7 @@ def test_import_dashboard_without_permission(
     mock_can_access.assert_called_once_with("can_write", "Dashboard")
 
 
-def test_import_existing_dashboard_without_permission(
+def test_import_existing_dashboard_without_access_permission(
     mocker: MockerFixture,
     session_with_data: Session,
 ) -> None:
@@ -142,7 +142,56 @@ def test_import_existing_dashboard_without_permission(
         .one_or_none()
     )
 
-    with override_user("admin"):
+    admin = User(
+        first_name="Alice",
+        last_name="Doe",
+        email="[email protected]",
+        username="admin",
+        roles=[Role(name="Admin")],
+    )
+
+    with override_user(admin):
+        with pytest.raises(ImportFailedError) as excinfo:
+            import_dashboard(dashboard_config, overwrite=True)
+        assert (
+            str(excinfo.value)
+            == "A dashboard already exists and user doesn't have permissions to overwrite it"  # noqa: E501
+        )
+
+    # Assert that the can write to dashboard was checked
+    mock_can_access.assert_called_once_with("can_write", "Dashboard")
+    mock_can_access_dashboard.assert_called_once_with(dashboard)
+
+
+def test_import_existing_dashboard_without_owner_permission(
+    mocker: MockerFixture,
+    session_with_data: Session,
+) -> None:
+    """
+    Test importing a dashboard when a user doesn't have ownership and is not an Admin.
+    """
+    mock_can_access = mocker.patch.object(
+        security_manager, "can_access", return_value=True
+    )
+    mock_can_access_dashboard = mocker.patch.object(
+        security_manager, "can_access_dashboard", return_value=True
+    )
+
+    dashboard = (
+        session_with_data.query(Dashboard)
+        .filter(Dashboard.uuid == dashboard_config["uuid"])
+        .one_or_none()
+    )
+
+    user = User(
+        first_name="Alice",
+        last_name="Doe",
+        email="[email protected]",
+        username="admin",
+        roles=[Role(name="Gamma")],
+    )
+
+    with override_user(user):
         with pytest.raises(ImportFailedError) as excinfo:
             import_dashboard(dashboard_config, overwrite=True)
         assert (

tests/unit_tests/datasets/commands/importers/v1/import_test.py

@@ -24,6 +24,7 @@
 
 import pytest
 from flask import current_app
+from flask_appbuilder.security.sqla.models import Role, User
 from pytest_mock import MockerFixture
 from sqlalchemy.orm.session import Session
 
@@ -32,7 +33,9 @@
     DatasetForbiddenDataURI,
 )
 from superset.commands.dataset.importers.v1.utils import validate_data_uri
+from superset.commands.exceptions import ImportFailedError
 from superset.utils import json
+from superset.utils.core import override_user
 
 
 def test_import_dataset(mocker: MockerFixture, session: Session) -> None:
@@ -536,6 +539,55 @@ def test_import_dataset_managed_externally(
     assert sqla_table.external_url == "https://example.org/my_table"
 
 
+def test_import_dataset_without_owner_permission(
+    mocker: MockerFixture,
+    session: Session,
+) -> None:
+    """
+    Test importing a dataset that is managed externally.
+    """
+    from superset import security_manager
+    from superset.commands.dataset.importers.v1.utils import import_dataset
+    from superset.connectors.sqla.models import SqlaTable
+    from superset.models.core import Database
+    from tests.integration_tests.fixtures.importexport import dataset_config
+
+    mock_can_access = mocker.patch.object(
+        security_manager, "can_access", return_value=True
+    )
+
+    engine = db.session.get_bind()
+    SqlaTable.metadata.create_all(engine)  # pylint: disable=no-member
+
+    database = Database(database_name="my_database", sqlalchemy_uri="sqlite://")
+    db.session.add(database)
+    db.session.flush()
+
+    config = copy.deepcopy(dataset_config)
+    config["database_id"] = database.id
+
+    import_dataset(config)
+    user = User(
+        first_name="Alice",
+        last_name="Doe",
+        email="[email protected]",
+        username="admin",
+        roles=[Role(name="Gamma")],
+    )
+
+    with override_user(user):
+        with pytest.raises(ImportFailedError) as excinfo:
+            import_dataset(config, overwrite=True)
+
+        assert (
+            str(excinfo.value)
+            == "A dataset already exists and user doesn't have permissions to overwrite it"  # noqa: E501
+        )
+
+    # Assert that the can write to chart was checked
+    mock_can_access.assert_called_with("can_write", "Dataset")
+
+
 @pytest.mark.parametrize(
     "allowed_urls, data_uri, expected, exception_class",
     [