refactoring + tests

bundle/config/validate/folder_permissions.go

@@ -2,7 +2,6 @@ package validate
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"path"
 	"strings"
@@ -14,6 +13,7 @@ import (
 	"github.com/databricks/databricks-sdk-go/apierr"
 	"github.com/databricks/databricks-sdk-go/service/workspace"
 	"golang.org/x/sync/errgroup"
+	"golang.org/x/sync/singleflight"
 )
 
 type folderPermissions struct {
@@ -58,10 +58,15 @@ func (f *folderPermissions) Apply(ctx context.Context, b bundle.ReadOnlyBundle)
 	var diags diag.Diagnostics
 	g, ctx := errgroup.WithContext(ctx)
 	results := make([]diag.Diagnostics, len(paths))
+	syncGroup := new(singleflight.Group)
 	for i, p := range paths {
 		g.Go(func() error {
-			results[i] = checkFolderPermission(ctx, b, p)
+			diags, err, _ := syncGroup.Do(p, func() (any, error) {
-			return nil
+				diags := checkFolderPermission(ctx, b, p)
+				return diags, nil
+			})
+			results[i] = diags.(diag.Diagnostics)
+			return err
 		})
 	}
 
@@ -91,41 +96,28 @@ func checkFolderPermission(ctx context.Context, b bundle.ReadOnlyBundle, folderP
 		return diag.FromErr(err)
 	}
 
-	p := permissions.NewFromWorkspaceObjectAcl(folderPath, objPermissions.AccessControlList)
+	p := permissions.ObjectAclToResourcePermissions(folderPath, objPermissions.AccessControlList)
 	return p.Compare(b.Config().Permissions)
 }
 
