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