Skip to content

feat(security_integration): implement custom OAuth integration (OAUTH_CLIENT = CUSTOM)#37

Open
GClunies wants to merge 1 commit into
datacoves:mainfrom
GClunies:snowcap-29
Open

feat(security_integration): implement custom OAuth integration (OAUTH_CLIENT = CUSTOM)#37
GClunies wants to merge 1 commit into
datacoves:mainfrom
GClunies:snowcap-29

Conversation

@GClunies

Copy link
Copy Markdown
Contributor

Summary

Implements SnowflakeCustomOAuthSecurityIntegration (TYPE = OAUTH, OAUTH_CLIENT = CUSTOM), closing #29. Custom OAuth clients — e.g. Claude.ai's Snowflake MCP connector — can now be managed declaratively instead of via manual SQL. Property changes are always applied with ALTER SECURITY INTEGRATION; the lifecycle never recreates the integration, which would rotate the Snowflake-issued client_id/client_secret and break live clients.

Changes

  • New SnowflakeCustomOAuthSecurityIntegration resource supporting oauth_client_type, oauth_redirect_uri, oauth_alternate_redirect_uris, oauth_issue_refresh_tokens, oauth_refresh_token_validity, oauth_use_secondary_roles, oauth_enforce_pkce, network_policy, pre_authorized_roles_list, blocked_roles_list; _resolver no longer raises NotImplementedError for OAuthClient.CUSTOM
  • Immutable oauth_client_type is marked triggers_replacement, so changing it fails at plan time rather than emitting an ALTER Snowflake rejects (and never auto-recreates)
  • fetch_security_integration gains a CUSTOM arm with drift-free normalization: role lists case-canonicalized and sorted on both sides, always-blocked admin roles (ALWAYS_BLOCKED_OAUTH_ROLES in builtins.py, enforced with a ValueError on the spec side) stripped from DESC output, empty values mapped to None
  • oauth_alternate_redirect_uris is create-only (fetchable=False; DESC support unconfirmed) and documented as such
  • Tests: JSON fixture auto-enrolled in the polymorphic resolver test, lifecycle unit tests (exact CREATE/ALTER SQL, validation, metadata), mocked fetch tests through the real DESC-parsing path, and a live integration round-trip test (--snowflake)
  • Docs page docs/resources/snowflake_custom_oauth_security_integration.md generated from the class docstring, wired into mkdocs.yml nav and docs/SUMMARY.md

Testing

  • pytest tests/ --ignore=tests/integration → 1552 passed
  • Live round-trip (pytest tests/integration/data_provider/test_fetch_resource.py -k custom_oauth --snowflake) still needs a run against a test account with CREATE INTEGRATION ON ACCOUNT

…_CLIENT = CUSTOM)

Add SnowflakeCustomOAuthSecurityIntegration so custom OAuth clients
(e.g. Claude.ai's Snowflake MCP connector) can be managed declaratively
instead of via manual SQL.

Property changes always apply via ALTER SECURITY INTEGRATION - never
CREATE OR REPLACE, which would rotate the Snowflake-issued
client_id/client_secret and break live clients. The immutable
oauth_client_type is marked triggers_replacement so changing it fails
at plan time instead of emitting an ALTER Snowflake rejects.

Fetch normalizes DESC output for drift-free plans: role lists are
case-canonicalized and sorted on both sides, always-blocked admin
roles (shared ALWAYS_BLOCKED_OAUTH_ROLES constant) are stripped, and
empty values map to None. oauth_alternate_redirect_uris is create-only
(not returned by DESC as far as we can confirm), documented as such.

Closes datacoves#29
@noel

noel commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Thanks @GClunies — this is a genuinely security-aware implementation, and correctly prioritizing "never rotate the Snowflake-issued secrets" is the right call. I checked out the branch and verified:

  • Secret-safety is correct — no client_id/client_secret on the spec, documented as retrieved via SYSTEM$SHOW_OAUTH_CLIENT_SECRETS.
  • Never-recreate works — no lifecycle override, so props go through the default ALTER; oauth_client_type is triggers_replacement, and since replacement isn't implemented, changing it hard-fails at plan time instead of rotating secrets.
  • ALWAYS_BLOCKED_OAUTH_ROLES is symmetric — rejected on the spec side, stripped on the fetch side, so an account echoing the admin roles in DESC won't drift.
  • Ran test_data_provider / test_lifecycle / test_resources: 385 passed, and the fetch tests exercise the real DESC parser (rows carry property_type).

Requested before merge

  1. Fetch-side role canonicalization asymmetry (correctness gap). The spec runs role names through _canonicalize_role_name (uppercase unless quoted), but the fetch arm only does sorted(...) — it relies on DESC always returning uppercase. A quoted, case-sensitive role would normalize on the spec side but not the fetch side → perpetual drift. No test covers a quoted role. Please apply _canonicalize_role_name in the fetch arm too for true symmetry (and add a quoted-role case).
  2. Run the live --snowflake round-trip. Drift-safety of the bool/int/list fields hinges on Snowflake's DESC reporting the property_type/property names the mocks assume. If e.g. OAUTH_ISSUE_REFRESH_TOKENS comes back as String rather than Boolean, the mocks still pass but live plans drift ("true" vs True). This is the gate before we rely on it.

Requested change (UX)

  1. Clearer error on oauth_client_type change. It currently surfaces as a generic NotImplementedError("replace_resource"). The intent is well-documented in the docstring, but a targeted plan-time message ("changing oauth_client_type would require recreation, which rotates client_id/client_secret and breaks live clients") would be much friendlier.

Looks good

  • oauth_alternate_redirect_uris create-only/not-fetchable is a foot-gun, but you've documented it clearly — fine as-is.
  • Good normalization coverage (admin-only→None, strips admins, sorted regardless of DESC order, empty→None).
  • Nice cleanup of the old commented-out fetch code.

Strong work — pending the live round-trip and the canonicalization fix.

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