sadpandajoe commented on code in PR #45870:
URL: https://github.com/apache/superset/pull/45870#discussion_r4419548127
##########
tests/unit_tests/databases/commands/importers/v1/import_test.py:
##########
@@ -970,3 +975,172 @@ def
test_import_database_masked_encrypted_extra_not_revealed_on_host_change(
assert database.password == "newpass" # noqa: S105
encrypted = json.loads(database.encrypted_extra)
assert encrypted["my_secret"] == PASSWORD_MASK
+
+
+def test_import_database_never_stores_a_mask_over_an_existing_db(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ Overwriting a connection that has no stored value for a masked secret
fails.
+
+ A connection promoted by a dashboard bundle exists without its secrets, so
+ there is nothing to reveal; storing the mask would make it the secret (QA
+ OEF-SECRET-04). The schema asks for the value first; this is the backstop.
+ """
+ from superset import security_manager
+ from superset.commands.database.importers.v1.utils import import_database
+ from superset.commands.exceptions import ImportFailedError
+ from superset.constants import PASSWORD_MASK
+ from superset.models.core import Database
+ from tests.integration_tests.fixtures.importexport import (
+ database_config_with_masked_encrypted_extra,
+ )
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+
mocker.patch("superset.commands.database.importers.v1.utils.add_permissions")
+
+ engine = db.session.get_bind()
+ Database.metadata.create_all(engine) # pylint: disable=no-member
+
+ # The promoted copy: same connection, no encrypted_extra.
+ config = copy.deepcopy(database_config_with_masked_encrypted_extra)
+ del config["masked_encrypted_extra"]
+ existing = import_database(config)
+ db.session.flush()
+
+ masked = copy.deepcopy(database_config_with_masked_encrypted_extra)
+ masked["masked_encrypted_extra"] = json.dumps(
+ {"credentials_info": {"type": "service_account", "private_key":
PASSWORD_MASK}}
+ )
+ with pytest.raises(ImportFailedError,
match="credentials_info.private_key"):
+ import_database(masked, overwrite=True)
+
+ assert existing.encrypted_extra is None
+
+
+EMBEDDED_KEYPAIR = {
+ "username": "EMBED_SVC",
+ "auth_method": "keypair",
+ "privatekey_body": "-----BEGIN PRIVATE KEY-----\nKEY\n-----END PRIVATE
KEY-----\n",
+ "privatekey_pass": "passphrase",
+}
+
+
+def _snowflake_config(embedded_credentials: dict[str, str]) -> dict[str,
object]:
+ """A per-user OAuth2 Snowflake connection carrying an embedded
credential."""
+ return {
+ "database_name": "imported_snowflake",
+ "sqlalchemy_uri": "snowflake://base_user@account/db?warehouse=WH",
+ "uuid": "6c1c5ee7-1d83-4f57-9a8e-1a2b3c4d5e6f",
+ "version": "1.0.0",
+ "extra": {},
+ "impersonate_user": True,
+ "masked_encrypted_extra": json.dumps(
Review Comment:
Every new command test builds its config through `masked_encrypted_extra`,
but this commit also moved the completeness check out of that branch so it now
runs for a bundle that supplies only `encrypted_extra`. If the check were moved
back inside the `masked_encrypted_extra` block, all four of these tests would
still pass while an overwrite carrying `{"embedded_credentials": {"username":
"u"}}` in plain `encrypted_extra` would again replace the stored credential
unchecked. Would it be worth adding one overwrite case that uses only
`encrypted_extra` and asserts the `$.embedded_credentials.password` refusal and
the unchanged stored credential?
--
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]
