From 32cdd42ccc6b7e40bc8b7540da9ced33a7222e5a Mon Sep 17 00:00:00 2001 From: Carlos Rocha Date: Wed, 3 Aug 2022 16:37:07 +0100 Subject: [PATCH] STAC-17136. Fix problem sessionId was not always validated if it was in the store. Swap the order of when the CSRF cookie was added in login callback to gurantee a valid sessionId is returned during login process. --- .../stackstate/pac4j/AkkaHttpSecurity.scala | 2 +- .../stackstate/pac4j/AkkaHttpWebContext.scala | 1 + .../pac4j/AkkaHttpSecurityTest.scala | 66 +++++++++++++++++++ .../pac4j/AkkaHttpWebContextTest.scala | 22 +++++++ 4 files changed, 90 insertions(+), 1 deletion(-) diff --git a/src/main/scala/com/stackstate/pac4j/AkkaHttpSecurity.scala b/src/main/scala/com/stackstate/pac4j/AkkaHttpSecurity.scala index f5768ba..fa4839c 100644 --- a/src/main/scala/com/stackstate/pac4j/AkkaHttpSecurity.scala +++ b/src/main/scala/com/stackstate/pac4j/AkkaHttpSecurity.scala @@ -180,8 +180,8 @@ class AkkaHttpSecurity(config: Config, sessionStorage: SessionStorage, val sessi _ => callbackLogic.perform(akkaWebContext, config, actionAdapter, defaultUrl, saveInSession, multiProfile, true, defaultClient.orNull).map { result => - akkaWebContext.addResponseSessionCookie() if (setCsrfCookie) akkaWebContext.addResponseCsrfCookie() + akkaWebContext.addResponseSessionCookie() result } } diff --git a/src/main/scala/com/stackstate/pac4j/AkkaHttpWebContext.scala b/src/main/scala/com/stackstate/pac4j/AkkaHttpWebContext.scala index d3c1319..b8246f3 100644 --- a/src/main/scala/com/stackstate/pac4j/AkkaHttpWebContext.scala +++ b/src/main/scala/com/stackstate/pac4j/AkkaHttpWebContext.scala @@ -40,6 +40,7 @@ class AkkaHttpWebContext(val request: HttpRequest, .find(_.name == sessionCookieName) .map(_.value) .filter(_.nonEmpty) + .find(session => sessionStorage.sessionExists(session)) def getOrCreateSessionId(): String = { val newSession = diff --git a/src/test/scala/com/stackstate/pac4j/AkkaHttpSecurityTest.scala b/src/test/scala/com/stackstate/pac4j/AkkaHttpSecurityTest.scala index cf97494..cd4c515 100644 --- a/src/test/scala/com/stackstate/pac4j/AkkaHttpSecurityTest.scala +++ b/src/test/scala/com/stackstate/pac4j/AkkaHttpSecurityTest.scala @@ -264,6 +264,72 @@ class AkkaHttpSecurityTest extends AnyWordSpecLike with Matchers with ScalatestR responseAs[String] shouldBe "called!" } } + + + "run the callbackLogic should not send back a sessionId if the set csrf cookie is false" in { + val config = new Config() + val existingContext = AkkaHttpWebContext(HttpRequest(), Seq.empty, new ForgetfulSessionStorage, AkkaHttpWebContext.DEFAULT_COOKIE_NAME) + + val actionAdapter = new HttpActionAdapter[HttpResponse, AkkaHttpWebContext] { + override def adapt(code: HttpAction, context: AkkaHttpWebContext): HttpResponse = ??? + } + config.setHttpActionAdapter(actionAdapter) + config.setCallbackLogic(new AkkaHttpCallbackLogic { + override def perform(context: AkkaHttpWebContext, + config: Config, + httpActionAdapter: HttpActionAdapter[Future[RouteResult], AkkaHttpWebContext], + defaultUrl: String, + saveInSession: lang.Boolean, + multiProfile: lang.Boolean, + renewSession: lang.Boolean, + client: String): Future[RouteResult] = { + Future.successful(Complete(HttpResponse(StatusCodes.OK, entity = "called!"))) + } + }) + + val akkaHttpSecurity = new AkkaHttpSecurity(config, new ForgetfulSessionStorage) + + Get("/") ~> akkaHttpSecurity + .callback("/blaat", saveInSession = false, multiProfile = false, Some("Yooo"), existingContext = Some(existingContext), setCsrfCookie = false) ~> check { + // Session Store is empty so `addResponseSessionCookie` will create a token that will expire immediately + header("Set-Cookie").get.value().contains("AkkaHttpPac4jSession=; Max-Age=0;") shouldBe true + + } + } + + "run the callbackLogic should send back a sessionId if the csrf cookie is true" in { + val config = new Config() + val existingContext = AkkaHttpWebContext(HttpRequest(), Seq.empty, new InMemorySessionStorage(3.minutes), AkkaHttpWebContext.DEFAULT_COOKIE_NAME) + + val actionAdapter = new HttpActionAdapter[HttpResponse, AkkaHttpWebContext] { + override def adapt(code: HttpAction, context: AkkaHttpWebContext): HttpResponse = ??? + } + config.setHttpActionAdapter(actionAdapter) + config.setCallbackLogic(new AkkaHttpCallbackLogic { + override def perform(context: AkkaHttpWebContext, + config: Config, + httpActionAdapter: HttpActionAdapter[Future[RouteResult], AkkaHttpWebContext], + defaultUrl: String, + saveInSession: lang.Boolean, + multiProfile: lang.Boolean, + renewSession: lang.Boolean, + client: String): Future[RouteResult] = { + Future.successful(Complete(HttpResponse(StatusCodes.OK, entity = "called!"))) + } + }) + + val akkaHttpSecurity = new AkkaHttpSecurity(config, new InMemorySessionStorage(3.minutes)) + Get("/") ~> akkaHttpSecurity + .callback("/blaat", saveInSession = false, multiProfile = false, Some("Yooo"), existingContext = Some(existingContext), setCsrfCookie = true) ~> check { + headers.size shouldBe 2 + val localHeaders: Seq[HttpHeader] = headers + val threeMinutesInSeconds = 180 + // When `addResponseCsrfCookie` is called the method `getOrCreateSessionId` is called which creates a Session + // when `addResponseSessionCookie` is called there is already a session so a cookie with value is added. + localHeaders.find(_.value().contains("pac4jCsrfToken")).get.value().contains(s"Max-Age=$threeMinutesInSeconds;") shouldBe true + localHeaders.find(_.value().contains("AkkaHttpPac4jSession")).get.value().contains(s"Max-Age=$threeMinutesInSeconds;") shouldBe true + } + } } "AkkaHttpSecurity.authorize" should { diff --git a/src/test/scala/com/stackstate/pac4j/AkkaHttpWebContextTest.scala b/src/test/scala/com/stackstate/pac4j/AkkaHttpWebContextTest.scala index 32fccd7..5ece332 100644 --- a/src/test/scala/com/stackstate/pac4j/AkkaHttpWebContextTest.scala +++ b/src/test/scala/com/stackstate/pac4j/AkkaHttpWebContextTest.scala @@ -266,6 +266,28 @@ class AkkaHttpWebContextTest extends AnyWordSpecLike with Matchers { webContext.getChanges.cookies shouldBe List(validCookie) } + + "when getSessionId is called and the session exists in the store should return it" in withContext( + cookies = List(Cookie(AkkaHttpWebContext.DEFAULT_COOKIE_NAME, "validId")), + sessionStorage = new ForgetfulSessionStorage { + override val sessionLifetime = 3.seconds + + override def sessionExists(sessionKey: SessionKey): Boolean = true + } + ) { webContext => + webContext.getSessionId shouldBe Some("validId") + } + + "when getSessionId is called and the session doesn't exists in the store should return None" in withContext( + cookies = List(Cookie(AkkaHttpWebContext.DEFAULT_COOKIE_NAME, "notValidAnyMore")), + sessionStorage = new ForgetfulSessionStorage { + override val sessionLifetime = 3.seconds + + override def sessionExists(sessionKey: SessionKey): Boolean = false + } + ) { webContext => + webContext.getSessionId shouldBe None + } } def withContext(requestHeaders: List[(String, String)] = List.empty,