Skip to content

Remove never-wired folder writability check #6638

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

Open
denik wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove never-wired folder writability check #6638

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
100 changes: 0 additions & 100 deletions bundle/permissions/folder_acl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,14 @@ package permissions

import (
"context"
"errors"
"fmt"
"path"
"strconv"

"github.com/databricks/cli/bundle/config/resources"
"github.com/databricks/databricks-sdk-go/apierr"
"github.com/databricks/databricks-sdk-go/service/iam"
"github.com/databricks/databricks-sdk-go/service/workspace"
)

// adminsGroup is the workspace admins group. Its members can write to any
// workspace folder regardless of the folder ACL, so writability checks must not
// conclude "cannot write" for an admin.
const adminsGroup = "admins"

// FolderACL is the resolved access-control state of a workspace folder: the
// permissions that actually apply to it, resolved by walking up to the closest
// existing ancestor when the folder itself does not exist yet.
Expand Down Expand Up @@ -58,98 +50,6 @@ func ResolveFolderACL(ctx context.Context, w workspace.WorkspaceInterface, folde
}, nil
}

// Writability is the outcome of a folder writability check. It is three-valued
// because the check cannot always reach a definite answer: reading a folder ACL
// itself requires manage access, so the common "cannot write" case surfaces as a
// permission error on the read rather than as a readable ACL that omits the user.
type Writability int

const (
// WritabilityUnknown means writability could not be determined (e.g. the
// folder does not exist and has no existing ancestor, or the ACL read failed
// for a reason other than permission denied). Callers should not warn.
WritabilityUnknown Writability = iota
// WritabilityWritable means the user has write access (directly, via a group,
// or as an admin), or it cannot be ruled out.
WritabilityWritable
// WritabilityNotWritable means the user definitely lacks write access: the
// permissions API denied the ACL read (which requires manage access), or the
// ACL was readable but grants the user no write-capable level.
WritabilityNotWritable
)

// CheckWritable reports whether user can write to folderPath, resolving the ACL
// (walking up to the closest existing ancestor for a not-yet-created folder) and
// interpreting a permission-denied error on the read as a definite "not writable".
//
// Reading a folder ACL requires CAN_MANAGE (managing permissions is exclusive to
// CAN_MANAGE per the workspace ACL model; the permissions GET returns 403
// "does not have Manage permissions" otherwise), so a 403 means the user cannot
// manage, and therefore cannot write to, the folder.
// See https://docs.databricks.com/aws/en/security/auth/access-control/
func CheckWritable(ctx context.Context, w workspace.WorkspaceInterface, folderPath string, user *iam.User) Writability {
acl, err := ResolveFolderACL(ctx, w, folderPath)
if err != nil {
if errors.Is(err, apierr.ErrPermissionDenied) {
return WritabilityNotWritable
}
return WritabilityUnknown
}

if acl.CanWrite(user) {
return WritabilityWritable
}
return WritabilityNotWritable
}

// CanWrite reports whether user has write access (CAN_EDIT or higher) to the
// folder, either directly or through one of their groups.
//
// It only inspects a readable ACL; prefer CheckWritable, which also interprets a
// permission-denied error on the ACL read. It is deliberately conservative:
// it assumes write access when the user is a workspace admin (admins bypass the
// folder ACL) or when their group membership is not known here, because
// concluding "cannot write" in those cases would be a false alarm.
func (f *FolderACL) CanWrite(user *iam.User) bool {
groups := userGroupNames(user)
if groups[adminsGroup] {
return true
}

for _, p := range f.Permissions.Permissions {
if !isWriteLevel(string(p.Level)) {
continue
}
switch {
case p.UserName != "" && p.UserName == user.UserName:
return true
case p.ServicePrincipalName != "" && p.ServicePrincipalName == user.UserName:
return true
case p.GroupName != "" && groups[p.GroupName]:
return true
}
}

return false
}

// isWriteLevel reports whether an ACL level grants write access to a folder,
// which is what deploying a bundle requires. CAN_EDIT is the lowest such level.
func isWriteLevel(level string) bool {
return resources.GetLevelScore(level) >= resources.GetLevelScore(resources.CAN_EDIT)
}

// userGroupNames returns the set of group display names the user belongs to.
func userGroupNames(user *iam.User) map[string]bool {
groups := make(map[string]bool, len(user.Groups))
for _, g := range user.Groups {
if g.Display != "" {
groups[g.Display] = true
}
}
return groups
}

// closestExistingObject returns the object at folderPath, or the closest existing
// ancestor when folderPath does not exist. A not-yet-created folder inherits the
// permissions of its closest existing ancestor.
Expand Down
205 changes: 0 additions & 205 deletions bundle/permissions/folder_acl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,216 +5,12 @@ import (

"github.com/databricks/databricks-sdk-go/apierr"
"github.com/databricks/databricks-sdk-go/experimental/mocks"
"github.com/databricks/databricks-sdk-go/service/iam"
"github.com/databricks/databricks-sdk-go/service/workspace"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

func user(name string, groups ...string) *iam.User {
u := &iam.User{UserName: name}
for _, g := range groups {
u.Groups = append(u.Groups, iam.ComplexValue{Display: g})
}
return u
}

func aclFor(t *testing.T, acl []workspace.WorkspaceObjectAccessControlResponse) *FolderACL {
t.Helper()
return &FolderACL{
RequestedPath: "/Workspace/f",
ResolvedPath: "/Workspace/f",
Permissions: ObjectAclToResourcePermissions("/Workspace/f", acl),
}
}

func TestFolderACLCanWrite(t *testing.T) {
editUser := []workspace.WorkspaceObjectAccessControlResponse{
{
UserName: "me@example.com",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_EDIT"},
},
},
}
manageGroup := []workspace.WorkspaceObjectAccessControlResponse{
{
GroupName: "devs",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_MANAGE"},
},
},
}
readOnlyUser := []workspace.WorkspaceObjectAccessControlResponse{
{
UserName: "me@example.com",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_READ"},
},
},
}
sp := []workspace.WorkspaceObjectAccessControlResponse{
{
ServicePrincipalName: "sp-uuid",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_MANAGE"},
},
},
}

