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 9a85562e2506f0710304c876eb8b1917a4faa47f Author: Rémi Kowalski <[email protected]> AuthorDate: Wed Sep 23 10:29:36 2020 +0200 JAMES-3386 do not allow blank mailbox path --- .../apache/james/mailbox/model/MailboxPath.java | 13 +- .../james/mailbox/model/MailboxPathTest.java | 54 +++++++ .../contract/MailboxSetMethodContract.scala | 167 +++++++++++++++++++++ .../scala/org/apache/james/jmap/mail/Mailbox.scala | 5 +- .../apache/james/jmap/model/MailboxFactory.scala | 5 +- 5 files changed, 234 insertions(+), 10 deletions(-) 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 3d29587..58044a7 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 @@ -25,6 +25,7 @@ import java.util.Objects; import java.util.Optional; import java.util.function.Predicate; +import org.apache.commons.lang3.StringUtils; import org.apache.james.core.Username; import org.apache.james.mailbox.DefaultMailboxes; import org.apache.james.mailbox.MailboxSession; @@ -35,7 +36,6 @@ import org.apache.james.mailbox.exception.TooLongMailboxNameException; import com.google.common.base.CharMatcher; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; -import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; @@ -114,7 +114,7 @@ public class MailboxPath { } public MailboxPath child(String childName, char delimiter) { - Preconditions.checkArgument(!Strings.isNullOrEmpty(childName), "'childName' should not be null or empty"); + Preconditions.checkArgument(!StringUtils.isBlank(childName), "'childName' should not be null or empty"); Preconditions.checkArgument(!childName.contains(String.valueOf(delimiter)), "'childName' should not contain delimiter"); return new MailboxPath(namespace, user, name + delimiter + childName); @@ -194,10 +194,11 @@ public class MailboxPath { boolean hasEmptyNameInHierarchy(char pathDelimiter) { String delimiterString = String.valueOf(pathDelimiter); - return this.name.isEmpty() - || this.name.contains(delimiterString + delimiterString) - || this.name.startsWith(delimiterString) - || this.name.endsWith(delimiterString); + String nameWithoutSpaces = name.replaceAll("\\s", ""); + return StringUtils.isBlank(nameWithoutSpaces) + || nameWithoutSpaces.contains(delimiterString + delimiterString) + || nameWithoutSpaces.startsWith(delimiterString) + || nameWithoutSpaces.endsWith(delimiterString); } public String asString() { diff --git a/mailbox/api/src/test/java/org/apache/james/mailbox/model/MailboxPathTest.java b/mailbox/api/src/test/java/org/apache/james/mailbox/model/MailboxPathTest.java index a4d63c7..a160b3c 100644 --- a/mailbox/api/src/test/java/org/apache/james/mailbox/model/MailboxPathTest.java +++ b/mailbox/api/src/test/java/org/apache/james/mailbox/model/MailboxPathTest.java @@ -69,6 +69,12 @@ class MailboxPathTest { } @Test + void getNameShouldNoopWhenBlank() { + assertThat(MailboxPath.forUser(USER, " ").getName('.')) + .isEqualTo(" "); + } + + @Test void getHierarchyLevelsShouldBeOrdered() { assertThat(MailboxPath.forUser(USER, "inbox.folder.subfolder") .getHierarchyLevels('.')) @@ -100,6 +106,13 @@ class MailboxPathTest { } @Test + void childShouldThrowWhenBlank() { + MailboxPath path = MailboxPath.forUser(USER, "folder"); + assertThatThrownBy(() -> path.child(" ", '.')) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test void childShouldThrowWhenContainsDelimiter() { MailboxPath path = MailboxPath.forUser(USER, "folder"); assertThatThrownBy(() -> path.child("a.b", '.')) @@ -123,6 +136,14 @@ class MailboxPathTest { } @Test + void getHierarchyLevelsShouldReturnPathWhenBlankName() { + assertThat(MailboxPath.forUser(USER, " ") + .getHierarchyLevels('.')) + .containsExactly( + MailboxPath.forUser(USER, " ")); + } + + @Test void getHierarchyLevelsShouldReturnPathWhenNullName() { assertThat(MailboxPath.forUser(USER, null) .getHierarchyLevels('.')) @@ -208,6 +229,13 @@ class MailboxPathTest { } @Test + void hasEmptyNameInHierarchyShouldBeTrueIfBlankPath() { + assertThat(MailboxPath.forUser(USER, " ") + .hasEmptyNameInHierarchy('.')) + .isTrue(); + } + + @Test void hasEmptyNameInHierarchyShouldBeTrueIfPathWithTwoEmptyNames() { assertThat(MailboxPath.forUser(USER, ".") .hasEmptyNameInHierarchy('.')) @@ -220,6 +248,12 @@ class MailboxPathTest { .hasEmptyNameInHierarchy('.')) .isTrue(); } + @Test + void hasEmptyNameInHierarchyShouldBeTrueIfPathWithABlankNameBetweenTwoNames() { + assertThat(MailboxPath.forUser(USER, "a. .b") + .hasEmptyNameInHierarchy('.')) + .isTrue(); + } @Test void hasEmptyNameInHierarchyShouldBeTrueIfPathWithHeadingEmptyNames() { @@ -229,6 +263,13 @@ class MailboxPathTest { } @Test + void hasEmptyNameInHierarchyShouldBeTrueIfPathWithHeadingBlankName() { + assertThat(MailboxPath.forUser(USER, " .a") + .hasEmptyNameInHierarchy('.')) + .isTrue(); + } + + @Test void hasEmptyNameInHierarchyShouldBeTrueIfPathWithATrailingEmptyName() { assertThat(MailboxPath.forUser(USER, "a.") .hasEmptyNameInHierarchy('.')) @@ -236,11 +277,24 @@ class MailboxPathTest { } @Test + void hasEmptyNameInHierarchyShouldBeTrueIfPathWithATrailingBlankName() { + assertThat(MailboxPath.forUser(USER, "a. ") + .hasEmptyNameInHierarchy('.')) + .isTrue(); + } + + @Test void hasEmptyNameInHierarchyShouldBeTrueIfPathWithTrailingEmptyNames() { assertThat(MailboxPath.forUser(USER, "a..") .hasEmptyNameInHierarchy('.')) .isTrue(); } + @Test + void hasEmptyNameInHierarchyShouldBeTrueIfPathWithTrailingBlankNames() { + assertThat(MailboxPath.forUser(USER, "a. . ") + .hasEmptyNameInHierarchy('.')) + .isTrue(); + } @Test void assertAcceptableShouldThrowOnDoubleSeparator() { diff --git a/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala b/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala index d87adb0..dca59bd 100644 --- a/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala +++ b/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala @@ -332,6 +332,60 @@ trait MailboxSetMethodContract { } @Test + def updateShouldFailWhenModifyingNameToBlank(server: GuiceJamesServer): Unit = { + val mailboxId: MailboxId = server.getProbe(classOf[MailboxProbeImpl]) + .createMailbox(MailboxPath.forUser(BOB, "mailbox")) + val request = + s""" + |{ + | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail" ], + | "methodCalls": [ + | ["Mailbox/set", + | { + | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6", + | "update": { + | "${mailboxId.serialize()}": { + | "name": " " + | } + | } + | }, + | "c1"]] + |} + |""".stripMargin + + val response = `given` + .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER) + .body(request) + .when + .post + .`then` + .log().ifValidationFails() + .statusCode(SC_OK) + .contentType(JSON) + .extract + .body + .asString + + assertThatJson(response).isEqualTo( + s"""{ + | "sessionState": "75128aab4b1b", + | "methodResponses": [ + | ["Mailbox/set", { + | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6", + | "newState": "000001", + | "notUpdated": { + | "${mailboxId.serialize()}": { + | "type": "invalidArguments", + | "description": "'#private:[email protected]: ' has an empty part within its mailbox name considering . as a delimiter", + | "properties":["name"] + | } + | } + | }, "c1"] + | ] + |}""".stripMargin) + } + + @Test def createShouldFailWhenNameWithDelimiter(): Unit = { val request = """ @@ -658,6 +712,119 @@ trait MailboxSetMethodContract { } @Test + def mailboxSetShouldReturnNotCreatedWhenNameIsEmpty(): Unit = { + val request = + """ + |{ + | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail" ], + | "methodCalls": [ + | [ + | "Mailbox/set", + | { + | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6", + | "create": { + | "C42": { + | "name": "" + | } + | } + | }, + | "c1" + | ] + | ] + |} + |""".stripMargin + + val response: String = + `given` + .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER) + .body(request) + .when + .post + .`then` + .log().ifValidationFails() + .statusCode(SC_OK) + .contentType(JSON) + .extract + .body + .asString + + assertThatJson(response).isEqualTo( + s"""{ + | "sessionState": "75128aab4b1b", + | "methodResponses": [[ + | "Mailbox/set", + | { + | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6", + | "newState": "000001", + | "notCreated": { + | "C42": { + | "type": "invalidArguments", + | "description": "'/name' property in mailbox object is not valid: Predicate isEmpty() did not fail." + | } + | } + | }, + | "c1"]] + |}""".stripMargin) + } + + @Test + def mailboxSetShouldReturnNotCreatedWhenNameIsBlank(): Unit = { + val request = + """ + |{ + | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail" ], + | "methodCalls": [ + | [ + | "Mailbox/set", + | { + | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6", + | "create": { + | "C42": { + | "name": " " + | } + | } + | }, + | "c1" + | ] + | ] + |} + |""".stripMargin + + val response: String = + `given` + .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER) + .body(request) + .when + .post + .`then` + .log().ifValidationFails() + .statusCode(SC_OK) + .contentType(JSON) + .extract + .body + .asString + + assertThatJson(response).isEqualTo( + s"""{ + | "sessionState": "75128aab4b1b", + | "methodResponses": [[ + | "Mailbox/set", + | { + | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6", + | "newState": "000001", + | "notCreated": { + | "C42": { + | "type": "invalidArguments", + | "description": "'#private:[email protected]: ' has an empty part within its mailbox name considering . as a delimiter", + | "properties":["name"] + | } + | } + | }, + | "c1"]] + |}""".stripMargin) + } + + @Test def mailboxSetShouldReturnNotCreatedWhenUnknownParameter(): Unit = { val request = """ diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/Mailbox.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/Mailbox.scala index 28b094c..e9f0985 100644 --- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/Mailbox.scala +++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/Mailbox.scala @@ -29,6 +29,7 @@ import org.apache.james.jmap.model.CapabilityIdentifier.CapabilityIdentifier import org.apache.james.jmap.model.UnsignedInt.UnsignedInt import org.apache.james.jmap.model.{CapabilityIdentifier, Properties} import org.apache.james.mailbox.Role +import org.apache.james.mailbox.exception.MailboxNameException import org.apache.james.mailbox.model.MailboxId case class MayReadItems(value: Boolean) extends AnyVal @@ -120,9 +121,9 @@ object MailboxName { type MailboxNameConstraint = NonEmpty type MailboxName = String Refined MailboxNameConstraint - def validate(value: String): Either[IllegalArgumentException, MailboxName] = + def validate(value: String): Either[MailboxNameException, MailboxName] = refined.refineV[MailboxNameConstraint](value) match { - case Left(error) => Left(new IllegalArgumentException(error)) + case Left(error) => Left(new MailboxNameException(error)) case scala.Right(value) => scala.Right(value) } } diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/MailboxFactory.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/MailboxFactory.scala index b18544f..4ed2cfb 100644 --- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/MailboxFactory.scala +++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/MailboxFactory.scala @@ -24,6 +24,7 @@ import org.apache.james.jmap.mail.MailboxName.MailboxName import org.apache.james.jmap.mail._ import org.apache.james.jmap.utils.quotas.QuotaLoader import org.apache.james.mailbox._ +import org.apache.james.mailbox.exception.MailboxNameException import org.apache.james.mailbox.model.MailboxACL.EntryKey import org.apache.james.mailbox.model.{MailboxCounters, MailboxId, MailboxMetaData, MailboxPath, MailboxACL => JavaMailboxACL} import reactor.core.scala.publisher.SMono @@ -32,12 +33,12 @@ import scala.jdk.CollectionConverters._ import scala.jdk.OptionConverters._ object MailboxValidation { - private def retrieveMailboxName(mailboxPath: MailboxPath, pathDelimiter: Char): Either[IllegalArgumentException, MailboxName] = + private def retrieveMailboxName(mailboxPath: MailboxPath, pathDelimiter: Char): Either[MailboxNameException, MailboxName] = mailboxPath.getName .split(pathDelimiter) .lastOption match { case Some(name) => MailboxName.validate(name) - case None => Left(new IllegalArgumentException("No name for the mailbox found")) + case None => Left(new MailboxNameException("No name for the mailbox found")) } def validate(mailboxPath: MailboxPath, --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
