fix: permission checks on import

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

@@ -21,17 +21,26 @@
 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()
+    print("BETO")
+    print(can_write, existing)
     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/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,7 +28,9 @@
 from sqlalchemy.orm.exc import MultipleResultsFound
 from sqlalchemy.sql.visitors import VisitableType
 
+from superset import security_manager
 from superset.connectors.sqla.models import SqlaTable
+from superset.commands.exceptions import ImportFailedError
 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()

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

@@ -18,19 +18,26 @@
 
 import copy
 
+import pytest
+from pytest_mock import MockFixture
 from sqlalchemy.orm.session import Session
 
+from superset.commands.exceptions import ImportFailedError
 
-def test_import_chart(session: Session) -> None:
+
+def test_import_chart(mocker: MockFixture, session: Session) -> None:
     """
     Test importing a chart.
     """
+    from superset import security_manager
     from superset.charts.commands.importers.v1.utils import import_chart
     from superset.connectors.sqla.models import SqlaTable
     from superset.models.core import Database
     from superset.models.slice import Slice
     from tests.integration_tests.fixtures.importexport import chart_config
 
+    mocker.patch.object(security_manager, "can_access", return_value=True)
+
     engine = session.get_bind()
     Slice.metadata.create_all(engine)  # pylint: disable=no-member
 
@@ -45,16 +52,19 @@ def test_import_chart(session: Session) -> None:
     assert chart.external_url is None
 
 
-def test_import_chart_managed_externally(session: Session) -> None:
+def test_import_chart_managed_externally(mocker: MockFixture, session: Session) -> None:
     """
     Test importing a chart that is managed externally.
     """
+    from superset import security_manager
     from superset.charts.commands.importers.v1.utils import import_chart
     from superset.connectors.sqla.models import SqlaTable
     from superset.models.core import Database
     from superset.models.slice import Slice
     from tests.integration_tests.fixtures.importexport import chart_config
 
+    mocker.patch.object(security_manager, "can_access", return_value=True)
+
     engine = session.get_bind()
     Slice.metadata.create_all(engine)  # pylint: disable=no-member
 
@@ -67,3 +77,34 @@ def test_import_chart_managed_externally(session: Session) -> None:
     chart = import_chart(session, config)
     assert chart.is_managed_externally is True
     assert chart.external_url == "https://example.org/my_chart"
+
+
+def test_import_chart_without_permission(
+    mocker: MockFixture,
+    session: Session,
+) -> None:
+    """
+    Test importing a chart when a user doesn't have permissions to create.
+    """
+    from superset import security_manager
+    from superset.charts.commands.importers.v1.utils import import_chart
+    from superset.connectors.sqla.models import SqlaTable
+    from superset.models.core import Database
+    from superset.models.slice import Slice
+    from tests.integration_tests.fixtures.importexport import chart_config
+
+    mocker.patch.object(security_manager, "can_access", return_value=False)
+
+    engine = session.get_bind()
+    Slice.metadata.create_all(engine)  # pylint: disable=no-member
+
+    config = copy.deepcopy(chart_config)
+    config["datasource_id"] = 1
+    config["datasource_type"] = "table"
+
+    with pytest.raises(ImportFailedError) as excinfo:
+        import_chart(session, config)
+    assert (
+        str(excinfo.value)
+        == "Chart doesn't exist and user doesn't have permission to create charts"
+    )

tests/unit_tests/commands/importers/v1/assets_test.py

@@ -17,6 +17,7 @@
 
 import copy
 
+from pytest_mock import MockFixture
 from sqlalchemy.orm.session import Session
 from sqlalchemy.sql import select
 
@@ -30,14 +31,17 @@
 )
 
 
-def test_import_new_assets(session: Session) -> None:
+def test_import_new_assets(mocker: MockFixture, session: Session) -> None:
     """
     Test that all new assets are imported correctly.
     """
+    from superset import security_manager
     from superset.commands.importers.v1.assets import ImportAssetsCommand
     from superset.models.dashboard import dashboard_slices
     from superset.models.slice import Slice
 
