feat: add tokenCacheEnabled to control kernel U2M on-disk token cache - #513
feat: add tokenCacheEnabled to control kernel U2M on-disk token cache#513eric-wang-1990 wants to merge 7 commits into
Conversation
eric-wang-1990
added
the
engineer-bot
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
There was a problem hiding this comment.
Pull request overview
Adds a new tokenCacheEnabled?: boolean connection option for authType: 'databricks-oauth' to control whether the kernel backend’s U2M OAuth flow may use an on-disk refresh-token cache, while keeping “no persistence” as the default posture when migrating to the kernel path.
Changes:
- Plumbs
tokenCacheEnabledthroughbuildKernelConnectionOptionsfor OAuth U2M, defaulting tofalsewhen unset. - Extends the public connection option types/docs to describe the new flag and its kernel/U2M-only behavior.
- Adds unit tests covering default
falseand opt-intruemapping for U2M.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/unit/kernel/auth-u2m.test.ts | Adds assertions/tests to ensure tokenCacheEnabled is forwarded (default false, opt-in true) for kernel OAuth U2M. |
| lib/kernel/KernelAuth.ts | Adds tokenCacheEnabled to the kernel native option type and wires defaulting logic into the U2M mapping. |
| lib/contracts/IDBSQLClient.ts | Exposes tokenCacheEnabled?: boolean on databricks-oauth connection options with API docs. |
| CONNECTION_PARAMETERS.md | Documents the new connection parameter and calls out kernel-only behavior and default. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Token cache is disabled by default for security (silent-no-persist parity); | ||
| // explicitly set to false unless the caller opts in. | ||
| tokenCacheEnabled: oauth.tokenCacheEnabled ?? false, |
There was a problem hiding this comment.
Fixed the misleading error wording and confirmed the mocked U2M auth suite still passes.
Pushed 83b91f0 (bundled with 1 other thread(s)).
| @@ -66,6 +66,7 @@ column. | |||
| | `oauthClientSecret` (M2M) | `string` | ✅ | ✅ | — | M2M client-credentials secret; its **presence** is the U2M-vs-M2M flow selector on both backends (`undefined` ⇒ U2M). Thrift → `DatabricksOAuth.clientSecret`. Kernel → native `oauthClientSecret` (workspace-OIDC M2M) or remapped to `azureClientSecret` (Entra-direct `AzureSpM2m`). A blank/reserved secret is forwarded verbatim and still selects M2M (Thrift parity) — except the Azure SP arm, which rejects it. | | |||
| | `azureTenantId` / `useDatabricksOAuthInAzure` | `string` / `boolean` | ✅ | ⚠️ | — | **Honored on both.** By design the kernel routes **all U2M** (no secret, any cloud) to its cloud-blind in-house OAuth U2M flow — there is no Azure-specific U2M mode, so `useDatabricksOAuthInAzure` is inert on U2M and every Azure workspace (including `.databricks.azure.us` US-gov) is always supported, on any kernel build. `useDatabricksOAuthInAzure` selects only the **M2M** mechanism on an Azure host: absent/`false` → Entra-direct service-principal M2M (native `AzureSpM2m` mode, creds ride `oauthClientId`/`oauthClientSecret`, `azureTenantId` optional — kernel auto-discovers from the workspace `/aad/auth` redirect when omitted); `true` → workspace-OIDC M2M. (`lib/kernel/KernelAuth.ts` `buildKernelConnectionOptions`.) | | |||
| | `persistence` (custom OAuth token store) | `OAuthPersistence` | ✅ | ❌ | — | **Thrift-only.** Kernel throws; it auto-persists U2M tokens to `~/.config/databricks-sql-kernel/oauth/` and does not cache M2M. | | |||
There was a problem hiding this comment.
Resolved with a documentation edit — the reviewer's point was valid and fully actionable in this PR.
Pushed 83b91f0 (bundled with 1 other thread(s)).
eric-wang-1990
added
engineer-bot
Addresses: - #3858665815 at lib/kernel/KernelAuth.ts:840 - #3858665845 at CONNECTION_PARAMETERS.md:68 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Adds tokenCacheEnabled on the databricks-oauth option, mapped to the napi ConnectionOptions field on the U2M path. U2M-only; disabled by default (matches Thrift no-persistence posture); enable-flag only; the Thrift-only persistence custom-store hook is untouched and still rejected on the kernel path. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Addresses: - #3858665815 at lib/kernel/KernelAuth.ts:840 - #3858665845 at CONNECTION_PARAMETERS.md:68 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Bumps KERNEL_REV 5e5dea9 -> 628abd6 (databricks-sql-kernel #283, "U2M token-cache control"; additive over #282 so the Azure SP surface is preserved) and regenerates the committed napi binding, which now exposes the `tokenCacheEnabled` / `tokenCachePassphrase` fields consumed by buildKernelConnectionOptions. The only index.js churn is napi-rs's embedded version guard (0.2.0 -> 0.2.1); the export set is unchanged. Replaces the placeholder skipped U2M e2e with a real interactive suite (gated behind DATABRICKS_KERNEL_U2M_INTERACTIVE so CI still skips it) that proves, end to end against a live workspace: - tokenCacheEnabled unset -> no on-disk token written (Thrift parity) - tokenCacheEnabled: true -> an AES-encrypted (non-JSON) token is persisted under dirs::config_dir()/ databricks-sql-kernel/oauth/ Verified: 2/2 e2e passing against an Azure pecotesting workspace; 322 kernel unit tests green against the rebuilt binding. Note: published optionalDependency pins remain 0.2.0 (no 0.2.1 kernel release yet) per the PR hold; local dev and the KERNEL_REV-built CI path use the freshly built .node. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Normalizes trailing whitespace in two table rows so `prettier . --check` (the repo's lint gate) passes. No content change. Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
e1f20b4 to
5ecd802
Compare
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium
Clean, well-scoped change wiring tokenCacheEnabled through to the kernel U2M path with sensible disable-by-default parity and good unit + interactive-e2e coverage. One medium concern: the regenerated native/kernel/index.js now enforces binding version 0.2.1 while package.json still pins 0.2.0 — an intentional-but-inconsistent split the author already flags as "do not merge" until the 0.2.1 release and pin bump land together.
| const bindingPackageVersion = require('@databricks/databricks-sql-kernel-android-arm64/package.json').version | ||
| if (bindingPackageVersion !== '0.2.0' && process.env.NAPI_RS_ENFORCE_VERSION_CHECK && process.env.NAPI_RS_ENFORCE_VERSION_CHECK !== '0') { | ||
| throw new Error(`Native binding package version mismatch, expected 0.2.0 but got ${bindingPackageVersion}. You can reinstall dependencies to fix this issue.`) | ||
| if (bindingPackageVersion !== '0.2.1' && process.env.NAPI_RS_ENFORCE_VERSION_CHECK && process.env.NAPI_RS_ENFORCE_VERSION_CHECK !== '0') { |
There was a problem hiding this comment.
🟡 Medium — The regenerated napi router now expects binding version 0.2.1 in every platform branch (and the WASI branch at ~L651), but package.json optionalDependencies still pin all 8 kernel packages to 0.2.0. The two were bumped out of lockstep.
The check is gated on NAPI_RS_ENFORCE_VERSION_CHECK, so default npm consumers are unaffected — but any environment that sets that flag will now hit Native binding package version mismatch, expected 0.2.1 but got 0.2.0 and fail to load, because npm installs the pinned 0.2.0. This contradicts the PR's own stated intent to keep consumers on 0.2.0 until 0.2.1 ships.
The PR description acknowledges this ("Do not merge until the 0.2.1 release ships and the pins are bumped"), so this is a state-of-the-PR gate rather than a logic bug — flagging so the version-check bump and the dependency pins land together in the same commit that flips to 0.2.1, not split across the merge boundary.
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
Valid point, but it cannot be actioned in this job. The version-check expects 0.2.1 in native/kernel/index.js (a generated file under the denied native/ path — not writable), while package.json optionalDependencies pin the 8 kernel packages to 0.2.0. Reconciling them requires either reverting the native router to 0.2.0 or bumping the package.json pins to 0.2.1 — but native/ and package.json are both off-limits here, and the reviewer/PR description explicitly gate this on the 0.2.1 kernel release shipping ("Do not merge until 0.2.1 ships and the pins are bumped"). This is a release-coordination / merge-boundary decision for a human: the napi-router version bump and the dependency pins must land together in the commit that flips to 0.2.1, once those packages are published.
The regenerated native/kernel/index.js baked the expected binding version to 0.2.1, but package.json optionalDependencies still pin all 8 kernel packages to 0.2.0 (no 0.2.1 npm release has shipped). The guard is gated on NAPI_RS_ENFORCE_VERSION_CHECK, so default consumers are unaffected, but any environment that sets the flag would hit "expected 0.2.1 but got 0.2.0" and fail to load. Keep the version literal at 0.2.0 so the loader stays in lockstep with the pinned binding. Bump both the guard literals and the optionalDependencies pins to 0.2.1 together in the follow-up commit once 0.2.1 publishes. Co-authored-by: Isaac
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
There was a problem hiding this comment.
Verdict: 1 High
Looks solid — the tokenCacheEnabled wiring in KernelAuth.ts is correctly scoped to the U2M branch, and unit/contract changes are consistent. One high concern: the new e2e test's after cleanup can delete a developer's real kernel OAuth token cache on the skip path, because this.skip() runs before the backup is taken but the after hook still deletes the cache dir unconditionally.
The connector wires only tokenCacheEnabled through to the kernel U2M path; the passphrase surface is intentionally out of scope for this PR (enable flag only). Remove the tokenCachePassphrase type entry so the committed napi surface matches what the connector actually forwards. It can be re-added alongside real wiring when the passphrase option is surfaced. Co-authored-by: Isaac
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Addresses: - #3860354328 at tests/e2e/kernel/auth-u2m-e2e.test.ts:111 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
What & why
Adds a
tokenCacheEnabled?: booleanconnection option (on thedatabricks-oauthauth type) to control the kernel backend's U2M on-disk OAuth token cache.On the kernel path with OAuth U2M, the Rust kernel can persist the refresh token to an AES-256 encrypted on-disk cache at
dirs::config_dir()/databricks-sql-kernel/oauth/(~/Library/Application Support/...on macOS,~/.config/...on Linux), so a later process skips the interactive browser login. This wires a client option through to the napi binding'stokenCacheEnabledfield.Behavior
tokenCacheEnabled: falseto the kernel — matching Thrift's in-memory (no on-disk persistence) posture, so migrating onto the kernel path doesn't silently start writing tokens to disk. Enabling is opt-in viatokenCacheEnabled: true. (The kernel's own default is enabled; the connector always sendsfalsewhen the option is unset, so disable-by-default is the connector's deliberate choice.)persistence(customOAuthPersistencestore) is untouched and still rejected on the kernel path — this new flag is a separate, simpler control.Dependency
Consumes the napi field
tokenCacheEnabledfrom databricks-sql-kernel #283, now merged to kernelmain(merge commit628abd6). This PR bumpsKERNEL_REV5e5dea9 → 628abd6(additive over #282, so the Azure SP surface is preserved) and regenerates the committed napi binding, which now exposes the field. The kernel-e2e CI path builds the binding fromKERNEL_REV, so it picks up the field.Published npm pins stay at
0.2.0. No@databricks/databricks-sql-kernel0.2.1release has shipped yet, so bumping the 8 platformoptionalDependenciesto a nonexistent version would breaknpm install. Until that release lands (a follow-up commit here bumps the pins), npm consumers still get the0.2.0binding, which silently drops the unknown field — so disable-by-default doesn't take effect for them yet. Do not merge until the0.2.1release ships and the pins are bumped.Testing
Unit
false,true⇒true, and thepersistencerejection still throws. 322 kernel unit tests green against the rebuilt binding.Runtime E2E (new)
Added an interactive U2M E2E suite (
tests/e2e/kernel/auth-u2m-e2e.test.ts), gated behindDATABRICKS_KERNEL_U2M_INTERACTIVE+ workspace host/path so CI still skips it (the browser flow needs a human). It backs up and restores the real on-disk cache dir around the run so a developer's own cached tokens are never destroyed.Ran it against a live Azure pecotesting workspace, completing the browser login for each case:
tokenCacheEnabledunsetSELECT 1= 1tokenCacheEnabled: trueSELECT 1= 1{sha256}.jsonis written ✓Result:
2 passing (17s). Confirms disable-by-default (Thrift parity) and opt-in enable end to end through DBSQLClient → KernelBackend → napi binding → live workspace. (Azure U2M routes through the kernel's cloud-blind in-houseOAuthU2mflow, as expected — it uses the workspace's OIDC-discovered authorize endpoint verbatim.)Related: databricks-sql-kernel #283.
This pull request and its description were written by Isaac.