Skip to content

feat: support Windows/Kerberos integrated authentication - #25

Open
egertaia wants to merge 5 commits into
TabularisDB:mainfrom
egertaia:feat/windows-integrated-auth
Open

egertaia wants to merge 5 commits into
TabularisDB:mainfrom
egertaia:feat/windows-integrated-auth

Conversation

@egertaia

@egertaia egertaia commented Sep 16, 2026

Copy link
Copy Markdown

Summary

Originally implemented integrated_auth as a discrete connection field plus
supports_integrated_auth capability on core (matching #775). While testing
end-to-end, @debba noted
that #775 was superseded by TabularisDB/tabularis#780,
a smaller host-side hook: connection-modal.extra_fields gains
credentialFieldsHidden / setCredentialFieldsHidden, and the flag itself
travels through the host's existing opaque extra map instead of a new
core field. This PR rebuilds the plugin side on top of that:

  • mssql-tds-preview's default sspi/gssapi features enabled (was
    default-features = false) and AuthMethod::Integrated mapped when
    integrated_auth is set.
  • New ui/: a Vite+React IIFE bundle (per PLUGIN_GUIDE.md's UI
    Extensions section) contributing the "Use Windows Authentication"
    checkbox to connection-modal.extra_fields, gated to driver: "sqlserver"
    via the manifest. Writes extra.integrated_auth and calls
    setCredentialFieldsHidden; degrades gracefully (checkbox works, login
    inputs just stay visible) on a host without that hook.
  • ConnectionParams gains extra: HashMap<String, String>;
    resolve_connection_params resolves integrated_auth from
    extra["integrated_auth"] == "true" or from Integrated Security=True /
    Trusted_Connection=True in connection_string — either source rejects a
    combined username/password.
  • Fixed build_connection_key: it never folded auth mode into the pool
    cache key, so editing a saved connection between SQL and Windows auth
    could reuse a stale pool built under the previous credentials.
  • .tabularium: dropped the now-unused supports_integrated_auth
    capability, added the ui_extensions entry.
  • README: documents the checkbox as a UI extension instead of a discrete
    connection field; also accepts the Command Timeout keyword as a
    client-side no-op (needed for real ADO.NET/SSMS-style connection strings).

Companion host PR: TabularisDB/tabularis#780

Test plan

  • cargo test — 213 tests passing (connection.rs, pool_manager.rs, driver/pool.rs)
  • cargo clippy --all-targets -- -D warnings — clean
  • cargo fmt --check — clean
  • ui/: npm run typecheck and npm run build — clean, produces a valid IIFE bundle exposing __tabularis_plugin__
  • Manually verified end-to-end against a real domain-joined SQL Server via a Windows build (pnpm tauri dev on #780's branch + this plugin installed): checkbox appears from the plugin's own UI extension, connects via SSPI with no username/password, same as SSMS
  • Not covered by CI: Linux/macOS GSSAPI path (needs a Kerberos KDC; only SSPI was live-tested)

Egert Aia and others added 2 commits September 16, 2026 15:14
Enable the mssql-tds-preview crate's default sspi/gssapi features and
map AuthMethod::Integrated when integrated_auth is set. Accept
Integrated Security / Trusted_Connection in connection strings instead
of hard-rejecting them, and reject the combination with a
username/password instead of silently picking one.

Also accept the Command Timeout keyword as a client-side no-op,
matching the existing Connect Timeout handling.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@debba

debba commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Thanks for this, the connection-string handling and the mssql-tds feature switch are exactly what was needed.

I made some changes on top of your branch to move the UI part into the plugin instead of core: the "Use Windows Authentication" checkbox is now a plugin UI extension on the connection-modal.extra_fields slot, the flag is read from the host's opaque extra map (extra.integrated_auth) as well as from Integrated Security=True in connection strings, and the supports_integrated_auth capability goes away. The matching host change is TabularisDB/tabularis#780.

Tomorrow I will look at how best to integrate this with your work here (most likely as a follow-up on top of this PR) and I will keep you posted. A live SSPI re-test on your side once that lands would be very welcome.

Egert Aia and others added 3 commits September 17, 2026 10:04
TabularisDB/tabularis#780 supersedes the host-side capability/field
approach from #775 with a generic connection-modal.extra_fields hook
(credentialFieldsHidden/setCredentialFieldsHidden) plus the existing
opaque extra map, so no core schema change is needed. Rebuild this
plugin's side on top of that:

- Add ui/, a Vite+React IIFE bundle (per PLUGIN_GUIDE.md) contributing
  the "Use Windows Authentication" checkbox to connection-modal.extra_fields,
  gated to driver "sqlserver". It writes extra.integrated_auth and calls
  setCredentialFieldsHidden; degrades to a visible-but-unhidden checkbox
  on hosts without that hook.
- ConnectionParams gains extra: HashMap<String, String>; resolve_connection_params
  now also resolves integrated_auth from extra["integrated_auth"] == "true",
  in addition to the existing Integrated Security=True connection-string path.
  Restructured the early-return so this works without a connection string.
- Fix build_connection_key: it never folded auth mode into the pool cache
  key, so editing a saved connection between SQL and Windows auth could
  reuse a stale pool built under the previous credentials.
- .tabularium: drop the now-unused supports_integrated_auth capability,
  add the ui_extensions entry for the new checkbox.
- README: document the checkbox as a UI extension instead of a discrete
  connection field.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Four parallel reviews (code quality, test coverage, silent failures,
comment accuracy) on the current diff surfaced two real correctness
bugs and a CI gap, all fixed here:

- resolve_connection_params: a connection-string Integrated Security=False
  silently overrode an extra["integrated_auth"]=true with no error, unlike
  every other dual-sourced field in this function (which reconcile/reject
  on conflict). Now raises a contradiction error instead.
- extra["integrated_auth"] used a brittle exact match against the literal
  string "true", unlike this file's own parse_bool convention used for the
  same flag in connection strings (case-insensitive, hard-errors on
  garbage instead of silently defaulting to SQL auth). Now reuses parse_bool.
- ui/ was never typechecked or built in CI — only in release.yml, so a
  TypeScript error would first surface at release time. Added a ci.yml job
  mirroring explain-package's pattern, and switched release.yml to `npm ci`
  since ui/package-lock.json is committed.
- Added the missing test coverage the reviews identified: extra-map +
  connection-string agreeing/disagreeing on integrated_auth, and a
  driver/pool.rs test for the AuthMethod::Integrated branch.
- Fixed two doc comments left inaccurate by earlier edits in this branch
  (pool_manager.rs's illustrative key layout and README wording).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cut the ratio of prose to code introduced across this branch's edits.
No behavior change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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