Merge pull request #52821 from nextcloud/fix/file-drop

fix(dav): handle uploading folders with names of existing file for file drop plugin

apps/dav/appinfo/v1/publicwebdav.php

@@ -104,11 +104,9 @@ $server = $serverFactory->createServer(false, $baseuri, $requestUri, $authPlugin
 	if (!$isReadable) {
 		$filesDropPlugin->enable();
 	}
-
-	$view = new View($node->getPath());
-	$filesDropPlugin->setView($view);
 	$filesDropPlugin->setShare($share);
 
+	$view = new View($node->getPath());
 	return $view;
 });
 

apps/dav/appinfo/v2/publicremote.php

@@ -131,11 +131,9 @@ $server = $serverFactory->createServer(true, $baseuri, $requestUri, $authPlugin,
 	if (!$isReadable) {
 		$filesDropPlugin->enable();
 	}
-
-	$view = new View($node->getPath());
-	$filesDropPlugin->setView($view);
 	$filesDropPlugin->setShare($share);
 
+	$view = new View($node->getPath());
 	return $view;
 });
 

apps/dav/lib/Files/Sharing/FilesDropPlugin.php

@@ -5,7 +5,8 @@
  */
 namespace OCA\DAV\Files\Sharing;
 
-use OC\Files\View;
+use OCP\Files\Folder;
+use OCP\Files\NotFoundException;
 use OCP\Share\IShare;
 use Sabre\DAV\Exception\MethodNotAllowed;
 use Sabre\DAV\ServerPlugin;
