[bugfix] "Secret scope ACL is MANAGE for all users by default" #326
Conversation
Travis tests have failedHey @alexott, 2nd Buildcurl -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 gotestsumcurl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.25.0time make lintTravisBuddy Request Identifier: 68711340-f8ed-11ea-addf-b708e47cc149 |
|
Hey @alexott, TravisBuddy Request Identifier: 5ef46000-f8ee-11ea-addf-b708e47cc149 |
Codecov Report
@@ Coverage Diff @@
## master #326 +/- ##
=======================================
Coverage 66.59% 66.59%
=======================================
Files 60 60
Lines 6337 6337
=======================================
Hits 4220 4220
Misses 1730 1730
Partials 387 387
|
|
not to forget adding this comment later:
|
|
@nfx updated with a new version |
|
Hey @alexott, TravisBuddy Request Identifier: e83d5020-f9c4-11ea-ad42-3b67a3c1de66 |
|
Hey @alexott, TravisBuddy Request Identifier: 28cd5400-f9c5-11ea-ad42-3b67a3c1de66 |
access/acceptance/secret_acl_test.go
Outdated
| name = "%s" | ||
| initial_manage_principal = "%s" | ||
| } | ||
| resource "databricks_secret_acl" "my_secret_acl" { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 all comments were addressed... The code coverage change - maybe because of new functions where we don't have test cases fir |
|
Hey @alexott, TravisBuddy Request Identifier: 851cc2f0-fd7f-11ea-a706-8f9ec0eccd66 |
There was a problem hiding this comment.
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
Outdated
| name = "%s" | ||
| initial_manage_principal = "%s" | ||
| } | ||
| resource "databricks_secret_acl" "my_secret_acl" { |
There was a problem hiding this comment.
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/acceptance/secret_acl_test.go
Outdated
| //var secretScope Secre | ||
| var secretACL ACLItem | ||
| scope := fmt.Sprintf("tf-scope-%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)) | ||
| initialPrincipal := "users" |
There was a problem hiding this comment.
we need two different automated tests for this PR:
initial_manage_principalis not set and verify, that only admins can manage this scope.initial_manage_principalset to group created specifically for this test and verify that only that group and admins can manage this scope.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
Travis tests have failedHey @alexott, 2nd Buildcurl -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 gotestsumcurl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.25.0time make lintTravisBuddy Request Identifier: c56bba50-03ce-11eb-b614-dd2b96524a99 |
|
Hey @alexott, TravisBuddy Request Identifier: d06d8d50-03d5-11eb-b614-dd2b96524a99 |
| @@ -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() | |||
There was a problem hiding this comment.
this is called "ignoring the error". we must handle errors everywhere, assert.NoError(t,err) in tests as well.
There was a problem hiding this comment.
just pushed the modified version, but it was already merged - will fix in the next iteration, when will do #235
If the scope is defined with
initial_manage_principal = "", then the corresponding field is omitted from the request body, and as result,usersgroup won't get theMANAGEpermission for created scope as described in Secrets API docs.This fixes #322