Feature: Add cleartext path to decryption failed event #278
Conversation
# Conflicts: # src/main/java/org/cryptomator/cryptofs/CryptoFileSystemModule.java # src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProperties.java # src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java # src/main/java/org/cryptomator/cryptofs/event/ConflictResolutionFailedEvent.java # src/main/java/org/cryptomator/cryptofs/event/ConflictResolvedEvent.java # src/main/java/org/cryptomator/cryptofs/event/DecryptionFailedEvent.java # src/main/java/org/cryptomator/cryptofs/event/FilesystemEvent.java # src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java # src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java # src/test/java/org/cryptomator/cryptofs/CryptoFileSystemPropertiesTest.java # src/test/java/org/cryptomator/cryptofs/dir/C9rConflictResolverTest.java # src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java # src/test/java/org/cryptomator/cryptofs/fh/FileHeaderHolderTest.java
WalkthroughThis pull request revises the method signatures and internal data handling for file operations across multiple classes in the cryptographic filesystem codebase. The changes introduce an additional parameter to methods such as Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFileModule.java (1)
37-38: Consider enhancing error handling for inaccessible paths.While the changes correctly adapt to using
ClearAndCipherPath, the error handling inreadBasicAttributessilently returnsEPOCHwhen the ciphertext path is inaccessible. Consider logging the error or propagating it to provide better visibility during decryption failures.@OpenFileModifiedDate public AtomicReference<Instant> provideLastModifiedDate(@OriginalOpenFilePaths ClearAndCipherPath originalPaths) { - Instant lastModifiedDate = readBasicAttributes(originalPaths.ciphertextPath()).map(BasicFileAttributes::lastModifiedTime).map(FileTime::toInstant).orElse(Instant.EPOCH); + Instant lastModifiedDate = readBasicAttributes(originalPaths.ciphertextPath()) + .map(BasicFileAttributes::lastModifiedTime) + .map(FileTime::toInstant) + .orElseGet(() -> { + logger.warn("Failed to read attributes for {}", originalPaths.cleartextPath()); + return Instant.EPOCH; + }); return new AtomicReference<>(lastModifiedDate); }src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java (1)
257-260: Consider using a more descriptive variable name.While the logic is correct, the variable name 'ps' could be more descriptive. Consider using 'paths' or 'filePaths' to improve code readability.
-var ps = currentFilePaths.get(); -if (ps != null) { - var ciphertextPath = ps.ciphertextPath(); +var paths = currentFilePaths.get(); +if (paths != null) { + var ciphertextPath = paths.ciphertextPath();src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java (1)
56-56: Consider enhancing logging with both paths for better debugging.While the current logging is correct, consider including both paths in debug logs for better troubleshooting capabilities.
- LOG.trace("Generating file header for {}", paths.get().ciphertextPath()); + var ps = paths.get(); + LOG.trace("Generating file header for {} (cleartext: {})", ps.ciphertextPath(), ps.cleartextPath()); - LOG.trace("Reading file header from {}", paths.get().cleartextPath()); + var ps = paths.get(); + LOG.trace("Reading file header from {} (ciphertext: {})", ps.cleartextPath(), ps.ciphertextPath());Also applies to: 72-72
src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java (1)
91-98: Consider updating delete method to handle cleartext paths.For consistency with other methods, the delete method should also track cleartext paths. This would maintain the pattern established in other methods and ensure complete path tracking.
-public void delete(Path ciphertextPath) { +public void delete(CryptoPath cleartextPath, Path ciphertextPath) { openCryptoFiles.compute(ciphertextPath, (p, openFile) -> { if (openFile != null) { - openFile.updateCurrentFilePath(null); + openFile.updateCurrentFilePath(new ClearAndCipherPath(null, null)); } return null; }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java(3 hunks)src/main/java/org/cryptomator/cryptofs/Symlinks.java(3 hunks)src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java(3 hunks)src/main/java/org/cryptomator/cryptofs/event/DecryptionFailedEvent.java(1 hunks)src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java(2 hunks)src/main/java/org/cryptomator/cryptofs/fh/ClearAndCipherPath.java(1 hunks)src/main/java/org/cryptomator/cryptofs/fh/CurrentOpenFilePaths.java(1 hunks)src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java(4 hunks)src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java(4 hunks)src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFileComponent.java(1 hunks)src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFileModule.java(1 hunks)src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java(5 hunks)src/main/java/org/cryptomator/cryptofs/fh/OriginalOpenFilePaths.java(1 hunks)src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java(3 hunks)src/test/java/org/cryptomator/cryptofs/SymlinksTest.java(6 hunks)src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java(5 hunks)src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java(2 hunks)src/test/java/org/cryptomator/cryptofs/fh/FileHeaderHolderTest.java(2 hunks)src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java(10 hunks)src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFilesTest.java(4 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/main/java/org/cryptomator/cryptofs/fh/ClearAndCipherPath.java
- src/main/java/org/cryptomator/cryptofs/fh/OriginalOpenFilePaths.java
- src/main/java/org/cryptomator/cryptofs/fh/CurrentOpenFilePaths.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyse
- GitHub Check: Build and Test
🔇 Additional comments (31)
src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java (3)
4-4: LGTM!The added imports are necessary for the new field declarations and align with the PR objectives.
Also applies to: 9-9
76-78: LGTM!The field declarations are well-structured:
- The mock paths are created with descriptive values.
- The use of
AtomicReferenceensures thread-safe updates.- The changes align with the PR objectives of incorporating cleartext path.
108-108: LGTM!The constructor usage changes are consistent across all test methods and align with the PR objectives.
Also applies to: 403-403, 575-575
src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFileModule.java (1)
29-31: LGTM! Changes align with PR objectives.The method signature and annotation changes correctly support the new
ClearAndCipherPathtype, enabling access to both cleartext and ciphertext paths. This aligns with the PR's goal of enhancing decryption failed events with cleartext path information.src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java (3)
10-11: LGTM! Import changes align with the new path handling strategy.The imports correctly reflect the transition from single path to dual path handling, supporting the PR's objective of incorporating cleartext paths.
53-53: LGTM! Field type and name changes are appropriate.The transition to
AtomicReference<ClearAndCipherPath>and the plural naming convention accurately represent the dual path nature of the field.
61-61: LGTM! Constructor changes maintain thread safety and consistency.The parameter and field assignment changes correctly handle the new path structure while preserving thread safety through
AtomicReference.Also applies to: 69-69
src/main/java/org/cryptomator/cryptofs/event/DecryptionFailedEvent.java (1)
10-14: LGTM! Well-structured record definition with clear documentation.The addition of
cleartextPathenhances the event's context, and the parameter ordering (cleartextPath first) makes sense from a user perspective.src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFileComponent.java (1)
20-21: LGTM! Clean transition to the new path handling structure.The factory method signature has been updated correctly to use
ClearAndCipherPath, maintaining proper dependency injection patterns.src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java (2)
19-19: LGTM! Thread-safe implementation with enhanced path handling.The transition to
ClearAndCipherPathis implemented correctly, maintaining thread safety throughAtomicReference.Also applies to: 27-29
54-55: LGTM! Improved error handling with comprehensive path information.The error handling now provides better context by including both cleartext and ciphertext paths in the event.
src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java (2)
27-27: LGTM! Clean implementation of the new path handling structure.The transition to
ClearAndCipherPathis implemented correctly, maintaining thread safety throughAtomicReference.Also applies to: 33-36
83-88: LGTM! Comprehensive error handling with improved context.The error handling provides better context by including both paths in the event and error message.
src/main/java/org/cryptomator/cryptofs/Symlinks.java (3)
33-38: LGTM! Constructor simplified by removing unused dependency.The constructor has been simplified by removing the
LongFileNameProviderdependency, making the class more focused.
48-49: LGTM! Enhanced error tracking by including cleartext path.The
writeCiphertextFilecall now includes the cleartext path, which will help in error reporting and debugging.
57-58: LGTM! Improved error context by including cleartext path.The
readCiphertextFilecall now includes the cleartext path, enhancing error messages and debugging capabilities.src/test/java/org/cryptomator/cryptofs/fh/FileHeaderHolderTest.java (1)
44-46: LGTM! Clear mock object naming.The mock objects for both cleartext and ciphertext paths are well-named and clearly distinguish their purposes.
src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java (1)
31-31: LGTM! Enhanced path management.The field type change from
PathtoClearAndCipherPathimproves path management by keeping related paths together.src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFilesTest.java (2)
25-26: LGTM! Improved test isolation using @tempdir.Using
@TempDirfor file paths ensures proper test isolation and cleanup.
135-135: LGTM! Thorough path verification.The verification ensures both cleartext and ciphertext paths are correctly updated during file operations.
src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java (2)
63-65: LGTM! Method signatures updated to include cleartext paths.The changes consistently add cleartext path parameters across all relevant methods, improving path tracking and aligning with the PR objectives.
Also applies to: 68-69, 74-75
127-163: LGTM! TwoPhaseMove class correctly updated.The changes to TwoPhaseMove maintain atomic operations while properly handling both cleartext and ciphertext paths.
src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java (1)
57-58: LGTM! Test setup correctly mocks both path types.The test setup properly initializes both cleartext and ciphertext paths using descriptive mock names.
src/test/java/org/cryptomator/cryptofs/SymlinksTest.java (1)
35-35: LGTM! Test cases updated to include cleartext paths.The test cases have been consistently updated to include cleartext paths in all relevant method calls.
Also applies to: 73-73, 87-87, 122-123, 169-171
src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java (1)
70-73: LGTM! Test initialization properly handles path setup.The new BeforeEach method ensures consistent path initialization for each test case.
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java (3)
405-405: LGTM! Enhanced error context by including cleartext path.The change improves error handling by providing both cleartext and ciphertext paths to
openCryptoFiles.getOrCreate(), which will help in generating more meaningful error messages.
591-591: LGTM! Enhanced error context for symlink moves.The change improves error handling by providing the cleartext target path to
openCryptoFiles.prepareMove(), which will help in generating more meaningful error messages during symlink move operations.
607-607: LGTM! Enhanced error context for file moves.The change improves error handling by providing the cleartext target path to
openCryptoFiles.prepareMove(), which will help in generating more meaningful error messages during file move operations.src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java (3)
456-456: LGTM! Test setup updated to match implementation changes.The test setup correctly reflects the changes in the implementation by passing both cleartext and ciphertext paths to
openCryptoFiles.getOrCreate().
755-755: LGTM! Test updated to match implementation changes for symlink moves.The test correctly reflects the changes in the implementation by passing the cleartext target path to
openCryptoFiles.prepareMove().
772-772: LGTM! Test updated to match implementation changes for file moves.The test correctly reflects the changes in the implementation by passing the cleartext target path to
openCryptoFiles.prepareMove().
| public String toString() { | ||
| return "OpenCryptoFile(path=" + currentFilePath.toString() + ")"; | ||
| var paths = currentFilePaths.get(); | ||
| return "OpenCryptoFile(path=" + (paths != null ? paths.ciphertextPath().toString() : "[deleted]") + ")"; | ||
| } |
There was a problem hiding this comment.
Add null check before accessing ciphertextPath.
The toString method might throw a NPE if paths is null and ciphertextPath() is called.
Apply this diff to add a null check:
- return "OpenCryptoFile(path=" + (paths != null ? paths.ciphertextPath().toString() : "[deleted]") + ")";
+ return "OpenCryptoFile(path=" + (paths != null && paths.ciphertextPath() != null ? paths.ciphertextPath().toString() : "[deleted]") + ")";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public String toString() { | |
| return "OpenCryptoFile(path=" + currentFilePath.toString() + ")"; | |
| var paths = currentFilePaths.get(); | |
| return "OpenCryptoFile(path=" + (paths != null ? paths.ciphertextPath().toString() : "[deleted]") + ")"; | |
| } | |
| public String toString() { | |
| var paths = currentFilePaths.get(); | |
| return "OpenCryptoFile(path=" + (paths != null && paths.ciphertextPath() != null ? paths.ciphertextPath().toString() : "[deleted]") + ")"; | |
| } |
This PR improves the decryption failed event by adding the cleartext path to it, making the event more meaningful.
Unfortunately, a lot of adjustments had to be made to make it possible. The problem was that inside the file handle package (containing
OpenCryptoFile,ChunkCacheand other decrytion related classes) the cleartext path did not exist anymore since everything could be done solely working with the ciphertext path.With this PR, a new record class is introduced:
ClearAndCipherPath. It represents a resource inside the cryptographic file system (symlink, directory or file) and stores its cleartext name (visible by the consumer) and its (principal) ciphertext path on the harddisk. Since the invariant "the cleartext path changes if and only if the ciphertext path changes` holds, it is quite logical to group those in a single object and update them simultanously.Coming back to the goal of the PR, the
ClearAndCipherPathobject is handed over to theOpenCryptoFileModuleclass, such that it is available in thefhpackage and the cleartext path is available if decryption fails.