Back-port WW-5012 improvements from PR#323 to 2.5.x: by JCgH4164838Gh792C124B5 · Pull Request #324 · apache/struts

JCgH4164838Gh792C124B5 · 2019-01-31T23:48:53Z

Back-port WW-5012 improvements from PR#323 to 2.5.x:

Back-port improvements from PR#323:
- Re-order SecurityMemberAccess to make public access check the 1st check.
- Improvements to checkStaticMethodAccess().
Back-port improvements from PR#320 that aligned with PR#323's enhancement:
- Make one public getter final.
- Brought additional ordering improvements that align and make 2.5.x's implementation easier to maintain.
Two improvements resulted directly from the back-porting:
- Eliminated unnecessary boolean allow flag within the access check.
- Eliminated a redundant call to !isClassExcluded(memberClass), implicitly possible due to re-ordering.

- Back-port improvements from PR#323: - Re-order SecurityMemberAccess to make public access check the 1st check. - Improvements to checkStaticMethodAccess(). - Back-port improvements from PR#320 that aligned with PR#323's enhancement: - Make one public getter final. - Brought additional ordering improvements that align and make 2.5.x's implementation easier to maintain. - Two improvements resulted directly from the back-porting: - Eliminated unnecessary boolean allow flag within the access check. - Eliminated a redundant call to !isClassExcluded(memberClass), implicitly possible due to re-ordering.

- improves effective checks for the PR.

aleksandr-m · 2019-02-01T20:39:08Z

Not sure about this. Currently public check is in the super.isAccessible. Maybe we should leave it as it is for 2.5.x branch.

JCgH4164838Gh792C124B5 · 2019-02-01T21:08:28Z

Hello @aleksandr-m .
If you think that leaving the public state check until the end (where it is still called in the PR, for consistency), the new checkPublicMemberAccess() method and call to it could be removed, but the remaining improvements drawn from your PR should still be applicable. That would also avoid the possibility in inadvertently impacting something by failing-out sooner than 2.5.20- did.
What do you think ?

coveralls · 2019-02-01T22:21:29Z

Coverage decreased (-0.003%) to 46.98% when pulling b268e03 on JCgH4164838Gh792C124B5:localS2_25x_B3 into 71267a9 on apache:struts-2-5-x.

yasserzamani

I'm still not sure if it's a good idea to import improvement changes into 2.5 (@apache/struts-committers ?) but if we want to import then let import all of them which are applicable from 2.6 - I commented some of them below :)

yasserzamani · 2019-02-06T12:57:20Z

core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java

    }

-    public boolean getAllowStaticMethodAccess() {
+    public final boolean getAllowStaticMethodAccess() {


why? In general I'm curious to learn why final is usually preferred in your PRs please :)

In this particular case it is because allowStaticMethodAccess is a private field, so I don't see a valid use case for any descendant class extending this one being allowed to change the behaviour of this getter (and thereby changing the intended behaviour of anything that calls getAllowStaticMethodAccess() ).

In more general terms, for other public/protected methods where it seems reasonable to prevent descendants from changing particular behaviour (but who can still call the super method in new methods it introduces), using final makes that intent clear ... and Java will attempt to enforce it too.

For variables and attributes, declaring items final helps guard against unintentional modification elsewhere (which sometimes helps avoid bugs since the compiler catches it right away).

The above is some of my reasoning anyway ... :)

I think when a method uses a private field doesn't mean it should be final. Think about this override for example:

@override public boolean getAllowStaticMethodAccess() { return super.getAllowStaticMethodAccess() || this.getMyOwnLogic(); }

Regarding second paragraph, I learnt from @lukaszlenart that it's not a good idea for Struts to decrease public methods flexibility even if they currently don't have public usage in framework because Struts is intended to be extendable and overridable by user.

Regarding third paragraph, yes I feel it's a good idea for local variables :)

Thanks for the thoughtful and considered response. :) :)

True ... having a private field getter doesn't necessarily mean the getter should always be final.
In this module, though, the current structure (from constructor through getter) implies that it is a pure "simple" boolean flag. From that point of view it seemed final would make sense (in this specific case).

Your comments about the second paragraph raise a good point as well ... I guess it's more of a consideration as to when extension points should be provided (or limited). It is certainly something to think about. :)

yasserzamani · 2019-02-06T13:38:21Z

core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java