-var cache = map[string]*workspace.ObjectInfo{}
-
 func getClosestExistingObject(ctx context.Context, w workspace.WorkspaceInterface, folderPath string) (*workspace.ObjectInfo, error) {
-	if obj, ok := cache[folderPath]; ok {
+	for {
-		return obj, nil
-	}
-
-	for folderPath != "/" {
 		obj, err := w.GetStatusByPath(ctx, folderPath)
 		if err == nil {
-			cache[folderPath] = obj
 			return obj, nil
 		}
 
-		var aerr *apierr.APIError
+		if !apierr.IsMissing(err) {
-		if !errors.As(err, &aerr) {
 			return nil, err
 		}
 
-		if aerr.ErrorCode != "RESOURCE_DOES_NOT_EXIST" {
+		parent := path.Dir(folderPath)
-			return nil, err
+		// If the parent is the same as the current folder, then we have reached the root
+		if folderPath == parent {
+			break
 		}
 
-		folderPath = path.Dir(folderPath)
+		folderPath = parent
-	}
-
-	// Check "/" root folder
-	obj, err := w.GetStatusByPath(ctx, folderPath)
-	if err == nil {
-		cache[folderPath] = obj
-		return obj, nil
 	}
 
 	return nil, fmt.Errorf("folder %s and its parent folders do not exist", folderPath)
@@ -136,6 +128,8 @@ func (f *folderPermissions) Name() string {
 	return "validate:folder_permissions"
 }
 
+// ValidateFolderPermissions validates that permissions for the folders in Workspace file system matches
+// the permissions in the top-level permissions section of the bundle.
 func ValidateFolderPermissions() bundle.ReadOnlyMutator {
 	return &folderPermissions{}
 }

bundle/config/validate/folder_permissions_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/databricks/cli/bundle/config"
 	"github.com/databricks/cli/bundle/config/resources"
 	"github.com/databricks/cli/bundle/permissions"
+	"github.com/databricks/cli/libs/diag"
 	"github.com/databricks/databricks-sdk-go/apierr"
 	"github.com/databricks/databricks-sdk-go/experimental/mocks"
 	"github.com/databricks/databricks-sdk-go/service/workspace"
@@ -16,7 +17,6 @@ import (
 )
 
 func TestValidateFolderPermissions(t *testing.T) {
-	setupTest(t)
 	b := &bundle.Bundle{
 		Config: config.Root{
 			Workspace: config.Workspace{
@@ -68,7 +68,6 @@ func TestValidateFolderPermissions(t *testing.T) {
 }
 
 func TestValidateFolderPermissionsDifferentCount(t *testing.T) {
-	setupTest(t)
 	b := &bundle.Bundle{
 		Config: config.Root{
 			Workspace: config.Workspace{
@@ -115,11 +114,12 @@ func TestValidateFolderPermissionsDifferentCount(t *testing.T) {
 
 	diags := bundle.ApplyReadOnly(context.Background(), rb, ValidateFolderPermissions())
 	require.Len(t, diags, 1)
-	require.Equal(t, "permissions count mismatch", diags[0].Summary)
+	require.Equal(t, "permissions missing", diags[0].Summary)
+	require.Equal(t, diag.Warning, diags[0].Severity)
+	require.Equal(t, "Following permissions set for the workspace folder but not set for bundle /Workspace/Users/foo@bar.com:\n- level: CAN_MANAGE\n  user_name: foo2@bar.com\n", diags[0].Detail)
 }
 
 func TestValidateFolderPermissionsDifferentPermission(t *testing.T) {
-	setupTest(t)
 	b := &bundle.Bundle{
 		Config: config.Root{
 			Workspace: config.Workspace{
@@ -159,12 +159,15 @@ func TestValidateFolderPermissionsDifferentPermission(t *testing.T) {
 	rb := bundle.ReadOnly(b)
 
 	diags := bundle.ApplyReadOnly(context.Background(), rb, ValidateFolderPermissions())
-	require.Len(t, diags, 1)
+	require.Len(t, diags, 2)
-	require.Equal(t, "permission not found", diags[0].Summary)
+	require.Equal(t, "permissions missing", diags[0].Summary)
+	require.Equal(t, diag.Warning, diags[0].Severity)
+
+	require.Equal(t, "permissions missing", diags[1].Summary)
+	require.Equal(t, diag.Warning, diags[1].Severity)
 }
 
 func TestNoRootFolder(t *testing.T) {
-	setupTest(t)
 	b := &bundle.Bundle{
 		Config: config.Root{
 			Workspace: config.Workspace{
@@ -196,8 +199,5 @@ func TestNoRootFolder(t *testing.T) {
 	diags := bundle.ApplyReadOnly(context.Background(), rb, ValidateFolderPermissions())
 	require.Len(t, diags, 1)
 	require.Equal(t, "folder / and its parent folders do not exist", diags[0].Summary)
-}
+	require.Equal(t, diag.Error, diags[0].Severity)
-
-func setupTest(t *testing.T) {
-	cache = make(map[string]*workspace.ObjectInfo)
 }

bundle/permissions/check.go

@@ -14,7 +14,7 @@ type WorkspacePathPermissions struct {
 	Permissions []resources.Permission
 }
 
-func NewFromWorkspaceObjectAcl(path string, acl []workspace.WorkspaceObjectAccessControlResponse) *WorkspacePathPermissions {
+func ObjectAclToResourcePermissions(path string, acl []workspace.WorkspaceObjectAccessControlResponse) *WorkspacePathPermissions {
 	permissions := make([]resources.Permission, 0)
 	for _, a := range acl {
 		// Skip the admin group because it's added to all resources by default.
@@ -24,7 +24,7 @@ func NewFromWorkspaceObjectAcl(path string, acl []workspace.WorkspaceObjectAcces
 
 		for _, pl := range a.AllPermissions {
 			permissions = append(permissions, resources.Permission{
-				Level:                string(pl.PermissionLevel),
+				Level:                convertWorkspaceObjectPermissionLevel(pl.PermissionLevel),
 				GroupName:            a.GroupName,
 				UserName:             a.UserName,
 				ServicePrincipalName: a.ServicePrincipalName,
@@ -38,55 +38,75 @@ func NewFromWorkspaceObjectAcl(path string, acl []workspace.WorkspaceObjectAcces
 func (p WorkspacePathPermissions) Compare(perms []resources.Permission) diag.Diagnostics {
 	var diags diag.Diagnostics
 
-	if len(p.Permissions) != len(perms) {
+	// Check the permissions in the bundle and see if they are all set in the workspace.
+	ok, missing := containsAll(perms, p.Permissions)
+	if !ok {
 		diags = diags.Append(diag.Diagnostic{
 			Severity: diag.Warning,
-			Summary:  "permissions count mismatch",
+			Summary:  "permissions missing",
-			Detail: fmt.Sprintf(
+			Detail:   fmt.Sprintf("Following permissions set in the bundle but not set for workspace folder %s:\n%s", p.Path, toString(missing)),
-				"The number of permissions in the bundle is %d, but the number of permissions in the workspace is %d\n%s\n%s",
-				len(perms), len(p.Permissions),
-				toString("Bundle permissions", p.Permissions), toString("Workspace permissions", perms)),
 		})
-		return diags
 	}
 
-	for _, perm := range perms {
+	// Check the permissions in the workspace and see if they are all set in the bundle.
-		level, err := GetWorkspaceObjectPermissionLevel(perm.Level)
+	ok, missing = containsAll(p.Permissions, perms)
-		if err != nil {
+	if !ok {
-			return diag.FromErr(err)
+		diags = diags.Append(diag.Diagnostic{
-		}
+			Severity: diag.Warning,
-
+			Summary:  "permissions missing",
-		found := false
+			Detail:   fmt.Sprintf("Following permissions set for the workspace folder but not set for bundle %s:\n%s", p.Path, toString(missing)),
-		for _, objPerm := range p.Permissions {
+		})
-			if objPerm.GroupName == perm.GroupName &&
-				objPerm.UserName == perm.UserName &&
-				objPerm.ServicePrincipalName == perm.ServicePrincipalName &&
-				objPerm.Level == string(level) {
-				found = true
-				break
-			}
-		}
-
-		if !found {
-			diags = diags.Append(diag.Diagnostic{
-				Severity: diag.Warning,
-				Summary:  "permission not found",
-				Detail: fmt.Sprintf(
-					"Permission (%s) not set for bundle workspace folder %s\n%s\n%s",
-					perm, p.Path,
-					toString("Bundle permissions", p.Permissions), toString("Workspace permissions", perms)),
-			})
-		}
 	}
 
 	return diags
 }
 
-func toString(title string, p []resources.Permission) string {
+// containsAll checks if permA contains all permissions in permB.
+func containsAll(permA []resources.Permission, permB []resources.Permission) (bool, []resources.Permission) {
+	missing := make([]resources.Permission, 0)
+	for _, a := range permA {
+		found := false
+		for _, b := range permB {
+			if a == b {
+				found = true
+				break
+			}
+		}
+		if !found {
+			missing = append(missing, a)
+		}
+	}
+	return len(missing) == 0, missing
+}
+
+// convertWorkspaceObjectPermissionLevel converts matching object permission levels to bundle ones.
+// If there is no matching permission level, it returns permission level as is, for example, CAN_EDIT.
+func convertWorkspaceObjectPermissionLevel(level workspace.WorkspaceObjectPermissionLevel) string {
+	switch level {
+	case workspace.WorkspaceObjectPermissionLevelCanRead:
+		return CAN_VIEW
+	default:
+		return string(level)
+	}
+}
+
+func toString(p []resources.Permission) string {
 	var sb strings.Builder
-	sb.WriteString(fmt.Sprintf("%s\n", title))
 	for _, perm := range p {
-		sb.WriteString(fmt.Sprintf("- level: %s, user_name: %s, group_name: %s, service_principal_name: %s\n", perm.Level, perm.UserName, perm.GroupName, perm.ServicePrincipalName))
+		if perm.ServicePrincipalName != "" {
+			sb.WriteString(fmt.Sprintf("- level: %s\n  service_principal_name: %s\n", perm.Level, perm.ServicePrincipalName))
+			continue
+		}
+
+		if perm.GroupName != "" {
+			sb.WriteString(fmt.Sprintf("- level: %s\n  group_name: %s\n", perm.Level, perm.GroupName))
+			continue
+		}
+
+		if perm.UserName != "" {
+			sb.WriteString(fmt.Sprintf("- level: %s\n  user_name: %s\n", perm.Level, perm.UserName))
+			continue
+		}
 	}
 	return sb.String()
 }

bundle/permissions/check_test.go

@@ -0,0 +1,132 @@
+package permissions
+
+import (
+	"testing"
+
+	"github.com/databricks/cli/bundle/config/resources"
+	"github.com/databricks/cli/libs/diag"
+	"github.com/databricks/databricks-sdk-go/service/workspace"
+	"github.com/stretchr/testify/require"
+)
+
+func TestWorkspacePathPermissionsCompare(t *testing.T) {
+	testCases := []struct {
+		perms    []resources.Permission
+		acl      []workspace.WorkspaceObjectAccessControlResponse
+		expected diag.Diagnostics
+	}{
+		{
+			perms: []resources.Permission{
+				{Level: CAN_MANAGE, UserName: "foo@bar.com"},
+			},
+			acl: []workspace.WorkspaceObjectAccessControlResponse{
+				{
+					UserName: "foo@bar.com",
+					AllPermissions: []workspace.WorkspaceObjectPermission{
+						{PermissionLevel: "CAN_MANAGE"},
+					},
+				},
+			},
+			expected: nil,
+		},
+		{
+			perms: []resources.Permission{
+				{Level: CAN_MANAGE, UserName: "foo@bar.com"},
+			},
+			acl: []workspace.WorkspaceObjectAccessControlResponse{
+				{
+					UserName: "foo@bar.com",
+					AllPermissions: []workspace.WorkspaceObjectPermission{
+						{PermissionLevel: "CAN_MANAGE"},
+					},
+				},
+				{
+					GroupName: "admin",
+					AllPermissions: []workspace.WorkspaceObjectPermission{
+						{PermissionLevel: "CAN_MANAGE"},
+					},
+				},
+			},
+			expected: nil,
+		},
+		{
+			perms: []resources.Permission{
+				{Level: CAN_VIEW, UserName: "foo@bar.com"},
+				{Level: CAN_MANAGE, ServicePrincipalName: "sp.com"},
+			},
+			acl: []workspace.WorkspaceObjectAccessControlResponse{
+				{
+					UserName: "foo@bar.com",
+					AllPermissions: []workspace.WorkspaceObjectPermission{
+						{PermissionLevel: "CAN_READ"},
+					},
+				},
+			},
+			expected: diag.Diagnostics{
+				{
+					Severity: diag.Warning,
+					Summary:  "permissions missing",
+					Detail:   "Following permissions set in the bundle but not set for workspace folder path:\n- level: CAN_MANAGE\n  service_principal_name: sp.com\n",
+				},
+			},
+		},
+		{
+			perms: []resources.Permission{
+				{Level: CAN_MANAGE, UserName: "foo@bar.com"},
+			},
+			acl: []workspace.WorkspaceObjectAccessControlResponse{
+				{
+					UserName: "foo@bar.com",
+					AllPermissions: []workspace.WorkspaceObjectPermission{
+						{PermissionLevel: "CAN_MANAGE"},
+					},
+				},
+				{
+					GroupName: "foo",
+					AllPermissions: []workspace.WorkspaceObjectPermission{
+						{PermissionLevel: "CAN_MANAGE"},
+					},
+				},
+			},
+			expected: diag.Diagnostics{
+				{
+					Severity: diag.Warning,
+					Summary:  "permissions missing",
+					Detail:   "Following permissions set for the workspace folder but not set for bundle path:\n- level: CAN_MANAGE\n  group_name: foo\n",
+				},
+			},
+		},
+		{
+			perms: []resources.Permission{
+				{Level: CAN_MANAGE, UserName: "foo@bar.com"},
+			},
+			acl: []workspace.WorkspaceObjectAccessControlResponse{
+				{
+					UserName: "foo2@bar.com",
+					AllPermissions: []workspace.WorkspaceObjectPermission{
+						{PermissionLevel: "CAN_MANAGE"},
+					},
+				},
+			},
+			expected: diag.Diagnostics{
+				{
+					Severity: diag.Warning,
+					Summary:  "permissions missing",
+					Detail:   "Following permissions set in the bundle but not set for workspace folder path:\n- level: CAN_MANAGE\n  user_name: foo@bar.com\n",
+				},
+				{
+					Severity: diag.Warning,
+					Summary:  "permissions missing",
+					Detail:   "Following permissions set for the workspace folder but not set for bundle path:\n- level: CAN_MANAGE\n  user_name: foo2@bar.com\n",
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		wp := ObjectAclToResourcePermissions("path", tc.acl)
+		diags := wp.Compare(tc.perms)
+		require.Equal(t, tc.expected, diags)
+	}
+
+}