diff --git a/bundle/permissions/workspace_root.go b/bundle/permissions/workspace_root.go index 78b9bfd704..3647a17d75 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 46347d2a9a..987f3d591f 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, ValidateWorkspaceSharedPermissions(), 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") +}