hughhhh opened a new pull request, #45870:
URL: https://github.com/apache/superset/pull/45870
### SUMMARY
Importing a connection file onto an existing per-user OAuth2 Snowflake
connection could quietly break every embedded dashboard on it. If the file's
embedded guest credential had the username, the Key pair method and the
passphrase but no private key, the overwrite import answered "OK" and asked for
nothing. The stored private key was gone afterwards, and every embedded viewer
got a generic "An error occurred while fetching the data."
The connection dialog and the database API already refuse that half-filled
credential ("Embedded guest credentials are invalid or incomplete…"). Import
never ran that check. It restores secrets that arrive masked, but one the file
leaves out entirely is neither restored nor asked for.
This PR runs the same completeness check on import when `EMBEDDED_SUPERSET`
and `EMBEDDED_CREDENTIAL_FALLBACK` are both on, as the API does. A half-filled
credential is refused with a message that names what's missing, and the stored
credential is left alone:
> Embedded guest credentials of database 'X' are invalid or incomplete.
Missing: $.embedded_credentials.privatekey_body (Embedded Fallback Private
Key). Provide all required fields or clear them to leave embedded viewers
unconfigured.
- The check runs on the credential after masked values are restored from the
stored one, so a complete export with masked secrets imports as before.
- A new connection imported with a half-filled credential is refused the
same way.
- With the fallback off, nothing changes. The import schema stays loose, so
existing exports still load.
- `/api/v1/assets/import/` used to turn every error into "Import failed for
an unknown reason". It now passes an `ImportFailedError` through unchanged, as
`/api/v1/database/import/` already does, so this refusal and the existing "Must
provide value…" refusals reach the caller.
- A new connection's engine spec is now looked up with
`Database.get_db_engine_spec`, which handles a missing driver instead of
crashing.
The other option was to keep the stored secret when the file omits it. I
didn't take it: it would keep a value the file deliberately left out, and it
still wouldn't stop a half-filled credential from being saved, which the dialog
and API already enforce.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend only).
### TESTING INSTRUCTIONS
1. Turn on `EMBEDDED_SUPERSET` and `EMBEDDED_CREDENTIAL_FALLBACK`. Create a
Snowflake connection with an OAuth2 client, "Impersonate logged in user" on,
and Key pair embedded guest credentials (username, private key, passphrase).
2. Export it. In the connection YAML, remove `privatekey_body` from
`embedded_credentials` in `masked_encrypted_extra`.
3. Import it with overwrite. Before: `200 OK` and the private key is gone.
After: the import is refused with a message naming
`$.embedded_credentials.privatekey_body`, and the Edit dialog still shows the
stored key.
4. Import the unedited export with overwrite: it succeeds and the key is
kept.
Unit tests:
```
pytest tests/unit_tests/databases/commands/importers/v1/import_test.py \
tests/unit_tests/importexport/api_test.py \
tests/unit_tests/databases/schema_tests.py
```
These cover an overwrite with the key missing (refused, key kept), a new
connection missing its password (refused), a complete credential (imports, key
restored), and the flag off (imports as before). The API cases send a real
bundle through `/api/v1/assets/import/`.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [x] Required feature flags: `EMBEDDED_SUPERSET`,
`EMBEDDED_CREDENTIAL_FALLBACK`
- [ ] 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
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
Stacked on #45068, which it fixes: the diff against `master` includes that
PR's commits. Review only the top commit, `dca0eb342a` ("fix(embedded): refuse
a half-filled embedded credential on import"). This should merge after #45068,
or be folded into it.
Created with Claude Opus 5.5 in Claude Code.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
