Spring Security
  1. Spring Security
  2. SEC-1665

AspectJMethodSecurityInterceptor need to be enhanced to be able to intercept private methods

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Complete
    • Affects Version/s: 3.0.5
    • Fix Version/s: 3.1.0.RC2
    • Component/s: Core
    • Labels:
      None
    • Environment:
      Sun JDK 1.6_22, Windows XP SP2

      Description

      Folks ,
      I'm not sure if this is a bug or an improvement , I initially make it as a Bug , please excuse me if this is against the Spring work as designed context.

      the issue is like the following :
      1-I had developped an AspectJ Aspect which is suppose to match methods annotated with @RolesAllowed annotation.

      2-after compiling the classes with Maven aspectJ plugin, and configuring the spring security to run in aspectj mode , the classes are weawed (compile time ).

      3-the aim of enabling aspectj mode is ,to avoid going throw the proxy that the default Spring aop alliance uses , so then , internal method calls could be intercepted and not gone throw the proxy .

      4-let's take a simple test case :
      @RolesAllowed("NON_PREMIUM_USER")
      public void SecMethA() {

      SecMethB();
      }

      @RolesAllowed("PREMIUM_USER")
      public void SecMethB() {
      System.out.println("I'm safe, I do not need to worry ");
      }

      => if a user having the Role NON_PREMIUM_USER , and calls SecMethA(),every thing works as expected ,and the aspect is matched, than Spring security throws an accessDenied exception .

      but when I switch the type of SecMethB() to private , I have the below exception :
      java.lang.IllegalArgumentException: Could not obtain target method from JoinPoint: 'execution(void test.SecMethB())'
      at org.springframework.util.Assert.notNull(Assert.java:112)
      at org.springframework.security.access.intercept.aspectj.MethodInvocationAdapter.<init>(MethodInvocationAdapter.java:38)
      at org.springframework.security.access.intercept.aspectj.AspectJMethodSecurityInterceptor.invoke(AspectJMethodSecurityInterceptor.java:27)

      I suppose the

        Activity

        Hide
        Christopher G. Stach II added a comment -

        This is definitely a problem, as it also breaks protected methods. In quite a few patterns, it's not unreasonable to put your secured code in a protected abstract method implementation. I recently switched a project to use the AspectJ mode and various bugs appeared due to this. The worst part of this is that it works without the AspectJ mode, and AspectJ is capable of instrumenting protected and private methods, so there is absolutely no reason why the behavior should change between modes. It should behave consistently, and for those of us who have to use the AspectJ mode, this is a major framework defect that we'll either need to patch around or refactor a lot of code that really shouldn't need to be.

        Show
        Christopher G. Stach II added a comment - This is definitely a problem, as it also breaks protected methods. In quite a few patterns, it's not unreasonable to put your secured code in a protected abstract method implementation. I recently switched a project to use the AspectJ mode and various bugs appeared due to this. The worst part of this is that it works without the AspectJ mode, and AspectJ is capable of instrumenting protected and private methods, so there is absolutely no reason why the behavior should change between modes. It should behave consistently, and for those of us who have to use the AspectJ mode, this is a major framework defect that we'll either need to patch around or refactor a lot of code that really shouldn't need to be.
        Hide
        Luke Taylor added a comment -

        I don't mind applying a patch to check for private/protected methods, since it would be a passive change.

        However, the usage patterns between AspectJ and proxy based AOP are different so you should not expect to be able to switch from one to the other without changes. For example, it's common to annotate an interface with proxy-based AOP, but AspectJ requires that annotations at class level and will completely ignore any on interfaces. It doesn't inherit annotations from superclass methods either. Also internal method calls would never be intercepted with a proxy. So you should always expect potential changes in behaviour.

        Could you clarify what you mean by "it works without AspectJ mode"? I'm not clear how, if you are using a proxy, you can intercept private methods.

        Show
        Luke Taylor added a comment - I don't mind applying a patch to check for private/protected methods, since it would be a passive change. However, the usage patterns between AspectJ and proxy based AOP are different so you should not expect to be able to switch from one to the other without changes. For example, it's common to annotate an interface with proxy-based AOP, but AspectJ requires that annotations at class level and will completely ignore any on interfaces. It doesn't inherit annotations from superclass methods either. Also internal method calls would never be intercepted with a proxy. So you should always expect potential changes in behaviour. Could you clarify what you mean by "it works without AspectJ mode"? I'm not clear how, if you are using a proxy, you can intercept private methods.
        Hide
        Anis Moussa added a comment -

        I completely agree with Luke, Internal methods will not be intercepted by a proxy.
        But what seems to be strange for me(may be out of the scope of this ticket), is that Spring AOP does not weave aspects that are already compiled with AspectJ compiler(CTW) ,It needs at least aspects that are compiled with a plain Java compiler ...is it a WAD?
        But I still believe that enhancing the current AspectJmethodSecurityIntercepter to handle also private/protected method, is a good approach and it does not need much effort and it does not broke this feature.
        I had tested the patch that my colleague Torben posted few weeks ago , and it does work. Luke, did you had the opportuinity to look at it .?

        Show
        Anis Moussa added a comment - I completely agree with Luke, Internal methods will not be intercepted by a proxy. But what seems to be strange for me(may be out of the scope of this ticket), is that Spring AOP does not weave aspects that are already compiled with AspectJ compiler(CTW) ,It needs at least aspects that are compiled with a plain Java compiler ...is it a WAD? But I still believe that enhancing the current AspectJmethodSecurityIntercepter to handle also private/protected method, is a good approach and it does not need much effort and it does not broke this feature. I had tested the patch that my colleague Torben posted few weeks ago , and it does work. Luke, did you had the opportuinity to look at it .?
        Hide
        Luke Taylor added a comment -

        I already committed some changes to look for non-public methods in addition to public ones.

        Show
        Luke Taylor added a comment - I already committed some changes to look for non-public methods in addition to public ones.
        Hide
        Christopher G. Stach II added a comment -

        Luke, in my most obvious example of breakage, there is are template superclasses which handle exporting data in certain formats and subclasses which implement the protected abstract methods for fetching and formatting the data. Those protected methods were marked with @Secured/@PreAuthorize and worked in the standard mode, so that is what I meant, not intercepting private methods. (We're in agreement that private methods aren't as big of a concern, but they can work with AspectJ.) I've since patched MethodInvocationAdapter, but thanks for fixing the next version.

        Show
        Christopher G. Stach II added a comment - Luke, in my most obvious example of breakage, there is are template superclasses which handle exporting data in certain formats and subclasses which implement the protected abstract methods for fetching and formatting the data. Those protected methods were marked with @Secured/@PreAuthorize and worked in the standard mode, so that is what I meant, not intercepting private methods. (We're in agreement that private methods aren't as big of a concern, but they can work with AspectJ.) I've since patched MethodInvocationAdapter, but thanks for fixing the next version.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Anis Moussa
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: