From 1ec43e6d80f5599bd8b24dd9ba6f128002d03c7d Mon Sep 17 00:00:00 2001 From: Tobias Hagemann Date: Mon, 27 Mar 2017 11:42:47 +0200 Subject: [PATCH 01/10] Update README.md --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 7d6ea86f..974b4ba5 100644 --- a/README.md +++ b/README.md @@ -1,8 +1,8 @@ ![cryptomator](cryptomator.png) [![Build Status](https://travis-ci.org/cryptomator/cryptofs.svg?branch=develop)](https://travis-ci.org/cryptomator/cryptofs) -[![codecov](https://codecov.io/gh/cryptomator/cryptofs/branch/develop/graph/badge.svg)](https://codecov.io/gh/cryptomator/cryptofs) [![Codacy Badge](https://api.codacy.com/project/badge/Grade/7248ca7d466843f785f79f33374302c2)](https://www.codacy.com/app/cryptomator/cryptofs) +[![Codacy Badge](https://api.codacy.com/project/badge/Coverage/7248ca7d466843f785f79f33374302c2)](https://www.codacy.com/app/cryptomator/cryptofs?utm_source=github.com&utm_medium=referral&utm_content=cryptomator/cryptofs&utm_campaign=Badge_Coverage) [![Coverity Scan Build Status](https://scan.coverity.com/projects/10006/badge.svg)](https://scan.coverity.com/projects/cryptomator-cryptofs) **CryptoFS** - Implementation of the [Cryptomator](https://github.com/cryptomator/cryptomator) encryption scheme. From be1ca161e46ba336e08a72dac5a4ad1c4a3f4bc2 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Wed, 29 Mar 2017 11:55:24 +0200 Subject: [PATCH 02/10] Simplified code --- .../cryptofs/CryptoDirectoryStream.java | 15 +++++++-------- .../cryptofs/CryptoDirectoryStreamTest.java | 10 ++++------ 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/CryptoDirectoryStream.java b/src/main/java/org/cryptomator/cryptofs/CryptoDirectoryStream.java index 80348c64..fd05c6a3 100644 --- a/src/main/java/org/cryptomator/cryptofs/CryptoDirectoryStream.java +++ b/src/main/java/org/cryptomator/cryptofs/CryptoDirectoryStream.java @@ -19,6 +19,8 @@ import java.util.function.Consumer; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Stream; +import java.util.stream.StreamSupport; import org.cryptomator.cryptofs.CryptoPathMapper.Directory; import org.cryptomator.cryptolib.api.AuthenticationFailedException; @@ -26,8 +28,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import com.google.common.collect.Iterators; - class CryptoDirectoryStream implements DirectoryStream { private static final Pattern BASE32_PATTERN = Pattern.compile("^0?(([A-Z2-7]{8})*[A-Z2-7=]{8})"); @@ -57,12 +57,11 @@ public CryptoDirectoryStream(Directory ciphertextDir, Path cleartextDir, FileNam @Override public Iterator iterator() { - Iterator ciphertextPathIter = ciphertextDirStream.iterator(); - Iterator longCiphertextPathOrNullIter = Iterators.transform(ciphertextPathIter, this::inflateIfNeeded); - Iterator longCiphertextPathIter = Iterators.filter(longCiphertextPathOrNullIter, Objects::nonNull); - Iterator cleartextPathOrNullIter = Iterators.transform(longCiphertextPathIter, this::decrypt); - Iterator cleartextPathIter = Iterators.filter(cleartextPathOrNullIter, Objects::nonNull); - return Iterators.filter(cleartextPathIter, this::isAcceptableByFilter); + Stream pathIter = StreamSupport.stream(ciphertextDirStream.spliterator(), false); + Stream inflated = pathIter.map(this::inflateIfNeeded).filter(Objects::nonNull); + Stream decrypted = inflated.map(this::decrypt).filter(Objects::nonNull); + Stream filtered = decrypted.filter(this::isAcceptableByFilter); + return filtered.iterator(); } private Path inflateIfNeeded(Path ciphertextPath) { diff --git a/src/test/java/org/cryptomator/cryptofs/CryptoDirectoryStreamTest.java b/src/test/java/org/cryptomator/cryptofs/CryptoDirectoryStreamTest.java index 536e196c..be79b5cb 100644 --- a/src/test/java/org/cryptomator/cryptofs/CryptoDirectoryStreamTest.java +++ b/src/test/java/org/cryptomator/cryptofs/CryptoDirectoryStreamTest.java @@ -8,7 +8,7 @@ *******************************************************************************/ package org.cryptomator.cryptofs; -import static org.mockito.Matchers.any; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; @@ -20,10 +20,10 @@ import java.nio.file.Paths; import java.nio.file.spi.FileSystemProvider; import java.util.ArrayList; -import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.NoSuchElementException; +import java.util.Spliterators; import java.util.function.Consumer; import org.apache.commons.lang3.StringUtils; @@ -39,8 +39,6 @@ import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; -import com.google.common.collect.Iterators; - public class CryptoDirectoryStreamTest { private static final Consumer DO_NOTHING_ON_CLOSE = ignored -> { @@ -111,7 +109,7 @@ public void testDirListing() throws IOException { ciphertextFileNames.add(filenameCryptor.encryptFilename("four", "foo".getBytes()) + ".lng"); ciphertextFileNames.add(filenameCryptor.encryptFilename("invalid", "bar".getBytes())); ciphertextFileNames.add("alsoInvalid"); - Mockito.when(dirStream.iterator()).thenReturn(Iterators.transform(ciphertextFileNames.iterator(), cleartextPath::resolve)); + Mockito.when(dirStream.spliterator()).thenReturn(ciphertextFileNames.stream().map(cleartextPath::resolve).spliterator()); try (CryptoDirectoryStream stream = new CryptoDirectoryStream(new Directory("foo", ciphertextDirPath), cleartextPath, filenameCryptor, longFileNameProvider, ACCEPT_ALL, DO_NOTHING_ON_CLOSE, finallyUtil)) { Iterator iter = stream.iterator(); @@ -133,7 +131,7 @@ public void testDirListing() throws IOException { public void testDirListingForEmptyDir() throws IOException { Path cleartextPath = Paths.get("/foo/bar"); - Mockito.when(dirStream.iterator()).thenReturn(Collections.emptyIterator()); + Mockito.when(dirStream.spliterator()).thenReturn(Spliterators.emptySpliterator()); try (CryptoDirectoryStream stream = new CryptoDirectoryStream(new Directory("foo", ciphertextDirPath), cleartextPath, filenameCryptor, longFileNameProvider, ACCEPT_ALL, DO_NOTHING_ON_CLOSE, finallyUtil)) { Iterator iter = stream.iterator(); From 9026a35114d846b6fc961d3e21301875ed4a3c14 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Wed, 29 Mar 2017 16:48:03 +0200 Subject: [PATCH 03/10] restored basic conflict resolver functionality --- .../cryptofs/ConflictResolver.java | 89 +++++++++++++++ .../cryptofs/LongFileNameProvider.java | 2 +- .../cryptofs/ConflictResolverTest.java | 104 ++++++++++++++++++ 3 files changed, 194 insertions(+), 1 deletion(-) create mode 100644 src/main/java/org/cryptomator/cryptofs/ConflictResolver.java create mode 100644 src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java diff --git a/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java b/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java new file mode 100644 index 00000000..715058ef --- /dev/null +++ b/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java @@ -0,0 +1,89 @@ +package org.cryptomator.cryptofs; + +import static org.cryptomator.cryptofs.Constants.DIR_PREFIX; +import static org.cryptomator.cryptofs.Constants.NAME_SHORTENING_THRESHOLD; +import static org.cryptomator.cryptofs.LongFileNameProvider.LONG_NAME_FILE_EXT; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.UUID; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import javax.inject.Inject; + +import org.apache.commons.lang3.StringUtils; +import org.cryptomator.cryptolib.api.AuthenticationFailedException; +import org.cryptomator.cryptolib.api.FileNameCryptor; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@PerFileSystem +class ConflictResolver { + + private static final Logger LOG = LoggerFactory.getLogger(ConflictResolver.class); + private static final Pattern BASE32_PATTERN = Pattern.compile("(([A-Z2-7]{8})*[A-Z2-7=]{8})"); + private static final int UUID_FIRST_GROUP_STRLEN = 8; + + private final LongFileNameProvider longFileNameProvider; + private final FileNameCryptor filenameCryptor; + + @Inject + public ConflictResolver(LongFileNameProvider longFileNameProvider, FileNameCryptor filenameCryptor) { + this.longFileNameProvider = longFileNameProvider; + this.filenameCryptor = filenameCryptor; + } + + public Path resolveConflicts(Path ciphertextPath, String dirId) throws IOException { + String ciphertextFileName = ciphertextPath.getFileName().toString(); + String resolvedCiphertextFileName = resolveNameConflictIfNecessary(ciphertextPath.getParent(), ciphertextFileName, dirId); + return ciphertextPath.resolveSibling(resolvedCiphertextFileName); + } + + private String resolveNameConflictIfNecessary(Path directory, String ciphertextFileName, String dirId) throws IOException { + String basename = StringUtils.removeEnd(ciphertextFileName, LONG_NAME_FILE_EXT); + Matcher m = BASE32_PATTERN.matcher(basename); + if (!m.matches() && m.find(0)) { + // no full match, but still contains base32 -> partial match + String ciphertext = m.group(); + if (LongFileNameProvider.isDeflated(ciphertextFileName)) { + ciphertext = longFileNameProvider.inflate(ciphertext + LONG_NAME_FILE_EXT); + } + return getAlternativeCiphertextName(directory, ciphertext, dirId); + } else { + // full match or no match at all -> nothing to resolve + return ciphertextFileName; + } + } + + private String getAlternativeCiphertextName(Path directory, String ciphertextFileName, String dirId) throws IOException { + boolean isDirectory = ciphertextFileName.startsWith(DIR_PREFIX); + String ciphertext = StringUtils.removeStart(ciphertextFileName, DIR_PREFIX); + try { + String cleartext = filenameCryptor.decryptFilename(ciphertext, dirId.getBytes(StandardCharsets.UTF_8)); + Path canonicalPath = directory.resolve(ciphertextFileName); + Path alternativePath = canonicalPath; + while (Files.exists(alternativePath)) { + String alternativeCleartext = cleartext + " (Conflict " + createConflictId() + ")"; + String alternativeCiphertext = filenameCryptor.encryptFilename(alternativeCleartext, dirId.getBytes(StandardCharsets.UTF_8)); + String alternativeCiphertextFileName = isDirectory ? DIR_PREFIX + alternativeCiphertext : alternativeCiphertext; + if (alternativeCiphertextFileName.length() >= NAME_SHORTENING_THRESHOLD) { + alternativeCiphertextFileName = longFileNameProvider.deflate(alternativeCiphertextFileName); + } + alternativePath = directory.resolve(alternativeCiphertextFileName); + } + return alternativePath.getFileName().toString(); + } catch (AuthenticationFailedException e) { + // not decryptable, no need to resolve any kind of conflict + LOG.info("Found valid Base32 string, which is an invalid ciphertext: {}", ciphertextFileName); + return ciphertextFileName; + } + } + + private static String createConflictId() { + return UUID.randomUUID().toString().substring(0, UUID_FIRST_GROUP_STRLEN); + } + +} diff --git a/src/main/java/org/cryptomator/cryptofs/LongFileNameProvider.java b/src/main/java/org/cryptomator/cryptofs/LongFileNameProvider.java index 07410c8a..5e763d46 100644 --- a/src/main/java/org/cryptomator/cryptofs/LongFileNameProvider.java +++ b/src/main/java/org/cryptomator/cryptofs/LongFileNameProvider.java @@ -32,7 +32,7 @@ class LongFileNameProvider { private static final BaseEncoding BASE32 = BaseEncoding.base32(); private static final int MAX_CACHE_SIZE = 5000; - private static final String LONG_NAME_FILE_EXT = ".lng"; + public static final String LONG_NAME_FILE_EXT = ".lng"; private final Path metadataRoot; private final LoadingCache ids; diff --git a/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java b/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java new file mode 100644 index 00000000..bce243b0 --- /dev/null +++ b/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java @@ -0,0 +1,104 @@ +package org.cryptomator.cryptofs; + +import java.io.IOException; +import java.nio.file.FileSystem; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; +import java.nio.file.spi.FileSystemProvider; + +import org.cryptomator.cryptolib.api.FileNameCryptor; +import org.hamcrest.CoreMatchers; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; +import org.mockito.invocation.InvocationOnMock; + +public class ConflictResolverTest { + + private LongFileNameProvider longFileNameProvider; + private FileNameCryptor filenameCryptor; + private ConflictResolver conflictResolver; + private String dirId; + private Path testFile; + private Path testFileName; + private Path testDir; + private FileSystem testFileSystem; + private FileSystemProvider testFileSystemProvider; + + @Before + public void setup() { + this.longFileNameProvider = Mockito.mock(LongFileNameProvider.class); + this.filenameCryptor = Mockito.mock(FileNameCryptor.class); + this.conflictResolver = new ConflictResolver(longFileNameProvider, filenameCryptor); + this.dirId = "foo"; + this.testFile = Mockito.mock(Path.class); + this.testFileName = Mockito.mock(Path.class); + this.testDir = Mockito.mock(Path.class); + this.testFileSystem = Mockito.mock(FileSystem.class); + this.testFileSystemProvider = Mockito.mock(FileSystemProvider.class); + + Mockito.when(testFile.getParent()).thenReturn(testDir); + Mockito.when(testFile.getFileName()).thenReturn(testFileName); + Mockito.when(testDir.resolve(Mockito.anyString())).then(this::resolveChildOfTestDir); + Mockito.when(testFile.resolveSibling(Mockito.anyString())).then(this::resolveChildOfTestDir); + Mockito.when(testFile.getFileSystem()).thenReturn(testFileSystem); + Mockito.when(testFileSystem.provider()).thenReturn(testFileSystemProvider); + } + + private Path resolveChildOfTestDir(InvocationOnMock invocation) { + Path result = Mockito.mock(Path.class); + Path resultName = Mockito.mock(Path.class); + Mockito.when(result.getFileName()).thenReturn(resultName); + Mockito.when(resultName.toString()).thenReturn(invocation.getArgument(0)); + Mockito.when(result.getParent()).thenReturn(testDir); + Mockito.when(result.getFileSystem()).thenReturn(testFileSystem); + return result; + } + + @Test + public void testPassthroughValidBase32NormalFile() throws IOException { + Mockito.when(testFileName.toString()).thenReturn("ABCDEF=="); + Path resolved = conflictResolver.resolveConflicts(testFile, dirId); + Mockito.verifyNoMoreInteractions(filenameCryptor); + Mockito.verifyNoMoreInteractions(longFileNameProvider); + Assert.assertEquals(testFile.getFileName().toString(), resolved.getFileName().toString()); + } + + @Test + public void testPassthroughValidBase32LongFile() throws IOException { + Mockito.when(testFileName.toString()).thenReturn("ABCDEF==.lng"); + Path resolved = conflictResolver.resolveConflicts(testFile, dirId); + Mockito.verifyNoMoreInteractions(filenameCryptor); + Mockito.verifyNoMoreInteractions(longFileNameProvider); + Assert.assertEquals(testFile.getFileName().toString(), resolved.getFileName().toString()); + } + + @Test + public void testAlternativeNameForNormalFile() throws IOException { + String ciphertextName = "ABCDEFGH2345===="; + Mockito.when(testFileName.toString()).thenReturn("ABCDEF== (1)"); + Mockito.when(filenameCryptor.decryptFilename(Mockito.eq("ABCDEF=="), Mockito.any())).thenReturn("abcdef"); + Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("abcdef (Conflict "), Mockito.any())).thenReturn(ciphertextName); + Mockito.doThrow(new NoSuchFileException(ciphertextName)).when(testFileSystemProvider).checkAccess(Mockito.argThat(p -> p.getFileName().toString().equals(ciphertextName))); + Path resolved = conflictResolver.resolveConflicts(testFile, dirId); + Mockito.verifyNoMoreInteractions(longFileNameProvider); + Assert.assertThat(resolved.getFileName().toString(), CoreMatchers.containsString(ciphertextName)); + } + + @Test + public void testAlternativeNameForLongFile() throws IOException { + String longCiphertextName = "ABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGH2345===="; + assert longCiphertextName.length() > Constants.NAME_SHORTENING_THRESHOLD; + Mockito.when(testFileName.toString()).thenReturn("ABCDEF== (1).lng"); + Mockito.when(longFileNameProvider.inflate("ABCDEF==.lng")).thenReturn("FEDCBA=="); + Mockito.when(longFileNameProvider.deflate(longCiphertextName)).thenReturn("FEDCBA==.lng"); + Mockito.when(filenameCryptor.decryptFilename(Mockito.eq("FEDCBA=="), Mockito.any())).thenReturn("fedcba"); + Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("fedcba (Conflict "), Mockito.any())).thenReturn(longCiphertextName); + Mockito.doThrow(new NoSuchFileException(longCiphertextName)).when(testFileSystemProvider).checkAccess(Mockito.argThat(p -> p.getFileName().toString().equals("FEDCBA==.lng"))); + Path resolved = conflictResolver.resolveConflicts(testFile, dirId); + Mockito.verify(longFileNameProvider).deflate(longCiphertextName); + Assert.assertThat(resolved.getFileName().toString(), CoreMatchers.containsString("FEDCBA==.lng")); + } + +} From d792dcfae1c054b0677ee2bf152307b73130794e Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Fri, 31 Mar 2017 16:50:53 +0200 Subject: [PATCH 04/10] Taught ConflictResolver to silently delete conflicting directory files, if they contain the same dirId as the canonical file. --- .../cryptofs/ConflictResolver.java | 45 +++++++++++++++---- .../cryptofs/ConflictResolverTest.java | 43 +++++++++++++++++- 2 files changed, 78 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java b/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java index 715058ef..8536b4a1 100644 --- a/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java +++ b/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java @@ -5,9 +5,12 @@ import static org.cryptomator.cryptofs.LongFileNameProvider.LONG_NAME_FILE_EXT; import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.channels.ReadableByteChannel; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardOpenOption; import java.util.UUID; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -25,6 +28,7 @@ class ConflictResolver { private static final Logger LOG = LoggerFactory.getLogger(ConflictResolver.class); private static final Pattern BASE32_PATTERN = Pattern.compile("(([A-Z2-7]{8})*[A-Z2-7=]{8})"); + private static final int MAX_DIR_FILE_SIZE = 87; // "normal" file header has 88 bytes private static final int UUID_FIRST_GROUP_STRLEN = 8; private final LongFileNameProvider longFileNameProvider; @@ -51,24 +55,30 @@ private String resolveNameConflictIfNecessary(Path directory, String ciphertextF if (LongFileNameProvider.isDeflated(ciphertextFileName)) { ciphertext = longFileNameProvider.inflate(ciphertext + LONG_NAME_FILE_EXT); } - return getAlternativeCiphertextName(directory, ciphertext, dirId); + return getAlternativeCiphertextName(directory, ciphertextFileName, ciphertext, dirId); } else { // full match or no match at all -> nothing to resolve return ciphertextFileName; } } - private String getAlternativeCiphertextName(Path directory, String ciphertextFileName, String dirId) throws IOException { - boolean isDirectory = ciphertextFileName.startsWith(DIR_PREFIX); - String ciphertext = StringUtils.removeStart(ciphertextFileName, DIR_PREFIX); + private String getAlternativeCiphertextName(Path directory, String filename, String ciphertext, String dirId) throws IOException { + final boolean isDirectory = filename.startsWith(DIR_PREFIX); + final String prefix = isDirectory ? DIR_PREFIX : ""; + final Path conflictingPath = directory.resolve(filename); + final Path canonicalPath = directory.resolve(prefix + ciphertext); + if (isDirectory && hasSameDirFileContent(conflictingPath, canonicalPath)) { + // there must not be two directories pointing to the same dirId. In this case no human interaction is needed to resolve this conflict: + Files.deleteIfExists(conflictingPath); + return canonicalPath.getFileName().toString(); + } try { String cleartext = filenameCryptor.decryptFilename(ciphertext, dirId.getBytes(StandardCharsets.UTF_8)); - Path canonicalPath = directory.resolve(ciphertextFileName); Path alternativePath = canonicalPath; while (Files.exists(alternativePath)) { String alternativeCleartext = cleartext + " (Conflict " + createConflictId() + ")"; String alternativeCiphertext = filenameCryptor.encryptFilename(alternativeCleartext, dirId.getBytes(StandardCharsets.UTF_8)); - String alternativeCiphertextFileName = isDirectory ? DIR_PREFIX + alternativeCiphertext : alternativeCiphertext; + String alternativeCiphertextFileName = prefix + alternativeCiphertext; if (alternativeCiphertextFileName.length() >= NAME_SHORTENING_THRESHOLD) { alternativeCiphertextFileName = longFileNameProvider.deflate(alternativeCiphertextFileName); } @@ -77,8 +87,27 @@ private String getAlternativeCiphertextName(Path directory, String ciphertextFil return alternativePath.getFileName().toString(); } catch (AuthenticationFailedException e) { // not decryptable, no need to resolve any kind of conflict - LOG.info("Found valid Base32 string, which is an invalid ciphertext: {}", ciphertextFileName); - return ciphertextFileName; + LOG.info("Found valid Base32 string, which is an invalid ciphertext: {}", filename); + return filename; + } + } + + /** + * @param conflictingPath Path to a potentially conflicting file supposedly containing a directory id + * @param canonicalPath Path to the canonical file containing a directory id + * @return true if the first {@value #MAX_DIR_FILE_SIZE} bytes are equal in both files. + * @throws IOException If an I/O exception occurs while reading either file. + */ + private boolean hasSameDirFileContent(Path conflictingPath, Path canonicalPath) throws IOException { + try (ReadableByteChannel in1 = Files.newByteChannel(conflictingPath, StandardOpenOption.READ); // + ReadableByteChannel in2 = Files.newByteChannel(canonicalPath, StandardOpenOption.READ)) { + ByteBuffer buf1 = ByteBuffer.allocate(MAX_DIR_FILE_SIZE); + ByteBuffer buf2 = ByteBuffer.allocate(MAX_DIR_FILE_SIZE); + in1.read(buf1); + in2.read(buf2); + buf1.flip(); + buf2.flip(); + return buf1.compareTo(buf2) == 0; } } diff --git a/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java b/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java index bce243b0..7279bdb8 100644 --- a/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java +++ b/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java @@ -1,6 +1,10 @@ package org.cryptomator.cryptofs; import java.io.IOException; +import java.lang.reflect.Field; +import java.nio.ByteBuffer; +import java.nio.channels.FileChannel; +import java.nio.channels.spi.AbstractInterruptibleChannel; import java.nio.file.FileSystem; import java.nio.file.NoSuchFileException; import java.nio.file.Path; @@ -11,8 +15,10 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.mockito.ArgumentMatcher; import org.mockito.Mockito; import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; public class ConflictResolverTest { @@ -56,6 +62,22 @@ private Path resolveChildOfTestDir(InvocationOnMock invocation) { return result; } + private ArgumentMatcher hasFileName(String name) { + return path -> { + Path filename = path.getFileName(); + assert filename != null; + return filename.toString().equals(name); + }; + } + + private Answer fillBufferWithBytes(byte[] bytes) { + return invocation -> { + ByteBuffer buffer = invocation.getArgument(0); + buffer.put(bytes); + return bytes.length; + }; + } + @Test public void testPassthroughValidBase32NormalFile() throws IOException { Mockito.when(testFileName.toString()).thenReturn("ABCDEF=="); @@ -80,7 +102,7 @@ public void testAlternativeNameForNormalFile() throws IOException { Mockito.when(testFileName.toString()).thenReturn("ABCDEF== (1)"); Mockito.when(filenameCryptor.decryptFilename(Mockito.eq("ABCDEF=="), Mockito.any())).thenReturn("abcdef"); Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("abcdef (Conflict "), Mockito.any())).thenReturn(ciphertextName); - Mockito.doThrow(new NoSuchFileException(ciphertextName)).when(testFileSystemProvider).checkAccess(Mockito.argThat(p -> p.getFileName().toString().equals(ciphertextName))); + Mockito.doThrow(new NoSuchFileException(ciphertextName)).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName(ciphertextName))); Path resolved = conflictResolver.resolveConflicts(testFile, dirId); Mockito.verifyNoMoreInteractions(longFileNameProvider); Assert.assertThat(resolved.getFileName().toString(), CoreMatchers.containsString(ciphertextName)); @@ -95,10 +117,27 @@ public void testAlternativeNameForLongFile() throws IOException { Mockito.when(longFileNameProvider.deflate(longCiphertextName)).thenReturn("FEDCBA==.lng"); Mockito.when(filenameCryptor.decryptFilename(Mockito.eq("FEDCBA=="), Mockito.any())).thenReturn("fedcba"); Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("fedcba (Conflict "), Mockito.any())).thenReturn(longCiphertextName); - Mockito.doThrow(new NoSuchFileException(longCiphertextName)).when(testFileSystemProvider).checkAccess(Mockito.argThat(p -> p.getFileName().toString().equals("FEDCBA==.lng"))); + Mockito.doThrow(new NoSuchFileException(longCiphertextName)).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName("FEDCBA==.lng"))); Path resolved = conflictResolver.resolveConflicts(testFile, dirId); Mockito.verify(longFileNameProvider).deflate(longCiphertextName); Assert.assertThat(resolved.getFileName().toString(), CoreMatchers.containsString("FEDCBA==.lng")); } + @Test + public void testAlternativeNameForDirectoryFile() throws IOException, ReflectiveOperationException { + Mockito.when(testFileName.toString()).thenReturn("0ABCDEF== (1)"); + FileChannel canonicalFc = Mockito.mock(FileChannel.class); + FileChannel conflictingFc = Mockito.mock(FileChannel.class); + Field channelCloseLockField = AbstractInterruptibleChannel.class.getDeclaredField("closeLock"); + channelCloseLockField.setAccessible(true); + channelCloseLockField.set(canonicalFc, new Object()); + channelCloseLockField.set(conflictingFc, new Object()); + Mockito.when(testFileSystemProvider.newByteChannel(Mockito.any(), Mockito.any(), Mockito.any())).thenReturn(conflictingFc, canonicalFc); + Mockito.when(canonicalFc.read(Mockito.any(ByteBuffer.class))).then(fillBufferWithBytes("12345".getBytes())); + Mockito.when(conflictingFc.read(Mockito.any(ByteBuffer.class))).then(fillBufferWithBytes("12345".getBytes())); + Path resolved = conflictResolver.resolveConflicts(testFile, dirId); + Mockito.verify(testFileSystemProvider).deleteIfExists(Mockito.argThat(hasFileName("0ABCDEF== (1)"))); + Assert.assertEquals("0ABCDEF==", resolved.getFileName().toString()); + } + } From 1737b9e804b646ed98e1fe0ae0096bf13d1087c9 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Mon, 3 Apr 2017 16:24:32 +0200 Subject: [PATCH 05/10] Fixes #9 (tests with actual files still outstanding) --- .../cryptofs/ConflictResolver.java | 67 +++++++++++++------ .../cryptofs/CryptoDirectoryStream.java | 18 ++++- .../cryptofs/DirectoryStreamFactory.java | 5 +- .../cryptofs/ConflictResolverTest.java | 25 ++++++- .../cryptofs/CryptoDirectoryStreamTest.java | 29 ++++---- .../cryptofs/DirectoryStreamFactoryTest.java | 3 +- 6 files changed, 103 insertions(+), 44 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java b/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java index 8536b4a1..7dfb2edf 100644 --- a/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java +++ b/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java @@ -10,6 +10,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; import java.util.UUID; import java.util.regex.Matcher; @@ -19,7 +20,7 @@ import org.apache.commons.lang3.StringUtils; import org.cryptomator.cryptolib.api.AuthenticationFailedException; -import org.cryptomator.cryptolib.api.FileNameCryptor; +import org.cryptomator.cryptolib.api.Cryptor; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -27,17 +28,17 @@ class ConflictResolver { private static final Logger LOG = LoggerFactory.getLogger(ConflictResolver.class); - private static final Pattern BASE32_PATTERN = Pattern.compile("(([A-Z2-7]{8})*[A-Z2-7=]{8})"); + private static final Pattern BASE32_PATTERN = Pattern.compile("0?(([A-Z2-7]{8})*[A-Z2-7=]{8})"); private static final int MAX_DIR_FILE_SIZE = 87; // "normal" file header has 88 bytes private static final int UUID_FIRST_GROUP_STRLEN = 8; private final LongFileNameProvider longFileNameProvider; - private final FileNameCryptor filenameCryptor; + private final Cryptor cryptor; @Inject - public ConflictResolver(LongFileNameProvider longFileNameProvider, FileNameCryptor filenameCryptor) { + public ConflictResolver(LongFileNameProvider longFileNameProvider, Cryptor cryptor) { this.longFileNameProvider = longFileNameProvider; - this.filenameCryptor = filenameCryptor; + this.cryptor = cryptor; } public Path resolveConflicts(Path ciphertextPath, String dirId) throws IOException { @@ -51,44 +52,68 @@ private String resolveNameConflictIfNecessary(Path directory, String ciphertextF Matcher m = BASE32_PATTERN.matcher(basename); if (!m.matches() && m.find(0)) { // no full match, but still contains base32 -> partial match - String ciphertext = m.group(); - if (LongFileNameProvider.isDeflated(ciphertextFileName)) { - ciphertext = longFileNameProvider.inflate(ciphertext + LONG_NAME_FILE_EXT); - } - return getAlternativeCiphertextName(directory, ciphertextFileName, ciphertext, dirId); + return resolveConflict(directory, ciphertextFileName, m.group(1), dirId); } else { // full match or no match at all -> nothing to resolve return ciphertextFileName; } } - private String getAlternativeCiphertextName(Path directory, String filename, String ciphertext, String dirId) throws IOException { - final boolean isDirectory = filename.startsWith(DIR_PREFIX); - final String prefix = isDirectory ? DIR_PREFIX : ""; - final Path conflictingPath = directory.resolve(filename); - final Path canonicalPath = directory.resolve(prefix + ciphertext); - if (isDirectory && hasSameDirFileContent(conflictingPath, canonicalPath)) { + private String resolveConflict(Path directory, String originalFileName, String base32match, String dirId) throws IOException { + final Path conflictingPath = directory.resolve(originalFileName); + final String ciphertext; + final boolean isDirectory; + final String dirPrefix; + final Path canonicalPath; + if (LongFileNameProvider.isDeflated(originalFileName)) { + String inflated = longFileNameProvider.inflate(base32match + LONG_NAME_FILE_EXT); + ciphertext = StringUtils.removeStart(inflated, DIR_PREFIX); + isDirectory = inflated.startsWith(DIR_PREFIX); + dirPrefix = isDirectory ? DIR_PREFIX : ""; + canonicalPath = directory.resolve(base32match + LONG_NAME_FILE_EXT); + } else { + ciphertext = base32match; + isDirectory = originalFileName.startsWith(DIR_PREFIX); + dirPrefix = isDirectory ? DIR_PREFIX : ""; + canonicalPath = directory.resolve(dirPrefix + ciphertext); + } + + /* + * Directory files must not result in symlink, make sure the conflicting file is not identical to the original. + */ + if (isDirectory && !Files.exists(canonicalPath)) { + // original file is, so this is no real conflict. + Files.move(conflictingPath, canonicalPath, StandardCopyOption.ATOMIC_MOVE); + return canonicalPath.getFileName().toString(); + } else if (isDirectory && hasSameDirFileContent(conflictingPath, canonicalPath)) { // there must not be two directories pointing to the same dirId. In this case no human interaction is needed to resolve this conflict: + LOG.info("Removing conflicting directory file {} (identical to {})", conflictingPath, canonicalPath); Files.deleteIfExists(conflictingPath); return canonicalPath.getFileName().toString(); } + + /* + * Find alternative name for conflicting file: + */ try { - String cleartext = filenameCryptor.decryptFilename(ciphertext, dirId.getBytes(StandardCharsets.UTF_8)); + String cleartext = cryptor.fileNameCryptor().decryptFilename(ciphertext, dirId.getBytes(StandardCharsets.UTF_8)); Path alternativePath = canonicalPath; while (Files.exists(alternativePath)) { String alternativeCleartext = cleartext + " (Conflict " + createConflictId() + ")"; - String alternativeCiphertext = filenameCryptor.encryptFilename(alternativeCleartext, dirId.getBytes(StandardCharsets.UTF_8)); - String alternativeCiphertextFileName = prefix + alternativeCiphertext; + String alternativeCiphertext = cryptor.fileNameCryptor().encryptFilename(alternativeCleartext, dirId.getBytes(StandardCharsets.UTF_8)); + String alternativeCiphertextFileName = dirPrefix + alternativeCiphertext; if (alternativeCiphertextFileName.length() >= NAME_SHORTENING_THRESHOLD) { alternativeCiphertextFileName = longFileNameProvider.deflate(alternativeCiphertextFileName); } alternativePath = directory.resolve(alternativeCiphertextFileName); } + LOG.info("Moving conflicting file {} to {}", conflictingPath, alternativePath); + Files.move(conflictingPath, alternativePath, StandardCopyOption.ATOMIC_MOVE); return alternativePath.getFileName().toString(); } catch (AuthenticationFailedException e) { // not decryptable, no need to resolve any kind of conflict - LOG.info("Found valid Base32 string, which is an invalid ciphertext: {}", filename); - return filename; + LOG.info("Found valid Base32 string, which is an invalid ciphertext: {}", originalFileName); + return originalFileName; } } diff --git a/src/main/java/org/cryptomator/cryptofs/CryptoDirectoryStream.java b/src/main/java/org/cryptomator/cryptofs/CryptoDirectoryStream.java index fd05c6a3..cd50ee1f 100644 --- a/src/main/java/org/cryptomator/cryptofs/CryptoDirectoryStream.java +++ b/src/main/java/org/cryptomator/cryptofs/CryptoDirectoryStream.java @@ -38,12 +38,13 @@ class CryptoDirectoryStream implements DirectoryStream { private final Path cleartextDir; private final FileNameCryptor filenameCryptor; private final LongFileNameProvider longFileNameProvider; + private final ConflictResolver conflictResolver; private final DirectoryStream.Filter filter; private final Consumer onClose; private final FinallyUtil finallyUtil; - public CryptoDirectoryStream(Directory ciphertextDir, Path cleartextDir, FileNameCryptor filenameCryptor, LongFileNameProvider longFileNameProvider, DirectoryStream.Filter filter, - Consumer onClose, FinallyUtil finallyUtil) throws IOException { + public CryptoDirectoryStream(Directory ciphertextDir, Path cleartextDir, FileNameCryptor filenameCryptor, LongFileNameProvider longFileNameProvider, ConflictResolver conflictResolver, + DirectoryStream.Filter filter, Consumer onClose, FinallyUtil finallyUtil) throws IOException { this.onClose = onClose; this.finallyUtil = finallyUtil; this.directoryId = ciphertextDir.dirId; @@ -52,18 +53,29 @@ public CryptoDirectoryStream(Directory ciphertextDir, Path cleartextDir, FileNam this.cleartextDir = cleartextDir; this.filenameCryptor = filenameCryptor; this.longFileNameProvider = longFileNameProvider; + this.conflictResolver = conflictResolver; this.filter = filter; } @Override public Iterator iterator() { Stream pathIter = StreamSupport.stream(ciphertextDirStream.spliterator(), false); - Stream inflated = pathIter.map(this::inflateIfNeeded).filter(Objects::nonNull); + Stream resolved = pathIter.map(this::resolveConflictingFileIfNeeded).filter(Objects::nonNull); + Stream inflated = resolved.map(this::inflateIfNeeded).filter(Objects::nonNull); Stream decrypted = inflated.map(this::decrypt).filter(Objects::nonNull); Stream filtered = decrypted.filter(this::isAcceptableByFilter); return filtered.iterator(); } + private Path resolveConflictingFileIfNeeded(Path potentiallyConflictingPath) { + try { + return conflictResolver.resolveConflicts(potentiallyConflictingPath, directoryId); + } catch (IOException e) { + LOG.warn("I/O exception while finding potentially conflicting file versions for {}.", potentiallyConflictingPath); + return null; + } + } + private Path inflateIfNeeded(Path ciphertextPath) { String fileName = ciphertextPath.getFileName().toString(); if (LongFileNameProvider.isDeflated(fileName)) { diff --git a/src/main/java/org/cryptomator/cryptofs/DirectoryStreamFactory.java b/src/main/java/org/cryptomator/cryptofs/DirectoryStreamFactory.java index ccfccf39..bb5d52ee 100644 --- a/src/main/java/org/cryptomator/cryptofs/DirectoryStreamFactory.java +++ b/src/main/java/org/cryptomator/cryptofs/DirectoryStreamFactory.java @@ -18,6 +18,7 @@ class DirectoryStreamFactory { private final Cryptor cryptor; private final LongFileNameProvider longFileNameProvider; + private final ConflictResolver conflictResolver; private final CryptoPathMapper cryptoPathMapper; private final FinallyUtil finallyUtil; @@ -26,9 +27,10 @@ class DirectoryStreamFactory { private volatile boolean closed = false; @Inject - public DirectoryStreamFactory(Cryptor cryptor, LongFileNameProvider longFileNameProvider, CryptoPathMapper cryptoPathMapper, FinallyUtil finallyUtil) { + public DirectoryStreamFactory(Cryptor cryptor, LongFileNameProvider longFileNameProvider, ConflictResolver conflictResolver, CryptoPathMapper cryptoPathMapper, FinallyUtil finallyUtil) { this.cryptor = cryptor; this.longFileNameProvider = longFileNameProvider; + this.conflictResolver = conflictResolver; this.cryptoPathMapper = cryptoPathMapper; this.finallyUtil = finallyUtil; } @@ -40,6 +42,7 @@ public DirectoryStream newDirectoryStream(CryptoPath cleartextDir, Filter< cleartextDir, // cryptor.fileNameCryptor(), // longFileNameProvider, // + conflictResolver, // filter, // closed -> streams.remove(closed), // finallyUtil); diff --git a/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java b/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java index 7279bdb8..4d964498 100644 --- a/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java +++ b/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java @@ -10,6 +10,7 @@ import java.nio.file.Path; import java.nio.file.spi.FileSystemProvider; +import org.cryptomator.cryptolib.api.Cryptor; import org.cryptomator.cryptolib.api.FileNameCryptor; import org.hamcrest.CoreMatchers; import org.junit.Assert; @@ -23,6 +24,7 @@ public class ConflictResolverTest { private LongFileNameProvider longFileNameProvider; + private Cryptor cryptor; private FileNameCryptor filenameCryptor; private ConflictResolver conflictResolver; private String dirId; @@ -35,8 +37,9 @@ public class ConflictResolverTest { @Before public void setup() { this.longFileNameProvider = Mockito.mock(LongFileNameProvider.class); + this.cryptor = Mockito.mock(Cryptor.class); this.filenameCryptor = Mockito.mock(FileNameCryptor.class); - this.conflictResolver = new ConflictResolver(longFileNameProvider, filenameCryptor); + this.conflictResolver = new ConflictResolver(longFileNameProvider, cryptor); this.dirId = "foo"; this.testFile = Mockito.mock(Path.class); this.testFileName = Mockito.mock(Path.class); @@ -44,6 +47,7 @@ public void setup() { this.testFileSystem = Mockito.mock(FileSystem.class); this.testFileSystemProvider = Mockito.mock(FileSystemProvider.class); + Mockito.when(cryptor.fileNameCryptor()).thenReturn(filenameCryptor); Mockito.when(testFile.getParent()).thenReturn(testDir); Mockito.when(testFile.getFileName()).thenReturn(testFileName); Mockito.when(testDir.resolve(Mockito.anyString())).then(this::resolveChildOfTestDir); @@ -64,6 +68,9 @@ private Path resolveChildOfTestDir(InvocationOnMock invocation) { private ArgumentMatcher hasFileName(String name) { return path -> { + if (path == null) { + return false; + } Path filename = path.getFileName(); assert filename != null; return filename.toString().equals(name); @@ -105,6 +112,7 @@ public void testAlternativeNameForNormalFile() throws IOException { Mockito.doThrow(new NoSuchFileException(ciphertextName)).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName(ciphertextName))); Path resolved = conflictResolver.resolveConflicts(testFile, dirId); Mockito.verifyNoMoreInteractions(longFileNameProvider); + Mockito.verify(testFileSystemProvider).move(Mockito.argThat(hasFileName("ABCDEF== (1)")), Mockito.argThat(hasFileName(ciphertextName)), Mockito.any()); Assert.assertThat(resolved.getFileName().toString(), CoreMatchers.containsString(ciphertextName)); } @@ -117,9 +125,10 @@ public void testAlternativeNameForLongFile() throws IOException { Mockito.when(longFileNameProvider.deflate(longCiphertextName)).thenReturn("FEDCBA==.lng"); Mockito.when(filenameCryptor.decryptFilename(Mockito.eq("FEDCBA=="), Mockito.any())).thenReturn("fedcba"); Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("fedcba (Conflict "), Mockito.any())).thenReturn(longCiphertextName); - Mockito.doThrow(new NoSuchFileException(longCiphertextName)).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName("FEDCBA==.lng"))); + Mockito.doThrow(new NoSuchFileException("FEDCBA==.lng")).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName("FEDCBA==.lng"))); Path resolved = conflictResolver.resolveConflicts(testFile, dirId); Mockito.verify(longFileNameProvider).deflate(longCiphertextName); + Mockito.verify(testFileSystemProvider).move(Mockito.argThat(hasFileName("ABCDEF== (1).lng")), Mockito.argThat(hasFileName("FEDCBA==.lng")), Mockito.any()); Assert.assertThat(resolved.getFileName().toString(), CoreMatchers.containsString("FEDCBA==.lng")); } @@ -132,7 +141,8 @@ public void testAlternativeNameForDirectoryFile() throws IOException, Reflective channelCloseLockField.setAccessible(true); channelCloseLockField.set(canonicalFc, new Object()); channelCloseLockField.set(conflictingFc, new Object()); - Mockito.when(testFileSystemProvider.newByteChannel(Mockito.any(), Mockito.any(), Mockito.any())).thenReturn(conflictingFc, canonicalFc); + Mockito.when(testFileSystemProvider.newByteChannel(Mockito.argThat(hasFileName("0ABCDEF==")), Mockito.any(), Mockito.any())).thenReturn(canonicalFc); + Mockito.when(testFileSystemProvider.newByteChannel(Mockito.argThat(hasFileName("0ABCDEF== (1)")), Mockito.any(), Mockito.any())).thenReturn(conflictingFc); Mockito.when(canonicalFc.read(Mockito.any(ByteBuffer.class))).then(fillBufferWithBytes("12345".getBytes())); Mockito.when(conflictingFc.read(Mockito.any(ByteBuffer.class))).then(fillBufferWithBytes("12345".getBytes())); Path resolved = conflictResolver.resolveConflicts(testFile, dirId); @@ -140,4 +150,13 @@ public void testAlternativeNameForDirectoryFile() throws IOException, Reflective Assert.assertEquals("0ABCDEF==", resolved.getFileName().toString()); } + @Test + public void testSilentRenamingForConflictingDirectoryFileWithMissingCanonicalFile() throws IOException, ReflectiveOperationException { + Mockito.when(testFileName.toString()).thenReturn("0ABCDEF== (1)"); + Mockito.doThrow(new NoSuchFileException("0ABCDEF==")).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName("0ABCDEF=="))); + Path resolved = conflictResolver.resolveConflicts(testFile, dirId); + Mockito.verify(testFileSystemProvider).move(Mockito.argThat(hasFileName("0ABCDEF== (1)")), Mockito.argThat(hasFileName("0ABCDEF==")), Mockito.any()); + Assert.assertEquals("0ABCDEF==", resolved.getFileName().toString()); + } + } diff --git a/src/test/java/org/cryptomator/cryptofs/CryptoDirectoryStreamTest.java b/src/test/java/org/cryptomator/cryptofs/CryptoDirectoryStreamTest.java index be79b5cb..460d07d0 100644 --- a/src/test/java/org/cryptomator/cryptofs/CryptoDirectoryStreamTest.java +++ b/src/test/java/org/cryptomator/cryptofs/CryptoDirectoryStreamTest.java @@ -8,6 +8,7 @@ *******************************************************************************/ package org.cryptomator.cryptofs; +import static org.mockito.AdditionalAnswers.returnsFirstArg; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; @@ -36,8 +37,6 @@ import org.junit.BeforeClass; import org.junit.Test; import org.mockito.Mockito; -import org.mockito.invocation.InvocationOnMock; -import org.mockito.stubbing.Answer; public class CryptoDirectoryStreamTest { @@ -59,6 +58,7 @@ public static void setupClass() { private Path ciphertextDirPath; private DirectoryStream dirStream; private LongFileNameProvider longFileNameProvider; + private ConflictResolver conflictResolver; private FinallyUtil finallyUtil; @Before @@ -74,20 +74,17 @@ public void setup() throws IOException { dirStream = Mockito.mock(DirectoryStream.class); Mockito.when(provider.newDirectoryStream(Mockito.same(ciphertextDirPath), Mockito.any())).thenReturn(dirStream); longFileNameProvider = Mockito.mock(LongFileNameProvider.class); + conflictResolver = Mockito.mock(ConflictResolver.class); finallyUtil = mock(FinallyUtil.class); - Mockito.when(longFileNameProvider.inflate(Mockito.anyString())).thenAnswer(new Answer() { - - @Override - public String answer(InvocationOnMock invocation) throws Throwable { - String shortName = invocation.getArgument(0); - if (shortName.contains("invalid")) { - throw new IOException("invalid shortened name"); - } else { - return StringUtils.removeEnd(shortName, ".lng"); - } + Mockito.when(longFileNameProvider.inflate(Mockito.anyString())).then(invocation -> { + String shortName = invocation.getArgument(0); + if (shortName.contains("invalid")) { + throw new IOException("invalid shortened name"); + } else { + return StringUtils.removeEnd(shortName, ".lng"); } - }); + Mockito.when(conflictResolver.resolveConflicts(Mockito.any(), Mockito.any())).then(returnsFirstArg()); doAnswer(invocation -> { for (Object runnable : invocation.getArguments()) { @@ -111,7 +108,8 @@ public void testDirListing() throws IOException { ciphertextFileNames.add("alsoInvalid"); Mockito.when(dirStream.spliterator()).thenReturn(ciphertextFileNames.stream().map(cleartextPath::resolve).spliterator()); - try (CryptoDirectoryStream stream = new CryptoDirectoryStream(new Directory("foo", ciphertextDirPath), cleartextPath, filenameCryptor, longFileNameProvider, ACCEPT_ALL, DO_NOTHING_ON_CLOSE, finallyUtil)) { + try (CryptoDirectoryStream stream = new CryptoDirectoryStream(new Directory("foo", ciphertextDirPath), cleartextPath, filenameCryptor, longFileNameProvider, conflictResolver, ACCEPT_ALL, DO_NOTHING_ON_CLOSE, + finallyUtil)) { Iterator iter = stream.iterator(); Assert.assertTrue(iter.hasNext()); Assert.assertEquals(cleartextPath.resolve("one"), iter.next()); @@ -133,7 +131,8 @@ public void testDirListingForEmptyDir() throws IOException { Mockito.when(dirStream.spliterator()).thenReturn(Spliterators.emptySpliterator()); - try (CryptoDirectoryStream stream = new CryptoDirectoryStream(new Directory("foo", ciphertextDirPath), cleartextPath, filenameCryptor, longFileNameProvider, ACCEPT_ALL, DO_NOTHING_ON_CLOSE, finallyUtil)) { + try (CryptoDirectoryStream stream = new CryptoDirectoryStream(new Directory("foo", ciphertextDirPath), cleartextPath, filenameCryptor, longFileNameProvider, conflictResolver, ACCEPT_ALL, DO_NOTHING_ON_CLOSE, + finallyUtil)) { Iterator iter = stream.iterator(); Assert.assertFalse(iter.hasNext()); iter.next(); diff --git a/src/test/java/org/cryptomator/cryptofs/DirectoryStreamFactoryTest.java b/src/test/java/org/cryptomator/cryptofs/DirectoryStreamFactoryTest.java index 367bc81b..6c93b3c2 100644 --- a/src/test/java/org/cryptomator/cryptofs/DirectoryStreamFactoryTest.java +++ b/src/test/java/org/cryptomator/cryptofs/DirectoryStreamFactoryTest.java @@ -39,9 +39,10 @@ public class DirectoryStreamFactoryTest { private final FinallyUtil finallyUtil = mock(FinallyUtil.class); private final Cryptor cryptor = mock(Cryptor.class); private final LongFileNameProvider longFileNameProvider = mock(LongFileNameProvider.class); + private final ConflictResolver conflictResolver = mock(ConflictResolver.class); private final CryptoPathMapper cryptoPathMapper = mock(CryptoPathMapper.class); - private final DirectoryStreamFactory inTest = new DirectoryStreamFactory(cryptor, longFileNameProvider, cryptoPathMapper, finallyUtil); + private final DirectoryStreamFactory inTest = new DirectoryStreamFactory(cryptor, longFileNameProvider, conflictResolver, cryptoPathMapper, finallyUtil); @SuppressWarnings("unchecked") From c3021a12deb405061f3d9a6b3604732b40f628b4 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 4 Apr 2017 12:30:33 +0200 Subject: [PATCH 06/10] Refactored code due to high complexity --- .../cryptofs/ConflictResolver.java | 95 +++++++++++++------ .../cryptofs/CryptoDirectoryStream.java | 2 +- .../cryptofs/ConflictResolverTest.java | 13 +-- .../cryptofs/CryptoDirectoryStreamTest.java | 2 +- 4 files changed, 77 insertions(+), 35 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java b/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java index 7dfb2edf..ce65b601 100644 --- a/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java +++ b/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java @@ -41,26 +41,41 @@ public ConflictResolver(LongFileNameProvider longFileNameProvider, Cryptor crypt this.cryptor = cryptor; } - public Path resolveConflicts(Path ciphertextPath, String dirId) throws IOException { + /** + * Checks if the name of the file represented by the given ciphertextPath is a valid ciphertext name without any additional chars. + * If any unexpected chars are found on the name but it still contains an authentic ciphertext, it is considered a conflicting file. + * Conflicting files will be given a new name. The caller must use the path returned by this function after invoking it, as the given ciphertextPath might be no longer valid. + * + * @param ciphertextPath The path to a file to check. + * @param dirId The directory id of the file's parent directory. + * @return Either the original name if no unexpected chars have been found or a completely new path. + * @throws IOException + */ + public Path resolveConflictsIfNecessary(Path ciphertextPath, String dirId) throws IOException { String ciphertextFileName = ciphertextPath.getFileName().toString(); - String resolvedCiphertextFileName = resolveNameConflictIfNecessary(ciphertextPath.getParent(), ciphertextFileName, dirId); - return ciphertextPath.resolveSibling(resolvedCiphertextFileName); - } - - private String resolveNameConflictIfNecessary(Path directory, String ciphertextFileName, String dirId) throws IOException { String basename = StringUtils.removeEnd(ciphertextFileName, LONG_NAME_FILE_EXT); Matcher m = BASE32_PATTERN.matcher(basename); if (!m.matches() && m.find(0)) { // no full match, but still contains base32 -> partial match - return resolveConflict(directory, ciphertextFileName, m.group(1), dirId); + return resolveConflict(ciphertextPath, m.group(1), dirId); } else { // full match or no match at all -> nothing to resolve - return ciphertextFileName; + return ciphertextPath; } } - private String resolveConflict(Path directory, String originalFileName, String base32match, String dirId) throws IOException { - final Path conflictingPath = directory.resolve(originalFileName); + /** + * Resolves a conflict. + * + * @param conflictingPath The path of a file containing a valid base 32 part. + * @param base32match The base32 part inside the filename of the conflicting file. + * @param dirId The directory id of the file's parent directory. + * @return The new path of the conflicting file after the conflict has been resolved. + * @throws IOException + */ + private Path resolveConflict(Path conflictingPath, String base32match, String dirId) throws IOException { + final Path directory = conflictingPath.getParent(); + final String originalFileName = conflictingPath.getFileName().toString(); final String ciphertext; final boolean isDirectory; final String dirPrefix; @@ -81,20 +96,25 @@ private String resolveConflict(Path directory, String originalFileName, String b /* * Directory files must not result in symlink, make sure the conflicting file is not identical to the original. */ - if (isDirectory && !Files.exists(canonicalPath)) { - // original file is, so this is no real conflict. - Files.move(conflictingPath, canonicalPath, StandardCopyOption.ATOMIC_MOVE); - return canonicalPath.getFileName().toString(); - } else if (isDirectory && hasSameDirFileContent(conflictingPath, canonicalPath)) { - // there must not be two directories pointing to the same dirId. In this case no human interaction is needed to resolve this conflict: - LOG.info("Removing conflicting directory file {} (identical to {})", conflictingPath, canonicalPath); - Files.deleteIfExists(conflictingPath); - return canonicalPath.getFileName().toString(); + if (isDirectory && resolveDirectoryConflictTrivially(canonicalPath, conflictingPath)) { + return canonicalPath; + } else { + return renameConflictingFile(canonicalPath, conflictingPath, ciphertext, dirId, dirPrefix); } + } - /* - * Find alternative name for conflicting file: - */ + /** + * Resolves a conflict by renaming the conflicting file. + * + * @param canonicalPath The path to the original (conflict-free) file. + * @param conflictingPath The path to the potentially conflicting file. + * @param ciphertext The (previously inflated) ciphertext name of the file without any preceeding directory prefix. + * @param dirId The directory id of the file's parent directory. + * @param dirPrefix The directory prefix (if the conflicting file is a directory file) or an empty string. + * @return The new path after renaming the conflicting file. + * @throws IOException + */ + private Path renameConflictingFile(Path canonicalPath, Path conflictingPath, String ciphertext, String dirId, String dirPrefix) throws IOException { try { String cleartext = cryptor.fileNameCryptor().decryptFilename(ciphertext, dirId.getBytes(StandardCharsets.UTF_8)); Path alternativePath = canonicalPath; @@ -105,15 +125,36 @@ private String resolveConflict(Path directory, String originalFileName, String b if (alternativeCiphertextFileName.length() >= NAME_SHORTENING_THRESHOLD) { alternativeCiphertextFileName = longFileNameProvider.deflate(alternativeCiphertextFileName); } - alternativePath = directory.resolve(alternativeCiphertextFileName); + alternativePath = canonicalPath.resolveSibling(alternativeCiphertextFileName); } LOG.info("Moving conflicting file {} to {}", conflictingPath, alternativePath); - Files.move(conflictingPath, alternativePath, StandardCopyOption.ATOMIC_MOVE); - return alternativePath.getFileName().toString(); + return Files.move(conflictingPath, alternativePath, StandardCopyOption.ATOMIC_MOVE); } catch (AuthenticationFailedException e) { // not decryptable, no need to resolve any kind of conflict - LOG.info("Found valid Base32 string, which is an invalid ciphertext: {}", originalFileName); - return originalFileName; + LOG.info("Found valid Base32 string, which is an unauthentic ciphertext: {}", conflictingPath); + return conflictingPath; + } + } + + /** + * Tries to resolve a conflicting directory file without renaming the file. If successful, only the file with the canonical path will exist afterwards. + * + * @param canonicalPath The path to the original (conflict-free) directory file (must not exist). + * @param conflictingPath The path to the potentially conflicting file (known to exist). + * @return true if the conflict has been resolved. + * @throws IOException + */ + private boolean resolveDirectoryConflictTrivially(Path canonicalPath, Path conflictingPath) throws IOException { + if (!Files.exists(canonicalPath)) { + Files.move(conflictingPath, canonicalPath, StandardCopyOption.ATOMIC_MOVE); + return true; + } else if (hasSameDirFileContent(conflictingPath, canonicalPath)) { + // there must not be two directories pointing to the same dirId. + LOG.info("Removing conflicting directory file {} (identical to {})", conflictingPath, canonicalPath); + Files.deleteIfExists(conflictingPath); + return true; + } else { + return false; } } diff --git a/src/main/java/org/cryptomator/cryptofs/CryptoDirectoryStream.java b/src/main/java/org/cryptomator/cryptofs/CryptoDirectoryStream.java index cd50ee1f..4492cff2 100644 --- a/src/main/java/org/cryptomator/cryptofs/CryptoDirectoryStream.java +++ b/src/main/java/org/cryptomator/cryptofs/CryptoDirectoryStream.java @@ -69,7 +69,7 @@ public Iterator iterator() { private Path resolveConflictingFileIfNeeded(Path potentiallyConflictingPath) { try { - return conflictResolver.resolveConflicts(potentiallyConflictingPath, directoryId); + return conflictResolver.resolveConflictsIfNecessary(potentiallyConflictingPath, directoryId); } catch (IOException e) { LOG.warn("I/O exception while finding potentially conflicting file versions for {}.", potentiallyConflictingPath); return null; diff --git a/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java b/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java index 4d964498..4b9c2c9a 100644 --- a/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java +++ b/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java @@ -63,6 +63,7 @@ private Path resolveChildOfTestDir(InvocationOnMock invocation) { Mockito.when(resultName.toString()).thenReturn(invocation.getArgument(0)); Mockito.when(result.getParent()).thenReturn(testDir); Mockito.when(result.getFileSystem()).thenReturn(testFileSystem); + Mockito.when(result.resolveSibling(Mockito.anyString())).then(this::resolveChildOfTestDir); return result; } @@ -88,7 +89,7 @@ private Answer fillBufferWithBytes(byte[] bytes) { @Test public void testPassthroughValidBase32NormalFile() throws IOException { Mockito.when(testFileName.toString()).thenReturn("ABCDEF=="); - Path resolved = conflictResolver.resolveConflicts(testFile, dirId); + Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); Mockito.verifyNoMoreInteractions(filenameCryptor); Mockito.verifyNoMoreInteractions(longFileNameProvider); Assert.assertEquals(testFile.getFileName().toString(), resolved.getFileName().toString()); @@ -97,7 +98,7 @@ public void testPassthroughValidBase32NormalFile() throws IOException { @Test public void testPassthroughValidBase32LongFile() throws IOException { Mockito.when(testFileName.toString()).thenReturn("ABCDEF==.lng"); - Path resolved = conflictResolver.resolveConflicts(testFile, dirId); + Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); Mockito.verifyNoMoreInteractions(filenameCryptor); Mockito.verifyNoMoreInteractions(longFileNameProvider); Assert.assertEquals(testFile.getFileName().toString(), resolved.getFileName().toString()); @@ -110,7 +111,7 @@ public void testAlternativeNameForNormalFile() throws IOException { Mockito.when(filenameCryptor.decryptFilename(Mockito.eq("ABCDEF=="), Mockito.any())).thenReturn("abcdef"); Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("abcdef (Conflict "), Mockito.any())).thenReturn(ciphertextName); Mockito.doThrow(new NoSuchFileException(ciphertextName)).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName(ciphertextName))); - Path resolved = conflictResolver.resolveConflicts(testFile, dirId); + Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); Mockito.verifyNoMoreInteractions(longFileNameProvider); Mockito.verify(testFileSystemProvider).move(Mockito.argThat(hasFileName("ABCDEF== (1)")), Mockito.argThat(hasFileName(ciphertextName)), Mockito.any()); Assert.assertThat(resolved.getFileName().toString(), CoreMatchers.containsString(ciphertextName)); @@ -126,7 +127,7 @@ public void testAlternativeNameForLongFile() throws IOException { Mockito.when(filenameCryptor.decryptFilename(Mockito.eq("FEDCBA=="), Mockito.any())).thenReturn("fedcba"); Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("fedcba (Conflict "), Mockito.any())).thenReturn(longCiphertextName); Mockito.doThrow(new NoSuchFileException("FEDCBA==.lng")).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName("FEDCBA==.lng"))); - Path resolved = conflictResolver.resolveConflicts(testFile, dirId); + Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); Mockito.verify(longFileNameProvider).deflate(longCiphertextName); Mockito.verify(testFileSystemProvider).move(Mockito.argThat(hasFileName("ABCDEF== (1).lng")), Mockito.argThat(hasFileName("FEDCBA==.lng")), Mockito.any()); Assert.assertThat(resolved.getFileName().toString(), CoreMatchers.containsString("FEDCBA==.lng")); @@ -145,7 +146,7 @@ public void testAlternativeNameForDirectoryFile() throws IOException, Reflective Mockito.when(testFileSystemProvider.newByteChannel(Mockito.argThat(hasFileName("0ABCDEF== (1)")), Mockito.any(), Mockito.any())).thenReturn(conflictingFc); Mockito.when(canonicalFc.read(Mockito.any(ByteBuffer.class))).then(fillBufferWithBytes("12345".getBytes())); Mockito.when(conflictingFc.read(Mockito.any(ByteBuffer.class))).then(fillBufferWithBytes("12345".getBytes())); - Path resolved = conflictResolver.resolveConflicts(testFile, dirId); + Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); Mockito.verify(testFileSystemProvider).deleteIfExists(Mockito.argThat(hasFileName("0ABCDEF== (1)"))); Assert.assertEquals("0ABCDEF==", resolved.getFileName().toString()); } @@ -154,7 +155,7 @@ public void testAlternativeNameForDirectoryFile() throws IOException, Reflective public void testSilentRenamingForConflictingDirectoryFileWithMissingCanonicalFile() throws IOException, ReflectiveOperationException { Mockito.when(testFileName.toString()).thenReturn("0ABCDEF== (1)"); Mockito.doThrow(new NoSuchFileException("0ABCDEF==")).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName("0ABCDEF=="))); - Path resolved = conflictResolver.resolveConflicts(testFile, dirId); + Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); Mockito.verify(testFileSystemProvider).move(Mockito.argThat(hasFileName("0ABCDEF== (1)")), Mockito.argThat(hasFileName("0ABCDEF==")), Mockito.any()); Assert.assertEquals("0ABCDEF==", resolved.getFileName().toString()); } diff --git a/src/test/java/org/cryptomator/cryptofs/CryptoDirectoryStreamTest.java b/src/test/java/org/cryptomator/cryptofs/CryptoDirectoryStreamTest.java index 460d07d0..60009ba1 100644 --- a/src/test/java/org/cryptomator/cryptofs/CryptoDirectoryStreamTest.java +++ b/src/test/java/org/cryptomator/cryptofs/CryptoDirectoryStreamTest.java @@ -84,7 +84,7 @@ public void setup() throws IOException { return StringUtils.removeEnd(shortName, ".lng"); } }); - Mockito.when(conflictResolver.resolveConflicts(Mockito.any(), Mockito.any())).then(returnsFirstArg()); + Mockito.when(conflictResolver.resolveConflictsIfNecessary(Mockito.any(), Mockito.any())).then(returnsFirstArg()); doAnswer(invocation -> { for (Object runnable : invocation.getArguments()) { From ae74e30d77231ba13d3b29ab43f3b28394a2d7a3 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 4 Apr 2017 14:02:30 +0200 Subject: [PATCH 07/10] Improved tests --- .../cryptofs/ConflictResolver.java | 3 -- .../cryptofs/ConflictResolverTest.java | 46 ++++++++++++++++--- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java b/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java index ce65b601..1698e8fa 100644 --- a/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java +++ b/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java @@ -93,9 +93,6 @@ private Path resolveConflict(Path conflictingPath, String base32match, String di canonicalPath = directory.resolve(dirPrefix + ciphertext); } - /* - * Directory files must not result in symlink, make sure the conflicting file is not identical to the original. - */ if (isDirectory && resolveDirectoryConflictTrivially(canonicalPath, conflictingPath)) { return canonicalPath; } else { diff --git a/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java b/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java index 4b9c2c9a..54e1f769 100644 --- a/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java +++ b/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java @@ -10,9 +10,9 @@ import java.nio.file.Path; import java.nio.file.spi.FileSystemProvider; +import org.cryptomator.cryptolib.api.AuthenticationFailedException; import org.cryptomator.cryptolib.api.Cryptor; import org.cryptomator.cryptolib.api.FileNameCryptor; -import org.hamcrest.CoreMatchers; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -95,6 +95,14 @@ public void testPassthroughValidBase32NormalFile() throws IOException { Assert.assertEquals(testFile.getFileName().toString(), resolved.getFileName().toString()); } + @Test + public void testPassthroughInvalidBase32NormalFile() throws IOException { + Mockito.when(testFileName.toString()).thenReturn("ABCDEF== (1)"); + Mockito.when(filenameCryptor.decryptFilename(Mockito.eq("ABCDEF=="), Mockito.any())).thenThrow(new AuthenticationFailedException("invalid ciphertext")); + Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); + Assert.assertSame(testFile, resolved); + } + @Test public void testPassthroughValidBase32LongFile() throws IOException { Mockito.when(testFileName.toString()).thenReturn("ABCDEF==.lng"); @@ -105,7 +113,7 @@ public void testPassthroughValidBase32LongFile() throws IOException { } @Test - public void testAlternativeNameForNormalFile() throws IOException { + public void testRenameNormalFile() throws IOException { String ciphertextName = "ABCDEFGH2345===="; Mockito.when(testFileName.toString()).thenReturn("ABCDEF== (1)"); Mockito.when(filenameCryptor.decryptFilename(Mockito.eq("ABCDEF=="), Mockito.any())).thenReturn("abcdef"); @@ -114,11 +122,11 @@ public void testAlternativeNameForNormalFile() throws IOException { Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); Mockito.verifyNoMoreInteractions(longFileNameProvider); Mockito.verify(testFileSystemProvider).move(Mockito.argThat(hasFileName("ABCDEF== (1)")), Mockito.argThat(hasFileName(ciphertextName)), Mockito.any()); - Assert.assertThat(resolved.getFileName().toString(), CoreMatchers.containsString(ciphertextName)); + Assert.assertEquals(ciphertextName, resolved.getFileName().toString()); } @Test - public void testAlternativeNameForLongFile() throws IOException { + public void testRenameLongFile() throws IOException { String longCiphertextName = "ABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGHABCDEFGH2345===="; assert longCiphertextName.length() > Constants.NAME_SHORTENING_THRESHOLD; Mockito.when(testFileName.toString()).thenReturn("ABCDEF== (1).lng"); @@ -130,11 +138,11 @@ public void testAlternativeNameForLongFile() throws IOException { Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); Mockito.verify(longFileNameProvider).deflate(longCiphertextName); Mockito.verify(testFileSystemProvider).move(Mockito.argThat(hasFileName("ABCDEF== (1).lng")), Mockito.argThat(hasFileName("FEDCBA==.lng")), Mockito.any()); - Assert.assertThat(resolved.getFileName().toString(), CoreMatchers.containsString("FEDCBA==.lng")); + Assert.assertEquals("FEDCBA==.lng", resolved.getFileName().toString()); } @Test - public void testAlternativeNameForDirectoryFile() throws IOException, ReflectiveOperationException { + public void testSilentlyDeleteConflictingDirectoryFileIdenticalToCanonicalFile() throws IOException, ReflectiveOperationException { Mockito.when(testFileName.toString()).thenReturn("0ABCDEF== (1)"); FileChannel canonicalFc = Mockito.mock(FileChannel.class); FileChannel conflictingFc = Mockito.mock(FileChannel.class); @@ -152,7 +160,7 @@ public void testAlternativeNameForDirectoryFile() throws IOException, Reflective } @Test - public void testSilentRenamingForConflictingDirectoryFileWithMissingCanonicalFile() throws IOException, ReflectiveOperationException { + public void testSilentlyRenameConflictingDirectoryFileWithMissingCanonicalFile() throws IOException, ReflectiveOperationException { Mockito.when(testFileName.toString()).thenReturn("0ABCDEF== (1)"); Mockito.doThrow(new NoSuchFileException("0ABCDEF==")).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName("0ABCDEF=="))); Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); @@ -160,4 +168,28 @@ public void testSilentRenamingForConflictingDirectoryFileWithMissingCanonicalFil Assert.assertEquals("0ABCDEF==", resolved.getFileName().toString()); } + @Test + public void testRenameDirectoryFile() throws IOException, ReflectiveOperationException { + Mockito.when(testFileName.toString()).thenReturn("0ABCDEF== (1)"); + FileChannel canonicalFc = Mockito.mock(FileChannel.class); + FileChannel conflictingFc = Mockito.mock(FileChannel.class); + Field channelCloseLockField = AbstractInterruptibleChannel.class.getDeclaredField("closeLock"); + channelCloseLockField.setAccessible(true); + channelCloseLockField.set(canonicalFc, new Object()); + channelCloseLockField.set(conflictingFc, new Object()); + Mockito.when(testFileSystemProvider.newByteChannel(Mockito.argThat(hasFileName("0ABCDEF==")), Mockito.any(), Mockito.any())).thenReturn(canonicalFc); + Mockito.when(testFileSystemProvider.newByteChannel(Mockito.argThat(hasFileName("0ABCDEF== (1)")), Mockito.any(), Mockito.any())).thenReturn(conflictingFc); + Mockito.when(canonicalFc.read(Mockito.any(ByteBuffer.class))).then(fillBufferWithBytes("12345".getBytes())); + Mockito.when(conflictingFc.read(Mockito.any(ByteBuffer.class))).then(fillBufferWithBytes("67890".getBytes())); + String ciphertext = "ABCDEFGH2345===="; + String ciphertextName = "0" + ciphertext; + Mockito.when(filenameCryptor.decryptFilename(Mockito.eq("ABCDEF=="), Mockito.any())).thenReturn("abcdef"); + Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("abcdef (Conflict "), Mockito.any())).thenReturn(ciphertext); + Mockito.doThrow(new NoSuchFileException(ciphertextName)).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName(ciphertextName))); + Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); + Mockito.verifyNoMoreInteractions(longFileNameProvider); + Mockito.verify(testFileSystemProvider).move(Mockito.argThat(hasFileName("0ABCDEF== (1)")), Mockito.argThat(hasFileName(ciphertextName)), Mockito.any()); + Assert.assertEquals(ciphertextName, resolved.getFileName().toString()); + } + } From 8e0591f993d452881200172693814515b0a54359 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 4 Apr 2017 17:15:35 +0200 Subject: [PATCH 08/10] Simplified conflict file name generation --- .../org/cryptomator/cryptofs/ConflictResolver.java | 10 ++-------- .../org/cryptomator/cryptofs/ConflictResolverTest.java | 6 +++--- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java b/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java index 1698e8fa..6ca2cd9a 100644 --- a/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java +++ b/src/main/java/org/cryptomator/cryptofs/ConflictResolver.java @@ -12,7 +12,6 @@ import java.nio.file.Path; import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; -import java.util.UUID; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -30,7 +29,6 @@ class ConflictResolver { private static final Logger LOG = LoggerFactory.getLogger(ConflictResolver.class); private static final Pattern BASE32_PATTERN = Pattern.compile("0?(([A-Z2-7]{8})*[A-Z2-7=]{8})"); private static final int MAX_DIR_FILE_SIZE = 87; // "normal" file header has 88 bytes - private static final int UUID_FIRST_GROUP_STRLEN = 8; private final LongFileNameProvider longFileNameProvider; private final Cryptor cryptor; @@ -115,8 +113,8 @@ private Path renameConflictingFile(Path canonicalPath, Path conflictingPath, Str try { String cleartext = cryptor.fileNameCryptor().decryptFilename(ciphertext, dirId.getBytes(StandardCharsets.UTF_8)); Path alternativePath = canonicalPath; - while (Files.exists(alternativePath)) { - String alternativeCleartext = cleartext + " (Conflict " + createConflictId() + ")"; + for (int i = 1; Files.exists(alternativePath); i++) { + String alternativeCleartext = cleartext + " (Conflict " + i + ")"; String alternativeCiphertext = cryptor.fileNameCryptor().encryptFilename(alternativeCleartext, dirId.getBytes(StandardCharsets.UTF_8)); String alternativeCiphertextFileName = dirPrefix + alternativeCiphertext; if (alternativeCiphertextFileName.length() >= NAME_SHORTENING_THRESHOLD) { @@ -174,8 +172,4 @@ private boolean hasSameDirFileContent(Path conflictingPath, Path canonicalPath) } } - private static String createConflictId() { - return UUID.randomUUID().toString().substring(0, UUID_FIRST_GROUP_STRLEN); - } - } diff --git a/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java b/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java index 54e1f769..da75c395 100644 --- a/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java +++ b/src/test/java/org/cryptomator/cryptofs/ConflictResolverTest.java @@ -117,7 +117,7 @@ public void testRenameNormalFile() throws IOException { String ciphertextName = "ABCDEFGH2345===="; Mockito.when(testFileName.toString()).thenReturn("ABCDEF== (1)"); Mockito.when(filenameCryptor.decryptFilename(Mockito.eq("ABCDEF=="), Mockito.any())).thenReturn("abcdef"); - Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("abcdef (Conflict "), Mockito.any())).thenReturn(ciphertextName); + Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("abcdef ("), Mockito.any())).thenReturn(ciphertextName); Mockito.doThrow(new NoSuchFileException(ciphertextName)).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName(ciphertextName))); Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); Mockito.verifyNoMoreInteractions(longFileNameProvider); @@ -133,7 +133,7 @@ public void testRenameLongFile() throws IOException { Mockito.when(longFileNameProvider.inflate("ABCDEF==.lng")).thenReturn("FEDCBA=="); Mockito.when(longFileNameProvider.deflate(longCiphertextName)).thenReturn("FEDCBA==.lng"); Mockito.when(filenameCryptor.decryptFilename(Mockito.eq("FEDCBA=="), Mockito.any())).thenReturn("fedcba"); - Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("fedcba (Conflict "), Mockito.any())).thenReturn(longCiphertextName); + Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("fedcba ("), Mockito.any())).thenReturn(longCiphertextName); Mockito.doThrow(new NoSuchFileException("FEDCBA==.lng")).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName("FEDCBA==.lng"))); Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); Mockito.verify(longFileNameProvider).deflate(longCiphertextName); @@ -184,7 +184,7 @@ public void testRenameDirectoryFile() throws IOException, ReflectiveOperationExc String ciphertext = "ABCDEFGH2345===="; String ciphertextName = "0" + ciphertext; Mockito.when(filenameCryptor.decryptFilename(Mockito.eq("ABCDEF=="), Mockito.any())).thenReturn("abcdef"); - Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("abcdef (Conflict "), Mockito.any())).thenReturn(ciphertext); + Mockito.when(filenameCryptor.encryptFilename(Mockito.startsWith("abcdef ("), Mockito.any())).thenReturn(ciphertext); Mockito.doThrow(new NoSuchFileException(ciphertextName)).when(testFileSystemProvider).checkAccess(Mockito.argThat(hasFileName(ciphertextName))); Path resolved = conflictResolver.resolveConflictsIfNecessary(testFile, dirId); Mockito.verifyNoMoreInteractions(longFileNameProvider); From 0724b0d285a926f3c7d5880c0b8b561f896caef0 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 4 Apr 2017 17:17:26 +0200 Subject: [PATCH 09/10] Preparing release of 1.2.0 --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 99e0bd2c..9ca9a50e 100644 --- a/pom.xml +++ b/pom.xml @@ -2,7 +2,7 @@ 4.0.0 org.cryptomator cryptofs - 1.2.0-SNAPSHOT + 1.2.0 Cryptomator Crypto Filesystem This library provides the Java filesystem provider used by Cryptomator. https://github.com/cryptomator/cryptofs From d86da82026ba23dd0d558c1857e7b6e85686e3bc Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 4 Apr 2017 17:28:48 +0200 Subject: [PATCH 10/10] Updated dependencies --- pom.xml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pom.xml b/pom.xml index 9ca9a50e..1c6c4b63 100644 --- a/pom.xml +++ b/pom.xml @@ -16,10 +16,10 @@ 1.8 1.1.1 - 2.9 + 2.10 21.0 3.5 - 1.7.24 + 1.7.25 UTF-8 @@ -114,7 +114,7 @@ org.mockito mockito-core - 2.7.12 + 2.7.21 test @@ -218,7 +218,7 @@ org.codehaus.mojo exec-maven-plugin - 1.5.0 + 1.6.0 verify