You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "hughhhh (via GitHub)" <gi...@apache.org> on 2026/10/07 22:17:17 UTC
[PR] [WIP] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4426418871
##########
superset/commands/dataset/export.py:
##########
@@ -137,6 +173,11 @@ 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
Review Comment:
This puts a partial Secure Extra (only `oauth2_client_info` / `embedded_credentials`) into every dashboard, chart and dataset bundle, but `import_database()` treats `masked_encrypted_extra` as the whole new `encrypted_extra`: `reveal_sensitive` only fills masks, it does not merge the stored blob. Take a Snowflake connection that has `auth_method: keypair` with `auth_params.privatekey_body` plus an `oauth2_client_info` block. Export a dashboard that uses it, then re-import with `overwrite` and `overwrite_all`: the connection's Secure Extra is replaced by just the OAuth2 block, the key pair is gone, and connection tests and background reports stop authenticating. Before this change these bundles carried no Secure Extra, so the stored one was left alone, and this happens with the fallback flag off too. Should the import keep stored blocks the bundle doesn't carry (or only apply this for a full database export)?
##########
superset/translations/messages.pot:
##########
@@ -146,7 +146,7 @@ msgstr ""
msgid "\"%s\" is now the system default theme"
msgstr ""
-#, no-python-format
+#, python-format
Review Comment:
This regeneration flips `% calculation`, `% of parent` and `% of total` from `no-python-format` to `python-format`, and drops about 25 existing `#. do-not-translate` notes elsewhere in the file. These are literal labels, so a catalog tool will read `% c` / `% o` as format specifiers and can reject or mangle valid translations, and translators lose the don't-translate hints. Could the pot be regenerated so it only adds the new strings and keeps the existing flags and notes?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4287864936
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `8277dee`.
The endpoint and scope defaults now use nullish coalescing, so an explicitly cleared empty string is preserved instead of being replaced by the default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `8277dee`.
Credential-shape validation now delegates to the supplied `validate_credentials` function, allowing engine-specific validators to accept alternate complete credential shapes.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #45068:
URL: https://github.com/apache/superset/pull/45068#issuecomment-6498179333
Reproduction for my follow-up inline note on `superset/utils/json.py`: the same local instance accepts the overwrite import on master, then returns a 500 without a secret prompt after upgrading to this PR (flags off).
https://github.com/user-attachments/assets/e0cf0760-a461-46f6-8366-d5afc513734e
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4340577213
##########
docs/sip/embedded-fallback-credential.md:
##########
Review Comment:
Are we sure we want to check in this into the REPO?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4288654557
##########
superset/commands/dataset/export.py:
##########
@@ -137,6 +137,17 @@ def _export(
)
payload["ssh_tunnel"] = mask_password_info(ssh_tunnel_payload)
+ # Carry Secure Extra like the database export does: masked when
+ # it holds secrets, so a promoted dashboard keeps its OAuth2
+ # client and embedded viewer credential and the import asks for
+ # their values.
+ if encrypted_extra := model.database.encrypted_extra:
+ masked_encrypted_extra = model.database.masked_encrypted_extra
+ if encrypted_extra != masked_encrypted_extra:
+ payload["masked_encrypted_extra"] = masked_encrypted_extra
+ else:
+ payload["encrypted_extra"] = encrypted_extra
Review Comment:
This `else` branch writes the raw Secure Extra into the dataset bundle whenever the engine's masking leaves the blob unchanged. Dataset export is what chart and dashboard exports reuse, and those are available to non-admin roles (a Gamma user with `can_export` on a chart or dashboard), so a connection whose Secure Extra carries credentials outside the declared sensitive paths now leaks them to a user without database-management permission. For example, `{"connect_args": {"password": "warehouse-secret"}}` on a Postgres connection is not masked, so `GET /api/v1/chart/export/` would return it in `databases/*.yaml`. This applies even with the new flag off, and before this change the dataset bundle never carried `encrypted_extra`.
Should this only be included for users who can manage databases, or should the non-masked case be dropped rather than exported raw?
##########
superset/databases/schemas.py:
##########
@@ -444,6 +447,76 @@ def build_sqlalchemy_uri(
return data
+ @validates_schema
+ def validate_embedded_credentials(
+ self, data: dict[str, Any], **kwargs: Any
+ ) -> None:
+ """Validate authored credentials with the effective connection's engine."""
+ value = data.get("masked_encrypted_extra")
+ if not value or not (
+ is_feature_enabled("EMBEDDED_CREDENTIAL_FALLBACK")
+ and is_feature_enabled("EMBEDDED_SUPERSET")
+ ):
+ return
+
+ encrypted_extra = json.loads(value)
+ if EMBEDDED_CREDENTIALS_KEY not in encrypted_extra:
+ return
+
+ engine_spec = BaseEngineSpec
+ stored = (
+ db.session.get(Database, self.database_id)
+ if self.database_id is not None
+ else None
+ )
+ if sqlalchemy_uri := data.get("sqlalchemy_uri"):
+ engine_spec = Database(sqlalchemy_uri=sqlalchemy_uri).db_engine_spec
+ elif stored:
+ engine_spec = stored.db_engine_spec
+
+ credentials = encrypted_extra[EMBEDDED_CREDENTIALS_KEY]
+ if embedded_credentials_incomplete(
+ credentials,
+ engine_spec.validate_embedded_credentials,
+ ):
+ raise ValidationError(
+ _(
+ "Embedded guest credentials are invalid or incomplete. Provide "
+ "all required fields or clear them to leave embedded viewers "
+ "unconfigured."
+ ),
+ field_name="masked_encrypted_extra",
+ )
+
+ # A credential only applies to a per-user OAuth2 connection with
+ # impersonation on (`Database.embedded_fallback_applies`). Anywhere else
+ # it would save and silently do nothing -- or, with impersonation off,
+ # leave every viewer on the connection's own account while the setting
+ # reads as configured -- so refuse it rather than store it.
+ if (
+ engine_spec.supports_embedded_credential_fallback
+ and engine_spec.validate_embedded_credentials(credentials) is not None
+ ):
+ impersonate_user = data.get(
+ "impersonate_user", stored.impersonate_user if stored else False
+ )
+ client_info = encrypted_extra.get("oauth2_client_info")
+ has_oauth2_client = bool(isinstance(client_info, dict) and client_info) or (
+ engine_spec.supports_oauth2
+ and engine_spec.engine_name
+ in current_app.config["DATABASE_OAUTH2_CLIENTS"]
+ )
+ if not (impersonate_user and has_oauth2_client):
Review Comment:
With both flags on, creating a Snowflake connection through the dynamic form and entering complete OAuth2 client info plus guest credentials gets a 400 here, because `impersonate_user` is not set yet. The create flow has no Advanced tab to turn impersonation on before Connect (that appears only after the connection exists), and the modal only auto-enables it for GSheets. That means the "configure credentials, connect, then enable impersonation" sequence the UI hint and `embedding.mdx` describe cannot complete: the admin has to clear the credentials, create the connection, enable impersonation, then re-enter them. No test submits the form in this state. Should the credential be allowed to save while impersonation is off (inert until it's enabled), or should the toggle be reachable before the first save?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4425651830
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `73a6c41`.
Endpoint and scope defaults now use nullish coalescing (`??`), so an explicitly cleared empty string is preserved instead of being replaced by the displayed default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `73a6c41`.
Credential completeness now delegates to `validate_credentials(value)`, with the helper documenting that callers use the engine’s query-time validator when the connection is known.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4275552285
##########
superset/models/core.py:
##########
@@ -1607,6 +1630,81 @@ def make_sqla_column_compatible(
sqla_col.key = label_expected
return sqla_col
+ def get_embedded_fallback_credentials(self) -> dict[str, str] | None:
+ """
+ Credentials to authenticate an embedded guest's queries, if any.
+
+ 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. This lets one
+ connection carry a credential used *only* for those requests, while logged-in
+ users continue to authenticate per-user via OAuth2.
+
+ Returns the credential only when all five conditions hold, so that there is no
+ route by which a logged-in user can reach it:
+
+ 1. the deployment opted in via ``EMBEDDED_CREDENTIAL_FALLBACK``;
+ 2. the engine opted in via ``supports_embedded_credential_fallback``;
+ 3. the connection stores a *complete* password or key-pair credential -- a
+ half-filled one is ignored rather than half-applied;
+ 4. the current principal is an embedded guest, and embedding is enabled;
+ 5. the connection actually uses OAuth2, so that this stays a fallback for the
+ per-user flow rather than a second identity on an ordinary connection.
+ """
+ if not is_feature_enabled("EMBEDDED_CREDENTIAL_FALLBACK"):
+ return None
+
+ if not self.db_engine_spec.supports_embedded_credential_fallback:
+ return None
+
+ # `get_current_guest_user_if_guest` is a bare isinstance check with no feature
+ # flag behind it. A `GuestUser` can only reach `g` via the guest-token request
+ # loader, which is itself gated -- but a security boundary should not rest on
+ # that implication, so the flag is checked explicitly.
+ if not is_feature_enabled("EMBEDDED_SUPERSET"):
+ return None
+
+ if security_manager.get_current_guest_user_if_guest() is None:
+ return None
+
+ if not self.is_oauth2_enabled():
Review Comment:
Async guest queries bypass the fallback here. With the Snowflake client configured via `DATABASE_OAUTH2_CLIENTS` (not per-connection `oauth2_client_info`), embedded credentials stored, and `GLOBAL_ASYNC_QUERIES` on, the Celery worker restores the guest but has no request context, so `SnowflakeEngineSpec.is_oauth2_enabled()` returns `False` (it returns early on `not has_request_context()`). This gate then returns `None` and the guest runs as the connection's ordinary credential, not the narrowed embedded user the docs promise, while the same dashboard loaded synchronously uses the fallback. Should condition 5 use a check that doesn't depend on request context (e.g. whether an OAuth2 client config exists for the engine), or should this limitation be documented?
##########
superset/models/core.py:
##########
@@ -746,7 +746,30 @@ def _get_sqla_engine( # pylint: disable=too-many-locals # noqa: C901
masked_url = self.get_password_masked_url(sqlalchemy_url)
logger.debug("Database._get_sqla_engine(). Masked URL: %s", str(masked_url))
- if self.impersonate_user:
+ if embedded_credentials := self.get_embedded_fallback_credentials():
Review Comment:
This branch is taken for a guest even when `impersonate_user` is off, so the fallback widens beyond per-user OAuth2. With Snowflake OAuth2 client info and embedded credentials saved but "Impersonate logged in user" unchecked, logged-in users query as the connection's ordinary credential (the `elif self.impersonate_user` path never runs), while every embedded guest silently switches to the fallback account. The new `embedding.mdx` walkthrough also never mentions that toggle, so an admin following it can end up with working guest queries but no per-user OAuth2 for logged-in users. Should the fallback also require `self.impersonate_user` (and the docs call out that prerequisite)?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4277647945
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `ba7904c`.
The URI and scope defaults use nullish coalescing, so an explicitly cleared empty string remains empty instead of being replaced by the displayed default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `ba7904c`.
Credential completeness is checked through the supplied `validate_credentials` callback, allowing engine-specific validation rules to accept alternate credential shapes.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/models/core.py:
##########
@@ -746,7 +755,50 @@ def _get_sqla_engine( # pylint: disable=too-many-locals # noqa: C901
masked_url = self.get_password_masked_url(sqlalchemy_url)
logger.debug("Database._get_sqla_engine(). Masked URL: %s", str(masked_url))
- if self.impersonate_user:
+ if embedded_fallback:
+ embedded_credentials = self.get_embedded_fallback_credentials()
+ if embedded_credentials is None:
+ # Falling through to the base credential here is the failure mode this
+ # whole feature exists to prevent: an embedded viewer would silently
+ # query as whichever user the connection URL carries. That happens on
+ # a per-user OAuth2 connection with no (or a half-filled)
+ # credential configured -- none of which the viewer or
+ # the dashboard author can see. Refuse instead, with a message an
+ # administrator can act on.
+ raise SupersetErrorException(
+ SupersetError(
+ message=__(
+ "This dashboard's database connection authenticates users "
+ "individually and has no credential configured for "
+ "embedded viewers, so this query cannot run."
+ ),
+ error_type=SupersetErrorType.CONNECTION_ACCESS_DENIED_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ # Deliberately *instead of* impersonation, not alongside it, and this is
+ # load-bearing twice over. First, an engine spec that opts in sets its
+ # OAuth2 authenticator during impersonation whether or not a token exists,
+ # so skipping it is what lets the stored credential authenticate at all.
+ # Second, `effective_username` above resolves to `g.user.username`, and a
+ # `GuestUser` carries whatever username the guest token claims -- so going
+ # through impersonation would let whoever mints the token choose the
+ # connection identity.
+ #
+ # How the credential reaches the driver is the engine's business -- a
+ # password on the URL, a PEM key in connect_args. Both the URL and the
+ # engine kwargs participate in the cache key below, keeping guest and
+ # OAuth engines separate even when they use the same database username.
+ sqlalchemy_url, engine_kwargs = (
+ self.db_engine_spec.apply_embedded_credentials(
+ self,
+ embedded_credentials,
+ sqlalchemy_url,
+ engine_kwargs,
+ )
+ )
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `ba7904c`.
Before invoking `DB_CONNECTION_MUTATOR`, `effective_username` is set from the selected embedded-credential URL rather than retaining the guest token's username claim.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/commands/database/validate.py:
##########
@@ -236,6 +242,54 @@ def _get_database_name_error(self) -> Optional[SupersetError]:
)
return None
+ def _get_embedded_credentials_error(self) -> Optional[SupersetError]:
+ """Reject an embedded fallback credential that is missing one of its halves.
+
+ `Database.get_embedded_fallback_credentials` ignores such a credential, so
+ saving one leaves a setting that reads as configured and never applies.
+
+ The connection dialog validates *before* it folds `x-encrypted-extra`
+ parameters into `masked_encrypted_extra`, so the value arrives under
+ `parameters` from the dialog and under `masked_encrypted_extra` from a direct
+ API caller. An explicit form value replaces the stored value, including
+ when the credential is cleared. Disabled fields must not block edits.
+ """
+ if not (
+ is_feature_enabled("EMBEDDED_CREDENTIAL_FALLBACK")
+ and is_feature_enabled("EMBEDDED_SUPERSET")
+ ):
+ return None
+
+ parameters = self._properties.get("parameters") or {}
+ if EMBEDDED_CREDENTIALS_KEY in parameters:
+ credentials = parameters[EMBEDDED_CREDENTIALS_KEY]
+ else:
+ try:
+ encrypted_extra = json.loads(
+ self._properties.get("masked_encrypted_extra") or "{}"
+ )
+ except json.JSONDecodeError:
+ encrypted_extra = {}
+ credentials = (
+ encrypted_extra.get(EMBEDDED_CREDENTIALS_KEY)
+ if isinstance(encrypted_extra, dict)
+ else None
+ )
+
+ if not embedded_credentials_incomplete(credentials):
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `ba7904c`.
Malformed JSON credentials are treated as incomplete by the shared check, so this validation returns an error instead of reporting success.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "geido (via GitHub) via notifications" <no...@superset.apache.org>.
geido commented on PR #45068:
URL: https://github.com/apache/superset/pull/45068#issuecomment-6581289989
Recording for my inline note on `superset/databases/schemas.py`, with both flags off: an Alpha user re-promoting their own dashboard with "Also overwrite all assets", at bdf7917 first, then at 73a6c41 (that code is unchanged at the current head).
https://github.com/user-attachments/assets/1cc6daa6-b4a7-4505-a536-f5ebc6064e29
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "sadpandajoe (via GitHub) via notifications" <no...@superset.apache.org>.
sadpandajoe commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4450789659
##########
superset/charts/api.py:
##########
@@ -1762,6 +1769,7 @@ def import_(self) -> Response:
ssh_tunnel_priv_key_passwords = parse_json_form_field(
"ssh_tunnel_private_key_passwords"
)
+ encrypted_extra_secrets = parse_json_form_field("encrypted_extra_secrets")
Review Comment:
`encrypted_extra_secrets` is only checked to be a JSON object at the top level, but `load_configs()` calls `.items()` on each per-file value. A request carrying `encrypted_extra_secrets={"databases/db.yaml": "secret"}` for a bundle with a masked Secure Extra parses fine and then raises an `AttributeError`, so the client gets an HTTP 500 instead of a 400/422. The same unchecked parse is repeated in the dashboard, dataset and saved-query import routes. Should the per-file values be validated as `dict[str, str]` before the command runs?
##########
tests/unit_tests/charts/commands/data/test_get_data_command.py:
##########
@@ -445,3 +445,28 @@ def test_get_query_handles_parsing_error_with_null_sql_value() -> None:
assert "query" not in result
assert result["error"] == "Parsing error occurred"
assert result["language"] == "sql"
+
+
+def test_database_error_while_building_the_query_is_a_failed_query() -> None:
+ """
+ A connection failure while the query is being built -- a virtual dataset's
+ default-schema lookup connects -- is reported as a failed query (a 400),
+ not left to escape as an unexpected server error (an HTML 500).
+ """
+ from sqlalchemy.exc import OperationalError
+
+ mock_query_context = Mock(spec=QueryContext)
+ mock_query_context.result_type = ChartDataResultType.FULL
+ mock_query_context.get_payload.side_effect = OperationalError(
+ "connect", {}, Exception("Incorrect username or password was specified.")
+ )
+ database = Mock()
+ database.db_engine_spec.extract_error_message.return_value = (
+ "Incorrect username or password was specified."
+ )
+ mock_query_context.datasource = Mock(database=database)
+
+ with pytest.raises(ChartDataQueryFailedError) as excinfo:
+ ChartDataCommand(mock_query_context).run()
Review Comment:
This test goes through `run()`, but the chart data API calls `execute()`, which has its own new `except SQLAlchemyError` branch. If that branch were removed, this test would still pass and chart requests would go back to surfacing the connection failure as an unhandled 500. Could you add a case where `get_payload_result` raises `OperationalError` and `execute()` is expected to raise `ChartDataQueryFailedError` with the extracted message?
##########
superset/utils/json.py:
##########
@@ -362,6 +367,37 @@ def get_masked_fields(
return masked
+def unrevealable_masked_fields(
+ old_payload: dict[str, Any] | None,
+ new_payload: dict[str, Any],
+ sensitive_fields: set[str],
+) -> list[str]:
+ """
+ Returns masked fields in ``new_payload`` that ``old_payload`` cannot reveal.
+
+ :func:`reveal_sensitive` swaps a mask back for the stored value at the same
+ path. Where the stored payload has no value there, the mask would be kept and
+ stored as if it were the secret, so callers that must not do that (imports
+ overwriting a connection) use this to ask for the real value instead. A
+ stored mask counts as no value: earlier releases saved one when an import
+ had nothing to reveal, and revealing it only hands the mask back.
+
+ :param old_payload: The stored payload, if any
+ :param new_payload: The incoming payload, possibly containing masks
+ :param sensitive_fields: The set of fields to check, as JSONPath expressions
+ :returns: JSONPath expressions for masked fields with nothing to reveal
+ """
+ return [
+ field
+ for field in get_masked_fields(new_payload, sensitive_fields)
+ if old_payload is None
+ or not any(
+ match.value and match.value != PASSWORD_MASK
Review Comment:
`match.value and ...` treats a stored `null` as no value. Before this PR, `redact_sensitive` masked `null` fields too, so an older Snowflake export of an unencrypted-key connection carries `auth_params.privatekey_pass` as the mask, and `reveal_sensitive` used to restore the stored `null` from it. Now, overwriting that connection from the same archived export lists the passphrase as unrevealable, and the import dialog requires a non-empty answer for it, so the import cannot be completed without inventing a passphrase. Should a stored `None` count as revealable here, with only a stored mask treated as missing?
##########
docs/docs/using-superset/embedding.mdx:
##########
@@ -214,6 +321,7 @@ One caveat when the host page lives on a different site than Superset: the defau
- **Row-level security** — pass `rls` rules in the guest token request to restrict which rows are visible to the embedded user. Semantic views cannot enforce guest-token SQL clauses, so queries and cached-result reads are rejected when a global rule or a rule scoped to the semantic view applies. For hosts, a global guest rule deliberately refuses every semantic view, and a dataset-scoped rule can also refuse a semantic view even when the rule was intended for a SQL dataset. Read permissions do not override these restrictions. Semantic views remain accessible when no guest rule applies; SQL datasets continue to enforce their applicable rules.
- **Allowed domains** — restrict which host origins can embed a dashboard by setting **Allowed Domains** per-dashboard in the _Embed_ settings modal. Superset checks the request's `Referer` header against this list before serving the embedded view; an empty list allows any origin, so configure this explicitly for production.
- **Redacted errors** — API responses to a guest token report a generic `An error occurred while fetching the data.` instead of the underlying error, since engine errors quote catalog, schema, table and column names. Errors Superset raises itself — access denials, timeouts, payload validation — keep their message, and the full error is always available in the server logs.
+- **OAuth2 connections and guests** — a guest cannot complete an OAuth2 authorization, so Superset does not start one for them, and an embedded viewer on an OAuth2 connection with no applicable credential is never sent into a login flow they cannot finish. With `EMBEDDED_CREDENTIAL_FALLBACK` off, the connection attempt fails with the database driver's authentication error. With it on, Superset refuses the query before connecting. Either way the viewer sees the generic `An error occurred while fetching the data.` message, and the reason is in the server logs. To serve embedded dashboards from such a connection, see [`EMBEDDED_CREDENTIAL_FALLBACK`](#embedded_credential_fallback).
Review Comment:
"With it on, Superset refuses the query before connecting" reads as true for every OAuth2 connection, but the refusal only applies when the engine opts in (Snowflake) and impersonation is on. An operator enabling the flag for, say, a Trino OAuth2 connection will still go through the ordinary connection path and not get the early refusal this describes. Can this sentence name those two conditions?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4285244313
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `4a5cb34`.
The endpoint and scope defaults now use nullish coalescing, so an explicitly cleared empty string is preserved instead of being replaced by the default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `4a5cb34`.
Credential-shape validation now delegates to the supplied `validate_credentials` function, allowing engine-specific validators to accept alternate complete credential shapes.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4277693299
##########
tests/unit_tests/models/core_test.py:
##########
@@ -2940,3 +2940,797 @@ def test_connection_does_not_re_exchange_a_refused_refresh_token(
isolated_session.assert_not_called()
start_dance.assert_called_once_with(database)
engine.raw_connection.assert_not_called()
+
+
+def _embedded_snowflake_database(
+ *,
+ username: str = "embedded_svc",
+ password: str = "embedded_pw", # noqa: S107
+) -> "Database":
+ """
+ A Snowflake connection carrying both an OAuth2 client and a guest credential.
+ """
+ from superset.models.core import Database
+
+ encrypted_extra: dict[str, Any] = {
+ "oauth2_client_info": {
+ "id": "client-id",
+ "secret": "client-secret",
+ "scope": "refresh_token",
+ "authorization_request_uri": "https://xy12345.snowflakecomputing.com/oauth/authorize",
+ "token_request_uri": "https://xy12345.snowflakecomputing.com/oauth/token-request",
+ },
+ }
+ if username or password:
+ encrypted_extra["embedded_credentials"] = {
+ "username": username,
+ "password": password,
+ }
+
+ return Database(
+ database_name="snowflake_db",
+ sqlalchemy_uri=(
+ "snowflake://svc_user:svc_pass@xy12345.us-east-2.aws/ANALYTICS_DB"
+ "?role=ANALYST&warehouse=COMPUTE_WH"
+ ),
+ impersonate_user=True,
+ encrypted_extra=json.dumps(encrypted_extra),
+ )
+
+
+def _become_guest(mocker: MockerFixture, username: str = "guest_user") -> None:
+ """
+ Install an embedded guest as the current principal.
+ """
+ from superset.security.guest_token import GuestToken, GuestUser
+
+ token: GuestToken = {
+ "user": {"username": username},
+ "resources": [],
+ "rls_rules": [],
+ "iat": 0,
+ "exp": 1,
+ }
+ guest = GuestUser(token=token, roles=[])
+ mocker.patch(
+ "superset.models.core.security_manager.get_current_guest_user_if_guest",
+ return_value=guest,
+ )
+ mocker.patch("superset.models.core.get_username", return_value=guest.username)
+
+
+def _stub_driver_dependent_uri_validation(mocker: MockerFixture) -> None:
+ """
+ Make URI validation driver-agnostic for these tests.
+
+ ``validate_database_uri`` resolves the dialect to look up disallowed query
+ params, which needs ``snowflake-sqlalchemy`` installed. That is an optional
+ extra and is absent from the unit-test environment, and it has nothing to do
+ with which credential is selected, so it is stubbed rather than depended on.
+ """
+ mocker.patch("superset.db_engine_specs.base.BaseEngineSpec.validate_database_uri")
+
+
+@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True)
+def test_embedded_fallback_credential_is_used_for_a_guest(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that an embedded guest connects with the stored fallback credential.
+
+ The guest URL must carry the fallback username and password and must *not* carry
+ `authenticator=oauth`: skipping impersonation is the whole mechanism, because
+ Snowflake's `impersonate_user` sets that authenticator whether or not a per-user
+ token exists -- and a guest never has one.
+ """
+ create_engine_mock = mocker.patch(
+ "superset.models.core.create_engine",
+ return_value=create_engine("sqlite://"),
+ )
+ _become_guest(mocker)
+ _stub_driver_dependent_uri_validation(mocker)
+
+ _embedded_snowflake_database()._get_sqla_engine(nullpool=False)
+
+ url = create_engine_mock.call_args[0][0]
+ assert url.username == "embedded_svc"
+ assert url.password == "embedded_pw" # noqa: S105
+ assert "authenticator" not in url.query
+
+
+@pytest.mark.parametrize("passphrase", ["", "key-passphrase"])
+@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True)
+def test_embedded_keypair_is_used_only_for_a_guest(
+ mocker: MockerFixture, passphrase: str
+) -> None:
+ """A guest uses the PEM key while a logged-in user stays on OAuth2."""
+ from cryptography.hazmat.primitives import serialization
+ from cryptography.hazmat.primitives.asymmetric import rsa
+
+ from superset.models.core import _ENGINE_CACHE
+
+ private_key = rsa.generate_private_key(public_exponent=65537, key_size=2048)
+ encryption = (
+ serialization.BestAvailableEncryption(passphrase.encode())
+ if passphrase
+ else serialization.NoEncryption()
+ )
+ pem = private_key.private_bytes(
+ serialization.Encoding.PEM,
+ serialization.PrivateFormat.PKCS8,
+ encryption,
+ ).decode()
+ expected_der = private_key.private_bytes(
+ serialization.Encoding.DER,
+ serialization.PrivateFormat.PKCS8,
+ serialization.NoEncryption(),
+ )
+ database = _embedded_snowflake_database()
+ extra = database.get_encrypted_extra()
+ extra["embedded_credentials"] = {
+ "username": "embedded_svc",
+ "auth_method": "keypair",
+ "privatekey_body": pem,
+ "privatekey_pass": passphrase,
+ }
+ extra["auth_method"] = "keypair"
+ extra["auth_params"] = {"privatekey_body": "not-the-embedded-key"}
+ database.encrypted_extra = json.dumps(extra)
+ database.id = 1
+
+ create_engine_mock = mocker.patch(
+ "superset.models.core.create_engine",
+ side_effect=lambda *a, **kw: create_engine("sqlite://"),
+ )
+ _stub_driver_dependent_uri_validation(mocker)
+ _ENGINE_CACHE.clear()
+
+ _become_guest(mocker, username="ACCOUNTADMIN")
+ database._get_sqla_engine(nullpool=False)
+ guest_url = create_engine_mock.call_args[0][0]
+ guest_kwargs = create_engine_mock.call_args[1]
+ assert guest_url.username == "embedded_svc"
+ assert guest_url.password is None
+ assert "authenticator" not in guest_url.query
+ assert guest_kwargs["connect_args"]["private_key"] == expected_der
+
+ mocker.patch(
+ "superset.models.core.security_manager.get_current_guest_user_if_guest",
+ return_value=None,
+ )
+ with current_app.test_request_context("/dashboard/"):
+ database._get_sqla_engine(nullpool=False)
+ user_url = create_engine_mock.call_args[0][0]
+ user_kwargs = create_engine_mock.call_args[1]
+ assert user_url.query["authenticator"] == "oauth"
+ assert "private_key" not in user_kwargs["connect_args"]
+ assert len(_ENGINE_CACHE) == 2
+ _ENGINE_CACHE.clear()
+
+
+@pytest.mark.parametrize(
+ "credentials",
+ [
+ {"username": " ", "password": "pw"},
+ {"username": " \t ", "auth_method": "keypair", "privatekey_body": "PEM"},
+ {"username": "embedded_svc", "auth_method": "keypair"},
+ {"auth_method": "keypair", "privatekey_body": "PEM"},
+ {"username": "embedded_svc", "auth_method": "keypair", "password": "pw"},
+ {"username": "embedded_svc", "auth_method": "unknown", "password": "pw"},
+ ],
+)
+@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True)
+def test_incomplete_embedded_keypair_is_ignored(
+ mocker: MockerFixture, credentials: dict[str, str]
+) -> None:
+ """An incomplete or unknown fallback method cannot authenticate a guest."""
+ _become_guest(mocker)
+ database = _embedded_snowflake_database()
+ extra = database.get_encrypted_extra()
+ extra["embedded_credentials"] = credentials
+ database.encrypted_extra = json.dumps(extra)
+
+ assert database.get_embedded_fallback_credentials() is None
+
+
+@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True)
+def test_guest_username_from_the_token_never_reaches_the_connection(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that the guest token's own username claim cannot become the connection
+ identity.
+
+ Whoever mints the guest token controls its `username`. If the fallback path went
+ through impersonation, that claim would be used to connect -- so a token minted
+ with `ACCOUNTADMIN` would try to connect as `ACCOUNTADMIN`.
+ """
+ create_engine_mock = mocker.patch(
+ "superset.models.core.create_engine",
+ return_value=create_engine("sqlite://"),
+ )
+ _become_guest(mocker, username="ACCOUNTADMIN")
+ _stub_driver_dependent_uri_validation(mocker)
+
+ _embedded_snowflake_database()._get_sqla_engine(nullpool=False)
+
+ url = create_engine_mock.call_args[0][0]
+ assert url.username == "embedded_svc"
+ assert "ACCOUNTADMIN" not in url.render_as_string(hide_password=False)
+
+
+@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True)
+def test_connection_mutator_receives_the_embedded_connection_identity(
+ mocker: MockerFixture,
+) -> None:
+ """A username-forwarding hook must retain the configured guest identity."""
+ from flask import current_app
+
+ create_engine_mock = mocker.patch(
+ "superset.models.core.create_engine",
+ return_value=create_engine("sqlite://"),
+ )
+ _become_guest(mocker, username="ACCOUNTADMIN")
+ _stub_driver_dependent_uri_validation(mocker)
+ mutator = mocker.Mock(
+ side_effect=lambda url, kwargs, username, security_manager, source: (
+ url.set(username=username),
+ kwargs,
+ )
+ )
+ mocker.patch.dict(current_app.config, {"DB_CONNECTION_MUTATOR": mutator})
+
+ _embedded_snowflake_database()._get_sqla_engine(nullpool=False)
+
+ assert mutator.call_args.args[2] == "embedded_svc"
+ assert create_engine_mock.call_args.args[0].username == "embedded_svc"
+
+
+@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True)
+def test_embedded_fallback_credential_is_not_used_for_a_logged_in_user(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that a logged-in user on the same connection never sees the credential.
+ """
+ create_engine_mock = mocker.patch(
+ "superset.models.core.create_engine",
+ return_value=create_engine("sqlite://"),
+ )
+ mocker.patch(
+ "superset.models.core.security_manager.get_current_guest_user_if_guest",
+ return_value=None,
+ )
+ mocker.patch("superset.models.core.get_username", return_value="alice")
+ _stub_driver_dependent_uri_validation(mocker)
+
+ _embedded_snowflake_database()._get_sqla_engine(nullpool=False)
+
+ rendered = create_engine_mock.call_args[0][0].render_as_string(hide_password=False)
+ assert "embedded_svc" not in rendered
+ assert "embedded_pw" not in rendered
+
+
+@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True)
+def test_guest_and_admin_do_not_share_an_engine_cache_entry(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that a guest and a logged-in user get distinct engines for one connection.
+
+ The engine cache is keyed on (database id, rendered URL, engine kwargs) with no
+ user component. Putting the fallback credential on the URL -- rather than into
+ `connect_args` -- is what keeps the two principals apart; routing it through
+ `connect_args` would reach the key only via `repr()` of a nested dict.
+ """
+ from superset.models.core import _ENGINE_CACHE
+
+ mocker.patch(
+ "superset.models.core.create_engine",
+ side_effect=lambda *a, **kw: create_engine("sqlite://"),
+ )
+ _ENGINE_CACHE.clear()
+ _stub_driver_dependent_uri_validation(mocker)
+
+ database = _embedded_snowflake_database()
+ database.id = 1
+
+ guest_patch = mocker.patch(
+ "superset.models.core.security_manager.get_current_guest_user_if_guest"
+ )
+ mocker.patch("superset.models.core.get_username", return_value="alice")
+
+ _become_guest(mocker)
+ database._get_sqla_engine(nullpool=False)
+
+ guest_patch.return_value = None
+ mocker.patch(
+ "superset.models.core.security_manager.get_current_guest_user_if_guest",
+ return_value=None,
+ )
+ database._get_sqla_engine(nullpool=False)
+
+ urls = {key[1] for key in _ENGINE_CACHE}
+ assert len(_ENGINE_CACHE) == 2, _ENGINE_CACHE
+ assert any("embedded_svc" in url for url in urls)
+ assert any("svc_user" in url for url in urls)
+ _ENGINE_CACHE.clear()
+
+
+@pytest.mark.parametrize(
+ "username,password",
+ [("embedded_svc", ""), ("", "embedded_pw"), ("", "")],
+)
+@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True)
+def test_a_half_filled_embedded_credential_is_ignored(
+ mocker: MockerFixture,
+ username: str,
+ password: str,
+) -> None:
+ """
+ Test that an incomplete credential is ignored rather than half-applied.
+ """
+ _become_guest(mocker)
+
+ database = _embedded_snowflake_database(username=username, password=password)
+
+ assert database.get_embedded_fallback_credentials() is None
+
+
+@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=False, EMBEDDED_SUPERSET=True)
+def test_embedded_fallback_credential_requires_its_feature_flag(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that the kill switch works on an otherwise fully configured connection.
+ """
+ _become_guest(mocker)
+
+ assert _embedded_snowflake_database().get_embedded_fallback_credentials() is None
+
+
+@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=False)
+def test_embedded_fallback_credential_requires_embedded_superset(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that the credential is unreachable when embedding itself is disabled.
+
+ `get_current_guest_user_if_guest` is a bare isinstance check with no flag behind
+ it, so this is checked explicitly rather than inherited.
+ """
+ _become_guest(mocker)
+
+ assert _embedded_snowflake_database().get_embedded_fallback_credentials() is None
+
+
+@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True)
+def test_embedded_fallback_credential_requires_an_oauth2_connection(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that an ordinary connection does not grow a second identity.
+
+ Without this condition a plain username/password Snowflake connection with the
+ field filled in would silently route guests elsewhere, which is wider than the
+ feature's purpose: a fallback for the per-user OAuth2 flow.
+ """
+ from superset.models.core import Database
+
+ _become_guest(mocker)
+
+ database = Database(
Review Comment:
This test returns `None` at the `impersonate_user` check, not at the OAuth2 check it is named for: the `Database(...)` here omits `impersonate_user=True`, so `embedded_fallback_applies()` is false before the OAuth2-client condition is reached. Removing that condition would leave the test green, and `test_embedded_fallback_credential_requires_engine_opt_in` just below has the same gap for the engine opt-in condition. Could both fixtures set `impersonate_user=True` so each test only fails when its own condition is removed?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4300481989
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `fdf237e`.
The endpoint and scope defaults now use nullish coalescing, so an explicitly cleared empty string is preserved instead of being replaced by the default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `fdf237e`.
Credential-shape validation now delegates to the supplied `validate_credentials` function, allowing engine-specific validators to accept alternate complete credential shapes.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "geido (via GitHub) via notifications" <no...@superset.apache.org>.
geido commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4456087447
##########
superset/utils/json.py:
##########
@@ -362,6 +367,37 @@ def get_masked_fields(
return masked
+def unrevealable_masked_fields(
+ old_payload: dict[str, Any] | None,
+ new_payload: dict[str, Any],
+ sensitive_fields: set[str],
+) -> list[str]:
+ """
+ Returns masked fields in ``new_payload`` that ``old_payload`` cannot reveal.
+
+ :func:`reveal_sensitive` swaps a mask back for the stored value at the same
+ path. Where the stored payload has no value there, the mask would be kept and
+ stored as if it were the secret, so callers that must not do that (imports
+ overwriting a connection) use this to ask for the real value instead. A
+ stored mask counts as no value: earlier releases saved one when an import
+ had nothing to reveal, and revealing it only hands the mask back.
+
+ :param old_payload: The stored payload, if any
+ :param new_payload: The incoming payload, possibly containing masks
+ :param sensitive_fields: The set of fields to check, as JSONPath expressions
+ :returns: JSONPath expressions for masked fields with nothing to reveal
+ """
+ return [
+ field
+ for field in get_masked_fields(new_payload, sensitive_fields)
+ if old_payload is None
+ or not any(
+ match.value and match.value != PASSWORD_MASK
Review Comment:
+1, this one's real. Re-importing an older database export of a Snowflake key pair with no passphrase (`privatekey_pass: null`) now gets a 422 on both #45870 and #45890, while master keeps the null. In the dialog the only way past it is typing something and deleting it. An AWS IAM `external_id` stored as `""`/null on Postgres/MySQL/Redshift hits the same thing, and through `/api/v1/assets/import/` it's a 500 with no hint. Counting a stored null/`""` as revealable and only a stored mask as missing sounds right to me.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4300653953
##########
superset-frontend/src/features/databases/DatabaseModal/index.test.tsx:
##########
@@ -1955,6 +2053,42 @@ describe('DatabaseModal', () => {
useSingleViewResourceMock.mockRestore();
},
);
+
+ test('keeps Connect disabled while a half-filled embedded credential is rejected', async () => {
Review Comment:
This test selects Postgres and never edits an embedded credential: it mocks the validate-params 422 and then blurs the ordinary `database` field, so it proves that any blur-time `INVALID_PAYLOAD_SCHEMA_ERROR` disables Connect, not that a half-filled credential reaches validation. If a Snowflake form stopped forwarding embedded-credential edits into blur-time validation, this test would still pass. Could it render the Snowflake form with OAuth2 and impersonation on, type only the embedded username, blur that input, assert the validate request carries the partial `embedded_credentials`, and then assert the error text and a disabled Connect button?
##########
superset/db_engine_specs/snowflake.py:
##########
@@ -370,6 +406,50 @@ 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
+ )
+ # The connection's `authenticator` belongs to the connection's own sign-in:
+ # `SNOWFLAKE_JWT` there would make the driver expect a key pair from a
+ # password guest. The driver picks key-pair sign-in itself when the
+ # embedded credential supplies `private_key`.
+ url = url.difference_update_query(["role", "authenticator"])
+ connect_args = engine_kwargs.get("connect_args", {})
+ connect_args.pop("role", None)
+ connect_args.pop("authenticator", None)
Review Comment:
A guest on a connection whose own `extra.engine_params.connect_args` carries `password` or `user` still authenticates as the connection, not as the fallback user. With both flags on, impersonation on, OAuth2 configured, `connect_args.password = "base-password"` and fallback credentials `embedded_user` / `embedded-password`, the fallback replaces the URL credential but SQLAlchemy merges `connect_args` over the URL-derived arguments, so Snowflake receives `base-password` and the correct fallback login is refused. Only `role` and `authenticator` are removed here. Should the other authentication-bearing keys (`password`, `user`, `token`, `private_key`, `private_key_file`) be dropped from `connect_args` on this path too?
##########
UPDATING.md:
##########
@@ -24,6 +24,29 @@ assists people when migrating to a new version.
## Next
+- `EMBEDDED_CREDENTIAL_FALLBACK` (new, off by default) changes behaviour for embedded
+ guests on database connections that authenticate users individually via OAuth2 with
+ **Impersonate logged in user** enabled. Ordinary service-account connections retain
+ their existing behavior, even with a deployment-wide OAuth2 client configured. With
+ the flag on, a guest whose request has no applicable embedded credential — none
+ configured on the connection, or one missing its username, password, or private key
+ required by the selected authentication method — is refused
+ with a `CONNECTION_ACCESS_DENIED_ERROR` instead of connecting with the credential in
+ the connection's own SQLAlchemy URI. Queries that do run under an embedded credential
+ get a dedicated cache partition distinct from every logged-in username, including
+ a separate partition for guests without credentials, so they no longer share a
+ DATA-cache entry (or, under `GLOBAL_ASYNC_QUERIES`, a deduplicated chart task) with a logged-in user's
+ per-user result; existing cached entries for such charts are effectively invalidated
+ for guests. A malformed or incomplete `embedded_credentials` is rejected by
+ `POST`/`PUT /api/v1/database/` and by `validate_parameters` rather than stored and
+ ignored while both embedding flags are enabled; asset import is unchanged. Validation
+ uses the engine's credential rules and edited credentials rather than stale stored
+ values, and rejects whitespace-only usernames. Disabling either flag preserves stored credentials without blocking unrelated edits. Restored async guest
+ queries use the same credential as synchronous queries. Connection-mutator hooks
+ receive the selected fallback username. Snowflake OAuth2 scope is
+ configured explicitly; a role scope selects that role, and omitting it uses the
+ user's default role. Deployments that leave the fallback flag off are unaffected.
Review Comment:
The closing sentence says deployments that leave the fallback flag off are unaffected, but some of this PR's import behaviour is not behind the flag. Importing a connection whose Secure Extra still holds a masked value, including one that already exists but has no stored value for it, now fails with "Must provide value for masked_encrypted_extra field" and asks for `encrypted_extra_secrets`. Operators who promote OAuth2 connections between environments by scripted import will hit this with the flag off. Should the note call this out so they know to supply those secrets?
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EmbeddedCredentialsField.tsx:
##########
@@ -0,0 +1,432 @@
+/**
+ * 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.
+ */
+
+import { ChangeEvent, useEffect, useRef, useState } from 'react';
+import { t } from '@apache-superset/core/translation';
+import { Alert } from '@apache-superset/core/components';
+import {
+ Button,
+ Collapse,
+ Flex,
+ Form,
+ FormItem,
+ Input,
+ LabeledErrorBoundInput as ValidatedInput,
+ Tag,
+ Typography,
+} from '@superset-ui/core/components';
+import { Radio } from '@superset-ui/core/components/Radio';
+import {
+ CustomParametersChangeType,
+ DatabaseObject,
+ FieldPropTypes,
+} from '../../types';
+
+const PANEL_KEY = 'embedded-credentials';
+const FIELD_NAME = 'embedded_credentials';
+
+const LABELS = {
+ USERNAME: t('Username'),
+ PASSWORD: t('Password'),
+ PRIVATE_KEY: t('Private key (PEM)'),
+ PRIVATE_KEY_PASSPHRASE: t('Private key passphrase (optional)'),
+};
+
+// Stated here rather than only in the docs: an admin who reads this as a
+// general-purpose fallback has made a security mistake.
+const HELP_TEXT = t(
+ 'Used only when a dashboard is viewed through an embedded guest token. ' +
+ 'Every embedded viewer of this connection queries as this one user, so it ' +
+ 'should be a dedicated account with the narrowest access those dashboards ' +
+ 'need. Logged-in users are unaffected and continue to authenticate ' +
+ 'individually via OAuth2.',
+);
+
+const INACTIVE_REASON = t(
+ 'Requires OAuth2 client information and “Impersonate logged in user” ' +
+ '(Advanced → Security).',
+);
+
+interface EmbeddedCredentials {
+ username: string;
+ password: string;
+ auth_method: 'password' | 'keypair';
+ privatekey_body: string;
+ privatekey_pass: string;
+}
+
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * Both `masked_encrypted_extra` and the `parameters.*` entries arrive either as
+ * an object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here rather than at each
+ * call site.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // through.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
+const asString = (value: unknown): string =>
+ typeof value === 'string' ? value : '';
+
+/**
+ * Does this connection have an OAuth2 client configured?
+ *
+ * The per-connection half is live form state (the admin may be filling the
+ * OAuth2 section during setup, so `parameters` is checked ahead of the stored
+ * blob); the deployment-wide half -- a client in `DATABASE_OAUTH2_CLIENTS` --
+ * is knowable only server-side and arrives through the schema property's
+ * `default`.
+ */
+export const connectionUsesOAuth2 = (
+ db?: DatabaseObject,
+ defaultValue?: { engine_oauth2_client_configured?: boolean },
+): boolean => {
+ if (defaultValue?.engine_oauth2_client_configured) {
+ return true;
+ }
+
+ const fromForm = asObject(db?.parameters?.oauth2_client_info);
+ const fromStorage = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+
+ return !!(asString(fromForm.id) || asString(fromStorage.id));
+};
+
+export const EmbeddedCredentialsField = ({
+ changeMethods,
+ db,
+ default_value: defaultValue,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => {
+ const deriveEmbeddedCredentials = (
+ blankAuthMethod: EmbeddedCredentials['auth_method'] = 'password',
+ ): EmbeddedCredentials => {
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.embedded_credentials` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a credential the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.embedded_credentials);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).embedded_credentials,
+ );
+ const source = Object.keys(typed).length ? typed : stored;
Review Comment:
Editing the credential in Basic and then changing or deleting it in Advanced > Security > Secure extra silently restores the earlier value. `parameters.embedded_credentials` keeps winning here, and `onSave` in `DatabaseModal` writes `parameters[embedded_credentials]` over the parsed Secure Extra, so an admin who clears the guest password in the JSON editor saves the one they typed in the form. Should the field clear or resync `parameters.embedded_credentials` when the Secure Extra JSON changes? `OAuth2ClientField` has the same precedence.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4277530316
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
**Suggestion:** When a GSheets user clears an endpoint or scope, these fallbacks restore the displayed default while the form parameters still submit an empty string.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Incorrect condition logic`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5d1a10208bd8438787612b34c1bed1af&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5d1a10208bd8438787612b34c1bed1af&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx
**Line:** 96:102
**Comment:**
*Incorrect Condition Logic: When a GSheets user clears an endpoint or scope, these fallbacks restore the displayed default while the form parameters still submit an empty string.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=827522511ccff47d94969d3d27921dbeaade112e7f63d37d59de08cef0be4f75&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=827522511ccff47d94969d3d27921dbeaade112e7f63d37d59de08cef0be4f75&reaction=dislike'>👎</a>
##########
superset/db_engine_specs/base.py:
##########
@@ -2544,6 +2568,67 @@ def impersonate_user(
return url, engine_kwargs
+ @classmethod
+ def validate_embedded_credentials(cls, credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ Called by ``Database.get_embedded_fallback_credentials`` once its gate has
+ decided the request is one the fallback governs. The canonical shape is
+ ``{username, password}`` or ``{username, auth_method: "keypair",
+ privatekey_body, privatekey_pass}``; an engine whose driver needs a
+ different shape overrides this, and must then relax ``validate_parameters``
+ to match so a credential it accepts here is not rejected on save.
+ """
+ return validated_embedded_credentials(credentials)
Review Comment:
**Suggestion:** `validated_embedded_credentials` accepts whitespace-only usernames, so this delegation treats them as complete and guest queries fail with unusable credentials.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Logic error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cff1ea0c75494e7ca7ad9ce5db9b640c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cff1ea0c75494e7ca7ad9ce5db9b640c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/base.py
**Line:** 2583:2583
**Comment:**
*Logic Error: `validated_embedded_credentials` accepts whitespace-only usernames, so this delegation treats them as complete and guest queries fail with unusable credentials.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=6ae6c6012d6473bb7842d32a8cf33e76cc56abe84e9d2b8e49b743582f0ff884&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=6ae6c6012d6473bb7842d32a8cf33e76cc56abe84e9d2b8e49b743582f0ff884&reaction=dislike'>👎</a>
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
**Suggestion:** Drivers may override `validate_embedded_credentials` for another complete credential shape, but this check still rejects that shape during API validation when fallback is enabled.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=139ab41f7bb34e7d96290c3edee87b99&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=139ab41f7bb34e7d96290c3edee87b99&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/databases/utils.py
**Line:** 253:253
**Comment:**
*Api Mismatch: Drivers may override `validate_embedded_credentials` for another complete credential shape, but this check still rejects that shape during API validation when fallback is enabled.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=615706ee6befd9ae56f543b92c37182aaaa81f0833afd1efa04665e7394f4df6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=615706ee6befd9ae56f543b92c37182aaaa81f0833afd1efa04665e7394f4df6&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4296891524
##########
superset/charts/api.py:
##########
@@ -1772,6 +1779,11 @@ def import_(self) -> Response:
if "ssh_tunnel_private_key_passwords" in request.form
else None
)
+ encrypted_extra_secrets = (
+ json.loads(request.form["encrypted_extra_secrets"])
Review Comment:
This route is wrapped in `log_this_with_context` without `include_request_data=False`, so `collect_request_payload` copies the whole form into the log record, including the new `encrypted_extra_secrets` (OAuth2 client secrets, fallback passwords, PEM keys), and `DBEventLogger` stores it verbatim in `logs.json`. A user whose role can read `Log` but cannot manage databases can then read those secrets from `/api/v1/log/`. The same applies to the dashboard, dataset and saved-query import routes. Should these handlers exclude or redact the secrets form field before event logging?
##########
superset/commands/dashboard/importers/v1/__init__.py:
##########
@@ -79,7 +79,8 @@ class ImportDashboardsCommand(ImportModelsCommand):
"charts/": ImportV1ChartSchema(),
"dashboards/": ImportV1DashboardSchema(),
"datasets/": ImportV1DatasetSchema(),
- "databases/": ImportV1DatabaseSchema(),
+ # Related databases are reused rather than overwritten.
+ "databases/": ImportV1DatabaseSchema(overwrites_existing=False),
Review Comment:
`overwrites_existing=False` here skips the new unrevealable-mask check, but `_import()` still passes `overwrite=overwrite_assets` to `import_database()` when both overwrite flags are set. An admin importing a dashboard bundle with "overwrite all assets" over an existing connection whose stored Secure Extra lacks the OAuth2/embedded secret passes validation, then hits the `ImportFailedError` backstop in `import_database()` and gets a 500 instead of the structured prompt that renders the secret inputs. Should this schema follow the effective overwrite setting (and should there be a dashboard-import test with `overwrite=True, overwrite_all=True` asserting the structured missing-secret response)?
##########
UPDATING.md:
##########
@@ -24,6 +24,29 @@ assists people when migrating to a new version.
## Next
+- `EMBEDDED_CREDENTIAL_FALLBACK` (new, off by default) changes behaviour for embedded
+ guests on database connections that authenticate users individually via OAuth2 with
+ **Impersonate logged in user** enabled. Ordinary service-account connections retain
+ their existing behavior, even with a deployment-wide OAuth2 client configured. With
Review Comment:
The note says ordinary service-account connections keep their behavior, but `embedded_guest_missing_oauth2_client()` refuses embedded guests on any Snowflake connection with `impersonate_user` on and no OAuth2 client (`_get_sqla_engine`, `CONNECTION_ACCESS_DENIED_ERROR`) once both flags are enabled. Before this, Snowflake impersonation left the ordinary credential in place when OAuth2 wasn't configured, so such a connection kept serving guests. Should the note call out this refusal and the remediation (configure OAuth2 plus the embedded credential, or turn impersonation off)?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4309507084
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `aca5dab`.
The endpoint and scope defaults now use nullish coalescing, so an explicitly cleared empty string is preserved instead of being replaced by the default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `aca5dab`.
Credential-shape validation now delegates to the supplied `validate_credentials` function, allowing engine-specific validators to accept alternate complete credential shapes.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4277164896
##########
superset/models/core.py:
##########
@@ -746,7 +752,50 @@ def _get_sqla_engine( # pylint: disable=too-many-locals # noqa: C901
masked_url = self.get_password_masked_url(sqlalchemy_url)
logger.debug("Database._get_sqla_engine(). Masked URL: %s", str(masked_url))
- if self.impersonate_user:
+ if self.embedded_fallback_applies():
Review Comment:
With `DATABASE_OAUTH2_CLIENTS` configured for Snowflake, `engine_spec.is_oauth2_enabled()` is true for every Snowflake connection, so `embedded_fallback_applies()` is true for each of them regardless of `impersonate_user` or any per-connection OAuth2 client. Once `EMBEDDED_CREDENTIAL_FALLBACK` is on, an embedded guest on an ordinary service-account Snowflake connection (impersonation off, no embedded credential) now hits the `CONNECTION_ACCESS_DENIED_ERROR` refusal below, where before it simply queried. The message also says the connection "authenticates users individually", which isn't true for those connections. Should the gate also require `self.impersonate_user` (or an OAuth2 client that this connection actually uses) so only connections that really do per-user OAuth2 are refused?
##########
superset/utils/cache_keys.py:
##########
@@ -38,6 +38,25 @@ 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`.
+ if embedded_credentials := database.get_embedded_fallback_credentials():
Review Comment:
The "no credential configured" refusal in `_get_sqla_engine` only runs when a query actually reaches the database. When `get_embedded_fallback_credentials()` returns `None` the guest falls through to the ordinary key, which (without per-user caching or distinguishing RLS/Jinja) equals the logged-in user's key. So if a logged-in user has already warmed the chart's cache, the guest gets those rows instead of the refusal, and only gets the error on a cold cache. Should the guest-without-credential case also get its own cache partition (or be refused before the cache read) so the outcome doesn't depend on cache warmth?
##########
superset/databases/schemas.py:
##########
@@ -271,6 +271,38 @@ def masked_encrypted_extra_validator(value: str) -> None:
encrypted_extra_validator(value)
+def api_encrypted_extra_validator(value: str | None) -> None:
+ """
+ Validate `masked_encrypted_extra` as posted to the database API.
+
+ Adds the embedded-credential completeness check on top of the shared JSON
+ checks. A half-filled credential is ignored at query time
+ (`Database.get_embedded_fallback_credentials`), so storing one leaves a
+ security-relevant setting that reads as configured and never applies.
+
+ Deliberately a separate function rather than a change to
+ `encrypted_extra_validator`, which also backs the import schemas: tightening
+ those would reject existing exports.
+ """
+ encrypted_extra_validator(value)
+
+ if not value:
+ return
+
+ if embedded_credentials_incomplete(json.loads(value).get(EMBEDDED_CREDENTIALS_KEY)):
Review Comment:
This check runs on every `masked_encrypted_extra` the API receives, including the stored blob the dialog sends back on an unrelated edit. Import deliberately accepts a partial `embedded_credentials` (e.g. only `username`), so a connection imported that way can no longer be saved or tested: the dialog posts the old partial value in `masked_encrypted_extra` (the completed credential is only merged in after validation), and with either flag off the sub-form that would let the admin fix it is hidden. `ValidateDatabaseParametersCommand._get_embedded_credentials_error` rejects it the same way. Should this validate only the effective, merged credential, and be skipped while `EMBEDDED_CREDENTIAL_FALLBACK` is off?
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EmbeddedCredentialsField.tsx:
##########
@@ -0,0 +1,369 @@
+/**
+ * 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.
+ */
+
+import { ChangeEvent, useEffect, useState } from 'react';
+import { t } from '@apache-superset/core/translation';
+import { Alert } from '@apache-superset/core/components';
+import { Input, Collapse, Form, FormItem } from '@superset-ui/core/components';
+import { Radio } from '@superset-ui/core/components/Radio';
+import {
+ CustomParametersChangeType,
+ DatabaseObject,
+ FieldPropTypes,
+} from '../../types';
+
+const PANEL_KEY = 'embedded-credentials';
+
+const LABELS = {
+ USERNAME: t('Username'),
+ PASSWORD: t('Password'),
+ PRIVATE_KEY: t('Private key (PEM)'),
+ PRIVATE_KEY_PASSPHRASE: t('Private key passphrase (optional)'),
+};
+
+// Stated here rather than only in the docs: an admin who reads this as a
+// general-purpose fallback has made a security mistake.
+const HELP_TEXT = t(
+ 'Used only when a dashboard is viewed through an embedded guest token. ' +
+ 'Every embedded viewer of this connection queries as this one user, so it ' +
+ 'should be a dedicated account with the narrowest access those dashboards ' +
+ 'need. Logged-in users are unaffected and continue to authenticate ' +
+ 'individually via OAuth2.',
+);
+
+const IMPERSONATION_WARNING = t(
+ '"Impersonate logged in user" is off under Advanced → Security. Embedded ' +
+ 'viewers will use the credential below, but logged-in users will query ' +
+ "with this connection's own credential instead of their individual OAuth2 " +
+ 'identity.',
+);
+
+interface EmbeddedCredentials {
+ username: string;
+ password: string;
+ auth_method: 'password' | 'keypair';
+ privatekey_body: string;
+ privatekey_pass: string;
+}
+
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * Both `masked_encrypted_extra` and the `parameters.*` entries arrive either as
+ * an object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here rather than at each
+ * call site.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // through.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
+const asString = (value: unknown): string =>
+ typeof value === 'string' ? value : '';
+
+/**
+ * Does this connection authenticate users individually via OAuth2?
+ *
+ * Mirrors `Database.is_oauth2_enabled` on the backend, which is condition 5 of
+ * the fallback gate: a credential saved on a connection that does not use
+ * OAuth2 would never be read. The per-connection half is live form state (the
+ * admin may be filling the OAuth2 section right now, so `parameters` is checked
+ * ahead of the stored blob); the deployment-wide half -- a client in
+ * `DATABASE_OAUTH2_CLIENTS` -- is knowable only server-side and arrives through
+ * the schema property's `default`.
+ */
+export const connectionUsesOAuth2 = (
+ db?: DatabaseObject,
+ defaultValue?: { engine_oauth2_client_configured?: boolean },
+): boolean => {
+ if (defaultValue?.engine_oauth2_client_configured) {
+ return true;
+ }
+
+ const fromForm = asObject(db?.parameters?.oauth2_client_info);
+ const fromStorage = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+
+ return !!(asString(fromForm.id) || asString(fromStorage.id));
+};
+
+export const EmbeddedCredentialsField = ({
+ changeMethods,
+ db,
+ default_value: defaultValue,
+ getValidation,
+ validationErrors,
+}: FieldPropTypes) => {
+ const deriveEmbeddedCredentials = (): EmbeddedCredentials => {
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.embedded_credentials` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a credential the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.embedded_credentials);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).embedded_credentials,
+ );
+ const source = Object.keys(typed).length ? typed : stored;
+
+ return {
+ username: asString(source.username),
+ auth_method: source.auth_method === 'keypair' ? 'keypair' : 'password',
+ // On edit these are the backend's masks. They are posted back verbatim
+ // and `unmask_encrypted_extra` restores the stored secrets.
+ password: asString(source.password),
+ privatekey_body: asString(source.privatekey_body),
+ privatekey_pass: asString(source.privatekey_pass),
+ };
+ };
+
+ const [embeddedCredentials, setEmbeddedCredentials] =
+ useState<EmbeddedCredentials>(deriveEmbeddedCredentials);
+ const [activeKeys, setActiveKeys] = useState<string[]>([]);
+
+ // Serialized so the effect below keys off the *value*: the reducer hands back
+ // a fresh object on every keystroke, which as a raw dependency would re-sync
+ // state with itself on each one.
+ const typedCredentials = JSON.stringify(
+ db?.parameters?.embedded_credentials ?? null,
+ );
+
+ useEffect(() => {
+ setEmbeddedCredentials(deriveEmbeddedCredentials());
+ // eslint-disable-next-line react-hooks/exhaustive-deps -- re-sync only when
+ // one of the two sources changes, mirroring OAuth2ClientField.
+ }, [db?.masked_encrypted_extra, typedCredentials]);
+
+ const errorMessage = asString(validationErrors?.embedded_credentials);
+
+ // The panel is collapsed by default, which would hide its own validation
+ // error. Open it when one arrives rather than leaving the user with a
+ // disabled Connect button and nothing to look at.
+ useEffect(() => {
+ if (errorMessage) {
+ setActiveKeys(keys =>
+ keys.includes(PANEL_KEY) ? keys : [...keys, PANEL_KEY],
+ );
+ }
+ }, [errorMessage]);
+
+ // Offering the field on a connection that does not use OAuth2 would be
+ // offering a setting the backend gate can never read. Placed after the hooks
+ // so hook order stays unconditional, as in OAuth2ClientField.
+ if (!connectionUsesOAuth2(db, defaultValue)) {
+ return null;
+ }
+
+ const updateCredentials = (updated: EmbeddedCredentials) => {
+ setEmbeddedCredentials(updated);
+
+ const isBlank = !(
+ updated.username ||
+ updated.password ||
+ updated.privatekey_body ||
+ updated.privatekey_pass
+ );
+
+ if (isBlank) {
+ // Everything cleared means "no embedded credential". The save-time merge
+ // treats any object as truthy, so reporting a shape full of empty
+ // strings would persist them instead of removing the key. `auth_method`
+ // alone is not a credential. Clear the in-flight parameter *and* the
+ // stored blob, as the gsheets public/private toggle does.
+ changeMethods.onParametersChange({
+ target: { name: 'embedded_credentials', value: '' },
Review Comment:
On a fresh form, picking "Key pair" before typing anything takes the `isBlank` branch, which writes `parameters.embedded_credentials = ''`. That changes `typedCredentials`, the sync effect re-derives from an empty source, and `auth_method` falls back to `password`, so the radio snaps back to "Password" and the first click is lost. Should the selected `auth_method` be kept in local state (or only clear the parameter when it was previously non-empty) so the selection 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4275346871
##########
superset/db_engine_specs/snowflake.py:
##########
@@ -125,6 +130,31 @@ class SnowflakeParametersSchema(Schema):
database = fields.Str(required=True)
role = fields.Str(required=True)
warehouse = fields.Str(required=True)
+ # Unlike GSheets, the endpoints cannot be defaulted: they live on the customer's
+ # own Snowflake account (`https://<account>.snowflakecomputing.com/oauth/...`),
+ # so there is no URI that would be right for every deployment. `scope` can be,
+ # since a refresh token is what makes an OAuth2 connection usable beyond the
+ # first hour.
+ oauth2_client_info = EncryptedString(
+ required=False,
+ metadata={
+ "description": "OAuth2 client information",
+ "default": {"scope": "refresh_token"},
+ },
Review Comment:
**Suggestion:** The prefilled scope requests only `refresh_token`; Snowflake also needs a `session:role-*` scope, so the resulting token cannot open a session and queries fail.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes` · 🏷️ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4cf3f0eddfba4ee1883230398b235d39&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4cf3f0eddfba4ee1883230398b235d39&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/snowflake.py
**Line:** 140:143
**Comment:**
*Api Mismatch: The prefilled scope requests only `refresh_token`; Snowflake also needs a `session:role-*` scope, so the resulting token cannot open a session and queries fail.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=8f98ce73f47d4bfe40ee9dee0238a3423217dd72848f60b58556fc5978f4b0fa&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=8f98ce73f47d4bfe40ee9dee0238a3423217dd72848f60b58556fc5978f4b0fa&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4296159585
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `9a301fd`.
The endpoint and scope defaults now use nullish coalescing, so an explicitly cleared empty string is preserved instead of being replaced by the default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `9a301fd`.
Credential-shape validation now delegates to the supplied `validate_credentials` function, allowing engine-specific validators to accept alternate complete credential shapes.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "geido (via GitHub) via notifications" <no...@superset.apache.org>.
geido commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4444372408
##########
superset/databases/schemas.py:
##########
@@ -1146,25 +1237,39 @@ 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(),
- )
+ if existing := db.session.query(Database).filter_by(uuid=data["uuid"]).first():
+ if not self.overwrites_existing:
+ return
Review Comment:
I think this needs the `can_write` check `import_database()` uses, since without it the connection is left alone. As Alpha, an "overwrite all" dashboard re-import now asks for the OAuth2 client secret and drops it, and `/api/v1/assets/import/` 500s when the bundle's URI or `engine_params` differ. Could we skip this for users who can't write databases? Both worked for Alpha at bdf7917.
Alpha user, flags off, previous head then this one:
https://github.com/user-attachments/assets/1cc6daa6-b4a7-4505-a536-f5ebc6064e29
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4277753255
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `f88330a`.
The endpoint and scope defaults now use nullish coalescing (`??`), so an explicitly cleared empty string remains empty instead of restoring the displayed default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `f88330a`.
Credential completeness now delegates to `validate_credentials(value)`, allowing the engine-specific validator to determine whether a credential shape is complete.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4412992862
##########
superset/utils/json.py:
##########
@@ -362,6 +367,32 @@ def get_masked_fields(
return masked
+def unrevealable_masked_fields(
+ old_payload: dict[str, Any] | None,
+ new_payload: dict[str, Any],
+ sensitive_fields: set[str],
+) -> list[str]:
+ """
+ Returns masked fields in ``new_payload`` that ``old_payload`` cannot reveal.
+
+ :func:`reveal_sensitive` swaps a mask back for the stored value at the same
+ path. Where the stored payload has no value there, the mask would be kept and
+ stored as if it were the secret, so callers that must not do that (imports
+ overwriting a connection) use this to ask for the real value instead.
+
+ :param old_payload: The stored payload, if any
+ :param new_payload: The incoming payload, possibly containing masks
+ :param sensitive_fields: The set of fields to check, as JSONPath expressions
+ :returns: JSONPath expressions for masked fields with nothing to reveal
+ """
+ return [
+ field
+ for field in get_masked_fields(new_payload, sensitive_fields)
+ if old_payload is None
+ or not any(match.value for match in parse(field).find(old_payload))
Review Comment:
I think a stored mask slips through here. master saves `XXXXXXXXXX` when an overwrite import can't reveal a secret; this counts it as revealable, so the schema asks for nothing and the backstop in `import_database` 500s with no field to enter it. Could a stored `PASSWORD_MASK` count as missing, so the normal prompt shows? I hit it re-importing a bundle right after upgrading from master.
Same local instance before and after upgrading (flags off):
https://github.com/user-attachments/assets/e0cf0760-a461-46f6-8366-d5afc513734e
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #45068:
URL: https://github.com/apache/superset/pull/45068#issuecomment-6496949134
Recordings for my inline note on `superset/commands/dataset/export.py`, with both flags off: this PR at bdf7917 first, then the same steps on master (b411454508).
https://github.com/user-attachments/assets/0347858b-cac0-444f-b189-d8c06efd7068
https://github.com/user-attachments/assets/221aabaa-689d-4392-968a-7fd4b73fb509
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "hughhhh (via GitHub) via notifications" <no...@superset.apache.org>.
hughhhh closed pull request #45068: feat(db): embedded-only fallback credential on OAuth2 connections
URL: https://github.com/apache/superset/pull/45068
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub) via notifications" <no...@superset.apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4432568216
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `3608687`.
Endpoint and scope defaults now use nullish coalescing (`??`), so an explicitly cleared empty string is preserved instead of being replaced by the displayed default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `3608687`.
Credential completeness now delegates to `validate_credentials(value)`, with the helper documenting that callers use the engine’s query-time validator when the connection is known.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4284769885
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `74c1c47`.
The endpoint and scope defaults now use nullish coalescing, so an explicitly cleared empty string is preserved instead of being replaced by the default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `74c1c47`.
Credential-shape validation now delegates to the supplied `validate_credentials` function, allowing engine-specific validators to accept alternate complete credential shapes.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4277537125
##########
superset/models/core.py:
##########
@@ -746,7 +755,50 @@ def _get_sqla_engine( # pylint: disable=too-many-locals # noqa: C901
masked_url = self.get_password_masked_url(sqlalchemy_url)
logger.debug("Database._get_sqla_engine(). Masked URL: %s", str(masked_url))
- if self.impersonate_user:
+ if embedded_fallback:
+ embedded_credentials = self.get_embedded_fallback_credentials()
+ if embedded_credentials is None:
+ # Falling through to the base credential here is the failure mode this
+ # whole feature exists to prevent: an embedded viewer would silently
+ # query as whichever user the connection URL carries. That happens on
+ # a per-user OAuth2 connection with no (or a half-filled)
+ # credential configured -- none of which the viewer or
+ # the dashboard author can see. Refuse instead, with a message an
+ # administrator can act on.
+ raise SupersetErrorException(
+ SupersetError(
+ message=__(
+ "This dashboard's database connection authenticates users "
+ "individually and has no credential configured for "
+ "embedded viewers, so this query cannot run."
+ ),
+ error_type=SupersetErrorType.CONNECTION_ACCESS_DENIED_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ # Deliberately *instead of* impersonation, not alongside it, and this is
+ # load-bearing twice over. First, an engine spec that opts in sets its
+ # OAuth2 authenticator during impersonation whether or not a token exists,
+ # so skipping it is what lets the stored credential authenticate at all.
+ # Second, `effective_username` above resolves to `g.user.username`, and a
+ # `GuestUser` carries whatever username the guest token claims -- so going
+ # through impersonation would let whoever mints the token choose the
+ # connection identity.
+ #
+ # How the credential reaches the driver is the engine's business -- a
+ # password on the URL, a PEM key in connect_args. Both the URL and the
+ # engine kwargs participate in the cache key below, keeping guest and
+ # OAuth engines separate even when they use the same database username.
+ sqlalchemy_url, engine_kwargs = (
+ self.db_engine_spec.apply_embedded_credentials(
+ self,
+ embedded_credentials,
+ sqlalchemy_url,
+ engine_kwargs,
+ )
+ )
Review Comment:
**Suggestion:** `DB_CONNECTION_MUTATOR` later receives `effective_username`, which comes from the guest token, and can rewrite the URL away from the configured embedded identity.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes` · 🏷️ `Security`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0b4f46f639194f10b90007b64560a828&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0b4f46f639194f10b90007b64560a828&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/core.py
**Line:** 793:800
**Comment:**
*Security: `DB_CONNECTION_MUTATOR` later receives `effective_username`, which comes from the guest token, and can rewrite the URL away from the configured embedded identity.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=b4a14817332c16112fbdf5f422430b378358849289a046149aa0b61905d5e561&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=b4a14817332c16112fbdf5f422430b378358849289a046149aa0b61905d5e561&reaction=dislike'>👎</a>
##########
docs/docs/using-superset/embedding.mdx:
##########
@@ -125,6 +125,84 @@ 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 in the connection dialog — but only on a connection that
+actually uses OAuth2, either through per-connection client information or a
+deployment-wide client in `DATABASE_OAUTH2_CLIENTS`. On any other connection the
+section is hidden, because the credential would never be read. Fallback requires
+**Impersonate logged in user** under *Advanced → Security*; the section remains
+available before enabling it so you can configure credentials during setup.
+
+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.
+
+Choose **Password** or **Key pair**. 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. 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. A key pair supplied
+here applies only to embedded guests; the connection's ordinary credential is still used
+for connection tests, alerts, and reports. Guest queries restored by
+`GLOBAL_ASYNC_QUERIES` use the same fallback as synchronous guest queries.
+
+The credential must be complete: a username with no password or private key, or the
+reverse, is rejected when both embedding flags are enabled. Validation uses the
+edited credential, so an incomplete imported credential can be completed or cleared.
+While either flag is disabled, the stored credential is preserved and does not block
+unrelated edits. Clearing every field removes the credential.
+
+After connecting, turn on **Impersonate logged in user** under *Advanced → Security*
+and save. Until it is enabled, the embedded credential is inactive and the connection
+keeps its ordinary service-account behavior. The dialog warns when a credential is
+set and impersonation is off.
+
+Understand what this grants before enabling it:
+
+- **Every embedded viewer of that connection queries as that one database user.** Guest
+ tokens carry no database identity, so per-user attribution and any database-side
+ row-level security keyed to the user do not apply. Use a dedicated account with the
+ narrowest access the embedded dashboards need, and enforce per-viewer restrictions
+ with Superset's own [row-level security](#security-notes) rules in the guest token.
+- **Logged-in users are unaffected.** They continue to authenticate individually via
+ OAuth2 and have no route to the stored credential.
+- **The guest token's `username` claim is never used to connect.** Whoever mints the
+ token controls that claim, so it is deliberately kept out of the connection identity.
+- **An embedded viewer never silently falls back to the connection's base credential.**
+ With this flag on, a guest on a per-user OAuth2 connection with impersonation enabled
+ that has no applicable credential —
+ none configured, or one that is incomplete — is refused with an error rather than
+ connected as whichever account the connection URL carries. Deployments that leave the
+ flag off keep their existing behaviour.
+- **Guest results are cached separately.** Queries run under the fallback credential get
+ their own cache identity, so an embedded viewer and a logged-in user never share a
+ cached result (or, with `GLOBAL_ASYNC_QUERIES`, a deduplicated query task) for the same
+ chart. Missing or incomplete credentials also receive a separate cache key, so a
Review Comment:
**Suggestion:** When per-user caching is enabled, a logged-in username equal to `embedded:<fallback username>` gets the same cache key, allowing guest and logged-in results to cross.
**Assessment:** 🔴 `Critical` · 🔁 `Occurrence: Rarely` · 🏷️ `Security`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=027db0ea6e004d29ba96bbaac4f50d65&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=027db0ea6e004d29ba96bbaac4f50d65&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/docs/using-superset/embedding.mdx
**Line:** 199:201
**Comment:**
*Security: When per-user caching is enabled, a logged-in username equal to `embedded:<fallback username>` gets the same cache key, allowing guest and logged-in results to cross.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=911b716319464b0906fece8e3eb94418cf1873d0888508be266c23b48cde3276&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=911b716319464b0906fece8e3eb94418cf1873d0888508be266c23b48cde3276&reaction=dislike'>👎</a>
##########
superset/commands/database/validate.py:
##########
@@ -236,6 +242,54 @@ def _get_database_name_error(self) -> Optional[SupersetError]:
)
return None
+ def _get_embedded_credentials_error(self) -> Optional[SupersetError]:
+ """Reject an embedded fallback credential that is missing one of its halves.
+
+ `Database.get_embedded_fallback_credentials` ignores such a credential, so
+ saving one leaves a setting that reads as configured and never applies.
+
+ The connection dialog validates *before* it folds `x-encrypted-extra`
+ parameters into `masked_encrypted_extra`, so the value arrives under
+ `parameters` from the dialog and under `masked_encrypted_extra` from a direct
+ API caller. An explicit form value replaces the stored value, including
+ when the credential is cleared. Disabled fields must not block edits.
+ """
+ if not (
+ is_feature_enabled("EMBEDDED_CREDENTIAL_FALLBACK")
+ and is_feature_enabled("EMBEDDED_SUPERSET")
+ ):
+ return None
+
+ parameters = self._properties.get("parameters") or {}
+ if EMBEDDED_CREDENTIALS_KEY in parameters:
+ credentials = parameters[EMBEDDED_CREDENTIALS_KEY]
+ else:
+ try:
+ encrypted_extra = json.loads(
+ self._properties.get("masked_encrypted_extra") or "{}"
+ )
+ except json.JSONDecodeError:
+ encrypted_extra = {}
+ credentials = (
+ encrypted_extra.get(EMBEDDED_CREDENTIALS_KEY)
+ if isinstance(encrypted_extra, dict)
+ else None
+ )
+
+ if not embedded_credentials_incomplete(credentials):
Review Comment:
**Suggestion:** A malformed JSON string in `parameters.embedded_credentials` makes the completeness check pass, so bypass engines report success for a credential that cannot be applied.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=de4b26516f114fa785bb97fb53f15db3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=de4b26516f114fa785bb97fb53f15db3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/database/validate.py
**Line:** 279:279
**Comment:**
*Api Mismatch: A malformed JSON string in `parameters.embedded_credentials` makes the completeness check pass, so bypass engines report success for a credential that cannot be applied.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=3331d37253a06e5cbdbed25df7673a38b77f27fd0331a74e427fb86fcfceb6cb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45068&comment_hash=3331d37253a06e5cbdbed25df7673a38b77f27fd0331a74e427fb86fcfceb6cb&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45068:
URL: https://github.com/apache/superset/pull/45068#issuecomment-6048323330
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45068?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `55.12821%` with `35 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 66.14%. Comparing base ([`888795b`](https://app.codecov.io/gh/apache/superset/commit/888795b90f9999adabc19263177cf6bb5500e191?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`50ebf80`](https://app.codecov.io/gh/apache/superset/commit/50ebf8065a97ea5df80e4c2915dcea5e50d049d4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 8 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/45068?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/models/core.py](https://app.codecov.io/gh/apache/superset/pull/45068?src=pr&el=tree&filepath=superset%2Fmodels%2Fcore.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | 13.63% | [17 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/45068?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/db\_engine\_specs/base.py](https://app.codecov.io/gh/apache/superset/pull/45068?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fbase.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | 36.36% | [5 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/45068?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/db\_engine\_specs/snowflake.py](https://app.codecov.io/gh/apache/superset/pull/45068?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fsnowflake.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Nub3dmbGFrZS5weQ==) | 50.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/45068?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/oauth2.py](https://app.codecov.io/gh/apache/superset/pull/45068?src=pr&el=tree&filepath=superset%2Futils%2Foauth2.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvb2F1dGgyLnB5) | 66.66% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/45068?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (888795b) and HEAD (50ebf80). Click for more details.
>
> <details><summary>HEAD has 4 uploads less than BASE</summary>
>
>| Flag | BASE (888795b) | HEAD (50ebf80) |
>|------|------|------|
>|python|7|3|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #45068 +/- ##
===========================================
- Coverage 82.59% 66.14% -16.46%
===========================================
Files 3005 3006 +1
Lines 186925 187012 +87
Branches 43302 43326 +24
===========================================
- Hits 154389 123690 -30699
- Misses 29772 60619 +30847
+ Partials 2764 2703 -61
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/45068/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/45068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.19% <20.75%> (-0.02%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/45068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.34% <100.00%> (+<0.01%)` | :arrow_up: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/45068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/45068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/45068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.04% <20.75%> (-0.02%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/45068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `54.92% <33.96%> (-31.48%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/45068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `54.80% <33.96%> (-0.02%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/45068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45068?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] [WIP] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45068:
URL: https://github.com/apache/superset/pull/45068#issuecomment-6048023820
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 50ebf8065a97ea5df80e4c2915dcea5e50d049d4 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ac6c4ee78e7e70008580af8 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-45068--superset-docs-preview.netlify.app](https://deploy-preview-45068--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpBCoAgEEDRu8w6SaJUukHHGJ0xIU0RXUR09_yrv3gvlJpTabADTGAr3i6MD_0Mo5mTZSImkbG3sAiPMVp017DEJebnoKEVOuVWZm1Ys5ZSms1I9Aa-Hw) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-45068--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4319525707
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -1097,3 +1099,89 @@ def test_execute_with_oauth2_retry_survives_lock_contention(
assert lock.call_count == OAUTH2_LOCK_BACKOFF_MAX_TRIES
db.session.query().filter_by().one_or_none.assert_called()
+
+
+def _guest_user() -> GuestUser:
+ """
+ Build an embedded guest principal, the way the guest-token loader would.
+ """
+ token: GuestToken = {
+ "user": {"username": "guest_user"},
+ "resources": [],
+ "rls_rules": [],
+ "iat": 0,
+ "exp": 1,
+ }
+ return GuestUser(token=token, roles=[])
+
+
+@with_feature_flags(EMBEDDED_SUPERSET=True)
+def test_check_for_oauth2_starts_the_dance_for_a_logged_in_user(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that `check_for_oauth2` still starts the dance for a real user.
+ """
+ mocker.patch("superset.utils.oauth2.g").user = mocker.MagicMock(
+ spec=["id", "username"]
+ )
+
+ database = mocker.MagicMock()
+ database.is_oauth2_enabled.return_value = True
+
+ with pytest.raises(OAuth2TokenRefreshError):
+ with check_for_oauth2(database):
+ raise OAuth2TokenRefreshError()
+
+ database.db_engine_spec.start_oauth2_dance.assert_called_once_with(database)
+
+
+@with_feature_flags(EMBEDDED_SUPERSET=True)
+def test_check_for_oauth2_skips_the_dance_for_an_embedded_guest(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that an embedded guest does not trigger the dance from `check_for_oauth2`.
+
+ The `OAuth2TokenRefreshError` arm of the condition reaches the dance without
+ consulting `needs_oauth2`, so the guest check has to be repeated here. Without it
+ an embedded viewer reaches `start_oauth2_dance`, which needs a user id the guest
+ token does not carry.
+ """
+ mocker.patch("superset.utils.oauth2.g").user = _guest_user()
+
+ database = mocker.MagicMock()
+ database.is_oauth2_enabled.return_value = True
+
+ with pytest.raises(OAuth2TokenRefreshError):
+ with check_for_oauth2(database):
+ raise OAuth2TokenRefreshError()
+
+ database.db_engine_spec.start_oauth2_dance.assert_not_called()
+
+
+@with_feature_flags(EMBEDDED_SUPERSET=True)
+def test_execute_with_oauth2_retry_skips_the_dance_for_an_embedded_guest(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that a guest gets the driver's error out of `execute_with_oauth2_retry`.
+
+ This passes on the strength of the `needs_oauth2` guard alone -- every arm of the
+ retry sits behind `is_oauth2_error`. It is here to pin that ordering, which is
+ easy to lose in a refactor of the surrounding branches.
+ """
+ mocker.patch("superset.utils.oauth2.g").user = _guest_user()
+ mocker.patch("superset.utils.oauth2.db")
+
+ database = mocker.MagicMock()
+ database.is_oauth2_enabled.return_value = True
+ database.db_engine_spec.needs_oauth2.return_value = False
Review Comment:
This test forces `needs_oauth2` to `False` on the mocked engine spec, so it passes whether or not the real guest guard exists, and the final assertion checks `database.start_oauth2_dance` rather than `database.db_engine_spec.start_oauth2_dance`, which the code never calls. A regression that lets a guest into the OAuth2 dance would go unnoticed here. Could this use a real engine spec (or the real `BaseEngineSpec.needs_oauth2`) with the guest principal and assert the dance on the engine spec is not called?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4300423152
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `aea27b5`.
The endpoint and scope defaults now use nullish coalescing, so an explicitly cleared empty string is preserved instead of being replaced by the default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `aea27b5`.
Credential-shape validation now delegates to the supplied `validate_credentials` function, allowing engine-specific validators to accept alternate complete credential shapes.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4412426383
##########
superset/commands/dataset/export.py:
##########
@@ -36,6 +37,37 @@
JSON_KEYS = {"params", "template_params", "extra"}
+# Secure Extra blocks a dashboard, chart or dataset bundle carries, so a promoted
+# dashboard keeps its OAuth2 client and embedded viewer credential. Every engine
+# masks their secrets (`BaseEngineSpec.encrypted_extra_sensitive_field_paths`).
+_CARRIED_SECURE_EXTRA_KEYS = ("oauth2_client_info", EMBEDDED_CREDENTIALS_KEY)
+
+
+def _carried_secure_extra(masked_encrypted_extra: str | None) -> str | None:
+ """
+ Return the masked Secure Extra blocks a bundle carries, or ``None``.
+
+ Only these Superset-owned blocks, never the rest of Secure Extra: the bundle
+ is open to anyone who can export a dashboard or dataset, and an engine's own
+ settings -- client certificates, service-account keys -- are not all
+ registered as sensitive, so masking cannot be relied on to hide them.
+ """
+ if not masked_encrypted_extra:
+ return None
+ try:
+ secure_extra = json.loads(masked_encrypted_extra)
+ except json.JSONDecodeError:
+ return None
+ if not isinstance(secure_extra, dict):
+ return None
+ carried = {
+ key: secure_extra[key]
+ for key in _CARRIED_SECURE_EXTRA_KEYS
+ if secure_extra.get(key)
+ }
Review Comment:
I think this needs a type check before merge. Databricks keeps the base `$.*` mask, so this carries `{"oauth2_client_info": "XXXXXXXXXX"}`; a fresh import asks for `$.oauth2_client_info`, saves what's typed as a string, and `get_oauth2_config()` fails every query with "Invalid input type". Could we carry a block only when it's still a dict? Flag off, and the same dashboard imported fine before.
This PR, flags off:
https://github.com/user-attachments/assets/0347858b-cac0-444f-b189-d8c06efd7068
Same steps on master:
https://github.com/user-attachments/assets/221aabaa-689d-4392-968a-7fd4b73fb509
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4278464667
##########
superset/utils/cache_keys.py:
##########
@@ -38,6 +38,34 @@ 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"] = (
+ database.db_engine_spec.get_embedded_credential_cache_key(
+ embedded_credentials
+ )
+ )
+ return
Review Comment:
This early return skips the `CACHE_QUERY_BY_USER` / `per_user_caching` branch below. With per-user caching on and a dataset whose Jinja uses `current_username(add_to_cache_keys=False)` (or an RLS clause on it), two embedded guests get the same `embedded_credentials_key` while rendering different SQL, so the second guest can be served the first guest's cached rows. Should the guest-namespace key be added alongside the existing per-user key instead of returning early?
##########
tests/unit_tests/db_engine_specs/test_snowflake.py:
##########
@@ -856,6 +857,392 @@ def test_snowflake_oauth2_exception_catches_refresh_token_error() -> None:
)
+def test_parameters_json_schema_has_encrypted_extra() -> None:
+ """
+ Test that ``parameters_json_schema`` marks ``oauth2_client_info`` with
+ ``x-encrypted-extra``.
+
+ That annotation is what makes the connection dialog render the OAuth2 client
+ fields and move their values into ``masked_encrypted_extra`` instead of posting
+ them as plain parameters. Without it the whole form half of Snowflake's OAuth2
+ support is unreachable, and an admin has to hand-edit the Secure extra JSON.
+ """
+ from superset.db_engine_specs.snowflake import SnowflakeEngineSpec
+
+ schema = SnowflakeEngineSpec.parameters_json_schema()
+
+ assert schema is not None
+ oauth2_client_info = schema["properties"]["oauth2_client_info"]
+ assert oauth2_client_info["x-encrypted-extra"] is True
+ assert "default" not in oauth2_client_info
+
+
+def test_parameters_json_schema_keeps_oauth2_client_info_optional() -> None:
+ """
+ Test that adding ``oauth2_client_info`` does not make it a required parameter.
+
+ Every other Snowflake parameter is required; a connection that does not use
+ OAuth2 must still validate.
+ """
+ from superset.db_engine_specs.snowflake import SnowflakeEngineSpec
+
+ schema = SnowflakeEngineSpec.parameters_json_schema()
+
+ assert schema is not None
+ assert "oauth2_client_info" in schema["properties"]
+ assert "oauth2_client_info" not in schema.get("required", [])
+ assert sorted(schema["required"]) == [
+ "account",
+ "database",
+ "password",
+ "role",
+ "username",
+ "warehouse",
+ ]
+
+
+def test_validate_parameters_unaffected_by_oauth2_client_info() -> None:
+ """
+ Test that supplying ``oauth2_client_info`` neither satisfies nor breaks
+ ``validate_parameters``.
+
+ Snowflake still needs a base credential for the connection test it runs on save,
+ so the required set must not shift just because OAuth2 is being configured.
+ """
+ from superset.db_engine_specs.snowflake import SnowflakeEngineSpec
+
+ properties: dict[str, Any] = {
+ "parameters": {
+ "oauth2_client_info": {"id": "client-id", "secret": "client-secret"},
+ }
+ }
+
+ errors = SnowflakeEngineSpec.validate_parameters(properties) # type: ignore[arg-type]
+
+ assert len(errors) == 1
+ assert errors[0].extra is not None
+ assert errors[0].extra["missing"] == [
+ "account",
+ "database",
+ "password",
+ "role",
+ "username",
+ "warehouse",
+ ]
+
+
+def test_build_sqlalchemy_uri_does_not_leak_the_oauth2_client() -> None:
+ """
+ Test that the OAuth2 client id and secret never reach the stored URI.
+
+ ``build_sqlalchemy_uri`` reads only named parameters, so this holds by
+ construction -- but a client secret leaked into a connection string would be the
+ worst possible outcome of exposing this field, so it is pinned rather than
+ reasoned about.
+ """
+ from superset.db_engine_specs.snowflake import SnowflakeEngineSpec
+
+ parameters: dict[str, Any] = {
+ "username": "username",
+ "password": "password",
+ "account": "account",
+ "database": "database",
+ "role": "role",
+ "warehouse": "warehouse",
+ "oauth2_client_info": {
+ "id": "super-secret-client-id",
+ "secret": "super-secret-client-secret",
+ },
+ }
+
+ uri = SnowflakeEngineSpec.build_sqlalchemy_uri(
+ parameters, # type: ignore[arg-type]
+ encrypted_extra={
+ "oauth2_client_info": {
+ "id": "super-secret-client-id",
+ "secret": "super-secret-client-secret",
+ }
+ },
+ )
+
+ assert "super-secret-client-id" not in uri
+ assert "super-secret-client-secret" not in uri
+ assert "oauth2_client_info" not in uri
+
+
+def test_update_params_from_encrypted_extra_does_not_leak_the_oauth2_client(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that the OAuth2 client does not end up in the engine kwargs.
+
+ An unrecognised key reaching ``create_engine(**engine_kwargs)`` would raise, and
+ a recognised one would be sent to the driver; neither is wanted for the client
+ config, which is consumed by Superset itself.
+ """
+ from superset.db_engine_specs.snowflake import SnowflakeEngineSpec
+
+ database = mocker.MagicMock()
+ database.encrypted_extra = json.dumps(
+ {"oauth2_client_info": {"id": "client-id", "secret": "client-secret"}}
+ )
+
+ params: dict[str, object] = {}
+ SnowflakeEngineSpec.update_params_from_encrypted_extra(database, params)
Review Comment:
`database` is a bare `MagicMock`, so `database.embedded_fallback_applies()` returns a truthy mock and `update_params_from_encrypted_extra` returns at the new guest early-return before it reads `encrypted_extra`. The assertions pass without exercising the code they're meant to guard, so a regression that forwards `oauth2_client_info` into the engine kwargs would go unnoticed. Could this set `database.embedded_fallback_applies.return_value = False`?
##########
docs/sip/embedded-fallback-credential.md:
##########
@@ -0,0 +1,279 @@
+<!--
+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 raises a
+`CONNECTION_ACCESS_DENIED_ERROR` instead. The refusal is scoped to the flag so that a
+deployment which has not opted in keeps its existing behaviour.
+
+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
+explaining that it is 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()`.
Review Comment:
`validated_embedded_credentials()` is listed as a `Database` method, but at this head it's a helper in `superset.databases.utils`; the extension hook is `BaseEngineSpec.validate_embedded_credentials()`. Anyone following this list and calling it on a `Database` gets an `AttributeError`. Could the interface list name the engine-spec hook instead?
##########
superset/utils/oauth2.py:
##########
@@ -596,6 +614,10 @@ def check_for_oauth2(database: Database) -> Iterator[None]:
if (
not is_oauth2_retry_active()
and database.is_oauth2_enabled()
Review Comment:
For an async guest request in a worker with no request context and no `redirect_uri` / `DATABASE_OAUTH2_REDIRECT_URI` / `SERVER_NAME`, `database.is_oauth2_enabled()` loads the client config, which resolves the redirect URI and raises `OAuth2Error` before `_is_guest_request()` is evaluated. That replaces the new access-denied refusal (or the driver's own error) for guests. Could `_is_guest_request()` be checked first in this condition?
##########
superset/databases/utils.py:
##########
@@ -160,3 +162,86 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. Engines may override this default through
+ ``BaseEngineSpec.validate_embedded_credentials`` for both query-time and
+ authoring-time validation.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not _is_present(username):
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(
+ value: Any,
+ validate_credentials: Callable[[Any], dict[str, str] | None],
+) -> bool:
+ """
+ Is an embedded credential malformed or incomplete for its engine?
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+ Use the engine's query-time validator when the connection is known, keeping
+ authoring checks consistent with engine-specific credential shapes.
+ """
+ if value is None or value == "":
+ return False
+
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
Review Comment:
This accepts an `embedded_credentials` value that is a JSON-encoded string, but the string is what gets persisted, and the query-time `validated_embedded_credentials()` requires a dict. Saving `{"embedded_credentials": "{\"username\":\"svc\",\"password\":\"pw\"}"}` passes validation, then every guest query is refused with a connection-access error, and the string form also skips the nested secret masking paths. Should the string form be rejected here, or normalized to a dict before it is stored?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4283432488
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `5a0ac6c`.
The endpoint and scope defaults now use nullish coalescing, so an explicitly cleared empty string is preserved instead of being replaced by the default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `5a0ac6c`.
Credential-shape validation now delegates to the supplied `validate_credentials` function, allowing engine-specific validators to accept alternate complete credential shapes.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(db): embedded-only fallback credential on OAuth2 connections [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45068:
URL: https://github.com/apache/superset/pull/45068#discussion_r4319287449
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -42,78 +42,109 @@ interface OAuth2ClientInfo {
scope: string;
}
+/**
+ * Coerce a backend- or form-supplied value to a plain object.
+ *
+ * `masked_encrypted_extra` and the `parameters.*` entries arrive either as an
+ * object or as a JSON string -- the save-time merge in `DatabaseModal`
+ * re-serializes `parameters.*` in place -- and `masked_encrypted_extra` has
+ * historically been the literal string "null". `JSON.parse('null')` returns
+ * null and malformed JSON throws, so both are absorbed here.
+ */
+const asObject = (value: unknown): Record<string, unknown> => {
+ if (typeof value === 'string') {
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(value || '{}');
+ } catch (e) {
+ // Only swallow JSON.parse's own SyntaxError; let real programmer errors
+ // (RangeError, TypeError from a broken stub, etc.) propagate.
+ if (!(e instanceof SyntaxError)) throw e;
+ return {};
+ }
+ return asObject(parsed);
+ }
+
+ return value && typeof value === 'object' && !Array.isArray(value)
+ ? (value as Record<string, unknown>)
+ : {};
+};
+
export const OAuth2ClientField = ({
changeMethods,
db,
default_value: defaultValue,
isPublic = true,
}: FieldPropTypes) => {
const deriveOauth2ClientInfo = (): OAuth2ClientInfo => {
- // `masked_encrypted_extra` is user/backend-supplied and historically
- // sometimes the string "null" — JSON.parse('null') returns null, and
- // malformed JSON throws. Defend against both so a single bad value
- // can't crash the component.
- let parsed: unknown;
- try {
- parsed = JSON.parse(db?.masked_encrypted_extra || '{}');
- } catch (e) {
- // Only swallow JSON.parse's own SyntaxError; let real programmer
- // errors (RangeError, TypeError from a broken stub, etc.) propagate.
- if (!(e instanceof SyntaxError)) throw e;
- parsed = {};
- }
- const encryptedExtra =
- parsed && typeof parsed === 'object' && !Array.isArray(parsed)
- ? (parsed as { oauth2_client_info?: Partial<OAuth2ClientInfo> })
- : {};
- const info = encryptedExtra.oauth2_client_info;
+ // The in-flight form value wins over the stored blob. Switching the
+ // configuration method to SQLAlchemy URI and back re-mounts this field
+ // while `parameters.oauth2_client_info` still holds what was typed;
+ // seeding from `masked_encrypted_extra` alone would show empty inputs for
+ // a client the save payload would nonetheless carry.
+ const typed = asObject(db?.parameters?.oauth2_client_info);
+ const stored = asObject(
+ asObject(db?.masked_encrypted_extra).oauth2_client_info,
+ );
+ const info: Partial<OAuth2ClientInfo> = (
+ Object.keys(typed).length ? typed : stored
+ ) as Partial<OAuth2ClientInfo>;
+
return {
- id: info?.id || '',
- secret: info?.secret || '',
+ id: info.id || '',
+ secret: info.secret || '',
authorization_request_uri:
- info?.authorization_request_uri ||
+ info.authorization_request_uri ||
defaultValue?.authorization_request_uri ||
'',
token_request_uri:
- info?.token_request_uri || defaultValue?.token_request_uri || '',
- scope: info?.scope || defaultValue?.scope || '',
+ info.token_request_uri || defaultValue?.token_request_uri || '',
+ scope: info.scope || defaultValue?.scope || '',
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `bdf7917`.
Endpoint and scope defaults now use nullish coalescing (`??`), so an explicitly cleared empty string is preserved instead of being replaced by the displayed default.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/databases/utils.py:
##########
@@ -160,3 +161,93 @@ def make_url_safe(raw_url: str | URL) -> URL:
else:
return raw_url
+
+
+def _is_present(value: Any) -> bool:
+ """Is a credential field actually filled in? Whitespace does not count."""
+ if isinstance(value, str):
+ return bool(value.strip())
+ return bool(value)
+
+
+def validated_embedded_credentials(credentials: Any) -> dict[str, str] | None:
+ """
+ Return a complete embedded fallback credential, or ``None``.
+
+ The canonical shape is ``{username, password}`` or ``{username, auth_method:
+ "keypair", privatekey_body, privatekey_pass}``; a half-filled credential is
+ ignored rather than half-applied. This is the single definition of what
+ "complete" means, reached from three directions: the query-time gate
+ (``BaseEngineSpec.validate_embedded_credentials``, which an engine may
+ override), the save-time check below, and the API schema validators. A second
+ copy of these rules would eventually reject a valid key pair for carrying no
+ password.
+ """
+ if not isinstance(credentials, dict):
+ return None
+
+ username = credentials.get("username")
+ if not isinstance(username, str) or not username:
+ return None
+
+ auth_method = credentials.get("auth_method", "password")
+ if auth_method == "keypair":
+ privatekey_body = credentials.get("privatekey_body")
+ privatekey_pass = credentials.get("privatekey_pass", "")
+ if (
+ not isinstance(privatekey_body, str)
+ or not privatekey_body
+ or not isinstance(privatekey_pass, str)
+ ):
+ return None
+ return {
+ "username": username,
+ "auth_method": "keypair",
+ "privatekey_body": privatekey_body,
+ "privatekey_pass": privatekey_pass,
+ }
+
+ password = credentials.get("password")
+ if auth_method != "password" or not isinstance(password, str) or not password:
+ return None
+ return {"username": username, "password": password}
+
+
+def embedded_credentials_incomplete(value: Any) -> bool:
+ """
+ Does ``value`` hold an ``embedded_credentials`` entry that is only half filled?
+
+ Asks :func:`validated_embedded_credentials` rather than restating its rules, so
+ the save-time check cannot drift from the query-time one.
+
+ A credential missing a half is ignored at query time, which is the right
+ behaviour there and the wrong one at authoring time: it saves and then silently
+ does nothing. So the configuration entry points reject it up front.
+
+ Absent and entirely blank values pass -- they mean "no credential configured".
+ Accepts the value as a dict or as the JSON string the connection dialog posts.
+
+ Deliberately engine-agnostic: it is reached from marshmallow field validators
+ that have no connection, and therefore no engine spec, in hand. An engine that
+ overrides ``validate_embedded_credentials`` to accept a shape the canonical rule
+ rejects must also relax its own ``validate_parameters``.
+ """
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ # Not our error to report: the surrounding JSON validators already
+ # reject undecodable payloads with a message about the real problem.
+ return False
+
+ if not isinstance(value, dict):
+ return False
+
+ # `auth_method` alone is not a credential -- the dialog writes it as soon as the
+ # selector renders, so an untouched sub-form must not count as half-filled.
+ if not any(
+ _is_present(field) for key, field in value.items() if key != "auth_method"
+ ):
+ return False
+
+ return validated_embedded_credentials(value) is None
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `bdf7917`.
Credential completeness now delegates to `validate_credentials(value)`, with the helper documenting that callers use the engine’s query-time validator when the connection is known.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org