From 793a8363d045761441280099b9a5c139fffeebb0 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 10 Oct 2024 15:16:06 +0200 Subject: [PATCH 1/9] Added validator for folder permissions --- bundle/config/resources/permission.go | 18 ++ bundle/config/validate/folder_permissions.go | 154 ++++++++++++++++++ .../validate/folder_permissions_test.go | 152 +++++++++++++++++ bundle/config/validate/validate.go | 1 + bundle/permissions/workspace_root.go | 4 +- 5 files changed, 327 insertions(+), 2 deletions(-) create mode 100644 bundle/config/validate/folder_permissions.go create mode 100644 bundle/config/validate/folder_permissions_test.go diff --git a/bundle/config/resources/permission.go b/bundle/config/resources/permission.go index fa2d8796c2..62e18a09ee 100644 --- a/bundle/config/resources/permission.go +++ b/bundle/config/resources/permission.go @@ -1,5 +1,7 @@ package resources +import "fmt" + // Permission holds the permission level setting for a single principal. // Multiple of these can be defined on any resource. type Permission struct { @@ -9,3 +11,19 @@ type Permission struct { ServicePrincipalName string `json:"service_principal_name,omitempty"` GroupName string `json:"group_name,omitempty"` } + +func (p Permission) String() string { + if p.UserName != "" { + return fmt.Sprintf("level: %s, user_name: %s", p.Level, p.UserName) + } + + if p.ServicePrincipalName != "" { + return fmt.Sprintf("level: %s, service_principal_name: %s", p.Level, p.ServicePrincipalName) + } + + if p.GroupName != "" { + return fmt.Sprintf("level: %s, group_name: %s", p.Level, p.GroupName) + } + + return fmt.Sprintf("level: %s", p.Level) +} diff --git a/bundle/config/validate/folder_permissions.go b/bundle/config/validate/folder_permissions.go new file mode 100644 index 0000000000..dc976fc50d --- /dev/null +++ b/bundle/config/validate/folder_permissions.go @@ -0,0 +1,154 @@ +package validate + +import ( + "context" + "fmt" + "strings" + + "github.com/databricks/cli/bundle" + "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/service/workspace" + "golang.org/x/sync/errgroup" +) + +type folderPermissions struct { +} + +// Apply implements bundle.ReadOnlyMutator. +func (f *folderPermissions) Apply(ctx context.Context, b bundle.ReadOnlyBundle) diag.Diagnostics { + if len(b.Config().Permissions) == 0 { + return nil + } + + paths := []string{b.Config().Workspace.RootPath} + + if !strings.HasPrefix(b.Config().Workspace.ArtifactPath, b.Config().Workspace.RootPath) { + paths = append(paths, b.Config().Workspace.ArtifactPath) + } + + if !strings.HasPrefix(b.Config().Workspace.FilePath, b.Config().Workspace.RootPath) { + paths = append(paths, b.Config().Workspace.FilePath) + } + + if !strings.HasPrefix(b.Config().Workspace.StatePath, b.Config().Workspace.RootPath) { + paths = append(paths, b.Config().Workspace.StatePath) + } + + if !strings.HasPrefix(b.Config().Workspace.ResourcePath, b.Config().Workspace.RootPath) { + paths = append(paths, b.Config().Workspace.ResourcePath) + } + + var diags diag.Diagnostics + errGrp, errCtx := errgroup.WithContext(ctx) + for _, path := range paths { + p := path + errGrp.Go(func() error { + diags = diags.Extend(checkFolderPermission(errCtx, b, p)) + return nil + }) + } + + if err := errGrp.Wait(); err != nil { + return diag.FromErr(err) + } + + return diags +} + +func checkFolderPermission(ctx context.Context, b bundle.ReadOnlyBundle, folderPath string) diag.Diagnostics { + var diags diag.Diagnostics + w := b.WorkspaceClient().Workspace + obj, err := w.GetStatusByPath(ctx, folderPath) + if err != nil { + return diag.FromErr(err) + } + + objPermissions, err := w.GetPermissions(ctx, workspace.GetWorkspaceObjectPermissionsRequest{ + WorkspaceObjectId: fmt.Sprint(obj.ObjectId), + WorkspaceObjectType: "directories", + }) + if err != nil { + return diag.FromErr(err) + } + + if len(objPermissions.AccessControlList) != len(b.Config().Permissions) { + 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", len(b.Config().Permissions), len(objPermissions.AccessControlList), permissionDetails(objPermissions.AccessControlList, b.Config().Permissions)), + }) + return diags + } + + for _, p := range b.Config().Permissions { + level, err := permissions.GetWorkspaceObjectPermissionLevel(p.Level) + if err != nil { + return diag.FromErr(err) + } + + found := false + for _, objPerm := range objPermissions.AccessControlList { + if objPerm.GroupName == p.GroupName && objPerm.UserName == p.UserName && objPerm.ServicePrincipalName == p.ServicePrincipalName { + for _, l := range objPerm.AllPermissions { + if l.PermissionLevel == 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", p, folderPath, permissionDetails(objPermissions.AccessControlList, b.Config().Permissions)), + }) + } + } + + return diags +} + +func permissionDetails(acl []workspace.WorkspaceObjectAccessControlResponse, p []resources.Permission) string { + return fmt.Sprintf("Bundle permissions:\n%s\nWorkspace permissions:\n%s", permissionsToString(p), aclToString(acl)) +} + +func aclToString(acl []workspace.WorkspaceObjectAccessControlResponse) string { + var sb strings.Builder + for _, p := range acl { + levels := make([]string, len(p.AllPermissions)) + for i, l := range p.AllPermissions { + levels[i] = string(l.PermissionLevel) + } + if p.UserName != "" { + sb.WriteString(fmt.Sprintf("- levels: %s, user_name: %s\n", levels, p.UserName)) + } + if p.GroupName != "" { + sb.WriteString(fmt.Sprintf("- levels: %s, group_name: %s\n", levels, p.GroupName)) + } + if p.ServicePrincipalName != "" { + sb.WriteString(fmt.Sprintf("- levels: %s, service_principal_name: %s\n", levels, p.ServicePrincipalName)) + } + } + return sb.String() +} + +func permissionsToString(p []resources.Permission) string { + var sb strings.Builder + for _, perm := range p { + sb.WriteString(fmt.Sprintf("- %s\n", perm)) + } + return sb.String() +} + +// Name implements bundle.ReadOnlyMutator. +func (f *folderPermissions) Name() string { + return "validate:folder_permissions" +} + +func ValidateFolderPermissions() bundle.ReadOnlyMutator { + return &folderPermissions{} +} diff --git a/bundle/config/validate/folder_permissions_test.go b/bundle/config/validate/folder_permissions_test.go new file mode 100644 index 0000000000..71dccac60d --- /dev/null +++ b/bundle/config/validate/folder_permissions_test.go @@ -0,0 +1,152 @@ +package validate + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/permissions" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/workspace" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestValidateFolderPermissions(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Workspace/Users/foo@bar.com", + ArtifactPath: "/Workspace/Users/foo@bar.com/artifacts", + FilePath: "/Workspace/Users/foo@bar.com/files", + StatePath: "/Workspace/Users/foo@bar.com/state", + ResourcePath: "/Workspace/Users/foo@bar.com/resources", + }, + Permissions: []resources.Permission{ + {Level: permissions.CAN_MANAGE, UserName: "foo@bar.com"}, + }, + }, + } + m := mocks.NewMockWorkspaceClient(t) + api := m.GetMockWorkspaceAPI() + api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Users/foo@bar.com").Return(&workspace.ObjectInfo{ + ObjectId: 1234, + }, nil) + + api.EXPECT().GetPermissions(mock.Anything, workspace.GetWorkspaceObjectPermissionsRequest{ + WorkspaceObjectId: "1234", + WorkspaceObjectType: "directories", + }).Return(&workspace.WorkspaceObjectPermissions{ + ObjectId: "1234", + AccessControlList: []workspace.WorkspaceObjectAccessControlResponse{ + { + UserName: "foo@bar.com", + AllPermissions: []workspace.WorkspaceObjectPermission{ + {PermissionLevel: "CAN_MANAGE"}, + }, + }, + }, + }, nil) + + b.SetWorkpaceClient(m.WorkspaceClient) + rb := bundle.ReadOnly(b) + + diags := bundle.ApplyReadOnly(context.Background(), rb, ValidateFolderPermissions()) + require.Empty(t, diags) +} + +func TestValidateFolderPermissionsDifferentCount(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Workspace/Users/foo@bar.com", + ArtifactPath: "/Workspace/Users/foo@bar.com/artifacts", + FilePath: "/Workspace/Users/foo@bar.com/files", + StatePath: "/Workspace/Users/foo@bar.com/state", + ResourcePath: "/Workspace/Users/foo@bar.com/resources", + }, + Permissions: []resources.Permission{ + {Level: permissions.CAN_MANAGE, UserName: "foo@bar.com"}, + }, + }, + } + m := mocks.NewMockWorkspaceClient(t) + api := m.GetMockWorkspaceAPI() + api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Users/foo@bar.com").Return(&workspace.ObjectInfo{ + ObjectId: 1234, + }, nil) + + api.EXPECT().GetPermissions(mock.Anything, workspace.GetWorkspaceObjectPermissionsRequest{ + WorkspaceObjectId: "1234", + WorkspaceObjectType: "directories", + }).Return(&workspace.WorkspaceObjectPermissions{ + ObjectId: "1234", + AccessControlList: []workspace.WorkspaceObjectAccessControlResponse{ + { + UserName: "foo@bar.com", + AllPermissions: []workspace.WorkspaceObjectPermission{ + {PermissionLevel: "CAN_MANAGE"}, + }, + }, + { + UserName: "foo2@bar.com", + AllPermissions: []workspace.WorkspaceObjectPermission{ + {PermissionLevel: "CAN_MANAGE"}, + }, + }, + }, + }, nil) + + b.SetWorkpaceClient(m.WorkspaceClient) + rb := bundle.ReadOnly(b) + + diags := bundle.ApplyReadOnly(context.Background(), rb, ValidateFolderPermissions()) + require.Len(t, diags, 1) + require.Equal(t, "permissions count mismatch", diags[0].Summary) +} + +func TestValidateFolderPermissionsDifferentPermission(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Workspace/Users/foo@bar.com", + ArtifactPath: "/Workspace/Users/foo@bar.com/artifacts", + FilePath: "/Workspace/Users/foo@bar.com/files", + StatePath: "/Workspace/Users/foo@bar.com/state", + ResourcePath: "/Workspace/Users/foo@bar.com/resources", + }, + Permissions: []resources.Permission{ + {Level: permissions.CAN_MANAGE, UserName: "foo@bar.com"}, + }, + }, + } + m := mocks.NewMockWorkspaceClient(t) + api := m.GetMockWorkspaceAPI() + api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Users/foo@bar.com").Return(&workspace.ObjectInfo{ + ObjectId: 1234, + }, nil) + + api.EXPECT().GetPermissions(mock.Anything, workspace.GetWorkspaceObjectPermissionsRequest{ + WorkspaceObjectId: "1234", + WorkspaceObjectType: "directories", + }).Return(&workspace.WorkspaceObjectPermissions{ + ObjectId: "1234", + AccessControlList: []workspace.WorkspaceObjectAccessControlResponse{ + { + UserName: "foo2@bar.com", + AllPermissions: []workspace.WorkspaceObjectPermission{ + {PermissionLevel: "CAN_MANAGE"}, + }, + }, + }, + }, nil) + + b.SetWorkpaceClient(m.WorkspaceClient) + 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) +} diff --git a/bundle/config/validate/validate.go b/bundle/config/validate/validate.go index 79f42bd232..440477e654 100644 --- a/bundle/config/validate/validate.go +++ b/bundle/config/validate/validate.go @@ -35,6 +35,7 @@ func (v *validate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics FilesToSync(), ValidateSyncPatterns(), JobTaskClusterSpec(), + ValidateFolderPermissions(), )) } diff --git a/bundle/permissions/workspace_root.go b/bundle/permissions/workspace_root.go index a59a039f6f..3630806799 100644 --- a/bundle/permissions/workspace_root.go +++ b/bundle/permissions/workspace_root.go @@ -34,7 +34,7 @@ func giveAccessForWorkspaceRoot(ctx context.Context, b *bundle.Bundle) error { permissions := make([]workspace.WorkspaceObjectAccessControlRequest, 0) for _, p := range b.Config.Permissions { - level, err := getWorkspaceObjectPermissionLevel(p.Level) + level, err := GetWorkspaceObjectPermissionLevel(p.Level) if err != nil { return err } @@ -65,7 +65,7 @@ func giveAccessForWorkspaceRoot(ctx context.Context, b *bundle.Bundle) error { return err } -func getWorkspaceObjectPermissionLevel(bundlePermission string) (workspace.WorkspaceObjectPermissionLevel, error) { +func GetWorkspaceObjectPermissionLevel(bundlePermission string) (workspace.WorkspaceObjectPermissionLevel, error) { switch bundlePermission { case CAN_MANAGE: return workspace.WorkspaceObjectPermissionLevelCanManage, nil From 0872d2a1f91957faee62ff10f78a280e5985ef61 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Mon, 14 Oct 2024 12:33:47 +0200 Subject: [PATCH 2/9] check permissions for parent folder --- bundle/config/validate/folder_permissions.go | 131 ++++++++---------- .../validate/folder_permissions_test.go | 53 ++++++- .../libraries/{workspace_path.go => path.go} | 5 + .../{workspace_path_test.go => path_test.go} | 10 ++ bundle/permissions/check.go | 92 ++++++++++++ 5 files changed, 218 insertions(+), 73 deletions(-) rename bundle/libraries/{workspace_path.go => path.go} (86%) rename bundle/libraries/{workspace_path_test.go => path_test.go} (77%) create mode 100644 bundle/permissions/check.go diff --git a/bundle/config/validate/folder_permissions.go b/bundle/config/validate/folder_permissions.go index dc976fc50d..b80aaeb34d 100644 --- a/bundle/config/validate/folder_permissions.go +++ b/bundle/config/validate/folder_permissions.go @@ -2,13 +2,16 @@ package validate import ( "context" + "errors" "fmt" + "path" "strings" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/libraries" "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/service/workspace" "golang.org/x/sync/errgroup" ) @@ -22,45 +25,60 @@ func (f *folderPermissions) Apply(ctx context.Context, b bundle.ReadOnlyBundle) return nil } - paths := []string{b.Config().Workspace.RootPath} + rootPath := b.Config().Workspace.RootPath + paths := []string{} + if !libraries.IsVolumesPath(rootPath) { + paths = append(paths, rootPath) + } + + if !strings.HasSuffix(rootPath, "/") { + rootPath += "/" + } - if !strings.HasPrefix(b.Config().Workspace.ArtifactPath, b.Config().Workspace.RootPath) { + if !strings.HasPrefix(b.Config().Workspace.ArtifactPath, rootPath) && + !libraries.IsVolumesPath(b.Config().Workspace.ArtifactPath) { paths = append(paths, b.Config().Workspace.ArtifactPath) } - if !strings.HasPrefix(b.Config().Workspace.FilePath, b.Config().Workspace.RootPath) { + if !strings.HasPrefix(b.Config().Workspace.FilePath, rootPath) && + !libraries.IsVolumesPath(b.Config().Workspace.FilePath) { paths = append(paths, b.Config().Workspace.FilePath) } - if !strings.HasPrefix(b.Config().Workspace.StatePath, b.Config().Workspace.RootPath) { + if !strings.HasPrefix(b.Config().Workspace.StatePath, rootPath) && + !libraries.IsVolumesPath(b.Config().Workspace.StatePath) { paths = append(paths, b.Config().Workspace.StatePath) } - if !strings.HasPrefix(b.Config().Workspace.ResourcePath, b.Config().Workspace.RootPath) { + if !strings.HasPrefix(b.Config().Workspace.ResourcePath, rootPath) && + !libraries.IsVolumesPath(b.Config().Workspace.ResourcePath) { paths = append(paths, b.Config().Workspace.ResourcePath) } var diags diag.Diagnostics - errGrp, errCtx := errgroup.WithContext(ctx) - for _, path := range paths { - p := path - errGrp.Go(func() error { - diags = diags.Extend(checkFolderPermission(errCtx, b, p)) + g, ctx := errgroup.WithContext(ctx) + results := make([]diag.Diagnostics, len(paths)) + for i, p := range paths { + g.Go(func() error { + results[i] = checkFolderPermission(ctx, b, p) return nil }) } - if err := errGrp.Wait(); err != nil { + if err := g.Wait(); err != nil { return diag.FromErr(err) } + for _, r := range results { + diags = diags.Extend(r) + } + return diags } func checkFolderPermission(ctx context.Context, b bundle.ReadOnlyBundle, folderPath string) diag.Diagnostics { - var diags diag.Diagnostics w := b.WorkspaceClient().Workspace - obj, err := w.GetStatusByPath(ctx, folderPath) + obj, err := getClosestExistingObject(ctx, w, folderPath) if err != nil { return diag.FromErr(err) } @@ -73,75 +91,44 @@ func checkFolderPermission(ctx context.Context, b bundle.ReadOnlyBundle, folderP return diag.FromErr(err) } - if len(objPermissions.AccessControlList) != len(b.Config().Permissions) { - 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", len(b.Config().Permissions), len(objPermissions.AccessControlList), permissionDetails(objPermissions.AccessControlList, b.Config().Permissions)), - }) - return diags + p := permissions.NewFromWorkspaceObjectAcl(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 _, p := range b.Config().Permissions { - level, err := permissions.GetWorkspaceObjectPermissionLevel(p.Level) - if err != nil { - return diag.FromErr(err) + for folderPath != "/" { + obj, err := w.GetStatusByPath(ctx, folderPath) + if err == nil { + cache[folderPath] = obj + return obj, nil } - found := false - for _, objPerm := range objPermissions.AccessControlList { - if objPerm.GroupName == p.GroupName && objPerm.UserName == p.UserName && objPerm.ServicePrincipalName == p.ServicePrincipalName { - for _, l := range objPerm.AllPermissions { - if l.PermissionLevel == level { - found = true - break - } - } - } + var aerr *apierr.APIError + if !errors.As(err, &aerr) { + return nil, err } - 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", p, folderPath, permissionDetails(objPermissions.AccessControlList, b.Config().Permissions)), - }) + if aerr.ErrorCode != "RESOURCE_DOES_NOT_EXIST" { + return nil, err } - } - return diags -} - -func permissionDetails(acl []workspace.WorkspaceObjectAccessControlResponse, p []resources.Permission) string { - return fmt.Sprintf("Bundle permissions:\n%s\nWorkspace permissions:\n%s", permissionsToString(p), aclToString(acl)) -} - -func aclToString(acl []workspace.WorkspaceObjectAccessControlResponse) string { - var sb strings.Builder - for _, p := range acl { - levels := make([]string, len(p.AllPermissions)) - for i, l := range p.AllPermissions { - levels[i] = string(l.PermissionLevel) - } - if p.UserName != "" { - sb.WriteString(fmt.Sprintf("- levels: %s, user_name: %s\n", levels, p.UserName)) - } - if p.GroupName != "" { - sb.WriteString(fmt.Sprintf("- levels: %s, group_name: %s\n", levels, p.GroupName)) - } - if p.ServicePrincipalName != "" { - sb.WriteString(fmt.Sprintf("- levels: %s, service_principal_name: %s\n", levels, p.ServicePrincipalName)) - } + folderPath = path.Dir(folderPath) } - return sb.String() -} -func permissionsToString(p []resources.Permission) string { - var sb strings.Builder - for _, perm := range p { - sb.WriteString(fmt.Sprintf("- %s\n", perm)) + // Check "/" root folder + obj, err := w.GetStatusByPath(ctx, folderPath) + if err == nil { + cache[folderPath] = obj + return obj, nil } - return sb.String() + + return nil, fmt.Errorf("folder %s and its parent folders do not exist", folderPath) } // Name implements bundle.ReadOnlyMutator. diff --git a/bundle/config/validate/folder_permissions_test.go b/bundle/config/validate/folder_permissions_test.go index 71dccac60d..d21d3e9ada 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/databricks-sdk-go/apierr" "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/databricks/databricks-sdk-go/service/workspace" "github.com/stretchr/testify/mock" @@ -15,6 +16,7 @@ import ( ) func TestValidateFolderPermissions(t *testing.T) { + setupTest(t) b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ @@ -31,7 +33,15 @@ func TestValidateFolderPermissions(t *testing.T) { } m := mocks.NewMockWorkspaceClient(t) api := m.GetMockWorkspaceAPI() - api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Users/foo@bar.com").Return(&workspace.ObjectInfo{ + api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Users/foo@bar.com").Return(nil, &apierr.APIError{ + StatusCode: 404, + ErrorCode: "RESOURCE_DOES_NOT_EXIST", + }) + api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Users").Return(nil, &apierr.APIError{ + StatusCode: 404, + ErrorCode: "RESOURCE_DOES_NOT_EXIST", + }) + api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace").Return(&workspace.ObjectInfo{ ObjectId: 1234, }, nil) @@ -58,6 +68,7 @@ func TestValidateFolderPermissions(t *testing.T) { } func TestValidateFolderPermissionsDifferentCount(t *testing.T) { + setupTest(t) b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ @@ -108,6 +119,7 @@ func TestValidateFolderPermissionsDifferentCount(t *testing.T) { } func TestValidateFolderPermissionsDifferentPermission(t *testing.T) { + setupTest(t) b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ @@ -150,3 +162,42 @@ func TestValidateFolderPermissionsDifferentPermission(t *testing.T) { require.Len(t, diags, 1) require.Equal(t, "permission not found", diags[0].Summary) } + +func TestNoRootFolder(t *testing.T) { + setupTest(t) + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/NotExisting", + ArtifactPath: "/NotExisting/artifacts", + FilePath: "/NotExisting/files", + StatePath: "/NotExisting/state", + ResourcePath: "/NotExisting/resources", + }, + Permissions: []resources.Permission{ + {Level: permissions.CAN_MANAGE, UserName: "foo@bar.com"}, + }, + }, + } + m := mocks.NewMockWorkspaceClient(t) + api := m.GetMockWorkspaceAPI() + api.EXPECT().GetStatusByPath(mock.Anything, "/NotExisting").Return(nil, &apierr.APIError{ + StatusCode: 404, + ErrorCode: "RESOURCE_DOES_NOT_EXIST", + }) + api.EXPECT().GetStatusByPath(mock.Anything, "/").Return(nil, &apierr.APIError{ + StatusCode: 404, + ErrorCode: "RESOURCE_DOES_NOT_EXIST", + }) + + b.SetWorkpaceClient(m.WorkspaceClient) + rb := bundle.ReadOnly(b) + + 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) +} diff --git a/bundle/libraries/workspace_path.go b/bundle/libraries/path.go similarity index 86% rename from bundle/libraries/workspace_path.go rename to bundle/libraries/path.go index 126ad3f13e..4533f6de35 100644 --- a/bundle/libraries/workspace_path.go +++ b/bundle/libraries/path.go @@ -36,3 +36,8 @@ func IsWorkspaceLibrary(library *compute.Library) bool { return IsWorkspacePath(path) } + +// IsVolumesPath returns true if the specified path indicates that +func IsVolumesPath(path string) bool { + return strings.HasPrefix(path, "/Volumes/") +} diff --git a/bundle/libraries/workspace_path_test.go b/bundle/libraries/path_test.go similarity index 77% rename from bundle/libraries/workspace_path_test.go rename to bundle/libraries/path_test.go index feaaab7f7b..90fe187a25 100644 --- a/bundle/libraries/workspace_path_test.go +++ b/bundle/libraries/path_test.go @@ -31,3 +31,13 @@ func TestIsWorkspaceLibrary(t *testing.T) { // Empty. assert.False(t, IsWorkspaceLibrary(&compute.Library{})) } + +func TestIsVolumesPath(t *testing.T) { + // Absolute paths with particular prefixes. + assert.True(t, IsVolumesPath("/Volumes/path/to/package")) + + // Relative paths. + assert.False(t, IsVolumesPath("myfile.txt")) + assert.False(t, IsVolumesPath("./myfile.txt")) + assert.False(t, IsVolumesPath("../myfile.txt")) +} diff --git a/bundle/permissions/check.go b/bundle/permissions/check.go new file mode 100644 index 0000000000..3377e12cf3 --- /dev/null +++ b/bundle/permissions/check.go @@ -0,0 +1,92 @@ +package permissions + +import ( + "fmt" + "strings" + + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/databricks-sdk-go/service/workspace" +) + +type WorkspacePathPermissions struct { + Path string + Permissions []resources.Permission +} + +func NewFromWorkspaceObjectAcl(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. + if a.GroupName == "admin" { + continue + } + + for _, pl := range a.AllPermissions { + permissions = append(permissions, resources.Permission{ + Level: string(pl.PermissionLevel), + GroupName: a.GroupName, + UserName: a.UserName, + ServicePrincipalName: a.ServicePrincipalName, + }) + } + } + + return &WorkspacePathPermissions{Permissions: permissions, Path: path} +} + +func (p WorkspacePathPermissions) Compare(perms []resources.Permission) diag.Diagnostics { + var diags diag.Diagnostics + + if len(p.Permissions) != len(perms) { + 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)), + }) + 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)), + }) + } + } + + return diags +} + +func toString(title string, 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)) + } + return sb.String() +} From f0a4e9e67f27392565ae42ae6d21881399dbe0fb Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 16 Oct 2024 14:32:20 +0200 Subject: [PATCH 3/9] refactoring + tests --- bundle/config/validate/folder_permissions.go | 42 +++--- .../validate/folder_permissions_test.go | 22 +-- bundle/permissions/check.go | 84 ++++++----- bundle/permissions/check_test.go | 132 ++++++++++++++++++ 4 files changed, 213 insertions(+), 67 deletions(-) create mode 100644 bundle/permissions/check_test.go diff --git a/bundle/config/validate/folder_permissions.go b/bundle/config/validate/folder_permissions.go index b80aaeb34d..ce3bb1a2f7 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 d21d3e9ada..83e9616760 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 3377e12cf3..27304a448e 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) - } + // 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 +} + +// 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 _, objPerm := range p.Permissions { - if objPerm.GroupName == perm.GroupName && - objPerm.UserName == perm.UserName && - objPerm.ServicePrincipalName == perm.ServicePrincipalName && - objPerm.Level == string(level) { + for _, b := range permB { + if a == b { 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)), - }) + missing = append(missing, a) } } + return len(missing) == 0, missing +} - return diags +// 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(title string, p []resources.Permission) string { +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 0000000000..164894e11f --- /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) + } + +} From 95f45afc5d9f9ef2df2bd9c9689cf479e5097d62 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 16 Oct 2024 15:32:03 +0200 Subject: [PATCH 4/9] Update bundle/permissions/check.go Co-authored-by: Pieter Noordhuis --- bundle/permissions/check.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/permissions/check.go b/bundle/permissions/check.go index 27304a448e..956b3be4ed 100644 --- a/bundle/permissions/check.go +++ b/bundle/permissions/check.go @@ -44,7 +44,7 @@ func (p WorkspacePathPermissions) Compare(perms []resources.Permission) diag.Dia diags = diags.Append(diag.Diagnostic{ Severity: diag.Warning, 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)), + Detail: fmt.Sprintf("The following permissions are configured in the bundle but are do not (yet) apply to the workspace folder at %q:\n%s", p.Path, toString(missing)), }) } From 21799b5d8335774fb778c7e31689bf14bb02a4f3 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 16 Oct 2024 15:32:13 +0200 Subject: [PATCH 5/9] Update bundle/permissions/check.go Co-authored-by: Pieter Noordhuis --- bundle/permissions/check.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/permissions/check.go b/bundle/permissions/check.go index 956b3be4ed..f5229afee5 100644 --- a/bundle/permissions/check.go +++ b/bundle/permissions/check.go @@ -54,7 +54,7 @@ func (p WorkspacePathPermissions) Compare(perms []resources.Permission) diag.Dia 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)), + 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)), }) } From 020705df71d5da2e90c82f130acf1557f32439bf Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 22 Oct 2024 15:32:54 +0200 Subject: [PATCH 6/9] fixes --- bundle/config/validate/folder_permissions.go | 10 ++-------- .../validate/folder_permissions_test.go | 20 +++++++++++++------ bundle/permissions/check.go | 15 +------------- bundle/permissions/check_test.go | 8 ++++---- 4 files changed, 21 insertions(+), 32 deletions(-) diff --git a/bundle/config/validate/folder_permissions.go b/bundle/config/validate/folder_permissions.go index ce3bb1a2f7..0541563035 100644 --- a/bundle/config/validate/folder_permissions.go +++ b/bundle/config/validate/folder_permissions.go @@ -13,7 +13,6 @@ 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,15 +57,10 @@ 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 { - diags, err, _ := syncGroup.Do(p, func() (any, error) { - diags := checkFolderPermission(ctx, b, p) - return diags, nil - }) - results[i] = diags.(diag.Diagnostics) - return err + results[i] = checkFolderPermission(ctx, b, p) + return nil }) } diff --git a/bundle/config/validate/folder_permissions_test.go b/bundle/config/validate/folder_permissions_test.go index 83e9616760..fc38291df9 100644 --- a/bundle/config/validate/folder_permissions_test.go +++ b/bundle/config/validate/folder_permissions_test.go @@ -16,12 +16,12 @@ import ( "github.com/stretchr/testify/require" ) -func TestValidateFolderPermissions(t *testing.T) { +func TestFolderPermissionsInheritedWhenRootPathDoesNotExist(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ RootPath: "/Workspace/Users/foo@bar.com", - ArtifactPath: "/Workspace/Users/foo@bar.com/artifacts", + ArtifactPath: "/Workspace/Users/otherfoo@bar.com/artifacts", FilePath: "/Workspace/Users/foo@bar.com/files", StatePath: "/Workspace/Users/foo@bar.com/state", ResourcePath: "/Workspace/Users/foo@bar.com/resources", @@ -33,6 +33,14 @@ func TestValidateFolderPermissions(t *testing.T) { } m := mocks.NewMockWorkspaceClient(t) api := m.GetMockWorkspaceAPI() + api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Users/otherfoo@bar.com/artifacts").Return(nil, &apierr.APIError{ + StatusCode: 404, + ErrorCode: "RESOURCE_DOES_NOT_EXIST", + }) + api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Users/otherfoo@bar.com").Return(nil, &apierr.APIError{ + StatusCode: 404, + ErrorCode: "RESOURCE_DOES_NOT_EXIST", + }) api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Users/foo@bar.com").Return(nil, &apierr.APIError{ StatusCode: 404, ErrorCode: "RESOURCE_DOES_NOT_EXIST", @@ -67,7 +75,7 @@ func TestValidateFolderPermissions(t *testing.T) { require.Empty(t, diags) } -func TestValidateFolderPermissionsDifferentCount(t *testing.T) { +func TestValidateFolderPermissionsFailsOnMissingBundlePermission(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ @@ -116,10 +124,10 @@ func TestValidateFolderPermissionsDifferentCount(t *testing.T) { require.Len(t, diags, 1) 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) + 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\n user_name: foo2@bar.com\n", diags[0].Detail) } -func TestValidateFolderPermissionsDifferentPermission(t *testing.T) { +func TestValidateFolderPermissionsFailsOnPermissionMismatch(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ @@ -167,7 +175,7 @@ func TestValidateFolderPermissionsDifferentPermission(t *testing.T) { require.Equal(t, diag.Warning, diags[1].Severity) } -func TestNoRootFolder(t *testing.T) { +func TestValidateFolderPermissionsFailsOnNoRootFolder(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ diff --git a/bundle/permissions/check.go b/bundle/permissions/check.go index f5229afee5..f37c956c38 100644 --- a/bundle/permissions/check.go +++ b/bundle/permissions/check.go @@ -93,20 +93,7 @@ func convertWorkspaceObjectPermissionLevel(level workspace.WorkspaceObjectPermis func toString(p []resources.Permission) string { var sb strings.Builder for _, perm := range p { - 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 - } + sb.WriteString(fmt.Sprintf("- %s\n", perm.String())) } return sb.String() } diff --git a/bundle/permissions/check_test.go b/bundle/permissions/check_test.go index 164894e11f..1c55e1f682 100644 --- a/bundle/permissions/check_test.go +++ b/bundle/permissions/check_test.go @@ -66,7 +66,7 @@ func TestWorkspacePathPermissionsCompare(t *testing.T) { { 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", + Detail: "The following permissions are configured in the bundle but are do not (yet) apply to the workspace folder at \"path\":\n- level: CAN_MANAGE, service_principal_name: sp.com\n", }, }, }, @@ -92,7 +92,7 @@ func TestWorkspacePathPermissionsCompare(t *testing.T) { { 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", + 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", }, }, }, @@ -112,12 +112,12 @@ func TestWorkspacePathPermissionsCompare(t *testing.T) { { 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", + Detail: "The following permissions are configured in the bundle but are do not (yet) apply to the workspace folder at \"path\":\n- level: CAN_MANAGE, 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", + 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", }, }, }, From 1ba769c68f874ba288c10fe335627e23e6a349be Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 22 Oct 2024 15:36:16 +0200 Subject: [PATCH 7/9] fixed test --- bundle/config/validate/folder_permissions_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/validate/folder_permissions_test.go b/bundle/config/validate/folder_permissions_test.go index fc38291df9..f98ea66a12 100644 --- a/bundle/config/validate/folder_permissions_test.go +++ b/bundle/config/validate/folder_permissions_test.go @@ -124,7 +124,7 @@ func TestValidateFolderPermissionsFailsOnMissingBundlePermission(t *testing.T) { require.Len(t, diags, 1) require.Equal(t, "permissions missing", 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\n user_name: foo2@bar.com\n", diags[0].Detail) + 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) } func TestValidateFolderPermissionsFailsOnPermissionMismatch(t *testing.T) { From acac0289c57690f6e375c050bff0cfd632c5296a Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 24 Oct 2024 13:31:37 +0200 Subject: [PATCH 8/9] fixes --- bundle/config/validate/folder_permissions.go | 27 +++++++------------ .../validate/folder_permissions_test.go | 9 +++---- ...check.go => workspace_path_permissions.go} | 16 +++-------- ....go => workspace_path_permissions_test.go} | 19 +++---------- 4 files changed, 19 insertions(+), 52 deletions(-) rename bundle/permissions/{check.go => workspace_path_permissions.go} (82%) rename bundle/permissions/{check_test.go => workspace_path_permissions_test.go} (81%) diff --git a/bundle/config/validate/folder_permissions.go b/bundle/config/validate/folder_permissions.go index 0541563035..acd0283b33 100644 --- a/bundle/config/validate/folder_permissions.go +++ b/bundle/config/validate/folder_permissions.go @@ -34,24 +34,15 @@ func (f *folderPermissions) Apply(ctx context.Context, b bundle.ReadOnlyBundle) rootPath += "/" } - if !strings.HasPrefix(b.Config().Workspace.ArtifactPath, rootPath) && - !libraries.IsVolumesPath(b.Config().Workspace.ArtifactPath) { - paths = append(paths, b.Config().Workspace.ArtifactPath) - } - - if !strings.HasPrefix(b.Config().Workspace.FilePath, rootPath) && - !libraries.IsVolumesPath(b.Config().Workspace.FilePath) { - paths = append(paths, b.Config().Workspace.FilePath) - } - - if !strings.HasPrefix(b.Config().Workspace.StatePath, rootPath) && - !libraries.IsVolumesPath(b.Config().Workspace.StatePath) { - paths = append(paths, b.Config().Workspace.StatePath) - } - - if !strings.HasPrefix(b.Config().Workspace.ResourcePath, rootPath) && - !libraries.IsVolumesPath(b.Config().Workspace.ResourcePath) { - paths = append(paths, b.Config().Workspace.ResourcePath) + for _, p := range []string{ + b.Config().Workspace.ArtifactPath, + b.Config().Workspace.FilePath, + b.Config().Workspace.StatePath, + b.Config().Workspace.ResourcePath, + } { + if !strings.HasPrefix(p, rootPath) && !libraries.IsVolumesPath(p) { + paths = append(paths, p) + } } var diags diag.Diagnostics diff --git a/bundle/config/validate/folder_permissions_test.go b/bundle/config/validate/folder_permissions_test.go index f98ea66a12..8e68c9fbf3 100644 --- a/bundle/config/validate/folder_permissions_test.go +++ b/bundle/config/validate/folder_permissions_test.go @@ -122,7 +122,7 @@ func TestValidateFolderPermissionsFailsOnMissingBundlePermission(t *testing.T) { diags := bundle.ApplyReadOnly(context.Background(), rb, ValidateFolderPermissions()) require.Len(t, diags, 1) - require.Equal(t, "permissions missing", diags[0].Summary) + require.Equal(t, "untracked permissions apply to target workspace path", 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) } @@ -167,12 +167,9 @@ func TestValidateFolderPermissionsFailsOnPermissionMismatch(t *testing.T) { rb := bundle.ReadOnly(b) diags := bundle.ApplyReadOnly(context.Background(), rb, ValidateFolderPermissions()) - require.Len(t, diags, 2) - require.Equal(t, "permissions missing", diags[0].Summary) + require.Len(t, diags, 1) + require.Equal(t, "untracked permissions apply to target workspace path", 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 TestValidateFolderPermissionsFailsOnNoRootFolder(t *testing.T) { diff --git a/bundle/permissions/check.go b/bundle/permissions/workspace_path_permissions.go similarity index 82% rename from bundle/permissions/check.go rename to bundle/permissions/workspace_path_permissions.go index f37c956c38..a3b4424c1c 100644 --- a/bundle/permissions/check.go +++ b/bundle/permissions/workspace_path_permissions.go @@ -18,7 +18,7 @@ func ObjectAclToResourcePermissions(path string, acl []workspace.WorkspaceObject permissions := make([]resources.Permission, 0) for _, a := range acl { // Skip the admin group because it's added to all resources by default. - if a.GroupName == "admin" { + if a.GroupName == "admins" { continue } @@ -38,22 +38,12 @@ func ObjectAclToResourcePermissions(path string, acl []workspace.WorkspaceObject func (p WorkspacePathPermissions) Compare(perms []resources.Permission) diag.Diagnostics { var diags diag.Diagnostics - // 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 missing", - Detail: fmt.Sprintf("The following permissions are configured in the bundle but are do not (yet) apply to the workspace folder at %q:\n%s", p.Path, toString(missing)), - }) - } - // Check the permissions in the workspace and see if they are all set in the bundle. - ok, missing = containsAll(p.Permissions, perms) + ok, missing := containsAll(p.Permissions, perms) if !ok { diags = diags.Append(diag.Diagnostic{ Severity: diag.Warning, - Summary: "permissions missing", + 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)), }) } diff --git a/bundle/permissions/check_test.go b/bundle/permissions/workspace_path_permissions_test.go similarity index 81% rename from bundle/permissions/check_test.go rename to bundle/permissions/workspace_path_permissions_test.go index 1c55e1f682..0bb00474c8 100644 --- a/bundle/permissions/check_test.go +++ b/bundle/permissions/workspace_path_permissions_test.go @@ -41,7 +41,7 @@ func TestWorkspacePathPermissionsCompare(t *testing.T) { }, }, { - GroupName: "admin", + GroupName: "admins", AllPermissions: []workspace.WorkspaceObjectPermission{ {PermissionLevel: "CAN_MANAGE"}, }, @@ -62,13 +62,7 @@ func TestWorkspacePathPermissionsCompare(t *testing.T) { }, }, }, - expected: diag.Diagnostics{ - { - Severity: diag.Warning, - Summary: "permissions missing", - Detail: "The following permissions are configured in the bundle but are do not (yet) apply to the workspace folder at \"path\":\n- level: CAN_MANAGE, service_principal_name: sp.com\n", - }, - }, + expected: nil, }, { perms: []resources.Permission{ @@ -91,7 +85,7 @@ func TestWorkspacePathPermissionsCompare(t *testing.T) { expected: diag.Diagnostics{ { Severity: diag.Warning, - Summary: "permissions missing", + 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", }, }, @@ -111,12 +105,7 @@ func TestWorkspacePathPermissionsCompare(t *testing.T) { expected: diag.Diagnostics{ { Severity: diag.Warning, - Summary: "permissions missing", - Detail: "The following permissions are configured in the bundle but are do not (yet) apply to the workspace folder at \"path\":\n- level: CAN_MANAGE, user_name: foo@bar.com\n", - }, - { - Severity: diag.Warning, - Summary: "permissions missing", + 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", }, }, From 48b9571f3f9dd0831bd46fddd6846239eade9409 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 24 Oct 2024 14:27:40 +0200 Subject: [PATCH 9/9] dont show warning twice --- bundle/config/validate/folder_permissions.go | 12 +++++++++--- bundle/libraries/path.go | 4 ++++ bundle/permissions/validate.go | 8 ++------ 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/bundle/config/validate/folder_permissions.go b/bundle/config/validate/folder_permissions.go index acd0283b33..a376bd776c 100644 --- a/bundle/config/validate/folder_permissions.go +++ b/bundle/config/validate/folder_permissions.go @@ -26,7 +26,7 @@ func (f *folderPermissions) Apply(ctx context.Context, b bundle.ReadOnlyBundle) rootPath := b.Config().Workspace.RootPath paths := []string{} - if !libraries.IsVolumesPath(rootPath) { + if !libraries.IsVolumesPath(rootPath) && !libraries.IsWorkspaceSharedPath(rootPath) { paths = append(paths, rootPath) } @@ -40,9 +40,15 @@ func (f *folderPermissions) Apply(ctx context.Context, b bundle.ReadOnlyBundle) b.Config().Workspace.StatePath, b.Config().Workspace.ResourcePath, } { - if !strings.HasPrefix(p, rootPath) && !libraries.IsVolumesPath(p) { - paths = append(paths, p) + if libraries.IsWorkspaceSharedPath(p) || libraries.IsVolumesPath(p) { + continue } + + if strings.HasPrefix(p, rootPath) { + continue + } + + paths = append(paths, p) } var diags diag.Diagnostics diff --git a/bundle/libraries/path.go b/bundle/libraries/path.go index 4533f6de35..3bad40face 100644 --- a/bundle/libraries/path.go +++ b/bundle/libraries/path.go @@ -41,3 +41,7 @@ func IsWorkspaceLibrary(library *compute.Library) bool { func IsVolumesPath(path string) bool { return strings.HasPrefix(path, "/Volumes/") } + +func IsWorkspaceSharedPath(path string) bool { + return strings.HasPrefix(path, "/Workspace/Shared/") +} diff --git a/bundle/permissions/validate.go b/bundle/permissions/validate.go index acd2e60628..f1a18f4309 100644 --- a/bundle/permissions/validate.go +++ b/bundle/permissions/validate.go @@ -3,9 +3,9 @@ package permissions import ( "context" "fmt" - "strings" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/libraries" "github.com/databricks/cli/libs/diag" ) @@ -21,17 +21,13 @@ func (*validateSharedRootPermissions) Name() string { } func (*validateSharedRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - if isWorkspaceSharedRoot(b.Config.Workspace.RootPath) { + if libraries.IsWorkspaceSharedPath(b.Config.Workspace.RootPath) { return isUsersGroupPermissionSet(b) } return nil } -func isWorkspaceSharedRoot(path string) bool { - return strings.HasPrefix(path, "/Workspace/Shared/") -} - // isUsersGroupPermissionSet checks that top-level permissions set for bundle contain group_name: users with CAN_MANAGE permission. func isUsersGroupPermissionSet(b *bundle.Bundle) diag.Diagnostics { var diags diag.Diagnostics