Skip to content
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.

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

Added validator for folder permissions #1824

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
18 changes: 18 additions & 0 deletions bundle/config/resources/permission.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package resources

import "fmt"

// Permission holds the permission level setting for a single principal.
// Multiple of these can be defined on any resource.
type Permission struct {
Expand All @@ -9,3 +11,19 @@ type Permission struct {
ServicePrincipalName string `json:"service_principal_name,omitempty"`
GroupName string `json:"group_name,omitempty"`
}

func (p Permission) String() string {
if p.UserName != "" {
return fmt.Sprintf("level: %s, user_name: %s", p.Level, p.UserName)
}

if p.ServicePrincipalName != "" {
return fmt.Sprintf("level: %s, service_principal_name: %s", p.Level, p.ServicePrincipalName)
}

if p.GroupName != "" {
return fmt.Sprintf("level: %s, group_name: %s", p.Level, p.GroupName)
}

return fmt.Sprintf("level: %s", p.Level)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see comment below; if this is not used it can be removed.

135 changes: 135 additions & 0 deletions bundle/config/validate/folder_permissions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
package validate

import (
"context"
"fmt"
"path"
"strings"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/libraries"
"github.com/databricks/cli/bundle/permissions"
"github.com/databricks/cli/libs/diag"
"github.com/databricks/databricks-sdk-go/apierr"
"github.com/databricks/databricks-sdk-go/service/workspace"
"golang.org/x/sync/errgroup"
"golang.org/x/sync/singleflight"
)

type folderPermissions struct {
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
}

// Apply implements bundle.ReadOnlyMutator.
func (f *folderPermissions) Apply(ctx context.Context, b bundle.ReadOnlyBundle) diag.Diagnostics {
if len(b.Config().Permissions) == 0 {
return nil
}

rootPath := b.Config().Workspace.RootPath
paths := []string{}
if !libraries.IsVolumesPath(rootPath) {
paths = append(paths, rootPath)
}

if !strings.HasSuffix(rootPath, "/") {
rootPath += "/"
}

if !strings.HasPrefix(b.Config().Workspace.ArtifactPath, rootPath) &&
!libraries.IsVolumesPath(b.Config().Workspace.ArtifactPath) {
paths = append(paths, b.Config().Workspace.ArtifactPath)
}

if !strings.HasPrefix(b.Config().Workspace.FilePath, rootPath) &&
!libraries.IsVolumesPath(b.Config().Workspace.FilePath) {
paths = append(paths, b.Config().Workspace.FilePath)
}

if !strings.HasPrefix(b.Config().Workspace.StatePath, rootPath) &&
!libraries.IsVolumesPath(b.Config().Workspace.StatePath) {
paths = append(paths, b.Config().Workspace.StatePath)
}

if !strings.HasPrefix(b.Config().Workspace.ResourcePath, rootPath) &&
!libraries.IsVolumesPath(b.Config().Workspace.ResourcePath) {
paths = append(paths, b.Config().Workspace.ResourcePath)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the logic expanded like this, it can be a loop on the different paths.


var diags diag.Diagnostics
g, ctx := errgroup.WithContext(ctx)
results := make([]diag.Diagnostics, len(paths))
syncGroup := new(singleflight.Group)
for i, p := range paths {
g.Go(func() error {
diags, err, _ := syncGroup.Do(p, func() (any, error) {
diags := checkFolderPermission(ctx, b, p)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These paths are different by design so there won't be reuse at this level. I figured you could initialize the sync group here and pass it to checkFolderPermissions. Then inside of getClosestExistingObject, you can use syncGroup.Do() to make sure you're only doing a single call for any given path at any level.

Those calls will be done multiple times in the current approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, this will apply only to cases where the different paths are not a child of the root. This should be infrequent enough that we can skip caching altogether.

return diags, nil
})
results[i] = diags.(diag.Diagnostics)
return err
})
}

if err := g.Wait(); err != nil {
return diag.FromErr(err)
}

for _, r := range results {
diags = diags.Extend(r)
}

return diags
}

func checkFolderPermission(ctx context.Context, b bundle.ReadOnlyBundle, folderPath string) diag.Diagnostics {
w := b.WorkspaceClient().Workspace
obj, err := getClosestExistingObject(ctx, w, folderPath)
if err != nil {
return diag.FromErr(err)
}

objPermissions, err := w.GetPermissions(ctx, workspace.GetWorkspaceObjectPermissionsRequest{
WorkspaceObjectId: fmt.Sprint(obj.ObjectId),
WorkspaceObjectType: "directories",
pietern marked this conversation as resolved.
Show resolved Hide resolved
})
if err != nil {
return diag.FromErr(err)
}

p := permissions.ObjectAclToResourcePermissions(folderPath, objPermissions.AccessControlList)
return p.Compare(b.Config().Permissions)
}

func getClosestExistingObject(ctx context.Context, w workspace.WorkspaceInterface, folderPath string) (*workspace.ObjectInfo, error) {
for {
obj, err := w.GetStatusByPath(ctx, folderPath)
if err == nil {
return obj, nil
}

if !apierr.IsMissing(err) {
return nil, err
}

parent := path.Dir(folderPath)
// If the parent is the same as the current folder, then we have reached the root
if folderPath == parent {
break
}
andrewnester marked this conversation as resolved.
Show resolved Hide resolved

folderPath = parent
}

return nil, fmt.Errorf("folder %s and its parent folders do not exist", folderPath)
}

// Name implements bundle.ReadOnlyMutator.
func (f *folderPermissions) Name() string {
return "validate:folder_permissions"
}

// ValidateFolderPermissions validates that permissions for the folders in Workspace file system matches
// the permissions in the top-level permissions section of the bundle.
func ValidateFolderPermissions() bundle.ReadOnlyMutator {
return &folderPermissions{}
}
203 changes: 203 additions & 0 deletions bundle/config/validate/folder_permissions_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
package validate

import (
"context"
"testing"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config"
"github.com/databricks/cli/bundle/config/resources"
"github.com/databricks/cli/bundle/permissions"
"github.com/databricks/cli/libs/diag"
"github.com/databricks/databricks-sdk-go/apierr"
"github.com/databricks/databricks-sdk-go/experimental/mocks"
"github.com/databricks/databricks-sdk-go/service/workspace"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

func TestValidateFolderPermissions(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add suffix to capture the intent; I believe it is to test what happens if the root path doesn't exist yet.

b := &bundle.Bundle{
Config: config.Root{
Workspace: config.Workspace{
RootPath: "/Workspace/Users/[email protected]",
ArtifactPath: "/Workspace/Users/[email protected]/artifacts",
FilePath: "/Workspace/Users/[email protected]/files",
StatePath: "/Workspace/Users/[email protected]/state",
ResourcePath: "/Workspace/Users/[email protected]/resources",
},
Permissions: []resources.Permission{
{Level: permissions.CAN_MANAGE, UserName: "[email protected]"},
},
},
}
m := mocks.NewMockWorkspaceClient(t)
api := m.GetMockWorkspaceAPI()
api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Users/[email protected]").Return(nil, &apierr.APIError{
StatusCode: 404,
ErrorCode: "RESOURCE_DOES_NOT_EXIST",
})
api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Users").Return(nil, &apierr.APIError{
StatusCode: 404,
ErrorCode: "RESOURCE_DOES_NOT_EXIST",
})
api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace").Return(&workspace.ObjectInfo{
ObjectId: 1234,
}, nil)

api.EXPECT().GetPermissions(mock.Anything, workspace.GetWorkspaceObjectPermissionsRequest{
WorkspaceObjectId: "1234",
WorkspaceObjectType: "directories",
}).Return(&workspace.WorkspaceObjectPermissions{
ObjectId: "1234",
AccessControlList: []workspace.WorkspaceObjectAccessControlResponse{
{
UserName: "[email protected]",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_MANAGE"},
},
},
},
}, nil)

b.SetWorkpaceClient(m.WorkspaceClient)
rb := bundle.ReadOnly(b)

diags := bundle.ApplyReadOnly(context.Background(), rb, ValidateFolderPermissions())
require.Empty(t, diags)
}

func TestValidateFolderPermissionsDifferentCount(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer about a different count.

b := &bundle.Bundle{
Config: config.Root{
Workspace: config.Workspace{
RootPath: "/Workspace/Users/[email protected]",
ArtifactPath: "/Workspace/Users/[email protected]/artifacts",
FilePath: "/Workspace/Users/[email protected]/files",
StatePath: "/Workspace/Users/[email protected]/state",
ResourcePath: "/Workspace/Users/[email protected]/resources",
},
Permissions: []resources.Permission{
{Level: permissions.CAN_MANAGE, UserName: "[email protected]"},
},
},
}
m := mocks.NewMockWorkspaceClient(t)
api := m.GetMockWorkspaceAPI()
api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Users/[email protected]").Return(&workspace.ObjectInfo{
ObjectId: 1234,
}, nil)

api.EXPECT().GetPermissions(mock.Anything, workspace.GetWorkspaceObjectPermissionsRequest{
WorkspaceObjectId: "1234",
WorkspaceObjectType: "directories",
}).Return(&workspace.WorkspaceObjectPermissions{
ObjectId: "1234",
AccessControlList: []workspace.WorkspaceObjectAccessControlResponse{
{
UserName: "[email protected]",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_MANAGE"},
},
},
{
UserName: "[email protected]",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_MANAGE"},
},
},
},
}, nil)

b.SetWorkpaceClient(m.WorkspaceClient)
rb := bundle.ReadOnly(b)

