Merge pull request #17189 from owncloud/files-straypartfilesonexception

Cleanup part file after upload exception

2 files changed, 347 insertions, 29 deletions

diff --git a/tests/lib/connector/sabre/file.php b/tests/lib/connector/sabre/file.php
index 834698d90cb..246f7f6d0a2 100644
--- a/tests/lib/connector/sabre/file.php
+++ b/tests/lib/connector/sabre/file.php
@@ -10,6 +10,7 @@ namespace Test\Connector\Sabre;
 
 use Test\HookHelper;
 use OC\Files\Filesystem;
+use OCP\Lock\ILockingProvider;
 
 class File extends \Test\TestCase {
 
@@ -33,6 +34,7 @@ class File extends \Test\TestCase {
 	public function tearDown() {
 		$userManager = \OC::$server->getUserManager();
 		$userManager->get($this->user)->delete();
+		unset($_SERVER['HTTP_OC_CHUNKED']);
 
 		parent::tearDown();
 	}
@@ -44,23 +46,92 @@ class File extends \Test\TestCase {
 		return $stream;
 	}
 
+
+	public function fopenFailuresProvider() {
+		return [
+			[
+				// return false
+				null,
+				'\Sabre\Dav\Exception',
+				false
+			],
+			[
+				new \OCP\Files\NotPermittedException(),
+				'Sabre\DAV\Exception\Forbidden'
+			],
+			[
+				new \OCP\Files\EntityTooLargeException(),
+				'OC\Connector\Sabre\Exception\EntityTooLarge'
+			],
+			[
+				new \OCP\Files\InvalidContentException(),
+				'OC\Connector\Sabre\Exception\UnsupportedMediaType'
+			],
+			[
+				new \OCP\Files\InvalidPathException(),
+				'Sabre\DAV\Exception\Forbidden'
+			],
+			[
+				new \OCP\Files\LockNotAcquiredException('/test.txt', 1),
+				'OC\Connector\Sabre\Exception\FileLocked'
+			],
+			[
+				new \OCP\Lock\LockedException('/test.txt'),
+				'OC\Connector\Sabre\Exception\FileLocked'
+			],
+			[
+				new \OCP\Encryption\Exceptions\GenericEncryptionException(),
+				'Sabre\DAV\Exception\ServiceUnavailable'
+			],
+			[
+				new \OCP\Files\StorageNotAvailableException(),
+				'Sabre\DAV\Exception\ServiceUnavailable'
+			],
+			[
+				new \Sabre\DAV\Exception('Generic sabre exception'),
+				'Sabre\DAV\Exception',
+				false
+			],
+			[
+				new \Exception('Generic exception'),
+				'Sabre\DAV\Exception'
+			],
+		];
+	}
+
 	/**
-	 * @expectedException \Sabre\DAV\Exception
+	 * @dataProvider fopenFailuresProvider
 	 */
-	public function testSimplePutFails() {
+	public function testSimplePutFails($thrownException, $expectedException, $checkPreviousClass = true) {
 		// setup
-		$storage = $this->getMock('\OC\Files\Storage\Local', ['fopen'], [['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]);
+		$storage = $this->getMock(
+			'\OC\Files\Storage\Local',
+			['fopen'],
+			[['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]
+		);
+		\OC\Files\Filesystem::mount($storage, [], $this->user . '/');
 		$view = $this->getMock('\OC\Files\View', array('getRelativePath', 'resolvePath'), array());
-		$view->expects($this->any())
+		$view->expects($this->atLeastOnce())
 			->method('resolvePath')
-			->will($this->returnValue(array($storage, '')));
-		$storage->expects($this->once())
-			->method('fopen')
-			->will($this->returnValue(false));
+			->will($this->returnCallback(
+				function($path) use ($storage){
+					return [$storage, $path];
+				}
+			));
+
+		if ($thrownException !== null) {
+			$storage->expects($this->once())
+				->method('fopen')
+				->will($this->throwException($thrownException));
+		} else {
+			$storage->expects($this->once())
+				->method('fopen')
+				->will($this->returnValue(false));
+		}
 
 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/test.txt'));
+			->will($this->returnArgument(0));
 
 		$info = new \OC\Files\FileInfo('/test.txt', null, null, array(
 			'permissions' => \OCP\Constants::PERMISSION_ALL
@@ -69,9 +140,97 @@ class File extends \Test\TestCase {
 		$file = new \OC\Connector\Sabre\File($view, $info);
 
 		// action
-		$file->put('test data');
+		$caughtException = null;
+		try {
+			$file->put('test data');
+		} catch (\Exception $e) {
+			$caughtException = $e;
+		}
+
+		$this->assertInstanceOf($expectedException, $caughtException);
+		if ($checkPreviousClass) {
+			$this->assertInstanceOf(get_class($thrownException), $caughtException->getPrevious());
+		}
+
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
 	}
 
+	/**
+	 * Test putting a file using chunking
+	 *
+	 * @dataProvider fopenFailuresProvider
+	 */
+	public function testChunkedPutFails($thrownException, $expectedException, $checkPreviousClass = false) {
+		// setup
+		$storage = $this->getMock(
+			'\OC\Files\Storage\Local',
+			['fopen'],
+			[['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]
+		);
+		\OC\Files\Filesystem::mount($storage, [], $this->user . '/');
+		$view = $this->getMock('\OC\Files\View', ['getRelativePath', 'resolvePath'], []);
+		$view->expects($this->atLeastOnce())
+			->method('resolvePath')
+			->will($this->returnCallback(
+				function($path) use ($storage){
+					return [$storage, $path];
+				}
+			));
+
+		if ($thrownException !== null) {
+			$storage->expects($this->once())
+				->method('fopen')
+				->will($this->throwException($thrownException));
+		} else {
+			$storage->expects($this->once())
+				->method('fopen')
+				->will($this->returnValue(false));
+		}
+
+		$view->expects($this->any())
+			->method('getRelativePath')
+			->will($this->returnArgument(0));
+
+		$_SERVER['HTTP_OC_CHUNKED'] = true;
+
+		$info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-0', null, null, [
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		], null);
+		$file = new \OC\Connector\Sabre\File($view, $info);
+
+		// put first chunk
+		$this->assertNull($file->put('test data one'));
+
+		$info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-1', null, null, [
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		], null);
+		$file = new \OC\Connector\Sabre\File($view, $info);
+
+		// action
+		$caughtException = null;
+		try {
+			// last chunk
+			$file->put('test data two');
+		} catch (\Exception $e) {
+			$caughtException = $e;
+		}
+
+		$this->assertInstanceOf($expectedException, $caughtException);
+		if ($checkPreviousClass) {
+			$this->assertInstanceOf(get_class($thrownException), $caughtException->getPrevious());
+		}
+
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
+	}
+
+	/**
+	 * Simulate putting a file to the given path.
+	 *
+	 * @param string $path path to put the file into
+	 * @param string $viewRoot root to use for the view
+	 *
+	 * @return result of the PUT operaiton which is usually the etag
+	 */
 	private function doPut($path, $viewRoot = null) {
 		$view = \OC\Files\Filesystem::getView();
 		if (!is_null($viewRoot)) {
@@ -90,14 +249,23 @@ class File extends \Test\TestCase {
 
 		$file = new \OC\Connector\Sabre\File($view, $info);
 
-		$this->assertNotEmpty($file->put($this->getStream('test data')));
+		return $file->put($this->getStream('test data'));
 	}
 
 	/**
 	 * Test putting a single file
 	 */
 	public function testPutSingleFile() {
-		$this->doPut('/foo.txt');
+		$this->assertNotEmpty($this->doPut('/foo.txt'));
+	}
+
+	/**
+	 * Test putting a file using chunking
+	 */
+	public function testChunkedPut() {
+		$_SERVER['HTTP_OC_CHUNKED'] = true;
+		$this->assertNull($this->doPut('/test.txt-chunking-12345-2-0'));
+		$this->assertNotEmpty($this->doPut('/test.txt-chunking-12345-2-1'));
 	}
 
 	/**
@@ -106,7 +274,7 @@ class File extends \Test\TestCase {
 	public function testPutSingleFileTriggersHooks() {
 		HookHelper::setUpHooks();
 
-		$this->doPut('/foo.txt');
+		$this->assertNotEmpty($this->doPut('/foo.txt'));
 
 		$this->assertCount(4, HookHelper::$hookCalls);
 		$this->assertHookCall(
@@ -140,7 +308,7 @@ class File extends \Test\TestCase {
 
 		HookHelper::setUpHooks();
 
-		$this->doPut('/foo.txt');
+		$this->assertNotEmpty($this->doPut('/foo.txt'));
 
 		$this->assertCount(4, HookHelper::$hookCalls);
 		$this->assertHookCall(
@@ -177,7 +345,7 @@ class File extends \Test\TestCase {
 		HookHelper::setUpHooks();
 
 		// happens with public webdav where the view root is the share root
-		$this->doPut('/foo.txt', '/' . $this->user . '/files/noderoot');
+		$this->assertNotEmpty($this->doPut('/foo.txt', '/' . $this->user . '/files/noderoot'));
 
 		$this->assertCount(4, HookHelper::$hookCalls);
 		$this->assertHookCall(
@@ -211,8 +379,6 @@ class File extends \Test\TestCase {
 
 	/**
 	 * Test put file with cancelled hook
-	 *
-	 * @expectedException \Sabre\DAV\Exception
 	 */
 	public function testPutSingleFileCancelPreHook() {
 		\OCP\Util::connectHook(
@@ -222,13 +388,22 @@ class File extends \Test\TestCase {
 			'cancellingCallback'
 		);
 
-		$this->doPut('/foo.txt');
+		// action
+		$thrown = false;
+		try {
+			$this->doPut('/foo.txt');
+		} catch (\Sabre\DAV\Exception $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles(), 'No stray part files');
 	}
 
 	/**
-	 * @expectedException \Sabre\DAV\Exception
+	 * Test exception when the uploaded size did not match
 	 */
-	public function testSimplePutFailsOnRename() {
+	public function testSimplePutFailsSizeCheck() {
 		// setup
 		$view = $this->getMock('\OC\Files\View',
 			array('rename', 'getRelativePath', 'filesize'));
@@ -238,7 +413,8 @@ class File extends \Test\TestCase {
 			->will($this->returnValue(false));
 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/test.txt'));
+			->will($this->returnArgument(0));
+
 		$view->expects($this->any())
 			->method('filesize')
 			->will($this->returnValue(123456));
@@ -253,18 +429,87 @@ class File extends \Test\TestCase {
 		$file = new \OC\Connector\Sabre\File($view, $info);
 
 		// action
-		$file->put($this->getStream('test data'));
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\Sabre\DAV\Exception\BadRequest $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
 	}
 
 	/**
-	 * @expectedException \OC\Connector\Sabre\Exception\InvalidPath
+	 * Test exception during final rename in simple upload mode
+	 */
+	public function testSimplePutFailsMoveFromStorage() {
+		$view = new \OC\Files\View('/' . $this->user . '/files');
+
+		// simulate situation where the target file is locked
+		$view->lockFile('/test.txt', ILockingProvider::LOCK_EXCLUSIVE);
+
+		$info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt', null, null, array(
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		), null);
+
+		$file = new \OC\Connector\Sabre\File($view, $info);
+
+		// action
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\OC\Connector\Sabre\Exception\FileLocked $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
+	}
+
+	/**
+	 * Test exception during final rename in chunk upload mode
+	 */
+	public function testChunkedPutFailsFinalRename() {
+		$view = new \OC\Files\View('/' . $this->user . '/files');
+
+		// simulate situation where the target file is locked
+		$view->lockFile('/test.txt', ILockingProvider::LOCK_EXCLUSIVE);
+
+		$_SERVER['HTTP_OC_CHUNKED'] = true;
+
+		$info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-0', null, null, [
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		], null);
+		$file = new \OC\Connector\Sabre\File($view, $info);
+		$this->assertNull($file->put('test data one'));
+
+		$info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-1', null, null, [
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		], null);
+		$file = new \OC\Connector\Sabre\File($view, $info);
+
+		// action
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\OC\Connector\Sabre\Exception\FileLocked $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
+	}
+
+	/**
+	 * Test put file with invalid chars
 	 */
 	public function testSimplePutInvalidChars() {
 		// setup
 		$view = $this->getMock('\OC\Files\View', array('getRelativePath'));
 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/*'));
+			->will($this->returnArgument(0));
 
 		$info = new \OC\Files\FileInfo('/*', null, null, array(
 			'permissions' => \OCP\Constants::PERMISSION_ALL
@@ -272,7 +517,15 @@ class File extends \Test\TestCase {
 		$file = new \OC\Connector\Sabre\File($view, $info);
 
 		// action
-		$file->put($this->getStream('test data'));
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\OC\Connector\Sabre\Exception\InvalidPath $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
 	}
 
 	/**
@@ -286,7 +539,7 @@ class File extends \Test\TestCase {
 
 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/*'));
+			->will($this->returnArgument(0));
 
 		$info = new \OC\Files\FileInfo('/*', null, null, array(
 			'permissions' => \OCP\Constants::PERMISSION_ALL
@@ -296,7 +549,6 @@ class File extends \Test\TestCase {
 	}
 
 	/**
-	 * @expectedException \Sabre\DAV\Exception\BadRequest
 	 */
 	public function testUploadAbort() {
 		// setup
@@ -308,7 +560,7 @@ class File extends \Test\TestCase {
 			->will($this->returnValue(false));
 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/test.txt'));
+			->will($this->returnArgument(0));
 		$view->expects($this->any())
 			->method('filesize')
 			->will($this->returnValue(123456));
@@ -323,7 +575,15 @@ class File extends \Test\TestCase {
 		$file = new \OC\Connector\Sabre\File($view, $info);
 
 		// action
-		$file->put($this->getStream('test data'));
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\Sabre\DAV\Exception\BadRequest $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
 	}
 
 	/**
@@ -484,4 +744,29 @@ class File extends \Test\TestCase {
 		);
 	}
 
+	/**
+	 * Returns part files in the given path
+	 *
+	 * @param \OC\Files\View view which root is the current user's "files" folder
+	 * @param string $path path for which to list part files
+	 *
+	 * @return array list of part files
+	 */
+	private function listPartFiles(\OC\Files\View $userView = null, $path = '') {
+		if ($userView === null) {
+			$userView = \OC\Files\Filesystem::getView();
+		}
+		$files = [];
+		list($storage, $internalPath) = $userView->resolvePath($path);
+		$realPath = $storage->getSourcePath($internalPath);
+		$dh = opendir($realPath);
+		while (($file = readdir($dh)) !== false) {
+			if (substr($file, strlen($file) - 5, 5) === '.part') {
+				$files[] = $file;
+			}
+		}
+		closedir($dh);
+		return $files;
+	}
+
 }
diff --git a/tests/lib/files/view.php b/tests/lib/files/view.php
index 52273c15f1a..382c033f19c 100644
--- a/tests/lib/files/view.php
+++ b/tests/lib/files/view.php
@@ -1758,6 +1758,39 @@ class View extends \Test\TestCase {
 		$this->assertNull($this->getFileLockType($view, $targetPath), 'Target file not locked after operation');
 	}
 
+	/**
+	 * Test rename operation: unlock first path when second path was locked
+	 */
+	public function testLockFileRenameUnlockOnException() {
+		$this->loginAsUser('test');
+
+		$view = new \OC\Files\View('/' . $this->user . '/files/');
+
+		$sourcePath = 'original.txt';
+		$targetPath = 'target.txt';
+		$view->file_put_contents($sourcePath, 'meh');
+
+		// simulate that the target path is already locked
+		$view->lockFile($targetPath, ILockingProvider::LOCK_EXCLUSIVE);
+
+		$this->assertNull($this->getFileLockType($view, $sourcePath), 'Source file not locked before operation');
+		$this->assertEquals(ILockingProvider::LOCK_EXCLUSIVE, $this->getFileLockType($view, $targetPath), 'Target file is locked before operation');
+
+		$thrown = false;
+		try {
+			$view->rename($sourcePath, $targetPath);
+		} catch (\OCP\Lock\LockedException $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown, 'LockedException thrown');
+
+		$this->assertNull($this->getFileLockType($view, $sourcePath), 'Source file not locked after operation');
+		$this->assertEquals(ILockingProvider::LOCK_EXCLUSIVE, $this->getFileLockType($view, $targetPath), 'Target file still locked after operation');
+
+		$view->unlockFile($targetPath, ILockingProvider::LOCK_EXCLUSIVE);
+	}
+
 	public function lockFileRenameOrCopyCrossStorageDataProvider() {
 		return [
 			['rename', 'moveFromStorage', ILockingProvider::LOCK_EXCLUSIVE],