WW-4805 Blocks ognl access to class members of Spring proxy

[yasserzamani]

Fixes what I sent to [email protected] (3/15/2017)

[aleksandr-m]

What about performance?

[yasserzamani]

I think it's not a problem. It's complexity is O(1) when target is not a Spring proxy and O(n) where n is MAX(number of declared methods/fields, inheritances, interfaces or inner proxies) which are usually small.

[yasserzamani]

However, in my next commit, I try to check if member is belong to Spring, which decreases search space and improves performance. Currently it checks if member is not belong to user's config time action class.

[aleksandr-m]

But isAccessible is called quite often even for rather simple actions.

Also it would be nice to have some property which allows to access proxy members.

[yasserzamani]

But isAccessible is called quite often even for rather simple actions.

So what do you recommend? Adding a cache? or something else you think about.

Also it would be nice to have some property which allows to access proxy members.

It's not hard to have but I could not think about any use case of accessing/modifying proxy members by USER. Have you any specific use case in your mind? If no, I think we can postpone this until users demand.

[yasserzamani]

*I forgot to emphasis that after my third commit, isProxyMember just have to search two Spring interfaces which includes a few members.

[lukaszlenart]

The case with those changes is that they will affect everyone even if you don't use Spring so its scope should be narrowed just to the Spring Plugin. The simplest solution is to add a flag, a constant that by default should turn off this check, but the Spring Plugin should have this flag set on to enable additional scanning.

The ultimate solution would be a voter mechanism injectable by the internal DI mechanism but this requires a bit more work.

[aleksandr-m]

@lukaszlenart Sounds good. Still, it would be nice to allow to turn this checking completely off even when spring plugin is presented. The issue then can be avoided with addition of a simple pattern which should be faster.

[lukaszlenart]

With the flag in place you can always disable it in your {{struts.xml}} event it the Spring Plugin is present.

[yasserzamani]

Thank you @lukaszlenart , I got your point but what about when user uses Spring but not S2's Spring Plugin? i.e. when user does not want to define his/her actions as Spring beans but wants to use AOP on them.

However, now, after my forth commit, I don't think we should be worry. I tested WW-4805's scenario heavily with hundreds concurrent users via JMeter while profiling via YourKit. All of ProxyUtil methods just consume 186ms of the whole execution time, 137000ms, i.e. 0.1% ~= 0%. Before caching it was 7%.

[lukaszlenart]

Yeah I understand but still this affects non-Spring users. And I think this can go in as is and we can improve and think about the Voters mechanism in 2.6.

[lukaszlenart]

If no objections I am going to merge this PR, btw. I have created a task to implement Voters
https://issues.apache.org/jira/browse/WW-4807

[cnenning]

IMO this can be merged

[lukaszlenart]

@yasserzamani do you want to port some of those changes to 2.3.33? Or at least implement what @aleksandr-m mentioned in a comment?

[yasserzamani]

@lukaszlenart , Yes with pleasure. I should come with a new PR but on branch support-2-3, right?

[lukaszlenart]

Yes, you must branch off from the support-2-3 branch and open a PR against that branch after all

[aleksandr-m]

@lukaszlenart

The simplest solution is to add a flag, a constant that by default should turn off this check, but the Spring Plugin should have this flag set on to enable additional scanning.

I'll create a PR implementing this in a few days.