+            return false;
+        }
+
+        if (isClassExcluded(memberClass)) {


This check can be moved prior to Class targetClass = (target != null ? target.getClass() : memberClass); - like 2.6.

yasserzamani · 2019-02-06T13:55:37Z

core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java

+        if (isClassExcluded(memberClass)) {
+            LOG.warn("Declaring class of member type [{}] is excluded!", member);
+            return false;
+        } else {


This else seems can be dropped also - like 2.6.

The else block was introduced (along with moving the memberClass check earlier in the call sequence) to try and eliminate two calls to isClassExcluded(memberClass) that could happen if the member was static in the original 2.5.x flow.
The only reason to consider/risk the order change was to avoid the 2nd call and use the result of the 1st call to guarantee the implicit state in the else block equivalent to !isClassExcluded(memberClass).
Walking though the only side-effect appears to be it could potentially fail-out earlier due to the memberClass check for cases where it might have also have failed-out due to isPackageExcluded() or isClassExcluded(targetClass) before ... but for all paths the same false result would be returned (just a different log warning might be seen).
For that reason it seemed safe-enough to consider/risk to avoid the redundant call.

Yes I know why it has been emerged - I meant we were able to drop this piece of code in 2.6 keeping same functionality by reordering checks :)

JCgH4164838Gh792C124B5 · 2019-02-07T04:27:44Z

Hello @yasserzamani .
Thanks for taking the time to review. :)
Attempting this backport of the (relevant) changes to 2.5.x might not be worth the risk in doing so at this time. :) :)

yasserzamani · 2019-02-07T09:27:21Z

core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java

+        final int memberModifiers = member.getModifiers();
+
+        if (!checkPublicMemberAccess(memberModifiers)) {
+            LOG.trace("Access to non-public [{}] is blocked!", member);


This recently has been changed to warn in 2.6's corresponding PR.

- After comments reduced the scope of the back-port, reverted to the current 2.5.x SecurityMemberAccess as the starting point. From that point: 1) getAllowStaticMethodAccess() made final (to match 2.6) 2) checkStaticMethodAccess() checks that member is not a Field (to match 2.6). Note: It also improves compatibility with OGNL 3.1.x versions (that failed with 2.5.20). 3) Keeps the improved SecurityMemberAccessTest unit test additions previously back-ported from 2.6 in an earlier version of this PR (still mproves effective checks for the PR).

JCgH4164838Gh792C124B5 · 2019-02-08T05:31:14Z

After all of the good points raised from discussions above, this PR has been significantly reduced in scope (to minimize change risk). The latest revision effectively started from the original 2.5.x SecurityMemberAccess and only applies these changes:

getAllowStaticMethodAccess() made final (to match 2.6)
checkStaticMethodAccess() checks that member is not a Field (to match 2.6, ensures the back-ported unit tests all pass).
Keeps the SecurityMemberAccessTest unit test additions previously back-ported from 2.6 in the previous commit (improves unit test effectiveness both for this PR and for 2.5.x moving forward).

I think this makes the PR more palatable (much less risk, and more of a "strengthening" than an improvement per-se).

Thoughts ?

yasserzamani · 2019-02-10T19:43:01Z

I had time to re-think freely :) and now, I would like last @aleksandr-m 's commit on this file at Revert some changes to be more consistent with 2.6 version - to keep it backward compatible, we shouldn't add any sense about Field to the file or unit tests i.e. let it behave as before, right or wrong -- WW-5012 was because of ognl upgrade and another upgrade now has fixed it.

So I think let keep it as is and close this one, thank you!

JCgH4164838Gh792C124B5 · 2019-02-11T15:16:01Z

It has been determined this PR still presents some risk for 2.5.x backward compatibility, so it will not be pursued.
Closing this PR as per above comments and request by Apache Struts Team members.

JCgH4164838Gh792C124B5 added 2 commits January 31, 2019 18:35

Back-port SecurityMemberAccess unit test improvement from 2.6 to 2.5.21.

34c3ce0

- improves effective checks for the PR.

yasserzamani requested changes Feb 6, 2019

View reviewed changes

yasserzamani reviewed Feb 7, 2019

View reviewed changes

JCgH4164838Gh792C124B5 closed this Feb 11, 2019

JCgH4164838Gh792C124B5 deleted the localS2_25x_B3 branch May 16, 2020 19:02

Conversation

JCgH4164838Gh792C124B5 commented Jan 31, 2019

Uh oh!

aleksandr-m commented Feb 1, 2019

Uh oh!

JCgH4164838Gh792C124B5 commented Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yasserzamani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JCgH4164838Gh792C124B5 commented Feb 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JCgH4164838Gh792C124B5 commented Feb 8, 2019

Uh oh!

yasserzamani commented Feb 10, 2019

Uh oh!

JCgH4164838Gh792C124B5 commented Feb 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JCgH4164838Gh792C124B5 commented Feb 1, 2019 •

edited

Loading

coveralls commented Feb 1, 2019 •

edited

Loading