feat: OAuth security integration for partner applications #763

gouline · 2021-11-19T11:59:58Z

Implementing OAuth security integration, largely based on the existing SCIM security integration that's quite similar.

This one only covers the partner applications (i.e. Tableau Desktop, Tableau Server and Looker), however extending it to support custom OAuth clients would just involve adding required and optional parameters.

Test Plan

acceptance tests

References

edulop91

just a couple of quick comments, thank you!

edulop91 · 2021-11-22T20:40:35Z

pkg/resources/oauth_integration.go

+	"oauth_refresh_token_validity": {
+		Type:        schema.TypeInt,
+		Optional:    true,
+		Description: "Specifies how long refresh tokens should be valid (in seconds). OAUTH_ISSUE_REFRESH_TOKENS must be set to TRUE.",


do we need oauth_issue_refresh_tokens or would the presence/absence of oauth_refresh_token_validity give us enough information?

I see that keeping both would more closely match Snowflake's api, so we might want to keep as is

Looks like oauth_issue_refresh_tokens is true by default, so you need it here if you want to set it to false. However, oauth_refresh_token_validity is optional too, it defaults to various values depending on the client (10 hours for Tableau Desktop, 90 days for Tableau Server, etc) - there will be cases where people omit both or specify only one.

pkg/resources/oauth_integration.go

Co-authored-by: Eduardo Lopez <[email protected]>

edulop91 · 2021-11-23T16:01:22Z

pkg/resources/oauth_integration.go

+		stmt.SetString(`OAUTH_USE_SECONDARY_ROLES`, d.Get("oauth_use_secondary_roles").(string))
+	}
+	if _, ok := d.GetOk("blocked_roles_list"); ok {
+		stmt.SetStringList(`BLOCKED_ROLES_LIST`, expandStringList(d.Get("blocked_roles_list").([]interface{})))


I think this'll now panic because blocked_roles_list is a set, not []interface{}

I can't get acceptance tests to run locally, so hopefully this fixes it. Found a similar instance in network policies and copied it:

expandStringList(d.Get("blocked_roles_list").(*schema.Set).List())

edulop91 · 2021-11-23T16:07:30Z

/ok-to-test sha=d10af87

github-actions · 2021-11-23T16:16:32Z

Integration tests failure for d10af87

edulop91 · 2021-11-23T16:27:55Z

Integration tests failure for d10af87

Looks like there might be a panic here: https://github.com/chanzuckerberg/terraform-provider-snowflake/runs/4301715646?check_suite_focus=true#step:9:1050 (not sure if this log is hidden behind GitHub access controls)

alldoami · 2021-11-24T23:30:36Z

/ok-to-test sha=3eb42aa

github-actions · 2021-11-24T23:38:32Z

Integration tests failure for 3eb42aa

gouline · 2021-11-25T00:19:50Z

CI step integration-trusted failed but I have no access to it, so cannot see what is causing it.

alldoami · 2021-11-29T19:34:24Z

=== CONT  TestAcc_OAuthIntegration
panic: interface conversion: interface {} is nil, not bool

goroutine 60401 [running]:
github.com/chanzuckerberg/terraform-provider-snowflake/pkg/resources.CreateOAuthIntegration(0x0, {0x14b1940, 0xc002a0c680})
	/home/runner/work/terraform-provider-snowflake/terraform-provider-snowflake/pkg/resources/oauth_integration.go:99 +0x7c5
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).create(0x1330a20, {0x17f2748, 0xc000f6ce80}, 0x2, {0x14b1940, 0xc002a0c680})
	/home/runner/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource.go:318 +0x178
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).Apply(0xc00073cb60, {0x17f2748, 0xc000f6ce80}, 0xc00053c270, 0xc003091580, {0x14b1940, 0xc002a0c680})
	/home/runner/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource.go:456 +0x871
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*GRPCProviderServer).ApplyResourceChange(0xc003b0c1b0, {0x17f2748, 0xc000f6ce80}, 0xc003e37900)
	/home/runner/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/grpc_provider.go:977 +0xd8a
github.com/hashicorp/terraform-plugin-go/tfprotov5/tf5server.(*server).ApplyResourceChange(0xc0026ff4e0, {0x17f27f0, 0xc001833b60}, 0x4423e0)
	/home/runner/go/pkg/mod/github.com/hashicorp/[email protected]/tfprotov5/tf5server/server.go:332 +0x6c
github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5._Provider_ApplyResourceChange_Handler({0x14722e0, 0xc0026ff4e0}, {0x17f27f0, 0xc001833b60}, 0xc002634e40, 0x0)
	/home/runner/go/pkg/mod/github.com/hashicorp/[email protected]/tfprotov5/internal/tfplugin5/tfplugin5_grpc.pb.go:380 +0x170
