This is an automated email from the ASF dual-hosted git repository. elizabeth pushed a commit to branch 2.1 in repository https://gitbox.apache.org/repos/asf/superset.git
commit 1f745e3790a979bf7bf574b994daf650a82e0f51 Author: Beto Dealmeida <[email protected]> AuthorDate: Wed Mar 15 08:31:09 2023 -0700 fix: permission checks on import (#23200) --- superset/charts/commands/importers/v1/utils.py | 9 +- superset/commands/importers/v1/examples.py | 21 +- superset/dashboards/commands/importers/v1/utils.py | 10 +- superset/databases/commands/importers/v1/utils.py | 14 +- superset/datasets/commands/importers/v1/utils.py | 9 +- superset/examples/utils.py | 4 +- tests/integration_tests/charts/commands_tests.py | 11 +- .../integration_tests/dashboards/commands_tests.py | 12 +- .../integration_tests/databases/commands_tests.py | 303 +++++++++++++++++---- tests/integration_tests/datasets/commands_tests.py | 17 +- .../queries/saved_queries/commands_tests.py | 10 +- .../charts/commands/importers/v1/import_test.py | 45 ++- .../commands/importers/v1/assets_test.py | 16 +- .../commands/importers/v1/import_test.py | 46 +++- .../databases/commands/importers/v1/import_test.py | 44 ++- .../datasets/commands/importers/v1/import_test.py | 38 ++- 16 files changed, 511 insertions(+), 98 deletions(-) diff --git a/superset/charts/commands/importers/v1/utils.py b/superset/charts/commands/importers/v1/utils.py index d4c69d3ada..cea8af7b4c 100644 --- a/superset/charts/commands/importers/v1/utils.py +++ b/superset/charts/commands/importers/v1/utils.py @@ -21,17 +21,24 @@ from typing import Any, Dict 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() 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"]) diff --git a/superset/commands/importers/v1/examples.py b/superset/commands/importers/v1/examples.py index 99aa831faa..8d14c8298a 100644 --- a/superset/commands/importers/v1/examples.py +++ b/superset/commands/importers/v1/examples.py @@ -21,7 +21,7 @@ from sqlalchemy.orm import Session from sqlalchemy.orm.exc import MultipleResultsFound from sqlalchemy.sql import select -from superset import db +from superset import db, security_manager from superset.charts.commands.importers.v1 import ImportChartsCommand from superset.charts.commands.importers.v1.utils import import_chart from superset.charts.schemas import ImportV1ChartSchema @@ -42,7 +42,7 @@ from superset.datasets.commands.importers.v1 import ImportDatasetsCommand from superset.datasets.commands.importers.v1.utils import import_dataset from superset.datasets.schemas import ImportV1DatasetSchema from superset.models.dashboard import dashboard_slices -from superset.utils.core import get_example_default_schema +from superset.utils.core import get_example_default_schema, override_user from superset.utils.database import get_example_database @@ -69,7 +69,13 @@ class ImportExamplesCommand(ImportModelsCommand): # rollback to prevent partial imports try: - self._import(db.session, self._configs, self.overwrite, self.force_data) + with override_user(security_manager.find_user(username="admin")): + self._import( + db.session, + self._configs, + self.overwrite, + self.force_data, + ) db.session.commit() except Exception as ex: db.session.rollback() @@ -119,13 +125,12 @@ class ImportExamplesCommand(ImportModelsCommand): if config["schema"] is None: config["schema"] = get_example_default_schema() - dataset = import_dataset( - session, config, overwrite=overwrite, force_data=force_data - ) - try: dataset = import_dataset( - session, config, overwrite=overwrite, force_data=force_data + session, + config, + overwrite=overwrite, + force_data=force_data, ) except MultipleResultsFound: # Multiple result can be found for datasets. There was a bug in diff --git a/superset/dashboards/commands/importers/v1/utils.py b/superset/dashboards/commands/importers/v1/utils.py index 513d1efcdb..30bad57cb1 100644 --- a/superset/dashboards/commands/importers/v1/utils.py +++ b/superset/dashboards/commands/importers/v1/utils.py @@ -22,6 +22,8 @@ from typing import Any, Dict, Set 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() diff --git a/superset/databases/commands/importers/v1/utils.py b/superset/databases/commands/importers/v1/utils.py index 6704ccd465..4c82ec3ef4 100644 --- a/superset/databases/commands/importers/v1/utils.py +++ b/superset/databases/commands/importers/v1/utils.py @@ -20,17 +20,27 @@ from typing import Any, Dict 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") diff --git a/superset/datasets/commands/importers/v1/utils.py b/superset/datasets/commands/importers/v1/utils.py index b3fb2a8041..2363cf7497 100644 --- a/superset/datasets/commands/importers/v1/utils.py +++ b/superset/datasets/commands/importers/v1/utils.py @@ -28,6 +28,8 @@ from sqlalchemy.orm import Session from sqlalchemy.orm.exc import MultipleResultsFound from sqlalchemy.sql.visitors import VisitableType +from superset import security_manager +from superset.commands.exceptions import ImportFailedError from superset.connectors.sqla.models import SqlaTable 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() diff --git a/superset/examples/utils.py b/superset/examples/utils.py index 4568e4f196..aea1f0f93d 100644 --- a/superset/examples/utils.py +++ b/superset/examples/utils.py @@ -92,7 +92,9 @@ def load_configs_from_directory( contents[METADATA_FILE_NAME] = yaml.dump(metadata) command = ImportExamplesCommand( - contents, overwrite=overwrite, force_data=force_data + contents, + overwrite=overwrite, + force_data=force_data, ) try: command.run() diff --git a/tests/integration_tests/charts/commands_tests.py b/tests/integration_tests/charts/commands_tests.py index da9a7550ac..fb2073499b 100644 --- a/tests/integration_tests/charts/commands_tests.py +++ b/tests/integration_tests/charts/commands_tests.py @@ -164,9 +164,10 @@ class TestExportChartsCommand(SupersetTestCase): class TestImportChartsCommand(SupersetTestCase): @patch("superset.charts.commands.importers.v1.utils.g") - def test_import_v1_chart(self, mock_g): + @patch("superset.security.manager.g") + def test_import_v1_chart(self, sm_g, utils_g): """Test that we can import a chart""" - mock_g.user = security_manager.find_user("admin") + admin = sm_g.user = utils_g.user = security_manager.find_user("admin") contents = { "metadata.yaml": yaml.safe_dump(chart_metadata_config), "databases/imported_database.yaml": yaml.safe_dump(database_config), @@ -227,7 +228,7 @@ class TestImportChartsCommand(SupersetTestCase): assert database.database_name == "imported_database" assert chart.table.database == database - assert chart.owners == [mock_g.user] + assert chart.owners == [admin] chart.owners = [] dataset.owners = [] @@ -237,8 +238,10 @@ class TestImportChartsCommand(SupersetTestCase): db.session.delete(database) db.session.commit() - def test_import_v1_chart_multiple(self): + @patch("superset.security.manager.g") + def test_import_v1_chart_multiple(self, sm_g): """Test that a chart can be imported multiple times""" + sm_g.user = security_manager.find_user("admin") contents = { "metadata.yaml": yaml.safe_dump(chart_metadata_config), "databases/imported_database.yaml": yaml.safe_dump(database_config), diff --git a/tests/integration_tests/dashboards/commands_tests.py b/tests/integration_tests/dashboards/commands_tests.py index 927865e4a0..ad9152585e 100644 --- a/tests/integration_tests/dashboards/commands_tests.py +++ b/tests/integration_tests/dashboards/commands_tests.py @@ -485,9 +485,10 @@ class TestImportDashboardsCommand(SupersetTestCase): db.session.commit() @patch("superset.dashboards.commands.importers.v1.utils.g") - def test_import_v1_dashboard(self, mock_g): + @patch("superset.security.manager.g") + def test_import_v1_dashboard(self, sm_g, utils_g): """Test that we can import a dashboard""" - mock_g.user = security_manager.find_user("admin") + admin = sm_g.user = utils_g.user = security_manager.find_user("admin") contents = { "metadata.yaml": yaml.safe_dump(dashboard_metadata_config), "databases/imported_database.yaml": yaml.safe_dump(database_config), @@ -570,7 +571,7 @@ class TestImportDashboardsCommand(SupersetTestCase): database = dataset.database assert str(database.uuid) == database_config["uuid"] - assert dashboard.owners == [mock_g.user] + assert dashboard.owners == [admin] dashboard.owners = [] chart.owners = [] @@ -582,8 +583,11 @@ class TestImportDashboardsCommand(SupersetTestCase): db.session.delete(database) db.session.commit() - def test_import_v1_dashboard_multiple(self): + @patch("superset.security.manager.g") + def test_import_v1_dashboard_multiple(self, mock_g): """Test that a dashboard can be imported multiple times""" + mock_g.user = security_manager.find_user("admin") + num_dashboards = db.session.query(Dashboard).count() contents = { diff --git a/tests/integration_tests/databases/commands_tests.py b/tests/integration_tests/databases/commands_tests.py index 7e4fcaad78..0ba60da1d9 100644 --- a/tests/integration_tests/databases/commands_tests.py +++ b/tests/integration_tests/databases/commands_tests.py @@ -69,10 +69,8 @@ from tests.integration_tests.fixtures.importexport import ( class TestCreateDatabaseCommand(SupersetTestCase): - @mock.patch( - "superset.databases.commands.test_connection.event_logger.log_with_context" - ) - @mock.patch("superset.utils.core.g") + @patch("superset.databases.commands.test_connection.event_logger.log_with_context") + @patch("superset.utils.core.g") def test_create_duplicate_error(self, mock_g, mock_logger): example_db = get_example_database() mock_g.user = security_manager.find_user("admin") @@ -90,10 +88,8 @@ class TestCreateDatabaseCommand(SupersetTestCase): "DatabaseRequiredFieldValidationError" ) - @mock.patch( - "superset.databases.commands.test_connection.event_logger.log_with_context" - ) - @mock.patch("superset.utils.core.g") + @patch("superset.databases.commands.test_connection.event_logger.log_with_context") + @patch("superset.utils.core.g") def test_multiple_error_logging(self, mock_g, mock_logger): mock_g.user = security_manager.find_user("admin") command = CreateDatabaseCommand({}) @@ -395,8 +391,11 @@ class TestExportDatabasesCommand(SupersetTestCase): class TestImportDatabasesCommand(SupersetTestCase): - def test_import_v1_database(self): + @patch("superset.security.manager.g") + def test_import_v1_database(self, mock_g): """Test that a database can be imported""" + mock_g.user = security_manager.find_user("admin") + contents = { "metadata.yaml": yaml.safe_dump(database_metadata_config), "databases/imported_database.yaml": yaml.safe_dump(database_config), @@ -421,7 +420,8 @@ class TestImportDatabasesCommand(SupersetTestCase): db.session.delete(database) db.session.commit() - def test_import_v1_database_broken_csv_fields(self): + @patch("superset.security.manager.g") + def test_import_v1_database_broken_csv_fields(self, mock_g): """ Test that a database can be imported with broken schema. @@ -429,6 +429,8 @@ class TestImportDatabasesCommand(SupersetTestCase): the V1 schema. This test ensures that we can import databases that were exported with the broken schema. """ + mock_g.user = security_manager.find_user("admin") + broken_config = database_config.copy() broken_config["allow_file_upload"] = broken_config.pop("allow_csv_upload") broken_config["extra"] = {"schemas_allowed_for_file_upload": ["upload"]} @@ -457,8 +459,11 @@ class TestImportDatabasesCommand(SupersetTestCase): db.session.delete(database) db.session.commit() - def test_import_v1_database_multiple(self): + @patch("superset.security.manager.g") + def test_import_v1_database_multiple(self, mock_g): """Test that a database can be imported multiple times""" + mock_g.user = security_manager.find_user("admin") + num_databases = db.session.query(Database).count() contents = { @@ -498,8 +503,11 @@ class TestImportDatabasesCommand(SupersetTestCase): db.session.delete(database) db.session.commit() - def test_import_v1_database_with_dataset(self): + @patch("superset.security.manager.g") + def test_import_v1_database_with_dataset(self, mock_g): """Test that a database can be imported with datasets""" + mock_g.user = security_manager.find_user("admin") + contents = { "databases/imported_database.yaml": yaml.safe_dump(database_config), "datasets/imported_dataset.yaml": yaml.safe_dump(dataset_config), @@ -518,8 +526,11 @@ class TestImportDatabasesCommand(SupersetTestCase): db.session.delete(database) db.session.commit() - def test_import_v1_database_with_dataset_multiple(self): + @patch("superset.security.manager.g") + def test_import_v1_database_with_dataset_multiple(self, mock_g): """Test that a database can be imported multiple times w/o changing datasets""" + mock_g.user = security_manager.find_user("admin") + contents = { "databases/imported_database.yaml": yaml.safe_dump(database_config), "datasets/imported_dataset.yaml": yaml.safe_dump(dataset_config), @@ -623,6 +634,200 @@ class TestImportDatabasesCommand(SupersetTestCase): } } + @patch("superset.databases.schemas.is_feature_enabled") + def test_import_v1_database_masked_ssh_tunnel_password( + self, mock_schema_is_feature_enabled + ): + """Test that database imports with masked ssh_tunnel passwords are rejected""" + mock_schema_is_feature_enabled.return_value = True + masked_database_config = database_with_ssh_tunnel_config_password.copy() + contents = { + "metadata.yaml": yaml.safe_dump(database_metadata_config), + "databases/imported_database.yaml": yaml.safe_dump(masked_database_config), + } + command = ImportDatabasesCommand(contents) + with pytest.raises(CommandInvalidError) as excinfo: + command.run() + assert str(excinfo.value) == "Error importing database" + assert excinfo.value.normalized_messages() == { + "databases/imported_database.yaml": { + "_schema": ["Must provide a password for the ssh tunnel"] + } + } + + @patch("superset.databases.schemas.is_feature_enabled") + def test_import_v1_database_masked_ssh_tunnel_private_key_and_password( + self, mock_schema_is_feature_enabled + ): + """Test that database imports with masked ssh_tunnel private_key and private_key_password are rejected""" + mock_schema_is_feature_enabled.return_value = True + masked_database_config = database_with_ssh_tunnel_config_private_key.copy() + contents = { + "metadata.yaml": yaml.safe_dump(database_metadata_config), + "databases/imported_database.yaml": yaml.safe_dump(masked_database_config), + } + command = ImportDatabasesCommand(contents) + with pytest.raises(CommandInvalidError) as excinfo: + command.run() + assert str(excinfo.value) == "Error importing database" + assert excinfo.value.normalized_messages() == { + "databases/imported_database.yaml": { + "_schema": [ + "Must provide a private key for the ssh tunnel", + "Must provide a private key password for the ssh tunnel", + ] + } + } + + @patch("superset.databases.schemas.is_feature_enabled") + @patch("superset.security.manager.g") + def test_import_v1_database_with_ssh_tunnel_password( + self, + mock_g, + mock_schema_is_feature_enabled, + ): + """Test that a database with ssh_tunnel password can be imported""" + mock_g.user = security_manager.find_user("admin") + mock_schema_is_feature_enabled.return_value = True + masked_database_config = database_with_ssh_tunnel_config_password.copy() + masked_database_config["ssh_tunnel"]["password"] = "TEST" + contents = { + "metadata.yaml": yaml.safe_dump(database_metadata_config), + "databases/imported_database.yaml": yaml.safe_dump(masked_database_config), + } + command = ImportDatabasesCommand(contents) + command.run() + + database = ( + db.session.query(Database).filter_by(uuid=database_config["uuid"]).one() + ) + assert database.allow_file_upload + assert database.allow_ctas + assert database.allow_cvas + assert database.allow_dml + assert not database.allow_run_async + assert database.cache_timeout is None + assert database.database_name == "imported_database" + assert database.expose_in_sqllab + assert database.extra == "{}" + assert database.sqlalchemy_uri == "sqlite:///test.db" + + model_ssh_tunnel = ( + db.session.query(SSHTunnel) + .filter(SSHTunnel.database_id == database.id) + .one() + ) + self.assertEqual(model_ssh_tunnel.password, "TEST") + + db.session.delete(database) + db.session.commit() + + @patch("superset.databases.schemas.is_feature_enabled") + @patch("superset.security.manager.g") + def test_import_v1_database_with_ssh_tunnel_private_key_and_password( + self, + mock_g, + mock_schema_is_feature_enabled, + ): + """Test that a database with ssh_tunnel private_key and private_key_password can be imported""" + mock_g.user = security_manager.find_user("admin") + + mock_schema_is_feature_enabled.return_value = True + masked_database_config = database_with_ssh_tunnel_config_private_key.copy() + masked_database_config["ssh_tunnel"]["private_key"] = "TestPrivateKey" + masked_database_config["ssh_tunnel"]["private_key_password"] = "TEST" + contents = { + "metadata.yaml": yaml.safe_dump(database_metadata_config), + "databases/imported_database.yaml": yaml.safe_dump(masked_database_config), + } + command = ImportDatabasesCommand(contents) + command.run() + + database = ( + db.session.query(Database).filter_by(uuid=database_config["uuid"]).one() + ) + assert database.allow_file_upload + assert database.allow_ctas + assert database.allow_cvas + assert database.allow_dml + assert not database.allow_run_async + assert database.cache_timeout is None + assert database.database_name == "imported_database" + assert database.expose_in_sqllab + assert database.extra == "{}" + assert database.sqlalchemy_uri == "sqlite:///test.db" + + model_ssh_tunnel = ( + db.session.query(SSHTunnel) + .filter(SSHTunnel.database_id == database.id) + .one() + ) + self.assertEqual(model_ssh_tunnel.private_key, "TestPrivateKey") + self.assertEqual(model_ssh_tunnel.private_key_password, "TEST") + + db.session.delete(database) + db.session.commit() + + @patch("superset.databases.schemas.is_feature_enabled") + def test_import_v1_database_masked_ssh_tunnel_no_credentials( + self, mock_schema_is_feature_enabled + ): + """Test that databases with ssh_tunnels that have no credentials are rejected""" + mock_schema_is_feature_enabled.return_value = True + masked_database_config = database_with_ssh_tunnel_config_no_credentials.copy() + contents = { + "metadata.yaml": yaml.safe_dump(database_metadata_config), + "databases/imported_database.yaml": yaml.safe_dump(masked_database_config), + } + command = ImportDatabasesCommand(contents) + with pytest.raises(CommandInvalidError) as excinfo: + command.run() + assert str(excinfo.value) == "Must provide credentials for the SSH Tunnel" + + @patch("superset.databases.schemas.is_feature_enabled") + def test_import_v1_database_masked_ssh_tunnel_multiple_credentials( + self, mock_schema_is_feature_enabled + ): + """Test that databases with ssh_tunnels that have multiple credentials are rejected""" + mock_schema_is_feature_enabled.return_value = True + masked_database_config = database_with_ssh_tunnel_config_mix_credentials.copy() + contents = { + "metadata.yaml": yaml.safe_dump(database_metadata_config), + "databases/imported_database.yaml": yaml.safe_dump(masked_database_config), + } + command = ImportDatabasesCommand(contents) + with pytest.raises(CommandInvalidError) as excinfo: + command.run() + assert ( + str(excinfo.value) == "Cannot have multiple credentials for the SSH Tunnel" + ) + + @patch("superset.databases.schemas.is_feature_enabled") + def test_import_v1_database_masked_ssh_tunnel_only_priv_key_psswd( + self, mock_schema_is_feature_enabled + ): + """Test that databases with ssh_tunnels that have multiple credentials are rejected""" + mock_schema_is_feature_enabled.return_value = True + masked_database_config = ( + database_with_ssh_tunnel_config_private_pass_only.copy() + ) + contents = { + "metadata.yaml": yaml.safe_dump(database_metadata_config), + "databases/imported_database.yaml": yaml.safe_dump(masked_database_config), + } + command = ImportDatabasesCommand(contents) + with pytest.raises(CommandInvalidError) as excinfo: + command.run() + assert str(excinfo.value) == "Error importing database" + assert excinfo.value.normalized_messages() == { + "databases/imported_database.yaml": { + "_schema": [ + "Must provide a private key for the ssh tunnel", + "Must provide a private key password for the ssh tunnel", + ] + } + } + @patch("superset.databases.commands.importers.v1.import_dataset") def test_import_v1_rollback(self, mock_import_dataset): """Test than on an exception everything is rolled back""" @@ -648,11 +853,9 @@ class TestImportDatabasesCommand(SupersetTestCase): class TestTestConnectionDatabaseCommand(SupersetTestCase): - @mock.patch("superset.databases.dao.Database._get_sqla_engine") - @mock.patch( - "superset.databases.commands.test_connection.event_logger.log_with_context" - ) - @mock.patch("superset.utils.core.g") + @patch("superset.databases.dao.Database._get_sqla_engine") + @patch("superset.databases.commands.test_connection.event_logger.log_with_context") + @patch("superset.utils.core.g") def test_connection_db_exception( self, mock_g, mock_event_logger, mock_get_sqla_engine ): @@ -671,11 +874,9 @@ class TestTestConnectionDatabaseCommand(SupersetTestCase): ) mock_event_logger.assert_called() - @mock.patch("superset.databases.dao.Database._get_sqla_engine") - @mock.patch( - "superset.databases.commands.test_connection.event_logger.log_with_context" - ) - @mock.patch("superset.utils.core.g") + @patch("superset.databases.dao.Database._get_sqla_engine") + @patch("superset.databases.commands.test_connection.event_logger.log_with_context") + @patch("superset.utils.core.g") def test_connection_do_ping_exception( self, mock_g, mock_event_logger, mock_get_sqla_engine ): @@ -696,11 +897,9 @@ class TestTestConnectionDatabaseCommand(SupersetTestCase): == SupersetErrorType.GENERIC_DB_ENGINE_ERROR ) - @mock.patch("superset.databases.commands.test_connection.func_timeout") - @mock.patch( - "superset.databases.commands.test_connection.event_logger.log_with_context" - ) - @mock.patch("superset.utils.core.g") + @patch("superset.databases.commands.test_connection.func_timeout") + @patch("superset.databases.commands.test_connection.event_logger.log_with_context") + @patch("superset.utils.core.g") def test_connection_do_ping_timeout( self, mock_g, mock_event_logger, mock_func_timeout ): @@ -720,11 +919,9 @@ class TestTestConnectionDatabaseCommand(SupersetTestCase): == SupersetErrorType.CONNECTION_DATABASE_TIMEOUT ) - @mock.patch("superset.databases.dao.Database._get_sqla_engine") - @mock.patch( - "superset.databases.commands.test_connection.event_logger.log_with_context" - ) - @mock.patch("superset.utils.core.g") + @patch("superset.databases.dao.Database._get_sqla_engine") + @patch("superset.databases.commands.test_connection.event_logger.log_with_context") + @patch("superset.utils.core.g") def test_connection_superset_security_connection( self, mock_g, mock_event_logger, mock_get_sqla_engine ): @@ -745,11 +942,9 @@ class TestTestConnectionDatabaseCommand(SupersetTestCase): mock_event_logger.assert_called() - @mock.patch("superset.databases.dao.Database._get_sqla_engine") - @mock.patch( - "superset.databases.commands.test_connection.event_logger.log_with_context" - ) - @mock.patch("superset.utils.core.g") + @patch("superset.databases.dao.Database._get_sqla_engine") + @patch("superset.databases.commands.test_connection.event_logger.log_with_context") + @patch("superset.utils.core.g") def test_connection_db_api_exc( self, mock_g, mock_event_logger, mock_get_sqla_engine ): @@ -772,9 +967,9 @@ class TestTestConnectionDatabaseCommand(SupersetTestCase): mock_event_logger.assert_called() [email protected]("superset.db_engine_specs.base.is_hostname_valid") [email protected]("superset.db_engine_specs.base.is_port_open") [email protected]("superset.databases.commands.validate.DatabaseDAO") +@patch("superset.db_engine_specs.base.is_hostname_valid") +@patch("superset.db_engine_specs.base.is_port_open") +@patch("superset.databases.commands.validate.DatabaseDAO") def test_validate(DatabaseDAO, is_port_open, is_hostname_valid, app_context): """ Test parameter validation. @@ -797,8 +992,8 @@ def test_validate(DatabaseDAO, is_port_open, is_hostname_valid, app_context): command.run() [email protected]("superset.db_engine_specs.base.is_hostname_valid") [email protected]("superset.db_engine_specs.base.is_port_open") +@patch("superset.db_engine_specs.base.is_hostname_valid") +@patch("superset.db_engine_specs.base.is_port_open") def test_validate_partial(is_port_open, is_hostname_valid, app_context): """ Test parameter validation when only some parameters are present. @@ -838,7 +1033,7 @@ def test_validate_partial(is_port_open, is_hostname_valid, app_context): ] [email protected]("superset.db_engine_specs.base.is_hostname_valid") +@patch("superset.db_engine_specs.base.is_hostname_valid") def test_validate_partial_invalid_hostname(is_hostname_valid, app_context): """ Test parameter validation when only some parameters are present. @@ -892,7 +1087,7 @@ def test_validate_partial_invalid_hostname(is_hostname_valid, app_context): class TestTablesDatabaseCommand(SupersetTestCase): - @mock.patch("superset.databases.dao.DatabaseDAO.find_by_id") + @patch("superset.databases.dao.DatabaseDAO.find_by_id") def test_database_tables_list_with_unknown_database(self, mock_find_by_id): mock_find_by_id.return_value = None command = TablesDatabaseCommand(1, "test", False) @@ -901,9 +1096,9 @@ class TestTablesDatabaseCommand(SupersetTestCase): command.run() assert str(excinfo.value) == ("Database not found.") - @mock.patch("superset.databases.dao.DatabaseDAO.find_by_id") - @mock.patch("superset.security.manager.SupersetSecurityManager.can_access_database") - @mock.patch("superset.utils.core.g") + @patch("superset.databases.dao.DatabaseDAO.find_by_id") + @patch("superset.security.manager.SupersetSecurityManager.can_access_database") + @patch("superset.utils.core.g") def test_database_tables_superset_exception( self, mock_g, mock_can_access_database, mock_find_by_id ): @@ -920,9 +1115,9 @@ class TestTablesDatabaseCommand(SupersetTestCase): command.run() assert str(excinfo.value) == "Test Error" - @mock.patch("superset.databases.dao.DatabaseDAO.find_by_id") - @mock.patch("superset.security.manager.SupersetSecurityManager.can_access_database") - @mock.patch("superset.utils.core.g") + @patch("superset.databases.dao.DatabaseDAO.find_by_id") + @patch("superset.security.manager.SupersetSecurityManager.can_access_database") + @patch("superset.utils.core.g") def test_database_tables_exception( self, mock_g, mock_can_access_database, mock_find_by_id ): @@ -939,9 +1134,9 @@ class TestTablesDatabaseCommand(SupersetTestCase): == "Unexpected error occurred, please check your logs for details" ) - @mock.patch("superset.databases.dao.DatabaseDAO.find_by_id") - @mock.patch("superset.security.manager.SupersetSecurityManager.can_access_database") - @mock.patch("superset.utils.core.g") + @patch("superset.databases.dao.DatabaseDAO.find_by_id") + @patch("superset.security.manager.SupersetSecurityManager.can_access_database") + @patch("superset.utils.core.g") def test_database_tables_list_tables( self, mock_g, mock_can_access_database, mock_find_by_id ): diff --git a/tests/integration_tests/datasets/commands_tests.py b/tests/integration_tests/datasets/commands_tests.py index 0ce98477a0..6f5b796c6a 100644 --- a/tests/integration_tests/datasets/commands_tests.py +++ b/tests/integration_tests/datasets/commands_tests.py @@ -327,10 +327,11 @@ class TestImportDatasetsCommand(SupersetTestCase): db.session.commit() @patch("superset.datasets.commands.importers.v1.utils.g") + @patch("superset.security.manager.g") @pytest.mark.usefixtures("load_energy_table_with_slice") - def test_import_v1_dataset(self, mock_g): + def test_import_v1_dataset(self, sm_g, utils_g): """Test that we can import a dataset""" - mock_g.user = security_manager.find_user("admin") + admin = sm_g.user = utils_g.user = security_manager.find_user("admin") contents = { "metadata.yaml": yaml.safe_dump(dataset_metadata_config), "databases/imported_database.yaml": yaml.safe_dump(database_config), @@ -360,7 +361,7 @@ class TestImportDatasetsCommand(SupersetTestCase): ) # user should be included as one of the owners - assert dataset.owners == [mock_g.user] + assert dataset.owners == [admin] # database is also imported assert str(dataset.database.uuid) == "b8a1ccd3-779d-4ab7-8ad8-9ab119d7fe89" @@ -395,8 +396,11 @@ class TestImportDatasetsCommand(SupersetTestCase): db.session.delete(dataset.database) db.session.commit() - def test_import_v1_dataset_multiple(self): + @patch("superset.security.manager.g") + def test_import_v1_dataset_multiple(self, mock_g): """Test that a dataset can be imported multiple times""" + mock_g.user = security_manager.find_user("admin") + contents = { "metadata.yaml": yaml.safe_dump(dataset_metadata_config), "databases/imported_database.yaml": yaml.safe_dump(database_config), @@ -483,8 +487,11 @@ class TestImportDatasetsCommand(SupersetTestCase): } } - def test_import_v1_dataset_existing_database(self): + @patch("superset.security.manager.g") + def test_import_v1_dataset_existing_database(self, mock_g): """Test that a dataset can be imported when the database already exists""" + mock_g.user = security_manager.find_user("admin") + # first import database... contents = { "metadata.yaml": yaml.safe_dump(database_metadata_config), diff --git a/tests/integration_tests/queries/saved_queries/commands_tests.py b/tests/integration_tests/queries/saved_queries/commands_tests.py index bd90419155..5c7b862209 100644 --- a/tests/integration_tests/queries/saved_queries/commands_tests.py +++ b/tests/integration_tests/queries/saved_queries/commands_tests.py @@ -142,8 +142,11 @@ class TestExportSavedQueriesCommand(SupersetTestCase): class TestImportSavedQueriesCommand(SupersetTestCase): - def test_import_v1_saved_queries(self): + @patch("superset.security.manager.g") + def test_import_v1_saved_queries(self, mock_g): """Test that we can import a saved query""" + mock_g.user = security_manager.find_user("admin") + contents = { "metadata.yaml": yaml.safe_dump(saved_queries_metadata_config), "databases/imported_database.yaml": yaml.safe_dump(database_config), @@ -169,8 +172,11 @@ class TestImportSavedQueriesCommand(SupersetTestCase): db.session.delete(database) db.session.commit() - def test_import_v1_saved_queries_multiple(self): + @patch("superset.security.manager.g") + def test_import_v1_saved_queries_multiple(self, mock_g): """Test that a saved query can be imported multiple times""" + mock_g.user = security_manager.find_user("admin") + contents = { "metadata.yaml": yaml.safe_dump(saved_queries_metadata_config), "databases/imported_database.yaml": yaml.safe_dump(database_config), 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 e29fd70fb8..06e0063fe9 100644 --- a/tests/unit_tests/charts/commands/importers/v1/import_test.py +++ b/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" + ) diff --git a/tests/unit_tests/commands/importers/v1/assets_test.py b/tests/unit_tests/commands/importers/v1/assets_test.py index 1a345ff2b9..d48eed1be7 100644 --- a/tests/unit_tests/commands/importers/v1/assets_test.py +++ b/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 @@ from tests.unit_tests.fixtures.assets_configs import ( ) -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 = { 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 08f681d916..e07a23f6bf 100644 --- a/tests/unit_tests/dashboards/commands/importers/v1/import_test.py +++ b/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" + ) diff --git a/tests/unit_tests/databases/commands/importers/v1/import_test.py b/tests/unit_tests/databases/commands/importers/v1/import_test.py index e665bcb505..f9d2695f26 100644 --- a/tests/unit_tests/databases/commands/importers/v1/import_test.py +++ b/tests/unit_tests/databases/commands/importers/v1/import_test.py @@ -18,17 +18,24 @@ import copy +import pytest +from pytest_mock import MockFixture from sqlalchemy.orm.session import Session +from superset.commands.exceptions import ImportFailedError -def test_import_database(session: Session) -> None: + +def test_import_database(mocker: MockFixture, session: Session) -> None: """ Test importing a database. """ + from superset import security_manager from superset.databases.commands.importers.v1.utils import import_database from superset.models.core import Database from tests.integration_tests.fixtures.importexport import database_config + mocker.patch.object(security_manager, "can_access", return_value=True) + engine = session.get_bind() Database.metadata.create_all(engine) # pylint: disable=no-member @@ -58,14 +65,20 @@ def test_import_database(session: Session) -> None: assert database.allow_dml is False -def test_import_database_managed_externally(session: Session) -> None: +def test_import_database_managed_externally( + mocker: MockFixture, + session: Session, +) -> None: """ Test importing a database that is managed externally. """ + from superset import security_manager from superset.databases.commands.importers.v1.utils import import_database from superset.models.core import Database from tests.integration_tests.fixtures.importexport import database_config + mocker.patch.object(security_manager, "can_access", return_value=True) + engine = session.get_bind() Database.metadata.create_all(engine) # pylint: disable=no-member @@ -76,3 +89,30 @@ def test_import_database_managed_externally(session: Session) -> None: database = import_database(session, config) assert database.is_managed_externally is True assert database.external_url == "https://example.org/my_database"; + + +def test_import_database_without_permission( + mocker: MockFixture, + session: Session, +) -> None: + """ + Test importing a database when a user doesn't have permissions to create. + """ + from superset import security_manager + from superset.databases.commands.importers.v1.utils import import_database + from superset.models.core import Database + from tests.integration_tests.fixtures.importexport import database_config + + mocker.patch.object(security_manager, "can_access", return_value=False) + + engine = session.get_bind() + Database.metadata.create_all(engine) # pylint: disable=no-member + + config = copy.deepcopy(database_config) + + with pytest.raises(ImportFailedError) as excinfo: + import_database(session, config) + assert ( + str(excinfo.value) + == "Database doesn't exist and user doesn't have permission to create databases" + ) 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 5b52ac7f1d..839374425b 100644 --- a/tests/unit_tests/datasets/commands/importers/v1/import_test.py +++ b/tests/unit_tests/datasets/commands/importers/v1/import_test.py @@ -25,20 +25,27 @@ from unittest.mock import Mock, patch import pytest from flask import current_app +from pytest_mock import MockFixture from sqlalchemy.orm.session import Session -from superset.datasets.commands.exceptions import DatasetForbiddenDataURI +from superset.datasets.commands.exceptions import ( + DatasetForbiddenDataURI, + ImportFailedError, +) from superset.datasets.commands.importers.v1.utils import validate_data_uri -def test_import_dataset(session: Session) -> None: +def test_import_dataset(mocker: MockFixture, session: Session) -> None: """ Test importing a dataset. """ + from superset import security_manager from superset.connectors.sqla.models import SqlaTable from superset.datasets.commands.importers.v1.utils import import_dataset from superset.models.core import Database + mocker.patch.object(security_manager, "can_access", return_value=True) + engine = session.get_bind() SqlaTable.metadata.create_all(engine) # pylint: disable=no-member @@ -143,15 +150,18 @@ def test_import_dataset(session: Session) -> None: assert sqla_table.database.id == database.id -def test_import_dataset_duplicate_column(session: Session) -> None: +def test_import_dataset_duplicate_column(mocker: MockFixture, session: Session) -> None: """ Test importing a dataset with a column that already exists. """ + from superset import security_manager from superset.columns.models import Column as NewColumn from superset.connectors.sqla.models import SqlaTable, TableColumn from superset.datasets.commands.importers.v1.utils import import_dataset from superset.models.core import Database + mocker.patch.object(security_manager, "can_access", return_value=True) + engine = session.get_bind() SqlaTable.metadata.create_all(engine) # pylint: disable=no-member @@ -266,15 +276,18 @@ def test_import_dataset_duplicate_column(session: Session) -> None: assert sqla_table.database.id == database.id -def test_import_column_extra_is_string(session: Session) -> None: +def test_import_column_extra_is_string(mocker: MockFixture, session: Session) -> None: """ Test importing a dataset when the column extra is a string. """ + from superset import security_manager from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn from superset.datasets.commands.importers.v1.utils import import_dataset from superset.datasets.schemas import ImportV1DatasetSchema from superset.models.core import Database + mocker.patch.object(security_manager, "can_access", return_value=True) + engine = session.get_bind() SqlaTable.metadata.create_all(engine) # pylint: disable=no-member @@ -347,12 +360,17 @@ def test_import_column_extra_is_string(session: Session) -> None: @patch("superset.datasets.commands.importers.v1.utils.request") -def test_import_column_allowed_data_url(request: Mock, session: Session) -> None: +def test_import_column_allowed_data_url( + request: Mock, + mocker: MockFixture, + session: Session, +) -> None: """ Test importing a dataset when using data key to fetch data from a URL. """ import io + from superset import security_manager from superset.connectors.sqla.models import SqlaTable from superset.datasets.commands.importers.v1.utils import import_dataset from superset.datasets.schemas import ImportV1DatasetSchema @@ -360,6 +378,8 @@ def test_import_column_allowed_data_url(request: Mock, session: Session) -> None request.urlopen.return_value = io.StringIO("col1\nvalue1\nvalue2\n") + mocker.patch.object(security_manager, "can_access", return_value=True) + engine = session.get_bind() SqlaTable.metadata.create_all(engine) # pylint: disable=no-member @@ -419,15 +439,21 @@ def test_import_column_allowed_data_url(request: Mock, session: Session) -> None ).fetchall() -def test_import_dataset_managed_externally(session: Session) -> None: +def test_import_dataset_managed_externally( + mocker: MockFixture, + session: Session, +) -> None: """ Test importing a dataset that is managed externally. """ + from superset import security_manager from superset.connectors.sqla.models import SqlaTable from superset.datasets.commands.importers.v1.utils import import_dataset from superset.models.core import Database from tests.integration_tests.fixtures.importexport import dataset_config + mocker.patch.object(security_manager, "can_access", return_value=True) + engine = session.get_bind() SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
