Merge pull request #16501 from owncloud/enc_fix_move_versions_between_storages

[encryption] fix move versions between storages

lib/private/encryption/keys/storage.php

@@ -234,13 +234,18 @@ class Storage implements IStorage {
 
 		list($owner, $source) = $this->util->getUidAndFilename($source);
 		list(, $target) = $this->util->getUidAndFilename($target);
-		$systemWide = $this->util->isSystemWideMountPoint($target, $owner);
+		$systemWideSource = $this->util->isSystemWideMountPoint($source, $owner);
+		$systemWideTarget = $this->util->isSystemWideMountPoint($target, $owner);
 
-		if ($systemWide) {
+		if ($systemWideSource) {
 			$sourcePath = $this->keys_base_dir . $source . '/';
-			$targetPath = $this->keys_base_dir . $target . '/';
 		} else {
 			$sourcePath = '/' . $owner . $this->keys_base_dir . $source . '/';
+		}
+
+		if ($systemWideTarget) {
+			$targetPath = $this->keys_base_dir . $target . '/';
+		} else {
 			$targetPath = '/' . $owner . $this->keys_base_dir . $target . '/';
 		}
 
@@ -265,13 +270,17 @@ class Storage implements IStorage {
 
 		list($owner, $source) = $this->util->getUidAndFilename($source);
 		list(, $target) = $this->util->getUidAndFilename($target);
-		$systemWide = $this->util->isSystemWideMountPoint($target, $owner);
+		$systemWideTarget = $this->util->isSystemWideMountPoint($target, $owner);
+		$systemWideSource = $this->util->isSystemWideMountPoint($source, $owner);
 
-		if ($systemWide) {
+		if ($systemWideSource) {
 			$sourcePath = $this->keys_base_dir . $source . '/';
-			$targetPath = $this->keys_base_dir . $target . '/';
 		} else {
 			$sourcePath = '/' . $owner . $this->keys_base_dir . $source . '/';
+		}
+		if ($systemWideTarget) {
+			$targetPath = $this->keys_base_dir . $target . '/';
+		} else {
 			$targetPath = '/' . $owner . $this->keys_base_dir . $target . '/';
 		}
 

lib/private/encryption/manager.php

@@ -26,6 +26,7 @@ use OC\Files\Filesystem;
 use OC\Files\Storage\Shared;
 use OC\Files\Storage\Wrapper\Encryption;
 use OC\Files\View;
+use OC\Search\Provider\File;
 use OCP\Encryption\IEncryptionModule;
 use OCP\Encryption\IManager;
 use OCP\Files\Mount\IMountPoint;
@@ -224,6 +225,7 @@ class Manager implements IManager {
 				);
 				$user = \OC::$server->getUserSession()->getUser();
 				$logger = \OC::$server->getLogger();
+				$mountManager = Filesystem::getMountManager();
 				$uid = $user ? $user->getUID() : null;
 				$fileHelper = \OC::$server->getEncryptionFilesHelper();
 				$keyStorage = \OC::$server->getEncryptionKeyStorage();
@@ -243,7 +245,8 @@ class Manager implements IManager {
 					$fileHelper,
 					$uid,
 					$keyStorage,
-					$update
+					$update,
+					$mountManager
 				);
 			} else {
 				return $storage;

lib/private/files/storage/wrapper/encryption.php

@@ -26,11 +26,13 @@ use OC\Encryption\Exceptions\ModuleDoesNotExistsException;
 use OC\Encryption\Update;
 use OC\Encryption\Util;
 use OC\Files\Filesystem;
+use OC\Files\Mount\Manager;
 use OC\Files\Storage\LocalTempFileTrait;
 use OCP\Encryption\IFile;
 use OCP\Encryption\IManager;
 use OCP\Encryption\Keys\IStorage;
 use OCP\Files\Mount\IMountPoint;
+use OCP\Files\Storage;
 use OCP\ILogger;
 
 class Encryption extends Wrapper {
@@ -68,6 +70,9 @@ class Encryption extends Wrapper {
 	/** @var Update */
 	private $update;
 
+	/** @var Manager */
+	private $mountManager;
+
 	/**
 	 * @param array $parameters
 	 * @param IManager $encryptionManager
@@ -77,6 +82,7 @@ class Encryption extends Wrapper {
 	 * @param string $uid
 	 * @param IStorage $keyStorage
 	 * @param Update $update
+	 * @param Manager $mountManager
 	 */
 	public function __construct(
 			$parameters,
@@ -86,9 +92,10 @@ class Encryption extends Wrapper {
 			IFile $fileHelper = null,
 			$uid = null,
 			IStorage $keyStorage = null,
-			Update $update = null
+			Update $update = null,
+			Manager $mountManager = null
 		) {
-
+		
 		$this->mountPoint = $parameters['mountPoint'];
 		$this->mount = $parameters['mount'];
 		$this->encryptionManager = $encryptionManager;
@@ -99,6 +106,7 @@ class Encryption extends Wrapper {
 		$this->keyStorage = $keyStorage;
 		$this->unencryptedSize = array();
 		$this->update = $update;
+		$this->mountManager = $mountManager;
 		parent::__construct($parameters);
 	}
 
@@ -289,34 +297,32 @@ class Encryption extends Wrapper {
 	 */
 	public function copy($path1, $path2) {
 
-		$fullPath1 = $this->getFullPath($path1);
-		$fullPath2 = $this->getFullPath($path2);
+		$source = $this->getFullPath($path1);
+		$target = $this->getFullPath($path2);
 
-		if ($this->util->isExcluded($fullPath1)) {
+		if ($this->util->isExcluded($source)) {
 			return $this->storage->copy($path1, $path2);
 		}
 
 		$result = $this->storage->copy($path1, $path2);
 
 		if ($result && $this->encryptionManager->isEnabled()) {
-			$source = $this->getFullPath($path1);
-			if (!$this->util->isExcluded($source)) {
-				$target = $this->getFullPath($path2);
-				$keysCopied = $this->keyStorage->copyKeys($source, $target);
-				if ($keysCopied &&
-					dirname($source) !== dirname($target) &&
-					$this->util->isFile($target)
-				) {
-					$this->update->update($target);
-				}
+			$keysCopied = $this->copyKeys($source, $target);
+
+			if ($keysCopied &&
+				dirname($source) !== dirname($target) &&
+				$this->util->isFile($target)
+			) {
+				$this->update->update($target);
 			}
+
 			$data = $this->getMetaData($path1);
 
 			if (isset($data['encrypted'])) {
 				$this->getCache()->put($path2, ['encrypted' => $data['encrypted']]);
 			}
 			if (isset($data['size'])) {
-				$this->updateUnencryptedSize($fullPath2, $data['size']);
+				$this->updateUnencryptedSize($target, $data['size']);
 			}
 		}
 
@@ -412,17 +418,18 @@ class Encryption extends Wrapper {
 	}
 
 	/**
-	 * @param \OCP\Files\Storage $sourceStorage
+	 * @param Storage $sourceStorage
 	 * @param string $sourceInternalPath
 	 * @param string $targetInternalPath
 	 * @param bool $preserveMtime
 	 * @return bool
 	 */
-	public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime = true) {
+	public function moveFromStorage(Storage $sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime = true) {
 
 		// TODO clean this up once the underlying moveFromStorage in OC\Files\Storage\Wrapper\Common is fixed:
 		// - call $this->storage->moveFromStorage() instead of $this->copyBetweenStorage
 		// - copy the file cache update from  $this->copyBetweenStorage to this method
+		// - copy the copyKeys() call from  $this->copyBetweenStorage to this method
 		// - remove $this->copyBetweenStorage
 
 		$result = $this->copyBetweenStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime, true);
@@ -438,17 +445,18 @@ class Encryption extends Wrapper {
 
 
 	/**
-	 * @param \OCP\Files\Storage $sourceStorage
+	 * @param Storage $sourceStorage
 	 * @param string $sourceInternalPath
 	 * @param string $targetInternalPath
 	 * @param bool $preserveMtime
 	 * @return bool
 	 */
-	public function copyFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime = false) {
+	public function copyFromStorage(Storage $sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime = false) {
 
 		// TODO clean this up once the underlying moveFromStorage in OC\Files\Storage\Wrapper\Common is fixed:
 		// - call $this->storage->moveFromStorage() instead of $this->copyBetweenStorage
 		// - copy the file cache update from  $this->copyBetweenStorage to this method
+		// - copy the copyKeys() call from  $this->copyBetweenStorage to this method
 		// - remove $this->copyBetweenStorage
 
 		return $this->copyBetweenStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime, false);
@@ -457,14 +465,27 @@ class Encryption extends Wrapper {
 	/**
 	 * copy file between two storages
 	 *
-	 * @param \OCP\Files\Storage $sourceStorage
+	 * @param Storage $sourceStorage
 	 * @param string $sourceInternalPath
 	 * @param string $targetInternalPath
 	 * @param bool $preserveMtime
 	 * @param bool $isRename
 	 * @return bool
 	 */
-	private function copyBetweenStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime, $isRename) {
+	private function copyBetweenStorage(Storage $sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime, $isRename) {
+
+		// first copy the keys that we reuse the existing file key on the target location
+		// and don't create a new one which would break versions for example.
+		$mount = $this->mountManager->findByStorageId($sourceStorage->getId());
+		if (count($mount) === 1) {
+			$mountPoint = $mount[0]->getMountPoint();
+			$source = $mountPoint . '/' . $sourceInternalPath;
+			$target = $this->getFullPath($targetInternalPath);
+			$this->copyKeys($source, $target);
+		} else {
+			$this->logger->error('Could not find mount point, can\'t keep encryption keys');
+		}
+
 		if ($sourceStorage->is_dir($sourceInternalPath)) {
 			$dh = $sourceStorage->opendir($sourceInternalPath);
 			$result = $this->mkdir($targetInternalPath);
@@ -620,4 +641,19 @@ class Encryption extends Wrapper {
 	public function updateUnencryptedSize($path, $unencryptedSize) {
 		$this->unencryptedSize[$path] = $unencryptedSize;
 	}
+
+	/**
+	 * copy keys to new location
+	 *
+	 * @param string $source path relative to data/
+	 * @param string $target path relative to data/
+	 * @return bool
+	 */
+	protected function copyKeys($source, $target) {
+		if (!$this->util->isExcluded($source)) {
+			return $this->keyStorage->copyKeys($source, $target);
+		}
+
+		return false;
+	}
 }

tests/lib/encryption/keys/storage.php

@@ -278,7 +278,7 @@ class StorageTest extends TestCase {
 	/**
 	 * @dataProvider dataProviderCopyRename
 	 */
-	public function testRenameKeys($source, $target, $systemWideMount, $expectedSource, $expectedTarget) {
+	public function testRenameKeys($source, $target, $systemWideMountSource, $systemWideMountTarget, $expectedSource, $expectedTarget) {
 		$this->view->expects($this->any())
 			->method('file_exists')
 			->willReturn(true);
@@ -296,7 +296,12 @@ class StorageTest extends TestCase {
 			->will($this->returnCallback(array($this, 'getUidAndFilenameCallback')));
 		$this->util->expects($this->any())
 			->method('isSystemWideMountPoint')
-			->willReturn($systemWideMount);
+			->willReturnCallback(function($path, $owner) use ($systemWideMountSource, $systemWideMountTarget) {
+				if(strpos($path, 'source.txt') !== false) {
+					return $systemWideMountSource;
+				}
+				return $systemWideMountTarget;
+			});
 
 		$this->storage->renameKeys($source, $target);
 	}
@@ -304,7 +309,7 @@ class StorageTest extends TestCase {
 	/**
 	 * @dataProvider dataProviderCopyRename
 	 */
-	public function testCopyKeys($source, $target, $systemWideMount, $expectedSource, $expectedTarget) {
+	public function testCopyKeys($source, $target, $systemWideMountSource, $systemWideMountTarget , $expectedSource, $expectedTarget) {
 		$this->view->expects($this->any())
 			->method('file_exists')
 			->willReturn(true);
@@ -322,7 +327,12 @@ class StorageTest extends TestCase {
 			->will($this->returnCallback(array($this, 'getUidAndFilenameCallback')));
 		$this->util->expects($this->any())
 			->method('isSystemWideMountPoint')
-			->willReturn($systemWideMount);
+			->willReturnCallback(function($path, $owner) use ($systemWideMountSource, $systemWideMountTarget) {
+				if(strpos($path, 'source.txt') !== false) {
+					return $systemWideMountSource;
+				}
+				return $systemWideMountTarget;
+			});
 
 		$this->storage->copyKeys($source, $target);
 	}
@@ -338,14 +348,20 @@ class StorageTest extends TestCase {
 
 	public function dataProviderCopyRename() {
 		return array(
-			array('/user1/files/foo.txt', '/user1/files/bar.txt', false,
-				'/user1/files_encryption/keys/files/foo.txt/', '/user1/files_encryption/keys/files/bar.txt/'),
-				array('/user1/files/foo/foo.txt', '/user1/files/bar.txt', false,
-				'/user1/files_encryption/keys/files/foo/foo.txt/', '/user1/files_encryption/keys/files/bar.txt/'),
-			array('/user1/files/foo.txt', '/user1/files/foo/bar.txt', false,
-				'/user1/files_encryption/keys/files/foo.txt/', '/user1/files_encryption/keys/files/foo/bar.txt/'),
-			array('/user1/files/foo.txt', '/user1/files/foo/bar.txt', true,
-				'/files_encryption/keys/files/foo.txt/', '/files_encryption/keys/files/foo/bar.txt/'),
+			array('/user1/files/source.txt', '/user1/files/target.txt', false, false,
+				'/user1/files_encryption/keys/files/source.txt/', '/user1/files_encryption/keys/files/target.txt/'),
+			array('/user1/files/foo/source.txt', '/user1/files/target.txt', false, false,
+				'/user1/files_encryption/keys/files/foo/source.txt/', '/user1/files_encryption/keys/files/target.txt/'),
+			array('/user1/files/source.txt', '/user1/files/foo/target.txt', false, false,
+				'/user1/files_encryption/keys/files/source.txt/', '/user1/files_encryption/keys/files/foo/target.txt/'),
+			array('/user1/files/source.txt', '/user1/files/foo/target.txt', true, true,
+				'/files_encryption/keys/files/source.txt/', '/files_encryption/keys/files/foo/target.txt/'),
+			array('/user1/files/source.txt', '/user1/files/target.txt', false, true,
+				'/user1/files_encryption/keys/files/source.txt/', '/files_encryption/keys/files/target.txt/'),
+			array('/user1/files/source.txt', '/user1/files/target.txt', true, false,
+				'/files_encryption/keys/files/source.txt/', '/user1/files_encryption/keys/files/target.txt/'),
+
+
 		);
 	}
 

tests/lib/files/storage/wrapper/encryption.php

@@ -63,6 +63,11 @@ class Encryption extends \Test\Files\Storage\Storage {
 	 */
 	private $mount;
 
+	/**
+	 * @var \OC\Files\Mount\Manager | \PHPUnit_Framework_MockObject_MockObject
+	 */
+	private $mountManager;
+
 	/** @var  integer dummy unencrypted size */
 	private $dummySize = -1;
 
@@ -86,7 +91,7 @@ class Encryption extends \Test\Files\Storage\Storage {
 			->disableOriginalConstructor()
 			->getMock();
 
-		$this->util = $this->getMock('\OC\Encryption\Util', ['getUidAndFilename', 'isFile'], [new View(), new \OC\User\Manager(), $groupManager, $config]);
+		$this->util = $this->getMock('\OC\Encryption\Util', ['getUidAndFilename', 'isFile', 'isExcluded'], [new View(), new \OC\User\Manager(), $groupManager, $config]);
 		$this->util->expects($this->any())
 			->method('getUidAndFilename')
 			->willReturnCallback(function ($path) {
@@ -119,7 +124,10 @@ class Encryption extends \Test\Files\Storage\Storage {
 			->disableOriginalConstructor()->getMock();
 		$this->cache->expects($this->any())
 			->method('get')
-			->willReturn(['encrypted' => false]);
+			->willReturnCallback(function($path) {return ['encrypted' => false, 'path' => $path];});
+
+		$this->mountManager = $this->getMockBuilder('\OC\Files\Mount\Manager')
+			->disableOriginalConstructor()->getMock();
 
 		$this->instance = $this->getMockBuilder('\OC\Files\Storage\Wrapper\Encryption')
 			->setConstructorArgs(
@@ -130,7 +138,7 @@ class Encryption extends \Test\Files\Storage\Storage {
 						'mountPoint' => '/',
 						'mount' => $this->mount
 					],
-					$this->encryptionManager, $this->util, $this->logger, $this->file, null, $this->keyStore, $this->update
+					$this->encryptionManager, $this->util, $this->logger, $this->file, null, $this->keyStore, $this->update, $this->mountManager
 				]
 			)
 			->setMethods(['getMetaData', 'getCache', 'getEncryptionModule'])
@@ -138,7 +146,9 @@ class Encryption extends \Test\Files\Storage\Storage {
 
 		$this->instance->expects($this->any())
 			->method('getMetaData')
-			->willReturn(['encrypted' => true, 'size' => $this->dummySize]);
+			->willReturnCallback(function ($path) {
+				return ['encrypted' => true, 'size' => $this->dummySize, 'path' => $path];
+			});
 
 		$this->instance->expects($this->any())
 			->method('getCache')
@@ -232,6 +242,8 @@ class Encryption extends \Test\Files\Storage\Storage {
 		}
 		$this->util->expects($this->any())
 			->method('isFile')->willReturn(true);
+		$this->util->expects($this->any())
+			->method('isExcluded')->willReturn(false);
 		$this->encryptionManager->expects($this->once())
 			->method('isEnabled')->willReturn($encryptionEnabled);
 		if ($shouldUpdate) {
@@ -324,4 +336,33 @@ class Encryption extends \Test\Files\Storage\Storage {
 			array('/file.txt', false, false, false),
 		);
 	}
+
+	/**
+	 * @dataProvider dataTestCopyKeys
+	 *
+	 * @param boolean $excluded
+	 * @param boolean $expected
+	 */
+	public function testCopyKeys($excluded, $expected) {
+		$this->util->expects($this->once())
+			->method('isExcluded')
+			->willReturn($excluded);
+
+		if ($excluded) {
+			$this->keyStore->expects($this->never())->method('copyKeys');
+		} else {
+			$this->keyStore->expects($this->once())->method('copyKeys')->willReturn(true);
+		}
+
+		$this->assertSame($expected,
+			\Test_Helper::invokePrivate($this->instance, 'copyKeys', ['/source', '/target'])
+		);
+	}
+
+	public function dataTestCopyKeys() {
+		return array(
+			array(true, false),
+			array(false, true),
+		);
+	}
 }