Skip to content

Commit

Permalink
Fix bug #710, matchers ignored (#728)
Browse files Browse the repository at this point in the history
* Precompile regex

* coalesce adjacent rules so that matchers behave properly

* use List explicitly

* Preserve case class member name for bincompat
  • Loading branch information
reardonj authored Dec 3, 2024
1 parent 5e909b1 commit 1d018b7
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 43 deletions.
80 changes: 50 additions & 30 deletions shared/src/main/scala/org/pac4j/play/filters/SecurityFilter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -67,55 +68,62 @@ 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)

val parameters = new PlayFrameworkParameters(request)
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 = {
Expand All @@ -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 = {
Expand All @@ -158,6 +178,6 @@ object SecurityFilter {
}
}

Rule(path.replace("\"", ""), ruleData)
Rule(path.replace("\"", ""), ruleData.toList)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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")))
)
}

Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 1d018b7

Please sign in to comment.