tests := []struct {
name string
acl []workspace.WorkspaceObjectAccessControlResponse
user *iam.User
want bool
}{
{"direct write", editUser, user("me@example.com"), true},
{"write via group", manageGroup, user("me@example.com", "devs"), true},
{"read-only is not writable", readOnlyUser, user("me@example.com"), false},
{"no matching entry", editUser, user("other@example.com"), false},
{"service principal write", sp, user("sp-uuid"), true},

// Conservative cases: assume writable when it cannot be ruled out.
{"admin bypasses folder ACL", readOnlyUser, user("me@example.com", "admins"), true},
{"write via group not in user's known groups", manageGroup, user("me@example.com"), false},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
assert.Equal(t, tc.want, aclFor(t, tc.acl).CanWrite(tc.user))
})
}
}

// These ACLs mirror the shape of real workspace folder permissions (verified
// against live traffic): a home-style folder granting the owner CAN_MANAGE
// directly plus inherited admin/service-principal entries, and a shared folder
// granting CAN_MANAGE to a group the user belongs to rather than to the user
// directly.

// homeDirACL: owner has direct CAN_MANAGE (writable).
var homeDirACL = []workspace.WorkspaceObjectAccessControlResponse{
{
UserName: "owner@example.com",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_MANAGE"},
},
},
{
ServicePrincipalName: "00000000-0000-0000-0000-000000000000",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_MANAGE", Inherited: true},
},
},
{
GroupName: "admins",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_MANAGE", Inherited: true},
},
},
}

// sharedDirACL: CAN_MANAGE granted to the "users" group (write via group, not a
// direct grant).
var sharedDirACL = []workspace.WorkspaceObjectAccessControlResponse{
{
GroupName: "users",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_MANAGE"},
},
},
{
GroupName: "admins",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_MANAGE", Inherited: true},
},
},
}

func TestFolderACLCanWriteRealisticACLs(t *testing.T) {
me := user("owner@example.com", "engineering", "users")

assert.True(t, aclFor(t, homeDirACL).CanWrite(me), "own home dir: direct CAN_MANAGE")
assert.True(t, aclFor(t, sharedDirACL).CanWrite(me), "shared dir: CAN_MANAGE via users group")

// A different user without any of these grants cannot write.
other := user("someone.else@example.com")
assert.False(t, aclFor(t, homeDirACL).CanWrite(other))
assert.False(t, aclFor(t, sharedDirACL).CanWrite(other))
}

func TestCheckWritable(t *testing.T) {
const folder = "/Workspace/f"
me := user("me@example.com")

writableACL := &workspace.WorkspaceObjectPermissions{
AccessControlList: []workspace.WorkspaceObjectAccessControlResponse{
{
UserName: "me@example.com",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_MANAGE"},
},
},
},
}
readOnlyACL := &workspace.WorkspaceObjectPermissions{
AccessControlList: []workspace.WorkspaceObjectAccessControlResponse{
{
UserName: "me@example.com",
AllPermissions: []workspace.WorkspaceObjectPermission{
{PermissionLevel: "CAN_READ"},
},
},
},
}

tests := []struct {
name string
getPerms func() (*workspace.WorkspaceObjectPermissions, error)
want Writability
}{
{
"writable",
func() (*workspace.WorkspaceObjectPermissions, error) { return writableACL, nil },
WritabilityWritable,
},
{
// Reading the ACL is denied because it requires manage access, so the
// user definitely cannot write. This is the common real-world case.
"permission denied on ACL read means not writable",
func() (*workspace.WorkspaceObjectPermissions, error) {
return nil, &apierr.APIError{StatusCode: 403, ErrorCode: "PERMISSION_DENIED"}
},
WritabilityNotWritable,
},
{
"readable ACL without a write grant means not writable",
func() (*workspace.WorkspaceObjectPermissions, error) { return readOnlyACL, nil },
WritabilityNotWritable,
},
{
"other errors are unknown",
func() (*workspace.WorkspaceObjectPermissions, error) {
return nil, &apierr.APIError{StatusCode: 500}
},
WritabilityUnknown,
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
m := mocks.NewMockWorkspaceClient(t)
w := m.GetMockWorkspaceAPI()
w.EXPECT().GetStatusByPath(mock.Anything, folder).
Return(&workspace.ObjectInfo{ObjectId: 7, Path: folder}, nil)
w.EXPECT().GetPermissions(mock.Anything, mock.Anything).Return(tc.getPerms())

assert.Equal(t, tc.want, CheckWritable(t.Context(), w, folder, me))
})
}
}

func TestResolveFolderACLWalksUpToClosestExistingAncestor(t *testing.T) {
m := mocks.NewMockWorkspaceClient(t)
w := m.GetMockWorkspaceAPI()
Expand Down Expand Up @@ -245,5 +41,4 @@ func TestResolveFolderACLWalksUpToClosestExistingAncestor(t *testing.T) {
assert.Equal(t, "/Workspace/root/files", acl.RequestedPath)
assert.Equal(t, "/Workspace/root", acl.ResolvedPath)
assert.Equal(t, "/Workspace/root/files", acl.Permissions.Path)
assert.True(t, acl.CanWrite(user("me@example.com")))
}
Loading