From 3decea2a95a54cf0cdc774bae9c61ab1444c7f35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Victor=20Peter=20Rouven=20M=C3=BCller?= Date: Tue, 3 May 2022 15:28:15 +0200 Subject: [PATCH] Use deriveFormat for ErrorDetailsFormat instead of hand written JsonFormat (#13770) * Use deriveFormat for ErrorDetailsFormat instead of hand written JsonFormat changelog_begin - [HTTP-JSON] The field "@type" was renamed to "type" for encoding the ErrorDetails case changelog_end * Use moar types! :) * Use proper tags, not aliases --- ledger-service/http-json/BUILD.bazel | 2 - .../AbstractHttpServiceIntegrationTest.scala | 7 +- .../http/EndpointsCompanion.scala | 2 +- .../scala/com/digitalasset/http/domain.scala | 29 ++++++- .../digitalasset/http/json/JsonProtocol.scala | 83 +++++-------------- .../http/json/JsonProtocolTest.scala | 30 ++++--- security-evidence.md | 8 +- 7 files changed, 72 insertions(+), 89 deletions(-) diff --git a/ledger-service/http-json/BUILD.bazel b/ledger-service/http-json/BUILD.bazel index 81eb4d5a03..16ae5b3dbc 100644 --- a/ledger-service/http-json/BUILD.bazel +++ b/ledger-service/http-json/BUILD.bazel @@ -254,7 +254,6 @@ daml_compile( "//ledger-service/jwt", "//ledger-service/lf-value-json", "//ledger-service/utils", - "//ledger/error", "//ledger/ledger-api-common", "//libs-scala/contextualized-logging", "//libs-scala/db-utils", @@ -314,7 +313,6 @@ alias( "//ledger-api/rs-grpc-bridge", "//ledger-api/testing-utils", "//ledger-service/fetch-contracts", - "//ledger/error", "//ledger/ledger-resources", "//ledger/metrics", "//ledger/sandbox-common", diff --git a/ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala b/ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala index 6685d8d74a..3c11b03d46 100644 --- a/ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala +++ b/ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala @@ -7,7 +7,6 @@ import java.time.{Instant, LocalDate} import akka.http.scaladsl.Http import akka.http.scaladsl.model._ import com.daml.api.util.TimestampConversion -import com.daml.error.utils.ErrorDetails.{ErrorInfoDetail, RequestInfoDetail, ResourceInfoDetail} import com.daml.lf.data.Ref import com.daml.http.domain.ContractId import com.daml.http.domain.TemplateId.OptionalPkg @@ -715,16 +714,16 @@ abstract class AbstractHttpServiceIntegrationTestTokenIndependent ) import org.scalatest.Inspectors._ forExactly(1, ledgerApiError.details) { - case ErrorInfoDetail(errorCodeId, _) => + case domain.ErrorInfoDetail(errorCodeId, _) => errorCodeId shouldBe "CONTRACT_NOT_FOUND" case _ => fail() } forExactly(1, ledgerApiError.details) { - case RequestInfoDetail(_) => succeed + case domain.RequestInfoDetail(_) => succeed case _ => fail() } forExactly(1, ledgerApiError.details) { - case ResourceInfoDetail(name, typ) => + case domain.ResourceInfoDetail(name, typ) => name shouldBe "000000000000000000000000000000000000000000000000000000000000000000" typ shouldBe "CONTRACT_ID" case _ => fail() diff --git a/ledger-service/http-json/src/main/scala/com/digitalasset/http/EndpointsCompanion.scala b/ledger-service/http-json/src/main/scala/com/digitalasset/http/EndpointsCompanion.scala index 91f5446314..bc3e33521a 100644 --- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/EndpointsCompanion.scala +++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/EndpointsCompanion.scala @@ -277,7 +277,7 @@ object EndpointsCompanion { domain.LedgerApiError( code = grpcStatus.getNumber, message = description, - details = details, + details = details.map(domain.ErrorDetail.fromErrorUtils), ) mkErrorResponse( grpcStatus.asAkkaHttpForJsonApi, diff --git a/ledger-service/http-json/src/main/scala/com/digitalasset/http/domain.scala b/ledger-service/http-json/src/main/scala/com/digitalasset/http/domain.scala index 937279e851..d4791a3447 100644 --- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/domain.scala +++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/domain.scala @@ -23,6 +23,8 @@ import spray.json.JsValue import scala.annotation.tailrec package object domain extends com.daml.fetchcontracts.domain.Aliases { + import scalaz.{@@, Tag} + type InputContractRef[LfV] = (TemplateId.OptionalPkg, LfV) \/ (Option[TemplateId.OptionalPkg], ContractId) @@ -45,11 +47,13 @@ package object domain extends com.daml.fetchcontracts.domain.Aliases { val CommandId = lar.CommandId type LfType = iface.Type + + type RetryInfoDetailDuration = scala.concurrent.duration.Duration @@ RetryInfoDetailDurationTag + val RetryInfoDetailDuration = Tag.of[RetryInfoDetailDurationTag] } package domain { - import com.daml.error.utils.ErrorDetails.ErrorDetail import com.daml.fetchcontracts.domain.`fc domain ErrorOps` trait JwtPayloadTag @@ -563,6 +567,29 @@ package domain { status: StatusCode = StatusCodes.OK, ) extends SyncResponse[R] + sealed trait RetryInfoDetailDurationTag + + sealed trait ErrorDetail extends Product with Serializable + final case class ResourceInfoDetail(name: String, typ: String) extends ErrorDetail + final case class ErrorInfoDetail(errorCodeId: String, metadata: Map[String, String]) + extends ErrorDetail + final case class RetryInfoDetail(duration: RetryInfoDetailDuration) extends ErrorDetail + final case class RequestInfoDetail(correlationId: String) extends ErrorDetail + + object ErrorDetail { + import com.daml.error.utils.ErrorDetails + def fromErrorUtils(errorDetail: ErrorDetails.ErrorDetail): domain.ErrorDetail = + errorDetail match { + case ErrorDetails.ResourceInfoDetail(name, typ) => domain.ResourceInfoDetail(name, typ) + case ErrorDetails.ErrorInfoDetail(errorCodeId, metadata) => + domain.ErrorInfoDetail(errorCodeId, metadata) + case ErrorDetails.RetryInfoDetail(duration) => + domain.RetryInfoDetail(domain.RetryInfoDetailDuration(duration)) + case ErrorDetails.RequestInfoDetail(correlationId) => + domain.RequestInfoDetail(correlationId) + } + } + final case class LedgerApiError( code: Int, message: String, diff --git a/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/JsonProtocol.scala b/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/JsonProtocol.scala index 0157162113..79eccc8340 100644 --- a/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/JsonProtocol.scala +++ b/ledger-service/http-json/src/main/scala/com/digitalasset/http/json/JsonProtocol.scala @@ -4,13 +4,6 @@ package com.daml.http.json import akka.http.scaladsl.model.StatusCode -import com.daml.error.utils.ErrorDetails.{ - ErrorDetail, - ErrorInfoDetail, - RequestInfoDetail, - ResourceInfoDetail, - RetryInfoDetail, -} import com.daml.http.domain import com.daml.http.domain.TemplateId import com.daml.ledger.api.refinements.{ApiTypes => lar} @@ -20,6 +13,7 @@ import scalaz.syntax.std.option._ import scalaz.{-\/, NonEmptyList, OneAnd, \/-} import spray.json._ import spray.json.derived.Discriminator +import scalaz.syntax.tag._ object JsonProtocol extends JsonProtocolLow { @@ -458,66 +452,33 @@ object JsonProtocol extends JsonProtocolLow { implicit def OkResponseFormat[R: JsonFormat]: RootJsonFormat[domain.OkResponse[R]] = jsonFormat3(domain.OkResponse[R]) - implicit val ResourceInfoDetailFormat: RootJsonFormat[ResourceInfoDetail] = jsonFormat2( - ResourceInfoDetail + implicit val ResourceInfoDetailFormat: RootJsonFormat[domain.ResourceInfoDetail] = jsonFormat2( + domain.ResourceInfoDetail ) - implicit val ErrorInfoDetailFormat: RootJsonFormat[ErrorInfoDetail] = jsonFormat2( - ErrorInfoDetail + implicit val ErrorInfoDetailFormat: RootJsonFormat[domain.ErrorInfoDetail] = jsonFormat2( + domain.ErrorInfoDetail ) - implicit val RetryInfoDetailFormat: RootJsonFormat[RetryInfoDetail] = - new RootJsonFormat[RetryInfoDetail] { - override def write(obj: RetryInfoDetail): JsValue = JsObject( - "duration" -> JsNumber(obj.duration.toNanos) - ) - override def read(json: JsValue): RetryInfoDetail = json match { - case JsObject(fields) => - fields - .get("duration") - .collect { case JsNumber(nanos) => - val duration = scala.concurrent.duration.Duration.fromNanos(nanos.toLongExact) - RetryInfoDetail(duration) - } - .getOrElse(deserializationError("Expected field duration of type number")) - case _ => deserializationError("Expected an object with field duration of type number") - } - } + implicit val durationFormat: JsonFormat[domain.RetryInfoDetailDuration] = + jsonFormat[domain.RetryInfoDetailDuration]( + JsonReader.func2Reader( + (LongJsonFormat.read _) + .andThen(scala.concurrent.duration.Duration.fromNanos) + .andThen(it => domain.RetryInfoDetailDuration(it: scala.concurrent.duration.Duration)) + ), + JsonWriter.func2Writer[domain.RetryInfoDetailDuration](duration => + LongJsonFormat.write(duration.unwrap.toNanos) + ), + ) - implicit val RequestInfoDetailFormat: RootJsonFormat[RequestInfoDetail] = jsonFormat1( - RequestInfoDetail + implicit val RetryInfoDetailFormat: RootJsonFormat[domain.RetryInfoDetail] = + jsonFormat1(domain.RetryInfoDetail) + + implicit val RequestInfoDetailFormat: RootJsonFormat[domain.RequestInfoDetail] = jsonFormat1( + domain.RequestInfoDetail ) - implicit val ErrorDetailsFormat: RootJsonFormat[ErrorDetail] = - new RootJsonFormat[ErrorDetail] { - override def write(obj: ErrorDetail): JsValue = { - val (jsValue, name) = obj match { - case a: ResourceInfoDetail => ResourceInfoDetailFormat.write(a) -> "ResourceInfoDetail" - case a: ErrorInfoDetail => ErrorInfoDetailFormat.write(a) -> "ErrorInfoDetail" - case a: RetryInfoDetail => RetryInfoDetailFormat.write(a) -> "RetryInfoDetail" - case a: RequestInfoDetail => RequestInfoDetailFormat.write(a) -> "RequestInfoDetail" - } - val fields = jsValue.asJsObject.fields ++ Map("@type" -> JsString(name)) - JsObject(fields.toList: _*) - } - override def read(json: JsValue): ErrorDetail = { - val obj = json.asJsObject - obj.fields - .get("@type") - .map { - case JsString(value) => value - case value => deserializationError(s"Expected string but got $value") - } - .map { - case "ResourceInfoDetail" => ResourceInfoDetailFormat.read(obj) - case "ErrorInfoDetail" => ErrorInfoDetailFormat.read(obj) - case "RetryInfoDetail" => RetryInfoDetailFormat.read(obj) - case "RequestInfoDetail" => - RequestInfoDetailFormat.read(obj) - case name => - deserializationError(s"Unknown value for @type field: $name") - } - }.getOrElse(deserializationError("Field @type is required for decoding ErrorDetail")) - } + implicit val ErrorDetailsFormat: JsonFormat[domain.ErrorDetail] = deriveFormat[domain.ErrorDetail] implicit val LedgerApiErrorFormat: RootJsonFormat[domain.LedgerApiError] = jsonFormat3(domain.LedgerApiError) diff --git a/ledger-service/http-json/src/test/scala/com/digitalasset/http/json/JsonProtocolTest.scala b/ledger-service/http-json/src/test/scala/com/digitalasset/http/json/JsonProtocolTest.scala index 8800e49556..94baf44eea 100644 --- a/ledger-service/http-json/src/test/scala/com/digitalasset/http/json/JsonProtocolTest.scala +++ b/ledger-service/http-json/src/test/scala/com/digitalasset/http/json/JsonProtocolTest.scala @@ -4,13 +4,6 @@ package com.daml.http.json import akka.http.scaladsl.model.StatusCodes -import com.daml.error.utils.ErrorDetails.{ - ErrorDetail, - ErrorInfoDetail, - RequestInfoDetail, - ResourceInfoDetail, - RetryInfoDetail, -} import com.daml.http.Generators.{ OptionalPackageIdGen, contractGen, @@ -186,24 +179,29 @@ class JsonProtocolTest "ErrorDetail" - { "Encoding and decoding ResourceInfoDetail should result in the same object" in { - val resourceInfoDetail: ErrorDetail = ResourceInfoDetail("test", "test") - resourceInfoDetail shouldBe resourceInfoDetail.toJson.convertTo[ErrorDetail] + val resourceInfoDetail: domain.ErrorDetail = domain.ResourceInfoDetail("test", "test") + resourceInfoDetail shouldBe resourceInfoDetail.toJson.convertTo[domain.ErrorDetail] } "Encoding and decoding RetryInfoDetail should result in the same object" in { - val retryInfoDetail: ErrorDetail = RetryInfoDetail(scala.concurrent.duration.Duration.Zero) - retryInfoDetail shouldBe retryInfoDetail.toJson.convertTo[ErrorDetail] + val retryInfoDetail: domain.ErrorDetail = + domain.RetryInfoDetail( + domain.RetryInfoDetailDuration( + scala.concurrent.duration.Duration.Zero: scala.concurrent.duration.Duration + ) + ) + retryInfoDetail shouldBe retryInfoDetail.toJson.convertTo[domain.ErrorDetail] } "Encoding and decoding RequestInfoDetail should result in the same object" in { - val requestInfoDetail: ErrorDetail = RequestInfoDetail("test") - requestInfoDetail shouldBe requestInfoDetail.toJson.convertTo[ErrorDetail] + val requestInfoDetail: domain.ErrorDetail = domain.RequestInfoDetail("test") + requestInfoDetail shouldBe requestInfoDetail.toJson.convertTo[domain.ErrorDetail] } "Encoding and decoding ErrorInfoDetail should result in the same object" in { - val errorInfoDetail: ErrorDetail = - ErrorInfoDetail("test", Map("test" -> "test1", "test2" -> "test3")) - errorInfoDetail shouldBe errorInfoDetail.toJson.convertTo[ErrorDetail] + val errorInfoDetail: domain.ErrorDetail = + domain.ErrorInfoDetail("test", Map("test" -> "test1", "test2" -> "test3")) + errorInfoDetail shouldBe errorInfoDetail.toJson.convertTo[domain.ErrorDetail] } } diff --git a/security-evidence.md b/security-evidence.md index cb6f47643f..c881bbc48c 100644 --- a/security-evidence.md +++ b/security-evidence.md @@ -5,7 +5,7 @@ - Updating the package service succeeds with sufficient authorization: [AuthorizationTest.scala](ledger-service/http-json/src/it/scala/http/AuthorizationTest.scala#L89) - accept user tokens: [TestMiddleware.scala](triggers/service/auth/src/test/scala/com/daml/auth/middleware/oauth2/TestMiddleware.scala#L152) - badly-authorized create is rejected: [AuthorizationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthorizationSpec.scala#L60) -- badly-authorized create is rejected: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L1069) +- badly-authorized create is rejected: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L1068) - badly-authorized exercise is rejected: [AuthorizationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthorizationSpec.scala#L158) - badly-authorized exercise/create (create is unauthorized) is rejected: [AuthPropagationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthPropagationSpec.scala#L274) - badly-authorized exercise/create (exercise is unauthorized) is rejected: [AuthPropagationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthPropagationSpec.scala#L242) @@ -18,7 +18,7 @@ - create with no signatories is rejected: [AuthorizationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthorizationSpec.scala#L50) - create with non-signatory maintainers is rejected: [AuthorizationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthorizationSpec.scala#L72) - exercise with no controllers is rejected: [AuthorizationSpec.scala](daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthorizationSpec.scala#L148) -- fetch fails when readAs not authed, even if prior fetch succeeded: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L1127) +- fetch fails when readAs not authed, even if prior fetch succeeded: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L1126) - forbid a non-authorized party to check the status of a trigger: [TriggerServiceTest.scala](triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/TriggerServiceTest.scala#L643) - forbid a non-authorized party to list triggers: [TriggerServiceTest.scala](triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/TriggerServiceTest.scala#L633) - forbid a non-authorized party to start a trigger: [TriggerServiceTest.scala](triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/TriggerServiceTest.scala#L622) @@ -26,7 +26,7 @@ - forbid a non-authorized user to upload a DAR: [TriggerServiceTest.scala](triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/TriggerServiceTest.scala#L675) - multiple websocket requests over the same WebSocket connection are NOT allowed: [AbstractWebsocketServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractWebsocketServiceIntegrationTest.scala#L118) - refresh a token after expiry on the server side: [TriggerServiceTest.scala](triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/TriggerServiceTest.scala#L700) -- reject requests with missing auth header: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L541) +- reject requests with missing auth header: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L540) - request a fresh token after expiry on user request: [TriggerServiceTest.scala](triggers/service/src/test/scala/com/digitalasset/daml/lf/engine/trigger/TriggerServiceTest.scala#L685) - return the token from a cookie: [TestMiddleware.scala](triggers/service/auth/src/test/scala/com/daml/auth/middleware/oauth2/TestMiddleware.scala#L96) - return unauthorized on an expired token: [TestMiddleware.scala](triggers/service/auth/src/test/scala/com/daml/auth/middleware/oauth2/TestMiddleware.scala#L139) @@ -202,7 +202,7 @@ ## Performance: - Tail call optimization: Tail recursion does not blow the scala JVM stack.: [TailCallTest.scala](daml-lf/interpreter/src/test/scala/com/digitalasset/daml/lf/speedy/TailCallTest.scala#L16) -- archiving a large number of contracts should succeed: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L1419) +- archiving a large number of contracts should succeed: [AbstractHttpServiceIntegrationTest.scala](ledger-service/http-json/src/itlib/scala/http/AbstractHttpServiceIntegrationTest.scala#L1418) - creating and listing 20K users should be possible: [HttpServiceIntegrationTestUserManagement.scala](ledger-service/http-json/src/it/scala/http/HttpServiceIntegrationTestUserManagement.scala#L562) ## Input Validation: