Security Filter ignores configured matchers when deciding on a rule

[TWJoachim]

Hello,

its basically the same issue from here: https://groups.google.com/g/pac4j-users/c/vCZ9-YItIUg .When the security filter config for pac4j defines two rules that would match the same request, but uses "matchers" (e.g. HttpMethodMatcher, one POST, one GET) to define a more specific rule matching behavior, the current org.pac4j.play.filters.SecurityFilter ignores this in its "apply" method.

The logic in "findRule" does only apply regex matching on the path and is not taking any matchers into account. So the matchers are evaluated later when the chosen endpoint is executed - but to my understanding of the docs they should also be relevant in choosing the right rule.

(see https://github.com/pac4j/play-pac4j/blob/master/shared/src/main/scala/org/pac4j/play/filters/SecurityFilter.scala , method "private def findRule(..)"

Thanks a lot,
Joachim

[leleuj]

Indeed, there is a flaw here, only the path is taken into account. This should be fixed. As I said, I'm not fluent in Scala. A pull request is welcome here. Thanks

[TWJoachim]

I'm also not fluent in Scala. Has the Play-Pac4j integration active maintainers? If not, I can close this issue (and we may look into rewriting the SecurityFilter in Java).

[leleuj]

I'm the main maintainer and there are also occasional contributors. But unfortunately, I'm not fluent in Scala either.

Rewriting the filter in Java is a great idea (as filters can be used both in Java and Scala), it would ease its maintenance. Keep the issue open if you intend to do so.

[reardonj]

I just came across this issue trying to set up a security filter. I can look into this. I know Scala.

[leleuj]

Sure. Your help will be appreciated.

[reardonj]

I'd guess noone is actually using the matchers config since it doesn't work. But how can/should the semantics change?

I'm a little afraid of having it fall-through to the next match if the matchers don't match, since that could change behavior of current users. The docs aren't explicit on what happens with a non-matching matchers, but falling through feels like the intent.

[reardonj]

Ah, I see. The matchers do work. If the matcher fails, the security rules do not apply. ie. if my config is like

{"/test" = { 
  authorizers = "testRole"
  matchers = "post"
}}

and I send a GET request without being authenticated, I can access the route. If I send an unauthenticated POST request, it fails.

Unfortunately, SecureAction doesn't tell me if the request succeeded because of matchers or auth. Someone could be using matchers successfully right now. I don't see a way to change this behavior without potentially changing the semantics of someone's security config.

However, I would really like some way to select rules based on matchers. My needs are similar to the google groups post. I have RESTful endpoints that should require different authorization for POST vs GET, but both must be authenticated. To do this safely, I think we would have to add another property (eg. filterMatchers) to allow checking the matchers before settling on which rule to use.

Does that make sense or am I missing something? I'm more familiar with Scala and Play than pac4j.

[leleuj]

I think we should be able to define the same URL several times with different configurations. Like:

{
  "/test" = { 
    authorizers = "testRole"
    matchers = "post"
  },
  "/test" = { 
    authorizers = "testSomethingElse"
    matchers = "get"
  }
}

[reardonj]

So we would have it evaluate all configs with the same regex, and fail at the first failure, and pass if all succeed?

[leleuj]

I think so. All should succeed.

[TWJoachim]

I cant share our workaround, but we basically have our own Filter in Java and dont use the Scala SecurityFilter any more. We made up a new attribute "methods" (instead of "matchers") and that is a string value of possibly comma separated http methods (e.g. "post,put").

We then found this behavior logical for us:

• first pass: scan all rules from top to bottom if there is a matching (meaning matching by regex) rule and that rule declares a 'methods' attribute and the current request's http method is contained in the declared 'methods' of that rule. If there is one rule like that - we pick the first one and depending on the outcome of the authorizer this might succeed or fail.

• if the first pass didnt find any matching rule, then a second pass looks for matching (meaning matching by regex) rules that are method agnostic, meaning they dont declare any 'methods'. We here again, take the first found and run the authorizer for the outcome.

So this would mean for a config like:

{
"/test" = {
authorizers = "testRole1"
},
"/test" = {
authorizers = "testRole2"
methods= "post"
},
"/test" = {
authorizers = "testRole3"
methods= "get,post"
}
}

• "GET /test" would hit authorizer "testRole3"
• "PUT /test" would hit authorizer "testRole1"
• "POST /test" would hit authorizer "testRole2"

Lastly we optimized the regex usage. The original Scala SecurityFilter does in "findRule(..)" this:

"pathNormalized.matches(rule.pathRegex)"

.. and this is basically "String.matches(..)" which compiles the regex pattern on every invocation ad-hoc again. So we cache the Pattern object on creation and our variant of "findRule" then re-uses the Java Pattern objects.

[leleuj]

I just cut the releases and the new versions 12.0.1-PLAYx.y should be available in the Maven central repo soon.