fix(encryption): decrypt versions and trashbin so encryption can be disabled

changelog/unreleased/41623

@@ -0,0 +1,14 @@
+Bugfix: Decrypt versions and trashbin so encryption can be disabled
+
+"occ encryption:decrypt-all" only walked the regular "files" folder, leaving the
+"encrypted" flag set on entries in "files_versions" and "files_trashbin". Because
+"occ encryption:disable" refuses while any file cache row is still flagged as
+encrypted, administrators were left unable to disable encryption even though
+decrypt-all reported success.
+
+decrypt-all now also descends into "files_versions" and "files_trashbin", and the
+disable command now lists the paths that are still flagged as encrypted together
+with a hint on how to clean them up, instead of printing a generic message.
+
+https://github.com/owncloud/core/issues/41623
+https://github.com/owncloud/core/pull/41624

core/Command/Encryption/Disable.php

@@ -28,6 +28,13 @@
 use Symfony\Component\Console\Output\OutputInterface;
 
 class Disable extends Command {
+	/**
+	 * Maximum number of still-encrypted paths to print so the output fits
+	 * on screen on systems with many leftover entries. The remaining count
+	 * is reported as a summary.
+	 */
+	private const MAX_REPORTED_PATHS = 20;
+
 	/** @var IDBConnection */
 	protected $db;
 	/** @var IConfig */
@@ -52,15 +59,35 @@ protected function configure() {
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
 		$qb = $this->db->getQueryBuilder();
-		$qb->select($qb->expr()->literal('1'))
+		$qb->select($qb->createFunction('COUNT(*)'))
 			->from('filecache', 'fc')
-			->where($qb->expr()->gte('fc.encrypted', $qb->expr()->literal('1')))
-			->setMaxResults(1);
+			->where($qb->expr()->gte('fc.encrypted', $qb->expr()->literal('1')));
 		$results = $qb->execute();
-		$hasEncryptedFiles = (bool) $results->fetchOne();
+		$encryptedCount = (int) $results->fetchOne();
 		$results->free();
-		if ($hasEncryptedFiles !== false) {
-			$output->writeln('<info>The system still has encrypted files. Please decrypt them all before disabling encryption.</info>');
+		if ($encryptedCount > 0) {
+			$qb = $this->db->getQueryBuilder();
+			$qb->select('fc.path')
+				->from('filecache', 'fc')
+				->where($qb->expr()->gte('fc.encrypted', $qb->expr()->literal('1')))
+				->setMaxResults(self::MAX_REPORTED_PATHS);
+			$results = $qb->execute();
+			$encryptedPaths = $results->fetchFirstColumn();
+			$results->free();
+
+			$output->writeln('<error>The system still has encrypted files. Please decrypt them all before disabling encryption.</error>');
+			$output->writeln('The following paths in the file cache are still flagged as encrypted:');
+			foreach ($encryptedPaths as $path) {
+				$output->writeln("    $path");
+			}
+			$remaining = $encryptedCount - \count($encryptedPaths);
+			if ($remaining > 0) {
+				$output->writeln("    ... and $remaining more still encrypted");
+			}
+			$output->writeln('');
+			$output->writeln('Run "occ encryption:decrypt-all" to decrypt these. Entries on shared or');
+			$output->writeln('external storage are skipped by decrypt-all and have to be decrypted by');
+			$output->writeln('their owner, e.g. via "occ encryption:decrypt-all <user>".');
 			return 1;
 		}
 

lib/private/Encryption/DecryptAll.php

@@ -214,7 +214,15 @@ protected function decryptAllUsersFiles($user = '') {
 	protected function decryptUsersFiles($uid, ProgressBar $progress, $userCount) {
 		$this->setupUserFS($uid);
 		$directories = [];
-		$directories[] = '/' . $uid . '/files';
+		// also decrypt files stored outside of the regular "files" folder,
+		// otherwise their filecache "encrypted" flag survives and blocks
+		// "occ encryption:disable" afterwards
+		foreach (['files', 'files_versions', 'files_trashbin'] as $folder) {
+			$root = '/' . $uid . '/' . $folder;
+			if ($this->rootView->is_dir($root)) {
+				$directories[] = $root;
+			}
+		}
 
 		while ($root = \array_pop($directories)) {
 			$content = $this->rootView->getDirectoryContent($root);

tests/Core/Command/Encryption/DisableTest.php

@@ -59,12 +59,15 @@ protected function setUp(): void {
 
 	public function dataDisable() {
 		return [
-			['yes', true, '1', false, 'Encryption disabled'],
-			['yes', true, '', false, 'Encryption disabled'],
-			['no', false, false, false, 'Encryption is already disabled'],
-			['yes', true, '1', true, 'Encryption disabled'],
-			['yes', true, '', true, 'Encryption disabled'],
-			['no', false, false, true, 'Encryption is already disabled'],
+			// [oldStatus, isUpdating, masterKeyEnabled, encryptedCount, reportedPaths, expectedString]
+			['yes', true, '1', 0, [], 'Encryption disabled'],
+			['yes', true, '', 0, [], 'Encryption disabled'],
+			['no', false, false, 0, [], 'Encryption is already disabled'],
+			['yes', true, '1', 1, ['files_versions/foo.txt'], 'Encryption disabled'],
+			['yes', true, '', 1, ['files_trashbin/files/bar.txt.d123'], 'Encryption disabled'],
+			['no', false, false, 1, ['files/baz.txt'], 'Encryption is already disabled'],
+			// more rows encrypted than are reported -> "... and N more" summary
+			['no', false, false, 25, ['files/a.txt', 'files_versions/b.txt'], 'Encryption is already disabled'],
 		];
 	}
 
@@ -74,29 +77,37 @@ public function dataDisable() {
 	 * @param string $oldStatus
 	 * @param bool $isUpdating
 	 * @param bool $masterKeyEnabled
-	 * @param int $hasEncryptedFiles
+	 * @param int $encryptedCount number of file cache rows still flagged as encrypted
+	 * @param string[] $reportedPaths capped list of paths the command prints
 	 * @param string $expectedString
 	 */
-	public function testDisable($oldStatus, $isUpdating, $masterKeyEnabled, $hasEncryptedFiles, $expectedString) {
+	public function testDisable($oldStatus, $isUpdating, $masterKeyEnabled, $encryptedCount, $reportedPaths, $expectedString) {
 		$stmt = $this->createMock(Result::class);
+		// first query counts the encrypted rows, second fetches the capped path list
 		$stmt->method('fetchOne')
-			->willReturn($hasEncryptedFiles);
+			->willReturn($encryptedCount);
+		$stmt->method('fetchFirstColumn')
+			->willReturn($reportedPaths);
 		$qbExpr = $this->createMock(ExpressionBuilder::class);
 		$qbMock = $this->createMock(QueryBuilder::class);
 		$qbMock->method('expr')
 			->willReturn($qbExpr);
+		$qbMock->method('createFunction')
+			->willReturnArgument(0);
 		$qbMock->method('select')
 			->willReturnSelf();
 		$qbMock->method('from')
 			->willReturnSelf();
 		$qbMock->method('where')
 			->willReturnSelf();
+		$qbMock->method('setMaxResults')
+			->willReturnSelf();
 		$qbMock->method('execute')
 			->willReturn($stmt);
 		$this->db->method('getQueryBuilder')
 			->willReturn($qbMock);
 
-		if ($hasEncryptedFiles === false) {
+		if ($encryptedCount === 0) {
 			$this->config->expects($this->exactly(1))
 				->method('getAppValue')
 				->willReturnMap(
@@ -129,13 +140,30 @@ public function testDisable($oldStatus, $isUpdating, $masterKeyEnabled, $hasEncr
 			}
 			$expectedExitCode = 0;
 		} else {
-			$this->consoleOutput->expects($this->once())
+			$writtenLines = [];
+			$this->consoleOutput->expects($this->atLeastOnce())
 				->method('writeln')
-				->with($this->stringContains('<info>The system still has encrypted files. Please decrypt them all before disabling encryption.</info>'));
+				->willReturnCallback(function ($line) use (&$writtenLines) {
+					$writtenLines[] = $line;
+				});
 			$expectedExitCode = 1;
 		}
 
 		$exitCode = self::invokePrivate($this->command, 'execute', [$this->consoleInput, $this->consoleOutput]);
 		$this->assertEquals($expectedExitCode, $exitCode);
+
+		if ($encryptedCount > 0) {
+			$output = \implode("\n", $writtenLines);
+			$this->assertStringContainsString('The system still has encrypted files.', $output);
+			foreach ($reportedPaths as $path) {
+				$this->assertStringContainsString($path, $output);
+			}
+			$remaining = $encryptedCount - \count($reportedPaths);
+			if ($remaining > 0) {
+				$this->assertStringContainsString("... and $remaining more still encrypted", $output);
+			} else {
+				$this->assertStringNotContainsString('more still encrypted', $output);
+			}
+		}
 	}
 }

tests/lib/Encryption/DecryptAllTest.php

@@ -522,7 +522,9 @@ public function testDecryptUsersFiles($decryptBehavior, $storageClass) {
 		$this->view->expects($this->any())->method('is_dir')
 			->willReturnCallback(
 				function ($path) {
-					if ($path === '/user1/files/foo') {
+					// the "files" folder is seeded into the walk, "files_versions"
+					// and "files_trashbin" do not exist in this scenario
+					if ($path === '/user1/files' || $path === '/user1/files/foo') {
 						return true;
 					}
 					return false;
@@ -554,6 +556,55 @@ function ($path) {
 		self::invokePrivate($instance, 'decryptUsersFiles', ['user1', $progressBar, '']);
 	}
 
+	/**
+	 * files_versions and files_trashbin must be decrypted as well, otherwise
+	 * their leftover "encrypted" filecache flags block encryption:disable.
+	 */
+	public function testDecryptUsersFilesAlsoCoversVersionsAndTrashbin() {
+		/** @var DecryptAll | \PHPUnit\Framework\MockObject\MockObject  $instance */
+		$instance = $this->getMockBuilder(DecryptAll::class)
+			->setConstructorArgs(
+				[
+					$this->encryptionManager,
+					$this->userManager,
+					$this->view,
+					$this->logger
+				]
+			)
+			->setMethods(['decryptFile'])
+			->getMock();
+
+		// all three top-level folders exist for this user
+		$this->view->expects($this->any())->method('is_dir')
+			->willReturnCallback(function ($path) {
+				return \in_array($path, [
+					'/user1/files',
+					'/user1/files_versions',
+					'/user1/files_trashbin',
+				], true);
+			});
+
+		$storage = $this->createMock(\OC\Files\Storage\Local::class);
+		$storage->expects($this->any())->method('instanceOfStorage')->willReturn(false);
+
+		$visited = [];
+		$this->view->expects($this->exactly(3))
+			->method('getDirectoryContent')
+			->willReturnCallback(function ($root) use (&$visited) {
+				$visited[] = $root;
+				return [];
+			});
+
+		$progressBar = new ProgressBar(new NullOutput());
+		self::invokePrivate($instance, 'decryptUsersFiles', ['user1', $progressBar, '']);
+
+		\sort($visited);
+		$this->assertEquals(
+			['/user1/files', '/user1/files_trashbin', '/user1/files_versions'],
+			$visited
+		);
+	}
+
 	public function testDecryptFile() {
 		$path = 'test.txt';