diff --git a/bundle/config/validate/folder_permissions.go b/bundle/config/validate/folder_permissions.go index b80aaeb3..ce3bb1a2 100644 --- a/bundle/config/validate/folder_permissions.go +++ b/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) - return nil + diags, err, _ := syncGroup.Do(p, func() (any, error) { + 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 { - return obj, nil - } - - for folderPath != "/" { + for { obj, err := w.GetStatusByPath(ctx, folderPath) if err == nil { - cache[folderPath] = obj return obj, nil } - var aerr *apierr.APIError - if !errors.As(err, &aerr) { + if !apierr.IsMissing(err) { return nil, err } - if aerr.ErrorCode != "RESOURCE_DOES_NOT_EXIST" { - return nil, err + parent := path.Dir(folderPath) + // If the parent is the same as the current folder, then we have reached the root + if folderPath == parent { + break } - folderPath = path.Dir(folderPath) - } - - // Check "/" root folder - obj, err := w.GetStatusByPath(ctx, folderPath) - if err == nil { - cache[folderPath] = obj - return obj, nil + folderPath = parent } 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{} } diff --git a/bundle/config/validate/folder_permissions_test.go b/bundle/config/validate/folder_permissions_test.go index d21d3e9a..83e96167 100644 --- a/bundle/config/validate/folder_permissions_test.go +++ b/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.Equal(t, "permission not found", diags[0].Summary) + require.Len(t, diags, 2) + 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) -} - -func setupTest(t *testing.T) { - cache = make(map[string]*workspace.ObjectInfo) + require.Equal(t, diag.Error, diags[0].Severity) } diff --git a/bundle/permissions/check.go b/bundle/permissions/check.go index 3377e12c..27304a44 100644 --- a/bundle/permissions/check.go +++ b/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", - Detail: fmt.Sprintf( - "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)), + Summary: "permissions missing", + Detail: fmt.Sprintf("Following permissions set in the bundle but not set for workspace folder %s:\n%s", p.Path, toString(missing)), }) - return diags } - for _, perm := range perms { - level, err := GetWorkspaceObjectPermissionLevel(perm.Level) - if err != nil { - return diag.FromErr(err) - } - - found := false - 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)), - }) - } + // Check the permissions in the workspace and see if they are all set in the bundle. + ok, missing = containsAll(p.Permissions, perms) + if !ok { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: "permissions missing", + Detail: fmt.Sprintf("Following permissions set for the workspace folder but not set for bundle %s:\n%s", p.Path, toString(missing)), + }) } 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() } diff --git a/bundle/permissions/check_test.go b/bundle/permissions/check_test.go new file mode 100644 index 00000000..164894e1 --- /dev/null +++ b/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) + } + +}