From 7c783aacd53dab3ff86cee3e3a010cbc094b4fc6 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 4 Jun 2026 13:50:47 +0200 Subject: [PATCH] bundle: warn during deploy when workspace folder permissions exceed the bundle's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ValidateFolderPermissions already compares the live workspace ACL against the declared permissions, but it only runs during `bundle validate`. This brings the same check to `bundle deploy` without adding any API latency: ApplyWorkspaceRoot- Permissions already calls SetPermissions on each workspace path prefix (root_path and, when separate, state_path), and the response carries the resulting ACL. Reusing that response, we compare against the declared permissions. Because the Set replaces the folder's direct ACL with the declared set, any principal still showing higher access is inherited from a parent folder — the broader access that actually persists after deploy, which is the scope mismatch worth surfacing. No extra GetPermissions round trip is made. The check is skipped for /Workspace/ Shared paths, consistent with the existing behavior. Co-authored-by: Shreyas Goenka --- bundle/permissions/workspace_root.go | 60 ++++++++++++++------- bundle/permissions/workspace_root_test.go | 65 ++++++++++++++++++++--- 2 files changed, 99 insertions(+), 26 deletions(-) diff --git a/bundle/permissions/workspace_root.go b/bundle/permissions/workspace_root.go index 78b9bfd704a..3647a17d753 100644 --- a/bundle/permissions/workspace_root.go +++ b/bundle/permissions/workspace_root.go @@ -6,6 +6,7 @@ import ( "strconv" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/bundle/libraries" "github.com/databricks/cli/bundle/paths" "github.com/databricks/cli/libs/diag" @@ -25,24 +26,19 @@ func (*workspaceRootPermissions) Name() string { // 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) - } - - return nil + return giveAccessForWorkspaceRoot(ctx, b) } -func giveAccessForWorkspaceRoot(ctx context.Context, b *bundle.Bundle) error { - var permissions []workspace.WorkspaceObjectAccessControlRequest +func giveAccessForWorkspaceRoot(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + var acl []workspace.WorkspaceObjectAccessControlRequest for _, p := range b.Config.Permissions { level, err := GetWorkspaceObjectPermissionLevel(string(p.Level)) if err != nil { - return err + return diag.FromErr(err) } - permissions = append(permissions, workspace.WorkspaceObjectAccessControlRequest{ + acl = append(acl, workspace.WorkspaceObjectAccessControlRequest{ GroupName: p.GroupName, UserName: p.UserName, ServicePrincipalName: p.ServicePrincipalName, @@ -50,41 +46,65 @@ func giveAccessForWorkspaceRoot(ctx context.Context, b *bundle.Bundle) error { }) } - if len(permissions) == 0 { + if len(acl) == 0 { return nil } w := b.WorkspaceClient(ctx).Workspace bundlePaths := paths.CollectUniqueWorkspacePathPrefixes(b.Config.Workspace) + // Each goroutine writes its own slot, so the results are merged after Wait + // rather than logged concurrently. + results := make([]diag.Diagnostics, len(bundlePaths)) g, ctx := errgroup.WithContext(ctx) - for _, p := range bundlePaths { + for i, p := range bundlePaths { g.Go(func() error { - return setPermissions(ctx, w, p, permissions) + diags, err := setPermissions(ctx, w, p, acl, b.Config.Permissions) + results[i] = diags + return err }) } - return g.Wait() + if err := g.Wait(); err != nil { + return diag.FromErr(err) + } + + var diags diag.Diagnostics + for _, r := range results { + diags = diags.Extend(r) + } + return diags } -func setPermissions(ctx context.Context, w workspace.WorkspaceInterface, path string, permissions []workspace.WorkspaceObjectAccessControlRequest) error { +// setPermissions applies the bundle's declared permissions to the workspace folder +// and, by reusing the SetPermissions response, warns when the folder's effective +// permissions are broader than the bundle declares — without an additional API call. +// +// The Set replaces the folder's direct ACL with the declared permissions, so any +// principal that still shows higher access in the response is inherited from a +// parent folder. That inherited access persists after the deploy, which is exactly +// the scope mismatch worth surfacing. +func setPermissions(ctx context.Context, w workspace.WorkspaceInterface, path string, acl []workspace.WorkspaceObjectAccessControlRequest, declared []resources.Permission) (diag.Diagnostics, error) { // If the folder is shared, then we don't need to set permissions since it's always set for all users and it's checked in mutators before. if libraries.IsWorkspaceSharedPath(path) { - return nil + return nil, nil } obj, err := w.GetStatusByPath(ctx, path) //nolint:staticcheck // Deprecated in SDK v0.127.0. Migration to WorkspaceHierarchyService tracked separately. if err != nil { - return err + return nil, err } - _, err = w.SetPermissions(ctx, workspace.WorkspaceObjectPermissionsRequest{ + resp, err := w.SetPermissions(ctx, workspace.WorkspaceObjectPermissionsRequest{ WorkspaceObjectId: strconv.FormatInt(obj.ObjectId, 10), WorkspaceObjectType: "directories", - AccessControlList: permissions, + AccessControlList: acl, }) + if err != nil { + return nil, err + } - return err + return ObjectAclToResourcePermissions(path, resp.AccessControlList).Compare(declared), nil } func GetWorkspaceObjectPermissionLevel(bundlePermission string) (workspace.WorkspaceObjectPermissionLevel, error) { diff --git a/bundle/permissions/workspace_root_test.go b/bundle/permissions/workspace_root_test.go index 657bc98fbb1..7b9b13f90d7 100644 --- a/bundle/permissions/workspace_root_test.go +++ b/bundle/permissions/workspace_root_test.go @@ -6,6 +6,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,7 +71,7 @@ func TestApplyWorkspaceRootPermissions(t *testing.T) { }, WorkspaceObjectId: "1234", WorkspaceObjectType: "directories", - }).Return(nil, nil) + }).Return(&workspace.WorkspaceObjectPermissions{}, nil) diags := bundle.ApplySeq(t.Context(), b, ValidateWorkspacePermissions(), ApplyWorkspaceRootPermissions()) require.Empty(t, diags) @@ -143,7 +144,7 @@ func TestApplyWorkspaceRootPermissionsForAllPaths(t *testing.T) { }, WorkspaceObjectId: "1", WorkspaceObjectType: "directories", - }).Return(nil, nil) + }).Return(&workspace.WorkspaceObjectPermissions{}, nil) workspaceApi.EXPECT().SetPermissions(mock.Anything, workspace.WorkspaceObjectPermissionsRequest{ AccessControlList: []workspace.WorkspaceObjectAccessControlRequest{ @@ -153,7 +154,7 @@ func TestApplyWorkspaceRootPermissionsForAllPaths(t *testing.T) { }, WorkspaceObjectId: "2", WorkspaceObjectType: "directories", - }).Return(nil, nil) + }).Return(&workspace.WorkspaceObjectPermissions{}, nil) workspaceApi.EXPECT().SetPermissions(mock.Anything, workspace.WorkspaceObjectPermissionsRequest{ AccessControlList: []workspace.WorkspaceObjectAccessControlRequest{ @@ -163,7 +164,7 @@ func TestApplyWorkspaceRootPermissionsForAllPaths(t *testing.T) { }, WorkspaceObjectId: "3", WorkspaceObjectType: "directories", - }).Return(nil, nil) + }).Return(&workspace.WorkspaceObjectPermissions{}, nil) workspaceApi.EXPECT().SetPermissions(mock.Anything, workspace.WorkspaceObjectPermissionsRequest{ AccessControlList: []workspace.WorkspaceObjectAccessControlRequest{ @@ -173,7 +174,7 @@ func TestApplyWorkspaceRootPermissionsForAllPaths(t *testing.T) { }, WorkspaceObjectId: "4", WorkspaceObjectType: "directories", - }).Return(nil, nil) + }).Return(&workspace.WorkspaceObjectPermissions{}, nil) workspaceApi.EXPECT().SetPermissions(mock.Anything, workspace.WorkspaceObjectPermissionsRequest{ AccessControlList: []workspace.WorkspaceObjectAccessControlRequest{ @@ -183,8 +184,60 @@ func TestApplyWorkspaceRootPermissionsForAllPaths(t *testing.T) { }, WorkspaceObjectId: "5", WorkspaceObjectType: "directories", - }).Return(nil, nil) + }).Return(&workspace.WorkspaceObjectPermissions{}, nil) diags := bundle.Apply(t.Context(), b, ApplyWorkspaceRootPermissions()) require.NoError(t, diags.Error()) } + +// TestApplyWorkspaceRootPermissionsWarnsOnBroaderPermissions verifies that the +// SetPermissions response is reused to detect permissions broader than the bundle +// declares. Here the folder inherits CAN_MANAGE for a group that is not in the +// bundle's permissions, so a warning is expected without any extra API call. +func TestApplyWorkspaceRootPermissionsWarnsOnBroaderPermissions(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/Users/foo@bar.test", + ArtifactPath: "/Users/foo@bar.test/artifacts", + FilePath: "/Users/foo@bar.test/files", + StatePath: "/Users/foo@bar.test/state", + ResourcePath: "/Users/foo@bar.test/resources", + }, + Permissions: []resources.Permission{ + {Level: CAN_MANAGE, UserName: "foo@bar.test"}, + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + b.SetWorkpaceClient(m.WorkspaceClient) + workspaceApi := m.GetMockWorkspaceAPI() + workspaceApi.EXPECT().GetStatusByPath(mock.Anything, "/Users/foo@bar.test").Return(&workspace.ObjectInfo{ + ObjectId: 1234, + }, nil) + workspaceApi.EXPECT().SetPermissions(mock.Anything, workspace.WorkspaceObjectPermissionsRequest{ + AccessControlList: []workspace.WorkspaceObjectAccessControlRequest{ + {UserName: "foo@bar.test", PermissionLevel: "CAN_MANAGE"}, + }, + WorkspaceObjectId: "1234", + WorkspaceObjectType: "directories", + }).Return(&workspace.WorkspaceObjectPermissions{ + AccessControlList: []workspace.WorkspaceObjectAccessControlResponse{ + { + UserName: "foo@bar.test", + AllPermissions: []workspace.WorkspaceObjectPermission{{PermissionLevel: "CAN_MANAGE"}}, + }, + { + GroupName: "data-engineers", + AllPermissions: []workspace.WorkspaceObjectPermission{{PermissionLevel: "CAN_MANAGE", Inherited: true}}, + }, + }, + }, nil) + + diags := bundle.Apply(t.Context(), b, ApplyWorkspaceRootPermissions()) + require.Len(t, diags, 1) + require.Equal(t, diag.Warning, diags[0].Severity) + require.Equal(t, "workspace folder has permissions not configured in bundle", diags[0].Summary) + require.Contains(t, diags[0].Detail, "data-engineers") +}