From bc5e29f9f2ddc8373e15026d89e0b19197987cc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 4 Sep 2025 10:53:10 +0200 Subject: [PATCH] fix(tests): Fix type issues and other problems with encryption tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../Sabre/RequestTest/UploadTest.php | 2 + apps/encryption/lib/Crypto/Crypt.php | 3 +- apps/encryption/lib/KeyManager.php | 6 +- apps/encryption/lib/Recovery.php | 8 +-- .../Controller/RecoveryControllerTest.php | 2 +- apps/encryption/tests/KeyManagerTest.php | 18 +++--- apps/encryption/tests/RecoveryTest.php | 59 +++++++------------ apps/encryption/tests/SessionTest.php | 4 +- tests/lib/Encryption/DecryptAllTest.php | 58 +++++++----------- .../Files/Storage/Wrapper/EncryptionTest.php | 34 +++++------ tests/lib/Traits/EncryptionTrait.php | 1 + 11 files changed, 80 insertions(+), 115 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php b/apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php index 5c6d0f03334..ec285363507 100644 --- a/apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php @@ -1,11 +1,13 @@ parseHeader($privateKey); if (isset($header['cipher'])) { diff --git a/apps/encryption/lib/KeyManager.php b/apps/encryption/lib/KeyManager.php index 429190d3698..c504e70ba51 100644 --- a/apps/encryption/lib/KeyManager.php +++ b/apps/encryption/lib/KeyManager.php @@ -136,7 +136,11 @@ class KeyManager { if (!$this->session->isPrivateKeySet()) { $masterKey = $this->getSystemPrivateKey($this->masterKeyId); $decryptedMasterKey = $this->crypt->decryptPrivateKey($masterKey, $this->getMasterKeyPassword(), $this->masterKeyId); - $this->session->setPrivateKey($decryptedMasterKey); + if ($decryptedMasterKey === false) { + $this->logger->error('A public master key is available but the decryption failed. This should never happen.'); + } else { + $this->session->setPrivateKey($decryptedMasterKey); + } } // after the encryption key is available we are ready to go diff --git a/apps/encryption/lib/Recovery.php b/apps/encryption/lib/Recovery.php index 38e78f5e822..9418e6fbea6 100644 --- a/apps/encryption/lib/Recovery.php +++ b/apps/encryption/lib/Recovery.php @@ -67,12 +67,8 @@ class Recovery { /** * change recovery key id - * - * @param string $newPassword - * @param string $oldPassword - * @return bool */ - public function changeRecoveryKeyPassword($newPassword, $oldPassword) { + public function changeRecoveryKeyPassword(string $newPassword, string $oldPassword): bool { $recoveryKey = $this->keyManager->getSystemPrivateKey($this->keyManager->getRecoveryKeyId()); $decryptedRecoveryKey = $this->crypt->decryptPrivateKey($recoveryKey, $oldPassword); if ($decryptedRecoveryKey === false) { @@ -80,7 +76,7 @@ class Recovery { } $encryptedRecoveryKey = $this->crypt->encryptPrivateKey($decryptedRecoveryKey, $newPassword); $header = $this->crypt->generateHeader(); - if ($encryptedRecoveryKey) { + if ($encryptedRecoveryKey !== false) { $this->keyManager->setSystemPrivateKey($this->keyManager->getRecoveryKeyId(), $header . $encryptedRecoveryKey); return true; } diff --git a/apps/encryption/tests/Controller/RecoveryControllerTest.php b/apps/encryption/tests/Controller/RecoveryControllerTest.php index 0fec3f4d6a9..717e31cf53b 100644 --- a/apps/encryption/tests/Controller/RecoveryControllerTest.php +++ b/apps/encryption/tests/Controller/RecoveryControllerTest.php @@ -85,7 +85,7 @@ class RecoveryControllerTest extends TestCase { ->method('changeRecoveryKeyPassword') ->with($password, $oldPassword) ->willReturnMap([ - ['test', 'oldTestFail', false], + ['test', 'oldtestFail', false], ['test', 'oldtest', true] ]); diff --git a/apps/encryption/tests/KeyManagerTest.php b/apps/encryption/tests/KeyManagerTest.php index 4c7a1fc6555..a6d895ce9f5 100644 --- a/apps/encryption/tests/KeyManagerTest.php +++ b/apps/encryption/tests/KeyManagerTest.php @@ -335,25 +335,25 @@ class KeyManagerTest extends TestCase { return [ ['user1', false, 'privateKey', 'legacyKey', 'multiKeyDecryptResult'], ['user1', false, 'privateKey', '', 'multiKeyDecryptResult'], - ['user1', false, false, 'legacyKey', ''], - ['user1', false, false, '', ''], + ['user1', false, '', 'legacyKey', ''], + ['user1', false, '', '', ''], ['user1', true, 'privateKey', 'legacyKey', 'multiKeyDecryptResult'], ['user1', true, 'privateKey', '', 'multiKeyDecryptResult'], - ['user1', true, false, 'legacyKey', ''], - ['user1', true, false, '', ''], + ['user1', true, '', 'legacyKey', ''], + ['user1', true, '', '', ''], [null, false, 'privateKey', 'legacyKey', 'multiKeyDecryptResult'], [null, false, 'privateKey', '', 'multiKeyDecryptResult'], - [null, false, false, 'legacyKey', ''], - [null, false, false, '', ''], + [null, false, '', 'legacyKey', ''], + [null, false, '', '', ''], [null, true, 'privateKey', 'legacyKey', 'multiKeyDecryptResult'], [null, true, 'privateKey', '', 'multiKeyDecryptResult'], - [null, true, false, 'legacyKey', ''], - [null, true, false, '', ''], + [null, true, '', 'legacyKey', ''], + [null, true, '', '', ''], ]; } #[\PHPUnit\Framework\Attributes\DataProvider('dataTestGetFileKey')] - public function testGetFileKey(?string $uid, bool $isMasterKeyEnabled, string|false $privateKey, string $encryptedFileKey, string $expected): void { + public function testGetFileKey(?string $uid, bool $isMasterKeyEnabled, string $privateKey, string $encryptedFileKey, string $expected): void { $path = '/foo.txt'; if ($isMasterKeyEnabled) { diff --git a/apps/encryption/tests/RecoveryTest.php b/apps/encryption/tests/RecoveryTest.php index 0627724a856..74d472adeec 100644 --- a/apps/encryption/tests/RecoveryTest.php +++ b/apps/encryption/tests/RecoveryTest.php @@ -7,6 +7,7 @@ declare(strict_types=1); * SPDX-FileCopyrightText: 2016 ownCloud, Inc. * SPDX-License-Identifier: AGPL-3.0-only */ + namespace OCA\Encryption\Tests; use OC\Files\View; @@ -22,38 +23,16 @@ use Test\TestCase; class RecoveryTest extends TestCase { private static $tempStorage = []; - /** - * @var IFile|\PHPUnit\Framework\MockObject\MockObject - */ - private $fileMock; - /** - * @var View|\PHPUnit\Framework\MockObject\MockObject - */ - private $viewMock; - /** - * @var IUserSession|\PHPUnit\Framework\MockObject\MockObject - */ - private $userSessionMock; - /** - * @var MockObject|IUser - */ - private $user; - /** - * @var KeyManager|\PHPUnit\Framework\MockObject\MockObject - */ - private $keyManagerMock; - /** - * @var IConfig|\PHPUnit\Framework\MockObject\MockObject - */ - private $configMock; - /** - * @var Crypt|\PHPUnit\Framework\MockObject\MockObject - */ - private $cryptMock; - /** - * @var Recovery - */ - private $instance; + + private IFile&MockObject $fileMock; + private View&MockObject $viewMock; + private IUserSession&MockObject $userSessionMock; + private IUser&MockObject $user; + private KeyManager&MockObject $keyManagerMock; + private IConfig&MockObject $configMock; + private Crypt&MockObject $cryptMock; + + private Recovery $instance; public function testEnableAdminRecoverySuccessful(): void { $this->keyManagerMock->expects($this->exactly(2)) @@ -122,21 +101,23 @@ class RecoveryTest extends TestCase { } public function testChangeRecoveryKeyPasswordSuccessful(): void { - $this->assertFalse($this->instance->changeRecoveryKeyPassword('password', - 'passwordOld')); + $this->assertFalse($this->instance->changeRecoveryKeyPassword('password', 'passwordOld')); $this->keyManagerMock->expects($this->once()) - ->method('getSystemPrivateKey'); + ->method('getSystemPrivateKey') + ->willReturn('privateKey'); $this->cryptMock->expects($this->once()) - ->method('decryptPrivateKey'); + ->method('decryptPrivateKey') + ->with('privateKey', 'passwordOld') + ->willReturn('decryptedPrivateKey'); $this->cryptMock->expects($this->once()) ->method('encryptPrivateKey') - ->willReturn(true); + ->with('decryptedPrivateKey', 'password') + ->willReturn('privateKey'); - $this->assertTrue($this->instance->changeRecoveryKeyPassword('password', - 'passwordOld')); + $this->assertTrue($this->instance->changeRecoveryKeyPassword('password', 'passwordOld')); } public function testChangeRecoveryKeyPasswordCouldNotDecryptPrivateRecoveryKey(): void { diff --git a/apps/encryption/tests/SessionTest.php b/apps/encryption/tests/SessionTest.php index 986502749c8..c24855e1ac4 100644 --- a/apps/encryption/tests/SessionTest.php +++ b/apps/encryption/tests/SessionTest.php @@ -76,7 +76,7 @@ class SessionTest extends TestCase { public function testGetDecryptAllUidException2(): void { $this->expectException(\Exception::class); - $this->instance->prepareDecryptAll(null, 'key'); + $this->instance->prepareDecryptAll('', 'key'); $this->instance->getDecryptAllUid(); } @@ -95,7 +95,7 @@ class SessionTest extends TestCase { public function testGetDecryptAllKeyException2(): void { $this->expectException(PrivateKeyMissingException::class); - $this->instance->prepareDecryptAll('user', null); + $this->instance->prepareDecryptAll('user', ''); $this->instance->getDecryptAllKey(); } diff --git a/tests/lib/Encryption/DecryptAllTest.php b/tests/lib/Encryption/DecryptAllTest.php index 979e12e03b3..5b56ac271c5 100644 --- a/tests/lib/Encryption/DecryptAllTest.php +++ b/tests/lib/Encryption/DecryptAllTest.php @@ -1,5 +1,7 @@ userManager->expects($this->once())->method('userExists')->willReturn(true); } else { $this->userManager->expects($this->never())->method('userExists'); } - /** @var DecryptAll | \PHPUnit\Framework\MockObject\MockObject | $instance */ + /** @var DecryptAll&MockObject $instance */ $instance = $this->getMockBuilder('OC\Encryption\DecryptAll') ->setConstructorArgs( [ @@ -119,13 +105,13 @@ class DecryptAllTest extends TestCase { $instance->expects($this->once()) ->method('prepareEncryptionModules') - ->with($user) + ->with($this->inputInterface, $this->outputInterface, $user) ->willReturn($prepareResult); if ($prepareResult) { $instance->expects($this->once()) ->method('decryptAllUsersFiles') - ->with($user); + ->with($this->outputInterface, $user); } else { $instance->expects($this->never())->method('decryptAllUsersFiles'); } @@ -182,13 +168,13 @@ class DecryptAllTest extends TestCase { ->willReturn([$moduleDescription]); $this->assertSame($success, - $this->invokePrivate($this->instance, 'prepareEncryptionModules', [$user]) + $this->invokePrivate($this->instance, 'prepareEncryptionModules', [$this->inputInterface, $this->outputInterface, $user]) ); } #[\PHPUnit\Framework\Attributes\DataProvider('dataTestDecryptAllUsersFiles')] public function testDecryptAllUsersFiles($user): void { - /** @var DecryptAll | \PHPUnit\Framework\MockObject\MockObject | $instance */ + /** @var DecryptAll&MockObject $instance */ $instance = $this->getMockBuilder('OC\Encryption\DecryptAll') ->setConstructorArgs( [ @@ -200,9 +186,6 @@ class DecryptAllTest extends TestCase { ->onlyMethods(['decryptUsersFiles']) ->getMock(); - $this->invokePrivate($instance, 'input', [$this->inputInterface]); - $this->invokePrivate($instance, 'output', [$this->outputInterface]); - if (empty($user)) { $this->userManager->expects($this->once()) ->method('getBackends') @@ -226,7 +209,7 @@ class DecryptAllTest extends TestCase { ->with($user); } - $this->invokePrivate($instance, 'decryptAllUsersFiles', [$user]); + $this->invokePrivate($instance, 'decryptAllUsersFiles', [$this->outputInterface, $user]); } public static function dataTestDecryptAllUsersFiles(): array { @@ -296,9 +279,10 @@ class DecryptAllTest extends TestCase { ]; $instance->expects($this->exactly(2)) ->method('decryptFile') - ->willReturnCallback(function ($path) use (&$calls): void { + ->willReturnCallback(function ($path) use (&$calls): bool { $expected = array_shift($calls); $this->assertEquals($expected, $path); + return true; }); @@ -320,7 +304,7 @@ class DecryptAllTest extends TestCase { public function testDecryptFile($isEncrypted): void { $path = 'test.txt'; - /** @var DecryptAll | \PHPUnit\Framework\MockObject\MockObject $instance */ + /** @var DecryptAll&MockObject $instance */ $instance = $this->getMockBuilder('OC\Encryption\DecryptAll') ->setConstructorArgs( [ @@ -360,7 +344,7 @@ class DecryptAllTest extends TestCase { public function testDecryptFileFailure(): void { $path = 'test.txt'; - /** @var DecryptAll | \PHPUnit\Framework\MockObject\MockObject $instance */ + /** @var DecryptAll&MockObject $instance */ $instance = $this->getMockBuilder('OC\Encryption\DecryptAll') ->setConstructorArgs( [ diff --git a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php index 3e643714300..cd82d200de8 100644 --- a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php +++ b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php @@ -936,19 +936,13 @@ class EncryptionTest extends Storage { ]; } - /** - * - * @param bool $encryptMountPoint - * @param mixed $encryptionModule - * @param bool $encryptionModuleShouldEncrypt - * @param bool $expected - */ #[\PHPUnit\Framework\Attributes\DataProvider('dataTestShouldEncrypt')] public function testShouldEncrypt( - $encryptMountPoint, - $encryptionModule, - $encryptionModuleShouldEncrypt, - $expected, + bool $encryptionEnabled, + bool $encryptMountPoint, + ?bool $encryptionModule, + bool $encryptionModuleShouldEncrypt, + bool $expected, ): void { $encryptionManager = $this->createMock(\OC\Encryption\Manager::class); $util = $this->createMock(Util::class); @@ -978,13 +972,15 @@ class EncryptionTest extends Storage { ->onlyMethods(['getFullPath', 'getEncryptionModule']) ->getMock(); + $encryptionManager->method('isEnabled')->willReturn($encryptionEnabled); + if ($encryptionModule === true) { /** @var IEncryptionModule|MockObject $encryptionModule */ $encryptionModule = $this->createMock(IEncryptionModule::class); } $wrapper->method('getFullPath')->with($path)->willReturn($fullPath); - $wrapper->expects($encryptMountPoint ? $this->once() : $this->never()) + $wrapper->expects(($encryptionEnabled && $encryptMountPoint) ? $this->once() : $this->never()) ->method('getEncryptionModule') ->with($fullPath) ->willReturnCallback( @@ -995,7 +991,8 @@ class EncryptionTest extends Storage { return $encryptionModule; } ); - $mount->expects($this->once())->method('getOption')->with('encrypt', true) + $mount->expects($encryptionEnabled ? $this->once() : $this->never()) + ->method('getOption')->with('encrypt', true) ->willReturn($encryptMountPoint); if ($encryptionModule !== null && $encryptionModule !== false) { @@ -1019,11 +1016,12 @@ class EncryptionTest extends Storage { public static function dataTestShouldEncrypt(): array { return [ - [false, false, false, false], - [true, false, false, false], - [true, true, false, false], - [true, true, true, true], - [true, null, false, true], + [true, false, false, false, false], + [true, true, false, false, false], + [true, true, true, false, false], + [true, true, true, true, true], + [true, true, null, false, true], + [false, true, true, true, false], ]; } } diff --git a/tests/lib/Traits/EncryptionTrait.php b/tests/lib/Traits/EncryptionTrait.php index 44d5dcab8cb..2dc9213ff24 100644 --- a/tests/lib/Traits/EncryptionTrait.php +++ b/tests/lib/Traits/EncryptionTrait.php @@ -82,6 +82,7 @@ trait EncryptionTrait { $encryptionManager = $container->query(IManager::class); $this->encryptionApp->setUp($encryptionManager); $keyManager->init($name, $password); + $this->invokePrivate($keyManager, 'keyUid', [$name]); } protected function postLogin() {