This is an automated email from the ASF dual-hosted git repository. michaelsmolina pushed a commit to branch 5.0 in repository https://gitbox.apache.org/repos/asf/superset.git
commit 98c0eeccb29c7b80f050f76ce0be717070ddf328 Author: Daniel Vaz Gaspar <[email protected]> AuthorDate: Wed Mar 5 08:47:49 2025 +0000 fix: dashboard, chart and dataset import validation (#32500) (cherry picked from commit fc844d3dfdace890b32c00a507a959b81122b425) --- superset/commands/chart/importers/v1/utils.py | 7 ++- superset/commands/dashboard/importers/v1/utils.py | 7 ++- superset/commands/dataset/importers/v1/utils.py | 8 ++++ .../charts/commands/importers/v1/import_test.py | 51 ++++++++++++++++++++- .../commands/importers/v1/import_test.py | 53 +++++++++++++++++++++- .../datasets/commands/importers/v1/import_test.py | 52 +++++++++++++++++++++ 6 files changed, 171 insertions(+), 7 deletions(-) diff --git a/superset/commands/chart/importers/v1/utils.py b/superset/commands/chart/importers/v1/utils.py index 35a7f6e270..dacef7c1e3 100644 --- a/superset/commands/chart/importers/v1/utils.py +++ b/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" diff --git a/superset/commands/dashboard/importers/v1/utils.py b/superset/commands/dashboard/importers/v1/utils.py index 26cbeb0382..53904890c1 100644 --- a/superset/commands/dashboard/importers/v1/utils.py +++ b/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" diff --git a/superset/commands/dataset/importers/v1/utils.py b/superset/commands/dataset/importers/v1/utils.py index f3611e00d9..f4bfc2e079 100644 --- a/superset/commands/dataset/importers/v1/utils.py +++ b/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" diff --git a/tests/unit_tests/charts/commands/importers/v1/import_test.py b/tests/unit_tests/charts/commands/importers/v1/import_test.py index c31aeb2574..56c4658a84 100644 --- a/tests/unit_tests/charts/commands/importers/v1/import_test.py +++ b/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 ( 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 09b3eed42d..a1e4a6f87e 100644 --- a/tests/unit_tests/dashboards/commands/importers/v1/import_test.py +++ b/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 ( diff --git a/tests/unit_tests/datasets/commands/importers/v1/import_test.py b/tests/unit_tests/datasets/commands/importers/v1/import_test.py index 2a2038046d..6bba6f039d 100644 --- a/tests/unit_tests/datasets/commands/importers/v1/import_test.py +++ b/tests/unit_tests/datasets/commands/importers/v1/import_test.py @@ -24,6 +24,7 @@ from unittest.mock import Mock, patch 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 @@ from superset.commands.dataset.exceptions import ( 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", [