google.golang.org/grpc.(*Server).processUnaryRPC(0xc00217e540, {0x180adc8, 0xc0025a7b00}, 0xc00299ca20, 0xc0036934a0, 0x2152ee0, 0x0)
	/home/runner/go/pkg/mod/google.golang.org/[email protected]/server.go:1279 +0xccf
google.golang.org/grpc.(*Server).handleStream(0xc00217e540, {0x180adc8, 0xc0025a7b00}, 0xc00299ca20, 0x0)
	/home/runner/go/pkg/mod/google.golang.org/[email protected]/server.go:1608 +0xa2a
google.golang.org/grpc.(*Server).serveStreams.func1.2()
	/home/runner/go/pkg/mod/google.golang.org/[email protected]/server.go:923 +0x98
created by google.golang.org/grpc.(*Server).serveStreams.func1
	/home/runner/go/pkg/mod/google.golang.org/[email protected]/server.go:921 +0x294
FAIL	github.com/chanzuckerberg/terraform-provider-snowflake/pkg/resources	356.529s

When you click on the link in this comment, what does it say? #763 (comment)

gouline · 2021-11-30T01:34:25Z

When you click on the link in this comment, what does it say? #763 (comment)

Sorry, this works. I was previously clicking on 'integration-trusted' in the footer (force of habit).

gouline · 2021-11-30T03:13:02Z

Found the problem! Silly copy-paste typo 🤦

alldoami · 2021-11-30T04:48:15Z

/ok-to-test sha=ba75402

github-actions · 2021-11-30T04:55:21Z

Integration tests failure for ba75402

gouline · 2021-11-30T05:26:28Z

Hmm, are these all meant to be treated as strings and parsed to booleans/integers? https://github.com/gouline/terraform-provider-snowflake/blob/161f112/pkg/resources/oauth_integration.go#L186-L201

case "OAUTH_ISSUE_REFRESH_TOKENS":
	if err = d.Set("oauth_issue_refresh_tokens", v.(bool)); err != nil {
		return errors.Wrap(err, "unable to set OAuth issue refresh tokens for security integration")
	}
case "OAUTH_REFRESH_TOKEN_VALIDITY":
	if err = d.Set("oauth_refresh_token_validity", v.(int64)); err != nil {
		return errors.Wrap(err, "unable to set OAuth refresh token validity for security integration")
	}
case "OAUTH_USE_SECONDARY_ROLES":
	if err = d.Set("oauth_use_secondary_roles", v.(string)); err != nil {
		return errors.Wrap(err, "unable to set OAuth use secondary roles for security integration")
	}
case "BLOCKED_ROLES_LIST":
	if err = d.Set("blocked_roles_list", strings.Split(v.(string), ",")); err != nil {
		return errors.Wrap(err, "unable to set blocked roles list for security integration")
	}

I assumed from the ENABLED parameter fed as a boolean in all unit tests, they should be treated as booleans/integers, but ENABLED is always ignored in the switch statement so it might just be copy-pasted mistake that nobody picked up?

descRows := sqlmock.NewRows([]string{
		"property", "property_type", "property_value", "property_default",
	}).AddRow("ENABLED", "Boolean", true, false)

Latest commit treats them as strings, so hopefully that works 🤷

alldoami · 2021-11-30T18:31:56Z

/ok-to-test sha=c165355

github-actions · 2021-11-30T18:41:55Z

Integration tests failure for c165355

gouline · 2021-11-30T23:31:49Z

Previous error looks fixed now, latest commit should fix the new error.

alldoami · 2021-12-01T00:20:47Z

/ok-to-test sha=e09bf36

github-actions · 2021-12-01T00:30:05Z

Integration tests failure for e09bf36

gouline · 2021-12-01T00:43:24Z

Should be fixed again.

alldoami · 2021-12-01T01:25:18Z

/ok-to-test sha=c2634c8

github-actions · 2021-12-01T01:33:34Z

Integration tests failure for c2634c8

gouline · 2021-12-01T02:14:02Z

This one was interesting: blocked_roles_list should only include roles not already implicitly enforced (ACCOUNTADMIN, ORGADMIN, SECURITYADMIN) on create, so I had to filter them out from the read. Should work now.

alldoami · 2021-12-01T05:23:24Z

/ok-to-test sha=e40332d

github-actions · 2021-12-01T05:32:48Z

Integration tests success for e40332d

…Labs#763)

gouline added 4 commits November 19, 2021 22:45

Update terraform-plugin-docs to support Apple M1

cbf5de6

Implement OAuth partner integrations

843fb2a

Unit and acceptance tests

5b4cdd8

Add documentation on oauth_use_secondary_roles

f6fe604

gouline marked this pull request as ready for review November 19, 2021 12:27

gouline added 3 commits November 20, 2021 00:23

Fix tests

b10c1ab

Undo go module changes

a9894bb

