Merge pull request #21027 from owncloud/fix-undefined-behaviour

Fix PHPDoc + Add handling for error cases

apps/encryption/lib/keymanager.php

@@ -213,7 +213,7 @@ class KeyManager {
 	}
 
 	/**
-	 * @param $password
+	 * @param string $password
 	 * @return bool
 	 */
 	public function checkRecoveryPassword($password) {

apps/encryption/lib/recovery.php

@@ -68,10 +68,6 @@ class Recovery {
 	 * @var IFile
 	 */
 	private $file;
-	/**
-	 * @var string
-	 */
-	private $recoveryKeyId;
 
 	/**
 	 * @param IUserSession $user
@@ -102,7 +98,6 @@ class Recovery {
 	}
 
 	/**
-	 * @param $recoveryKeyId
 	 * @param string $password
 	 * @return bool
 	 */
@@ -112,6 +107,9 @@ class Recovery {
 
 		if (!$keyManager->recoveryKeyExists()) {
 			$keyPair = $this->crypt->createKeyPair();
+			if(!is_array($keyPair)) {
+				return false;
+			}
 
 			$this->keyManager->setRecoveryKey($password, $keyPair);
 		}
@@ -134,6 +132,9 @@ class Recovery {
 	public function changeRecoveryKeyPassword($newPassword, $oldPassword) {
 		$recoveryKey = $this->keyManager->getSystemPrivateKey($this->keyManager->getRecoveryKeyId());
 		$decryptedRecoveryKey = $this->crypt->decryptPrivateKey($recoveryKey, $oldPassword);
+		if($decryptedRecoveryKey === false) {
+			return false;
+		}
 		$encryptedRecoveryKey = $this->crypt->encryptPrivateKey($decryptedRecoveryKey, $newPassword);
 		$header = $this->crypt->generateHeader();
 		if ($encryptedRecoveryKey) {
@@ -264,8 +265,9 @@ class Recovery {
 		$encryptedKey = $this->keyManager->getSystemPrivateKey($this->keyManager->getRecoveryKeyId());
 
 		$privateKey = $this->crypt->decryptPrivateKey($encryptedKey, $recoveryPassword);
-
-		$this->recoverAllFiles('/' . $user . '/files/', $privateKey, $user);
+		if($privateKey !== false) {
+			$this->recoverAllFiles('/' . $user . '/files/', $privateKey, $user);
+		}
 	}
 
 	/**

apps/encryption/tests/lib/RecoveryTest.php

@@ -59,14 +59,44 @@ class RecoveryTest extends TestCase {
 	 */
 	private $instance;
 
-	public function testEnableAdminRecovery() {
+	public function testEnableAdminRecoverySuccessful() {
 		$this->keyManagerMock->expects($this->exactly(2))
 			->method('recoveryKeyExists')
 			->willReturnOnConsecutiveCalls(false, true);
 
 		$this->cryptMock->expects($this->once())
 			->method('createKeyPair')
-			->willReturn(true);
+			->willReturn([
+				'publicKey' => 'privateKey',
+				'privateKey' => 'publicKey',
+			]);
+
+		$this->keyManagerMock->expects($this->once())
+			->method('setRecoveryKey')
+			->willReturn(false);
+
+		$this->keyManagerMock->expects($this->exactly(2))
+			->method('checkRecoveryPassword')
+			->willReturnOnConsecutiveCalls(true, true);
+
+		$this->assertTrue($this->instance->enableAdminRecovery('password'));
+		$this->assertArrayHasKey('recoveryAdminEnabled', self::$tempStorage);
+		$this->assertEquals(1, self::$tempStorage['recoveryAdminEnabled']);
+
+		$this->assertTrue($this->instance->enableAdminRecovery('password'));
+	}
+
+	public function testEnableAdminRecoveryCouldNotCheckPassword() {
+		$this->keyManagerMock->expects($this->exactly(2))
+			->method('recoveryKeyExists')
+			->willReturnOnConsecutiveCalls(false, true);
+
+		$this->cryptMock->expects($this->once())
+			->method('createKeyPair')
+			->willReturn([
+					'publicKey' => 'privateKey',
+					'privateKey' => 'publicKey',
+			]);
 
 		$this->keyManagerMock->expects($this->once())
 			->method('setRecoveryKey')
@@ -83,7 +113,19 @@ class RecoveryTest extends TestCase {
 		$this->assertFalse($this->instance->enableAdminRecovery('password'));
 	}
 
-	public function testChangeRecoveryKeyPassword() {
+	public function testEnableAdminRecoveryCouldNotCreateKey() {
+		$this->keyManagerMock->expects($this->once())
+			->method('recoveryKeyExists')
+			->willReturn(false);
+
+		$this->cryptMock->expects($this->once())
+			->method('createKeyPair')
+			->willReturn(false);
+
+		$this->assertFalse($this->instance->enableAdminRecovery('password'));
+	}
+
+	public function testChangeRecoveryKeyPasswordSuccessful() {
 		$this->assertFalse($this->instance->changeRecoveryKeyPassword('password',
 			'passwordOld'));
 
@@ -101,6 +143,19 @@ class RecoveryTest extends TestCase {
 			'passwordOld'));
 	}
 
+	public function testChangeRecoveryKeyPasswordCouldNotDecryptPrivateRecoveryKey() {
+		$this->assertFalse($this->instance->changeRecoveryKeyPassword('password', 'passwordOld'));
+
+		$this->keyManagerMock->expects($this->once())
+			->method('getSystemPrivateKey');
+
+		$this->cryptMock->expects($this->once())
+			->method('decryptPrivateKey')
+			->will($this->returnValue(false));
+
+		$this->assertFalse($this->instance->changeRecoveryKeyPassword('password', 'passwordOld'));
+	}
+
 	public function testDisableAdminRecovery() {
 
 		$this->keyManagerMock->expects($this->exactly(2))
@@ -145,8 +200,7 @@ class RecoveryTest extends TestCase {
 
 		$this->cryptMock->expects($this->once())
 			->method('decryptPrivateKey');
-		$this->assertNull($this->instance->recoverUsersFiles('password',
-			'admin'));
+		$this->assertNull($this->instance->recoverUsersFiles('password', 'admin'));
 	}
 
 	public function testRecoverFile() {