From 1d018b7fa15c89b7f21bb717763ee4a1f65fd9f5 Mon Sep 17 00:00:00 2001 From: Justin Reardon Date: Tue, 3 Dec 2024 09:04:46 -0500 Subject: [PATCH] Fix bug #710, matchers ignored (#728) * Precompile regex * coalesce adjacent rules so that matchers behave properly * use List explicitly * Preserve case class member name for bincompat --- .../pac4j/play/filters/SecurityFilter.scala | 80 ++++++++++++------- .../play/filters/SecurityFilterTests.scala | 72 ++++++++++++++--- 2 files changed, 109 insertions(+), 43 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 45386bec..df9afd32 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.jdk.FutureConverters._ 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) .asScala - .flatMap[Result](calculateResult) + .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 36f34f2d..26acac97 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 + } + + @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("/path_anonymous")) shouldBe 200 - status(tryFilterApply("any/other/path")) shouldBe 200 + 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