chore(database): Creating helper make_url_safe to wrap potential erro…

…rs (#19526)

* Creating helper make_url_safe to wrap potential errors

* Fixing imports

* Fixing imports again

* Adding comment

* Linting

* Fixing test

* Fixing test again...

* Fixing import

superset/cli/importexport.py

@@ -74,9 +74,8 @@ def export_dashboards(dashboard_file: Optional[str] = None) -> None:
         from superset.dashboards.commands.export import ExportDashboardsCommand
         from superset.models.dashboard import Dashboard
 
-        g.user = security_manager.find_user(  # pylint: disable=assigning-non-slot
-            username="admin"
-        )
+        # pylint: disable=assigning-non-slot
+        g.user = security_manager.find_user(username="admin")
 
         dashboard_ids = [id_ for (id_,) in db.session.query(Dashboard.id).all()]
         timestamp = datetime.now().strftime("%Y%m%dT%H%M%S")
@@ -110,9 +109,8 @@ def export_datasources(datasource_file: Optional[str] = None) -> None:
         from superset.connectors.sqla.models import SqlaTable
         from superset.datasets.commands.export import ExportDatasetsCommand
 
-        g.user = security_manager.find_user(  # pylint: disable=assigning-non-slot
-            username="admin"
-        )
+        # pylint: disable=assigning-non-slot
+        g.user = security_manager.find_user(username="admin")
 
         dataset_ids = [id_ for (id_,) in db.session.query(SqlaTable.id).all()]
         timestamp = datetime.now().strftime("%Y%m%dT%H%M%S")
@@ -153,9 +151,8 @@ def import_dashboards(path: str, username: Optional[str]) -> None:
         )
 
         if username is not None:
-            g.user = security_manager.find_user(  # pylint: disable=assigning-non-slot
-                username=username
-            )
+            # pylint: disable=assigning-non-slot
+            g.user = security_manager.find_user(username=username)
         if is_zipfile(path):
             with ZipFile(path) as bundle:
                 contents = get_contents_from_bundle(bundle)
@@ -320,9 +317,8 @@ def import_dashboards(path: str, recursive: bool, username: str) -> None:
         elif path_object.exists() and recursive:
             files.extend(path_object.rglob("*.json"))
         if username is not None:
-            g.user = security_manager.find_user(  # pylint: disable=assigning-non-slot
-                username=username
-            )
+            # pylint: disable=assigning-non-slot
+            g.user = security_manager.find_user(username=username)
         contents = {}
         for path_ in files:
             with open(path_) as file:

superset/databases/commands/test_connection.py

@@ -23,7 +23,6 @@
 from flask_appbuilder.security.sqla.models import User
 from flask_babel import gettext as _
 from func_timeout import func_timeout, FunctionTimedOut
-from sqlalchemy.engine.url import make_url
 from sqlalchemy.exc import DBAPIError, NoSuchModuleError
 
 from superset.commands.base import BaseCommand
@@ -34,6 +33,7 @@
     DatabaseTestConnectionUnexpectedError,
 )
 from superset.databases.dao import DatabaseDAO
+from superset.databases.utils import make_url_safe
 from superset.errors import ErrorLevel, SupersetErrorType
 from superset.exceptions import SupersetSecurityException, SupersetTimeoutException
 from superset.extensions import event_logger
@@ -55,7 +55,7 @@ def run(self) -> None:
             uri = self._model.sqlalchemy_uri_decrypted
 
         # context for error messages
