From df7f9b43959e9617ceb26fe44983e7a45fbd4a55 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 10 Oct 2024 13:52:13 +0200 Subject: [PATCH 1/8] Added a warning when incorrect permissions used for /Workspace/Shared bundle root --- bundle/permissions/workspace_root.go | 36 +++++++++++++ bundle/permissions/workspace_root_test.go | 66 ++++++++++++++++++++++- 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/bundle/permissions/workspace_root.go b/bundle/permissions/workspace_root.go index a59a039f6f..d2a428f18a 100644 --- a/bundle/permissions/workspace_root.go +++ b/bundle/permissions/workspace_root.go @@ -3,6 +3,7 @@ package permissions import ( "context" "fmt" + "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" @@ -18,6 +19,12 @@ func ApplyWorkspaceRootPermissions() bundle.Mutator { // Apply implements bundle.Mutator. func (*workspaceRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + + diags := checkWorkspaceRootPermissions(b) + if len(diags) > 0 { + return diags + } + err := giveAccessForWorkspaceRoot(ctx, b) if err != nil { return diag.FromErr(err) @@ -77,3 +84,32 @@ func getWorkspaceObjectPermissionLevel(bundlePermission string) (workspace.Works return "", fmt.Errorf("unsupported bundle permission level %s", bundlePermission) } } + +// checkWorkspaceRootPermissions checks that if permissions are set for the workspace root, and workspace root starts with /Workspace/Shared, then permissions should be set for group: users +func checkWorkspaceRootPermissions(b *bundle.Bundle) diag.Diagnostics { + var diags diag.Diagnostics + if len(b.Config.Permissions) == 0 { + return nil + } + + if !strings.HasPrefix(b.Config.Workspace.RootPath, "/Workspace/Shared") { + return nil + } + + allUsers := false + for _, p := range b.Config.Permissions { + if p.GroupName == "users" && p.Level == CAN_MANAGE { + allUsers = true + } + } + + if !allUsers { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: "workspace_root_permissions", + Detail: "bundle is configured to /Workspace/Shared, which will give read/write access to all users. If all users should have access, add CAN_MANAGE for 'group_name: users' permission to your bundle configuration. If the deployment should be restricted, move it to a restricted folder such as /Users/", + }) + } + + return diags +} diff --git a/bundle/permissions/workspace_root_test.go b/bundle/permissions/workspace_root_test.go index 5e23a1da81..e7bf333fa3 100644 --- a/bundle/permissions/workspace_root_test.go +++ b/bundle/permissions/workspace_root_test.go @@ -7,6 +7,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/libs/diag" "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/databricks/databricks-sdk-go/service/ml" @@ -70,5 +71,68 @@ func TestApplyWorkspaceRootPermissions(t *testing.T) { }).Return(nil, nil) diags := bundle.Apply(context.Background(), b, ApplyWorkspaceRootPermissions()) - require.NoError(t, diags.Error()) + require.Empty(t, diags) +} + +func TestApplyWorkspaceRootPermissionsForShared(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Workspace/Shared/foo/bar", + }, + Permissions: []resources.Permission{ + {Level: CAN_MANAGE, GroupName: "users"}, + }, + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job_1": {JobSettings: &jobs.JobSettings{Name: "job_1"}}, + "job_2": {JobSettings: &jobs.JobSettings{Name: "job_2"}}, + }, + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + b.SetWorkpaceClient(m.WorkspaceClient) + workspaceApi := m.GetMockWorkspaceAPI() + workspaceApi.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Shared/foo/bar").Return(&workspace.ObjectInfo{ + ObjectId: 1234, + }, nil) + workspaceApi.EXPECT().UpdatePermissions(mock.Anything, workspace.WorkspaceObjectPermissionsRequest{ + AccessControlList: []workspace.WorkspaceObjectAccessControlRequest{ + {GroupName: "users", PermissionLevel: "CAN_MANAGE"}, + }, + WorkspaceObjectId: "1234", + WorkspaceObjectType: "directories", + }).Return(nil, nil) + + diags := bundle.Apply(context.Background(), b, ApplyWorkspaceRootPermissions()) + require.Empty(t, diags) +} + +func TestApplyWorkspaceRootPermissionsForSharedError(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Workspace/Shared/foo/bar", + }, + Permissions: []resources.Permission{ + {Level: CAN_MANAGE, UserName: "foo@bar.com"}, + }, + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job_1": {JobSettings: &jobs.JobSettings{Name: "job_1"}}, + "job_2": {JobSettings: &jobs.JobSettings{Name: "job_2"}}, + }, + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + b.SetWorkpaceClient(m.WorkspaceClient) + + diags := bundle.Apply(context.Background(), b, ApplyWorkspaceRootPermissions()) + require.Len(t, diags, 1) + require.Equal(t, "workspace_root_permissions", diags[0].Summary) + require.Equal(t, diag.Warning, diags[0].Severity) } From 11afdfbea3f97c2063a3a69423329af7a65d5d80 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 10 Oct 2024 15:18:40 +0200 Subject: [PATCH 2/8] Update bundle/permissions/workspace_root.go Co-authored-by: Pieter Noordhuis --- bundle/permissions/workspace_root.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/permissions/workspace_root.go b/bundle/permissions/workspace_root.go index d2a428f18a..4b84a0bbc3 100644 --- a/bundle/permissions/workspace_root.go +++ b/bundle/permissions/workspace_root.go @@ -92,7 +92,7 @@ func checkWorkspaceRootPermissions(b *bundle.Bundle) diag.Diagnostics { return nil } - if !strings.HasPrefix(b.Config.Workspace.RootPath, "/Workspace/Shared") { + if !strings.HasPrefix(b.Config.Workspace.RootPath, "/Workspace/Shared/") { return nil } From 8ccec0a86f617b27fd69474d924be1dada2bbed6 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 10 Oct 2024 15:21:50 +0200 Subject: [PATCH 3/8] fixes --- bundle/permissions/workspace_root.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/bundle/permissions/workspace_root.go b/bundle/permissions/workspace_root.go index 4b84a0bbc3..10e85c9b54 100644 --- a/bundle/permissions/workspace_root.go +++ b/bundle/permissions/workspace_root.go @@ -88,9 +88,6 @@ func getWorkspaceObjectPermissionLevel(bundlePermission string) (workspace.Works // checkWorkspaceRootPermissions checks that if permissions are set for the workspace root, and workspace root starts with /Workspace/Shared, then permissions should be set for group: users func checkWorkspaceRootPermissions(b *bundle.Bundle) diag.Diagnostics { var diags diag.Diagnostics - if len(b.Config.Permissions) == 0 { - return nil - } if !strings.HasPrefix(b.Config.Workspace.RootPath, "/Workspace/Shared/") { return nil @@ -106,7 +103,7 @@ func checkWorkspaceRootPermissions(b *bundle.Bundle) diag.Diagnostics { if !allUsers { diags = diags.Append(diag.Diagnostic{ Severity: diag.Warning, - Summary: "workspace_root_permissions", + Summary: fmt.Sprintf("the bundle root path %s is writable by all workspace users", b.Config.Workspace.RootPath), Detail: "bundle is configured to /Workspace/Shared, which will give read/write access to all users. If all users should have access, add CAN_MANAGE for 'group_name: users' permission to your bundle configuration. If the deployment should be restricted, move it to a restricted folder such as /Users/", }) } From ad2790daf63e61d6e5f89e8a4bce77847c3b6465 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 10 Oct 2024 15:26:05 +0200 Subject: [PATCH 4/8] fix tests --- bundle/permissions/workspace_root_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/permissions/workspace_root_test.go b/bundle/permissions/workspace_root_test.go index e7bf333fa3..cf1164dc31 100644 --- a/bundle/permissions/workspace_root_test.go +++ b/bundle/permissions/workspace_root_test.go @@ -133,6 +133,6 @@ func TestApplyWorkspaceRootPermissionsForSharedError(t *testing.T) { diags := bundle.Apply(context.Background(), b, ApplyWorkspaceRootPermissions()) require.Len(t, diags, 1) - require.Equal(t, "workspace_root_permissions", diags[0].Summary) + require.Equal(t, "the bundle root path /Workspace/Shared/foo/bar is writable by all workspace users", diags[0].Summary) require.Equal(t, diag.Warning, diags[0].Severity) } From b91816653adb474bf2d2c60c57a5e1c2658b651c Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Mon, 14 Oct 2024 11:05:16 +0200 Subject: [PATCH 5/8] fixes --- bundle/permissions/workspace_root.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/bundle/permissions/workspace_root.go b/bundle/permissions/workspace_root.go index 10e85c9b54..49cac6dd36 100644 --- a/bundle/permissions/workspace_root.go +++ b/bundle/permissions/workspace_root.go @@ -20,9 +20,13 @@ func ApplyWorkspaceRootPermissions() bundle.Mutator { // Apply implements bundle.Mutator. func (*workspaceRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - diags := checkWorkspaceRootPermissions(b) - if len(diags) > 0 { - return diags + if isWorkspaceSharedRoot(b.Config.Workspace.RootPath) { + diags := checkWorkspaceRootPermissions(b) + // If there are permissions warnings, return them and do not apply permissions + // because they are not set correctly for /Workspace/Shared root anyway + if len(diags) > 0 { + return diags + } } err := giveAccessForWorkspaceRoot(ctx, b) @@ -85,14 +89,14 @@ func getWorkspaceObjectPermissionLevel(bundlePermission string) (workspace.Works } } +func isWorkspaceSharedRoot(path string) bool { + return strings.HasPrefix(path, "/Workspace/Shared/") +} + // checkWorkspaceRootPermissions checks that if permissions are set for the workspace root, and workspace root starts with /Workspace/Shared, then permissions should be set for group: users func checkWorkspaceRootPermissions(b *bundle.Bundle) diag.Diagnostics { var diags diag.Diagnostics - if !strings.HasPrefix(b.Config.Workspace.RootPath, "/Workspace/Shared/") { - return nil - } - allUsers := false for _, p := range b.Config.Permissions { if p.GroupName == "users" && p.Level == CAN_MANAGE { From c9252589d9cf54e6b35f0cb0194cf334357b0c62 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Mon, 14 Oct 2024 11:05:55 +0200 Subject: [PATCH 6/8] Update bundle/permissions/workspace_root.go Co-authored-by: Pieter Noordhuis --- bundle/permissions/workspace_root.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/permissions/workspace_root.go b/bundle/permissions/workspace_root.go index 49cac6dd36..cccdc72fef 100644 --- a/bundle/permissions/workspace_root.go +++ b/bundle/permissions/workspace_root.go @@ -108,7 +108,7 @@ func checkWorkspaceRootPermissions(b *bundle.Bundle) diag.Diagnostics { diags = diags.Append(diag.Diagnostic{ Severity: diag.Warning, Summary: fmt.Sprintf("the bundle root path %s is writable by all workspace users", b.Config.Workspace.RootPath), - Detail: "bundle is configured to /Workspace/Shared, which will give read/write access to all users. If all users should have access, add CAN_MANAGE for 'group_name: users' permission to your bundle configuration. If the deployment should be restricted, move it to a restricted folder such as /Users/", + Detail: "The bundle is configured to use /Workspace/Shared, which will give read/write access to all users. If this is intentional, add CAN_MANAGE for 'group_name: users' permission to your bundle configuration. If the deployment should be restricted, move it to a restricted folder such as /Workspace/Users/.", }) } From 00efa74514f2402e0c32b5835a47ac42c136f2ae Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 16 Oct 2024 11:14:01 +0200 Subject: [PATCH 7/8] fixes --- bundle/permissions/workspace_root.go | 38 ++++++++++++++--------- bundle/permissions/workspace_root_test.go | 18 +++++++++-- bundle/phases/initialize.go | 3 ++ 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/bundle/permissions/workspace_root.go b/bundle/permissions/workspace_root.go index cccdc72fef..1108c8f3e7 100644 --- a/bundle/permissions/workspace_root.go +++ b/bundle/permissions/workspace_root.go @@ -13,22 +13,27 @@ import ( type workspaceRootPermissions struct { } +type validateSharedRootPermissions struct { +} + func ApplyWorkspaceRootPermissions() bundle.Mutator { return &workspaceRootPermissions{} } -// Apply implements bundle.Mutator. -func (*workspaceRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { +func (*workspaceRootPermissions) Name() string { + return "ApplyWorkspaceRootPermissions" +} - if isWorkspaceSharedRoot(b.Config.Workspace.RootPath) { - diags := checkWorkspaceRootPermissions(b) - // If there are permissions warnings, return them and do not apply permissions - // because they are not set correctly for /Workspace/Shared root anyway - if len(diags) > 0 { - return diags - } - } +func ValidateSharedRootPermissions() bundle.Mutator { + return &validateSharedRootPermissions{} +} +func (*validateSharedRootPermissions) Name() string { + return "ValidateSharedRootPermissions" +} + +// Apply implements bundle.Mutator. +func (*workspaceRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { err := giveAccessForWorkspaceRoot(ctx, b) if err != nil { return diag.FromErr(err) @@ -37,8 +42,12 @@ func (*workspaceRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) di return nil } -func (*workspaceRootPermissions) Name() string { - return "ApplyWorkspaceRootPermissions" +func (*validateSharedRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + if isWorkspaceSharedRoot(b.Config.Workspace.RootPath) { + return isUsersGroupPermissionSet(b) + } + + return nil } func giveAccessForWorkspaceRoot(ctx context.Context, b *bundle.Bundle) error { @@ -93,14 +102,15 @@ func isWorkspaceSharedRoot(path string) bool { return strings.HasPrefix(path, "/Workspace/Shared/") } -// checkWorkspaceRootPermissions checks that if permissions are set for the workspace root, and workspace root starts with /Workspace/Shared, then permissions should be set for group: users -func checkWorkspaceRootPermissions(b *bundle.Bundle) diag.Diagnostics { +// 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 allUsers := false for _, p := range b.Config.Permissions { if p.GroupName == "users" && p.Level == CAN_MANAGE { allUsers = true + break } } diff --git a/bundle/permissions/workspace_root_test.go b/bundle/permissions/workspace_root_test.go index cf1164dc31..905896c869 100644 --- a/bundle/permissions/workspace_root_test.go +++ b/bundle/permissions/workspace_root_test.go @@ -70,7 +70,7 @@ func TestApplyWorkspaceRootPermissions(t *testing.T) { WorkspaceObjectType: "directories", }).Return(nil, nil) - diags := bundle.Apply(context.Background(), b, ApplyWorkspaceRootPermissions()) + diags := bundle.Apply(context.Background(), b, bundle.Seq(ValidateSharedRootPermissions(), ApplyWorkspaceRootPermissions())) require.Empty(t, diags) } @@ -106,7 +106,7 @@ func TestApplyWorkspaceRootPermissionsForShared(t *testing.T) { WorkspaceObjectType: "directories", }).Return(nil, nil) - diags := bundle.Apply(context.Background(), b, ApplyWorkspaceRootPermissions()) + diags := bundle.Apply(context.Background(), b, bundle.Seq(ValidateSharedRootPermissions(), ApplyWorkspaceRootPermissions())) require.Empty(t, diags) } @@ -130,8 +130,20 @@ func TestApplyWorkspaceRootPermissionsForSharedError(t *testing.T) { m := mocks.NewMockWorkspaceClient(t) b.SetWorkpaceClient(m.WorkspaceClient) + workspaceApi := m.GetMockWorkspaceAPI() + workspaceApi.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Shared/foo/bar").Return(&workspace.ObjectInfo{ + ObjectId: 1234, + }, nil) + + workspaceApi.EXPECT().UpdatePermissions(mock.Anything, workspace.WorkspaceObjectPermissionsRequest{ + AccessControlList: []workspace.WorkspaceObjectAccessControlRequest{ + {UserName: "foo@bar.com", PermissionLevel: "CAN_MANAGE"}, + }, + WorkspaceObjectId: "1234", + WorkspaceObjectType: "directories", + }).Return(nil, nil) - diags := bundle.Apply(context.Background(), b, ApplyWorkspaceRootPermissions()) + diags := bundle.Apply(context.Background(), b, bundle.Seq(ValidateSharedRootPermissions(), ApplyWorkspaceRootPermissions())) require.Len(t, diags, 1) require.Equal(t, "the bundle root path /Workspace/Shared/foo/bar is writable by all workspace users", diags[0].Summary) require.Equal(t, diag.Warning, diags[0].Severity) diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index a41819c769..e1c1f26191 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -74,8 +74,11 @@ func Initialize() bundle.Mutator { mutator.TranslatePaths(), trampoline.WrapperWarning(), + + permissions.ValidateSharedRootPermissions(), permissions.ApplyBundlePermissions(), permissions.FilterCurrentUser(), + metadata.AnnotateJobs(), metadata.AnnotatePipelines(), terraform.Initialize(), From ccd4d9748f10ab078684f4ab982abb9812ee390c Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Fri, 18 Oct 2024 14:25:36 +0200 Subject: [PATCH 8/8] refactoring --- bundle/permissions/validate.go | 56 +++++++++++++++++ bundle/permissions/validate_test.go | 66 ++++++++++++++++++++ bundle/permissions/workspace_root.go | 47 -------------- bundle/permissions/workspace_root_test.go | 76 ----------------------- 4 files changed, 122 insertions(+), 123 deletions(-) create mode 100644 bundle/permissions/validate.go create mode 100644 bundle/permissions/validate_test.go diff --git a/bundle/permissions/validate.go b/bundle/permissions/validate.go new file mode 100644 index 0000000000..acd2e60628 --- /dev/null +++ b/bundle/permissions/validate.go @@ -0,0 +1,56 @@ +package permissions + +import ( + "context" + "fmt" + "strings" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" +) + +type validateSharedRootPermissions struct { +} + +func ValidateSharedRootPermissions() bundle.Mutator { + return &validateSharedRootPermissions{} +} + +func (*validateSharedRootPermissions) Name() string { + return "ValidateSharedRootPermissions" +} + +func (*validateSharedRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + if isWorkspaceSharedRoot(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 + + allUsers := false + for _, p := range b.Config.Permissions { + if p.GroupName == "users" && p.Level == CAN_MANAGE { + allUsers = true + break + } + } + + if !allUsers { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("the bundle root path %s is writable by all workspace users", b.Config.Workspace.RootPath), + Detail: "The bundle is configured to use /Workspace/Shared, which will give read/write access to all users. If this is intentional, add CAN_MANAGE for 'group_name: users' permission to your bundle configuration. If the deployment should be restricted, move it to a restricted folder such as /Workspace/Users/.", + }) + } + + return diags +} diff --git a/bundle/permissions/validate_test.go b/bundle/permissions/validate_test.go new file mode 100644 index 0000000000..ff132b4e11 --- /dev/null +++ b/bundle/permissions/validate_test.go @@ -0,0 +1,66 @@ +package permissions + +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/libs/diag" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/stretchr/testify/require" +) + +func TestValidateSharedRootPermissionsForShared(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Workspace/Shared/foo/bar", + }, + Permissions: []resources.Permission{ + {Level: CAN_MANAGE, GroupName: "users"}, + }, + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job_1": {JobSettings: &jobs.JobSettings{Name: "job_1"}}, + "job_2": {JobSettings: &jobs.JobSettings{Name: "job_2"}}, + }, + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + b.SetWorkpaceClient(m.WorkspaceClient) + + diags := bundle.Apply(context.Background(), b, bundle.Seq(ValidateSharedRootPermissions())) + require.Empty(t, diags) +} + +func TestValidateSharedRootPermissionsForSharedError(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Workspace/Shared/foo/bar", + }, + Permissions: []resources.Permission{ + {Level: CAN_MANAGE, UserName: "foo@bar.com"}, + }, + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job_1": {JobSettings: &jobs.JobSettings{Name: "job_1"}}, + "job_2": {JobSettings: &jobs.JobSettings{Name: "job_2"}}, + }, + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + b.SetWorkpaceClient(m.WorkspaceClient) + + diags := bundle.Apply(context.Background(), b, bundle.Seq(ValidateSharedRootPermissions())) + require.Len(t, diags, 1) + require.Equal(t, "the bundle root path /Workspace/Shared/foo/bar is writable by all workspace users", diags[0].Summary) + require.Equal(t, diag.Warning, diags[0].Severity) +} diff --git a/bundle/permissions/workspace_root.go b/bundle/permissions/workspace_root.go index 1108c8f3e7..e7867521e0 100644 --- a/bundle/permissions/workspace_root.go +++ b/bundle/permissions/workspace_root.go @@ -3,7 +3,6 @@ package permissions import ( "context" "fmt" - "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" @@ -13,9 +12,6 @@ import ( type workspaceRootPermissions struct { } -type validateSharedRootPermissions struct { -} - func ApplyWorkspaceRootPermissions() bundle.Mutator { return &workspaceRootPermissions{} } @@ -24,14 +20,6 @@ func (*workspaceRootPermissions) Name() string { return "ApplyWorkspaceRootPermissions" } -func ValidateSharedRootPermissions() bundle.Mutator { - return &validateSharedRootPermissions{} -} - -func (*validateSharedRootPermissions) Name() string { - return "ValidateSharedRootPermissions" -} - // Apply implements bundle.Mutator. func (*workspaceRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { err := giveAccessForWorkspaceRoot(ctx, b) @@ -42,14 +30,6 @@ func (*workspaceRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) di return nil } -func (*validateSharedRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - if isWorkspaceSharedRoot(b.Config.Workspace.RootPath) { - return isUsersGroupPermissionSet(b) - } - - return nil -} - func giveAccessForWorkspaceRoot(ctx context.Context, b *bundle.Bundle) error { permissions := make([]workspace.WorkspaceObjectAccessControlRequest, 0) @@ -97,30 +77,3 @@ func getWorkspaceObjectPermissionLevel(bundlePermission string) (workspace.Works return "", fmt.Errorf("unsupported bundle permission level %s", bundlePermission) } } - -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 - - allUsers := false - for _, p := range b.Config.Permissions { - if p.GroupName == "users" && p.Level == CAN_MANAGE { - allUsers = true - break - } - } - - if !allUsers { - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Warning, - Summary: fmt.Sprintf("the bundle root path %s is writable by all workspace users", b.Config.Workspace.RootPath), - Detail: "The bundle is configured to use /Workspace/Shared, which will give read/write access to all users. If this is intentional, add CAN_MANAGE for 'group_name: users' permission to your bundle configuration. If the deployment should be restricted, move it to a restricted folder such as /Workspace/Users/.", - }) - } - - return diags -} diff --git a/bundle/permissions/workspace_root_test.go b/bundle/permissions/workspace_root_test.go index 905896c869..6b37b2c411 100644 --- a/bundle/permissions/workspace_root_test.go +++ b/bundle/permissions/workspace_root_test.go @@ -7,7 +7,6 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" - "github.com/databricks/cli/libs/diag" "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/databricks/databricks-sdk-go/service/ml" @@ -73,78 +72,3 @@ func TestApplyWorkspaceRootPermissions(t *testing.T) { diags := bundle.Apply(context.Background(), b, bundle.Seq(ValidateSharedRootPermissions(), ApplyWorkspaceRootPermissions())) require.Empty(t, diags) } - -func TestApplyWorkspaceRootPermissionsForShared(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - RootPath: "/Workspace/Shared/foo/bar", - }, - Permissions: []resources.Permission{ - {Level: CAN_MANAGE, GroupName: "users"}, - }, - Resources: config.Resources{ - Jobs: map[string]*resources.Job{ - "job_1": {JobSettings: &jobs.JobSettings{Name: "job_1"}}, - "job_2": {JobSettings: &jobs.JobSettings{Name: "job_2"}}, - }, - }, - }, - } - - m := mocks.NewMockWorkspaceClient(t) - b.SetWorkpaceClient(m.WorkspaceClient) - workspaceApi := m.GetMockWorkspaceAPI() - workspaceApi.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Shared/foo/bar").Return(&workspace.ObjectInfo{ - ObjectId: 1234, - }, nil) - workspaceApi.EXPECT().UpdatePermissions(mock.Anything, workspace.WorkspaceObjectPermissionsRequest{ - AccessControlList: []workspace.WorkspaceObjectAccessControlRequest{ - {GroupName: "users", PermissionLevel: "CAN_MANAGE"}, - }, - WorkspaceObjectId: "1234", - WorkspaceObjectType: "directories", - }).Return(nil, nil) - - diags := bundle.Apply(context.Background(), b, bundle.Seq(ValidateSharedRootPermissions(), ApplyWorkspaceRootPermissions())) - require.Empty(t, diags) -} - -func TestApplyWorkspaceRootPermissionsForSharedError(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - RootPath: "/Workspace/Shared/foo/bar", - }, - Permissions: []resources.Permission{ - {Level: CAN_MANAGE, UserName: "foo@bar.com"}, - }, - Resources: config.Resources{ - Jobs: map[string]*resources.Job{ - "job_1": {JobSettings: &jobs.JobSettings{Name: "job_1"}}, - "job_2": {JobSettings: &jobs.JobSettings{Name: "job_2"}}, - }, - }, - }, - } - - m := mocks.NewMockWorkspaceClient(t) - b.SetWorkpaceClient(m.WorkspaceClient) - workspaceApi := m.GetMockWorkspaceAPI() - workspaceApi.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Shared/foo/bar").Return(&workspace.ObjectInfo{ - ObjectId: 1234, - }, nil) - - workspaceApi.EXPECT().UpdatePermissions(mock.Anything, workspace.WorkspaceObjectPermissionsRequest{ - AccessControlList: []workspace.WorkspaceObjectAccessControlRequest{ - {UserName: "foo@bar.com", PermissionLevel: "CAN_MANAGE"}, - }, - WorkspaceObjectId: "1234", - WorkspaceObjectType: "directories", - }).Return(nil, nil) - - diags := bundle.Apply(context.Background(), b, bundle.Seq(ValidateSharedRootPermissions(), ApplyWorkspaceRootPermissions())) - require.Len(t, diags, 1) - require.Equal(t, "the bundle root path /Workspace/Shared/foo/bar is writable by all workspace users", diags[0].Summary) - require.Equal(t, diag.Warning, diags[0].Severity) -}