You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by bt...@apache.org on 2020/08/27 02:24:36 UTC
[james-project] 04/14: JAMES-3359 Mailbox/set update should handle
existing/not found mailboxes
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 76068ab22c014b1ca5dd6a8ea6d65da324c4d499
Author: Benoit Tellier <bt...@linagora.com>
AuthorDate: Thu Aug 20 13:55:17 2020 +0700
JAMES-3359 Mailbox/set update should handle existing/not found mailboxes
---
.../contract/MailboxSetMethodContract.scala | 175 +++++++++++++++++++++
.../org/apache/james/jmap/mail/MailboxGet.scala | 2 +
.../org/apache/james/jmap/mail/MailboxSet.scala | 25 ++-
.../james/jmap/method/MailboxSetMethod.scala | 71 +++++----
4 files changed, 237 insertions(+), 36 deletions(-)
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 07a7ed9..25760d5 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
@@ -6461,4 +6461,179 @@ trait MailboxSetMethodContract {
| ]
|}""".stripMargin)
}
+
+ @Test
+ def updateShouldFailWhenParentIdNotFound(server: GuiceJamesServer): Unit = {
+ val mailboxId: MailboxId = server.getProbe(classOf[MailboxProbeImpl])
+ .createMailbox(MailboxPath.forUser(BOB, "mailbox"))
+ val parentId = randomMailboxId
+ val request = s"""
+ |{
+ | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail" ],
+ | "methodCalls": [
+ | [
+ | "Mailbox/set",
+ | {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "update": {
+ | "${mailboxId.serialize()}": {
+ | "/parentId": "${parentId.serialize()}"
+ | }
+ | }
+ | },
+ | "c2"
+ | ]
+ | ]
+ |}
+ |""".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": "notFound",
+ | "description": "${parentId.serialize()} can not be found"
+ | }
+ | }
+ | }, "c2"]
+ | ]
+ |}""".stripMargin)
+ }
+
+ @Test
+ def updateShouldFailWhenTargetMailboxAlreadyExists(server: GuiceJamesServer): Unit = {
+ val mailboxProbe = server.getProbe(classOf[MailboxProbeImpl])
+ val mailboxId: MailboxId = mailboxProbe.createMailbox(MailboxPath.forUser(BOB, "mailbox"))
+ val parentId = mailboxProbe.createMailbox(MailboxPath.forUser(BOB, "parent"))
+ mailboxProbe.createMailbox(MailboxPath.forUser(BOB, "parent.mailbox"))
+ val request = s"""
+ |{
+ | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail" ],
+ | "methodCalls": [
+ | [
+ | "Mailbox/set",
+ | {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "update": {
+ | "${mailboxId.serialize()}": {
+ | "/parentId": "${parentId.serialize()}"
+ | }
+ | }
+ | },
+ | "c2"
+ | ]
+ | ]
+ |}
+ |""".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": "Mailbox with name=#private:bob@domain.tld:parent.mailbox already exists.",
+ | "properties":["/parentId"]
+ | }
+ | }
+ | }, "c2"]
+ | ]
+ |}""".stripMargin)
+ }
+
+ @Test
+ def updateShouldFailWhenTargetMailboxAlreadyExistsAndBothPropertiesSpecified(server: GuiceJamesServer): Unit = {
+ val mailboxProbe = server.getProbe(classOf[MailboxProbeImpl])
+ val mailboxId: MailboxId = mailboxProbe.createMailbox(MailboxPath.forUser(BOB, "mailbox"))
+ val parentId = mailboxProbe.createMailbox(MailboxPath.forUser(BOB, "parent"))
+ mailboxProbe.createMailbox(MailboxPath.forUser(BOB, "parent.newMailbox"))
+ val request = s"""
+ |{
+ | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail" ],
+ | "methodCalls": [
+ | [
+ | "Mailbox/set",
+ | {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "update": {
+ | "${mailboxId.serialize()}": {
+ | "/parentId": "${parentId.serialize()}",
+ | "/name": "newMailbox"
+ | }
+ | }
+ | },
+ | "c2"
+ | ]
+ | ]
+ |}
+ |""".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
+
+ println(response)
+
+ assertThatJson(response).isEqualTo(
+ s"""{
+ | "sessionState": "75128aab4b1b",
+ | "methodResponses": [
+ | ["Mailbox/set", {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "newState": "000001",
+ | "notUpdated": {
+ | "${mailboxId.serialize()}": {
+ | "type": "invalidArguments",
+ | "description": "Mailbox with name=#private:bob@domain.tld:parent.newMailbox already exists.",
+ | "properties":["/name", "/parentId"]
+ | }
+ | }
+ | }, "c2"]
+ | ]
+ |}""".stripMargin)
+ }
}
diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxGet.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxGet.scala
index 1169652..1fde4e0 100644
--- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxGet.scala
+++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxGet.scala
@@ -28,6 +28,8 @@ case class Ids(value: List[UnparsedMailboxId])
case class Properties(value: List[NonEmptyString]) {
def asSetOfString: Set[String] = value.map(_.toString()).toSet
+
+ def intersect(properties: Properties): Properties = Properties(value.toSet.intersect(properties.value.toSet).toList)
}
diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxSet.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxSet.scala
index 6c9ff08..3bc5fac 100644
--- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxSet.scala
+++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxSet.scala
@@ -26,6 +26,7 @@ import eu.timepit.refined.boolean.And
import eu.timepit.refined.collection.NonEmpty
import eu.timepit.refined.refineV
import eu.timepit.refined.string.StartsWith
+import eu.timepit.refined.types.string.NonEmptyString
import org.apache.james.core.Username
import org.apache.james.jmap.json.Serializer
import org.apache.james.jmap.mail.MailboxName.MailboxName
@@ -90,7 +91,7 @@ case class MailboxPatchObject(value: Map[String, JsValue]) {
def validate(processingContext: ProcessingContext,
mailboxIdFactory: MailboxId.Factory,
serializer: Serializer,
- capabilities: Set[CapabilityIdentifier]): Either[PatchUpdateValidationException, ValidatedMailboxPathObject] = {
+ capabilities: Set[CapabilityIdentifier]): Either[PatchUpdateValidationException, ValidatedMailboxPatchObject] = {
val asUpdatedIterable = updates(serializer, capabilities, processingContext, mailboxIdFactory)
val maybeParseException: Option[PatchUpdateValidationException] = asUpdatedIterable
@@ -136,7 +137,7 @@ case class MailboxPatchObject(value: Map[String, JsValue]) {
maybeParseException
.orElse(bothPartialAndResetRights)
.map(e => Left(e))
- .getOrElse(scala.Right(ValidatedMailboxPathObject(
+ .getOrElse(scala.Right(ValidatedMailboxPatchObject(
nameUpdate = nameUpdate,
parentIdUpdate = parentIdUpdate,
isSubscribedUpdate = isSubscribedUpdate,
@@ -172,12 +173,22 @@ case class MailboxPatchObject(value: Map[String, JsValue]) {
}
}
-case class ValidatedMailboxPathObject(nameUpdate: Option[NameUpdate],
- parentIdUpdate: Option[ParentIdUpdate],
- isSubscribedUpdate: Option[IsSubscribedUpdate],
- rightsReset: Option[SharedWithResetUpdate],
- rightsPartialUpdates: Seq[SharedWithPartialUpdate]) {
+object ValidatedMailboxPatchObject {
+ val nameProperty: NonEmptyString = "/name"
+ val parentIdProperty: NonEmptyString = "/parentId"
+}
+
+case class ValidatedMailboxPatchObject(nameUpdate: Option[NameUpdate],
+ parentIdUpdate: Option[ParentIdUpdate],
+ isSubscribedUpdate: Option[IsSubscribedUpdate],
+ rightsReset: Option[SharedWithResetUpdate],
+ rightsPartialUpdates: Seq[SharedWithPartialUpdate]) {
val shouldUpdateMailboxPath: Boolean = nameUpdate.isDefined || parentIdUpdate.isDefined
+
+ val updatedProperties: Properties = Properties(List(
+ nameUpdate.map(_ => ValidatedMailboxPatchObject.nameProperty),
+ parentIdUpdate.map(_ => ValidatedMailboxPatchObject.parentIdProperty))
+ .flatMap(_.toList))
}
case class MailboxSetResponse(accountId: AccountId,
diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MailboxSetMethod.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MailboxSetMethod.scala
index 3e85d105..2a22946 100644
--- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MailboxSetMethod.scala
+++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MailboxSetMethod.scala
@@ -23,7 +23,7 @@ import eu.timepit.refined.auto._
import javax.inject.Inject
import org.apache.james.jmap.json.Serializer
import org.apache.james.jmap.mail.MailboxSetRequest.{MailboxCreationId, UnparsedMailboxId}
-import org.apache.james.jmap.mail.{InvalidPatchException, InvalidPropertyException, InvalidUpdateException, IsSubscribed, MailboxCreationRequest, MailboxCreationResponse, MailboxPatchObject, MailboxRights, MailboxSetError, MailboxSetRequest, MailboxSetResponse, MailboxUpdateResponse, NameUpdate, ParentIdUpdate, Properties, RemoveEmailsOnDestroy, ServerSetPropertyException, SetErrorDescription, SortOrder, TotalEmails, TotalThreads, UnreadEmails, UnreadThreads, UnsupportedPropertyUpdatedEx [...]
+import org.apache.james.jmap.mail.{InvalidPatchException, InvalidPropertyException, InvalidUpdateException, IsSubscribed, MailboxCreationRequest, MailboxCreationResponse, MailboxPatchObject, MailboxRights, MailboxSetError, MailboxSetRequest, MailboxSetResponse, MailboxUpdateResponse, NameUpdate, ParentIdUpdate, Properties, RemoveEmailsOnDestroy, ServerSetPropertyException, SetErrorDescription, SortOrder, TotalEmails, TotalThreads, UnreadEmails, UnreadThreads, UnsupportedPropertyUpdatedEx [...]
import org.apache.james.jmap.model.CapabilityIdentifier.CapabilityIdentifier
import org.apache.james.jmap.model.Invocation.{Arguments, MethodName}
import org.apache.james.jmap.model.{ClientId, Id, Invocation, ServerId, State}
@@ -106,17 +106,21 @@ case class DeletionResults(results: Seq[DeletionResult]) {
sealed trait UpdateResult
case class UpdateSuccess(mailboxId: MailboxId) extends UpdateResult
-case class UpdateFailure(mailboxId: UnparsedMailboxId, exception: Throwable) extends UpdateResult {
+case class UpdateFailure(mailboxId: UnparsedMailboxId, exception: Throwable, patch: Option[ValidatedMailboxPatchObject]) extends UpdateResult {
+ def filter(acceptableProperties: Properties): Option[Properties] = Some(patch
+ .map(_.updatedProperties.intersect(acceptableProperties))
+ .getOrElse(acceptableProperties))
+
def asMailboxSetError: MailboxSetError = exception match {
case e: MailboxNotFoundException => MailboxSetError.notFound(Some(SetErrorDescription(e.getMessage)))
- case e: MailboxNameException => MailboxSetError.invalidArgument(Some(SetErrorDescription(e.getMessage)), Some(Properties(List("/name"))))
- case e: MailboxExistsException => MailboxSetError.invalidArgument(Some(SetErrorDescription(e.getMessage)), Some(Properties(List("/name"))))
+ case e: MailboxNameException => MailboxSetError.invalidArgument(Some(SetErrorDescription(e.getMessage)), filter(Properties(List("/name", "/parentId"))))
+ case e: MailboxExistsException => MailboxSetError.invalidArgument(Some(SetErrorDescription(e.getMessage)), filter(Properties(List("/name", "/parentId"))))
case e: UnsupportedPropertyUpdatedException => MailboxSetError.invalidArgument(Some(SetErrorDescription(s"${e.property} property do not exist thus cannot be updated")), Some(Properties(List(e.property))))
case e: InvalidUpdateException => MailboxSetError.invalidArgument(Some(SetErrorDescription(s"${e.cause}")), Some(Properties(List(e.property))))
case e: ServerSetPropertyException => MailboxSetError.invalidArgument(Some(SetErrorDescription("Can not modify server-set properties")), Some(Properties(List(e.property))))
case e: InvalidPropertyException => MailboxSetError.invalidPatch(Some(SetErrorDescription(s"${e.cause}")))
case e: InvalidPatchException => MailboxSetError.invalidPatch(Some(SetErrorDescription(s"${e.cause}")))
- case e: SystemMailboxChangeException => MailboxSetError.invalidArgument(Some(SetErrorDescription("Invalid change to a system mailbox")), Some(Properties(List("/name"))))
+ case e: SystemMailboxChangeException => MailboxSetError.invalidArgument(Some(SetErrorDescription("Invalid change to a system mailbox")), filter(Properties(List("/name", "parentId"))))
case e: InsufficientRightsException => MailboxSetError.invalidArgument(Some(SetErrorDescription("Invalid change to a delegated mailbox")), None)
case _ => MailboxSetError.serverFail(Some(SetErrorDescription(exception.getMessage)), None)
}
@@ -161,9 +165,9 @@ class MailboxSetMethod @Inject()(serializer: Serializer,
.flatMap({
case (unparsedMailboxId: UnparsedMailboxId, patch: MailboxPatchObject) =>
processingContext.resolveMailboxId(unparsedMailboxId, mailboxIdFactory).fold(
- e => SMono.just(UpdateFailure(unparsedMailboxId, e)),
- mailboxId => updateMailbox(mailboxSession, processingContext, mailboxId, patch, capabilities))
- .onErrorResume(e => SMono.just(UpdateFailure(unparsedMailboxId, e)))
+ e => SMono.just(UpdateFailure(unparsedMailboxId, e, None)),
+ mailboxId => updateMailbox(mailboxSession, processingContext, mailboxId, unparsedMailboxId, patch, capabilities))
+ .onErrorResume(e => SMono.just(UpdateFailure(unparsedMailboxId, e, None)))
})
.collectSeq()
.map(UpdateResults)
@@ -172,16 +176,17 @@ class MailboxSetMethod @Inject()(serializer: Serializer,
private def updateMailbox(mailboxSession: MailboxSession,
processingContext: ProcessingContext,
mailboxId: MailboxId,
+ unparsedMailboxId: UnparsedMailboxId,
patch: MailboxPatchObject,
capabilities: Set[CapabilityIdentifier]): SMono[UpdateResult] = {
patch.validate(processingContext, mailboxIdFactory, serializer, capabilities)
.fold(e => SMono.raiseError(e), validatedPatch =>
- updateMailboxPath(mailboxId, validatedPatch, mailboxSession)
- .`then`(updateMailboxRights(mailboxId, validatedPatch, mailboxSession))
- .`then`(updateSubscription(mailboxId, validatedPatch, mailboxSession)))
+ updateMailboxRights(mailboxId, validatedPatch, mailboxSession)
+ .`then`(updateSubscription(mailboxId, validatedPatch, mailboxSession))
+ .`then`(updateMailboxPath(mailboxId, unparsedMailboxId, validatedPatch, mailboxSession)))
}
- private def updateSubscription(mailboxId: MailboxId, validatedPatch: ValidatedMailboxPathObject, mailboxSession: MailboxSession): SMono[UpdateResult] = {
+ private def updateSubscription(mailboxId: MailboxId, validatedPatch: ValidatedMailboxPatchObject, mailboxSession: MailboxSession): SMono[UpdateResult] = {
validatedPatch.isSubscribedUpdate.map(isSubscribedUpdate => {
SMono.fromCallable(() => {
val mailbox = mailboxManager.getMailbox(mailboxId, mailboxSession)
@@ -199,24 +204,32 @@ class MailboxSetMethod @Inject()(serializer: Serializer,
.getOrElse(SMono.just[UpdateResult](UpdateSuccess(mailboxId)))
}
- private def updateMailboxPath(mailboxId: MailboxId, validatedPatch: ValidatedMailboxPathObject, mailboxSession: MailboxSession): SMono[UpdateResult] = {
+ private def updateMailboxPath(mailboxId: MailboxId,
+ unparsedMailboxId: UnparsedMailboxId,
+ validatedPatch: ValidatedMailboxPatchObject,
+ mailboxSession: MailboxSession): SMono[UpdateResult] = {
if (validatedPatch.shouldUpdateMailboxPath) {
- SMono.fromCallable(() => {
- val mailbox = mailboxManager.getMailbox(mailboxId, mailboxSession)
- if (isASystemMailbox(mailbox)) {
- throw SystemMailboxChangeException(mailboxId)
+ SMono.fromCallable[UpdateResult](() => {
+ try {
+ val mailbox = mailboxManager.getMailbox(mailboxId, mailboxSession)
+ if (isASystemMailbox(mailbox)) {
+ throw SystemMailboxChangeException(mailboxId)
+ }
+ val oldPath = mailbox.getMailboxPath
+ val newPath = applyNameUpdate(validatedPatch.nameUpdate, mailboxSession)
+ .andThen(applyParentIdUpdate(validatedPatch.parentIdUpdate, mailboxSession))
+ .apply(oldPath)
+ if (!oldPath.equals(newPath)) {
+ mailboxManager.renameMailbox(mailboxId,
+ newPath,
+ RenameOption.RENAME_SUBSCRIPTIONS,
+ mailboxSession)
+ }
+ UpdateSuccess(mailboxId)
+ } catch {
+ case e: Exception => UpdateFailure(unparsedMailboxId, e, Some(validatedPatch))
}
- val oldPath = mailbox.getMailboxPath
- val newPath = applyNameUpdate(validatedPatch.nameUpdate, mailboxSession)
- .andThen(applyParentIdUpdate(validatedPatch.parentIdUpdate, mailboxSession))
- .apply(oldPath)
- if (!oldPath.equals(newPath)) {
- mailboxManager.renameMailbox(mailboxId,
- newPath,
- RenameOption.RENAME_SUBSCRIPTIONS,
- mailboxSession)
- }
- }).`then`(SMono.just[UpdateResult](UpdateSuccess(mailboxId)))
+ })
.subscribeOn(Schedulers.elastic())
} else {
SMono.just[UpdateResult](UpdateSuccess(mailboxId))
@@ -253,7 +266,7 @@ class MailboxSetMethod @Inject()(serializer: Serializer,
}
private def updateMailboxRights(mailboxId: MailboxId,
- validatedPatch: ValidatedMailboxPathObject,
+ validatedPatch: ValidatedMailboxPatchObject,
mailboxSession: MailboxSession): SMono[UpdateResult] = {
val resetOperation: SMono[Unit] = validatedPatch.rightsReset.map(sharedWithResetUpdate => {
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org