Merge pull request #17811 from owncloud/dav-lock-wide

Wrap the entire dav PUT in a read lock

6 files changed, 221 insertions, 62 deletions

diff --git a/lib/private/connector/sabre/directory.php b/lib/private/connector/sabre/directory.php
index 551176e4bd2..0261ab18047 100644
--- a/lib/private/connector/sabre/directory.php
+++ b/lib/private/connector/sabre/directory.php
@@ -30,6 +30,7 @@ namespace OC\Connector\Sabre;
 
 use OC\Connector\Sabre\Exception\InvalidPath;
 use OC\Connector\Sabre\Exception\FileLocked;
+use OCP\Lock\ILockingProvider;
 use OCP\Lock\LockedException;
 use Sabre\DAV\Exception\Locked;
 
@@ -110,6 +111,7 @@ class Directory extends \OC\Connector\Sabre\Node
 			// using a dummy FileInfo is acceptable here since it will be refreshed after the put is complete
 			$info = new \OC\Files\FileInfo($path, null, null, array(), null);
 			$node = new \OC\Connector\Sabre\File($this->fileView, $info);
+			$node->acquireLock(ILockingProvider::LOCK_SHARED);
 			return $node->put($data);
 		} catch (\OCP\Files\StorageNotAvailableException $e) {
 			throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage());
diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php
index b7d0c547f24..17659c96b07 100644
--- a/lib/private/connector/sabre/file.php
+++ b/lib/private/connector/sabre/file.php
@@ -35,6 +35,7 @@ namespace OC\Connector\Sabre;
 use OC\Connector\Sabre\Exception\EntityTooLarge;
 use OC\Connector\Sabre\Exception\FileLocked;
 use OC\Connector\Sabre\Exception\UnsupportedMediaType;
+use OC\Files\Filesystem;
 use OCP\Encryption\Exceptions\GenericEncryptionException;
 use OCP\Files\EntityTooLargeException;
 use OCP\Files\InvalidContentException;
@@ -114,12 +115,6 @@ class File extends Node implements IFile {
 			$partFilePath = $this->path;
 		}
 
-		try {
-			$this->fileView->lockFile($this->path, ILockingProvider::LOCK_SHARED);
-		} catch (LockedException $e) {
-			throw new FileLocked($e->getMessage(), $e->getCode(), $e);
-		}
-
 		// the part file and target file might be on a different storage in case of a single file storage (e.g. single file share)
 		/** @var \OC\Files\Storage\Storage $partStorage */
 		list($partStorage, $internalPartPath) = $this->fileView->resolvePath($partFilePath);
@@ -132,7 +127,7 @@ class File extends Node implements IFile {
 				// because we have no clue about the cause we can only throw back a 500/Internal Server Error
 				throw new Exception('Could not write file contents');
 			}
-			list($count, ) = \OC_Helper::streamCopy($data, $target);
+			list($count,) = \OC_Helper::streamCopy($data, $target);
 			fclose($target);
 
 			// if content length is sent by client:
