This is an automated email from the ASF dual-hosted git repository. btellier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 5b9639171bbf6da99ec7cc7bf9cbb01d4b0393dd Author: duc91 <duc91....@gmail.com> AuthorDate: Tue Jun 2 10:41:09 2020 +0700 JAMES-3199: add validate mailboxname and also test at UserMailboxesRoutesTest --- .../HasEmptyMailboxNameInHierarchyException.java | 9 -- .../apache/james/mailbox/model/MailboxPath.java | 6 +- .../james/webadmin/routes/UserMailboxesRoutes.java | 17 +-- .../webadmin/service/UserMailboxesService.java | 15 +-- .../james/webadmin/validation/MailboxName.java | 7 +- .../webadmin/routes/UserMailboxesRoutesTest.java | 116 ++++++++++++++++----- 6 files changed, 111 insertions(+), 59 deletions(-) diff --git a/mailbox/api/src/main/java/org/apache/james/mailbox/exception/HasEmptyMailboxNameInHierarchyException.java b/mailbox/api/src/main/java/org/apache/james/mailbox/exception/HasEmptyMailboxNameInHierarchyException.java index 5363c95..fc49e42 100644 --- a/mailbox/api/src/main/java/org/apache/james/mailbox/exception/HasEmptyMailboxNameInHierarchyException.java +++ b/mailbox/api/src/main/java/org/apache/james/mailbox/exception/HasEmptyMailboxNameInHierarchyException.java @@ -20,16 +20,7 @@ package org.apache.james.mailbox.exception; public class HasEmptyMailboxNameInHierarchyException extends MailboxNameException { - public HasEmptyMailboxNameInHierarchyException() { - super(); - } - public HasEmptyMailboxNameInHierarchyException(String message) { super(message); } - - public HasEmptyMailboxNameInHierarchyException(String message, Throwable cause) { - super(message, cause); - } - } diff --git a/mailbox/api/src/main/java/org/apache/james/mailbox/model/MailboxPath.java b/mailbox/api/src/main/java/org/apache/james/mailbox/model/MailboxPath.java index 3c6a8ed..36340ee 100644 --- a/mailbox/api/src/main/java/org/apache/james/mailbox/model/MailboxPath.java +++ b/mailbox/api/src/main/java/org/apache/james/mailbox/model/MailboxPath.java @@ -159,9 +159,10 @@ public class MailboxPath { return this; } - public void assertAcceptable(char pathDelimiter) throws MailboxNameException { + public MailboxPath assertAcceptable(char pathDelimiter) throws MailboxNameException { if (hasEmptyNameInHierarchy(pathDelimiter)) { - throw new HasEmptyMailboxNameInHierarchyException(asString()); + throw new HasEmptyMailboxNameInHierarchyException( + String.format("'%s' has an empty part within its mailbox name considering %s as a delimiter", asString(), pathDelimiter)); } if (nameContainsForbiddenCharacters()) { throw new MailboxNameException(asString() + " contains one of the forbidden characters " + INVALID_CHARS); @@ -169,6 +170,7 @@ public class MailboxPath { if (isMailboxNameTooLong()) { throw new TooLongMailboxNameException("Mailbox name exceeds maximum size of " + MAX_MAILBOX_NAME_LENGTH + " characters"); } + return this; } private boolean nameContainsForbiddenCharacters() { diff --git a/server/protocols/webadmin/webadmin-mailbox/src/main/java/org/apache/james/webadmin/routes/UserMailboxesRoutes.java b/server/protocols/webadmin/webadmin-mailbox/src/main/java/org/apache/james/webadmin/routes/UserMailboxesRoutes.java index c714376..a18747d 100644 --- a/server/protocols/webadmin/webadmin-mailbox/src/main/java/org/apache/james/webadmin/routes/UserMailboxesRoutes.java +++ b/server/protocols/webadmin/webadmin-mailbox/src/main/java/org/apache/james/webadmin/routes/UserMailboxesRoutes.java @@ -35,6 +35,7 @@ import javax.ws.rs.Path; import javax.ws.rs.Produces; import org.apache.james.core.Username; +import org.apache.james.mailbox.exception.MailboxNameException; import org.apache.james.mailbox.indexer.ReIndexer; import org.apache.james.task.TaskManager; import org.apache.james.webadmin.Constants; @@ -217,12 +218,12 @@ public class UserMailboxesRoutes implements Routes { .message("Attempt to delete a mailbox with children") .cause(e) .haltError(); - } catch (IllegalArgumentException e) { - LOGGER.info("Attempt to create an invalid mailbox"); + } catch (IllegalArgumentException | MailboxNameException e) { + LOGGER.info("Attempt to delete an invalid mailbox", e); throw ErrorResponder.builder() .statusCode(HttpStatus.BAD_REQUEST_400) .type(ErrorType.INVALID_ARGUMENT) - .message("Attempt to create an invalid mailbox") + .message("Attempt to delete an invalid mailbox") .cause(e) .haltError(); } @@ -291,12 +292,12 @@ public class UserMailboxesRoutes implements Routes { .message("Invalid get on user mailboxes") .cause(e) .haltError(); - } catch (IllegalArgumentException e) { - LOGGER.info("Attempt to create an invalid mailbox"); + } catch (IllegalArgumentException | MailboxNameException e) { + LOGGER.info("Attempt to test existence of an invalid mailbox", e); throw ErrorResponder.builder() .statusCode(HttpStatus.BAD_REQUEST_400) .type(ErrorType.INVALID_ARGUMENT) - .message("Attempt to create an invalid mailbox") + .message("Attempt to test existence of an invalid mailbox") .cause(e) .haltError(); } @@ -330,8 +331,8 @@ public class UserMailboxesRoutes implements Routes { .message("Invalid get on user mailboxes") .cause(e) .haltError(); - } catch (IllegalArgumentException e) { - LOGGER.info("Attempt to create an invalid mailbox"); + } catch (IllegalArgumentException | MailboxNameException e) { + LOGGER.info("Attempt to create an invalid mailbox", e); throw ErrorResponder.builder() .statusCode(HttpStatus.BAD_REQUEST_400) .type(ErrorType.INVALID_ARGUMENT) diff --git a/server/protocols/webadmin/webadmin-mailbox/src/main/java/org/apache/james/webadmin/service/UserMailboxesService.java b/server/protocols/webadmin/webadmin-mailbox/src/main/java/org/apache/james/webadmin/service/UserMailboxesService.java index 003220f..8f4e8e7 100644 --- a/server/protocols/webadmin/webadmin-mailbox/src/main/java/org/apache/james/webadmin/service/UserMailboxesService.java +++ b/server/protocols/webadmin/webadmin-mailbox/src/main/java/org/apache/james/webadmin/service/UserMailboxesService.java @@ -64,9 +64,9 @@ public class UserMailboxesService { usernamePreconditions(username); MailboxSession mailboxSession = mailboxManager.createSystemSession(username); try { - mailboxManager.createMailbox( - MailboxPath.forUser(username, mailboxName.asString()), - mailboxSession); + MailboxPath mailboxPath = MailboxPath.forUser(username, mailboxName.asString()) + .assertAcceptable(mailboxSession.getPathDelimiter()); + mailboxManager.createMailbox(mailboxPath, mailboxSession); } catch (MailboxExistsException e) { LOGGER.info("Attempt to create mailbox {} for user {} that already exists", mailboxName, username); } @@ -91,16 +91,17 @@ public class UserMailboxesService { public boolean testMailboxExists(Username username, MailboxName mailboxName) throws MailboxException, UsersRepositoryException { usernamePreconditions(username); MailboxSession mailboxSession = mailboxManager.createSystemSession(username); - return Mono.from(mailboxManager.mailboxExists( - MailboxPath.forUser(username, mailboxName.asString()), - mailboxSession)) + MailboxPath mailboxPath = MailboxPath.forUser(username, mailboxName.asString()) + .assertAcceptable(mailboxSession.getPathDelimiter()); + return Mono.from(mailboxManager.mailboxExists(mailboxPath, mailboxSession)) .block(); } public void deleteMailbox(Username username, MailboxName mailboxName) throws MailboxException, UsersRepositoryException, MailboxHaveChildrenException { usernamePreconditions(username); MailboxSession mailboxSession = mailboxManager.createSystemSession(username); - MailboxPath mailboxPath = MailboxPath.forUser(username, mailboxName.asString()); + MailboxPath mailboxPath = MailboxPath.forUser(username, mailboxName.asString()) + .assertAcceptable(mailboxSession.getPathDelimiter()); listChildren(mailboxPath, mailboxSession) .forEach(Throwing.consumer(path -> deleteMailbox(mailboxSession, path))); } diff --git a/server/protocols/webadmin/webadmin-mailbox/src/main/java/org/apache/james/webadmin/validation/MailboxName.java b/server/protocols/webadmin/webadmin-mailbox/src/main/java/org/apache/james/webadmin/validation/MailboxName.java index fdd079d..35d3844 100644 --- a/server/protocols/webadmin/webadmin-mailbox/src/main/java/org/apache/james/webadmin/validation/MailboxName.java +++ b/server/protocols/webadmin/webadmin-mailbox/src/main/java/org/apache/james/webadmin/validation/MailboxName.java @@ -19,18 +19,15 @@ package org.apache.james.webadmin.validation; -import com.google.common.base.CharMatcher; import com.google.common.base.Preconditions; import com.google.common.base.Strings; public class MailboxName { - - public static final CharMatcher INVALID_CHARS_MATCHER = CharMatcher.anyOf("%*&#"); private final String mailboxName; public MailboxName(String mailboxName) { - Preconditions.checkArgument(!Strings.isNullOrEmpty(mailboxName)); - Preconditions.checkArgument(INVALID_CHARS_MATCHER.matchesNoneOf(mailboxName)); + Preconditions.checkArgument(!Strings.isNullOrEmpty(mailboxName), "MailboxName must not be null or empty"); + this.mailboxName = mailboxName; } diff --git a/server/protocols/webadmin/webadmin-mailbox/src/test/java/org/apache/james/webadmin/routes/UserMailboxesRoutesTest.java b/server/protocols/webadmin/webadmin-mailbox/src/test/java/org/apache/james/webadmin/routes/UserMailboxesRoutesTest.java index 5d21359..35f8374 100644 --- a/server/protocols/webadmin/webadmin-mailbox/src/test/java/org/apache/james/webadmin/routes/UserMailboxesRoutesTest.java +++ b/server/protocols/webadmin/webadmin-mailbox/src/test/java/org/apache/james/webadmin/routes/UserMailboxesRoutesTest.java @@ -90,6 +90,8 @@ import reactor.core.publisher.Mono; class UserMailboxesRoutesTest { private static final Username USERNAME = Username.of("username"); private static final String MAILBOX_NAME = "myMailboxName"; + private static final String MAILBOX_NAME_WITH_DOTS = "my..MailboxName"; + private static final String INVALID_MAILBOX_NAME = "myMailboxName#"; private static final MailboxPath INBOX = MailboxPath.inbox(USERNAME); private WebAdminServer webAdminServer; @@ -203,6 +205,84 @@ class UserMailboxesRoutesTest { } @Test + void putShouldThrowWhenMailboxNameWithDots() throws Exception { + Map<String, Object> errors = when() + .put(MAILBOX_NAME_WITH_DOTS) + .then() + .statusCode(HttpStatus.BAD_REQUEST_400) + .contentType(ContentType.JSON) + .extract() + .body() + .jsonPath() + .getMap("."); + + assertThat(errors) + .containsEntry("statusCode", HttpStatus.BAD_REQUEST_400) + .containsEntry("type", "InvalidArgument") + .containsEntry("message", "Attempt to create an invalid mailbox") + .containsEntry("details", "'#private:username:my..MailboxName' has an empty part within its mailbox name considering . as a delimiter"); + } + + @Test + void putShouldThrowWhenMailboxNameStartsWithDot() throws Exception { + Map<String, Object> errors = when() + .put(".startWithDot") + .then() + .statusCode(HttpStatus.BAD_REQUEST_400) + .contentType(ContentType.JSON) + .extract() + .body() + .jsonPath() + .getMap("."); + + assertThat(errors) + .containsEntry("statusCode", HttpStatus.BAD_REQUEST_400) + .containsEntry("type", "InvalidArgument") + .containsEntry("message", "Attempt to create an invalid mailbox") + .containsEntry("details", "'#private:username:.startWithDot' has an empty part within its mailbox name considering . as a delimiter"); + } + + @Test + void putShouldThrowWhenMailboxNameEndsWithDots() throws Exception { + Map<String, Object> errors = when() + .put("endWithDot.") + .then() + .statusCode(HttpStatus.BAD_REQUEST_400) + .contentType(ContentType.JSON) + .extract() + .body() + .jsonPath() + .getMap("."); + + assertThat(errors) + .containsEntry("statusCode", HttpStatus.BAD_REQUEST_400) + .containsEntry("type", "InvalidArgument") + .containsEntry("message", "Attempt to create an invalid mailbox") + .containsEntry("details", "'#private:username:endWithDot.' has an empty part within its mailbox name considering . as a delimiter"); + } + + @Test + void putShouldThrowWhenInvalidMailboxName() throws Exception { + when(usersRepository.contains(USERNAME)).thenReturn(true); + + Map<String, Object> errors = when() + .put(INVALID_MAILBOX_NAME) + .then() + .statusCode(HttpStatus.BAD_REQUEST_400) + .contentType(ContentType.JSON) + .extract() + .body() + .jsonPath() + .getMap("."); + + assertThat(errors) + .containsEntry("statusCode", HttpStatus.BAD_REQUEST_400) + .containsEntry("type", "InvalidArgument") + .containsEntry("message", "Attempt to create an invalid mailbox") + .containsEntry("details", "#private:username:myMailboxName# contains one of the forbidden characters %*&#"); + } + + @Test void deleteShouldReturnNotFoundWithNonExistingUser() throws Exception { when(usersRepository.contains(USERNAME)).thenReturn(false); @@ -224,8 +304,6 @@ class UserMailboxesRoutesTest { @Test void getShouldReturnUserErrorWithInvalidWildcardMailboxName() throws Exception { - when(usersRepository.contains(USERNAME)).thenReturn(false); - Map<String, Object> errors = when() .get(MAILBOX_NAME + "*") .then() @@ -239,13 +317,12 @@ class UserMailboxesRoutesTest { assertThat(errors) .containsEntry("statusCode", HttpStatus.BAD_REQUEST_400) .containsEntry("type", "InvalidArgument") - .containsEntry("message", "Attempt to create an invalid mailbox"); + .containsEntry("message", "Attempt to test existence of an invalid mailbox") + .containsEntry("details", "#private:username:myMailboxName* contains one of the forbidden characters %*&#"); } @Test void putShouldReturnUserErrorWithInvalidWildcardMailboxName() throws Exception { - when(usersRepository.contains(USERNAME)).thenReturn(false); - Map<String, Object> errors = when() .put(MAILBOX_NAME + "*") .then() @@ -264,8 +341,6 @@ class UserMailboxesRoutesTest { @Test void deleteShouldReturnUserErrorWithInvalidWildcardMailboxName() throws Exception { - when(usersRepository.contains(USERNAME)).thenReturn(false); - Map<String, Object> errors = when() .put(MAILBOX_NAME + "*") .then() @@ -284,8 +359,6 @@ class UserMailboxesRoutesTest { @Test void getShouldReturnUserErrorWithInvalidPercentMailboxName() throws Exception { - when(usersRepository.contains(USERNAME)).thenReturn(false); - Map<String, Object> errors = when() .get(MAILBOX_NAME + "%") .then() @@ -299,13 +372,12 @@ class UserMailboxesRoutesTest { assertThat(errors) .containsEntry("statusCode", HttpStatus.BAD_REQUEST_400) .containsEntry("type", "InvalidArgument") - .containsEntry("message", "Attempt to create an invalid mailbox"); + .containsEntry("message", "Attempt to test existence of an invalid mailbox") + .containsEntry("details", "#private:username:myMailboxName% contains one of the forbidden characters %*&#"); } @Test void putShouldReturnUserErrorWithInvalidPercentMailboxName() throws Exception { - when(usersRepository.contains(USERNAME)).thenReturn(false); - Map<String, Object> errors = when() .put(MAILBOX_NAME + "%") .then() @@ -324,8 +396,6 @@ class UserMailboxesRoutesTest { @Test void deleteShouldReturnUserErrorWithInvalidPercentMailboxName() throws Exception { - when(usersRepository.contains(USERNAME)).thenReturn(false); - Map<String, Object> errors = when() .put(MAILBOX_NAME + "%") .then() @@ -344,8 +414,6 @@ class UserMailboxesRoutesTest { @Test void getShouldReturnUserErrorWithInvalidSharpMailboxName() throws Exception { - when(usersRepository.contains(USERNAME)).thenReturn(false); - Map<String, Object> errors = when() .get(MAILBOX_NAME + "#") .then() @@ -359,13 +427,12 @@ class UserMailboxesRoutesTest { assertThat(errors) .containsEntry("statusCode", HttpStatus.BAD_REQUEST_400) .containsEntry("type", "InvalidArgument") - .containsEntry("message", "Attempt to create an invalid mailbox"); + .containsEntry("message", "Attempt to test existence of an invalid mailbox") + .containsEntry("details", "#private:username:myMailboxName# contains one of the forbidden characters %*&#"); } @Test void putShouldReturnUserErrorWithInvalidSharpMailboxName() throws Exception { - when(usersRepository.contains(USERNAME)).thenReturn(false); - Map<String, Object> errors = when() .put(MAILBOX_NAME + "#") .then() @@ -384,8 +451,6 @@ class UserMailboxesRoutesTest { @Test void deleteShouldReturnUserErrorWithInvalidSharpMailboxName() throws Exception { - when(usersRepository.contains(USERNAME)).thenReturn(false); - Map<String, Object> errors = when() .put(MAILBOX_NAME + "#") .then() @@ -404,8 +469,6 @@ class UserMailboxesRoutesTest { @Test void getShouldReturnUserErrorWithInvalidAndMailboxName() throws Exception { - when(usersRepository.contains(USERNAME)).thenReturn(false); - Map<String, Object> errors = when() .get(MAILBOX_NAME + "&") .then() @@ -419,13 +482,12 @@ class UserMailboxesRoutesTest { assertThat(errors) .containsEntry("statusCode", HttpStatus.BAD_REQUEST_400) .containsEntry("type", "InvalidArgument") - .containsEntry("message", "Attempt to create an invalid mailbox"); + .containsEntry("message", "Attempt to test existence of an invalid mailbox") + .containsEntry("details", "#private:username:myMailboxName& contains one of the forbidden characters %*&#"); } @Test void putShouldReturnUserErrorWithInvalidAndMailboxName() throws Exception { - when(usersRepository.contains(USERNAME)).thenReturn(false); - Map<String, Object> errors = when() .put(MAILBOX_NAME + "&") .then() @@ -444,8 +506,6 @@ class UserMailboxesRoutesTest { @Test void deleteShouldReturnUserErrorWithInvalidAndMailboxName() throws Exception { - when(usersRepository.contains(USERNAME)).thenReturn(false); - Map<String, Object> errors = when() .put(MAILBOX_NAME + "&") .then() --------------------------------------------------------------------- To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org