SEC-1665: AspectJMethodSecurityInterceptor need to be enhanced to be able to intercept private methods

[spring-projects-issues]

Anis Moussa (Migrated from SEC-1665) said:

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.(MethodInvocationAdapter.java:38)
at org.springframework.security.access.intercept.aspectj.AspectJMethodSecurityInterceptor.invoke(AspectJMethodSecurityInterceptor.java:27)

I suppose the

[spring-projects-issues]

Anis Moussa said:

Continuation of the description :

I suppose MethodInvocationAdapter is actually trying to use reflection to retrieve the JoinPoint Signature and the method behind, since the method is private , the exception is thrown .

is there a way to enhance or fix this ?

MethodInvocationAdapter(JoinPoint jp) {
this.jp = (ProceedingJoinPoint)jp;
if (jp.getTarget() != null) {
target = jp.getTarget();
} else {
// SEC-1295: target may be null if an ITD is in use
target = jp.getSignature().getDeclaringType();
}
String targetMethodName = jp.getStaticPart().getSignature().getName();
Class[] types = ((CodeSignature) jp.getStaticPart().getSignature()).getParameterTypes(); Class declaringType = ((CodeSignature) jp.getStaticPart().getSignature()).getDeclaringType();

    method = ClassUtils.getMethodIfAvailable(declaringType, targetMethodName, types);
    Assert.notNull(method, "Could not obtain target method from JoinPoint: '"+ jp + "'");

}

[spring-projects-issues]

Luke Taylor said:

ClassUtils.getMethodIfAvailable() will only return public methods.

Could you explain the requirement for securing private methods? Usually only the part of the API that is exposed to external callers would be secured.

[spring-projects-issues]

Anis Moussa said:

take the below example :
@RolesAllowed("NON_PREMIUM_USER")
public void SecMethA() {

SecMethB();
}

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

if a user have only NON_PREMIUM_USER as a role ,the whole call will fail .Thus all security layer should apply only for Public method
I think this is very limiting.

is there any work arround to have this working ?

Regards
Anis

[spring-projects-issues]

Luke Taylor said:

That is just the same example you gave originally.

Can you provide an example of a real-world use-case, explaining why it cannot be accomodated using the existing support and explain in what way you find this "very limiting". To me it seems confusing to place security annotations on private methods, which are essentially an internal implementation detail of the class/interface in question. A security policy which was implemented in this way would be difficult to understand from the perspective of the actual business functionality (which is essentially defined by the public APIs).

[spring-projects-issues]

Anis Moussa said:

I feel that this is a work as designed. I'm exploring all the possibilities of securing a method. I know that conceptuality ,securing a private method is confusing. Thus a developer using an out of the box framework should be carefull while securing a peace of code taking in consideration that a private method should not be called internally with a secure scope different than the caller.
for me now,It make sence , but I'm afraid users of my framework may fall in this error,I will document it then.

I believe this discussion reached an end. Thanks a lot for ur support.

Regards
Anis

[spring-projects-issues]

Luke Taylor said:

OK, thanks. I'll close the issue then. I don't think this would represent best practice and it would also require checking private methods manually, which would require quite a bit of extra code.

[spring-projects-issues]

Torben Knerr said:

For me there are two different use cases for annotation-based security:

you want to secure public methods on the API level (this is what Luke suggests)

you want to make sure that a specific segment of code is executed in a secure context (this is what Anis seens to aim at)

Use case #1 fits very well with Spring AOP / proxying as it suggest that you only secure external method calls. Use case #2 becomes immediately valid if you switch to AspectJ AOP / weaving for the ability advise internal method calls (which is not possible with the proxy-based approaches).

Personally I believe that use case #2 is a very likely one, especially if you are using AspectJ / weaving. Securing private methods (or: "reusable code segments") with annotations is perfectly possible and IMHO intuitive as well.

Please consider re-opening the ticket.

[spring-projects-issues]

Torben Knerr said:

An Example:

• consider our public API which exposes two functions, funcA(...) and funcB(...)
• depending on the input parameters of the functions it could happen that in both cases secureFunc() is invoked, which performs a critical operation that should only be performed by users with role "ADMIN"

What should we do in this case? We can not annotate both funcA(...) and funcB(...) with @PreAuthorize("hasRole('ADMIN')") as we require this role only conditionally. Making secureFunc() public only for that reason sounds cumbersome to me.

[spring-projects-issues]

Luke Taylor said:

Feel free to submit a patch. Alternatively if you really want to do this, you can extend MethodSecurityInterceptor and supply your own version of AspectJMethodSecurityInterceptor. There's not a great deal of code in there.

[spring-projects-issues]

Torben Knerr said:

Luke,

sure, I can provide a patch for this, but what I am really interested in is an expert opinion on this.

Do you think this makes sense at all? I doubt that it would be good to include a patch if you / spring security team think that this makes no sense :-)

Maybe my thinking (i.e. belief that use case #2 is valid) is just flawed. So far I could not see that flaw, so please let me know if it is.

Best regards,
Torben

[spring-projects-issues]

Torben Knerr said:

Hi Luke,

please find below the suggested patch (couldn't figure out how to attach it):
http://pastebin.com/raw.php?i=eneHHi6a

It keeps backwards-compatibility with the existing behaviour (i.e. throwing an IllegalArgumentException when trying to intercept private methods) unless you explicitly set AspectJMethodSecurityInterceptor.setInterceptPublicMethodsOnly(false).

Let me know what you think about it. Not sure whether the default for AspectJMethodSecurityInterceptor.interceptPublicMethodsOnly should be true or false.

Best regards,
Torben

[spring-projects-issues]

Christopher G. Stach II said:

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.

[spring-projects-issues]

Luke Taylor said:

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.

[spring-projects-issues]

Anis Moussa said:

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 .?

[spring-projects-issues]

Luke Taylor said:

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