diags := bundle.ApplyReadOnly(context.Background(), rb, ValidateFolderPermissions())
require.Len(t, diags, 1)
require.Equal(t, "permissions missing", diags[0].Summary)
require.Equal(t, diag.Warning, diags[0].Severity)
require.Equal(t, "Following permissions set for the workspace folder but not set for bundle /Workspace/Users/[email protected]:\n- level: CAN_MANAGE\n user_name: [email protected]\n", diags[0].Detail)
}

func TestValidateFolderPermissionsDifferentPermission(t *testing.T) {
b := &bundle.Bundle{
Config: config.Root{
Workspace: config.Workspace{
RootPath: "/Workspace/Users/[email protected]",
ArtifactPath: "/Workspace/Users/[email protected]/artifacts",
FilePath: "/Workspace/Users/[email protected]/files",
StatePath: "/Workspace/Users/[email protected]/state",
ResourcePath: "/Workspace/Users/[email protected]/resources",
},
Permissions: []resources.Permission{
{Level: permissions.CAN_MANAGE, UserName: "[email protected]"},
},
},
}
m := mocks.NewMockWorkspaceClient(t)
api := m.GetMockWorkspaceAPI()
api.EXPECT().GetStatusByPath(mock.Anything, "/Workspace/Users/[email protected]").Return(&workspace.ObjectInfo{
ObjectId: 1234,
}, nil)

api.EXPECT().GetPermissions(mock.Anything, workspace.GetWorkspaceObjectPermissionsRequest{
WorkspaceObjectId: "1234",
WorkspaceObjectType: "directories",
}).Return(&workspace.WorkspaceObjectPermissions{
ObjectId: "1234",
AccessControlList: []workspace.WorkspaceObjectAccessControlResponse{
{
UserName: "[email protected]",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_MANAGE"},
},
},
},
}, nil)

b.SetWorkpaceClient(m.WorkspaceClient)
rb := bundle.ReadOnly(b)

diags := bundle.ApplyReadOnly(context.Background(), rb, ValidateFolderPermissions())
require.Len(t, diags, 2)
require.Equal(t, "permissions missing", diags[0].Summary)
require.Equal(t, diag.Warning, diags[0].Severity)

require.Equal(t, "permissions missing", diags[1].Summary)
require.Equal(t, diag.Warning, diags[1].Severity)
}

func TestNoRootFolder(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing prefix: TestValidateFolderPermissions

b := &bundle.Bundle{
Config: config.Root{
Workspace: config.Workspace{
RootPath: "/NotExisting",
ArtifactPath: "/NotExisting/artifacts",
FilePath: "/NotExisting/files",
StatePath: "/NotExisting/state",
ResourcePath: "/NotExisting/resources",
},
Permissions: []resources.Permission{
{Level: permissions.CAN_MANAGE, UserName: "[email protected]"},
},
},
}
m := mocks.NewMockWorkspaceClient(t)
api := m.GetMockWorkspaceAPI()
api.EXPECT().GetStatusByPath(mock.Anything, "/NotExisting").Return(nil, &apierr.APIError{
StatusCode: 404,
ErrorCode: "RESOURCE_DOES_NOT_EXIST",
})
api.EXPECT().GetStatusByPath(mock.Anything, "/").Return(nil, &apierr.APIError{
StatusCode: 404,
ErrorCode: "RESOURCE_DOES_NOT_EXIST",
})

b.SetWorkpaceClient(m.WorkspaceClient)
rb := bundle.ReadOnly(b)

diags := bundle.ApplyReadOnly(context.Background(), rb, ValidateFolderPermissions())
require.Len(t, diags, 1)
require.Equal(t, "folder / and its parent folders do not exist", diags[0].Summary)
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
require.Equal(t, diag.Error, diags[0].Severity)
}
1 change: 1 addition & 0 deletions bundle/config/validate/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ func (v *validate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics
FilesToSync(),
ValidateSyncPatterns(),
JobTaskClusterSpec(),
ValidateFolderPermissions(),
))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,8 @@ func IsWorkspaceLibrary(library *compute.Library) bool {

return IsWorkspacePath(path)
}

// IsVolumesPath returns true if the specified path indicates that
func IsVolumesPath(path string) bool {
return strings.HasPrefix(path, "/Volumes/")
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,13 @@ func TestIsWorkspaceLibrary(t *testing.T) {
// Empty.
assert.False(t, IsWorkspaceLibrary(&compute.Library{}))
}

func TestIsVolumesPath(t *testing.T) {
// Absolute paths with particular prefixes.
assert.True(t, IsVolumesPath("/Volumes/path/to/package"))

// Relative paths.
assert.False(t, IsVolumesPath("myfile.txt"))
assert.False(t, IsVolumesPath("./myfile.txt"))
assert.False(t, IsVolumesPath("../myfile.txt"))
}
Loading
Loading