This is an automated email from the ASF dual-hosted git repository. michaelsmolina pushed a commit to branch 3.0 in repository https://gitbox.apache.org/repos/asf/superset.git
commit 9133837501f77fe5f7cda5e81fb7b2d2ab2d86b9 Author: Daniel Vaz Gaspar <[email protected]> AuthorDate: Thu Feb 1 10:09:59 2024 +0000 fix: dashboard import validation (#26887) --- superset/dashboards/commands/importers/v1/utils.py | 8 +- .../commands/importers/v1/import_test.py | 110 +++++++++++++++++++-- 2 files changed, 111 insertions(+), 7 deletions(-) diff --git a/superset/dashboards/commands/importers/v1/utils.py b/superset/dashboards/commands/importers/v1/utils.py index 1deb44949a..712161bc16 100644 --- a/superset/dashboards/commands/importers/v1/utils.py +++ b/superset/dashboards/commands/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 e07a23f6bf..95920683b7 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: @@ -30,9 +31,7 @@ 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 @@ -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( @@ -58,9 +59,7 @@ def test_import_dashboard_managed_externally( 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 @@ -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, @@ -86,9 +88,7 @@ def test_import_dashboard_without_permission( 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 @@ -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.dashboards.commands.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.dashboards.commands.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.dashboards.commands.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="[email protected]", + 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)
