Skip to content

[DBA-299] Add OAuth authentication method for Snowflake adapter#275

Closed
Lenar (catstrike) wants to merge 5 commits into
mainfrom
ls/sf-oauth
Closed

[DBA-299] Add OAuth authentication method for Snowflake adapter#275
Lenar (catstrike) wants to merge 5 commits into
mainfrom
ls/sf-oauth

Conversation

@catstrike
Copy link
Copy Markdown
Collaborator

Summary

  • Add OAuth-based authentication for the Snowflake adapter as an alternative to username/password
  • Handle exclusion of SQLAlchemy-internal keys from Snowflake connection parameters
  • Add an integration example demonstrating Snowflake OAuth usage

Changes

OAuth authentication flow

Implement OAuth authentication method in the Snowflake adapter.

Affected files
  • databao/agent/databases/snowflake_adapter.py

SQLAlchemy-internal key exclusion

Filter out SQLAlchemy-internal keys when building the Snowflake connection.

Affected files
  • databao/agent/databases/snowflake_adapter.py

Integration example

Add an example script demonstrating Snowflake OAuth usage.

Affected files
  • examples/snowflake-oauth.py

Tests

Add tests for the new OAuth authentication flow.

Affected files
  • tests/test_snowflake_adapter.py

Test Plan

  • Run make test to verify existing and new tests pass
  • Manual testing with a Snowflake instance configured for OAuth

DBA-299

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds OAuth token authentication support to the Snowflake adapter (alongside existing password/key-pair/SSO flows), improves runtime-connection parsing by dropping SQLAlchemy dialect-internal keys, and includes an example plus unit tests to demonstrate/validate the new OAuth path.

Changes:

  • Add SnowflakeOAuthAuth handling in SnowflakeAdapter for secret param creation and auth parsing.
  • Filter SQLAlchemy-internal connection keys (port, autocommit) when reconstructing Snowflake configs from runtime engines.
  • Add a Snowflake OAuth usage example and unit tests for token-based auth.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

File Description
databao/agent/databases/snowflake_adapter.py Implements OAuth token auth handling and filters SQLAlchemy-internal keys when building configs from runtime connections.
tests/test_snowflake_adapter.py Adds tests for OAuth token secret params and config/auth creation.
examples/snowflake-oauth.py Demonstrates configuring a Snowflake connection using an OAuth token.
pyproject.toml Bumps databao-context-engine dependency to a version that includes SnowflakeOAuthAuth.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +50 to 54
# Keys injected by SQLAlchemy's Snowflake dialect that are not valid Snowflake connection properties.
# Note: "host" is also dialect-internal but handled separately because its value is used to derive the account.
_SQLALCHEMY_INTERNAL_KEYS = {"port", "autocommit"}

EXCLUDED_QUERY_KEYS = {*MAIN_KEYS, *AUTH_KEYS}
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

OAuth tokens are treated as auth secrets, but TOKEN_KEY ("token") isn’t included in AUTH_KEYS/EXCLUDED_QUERY_KEYS. As a result, create_config_from_runtime() will leave "token" in content and it can get persisted into additional_properties, which risks leaking the token via config serialization/logging. Add TOKEN_KEY to AUTH_KEYS (or otherwise ensure it’s excluded from additional_properties, e.g., pop it after _create_auth()).

Copilot uses AI. Check for mistakes.
assert isinstance(config.auth, SnowflakeOAuthAuth)
assert config.auth.token == "my_oauth_token"


Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

The new OAuth auth path adds token handling, but there’s no test ensuring that when building a config from a SQLAlchemy runtime connection, the OAuth token does not end up in config.additional_properties (it should remain only in config.auth). Please add a regression test for create_config_from_runtime() with token in connect_args, asserting it’s parsed into SnowflakeOAuthAuth and excluded from additional_properties.

Suggested change
def test_create_config_from_runtime_oauth_token_in_connect_args() -> None:
# Simulate a SQLAlchemy-style runtime connection where the OAuth token is
# provided via connect_args. The token should end up only in config.auth,
# not in config.additional_properties.
url = "snowflake://user:password@account/db/schema"
connect_args = {"token": "my_oauth_token"}
config = SnowflakeAdapter.create_config_from_runtime(url, connect_args=connect_args)
assert isinstance(config, SnowflakeConnectionProperties)
assert isinstance(config.auth, SnowflakeOAuthAuth)
assert config.auth.token == "my_oauth_token"
# The OAuth token must not be leaked into additional_properties.
if config.additional_properties is not None:
assert "token" not in config.additional_properties

Copilot uses AI. Check for mistakes.
SnowflakeConnectionProperties(
user=from_env("SNOWFLAKE_USER"),
account=from_env("SNOWFLAKE_ACCOUNT"),
database="CALIFORNIA_TRAFFIC_COLLISION",
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

This example hardcodes database="CALIFORNIA_TRAFFIC_COLLISION", which will fail for most users and is inconsistent with examples/snowflake-example.py (which reads the database from env). Consider reading the database (and likely warehouse) from environment variables to make the OAuth example runnable out of the box for any Snowflake account.

Suggested change
database="CALIFORNIA_TRAFFIC_COLLISION",
database=from_env("SNOWFLAKE_DATABASE"),

Copilot uses AI. Check for mistakes.
@catstrike
Copy link
Copy Markdown
Collaborator Author

These changes are already in main via PR #252

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