@@ -154,29 +149,14 @@ class File extends Node implements IFile {
 
 		try {
 			$view = \OC\Files\Filesystem::getView();
-			$run = true;
 			if ($view) {
-				$hookPath = $view->getRelativePath($this->fileView->getAbsolutePath($this->path));
-
-				if (!$exists) {
-					\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_create, array(
-						\OC\Files\Filesystem::signal_param_path => $hookPath,
-						\OC\Files\Filesystem::signal_param_run => &$run,
-					));
-				} else {
-					\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_update, array(
-						\OC\Files\Filesystem::signal_param_path => $hookPath,
-						\OC\Files\Filesystem::signal_param_run => &$run,
-					));
-				}
-				\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_write, array(
-					\OC\Files\Filesystem::signal_param_path => $hookPath,
-					\OC\Files\Filesystem::signal_param_run => &$run,
-				));
+				$run = $this->emitPreHooks($exists);
+			} else {
+				$run = true;
 			}
 
 			try {
-				$this->fileView->changeLock($this->path, ILockingProvider::LOCK_EXCLUSIVE);
+				$this->changeLock(ILockingProvider::LOCK_EXCLUSIVE);
 			} catch (LockedException $e) {
 				if ($needsPartFile) {
 					$partStorage->unlink($internalPartPath);
@@ -202,7 +182,7 @@ class File extends Node implements IFile {
 			}
 
 			try {
-				$this->fileView->changeLock($this->path, ILockingProvider::LOCK_SHARED);
+				$this->changeLock(ILockingProvider::LOCK_SHARED);
 			} catch (LockedException $e) {
 				throw new FileLocked($e->getMessage(), $e->getCode(), $e);
 			}
@@ -211,18 +191,7 @@ class File extends Node implements IFile {
 			$this->fileView->getUpdater()->update($this->path);
 
 			if ($view) {
-				if (!$exists) {
-					\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_create, array(
-						\OC\Files\Filesystem::signal_param_path => $hookPath
-					));
-				} else {
-					\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_update, array(
-						\OC\Files\Filesystem::signal_param_path => $hookPath
-					));
-				}
-				\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_write, array(
-					\OC\Files\Filesystem::signal_param_path => $hookPath
-				));
+				$this->emitPostHooks($exists);
 			}
 
 			// allow sync clients to send the mtime along in a header
@@ -233,7 +202,6 @@ class File extends Node implements IFile {
 				}
 			}
 			$this->refreshInfo();
-			$this->fileView->unlockFile($this->path, ILockingProvider::LOCK_SHARED);
 		} catch (StorageNotAvailableException $e) {
 			throw new ServiceUnavailable("Failed to check file size: " . $e->getMessage());
 		}
@@ -241,6 +209,50 @@ class File extends Node implements IFile {
 		return '"' . $this->info->getEtag() . '"';
 	}
 
