permissions: improve rename

Allow to enable rename permission in a more controlled way granting "delete"
permission on source directory and "upload" permission on target directory

docs/account.md

@@ -20,7 +20,7 @@ For each account, the following properties can be configured:
     - `upload` upload files is allowed
     - `overwrite` overwrite an existing file, while uploading, is allowed. `upload` permission is required to allow file overwrite
     - `delete` delete files or directories is allowed
-    - `rename` rename files or directories is allowed
+    - `rename` rename files or directories is allowed if this permission is granted on target path. You can enable rename in a more controlled way granting `delete` permission on source directory and `upload` permission on target directory
     - `create_dirs` create directories is allowed
     - `create_symlinks` create symbolic links is allowed
     - `chmod` changing file or directory permissions is allowed. On Windows, only the 0200 bit (owner writable) of mode is used; it controls whether the file's read-only attribute is set or cleared. The other bits are currently unused. Use mode 0400 for a read-only file and 0600 for a readable+writable file.

docs/ssh-commands.md

@@ -10,7 +10,7 @@ We support the following SSH commands:
 - `git-receive-pack`, `git-upload-pack`, `git-upload-archive`. These commands enable support for Git repositories over SSH. They need to be installed and in your system's `PATH`. Git commands are not allowed inside virtual folders or inside directories with file extensions filters.
 - `rsync`. The `rsync` command needs to be installed and in your system's `PATH`. We cannot avoid that rsync creates symlinks, so if the user has the permission to create symlinks, we add the option `--safe-links` to the received rsync command if it is not already set. This should prevent creating symlinks that point outside the home dir. If the user cannot create symlinks, we add the option `--munge-links` if it is not already set. This should make symlinks unusable (but manually recoverable). The `rsync` command interacts with the filesystem directly and it is not aware of virtual folders and file extensions filters, so it will be automatically disabled for users with these features enabled.
 - `sftpgo-copy`. This is a builtin copy implementation. It allows server side copy for files and directories. The first argument is the source file/directory and the second one is the destination file/directory, for example `sftpgo-copy <src> <dst>`. The command will fail if the destination directory exists. Copy for directories spanning virtual folders is not supported.
-- `sftpgo-remove`. This is a builtin remove implementation. It allows to remove files and directory recursively. The first argument is the file/directory to remove, for example `sftpgo-remove <dst>`.
+- `sftpgo-remove`. This is a builtin remove implementation. It allows to remove single files and to recursively remove directories. The first argument is the file/directory to remove, for example `sftpgo-remove <dst>`.
 
 The following SSH commands are enabled by default:
 

sftpd/handler.go

@@ -751,7 +751,9 @@ func (c Connection) isRenamePermitted(sourcePath string, request *sftp.Request)
 			return false
 		}
 	}
