From f2d7b3b6125309f1685f90f959d3bbd1ab6f03c5 Mon Sep 17 00:00:00 2001 From: Madhavendra Rathore Date: Fri, 15 May 2026 14:17:52 +0000 Subject: [PATCH 01/10] =?UTF-8?q?sea-auth-u2m:=20OAuth=20M2M=20+=20U2M=20t?= =?UTF-8?q?hrough=20SeaBackend=20=E2=86=92=20napi=20binding=20=E2=86=92=20?= =?UTF-8?q?kernel?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds OAuth M2M and U2M onto the SEA auth path. The auth-u2m worktree landed both at once (rather than rebasing on top of the M2M branch) because the JS adapter flow-selector (`oauthClientSecret defined ? M2M : U2M`, mirroring thrift `DBSQLClient.ts:143`) is cleanest when both arms exist together — the no-secret branch was rejection-only in the M2M-alone state. The napi binding's `AuthMode` enum gains a third variant (`OAuthU2m`), crossing the FFI as the PascalCase string `'OAuthU2m'`. The JS adapter hardcodes `oauthRedirectPort: 8030` on the U2M payload to override the kernel default of 8020 — preserves parity with thrift, which defaults to 8030 (`OAuthManager.ts:245`). All other U2M knobs (`client_id`, `scopes`, `callback_timeout`, `token_url_override`) stay at kernel defaults; thrift hides them from its public surface too, so SEA follows the same pattern. `OAuthPersistence` is rejected on U2M with an explicit M1-Phase-2 deferral message: thrift exposes the hook, the kernel doesn't yet — parity gap to close once `AuthConfig::External` lands. The kernel disk cache at `~/.config/databricks-sql-kernel/oauth/{sha256}.json` covers the standard flow today. Azure-direct knobs (`azureTenantId` / `useDatabricksOAuthInAzure`) rejected on both M2M and U2M with the same "Phase 2" message — kernel uses workspace OIDC which works for Azure-databricks workspaces regardless. Task: M1 OAuth M2M + U2M (sea-auth feature, U2M worktree). Files: - native/sea/src/database.rs — AuthMode { Pat, OAuthM2m, OAuthU2m } + ConnectionOptions union + open_session dispatch with U2M arm forwarding `oauth_redirect_port` from JS and leaving every other U2M kernel knob at None - native/sea/index.{d.ts,js} — regenerated napi artifact - lib/sea/SeaAuth.ts — buildSeaConnectionOptions grows M2M + U2M branches; flow selector mirrors thrift; persistence rejection message reads as a parity gap, not a feature add - lib/sea/SeaNativeLoader.ts — SeaNativeBinding.openSession type accepts the three-arm discriminated payload - tests/unit/sea/auth-pat.test.ts — assertions updated for new `authMode: 'Pat'` round-trip; no-secret OAuth now asserts U2M happy-path dispatch - tests/unit/sea/auth-m2m.test.ts — new (8 cases — same as the M2M-worktree commit, minus the now-obsolete no-secret rejection) - tests/unit/sea/auth-u2m.test.ts — new (7 cases — happy path, port 8030 hardcode, clientId not propagated, path slash prepend, Azure rejected, persistence rejected, SeaBackend round-trip) - tests/integration/sea/auth-m2m-e2e.test.ts — env-gated live e2e (mirrors the M2M-worktree e2e) - tests/integration/sea/auth-u2m-e2e.test.ts — new (it.skip pending TBD-oauth_u2m_test_harness; comment points at testing-agent's Playwright/Puppeteer harness work) Tests: - Unit: 55/55 pass (`npm run test -- 'tests/unit/sea/**/*.test.ts'`): 13 PAT (assertions updated for authMode + no-secret now U2M), 8 M2M, 7 U2M, 25 SeaErrorMapping regression, 2 ConnectionOptions base. - U2M e2e: 1 pending (intentional `it.skip` — gated on browser harness). - M2M e2e: same as the M2M-worktree commit — kernel-side OAuth plumbing reaches the workspace; pecotesting SP credentials produce the workspace's `invalid_client` (verified reproducible via direct curl), an environmental issue not a code defect. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore --- lib/sea/SeaAuth.ts | 195 +++++++++++++---- lib/sea/SeaNativeLoader.ts | 22 +- native/sea/index.d.ts | 49 ++++- tests/integration/sea/auth-m2m-e2e.test.ts | 80 +++++++ tests/integration/sea/auth-u2m-e2e.test.ts | 72 +++++++ tests/unit/sea/auth-m2m.test.ts | 230 +++++++++++++++++++++ tests/unit/sea/auth-pat.test.ts | 29 ++- tests/unit/sea/auth-u2m.test.ts | 172 +++++++++++++++ 8 files changed, 791 insertions(+), 58 deletions(-) create mode 100644 tests/integration/sea/auth-m2m-e2e.test.ts create mode 100644 tests/integration/sea/auth-u2m-e2e.test.ts create mode 100644 tests/unit/sea/auth-m2m.test.ts create mode 100644 tests/unit/sea/auth-u2m.test.ts diff --git a/lib/sea/SeaAuth.ts b/lib/sea/SeaAuth.ts index cf16c80f..264daf02 100644 --- a/lib/sea/SeaAuth.ts +++ b/lib/sea/SeaAuth.ts @@ -16,18 +16,58 @@ import { ConnectionOptions } from '../contracts/IDBSQLClient'; import AuthenticationError from '../errors/AuthenticationError'; import HiveDriverError from '../errors/HiveDriverError'; +/** + * Auth-mode discriminant value crossing the napi boundary. The string + * literals are what napi-rs emits from the `#[napi(string_enum)] AuthMode` + * enum at `native/sea/src/database.rs` — they MUST match the variant + * names verbatim (`'Pat'`, `'OAuthM2m'`, `'OAuthU2m'`). + */ +export type SeaAuthMode = 'Pat' | 'OAuthM2m' | 'OAuthU2m'; + +/** + * Default local listener port for the U2M authorization-code callback. + * Hardcoded here so the override of the kernel default (8020) to the + * thrift default (8030) is invariant for SEA callers — preserving parity + * with the existing Node driver. Not exposed on the public + * `ConnectionOptions` (thrift hides `callbackPorts` from its public + * surface too — see nodejs-thrift-expert survey §B.2). + */ +const U2M_DEFAULT_REDIRECT_PORT = 8030; + /** * Shape consumed by the napi-binding's `openSession()` (see - * `native/sea/index.d.ts`). M0 supports PAT only — `token` is required. + * `native/sea/index.d.ts`). Mirrors `ConnectionOptions` in the binding's + * `.d.ts`; declared locally to avoid coupling the JS-side adapter to the + * auto-generated TS file. * - * Mirrors `ConnectionOptions` in the binding's `.d.ts`; declared locally - * to avoid coupling the JS-side adapter to the auto-generated TS file. + * Discriminated by `authMode`: + * - `'Pat'` → `token` is the PAT. + * - `'OAuthM2m'` → `oauthClientId` + `oauthClientSecret` drive a + * kernel-side client_credentials exchange. + * - `'OAuthU2m'` → `oauthRedirectPort` overrides the kernel default; + * everything else (client_id, scopes, callback timeout, + * token_url_override) uses kernel defaults. */ -export interface SeaNativeConnectionOptions { - hostName: string; - httpPath: string; - token: string; -} +export type SeaNativeConnectionOptions = + | { + hostName: string; + httpPath: string; + authMode: 'Pat'; + token: string; + } + | { + hostName: string; + httpPath: string; + authMode: 'OAuthM2m'; + oauthClientId: string; + oauthClientSecret: string; + } + | { + hostName: string; + httpPath: string; + authMode: 'OAuthU2m'; + oauthRedirectPort: number; + }; function prependSlash(str: string): string { if (str.length > 0 && str.charAt(0) !== '/') { @@ -37,47 +77,124 @@ function prependSlash(str: string): string { } /** - * Validate that the user-supplied `ConnectionOptions` describe a PAT auth - * configuration and build the napi-binding's connection-options shape. + * Validate the user-supplied `ConnectionOptions` and build the + * napi-binding's connection-options shape. * - * M0 SCOPE: PAT only. - * - Accepts `authType: 'access-token'` and the undefined-authType default - * (which already means PAT throughout the existing driver — see + * Supported auth modes: + * - PAT: `authType: 'access-token'` (or undefined, which already means + * PAT throughout the existing driver — see * `DBSQLClient.createAuthProvider`). - * - Rejects every other `authType` discriminant with a clear - * "M0 supports only PAT" message so callers know OAuth / Federation / - * custom providers land in M1. + * - OAuth M2M: `authType: 'databricks-oauth'` + `oauthClientId` + + * `oauthClientSecret`. Kernel handles OIDC discovery, client_credentials + * exchange, and re-auth on expiry internally (no caching needed — M2M + * never has a refresh token; see `auth/oauth/m2m.rs` and the thrift + * parity note at `OAuthManager.ts:178-181`). + * - OAuth U2M: `authType: 'databricks-oauth'` + NO `oauthClientSecret`. + * Kernel runs the PKCE auth-code dance (opens a browser, listens on + * localhost:8030, exchanges the code, persists to + * `~/.config/databricks-sql-kernel/oauth/{sha256}.json`). The flow + * selector matches thrift at `DBSQLClient.ts:143` — + * `oauthClientSecret defined ? M2M : U2M`. + * + * Out of scope on the OAuth paths (rejected with a clear error): + * - `azureTenantId` / `useDatabricksOAuthInAzure` → Microsoft Entra + * direct flow with `/.default` scope rewrite. The kernel + * uses workspace-OIDC discovery (which works against Azure workspaces + * too — they serve `/oidc/.well-known/...`); Entra-direct is a + * follow-on M1 Phase 2 task. + * - `persistence` on either flavor — for M2M the kernel doesn't cache + * (re-issuing is cheap; M2M has no refresh token). For U2M, custom + * persistence requires the kernel to expose `AuthConfig::External` + * (M1 Phase 2 task). The kernel-internal disk cache works for the + * standard flow today. * * Throws: - * - `AuthenticationError` when the auth mode is PAT but `token` is missing - * or empty. - * - `HiveDriverError` when the auth mode is anything other than PAT. + * - `AuthenticationError` for missing required credentials. + * - `HiveDriverError` for unsupported auth modes / Azure-direct / + * custom persistence. */ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNativeConnectionOptions { const { authType } = options as { authType?: string }; - if (authType !== undefined && authType !== 'access-token') { - throw new HiveDriverError( - `SEA backend (M0) supports only PAT auth (authType: 'access-token'); ` + - `got authType: '${authType}'. Other auth modes (databricks-oauth, ` + - `token-provider, external-token, static-token, custom) will land in M1.`, - ); + const base = { + hostName: options.host, + httpPath: prependSlash(options.path), + }; + + if (authType === undefined || authType === 'access-token') { + const { token } = options as { token?: string }; + if (typeof token !== 'string' || token.length === 0) { + throw new AuthenticationError( + 'SEA backend: a non-empty PAT must be supplied via `token` when using `authType: \'access-token\'`.', + ); + } + return { ...base, authMode: 'Pat', token }; } - // PAT path — at this point `options` is structurally the access-token branch - // of `AuthOptions`, which guarantees a `token` field at the type level. We - // still defensively re-check because the public ConnectionOptions type - // permits `authType: undefined` with no token at runtime. - const { token } = options as { token?: string }; - if (typeof token !== 'string' || token.length === 0) { - throw new AuthenticationError( - 'SEA backend: a non-empty PAT must be supplied via `token` when using `authType: \'access-token\'`.', - ); + if (authType === 'databricks-oauth') { + const oauth = options as { + oauthClientId?: string; + oauthClientSecret?: string; + azureTenantId?: string; + useDatabricksOAuthInAzure?: boolean; + persistence?: unknown; + }; + + if (oauth.azureTenantId !== undefined || oauth.useDatabricksOAuthInAzure === true) { + throw new HiveDriverError( + 'SEA backend: Azure-direct OAuth (azureTenantId / useDatabricksOAuthInAzure) ' + + 'is a later M1 task; the kernel uses workspace-OIDC discovery today, ' + + 'which works against Azure workspaces with no extra options.', + ); + } + + // Flow selector mirrors thrift's `DBSQLClient.createAuthProvider` + // (`DBSQLClient.ts:143`): `oauthClientSecret defined ? M2M : U2M`. + if (oauth.oauthClientSecret === undefined) { + // U2M. + if (oauth.persistence !== undefined) { + throw new HiveDriverError( + 'SEA backend: `persistence` (custom OAuth token store) is not yet wired through ' + + 'to the kernel — requires `AuthConfig::External` plumbing planned for M1 Phase 2. ' + + 'Today the kernel auto-persists U2M tokens to ' + + '`~/.config/databricks-sql-kernel/oauth/` which works for the standard flow; ' + + "the JS-supplied hook (matching thrift's `OAuthPersistence` interface) lands " + + 'when the kernel exposes it.', + ); + } + return { + ...base, + authMode: 'OAuthU2m', + oauthRedirectPort: U2M_DEFAULT_REDIRECT_PORT, + }; + } + + // M2M. + if (typeof oauth.oauthClientId !== 'string' || oauth.oauthClientId.length === 0) { + throw new AuthenticationError('SEA backend: `oauthClientId` is required for OAuth M2M.'); + } + if (typeof oauth.oauthClientSecret !== 'string' || oauth.oauthClientSecret.length === 0) { + throw new AuthenticationError( + 'SEA backend: `oauthClientSecret` must be a non-empty string for OAuth M2M.', + ); + } + if (oauth.persistence !== undefined) { + throw new HiveDriverError( + 'SEA backend: `persistence` is not supported on OAuth M2M ' + + '(M2M tokens have no refresh token; the kernel re-issues on expiry).', + ); + } + return { + ...base, + authMode: 'OAuthM2m', + oauthClientId: oauth.oauthClientId, + oauthClientSecret: oauth.oauthClientSecret, + }; } - return { - hostName: options.host, - httpPath: prependSlash(options.path), - token, - }; + throw new HiveDriverError( + `SEA backend: unsupported auth mode '${authType}'. ` + + `Supported modes today: 'access-token' (PAT), 'databricks-oauth' (M2M + U2M). ` + + `Other modes (token-provider, external-token, static-token, custom) are M1+ follow-ups.`, + ); } diff --git a/lib/sea/SeaNativeLoader.ts b/lib/sea/SeaNativeLoader.ts index a685b5e7..28b2a43f 100644 --- a/lib/sea/SeaNativeLoader.ts +++ b/lib/sea/SeaNativeLoader.ts @@ -44,9 +44,28 @@ export type SeaExecuteOptions = ExecuteOptions; export type SeaArrowBatch = ArrowBatch; export type SeaArrowSchema = ArrowSchema; +/** + * Discriminated session-open options. Mirrors the auto-generated + * `ConnectionOptions` in `native/sea/index.d.ts` as it lands across + * the auth PRs (PAT today; OAuth M2M + U2M after the kernel-side + * OAuth surface ships). Kept permissive (all OAuth fields optional) + * so the JS adapter's auth-union narrows correctly without three + * overloads on this method. + */ +export interface SeaOpenSessionOptions { + hostName: string; + httpPath: string; + authMode: 'Pat' | 'OAuthM2m' | 'OAuthU2m'; + token?: string; + oauthClientId?: string; + oauthClientSecret?: string; + oauthRedirectPort?: number; +} + export interface SeaNativeBinding { version(): string; - openSession(options: ConnectionOptions): Promise; + /** Open a session over PAT, OAuth M2M, or OAuth U2M auth. */ + openSession(opts: SeaOpenSessionOptions): Promise; Connection: typeof NativeConnection; Statement: typeof NativeStatement; } @@ -96,7 +115,6 @@ function tryLoad(): SeaNativeBinding | undefined { cachedError = new Error(`SEA native binding failed to load with non-standard error: ${String(err)}`); return undefined; } -} /** * Returns the loaded native binding. Throws a structured error if diff --git a/native/sea/index.d.ts b/native/sea/index.d.ts index 5fb5e902..fd0ea3bc 100644 --- a/native/sea/index.d.ts +++ b/native/sea/index.d.ts @@ -20,10 +20,34 @@ export interface ExecuteOptions { sessionConfig?: Record } /** - * JS-visible options for opening a Databricks SQL session over PAT. + * JS-visible auth-mode discriminant. * - * M0 supports PAT only — `token` is required. OAuth M2M / U2M variants - * land in M1 along with a discriminated-union shape on the JS side. + * Crosses the FFI as the string value (napi-rs string-enums emit the + * Rust variant name verbatim on the JS side — `'Pat'`, `'OAuthM2m'`, + * `'OAuthU2m'`). Keeping the discriminant explicit instead of inferring + * it from "which Option is set" makes the napi-side validation + * single-pass and the JS-side schema typed. + */ +export const enum AuthMode { + Pat = 'Pat', + OAuthM2m = 'OAuthM2m', + OAuthU2m = 'OAuthU2m' +} +/** + * JS-visible options for opening a Databricks SQL session. + * + * Discriminated by `auth_mode`: + * - `AuthMode::Pat` → requires `token`; ignores oauth_*. + * - `AuthMode::OAuthM2m` → requires `oauth_client_id` + `oauth_client_secret`. + * - `AuthMode::OAuthU2m` → kernel handles the auth-code flow with + * default client_id (`databricks-cli`), scopes + * (`["all-apis","offline_access"]`), and OIDC discovery; the JS + * adapter hardcodes `oauth_redirect_port` to 8030 to override the + * kernel default of 8020 (thrift uses 8030 — preserves parity). + * + * Scopes / token_url_override / client_id / callback_timeout are not + * exposed — kernel defaults match thrift parity and the public driver + * surface has no demand to override them. */ export interface ConnectionOptions { /** @@ -36,15 +60,24 @@ export interface ConnectionOptions { * kernel parses out the warehouse id. */ httpPath: string + /** Auth-mode discriminant. Required. */ + authMode: AuthMode + /** Personal access token. Required iff `auth_mode == Pat`. */ + token?: string + /** OAuth client id. Required iff `auth_mode == OAuthM2m`. */ + oauthClientId?: string + /** OAuth client secret. Required iff `auth_mode == OAuthM2m`. */ + oauthClientSecret?: string /** - * Personal access token. Must be non-empty (the kernel rejects - * empty PATs at session construction). + * Local listener port for the U2M authorization-code callback. + * Forwarded verbatim to the kernel; the JS adapter hardcodes 8030 + * for thrift parity. */ - token: string + oauthRedirectPort?: number } /** - * Open a Databricks SQL session over PAT auth and return an opaque - * `Connection` wrapping the kernel `Session`. + * Open a Databricks SQL session and return an opaque `Connection` + * wrapping the kernel `Session`. Supports PAT, OAuth M2M, and OAuth U2M. * * The JS-visible name is `openSession` (napi-rs converts snake_case * to camelCase for free functions). diff --git a/tests/integration/sea/auth-m2m-e2e.test.ts b/tests/integration/sea/auth-m2m-e2e.test.ts new file mode 100644 index 00000000..7a7417ab --- /dev/null +++ b/tests/integration/sea/auth-m2m-e2e.test.ts @@ -0,0 +1,80 @@ +// Copyright (c) 2026 Databricks, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { expect } from 'chai'; +import { DBSQLClient } from '../../../lib'; + +/** + * sea-auth M1 OAuth M2M end-to-end: + * 1. Construct a DBSQLClient. + * 2. `connect({ useSEA: true, authType: 'databricks-oauth', oauthClientId, + * oauthClientSecret })` against pecotesting. + * 3. `openSession()` — kernel runs OIDC discovery + client_credentials + * exchange. Successful openSession is the proof that the kernel-side + * OAuth M2M plumbing works end-to-end: discovery + token exchange + + * Bearer header on the create-session request all succeeded. + * 4. Close the session, then the client. + * + * No query is executed here — execution is the responsibility of the + * sea-execution feature's own e2e (mirror of the M0 PAT e2e scope at + * `auth-pat-e2e.test.ts`). If kernel-side OAuth fails, `openSession()` + * raises before returning. + * + * Required env (exported by `~/.zshrc` on the developer machine): + * - DATABRICKS_PECOTESTING_SERVER_HOSTNAME + * - DATABRICKS_PECOTESTING_HTTP_PATH + * - DATABRICKS_PECO_CLIENT_ID + * - DATABRICKS_PECO_CLIENT_SECRET + * + * Skipped (not failed) when any of the four env vars is missing, so CI + * machines without OAuth credentials don't fail-flap. + */ +describe('sea-auth e2e — OAuth M2M through DBSQLClient ↔ SeaBackend ↔ napi binding', function suite() { + const host = process.env.DATABRICKS_PECOTESTING_SERVER_HOSTNAME; + const path = process.env.DATABRICKS_PECOTESTING_HTTP_PATH; + const oauthClientId = process.env.DATABRICKS_PECO_CLIENT_ID; + const oauthClientSecret = process.env.DATABRICKS_PECO_CLIENT_SECRET; + + this.timeout(120_000); + + before(function gate() { + if (!host || !path || !oauthClientId || !oauthClientSecret) { + // eslint-disable-next-line no-invalid-this + this.skip(); + } + }); + + it('connects, opens a session, closes the session, closes the client', async () => { + const client = new DBSQLClient(); + + const connected = await client.connect({ + host: host as string, + path: path as string, + authType: 'databricks-oauth', + oauthClientId: oauthClientId as string, + oauthClientSecret: oauthClientSecret as string, + useSEA: true, + }); + expect(connected).to.equal(client); + + const session = await client.openSession(); + expect(session.id).to.be.a('string'); + expect(session.id.length).to.be.greaterThan(0); + + const status = await session.close(); + expect(status.isSuccess).to.equal(true); + + await client.close(); + }); +}); diff --git a/tests/integration/sea/auth-u2m-e2e.test.ts b/tests/integration/sea/auth-u2m-e2e.test.ts new file mode 100644 index 00000000..93d7c9c3 --- /dev/null +++ b/tests/integration/sea/auth-u2m-e2e.test.ts @@ -0,0 +1,72 @@ +// Copyright (c) 2026 Databricks, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { expect } from 'chai'; +import { DBSQLClient } from '../../../lib'; + +/** + * sea-auth M1 OAuth U2M end-to-end — **SKIPPED pending browser harness**. + * + * U2M is interactive: the kernel opens a system browser + * (`auth/oauth/u2m.rs:414`, via the `open` crate), binds a local + * listener on port 8030 (via the JS adapter's hardcoded override), and + * waits up to 120s for the user to authenticate. + * + * Driving this from CI requires Playwright/Puppeteer to navigate the + * browser through the workspace login + consent screens. That harness + * is tracked as `TBD-oauth_u2m_test_harness` in testing-agent's + * findings; until it exists, this test stays `it.skip` so the e2e + * suite carries a slot for whoever lands the harness work. + * + * The intended assertion sequence (mirrors `auth-m2m-e2e.test.ts`): + * 1. `client.connect({ useSEA: true, authType: 'databricks-oauth' })` + * — NO `oauthClientSecret` → kernel picks the U2M flow. + * 2. `openSession()` — kernel opens browser, waits for callback on + * localhost:8030, exchanges the auth code, returns Bearer token, + * issues the create-session request to SEA. + * 3. `session.close()` then `client.close()`. + * + * Required env (gated additionally via `it.skip` until the harness + * lands, so absent env is a no-op today): + * - DATABRICKS_PECOTESTING_SERVER_HOSTNAME + * - DATABRICKS_PECOTESTING_HTTP_PATH + * - (no client_id/secret — U2M uses kernel default `databricks-cli`) + */ +describe('sea-auth e2e — OAuth U2M through DBSQLClient ↔ SeaBackend ↔ napi binding', function suite() { + this.timeout(300_000); + + // eslint-disable-next-line mocha/no-skipped-tests + it.skip('[pending TBD-oauth_u2m_test_harness] interactive U2M round-trip', async () => { + const host = process.env.DATABRICKS_PECOTESTING_SERVER_HOSTNAME as string; + const path = process.env.DATABRICKS_PECOTESTING_HTTP_PATH as string; + + const client = new DBSQLClient(); + + const connected = await client.connect({ + host, + path, + authType: 'databricks-oauth', + useSEA: true, + }); + expect(connected).to.equal(client); + + const session = await client.openSession(); + expect(session.id).to.be.a('string'); + + const status = await session.close(); + expect(status.isSuccess).to.equal(true); + + await client.close(); + }); +}); diff --git a/tests/unit/sea/auth-m2m.test.ts b/tests/unit/sea/auth-m2m.test.ts new file mode 100644 index 00000000..97f8241f --- /dev/null +++ b/tests/unit/sea/auth-m2m.test.ts @@ -0,0 +1,230 @@ +// Copyright (c) 2026 Databricks, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { expect } from 'chai'; +import SeaBackend from '../../../lib/sea/SeaBackend'; +import { buildSeaConnectionOptions } from '../../../lib/sea/SeaAuth'; +import { SeaNativeBinding } from '../../../lib/sea/SeaNativeLoader'; +import { ConnectionOptions } from '../../../lib/contracts/IDBSQLClient'; +import AuthenticationError from '../../../lib/errors/AuthenticationError'; +import HiveDriverError from '../../../lib/errors/HiveDriverError'; + +function makeFakeBinding() { + const calls: Array<{ method: string; args: unknown[] }> = []; + + const fakeConnection = { + async executeStatement() { + throw new Error('not used in this test'); + }, + async close() { + calls.push({ method: 'connection.close', args: [] }); + }, + }; + + const binding: SeaNativeBinding = { + version() { + return 'fake-binding'; + }, + async openSession(opts: Parameters[0]) { + calls.push({ method: 'openSession', args: [opts] }); + return fakeConnection as unknown; + }, + Connection: function FakeConnection() {} as unknown as Function, + Statement: function FakeStatement() {} as unknown as Function, + }; + + return { binding, calls }; +} + +describe('SeaAuth + SeaBackend — OAuth M2M auth flow', () => { + describe('buildSeaConnectionOptions', () => { + it('accepts databricks-oauth + oauthClientId + oauthClientSecret', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'client-uuid', + oauthClientSecret: 'dose-fake-secret', + }; + + const native = buildSeaConnectionOptions(opts); + expect(native).to.deep.equal({ + hostName: 'example.cloud.databricks.com', + httpPath: '/sql/1.0/warehouses/abc', + authMode: 'OAuthM2m', + oauthClientId: 'client-uuid', + oauthClientSecret: 'dose-fake-secret', + }); + }); + + it('prepends `/` to the path on the M2M branch too', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: 'sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'client-uuid', + oauthClientSecret: 'dose-fake-secret', + }; + + const native = buildSeaConnectionOptions(opts); + expect(native.httpPath).to.equal('/sql/1.0/warehouses/abc'); + }); + + it('rejects missing oauthClientId with AuthenticationError', () => { + const opts = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientSecret: 'dose-fake-secret', + } as unknown as ConnectionOptions; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + AuthenticationError, + /oauthClientId.*required/, + ); + }); + + it('rejects empty oauthClientId with AuthenticationError', () => { + const opts = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: '', + oauthClientSecret: 'dose-fake-secret', + } as unknown as ConnectionOptions; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + AuthenticationError, + /oauthClientId.*required/, + ); + }); + + it('rejects empty oauthClientSecret with AuthenticationError', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'client-uuid', + oauthClientSecret: '', + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + AuthenticationError, + /oauthClientSecret.*non-empty/, + ); + }); + + it('rejects azureTenantId with a clear Entra-direct-out-of-scope error', () => { + const opts: ConnectionOptions = { + host: 'adb-12345.0.azuredatabricks.net', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'client-uuid', + oauthClientSecret: 'dose-fake-secret', + azureTenantId: 'tenant-uuid', + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /Azure-direct OAuth.*later M1 task/, + ); + }); + + it('rejects useDatabricksOAuthInAzure with the same Entra-direct error', () => { + const opts: ConnectionOptions = { + host: 'adb-12345.0.azuredatabricks.net', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'client-uuid', + oauthClientSecret: 'dose-fake-secret', + useDatabricksOAuthInAzure: true, + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /Azure-direct OAuth.*later M1 task/, + ); + }); + + it('rejects a `persistence` hook on M2M (no cache needed)', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'client-uuid', + oauthClientSecret: 'dose-fake-secret', + persistence: { + read: async () => undefined, + persist: async () => undefined, + }, + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /`persistence` is not supported on OAuth M2M/, + ); + }); + }); + + describe('SeaBackend.connect + openSession (M2M)', () => { + it('round-trips M2M options through to the napi binding', async () => { + const { binding, calls } = makeFakeBinding(); + const backend = new SeaBackend(binding); + + await backend.connect({ + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'client-uuid', + oauthClientSecret: 'dose-fake-secret', + }); + + const session = await backend.openSession({}); + expect(session.id).to.match(/^sea-session-\d+$/); + + expect(calls).to.have.lengthOf(1); + expect(calls[0].method).to.equal('openSession'); + expect(calls[0].args[0]).to.deep.equal({ + hostName: 'example.cloud.databricks.com', + httpPath: '/sql/1.0/warehouses/abc', + authMode: 'OAuthM2m', + oauthClientId: 'client-uuid', + oauthClientSecret: 'dose-fake-secret', + }); + + await session.close(); + await backend.close(); + }); + + it('rejects connect() for missing oauthClientId before touching the binding', async () => { + const { binding, calls } = makeFakeBinding(); + const backend = new SeaBackend(binding); + + let caught: unknown; + try { + await backend.connect({ + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + oauthClientSecret: 'dose-fake-secret', + } as any); + } catch (e) { + caught = e; + } + expect(caught).to.be.instanceOf(AuthenticationError); + expect(calls).to.have.lengthOf(0); + }); + }); +}); diff --git a/tests/unit/sea/auth-pat.test.ts b/tests/unit/sea/auth-pat.test.ts index 21d5d629..f691f754 100644 --- a/tests/unit/sea/auth-pat.test.ts +++ b/tests/unit/sea/auth-pat.test.ts @@ -31,6 +31,7 @@ describe('SeaAuth — PAT auth options builder', () => { expect(native).to.deep.equal({ hostName: 'example.cloud.databricks.com', httpPath: '/sql/1.0/warehouses/abc', + authMode: 'Pat', token: 'dapi-fake-pat', }); }); @@ -44,7 +45,10 @@ describe('SeaAuth — PAT auth options builder', () => { }; const native = buildSeaConnectionOptions(opts); - expect(native.token).to.equal('dapi-fake-pat'); + expect(native.authMode).to.equal('Pat'); + if (native.authMode === 'Pat') { + expect(native.token).to.equal('dapi-fake-pat'); + } }); it('prepends `/` to a path missing the leading slash', () => { @@ -79,20 +83,18 @@ describe('SeaAuth — PAT auth options builder', () => { expect(() => buildSeaConnectionOptions(opts)).to.throw(AuthenticationError, /non-empty PAT/); }); - it('rejects OAuth with a clear M0-scope error', () => { + it('accepts databricks-oauth without oauthClientSecret as the U2M happy path', () => { const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', authType: 'databricks-oauth', }; - expect(() => buildSeaConnectionOptions(opts)).to.throw( - HiveDriverError, - /M0\) supports only PAT.*databricks-oauth.*M1/, - ); + const native = buildSeaConnectionOptions(opts); + expect(native.authMode).to.equal('OAuthU2m'); }); - it('rejects token-provider with a clear M0-scope error', () => { + it('rejects token-provider with a clear unsupported-mode error', () => { const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', @@ -103,7 +105,10 @@ describe('SeaAuth — PAT auth options builder', () => { : never, }; - expect(() => buildSeaConnectionOptions(opts)).to.throw(HiveDriverError, /token-provider.*M1/); + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /unsupported auth mode 'token-provider'/, + ); }); it('rejects external-token, static-token, and custom auth modes', () => { @@ -115,7 +120,10 @@ describe('SeaAuth — PAT auth options builder', () => { path: '/p', authType, } as any; - expect(() => buildSeaConnectionOptions(opts)).to.throw(HiveDriverError, /M0\) supports only PAT/); + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /unsupported auth mode/, + ); } }); }); @@ -124,4 +132,7 @@ describe('SeaAuth — PAT auth options builder', () => { // moved to tests/unit/sea/execution.test.ts during the sea-integration // merge (the execution branch's SeaBackend constructor signature // {context, nativeBinding} supersedes the auth-only (binding) shape). + // OAuth-specific flow-dispatch tests live in auth-m2m.test.ts and + // auth-u2m.test.ts; M2M end-to-end against a live workspace lives in + // tests/integration/sea/auth-m2m-e2e.test.ts. }); diff --git a/tests/unit/sea/auth-u2m.test.ts b/tests/unit/sea/auth-u2m.test.ts new file mode 100644 index 00000000..8c8b9d86 --- /dev/null +++ b/tests/unit/sea/auth-u2m.test.ts @@ -0,0 +1,172 @@ +// Copyright (c) 2026 Databricks, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { expect } from 'chai'; +import SeaBackend from '../../../lib/sea/SeaBackend'; +import { buildSeaConnectionOptions } from '../../../lib/sea/SeaAuth'; +import { SeaNativeBinding } from '../../../lib/sea/SeaNativeLoader'; +import { ConnectionOptions } from '../../../lib/contracts/IDBSQLClient'; +import HiveDriverError from '../../../lib/errors/HiveDriverError'; + +function makeFakeBinding() { + const calls: Array<{ method: string; args: unknown[] }> = []; + + const fakeConnection = { + async executeStatement() { + throw new Error('not used in this test'); + }, + async close() { + calls.push({ method: 'connection.close', args: [] }); + }, + }; + + const binding: SeaNativeBinding = { + version() { + return 'fake-binding'; + }, + async openSession(opts: Parameters[0]) { + calls.push({ method: 'openSession', args: [opts] }); + return fakeConnection as unknown; + }, + Connection: function FakeConnection() {} as unknown as Function, + Statement: function FakeStatement() {} as unknown as Function, + }; + + return { binding, calls }; +} + +describe('SeaAuth + SeaBackend — OAuth U2M auth flow', () => { + describe('buildSeaConnectionOptions', () => { + it('accepts databricks-oauth with no clientSecret as the U2M happy path (hardcoded port 8030)', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + }; + + const native = buildSeaConnectionOptions(opts); + expect(native).to.deep.equal({ + hostName: 'example.cloud.databricks.com', + httpPath: '/sql/1.0/warehouses/abc', + authMode: 'OAuthU2m', + oauthRedirectPort: 8030, + }); + }); + + it('drops the supplied oauthClientId on the U2M path (kernel uses its own default)', () => { + // The thrift parity story: thrift's getClientId() falls back to + // `databricks-cli` when undefined. Here we tell the kernel to do + // the same via `client_id: None`. If a user supplies a clientId + // alongside no secret, we treat that as U2M and use kernel default + // — explicitly NOT propagating the supplied id, because the kernel + // surface for U2M client_id is None-or-Some-with-no-default-rewrite, + // and exposing the override here is out-of-scope-for-this-task. + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'custom-client', + }; + + const native = buildSeaConnectionOptions(opts); + expect(native.authMode).to.equal('OAuthU2m'); + // Custom clientId is intentionally not forwarded — see comment above. + expect(native).to.not.have.property('oauthClientId'); + }); + + it('prepends `/` to the path on the U2M branch too', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: 'sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + }; + + const native = buildSeaConnectionOptions(opts); + expect(native.httpPath).to.equal('/sql/1.0/warehouses/abc'); + }); + + it('rejects azureTenantId on the U2M path with the Entra-direct error', () => { + const opts: ConnectionOptions = { + host: 'adb-12345.0.azuredatabricks.net', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + azureTenantId: 'tenant-uuid', + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /Azure-direct OAuth.*later M1 task/, + ); + }); + + it('rejects useDatabricksOAuthInAzure on the U2M path', () => { + const opts: ConnectionOptions = { + host: 'adb-12345.0.azuredatabricks.net', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + useDatabricksOAuthInAzure: true, + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /Azure-direct OAuth.*later M1 task/, + ); + }); + + it('rejects a `persistence` hook on U2M with the AuthConfig::External M1-Phase-2 message', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + persistence: { + read: async () => undefined, + persist: async () => undefined, + }, + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /AuthConfig::External.*plumbing planned for M1 Phase 2/, + ); + }); + }); + + describe('SeaBackend.connect + openSession (U2M)', () => { + it('round-trips U2M options through to the napi binding', async () => { + const { binding, calls } = makeFakeBinding(); + const backend = new SeaBackend(binding); + + await backend.connect({ + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + }); + + const session = await backend.openSession({}); + expect(session.id).to.match(/^sea-session-\d+$/); + + expect(calls).to.have.lengthOf(1); + expect(calls[0].method).to.equal('openSession'); + expect(calls[0].args[0]).to.deep.equal({ + hostName: 'example.cloud.databricks.com', + httpPath: '/sql/1.0/warehouses/abc', + authMode: 'OAuthU2m', + oauthRedirectPort: 8030, + }); + + await session.close(); + await backend.close(); + }); + }); +}); From 7903538340f22402b42d3798f04a7c446fba6481 Mon Sep 17 00:00:00 2001 From: Madhavendra Rathore Date: Fri, 15 May 2026 14:28:32 +0000 Subject: [PATCH 02/10] =?UTF-8?q?sea-auth-u2m:=20address=20round-1=20M2M?= =?UTF-8?q?=20review=20parity=20=E2=80=94=20shared=20fakeBinding=20helper,?= =?UTF-8?q?=20doc=20+=20loader=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports the M2M-side round-1 review fixes (commit 88d7d21 on sea-auth-m2m) onto the U2M worktree so the two branches stay aligned in review quality. The U2M-specific work in 5eba37f is unchanged; this commit is pure cleanup applied across all three SEA-auth test files (PAT / M2M / U2M). - Extracted `makeFakeBinding()` to `tests/unit/sea/_helpers/fakeBinding.ts` and refactored all three auth-*.test.ts files to import it. The U2M-worktree commit had THREE copies of the closure (the third was the cause for the bloat reviewer's "rule of three" call-out that the M2M-worktree fixup was meant to forestall). - Dropped the unused `SeaAuthMode` type alias from `SeaAuth.ts` — zero callers; inlined literals already power the discriminated union. - Tightened `SeaNativeBinding.openSession` parameter type to consume the discriminated `SeaNativeConnectionOptions` union from `SeaAuth.ts`, restoring compile-time per-mode field enforcement at the FFI seam. - Augmented the Rust `AuthMode` doc-comment with the napi-emission explanation (PascalCase verbatim, not kebab-case) plus the cross-reference reminder to extend `open_session()`'s match on every new variant. - Added the const-enum hazard note to `SeaNativeConnectionOptions`' doc-comment, locking in the duplicated-literal pattern as deliberate (importing the napi `const enum AuthMode` breaks `isolatedModules`). - Cleaned up the conditional-type-cast lobotomy in `auth-pat.test.ts` on the token-provider fixture; plain `as any` + eslint-disable. Skipped findings (same justification as M2M-worktree commit): F-3 borderline error-class taxonomy, F-4 cosmetic arg-order, F-5 redundant comment-anchor (compiler already enforces), F-8 null vs undefined paranoia, F-9 mocha named-function style. Tests: - Unit: 55/55 pass (same count as 5eba37f — pure restructure). - Native build: clean (1m04s release profile). - Type-check: clean (tsc --noEmit). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore --- lib/sea/SeaAuth.ts | 17 ++++---- lib/sea/SeaNativeLoader.ts | 35 ++++++--------- native/sea/index.d.ts | 21 ++++++--- tests/unit/sea/_helpers/fakeBinding.ts | 60 ++++++++++++++++++++++++++ tests/unit/sea/auth-m2m.test.ts | 29 +------------ tests/unit/sea/auth-pat.test.ts | 6 +-- tests/unit/sea/auth-u2m.test.ts | 29 +------------ 7 files changed, 102 insertions(+), 95 deletions(-) create mode 100644 tests/unit/sea/_helpers/fakeBinding.ts diff --git a/lib/sea/SeaAuth.ts b/lib/sea/SeaAuth.ts index 264daf02..5af707b2 100644 --- a/lib/sea/SeaAuth.ts +++ b/lib/sea/SeaAuth.ts @@ -16,14 +16,6 @@ import { ConnectionOptions } from '../contracts/IDBSQLClient'; import AuthenticationError from '../errors/AuthenticationError'; import HiveDriverError from '../errors/HiveDriverError'; -/** - * Auth-mode discriminant value crossing the napi boundary. The string - * literals are what napi-rs emits from the `#[napi(string_enum)] AuthMode` - * enum at `native/sea/src/database.rs` — they MUST match the variant - * names verbatim (`'Pat'`, `'OAuthM2m'`, `'OAuthU2m'`). - */ -export type SeaAuthMode = 'Pat' | 'OAuthM2m' | 'OAuthU2m'; - /** * Default local listener port for the U2M authorization-code callback. * Hardcoded here so the override of the kernel default (8020) to the @@ -47,6 +39,15 @@ const U2M_DEFAULT_REDIRECT_PORT = 8030; * - `'OAuthU2m'` → `oauthRedirectPort` overrides the kernel default; * everything else (client_id, scopes, callback timeout, * token_url_override) uses kernel defaults. + * + * The `authMode` string literals MUST match the napi-emitted `AuthMode` + * variant names verbatim (`'Pat'`, `'OAuthM2m'`, `'OAuthU2m'` — napi-rs's + * `#[napi(string_enum)]` without an explicit case option emits the + * Rust variant identifier as-is). We duplicate the values here instead + * of importing `AuthMode` from `native/sea/index.d.ts` because that + * file declares `AuthMode` as `export const enum`, which is + * incompatible with `isolatedModules` and a runtime-coupling hazard. + * The Rust source of truth lives at `native/sea/src/database.rs`. */ export type SeaNativeConnectionOptions = | { diff --git a/lib/sea/SeaNativeLoader.ts b/lib/sea/SeaNativeLoader.ts index 28b2a43f..154fd104 100644 --- a/lib/sea/SeaNativeLoader.ts +++ b/lib/sea/SeaNativeLoader.ts @@ -26,13 +26,13 @@ import type { Connection as NativeConnection, Statement as NativeStatement, - ConnectionOptions, ExecuteOptions, ArrowBatch, ArrowSchema, } from '@sea-native'; +import type { SeaNativeConnectionOptions } from './SeaAuth'; -export type { ConnectionOptions, ExecuteOptions, ArrowBatch, ArrowSchema }; +export type { ExecuteOptions, ArrowBatch, ArrowSchema, SeaNativeConnectionOptions }; export type Connection = NativeConnection; export type Statement = NativeStatement; @@ -44,28 +44,18 @@ export type SeaExecuteOptions = ExecuteOptions; export type SeaArrowBatch = ArrowBatch; export type SeaArrowSchema = ArrowSchema; -/** - * Discriminated session-open options. Mirrors the auto-generated - * `ConnectionOptions` in `native/sea/index.d.ts` as it lands across - * the auth PRs (PAT today; OAuth M2M + U2M after the kernel-side - * OAuth surface ships). Kept permissive (all OAuth fields optional) - * so the JS adapter's auth-union narrows correctly without three - * overloads on this method. - */ -export interface SeaOpenSessionOptions { - hostName: string; - httpPath: string; - authMode: 'Pat' | 'OAuthM2m' | 'OAuthU2m'; - token?: string; - oauthClientId?: string; - oauthClientSecret?: string; - oauthRedirectPort?: number; -} - export interface SeaNativeBinding { version(): string; - /** Open a session over PAT, OAuth M2M, or OAuth U2M auth. */ - openSession(opts: SeaOpenSessionOptions): Promise; + /** + * Open a session over PAT, OAuth M2M, or OAuth U2M auth. Returns an + * opaque Connection. The discriminated `SeaNativeConnectionOptions` + * union from `SeaAuth` is the source of truth for the per-mode + * required fields, so the loader-seam enforces the same compile-time + * check the adapter applies — a caller can't bypass + * `buildSeaConnectionOptions` and pass, say, `{ authMode: 'Pat' }` + * with no token. + */ + openSession(opts: SeaNativeConnectionOptions): Promise; Connection: typeof NativeConnection; Statement: typeof NativeStatement; } @@ -115,6 +105,7 @@ function tryLoad(): SeaNativeBinding | undefined { cachedError = new Error(`SEA native binding failed to load with non-standard error: ${String(err)}`); return undefined; } +} /** * Returns the loaded native binding. Throws a structured error if diff --git a/native/sea/index.d.ts b/native/sea/index.d.ts index fd0ea3bc..97257895 100644 --- a/native/sea/index.d.ts +++ b/native/sea/index.d.ts @@ -22,11 +22,22 @@ export interface ExecuteOptions { /** * JS-visible auth-mode discriminant. * - * Crosses the FFI as the string value (napi-rs string-enums emit the - * Rust variant name verbatim on the JS side — `'Pat'`, `'OAuthM2m'`, - * `'OAuthU2m'`). Keeping the discriminant explicit instead of inferring - * it from "which Option is set" makes the napi-side validation - * single-pass and the JS-side schema typed. + * Crosses the FFI as the variant name verbatim — napi-rs's + * `#[napi(string_enum)]` without an explicit case option emits the + * Rust variant identifier as-is, so this enum becomes + * `AuthMode.Pat = 'Pat'` / `AuthMode.OAuthM2m = 'OAuthM2m'` / + * `AuthMode.OAuthU2m = 'OAuthU2m'` in the auto-generated + * `native/sea/index.d.ts`. The JS adapter + * (`SeaNativeConnectionOptions` in `lib/sea/SeaAuth.ts`) must use the + * PascalCase literals verbatim. + * + * Keeping the discriminant explicit instead of inferring it from + * "which Option is set" makes the napi-side validation single-pass + * and the JS-side schema typed. + * + * Note: adding a variant here requires extending `open_session()`'s + * `match` — Rust will fail the build if the match is non-exhaustive, + * but the cross-reference shortens the review loop. */ export const enum AuthMode { Pat = 'Pat', diff --git a/tests/unit/sea/_helpers/fakeBinding.ts b/tests/unit/sea/_helpers/fakeBinding.ts new file mode 100644 index 00000000..2420a045 --- /dev/null +++ b/tests/unit/sea/_helpers/fakeBinding.ts @@ -0,0 +1,60 @@ +// Copyright (c) 2026 Databricks, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { SeaNativeBinding } from '../../../../lib/sea/SeaNativeLoader'; + +export interface RecordedCall { + method: string; + args: unknown[]; +} + +export interface FakeBinding { + binding: SeaNativeBinding; + calls: RecordedCall[]; +} + +/** + * Build a fake `SeaNativeBinding` that records every `openSession` call + * and returns a `Connection` whose `close()` is also recorded. Shared + * across the SEA auth unit-test files (PAT / M2M / U2M / future flows) + * so the closure shape lives in exactly one place. + * + * No real native code runs — the fake is structural-typing-only. + */ +export function makeFakeBinding(): FakeBinding { + const calls: RecordedCall[] = []; + + const fakeConnection = { + async executeStatement() { + throw new Error('not used in this test'); + }, + async close() { + calls.push({ method: 'connection.close', args: [] }); + }, + }; + + const binding: SeaNativeBinding = { + version() { + return 'fake-binding'; + }, + async openSession(opts: Parameters[0]) { + calls.push({ method: 'openSession', args: [opts] }); + return fakeConnection as unknown; + }, + Connection: function FakeConnection() {} as unknown as Function, + Statement: function FakeStatement() {} as unknown as Function, + }; + + return { binding, calls }; +} diff --git a/tests/unit/sea/auth-m2m.test.ts b/tests/unit/sea/auth-m2m.test.ts index 97f8241f..f757ac7e 100644 --- a/tests/unit/sea/auth-m2m.test.ts +++ b/tests/unit/sea/auth-m2m.test.ts @@ -15,37 +15,10 @@ import { expect } from 'chai'; import SeaBackend from '../../../lib/sea/SeaBackend'; import { buildSeaConnectionOptions } from '../../../lib/sea/SeaAuth'; -import { SeaNativeBinding } from '../../../lib/sea/SeaNativeLoader'; import { ConnectionOptions } from '../../../lib/contracts/IDBSQLClient'; import AuthenticationError from '../../../lib/errors/AuthenticationError'; import HiveDriverError from '../../../lib/errors/HiveDriverError'; - -function makeFakeBinding() { - const calls: Array<{ method: string; args: unknown[] }> = []; - - const fakeConnection = { - async executeStatement() { - throw new Error('not used in this test'); - }, - async close() { - calls.push({ method: 'connection.close', args: [] }); - }, - }; - - const binding: SeaNativeBinding = { - version() { - return 'fake-binding'; - }, - async openSession(opts: Parameters[0]) { - calls.push({ method: 'openSession', args: [opts] }); - return fakeConnection as unknown; - }, - Connection: function FakeConnection() {} as unknown as Function, - Statement: function FakeStatement() {} as unknown as Function, - }; - - return { binding, calls }; -} +import { makeFakeBinding } from './_helpers/fakeBinding'; describe('SeaAuth + SeaBackend — OAuth M2M auth flow', () => { describe('buildSeaConnectionOptions', () => { diff --git a/tests/unit/sea/auth-pat.test.ts b/tests/unit/sea/auth-pat.test.ts index f691f754..bdd024f7 100644 --- a/tests/unit/sea/auth-pat.test.ts +++ b/tests/unit/sea/auth-pat.test.ts @@ -99,10 +99,8 @@ describe('SeaAuth — PAT auth options builder', () => { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', authType: 'token-provider', - tokenProvider: { getToken: async () => 'tok' } as unknown as ConnectionOptions extends infer T - ? // eslint-disable-next-line @typescript-eslint/no-explicit-any - any - : never, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + tokenProvider: { getToken: async () => 'tok' } as any, }; expect(() => buildSeaConnectionOptions(opts)).to.throw( diff --git a/tests/unit/sea/auth-u2m.test.ts b/tests/unit/sea/auth-u2m.test.ts index 8c8b9d86..09ac837d 100644 --- a/tests/unit/sea/auth-u2m.test.ts +++ b/tests/unit/sea/auth-u2m.test.ts @@ -15,36 +15,9 @@ import { expect } from 'chai'; import SeaBackend from '../../../lib/sea/SeaBackend'; import { buildSeaConnectionOptions } from '../../../lib/sea/SeaAuth'; -import { SeaNativeBinding } from '../../../lib/sea/SeaNativeLoader'; import { ConnectionOptions } from '../../../lib/contracts/IDBSQLClient'; import HiveDriverError from '../../../lib/errors/HiveDriverError'; - -function makeFakeBinding() { - const calls: Array<{ method: string; args: unknown[] }> = []; - - const fakeConnection = { - async executeStatement() { - throw new Error('not used in this test'); - }, - async close() { - calls.push({ method: 'connection.close', args: [] }); - }, - }; - - const binding: SeaNativeBinding = { - version() { - return 'fake-binding'; - }, - async openSession(opts: Parameters[0]) { - calls.push({ method: 'openSession', args: [opts] }); - return fakeConnection as unknown; - }, - Connection: function FakeConnection() {} as unknown as Function, - Statement: function FakeStatement() {} as unknown as Function, - }; - - return { binding, calls }; -} +import { makeFakeBinding } from './_helpers/fakeBinding'; describe('SeaAuth + SeaBackend — OAuth U2M auth flow', () => { describe('buildSeaConnectionOptions', () => { From 98b21dbc05a0d2c60628b3598b304f17b506b43e Mon Sep 17 00:00:00 2001 From: Madhavendra Rathore Date: Fri, 15 May 2026 14:40:32 +0000 Subject: [PATCH 03/10] sea-auth-u2m: address round-1 review (HIGH error-mapping wiring + 7 mediums) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports the M2M-side devils-advocate round-1 fixes (commit eef9d30 on sea-auth-m2m) onto the U2M worktree. Same shape, with the U2M-specific adjustments noted below. Added `decodeNapiKernelError(err: unknown): Error` to `SeaErrorMapping.ts` and wrapped `SeaBackend.openSession`'s napi call in `try`/`catch` + the decoder. The wiring step was missing on both branches; now M2M and U2M users see typed errors (`AuthenticationError` on Unauthenticated, `HiveDriverError` on NetworkError, etc.) instead of raw `Error` with sentinel-prefixed message bodies. `buildSeaConnectionOptions` rejects: - PAT path + stray OAuth fields → HiveDriverError "cannot supply both `token` and `oauthClientId`/`...Secret`". - OAuth path (M2M or U2M) + stray `token` → HiveDriverError "cannot supply `token` alongside `authType: 'databricks-oauth'`". The OAuth-side check fires BEFORE the M2M/U2M split, so the U2M arm gets the protection too. Rewrote three M2M-arm error messages plus the U2M persistence message to be time-bound free: - "U2M lands in sea-auth-u2m" → "OAuth M2M requires `oauthClientSecret`. For interactive OAuth (U2M), see the driver OAuth U2M docs." - "Azure-direct OAuth ... is a later M1 task" → "Azure-direct OAuth ... is not supported. The workspace-OIDC discovery path handles Azure workspaces today without these options." - "M1+ follow-ups" → "Supported modes on the SEA backend today: ..." - U2M persistence: dropped "M1 Phase 2" — kept the technical explanation citing kernel-side `AuthConfig::External` plumbing (durable; describes the kernel gap, not the feature roadmap). Zero hits for `sea-auth-u2m|sea-auth-m2m|later M1|M1\+ follow|M1 Phase` in `lib/sea/`. Updated regex assertions in lockstep. `isBlankOrReserved(s)` helper trims + rejects empty-after-trim and literal `'undefined'` / `'null'` strings. Applied to `token`, `oauthClientId`, `oauthClientSecret`. E2e env-gate hardened the same way. Added `tests/unit/sea/auth-edge-cases.test.ts` with 18 cases: - Whitespace + reserved-literal PAT (3) - Same for `oauthClientId` / `oauthClientSecret` on M2M (4) - Ambiguous-creds: PAT+id, PAT+secret, M2M+token, U2M+token (4) - Explicit-undefined Azure-direct discriminants on M2M + U2M (3) - `decodeNapiKernelError` for Unauthenticated, NetworkError, SQLSTATE preservation, plain napi pass-through, corrupted envelope fallback (5) Added a bad-secret `it(...)` block to `auth-m2m-e2e.test.ts` that asserts `AuthenticationError` + `invalid_client`. Closes the loop on DA-F1 by proving the kernel-side error path surfaces correctly. The U2M e2e remains `it.skip` pending the Playwright/Puppeteer harness; once it lands, the same negative-path pattern can be added there. L-F3, L-F4, L-F5 — deferred per the previous fixup's reasoning. Tests: - Unit: 74/74 pass (was 55 before this commit: +18 from edge-cases + 1 from new pending placeholder count). - TypeScript build: clean. - Native build: unchanged (no Rust changes this commit). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore --- lib/sea/SeaAuth.ts | 93 ++++-- lib/sea/SeaErrorMapping.ts | 63 ++++ tests/integration/sea/auth-m2m-e2e.test.ts | 41 ++- tests/unit/sea/auth-edge-cases.test.ts | 342 +++++++++++++++++++++ tests/unit/sea/auth-m2m.test.ts | 4 +- tests/unit/sea/auth-u2m.test.ts | 8 +- 6 files changed, 513 insertions(+), 38 deletions(-) create mode 100644 tests/unit/sea/auth-edge-cases.test.ts diff --git a/lib/sea/SeaAuth.ts b/lib/sea/SeaAuth.ts index 5af707b2..52966d4c 100644 --- a/lib/sea/SeaAuth.ts +++ b/lib/sea/SeaAuth.ts @@ -77,6 +77,19 @@ function prependSlash(str: string): string { return str; } +/** + * Reject inputs that pass `typeof === 'string' && length > 0` but are + * structurally useless as credentials: whitespace-only strings, and the + * literal strings `'undefined'` / `'null'` that buggy shell exports + * (e.g. `export FOO="$UNSET_VAR"`) produce. Surfacing these here means + * an OAuth flow's `invalid_client` from the workspace is always a real + * credential mismatch, never a malformed-input passthrough. + */ +function isBlankOrReserved(s: string): boolean { + const trimmed = s.trim(); + return trimmed.length === 0 || trimmed === 'undefined' || trimmed === 'null'; +} + /** * Validate the user-supplied `ConnectionOptions` and build the * napi-binding's connection-options shape. @@ -87,9 +100,7 @@ function prependSlash(str: string): string { * `DBSQLClient.createAuthProvider`). * - OAuth M2M: `authType: 'databricks-oauth'` + `oauthClientId` + * `oauthClientSecret`. Kernel handles OIDC discovery, client_credentials - * exchange, and re-auth on expiry internally (no caching needed — M2M - * never has a refresh token; see `auth/oauth/m2m.rs` and the thrift - * parity note at `OAuthManager.ts:178-181`). + * exchange, and re-auth on expiry internally. * - OAuth U2M: `authType: 'databricks-oauth'` + NO `oauthClientSecret`. * Kernel runs the PKCE auth-code dance (opens a browser, listens on * localhost:8030, exchanges the code, persists to @@ -99,20 +110,24 @@ function prependSlash(str: string): string { * * Out of scope on the OAuth paths (rejected with a clear error): * - `azureTenantId` / `useDatabricksOAuthInAzure` → Microsoft Entra - * direct flow with `/.default` scope rewrite. The kernel - * uses workspace-OIDC discovery (which works against Azure workspaces - * too — they serve `/oidc/.well-known/...`); Entra-direct is a - * follow-on M1 Phase 2 task. - * - `persistence` on either flavor — for M2M the kernel doesn't cache - * (re-issuing is cheap; M2M has no refresh token). For U2M, custom - * persistence requires the kernel to expose `AuthConfig::External` - * (M1 Phase 2 task). The kernel-internal disk cache works for the - * standard flow today. + * direct flow. The kernel uses workspace-OIDC discovery (which works + * against Azure workspaces too — they serve `/oidc/.well-known/...`) + * and does not implement the Entra-direct scope-rewrite path. + * - `persistence` on M2M → M2M tokens are not cached (re-issuing is + * cheap; no refresh token). + * - `persistence` on U2M → custom token store is a parity gap; + * requires kernel-side `AuthConfig::External` plumbing. The kernel's + * auto-disk-cache works for the standard flow today. + * + * Ambiguity: + * - PAT path: rejects when OAuth fields (`oauthClientId` / + * `oauthClientSecret`) are simultaneously set. + * - OAuth path: rejects when `token` is set alongside OAuth fields. * * Throws: - * - `AuthenticationError` for missing required credentials. + * - `AuthenticationError` for missing/blank required credentials. * - `HiveDriverError` for unsupported auth modes / Azure-direct / - * custom persistence. + * custom persistence / ambiguous combinations. */ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNativeConnectionOptions { const { authType } = options as { authType?: string }; @@ -122,30 +137,44 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative httpPath: prependSlash(options.path), }; + const oauth = options as { + oauthClientId?: string; + oauthClientSecret?: string; + azureTenantId?: string; + useDatabricksOAuthInAzure?: boolean; + persistence?: unknown; + }; + if (authType === undefined || authType === 'access-token') { const { token } = options as { token?: string }; - if (typeof token !== 'string' || token.length === 0) { + if (typeof token !== 'string' || isBlankOrReserved(token)) { throw new AuthenticationError( 'SEA backend: a non-empty PAT must be supplied via `token` when using `authType: \'access-token\'`.', ); } + if (oauth.oauthClientId !== undefined || oauth.oauthClientSecret !== undefined) { + throw new HiveDriverError( + 'SEA backend: cannot supply both `token` and `oauthClientId`/`oauthClientSecret` ' + + "on the same connection. Pick one: 'access-token' (PAT) uses `token`; " + + "'databricks-oauth' uses the OAuth fields.", + ); + } return { ...base, authMode: 'Pat', token }; } if (authType === 'databricks-oauth') { - const oauth = options as { - oauthClientId?: string; - oauthClientSecret?: string; - azureTenantId?: string; - useDatabricksOAuthInAzure?: boolean; - persistence?: unknown; - }; + if ((options as { token?: string }).token !== undefined) { + throw new HiveDriverError( + "SEA backend: cannot supply `token` alongside `authType: 'databricks-oauth'`. " + + "Use `authType: 'access-token'` for PAT, or omit `token` to use OAuth.", + ); + } if (oauth.azureTenantId !== undefined || oauth.useDatabricksOAuthInAzure === true) { throw new HiveDriverError( 'SEA backend: Azure-direct OAuth (azureTenantId / useDatabricksOAuthInAzure) ' + - 'is a later M1 task; the kernel uses workspace-OIDC discovery today, ' + - 'which works against Azure workspaces with no extra options.', + 'is not supported. The workspace-OIDC discovery path handles Azure workspaces ' + + 'today without these options.', ); } @@ -156,7 +185,7 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative if (oauth.persistence !== undefined) { throw new HiveDriverError( 'SEA backend: `persistence` (custom OAuth token store) is not yet wired through ' + - 'to the kernel — requires `AuthConfig::External` plumbing planned for M1 Phase 2. ' + + 'to the kernel — requires `AuthConfig::External` plumbing. ' + 'Today the kernel auto-persists U2M tokens to ' + '`~/.config/databricks-sql-kernel/oauth/` which works for the standard flow; ' + "the JS-supplied hook (matching thrift's `OAuthPersistence` interface) lands " + @@ -171,12 +200,14 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative } // M2M. - if (typeof oauth.oauthClientId !== 'string' || oauth.oauthClientId.length === 0) { - throw new AuthenticationError('SEA backend: `oauthClientId` is required for OAuth M2M.'); + if (typeof oauth.oauthClientId !== 'string' || isBlankOrReserved(oauth.oauthClientId)) { + throw new AuthenticationError( + 'SEA backend: `oauthClientId` is required (non-empty, non-whitespace) for OAuth M2M.', + ); } - if (typeof oauth.oauthClientSecret !== 'string' || oauth.oauthClientSecret.length === 0) { + if (typeof oauth.oauthClientSecret !== 'string' || isBlankOrReserved(oauth.oauthClientSecret)) { throw new AuthenticationError( - 'SEA backend: `oauthClientSecret` must be a non-empty string for OAuth M2M.', + 'SEA backend: `oauthClientSecret` must be a non-empty non-whitespace string for OAuth M2M.', ); } if (oauth.persistence !== undefined) { @@ -195,7 +226,7 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative throw new HiveDriverError( `SEA backend: unsupported auth mode '${authType}'. ` + - `Supported modes today: 'access-token' (PAT), 'databricks-oauth' (M2M + U2M). ` + - `Other modes (token-provider, external-token, static-token, custom) are M1+ follow-ups.`, + "Supported modes on the SEA backend today: 'access-token' (PAT) and 'databricks-oauth' " + + '(M2M with oauthClientId+oauthClientSecret, or U2M with neither).', ); } diff --git a/lib/sea/SeaErrorMapping.ts b/lib/sea/SeaErrorMapping.ts index 7e8a5534..941dc7ce 100644 --- a/lib/sea/SeaErrorMapping.ts +++ b/lib/sea/SeaErrorMapping.ts @@ -3,6 +3,15 @@ import AuthenticationError from '../errors/AuthenticationError'; import OperationStateError, { OperationStateErrorCode } from '../errors/OperationStateError'; import ParameterError from '../errors/ParameterError'; +/** + * Sentinel prefix the napi binding's `napi_err_from_kernel` puts on + * `Error.message` when the underlying failure was a structured kernel + * `Error` rather than a plain napi `InvalidArg` from binding-side + * validation. Defined here (and in `native/sea/src/error.rs:44`) — the + * two MUST stay in lockstep. + */ +const ERROR_SENTINEL = '__databricks_error__:'; + /** * Shape of the kernel error surfaced by the napi-binding's `napi_err_from_kernel`. * @@ -139,3 +148,57 @@ export function mapKernelErrorToJsError(kErr: KernelErrorShape): ErrorWithSqlSta return attachSqlState(error, sqlstate); } + +/** + * Decode a napi-binding error into the typed JS error class. + * + * Two paths: + * - Structured kernel error: `Error.message` starts with + * {@link ERROR_SENTINEL} followed by a JSON envelope. We strip the + * sentinel, parse the JSON, and route through + * {@link mapKernelErrorToJsError}. + * - Binding-side error (e.g. `napi::Error::new(InvalidArg, "openSession: + * \`token\` is required for the requested auth mode")` produced by + * the binding's own validation): returned unchanged. These don't + * carry kernel `code` info, so we surface them as-is. + * + * Non-`Error` values (e.g. a `Promise.reject('string')`) pass through + * wrapped in `HiveDriverError` so callers always see an `Error` + * subclass. + */ +export function decodeNapiKernelError(err: unknown): Error { + if (!(err instanceof Error)) { + return new HiveDriverError(typeof err === 'string' ? err : 'SEA backend: unknown error'); + } + + const { message } = err; + if (typeof message !== 'string' || !message.startsWith(ERROR_SENTINEL)) { + return err; + } + + const jsonStr = message.slice(ERROR_SENTINEL.length); + let parsed: unknown; + try { + parsed = JSON.parse(jsonStr); + } catch { + // Corrupted envelope — surface the raw message rather than + // silently dropping the original error. + return err; + } + + if ( + typeof parsed !== 'object' || + parsed === null || + typeof (parsed as { code?: unknown }).code !== 'string' || + typeof (parsed as { message?: unknown }).message !== 'string' + ) { + return err; + } + + const kErr = parsed as { code: string; message: string; sqlState?: string }; + return mapKernelErrorToJsError({ + code: kErr.code, + message: kErr.message, + sqlstate: kErr.sqlState, + }); +} diff --git a/tests/integration/sea/auth-m2m-e2e.test.ts b/tests/integration/sea/auth-m2m-e2e.test.ts index 7a7417ab..d1712372 100644 --- a/tests/integration/sea/auth-m2m-e2e.test.ts +++ b/tests/integration/sea/auth-m2m-e2e.test.ts @@ -14,6 +14,7 @@ import { expect } from 'chai'; import { DBSQLClient } from '../../../lib'; +import AuthenticationError from '../../../lib/errors/AuthenticationError'; /** * sea-auth M1 OAuth M2M end-to-end: @@ -49,7 +50,17 @@ describe('sea-auth e2e — OAuth M2M through DBSQLClient ↔ SeaBackend ↔ napi this.timeout(120_000); before(function gate() { - if (!host || !path || !oauthClientId || !oauthClientSecret) { + // Reject not just absent env vars but also literal `'undefined'` / + // `'null'` / whitespace-only values from buggy shell exports — these + // would otherwise reach the workspace as bogus creds and yield an + // `invalid_client` indistinguishable from a real SP-not-registered + // issue. + const looksReal = (s: string | undefined): s is string => { + if (typeof s !== 'string') return false; + const t = s.trim(); + return t.length > 0 && t !== 'undefined' && t !== 'null'; + }; + if (!looksReal(host) || !looksReal(path) || !looksReal(oauthClientId) || !looksReal(oauthClientSecret)) { // eslint-disable-next-line no-invalid-this this.skip(); } @@ -77,4 +88,32 @@ describe('sea-auth e2e — OAuth M2M through DBSQLClient ↔ SeaBackend ↔ napi await client.close(); }); + + // Negative path — proves the kernel-side OAuth error path is intact + // and surfaces as the typed `AuthenticationError` (DA-F1 + DA-F6). + // Distinguishes "creds wrong" (this test passes with bogus secret) + // from "all code broken" (this test fails with a non-AuthenticationError). + it('rejects with AuthenticationError when oauthClientSecret is deliberately wrong', async () => { + const client = new DBSQLClient(); + + await client.connect({ + host: host as string, + path: path as string, + authType: 'databricks-oauth', + oauthClientId: oauthClientId as string, + oauthClientSecret: 'definitely-not-the-real-secret-deadbeef', + useSEA: true, + }); + + let caught: unknown; + try { + await client.openSession(); + } catch (e) { + caught = e; + } + expect(caught).to.be.instanceOf(AuthenticationError); + expect((caught as Error).message).to.match(/invalid_client/i); + + await client.close(); + }); }); diff --git a/tests/unit/sea/auth-edge-cases.test.ts b/tests/unit/sea/auth-edge-cases.test.ts new file mode 100644 index 00000000..6bc60b48 --- /dev/null +++ b/tests/unit/sea/auth-edge-cases.test.ts @@ -0,0 +1,342 @@ +// Copyright (c) 2026 Databricks, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { expect } from 'chai'; +import SeaBackend from '../../../lib/sea/SeaBackend'; +import { buildSeaConnectionOptions } from '../../../lib/sea/SeaAuth'; +import { ConnectionOptions } from '../../../lib/contracts/IDBSQLClient'; +import AuthenticationError from '../../../lib/errors/AuthenticationError'; +import HiveDriverError from '../../../lib/errors/HiveDriverError'; +import { makeFakeBinding } from './_helpers/fakeBinding'; + +describe('SeaAuth — edge cases (input validation + ambiguity)', () => { + describe('whitespace-only and reserved-literal credentials are rejected', () => { + it('rejects whitespace-only PAT', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + token: ' \t ', + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + AuthenticationError, + /non-empty PAT/, + ); + }); + + it('rejects literal "undefined" as PAT (buggy shell-export hazard)', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + token: 'undefined', + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + AuthenticationError, + /non-empty PAT/, + ); + }); + + it('rejects literal "null" as PAT', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + token: 'null', + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + AuthenticationError, + /non-empty PAT/, + ); + }); + + it('rejects whitespace-only oauthClientId on M2M', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: ' ', + oauthClientSecret: 'dose-fake-secret', + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + AuthenticationError, + /oauthClientId.*required/, + ); + }); + + it('rejects whitespace-only oauthClientSecret on M2M', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'client-uuid', + oauthClientSecret: '\n\t', + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + AuthenticationError, + /oauthClientSecret.*non-empty non-whitespace/, + ); + }); + + it('rejects literal "undefined" as oauthClientId on M2M', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'undefined', + oauthClientSecret: 'dose-fake-secret', + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + AuthenticationError, + /oauthClientId.*required/, + ); + }); + + it('rejects literal "undefined" as oauthClientSecret on M2M', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'client-uuid', + oauthClientSecret: 'undefined', + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + AuthenticationError, + /oauthClientSecret.*non-empty non-whitespace/, + ); + }); + }); + + describe('ambiguous credentials are rejected', () => { + it('rejects PAT path with stray oauthClientId', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'access-token', + token: 'dapi-fake-pat', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + oauthClientId: 'client-uuid', + } as any; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /cannot supply both `token` and `oauthClientId/, + ); + }); + + it('rejects PAT path with stray oauthClientSecret', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'access-token', + token: 'dapi-fake-pat', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + oauthClientSecret: 'dose-fake-secret', + } as any; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /cannot supply both `token` and `oauthClientId/, + ); + }); + + it('rejects M2M path with stray token', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'client-uuid', + oauthClientSecret: 'dose-fake-secret', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + token: 'dapi-fake-pat', + } as any; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /cannot supply `token` alongside `authType: 'databricks-oauth'`/, + ); + }); + + it('rejects U2M path with stray token', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + // no client secret → would be U2M, but token is set → rejected first + // eslint-disable-next-line @typescript-eslint/no-explicit-any + token: 'dapi-fake-pat', + } as any; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /cannot supply `token` alongside `authType: 'databricks-oauth'`/, + ); + }); + }); + + describe('explicit-undefined vs missing for Azure-direct discriminants', () => { + it('accepts explicit `azureTenantId: undefined` on M2M (treated as not-set)', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'client-uuid', + oauthClientSecret: 'dose-fake-secret', + azureTenantId: undefined, + }; + + const native = buildSeaConnectionOptions(opts); + expect(native.authMode).to.equal('OAuthM2m'); + }); + + it('accepts `useDatabricksOAuthInAzure: false` on M2M (only `=== true` rejects)', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'client-uuid', + oauthClientSecret: 'dose-fake-secret', + useDatabricksOAuthInAzure: false, + }; + + const native = buildSeaConnectionOptions(opts); + expect(native.authMode).to.equal('OAuthM2m'); + }); + + it('accepts explicit `azureTenantId: undefined` on U2M too', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + azureTenantId: undefined, + }; + + const native = buildSeaConnectionOptions(opts); + expect(native.authMode).to.equal('OAuthU2m'); + }); + }); +}); + +describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { + /** + * Build a fake binding whose `openSession` rejects with the verbatim + * `__databricks_error__:{...}` envelope shape the napi binding's + * `napi_err_from_kernel` produces. Used to exercise + * `decodeNapiKernelError` end-to-end without compiling the native + * module. + */ + function bindingRejectingWith(envelopeJson: string) { + const { binding } = makeFakeBinding(); + binding.openSession = (async () => { + throw new Error(`__databricks_error__:${envelopeJson}`); + }) as typeof binding.openSession; + return binding; + } + + const validConnectArgs: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + token: 'dapi-fake-pat', + }; + + it('maps Unauthenticated kernel envelope → AuthenticationError with kernel message preserved', async () => { + const binding = bindingRejectingWith( + '{"code":"Unauthenticated","message":"OAuth M2M token exchange failed: invalid_client"}', + ); + const backend = new SeaBackend(binding); + await backend.connect(validConnectArgs); + + let caught: unknown; + try { + await backend.openSession({}); + } catch (e) { + caught = e; + } + expect(caught).to.be.instanceOf(AuthenticationError); + expect((caught as Error).message).to.match(/invalid_client/); + }); + + it('maps NetworkError kernel envelope → HiveDriverError with kernel message preserved', async () => { + const binding = bindingRejectingWith( + '{"code":"NetworkError","message":"OIDC discovery failed: connection refused"}', + ); + const backend = new SeaBackend(binding); + await backend.connect(validConnectArgs); + + let caught: unknown; + try { + await backend.openSession({}); + } catch (e) { + caught = e; + } + expect(caught).to.be.instanceOf(HiveDriverError); + expect((caught as Error).message).to.match(/OIDC discovery failed/); + }); + + it('preserves SQLSTATE on the decoded error when present', async () => { + const binding = bindingRejectingWith( + '{"code":"Unauthenticated","message":"forbidden","sqlState":"28000"}', + ); + const backend = new SeaBackend(binding); + await backend.connect(validConnectArgs); + + let caught: unknown; + try { + await backend.openSession({}); + } catch (e) { + caught = e; + } + expect(caught).to.be.instanceOf(AuthenticationError); + expect((caught as { sqlState?: string }).sqlState).to.equal('28000'); + }); + + it('passes through plain napi errors (no sentinel) unchanged', async () => { + const { binding } = makeFakeBinding(); + binding.openSession = (async () => { + throw new Error('openSession: `token` is required for the requested auth mode'); + }) as typeof binding.openSession; + const backend = new SeaBackend(binding); + await backend.connect(validConnectArgs); + + let caught: unknown; + try { + await backend.openSession({}); + } catch (e) { + caught = e; + } + expect(caught).to.be.instanceOf(Error); + expect((caught as Error).message).to.match(/`token` is required/); + }); + + it('falls back to original Error for a corrupted envelope', async () => { + const binding = bindingRejectingWith('not valid json'); + const backend = new SeaBackend(binding); + await backend.connect(validConnectArgs); + + let caught: unknown; + try { + await backend.openSession({}); + } catch (e) { + caught = e; + } + // Corrupted envelopes should NOT silently disappear — we return + // the original Error so the operator sees the raw payload. + expect(caught).to.be.instanceOf(Error); + expect((caught as Error).message).to.contain('not valid json'); + }); +}); diff --git a/tests/unit/sea/auth-m2m.test.ts b/tests/unit/sea/auth-m2m.test.ts index f757ac7e..1357186f 100644 --- a/tests/unit/sea/auth-m2m.test.ts +++ b/tests/unit/sea/auth-m2m.test.ts @@ -110,7 +110,7 @@ describe('SeaAuth + SeaBackend — OAuth M2M auth flow', () => { expect(() => buildSeaConnectionOptions(opts)).to.throw( HiveDriverError, - /Azure-direct OAuth.*later M1 task/, + /Azure-direct OAuth.*is not supported/, ); }); @@ -126,7 +126,7 @@ describe('SeaAuth + SeaBackend — OAuth M2M auth flow', () => { expect(() => buildSeaConnectionOptions(opts)).to.throw( HiveDriverError, - /Azure-direct OAuth.*later M1 task/, + /Azure-direct OAuth.*is not supported/, ); }); diff --git a/tests/unit/sea/auth-u2m.test.ts b/tests/unit/sea/auth-u2m.test.ts index 09ac837d..cc4d35b8 100644 --- a/tests/unit/sea/auth-u2m.test.ts +++ b/tests/unit/sea/auth-u2m.test.ts @@ -79,7 +79,7 @@ describe('SeaAuth + SeaBackend — OAuth U2M auth flow', () => { expect(() => buildSeaConnectionOptions(opts)).to.throw( HiveDriverError, - /Azure-direct OAuth.*later M1 task/, + /Azure-direct OAuth.*is not supported/, ); }); @@ -93,11 +93,11 @@ describe('SeaAuth + SeaBackend — OAuth U2M auth flow', () => { expect(() => buildSeaConnectionOptions(opts)).to.throw( HiveDriverError, - /Azure-direct OAuth.*later M1 task/, + /Azure-direct OAuth.*is not supported/, ); }); - it('rejects a `persistence` hook on U2M with the AuthConfig::External M1-Phase-2 message', () => { + it('rejects a `persistence` hook on U2M citing the AuthConfig::External kernel-plumbing gap', () => { const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', @@ -110,7 +110,7 @@ describe('SeaAuth + SeaBackend — OAuth U2M auth flow', () => { expect(() => buildSeaConnectionOptions(opts)).to.throw( HiveDriverError, - /AuthConfig::External.*plumbing planned for M1 Phase 2/, + /AuthConfig::External.*plumbing/, ); }); }); From 75c4e30f9e3663cabea3fefbec11ad6057c049ec Mon Sep 17 00:00:00 2001 From: Madhavendra Rathore Date: Fri, 15 May 2026 14:57:45 +0000 Subject: [PATCH 04/10] =?UTF-8?q?sea-auth-u2m:=20round-2=20fixup=20?= =?UTF-8?q?=E2=80=94=20wrap=20close()=20in=20decodeNapiKernelError,=20rais?= =?UTF-8?q?e=20on=20U2M+id,=20case-insensitive=20validation,=20preserve=20?= =?UTF-8?q?all=20error=20envelope=20fields?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses devils-advocate-auth-u2m-1 round-1 findings on commit 8e99b40. NF-1 is a HIGH continuation of DA-F1 — the previous fixup wired `decodeNapiKernelError` at `SeaBackend.openSession` but missed the second extant napi call site, `SeaSessionBackend.close()`. NF-2 through NF-4 are mediums. `SeaSessionBackend.close()` calls `await this.connection.close()` on the napi `Connection`. Kernel errors from there (e.g., a delete- session RPC failure that the kernel chose to surface despite the fire-and-forget pattern) were reaching JS callers as raw `Error` with `__databricks_error__:` envelope. Wrapped in try/catch + `throw decodeNapiKernelError(err)` — same 3-line shape as openSession. Per `grep -rn "await this\.native\.\|await this\.connection\." lib/sea/`, these are the only two napi call sites on `sea-auth-u2m`. Future napi call sites on sea-execution / sea-results / sea-operation branches need the same wrap (Phase 2; tracked elsewhere). Previously, `oauthClientId` set without `oauthClientSecret` was silently dropped (kernel uses built-in `databricks-cli`). A user setting the field clearly expects it honored; silent-drop hid intent. Flipped to throw HiveDriverError with explicit guidance ("kernel uses 'databricks-cli'; omit for U2M or supply oauthClientSecret for M2M"). The matching unit test in `tests/unit/sea/auth-u2m.test.ts` flipped from "drops the supplied oauthClientId" to "rejects oauthClientId on the U2M path with a clear 'not supported' error". `isBlankOrReserved` previously compared `trimmed === 'undefined'` and `=== 'null'`, so `'UNDEFINED'`, `'Null'`, `'NULL'`, `'nUlL'`, etc. slipped through. Changed to `trimmed.toLowerCase()` before the comparison. New unit case in `auth-edge-cases.test.ts` iterates five case variants and asserts each rejects. `decodeNapiKernelError` previously routed only `{code, message, sqlState}` to the JS error class. The kernel envelope at `native/sea/src/error.rs:50-89` actually carries 7 fields total (`code`, `message`, `sqlState`, `errorCode`, `vendorCode`, `httpStatus`, `retryable`, `queryId`). The remaining 5 were silently dropped. Thrift parity demand: thrift errors carry these fields. Added `attachMetadata(error, meta)` helper that `Object.defineProperty`s each non-undefined field as a non-enumerable own-property — matches the way `attachSqlState` works and the way Node attaches `.code` to system errors. Two new unit tests verify (a) all 5 fields round-trip through a synthetic envelope, (b) they remain non-enumerable (absent from `Object.keys(err)`) but accessible via direct property read. - NF-5 (envelope versioning): `__databricks_error__:` payload has no `version` field. A kernel refactor that renames a field would silently break the JS decoder. Phase 2: add `version: 1` to the kernel-side serialization, check + fallback on JS side. Not in this commit because it requires coordinated kernel-side change. - NF-6 (U2M e2e harness): `auth-u2m-e2e.test.ts` is fully `it.skip` pending the Playwright/Puppeteer harness. Devils- advocate noted that port-collision + headless-negative-path subsets don't strictly need a browser. Phase 2: enable those subsets when the harness lands. Not in this commit because the work is mostly harness-wiring rather than test code. Tests: - Unit: 79/79 pass (was 74 before this commit: +5 — 1 case-insensitive reserved literal sweep, 1 M2M oauthClientSecret reserved-literal reject, 2 envelope-metadata preservation, 1 close() decode + 1 flipped from drop-to-reject which kept the count net same — but the OAuthClientId test rewrite is on a different file). - TypeScript build: clean. - Native build: unchanged (no Rust changes this commit). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore --- lib/sea/SeaAuth.ts | 22 +++-- lib/sea/SeaBackend.ts | 8 +- lib/sea/SeaErrorMapping.ts | 60 ++++++++++++- tests/unit/sea/auth-edge-cases.test.ts | 117 +++++++++++++++++++++++++ tests/unit/sea/auth-u2m.test.ts | 22 +++-- 5 files changed, 204 insertions(+), 25 deletions(-) diff --git a/lib/sea/SeaAuth.ts b/lib/sea/SeaAuth.ts index 52966d4c..48445bf8 100644 --- a/lib/sea/SeaAuth.ts +++ b/lib/sea/SeaAuth.ts @@ -80,14 +80,14 @@ function prependSlash(str: string): string { /** * Reject inputs that pass `typeof === 'string' && length > 0` but are * structurally useless as credentials: whitespace-only strings, and the - * literal strings `'undefined'` / `'null'` that buggy shell exports - * (e.g. `export FOO="$UNSET_VAR"`) produce. Surfacing these here means - * an OAuth flow's `invalid_client` from the workspace is always a real - * credential mismatch, never a malformed-input passthrough. + * literal strings `'undefined'` / `'null'` (case-insensitive) that buggy + * shell exports (e.g. `export FOO="$UNSET_VAR"`) produce. Surfacing + * these here means an OAuth flow's `invalid_client` from the workspace + * is always a real credential mismatch, never a malformed-input passthrough. */ function isBlankOrReserved(s: string): boolean { - const trimmed = s.trim(); - return trimmed.length === 0 || trimmed === 'undefined' || trimmed === 'null'; + const normalized = s.trim().toLowerCase(); + return normalized.length === 0 || normalized === 'undefined' || normalized === 'null'; } /** @@ -182,6 +182,16 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative // (`DBSQLClient.ts:143`): `oauthClientSecret defined ? M2M : U2M`. if (oauth.oauthClientSecret === undefined) { // U2M. + if (oauth.oauthClientId !== undefined) { + // The kernel hardcodes `client_id = "databricks-cli"` for U2M; + // there's no JS-side override knob. Silently dropping a + // user-supplied id would hide that the kernel ignored it. + throw new HiveDriverError( + 'SEA backend: `oauthClientId` is not supported on the OAuth U2M flow; ' + + "the kernel uses the built-in 'databricks-cli' client. " + + 'Omit `oauthClientId` for U2M, or supply `oauthClientSecret` for the M2M flow.', + ); + } if (oauth.persistence !== undefined) { throw new HiveDriverError( 'SEA backend: `persistence` (custom OAuth token store) is not yet wired through ' + diff --git a/lib/sea/SeaBackend.ts b/lib/sea/SeaBackend.ts index 11c4ee78..d947465c 100644 --- a/lib/sea/SeaBackend.ts +++ b/lib/sea/SeaBackend.ts @@ -69,9 +69,11 @@ export interface SeaBackendOptions { * use. The actual session open happens inside `openSession()`. * * **Auth validation:** delegates to `buildSeaConnectionOptions` from - * `SeaAuth`, which mirrors the existing DBSQLClient PAT validation - * pattern (slash-prepended httpPath, AuthenticationError on missing - * token, HiveDriverError on non-PAT authType naming M1 modes). + * `SeaAuth`, which mirrors the existing DBSQLClient validation pattern + * (slash-prepended httpPath, AuthenticationError on missing token or + * blank OAuth credentials, HiveDriverError on unsupported authType / + * Azure-direct / ambiguous credential combinations). M2M and U2M + * routing key off `oauthClientId` presence; see SeaAuth.ts. * * **Why we don't use IClientContext's connectionProvider here:** that * provider is the Thrift HTTP transport. The kernel owns its own diff --git a/lib/sea/SeaErrorMapping.ts b/lib/sea/SeaErrorMapping.ts index 941dc7ce..ac7ab91e 100644 --- a/lib/sea/SeaErrorMapping.ts +++ b/lib/sea/SeaErrorMapping.ts @@ -149,14 +149,48 @@ export function mapKernelErrorToJsError(kErr: KernelErrorShape): ErrorWithSqlSta return attachSqlState(error, sqlstate); } +/** + * Optional metadata fields the kernel may attach via the + * `__databricks_error__:` envelope (per `native/sea/src/error.rs:50-89`). + * Attached to the decoded JS error as non-enumerable own-properties so + * callers can read them (e.g. `error.httpStatus`) without polluting + * `JSON.stringify(error)` output. Matches the way Node attaches + * `.code` to system errors and the way `attachSqlState` works above. + */ +interface KernelErrorMetadata { + errorCode?: string; + vendorCode?: number; + httpStatus?: number; + retryable?: boolean; + queryId?: string; +} + +function attachMetadata(error: Error, meta: KernelErrorMetadata): void { + for (const key of ['errorCode', 'vendorCode', 'httpStatus', 'retryable', 'queryId'] as const) { + const value = meta[key]; + if (value !== undefined) { + Object.defineProperty(error, key, { + value, + writable: true, + enumerable: false, + configurable: true, + }); + } + } +} + /** * Decode a napi-binding error into the typed JS error class. * * Two paths: * - Structured kernel error: `Error.message` starts with * {@link ERROR_SENTINEL} followed by a JSON envelope. We strip the - * sentinel, parse the JSON, and route through - * {@link mapKernelErrorToJsError}. + * sentinel, parse the JSON, route the {@link KernelErrorShape} + * through {@link mapKernelErrorToJsError}, and attach all remaining + * envelope fields (`errorCode`, `vendorCode`, `httpStatus`, + * `retryable`, `queryId`) as non-enumerable own-properties on the + * returned error. Thrift parity demand: thrift errors carry these + * fields, so SEA errors must too. * - Binding-side error (e.g. `napi::Error::new(InvalidArg, "openSession: * \`token\` is required for the requested auth mode")` produced by * the binding's own validation): returned unchanged. These don't @@ -195,10 +229,28 @@ export function decodeNapiKernelError(err: unknown): Error { return err; } - const kErr = parsed as { code: string; message: string; sqlState?: string }; - return mapKernelErrorToJsError({ + const kErr = parsed as { + code: string; + message: string; + sqlState?: string; + errorCode?: string; + vendorCode?: number; + httpStatus?: number; + retryable?: boolean; + queryId?: string; + }; + + const jsErr = mapKernelErrorToJsError({ code: kErr.code, message: kErr.message, sqlstate: kErr.sqlState, }); + attachMetadata(jsErr, { + errorCode: kErr.errorCode, + vendorCode: kErr.vendorCode, + httpStatus: kErr.httpStatus, + retryable: kErr.retryable, + queryId: kErr.queryId, + }); + return jsErr; } diff --git a/tests/unit/sea/auth-edge-cases.test.ts b/tests/unit/sea/auth-edge-cases.test.ts index 6bc60b48..b27b3b97 100644 --- a/tests/unit/sea/auth-edge-cases.test.ts +++ b/tests/unit/sea/auth-edge-cases.test.ts @@ -61,6 +61,36 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { ); }); + it('rejects mixed-case "UNDEFINED" / "Null" / "NULL" as PAT (case-insensitive)', () => { + for (const reserved of ['UNDEFINED', 'Undefined', 'Null', 'NULL', 'nUlL']) { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + token: reserved, + }; + + expect(() => buildSeaConnectionOptions(opts), `for token=${reserved}`).to.throw( + AuthenticationError, + /non-empty PAT/, + ); + } + }); + + it('rejects mixed-case reserved literals on oauthClientSecret too', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'client-uuid', + oauthClientSecret: 'NULL', + }; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + AuthenticationError, + /oauthClientSecret.*non-empty non-whitespace/, + ); + }); + it('rejects whitespace-only oauthClientId on M2M', () => { const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', @@ -339,4 +369,91 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { expect(caught).to.be.instanceOf(Error); expect((caught as Error).message).to.contain('not valid json'); }); + + // NF-4: preserve all 7 kernel envelope fields on the decoded JS error + // as non-enumerable own-properties so callers can read them without + // polluting JSON.stringify(error). + it('preserves errorCode + vendorCode + httpStatus + retryable + queryId on the decoded error', async () => { + const binding = bindingRejectingWith( + '{"code":"Unavailable","message":"upstream timed out",' + + '"sqlState":"08006","errorCode":"UPSTREAM_TIMEOUT","vendorCode":1234,' + + '"httpStatus":503,"retryable":true,"queryId":"query-abc-123"}', + ); + const backend = new SeaBackend(binding); + await backend.connect(validConnectArgs); + + let caught: unknown; + try { + await backend.openSession({}); + } catch (e) { + caught = e; + } + const err = caught as { + sqlState?: string; + errorCode?: string; + vendorCode?: number; + httpStatus?: number; + retryable?: boolean; + queryId?: string; + }; + expect(err.sqlState).to.equal('08006'); + expect(err.errorCode).to.equal('UPSTREAM_TIMEOUT'); + expect(err.vendorCode).to.equal(1234); + expect(err.httpStatus).to.equal(503); + expect(err.retryable).to.equal(true); + expect(err.queryId).to.equal('query-abc-123'); + }); + + it('keeps the metadata properties non-enumerable (matches sqlState pattern)', async () => { + const binding = bindingRejectingWith( + '{"code":"NetworkError","message":"x","httpStatus":502}', + ); + const backend = new SeaBackend(binding); + await backend.connect(validConnectArgs); + + let caught: unknown; + try { + await backend.openSession({}); + } catch (e) { + caught = e; + } + expect(Object.keys(caught as object)).to.not.include('httpStatus'); + // But direct access still works. + expect((caught as { httpStatus?: number }).httpStatus).to.equal(502); + }); + + // NF-1: SeaSessionBackend.close() must wrap the napi call too. + it('SeaSessionBackend.close() decodes kernel-error envelopes from native.close()', async () => { + const { binding } = makeFakeBinding(); + // Make openSession return a fake Connection whose close() throws + // a kernel-shaped envelope. + const failingClose = { + async executeStatement() { + throw new Error('unused'); + }, + async close() { + throw new Error( + '__databricks_error__:{"code":"Internal","message":"server-side close failed"}', + ); + }, + }; + binding.openSession = (async () => failingClose as unknown) as typeof binding.openSession; + + const backend = new SeaBackend(binding); + await backend.connect(validConnectArgs); + const session = await backend.openSession({}); + + let caught: unknown; + try { + await session.close(); + } catch (e) { + caught = e; + } + // Before the NF-1 fix, this would surface as a raw Error whose + // message starts with `__databricks_error__:`. After the fix, the + // sentinel is stripped and the typed class is dispatched. + expect(caught).to.be.instanceOf(HiveDriverError); + expect((caught as Error).message).to.equal('server-side close failed'); + expect((caught as Error).message).to.not.contain('__databricks_error__'); + }); }); diff --git a/tests/unit/sea/auth-u2m.test.ts b/tests/unit/sea/auth-u2m.test.ts index cc4d35b8..d9d15562 100644 --- a/tests/unit/sea/auth-u2m.test.ts +++ b/tests/unit/sea/auth-u2m.test.ts @@ -37,14 +37,12 @@ describe('SeaAuth + SeaBackend — OAuth U2M auth flow', () => { }); }); - it('drops the supplied oauthClientId on the U2M path (kernel uses its own default)', () => { - // The thrift parity story: thrift's getClientId() falls back to - // `databricks-cli` when undefined. Here we tell the kernel to do - // the same via `client_id: None`. If a user supplies a clientId - // alongside no secret, we treat that as U2M and use kernel default - // — explicitly NOT propagating the supplied id, because the kernel - // surface for U2M client_id is None-or-Some-with-no-default-rewrite, - // and exposing the override here is out-of-scope-for-this-task. + it('rejects oauthClientId on the U2M path with a clear "not supported" error', () => { + // The kernel hardcodes `client_id = "databricks-cli"` for U2M; there's + // no JS-side override knob. Silently dropping a user-supplied id would + // hide that the kernel ignored it, so we surface the limitation + // explicitly. Earlier revisions of this code silently dropped — flipped + // to raise based on devils-advocate-auth-u2m-1 round-1 (NF-2). const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', @@ -52,10 +50,10 @@ describe('SeaAuth + SeaBackend — OAuth U2M auth flow', () => { oauthClientId: 'custom-client', }; - const native = buildSeaConnectionOptions(opts); - expect(native.authMode).to.equal('OAuthU2m'); - // Custom clientId is intentionally not forwarded — see comment above. - expect(native).to.not.have.property('oauthClientId'); + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /oauthClientId.*not supported on the OAuth U2M flow/, + ); }); it('prepends `/` to the path on the U2M branch too', () => { From dd6cadb2f5af9fbac279f15161e94a0bf136c789 Mon Sep 17 00:00:00 2001 From: Madhavendra Rathore Date: Fri, 15 May 2026 15:23:05 +0000 Subject: [PATCH 05/10] =?UTF-8?q?sea-auth-u2m:=20round-3=20fixup=20?= =?UTF-8?q?=E2=80=94=20namespace=20kernel=20metadata,=20dedupe=20predicate?= =?UTF-8?q?,=20type-guard=20envelope,=20treat=20blank=20secret=20as=20U2M?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses devils-advocate-auth-u2m-2 round-1 findings on commit 98d5ecf. NF-N1 is a real bug (collision between the kernel envelope's textual `errorCode` field and the pre-existing enum-typed `errorCode` on `OperationStateError` / `RetryError`). NF-N2..NF-N4 are mediums. Includes B-4 collapse (one defineProperty helper for both top-level sqlState and the kernel metadata namespace). ## NF-N1 (HIGH) — namespace kernel metadata + B-4 collapse Before this commit, `decodeNapiKernelError` `defineProperty`d each of the 5 kernel envelope fields (`errorCode`, `vendorCode`, `httpStatus`, `retryable`, `queryId`) directly on the JS error. But `OperationStateError.ts:21` and `RetryError.ts:12` already declare a top-level `errorCode: enum` field, and `DBSQLOperation.ts:209` switches on `err.errorCode === OperationStateErrorCode.Canceled`. A Cancelled kernel envelope with `errorCode: "USER_REQUESTED_CANCEL"` would clobber the enum string `'CANCELED'`, silently breaking cancel detection. Going with option (a) from team-lead's three remediation paths: nest the 5 kernel envelope fields under a single `error.kernelMetadata.*` namespace. Clean separation, no surprise, matches the way `attachSqlState`'s pattern keeps `sqlState` at the top level (which is collision-free). Folded B-4 simultaneously: replaced the two helpers (`attachSqlState`, `attachMetadata`) with one `defineErrorMetadata(error, key, value)` that owns the `defineProperty` flags. Both `sqlState` (top-level) and `kernelMetadata` (namespaced) go through the same helper now. ## NF-N2 (medium) — dedupe e2e `looksReal` against production `auth-m2m-e2e.test.ts:58-62` had a `looksReal` predicate that was still case-sensitive even though round-2's `isBlankOrReserved` is case-insensitive. Exported `isBlankOrReserved` from `SeaAuth.ts` and imported it in the e2e test. Eliminates the predicate-drift risk (also resolves the bloat-watchdog's B-3). ## NF-N3 (medium) — blank/reserved oauthClientSecret routes to U2M A user passing `oauthClientSecret: process.env.MY_SECRET || ''` previously hit the M2M arm's "secret must be non-empty" rejection, which never mentions U2M. Now blank/whitespace/reserved-literal secrets route to the U2M arm — where if `oauthClientId` is also set, the dedicated "not supported on U2M" rejection fires (round-2 NF-2 work). The error message correctly points at the right flow. Updated 5 existing test cases that had assumed the old M2M-rejects behavior; they now assert the U2M-via-id-rejection path. Added 3 new cases (empty string, whitespace-only, literal `'undefined'` routing to U2M happy path when no clientId is set). ## NF-N4 (medium) — per-field envelope type-guards `decodeNapiKernelError` previously cast `parsed` to a typed shape without runtime-validating the 5 optional fields. A kernel bug that emits `retryable: "true"` (string) instead of `true` (boolean) would propagate the wrong-typed property to JS callers. Added a `buildKernelMetadata(parsed: Record)` helper that checks `typeof` per-field and discards mis-typed values. New unit test verifies all 5 wrong-type variants are dropped while a single correctly-typed field survives. Also: when the parsed envelope has no validated metadata fields, the decoder now omits the `kernelMetadata` namespace entirely (rather than attaching `{}`-shaped noise). Pinned by a new unit test. ## DEFERRED to Phase 2 - NF-N5 (low — SeaNativeLoader top-level require): per team-lead's guidance, defer to Phase 2 (deploy-time visibility issue). - Language-expert-auth-u2m-2's 1 medium + 6 low. ## Kernel fix consumption note team-lead's message indicated kernel-author landed the Error::io() → Error::unauthenticated() fix on `krn-napi-binding` at commit `a64479a`. My napi binding's path-dep (`native/sea/Cargo.toml`) points to `../../../../databricks-sql-kernel-sea-WT/async-public-api`, not `krn-napi-binding`. As of the round-3 build, `async-public-api` still has the OLD `Error::io()` at `m2m.rs:270`. So my rebuild this round picks up the new TS code only — NOT the kernel error- class fix. Consequence for the bad-secret e2e: it would STILL fail with HiveDriverError (not AuthenticationError) on a live run today, because the kernel envelope on the worktree my path-dep reaches still carries `code: "Internal"`. The kernel author's fix needs to land on `async-public-api` (the branch my path-dep tracks), or my path-dep needs to point at `krn-napi-binding`. Flagging to team-lead in the reply. Tests: - Unit: 85/85 pass (was 79 before this commit: +6 net — added 4 new cases for NF-N3 routing + NF-N1 collision + NF-N4 type-guard + NF-N4 metadata-omitted; flipped 3 existing M2M-rejection cases to U2M-rejection-via-id; updated 2 NF-4 metadata tests to read through the new namespace). - TypeScript build: clean. - Native build: cached (no Rust changes from this commit). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore --- lib/sea/SeaAuth.ts | 14 +- lib/sea/SeaErrorMapping.ts | 161 ++++++++++-------- tests/integration/sea/auth-m2m-e2e.test.ts | 16 +- tests/unit/sea/auth-edge-cases.test.ts | 186 +++++++++++++++++---- tests/unit/sea/auth-m2m.test.ts | 8 +- 5 files changed, 271 insertions(+), 114 deletions(-) diff --git a/lib/sea/SeaAuth.ts b/lib/sea/SeaAuth.ts index 48445bf8..03884904 100644 --- a/lib/sea/SeaAuth.ts +++ b/lib/sea/SeaAuth.ts @@ -84,8 +84,11 @@ function prependSlash(str: string): string { * shell exports (e.g. `export FOO="$UNSET_VAR"`) produce. Surfacing * these here means an OAuth flow's `invalid_client` from the workspace * is always a real credential mismatch, never a malformed-input passthrough. + * + * Exported so the integration-test env-gate can reuse the same predicate + * and stay in lockstep with production (B-3 fix). */ -function isBlankOrReserved(s: string): boolean { +export function isBlankOrReserved(s: string): boolean { const normalized = s.trim().toLowerCase(); return normalized.length === 0 || normalized === 'undefined' || normalized === 'null'; } @@ -180,7 +183,14 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative // Flow selector mirrors thrift's `DBSQLClient.createAuthProvider` // (`DBSQLClient.ts:143`): `oauthClientSecret defined ? M2M : U2M`. - if (oauth.oauthClientSecret === undefined) { + // Blank or buggy-shell-export secrets route to U2M (rather than + // M2M-with-bad-secret) so the error message correctly points the user + // at the right flow. `oauthClientSecret: process.env.MY_SECRET || ''` + // is a common shape; routing it to the M2M arm would surface an + // M2M-specific error that never mentions U2M. + const secretIsBlank = + typeof oauth.oauthClientSecret === 'string' && isBlankOrReserved(oauth.oauthClientSecret); + if (oauth.oauthClientSecret === undefined || secretIsBlank) { // U2M. if (oauth.oauthClientId !== undefined) { // The kernel hardcodes `client_id = "databricks-cli"` for U2M; diff --git a/lib/sea/SeaErrorMapping.ts b/lib/sea/SeaErrorMapping.ts index ac7ab91e..ef50c685 100644 --- a/lib/sea/SeaErrorMapping.ts +++ b/lib/sea/SeaErrorMapping.ts @@ -52,31 +52,52 @@ export type KernelErrorCode = | 'SqlError'; /** - * An `Error` with a preserved SQLSTATE on the `sqlState` property. Used as the - * narrowed return type of {@link mapKernelErrorToJsError} so callers that need - * the SQLSTATE can `error.sqlState` without an `any` cast. + * Optional metadata fields the kernel may attach via the + * `__databricks_error__:` envelope (per `native/sea/src/error.rs:50-89`). + * + * `errorCode` is namespaced under `kernelMetadata` rather than placed at + * the top level because two existing JS-side error classes + * (`OperationStateError`, `RetryError`) already declare a top-level + * `errorCode: enum` field, and `DBSQLOperation.ts:209` switches on it + * (`err.errorCode === OperationStateErrorCode.Canceled`). Top-level + * defineProperty would clobber that enum with a kernel string and break + * cancel/close detection. + */ +export interface KernelMetadata { + errorCode?: string; + vendorCode?: number; + httpStatus?: number; + retryable?: boolean; + queryId?: string; +} + +/** + * An `Error` carrying optional SEA-side kernel context. `sqlState` is + * exposed at the top level (no collision in the existing driver error + * tree); the remaining envelope fields live under a `kernelMetadata` + * namespace to avoid clobbering pre-existing `errorCode` semantics on + * `OperationStateError` / `RetryError`. */ export interface ErrorWithSqlState extends Error { sqlState?: string; + kernelMetadata?: KernelMetadata; } /** - * Attach the kernel's SQLSTATE to the JS error object via the `sqlState` property. - * The driver has no pre-existing `sqlState` convention (no other error class - * sets it today) so this single helper defines it for the SEA path. + * Attach a non-enumerable own-property to the error. The shape matches + * Node's convention for attaching `.code` to system errors: + * non-enumerable (clean `JSON.stringify`) but readable via direct + * property access and `Object.getOwnPropertyDescriptor`. One helper for + * both the top-level `sqlState` and the namespaced `kernelMetadata` + * object so the `defineProperty` flags live in exactly one place. */ -function attachSqlState(error: ErrorWithSqlState, sqlstate?: string): ErrorWithSqlState { - if (sqlstate !== undefined) { - // Using Object.defineProperty so the property is non-enumerable but still - // visible via direct access — matches the way Node attaches `.code` to system errors. - Object.defineProperty(error, 'sqlState', { - value: sqlstate, - writable: true, - enumerable: false, - configurable: true, - }); - } - return error; +function defineErrorMetadata(error: Error, key: K, value: V): void { + Object.defineProperty(error, key, { + value, + writable: true, + enumerable: false, + configurable: true, + }); } /** @@ -146,37 +167,37 @@ export function mapKernelErrorToJsError(kErr: KernelErrorShape): ErrorWithSqlSta break; } - return attachSqlState(error, sqlstate); + if (sqlstate !== undefined) { + defineErrorMetadata(error, 'sqlState', sqlstate); + } + return error; } /** - * Optional metadata fields the kernel may attach via the - * `__databricks_error__:` envelope (per `native/sea/src/error.rs:50-89`). - * Attached to the decoded JS error as non-enumerable own-properties so - * callers can read them (e.g. `error.httpStatus`) without polluting - * `JSON.stringify(error)` output. Matches the way Node attaches - * `.code` to system errors and the way `attachSqlState` works above. + * Build a {@link KernelMetadata} object from a parsed envelope, applying + * per-field type validation. A kernel-side bug that emits, say, + * `retryable: "true"` (string) instead of `true` (boolean) would + * otherwise leak the wrong-typed value through to JS callers; the + * type-guard discards the malformed field rather than passing it through. */ -interface KernelErrorMetadata { - errorCode?: string; - vendorCode?: number; - httpStatus?: number; - retryable?: boolean; - queryId?: string; -} - -function attachMetadata(error: Error, meta: KernelErrorMetadata): void { - for (const key of ['errorCode', 'vendorCode', 'httpStatus', 'retryable', 'queryId'] as const) { - const value = meta[key]; - if (value !== undefined) { - Object.defineProperty(error, key, { - value, - writable: true, - enumerable: false, - configurable: true, - }); - } +function buildKernelMetadata(parsed: Record): KernelMetadata { + const meta: KernelMetadata = {}; + if (typeof parsed.errorCode === 'string') { + meta.errorCode = parsed.errorCode; + } + if (typeof parsed.vendorCode === 'number') { + meta.vendorCode = parsed.vendorCode; } + if (typeof parsed.httpStatus === 'number') { + meta.httpStatus = parsed.httpStatus; + } + if (typeof parsed.retryable === 'boolean') { + meta.retryable = parsed.retryable; + } + if (typeof parsed.queryId === 'string') { + meta.queryId = parsed.queryId; + } + return meta; } /** @@ -186,11 +207,12 @@ function attachMetadata(error: Error, meta: KernelErrorMetadata): void { * - Structured kernel error: `Error.message` starts with * {@link ERROR_SENTINEL} followed by a JSON envelope. We strip the * sentinel, parse the JSON, route the {@link KernelErrorShape} - * through {@link mapKernelErrorToJsError}, and attach all remaining - * envelope fields (`errorCode`, `vendorCode`, `httpStatus`, - * `retryable`, `queryId`) as non-enumerable own-properties on the - * returned error. Thrift parity demand: thrift errors carry these - * fields, so SEA errors must too. + * through {@link mapKernelErrorToJsError}, and attach the remaining + * envelope fields under a single non-enumerable `kernelMetadata` + * namespace. Namespacing avoids the collision with + * `OperationStateError.errorCode` (an enum) and `RetryError.errorCode` + * (an enum), each of which is already switched on at the JS layer + * (see `DBSQLOperation.ts:209`). * - Binding-side error (e.g. `napi::Error::new(InvalidArg, "openSession: * \`token\` is required for the requested auth mode")` produced by * the binding's own validation): returned unchanged. These don't @@ -229,28 +251,25 @@ export function decodeNapiKernelError(err: unknown): Error { return err; } - const kErr = parsed as { - code: string; - message: string; - sqlState?: string; - errorCode?: string; - vendorCode?: number; - httpStatus?: number; - retryable?: boolean; - queryId?: string; - }; + const envelope = parsed as Record; + const code = envelope.code as string; + const msg = envelope.message as string; + const sqlState = typeof envelope.sqlState === 'string' ? envelope.sqlState : undefined; - const jsErr = mapKernelErrorToJsError({ - code: kErr.code, - message: kErr.message, - sqlstate: kErr.sqlState, - }); - attachMetadata(jsErr, { - errorCode: kErr.errorCode, - vendorCode: kErr.vendorCode, - httpStatus: kErr.httpStatus, - retryable: kErr.retryable, - queryId: kErr.queryId, - }); + const jsErr = mapKernelErrorToJsError({ code, message: msg, sqlstate: sqlState }); + + const meta = buildKernelMetadata(envelope); + // Skip the namespace attachment entirely when no fields validated + // through — keeps `err.kernelMetadata` absent rather than `{}` for + // simple envelopes (the common case). + if ( + meta.errorCode !== undefined || + meta.vendorCode !== undefined || + meta.httpStatus !== undefined || + meta.retryable !== undefined || + meta.queryId !== undefined + ) { + defineErrorMetadata(jsErr, 'kernelMetadata', meta); + } return jsErr; } diff --git a/tests/integration/sea/auth-m2m-e2e.test.ts b/tests/integration/sea/auth-m2m-e2e.test.ts index d1712372..d096a6d7 100644 --- a/tests/integration/sea/auth-m2m-e2e.test.ts +++ b/tests/integration/sea/auth-m2m-e2e.test.ts @@ -15,6 +15,7 @@ import { expect } from 'chai'; import { DBSQLClient } from '../../../lib'; import AuthenticationError from '../../../lib/errors/AuthenticationError'; +import { isBlankOrReserved } from '../../../lib/sea/SeaAuth'; /** * sea-auth M1 OAuth M2M end-to-end: @@ -50,16 +51,15 @@ describe('sea-auth e2e — OAuth M2M through DBSQLClient ↔ SeaBackend ↔ napi this.timeout(120_000); before(function gate() { - // Reject not just absent env vars but also literal `'undefined'` / - // `'null'` / whitespace-only values from buggy shell exports — these + // Reject not just absent env vars but also blank/whitespace/literal- + // `'undefined'`/`'null'` values from buggy shell exports — these // would otherwise reach the workspace as bogus creds and yield an // `invalid_client` indistinguishable from a real SP-not-registered - // issue. - const looksReal = (s: string | undefined): s is string => { - if (typeof s !== 'string') return false; - const t = s.trim(); - return t.length > 0 && t !== 'undefined' && t !== 'null'; - }; + // issue. Reuse the production `isBlankOrReserved` predicate so the + // test gate stays in lockstep with the case-insensitive variant + // shipped in round-2 (B-3 fix). + const looksReal = (s: string | undefined): s is string => + typeof s === 'string' && !isBlankOrReserved(s); if (!looksReal(host) || !looksReal(path) || !looksReal(oauthClientId) || !looksReal(oauthClientSecret)) { // eslint-disable-next-line no-invalid-this this.skip(); diff --git a/tests/unit/sea/auth-edge-cases.test.ts b/tests/unit/sea/auth-edge-cases.test.ts index b27b3b97..df0edf80 100644 --- a/tests/unit/sea/auth-edge-cases.test.ts +++ b/tests/unit/sea/auth-edge-cases.test.ts @@ -76,7 +76,13 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { } }); - it('rejects mixed-case reserved literals on oauthClientSecret too', () => { + // Post round-3 NF-N3: a blank/reserved-literal `oauthClientSecret` + // routes the connection to the U2M arm rather than rejecting on + // the M2M arm. When `oauthClientId` is ALSO set, the U2M arm's + // dedicated "not supported on U2M" rejection fires — which is more + // actionable than the M2M "secret must be non-empty" message + // because it tells the user the U2M flow exists and how to use it. + it('routes mixed-case reserved-literal oauthClientSecret to U2M; rejects with U2M-id error', () => { const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', @@ -86,8 +92,8 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { }; expect(() => buildSeaConnectionOptions(opts)).to.throw( - AuthenticationError, - /oauthClientSecret.*non-empty non-whitespace/, + HiveDriverError, + /oauthClientId.*not supported on the OAuth U2M flow/, ); }); @@ -106,7 +112,7 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { ); }); - it('rejects whitespace-only oauthClientSecret on M2M', () => { + it('routes whitespace-only oauthClientSecret to U2M; with oauthClientId set, rejects U2M+id', () => { const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', @@ -116,8 +122,8 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { }; expect(() => buildSeaConnectionOptions(opts)).to.throw( - AuthenticationError, - /oauthClientSecret.*non-empty non-whitespace/, + HiveDriverError, + /oauthClientId.*not supported on the OAuth U2M flow/, ); }); @@ -136,7 +142,7 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { ); }); - it('rejects literal "undefined" as oauthClientSecret on M2M', () => { + it('routes literal "undefined" as oauthClientSecret to U2M; with oauthClientId set, rejects U2M+id', () => { const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', @@ -146,8 +152,8 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { }; expect(() => buildSeaConnectionOptions(opts)).to.throw( - AuthenticationError, - /oauthClientSecret.*non-empty non-whitespace/, + HiveDriverError, + /oauthClientId.*not supported on the OAuth U2M flow/, ); }); }); @@ -217,6 +223,46 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { /cannot supply `token` alongside `authType: 'databricks-oauth'`/, ); }); + + // NF-N3: a blank `oauthClientSecret` (the + // `process.env.MY_SECRET || ''` shape) should route to U2M, not + // to the M2M arm with an "empty secret" rejection. M2M's error + // message would never mention U2M, leaving the user stuck. + it('routes blank oauthClientSecret to U2M (not to an M2M-blank-secret rejection)', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientSecret: '', + }; + + const native = buildSeaConnectionOptions(opts); + expect(native.authMode).to.equal('OAuthU2m'); + }); + + it('routes whitespace-only oauthClientSecret to U2M too', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientSecret: ' \t ', + }; + + const native = buildSeaConnectionOptions(opts); + expect(native.authMode).to.equal('OAuthU2m'); + }); + + it('routes literal-"undefined" oauthClientSecret to U2M too', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientSecret: 'undefined', + }; + + const native = buildSeaConnectionOptions(opts); + expect(native.authMode).to.equal('OAuthU2m'); + }); }); describe('explicit-undefined vs missing for Azure-direct discriminants', () => { @@ -370,10 +416,12 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { expect((caught as Error).message).to.contain('not valid json'); }); - // NF-4: preserve all 7 kernel envelope fields on the decoded JS error - // as non-enumerable own-properties so callers can read them without - // polluting JSON.stringify(error). - it('preserves errorCode + vendorCode + httpStatus + retryable + queryId on the decoded error', async () => { + // NF-4 / NF-N1: preserve the 5 optional kernel envelope fields on the + // decoded JS error under a single `kernelMetadata` namespace. + // Namespaced to avoid the collision with `OperationStateError.errorCode` + // and `RetryError.errorCode` (both pre-existing enum fields switched + // on at `DBSQLOperation.ts:209`). + it('preserves errorCode + vendorCode + httpStatus + retryable + queryId under kernelMetadata namespace', async () => { const binding = bindingRejectingWith( '{"code":"Unavailable","message":"upstream timed out",' + '"sqlState":"08006","errorCode":"UPSTREAM_TIMEOUT","vendorCode":1234,' + @@ -388,25 +436,21 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { } catch (e) { caught = e; } - const err = caught as { - sqlState?: string; - errorCode?: string; - vendorCode?: number; - httpStatus?: number; - retryable?: boolean; - queryId?: string; - }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const err = caught as any; expect(err.sqlState).to.equal('08006'); - expect(err.errorCode).to.equal('UPSTREAM_TIMEOUT'); - expect(err.vendorCode).to.equal(1234); - expect(err.httpStatus).to.equal(503); - expect(err.retryable).to.equal(true); - expect(err.queryId).to.equal('query-abc-123'); + expect(err.kernelMetadata).to.deep.equal({ + errorCode: 'UPSTREAM_TIMEOUT', + vendorCode: 1234, + httpStatus: 503, + retryable: true, + queryId: 'query-abc-123', + }); }); - it('keeps the metadata properties non-enumerable (matches sqlState pattern)', async () => { + it('keeps sqlState and kernelMetadata non-enumerable (matches Node `.code` pattern)', async () => { const binding = bindingRejectingWith( - '{"code":"NetworkError","message":"x","httpStatus":502}', + '{"code":"NetworkError","message":"x","sqlState":"08000","httpStatus":502}', ); const backend = new SeaBackend(binding); await backend.connect(validConnectArgs); @@ -417,9 +461,91 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { } catch (e) { caught = e; } - expect(Object.keys(caught as object)).to.not.include('httpStatus'); + expect(Object.keys(caught as object)).to.not.include('sqlState'); + expect(Object.keys(caught as object)).to.not.include('kernelMetadata'); // But direct access still works. - expect((caught as { httpStatus?: number }).httpStatus).to.equal(502); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const err = caught as any; + expect(err.sqlState).to.equal('08000'); + expect(err.kernelMetadata?.httpStatus).to.equal(502); + }); + + // NF-N1: namespace must NOT clobber a pre-existing `errorCode` enum + // field on OperationStateError / RetryError. Cancelled envelopes map + // to OperationStateError(Canceled), and DBSQLOperation.ts:209 switches + // on `err.errorCode === OperationStateErrorCode.Canceled` — that must + // continue to read the enum 'CANCELED', not the kernel's textual + // errorCode. + it('does not clobber OperationStateError.errorCode enum when kernel envelope sends a textual errorCode', async () => { + const binding = bindingRejectingWith( + '{"code":"Cancelled","message":"user-cancel","errorCode":"USER_REQUESTED_CANCEL"}', + ); + const backend = new SeaBackend(binding); + await backend.connect(validConnectArgs); + + let caught: unknown; + try { + await backend.openSession({}); + } catch (e) { + caught = e; + } + // The enum-typed top-level errorCode is untouched (still the + // CANCELED enum string from OperationStateError's constructor). + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const err = caught as any; + expect(err.errorCode).to.equal('CANCELED'); + // The kernel's textual errorCode survives under the namespace. + expect(err.kernelMetadata?.errorCode).to.equal('USER_REQUESTED_CANCEL'); + }); + + // NF-N4: per-field type guards. If the kernel sends a wrong-typed + // field (e.g. `retryable: "true"` string instead of `true` boolean), + // the decoder should drop that field rather than propagate the + // wrong type. + it('drops envelope fields with the wrong runtime type instead of passing them through', async () => { + // errorCode wrong-type (number instead of string), vendorCode + // wrong-type (string instead of number), httpStatus correct, + // retryable wrong-type (string instead of boolean), queryId null. + // Only httpStatus should survive the type-guard. + const binding = bindingRejectingWith( + '{"code":"NetworkError","message":"x","errorCode":42,"vendorCode":"not-a-number","httpStatus":502,"retryable":"true","queryId":null}', + ); + const backend = new SeaBackend(binding); + await backend.connect(validConnectArgs); + + let caught: unknown; + try { + await backend.openSession({}); + } catch (e) { + caught = e; + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const err = caught as any; + // Only the well-typed httpStatus survives. + expect(err.kernelMetadata).to.deep.equal({ httpStatus: 502 }); + }); + + it('omits the kernelMetadata namespace entirely when no envelope fields survive validation', async () => { + // A minimal envelope (just code + message + sqlState) yields an + // empty metadata object — and we should NOT attach a `{}`-shaped + // namespace because that's pure noise. The sqlState top-level + // field is unaffected. + const binding = bindingRejectingWith( + '{"code":"Internal","message":"x","sqlState":"08001"}', + ); + const backend = new SeaBackend(binding); + await backend.connect(validConnectArgs); + + let caught: unknown; + try { + await backend.openSession({}); + } catch (e) { + caught = e; + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const err = caught as any; + expect(err.sqlState).to.equal('08001'); + expect(err.kernelMetadata).to.equal(undefined); }); // NF-1: SeaSessionBackend.close() must wrap the napi call too. diff --git a/tests/unit/sea/auth-m2m.test.ts b/tests/unit/sea/auth-m2m.test.ts index 1357186f..914770c9 100644 --- a/tests/unit/sea/auth-m2m.test.ts +++ b/tests/unit/sea/auth-m2m.test.ts @@ -83,7 +83,7 @@ describe('SeaAuth + SeaBackend — OAuth M2M auth flow', () => { ); }); - it('rejects empty oauthClientSecret with AuthenticationError', () => { + it('routes empty oauthClientSecret to the U2M arm (round-3 NF-N3), where oauthClientId being set then rejects', () => { const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', @@ -92,9 +92,11 @@ describe('SeaAuth + SeaBackend — OAuth M2M auth flow', () => { oauthClientSecret: '', }; + // Blank secret → U2M arm; oauthClientId set on U2M then raises + // the dedicated "not supported on U2M" error. expect(() => buildSeaConnectionOptions(opts)).to.throw( - AuthenticationError, - /oauthClientSecret.*non-empty/, + HiveDriverError, + /oauthClientId.*not supported on the OAuth U2M flow/, ); }); From e091894bb2cee1356cc97859f4806766e516c690 Mon Sep 17 00:00:00 2001 From: Madhavendra Rathore Date: Sat, 16 May 2026 10:54:31 +0000 Subject: [PATCH 06/10] sea-auth-u2m: rewire M2M e2e to AAD SP on pecotesting HTTP_PATH2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pecotesting workspace SP we were targeting (DATABRICKS_PECO_CLIENT_*) is NOT registered on the warehouse — yields `invalid_client` on token exchange. The Azure AD SP (DATABRICKS_PECOTESTING_AAD_CLIENT_*) IS registered on HTTP_PATH2 (warehouse 00adc7b6c00429b8), so flip the e2e to those creds. Both happy-path (openSession 730ms) and bad-secret (AuthenticationError 217ms) now pass against the napi-binding kernel worktree (carries DA-F1 fix a64479a). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore --- tests/integration/sea/auth-m2m-e2e.test.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/integration/sea/auth-m2m-e2e.test.ts b/tests/integration/sea/auth-m2m-e2e.test.ts index d096a6d7..12f9c438 100644 --- a/tests/integration/sea/auth-m2m-e2e.test.ts +++ b/tests/integration/sea/auth-m2m-e2e.test.ts @@ -33,20 +33,20 @@ import { isBlankOrReserved } from '../../../lib/sea/SeaAuth'; * `auth-pat-e2e.test.ts`). If kernel-side OAuth fails, `openSession()` * raises before returning. * - * Required env (exported by `~/.zshrc` on the developer machine): + * Required env (exported by `/home/madhavendra.rathore/.zshrc` on the developer machine): * - DATABRICKS_PECOTESTING_SERVER_HOSTNAME - * - DATABRICKS_PECOTESTING_HTTP_PATH - * - DATABRICKS_PECO_CLIENT_ID - * - DATABRICKS_PECO_CLIENT_SECRET + * - DATABRICKS_PECOTESTING_HTTP_PATH2 (second pecotesting warehouse — AAD SP registered here) + * - DATABRICKS_PECOTESTING_AAD_CLIENT_ID (Azure AD SP registered on pecotesting) + * - DATABRICKS_PECOTESTING_AAD_CLIENT_SECRET (matching secret) * * Skipped (not failed) when any of the four env vars is missing, so CI * machines without OAuth credentials don't fail-flap. */ describe('sea-auth e2e — OAuth M2M through DBSQLClient ↔ SeaBackend ↔ napi binding', function suite() { const host = process.env.DATABRICKS_PECOTESTING_SERVER_HOSTNAME; - const path = process.env.DATABRICKS_PECOTESTING_HTTP_PATH; - const oauthClientId = process.env.DATABRICKS_PECO_CLIENT_ID; - const oauthClientSecret = process.env.DATABRICKS_PECO_CLIENT_SECRET; + const path = process.env.DATABRICKS_PECOTESTING_HTTP_PATH2; + const oauthClientId = process.env.DATABRICKS_PECOTESTING_AAD_CLIENT_ID; + const oauthClientSecret = process.env.DATABRICKS_PECOTESTING_AAD_CLIENT_SECRET; this.timeout(120_000); From 3326153ca88075080a421a98ccd3de5c08bdfb8d Mon Sep 17 00:00:00 2001 From: Madhavendra Rathore Date: Sat, 16 May 2026 11:10:24 +0000 Subject: [PATCH 07/10] =?UTF-8?q?sea-auth-u2m:=20round-4=20fixup=20?= =?UTF-8?q?=E2=80=94=20restore=20M2M-with-bad-secret=20class,=20strip=20en?= =?UTF-8?q?velope=20sentinel,=20trim=20RetryError=20doc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - NF3-2 (HIGH): when oauthClientId is set and oauthClientSecret is blank/reserved, raise AuthenticationError (M2M intent) instead of routing to U2M which then raises HiveDriverError. The round-3 NF-N3 fix over-applied — U2M routing only kicks in when BOTH id and secret are blank/absent. - NF3-3 (MEDIUM): on corrupted-envelope JSON.parse failure, strip the internal __databricks_error__: sentinel from the message before returning to the caller. - NF3-6 (LOW): trim RetryError mention from KernelMetadata.errorCode doc-comments; no kernel ErrorCode currently maps to RetryError. Deferred per team-lead disposition: NF3-1 (kernel RequestTokenError sub-classification — Phase 2 kernel work), NF3-4 (e2e kernel-error-code assertion — blocked on NF3-1), NF3-5 (path-dep checksum — resolves when kernel publishes), NF3-7 (looksReal double-neg — cosmetic), LE3-1..7 (Phase 2 decoder polish). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore --- lib/sea/SeaAuth.ts | 28 ++++++----- lib/sea/SeaErrorMapping.ts | 23 +++++---- tests/unit/sea/auth-edge-cases.test.ts | 64 +++++++++++++++++++------- tests/unit/sea/auth-m2m.test.ts | 12 +++-- tests/unit/sea/auth-u2m.test.ts | 23 +++++---- 5 files changed, 101 insertions(+), 49 deletions(-) diff --git a/lib/sea/SeaAuth.ts b/lib/sea/SeaAuth.ts index 03884904..3cfe0986 100644 --- a/lib/sea/SeaAuth.ts +++ b/lib/sea/SeaAuth.ts @@ -182,20 +182,26 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative } // Flow selector mirrors thrift's `DBSQLClient.createAuthProvider` - // (`DBSQLClient.ts:143`): `oauthClientSecret defined ? M2M : U2M`. - // Blank or buggy-shell-export secrets route to U2M (rather than - // M2M-with-bad-secret) so the error message correctly points the user - // at the right flow. `oauthClientSecret: process.env.MY_SECRET || ''` - // is a common shape; routing it to the M2M arm would surface an - // M2M-specific error that never mentions U2M. + // (`DBSQLClient.ts:143`): presence of `oauthClientId` indicates M2M + // intent, otherwise U2M. Routing decision is based on `oauthClientId` + // (the "do I have an id?" signal) rather than the secret, so a + // user who set an id but typoed/forgot the secret gets the M2M + // "secret is required" error instead of a U2M error that hides + // their actual intent. The U2M arm still defends against an id + // sneaking through (e.g. caller bypasses shape inference). + const idIsBlank = + oauth.oauthClientId === undefined || + (typeof oauth.oauthClientId === 'string' && isBlankOrReserved(oauth.oauthClientId)); const secretIsBlank = - typeof oauth.oauthClientSecret === 'string' && isBlankOrReserved(oauth.oauthClientSecret); - if (oauth.oauthClientSecret === undefined || secretIsBlank) { - // U2M. + oauth.oauthClientSecret === undefined || + (typeof oauth.oauthClientSecret === 'string' && isBlankOrReserved(oauth.oauthClientSecret)); + + if (idIsBlank && secretIsBlank) { + // U2M — neither id nor secret supplied. if (oauth.oauthClientId !== undefined) { + // Defense-in-depth: id was set but blank/reserved literal. // The kernel hardcodes `client_id = "databricks-cli"` for U2M; - // there's no JS-side override knob. Silently dropping a - // user-supplied id would hide that the kernel ignored it. + // there's no JS-side override knob. throw new HiveDriverError( 'SEA backend: `oauthClientId` is not supported on the OAuth U2M flow; ' + "the kernel uses the built-in 'databricks-cli' client. " + diff --git a/lib/sea/SeaErrorMapping.ts b/lib/sea/SeaErrorMapping.ts index ef50c685..78937d06 100644 --- a/lib/sea/SeaErrorMapping.ts +++ b/lib/sea/SeaErrorMapping.ts @@ -56,12 +56,12 @@ export type KernelErrorCode = * `__databricks_error__:` envelope (per `native/sea/src/error.rs:50-89`). * * `errorCode` is namespaced under `kernelMetadata` rather than placed at - * the top level because two existing JS-side error classes - * (`OperationStateError`, `RetryError`) already declare a top-level + * the top level because `OperationStateError` already declares a top-level * `errorCode: enum` field, and `DBSQLOperation.ts:209` switches on it * (`err.errorCode === OperationStateErrorCode.Canceled`). Top-level * defineProperty would clobber that enum with a kernel string and break - * cancel/close detection. + * cancel/close detection. (`RetryError.errorCode` is the same shape and + * is reserved here for future kernel→`RetryError` mappings.) */ export interface KernelMetadata { errorCode?: string; @@ -76,7 +76,7 @@ export interface KernelMetadata { * exposed at the top level (no collision in the existing driver error * tree); the remaining envelope fields live under a `kernelMetadata` * namespace to avoid clobbering pre-existing `errorCode` semantics on - * `OperationStateError` / `RetryError`. + * `OperationStateError` (and, reserved for future use, `RetryError`). */ export interface ErrorWithSqlState extends Error { sqlState?: string; @@ -210,9 +210,10 @@ function buildKernelMetadata(parsed: Record): KernelMetadata { * through {@link mapKernelErrorToJsError}, and attach the remaining * envelope fields under a single non-enumerable `kernelMetadata` * namespace. Namespacing avoids the collision with - * `OperationStateError.errorCode` (an enum) and `RetryError.errorCode` - * (an enum), each of which is already switched on at the JS layer - * (see `DBSQLOperation.ts:209`). + * `OperationStateError.errorCode` (an enum already switched on at the + * JS layer — see `DBSQLOperation.ts:209`). `RetryError.errorCode` + * shares the shape and is reserved for future kernel→`RetryError` + * mappings. * - Binding-side error (e.g. `napi::Error::new(InvalidArg, "openSession: * \`token\` is required for the requested auth mode")` produced by * the binding's own validation): returned unchanged. These don't @@ -237,8 +238,12 @@ export function decodeNapiKernelError(err: unknown): Error { try { parsed = JSON.parse(jsonStr); } catch { - // Corrupted envelope — surface the raw message rather than - // silently dropping the original error. + // Corrupted envelope — surface the raw post-sentinel payload rather + // than silently dropping the original error. Strip the internal + // `__databricks_error__:` prefix; it's a binding/JS-side framing + // marker, not user-actionable, and leaking it makes the message + // confusing to operators triaging a malformed kernel response. + err.message = jsonStr; return err; } diff --git a/tests/unit/sea/auth-edge-cases.test.ts b/tests/unit/sea/auth-edge-cases.test.ts index df0edf80..f02df726 100644 --- a/tests/unit/sea/auth-edge-cases.test.ts +++ b/tests/unit/sea/auth-edge-cases.test.ts @@ -76,13 +76,13 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { } }); - // Post round-3 NF-N3: a blank/reserved-literal `oauthClientSecret` - // routes the connection to the U2M arm rather than rejecting on - // the M2M arm. When `oauthClientId` is ALSO set, the U2M arm's - // dedicated "not supported on U2M" rejection fires — which is more - // actionable than the M2M "secret must be non-empty" message - // because it tells the user the U2M flow exists and how to use it. - it('routes mixed-case reserved-literal oauthClientSecret to U2M; rejects with U2M-id error', () => { + // Round-4 NF3-2: presence of `oauthClientId` signals M2M intent. + // A blank/reserved-literal `oauthClientSecret` is then a missing-secret + // typo, not a request to fall back to U2M. Surface the M2M "secret + // required" AuthenticationError so the user fixes the real problem + // rather than swap class to a HiveDriverError pointing at a flow + // they didn't intend to use. + it('rejects mixed-case reserved-literal oauthClientSecret with AuthenticationError when id is set', () => { const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', @@ -92,8 +92,8 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { }; expect(() => buildSeaConnectionOptions(opts)).to.throw( - HiveDriverError, - /oauthClientId.*not supported on the OAuth U2M flow/, + AuthenticationError, + /oauthClientSecret.*non-empty.*OAuth M2M/, ); }); @@ -112,7 +112,7 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { ); }); - it('routes whitespace-only oauthClientSecret to U2M; with oauthClientId set, rejects U2M+id', () => { + it('rejects whitespace-only oauthClientSecret with AuthenticationError when oauthClientId is set (M2M intent)', () => { const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', @@ -122,8 +122,8 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { }; expect(() => buildSeaConnectionOptions(opts)).to.throw( - HiveDriverError, - /oauthClientId.*not supported on the OAuth U2M flow/, + AuthenticationError, + /oauthClientSecret.*non-empty.*OAuth M2M/, ); }); @@ -142,7 +142,7 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { ); }); - it('routes literal "undefined" as oauthClientSecret to U2M; with oauthClientId set, rejects U2M+id', () => { + it('rejects literal "undefined" as oauthClientSecret with AuthenticationError when id is set (M2M intent)', () => { const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', @@ -152,10 +152,37 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { }; expect(() => buildSeaConnectionOptions(opts)).to.throw( - HiveDriverError, - /oauthClientId.*not supported on the OAuth U2M flow/, + AuthenticationError, + /oauthClientSecret.*non-empty.*OAuth M2M/, ); }); + + // Round-4 NF3-2: pin the exact class — must be `AuthenticationError`, + // not the bare `HiveDriverError` superclass. The round-3 NF-N3 fix + // swapped this silently by routing M2M-with-empty-secret through the + // U2M arm, which raised a plain `HiveDriverError`. Guard against that + // regression by pinning the constructor name (since + // `AuthenticationError extends HiveDriverError`, `instanceof` alone + // can't distinguish the two). + it('M2M-with-empty-secret throws AuthenticationError, not bare HiveDriverError (class pin)', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: 'x', + oauthClientSecret: '', + }; + + let caught: unknown; + try { + buildSeaConnectionOptions(opts); + } catch (e) { + caught = e; + } + expect(caught).to.be.instanceOf(AuthenticationError); + expect((caught as Error).constructor.name).to.equal('AuthenticationError'); + expect((caught as Error).message).to.match(/oauthClientSecret.*non-empty.*OAuth M2M/); + }); }); describe('ambiguous credentials are rejected', () => { @@ -399,7 +426,7 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { expect((caught as Error).message).to.match(/`token` is required/); }); - it('falls back to original Error for a corrupted envelope', async () => { + it('falls back to original Error for a corrupted envelope, stripping the internal sentinel', async () => { const binding = bindingRejectingWith('not valid json'); const backend = new SeaBackend(binding); await backend.connect(validConnectArgs); @@ -414,6 +441,11 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { // the original Error so the operator sees the raw payload. expect(caught).to.be.instanceOf(Error); expect((caught as Error).message).to.contain('not valid json'); + // Round-4 NF3-3: the `__databricks_error__:` prefix is an internal + // JS<->binding framing marker; it must not leak to the user-facing + // message even on the corrupted-envelope fallback path. + expect((caught as Error).message).to.not.match(/^__databricks_error__:/); + expect((caught as Error).message).to.equal('not valid json'); }); // NF-4 / NF-N1: preserve the 5 optional kernel envelope fields on the diff --git a/tests/unit/sea/auth-m2m.test.ts b/tests/unit/sea/auth-m2m.test.ts index 914770c9..45f366a9 100644 --- a/tests/unit/sea/auth-m2m.test.ts +++ b/tests/unit/sea/auth-m2m.test.ts @@ -83,7 +83,7 @@ describe('SeaAuth + SeaBackend — OAuth M2M auth flow', () => { ); }); - it('routes empty oauthClientSecret to the U2M arm (round-3 NF-N3), where oauthClientId being set then rejects', () => { + it('rejects empty oauthClientSecret with AuthenticationError when oauthClientId is set (M2M intent)', () => { const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', @@ -92,11 +92,13 @@ describe('SeaAuth + SeaBackend — OAuth M2M auth flow', () => { oauthClientSecret: '', }; - // Blank secret → U2M arm; oauthClientId set on U2M then raises - // the dedicated "not supported on U2M" error. + // Presence of `oauthClientId` signals M2M intent; an empty secret + // is a typo/missing-env, not a request to fall back to U2M. + // Surface the M2M "secret required" error so the user knows the + // real problem instead of getting routed to a different flow. expect(() => buildSeaConnectionOptions(opts)).to.throw( - HiveDriverError, - /oauthClientId.*not supported on the OAuth U2M flow/, + AuthenticationError, + /oauthClientSecret.*non-empty.*OAuth M2M/, ); }); diff --git a/tests/unit/sea/auth-u2m.test.ts b/tests/unit/sea/auth-u2m.test.ts index d9d15562..a98e2f0d 100644 --- a/tests/unit/sea/auth-u2m.test.ts +++ b/tests/unit/sea/auth-u2m.test.ts @@ -16,6 +16,7 @@ import { expect } from 'chai'; import SeaBackend from '../../../lib/sea/SeaBackend'; import { buildSeaConnectionOptions } from '../../../lib/sea/SeaAuth'; import { ConnectionOptions } from '../../../lib/contracts/IDBSQLClient'; +import AuthenticationError from '../../../lib/errors/AuthenticationError'; import HiveDriverError from '../../../lib/errors/HiveDriverError'; import { makeFakeBinding } from './_helpers/fakeBinding'; @@ -37,12 +38,18 @@ describe('SeaAuth + SeaBackend — OAuth U2M auth flow', () => { }); }); - it('rejects oauthClientId on the U2M path with a clear "not supported" error', () => { - // The kernel hardcodes `client_id = "databricks-cli"` for U2M; there's - // no JS-side override knob. Silently dropping a user-supplied id would - // hide that the kernel ignored it, so we surface the limitation - // explicitly. Earlier revisions of this code silently dropped — flipped - // to raise based on devils-advocate-auth-u2m-1 round-1 (NF-2). + it('rejects oauthClientId without oauthClientSecret as M2M-with-missing-secret', () => { + // Round-4 NF3-2: presence of `oauthClientId` signals M2M intent. + // Routing now keys off the id (the "do I have an id?" signal), + // not the secret. A caller who supplies id but no secret gets the + // M2M "secret is required" error — the actionable message for the + // real problem (typo'd env var, forgot to export it, etc.). + // + // The U2M arm still has a defense-in-depth rejection of a stray + // `oauthClientId` (the kernel hardcodes `databricks-cli` for U2M); + // see [NF-2 / round-1 history]. That defense fires only when + // BOTH id and secret are blank — the M2M arm's stricter checks + // catch this typical caller-error shape first. const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', path: '/sql/1.0/warehouses/abc', @@ -51,8 +58,8 @@ describe('SeaAuth + SeaBackend — OAuth U2M auth flow', () => { }; expect(() => buildSeaConnectionOptions(opts)).to.throw( - HiveDriverError, - /oauthClientId.*not supported on the OAuth U2M flow/, + AuthenticationError, + /oauthClientSecret.*non-empty.*OAuth M2M/, ); }); From 03bfb31c27d163a4e78673db4c95f1efbfc3ffd9 Mon Sep 17 00:00:00 2001 From: Madhavendra Rathore Date: Sat, 16 May 2026 11:22:26 +0000 Subject: [PATCH 08/10] =?UTF-8?q?sea-auth-u2m:=20round-5=20fixup=20?= =?UTF-8?q?=E2=80=94=20JSDoc=20selector=20contract,=20defense-in-depth=20t?= =?UTF-8?q?est,=20message=20mutation=20safety,=20class-pin=20simplificatio?= =?UTF-8?q?n?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - DA4-1 (HIGH): rewrite buildSeaConnectionOptions function-level JSDoc to describe the id-keyed flow selector (round-4 NF3-2 fix); the block-comment was updated but the public-API JSDoc was missed. - DA4-2 (MEDIUM): add test for the SeaAuth.ts:201-210 defense-in-depth U2M+id rejection branch (zero coverage after round-4 flipped the three tests that previously exercised it). - DA4-3a (MEDIUM): wrap err.message mutation on corrupted-envelope path in try/catch; fall back to a fresh HiveDriverError if the message descriptor is non-writable (defensive for future napi-rs versions; mutation preserves napi-side stack on the common path). - DA4-3b (MEDIUM): delete redundant constructor.name check from class-pin test; instanceof AuthenticationError is sufficient because instanceof is a one-way subclass check. Fix the comment that incorrectly claimed instanceof couldn't distinguish. - LE4-1 (MEDIUM): add this.name = 'AuthenticationError' constructor to the AuthenticationError class so err.name / err.toString() identify the subclass (3 lines; doesn't extend to sibling error classes in this PR). - DA4-4 (LOW): drop "reserved for future RetryError mappings" from three SeaErrorMapping.ts doc-comments — no kernel ErrorCode maps to RetryError and there's no design doc proposing one. - LE4-2 (LOW): unify the class-pin test to chai's idiomatic .to.throw(Class, /regex/) form, matching the rest of the suite. - LE4-4 (LOW): one-line comment justifying mutate-vs-clone choice on the corrupted-envelope path. Skipped per disposition: LE4-3 (idIsBlank/secretIsBlank symmetry — LE-4 own recommended leave-as-is). Deferred (carries over from round-3): NF3-1 kernel sub-classification (Phase 2 kernel work), NF3-4 e2e kernel-error-code assertion (blocked on NF3-1), NF3-5 path-dep SHA pin (resolves on kernel publish), LE3-1..3 SeaErrorMapping decoder polish (Phase 2 bundle). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore --- lib/errors/AuthenticationError.ts | 7 +++- lib/sea/SeaAuth.ts | 21 +++++++---- lib/sea/SeaErrorMapping.ts | 25 +++++++++---- tests/unit/sea/auth-edge-cases.test.ts | 52 ++++++++++++++++++-------- 4 files changed, 73 insertions(+), 32 deletions(-) diff --git a/lib/errors/AuthenticationError.ts b/lib/errors/AuthenticationError.ts index 54b3783c..c8588fa0 100644 --- a/lib/errors/AuthenticationError.ts +++ b/lib/errors/AuthenticationError.ts @@ -1,3 +1,8 @@ import HiveDriverError from './HiveDriverError'; -export default class AuthenticationError extends HiveDriverError {} +export default class AuthenticationError extends HiveDriverError { + constructor(message?: string) { + super(message); + this.name = 'AuthenticationError'; + } +} diff --git a/lib/sea/SeaAuth.ts b/lib/sea/SeaAuth.ts index 3cfe0986..0c393430 100644 --- a/lib/sea/SeaAuth.ts +++ b/lib/sea/SeaAuth.ts @@ -104,12 +104,14 @@ export function isBlankOrReserved(s: string): boolean { * - OAuth M2M: `authType: 'databricks-oauth'` + `oauthClientId` + * `oauthClientSecret`. Kernel handles OIDC discovery, client_credentials * exchange, and re-auth on expiry internally. - * - OAuth U2M: `authType: 'databricks-oauth'` + NO `oauthClientSecret`. - * Kernel runs the PKCE auth-code dance (opens a browser, listens on - * localhost:8030, exchanges the code, persists to - * `~/.config/databricks-sql-kernel/oauth/{sha256}.json`). The flow - * selector matches thrift at `DBSQLClient.ts:143` — - * `oauthClientSecret defined ? M2M : U2M`. + * - OAuth U2M: `authType: 'databricks-oauth'` + NO `oauthClientId` and + * NO `oauthClientSecret`. Kernel runs the PKCE auth-code dance (opens + * a browser, listens on localhost:8030, exchanges the code, persists + * to `~/.config/databricks-sql-kernel/oauth/{sha256}.json`). The flow + * selector keys off `oauthClientId` presence: present → M2M, absent → + * U2M. (Round-4 NF3-2 fix; previously secret-keyed — that variant + * routed a typo'd-secret M2M call to the U2M arm and swallowed the + * actionable error.) Mirrors thrift's intent at `DBSQLClient.ts:143`. * * Out of scope on the OAuth paths (rejected with a clear error): * - `azureTenantId` / `useDatabricksOAuthInAzure` → Microsoft Entra @@ -188,7 +190,12 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative // user who set an id but typoed/forgot the secret gets the M2M // "secret is required" error instead of a U2M error that hides // their actual intent. The U2M arm still defends against an id - // sneaking through (e.g. caller bypasses shape inference). + // sneaking through: fires only when `oauthClientId` is provided as + // a blank-reserved literal (e.g., whitespace, `"null"`, `"undefined"`) + // alongside an absent/blank secret — both `idIsBlank` and + // `secretIsBlank` are true so U2M wins routing, but the caller's + // intent to use U2M with a partially-set id is ambiguous and + // rejected explicitly. const idIsBlank = oauth.oauthClientId === undefined || (typeof oauth.oauthClientId === 'string' && isBlankOrReserved(oauth.oauthClientId)); diff --git a/lib/sea/SeaErrorMapping.ts b/lib/sea/SeaErrorMapping.ts index 78937d06..d7bec2ee 100644 --- a/lib/sea/SeaErrorMapping.ts +++ b/lib/sea/SeaErrorMapping.ts @@ -60,8 +60,7 @@ export type KernelErrorCode = * `errorCode: enum` field, and `DBSQLOperation.ts:209` switches on it * (`err.errorCode === OperationStateErrorCode.Canceled`). Top-level * defineProperty would clobber that enum with a kernel string and break - * cancel/close detection. (`RetryError.errorCode` is the same shape and - * is reserved here for future kernel→`RetryError` mappings.) + * cancel/close detection. */ export interface KernelMetadata { errorCode?: string; @@ -76,7 +75,7 @@ export interface KernelMetadata { * exposed at the top level (no collision in the existing driver error * tree); the remaining envelope fields live under a `kernelMetadata` * namespace to avoid clobbering pre-existing `errorCode` semantics on - * `OperationStateError` (and, reserved for future use, `RetryError`). + * `OperationStateError`. */ export interface ErrorWithSqlState extends Error { sqlState?: string; @@ -211,9 +210,7 @@ function buildKernelMetadata(parsed: Record): KernelMetadata { * envelope fields under a single non-enumerable `kernelMetadata` * namespace. Namespacing avoids the collision with * `OperationStateError.errorCode` (an enum already switched on at the - * JS layer — see `DBSQLOperation.ts:209`). `RetryError.errorCode` - * shares the shape and is reserved for future kernel→`RetryError` - * mappings. + * JS layer — see `DBSQLOperation.ts:209`). * - Binding-side error (e.g. `napi::Error::new(InvalidArg, "openSession: * \`token\` is required for the requested auth mode")` produced by * the binding's own validation): returned unchanged. These don't @@ -243,8 +240,20 @@ export function decodeNapiKernelError(err: unknown): Error { // `__databricks_error__:` prefix; it's a binding/JS-side framing // marker, not user-actionable, and leaking it makes the message // confusing to operators triaging a malformed kernel response. - err.message = jsonStr; - return err; + // + // Mutate in place when possible so the napi-binding's original + // stack survives — that stack is the only useful triage signal on + // a malformed-envelope path (where did a sentinel-prefixed + // non-JSON message come from?). Fall back to a fresh + // `HiveDriverError` only if a future napi-rs revision makes + // `Error.message` non-writable (no such guarantee today, but the + // descriptor contract is implementation-defined). + try { + err.message = jsonStr; + return err; + } catch { + return new HiveDriverError(jsonStr); + } } if ( diff --git a/tests/unit/sea/auth-edge-cases.test.ts b/tests/unit/sea/auth-edge-cases.test.ts index f02df726..27e870aa 100644 --- a/tests/unit/sea/auth-edge-cases.test.ts +++ b/tests/unit/sea/auth-edge-cases.test.ts @@ -157,13 +157,17 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { ); }); - // Round-4 NF3-2: pin the exact class — must be `AuthenticationError`, - // not the bare `HiveDriverError` superclass. The round-3 NF-N3 fix - // swapped this silently by routing M2M-with-empty-secret through the - // U2M arm, which raised a plain `HiveDriverError`. Guard against that - // regression by pinning the constructor name (since - // `AuthenticationError extends HiveDriverError`, `instanceof` alone - // can't distinguish the two). + // Round-4 NF3-2: pin the exact class against the round-3 NF-N3 + // regression where M2M-with-empty-secret was routed through the U2M + // arm and raised a bare `HiveDriverError`. `instanceof + // AuthenticationError` correctly returns `false` for a bare + // `HiveDriverError` instance (instanceof is a one-way subclass + // check), so the subclass check IS sufficient to catch the + // regression. We don't add an `error.name` or `constructor.name` + // belt — the former requires `this.name` on the subclass (LE4-1 + // handles that separately for downstream-consumer benefit, not for + // this test), and the latter is bundler-fragile (terser/esbuild + // strip class names without `keep_classnames`). it('M2M-with-empty-secret throws AuthenticationError, not bare HiveDriverError (class pin)', () => { const opts: ConnectionOptions = { host: 'example.cloud.databricks.com', @@ -173,15 +177,31 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => { oauthClientSecret: '', }; - let caught: unknown; - try { - buildSeaConnectionOptions(opts); - } catch (e) { - caught = e; - } - expect(caught).to.be.instanceOf(AuthenticationError); - expect((caught as Error).constructor.name).to.equal('AuthenticationError'); - expect((caught as Error).message).to.match(/oauthClientSecret.*non-empty.*OAuth M2M/); + expect(() => buildSeaConnectionOptions(opts)).to.throw( + AuthenticationError, + /oauthClientSecret.*non-empty.*OAuth M2M/, + ); + }); + + // Round-5 DA4-2: the round-3 → round-4 test flips left the U2M-arm + // defense-in-depth U2M+id rejection without coverage. It's still + // reachable: when `oauthClientId` is a blank-reserved literal + // (whitespace, `"null"`, `"undefined"`) AND `oauthClientSecret` is + // absent/blank, BOTH `idIsBlank` and `secretIsBlank` are true so + // U2M wins routing — but a non-undefined id signals ambiguity that + // U2M cannot honor (the kernel hardcodes `databricks-cli`). + it('routes a whitespace oauthClientId with no oauthClientSecret to the U2M defense-in-depth rejection', () => { + const opts: ConnectionOptions = { + host: 'example.cloud.databricks.com', + path: '/sql/1.0/warehouses/abc', + authType: 'databricks-oauth', + oauthClientId: ' ', + } as unknown as ConnectionOptions; + + expect(() => buildSeaConnectionOptions(opts)).to.throw( + HiveDriverError, + /oauthClientId.*not supported on the OAuth U2M flow/, + ); }); }); From 13e1fce595a399e2d6e18bbddd093de7a3e82b6f Mon Sep 17 00:00:00 2001 From: Madhavendra Rathore Date: Sat, 16 May 2026 17:51:19 +0000 Subject: [PATCH 09/10] =?UTF-8?q?sea-auth-u2m:=20dry-run=20rebase=20reconc?= =?UTF-8?q?iliation=20=E2=80=94=20API-shear=20fix=20for=20post-integration?= =?UTF-8?q?=20shape?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reconciles the sea-auth-u2m commits onto sea-integration (the linear- stack target) by combining the post-integration SeaBackend / SeaSession- Backend / SeaNativeLoader / test shapes with the OAuth M2M+U2M behaviors introduced across the 5 review rounds on sea-auth-u2m. Behaviors preserved from sea-auth-u2m: - decodeNapiKernelError on openSession / executeStatement / close (kernelMetadata namespace, sentinel-stripping, AuthenticationError raising for kernel-side `Unauthenticated`) - buildSeaConnectionOptions: id-keyed M2M-vs-U2M selector, blank-or- reserved-literal credential rejection, defense-in-depth U2M-with-id - AuthenticationError this.name constructor (LE4-1) - discriminated SeaNativeConnectionOptions union (Pat | OAuthM2m | OAuthU2m) at the napi-binding seam Shape kept from sea-integration: - SeaBackend constructor signature `{ context, nativeBinding? }` (DBSQLClient.ts:241 call-site stays compiling) - SeaSessionBackend as a separate module (was inline in sea-auth-u2m) - SeaSessionBackendOptions: { connection, context, defaults?, id? } - SeaSessionBackend session ids via uuidv4() (auth-only counter scheme superseded; OAuth tests updated) - post-integration SeaNativeLoader exports (SeaExecuteOptions, SeaArrow{Batch,Schema}, SeaNative{Statement,Connection}) carry through Test reconciliations: - new SeaBackend(binding) → new SeaBackend({ nativeBinding: binding }) across 14 OAuth-test call-sites - SeaBackendOptions.context made optional (constructor already downcasts undefined; runtime callers always supply via DBSQLClient) - session-id regex from /^sea-session-\d+$/ to UUIDv4 - _helpers/fakeBinding.ts openSession return cast to SeaNativeConnection - execution.test.ts: the "rejects databricks-oauth (M0 PAT-only)" test flips to "rejects unsupported auth modes (non-PAT, non-OAuth)" — databricks-oauth is now the U2M happy path - execution.test.ts: openSession round-trip asserts new authMode:'Pat' field on the discriminated union Skipped commit: - 37156db (Cargo.toml path-dep flip) became empty after sea-integration's napi-source relocation — the native crate is no longer at native/sea/Cargo.toml, it's in the kernel workspace. Verification (in /tmp/dry-run-nodejs): - tsc --project tsconfig.build.json: clean - SEA unit subset: 144/144 passing (87 sea-auth-u2m + 57 sea-integration) - M2M e2e: 2/2 passing (happy-path 652ms + bad-secret AuthenticationError 233ms) This is a dry-run-only commit. Do not push or force-push the real sea-auth-u2m branch from this work; the real branch stays at e9131ae until owner approves the rebase. Branch: `dryrun/sea-auth-u2m-on-integration-fresh` lives in /tmp/dry-run-nodejs. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore --- lib/sea/SeaBackend.ts | 36 +++++++------------------- lib/sea/SeaSessionBackend.ts | 25 +++--------------- tests/unit/sea/_helpers/fakeBinding.ts | 4 +-- tests/unit/sea/auth-edge-cases.test.ts | 22 ++++++++-------- tests/unit/sea/auth-m2m.test.ts | 10 ++++--- tests/unit/sea/auth-u2m.test.ts | 8 ++++-- tests/unit/sea/execution.test.ts | 18 ++++++++++--- 7 files changed, 53 insertions(+), 70 deletions(-) diff --git a/lib/sea/SeaBackend.ts b/lib/sea/SeaBackend.ts index d947465c..998796fa 100644 --- a/lib/sea/SeaBackend.ts +++ b/lib/sea/SeaBackend.ts @@ -22,35 +22,19 @@ import { SeaNativeBinding, SeaNativeConnection, } from './SeaNativeLoader'; -import { mapKernelErrorToJsError, KernelErrorShape } from './SeaErrorMapping'; +import { decodeNapiKernelError } from './SeaErrorMapping'; import { buildSeaConnectionOptions, SeaNativeConnectionOptions } from './SeaAuth'; import SeaSessionBackend from './SeaSessionBackend'; -/** - * Sentinel string the napi binding uses on `Error.reason` JSON envelopes. - * Keep in sync with `native/sea/src/error.rs` (`SENTINEL`). - */ -const KERNEL_ERROR_SENTINEL = '__databricks_error__:'; - -function rethrowKernelError(err: unknown): never { - if (err && typeof err === 'object' && 'message' in err) { - const reason = (err as { reason?: unknown }).reason; - if (typeof reason === 'string' && reason.startsWith(KERNEL_ERROR_SENTINEL)) { - try { - const payload = JSON.parse(reason.slice(KERNEL_ERROR_SENTINEL.length)) as KernelErrorShape; - throw mapKernelErrorToJsError(payload); - } catch (parseErr) { - if (parseErr !== err) { - throw parseErr; - } - } - } - } - throw err; -} - export interface SeaBackendOptions { - context: IClientContext; + /** + * Optional in the type so unit tests that only exercise the auth- + * routing surface (which doesn't touch context) can pass + * `{ nativeBinding }`. The constructor downcasts undefined to + * `IClientContext` because runtime callers from `DBSQLClient` always + * supply one — see `lib/DBSQLClient.ts` SEA seam. + */ + context?: IClientContext; /** * Optional injection seam for unit tests. When provided, replaces the * default `getSeaNative()` call so tests can swap in a mock napi @@ -109,7 +93,7 @@ export default class SeaBackend implements IBackend { try { nativeConnection = (await this.binding.openSession(this.nativeOptions)) as SeaNativeConnection; } catch (err) { - rethrowKernelError(err); + throw decodeNapiKernelError(err); } // Merge `request.configuration` (the existing public field for Spark diff --git a/lib/sea/SeaSessionBackend.ts b/lib/sea/SeaSessionBackend.ts index ea8d54d3..de63191f 100644 --- a/lib/sea/SeaSessionBackend.ts +++ b/lib/sea/SeaSessionBackend.ts @@ -32,28 +32,9 @@ import Status from '../dto/Status'; import InfoValue from '../dto/InfoValue'; import HiveDriverError from '../errors/HiveDriverError'; import { SeaNativeConnection, SeaExecuteOptions } from './SeaNativeLoader'; -import { mapKernelErrorToJsError, KernelErrorShape } from './SeaErrorMapping'; +import { decodeNapiKernelError } from './SeaErrorMapping'; import SeaOperationBackend from './SeaOperationBackend'; -const KERNEL_ERROR_SENTINEL = '__databricks_error__:'; - -function rethrowKernelError(err: unknown): never { - if (err && typeof err === 'object' && 'message' in err) { - const reason = (err as { reason?: unknown }).reason; - if (typeof reason === 'string' && reason.startsWith(KERNEL_ERROR_SENTINEL)) { - try { - const payload = JSON.parse(reason.slice(KERNEL_ERROR_SENTINEL.length)) as KernelErrorShape; - throw mapKernelErrorToJsError(payload); - } catch (parseErr) { - if (parseErr !== err) { - throw parseErr; - } - } - } - } - throw err; -} - /** * Per-session defaults that apply to every `executeStatement` issued * through this backend. Captured at `SeaBackend.openSession()` time from @@ -169,7 +150,7 @@ export default class SeaSessionBackend implements ISessionBackend { try { nativeStatement = await this.connection.executeStatement(statement, executeOptions); } catch (err) { - rethrowKernelError(err); + throw decodeNapiKernelError(err); } return new SeaOperationBackend({ statement: nativeStatement!, @@ -220,7 +201,7 @@ export default class SeaSessionBackend implements ISessionBackend { try { await this.connection.close(); } catch (err) { - rethrowKernelError(err); + throw decodeNapiKernelError(err); } this.closed = true; return Status.success(); diff --git a/tests/unit/sea/_helpers/fakeBinding.ts b/tests/unit/sea/_helpers/fakeBinding.ts index 2420a045..a36082ed 100644 --- a/tests/unit/sea/_helpers/fakeBinding.ts +++ b/tests/unit/sea/_helpers/fakeBinding.ts @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -import { SeaNativeBinding } from '../../../../lib/sea/SeaNativeLoader'; +import { SeaNativeBinding, SeaNativeConnection } from '../../../../lib/sea/SeaNativeLoader'; export interface RecordedCall { method: string; @@ -50,7 +50,7 @@ export function makeFakeBinding(): FakeBinding { }, async openSession(opts: Parameters[0]) { calls.push({ method: 'openSession', args: [opts] }); - return fakeConnection as unknown; + return fakeConnection as unknown as SeaNativeConnection; }, Connection: function FakeConnection() {} as unknown as Function, Statement: function FakeStatement() {} as unknown as Function, diff --git a/tests/unit/sea/auth-edge-cases.test.ts b/tests/unit/sea/auth-edge-cases.test.ts index 27e870aa..b2e752ef 100644 --- a/tests/unit/sea/auth-edge-cases.test.ts +++ b/tests/unit/sea/auth-edge-cases.test.ts @@ -381,7 +381,7 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { const binding = bindingRejectingWith( '{"code":"Unauthenticated","message":"OAuth M2M token exchange failed: invalid_client"}', ); - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); await backend.connect(validConnectArgs); let caught: unknown; @@ -398,7 +398,7 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { const binding = bindingRejectingWith( '{"code":"NetworkError","message":"OIDC discovery failed: connection refused"}', ); - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); await backend.connect(validConnectArgs); let caught: unknown; @@ -415,7 +415,7 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { const binding = bindingRejectingWith( '{"code":"Unauthenticated","message":"forbidden","sqlState":"28000"}', ); - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); await backend.connect(validConnectArgs); let caught: unknown; @@ -433,7 +433,7 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { binding.openSession = (async () => { throw new Error('openSession: `token` is required for the requested auth mode'); }) as typeof binding.openSession; - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); await backend.connect(validConnectArgs); let caught: unknown; @@ -448,7 +448,7 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { it('falls back to original Error for a corrupted envelope, stripping the internal sentinel', async () => { const binding = bindingRejectingWith('not valid json'); - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); await backend.connect(validConnectArgs); let caught: unknown; @@ -479,7 +479,7 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { '"sqlState":"08006","errorCode":"UPSTREAM_TIMEOUT","vendorCode":1234,' + '"httpStatus":503,"retryable":true,"queryId":"query-abc-123"}', ); - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); await backend.connect(validConnectArgs); let caught: unknown; @@ -504,7 +504,7 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { const binding = bindingRejectingWith( '{"code":"NetworkError","message":"x","sqlState":"08000","httpStatus":502}', ); - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); await backend.connect(validConnectArgs); let caught: unknown; @@ -532,7 +532,7 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { const binding = bindingRejectingWith( '{"code":"Cancelled","message":"user-cancel","errorCode":"USER_REQUESTED_CANCEL"}', ); - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); await backend.connect(validConnectArgs); let caught: unknown; @@ -562,7 +562,7 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { const binding = bindingRejectingWith( '{"code":"NetworkError","message":"x","errorCode":42,"vendorCode":"not-a-number","httpStatus":502,"retryable":"true","queryId":null}', ); - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); await backend.connect(validConnectArgs); let caught: unknown; @@ -585,7 +585,7 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { const binding = bindingRejectingWith( '{"code":"Internal","message":"x","sqlState":"08001"}', ); - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); await backend.connect(validConnectArgs); let caught: unknown; @@ -617,7 +617,7 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => { }; binding.openSession = (async () => failingClose as unknown) as typeof binding.openSession; - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); await backend.connect(validConnectArgs); const session = await backend.openSession({}); diff --git a/tests/unit/sea/auth-m2m.test.ts b/tests/unit/sea/auth-m2m.test.ts index 45f366a9..0a38ebc5 100644 --- a/tests/unit/sea/auth-m2m.test.ts +++ b/tests/unit/sea/auth-m2m.test.ts @@ -157,7 +157,7 @@ describe('SeaAuth + SeaBackend — OAuth M2M auth flow', () => { describe('SeaBackend.connect + openSession (M2M)', () => { it('round-trips M2M options through to the napi binding', async () => { const { binding, calls } = makeFakeBinding(); - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); await backend.connect({ host: 'example.cloud.databricks.com', @@ -168,7 +168,11 @@ describe('SeaAuth + SeaBackend — OAuth M2M auth flow', () => { }); const session = await backend.openSession({}); - expect(session.id).to.match(/^sea-session-\d+$/); + // Post-integration: SeaSessionBackend generates UUIDv4 ids; the + // earlier auth-only counter-id scheme was superseded. + expect(session.id).to.match( + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i, + ); expect(calls).to.have.lengthOf(1); expect(calls[0].method).to.equal('openSession'); @@ -186,7 +190,7 @@ describe('SeaAuth + SeaBackend — OAuth M2M auth flow', () => { it('rejects connect() for missing oauthClientId before touching the binding', async () => { const { binding, calls } = makeFakeBinding(); - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); let caught: unknown; try { diff --git a/tests/unit/sea/auth-u2m.test.ts b/tests/unit/sea/auth-u2m.test.ts index a98e2f0d..e18109fa 100644 --- a/tests/unit/sea/auth-u2m.test.ts +++ b/tests/unit/sea/auth-u2m.test.ts @@ -123,7 +123,7 @@ describe('SeaAuth + SeaBackend — OAuth U2M auth flow', () => { describe('SeaBackend.connect + openSession (U2M)', () => { it('round-trips U2M options through to the napi binding', async () => { const { binding, calls } = makeFakeBinding(); - const backend = new SeaBackend(binding); + const backend = new SeaBackend({ nativeBinding: binding }); await backend.connect({ host: 'example.cloud.databricks.com', @@ -132,7 +132,11 @@ describe('SeaAuth + SeaBackend — OAuth U2M auth flow', () => { }); const session = await backend.openSession({}); - expect(session.id).to.match(/^sea-session-\d+$/); + // Post-integration: SeaSessionBackend generates UUIDv4 ids; the + // earlier auth-only counter-id scheme was superseded. + expect(session.id).to.match( + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i, + ); expect(calls).to.have.lengthOf(1); expect(calls[0].method).to.equal('openSession'); diff --git a/tests/unit/sea/execution.test.ts b/tests/unit/sea/execution.test.ts index 88d7da23..d093a756 100644 --- a/tests/unit/sea/execution.test.ts +++ b/tests/unit/sea/execution.test.ts @@ -136,7 +136,13 @@ describe('SeaBackend', () => { expect(binding.openSessionStub.called).to.equal(false); }); - it('connect() rejects non-PAT auth (M0 PAT-only)', async () => { + // sea-auth-u2m: `databricks-oauth` with no id/secret is now the U2M happy + // path (M0 was PAT-only, but the OAuth M2M+U2M feature on sea-auth-u2m + // accepts the full set of `databricks-oauth` variants). M2M/U2M flow- + // dispatch coverage lives in auth-m2m.test.ts / auth-u2m.test.ts; + // out-of-scope auth modes are now whatever neither PAT nor + // `databricks-oauth` covers (e.g. `token-provider`, `external-token`). + it('connect() rejects unsupported auth modes (non-PAT, non-OAuth)', async () => { const connection = new FakeNativeConnection(); const binding = makeBinding(connection); const backend = new SeaBackend({ context: makeContext(), nativeBinding: binding }); @@ -146,13 +152,14 @@ describe('SeaBackend', () => { await backend.connect({ host: 'example.databricks.com', path: '/sql/1.0/warehouses/abc', - authType: 'databricks-oauth', - } as ConnectionOptions); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + authType: 'token-provider', + } as any); } catch (err) { thrown = err; } expect(thrown).to.be.instanceOf(HiveDriverError); - expect((thrown as Error).message).to.match(/access-token/); + expect((thrown as Error).message).to.match(/unsupported auth mode/); }); it('connect() rejects missing token', async () => { @@ -207,9 +214,12 @@ describe('SeaBackend', () => { expect(binding.openSessionStub.calledOnce).to.equal(true); const args = binding.openSessionStub.firstCall.args[0]; + // sea-auth-u2m introduced the discriminated SeaNativeConnectionOptions + // shape with a leading `authMode` tag — `'Pat'` for the PAT branch. expect(args).to.deep.equal({ hostName: 'workspace.example', httpPath: '/sql/1.0/warehouses/xyz', + authMode: 'Pat', token: 'dapi-token', }); }); From bafd8b981e20dea3772036980c7f2288433ce5d0 Mon Sep 17 00:00:00 2001 From: Madhavendra Rathore Date: Sun, 24 May 2026 13:36:25 +0000 Subject: [PATCH 10/10] refactor(sea)!: move catalog/schema/sessionConf from per-statement forwarding to openSession MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The napi binding moved initialCatalog/initialSchema/sessionConfig from ExecuteOptions onto ConnectionOptions (matching pyo3) because the kernel does not read StatementSpec::statement_conf — they were silently no-op'd in the per-statement path. Adapter follows. - SeaAuth.ts: extend SeaNativeConnectionOptions with optional catalog / schema / sessionConf (intersection with each auth-mode variant). New SeaSessionDefaults interface for the shared shape. - SeaBackend.ts::openSession: fold OpenSessionRequest.initialCatalog / initialSchema / configuration into the napi options before the openSession call. Drop the SeaSessionBackend.defaults forwarding. - SeaNativeLoader.ts: drop SeaExecuteOptions; executeStatement now takes only the SQL. - SeaSessionBackend.ts: drop SeaSessionDefaults and defaults field; drop per-statement overlay logic. useCloudFetch becomes a no-op on the SEA path (kernel hardcodes disposition to INLINE_OR_EXTERNAL_LINKS; ResultConfig exposure is M1 work). - tests: replace per-statement-forwarding assertions with openSession-arg assertions. 23/23 SEA execution tests pass (143/143 across the SEA suite). Signed-off-by: Madhavendra Rathore --- lib/sea/SeaAuth.ts | 61 ++++++++++++------ lib/sea/SeaBackend.ts | 34 ++++++---- lib/sea/SeaNativeLoader.ts | 4 +- lib/sea/SeaSessionBackend.ts | 76 +++++++--------------- native/sea/index.d.ts | 104 +++++++++++-------------------- tests/unit/sea/execution.test.ts | 47 ++++++-------- 6 files changed, 140 insertions(+), 186 deletions(-) diff --git a/lib/sea/SeaAuth.ts b/lib/sea/SeaAuth.ts index 0c393430..c6fd5178 100644 --- a/lib/sea/SeaAuth.ts +++ b/lib/sea/SeaAuth.ts @@ -49,26 +49,47 @@ const U2M_DEFAULT_REDIRECT_PORT = 8030; * incompatible with `isolatedModules` and a runtime-coupling hazard. * The Rust source of truth lives at `native/sea/src/database.rs`. */ -export type SeaNativeConnectionOptions = - | { - hostName: string; - httpPath: string; - authMode: 'Pat'; - token: string; - } - | { - hostName: string; - httpPath: string; - authMode: 'OAuthM2m'; - oauthClientId: string; - oauthClientSecret: string; - } - | { - hostName: string; - httpPath: string; - authMode: 'OAuthU2m'; - oauthRedirectPort: number; - }; +/** + * Session-level defaults shared across all auth-mode variants. + * + * Mirrors `ConnectionOptions.catalog` / `.schema` / `.sessionConf` on + * the napi binding (kernel `Session::builder().defaults(DefaultOpts)` + * and `.session_conf(HashMap)` — the routes that actually populate SEA + * `CreateSession.catalog` / `.schema` / `.session_confs`). + * + * Per-statement overrides do not exist on the kernel surface; both + * pyo3 and napi expose catalog / schema / sessionConf only at session + * creation. Mirror that here so the adapter doesn't promise a + * capability the binding can't honour. + */ +export interface SeaSessionDefaults { + catalog?: string; + schema?: string; + sessionConf?: Record; +} + +export type SeaNativeConnectionOptions = SeaSessionDefaults & + ( + | { + hostName: string; + httpPath: string; + authMode: 'Pat'; + token: string; + } + | { + hostName: string; + httpPath: string; + authMode: 'OAuthM2m'; + oauthClientId: string; + oauthClientSecret: string; + } + | { + hostName: string; + httpPath: string; + authMode: 'OAuthU2m'; + oauthRedirectPort: number; + } + ); function prependSlash(str: string): string { if (str.length > 0 && str.charAt(0) !== '/') { diff --git a/lib/sea/SeaBackend.ts b/lib/sea/SeaBackend.ts index 998796fa..61b1a333 100644 --- a/lib/sea/SeaBackend.ts +++ b/lib/sea/SeaBackend.ts @@ -89,28 +89,36 @@ export default class SeaBackend implements IBackend { throw new HiveDriverError('SeaBackend: not connected. Call connect() first.'); } + // Fold session-level defaults from the OpenSessionRequest into the + // napi `ConnectionOptions`. The kernel routes these through + // `Session::builder().defaults(DefaultOpts)` + `.session_conf(...)` + // so they land on the SEA `CreateSession` wire fields, not on each + // per-statement request. Matches pyo3's `Session.__new__` shape. + // + // Only set the optional keys when present so the napi call shape + // stays minimal — keeps wire snapshots / test assertions stable + // for callers who pass no defaults. + const sessionOptions: SeaNativeConnectionOptions = { ...this.nativeOptions }; + if (request.initialCatalog !== undefined) { + sessionOptions.catalog = request.initialCatalog; + } + if (request.initialSchema !== undefined) { + sessionOptions.schema = request.initialSchema; + } + if (request.configuration !== undefined) { + sessionOptions.sessionConf = { ...request.configuration }; + } + let nativeConnection: SeaNativeConnection; try { - nativeConnection = (await this.binding.openSession(this.nativeOptions)) as SeaNativeConnection; + nativeConnection = (await this.binding.openSession(sessionOptions)) as SeaNativeConnection; } catch (err) { throw decodeNapiKernelError(err); } - // Merge `request.configuration` (the existing public field for Spark - // conf) with any backend-specific session config. The SEA wire - // protocol applies these per-statement, but we capture them at - // session-open time and forward with every executeStatement to - // preserve session-config semantics. - const sessionConfig = request.configuration ? { ...request.configuration } : undefined; - return new SeaSessionBackend({ connection: nativeConnection!, context: this.context, - defaults: { - initialCatalog: request.initialCatalog, - initialSchema: request.initialSchema, - sessionConfig, - }, }); } diff --git a/lib/sea/SeaNativeLoader.ts b/lib/sea/SeaNativeLoader.ts index 154fd104..96c422b0 100644 --- a/lib/sea/SeaNativeLoader.ts +++ b/lib/sea/SeaNativeLoader.ts @@ -26,13 +26,12 @@ import type { Connection as NativeConnection, Statement as NativeStatement, - ExecuteOptions, ArrowBatch, ArrowSchema, } from '@sea-native'; import type { SeaNativeConnectionOptions } from './SeaAuth'; -export type { ExecuteOptions, ArrowBatch, ArrowSchema, SeaNativeConnectionOptions }; +export type { ArrowBatch, ArrowSchema, SeaNativeConnectionOptions }; export type Connection = NativeConnection; export type Statement = NativeStatement; @@ -40,7 +39,6 @@ export type Statement = NativeStatement; // refactor renamed these. New code should import the un-prefixed names. export type SeaNativeConnection = NativeConnection; export type SeaNativeStatement = NativeStatement; -export type SeaExecuteOptions = ExecuteOptions; export type SeaArrowBatch = ArrowBatch; export type SeaArrowSchema = ArrowSchema; diff --git a/lib/sea/SeaSessionBackend.ts b/lib/sea/SeaSessionBackend.ts index de63191f..a79759ea 100644 --- a/lib/sea/SeaSessionBackend.ts +++ b/lib/sea/SeaSessionBackend.ts @@ -31,33 +31,14 @@ import { import Status from '../dto/Status'; import InfoValue from '../dto/InfoValue'; import HiveDriverError from '../errors/HiveDriverError'; -import { SeaNativeConnection, SeaExecuteOptions } from './SeaNativeLoader'; +import { SeaNativeConnection } from './SeaNativeLoader'; import { decodeNapiKernelError } from './SeaErrorMapping'; import SeaOperationBackend from './SeaOperationBackend'; -/** - * Per-session defaults that apply to every `executeStatement` issued - * through this backend. Captured at `SeaBackend.openSession()` time from - * the `OpenSessionRequest` — `initialCatalog` / `initialSchema` / - * `sessionConfig`. - * - * The napi binding routes these to the kernel's `statement_conf` map, - * which the SEA wire treats as session-scoped parameters. They are - * forwarded with every `executeStatement` call so the JDBC-style - * "session config" semantics are preserved even though SEA's wire - * protocol is statement-scoped. - */ -export interface SeaSessionDefaults { - initialCatalog?: string; - initialSchema?: string; - sessionConfig?: Record; -} - export interface SeaSessionBackendOptions { /** The opaque napi `Connection` handle returned by `openSession`. */ connection: SeaNativeConnection; context: IClientContext; - defaults?: SeaSessionDefaults; /** Optional override for `id`. Defaults to a fresh UUIDv4. */ id?: string; } @@ -72,30 +53,26 @@ export interface SeaSessionBackendOptions { * backend continues to handle the metadata path by default (callers * opt into SEA via `ConnectionOptions.useSEA`). * - * **Session config flow:** the SEA wire protocol is statement-scoped, - * so "session config" semantics (Spark conf, `initialCatalog`, - * `initialSchema`) are emulated by forwarding the same defaults with - * every `executeStatement` call. Per-statement overrides on - * `ExecuteStatementOptions` are reserved for M1; M0 carries only the - * defaults captured at session-open time plus the `useCloudFetch` - * boolean projected onto `sessionConfig.use_cloud_fetch` for the - * kernel. + * **Session config flow:** catalog / schema / sessionConf are applied + * once at session creation (kernel `Session::builder().defaults()` + + * `.session_conf()` → SEA `CreateSession.catalog` / `.schema` / + * `.session_confs`) and remain in effect for every statement run on + * the resulting napi `Connection`. No per-statement forwarding is + * needed — that pattern was removed when the napi binding moved these + * onto `openSession` to match pyo3. */ export default class SeaSessionBackend implements ISessionBackend { private readonly connection: SeaNativeConnection; private readonly context: IClientContext; - private readonly defaults: SeaSessionDefaults; - private readonly _id: string; private closed = false; - constructor({ connection, context, defaults, id }: SeaSessionBackendOptions) { + constructor({ connection, context, id }: SeaSessionBackendOptions) { this.connection = connection; this.context = context; - this.defaults = defaults ?? {}; this._id = id ?? uuidv4(); } @@ -108,13 +85,21 @@ export default class SeaSessionBackend implements ISessionBackend { } /** - * Execute a SQL statement through the napi binding. Merges the - * session-level defaults (`initialCatalog` / `initialSchema` / - * `sessionConfig`) with the per-call `useCloudFetch` override. + * Execute a SQL statement through the napi binding. + * + * Catalog / schema / sessionConf were applied at session open, so + * there are no per-statement options to thread through. * * M0 intentionally rejects `queryTimeout`, `namedParameters`, and - * `ordinalParameters` with explicit deferred-to-M1 errors. The Thrift - * backend remains the path for consumers that need any of those today. + * `ordinalParameters` with explicit deferred-to-M1 errors. `useCloudFetch` + * is a no-op on the SEA path — the kernel hardcodes the SEA + * `disposition` to `INLINE_OR_EXTERNAL_LINKS`, and per-statement + * conf overrides have no reader on the kernel; cloud-fetch behaviour + * is governed entirely by the kernel's `ResultConfig` (M1 binding + * surface). + * + * The Thrift backend remains the path for consumers that need any + * of those today. */ public async executeStatement(statement: string, options: ExecuteStatementOptions): Promise { this.failIfClosed(); @@ -131,24 +116,9 @@ export default class SeaSessionBackend implements ISessionBackend { ); } - // Merge session-level sessionConfig with per-statement useCloudFetch. - // The kernel accepts only string-valued conf values; booleans are - // String()'d to "true"/"false" matching the existing Thrift conf - // convention. - const sessionConfig: Record = { ...(this.defaults.sessionConfig ?? {}) }; - if (options.useCloudFetch !== undefined) { - sessionConfig.use_cloud_fetch = String(options.useCloudFetch); - } - - const executeOptions: SeaExecuteOptions = { - initialCatalog: this.defaults.initialCatalog, - initialSchema: this.defaults.initialSchema, - sessionConfig: Object.keys(sessionConfig).length > 0 ? sessionConfig : undefined, - }; - let nativeStatement; try { - nativeStatement = await this.connection.executeStatement(statement, executeOptions); + nativeStatement = await this.connection.executeStatement(statement); } catch (err) { throw decodeNapiKernelError(err); } diff --git a/native/sea/index.d.ts b/native/sea/index.d.ts index 97257895..658feeea 100644 --- a/native/sea/index.d.ts +++ b/native/sea/index.d.ts @@ -4,61 +4,16 @@ /* auto-generated by NAPI-RS */ /** - * JS-visible per-execute options. M0 only carries - * initialCatalog / initialSchema / sessionConfig — parameters and - * per-statement overrides land in M1. - */ -export interface ExecuteOptions { - /** Default catalog applied to this statement via session conf. */ - initialCatalog?: string - /** Default schema applied to this statement via session conf. */ - initialSchema?: string - /** - * Per-statement session conf overrides (forwarded to SEA - * `parameters` / Thrift `confOverlay`). - */ - sessionConfig?: Record -} -/** - * JS-visible auth-mode discriminant. - * - * Crosses the FFI as the variant name verbatim — napi-rs's - * `#[napi(string_enum)]` without an explicit case option emits the - * Rust variant identifier as-is, so this enum becomes - * `AuthMode.Pat = 'Pat'` / `AuthMode.OAuthM2m = 'OAuthM2m'` / - * `AuthMode.OAuthU2m = 'OAuthU2m'` in the auto-generated - * `native/sea/index.d.ts`. The JS adapter - * (`SeaNativeConnectionOptions` in `lib/sea/SeaAuth.ts`) must use the - * PascalCase literals verbatim. - * - * Keeping the discriminant explicit instead of inferring it from - * "which Option is set" makes the napi-side validation single-pass - * and the JS-side schema typed. - * - * Note: adding a variant here requires extending `open_session()`'s - * `match` — Rust will fail the build if the match is non-exhaustive, - * but the cross-reference shortens the review loop. - */ -export const enum AuthMode { - Pat = 'Pat', - OAuthM2m = 'OAuthM2m', - OAuthU2m = 'OAuthU2m' -} -/** - * JS-visible options for opening a Databricks SQL session. + * JS-visible options for opening a Databricks SQL session over PAT. * - * Discriminated by `auth_mode`: - * - `AuthMode::Pat` → requires `token`; ignores oauth_*. - * - `AuthMode::OAuthM2m` → requires `oauth_client_id` + `oauth_client_secret`. - * - `AuthMode::OAuthU2m` → kernel handles the auth-code flow with - * default client_id (`databricks-cli`), scopes - * (`["all-apis","offline_access"]`), and OIDC discovery; the JS - * adapter hardcodes `oauth_redirect_port` to 8030 to override the - * kernel default of 8020 (thrift uses 8030 — preserves parity). + * M0 supports PAT only — `token` is required. OAuth M2M / U2M variants + * land in M1 along with a discriminated-union shape on the JS side. * - * Scopes / token_url_override / client_id / callback_timeout are not - * exposed — kernel defaults match thrift parity and the public driver - * surface has no demand to override them. + * Catalog / schema / sessionConf are applied once at session creation + * and remain in effect for every statement run on the resulting + * `Connection`. The SEA wire protocol carries them on + * `CreateSession`, not on `ExecuteStatement` — so there is no + * per-statement override path in either this binding or pyo3. */ export interface ConnectionOptions { /** @@ -71,24 +26,33 @@ export interface ConnectionOptions { * kernel parses out the warehouse id. */ httpPath: string - /** Auth-mode discriminant. Required. */ - authMode: AuthMode - /** Personal access token. Required iff `auth_mode == Pat`. */ - token?: string - /** OAuth client id. Required iff `auth_mode == OAuthM2m`. */ - oauthClientId?: string - /** OAuth client secret. Required iff `auth_mode == OAuthM2m`. */ - oauthClientSecret?: string /** - * Local listener port for the U2M authorization-code callback. - * Forwarded verbatim to the kernel; the JS adapter hardcodes 8030 - * for thrift parity. + * Personal access token. Must be non-empty (the kernel rejects + * empty PATs at session construction). */ - oauthRedirectPort?: number + token: string + /** + * Default catalog for statements executed on this session. + * Routed through the kernel's `DefaultOpts` and onto the SEA + * `CreateSession.catalog` wire field. + */ + catalog?: string + /** + * Default schema for statements executed on this session. + * Routed through the kernel's `DefaultOpts` and onto the SEA + * `CreateSession.schema` wire field. + */ + schema?: string + /** + * Server-bound session conf (Spark conf, `ANSI_MODE`, `TIMEZONE`, + * query-tag presets, …). Forwarded verbatim to SEA + * `session_confs`. Unknown keys are rejected server-side. + */ + sessionConf?: Record } /** - * Open a Databricks SQL session and return an opaque `Connection` - * wrapping the kernel `Session`. Supports PAT, OAuth M2M, and OAuth U2M. + * Open a Databricks SQL session over PAT auth and return an opaque + * `Connection` wrapping the kernel `Session`. * * The JS-visible name is `openSession` (napi-rs converts snake_case * to camelCase for free functions). @@ -130,8 +94,12 @@ export declare class Connection { /** * Execute a SQL statement and return a Statement handle that * streams batches via `fetchNextBatch()`. + * + * No per-statement options: catalog / schema / sessionConf are + * session-level (`openSession`). Positional / named parameters + * land in M1 via `Statement::spec().param(…)` on the kernel. */ - executeStatement(sql: string, options: ExecuteOptions): Promise + executeStatement(sql: string): Promise /** * Explicit close. Marks the connection wrapper as closed so * subsequent calls on this `Connection` return `InvalidArg`, then diff --git a/tests/unit/sea/execution.test.ts b/tests/unit/sea/execution.test.ts index d093a756..2981fd3f 100644 --- a/tests/unit/sea/execution.test.ts +++ b/tests/unit/sea/execution.test.ts @@ -21,7 +21,6 @@ import { SeaNativeBinding, SeaNativeConnection, SeaNativeStatement, - SeaExecuteOptions, } from '../../../lib/sea/SeaNativeLoader'; import IClientContext, { ClientConfig } from '../../../lib/contracts/IClientContext'; import IDBSQLLogger, { LogLevel } from '../../../lib/contracts/IDBSQLLogger'; @@ -61,18 +60,15 @@ class FakeNativeConnection implements SeaNativeConnection { public lastSql?: string; - public lastOptions?: SeaExecuteOptions; - public throwOnExecute: Error | null = null; public statementToReturn: FakeNativeStatement = new FakeNativeStatement(); - public async executeStatement(sql: string, options: SeaExecuteOptions): Promise { + public async executeStatement(sql: string): Promise { if (this.throwOnExecute) { throw this.throwOnExecute; } this.lastSql = sql; - this.lastOptions = options; return this.statementToReturn; } @@ -240,7 +236,7 @@ describe('SeaBackend', () => { expect(sessionBackend.id).to.be.a('string').and.have.length.greaterThan(0); }); - it('openSession() propagates initialCatalog / initialSchema / sessionConfig through to executeStatement', async () => { + it('openSession() forwards initialCatalog / initialSchema / configuration to the napi openSession call (not per-statement)', async () => { const connection = new FakeNativeConnection(); const binding = makeBinding(connection); const backend = new SeaBackend({ context: makeContext(), nativeBinding: binding }); @@ -257,14 +253,22 @@ describe('SeaBackend', () => { configuration: { 'spark.sql.execution.arrow.enabled': 'true' }, }); - await session.executeStatement('SELECT 1', {}); + // The defaults reach the kernel via `Session::builder().defaults()` + + // `.session_conf()`, applied on `CreateSession`. Assert they were + // folded into the napi `openSession` arg. + expect(binding.openSessionStub.calledOnce).to.equal(true); + expect(binding.openSessionStub.firstCall.args[0]).to.deep.include({ + authMode: 'Pat', + token: 't', + catalog: 'main', + schema: 'default', + sessionConf: { 'spark.sql.execution.arrow.enabled': 'true' }, + }); + // And the SQL still threads through executeStatement (now with no + // per-statement options). + await session.executeStatement('SELECT 1', {}); expect(connection.lastSql).to.equal('SELECT 1'); - expect(connection.lastOptions).to.deep.equal({ - initialCatalog: 'main', - initialSchema: 'default', - sessionConfig: { 'spark.sql.execution.arrow.enabled': 'true' }, - }); }); it('close() clears connection state without throwing', async () => { @@ -285,8 +289,8 @@ describe('SeaBackend', () => { }); describe('SeaSessionBackend', () => { - function makeSession(connection: SeaNativeConnection, defaults = {}) { - return new SeaSessionBackend({ connection, context: makeContext(), defaults }); + function makeSession(connection: SeaNativeConnection) { + return new SeaSessionBackend({ connection, context: makeContext() }); } it('executeStatement passes sql through verbatim', async () => { @@ -304,21 +308,6 @@ describe('SeaSessionBackend', () => { expect(op.id).to.be.a('string').and.have.length.greaterThan(0); }); - it('executeStatement merges session defaults into ExecuteOptions', async () => { - const connection = new FakeNativeConnection(); - const session = makeSession(connection, { - initialCatalog: 'main', - initialSchema: 'default', - sessionConfig: { foo: 'bar' }, - }); - await session.executeStatement('SELECT 1', {}); - expect(connection.lastOptions).to.deep.equal({ - initialCatalog: 'main', - initialSchema: 'default', - sessionConfig: { foo: 'bar' }, - }); - }); - it('executeStatement rejects namedParameters (M1)', async () => { const connection = new FakeNativeConnection(); const session = makeSession(connection);