From 70a44621beac44e258b46ff17e1d68d86e18d00d Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Wed, 29 Apr 2015 17:18:41 +0200 Subject: [PATCH 1/2] check recovery setting for the right user --- apps/encryption/lib/crypto/encryption.php | 28 +-------- apps/encryption/lib/keymanager.php | 5 +- apps/encryption/lib/recovery.php | 28 +++++---- apps/encryption/lib/util.php | 7 ++- .../encryption/settings/settings-personal.php | 2 +- apps/encryption/tests/lib/KeyManagerTest.php | 59 +++++++++++++++++++ apps/encryption/tests/lib/RecoveryTest.php | 3 +- apps/encryption/tests/lib/UtilTest.php | 4 +- 8 files changed, 92 insertions(+), 44 deletions(-) diff --git a/apps/encryption/lib/crypto/encryption.php b/apps/encryption/lib/crypto/encryption.php index 4e181b0712..c622836b33 100644 --- a/apps/encryption/lib/crypto/encryption.php +++ b/apps/encryption/lib/crypto/encryption.php @@ -174,7 +174,7 @@ class Encryption implements IEncryptionModule { $publicKeys[$uid] = $this->keyManager->getPublicKey($uid); } - $publicKeys = $this->keyManager->addSystemKeys($this->accessList, $publicKeys); + $publicKeys = $this->keyManager->addSystemKeys($this->accessList, $publicKeys, $this->user); $encryptedKeyfiles = $this->crypt->multiKeyEncrypt($this->fileKey, $publicKeys); $this->keyManager->setAllFileKeys($this->path, $encryptedKeyfiles); @@ -271,7 +271,7 @@ class Encryption implements IEncryptionModule { */ public function update($path, $uid, array $accessList) { $fileKey = $this->keyManager->getFileKey($path, $uid); - + if (!empty($fileKey)) { $publicKeys = array(); @@ -279,7 +279,7 @@ class Encryption implements IEncryptionModule { $publicKeys[$user] = $this->keyManager->getPublicKey($user); } - $publicKeys = $this->keyManager->addSystemKeys($accessList, $publicKeys); + $publicKeys = $this->keyManager->addSystemKeys($accessList, $publicKeys, $uid); $encryptedFileKey = $this->crypt->multiKeyEncrypt($fileKey, $publicKeys); @@ -297,28 +297,6 @@ class Encryption implements IEncryptionModule { return true; } - /** - * add system keys such as the public share key and the recovery key - * - * @param array $accessList - * @param array $publicKeys - * @return array - */ - public function addSystemKeys(array $accessList, array $publicKeys) { - if (!empty($accessList['public'])) { - $publicKeys[$this->keyManager->getPublicShareKeyId()] = $this->keyManager->getPublicShareKey(); - } - - if ($this->keyManager->recoveryKeyExists() && - $this->util->isRecoveryEnabledForUser()) { - - $publicKeys[$this->keyManager->getRecoveryKeyId()] = $this->keyManager->getRecoveryKey(); - } - - return $publicKeys; - } - - /** * should the file be encrypted or not * diff --git a/apps/encryption/lib/keymanager.php b/apps/encryption/lib/keymanager.php index bde8212e9d..aa9614812b 100644 --- a/apps/encryption/lib/keymanager.php +++ b/apps/encryption/lib/keymanager.php @@ -531,10 +531,11 @@ class KeyManager { * * @param array $accessList * @param array $publicKeys + * @param string $uid * @return array * @throws PublicKeyMissingException */ - public function addSystemKeys(array $accessList, array $publicKeys) { + public function addSystemKeys(array $accessList, array $publicKeys, $uid) { if (!empty($accessList['public'])) { $publicShareKey = $this->getPublicShareKey(); if (empty($publicShareKey)) { @@ -544,7 +545,7 @@ class KeyManager { } if ($this->recoveryKeyExists() && - $this->util->isRecoveryEnabledForUser()) { + $this->util->isRecoveryEnabledForUser($uid)) { $publicKeys[$this->getRecoveryKeyId()] = $this->getRecoveryKey(); } diff --git a/apps/encryption/lib/recovery.php b/apps/encryption/lib/recovery.php index cfaa3e4961..61a659e484 100644 --- a/apps/encryption/lib/recovery.php +++ b/apps/encryption/lib/recovery.php @@ -228,7 +228,7 @@ class Recovery { $publicKeys[$uid] = $this->keyManager->getPublicKey($uid); } - $publicKeys = $this->keyManager->addSystemKeys($accessList, $publicKeys); + $publicKeys = $this->keyManager->addSystemKeys($accessList, $publicKeys, $this->user->getUID()); $encryptedKeyfiles = $this->crypt->multiKeyEncrypt($fileKey, $publicKeys); $this->keyManager->setAllFileKeys($filePath, $encryptedKeyfiles); @@ -264,33 +264,39 @@ class Recovery { $privateKey = $this->crypt->decryptPrivateKey($encryptedKey, $recoveryPassword); - $this->recoverAllFiles('/' . $user . '/files/', $privateKey); + $this->recoverAllFiles('/' . $user . '/files/', $privateKey, $user); } /** - * @param $path - * @param $privateKey + * recover users files + * + * @param string $path + * @param string $privateKey + * @param string $uid */ - private function recoverAllFiles($path, $privateKey) { + private function recoverAllFiles($path, $privateKey, $uid) { $dirContent = $this->view->getDirectoryContent($path); foreach ($dirContent as $item) { // Get relative path from encryption/keyfiles $filePath = $item->getPath(); if ($this->view->is_dir($filePath)) { - $this->recoverAllFiles($filePath . '/', $privateKey); + $this->recoverAllFiles($filePath . '/', $privateKey, $uid); } else { - $this->recoverFile($filePath, $privateKey); + $this->recoverFile($filePath, $privateKey, $uid); } } } /** + * recover file + * * @param string $path * @param string $privateKey + * @param string $uid */ - private function recoverFile($path, $privateKey) { + private function recoverFile($path, $privateKey, $uid) { $encryptedFileKey = $this->keyManager->getEncryptedFileKey($path); $shareKey = $this->keyManager->getShareKey($path, $this->keyManager->getRecoveryKeyId()); @@ -303,11 +309,11 @@ class Recovery { if (!empty($fileKey)) { $accessList = $this->file->getAccessList($path); $publicKeys = array(); - foreach ($accessList['users'] as $uid) { - $publicKeys[$uid] = $this->keyManager->getPublicKey($uid); + foreach ($accessList['users'] as $user) { + $publicKeys[$user] = $this->keyManager->getPublicKey($user); } - $publicKeys = $this->keyManager->addSystemKeys($accessList, $publicKeys); + $publicKeys = $this->keyManager->addSystemKeys($accessList, $publicKeys, $uid); $encryptedKeyfiles = $this->crypt->multiKeyEncrypt($fileKey, $publicKeys); $this->keyManager->setAllFileKeys($path, $encryptedKeyfiles); diff --git a/apps/encryption/lib/util.php b/apps/encryption/lib/util.php index 04e04028ca..51d5241122 100644 --- a/apps/encryption/lib/util.php +++ b/apps/encryption/lib/util.php @@ -77,10 +77,13 @@ class Util { } /** + * check if recovery key is enabled for user + * + * @param string $uid * @return bool */ - public function isRecoveryEnabledForUser() { - $recoveryMode = $this->config->getUserValue($this->user->getUID(), + public function isRecoveryEnabledForUser($uid) { + $recoveryMode = $this->config->getUserValue($uid, 'encryption', 'recoveryEnabled', 0); diff --git a/apps/encryption/settings/settings-personal.php b/apps/encryption/settings/settings-personal.php index 01e1bdab0e..003a27da71 100644 --- a/apps/encryption/settings/settings-personal.php +++ b/apps/encryption/settings/settings-personal.php @@ -56,7 +56,7 @@ $privateKeySet = $session->isPrivateKeySet(); $initialized = $session->getStatus(); $recoveryAdminEnabled = \OC::$server->getConfig()->getAppValue('encryption', 'recoveryAdminEnabled'); -$recoveryEnabledForUser = $util->isRecoveryEnabledForUser(); +$recoveryEnabledForUser = $util->isRecoveryEnabledForUser($user); $result = false; diff --git a/apps/encryption/tests/lib/KeyManagerTest.php b/apps/encryption/tests/lib/KeyManagerTest.php index 251628d99f..6e9c6d1581 100644 --- a/apps/encryption/tests/lib/KeyManagerTest.php +++ b/apps/encryption/tests/lib/KeyManagerTest.php @@ -297,4 +297,63 @@ class KeyManagerTest extends TestCase { $this->assertTrue($this->instance->deleteAllFileKeys('/')); } + + /** + * test add public share key and or recovery key to the list of public keys + * + * @dataProvider dataTestAddSystemKeys + * + * @param array $accessList + * @param array $publicKeys + * @param string $uid + * @param array $expectedKeys + */ + public function testAddSystemKeys($accessList, $publicKeys, $uid, $expectedKeys) { + + $publicShareKeyId = 'publicShareKey'; + $recoveryKeyId = 'recoveryKey'; + + $this->keyStorageMock->expects($this->any()) + ->method('getSystemUserKey') + ->willReturnCallback(function($keyId, $encryptionModuleId) { + return $keyId; + }); + + $this->utilMock->expects($this->any()) + ->method('isRecoveryEnabledForUser') + ->willReturnCallback(function($uid) { + if ($uid === 'user1') { + return true; + } + return false; + }); + + // set key IDs + \Test_Helper::invokePrivate($this->instance, 'publicShareKeyId', [$publicShareKeyId]); + \Test_Helper::invokePrivate($this->instance, 'recoveryKeyId', [$recoveryKeyId]); + + $result = $this->instance->addSystemKeys($accessList, $publicKeys, $uid); + + foreach ($expectedKeys as $expected) { + $this->assertArrayHasKey($expected, $result); + } + + $this->assertSameSize($expectedKeys, $result); + } + + /** + * data provider for testAddSystemKeys() + * + * @return array + */ + public function dataTestAddSystemKeys() { + return array( + array(['public' => true],[], 'user1', ['publicShareKey', 'recoveryKey']), + array(['public' => false], [], 'user1', ['recoveryKey']), + array(['public' => true],[], 'user2', ['publicShareKey']), + array(['public' => false], [], 'user2', []), + ); + } + + } diff --git a/apps/encryption/tests/lib/RecoveryTest.php b/apps/encryption/tests/lib/RecoveryTest.php index b3fd403949..5bfafa3a98 100644 --- a/apps/encryption/tests/lib/RecoveryTest.php +++ b/apps/encryption/tests/lib/RecoveryTest.php @@ -170,6 +170,7 @@ class RecoveryTest extends TestCase { $this->keyManagerMock->expects($this->once()) ->method('addSystemKeys') + ->with($this->anything(), $this->anything(), $this->equalTo('admin')) ->willReturn(['admin' => 'publicKey']); @@ -181,7 +182,7 @@ class RecoveryTest extends TestCase { $this->assertNull(\Test_Helper::invokePrivate($this->instance, 'recoverFile', - ['/', 'testkey'])); + ['/', 'testkey', 'admin'])); } protected function setUp() { diff --git a/apps/encryption/tests/lib/UtilTest.php b/apps/encryption/tests/lib/UtilTest.php index 5f086a8e47..eab912b82d 100644 --- a/apps/encryption/tests/lib/UtilTest.php +++ b/apps/encryption/tests/lib/UtilTest.php @@ -44,11 +44,11 @@ class UtilTest extends TestCase { * */ public function testIsRecoveryEnabledForUser() { - $this->assertTrue($this->instance->isRecoveryEnabledForUser()); + $this->assertTrue($this->instance->isRecoveryEnabledForUser('admin')); // Assert recovery will return default value if not set unset(self::$tempStorage['recoveryEnabled']); - $this->assertEquals(0, $this->instance->isRecoveryEnabledForUser()); + $this->assertEquals(0, $this->instance->isRecoveryEnabledForUser('admin')); } public function testUserHasFiles() { From 31b65749dd69baded46e5924082db5de856e7cea Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 27 Apr 2015 15:22:48 +0200 Subject: [PATCH 2/2] Allow setting protected properties --- tests/lib/helper.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/lib/helper.php b/tests/lib/helper.php index 53a3e1a0ec..ed15a67730 100644 --- a/tests/lib/helper.php +++ b/tests/lib/helper.php @@ -523,6 +523,10 @@ class Test_Helper extends \Test\TestCase { $property->setAccessible(true); + if (!empty($parameters)) { + $property->setValue($object, array_pop($parameters)); + } + return $property->getValue($object); }