aminghadersohi opened a new pull request, #46076: URL: https://github.com/apache/superset/pull/46076
### SUMMARY Editing a Snowflake connection that signs in only through OAuth2 (no username or password in the URI) and changing its warehouse, database or role was refused with: > This update would change the connection's effective destination (host/port, engine parameters, SSH tunnel endpoint, or OAuth2 endpoint URIs) while reusing the stored credential. Provide the real password (or SSH tunnel credential / OAuth2 client secret) to confirm a connection move. This happened even after the admin re-entered the OAuth2 client secret, as the message asks. **Root cause.** `UpdateDatabaseCommand._check_no_unsafe_secret_rebind` (added in https://github.com/apache/superset/pull/44003) treats any URI change other than the password as a destination change, and a warehouse, database or role change is a URI query or path change. With no URI password, the only credential it checks is the OAuth2 client secret in `encrypted_extra`. Before https://github.com/apache/superset/pull/45866 it judged "reused" by comparing whole blobs: `unmask_encrypted_extra(stored, submitted) == stored`. If the admin re-enters the same client secret, unmasking rebuilds the stored blob byte for byte, because an earlier save had stored it in `json.dumps` form. So a freshly entered secret read as reuse and the save was refused. https://github.com/apache/superset/pull/45866 judges each stored secret separately: only a secret left masked, or left out, counts as reused. That already fixes this on master. **This PR** adds no production code. It adds an API-level regression test for the exact reported shape, which the existing tests don't cover: those mock the update command, while this one goes through the real `PUT /api/v1/database/<id>` with the real `SnowflakeEngineSpec`, an OAuth2-only connection (no stored password), and warehouse, database and role edits: - `test_update_oauth2_only_snowflake_connection_settings`, 9 cases (3 URI edits × 3 secrets): - the stored secret re-entered: saves (200), URI updated - a new secret: saves (200), new secret stored - the secret left masked: refused (422), row and stored secret unchanged - `test_update_oauth2_only_snowflake_connection_keeps_masked_secret`: a save that keeps the connection settings (rename only) round-trips the masked secret from `GET /connection` and keeps the stored secret. **Why the security check stays sound.** Nothing in the guard changes. A destination change with the client secret left masked is still refused, and the test asserts that for every edit. A secret is attached to the new destination only when the caller typed it. A caller who already knows the secret gains nothing from re-entering it. Masking is untouched: `GET /connection` still returns the mask, and the mask still restores the stored value only when the destination is unchanged. **Sibling paths checked:** - **Test-connection and validate-parameters guards** (`test_connection.py`, `validate.py`): only decide whether to unmask; neither refuses on a blob comparison. Not affected. - **`oauth2_endpoint_rebind_unsafe`**: compares the secret with the mask directly. Not affected. - **Other OAuth2 engines** (Databricks, Trino, GSheets): the per-secret check is shared code, so they get the same fix. Databricks keeps its own `get_connection_identity`, so a different `http_path` still counts as a move (existing test). - **Database import** (`_refuse_stored_secret_reuse`): refuses a URI change on a connection with no stored password and asks for a password that doesn't exist. Not changed here. A bundle that omits `masked_encrypted_extra` keeps the stored `encrypted_extra` attached, so that refusal is what stops a key-pair key or service-account secret following the move. Relaxing it safely needs the same per-secret check on import; a separate change. ### TESTING INSTRUCTIONS ``` pytest tests/unit_tests/databases/api_test.py -k oauth2_only_snowflake ``` The three "stored secret re-entered" cases fail with the 422 above when the `update.py` part of https://github.com/apache/superset/pull/45866 is reverted (`git show de9ece0b2a -- superset/commands/database/update.py | git apply -R`), and pass on master. `tests/unit_tests/databases` and `tests/unit_tests/commands/databases`: 751 passed, 1 skipped. Manual: create a Snowflake connection with an OAuth2 client and no username or password, save it once, then edit it. Change the warehouse and re-enter the client secret: it saves. Change it again but leave the secret masked: refused with the message above. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] 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 -- 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]
