Skip to content

Commit f436d6f

Browse files
msrathore-dbmsrathore-db
committed
feat(kernel): route OAuth by secret (Thrift parity) + support custom U2M client id
Switch the kernel auth flow selector to key off the SECRET, matching the Thrift driver (DBSQLClient.createAuthProvider): a usable oauthClientSecret => M2M, otherwise => U2M. Previously the kernel keyed off oauthClientId *presence*, which diverged from Thrift and rejected `id + no secret` rather than running U2M. Consequences: - `oauthClientId + no secret` now runs U2M (browser), like Thrift — instead of an M2M "secret required" error. - A non-blank oauthClientId on U2M is now forwarded as a *custom U2M client* (parity with Thrift, which passes options.oauthClientId to its U2M flow); a blank/reserved id is treated as absent and the napi default (databricks-sql-connector) is used. Blank/reserved literals (whitespace, "null", "undefined" — typical of an unset env var) count as absent for routing. Trade-off vs the prior id-presence routing: `id set + secret forgotten` now silently runs U2M (browser) rather than a clear error — this matches Thrift's behaviour. Updated the auth unit tests (u2m/m2m/edge-cases) that encoded the old id-presence routing to the new secret-based semantics. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
1 parent 5be4626 commit f436d6f

4 files changed

Lines changed: 69 additions & 119 deletions

File tree

‎lib/kernel/KernelAuth.ts‎

Lines changed: 25 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,7 @@ export type KernelNativeConnectionOptions = KernelSessionDefaults &
211211
authMode: 'OAuthU2m';
212212
oauthRedirectPort: number;
213213
oauthScopes?: Array<string>;
214+
oauthClientId?: string;
214215
}
215216
);
216217

@@ -596,40 +597,22 @@ export function buildKernelConnectionOptions(options: ConnectionOptions): Kernel
596597
);
597598
}
598599

599-
// Flow selector — DELIBERATELY DIFFERENT from thrift's
600-
// `DBSQLClient.createAuthProvider` (`DBSQLClient.ts:216`), which keys off
601-
// the secret (`oauthClientSecret === undefined ? U2M : M2M`). kernel keys off
602-
// `oauthClientId` *presence* (the "do I have an id?" signal) instead, so a
603-
// user who set an id but typoed/forgot the secret gets the actionable M2M
604-
// "secret is required" error rather than being silently routed to U2M
605-
// (which would hide their intent). Cost: `id + no secret` throws here
606-
// where thrift would run U2M, and kernel U2M has no custom-client-id support
607-
// (see buildKernelConnectionOptions header). The U2M arm still defends against an id
608-
// sneaking through: fires only when `oauthClientId` is provided as
609-
// a blank-reserved literal (e.g., whitespace, `"null"`, `"undefined"`)
610-
// alongside an absent/blank secret — both `idIsBlank` and
611-
// `secretIsBlank` are true so U2M wins routing, but the caller's
612-
// intent to use U2M with a partially-set id is ambiguous and
613-
// rejected explicitly.
614-
const idIsBlank =
615-
oauth.oauthClientId === undefined ||
616-
(typeof oauth.oauthClientId === 'string' && isBlankOrReserved(oauth.oauthClientId));
600+
// Flow selector — keyed off the SECRET, matching the Thrift driver
601+
// (`DBSQLClient.createAuthProvider`, DBSQLClient.ts:220:
602+
// `oauthClientSecret === undefined ? U2M : M2M`). A usable
603+
// `oauthClientSecret` ⇒ M2M (client-credentials); otherwise ⇒ U2M (browser).
604+
// Blank/reserved literals (whitespace, `"null"`, `"undefined"` — typical of
605+
// an unset env var) count as absent. Trade-off vs the prior id-presence
606+
// routing: `id set + secret forgotten` now runs U2M (browser) rather than a
607+
// "secret required" error — this is exactly Thrift's behaviour.
617608
const secretIsBlank =
618609
oauth.oauthClientSecret === undefined ||
619610
(typeof oauth.oauthClientSecret === 'string' && isBlankOrReserved(oauth.oauthClientSecret));
611+
const hasUsableClientId =
612+
typeof oauth.oauthClientId === 'string' && !isBlankOrReserved(oauth.oauthClientId);
620613

