fix: Renaming does not need update but delete permissions

Renaming is basically copy + delete (a move), so no need to update permissions.
Especially if the node is in a invalid directory the node should be moveable but not editable.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

6 files changed, 18 insertions, 15 deletions

diff --git a/apps/dav/lib/Connector/Sabre/Node.php b/apps/dav/lib/Connector/Sabre/Node.php
index 973d5ca6f0e..1d3773220f8 100644
--- a/apps/dav/lib/Connector/Sabre/Node.php
+++ b/apps/dav/lib/Connector/Sabre/Node.php
@@ -117,8 +117,9 @@ abstract class Node implements \Sabre\DAV\INode {
 	 * @throws \Sabre\DAV\Exception\Forbidden
 	 */
 	public function setName($name) {
-		// rename is only allowed if the update privilege is granted
-		if (!($this->info->isUpdateable() || ($this->info->getMountPoint() instanceof MoveableMount && $this->info->getInternalPath() === ''))) {
+		// rename is only allowed if the delete privilege is granted
+		// (basically rename is a copy with delete of the original node)
+		if (!($this->info->isDeletable() || ($this->info->getMountPoint() instanceof MoveableMount && $this->info->getInternalPath() === ''))) {
 			throw new \Sabre\DAV\Exception\Forbidden();
 		}
 
@@ -129,7 +130,6 @@ abstract class Node implements \Sabre\DAV\INode {
 		// verify path of the target
 		$this->verifyPath($newPath);
 
-
 		if (!$this->fileView->rename($this->path, $newPath)) {
 			throw new \Sabre\DAV\Exception('Failed to rename '. $this->path . ' to ' . $newPath);
 		}
diff --git a/apps/files/src/actions/moveOrCopyActionUtils.ts b/apps/files/src/actions/moveOrCopyActionUtils.ts
index d2e276b2b93..0c7822390ac 100644
--- a/apps/files/src/actions/moveOrCopyActionUtils.ts
+++ b/apps/files/src/actions/moveOrCopyActionUtils.ts
@@ -38,7 +38,7 @@ export type MoveCopyResult = {
 
 export const canMove = (nodes: Node[]) => {
 	const minPermission = nodes.reduce((min, node) => Math.min(min, node.permissions), Permission.ALL)
-	return (minPermission & Permission.UPDATE) !== 0
+	return Boolean(minPermission & Permission.DELETE)
 }
 
 export const canDownload = (nodes: Node[]) => {
diff --git a/apps/files/src/actions/renameAction.spec.ts b/apps/files/src/actions/renameAction.spec.ts
index b309c11d9a6..954eca5820f 100644
--- a/apps/files/src/actions/renameAction.spec.ts
+++ b/apps/files/src/actions/renameAction.spec.ts
@@ -30,14 +30,14 @@ describe('Rename action enabled tests', () => {
 			source: 'https://cloud.domain.com/remote.php/dav/files/admin/foobar.txt',
 			owner: 'admin',
 			mime: 'text/plain',
-			permissions: Permission.UPDATE,
+			permissions: Permission.UPDATE | Permission.DELETE,
 		})
 
 		expect(action.enabled).toBeDefined()
 		expect(action.enabled!([file], view)).toBe(true)
 	})
 
-	test('Disabled for node without UPDATE permission', () => {
+	test('Disabled for node without DELETE permission', () => {
 		const file = new File({
 			id: 1,
 			source: 'https://cloud.domain.com/remote.php/dav/files/admin/foobar.txt',
diff --git a/apps/files/src/actions/renameAction.ts b/apps/files/src/actions/renameAction.ts
index c00a99b4de1..e4dbb0ed129 100644
--- a/apps/files/src/actions/renameAction.ts
+++ b/apps/files/src/actions/renameAction.ts
@@ -17,7 +17,7 @@ export const action = new FileAction({
 	enabled: (nodes: Node[]) => {
 		return nodes.length > 0 && nodes
 			.map(node => node.permissions)
-			.every(permission => (permission & Permission.UPDATE) !== 0)
+			.every(permission => Boolean(permission & Permission.DELETE))
 	},
 
 	async exec(node: Node) {
diff --git a/lib/private/Files/Node/Node.php b/lib/private/Files/Node/Node.php
index b49a04e6f62..5dbdc4054bf 100644
--- a/lib/private/Files/Node/Node.php
+++ b/lib/private/Files/Node/Node.php
@@ -432,11 +432,14 @@ class Node implements INode {
 		$targetPath = $this->normalizePath($targetPath);
 		$parent = $this->root->get(dirname($targetPath));
 		if (
-			$parent instanceof Folder and
-			$this->isValidPath($targetPath) and
-			(
-				$parent->isCreatable() ||
-				($parent->getInternalPath() === '' && $parent->getMountPoint() instanceof MoveableMount)
+			($parent instanceof Folder)
+			&& $this->isValidPath($targetPath)
+			&& (
+				$parent->isCreatable()
+				|| (
+					$parent->getInternalPath() === ''
+					&& ($parent->getMountPoint() instanceof MoveableMount)
+				)
 			)
 		) {
 			$nonExisting = $this->createNonExistingNode($targetPath);
diff --git a/tests/lib/Files/Node/IntegrationTest.php b/tests/lib/Files/Node/IntegrationTest.php
index 7d87cdb5dd0..fe6fe779ad0 100644
--- a/tests/lib/Files/Node/IntegrationTest.php
+++ b/tests/lib/Files/Node/IntegrationTest.php
@@ -12,6 +12,7 @@ use OC\Files\Storage\Temporary;
 use OC\Files\View;
 use OC\Memcache\ArrayCache;
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\Files\Config\IUserMountCache;
 use OCP\Files\Mount\IMountManager;
 use OCP\ICacheFactory;
 use OCP\IUserManager;
@@ -46,8 +47,7 @@ class IntegrationTest extends \Test\TestCase {
 	protected function setUp(): void {
 		parent::setUp();
 
-		/** @var IMountManager $manager */
-		$manager = \OC::$server->get(IMountManager::class);
+		$manager = \OCP\Server::get(IMountManager::class);
 
 		\OC_Hook::clear('OC_Filesystem');
 
@@ -64,7 +64,7 @@ class IntegrationTest extends \Test\TestCase {
 			$manager,
 			$this->view,
 			$user,
-			\OC::$server->getUserMountCache(),
+			\OCP\Server::get(IUserMountCache::class),
 			$this->createMock(LoggerInterface::class),
 			$this->createMock(IUserManager::class),
 			$this->createMock(IEventDispatcher::class),