From 526285590bb2129bc8ed91e0bbaf029f98f6c831 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Sat, 26 Sep 2026 09:57:56 +0000 Subject: [PATCH 1/2] Raise DB-API errors for transport failures and empty access tokens - ThriftDatabricksClient.make_request re-raised urllib3 HTTP errors of every request except GetOperationStatus unchanged, so an unreachable workspace (refused connection, proxy/tunnel failure) surfaced from connect() and execute() as a raw urllib3 MaxRetryError instead of a DB-API error. Report it through the normal non-retryable path as a RequestError (OperationalError). - connect(access_token="") dropped the empty token and fell back to the interactive browser OAuth flow, blocking on a local callback server. An explicitly empty token now raises 'No valid authentication settings!' at once. Signed-off-by: Amin Ghadersohi --- src/databricks/sql/auth/auth.py | 4 +++ src/databricks/sql/backend/thrift_backend.py | 6 +++- src/databricks/sql/client.py | 2 +- tests/unit/test_auth.py | 7 +++++ tests/unit/test_thrift_backend.py | 32 +++++++++++++++++++- 5 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/databricks/sql/auth/auth.py b/src/databricks/sql/auth/auth.py index a4d4d6f2e..8ce76c5a2 100755 --- a/src/databricks/sql/auth/auth.py +++ b/src/databricks/sql/auth/auth.py @@ -43,6 +43,10 @@ def get_auth_provider(cfg: ClientContext, http_client): cfg.auth_type, ) elif cfg.access_token is not None: + if not cfg.access_token: + # An explicitly empty token is a missing credential; never fall + # back to an interactive browser login for it. + raise RuntimeError("No valid authentication settings! access_token is empty") base_provider = AccessTokenAuthProvider(cfg.access_token) elif cfg.use_cert_as_auth and cfg.tls_client_cert_file: # no op authenticator. authentication is performed using ssl certificate outside of headers diff --git a/src/databricks/sql/backend/thrift_backend.py b/src/databricks/sql/backend/thrift_backend.py index fcf363cf0..a98053760 100644 --- a/src/databricks/sql/backend/thrift_backend.py +++ b/src/databricks/sql/backend/thrift_backend.py @@ -459,7 +459,11 @@ def attempt_request(attempt): f"GetOperationStatus failed with HTTP error and will be retried: {str(err)}" ) else: - raise err + # Not retried here (urllib3 already applied the retry policy), + # but surfaced as a DB-API RequestError rather than a raw + # urllib3 exception. + error = err + error_message = str(err) except OSError as err: error = err error_message = str(err) diff --git a/src/databricks/sql/client.py b/src/databricks/sql/client.py index 3e06aa39b..f5c0c9684 100755 --- a/src/databricks/sql/client.py +++ b/src/databricks/sql/client.py @@ -372,7 +372,7 @@ def read(self) -> Optional[OAuthToken]: http_path, ) - if access_token: + if access_token is not None: access_token_kv = {"access_token": access_token} kwargs = {**kwargs, **access_token_kv} diff --git a/tests/unit/test_auth.py b/tests/unit/test_auth.py index d1b941208..6ab0fadb8 100644 --- a/tests/unit/test_auth.py +++ b/tests/unit/test_auth.py @@ -152,6 +152,13 @@ def test_get_python_sql_connector_auth_provider_access_token(self): auth_provider.add_headers(headers) self.assertEqual(headers["Authorization"], "Bearer dpi123") + def test_get_python_sql_connector_auth_provider_empty_access_token(self): + """An explicitly empty token must not start an interactive OAuth login.""" + with self.assertRaisesRegex(RuntimeError, "No valid authentication settings!"): + get_python_sql_connector_auth_provider( + "moderakh-test.cloud.databricks.com", MagicMock(), access_token="" + ) + def test_get_python_sql_connector_auth_provider_external(self): class MyProvider(CredentialsProvider): def auth_type(self) -> str: diff --git a/tests/unit/test_thrift_backend.py b/tests/unit/test_thrift_backend.py index ddcb378ff..1d674b1d8 100644 --- a/tests/unit/test_thrift_backend.py +++ b/tests/unit/test_thrift_backend.py @@ -1927,7 +1927,7 @@ def test_make_request_will_retry_GetOperationStatus( import thrift, errno from databricks.sql.thrift_api.TCLIService.TCLIService import Client - from databricks.sql.exc import RequestError + from databricks.sql.exc import RequestError, RequestError from databricks.sql.utils import NoRetryReason this_gos_name = "GetOperationStatus" @@ -2047,6 +2047,36 @@ def test_make_request_will_retry_GetOperationStatus_for_http_error( f"{EXPECTED_RETRIES}/{EXPECTED_RETRIES}", cm.exception.context["attempt"] ) + @patch("thrift.transport.THttpClient.THttpClient") + def test_make_request_wraps_urllib3_http_error_as_request_error( + self, t_transport_class + ): + import urllib3 + + t_transport_instance = t_transport_class.return_value + t_transport_instance.code = None + t_transport_instance.headers = {} + mock_method = Mock() + mock_method.__name__ = "OpenSession" + mock_method.side_effect = urllib3.exceptions.MaxRetryError( + None, "/", "Tunnel connection failed: 503" + ) + + thrift_backend = ThriftDatabricksClient( + "foobar", + 443, + "path", + [], + auth_provider=AuthProvider(), + ssl_options=SSLOptions(), + http_client=MagicMock(), + ) + + with self.assertRaises(RequestError) as cm: + thrift_backend.make_request(mock_method, Mock()) + + self.assertIn("Tunnel connection failed", str(cm.exception.message_with_context())) + @patch("thrift.transport.THttpClient.THttpClient") def test_make_request_wont_retry_if_error_code_not_429_or_503( self, t_transport_class From 045b6341ab419416e7541f65c87a555018bc4484 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Sat, 26 Sep 2026 23:34:52 +0000 Subject: [PATCH 2/2] Handle an empty access_token the same way on every auth path - Keep certificate authentication working when an empty token is passed: the empty-token rejection now comes after the cert-auth branch, where the default OAuth login would otherwise start. - Reject an empty token on the Reyden pre-check path too, which skips Thrift and previously upgraded the connection to databricks-oauth. - Remove the duplicate RequestError import in the test. - Add changelog entries. Signed-off-by: Amin Ghadersohi --- CHANGELOG.md | 4 ++++ src/databricks/sql/auth/auth.py | 12 +++++++----- src/databricks/sql/client.py | 10 +++++++++- tests/unit/test_auth.py | 11 +++++++++++ tests/unit/test_session.py | 14 ++++++++++++++ tests/unit/test_thrift_backend.py | 2 +- 6 files changed, 46 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bce9e16e4..931acda0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Release History +# Unreleased +- Fix: transport failures (connection refused, proxy/tunnel errors) on the Thrift backend raise `RequestError` (`OperationalError`) instead of a raw `urllib3` exception. +- Fix: an explicitly empty `access_token` raises `No valid authentication settings! access_token is empty` instead of silently starting the interactive browser OAuth login, including when a known Reyden warehouse skips Thrift. Explicit `auth_type`, `credentials_provider` and certificate authentication still take precedence. + # 4.6.0 (2026-09-24) - Upgrade Databricks SQL Kernel to 1.1.0; the kernel dependency is now stable and no longer experimental. - Transparently auto-recover Thrift connections to Reyden / Real-Time warehouses: when a warehouse rejects the default Thrift protocol (SQLSTATE `KP001`), the session is re-opened on the kernel backend and the warehouse is remembered so later connections skip Thrift. Applies only when no backend was chosen explicitly. diff --git a/src/databricks/sql/auth/auth.py b/src/databricks/sql/auth/auth.py index 8ce76c5a2..819cd7f39 100755 --- a/src/databricks/sql/auth/auth.py +++ b/src/databricks/sql/auth/auth.py @@ -10,6 +10,8 @@ from databricks.sql.auth.common import AuthType, ClientContext from databricks.sql.auth.token_federation import TokenFederationProvider +EMPTY_ACCESS_TOKEN_MESSAGE = "No valid authentication settings! access_token is empty" + def get_auth_provider(cfg: ClientContext, http_client): # Determine the base auth provider @@ -42,15 +44,15 @@ def get_auth_provider(cfg: ClientContext, http_client): http_client, cfg.auth_type, ) - elif cfg.access_token is not None: - if not cfg.access_token: - # An explicitly empty token is a missing credential; never fall - # back to an interactive browser login for it. - raise RuntimeError("No valid authentication settings! access_token is empty") + elif cfg.access_token: base_provider = AccessTokenAuthProvider(cfg.access_token) elif cfg.use_cert_as_auth and cfg.tls_client_cert_file: # no op authenticator. authentication is performed using ssl certificate outside of headers base_provider = AuthProvider() + elif cfg.access_token is not None: + # An explicitly empty token is a missing credential; never fall back + # to the default interactive browser login for it. + raise RuntimeError(EMPTY_ACCESS_TOKEN_MESSAGE) else: if ( cfg.oauth_redirect_port_range is not None diff --git a/src/databricks/sql/client.py b/src/databricks/sql/client.py index f5c0c9684..f5a0612c6 100755 --- a/src/databricks/sql/client.py +++ b/src/databricks/sql/client.py @@ -67,7 +67,10 @@ from databricks.sql.result_set import ResultSet from databricks.sql.types import Row, SSLOptions -from databricks.sql.auth.auth import get_python_sql_connector_auth_provider +from databricks.sql.auth.auth import ( + EMPTY_ACCESS_TOKEN_MESSAGE, + get_python_sql_connector_auth_provider, +) from databricks.sql.experimental.oauth_persistence import OAuthPersistence from databricks.sql.session import Session from databricks.sql.backend.types import CommandId, BackendType, CommandState, SessionId @@ -581,6 +584,11 @@ def kernel_recovery_kwargs() -> dict: or recovery_kwargs.get("credentials_provider") ) if recovery_kwargs.get("auth_type") is None and not has_credential_shape: + if recovery_kwargs.get("access_token") is not None: + # An explicitly empty token: the Thrift path rejects it + # rather than defaulting to the interactive OAuth login + # (see get_auth_provider), so do the same here. + raise RuntimeError(EMPTY_ACCESS_TOKEN_MESSAGE) recovery_kwargs["auth_type"] = AuthType.DATABRICKS_OAUTH.value return recovery_kwargs diff --git a/tests/unit/test_auth.py b/tests/unit/test_auth.py index 6ab0fadb8..d3a9b7e8a 100644 --- a/tests/unit/test_auth.py +++ b/tests/unit/test_auth.py @@ -159,6 +159,17 @@ def test_get_python_sql_connector_auth_provider_empty_access_token(self): "moderakh-test.cloud.databricks.com", MagicMock(), access_token="" ) + def test_get_python_sql_connector_auth_provider_empty_access_token_cert_auth(self): + """An empty token does not override certificate authentication.""" + auth_provider = get_python_sql_connector_auth_provider( + "moderakh-test.cloud.databricks.com", + MagicMock(), + access_token="", + _tls_client_cert_file="fake.cert", + _use_cert_as_auth="abc", + ) + self.assertIsNotNone(auth_provider) + def test_get_python_sql_connector_auth_provider_external(self): class MyProvider(CredentialsProvider): def auth_type(self) -> str: diff --git a/tests/unit/test_session.py b/tests/unit/test_session.py index 69522bc60..1ec10d758 100644 --- a/tests/unit/test_session.py +++ b/tests/unit/test_session.py @@ -954,6 +954,20 @@ def test_oauth_default_recovery_injects_databricks_oauth_auth_type( finally: conn.close() + @patch("%s.session.ThriftDatabricksClient" % PACKAGE) + def test_known_reyden_empty_access_token_is_rejected(self, mock_thrift): + # The Thrift path rejects an explicitly empty token instead of starting + # the interactive OAuth login; skipping Thrift for a known Reyden + # warehouse must not turn it into a databricks-oauth login either. + from databricks.sql.backend import reyden_warehouse_cache + + reyden_warehouse_cache.mark_reyden(self.HOST, "wh-reyden") + with self._fake_kernel() as mock_kernel: + with pytest.raises(RuntimeError, match="access_token is empty"): + self._connect(access_token="") + mock_kernel.assert_not_called() + mock_thrift.return_value.open_session.assert_not_called() + @patch("%s.session.ThriftDatabricksClient" % PACKAGE) def test_pat_recovery_does_not_inject_auth_type(self, mock_thrift): # With a credential shape present (here a PAT) the kernel routes on it diff --git a/tests/unit/test_thrift_backend.py b/tests/unit/test_thrift_backend.py index 1d674b1d8..3c55c8820 100644 --- a/tests/unit/test_thrift_backend.py +++ b/tests/unit/test_thrift_backend.py @@ -1927,7 +1927,7 @@ def test_make_request_will_retry_GetOperationStatus( import thrift, errno from databricks.sql.thrift_api.TCLIService.TCLIService import Client - from databricks.sql.exc import RequestError, RequestError + from databricks.sql.exc import RequestError from databricks.sql.utils import NoRetryReason this_gos_name = "GetOperationStatus"