621-
if (idIsBlank && secretIsBlank) {
622-
// U2M — neither id nor secret supplied.
623-
if (oauth.oauthClientId !== undefined) {
624-
// Defense-in-depth: id was set but blank/reserved literal.
625-
// The kernel hardcodes `client_id = "databricks-cli"` for U2M;
626-
// there's no JS-side override knob.
627-
throw new HiveDriverError(
628-
'kernel backend: `oauthClientId` is not supported on the OAuth U2M flow; ' +
629-
"the kernel uses the built-in 'databricks-cli' client. " +
630-
'Omit `oauthClientId` for U2M, or supply `oauthClientSecret` for the M2M flow.',
631-
);
632-
}
614+
if (secretIsBlank) {
615+
// U2M — no usable secret.
633616
if (oauth.persistence !== undefined) {
634617
throw new HiveDriverError(
635618
'kernel backend: `persistence` (custom OAuth token store) is not yet wired through ' +
@@ -640,31 +623,29 @@ export function buildKernelConnectionOptions(options: ConnectionOptions): Kernel
640623
'when the kernel exposes it.',
641624
);
642625
}
643-
return {
626+
const u2m = {
644627
...base,
645-
authMode: 'OAuthU2m',
628+
authMode: 'OAuthU2m' as const,
646629
oauthRedirectPort: U2M_DEFAULT_REDIRECT_PORT,
647-
// Pass scopes explicitly so the kernel requests the same set as the
648-
// Thrift driver (`sql offline_access`) rather than its bare-Rust
649-
// `all-apis offline_access` default. Caller can override via `oauthScopes`.
630+
// Scopes default to Thrift parity (`sql offline_access`); overridable.
650631
oauthScopes:
651632
Array.isArray(oauth.oauthScopes) && oauth.oauthScopes.length > 0
652633
? oauth.oauthScopes
653634
: U2M_DEFAULT_SCOPES,
654635
};
636+
// A non-blank `oauthClientId` is honoured as a custom U2M client (parity
637+
// with Thrift, which forwards `options.oauthClientId` to its U2M flow). A
638+
// blank/reserved id is treated as absent → the napi default
639+
// (`databricks-sql-connector`) is used.
640+
return hasUsableClientId ? { ...u2m, oauthClientId: oauth.oauthClientId as string } : u2m;
655641
}
656642

657-
// M2M.
658-
if (typeof oauth.oauthClientId !== 'string' || isBlankOrReserved(oauth.oauthClientId)) {
643+
// M2M — a usable secret is present; the client id is required.
644+
if (!hasUsableClientId) {
659645
throw new AuthenticationError(
660646
'kernel backend: `oauthClientId` is required (non-empty, non-whitespace) for OAuth M2M.',
661647
);
662648
}
663-
if (typeof oauth.oauthClientSecret !== 'string' || isBlankOrReserved(oauth.oauthClientSecret)) {
664-
throw new AuthenticationError(
665-
'kernel backend: `oauthClientSecret` must be a non-empty non-whitespace string for OAuth M2M.',
666-
);
667-
}
668649
if (oauth.persistence !== undefined) {
669650
throw new HiveDriverError(
670651
'kernel backend: `persistence` is not supported on OAuth M2M ' +
@@ -674,8 +655,8 @@ export function buildKernelConnectionOptions(options: ConnectionOptions): Kernel
674655
return {
675656
...base,
676657
authMode: 'OAuthM2m',
677-
oauthClientId: oauth.oauthClientId,
678-
oauthClientSecret: oauth.oauthClientSecret,
658+
oauthClientId: oauth.oauthClientId as string,
659+
oauthClientSecret: oauth.oauthClientSecret as string,
679660
// Configurable (parity with pyo3); defaults to `['all-apis']` — the only
680661
// scope the client-credentials flow allows, matching Thrift + the kernel.
681662
oauthScopes:

‎tests/unit/kernel/auth-edge-cases.test.ts‎

Lines changed: 29 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -67,13 +67,11 @@ describe('KernelAuth — edge cases (input validation + ambiguity)', () => {
6767
}
6868
});
6969

70-
// Round-4 NF3-2: presence of `oauthClientId` signals M2M intent.
71-
// A blank/reserved-literal `oauthClientSecret` is then a missing-secret
72-
// typo, not a request to fall back to U2M. Surface the M2M "secret
73-
// required" AuthenticationError so the user fixes the real problem
74-
// rather than swap class to a HiveDriverError pointing at a flow
75-
// they didn't intend to use.
76-
it('rejects mixed-case reserved-literal oauthClientSecret with AuthenticationError when id is set', () => {
70+
// Secret-based routing (Thrift parity): a blank/reserved-literal
71+
// `oauthClientSecret` counts as absent ⇒ U2M, with the non-blank id carried
72+
// as a custom U2M client. (Old id-presence routing surfaced an M2M
73+
// "secret required" error for these.)
74+
it('routes a reserved-literal oauthClientSecret (id set) to U2M with the custom id', () => {
7775
const opts: ConnectionOptions = {
7876
host: 'example.cloud.databricks.com',
7977
path: '/sql/1.0/warehouses/abc',
@@ -82,10 +80,9 @@ describe('KernelAuth — edge cases (input validation + ambiguity)', () => {
8280
oauthClientSecret: 'NULL',
8381
};
8482

85-
expect(() => buildKernelConnectionOptions(opts)).to.throw(
86-
AuthenticationError,
87-
/oauthClientSecret.*non-empty.*OAuth M2M/,
88-
);
83+
const native = buildKernelConnectionOptions(opts);
84+
expect(native.authMode).to.equal('OAuthU2m');
85+
expect((native as { oauthClientId?: string }).oauthClientId).to.equal('client-uuid');
8986
});
9087

9188
it('rejects whitespace-only oauthClientId on M2M', () => {
@@ -100,7 +97,7 @@ describe('KernelAuth — edge cases (input validation + ambiguity)', () => {
10097
expect(() => buildKernelConnectionOptions(opts)).to.throw(AuthenticationError, /oauthClientId.*required/);
10198
});
10299

103-
it('rejects whitespace-only oauthClientSecret with AuthenticationError when oauthClientId is set (M2M intent)', () => {
100+
it('routes a whitespace-only oauthClientSecret (id set) to U2M with the custom id', () => {
104101
const opts: ConnectionOptions = {
105102
host: 'example.cloud.databricks.com',
106103
path: '/sql/1.0/warehouses/abc',
@@ -109,10 +106,9 @@ describe('KernelAuth — edge cases (input validation + ambiguity)', () => {
109106
oauthClientSecret: '\n\t',
110107
};
111108

112-
expect(() => buildKernelConnectionOptions(opts)).to.throw(
113-
AuthenticationError,
114-
/oauthClientSecret.*non-empty.*OAuth M2M/,
115-
);
109+
const native = buildKernelConnectionOptions(opts);
110+
expect(native.authMode).to.equal('OAuthU2m');
111+
expect((native as { oauthClientId?: string }).oauthClientId).to.equal('client-uuid');
116112
});
117113

118114
it('rejects literal "undefined" as oauthClientId on M2M', () => {
@@ -127,7 +123,7 @@ describe('KernelAuth — edge cases (input validation + ambiguity)', () => {
127123
expect(() => buildKernelConnectionOptions(opts)).to.throw(AuthenticationError, /oauthClientId.*required/);
128124
});
129125

130-
it('rejects literal "undefined" as oauthClientSecret with AuthenticationError when id is set (M2M intent)', () => {
126+
it('routes a literal "undefined" oauthClientSecret (id set) to U2M with the custom id', () => {
131127
const opts: ConnectionOptions = {
132128
host: 'example.cloud.databricks.com',
133129
path: '/sql/1.0/warehouses/abc',
@@ -136,24 +132,14 @@ describe('KernelAuth — edge cases (input validation + ambiguity)', () => {
136132
oauthClientSecret: 'undefined',
137133
};
138134

139-
expect(() => buildKernelConnectionOptions(opts)).to.throw(
140-
AuthenticationError,
141-
/oauthClientSecret.*non-empty.*OAuth M2M/,
142-
);
135+
const native = buildKernelConnectionOptions(opts);
136+
expect(native.authMode).to.equal('OAuthU2m');
137+
expect((native as { oauthClientId?: string }).oauthClientId).to.equal('client-uuid');
143138
});
144139

145-
// Round-4 NF3-2: pin the exact class against the round-3 NF-N3
146-
// regression where M2M-with-empty-secret was routed through the U2M
147-
// arm and raised a bare `HiveDriverError`. `instanceof
148-
// AuthenticationError` correctly returns `false` for a bare
149-
// `HiveDriverError` instance (instanceof is a one-way subclass
150-
// check), so the subclass check IS sufficient to catch the
151-
// regression. We don't add an `error.name` or `constructor.name`
152-
// belt — the former requires `this.name` on the subclass (LE4-1
153-
// handles that separately for downstream-consumer benefit, not for
154-
// this test), and the latter is bundler-fragile (terser/esbuild
155-
// strip class names without `keep_classnames`).
156-
it('M2M-with-empty-secret throws AuthenticationError, not bare HiveDriverError (class pin)', () => {
140+
// Secret-based routing: id + empty secret ⇒ U2M (the empty secret counts as
141+
// absent), carrying the id as a custom U2M client — it does NOT throw.
142+
it('routes id + empty secret to U2M (does not throw)', () => {
157143
const opts: ConnectionOptions = {
158144
host: 'example.cloud.databricks.com',
159145
path: '/sql/1.0/warehouses/abc',
@@ -162,31 +148,25 @@ describe('KernelAuth — edge cases (input validation + ambiguity)', () => {
162148
oauthClientSecret: '',
163149
};
164150

165-
expect(() => buildKernelConnectionOptions(opts)).to.throw(
166-
AuthenticationError,
167-
/oauthClientSecret.*non-empty.*OAuth M2M/,
168-
);
151+
const native = buildKernelConnectionOptions(opts);
152+
expect(native.authMode).to.equal('OAuthU2m');
153+
expect((native as { oauthClientId?: string }).oauthClientId).to.equal('x');
169154
});
170155

171-
// Round-5 DA4-2: the round-3 → round-4 test flips left the U2M-arm
172-
// defense-in-depth U2M+id rejection without coverage. It's still
173-
// reachable: when `oauthClientId` is a blank-reserved literal
174-
// (whitespace, `"null"`, `"undefined"`) AND `oauthClientSecret` is
175-
// absent/blank, BOTH `idIsBlank` and `secretIsBlank` are true so
176-
// U2M wins routing — but a non-undefined id signals ambiguity that
177-
// U2M cannot honor (the kernel hardcodes `databricks-cli`).
178-
it('routes a whitespace oauthClientId with no oauthClientSecret to the U2M defense-in-depth rejection', () => {
156+
// Secret-based routing: a blank/reserved `oauthClientId` (whitespace) with no
157+
// secret ⇒ U2M, and the blank id is treated as absent → the napi default
158+
// client is used (no `oauthClientId` forwarded). It does NOT throw.
159+
it('routes a whitespace oauthClientId with no secret to U2M using the default client (no id forwarded)', () => {
179160
const opts: ConnectionOptions = {
180161
host: 'example.cloud.databricks.com',
181162
path: '/sql/1.0/warehouses/abc',
182163
authType: 'databricks-oauth',
183164
oauthClientId: ' ',
184165
} as unknown as ConnectionOptions;
185166

186-
expect(() => buildKernelConnectionOptions(opts)).to.throw(
187-
HiveDriverError,
188-
/oauthClientId.*not supported on the OAuth U2M flow/,
189-
);
167+
const native = buildKernelConnectionOptions(opts);
168+
expect(native.authMode).to.equal('OAuthU2m');
169+
expect((native as { oauthClientId?: string }).oauthClientId).to.equal(undefined);
190170
});
191171
});
192172

‎tests/unit/kernel/auth-m2m.test.ts‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ describe('KernelAuth + KernelBackend — OAuth M2M auth flow', () => {
104104
expect(() => buildKernelConnectionOptions(opts)).to.throw(AuthenticationError, /oauthClientId.*required/);
105105
});
106106

107-
it('rejects empty oauthClientSecret with AuthenticationError when oauthClientId is set (M2M intent)', () => {
107+
it('routes id + empty/blank secret to U2M (secret-based routing, Thrift parity)', () => {
108108
const opts: ConnectionOptions = {
109109
host: 'example.cloud.databricks.com',
110110
path: '/sql/1.0/warehouses/abc',
@@ -113,14 +113,12 @@ describe('KernelAuth + KernelBackend — OAuth M2M auth flow', () => {
113113
oauthClientSecret: '',
114114
};
115115

116-
// Presence of `oauthClientId` signals M2M intent; an empty secret
117-
// is a typo/missing-env, not a request to fall back to U2M.
118-
// Surface the M2M "secret required" error so the user knows the
119-
// real problem instead of getting routed to a different flow.
120-
expect(() => buildKernelConnectionOptions(opts)).to.throw(
121-
AuthenticationError,
122-
/oauthClientSecret.*non-empty.*OAuth M2M/,
123-
);
116+
// Routing keys off the SECRET (matching Thrift): a blank/empty secret
117+
// counts as absent ⇒ U2M, with the id carried as a custom U2M client.
118+
// (Old id-presence routing surfaced an M2M "secret required" error here.)
119+
const native = buildKernelConnectionOptions(opts);
120+
expect(native.authMode).to.equal('OAuthU2m');
121+
expect((native as { oauthClientId?: string }).oauthClientId).to.equal('client-uuid');
124122
});
125123

126124
it('rejects azureTenantId with a clear Entra-direct-out-of-scope error', () => {

‎tests/unit/kernel/auth-u2m.test.ts‎

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import expectNativeConnectionOptions from './_helpers/nativeOptions';
1717
import KernelBackend from '../../../lib/kernel/KernelBackend';
1818
import { buildKernelConnectionOptions } from '../../../lib/kernel/KernelAuth';
1919
import { ConnectionOptions } from '../../../lib/contracts/IDBSQLClient';
20-
import AuthenticationError from '../../../lib/errors/AuthenticationError';
2120
import HiveDriverError from '../../../lib/errors/HiveDriverError';
2221
import { makeFakeBinding, makeFakeContext } from './_helpers/fakeBinding';
2322

@@ -73,29 +72,21 @@ describe('KernelAuth + KernelBackend — OAuth U2M auth flow', () => {
7372
expect((native as { oauthScopes?: string[] }).oauthScopes).to.deep.equal(['sql', 'offline_access']);
7473
});
7574

76-
it('rejects oauthClientId without oauthClientSecret as M2M-with-missing-secret', () => {
77-
// Round-4 NF3-2: presence of `oauthClientId` signals M2M intent.
78-
// Routing now keys off the id (the "do I have an id?" signal),
79-
// not the secret. A caller who supplies id but no secret gets the
80-
// M2M "secret is required" error — the actionable message for the
81-
// real problem (typo'd env var, forgot to export it, etc.).
82-
//
83-
// The U2M arm still has a defense-in-depth rejection of a stray
84-
// `oauthClientId` (the kernel hardcodes `databricks-cli` for U2M);
85-
// see [NF-2 / round-1 history]. That defense fires only when
86-
// BOTH id and secret are blank — the M2M arm's stricter checks
87-
// catch this typical caller-error shape first.
75+
it('routes oauthClientId + no secret to U2M with that id as a custom client (secret-based routing, Thrift parity)', () => {
76+
// Routing keys off the SECRET (matching Thrift): no usable secret ⇒ U2M,
77+
// regardless of the id. A non-blank `oauthClientId` is then honoured as a
78+
// custom U2M client (Thrift forwards `options.oauthClientId` to its U2M
79+
// flow too). (Old id-presence routing rejected this as M2M-missing-secret.)
8880
const opts: ConnectionOptions = {
8981
host: 'example.cloud.databricks.com',
9082
path: '/sql/1.0/warehouses/abc',
9183
authType: 'databricks-oauth',
9284
oauthClientId: 'custom-client',
9385
};
9486

95-
expect(() => buildKernelConnectionOptions(opts)).to.throw(
96-
AuthenticationError,
97-
/oauthClientSecret.*non-empty.*OAuth M2M/,
98-
);
87+
const native = buildKernelConnectionOptions(opts);
88+
expect(native.authMode).to.equal('OAuthU2m');
89+
expect((native as { oauthClientId?: string }).oauthClientId).to.equal('custom-client');
9990
});
10091

10192
it('prepends `/` to the path on the U2M branch too', () => {

0 commit comments

Comments
 (0)