Skip to content

Commit f4f794c

Browse files
lennartkats-dbclaude
lennartkats-db
and
claude
authored
Improve folder permissions warning (#4216)
## Changes This improves the folder permissions warnings, fixing false positives and making any warnings more actionable. - Make the warning message more actionable with clearer details - Fix false positive warnings when bundle grants higher permission than workspace - Fix duplicate permission entries from inherited + explicit permissions - Simplify code by removing untested current user group tracking ## Testing - [x] Unit tests pass - [x] Manually tested with bundle deploy --------- Co-authored-by: Claude Opus 4.5 <[email protected]>
1 parent b4cc378 commit f4f794c

File tree

7 files changed

+216
-84
lines changed

7 files changed

+216
-84
lines changed

‎NEXT_CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ To disable this, set the environment variable DATABRICKS_CACHE_ENABLED to false.
1010

1111
### Bundles
1212
* Enable caching user identity by default ([#4202](https://github.com/databricks/cli/pull/4202))
13+
* Fix false positive folder permission warnings and make them more actionable ([#4216](https://github.com/databricks/cli/pull/4216))
1314
* Pass additional Azure DevOps system variables ([#4236](https://github.com/databricks/cli/pull/4236))
1415

1516
### Dependency updates

‎bundle/config/mutator/resourcemutator/fix_permissions.go‎

Lines changed: 2 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"strings"
66

77
"github.com/databricks/cli/bundle"
8+
"github.com/databricks/cli/bundle/config/resources"
89
"github.com/databricks/cli/libs/diag"
910
"github.com/databricks/cli/libs/dyn"
1011
"github.com/databricks/cli/libs/iamutil"
@@ -170,7 +171,7 @@ func useMaximumLevel(permissions dyn.Value, resourceType string) (dyn.Value, err
170171
principalIndex[principal] = ind
171172
principals = append(principals, principal)
172173
}
173-
levelPerPrincipal[principal] = getMaxLevel(levelPerPrincipal[principal], level)
174+
levelPerPrincipal[principal] = resources.GetMaxLevel(levelPerPrincipal[principal], level)
174175
}
175176

176177
var newPermissions []dyn.Value
@@ -182,62 +183,6 @@ func useMaximumLevel(permissions dyn.Value, resourceType string) (dyn.Value, err
182183
return dyn.V(newPermissions), nil
183184
}
184185

185-
// Unified permission order map
186-
// Based on https://docs.databricks.com/aws/en/security/auth/access-control
187-
var PermissionOrder = map[string]int{
188-
"": -1,
189-
"CAN_VIEW": 2,
190-
"CAN_READ": 3,
191-
"CAN_VIEW_METADATA": 4,
192-
"CAN_RUN": 5,
193-
"CAN_QUERY": 6,
194-
"CAN_USE": 7,
195-
"CAN_EDIT": 8,
196-
"CAN_EDIT_METADATA": 9,
197-
"CAN_CREATE": 10,
198-
"CAN_ATTACH_TO": 11,
199-
"CAN_RESTART": 12,
200-
"CAN_MONITOR": 13,
201-
"CAN_MANAGE_RUN": 14,
202-
"CAN_MANAGE_STAGING_VERSIONS": 15,
203-
"CAN_MANAGE_PRODUCTION_VERSIONS": 16,
204-
"CAN_MANAGE": 17,
205-
"IS_OWNER": 18,
206-
// One known exception from this order: for SQL Warehouses, CAN_USE and CAN_RUN cannot be ordered and must be upgraded to CAN_MONITOR.
207-
// We're not doing that currently.
208-
}
209-
210-
func getLevelScore(a string) int {
211-
score, ok := PermissionOrder[a]
212-
if ok {
213-
return score
214-
}
215-
maxPrefixLength := 0
216-
for levelName, levelScore := range PermissionOrder {
217-
if strings.HasPrefix(a, levelName) && len(levelName) > maxPrefixLength {
218-
score = levelScore
219-
maxPrefixLength = len(levelName)
220-
}
221-
}
222-
return score
223-
}
224-
225-
func compareLevels(a, b string) int {
226-
s1 := getLevelScore(a)
227-
s2 := getLevelScore(b)
228-
if s1 == s2 {
229-
return strings.Compare(a, b)
230-
}
231-
return s1 - s2
232-
}
233-
234-
func getMaxLevel(a, b string) string {
235-
if compareLevels(a, b) >= 0 {
236-
return a
237-
}
238-
return b
239-
}
240-
241186
func createPermission(user, level string) dyn.Value {
242187
permission := map[string]dyn.Value{
243188
"level": dyn.V(level),

‎bundle/config/mutator/resourcemutator/fix_permissions_test.go‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,27 +3,28 @@ package resourcemutator
33
import (
44
"testing"
55

6+
"github.com/databricks/cli/bundle/config/resources"
67
"github.com/stretchr/testify/assert"
78
)
89

910
func TestGetLevelScore(t *testing.T) {
10-
assert.Equal(t, 17, getLevelScore("CAN_MANAGE"))
11-
assert.Equal(t, 0, getLevelScore("UNKNOWN_PERMISSION"))
12-
assert.Equal(t, getLevelScore("CAN_MANAGE"), getLevelScore("CAN_MANAGE_SOMETHING_ELSE"))
13-
assert.Equal(t, getLevelScore("CAN_MANAGE_RUN"), getLevelScore("CAN_MANAGE_RUN1"))
11+
assert.Equal(t, 17, resources.GetLevelScore("CAN_MANAGE"))
12+
assert.Equal(t, 0, resources.GetLevelScore("UNKNOWN_PERMISSION"))
13+
assert.Equal(t, resources.GetLevelScore("CAN_MANAGE"), resources.GetLevelScore("CAN_MANAGE_SOMETHING_ELSE"))
14+
assert.Equal(t, resources.GetLevelScore("CAN_MANAGE_RUN"), resources.GetLevelScore("CAN_MANAGE_RUN1"))
1415
}
1516

1617
func TestGetMaxLevel(t *testing.T) {
17-
assert.Equal(t, "IS_OWNER", getMaxLevel("IS_OWNER", "CAN_MANAGE"))
18-
assert.Equal(t, "IS_OWNER", getMaxLevel("CAN_MANAGE", "IS_OWNER"))
19-
assert.Equal(t, "CAN_MANAGE", getMaxLevel("CAN_MANAGE", "CAN_EDIT"))
20-
assert.Equal(t, "CAN_EDIT", getMaxLevel("CAN_READ", "CAN_EDIT"))
18+
assert.Equal(t, "IS_OWNER", resources.GetMaxLevel("IS_OWNER", "CAN_MANAGE"))
19+
assert.Equal(t, "IS_OWNER", resources.GetMaxLevel("CAN_MANAGE", "IS_OWNER"))
20+
assert.Equal(t, "CAN_MANAGE", resources.GetMaxLevel("CAN_MANAGE", "CAN_EDIT"))
21+
assert.Equal(t, "CAN_EDIT", resources.GetMaxLevel("CAN_READ", "CAN_EDIT"))
2122

22-
assert.Equal(t, "CAN_MANAGE", getMaxLevel("CAN_MANAGE", "CAN_MANAGE"))
23-
assert.Equal(t, "CAN_READ", getMaxLevel("CAN_READ", ""))
24-
assert.Equal(t, "CAN_READ", getMaxLevel("", "CAN_READ"))
25-
assert.Equal(t, "", getMaxLevel("", ""))
23+
assert.Equal(t, "CAN_MANAGE", resources.GetMaxLevel("CAN_MANAGE", "CAN_MANAGE"))
24+
assert.Equal(t, "CAN_READ", resources.GetMaxLevel("CAN_READ", ""))
25+
assert.Equal(t, "CAN_READ", resources.GetMaxLevel("", "CAN_READ"))
26+
assert.Equal(t, "", resources.GetMaxLevel("", ""))
2627

27-
assert.Equal(t, "UNKNOWN_B", getMaxLevel("UNKNOWN_A", "UNKNOWN_B"))
28-
assert.Equal(t, "UNKNOWN_B", getMaxLevel("UNKNOWN_B", "UNKNOWN_A"))
28+
assert.Equal(t, "UNKNOWN_B", resources.GetMaxLevel("UNKNOWN_A", "UNKNOWN_B"))
29+
assert.Equal(t, "UNKNOWN_B", resources.GetMaxLevel("UNKNOWN_B", "UNKNOWN_A"))
2930
}

‎bundle/config/resources/permission.go‎

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package resources
22

3-
import "fmt"
3+
import (
4+
"fmt"
5+
"strings"
6+
)
47

58
// Permission holds the permission level setting for a single principal.
69
type Permission struct {
@@ -221,3 +224,60 @@ func (p SqlWarehousePermission) GetLevel() string { return string
221224
func (p SqlWarehousePermission) GetUserName() string { return p.UserName }
222225
func (p SqlWarehousePermission) GetServicePrincipalName() string { return p.ServicePrincipalName }
223226
func (p SqlWarehousePermission) GetGroupName() string { return p.GroupName }
227+
228+
// PermissionOrder defines the hierarchy of permission levels.
229+
// Higher numbers mean more permissive access.
230+
// Based on https://docs.databricks.com/aws/en/security/auth/access-control
231+
var PermissionOrder = map[string]int{
232+
"": -1,
233+
"CAN_VIEW": 2,
234+
"CAN_READ": 3,
235+
"CAN_VIEW_METADATA": 4,
236+
"CAN_RUN": 5,
237+
"CAN_QUERY": 6,
238+
"CAN_USE": 7,
239+
"CAN_EDIT": 8,
240+
"CAN_EDIT_METADATA": 9,
241+
"CAN_CREATE": 10,
242+
"CAN_ATTACH_TO": 11,
243+
"CAN_RESTART": 12,
244+
"CAN_MONITOR": 13,
245+
"CAN_MANAGE_RUN": 14,
246+
"CAN_MANAGE_STAGING_VERSIONS": 15,
247+
"CAN_MANAGE_PRODUCTION_VERSIONS": 16,
248+
"CAN_MANAGE": 17,
249+
"IS_OWNER": 18,
250+
// One known exception from this order: for SQL Warehouses, CAN_USE and CAN_RUN cannot be ordered and must be upgraded to CAN_MONITOR.
251+
// We're not doing that currently.
252+
}
253+
254+
func GetLevelScore(a string) int {
255+
score, ok := PermissionOrder[a]
256+
if ok {
257+
return score
258+
}
259+
maxPrefixLength := 0
260+
for levelName, levelScore := range PermissionOrder {
261+
if strings.HasPrefix(a, levelName) && len(levelName) > maxPrefixLength {
262+
score = levelScore
263+
maxPrefixLength = len(levelName)
264+
}
265+
}
266+
return score
267+
}
268+
269+
func CompareLevels(a, b string) int {
270+
s1 := GetLevelScore(a)
271+
s2 := GetLevelScore(b)
272+
if s1 == s2 {
273+
return strings.Compare(a, b)
274+
}
275+
return s1 - s2
276+
}
277+
278+
func GetMaxLevel(a, b string) string {
279+
if CompareLevels(a, b) >= 0 {
280+
return a
281+
}
282+
return b
283+
}

‎bundle/config/validate/folder_permissions_test.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -118,9 +118,13 @@ func TestValidateFolderPermissionsFailsOnMissingBundlePermission(t *testing.T) {
118118
b.SetWorkpaceClient(m.WorkspaceClient)
119119
diags := ValidateFolderPermissions().Apply(context.Background(), b)
120120
require.Len(t, diags, 1)
121-
require.Equal(t, "untracked permissions apply to target workspace path", diags[0].Summary)
121+
require.Equal(t, "workspace folder has permissions not configured in bundle", diags[0].Summary)
122122
require.Equal(t, diag.Warning, diags[0].Severity)
123-
require.Equal(t, "The following permissions apply to the workspace folder at \"/Workspace/Users/[email protected]\" but are not configured in the bundle:\n- level: CAN_MANAGE, user_name: [email protected]\n", diags[0].Detail)
123+
expectedDetail := "The following permissions apply to the workspace folder at \"/Workspace/Users/[email protected]\" " +
124+
"but are not configured in the bundle:\n- level: CAN_MANAGE, user_name: [email protected]\n\n" +
125+
"Add them to your bundle permissions or remove them from the folder.\n" +
126+
"See https://docs.databricks.com/dev-tools/bundles/permissions"
127+
require.Equal(t, expectedDetail, diags[0].Detail)
124128
}
125129

126130
func TestValidateFolderPermissionsFailsOnPermissionMismatch(t *testing.T) {
@@ -162,7 +166,7 @@ func TestValidateFolderPermissionsFailsOnPermissionMismatch(t *testing.T) {
162166
b.SetWorkpaceClient(m.WorkspaceClient)
163167
diags := ValidateFolderPermissions().Apply(context.Background(), b)
164168
require.Len(t, diags, 1)
165-
require.Equal(t, "untracked permissions apply to target workspace path", diags[0].Summary)
169+
require.Equal(t, "workspace folder has permissions not configured in bundle", diags[0].Summary)
166170
require.Equal(t, diag.Warning, diags[0].Severity)
167171
}
168172

‎bundle/permissions/workspace_path_permissions.go‎

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,18 @@ func ObjectAclToResourcePermissions(path string, acl []workspace.WorkspaceObject
2222
continue
2323
}
2424

25+
// Find the highest permission level for this principal (handles inherited + explicit permissions)
26+
var highestLevel string
2527
for _, pl := range a.AllPermissions {
28+
level := convertWorkspaceObjectPermissionLevel(pl.PermissionLevel)
29+
if resources.GetLevelScore(level) > resources.GetLevelScore(highestLevel) {
30+
highestLevel = level
31+
}
32+
}
33+
34+
if highestLevel != "" {
2635
permissions = append(permissions, resources.Permission{
27-
Level: convertWorkspaceObjectPermissionLevel(pl.PermissionLevel),
36+
Level: highestLevel,
2837
GroupName: a.GroupName,
2938
UserName: a.UserName,
3039
ServicePrincipalName: a.ServicePrincipalName,
@@ -43,21 +52,35 @@ func (p WorkspacePathPermissions) Compare(perms []resources.Permission) diag.Dia
4352
if !ok {
4453
diags = diags.Append(diag.Diagnostic{
4554
Severity: diag.Warning,
46-
Summary: "untracked permissions apply to target workspace path",
47-
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)),
55+
Summary: "workspace folder has permissions not configured in bundle",
56+
Detail: fmt.Sprintf(
57+
"The following permissions apply to the workspace folder at %q "+
58+
"but are not configured in the bundle:\n%s\n"+
59+
"Add them to your bundle permissions or remove them from the folder.\n"+
60+
"See https://docs.databricks.com/dev-tools/bundles/permissions",
61+
p.Path, toString(missing)),
4862
})
4963
}
5064

5165
return diags
5266
}
5367

54-
// containsAll checks if permA contains all permissions in permB.
68+
// samePrincipal checks if two permissions refer to the same user/group/service principal.
69+
func samePrincipal(a, b resources.Permission) bool {
70+
return a.UserName == b.UserName &&
71+
a.GroupName == b.GroupName &&
72+
a.ServicePrincipalName == b.ServicePrincipalName
73+
}
74+
75+
// containsAll checks if all permissions in permA (workspace) are accounted for in permB (bundle).
76+
// A workspace permission is considered accounted for if the bundle has the same principal
77+
// with an equal or higher permission level.
5578
func containsAll(permA, permB []resources.Permission) (bool, []resources.Permission) {
5679
var missing []resources.Permission
5780
for _, a := range permA {
5881
found := false
5982
for _, b := range permB {
60-
if a == b {
83+
if samePrincipal(a, b) && resources.GetLevelScore(b.Level) >= resources.GetLevelScore(a.Level) {
6184
found = true
6285
break
6386
}

0 commit comments

Comments
 (0)