Merge pull request #17104 from owncloud/chunked-upload-locking

locking for chunked dav upload

4 files changed, 104 insertions, 13 deletions

diff --git a/apps/dav/lib/connector/sabre/file.php b/apps/dav/lib/connector/sabre/file.php
index 9e515cdc687..961532daf50 100644
--- a/apps/dav/lib/connector/sabre/file.php
+++ b/apps/dav/lib/connector/sabre/file.php
@@ -349,6 +349,7 @@ class File extends Node implements IFile {
 		if (empty($info)) {
 			throw new NotImplemented('Invalid chunk name');
 		}
+
 		$chunk_handler = new \OC_FileChunking($info);
 		$bytesWritten = $chunk_handler->store($info['index'], $data);
 
@@ -376,15 +377,17 @@ class File extends Node implements IFile {
 			$exists = $this->fileView->file_exists($targetPath);
 
 			try {
-				$this->emitPreHooks($exists, $targetPath);
+				$this->fileView->lockFile($targetPath, ILockingProvider::LOCK_SHARED);
 
-				$this->changeLock(ILockingProvider::LOCK_EXCLUSIVE);
+				$this->emitPreHooks($exists, $targetPath);
+				$this->fileView->changeLock($targetPath, ILockingProvider::LOCK_EXCLUSIVE);
+				/** @var \OC\Files\Storage\Storage $targetStorage */
+				list($targetStorage, $targetInternalPath) = $this->fileView->resolvePath($targetPath);
 
 				if ($needsPartFile) {
 					// we first assembly the target file as a part file
 					$partFile = $path . '/' . $info['name'] . '.ocTransferId' . $info['transferid'] . '.part';
-
-
+					/** @var \OC\Files\Storage\Storage $targetStorage */
 					list($partStorage, $partInternalPath) = $this->fileView->resolvePath($partFile);
 
 
@@ -392,8 +395,7 @@ class File extends Node implements IFile {
 
 					// here is the final atomic rename
 					$renameOkay = $targetStorage->moveFromStorage($partStorage, $partInternalPath, $targetInternalPath);
-
-					$fileExists = $this->fileView->file_exists($targetPath);
+					$fileExists = $targetStorage->file_exists($targetInternalPath);
 					if ($renameOkay === false || $fileExists === false) {
 						\OCP\Util::writeLog('webdav', '\OC\Files\Filesystem::rename() failed', \OCP\Util::ERROR);
 						// only delete if an error occurred and the target file was already created
@@ -403,7 +405,7 @@ class File extends Node implements IFile {
 							$partFile = null;
 							$targetStorage->unlink($targetInternalPath);
 						}
-						$this->changeLock(ILockingProvider::LOCK_SHARED);
+						$this->fileView->changeLock($targetPath, ILockingProvider::LOCK_SHARED);
 						throw new Exception('Could not rename part file assembled from chunks');
 					}
 				} else {
@@ -419,7 +421,7 @@ class File extends Node implements IFile {
 					}
 				}
 
-				$this->changeLock(ILockingProvider::LOCK_SHARED);
+				$this->fileView->changeLock($targetPath, ILockingProvider::LOCK_SHARED);
 
 				// since we skipped the view we need to scan and emit the hooks ourselves
 				$this->fileView->getUpdater()->update($targetPath);
@@ -427,6 +429,9 @@ class File extends Node implements IFile {
 				$this->emitPostHooks($exists, $targetPath);
 
 				$info = $this->fileView->getFileInfo($targetPath);
+
+				$this->fileView->unlockFile($targetPath, ILockingProvider::LOCK_SHARED);
+
 				return $info->getEtag();
 			} catch (\Exception $e) {
 				if ($partFile !== null) {
diff --git a/apps/dav/lib/connector/sabre/lockplugin.php b/apps/dav/lib/connector/sabre/lockplugin.php
index c564e066f8e..5840e59854c 100644
--- a/apps/dav/lib/connector/sabre/lockplugin.php
+++ b/apps/dav/lib/connector/sabre/lockplugin.php
@@ -62,7 +62,7 @@ class LockPlugin extends ServerPlugin {
 	public function getLock(RequestInterface $request) {
 		// we cant listen on 'beforeMethod:PUT' due to order of operations with setting up the tree
 		// so instead we limit ourselves to the PUT method manually
-		if ($request->getMethod() !== 'PUT') {
+		if ($request->getMethod() !== 'PUT' || isset($_SERVER['HTTP_OC_CHUNKED'])) {
 			return;
 		}
 		try {
@@ -80,7 +80,7 @@ class LockPlugin extends ServerPlugin {
 	}
 
 	public function releaseLock(RequestInterface $request) {
-		if ($request->getMethod() !== 'PUT') {
+		if ($request->getMethod() !== 'PUT' || isset($_SERVER['HTTP_OC_CHUNKED'])) {
 			return;
 		}
 		try {
diff --git a/apps/dav/tests/unit/connector/sabre/file.php b/apps/dav/tests/unit/connector/sabre/file.php
index 9171fc3b786..d874b7f33c2 100644
--- a/apps/dav/tests/unit/connector/sabre/file.php
+++ b/apps/dav/tests/unit/connector/sabre/file.php
@@ -200,7 +200,9 @@ class File extends \Test\TestCase {
 		$file = new \OCA\DAV\Connector\Sabre\File($view, $info);
 
 		// put first chunk
+		$file->acquireLock(ILockingProvider::LOCK_SHARED);
 		$this->assertNull($file->put('test data one'));
+		$file->releaseLock(ILockingProvider::LOCK_SHARED);
 
 		$info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-1', null, null, [
 			'permissions' => \OCP\Constants::PERMISSION_ALL
@@ -443,12 +445,12 @@ class File extends \Test\TestCase {
 		$thrown = false;
 		try {
 			// beforeMethod locks
-			$view->lockFile('/test.txt', ILockingProvider::LOCK_SHARED);
+			$file->acquireLock(ILockingProvider::LOCK_SHARED);
 
 			$file->put($this->getStream('test data'));
 
 			// afterMethod unlocks
-			$view->unlockFile('/test.txt', ILockingProvider::LOCK_SHARED);
+			$file->releaseLock(ILockingProvider::LOCK_SHARED);
 		} catch (\Sabre\DAV\Exception\BadRequest $e) {
 			$thrown = true;
 		}
@@ -505,7 +507,9 @@ class File extends \Test\TestCase {
 			'permissions' => \OCP\Constants::PERMISSION_ALL
 		], null);
 		$file = new \OCA\DAV\Connector\Sabre\File($view, $info);
+		$file->acquireLock(ILockingProvider::LOCK_SHARED);
 		$this->assertNull($file->put('test data one'));
+		$file->releaseLock(ILockingProvider::LOCK_SHARED);
 
 		$info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-1', null, null, [
 			'permissions' => \OCP\Constants::PERMISSION_ALL
@@ -515,7 +519,9 @@ class File extends \Test\TestCase {
 		// action
 		$thrown = false;
 		try {
+			$file->acquireLock(ILockingProvider::LOCK_SHARED);
 			$file->put($this->getStream('test data'));
+			$file->releaseLock(ILockingProvider::LOCK_SHARED);
 		} catch (\OCA\DAV\Connector\Sabre\Exception\FileLocked $e) {
 			$thrown = true;
 		}
diff --git a/apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php b/apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php
index 9a067f230a3..a2a8326f4ff 100644
--- a/apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php
+++ b/apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php
@@ -8,7 +8,9 @@
 
 namespace OCA\DAV\Tests\Unit\Connector\Sabre\RequestTest;
 
-use OC\AppFramework\Http;
+use OC\Connector\Sabre\Exception\FileLocked;
+use OCP\AppFramework\Http;
+use OCP\Lock\ILockingProvider;
 
 class UploadTest extends RequestTest {
 	public function testBasicUpload() {
@@ -43,6 +45,34 @@ class UploadTest extends RequestTest {
 		$this->assertEquals(3, $info->getSize());
 	}
 
+	/**
+	 * @expectedException \OCA\DAV\Connector\Sabre\Exception\FileLocked
+	 */
+	public function testUploadOverWriteReadLocked() {
+		$user = $this->getUniqueID();
+		$view = $this->setupUser($user, 'pass');
+
+		$view->file_put_contents('foo.txt', 'bar');
+
+		$view->lockFile('/foo.txt', ILockingProvider::LOCK_SHARED);
+
+		$this->request($view, $user, 'pass', 'PUT', '/foo.txt', 'asd');
+	}
+
+	/**
+	 * @expectedException \OCA\DAV\Connector\Sabre\Exception\FileLocked
+	 */
+	public function testUploadOverWriteWriteLocked() {
+		$user = $this->getUniqueID();
+		$view = $this->setupUser($user, 'pass');
+
+		$view->file_put_contents('foo.txt', 'bar');
+
+		$view->lockFile('/foo.txt', ILockingProvider::LOCK_EXCLUSIVE);
+
+		$this->request($view, $user, 'pass', 'PUT', '/foo.txt', 'asd');
+	}
+
 	public function testChunkedUpload() {
 		$user = $this->getUniqueID();
 		$view = $this->setupUser($user, 'pass');
@@ -107,4 +137,54 @@ class UploadTest extends RequestTest {
 		$this->assertInstanceOf('\OC\Files\FileInfo', $info);
 		$this->assertEquals(6, $info->getSize());
 	}
+
+	/**
+	 * @expectedException \OCA\DAV\Connector\Sabre\Exception\FileLocked
+	 */
+	public function testChunkedUploadOutOfOrderReadLocked() {
+		$user = $this->getUniqueID();
+		$view = $this->setupUser($user, 'pass');
+
+		$this->assertFalse($view->file_exists('foo.txt'));
+
+		$view->lockFile('/foo.txt', ILockingProvider::LOCK_SHARED);
+
+		try {
+			$response = $this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-1', 'bar', ['OC-Chunked' => '1']);
+		} catch (\OCA\DAV\Connector\Sabre\Exception\FileLocked $e) {
+			$this->fail('Didn\'t expect locked error for the first chunk on read lock');
+			return;
+		}
+
+		$this->assertEquals(Http::STATUS_CREATED, $response->getStatus());
+		$this->assertFalse($view->file_exists('foo.txt'));
+
+		// last chunk should trigger the locked error since it tries to assemble
+		$this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-0', 'asd', ['OC-Chunked' => '1']);
+	}
+
+	/**
+	 * @expectedException \OCA\DAV\Connector\Sabre\Exception\FileLocked
+	 */
+	public function testChunkedUploadOutOfOrderWriteLocked() {
+		$user = $this->getUniqueID();
+		$view = $this->setupUser($user, 'pass');
+
+		$this->assertFalse($view->file_exists('foo.txt'));
+
+		$view->lockFile('/foo.txt', ILockingProvider::LOCK_EXCLUSIVE);
+
+		try {
+			$response = $this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-1', 'bar', ['OC-Chunked' => '1']);
+		} catch (\OCA\DAV\Connector\Sabre\Exception\FileLocked $e) {
+			$this->fail('Didn\'t expect locked error for the first chunk on write lock'); // maybe forbid this in the future for write locks only?
+			return;
+		}
+
+		$this->assertEquals(Http::STATUS_CREATED, $response->getStatus());
+		$this->assertFalse($view->file_exists('foo.txt'));
+
+		// last chunk should trigger the locked error since it tries to assemble
+		$this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-0', 'asd', ['OC-Chunked' => '1']);
+	}
 }