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 <[email protected]> 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:[email protected]: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:[email protected]: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: [email protected] For additional commands, e-mail: [email protected]
