From b703045f55d00fd9010f8e363db76f9a2f7d6bea Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 4 Jun 2026 13:49:03 +0200 Subject: [PATCH] bundle: warn when a workspace path is in /Workspace/Shared without users CAN_MANAGE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renames ValidateSharedRootPermissions to ValidateWorkspaceSharedPermissions and extends it to also cover workspace.state_path. It warns when root_path or state_path is in /Workspace/Shared — granting read/write to all workspace users — but the top-level permissions section does not declare that access via group_name: users CAN_MANAGE. The state_path warning is suppressed only when state_path is nested under root_path, since the root warning already covers it. When state_path is a separate shared folder, both warnings fire. Co-authored-by: Shreyas Goenka --- .../jobs/shared-root-path/output.txt | 4 +- .../databricks.yml | 16 ++++ .../out.test.toml | 3 + .../workspace_shared_permissions/output.txt | 30 ++++++ .../workspace_shared_permissions/script | 2 + bundle/permissions/validate.go | 77 +++++++++++---- bundle/permissions/validate_test.go | 93 +++++++++++++------ bundle/permissions/workspace_root_test.go | 2 +- bundle/phases/initialize.go | 8 +- 9 files changed, 181 insertions(+), 54 deletions(-) create mode 100644 acceptance/bundle/validate/workspace_shared_permissions/databricks.yml create mode 100644 acceptance/bundle/validate/workspace_shared_permissions/out.test.toml create mode 100644 acceptance/bundle/validate/workspace_shared_permissions/output.txt create mode 100644 acceptance/bundle/validate/workspace_shared_permissions/script diff --git a/acceptance/bundle/resources/jobs/shared-root-path/output.txt b/acceptance/bundle/resources/jobs/shared-root-path/output.txt index b485c092416..83347c73793 100644 --- a/acceptance/bundle/resources/jobs/shared-root-path/output.txt +++ b/acceptance/bundle/resources/jobs/shared-root-path/output.txt @@ -2,7 +2,7 @@ >>> [CLI] bundle deploy Warning: the bundle root path /Workspace/Shared/[USERNAME]/.bundle/[UNIQUE_NAME] is writable by all workspace users -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/. +The bundle root path is in /Workspace/Shared, giving read/write access to all workspace users that is not reflected in the permissions section. If this is intentional, add CAN_MANAGE for 'group_name: users' to your bundle permissions. Otherwise, move the bundle to a restricted path such as /Workspace/Users/. Uploading bundle files to /Workspace/Shared/[USERNAME]/.bundle/[UNIQUE_NAME]/files... Deploying resources... @@ -12,7 +12,7 @@ Deployment complete! >>> [CLI] bundle destroy --auto-approve Warning: the bundle root path /Workspace/Shared/[USERNAME]/.bundle/[UNIQUE_NAME] is writable by all workspace users -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/. +The bundle root path is in /Workspace/Shared, giving read/write access to all workspace users that is not reflected in the permissions section. If this is intentional, add CAN_MANAGE for 'group_name: users' to your bundle permissions. Otherwise, move the bundle to a restricted path such as /Workspace/Users/. The following resources will be deleted: delete resources.jobs.foo diff --git a/acceptance/bundle/validate/workspace_shared_permissions/databricks.yml b/acceptance/bundle/validate/workspace_shared_permissions/databricks.yml new file mode 100644 index 00000000000..ec4871a7644 --- /dev/null +++ b/acceptance/bundle/validate/workspace_shared_permissions/databricks.yml @@ -0,0 +1,16 @@ +bundle: + name: test-bundle + +targets: + # state_path defaults to a directory under root_path, so only the root path + # warning fires. + one_warning: + workspace: + root_path: /Workspace/Shared/test-bundle/one + + # state_path is a separate folder under /Workspace/Shared (not nested under + # root_path), so both the root path and state path warnings fire. + two_warnings: + workspace: + root_path: /Workspace/Shared/test-bundle/two + state_path: /Workspace/Shared/test-bundle-state diff --git a/acceptance/bundle/validate/workspace_shared_permissions/out.test.toml b/acceptance/bundle/validate/workspace_shared_permissions/out.test.toml new file mode 100644 index 00000000000..f784a183258 --- /dev/null +++ b/acceptance/bundle/validate/workspace_shared_permissions/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/validate/workspace_shared_permissions/output.txt b/acceptance/bundle/validate/workspace_shared_permissions/output.txt new file mode 100644 index 00000000000..84f87056018 --- /dev/null +++ b/acceptance/bundle/validate/workspace_shared_permissions/output.txt @@ -0,0 +1,30 @@ + +>>> [CLI] bundle validate -t one_warning +Warning: the bundle root path /Workspace/Shared/test-bundle/one is writable by all workspace users + +The bundle root path is in /Workspace/Shared, giving read/write access to all workspace users that is not reflected in the permissions section. If this is intentional, add CAN_MANAGE for 'group_name: users' to your bundle permissions. Otherwise, move the bundle to a restricted path such as /Workspace/Users/. + +Name: test-bundle +Target: one_warning +Workspace: + User: [USERNAME] + Path: /Workspace/Shared/test-bundle/one + +Found 1 warning + +>>> [CLI] bundle validate -t two_warnings +Warning: the bundle root path /Workspace/Shared/test-bundle/two is writable by all workspace users + +The bundle root path is in /Workspace/Shared, giving read/write access to all workspace users that is not reflected in the permissions section. If this is intentional, add CAN_MANAGE for 'group_name: users' to your bundle permissions. Otherwise, move the bundle to a restricted path such as /Workspace/Users/. + +Warning: the bundle state path /Workspace/Shared/test-bundle-state is writable by all workspace users + +The bundle state path is in /Workspace/Shared, giving read/write access to all workspace users that is not reflected in the permissions section. If this is intentional, add CAN_MANAGE for 'group_name: users' to your bundle permissions. Otherwise, move the state path to a restricted location such as /Workspace/Users/. + +Name: test-bundle +Target: two_warnings +Workspace: + User: [USERNAME] + Path: /Workspace/Shared/test-bundle/two + +Found 2 warnings diff --git a/acceptance/bundle/validate/workspace_shared_permissions/script b/acceptance/bundle/validate/workspace_shared_permissions/script new file mode 100644 index 00000000000..446ff5efd29 --- /dev/null +++ b/acceptance/bundle/validate/workspace_shared_permissions/script @@ -0,0 +1,2 @@ +trace $CLI bundle validate -t one_warning +trace $CLI bundle validate -t two_warnings diff --git a/bundle/permissions/validate.go b/bundle/permissions/validate.go index dee7326cfa1..82f59a0e383 100644 --- a/bundle/permissions/validate.go +++ b/bundle/permissions/validate.go @@ -3,49 +3,86 @@ package permissions import ( "context" "fmt" + "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/libraries" "github.com/databricks/cli/libs/diag" ) -type validateSharedRootPermissions struct{} +type validateWorkspaceSharedPermissions struct{} -func ValidateSharedRootPermissions() bundle.Mutator { - return &validateSharedRootPermissions{} +// ValidateWorkspaceSharedPermissions warns when a workspace path is configured +// under /Workspace/Shared — which grants read/write access to all workspace users — +// without the top-level permissions section declaring that broad access via +// group_name: users with CAN_MANAGE. +func ValidateWorkspaceSharedPermissions() bundle.Mutator { + return &validateWorkspaceSharedPermissions{} } -func (*validateSharedRootPermissions) Name() string { - return "ValidateSharedRootPermissions" +func (*validateWorkspaceSharedPermissions) Name() string { + return "ValidateWorkspaceSharedPermissions" } -func (*validateSharedRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - if libraries.IsWorkspaceSharedPath(b.Config.Workspace.RootPath) { - return isUsersGroupPermissionSet(b) - } - - return nil -} - -// isUsersGroupPermissionSet checks that top-level permissions set for bundle contain group_name: users with CAN_MANAGE permission. -func isUsersGroupPermissionSet(b *bundle.Bundle) diag.Diagnostics { +func (*validateWorkspaceSharedPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { var diags diag.Diagnostics - allUsers := false + rootPath := b.Config.Workspace.RootPath + statePath := b.Config.Workspace.StatePath + rootIsShared := libraries.IsWorkspaceSharedPath(rootPath) + + // Whether the top-level permissions grant group_name: users CAN_MANAGE, i.e. + // the broad /Workspace/Shared access is intentional and declared. + usersCanManage := false for _, p := range b.Config.Permissions { if p.GroupName == "users" && p.Level == CAN_MANAGE { - allUsers = true + usersCanManage = true break } } - if !allUsers { + // root_path is in /Workspace/Shared without users CAN_MANAGE. + if rootIsShared && !usersCanManage { 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/.", + Summary: fmt.Sprintf("the bundle root path %s is writable by all workspace users", rootPath), + Detail: "The bundle root path is in /Workspace/Shared, giving read/write access to all workspace users that is not reflected in the permissions section. If this is intentional, add CAN_MANAGE for 'group_name: users' to your bundle permissions. Otherwise, move the bundle to a restricted path such as /Workspace/Users/.", + }) + } + + // state_path is in /Workspace/Shared without users CAN_MANAGE. Skip only when + // state_path is nested under root_path, since the root warning above already + // covers it. When state_path is a separate folder, warn about it on its own. + if libraries.IsWorkspaceSharedPath(statePath) && !statePathUnderRootPath(rootPath, statePath) && !usersCanManage { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("the bundle state path %s is writable by all workspace users", statePath), + Detail: "The bundle state path is in /Workspace/Shared, giving read/write access to all workspace users that is not reflected in the permissions section. If this is intentional, add CAN_MANAGE for 'group_name: users' to your bundle permissions. Otherwise, move the state path to a restricted location such as /Workspace/Users/.", }) } return diags } + +// statePathUnderRootPath returns true when statePath is nested under rootPath, in +// which case permissions applied to root_path also cover the state directory. +// +// By default state_path lives under root_path (it defaults to "${root_path}/state"), +// so we treat it as nested unless both paths are set and root_path is genuinely not a +// prefix of state_path. That keeps us from emitting a separate state warning for the +// common case. +// +// Both paths are /Workspace-normalized by PrependWorkspacePrefix before this mutator +// runs, so the prefix comparison here is reliable. +func statePathUnderRootPath(rootPath, statePath string) bool { + if rootPath == "" || statePath == "" { + return true + } + if statePath == rootPath { + return true + } + if !strings.HasSuffix(rootPath, "/") { + rootPath += "/" + } + return strings.HasPrefix(statePath, rootPath) +} diff --git a/bundle/permissions/validate_test.go b/bundle/permissions/validate_test.go index afaab38f3a1..759d0884558 100644 --- a/bundle/permissions/validate_test.go +++ b/bundle/permissions/validate_test.go @@ -8,11 +8,18 @@ import ( "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/assert" "github.com/stretchr/testify/require" ) -func TestValidateSharedRootPermissionsForShared(t *testing.T) { +func applyValidate(t *testing.T, b *bundle.Bundle) diag.Diagnostics { + t.Helper() + m := mocks.NewMockWorkspaceClient(t) + b.SetWorkpaceClient(m.WorkspaceClient) + return bundle.Apply(t.Context(), b, ValidateWorkspaceSharedPermissions()) +} + +func TestValidateWorkspaceSharedPermissions_RootShared_NoWarningWithUsersManage(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ @@ -21,45 +28,77 @@ func TestValidateSharedRootPermissionsForShared(t *testing.T) { 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(t.Context(), b, ValidateSharedRootPermissions()) + diags := applyValidate(t, b) require.Empty(t, diags) } -func TestValidateSharedRootPermissionsForSharedError(t *testing.T) { +func TestValidateWorkspaceSharedPermissions_StateShared_WarnWithoutUsersManage(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ - RootPath: "/Workspace/Shared/foo/bar", + RootPath: "/Workspace/Users/user@example.test", + StatePath: "/Workspace/Shared/state", }, Permissions: []resources.Permission{ - {Level: CAN_MANAGE, UserName: "foo@bar.test"}, + {Level: CAN_MANAGE, UserName: "user@example.test"}, + }, + }, + } + diags := applyValidate(t, b) + require.Len(t, diags, 1) + assert.Equal(t, diag.Warning, diags[0].Severity) + assert.Contains(t, diags[0].Summary, "state path") + assert.Contains(t, diags[0].Summary, "/Workspace/Shared/state") +} + +func TestValidateWorkspaceSharedPermissions_StateShared_NoWarningWithUsersManage(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Workspace/Users/user@example.test", + StatePath: "/Workspace/Shared/state", }, - Resources: config.Resources{ - Jobs: map[string]*resources.Job{ - "job_1": {JobSettings: jobs.JobSettings{Name: "job_1"}}, - "job_2": {JobSettings: jobs.JobSettings{Name: "job_2"}}, - }, + Permissions: []resources.Permission{ + {Level: CAN_MANAGE, GroupName: "users"}, }, }, } + diags := applyValidate(t, b) + require.Empty(t, diags) +} - m := mocks.NewMockWorkspaceClient(t) - b.SetWorkpaceClient(m.WorkspaceClient) +func TestValidateWorkspaceSharedPermissions_NoSharedPaths_NoWarning(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Workspace/Users/user@example.test/bundle", + StatePath: "/Workspace/Users/user@example.test/other-state", + }, + }, + } + diags := applyValidate(t, b) + require.Empty(t, diags) +} - diags := bundle.Apply(t.Context(), b, 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) +func TestStatePathUnderRootPath(t *testing.T) { + cases := []struct { + name string + rootPath string + statePath string + want bool + }{ + {name: "default nested state", rootPath: "/Workspace/Users/me/bundle", statePath: "/Workspace/Users/me/bundle/state", want: true}, + {name: "equal paths", rootPath: "/Workspace/Users/me/bundle", statePath: "/Workspace/Users/me/bundle", want: true}, + {name: "separate folder", rootPath: "/Workspace/Users/me/bundle", statePath: "/Workspace/Shared/state", want: false}, + {name: "sibling prefix is not nested", rootPath: "/Workspace/Users/me/bundle", statePath: "/Workspace/Users/me/bundle-2/state", want: false}, + {name: "empty state defaults to nested", rootPath: "/Workspace/Users/me/bundle", statePath: "", want: true}, + {name: "empty root defaults to nested", rootPath: "", statePath: "/Workspace/Shared/state", want: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, statePathUnderRootPath(tc.rootPath, tc.statePath)) + }) + } } diff --git a/bundle/permissions/workspace_root_test.go b/bundle/permissions/workspace_root_test.go index a3b5f5ac2d9..46347d2a9ab 100644 --- a/bundle/permissions/workspace_root_test.go +++ b/bundle/permissions/workspace_root_test.go @@ -72,7 +72,7 @@ func TestApplyWorkspaceRootPermissions(t *testing.T) { WorkspaceObjectType: "directories", }).Return(nil, nil) - diags := bundle.ApplySeq(t.Context(), b, ValidateSharedRootPermissions(), ApplyWorkspaceRootPermissions()) + diags := bundle.ApplySeq(t.Context(), b, ValidateWorkspaceSharedPermissions(), ApplyWorkspaceRootPermissions()) require.Empty(t, diags) } diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index a40506ebb18..297909314e3 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -191,10 +191,10 @@ func Initialize(ctx context.Context, b *bundle.Bundle) { apps.Validate(), resourcemutator.ValidateTargetMode(), - // Reads (typed): b.Config.Workspace.RootPath (checks if path is in shared workspace) - // Reads (typed): b.Config.Permissions (checks if users group has CAN_MANAGE permission) - // Validates that when using a shared workspace path, appropriate permissions are configured - permissions.ValidateSharedRootPermissions(), + // Reads (typed): b.Config.Workspace.{RootPath,StatePath} and b.Config.Permissions. + // Warns when a workspace path is in /Workspace/Shared without the permissions + // section granting all workspace users CAN_MANAGE. + permissions.ValidateWorkspaceSharedPermissions(), // Annotate resources with "deployment" metadata. //