databricks_secret_scope: make initial_manage_principal empty by default

Author: alexott

access/acceptance/secret_acl_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	. "github.com/databrickslabs/databricks-terraform/access"
+	"github.com/databrickslabs/databricks-terraform/identity"
 
 	"github.com/databrickslabs/databricks-terraform/common"
 	"github.com/databrickslabs/databricks-terraform/internal/acceptance"
@@ -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()
+	userName := me.UserName
 
 	acceptance.AccTest(t, resource.TestCase{
 		CheckDestroy: testSecretACLResourceDestroy,
@@ -35,6 +39,8 @@ func TestAccSecretAclResource(t *testing.T) {
 				Config: testSecretACLResource(scope, principal, permission),
 				// compose a basic test, checking both remote and local values
 				Check: resource.ComposeTestCheckFunc(
+					// test scope permissions - it should be current user
+					testSecretScopeHasPrincipal(t, scope, userName, "MANAGE"),
 					// query the API to retrieve the tokenInfo object
 					testSecretACLResourceExists("databricks_secret_acl.my_secret_acl", &secretACL, t),
 					// verify remote values
@@ -69,6 +75,45 @@ func TestAccSecretAclResource(t *testing.T) {
 	})
 }
 
+// this test checks that any user has access when initial principal is set to 'users'
+func TestAccSecretAclResourceDefaultPrincipal(t *testing.T) {
+	// TODO: refactor for common instance pool & AZ CLI
+	if _, ok := os.LookupEnv("CLOUD_ENV"); !ok {
+		t.Skip("Acceptance tests skipped unless env 'CLOUD_ENV' is set")
+	}
+	scope := fmt.Sprintf("tf-scope-%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum))
+	client := common.CommonEnvironmentClient()
+	me, _ := identity.NewUsersAPI(client).Me()
+	userName := me.UserName
+	userPermission := "READ"
+	initialPrincipal := "users"
+	initialPermission := "MANAGE"
+	var secretACL ACLItem
+
+	acceptance.AccTest(t, resource.TestCase{
+		CheckDestroy: testSecretACLResourceDestroy,
+		Steps: []resource.TestStep{
+			{
+				// use a dynamic configuration with the random name from above
+				Config: testSecretACLResourceWithDefaultPrincipal(scope, initialPrincipal, userName, userPermission),
+				// compose a basic test, checking both remote and local values
+				Check: resource.ComposeTestCheckFunc(
+					// test scope permissions - it should be users
+					testSecretScopeHasPrincipal(t, scope, initialPrincipal, initialPermission),
+					// query the API to retrieve the tokenInfo object
+					testSecretACLResourceExists("databricks_secret_acl.my_secret_acl", &secretACL, t),
+					// verify remote values
+					testSecretACLValues(t, &secretACL, userPermission, userName),
+					// verify local values
+					resource.TestCheckResourceAttr("databricks_secret_acl.my_secret_acl", "scope", scope),
+					resource.TestCheckResourceAttr("databricks_secret_acl.my_secret_acl", "principal", userName),
+					resource.TestCheckResourceAttr("databricks_secret_acl.my_secret_acl", "permission", userPermission),
+				),
+			},
+		},
+	})
+}
+
 func testSecretACLResourceDestroy(s *terraform.State) error {
 	client := common.CommonEnvironmentClient()
 	for _, rs := range s.RootModule().Resources {
@@ -89,12 +134,37 @@ func testSecretACLResourceDestroy(s *terraform.State) error {
 
 func testSecretACLValues(t *testing.T, acl *ACLItem, permission, principal string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
-		assert.True(t, acl.Permission == ACLPermissionRead)
-		assert.True(t, acl.Principal == principal)
+		assert.EqualValues(t, permission, acl.Permission)
+		assert.EqualValues(t, principal, acl.Principal)
 		return nil
 	}
 }
 
+func testSecretScopeHasPrincipal(t *testing.T, scope, principal, permission string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		var acl ACLItem
+		err := getSecretACLResourceExistsForScopeAndPrincipal(scope, principal, &acl)
+		if err != nil {
+			return err
+		}
+		assert.EqualValues(t, permission, acl.Permission)
+		assert.EqualValues(t, principal, acl.Principal)
+		return nil
+	}
+}
+
+func getSecretACLResourceExistsForScopeAndPrincipal(scope, principal string, aclItem *ACLItem) error {
+	// retrieve the configured client from the test setup
+	conn := common.CommonEnvironmentClient()
+	resp, err := NewSecretAclsAPI(conn).Read(scope, principal)
+	if err != nil {
+		return err
+	}
+	// If no error, assign the response Widget attribute to the widget pointer
+	*aclItem = resp
+	return nil
+}
+
 // testAccCheckTokenResourceExists queries the API and retrieves the matching Widget.
 func testSecretACLResourceExists(n string, aclItem *ACLItem, t *testing.T) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
@@ -103,32 +173,36 @@ func testSecretACLResourceExists(n string, aclItem *ACLItem, t *testing.T) resou
 		if !ok {
 			return fmt.Errorf("Not found: %s", n)
 		}
-
-		// retrieve the configured client from the test setup
-		conn := common.CommonEnvironmentClient()
-		resp, err := NewSecretAclsAPI(conn).Read(rs.Primary.Attributes["scope"], rs.Primary.Attributes["principal"])
-		//t.Log(resp)
-		if err != nil {
-			return err
-		}
-
-		// If no error, assign the response Widget attribute to the widget pointer
-		*aclItem = resp
-		return nil
-		//return fmt.Errorf("Token (%s) not found", rs.Primary.ID)
+		return getSecretACLResourceExistsForScopeAndPrincipal(rs.Primary.Attributes["scope"],
+			rs.Primary.Attributes["principal"], aclItem)
 	}
 }
 
 // testAccTokenResource returns an configuration for an Example Widget with the provided name
 func testSecretACLResource(scopeName, principal, permission string) string {
 	return fmt.Sprintf(`
-		resource "databricks_secret_scope" "my_scope" {
-			name = "%s"
-		}
-		resource "databricks_secret_acl" "my_secret_acl" {
-			principal = "%s"
-			permission = "%s"
-			scope = databricks_secret_scope.my_scope.name
-		}
-		`, scopeName, principal, permission)
+        resource "databricks_secret_scope" "my_scope" {
+            name = "%s"
+        }
+        resource "databricks_secret_acl" "my_secret_acl" {
+            principal = "%s"
+            permission = "%s"
+            scope = databricks_secret_scope.my_scope.name
+        }
+        `, scopeName, principal, permission)
+}
+
+// testAccTokenResource returns an configuration for an Example Widget with the provided name
+func testSecretACLResourceWithDefaultPrincipal(scopeName, defaultPrincipal, principal, permission string) string {
+	return fmt.Sprintf(`
+        resource "databricks_secret_scope" "my_scope" {
+            name = "%s"
+            initial_manage_principal = "%s"
+        }
+        resource "databricks_secret_acl" "my_secret_acl" {
+            principal = "%s"
+            permission = "%s"
+            scope = databricks_secret_scope.my_scope.name
+        }
+        `, scopeName, defaultPrincipal, principal, permission)
 }

access/resource_secret_scope.go

@@ -21,10 +21,14 @@ type SecretScopesAPI struct {
 
 // Create creates a new secret scope
 func (a SecretScopesAPI) Create(scope string, initialManagePrincipal string) error {
-	return a.C.Post("/secrets/scopes/create", map[string]string{
-		"scope":                    scope,
-		"initial_manage_principal": initialManagePrincipal,
-	}, nil)
+	paramsMap := map[string]string{
+		"scope": scope,
+	}
+	if len(initialManagePrincipal) > 0 {
+		paramsMap["initial_manage_principal"] = initialManagePrincipal
+	}
+
+	return a.C.Post("/secrets/scopes/create", paramsMap, nil)
 }
 
 // Delete deletes a secret scope
@@ -79,8 +83,6 @@ func ResourceSecretScope() *schema.Resource {
 				Type:     schema.TypeString,
 				Optional: true,
 				ForceNew: true,
-				// or creator...
-				Default: "users",
 			},
 			"backend_type": {
 				Type:     schema.TypeString,

access/resource_secret_scope_test.go

@@ -35,7 +35,7 @@ func TestResourceSecretScopeRead(t *testing.T) {
 	assert.NoError(t, err, err)
 	assert.Equal(t, "abc", d.Id())
 	assert.Equal(t, "DATABRICKS", d.Get("backend_type"))
-	assert.Equal(t, "users", d.Get("initial_manage_principal"))
+	assert.Equal(t, "", d.Get("initial_manage_principal"))
 	assert.Equal(t, "abc", d.Get("name"))
 }
 

docs/changelog.md

@@ -15,6 +15,7 @@
 **Behavior changes**
 * State changes to legacy `spark.databricks.delta.preview.enabled` config option are [now ignored](https://github.com/databrickslabs/terraform-provider-databricks/pull/334) by `databricks_job` & `databricks_cluster`
 * Libraries, which are installed on all clusters and are not part of cluster resource definition, won't be waited for INSTALLED status
+* Fixed "[Secret scope ACL is MANAGE for all users by default](https://github.com/databrickslabs/terraform-provider-databricks/pull/326)" ([issue 322](https://github.com/databrickslabs/terraform-provider-databricks/issues/322)).  If you were relying on setting `MANAGE` permission to all users by default, you need to add `initial_manage_principal = "users"` to your `resource "databricks_secret_scope"` declaration. 
 
 ## 0.2.5
 

docs/resources/secret_scope.md

@@ -23,7 +23,7 @@ resource "databricks_secret_scope" "my-scope" {
 The following arguments are supported:
 
 * `name` - (Required) Scope name requested by the user. Scope names are unique. This field is required.
-* `initial_manage_principal` - (Optional) The principal that is initially granted `MANAGE` permission to the created scope. Defaults to `users`. Additional principals can be added with [databricks_secret_acl](secret_acl.md)
+* `initial_manage_principal` - (Optional) The principal that is initially granted `MANAGE` permission to the created scope.  If it's omitted, then the initial ACL with `MANAGE` permission applied to the scope is assigned to the API request issuer's user identity (see [documentation](https://docs.databricks.com/dev-tools/api/latest/secrets.html#create-secret-scope)).
 
 ## Attribute Reference
 
@@ -37,4 +37,4 @@ The resource secret scope can be imported using the scope name:
 
 ```bash
 $ terraform import databricks_secret_scope.object <scopeName>
-```
+```