fix(tests): Fix type issues and other problems with encryption tests

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>

apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php

@@ -1,11 +1,13 @@
 <?php
 
 declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
  * SPDX-License-Identifier: AGPL-3.0-only
  */
+
 namespace OCA\DAV\Tests\unit\Connector\Sabre\RequestTest;
 
 use OCP\AppFramework\Http;

apps/encryption/lib/Crypto/Crypt.php

@@ -342,9 +342,8 @@ class Crypt {
 	 * @param string $privateKey
 	 * @param string $password
 	 * @param string $uid for regular users, empty for system keys
-	 * @return false|string
 	 */
-	public function decryptPrivateKey($privateKey, $password = '', $uid = '') {
+	public function decryptPrivateKey($privateKey, $password = '', $uid = '') : string|false {
 		$header = $this->parseHeader($privateKey);
 
 		if (isset($header['cipher'])) {

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

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

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

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

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 {

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

tests/lib/Encryption/DecryptAllTest.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
@@ -16,6 +18,7 @@ use OC\Files\View;
 use OCP\Files\Storage\IStorage;
 use OCP\IUserManager;
 use OCP\UserInterface;
+use PHPUnit\Framework\MockObject\MockObject;
 use Symfony\Component\Console\Formatter\OutputFormatterInterface;
 use Symfony\Component\Console\Helper\ProgressBar;
 use Symfony\Component\Console\Input\InputInterface;
@@ -30,26 +33,14 @@ use Test\TestCase;
  * @package Test\Encryption
  */
 class DecryptAllTest extends TestCase {
-	/** @var \PHPUnit\Framework\MockObject\MockObject | IUserManager */
-	protected $userManager;
+	private IUserManager&MockObject $userManager;
+	private Manager&MockObject $encryptionManager;
+	private View&MockObject $view;
+	private InputInterface&MockObject $inputInterface;
+	private OutputInterface&MockObject $outputInterface;
+	private UserInterface&MockObject $userInterface;
 
-	/** @var \PHPUnit\Framework\MockObject\MockObject | Manager */
-	protected $encryptionManager;
-
-	/** @var \PHPUnit\Framework\MockObject\MockObject | View */
-	protected $view;
-
-	/** @var \PHPUnit\Framework\MockObject\MockObject | \Symfony\Component\Console\Input\InputInterface */
-	protected $inputInterface;
-
-	/** @var \PHPUnit\Framework\MockObject\MockObject | \Symfony\Component\Console\Output\OutputInterface */
-	protected $outputInterface;
-
-	/** @var \PHPUnit\Framework\MockObject\MockObject|UserInterface */
-	protected $userInterface;
-
-	/** @var DecryptAll */
-	protected $instance;
+	private DecryptAll $instance;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -93,19 +84,14 @@ class DecryptAllTest extends TestCase {
 		];
 	}
 
-	/**
-	 * @param bool $prepareResult
-	 * @param string $user
-	 * @param bool $userExistsChecked
-	 */
 	#[\PHPUnit\Framework\Attributes\DataProvider('dataDecryptAll')]
-	public function testDecryptAll($prepareResult, $user, $userExistsChecked): void {
+	public function testDecryptAll(bool $prepareResult, string $user, bool $userExistsChecked): void {
 		if ($userExistsChecked) {
 			$this->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(
 				[

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],
 		];
 	}
 }

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() {