Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions superset/db_engine_specs/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -532,10 +532,13 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
Pattern[str], tuple[str, SupersetErrorType, dict[str, Any]]
] = {}

# List of JSON path to fields in `encrypted_extra` that should be masked when the
# database is edited. By default everything is masked.
# JSONPath fields in `encrypted_extra` that should be masked when the database is
# edited. Can be a set of paths (labels will default to the path) or a dict mapping
# paths to human-readable labels for import validation error messages.
# pylint: disable=invalid-name
encrypted_extra_sensitive_fields: set[str] = {"$.*"}
encrypted_extra_sensitive_fields: set[str] | dict[str, str] = {
"$.*": "Encrypted Extra",
}

# Whether the engine supports file uploads
# if True, database will be listed as option in the upload file form
Expand Down Expand Up @@ -580,6 +583,22 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
# the `cancel_query` value in the `extra` field of the `query` object
has_query_id_before_execute = True

@classmethod
def encrypted_extra_sensitive_field_paths(cls) -> set[str]:
"""
Returns a set of paths for fields that should be masked in the
``masked_encrypted_extra`` JSON.

:param cls: Description
:return: Description
:rtype: set[str]
"""
return (
set(cls.encrypted_extra_sensitive_fields)
if isinstance(cls.encrypted_extra_sensitive_fields, dict)
else cls.encrypted_extra_sensitive_fields
)

@classmethod
def get_rls_method(cls) -> RLSMethod:
"""
Expand Down Expand Up @@ -2443,7 +2462,7 @@ def mask_encrypted_extra(cls, encrypted_extra: str | None) -> str | None:

masked_encrypted_extra = redact_sensitive(
config,
cls.encrypted_extra_sensitive_fields,
cls.encrypted_extra_sensitive_field_paths(),
)

return json.dumps(masked_encrypted_extra)
Expand All @@ -2469,7 +2488,7 @@ def unmask_encrypted_extra(cls, old: str | None, new: str | None) -> str | None:
new_config = reveal_sensitive(
old_config,
new_config,
cls.encrypted_extra_sensitive_fields,
cls.encrypted_extra_sensitive_field_paths(),
)

return json.dumps(new_config)
Expand Down
4 changes: 3 additions & 1 deletion superset/db_engine_specs/bigquery.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,9 @@ class BigQueryEngineSpec(BaseEngineSpec): # pylint: disable=too-many-public-met

# when editing the database, mask this field in `encrypted_extra`
# pylint: disable=invalid-name
encrypted_extra_sensitive_fields = {"$.credentials_info.private_key"}
encrypted_extra_sensitive_fields = {
"$.credentials_info.private_key": "Service Account Private Key",
}

"""
https://www.python.org/dev/peps/pep-0249/#arraysize
Expand Down
4 changes: 2 additions & 2 deletions superset/db_engine_specs/gsheets.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,8 @@ class GSheetsEngineSpec(ShillelaghEngineSpec):
# when editing the database, mask this field in `encrypted_extra`
# pylint: disable=invalid-name
encrypted_extra_sensitive_fields = {
"$.service_account_info.private_key",
"$.oauth2_client_info.secret",
"$.service_account_info.private_key": "Service Account Private Key",
"$.oauth2_client_info.secret": "OAuth2 Client Secret",
}

