From 88aa95562a0284781e17d470102e7c31c43a95cf Mon Sep 17 00:00:00 2001 From: "Saqib@Ee" Date: Fri, 16 Feb 2024 13:43:26 +0000 Subject: [PATCH 1/2] CIR-1451: Don't give unnecessary details of json parse failures when json payload is malformed. --- app/controllers/AddressSearchController.scala | 24 ++++++++++++------- app/model/response.scala | 20 ++++++++++++++++ it/test/suites/PostcodeLookupSuiteV2.scala | 15 +++++++++++- 3 files changed, 49 insertions(+), 10 deletions(-) diff --git a/app/controllers/AddressSearchController.scala b/app/controllers/AddressSearchController.scala index 0780ddf..39992c2 100644 --- a/app/controllers/AddressSearchController.scala +++ b/app/controllers/AddressSearchController.scala @@ -22,7 +22,7 @@ import model._ import model.address.{AddressRecord, Postcode} import model.internal.NonUKAddress import model.request.{LookupByCountryRequest, LookupByPostTownRequest, LookupByPostcodeRequest, LookupByUprnRequest} -import model.response.SupportedCountryCodes +import model.response.{ErrorResponse, SupportedCountryCodes} import play.api.libs.json.{JsError, JsSuccess, Json} import play.api.mvc._ import repositories.{ABPAddressRepository, NonABPAddressRepository} @@ -41,16 +41,22 @@ class AddressSearchController @Inject()(addressSearch: ABPAddressRepository, non scheduler: CheckAddressDataScheduler, val configHelper: ConfigHelper)( implicit ec: ExecutionContext) extends AddressController(cc) with AccessChecker { + import ErrorResponse.Implicits._ scheduler.enable() def search(): Action[String] = accessCheckedAction(parse.tolerantText) { request => - Json.parse(request.body).validate[LookupByPostcodeRequest](LookupByPostcodeRequest.reads) match { - case JsSuccess(lookupByPostcodeRequest, _) => - searchByPostcode(request, lookupByPostcodeRequest.postcode, lookupByPostcodeRequest.filter) - case JsError(errors) => - Future.successful(BadRequest(JsError.toJson(errors))) + val maybeJson = Try(Json.parse(request.body)) + maybeJson match { + case Success(json) => + json.validate[LookupByPostcodeRequest](LookupByPostcodeRequest.reads) match { + case JsSuccess(lookupByPostcodeRequest, _) => + searchByPostcode(request, lookupByPostcodeRequest.postcode, lookupByPostcodeRequest.filter) + case JsError(errors) => + Future.successful(BadRequest(JsError.toJson(errors))) + } + case Failure(_) => Future.successful(BadRequest(Json.toJson(ErrorResponse.invalidJson))) } } @@ -64,7 +70,7 @@ class AddressSearchController @Inject()(addressSearch: ABPAddressRepository, non case JsError(errors) => Future.successful(BadRequest(JsError.toJson(errors))) } - case Failure(exception) => Future.successful(BadRequest("""{"obj":[{"msg":["error.payload.missing"],"args":[]}]}""")) + case Failure(_) => Future.successful(BadRequest(Json.toJson(ErrorResponse.invalidJson))) } } @@ -78,7 +84,7 @@ class AddressSearchController @Inject()(addressSearch: ABPAddressRepository, non case JsError(errors) => Future.successful(BadRequest(JsError.toJson(errors))) } - case Failure(exception) => Future.successful(BadRequest("""{"obj":[{"msg":["error.payload.missing"],"args":[]}]}""")) + case Failure(_) => Future.successful(BadRequest(Json.toJson(ErrorResponse.invalidJson))) } } @@ -100,7 +106,7 @@ class AddressSearchController @Inject()(addressSearch: ABPAddressRepository, non case JsError(errors) => Future.successful(BadRequest(JsError.toJson(errors))) } - case Failure(exception) => Future.successful(BadRequest("""{"obj":[{"msg":["error.payload.missing"],"args":[]}]}""")) + case Failure(_) => Future.successful(BadRequest(Json.toJson(ErrorResponse.invalidJson))) } } diff --git a/app/model/response.scala b/app/model/response.scala index e97aa8c..383403a 100644 --- a/app/model/response.scala +++ b/app/model/response.scala @@ -40,4 +40,24 @@ object response { implicit val writes: Writes[SupportedCountryCodes] = Json.writes[SupportedCountryCodes] implicit val reads: Reads[SupportedCountryCodes] = Json.reads[SupportedCountryCodes] } + + case class ErrorMessage(msg: List[String], args: List[String]) + object ErrorMessage { + val invalidJson: ErrorMessage = ErrorMessage(msg = List("error.payload.invalid"), args = List()) + + object Implicits { + implicit val errorMessageReads: Reads[ErrorMessage] = Json.reads[ErrorMessage] + implicit val errorMessageWrites: Writes[ErrorMessage] = Json.writes[ErrorMessage] + } + } + case class ErrorResponse(obj: List[ErrorMessage]) + object ErrorResponse { + val invalidJson = ErrorResponse(obj = List(ErrorMessage.invalidJson)) + + object Implicits { + import ErrorMessage.Implicits._ + implicit val errorResponseReads: Reads[ErrorResponse] = Json.reads[ErrorResponse] + implicit val errorResponseWrites: Writes[ErrorResponse] = Json.writes[ErrorResponse] + } + } } diff --git a/it/test/suites/PostcodeLookupSuiteV2.scala b/it/test/suites/PostcodeLookupSuiteV2.scala index a12500a..8b51090 100644 --- a/it/test/suites/PostcodeLookupSuiteV2.scala +++ b/it/test/suites/PostcodeLookupSuiteV2.scala @@ -19,6 +19,7 @@ package it.suites import com.codahale.metrics.SharedMetricRegistries import it.helper.AppServerTestApi import model.address.{AddressRecord, Postcode} +import model.response.ErrorResponse import org.mockito.ArgumentMatchers.{any, eq => meq} import org.mockito.Mockito import org.mockito.Mockito.{times, when} @@ -27,7 +28,7 @@ import org.scalatestplus.mockito.MockitoSugar.mock import org.scalatestplus.play.guice.GuiceOneServerPerSuite import play.api.Application import play.api.inject.guice.GuiceApplicationBuilder -import play.api.libs.json.{JsArray, Json} +import play.api.libs.json.{JsArray, JsObject, JsString, JsValue, Json} import play.api.libs.ws.WSClient import play.api.test.Helpers.{await, defaultAwaitTimeout} import play.inject.Bindings @@ -260,6 +261,18 @@ class PostcodeLookupSuiteV2() response.status shouldBe BAD_REQUEST } + "give a bad request when the payload is invalid json" in { + import ErrorResponse.Implicits._ + val response = post("/lookup", """{"foo":"FX1 4AC""") + response.status shouldBe BAD_REQUEST + val responseText = response.body + val errorResponse = Json.parse(responseText).validate[ErrorResponse].get + + errorResponse.obj should have length(1) + errorResponse.obj.head.msg should have length(1) + errorResponse.obj.head.msg.head shouldBe "error.payload.invalid" + } + "give a not found when an unknown path is requested" in { val response = post("/somethingElse", """{"foo":"FX1 4AD"}""") response.status shouldBe NOT_FOUND From b05c05209f8d103956d268ab8a8530057f91e769 Mon Sep 17 00:00:00 2001 From: "Saqib@Ee" Date: Fri, 16 Feb 2024 16:02:21 +0000 Subject: [PATCH 2/2] CIR-1451: platops-pr-bot updates --- project/plugins.sbt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/project/plugins.sbt b/project/plugins.sbt index 1b45e91..34ca142 100644 --- a/project/plugins.sbt +++ b/project/plugins.sbt @@ -2,7 +2,7 @@ resolvers += Resolver.typesafeRepo("releases") resolvers += MavenRepository("HMRC-open-artefacts-maven2", "https://open.artefacts.tax.service.gov.uk/maven2") resolvers += Resolver.url("HMRC-open-artefacts-ivy", url("https://open.artefacts.tax.service.gov.uk/ivy2"))(Resolver.ivyStylePatterns) -addSbtPlugin("uk.gov.hmrc" % "sbt-auto-build" % "3.18.0") -addSbtPlugin("uk.gov.hmrc" % "sbt-distributables" % "2.4.0") +addSbtPlugin("uk.gov.hmrc" % "sbt-auto-build" % "3.20.0") +addSbtPlugin("uk.gov.hmrc" % "sbt-distributables" % "2.5.0") addSbtPlugin("org.playframework" % "sbt-plugin" % "3.0.0") addSbtPlugin("org.scoverage" % "sbt-scoverage" % "2.0.9")