sadpandajoe commented on code in PR #45870:
URL: https://github.com/apache/superset/pull/45870#discussion_r4450408493
##########
superset/commands/database/importers/v1/utils.py:
##########
@@ -96,6 +83,46 @@ def _refuse_stored_secret_reuse(existing: Database, config:
dict[str, Any]) -> N
)
+def _refuse_incomplete_embedded_credentials(
+ database_name: str,
+ encrypted_extra: dict[str, Any],
+ engine_spec: type[BaseEngineSpec],
+) -> None:
+ """
+ Refuse a half-filled embedded guest credential, as the dialog and API do.
+
+ Only the secrets that arrive masked are revealed from the stored
credential,
+ so one the file leaves out entirely (e.g. a key-pair credential without its
+ private key) would replace the stored credential with one no embedded
+ viewer can log in with.
+ """
+ if not (
+ is_feature_enabled("EMBEDDED_CREDENTIAL_FALLBACK")
+ and is_feature_enabled("EMBEDDED_SUPERSET")
+ ):
+ return
+
+ credentials = encrypted_extra.get(EMBEDDED_CREDENTIALS_KEY)
+ if not embedded_credentials_incomplete(
Review Comment:
A bundle whose `embedded_credentials` is a JSON string, e.g.
`"{\"username\":\"svc\",\"password\":\"pw\"}"`, passes this check because
`accept_json_string` defaults to `True`, but the importer stores the string
as-is. Query time then rejects a non-dict value, so embedded viewers get the
missing-credential refusal and an overwrite replaces a working credential with
one that can't be used. The save schema passes `accept_json_string=False` for
this reason. Should the import path do the same?
##########
superset/commands/database/importers/v1/utils.py:
##########
@@ -186,10 +214,40 @@ def import_database( # noqa: C901
new_config,
sensitive_fields,
)
+ # A database export carries all of Secure Extra, so what it omits
is
+ # removed. Anything else keeps what it does not carry.
+ if partial_secure_extra:
+ revealed = {**old_config, **revealed}
config["encrypted_extra"] = json.dumps(revealed)
else:
config["encrypted_extra"] = masked_encrypted_extra
+ if encrypted_extra := config.get("encrypted_extra"):
+ # The incoming URI decides the engine: an overwrite can change it, and
the
+ # stored engine's rules would miss the new engine's secrets and fields.
+ engine_spec = Database.get_db_engine_spec(
+ make_url_safe(config["sqlalchemy_uri"])
+ )
+ payload = json.loads(encrypted_extra)
+
+ # Never store a mask as if it were the secret: the schema asks for any
+ # value that cannot be revealed, so one left here is a bug, not input.
+ if masked_encrypted_extra and (
Review Comment:
This still-masked check only runs when the bundle carried
`masked_encrypted_extra`. A file using the plain `encrypted_extra` key skips it
(the import schema has no rename like the API schemas), and
`_refuse_incomplete_embedded_credentials` counts `PASSWORD_MASK` as a present
password. Database export writes plain `encrypted_extra` when the stored value
already equals its masked form, so exporting such a connection and importing it
with both flags on would store `XXXXXXXXXX` as the guest password. Should the
mask check also run for the plain `encrypted_extra` path?
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -553,11 +561,26 @@ export function dbReducer(
return {
...trimmedState,
};
- case ActionType.EditorChange:
- return {
+ case ActionType.EditorChange: {
+ const edited = {
...trimmedState,
[action.payload.name]: action.payload.json,
};
+ if (
+ action.payload.name === 'masked_encrypted_extra' &&
+ edited.parameters
+ ) {
+ // `onSave` copies these typed blocks over Secure Extra, which would
undo
+ // this later edit of them.
+ const {
+ embedded_credentials: _typedCredentials,
Review Comment:
Dropping `oauth2_client_info` from `parameters` on any Secure Extra edit
loses OAuth2 values that only live there. `OAuth2ClientField.handleChange` only
calls `onParametersChange`, so a typed client ID never reaches
`masked_encrypted_extra`. Changing the ID to `B` in the OAuth2 sub-form and
then typing in the Secure Extra editor resets the inputs to the stored `A`, and
Save keeps `A`. Should only `embedded_credentials` be stripped here, or should
the OAuth2 field also copy into Secure Extra?
--
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]
