Spring Security
  1. Spring Security
  2. SEC-1525

PostAuthorize being evaluated even on exception

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.3
    • Fix Version/s: 3.1.0.M2
    • Component/s: Core
    • Labels:
      None

      Description

      Given the code :
      @PostAuthorize(hasPermission(returnObject.field, 'admin'))
      MyObject someMethod();

      when someMethod throws an exception, the PostAuthorize processing throws an exception :
      org.springframework.expression.spel.SpelEvaluation Exception: EL1007Epos 27): Field or property 'field' cannot be found on null

      IMHO, the PostAuthorize should not be evaluated when returnObject is null (exactly like PostFilter)
      This is possible if we consider that PostAuthorize is a nonsense if you do not use the returnObject inside the expression (and IMHO this consideration is true because if you do not use returnObject in the expression then you could have use PreAuthorize...)

      By code inspection, the problem comes from ExpressionBasedPostInvocationAdvice :

      when you do :
      if (postAuthorize != null && !ExpressionUtils.evaluateAsBoolean(postAuthorize, ctx))

      you should have done :
      if (postAuthorize != null && returnedObject != null && !ExpressionUtils.evaluateAsBoolean(postAuthorize, ctx))

      BTW, this is exactly what you do in PostFilter 10 lines above...

        Issue Links

          Activity

          Hide
          Luke Taylor added a comment -

          I disagree. I don't think it's "a nonsense" to consider the possibility that there may be use cases where the authorization decision can only realistically be made after the method invocation has taken place, and which may not require the use of the returned object (or even have one).

          I'm not sure what the best option is for dealing gracefully with an exception condition, though. It might make more sense if the MethodSecurityInterceptor didn't actually perform any after-invocation checks at all.

          Show
          Luke Taylor added a comment - I disagree. I don't think it's "a nonsense" to consider the possibility that there may be use cases where the authorization decision can only realistically be made after the method invocation has taken place, and which may not require the use of the returned object (or even have one). I'm not sure what the best option is for dealing gracefully with an exception condition, though. It might make more sense if the MethodSecurityInterceptor didn't actually perform any after-invocation checks at all.
          Hide
          christophe blin added a comment -

          At the moment, from the moment the annotated method may throw an exception (i.e > 90% of the cases), the current implementation requires that the programmer writes PostAuthorize like this :
          @PostAuthorize("returnObject == null or returnObject.res.libraryId == #libraryId")

          If he does not, a NPE will be thrown when the exception occurs...

          Please, provide a use case where the user can not rewrite its PostAuthorize with a PreAuthorize when the returnObject is not used so that I am comfortable with the idea that I do not write all this "returnObject == null or " for nothing

          Show
          christophe blin added a comment - At the moment, from the moment the annotated method may throw an exception (i.e > 90% of the cases), the current implementation requires that the programmer writes PostAuthorize like this : @PostAuthorize("returnObject == null or returnObject.res.libraryId == #libraryId") If he does not, a NPE will be thrown when the exception occurs... Please, provide a use case where the user can not rewrite its PostAuthorize with a PreAuthorize when the returnObject is not used so that I am comfortable with the idea that I do not write all this "returnObject == null or " for nothing
          Hide
          christophe blin added a comment -

          and please, if the implementation stays the same, update the documentation to mention that returnObject is null when the method has thrown an exception so that programmers know they have this test to add to each and every PostAuthorize of their app

          Show
          christophe blin added a comment - and please, if the implementation stays the same, update the documentation to mention that returnObject is null when the method has thrown an exception so that programmers know they have this test to add to each and every PostAuthorize of their app
          Hide
          Luke Taylor added a comment -
          Show
          Luke Taylor added a comment - See SEC-1635

            People

            • Assignee:
              Luke Taylor
              Reporter:
              christophe blin
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: