DCWL-1384 Refactor AuthAction.scala to be more readable (#178)

* DCWL-1384 Refactor AuthAction.scala to be more readable * DCWL-1348 Improve parameter name * DCWL-1384 Use isBlank from JDK11 * DCWL-1384 Fix client ID validation
hmrc · Jun 14, 2023 · a3e7da6 · a3e7da6
1 parent 33eaa4d
commit a3e7da6
Show file tree

Hide file tree

Showing 16 changed files with 201 additions and 212 deletions.
diff --git a/app/uk/gov/hmrc/customs/inventorylinking/export/controllers/HeaderValidator.scala b/app/uk/gov/hmrc/customs/inventorylinking/export/controllers/HeaderValidator.scala
@@ -16,25 +16,24 @@
 
 package uk.gov.hmrc.customs.inventorylinking.export.controllers
 
-import javax.inject.{Inject, Singleton}
 import play.api.http.HeaderNames._
 import play.api.http.MimeTypes
 import play.api.mvc.Headers
 import uk.gov.hmrc.customs.api.common.controllers.ErrorResponse
 import uk.gov.hmrc.customs.api.common.controllers.ErrorResponse._
 import uk.gov.hmrc.customs.inventorylinking.export.controllers.CustomHeaderNames._
 import uk.gov.hmrc.customs.inventorylinking.export.logging.ExportsLogger
+import uk.gov.hmrc.customs.inventorylinking.export.model._
 import uk.gov.hmrc.customs.inventorylinking.export.model.actionbuilders.{ApiVersionRequest, ExtractedHeadersImpl, HasConversationId, HasRequest}
-import uk.gov.hmrc.customs.inventorylinking.export.model.{ClientId, _}
+
+import javax.inject.{Inject, Singleton}
 
 @Singleton
 class HeaderValidator @Inject()(logger: ExportsLogger) {
 
   private lazy val validContentTypeHeaders = Seq(MimeTypes.XML + ";charset=utf-8", MimeTypes.XML + "; charset=utf-8")
-  private lazy val xClientIdRegex = "^\\S+$".r
 
   private lazy val xBadgeIdentifierRegex = "^[0-9A-Z]{6,12}$".r
-  private lazy val InvalidEoriHeaderRegex = "(^[\\s]*$|^.{18,}$)".r
 
   private val errorResponseBadgeIdentifierHeaderMissing = errorBadRequest(s"$XBadgeIdentifierHeaderName header is missing or invalid")
   private val errorResponseEoriIdentifierHeaderInvalid = errorBadRequest(s"$XSubmitterIdentifierHeaderName header is invalid")
@@ -44,79 +43,64 @@ class HeaderValidator @Inject()(logger: ExportsLogger) {
 
     def hasContentType: Either[ErrorResponse, String] = validateHeader(CONTENT_TYPE, s => validContentTypeHeaders.contains(s.toLowerCase()), ErrorContentTypeHeaderInvalid)
 
-    def hasXClientId: Either[ErrorResponse, String] = validateHeader(XClientIdHeaderName, xClientIdRegex.findFirstIn(_).nonEmpty, ErrorInternalServerError)
+    def hasXClientId: Either[ErrorResponse, String] = validateHeader(XClientIdHeaderName, _.forall(!_.isWhitespace), ErrorInternalServerError)
 
     val theResult: Either[ErrorResponse, ExtractedHeadersImpl] = for {
       contentTypeValue <- hasContentType
       xClientIdValue <- hasXClientId
     } yield {
       logger.debug(
-      s"\n$CONTENT_TYPE header passed validation: $contentTypeValue"
-      + s"\n$XClientIdHeaderName header passed validation: $xClientIdValue")
+        s"\n$CONTENT_TYPE header passed validation: $contentTypeValue"
+          + s"\n$XClientIdHeaderName header passed validation: $xClientIdValue")
       ExtractedHeadersImpl(ClientId(xClientIdValue))
     }
     theResult
   }
 
-  private def validateHeader[A](headerName: String, rule: String => Boolean, errorResponse: ErrorResponse)(implicit apiVersionRequest: ApiVersionRequest[A], h: Headers): Either[ErrorResponse, String] = {
-    val left = Left(errorResponse)
-    def leftWithLog(headerName: String) = {
-      logger.error(s"Error - header '$headerName' not present")
-      left
-    }
-    def leftWithLogContainingValue(headerName: String, value: String) = {
-      logger.error(s"Error - header '$headerName' value '$value' is not valid")
-      left
-    }
-
-    h.get(headerName).fold[Either[ErrorResponse, String]]{
-      leftWithLog(headerName)
-    }{
-      v =>
-        if (rule(v)) Right(v) else leftWithLogContainingValue(headerName, v)
-    }
-  }
-
-  def eitherBadgeIdentifier[A](allowNone: Boolean)(implicit vhr: HasRequest[A] with HasConversationId): Either[ErrorResponse, Option[BadgeIdentifier]] = {
-    val maybeBadgeId: Option[String] = vhr.request.headers.toSimpleMap.get(XBadgeIdentifierHeaderName)
-
-    if (allowNone && maybeBadgeId.isEmpty) {
-      logger.info(s"$XBadgeIdentifierHeaderName header empty and allowed")
-      Right(None)
-    } else {
-      maybeBadgeId.filter(xBadgeIdentifierRegex.findFirstIn(_).nonEmpty).map { b =>
-        logger.info(s"$XBadgeIdentifierHeaderName header passed validation: $b")
-        Some(BadgeIdentifier(b))
-      }.toRight[ErrorResponse] {
-        logger.error(s"$XBadgeIdentifierHeaderName invalid or not present for CSP")
-        errorResponseBadgeIdentifierHeaderMissing
-      }
+  private def validateHeader[A](headerName: String, rule: String => Boolean, errorResponse: ErrorResponse)(implicit apiVersionRequest: ApiVersionRequest[A], headers: Headers): Either[ErrorResponse, String] = {
+    headers.get(headerName) match {
+      case Some(value) if rule(value) =>
+        Right(value)
+      case Some(invalidValue) =>
+        logger.error(s"Error - header '$headerName' value '$invalidValue' is not valid")
+        Left(errorResponse)
+      case None =>
+        logger.error(s"Error - header '$headerName' not present")
+        Left(errorResponse)
     }
   }
 
-  private def validEori(eori: String) = InvalidEoriHeaderRegex.findFirstIn(eori).isEmpty
+  def eitherBadgeIdentifier[A](implicit vhr: HasRequest[A] with HasConversationId): Either[ErrorResponse, Option[BadgeIdentifier]] = {
+    val maybeBadgeId = vhr.request.headers.toSimpleMap.get(XBadgeIdentifierHeaderName)
+    logger.debug(s"maybeBadgeId => $maybeBadgeId")
 
-  private def convertEmptyHeaderToNone(eori: Option[String]) = {
-    if (eori.isDefined && eori.get.trim.isEmpty) {
-      None
-    } else {
-      eori
+    maybeBadgeId match {
+      case Some(badgeId) if xBadgeIdentifierRegex.pattern.matcher(badgeId).matches =>
+        logger.info(s"$XBadgeIdentifierHeaderName header passed validation: $badgeId")
+        Right(Some(BadgeIdentifier(badgeId)))
+      case Some(_) =>
+        logger.error(s"$XBadgeIdentifierHeaderName invalid or not present for CSP")
+        Left(errorResponseBadgeIdentifierHeaderMissing)
+      case None =>
+        logger.info(s"$XBadgeIdentifierHeaderName header empty and allowed")
+        Right(None)
     }
   }
 
   def eoriMustBeValidIfPresent[A](implicit vhr: HasRequest[A] with HasConversationId): Either[ErrorResponse, Option[Eori]] = {
-    val maybeEoriHeader: Option[String] = vhr.request.headers.toSimpleMap.get(XSubmitterIdentifierHeaderName)
+    val maybeEoriHeader: Option[String] = vhr.request.headers.toSimpleMap.get(XSubmitterIdentifierHeaderName).filter(!_.isBlank)
     logger.debug(s"maybeEori => $maybeEoriHeader")
-    val maybeEori = convertEmptyHeaderToNone(maybeEoriHeader)
-
-    maybeEori match {
-      case Some(eori) => if (validEori(eori)) {
-        logger.info(s"$XSubmitterIdentifierHeaderName header passed validation: $eori")
-        Right(Some(Eori(eori)))
-      } else {
-        logger.error(s"$XSubmitterIdentifierHeaderName header is invalid for CSP: $eori")
-        Left(errorResponseEoriIdentifierHeaderInvalid)
-      }
+
+    maybeEoriHeader match {
+      case Some(unvalidatedEori) =>
+        Eori.fromString(unvalidatedEori) match {
+          case Some(eori) =>
+            logger.info(s"$XSubmitterIdentifierHeaderName header passed validation: $eori")
+            Right(Some(eori))
+          case None =>
+            logger.error(s"$XSubmitterIdentifierHeaderName header is invalid for CSP: $unvalidatedEori")
+            Left(errorResponseEoriIdentifierHeaderInvalid)
+        }
       case None =>
         logger.info(s"$XSubmitterIdentifierHeaderName header not present or is empty")
         Right(None)

diff --git a/app/uk/gov/hmrc/customs/inventorylinking/export/controllers/actionbuilders/AuthAction.scala b/app/uk/gov/hmrc/customs/inventorylinking/export/controllers/actionbuilders/AuthAction.scala
@@ -16,22 +16,19 @@
 
 package uk.gov.hmrc.customs.inventorylinking.export.controllers.actionbuilders
 
-import javax.inject.{Inject, Singleton}
 import play.api.mvc._
-import uk.gov.hmrc.customs.api.common.controllers.ErrorResponse
 import uk.gov.hmrc.customs.api.common.controllers.ErrorResponse.errorInternalServerError
-import uk.gov.hmrc.customs.inventorylinking.export.model.actionbuilders.{ApiSubscriptionFieldsRequest, HasConversationId, HasRequest}
-import uk.gov.hmrc.customs.inventorylinking.export.model.{AuthorisedAsCsp, BadgeIdentifier, Csp, Eori}
+import uk.gov.hmrc.customs.inventorylinking.`export`.model.{Csp, CspWithBadgeId, CspWithEori, CspWithEoriAndBadgeId, Eori}
 import uk.gov.hmrc.customs.inventorylinking.export.controllers.HeaderValidator
 import uk.gov.hmrc.customs.inventorylinking.export.logging.ExportsLogger
 import uk.gov.hmrc.customs.inventorylinking.export.model.actionbuilders.ActionBuilderModelHelper._
-import uk.gov.hmrc.customs.inventorylinking.export.model.actionbuilders.AuthorisedRequest
+import uk.gov.hmrc.customs.inventorylinking.export.model.actionbuilders.{ApiSubscriptionFieldsRequest, AuthorisedRequest}
 import uk.gov.hmrc.customs.inventorylinking.export.services.CustomsAuthService
 import uk.gov.hmrc.http.HeaderCarrier
 import uk.gov.hmrc.play.http.HeaderCarrierConverter
 
+import javax.inject.{Inject, Singleton}
 import scala.concurrent.{ExecutionContext, Future}
-import scala.util.Left
 
 /** Action builder that attempts to authorise request as a CSP or else NON CSP
  * <ul>
@@ -56,65 +53,47 @@ class AuthAction @Inject()(customsAuthService: CustomsAuthService,
 
   override def refine[A](asfr: ApiSubscriptionFieldsRequest[A]): Future[Either[Result, AuthorisedRequest[A]]] = {
     implicit val implicitAsfr: ApiSubscriptionFieldsRequest[A] = asfr
-    implicit def hc(implicit rh: RequestHeader): HeaderCarrier = HeaderCarrierConverter.fromRequest(rh)
 
-    authAsCspWithOptionalAuthHeaders.flatMap{
-      case Right(maybeAuthorisedAsCspWithIdentifierHeaders) =>
-        maybeAuthorisedAsCspWithIdentifierHeaders.fold{
-          customsAuthService.authAsNonCsp.map[Either[Result, AuthorisedRequest[A]]]{
-            case Left(errorResponse) =>
-              Left(errorResponse.XmlResult.withConversationId)
-            case Right(nonCspData) =>
-              Right(asfr.toNonCspAuthorisedRequest(nonCspData.eori))
-          }
-        }{ cspData =>
-          if (validIdentifier(cspData, asfr.apiSubscriptionFields.fields.authenticatedEori)) {
-            Future.successful(Right(asfr.toCspAuthorisedRequest(cspData)))
-          } else {
-            val msg = "Missing authenticated eori in service lookup. Alternately, use X-Badge-Identifier or X-Submitter-Identifier headers."
-            logger.error(s"For CSP request $msg")
-            Future.successful(Left(errorInternalServerError(msg).XmlResult.withConversationId))
-          }
+    implicit val hc: HeaderCarrier = HeaderCarrierConverter.fromRequest(asfr)
+
+    customsAuthService.authAsCsp.flatMap {
+      case Right(true) =>
+        Future.successful(maybeCspAuthorisedRequest(asfr))
+      case Right(false) =>
+        customsAuthService.authAsNonCsp.map {
+          case Right(nonCspData) =>
+            Right(asfr.toAuthorisedRequest(nonCspData))
+          case Left(errorResponse) =>
+            Left(errorResponse.XmlResult.withConversationId)
         }
       case Left(result) =>
         Future.successful(Left(result.XmlResult.withConversationId))
     }
   }
 
-  private def authAsCspWithOptionalAuthHeaders[A](implicit vfr: HasRequest[A] with HasConversationId, hc: HeaderCarrier): Future[Either[ErrorResponse, Option[AuthorisedAsCsp]]] = {
-    customsAuthService.authAsCsp.map {
-      case Right(isCsp) =>
-        if (isCsp) {
-          eitherCspAuthData.map(authAsCsp => Some(authAsCsp))
-        } else {
-          Right(None)
-        }
-      case Left(errorResponse) =>
-        Left(errorResponse)
-    }
-  }
-
-  def eitherCspAuthData[A](implicit vhr: HasRequest[A] with HasConversationId): Either[ErrorResponse, AuthorisedAsCsp] = {
-    for {
-      maybeBadgeId <- eitherBadgeIdentifier(allowNone = true)
-      maybeEori <- eitherEori
-    } yield Csp(maybeEori, maybeBadgeId)
-  }
-
-  private def eitherEori[A](implicit vhr: HasRequest[A] with HasConversationId): Either[ErrorResponse, Option[Eori]] = {
-    headerValidator.eoriMustBeValidIfPresent
-  }
+  private def maybeCspAuthorisedRequest[A](implicit asfr: ApiSubscriptionFieldsRequest[A])
+  : Either[Result, AuthorisedRequest[A]] = {
+    def toRight(csp: Csp): Right[Nothing, AuthorisedRequest[A]] = Right(asfr.toAuthorisedRequest(csp))
 
-  protected def eitherBadgeIdentifier[A](allowNone: Boolean)(implicit vhr: HasRequest[A] with HasConversationId): Either[ErrorResponse, Option[BadgeIdentifier]] = {
-    headerValidator.eitherBadgeIdentifier(allowNone = allowNone)
-  }
-
-  private def validIdentifier(cspData: AuthorisedAsCsp, authenticatedEori: Option[String]): Boolean = {
-    if (!cspData.isEmpty || (authenticatedEori.isDefined && !authenticatedEori.get.trim.isEmpty)) {
-      true
-    } else {
-      false
+    val maybeAuthenticatedEori = asfr.apiSubscriptionFields.fields.authenticatedEori.flatMap(Eori.fromString)
+    (for {
+      maybeEori <- headerValidator.eoriMustBeValidIfPresent
+      maybeBadgeId <- headerValidator.eitherBadgeIdentifier
+    } yield (maybeEori, maybeBadgeId, maybeAuthenticatedEori)) match {
+      case Right((Some(eori), None, _)) =>
+        toRight(CspWithEori(eori))
+      case Right((None, Some(badgeId), _)) =>
+        toRight(CspWithBadgeId(badgeId))
+      case Right((Some(eori), Some(badgeId), _)) =>
+        toRight(CspWithEoriAndBadgeId(eori, badgeId))
+      case Right((None, None, Some(authenticatedEori))) =>
+        toRight(CspWithEori(authenticatedEori))
+      case Right((None, None, None)) =>
+        val msg = "Missing authenticated eori in service lookup. Alternately, use X-Badge-Identifier or X-Submitter-Identifier headers."
+        logger.error(s"For CSP request $msg")
+        Left(errorInternalServerError(msg).XmlResult.withConversationId)
+      case Left(result) =>
+        Left(result.XmlResult.withConversationId)
     }
   }
-
 }
diff --git a/...k/gov/hmrc/customs/inventorylinking/export/model/actionbuilders/actionBuildersModel.scala b/...k/gov/hmrc/customs/inventorylinking/export/model/actionbuilders/actionBuildersModel.scala
@@ -65,10 +65,6 @@ object ActionBuilderModelHelper {
 
   implicit class ApiSubscriptionFieldsRequestOps[A](val asf: ApiSubscriptionFieldsRequest[A]) extends AnyVal {
 
-    def toCspAuthorisedRequest(a: AuthorisedAsCsp): AuthorisedRequest[A] = toAuthorisedRequest(a)
-
-    def toNonCspAuthorisedRequest(eori: Eori): AuthorisedRequest[A] = toAuthorisedRequest(NonCsp(eori))
-
     def toAuthorisedRequest(authorisedAs: AuthorisedAs): AuthorisedRequest[A] = AuthorisedRequest(
       asf.conversationId,
       asf.start,

diff --git a/app/uk/gov/hmrc/customs/inventorylinking/export/model/models.scala b/app/uk/gov/hmrc/customs/inventorylinking/export/model/models.scala
@@ -17,18 +17,35 @@
 package uk.gov.hmrc.customs.inventorylinking.export.model
 
 import java.util.UUID
-
 import play.api.libs.json.{JsString, Reads, Writes}
 
-case class Eori(value: String) extends AnyVal {
+import scala.util.matching.Regex
+
+case class Eori private(value: String) extends AnyVal {
   override def toString: String = value
 }
 
+object Eori {
+
+  private val ValidEoriHeaderRegex: Regex = "(^[A-Za-z]{2}[a-zA-Z0-9]{1,15}$)".r
+
+  private def apply(value: String): Eori = new Eori(value)
+
+  def fromString(value: String): Option[Eori] = {
+    if (ValidEoriHeaderRegex.pattern.matcher(value).matches) {
+      Some(new Eori(value))
+    } else {
+      None
+    }
+  }
+}
+
 case class ClientId(value: String) extends AnyVal
 
 case class ConversationId(uuid: UUID) extends AnyVal {
   override def toString: String = uuid.toString
 }
+
 object ConversationId {
   implicit val writer: Writes[ConversationId] = Writes[ConversationId] { x => JsString(x.uuid.toString) }
   implicit val reader: Reads[ConversationId] = Reads.of[UUID].map(new ConversationId(_))
@@ -45,22 +62,28 @@ case class SubscriptionFieldsId(value: String) extends AnyVal
 sealed trait ApiVersion {
   val value: String
   val configPrefix: String
+
   override def toString: String = value
 }
-object VersionOne extends ApiVersion{
+
+object VersionOne extends ApiVersion {
   override val value: String = "1.0"
   override val configPrefix: String = ""
 }
-object VersionTwo extends ApiVersion{
+
+object VersionTwo extends ApiVersion {
   override val value: String = "2.0"
   override val configPrefix: String = "v2."
 }
 
 sealed trait AuthorisedAs
-sealed trait AuthorisedAsCsp extends AuthorisedAs {
-  val eori: Option[Eori]
-  val badgeIdentifier: Option[BadgeIdentifier]
-  val isEmpty: Boolean = eori.isEmpty && badgeIdentifier.isEmpty
-}
-case class Csp(eori: Option[Eori], badgeIdentifier: Option[BadgeIdentifier]) extends AuthorisedAsCsp
+
+sealed trait Csp extends AuthorisedAs
+
+case class CspWithEori(eori: Eori) extends Csp
+
+case class CspWithBadgeId(badgeIdentifier: BadgeIdentifier) extends Csp
+
+case class CspWithEoriAndBadgeId(eori: Eori, badgeIdentifier: BadgeIdentifier) extends Csp
+
 case class NonCsp(eori: Eori) extends AuthorisedAs
diff --git a/app/uk/gov/hmrc/customs/inventorylinking/export/services/CustomsAuthService.scala b/app/uk/gov/hmrc/customs/inventorylinking/export/services/CustomsAuthService.scala
@@ -110,7 +110,8 @@ class CustomsAuthService @Inject()(override val authConnector: AuthConnector,
     for {
       customsEnrolment <- maybeCustomsEnrolment
       eoriIdentifier <- customsEnrolment.getIdentifier("EORINumber")
-    } yield Eori(eoriIdentifier.value)
+      eori <- Eori.fromString(eoriIdentifier.value)
+    } yield eori
   }
 
 }