Merge pull request #61243 from nextcloud/backport/60801/stable34

[stable34] fix(encryption): Fix endpoint /ajax/userSetRecovery to support boolean

Author: artonge

apps/encryption/lib/Controller/RecoveryController.php

@@ -117,37 +117,31 @@ public function changeRecoveryPassword(string $newPassword, string $oldPassword,
 		}
 	}
 
-	/**
-	 * @param string $userEnableRecovery
-	 * @return DataResponse
-	 */
 	#[NoAdminRequired]
-	public function userSetRecovery($userEnableRecovery) {
-		if ($userEnableRecovery === '0' || $userEnableRecovery === '1') {
-			$result = $this->recovery->setRecoveryForUser($userEnableRecovery);
+	public function userSetRecovery(bool $userEnableRecovery): DataResponse {
+		$result = $this->recovery->setRecoveryForUser($userEnableRecovery);
 
-			if ($result) {
-				if ($userEnableRecovery === '0') {
-					return new DataResponse(
-						[
-							'data' => [
-								'message' => $this->l->t('Recovery Key disabled')]
-						]
-					);
-				}
+		if ($result) {
+			if (!$userEnableRecovery) {
 				return new DataResponse(
 					[
 						'data' => [
-							'message' => $this->l->t('Recovery Key enabled')]
+							'message' => $this->l->t('Recovery Key disabled')]
 					]
 				);
 			}
+			return new DataResponse(
+				[
+					'data' => [
+						'message' => $this->l->t('Recovery Key enabled')]
+				]
+			);
 		}
+
 		return new DataResponse(
 			[
 				'data' => [
-					'message' => $this->l->t('Could not enable the recovery key, please try again or contact your administrator')
-				]
+					'message' => $this->l->t('Could not enable the recovery key, please try again or contact your administrator')]
 			], Http::STATUS_BAD_REQUEST);
 	}
 }

apps/encryption/lib/Recovery.php

