sadpandajoe commented on code in PR #45890:
URL: https://github.com/apache/superset/pull/45890#discussion_r4447957488
##########
superset/utils/cache_keys.py:
##########
@@ -38,6 +38,38 @@ def add_impersonation_cache_key_if_needed(
Add a per-user cache-key when the DB connection is configured for
per-user caching, no-op otherwise.
"""
+ # The embedded fallback credential replaces the connection identity
*without*
+ # going through impersonation (`Database._get_sqla_engine`), so none of the
+ # conditions below would notice it. Its result must never share a
DATA-cache
Review Comment:
This says an embedded result must never share a DATA-cache entry with a
logged-in user's per-user result, but the native-filter value lookup still
does: `_column_values_cache_key` in `superset/datasource/api.py` keys on
dataset, column, RLS and `changed_on`, with no user or connection identity.
With impersonation on and an embedded credential less privileged than an
analyst's OAuth2 identity, an analyst opening the dashboard's select filter
caches the `DISTINCT` values under their own identity, and a guest loading the
same filter before the cache expires gets values the embedded identity couldn't
read (and the reverse). Should that key take the same embedded/impersonation
discriminator?
##########
superset-frontend/src/components/ImportModal/index.tsx:
##########
@@ -351,20 +351,39 @@ export const ImportModal:
FunctionComponent<ImportModelsModalProps> = ({
{field.label}
<span className="required">*</span>
</div>
- <Input.Password
- name={`encrypted-extra-${fileName}-${field.path}`}
- value={encryptedExtraSecrets[fileName]?.[field.path] || ''}
- onChange={event =>
- setEncryptedExtraSecrets({
- ...encryptedExtraSecrets,
- [fileName]: {
- ...encryptedExtraSecrets[fileName],
- [field.path]: event.target.value,
- },
- })
- }
- data-test="encrypted_extra_secret"
- />
+ {/* A PEM key is unusable without its newlines. */}
+ {/private_?key(_body)?$/.test(field.path) ? (
Review Comment:
This PEM textarea only covers the asset-bundle imports. A database-file
import asks for secrets through `encryptedExtraNeededField` in
`DatabaseModal/index.tsx` (the `ValidatedInput` with `visibilityToggle`, around
line 1777), which is still a single-line password input. An exported Snowflake
database that carries an embedded key pair asks for
`embedded_credentials.privatekey_body` there, so a pasted PEM loses its
newlines and the stored key can't be loaded for embedded queries. Should that
renderer use the same textarea for private-key fields?
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -552,11 +559,23 @@ export function dbReducer(
return {
...trimmedState,
};
- case ActionType.EditorChange:
- return {
+ case ActionType.EditorChange: {
+ const edited = {
...trimmedState,
[action.payload.name]: action.payload.json,
};
+ if (
+ action.payload.name === 'masked_encrypted_extra' &&
+ edited.parameters?.embedded_credentials
Review Comment:
A Secure Extra edit retires only `parameters.embedded_credentials`.
`parameters.oauth2_client_info` stays, and `OAuth2ClientField` now prefers it
over the stored blob. If someone types a client secret in the Basic section and
then replaces or deletes that client in Advanced → Secure Extra, `onSave`
copies the stale typed client over the JSON edit, so the new secret is silently
dropped. Should this branch clear `oauth2_client_info` from `parameters` as
well?
##########
UPDATING.md:
##########
@@ -24,6 +24,47 @@ 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 instead of
connecting
+ with the credential in the connection's own SQLAlchemy URI. The viewer sees a
+ `DATABASE_SECURITY_ACCESS_ERROR` saying the chart's connection isn't set up
for
+ embedded dashboards (with `GLOBAL_ASYNC_QUERIES` on, a generic error
instead); the
+ server log names the connection and how to fix it.
+ **Embedded dashboards that stop loading when the flag is turned on:** any
dashboard
+ whose charts use a Snowflake connection with **Impersonate logged in user**
on but no
+ OAuth2 client (none on the connection and no `DATABASE_OAUTH2_CLIENTS` entry
for
+ Snowflake) stops serving embedded viewers, even though impersonation never
changed
+ the login on such a connection and those viewers were served under its base
+ account. Logged-in users are unaffected. Before turning the flag on, update
each
+ such connection: either turn off **Impersonate logged in user**, which keeps
serving
+ everyone under the connection's own credential as before, or configure an
OAuth2
+ client and **Embedded guest credentials**. 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
Review Comment:
This says asset import is unchanged and that deployments leaving the flag
off are unaffected, but dashboard, chart and dataset exports now write
`masked_encrypted_extra_partial: true` whenever the connection has an OAuth2
client or an embedded credential, regardless of the flag. A destination still
on the previous release doesn't know that field on `ImportV1DatabaseSchema` and
rejects the file as an unknown field, so promoting an export from an upgraded
staging instance to production fails. Should the note call out the
bundle-format change and ask admins to upgrade import destinations first?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