-	if !c.User.HasPerm(dataprovider.PermRename, path.Dir(request.Target)) {
+	if !c.User.HasPerm(dataprovider.PermRename, path.Dir(request.Target)) &&
+		(!c.User.HasPerm(dataprovider.PermDelete, path.Dir(request.Filepath)) ||
+			!c.User.HasPerm(dataprovider.PermUpload, path.Dir(request.Target))) {
 		return false
 	}
 	return true

sftpd/internal_test.go

@@ -1968,3 +1968,42 @@ func TestUpdateQuotaAfterRenameMissingFile(t *testing.T) {
 	err := c.updateQuotaAfterRename(request, filepath.Join(os.TempDir(), "vdir", "file"), 0)
 	assert.Error(t, err)
 }
+
+func TestRenamePermission(t *testing.T) {
+	permissions := make(map[string][]string)
+	permissions["/"] = []string{dataprovider.PermAny}
+	permissions["/dir1"] = []string{dataprovider.PermRename}
+	permissions["/dir2"] = []string{dataprovider.PermUpload}
+	permissions["/dir3"] = []string{dataprovider.PermDelete}
+	permissions["/dir4"] = []string{dataprovider.PermListItems}
+
+	user := dataprovider.User{
+		Permissions: permissions,
+		HomeDir:     os.TempDir(),
+	}
+	fs, err := user.GetFilesystem("123")
+	assert.NoError(t, err)
+	conn := Connection{
+		User: user,
+		fs:   fs,
+	}
+	request := sftp.NewRequest("Rename", "/testfile")
+	request.Target = "/dir4/testfile"
+	// rename is not granted on Target
+	assert.False(t, conn.isRenamePermitted("", request))
+	request = sftp.NewRequest("Rename", "/dir4/testfile")
+	request.Target = "/dir1/testfile"
+	// rename is granted on Target, this is enough
+	assert.True(t, conn.isRenamePermitted("", request))
+	request = sftp.NewRequest("Rename", "/dir4/testfile")
+	request.Target = "/testfile"
+	// rename is granted on Target, this is enough
+	assert.True(t, conn.isRenamePermitted("", request))
+	request = sftp.NewRequest("Rename", "/dir3/testfile")
+	request.Target = "/dir2/testfile"
+	// delete is granted on Source and Upload on Target, this is enough
+	assert.True(t, conn.isRenamePermitted("", request))
+	request = sftp.NewRequest("Rename", "/dir2/testfile")
+	request.Target = "/dir3/testfile"
+	assert.False(t, conn.isRenamePermitted("", request))
+}

sftpd/sftpd_test.go

@@ -4244,7 +4244,7 @@ func TestPermDelete(t *testing.T) {
 func TestPermRename(t *testing.T) {
 	usePubKey := false
 	u := getTestUser(usePubKey)
-	u.Permissions["/"] = []string{dataprovider.PermListItems, dataprovider.PermDownload, dataprovider.PermUpload, dataprovider.PermDelete,
+	u.Permissions["/"] = []string{dataprovider.PermListItems, dataprovider.PermDownload, dataprovider.PermUpload,
 		dataprovider.PermCreateDirs, dataprovider.PermCreateSymlinks, dataprovider.PermOverwrite, dataprovider.PermChmod,
 		dataprovider.PermChown, dataprovider.PermChtimes}
 	user, _, err := httpd.AddUser(u, http.StatusOK)
@@ -4263,7 +4263,7 @@ func TestPermRename(t *testing.T) {
 		if assert.Error(t, err) {
 			assert.Contains(t, err.Error(), permissionErrorString)
 		}
-		err = client.Remove(testFileName)
+		_, err = client.Stat(testFileName)
 		assert.NoError(t, err)
 		err = os.Remove(testFilePath)
 		assert.NoError(t, err)
@@ -4659,7 +4659,8 @@ func TestPermsSubDirsCommands(t *testing.T) {
 	usePubKey := true
 	u := getTestUser(usePubKey)
 	u.Permissions["/"] = []string{dataprovider.PermAny}
-	u.Permissions["/subdir"] = []string{dataprovider.PermDownload, dataprovider.PermUpload}
+	u.Permissions["/subdir"] = []string{dataprovider.PermDownload, dataprovider.PermUpload, dataprovider.PermCreateDirs}
+	u.Permissions["/subdir/otherdir"] = []string{dataprovider.PermListItems, dataprovider.PermDownload}
 	user, _, err := httpd.AddUser(u, http.StatusOK)
 	assert.NoError(t, err)
 	client, err := getSftpClient(user, usePubKey)
@@ -4682,13 +4683,15 @@ func TestPermsSubDirsCommands(t *testing.T) {
 		if assert.Error(t, err) {
 			assert.Contains(t, err.Error(), permissionErrorString)
 		}
-		err = client.Mkdir("/subdir/dir")
+		err = client.Mkdir("/subdir/otherdir/dir")
 		if assert.Error(t, err) {
 			assert.Contains(t, err.Error(), permissionErrorString)
 		}
 		err = client.Mkdir("/otherdir")
 		assert.NoError(t, err)
-		err = client.Rename("/otherdir", "/subdir/otherdir")
+		err = client.Mkdir("/subdir/otherdir")
+		assert.NoError(t, err)
+		err = client.Rename("/otherdir", "/subdir/otherdir/adir")
 		if assert.Error(t, err) {
 			assert.Contains(t, err.Error(), permissionErrorString)
 		}
@@ -4700,7 +4703,7 @@ func TestPermsSubDirsCommands(t *testing.T) {
 		assert.NoError(t, err)
 		err = client.Rename("/otherdir", "/otherdir1")
 		assert.NoError(t, err)
-		err = client.RemoveDirectory("/subdir")
+		err = client.RemoveDirectory("/otherdir1")
 		assert.NoError(t, err)
 	}
 	_, err = httpd.RemoveUser(user, http.StatusOK)