Revert "Revert "Filter out invalid topics from registration requests""

Author: aracho1

common/src/main/scala/models/Registration.scala

@@ -21,17 +21,17 @@ object Registration {
           val n = (v \ "name").asOpt[String].getOrElse("unknown")
           s"$t (id=$n)"
         }
-        val all = (json \ "topics").asOpt[JsArray].toSeq.flatMap(_.value.map(topicSummary))
-        val invalid = (json \ "topics").asOpt[JsArray].toSeq.flatMap { arr =>
-          arr.value.flatMap { v =>
-            if (v.validate[Topic].isError) Some(topicSummary(v)) else None
-          }
-        }
-        if (invalid.nonEmpty) {
-          MDC.put("invalidTopics", invalid.mkString(", "))
-          if (all.nonEmpty) MDC.put("allTopics", all.mkString(", "))
+
+        val topicsArray = (json \ "topics").asOpt[JsArray].getOrElse(JsArray())
+        val (validTopics, invalidTopics) = topicsArray.value.partition(_.validate[Topic].isSuccess)
+
+        if (invalidTopics.nonEmpty) {
+          MDC.put("invalidTopics", invalidTopics.map(topicSummary).mkString(", "))
+          MDC.put("validTopics", validTopics.map(topicSummary).mkString(", "))
         }
-        base.reads(json)
+
+        val filteredJson = json.as[JsObject] + ("topics" -> JsArray(validTopics))
+        base.reads(filteredJson)
       },
       base
     )

common/src/test/scala/models/RegistrationSpec.scala

@@ -0,0 +1,43 @@
+ package models
+
+import models.TopicTypes.{Breaking, FootballMatch}
+import org.specs2.mutable.Specification
+import play.api.libs.json.Json
+
+class RegistrationSpec extends Specification {
+  "Registration" should {
+    "parse a valid registration" in {
+      val json = Json.parse(
+        """{"deviceToken":"abc","platform":"ios","topics":[{"type":"breaking","name":"uk"}]}"""
+      )
+      val result = json.validate[Registration]
+      result.isSuccess must beTrue
+      result.get.topics must_== Set(Topic(Breaking, "uk"))
+    }
+
+    "filter out invalid topic types and keep valid ones" in {
+      val json = Json.parse(
+        """{"deviceToken":"abc","platform":"ios","topics":[{"type":"tag","name":"some-tag"},{"type":"breaking","name":"uk"}]}"""
+      )
+      val result = json.validate[Registration]
+      result.isSuccess must beTrue
+      result.get.topics must_== Set(Topic(Breaking, "uk"))
+    }
+
+    "parse successfully with an empty topic set if all topics are invalid (controller will reject)" in {
+      val json = Json.parse(
+        """{"deviceToken":"abc","platform":"ios","topics":[{"type":"tag","name":"some-tag"}]}"""
+      )
+      val result = json.validate[Registration]
+      result.isSuccess must beTrue
+      result.get.topics must beEmpty
+    }
+
+    "still fail for other invalid fields" in {
+      val json = Json.parse(
+        """{"deviceToken":"abc","platform":"unknown-platform","topics":[{"type":"breaking","name":"uk"}]}"""
+      )
+      json.validate[Registration].isError must beTrue
+    }
+  }
+}

registration/app/registration/controllers/Main.scala

@@ -14,7 +14,7 @@ import registration.services._
 import registration.services.topic.TopicValidator
 
 import scala.concurrent.{ExecutionContext, Future}
-import org.slf4j.{Logger, LoggerFactory}
+import org.slf4j.{Logger, LoggerFactory, MDC}
 import play.api.http.HttpEntity
 import providers.ProviderError
 
@@ -62,7 +62,18 @@ final class Main(
     registerWithConverter(legacyRegistrationConverter)
 
   def register: Action[Registration] = actionWithTimeout(parse.json[Registration]) { request: Request[Registration] =>
-    registerCommon(request.body).map(processResponse(_))
+    Option(MDC.get("invalidTopics")).foreach { invalidTopics =>
+      val validTopics = Option(MDC.get("validTopics")).getOrElse("")
+      val userAgent = request.headers.get("User-Agent").getOrElse("unknown")
+      val deviceToken = request.body.deviceToken
+      logger.warn(s"Request contains invalid topic type(s): [$invalidTopics]. Filtering them out. Keeping: [$validTopics]. Device token: $deviceToken. User agent: $userAgent")
+      MDC.remove("invalidTopics")
+      MDC.remove("validTopics")
+    }
+    if (request.body.topics.isEmpty)
+      Future.successful(BadRequest("Request contains no valid topics"))
+    else
+      registerCommon(request.body).map(processResponse(_))
   }
 
   private def registerWithConverter[T](converter: RegistrationConverter[T])(implicit format: Format[T]): Action[T] = actionWithTimeout(parse.json[T]) { request =>