Merge pull request #2918 from nextcloud/encryption-recovery-improvements

create new encryption keys on password reset and backup the old one

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,15 +242,12 @@ class UserHooks implements IHook {
 	 * @return boolean|null
 	 */
 	public function preSetPassphrase($params) {
-		if (App::isEnabled('encryption')) {
-
 		$user = $this->userManager->get($params['uid']);
 
 		if ($user && !$user->canChangePassword()) {
 			$this->setPassphrase($params);
 		}
 	}
-	}
 
 	/**
 	 * Change a user's encryption passphrase
@@ -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
 	 *

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);
 	}

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);

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');
+	}
+
 }

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());
 		}

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.<br>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.<br />If you are not sure what to do, please contact your administrator before you continue. <br />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.<br />If you are not sure what to do, please contact your administrator before you continue. <br />Do you really want to continue?")
 			+ ('<br /><input type="checkbox" id="encrypted-continue" value="Yes" />')
 			+ '<label for="encrypted-continue">'
 			+ t('core', 'I know what I\'m doing')

lib/private/Encryption/Keys/Storage.php

@@ -51,6 +51,9 @@ class Storage implements IStorage {
 	/** @var string */
 	private $encryption_base_dir;
 
+	/** @var string */
+	private $backup_base_dir;
+
 	/** @var array */
 	private $keyCache = [];
 
@@ -64,6 +67,7 @@ class Storage implements IStorage {
 
 		$this->encryption_base_dir = '/files_encryption';
 		$this->keys_base_dir = $this->encryption_base_dir .'/keys';
+		$this->backup_base_dir = $this->encryption_base_dir .'/backup';
 		$this->root_dir = $this->util->getKeyStorageRoot();
 	}
 
@@ -286,6 +290,37 @@ class Storage implements IStorage {
 		return false;
 	}
 
+	/**
+	 * backup keys of a given encryption module
+	 *
+	 * @param string $encryptionModuleId
+	 * @param string $purpose
+	 * @param string $uid
+	 * @return bool
+	 * @since 12.0.0
+	 */
+	public function backupUserKeys($encryptionModuleId, $purpose, $uid) {
+		$source = $uid . $this->encryption_base_dir . '/' . $encryptionModuleId;
+		$backupDir = $uid . $this->backup_base_dir;
+		if (!$this->view->file_exists($backupDir)) {
+			$this->view->mkdir($backupDir);
+		}
+
+		$backupDir = $backupDir . '/' . $purpose . '.' . $encryptionModuleId . '.' . $this->getTimestamp();
+		$this->view->mkdir($backupDir);
+
+		return $this->view->copy($source, $backupDir);
+	}
+
+	/**
+	 * get the current timestamp
+	 *
+	 * @return int
+	 */
+	protected function getTimestamp() {
+		return time();
+	}
+
 	/**
 	 * get system wide path and detect mount points
 	 *

lib/public/Encryption/Keys/IStorage.php

@@ -170,4 +170,14 @@ interface IStorage {
 	 */
 	public function copyKeys($source, $target);
 
+	/**
+	 * backup keys of a given encryption module
+	 *
+	 * @param string $encryptionModuleId
+	 * @param string $purpose
+	 * @param string $uid
+	 * @return bool
+	 * @since 12.0.0
+	 */
+	public function backupUserKeys($encryptionModuleId, $purpose, $uid);
 }

tests/Core/Controller/LostControllerTest.php

@@ -591,42 +591,4 @@ class LostControllerTest extends \Test\TestCase {
 		$this->assertSame($expectedResponse, $response);
 	}
 
-	public function testSetPasswordEncryptionProceed() {
-
-		/** @var LostController | PHPUnit_Framework_MockObject_MockObject $lostController */
-		$lostController = $this->getMockBuilder(LostController::class)
-			->setConstructorArgs(
-				[
-					'Core',
-					$this->request,
-					$this->urlGenerator,
-					$this->userManager,
-					$this->defaults,
-					$this->l10n,
-					$this->config,
-					$this->secureRandom,
-					'lostpassword-noreply@localhost',
-					$this->encryptionManager,
-					$this->mailer,
-					$this->timeFactory,
-					$this->crypto
-				]
-			)->setMethods(['checkPasswordResetToken'])->getMock();
-
-		$lostController->expects($this->once())->method('checkPasswordResetToken')->willReturn(true);
-
-		$user = $this->createMock(IUser::class);
-		$user->method('setPassword')->willReturnCallback(
-			function() {
-				throw new PrivateKeyMissingException('user');
-			}
-		);
-		$this->userManager->method('get')->with('user')->willReturn($user);
-
-		$response = $lostController->setPassword('myToken', 'user', 'newpass', true);
-
-		$expectedResponse = ['status' => 'success'];
-		$this->assertSame($expectedResponse, $response);
-	}
-
 }

tests/lib/Encryption/Keys/StorageTest.php

@@ -510,4 +510,47 @@ class StorageTest extends TestCase {
 		];
 	}
 
+
+	/**
+	 * @dataProvider dataTestBackupUserKeys
+	 * @param bool $createBackupDir
+	 */
+	public function testBackupUserKeys($createBackupDir) {
+
+		$storage = $this->getMockBuilder('OC\Encryption\Keys\Storage')
+			->setConstructorArgs([$this->view, $this->util])
+			->setMethods(['getTimestamp'])
+			->getMock();
+
+		$storage->expects($this->any())->method('getTimestamp')->willReturn('1234567');
+
+		$this->view->expects($this->once())->method('file_exists')
+			->with('user1/files_encryption/backup')->willReturn(!$createBackupDir);
+
+		if ($createBackupDir) {
+			$this->view->expects($this->at(1))->method('mkdir')
+				->with('user1/files_encryption/backup');
+			$this->view->expects($this->at(2))->method('mkdir')
+				->with('user1/files_encryption/backup/test.encryptionModule.1234567');
+		} else {
+			$this->view->expects($this->once())->method('mkdir')
+				->with('user1/files_encryption/backup/test.encryptionModule.1234567');
+		}
+
+		$this->view->expects($this->once())->method('copy')
+			->with(
+				'user1/files_encryption/encryptionModule',
+				'user1/files_encryption/backup/test.encryptionModule.1234567'
+			)->willReturn(true);
+
+		$this->assertTrue($storage->backupUserKeys('encryptionModule', 'test', 'user1'));
+
+	}
+
+	public function dataTestBackupUserKeys() {
+		return [
+			[true], [false]
+		];
+	}
+
 }