Feature: Public method for decrypting ciphertext name. #263
Conversation
WalkthroughThe pull request introduces significant modifications across several classes within the Possibly related PRs
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: 3
🧹 Outside diff range and nitpick comments (7)
src/main/java/org/cryptomator/cryptofs/FileNameDecryptor.java (2)
63-63: Correct typo in exception messageThere is a typo in the exception message: "Filname" should be "Filename".
Apply this diff to correct the typo:
-throw new FileSystemException(ciphertextNode.toString(), null, "Filname decryption failed:" + e); +throw new FileSystemException(ciphertextNode.toString(), null, "Filename decryption failed:" + e);
76-76: Clarify error message invalidatePathmethodThe error message in the
validatePathmethod could be clearer. Since both conditions (correct extension and minimum filename length) need to be met, consider updating "or" to "and/or" to avoid confusion.Apply this diff to improve the error message:
-throw new IllegalArgumentException("Node %s does not end with %s or %s or filename is shorter than %d characters.".formatted(absolutePath, Constants.CRYPTOMATOR_FILE_SUFFIX, Constants.DEFLATED_FILE_SUFFIX, Constants.MIN_CIPHER_NAME_LENGTH)); +throw new IllegalArgumentException("Node %s does not end with %s or %s and/or filename is shorter than %d characters.".formatted(absolutePath, Constants.CRYPTOMATOR_FILE_SUFFIX, Constants.DEFLATED_FILE_SUFFIX, Constants.MIN_CIPHER_NAME_LENGTH));src/test/java/org/cryptomator/cryptofs/FileNameDecryptorTest.java (1)
116-116: HandleCryptoExceptionspecifically ininflateThrowstestIn the
inflateThrowstest, the mock throws aTestCryptoException, but the catch block in the production code only handlesIOException. This mismatch may lead to the exception not being correctly tested.Consider adjusting the test to throw an
IOExceptionor update the production code to handleCryptoExceptionappropriately.src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java (4)
Line range hint
108-131: Consider reducing the number of dependencies and using constructor injectionThe large number of mocked dependencies (20+) suggests that
CryptoFileSystemImplmight have too many responsibilities. Consider:
- Breaking down the class into smaller components
- Using constructor injection instead of field injection
- Implementing a builder pattern for test setup
This would improve:
- Testability by making dependencies explicit
- Maintainability by reducing coupling
- Readability by encapsulating test setup
Example refactor:
@Builder class CryptoFileSystemImpl { private final FileSystemProvider provider; private final CryptoFileSystems cryptoFileSystems; // ... other dependencies public CryptoFileSystemImpl(FileSystemProvider provider, CryptoFileSystems cryptoFileSystems, // ... other dependencies ) { this.provider = provider; this.cryptoFileSystems = cryptoFileSystems; // ... } } // In test: @Test void testMethod() { var sut = CryptoFileSystemImpl.builder() .provider(mock(FileSystemProvider.class)) .cryptoFileSystems(mock(CryptoFileSystems.class)) // ... .build(); // test logic }
Line range hint
1037-1077: Simplify mock chains using test doublesThe current implementation uses complex mock chains which can make tests brittle and harder to maintain. Consider:
- Creating test-specific implementations for simpler interfaces
- Using
ArgumentCaptorto verify complex interactions- Extracting common mock setup into helper methods
Example:
class TestCryptoPath implements CryptoPath { private final String path; TestCryptoPath(String path) { this.path = path; } @Override public String toString() { return path; } // implement other methods } @Test void testMethod() { var path = new TestCryptoPath("/test/path"); // test logic using path }
Line range hint
1164-1187: Improve error handling test organization and messagesThe error handling tests could be improved by:
- Grouping related error cases using
@Nestedclasses- Using more descriptive error messages
- Adding test cases for concurrent access scenarios
Example:
@Nested class ErrorHandling { @Nested class AccessDenied { @Test void throwsAccessDeniedWithDescriptiveMessage() { var e = assertThrows(AccessDeniedException.class, () -> inTest.checkAccess(path, AccessMode.WRITE)); assertEquals("Cannot write to read-only file: " + path, e.getMessage()); } } }
Line range hint
1279-1307: Split multi-behavior tests into focused test methodsSome test methods verify multiple behaviors. Consider:
- Splitting into smaller, focused tests
- Using more descriptive test names following the given/when/then pattern
- Extracting common setup into helper methods
Example refactor:
@Test void givenReadOnlyFile_whenCheckingWriteAccess_thenThrowsAccessDenied() { // test logic } @Test void givenReadOnlyFile_whenCheckingReadAccess_thenSucceeds() { // test logic }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/main/java/org/cryptomator/cryptofs/CryptoFileSystem.java(2 hunks)src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java(4 hunks)src/main/java/org/cryptomator/cryptofs/FileNameDecryptor.java(1 hunks)src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java(2 hunks)src/test/java/org/cryptomator/cryptofs/DirectoryIdBackupTest.java(2 hunks)src/test/java/org/cryptomator/cryptofs/FileNameDecryptorTest.java(1 hunks)src/test/java/org/cryptomator/cryptofs/health/dirid/OrphanContentDirTest.java(2 hunks)src/test/java/org/cryptomator/cryptofs/util/TestCryptoException.java(1 hunks)
🔇 Additional comments (8)
src/test/java/org/cryptomator/cryptofs/util/TestCryptoException.java (1)
1-11: LGTM! Well-structured test utility class.
The implementation is clean and follows test utility best practices:
- Properly placed in the test util package
- Extends the appropriate base exception
- Minimal implementation sufficient for test purposes
src/test/java/org/cryptomator/cryptofs/DirectoryIdBackupTest.java (1)
108-108: LGTM! Consistent use of TestCryptoException.
Good refactoring to use the standardized TestCryptoException instead of a custom exception class.
src/test/java/org/cryptomator/cryptofs/health/dirid/OrphanContentDirTest.java (1)
230-230: LGTM! Consistent exception handling update.
Good update to use TestCryptoException in the expected exceptions list, maintaining consistency with the new test exception handling approach.
src/main/java/org/cryptomator/cryptofs/CryptoFileSystem.java (1)
46-64: Method getCleartextName is well-documented and aligns with class responsibilities
The addition of the abstract method getCleartextName(Path ciphertextNode) enhances the functionality of CryptoFileSystem. The method is appropriately documented with clear parameters, return value, and exceptions.
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java (3)
Line range hint 103-133: Update constructor to match new dependencies
The constructor of CryptoFileSystemImpl has been updated to include FileNameDecryptor. Ensure that all dependencies are properly injected and initialized.
156-159: getCleartextName method implementation uses fileNameDecryptor correctly
The implementation of getCleartextName(Path ciphertextNode) correctly delegates to fileNameDecryptor.decryptFilename(ciphertextNode), aligning with the design.
160-160: Consider handling potential exceptions from decryptFilename
While decryptFilename throws IOException and UnsupportedOperationException, consider if additional exception handling is needed in getCleartextName to provide more context or to handle unexpected exceptions.
Ensure that any exceptions are appropriately propagated or handled.
src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java (1)
Line range hint 582-582: Address TODO comment regarding behavior verification
The TODO comment questions whether the current behavior is correct. This needs to be:
- Verified against the specification
- Documented with the reasoning
- Tested appropriately
Would you like me to help create additional test cases after the behavior is verified?
| } | ||
|
|
||
| boolean isAtCipherNodeLevel(Path p) { | ||
| return vaultPath.relativize(p).getNameCount() == 4; //TODO: relativize is defined for two relative Paths. For two absolute paths, the result depends on the OS |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure OS-independent behavior when using Path.relativize
The use of vaultPath.relativize(p) may not behave consistently across different operating systems when both paths are absolute, as noted in the TODO comment. This could lead to incorrect path depth calculations.
Consider adjusting the implementation to ensure OS-independent behavior. One approach is to convert both paths to relative paths before calling relativize, or manually calculate the depth using getNameCount() after verifying that p starts with vaultPath.
| var name = "toDecrypt"; | ||
| var ciphertextNode = tmpPath.resolve(name + ".c9s"); |
There was a problem hiding this comment.
Incorrect file extension in test case
In the notDecryptableCiphertext test, the file extension used is ".c9s", which may not match the expected file extensions defined in Constants.
Apply this diff to correct the file extension:
var ciphertextNode = tmpPath.resolve(name + ".c9s");
+// Assuming the correct extension is Constants.CRYPTOMATOR_FILE_SUFFIX or Constants.DEFLATED_FILE_SUFFIX
+var ciphertextNode = tmpPath.resolve(name + Constants.CRYPTOMATOR_FILE_SUFFIX);Ensure that test cases use valid file extensions to accurately reflect production scenarios.
Committable suggestion skipped: line range outside the PR's diff.
This PR adds the method
getCleartextName(Path p)to the CryptoFileSystem interface, which decrypts the filename of the given path and return the cleartext name.Note: The method does not compute the whole cleartext path, since for this a recursive directory listing of the cryptoFileSystem is needed.
The feature is encapsulated in its own class
FileNameDecryptor, which first validates the input path and second tries to decrypt it by using the DirId backup file. If the directory does not have such a file, anUnsupportedOperationExceptionis thrown.