Merge pull request #5447 from owncloud/fixing-5117-master

No data corruption duriing parallel upload

apps/files_encryption/lib/helper.php

@@ -157,6 +157,49 @@ class Helper {
 		return $return;
 	}
 
+	/**
+	 * @brief Check if a path is a .part file
+	 * @param string $path Path that may identify a .part file
+	 * @return bool
+	 */
+	public static function isPartialFilePath($path) {
+
+		$extension = pathinfo($path, PATHINFO_EXTENSION);
+		if ( $extension === 'part' || $extension === 'etmp') {
+			return true;
+		} else {
+			return false;
+		}
+
+	}
+
+
+	/**
+	 * @brief Remove .path extension from a file path
+	 * @param string $path Path that may identify a .part file
+	 * @return string File path without .part extension
+	 * @note this is needed for reusing keys
+	 */
+	public static function stripPartialFileExtension($path) {
+		$extension = pathinfo($path, PATHINFO_EXTENSION);
+
+		if ( $extension === 'part' || $extension === 'etmp') {
+
+			$newLength = strlen($path) - 5; // 5 = strlen(".part") = strlen(".etmp")
+			$fPath = substr($path, 0, $newLength);
+
+			// if path also contains a transaction id, we remove it too
+			$extension = pathinfo($fPath, PATHINFO_EXTENSION);
+			if(substr($extension, 0, 12) === 'ocTransferId') { // 12 = strlen("ocTransferId")
+				$newLength = strlen($fPath) - strlen($extension) -1;
+				$fPath = substr($fPath, 0, $newLength);
+			}
+			return $fPath;
+
+		} else {
+			return $path;
+		}
+	}
 
 	/**
 	 * @brief disable recovery

apps/files_encryption/lib/keymanager.php

@@ -152,10 +152,10 @@ class Keymanager {
 		}
 
 		// try reusing key file if part file
-		if (self::isPartialFilePath($targetPath)) {
+		if (Helper::isPartialFilePath($targetPath)) {
 
 			$result = $view->file_put_contents(
-				$basePath . '/' . self::fixPartialFilePath($targetPath) . '.key', $catfile);
+				$basePath . '/' . Helper::stripPartialFileExtension($targetPath) . '.key', $catfile);
 
 		} else {
 
@@ -169,48 +169,6 @@ class Keymanager {
 
 	}
 
-	/**
-	 * @brief Remove .path extension from a file path
-	 * @param string $path Path that may identify a .part file
-	 * @return string File path without .part extension
-	 * @note this is needed for reusing keys
-	 */
-	public static function fixPartialFilePath($path) {
-
-		if (preg_match('/\.part$/', $path) || preg_match('/\.etmp$/', $path)) {
-
-			$newLength = strlen($path) - 5;
-			$fPath = substr($path, 0, $newLength);
-
-			return $fPath;
-
-		} else {
-
-			return $path;
-
-		}
-
-	}
-
-	/**
-	 * @brief Check if a path is a .part file
-	 * @param string $path Path that may identify a .part file
-	 * @return bool
-	 */
-	public static function isPartialFilePath($path) {
-
-		if (preg_match('/\.part$/', $path) || preg_match('/\.etmp$/', $path)) {
-
-			return true;
-
-		} else {
-
-			return false;
-
-		}
-
-	}
-
 	/**
 	 * @brief retrieve keyfile for an encrypted file
 	 * @param \OC_FilesystemView $view
@@ -226,7 +184,7 @@ class Keymanager {
 		$util = new Util($view, \OCP\User::getUser());
 
 		list($owner, $filename) = $util->getUidAndFilename($filePath);
-		$filename = self::fixPartialFilePath($filename);
+		$filename = Helper::stripPartialFileExtension($filename);
 		$filePath_f = ltrim($filename, '/');
 
 		// in case of system wide mount points the keys are stored directly in the data directory
@@ -385,8 +343,8 @@ class Keymanager {
 		foreach ($shareKeys as $userId => $shareKey) {
 
 			// try reusing key file if part file
-			if (self::isPartialFilePath($shareKeyPath)) {
-				$writePath = $basePath . '/' . self::fixPartialFilePath($shareKeyPath) . '.' . $userId . '.shareKey';
+			if (Helper::isPartialFilePath($shareKeyPath)) {
+				$writePath = $basePath . '/' . Helper::stripPartialFileExtension($shareKeyPath) . '.' . $userId . '.shareKey';
 			} else {
 				$writePath = $basePath . '/' . $shareKeyPath . '.' . $userId . '.shareKey';
 			}
@@ -422,7 +380,7 @@ class Keymanager {
 		$util = new Util($view, \OCP\User::getUser());
 
 		list($owner, $filename) = $util->getUidAndFilename($filePath);
-		$filename = self::fixPartialFilePath($filename);
+		$filename = Helper::stripPartialFileExtension($filename);
 		// in case of system wide mount points the keys are stored directly in the data directory
 		if ($util->isSystemWideMountPoint($filename)) {
 			$shareKeyPath = '/files_encryption/share-keys/' . $filename . '.' . $userId . '.shareKey';

apps/files_encryption/lib/proxy.php

@@ -342,7 +342,7 @@ class Proxy extends \OC_FileProxy {
 
 		$fileInfo = false;
 		// get file info from database/cache if not .part file
-		if (!Keymanager::isPartialFilePath($path)) {
+		if (!Helper::isPartialFilePath($path)) {
 			$fileInfo = $view->getFileInfo($path);
 		}
 
@@ -353,7 +353,7 @@ class Proxy extends \OC_FileProxy {
 				$fixSize = $util->getFileSize($path);
 				$fileInfo['unencrypted_size'] = $fixSize;
 				// put file info if not .part file
-				if (!Keymanager::isPartialFilePath($relativePath)) {
+				if (!Helper::isPartialFilePath($relativePath)) {
 					$view->putFileInfo($path, $fileInfo);
 				}
 			}
@@ -372,7 +372,7 @@ class Proxy extends \OC_FileProxy {
 				$fileInfo['unencrypted_size'] = $size;
 
 				// put file info if not .part file
-				if (!Keymanager::isPartialFilePath($relativePath)) {
+				if (!Helper::isPartialFilePath($relativePath)) {
 					$view->putFileInfo($path, $fileInfo);
 				}
 			}

apps/files_encryption/lib/util.php

@@ -1145,10 +1145,7 @@ class Util {
 		// Make sure that a share key is generated for the owner too
 		list($owner, $ownerPath) = $this->getUidAndFilename($filePath);
 
-		$pathinfo = pathinfo($ownerPath);
-		if(array_key_exists('extension', $pathinfo) && $pathinfo['extension'] === 'part') {
-			$ownerPath = $pathinfo['dirname'] . '/' . $pathinfo['filename'];
-		}
+		$ownerPath = \OCA\Encryption\Helper::stripPartialFileExtension($ownerPath);
 
 		$userIds = array();
 		if ($sharingEnabled) {

apps/files_encryption/tests/helper.php

+<?php
+/**
+ * Copyright (c) 2013 Bjoern Schiessle <schiessle@owncloud.com>
+ * This file is licensed under the Affero General Public License version 3 or
+ * later.
+ * See the COPYING-README file.
+ */
+
+
+require_once __DIR__ . '/../lib/helper.php';
+
+use OCA\Encryption;
+
+/**
+ * Class Test_Encryption_Helper
+ */
+class Test_Encryption_Helper extends \PHPUnit_Framework_TestCase {
+
+	/**
+	 * @medium
+	 */
+	function testStripPartialFileExtension() {
+
+		$partFilename = 'testfile.txt.part';
+		$filename = 'testfile.txt';
+
+		$this->assertTrue(Encryption\Helper::isPartialFilePath($partFilename));
+
+		$this->assertEquals('testfile.txt', Encryption\Helper::stripPartialFileExtension($partFilename));
+
+		$this->assertFalse(Encryption\Helper::isPartialFilePath($filename));
+
+		$this->assertEquals('testfile.txt', Encryption\Helper::stripPartialFileExtension($filename));
+	}
+
+
+	/**
+	 * @medium
+	 */
+	function testStripPartialFileExtensionWithTransferIdPath() {
+
+		$partFilename = 'testfile.txt.ocTransferId643653835.part';
+		$filename = 'testfile.txt';
+
+		$this->assertTrue(Encryption\Helper::isPartialFilePath($partFilename));
+
+		$this->assertEquals('testfile.txt', Encryption\Helper::stripPartialFileExtension($partFilename));
+
+		$this->assertFalse(Encryption\Helper::isPartialFilePath($filename));
+
+		$this->assertEquals('testfile.txt', Encryption\Helper::stripPartialFileExtension($filename));
+	}
+
+}
\ No newline at end of file

