You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by el...@apache.org on 2023/07/20 23:40:21 UTC
[superset] branch 2.1 updated: fix: import database engine validation (#24697)
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
The following commit(s) were added to refs/heads/2.1 by this push:
new 9a647de1a7 fix: import database engine validation (#24697)
9a647de1a7 is described below
commit 9a647de1a70eb035894b497103895bb8c95954e4
Author: Daniel Vaz Gaspar <da...@gmail.com>
AuthorDate: Thu Jul 20 13:08:07 2023 +0100
fix: import database engine validation (#24697)
---
superset/databases/commands/importers/v1/utils.py | 12 ++++++++--
.../integration_tests/databases/commands_tests.py | 4 ++--
tests/integration_tests/fixtures/importexport.py | 24 +++++++++++++++----
.../databases/commands/importers/v1/import_test.py | 28 +++++++++++++++++++++-
4 files changed, 59 insertions(+), 9 deletions(-)
diff --git a/superset/databases/commands/importers/v1/utils.py b/superset/databases/commands/importers/v1/utils.py
index 5f7af41e6e..9cffc3d591 100644
--- a/superset/databases/commands/importers/v1/utils.py
+++ b/superset/databases/commands/importers/v1/utils.py
@@ -20,9 +20,12 @@ from typing import Any, Dict
from sqlalchemy.orm import Session
-from superset import security_manager
+from superset import app, security_manager
from superset.commands.exceptions import ImportFailedError
+from superset.databases.utils import make_url_safe
+from superset.exceptions import SupersetSecurityException
from superset.models.core import Database
+from superset.security.analytics_db_safety import check_sqlalchemy_uri
def import_database(
@@ -44,7 +47,12 @@ def import_database(
raise ImportFailedError(
"Database doesn't exist and user doesn't have permission to create databases"
)
-
+ # Check if this URI is allowed
+ if app.config["PREVENT_UNSAFE_DB_CONNECTIONS"]:
+ try:
+ check_sqlalchemy_uri(make_url_safe(config["sqlalchemy_uri"]))
+ except SupersetSecurityException as exc:
+ raise ImportFailedError(exc.message) from exc
# https://github.com/apache/superset/pull/16756 renamed ``csv`` to ``file``.
config["allow_file_upload"] = config.pop("allow_csv_upload")
if "schemas_allowed_for_csv_upload" in config["extra"]:
diff --git a/tests/integration_tests/databases/commands_tests.py b/tests/integration_tests/databases/commands_tests.py
index 22b5be492d..4c62c08124 100644
--- a/tests/integration_tests/databases/commands_tests.py
+++ b/tests/integration_tests/databases/commands_tests.py
@@ -414,7 +414,7 @@ class TestImportDatabasesCommand(SupersetTestCase):
assert database.database_name == "imported_database"
assert database.expose_in_sqllab
assert database.extra == "{}"
- assert database.sqlalchemy_uri == "sqlite:///test.db"
+ assert database.sqlalchemy_uri == "someengine://user:pass@host1"
db.session.delete(database)
db.session.commit()
@@ -453,7 +453,7 @@ class TestImportDatabasesCommand(SupersetTestCase):
assert database.database_name == "imported_database"
assert database.expose_in_sqllab
assert database.extra == '{"schemas_allowed_for_file_upload": ["upload"]}'
- assert database.sqlalchemy_uri == "sqlite:///test.db"
+ assert database.sqlalchemy_uri == "someengine://user:pass@host1"
db.session.delete(database)
db.session.commit()
diff --git a/tests/integration_tests/fixtures/importexport.py b/tests/integration_tests/fixtures/importexport.py
index 0f695f044e..ab9bee4776 100644
--- a/tests/integration_tests/fixtures/importexport.py
+++ b/tests/integration_tests/fixtures/importexport.py
@@ -346,7 +346,7 @@ saved_queries_metadata_config: Dict[str, Any] = {
"type": "SavedQuery",
"timestamp": "2021-03-30T20:37:54.791187+00:00",
}
-database_config: Dict[str, Any] = {
+database_config_sqlite: dict[str, Any] = {
"allow_csv_upload": True,
"allow_ctas": True,
"allow_cvas": True,
@@ -360,7 +360,8 @@ database_config: Dict[str, Any] = {
"uuid": "b8a1ccd3-779d-4ab7-8ad8-9ab119d7fe89",
"version": "1.0.0",
}
-database_with_ssh_tunnel_config_private_key: Dict[str, Any] = {
+
+database_config: dict[str, Any] = {
"allow_csv_upload": True,
"allow_ctas": True,
"allow_cvas": True,
@@ -370,7 +371,22 @@ database_with_ssh_tunnel_config_private_key: Dict[str, Any] = {
"database_name": "imported_database",
"expose_in_sqllab": True,
"extra": {},
- "sqlalchemy_uri": "sqlite:///test.db",
+ "sqlalchemy_uri": "someengine://user:pass@host1",
+ "uuid": "b8a1ccd3-779d-4ab7-8ad8-9ab119d7fe89",
+ "version": "1.0.0",
+}
+
+database_with_ssh_tunnel_config_private_key: dict[str, Any] = {
+ "allow_csv_upload": True,
+ "allow_ctas": True,
+ "allow_cvas": True,
+ "allow_dml": True,
+ "allow_run_async": False,
+ "cache_timeout": None,
+ "database_name": "imported_database",
+ "expose_in_sqllab": True,
+ "extra": {},
+ "sqlalchemy_uri": "someengine://user:pass@host1",
"uuid": "b8a1ccd3-779d-4ab7-8ad8-9ab119d7fe89",
"ssh_tunnel": {
"server_address": "localhost",
@@ -392,7 +408,7 @@ database_with_ssh_tunnel_config_password: Dict[str, Any] = {
"database_name": "imported_database",
"expose_in_sqllab": True,
"extra": {},
- "sqlalchemy_uri": "sqlite:///test.db",
+ "sqlalchemy_uri": "someengine://user:pass@host1",
"uuid": "b8a1ccd3-779d-4ab7-8ad8-9ab119d7fe89",
"ssh_tunnel": {
"server_address": "localhost",
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 f9d2695f26..b8bd24d94d 100644
--- a/tests/unit_tests/databases/commands/importers/v1/import_test.py
+++ b/tests/unit_tests/databases/commands/importers/v1/import_test.py
@@ -42,7 +42,7 @@ def test_import_database(mocker: MockFixture, session: Session) -> None:
config = copy.deepcopy(database_config)
database = import_database(session, config)
assert database.database_name == "imported_database"
- assert database.sqlalchemy_uri == "sqlite:///test.db"
+ assert database.sqlalchemy_uri == "someengine://user:pass@host1"
assert database.cache_timeout is None
assert database.expose_in_sqllab is True
assert database.allow_run_async is False
@@ -65,6 +65,32 @@ def test_import_database(mocker: MockFixture, session: Session) -> None:
assert database.allow_dml is False
+def test_import_database_sqlite_invalid(mocker: MockFixture, session: Session) -> None:
+ """
+ Test importing a database.
+ """
+ from superset import app, 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_sqlite
+
+ app.config["PREVENT_UNSAFE_DB_CONNECTIONS"] = True
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+
+ engine = session.get_bind()
+ Database.metadata.create_all(engine) # pylint: disable=no-member
+
+ config = copy.deepcopy(database_config_sqlite)
+ with pytest.raises(ImportFailedError) as excinfo:
+ _ = import_database(session, config)
+ assert (
+ str(excinfo.value)
+ == "SQLiteDialect_pysqlite cannot be used as a data source for security reasons."
+ )
+ # restore app config
+ app.config["PREVENT_UNSAFE_DB_CONNECTIONS"] = True
+
+
def test_import_database_managed_externally(
mocker: MockFixture,
session: Session,