Ensure that Security annotation are always placed on non-final methods #5089
Conversation
We need security annotations to result in the creation of an interceptor which is not possible when a method is final. The default behavior of Arc to simply warn about the final method is not good enough for security annotations because it could cause methods that should have been secure to not be by a simple oversight. Fixes: quarkusio#5051
geoand
changed the title
| if (bean.isClassBean()) { | ||
| // Ensure that all security annotations are placed on non final methods | ||
| // this is done because Arc only warns about final methods not being interceptable | ||
| for (MethodInfo method : bean.getTarget().get().asClass().methods()) { |
There was a problem hiding this comment.
Inherited methods are not processed. Also iterating over all methods of all beans could be quite time-consuming (for large deployments). I'd rather move the validation to io.quarkus.arc.processor.BeanInfo.initInterceptedMethods() or somewhere "nearby".
There was a problem hiding this comment.
I pondered that option as well but went with the current solution because it seemed cleaner since it wouldn't involve baking specific security annotations inside the general Arc code.
I can certainly move things inside initInterceptedMethods but I assume that the security annotations will have to be hard coded - or at best have some kind of build item that is dedicated to this case and will propagate the information down the Arc layers.
What would be your preference?
There was a problem hiding this comment.
It might be a stupid question but why don't we throw an error for any interceptor binding annotation on final methods? I mean it won't work and could have very bad consequences. Security is one of the occurrences of this problem but there might be other.
There was a problem hiding this comment.
That's fine with me TBH. I just didn't do it because it was mentioned in the issue that we might not want that as a default.
There was a problem hiding this comment.
I think that we should modify BeanDeploymentValidator#skipValidation() to handle similar use cases...
There was a problem hiding this comment.
Really I think that automatically removing the final modifier is probably the most user friendly approach, especially for Kotlin applications.
There was a problem hiding this comment.
@stuartwdouglas are you thinking for this interceptor use case or for all types of issues we might encounter with Kotlin?
There was a problem hiding this comment.
I was just thinking for the interceptor use case.
There was a problem hiding this comment.
OK, I'll do it and we can see what @mkouba thinks :)
|
I am going to close this one as #5104 is the more accepted solution |
gsmet
added
triage/out-of-date
4 participants
We need security annotations to result in the creation of an interceptor
which is not possible when a method is final.
The default behavior of Arc to simply warn about the final method is not
good enough for security annotations because it could cause methods
that should have been secure to not be by a simple oversight.
Fixes: #5051
A note on the implementation: I went for a simple approach here instead of introducing some more build items and tying deeper into Arc since IMHO that would require us to handle other Arc warnings in a uniform manner instead of having a specific implementation for interceptors