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