@@ -17,14 +18,9 @@ use Sabre\HTTP\ResponseInterface;
  */
 class FilesDropPlugin extends ServerPlugin {
 
-	private ?View $view = null;
 	private ?IShare $share = null;
 	private bool $enabled = false;
 
-	public function setView(View $view): void {
-		$this->view = $view;
-	}
-
 	public function setShare(IShare $share): void {
 		$this->share = $share;
 	}
@@ -33,7 +29,6 @@ class FilesDropPlugin extends ServerPlugin {
 		$this->enabled = true;
 	}
 
-
 	/**
 	 * This initializes the plugin.
 	 * It is ONLY initialized by the server on a file drop request.
@@ -45,7 +40,12 @@ class FilesDropPlugin extends ServerPlugin {
 	}
 
 	public function onMkcol(RequestInterface $request, ResponseInterface $response) {
-		if (!$this->enabled || $this->share === null || $this->view === null) {
+		if (!$this->enabled || $this->share === null) {
+			return;
+		}
+
+		$node = $this->share->getNode();
+		if (!($node instanceof Folder)) {
 			return;
 		}
 
@@ -57,7 +57,12 @@ class FilesDropPlugin extends ServerPlugin {
 	}
 
 	public function beforeMethod(RequestInterface $request, ResponseInterface $response) {
-		if (!$this->enabled || $this->share === null || $this->view === null) {
+		if (!$this->enabled || $this->share === null) {
+			return;
+		}
+
+		$node = $this->share->getNode();
+		if (!($node instanceof Folder)) {
 			return;
 		}
 
@@ -127,45 +132,55 @@ class FilesDropPlugin extends ServerPlugin {
 		}
 
 		// Create the folders along the way
-		$folders = $this->getPathSegments(dirname($relativePath));
-		foreach ($folders as $folder) {
-			if ($folder === '') {
+		$folder = $node;
+		$pathSegments = $this->getPathSegments(dirname($relativePath));
+		foreach ($pathSegments as $pathSegment) {
+			if ($pathSegment === '') {
 				continue;
-			} // skip empty parts
-			if (!$this->view->file_exists($folder)) {
-				$this->view->mkdir($folder);
+			}
+
+			try {
+				// get the current folder
+				$currentFolder = $folder->get($pathSegment);
+				// check target is a folder
+				if ($currentFolder instanceof Folder) {
+					$folder = $currentFolder;
+				} else {
+					// otherwise look in the parent folder if we already create an unique folder name
+					foreach ($folder->getDirectoryListing() as $child) {
+						// we look for folders which match "NAME (SUFFIX)"
+						if ($child instanceof Folder && str_starts_with($child->getName(), $pathSegment)) {
+							$suffix = substr($child->getName(), strlen($pathSegment));
+							if (preg_match('/^ \(\d+\)$/', $suffix)) {
+								// we found the unique folder name and can use it
+								$folder = $child;
+								break;
+							}
+						}
+					}
+					// no folder found so we need to create a new unique folder name
+					if (!isset($child) || $child !== $folder) {
+						$folder = $folder->newFolder($folder->getNonExistingName($pathSegment));
+					}
+				}
+			} catch (NotFoundException) {
+				// the folder does simply not exist so we create it
+				$folder = $folder->newFolder($pathSegment);
 			}
 		}
 
 		// Finally handle conflicts on the end files
-		$noConflictPath = \OC_Helper::buildNotExistingFileNameForView(dirname($relativePath), basename($relativePath), $this->view);
-		$path = '/files/' . $token . '/' . $noConflictPath;
-		$url = $request->getBaseUrl() . str_replace('//', '/', $path);
+		$uniqueName = $folder->getNonExistingName(basename($relativePath));
+		$relativePath = substr($folder->getPath(), strlen($node->getPath()));
+		$path = '/files/' . $token . '/' . $relativePath . '/' . $uniqueName;
+		$url = rtrim($request->getBaseUrl(), '/') . str_replace('//', '/', $path);
 		$request->setUrl($url);
 	}
 
 	private function getPathSegments(string $path): array {
 		// Normalize slashes and remove trailing slash
-		$path = rtrim(str_replace('\\', '/', $path), '/');
+		$path = trim(str_replace('\\', '/', $path), '/');
 
-		// Handle absolute paths starting with /
-		$isAbsolute = str_starts_with($path, '/');
-
-		$segments = explode('/', $path);
-
-		// Add back the leading slash for the first segment if needed
-		$result = [];
-		$current = $isAbsolute ? '/' : '';
-
-		foreach ($segments as $segment) {
-			if ($segment === '') {
-				// skip empty parts
-				continue;
-			}
-			$current = rtrim($current, '/') . '/' . $segment;
-			$result[] = $current;
-		}
-
-		return $result;
+		return explode('/', $path);
 	}
 }

apps/dav/tests/unit/Files/Sharing/FilesDropPluginTest.php

@@ -5,10 +5,12 @@
  */
 namespace OCA\DAV\Tests\Files\Sharing;
 
-use OC\Files\View;
 use OCA\DAV\Files\Sharing\FilesDropPlugin;
+use OCP\Files\Folder;
+use OCP\Files\NotFoundException;
 use OCP\Share\IAttributes;
 use OCP\Share\IShare;
+use PHPUnit\Framework\MockObject\MockObject;
 use Sabre\DAV\Exception\MethodNotAllowed;
 use Sabre\DAV\Server;
 use Sabre\HTTP\RequestInterface;
@@ -17,29 +19,25 @@ use Test\TestCase;
 
 class FilesDropPluginTest extends TestCase {
 
-	/** @var View|\PHPUnit\Framework\MockObject\MockObject */
-	private $view;
+	private FilesDropPlugin $plugin;
 
-	/** @var IShare|\PHPUnit\Framework\MockObject\MockObject */
-	private $share;
-
-	/** @var Server|\PHPUnit\Framework\MockObject\MockObject */
-	private $server;
-
-	/** @var FilesDropPlugin */
-	private $plugin;
-
-	/** @var RequestInterface|\PHPUnit\Framework\MockObject\MockObject */
-	private $request;
-
-	/** @var ResponseInterface|\PHPUnit\Framework\MockObject\MockObject */
-	private $response;
+	private Folder&MockObject $node;
+	private IShare&MockObject $share;
+	private Server&MockObject $server;
+	private RequestInterface&MockObject $request;
+	private ResponseInterface&MockObject $response;
 
 	protected function setUp(): void {
 		parent::setUp();
 
-		$this->view = $this->createMock(View::class);
+		$this->node = $this->createMock(Folder::class);
+		$this->node->method('getPath')
+			->willReturn('/files/token');
+
 		$this->share = $this->createMock(IShare::class);
+		$this->share->expects(self::any())
+			->method('getNode')
+			->willReturn($this->node);
 		$this->server = $this->createMock(Server::class);
 		$this->plugin = new FilesDropPlugin();
 
@@ -56,28 +54,7 @@ class FilesDropPluginTest extends TestCase {
 			->willReturn('token');
 	}
 
-	public function testInitialize(): void {
-		$this->server->expects($this->at(0))
-			->method('on')
-			->with(
-				$this->equalTo('beforeMethod:*'),
-				$this->equalTo([$this->plugin, 'beforeMethod']),
-				$this->equalTo(999)
-			);
-		$this->server->expects($this->at(1))
-			->method('on')
-			->with(
-				$this->equalTo('method:MKCOL'),
-				$this->equalTo([$this->plugin, 'onMkcol']),
-			);
-
-		$this->plugin->initialize($this->server);
-	}
-
 	public function testNotEnabled(): void {
-		$this->view->expects($this->never())
-			->method($this->anything());
-
 		$this->request->expects($this->never())
 			->method($this->anything());
 
@@ -86,7 +63,6 @@ class FilesDropPluginTest extends TestCase {
 
 	public function testValid(): void {
 		$this->plugin->enable();
-		$this->plugin->setView($this->view);
 		$this->plugin->setShare($this->share);
 
 		$this->request->method('getMethod')
@@ -98,9 +74,10 @@ class FilesDropPluginTest extends TestCase {
 		$this->request->method('getBaseUrl')
 			->willReturn('https://example.com');
 
-		$this->view->method('file_exists')
-			->with('/file.txt')
-			->willReturn(false);
+		$this->node->expects(self::once())
+			->method('getNonExistingName')
+			->with('file.txt')
+			->willReturn('file.txt');
 
 		$this->request->expects($this->once())
 			->method('setUrl')
@@ -111,7 +88,6 @@ class FilesDropPluginTest extends TestCase {
 
 	public function testFileAlreadyExistsValid(): void {
 		$this->plugin->enable();
-		$this->plugin->setView($this->view);
 		$this->plugin->setShare($this->share);
 
 		$this->request->method('getMethod')
@@ -123,14 +99,9 @@ class FilesDropPluginTest extends TestCase {
 		$this->request->method('getBaseUrl')
 			->willReturn('https://example.com');
 
-		$this->view->method('file_exists')
-			->willReturnCallback(function ($path) {
-				if ($path === 'file.txt' || $path === '/file.txt') {
-					return true;
-				} else {
-					return false;
-				}
-			});
+		$this->node->method('getNonExistingName')
+			->with('file.txt')
+			->willReturn('file (2).txt');
 
 		$this->request->expects($this->once())
 			->method('setUrl')
@@ -141,7 +112,6 @@ class FilesDropPluginTest extends TestCase {
 
 	public function testNoMKCOLWithoutNickname(): void {
 		$this->plugin->enable();
-		$this->plugin->setView($this->view);
 		$this->plugin->setShare($this->share);
 
 		$this->request->method('getMethod')
@@ -154,7 +124,6 @@ class FilesDropPluginTest extends TestCase {
 
 	public function testMKCOLWithNickname(): void {
 		$this->plugin->enable();
-		$this->plugin->setView($this->view);
 		$this->plugin->setShare($this->share);
 
 		$this->request->method('getMethod')
@@ -174,7 +143,6 @@ class FilesDropPluginTest extends TestCase {
 
 	public function testSubdirPut(): void {
 		$this->plugin->enable();
-		$this->plugin->setView($this->view);
 		$this->plugin->setShare($this->share);
 
 		$this->request->method('getMethod')
@@ -193,14 +161,30 @@ class FilesDropPluginTest extends TestCase {
 		$this->request->method('getBaseUrl')
 			->willReturn('https://example.com');
 
-		$this->view->method('file_exists')
-			->willReturnCallback(function ($path) {
-				if ($path === 'file.txt' || $path === '/folder/file.txt') {
-					return true;
-				} else {
-					return false;
-				}
-			});
+		$nodeName = $this->createMock(Folder::class);
+		$nodeFolder = $this->createMock(Folder::class);
+		$nodeFolder->expects(self::once())
+			->method('getPath')
+			->willReturn('/files/token/nickname/folder');
+		$nodeFolder->method('getNonExistingName')
+			->with('file.txt')
+			->willReturn('file.txt');
+		$nodeName->expects(self::once())
+			->method('get')
+			->with('folder')
+			->willThrowException(new NotFoundException());
+		$nodeName->expects(self::once())
+			->method('newFolder')
+			->with('folder')
+			->willReturn($nodeFolder);
+
+		$this->node->expects(self::once())
+			->method('get')
+			->willThrowException(new NotFoundException());
+		$this->node->expects(self::once())
+			->method('newFolder')
+			->with('nickname')
+			->willReturn($nodeName);
 
 		$this->request->expects($this->once())
 			->method('setUrl')
@@ -211,7 +195,6 @@ class FilesDropPluginTest extends TestCase {
 
 	public function testRecursiveFolderCreation(): void {
 		$this->plugin->enable();
-		$this->plugin->setView($this->view);
 		$this->plugin->setShare($this->share);
 
 		$this->request->method('getMethod')
@@ -227,40 +210,40 @@ class FilesDropPluginTest extends TestCase {
 			->willReturn('/files/token/folder/subfolder/file.txt');
 		$this->request->method('getBaseUrl')
 			->willReturn('https://example.com');
-		$this->view->method('file_exists')
-			->willReturn(false);
-
-		$this->view->expects($this->exactly(4))
-			->method('file_exists')
-			->withConsecutive(
-				['/nickname'],
-				['/nickname/folder'],
-				['/nickname/folder/subfolder'],
-				['/nickname/folder/subfolder/file.txt']
-			)
-			->willReturnOnConsecutiveCalls(
-				false,
-				false,
-				false,
-				false,
-			);
-		$this->view->expects($this->exactly(3))
-			->method('mkdir')
-			->withConsecutive(
-				['/nickname'],
-				['/nickname/folder'],
-				['/nickname/folder/subfolder'],
-			);
 
 		$this->request->expects($this->once())
 			->method('setUrl')
 			->with($this->equalTo('https://example.com/files/token/nickname/folder/subfolder/file.txt'));
+
+		$subfolder = $this->createMock(Folder::class);
+		$subfolder->expects(self::once())
+			->method('getNonExistingName')
+			->with('file.txt')
+			->willReturn('file.txt');
+		$subfolder->expects(self::once())
+			->method('getPath')
+			->willReturn('/files/token/nickname/folder/subfolder');
+
+		$folder = $this->createMock(Folder::class);
+		$folder->expects(self::once())
+			->method('get')
+			->with('subfolder')
+			->willReturn($subfolder);
+
+		$nickname = $this->createMock(Folder::class);
+		$nickname->expects(self::once())
+			->method('get')
+			->with('folder')
+			->willReturn($folder);
+
+		$this->node->method('get')
+			->with('nickname')
+			->willReturn($nickname);
 		$this->plugin->beforeMethod($this->request, $this->response);
 	}
 
 	public function testOnMkcol(): void {
 		$this->plugin->enable();
-		$this->plugin->setView($this->view);
 		$this->plugin->setShare($this->share);
 
 		$this->response->expects($this->once())

build/integration/filesdrop_features/filesdrop.feature

@@ -99,6 +99,47 @@ Feature: FilesDrop
     And Downloading file "/drop/Alice/folder/a.txt"
     Then Downloaded content should be "abc"
 
+  Scenario: File drop uploading folder with name of file
+    Given user "user0" exists
+    And As an "user0"
+    And user "user0" created a folder "/drop"
+    And as "user0" creating a share with
+      | path | drop |
+      | shareType | 4 |
+      | permissions | 4 |
+      | attributes | [{"scope":"fileRequest","key":"enabled","value":true}] |
+      | shareWith |  |
+    When Dropping file "/folder" with "its a file" as "Alice"
+    Then the HTTP status code should be "201"
+    When Dropping file "/folder/a.txt" with "abc" as "Alice"
+    Then the HTTP status code should be "201"
+    When Downloading file "/drop/Alice/folder"
+    Then the HTTP status code should be "200"
+    And Downloaded content should be "its a file"
+    When Downloading file "/drop/Alice/folder (2)/a.txt"
+    Then Downloaded content should be "abc"
+
+  Scenario: File drop uploading file with name of folder
+    Given user "user0" exists
+    And As an "user0"
+    And user "user0" created a folder "/drop"
+    And as "user0" creating a share with
+      | path | drop |
+      | shareType | 4 |
+      | permissions | 4 |
+      | attributes | [{"scope":"fileRequest","key":"enabled","value":true}] |
+      | shareWith |  |
+    When Dropping file "/folder/a.txt" with "abc" as "Alice"
+    Then the HTTP status code should be "201"
+    When Dropping file "/folder" with "its a file" as "Alice"
+    Then the HTTP status code should be "201"
+    When Downloading file "/drop/Alice/folder/a.txt"
+    Then the HTTP status code should be "200"
+    And Downloaded content should be "abc"
+    When Downloading file "/drop/Alice/folder (2)"
+    Then the HTTP status code should be "200"
+    And Downloaded content should be "its a file"
+    
   Scenario: Put file same file multiple times via files drop
     Given user "user0" exists
     And As an "user0"