APIS-4826: Url validation improvements

hmrc · Jul 31, 2020 · 2736cd2 · 2736cd2
1 parent 0a5f99e
commit 2736cd2
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 17 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
+     }
+   })
   }
-}
+}
diff --git 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")
 
 }