From fcda3a20f455795b898161ec4ada0aeb500b9218 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Mon, 2 Jan 2017 21:24:37 +0100 Subject: [PATCH] create new encryption keys on password reset and backup the old one Signed-off-by: Bjoern Schiessle --- apps/encryption/lib/Hooks/UserHooks.php | 61 +++++++++++++------ apps/encryption/lib/KeyManager.php | 8 +-- apps/encryption/tests/Hooks/UserHooksTest.php | 47 ++++++++++---- apps/encryption/tests/KeyManagerTest.php | 6 ++ core/Controller/LostController.php | 7 +-- core/js/lostpassword.js | 2 +- lib/private/Encryption/Keys/Storage.php | 35 +++++++++++ lib/public/Encryption/Keys/IStorage.php | 10 +++ tests/Core/Controller/LostControllerTest.php | 38 ------------ tests/lib/Encryption/Keys/StorageTest.php | 43 +++++++++++++ 10 files changed, 176 insertions(+), 81 deletions(-) diff --git a/apps/encryption/lib/Hooks/UserHooks.php b/apps/encryption/lib/Hooks/UserHooks.php index 16e4e962432..d189ce3eeef 100644 --- a/apps/encryption/lib/Hooks/UserHooks.php +++ b/apps/encryption/lib/Hooks/UserHooks.php @@ -40,6 +40,13 @@ use OCA\Encryption\Session; use OCA\Encryption\Recovery; class UserHooks implements IHook { + + /** + * list of user for which we perform a password reset + * @var array + */ + protected static $passwordResetUsers = []; + /** * @var KeyManager */ @@ -132,6 +139,16 @@ class UserHooks implements IHook { $this, 'preSetPassphrase'); + OCUtil::connectHook('\OC\Core\LostPassword\Controller\LostController', + 'post_passwordReset', + $this, + 'postPasswordReset'); + + OCUtil::connectHook('\OC\Core\LostPassword\Controller\LostController', + 'pre_passwordReset', + $this, + 'prePasswordReset'); + OCUtil::connectHook('OC_User', 'post_createUser', $this, @@ -202,6 +219,22 @@ class UserHooks implements IHook { } } + public function prePasswordReset($params) { + if (App::isEnabled('encryption')) { + $user = $params['uid']; + self::$passwordResetUsers[$user] = true; + } + } + + public function postPasswordReset($params) { + $uid = $params['uid']; + $password = $params['password']; + $this->keyManager->backupUserKeys('passwordReset', $uid); + $this->keyManager->deleteUserKeys($uid); + $this->userSetup->setupUser($uid, $password); + unset(self::$passwordResetUsers[$uid]); + } + /** * If the password can't be changed within ownCloud, than update the key password in advance. * @@ -209,13 +242,10 @@ class UserHooks implements IHook { * @return boolean|null */ public function preSetPassphrase($params) { - if (App::isEnabled('encryption')) { - - $user = $this->userManager->get($params['uid']); + $user = $this->userManager->get($params['uid']); - if ($user && !$user->canChangePassword()) { - $this->setPassphrase($params); - } + if ($user && !$user->canChangePassword()) { + $this->setPassphrase($params); } } @@ -227,6 +257,12 @@ class UserHooks implements IHook { */ public function setPassphrase($params) { + // if we are in the process to resetting a user password, we have nothing + // to do here + if (isset(self::$passwordResetUsers[$params['uid']])) { + return true; + } + // Get existing decrypted private key $privateKey = $this->session->getPrivateKey(); $user = $this->user->getUser(); @@ -299,19 +335,6 @@ class UserHooks implements IHook { Filesystem::initMountPoints($user); } - - /** - * after password reset we create a new key pair for the user - * - * @param array $params - */ - public function postPasswordReset($params) { - $password = $params['password']; - - $this->keyManager->deleteUserKeys($params['uid']); - $this->userSetup->setupUser($params['uid'], $password); - } - /** * setup file system for user * diff --git a/apps/encryption/lib/KeyManager.php b/apps/encryption/lib/KeyManager.php index 26f023ed8f9..caae154b2d3 100644 --- a/apps/encryption/lib/KeyManager.php +++ b/apps/encryption/lib/KeyManager.php @@ -560,11 +560,10 @@ class KeyManager { /** * @param string $purpose - * @param bool $timestamp - * @param bool $includeUserKeys + * @param string $uid */ - public function backupAllKeys($purpose, $timestamp = true, $includeUserKeys = true) { -// $backupDir = $this->keyStorage->; + public function backupUserKeys($purpose, $uid) { + $this->keyStorage->backupUserKeys(Encryption::ID, $purpose, $uid); } /** @@ -573,7 +572,6 @@ class KeyManager { * @param string $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 43cc54f8901..f9477e3e038 100644 --- a/apps/encryption/tests/Hooks/UserHooksTest.php +++ b/apps/encryption/tests/Hooks/UserHooksTest.php @@ -120,6 +120,31 @@ class UserHooksTest extends TestCase { $this->assertTrue(true); } + public function testPrePasswordReset() { + $params = ['uid' => 'user1']; + $expected = ['user1' => true]; + $this->instance->prePasswordReset($params); + $passwordResetUsers = $this->invokePrivate($this->instance, 'passwordResetUsers'); + + $this->assertSame($expected, $passwordResetUsers); + } + + public function testPostPasswordReset() { + $params = ['uid' => 'user1', 'password' => 'password']; + $this->invokePrivate($this->instance, 'passwordResetUsers', [['user1' => true]]); + $this->keyManagerMock->expects($this->once())->method('backupUserKeys') + ->with('passwordReset', 'user1'); + $this->keyManagerMock->expects($this->once())->method('deleteUserKeys') + ->with('user1'); + $this->userSetupMock->expects($this->once())->method('setupUser') + ->with('user1', 'password'); + + $this->instance->postPasswordReset($params); + $passwordResetUsers = $this->invokePrivate($this->instance, 'passwordResetUsers'); + $this->assertEmpty($passwordResetUsers); + + } + /** * @dataProvider dataTestPreSetPassphrase */ @@ -252,6 +277,15 @@ class UserHooksTest extends TestCase { $this->assertNull($this->instance->setPassphrase($this->params)); } + public function testSetPassphraseResetUserMode() { + $params = ['uid' => 'user1', 'password' => 'password']; + $this->invokePrivate($this->instance, 'passwordResetUsers', [[$params['uid'] => true]]); + $this->sessionMock->expects($this->never())->method('getPrivateKey'); + $this->keyManagerMock->expects($this->never())->method('setPrivateKey'); + $this->assertTrue($this->instance->setPassphrase($params)); + $this->invokePrivate($this->instance, 'passwordResetUsers', [[]]); + } + public function testSetPasswordNoUser() { $this->sessionMock->expects($this->once()) ->method('getPrivateKey') @@ -287,19 +321,6 @@ class UserHooksTest extends TestCase { $this->assertNull($userHooks->setPassphrase($this->params)); } - public function testPostPasswordReset() { - $this->keyManagerMock->expects($this->once()) - ->method('deleteUserKeys') - ->with('testUser'); - - $this->userSetupMock->expects($this->once()) - ->method('setupUser') - ->with('testUser', 'password'); - - $this->instance->postPasswordReset($this->params); - $this->assertTrue(true); - } - protected function setUp() { parent::setUp(); $this->loggerMock = $this->createMock(ILogger::class); diff --git a/apps/encryption/tests/KeyManagerTest.php b/apps/encryption/tests/KeyManagerTest.php index fec311afa35..40def135816 100644 --- a/apps/encryption/tests/KeyManagerTest.php +++ b/apps/encryption/tests/KeyManagerTest.php @@ -657,4 +657,10 @@ class KeyManagerTest extends TestCase { $this->instance->setVersion('/admin/files/myfile.txt', 5, $view); } + public function testBackupUserKeys() { + $this->keyStorageMock->expects($this->once())->method('backupUserKeys') + ->with('OC_DEFAULT_MODULE', 'test', 'user1'); + $this->instance->backupUserKeys('test', 'user1'); + } + } diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php index b12abf38142..a0ef87e50d8 100644 --- a/core/Controller/LostController.php +++ b/core/Controller/LostController.php @@ -234,6 +234,8 @@ class LostController extends Controller { $this->checkPasswordResetToken($token, $userId); $user = $this->userManager->get($userId); + \OC_Hook::emit('\OC\Core\LostPassword\Controller\LostController', 'pre_passwordReset', array('uid' => $userId, 'password' => $password)); + if (!$user->setPassword($password)) { throw new \Exception(); } @@ -242,11 +244,6 @@ class LostController extends Controller { $this->config->deleteUserValue($userId, 'core', 'lostpassword'); @\OC_User::unsetMagicInCookie(); - } catch (PrivateKeyMissingException $e) { - // in this case it is OK if we couldn't reset the users private key - // They chose explicitely to continue at the password reset dialog - // (see $proceed flag) - return $this->success(); } catch (\Exception $e){ return $this->error($e->getMessage()); } diff --git a/core/js/lostpassword.js b/core/js/lostpassword.js index 30d7b98f4e8..6e18dcc1f8b 100644 --- a/core/js/lostpassword.js +++ b/core/js/lostpassword.js @@ -4,7 +4,7 @@ OC.Lostpassword = { sendSuccessMsg : t('core', 'The link to reset your password has been sent to your email. If you do not receive it within a reasonable amount of time, check your spam/junk folders.
If it is not there ask your local administrator.'), - encryptedMsg : t('core', "Your files are encrypted. If you haven't enabled the recovery key, there will be no way to get your data back after your password is reset.
If you are not sure what to do, please contact your administrator before you continue.
Do you really want to continue?") + encryptedMsg : t('core', "Your files are encrypted. There will be no way to get your data back after your password is reset.
If you are not sure what to do, please contact your administrator before you continue.
Do you really want to continue?") + ('
') + '