Fix FutureFile MOVE to keep destination node

Sabre usually deletes the target node on MOVE before proceeding with the
actual move operation. This fix prevents this to happen in case the
source node is a FutureFile.

diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php
index ddf324ce8518416a65e56f8fca48ed2f5677e58f..9afa63676856c48f2626955f2be0f720a7f362a9 100644 (file)
--- a/apps/dav/lib/Connector/Sabre/Directory.php
+++ b/apps/dav/lib/Connector/Sabre/Directory.php
@@ -120,9 +120,9 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node
                        if (isset($_SERVER['HTTP_OC_CHUNKED'])) {
 
                                // exit if we can't create a new file and we don't updatable existing file
-                               $info = \OC_FileChunking::decodeName($name);
+                               $chunkInfo = \OC_FileChunking::decodeName($name);
                                if (!$this->fileView->isCreatable($this->path) &&
-                                       !$this->fileView->isUpdatable($this->path . '/' . $info['name'])
+                                       !$this->fileView->isUpdatable($this->path . '/' . $chunkInfo['name'])
                                ) {
                                        throw new \Sabre\DAV\Exception\Forbidden();
                                }
@@ -137,8 +137,12 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node
                        $this->fileView->verifyPath($this->path, $name);
 
                        $path = $this->fileView->getAbsolutePath($this->path) . '/' . $name;
-                       // using a dummy FileInfo is acceptable here since it will be refreshed after the put is complete
-                       $info = new \OC\Files\FileInfo($path, null, null, array(), null);
+                       // in case the file already exists/overwriting
+                       $info = $this->fileView->getFileInfo($this->path . '/' . $name);
+                       if (!$info) {
+                               // use a dummy FileInfo which is acceptable here since it will be refreshed after the put is complete
+                               $info = new \OC\Files\FileInfo($path, null, null, [], null);
+                       }
                        $node = new \OCA\DAV\Connector\Sabre\File($this->fileView, $info);
                        $node->acquireLock(ILockingProvider::LOCK_SHARED);
                        return $node->put($data);

diff --git a/apps/dav/lib/Connector/Sabre/FilesPlugin.php b/apps/dav/lib/Connector/Sabre/FilesPlugin.php
index 1b1f43df9eac433a8b004b58a19ba45758bf193c..929cd1b0bea2626eb26c0a3c864f31b0e012be48 100644 (file)
--- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php
+++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php
@@ -45,6 +45,7 @@ use \Sabre\HTTP\ResponseInterface;
 use OCP\Files\StorageNotAvailableException;
 use OCP\IConfig;
 use OCP\IRequest;
+use OCA\DAV\Upload\FutureFile;
 
 class FilesPlugin extends ServerPlugin {
 
@@ -177,6 +178,7 @@ class FilesPlugin extends ServerPlugin {
                        }
                });
                $this->server->on('beforeMove', [$this, 'checkMove']);
+               $this->server->on('beforeMove', [$this, 'beforeMoveFutureFile']);
        }
 
        /**
@@ -436,4 +438,43 @@ class FilesPlugin extends ServerPlugin {
                        }
                }
        }
+
+       /**
+        * Move handler for future file.
+        *
+        * This overrides the default move behavior to prevent Sabre
+        * to delete the target file before moving. Because deleting would
+        * lose the file id and metadata.
+        *
+        * @param string $path source path
+        * @param string $destination destination path
+        * @return bool|void false to stop handling, void to skip this handler
+        */
+       public function beforeMoveFutureFile($path, $destination) {
+               $sourceNode = $this->tree->getNodeForPath($path);
+               if (!$sourceNode instanceof FutureFile) {
+                       // skip handling as the source is not a chunked FutureFile
+                       return;
+               }
+
+               if (!$this->tree->nodeExists($destination)) {
+                       // skip and let the default handler do its work
+                       return;
+               }
+
+               // do a move manually, skipping Sabre's default "delete" for existing nodes
+               $this->tree->move($path, $destination);
+
+               // trigger all default events (copied from CorePlugin::move)
+               $this->server->emit('afterMove', [$path, $destination]);
+               $this->server->emit('afterUnbind', [$path]);
+               $this->server->emit('afterBind', [$destination]);
+
+               $response = $this->server->httpResponse;
+               $response->setHeader('Content-Length', '0');
+               $response->setStatus(204);
+
+               return false;
+       }
+
 }

diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php
index 1c9ebdd09b6c09b74fb9bce5b7a124b0891a10aa..e0dea49c49b4d617cb838318ad9f5b337c43cfce 100644 (file)
--- a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php
+++ b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php
@@ -32,6 +32,8 @@ use Sabre\DAV\PropPatch;
 use Sabre\HTTP\RequestInterface;
 use Sabre\HTTP\ResponseInterface;
 use Test\TestCase;
+use OCA\DAV\Upload\FutureFile;
+use OCA\DAV\Connector\Sabre\Directory;
 
 /**
  * Copyright (c) 2015 Vincent Petry <pvince81@owncloud.com>
@@ -107,6 +109,12 @@ class FilesPluginTest extends TestCase {
                        $this->request,
                        $this->previewManager
                );
+
+               $response = $this->getMockBuilder(ResponseInterface::class)
+                               ->disableOriginalConstructor()
+                               ->getMock();
+               $this->server->httpResponse = $response;
+
                $this->plugin->initialize($this->server);
        }
 
@@ -535,4 +543,59 @@ class FilesPluginTest extends TestCase {
 
                $this->assertEquals("false", $propFind->get(self::HAS_PREVIEW_PROPERTYNAME));
        }
+
+       public function testBeforeMoveFutureFileSkip() {
+               $node = $this->createMock(Directory::class);
+
+               $this->tree->expects($this->any())
+                       ->method('getNodeForPath')
+                       ->with('source')
+                       ->will($this->returnValue($node));
+               $this->server->httpResponse->expects($this->never())
+                       ->method('setStatus');
+
+               $this->assertNull($this->plugin->beforeMoveFutureFile('source', 'target'));
+       }
+
+       public function testBeforeMoveFutureFileSkipNonExisting() {
+               $sourceNode = $this->createMock(FutureFile::class);
+
+               $this->tree->expects($this->any())
+                       ->method('getNodeForPath')
+                       ->with('source')
+                       ->will($this->returnValue($sourceNode));
+               $this->tree->expects($this->any())
+                       ->method('nodeExists')
+                       ->with('target')
+                       ->will($this->returnValue(false));
+               $this->server->httpResponse->expects($this->never())
+                       ->method('setStatus');
+
+               $this->assertNull($this->plugin->beforeMoveFutureFile('source', 'target'));
+       }
+
+       public function testBeforeMoveFutureFileMoveIt() {
+               $sourceNode = $this->createMock(FutureFile::class);
+
+               $this->tree->expects($this->any())
+                       ->method('getNodeForPath')
+                       ->with('source')
+                       ->will($this->returnValue($sourceNode));
+               $this->tree->expects($this->any())
+                       ->method('nodeExists')
+                       ->with('target')
+                       ->will($this->returnValue(true));
+               $this->tree->expects($this->once())
+                       ->method('move')
+                       ->with('source', 'target');
+
+               $this->server->httpResponse->expects($this->once())
+                       ->method('setHeader')
+                       ->with('Content-Length', '0');
+               $this->server->httpResponse->expects($this->once())
+                       ->method('setStatus')
+                       ->with(204);
+
+               $this->assertFalse($this->plugin->beforeMoveFutureFile('source', 'target'));
+       }
 }