FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Give warning for deprecated clientId param by shivam2680 · Pull Request #282 · databricks/databricks-sql-nodejs · GitHub

Give warning for deprecated clientId param - #282

Merged
shivam2680 merged 6 commits into
mainfrom
shivam2680/add-clientId-warning
Mar 12, 2025
Merged

Give warning for deprecated clientId param#282
shivam2680 merged 6 commits into
mainfrom
shivam2680/add-clientId-warning

Conversation

Copy link
Copy Markdown
Contributor

Description

Added check in connect call to see if clientId is passed and raise warning to use userAgentEntry

Testing

Unit test added

Comment thread lib/DBSQLClient.ts
'Warning: The "clientId" option is deprecated. Please use "userAgentEntry" instead.',
);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

can we assign it to userAgentEntry if it only give the warning.

shivam2680 Feb 21, 2025
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We can,
but the issue of sensitive info being passed through this param will persist.

jackyhu-db Feb 21, 2025
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

can you try to redact it if it is service principal client id

shivam2680 merged commit cd43bcf into main Mar 12, 2025
shivam2680 deleted the shivam2680/add-clientId-warning branch March 28, 2025 08:02
eric-wang-1990 added a commit that referenced this pull request Aug 21, 2026
…n main)

kernel #282 (napi AuthMode::AzureSpM2m + C-ABI set_auth_azure_sp) merged to
databricks-sql-kernel main at 5e5dea9. Repoint KERNEL_REV from the old #280
feature-branch tip (1495174 — now orphaned; #280 was mis-merged into the
already-merged #263 branch) to the merged-main SHA, so kernel-e2e builds the
native module from a main that actually carries the Azure SP surface.

Co-authored-by: Isaac
Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
eric-wang-1990 added a commit that referenced this pull request Aug 21, 2026
Regenerate the committed napi artifacts (native/kernel/index.d.ts + index.js)
to match the bumped KERNEL_REV (kernel main 5e5dea9, which includes the Azure SP
surface via #282). index.d.ts now exposes the AuthMode.AzureSpM2m arm and the
azureClientId/azureClientSecret/azureTenantId/azureWorkspaceResourceId
ConnectionOptions fields. Equivalent to `npm run build:native`'s `cp index.*`
step (the kernel's committed napi/index.* at 5e5dea9 are the generator output);
run here because @napi-rs/cli isn't reachable from the CI npm registry.

Addresses the review thread flagging the stale generated contract.

Co-authored-by: Isaac
Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
pull Bot pushed a commit to rebeccalarner/databricks-sql-nodejs that referenced this pull request Aug 22, 2026
…el path (databricks#505)

* feat(kernel): thread Azure Entra auth (U2M + SP M2M) through the kernel path

On useKernel=true + authType='databricks-oauth', route Azure Entra auth to the
kernel instead of rejecting it. Mirrors the Thrift OAuthManager.getManager:
useDatabricksOAuthInAzure selects the flavour on an Azure host.

- useDatabricksOAuthInAzure: true -> in-house workspace-federated flow, which
  the kernel runs natively: no secret -> OAuthU2m (browser), secret -> OAuthM2m
  (workspace-OIDC client-credentials). Works against Azure workspaces.
- absent/false on an Azure host -> Entra-direct: with a secret -> the kernel's
  Azure SP M2M (AzureSpM2m; Entra SP creds ride oauthClientId/oauthClientSecret,
  azureTenantId optional/auto-discovered); without a secret -> Entra-direct
  browser U2M, which the kernel does not implement -> rejected with a pointer to
  useDatabricksOAuthInAzure: true or the Thrift backend.
- Non-Azure host: the flags are inert (in-house is the only flow).

Adds an AzureSpM2m variant to the native-options union + an isAzureHost helper.
Replaces the previous blanket Azure-OAuth rejection. Verified end-to-end
against a live Azure workspace (U2M browser flow + SP M2M data token) with a
native module built from databricks-sql-kernel#280.

Co-authored-by: Isaac
Signed-off-by: eric-wang-1990 <e.wang@databricks.com>

* ci(kernel): bump KERNEL_REV to the build carrying the Azure SP surface

Point the kernel-e2e KERNEL_REV at the kernel commit that adds the napi
AzureSpM2m surface (databricks-sql-kernel#280, stacked on databricks#263), so the
connector's kernel-e2e builds a kernel that can accept azure-sp-m2m. This is a
temporary pin to the unmerged databricks#280 branch tip; re-point to a kernel main SHA
once databricks#263 + databricks#280 merge and a kernel release is cut.

Co-authored-by: Isaac
Signed-off-by: eric-wang-1990 <e.wang@databricks.com>

* fix(kernel): route Azure U2M to the in-house flow instead of rejecting it

The prior routing rejected Azure U2M when useDatabricksOAuthInAzure was
absent/false (the 'Entra-direct' default), on the assumption the kernel needed
an Entra-direct browser U2M flow it lacks. That was wrong: the kernel runs a
single, cloud-blind in-house U2M flow — it uses the workspace's OIDC-discovered
authorize endpoint ({host}/oidc/v1/authorize) verbatim — and that
workspace-federated flow works against Azure workspaces (the workspace federates
the browser login to Entra server-side; verified E2E).

So all U2M now falls through to OAuthU2m with the in-house app
(databricks-sql-connector) + sql/offline_access, regardless of cloud or
useDatabricksOAuthInAzure — consistent with the Go connector's uniform U2M
mapping. Only Entra-direct M2M (Azure host + secret + not
useDatabricksOAuthInAzure) still maps to the kernel's azure-sp-m2m.

Updated the test that asserted the rejection to assert OAuthU2m, and the
docstring/CHANGELOG.

Co-authored-by: Isaac
Signed-off-by: eric-wang-1990 <e.wang@databricks.com>

* ai: apply changes for databricks#505 (4 review threads)

Addresses:
  - #3828516211 at lib/kernel/KernelAuth.ts:747
  - #3828516217 at lib/kernel/KernelAuth.ts:741
  - #3828530379 at lib/kernel/KernelAuth.ts:292
  - #3832331772 at lib/kernel/KernelAuth.ts:546

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>

* ci(kernel): repoint KERNEL_REV to merged main (Azure SP surface now on main)

kernel databricks#282 (napi AuthMode::AzureSpM2m + C-ABI set_auth_azure_sp) merged to
databricks-sql-kernel main at 5e5dea9. Repoint KERNEL_REV from the old databricks#280
feature-branch tip (1495174 — now orphaned; databricks#280 was mis-merged into the
already-merged databricks#263 branch) to the merged-main SHA, so kernel-e2e builds the
native module from a main that actually carries the Azure SP surface.

Co-authored-by: Isaac
Signed-off-by: eric-wang-1990 <e.wang@databricks.com>

* ai: apply changes for databricks#505 (1 review thread)

Addresses:
  - #3834110807 at lib/kernel/KernelAuth.ts:760

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>

* ai: apply changes for databricks#505 (1 review thread)

Addresses:
  - #3834140852 at lib/kernel/KernelAuth.ts:778

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>

* ai: apply changes for databricks#505 (1 review thread)

Addresses:
  - #3834161802 at CHANGELOG.md:5

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>

* ai: apply changes for databricks#505 (1 review thread)

Addresses:
  - #3834195415 at lib/kernel/KernelAuth.ts:740

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>

* chore(kernel): refresh generated native contract for KERNEL_REV 5e5dea9

Regenerate the committed napi artifacts (native/kernel/index.d.ts + index.js)
to match the bumped KERNEL_REV (kernel main 5e5dea9, which includes the Azure SP
surface via databricks#282). index.d.ts now exposes the AuthMode.AzureSpM2m arm and the
azureClientId/azureClientSecret/azureTenantId/azureWorkspaceResourceId
ConnectionOptions fields. Equivalent to `npm run build:native`'s `cp index.*`
step (the kernel's committed napi/index.* at 5e5dea9 are the generator output);
run here because @napi-rs/cli isn't reachable from the CI npm registry.

Addresses the review thread flagging the stale generated contract.

Co-authored-by: Isaac
Signed-off-by: eric-wang-1990 <e.wang@databricks.com>

* ai: apply changes for databricks#505 (1 review thread)

Addresses:
  - #3834226992 at lib/kernel/KernelAuth.ts:782

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>

* ai: apply changes for databricks#505 (1 review thread)

Addresses:
  - #3834265600 at lib/kernel/KernelAuth.ts:289

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>

* ai: apply changes for databricks#505 (1 review thread)

Addresses:
  - #3834302149 at lib/kernel/KernelAuth.ts:288

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>

* fix(kernel): regenerate native contract via build:native, not hand-copy

The prior refresh (d74ffaa) hand-copied the kernel's committed napi/index.*
into native/kernel/. Those are @napi-rs/cli v2-style output, but the kernel's
napi crate is 3.11.0, so build:native runs @napi-rs/cli@3.8.2 (v3-style) — the
format the committed contract actually uses. The hand-copy therefore regressed
index.js's loader (dropped the NAPI_RS_NATIVE_LIBRARY_PATH override, the
per-package version-consistency checks, and the win32-x64-gnu/darwin-universal
targets) and reformatted index.d.ts wholesale.

Regenerate authoritatively by running npm run build:native against the pinned
KERNEL_REV (kernel main 5e5dea9) with @napi-rs/cli@3.8.2. Result:
- index.js: byte-identical to the pre-d74ffaa baseline (a new auth mode does
  not change the platform-resolution loader) — net zero change vs the PR base.
- index.d.ts: purely additive (+158/-2) — exposes AuthMode.AzureSpM2m and the
  azureClientId/azureClientSecret/azureTenantId/azureWorkspaceResourceId fields,
  plus the OAuthM2mJwt mode, tokenUrl/jwt* and telemetry/system-config fields
  that had also drifted out of the committed contract.

Co-authored-by: Isaac
Signed-off-by: eric-wang-1990 <e.wang@databricks.com>

---------

Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Co-authored-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
eric-wang-1990 added a commit that referenced this pull request Aug 26, 2026
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>
eric-wang-1990 added a commit that referenced this pull request Aug 26, 2026
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>
pull Bot pushed a commit to rebeccalarner/databricks-sql-nodejs that referenced this pull request Aug 26, 2026
…databricks#513)

* feat: add tokenCacheEnabled to control kernel U2M on-disk token cache

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>

* ai: apply changes for databricks#513 (2 review threads)

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>

* test(kernel): bump KERNEL_REV to kernel databricks#283 and add U2M token-cache E2E

Bumps KERNEL_REV 5e5dea9 -> 628abd6 (databricks-sql-kernel databricks#283, "U2M
token-cache control"; additive over databricks#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>

* style: prettier-format CONNECTION_PARAMETERS.md

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>

* fix(kernel): keep napi version guard at 0.2.0 until 0.2.1 npm release

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
Signed-off-by: eric-wang-1990 <e.wang@databricks.com>

* chore(kernel): drop unused tokenCachePassphrase from napi index.d.ts

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
Signed-off-by: eric-wang-1990 <e.wang@databricks.com>

* ai: apply changes for databricks#513 (1 review thread)

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>

---------

Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Co-authored-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL