Merge pull request #15938 from owncloud/enc_update_recovery_share_key

[encryption] check recovery key setting for the correct user

8 files changed, 92 insertions, 44 deletions

diff --git a/apps/encryption/lib/crypto/encryption.php b/apps/encryption/lib/crypto/encryption.php
index 29fda09e87f..a4abcd7dc5a 100644
--- a/apps/encryption/lib/crypto/encryption.php
+++ b/apps/encryption/lib/crypto/encryption.php
@@ -190,7 +190,7 @@ class Encryption implements IEncryptionModule {
 				}
 			}
 
-			$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);
@@ -287,7 +287,7 @@ class Encryption implements IEncryptionModule {
 	 */
 	public function update($path, $uid, array $accessList) {
 		$fileKey = $this->keyManager->getFileKey($path, $uid);
-
+		
 		if (!empty($fileKey)) {
 
 			$publicKeys = array();
@@ -295,7 +295,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);
 
@@ -314,28 +314,6 @@ class Encryption implements IEncryptionModule {
 	}
 
 	/**
-	 * 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
 	 *
 	 * @param string $path
diff --git a/apps/encryption/lib/keymanager.php b/apps/encryption/lib/keymanager.php
index bde8212e9dc..aa9614812bc 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 cfaa3e49619..61a659e484e 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 04e04028caf..51d5241122f 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 01e1bdab0ea..003a27da71d 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 251628d99f2..6e9c6d15818 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 b3fd403949c..5bfafa3a98e 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 5f086a8e475..eab912b82d4 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() {