From a00086ff2d6e22ae297388b9755eec88129ff614 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Sat, 4 Jul 2015 20:47:23 +0200 Subject: [PATCH] - simplified range request handling - correct handling of HTTP 416 responses - moved unit test to apache httpclient (old version 3.1 due to jackrabbit's dependency) --- main/core/pom.xml | 5 + .../jackrabbit/CryptoResourceFactory.java | 73 ++++++++++++++- .../webdav/jackrabbit/EncryptedFile.java | 4 + .../webdav/jackrabbit/EncryptedFilePart.java | 91 ++++--------------- .../webdav/jackrabbit/RangeRequestTest.java | 91 ++++++++++++------- 5 files changed, 153 insertions(+), 111 deletions(-) diff --git a/main/core/pom.xml b/main/core/pom.xml index 72ecc5ea2..0e85e556b 100644 --- a/main/core/pom.xml +++ b/main/core/pom.xml @@ -46,6 +46,11 @@ jetty-webapp ${jetty.version} + + commons-httpclient + commons-httpclient + test + diff --git a/main/core/src/main/java/org/cryptomator/webdav/jackrabbit/CryptoResourceFactory.java b/main/core/src/main/java/org/cryptomator/webdav/jackrabbit/CryptoResourceFactory.java index 64fcf009f..a048c1000 100644 --- a/main/core/src/main/java/org/cryptomator/webdav/jackrabbit/CryptoResourceFactory.java +++ b/main/core/src/main/java/org/cryptomator/webdav/jackrabbit/CryptoResourceFactory.java @@ -8,6 +8,9 @@ import java.nio.file.Path; import org.apache.commons.httpclient.HttpStatus; import org.apache.commons.io.FilenameUtils; +import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.tuple.ImmutablePair; +import org.apache.commons.lang3.tuple.Pair; import org.apache.jackrabbit.webdav.DavException; import org.apache.jackrabbit.webdav.DavMethods; import org.apache.jackrabbit.webdav.DavResource; @@ -24,6 +27,10 @@ import org.eclipse.jetty.http.HttpHeader; public class CryptoResourceFactory implements DavResourceFactory, FileConstants { + private static final String RANGE_BYTE_PREFIX = "bytes="; + private static final char RANGE_SET_SEP = ','; + private static final char RANGE_SEP = '-'; + private final LockManager lockManager = new SimpleLockManager(); private final Cryptor cryptor; private final CryptoWarningHandler cryptoWarningHandler; @@ -48,14 +55,24 @@ public class CryptoResourceFactory implements DavResourceFactory, FileConstants final Path dirFilePath = getEncryptedDirectoryFilePath(locator.getResourcePath()); final String rangeHeader = request.getHeader(HttpHeader.RANGE.asString()); if (Files.exists(dirFilePath) || DavMethods.METHOD_MKCOL.equals(request.getMethod())) { + // DIRECTORY return createDirectory(locator, request.getDavSession(), dirFilePath); - } else if (Files.exists(filePath) && DavMethods.METHOD_GET.equals(request.getMethod()) && rangeHeader != null) { + } else if (Files.exists(filePath) && DavMethods.METHOD_GET.equals(request.getMethod()) && rangeHeader != null && isRangeSatisfiable(rangeHeader)) { + // FILE RANGE + final Pair requestRange = getRequestRange(rangeHeader); response.setStatus(HttpStatus.SC_PARTIAL_CONTENT); - return createFilePart(locator, request.getDavSession(), request, filePath); + return createFilePart(locator, request.getDavSession(), requestRange, filePath); + } else if (Files.exists(filePath) && DavMethods.METHOD_GET.equals(request.getMethod()) && rangeHeader != null && !isRangeSatisfiable(rangeHeader)) { + // FULL FILE (unsatisfiable range) + response.setStatus(HttpStatus.SC_REQUESTED_RANGE_NOT_SATISFIABLE); + final EncryptedFile file = createFile(locator, request.getDavSession(), filePath); + response.addHeader(HttpHeader.CONTENT_RANGE.asString(), "bytes */" + file.getContentLength()); + return file; } else if (Files.exists(filePath) || DavMethods.METHOD_PUT.equals(request.getMethod())) { + // FULL FILE (as requested) return createFile(locator, request.getDavSession(), filePath); } else { - // e.g. for MOVE operations: + // NO FILE OR FOLDER (e.g. for MOVE operations): return createNonExisting(locator, request.getDavSession(), filePath, dirFilePath); } } @@ -86,6 +103,52 @@ public class CryptoResourceFactory implements DavResourceFactory, FileConstants return createFile(locator, session, existingFile); } + /** + * @return true if and only if exactly one byte range has been requested. + */ + private boolean isRangeSatisfiable(String rangeHeader) { + assert rangeHeader != null; + if (!rangeHeader.startsWith(RANGE_BYTE_PREFIX)) { + return false; + } + final String byteRangeSet = StringUtils.removeStartIgnoreCase(rangeHeader, RANGE_BYTE_PREFIX); + final String[] byteRanges = StringUtils.split(byteRangeSet, RANGE_SET_SEP); + if (byteRanges.length != 1) { + return false; + } + return true; + } + + /** + * Processes the given range header field, if it is supported. Only headers containing a single byte range are supported.
+ * + * bytes=100-200
+ * bytes=-500
+ * bytes=1000- + * + * + * @return Tuple of left and right range. + * @throws DavException HTTP statuscode 400 for malformed requests. + * @throws IllegalArgumentException If the given rangeHeader is not satisfiable. Check with {@link #isRangeSatisfiable(String)} before. + */ + private Pair getRequestRange(String rangeHeader) throws DavException { + assert rangeHeader != null; + if (!rangeHeader.startsWith(RANGE_BYTE_PREFIX)) { + throw new IllegalArgumentException("Unsatisfiable range. Should have generated 416 resonse."); + } + final String byteRangeSet = StringUtils.removeStartIgnoreCase(rangeHeader, RANGE_BYTE_PREFIX); + final String[] byteRanges = StringUtils.split(byteRangeSet, RANGE_SET_SEP); + if (byteRanges.length != 1) { + throw new IllegalArgumentException("Unsatisfiable range. Should have generated 416 resonse."); + } + final String byteRange = byteRanges[0]; + final String[] bytePos = StringUtils.splitPreserveAllTokens(byteRange, RANGE_SEP); + if (bytePos.length != 2 || bytePos[0].isEmpty() && bytePos[1].isEmpty()) { + throw new DavException(DavServletResponse.SC_BAD_REQUEST, "malformed range header: " + rangeHeader); + } + return new ImmutablePair<>(bytePos[0], bytePos[1]); + } + /** * @return Absolute file path for a given cleartext file resourcePath. * @throws IOException @@ -147,8 +210,8 @@ public class CryptoResourceFactory implements DavResourceFactory, FileConstants } } - private EncryptedFile createFilePart(DavResourceLocator locator, DavSession session, DavServletRequest request, Path filePath) { - return new EncryptedFilePart(this, locator, session, request, lockManager, cryptor, cryptoWarningHandler, filePath); + private EncryptedFile createFilePart(DavResourceLocator locator, DavSession session, Pair requestRange, Path filePath) { + return new EncryptedFilePart(this, locator, session, requestRange, lockManager, cryptor, cryptoWarningHandler, filePath); } private EncryptedFile createFile(DavResourceLocator locator, DavSession session, Path filePath) { diff --git a/main/core/src/main/java/org/cryptomator/webdav/jackrabbit/EncryptedFile.java b/main/core/src/main/java/org/cryptomator/webdav/jackrabbit/EncryptedFile.java index d0057c598..9be4ddf39 100644 --- a/main/core/src/main/java/org/cryptomator/webdav/jackrabbit/EncryptedFile.java +++ b/main/core/src/main/java/org/cryptomator/webdav/jackrabbit/EncryptedFile.java @@ -72,6 +72,10 @@ class EncryptedFile extends AbstractEncryptedNode implements FileConstants { this.contentLength = contentLength; } + public Long getContentLength() { + return contentLength; + } + @Override public boolean isCollection() { return false; diff --git a/main/core/src/main/java/org/cryptomator/webdav/jackrabbit/EncryptedFilePart.java b/main/core/src/main/java/org/cryptomator/webdav/jackrabbit/EncryptedFilePart.java index fe3963b7b..a506becd1 100644 --- a/main/core/src/main/java/org/cryptomator/webdav/jackrabbit/EncryptedFilePart.java +++ b/main/core/src/main/java/org/cryptomator/webdav/jackrabbit/EncryptedFilePart.java @@ -6,15 +6,10 @@ import java.nio.channels.FileChannel; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardOpenOption; -import java.util.HashSet; -import java.util.Set; -import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.tuple.ImmutablePair; -import org.apache.commons.lang3.tuple.MutablePair; import org.apache.commons.lang3.tuple.Pair; import org.apache.jackrabbit.webdav.DavResourceLocator; -import org.apache.jackrabbit.webdav.DavServletRequest; import org.apache.jackrabbit.webdav.DavSession; import org.apache.jackrabbit.webdav.io.OutputContext; import org.apache.jackrabbit.webdav.lock.LockManager; @@ -33,79 +28,26 @@ import org.slf4j.LoggerFactory; class EncryptedFilePart extends EncryptedFile { private static final Logger LOG = LoggerFactory.getLogger(EncryptedFilePart.class); - private static final String BYTE_UNIT_PREFIX = "bytes="; - private static final char RANGE_SET_SEP = ','; - private static final char RANGE_SEP = '-'; - /** - * e.g. range -500 (gets the last 500 bytes) -> (-1, 500) - */ - private static final Long SUFFIX_BYTE_RANGE_LOWER = -1L; + private final Pair range; - /** - * e.g. range 500- (gets all bytes from 500) -> (500, MAX_LONG) - */ - private static final Long SUFFIX_BYTE_RANGE_UPPER = Long.MAX_VALUE; - - private final Set> requestedContentRanges = new HashSet>(); - - public EncryptedFilePart(CryptoResourceFactory factory, DavResourceLocator locator, DavSession session, DavServletRequest request, LockManager lockManager, Cryptor cryptor, CryptoWarningHandler cryptoWarningHandler, - Path filePath) { + public EncryptedFilePart(CryptoResourceFactory factory, DavResourceLocator locator, DavSession session, Pair requestRange, LockManager lockManager, Cryptor cryptor, + CryptoWarningHandler cryptoWarningHandler, Path filePath) { super(factory, locator, session, lockManager, cryptor, cryptoWarningHandler, filePath); - final String rangeHeader = request.getHeader(HttpHeader.RANGE.asString()); - if (rangeHeader == null) { - throw new IllegalArgumentException("HTTP request doesn't contain a range header"); - } - determineByteRanges(rangeHeader); - } - private void determineByteRanges(String rangeHeader) { - final String byteRangeSet = StringUtils.removeStartIgnoreCase(rangeHeader, BYTE_UNIT_PREFIX); - final String[] byteRanges = StringUtils.split(byteRangeSet, RANGE_SET_SEP); - if (byteRanges.length == 0) { - throw new IllegalArgumentException("Invalid range: " + rangeHeader); - } - for (final String byteRange : byteRanges) { - final String[] bytePos = StringUtils.splitPreserveAllTokens(byteRange, RANGE_SEP); - if (bytePos.length != 2 || bytePos[0].isEmpty() && bytePos[1].isEmpty()) { - throw new IllegalArgumentException("Invalid range: " + rangeHeader); - } - final Long lower = bytePos[0].isEmpty() ? SUFFIX_BYTE_RANGE_LOWER : Long.valueOf(bytePos[0]); - final Long upper = bytePos[1].isEmpty() ? SUFFIX_BYTE_RANGE_UPPER : Long.valueOf(bytePos[1]); - if (lower > upper) { - throw new IllegalArgumentException("Invalid range: " + rangeHeader); - } - requestedContentRanges.add(new ImmutablePair(lower, upper)); - } - } - - /** - * @return One range, that spans all requested ranges. - */ - private Pair getUnionRange(Long fileSize) { - final long lastByte = fileSize - 1; - final MutablePair result = new MutablePair(); - for (Pair range : requestedContentRanges) { - final long left; - final long right; - if (SUFFIX_BYTE_RANGE_LOWER.equals(range.getLeft())) { - left = lastByte - range.getRight(); - right = lastByte; - } else if (SUFFIX_BYTE_RANGE_UPPER.equals(range.getRight())) { - left = range.getLeft(); - right = lastByte; + try { + final Long lower = requestRange.getLeft().isEmpty() ? null : Long.valueOf(requestRange.getLeft()); + final Long upper = requestRange.getRight().isEmpty() ? null : Long.valueOf(requestRange.getRight()); + if (lower == null) { + range = new ImmutablePair(contentLength - upper, contentLength - 1); + } else if (upper == null) { + range = new ImmutablePair(lower, contentLength - 1); } else { - left = range.getLeft(); - right = range.getRight(); - } - if (result.getLeft() == null || left < result.getLeft()) { - result.setLeft(left); - } - if (result.getRight() == null || right > result.getRight()) { - result.setRight(right); + range = new ImmutablePair(lower, upper); } + } catch (NumberFormatException e) { + throw new IllegalArgumentException("Invalid byte range: " + requestRange, e); } - return result; } @Override @@ -113,10 +55,9 @@ class EncryptedFilePart extends EncryptedFile { assert Files.isRegularFile(filePath); assert this.contentLength != null; - final Pair range = getUnionRange(this.contentLength); - final Long rangeLength = Math.min(this.contentLength, range.getRight()) - range.getLeft() + 1; + final Long rangeLength = range.getRight() - range.getLeft() + 1; outputContext.setContentLength(rangeLength); - outputContext.setProperty(HttpHeader.CONTENT_RANGE.asString(), getContentRangeHeader(range.getLeft(), range.getRight(), this.contentLength)); + outputContext.setProperty(HttpHeader.CONTENT_RANGE.asString(), getContentRangeHeader(range.getLeft(), range.getRight(), contentLength)); outputContext.setModificationTime(Files.getLastModifiedTime(filePath).toMillis()); try (final FileChannel c = FileChannel.open(filePath, StandardOpenOption.READ)) { @@ -137,7 +78,7 @@ class EncryptedFilePart extends EncryptedFile { } private String getContentRangeHeader(long firstByte, long lastByte, long completeLength) { - return String.format("%d-%d/%d", firstByte, lastByte, completeLength); + return String.format("bytes %d-%d/%d", firstByte, lastByte, completeLength); } } diff --git a/main/core/src/test/java/org/cryptomator/webdav/jackrabbit/RangeRequestTest.java b/main/core/src/test/java/org/cryptomator/webdav/jackrabbit/RangeRequestTest.java index 40bc05d2a..a79daf23d 100644 --- a/main/core/src/test/java/org/cryptomator/webdav/jackrabbit/RangeRequestTest.java +++ b/main/core/src/test/java/org/cryptomator/webdav/jackrabbit/RangeRequestTest.java @@ -1,10 +1,7 @@ package org.cryptomator.webdav.jackrabbit; -import java.io.ByteArrayInputStream; import java.io.File; import java.io.IOException; -import java.io.InputStream; -import java.net.HttpURLConnection; import java.net.URI; import java.net.URISyntaxException; import java.net.URL; @@ -15,6 +12,13 @@ import java.util.Collection; import java.util.Random; import java.util.concurrent.ForkJoinTask; +import org.apache.commons.httpclient.HttpClient; +import org.apache.commons.httpclient.HttpMethod; +import org.apache.commons.httpclient.MultiThreadedHttpConnectionManager; +import org.apache.commons.httpclient.methods.ByteArrayRequestEntity; +import org.apache.commons.httpclient.methods.EntityEnclosingMethod; +import org.apache.commons.httpclient.methods.GetMethod; +import org.apache.commons.httpclient.methods.PutMethod; import org.apache.commons.io.FileUtils; import org.apache.commons.io.IOUtils; import org.cryptomator.crypto.aes256.Aes256Cryptor; @@ -31,27 +35,34 @@ public class RangeRequestTest { private static final Aes256Cryptor CRYPTOR = new Aes256Cryptor(); private static final WebDavServer SERVER = new WebDavServer(); + private static final File TMP_VAULT = Files.createTempDir(); + private static ServletLifeCycleAdapter SERVLET; + private static URI VAULT_BASE_URI; @BeforeClass - public static void startServer() { + public static void startServer() throws URISyntaxException { SERVER.start(); + SERVLET = SERVER.createServlet(TMP_VAULT.toPath(), CRYPTOR, new ArrayList(), new ArrayList(), "JUnitTestVault"); + SERVLET.start(); + VAULT_BASE_URI = new URI("http", SERVLET.getServletUri().getSchemeSpecificPart() + "/", null); + Assert.assertTrue(SERVLET.isRunning()); + Assert.assertNotNull(VAULT_BASE_URI); } @AfterClass public static void stopServer() { + SERVLET.stop(); SERVER.stop(); + FileUtils.deleteQuietly(TMP_VAULT); } @Test public void testAsyncRangeRequests() throws IOException, URISyntaxException { - final File tmpVault = Files.createTempDir(); - final ServletLifeCycleAdapter servlet = SERVER.createServlet(tmpVault.toPath(), CRYPTOR, new ArrayList(), new ArrayList(), "JUnitTestVault"); - final boolean started = servlet.start(); - final URI vaultBaseUri = new URI("http", servlet.getServletUri().getSchemeSpecificPart() + "/", null); - final URL testResourceUrl = new URL(vaultBaseUri.toURL(), "testfile.txt"); + final URL testResourceUrl = new URL(VAULT_BASE_URI.toURL(), "asyncRangeRequestTestFile.txt"); - Assert.assertTrue(started); - Assert.assertNotNull(vaultBaseUri); + final MultiThreadedHttpConnectionManager cm = new MultiThreadedHttpConnectionManager(); + cm.getParams().setDefaultMaxConnectionsPerHost(50); + final HttpClient client = new HttpClient(cm); // prepare 8MiB test data: final byte[] plaintextData = new byte[2097152 * Integer.BYTES]; @@ -59,37 +70,32 @@ public class RangeRequestTest { for (int i = 0; i < 2097152; i++) { bbIn.putInt(i); } - final InputStream plaintextIn = new ByteArrayInputStream(plaintextData); // put request: - final HttpURLConnection putConn = (HttpURLConnection) testResourceUrl.openConnection(); - putConn.setDoOutput(true); - putConn.setRequestMethod("PUT"); - IOUtils.copy(plaintextIn, putConn.getOutputStream()); - putConn.getOutputStream().close(); - final int putResponse = putConn.getResponseCode(); - putConn.disconnect(); + final EntityEnclosingMethod putMethod = new PutMethod(testResourceUrl.toString()); + putMethod.setRequestEntity(new ByteArrayRequestEntity(plaintextData)); + final int putResponse = client.executeMethod(putMethod); + putMethod.releaseConnection(); Assert.assertEquals(201, putResponse); // multiple async range requests: final Collection> tasks = new ArrayList<>(); final Random generator = new Random(System.currentTimeMillis()); - for (int i = 0; i < 100; i++) { + for (int i = 0; i < 200; i++) { final int pos1 = generator.nextInt(plaintextData.length); final int pos2 = generator.nextInt(plaintextData.length); final ForkJoinTask task = ForkJoinTask.adapt(() -> { try { - final HttpURLConnection conn = (HttpURLConnection) testResourceUrl.openConnection(); final int lower = Math.min(pos1, pos2); final int upper = Math.max(pos1, pos2); - conn.setRequestMethod("GET"); - conn.addRequestProperty("Range", "bytes=" + lower + "-" + upper); - final int rangeResponse = conn.getResponseCode(); - final byte[] buffer = new byte[upper - lower + 1]; - final int bytesReceived = IOUtils.read(conn.getInputStream(), buffer); - Assert.assertEquals(206, rangeResponse); - Assert.assertEquals(buffer.length, bytesReceived); - Assert.assertArrayEquals(Arrays.copyOfRange(plaintextData, lower, upper + 1), buffer); + final HttpMethod getMethod = new GetMethod(testResourceUrl.toString()); + getMethod.addRequestHeader("Range", "bytes=" + lower + "-" + upper); + final int statusCode = client.executeMethod(getMethod); + final byte[] responseBody = new byte[upper - lower + 1]; + IOUtils.read(getMethod.getResponseBodyAsStream(), responseBody); + getMethod.releaseConnection(); + Assert.assertEquals(206, statusCode); + Assert.assertArrayEquals(Arrays.copyOfRange(plaintextData, lower, upper + 1), responseBody); } catch (IOException e) { throw new RuntimeException(e); } @@ -101,9 +107,32 @@ public class RangeRequestTest { task.join(); } - servlet.stop(); + cm.shutdown(); + } - FileUtils.deleteQuietly(tmpVault); + @Test + public void testUnsatisfiableRangeRequest() throws IOException, URISyntaxException { + final URL testResourceUrl = new URL(VAULT_BASE_URI.toURL(), "unsatisfiableRangeRequestTestFile.txt"); + final HttpClient client = new HttpClient(); + + // prepare file content: + final byte[] fileContent = "This is some test file content.".getBytes(); + + // put request: + final EntityEnclosingMethod putMethod = new PutMethod(testResourceUrl.toString()); + putMethod.setRequestEntity(new ByteArrayRequestEntity(fileContent)); + final int putResponse = client.executeMethod(putMethod); + putMethod.releaseConnection(); + Assert.assertEquals(201, putResponse); + + // get request: + final HttpMethod getMethod = new GetMethod(testResourceUrl.toString()); + getMethod.addRequestHeader("Range", "chunks=1-2"); + final int getResponse = client.executeMethod(getMethod); + final byte[] response = getMethod.getResponseBody(); + getMethod.releaseConnection(); + Assert.assertEquals(416, getResponse); + Assert.assertArrayEquals(fileContent, response); } }