JAMES-1777 Add name variable in download endpoint
Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/f643c184 Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/f643c184 Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/f643c184 Branch: refs/heads/master Commit: f643c184bfba52c003d70265063063aec5995b9d Parents: bc5afa0 Author: Antoine Duprat <[email protected]> Authored: Thu Jun 23 14:26:01 2016 +0200 Committer: Antoine Duprat <[email protected]> Committed: Fri Jul 1 16:32:41 2016 +0200 ---------------------------------------------------------------------- .../integration/cucumber/DownloadStepdefs.java | 30 +++++-- .../test/resources/cucumber/DownloadGet.feature | 6 ++ .../org/apache/james/jmap/DownloadServlet.java | 35 ++++---- .../apache/james/jmap/utils/DownloadPath.java | 65 +++++++++++++++ .../apache/james/jmap/DownloadServletTest.java | 13 +-- .../james/jmap/utils/DownloadPathTest.java | 84 ++++++++++++++++++++ 6 files changed, 202 insertions(+), 31 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/f643c184/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/java/org/apache/james/jmap/methods/integration/cucumber/DownloadStepdefs.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/java/org/apache/james/jmap/methods/integration/cucumber/DownloadStepdefs.java b/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/java/org/apache/james/jmap/methods/integration/cucumber/DownloadStepdefs.java index 74b2d49..e675d10 100644 --- a/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/java/org/apache/james/jmap/methods/integration/cucumber/DownloadStepdefs.java +++ b/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/java/org/apache/james/jmap/methods/integration/cucumber/DownloadStepdefs.java @@ -38,6 +38,7 @@ import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.Multimap; import com.jayway.restassured.http.ContentType; import com.jayway.restassured.response.Response; +import com.jayway.restassured.response.ValidatableResponse; import com.jayway.restassured.specification.RequestSpecification; import cucumber.api.java.en.Given; @@ -53,6 +54,7 @@ public class DownloadStepdefs { private Response response; private Multimap<String, String> attachmentsByMessageId; private Map<String, String> blobIdByAttachmentId; + private ValidatableResponse validatableResponse; @Inject private DownloadStepdefs(MainStepdefs mainStepdefs, UserStepdefs userStepdefs) { @@ -125,33 +127,44 @@ public class DownloadStepdefs { .post("/download/" + blobId); } + @When("^\"([^\"]*)\" downloads \"([^\"]*)\" with \"([^\"]*)\" name$") + public void downloadsWithName(String username, String attachmentId, String name) { + String blobId = blobIdByAttachmentId.get(attachmentId); + AccessToken accessToken = userStepdefs.tokenByUser.get(username); + RequestSpecification with = with(); + if (accessToken != null) { + with.header("Authorization", accessToken.serialize()); + } + response = with.get("/download/" + blobId + "/" + name); + } + @Then("^the user should be authorized$") - public void httpStatusDifferentFromUnauthorized() throws Exception { + public void httpStatusDifferentFromUnauthorized() { response.then() .statusCode(not(401)); } @Then("^the user should not be authorized$") - public void httpUnauthorizedStatus() throws Exception { + public void httpUnauthorizedStatus() { response.then() .statusCode(401); } @Then("^the user should receive a bad request response$") - public void httpBadRequestStatus() throws Throwable { + public void httpBadRequestStatus() { response.then() .statusCode(400); } @Then("^the user should receive that attachment$") - public void httpOkStatusAndExpectedContent() throws Throwable { - response.then() + public void httpOkStatusAndExpectedContent() { + validatableResponse = response.then() .statusCode(200) .content(notNullValue()); } @Then("^the user should receive a not found response$") - public void httpNotFoundStatus() throws Throwable { + public void httpNotFoundStatus() { response.then() .statusCode(404); } @@ -163,4 +176,9 @@ public class DownloadStepdefs { .contentType(ContentType.TEXT) .content(notNullValue()); } + + @Then("^the attachment is named \"([^\"]*)\"$") + public void assertContentDisposition(String name) { + validatableResponse.header("Content-Disposition", name); + } } http://git-wip-us.apache.org/repos/asf/james-project/blob/f643c184/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/resources/cucumber/DownloadGet.feature ---------------------------------------------------------------------- diff --git a/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/resources/cucumber/DownloadGet.feature b/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/resources/cucumber/DownloadGet.feature index 7b02ee8..bf3d21a 100644 --- a/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/resources/cucumber/DownloadGet.feature +++ b/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/resources/cucumber/DownloadGet.feature @@ -15,3 +15,9 @@ Feature: Download GET Scenario: Getting an attachment with an unknown blobId When "[email protected]" downloads "123" Then the user should receive a not found response + + Scenario: Getting an attachment previously stored with a desired name + Given "[email protected]" mailbox "inbox" contains a message "1" with an attachment "2" + When "[email protected]" downloads "2" with "myFileName.txt" name + Then the user should receive that attachment + And the attachment is named "myFileName.txt" http://git-wip-us.apache.org/repos/asf/james-project/blob/f643c184/server/protocols/jmap/src/main/java/org/apache/james/jmap/DownloadServlet.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/DownloadServlet.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/DownloadServlet.java index ccd8d86..49d0c79 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/DownloadServlet.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/DownloadServlet.java @@ -24,6 +24,7 @@ import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND; import static javax.servlet.http.HttpServletResponse.SC_OK; import java.io.IOException; +import java.util.Optional; import javax.inject.Inject; import javax.servlet.ServletException; @@ -33,6 +34,7 @@ import javax.servlet.http.HttpServletResponse; import org.apache.commons.io.IOUtils; import org.apache.james.jmap.api.SimpleTokenFactory; +import org.apache.james.jmap.utils.DownloadPath; import org.apache.james.mailbox.MailboxSession; import org.apache.james.mailbox.exception.AttachmentNotFoundException; import org.apache.james.mailbox.exception.MailboxException; @@ -44,11 +46,9 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Strings; public class DownloadServlet extends HttpServlet { - private static final String ROOT_URL = "/"; private static final Logger LOGGER = LoggerFactory.getLogger(DownloadServlet.class); private static final String TEXT_PLAIN_CONTENT_TYPE = "text/plain"; @@ -64,14 +64,16 @@ public class DownloadServlet extends HttpServlet { @Override protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException { String pathInfo = req.getPathInfo(); - if (Strings.isNullOrEmpty(pathInfo) || pathInfo.equals(ROOT_URL)) { + try { + respondAttachmentAccessToken(getMailboxSession(req), DownloadPath.from(pathInfo), resp); + } catch (IllegalArgumentException e) { + LOGGER.error(String.format("Error while generating attachment access token '%s'", pathInfo), e); resp.setStatus(SC_BAD_REQUEST); - } else { - respondAttachmentAccessToken(getMailboxSession(req), blobIdFrom(pathInfo), resp); } } - private void respondAttachmentAccessToken(MailboxSession mailboxSession, String blobId, HttpServletResponse resp) { + private void respondAttachmentAccessToken(MailboxSession mailboxSession, DownloadPath downloadPath, HttpServletResponse resp) { + String blobId = downloadPath.getBlobId(); try { if (! attachmentExists(mailboxSession, blobId)) { resp.setStatus(SC_NOT_FOUND); @@ -99,22 +101,23 @@ public class DownloadServlet extends HttpServlet { @Override protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException { String pathInfo = req.getPathInfo(); - if (Strings.isNullOrEmpty(pathInfo) || pathInfo.equals(ROOT_URL)) { + try { + download(getMailboxSession(req), DownloadPath.from(pathInfo), resp); + } catch (IllegalArgumentException e) { + LOGGER.error(String.format("Error while downloading '%s'", pathInfo), e); resp.setStatus(SC_BAD_REQUEST); - } else { - download(getMailboxSession(req), blobIdFrom(pathInfo), resp); } } - @VisibleForTesting String blobIdFrom(String pathInfo) { - return pathInfo.substring(1); - } - - @VisibleForTesting void download(MailboxSession mailboxSession, String blobId, HttpServletResponse resp) { + @VisibleForTesting void download(MailboxSession mailboxSession, DownloadPath downloadPath, HttpServletResponse resp) { + String blobId = downloadPath.getBlobId(); try { + addContentDispositionHeader(downloadPath.getName(), resp); + AttachmentMapper attachmentMapper = mailboxSessionMapperFactory.createAttachmentMapper(mailboxSession); Attachment attachment = attachmentMapper.getAttachment(AttachmentId.from(blobId)); IOUtils.copy(attachment.getStream(), resp.getOutputStream()); + resp.setStatus(SC_OK); } catch (AttachmentNotFoundException e) { LOGGER.info(String.format("Attachment '%s' not found", blobId), e); @@ -125,6 +128,10 @@ public class DownloadServlet extends HttpServlet { } } + private void addContentDispositionHeader(Optional<String> optionalName, HttpServletResponse resp) { + optionalName.ifPresent(name -> resp.addHeader("Content-Disposition", name)); + } + private MailboxSession getMailboxSession(HttpServletRequest req) { return (MailboxSession) req.getAttribute(AuthenticationFilter.MAILBOX_SESSION); } http://git-wip-us.apache.org/repos/asf/james-project/blob/f643c184/server/protocols/jmap/src/main/java/org/apache/james/jmap/utils/DownloadPath.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/utils/DownloadPath.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/utils/DownloadPath.java new file mode 100644 index 0000000..3755e8a --- /dev/null +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/utils/DownloadPath.java @@ -0,0 +1,65 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you under the Apache License, Version 2.0 (the * + * "License"); you may not use this file except in compliance * + * with the License. You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, * + * software distributed under the License is distributed on an * + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * + * KIND, either express or implied. See the License for the * + * specific language governing permissions and limitations * + * under the License. * + ****************************************************************/ + +package org.apache.james.jmap.utils; + +import java.util.List; +import java.util.Optional; + +import com.google.common.base.Preconditions; +import com.google.common.base.Splitter; +import com.google.common.base.Strings; +import com.google.common.collect.Iterables; + +public class DownloadPath { + + public static DownloadPath from(String path) { + Preconditions.checkArgument(!Strings.isNullOrEmpty(path), "'path' is mandatory"); + + // path = /blobId/name + // idx = 0 1 2 + List<String> pathVariables = Splitter.on('/').splitToList(path); + Preconditions.checkArgument(pathVariables.size() >= 1 && pathVariables.size() <= 3, "'blobId' is mandatory"); + + String blobId = Iterables.get(pathVariables, 1, null); + Preconditions.checkArgument(!Strings.isNullOrEmpty(blobId), "'blobId' is mandatory"); + + return new DownloadPath(blobId, name(pathVariables)); + } + + private static Optional<String> name(List<String> pathVariables) { + return Optional.ofNullable(Strings.emptyToNull(Iterables.get(pathVariables, 2, null))); + } + + private final String blobId; + private final Optional<String> name; + + private DownloadPath(String blobId, Optional<String> name) { + this.blobId = blobId; + this.name = name; + } + + public String getBlobId() { + return blobId; + } + + public Optional<String> getName() { + return name; + } +} http://git-wip-us.apache.org/repos/asf/james-project/blob/f643c184/server/protocols/jmap/src/test/java/org/apache/james/jmap/DownloadServletTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/test/java/org/apache/james/jmap/DownloadServletTest.java b/server/protocols/jmap/src/test/java/org/apache/james/jmap/DownloadServletTest.java index de9058b..79302b7 100644 --- a/server/protocols/jmap/src/test/java/org/apache/james/jmap/DownloadServletTest.java +++ b/server/protocols/jmap/src/test/java/org/apache/james/jmap/DownloadServletTest.java @@ -19,7 +19,6 @@ package org.apache.james.jmap; -import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -27,6 +26,7 @@ import static org.mockito.Mockito.when; import javax.servlet.http.HttpServletResponse; import org.apache.james.jmap.api.SimpleTokenFactory; +import org.apache.james.jmap.utils.DownloadPath; import org.apache.james.mailbox.MailboxSession; import org.apache.james.mailbox.exception.MailboxException; import org.apache.james.mailbox.store.MailboxSessionMapperFactory; @@ -35,14 +35,6 @@ import org.junit.Test; public class DownloadServletTest { @Test - public void blobIdFromShouldSkipTheFirstCharacter() { - MailboxSessionMapperFactory nullMailboxSessionMapperFactory = null; - SimpleTokenFactory nullSimpleTokenFactory = null; - String blobId = new DownloadServlet(nullMailboxSessionMapperFactory, nullSimpleTokenFactory).blobIdFrom("1234"); - assertThat(blobId).isEqualTo("234"); - } - - @Test public void downloadMayFailWhenUnableToCreateAttachmentMapper() throws Exception { MailboxSession mailboxSession = mock(MailboxSession.class); MailboxSessionMapperFactory mailboxSessionMapperFactory = mock(MailboxSessionMapperFactory.class); @@ -52,9 +44,8 @@ public class DownloadServletTest { DownloadServlet testee = new DownloadServlet(mailboxSessionMapperFactory, nullSimpleTokenFactory); - String blobId = null; HttpServletResponse resp = mock(HttpServletResponse.class); - testee.download(mailboxSession, blobId, resp); + testee.download(mailboxSession, DownloadPath.from("/blobId"), resp); verify(resp).setStatus(500); } http://git-wip-us.apache.org/repos/asf/james-project/blob/f643c184/server/protocols/jmap/src/test/java/org/apache/james/jmap/utils/DownloadPathTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/test/java/org/apache/james/jmap/utils/DownloadPathTest.java b/server/protocols/jmap/src/test/java/org/apache/james/jmap/utils/DownloadPathTest.java new file mode 100644 index 0000000..53eff13 --- /dev/null +++ b/server/protocols/jmap/src/test/java/org/apache/james/jmap/utils/DownloadPathTest.java @@ -0,0 +1,84 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you under the Apache License, Version 2.0 (the * + * "License"); you may not use this file except in compliance * + * with the License. You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, * + * software distributed under the License is distributed on an * + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * + * KIND, either express or implied. See the License for the * + * specific language governing permissions and limitations * + * under the License. * + ****************************************************************/ + +package org.apache.james.jmap.utils; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.junit.Test; + +public class DownloadPathTest { + + @Test + public void fromShouldThrowWhenPathIsNull() { + assertThatThrownBy(()-> DownloadPath.from(null)).isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void fromShouldThrowWhenPathIsEmpty() { + assertThatThrownBy(()-> DownloadPath.from("")).isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void fromShouldThrowWhenNoBlobId() { + assertThatThrownBy(()-> DownloadPath.from("/")).isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void fromShouldThrowWhenBlobIdIsEmpty() { + assertThatThrownBy(()-> DownloadPath.from("//")).isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void fromShouldParseWhenBlobId() { + String expectedBlobId = "123456789"; + DownloadPath downloadPath = DownloadPath.from("/" + expectedBlobId); + assertThat(downloadPath.getBlobId()).isEqualTo(expectedBlobId); + } + + @Test + public void nameShouldBeEmptyWhenNone() { + DownloadPath downloadPath = DownloadPath.from("/123456789"); + assertThat(downloadPath.getName()).isEmpty(); + } + + @Test + public void nameShouldBeEmptyWhenNoneButSlash() { + DownloadPath downloadPath = DownloadPath.from("/123456789/"); + assertThat(downloadPath.getName()).isEmpty(); + } + + @Test + public void nameShouldBePresentWhenGiven() { + String expectedName = "myName"; + DownloadPath downloadPath = DownloadPath.from("/123456789/" + expectedName); + assertThat(downloadPath.getName()).hasValue(expectedName); + } + + @Test + public void fromShouldThrowWhenExtraPathVariables() { + assertThatThrownBy(()-> DownloadPath.from("/123456789/myName/132/456/789")).isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void fromShouldThrowWhenExtraPathSeparator() { + assertThatThrownBy(()-> DownloadPath.from("/123456789//myName")).isInstanceOf(IllegalArgumentException.class); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
