sadpandajoe commented on code in PR #45890:
URL: https://github.com/apache/superset/pull/45890#discussion_r4444880970
##########
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:
A bundle now carries a `masked_encrypted_extra` holding only the
`oauth2_client_info` and `embedded_credentials` blocks. On an overwrite import,
`import_database` reveals the masks and then stores the whole result as
`encrypted_extra`, so any other Secure Extra the target connection has (for
example Snowflake `auth_method` / `auth_params` key-pair settings) is dropped.
Alerts, reports and the base-account path then lose their credentials after a
dashboard promote with overwrite. Should the import merge the carried blocks
into the stored Secure Extra instead of replacing it?
##########
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:
With both flags on, "Test connection" on an existing Snowflake connection
now returns a 400 as soon as the dialog sends back the masked embedded secret.
`DatabaseTestConnectionSchema` is loaded without a `database_id`
(`DatabasesRestApi.test_connection` calls `.load(request.json)` with no
arguments), so `stored` is always `None`, `unrevealable_masked_fields` reports
every mask as unrevealable, and the request is rejected before
`TestConnectionDatabaseCommand` can look the connection up by name and unmask
it. Does this check need to skip the stored-secret test when no `database_id`
is known, or should it defer to the command's unmasking?
##########
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:
A Secure Extra whose `embedded_credentials` is a JSON string holding a
complete credential (for example
`"{\"username\":\"svc\",\"password\":\"pw\"}"`) passes this check because the
string is decoded locally, but the stored value stays a string. At query time
`validated_embedded_credentials` requires a dict and returns `None`, so the
save succeeds and every embedded chart is then refused. The schema tests in
`schema_tests.py` cover invalid JSON and stringified primitives but not a
stringified object, so nothing pins this. Should a string value be rejected (or
normalized to a dict before it is persisted), with a case added for it?
##########
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:
This only exercises `ChartDataCommand.run()`, but the chart-data API goes
through `execute()`, which has its own new `except SQLAlchemyError` handler in
`get_data_command.py`. The existing `execute()` tests return error payloads
rather than raising during planning, so deleting that handler would leave every
test here passing and a rejected login on a virtual dataset would surface as an
HTML 500 again. Could this be parametrized over `run` and `execute` (as
`test_validation_failure_raises_typed_error` is), with
`get_payload_result.side_effect = OperationalError(...)` for the `execute` case
and the same message assertion?
##########
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:
For a guest on an impersonating connection whose stored `oauth2_client_info`
is incomplete (for example an imported connection with only an `id`),
`_has_embedded_oauth2_client()` is false, so `embedded_fallback` is false and
`self.get_oauth2_config()` runs here. It raises a Marshmallow `ValidationError`
from `schema.load`, so the `elif self.embedded_guest_missing_oauth2_client()`
refusal below is never reached and the viewer gets the generic error with no
refusal warning in the server log, which is the case this PR sets out to
explain. Could the guest refusal be evaluated before the ordinary OAuth2 config
is resolved?
--
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]
