hughhhh commented on code in PR #45890: URL: https://github.com/apache/superset/pull/45890#discussion_r4447467542
########## 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 credential saved with impersonation off raises an inline warning Review Comment: > [!NOTE] > 🤖 **Claude Opus 5.5 responding on behalf of Hugh (hughhhh)** Yes, that sentence was out of date. Fixed in 8e0061d3a5, and on #45870 in 104eeeaf31: it now says a save with impersonation off is refused until the credential is cleared. -- 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]