@@ -16,19 +16,8 @@
 use OCP\PreConditionNotMetException;
 
 class Recovery {
-	/**
-	 * @var null|IUser
-	 */
-	protected $user;
+	protected ?IUser $user;
 
-	/**
-	 * @param IUserSession $userSession
-	 * @param Crypt $crypt
-	 * @param KeyManager $keyManager
-	 * @param IConfig $config
-	 * @param IFile $file
-	 * @param View $view
-	 */
 	public function __construct(
 		IUserSession $userSession,
 		protected Crypt $crypt,
@@ -40,11 +29,7 @@ public function __construct(
 		$this->user = ($userSession->isLoggedIn()) ? $userSession->getUser() : null;
 	}
 
-	/**
-	 * @param string $password
-	 * @return bool
-	 */
-	public function enableAdminRecovery($password) {
+	public function enableAdminRecovery(string $password): bool {
 		$appConfig = $this->config;
 		$keyManager = $this->keyManager;
 
@@ -83,11 +68,7 @@ public function changeRecoveryKeyPassword(string $newPassword, string $oldPasswo
 		return false;
 	}
 
-	/**
-	 * @param string $recoveryPassword
-	 * @return bool
-	 */
-	public function disableAdminRecovery($recoveryPassword) {
+	public function disableAdminRecovery(string $recoveryPassword): bool {
 		$keyManager = $this->keyManager;
 
 		if ($keyManager->checkRecoveryPassword($recoveryPassword)) {
@@ -102,42 +83,34 @@ public function disableAdminRecovery($recoveryPassword) {
 	 * check if recovery is enabled for user
 	 *
 	 * @param string $user if no user is given we check the current logged-in user
-	 *
-	 * @return bool
 	 */
-	public function isRecoveryEnabledForUser($user = '') {
+	public function isRecoveryEnabledForUser(string $user = ''): bool {
 		$uid = $user === '' ? $this->user->getUID() : $user;
 		$recoveryMode = $this->config->getUserValue($uid,
 			'encryption',
 			'recoveryEnabled',
-			0);
+			'0');
 
 		return ($recoveryMode === '1');
 	}
 
 	/**
 	 * check if recovery is key is enabled by the administrator
-	 *
-	 * @return bool
 	 */
-	public function isRecoveryKeyEnabled() {
+	public function isRecoveryKeyEnabled(): bool {
 		$enabled = $this->config->getAppValue('encryption', 'recoveryAdminEnabled', '0');
 
 		return ($enabled === '1');
 	}
 
-	/**
-	 * @param string $value
-	 * @return bool
-	 */
-	public function setRecoveryForUser($value) {
+	public function setRecoveryForUser(bool $value): bool {
 		try {
 			$this->config->setUserValue($this->user->getUID(),
 				'encryption',
 				'recoveryEnabled',
-				$value);
+				$value ? '1' : '0');
 
-			if ($value === '1') {
+			if ($value) {
 				$this->addRecoveryKeys('/' . $this->user->getUID() . '/files/');
 			} else {
 				$this->removeRecoveryKeys('/' . $this->user->getUID() . '/files/');

apps/encryption/lib/Util.php

@@ -82,11 +82,7 @@ public function isMasterKeyEnabled(): bool {
 		return ($userMasterKey === '1');
 	}
 
-	/**
-	 * @param $enabled
-	 * @return bool
-	 */
-	public function setRecoveryForUser($enabled) {
+	public function setRecoveryForUser(bool $enabled): bool {
 		$value = $enabled ? '1' : '0';
 
 		try {

apps/encryption/tests/Controller/RecoveryControllerTest.php

@@ -100,24 +100,19 @@ public function testChangeRecoveryPassword($password, $confirmPassword, $oldPass
 
 	public static function userSetRecoveryProvider(): array {
 		return [
-			['1', 'Recovery Key enabled', Http::STATUS_OK],
-			['0', 'Could not enable the recovery key, please try again or contact your administrator', Http::STATUS_BAD_REQUEST]
+			[true, 'Recovery Key enabled', Http::STATUS_OK],
+			[false, 'Could not enable the recovery key, please try again or contact your administrator', Http::STATUS_BAD_REQUEST]
 		];
 	}
 
-	/**
-	 * @param $enableRecovery
-	 * @param $expectedMessage
-	 * @param $expectedStatus
-	 */
 	#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'userSetRecoveryProvider')]
-	public function testUserSetRecovery($enableRecovery, $expectedMessage, $expectedStatus): void {
+	public function testUserSetRecovery(bool $enableRecovery, string $expectedMessage, int $expectedStatus): void {
 		$this->recoveryMock->expects($this->any())
 			->method('setRecoveryForUser')
 			->with($enableRecovery)
 			->willReturnMap([
-				['1', true],
-				['0', false]
+				[true, true],
+				[false, false]
 			]);
 
 

apps/encryption/tests/RecoveryTest.php

@@ -164,8 +164,8 @@ public function testSetRecoveryFolderForUser(): void {
 		$this->viewMock->expects($this->exactly(2))
 			->method('getDirectoryContent')
 			->willReturn([]);
-		$this->assertTrue($this->instance->setRecoveryForUser(0));
-		$this->assertTrue($this->instance->setRecoveryForUser('1'));
+		$this->assertTrue($this->instance->setRecoveryForUser(false));
+		$this->assertTrue($this->instance->setRecoveryForUser(true));
 	}
 
 	public function testRecoverUserFiles(): void {

apps/encryption/tests/UtilTest.php

@@ -32,7 +32,7 @@ class UtilTest extends TestCase {
 	protected IMountPoint&MockObject $mountMock;
 
 	public function testSetRecoveryForUser(): void {
-		$this->instance->setRecoveryForUser('1');
+		$this->instance->setRecoveryForUser(true);
 		$this->assertArrayHasKey('recoveryEnabled', self::$tempStorage);
 	}
 

apps/settings/lib/Controller/ChangePasswordController.php

@@ -173,20 +173,22 @@ public function changeUserPassword(?string $username = null, ?string $password =
 						],
 					]);
 				}
-				if (!$result && $recoveryEnabledForUser) {
-					return new JSONResponse([
-						'status' => 'error',
-						'data' => [
-							'message' => $this->l->t('Backend does not support password change, but the encryption of the account key was updated.'),
-						]
-					]);
-				} elseif (!$result && !$recoveryEnabledForUser) {
-					return new JSONResponse([
-						'status' => 'error',
-						'data' => [
-							'message' => $this->l->t('Unable to change password'),
-						]
-					]);
+				if (!$result) {
+					if ($recoveryEnabledForUser) {
+						return new JSONResponse([
+							'status' => 'error',
+							'data' => [
+								'message' => $this->l->t('Backend does not support password change, but the encryption of the account key was updated.'),
+							]
+						]);
+					} else {
+						return new JSONResponse([
+							'status' => 'error',
+							'data' => [
+								'message' => $this->l->t('Unable to change password'),
+							]
+						]);
+					}
 				}
 			}
 		} else {