hughhhh commented on code in PR #45890:
URL: https://github.com/apache/superset/pull/45890#discussion_r4457311870
##########
superset/commands/dashboard/importers/v1/__init__.py:
##########
@@ -79,14 +79,24 @@ class ImportDashboardsCommand(ImportModelsCommand):
"charts/": ImportV1ChartSchema(),
"dashboards/": ImportV1DashboardSchema(),
"datasets/": ImportV1DatasetSchema(),
- "databases/": ImportV1DatabaseSchema(),
+ # Related databases are reused rather than overwritten, unless
+ # `overwrite_all` is set (see `__init__`).
+ "databases/": ImportV1DatabaseSchema(overwrites_existing=False),
"themes/": ImportV1ThemeSchema(),
}
import_error = DashboardImportError
def __init__(self, contents: dict[str, str], *args: Any, **kwargs: Any) ->
None:
self.overwrite_all = kwargs.pop("overwrite_all", False)
super().__init__(contents, *args, **kwargs)
+ # With both flags set, `_import` overwrites related databases, so their
+ # schema must ask for any secret the stored connection cannot reveal.
+ self.schemas = {
+ **self.schemas,
+ "databases/": ImportV1DatabaseSchema(
+ overwrites_existing=self.overwrite and self.overwrite_all
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)**
Fixed in `6d02100f3f`. `validate_masked_encrypted_extra` now also checks
`can_write` on Database next to `overwrites_existing`, so validation and
`import_database` make the same decision. This sits in the schema, so it covers
both the dashboard and `/api/v1/assets/import/` paths.
`test_imports_without_database_write_leave_the_connection_alone` covers both
schemas with an unchanged and a changed URI, and fails if the check is removed.
I tested at the schema and `import_database` level, not with a live Alpha login.
The 500 itself is separate: `_on_import_error` turns every asset validation
failure into `ImportFailedError`. A user who can write databases still gets
that 500, not a prompt, when an asset import needs a secret. I left that
unchanged.
##########
superset/databases/schemas.py:
##########
@@ -1155,26 +1274,40 @@ def validate_masked_encrypted_extra(
"File contains both `encrypted_extra` and
`masked_encrypted_extra`"
)
- if db.session.query(Database).filter_by(uuid=data["uuid"]).first():
- # Existing DB: sensitive values will be revealed from existing
- # encrypted_extra in import_database()
- return
-
masked_encrypted_extra = json.loads(data["masked_encrypted_extra"])
- # Determine engine spec from sqlalchemy_uri to get sensitive fields
- sqlalchemy_uri = data["sqlalchemy_uri"]
- url = make_url_safe(sqlalchemy_uri)
- backend = url.get_backend_name()
- driver = url.get_driver_name()
- db_engine_spec = get_engine_spec(backend, driver=driver)
-
- # Check if any sensitive field is still masked
- masked_fields = get_masked_fields(
- masked_encrypted_extra,
- db_engine_spec.encrypted_extra_sensitive_field_paths(),
+ existing =
db.session.query(Database).filter_by(uuid=data["uuid"]).first()
+ if existing and not self.overwrites_existing:
+ return
+
+ # The incoming URI decides the engine, and so which fields are secret:
an
+ # overwrite can change it, and the stored engine's paths would miss
them.
+ db_engine_spec = Database.get_db_engine_spec(
+ make_url_safe(data["sqlalchemy_uri"])
)
+ if existing:
+ # Existing DB: masked values are revealed from its stored
+ # encrypted_extra in import_database(). Any the stored payload has
no
+ # value for -- e.g. a connection promoted without its OAuth2
client or
+ # embedded credential -- would otherwise be stored as the mask
itself,
+ # so ask for those like a new DB. Nothing is revealed onto a
+ # connection the import moves, so then ask for every masked value.
+ masked_fields = json.unrevealable_masked_fields(
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)**
Fixed in `6d02100f3f`. A stored `oauth2_client_info` no longer counts as
revealable when the file names a different `id`, and the same goes for
`embedded_credentials` with a different `username`, including its key-pair
secrets. Each case now gets the usual prompt.
The check is in the shared schema, so it also changes database-file imports,
which is noted in `UPDATING.md`. A hand-written block with no `id` or
`username` still reveals, because there is nothing to compare. Tests:
`test_import_schema_asks_for_a_secret_when_the_file_names_someone_else` and
`test_import_schema_reveals_a_secret_for_the_same_identity`.
Also fixed here, for your +1 on the `json.py:395` note: a stored `None` or
empty string now counts as revealable, and only a stored mask counts as missing
(`test_import_schema_reveals_a_stored_null_secret`).
##########
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
+ # entry -- nor, since the async `task_key` is the query cache key, a SHARED
+ # chart task -- with a logged-in user's per-user OAuth2 result. That is
+ # correctness rather than a caching preference, so it is checked ahead of,
and
+ # independently of, the opt-in flags.
+ #
+ # The engine spec owns the key material -- see
+ # `BaseEngineSpec.get_embedded_credential_cache_key` for why it is keyed
on the
+ # configured identity rather than on `g.user.username`.
+ # A separate field prevents collisions with a logged-in username that
equals
+ # the engine-provided guest key.
+ if embedded_credentials := database.get_embedded_fallback_credentials():
+ cache_dict["embedded_credentials_key"] = (
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)**
Fixed in `6d02100f3f`. `_annotation_cache_context` now adds the source
connection's embedded identity as `source_embedded_identity`, for `SqlaTable`
and `Query` sources. Swapping the credential changes the key, and clearing it
gives the missing-credential marker, so a warm entry can't skip the refusal.
The material comes from a new `embedded_guest_cache_key` helper rather than
`add_impersonation_cache_key_if_needed`, and the component is left out when
empty, so non-guest keys don't change. Semantic view sources aren't covered,
because they don't have a `Database` connection here. Test:
`test_annotation_cache_key_binds_the_source_connection_embedded_identity`.
##########
docs/sip/embedded-fallback-credential.md:
##########
@@ -0,0 +1,291 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+# SIP: Embedded-only fallback credential on OAuth2 database connections
+
+## [DRAFT — proposal for discussion]
+
+This document is a draft proposal accompanying the code in this PR. It is
intended to
+seed the formal SIP discussion. The code is complete and tested, gated behind
a new
+feature flag that defaults to `False`, and changes nothing for any existing
deployment
+until that flag is turned on.
+
+## Motivation
+
+Per-user database OAuth2 and embedded dashboards do not currently compose.
+
+When a database connection authenticates users individually via OAuth2,
Superset
+resolves a per-user token for the requesting principal and uses it to connect.
An
+embedded viewer, however, signs in with a guest token rather than a Superset
account.
+There is no Superset user, so there is no per-user token to resolve — and an
external
+viewer (a customer, a partner, a member of the public) frequently has no
identity on the
+analytical database at all, so there is no token that *could* be minted for
them.
+
+The result today is that embedded dashboards simply cannot run queries against
a
+per-user OAuth2 connection. Until the companion fix in
+`fix(oauth2): don't start the OAuth2 dance for embedded guests`, the failure
was also
+unhelpful: Superset tried to start an authorization flow the guest cannot
complete,
+read `g.user.id` on a principal that has none, and returned an HTTP 500
instead of the
+driver's error.
+
+This is not a hypothetical. It is the shape of every deployment that embeds
dashboards
+for external viewers while moving internal users onto per-user OAuth2 — a
direction
+several warehouses, Snowflake among them, actively push customers toward.
+
+The workarounds available today are all worse:
+
+- **Keep a second connection** with service-account credentials for embedded
use. This
+ duplicates every connection-level setting and relies on nothing ever
pointing an
+ internal dashboard at the service-account copy.
+- **Drop per-user OAuth2** for any database that feeds an embedded dashboard,
losing
+ per-user attribution for internal users to serve external ones.
+- **Mint a Superset account per external viewer**, which defeats the purpose
of guest
+ tokens.
+
+## Proposed change
+
+Let a single connection carry a username and either a password or a PEM
private key used
+**only** when the requesting principal is an embedded guest. Logged-in users
continue to
+authenticate per-user via OAuth2, with no route by which they can reach the
stored
+credential.
+
+The credential is stored per-connection in the existing `encrypted_extra`
column
+(`embedded_credentials: {username, password}` or
+`{username, auth_method: "keypair", privatekey_body, privatekey_pass}`) and
configured
+through the connection dialog like any other connection secret. The passphrase
is
+optional. It is **not** configured in `superset_config.py`:
+operators manage connections in the UI, and a credential in config could not
be scoped
+to one connection.
+
+### The gate
+
+One function — `Database.get_embedded_fallback_credentials()` — returns the
credential
+only when all five conditions hold:
+
+1. the deployment opted in via `EMBEDDED_CREDENTIAL_FALLBACK` (default
`False`);
+2. the engine opted in via
`BaseEngineSpec.supports_embedded_credential_fallback`
+ (Snowflake is the only `True`);
+3. the connection stores a **complete** credential — a half-filled one is
ignored
+ rather than half-applied;
+4. the current principal is an embedded guest **and** `EMBEDDED_SUPERSET` is
enabled;
+5. the connection has an OAuth2 client configured **and** `impersonate_user`
enabled,
+ keeping this a fallback for the per-user flow rather than a second identity
on an
+ ordinary connection. Deployment-wide client configuration is checked
independently
+ of request context so restored async guests use the same credential.
+
+Condition 4 checks the feature flag explicitly rather than relying on
+`get_current_guest_user_if_guest()`, which is a bare `isinstance` check. A
`GuestUser`
+can only reach `g` through the guest-token request loader, which is itself
gated — but a
+security boundary should not rest on that implication.
+
+Conditions 1, 2, 4 and 5 — everything except the stored credential — are also
exposed on
+their own as `Database.embedded_fallback_applies()`. That split is what lets
+`_get_sqla_engine` tell two cases apart that otherwise look identical: *this
request has
+nothing to do with the fallback*, and *this is an embedded guest on a
connection that has
+no credential for them*. See **Refusing rather than falling back** below.
+
+### Refusing rather than falling back
+
+`_get_sqla_engine` applies the fallback *instead of* impersonation. An
embedded viewer
+on a per-user OAuth2 connection with no (or a half-filled) credential must not
query
+as the connection's base account. Neither the viewer nor the dashboard author
can see
+that identity change. Connections with impersonation disabled retain their
ordinary
+service-account behavior and do not use the fallback.
+
+With `EMBEDDED_CREDENTIAL_FALLBACK` on, that case is refused instead. So is a
guest on
+a connection with impersonation enabled but no OAuth2 client at all: on
Snowflake,
+impersonation without OAuth2 never changed the login, so such a connection
served
+guests under its base account, and with the flag on it stops serving them
until an
+administrator either turns impersonation off or configures an OAuth2 client
and an
+embedded credential. The refusal is scoped to the flag so that a deployment
which has
+not opted in keeps its existing behaviour.
+
+The refusal reaches only guests, whose error responses are redacted to a
generic
+message unless Superset authored the error type. It is therefore raised as a
+`DATABASE_SECURITY_ACCESS_ERROR` with a fixed message that names nothing about
the
+connection, so the viewer learns the chart's connection isn't set up for
embedded
+dashboards. The administrator's explanation, naming the connection and the
fix, goes
+to the server log. Under `GLOBAL_ASYNC_QUERIES` the task keeps only an untyped
message,
+so the viewer gets the generic error instead; the server log is the same.
+
+The connection dialog removes the ways to reach that state in the first place:
the
+section is offered only when both flags are on *and* the connection uses
OAuth2, a
+malformed or incomplete effective credential is rejected on save while both
flags are
+enabled, and a save with impersonation off is refused until the credential is
+cleared, because it would be inactive. Form edits replace the stored
credential before
+completeness is checked. The save/test schemas and parameter-validation
command use the
+engine's `validate_embedded_credentials` hook, matching query-time rules even
for custom credential shapes. The default rule
+rejects whitespace-only usernames without trimming valid credential values.
+Disabling either flag leaves stored credentials intact and skips completeness
checks,
+allowing unrelated edits to imported connections. The backend refusal is the
backstop
+for the paths that do not go through the dialog.
+
+### Cache identity
+
+The fallback changes the connection identity without going through
impersonation, and
+the DATA cache key has no acting-user component of its own —
+`add_impersonation_cache_key_if_needed` only contributes one under
`CACHE_IMPERSONATION`,
+`CACHE_QUERY_BY_USER` or `per_user_caching`. Left alone, an embedded guest and
a
+logged-in user would compute the same `query_cache_key` for a physical dataset
with no
+RLS and no Jinja, and read each other's rows. Because that key is also the
`task_key` of
+the SHARED async chart task, they would additionally dedup onto one task,
letting
+whichever arrived first choose the credential for both.
+
+So a guest-run query contributes the engine-provided key (by default,
+`embedded:<configured username>`) in a dedicated `embedded_credentials_key`
field,
+distinct from the logged-in `impersonation_key` even when their values match.
This applies
+unconditionally — this is correctness, not a caching preference. The key comes
from the
+stored credential rather than from `g.user.username`, which a guest token's
minter
+controls. All guests on the connection share one entry, which is correct: they
run as one
+database user, and guest-token RLS already separates them.
+
+A guest with missing or incomplete credentials contributes a separate
+`embedded_credentials_missing` marker. That partition cannot reuse logged-in or
+configured-fallback cache entries or async tasks, so warming the cache does
not bypass
+the refusal at engine creation.
+
+### Why impersonation is skipped, not extended
+
+On the fallback path `Database._get_sqla_engine` sets the configured username
on the
+SQLAlchemy URL, adds its password there when password authentication is
selected, and
+does **not** call `impersonate_user`. For key-pair authentication, the
Snowflake engine
+spec converts the stored PEM key into `connect_args.private_key`. Skipping
impersonation
+is deliberate and load-bearing twice over:
+
+1. An engine that opts in sets its OAuth2 authenticator during impersonation
whether or
+ not a token exists. Skipping impersonation is what lets the stored
credential
+ authenticate at all.
+2. `get_effective_user` resolves to `g.user.username`, and a `GuestUser`
carries
+ whatever username the guest token claims. Whoever mints the token controls
that
+ claim, so routing the fallback through impersonation would let the token
choose the
+ connection identity. A test pins this specifically: a token minted with
+ `ACCOUNTADMIN` does not reach the connection.
+
+After applying fallback credentials, `DB_CONNECTION_MUTATOR` receives the
selected
+connection username, keeping a hook that forwards that argument from
reintroducing the
+guest token's username claim.
+
+The engine cache key includes both the URL and the final engine kwargs. A
password
+changes the URL; a PEM key changes `connect_args`. Tests assert that a guest
and a
+logged-in user produce distinct cache entries.
+
+## Security model
+
+This is the part worth the most scrutiny, so it is stated plainly rather than
left to be
+discovered.
+
+`SECURITY.md` treats the embedded guest token as a distinct principal with no
Superset
+account and no database identity. This proposal grants that principal the
ability to run
+queries **as one operator-configured database user**, on connections where an
+administrator has explicitly configured it, in deployments that have
explicitly enabled
+it.
+
+What that does and does not mean:
+
+- **Per-viewer database-side authorization does not apply.** Every embedded
viewer of
+ that connection queries as the same database user, so warehouse-side
row-level
+ security or masking policies keyed to the connecting user cannot distinguish
viewers.
+ Per-viewer restrictions must come from Superset's own guest-token RLS rules,
which
+ continue to apply unchanged. The documentation says this in the help text
next to the
+ field, not only in prose.
+- **It does not widen what a guest can reach in Superset.** Dashboard, chart
and dataset
+ access are still governed entirely by the guest token's `resources` and the
embedded
+ dashboard's configuration. This changes only which credential the query runs
under.
+- **It is not reachable by a logged-in user.** Conditions 1–5 are evaluated on
every
+ engine creation, and the engine cache cannot leak a guest engine to a
logged-in user.
+- **An embedded viewer on a per-user OAuth2 connection cannot reach the base
credential
+ either.** Previously a guest with no applicable credential connected with
the URL's own
+ username and password. With the flag on, that is refused. See *Refusing
rather than
+ falling back* above.
+- **Guest results do not cross into logged-in results.** The fallback
contributes its own
+ cache identity, so a shared DATA-cache entry — or a shared async task —
cannot serve a
+ logged-in user rows fetched under the guest credential, or the reverse.
Guests without
+ credentials also receive a separate partition. See *Cache
+ identity* above.
+- **The credential is never exposed.** The password, PEM private key, and
optional
+ passphrase are in Snowflake's `encrypted_extra_sensitive_fields`, so they
mask to
+ `XXXXXXXXXX` on read and are restored from storage when the mask is
re-saved. The
+ username is deliberately left visible so an administrator can audit which
database
+ user embedded queries run as.
+
+The honest summary: this is the same trust decision an operator makes today
when they
+point a connection at a service account, narrowed to apply only to embedded
guests and
+only where explicitly configured. It does not create a new class of
credential, and it
+does not let a token holder choose an identity.
+
+## New or changed public interfaces
+
+- **New feature flag** `EMBEDDED_CREDENTIAL_FALLBACK`, default `False`.
+- **New engine-spec attribute**
`BaseEngineSpec.supports_embedded_credential_fallback`,
+ default `False`; `SnowflakeEngineSpec` overrides it to `True`.
+- **New `Database` methods** `get_embedded_fallback_credentials()`,
+ `embedded_fallback_applies()` and `validated_embedded_credentials()`.
+- **New `encrypted_extra` key** `embedded_credentials`, supporting password
and key-pair
+ credentials, with their secrets added to Snowflake's sensitive-field paths.
+- **New connection-dialog field** for engines that opt in, rendered only when
both
+ `EMBEDDED_CREDENTIAL_FALLBACK` and `EMBEDDED_SUPERSET` are on and the
connection uses
+ OAuth2.
+- **New validation** rejecting malformed or incomplete `embedded_credentials`
on
+ `POST`/`PUT /api/v1/database/` and on `validate_parameters` while both
embedding flags
+ are enabled. Import is deliberately not tightened, so existing exports still
load.
+
+No database migration: the credential lives in the existing `encrypted_extra`
column.
+
+## Migration plan and compatibility
+
+Nothing changes for any existing deployment. The flag defaults to `False`;
with it off,
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)**
Fixed in `6d02100f3f`. The SIP's compatibility section now states the one
change that applies whatever the flag, and tells operators to upgrade import
destinations first, matching `UPDATING.md`. I also removed the round-trip claim
from the kill-switch paragraph, and the interface list now describes
`validated_embedded_credentials()` as a helper in `superset/databases/utils.py`.
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -1799,7 +1834,8 @@ const DatabaseModal:
FunctionComponent<DatabaseModalProps> = ({
id={`encrypted_extra_${field.path}`}
name={`encrypted_extra_${field.path}`}
required
- visibilityToggle
+ visibilityToggle={!isPrivateKeyField(field.path)}
+ renderAsTextArea={isPrivateKeyField(field.path)}
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)**
Added in `6d02100f3f`: `keeps the newlines of a private key asked for by an
import` in `DatabaseModal/index.test.tsx`. It imports a bundle asking for
`$.embedded_credentials.privatekey_body` and `privatekey_pass`, pastes a
multi-line PEM, and asserts a textarea for the key, a password input for the
passphrase, and unchanged newlines in the submitted `encrypted_extra_secrets`.
It fails if `renderAsTextArea` is removed.
##########
superset/db_engine_specs/snowflake.py:
##########
@@ -370,6 +424,52 @@ def impersonate_user(
return url, engine_kwargs
+ @classmethod
+ def apply_embedded_credentials(
+ cls,
+ database: Database,
+ credentials: dict[str, str],
+ url: URL,
+ engine_kwargs: dict[str, Any],
+ ) -> tuple[URL, dict[str, Any]]:
+ """
+ Apply an embedded guest credential, by password or by key pair.
+
+ Called instead of :meth:`impersonate_user`, which above sets
+ ``authenticator=oauth`` whether or not a token exists -- skipping it
is what
+ lets a stored credential authenticate at all, and is why this engine
opts
+ into the fallback.
+
+ The base implementation puts the username and, for password
authentication,
+ the password on the URL. A key pair cannot go there: the driver takes
it as
+ DER bytes in ``connect_args``.
+
+ Guests run with the fallback user's default role, not the connection's
+ ``role``: the connection form always writes one into the URI, and a
+ least-privilege fallback user -- or a programmatic access token
restricted
+ to its own role -- is not granted it, so Snowflake would refuse the
session.
+ """
+ url, engine_kwargs = super().apply_embedded_credentials(
+ database, credentials, url, engine_kwargs
+ )
+ # Everything that signs in as the connection's own account goes: the
+ # driver merges the URI query and `connect_args` into its arguments,
so a
+ # `user`/`password`, key or token left in either would win over the
+ # embedded account. `authenticator=SNOWFLAKE_JWT` would also make the
+ # driver expect a key pair from a password guest; it picks key-pair
+ # sign-in itself when the embedded credential supplies `private_key`.
+ url = url.difference_update_query(_CONNECTION_IDENTITY_ARGUMENTS)
+ connect_args = engine_kwargs.get("connect_args", {})
+ for argument in _CONNECTION_IDENTITY_ARGUMENTS:
+ connect_args.pop(argument, None)
+
+ if credentials.get("auth_method") == "keypair":
+ engine_kwargs.setdefault("connect_args", {})["private_key"] = (
+ cls._load_private_key(credentials)
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)**
Yes, fixed in `6d02100f3f`. `apply_embedded_credentials` now catches
`ValueError`, `TypeError` and `UnsupportedAlgorithm` from `_load_private_key`
and goes through the same refusal as a missing credential, so the viewer gets
the fixed message with a 400. The admin gets a warning naming the connection,
the exception class and the fix. Only the class is logged, never the library's
message or any key material.
To call it from the engine spec, I renamed `_refuse_embedded_guest` to
`refuse_embedded_guest`. Test:
`test_embedded_keypair_that_cannot_load_refuses_the_guest`.
--
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]
