From b64d3c2fbfe9388e5a57319f5fc197884d4bc5b8 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sat, 26 Feb 2022 12:19:09 +0100 Subject: [PATCH] simplify rename permission before this patch we allow a rename in the following cases: - the user has rename permission on both source and target path - the user has delete permission on source path and create/upload on target path we now check only the rename/rename_files/rename_dirs permissions. This is what SFTPGo users expect. This is a backward incompatible change and it will not backported to the 2.2.x branch Signed-off-by: Nicola Murino --- common/connection.go | 55 +++++++++++---------------------------- common/connection_test.go | 31 +++++++++++++++++++--- common/protocol_test.go | 3 ++- dataprovider/user.go | 9 +------ go.mod | 6 +++-- go.sum | 12 ++++++--- sftpd/sftpd_test.go | 10 +++---- 7 files changed, 61 insertions(+), 65 deletions(-) diff --git a/common/connection.go b/common/connection.go index ee16d453..839b8d2e 100644 --- a/common/connection.go +++ b/common/connection.go @@ -763,12 +763,6 @@ func (c *BaseConnection) truncateFile(fs vfs.Fs, fsPath, virtualPath string, siz } func (c *BaseConnection) checkRecursiveRenameDirPermissions(fsSrc, fsDst vfs.Fs, sourcePath, targetPath string) error { - dstPerms := []string{ - dataprovider.PermCreateDirs, - dataprovider.PermUpload, - dataprovider.PermCreateSymlinks, - } - err := fsSrc.Walk(sourcePath, func(walkedPath string, info os.FileInfo, err error) error { if err != nil { return c.GetFsError(fsSrc, err) @@ -784,10 +778,6 @@ func (c *BaseConnection) checkRecursiveRenameDirPermissions(fsSrc, fsDst vfs.Fs, c.User.HasPermsRenameAll(path.Dir(virtualDstPath)) { return ErrSkipPermissionsCheck } - if c.User.HasPermsDeleteAll(path.Dir(virtualSrcPath)) && - c.User.HasPerms(dstPerms, path.Dir(virtualDstPath)) { - return ErrSkipPermissionsCheck - } } if !c.isRenamePermitted(fsSrc, fsDst, walkedPath, dstPath, virtualSrcPath, virtualDstPath, info) { c.Log(logger.LevelInfo, "rename %#v -> %#v is not allowed, virtual destination path: %#v", @@ -808,39 +798,24 @@ func (c *BaseConnection) hasRenamePerms(virtualSourcePath, virtualTargetPath str return true } if fi == nil { - // we don't know if this is a file or a directory we have to check all the required permissions - if !c.User.HasPermsDeleteAll(path.Dir(virtualSourcePath)) { - return false - } - dstPerms := []string{ - dataprovider.PermCreateDirs, - dataprovider.PermUpload, - dataprovider.PermCreateSymlinks, - } - return c.User.HasPerms(dstPerms, path.Dir(virtualTargetPath)) - } - if fi.IsDir() { - if c.User.HasPerm(dataprovider.PermRenameDirs, path.Dir(virtualSourcePath)) && - c.User.HasPerm(dataprovider.PermRenameDirs, path.Dir(virtualTargetPath)) { - return true - } - if !c.User.HasAnyPerm([]string{dataprovider.PermDeleteDirs, dataprovider.PermDelete}, path.Dir(virtualSourcePath)) { - return false - } - return c.User.HasPerm(dataprovider.PermCreateDirs, path.Dir(virtualTargetPath)) - } - // file or symlink - if c.User.HasPerm(dataprovider.PermRenameFiles, path.Dir(virtualSourcePath)) && - c.User.HasPerm(dataprovider.PermRenameFiles, path.Dir(virtualTargetPath)) { - return true - } - if !c.User.HasAnyPerm([]string{dataprovider.PermDeleteFiles, dataprovider.PermDelete}, path.Dir(virtualSourcePath)) { + // we don't know if this is a file or a directory and we don't have all the rename perms, return false return false } - if fi.Mode()&os.ModeSymlink != 0 { - return c.User.HasPerm(dataprovider.PermCreateSymlinks, path.Dir(virtualTargetPath)) + if fi.IsDir() { + perms := []string{ + dataprovider.PermRenameDirs, + dataprovider.PermRename, + } + return c.User.HasAnyPerm(perms, path.Dir(virtualSourcePath)) && + c.User.HasAnyPerm(perms, path.Dir(virtualTargetPath)) } - return c.User.HasPerm(dataprovider.PermUpload, path.Dir(virtualTargetPath)) + // file or symlink + perms := []string{ + dataprovider.PermRenameFiles, + dataprovider.PermRename, + } + return c.User.HasAnyPerm(perms, path.Dir(virtualSourcePath)) && + c.User.HasAnyPerm(perms, path.Dir(virtualTargetPath)) } func (c *BaseConnection) isRenamePermitted(fsSrc, fsDst vfs.Fs, fsSourcePath, fsTargetPath, virtualSourcePath, virtualTargetPath string, fi os.FileInfo) bool { diff --git a/common/connection_test.go b/common/connection_test.go index 826bfc97..f14a8c23 100644 --- a/common/connection_test.go +++ b/common/connection_test.go @@ -163,26 +163,49 @@ func TestRenameVirtualFolders(t *testing.T) { func TestRenamePerms(t *testing.T) { src := "source" target := "target" + sub := "/sub" + subTarget := sub + "/target" u := dataprovider.User{} u.Permissions = map[string][]string{} u.Permissions["/"] = []string{dataprovider.PermCreateDirs, dataprovider.PermUpload, dataprovider.PermCreateSymlinks, dataprovider.PermDeleteFiles} conn := NewBaseConnection("", ProtocolSFTP, "", "", u) assert.False(t, conn.hasRenamePerms(src, target, nil)) - u.Permissions["/"] = []string{dataprovider.PermCreateDirs, dataprovider.PermUpload, dataprovider.PermCreateSymlinks, - dataprovider.PermDeleteFiles, dataprovider.PermDeleteDirs} + u.Permissions["/"] = []string{dataprovider.PermRename} assert.True(t, conn.hasRenamePerms(src, target, nil)) u.Permissions["/"] = []string{dataprovider.PermCreateDirs, dataprovider.PermUpload, dataprovider.PermDeleteFiles, dataprovider.PermDeleteDirs} assert.False(t, conn.hasRenamePerms(src, target, nil)) info := vfs.NewFileInfo(src, true, 0, time.Now(), false) - u.Permissions["/"] = []string{dataprovider.PermCreateDirs, dataprovider.PermUpload, dataprovider.PermDeleteFiles} + u.Permissions["/"] = []string{dataprovider.PermRenameFiles} assert.False(t, conn.hasRenamePerms(src, target, info)) - u.Permissions["/"] = []string{dataprovider.PermCreateDirs, dataprovider.PermUpload, dataprovider.PermDeleteDirs} + u.Permissions["/"] = []string{dataprovider.PermRenameDirs} + assert.True(t, conn.hasRenamePerms(src, target, info)) + u.Permissions["/"] = []string{dataprovider.PermRename} assert.True(t, conn.hasRenamePerms(src, target, info)) u.Permissions["/"] = []string{dataprovider.PermDownload, dataprovider.PermUpload, dataprovider.PermDeleteDirs} assert.False(t, conn.hasRenamePerms(src, target, info)) + // test with different permissions between source and target + u.Permissions["/"] = []string{dataprovider.PermRename} + u.Permissions[sub] = []string{dataprovider.PermRenameFiles} + assert.False(t, conn.hasRenamePerms(src, subTarget, info)) + u.Permissions[sub] = []string{dataprovider.PermRenameDirs} + assert.True(t, conn.hasRenamePerms(src, subTarget, info)) + // test files + info = vfs.NewFileInfo(src, false, 0, time.Now(), false) + u.Permissions["/"] = []string{dataprovider.PermRenameDirs} + assert.False(t, conn.hasRenamePerms(src, target, info)) + u.Permissions["/"] = []string{dataprovider.PermRenameFiles} + assert.True(t, conn.hasRenamePerms(src, target, info)) + u.Permissions["/"] = []string{dataprovider.PermRename} + assert.True(t, conn.hasRenamePerms(src, target, info)) + // test with different permissions between source and target + u.Permissions["/"] = []string{dataprovider.PermRename} + u.Permissions[sub] = []string{dataprovider.PermRenameDirs} + assert.False(t, conn.hasRenamePerms(src, subTarget, info)) + u.Permissions[sub] = []string{dataprovider.PermRenameFiles} + assert.True(t, conn.hasRenamePerms(src, subTarget, info)) } func TestUpdateQuotaAfterRename(t *testing.T) { diff --git a/common/protocol_test.go b/common/protocol_test.go index 646d184e..6478fc67 100644 --- a/common/protocol_test.go +++ b/common/protocol_test.go @@ -1760,7 +1760,8 @@ func TestQuotaRenameToVirtualFolder(t *testing.T) { QuotaSize: 0, }) u.Permissions[vdirPath1] = []string{dataprovider.PermListItems, dataprovider.PermDownload, dataprovider.PermUpload, - dataprovider.PermOverwrite, dataprovider.PermDelete, dataprovider.PermCreateSymlinks, dataprovider.PermCreateDirs} + dataprovider.PermOverwrite, dataprovider.PermDelete, dataprovider.PermCreateSymlinks, dataprovider.PermCreateDirs, + dataprovider.PermRename} user, _, err := httpdtest.AddUser(u, http.StatusCreated) assert.NoError(t, err) conn, client, err := getSftpClient(user) diff --git a/dataprovider/user.go b/dataprovider/user.go index 9a4f5676..95ac0fd5 100644 --- a/dataprovider/user.go +++ b/dataprovider/user.go @@ -78,7 +78,6 @@ var ( errNoMatchingVirtualFolder = errors.New("no matching virtual folder found") permsRenameAny = []string{PermRename, PermRenameDirs, PermRenameFiles} permsDeleteAny = []string{PermDelete, PermDeleteDirs, PermDeleteFiles} - permsCreateAny = []string{PermUpload, PermCreateDirs} ) // RecoveryCode defines a 2FA recovery code @@ -998,13 +997,7 @@ func (u *User) CanRenameFromWeb(src, dest string) bool { if util.IsStringInSlice(sdk.WebClientWriteDisabled, u.Filters.WebClient) { return false } - if u.HasAnyPerm(permsRenameAny, src) && u.HasAnyPerm(permsRenameAny, dest) { - return true - } - if !u.HasAnyPerm(permsDeleteAny, src) { - return false - } - return u.HasAnyPerm(permsCreateAny, dest) + return u.HasAnyPerm(permsRenameAny, src) && u.HasAnyPerm(permsRenameAny, dest) } // CanDeleteFromWeb returns true if the client can delete objects from the web UI. diff --git a/go.mod b/go.mod index e86c008b..6564c346 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/storage/azblob v0.3.0 github.com/GehirnInc/crypt v0.0.0-20200316065508-bb7000b8a962 github.com/alexedwards/argon2id v0.0.0-20211130144151-3585854a6387 - github.com/aws/aws-sdk-go v1.43.6 + github.com/aws/aws-sdk-go v1.43.7 github.com/cockroachdb/cockroach-go/v2 v2.2.8 github.com/coreos/go-oidc/v3 v3.1.0 github.com/eikenb/pipeat v0.0.0-20210730190139-06b3e6902001 @@ -50,7 +50,7 @@ require ( github.com/studio-b12/gowebdav v0.0.0-20220128162035-c7b1ff8a5e62 github.com/unrolled/secure v1.10.0 github.com/wagslane/go-password-validator v0.3.0 - github.com/xhit/go-simple-mail/v2 v2.10.0 + github.com/xhit/go-simple-mail/v2 v2.11.0 github.com/yl2chen/cidranger v1.0.3-0.20210928021809-d1cb2c52f37a go.etcd.io/bbolt v1.3.6 go.uber.org/automaxprocs v1.4.0 @@ -80,6 +80,7 @@ require ( github.com/fatih/color v1.13.0 // indirect github.com/fsnotify/fsnotify v1.5.1 // indirect github.com/go-ole/go-ole v1.2.6 // indirect + github.com/go-test/deep v1.0.8 // indirect github.com/goccy/go-json v0.9.4 // indirect github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/golang/protobuf v1.5.2 // indirect @@ -121,6 +122,7 @@ require ( github.com/subosito/gotenv v1.2.0 // indirect github.com/tklauser/go-sysconf v0.3.9 // indirect github.com/tklauser/numcpus v0.4.0 // indirect + github.com/toorop/go-dkim v0.0.0-20201103131630-e1cd1a0a5208 // indirect github.com/yusufpapurcu/wmi v1.2.2 // indirect go.opencensus.io v0.23.0 // indirect golang.org/x/mod v0.5.1 // indirect diff --git a/go.sum b/go.sum index 62849027..9f1c9266 100644 --- a/go.sum +++ b/go.sum @@ -144,8 +144,8 @@ github.com/armon/go-radix v1.0.0/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgI github.com/aws/aws-sdk-go v1.15.27/go.mod h1:mFuSZ37Z9YOHbQEwBWztmVzqXrEkub65tZoCYDt7FT0= github.com/aws/aws-sdk-go v1.37.0/go.mod h1:hcU610XS61/+aQV88ixoOzUoG7v3b31pl2zKMmprdro= github.com/aws/aws-sdk-go v1.40.34/go.mod h1:585smgzpB/KqRA+K3y/NL/oYRqQvpNJYvLm+LY1U59Q= -github.com/aws/aws-sdk-go v1.43.6 h1:FkwmndZR4LjnT2fiKaD18bnqfQ188E8A1IMNI5rcv00= -github.com/aws/aws-sdk-go v1.43.6/go.mod h1:y4AeaBuwd2Lk+GepC1E9v0qOiTws0MIWAX4oIKwKHZo= +github.com/aws/aws-sdk-go v1.43.7 h1:Gbs53KxXJWbO3txoVkevf56bhdDFqRisl7MQQ6581vc= +github.com/aws/aws-sdk-go v1.43.7/go.mod h1:y4AeaBuwd2Lk+GepC1E9v0qOiTws0MIWAX4oIKwKHZo= github.com/aws/aws-sdk-go-v2 v1.9.0/go.mod h1:cK/D0BBs0b/oWPIcX/Z/obahJK1TT7IPVjy53i/mX/4= github.com/aws/aws-sdk-go-v2/config v1.7.0/go.mod h1:w9+nMZ7soXCe5nT46Ri354SNhXDQ6v+V5wqDjnZE+GY= github.com/aws/aws-sdk-go-v2/credentials v1.4.0/go.mod h1:dgGR+Qq7Wjcd4AOAW5Rf5Tnv3+x7ed6kETXyS9WCuAY= @@ -287,6 +287,8 @@ github.com/go-sql-driver/mysql v1.5.0/go.mod h1:DCzpHaOWr8IXmIStZouvnhqoel9Qv2LB github.com/go-sql-driver/mysql v1.6.0 h1:BCTh4TKNUYmOmMUcQ3IipzF5prigylS7XXjEkfCHuOE= github.com/go-sql-driver/mysql v1.6.0/go.mod h1:DCzpHaOWr8IXmIStZouvnhqoel9Qv2LBy8hT2VhHyBg= github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY= +github.com/go-test/deep v1.0.8 h1:TDsG77qcSprGbC6vTN8OuXp5g+J+b5Pcguhf7Zt61VM= +github.com/go-test/deep v1.0.8/go.mod h1:5C2ZWiW0ErCdrYzpqxLbTX7MG14M9iiw8DgHncVwcsE= github.com/gobwas/httphead v0.0.0-20180130184737-2c6c146eadee/go.mod h1:L0fX3K22YWvt/FAX9NnzrNzcI4wNYi9Yku4O0LKYflo= github.com/gobwas/pool v0.2.0/go.mod h1:q8bcK0KcYlCgd9e7WYLm9LpyS+YeLd8JVDW6WezmKEw= github.com/gobwas/ws v1.0.2/go.mod h1:szmBTxLgaFppYjEmNtny/v3w89xOydFnnZMcgRRu/EM= @@ -744,6 +746,8 @@ github.com/tklauser/go-sysconf v0.3.9/go.mod h1:11DU/5sG7UexIrp/O6g35hrWzu0JxlwQ github.com/tklauser/numcpus v0.3.0/go.mod h1:yFGUr7TUHQRAhyqBcEg0Ge34zDBAsIvJJcyE6boqnA8= github.com/tklauser/numcpus v0.4.0 h1:E53Dm1HjH1/R2/aoCtXtPgzmElmn51aOkhCFSuZq//o= github.com/tklauser/numcpus v0.4.0/go.mod h1:1+UI3pD8NW14VMwdgJNJ1ESk2UnwhAnz5hMwiKKqXCQ= +github.com/toorop/go-dkim v0.0.0-20201103131630-e1cd1a0a5208 h1:PM5hJF7HVfNWmCjMdEfbuOBNXSVF2cMFGgQTPdKCbwM= +github.com/toorop/go-dkim v0.0.0-20201103131630-e1cd1a0a5208/go.mod h1:BzWtXXrXzZUvMacR0oF/fbDDgUPO8L36tDMmRAf14ns= github.com/tv42/httpunix v0.0.0-20150427012821-b75d8614f926/go.mod h1:9ESjWnEqriFuLhtthL60Sar/7RFoluCcXsuvEwTV5KM= github.com/ugorji/go v1.1.7/go.mod h1:kZn38zHttfInRq0xu/PH0az30d+z6vm202qpg1oXVMw= github.com/ugorji/go/codec v1.1.7/go.mod h1:Ax+UKWsSmolVDwsd+7N3ZtXu+yMGCf907BLYF3GoBXY= @@ -751,8 +755,8 @@ github.com/unrolled/secure v1.10.0 h1:TBNP42z2AB+2pW9PR6vdbqhlQuv1iTeSVzK1qHjOBz github.com/unrolled/secure v1.10.0/go.mod h1:BmF5hyM6tXczk3MpQkFf1hpKSRqCyhqcbiQtiAF7+40= github.com/wagslane/go-password-validator v0.3.0 h1:vfxOPzGHkz5S146HDpavl0cw1DSVP061Ry2PX0/ON6I= github.com/wagslane/go-password-validator v0.3.0/go.mod h1:TI1XJ6T5fRdRnHqHt14pvy1tNVnrwe7m3/f1f2fDphQ= -github.com/xhit/go-simple-mail/v2 v2.10.0 h1:nib6RaJ4qVh5HD9UE9QJqnUZyWp3upv+Z6CFxaMj0V8= -github.com/xhit/go-simple-mail/v2 v2.10.0/go.mod h1:kA1XbQfCI4JxQ9ccSN6VFyIEkkugOm7YiPkA5hKiQn4= +github.com/xhit/go-simple-mail/v2 v2.11.0 h1:o/056V50zfkO3Mm5tVdo9rG3ryg4ZmJ2XW5GMinHfVs= +github.com/xhit/go-simple-mail/v2 v2.11.0/go.mod h1:b7P5ygho6SYE+VIqpxA6QkYfv4teeyG4MKqB3utRu98= github.com/yl2chen/cidranger v1.0.3-0.20210928021809-d1cb2c52f37a h1:XfF01GyP+0eWCaVp0y6rNN+kFp7pt9Da4UUYrJ5XPWA= github.com/yl2chen/cidranger v1.0.3-0.20210928021809-d1cb2c52f37a/go.mod h1:aXb8yZQEWo1XHGMf1qQfnb83GR/EJ2EBlwtUgAaNBoE= github.com/yuin/goldmark v1.1.25/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= diff --git a/sftpd/sftpd_test.go b/sftpd/sftpd_test.go index 8bfc4f5b..47365bf0 100644 --- a/sftpd/sftpd_test.go +++ b/sftpd/sftpd_test.go @@ -4431,9 +4431,8 @@ func TestVirtualFolders(t *testing.T) { VirtualPath: vdirPath, }) u.Permissions[testDir] = []string{dataprovider.PermCreateDirs} - u.Permissions[testDir1] = []string{dataprovider.PermCreateDirs, dataprovider.PermUpload, dataprovider.PermDelete} - u.Permissions[path.Join(testDir1, "subdir")] = []string{dataprovider.PermCreateSymlinks, dataprovider.PermUpload, - dataprovider.PermDelete} + u.Permissions[testDir1] = []string{dataprovider.PermCreateDirs, dataprovider.PermUpload, dataprovider.PermRename} + u.Permissions[path.Join(testDir1, "subdir")] = []string{dataprovider.PermRename} user, _, err := httpdtest.AddUser(u, http.StatusCreated) assert.NoError(t, err) @@ -4492,10 +4491,9 @@ func TestVirtualFolders(t *testing.T) { assert.NoError(t, err) err = sftpUploadFile(testFilePath, path.Join("vdir2", "subdir", "subdir", testFileName), testFileSize, client) assert.NoError(t, err) - // we cannot create dirs inside /userDir1/subdir err = client.Rename("vdir2", testDir1) - assert.Error(t, err) - err = client.Rename("vdir2", "vdir3") + assert.NoError(t, err) + err = client.Rename(testDir1, "vdir3") assert.NoError(t, err) err = client.Remove(path.Join("vdir3", "subdir", "subdir", testFileName)) assert.NoError(t, err)