[bugfix] "Secret scope ACL is MANAGE for all users by default" #326

alexott · 2020-09-17T13:51:37Z

If the scope is defined with initial_manage_principal = "", then the corresponding field is omitted from the request body, and as result, users group won't get the MANAGE permission for created scope as described in Secrets API docs.

This fixes #322

TravisBuddy · 2020-09-17T13:55:15Z

Travis tests have failed

Hey @alexott,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

2nd Build

View build log

curl -sSL "https://github.com/gotestyourself/gotestsum/releases/download/v0.4.2/gotestsum_0.4.2_linux_amd64.tar.gz" | sudo tar -xz -C /usr/local/bin gotestsum

The command "curl -sSL "https://github.com/gotestyourself/gotestsum/releases/download/v0.4.2/gotestsum_0.4.2_linux_amd64.tar.gz" | sudo tar -xz -C /usr/local/bin gotestsum" exited with 0.
$ curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.25.0
golangci/golangci-lint info checking GitHub for tag 'v1.25.0'
golangci/golangci-lint info found version: 1.25.0 for v1.25.0/linux/amd64
golangci/golangci-lint info installed /home/travis/gopath/bin/golangci-lint
The command "curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.25.0" exited with 0.
$ time make lint
✓ Linting source code with golangci-lint make sure you run make fmt ...
access/resource_secret_scope.go:25: File is not `gofmt`-ed with `-s` (gofmt)
		"scope":                    scope,
Makefile:10: recipe for target 'lint' failed
make: *** [lint] Error 1

real	0m26.443s
user	0m46.558s
sys	0m1.419s
The command "time make lint" exited with 2.
$ time make test
✓ Running tests...
∅  .
✓  access (81ms) (coverage: 83.7% of statements)
✓  access/acceptance (27ms)
✓  common (61ms) (coverage: 68.2% of statements)
✓  compute/acceptance (33ms)
✓  compute (1.766s) (coverage: 72.0% of statements)
✓  identity (107ms) (coverage: 42.4% of statements)
✓  internal (18ms) (coverage: 67.5% of statements)
✓  internal/qa (12ms) (coverage: 54.0% of statements)
✓  identity/acceptance (34ms)
∅  internal/acceptance
✓  internal/sanity (21ms) (coverage: 100.0% of statements)
✓  mws (1.092s) (coverage: 70.5% of statements)
✓  mws/acceptance (25ms)
✓  provider (132ms) (coverage: 70.7% of statements)
✓  storage (99ms) (coverage: 73.5% of statements)
✓  storage/acceptance (47ms)
✓  workspace/acceptance (14ms)
✓  workspace (10.055s) (coverage: 62.9% of statements)

DONE 567 tests, 65 skipped in 35.216s

real	0m35.222s
user	0m37.691s
sys	0m4.630s
The command "time make test" exited with 0.
$ time make build
✓ Building source code with go build...

real	0m2.898s
user	0m3.540s
sys	0m0.432s
The command "time make build" exited with 0.
store build cache
changes detected (content changed, file is created, or file is deleted):\n/home/travis/.cache/go-build/01/01efa3ed220fac1988c0defea7ff4eaf1841521e410f79a00fcdab6cea4fbef4-a
/home/travis/.cache/go-build/05/05232bb1c187df04f0cc2bff55a6ab376033243d6f5686f625d0bb05bca68029-a
/home/travis/.cache/go-build/08/086e060e035a04294ddb32d69b508ff4d95cb7f9f5c24dcaa11fc92fb9f14a38-d
/home/travis/.cache/go-build/09/09304db6596ce06c97b2d40f68fe7ede3254be0062ad91503bb5f0f4376cf7c2-a
/home/travis/.cache/go-build/0b/0b2e7540acd766147b0111507396969533135c9412e2e47456dd60598b949060-a
/home/travis/.cache/go-build/0d/0d4d8e5e54822eaff90433c439c9af7a7c9fcd757acfac8061413c61e1130403-a
/home/travis/.cache/go-build/0d/0d93a7004323cb420a3d992195306b35c5a54e1148c87a3f6428252b9a884afd-a
/home/travis/.cache/go-build/0e/0ec25cc57fdf67348179a0e61dbdaeebdaa40ad8b22fa4b8d0c1ac6f18e33bff-a
/home/travis/.cache/go-build/0f/0fbc35a8f6c8118755059c47e6426d714cde4be4e9e5e43b67e38a16419853ea-d
/home/travis/.cache/go-build/0f/0fed89fef01d1d364a8a6d2dd9ad0f0875ff16ed52a4cbb39c77e22d76b94157-a
/home/trav\n...
changes detected, packing new archive
uploading PR.326/cache--linux-xenial-e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855--go-1.14.x.tgz
cache uploaded


Done. Your build exited with 1.

curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.25.0

golangci/golangci-lint info checking GitHub for tag 'v1.25.0'
golangci/golangci-lint info found version: 1.25.0 for v1.25.0/linux/amd64
golangci/golangci-lint info installed /home/travis/gopath/bin/golangci-lint

time make lint

