diff --git a/apps/encryption/lib/keymanager.php b/apps/encryption/lib/keymanager.php index 0c8418c67a..8fa42be27f 100644 --- a/apps/encryption/lib/keymanager.php +++ b/apps/encryption/lib/keymanager.php @@ -213,7 +213,7 @@ class KeyManager { } /** - * @param $password + * @param string $password * @return bool */ public function checkRecoveryPassword($password) { diff --git a/apps/encryption/lib/recovery.php b/apps/encryption/lib/recovery.php index cffa641f51..98d8c79a40 100644 --- a/apps/encryption/lib/recovery.php +++ b/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); + } } /** diff --git a/apps/encryption/tests/lib/RecoveryTest.php b/apps/encryption/tests/lib/RecoveryTest.php index 0cdd76d6b4..82f5341960 100644 --- a/apps/encryption/tests/lib/RecoveryTest.php +++ b/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() {