hughhhh commented on code in PR #45890:
URL: https://github.com/apache/superset/pull/45890#discussion_r4448027236
##########
UPDATING.md:
##########
@@ -24,6 +24,47 @@ assists people when migrating to a new version.
## Next
+- `EMBEDDED_CREDENTIAL_FALLBACK` (new, off by default) changes behaviour for
embedded
+ guests on database connections that authenticate users individually via
OAuth2 with
+ **Impersonate logged in user** enabled. Ordinary service-account connections
retain
+ their existing behavior, even with a deployment-wide OAuth2 client
configured. With
+ the flag on, a guest whose request has no applicable embedded credential —
none
+ configured on the connection, or one missing its username, password, or
private key
+ required by the selected authentication method — is refused instead of
connecting
+ with the credential in the connection's own SQLAlchemy URI. The viewer sees a
+ `DATABASE_SECURITY_ACCESS_ERROR` saying the chart's connection isn't set up
for
+ embedded dashboards (with `GLOBAL_ASYNC_QUERIES` on, a generic error
instead); the
+ server log names the connection and how to fix it.
+ **Embedded dashboards that stop loading when the flag is turned on:** any
dashboard
+ whose charts use a Snowflake connection with **Impersonate logged in user**
on but no
+ OAuth2 client (none on the connection and no `DATABASE_OAUTH2_CLIENTS` entry
for
+ Snowflake) stops serving embedded viewers, even though impersonation never
changed
+ the login on such a connection and those viewers were served under its base
+ account. Logged-in users are unaffected. Before turning the flag on, update
each
+ such connection: either turn off **Impersonate logged in user**, which keeps
serving
+ everyone under the connection's own credential as before, or configure an
OAuth2
+ client and **Embedded guest credentials**. Queries that do run under an
embedded credential
+ get a dedicated cache partition distinct from every logged-in username,
including
+ a separate partition for guests without credentials, so they no longer share
a
+ DATA-cache entry (or, under `GLOBAL_ASYNC_QUERIES`, a deduplicated chart
task) with a logged-in user's
+ per-user result; existing cached entries for such charts are effectively
invalidated
+ for guests. A malformed or incomplete `embedded_credentials` is rejected by
+ `POST`/`PUT /api/v1/database/` and by `validate_parameters` rather than
stored and
+ ignored while both embedding flags are enabled; asset import is unchanged.
Validation
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)**
Rather than only documenting it, I removed the cause. Fixed in d1272877ed,
and the same change is on #45870 in f02cc08e08. The marker is gone, so exports
no longer add a field an earlier release rejects. `import_database` now takes
`partial_secure_extra`, and only `ImportDashboardsCommand` passes it, since
that's the only import that overwrites from a partial bundle. Chart, dataset
and saved-query imports don't overwrite the connection, and an assets export
still writes the full database file first.
`test_dashboard_import_overwriting_all_keeps_what_the_bundle_does_not_carry`
replaces the earlier marker test and runs the real command. UPDATING now says
exports carry these blocks whatever the flag, and that a destination on an
earlier release replaces its whole Secure Extra with them on overwrite, so
destinations should upgrade first. It also says the save check doesn't apply to
asset import, instead of "unchanged".
##########
superset/utils/cache_keys.py:
##########
@@ -38,6 +38,38 @@ def add_impersonation_cache_key_if_needed(
Add a per-user cache-key when the DB connection is configured for
per-user caching, no-op otherwise.
"""
+ # The embedded fallback credential replaces the connection identity
*without*
+ # going through impersonation (`Database._get_sqla_engine`), so none of the
+ # conditions below would notice it. Its result must never share a
DATA-cache
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)**
Yes. Fixed in d1272877ed, and the same change is on #45870 in f02cc08e08.
`_column_values_cache_key` now calls `add_impersonation_cache_key_if_needed`
for `SqlaTable` and `Query` datasources, so guests get their own partition, and
a guest with no usable credential can't hit a warm entry. Logged-in users are
only separated from each other when per-user caching is on, matching chart
data. Other keys keep their bytes unless one of those conditions applies.
`test_embedded_credential_partitions_column_values_cache_key` covers the guest
case. The semantic view path still has no identity in its key, and I haven't
looked at whether it needs one.
--
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]
