Skip to content

add support for snowflake - #649

Open
Edwardvaneechoud wants to merge 5 commits into
mainfrom
feature/support-snowflake
Open

add support for snowflake#649
Edwardvaneechoud wants to merge 5 commits into
mainfrom
feature/support-snowflake

Conversation

@Edwardvaneechoud

Copy link
Copy Markdown
Owner

This pull request adds first-class support for Snowflake as a database connection in Flowfile, including both backend and documentation changes. It introduces a new extra_params field to database connections to support dialect-specific parameters (such as Snowflake's account, warehouse, and role), updates the database connection creation and usage logic to handle these parameters, and documents Snowflake usage throughout the user and API documentation. The changes also include an example integration and update the UI and feature descriptions to reflect Snowflake support.

Snowflake support and backend changes:

  • Added a new extra_params JSON column to the database_connections table (with Alembic migration) to store dialect-specific connection parameters, such as those required by Snowflake. [1] [2]
  • Updated the database connection manager to serialize, store, and parse extra_params, and to pass them through to connection creation and query execution. [1] [2] [3] [4] [5] [6]
  • Modified flow execution and SQL source resolution to unpack and use extra_params when connecting to databases, enabling support for Snowflake's required fields. [1] [2] [3] [4]
  • Ensured the Snowflake Python connector is included in PyInstaller builds for proper packaging.

Documentation and UI updates:

  • Added a full example for reading from Snowflake and documented how to configure and use Snowflake connections, including details on required parameters and column handling. [1] [2] [3]
  • Updated user documentation, feature lists, and connection setup guides to include Snowflake as a supported database throughout the UI and technical documentation. [1] [2] [3] [4] [5] [6] [7] [8]

@netlify

netlify Bot commented Aug 5, 2026

Copy link
Copy Markdown

Deploy Preview for flowfile-wasm canceled.

Name Link
🔨 Latest commit 4e42038
🔍 Latest deploy log https://app.netlify.com/projects/flowfile-wasm/deploys/6a73ed420b92db0009eef4be

@Edwardvaneechoud
Edwardvaneechoud marked this pull request as ready for review August 5, 2026 12:27
@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @Edwardvaneechoud's task in 1m 59s —— View job


Claude finished reviewing this PR

Todo list:

  • Gather context (PR body, CLAUDE.md, changed files)
  • Read root CLAUDE.md + shared/CLAUDE.md + flowfile_core CLAUDE.md
  • Review backend changes (db_connections.py, snowflake.py, migration, flow_graph.py, sql_source.py)
  • Review schema/API changes (input_schema.py, routes.py)
  • Review frontend changes (DatabaseConnectionSettings.vue, dbDialects.ts)
  • Review tests & migration coverage
  • Post final review summary

Summary

This is a well-executed, thoroughly self-consistent implementation that follows the repo's existing db_dialects registry pattern closely (mirrors the recent SQL Server dialect addition). I traced the extra_params field end-to-end — model boundary → DB → worker offload → dialect URI build → connection — and didn't find a broken link.

Correctness

  • The security-sensitive bit — guarding extra_params so it can never override credentials/target — is enforced in two independent places, as documented: at the Pydantic boundary (input_schema.py:920-928, _validate_extra_params raising ValueError for blocked keys) and again defensively at URI-build time (shared/db_dialects/base.py:57-60, is_blocked_extra_param dropping them). Good defense in depth, and both paths are tested (test_dialect_vocabulary.py, test_snowflake_dialect.py::test_build_uri_drops_blocked_extra_params).
  • extra_params correctly flows through both the local-execution path (flow_graph.py:4595/4684/4734) and the worker-offload path (ExtDatabaseConnectionflowfile_worker/external_sources/sql_source/models.py:20,47), so Snowflake reads/writes work whether or not OFFLOAD_TO_WORKER is on.
  • routes.py:790-793 correctly treats a changed extra_params as a target change for the anti-repoint-harvest guard (require_credentials_on_target_change), consistent with the sharing doctrine in the root CLAUDE.md — a manage-grantee can't silently repoint a shared Snowflake connection to a different account without re-entering credentials.
  • Project git-tracking round-trip (manifest_entries.py, importer.py, projection.py) was updated in lockstep, and test_roundtrip.py covers the Snowflake case specifically.
  • Migration 030 is idempotent (_has_column guards both upgrade/downgrade) and correctly sequenced after 029.

Conventions

  • Matches the shared/db_dialects extension pattern described in shared/CLAUDE.md (which this PR itself updates) almost exactly — no divergence from the documented contract.
  • Core still never .collect()s — the Snowflake dialect's read()/write() live in shared/db_dialects/snowflake.py, invoked from the worker or from core's local execution path only, same as other dialects.
  • Frontend: Composition API + the existing useDbDialects composable extended cleanly (extraFields/isFieldHidden), with a documented backward-compat fallback (extra_fields/hidden_fields optional in DbDialectInfo for older cores).

Tests & migrations

  • Good coverage: dialect unit tests against fakesnow (no Docker/live account needed) in shared/tests/db_dialects/test_snowflake_dialect.py, contract tests via the shared test_dialect_contract.py, endpoint tests for blocked-param rejection, and a live-account-gated integration path (FLOWFILE_TEST_SNOWFLAKE_*) that's skipped by default in CI (_snowflake_available() gate) — appropriate given no Snowflake test fixture exists in test_utils/.
  • flowfile_frame's public API (create_database_connection, create_database_connection_if_not_exists) gained the extra_params kwarg and the .pyi stub was regenerated to match exactly — satisfies the make check_stubs drift gate.
  • PyInstaller packaging (build_backends/main.py) explicitly lists snowflake.connector as a hidden import, consistent with how pymssql is handled for the same reason (lazy import inside the dialect).

Minor observations (non-blocking)

  • pyproject.toml widens the sqlglot ceiling from <28.0.0 to <31.0.0 alongside adding snowflake-connector-python. Worth a one-line confirmation in the PR description that this was verified safe (per the flowfile-change-control skill's guidance on deliberate dependency pins) — it doesn't look required by the Snowflake dialect itself (which only needs sqlglot_name = "snowflake", already supported), so if it was just picked up by poetry lock it may be worth double-checking nothing else in the lock file shifted unexpectedly.
  • shared/db_dialects/snowflake.py's write() path builds SQL by string-interpolating _ident()-quoted identifiers (schema/table/column names) — this matches the existing pattern in other dialects (identifiers aren't parameterizable in DDL), and values are bound via ? placeholders, so this isn't a new SQL-injection surface, just noting it's chema/column-name trust boundary is the same as elsewhere (caller-controlled, not end-user-controlled at that layer).

No functional issues found. Nice work keeping the migration, both execution paths (local/worker), the sharing/anti-repoint guard, project projection, frontend form, and docs all in sync in one PR.

@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @Edwardvaneechoud's task in 4m 10s —— View job


Claude finished reviewing this PR update

Todo list:

  • Gather context (PR body, CLAUDE.md, changed files)
  • Identify what changed since the last review (new commit 779aaa7)
  • Review new migration 031 (key-pair auth columns)
  • Review db_connections.py / flow_graph.py / sql_source changes for key-pair auth
  • Review schema/API changes (input_schema.py, routes.py)
  • Review frontend changes for key-pair auth UI
  • Review tests for new auth method
  • Post final review summary

Summary

Commit 779aaa7 is mislabeled ("feat: add SQL Server support...") but contains no SQL Server code at all — it actually adds Snowflake key-pair (JWT) authentication on top of the first commit's dialect. Worth renaming/squashing before merge so git log/git blame aren't misleading, but not blocking.

Correctness — found a real bug

Switching a connection's auth method back to "password" doesn't reliably discard a leftover private key, and the stale key can then be silently reused for authentication.

The intended invariant is explicit in the code's own comment (db_connections.py:151-153): "Switching away from key-pair auth: detach AND delete the key secrets, or a rotated-away (possibly compromised) key would keep authenticating silently." The implementation doesn't actually guarantee that:

  1. Frontend never clears the private-key field on an auth-method-only change. In flowfile_frontend/.../DatabaseView/DatabaseConnectionSettings.vue, connection.value.privateKey/privateKeyPassphrase are only cleared in the databaseType watcher (line ~239-266) — there's no watcher on authMethodModel. If a user picks "Key pair (JWT)", pastes a PEM, then switches back to "Password" without changing the database type, the (now-hidden) privateKey field still holds the pasted text. api.ts's toPythonFormat (line ~28-30) sends it unconditionally: private_key: connection.privateKey || undefined. So the PUT payload ends up as auth_method: "password" plus a non-empty private_key.

  2. Backend keeps it anyway. In db_connections.py:130-131:

    incoming_key = connection.private_key.get_secret_value() if connection.private_key else ""
    keeps_key_material = connection.auth_method == "key_pair" or bool(incoming_key)

    Because bool(incoming_key) is True, keeps_key_material is True even though auth_method == "password", so the "delete stale key secrets" branch (line 151-162) never runs — the private key secret is persisted and linked via private_key_id on a row whose auth_method is "password".

  3. It doesn't stay dormant — it gets used. The "reference"-mode credential path in flow_graph.py:_resolve_database_credentials (line ~1382-1388) passes through ref_settings.private_key unconditionally (no auth_method == "key_pair" gate, unlike the inline-mode branch above it at line 1345/1355). That decrypted key reaches SnowflakeDialect.build_uri, whose own fallback (shared/db_dialects/snowflake.py) is:

    use_key_pair = auth_method == "key_pair" or bool(private_key and auth_method in (None, "", "password"))

    i.e. any non-empty private_key wins even when auth_method says "password". So the connection silently authenticates via the orphaned key instead of the password the user believes they configured — exactly the scenario the removal logic was written to prevent.

test_switching_to_password_detaches_and_deletes_key_secrets in test_snowflake_source.py only exercises the case where private_key is omitted from the update payload, not the case where it's present-but-stale — so this gap isn't caught by the new tests.

Suggested fix: base keeps_key_material solely on connection.auth_method == "key_pair" (drop the or bool(incoming_key)), and add a watcher in both Vue settings components that clears privateKey/privateKeyPassphrase (and private_key_ref/private_key_passphrase_ref in the node-settings variant) whenever the auth method changes away from key_pair, not just when the database type changes.

Fix this →

Everything else looks solid

  • Migration 031_database_connection_key_pair.py follows the established idempotent pattern (_has_column guards on both upgrade/downgrade), correctly sequenced after 030.
  • _validate_auth_method / dialect auth_methods vocabulary check is a good defense-in-depth pairing with the existing extra_params blocklist (auth_method was correctly added to _BLOCKED_EXTRA_PARAMS too, so it can't be smuggled in as an extra param).
  • Delete/rotate paths correctly batch-delete all three secret ids (password, private_key, private_key_passphrase) together (delete_database_connection).
  • routes.py's anti-repoint-harvest guard was correctly extended: an auth_method change (normalized so stored NULL == "password") now also trips require_credentials_on_target_change, and has_new_credentials/has_bundled_secrets account for key material alongside the password.
  • The PEM→DER decryption in SnowflakeDialect._private_key_der keeps the passphrase out of the connector call and the key out of any file, consistent with the "core never touches disk for secrets" posture.
  • Good test coverage for the tested paths: create-requires-key, rotate, delete-cascades, interface-never-leaks-key-material, and the guard-trip test — just missing the specific gap above.

@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @Edwardvaneechoud's task in 1m 45s —— View job


Review of latest commit (12b7b7f)

Todo list:

  • Gather context (PR body, CLAUDE.md, prior review comments)
  • Identify what changed in the latest commit (12b7b7f)
  • Check whether it fixes the previously reported key-pair auth bug
  • Review the fix for correctness/completeness (backend + frontend)
  • Check test coverage for the fix
  • Post final review summary

Summary

Commit 12b7b7f directly fixes the stray-private-key bug flagged in my previous review of this PR (switching a Snowflake connection's auth method back to "password" could leave an orphaned key that silently kept authenticating). I re-traced the full path end-to-end and the fix closes it correctly.

Backend (db_connections.py:130-162)

  • keeps_key_material = connection.auth_method == "key_pair" or bool(incoming_key) is gone. The branch is now if connection.auth_method == "key_pair": ... else: <detach + delete key secrets> — so any auth method other than "key_pair" unconditionally deletes stale key secrets and ignores a stray non-empty private_key in the same payload, rather than being tricked into keeping it. The updated comment correctly documents why (build_uri infers key-pair from key presence; the reference resolver forwards whatever the row links).
  • I verified this closes the downstream propagation path too: get_database_connection_schema (line 249-262) only populates FullDatabaseConnection.private_key from db_connection.private_key_id, and flow_graph.py's reference-mode branch (_resolve_database_credentials, line 1382-1388) passes that through unconditionally — so with private_key_id now guaranteed None whenever auth_method != "key_pair", the previously-unconditional pass-through is safe in practice. The inline-mode branch (line 1343-1345) was already correctly gated on auth_method == "key_pair" and is unaffected.
  • Confirmed "password"/"key_pair" is the exhaustive vocabulary (shared/db_dialects/snowflake.py:84, base.py:92), so the binary if/else fully covers the auth-method space — no third case can fall through un-handled.

Frontend

  • databaseReader/DatabaseConnectionSettings.vue: new updateAuthMethod handler clears private_key_ref/private_key_passphrase_ref whenever the auth method changes away from key_pair, wired to the select's @change in place of the old direct updateField call.
  • views/DatabaseView/DatabaseConnectionSettings.vue: the authMethodModel computed setter now clears privateKey/privateKeyPassphrase on the same transition.
  • Both match exactly what was requested — closing the gap where only the databaseType watcher cleared these fields, not an auth-method-only change.

Tests

  • New test_switching_to_password_drops_a_stray_incoming_key in test_snowflake_source.py is the precise regression test for this bug: it submits auth_method="password" alongside a non-empty private_key/private_key_passphrase and asserts the old key secrets are deleted, the new ones are never persisted, and the reloaded schema has private_key is None. This is exactly the case the previous test suite was missing (it only covered private_key omitted from the payload).

No new issues found in this update — the fix is well-scoped, backed by the right regression test, and doesn't touch unrelated code. Nice turnaround.

One minor housekeeping note carried over from the last review: commit 779aaa7's message ("feat: add SQL Server support...") still doesn't match its actual content (Snowflake key-pair auth, no SQL Server code) — worth a git commit --amend/squash before merge so history stays accurate, but non-blocking.

@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

Claude finished @Edwardvaneechoud's task in 4m 0s —— View job


Review of latest commit (4e42038)

Todo list:

  • Gather context (PR body, CLAUDE.md, prior review comments)
  • Identify what changed since the last review (commit 12b7b7f)
  • Review new OAuth support (db_oauth.py, routes/db_oauth.py, snowflake_oauth.py)
  • Review frontend OAuth sign-in card changes
  • Review migration 032 (oauth columns)
  • Review tests for OAuth
  • Post final review summary

Summary

Despite the commit message ("show OAuth sign-in card immediately after creating a connection"), 4e42038 actually lands the entire Snowflake OAuth/SSO feature (~2175 lines): a new token-endpoint client (shared/snowflake_oauth.py), core-side token custody (db_oauth.py manager + router), migration 032, dialect wiring in shared/db_dialects/snowflake.py, worker/core wire-model plumbing, and the frontend sign-in card. The UX tweak described in the message is only the last ~30 lines (DatabaseView.vue's handleFormSubmit/handleOauthChanged). Worth a git commit --amend/reword before merge — this is the second commit in this PR with a message that doesn't match its diff (same note as the 779aaa7 review).

I focused on the areas most likely to repeat the credential-harvesting bug class found in the previous review round (stale key-pair material surviving an auth-method switch).

Security — the anti-repoint-harvest guard was correctly extended to OAuth, and defended in two independent layers

  • routes.py:799-804 now treats oauth_client_id/oauth_authorize_endpoint/oauth_token_endpoint changes as target changes, tripping require_credentials_on_target_change unless a new oauth_client_secret is supplied — this is the scenario where a manage-grantee could otherwise repoint the token endpoint to harvest the refresh token on the next silent refresh.
  • Independently, db_connections.py's update_database_connection (new oauth_target_changed block) unconditionally deletes the stored refresh token whenever the client id or either endpoint changes, regardless of whether the routes.py guard was satisfied — so even a request that supplies a plausible "new credential" alongside a repointed endpoint can't smuggle the existing refresh token to a new token endpoint, because it's wiped in the same transaction. This mirrors the two-layer defense already used for the key-pair auth-method switch.
  • Both scenarios are explicitly tested: test_client_change_drops_the_refresh_token, test_endpoint_change_without_credentials_blocked, test_endpoint_change_with_new_client_secret_allowed (test_snowflake_oauth.py).
  • Switching away from oauth (db_connections.py's new else branch) deletes both oauth_client_secret_id and oauth_refresh_token_id unconditionally based on connection.auth_method, not on whether the request happened to include stray secret values — correctly avoiding the exact bug shape flagged (and since-fixed) for key-pair auth in the prior round. test_switching_away_from_oauth_deletes_oauth_secrets covers it.
  • The dialect layer (shared/db_dialects/snowflake.py) keeps use_oauth = auth_method == "oauth" strict (no truthy-fallback like the pre-existing key-pair path has), so a stray oauth_token can't accidentally activate OAuth auth the way a stray private_key once could.
  • oauth_callback is intentionally unauthenticated (IdP redirects can't carry a JWT), with trust coming entirely from an HMAC-signed, 10-minute-TTL state token embedding user_id/connection_name; the callback still re-resolves the connection through get_database_connection(db, connection_name, user_id), so state forgery would need the JWT secret. ReconnectRequiredError is deliberately mapped to 422 (never 401), consistent with the documented "frontend treats 401 as JWT expiry" doctrine.
  • get_database_connection_schema's decrypted oauth_refresh_token/oauth_client_secret ciphertexts are only consumed internally (db_oauth.py, tests) — the interface exposed to the frontend (database_connection_interface_from_db_connection) only ever returns a boolean oauth_connected, never the secret material itself.

Correctness

  • flow_graph.py's reference-mode credential resolution mints a fresh access token via resolve_oauth_access_token at every resolve (both the worker-offload and local-execution paths get oauth_token threaded through consistently), and a rotated refresh token from the IdP is persisted back (store_refresh_token) — Okta-style rotation is handled.
  • Inline (non-reference) database settings correctly reject auth_method="oauth" (input_schema.py:986-992) since there's no interactive flow available for inline/node-level settings — must go through a stored connection.
  • Migration 032 follows the established idempotent _has_column-guarded pattern, correctly sequenced after 031.
  • Docs (connections.md) explicitly call out that the default redirect URI (http://localhost:63578/...) must be overridden in the form for non-default ports/hosts — addresses what would otherwise be a footgun for the desktop app's dynamic port allocation.

Minor / non-blocking

  • DbOauthSignInCard.vue's handleOAuthMessage correctly checks event.source !== oauthPopup rather than trusting event.origin (core and frontend can be on different origins) — good. The callback HTML's own postMessage(..., "*") (db_oauth.py's _callback_html) doesn't pin a target origin, but the only data carried is a status string and human-readable message, so this isn't a credential-leak vector, just a minor hardening nit.
  • Commit message mismatch noted above — cosmetic, but the second one in this PR (see prior review of 779aaa7).

No functional issues found in this update. The OAuth flow reuses the GA connection's signed-state pattern, and the credential-hygiene lessons from the key-pair auth review were applied proactively here rather than needing a follow-up fix.

endpoints = resolve_oauth_endpoints(db_connection)
except (HTTPException, ValueError) as e:
detail = e.detail if isinstance(e, HTTPException) else str(e)
return HTMLResponse(_callback_html("error", str(detail)), status_code=400)
)
except SnowflakeOAuthError as e:
logger.error("DB OAuth token exchange failed for %s: %s", connection_name, e)
return HTMLResponse(_callback_html("error", f"Token exchange failed: {e}"), status_code=400)
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