fix(kernel): forward full OAuth U2M app bundle into kernel (PECOBLR-4040) - #914
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the kernel-auth bridge so OAuth U2M connections (use_kernel=True) explicitly forward the connector’s full coupled OAuth app configuration into the kernel, ensuring the kernel path authenticates as the Python connector’s OAuth app (parity with the Thrift path) rather than inheriting the kernel’s own default app.
Changes:
- Forward full OAuth U2M bundle to the kernel (
client_id,oauth_scopes,redirect_port) with connector defaults when caller values are omitted. - Add/expand unit tests covering bare U2M defaults, full overrides, client-id-only fallback behavior, redirect port int coercion, and custom scopes.
- Document the behavior change in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/unit/test_kernel_auth_bridge.py | Adds coverage for default/override routing of full OAuth U2M bundle into the kernel. |
| src/databricks/sql/backend/kernel/auth_bridge.py | Implements forwarding of the full OAuth U2M app bundle with connector defaults for missing fields. |
| CHANGELOG.md | Notes the kernel OAuth U2M bundle-forwarding behavior change under Unreleased. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Solid, well-tested change that fixes the wrong-identity U2M bug. One medium concern: the branch honors oauth_redirect_port even without oauth_client_id, unlike the Thrift path it claims to mirror — which can pair a custom port with the default app and hit an unregistered redirect URI. A related low: caller oauth_scopes are honored here but dropped on Thrift, so the "exact parity" framing is slightly off.
Addresses: - #3799256419 at src/databricks/sql/backend/kernel/auth_bridge.py:266 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Looks solid overall — the client_id/redirect_port coupling correctly mirrors the Thrift path and is well-tested. One medium concern: the U2M branch honors caller-supplied oauth_scopes, which the Thrift path (get_python_sql_connector_auth_provider) does not, so the "exact parity" claim is inaccurate for scopes.
Addresses: - #3799336213 at src/databricks/sql/backend/kernel/auth_bridge.py:275 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
| kwargs = {"auth_type": "oauth-u2m"} | ||
| if client_id: | ||
| kwargs["client_id"] = client_id | ||
| # Only databricks-oauth reaches here (azure-oauth was rejected up |
There was a problem hiding this comment.
Comment too verbose, make it concise
There was a problem hiding this comment.
Shortened the comment as requested. The 32-line block on auth_bridge.py:254 is now an 8-line summary that keeps the essentials (databricks-oauth-only, forwarding the connector's own bundle for Thrift parity, client_id/redirect_port coupling, non-overridable scopes) and drops the redundant prose. No code behavior changed.
Pushed bfd3629 (bundled with 1 other thread(s)).
| if client_id and redirect_port is not None | ||
| else PYSQL_OAUTH_REDIRECT_PORT_RANGE[0] | ||
| ), | ||
| "oauth_scopes": list(PYSQL_OAUTH_SCOPES), |
There was a problem hiding this comment.
Do not hardcode, we should be able to take in customized scopes
There was a problem hiding this comment.
Made the change the reviewer asked for: the kernel U2M path now accepts customized oauth_scopes rather than hardcoding them, with PYSQL_OAUTH_SCOPES retained only as the default. Tests updated and passing.
Pushed bfd3629 (bundled with 1 other thread(s)).
| kwargs = { | ||
| "auth_type": "oauth-u2m", | ||
| "client_id": client_id or PYSQL_OAUTH_CLIENT_ID, | ||
| "redirect_port": ( |
There was a problem hiding this comment.
redirect_port should just get passed in, we either use the pass in one or default one, no need to judge based on client_id existense.
There was a problem hiding this comment.
The current coupling (forward caller redirect_port only when an explicit client_id is also supplied) deliberately mirrors the Thrift path in src/databricks/sql/auth/auth.py:127-130, which overrides oauth_redirect_port_range only when both oauth_client_id and oauth_redirect_port are set. The rationale is correctness: the default databricks-sql-python app only registers its default port range as redirect URIs, so forwarding an arbitrary caller port without a matching custom client_id yields an unregistered redirect URI and fails the U2M flow. Rather than silently regress this parity/correctness behavior on a one-line request, I replied asking whether the kernel is intended to decouple port from app registration (differing from Thrift). If confirmed, I'll drop the client_id and guard and update test_u2m_redirect_port_ignored_without_client_id accordingly. No code change made yet.
Addresses: - #3799622645 at src/databricks/sql/backend/kernel/auth_bridge.py:254 - #3799650240 at src/databricks/sql/backend/kernel/auth_bridge.py:300 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-tested change — the azure-oauth up-front rejection, ambiguity guards, client_id/redirect_port coupling, and int coercion all line up with the Thrift reference and have matching unit coverage. One medium concern: the scopes handling honors a caller override while the Thrift path hardcodes PYSQL_OAUTH_SCOPES, which breaks the stated parity and contradicts this file's own docstring and the CHANGELOG ("fixed" scopes).
Addresses: - #3799717722 at src/databricks/sql/backend/kernel/auth_bridge.py:275 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the U2M bundle-forwarding logic faithfully mirrors the Thrift get_python_sql_connector_auth_provider coupling (client_id-or-default, and port override only when an explicit client_id is also present), the azure-oauth rejection is correctly hoisted before all routing, and test coverage is thorough (bare bundle, full override, client_id-only fallback, int coercion, port-without-client_id, scope normalization). One low-severity note about single-port forwarding reducing retry resilience versus the Thrift range; likely an inherent kernel constraint rather than a code defect.
Addresses: - #3799762589 at src/databricks/sql/backend/kernel/auth_bridge.py:261 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the U2M bundle forwarding correctly mirrors the Thrift path's client_id/redirect_port coupling, the azure-oauth rejection is intentional and well-tested, and the scopes deviation is documented. One low-severity note about exception-type consistency on the port coercion.
Addresses: - #3799812091 at src/databricks/sql/backend/kernel/auth_bridge.py:258 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
databricks-sql-kernel #257 landed: the pyo3 Session now takes redirect_ports (a list) and no longer accepts the single redirect_port kwarg. Update the kernel auth bridge to emit redirect_ports for databricks-oauth U2M, forwarding the databricks-sql-python app's FULL registered port list (PYSQL_OAUTH_REDIRECT_PORT_RANGE, 8020-8024) so the kernel binds the first free port — busy-port fallback, matching the Thrift DatabricksOAuthProvider. A custom client_id + explicit port pins that single port ([port]). Bump KERNEL_REV to the merged #257 commit (45a0d6a) so kernel-e2e builds against the kernel that exposes redirect_ports. Tests updated. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Solid, well-tested change. The U2M branch faithfully mirrors the Thrift path's client_id/redirect-port coupling (auth.py:118-124) and the azure-oauth up-front rejection is a correctness improvement (fail loud vs. authenticating against wrong endpoints). One low-severity note about the kernel-side key contract change being unverified at the integration boundary; otherwise looks good.
What
On the
use_kernel=Truepath, OAuth U2M now forwards the connector's full coupled OAuth-app bundle —client_id+oauth_scopes+redirect_port— into the kernel'soauth-u2m, instead of sending a bare{"auth_type": "oauth-u2m"}and relying on the kernel default.Why
PECOBLR-4039 (Done) changed the kernel core default U2M app to
databricks-sql-connector/sql offline_access/ port8030. The Python connector is an override of that default, so a bare U2M connection via the kernel was authenticating as the wrong identity (databricks-sql-connector) instead ofdatabricks-sql-python.Because
client_id+ scopes +redirect_portare coupled per OAuth app (each app registers its own redirect URI), the connector must forward all three together rather than a partial bundle.How
In
auth_bridge.py's U2M branch, each field falls back to the connector's registered default when the caller doesn't override it, sourced from the existingPYSQL_OAUTH_*constants (no hardcoded port):databricks-oauth→databricks-sql-python/["sql", "offline_access"]/ first of8020-8024azure-oauth→ the azure client GUID /["sql", "offline_access"]/8030Only the redirect port is routable into the kernel (it derives
http://localhost:{port}); the kernel takes a single port, so the first registered port of the range is forwarded. Explicit calleroauth_client_id/oauth_scopes/oauth_redirect_portare honored verbatim, and theidentity_federation_client_idforwarding already onmainis preserved.This mirrors
get_python_sql_connector_auth_providerexactly, so the kernel path is at parity with the Thrift path — even a customclient_idwith no scopes/port fills insql offline_access+ the default port.Acceptance criteria
use_kernelauthenticates asdatabricks-sql-pythonwithsql offline_access(parity with Thrift)client_id+ scopes + redirect port honored end-to-endTesting
TDD (watched tests fail, then pass).
tests/unit/test_kernel_auth_bridge.py: baredatabricks-oauth/azure-oauthfull-bundle, custom full override, custom-client_id-only fallback, port int-coercion, custom scopes. 44/44 pass, black clean, no regressions in kernel/session/auth unit suites.Related: PECOBLR-4039 (kernel-side default, Done).
This pull request and its description were written by Isaac.
This PR was created with GitHub MCP.