✓ Linting source code with golangci-lint make sure you run make fmt ...
access/resource_secret_scope.go:25: File is not `gofmt`-ed with `-s` (gofmt)
		"scope":                    scope,
Makefile:10: recipe for target 'lint' failed
make: *** [lint] Error 1

real	0m26.443s
user	0m46.558s
sys	0m1.419s

TravisBuddy Request Identifier: 68711340-f8ed-11ea-addf-b708e47cc149

TravisBuddy · 2020-09-17T14:02:08Z

Hey @alexott,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 5ef46000-f8ee-11ea-addf-b708e47cc149

codecov-commenter · 2020-09-17T14:02:22Z

Codecov Report

Merging #326 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #326   +/-   ##
=======================================
  Coverage   66.59%   66.59%           
=======================================
  Files          60       60           
  Lines        6337     6337           
=======================================
  Hits         4220     4220           
  Misses       1730     1730           
  Partials      387      387

Impacted Files	Coverage Δ
access/resource_secret_scope.go	`97.26% <100.00%> (ø)`

nfx · 2020-09-17T16:44:12Z

not to forget adding this comment later:

entry within changelog.md
updated documentation for markdown file
integration and/or acceptance test

alexott · 2020-09-18T15:36:21Z

@nfx updated with a new version

TravisBuddy · 2020-09-18T15:37:51Z

Hey @alexott,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: e83d5020-f9c4-11ea-ad42-3b67a3c1de66

TravisBuddy · 2020-09-18T15:39:39Z

Hey @alexott,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 28cd5400-f9c5-11ea-ad42-3b67a3c1de66

nfx

Refactor test step into it's own integration test, so that we can test the creation of a secret scope.

access/resource_secret_scope.go

nfx · 2020-09-18T18:56:36Z

access/acceptance/secret_acl_test.go

