diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index ba69c62c5c..ff8b2d38fc 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -10,6 +10,7 @@ To disable this, set the environment variable DATABRICKS_CACHE_ENABLED to false. ### Bundles * Enable caching user identity by default ([#4202](https://github.com/databricks/cli/pull/4202)) +* Fix false positive folder permission warnings and make them more actionable ([#4216](https://github.com/databricks/cli/pull/4216)) * Pass additional Azure DevOps system variables ([#4236](https://github.com/databricks/cli/pull/4236)) ### Dependency updates diff --git a/bundle/config/mutator/resourcemutator/fix_permissions.go b/bundle/config/mutator/resourcemutator/fix_permissions.go index a15e905fe9..7f685015e2 100644 --- a/bundle/config/mutator/resourcemutator/fix_permissions.go +++ b/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), diff --git a/bundle/config/mutator/resourcemutator/fix_permissions_test.go b/bundle/config/mutator/resourcemutator/fix_permissions_test.go index 9ff42ca9a5..507096c3fd 100644 --- a/bundle/config/mutator/resourcemutator/fix_permissions_test.go +++ b/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")) } diff --git a/bundle/config/resources/permission.go b/bundle/config/resources/permission.go index d7c3f6fca9..b13c81e5e9 100644 --- a/bundle/config/resources/permission.go +++ b/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 +} diff --git a/bundle/config/validate/folder_permissions_test.go b/bundle/config/validate/folder_permissions_test.go index 40906ce6f7..e41d731b3e 100644 --- a/bundle/config/validate/folder_permissions_test.go +++ 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) } diff --git a/bundle/permissions/workspace_path_permissions.go b/bundle/permissions/workspace_path_permissions.go index e6ef16540c..cabf4117f6 100644 --- a/bundle/permissions/workspace_path_permissions.go +++ b/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 } diff --git a/bundle/permissions/workspace_path_permissions_test.go b/bundle/permissions/workspace_path_permissions_test.go index eaefad906c..df2f86a8a8 100644 --- a/bundle/permissions/workspace_path_permissions_test.go +++ b/bundle/permissions/workspace_path_permissions_test.go @@ -85,8 +85,11 @@ func TestWorkspacePathPermissionsCompare(t *testing.T) { expected: diag.Diagnostics{ { Severity: diag.Warning, - Summary: "untracked permissions apply to target workspace path", - Detail: "The following permissions apply to the workspace folder at \"path\" but are not configured in the bundle:\n- level: CAN_MANAGE, group_name: foo\n", + Summary: "workspace folder has permissions not configured in bundle", + Detail: "The following permissions apply to the workspace folder at \"path\" " + + "but are not configured in the bundle:\n- level: CAN_MANAGE, group_name: foo\n\n" + + "Add them to your bundle permissions or remove them from the folder.\n" + + "See https://docs.databricks.com/dev-tools/bundles/permissions", }, }, }, @@ -105,8 +108,11 @@ func TestWorkspacePathPermissionsCompare(t *testing.T) { expected: diag.Diagnostics{ { Severity: diag.Warning, - Summary: "untracked permissions apply to target workspace path", - Detail: "The following permissions apply to the workspace folder at \"path\" but are not configured in the bundle:\n- level: CAN_MANAGE, user_name: foo2@bar.com\n", + Summary: "workspace folder has permissions not configured in bundle", + Detail: "The following permissions apply to the workspace folder at \"path\" " + + "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", }, }, }, @@ -118,3 +124,95 @@ func TestWorkspacePathPermissionsCompare(t *testing.T) { require.Equal(t, tc.expected, diags) } } + +func TestWorkspacePathPermissionsCompareWithHierarchy(t *testing.T) { + testCases := []struct { + name string + perms []resources.Permission + acl []workspace.WorkspaceObjectAccessControlResponse + expected diag.Diagnostics + }{ + { + name: "bundle grants higher permission than workspace - no warning", + perms: []resources.Permission{ + {Level: CAN_MANAGE, UserName: "foo@bar.com"}, + }, + acl: []workspace.WorkspaceObjectAccessControlResponse{ + { + UserName: "foo@bar.com", + AllPermissions: []workspace.WorkspaceObjectPermission{ + {PermissionLevel: "CAN_READ"}, + }, + }, + }, + expected: nil, + }, + { + name: "bundle grants lower permission than workspace - warning", + perms: []resources.Permission{ + {Level: CAN_VIEW, UserName: "foo@bar.com"}, + }, + acl: []workspace.WorkspaceObjectAccessControlResponse{ + { + UserName: "foo@bar.com", + AllPermissions: []workspace.WorkspaceObjectPermission{ + {PermissionLevel: "CAN_MANAGE"}, + }, + }, + }, + expected: diag.Diagnostics{ + { + Severity: diag.Warning, + Summary: "workspace folder has permissions not configured in bundle", + Detail: "The following permissions apply to the workspace folder at \"path\" " + + "but are not configured in the bundle:\n- level: CAN_MANAGE, user_name: foo@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", + }, + }, + }, + { + name: "bundle grants same permission as workspace - no warning", + 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, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + wp := ObjectAclToResourcePermissions("path", tc.acl) + diags := wp.Compare(tc.perms) + require.Equal(t, tc.expected, diags) + }) + } +} + +func TestWorkspacePathPermissionsDeduplication(t *testing.T) { + // User has both inherited CAN_VIEW and explicit CAN_MANAGE + acl := []workspace.WorkspaceObjectAccessControlResponse{ + { + UserName: "foo@bar.com", + AllPermissions: []workspace.WorkspaceObjectPermission{ + {PermissionLevel: "CAN_READ"}, // inherited + {PermissionLevel: "CAN_MANAGE"}, // explicit + }, + }, + } + + wp := ObjectAclToResourcePermissions("path", acl) + + // Should only have one permission entry with the highest level + require.Len(t, wp.Permissions, 1) + require.Equal(t, CAN_MANAGE, wp.Permissions[0].Level) + require.Equal(t, "foo@bar.com", wp.Permissions[0].UserName) +}