Improve folder permissions warning

NEXT_CHANGELOG.md

@@ -9,6 +9,7 @@ To disable this, set the environment variable DATABRICKS_CACHE_ENABLED to false.
 ### CLI
 
 ### Bundles
+* Fix false positive folder permission warnings and make them more actionable ([#4216](https://github.com/databricks/cli/pull/4216))
 * Enable caching user identity by default ([#4202](https://github.com/databricks/cli/pull/4202))
 * Pass additional Azure DevOps system variables ([#4236](https://github.com/databricks/cli/pull/4236))
 

bundle/config/mutator/resourcemutator/fix_permissions.go

@@ -5,6 +5,7 @@ import (
 	"strings"
 
 	"github.com/databricks/cli/bundle"
+	"github.com/databricks/cli/bundle/config/resources"
 	"github.com/databricks/cli/libs/diag"
 	"github.com/databricks/cli/libs/dyn"
 	"github.com/databricks/cli/libs/iamutil"
@@ -170,7 +171,7 @@ func useMaximumLevel(permissions dyn.Value, resourceType string) (dyn.Value, err
 			principalIndex[principal] = ind
 			principals = append(principals, principal)
 		}
-		levelPerPrincipal[principal] = getMaxLevel(levelPerPrincipal[principal], level)
+		levelPerPrincipal[principal] = resources.GetMaxLevel(levelPerPrincipal[principal], level)
 	}
 
 	var newPermissions []dyn.Value
@@ -182,62 +183,6 @@ func useMaximumLevel(permissions dyn.Value, resourceType string) (dyn.Value, err
 	return dyn.V(newPermissions), nil
 }
 
-// Unified permission order map
-// Based on https://docs.databricks.com/aws/en/security/auth/access-control
-var PermissionOrder = map[string]int{
-	"":                               -1,
-	"CAN_VIEW":                       2,
-	"CAN_READ":                       3,
-	"CAN_VIEW_METADATA":              4,
-	"CAN_RUN":                        5,
-	"CAN_QUERY":                      6,
-	"CAN_USE":                        7,
-	"CAN_EDIT":                       8,
-	"CAN_EDIT_METADATA":              9,
-	"CAN_CREATE":                     10,
-	"CAN_ATTACH_TO":                  11,
-	"CAN_RESTART":                    12,
-	"CAN_MONITOR":                    13,
-	"CAN_MANAGE_RUN":                 14,
-	"CAN_MANAGE_STAGING_VERSIONS":    15,
-	"CAN_MANAGE_PRODUCTION_VERSIONS": 16,
-	"CAN_MANAGE":                     17,
-	"IS_OWNER":                       18,
-	// One known exception from this order: for SQL Warehouses, CAN_USE and CAN_RUN cannot be ordered and must be upgraded to CAN_MONITOR.
-	// We're not doing that currently.
-}
-
-func getLevelScore(a string) int {
-	score, ok := PermissionOrder[a]
-	if ok {
-		return score
-	}
-	maxPrefixLength := 0
-	for levelName, levelScore := range PermissionOrder {
-		if strings.HasPrefix(a, levelName) && len(levelName) > maxPrefixLength {
-			score = levelScore
-			maxPrefixLength = len(levelName)
-		}
-	}
-	return score
-}
-
-func compareLevels(a, b string) int {
-	s1 := getLevelScore(a)
-	s2 := getLevelScore(b)
-	if s1 == s2 {
-		return strings.Compare(a, b)
-	}
-	return s1 - s2
-}
-
-func getMaxLevel(a, b string) string {
-	if compareLevels(a, b) >= 0 {
-		return a
-	}
-	return b
-}
-
 func createPermission(user, level string) dyn.Value {
 	permission := map[string]dyn.Value{
 		"level": dyn.V(level),

bundle/config/mutator/resourcemutator/fix_permissions_test.go

@@ -3,27 +3,28 @@ package resourcemutator
 import (
 	"testing"
 
+	"github.com/databricks/cli/bundle/config/resources"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestGetLevelScore(t *testing.T) {
-	assert.Equal(t, 17, getLevelScore("CAN_MANAGE"))
-	assert.Equal(t, 0, getLevelScore("UNKNOWN_PERMISSION"))
-	assert.Equal(t, getLevelScore("CAN_MANAGE"), getLevelScore("CAN_MANAGE_SOMETHING_ELSE"))
-	assert.Equal(t, getLevelScore("CAN_MANAGE_RUN"), getLevelScore("CAN_MANAGE_RUN1"))
+	assert.Equal(t, 17, resources.GetLevelScore("CAN_MANAGE"))
+	assert.Equal(t, 0, resources.GetLevelScore("UNKNOWN_PERMISSION"))
+	assert.Equal(t, resources.GetLevelScore("CAN_MANAGE"), resources.GetLevelScore("CAN_MANAGE_SOMETHING_ELSE"))
+	assert.Equal(t, resources.GetLevelScore("CAN_MANAGE_RUN"), resources.GetLevelScore("CAN_MANAGE_RUN1"))
 }
 
 func TestGetMaxLevel(t *testing.T) {
-	assert.Equal(t, "IS_OWNER", getMaxLevel("IS_OWNER", "CAN_MANAGE"))
-	assert.Equal(t, "IS_OWNER", getMaxLevel("CAN_MANAGE", "IS_OWNER"))
-	assert.Equal(t, "CAN_MANAGE", getMaxLevel("CAN_MANAGE", "CAN_EDIT"))
-	assert.Equal(t, "CAN_EDIT", getMaxLevel("CAN_READ", "CAN_EDIT"))
+	assert.Equal(t, "IS_OWNER", resources.GetMaxLevel("IS_OWNER", "CAN_MANAGE"))
+	assert.Equal(t, "IS_OWNER", resources.GetMaxLevel("CAN_MANAGE", "IS_OWNER"))
+	assert.Equal(t, "CAN_MANAGE", resources.GetMaxLevel("CAN_MANAGE", "CAN_EDIT"))
+	assert.Equal(t, "CAN_EDIT", resources.GetMaxLevel("CAN_READ", "CAN_EDIT"))
 
-	assert.Equal(t, "CAN_MANAGE", getMaxLevel("CAN_MANAGE", "CAN_MANAGE"))
-	assert.Equal(t, "CAN_READ", getMaxLevel("CAN_READ", ""))
-	assert.Equal(t, "CAN_READ", getMaxLevel("", "CAN_READ"))
-	assert.Equal(t, "", getMaxLevel("", ""))
+	assert.Equal(t, "CAN_MANAGE", resources.GetMaxLevel("CAN_MANAGE", "CAN_MANAGE"))
+	assert.Equal(t, "CAN_READ", resources.GetMaxLevel("CAN_READ", ""))
+	assert.Equal(t, "CAN_READ", resources.GetMaxLevel("", "CAN_READ"))
+	assert.Equal(t, "", resources.GetMaxLevel("", ""))
 
-	assert.Equal(t, "UNKNOWN_B", getMaxLevel("UNKNOWN_A", "UNKNOWN_B"))
-	assert.Equal(t, "UNKNOWN_B", getMaxLevel("UNKNOWN_B", "UNKNOWN_A"))
+	assert.Equal(t, "UNKNOWN_B", resources.GetMaxLevel("UNKNOWN_A", "UNKNOWN_B"))
+	assert.Equal(t, "UNKNOWN_B", resources.GetMaxLevel("UNKNOWN_B", "UNKNOWN_A"))
 }

bundle/config/resources/permission.go

@@ -1,6 +1,9 @@
 package resources
 
-import "fmt"
+import (
+	"fmt"
+	"strings"
+)
 
 // Permission holds the permission level setting for a single principal.
 type Permission struct {
@@ -221,3 +224,60 @@ func (p SqlWarehousePermission) GetLevel() string                { return string
 func (p SqlWarehousePermission) GetUserName() string             { return p.UserName }
 func (p SqlWarehousePermission) GetServicePrincipalName() string { return p.ServicePrincipalName }
 func (p SqlWarehousePermission) GetGroupName() string            { return p.GroupName }
+
+// PermissionOrder defines the hierarchy of permission levels.
+// Higher numbers mean more permissive access.
+// Based on https://docs.databricks.com/aws/en/security/auth/access-control
+var PermissionOrder = map[string]int{
+	"":                               -1,
+	"CAN_VIEW":                       2,
+	"CAN_READ":                       3,
+	"CAN_VIEW_METADATA":              4,
+	"CAN_RUN":                        5,
+	"CAN_QUERY":                      6,
+	"CAN_USE":                        7,
+	"CAN_EDIT":                       8,
+	"CAN_EDIT_METADATA":              9,
+	"CAN_CREATE":                     10,
+	"CAN_ATTACH_TO":                  11,
+	"CAN_RESTART":                    12,
+	"CAN_MONITOR":                    13,
+	"CAN_MANAGE_RUN":                 14,
+	"CAN_MANAGE_STAGING_VERSIONS":    15,
+	"CAN_MANAGE_PRODUCTION_VERSIONS": 16,
+	"CAN_MANAGE":                     17,
+	"IS_OWNER":                       18,
+	// One known exception from this order: for SQL Warehouses, CAN_USE and CAN_RUN cannot be ordered and must be upgraded to CAN_MONITOR.
+	// We're not doing that currently.
+}
+
+func GetLevelScore(a string) int {
+	score, ok := PermissionOrder[a]
+	if ok {
+		return score
+	}
+	maxPrefixLength := 0
+	for levelName, levelScore := range PermissionOrder {
+		if strings.HasPrefix(a, levelName) && len(levelName) > maxPrefixLength {
+			score = levelScore
+			maxPrefixLength = len(levelName)
+		}
+	}
+	return score
+}
+
+func CompareLevels(a, b string) int {
+	s1 := GetLevelScore(a)
+	s2 := GetLevelScore(b)
+	if s1 == s2 {
+		return strings.Compare(a, b)
+	}
+	return s1 - s2
+}
+
+func GetMaxLevel(a, b string) string {
+	if CompareLevels(a, b) >= 0 {
+		return a
+	}
+	return b
+}

bundle/config/validate/folder_permissions_test.go

@@ -118,9 +118,13 @@ func TestValidateFolderPermissionsFailsOnMissingBundlePermission(t *testing.T) {
 	b.SetWorkpaceClient(m.WorkspaceClient)
 	diags := ValidateFolderPermissions().Apply(context.Background(), b)
 	require.Len(t, diags, 1)
-	require.Equal(t, "untracked permissions apply to target workspace path", diags[0].Summary)
+	require.Equal(t, "workspace folder has permissions not configured in bundle", diags[0].Summary)
 	require.Equal(t, diag.Warning, diags[0].Severity)
-	require.Equal(t, "The following permissions apply to the workspace folder at \"/Workspace/Users/foo@bar.com\" but are not configured in the bundle:\n- level: CAN_MANAGE, user_name: foo2@bar.com\n", diags[0].Detail)
+	expectedDetail := "The following permissions apply to the workspace folder at \"/Workspace/Users/foo@bar.com\" " +
+		"but are not configured in the bundle:\n- level: CAN_MANAGE, user_name: foo2@bar.com\n\n" +
+		"Add them to your bundle permissions or remove them from the folder.\n" +
+		"See https://docs.databricks.com/dev-tools/bundles/permissions"
+	require.Equal(t, expectedDetail, diags[0].Detail)
 }
 
 func TestValidateFolderPermissionsFailsOnPermissionMismatch(t *testing.T) {
@@ -162,7 +166,7 @@ func TestValidateFolderPermissionsFailsOnPermissionMismatch(t *testing.T) {
 	b.SetWorkpaceClient(m.WorkspaceClient)
 	diags := ValidateFolderPermissions().Apply(context.Background(), b)
 	require.Len(t, diags, 1)
-	require.Equal(t, "untracked permissions apply to target workspace path", diags[0].Summary)
+	require.Equal(t, "workspace folder has permissions not configured in bundle", diags[0].Summary)
 	require.Equal(t, diag.Warning, diags[0].Severity)
 }
 

bundle/permissions/workspace_path_permissions.go

@@ -22,9 +22,18 @@ func ObjectAclToResourcePermissions(path string, acl []workspace.WorkspaceObject
 			continue
 		}
 
+		// Find the highest permission level for this principal (handles inherited + explicit permissions)
+		var highestLevel string
 		for _, pl := range a.AllPermissions {
+			level := convertWorkspaceObjectPermissionLevel(pl.PermissionLevel)
+			if resources.GetLevelScore(level) > resources.GetLevelScore(highestLevel) {
+				highestLevel = level
+			}
+		}
+
+		if highestLevel != "" {
 			permissions = append(permissions, resources.Permission{
-				Level:                convertWorkspaceObjectPermissionLevel(pl.PermissionLevel),
+				Level:                highestLevel,
 				GroupName:            a.GroupName,
 				UserName:             a.UserName,
 				ServicePrincipalName: a.ServicePrincipalName,
@@ -43,21 +52,35 @@ func (p WorkspacePathPermissions) Compare(perms []resources.Permission) diag.Dia
 	if !ok {
 		diags = diags.Append(diag.Diagnostic{
 			Severity: diag.Warning,
-			Summary:  "untracked permissions apply to target workspace path",
-			Detail:   fmt.Sprintf("The following permissions apply to the workspace folder at %q but are not configured in the bundle:\n%s", p.Path, toString(missing)),
+			Summary:  "workspace folder has permissions not configured in bundle",
+			Detail: fmt.Sprintf(
+				"The following permissions apply to the workspace folder at %q "+
+					"but are not configured in the bundle:\n%s\n"+
+					"Add them to your bundle permissions or remove them from the folder.\n"+
+					"See https://docs.databricks.com/dev-tools/bundles/permissions",
+				p.Path, toString(missing)),
 		})
 	}
 
 	return diags
 }
 
-// containsAll checks if permA contains all permissions in permB.
+// samePrincipal checks if two permissions refer to the same user/group/service principal.
+func samePrincipal(a, b resources.Permission) bool {
+	return a.UserName == b.UserName &&
+		a.GroupName == b.GroupName &&
+		a.ServicePrincipalName == b.ServicePrincipalName
+}
+
+// containsAll checks if all permissions in permA (workspace) are accounted for in permB (bundle).
+// A workspace permission is considered accounted for if the bundle has the same principal
+// with an equal or higher permission level.
 func containsAll(permA, permB []resources.Permission) (bool, []resources.Permission) {
 	var missing []resources.Permission
 	for _, a := range permA {
 		found := false
 		for _, b := range permB {
-			if a == b {
+			if samePrincipal(a, b) && resources.GetLevelScore(b.Level) >= resources.GetLevelScore(a.Level) {
 				found = true
 				break
 			}