aminghadersohi commented on code in PR #45350:
URL: https://github.com/apache/superset/pull/45350#discussion_r4332802163
##########
superset/commands/database/utils.py:
##########
@@ -182,6 +183,55 @@ def oauth2_endpoint_rebind_unsafe(
return secret_reused and endpoint_changed
+def encrypted_extra_secrets_reused(
+ sensitive_fields: set[str],
+ existing_encrypted_extra: str | None,
+ submitted_masked_encrypted_extra: str | None,
+ *,
+ submitted: bool,
+) -> bool:
+ """
+ Whether an update would carry a stored ``encrypted_extra`` secret over.
+
+ A stored secret (a non-empty value at one of ``sensitive_fields``) is
reused
+ when the update leaves ``masked_encrypted_extra`` out -- the stored blob
stays
+ attached as is -- or sends the mask at that secret's path, which
+ ``unmask_encrypted_extra`` restores verbatim. Judged per secret, not by
+ comparing whole blobs: any other field changing in the same save must not
hide
+ a still-masked secret, and a secret re-entered with its stored value is a
fresh
+ one the caller is entitled to attach. A dropped path drops the secret.
+ """
+ try:
+ existing = json.loads(existing_encrypted_extra or "{}")
+ except (TypeError, ValueError):
+ return False
+ if not isinstance(existing, dict):
+ return False
+
+ def is_stored(field: str) -> bool:
+ return any(match.value for match in parse(field).find(existing))
+
+ if not any(
+ match.value
+ for field in sensitive_fields
+ for match in parse(field).find(existing)
+ ):
+ return False # nothing stored to carry over
Review Comment:
Druid accepts JSON `0`/`false` passwords and Requests serializes them to
nonempty text. Both truthiness checks label these as absent, so a masked
password survives a host change; checking for null/empty values keeps the guard
aligned with the driver.
```suggestion
def is_stored(field: str) -> bool:
return any(
match.value not in (None, "") for match in
parse(field).find(existing)
)
if not any(is_stored(field) for field in sensitive_fields):
return False # nothing stored to carry over
```
##########
tests/unit_tests/commands/databases/update_test.py:
##########
@@ -1285,3 +1291,108 @@ def
test_update_oauth2_unresolvable_endpoints_purges_tokens(
command._handle_oauth2()
purge_oauth2_tokens.assert_called_once()
+
+
+def _snowflake_like_database(
+ mocker: MockerFixture, encrypted_extra: str, password: str | None
+) -> MagicMock:
+ """An existing connection whose engine masks the real sensitive paths."""
+ from superset.db_engine_specs.base import BaseEngineSpec
+
+ class Spec(BaseEngineSpec):
+ encrypted_extra_sensitive_fields = {
+ "$.oauth2_client_info.secret": "OAuth2 Client Secret",
+ "$.auth_params.privatekey_body": "Private Key Body",
+ }
+
+ model = mocker.MagicMock(allow_multi_catalog=False)
+ model.sqlalchemy_uri = "snowflake://svc:XXXXXXXXXX@old-account/DB"
+ model.password = password
+ model.extra = "{}"
+ model.encrypted_extra = encrypted_extra
+ model.ssh_tunnel = None
+ model.db_engine_spec = Spec
+ model.get_default_catalog.return_value = "DB"
+ model.id = 1
+ return model
+
+
[email protected](
+ "stored,submitted,password",
+ [
+ # A scope change alongside the move used to hide the masked secret.
+ (
+ json.dumps({"oauth2_client_info": {"id": "c", "secret": "real"}}),
+ {"oauth2_client_info": {"id": "c", "secret": PASSWORD_MASK,
"scope": "x"}},
+ "real-pw",
+ ),
+ # Compact JSON stored on create never equalled the re-serialized form.
+ (
+ '{"oauth2_client_info":{"id":"c","secret":"real"}}',
+ {"oauth2_client_info": {"id": "c", "secret": PASSWORD_MASK}},
+ "real-pw",
+ ),
+ # Key pair, no URI password: only the encrypted_extra check guards it.
+ (
+ json.dumps({"auth_params": {"privatekey_body": "PEM"}}),
+ {"auth_params": {"privatekey_body": PASSWORD_MASK}, "auth_method":
"kp"},
+ None,
+ ),
+ ],
+)
+def test_update_move_refuses_a_masked_secret_whatever_else_changes(
+ mocker: MockerFixture,
+ stored: str,
+ submitted: dict[str, Any],
+ password: str | None,
+) -> None:
+ """
+ A move keeps no stored secret the submission leaves masked (sc-126869).
Review Comment:
Nit: Please drop the private tracker ids from the comments/docstrings in
both test files; these identifiers land in the public source.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
