Merge pull request #9507 from owncloud/fix-9302-master

Upload abortion is now detected within the  OC_Connector_Sabre_File::put...

6 files changed, 60 insertions, 211 deletions

diff --git a/apps/files/appinfo/remote.php b/apps/files/appinfo/remote.php
index dd5c470431a..3ba25085bad 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 684edd97670..03e43967a40 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 b569f9a83c3..00000000000
--- a/lib/private/connector/sabre/aborteduploaddetectionplugin.php
+++ /dev/null
@@ -1,98 +0,0 @@
-<?php
-/**
- * Copyright (c) 2013 Thomas Müller <thomas.mueller@tmit.eu>
- * 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 7591cc5c066..ece6885f24f 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 7e9f70ddcd3..00000000000
--- a/tests/lib/connector/sabre/aborteduploaddetectionplugin.php
+++ /dev/null
@@ -1,100 +0,0 @@
-<?php
-
-/**
- * Copyright (c) 2013 Thomas Müller <thomas.mueller@tmit.eu>
- * 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 3dd5b328f46..ba4e775813b 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');
+	}
 }