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: [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]
