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 83e047842eaf52c3d83983ccb985b43778691b79 Author: LanKhuat <dlkh...@linagora.com> AuthorDate: Wed Aug 19 10:44:41 2020 +0700 JAMES-3359 Mailbox/set partial rights update validation --- .../contract/MailboxSetMethodContract.scala | 188 +++++++++++++++++++++ .../org/apache/james/jmap/mail/MailboxSet.scala | 57 ++++++- .../james/jmap/method/MailboxSetMethod.scala | 87 ++++------ 3 files changed, 272 insertions(+), 60 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 b18f8db..90828ee 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 @@ -5167,4 +5167,192 @@ trait MailboxSetMethodContract { | ] |}""".stripMargin) } + + // TODO testing delegation & partial updates + // TODO write test for rights store manager applyCommandById in MailboxManagerTest + + @Test + def partialRightsUpdateShouldFailWhenInvalidUsername(server: GuiceJamesServer): Unit = { + val path = MailboxPath.forUser(BOB, "mailbox") + val mailboxId = server.getProbe(classOf[MailboxProbeImpl]).createMailbox(path) + + val request = + s""" + |{ + | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail", "urn:apache:james:params:jmap:mail:shares" ], + | "methodCalls": [ + | [ + | "Mailbox/set", + | { + | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6", + | "update": { + | "${mailboxId.serialize()}": { + | "/sharedWith/invalid@invalid@${DOMAIN.asString()}": ["r", "l"] + | } + | } + | }, + | "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": "invalidPatch", + | "description": "The username should not contain multiple domain delimiter. Value: invalid@inva...@domain.tld" + | } + | } + | }, "c1"] + | ] + |}""".stripMargin) + } + + @Test + def partialRightsUpdateShouldFailWhenInvalidRights(server: GuiceJamesServer): Unit = { + val path = MailboxPath.forUser(BOB, "mailbox") + val mailboxId = server.getProbe(classOf[MailboxProbeImpl]).createMailbox(path) + + val request = + s""" + |{ + | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail", "urn:apache:james:params:jmap:mail:shares" ], + | "methodCalls": [ + | [ + | "Mailbox/set", + | { + | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6", + | "update": { + | "${mailboxId.serialize()}": { + | "/sharedWith/${ANDRE.asString()}": ["p"] + | } + | } + | }, + | "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": "Specified value do not match the expected JSON format: List(((0),List(JsonValidationError(List(Unknown right 'p'),ArraySeq()))))", + | "properties": [ + | "/sharedWith/an...@domain.tld" + | ] + | } + | } + | }, + | "c1" + | ] + | ] + |}""".stripMargin) + } + + @Test + def rightsUpdateShouldFailWhenBothPartialAndReset(server: GuiceJamesServer): Unit = { + val path = MailboxPath.forUser(BOB, "mailbox") + val mailboxId = server.getProbe(classOf[MailboxProbeImpl]).createMailbox(path) + + val request = + s""" + |{ + | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail", "urn:apache:james:params:jmap:mail:shares" ], + | "methodCalls": [ + | [ + | "Mailbox/set", + | { + | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6", + | "update": { + | "${mailboxId.serialize()}": { + | "/sharedWith": { + | "${DAVID.asString()}":["r", "l", "w"] + | }, + | "/sharedWith/${ANDRE.asString()}": ["r"] + | } + | } + | }, + | "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": "invalidPatch", + | "description": "Resetting rights and partial updates cannot be done in the same method call" + | } + | } + | }, + | "c1" + | ] + | ] + |}""".stripMargin) + } } 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 e5e5525..267fb4d 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 @@ -79,7 +79,54 @@ object MailboxPatchObject { } case class MailboxPatchObject(value: Map[String, JsValue]) { - def updates(serializer: Serializer): Iterable[Either[PatchUpdateValidationException, Update]] = value.map({ + def validate(serializer: Serializer): Either[PatchUpdateValidationException, ValidatedMailboxPathObject] = { + val asUpdatedIterable = updates(serializer) + + val maybeParseException: Option[PatchUpdateValidationException] = asUpdatedIterable + .flatMap(x => x match { + case Left(e) => Some(e) + case _ => None + }).headOption + + val nameUpdate: Option[NameUpdate] = asUpdatedIterable + .flatMap(x => x match { + case scala.Right(NameUpdate(newName)) => Some(NameUpdate(newName)) + case _ => None + }).headOption + + val isSubscribedUpdate: Option[IsSubscribedUpdate] = asUpdatedIterable + .flatMap(x => x match { + case scala.Right(IsSubscribedUpdate(isSubscribed)) => Some(IsSubscribedUpdate(isSubscribed)) + case _ => None + }).headOption + + val rightsReset: Option[SharedWithResetUpdate] = asUpdatedIterable + .flatMap(x => x match { + case scala.Right(SharedWithResetUpdate(rights)) => Some(SharedWithResetUpdate(rights)) + case _ => None + }).headOption + + val partialRightsUpdates: Seq[SharedWithPartialUpdate] = asUpdatedIterable.flatMap(x => x match { + case scala.Right(SharedWithPartialUpdate(username, rights)) => Some(SharedWithPartialUpdate(username, rights)) + case _ => None + }).toSeq + + val bothPartialAndResetRights: Option[PatchUpdateValidationException] = if (rightsReset.isDefined && partialRightsUpdates.nonEmpty) { + Some(InvalidPatchException("Resetting rights and partial updates cannot be done in the same method call")) + } else { + None + } + maybeParseException + .orElse(bothPartialAndResetRights) + .map(e => Left(e)) + .getOrElse(scala.Right(ValidatedMailboxPathObject( + nameUpdate = nameUpdate, + isSubscribedUpdate = isSubscribedUpdate, + rightsReset = rightsReset, + rightsPartialUpdates = partialRightsUpdates))) + } + + private def updates(serializer: Serializer): Iterable[Either[PatchUpdateValidationException, Update]] = value.map({ case (property, newValue) => property match { case "/name" => NameUpdate.parse(newValue) case "/sharedWith" => SharedWithResetUpdate.parse(newValue, serializer) @@ -103,6 +150,11 @@ case class MailboxPatchObject(value: Map[String, JsValue]) { }) } +case class ValidatedMailboxPathObject(nameUpdate: Option[NameUpdate], + isSubscribedUpdate: Option[IsSubscribedUpdate], + rightsReset: Option[SharedWithResetUpdate], + rightsPartialUpdates: Seq[SharedWithPartialUpdate]) + case class MailboxSetResponse(accountId: AccountId, oldState: Option[State], newState: State, @@ -224,4 +276,5 @@ case class UnsupportedPropertyUpdatedException(property: MailboxPatchObjectKey) case class InvalidPropertyUpdatedException(property: MailboxPatchObjectKey) extends PatchUpdateValidationException case class InvalidPropertyException(property: String, cause: String) extends PatchUpdateValidationException case class InvalidUpdateException(property: MailboxPatchObjectKey, cause: String) extends PatchUpdateValidationException -case class ServerSetPropertyException(property: MailboxPatchObjectKey) extends PatchUpdateValidationException \ No newline at end of file +case class ServerSetPropertyException(property: MailboxPatchObjectKey) extends PatchUpdateValidationException +case class InvalidPatchException(cause: String) extends PatchUpdateValidationException 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 2f523aa..46ad2d7 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.{InvalidPropertyException, InvalidUpdateException, IsSubscribed, IsSubscribedUpdate, MailboxCreationRequest, MailboxCreationResponse, MailboxPatchObject, MailboxRights, MailboxSetError, MailboxSetRequest, MailboxSetResponse, MailboxUpdateResponse, NameUpdate, PatchUpdateValidationException, Properties, RemoveEmailsOnDestroy, ServerSetPropertyException, SetErrorDescription, SharedWithPartialUpdate, SharedWithResetUpdate, SortOrder, TotalEmails, TotalThrea [...] +import org.apache.james.jmap.mail.{InvalidPatchException, InvalidPropertyException, InvalidUpdateException, IsSubscribed, IsSubscribedUpdate, MailboxCreationRequest, MailboxCreationResponse, MailboxPatchObject, MailboxRights, MailboxSetError, MailboxSetRequest, MailboxSetResponse, MailboxUpdateResponse, NameUpdate, PatchUpdateValidationException, Properties, RemoveEmailsOnDestroy, ServerSetPropertyException, SetErrorDescription, SharedWithPartialUpdate, SharedWithResetUpdate, SortOrder, [...] 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} @@ -101,7 +101,7 @@ case class DeletionResults(results: Seq[DeletionResult]) { case failure: DeletionFailure => Some(failure.mailboxId, failure.asMailboxSetError) case _ => None }) - .toMap + .toMap } sealed trait UpdateResult @@ -115,6 +115,7 @@ case class UpdateFailure(mailboxId: UnparsedMailboxId, exception: Throwable) ext 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: InsufficientRightsException => MailboxSetError.invalidArgument(Some(SetErrorDescription("Invalid change to a delegated mailbox")), None) case _ => MailboxSetError.serverFail(Some(SetErrorDescription(exception.getMessage)), None) @@ -159,8 +160,8 @@ 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, mailboxId, patch)) + e => SMono.just(UpdateFailure(unparsedMailboxId, e)), + mailboxId => updateMailbox(mailboxSession, mailboxId, patch)) .onErrorResume(e => SMono.just(UpdateFailure(unparsedMailboxId, e))) }) .collectSeq() @@ -168,46 +169,17 @@ class MailboxSetMethod @Inject()(serializer: Serializer, } private def updateMailbox(mailboxSession: MailboxSession, - maiboxId: MailboxId, + mailboxId: MailboxId, patch: MailboxPatchObject): SMono[UpdateResult] = { - val updates = patch.updates(serializer) - val maybeParseException: Option[PatchUpdateValidationException] = updates - .flatMap(x => x match { - case Left(e) => Some(e) - case _ => None - }).headOption - - val maybeNameUpdate: Option[NameUpdate] = updates - .flatMap(x => x match { - case Right(NameUpdate(newName)) => Some(NameUpdate(newName)) - case _ => None - }).headOption - - val maybeSharedWithResetUpdate: Option[SharedWithResetUpdate] = updates - .flatMap(x => x match { - case Right(SharedWithResetUpdate(rights)) => Some(SharedWithResetUpdate(rights)) - case _ => None - }).headOption - - val maybeIsSubscribedUpdate: Option[IsSubscribedUpdate] = updates - .flatMap(x => x match { - case Right(IsSubscribedUpdate(isSubscribed)) => Some(IsSubscribedUpdate(isSubscribed)) - case _ => None - }).headOption - - val partialRightsUpdates: Seq[SharedWithPartialUpdate] = updates.flatMap(x => x match { - case Right(SharedWithPartialUpdate(username, rights)) => Some(SharedWithPartialUpdate(username, rights)) - case _ => None - }).toSeq - - maybeParseException.map(e => SMono.raiseError[UpdateResult](e)) - .getOrElse(updateMailboxPath(maiboxId, maybeNameUpdate, mailboxSession) - .`then`(updateMailboxRights(maiboxId, maybeSharedWithResetUpdate, partialRightsUpdates, mailboxSession)) - .`then`(updateSubscription(maiboxId, maybeIsSubscribedUpdate, mailboxSession))) + patch.validate(serializer) + .fold(e => SMono.raiseError(e), validatedPatch => + updateMailboxPath(mailboxId, validatedPatch, mailboxSession) + .`then`(updateMailboxRights(mailboxId, validatedPatch, mailboxSession)) + .`then`(updateSubscription(mailboxId, validatedPatch, mailboxSession))) } - private def updateSubscription(mailboxId: MailboxId, maybeIsSubscribedUpdate: Option[IsSubscribedUpdate], mailboxSession: MailboxSession): SMono[UpdateResult] = { - maybeIsSubscribedUpdate.map(isSubscribedUpdate => { + private def updateSubscription(mailboxId: MailboxId, validatedPatch: ValidatedMailboxPathObject, mailboxSession: MailboxSession): SMono[UpdateResult] = { + validatedPatch.isSubscribedUpdate.map(isSubscribedUpdate => { SMono.fromCallable(() => { val mailbox = mailboxManager.getMailbox(mailboxId, mailboxSession) val isOwner = mailbox.getMailboxPath.belongsTo(mailboxSession) @@ -224,8 +196,8 @@ class MailboxSetMethod @Inject()(serializer: Serializer, .getOrElse(SMono.just[UpdateResult](UpdateSuccess(mailboxId))) } - private def updateMailboxPath(mailboxId: MailboxId, maybeNameUpdate: Option[NameUpdate], mailboxSession: MailboxSession): SMono[UpdateResult] = { - maybeNameUpdate.map(nameUpdate => { + private def updateMailboxPath(mailboxId: MailboxId, validatedPatch: ValidatedMailboxPathObject, mailboxSession: MailboxSession): SMono[UpdateResult] = { + validatedPatch.nameUpdate.map(nameUpdate => { SMono.fromCallable(() => { val mailbox = mailboxManager.getMailbox(mailboxId, mailboxSession) if (isASystemMailbox(mailbox)) { @@ -243,17 +215,16 @@ class MailboxSetMethod @Inject()(serializer: Serializer, } private def updateMailboxRights(mailboxId: MailboxId, - maybeSharedWithResetUpdate: Option[SharedWithResetUpdate], - partialUpdates: Seq[SharedWithPartialUpdate], + validatedPatch: ValidatedMailboxPathObject, mailboxSession: MailboxSession): SMono[UpdateResult] = { - val resetOperation: SMono[Unit] = maybeSharedWithResetUpdate.map(sharedWithResetUpdate => { + val resetOperation: SMono[Unit] = validatedPatch.rightsReset.map(sharedWithResetUpdate => { SMono.fromCallable(() => { mailboxManager.setRights(mailboxId, sharedWithResetUpdate.rights.toMailboxAcl.asJava, mailboxSession) }).`then`() }).getOrElse(SMono.empty) - val partialUpdatesOperation: SMono[Unit] = SFlux.fromIterable(partialUpdates) + val partialUpdatesOperation: SMono[Unit] = SFlux.fromIterable(validatedPatch.rightsPartialUpdates) .flatMap(partialUpdate => SMono.fromCallable(() => { mailboxManager.applyRightsCommand(mailboxId, partialUpdate.asACLCommand(), mailboxSession) })) @@ -327,11 +298,11 @@ class MailboxSetMethod @Inject()(serializer: Serializer, .getOrElse(Map.empty) .view) .flatMap { - case (mailboxCreationId: MailboxCreationId, jsObject: JsObject) => - SMono.fromCallable(() => { - createMailbox(mailboxSession, mailboxCreationId, jsObject, processingContext) - }).subscribeOn(Schedulers.elastic()) - } + case (mailboxCreationId: MailboxCreationId, jsObject: JsObject) => + SMono.fromCallable(() => { + createMailbox(mailboxSession, mailboxCreationId, jsObject, processingContext) + }).subscribeOn(Schedulers.elastic()) + } .collectSeq() .map(CreationResults) } @@ -379,7 +350,7 @@ class MailboxSetMethod @Inject()(serializer: Serializer, } mailboxCreationRequest.rights - .foreach(rights => mailboxManager.setRights(mailboxId, rights.toMailboxAcl.asJava, mailboxSession)) + .foreach(rights => mailboxManager.setRights(mailboxId, rights.toMailboxAcl.asJava, mailboxSession)) val quotas = quotaFactory.loadFor(mailboxSession) .flatMap(quotaLoader => quotaLoader.getQuotas(path)) @@ -433,10 +404,10 @@ class MailboxSetMethod @Inject()(serializer: Serializer, } private def retrievePath(mailboxId: MailboxId, mailboxSession: MailboxSession): Either[Exception, MailboxPath] = try { - Right(mailboxManager.getMailbox(mailboxId, mailboxSession).getMailboxPath) - } catch { - case e: Exception => Left(e) - } + Right(mailboxManager.getMailbox(mailboxId, mailboxSession).getMailboxPath) + } catch { + case e: Exception => Left(e) + } private def createResponse(capabilities: Set[CapabilityIdentifier], invocation: Invocation, @@ -454,7 +425,7 @@ class MailboxSetMethod @Inject()(serializer: Serializer, updated = Some(updateResults.updated).filter(_.nonEmpty), notUpdated = Some(updateResults.notUpdated).filter(_.nonEmpty), notDestroyed = Some(deletionResults.retrieveErrors).filter(_.nonEmpty)) - + Invocation(methodName, Arguments(serializer.serialize(response, capabilities).as[JsObject]), invocation.methodCallId) --------------------------------------------------------------------- To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org