-
Notifications
You must be signed in to change notification settings - Fork 50
feat: add tokenCacheEnabled to control kernel U2M on-disk token cache #513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
386c30b
feat: add tokenCacheEnabled to control kernel U2M on-disk token cache
eric-wang-1990 1f7dcde
ai: apply changes for #513 (2 review threads)
peco-engineer-bot[bot] e2c4585
test(kernel): bump KERNEL_REV to kernel #283 and add U2M token-cache E2E
eric-wang-1990 5ecd802
style: prettier-format CONNECTION_PARAMETERS.md
eric-wang-1990 a2013a9
fix(kernel): keep napi version guard at 0.2.0 until 0.2.1 npm release
eric-wang-1990 e4c92da
chore(kernel): drop unused tokenCachePassphrase from napi index.d.ts
eric-wang-1990 f3f9eaf
ai: apply changes for #513 (1 review thread)
peco-engineer-bot[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| ef1a6f2dbe72c66a9b6eefc4ca1ff31788f1efa9 | ||
| 628abd6f5045897efcadb38ec77a1e9e0c23544e | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -251,6 +251,7 @@ export type KernelNativeConnectionOptions = KernelSessionDefaults & | |
| oauthRedirectPort: number; | ||
| oauthScopes?: Array<string>; | ||
| oauthClientId?: string; | ||
| tokenCacheEnabled?: boolean; | ||
| } | ||
| | { | ||
| hostName: string; | ||
|
|
@@ -669,6 +670,7 @@ export function buildKernelConnectionOptions(options: ConnectionOptions): Kernel | |
| oauthScopes?: Array<string>; | ||
| azureTenantId?: string; | ||
| useDatabricksOAuthInAzure?: boolean; | ||
| tokenCacheEnabled?: boolean; | ||
| persistence?: unknown; | ||
| }; | ||
|
|
||
|
|
@@ -820,8 +822,9 @@ export function buildKernelConnectionOptions(options: ConnectionOptions): Kernel | |
| throw new HiveDriverError( | ||
| 'kernel backend: `persistence` (custom OAuth token store) is not yet wired through ' + | ||
| '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 kernel offers an optional built-in on-disk token cache at ' + | ||
| '`~/.config/databricks-sql-kernel/oauth/`, controlled by `tokenCacheEnabled` ' + | ||
| '(disabled by default); ' + | ||
| "the JS-supplied hook (matching thrift's `OAuthPersistence` interface) lands " + | ||
| 'when the kernel exposes it.', | ||
| ); | ||
|
|
@@ -833,6 +836,9 @@ export function buildKernelConnectionOptions(options: ConnectionOptions): Kernel | |
| // Scopes default to Thrift parity (`sql offline_access`); overridable. | ||
| oauthScopes: | ||
| Array.isArray(oauth.oauthScopes) && oauth.oauthScopes.length > 0 ? oauth.oauthScopes : U2M_DEFAULT_SCOPES, | ||
| // 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, | ||
|
Comment on lines
+839
to
+841
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed the misleading error wording and confirmed the mocked U2M auth suite still passes. Pushed 83b91f0 (bundled with 1 other thread(s)). |
||
| }; | ||
| // clientId: Thrift uses `oauthClientId ?? default`. Forward it verbatim | ||
| // when set; when absent the napi applies the same default | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Medium — The
KERNEL_REVbump exposestokenCacheEnabledin the CI-built binding, but the publishedoptionalDependenciesbinding stays pinned at@databricks/databricks-sql-kernel@0.2.0, which does not know the field and silently drops it. For npm consumers on the kernel U2M path, dropping the connector-suppliedtokenCacheEnabled: falsemeans the kernel falls back to its own default — cache enabled — so the U2M refresh token gets persisted to disk. That is the exact opposite of the "disabled by default / silent-no-persist parity" security posture this PR documents and its unit tests assert. This is a real, non-obvious behavior gap (security-relevant: tokens land on disk unexpectedly), not just a version-string mismatch. The author has already flagged this as a hard "Do not merge until 0.2.1 pins land" gate — surfacing it here so the merge gate isn't lost: confirm the0.2.1release has shipped and all 8 platform pins are bumped in the same change before this merges.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid, security-relevant merge gate, but not actionable in this PR. The connector code is correct (KernelAuth.ts:841 sends tokenCacheEnabled: false by default), but resolving the gap requires bumping the 8 @databricks/databricks-sql-kernel platform pins from 0.2.0 to 0.2.1 in package.json/package-lock.json (both out of scope / non-writable for this job) AND the 0.2.1 kernel release actually shipping to the registry with a tokenCacheEnabled-aware binding (out-of-band). Neither can ride in this KERNEL_REV change, and the on-disk-token behavior can only be confirmed by the live e2e suite this job can't run. HUMAN MUST CONFIRM before merge: (1) @databricks/databricks-sql-kernel@0.2.1 is published, (2) all 8 optionalDependencies pins in package.json + package-lock.json are bumped to 0.2.1 in the same change, and (3) kernel U2M does not persist refresh tokens with tokenCacheEnabled: false.