custom_errors: dict[Pattern[str], tuple[str, SupersetErrorType, dict[str, Any]]] = {
Expand Down
4 changes: 2 additions & 2 deletions superset/db_engine_specs/mysql.py
Original file line number Diff line number Diff line change
Expand Up @@ -307,8 +307,8 @@ class MySQLEngineSpec(BasicParametersMixin, BaseEngineSpec):
# This follows the pattern used by other engine specs (bigquery, snowflake, etc.)
# that specify exact paths rather than using the base class's catch-all "$.*".
encrypted_extra_sensitive_fields = {
"$.aws_iam.external_id",
"$.aws_iam.role_arn",
"$.aws_iam.external_id": "AWS IAM External ID",
"$.aws_iam.role_arn": "AWS IAM Role ARN",
}

@staticmethod
Expand Down
4 changes: 2 additions & 2 deletions superset/db_engine_specs/postgres.py
Original file line number Diff line number Diff line change
Expand Up @@ -464,8 +464,8 @@ class PostgresEngineSpec(BasicParametersMixin, PostgresBaseEngineSpec):
# This follows the pattern used by other engine specs (bigquery, snowflake, etc.)
# that specify exact paths rather than using the base class's catch-all "$.*".
encrypted_extra_sensitive_fields = {
"$.aws_iam.external_id",
"$.aws_iam.role_arn",
"$.aws_iam.external_id": "AWS IAM External ID",
"$.aws_iam.role_arn": "AWS IAM Role ARN",
}

column_type_mappings = (
Expand Down
4 changes: 2 additions & 2 deletions superset/db_engine_specs/redshift.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,8 @@ def normalize_table_name_for_upload(
# This follows the pattern used by other engine specs (bigquery, snowflake, etc.)
# that specify exact paths rather than using the base class's catch-all "$.*".
encrypted_extra_sensitive_fields = {
"$.aws_iam.external_id",
"$.aws_iam.role_arn",
"$.aws_iam.external_id": "AWS IAM External ID",
"$.aws_iam.role_arn": "AWS IAM Role ARN",
}

@staticmethod
Expand Down
4 changes: 2 additions & 2 deletions superset/db_engine_specs/snowflake.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,8 @@ class SnowflakeEngineSpec(PostgresBaseEngineSpec):

# pylint: disable=invalid-name
encrypted_extra_sensitive_fields = {
"$.auth_params.privatekey_body",
"$.auth_params.privatekey_pass",
"$.auth_params.privatekey_body": "Private Key Body",
"$.auth_params.privatekey_pass": "Private Key Password",
}

_time_grain_expressions = {
Expand Down
5 changes: 4 additions & 1 deletion superset/db_engine_specs/ydb.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ class YDBEngineSpec(BaseEngineSpec):
sqlalchemy_uri_placeholder = "ydb://{host}:{port}/{database_name}"

# pylint: disable=invalid-name
encrypted_extra_sensitive_fields = {"$.connect_args.credentials", "$.credentials"}
encrypted_extra_sensitive_fields = {
"$.connect_args.credentials": "Connection Credentials",
"$.credentials": "Credentials",
}

disable_ssh_tunneling = False

Expand Down
45 changes: 45 additions & 0 deletions tests/unit_tests/db_engine_specs/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,51 @@ def test_unmask_encrypted_extra() -> None:
)


@pytest.mark.parametrize(
"masked_encrypted_extra,expected_result",
Comment on lines +363 to +364
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parametrize decorator expects tuple not string

The @pytest.mark.parametrize decorator at line 364 uses a string for the first argument instead of a tuple. Change "masked_encrypted_extra,expected_result" to a tuple ("masked_encrypted_extra", "expected_result") to comply with pytest requirements.

Code suggestion
Check the AI-generated fix before applying
Suggested change
@pytest.mark.parametrize(
"masked_encrypted_extra,expected_result",
@pytest.mark.parametrize(
("masked_encrypted_extra", "expected_result"),

Code Review Run #824288


Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

[
(
{
"$.credentials_info.private_key": "Private Key",
"$.access_token": "Access Token",
},
{
"$.credentials_info.private_key",
"$.access_token",
},
),
(
{
"$.credentials_info.private_key",
"$.access_token",
},
{
"$.credentials_info.private_key",
"$.access_token",
},
),
(
None,
{"$.*"},
),
],
)
def test_encrypted_extra_sensitive_field_paths_from_dict(
masked_encrypted_extra: set[str] | dict[str, str] | None,
expected_result: set[str],
) -> None:
"""
Test that `encrypted_extra_sensitive_field_paths` extracts the keys
when `encrypted_extra_sensitive_fields` is a dict.
"""

class DictFieldsSpec(BaseEngineSpec):
if masked_encrypted_extra:
encrypted_extra_sensitive_fields = masked_encrypted_extra

assert DictFieldsSpec.encrypted_extra_sensitive_field_paths() == expected_result


def test_impersonate_user_backwards_compatible(mocker: MockerFixture) -> None:
"""
Test that the `impersonate_user` method calls the original methods it replaced.
Expand Down
Loading