You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/09/26 12:12:50 UTC
[PR] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [superset]
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: 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] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44705:
URL: https://github.com/apache/superset/pull/44705#issuecomment-5846201593
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44705?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 `4.34783%` with `22 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.13%. Comparing base ([`0501796`](https://app.codecov.io/gh/apache/superset/commit/05017968378f81e44071f44aa16796ec5d2e0b9a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3cc446f`](https://app.codecov.io/gh/apache/superset/commit/3cc446f3ec350a4af60dce80c1bba06a19db2f9b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44705?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/db\_engine\_specs/databricks.py](https://app.codecov.io/gh/apache/superset/pull/44705?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fdatabricks.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2RhdGFicmlja3MucHk=) | 4.34% | [22 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44705?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 (0501796) and HEAD (3cc446f). Click for more details.
>
> <details><summary>HEAD has 28 uploads less than BASE</summary>
>
>| Flag | BASE (0501796) | HEAD (3cc446f) |
>|------|------|------|
>|hive|4|1|
>|python|24|2|
>|presto|4|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44705 +/- ##
===========================================
- Coverage 81.12% 57.13% -24.00%
===========================================
Files 2956 2956
Lines 178436 178458 +22
Branches 41341 41346 +5
===========================================
- Hits 144762 101962 -42800
- Misses 30969 75513 +44544
+ Partials 2705 983 -1722
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44705/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/44705/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.77% <4.34%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44705/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/44705/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/44705/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.67% <4.34%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44705/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.72% <4.34%> (-46.69%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44705/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44705/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/44705?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] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #44705:
URL: https://github.com/apache/superset/pull/44705
--
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] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44705:
URL: https://github.com/apache/superset/pull/44705#issuecomment-5846173904
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44705:
URL: https://github.com/apache/superset/pull/44705#issuecomment-5852291858
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44705:
URL: https://github.com/apache/superset/pull/44705#issuecomment-5846202260
<!-- Bito Reply -->
The flagged issue is correct. When `impersonate_user` is enabled, the current implementation filters out shared credentials from `connect_args` but does not explicitly ensure that the user's `access_token` is preserved if it was only provided via `encrypted_extra` (and thus not yet in `params['connect_args']`).
To resolve this, you should ensure the user's token is explicitly merged into the `connect_args` before the filtering logic runs, or ensure the filter logic accounts for the token's source. Given the existing logic in `update_params_from_encrypted_extra`, you can ensure the `access_token` is present in the `connect_args` dictionary before the filtering step.
Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well?
**superset/db_engine_specs/databricks.py**
```
if database.impersonate_user:
# Ensure the user's token is available for the filter
if "access_token" not in connect_args and "access_token" in secure_connect_args:
connect_args["access_token"] = secure_connect_args.pop("access_token")
connect_args = {
key: value
for key, value in connect_args.items()
if key == "access_token"
or key not in DATABRICKS_SHARED_CREDENTIAL_CONNECT_ARGS
}
```
--
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] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44705:
URL: https://github.com/apache/superset/pull/44705#discussion_r4140766803
##########
superset/db_engine_specs/databricks.py:
##########
@@ -335,7 +352,21 @@ def needs_oauth2(cls, ex: Exception) -> bool:
if isinstance(ex, cls.oauth2_exception):
return True
message = str(ex).lower()
- return any(signal in message for signal in cls.oauth2_auth_failure_signals)
+ if any(signal in message for signal in cls.oauth2_auth_failure_signals):
+ return True
+ # A rejected bearer token fails the request with HTTP 401 and a message
+ # such as "Credential was not sent or was of an unsupported type for this
+ # API", which no signal above matches; the connector reports the status
+ # in ``RequestError.context["http-code"]``. 403 is left out on purpose: it
+ # also means a missing permission, which re-authorizing cannot fix.
+ error = getattr(ex, "orig", None) or ex
+ context = getattr(error, "context", None)
+ if isinstance(context, dict):
+ try:
+ return int(context.get("http-code") or 0) == 401
Review Comment:
You're reading it right: for Databricks the user's OAuth2 token only reaches the connection through `impersonate_user`, so without impersonation a 401 comes from the shared credential and re-authorizing can't fix it.
`needs_oauth2` only receives the exception, so I gated the step that sends the user to the prompt instead. In c5e4c5e, `DatabricksDynamicBaseEngineSpec.start_oauth2_dance` returns without redirecting when `database.impersonate_user` is off. Every caller (`_handle_oauth2_error`, `check_for_oauth2`, `execute`, test connection, the forced-refresh retry) then raises the original error, so the admin sees the actual failure. This applies to the older message signals (`unauthorized`, `http 401`, ...) as well as the new `http-code` check. `test_start_oauth2_dance_requires_impersonation` covers both specs.
One remaining edge: `ValidateDatabaseParametersCommand` treats `needs_oauth2` as "valid, authorize later" without starting the dance. That check is engine-agnostic and predates this PR, so I left it alone here.
##########
superset/db_engine_specs/databricks.py:
##########
@@ -437,6 +468,13 @@ def update_params_from_encrypted_extra(
consumed by ``Database.get_oauth2_config``; it is not a Databricks driver
connection argument, so it must be stripped here to avoid poisoning the
connection when OAuth2 is configured on the database itself.
+
+ ``connect_args`` are merged key by key, so credentials kept in the secure
+ extra (an access token, an OAuth M2M client secret) do not discard the
+ ``connect_args`` from ``extra`` or Superset's own (e.g. the User-Agent).
+ With user impersonation enabled, shared credentials are dropped: the
+ user's OAuth2 token set by ``impersonate_user`` is the only credential,
+ instead of being silently replaced by a shared one.
"""
if not database.encrypted_extra:
Review Comment:
Agreed. Done in c5e4c5e. `impersonate_user` now drops every key in `DATABRICKS_SHARED_CREDENTIAL_CONNECT_ARGS` from the `extra` connect_args. It then writes the user's token as `access_token` only when `extra` had one, which keeps the Python connector path. So the filter runs whenever impersonation is on, whether or not there is a secure extra. `update_params_from_encrypted_extra` now filters only the secure side, and the `key == "access_token"` special case is gone from there.
New test `test_impersonate_user_drops_shared_credentials_without_secure_extra` covers impersonation on with no `encrypted_extra` and `auth_type`/`oauth_client_*` in `extra`. It fails on the previous head. `test_update_params_impersonation_keeps_only_the_user_token` now runs `impersonate_user` then `update_params_from_encrypted_extra`, in the same order as `_get_sqla_engine`.
--
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] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44705:
URL: https://github.com/apache/superset/pull/44705#issuecomment-5903947383
Following up on the bito review (run #ba9e90) and the bito reply that agreed with codeant:
- **"Misleading credential set contract"**: fixed in c5e4c5e. The `access_token` exception in the filter is gone. `impersonate_user` drops every key in `DATABRICKS_SHARED_CREDENTIAL_CONNECT_ARGS` and then writes the user's token back explicitly. The comment on the constant now describes that contract.
- **"Missing mock type annotation"**: the `database` mocks in the tests this PR adds are annotated as `MagicMock` in c5e4c5e.
- **codeant/bito on the shared token only in the secure extra**: answered in the thread. The user's token reaches the connector through the URL password that `impersonate_user` sets. `test_impersonation_with_shared_token_only_in_secure_extra` (5e30ad1) covers it.
--
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] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44705:
URL: https://github.com/apache/superset/pull/44705#discussion_r4113535019
##########
superset/db_engine_specs/databricks.py:
##########
@@ -446,7 +484,31 @@ def update_params_from_encrypted_extra(
logger.error(ex, exc_info=True)
raise
encrypted_extra.pop("oauth2_client_info", None)
+ secure_connect_args = encrypted_extra.pop("connect_args", None)
params.update(encrypted_extra)
+ if secure_connect_args is not None and not isinstance(
+ secure_connect_args, dict
+ ):
+ params["connect_args"] = secure_connect_args
+ return
+ if secure_connect_args is None and "connect_args" not in params:
+ return
+
+ secure_connect_args = secure_connect_args or {}
+ connect_args = dict(params.get("connect_args") or {})
+ if database.impersonate_user:
+ connect_args = {
+ key: value
+ for key, value in connect_args.items()
+ if key == "access_token"
+ or key not in DATABRICKS_SHARED_CREDENTIAL_CONNECT_ARGS
+ }
+ secure_connect_args = {
+ key: value
+ for key, value in secure_connect_args.items()
+ if key not in DATABRICKS_SHARED_CREDENTIAL_CONNECT_ARGS
Review Comment:
The user's credential is still there. `impersonate_user` runs before `update_params_from_encrypted_extra` in `Database._get_sqla_engine` and sets the URL password to the user's token, and the Databricks dialect uses `url.password` as `access_token` (`connect_args` only override it). Dropping the shared token is intended, so it can't replace the user's. Added `test_impersonation_with_shared_token_only_in_secure_extra` in 5e30ad158b, which runs both calls in that order.
--
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] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [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 #44705:
URL: https://github.com/apache/superset/pull/44705#discussion_r4111340817
##########
superset/db_engine_specs/databricks.py:
##########
@@ -446,7 +484,31 @@ def update_params_from_encrypted_extra(
logger.error(ex, exc_info=True)
raise
encrypted_extra.pop("oauth2_client_info", None)
+ secure_connect_args = encrypted_extra.pop("connect_args", None)
params.update(encrypted_extra)
+ if secure_connect_args is not None and not isinstance(
+ secure_connect_args, dict
+ ):
+ params["connect_args"] = secure_connect_args
+ return
+ if secure_connect_args is None and "connect_args" not in params:
+ return
+
+ secure_connect_args = secure_connect_args or {}
+ connect_args = dict(params.get("connect_args") or {})
+ if database.impersonate_user:
+ connect_args = {
+ key: value
+ for key, value in connect_args.items()
+ if key == "access_token"
+ or key not in DATABRICKS_SHARED_CREDENTIAL_CONNECT_ARGS
+ }
+ secure_connect_args = {
+ key: value
+ for key, value in secure_connect_args.items()
+ if key not in DATABRICKS_SHARED_CREDENTIAL_CONNECT_ARGS
Review Comment:
**Suggestion:** When the shared token exists only in encrypted extra, `impersonate_user` never creates an `access_token` entry. This filter then removes it, leaving the Python connector without the user's credential.
**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=020d42938bc241e9b8d7f18c4b044346&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=020d42938bc241e9b8d7f18c4b044346&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/databricks.py
**Line:** 506:509
**Comment:**
*Security: When the shared token exists only in encrypted extra, `impersonate_user` never creates an `access_token` entry. This filter then removes it, leaving the Python connector without the user's credential.
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%2F44705&comment_hash=a35a01ccf77cca558fcb6fb3da6b0ba9b52b681f5864a96287d95e05fbd52598&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44705&comment_hash=a35a01ccf77cca558fcb6fb3da6b0ba9b52b681f5864a96287d95e05fbd52598&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] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44705:
URL: https://github.com/apache/superset/pull/44705#issuecomment-5906706879
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #44705:
URL: https://github.com/apache/superset/pull/44705#discussion_r4141765808
##########
superset/db_engine_specs/databricks.py:
##########
@@ -447,14 +480,40 @@ def impersonate_user(
# back to an empty string to force re-authentication when none is set.
url = url.set(password=user_token or "")
+ # Drop shared credentials from ``extra`` (a PAT, OAuth M2M client, Azure
+ # service principal) so none of them reaches the connector next to the
+ # user's token. The secure extra is filtered the same way in
+ # ``update_params_from_encrypted_extra``.
+ connect_args = engine_kwargs.get("connect_args") or {}
+ filtered_connect_args = {
+ key: value
+ for key, value in connect_args.items()
+ if key not in DATABRICKS_SHARED_CREDENTIAL_CONNECT_ARGS
+ }
# The Python connector passes the token via ``connect_args`` instead of the
# URL password, so keep it in sync (clearing it likewise forces re-auth).
- connect_args = engine_kwargs.setdefault("connect_args", {})
if "access_token" in connect_args:
- connect_args["access_token"] = user_token or ""
+ filtered_connect_args["access_token"] = user_token or ""
+ engine_kwargs["connect_args"] = filtered_connect_args
return url, engine_kwargs
+ @classmethod
+ def start_oauth2_dance(cls, database: Database) -> None:
+ """
+ Start the OAuth2 dance only when the database impersonates the user.
+
+ The user's OAuth2 token only reaches the connection through
+ ``impersonate_user``. Without impersonation the connection uses the
+ shared credential, so an authorization prompt cannot fix an auth failure
+ (e.g. a revoked shared token returning HTTP 401). Return instead, so the
+ caller raises the original error for an admin to act on.
+ """
+ if not database.impersonate_user:
Review Comment:
Same family as the `ValidateDatabaseParametersCommand` edge: `SyncPermissionsCommand.validate` never reaches this gate. It turns `needs_oauth2` straight into `MissingOAuth2TokenError`, and `UpdateDatabaseCommand` swallows that one (`update.py:129`). So saving a non-impersonating Databricks DB whose shared token is revoked succeeds quietly and skips the permission sync, instead of failing with "Connection failed".
Happy for that to go in the same follow-up as the validate path. Worth a line in the PR description so it doesn't get lost?
--
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] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #44705:
URL: https://github.com/apache/superset/pull/44705#discussion_r4137497088
##########
superset/db_engine_specs/databricks.py:
##########
@@ -437,6 +468,13 @@ def update_params_from_encrypted_extra(
consumed by ``Database.get_oauth2_config``; it is not a Databricks driver
connection argument, so it must be stripped here to avoid poisoning the
connection when OAuth2 is configured on the database itself.
+
+ ``connect_args`` are merged key by key, so credentials kept in the secure
+ extra (an access token, an OAuth M2M client secret) do not discard the
+ ``connect_args`` from ``extra`` or Superset's own (e.g. the User-Agent).
+ With user impersonation enabled, shared credentials are dropped: the
+ user's OAuth2 token set by ``impersonate_user`` is the only credential,
+ instead of being silently replaced by a shared one.
"""
if not database.encrypted_extra:
Review Comment:
This early return skips the impersonation filter whenever `encrypted_extra` is empty (OAuth2 from the global `DATABASE_OAUTH2_CLIENTS`, say), so an `auth_type` or `oauth_client_*` in `extra` still reaches the connector next to the user's token.
What about stripping the `extra` side in `impersonate_user` instead? It always runs when impersonating, drops the `key == "access_token"` special case, and a test with impersonation on and no `encrypted_extra` would pin it.
##########
superset/db_engine_specs/databricks.py:
##########
@@ -335,7 +352,21 @@ def needs_oauth2(cls, ex: Exception) -> bool:
if isinstance(ex, cls.oauth2_exception):
return True
message = str(ex).lower()
- return any(signal in message for signal in cls.oauth2_auth_failure_signals)
+ if any(signal in message for signal in cls.oauth2_auth_failure_signals):
+ return True
+ # A rejected bearer token fails the request with HTTP 401 and a message
+ # such as "Credential was not sent or was of an unsupported type for this
+ # API", which no signal above matches; the connector reports the status
+ # in ``RequestError.context["http-code"]``. 403 is left out on purpose: it
+ # also means a missing permission, which re-authorizing cannot fix.
+ error = getattr(ex, "orig", None) or ex
+ context = getattr(error, "context", None)
+ if isinstance(context, dict):
+ try:
+ return int(context.get("http-code") or 0) == 401
Review Comment:
`_handle_oauth2_error` and `check_for_oauth2` start the dance whenever OAuth2 is enabled, without looking at `impersonate_user`. So on a non-impersonating database with a Databricks entry in `DATABASE_OAUTH2_CLIENTS`, a revoked shared token would send every user through an authorize prompt that can't fix it, and hide the error an admin needs to see.
Might be misreading which failures come back as 401 there, but is it worth only treating a 401 as OAuth2 when the database impersonates?
--
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] fix(databricks): keep the user's OAuth2 token and extra connect_args; re-auth on HTTP 401 [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44705:
URL: https://github.com/apache/superset/pull/44705#discussion_r4141831064
##########
superset/db_engine_specs/databricks.py:
##########
@@ -447,14 +480,40 @@ def impersonate_user(
# back to an empty string to force re-authentication when none is set.
url = url.set(password=user_token or "")
+ # Drop shared credentials from ``extra`` (a PAT, OAuth M2M client, Azure
+ # service principal) so none of them reaches the connector next to the
+ # user's token. The secure extra is filtered the same way in
+ # ``update_params_from_encrypted_extra``.
+ connect_args = engine_kwargs.get("connect_args") or {}
+ filtered_connect_args = {
+ key: value
+ for key, value in connect_args.items()
+ if key not in DATABRICKS_SHARED_CREDENTIAL_CONNECT_ARGS
+ }
# The Python connector passes the token via ``connect_args`` instead of the
# URL password, so keep it in sync (clearing it likewise forces re-auth).
- connect_args = engine_kwargs.setdefault("connect_args", {})
if "access_token" in connect_args:
- connect_args["access_token"] = user_token or ""
+ filtered_connect_args["access_token"] = user_token or ""
+ engine_kwargs["connect_args"] = filtered_connect_args
return url, engine_kwargs
+ @classmethod
+ def start_oauth2_dance(cls, database: Database) -> None:
+ """
+ Start the OAuth2 dance only when the database impersonates the user.
+
+ The user's OAuth2 token only reaches the connection through
+ ``impersonate_user``. Without impersonation the connection uses the
+ shared credential, so an authorization prompt cannot fix an auth failure
+ (e.g. a revoked shared token returning HTTP 401). Return instead, so the
+ caller raises the original error for an admin to act on.
+ """
+ if not database.impersonate_user:
Review Comment:
Confirmed: `UpdateDatabaseCommand` catches `MissingOAuth2TokenError` at `update.py:128` and skips the sync. I added a "Known limitations (follow-up)" section to the PR description covering this path and the `ValidateDatabaseParametersCommand` one, so both are tracked for the same engine-agnostic follow-up.
--
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