hughhhh commented on code in PR #45890:
URL: https://github.com/apache/superset/pull/45890#discussion_r4445366566
##########
superset/databases/schemas.py:
##########
@@ -444,6 +448,97 @@ def build_sqlalchemy_uri(
return data
+ @validates_schema
+ def validate_embedded_credentials(
+ self, data: dict[str, Any], **kwargs: Any
+ ) -> None:
+ """Validate authored credentials with the effective connection's
engine."""
+ value = data.get("masked_encrypted_extra")
+ if not value or not (
+ is_feature_enabled("EMBEDDED_CREDENTIAL_FALLBACK")
+ and is_feature_enabled("EMBEDDED_SUPERSET")
+ ):
+ return
+
+ encrypted_extra = json.loads(value)
+ if EMBEDDED_CREDENTIALS_KEY not in encrypted_extra:
+ return
+
+ engine_spec = BaseEngineSpec
+ stored = (
+ db.session.get(Database, self.database_id)
+ if self.database_id is not None
+ else None
+ )
+ if sqlalchemy_uri := data.get("sqlalchemy_uri"):
+ engine_spec =
Database(sqlalchemy_uri=sqlalchemy_uri).db_engine_spec
+ elif stored:
+ engine_spec = stored.db_engine_spec
+
+ credentials = encrypted_extra[EMBEDDED_CREDENTIALS_KEY]
+ if embedded_credentials_incomplete(
+ credentials,
+ engine_spec.validate_embedded_credentials,
+ ):
+ raise ValidationError(
+ _(
+ "Embedded guest credentials are invalid or incomplete.
Provide "
+ "all required fields or clear them to leave embedded
viewers "
+ "unconfigured."
+ ),
+ field_name="masked_encrypted_extra",
+ )
+
+ # A mask only stands in for a secret the connection has stored.
Anywhere
+ # else `unmask_encrypted_extra` keeps it and saves it as the
credential.
+ if any(
+ field.startswith(f"$.{EMBEDDED_CREDENTIALS_KEY}.")
+ for field in json.unrevealable_masked_fields(
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)**
Yes, the check should stay out of test connection. Fixed in 2beea0ce00, and
the same change is on #45870 in cccf05251d. `DatabaseTestConnectionSchema` now
sets `masks_need_stored_secret = False`, because it saves nothing and
`TestConnectionDatabaseCommand` does the unmasking. `DatabasePostSchema` and
`DatabasePutSchema` still refuse a mask with no stored secret.
`test_test_connection_leaves_masks_to_the_command` covers both sides.
##########
superset/commands/dataset/export.py:
##########
@@ -137,6 +173,11 @@ def _export(
)
payload["ssh_tunnel"] =
mask_password_info(ssh_tunnel_payload)
+ if carried := _carried_secure_extra(
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)**
Yes, it now merges. Fixed in 2beea0ce00, and the same change is on #45870 in
cccf05251d. When every key in the bundle's `masked_encrypted_extra` is in
`CARRIED_SECURE_EXTRA_KEYS`, `import_database` keeps the rest of the stored
Secure Extra. A database export carries the whole value, so it still replaces.
One limit: a database export holding only those two blocks also merges, so it
won't remove other stored keys on the target. The merge sits in the existing
branch that only runs when the connection identity is unchanged.
`test_import_database_keeps_secure_extra_a_bundle_does_not_carry` covers both
bundle shapes.
##########
superset/models/core.py:
##########
@@ -743,7 +795,10 @@ def _get_sqla_engine( # pylint: disable=too-many-locals
# noqa: C901
effective_username = self.get_impersonation_username(sqlalchemy_url)
- oauth2_config = self.get_oauth2_config()
+ embedded_fallback = self.embedded_fallback_applies()
+ # Embedded guests do not use OAuth2 tokens or redirect URLs, including
when
+ # a worker restores their identity without a request context.
+ oauth2_config = None if embedded_fallback else self.get_oauth2_config()
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)**
Yes, the refusal now runs first. Fixed in 2beea0ce00, and the same change is
on #45870 in cccf05251d. `_get_sqla_engine` checks
`embedded_guest_missing_oauth2_client()` before `get_oauth2_config()`, so an
incomplete client gets the refusal and its server-log warning.
`test_a_guest_on_an_incomplete_oauth2_client_is_refused` covers it, using a
client with only an `id`.
##########
superset/databases/utils.py:
##########
@@ -160,3 +162,86 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username,
auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. Engines may override this default through
+ ``BaseEngineSpec.validate_embedded_credentials`` for both query-time and
+ authoring-time validation.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not _is_present(username):
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not _is_present(privatekey_body)
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not
password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(
+ value: Any,
+ validate_credentials: Callable[[Any], dict[str, str] | None],
+) -> bool:
+ """
+ Is an embedded credential malformed or incomplete for its engine?
+
+ Absent and entirely blank values pass -- they mean "no credential
configured".
+ Accepts the value as a dict or as the JSON string the connection dialog
posts.
+ Use the engine's query-time validator when the connection is known, keeping
+ authoring checks consistent with engine-specific credential shapes.
+ """
+ if value is None or value == "":
+ return False
+
+ if isinstance(value, str):
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)**
Rejected rather than normalized. Fixed in 2beea0ce00, and the same change is
on #45870 in cccf05251d. The API schemas now pass `accept_json_string=False`,
so a non-blank credential sent as a JSON string gets a 400 on save. Blank
values still pass. `ValidateDatabaseParametersCommand` still accepts a string
because `validate_test.py` pins that, so `validate_parameters` can pass a value
that saving then refuses. The stringified object is now a case in
`test_api_rejects_malformed_embedded_credentials`.
##########
tests/unit_tests/charts/commands/data/test_get_data_command.py:
##########
@@ -445,3 +445,28 @@ def
test_get_query_handles_parsing_error_with_null_sql_value() -> None:
assert "query" not in result
assert result["error"] == "Parsing error occurred"
assert result["language"] == "sql"
+
+
+def test_database_error_while_building_the_query_is_a_failed_query() -> None:
+ """
+ A connection failure while the query is being built -- a virtual dataset's
+ default-schema lookup connects -- is reported as a failed query (a 400),
+ not left to escape as an unexpected server error (an HTML 500).
+ """
+ from sqlalchemy.exc import OperationalError
+
+ mock_query_context = Mock(spec=QueryContext)
+ mock_query_context.result_type = ChartDataResultType.FULL
+ mock_query_context.get_payload.side_effect = OperationalError(
+ "connect", {}, Exception("Incorrect username or password was
specified.")
+ )
+ database = Mock()
+ database.db_engine_spec.extract_error_message.return_value = (
+ "Incorrect username or password was specified."
+ )
+ mock_query_context.datasource = Mock(database=database)
+
+ with pytest.raises(ChartDataQueryFailedError) as excinfo:
+ ChartDataCommand(mock_query_context).run()
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)**
Done. Fixed in 2beea0ce00, and the same change is on #45870 in cccf05251d.
The test is now parametrized over `run` and `execute`, with
`get_payload_result.side_effect` set to the same `OperationalError` and the
same message assertion.
--
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]
