Merge pull request #16912 from owncloud/webdav-smalltransferlockfix

Webdav PUT small file lock must be shared during hooks

4 files changed, 132 insertions, 3 deletions

diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php
index 3e1b29a4f28..5aef30cc577 100644
--- a/lib/private/connector/sabre/file.php
+++ b/lib/private/connector/sabre/file.php
@@ -114,7 +114,7 @@ class File extends Node implements IFile {
 		}
 
 		try {
-			$this->fileView->lockFile($this->path, ILockingProvider::LOCK_EXCLUSIVE);
+			$this->fileView->lockFile($this->path, ILockingProvider::LOCK_SHARED);
 		} catch (LockedException $e) {
 			throw new FileLocked($e->getMessage(), $e->getCode(), $e);
 		}
@@ -192,6 +192,12 @@ class File extends Node implements IFile {
 				));
 			}
 
+			try {
+				$this->fileView->changeLock($this->path, ILockingProvider::LOCK_EXCLUSIVE);
+			} catch (LockedException $e) {
+				throw new FileLocked($e->getMessage(), $e->getCode(), $e);
+			}
+
 			if ($needsPartFile) {
 				// rename to correct path
 				try {
@@ -213,6 +219,12 @@ class File extends Node implements IFile {
 			// since we skipped the view we need to scan and emit the hooks ourselves
 			$partStorage->getScanner()->scanFile($internalPath);
 
+			try {
+				$this->fileView->changeLock($this->path, ILockingProvider::LOCK_SHARED);
+			} catch (LockedException $e) {
+				throw new FileLocked($e->getMessage(), $e->getCode(), $e);
+			}
+
 			if ($view) {
 				$this->fileView->getUpdater()->propagate($hookPath);
 				if (!$exists) {
@@ -241,7 +253,7 @@ class File extends Node implements IFile {
 			throw new ServiceUnavailable("Failed to check file size: " . $e->getMessage());
 		}
 
-		$this->fileView->unlockFile($this->path, ILockingProvider::LOCK_EXCLUSIVE);
+		$this->fileView->unlockFile($this->path, ILockingProvider::LOCK_SHARED);
 
 		return '"' . $this->info->getEtag() . '"';
 	}
diff --git a/lib/private/files/view.php b/lib/private/files/view.php
index a206eab54e4..99db05f65c2 100644
--- a/lib/private/files/view.php
+++ b/lib/private/files/view.php
@@ -1683,12 +1683,14 @@ class View {
 	}
 
 	/**
+	 * Change the lock type
+	 *
 	 * @param string $path the path of the file to lock, relative to the view
 	 * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE
 	 * @return bool False if the path is excluded from locking, true otherwise
 	 * @throws \OCP\Lock\LockedException if the path is already locked
 	 */
-	private function changeLock($path, $type) {
+	public function changeLock($path, $type) {
 		$absolutePath = $this->getAbsolutePath($path);
 		if (!$this->shouldLockFile($absolutePath)) {
 			return false;
diff --git a/tests/lib/connector/sabre/file.php b/tests/lib/connector/sabre/file.php
index 6602a2df24f..834698d90cb 100644
--- a/tests/lib/connector/sabre/file.php
+++ b/tests/lib/connector/sabre/file.php
@@ -404,4 +404,84 @@ class File extends \Test\TestCase {
 			$params[Filesystem::signal_param_path]
 		);
 	}
+
+	/**
+	 * Test whether locks are set before and after the operation
+	 */
+	public function testPutLocking() {
+		$view = new \OC\Files\View('/' . $this->user . '/files/');
+
+		$path = 'test-locking.txt';
+		$info = new \OC\Files\FileInfo(
+			'/' . $this->user . '/files/' . $path,
+			null,
+			null,
+			['permissions' => \OCP\Constants::PERMISSION_ALL],
+			null
+		);
+
+		$file = new \OC\Connector\Sabre\File($view, $info);
+
+		$this->assertFalse(
+			$this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_SHARED),
+			'File unlocked before put'
+		);
+		$this->assertFalse(
+			$this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE),
+			'File unlocked before put'
+		);
+
+		$wasLockedPre = false;
+		$wasLockedPost = false;
+		$eventHandler = $this->getMockBuilder('\stdclass')
+			->setMethods(['writeCallback', 'postWriteCallback'])
+			->getMock();
+
+		// both pre and post hooks might need access to the file,
+		// so only shared lock is acceptable
+		$eventHandler->expects($this->once())
+			->method('writeCallback')
+			->will($this->returnCallback(
+				function() use ($view, $path, &$wasLockedPre){
+					$wasLockedPre = $this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_SHARED);
+					$wasLockedPre = $wasLockedPre && !$this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE);
+				}
+			));
+		$eventHandler->expects($this->once())
+			->method('postWriteCallback')
+			->will($this->returnCallback(
+				function() use ($view, $path, &$wasLockedPost){
+					$wasLockedPost = $this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_SHARED);
+					$wasLockedPost = $wasLockedPost && !$this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE);
+				}
+			));
+
+		\OCP\Util::connectHook(
+			Filesystem::CLASSNAME,
+			Filesystem::signal_write,
+			$eventHandler,
+			'writeCallback'
+		);
+		\OCP\Util::connectHook(
+			Filesystem::CLASSNAME,
+			Filesystem::signal_post_write,
+			$eventHandler,
+			'postWriteCallback'
+		);
+
+		$this->assertNotEmpty($file->put($this->getStream('test data')));
+
+		$this->assertTrue($wasLockedPre, 'File was locked during pre-hooks');
+		$this->assertTrue($wasLockedPost, 'File was locked during post-hooks');
+
+		$this->assertFalse(
+			$this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_SHARED),
+			'File unlocked after put'
+		);
+		$this->assertFalse(
+			$this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE),
+			'File unlocked after put'
+		);
+	}
+
 }
diff --git a/tests/lib/testcase.php b/tests/lib/testcase.php
index 8551edab71f..d385a903d1e 100644
--- a/tests/lib/testcase.php
+++ b/tests/lib/testcase.php
@@ -242,4 +242,39 @@ abstract class TestCase extends \PHPUnit_Framework_TestCase {
 			\OC_Util::setupFS($user);
 		}
 	}
+
+	/**
+	 * Check if the given path is locked with a given type
+	 *
+	 * @param \OC\Files\View $view view
+	 * @param string $path path to check
+	 * @param int $type lock type
+	 *
+	 * @return boolean true if the file is locked with the
+	 * given type, false otherwise
+	 */
+	protected function isFileLocked($view, $path, $type) {
+		// Note: this seems convoluted but is necessary because
+		// the format of the lock key depends on the storage implementation
+		// (in our case mostly md5)
+
+		if ($type === \OCP\Lock\ILockingProvider::LOCK_SHARED) {
+			// to check if the file has a shared lock, try acquiring an exclusive lock
+			$checkType = \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE;
+		} else {
+			// a shared lock cannot be set if exclusive lock is in place
+			$checkType = \OCP\Lock\ILockingProvider::LOCK_SHARED;
+		}
+		try {
+			$view->lockFile($path, $checkType);
+			// no exception, which means the lock of $type is not set
+			// clean up
+			$view->unlockFile($path, $checkType);
+			return false;
+		} catch (\OCP\Lock\LockedException $e) {
+			// we could not acquire the counter-lock, which means
+			// the lock of $type was in place
+			return true;
+		}
+	}
 }