Skip to content

Commit 4179dd5

Browse files
refactor(kernel-auth): drop _coerce_bool; treat oauth_token_cache_enabled as Optional[bool]
oauth_token_cache_enabled is a typed connection kwarg like the connector's other booleans (use_cloud_fetch, _use_arrow_native_complex_types, ...), none of which coerce string inputs. Replace the one-off _coerce_bool with `opts.get("oauth_token_cache_enabled") is True`: only a real True enables the kernel's on-disk U2M token cache, and unset (None)/False forward an explicit False so the kernel — whose own default is enabled — stays disabled by default. Any non-bool value fails safe to disabled rather than silently enabling. Matches the nodejs connector's `tokenCacheEnabled ?? false` and removes the inconsistency of coercing this one flag while every sibling boolean is passed raw. Tests: the string-coercion cases are replaced by one asserting non-bool inputs never enable the cache. Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
1 parent aab4203 commit 4179dd5

3 files changed

Lines changed: 15 additions & 50 deletions

File tree

src/databricks/sql/backend/kernel/auth_bridge.py

Lines changed: 8 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -460,16 +460,14 @@ def kernel_auth_kwargs(
460460
else list(PYSQL_OAUTH_REDIRECT_PORT_RANGE)
461461
),
462462
"oauth_scopes": scopes if scopes is not None else list(PYSQL_OAUTH_SCOPES),
463-
# OAuth U2M token-cache enable/disable: when present in auth_options,
464-
# forward to the kernel as token_cache_enabled on the U2M branch.
465-
# Default disabled for backward compatibility when moving token
466-
# persistence control to the kernel. This ensures callers must
467-
# opt-in to on-disk persistence rather than silently enabling it.
468-
# Coerced via _coerce_bool so a string DSN/env value like "False"
469-
# is not treated as truthy (bool("False") is True).
470-
"token_cache_enabled": _coerce_bool(
471-
opts.get("oauth_token_cache_enabled")
472-
),
463+
# OAuth U2M on-disk token cache. A typed Optional[bool], like the
464+
# connector's other boolean options; only a real ``True`` enables
465+
# it. The kernel's own default is *enabled*, so unset (None) must be
466+
# forwarded as an explicit ``False`` — disabled, in-memory only —
467+
# matching the Thrift posture so moving persistence control to the
468+
# kernel never silently starts writing tokens to disk. Opt-in is
469+
# therefore an explicit ``oauth_token_cache_enabled=True``.
470+
"token_cache_enabled": opts.get("oauth_token_cache_enabled") is True,
473471
}
474472
if federation_client_id:
475473
kwargs["identity_federation_client_id"] = federation_client_id
@@ -523,28 +521,6 @@ def _coerce_redirect_port(redirect_port: Any) -> int:
523521
)
524522

525523

526-
def _coerce_bool(value: Any) -> bool:
527-
"""Coerce an opt-in boolean flag (e.g. ``oauth_token_cache_enabled``,
528-
which may arrive as a string from a DSN/env) to a ``bool``.
529-
530-
A plain ``bool(value)`` is wrong for string inputs: ``bool("False")`` is
531-
``True``, which would silently enable on-disk token persistence whenever
532-
the flag arrived as the string ``"False"``. Only genuinely truthy values
533-
enable the flag: real booleans, and the usual textual/numeric truthy
534-
spellings ("true"/"1"/"yes"/"on"). ``None`` (unset) and anything else
535-
disable it (opt-in default)."""
536-
if isinstance(value, bool):
537-
return value
538-
if value is None:
539-
return False
540-
if isinstance(value, str):
541-
return value.strip().lower() in ("true", "1", "yes", "on")
542-
if isinstance(value, (int, float)):
543-
return value != 0
544-
# Unknown types default to disabled rather than truthy-by-accident.
545-
return False
546-
547-
548524
def _normalize_scopes(scopes: Any) -> Optional[list]:
549525
"""Normalise an ``oauth_scopes`` value to a list of strings, or
550526
``None`` to let the kernel apply its defaults.

src/databricks/sql/session.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ def _create_backend(
188188
),
189189
# OAuth U2M token-cache enable/disable: controls whether the kernel
190190
# persists U2M refresh tokens to disk (encrypted, at ~/.config/databricks-sql-kernel/oauth/).
191-
# Coerced via _coerce_bool on the oauth-u2m branch, so omitted/None
191+
# A typed Optional[bool]; on the oauth-u2m branch omitted/None
192192
# ⇒ token_cache_enabled=False (disabled, in-memory only) — the
193193
# opt-in default that preserves backward compat when token
194194
# persistence moves to the kernel path; True ⇒ on-disk persistence.

tests/unit/test_kernel_auth_bridge.py

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -622,12 +622,13 @@ def test_u2m_token_cache_enabled_true_forwarded(self):
622622

623623
@pytest.mark.parametrize(
624624
"raw_value",
625-
["False", "false", "0", "no", "off", "", " ", "nope"],
625+
["True", "true", "1", "yes", "on", "False", "false", "0", "", 1, 0],
626626
)
627-
def test_u2m_token_cache_enabled_falsey_string_stays_false(self, raw_value):
628-
# A string DSN/env value that reads as falsey (e.g. "False") must NOT
629-
# enable on-disk persistence: bool("False") is True, so the flag is
630-
# coerced via _coerce_bool rather than bool().
627+
def test_u2m_token_cache_enabled_non_bool_never_enables(self, raw_value):
628+
# oauth_token_cache_enabled is a typed Optional[bool] (like the
629+
# connector's other boolean options); only a real ``True`` enables
630+
# on-disk persistence. Any non-bool value (e.g. a stray string from a
631+
# DSN) fails safe to disabled rather than silently enabling.
631632
kwargs = kernel_auth_kwargs(
632633
_FakeOAuthProvider(),
633634
{
@@ -637,18 +638,6 @@ def test_u2m_token_cache_enabled_falsey_string_stays_false(self, raw_value):
637638
)
638639
assert kwargs["token_cache_enabled"] is False
639640

640-
@pytest.mark.parametrize("raw_value", ["True", "true", "1", "yes", "on"])
641-
def test_u2m_token_cache_enabled_truthy_string_enables(self, raw_value):
642-
# An explicit truthy string DSN/env value enables persistence.
643-
kwargs = kernel_auth_kwargs(
644-
_FakeOAuthProvider(),
645-
{
646-
"auth_type": "databricks-oauth",
647-
"oauth_token_cache_enabled": raw_value,
648-
},
649-
)
650-
assert kwargs["token_cache_enabled"] is True
651-
652641
@pytest.mark.parametrize("u2m_auth_type", ["databricks-oauth", "azure-oauth"])
653642
def test_u2m_token_cache_enabled_both_auth_types(self, u2m_auth_type):
654643
# token_cache_enabled applies to both databricks-oauth and azure-oauth U2M types.

0 commit comments

Comments
 (0)