[
https://issues.apache.org/jira/browse/WW-5534?focusedWorklogId=959780&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-959780
]
ASF GitHub Bot logged work on WW-5534:
--------------------------------------
Author: ASF GitHub Bot
Created on: 03/Mar/25 11:09
Start Date: 03/Mar/25 11:09
Worklog Time Spent: 10m
Work Description: kusalk commented on code in PR #1237:
URL: https://github.com/apache/struts/pull/1237#discussion_r1977316681
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -510,9 +519,16 @@ protected StrutsParameter
getParameterAnnotation(AnnotatedElement element) {
return element.getAnnotation(StrutsParameter.class);
}
+ protected Class<?> ultimateClass(Object action) {
+ if (ProxyUtil.isProxy(action)) {
+ return ProxyUtil.ultimateTargetClass(action);
+ }
+ return action.getClass();
+ }
+
protected BeanInfo getBeanInfo(Object action) {
try {
- return Introspector.getBeanInfo(action.getClass());
+ return ognlUtil.getBeanInfo(ultimateClass(action));
Review Comment:
When I implemented this class, I forgot `OgnlUtil` already had a cached
variant of this capability. We are now using that, and resolving any proxies to
ensure annotation detection works as expected.
##########
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:
##########
@@ -212,16 +212,17 @@ protected boolean checkAllowlist(Object target, Member
member) {
return true;
}
+ Class<?> targetClass = target != null ? target.getClass() : null;
+
if (!disallowProxyObjectAccess && ProxyUtil.isProxy(target)) {
- // If `disallowProxyObjectAccess` is not set, allow resolving
Hibernate entities to their underlying
- // classes/members. This allows the allowlist capability to
continue working and offer some level of
+ // If `disallowProxyObjectAccess` is not set, allow resolving
Hibernate entities and Spring proxies to their
+ // underlying classes/members. This allows the allowlist
capability to continue working and still offer
// protection in applications where the developer has accepted the
risk of allowing OGNL access to Hibernate
- // entities. This is preferred to having to disable the allowlist
capability entirely.
- Object newTarget = ProxyUtil.getHibernateProxyTarget(target);
- if (newTarget != target) {
- logAllowlistHibernateEntity(target, newTarget);
- target = newTarget;
- member = ProxyUtil.resolveTargetMember(member, newTarget);
+ // entities and Spring proxies. This is preferred to having to
disable the allowlist capability entirely.
+ Class<?> newTargetClass = ProxyUtil.ultimateTargetClass(target);
Review Comment:
We replaced `#getHibernateProxyTarget` with `#ultimateTargetClass` which can
also resolve Spring proxies. This allows the OGNL allowlist to function in the
presence of Spring proxies in applications where
`struts.disallowProxyObjectAccess` has been reverted to `false`.
##########
core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java:
##########
Review Comment:
I added comments for all the other tests in this class as I realised I
didn't name them very well initially
##########
core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java:
##########
@@ -276,6 +384,27 @@ public void publicModelPojo() {
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Object.class,
Pojo.class));
}
+ /**
+ * Models of ModelDriven actions can be injected without any annotations
on the Action, even when the Action is
+ * proxied.
+ */
+ @Test
+ public void publicModelPojo_proxied() {
+ var proxyFactory = new ProxyFactory(new ModelAction());
Review Comment:
We use the Spring proxy factory to create a CGLIB proxy like would occur
when a transactional proxy is applied to a concrete Action class (like in the
original bug report)
Issue Time Tracking
-------------------
Worklog Id: (was: 959780)
Time Spent: 2h 10m (was: 2h)
> Actions with Spring's @Transactional and ModelDriven
> ----------------------------------------------------
>
> Key: WW-5534
> URL: https://issues.apache.org/jira/browse/WW-5534
> Project: Struts 2
> Issue Type: Bug
> Components: Core Interceptors, Plugin - Spring
> Affects Versions: 7.0.0
> Reporter: Johannes Mayer
> Priority: Minor
> Fix For: 7.1.0
>
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> Hi,
> When using the ModelDriven interface, the getModel method has to be annotated
> with {_}@StrutsParameter{_}.
> When Spring decides to wrap an Action object with SpringCGLIB (e.g. when
> annotating a method with {_}@Transactional){_}, one has to add the Package to
> the allowList, so execute can be called. No harm done, just add this to the
> {_}struts.xml{_}:
> {code:java}
> <constant name="struts.allowlist.packageNames" value="your.action.package"/>
> {code}
> The now emerging problem is, that
> _org.apache.struts2.interceptor.parameter.ParametersInterceptor_ is not able
> to map the request parameter to the model, because it is not able to find a
> suitable _getModel_ method. The reason for this is, that the interceptor is
> trying to find the annotation on the SpringCGLIB class, which does not work.
> As a workaround, I can tell the ParameterInterceptor to not need a
> _@StrutsParameter_ annotation, but imo that defeats the purpose of this
> annotation. I am also warned not to make this configuration. I therefore
> assume that this scenario is not desirable.
> {code:java}
> <constant name="struts.parameters.requireAnnotations" value="false" /> {code}
> Spring's AopUtils gives the option the get to the real class:
> _AopUtils.getTargetClass(springCGLIBObject);_
> I created a project to showcase this:
> [https://github.com/sf-JMA/struts7-model-driven/|https://github.com/sf-JMA/struts7-model-driven/tree/main/src/main]
> I added a test
> [https://github.com/sf-JMA/struts7-model-driven/blob/main/src/test/java/com/steadforce/aek/struts7modeldriven/SpringAopVersusModelDrivenTest.java]
> to show the AopUtils method.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