+    mocker.patch.object(security_manager, "can_access", return_value=True)
+
     engine = session.get_bind()
     Slice.metadata.create_all(engine)  # pylint: disable=no-member
     configs = {
@@ -59,14 +63,17 @@ def test_import_new_assets(session: Session) -> None:
     assert len(dashboard_ids) == expected_number_of_dashboards
 
 
-def test_import_adds_dashboard_charts(session: Session) -> None:
+def test_import_adds_dashboard_charts(mocker: MockFixture, session: Session) -> None:
     """
     Test that existing dashboards are updated with new charts.
     """
+    from superset import security_manager
     from superset.commands.importers.v1.assets import ImportAssetsCommand
     from superset.models.dashboard import dashboard_slices
     from superset.models.slice import Slice
 
+    mocker.patch.object(security_manager, "can_access", return_value=True)
+
     engine = session.get_bind()
     Slice.metadata.create_all(engine)  # pylint: disable=no-member
     base_configs = {
@@ -95,14 +102,17 @@ def test_import_adds_dashboard_charts(session: Session) -> None:
     assert len(dashboard_ids) == expected_number_of_dashboards
 
 
-def test_import_removes_dashboard_charts(session: Session) -> None:
+def test_import_removes_dashboard_charts(mocker: MockFixture, session: Session) -> None:
     """
     Test that existing dashboards are updated without old charts.
     """
+    from superset import security_manager
     from superset.commands.importers.v1.assets import ImportAssetsCommand
     from superset.models.dashboard import dashboard_slices
     from superset.models.slice import Slice
 
+    mocker.patch.object(security_manager, "can_access", return_value=True)
+
     engine = session.get_bind()
     Slice.metadata.create_all(engine)  # pylint: disable=no-member
     base_configs = {

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

@@ -18,19 +18,26 @@
 
 import copy
 
+import pytest
+from pytest_mock import MockFixture
 from sqlalchemy.orm.session import Session
 
+from superset.commands.exceptions import ImportFailedError
 
-def test_import_dashboard(session: Session) -> None:
+
+def test_import_dashboard(mocker: MockFixture, session: Session) -> None:
     """
     Test importing a dashboard.
     """
+    from superset import security_manager
     from superset.connectors.sqla.models import SqlaTable
     from superset.dashboards.commands.importers.v1.utils import import_dashboard
     from superset.models.core import Database
     from superset.models.slice import Slice
     from tests.integration_tests.fixtures.importexport import dashboard_config
 
+    mocker.patch.object(security_manager, "can_access", return_value=True)
+
     engine = session.get_bind()
     Slice.metadata.create_all(engine)  # pylint: disable=no-member
 
@@ -43,16 +50,22 @@ def test_import_dashboard(session: Session) -> None:
     assert dashboard.external_url is None
 
 
-def test_import_dashboard_managed_externally(session: Session) -> None:
+def test_import_dashboard_managed_externally(
+    mocker: MockFixture,
+    session: Session,
+) -> None:
     """
     Test importing a dashboard that is managed externally.
     """
+    from superset import security_manager
     from superset.connectors.sqla.models import SqlaTable
     from superset.dashboards.commands.importers.v1.utils import import_dashboard
     from superset.models.core import Database
     from superset.models.slice import Slice
     from tests.integration_tests.fixtures.importexport import dashboard_config
 
+    mocker.patch.object(security_manager, "can_access", return_value=True)
+
     engine = session.get_bind()
     Slice.metadata.create_all(engine)  # pylint: disable=no-member
 
@@ -63,3 +76,32 @@ def test_import_dashboard_managed_externally(session: Session) -> None:
     dashboard = import_dashboard(session, config)
     assert dashboard.is_managed_externally is True
     assert dashboard.external_url == "https://example.org/my_dashboard"
+
+
+def test_import_dashboard_without_permission(
+    mocker: MockFixture,
+    session: Session,
+) -> None:
+    """
+    Test importing a dashboard when a user doesn't have permissions to create.
+    """
+    from superset import security_manager
+    from superset.connectors.sqla.models import SqlaTable
+    from superset.dashboards.commands.importers.v1.utils import import_dashboard
+    from superset.models.core import Database
+    from superset.models.slice import Slice
+    from tests.integration_tests.fixtures.importexport import dashboard_config
+
+    mocker.patch.object(security_manager, "can_access", return_value=False)
+
+    engine = session.get_bind()
+    Slice.metadata.create_all(engine)  # pylint: disable=no-member
+
+    config = copy.deepcopy(dashboard_config)
+
+    with pytest.raises(ImportFailedError) as excinfo:
+        import_dashboard(session, config)
+    assert (
+        str(excinfo.value)
+        == "Dashboard doesn't exist and user doesn't have permission to create dashboards"
+    )