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

[DeepDiver1975]

Summary

Fixes #41623.

occ encryption:decrypt-all reports success but occ encryption:disable still fails with "The system still has encrypted files. Please decrypt them all before disabling encryption." — leaving admins unable to turn encryption off (also see the central thread linked in the issue, and the closely related #39212).

Root cause is a scope mismatch between the two commands:

• encryption:disable (core/Command/Encryption/Disable.php) refuses if any filecache row has encrypted >= 1.
• decrypt-all (lib/private/Encryption/DecryptAll::decryptUsersFiles()) only ever walks '/' . $uid . '/files', and decryptFile() is the only place that resets the flag.

So encrypted entries under files_versions and files_trashbin keep their flag and permanently block disabling, even though decrypt-all printed "all files could be decrypted successfully!".

Changes

• DecryptAll: decryptUsersFiles() now also descends into files_versions and files_trashbin (when they exist), so their entries are actually decrypted and their encrypted flag cleared.
• Disable: instead of a generic message, the command now lists the paths still flagged as encrypted (capped at 50) plus a remediation hint, so admins can see what is blocking them — e.g. entries on shared/external storage, which decrypt-all skips by design and must be decrypted by their owner.

Tests

• Updated DisableTest for the new fetchFirstColumn() query and multi-line output; data provider now drives blocking-path scenarios and asserts the offending paths are printed.
• Updated DecryptAllTest::testDecryptUsersFiles for the new seeding contract and added testDecryptUsersFilesAlsoCoversVersionsAndTrashbin proving all three roots are walked when present.
• DisableTest (6 tests) and DecryptAllTest (24 tests) pass locally on sqlite; php-cs-fixer clean.

Note for QA

Unit tests prove the new roots are visited; end-to-end decryption of versions/trashbin against a live encrypted instance (master-key and per-user-key modules) should be verified manually before release.

🤖 Generated with Claude Code

[jvillafanez]

it makes sense, just nitpicking.

[DeepDiver1975]

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

🤖 Automated review by Claude Code review agent.

Overview

Fixes the scope mismatch where encryption:decrypt-all only walked /<uid>/files while encryption:disable refuses if any filecache row has encrypted >= 1. The fix makes decrypt-all also descend into files_versions and files_trashbin, and improves the disable command's diagnostics by listing the still-encrypted paths. The root-cause analysis in the description is accurate and matches the code.

Strengths

• Correct root cause & minimal fix. Adding the two extra roots in decryptUsersFiles() is the right place — the existing BFS loop, share-skipping, and decryptFile() flag-clearing all apply unchanged to the new roots.
• Defensive is_dir() guard avoids seeding non-existent roots into the walk (versions/trashbin are absent on many setups).
• Much better operator UX. Listing blocking paths plus the remediation hint about shared/external storage (which decrypt-all skips by design) directly addresses the "decrypt-all said success but disable still fails" confusion.
• Test coverage is solid — testDecryptUsersFilesAlsoCoversVersionsAndTrashbin asserts exactly the three roots are visited, and DisableTest now exercises the capped-path + "... and N more" summary path.

Issues / Suggestions

1. Two queries where one would do (minor, performance). Disable::execute() now runs a COUNT(*) and then a second SELECT ... LIMIT 20. You could drop the count query and derive the "... and N more" suffix only when the path query hits the cap — though knowing the exact remaining count requires the count anyway. As-is it's fine; just noting the extra round-trip runs on the common (zero-encrypted) success path too. Consider short-circuiting: if the path-list query returns 0 rows, skip straight to disabling. Currently the success path pays for a COUNT(*) scan that could be a cheap LIMIT 1 existence check when you don't need the number. Trade-off is acceptable given disable is a rare admin command.

2. createFunction('COUNT(*)') portability. Using createFunction for the count bypasses the query builder's aggregate handling. It works across OC's supported DBs here, but $qb->func()->count('*') (or selectAlias) is the more idiomatic and safer route. Low priority.

3. getDirectoryContent on files_trashbin root. Trashbin layout is files_trashbin/files/... plus versions/ and keys/ siblings. The BFS will recurse into all of them and attempt decryptFile() on non-file artifacts. decryptFile() returning false is handled gracefully (counted as "already decrypted"), so this is safe, but worth a manual check that decrypting trashbin versions/ entries actually clears their encrypted flag — otherwise the disable block could persist for those rows. The PR's own "Note for QA" already flags this; +1, please verify against a live per-user-key instance.

4. Test seeding contract change. testDecryptUsersFiles now also returns true for /user1/files (previously only /user1/files/foo). Correct given the new is_dir guard, but confirm no other test relied on the old narrower stub.

Risk Assessment

Low-to-moderate. Logic change is contained and well-tested at the unit level. The main residual risk is the real-world behavior of decryption on trashbin/versions storage backends (master-key vs per-user-key), which unit tests can't cover — the manual QA note correctly calls this out.

Verdict

👍 Approve with minor suggestions. The fix is correct, minimal, and well-tested. Recommend addressing the live-instance QA verification before release per the author's own note.