sadpandajoe commented on code in PR #45890: URL: https://github.com/apache/superset/pull/45890#discussion_r4447444650
########## 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: This describes a credential saved with impersonation off as only raising an inline warning, but the schema now rejects that save ("Embedded guest credentials only apply to connections that use OAuth2 with \"Impersonate logged in user\" turned on"), and `embedding.mdx` says the same. An admin following this SIP to keep the credentials while turning impersonation off hits a blocking error on save. Should this sentence say the save is refused until the credentials are cleared? ########## docs/docs/using-superset/embedding.mdx: ########## @@ -172,6 +172,164 @@ When enabled, the **Logout** menu item is removed from the user avatar dropdown Must be `True` to enable the embedded SDK and the guest token endpoint. Without this flag, `embedDashboard` will fail to load. +### `EMBEDDED_CREDENTIAL_FALLBACK` + +Allows a database connection that authenticates logged-in users per-user via OAuth2 to +also carry a single Snowflake username with either a password or a PEM key pair, used +**only** for embedded guest requests. + +This exists because the two authentication models do not meet. 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 may have no identity on the +analytical database at all. Without a fallback, embedded dashboards on a per-user OAuth2 +connection cannot run queries. + +```python +# superset_config.py +FEATURE_FLAGS = { + "EMBEDDED_SUPERSET": True, + "EMBEDDED_CREDENTIAL_FALLBACK": True, +} +``` + +With both flags on, engines that support it (currently Snowflake) show an **Embedded +guest credentials** section on the **Basic** tab of the connection dialog. The section +is editable only when the connection has an OAuth2 client and **Impersonate logged in +user** is on; otherwise it is disabled and states what is missing. + +#### Before turning the flag on + +Turning the flag on stops embedded dashboards on some existing connections from +loading for embedded viewers. A Snowflake connection with **Impersonate logged in +user** on but no OAuth2 client (none in its **OAuth2 client information** and no +`DATABASE_OAUTH2_CLIENTS` entry for Snowflake) served embedded viewers under its base +account, because impersonation without OAuth2 never changed the Snowflake login. With +the flag on, Superset refuses those viewers rather than serve them under an identity +nobody chose for them. The viewer sees *This chart can't be shown here because its +database connection isn't set up for embedded dashboards*, and the server log has a +`Refused an embedded viewer's query on database connection ...` warning naming the +connection and the fix. With `GLOBAL_ASYNC_QUERIES` on, the viewer sees the generic +*An error occurred while fetching the data.* instead, and the warning is the same. +Logged-in users are not affected. + +Update each such connection before turning the flag on, in one of two ways: + +- **Keep serving embedded viewers under the base account:** on the **Advanced** tab, + under **Security**, turn off **Impersonate logged in user**. On a connection with no + OAuth2 client this changes nothing for logged-in users, who were already on the + base account. +- **Serve embedded viewers under a dedicated account:** configure an OAuth2 client and + **Embedded guest credentials** as described below. Logged-in users then sign in to + Snowflake individually through OAuth2. + +#### Setting up the credential + +1. Enable `EMBEDDED_SUPERSET` and `EMBEDDED_CREDENTIAL_FALLBACK` (above). +2. Open the Snowflake connection for editing. On the **Basic** tab, expand **OAuth2 + client information** and enter the client ID, client secret, authorization and token + request URIs, and scope. Alternatively, configure a deployment-wide client for + Snowflake in `DATABASE_OAUTH2_CLIENTS`. +3. On the **Advanced** tab, under **Security**, turn on **Impersonate logged in user**. + On a new connection this option appears only after you connect, so connect first and + then edit the connection. +4. Back on the **Basic** tab, expand **Embedded guest credentials**, enter the Snowflake + username, choose **Password** or **Key pair**, and fill in the secret. +5. Save. The section header shows **Configured** once a credential is stored. + +Configure the Snowflake OAuth2 scopes explicitly to match your integration. For +example, `session:role:ANALYST refresh_token` selects the `ANALYST` role and requests a +refresh token. Omitting a role scope uses the user's default role; the dialog does not +prefill a scope or require a specific role scope. + +For key-pair authentication, enter the Snowflake username, its PEM private key, and an +optional passphrase; register the corresponding public key with that Snowflake user. The +credential is stored encrypted in the connection's *Secure Extra* under +`embedded_credentials`. Passwords, private keys, and passphrases are masked on read, +while the username stays visible so an administrator can see which database user +embedded queries run as. + +The credential must be complete: a username with no password or private key, or the +reverse, is rejected when both embedding flags are enabled. A whitespace-only username +is invalid. Validation uses the engine's credential rules and rejects malformed JSON +or non-object credential values. It checks the edited credential, so an incomplete +imported credential can be completed or cleared. + +To remove the credential, expand **Embedded guest credentials**, click **Remove embedded +guest credentials** (or clear every field), and save. This deletes the +`embedded_credentials` key from *Secure Extra*. Removal stays available while the section +is otherwise disabled. + +#### Requirements and limitations + +- Both `EMBEDDED_SUPERSET` and `EMBEDDED_CREDENTIAL_FALLBACK` must be enabled. While + either is off, a stored credential is preserved, unused, and does not block unrelated + edits. +- The connection needs an OAuth2 client — per-connection **OAuth2 client information** + or a deployment-wide `DATABASE_OAUTH2_CLIENTS` entry — and **Impersonate logged in + user** must be on. Superset refuses to save a credential otherwise, and it never + applies to a connection without OAuth2. +- Embedded viewers run with the embedded account's **default Snowflake role**. The + connection's configured role and authenticator are not used for guest connections. +- The embedded account needs `USAGE` on the connection's warehouse. +- Supported secrets are a password, a Snowflake programmatic access token entered as the + password, or a key pair (PEM private key with an optional passphrase). +- Alerts, reports, connection tests, and logged-in users never use the credential; they + keep using the connection's own credential or their per-user OAuth2 token. +- **Async queries run logged-in users as the base account.** With + `GLOBAL_ASYNC_QUERIES` on, guest queries use the same fallback credential as + synchronous guest queries. Logged-in users' chart queries on a Snowflake OAuth2 + connection, however, run in the async worker under the connection's own base + credential instead of the user's OAuth2 token, so Snowflake grants and row access + policies keyed to the user do not apply and the queries are attributed to the base + account. This predates `EMBEDDED_CREDENTIAL_FALLBACK`, and the flag does not change + it. Leave `GLOBAL_ASYNC_QUERIES` off on deployments that depend on per-user OAuth2. +- **Turn on Per user caching on OAuth2 connections.** Guest results always get their + own cache partition (below), but logged-in users do not unless **Per user caching** + (**Advanced** → **Security**) is on, or the `CACHE_IMPERSONATION` or + `CACHE_QUERY_BY_USER` feature flag is enabled. Without one of them, logged-in users + share one cache entry per query, so a user can be shown a result fetched under + another user's OAuth2 identity, including rows their own Snowflake policies would + hide, until the entry expires. +- **Connections created in SQLAlchemy URI mode** show neither the **OAuth2 client + information** nor the **Embedded guest credentials** section, and an existing + connection cannot be switched to the dynamic form while editing. For such a + connection, set `oauth2_client_info` and `embedded_credentials` directly in + **Secure extra** (**Advanced** → **Security**), or configure a deployment-wide + client in `DATABASE_OAUTH2_CLIENTS`. +- **Turning off Impersonate logged in user moves embedded viewers onto the base + account.** The credential applies only while impersonation is on. With it off, + embedded viewers query under the connection's own credential, and so do logged-in + users, who also stop signing in through OAuth2. A save with impersonation off is Review Comment: This reads as if every save with impersonation off is refused, but `validate_embedded_credentials` in `superset/databases/schemas.py` returns early unless the request carries `masked_encrypted_extra`. A `PUT /api/v1/database/<id>` with only `{"impersonate_user": false}` is accepted and leaves the stored embedded credentials in place, so API-driven admins move viewers onto the base account with no refusal. Should the docs limit the refusal to saves from the connection dialog, or should the update path run the same check? ########## superset/commands/dataset/export.py: ########## @@ -137,6 +173,12 @@ def _export( ) payload["ssh_tunnel"] = mask_password_info(ssh_tunnel_payload) + if carried := _carried_secure_extra( + model.database.masked_encrypted_extra + ): + payload["masked_encrypted_extra"] = carried + payload["masked_encrypted_extra_partial"] = True Review Comment: Nothing in the tests asserts this flag is emitted. `test_bundle_export_carries_secure_extra_masked` checks only the masked payload, and the overwrite test in `import_test.py` sets `masked_encrypted_extra_partial` by hand. If this line were dropped, every unit test would still pass, but a dashboard import with `overwrite` and `overwrite_all` would again replace the whole stored Secure Extra and delete unrelated settings such as Snowflake `auth_params`. Could a test assert the flag in the exported `databases/*.yaml` and feed that bundle through an overwrite import, checking that an unrelated stored key survives? -- 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]
