From 2736cd23a4e105d67d73b867a0b4a9f23aa98191 Mon Sep 17 00:00:00 2001 From: Nick Choudhry <28593658+NC-1234@users.noreply.github.com> Date: Fri, 31 Jul 2020 15:50:05 +0100 Subject: [PATCH] APIS-4826: Url validation improvements --- .../apisubscriptionfields/model/Model.scala | 7 ++++- .../apisubscriptionfields/model/Types.scala | 1 - build.sbt | 4 ++- .../model/ModelSpec.scala | 30 +++++++++++-------- .../model/ValidationRuleTestData.scala | 2 +- 5 files changed, 27 insertions(+), 17 deletions(-) diff --git a/app/uk/gov/hmrc/apisubscriptionfields/model/Model.scala b/app/uk/gov/hmrc/apisubscriptionfields/model/Model.scala index 454d3f8..1e3d7c3 100644 --- a/app/uk/gov/hmrc/apisubscriptionfields/model/Model.scala +++ b/app/uk/gov/hmrc/apisubscriptionfields/model/Model.scala @@ -33,8 +33,13 @@ case class RegexValidationRule(regex: RegexExpr) extends ValidationRule { def validateAgainstRule(value: FieldValue): Boolean = value.matches(regex.value) } +// Taken from: https://stackoverflow.com/a/5078838 case object UrlValidationRule extends ValidationRule { - def validateAgainstRule(value: FieldValue): Boolean = refineV[NonFtpUrl](value).isRight + def validateAgainstRule(value: FieldValue): Boolean = { + val schemes = Array("http","https") + val urlValidator = new org.apache.commons.validator.routines.UrlValidator(schemes) + urlValidator.isValid(value) + } } case class ValidationGroup(errorMessage: String, rules: NEL[ValidationRule]) diff --git a/app/uk/gov/hmrc/apisubscriptionfields/model/Types.scala b/app/uk/gov/hmrc/apisubscriptionfields/model/Types.scala index 3eb2c6a..65b748e 100644 --- a/app/uk/gov/hmrc/apisubscriptionfields/model/Types.scala +++ b/app/uk/gov/hmrc/apisubscriptionfields/model/Types.scala @@ -23,7 +23,6 @@ import eu.timepit.refined.boolean._ object Types { type RegexExpr = String Refined Regex - type NonFtpUrl = Url And Not[StartsWith[W.`"ftp"`.T]] type FieldNameRegex = MatchesRegex[W.`"^[a-zA-Z]+$"`.T] type FieldName = Refined[String,FieldNameRegex] diff --git a/build.sbt b/build.sbt index f354300..8a21788 100644 --- a/build.sbt +++ b/build.sbt @@ -33,7 +33,8 @@ val compile = Seq( "uk.gov.hmrc" %% "http-metrics" % "1.10.0", "org.typelevel" %% "cats-core" % "2.1.0", "eu.timepit" %% "refined" % "0.9.13", - "be.venneborg" %% "play26-refined" % "0.5.0" + "be.venneborg" %% "play26-refined" % "0.5.0", + "commons-validator" % "commons-validator" % "1.6" ) // we need to override the akka version for now as newer versions are not compatible with reactivemongo @@ -55,6 +56,7 @@ def test(scope: String = "test,acceptance") = Seq( "org.pegdown" % "pegdown" % "1.6.0" % scope, "com.typesafe.play" %% "play-test" % play.core.PlayVersion.current % scope, "com.github.tomakehurst" % "wiremock-jre8" % "2.26.3" % scope + // "org.scalatest" % "scalatest-funspec" % "3.0.8" % scope ) val appName = "api-subscription-fields" diff --git a/test/uk/gov/hmrc/apisubscriptionfields/model/ModelSpec.scala b/test/uk/gov/hmrc/apisubscriptionfields/model/ModelSpec.scala index 085f925..d90b3fd 100644 --- a/test/uk/gov/hmrc/apisubscriptionfields/model/ModelSpec.scala +++ b/test/uk/gov/hmrc/apisubscriptionfields/model/ModelSpec.scala @@ -19,7 +19,8 @@ package uk.gov.hmrc.apisubscriptionfields.model import uk.gov.hmrc.apisubscriptionfields.SubscriptionFieldsTestData import uk.gov.hmrc.apisubscriptionfields.FieldDefinitionTestData import uk.gov.hmrc.apisubscriptionfields.HmrcSpec - +import org.scalatest.FunSpec +import org.scalatest.Matchers class ModelSpec extends HmrcSpec with SubscriptionFieldsTestData with FieldDefinitionTestData with ValidationRuleTestData { "RegexValidationRule" should { @@ -35,23 +36,26 @@ class ModelSpec extends HmrcSpec with SubscriptionFieldsTestData with FieldDefin "return false when the value is invalid - too short" in { atLeastTenLongRule.validate(mixedCaseValue) shouldBe false } - "return true when the value is blank" in { atLeastTenLongRule.validate("") shouldBe true } } +} - "UrlValidationRule" should { - "pass for a matching value" in { - UrlValidationRule.validate(validUrl) shouldBe true - } +class UrlValidationRuleSpec extends FunSpec with ValidationRuleTestData with Matchers { + describe("pass for a matching value") { + UrlValidationRule.validate(validUrl) shouldBe true + } - "fail for a value that does not match" in { - invalidUrls.map(invalidUrl => UrlValidationRule.validate(invalidUrl) shouldBe false) - } + describe("return true when the value is blank") { + UrlValidationRule.validate("") shouldBe true + } - "return true when the value is blank" in { - UrlValidationRule.validate("") shouldBe true - } + describe("invalid urls") { + invalidUrls.map(invalidUrl => { + it(s"$invalidUrl should not be valid") { + UrlValidationRule.validate(invalidUrl) shouldBe false + } + }) } -} +} \ No newline at end of file diff --git a/test/uk/gov/hmrc/apisubscriptionfields/model/ValidationRuleTestData.scala b/test/uk/gov/hmrc/apisubscriptionfields/model/ValidationRuleTestData.scala index e6fdef7..395b835 100644 --- a/test/uk/gov/hmrc/apisubscriptionfields/model/ValidationRuleTestData.scala +++ b/test/uk/gov/hmrc/apisubscriptionfields/model/ValidationRuleTestData.scala @@ -29,6 +29,6 @@ trait ValidationRuleTestData { val atLeastTenLongRule: ValidationRule = RegexValidationRule("""^.{10}.*$""") val validUrl = "https://www.example.com/here/and/there" - val invalidUrls = List("www.example.com", "ftp://example.com/abc") + val invalidUrls = List("www.example.com", "ftp://example.com/abc", "https://www example.com", "https://www&example.com", "https://www,example.com") }