apps/files_encryption/tests/keymanager.php

@@ -188,23 +188,6 @@ class Test_Encryption_Keymanager extends \PHPUnit_Framework_TestCase {
 		$this->assertArrayHasKey('key', $sslInfoPrivate);
 	}
 
-	/**
-	 * @medium
-	 */
-	function testFixPartialFilePath() {
-
-		$partFilename = 'testfile.txt.part';
-		$filename = 'testfile.txt';
-
-		$this->assertTrue(Encryption\Keymanager::isPartialFilePath($partFilename));
-
-		$this->assertEquals('testfile.txt', Encryption\Keymanager::fixPartialFilePath($partFilename));
-
-		$this->assertFalse(Encryption\Keymanager::isPartialFilePath($filename));
-
-		$this->assertEquals('testfile.txt', Encryption\Keymanager::fixPartialFilePath($filename));
-	}
-
 	/**
 	 * @medium
 	 */

lib/private/connector/sabre/file.php

@@ -230,9 +230,31 @@ class OC_Connector_Sabre_File extends OC_Connector_Sabre_Node implements Sabre_D
 		}
 
 		if ($chunk_handler->isComplete()) {
-			$newPath = $path . '/' . $info['name'];
-			$chunk_handler->file_assemble($newPath);
-			return OC_Connector_Sabre_Node::getETagPropertyForPath($newPath);
+
+			// we first assembly the target file as a part file
+			$partFile = $path . '/' . $info['name'] . '.ocTransferId' . $info['transferid'] . '.part';
+			$chunk_handler->file_assemble($partFile);
+
+			// here is the final atomic rename
+			$fs = $this->getFS();
+			$targetPath = $path . '/' . $info['name'];
+			$renameOkay = $fs->rename($partFile, $targetPath);
+			$fileExists = $fs->file_exists($targetPath);
+			if ($renameOkay === false || $fileExists === false) {
+				\OC_Log::write('webdav', '\OC\Files\Filesystem::rename() failed', \OC_Log::ERROR);
+				$fs->unlink($targetPath);
+				throw new Sabre_DAV_Exception();
+			}
+
+			// allow sync clients to send the mtime along in a header
+			$mtime = OC_Request::hasModificationTime();
+			if ($mtime !== false) {
+				if($fs->touch($this->path, $mtime)) {
+					header('X-OC-MTime: accepted');
+				}
+			}
+
+			return OC_Connector_Sabre_Node::getETagPropertyForPath($targetPath);
 		}
 
 		return null;