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..5f0ccc07e97 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,23 +28,13 @@ 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_RootShared_WarnWithoutUsersManage(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ @@ -46,20 +43,119 @@ func TestValidateSharedRootPermissionsForSharedError(t *testing.T) { Permissions: []resources.Permission{ {Level: CAN_MANAGE, UserName: "foo@bar.test"}, }, - Resources: config.Resources{ - Jobs: map[string]*resources.Job{ - "job_1": {JobSettings: jobs.JobSettings{Name: "job_1"}}, - "job_2": {JobSettings: jobs.JobSettings{Name: "job_2"}}, - }, + }, + } + diags := applyValidate(t, b) + require.Len(t, diags, 1) + assert.Equal(t, diag.Warning, diags[0].Severity) + assert.Contains(t, diags[0].Summary, "root path") + assert.Contains(t, diags[0].Summary, "/Workspace/Shared/foo/bar") +} + +func TestValidateWorkspaceSharedPermissions_StateShared_WarnWithoutUsersManage(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Workspace/Users/user@example.test", + StatePath: "/Workspace/Shared/state", + }, + Permissions: []resources.Permission{ + {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") +} - m := mocks.NewMockWorkspaceClient(t) - b.SetWorkpaceClient(m.WorkspaceClient) +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", + }, + Permissions: []resources.Permission{ + {Level: CAN_MANAGE, GroupName: "users"}, + }, + }, + } + diags := applyValidate(t, b) + require.Empty(t, diags) +} - diags := bundle.Apply(t.Context(), b, ValidateSharedRootPermissions()) +func TestValidateWorkspaceSharedPermissions_StateUnderSharedRoot_OnlyRootWarning(t *testing.T) { + // state_path is nested under root_path — the root warning already covers it, + // so the state warning must not fire. + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Workspace/Shared/bundle", + StatePath: "/Workspace/Shared/bundle/state", + }, + Permissions: []resources.Permission{ + {Level: CAN_MANAGE, UserName: "user@example.test"}, + }, + }, + } + diags := applyValidate(t, b) 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) + assert.Contains(t, diags[0].Summary, "root path") +} + +func TestValidateWorkspaceSharedPermissions_BothSharedSeparateFolders_TwoWarnings(t *testing.T) { + // root_path and state_path are both shared but state_path is not under root_path, + // so both warnings fire. + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Workspace/Shared/bundle", + StatePath: "/Workspace/Shared/separate-state", + }, + Permissions: []resources.Permission{ + {Level: CAN_MANAGE, UserName: "user@example.test"}, + }, + }, + } + diags := applyValidate(t, b) + require.Len(t, diags, 2) + assert.Contains(t, diags[0].Summary, "root path") + assert.Contains(t, diags[1].Summary, "state path") +} + +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) +} + +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. //