From ea269f0067dda7cee0621443a9620607297b0387 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Tue, 8 Jul 2014 09:44:46 +0200 Subject: [PATCH] Upload abortion is now detected within the OC_Connector_Sabre_File::put() OC_Connector_Sabre_AbortedUploadDetectionPlugin is pointless Adding unit test testUploadAbort() --- apps/files/appinfo/remote.php | 1 - apps/files_sharing/publicwebdav.php | 1 - .../sabre/aborteduploaddetectionplugin.php | 98 ----------------- lib/private/connector/sabre/file.php | 20 ++-- .../sabre/aborteduploaddetectionplugin.php | 100 ------------------ tests/lib/connector/sabre/file.php | 51 ++++++++- 6 files changed, 60 insertions(+), 211 deletions(-) delete mode 100644 lib/private/connector/sabre/aborteduploaddetectionplugin.php delete mode 100644 tests/lib/connector/sabre/aborteduploaddetectionplugin.php diff --git a/apps/files/appinfo/remote.php b/apps/files/appinfo/remote.php index dd5c470431..3ba25085ba 100644 --- a/apps/files/appinfo/remote.php +++ b/apps/files/appinfo/remote.php @@ -53,7 +53,6 @@ $server->subscribeEvent('beforeMethod', function () use ($server, $objectTree) { $rootDir = new OC_Connector_Sabre_Directory($view, $rootInfo); $objectTree->init($rootDir, $view, $mountManager); - $server->addPlugin(new OC_Connector_Sabre_AbortedUploadDetectionPlugin($view)); $server->addPlugin(new OC_Connector_Sabre_QuotaPlugin($view)); }, 30); // priority 30: after auth (10) and acl(20), before lock(50) and handling the request diff --git a/apps/files_sharing/publicwebdav.php b/apps/files_sharing/publicwebdav.php index 684edd9767..03e43967a4 100644 --- a/apps/files_sharing/publicwebdav.php +++ b/apps/files_sharing/publicwebdav.php @@ -67,7 +67,6 @@ $server->subscribeEvent('beforeMethod', function () use ($server, $objectTree, $ $mountManager = \OC\Files\Filesystem::getMountManager(); $objectTree->init($root, $view, $mountManager); - $server->addPlugin(new OC_Connector_Sabre_AbortedUploadDetectionPlugin($view)); $server->addPlugin(new OC_Connector_Sabre_QuotaPlugin($view)); }, 30); // priority 30: after auth (10) and acl(20), before lock(50) and handling the request diff --git a/lib/private/connector/sabre/aborteduploaddetectionplugin.php b/lib/private/connector/sabre/aborteduploaddetectionplugin.php deleted file mode 100644 index b569f9a83c..0000000000 --- a/lib/private/connector/sabre/aborteduploaddetectionplugin.php +++ /dev/null @@ -1,98 +0,0 @@ - - * This file is licensed under the Affero General Public License version 3 or - * later. - * See the COPYING-README file. - */ - -/** - * Class OC_Connector_Sabre_AbortedUploadDetectionPlugin - * - * This plugin will verify if the uploaded data has been stored completely. - * This is done by comparing the content length of the request with the file size on storage. - */ -class OC_Connector_Sabre_AbortedUploadDetectionPlugin extends \Sabre\DAV\ServerPlugin { - - /** - * Reference to main server object - * - * @var \Sabre\DAV\Server - */ - private $server; - - /** - * @var \OC\Files\View - */ - private $fileView; - - /** - * @param \OC\Files\View $view - */ - public function __construct($view) { - $this->fileView = $view; - } - - /** - * This initializes the plugin. - * - * This function is called by \Sabre\DAV\Server, after - * addPlugin is called. - * - * This method should set up the requires event subscriptions. - * - * @param \Sabre\DAV\Server $server - */ - public function initialize(\Sabre\DAV\Server $server) { - - $this->server = $server; - - $server->subscribeEvent('afterCreateFile', array($this, 'verifyContentLength'), 10); - $server->subscribeEvent('afterWriteContent', array($this, 'verifyContentLength'), 10); - } - - /** - * @param string $filePath - * @param \Sabre\DAV\INode $node - * @throws \Sabre\DAV\Exception\BadRequest - */ - public function verifyContentLength($filePath, \Sabre\DAV\INode $node = null) { - - // we should only react on PUT which is used for upload - // e.g. with LOCK this will not work, but LOCK uses createFile() as well - if ($this->server->httpRequest->getMethod() !== 'PUT') { - return; - } - - // ownCloud chunked upload will be handled in its own plugin - $chunkHeader = $this->server->httpRequest->getHeader('OC-Chunked'); - if ($chunkHeader) { - return; - } - - // compare expected and actual size - $expected = $this->getLength(); - if (!$expected) { - return; - } - $actual = $this->fileView->filesize($filePath); - if ($actual != $expected) { - $this->fileView->unlink($filePath); - throw new \Sabre\DAV\Exception\BadRequest('expected filesize ' . $expected . ' got ' . $actual); - } - - } - - /** - * @return string - */ - public function getLength() { - $req = $this->server->httpRequest; - $length = $req->getHeader('X-Expected-Entity-Length'); - if (!$length) { - $length = $req->getHeader('Content-Length'); - } - - return $length; - } -} diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php index 7591cc5c06..ece6885f24 100644 --- a/lib/private/connector/sabre/file.php +++ b/lib/private/connector/sabre/file.php @@ -71,13 +71,13 @@ class OC_Connector_Sabre_File extends OC_Connector_Sabre_Node implements \Sabre\ } // mark file as partial while uploading (ignored by the scanner) - $partpath = $this->path . '.ocTransferId' . rand() . '.part'; + $partFilePath = $this->path . '.ocTransferId' . rand() . '.part'; try { - $putOkay = $this->fileView->file_put_contents($partpath, $data); + $putOkay = $this->fileView->file_put_contents($partFilePath, $data); if ($putOkay === false) { \OC_Log::write('webdav', '\OC\Files\Filesystem::file_put_contents() failed', \OC_Log::ERROR); - $this->fileView->unlink($partpath); + $this->fileView->unlink($partFilePath); // because we have no clue about the cause we can only throw back a 500/Internal Server Error throw new \Sabre\DAV\Exception('Could not write file contents'); } @@ -102,13 +102,22 @@ class OC_Connector_Sabre_File extends OC_Connector_Sabre_Node implements \Sabre\ throw new OC_Connector_Sabre_Exception_FileLocked($e->getMessage(), $e->getCode(), $e); } + // double check if the file was fully received + // compare expected and actual size + $expected = $_SERVER['CONTENT_LENGTH']; + $actual = $this->fileView->filesize($partFilePath); + if ($actual != $expected) { + $this->fileView->unlink($partFilePath); + throw new \Sabre\DAV\Exception\BadRequest('expected filesize ' . $expected . ' got ' . $actual); + } + // rename to correct path try { - $renameOkay = $this->fileView->rename($partpath, $this->path); + $renameOkay = $this->fileView->rename($partFilePath, $this->path); $fileExists = $this->fileView->file_exists($this->path); if ($renameOkay === false || $fileExists === false) { \OC_Log::write('webdav', '\OC\Files\Filesystem::rename() failed', \OC_Log::ERROR); - $this->fileView->unlink($partpath); + $this->fileView->unlink($partFilePath); throw new \Sabre\DAV\Exception('Could not rename part file to final file'); } } @@ -259,5 +268,4 @@ class OC_Connector_Sabre_File extends OC_Connector_Sabre_Node implements \Sabre\ return null; } - } diff --git a/tests/lib/connector/sabre/aborteduploaddetectionplugin.php b/tests/lib/connector/sabre/aborteduploaddetectionplugin.php deleted file mode 100644 index 7e9f70ddcd..0000000000 --- a/tests/lib/connector/sabre/aborteduploaddetectionplugin.php +++ /dev/null @@ -1,100 +0,0 @@ - - * This file is licensed under the Affero General Public License version 3 or - * later. - * See the COPYING-README file. - */ -class Test_OC_Connector_Sabre_AbortedUploadDetectionPlugin extends PHPUnit_Framework_TestCase { - - /** - * @var \Sabre\DAV\Server - */ - private $server; - - /** - * @var OC_Connector_Sabre_AbortedUploadDetectionPlugin - */ - private $plugin; - - private function init($view) { - $this->server = new \Sabre\DAV\Server(); - $this->plugin = new OC_Connector_Sabre_AbortedUploadDetectionPlugin($view); - $this->plugin->initialize($this->server); - } - - /** - * @dataProvider lengthProvider - */ - public function testLength($expected, $headers) { - $this->init(null); - - $this->server->httpRequest = new \Sabre\HTTP\Request($headers); - $length = $this->plugin->getLength(); - $this->assertEquals($expected, $length); - } - - /** - * @dataProvider verifyContentLengthProvider - */ - public function testVerifyContentLength($method, $fileSize, $headers) { - $this->init($this->buildFileViewMock($fileSize)); - - $headers['REQUEST_METHOD'] = $method; - $this->server->httpRequest = new Sabre\HTTP\Request($headers); - $this->plugin->verifyContentLength('foo.txt'); - $this->assertTrue(true); - } - - /** - * @dataProvider verifyContentLengthFailedProvider - * @expectedException \Sabre\DAV\Exception\BadRequest - */ - public function testVerifyContentLengthFailed($method, $fileSize, $headers) { - $view = $this->buildFileViewMock($fileSize); - $this->init($view); - // we expect unlink to be called - $view->expects($this->once())->method('unlink'); - - $headers['REQUEST_METHOD'] = $method; - $this->server->httpRequest = new Sabre\HTTP\Request($headers); - $this->plugin->verifyContentLength('foo.txt'); - } - - public function verifyContentLengthProvider() { - return array( - array('PUT', 1024, array()), - array('PUT', 1024, array('HTTP_X_EXPECTED_ENTITY_LENGTH' => '1024')), - array('PUT', 512, array('HTTP_CONTENT_LENGTH' => '512')), - array('LOCK', 1024, array()), - array('LOCK', 1024, array('HTTP_X_EXPECTED_ENTITY_LENGTH' => '1024')), - array('LOCK', 512, array('HTTP_CONTENT_LENGTH' => '512')), - ); - } - - public function verifyContentLengthFailedProvider() { - return array( - array('PUT', 1025, array('HTTP_X_EXPECTED_ENTITY_LENGTH' => '1024')), - array('PUT', 525, array('HTTP_CONTENT_LENGTH' => '512')), - ); - } - - public function lengthProvider() { - return array( - array(null, array()), - array(1024, array('HTTP_X_EXPECTED_ENTITY_LENGTH' => '1024')), - array(512, array('HTTP_CONTENT_LENGTH' => '512')), - array(2048, array('HTTP_X_EXPECTED_ENTITY_LENGTH' => '2048', 'HTTP_CONTENT_LENGTH' => '1024')), - ); - } - - private function buildFileViewMock($fileSize) { - // mock filesystem - $view = $this->getMock('\OC\Files\View', array('filesize', 'unlink'), array(), '', false); - $view->expects($this->any())->method('filesize')->withAnyParameters()->will($this->returnValue($fileSize)); - - return $view; - } - -} diff --git a/tests/lib/connector/sabre/file.php b/tests/lib/connector/sabre/file.php index 3dd5b328f4..ba4e775813 100644 --- a/tests/lib/connector/sabre/file.php +++ b/tests/lib/connector/sabre/file.php @@ -29,7 +29,7 @@ class Test_OC_Connector_Sabre_File extends PHPUnit_Framework_TestCase { $file = new OC_Connector_Sabre_File($view, $info); // action - $etag = $file->put('test data'); + $file->put('test data'); } /** @@ -37,7 +37,9 @@ class Test_OC_Connector_Sabre_File extends PHPUnit_Framework_TestCase { */ public function testSimplePutFailsOnRename() { // setup - $view = $this->getMock('\OC\Files\View', array('file_put_contents', 'rename', 'getRelativePath'), array(), '', false); + $view = $this->getMock('\OC\Files\View', + array('file_put_contents', 'rename', 'getRelativePath', 'filesize'), + array(), '', false); $view->expects($this->any()) ->method('file_put_contents') ->withAnyParameters() @@ -46,10 +48,14 @@ class Test_OC_Connector_Sabre_File extends PHPUnit_Framework_TestCase { ->method('rename') ->withAnyParameters() ->will($this->returnValue(false)); - $view->expects($this->any()) ->method('getRelativePath') ->will($this->returnValue('/test.txt')); + $view->expects($this->any()) + ->method('filesize') + ->will($this->returnValue(123456)); + + $_SERVER['CONTENT_LENGTH'] = 123456; $info = new \OC\Files\FileInfo('/test.txt', null, null, array( 'permissions' => \OCP\PERMISSION_ALL @@ -58,7 +64,7 @@ class Test_OC_Connector_Sabre_File extends PHPUnit_Framework_TestCase { $file = new OC_Connector_Sabre_File($view, $info); // action - $etag = $file->put('test data'); + $file->put('test data'); } /** @@ -81,7 +87,7 @@ class Test_OC_Connector_Sabre_File extends PHPUnit_Framework_TestCase { $file = new OC_Connector_Sabre_File($view, $info); // action - $etag = $file->put('test data'); + $file->put('test data'); } /** @@ -102,4 +108,39 @@ class Test_OC_Connector_Sabre_File extends PHPUnit_Framework_TestCase { $file = new OC_Connector_Sabre_File($view, $info); $file->setName('/super*star.txt'); } + + /** + * @expectedException \Sabre\DAV\Exception\BadRequest + */ + public function testUploadAbort() { + // setup + $view = $this->getMock('\OC\Files\View', + array('file_put_contents', 'rename', 'getRelativePath', 'filesize'), + array(), '', false); + $view->expects($this->any()) + ->method('file_put_contents') + ->withAnyParameters() + ->will($this->returnValue(true)); + $view->expects($this->any()) + ->method('rename') + ->withAnyParameters() + ->will($this->returnValue(false)); + $view->expects($this->any()) + ->method('getRelativePath') + ->will($this->returnValue('/test.txt')); + $view->expects($this->any()) + ->method('filesize') + ->will($this->returnValue(123456)); + + $_SERVER['CONTENT_LENGTH'] = 12345; + + $info = new \OC\Files\FileInfo('/test.txt', null, null, array( + 'permissions' => \OCP\PERMISSION_ALL + )); + + $file = new OC_Connector_Sabre_File($view, $info); + + // action + $file->put('test data'); + } }