aminghadersohi opened a new pull request, #44705:
URL: https://github.com/apache/superset/pull/44705
### SUMMARY
Two fixes to the Databricks engine specs' OAuth2 / credential handling:
1. **`update_params_from_encrypted_extra` merges `connect_args` instead of
replacing them.** Credentials such as an access token or an OAuth M2M client
secret belong in the secure extra's `connect_args`. With the old code, that
block replaced the whole `connect_args` built from `extra`, which dropped
entries like `session_configuration` and Superset's
`_user_agent_entry`/`http_headers`. **With user impersonation enabled**, a
credential kept there also replaced the user's OAuth2 token that
`impersonate_user` had just set, so queries silently ran as the shared
principal instead of the user. Now `connect_args` are merged key by key, and
shared credentials (`access_token`, `oauth_client_*`, `credentials_provider`,
`auth_type`, `azure_*`) are dropped from both extras when the database
impersonates users.
2. **`needs_oauth2` recognises an HTTP 401 from the connector.** Databricks
rejects an invalid or revoked OAuth bearer token with HTTP 401 and the message
`Credential was not sent or was of an unsupported type for this API`. That
message matches none of `oauth2_auth_failure_signals`, so the user got the raw
error and no new authorization started. The connector reports the status in
`RequestError.context["http-code"]`, and this change uses that value (also when
SQLAlchemy wraps the error). 403 is deliberately left out: it also means a
missing permission, which re-authorizing cannot fix.
### TESTING INSTRUCTIONS
- New unit tests in `tests/unit_tests/db_engine_specs/test_databricks.py`:
- `test_needs_oauth2_detects_http_401_from_the_connector`: 401 as int/str,
raw and wrapped, and 403/None negatives, for both specs.
- `test_update_params_merges_connect_args`.
- `test_update_params_impersonation_keeps_only_the_user_token`.
- The positive cases fail on `master` and pass with this change. The rest of
the file is unchanged.
- The behaviour was also checked live against a Databricks SQL warehouse:
- Before: a database with extra
`session_configuration.TIMEZONE=Asia/Kolkata` and secure-extra `connect_args`
ran in `Etc/UTC`. With impersonation plus a stored per-user token,
`current_user()` was the owner of the shared token.
- After: the session time zone is kept and the query runs as the user's
token principal.
- A stored invalid bearer token now restarts the OAuth2 dance.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] 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]
