Browse Source

- 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)

Sebastian Stenzel 9 years ago
parent
commit
a00086ff2d

+ 5 - 0
main/core/pom.xml

@@ -46,6 +46,11 @@
 			<artifactId>jetty-webapp</artifactId>
 			<version>${jetty.version}</version>
 		</dependency>
+		<dependency>
+			<groupId>commons-httpclient</groupId>
+			<artifactId>commons-httpclient</artifactId>
+			<scope>test</scope>
+		</dependency>
 
 		<!-- Jackrabbit -->
 		<dependency>

+ 68 - 5
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<String, String> 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 <code>true</code> 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.<br/>
+	 * <code>
+	 * bytes=100-200<br/>
+	 * bytes=-500<br/>
+	 * bytes=1000-
+	 * </code>
+	 * 
+	 * @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<String, String> 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<String, String> requestRange, Path filePath) {
+		return new EncryptedFilePart(this, locator, session, requestRange, lockManager, cryptor, cryptoWarningHandler, filePath);
 	}
 
 	private EncryptedFile createFile(DavResourceLocator locator, DavSession session, Path filePath) {

+ 4 - 0
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;

+ 16 - 75
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<Long, Long> 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<Pair<Long, Long>> requestedContentRanges = new HashSet<Pair<Long, Long>>();
-
-	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<String, String> 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<Long, Long>(lower, upper));
-		}
-	}
 
-	/**
-	 * @return One range, that spans all requested ranges.
-	 */
-	private Pair<Long, Long> getUnionRange(Long fileSize) {
-		final long lastByte = fileSize - 1;
-		final MutablePair<Long, Long> result = new MutablePair<Long, Long>();
-		for (Pair<Long, Long> 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<Long, Long>(contentLength - upper, contentLength - 1);
+			} else if (upper == null) {
+				range = new ImmutablePair<Long, Long>(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<Long, Long>(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<Long, Long> 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);
 	}
 
 }

+ 60 - 31
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<String>(), new ArrayList<String>(), "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<String>(), new ArrayList<String>(), "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<ForkJoinTask<?>> 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();
+	}
+
+	@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);
 
-		FileUtils.deleteQuietly(tmpVault);
+		// 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);
 	}
 
 }