+	private function emitPreHooks($exists, $path = null) {
+		if (is_null($path)) {
+			$path = $this->path;
+		}
+		$hookPath = Filesystem::getView()->getRelativePath($this->fileView->getAbsolutePath($path));
+		$run = true;
+
+		if (!$exists) {
+			\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_create, array(
+				\OC\Files\Filesystem::signal_param_path => $hookPath,
+				\OC\Files\Filesystem::signal_param_run => &$run,
+			));
+		} else {
+			\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_update, array(
+				\OC\Files\Filesystem::signal_param_path => $hookPath,
+				\OC\Files\Filesystem::signal_param_run => &$run,
+			));
+		}
+		\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_write, array(
+			\OC\Files\Filesystem::signal_param_path => $hookPath,
+			\OC\Files\Filesystem::signal_param_run => &$run,
+		));
+		return $run;
+	}
+
+	private function emitPostHooks($exists, $path = null) {
+		if (is_null($path)) {
+			$path = $this->path;
+		}
+		$hookPath = Filesystem::getView()->getRelativePath($this->fileView->getAbsolutePath($path));
+		if (!$exists) {
+			\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_create, array(
+				\OC\Files\Filesystem::signal_param_path => $hookPath
+			));
+		} else {
+			\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_update, array(
+				\OC\Files\Filesystem::signal_param_path => $hookPath
+			));
+		}
+		\OC_Hook::emit(\OC\Files\Filesystem::CLASSNAME, \OC\Files\Filesystem::signal_post_write, array(
+			\OC\Files\Filesystem::signal_param_path => $hookPath
+		));
+	}
+
 	/**
 	 * Returns the data
 	 *
@@ -354,15 +366,30 @@ class File extends Node implements IFile {
 			$needsPartFile = $this->needsPartFile($storage);
 			$partFile = null;
 
+			$targetPath = $path . '/' . $info['name'];
+			/** @var \OC\Files\Storage\Storage $targetStorage */
+			list($targetStorage, $targetInternalPath) = $this->fileView->resolvePath($targetPath);
+
+			$exists = $this->fileView->file_exists($targetPath);
+
 			try {
-				$targetPath = $path . '/' . $info['name'];
+				$this->emitPreHooks($exists, $targetPath);
+
+				$this->changeLock(ILockingProvider::LOCK_EXCLUSIVE);
+
 				if ($needsPartFile) {
 					// we first assembly the target file as a part file
 					$partFile = $path . '/' . $info['name'] . '.ocTransferId' . $info['transferid'] . '.part';
-					$chunk_handler->file_assemble($partFile);
+
+
+					list($partStorage, $partInternalPath) = $this->fileView->resolvePath($partFile);
+
+
+					$chunk_handler->file_assemble($partStorage, $partInternalPath, $this->fileView->getAbsolutePath($targetPath));
 
 					// here is the final atomic rename
-					$renameOkay = $this->fileView->rename($partFile, $targetPath);
+					$renameOkay = $targetStorage->moveFromStorage($partStorage, $partInternalPath, $targetInternalPath);
+
 					$fileExists = $this->fileView->file_exists($targetPath);
 					if ($renameOkay === false || $fileExists === false) {
 						\OCP\Util::writeLog('webdav', '\OC\Files\Filesystem::rename() failed', \OCP\Util::ERROR);
@@ -371,28 +398,36 @@ class File extends Node implements IFile {
 							// set to null to avoid double-deletion when handling exception
 							// stray part file
 							$partFile = null;
-							$this->fileView->unlink($targetPath);
+							$targetStorage->unlink($targetInternalPath);
 						}
+						$this->changeLock(ILockingProvider::LOCK_SHARED);
 						throw new Exception('Could not rename part file assembled from chunks');
 					}
 				} else {
 					// assemble directly into the final file
-					$chunk_handler->file_assemble($targetPath);
+					$chunk_handler->file_assemble($targetStorage, $targetInternalPath, $this->fileView->getAbsolutePath($targetPath));
 				}
 
 				// allow sync clients to send the mtime along in a header
 				$request = \OC::$server->getRequest();
 				if (isset($request->server['HTTP_X_OC_MTIME'])) {
-					if ($this->fileView->touch($targetPath, $request->server['HTTP_X_OC_MTIME'])) {
+					if ($targetStorage->touch($targetInternalPath, $request->server['HTTP_X_OC_MTIME'])) {
 						header('X-OC-MTime: accepted');
 					}
 				}
 
+				$this->changeLock(ILockingProvider::LOCK_SHARED);
+
+				// since we skipped the view we need to scan and emit the hooks ourselves
+				$this->fileView->getUpdater()->update($targetPath);
+
+				$this->emitPostHooks($exists, $targetPath);
+
 				$info = $this->fileView->getFileInfo($targetPath);
 				return $info->getEtag();
 			} catch (\Exception $e) {
 				if ($partFile !== null) {
-					$this->fileView->unlink($partFile);
+					$targetStorage->unlink($targetInternalPath);
 				}
 				$this->convertToSabreException($e);
 			}
diff --git a/lib/private/connector/sabre/lockplugin.php b/lib/private/connector/sabre/lockplugin.php
new file mode 100644
index 00000000000..a3a7bb84e39
--- /dev/null
+++ b/lib/private/connector/sabre/lockplugin.php
@@ -0,0 +1,97 @@
+<?php
+/**
+ * @author Robin Appelman <icewind@owncloud.com>
+ *
+ * @copyright Copyright (c) 2015, ownCloud, Inc.
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OC\Connector\Sabre;
+
+use OC\Connector\Sabre\Exception\FileLocked;
+use OCP\Lock\ILockingProvider;
+use OCP\Lock\LockedException;
+use Sabre\DAV\Exception\NotFound;
+use \Sabre\DAV\PropFind;
+use \Sabre\DAV\PropPatch;
+use Sabre\DAV\ServerPlugin;
+use Sabre\DAV\Tree;
+use Sabre\HTTP\RequestInterface;
+use Sabre\HTTP\ResponseInterface;
+
+class LockPlugin extends ServerPlugin {
+	/**
+	 * Reference to main server object
+	 *
+	 * @var \Sabre\DAV\Server
+	 */
+	private $server;
+
+	/**
+	 * @var \Sabre\DAV\Tree
+	 */
+	private $tree;
+
+	/**
+	 * @param \Sabre\DAV\Tree $tree tree
+	 */
+	public function __construct(Tree $tree) {
+		$this->tree = $tree;
+	}
+
+	/**
+	 * {@inheritdoc}
+	 */
+	public function initialize(\Sabre\DAV\Server $server) {
+		$this->server = $server;
+		$this->server->on('beforeMethod', [$this, 'getLock'], 50);
+		$this->server->on('afterMethod', [$this, 'releaseLock'], 50);
+	}
+
+	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') {
+			return;
+		}
+		try {
+			$node = $this->tree->getNodeForPath($request->getPath());
+		} catch (NotFound $e) {
+			return;
+		}
+		if ($node instanceof Node) {
+			try {
+				$node->acquireLock(ILockingProvider::LOCK_SHARED);
+			} catch (LockedException $e) {
+				throw new FileLocked($e->getMessage(), $e->getCode(), $e);
+			}
+		}
+	}
+
+	public function releaseLock(RequestInterface $request) {
+		if ($request->getMethod() !== 'PUT') {
+			return;
+		}
+		try {
+			$node = $this->tree->getNodeForPath($request->getPath());
+		} catch (NotFound $e) {
+			return;
+		}
+		if ($node instanceof Node) {
+			$node->releaseLock(ILockingProvider::LOCK_SHARED);
+		}
+	}
+}
diff --git a/lib/private/connector/sabre/node.php b/lib/private/connector/sabre/node.php
index 7f4bf8ffed1..30faf9941bd 100644
--- a/lib/private/connector/sabre/node.php
+++ b/lib/private/connector/sabre/node.php
@@ -67,6 +67,7 @@ abstract class Node implements \Sabre\DAV\INode {
 
 	/**
 	 * Sets up the node, expects a full path name
+	 *
 	 * @param \OC\Files\View $view
 	 * @param \OCP\Files\FileInfo $info
 	 */
@@ -82,6 +83,7 @@ abstract class Node implements \Sabre\DAV\INode {
 
 	/**
 	 *  Returns the name of the node
+	 *
 	 * @return string
 	 */
 	public function getName() {
@@ -99,6 +101,7 @@ abstract class Node implements \Sabre\DAV\INode {
 
 	/**
 	 * Renames the node
+	 *
 	 * @param string $name The new name
 	 * @throws \Sabre\DAV\Exception\BadRequest
 	 * @throws \Sabre\DAV\Exception\Forbidden
@@ -131,6 +134,7 @@ abstract class Node implements \Sabre\DAV\INode {
 
 	/**
 	 * Returns the last modification time, as a unix timestamp
+	 *
 	 * @return int timestamp as integer
 	 */
 	public function getLastModified() {
@@ -212,7 +216,7 @@ abstract class Node implements \Sabre\DAV\INode {
 	 * @return string|null
 	 */
 	public function getDavPermissions() {
-		$p ='';
+		$p = '';
 		if ($this->info->isShared()) {
 			$p .= 'S';
 		}
@@ -248,4 +252,25 @@ abstract class Node implements \Sabre\DAV\INode {
 			throw new InvalidPath($ex->getMessage());
 		}
 	}
+
+	/**
+	 * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE
+	 */
+	public function acquireLock($type) {
+		$this->fileView->lockFile($this->path, $type);
+	}
+
+	/**
+	 * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE
+	 */
+	public function releaseLock($type) {
+		$this->fileView->unlockFile($this->path, $type);
+	}
+
+	/**
+	 * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE
+	 */
+	public function changeLock($type) {
+		$this->fileView->changeLock($this->path, $type);
+	}
 }
diff --git a/lib/private/connector/sabre/serverfactory.php b/lib/private/connector/sabre/serverfactory.php
index 525ff0104cd..86f60633541 100644
--- a/lib/private/connector/sabre/serverfactory.php
+++ b/lib/private/connector/sabre/serverfactory.php
@@ -70,6 +70,7 @@ class ServerFactory {
 		$server->addPlugin(new \OC\Connector\Sabre\FilesPlugin($objectTree));
 		$server->addPlugin(new \OC\Connector\Sabre\MaintenancePlugin($this->config));
 		$server->addPlugin(new \OC\Connector\Sabre\ExceptionLoggerPlugin('webdav', $this->logger));
+		$server->addPlugin(new \OC\Connector\Sabre\LockPlugin($objectTree));
 
 		// wait with registering these until auth is handled and the filesystem is setup
 		$server->on('beforeMethod', function () use ($server, $objectTree, $viewCallBack) {
diff --git a/lib/private/filechunking.php b/lib/private/filechunking.php
index 82bf61fa7b1..64399ad4366 100644
--- a/lib/private/filechunking.php
+++ b/lib/private/filechunking.php
@@ -178,27 +178,26 @@ class OC_FileChunking {
 	 * Assembles the chunks into the file specified by the path.
 	 * Also triggers the relevant hooks and proxies.
 	 *
-	 * @param string $path target path
+	 * @param \OC\Files\Storage\Storage $storage
+	 * @param string $path target path relative to the storage
+	 * @param string $absolutePath
+	 * @return bool assembled file size or false if file could not be created
 	 *
-	 * @return boolean assembled file size or false if file could not be created
-	 *
-	 * @throws \OC\InsufficientStorageException when file could not be fully
-	 * assembled due to lack of free space
+	 * @throws \OC\ServerNotAvailableException
 	 */
-	public function file_assemble($path) {
-		$absolutePath = \OC\Files\Filesystem::normalizePath(\OC\Files\Filesystem::getView()->getAbsolutePath($path));
+	public function file_assemble($storage, $path, $absolutePath) {
 		$data = '';
 		// use file_put_contents as method because that best matches what this function does
 		if (\OC\Files\Filesystem::isValidPath($path)) {
-			$path = \OC\Files\Filesystem::getView()->getRelativePath($absolutePath);
-			$exists = \OC\Files\Filesystem::file_exists($path);
+			$exists = $storage->file_exists($path);
 			$run = true;
+			$hookPath = \OC\Files\Filesystem::getView()->getRelativePath($absolutePath);
 			if(!$exists) {
 				OC_Hook::emit(
 					\OC\Files\Filesystem::CLASSNAME,
 					\OC\Files\Filesystem::signal_create,
 					array(
-						\OC\Files\Filesystem::signal_param_path => $path,
+						\OC\Files\Filesystem::signal_param_path => $hookPath,
 						\OC\Files\Filesystem::signal_param_run => &$run
 					)
 				);
@@ -207,14 +206,14 @@ class OC_FileChunking {
 				\OC\Files\Filesystem::CLASSNAME,
 				\OC\Files\Filesystem::signal_write,
 				array(
-					\OC\Files\Filesystem::signal_param_path => $path,
+					\OC\Files\Filesystem::signal_param_path => $hookPath,
 					\OC\Files\Filesystem::signal_param_run => &$run
 				)
 			);
 			if(!$run) {
 				return false;
 			}
-			$target = \OC\Files\Filesystem::fopen($path, 'w');
+			$target = $storage->fopen($path, 'w');
 			if($target) {
 				$count = $this->assemble($target);
 				fclose($target);
@@ -222,13 +221,13 @@ class OC_FileChunking {
 					OC_Hook::emit(
 						\OC\Files\Filesystem::CLASSNAME,
 						\OC\Files\Filesystem::signal_post_create,
-						array( \OC\Files\Filesystem::signal_param_path => $path)
+						array( \OC\Files\Filesystem::signal_param_path => $hookPath)
 					);
 				}
 				OC_Hook::emit(
 					\OC\Files\Filesystem::CLASSNAME,
 					\OC\Files\Filesystem::signal_post_write,
-					array( \OC\Files\Filesystem::signal_param_path => $path)
+					array( \OC\Files\Filesystem::signal_param_path => $hookPath)
 				);
 				return $count > 0;
 			}else{