From 2990b0e07e418577d55368c21200ada86c381b51 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Thu, 23 Apr 2015 16:48:11 +0200 Subject: [PATCH 1/6] update share keys if a file is moved to a shared folder --- lib/private/encryption/file.php | 4 +- lib/private/encryption/manager.php | 20 ++- lib/private/encryption/update.php | 76 +++++++---- lib/private/encryption/util.php | 20 ++- .../files/storage/wrapper/encryption.php | 28 +++- tests/lib/encryption/updatetest.php | 129 ++++++++++++++++++ tests/lib/encryption/utiltest.php | 21 +++ .../lib/files/storage/wrapper/encryption.php | 59 +++++++- 8 files changed, 306 insertions(+), 51 deletions(-) create mode 100644 tests/lib/encryption/updatetest.php diff --git a/lib/private/encryption/file.php b/lib/private/encryption/file.php index 48cd0d1187b..5a7357b9e28 100644 --- a/lib/private/encryption/file.php +++ b/lib/private/encryption/file.php @@ -36,7 +36,7 @@ class File implements \OCP\Encryption\IFile { * get list of users with access to the file * * @param string $path to the file - * @return array + * @return array ['users' => $uniqueUserIds, 'public' => $public] */ public function getAccessList($path) { @@ -46,7 +46,7 @@ class File implements \OCP\Encryption\IFile { // always add owner to the list of users with access to the file $userIds = array($owner); - if (!$this->util->isFile($ownerPath)) { + if (!$this->util->isFile($owner . '/' . $ownerPath)) { return array('users' => $userIds, 'public' => false); } diff --git a/lib/private/encryption/manager.php b/lib/private/encryption/manager.php index 97203b7756d..8e080507c81 100644 --- a/lib/private/encryption/manager.php +++ b/lib/private/encryption/manager.php @@ -22,6 +22,7 @@ namespace OC\Encryption; +use OC\Files\Filesystem; use OC\Files\Storage\Shared; use OC\Files\Storage\Wrapper\Encryption; use OC\Files\View; @@ -222,7 +223,24 @@ class Manager implements IManager { $uid = $user ? $user->getUID() : null; $fileHelper = \OC::$server->getEncryptionFilesHelper(); $keyStorage = \OC::$server->getEncryptionKeyStorage(); - return new Encryption($parameters, $manager, $util, $logger, $fileHelper, $uid, $keyStorage); + $update = new Update( + new View(), + $util, + Filesystem::getMountManager(), + $manager, + $fileHelper, + $uid + ); + return new Encryption( + $parameters, + $manager, + $util, + $logger, + $fileHelper, + $uid, + $keyStorage, + $update + ); } else { return $storage; } diff --git a/lib/private/encryption/update.php b/lib/private/encryption/update.php index 7a170a03adc..f262099a3c5 100644 --- a/lib/private/encryption/update.php +++ b/lib/private/encryption/update.php @@ -22,6 +22,7 @@ namespace OC\Encryption; +use OC\Files\Filesystem; use \OC\Files\Mount; use \OC\Files\View; @@ -74,46 +75,73 @@ class Update { $this->uid = $uid; } + /** + * hook after file was shared + * + * @param array $params + */ public function postShared($params) { if ($params['itemType'] === 'file' || $params['itemType'] === 'folder') { - $this->update($params['fileSource']); - } - } - - public function postUnshared($params) { - if ($params['itemType'] === 'file' || $params['itemType'] === 'folder') { - $this->update($params['fileSource']); + $path = Filesystem::getPath($params['fileSource']); + list($owner, $ownerPath) = $this->getOwnerPath($path); + $absPath = '/' . $owner . '/files/' . $ownerPath; + $this->update($absPath); } } /** - * update keyfiles and share keys recursively + * hook after file was unshared * - * @param int $fileSource file source id + * @param array $params */ - private function update($fileSource) { - $path = \OC\Files\Filesystem::getPath($fileSource); - $info = \OC\Files\Filesystem::getFileInfo($path); - $owner = \OC\Files\Filesystem::getOwner($path); - $view = new \OC\Files\View('/' . $owner . '/files'); - $ownerPath = $view->getPath($info->getId()); - $absPath = '/' . $owner . '/files' . $ownerPath; + public function postUnshared($params) { + if ($params['itemType'] === 'file' || $params['itemType'] === 'folder') { + $path = Filesystem::getPath($params['fileSource']); + list($owner, $ownerPath) = $this->getOwnerPath($path); + $absPath = '/' . $owner . '/files/' . $ownerPath; + $this->update($absPath); + } + } - $mount = $this->mountManager->find($path); - $mountPoint = $mount->getMountPoint(); + /** + * get owner and path relative to data//files + * + * @param string $path path to file for current user + * @return array ['owner' => $owner, 'path' => $path] + * @throw \InvalidArgumentException + */ + private function getOwnerPath($path) { + $info = Filesystem::getFileInfo($path); + $owner = Filesystem::getOwner($path); + $view = new View('/' . $owner . '/files'); + $path = $view->getPath($info->getId()); + if ($path === null) { + throw new \InvalidArgumentException('No file found for ' . $info->getId()); + } + + return array($owner, $path); + } + + /** + * notify encryption module about added/removed users from a file/folder + * + * @param string $path relative to data/ + * @throws Exceptions\ModuleDoesNotExistsException + */ + public function update($path) { // if a folder was shared, get a list of all (sub-)folders - if ($this->view->is_dir($absPath)) { - $allFiles = $this->util->getAllFiles($absPath, $mountPoint); + if ($this->view->is_dir($path)) { + $allFiles = $this->util->getAllFiles($path); } else { - $allFiles = array($absPath); + $allFiles = array($path); } $encryptionModule = $this->encryptionManager->getDefaultEncryptionModule(); - foreach ($allFiles as $path) { - $usersSharing = $this->file->getAccessList($path); - $encryptionModule->update($path, $this->uid, $usersSharing); + foreach ($allFiles as $file) { + $usersSharing = $this->file->getAccessList($file); + $encryptionModule->update($file, $this->uid, $usersSharing); } } diff --git a/lib/private/encryption/util.php b/lib/private/encryption/util.php index 98a38012dba..032ac83f37e 100644 --- a/lib/private/encryption/util.php +++ b/lib/private/encryption/util.php @@ -25,6 +25,7 @@ namespace OC\Encryption; use OC\Encryption\Exceptions\EncryptionHeaderKeyExistsException; use OC\Encryption\Exceptions\EncryptionHeaderToLargeException; use OC\Encryption\Exceptions\ModuleDoesNotExistsException; +use OC\Files\Filesystem; use OC\Files\View; use OCP\Encryption\IEncryptionModule; use OCP\IConfig; @@ -181,10 +182,9 @@ class Util { * go recursively through a dir and collect all files and sub files. * * @param string $dir relative to the users files folder - * @param string $mountPoint * @return array with list of files relative to the users files folder */ - public function getAllFiles($dir, $mountPoint = '') { + public function getAllFiles($dir) { $result = array(); $dirList = array($dir); @@ -193,13 +193,10 @@ class Util { $content = $this->view->getDirectoryContent($dir); foreach ($content as $c) { - // getDirectoryContent() returns the paths relative to the mount points, so we need - // to re-construct the complete path - $path = ($mountPoint !== '') ? $mountPoint . '/' . $c->getPath() : $c->getPath(); - if ($c['type'] === 'dir') { - $dirList[] = \OC\Files\Filesystem::normalizePath($path); + if ($c->getType() === 'dir') { + $dirList[] = $c->getPath(); } else { - $result[] = \OC\Files\Filesystem::normalizePath($path); + $result[] = $c->getPath(); } } @@ -212,11 +209,12 @@ class Util { * check if it is a file uploaded by the user stored in data/user/files * or a metadata file * - * @param string $path + * @param string $path relative to the data/ folder * @return boolean */ public function isFile($path) { - if (substr($path, 0, strlen('/files/')) === '/files/') { + $parts = explode('/', Filesystem::normalizePath($path), 4); + if (isset($parts[2]) && $parts[2] === 'files') { return true; } return false; @@ -262,7 +260,7 @@ class Util { $ownerPath = implode('/', array_slice($parts, 2)); - return array($uid, \OC\Files\Filesystem::normalizePath($ownerPath)); + return array($uid, Filesystem::normalizePath($ownerPath)); } diff --git a/lib/private/files/storage/wrapper/encryption.php b/lib/private/files/storage/wrapper/encryption.php index 0a9e6d61d2e..089da941756 100644 --- a/lib/private/files/storage/wrapper/encryption.php +++ b/lib/private/files/storage/wrapper/encryption.php @@ -24,8 +24,13 @@ namespace OC\Files\Storage\Wrapper; use OC\Encryption\Exceptions\ModuleDoesNotExistsException; use OC\Encryption\File; +use OC\Encryption\Manager; +use OC\Encryption\Update; +use OC\Encryption\Util; use OC\Files\Filesystem; use OC\Files\Storage\LocalTempFileTrait; +use OC\Log; +use OCP\Encryption\Keys\IStorage; use OCP\Files\Mount\IMountPoint; class Encryption extends Wrapper { @@ -59,6 +64,9 @@ class Encryption extends Wrapper { /** @var \OCP\Encryption\Keys\IStorage */ private $keyStorage; + /** @var \OC\Encryption\Update */ + private $update; + /** * @param array $parameters * @param \OC\Encryption\Manager $encryptionManager @@ -66,15 +74,18 @@ class Encryption extends Wrapper { * @param \OC\Log $logger * @param File $fileHelper * @param string $uid user who perform the read/write operation (null for public access) + * @param IStorage $keyStorage + * @param Update $update */ public function __construct( $parameters, - \OC\Encryption\Manager $encryptionManager = null, - \OC\Encryption\Util $util = null, - \OC\Log $logger = null, + Manager $encryptionManager = null, + Util $util = null, + Log $logger = null, File $fileHelper = null, $uid = null, - $keyStorage = null + IStorage $keyStorage = null, + Update $update = null ) { $this->mountPoint = $parameters['mountPoint']; @@ -86,6 +97,7 @@ class Encryption extends Wrapper { $this->fileHelper = $fileHelper; $this->keyStorage = $keyStorage; $this->unencryptedSize = array(); + $this->update = $update; parent::__construct($parameters); } @@ -207,12 +219,11 @@ class Encryption extends Wrapper { * @return bool */ public function rename($path1, $path2) { - $fullPath1 = $this->getFullPath($path1); - if ($this->util->isExcluded($fullPath1)) { + $source = $this->getFullPath($path1); + if ($this->util->isExcluded($source)) { return $this->storage->rename($path1, $path2); } - $source = $this->getFullPath($path1); $result = $this->storage->rename($path1, $path2); if ($result) { $target = $this->getFullPath($path2); @@ -220,6 +231,9 @@ class Encryption extends Wrapper { $this->unencryptedSize[$target] = $this->unencryptedSize[$source]; } $this->keyStorage->renameKeys($source, $target); + if (dirname($source) !== dirname($target) && $this->util->isFile($target)) { + $this->update->update($target); + } } return $result; diff --git a/tests/lib/encryption/updatetest.php b/tests/lib/encryption/updatetest.php new file mode 100644 index 00000000000..28bb0031308 --- /dev/null +++ b/tests/lib/encryption/updatetest.php @@ -0,0 +1,129 @@ + + * + * @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 + * + */ + + +namespace Test\Encryption; + + +use OC\Encryption\Update; +use Test\TestCase; + +class UpdateTest extends TestCase { + + /** @var \OC\Encryption\Update */ + private $update; + + /** @var string */ + private $uid; + + /** @var \OC\Files\View | \PHPUnit_Framework_MockObject_MockObject */ + private $view; + + /** @var \OC\Encryption\Util | \PHPUnit_Framework_MockObject_MockObject */ + private $util; + + /** @var \OC\Files\Mount\Manager | \PHPUnit_Framework_MockObject_MockObject */ + private $mountManager; + + /** @var \OC\Encryption\Manager | \PHPUnit_Framework_MockObject_MockObject */ + private $encryptionManager; + + /** @var \OCP\Encryption\IEncryptionModule | \PHPUnit_Framework_MockObject_MockObject */ + private $encryptionModule; + + /** @var \OC\Encryption\File | \PHPUnit_Framework_MockObject_MockObject */ + private $fileHelper; + + public function setUp() { + parent::setUp(); + + $this->view = $this->getMockBuilder('\OC\Files\View') + ->disableOriginalConstructor()->getMock(); + $this->util = $this->getMockBuilder('\OC\Encryption\Util') + ->disableOriginalConstructor()->getMock(); + $this->mountManager = $this->getMockBuilder('\OC\Files\Mount\Manager') + ->disableOriginalConstructor()->getMock(); + $this->encryptionManager = $this->getMockBuilder('\OC\Encryption\Manager') + ->disableOriginalConstructor()->getMock(); + $this->fileHelper = $this->getMockBuilder('\OC\Encryption\File') + ->disableOriginalConstructor()->getMock(); + $this->encryptionModule = $this->getMockBuilder('\OCP\Encryption\IEncryptionModule') + ->disableOriginalConstructor()->getMock(); + + $this->encryptionManager->expects($this->once()) + ->method('getDefaultEncryptionModule') + ->willReturn($this->encryptionModule); + + $this->uid = 'testUser1'; + + $this->update = new Update( + $this->view, + $this->util, + $this->mountManager, + $this->encryptionManager, + $this->fileHelper, + $this->uid); + } + + /** + * @dataProvider dataTestUpdate + * + * @param string $path + * @param boolean $isDir + * @param array $allFiles + * @param integer $numberOfFiles + */ + public function testUpdate($path, $isDir, $allFiles, $numberOfFiles) { + + $this->view->expects($this->once()) + ->method('is_dir') + ->willReturn($isDir); + + if($isDir) { + $this->util->expects($this->once()) + ->method('getAllFiles') + ->willReturn($allFiles); + } + + $this->fileHelper->expects($this->exactly($numberOfFiles)) + ->method('getAccessList') + ->willReturn(['users' => [], 'public' => false]); + + $this->encryptionModule->expects($this->exactly($numberOfFiles)) + ->method('update') + ->willReturn(true); + + $this->update->update($path); + } + + /** + * data provider for testUpdate() + * + * @return array + */ + public function dataTestUpdate() { + return array( + array('/user/files/foo', true, ['/user/files/foo/file1.txt', '/user/files/foo/file1.txt'], 2), + array('/user/files/test.txt', false, [], 1), + ); + } + +} diff --git a/tests/lib/encryption/utiltest.php b/tests/lib/encryption/utiltest.php index dc6205e16fd..7de57043920 100644 --- a/tests/lib/encryption/utiltest.php +++ b/tests/lib/encryption/utiltest.php @@ -152,4 +152,25 @@ class UtilTest extends TestCase { return false; } + /** + * @dataProvider dataTestIsFile + */ + public function testIsFile($path, $expected) { + $this->assertSame($expected, + $this->util->isFile($path) + ); + } + + public function dataTestIsFile() { + return array( + array('/user/files/test.txt', true), + array('/user/files', true), + array('/user/files_versions/test.txt', false), + array('/user/foo/files/test.txt', false), + array('/files/foo/files/test.txt', false), + array('/user', false), + array('/user/test.txt', false), + ); + } + } diff --git a/tests/lib/files/storage/wrapper/encryption.php b/tests/lib/files/storage/wrapper/encryption.php index 1d776555503..12309c5ac44 100644 --- a/tests/lib/files/storage/wrapper/encryption.php +++ b/tests/lib/files/storage/wrapper/encryption.php @@ -12,11 +12,26 @@ class Encryption extends \Test\Files\Storage\Storage { */ private $sourceStorage; + /** + * @var \OC\Files\Storage\Wrapper\Encryption + */ + protected $instance; + /** * @var \OC\Encryption\Keys\Storage | \PHPUnit_Framework_MockObject_MockObject */ private $keyStore; + /** + * @var \OC\Encryption\Util | \PHPUnit_Framework_MockObject_MockObject + */ + private $util; + + /** + * @var \OC\Encryption\Update | \PHPUnit_Framework_MockObject_MockObject + */ + private $update; + public function setUp() { parent::setUp(); @@ -43,8 +58,8 @@ class Encryption extends \Test\Files\Storage\Storage { ->disableOriginalConstructor() ->getMock(); - $util = $this->getMock('\OC\Encryption\Util', ['getUidAndFilename'], [new View(), new \OC\User\Manager(), $groupManager, $config]); - $util->expects($this->any()) + $this->util = $this->getMock('\OC\Encryption\Util', ['getUidAndFilename', 'isFile'], [new View(), new \OC\User\Manager(), $groupManager, $config]); + $this->util->expects($this->any()) ->method('getUidAndFilename') ->willReturnCallback(function ($path) { return ['user1', $path]; @@ -61,6 +76,8 @@ class Encryption extends \Test\Files\Storage\Storage { $this->sourceStorage = new Temporary(array()); $this->keyStore = $this->getMockBuilder('\OC\Encryption\Keys\Storage') ->disableOriginalConstructor()->getMock(); + $this->update = $this->getMockBuilder('\OC\Encryption\Update') + ->disableOriginalConstructor()->getMock(); $mount = $this->getMockBuilder('\OC\Files\Mount\MountPoint') ->disableOriginalConstructor() ->setMethods(['getOption']) @@ -72,7 +89,7 @@ class Encryption extends \Test\Files\Storage\Storage { 'mountPoint' => '/', 'mount' => $mount ], - $encryptionManager, $util, $logger, $file, null, $this->keyStore + $encryptionManager, $this->util, $logger, $file, null, $this->keyStore, $this->update ); } @@ -97,11 +114,41 @@ class Encryption extends \Test\Files\Storage\Storage { return $encryptionModule; } - public function testRename() { + /** + * @dataProvider dataTestRename + * + * @param string $source + * @param string $target + * @param boolean $shouldUpdate + */ + public function testRename($source, $target, $shouldUpdate) { $this->keyStore ->expects($this->once()) ->method('renameKeys'); - $this->instance->mkdir('folder'); - $this->instance->rename('folder', 'flodder'); + $this->util->expects($this->any()) + ->method('isFile')->willReturn(true); + if ($shouldUpdate) { + $this->update->expects($this->once()) + ->method('update'); + } else { + $this->update->expects($this->never()) + ->method('update'); + } + + $this->instance->mkdir($source); + $this->instance->mkdir(dirname($target)); + $this->instance->rename($source, $target); + } + + /** + * data provider for testRename() + * + * @return array + */ + public function dataTestRename() { + return array( + array('source', 'target', false), + array('source', '/subFolder/target', true), + ); } } From 2646bccb83d06f575722e3fb8c5bd87ed42775c9 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Thu, 23 Apr 2015 17:06:55 +0200 Subject: [PATCH 2/6] update share keys if file gets copied --- .../files/storage/wrapper/encryption.php | 6 +- .../lib/files/storage/wrapper/encryption.php | 83 +++++++++++++++---- 2 files changed, 70 insertions(+), 19 deletions(-) diff --git a/lib/private/files/storage/wrapper/encryption.php b/lib/private/files/storage/wrapper/encryption.php index 089da941756..0f6096adb76 100644 --- a/lib/private/files/storage/wrapper/encryption.php +++ b/lib/private/files/storage/wrapper/encryption.php @@ -256,9 +256,9 @@ class Encryption extends Wrapper { $result = $this->storage->copy($path1, $path2); if ($result) { $target = $this->getFullPath($path2); - $encryptionModule = $this->getEncryptionModule($path2); - if ($encryptionModule) { - $this->keyStorage->copyKeys($source, $target); + $this->keyStorage->copyKeys($source, $target); + if (dirname($source) !== dirname($target) && $this->util->isFile($target)) { + $this->update->update($target); } } diff --git a/tests/lib/files/storage/wrapper/encryption.php b/tests/lib/files/storage/wrapper/encryption.php index 12309c5ac44..f22f02f568c 100644 --- a/tests/lib/files/storage/wrapper/encryption.php +++ b/tests/lib/files/storage/wrapper/encryption.php @@ -27,6 +27,18 @@ class Encryption extends \Test\Files\Storage\Storage { */ private $util; + + /** + * @var \OC\Encryption\Manager | \PHPUnit_Framework_MockObject_MockObject + */ + private $encryptionManager; + + /** + * @var \OCP\Encryption\IEncryptionModule | \PHPUnit_Framework_MockObject_MockObject + */ + private $encryptionModule; + + /** * @var \OC\Encryption\Update | \PHPUnit_Framework_MockObject_MockObject */ @@ -37,17 +49,17 @@ class Encryption extends \Test\Files\Storage\Storage { parent::setUp(); $mockModule = $this->buildMockModule(); - $encryptionManager = $this->getMockBuilder('\OC\Encryption\Manager') + $this->encryptionManager = $this->getMockBuilder('\OC\Encryption\Manager') ->disableOriginalConstructor() ->setMethods(['getDefaultEncryptionModule', 'getEncryptionModule', 'isEnabled']) ->getMock(); - $encryptionManager->expects($this->any()) + $this->encryptionManager->expects($this->any()) ->method('getDefaultEncryptionModule') ->willReturn($mockModule); - $encryptionManager->expects($this->any()) + $this->encryptionManager->expects($this->any()) ->method('getEncryptionModule') ->willReturn($mockModule); - $encryptionManager->expects($this->any()) + $this->encryptionManager->expects($this->any()) ->method('isEnabled') ->willReturn(true); @@ -89,7 +101,7 @@ class Encryption extends \Test\Files\Storage\Storage { 'mountPoint' => '/', 'mount' => $mount ], - $encryptionManager, $this->util, $logger, $file, null, $this->keyStore, $this->update + $this->encryptionManager, $this->util, $logger, $file, null, $this->keyStore, $this->update ); } @@ -97,21 +109,21 @@ class Encryption extends \Test\Files\Storage\Storage { * @return \PHPUnit_Framework_MockObject_MockObject */ protected function buildMockModule() { - $encryptionModule = $this->getMockBuilder('\OCP\Encryption\IEncryptionModule') + $this->encryptionModule = $this->getMockBuilder('\OCP\Encryption\IEncryptionModule') ->disableOriginalConstructor() ->setMethods(['getId', 'getDisplayName', 'begin', 'end', 'encrypt', 'decrypt', 'update', 'shouldEncrypt', 'getUnencryptedBlockSize']) ->getMock(); - $encryptionModule->expects($this->any())->method('getId')->willReturn('UNIT_TEST_MODULE'); - $encryptionModule->expects($this->any())->method('getDisplayName')->willReturn('Unit test module'); - $encryptionModule->expects($this->any())->method('begin')->willReturn([]); - $encryptionModule->expects($this->any())->method('end')->willReturn(''); - $encryptionModule->expects($this->any())->method('encrypt')->willReturnArgument(0); - $encryptionModule->expects($this->any())->method('decrypt')->willReturnArgument(0); - $encryptionModule->expects($this->any())->method('update')->willReturn(true); - $encryptionModule->expects($this->any())->method('shouldEncrypt')->willReturn(true); - $encryptionModule->expects($this->any())->method('getUnencryptedBlockSize')->willReturn(8192); - return $encryptionModule; + $this->encryptionModule->expects($this->any())->method('getId')->willReturn('UNIT_TEST_MODULE'); + $this->encryptionModule->expects($this->any())->method('getDisplayName')->willReturn('Unit test module'); + $this->encryptionModule->expects($this->any())->method('begin')->willReturn([]); + $this->encryptionModule->expects($this->any())->method('end')->willReturn(''); + $this->encryptionModule->expects($this->any())->method('encrypt')->willReturnArgument(0); + $this->encryptionModule->expects($this->any())->method('decrypt')->willReturnArgument(0); + $this->encryptionModule->expects($this->any())->method('update')->willReturn(true); + $this->encryptionModule->expects($this->any())->method('shouldEncrypt')->willReturn(true); + $this->encryptionModule->expects($this->any())->method('getUnencryptedBlockSize')->willReturn(8192); + return $this->encryptionModule; } /** @@ -151,4 +163,43 @@ class Encryption extends \Test\Files\Storage\Storage { array('source', '/subFolder/target', true), ); } + + /** + * @dataProvider dataTestCopy + * + * @param string $source + * @param string $target + * @param boolean $shouldUpdate + */ + public function testCopy($source, $target, $shouldUpdate) { + $this->keyStore + ->expects($this->once()) + ->method('copyKeys'); + $this->util->expects($this->any()) + ->method('isFile')->willReturn(true); + if ($shouldUpdate) { + $this->update->expects($this->once()) + ->method('update'); + } else { + $this->update->expects($this->never()) + ->method('update'); + } + + $this->instance->mkdir($source); + $this->instance->mkdir(dirname($target)); + $this->instance->copy($source, $target); + } + + /** + * data provider for testRename() + * + * @return array + */ + public function dataTestCopy() { + return array( + array('source', 'target', false), + array('source', '/subFolder/target', true), + ); + } + } From 24128d1384f2548f0cdc35c26d684dbeb61d091b Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 24 Apr 2015 10:16:06 +0200 Subject: [PATCH 3/6] only update share keys if the file was encrypted --- lib/private/encryption/keys/storage.php | 9 +++++++++ lib/private/files/storage/wrapper/encryption.php | 14 ++++++++++---- lib/public/encryption/keys/istorage.php | 2 ++ tests/lib/files/storage/wrapper/encryption.php | 6 ++++-- 4 files changed, 25 insertions(+), 6 deletions(-) diff --git a/lib/private/encryption/keys/storage.php b/lib/private/encryption/keys/storage.php index e34d7370ef1..118c8dc920d 100644 --- a/lib/private/encryption/keys/storage.php +++ b/lib/private/encryption/keys/storage.php @@ -235,6 +235,7 @@ class Storage implements IStorage { * * @param string $source * @param string $target + * @return boolean */ public function renameKeys($source, $target) { @@ -253,7 +254,11 @@ class Storage implements IStorage { if ($this->view->file_exists($sourcePath)) { $this->keySetPreparation(dirname($targetPath)); $this->view->rename($sourcePath, $targetPath); + + return true; } + + return false; } /** @@ -261,6 +266,7 @@ class Storage implements IStorage { * * @param string $source * @param string $target + * @return boolean */ public function copyKeys($source, $target) { @@ -279,7 +285,10 @@ class Storage implements IStorage { if ($this->view->file_exists($sourcePath)) { $this->keySetPreparation(dirname($targetPath)); $this->view->copy($sourcePath, $targetPath); + return true; } + + return false; } /** diff --git a/lib/private/files/storage/wrapper/encryption.php b/lib/private/files/storage/wrapper/encryption.php index 0f6096adb76..4d546495aaa 100644 --- a/lib/private/files/storage/wrapper/encryption.php +++ b/lib/private/files/storage/wrapper/encryption.php @@ -230,8 +230,11 @@ class Encryption extends Wrapper { if (isset($this->unencryptedSize[$source])) { $this->unencryptedSize[$target] = $this->unencryptedSize[$source]; } - $this->keyStorage->renameKeys($source, $target); - if (dirname($source) !== dirname($target) && $this->util->isFile($target)) { + $keysRenamed = $this->keyStorage->renameKeys($source, $target); + if ($keysRenamed && + dirname($source) !== dirname($target) && + $this->util->isFile($target) + ) { $this->update->update($target); } } @@ -256,8 +259,11 @@ class Encryption extends Wrapper { $result = $this->storage->copy($path1, $path2); if ($result) { $target = $this->getFullPath($path2); - $this->keyStorage->copyKeys($source, $target); - if (dirname($source) !== dirname($target) && $this->util->isFile($target)) { + $keysCopied = $this->keyStorage->copyKeys($source, $target); + if ($keysCopied && + dirname($source) !== dirname($target) && + $this->util->isFile($target) + ) { $this->update->update($target); } } diff --git a/lib/public/encryption/keys/istorage.php b/lib/public/encryption/keys/istorage.php index 696d5373310..ffbffdc1a27 100644 --- a/lib/public/encryption/keys/istorage.php +++ b/lib/public/encryption/keys/istorage.php @@ -153,6 +153,7 @@ interface IStorage { * * @param string $source * @param string $target + * @return boolean * @since 8.1.0 */ public function renameKeys($source, $target); @@ -162,6 +163,7 @@ interface IStorage { * * @param string $source * @param string $target + * @retrun boolean * @since 8.1.0 */ public function copyKeys($source, $target); diff --git a/tests/lib/files/storage/wrapper/encryption.php b/tests/lib/files/storage/wrapper/encryption.php index f22f02f568c..2d3f10ecdfa 100644 --- a/tests/lib/files/storage/wrapper/encryption.php +++ b/tests/lib/files/storage/wrapper/encryption.php @@ -136,7 +136,8 @@ class Encryption extends \Test\Files\Storage\Storage { public function testRename($source, $target, $shouldUpdate) { $this->keyStore ->expects($this->once()) - ->method('renameKeys'); + ->method('renameKeys') + ->willReturn(true); $this->util->expects($this->any()) ->method('isFile')->willReturn(true); if ($shouldUpdate) { @@ -174,7 +175,8 @@ class Encryption extends \Test\Files\Storage\Storage { public function testCopy($source, $target, $shouldUpdate) { $this->keyStore ->expects($this->once()) - ->method('copyKeys'); + ->method('copyKeys') + ->willReturn(true); $this->util->expects($this->any()) ->method('isFile')->willReturn(true); if ($shouldUpdate) { From 1592be117a4286508b7841a2b1e640d3655e7503 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 24 Apr 2015 13:06:03 +0200 Subject: [PATCH 4/6] Use public interfaces for type hinting --- .../files/storage/wrapper/encryption.php | 24 +++++++++---------- lib/public/encryption/keys/istorage.php | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/private/files/storage/wrapper/encryption.php b/lib/private/files/storage/wrapper/encryption.php index 4d546495aaa..0dc59cbb2a0 100644 --- a/lib/private/files/storage/wrapper/encryption.php +++ b/lib/private/files/storage/wrapper/encryption.php @@ -23,15 +23,15 @@ namespace OC\Files\Storage\Wrapper; use OC\Encryption\Exceptions\ModuleDoesNotExistsException; -use OC\Encryption\File; -use OC\Encryption\Manager; use OC\Encryption\Update; use OC\Encryption\Util; use OC\Files\Filesystem; use OC\Files\Storage\LocalTempFileTrait; -use OC\Log; +use OCP\Encryption\IFile; +use OCP\Encryption\IManager; use OCP\Encryption\Keys\IStorage; use OCP\Files\Mount\IMountPoint; +use OCP\ILogger; class Encryption extends Wrapper { @@ -43,10 +43,10 @@ class Encryption extends Wrapper { /** @var \OC\Encryption\Util */ private $util; - /** @var \OC\Encryption\Manager */ + /** @var \OCP\Encryption\IManager */ private $encryptionManager; - /** @var \OC\Log */ + /** @var \OCP\ILogger */ private $logger; /** @var string */ @@ -55,7 +55,7 @@ class Encryption extends Wrapper { /** @var array */ private $unencryptedSize; - /** @var File */ + /** @var \OCP\Encryption\IFile */ private $fileHelper; /** @var IMountPoint */ @@ -69,20 +69,20 @@ class Encryption extends Wrapper { /** * @param array $parameters - * @param \OC\Encryption\Manager $encryptionManager + * @param \OCP\Encryption\IManager $encryptionManager * @param \OC\Encryption\Util $util - * @param \OC\Log $logger - * @param File $fileHelper + * @param \OCP\ILogger $logger + * @param \OCP\Encryption\IFile $fileHelper * @param string $uid user who perform the read/write operation (null for public access) * @param IStorage $keyStorage * @param Update $update */ public function __construct( $parameters, - Manager $encryptionManager = null, + IManager $encryptionManager = null, Util $util = null, - Log $logger = null, - File $fileHelper = null, + ILogger $logger = null, + IFile $fileHelper = null, $uid = null, IStorage $keyStorage = null, Update $update = null diff --git a/lib/public/encryption/keys/istorage.php b/lib/public/encryption/keys/istorage.php index ffbffdc1a27..752c073375d 100644 --- a/lib/public/encryption/keys/istorage.php +++ b/lib/public/encryption/keys/istorage.php @@ -163,7 +163,7 @@ interface IStorage { * * @param string $source * @param string $target - * @retrun boolean + * @return boolean * @since 8.1.0 */ public function copyKeys($source, $target); From 781cfff2216c4dfab109996f62ee7afd912dce17 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 24 Apr 2015 13:06:27 +0200 Subject: [PATCH 5/6] Deduplicate data provider and fix method visibility --- tests/lib/encryption/updatetest.php | 2 +- .../lib/files/storage/wrapper/encryption.php | 32 ++++++++----------- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/tests/lib/encryption/updatetest.php b/tests/lib/encryption/updatetest.php index 28bb0031308..08d4125735d 100644 --- a/tests/lib/encryption/updatetest.php +++ b/tests/lib/encryption/updatetest.php @@ -52,7 +52,7 @@ class UpdateTest extends TestCase { /** @var \OC\Encryption\File | \PHPUnit_Framework_MockObject_MockObject */ private $fileHelper; - public function setUp() { + protected function setUp() { parent::setUp(); $this->view = $this->getMockBuilder('\OC\Files\View') diff --git a/tests/lib/files/storage/wrapper/encryption.php b/tests/lib/files/storage/wrapper/encryption.php index 2d3f10ecdfa..a257ca24a0c 100644 --- a/tests/lib/files/storage/wrapper/encryption.php +++ b/tests/lib/files/storage/wrapper/encryption.php @@ -44,7 +44,7 @@ class Encryption extends \Test\Files\Storage\Storage { */ private $update; - public function setUp() { + protected function setUp() { parent::setUp(); @@ -127,7 +127,7 @@ class Encryption extends \Test\Files\Storage\Storage { } /** - * @dataProvider dataTestRename + * @dataProvider dataTestCopyAndRename * * @param string $source * @param string $target @@ -154,25 +154,13 @@ class Encryption extends \Test\Files\Storage\Storage { } /** - * data provider for testRename() - * - * @return array - */ - public function dataTestRename() { - return array( - array('source', 'target', false), - array('source', '/subFolder/target', true), - ); - } - - /** - * @dataProvider dataTestCopy + * @dataProvider dataTestCopyAndRename * * @param string $source * @param string $target * @param boolean $shouldUpdate */ - public function testCopy($source, $target, $shouldUpdate) { + public function testCopyTesting($source, $target, $shouldUpdate) { $this->keyStore ->expects($this->once()) ->method('copyKeys') @@ -193,12 +181,20 @@ class Encryption extends \Test\Files\Storage\Storage { } /** - * data provider for testRename() + * @dataProvider copyAndMoveProvider + */ + public function testCopy($source, $target) { + $this->assertTrue(true, 'Replaced by testCopyTesting()'); + } + + /** + * data provider for testCopyTesting() and dataTestCopyAndRename() * * @return array */ - public function dataTestCopy() { + public function dataTestCopyAndRename() { return array( + array('source', 'target', false), array('source', 'target', false), array('source', '/subFolder/target', true), ); From 411f7893bf34507ba0b12b35a35596cd65c90b48 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 24 Apr 2015 14:27:23 +0200 Subject: [PATCH 6/6] Add test "operation on keys failed" --- tests/lib/files/storage/wrapper/encryption.php | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/tests/lib/files/storage/wrapper/encryption.php b/tests/lib/files/storage/wrapper/encryption.php index a257ca24a0c..de43c24659e 100644 --- a/tests/lib/files/storage/wrapper/encryption.php +++ b/tests/lib/files/storage/wrapper/encryption.php @@ -131,13 +131,14 @@ class Encryption extends \Test\Files\Storage\Storage { * * @param string $source * @param string $target + * @param boolean $renameKeysReturn * @param boolean $shouldUpdate */ - public function testRename($source, $target, $shouldUpdate) { + public function testRename($source, $target, $renameKeysReturn, $shouldUpdate) { $this->keyStore ->expects($this->once()) ->method('renameKeys') - ->willReturn(true); + ->willReturn($renameKeysReturn); $this->util->expects($this->any()) ->method('isFile')->willReturn(true); if ($shouldUpdate) { @@ -158,13 +159,14 @@ class Encryption extends \Test\Files\Storage\Storage { * * @param string $source * @param string $target + * @param boolean $copyKeysReturn * @param boolean $shouldUpdate */ - public function testCopyTesting($source, $target, $shouldUpdate) { + public function testCopyTesting($source, $target, $copyKeysReturn, $shouldUpdate) { $this->keyStore ->expects($this->once()) ->method('copyKeys') - ->willReturn(true); + ->willReturn($copyKeysReturn); $this->util->expects($this->any()) ->method('isFile')->willReturn(true); if ($shouldUpdate) { @@ -194,9 +196,10 @@ class Encryption extends \Test\Files\Storage\Storage { */ public function dataTestCopyAndRename() { return array( - array('source', 'target', false), - array('source', 'target', false), - array('source', '/subFolder/target', true), + array('source', 'target', false, false), + array('source', 'target', true, false), + array('source', '/subFolder/target', false, false), + array('source', '/subFolder/target', true, true), ); }