Skip to content

feat(plugins): let the extra_fields slot hide host credential inputs - #780

Merged
debba merged 2 commits into
mainfrom
feat/connection-modal-credential-fields-slot
Sep 17, 2026
Merged

debba merged 2 commits into
mainfrom
feat/connection-modal-credential-fields-slot

Conversation

@debba

@debba debba commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

Supersedes #775 with a smaller, driver-agnostic host change. Instead of a SQL Server specific supports_integrated_auth capability plus a first-class integrated_auth connection parameter, the host only gains a generic hook on the existing connection-modal.extra_fields slot. The "Use Windows Authentication" checkbox itself moves into the SQL Server plugin as a UI extension, and its value travels in the opaque extra map that the host already persists and forwards verbatim (#596).

What changed

  • connection-modal.extra_fields slot context: added credentialFieldsHidden: boolean and setCredentialFieldsHidden(hidden: boolean). When a plugin passes true, the modal removes the username/password block and clears both values, so a stale login never reaches the driver. Passing false shows the inputs again (empty).
  • The flag is reset on every driver change (catalogue pick and connection-string import that switches driver) and when the modal is re-initialised, so it cannot leak from one driver to another.
  • Save while hidden: the modal sends the explicit empty password instead of omitting it. An untouched empty password on an existing connection is still omitted, so the "keep the stored one" behaviour of the edit dialog is unchanged.
  • Backend: keychain_utils::stored_password_change maps an omitted password to keep, an explicit "" to delete and anything else to store. update_connection deletes the keychain entry and invalidates the credential cache on "", so the next connect no longer injects the old secret; save_connection skips storing an empty password. Storing "" was not an option because the Linux keyutils store used by keyring rejects empty secrets.
  • Connection-string import: while the inputs are hidden and the driver stays the same, the login part of the string is ignored (it would otherwise sit in invisible fields and be rejected by the driver). When the import switches driver, the plugin extra map is dropped, as the catalogue driver switch already does.
  • @tabularis/plugin-api: typed context updated, version bumped to 0.1.2 (additive change, check:sync passes).
  • plugins/PLUGIN_GUIDE.md: slot table row documents the new context members and the save/import behaviour while hidden.
  • Tests: six cases in tests/components/modals/NewConnectionModal.test.tsx render a fake plugin contribution through PluginSlotContext and verify hide/clear/restore, the explicit empty password in the update_connection payload, the unchanged omission when the inputs are visible, and both import scenarios. Three unit tests cover stored_password_change.

The only backend behaviour change is the password == "" branch with keychain enabled, which the frontend never sent on edit before and which failed on Linux on create. Other callers of update_connection (built-in driver migration) resend the password loaded from the keychain, so they are unaffected. No new capability flag, no new ConnectionParams field, no changes to the built-in drivers. Existing plugins ignore the extra context members; plugins built against @tabularis/plugin-api 0.1.1 keep type-checking.

Plugin side

The SQL Server plugin ships ui/dist/index.js on this slot (gated with driver: "sqlserver"): a checkbox that writes extra.integrated_auth = "true" and calls setCredentialFieldsHidden. The Rust side resolves the flag from extra or from Integrated Security=True in a connection string, rejects a login combined with integrated authentication, and folds the flag into the pool key. On hosts without this hook the checkbox still works, only the login inputs stay visible. That work builds on TabularisDB/tabularis-sqlserver-plugin#25 and will be opened as a follow-up PR there.

Verification

  • pnpm typecheck, pnpm exec eslint on the changed files: clean
  • pnpm build:plugin-api and pnpm check:plugin-api: OK
  • pnpm exec vitest run: 271 files, 4422 tests passing (35 in the modal file, 6 new)
  • cargo test --lib keychain_utils_tests: 6 passing; clippy and rustfmt clean on the touched hunks
  • GitNexus detect-changes against the merge base: 8 files; save_connection and update_connection sit on 15 execution flows, the behaviour change is limited to the explicit empty password branch described above
  • Installed SQL Server plugin driven over stdio against a local SQL Server 2022 container: SQL authentication succeeds, integrated auth alone reaches GSSAPI and fails with the expected "run kinit" error on a machine without a ticket, integrated auth combined with a username or with a password (the keychain injection case this PR fixes) is rejected, Integrated Security=True in a connection string also reaches GSSAPI
  • Still to do: end-to-end check of the checkbox in a dev build (pnpm tauri dev with the modified plugin installed), including edit of an existing keychain-stored SQL auth connection to Windows auth, and a live SSPI test on a domain-joined Windows machine as in feat: add Windows Authentication checkbox for integrated-auth drivers #775

Credits

The Windows Authentication checkbox, the hide/clear behaviour of the login inputs and the live SSPI verification against a domain-joined SQL Server come from @egertaia's work in #775 and TabularisDB/tabularis-sqlserver-plugin#25. This PR moves that design onto a generic slot hook; @egertaia is listed as co-author on the commit.

@kilo-code-bot

kilo-code-bot Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Kilo Code Review could not run — your account is out of credits.

Add credits or switch to a free model to enable reviews on this change.

egertaia pushed a commit to egertaia/tabularis-sqlserver-plugin that referenced this pull request Sep 17, 2026
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>
@debba
debba force-pushed the feat/connection-modal-credential-fields-slot branch from ef16a27 to 8bf0680 Compare September 17, 2026 07:20
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Preview build

The preview build of commit ea3b0af failed (skipped). See the workflow run for logs.

Driver plugins that authenticate without a database login (Windows
integrated authentication, IAM tokens, Kerberos) had no way to remove the
username/password inputs from the connection form: the
connection-modal.extra_fields slot only exposed `driver`, `extra` and
`setExtraField`.

The slot context now also carries `credentialFieldsHidden` and
`setCredentialFieldsHidden(hidden)`. Hiding removes the username/password
block and clears both values so a stale login never reaches the driver.
The flag is reset whenever the driver changes (catalogue pick or
connection-string import) and when the modal is re-initialised, so it
never leaks between drivers. The plugin's own choice keeps living in the
opaque `extra` map, which is already persisted and forwarded verbatim.

No Rust changes, no new capability flag and no new ConnectionParams field:
the context additions are additive and existing plugins ignore them.
@tabularis/plugin-api is bumped to 0.1.2 for the typed context.

Co-authored-by: Egert Aia <aiaegert@gmail.com>
@debba
debba force-pushed the feat/connection-modal-credential-fields-slot branch from 8bf0680 to d1d26ac Compare September 17, 2026 07:22
…gin inputs

Hiding the host username/password inputs through the extra_fields slot
cleared the form values, but on edit an empty password was omitted from
the payload and update_connection kept the keychain entry. The next
connect injected the stale secret again and the driver rejected it with
no visible field to fix.

- The modal now sends the explicit empty password while the inputs are
  hidden; an untouched empty password is still omitted.
- keychain_utils::stored_password_change maps None to keep, "" to delete
  and anything else to store. update_connection deletes the keychain
  entry and invalidates the cache on "", save_connection skips storing
  it (the Linux keyutils store rejects empty secrets).
- A connection-string import ignores the login part while the inputs are
  hidden, and drops the plugin extra fields when it switches driver, as
  the catalogue already does.
- Docs in @tabularis/plugin-api and PLUGIN_GUIDE describe the behaviour.
- Four new modal tests and three unit tests for the helper.
@debba
debba merged commit 012aa87 into main Sep 17, 2026
3 of 4 checks passed
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.

1 participant