sadpandajoe commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4450789659
##########
superset/charts/api.py:
##########
@@ -1762,6 +1769,7 @@ def import_(self) -> Response:
ssh_tunnel_priv_key_passwords = parse_json_form_field(
"ssh_tunnel_private_key_passwords"
)
+ encrypted_extra_secrets =
parse_json_form_field("encrypted_extra_secrets")
Review Comment:
`encrypted_extra_secrets` is only checked to be a JSON object at the top
level, but `load_configs()` calls `.items()` on each per-file value. A request
carrying `encrypted_extra_secrets={"databases/db.yaml": "secret"}` for a bundle
with a masked Secure Extra parses fine and then raises an `AttributeError`, so
the client gets an HTTP 500 instead of a 400/422. The same unchecked parse is
repeated in the dashboard, dataset and saved-query import routes. Should the
per-file values be validated as `dict[str, str]` before the command runs?
##########
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 test goes through `run()`, but the chart data API calls `execute()`,
which has its own new `except SQLAlchemyError` branch. If that branch were
removed, this test would still pass and chart requests would go back to
surfacing the connection failure as an unhandled 500. Could you add a case
where `get_payload_result` raises `OperationalError` and `execute()` is
expected to raise `ChartDataQueryFailedError` with the extracted message?
##########
superset/utils/json.py:
##########
@@ -362,6 +367,37 @@ def get_masked_fields(
return masked
+def unrevealable_masked_fields(
+ old_payload: dict[str, Any] | None,
+ new_payload: dict[str, Any],
+ sensitive_fields: set[str],
+) -> list[str]:
+ """
+ Returns masked fields in ``new_payload`` that ``old_payload`` cannot
reveal.
+
+ :func:`reveal_sensitive` swaps a mask back for the stored value at the same
+ path. Where the stored payload has no value there, the mask would be kept
and
+ stored as if it were the secret, so callers that must not do that (imports
+ overwriting a connection) use this to ask for the real value instead. A
+ stored mask counts as no value: earlier releases saved one when an import
+ had nothing to reveal, and revealing it only hands the mask back.
+
+ :param old_payload: The stored payload, if any
+ :param new_payload: The incoming payload, possibly containing masks
+ :param sensitive_fields: The set of fields to check, as JSONPath
expressions
+ :returns: JSONPath expressions for masked fields with nothing to reveal
+ """
+ return [
+ field
+ for field in get_masked_fields(new_payload, sensitive_fields)
+ if old_payload is None
+ or not any(
+ match.value and match.value != PASSWORD_MASK
Review Comment:
`match.value and ...` treats a stored `null` as no value. Before this PR,
`redact_sensitive` masked `null` fields too, so an older Snowflake export of an
unencrypted-key connection carries `auth_params.privatekey_pass` as the mask,
and `reveal_sensitive` used to restore the stored `null` from it. Now,
overwriting that connection from the same archived export lists the passphrase
as unrevealable, and the import dialog requires a non-empty answer for it, so
the import cannot be completed without inventing a passphrase. Should a stored
`None` count as revealable here, with only a stored mask treated as missing?
##########
docs/docs/using-superset/embedding.mdx:
##########
@@ -214,6 +321,7 @@ One caveat when the host page lives on a different site
than Superset: the defau
- **Row-level security** — pass `rls` rules in the guest token request to
restrict which rows are visible to the embedded user. Semantic views cannot
enforce guest-token SQL clauses, so queries and cached-result reads are
rejected when a global rule or a rule scoped to the semantic view applies. For
hosts, a global guest rule deliberately refuses every semantic view, and a
dataset-scoped rule can also refuse a semantic view even when the rule was
intended for a SQL dataset. Read permissions do not override these
restrictions. Semantic views remain accessible when no guest rule applies; SQL
datasets continue to enforce their applicable rules.
- **Allowed domains** — restrict which host origins can embed a dashboard by
setting **Allowed Domains** per-dashboard in the _Embed_ settings modal.
Superset checks the request's `Referer` header against this list before serving
the embedded view; an empty list allows any origin, so configure this
explicitly for production.
- **Redacted errors** — API responses to a guest token report a generic `An
error occurred while fetching the data.` instead of the underlying error, since
engine errors quote catalog, schema, table and column names. Errors Superset
raises itself — access denials, timeouts, payload validation — keep their
message, and the full error is always available in the server logs.
+- **OAuth2 connections and guests** — a guest cannot complete an OAuth2
authorization, so Superset does not start one for them, and an embedded viewer
on an OAuth2 connection with no applicable credential is never sent into a
login flow they cannot finish. With `EMBEDDED_CREDENTIAL_FALLBACK` off, the
connection attempt fails with the database driver's authentication error. With
it on, Superset refuses the query before connecting. Either way the viewer sees
the generic `An error occurred while fetching the data.` message, and the
reason is in the server logs. To serve embedded dashboards from such a
connection, see [`EMBEDDED_CREDENTIAL_FALLBACK`](#embedded_credential_fallback).
Review Comment:
"With it on, Superset refuses the query before connecting" reads as true for
every OAuth2 connection, but the refusal only applies when the engine opts in
(Snowflake) and impersonation is on. An operator enabling the flag for, say, a
Trino OAuth2 connection will still go through the ordinary connection path and
not get the early refusal this describes. Can this sentence name those two
conditions?
--
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]
