diff --git a/bundle/permissions/validate.go b/bundle/permissions/validate.go index dee7326cfa1..b0768d237b2 100644 --- a/bundle/permissions/validate.go +++ b/bundle/permissions/validate.go @@ -9,43 +9,58 @@ import ( "github.com/databricks/cli/libs/diag" ) -type validateSharedRootPermissions struct{} +type validateWorkspacePermissions struct{} -func ValidateSharedRootPermissions() bundle.Mutator { - return &validateSharedRootPermissions{} +// ValidateWorkspacePermissions 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 ValidateWorkspacePermissions() bundle.Mutator { + return &validateWorkspacePermissions{} } -func (*validateSharedRootPermissions) Name() string { - return "ValidateSharedRootPermissions" +func (*validateWorkspacePermissions) Name() string { + return "ValidateWorkspacePermissions" } -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 (*validateWorkspacePermissions) Apply(ctx context.Context, 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 - } + rootPath := b.Config.Workspace.RootPath + statePath := b.Config.Workspace.StatePath + rootIsShared := libraries.IsWorkspaceSharedPath(rootPath) + usersCanManage := hasUsersGroupManagePermission(b) + + // 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", 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/.", + }) } - if !allUsers { + // state_path is in /Workspace/Shared without users CAN_MANAGE. Skip when + // root_path is also shared, since the warning above already covers the entire + // bundle tree including the state directory. + if libraries.IsWorkspaceSharedPath(statePath) && !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 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 } + +// hasUsersGroupManagePermission returns true if the top-level permissions include +// group_name: users with CAN_MANAGE level. +func hasUsersGroupManagePermission(b *bundle.Bundle) bool { + for _, p := range b.Config.Permissions { + if p.GroupName == "users" && p.Level == CAN_MANAGE { + return true + } + } + return false +} diff --git a/bundle/permissions/validate_test.go b/bundle/permissions/validate_test.go index afaab38f3a1..738fe7dd638 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, ValidateWorkspacePermissions()) +} + +func TestValidateWorkspacePermissions_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 TestValidateWorkspacePermissions_RootShared_WarnWithoutUsersManage(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ @@ -46,20 +43,78 @@ 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 TestValidateWorkspacePermissions_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 TestValidateWorkspacePermissions_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 TestValidateWorkspacePermissions_BothShared_OnlyOneWarning(t *testing.T) { + // root_path is also in Shared — the root warning covers the whole tree, so the + // state warning must not fire. + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Workspace/Shared/root", + StatePath: "/Workspace/Shared/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 TestValidateWorkspacePermissions_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) } diff --git a/bundle/permissions/workspace_root_test.go b/bundle/permissions/workspace_root_test.go index a3b5f5ac2d9..657bc98fbb1 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, ValidateWorkspacePermissions(), ApplyWorkspaceRootPermissions()) require.Empty(t, diags) } diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index a40506ebb18..8363d300292 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.ValidateWorkspacePermissions(), // Annotate resources with "deployment" metadata. //