Skip to content

Commit

Permalink
fix: validate DB-specific parameters (apache#15155)
Browse files Browse the repository at this point in the history 
* fix: validate DB-specific parameters

* Fix lint

* Update test

* Fix lint/test

* Fix lint

* Update superset/databases/api.py
  • Loading branch information
@betodealmeida
betodealmeida authored and cccs-RyanS committed Dec 17, 2021
1 parent 4fe915c commit 43a7972
Show file tree
Hide file tree
Showing 4 changed files with 141 additions and 30 deletions.
15 changes: 13 additions & 2 deletions superset/databases/api.py
View file
Expand Up @@ -41,6 +41,7 @@
DatabaseInvalidError,
DatabaseNotFoundError,
DatabaseUpdateFailedError,
InvalidParametersError,
)
from superset.databases.commands.export import ExportDatabasesCommand
from superset.databases.commands.importers.dispatcher import ImportDatabasesCommand
Expand All @@ -65,7 +66,8 @@
)
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
Expand Down Expand Up @@ -1003,7 +1005,16 @@ def validate_parameters( # pylint: disable=too-many-return-statements
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()
Expand Down
54 changes: 33 additions & 21 deletions superset/databases/schemas.py
View file
Expand Up @@ -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 _
Expand All @@ -27,7 +27,7 @@
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
Expand Down Expand Up @@ -253,28 +253,11 @@ def build_sqlalchemy_uri(
the constructed SQLAlchemy URI to be passed.
"""
parameters = data.pop("parameters", {})

# 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)
engine = data.pop("engine", None)

configuration_method = data.get("configuration_method")
if configuration_method == ConfigurationMethod.DYNAMIC_FORM:
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,)]
)
engine_spec = engine_specs[engine]
engine_spec = get_engine_spec(engine)

if not hasattr(engine_spec, "build_sqlalchemy_uri") or not hasattr(
engine_spec, "parameters_schema"
Expand Down Expand Up @@ -304,6 +287,24 @@ def build_sqlalchemy_uri(
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(
Expand Down Expand Up @@ -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
Expand Down
8 changes: 7 additions & 1 deletion superset/db_engine_specs/bigquery.py
View file
Expand Up @@ -29,7 +29,7 @@

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
Expand Down Expand Up @@ -331,6 +331,12 @@ def get_parameters_from_uri(
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:
"""
Expand Down
94 changes: 88 additions & 6 deletions tests/databases/api_tests.py
View file
Expand Up @@ -1674,22 +1674,33 @@ def test_validate_parameters_invalid_payload_schema(self):
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": {
"engine": ["Missing data for required field."],
"foo": ["Unknown field."],
},
"invalid": ["engine"],
"issue_codes": [
{
"code": 1020,
"message": "Issue 1020 - The submitted payload has the incorrect schema.",
}
],
},
}
},
{
"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.",
}
],
},
},
]
}

Expand Down Expand Up @@ -1733,6 +1744,77 @@ def test_validate_parameters_missing_fields(self):
]
}

@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
Expand Down

0 comments on commit 43a7972

Please sign in to comment.