chore: fixup DecryptAll.php

Signed-off-by: Josh <josh.t.richards@gmail.com>

apps/encryption/lib/Crypto/EncryptAll.php

@@ -10,7 +10,7 @@ declare(strict_types=1);
 
 namespace OCA\Encryption\Crypto;
 
-use OC\Encryption\Exceptions\DecryptionFailedException;
+use OC\Encryption\Exceptions\EncryptionFailedException;
 use OC\Files\SetupManager;
 use OC\Files\View;
 use OCA\Encryption\KeyManager;
@@ -33,6 +33,9 @@ use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Question\ConfirmationQuestion;
 
+/**
+ * Handles bulk encryption of files for users.
+ */
 class EncryptAll {
 
 	/** @var array<string, array{password: string, user: IUser}> $userCache store one time passwords for the users */
@@ -168,7 +171,10 @@ class EncryptAll {
 	}
 
 	/**
-	 * encrypt files from the given user
+	 * Encrypt all files from the given user.
+	 *
+	 * Recursively traverses the user's files directory, skipping files and folders not owned by the user,
+	 * and attempts to encrypt each file.
 	 */
 	protected function encryptUsersFiles(IUser $user, ProgressBar $progress, string $userCount): void {
 		$this->setupUserFileSystem($user);
@@ -178,64 +184,63 @@ class EncryptAll {
 
 		while ($root = array_pop($directories)) {
 			$content = $this->rootView->getDirectoryContent($root);
+			/** @var FileInfo $file */
 			foreach ($content as $file) {
 				$path = $root . '/' . $file->getName();
-				if ($file->isShared()) {
-					$progress->setMessage("Skip shared file/folder $path");
+
+				if ($file->getOwner() !== $uid) {
+					$progress->setMessage("Skipping shared/unowned file/folder $path");
 					$progress->advance();
 					continue;
-				} elseif ($file->getType() === FileInfo::TYPE_FOLDER) {
+				}
+				
+				if ($file->getType() === FileInfo::TYPE_FOLDER) {
 					$directories[] = $path;
 					continue;
-				} else {
-					$progress->setMessage("encrypt files for user $userCount: $path");
-					$progress->advance();
-					try {
-						if ($this->encryptFile($file, $path) === false) {
-							$progress->setMessage("encrypt files for user $userCount: $path (already encrypted)");
-							$progress->advance();
-						}
-					} catch (\Exception $e) {
-						$progress->setMessage("Failed to encrypt path $path: " . $e->getMessage());
+				}
+				
+				$progress->setMessage("Encrypting file for user $userCount: $path");
+				$progress->advance();
+
+				try {
+					if ($this->encryptFile($file, $path) === false) {
+						$progress->setMessage("Skipping already encrypted file $path for user $userCount");
 						$progress->advance();
-						$this->logger->error(
-							'Failed to encrypt path {path}',
-							[
-								'user' => $uid,
-								'path' => $path,
-								'exception' => $e,
-							]
-						);
 					}
+				} catch (\Exception $e) {
+					$progress->setMessage("Failed to encrypt path $path: " . $e->getMessage());
+					$progress->advance();
+					$this->logger->error('Failed to encrypt path {path}', [ 'user' => $uid, 'path' => $path, 'exception' => $e, ]);
 				}
 			}
 		}
 	}
 
 	protected function encryptFile(FileInfo $fileInfo, string $path): bool {
-		// skip already encrypted files
 		if ($fileInfo->isEncrypted()) {
-			return true;
+			return false;
 		}
 
 		$source = $path;
 		$target = $path . '.encrypted.' . time();
 
 		try {
-			$copySuccess = $this->rootView->copy($source, $target);
-			if ($copySuccess === false) {
-				/* Copy failed, abort */
-				if ($this->rootView->file_exists($target)) {
-					$this->rootView->unlink($target);
-				}
-				throw new \Exception('Copy failed for ' . $source);
+			if ($this->rootView->copy($source, $target) === false) {
+				throw new EncryptionFailedException("Failed to copy $source -> $target");
 			}
-			$this->rootView->rename($target, $source);
-		} catch (DecryptionFailedException $e) {
+
+			if ($this->rootView->rename($target, $source) === false) {
+				throw new EncryptionFailedException("Failed to rename $target -> $source");
+			}
+		} catch (\Exception $e) {
 			if ($this->rootView->file_exists($target)) {
+				$this->logger->debug(
+					"Cleaning up failed temp file $target after encryption exception",
+					[ 'user' => $fileInfo->getOwner(), 'path' => $path, ]
+				);
 				$this->rootView->unlink($target);
 			}
-			return false;
+			throw $e;
 		}
 
 		return true;

lib/private/Encryption/DecryptAll.php

@@ -11,23 +11,33 @@ declare(strict_types=1);
 namespace OC\Encryption;
 
 use OC\Encryption\Exceptions\DecryptionFailedException;
+use OC\Files\FileInfo;
 use OC\Files\View;
 use OCP\Encryption\IEncryptionModule;
 use OCP\Encryption\IManager;
 use OCP\IUserManager;
+use Psr\Log\LoggerInterface;
 use Symfony\Component\Console\Helper\ProgressBar;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 
+/**
+ * DecryptAll handles bulk decryption of files for users.
+ */
 class DecryptAll {
 	/** @var array<string,list<string>> files which couldn't be decrypted */
 	protected array $failed = [];
 
+	protected readonly LoggerInterface $logger;
+
 	public function __construct(
-		protected IManager $encryptionManager,
-		protected IUserManager $userManager,
-		protected View $rootView,
+		protected readonly IManager $encryptionManager,
+		protected readonly IUserManager $userManager,
+		protected readonly View $rootView,
 	) {
+		// TODO: Inject LoggerInterface
+		$this->logger = \OC::$server->get(LoggerInterface::class);
+		// TODO: Inject SetupManager
 	}
 
 	/**
@@ -51,6 +61,7 @@ class DecryptAll {
 		$this->failed = [];
 		$this->decryptAllUsersFiles($output, $user);
 
+		// TODO: Drop below output; maybe still use $this->failed to return false (if we can't switch to void)
 		/** @psalm-suppress RedundantCondition $this->failed is modified by decryptAllUsersFiles, not clear why psalm fails to see it */
 		if (empty($this->failed)) {
 			$output->writeln('all files could be decrypted successfully!');
@@ -91,7 +102,7 @@ class DecryptAll {
 	}
 
 	/**
-	 * iterate over all user and encrypt their files
+	 * iterate over all user and decrypt their files
 	 *
 	 * @param string $user which users files should be decrypted, default = all users
 	 */
@@ -129,7 +140,7 @@ class DecryptAll {
 		$progress = new ProgressBar($output);
 		$progress->setFormat(" %message% \n [%bar%]");
 		$progress->start();
-		$progress->setMessage('starting to decrypt files...');
+		$progress->setMessage('Decrypting files...');
 		$progress->advance();
 
 		$numberOfUsers = count($userList);
@@ -140,50 +151,57 @@ class DecryptAll {
 			$userNo++;
 		}
 
-		$progress->setMessage('starting to decrypt files... finished');
+		$progress->setMessage('Decrypting files... finished');
 		$progress->finish();
 
 		$output->writeln("\n\n");
 	}
 
-	/**
-	 * encrypt files from the given user
-	 */
-	protected function decryptUsersFiles(string $uid, ProgressBar $progress, string $userCount): void {
+/**
+ * Decrypt all files as the given user.
+ *
+ * Recursively traverses the user's files directory, skipping files and folders not owned by the user,
+ * and attempts to decrypt each file.
+ */
+ protected function decryptUsersFiles(string $uid, ProgressBar $progress, string $userCount): void {
 		$this->setupUserFS($uid);
 		$directories = [];
 		$directories[] = '/' . $uid . '/files';
 
 		while ($root = array_pop($directories)) {
 			$content = $this->rootView->getDirectoryContent($root);
+			/** @var FileInfo $file */
 			foreach ($content as $file) {
-				// only decrypt files owned by the user
-				if ($file->getStorage()->instanceOfStorage('OCA\Files_Sharing\SharedStorage')) {
+				$path = $root . '/' . $file->getName();
+
+				if ($file->getOwner() !== $uid) {
+					$progress->setMessage("Skipping shared/unowned file/folder $path");
+					$progress->advance();
 					continue;
 				}
-				$path = $root . '/' . $file['name'];
-				if ($this->rootView->is_dir($path)) {
+
+				if ($file->getType() === FileInfo::TYPE_FOLDER) {
 					$directories[] = $path;
 					continue;
-				} else {
-					try {
-						$progress->setMessage("decrypt files for user $userCount: $path");
+				}
+
+				$progress->setMessage("Decrypting file for user $userCount: $path");
+				$progress->advance();
+
+				try {
+					if ($this->decryptFile($path) === false) {
+						$progress->setMessage("Skipping already decrypted file $path for user $userCount");
 						$progress->advance();
-						if ($file->isEncrypted() === false) {
-							$progress->setMessage("decrypt files for user $userCount: $path (already decrypted)");
-							$progress->advance();
-						} else {
-							if ($this->decryptFile($path) === false) {
-								$progress->setMessage("decrypt files for user $userCount: $path (already decrypted)");
-								$progress->advance();
-							}
-						}
-					} catch (\Exception $e) {
-						if (isset($this->failed[$uid])) {
-							$this->failed[$uid][] = $path;
-						} else {
-							$this->failed[$uid] = [$path];
-						}
+					}
+				} catch (\Exception $e) {
+					$progress->setMessage("Failed to decrypt path $path: " . $e->getMessage());					
+					$progress->advance();
+					$this->logger->error('Failed to decrypt path {path}', [ 'user' => $uid, 'path' => $path, 'exception' => $e, ]);
+					// TODO: we can probably drop this since we're now outputting above like we do in EncryptAll
+					if (isset($this->failed[$uid])) {
+						$this->failed[$uid][] = $path;
+					} else {
+						$this->failed[$uid] = [$path];
 					}
 				}
 			}
@@ -191,43 +209,56 @@ class DecryptAll {
 	}
 
 	/**
-	 * encrypt file
+	 * Attempt to decrypt a single file.
+	 * @param string $path  The full filesystem path to the file.
+	 *
+	 * @throws DecryptionFailedException If file copy or rename fails during decryption.
+	 * @throws \RuntimeException If file info cannot be retrieved or touch fails.
+	 *
+	 * @return bool True if decryption succeeded, false if file is already decrypted.
 	 */
 	protected function decryptFile(string $path): bool {
-		// skip already decrypted files
 		$fileInfo = $this->rootView->getFileInfo($path);
-		if ($fileInfo !== false && !$fileInfo->isEncrypted()) {
-			return true;
+	
+		if ($fileInfo === false) {
+    		throw new \RuntimeException("Could not retrieve file info for $path");
+		}
+		
+		if (!$fileInfo->isEncrypted()) {
+			return false;
 		}
 
 		$source = $path;
-		$target = $path . '.decrypted.' . $this->getTimestamp();
+		$target = $path . '.decrypted.' . time();
 
 		try {
-			$this->rootView->copy($source, $target);
-			$this->rootView->touch($target, $fileInfo->getMTime());
-			$this->rootView->rename($target, $source);
-		} catch (DecryptionFailedException $e) {
+			if ($this->rootView->copy($source, $target) === false) {
+				throw new DecryptionFailedException("Failed to copy $source -> $target");
+			}
+
+			if ($this->rootView->touch($target, $fileInfo->getMTime()) === false) {
+				throw new \RuntimeException("Failed to update mtime for $target");
+			}
+
+			if ($this->rootView->rename($target, $source) === false) {
+				throw new DecryptionFailedException("Failed to rename $target -> $source");
+			}
+		} catch (\Exception $e) {
 			if ($this->rootView->file_exists($target)) {
+				$this->logger->debug("Cleaning up failed temp file $target after decryption exception", [ 'path' => $path, ]);
 				$this->rootView->unlink($target);
 			}
-			return false;
+			throw $e;
 		}
 
 		return true;
 	}
 
-	/**
-	 * get current timestamp
-	 */
-	protected function getTimestamp(): int {
-		return time();
-	}
-
 	/**
 	 * setup user file system
 	 */
 	protected function setupUserFS(string $uid): void {
+		// TODO: Refactor to use injected SetupManager (like EncryptAll does) + the IUser objeect
 		\OC_Util::tearDownFS();
 		\OC_Util::setupFS($uid);
 	}

tests/lib/Encryption/DecryptAllTest.php

@@ -11,13 +11,13 @@ declare(strict_types=1);
 namespace Test\Encryption;
 
 use OC\Encryption\DecryptAll;
-use OC\Encryption\Exceptions\DecryptionFailedException;
 use OC\Encryption\Manager;
 use OC\Files\FileInfo;
 use OC\Files\View;
 use OCP\Files\Storage\IStorage;
 use OCP\IUserManager;
 use OCP\UserInterface;
+use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\MockObject\MockObject;
 use Symfony\Component\Console\Formatter\OutputFormatterInterface;
 use Symfony\Component\Console\Helper\ProgressBar;
@@ -26,54 +26,182 @@ use Symfony\Component\Console\Output\OutputInterface;
 use Test\TestCase;
 
 /**
- * Class DecryptAllTest
+ * Unit tests for DecryptAll orchestration logic in lib/private/Encryption.
  *
- *
- * @package Test\Encryption
+ * This class covers high-level user and file decryption workflows as exposed by the main encryption service,
+ * differentiating it from tests in app-layer or crypto-layer classes that cover low-level session, key, or CLI logic.
  */
-#[\PHPUnit\Framework\Attributes\Group('DB')]
 class DecryptAllTest extends TestCase {
-	private IUserManager&MockObject $userManager;
-	private Manager&MockObject $encryptionManager;
-	private View&MockObject $view;
-	private InputInterface&MockObject $inputInterface;
-	private OutputInterface&MockObject $outputInterface;
-	private UserInterface&MockObject $userInterface;
+	private const TEST_USER = 'user1';
+	private const TEST_FILE = 'test.txt';
+	
+	private IUserManager&MockObject $mockUserManager;
+	private Manager&MockObject $mockEncryptionManager;
+	private View&MockObject $mockView;
+	private InputInterface&MockObject $mockInputInterface;
+	private OutputInterface&MockObject $mockOutputInterface;
+	private UserInterface&MockObject $mockUserInterface;
 
 	private DecryptAll $instance;
 
 	protected function setUp(): void {
 		parent::setUp();
-
-		$this->userManager = $this->getMockBuilder(IUserManager::class)
-			->disableOriginalConstructor()->getMock();
-		$this->encryptionManager = $this->getMockBuilder('OC\Encryption\Manager')
-			->disableOriginalConstructor()->getMock();
-		$this->view = $this->getMockBuilder(View::class)
-			->disableOriginalConstructor()->getMock();
-		$this->inputInterface = $this->getMockBuilder(InputInterface::class)
-			->disableOriginalConstructor()->getMock();
-		$this->outputInterface = $this->getMockBuilder(OutputInterface::class)
-			->disableOriginalConstructor()->getMock();
-		$this->outputInterface->expects($this->any())->method('isDecorated')
-			->willReturn(false);
-		$this->userInterface = $this->getMockBuilder(UserInterface::class)
-			->disableOriginalConstructor()->getMock();
-
-		/* We need format method to return a string */
-		$outputFormatter = $this->createMock(OutputFormatterInterface::class);
-		$outputFormatter->method('format')->willReturn('foo');
-		$outputFormatter->method('isDecorated')->willReturn(false);
-
-		$this->outputInterface->expects($this->any())->method('getFormatter')
-			->willReturn($outputFormatter);
-
-		$this->instance = new DecryptAll($this->encryptionManager, $this->userManager, $this->view);
-
-		$this->invokePrivate($this->instance, 'input', [$this->inputInterface]);
-		$this->invokePrivate($this->instance, 'output', [$this->outputInterface]);
+		$this->initializeMocks();
+		$this->instance = new DecryptAll(
+			$this->mockEncryptionManager,
+			$this->mockUserManager,
+			$this->mockView
+		);
 	}
 
+	private function initializeMocks(): void {
+		$this->mockUserManager = $this->createMock(IUserManager::class);
+		$this->mockEncryptionManager = $this->createMock(Manager::class);
+		$this->mockView = $this->createMock(View::class);
+		$this->mockInputInterface = $this->createMock(InputInterface::class);
+		$this->mockOutputInterface = $this->createMock(OutputInterface::class);
+		$this->mockOutputInterface->method('isDecorated')->willReturn(false);
+		$this->mockUserInterface = $this->createMock(UserInterface::class);
+	}
+
+	/**
+	 * Covers: scenarios where user does/does not exist when running decryptAll.
+	 * Ensures expected messaging and correct function return.
+	 */
+	#[DataProvider('provideUserExistence')]
+	public function testDecryptAllWithNonexistentUser(bool $userExists): void {
+		$this->mockUserManager->method('userExists')->willReturn($userExists);
+
+		if (!$userExists) {
+			$this->mockOutputInterface->expects($this->once())->method('writeln')
+				->with(sprintf('User "%s" does not exist. Please check the username and try again', self::TEST_USER));
+		}
+
+		$result = $this->instance->decryptAll($this->mockInputInterface, $this->mockOutputInterface, self::TEST_USER);
+
+		$this->assertSame($userExists, $result);
+	}
+
+	public static function provideUserExistence(): array {
+		return [
+			 [true],	// userExists
+			 [false],	// !userExists
+		];
+	}
+	
+	/**
+	 * Covers: encrypted vs. non-encrypted file decryption path and side effects.
+	 * Ensures expected filesystem operations happen only when appropriate.
+	 */
+	#[DataProvider('provideDecryptionStatus')]
+	public function testDecryptFileBehavior(bool $isEncrypted): void {
+		// Configure fileInfo for encrypted/non-encrypted
+		$fileInfo = $this->createMock(FileInfo::class);
+		$fileInfo->method('isEncrypted')->willReturn($isEncrypted);
+		$this->mockView->method('getFileInfo')->willReturn($fileInfo);
+
+		// Set up mock for the timestamp function
+		$instance = $this->getMockBuilder(DecryptAll::class)
+			->setConstructorArgs([$this->mockEncryptionManager, $this->mockUserManager, $this->mockView])
+			->onlyMethods(['getTimestamp'])
+			->getMock();
+
+		if ($isEncrypted) {
+			// Only when encrypted, "copy" and "rename" should be called
+			$instance->method('getTimestamp')->willReturn(42);
+
+			$this->mockView->expects($this->once())
+				->method('copy')
+				->with(self::TEST_FILE, self::TEST_FILE . '.decrypted.42');
+			$this->mockView->expects($this->once())
+				->method('rename')
+				->with(self::TEST_FILE . '.decrypted.42', self::TEST_FILE);
+		} else {
+			// If not encrypted, these operations should not happen
+			$instance->expects($this->never())->method('getTimestamp');
+			$this->mockView->expects($this->never())->method('copy');
+			$this->mockView->expects($this->never())->method('rename');
+		}
+
+		$result = $instance->decryptFile(self::TEST_FILE);
+
+		$this->assertTrue($result);
+	}
+
+	public static function provideDecryptionStatus(): array {
+		return [
+			[true],	// isEncrypted
+			[false]	// !isEncrypted
+		];
+	}
+
+	/**
+	 * Covers: workflow for successful module preparation and user file decryption
+	 * Ensures the main orchestration calls collaborators as expected.
+	 */
+	public function testDecryptAllTriggersModuleAndUserWorkflows(): void {
+		$this->mockUserManager->method('userExists')->willReturn(true);
+
+		// Partial mock to verify internal orchestration logic
+		$instance = $this->getMockBuilder(DecryptAll::class)
+			->setConstructorArgs([
+				$this->mockEncryptionManager,
+				$this->mockUserManager,
+				$this->mockView
+			])
+			->onlyMethods(['prepareEncryptionModules', 'decryptAllUsersFiles'])
+			->getMock();
+
+		// If module preparation succeeds, proceed with user file decryption
+		$instance->expects($this->once())
+			->method('prepareEncryptionModules')
+			->with($this->mockInputInterface, $this->mockOutputInterface, self::TEST_USER)
+			->willReturn(true);
+
+		$instance->expects($this->once())
+			->method('decryptAllUsersFiles')
+			->with($this->mockOutputInterface, self::TEST_USER);
+
+		$result = $instance->decryptAll($this->mockInputInterface, $this->mockOutputInterface, self::TEST_USER);
+		$this->assertTrue($result);
+	}
+
+	/**
+	 * Covers: early abort if module preparation fails.
+	 * Ensures "decryptAllUsersFiles" is not called and function returns false.
+	 */
+	public function testDecryptAllAbortsIfPreparationFails(): void {
+		$this->mockUserManager->method('userExists')->willReturn(true);
+
+		$instance = $this->getMockBuilder(DecryptAll::class)
+			->setConstructorArgs([
+				$this->mockEncryptionManager,
+				$this->mockUserManager,
+				$this->mockView
+			])
+			->onlyMethods(['prepareEncryptionModules', 'decryptAllUsersFiles'])
+			->getMock();
+
+		$instance->expects($this->once())
+			->method('prepareEncryptionModules')
+			->with($this->mockInputInterface, $this->mockOutputInterface, self::TEST_USER)
+			->willReturn(false);
+
+		$instance->expects($this->never())
+			->method('decryptAllUsersFiles');
+
+		$result = $instance->decryptAll($this->mockInputInterface, $this->mockOutputInterface, self::TEST_USER);
+		$this->assertFalse($result);
+	}
+
+	/**
+	 * TODO: 
+	 * - Add file operation failure (fileInfo/copy/rename)
+	 * - Add teamfolder scenario
+	 */
+	
+	/*********** LEGACY TESTS ************/
+	
 	public static function dataDecryptAll(): array {
 		return [
 			[true, 'user1', true],