feat: Add authentication policies to SDK #2937

cmonty-paypal · 2024-07-12T21:30:31Z

Adds Authentication Policy methods to the SDK.

Test Plan

unit tests
integration tests

References

[Feature]: Add support for Authentication Policies #2880

cmonty-paypal · 2024-07-12T21:31:41Z

pkg/sdk/authentication_policies_def.go

+const (
+	AuthenticationMethodsAll      AuthenticationMethodsOption = "ALL"
+	AuthenticationMethodsSaml     AuthenticationMethodsOption = "SAML"
+	AuthenticationMethodsPassword AuthenticationMethodsOption = "PASSWORD"
+	AuthenticationMethodsOauth    AuthenticationMethodsOption = "OAUTH"
+	AuthenticationMethodsKeyPair  AuthenticationMethodsOption = "KEYPAIR"
+)


I wasn't sure how to get a list of an "enum" to work with QueryStruct and I couldn't find an existing pattern. If you have one let me know and I'll use these vs strings. Otherwise, I'll remove these sections.

You can look at network_rule_def.go (it's for single attribute enums, but a list with specified type should be similar). But I'm guess instead of g.KindOfT you would use g.KindOfTSlice (not sure, but strings are also fine, so you can leave it this way if the approach from network rules wouldn't apply here).

cmonty-paypal · 2024-07-13T01:29:43Z

Also, is there a way to configure the integration test client with a keypair? If not, I'll create a disposable account and use a password.

cmonty-paypal · 2024-07-15T02:52:48Z

pkg/sdk/users_test.go

 	t.Run("with unsetting a policy", func(t *testing.T) {
-		sessionPolicy := "SESSION_POLICY1"
 		opts := &AlterUserOptions{
 			name: id,
-			Set: &UserSet{
-				SessionPolicy: String(sessionPolicy),
+			Unset: &UserUnset{
+				SessionPolicy: Bool(true),
+			},
+		}
+		assertOptsValidAndSQLEquals(t, opts, "ALTER USER %s UNSET SESSION POLICY", id.FullyQualifiedName())
+	})


This test was incorrect, it was setting a policy instead of unsetting one.

sfc-gh-asawicki · 2024-07-16T07:55:24Z

Hey @cmonty-paypal. Thanks for the contribution!

We will do the review on Monday at the latest (we have a smaller team this week, sorry for the delay.)

cmonty-paypal · 2024-07-17T02:49:07Z

Hey @cmonty-paypal. Thanks for the contribution!

We will do the review on Monday at the latest (we have a smaller team this week, sorry for the delay.)

No worries. I still had some work to take care of in this PR.

pkg/sdk/authentication_policies_def.go

sfc-gh-jcieslak · 2024-07-30T14:14:02Z

pkg/sdk/authentication_policies_def.go

+const (
+	AuthenticationMethodsAll      AuthenticationMethodsOption = "ALL"
+	AuthenticationMethodsSaml     AuthenticationMethodsOption = "SAML"
+	AuthenticationMethodsPassword AuthenticationMethodsOption = "PASSWORD"
+	AuthenticationMethodsOauth    AuthenticationMethodsOption = "OAUTH"
+	AuthenticationMethodsKeyPair  AuthenticationMethodsOption = "KEYPAIR"
+)


You can look at network_rule_def.go (it's for single attribute enums, but a list with specified type should be similar). But I'm guess instead of g.KindOfT you would use g.KindOfTSlice (not sure, but strings are also fine, so you can leave it this way if the approach from network rules wouldn't apply here).

sfc-gh-jcieslak

Hey 👋
Thanks for the contribution! I left some comments, but overall the change is very good. After the changes I'll run the CI.

pkg/sdk/authentication_policies_def.go

pkg/sdk/authentication_policies_gen_test.go

pkg/sdk/authentication_policies_impl_gen.go

pkg/sdk/users.go

pkg/sdk/testint/users_integration_test.go

pkg/sdk/testint/authentication_policies_gen_integration_test.go

sfc-gh-jcieslak · 2024-08-08T11:41:51Z

Hey @cmonty-paypal 👋
Don't you mind if we take care of this, apply the needed changes, and merge it? There are many new changes incoming with new conventions that need to be applied here. The PR is already in very good shape, and we wanted to push this through a bit faster. We could take it this or next week.

cmonty-paypal · 2024-08-08T18:50:26Z

Hey @cmonty-paypal 👋 Don't you mind if we take care of this, apply the needed changes, and merge it? There are many new changes incoming with new conventions that need to be applied here. The PR is already in very good shape, and we wanted to push this through a bit faster. We could take it this or next week.

@sfc-gh-jcieslak Yes, please do! Sorry I had a bit of travel last week and I'm gonna be out of office next week. I was hoping to get to this tomorrow but if you want to take over, I'm all for it.

sfc-gh-jcieslak · 2024-08-09T07:34:54Z

No worries, Thank You! We still have to finish some other topics, but we'll get to it as soon as possible.

sfc-gh-asawicki · 2024-09-10T10:53:37Z

pkg/sdk/accounts_test.go

@@ -122,6 +122,16 @@ func TestAccountAlter(t *testing.T) {
 		assertOptsValidAndSQLEquals(t, opts, `ALTER ACCOUNT SET SESSION POLICY %s`, id.FullyQualifiedName())
 	})

+	t.Run("with set authentication policy", func(t *testing.T) {


the validation tests are missing (validations were updated but no test failed here)

Addressed in #3068

sfc-gh-asawicki · 2024-09-10T10:55:10Z

pkg/sdk/authentication_policies_impl_gen.go

+import (
+	"context"
+
+	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk/internal/collections"


this was moved to pkg/internal/collections

Resolved in the follow-up pr

sfc-gh-asawicki · 2024-09-10T10:56:26Z

pkg/sdk/poc/main.go

@@ -44,6 +44,7 @@ var definitionMapping = map[string]*generator.Interface{
 	"cortex_search_services_def.go":          sdk.CortexSearchServiceDef,
 	"data_metric_function_references_def.go": sdk.DataMetricFunctionReferenceDef,
 	"external_volumes_def.go":                sdk.ExternalVolumesDef,
+	"authentication_policies_def.go":   sdk.AuthenticationPoliciesDef,


formatting?

Resolved in the follow-up pr

sfc-gh-asawicki · 2024-09-10T10:57:52Z

pkg/sdk/testint/accounts_integration_test.go

@@ -267,6 +267,27 @@ func TestInt_AccountAlter(t *testing.T) {
 		require.NoError(t, err)
 	})

+	t.Run("set and unset authentication policy", func(t *testing.T) {


skip this test for now @sfc-gh-jcieslak - can you have multiple authentication policies on the account? if not, it messes with our account setup

Addressed in this pr, can be resolved right away

sfc-gh-asawicki · 2024-09-10T10:59:56Z

pkg/sdk/testint/authentication_policies_gen_integration_test.go

+
+	assertAuthenticationPolicy := func(t *testing.T, authenticationPolicy *sdk.AuthenticationPolicy, id sdk.SchemaObjectIdentifier, expectedComment string) {
+		t.Helper()
+		assert.NotEmpty(t, authenticationPolicy.CreatedOn)


please generate assertion in the following change @sfc-gh-jcieslak

Addressed in #3068

sfc-gh-asawicki · 2024-09-10T11:00:50Z

pkg/sdk/testint/authentication_policies_gen_integration_test.go

+		assert.Contains(t, desc, sdk.AuthenticationPolicyDescription{Property: "MFA_ENROLLMENT", Value: "REQUIRED"})
+	})
+
+	t.Run("Alter - set comment", func(t *testing.T) {


@sfc-gh-jcieslak do we need such granular tests here?

Addressed in #3068 I merged those tests into one set and unset

pkg/sdk/testint/users_integration_test.go

## Changes * Addressed comments from #2937 * Fixed failing tests caused by this change * Changed and added multiple tests connected to auth policies * Adjusted a few parts of the SDK implementation (using enums where possible, added a few missing parts, etc.) ## TODO * Mention in #2880 that the SDK for Auth Policies is ready

🤖 I have created a release *beep* *boop* --- ## [0.96.0](v0.95.0...v0.96.0) (2024-09-18) Essential GA object readiness for V1: [link](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/v1-preparations/ESSENTIAL_GA_OBJECTS.MD) ([Roadmap reference](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#preparing-essential-ga-objects-for-the-provider-v1)). :exclamation: Migration guide: [v0.95.0 -> v0.96.0](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#v0950--v0960) ### 🎉 **What's new:** * V1 redesign of resources and data sources * Row access policy ([#3066](#3066)) ([#3063](#3063)) * Resource monitor ([#3052](#3052)) ([#3064](#3064)) * Masking policy ([#3078](#3078)) ([#3083](#3083)) * SDK upgrades * External volume ([#3033](#3033)) * Authentication policy ([#2937](#2937)) ([#3068](#3068)) ([#3061](#3061)) ### 🔧 **Misc** * Clean up old test object helpers ([#3049](#3049)) * Add example of granting role to multiple objects ([#3047](#3047)) * Update readme and objects rework state ([#3046](#3046)) ### 🐛 **Bug fixes:** * Fix model grants ([#3070](#3070)) * Fix database show by and resource logic ([#3055](#3055)) * Fix default secondary roles option import ([#3041](#3041)) * Fix sweepers for warehouse and database ([#3057](#3057)) * Fix views permadiff ([#3079](#3079)) * Update v0.95.0 migration guide ([#3062](#3062)) Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>

cmonty-paypal commented Jul 12, 2024

View reviewed changes

cmonty-paypal force-pushed the add_authentication_policies branch from 35dc61b to 2cb8ee3 Compare July 12, 2024 21:32

cmonty-paypal commented Jul 15, 2024

View reviewed changes

sfc-gh-asawicki requested review from sfc-gh-asawicki and sfc-gh-jmichalak July 16, 2024 07:53

cmonty-paypal marked this pull request as ready for review July 17, 2024 03:57

cmonty-paypal force-pushed the add_authentication_policies branch from 401e8b6 to 379089f Compare July 17, 2024 03:57

cmonty-paypal added 11 commits July 17, 2024 08:23

Adds authentiation_policies_def

461ed5c

codegen and test for Authentication Policy

03e0268

more tests

7f0b0d1

finish unit tests

df67697

basic int test

7894c64

more int tests

548fa11

more int tests

fbbc3b3

Adds ALTER {ACCOUNT,USER} DDL

57b259b

add conversion functions

bd09910

passing int tests

2622636

remove unused def types

2b966a3

cmonty-paypal force-pushed the add_authentication_policies branch from 15d19dd to 2b966a3 Compare July 17, 2024 15:23

sfc-gh-jcieslak reviewed Jul 30, 2024

View reviewed changes

Merge branch 'main' into add_authentication_policies

f01f7d3

sfc-gh-asawicki requested changes Sep 10, 2024

View reviewed changes

Update accounts_integration_test.go

1a9773d

sfc-gh-jcieslak changed the title ~~[Feature] Add authentication policies to SDK~~ feat: Add authentication policies to SDK Sep 10, 2024

sfc-gh-jcieslak added 2 commits September 10, 2024 13:37

Update authentication_policies_impl_gen.go

2b60080

Update authentication_policies_impl_gen.go

cd6889d

sfc-gh-asawicki approved these changes Sep 10, 2024

View reviewed changes

sfc-gh-jcieslak merged commit 7b49685 into Snowflake-Labs:main Sep 10, 2024
6 of 8 checks passed

snowflake-release-please bot mentioned this pull request Sep 10, 2024

chore(main): release 0.96.0 #3042

Merged

sfc-gh-jcieslak mentioned this pull request Sep 12, 2024

chore: Adjust authentication policy #3068

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add authentication policies to SDK #2937

feat: Add authentication policies to SDK #2937

cmonty-paypal commented Jul 12, 2024

cmonty-paypal Jul 12, 2024 •

edited

Loading

sfc-gh-jcieslak Jul 30, 2024 •

edited

Loading

cmonty-paypal commented Jul 13, 2024

cmonty-paypal Jul 15, 2024

sfc-gh-asawicki commented Jul 16, 2024

cmonty-paypal commented Jul 17, 2024

sfc-gh-jcieslak Jul 30, 2024 •

edited

Loading

sfc-gh-jcieslak left a comment

sfc-gh-jcieslak commented Aug 8, 2024

cmonty-paypal commented Aug 8, 2024 •

edited

Loading

sfc-gh-jcieslak commented Aug 9, 2024

sfc-gh-asawicki Sep 10, 2024

sfc-gh-jcieslak Sep 12, 2024

sfc-gh-asawicki Sep 10, 2024

sfc-gh-jcieslak Sep 12, 2024

sfc-gh-asawicki Sep 10, 2024

sfc-gh-jcieslak Sep 12, 2024

sfc-gh-asawicki Sep 10, 2024

sfc-gh-jcieslak Sep 12, 2024

sfc-gh-asawicki Sep 10, 2024

sfc-gh-jcieslak Sep 12, 2024

sfc-gh-asawicki Sep 10, 2024

sfc-gh-jcieslak Sep 12, 2024

feat: Add authentication policies to SDK #2937

feat: Add authentication policies to SDK #2937

Conversation

cmonty-paypal commented Jul 12, 2024

Test Plan

References

cmonty-paypal Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

sfc-gh-jcieslak Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

cmonty-paypal commented Jul 13, 2024

Choose a reason for hiding this comment

sfc-gh-asawicki commented Jul 16, 2024

cmonty-paypal commented Jul 17, 2024

sfc-gh-jcieslak Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

sfc-gh-jcieslak left a comment

Choose a reason for hiding this comment

sfc-gh-jcieslak commented Aug 8, 2024

cmonty-paypal commented Aug 8, 2024 • edited Loading

sfc-gh-jcieslak commented Aug 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmonty-paypal Jul 12, 2024 •

edited

Loading

sfc-gh-jcieslak Jul 30, 2024 •

edited

Loading

sfc-gh-jcieslak Jul 30, 2024 •

edited

Loading

cmonty-paypal commented Aug 8, 2024 •

edited

Loading