You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by dp...@apache.org on 2024/02/01 10:10:07 UTC
(superset) branch master updated: fix: dashboard import validation (#26887)
This is an automated email from the ASF dual-hosted git repository.
dpgaspar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 36ce9e26f0 fix: dashboard import validation (#26887)
36ce9e26f0 is described below
commit 36ce9e26f0da7893946d787488a30722bdb4d51b
Author: Daniel Vaz Gaspar <da...@gmail.com>
AuthorDate: Thu Feb 1 10:09:59 2024 +0000
fix: dashboard import validation (#26887)
---
superset/commands/dashboard/importers/v1/utils.py | 8 +-
.../commands/importers/v1/import_test.py | 110 +++++++++++++++++++--
2 files changed, 111 insertions(+), 7 deletions(-)
diff --git a/superset/commands/dashboard/importers/v1/utils.py b/superset/commands/dashboard/importers/v1/utils.py
index 1deb44949a..712161bc16 100644
--- a/superset/commands/dashboard/importers/v1/utils.py
+++ b/superset/commands/dashboard/importers/v1/utils.py
@@ -157,7 +157,13 @@ def import_dashboard(
)
existing = session.query(Dashboard).filter_by(uuid=config["uuid"]).first()
if existing:
- if not overwrite or not can_write:
+ if overwrite and can_write and hasattr(g, "user") and g.user:
+ if not security_manager.can_access_dashboard(existing):
+ raise ImportFailedError(
+ "A dashboard already exists and user doesn't "
+ "have permissions to overwrite it"
+ )
+ elif not overwrite or not can_write:
return existing
config["id"] = existing.id
elif not can_write:
diff --git a/tests/unit_tests/dashboards/commands/importers/v1/import_test.py b/tests/unit_tests/dashboards/commands/importers/v1/import_test.py
index 67e0897755..190f0660f5 100644
--- a/tests/unit_tests/dashboards/commands/importers/v1/import_test.py
+++ b/tests/unit_tests/dashboards/commands/importers/v1/import_test.py
@@ -23,6 +23,7 @@ from pytest_mock import MockFixture
from sqlalchemy.orm.session import Session
from superset.commands.exceptions import ImportFailedError
+from superset.utils.core import override_user
def test_import_dashboard(mocker: MockFixture, session: Session) -> None:
@@ -31,8 +32,6 @@ def test_import_dashboard(mocker: MockFixture, session: Session) -> None:
"""
from superset import security_manager
from superset.commands.dashboard.importers.v1.utils import import_dashboard
- 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 dashboard_config
@@ -48,6 +47,8 @@ def test_import_dashboard(mocker: MockFixture, session: Session) -> None:
assert dashboard.description is None
assert dashboard.is_managed_externally is False
assert dashboard.external_url is None
+ # Assert that the can write to dashboard was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
def test_import_dashboard_managed_externally(
@@ -59,8 +60,6 @@ def test_import_dashboard_managed_externally(
"""
from superset import security_manager
from superset.commands.dashboard.importers.v1.utils import import_dashboard
- 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 dashboard_config
@@ -77,6 +76,9 @@ def test_import_dashboard_managed_externally(
assert dashboard.is_managed_externally is True
assert dashboard.external_url == "https://example.org/my_dashboard"
+ # Assert that the can write to dashboard was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
+
def test_import_dashboard_without_permission(
mocker: MockFixture,
@@ -87,8 +89,6 @@ def test_import_dashboard_without_permission(
"""
from superset import security_manager
from superset.commands.dashboard.importers.v1.utils import import_dashboard
- 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 dashboard_config
@@ -105,3 +105,101 @@ def test_import_dashboard_without_permission(
str(excinfo.value)
== "Dashboard doesn't exist and user doesn't have permission to create dashboards"
)
+
+ # Assert that the can write to dashboard was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
+
+
+def test_import_existing_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.commands.dashboard.importers.v1.utils import g, import_dashboard
+ from superset.models.dashboard import Dashboard
+ 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)
+ mocker.patch.object(security_manager, "can_access_dashboard", return_value=False)
+ mock_g = mocker.patch(
+ "superset.commands.dashboard.importers.v1.utils.g"
+ ) # Replace with the actual path to g
+ mock_g.user = mocker.MagicMock(return_value=True)
+
+ engine = session.get_bind()
+ Slice.metadata.create_all(engine) # pylint: disable=no-member
+ Dashboard.metadata.create_all(engine) # pylint: disable=no-member
+
+ dashboard_obj = Dashboard(
+ id=100,
+ dashboard_title="Test dash",
+ slug=None,
+ slices=[],
+ published=True,
+ uuid="c4b28c4e-a1fe-4cf8-a5ac-d6f11d6fdd51",
+ )
+ session.add(dashboard_obj)
+ session.flush()
+ config = copy.deepcopy(dashboard_config)
+
+ with pytest.raises(ImportFailedError) as excinfo:
+ import_dashboard(session, config, overwrite=True)
+ assert (
+ str(excinfo.value)
+ == "A dashboard already exists and user doesn't have permissions to overwrite it"
+ )
+
+ # Assert that the can write to dashboard was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
+ security_manager.can_access_dashboard.assert_called_once_with(dashboard_obj)
+
+
+def test_import_existing_dashboard_with_permission(
+ mocker: MockFixture,
+ session: Session,
+) -> None:
+ """
+ Test importing a dashboard when a user doesn't have permissions to create.
+ """
+ from flask_appbuilder.security.sqla.models import Role, User
+
+ from superset import security_manager
+ from superset.commands.dashboard.importers.v1.utils import import_dashboard
+ from superset.models.dashboard import Dashboard
+ from tests.integration_tests.fixtures.importexport import dashboard_config
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "can_access_dashboard", return_value=True)
+
+ engine = session.get_bind()
+ Dashboard.metadata.create_all(engine) # pylint: disable=no-member
+
+ admin = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="adoe@example.org",
+ username="admin",
+ roles=[Role(name="Admin")],
+ )
+
+ dashboard_obj = Dashboard(
+ id=100,
+ dashboard_title="Test dash",
+ slug=None,
+ slices=[],
+ published=True,
+ uuid="c4b28c4e-a1fe-4cf8-a5ac-d6f11d6fdd51",
+ )
+ session.add(dashboard_obj)
+ session.flush()
+ config = copy.deepcopy(dashboard_config)
+
+ with override_user(admin):
+ import_dashboard(session, config, overwrite=True)
+ # Assert that the can write to dashboard was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
+ security_manager.can_access_dashboard.assert_called_once_with(dashboard_obj)