Skip to content

Commit d00e1b2

Browse files
DeepDiver1975claude
DeepDiver1975
and
claude
authored
fix(encryption): decrypt versions and trashbin so encryption can be disabled (#41624)
* fix(encryption): 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 file cache entries under "files_versions" and "files_trashbin". Since occ encryption:disable refuses while any file cache row is still flagged as encrypted, administrators could not disable encryption even though decrypt-all reported success. decrypt-all now also descends into "files_versions" and "files_trashbin" when they exist, and the disable command now lists the paths that are still flagged as encrypted along with a remediation hint instead of printing a generic message. Fixes #41623 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> * fix(encryption): cap reported encrypted paths at 20 with a summary count Address review feedback: instead of an arbitrary 50-path cap, print at most 20 still-encrypted paths so the output fits on screen, followed by an exact "... and N more still encrypted" summary. The count is determined with a dedicated COUNT(*) query so the full file cache is never loaded into memory. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --------- Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent bada5a2 commit d00e1b2

5 files changed

Lines changed: 148 additions & 20 deletions

File tree

‎changelog/unreleased/41623‎

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

‎core/Command/Encryption/Disable.php‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,13 @@
2828
use Symfony\Component\Console\Output\OutputInterface;
2929

3030
class Disable extends Command {
31+
/**
32+
* Maximum number of still-encrypted paths to print so the output fits
33+
* on screen on systems with many leftover entries. The remaining count
34+
* is reported as a summary.
35+
*/
36+
private const MAX_REPORTED_PATHS = 20;
37+
3138
/** @var IDBConnection */
3239
protected $db;
3340
/** @var IConfig */
@@ -52,15 +59,35 @@ protected function configure() {
5259

5360
protected function execute(InputInterface $input, OutputInterface $output): int {
5461
$qb = $this->db->getQueryBuilder();
55-
$qb->select($qb->expr()->literal('1'))
62+
$qb->select($qb->createFunction('COUNT(*)'))
5663
->from('filecache', 'fc')
57-
->where($qb->expr()->gte('fc.encrypted', $qb->expr()->literal('1')))
58-
->setMaxResults(1);
64+
->where($qb->expr()->gte('fc.encrypted', $qb->expr()->literal('1')));
5965
$results = $qb->execute();
60-
$hasEncryptedFiles = (bool) $results->fetchOne();
66+
$encryptedCount = (int) $results->fetchOne();
6167
$results->free();
62-
if ($hasEncryptedFiles !== false) {
63-
$output->writeln('<info>The system still has encrypted files. Please decrypt them all before disabling encryption.</info>');
68+
if ($encryptedCount > 0) {
69+
$qb = $this->db->getQueryBuilder();
70+
$qb->select('fc.path')
71+
->from('filecache', 'fc')
72+
->where($qb->expr()->gte('fc.encrypted', $qb->expr()->literal('1')))
73+
->setMaxResults(self::MAX_REPORTED_PATHS);
74+
$results = $qb->execute();
75+
$encryptedPaths = $results->fetchFirstColumn();
76+
$results->free();
77+
78+
$output->writeln('<error>The system still has encrypted files. Please decrypt them all before disabling encryption.</error>');
79+
$output->writeln('The following paths in the file cache are still flagged as encrypted:');
80+
foreach ($encryptedPaths as $path) {
81+
$output->writeln(" $path");
82+
}
83+
$remaining = $encryptedCount - \count($encryptedPaths);
84+
if ($remaining > 0) {
85+
$output->writeln(" ... and $remaining more still encrypted");
86+
}
87+
$output->writeln('');
88+
$output->writeln('Run "occ encryption:decrypt-all" to decrypt these. Entries on shared or');
89+
$output->writeln('external storage are skipped by decrypt-all and have to be decrypted by');
90+
$output->writeln('their owner, e.g. via "occ encryption:decrypt-all <user>".');
6491
return 1;
6592
}
6693

‎lib/private/Encryption/DecryptAll.php‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,15 @@ protected function decryptAllUsersFiles($user = '') {
214214
protected function decryptUsersFiles($uid, ProgressBar $progress, $userCount) {
215215
$this->setupUserFS($uid);
216216
$directories = [];
217-
$directories[] = '/' . $uid . '/files';
217+
// also decrypt files stored outside of the regular "files" folder,
218+
// otherwise their filecache "encrypted" flag survives and blocks
219+
// "occ encryption:disable" afterwards
220+
foreach (['files', 'files_versions', 'files_trashbin'] as $folder) {
221+
$root = '/' . $uid . '/' . $folder;
222+
if ($this->rootView->is_dir($root)) {
223+
$directories[] = $root;
224+
}
225+
}
218226

219227
while ($root = \array_pop($directories)) {
220228
$content = $this->rootView->getDirectoryContent($root);

‎tests/Core/Command/Encryption/DisableTest.php‎

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,15 @@ protected function setUp(): void {
5959

6060
public function dataDisable() {
6161
return [
62-
['yes', true, '1', false, 'Encryption disabled'],
63-
['yes', true, '', false, 'Encryption disabled'],
64-
['no', false, false, false, 'Encryption is already disabled'],
65-
['yes', true, '1', true, 'Encryption disabled'],
66-
['yes', true, '', true, 'Encryption disabled'],
67-
['no', false, false, true, 'Encryption is already disabled'],
62+
// [oldStatus, isUpdating, masterKeyEnabled, encryptedCount, reportedPaths, expectedString]
63+
['yes', true, '1', 0, [], 'Encryption disabled'],
64+
['yes', true, '', 0, [], 'Encryption disabled'],
65+
['no', false, false, 0, [], 'Encryption is already disabled'],
66+
['yes', true, '1', 1, ['files_versions/foo.txt'], 'Encryption disabled'],
67+
['yes', true, '', 1, ['files_trashbin/files/bar.txt.d123'], 'Encryption disabled'],
68+
['no', false, false, 1, ['files/baz.txt'], 'Encryption is already disabled'],
69+
// more rows encrypted than are reported -> "... and N more" summary
70+
['no', false, false, 25, ['files/a.txt', 'files_versions/b.txt'], 'Encryption is already disabled'],
6871
];
6972
}
7073

@@ -74,29 +77,37 @@ public function dataDisable() {
7477
* @param string $oldStatus
7578
* @param bool $isUpdating
7679
* @param bool $masterKeyEnabled
77-
* @param int $hasEncryptedFiles
80+
* @param int $encryptedCount number of file cache rows still flagged as encrypted
81+
* @param string[] $reportedPaths capped list of paths the command prints
7882
* @param string $expectedString
7983
*/
80-
public function testDisable($oldStatus, $isUpdating, $masterKeyEnabled, $hasEncryptedFiles, $expectedString) {
84+
public function testDisable($oldStatus, $isUpdating, $masterKeyEnabled, $encryptedCount, $reportedPaths, $expectedString) {
8185
$stmt = $this->createMock(Result::class);
86+
// first query counts the encrypted rows, second fetches the capped path list
8287
$stmt->method('fetchOne')
83-
->willReturn($hasEncryptedFiles);
88+
->willReturn($encryptedCount);
89+
$stmt->method('fetchFirstColumn')
90+
->willReturn($reportedPaths);
8491
$qbExpr = $this->createMock(ExpressionBuilder::class);
8592
$qbMock = $this->createMock(QueryBuilder::class);
8693
$qbMock->method('expr')
8794
->willReturn($qbExpr);
95+
$qbMock->method('createFunction')
96+
->willReturnArgument(0);
8897
$qbMock->method('select')
8998
->willReturnSelf();
9099
$qbMock->method('from')
91100
->willReturnSelf();
92101
$qbMock->method('where')
93102
->willReturnSelf();
103+
$qbMock->method('setMaxResults')
104+
->willReturnSelf();
94105
$qbMock->method('execute')
95106
->willReturn($stmt);
96107
$this->db->method('getQueryBuilder')
97108
->willReturn($qbMock);
98109

99-
if ($hasEncryptedFiles === false) {
110+
if ($encryptedCount === 0) {
100111
$this->config->expects($this->exactly(1))
101112
->method('getAppValue')
102113
->willReturnMap(
@@ -129,13 +140,30 @@ public function testDisable($oldStatus, $isUpdating, $masterKeyEnabled, $hasEncr
129140
}
130141
$expectedExitCode = 0;
131142
} else {
132-
$this->consoleOutput->expects($this->once())
143+
$writtenLines = [];
144+
$this->consoleOutput->expects($this->atLeastOnce())
133145
->method('writeln')
134-
->with($this->stringContains('<info>The system still has encrypted files. Please decrypt them all before disabling encryption.</info>'));
146+
->willReturnCallback(function ($line) use (&$writtenLines) {
147+
$writtenLines[] = $line;
148+
});
135149
$expectedExitCode = 1;
136150
}
137151

138152
$exitCode = self::invokePrivate($this->command, 'execute', [$this->consoleInput, $this->consoleOutput]);
139153
$this->assertEquals($expectedExitCode, $exitCode);
154+
155+
if ($encryptedCount > 0) {
156+
$output = \implode("\n", $writtenLines);
157+
$this->assertStringContainsString('The system still has encrypted files.', $output);
158+
foreach ($reportedPaths as $path) {
159+
$this->assertStringContainsString($path, $output);
160+
}
161+
$remaining = $encryptedCount - \count($reportedPaths);
162+
if ($remaining > 0) {
163+
$this->assertStringContainsString("... and $remaining more still encrypted", $output);
164+
} else {
165+
$this->assertStringNotContainsString('more still encrypted', $output);
166+
}
167+
}
140168
}
141169
}

‎tests/lib/Encryption/DecryptAllTest.php‎

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -522,7 +522,9 @@ public function testDecryptUsersFiles($decryptBehavior, $storageClass) {
522522
$this->view->expects($this->any())->method('is_dir')
523523
->willReturnCallback(
524524
function ($path) {
525-
if ($path === '/user1/files/foo') {
525+
// the "files" folder is seeded into the walk, "files_versions"
526+
// and "files_trashbin" do not exist in this scenario
527+
if ($path === '/user1/files' || $path === '/user1/files/foo') {
526528
return true;
527529
}
528530
return false;
@@ -554,6 +556,55 @@ function ($path) {
554556
self::invokePrivate($instance, 'decryptUsersFiles', ['user1', $progressBar, '']);
555557
}
556558

559+
/**
560+
* files_versions and files_trashbin must be decrypted as well, otherwise
561+
* their leftover "encrypted" filecache flags block encryption:disable.
562+
*/
563+
public function testDecryptUsersFilesAlsoCoversVersionsAndTrashbin() {
564+
/** @var DecryptAll | \PHPUnit\Framework\MockObject\MockObject $instance */
565+
$instance = $this->getMockBuilder(DecryptAll::class)
566+
->setConstructorArgs(
567+
[
568+
$this->encryptionManager,
569+
$this->userManager,
570+
$this->view,
571+
$this->logger
572+
]
573+
)
574+
->setMethods(['decryptFile'])
575+
->getMock();
576+
577+
// all three top-level folders exist for this user
578+
$this->view->expects($this->any())->method('is_dir')
579+
->willReturnCallback(function ($path) {
580+
return \in_array($path, [
581+
'/user1/files',
582+
'/user1/files_versions',
583+
'/user1/files_trashbin',
584+
], true);
585+
});
586+
587+
$storage = $this->createMock(\OC\Files\Storage\Local::class);
588+
$storage->expects($this->any())->method('instanceOfStorage')->willReturn(false);
589+
590+
$visited = [];
591+
$this->view->expects($this->exactly(3))
592+
->method('getDirectoryContent')
593+
->willReturnCallback(function ($root) use (&$visited) {
594+
$visited[] = $root;
595+
return [];
596+
});
597+
598+
$progressBar = new ProgressBar(new NullOutput());
599+
self::invokePrivate($instance, 'decryptUsersFiles', ['user1', $progressBar, '']);
600+
601+
\sort($visited);
602+
$this->assertEquals(
603+
['/user1/files', '/user1/files_trashbin', '/user1/files_versions'],
604+
$visited
605+
);
606+
}
607+
557608
public function testDecryptFile() {
558609
$path = 'test.txt';
559610

0 commit comments

Comments
 (0)