From 5e267589d40400223e5dce692568ab2933be14f7 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Wed, 2 Mar 2016 13:58:06 +0100 Subject: [PATCH 1/2] only create and update user specific key if no master key is enabled --- apps/encryption/appinfo/application.php | 5 ++ apps/encryption/hooks/userhooks.php | 58 +++++++++++------- apps/encryption/lib/keymanager.php | 8 ++- apps/encryption/lib/users/setup.php | 24 +++----- apps/encryption/tests/hooks/UserHooksTest.php | 31 ++++++---- apps/encryption/tests/lib/users/SetupTest.php | 59 ++++++++++++------- tests/lib/traits/encryptiontrait.php | 2 +- 7 files changed, 114 insertions(+), 73 deletions(-) diff --git a/apps/encryption/appinfo/application.php b/apps/encryption/appinfo/application.php index 6d01d3e835..6080a29d5f 100644 --- a/apps/encryption/appinfo/application.php +++ b/apps/encryption/appinfo/application.php @@ -66,6 +66,11 @@ class Application extends \OCP\AppFramework\App { $session = $this->getContainer()->query('Session'); $session->setStatus(Session::RUN_MIGRATION); } + if ($this->encryptionManager->isEnabled() && $encryptionSystemReady) { + /** @var Setup $setup */ + $setup = $this->getContainer()->query('UserSetup'); + $setup->setupSystem(); + } } /** diff --git a/apps/encryption/hooks/userhooks.php b/apps/encryption/hooks/userhooks.php index 62acd16890..08b6c15518 100644 --- a/apps/encryption/hooks/userhooks.php +++ b/apps/encryption/hooks/userhooks.php @@ -117,22 +117,29 @@ class UserHooks implements IHook { public function addHooks() { OCUtil::connectHook('OC_User', 'post_login', $this, 'login'); OCUtil::connectHook('OC_User', 'logout', $this, 'logout'); - OCUtil::connectHook('OC_User', - 'post_setPassword', - $this, - 'setPassphrase'); - OCUtil::connectHook('OC_User', - 'pre_setPassword', - $this, - 'preSetPassphrase'); - OCUtil::connectHook('OC_User', - 'post_createUser', - $this, - 'postCreateUser'); - OCUtil::connectHook('OC_User', - 'post_deleteUser', - $this, - 'postDeleteUser'); + + // this hooks only make sense if no master key is used + if ($this->util->isMasterKeyEnabled() === false) { + OCUtil::connectHook('OC_User', + 'post_setPassword', + $this, + 'setPassphrase'); + + OCUtil::connectHook('OC_User', + 'pre_setPassword', + $this, + 'preSetPassphrase'); + + OCUtil::connectHook('OC_User', + 'post_createUser', + $this, + 'postCreateUser'); + + OCUtil::connectHook('OC_User', + 'post_deleteUser', + $this, + 'postDeleteUser'); + } } @@ -151,12 +158,10 @@ class UserHooks implements IHook { // ensure filesystem is loaded if (!\OC\Files\Filesystem::$loaded) { - \OC_Util::setupFS($params['uid']); + $this->setupFS($params['uid']); } - - // setup user, if user not ready force relogin - if (!$this->userSetup->setupUser($params['uid'], $params['password'])) { - return false; + if ($this->util->isMasterKeyEnabled() === false) { + $this->userSetup->setupUser($params['uid'], $params['password']); } $this->keyManager->init($params['uid'], $params['password']); @@ -292,6 +297,15 @@ class UserHooks implements IHook { $password = $params['password']; $this->keyManager->replaceUserKeys($params['uid']); - $this->userSetup->setupServerSide($params['uid'], $password); + $this->userSetup->setupUser($params['uid'], $password); + } + + /** + * setup file system for user + * + * @param string $uid user id + */ + protected function setupFS($uid) { + \OC_Util::setupFS($uid); } } diff --git a/apps/encryption/lib/keymanager.php b/apps/encryption/lib/keymanager.php index 12fa5f92bd..1b81936aed 100644 --- a/apps/encryption/lib/keymanager.php +++ b/apps/encryption/lib/keymanager.php @@ -174,6 +174,11 @@ class KeyManager { * check if a key pair for the master key exists, if not we create one */ public function validateMasterKey() { + + if ($this->util->isMasterKeyEnabled() === false) { + return; + } + $masterKey = $this->getPublicMasterKey(); if (empty($masterKey)) { $keyPair = $this->crypt->createKeyPair(); @@ -334,7 +339,7 @@ class KeyManager { /** * Decrypt private key and store it * - * @param string $uid userid + * @param string $uid user id * @param string $passPhrase users password * @return boolean */ @@ -342,7 +347,6 @@ class KeyManager { $this->session->setStatus(Session::INIT_EXECUTED); - try { if($this->util->isMasterKeyEnabled()) { $uid = $this->getMasterKeyId(); diff --git a/apps/encryption/lib/users/setup.php b/apps/encryption/lib/users/setup.php index 0b5fb351ac..e59340c4ce 100644 --- a/apps/encryption/lib/users/setup.php +++ b/apps/encryption/lib/users/setup.php @@ -66,29 +66,23 @@ class Setup { } /** - * @param string $uid userid + * @param string $uid user id * @param string $password user password * @return bool */ public function setupUser($uid, $password) { - return $this->setupServerSide($uid, $password); - } - - /** - * check if user has a key pair, if not we create one - * - * @param string $uid userid - * @param string $password user password - * @return bool - */ - public function setupServerSide($uid, $password) { - $this->keyManager->validateShareKey(); - $this->keyManager->validateMasterKey(); - // Check if user already has keys if (!$this->keyManager->userHasKeys($uid)) { return $this->keyManager->storeKeyPair($uid, $password, $this->crypt->createKeyPair()); } return true; } + + /** + * make sure that all system keys exists + */ + public function setupSystem() { + $this->keyManager->validateShareKey(); + $this->keyManager->validateMasterKey(); + } } diff --git a/apps/encryption/tests/hooks/UserHooksTest.php b/apps/encryption/tests/hooks/UserHooksTest.php index 08d1981266..1aeafad0ba 100644 --- a/apps/encryption/tests/hooks/UserHooksTest.php +++ b/apps/encryption/tests/hooks/UserHooksTest.php @@ -71,7 +71,7 @@ class UserHooksTest extends TestCase { private $params = ['uid' => 'testUser', 'password' => 'password']; public function testLogin() { - $this->userSetupMock->expects($this->exactly(2)) + $this->userSetupMock->expects($this->once()) ->method('setupUser') ->willReturnOnConsecutiveCalls(true, false); @@ -80,7 +80,6 @@ class UserHooksTest extends TestCase { ->with('testUser', 'password'); $this->assertNull($this->instance->login($this->params)); - $this->assertFalse($this->instance->login($this->params)); } public function testLogout() { @@ -256,7 +255,7 @@ class UserHooksTest extends TestCase { ->with('testUser'); $this->userSetupMock->expects($this->once()) - ->method('setupServerSide') + ->method('setupUser') ->with('testUser', 'password'); $this->assertNull($this->instance->postPasswordReset($this->params)); @@ -312,16 +311,22 @@ class UserHooksTest extends TestCase { $this->sessionMock = $sessionMock; $this->recoveryMock = $recoveryMock; $this->utilMock = $utilMock; - $this->instance = new UserHooks($this->keyManagerMock, - $this->userManagerMock, - $this->loggerMock, - $this->userSetupMock, - $this->userSessionMock, - $this->utilMock, - $this->sessionMock, - $this->cryptMock, - $this->recoveryMock - ); + $this->utilMock->expects($this->any())->method('isMasterKeyEnabled')->willReturn(false); + + $this->instance = $this->getMockBuilder('OCA\Encryption\Hooks\UserHooks') + ->setConstructorArgs( + [ + $this->keyManagerMock, + $this->userManagerMock, + $this->loggerMock, + $this->userSetupMock, + $this->userSessionMock, + $this->utilMock, + $this->sessionMock, + $this->cryptMock, + $this->recoveryMock + ] + )->setMethods(['setupFS'])->getMock(); } diff --git a/apps/encryption/tests/lib/users/SetupTest.php b/apps/encryption/tests/lib/users/SetupTest.php index 0cc59384b1..e7d8afbb10 100644 --- a/apps/encryption/tests/lib/users/SetupTest.php +++ b/apps/encryption/tests/lib/users/SetupTest.php @@ -41,26 +41,6 @@ class SetupTest extends TestCase { */ private $instance; - public function testSetupServerSide() { - $this->keyManagerMock->expects($this->exactly(2))->method('validateShareKey'); - $this->keyManagerMock->expects($this->exactly(2))->method('validateMasterKey'); - $this->keyManagerMock->expects($this->exactly(2)) - ->method('userHasKeys') - ->with('admin') - ->willReturnOnConsecutiveCalls(true, false); - - $this->assertTrue($this->instance->setupServerSide('admin', - 'password')); - - $this->keyManagerMock->expects($this->once()) - ->method('storeKeyPair') - ->with('admin', 'password') - ->willReturn(false); - - $this->assertFalse($this->instance->setupServerSide('admin', - 'password')); - } - protected function setUp() { parent::setUp(); $logMock = $this->getMock('OCP\ILogger'); @@ -81,4 +61,43 @@ class SetupTest extends TestCase { $this->keyManagerMock); } + + public function testSetupSystem() { + $this->keyManagerMock->expects($this->once())->method('validateShareKey'); + $this->keyManagerMock->expects($this->once())->method('validateMasterKey'); + + $this->instance->setupSystem(); + } + + /** + * @dataProvider dataTestSetupUser + * + * @param bool $hasKeys + * @param bool $expected + */ + public function testSetupUser($hasKeys, $expected) { + + $this->keyManagerMock->expects($this->once())->method('userHasKeys') + ->with('uid')->willReturn($hasKeys); + + if ($hasKeys) { + $this->keyManagerMock->expects($this->never())->method('storeKeyPair'); + } else { + $this->cryptMock->expects($this->once())->method('createKeyPair')->willReturn('keyPair'); + $this->keyManagerMock->expects($this->once())->method('storeKeyPair') + ->with('uid', 'password', 'keyPair')->willReturn(true); + } + + $this->assertSame($expected, + $this->instance->setupUser('uid', 'password') + ); + } + + public function dataTestSetupUser() { + return [ + [true, true], + [false, true] + ]; + } + } diff --git a/tests/lib/traits/encryptiontrait.php b/tests/lib/traits/encryptiontrait.php index 92ba373487..7fad5d826e 100644 --- a/tests/lib/traits/encryptiontrait.php +++ b/tests/lib/traits/encryptiontrait.php @@ -63,7 +63,7 @@ trait EncryptionTrait { $keyManager = $container->query('KeyManager'); /** @var Setup $userSetup */ $userSetup = $container->query('UserSetup'); - $userSetup->setupServerSide($name, $password); + $userSetup->setupUser($name, $password); $keyManager->init($name, $password); } From 89223379ad155ae0896d1481089e3751257aa76f Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Wed, 2 Mar 2016 19:25:44 +0100 Subject: [PATCH 2/2] replaceUserKeys() actually deletes the users keys -> update method name and doc-block --- apps/encryption/hooks/userhooks.php | 2 +- apps/encryption/lib/keymanager.php | 4 +++- apps/encryption/tests/hooks/UserHooksTest.php | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/apps/encryption/hooks/userhooks.php b/apps/encryption/hooks/userhooks.php index 08b6c15518..b1141030bd 100644 --- a/apps/encryption/hooks/userhooks.php +++ b/apps/encryption/hooks/userhooks.php @@ -296,7 +296,7 @@ class UserHooks implements IHook { public function postPasswordReset($params) { $password = $params['password']; - $this->keyManager->replaceUserKeys($params['uid']); + $this->keyManager->deleteUserKeys($params['uid']); $this->userSetup->setupUser($params['uid'], $password); } diff --git a/apps/encryption/lib/keymanager.php b/apps/encryption/lib/keymanager.php index 1b81936aed..f9a8f7bec3 100644 --- a/apps/encryption/lib/keymanager.php +++ b/apps/encryption/lib/keymanager.php @@ -557,9 +557,11 @@ class KeyManager { } /** + * creat a backup of the users private and public key and then delete it + * * @param string $uid */ - public function replaceUserKeys($uid) { + public function deleteUserKeys($uid) { $this->backupAllKeys('password_reset'); $this->deletePublicKey($uid); $this->deletePrivateKey($uid); diff --git a/apps/encryption/tests/hooks/UserHooksTest.php b/apps/encryption/tests/hooks/UserHooksTest.php index 1aeafad0ba..34cd74cd36 100644 --- a/apps/encryption/tests/hooks/UserHooksTest.php +++ b/apps/encryption/tests/hooks/UserHooksTest.php @@ -251,7 +251,7 @@ class UserHooksTest extends TestCase { public function testPostPasswordReset() { $this->keyManagerMock->expects($this->once()) - ->method('replaceUserKeys') + ->method('deleteUserKeys') ->with('testUser'); $this->userSetupMock->expects($this->once())