+			name = "%s"
+            initial_manage_principal = "%s"
+		}
+		resource "databricks_secret_acl" "my_secret_acl" {


so to test this, you should only create databricks_secret_scope and check what ACLs are returned through NewSecretAclsAPI in a custom resource check. In this integration test you're changing existing resource, which is not really testing the case of the bug, so i'd ask to create new TestAcc... for it.

to rephrase previous comment - can you remove databricks_secret_acl.my_secret_acl, because this test should verify if there's no ACL for users when initial_manage_principal is not set.

access/resource_secret_scope.go

docs/resources/secret_scope.md

alexott · 2020-09-22T06:59:52Z

@nfx all comments were addressed... The code coverage change - maybe because of new functions where we don't have test cases fir if err != nil cases.

TravisBuddy · 2020-09-23T09:31:14Z

Hey @alexott,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 851cc2f0-fd7f-11ea-a706-8f9ec0eccd66

nfx

we need two different automated tests for this PR:

initial_manage_principal is not set and verify, that only admins can manage this scope.
initial_manage_principal set to group created specifically for this test and verify that only that group and admins can manage this scope.

access/acceptance/secret_acl_test.go

nfx · 2020-09-28T12:01:29Z

access/acceptance/secret_acl_test.go

+			name = "%s"
+            initial_manage_principal = "%s"
+		}
+		resource "databricks_secret_acl" "my_secret_acl" {


to rephrase previous comment - can you remove databricks_secret_acl.my_secret_acl, because this test should verify if there's no ACL for users when initial_manage_principal is not set.

nfx · 2020-09-28T12:05:33Z

access/acceptance/secret_acl_test.go

+	//var secretScope Secre
+	var secretACL ACLItem
+	scope := fmt.Sprintf("tf-scope-%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum))
+	initialPrincipal := "users"


we need two different automated tests for this PR:

initial_manage_principal is not set and verify, that only admins can manage this scope.

initial_manage_principal set to group created specifically for this test and verify that only that group and admins can manage this scope.

we can't use arbitrary group for initial_principal as per documentation:

The only supported principal for this option is the group users, which contains all users in the workspace.

and if I create any other group, and use it as initial principal, then Secrets API returns error saying that I can't use that group as initial principal

and if I create any other group, and use it as initial principal, then Secrets API returns error saying that I can't use that group as initial principal

that one is strange. what about user?

same - you can't explicitly set user as initial_principal as well, but this happens implicitly when no initial_principal is provided - I've added corresponding test that checks that user has MANAGE permission for scope if initial_principal is not provided.

But I think that we need to talk with developers to understand that design decision, I also wondering why this happens...

TravisBuddy · 2020-10-01T10:13:42Z

Travis tests have failed

Hey @alexott,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

2nd Build

View build log

curl -sSL "https://github.com/gotestyourself/gotestsum/releases/download/v0.4.2/gotestsum_0.4.2_linux_amd64.tar.gz" | sudo tar -xz -C /usr/local/bin gotestsum

travis_time:end:0d616a90:start=1601541329051515129,finish=1601541329697163302,duration=645648173,event=script

curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.25.0

golangci/golangci-lint info checking GitHub for tag 'v1.25.0'
golangci/golangci-lint info found version: 1.25.0 for v1.25.0/linux/amd64
golangci/golangci-lint info installed /home/travis/gopath/bin/golangci-lint
travis_time:end:0af73573:start=1601541329701439446,finish=1601541331675883495,duration=1974444049,event=script

time make lint

✓ Linting source code with golangci-lint make sure you run make fmt ...
access/acceptance/secret_acl_test.go:169:6: func `testSecretACLResourceExistsForScopeAndPrincipal` is unused (unused)
access/acceptance/secret_acl_test.go:94:17: unlambda: replace `func(s *terraform.State) error {

	return testSecretACLResourceDestroy(s)
}` with `testSecretACLResourceDestroy` (gocritic)
		CheckDestroy: func(s *terraform.State) error {
		              ^
access/acceptance/secret_acl_test.go:149:49: Error return value of `getSecretACLResourceExistsForScopeAndPrincipal` is not checked (errcheck)
		getSecretACLResourceExistsForScopeAndPrincipal(scope, principal, &acl)
		                                              ^
Makefile:10: recipe for target 'lint' failed
make: *** [lint] Error 1

real	0m30.743s
user	0m51.408s
sys	0m2.122s
travis_time:end:00c68e02:start=1601541331680615128,finish=1601541362426304341,duration=30745689213,event=script

TravisBuddy Request Identifier: c56bba50-03ce-11eb-b614-dd2b96524a99

TravisBuddy · 2020-10-01T11:04:05Z

Hey @alexott,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: d06d8d50-03d5-11eb-b614-dd2b96524a99

nfx · 2020-10-02T14:17:56Z

access/acceptance/secret_acl_test.go

@@ -26,6 +27,9 @@ func TestAccSecretAclResource(t *testing.T) {
 	scope := fmt.Sprintf("tf-scope-%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum))
 	principal := "users"
 	permission := "READ"
+	client := common.CommonEnvironmentClient()
+	me, _ := identity.NewUsersAPI(client).Me()


this is called "ignoring the error". we must handle errors everywhere, assert.NoError(t,err) in tests as well.

just pushed the modified version, but it was already merged - will fix in the next iteration, when will do #235

nfx self-requested a review September 17, 2020 14:01

nfx requested changes Sep 18, 2020

View reviewed changes

nfx added this to the v0.3.0 milestone Sep 21, 2020

nfx modified the milestones: v0.3.0, 0.2.6 Sep 27, 2020

nfx requested changes Sep 28, 2020

View reviewed changes

[bugfix] "Secret scope ACL is MANAGE for all users by default"

f83d4ca

nfx approved these changes Oct 2, 2020

View reviewed changes

nfx merged commit 719e774 into databricks:master Oct 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] "Secret scope ACL is MANAGE for all users by default" #326

[bugfix] "Secret scope ACL is MANAGE for all users by default" #326

alexott commented Sep 17, 2020

TravisBuddy commented Sep 17, 2020

TravisBuddy commented Sep 17, 2020

codecov-commenter commented Sep 17, 2020 •

edited

Loading

nfx commented Sep 17, 2020

alexott commented Sep 18, 2020

TravisBuddy commented Sep 18, 2020

TravisBuddy commented Sep 18, 2020

nfx left a comment

nfx Sep 18, 2020

nfx Sep 28, 2020

alexott commented Sep 22, 2020

TravisBuddy commented Sep 23, 2020

nfx left a comment

nfx Sep 28, 2020

nfx Sep 28, 2020

alexott Oct 1, 2020

nfx Oct 1, 2020

alexott Oct 1, 2020

TravisBuddy commented Oct 1, 2020

TravisBuddy commented Oct 1, 2020

nfx Oct 2, 2020

alexott Oct 2, 2020

[bugfix] "Secret scope ACL is MANAGE for all users by default" #326

[bugfix] "Secret scope ACL is MANAGE for all users by default" #326

Conversation

alexott commented Sep 17, 2020

TravisBuddy commented Sep 17, 2020

Travis tests have failed

2nd Build

TravisBuddy Request Identifier: 68711340-f8ed-11ea-addf-b708e47cc149

TravisBuddy commented Sep 17, 2020

TravisBuddy Request Identifier: 5ef46000-f8ee-11ea-addf-b708e47cc149

codecov-commenter commented Sep 17, 2020 • edited Loading

Codecov Report

nfx commented Sep 17, 2020

alexott commented Sep 18, 2020

TravisBuddy commented Sep 18, 2020

TravisBuddy Request Identifier: e83d5020-f9c4-11ea-ad42-3b67a3c1de66

TravisBuddy commented Sep 18, 2020

TravisBuddy Request Identifier: 28cd5400-f9c5-11ea-ad42-3b67a3c1de66

nfx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexott commented Sep 22, 2020

TravisBuddy commented Sep 23, 2020

TravisBuddy Request Identifier: 851cc2f0-fd7f-11ea-a706-8f9ec0eccd66

nfx left a 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

TravisBuddy commented Oct 1, 2020

Travis tests have failed

2nd Build

TravisBuddy Request Identifier: c56bba50-03ce-11eb-b614-dd2b96524a99

TravisBuddy commented Oct 1, 2020

TravisBuddy Request Identifier: d06d8d50-03d5-11eb-b614-dd2b96524a99

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 17, 2020 •

edited

Loading