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 <[email protected]>

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))
+		// 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.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
+		perms := []string{
+			dataprovider.PermRenameDirs,
+			dataprovider.PermRename,
 		}
-		return c.User.HasPerm(dataprovider.PermCreateDirs, path.Dir(virtualTargetPath))
+		return c.User.HasAnyPerm(perms, path.Dir(virtualSourcePath)) &&
+			c.User.HasAnyPerm(perms, 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)) {
-		return false
-	}
-	if fi.Mode()&os.ModeSymlink != 0 {
-		return c.User.HasPerm(dataprovider.PermCreateSymlinks, path.Dir(virtualTargetPath))
+	perms := []string{
+		dataprovider.PermRenameFiles,
+		dataprovider.PermRename,
 	}
-	return c.User.HasPerm(dataprovider.PermUpload, path.Dir(virtualTargetPath))
+	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 {

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) {

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)

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.

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

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=

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)