From 58e9d0c24065ba27ecce511958b575b8ef7f29eb Mon Sep 17 00:00:00 2001 From: spenes Date: Wed, 9 Aug 2023 15:06:55 +0300 Subject: [PATCH] Address Ian's feedback --- .../CollectorService.scala | 30 ++++++++----------- .../model.scala | 6 ++-- .../CollectorServiceSpec.scala | 18 +++++------ .../TestUtils.scala | 2 +- .../StdoutCollector.scala | 2 +- 5 files changed, 28 insertions(+), 30 deletions(-) diff --git a/http4s/src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/CollectorService.scala b/http4s/src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/CollectorService.scala index aaa18547d..75cddc2e9 100644 --- a/http4s/src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/CollectorService.scala +++ b/http4s/src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/CollectorService.scala @@ -190,17 +190,12 @@ class CollectorService[F[_]: Sync]( case None => cookieConfig.map { config => val responseCookie = ResponseCookie( - name = config.name, - content = networkUserId, - expires = Some(HttpDate.unsafeFromEpochSecond((now + config.expiration).toSeconds)), - domain = cookieDomain(headers, config.domains, config.fallbackDomain), - path = Some("/"), - sameSite = config.sameSite.flatMap { - case "Strict" => SameSite.Strict.some - case "Lax" => SameSite.Lax.some - case "None" => SameSite.None.some - case _ => None - }, + name = config.name, + content = networkUserId, + expires = Some(HttpDate.unsafeFromEpochSecond((now + config.expiration).toSeconds)), + domain = cookieDomain(headers, config.domains, config.fallbackDomain), + path = Some("/"), + sameSite = config.sameSite, secure = config.secure, httpOnly = config.httpOnly ) @@ -222,14 +217,15 @@ class CollectorService[F[_]: Sync]( */ def cookieDomain( headers: Headers, - domains: Option[List[String]], + domains: List[String], fallbackDomain: Option[String] ): Option[String] = - (for { - domainList <- domains - originHosts = extractHosts(headers) - domainToUse <- domainList.find(domain => originHosts.exists(validMatch(_, domain))) - } yield domainToUse).orElse(fallbackDomain) + (domains match { + case Nil => None + case _ => + val originHosts = extractHosts(headers) + domains.find(domain => originHosts.exists(validMatch(_, domain))) + }).orElse(fallbackDomain) /** Extracts the host names from a list of values in the request's Origin header. */ def extractHosts(headers: Headers): List[String] = diff --git a/http4s/src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/model.scala b/http4s/src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/model.scala index 245f5a1a6..18c0b4563 100644 --- a/http4s/src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/model.scala +++ b/http4s/src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/model.scala @@ -2,6 +2,8 @@ package com.snowplowanalytics.snowplow.collectors.scalastream import scala.concurrent.duration._ +import org.http4s.SameSite + import io.circe.Json object model { @@ -33,11 +35,11 @@ object model { enabled: Boolean, name: String, expiration: FiniteDuration, - domains: Option[List[String]], + domains: List[String], fallbackDomain: Option[String], secure: Boolean, httpOnly: Boolean, - sameSite: Option[String] + sameSite: Option[SameSite] ) final case class CollectorConfig( diff --git a/http4s/src/test/scala/com.snowplowanalytics.snowplow.collectors.scalastream/CollectorServiceSpec.scala b/http4s/src/test/scala/com.snowplowanalytics.snowplow.collectors.scalastream/CollectorServiceSpec.scala index c0e232e05..08720df71 100644 --- a/http4s/src/test/scala/com.snowplowanalytics.snowplow.collectors.scalastream/CollectorServiceSpec.scala +++ b/http4s/src/test/scala/com.snowplowanalytics.snowplow.collectors.scalastream/CollectorServiceSpec.scala @@ -258,7 +258,7 @@ class CollectorServiceSpec extends Specification { enabled = true, name = "name", expiration = 5.seconds, - domains = Some(List("domain")), + domains = List("domain"), fallbackDomain = None, secure = false, httpOnly = false, @@ -325,7 +325,7 @@ class CollectorServiceSpec extends Specification { val conf = testCookieConfig.copy( secure = true, httpOnly = true, - sameSite = Some("None") + sameSite = Some(SameSite.None) ) val Some(`Set-Cookie`(cookie)) = service.cookieHeader( @@ -356,7 +356,7 @@ class CollectorServiceSpec extends Specification { enabled = true, name = "name", expiration = 5.seconds, - domains = None, + domains = List.empty, fallbackDomain = None, secure = false, httpOnly = false, @@ -370,7 +370,7 @@ class CollectorServiceSpec extends Specification { } "if a list of domains is supplied in the config but the Origin request header is empty and there is no fallback domain" in { val headers = Headers.empty - val cookieConfig = testCookieConfig.copy(domains = Some(List("domain.com"))) + val cookieConfig = testCookieConfig.copy(domains = List("domain.com")) service.cookieDomain(headers, cookieConfig.domains, cookieConfig.fallbackDomain) shouldEqual None } "if none of the domains in the request's Origin header has a match in the list of domains supplied with the config and there is no fallback domain" in { @@ -383,7 +383,7 @@ class CollectorServiceSpec extends Specification { ) val headers = Headers(origin.toRaw1) val cookieConfig = testCookieConfig.copy( - domains = Some(List("domain.com", "otherdomain.com")) + domains = List("domain.com", "otherdomain.com") ) service.cookieDomain(headers, cookieConfig.domains, cookieConfig.fallbackDomain) shouldEqual None } @@ -401,7 +401,7 @@ class CollectorServiceSpec extends Specification { "if the Origin header is empty and a fallback domain is configured" in { val headers = Headers.empty val cookieConfig = testCookieConfig.copy( - domains = Some(List("domain.com")), + domains = List("domain.com"), fallbackDomain = Some("fallbackDomain") ) service.cookieDomain(headers, cookieConfig.domains, cookieConfig.fallbackDomain) shouldEqual Some( @@ -418,7 +418,7 @@ class CollectorServiceSpec extends Specification { ) val headers = Headers(origin.toRaw1) val cookieConfig = testCookieConfig.copy( - domains = Some(List("domain.com", "otherdomain.com")), + domains = List("domain.com", "otherdomain.com"), fallbackDomain = Some("fallbackDomain") ) service.cookieDomain(headers, cookieConfig.domains, cookieConfig.fallbackDomain) shouldEqual Some( @@ -435,7 +435,7 @@ class CollectorServiceSpec extends Specification { ) val headers = Headers(origin.toRaw1) val cookieConfig = testCookieConfig.copy( - domains = Some(List("domain.com", "otherdomain.com")), + domains = List("domain.com", "otherdomain.com"), fallbackDomain = Some("fallbackDomain") ) service.cookieDomain(headers, cookieConfig.domains, cookieConfig.fallbackDomain) shouldEqual Some( @@ -456,7 +456,7 @@ class CollectorServiceSpec extends Specification { ) val headers = Headers(origin.toRaw1) val cookieConfig = testCookieConfig.copy( - domains = Some(List("domain.com", "otherdomain.com")), + domains = List("domain.com", "otherdomain.com"), fallbackDomain = Some("fallbackDomain") ) service.cookieDomain(headers, cookieConfig.domains, cookieConfig.fallbackDomain) shouldEqual Some( diff --git a/http4s/src/test/scala/com.snowplowanalytics.snowplow.collectors.scalastream/TestUtils.scala b/http4s/src/test/scala/com.snowplowanalytics.snowplow.collectors.scalastream/TestUtils.scala index ddb7fa000..a60a79c0a 100644 --- a/http4s/src/test/scala/com.snowplowanalytics.snowplow.collectors.scalastream/TestUtils.scala +++ b/http4s/src/test/scala/com.snowplowanalytics.snowplow.collectors.scalastream/TestUtils.scala @@ -15,7 +15,7 @@ object TestUtils { enabled = true, name = "sp", expiration = 365.days, - domains = None, + domains = List.empty, fallbackDomain = None, secure = false, httpOnly = false, diff --git a/stdout/src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/StdoutCollector.scala b/stdout/src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/StdoutCollector.scala index 0dd3b0c0b..7a1a7f592 100644 --- a/stdout/src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/StdoutCollector.scala +++ b/stdout/src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/StdoutCollector.scala @@ -34,7 +34,7 @@ object StdoutCollector extends IOApp { enabled = true, name = "sp", expiration = 365.days, - domains = None, + domains = List.empty, fallbackDomain = None, secure = false, httpOnly = false,