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 62b20ec9676bf37223bc896eb608d20e233b7117 Author: LanKhuat <[email protected]> AuthorDate: Fri Jul 17 16:46:04 2020 +0700 JAMES-2892 Request level error handling --- .../org/apache/james/jmap/json/Serializer.scala | 4 ++ .../org/apache/james/jmap/model/Capability.scala | 2 + .../apache/james/jmap/model/ProblemDetails.scala | 29 ++++++++ .../james/jmap/model/RequestLevelErrorType.scala | 31 ++++++++ .../org/apache/james/jmap/model/StatusCode.scala | 27 +++++++ .../apache/james/jmap/routes/JMAPApiRoutes.scala | 65 +++++++++++------ .../james/jmap/routes/JMAPApiRoutesTest.scala | 82 ++++++++++++++++++++-- 7 files changed, 212 insertions(+), 28 deletions(-) diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/json/Serializer.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/json/Serializer.scala index b31e0b5..d9e234a 100644 --- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/json/Serializer.scala +++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/json/Serializer.scala @@ -236,12 +236,16 @@ class Serializer @Inject() (mailboxIdFactory: MailboxId.Factory) { private implicit def jsErrorWrites: Writes[JsError] = Json.writes[JsError] + private implicit val problemDetailsWrites: Writes[ProblemDetails] = Json.writes[ProblemDetails] + def serialize(session: Session): JsValue = Json.toJson(session) def serialize(requestObject: RequestObject): JsValue = Json.toJson(requestObject) def serialize(responseObject: ResponseObject): JsValue = Json.toJson(responseObject) + def serialize(problemDetails: ProblemDetails): JsValue = Json.toJson(problemDetails) + def serialize(mailbox: Mailbox)(implicit mailboxWrites: Writes[Mailbox]): JsValue = Json.toJson(mailbox) def serialize(mailboxGetResponse: MailboxGetResponse)(implicit mailboxWrites: Writes[Mailbox]): JsValue = Json.toJson(mailboxGetResponse) diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/Capability.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/Capability.scala index 6fd841c..8eaaefb 100644 --- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/Capability.scala +++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/Capability.scala @@ -34,6 +34,8 @@ object CapabilityIdentifier { val JMAP_MAIL: CapabilityIdentifier = "urn:ietf:params:jmap:mail" val JAMES_QUOTA: CapabilityIdentifier = "urn:apache:james:params:jmap:mail:quota" val JAMES_SHARES: CapabilityIdentifier = "urn:apache:james:params:jmap:mail:shares" + + val SUPPORTED_CAPABILITIES: Set[CapabilityIdentifier] = Set(JMAP_CORE, JMAP_MAIL, JAMES_QUOTA, JAMES_SHARES) } sealed trait CapabilityProperties diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/ProblemDetails.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/ProblemDetails.scala new file mode 100644 index 0000000..f617c86 --- /dev/null +++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/ProblemDetails.scala @@ -0,0 +1,29 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you under the Apache License, Version 2.0 (the * + * "License"); you may not use this file except in compliance * + * with the License. You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, * + * software distributed under the License is distributed on an * + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * + * KIND, either express or implied. See the License for the * + * specific language governing permissions and limitations * + * under the License. * + ****************************************************************/ +package org.apache.james.jmap.model + +import org.apache.james.jmap.model.RequestLevelErrorType.ErrorTypeIdentifier +import org.apache.james.jmap.model.StatusCode.ErrorStatus + +/** + * Problem Details for HTTP APIs within the JMAP context + * https://tools.ietf.org/html/rfc7807 + * see https://jmap.io/spec-core.html#errors + */ +case class ProblemDetails(`type`: ErrorTypeIdentifier, status: ErrorStatus, limit: Option[String], detail: String) diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/RequestLevelErrorType.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/RequestLevelErrorType.scala new file mode 100644 index 0000000..5013a73 --- /dev/null +++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/RequestLevelErrorType.scala @@ -0,0 +1,31 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you under the Apache License, Version 2.0 (the * + * "License"); you may not use this file except in compliance * + * with the License. You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, * + * software distributed under the License is distributed on an * + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * + * KIND, either express or implied. See the License for the * + * specific language governing permissions and limitations * + * under the License. * + ****************************************************************/ +package org.apache.james.jmap.model + +import eu.timepit.refined.api.Refined +import eu.timepit.refined.string.Uri +import eu.timepit.refined.auto._ + +object RequestLevelErrorType { + type ErrorTypeIdentifier = String Refined Uri + val UNKNOWN_CAPABILITY: ErrorTypeIdentifier = "urn:ietf:params:jmap:error:unknownCapability" + val NOT_JSON: ErrorTypeIdentifier = "urn:ietf:params:jmap:error:notJSON" + val NOT_REQUEST: ErrorTypeIdentifier = "urn:ietf:params:jmap:error:notRequest" + val LIMIT: ErrorTypeIdentifier = "urn:ietf:params:jmap:error:limit" +} diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/StatusCode.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/StatusCode.scala new file mode 100644 index 0000000..4de45e7 --- /dev/null +++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/model/StatusCode.scala @@ -0,0 +1,27 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you under the Apache License, Version 2.0 (the * + * "License"); you may not use this file except in compliance * + * with the License. You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, * + * software distributed under the License is distributed on an * + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * + * KIND, either express or implied. See the License for the * + * specific language governing permissions and limitations * + * under the License. * + ****************************************************************/ + +package org.apache.james.jmap.model + +import eu.timepit.refined.api.Refined +import eu.timepit.refined.numeric.Interval.Closed + +object StatusCode { + type ErrorStatus = Int Refined Closed[100, 599] +} diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/JMAPApiRoutes.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/JMAPApiRoutes.scala index 0a89a16..45dfda5 100644 --- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/JMAPApiRoutes.scala +++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/JMAPApiRoutes.scala @@ -23,6 +23,7 @@ import java.nio.charset.StandardCharsets import java.util.stream import java.util.stream.Stream +import com.fasterxml.jackson.core.JsonParseException import eu.timepit.refined.auto._ import io.netty.handler.codec.http.HttpHeaderNames.CONTENT_TYPE import io.netty.handler.codec.http.HttpMethod @@ -38,7 +39,7 @@ import org.apache.james.jmap.json.Serializer import org.apache.james.jmap.method.Method import org.apache.james.jmap.model.CapabilityIdentifier.CapabilityIdentifier import org.apache.james.jmap.model.Invocation.{Arguments, MethodName} -import org.apache.james.jmap.model.{Invocation, RequestObject, ResponseObject} +import org.apache.james.jmap.model._ import org.apache.james.jmap.{Endpoint, JMAPRoute, JMAPRoutes} import org.apache.james.mailbox.MailboxSession import org.slf4j.{Logger, LoggerFactory} @@ -97,26 +98,33 @@ class JMAPApiRoutes (val authenticator: Authenticator, private def parseRequestObject(inputStream: InputStream): SMono[RequestObject] = serializer.deserializeRequestObject(inputStream) match { case JsSuccess(requestObject, _) => SMono.just(requestObject) - case JsError(_) => SMono.raiseError(new IllegalArgumentException("Invalid RequestObject")) + case errors: JsError => SMono.raiseError(new IllegalArgumentException(serializer.serialize(errors).toString())) } private def process(requestObject: RequestObject, httpServerResponse: HttpServerResponse, - mailboxSession: MailboxSession): SMono[Void] = - requestObject - .methodCalls - .map(invocation => this.processMethodWithMatchName(requestObject.using.toSet, invocation, mailboxSession)) - .foldLeft(SFlux.empty[Invocation]) { (flux: SFlux[Invocation], mono: SMono[Invocation]) => flux.mergeWith(mono) } - .collectSeq() - .flatMap((invocations: Seq[Invocation]) => - SMono.fromPublisher(httpServerResponse.status(OK) - .header(CONTENT_TYPE, JSON_CONTENT_TYPE) - .sendString( - SMono.fromCallable(() => - serializer.serialize(ResponseObject(ResponseObject.SESSION_STATE, invocations)).toString), - StandardCharsets.UTF_8 - ).`then`()) - ) + mailboxSession: MailboxSession): SMono[Void] = { + val unsupportedCapabilities = requestObject.using.toSet -- CapabilityIdentifier.SUPPORTED_CAPABILITIES + + if (unsupportedCapabilities.nonEmpty) { + SMono.raiseError(UnsupportedCapabilitiesException(unsupportedCapabilities)) + } else { + requestObject + .methodCalls + .map(invocation => this.processMethodWithMatchName(requestObject.using.toSet, invocation, mailboxSession)) + .foldLeft(SFlux.empty[Invocation]) { (flux: SFlux[Invocation], mono: SMono[Invocation]) => flux.mergeWith(mono) } + .collectSeq() + .flatMap((invocations: Seq[Invocation]) => + SMono.fromPublisher(httpServerResponse.status(OK) + .header(CONTENT_TYPE, JSON_CONTENT_TYPE) + .sendString( + SMono.fromCallable(() => + serializer.serialize(ResponseObject(ResponseObject.SESSION_STATE, invocations)).toString), + StandardCharsets.UTF_8 + ).`then`()) + ) + } + } private def processMethodWithMatchName(capabilities: Set[CapabilityIdentifier], invocation: Invocation, mailboxSession: MailboxSession): SMono[Invocation] = SMono.justOrEmpty(methodsByName.get(invocation.methodName)) @@ -127,11 +135,26 @@ class JMAPApiRoutes (val authenticator: Authenticator, invocation.methodCallId))) private def handleError(throwable: Throwable, httpServerResponse: HttpServerResponse): SMono[Void] = throwable match { - case exception: IllegalArgumentException => SMono.fromPublisher(httpServerResponse.status(SC_BAD_REQUEST) - .header(CONTENT_TYPE, JSON_CONTENT_TYPE) - .sendString(SMono.fromCallable(() => exception.getMessage), StandardCharsets.UTF_8) - .`then`) + case exception: IllegalArgumentException => respondDetails(httpServerResponse, + ProblemDetails(RequestLevelErrorType.NOT_REQUEST, SC_BAD_REQUEST, None, + s"The request was successfully parsed as JSON but did not match the type signature of the Request object: ${exception.getMessage}")) case exception: UnauthorizedException => SMono(handleAuthenticationFailure(httpServerResponse, JMAPApiRoutes.LOGGER, exception)) + case exception: JsonParseException => respondDetails(httpServerResponse, + ProblemDetails(RequestLevelErrorType.NOT_JSON, SC_BAD_REQUEST, None, + s"The content type of the request was not application/json or the request did not parse as I-JSON: ${exception.getMessage}")) + case exception: UnsupportedCapabilitiesException => respondDetails(httpServerResponse, + ProblemDetails(RequestLevelErrorType.UNKNOWN_CAPABILITY, + SC_BAD_REQUEST, None, + s"The request used unsupported capabilities: ${exception.capabilities}")) case _ => SMono.fromPublisher(handleInternalError(httpServerResponse, throwable)) } + + private def respondDetails(httpServerResponse: HttpServerResponse, details: ProblemDetails): SMono[Void] = + SMono.fromPublisher(httpServerResponse.status(SC_BAD_REQUEST) + .header(CONTENT_TYPE, JSON_CONTENT_TYPE) + .sendString(SMono.fromCallable(() => serializer.serialize(details).toString), + StandardCharsets.UTF_8) + .`then`) } + +case class UnsupportedCapabilitiesException(capabilities: Set[CapabilityIdentifier]) extends RuntimeException diff --git a/server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/routes/JMAPApiRoutesTest.scala b/server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/routes/JMAPApiRoutesTest.scala index 37ffb9a..05e4029 100644 --- a/server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/routes/JMAPApiRoutesTest.scala +++ b/server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/routes/JMAPApiRoutesTest.scala @@ -38,6 +38,7 @@ import org.apache.james.jmap._ import org.apache.james.jmap.http.{Authenticator, BasicAuthenticationStrategy} import org.apache.james.jmap.json.Serializer import org.apache.james.jmap.method.{CoreEchoMethod, Method} +import org.apache.james.jmap.model.RequestLevelErrorType import org.apache.james.jmap.routes.JMAPApiRoutesTest._ import org.apache.james.mailbox.MailboxManager import org.apache.james.mailbox.extension.PreDeletionHook @@ -45,6 +46,7 @@ import org.apache.james.mailbox.inmemory.MemoryMailboxManagerProvider import org.apache.james.mailbox.model.TestId import org.apache.james.metrics.tests.RecordingMetricFactory import org.apache.james.user.memory.MemoryUsersRepository +import org.hamcrest.Matchers.equalTo import org.mockito.Mockito.mock import org.scalatest.BeforeAndAfter import org.scalatest.flatspec.AnyFlatSpec @@ -164,6 +166,32 @@ object JMAPApiRoutesTest { | } |} |""".stripMargin + + private val NOT_JSON_REQUEST: String = + """ + |{ + | "using": [ "urn:ietf:params:jmap:core"], + | "methodCalls": { + | "arg1": "arg1data", + |} + |""".stripMargin + + private val UNKNOWN_CAPABILITY_REQUEST: String = + """ + |{ + | "using": [ "urn:ietf:params:jmap:core1"], + | "methodCalls": [ + | [ + | "Core/echo", + | { + | "arg1": "arg1data", + | "arg2": "arg2data" + | }, + | "c1" + | ] + | ] + |} + |""".stripMargin } class JMAPApiRoutesTest extends AnyFlatSpec with BeforeAndAfter with Matchers { @@ -199,7 +227,7 @@ class JMAPApiRoutesTest extends AnyFlatSpec with BeforeAndAfter with Matchers { .headers(headers) .when() .get - .then + .`then` .statusCode(HttpStatus.SC_NOT_FOUND) } @@ -214,7 +242,7 @@ class JMAPApiRoutesTest extends AnyFlatSpec with BeforeAndAfter with Matchers { .headers(headers) .when() .post - .then + .`then` .statusCode(HttpStatus.SC_OK) } @@ -230,7 +258,7 @@ class JMAPApiRoutesTest extends AnyFlatSpec with BeforeAndAfter with Matchers { .body(REQUEST_OBJECT) .when() .post() - .then + .`then` .statusCode(HttpStatus.SC_OK) .contentType(ContentType.JSON) .extract() @@ -253,7 +281,7 @@ class JMAPApiRoutesTest extends AnyFlatSpec with BeforeAndAfter with Matchers { .body(REQUEST_OBJECT_WITH_UNSUPPORTED_METHOD) .when() .post() - .then + .`then` .statusCode(HttpStatus.SC_OK) .contentType(ContentType.JSON) .extract() @@ -274,7 +302,7 @@ class JMAPApiRoutesTest extends AnyFlatSpec with BeforeAndAfter with Matchers { .headers(headers) .when() .get - .then + .`then` .statusCode(HttpStatus.SC_NOT_FOUND) } @@ -288,7 +316,7 @@ class JMAPApiRoutesTest extends AnyFlatSpec with BeforeAndAfter with Matchers { .headers(headers) .when() .post - .then + .`then` .statusCode(HttpStatus.SC_NOT_FOUND) } @@ -303,7 +331,47 @@ class JMAPApiRoutesTest extends AnyFlatSpec with BeforeAndAfter with Matchers { .body(WRONG_OBJECT_REQUEST) .when() .post - .then + .`then` + .statusCode(HttpStatus.SC_BAD_REQUEST) + .body("status", equalTo(400)) + .body("type", equalTo(RequestLevelErrorType.NOT_REQUEST.value)) + .body("detail", equalTo("The request was successfully parsed as JSON but did not match the type signature of the Request object: {\"errors\":[{\"path\":\"obj.methodCalls\",\"messages\":[\"error.expected.jsarray\"]}]}")) + } + + "RFC-8621 version, POST, with not json request body" should "return 400 status" in { + val headers: Headers = Headers.headers( + new Header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER), + new Header("Authorization", s"Basic ${userBase64String}") + ) + RestAssured + .`given`() + .headers(headers) + .body(NOT_JSON_REQUEST) + .when() + .post + .`then` + .statusCode(HttpStatus.SC_BAD_REQUEST) + .body("status", equalTo(400)) + .body("type", equalTo(RequestLevelErrorType.NOT_JSON.value)) + .body("detail", equalTo("The content type of the request was not application/json or the request did not parse as I-JSON: Unexpected character ('}' (code 125)): was expecting double-quote to start field name\n " + + "at [Source: (reactor.netty.ByteBufMono$ReleasingInputStream); line: 6, column: 2]")) + } + + "RFC-8621 version, POST, with unknown capability" should "return 400 status" in { + val headers: Headers = Headers.headers( + new Header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER), + new Header("Authorization", s"Basic ${userBase64String}") + ) + RestAssured + .`given`() + .headers(headers) + .body(UNKNOWN_CAPABILITY_REQUEST) + .when() + .post + .`then` .statusCode(HttpStatus.SC_BAD_REQUEST) + .body("status", equalTo(400)) + .body("type", equalTo(RequestLevelErrorType.UNKNOWN_CAPABILITY.value)) + .body("detail", equalTo("The request used unsupported capabilities: Set(urn:ietf:params:jmap:core1)")) } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
