Skip to content

Commit

Permalink
Added a warning when incorrect permissions used for `/Workspace/Share…
Browse files Browse the repository at this point in the history
…d` bundle root (#1821)

## Changes
Added a warning when incorrect permissions used for `/Workspace/Shared`
bundle root

## Tests
Added unit test

---------

Co-authored-by: Pieter Noordhuis <[email protected]>
  • Loading branch information
andrewnester and pietern authored Oct 18, 2024
1 parent c5043c3 commit 0c9c902
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 6 deletions.
56 changes: 56 additions & 0 deletions bundle/permissions/validate.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package permissions

import (
"context"
"fmt"
"strings"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/libs/diag"
)

type validateSharedRootPermissions struct {
}

func ValidateSharedRootPermissions() bundle.Mutator {
return &validateSharedRootPermissions{}
}

func (*validateSharedRootPermissions) Name() string {
return "ValidateSharedRootPermissions"
}

func (*validateSharedRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
if isWorkspaceSharedRoot(b.Config.Workspace.RootPath) {
return isUsersGroupPermissionSet(b)
}

return nil
}

func isWorkspaceSharedRoot(path string) bool {
return strings.HasPrefix(path, "/Workspace/Shared/")
}

// isUsersGroupPermissionSet checks that top-level permissions set for bundle contain group_name: users with CAN_MANAGE permission.
func isUsersGroupPermissionSet(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
}
}

if !allUsers {
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/<username or principal name>.",
})
}

return diags
}
66 changes: 66 additions & 0 deletions bundle/permissions/validate_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package permissions

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/libs/diag"
"github.com/databricks/databricks-sdk-go/experimental/mocks"
"github.com/databricks/databricks-sdk-go/service/jobs"
"github.com/stretchr/testify/require"
)

func TestValidateSharedRootPermissionsForShared(t *testing.T) {
b := &bundle.Bundle{
Config: config.Root{
Workspace: config.Workspace{
RootPath: "/Workspace/Shared/foo/bar",
},
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(context.Background(), b, bundle.Seq(ValidateSharedRootPermissions()))
require.Empty(t, diags)
}

func TestValidateSharedRootPermissionsForSharedError(t *testing.T) {
b := &bundle.Bundle{
Config: config.Root{
Workspace: config.Workspace{
RootPath: "/Workspace/Shared/foo/bar",
},
Permissions: []resources.Permission{
{Level: CAN_MANAGE, UserName: "[email protected]"},
},
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(context.Background(), b, bundle.Seq(ValidateSharedRootPermissions()))
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)
}
8 changes: 4 additions & 4 deletions bundle/permissions/workspace_root.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ func ApplyWorkspaceRootPermissions() bundle.Mutator {
return &workspaceRootPermissions{}
}

func (*workspaceRootPermissions) Name() string {
return "ApplyWorkspaceRootPermissions"
}

// Apply implements bundle.Mutator.
func (*workspaceRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
err := giveAccessForWorkspaceRoot(ctx, b)
Expand All @@ -26,10 +30,6 @@ func (*workspaceRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) di
return nil
}

func (*workspaceRootPermissions) Name() string {
return "ApplyWorkspaceRootPermissions"
}

func giveAccessForWorkspaceRoot(ctx context.Context, b *bundle.Bundle) error {
permissions := make([]workspace.WorkspaceObjectAccessControlRequest, 0)

Expand Down
4 changes: 2 additions & 2 deletions bundle/permissions/workspace_root_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,6 @@ func TestApplyWorkspaceRootPermissions(t *testing.T) {
WorkspaceObjectType: "directories",
}).Return(nil, nil)

diags := bundle.Apply(context.Background(), b, ApplyWorkspaceRootPermissions())
require.NoError(t, diags.Error())
diags := bundle.Apply(context.Background(), b, bundle.Seq(ValidateSharedRootPermissions(), ApplyWorkspaceRootPermissions()))
require.Empty(t, diags)
}
3 changes: 3 additions & 0 deletions bundle/phases/initialize.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,11 @@ func Initialize() bundle.Mutator {

mutator.TranslatePaths(),
trampoline.WrapperWarning(),

permissions.ValidateSharedRootPermissions(),
permissions.ApplyBundlePermissions(),
permissions.FilterCurrentUser(),

metadata.AnnotateJobs(),
metadata.AnnotatePipelines(),
terraform.Initialize(),
Expand Down

0 comments on commit 0c9c902

Please sign in to comment.