Fix typo and regenerate docs

7142662

alldoami requested a review from edulop91 November 19, 2021 19:59

edulop91 reviewed Nov 22, 2021

View reviewed changes

Update pkg/resources/oauth_integration.go

d10af87

Co-authored-by: Eduardo Lopez <[email protected]>

edulop91 reviewed Nov 23, 2021

View reviewed changes

gouline added 3 commits November 24, 2021 15:30

Update casting for set type

88c819e

Merge branch 'main' into main

53fc3b1

Update set type doc

3eb42aa

Merge branch 'main' into main

480c7b9

gouline added 2 commits November 30, 2021 14:12

Fix silly typo and enhance acceptance test

161f112

Merge branch 'main' into main

ba75402

Treat property list values as strings

c165355

gouline added 2 commits December 1, 2021 10:30

Change OAuth integration type for acceptance test

5033fa6

Merge branch 'main' into main

e09bf36

Add oauth_use_secondary_roles default for OAuth integration

c2634c8

Filter out implicit values from blocked_roles_list

e40332d

alldoami approved these changes Dec 1, 2021

View reviewed changes

alldoami merged commit 0ec5f4e into Snowflake-Labs:main Dec 1, 2021

dapearce mentioned this pull request Dec 9, 2021

Support Custom OAuth Integration Type #785

Closed

daniepett pushed a commit to daniepett/terraform-provider-snowflake that referenced this pull request Feb 9, 2022

feat: OAuth security integration for partner applications (Snowflake-…

0cf4b52

…Labs#763)

snowflake-release-please bot mentioned this pull request May 25, 2022

chore(main): release 0.34.0 #1022

Merged

This was referenced Nov 1, 2022

chore(main): release 0.34.0 #1329

Closed

chore(main): release 0.34.0 #1330

Closed

chore(main): release 0.49.0 #1332

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: OAuth security integration for partner applications #763

feat: OAuth security integration for partner applications #763

gouline commented Nov 19, 2021 •

edited

Loading

edulop91 left a comment

edulop91 Nov 22, 2021

edulop91 Nov 22, 2021

gouline Nov 22, 2021

edulop91 Nov 23, 2021

gouline Nov 24, 2021

edulop91 commented Nov 23, 2021

github-actions bot commented Nov 23, 2021

edulop91 commented Nov 23, 2021

alldoami commented Nov 24, 2021

github-actions bot commented Nov 24, 2021

gouline commented Nov 25, 2021

alldoami commented Nov 29, 2021

gouline commented Nov 30, 2021

gouline commented Nov 30, 2021

alldoami commented Nov 30, 2021

github-actions bot commented Nov 30, 2021

gouline commented Nov 30, 2021 •

edited

Loading

alldoami commented Nov 30, 2021

github-actions bot commented Nov 30, 2021

gouline commented Nov 30, 2021

alldoami commented Dec 1, 2021

github-actions bot commented Dec 1, 2021

gouline commented Dec 1, 2021

alldoami commented Dec 1, 2021

github-actions bot commented Dec 1, 2021

gouline commented Dec 1, 2021

alldoami commented Dec 1, 2021

github-actions bot commented Dec 1, 2021

feat: OAuth security integration for partner applications #763

feat: OAuth security integration for partner applications #763

Conversation

gouline commented Nov 19, 2021 • edited Loading

Test Plan

References

edulop91 left a comment

Choose a reason for hiding this comment

edulop91 Nov 22, 2021

Choose a reason for hiding this comment

edulop91 Nov 22, 2021

Choose a reason for hiding this comment

gouline Nov 22, 2021

Choose a reason for hiding this comment

edulop91 Nov 23, 2021

Choose a reason for hiding this comment

gouline Nov 24, 2021

Choose a reason for hiding this comment

edulop91 commented Nov 23, 2021

github-actions bot commented Nov 23, 2021

edulop91 commented Nov 23, 2021

alldoami commented Nov 24, 2021

github-actions bot commented Nov 24, 2021

gouline commented Nov 25, 2021

alldoami commented Nov 29, 2021

gouline commented Nov 30, 2021

gouline commented Nov 30, 2021

alldoami commented Nov 30, 2021

github-actions bot commented Nov 30, 2021

gouline commented Nov 30, 2021 • edited Loading

alldoami commented Nov 30, 2021

github-actions bot commented Nov 30, 2021

gouline commented Nov 30, 2021

alldoami commented Dec 1, 2021

github-actions bot commented Dec 1, 2021

gouline commented Dec 1, 2021

alldoami commented Dec 1, 2021

github-actions bot commented Dec 1, 2021

gouline commented Dec 1, 2021

alldoami commented Dec 1, 2021

github-actions bot commented Dec 1, 2021

gouline commented Nov 19, 2021 •

edited

Loading

gouline commented Nov 30, 2021 •

edited

Loading