hughhhh opened a new pull request, #45068:
URL: https://github.com/apache/superset/pull/45068
### SUMMARY
> **This PR accompanies a SIP and is deliberately a draft.** The third
commit introduces
> a new feature, and the contributing guidelines require a `#SIP` before
such code can be
> reviewed or merged. The proposal is drafted in
> `docs/sip/embedded-fallback-credential.md` in this PR. Feedback on the
code is very
> welcome; the design discussion belongs on the SIP.
Per-user database OAuth2 and embedded dashboards do not currently compose.
An embedded viewer signs in with a guest token rather than a Superset
account, so there
is no per-user OAuth2 token to resolve for them — 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. Embedded
dashboards on a
per-user OAuth2 connection therefore cannot run queries, and today they fail
unhelpfully.
Three commits, deliberately separate so they can be reviewed — or split —
independently.
**The first two are plain bug fixes that stand on their own and need no
SIP.**
---
#### 1. `fix(oauth2): don't start the OAuth2 dance for embedded guests`
On any OAuth2-enabled connection — Snowflake, Databricks, Trino, GSheets —
an embedded
guest who hits an authentication failure gets
`AttributeError: 'GuestUser' object has no attribute 'id'` and an HTTP 500,
instead of
the driver's actual error. `start_oauth2_dance` reads `g.user.id` to build
its OAuth2
state and `GuestUser` defines no `id`. More fundamentally, a guest can never
complete the
dance, so starting one is always wrong rather than occasionally fatal.
- **`BaseEngineSpec.needs_oauth2`** returns `False` for guests, covering the
nine call
sites that gate on it, and transitively `execute_with_oauth2_retry`'s four
`start_oauth2_dance()` calls, which all sit behind its `is_oauth2_error`
early-out. The
cheap `isinstance` check moves first so the common not-an-OAuth2-error
case never
reaches the `security_manager` import or the feature-flag lookup.
- **`check_for_oauth2`** needs the check repeated: its condition is
`isinstance(ex, OAuth2TokenRefreshError) or ...needs_oauth2(ex)`, and that
`or` reaches
the dance without consulting `needs_oauth2`. A live path, not a
theoretical one —
Snowflake lists `OAuth2TokenRefreshError` in its `oauth2_exception` tuple.
- **`start_oauth2_dance`** now raises `OAuth2Error` naming the problem
rather than
`AttributeError`, as a backstop for any caller that skips both guards.
Not fixed here but worth flagging: `gsheets.py` reads `g.user.id` directly in
`get_table_names`. A guest is unlikely to reach it, but it is the same
latent bug.
#### 2. `fix(snowflake): allow OAuth2 client config from the connection
dialog`
Snowflake's OAuth2 support is complete on the backend — `supports_oauth2`,
`is_oauth2_enabled`, `get_oauth2_config`, an OAuth2-aware
`impersonate_user`, and
`$.oauth2_client_info.secret` already masked — but unreachable from the UI.
An admin
could only configure it by hand-editing the Secure extra JSON blob.
Two missing pieces, both of which gsheets and bigquery already have:
`oauth2_client_info`
on `SnowflakeParametersSchema`, and
`add_attribute_function(encrypted_field_properties)`
in `parameters_json_schema()`. Together they attach `x-encrypted-extra:
true`, which is
what makes the modal render the existing `OAuth2ClientField` and move the
value into
`masked_encrypted_extra`.
Unlike gsheets the endpoints get no defaults — they live on the customer's
own Snowflake
account, so no URI is right for every deployment. `scope` defaults to
`refresh_token`,
since a refresh token is what makes an OAuth2 connection usable past the
first hour.
Backend-only: `oauth2_client_info` is already in `FormFieldOrder` and
already mapped to
`OAuth2ClientField`. Two leak-prevention properties that held only by
construction —
`build_sqlalchemy_uri` and `update_params_from_encrypted_extra` keeping the
client secret
out of the stored URI and the engine kwargs — are now pinned by test.
#### 3. `feat(db): embedded-only fallback credential on OAuth2 connections`
Lets one connection carry a username and password used **only** when the
requesting
principal is an embedded guest, while logged-in users continue to
authenticate
individually via OAuth2 with no route to the stored credential.
**Behind a new `EMBEDDED_CREDENTIAL_FALLBACK` feature flag, default
`False`**, so nothing
changes for any existing deployment. The credential lives per-connection in
the existing
`encrypted_extra` column — **no migration** — and is configured in the
connection dialog.
`Database.get_embedded_fallback_credentials()` is the single gate and
requires all five
of: the deployment flag; an engine opt-in
(`supports_embedded_credential_fallback`,
Snowflake only); a **complete** stored credential (a half-filled one is
ignored, not
half-applied); an embedded guest principal with `EMBEDDED_SUPERSET` on; and
a connection
that actually uses OAuth2, keeping this a fallback for the per-user flow
rather than a
second identity on an ordinary connection.
**Why impersonation is skipped rather than extended** — load-bearing twice
over:
1. An opted-in engine sets its OAuth2 authenticator during impersonation
whether or not a
token exists (Snowflake sets `authenticator=oauth` unconditionally), so
skipping it 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 — which whoever mints the token controls.
Going
through impersonation would let the token choose the connection identity.
A test is
pinned to exactly this: a token minted with `ACCOUNTADMIN` does not reach
the URL.
The credential is set on the URL rather than in `connect_args` so the
per-process engine
cache key — which has **no** user component — differs between a guest and a
logged-in
user. A test asserts the two produce distinct cache entries.
**Security model, stated plainly.** This grants the embedded-guest principal
the ability
to query **as one operator-configured database user**, on connections where
an admin
configured it, in deployments that enabled it.
- Every embedded viewer of that connection queries as the same database
user, so
warehouse-side RLS or masking keyed to the connecting user cannot
distinguish viewers.
Per-viewer restriction must come from Superset's guest-token RLS rules,
which are
unchanged. The UI says this next to the field, not only in the docs.
- It does **not** widen what a guest can reach in Superset — resource access
is still
governed entirely by the guest token and the embedded dashboard config.
- The password masks on read and is restored from storage when the mask is
re-saved; the
username stays visible so an admin can audit which database user embedded
queries run
as.
- Turning the flag off is a kill switch, not a deletion: the credential
stops being
offered and used but is preserved, so the operation is reversible and
export/import
round-trips.
The full argument, including rejected alternatives, is in the SIP draft.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!-- SCREENSHOTS: drag before-modal.png / after-modal.png / after-bottom.png
here -->
**Before** — the Snowflake connection dialog ends at Warehouse/Role.
**After** — an **OAuth2 client information** section (commit 2) and an
**Embedded guest
credentials** section (commit 3), the latter carrying the warning that every
embedded
viewer shares the credential.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/db_engine_specs/test_base.py
tests/unit_tests/utils/oauth2_tests.py
pytest tests/unit_tests/db_engine_specs/test_snowflake.py
tests/unit_tests/models/core_test.py
cd superset-frontend && npx jest src/features/databases
```
402 backend tests and 222 frontend tests pass. The full unit suite shows the
same set of
failures as the base commit — none introduced.
The guard tests in commit 1 fail against an unpatched tree with exactly the
reported
error, `AttributeError: 'GuestUser' object has no attribute 'id'` at
`base.py:888`, while
the "still works for a logged-in user" tests pass either way. Commit 3's
security
properties are its tests: the decisive one builds two engines from the same
`Database`
row and asserts the guest gets the fallback credential with **no**
`authenticator=oauth`,
while a logged-in user gets neither the fallback username nor its password.
Each of the
five gate conditions has a negative test, including the kill switch on an
otherwise fully
configured connection and the "disabled, not erased" property.
**Not verified, and it cannot be in CI:** whether Snowflake accepts the
credential on a
connection whose client is configured for OAuth2. Everything up to the
connection URL is
proven by test; the handshake needs an account with an OAuth2 security
integration.
Confirmation from anyone who has one would be very welcome.
Manually, with `snowflake-sqlalchemy` installed (the engine does not appear
in the dialog
at all without it) and both flags on:
1. **+ Database → Snowflake.** Both new sections are present.
2. Fill in the OAuth2 client, and a dedicated Snowflake user under
**Embedded guest
credentials**.
3. Embed a dashboard backed by that connection and load it with a guest
token: queries
run as the fallback user.
4. Open the same dashboard logged in: the OAuth2 flow is used as before.
5. Set `EMBEDDED_CREDENTIAL_FALLBACK = False` and restart: the section
disappears,
embedded queries stop using the credential, and the stored value survives.
Note that Superset runs a real connection test on both POST and PUT, so
against an
unreachable host nothing is written — check for the error toast rather than
reading the
metadata DB afterwards.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [x] Required feature flags: `EMBEDDED_CREDENTIAL_FALLBACK` (new, default
off), and
`EMBEDDED_SUPERSET`
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
No DB migration: the credential is stored in the existing `encrypted_extra`
column.
**Happy to split this** — the first two commits are independent bug fixes
and could land
ahead of the SIP discussion if reviewers prefer.
--
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]
