From 7b6e4d0dd2dce40fcc7da9de2b7e49b26dfe1914 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 30 Mar 2017 11:30:37 +0200 Subject: [PATCH] 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. --- apps/dav/lib/Connector/Sabre/Directory.php | 12 ++-- apps/dav/lib/Connector/Sabre/FilesPlugin.php | 41 ++++++++++++ .../unit/Connector/Sabre/FilesPluginTest.php | 63 +++++++++++++++++++ 3 files changed, 112 insertions(+), 4 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php index ddf324ce85..9afa636768 100644 --- 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 1b1f43df9e..929cd1b0be 100644 --- 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 1c9ebdd09b..e0dea49c49 100644 --- 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 @@ -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')); + } }