sadpandajoe commented on code in PR #45890:
URL: https://github.com/apache/superset/pull/45890#discussion_r4451749622
##########
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:
With both flags set, this makes the database schema ask for any OAuth2 or
embedded secret the stored connection can't reveal. But `import_database()`
only overwrites when the caller also has `can_write` on Database
(`superset/commands/database/importers/v1/utils.py`); without it, it returns
the existing connection untouched. A role that can import dashboards but not
edit connections, importing with `overwrite=true&overwrite_all=true` onto an
existing database whose Secure Extra lacks the exported blocks, now fails
validation asking for secrets that would never be applied, where that import
used to succeed. Should the schema flag also account for the Database write
permission, so validation and execution share one overwrite decision?
##########
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:
This partitions the chart's own connection, but a chart-backed annotation
layer reads through another dataset's connection, and
`_annotation_cache_context` in `query_context_processor.py` binds only that
source's RLS and the user id. Guests share one user id, so after a guest warms
chart A, an admin who swaps the annotation source's embedded credential for a
less-privileged one (or clears it) still gets A's cached `annotation_data`
replayed, without reaching the refusal. Should the annotation source
connection's embedded discriminator go into the parent key too?
##########
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:
Nothing in `DatabaseModal/index.test.tsx` exercises this renderer; the new
textarea assertions cover `ImportModal`. If `renderAsTextArea` were dropped
here, every existing test would still pass while a pasted multi-line key is
flattened by a single-line input and the stored key can't be loaded. Could you
add a test that imports a database asking for
`$.embedded_credentials.privatekey_body` and its passphrase, 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`?
##########
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:
A key pair with a wrong passphrase or a malformed PEM raises a plain
`ValueError` from `_load_private_key` here, while the engine URL is still being
built. That is before `create_engine`'s error mapping and before
`_log_rejected_embedded_credential` attaches its listener, and save-time
validation only checks that the fields are present. The embedded viewer gets an
unmapped server error instead of the refusal message, and the admin gets a bare
traceback instead of the rejected-credential warning this change adds. Should a
load failure here be turned into the same refusal and warning as a rejected
sign-in?
##########
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:
This says nothing changes for an existing deployment and that export/import
round-trips unchanged with the flag off. But
`superset/commands/dataset/export.py` now writes the carried Secure Extra
blocks whatever the flag, and a destination on an earlier release replaces its
whole Secure Extra on an overwrite import, which UPDATING.md already warns
about. An operator who reads only this section could upgrade the export source
first and lose the destination's unrelated Secure Extra. Could this section
match UPDATING.md? Also, the interface list above names
`validated_embedded_credentials()` as a new `Database` method, but it is a
module-level function in `superset/databases/utils.py`.
--
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]
