From 37e59e4230fcadfeff2bf579c4dbeaf439dbe150 Mon Sep 17 00:00:00 2001 From: dg-hmrc-21 <88377883+dg-21-hmrc@users.noreply.github.com> Date: Thu, 5 Dec 2024 17:55:21 +0000 Subject: [PATCH 1/4] GG-8234 Remove deprecated elements in API definition --- app/config/APIAccessConfig.scala | 2 +- app/views/definition.scala.txt | 3 +-- test/config/APIAccessConfigSpec.scala | 10 +++++----- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/app/config/APIAccessConfig.scala b/app/config/APIAccessConfig.scala index df3497f..5b155b1 100644 --- a/app/config/APIAccessConfig.scala +++ b/app/config/APIAccessConfig.scala @@ -20,7 +20,7 @@ import com.typesafe.config.{Config, ConfigObject} import scala.jdk.CollectionConverters.* -case class APIAccessConfig(version: String, status: String, accessType: String, endpointsEnabled: Boolean, whiteListedApplicationIds: List[String]) +case class APIAccessConfig(version: String, status: String, accessType: String, endpointsEnabled: Boolean, allowListedApplicationIds: List[String]) case class APIAccessVersions(versionConfigs: Option[ConfigObject]) { def findAPIs(versions: List[String], config: Config): List[APIAccessConfig] = { diff --git a/app/views/definition.scala.txt b/app/views/definition.scala.txt index 3107b61..018d7a6 100644 --- a/app/views/definition.scala.txt +++ b/app/views/definition.scala.txt @@ -18,8 +18,7 @@ "status":"@version.status", "endpointsEnabled": @version.endpointsEnabled, "access" : { - "type" : "@version.accessType", - "whitelistedApplicationIds" : @Json.toJson(version.whiteListedApplicationIds) + "type" : "@version.accessType" } } } \ No newline at end of file diff --git a/test/config/APIAccessConfigSpec.scala b/test/config/APIAccessConfigSpec.scala index 16a811f..ada1cad 100644 --- a/test/config/APIAccessConfigSpec.scala +++ b/test/config/APIAccessConfigSpec.scala @@ -44,11 +44,11 @@ class APIAccessConfigSpec extends UnitSpec { version.accessType shouldBe "PRIVATE" version.status shouldBe "STABLE" version.endpointsEnabled shouldBe true - version.whiteListedApplicationIds shouldBe a[List[_]] - version.whiteListedApplicationIds.size shouldBe 3 - version.whiteListedApplicationIds should contain("649def0f-3ed3-4df5-8ae1-3e687a9143ea") - version.whiteListedApplicationIds should contain("df8c10db-01fb-4543-b77e-859267462231") - version.whiteListedApplicationIds should contain("9a32c713-7741-4aae-b39d-957021fb97a9") + version.allowListedApplicationIds shouldBe a[List[_]] + version.allowListedApplicationIds.size shouldBe 3 + version.allowListedApplicationIds should contain("649def0f-3ed3-4df5-8ae1-3e687a9143ea") + version.allowListedApplicationIds should contain("df8c10db-01fb-4543-b77e-859267462231") + version.allowListedApplicationIds should contain("9a32c713-7741-4aae-b39d-957021fb97a9") case unknown => fail(s"Unknown version found : $unknown") } } From d4a2e79c8029a92313eb1ee8032956c13f8cfa49 Mon Sep 17 00:00:00 2001 From: dg-hmrc-21 <88377883+dg-21-hmrc@users.noreply.github.com> Date: Fri, 6 Dec 2024 11:17:36 +0000 Subject: [PATCH 2/4] GG-8234 Remove unused config values --- app/config/APIAccessConfig.scala | 6 ++---- conf/application.conf | 5 ----- project/plugins.sbt | 3 +-- test/config/APIAccessConfigSpec.scala | 20 ++++++-------------- 4 files changed, 9 insertions(+), 25 deletions(-) diff --git a/app/config/APIAccessConfig.scala b/app/config/APIAccessConfig.scala index 5b155b1..4f29d5a 100644 --- a/app/config/APIAccessConfig.scala +++ b/app/config/APIAccessConfig.scala @@ -20,7 +20,7 @@ import com.typesafe.config.{Config, ConfigObject} import scala.jdk.CollectionConverters.* -case class APIAccessConfig(version: String, status: String, accessType: String, endpointsEnabled: Boolean, allowListedApplicationIds: List[String]) +case class APIAccessConfig(version: String, status: String, accessType: String, endpointsEnabled: Boolean) case class APIAccessVersions(versionConfigs: Option[ConfigObject]) { def findAPIs(versions: List[String], config: Config): List[APIAccessConfig] = { @@ -29,12 +29,10 @@ case class APIAccessVersions(versionConfigs: Option[ConfigObject]) { val accessType = if (value.hasPath("type")) value.getString("type") else "PRIVATE" val status = if (value.hasPath("status")) value.getString("status") else throw new IllegalArgumentException("Status missing") - val allowListedApplicationIds = - if (value.hasPath("allow-list.applicationIds")) Some(value.getStringList("allow-list.applicationIds").asScala.toList) else None val endpointsEnabled = if (value.hasPath("endpointsEnabled")) value.getBoolean("endpointsEnabled") else false val versionNumber = version.replace('_', '.') - APIAccessConfig(versionNumber, status, accessType, endpointsEnabled, allowListedApplicationIds.getOrElse(List())) + APIAccessConfig(versionNumber, status, accessType, endpointsEnabled) } } diff --git a/conf/application.conf b/conf/application.conf index 94af571..1d390cc 100644 --- a/conf/application.conf +++ b/conf/application.conf @@ -107,11 +107,6 @@ api.access.version { 1_0 { type = PRIVATE status = STABLE - allow-list { - applicationIds.0 = 649def0f-3ed3-4df5-8ae1-3e687a9143ea - applicationIds.1 = df8c10db-01fb-4543-b77e-859267462231 - applicationIds.2 = 9a32c713-7741-4aae-b39d-957021fb97a9 - } endpointsEnabled = true } } diff --git a/project/plugins.sbt b/project/plugins.sbt index bb257ea..85afe14 100644 --- a/project/plugins.sbt +++ b/project/plugins.sbt @@ -2,9 +2,8 @@ resolvers += "HMRC-open-artefacts-maven" at "https://open.artefacts.tax.service. 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.22.0") +addSbtPlugin("uk.gov.hmrc" % "sbt-auto-build" % "3.24.0") addSbtPlugin("uk.gov.hmrc" % "sbt-distributables" % "2.5.0") addSbtPlugin("org.scoverage" % "sbt-scoverage" % "2.2.0") addSbtPlugin("org.playframework" % "sbt-plugin" % "3.0.5") addSbtPlugin("org.scalameta" % "sbt-scalafmt" % "2.5.2") -addSbtPlugin("ch.epfl.scala" % "sbt-scala3-migrate" % "0.6.1") \ No newline at end of file diff --git a/test/config/APIAccessConfigSpec.scala b/test/config/APIAccessConfigSpec.scala index ada1cad..1ef6ca0 100644 --- a/test/config/APIAccessConfigSpec.scala +++ b/test/config/APIAccessConfigSpec.scala @@ -23,12 +23,9 @@ import testSupport.UnitSpec class APIAccessConfigSpec extends UnitSpec { val configuration: Configuration = Configuration.from( Map( - "api.access.version.1_0.type" -> "PRIVATE", - "api.access.version.1_0.status" -> "STABLE", - "api.access.version.1_0.allow-list.applicationIds.0" -> "649def0f-3ed3-4df5-8ae1-3e687a9143ea", - "api.access.version.1_0.allow-list.applicationIds.1" -> "df8c10db-01fb-4543-b77e-859267462231", - "api.access.version.1_0.allow-list.applicationIds.2" -> "9a32c713-7741-4aae-b39d-957021fb97a9", - "api.access.version.1_0.endpointsEnabled" -> true + "api.access.version.1_0.type" -> "PRIVATE", + "api.access.version.1_0.status" -> "STABLE", + "api.access.version.1_0.endpointsEnabled" -> true ) ) @@ -41,14 +38,9 @@ class APIAccessConfigSpec extends UnitSpec { version shouldBe a[APIAccessConfig] version.version match { case "1.0" => - version.accessType shouldBe "PRIVATE" - version.status shouldBe "STABLE" - version.endpointsEnabled shouldBe true - version.allowListedApplicationIds shouldBe a[List[_]] - version.allowListedApplicationIds.size shouldBe 3 - version.allowListedApplicationIds should contain("649def0f-3ed3-4df5-8ae1-3e687a9143ea") - version.allowListedApplicationIds should contain("df8c10db-01fb-4543-b77e-859267462231") - version.allowListedApplicationIds should contain("9a32c713-7741-4aae-b39d-957021fb97a9") + version.accessType shouldBe "PRIVATE" + version.status shouldBe "STABLE" + version.endpointsEnabled shouldBe true case unknown => fail(s"Unknown version found : $unknown") } } From fe02022443b5283d96bfabd9639af0258115c67b Mon Sep 17 00:00:00 2001 From: dg-hmrc-21 <88377883+dg-21-hmrc@users.noreply.github.com> Date: Mon, 9 Dec 2024 08:56:31 +0000 Subject: [PATCH 3/4] Refactor: Finish Scala 3 migration, move Format instances, simplify error model --- ...hirdPartyDelegatedAuthorityConnector.scala | 2 +- app/controllers/ErrorResponses.scala | 30 +++----------- app/controllers/HeaderValidator.scala | 2 +- app/controllers/UserInfoController.scala | 10 ++--- app/controllers/package.scala | 29 -------------- app/domain/APIDefinition.scala | 8 +++- app/domain/GovernmentGatewayDetails.scala | 6 +++ app/domain/UserDetails.scala | 10 ++++- app/domain/UserInfo.scala | 28 ++++++++++++- app/domain/package.scala | 40 ------------------- app/filters/MicroserviceAuthFilter.scala | 11 ++--- app/handlers/ErrorHandler.scala | 11 ++--- it/test/stubs/AuthStub.scala | 10 ++++- test/controllers/ErrorResponseSpec.scala | 2 +- test/controllers/UserInfoControllerSpec.scala | 5 +-- 15 files changed, 85 insertions(+), 119 deletions(-) delete mode 100644 app/controllers/package.scala delete mode 100644 app/domain/package.scala diff --git a/app/connectors/ThirdPartyDelegatedAuthorityConnector.scala b/app/connectors/ThirdPartyDelegatedAuthorityConnector.scala index afb5d39..01804c3 100644 --- a/app/connectors/ThirdPartyDelegatedAuthorityConnector.scala +++ b/app/connectors/ThirdPartyDelegatedAuthorityConnector.scala @@ -33,7 +33,7 @@ class ThirdPartyDelegatedAuthorityConnector @Inject() (appContext: AppContext, h httpClient .get(url"$serviceUrl/delegated-authority") .setHeader("access-token" -> accessToken) - .execute(readRaw, ec) + .execute(using readRaw, ec) .map { response => if (response.status == Status.NOT_FOUND) { Set[String]() diff --git a/app/controllers/ErrorResponses.scala b/app/controllers/ErrorResponses.scala index 8aedec2..e2d3d6b 100644 --- a/app/controllers/ErrorResponses.scala +++ b/app/controllers/ErrorResponses.scala @@ -19,31 +19,13 @@ package controllers import play.mvc.Http.Status.* import play.api.libs.json.* -sealed abstract class ErrorResponse(val httpStatusCode: Int, val errorCode: String, val message: String) - -case class ErrorUnauthorized(msg: String = "Bearer token is missing or not authorized") extends ErrorResponse(UNAUTHORIZED, "UNAUTHORIZED", msg) - -case class ErrorNotFound(msg: String = "Resource was not found") extends ErrorResponse(NOT_FOUND, "NOT_FOUND", msg) - -case class ErrorAcceptHeaderInvalid(msg: String = "The accept header is invalid") extends ErrorResponse(NOT_ACCEPTABLE, "ACCEPT_HEADER_INVALID", msg) - -case class ErrorBadGateway(msg: String = "Bad gateway") extends ErrorResponse(BAD_GATEWAY, "BAD_GATEWAY", msg) - -case class ErrorBadRequest(msg: String = "Bad request") extends ErrorResponse(BAD_REQUEST, "BAD_REQUEST", msg) - -case object ErrorUnauthorizedLowCL extends ErrorResponse(401, "LOW_CONFIDENCE_LEVEL", "Confidence Level on account does not allow access") - -object ErrorBadRequest { - def apply(errors: Seq[(JsPath, Seq[JsonValidationError])]): ErrorBadRequest = - ErrorBadRequest(JsError.toJson(errors).as[String]) -} - -case object ErrorAcceptHeaderInvalid extends ErrorResponse(406, "ACCEPT_HEADER_INVALID", "The accept header is missing or invalid") - -case object ErrorInternalServerError extends ErrorResponse(500, "INTERNAL_SERVER_ERROR", "Internal server error") - -case object PreferencesSettingsError extends ErrorResponse(500, "PREFERENCE_SETTINGS_ERROR", "Failed to set preferences") +case class ErrorResponse(val httpStatusCode: Int, val errorCode: String, val message: String) object ErrorResponse { implicit val writes: Writes[ErrorResponse] = (e: ErrorResponse) => Json.obj("code" -> e.errorCode, "message" -> e.message) + + def badGateway(msg: String = "Bad gateway"): ErrorResponse = ErrorResponse(BAD_GATEWAY, "BAD_GATEWAY", msg) + def unauthorized(msg: String = "Bearer token is missing or not authorized"): ErrorResponse = ErrorResponse(UNAUTHORIZED, "UNAUTHORIZED", msg) + def badRequest(msg: String = "Bad request"): ErrorResponse = ErrorResponse(BAD_REQUEST, "BAD_REQUEST", msg) + def notAcceptable(msg: String = "The accept header is invalid"): ErrorResponse = ErrorResponse(NOT_ACCEPTABLE, "ACCEPT_HEADER_INVALID", msg) } diff --git a/app/controllers/HeaderValidator.scala b/app/controllers/HeaderValidator.scala index d63efca..e6d768b 100644 --- a/app/controllers/HeaderValidator.scala +++ b/app/controllers/HeaderValidator.scala @@ -44,7 +44,7 @@ trait HeaderValidator extends Results { block: (Request[A]) => Future[Result] ): Future[Result] = if (rules(request.headers.get("Accept"))) block(request) - else Future.successful(Status(ErrorAcceptHeaderInvalid.httpStatusCode)(Json.toJson[ErrorResponse](ErrorAcceptHeaderInvalid))) + else Future.successful(NotAcceptable(Json.toJson(ErrorResponse.notAcceptable()))) override def parser: BodyParser[AnyContent] = outer.parser diff --git a/app/controllers/UserInfoController.scala b/app/controllers/UserInfoController.scala index c8ca02c..5a97aaf 100644 --- a/app/controllers/UserInfoController.scala +++ b/app/controllers/UserInfoController.scala @@ -66,11 +66,11 @@ trait UserInfoController extends BackendBaseController with HeaderValidator { Ok(json) } recover { - case Upstream4xxResponse(UER(_, 401, _, _)) => Unauthorized(Json.toJson(ErrorUnauthorized())) - case Upstream4xxResponse(UER(msg4xx, _, _, _)) => BadGateway(Json.toJson(ErrorBadGateway(msg4xx))) - case Upstream5xxResponse(UER(msg5xx, _, _, _)) => BadGateway(Json.toJson(ErrorBadGateway(msg5xx))) - case bex: BadRequestException => BadRequest(Json.toJson(ErrorBadRequest(bex.getMessage))) - case uex: UnauthorizedException => Unauthorized(Json.toJson(ErrorUnauthorized(uex.getMessage))) + case Upstream4xxResponse(UER(_, 401, _, _)) => Unauthorized(Json.toJson(ErrorResponse.unauthorized())) + case Upstream4xxResponse(UER(msg4xx, _, _, _)) => BadGateway(Json.toJson(ErrorResponse.badGateway(msg4xx))) + case Upstream5xxResponse(UER(msg5xx, _, _, _)) => BadGateway(Json.toJson(ErrorResponse.badGateway(msg5xx))) + case bex: BadRequestException => BadRequest(Json.toJson(ErrorResponse.badRequest(bex.getMessage))) + case uex: UnauthorizedException => Unauthorized(Json.toJson(ErrorResponse.unauthorized(uex.getMessage))) } } } diff --git a/app/controllers/package.scala b/app/controllers/package.scala deleted file mode 100644 index 85557bc..0000000 --- a/app/controllers/package.scala +++ /dev/null @@ -1,29 +0,0 @@ -/* - * Copyright 2024 HM Revenue & Customs - * - * Licensed 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. - */ - -import play.api.libs.json.{Json, Writes} - -package object controllers { - - private def errorWrites[T <: ErrorResponse]: Writes[T] = (o: T) => Json.obj("code" -> o.errorCode, "message" -> o.message) - - implicit val errorResponseWrites: Writes[ErrorResponse] = errorWrites[ErrorResponse] - implicit val errorUnauthorizedWrites: Writes[ErrorUnauthorized] = errorWrites[ErrorUnauthorized] - implicit val errorNotFoundWrites: Writes[ErrorNotFound] = errorWrites[ErrorNotFound] - implicit val errorAcceptHeaderInvalidWrites: Writes[ErrorAcceptHeaderInvalid] = errorWrites[ErrorAcceptHeaderInvalid] - implicit val errorBadGatewayWrites: Writes[ErrorBadGateway] = errorWrites[ErrorBadGateway] - implicit val errorBadRequestWrites: Writes[ErrorBadRequest] = errorWrites[ErrorBadRequest] -} diff --git a/app/domain/APIDefinition.scala b/app/domain/APIDefinition.scala index 0ccf982..550752d 100644 --- a/app/domain/APIDefinition.scala +++ b/app/domain/APIDefinition.scala @@ -16,4 +16,10 @@ package domain -case class APIAccess(`type`: String, allowlistedApplicationIds: Option[Seq[String]]) +import play.api.libs.json.{Json, OFormat} + +case class APIAccess(`type`: String) + +object APIAccess { + implicit val format: OFormat[APIAccess] = Json.format[APIAccess] +} diff --git a/app/domain/GovernmentGatewayDetails.scala b/app/domain/GovernmentGatewayDetails.scala index 05d040f..3cef00e 100644 --- a/app/domain/GovernmentGatewayDetails.scala +++ b/app/domain/GovernmentGatewayDetails.scala @@ -16,6 +16,8 @@ package domain +import play.api.libs.json.{Json, OFormat} + case class GovernmentGatewayDetails( user_id: Option[String], roles: Option[Seq[String]], @@ -29,3 +31,7 @@ case class GovernmentGatewayDetails( profile_uri: Option[String], group_profile_uri: Option[String] ) + +object GovernmentGatewayDetails { + implicit val format: OFormat[GovernmentGatewayDetails] = Json.format[GovernmentGatewayDetails] +} diff --git a/app/domain/UserDetails.scala b/app/domain/UserDetails.scala index f1d1501..8fe090d 100644 --- a/app/domain/UserDetails.scala +++ b/app/domain/UserDetails.scala @@ -16,9 +16,11 @@ package domain -import java.time.LocalDate +import play.api.libs.json.{Json, OFormat} import uk.gov.hmrc.auth.core.retrieve.{GatewayInformation, MdtpInformation} +import java.time.LocalDate + case class UserDetails( authProviderId: Option[String] = None, authProviderType: Option[String] = None, @@ -39,3 +41,9 @@ case class UserDetails( profile: Option[String], groupProfile: Option[String] ) + +object UserDetails { + private implicit val giFormat: OFormat[GatewayInformation] = Json.format[GatewayInformation] + private implicit val miFormat: OFormat[MdtpInformation] = Json.format[MdtpInformation] + implicit val format: OFormat[UserDetails] = Json.format[UserDetails] +} diff --git a/app/domain/UserInfo.scala b/app/domain/UserInfo.scala index 569c193..57947c7 100644 --- a/app/domain/UserInfo.scala +++ b/app/domain/UserInfo.scala @@ -16,14 +16,24 @@ package domain +import play.api.libs.json.{Format, Json, OFormat, Reads} + import java.time.LocalDate -import uk.gov.hmrc.auth.core.Enrolment +import uk.gov.hmrc.auth.core.{Enrolment, EnrolmentIdentifier} import uk.gov.hmrc.auth.core.retrieve.{ItmpAddress, ItmpName} case class Address(formatted: String, postal_code: Option[String], country: Option[String], country_code: Option[String]) +object Address { + implicit val format: OFormat[Address] = Json.format[Address] +} + case class Mdtp(device_id: String, session_id: String) +object Mdtp { + implicit val format: OFormat[Mdtp] = Json.format[Mdtp] +} + case class UserInfo(given_name: Option[String] = None, family_name: Option[String] = None, middle_name: Option[String] = None, @@ -38,8 +48,24 @@ case class UserInfo(given_name: Option[String] = None, group_profile_url: Option[String] = None ) +object UserInfo { + /* We re-define our own simple Format for Enrolment because the one provided but the library one + renames a field ("key" to "enrolment") in a way which disagrees with the UserInfo schema */ + private implicit val enrFormat: OFormat[Enrolment] = { + implicit val idFormat: Format[EnrolmentIdentifier] = Json.format[EnrolmentIdentifier] + Json.format[Enrolment] + } + implicit val format: OFormat[UserInfo] = Json.format[UserInfo] +} + case class UserInformation(profile: Option[UserProfile], address: Option[Address], uk_gov_nino: Option[String]) case class UserProfile(given_name: Option[String], family_name: Option[String], middle_name: Option[String], birthdate: Option[LocalDate]) case class DesUserInfo(name: Option[ItmpName], dateOfBirth: Option[LocalDate], address: Option[ItmpAddress]) + +object DesUserInfo { + private implicit val inFormat: Format[ItmpName] = Json.format[ItmpName] + private implicit val iaFormat: Format[ItmpAddress] = Json.format[ItmpAddress] + implicit val reads: Format[DesUserInfo] = Json.format[DesUserInfo] +} diff --git a/app/domain/package.scala b/app/domain/package.scala deleted file mode 100644 index 7ccff1a..0000000 --- a/app/domain/package.scala +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright 2024 HM Revenue & Customs - * - * Licensed 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. - */ - -import play.api.libs.json.* -import uk.gov.hmrc.auth.core.retrieve.{GatewayInformation, ItmpAddress, ItmpName, MdtpInformation} -import uk.gov.hmrc.auth.core.{Enrolment, EnrolmentIdentifier} - -package object domain { - - implicit val desUserName: OFormat[ItmpName] = Json.format - - implicit val desAddress: OFormat[ItmpAddress] = Json.format - - implicit val enrolmentIdentifier: OFormat[EnrolmentIdentifier] = Json.format - implicit val enrloment: OFormat[Enrolment] = Json.format - - implicit val gatewayInformationFmt: Format[GatewayInformation] = Json.format - implicit val mdtpInformationFmt: Format[MdtpInformation] = Json.format - implicit val userDetails: OFormat[UserDetails] = Json.format - implicit val mdtp: OFormat[Mdtp] = Json.format - implicit val governmentGatewayDetails: OFormat[GovernmentGatewayDetails] = Json.format - - implicit val addressFmt: OFormat[Address] = Json.format - implicit val userInfoFmt: OFormat[UserInfo] = Json.format - implicit val apiAccessFmt: OFormat[APIAccess] = Json.format - -} diff --git a/app/filters/MicroserviceAuthFilter.scala b/app/filters/MicroserviceAuthFilter.scala index 44d29a1..863f4c8 100644 --- a/app/filters/MicroserviceAuthFilter.scala +++ b/app/filters/MicroserviceAuthFilter.scala @@ -16,18 +16,19 @@ package filters -import javax.inject.{Inject, Singleton} +import connectors.AuthConnector +import controllers.ErrorResponse +import controllers.ErrorResponse.writes +import org.apache.pekko.stream.Materializer import play.api.Configuration import play.api.libs.json.Json import play.api.mvc.{Filter, RequestHeader, Result, Results} import play.api.routing.Router import uk.gov.hmrc.auth.core.{AuthorisationException, AuthorisedFunctions} -import connectors.AuthConnector -import controllers.ErrorUnauthorized -import org.apache.pekko.stream.Materializer import uk.gov.hmrc.http.HeaderCarrier import uk.gov.hmrc.play.http.HeaderCarrierConverter +import javax.inject.{Inject, Singleton} import scala.concurrent.{ExecutionContext, Future} @Singleton @@ -46,7 +47,7 @@ class MicroserviceAuthFilter @Inject() (configuration: Configuration, val authCo authorised() { next(rh) } recoverWith { case e: AuthorisationException => - Future.successful(Unauthorized(Json.toJson(ErrorUnauthorized()))) + Future.successful(Unauthorized(Json.toJson(ErrorResponse.unauthorized()))) } case _ => next(rh) } diff --git a/app/handlers/ErrorHandler.scala b/app/handlers/ErrorHandler.scala index e51ef44..b6e19b8 100644 --- a/app/handlers/ErrorHandler.scala +++ b/app/handlers/ErrorHandler.scala @@ -16,15 +16,16 @@ package handlers -import javax.inject.{Inject, Singleton} import com.google.inject.Provider +import controllers.ErrorResponse import play.api.http.DefaultHttpErrorHandler import play.api.libs.json.Json -import play.api.mvc.Results.Status +import play.api.mvc.Results.{BadGateway, Unauthorized} import play.api.mvc.{RequestHeader, Result} import play.api.routing.Router import play.api.{Configuration, Environment, OptionalSourceMapper} -import controllers.{ErrorBadGateway, ErrorUnauthorized} + +import javax.inject.{Inject, Singleton} import scala.concurrent.{ExecutionContext, Future} @Singleton @@ -39,8 +40,8 @@ class ErrorHandler @Inject() ( override def onServerError(request: RequestHeader, exception: Throwable): Future[Result] = { super.onServerError(request, exception) map (res => { res.header.status match { - case 401 => Status(ErrorUnauthorized().httpStatusCode)(Json.toJson(ErrorUnauthorized())) - case _ => Status(ErrorBadGateway().httpStatusCode)(Json.toJson(ErrorBadGateway(exception.getMessage))) + case 401 => Unauthorized(Json.toJson(ErrorResponse.unauthorized())) + case _ => BadGateway(Json.toJson(ErrorResponse.badGateway(exception.getMessage))) } }) } diff --git a/it/test/stubs/AuthStub.scala b/it/test/stubs/AuthStub.scala index 3a5d828..593d7f5 100644 --- a/it/test/stubs/AuthStub.scala +++ b/it/test/stubs/AuthStub.scala @@ -17,17 +17,23 @@ package stubs import com.github.tomakehurst.wiremock.client.WireMock.* +import controllers.{Version, Version_1_0} +import domain.* import play.api.libs.json.* import play.api.libs.json.Writes.DefaultLocalDateWrites import uk.gov.hmrc.auth.core.retrieve.* import uk.gov.hmrc.auth.core.retrieve.v2.Retrievals import uk.gov.hmrc.auth.core.{AffinityGroup, CredentialRole} import uk.gov.hmrc.domain.Nino -import controllers.{Version, Version_1_0} -import domain.{DesUserInfo, *} trait AuthStub { + private implicit val itmpNameFormat: OFormat[ItmpName] = Json.format + private implicit val itmpAddressFormat: OFormat[ItmpAddress] = Json.format + + private implicit val gatewayInformationFormat: Format[GatewayInformation] = Json.format + private implicit val mdtpInformationFormat: Format[MdtpInformation] = Json.format + implicit class JsOptAppendable(jsObject: JsObject) { def appendOptional(key: String, value: Option[JsValue]): JsObject = value .map(js => jsObject + (key -> js)) diff --git a/test/controllers/ErrorResponseSpec.scala b/test/controllers/ErrorResponseSpec.scala index e20ea73..a1a28a4 100644 --- a/test/controllers/ErrorResponseSpec.scala +++ b/test/controllers/ErrorResponseSpec.scala @@ -23,7 +23,7 @@ import testSupport.UnitSpec class ErrorResponseSpec extends UnitSpec with Matchers { "errorResponse" should { "be translated to error Json with only the required fields" in { - Json.toJson(ErrorAcceptHeaderInvalid()).toString() shouldBe + Json.toJson(ErrorResponse.notAcceptable()).toString() shouldBe """{"code":"ACCEPT_HEADER_INVALID","message":"The accept header is invalid"}""" } } diff --git a/test/controllers/UserInfoControllerSpec.scala b/test/controllers/UserInfoControllerSpec.scala index 4aba46e..896a4bb 100644 --- a/test/controllers/UserInfoControllerSpec.scala +++ b/test/controllers/UserInfoControllerSpec.scala @@ -20,8 +20,6 @@ import config.AppContext import domain.{Address, GovernmentGatewayDetails, UserInfo} import org.apache.pekko.actor.ActorSystem import org.mockito.ArgumentMatchers.{any, eq as eqTo} - -import java.time.LocalDate import org.mockito.Mockito.when import org.scalatest.concurrent.ScalaFutures import org.scalatestplus.mockito.MockitoSugar @@ -33,6 +31,7 @@ import testSupport.UnitSpec import uk.gov.hmrc.auth.core.{Enrolment, EnrolmentIdentifier} import uk.gov.hmrc.http.{BadRequestException, HeaderCarrier} +import java.time.LocalDate import scala.concurrent.{ExecutionContext, Future} class UserInfoControllerSpec(implicit val cc: ControllerComponents, ex: ExecutionContext) extends UnitSpec with MockitoSugar with ScalaFutures { @@ -125,7 +124,7 @@ class UserInfoControllerSpec(implicit val cc: ControllerComponents, ex: Executio val result = await(liveController.userInfo()(FakeRequest().withHeaders("Accept" -> "application/vnd.hmrc.1.0+json"))) status(result) shouldBe 400 - jsonBodyOf(result) shouldBe Json.toJson(ErrorBadRequest("NINO is required")) + jsonBodyOf(result) shouldBe Json.toJson(ErrorResponse.badRequest("NINO is required")) } } } From ddf9a75bda05d3f6f5f453777796f0f136155df4 Mon Sep 17 00:00:00 2001 From: dg-hmrc-21 <88377883+dg-21-hmrc@users.noreply.github.com> Date: Mon, 9 Dec 2024 12:52:56 +0000 Subject: [PATCH 4/4] Add extra tests for error responses following refactor --- ...rorResponses.scala => ErrorResponse.scala} | 16 ++++- app/controllers/HeaderValidator.scala | 3 +- app/controllers/UserInfoController.scala | 10 ++-- app/filters/MicroserviceAuthFilter.scala | 4 +- app/handlers/ErrorHandler.scala | 6 +- test/controllers/ErrorResponseSpec.scala | 59 +++++++++++++++++-- 6 files changed, 76 insertions(+), 22 deletions(-) rename app/controllers/{ErrorResponses.scala => ErrorResponse.scala} (50%) diff --git a/app/controllers/ErrorResponses.scala b/app/controllers/ErrorResponse.scala similarity index 50% rename from app/controllers/ErrorResponses.scala rename to app/controllers/ErrorResponse.scala index e2d3d6b..3824216 100644 --- a/app/controllers/ErrorResponses.scala +++ b/app/controllers/ErrorResponse.scala @@ -18,8 +18,15 @@ package controllers import play.mvc.Http.Status.* import play.api.libs.json.* +import play.api.mvc.Result +import play.api.mvc.Results.Status -case class ErrorResponse(val httpStatusCode: Int, val errorCode: String, val message: String) +/** Please use the constructors provided in the companion object and not the main constructor as these responses are expected to contain specific + * wording. + */ +case class ErrorResponse private (httpStatusCode: Int, errorCode: String, message: String) { + def toResult: Result = Status(httpStatusCode)(Json.toJson(this)) +} object ErrorResponse { implicit val writes: Writes[ErrorResponse] = (e: ErrorResponse) => Json.obj("code" -> e.errorCode, "message" -> e.message) @@ -27,5 +34,10 @@ object ErrorResponse { def badGateway(msg: String = "Bad gateway"): ErrorResponse = ErrorResponse(BAD_GATEWAY, "BAD_GATEWAY", msg) def unauthorized(msg: String = "Bearer token is missing or not authorized"): ErrorResponse = ErrorResponse(UNAUTHORIZED, "UNAUTHORIZED", msg) def badRequest(msg: String = "Bad request"): ErrorResponse = ErrorResponse(BAD_REQUEST, "BAD_REQUEST", msg) - def notAcceptable(msg: String = "The accept header is invalid"): ErrorResponse = ErrorResponse(NOT_ACCEPTABLE, "ACCEPT_HEADER_INVALID", msg) + def badRequest(errors: Seq[(JsPath, Seq[JsonValidationError])]): ErrorResponse = badRequest(JsError.toJson(errors).as[String]) + val acceptHeaderInvalid: ErrorResponse = ErrorResponse(NOT_ACCEPTABLE, "ACCEPT_HEADER_INVALID", "The accept header is missing or invalid") + val notFound: ErrorResponse = ErrorResponse(NOT_FOUND, "NOT_FOUND", "Resource was not found") + val unauthorizedLowCL: ErrorResponse = ErrorResponse(UNAUTHORIZED, "LOW_CONFIDENCE_LEVEL", "Confidence Level on account does not allow access") + val internalServerError: ErrorResponse = ErrorResponse(INTERNAL_SERVER_ERROR, "INTERNAL_SERVER_ERROR", "Internal server error") + val preferencesSettingsError: ErrorResponse = ErrorResponse(INTERNAL_SERVER_ERROR, "PREFERENCE_SETTINGS_ERROR", "Failed to set preferences") } diff --git a/app/controllers/HeaderValidator.scala b/app/controllers/HeaderValidator.scala index e6d768b..e00dae0 100644 --- a/app/controllers/HeaderValidator.scala +++ b/app/controllers/HeaderValidator.scala @@ -16,7 +16,6 @@ package controllers -import play.api.libs.json.Json import play.api.mvc.* import scala.concurrent.{ExecutionContext, Future} @@ -44,7 +43,7 @@ trait HeaderValidator extends Results { block: (Request[A]) => Future[Result] ): Future[Result] = if (rules(request.headers.get("Accept"))) block(request) - else Future.successful(NotAcceptable(Json.toJson(ErrorResponse.notAcceptable()))) + else Future.successful(ErrorResponse.acceptHeaderInvalid.toResult) override def parser: BodyParser[AnyContent] = outer.parser diff --git a/app/controllers/UserInfoController.scala b/app/controllers/UserInfoController.scala index 5a97aaf..20aafd2 100644 --- a/app/controllers/UserInfoController.scala +++ b/app/controllers/UserInfoController.scala @@ -66,11 +66,11 @@ trait UserInfoController extends BackendBaseController with HeaderValidator { Ok(json) } recover { - case Upstream4xxResponse(UER(_, 401, _, _)) => Unauthorized(Json.toJson(ErrorResponse.unauthorized())) - case Upstream4xxResponse(UER(msg4xx, _, _, _)) => BadGateway(Json.toJson(ErrorResponse.badGateway(msg4xx))) - case Upstream5xxResponse(UER(msg5xx, _, _, _)) => BadGateway(Json.toJson(ErrorResponse.badGateway(msg5xx))) - case bex: BadRequestException => BadRequest(Json.toJson(ErrorResponse.badRequest(bex.getMessage))) - case uex: UnauthorizedException => Unauthorized(Json.toJson(ErrorResponse.unauthorized(uex.getMessage))) + case Upstream4xxResponse(UER(_, 401, _, _)) => ErrorResponse.unauthorized().toResult + case Upstream4xxResponse(UER(msg4xx, _, _, _)) => ErrorResponse.badGateway(msg4xx).toResult + case Upstream5xxResponse(UER(msg5xx, _, _, _)) => ErrorResponse.badGateway(msg5xx).toResult + case bex: BadRequestException => ErrorResponse.badRequest(bex.getMessage).toResult + case uex: UnauthorizedException => ErrorResponse.unauthorized(uex.getMessage).toResult } } } diff --git a/app/filters/MicroserviceAuthFilter.scala b/app/filters/MicroserviceAuthFilter.scala index 863f4c8..8e33291 100644 --- a/app/filters/MicroserviceAuthFilter.scala +++ b/app/filters/MicroserviceAuthFilter.scala @@ -18,10 +18,8 @@ package filters import connectors.AuthConnector import controllers.ErrorResponse -import controllers.ErrorResponse.writes import org.apache.pekko.stream.Materializer import play.api.Configuration -import play.api.libs.json.Json import play.api.mvc.{Filter, RequestHeader, Result, Results} import play.api.routing.Router import uk.gov.hmrc.auth.core.{AuthorisationException, AuthorisedFunctions} @@ -47,7 +45,7 @@ class MicroserviceAuthFilter @Inject() (configuration: Configuration, val authCo authorised() { next(rh) } recoverWith { case e: AuthorisationException => - Future.successful(Unauthorized(Json.toJson(ErrorResponse.unauthorized()))) + Future.successful(ErrorResponse.unauthorized().toResult) } case _ => next(rh) } diff --git a/app/handlers/ErrorHandler.scala b/app/handlers/ErrorHandler.scala index b6e19b8..8b55c1e 100644 --- a/app/handlers/ErrorHandler.scala +++ b/app/handlers/ErrorHandler.scala @@ -19,8 +19,6 @@ package handlers import com.google.inject.Provider import controllers.ErrorResponse import play.api.http.DefaultHttpErrorHandler -import play.api.libs.json.Json -import play.api.mvc.Results.{BadGateway, Unauthorized} import play.api.mvc.{RequestHeader, Result} import play.api.routing.Router import play.api.{Configuration, Environment, OptionalSourceMapper} @@ -40,8 +38,8 @@ class ErrorHandler @Inject() ( override def onServerError(request: RequestHeader, exception: Throwable): Future[Result] = { super.onServerError(request, exception) map (res => { res.header.status match { - case 401 => Unauthorized(Json.toJson(ErrorResponse.unauthorized())) - case _ => BadGateway(Json.toJson(ErrorResponse.badGateway(exception.getMessage))) + case 401 => ErrorResponse.unauthorized().toResult + case _ => ErrorResponse.badGateway(exception.getMessage).toResult } }) } diff --git a/test/controllers/ErrorResponseSpec.scala b/test/controllers/ErrorResponseSpec.scala index a1a28a4..daa7908 100644 --- a/test/controllers/ErrorResponseSpec.scala +++ b/test/controllers/ErrorResponseSpec.scala @@ -16,16 +16,63 @@ package controllers +import org.apache.pekko.actor.ActorSystem +import org.apache.pekko.stream.Materializer import org.scalatest.matchers.should.Matchers -import play.api.libs.json.Json +import play.api.http.Status.* import testSupport.UnitSpec class ErrorResponseSpec extends UnitSpec with Matchers { - "errorResponse" should { - "be translated to error Json with only the required fields" in { - Json.toJson(ErrorResponse.notAcceptable()).toString() shouldBe - """{"code":"ACCEPT_HEADER_INVALID","message":"The accept header is invalid"}""" + implicit lazy val system: ActorSystem = ActorSystem() + implicit lazy val materializer: Materializer = Materializer(system) + + "error response should serialise with the expected code and description" when { + "unauthorized" in { + val result = ErrorResponse.unauthorized().toResult + status(result) shouldBe UNAUTHORIZED + bodyOf(result) shouldBe """{"code":"UNAUTHORIZED","message":"Bearer token is missing or not authorized"}""" + } + + "notFound" in { + val result = ErrorResponse.notFound.toResult + status(result) shouldBe NOT_FOUND + bodyOf(result) shouldBe """{"code":"NOT_FOUND","message":"Resource was not found"}""" + } + + "badGateway" in { + val result = ErrorResponse.badGateway().toResult + status(result) shouldBe BAD_GATEWAY + bodyOf(result) shouldBe """{"code":"BAD_GATEWAY","message":"Bad gateway"}""" + } + + "badRequest" in { + val result = ErrorResponse.badRequest().toResult + status(result) shouldBe BAD_REQUEST + bodyOf(result) shouldBe """{"code":"BAD_REQUEST","message":"Bad request"}""" } - } + "unauthorizedLowCL" in { + val result = ErrorResponse.unauthorizedLowCL.toResult + status(result) shouldBe UNAUTHORIZED + bodyOf(result) shouldBe """{"code":"LOW_CONFIDENCE_LEVEL","message":"Confidence Level on account does not allow access"}""" + } + + "acceptHeaderInvalid" in { + val result = ErrorResponse.acceptHeaderInvalid.toResult + status(result) shouldBe NOT_ACCEPTABLE + bodyOf(result) shouldBe """{"code":"ACCEPT_HEADER_INVALID","message":"The accept header is missing or invalid"}""" + } + + "internalServerError" in { + val result = ErrorResponse.internalServerError.toResult + status(result) shouldBe INTERNAL_SERVER_ERROR + bodyOf(result) shouldBe """{"code":"INTERNAL_SERVER_ERROR","message":"Internal server error"}""" + } + + "preferencesSettingsError" in { + val result = ErrorResponse.preferencesSettingsError.toResult + status(result) shouldBe INTERNAL_SERVER_ERROR + bodyOf(result) shouldBe """{"code":"PREFERENCE_SETTINGS_ERROR","message":"Failed to set preferences"}""" + } + } }