fix: validate DB-specific parameters (#15155)

* fix: validate DB-specific parameters

* Fix lint

* Update test

* Fix lint/test

* Fix lint

* Update superset/databases/api.py

superset/databases/api.py

@@ -41,6 +41,7 @@ from superset.databases.commands.exceptions import (
     DatabaseInvalidError,
     DatabaseNotFoundError,
     DatabaseUpdateFailedError,
+    InvalidParametersError,
 )
 from superset.databases.commands.export import ExportDatabasesCommand
 from superset.databases.commands.importers.dispatcher import ImportDatabasesCommand
@@ -65,7 +66,8 @@ from superset.databases.schemas import (
 )
 from superset.databases.utils import get_table_metadata
 from superset.db_engine_specs import get_available_engine_specs
-from superset.exceptions import InvalidPayloadFormatError, InvalidPayloadSchemaError
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.exceptions import InvalidPayloadFormatError
 from superset.extensions import security_manager
 from superset.models.core import Database
 from superset.typing import FlaskResponse
@@ -1003,7 +1005,16 @@ class DatabaseRestApi(BaseSupersetModelRestApi):
         try:
             payload = DatabaseValidateParametersSchema().load(request.json)
         except ValidationError as error:
-            raise InvalidPayloadSchemaError(error)
+            errors = [
+                SupersetError(
+                    message="\n".join(messages),
+                    error_type=SupersetErrorType.INVALID_PAYLOAD_SCHEMA_ERROR,
+                    level=ErrorLevel.ERROR,
+                    extra={"invalid": [attribute]},
+                )
+                for attribute, messages in error.messages.items()
+            ]
+            raise InvalidParametersError(errors)
 
         command = ValidateDatabaseParametersCommand(g.user, payload)
         command.run()

superset/databases/schemas.py

@@ -16,7 +16,7 @@
 # under the License.
 import inspect
 import json
-from typing import Any, Dict
+from typing import Any, Dict, Optional, Type
 
 from flask import current_app
 from flask_babel import lazy_gettext as _
@@ -27,7 +27,7 @@ from sqlalchemy import MetaData
 from sqlalchemy.engine.url import make_url
 from sqlalchemy.exc import ArgumentError
 
-from superset.db_engine_specs import get_engine_specs
+from superset.db_engine_specs import BaseEngineSpec, get_engine_specs
 from superset.exceptions import CertificateException, SupersetSecurityException
 from superset.models.core import ConfigurationMethod, PASSWORD_MASK
 from superset.security.analytics_db_safety import check_sqlalchemy_uri
@@ -253,28 +253,11 @@ class DatabaseParametersSchemaMixin:
         the constructed SQLAlchemy URI to be passed.
         """
         parameters = data.pop("parameters", {})
-
+        engine = data.pop("engine", None)
-        # TODO (betodealmeida): remove second expression after making sure
-        # frontend is not passing engine inside parameters
-        engine = data.pop("engine", None) or parameters.pop("engine", None)
 
         configuration_method = data.get("configuration_method")
         if configuration_method == ConfigurationMethod.DYNAMIC_FORM:
-            if not engine:
+            engine_spec = get_engine_spec(engine)
-                raise ValidationError(
-                    [
-                        _(
-                            "An engine must be specified when passing "
-                            "individual parameters to a database."
-                        )
-                    ]
-                )
-            engine_specs = get_engine_specs()
-            if engine not in engine_specs:
-                raise ValidationError(
-                    [_('Engine "%(engine)s" is not a valid engine.', engine=engine,)]
-                )
-            engine_spec = engine_specs[engine]
 
             if not hasattr(engine_spec, "build_sqlalchemy_uri") or not hasattr(
                 engine_spec, "parameters_schema"
@@ -304,6 +287,24 @@ class DatabaseParametersSchemaMixin:
         return data
 
 
+def get_engine_spec(engine: Optional[str]) -> Type[BaseEngineSpec]:
+    if not engine:
+        raise ValidationError(
+            [
+                _(
+                    "An engine must be specified when passing "
+                    "individual parameters to a database."
+                )
+            ]
+        )
+    engine_specs = get_engine_specs()
+    if engine not in engine_specs:
+        raise ValidationError(
+            [_('Engine "%(engine)s" is not a valid engine.', engine=engine,)]
+        )
+    return engine_specs[engine]
+
+
 class DatabaseValidateParametersSchema(Schema):
     engine = fields.String(required=True, description="SQLAlchemy engine to use")
     parameters = fields.Dict(
@@ -333,6 +334,17 @@ class DatabaseValidateParametersSchema(Schema):
         description=configuration_method_description,
     )
 
+    @validates_schema
+    def validate_parameters(  # pylint: disable=no-self-use
+        self, data: Dict[str, Any], **kwargs: Any  # pylint: disable=unused-argument
+    ) -> None:
+        """
+        Validate the DB engine spec specific parameters schema.
+        """
+        # TODO (aafghahi): use a single parameter
+        engine_spec = get_engine_spec(data.get("engine") or data.get("backend"))
+        engine_spec.parameters_schema.load(data["parameters"])  # type: ignore
+
 
 class DatabasePostSchema(Schema, DatabaseParametersSchemaMixin):
     class Meta:  # pylint: disable=too-few-public-methods

superset/db_engine_specs/bigquery.py

@@ -29,7 +29,7 @@ from typing_extensions import TypedDict
 
 from superset.databases.schemas import encrypted_field_properties, EncryptedField
 from superset.db_engine_specs.base import BaseEngineSpec
-from superset.errors import SupersetErrorType
+from superset.errors import SupersetError, SupersetErrorType
 from superset.exceptions import SupersetGenericDBErrorException
 from superset.sql_parse import Table
 from superset.utils import core as utils
@@ -331,6 +331,12 @@ class BigQueryEngineSpec(BaseEngineSpec):
             message="Big Query encrypted_extra is not available.",
         )
 
+    @classmethod
+    def validate_parameters(
+        cls, parameters: BigQueryParametersType  # pylint: disable=unused-argument
+    ) -> List[SupersetError]:
+        return []
+
     @classmethod
     def parameters_json_schema(cls) -> Any:
         """

tests/databases/api_tests.py

@@ -1674,14 +1674,11 @@ class TestDatabaseApi(SupersetTestCase):
         assert response == {
             "errors": [
                 {
-                    "message": "An error happened when validating the request",
+                    "message": "Missing data for required field.",
                     "error_type": "INVALID_PAYLOAD_SCHEMA_ERROR",
                     "level": "error",
                     "extra": {
-                        "messages": {
+                        "invalid": ["engine"],
-                            "engine": ["Missing data for required field."],
-                            "foo": ["Unknown field."],
-                        },
                         "issue_codes": [
                             {
                                 "code": 1020,
@@ -1689,7 +1686,21 @@ class TestDatabaseApi(SupersetTestCase):
                             }
                         ],
                     },
-                }
+                },
+                {
+                    "message": "Unknown field.",
+                    "error_type": "INVALID_PAYLOAD_SCHEMA_ERROR",
+                    "level": "error",
+                    "extra": {
+                        "invalid": ["foo"],
+                        "issue_codes": [
+                            {
+                                "code": 1020,
+                                "message": "Issue 1020 - The submitted payload has the incorrect schema.",
+                            }
+                        ],
+                    },
+                },
             ]
         }
 
@@ -1733,6 +1744,77 @@ class TestDatabaseApi(SupersetTestCase):
             ]
         }
 
+    @mock.patch("superset.db_engine_specs.base.is_hostname_valid")
+    @mock.patch("superset.db_engine_specs.base.is_port_open")
+    @mock.patch("superset.databases.api.ValidateDatabaseParametersCommand")
+    def test_validate_parameters_valid_payload(
+        self, ValidateDatabaseParametersCommand, is_port_open, is_hostname_valid
+    ):
+        is_hostname_valid.return_value = True
+        is_port_open.return_value = True
+
+        self.login(username="admin")
+        url = "api/v1/database/validate_parameters"
+        payload = {
+            "engine": "postgresql",
+            "parameters": defaultdict(dict),
+        }
+        payload["parameters"].update(
+            {
+                "host": "localhost",
+                "port": 6789,
+                "username": "superset",
+                "password": "XXX",
+                "database": "test",
+                "query": {},
+            }
+        )
+        rv = self.client.post(url, json=payload)
+        response = json.loads(rv.data.decode("utf-8"))
+
+        assert rv.status_code == 200
+        assert response == {"message": "OK"}
+
+    def test_validate_parameters_invalid_port(self):
+        self.login(username="admin")
+        url = "api/v1/database/validate_parameters"
+        payload = {
+            "engine": "postgresql",
+            "parameters": defaultdict(dict),
+        }
+        payload["parameters"].update(
+            {
+                "host": "localhost",
+                "port": "string",
+                "username": "superset",
+                "password": "XXX",
+                "database": "test",
+                "query": {},
+            }
+        )
+        rv = self.client.post(url, json=payload)
+        response = json.loads(rv.data.decode("utf-8"))
+
+        assert rv.status_code == 422
+        assert response == {
+            "errors": [
+                {
+                    "message": "Not a valid integer.",
+                    "error_type": "INVALID_PAYLOAD_SCHEMA_ERROR",
+                    "level": "error",
+                    "extra": {
+                        "invalid": ["port"],
+                        "issue_codes": [
+                            {
+                                "code": 1020,
+                                "message": "Issue 1020 - The submitted payload has the incorrect schema.",
+                            }
+                        ],
+                    },
+                }
+            ]
+        }
+
     @mock.patch("superset.db_engine_specs.base.is_hostname_valid")
     def test_validate_parameters_invalid_host(self, is_hostname_valid):
         is_hostname_valid.return_value = False