Skip to content

feat(security_integration): complete custom OAuth field coverage - #63

Merged
noel merged 3 commits into
mainfrom
feat/custom-oauth-security-integration
Aug 19, 2026
Merged

feat(security_integration): complete custom OAuth field coverage#63
noel merged 3 commits into
mainfrom
feat/custom-oauth-security-integration

Conversation

@noel

@noel noel commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

What

Completes SnowflakeCustomOAuthSecurityIntegration (OAUTH_CLIENT = CUSTOM). PR #37 landed the resource but omitted four real CREATE SECURITY INTEGRATION options; this adds them so the full custom-OAuth surface round-trips:

  • oauth_allow_non_tls_redirect_uri (bool, FALSE)
  • oauth_single_use_refresh_tokens_required (bool, FALSE)
  • oauth_enable_role_selection (bool, FALSE)
  • oauth_any_role_mode (DISABLE/ENABLE/ENABLE_FOR_PRIVILEGE, DISABLE) + new OAuthAnyRoleMode enum

Each is wired through the spec, Props, __init__, docstring, fetch_security_integration, and the JSON fixture. Verified against the Snowflake docs that all four are valid for OAUTH_CLIENT = CUSTOM.

Also

  • Softened fetch_security_integration: a security integration type snowcap doesn't model (SAML2/SCIM/EXTERNAL_OAUTH) now logs a warning and returns None instead of raising, so one unmodeled integration can't break list/export. Partner OAuth (LOOKER/TABLEAU) still raises — it's modeled and declarable but not fetchable, so returning None would make a declared resource look absent and churn a spurious CREATE (caught in review).
  • Update path unchanged: single-field edits emit ALTER SECURITY INTEGRATION ... SET; only oauth_client_type triggers replacement (fails the plan, doesn't rotate credentials).

Tests

YAML-config resolver test, polymorphic resolver test, fetch round-trip for the new fields, and separate tests pinning the warn+None (unmodeled) vs raise (partner OAuth) paths. create_sql and fixture identity updated. Full gate green: black, ruff, codespell, mypy, 2053 passed. Integration tests need a live account (skipped here).

Reviews

code-review + ponytail run per request; the one real finding (partner-OAuth swallowed by the softened error) is fixed.

noel added 2 commits August 19, 2026 09:31
PR #37 landed custom OAuth (OAUTH_CLIENT = CUSTOM) but omitted four real
CREATE SECURITY INTEGRATION options. Add them: oauth_allow_non_tls_redirect_uri,
oauth_single_use_refresh_tokens_required, oauth_enable_role_selection, and
oauth_any_role_mode (+ OAuthAnyRoleMode enum), wired through props, __init__,
docstring, fetch, and the fixture so they round-trip.

Also soften fetch_security_integration: an unmodeled integration type now logs a
warning and returns None instead of raising, so one unknown integration can't
break list/export. Add YAML-config and polymorphic-resolver tests.
…e partner OAuth

Code-review caught that softening the fetch fallthrough to warn+None also swallowed
partner OAuth (LOOKER/TABLEAU) -- which is modeled and declarable but has no fetch
branch. Returning None made a declared partner integration look absent and plan a
spurious CREATE every apply. Raise for partner OAuth; warn+None only for genuinely
unmodeled types (SAML2/SCIM/EXTERNAL_OAUTH). Adds a test for each path.
@github-actions

Copy link
Copy Markdown

Review of PR #63

Reviewed the diff (snowcap/data_provider.py, snowcap/resources/security_integration.py, plus fixtures/tests). No correctness or security issues found.

Notes on what I checked:

  • snowcap/data_provider.py:3123-3132 — new custom-OAuth fields (oauth_allow_non_tls_redirect_uri, oauth_single_use_refresh_tokens_required, oauth_any_role_mode, oauth_enable_role_selection) are read via properties.get(...), consistent with the existing fields in this branch. Boolean coercion happens centrally in _desc_type2_result_to_dict (snowcap/data_provider.py:128-129), so no missing == "true" conversion here.
  • snowcap/data_provider.py:3131-3149 — the fallback path now distinguishes "modeled but unfetchable" (partner OAuth: raises, to avoid a spurious CREATE churn on plan/apply) from "unmodeled type" (SAML2/SCIM/EXTERNAL_OAUTH: warns + returns None, so one unsupported integration in an account doesn't abort list/export). Both branches are covered by new tests in tests/test_data_provider.py (test_unmodeled_type_returns_none_with_warning, test_partner_oauth_still_raises).
  • snowcap/resources/security_integration.py — new fields follow the existing Props/dataclass pattern exactly (spec field → docstring → Props entry → __init__ param → super().__init__ passthrough), with sensible Snowflake-matching defaults. oauth_any_role_mode reuses the ParseableEnum pattern used by OAuthUseSecondaryRoles right above it.
  • Prop insertion order in both the spec dict and generated SQL matches the updated tests/test_lifecycle.py expected DDL string.
  • Fixture (tests/fixtures/json/snowflake_custom_oauth_security_integration.json), round-trip test, YAML-config test, and polymorphic-resolution test were all updated/added consistently with the new fields.

No over-engineering, no unsafe string interpolation, no privilege/grant logic touched. This is a small, additive, well-tested change.

A live plan showed oauth_enable_role_selection drifting empty -> False every run:
Snowflake accepts it at CREATE but DESC never echoes it back, so fetch always saw
None while the spec default is False. Mark it fetchable=False (like
oauth_alternate_redirect_uris) so it's excluded from the diff. The other three new
fields do round-trip via DESC and are unchanged.
@github-actions

Copy link
Copy Markdown

Review of PR #63

Reviewed the diff (snowcap/data_provider.py, snowcap/resources/security_integration.py, and associated tests). This PR adds fetch/DDL coverage for several custom-OAuth security integration fields (oauth_allow_non_tls_redirect_uri, oauth_single_use_refresh_tokens_required, oauth_any_role_mode, oauth_enable_role_selection) and changes fetch_security_integration to warn-and-skip (return None) unmodeled integration types (e.g. SAML2/SCIM) instead of raising, while still hard-erroring for modeled-but-unfetchable partner OAuth types.

No issues found:

  • New props/fields follow the existing ResourceSpec/Props pattern exactly (snowcap/resources/security_integration.py:386-396, 510-523), matching sibling boolean/enum fields already in the class.
  • oauth_enable_role_selection and oauth_alternate_redirect_uris are both correctly marked fetchable: False for CREATE-only fields that DESC never echoes back, preventing phantom diffs (snowcap/resources/security_integration.py:387, 396).
  • The new elif oauth_client in (...) partner-OAuth branch in fetch_security_integration (snowcap/data_provider.py:3140-3148) preserves the old hard-error behavior for partner integrations (declared/declarable but unfetchable), while the new fallthrough (snowcap/data_provider.py:3149-3153) only fires for types with no corresponding resource class at all (SAML2/SCIM/EXTERNAL_OAUTH) — those can only be hit via account-wide list/export scans, never via a declared resource, so downgrading to logger.warning + None there is safe and matches the existing skip-and-continue pattern already used in snowcap/operations/export.py:88-95.
  • CREATE SQL field order in the new test expectations (tests/test_lifecycle.py:1649-1659) matches the dataclass/Props declaration order.
  • Verified locally: TestFetchSecurityIntegration, TestCreateSnowflakeCustomOAuthSecurityIntegration, TestUpdateSnowflakeCustomOAuthSecurityIntegration, test_custom_oauth_security_integration, and TestSecurityIntegrationConfig all pass (24/24).

Nothing else stood out — no injection risk (all new values flow through the existing typed Prop classes), no over-engineering, and test coverage for the new behavior (fetch round-trip, unmodeled-type skip, partner-OAuth still raises, YAML config, CREATE SQL) looks adequate.

@noel
noel merged commit 503ecfa into main Aug 19, 2026
6 checks passed
@noel
noel deleted the feat/custom-oauth-security-integration branch August 19, 2026 18:45
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