From 0a17d967d10ce454bb362f2bd60c73ca7695fc0b Mon Sep 17 00:00:00 2001 From: Justin Reardon Date: Fri, 29 Nov 2024 20:24:17 -0500 Subject: [PATCH 1/2] Fix matcher handling - Precompile regex - coalesce adjacent rules so that matchers behave properly --- .../pac4j/play/filters/SecurityFilter.scala | 82 ++++++++++++------- .../play/filters/SecurityFilterTests.scala | 72 +++++++++++++--- 2 files changed, 110 insertions(+), 44 deletions(-) diff --git a/shared/src/main/scala/org/pac4j/play/filters/SecurityFilter.scala b/shared/src/main/scala/org/pac4j/play/filters/SecurityFilter.scala index 06ba62d0..cabd553f 100644 --- a/shared/src/main/scala/org/pac4j/play/filters/SecurityFilter.scala +++ b/shared/src/main/scala/org/pac4j/play/filters/SecurityFilter.scala @@ -17,6 +17,7 @@ import javax.inject.{Inject, Singleton} import scala.compat.java8.FutureConverters.CompletionStageOps import scala.concurrent.{ExecutionContext, Future} import scala.util.Failure +import scala.util.matching.Regex /** * Filter on all requests to apply security by the Pac4J framework. @@ -67,18 +68,23 @@ class SecurityFilter @Inject()(configuration: Configuration, config: Config) override def apply(nextFilter: RequestHeader => Future[play.api.mvc.Result]) (request: RequestHeader): Future[play.api.mvc.Result] = { - findRule(request).flatMap(_.data) match { - case Some(rule) => + findRule(request).map(_.data) match { + case Some(rule :: remainingRules) => log.debug(s"Authentication needed for ${request.uri}") - proceedRuleLogic(nextFilter, request, rule) + proceedRuleLogic(nextFilter, request, rule, remainingRules) - case None => + case _ => log.debug(s"No authentication needed for ${request.uri}") nextFilter(request) } } - private def proceedRuleLogic(nextFilter: RequestHeader => Future[Result], request: RequestHeader, rule: RuleData): Future[Result] = { + private def proceedRuleLogic( + nextFilter: RequestHeader => Future[Result], + request: RequestHeader, + rule: RuleData, + remainingRules: Seq[RuleData] + ): Future[Result] = { FrameworkAdapter.INSTANCE.applyDefaultSettingsIfUndefined(config) @@ -86,36 +92,38 @@ class SecurityFilter @Inject()(configuration: Configuration, config: Config) val webContext = config.getWebContextFactory().newContext(parameters).asInstanceOf[PlayWebContext] val securityAction = new SecureAction(config) - def calculateResult(secureActionResult: mvc.Result): Future[Result] = { - if (secureActionResult.isInstanceOf[PlayWebContextResultHolder]) { - val newCtx = secureActionResult.asInstanceOf[PlayWebContextResultHolder].getPlayWebContext - val newRequest = newCtx.supplementRequest(request.asJava).asScala - nextFilter(newRequest) - } else { - // When the user is not authenticated, the result is one of the following: - // - forbidden - // - redirect to IDP - // - unauthorized - // Or the future results in an exception - Future { - log.info(s"Authentication failed for ${request.uri} with clients ${rule.clients} and authorizers ${rule.authorizers} and matchers ${rule.matchers}. Authentication response code ${secureActionResult.status}.") - secureActionResult.asScala - } - } - } - - val futureResult: Future[Result] = + def checkSecurity(request: RequestHeader, rule: RuleData, remainingRules: Seq[RuleData]): Future[Result] = securityAction .call(parameters, rule.clients, rule.authorizers, rule.matchers) - .toScala - .flatMap[Result](calculateResult) + .asScala + .flatMap { secureActionResult => + if (secureActionResult.isInstanceOf[PlayWebContextResultHolder]) { + val newCtx = secureActionResult.asInstanceOf[PlayWebContextResultHolder].getPlayWebContext + val newRequest = newCtx.supplementRequest(request.asJava).asScala + + remainingRules match { + case Nil => nextFilter(newRequest) + case head :: tail => checkSecurity(newRequest, head, tail) + } + } else { + // When the user is not authenticated, the result is one of the following: + // - forbidden + // - redirect to IDP + // - unauthorized + // Or the future results in an exception + Future.successful { + log.info(s"Authentication failed for ${request.uri} with clients ${rule.clients} and authorizers ${rule.authorizers} and matchers ${rule.matchers}. Authentication response code ${secureActionResult.status}.") + secureActionResult.asScala + } + } + } - futureResult.andThen { case Failure(ex) => log.error("Exception during authentication procedure", ex) } + checkSecurity(request, rule, remainingRules).andThen { case Failure(ex) => log.error("Exception during authentication procedure", ex) } } private def findRule(request: RequestHeader): Option[Rule] = { val pathNormalized = getNormalizedPath(request) - rules.find(rule => pathNormalized.matches(rule.pathRegex)) + rules.find(rule => rule.compiledRegex.matches(pathNormalized)) } private def getNormalizedPath(request: RequestHeader): String = { @@ -132,13 +140,25 @@ class SecurityFilter @Inject()(configuration: Configuration, config: Config) } object SecurityFilter { - private[filters] case class Rule(pathRegex: String, data: Option[RuleData]) + private[filters] case class Rule(pathRegex: String, data: List[RuleData]) { + val compiledRegex = pathRegex.r + + def mergeData(other: Rule) = this.copy(data = this.data ++ other.data) + } private[filters] case class RuleData(clients: String, authorizers: String, matchers: String) private[filters] def loadRules(configuration: Configuration): Seq[Rule] = { val ruleConfigs = configuration.getOptional[Seq[Configuration]]("pac4j.security.rules").getOrElse(Seq()) - ruleConfigs.map(convertConfToRule) + ruleConfigs + .map(convertConfToRule) + // coalesce adjacent rules with the exact same path + .foldLeft(List.empty[Rule]) { + case (Nil, rule) => List(rule) + case (head :: tail, rule) if head.pathRegex == rule.pathRegex => head.mergeData(rule) :: tail + case (list, rule) => rule :: list + } + .reverse } private def convertConfToRule(conf: Configuration): Rule = { @@ -158,6 +178,6 @@ object SecurityFilter { } } - Rule(path.replace("\"", ""), ruleData) + Rule(path.replace("\"", ""), ruleData.toList) } } diff --git a/shared/src/test/java/org/pac4j/play/filters/SecurityFilterTests.scala b/shared/src/test/java/org/pac4j/play/filters/SecurityFilterTests.scala index a6b738aa..78b8c8fe 100644 --- a/shared/src/test/java/org/pac4j/play/filters/SecurityFilterTests.scala +++ b/shared/src/test/java/org/pac4j/play/filters/SecurityFilterTests.scala @@ -35,10 +35,10 @@ class SecurityFilterTests extends ScalaFutures with Results { val config: Configuration = new Configuration(ConfigFactory.load("config/security_filter.conf")) SecurityFilter.loadRules(config) shouldBe Seq( - Rule("/path_anonymous", Some(RuleData("AnonymousClient", null, null))), - Rule("/path_secure_1", Some(RuleData("client1,client2", null, null))), - Rule("/path_secure_3", Some(RuleData(null, "authorizer1,authorizer2", null))), - Rule("/path_secure_4", Some(RuleData("client1,client2", "authorizer1,authorizer2", "matcher1,matcher2"))) + Rule("/path_anonymous", List(RuleData("AnonymousClient", null, null))), + Rule("/path_secure_1", List(RuleData("client1,client2", null, null))), + Rule("/path_secure_3", List(RuleData(null, "authorizer1,authorizer2", null))), + Rule("/path_secure_4", List(RuleData("client1,client2", "authorizer1,authorizer2", "matcher1,matcher2"))) ) } @@ -69,19 +69,65 @@ class SecurityFilterTests extends ScalaFutures with Results { """.stripMargin ) - def tryFilterApply(path: String): Future[Result] = { - val nextFilter = (_: RequestHeader) => Future.successful(Ok("ok")) - val testRequest: RequestHeader = FakeRequest(POST, path) - securityFilter.apply(nextFilter)(testRequest) - } + status(tryFilterApply(securityFilter, "/path_secure", POST)) shouldBe 401 + status(tryFilterApply(securityFilter, "/path_secure_2/any_path_", POST)) shouldBe 401 - status(tryFilterApply("/path_secure")) shouldBe 401 - status(tryFilterApply("/path_secure_2/any_path_")) shouldBe 401 + status(tryFilterApply(securityFilter, "/path_anonymous", POST)) shouldBe 200 + status(tryFilterApply(securityFilter, "any/other/path", POST)) shouldBe 200 + } - status(tryFilterApply("/path_anonymous")) shouldBe 200 - status(tryFilterApply("any/other/path")) shouldBe 200 + @Test + def testThatSecurityFilterBlocksUnauthorizedRequestsWithMatchers(): Unit = { + implicit val ec = scala.concurrent.ExecutionContext.global + implicit val as = ActorSystem("text-actor-system") + implicit val mat: ActorMaterializer = ActorMaterializer() + + val securityFilter = prepareSecurityFilter( + """ + |pac4j.security.rules = [ + | { + | "/path_secure" = { + | "clients" = "AnonymousClient" + | "authorizers" = "none" + | "matchers" = "put" + | } + | }, + | { + | "/path_secure" = { + | "clients" = "AnonymousClient" + | "authorizers" = "none" + | "matchers" = "get" + | } + | }, { + | "/path_secure" = { + | clients = "client1" + | "matchers" = "post" + | } + | }, { + | "/path_secure" = { + | "clients" = "AnonymousClient" + | } + | }, { + | "/path_secure/deeper" = { + | clients = "client1" + | } + | } + |] + """.stripMargin + ) + + status(tryFilterApply(securityFilter, "/path_secure", GET)) shouldBe 200 + status(tryFilterApply(securityFilter, "/path_secure", POST)) shouldBe 401 + status(tryFilterApply(securityFilter, "/path_secure/deeper", GET)) shouldBe 401 + status(tryFilterApply(securityFilter, "/path_secure/deeper", POST)) shouldBe 401 } + private def tryFilterApply(securityFilter: SecurityFilter, path: String, method: String): Future[Result] = { + val nextFilter = (_: RequestHeader) => Future.successful(Ok("ok")) + val testRequest: RequestHeader = FakeRequest(method, path) + securityFilter.apply(nextFilter)(testRequest) + } + private def prepareSecurityFilter(configString: String) (implicit ec: ExecutionContext, mat: Materializer): SecurityFilter = { val pac4jConfig = new Config From f7cf2c656fc0ab8c5a13bd1eb96958c5d8f413e4 Mon Sep 17 00:00:00 2001 From: Justin Reardon Date: Tue, 3 Dec 2024 08:06:47 -0500 Subject: [PATCH 2/2] fix scala 2.12 compat issues --- .../main/scala/org/pac4j/play/filters/SecurityFilter.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/shared/src/main/scala/org/pac4j/play/filters/SecurityFilter.scala b/shared/src/main/scala/org/pac4j/play/filters/SecurityFilter.scala index cabd553f..4c8aac07 100644 --- a/shared/src/main/scala/org/pac4j/play/filters/SecurityFilter.scala +++ b/shared/src/main/scala/org/pac4j/play/filters/SecurityFilter.scala @@ -94,8 +94,8 @@ class SecurityFilter @Inject()(configuration: Configuration, config: Config) def checkSecurity(request: RequestHeader, rule: RuleData, remainingRules: Seq[RuleData]): Future[Result] = securityAction - .call(parameters, rule.clients, rule.authorizers, rule.matchers) - .asScala + .call(parameters, rule.clients, rule.authorizers, rule.matchers) + .toScala .flatMap { secureActionResult => if (secureActionResult.isInstanceOf[PlayWebContextResultHolder]) { val newCtx = secureActionResult.asInstanceOf[PlayWebContextResultHolder].getPlayWebContext @@ -123,7 +123,7 @@ class SecurityFilter @Inject()(configuration: Configuration, config: Config) private def findRule(request: RequestHeader): Option[Rule] = { val pathNormalized = getNormalizedPath(request) - rules.find(rule => rule.compiledRegex.matches(pathNormalized)) + rules.find(rule => rule.compiledRegex.pattern.matcher(pathNormalized).matches) } private def getNormalizedPath(request: RequestHeader): String = {