sadpandajoe commented on code in PR #45186:
URL: https://github.com/apache/superset/pull/45186#discussion_r4297185318
##########
superset/db_engine_specs/snowflake.py:
##########
@@ -686,11 +686,13 @@ def update_params_from_encrypted_extra(
key = None
if privatekey_body:
key = privatekey_body.encode()
- else:
+ elif auth_params.get("privatekey_path"):
with open(auth_params["privatekey_path"], "rb") as key_temp:
key = key_temp.read()
+ else:
+ return
Review Comment:
With `auth_method` set to `keypair` but neither `privatekey_body` nor
`privatekey_path` supplied, this now returns without adding `private_key`,
where the previous code raised on the missing path. If the SQLAlchemy URI still
carries a password, Test Connection can succeed on password auth while the
saved keypair config is incomplete, and queries fail later once that password
is rotated or removed. Should an explicit keypair method with no key material
raise a validation error instead of returning silently?
##########
pyproject.toml:
##########
@@ -383,7 +383,7 @@ disallow_untyped_calls = false
disallow_untyped_defs = false
disable_error_code = "annotation-unchecked"
-# TODO: remove this once cryptography is fixed, introduced in cryptography
44.0.3
+# Cryptography typing overrides (cryptography 48.0.1+ and 50.0.2+ provide
modern typing)
Review Comment:
The `ignore_errors` and `follow_imports = "skip"` override below is
unchanged, but the comment now says cryptography provides modern typing, which
reads as if the suppression no longer applies, and the original removal
condition is gone. Should the comment keep the reason this override exists and
when it can be dropped, or should the override itself go if the typing problem
is fixed?
##########
superset/db_engine_specs/snowflake.py:
##########
@@ -686,11 +686,13 @@ def update_params_from_encrypted_extra(
key = None
if privatekey_body:
key = privatekey_body.encode()
- else:
+ elif auth_params.get("privatekey_path"):
Review Comment:
The `privatekey_path` branch changed here has no test: every added case
passes `privatekey_body` or no key material, so a regression that stops reading
file-backed keys would leave the suite green while path-only keypair auth
breaks. Could a test write a PEM to `tmp_path`, set only `privatekey_path`, and
assert `connect_args["private_key"]` equals the expected PKCS8 DER bytes?
--
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]
