Skip to content

bundle: warn during deploy when workspace folder permissions exceed the bundle's #5439

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
shreyas-goenka wants to merge 1 commit into
base: ticklish-munching-bear
Choose a base branch
from
+99 −26
Draft

bundle: warn during deploy when workspace folder permissions exceed the bundle's #5439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 40 additions & 20 deletions bundle/permissions/workspace_root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -25,66 +26,85 @@ 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,
PermissionLevel: level,
})
}

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) {
Expand Down
65 changes: 59 additions & 6 deletions bundle/permissions/workspace_root_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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")
}
Loading