-        url = make_url(uri)
+        url = make_url_safe(uri)
         context = {
             "hostname": url.host,
             "password": url.password,

superset/databases/commands/validate.py

@@ -20,7 +20,6 @@
 
 from flask_appbuilder.security.sqla.models import User
 from flask_babel import gettext as __
-from sqlalchemy.engine.url import make_url
 
 from superset.commands.base import BaseCommand
 from superset.databases.commands.exceptions import (
@@ -30,6 +29,7 @@
     InvalidParametersError,
 )
 from superset.databases.dao import DatabaseDAO
+from superset.databases.utils import make_url_safe
 from superset.db_engine_specs import get_engine_specs
 from superset.db_engine_specs.base import BasicParametersMixin
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
@@ -121,7 +121,7 @@ def run(self) -> None:
             with closing(engine.raw_connection()) as conn:
                 alive = engine.dialect.do_ping(conn)
         except Exception as ex:
-            url = make_url(sqlalchemy_uri)
+            url = make_url_safe(sqlalchemy_uri)
             context = {
                 "hostname": url.host,
                 "password": url.password,

superset/databases/schemas.py

@@ -24,10 +24,10 @@
 from marshmallow.validate import Length, ValidationError
 from marshmallow_enum import EnumField
 from sqlalchemy import MetaData
-from sqlalchemy.engine.url import make_url
-from sqlalchemy.exc import ArgumentError
 
 from superset import db
+from superset.databases.commands.exceptions import DatabaseInvalidError
+from superset.databases.utils import make_url_safe
 from superset.db_engine_specs import BaseEngineSpec, get_engine_specs
 from superset.exceptions import CertificateException, SupersetSecurityException
 from superset.models.core import ConfigurationMethod, Database, PASSWORD_MASK
@@ -144,8 +144,8 @@ def sqlalchemy_uri_validator(value: str) -> str:
     Validate if it's a valid SQLAlchemy URI and refuse SQLLite by default
     """
     try:
-        uri = make_url(value.strip())
-    except (ArgumentError, AttributeError, ValueError) as ex:
+        uri = make_url_safe(value.strip())
+    except DatabaseInvalidError as ex:
         raise ValidationError(
             [
                 _(
@@ -649,7 +649,7 @@ def validate_password(self, data: Dict[str, Any], **kwargs: Any) -> None:
             return
 
         uri = data["sqlalchemy_uri"]
-        password = make_url(uri).password
+        password = make_url_safe(uri).password
         if password == PASSWORD_MASK and data.get("password") is None:
             raise ValidationError("Must provide a password for the database")
 

superset/databases/utils.py

@@ -16,14 +16,15 @@
 # under the License.
 from typing import Any, Dict, List, Optional
 
-from superset import app
-from superset.models.core import Database
+from sqlalchemy.engine.url import make_url, URL
 
-custom_password_store = app.config["SQLALCHEMY_CUSTOM_PASSWORD_STORE"]
+from superset.databases.commands.exceptions import DatabaseInvalidError
 
 
 def get_foreign_keys_metadata(
-    database: Database, table_name: str, schema_name: Optional[str]
+    database: Any,
+    table_name: str,
+    schema_name: Optional[str],
 ) -> List[Dict[str, Any]]:
     foreign_keys = database.get_foreign_keys(table_name, schema_name)
     for fk in foreign_keys:
@@ -33,7 +34,7 @@ def get_foreign_keys_metadata(
 
 
 def get_indexes_metadata(
-    database: Database, table_name: str, schema_name: Optional[str]
+    database: Any, table_name: str, schema_name: Optional[str]
 ) -> List[Dict[str, Any]]:
     indexes = database.get_indexes(table_name, schema_name)
     for idx in indexes:
@@ -51,7 +52,7 @@ def get_col_type(col: Dict[Any, Any]) -> str:
 
 
 def get_table_metadata(
-    database: Database, table_name: str, schema_name: Optional[str]
+    database: Any, table_name: str, schema_name: Optional[str]
 ) -> Dict[str, Any]:
     """
     Get table metadata information, including type, pk, fks.
@@ -101,3 +102,17 @@ def get_table_metadata(
         "indexes": keys,
         "comment": table_comment,
     }
+
+
+def make_url_safe(raw_url: str) -> URL:
+    """
+    Wrapper for SQLAlchemy's make_url(), which tends to raise too detailed of
+    errors, which inevitably find their way into server logs. ArgumentErrors
+    tend to contain usernames and passwords, which makes them non-log-friendly
+    :param raw_url:
+    :return:
+    """
+    try:
+        return make_url(raw_url.strip())
+    except Exception:
+        raise DatabaseInvalidError()  # pylint: disable=raise-missing-from

superset/db_engine_specs/base.py

@@ -48,7 +48,7 @@
 from sqlalchemy.engine.base import Engine
 from sqlalchemy.engine.interfaces import Compiled, Dialect
 from sqlalchemy.engine.reflection import Inspector
-from sqlalchemy.engine.url import make_url, URL
+from sqlalchemy.engine.url import URL
 from sqlalchemy.ext.compiler import compiles
 from sqlalchemy.orm import Session
 from sqlalchemy.sql import quoted_name, text
@@ -58,6 +58,7 @@
 from typing_extensions import TypedDict
 
 from superset import security_manager, sql_parse
+from superset.databases.utils import make_url_safe
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
 from superset.models.sql_lab import Query
 from superset.models.sql_types.base import literal_dttm_type_factory
@@ -1633,7 +1634,7 @@ def build_sqlalchemy_uri(  # pylint: disable=unused-argument
     def get_parameters_from_uri(  # pylint: disable=unused-argument
         cls, uri: str, encrypted_extra: Optional[Dict[str, Any]] = None
     ) -> BasicParametersType:
-        url = make_url(uri)
+        url = make_url_safe(uri)
         query = {
             key: value
             for (key, value) in url.query.items()

superset/db_engine_specs/bigquery.py

@@ -28,11 +28,11 @@
 from marshmallow.exceptions import ValidationError
 from sqlalchemy import column
 from sqlalchemy.engine.base import Engine
-from sqlalchemy.engine.url import make_url
 from sqlalchemy.sql import sqltypes
 from typing_extensions import TypedDict
 
 from superset.databases.schemas import encrypted_field_properties, EncryptedString
+from superset.databases.utils import make_url_safe
 from superset.db_engine_specs.base import BaseEngineSpec
 from superset.db_engine_specs.exceptions import SupersetDBAPIDisconnectionError
 from superset.errors import SupersetError, SupersetErrorType
@@ -377,7 +377,7 @@ def build_sqlalchemy_uri(
     def get_parameters_from_uri(
         cls, uri: str, encrypted_extra: Optional[Dict[str, str]] = None
     ) -> Any:
-        value = make_url(uri)
+        value = make_url_safe(uri)
 
         # Building parameters from encrypted_extra and uri
         if encrypted_extra:

superset/db_engine_specs/hive.py

@@ -31,11 +31,12 @@
 from sqlalchemy import Column, text
 from sqlalchemy.engine.base import Engine
 from sqlalchemy.engine.reflection import Inspector
-from sqlalchemy.engine.url import make_url, URL
+from sqlalchemy.engine.url import URL
 from sqlalchemy.orm import Session
 from sqlalchemy.sql.expression import ColumnClause, Select
 
 from superset.common.db_query_status import QueryStatus
+from superset.databases.utils import make_url_safe
 from superset.db_engine_specs.base import BaseEngineSpec
 from superset.db_engine_specs.presto import PrestoEngineSpec
 from superset.exceptions import SupersetException
@@ -510,7 +511,7 @@ def update_impersonation_config(
         :param username: Effective username
         :return: None
         """
-        url = make_url(uri)
+        url = make_url_safe(uri)
         backend_name = url.get_backend_name()
 
         # Must be Hive connection, enable impersonation, and set optional param

superset/db_engine_specs/presto.py

@@ -34,12 +34,13 @@
 from sqlalchemy.engine.base import Engine
 from sqlalchemy.engine.reflection import Inspector
 from sqlalchemy.engine.result import RowProxy
-from sqlalchemy.engine.url import make_url, URL
+from sqlalchemy.engine.url import URL
 from sqlalchemy.orm import Session
 from sqlalchemy.sql.expression import ColumnClause, Select
 
 from superset import cache_manager, is_feature_enabled
 from superset.common.db_query_status import QueryStatus
+from superset.databases.utils import make_url_safe
 from superset.db_engine_specs.base import BaseEngineSpec, ColumnTypeMapping
 from superset.errors import SupersetErrorType
 from superset.exceptions import SupersetTemplateException
@@ -235,7 +236,7 @@ def update_impersonation_config(
         :param username: Effective username
         :return: None
         """
-        url = make_url(uri)
+        url = make_url_safe(uri)
         backend_name = url.get_backend_name()
 
         # Must be Presto connection, enable impersonation, and set optional param

superset/db_engine_specs/snowflake.py

@@ -24,9 +24,10 @@
 from apispec.ext.marshmallow import MarshmallowPlugin
 from flask_babel import gettext as __
 from marshmallow import fields, Schema
-from sqlalchemy.engine.url import make_url, URL
+from sqlalchemy.engine.url import URL
 from typing_extensions import TypedDict
 
+from superset.databases.utils import make_url_safe
 from superset.db_engine_specs.postgres import PostgresBaseEngineSpec
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
 from superset.models.sql_lab import Query
@@ -220,7 +221,7 @@ def get_parameters_from_uri(
             Dict[str, str]
         ] = None,
     ) -> Any:
-        url = make_url(uri)
+        url = make_url_safe(uri)
         query = dict(url.query.items())
         return {
             "username": url.username,

superset/db_engine_specs/trino.py

@@ -21,8 +21,9 @@
 
 import simplejson as json
 from flask import current_app
-from sqlalchemy.engine.url import make_url, URL
+from sqlalchemy.engine.url import URL
 
+from superset.databases.utils import make_url_safe
 from superset.db_engine_specs.base import BaseEngineSpec
 from superset.utils import core as utils
 
@@ -107,7 +108,7 @@ def update_impersonation_config(
         :param username: Effective username
         :return: None
         """
-        url = make_url(uri)
+        url = make_url_safe(uri)
         backend_name = url.get_backend_name()
 
         # Must be Trino connection, enable impersonation, and set optional param