From 3000f0125fe9470012c362a13ecc33a31cceff18 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 17 Jul 2015 12:49:45 +0200 Subject: [PATCH 1/2] don't move keys if the key where already moved in a previous migration run --- apps/encryption/appinfo/register_command.php | 3 +- apps/encryption/command/migratekeys.php | 10 ++- apps/encryption/lib/migration.php | 74 ++++++++++++++++---- apps/encryption/tests/lib/MigrationTest.php | 13 ++-- settings/application.php | 3 +- settings/controller/encryptioncontroller.php | 10 ++- 6 files changed, 89 insertions(+), 24 deletions(-) diff --git a/apps/encryption/appinfo/register_command.php b/apps/encryption/appinfo/register_command.php index f727fdf9d7..4fdf7ecec3 100644 --- a/apps/encryption/appinfo/register_command.php +++ b/apps/encryption/appinfo/register_command.php @@ -26,4 +26,5 @@ $userManager = OC::$server->getUserManager(); $view = new \OC\Files\View(); $config = \OC::$server->getConfig(); $connection = \OC::$server->getDatabaseConnection(); -$application->add(new MigrateKeys($userManager, $view, $connection, $config)); +$logger = \OC::$server->getLogger(); +$application->add(new MigrateKeys($userManager, $view, $connection, $config, $logger)); diff --git a/apps/encryption/command/migratekeys.php b/apps/encryption/command/migratekeys.php index d0fc157306..7e32010217 100644 --- a/apps/encryption/command/migratekeys.php +++ b/apps/encryption/command/migratekeys.php @@ -27,6 +27,7 @@ use OC\Files\View; use OC\User\Manager; use OCA\Encryption\Migration; use OCP\IConfig; +use OCP\ILogger; use OCP\IUserBackend; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -44,22 +45,27 @@ class MigrateKeys extends Command { private $connection; /** @var IConfig */ private $config; + /** @var ILogger */ + private $logger; /** * @param Manager $userManager * @param View $view * @param Connection $connection * @param IConfig $config + * @param ILogger $logger */ public function __construct(Manager $userManager, View $view, Connection $connection, - IConfig $config) { + IConfig $config, + ILogger $logger) { $this->userManager = $userManager; $this->view = $view; $this->connection = $connection; $this->config = $config; + $this->logger = $logger; parent::__construct(); } @@ -77,7 +83,7 @@ class MigrateKeys extends Command { protected function execute(InputInterface $input, OutputInterface $output) { // perform system reorganization - $migration = new Migration($this->config, $this->view, $this->connection); + $migration = new Migration($this->config, $this->view, $this->connection, $this->logger); $users = $input->getArgument('user_id'); if (!empty($users)) { diff --git a/apps/encryption/lib/migration.php b/apps/encryption/lib/migration.php index 26e2a143f6..0903587e87 100644 --- a/apps/encryption/lib/migration.php +++ b/apps/encryption/lib/migration.php @@ -26,6 +26,7 @@ namespace OCA\Encryption; use OC\DB\Connection; use OC\Files\View; use OCP\IConfig; +use OCP\ILogger; class Migration { @@ -37,17 +38,22 @@ class Migration { /** @var IConfig */ private $config; + /** @var ILogger */ + private $logger; + /** * @param IConfig $config * @param View $view * @param Connection $connection + * @param ILogger $logger */ - public function __construct(IConfig $config, View $view, Connection $connection) { + public function __construct(IConfig $config, View $view, Connection $connection, ILogger $logger) { $this->view = $view; $this->view->getUpdater()->disable(); $this->connection = $connection; $this->moduleId = \OCA\Encryption\Crypto\Encryption::ID; $this->config = $config; + $this->logger = $logger; } public function finalCleanUp() { @@ -234,9 +240,10 @@ class Migration { private function renameUsersPrivateKey($user) { $oldPrivateKey = $user . '/files_encryption/' . $user . '.privateKey'; $newPrivateKey = $user . '/files_encryption/' . $this->moduleId . '/' . $user . '.privateKey'; - $this->createPathForKeys(dirname($newPrivateKey)); - - $this->view->rename($oldPrivateKey, $newPrivateKey); + if ($this->view->file_exists($oldPrivateKey)) { + $this->createPathForKeys(dirname($newPrivateKey)); + $this->view->rename($oldPrivateKey, $newPrivateKey); + } } /** @@ -247,9 +254,10 @@ class Migration { private function renameUsersPublicKey($user) { $oldPublicKey = '/files_encryption/public_keys/' . $user . '.publicKey'; $newPublicKey = $user . '/files_encryption/' . $this->moduleId . '/' . $user . '.publicKey'; - $this->createPathForKeys(dirname($newPublicKey)); - - $this->view->rename($oldPublicKey, $newPublicKey); + if ($this->view->file_exists($oldPublicKey)) { + $this->createPathForKeys(dirname($newPublicKey)); + $this->view->rename($oldPublicKey, $newPublicKey); + } } /** @@ -261,6 +269,11 @@ class Migration { */ private function renameFileKeys($user, $path, $trash = false) { + if ($this->view->is_dir($user . '/' . $path) === false) { + $this->logger->info('Skip dir /' . $user . '/' . $path . ': does not exist'); + return; + } + $dh = $this->view->opendir($user . '/' . $path); if (is_resource($dh)) { @@ -270,8 +283,15 @@ class Migration { $this->renameFileKeys($user, $path . '/' . $file, $trash); } else { $target = $this->getTargetDir($user, $path, $file, $trash); - $this->createPathForKeys(dirname($target)); - $this->view->rename($user . '/' . $path . '/' . $file, $target); + if ($target) { + $this->createPathForKeys(dirname($target)); + $this->view->rename($user . '/' . $path . '/' . $file, $target); + } else { + $this->logger->warning( + 'did not move key "' . $file + . '" could not find the corresponding file in /data/' . $user . '/files.' + . 'Most likely the key was already moved in a previous migration run and is already on the right place.'); + } } } } @@ -279,23 +299,49 @@ class Migration { } } + /** + * get system mount points + * wrap static method so that it can be mocked for testing + * + * @return array + */ + protected function getSystemMountPoints() { + return \OC_Mount_Config::getSystemMountPoints(); + } + /** * generate target directory * * @param string $user - * @param string $filePath + * @param string $keyPath * @param string $filename * @param bool $trash * @return string */ - private function getTargetDir($user, $filePath, $filename, $trash) { + private function getTargetDir($user, $keyPath, $filename, $trash) { if ($trash) { - $targetDir = $user . '/files_encryption/keys/files_trashbin/' . substr($filePath, strlen('/files_trashbin/keys/')) . '/' . $this->moduleId . '/' . $filename; + $filePath = substr($keyPath, strlen('/files_trashbin/keys/')); + $targetDir = $user . '/files_encryption/keys/files_trashbin/' . $filePath . '/' . $this->moduleId . '/' . $filename; } else { - $targetDir = $user . '/files_encryption/keys/files/' . substr($filePath, strlen('/files_encryption/keys/')) . '/' . $this->moduleId . '/' . $filename; + $filePath = substr($keyPath, strlen('/files_encryption/keys/')); + $targetDir = $user . '/files_encryption/keys/files/' . $filePath . '/' . $this->moduleId . '/' . $filename; } - return $targetDir; + if ($user === '') { + // for system wide mounts we need to check if the mount point really exists + $normalized = trim($filePath, '/'); + $systemMountPoints = $this->getSystemMountPoints(); + foreach ($systemMountPoints as $mountPoint) { + if (strpos($normalized, $mountPoint['mountpoint']) === 0) + return $targetDir; + } + } else if ($trash === false && $this->view->file_exists('/' . $user. '/files/' . $filePath)) { + return $targetDir; + } else if ($trash === true && $this->view->file_exists('/' . $user. '/files_trashbin/' . $filePath)) { + return $targetDir; + } + + return false; } /** diff --git a/apps/encryption/tests/lib/MigrationTest.php b/apps/encryption/tests/lib/MigrationTest.php index de1e2bd268..786bce33a2 100644 --- a/apps/encryption/tests/lib/MigrationTest.php +++ b/apps/encryption/tests/lib/MigrationTest.php @@ -24,6 +24,7 @@ namespace OCA\Encryption\Tests; use OCA\Encryption\Migration; +use OCP\ILogger; class MigrationTest extends \Test\TestCase { @@ -37,6 +38,9 @@ class MigrationTest extends \Test\TestCase { private $recovery_key_id = 'recovery_key_id'; private $moduleId; + /** @var PHPUnit_Framework_MockObject_MockObject | ILogger */ + private $logger; + public static function setUpBeforeClass() { parent::setUpBeforeClass(); \OC_User::createUser(self::TEST_ENCRYPTION_MIGRATION_USER1, 'foo'); @@ -53,6 +57,7 @@ class MigrationTest extends \Test\TestCase { public function setUp() { + $this->logger = $this->getMockBuilder('\OCP\ILogger')->disableOriginalConstructor()->getMock(); $this->view = new \OC\Files\View(); $this->moduleId = \OCA\Encryption\Crypto\Encryption::ID; } @@ -142,7 +147,7 @@ class MigrationTest extends \Test\TestCase { $this->createDummySystemWideKeys(); - $m = new Migration(\OC::$server->getConfig(), new \OC\Files\View(), \OC::$server->getDatabaseConnection()); + $m = new Migration(\OC::$server->getConfig(), new \OC\Files\View(), \OC::$server->getDatabaseConnection(), $this->logger); $m->reorganizeFolderStructure(); $this->assertTrue( @@ -267,7 +272,7 @@ class MigrationTest extends \Test\TestCase { public function testUpdateDB() { $this->prepareDB(); - $m = new Migration(\OC::$server->getConfig(), new \OC\Files\View(), \OC::$server->getDatabaseConnection()); + $m = new Migration(\OC::$server->getConfig(), new \OC\Files\View(), \OC::$server->getDatabaseConnection(), $this->logger); $m->updateDB(); $this->verifyDB('`*PREFIX*appconfig`', 'files_encryption', 0); @@ -286,7 +291,7 @@ class MigrationTest extends \Test\TestCase { $config->setAppValue('encryption', 'publicShareKeyId', 'wrong_share_id'); $config->setUserValue(self::TEST_ENCRYPTION_MIGRATION_USER1, 'encryption', 'recoverKeyEnabled', '9'); - $m = new Migration(\OC::$server->getConfig(), new \OC\Files\View(), \OC::$server->getDatabaseConnection()); + $m = new Migration(\OC::$server->getConfig(), new \OC\Files\View(), \OC::$server->getDatabaseConnection(), $this->logger); $m->updateDB(); $this->verifyDB('`*PREFIX*appconfig`', 'files_encryption', 0); @@ -349,7 +354,7 @@ class MigrationTest extends \Test\TestCase { */ public function testUpdateFileCache() { $this->prepareFileCache(); - $m = new Migration(\OC::$server->getConfig(), new \OC\Files\View(), \OC::$server->getDatabaseConnection()); + $m = new Migration(\OC::$server->getConfig(), new \OC\Files\View(), \OC::$server->getDatabaseConnection(), $this->logger); self::invokePrivate($m, 'updateFileCache'); // check results diff --git a/settings/application.php b/settings/application.php index 03203b4856..a2f25935e1 100644 --- a/settings/application.php +++ b/settings/application.php @@ -79,7 +79,8 @@ class Application extends App { $c->query('Config'), $c->query('DatabaseConnection'), $c->query('UserManager'), - new View() + new View(), + $c->query('Logger') ); }); $container->registerService('AppSettingsController', function(IContainer $c) { diff --git a/settings/controller/encryptioncontroller.php b/settings/controller/encryptioncontroller.php index 87cbf0a4bf..7c952962c1 100644 --- a/settings/controller/encryptioncontroller.php +++ b/settings/controller/encryptioncontroller.php @@ -25,6 +25,7 @@ use OC\Files\View; use OCA\Encryption\Migration; use OCP\IL10N; use OCP\AppFramework\Controller; +use OCP\ILogger; use OCP\IRequest; use OCP\IConfig; use OC\DB\Connection; @@ -50,6 +51,9 @@ class EncryptionController extends Controller { /** @var View */ private $view; + /** @var ILogger */ + private $logger; + /** * @param string $appName * @param IRequest $request @@ -58,6 +62,7 @@ class EncryptionController extends Controller { * @param \OC\DB\Connection $connection * @param IUserManager $userManager * @param View $view + * @param ILogger $logger */ public function __construct($appName, IRequest $request, @@ -65,7 +70,8 @@ class EncryptionController extends Controller { IConfig $config, Connection $connection, IUserManager $userManager, - View $view) { + View $view, + ILogger $logger) { parent::__construct($appName, $request); $this->l10n = $l10n; $this->config = $config; @@ -85,7 +91,7 @@ class EncryptionController extends Controller { try { - $migration = new Migration($this->config, $this->view, $this->connection); + $migration = new Migration($this->config, $this->view, $this->connection, $this->logger); $migration->reorganizeSystemFolderStructure(); $migration->updateDB(); From 4dba920fddef822695212f0387623ce65d349f29 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 17 Jul 2015 14:47:19 +0200 Subject: [PATCH 2/2] unit tests --- apps/encryption/tests/lib/MigrationTest.php | 38 +++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/apps/encryption/tests/lib/MigrationTest.php b/apps/encryption/tests/lib/MigrationTest.php index 786bce33a2..a83909ac62 100644 --- a/apps/encryption/tests/lib/MigrationTest.php +++ b/apps/encryption/tests/lib/MigrationTest.php @@ -105,6 +105,17 @@ class MigrationTest extends \Test\TestCase { $this->view->file_put_contents($uid . '/files_encryption/keys/folder2/file.2.1/fileKey' , 'data'); } + protected function createDummyFiles($uid) { + $this->view->mkdir($uid . '/files/folder1/folder2/folder3/file3'); + $this->view->mkdir($uid . '/files/folder1/folder2/file2'); + $this->view->mkdir($uid . '/files/folder1/file.1'); + $this->view->mkdir($uid . '/files/folder2/file.2.1'); + $this->view->file_put_contents($uid . '/files/folder1/folder2/folder3/file3/fileKey' , 'data'); + $this->view->file_put_contents($uid . '/files/folder1/folder2/file2/fileKey' , 'data'); + $this->view->file_put_contents($uid . '/files/folder1/file.1/fileKey' , 'data'); + $this->view->file_put_contents($uid . '/files/folder2/file.2.1/fileKey' , 'data'); + } + protected function createDummyFilesInTrash($uid) { $this->view->mkdir($uid . '/files_trashbin/keys/file1.d5457864'); $this->view->mkdir($uid . '/files_trashbin/keys/folder1.d7437648723/file2'); @@ -114,6 +125,11 @@ class MigrationTest extends \Test\TestCase { $this->view->file_put_contents($uid . '/files_trashbin/keys/file1.d5457864/fileKey' , 'data'); $this->view->file_put_contents($uid . '/files_trashbin/keys/folder1.d7437648723/file2/fileKey' , 'data'); + + // create the files itself + $this->view->mkdir($uid . '/files_trashbin/folder1.d7437648723'); + $this->view->file_put_contents($uid . '/files_trashbin/file1.d5457864' , 'data'); + $this->view->file_put_contents($uid . '/files_trashbin/folder1.d7437648723/file2' , 'data'); } protected function createDummySystemWideKeys() { @@ -123,7 +139,6 @@ class MigrationTest extends \Test\TestCase { $this->view->file_put_contents('files_encryption/systemwide_2.privateKey', 'data'); $this->view->file_put_contents('files_encryption/public_keys/systemwide_1.publicKey', 'data'); $this->view->file_put_contents('files_encryption/public_keys/systemwide_2.publicKey', 'data'); - } public function testMigrateToNewFolderStructure() { @@ -139,6 +154,10 @@ class MigrationTest extends \Test\TestCase { $this->createDummyFileKeys(self::TEST_ENCRYPTION_MIGRATION_USER2); $this->createDummyFileKeys(self::TEST_ENCRYPTION_MIGRATION_USER3); + $this->createDummyFiles(self::TEST_ENCRYPTION_MIGRATION_USER1); + $this->createDummyFiles(self::TEST_ENCRYPTION_MIGRATION_USER2); + $this->createDummyFiles(self::TEST_ENCRYPTION_MIGRATION_USER3); + $this->createDummyFilesInTrash(self::TEST_ENCRYPTION_MIGRATION_USER2); // no user for system wide mount points @@ -147,7 +166,22 @@ class MigrationTest extends \Test\TestCase { $this->createDummySystemWideKeys(); - $m = new Migration(\OC::$server->getConfig(), new \OC\Files\View(), \OC::$server->getDatabaseConnection(), $this->logger); + $m = $this->getMockBuilder('OCA\Encryption\Migration') + ->setConstructorArgs( + [ + \OC::$server->getConfig(), + new \OC\Files\View(), + \OC::$server->getDatabaseConnection(), + $this->logger + ] + )->setMethods(['getSystemMountPoints'])->getMock(); + + $m->expects($this->any())->method('getSystemMountPoints') + ->willReturn([['mountpoint' => 'folder1'], ['mountpoint' => 'folder2']]); + + //$m = new Migration(\OC::$server->getConfig(), new \OC\Files\View(), \OC::$server->getDatabaseConnection(), $this->logger); + $m->reorganizeFolderStructure(); + // even if it runs twice folder should always move only once $m->reorganizeFolderStructure(); $this->assertTrue(