From 7cdcd84b838eaea5a3f10af5ea64e6856dc3c27d Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 14 Oct 2024 18:59:11 +1100 Subject: [PATCH 1/4] Merge pull request #1072 from apache/fix/WW-5468-modeldriven-2 WW-5468 Exempt ModelDriven Actions from @StrutsParameter requirement --- .../modelDriven/ModelDrivenAction.java | 2 +- .../java/org/apache/struts2/ModelDriven.java | 6 + .../parameter/ParametersInterceptor.java | 45 +- .../xwork2/ModelDrivenAction.java | 3 +- .../xwork2/ModelDrivenAnnotationAction.java | 3 +- .../ModelDrivenInterceptorTest.java | 2 +- .../xwork2/test/ModelDrivenAction2.java | 4 +- .../test/ModelDrivenAnnotationAction2.java | 4 +- .../test/subtest/NullModelDrivenAction.java | 3 +- .../VisitorValidatorModelAction.java | 5 +- .../StrutsParameterAnnotationTest.java | 36 +- .../struts2/result/StreamResultTest.java | 6 +- .../actions/ModelDrivenAction.java | 2 - .../actions/ValidateGroupAction.java | 2 - .../struts2/junit/StrutsRestTestCase.java | 3 +- .../rest/RestActionInvocationTest.java | 400 +++++++++--------- .../xwork2/ModelDrivenAction.java | 3 +- 17 files changed, 281 insertions(+), 248 deletions(-) diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/modelDriven/ModelDrivenAction.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/modelDriven/ModelDrivenAction.java index 60692b0e69..0fae6c4811 100644 --- a/apps/showcase/src/main/java/org/apache/struts2/showcase/modelDriven/ModelDrivenAction.java +++ b/apps/showcase/src/main/java/org/apache/struts2/showcase/modelDriven/ModelDrivenAction.java @@ -42,7 +42,7 @@ public String execute() throws Exception { } @Override - public Object getModel() { + public Gangster getModel() { return new Gangster(); } } diff --git a/core/src/main/java/org/apache/struts2/ModelDriven.java b/core/src/main/java/org/apache/struts2/ModelDriven.java index 0704109f15..993a6f5eb9 100644 --- a/core/src/main/java/org/apache/struts2/ModelDriven.java +++ b/core/src/main/java/org/apache/struts2/ModelDriven.java @@ -18,6 +18,8 @@ */ package org.apache.struts2; +import org.apache.struts2.interceptor.parameter.StrutsParameter; + /** * ModelDriven Actions provide a model object to be pushed onto the ValueStack * in addition to the Action itself, allowing a FormBean type approach like Struts. @@ -28,9 +30,13 @@ public interface ModelDriven { /** * Gets the model to be pushed onto the ValueStack instead of the Action itself. + *

+ * Please be aware that all setters and getters of every depth on the object returned by this method are available + * for user parameter injection! * * @return the model */ + @StrutsParameter(depth = Integer.MAX_VALUE) T getModel(); } diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index 239bc6d6ca..9f6ff8f539 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -35,6 +35,7 @@ import org.apache.commons.lang3.ClassUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.struts2.ModelDriven; import org.apache.struts2.StrutsConstants; import org.apache.struts2.action.NoParameters; import org.apache.struts2.action.ParameterNameAware; @@ -348,7 +349,15 @@ protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) { } long paramDepth = name.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count(); + + if (action instanceof ModelDriven && !ActionContext.getContext().getValueStack().peek().equals(action)) { + LOG.debug("Model driven Action detected, exempting from @StrutsParameter annotation requirement and OGNL allowlisting model type"); + // (Exempted by annotation on com.opensymphony.xwork2.ModelDriven#getModel) + return hasValidAnnotatedMember("model", action, paramDepth + 1); + } + if (requireAnnotationsTransitionMode && paramDepth == 0) { + LOG.debug("Annotation transition mode enabled, exempting non-nested parameter [{}] from @StrutsParameter annotation requirement", name); return true; } @@ -365,6 +374,8 @@ protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) { * save computation by checking this last. */ protected boolean hasValidAnnotatedMember(String rootProperty, Object action, long paramDepth) { + LOG.debug("Checking Action [{}] for a matching, correctly annotated member for property [{}]", + action.getClass().getSimpleName(), rootProperty); BeanInfo beanInfo = getBeanInfo(action); if (beanInfo == null) { return hasValidAnnotatedField(action, rootProperty, paramDepth); @@ -399,7 +410,7 @@ protected boolean hasValidAnnotatedPropertyDescriptor(Object action, PropertyDes } if (getPermittedInjectionDepth(relevantMethod) < paramDepth) { String logMessage = format( - "Parameter injection for method [%s] on action [%s] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.", + "Parameter injection for method [%s] on Action [%s] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.", relevantMethod.getName(), relevantMethod.getDeclaringClass().getName()); if (devMode) { @@ -409,8 +420,10 @@ protected boolean hasValidAnnotatedPropertyDescriptor(Object action, PropertyDes } return false; } + LOG.debug("Success: Matching annotated method [{}] found for property [{}] of depth [{}] on Action [{}]", + relevantMethod.getName(), propDesc.getName(), paramDepth, action.getClass().getSimpleName()); if (paramDepth >= 1) { - allowlistClass(relevantMethod.getReturnType()); + allowlistClass(propDesc.getPropertyType()); } if (paramDepth >= 2) { allowlistReturnTypeIfParameterized(relevantMethod); @@ -447,19 +460,23 @@ protected void allowlistClass(Class clazz) { } protected boolean hasValidAnnotatedField(Object action, String fieldName, long paramDepth) { + LOG.debug("No matching annotated method found for property [{}] of depth [{}] on Action [{}], now also checking for public field", + fieldName, paramDepth, action.getClass().getSimpleName()); Field field; try { field = action.getClass().getDeclaredField(fieldName); } catch (NoSuchFieldException e) { + LOG.debug("Matching field for property [{}] not found on Action [{}]", fieldName, action.getClass().getSimpleName()); return false; } if (!Modifier.isPublic(field.getModifiers())) { + LOG.debug("Matching field [{}] is not public on Action [{}]", field.getName(), action.getClass().getSimpleName()); return false; } if (getPermittedInjectionDepth(field) < paramDepth) { String logMessage = format( - "Parameter injection for field [%s] on action [%s] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.", - fieldName, + "Parameter injection for field [%s] on Action [%s] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.", + field.getName(), action.getClass().getName()); if (devMode) { notifyDeveloperOfError(LOG, action, logMessage); @@ -468,6 +485,8 @@ protected boolean hasValidAnnotatedField(Object action, String fieldName, long p } return false; } + LOG.debug("Success: Matching annotated public field [{}] found for property of depth [{}] on Action [{}]", + field.getName(), paramDepth, action.getClass().getSimpleName()); if (paramDepth >= 1) { allowlistClass(field.getType()); } @@ -629,7 +648,7 @@ protected boolean isAccepted(String paramName) { if (!result.isAccepted()) { if (devMode) { LOG.warn("Parameter [{}] didn't match accepted pattern [{}]! See Accepted / Excluded patterns at\n" + - "https://struts.apache.org/security/#accepted--excluded-patterns", + "https://struts.apache.org/security/#accepted--excluded-patterns", paramName, result.getAcceptedPattern()); } else { LOG.debug("Parameter [{}] didn't match accepted pattern [{}]!", paramName, result.getAcceptedPattern()); @@ -644,8 +663,8 @@ protected boolean isExcluded(String paramName) { if (result.isExcluded()) { if (devMode) { LOG.warn("Parameter [{}] matches excluded pattern [{}]! See Accepted / Excluded patterns at\n" + - "https://struts.apache.org/security/#accepted--excluded-patterns", - paramName, result.getExcludedPattern()); + "https://struts.apache.org/security/#accepted--excluded-patterns", + paramName, result.getExcludedPattern()); } else { LOG.debug("Parameter [{}] matches excluded pattern [{}]!", paramName, result.getExcludedPattern()); } @@ -663,8 +682,8 @@ protected boolean isParamValueExcluded(String value) { if (excludedValuePattern.matcher(value).matches()) { if (devMode) { LOG.warn("Parameter value [{}] matches excluded pattern [{}]! See Accepting/Excluding parameter values at\n" + - "https://struts.apache.org/core-developers/parameters-interceptor#excluding-parameter-values", - value, excludedValuePatterns); + "https://struts.apache.org/core-developers/parameters-interceptor#excluding-parameter-values", + value, excludedValuePatterns); } else { LOG.debug("Parameter value [{}] matches excluded pattern [{}]", value, excludedValuePattern); } @@ -686,8 +705,8 @@ protected boolean isParamValueAccepted(String value) { } if (devMode) { LOG.warn("Parameter value [{}] didn't match accepted pattern [{}]! See Accepting/Excluding parameter values at\n" + - "https://struts.apache.org/core-developers/parameters-interceptor#excluding-parameter-values", - value, acceptedValuePatterns); + "https://struts.apache.org/core-developers/parameters-interceptor#excluding-parameter-values", + value, acceptedValuePatterns); } else { LOG.debug("Parameter value [{}] was not accepted!", value); } @@ -757,7 +776,7 @@ public void setAcceptedValuePatterns(String commaDelimitedPatterns) { LOG.debug("Sets accepted value patterns to [{}], note this may impact the safety of your application!", patterns); } else { LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this may impact safety of your application!", - acceptedValuePatterns, patterns); + acceptedValuePatterns, patterns); } acceptedValuePatterns = new HashSet<>(patterns.size()); try { @@ -782,7 +801,7 @@ public void setExcludedValuePatterns(String commaDelimitedPatterns) { LOG.debug("Setting excluded value patterns to [{}]", patterns); } else { LOG.warn("Replacing excluded value patterns [{}] with [{}], be aware that this may impact safety of your application!", - excludedValuePatterns, patterns); + excludedValuePatterns, patterns); } excludedValuePatterns = new HashSet<>(patterns.size()); try { diff --git a/core/src/test/java/com/opensymphony/xwork2/ModelDrivenAction.java b/core/src/test/java/com/opensymphony/xwork2/ModelDrivenAction.java index 6ffcad2ff2..e300d036b9 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ModelDrivenAction.java +++ b/core/src/test/java/com/opensymphony/xwork2/ModelDrivenAction.java @@ -44,9 +44,8 @@ public String getFoo() { /** * @return the model to be pushed onto the ValueStack after the Action itself */ - @StrutsParameter(depth = 2) @Override - public Object getModel() { + public TestBean getModel() { return model; } } diff --git a/core/src/test/java/com/opensymphony/xwork2/ModelDrivenAnnotationAction.java b/core/src/test/java/com/opensymphony/xwork2/ModelDrivenAnnotationAction.java index 5549b60b10..f056417329 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ModelDrivenAnnotationAction.java +++ b/core/src/test/java/com/opensymphony/xwork2/ModelDrivenAnnotationAction.java @@ -44,9 +44,8 @@ public String getFoo() { /** * @return the model to be pushed onto the ValueStack after the Action itself */ - @StrutsParameter(depth = 2) @Override - public Object getModel() { + public AnnotatedTestBean getModel() { return model; } } diff --git a/core/src/test/java/com/opensymphony/xwork2/interceptor/ModelDrivenInterceptorTest.java b/core/src/test/java/com/opensymphony/xwork2/interceptor/ModelDrivenInterceptorTest.java index 65b21c86d1..f6513f5f25 100644 --- a/core/src/test/java/com/opensymphony/xwork2/interceptor/ModelDrivenInterceptorTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/interceptor/ModelDrivenInterceptorTest.java @@ -178,7 +178,7 @@ protected void tearDown() throws Exception { } - public class ModelDrivenAction extends ActionSupport implements ModelDriven { + public class ModelDrivenAction extends ActionSupport implements ModelDriven { @Override public Object getModel() { diff --git a/core/src/test/java/com/opensymphony/xwork2/test/ModelDrivenAction2.java b/core/src/test/java/com/opensymphony/xwork2/test/ModelDrivenAction2.java index 0c445860b8..5e29e6899f 100644 --- a/core/src/test/java/com/opensymphony/xwork2/test/ModelDrivenAction2.java +++ b/core/src/test/java/com/opensymphony/xwork2/test/ModelDrivenAction2.java @@ -19,7 +19,6 @@ package com.opensymphony.xwork2.test; import com.opensymphony.xwork2.ModelDrivenAction; -import org.apache.struts2.interceptor.parameter.StrutsParameter; /** @@ -35,9 +34,8 @@ public class ModelDrivenAction2 extends ModelDrivenAction { /** * @return the model to be pushed onto the ValueStack after the Action itself */ - @StrutsParameter(depth = 3) @Override - public Object getModel() { + public TestBean2 getModel() { return model; } } diff --git a/core/src/test/java/com/opensymphony/xwork2/test/ModelDrivenAnnotationAction2.java b/core/src/test/java/com/opensymphony/xwork2/test/ModelDrivenAnnotationAction2.java index 7c26dcfabc..b2fcdf542b 100644 --- a/core/src/test/java/com/opensymphony/xwork2/test/ModelDrivenAnnotationAction2.java +++ b/core/src/test/java/com/opensymphony/xwork2/test/ModelDrivenAnnotationAction2.java @@ -19,7 +19,6 @@ package com.opensymphony.xwork2.test; import com.opensymphony.xwork2.ModelDrivenAnnotationAction; -import org.apache.struts2.interceptor.parameter.StrutsParameter; /** @@ -36,9 +35,8 @@ public class ModelDrivenAnnotationAction2 extends ModelDrivenAnnotationAction { /** * @return the model to be pushed onto the ValueStack after the Action itself */ - @StrutsParameter(depth = 3) @Override - public Object getModel() { + public AnnotationTestBean2 getModel() { return model; } } diff --git a/core/src/test/java/com/opensymphony/xwork2/test/subtest/NullModelDrivenAction.java b/core/src/test/java/com/opensymphony/xwork2/test/subtest/NullModelDrivenAction.java index be0916d9dc..3551a46434 100644 --- a/core/src/test/java/com/opensymphony/xwork2/test/subtest/NullModelDrivenAction.java +++ b/core/src/test/java/com/opensymphony/xwork2/test/subtest/NullModelDrivenAction.java @@ -19,6 +19,7 @@ package com.opensymphony.xwork2.test.subtest; import com.opensymphony.xwork2.ModelDrivenAction; +import com.opensymphony.xwork2.TestBean; /** * Extends ModelDrivenAction to return a null model. @@ -31,7 +32,7 @@ public class NullModelDrivenAction extends ModelDrivenAction { * @return the model to be pushed onto the ValueStack instead of the Action itself */ @Override - public Object getModel() { + public TestBean getModel() { return null; } } diff --git a/core/src/test/java/com/opensymphony/xwork2/validator/VisitorValidatorModelAction.java b/core/src/test/java/com/opensymphony/xwork2/validator/VisitorValidatorModelAction.java index 9f5baeee0f..9ec226f983 100644 --- a/core/src/test/java/com/opensymphony/xwork2/validator/VisitorValidatorModelAction.java +++ b/core/src/test/java/com/opensymphony/xwork2/validator/VisitorValidatorModelAction.java @@ -19,7 +19,7 @@ package com.opensymphony.xwork2.validator; import com.opensymphony.xwork2.ModelDriven; -import org.apache.struts2.interceptor.parameter.StrutsParameter; +import com.opensymphony.xwork2.TestBean; /** @@ -33,9 +33,8 @@ public class VisitorValidatorModelAction extends VisitorValidatorTestAction impl /** * @return the model to be pushed onto the ValueStack instead of the Action itself */ - @StrutsParameter(depth = 2) @Override - public Object getModel() { + public TestBean getModel() { return getBean(); } } diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java index 53fa147170..dfcecddcb2 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java @@ -18,8 +18,12 @@ */ package org.apache.struts2.interceptor.parameter; +import com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.ModelDriven; +import com.opensymphony.xwork2.StubValueStack; import com.opensymphony.xwork2.security.AcceptedPatternsChecker; import com.opensymphony.xwork2.security.NotExcludedAcceptedPatternsChecker; +import com.opensymphony.xwork2.util.ValueStack; import org.apache.commons.lang3.ClassUtils; import org.apache.struts2.dispatcher.HttpParameters; import org.apache.struts2.dispatcher.Parameter; @@ -63,6 +67,7 @@ public void setUp() throws Exception { @After public void tearDown() throws Exception { threadAllowlist.clearAllowlist(); + ActionContext.clear(); } private void testParameter(Object action, String paramName, boolean shouldContain) { @@ -80,7 +85,7 @@ private void testParameter(Object action, String paramName, boolean shouldContai } } - private Set> getParentClasses(Class ...clazzes) { + private Set> getParentClasses(Class... clazzes) { Set> set = new HashSet<>(); for (Class clazz : clazzes) { set.add(clazz); @@ -258,8 +263,21 @@ public void publicStrNotAnnotatedMethod_transitionMode() { testParameter(new MethodAction(), "publicStrNotAnnotated", true); } + @Test + public void publicModelPojo() { + ModelAction action = new ModelAction(); + + // Emulate ModelDrivenInterceptor running previously + ValueStack valueStack = new StubValueStack(); + valueStack.push(action.getModel()); + ActionContext.of().withValueStack(valueStack).bind(); + + testParameter(action, "name", true); + testParameter(action, "name.nested", true); + assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Object.class, Pojo.class)); + } - class FieldAction { + static class FieldAction { @StrutsParameter private String privateStr; @@ -275,7 +293,7 @@ class FieldAction { public Pojo publicPojoDepthZero; @StrutsParameter(depth = 1) - public Pojo publicPojoDepthOne ; + public Pojo publicPojoDepthOne; @StrutsParameter(depth = 2) public Pojo publicPojoDepthTwo; @@ -290,7 +308,7 @@ class FieldAction { public Map publicPojoMapDepthTwo; } - class MethodAction { + static class MethodAction { @StrutsParameter private void setPrivateStr(String str) { @@ -343,6 +361,14 @@ public Map getPublicPojoMapDepthTwo() { } } - class Pojo { + static class ModelAction implements ModelDriven { + + @Override + public Pojo getModel() { + return new Pojo(); + } + } + + static class Pojo { } } diff --git a/core/src/test/java/org/apache/struts2/result/StreamResultTest.java b/core/src/test/java/org/apache/struts2/result/StreamResultTest.java index 5661db5d3d..a02781812f 100644 --- a/core/src/test/java/org/apache/struts2/result/StreamResultTest.java +++ b/core/src/test/java/org/apache/struts2/result/StreamResultTest.java @@ -37,7 +37,6 @@ /** * Unit test for {@link StreamResult}. - * */ public class StreamResultTest extends StrutsInternalTestCase { @@ -126,12 +125,12 @@ public void testAllowCacheDefault() throws Exception { result.doExecute("helloworld", mai); - //check that that headers are not set by default + //check that that headers are not set by default assertNull(response.getHeader("Pragma")); assertNull(response.getHeader("Cache-Control")); } - public void testAllowCacheFalse() throws Exception { + public void testAllowCacheFalse() throws Exception { result.setInputName("streamForImage"); result.setAllowCaching(false); result.doExecute("helloworld", mai); @@ -266,7 +265,6 @@ protected void setUp() throws Exception { } - protected void tearDown() throws Exception { super.tearDown(); response = null; diff --git a/plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/actions/ModelDrivenAction.java b/plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/actions/ModelDrivenAction.java index 9b32fa24b5..3964ed3975 100644 --- a/plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/actions/ModelDrivenAction.java +++ b/plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/actions/ModelDrivenAction.java @@ -21,7 +21,6 @@ import com.opensymphony.xwork2.ActionSupport; import com.opensymphony.xwork2.ModelDriven; import org.apache.struts.beanvalidation.models.Person; -import org.apache.struts2.interceptor.parameter.StrutsParameter; import javax.validation.Valid; @@ -30,7 +29,6 @@ public class ModelDrivenAction extends ActionSupport implements ModelDriven errors = new HashMap(); - List list = new ArrayList(); - list.add(actionMessage); - errors.put("actionErrors", list); - restActionInvocation.selectTarget(); - assertEquals(errors, restActionInvocation.target); - - // Model with get and no content in post, put, delete - setUp(); - RestAction restAction = (RestAction)restActionInvocation.getAction(); - List model = new ArrayList(); - model.add("Item"); - restAction.model = model; - request.setMethod("GET"); - restActionInvocation.selectTarget(); - assertEquals(model, restActionInvocation.target); - request.setMethod("POST"); - restActionInvocation.selectTarget(); - assertEquals(null, restActionInvocation.target); - request.setMethod("PUT"); - restActionInvocation.selectTarget(); - assertEquals(null, restActionInvocation.target); - request.setMethod("DELETE"); - restActionInvocation.selectTarget(); - assertEquals(null, restActionInvocation.target); + RestActionInvocation restActionInvocation; + MockHttpServletRequest request; + MockHttpServletResponse response; + + @Override + protected void setUp() throws Exception { + super.setUp(); + + restActionInvocation = new RestActionInvocationTester(); + request = new MockHttpServletRequest(); + response = new MockHttpServletResponse(); + ServletActionContext.setRequest(request); + ServletActionContext.setResponse(response); + + } + + /** + * Test the correct action results: null, String, HttpHeaders, Result + * @throws Exception + */ + public void testSaveResult() throws Exception { + + Object methodResult = "index"; + ActionConfig actionConfig = restActionInvocation.getProxy().getConfig(); + assertEquals("index", restActionInvocation.saveResult(actionConfig, methodResult)); + + setUp(); + methodResult = new DefaultHttpHeaders("show"); + assertEquals("show", restActionInvocation.saveResult(actionConfig, methodResult)); + assertEquals(methodResult, restActionInvocation.httpHeaders); + + setUp(); + methodResult = new HttpHeaderResult(HttpServletResponse.SC_ACCEPTED); + assertEquals(null, restActionInvocation.saveResult(actionConfig, methodResult)); + assertEquals(methodResult, restActionInvocation.createResult()); + + setUp(); + try { + methodResult = new Object(); + restActionInvocation.saveResult(actionConfig, methodResult); + + // ko + assertFalse(true); + + } catch (ConfigurationException c) { + // ok, object not allowed + } + } + + /** + * Test the target selection: exception, error messages, model and null + * @throws Exception + */ + public void testSelectTarget() throws Exception { + + // Exception + Exception e = new Exception(); + restActionInvocation.getStack().set("exception", e); + restActionInvocation.selectTarget(); + assertEquals(e, restActionInvocation.target); + + // Error messages + setUp(); + String actionMessage = "Error!"; + RestActionSupport action = (RestActionSupport)restActionInvocation.getAction(); + action.addActionError(actionMessage); + Map errors = new HashMap(); + List list = new ArrayList(); + list.add(actionMessage); + errors.put("actionErrors", list); + restActionInvocation.selectTarget(); + assertEquals(errors, restActionInvocation.target); + + // Model with get and no content in post, put, delete + setUp(); + RestAction restAction = (RestAction)restActionInvocation.getAction(); + List model = new ArrayList(); + model.add("Item"); + restAction.model = model; + request.setMethod("GET"); + restActionInvocation.selectTarget(); + assertEquals(model, restActionInvocation.target); + request.setMethod("POST"); + restActionInvocation.selectTarget(); + assertEquals(null, restActionInvocation.target); + request.setMethod("PUT"); + restActionInvocation.selectTarget(); + assertEquals(null, restActionInvocation.target); + request.setMethod("DELETE"); + restActionInvocation.selectTarget(); + assertEquals(null, restActionInvocation.target); // disable content restriction to GET only model = new ArrayList(); @@ -151,102 +150,100 @@ public void testSelectTarget() throws Exception { assertEquals(model.get(0), "Item1"); } - /** - * Test the not modified status code. - * @throws Exception - */ - public void testResultNotModified() throws Exception { + /** + * Test the not modified status code. + * @throws Exception + */ + public void testResultNotModified() throws Exception { + + request.addHeader("If-None-Match", "123"); + request.setMethod("GET"); + + RestAction restAction = (RestAction)restActionInvocation.getAction(); + List model = new ArrayList() { + @Override + public int hashCode() { + return 123; + } + }; + model.add("Item"); + restAction.model = model; + + restActionInvocation.processResult(); + assertEquals(SC_NOT_MODIFIED, response.getStatus()); + + } + + /** + * Test the default error result. + * @throws Exception + */ + public void testDefaultErrorResult() throws Exception { + + // Exception + Exception e = new Exception(); + restActionInvocation.getStack().set("exception", e); + request.setMethod("GET"); + + RestAction restAction = (RestAction)restActionInvocation.getAction(); + List model = new ArrayList(); + model.add("Item"); + restAction.model = model; + + restActionInvocation.setDefaultErrorResultName("default-error"); + ResultConfig resultConfig = new ResultConfig.Builder("default-error", + "org.apache.struts2.result.HttpHeaderResult") + .addParam("status", "123").build(); + ActionConfig actionConfig = new ActionConfig.Builder("org.apache.rest", + "RestAction", "org.apache.rest.RestAction") + .addResultConfig(resultConfig) + .build(); + ((MockActionProxy)restActionInvocation.getProxy()).setConfig(actionConfig); + + restActionInvocation.processResult(); + assertEquals(123, response.getStatus()); + + } + + public void testNoResult() throws Exception { - request.addHeader("If-None-Match", "123"); - request.setMethod("GET"); + RestAction restAction = (RestAction)restActionInvocation.getAction(); + List model = new ArrayList(); + model.add("Item"); + restAction.model = model; + request.setMethod("GET"); + restActionInvocation.setResultCode("index"); - RestAction restAction = (RestAction)restActionInvocation.getAction(); - List model = new ArrayList() { - @Override - public int hashCode() { - return 123; - } - }; - model.add("Item"); - restAction.model = model; + try { + restActionInvocation.processResult(); - restActionInvocation.processResult(); - assertEquals(SC_NOT_MODIFIED, response.getStatus()); + // ko + assertFalse(true); + + } catch (ConfigurationException c) { + // ok, no result + } } - /** - * Test the default error result. - * @throws Exception - */ - public void testDefaultErrorResult() throws Exception { - - // Exception - Exception e = new Exception(); - restActionInvocation.getStack().set("exception", e); - request.setMethod("GET"); - - RestAction restAction = (RestAction)restActionInvocation.getAction(); - List model = new ArrayList(); - model.add("Item"); - restAction.model = model; - - restActionInvocation.setDefaultErrorResultName("default-error"); - ResultConfig resultConfig = new ResultConfig.Builder("default-error", - "org.apache.struts2.result.HttpHeaderResult") - .addParam("status", "123").build(); - ActionConfig actionConfig = new ActionConfig.Builder("org.apache.rest", - "RestAction", "org.apache.rest.RestAction") - .addResultConfig(resultConfig) - .build(); - ((MockActionProxy)restActionInvocation.getProxy()).setConfig(actionConfig); - - restActionInvocation.processResult(); - assertEquals(123, response.getStatus()); - - } - - public void testNoResult() throws Exception { - - RestAction restAction = (RestAction)restActionInvocation.getAction(); - List model = new ArrayList(); - model.add("Item"); - restAction.model = model; - request.setMethod("GET"); - restActionInvocation.setResultCode("index"); - - try { - restActionInvocation.processResult(); - - // ko - assertFalse(true); - - } catch (ConfigurationException c) { - // ok, no result - } - - } - - /** - * Test the global execution - * @throws Exception - */ - public void testInvoke() throws Exception { - - // Default index method return 'success' - ((MockActionProxy)restActionInvocation.getProxy()).setMethod("index"); - - // Define result 'success' - ResultConfig resultConfig = new ResultConfig.Builder("success", - "org.apache.struts2.result.HttpHeaderResult") - .addParam("status", "123").build(); - ActionConfig actionConfig = new ActionConfig.Builder("org.apache.rest", - "RestAction", "org.apache.rest.RestAction") - .addResultConfig(resultConfig) - .build(); - ((MockActionProxy)restActionInvocation.getProxy()).setConfig(actionConfig); - - request.setMethod("GET"); + /** + * Test the global execution + * @throws Exception + */ + public void testInvoke() throws Exception { + + // Default index method return 'success' + ((MockActionProxy)restActionInvocation.getProxy()).setMethod("index"); + + // Define result 'success' + ResultConfig resultConfig = new ResultConfig.Builder("success", "org.apache.struts2.result.HttpHeaderResult") + .addParam("status", "123").build(); + ActionConfig actionConfig = new ActionConfig.Builder("org.apache.rest", "RestAction", "org.apache.rest.RestAction") + .addResultConfig(resultConfig) + .build(); + ((MockActionProxy)restActionInvocation.getProxy()).setConfig(actionConfig); + + request.setMethod("GET"); restActionInvocation.setOgnlUtil(new OgnlUtil()); restActionInvocation.invoke(); @@ -256,31 +253,31 @@ public void testInvoke() throws Exception { class RestActionInvocationTester extends RestActionInvocation { - RestActionInvocationTester() { - super(new HashMap(), true); - List interceptorMappings = new ArrayList(); + RestActionInvocationTester() { + super(new HashMap<>(), true); + List interceptorMappings = new ArrayList<>(); MockInterceptor mockInterceptor = new MockInterceptor(); mockInterceptor.setFoo("interceptor"); mockInterceptor.setExpectedFoo("interceptor"); interceptorMappings.add(new InterceptorMapping("interceptor", mockInterceptor)); interceptors = interceptorMappings.iterator(); MockActionProxy actionProxy = new MockActionProxy(); - ActionConfig actionConfig = new ActionConfig.Builder("org.apache.rest", - "RestAction", "org.apache.rest.RestAction").build(); + ActionConfig actionConfig = new ActionConfig.Builder( + "org.apache.rest", "RestAction", "org.apache.rest.RestAction").build(); actionProxy.setConfig(actionConfig); proxy = actionProxy; action = new RestAction(); setMimeTypeHandlerSelector(new DefaultContentTypeHandlerManager()); unknownHandlerManager = new DefaultUnknownHandlerManager(); - try { - XWorkTestCaseHelper.setUp(); - } catch (Exception e) { - throw new RuntimeException(e); - } - invocationContext = ActionContext.getContext(); - container = ActionContext.getContext().getContainer(); - stack = ActionContext.getContext().getValueStack(); - objectFactory = container.getInstance(ObjectFactory.class); + try { + XWorkTestCaseHelper.setUp(); + } catch (Exception e) { + throw new RuntimeException(e); + } + invocationContext = ActionContext.getContext(); + container = ActionContext.getContext().getContainer(); + stack = ActionContext.getContext().getValueStack(); + objectFactory = container.getInstance(ObjectFactory.class); } @@ -288,13 +285,12 @@ class RestActionInvocationTester extends RestActionInvocation { static class RestAction extends RestActionSupport implements ModelDriven> { - List model; + List model; - @StrutsParameter(depth = 1) - @Override - public List getModel() { - return model; - } + @Override + public List getModel() { + return model; + } } } diff --git a/plugins/spring/src/test/java/com/opensymphony/xwork2/ModelDrivenAction.java b/plugins/spring/src/test/java/com/opensymphony/xwork2/ModelDrivenAction.java index 6ffcad2ff2..e300d036b9 100644 --- a/plugins/spring/src/test/java/com/opensymphony/xwork2/ModelDrivenAction.java +++ b/plugins/spring/src/test/java/com/opensymphony/xwork2/ModelDrivenAction.java @@ -44,9 +44,8 @@ public String getFoo() { /** * @return the model to be pushed onto the ValueStack after the Action itself */ - @StrutsParameter(depth = 2) @Override - public Object getModel() { + public TestBean getModel() { return model; } } From fe46ad9f4a7d30d68727b9d7fae34d62a7f31dc6 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 1 Nov 2024 19:48:46 +1100 Subject: [PATCH 2/4] WW-5478 Deprecate DefaultResultFactory --- .../opensymphony/xwork2/config/entities/ResultConfig.java | 6 +++--- .../xwork2/config/impl/DefaultConfiguration.java | 4 ++-- .../opensymphony/xwork2/factory/DefaultResultFactory.java | 4 ++++ .../interceptor/ChainingInterceptorWithConfigTest.java | 7 ++++--- .../convention/PackageBasedActionConfigBuilderTest.java | 4 ++-- 5 files changed, 15 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/config/entities/ResultConfig.java b/core/src/main/java/com/opensymphony/xwork2/config/entities/ResultConfig.java index 50602636b9..9cb78c0fde 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/entities/ResultConfig.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/entities/ResultConfig.java @@ -38,7 +38,7 @@ */ public class ResultConfig extends Located implements Serializable { - protected Map params; + protected Map params; protected String className; protected String name; @@ -63,7 +63,7 @@ public String getName() { return name; } - public Map getParams() { + public Map getParams() { return params; } @@ -140,7 +140,7 @@ public Builder addParam(String name, String value) { return this; } - public Builder addParams(Map params) { + public Builder addParams(Map params) { target.params.putAll(params); return this; } diff --git a/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java b/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java index 7c725e15b0..d1560f53f1 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java @@ -66,7 +66,6 @@ import com.opensymphony.xwork2.factory.ConverterFactory; import com.opensymphony.xwork2.factory.DefaultActionFactory; import com.opensymphony.xwork2.factory.DefaultInterceptorFactory; -import com.opensymphony.xwork2.factory.DefaultResultFactory; import com.opensymphony.xwork2.factory.DefaultUnknownHandlerFactory; import com.opensymphony.xwork2.factory.DefaultValidatorFactory; import com.opensymphony.xwork2.factory.InterceptorFactory; @@ -109,6 +108,7 @@ import org.apache.struts2.conversion.StrutsConversionPropertiesProcessor; import org.apache.struts2.conversion.StrutsTypeConverterCreator; import org.apache.struts2.conversion.StrutsTypeConverterHolder; +import org.apache.struts2.factory.StrutsResultFactory; import org.apache.struts2.ognl.OgnlGuard; import org.apache.struts2.ognl.ProviderAllowlist; import org.apache.struts2.ognl.StrutsOgnlGuard; @@ -363,7 +363,7 @@ public static ContainerBuilder bootstrapFactories(ContainerBuilder builder) { // TODO: SpringObjectFactoryTest fails when these are SINGLETON .factory(ObjectFactory.class, Scope.PROTOTYPE) .factory(ActionFactory.class, DefaultActionFactory.class, Scope.PROTOTYPE) - .factory(ResultFactory.class, DefaultResultFactory.class, Scope.PROTOTYPE) + .factory(ResultFactory.class, StrutsResultFactory.class, Scope.PROTOTYPE) .factory(InterceptorFactory.class, DefaultInterceptorFactory.class, Scope.PROTOTYPE) .factory(ValidatorFactory.class, DefaultValidatorFactory.class, Scope.PROTOTYPE) .factory(ConverterFactory.class, StrutsConverterFactory.class, Scope.PROTOTYPE) diff --git a/core/src/main/java/com/opensymphony/xwork2/factory/DefaultResultFactory.java b/core/src/main/java/com/opensymphony/xwork2/factory/DefaultResultFactory.java index 42527494e1..b4e312bfd8 100644 --- a/core/src/main/java/com/opensymphony/xwork2/factory/DefaultResultFactory.java +++ b/core/src/main/java/com/opensymphony/xwork2/factory/DefaultResultFactory.java @@ -26,12 +26,16 @@ import com.opensymphony.xwork2.util.reflection.ReflectionException; import com.opensymphony.xwork2.util.reflection.ReflectionExceptionHandler; import com.opensymphony.xwork2.util.reflection.ReflectionProvider; +import org.apache.struts2.factory.StrutsResultFactory; import java.util.Map; /** * Default implementation + * + * @deprecated since 6.7.0, use {@link StrutsResultFactory} instead. */ +@Deprecated public class DefaultResultFactory implements ResultFactory { private ObjectFactory objectFactory; diff --git a/core/src/test/java/com/opensymphony/xwork2/interceptor/ChainingInterceptorWithConfigTest.java b/core/src/test/java/com/opensymphony/xwork2/interceptor/ChainingInterceptorWithConfigTest.java index ab1bfbf5f7..41e5a2c07d 100644 --- a/core/src/test/java/com/opensymphony/xwork2/interceptor/ChainingInterceptorWithConfigTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/interceptor/ChainingInterceptorWithConfigTest.java @@ -42,6 +42,7 @@ import java.util.Collections; import java.util.HashMap; +import java.util.Map; /** @@ -101,11 +102,11 @@ public void loadPackages() throws ConfigurationException { HashMap interceptorParams = new HashMap<>(); interceptorParams.put("excludes", "blah,bar"); - HashMap successParams1 = new HashMap(); + Map successParams1 = new HashMap<>(); successParams1.put("propertyName", "baz"); - successParams1.put("expectedValue", 1); + successParams1.put("expectedValue", "1"); - HashMap successParams2 = new HashMap(); + Map successParams2 = new HashMap<>(); successParams2.put("propertyName", "blah"); successParams2.put("expectedValue", null); diff --git a/plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderTest.java b/plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderTest.java index 1643ce7ae5..b6a9de4bee 100644 --- a/plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderTest.java +++ b/plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderTest.java @@ -35,7 +35,6 @@ import com.opensymphony.xwork2.config.entities.ResultTypeConfig; import com.opensymphony.xwork2.config.impl.DefaultConfiguration; import com.opensymphony.xwork2.factory.DefaultInterceptorFactory; -import com.opensymphony.xwork2.factory.DefaultResultFactory; import com.opensymphony.xwork2.inject.Container; import com.opensymphony.xwork2.inject.Scope.Strategy; import com.opensymphony.xwork2.ognl.OgnlReflectionProvider; @@ -97,6 +96,7 @@ import org.apache.struts2.convention.annotation.Action; import org.apache.struts2.convention.annotation.Actions; import org.apache.struts2.convention.dontfind.DontFindMeAction; +import org.apache.struts2.factory.StrutsResultFactory; import org.apache.struts2.ognl.ProviderAllowlist; import org.apache.struts2.result.ServletDispatcherResult; import org.easymock.EasyMock; @@ -918,7 +918,7 @@ public void setProperty(String name, Object value, Object o, Map dif.setObjectFactory((ObjectFactory) obj); dif.setReflectionProvider(rp); - DefaultResultFactory drf = new DefaultResultFactory(); + StrutsResultFactory drf = new StrutsResultFactory(); drf.setObjectFactory((ObjectFactory) obj); drf.setReflectionProvider(rp); From c1763909269dfabbabf4b451300f3cfca55e4aaf Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 1 Nov 2024 19:59:52 +1100 Subject: [PATCH 3/4] WW-5478 Fix ActionContext equals/hashCode contract --- core/src/main/java/org/apache/struts2/ActionContext.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/struts2/ActionContext.java b/core/src/main/java/org/apache/struts2/ActionContext.java index 79f7552d52..335c24e95f 100644 --- a/core/src/main/java/org/apache/struts2/ActionContext.java +++ b/core/src/main/java/org/apache/struts2/ActionContext.java @@ -542,11 +542,16 @@ public ActionContext with(String key, Object value) { } @Override - public boolean equals(Object obj) { + public final boolean equals(Object obj) { if (!(obj instanceof ActionContext)) { return false; } ActionContext other = (ActionContext) obj; return Objects.equals(getContextMap(), other.getContextMap()); } + + @Override + public final int hashCode() { + return Objects.hash(getContextMap()); + } } From ecad4a576b0e4b854f6e8fe71d1e56bb3cc95427 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sat, 2 Nov 2024 14:18:42 +1100 Subject: [PATCH 4/4] Replace com.opensymphony.xwork2 mentions --- .../apache/struts2/interceptor/ActionFileUploadInterceptor.java | 2 +- .../struts2/interceptor/parameter/ParametersInterceptor.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java index d5082cc553..911a6e4a96 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java @@ -99,7 +99,7 @@ * package com.example; * * import java.io.File; - * import com.opensymphony.xwork2.ActionSupport; + * import org.apache.struts2.ActionSupport; * import org.apache.struts2.action.UploadedFilesAware; * * public UploadAction extends ActionSupport implements UploadedFilesAware { diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index 63d94b3888..92449415e3 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -352,7 +352,7 @@ protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) { if (action instanceof ModelDriven && !ActionContext.getContext().getValueStack().peek().equals(action)) { LOG.debug("Model driven Action detected, exempting from @StrutsParameter annotation requirement and OGNL allowlisting model type"); - // (Exempted by annotation on com.opensymphony.xwork2.ModelDriven#getModel) + // (Exempted by annotation on org.apache.struts2.ModelDriven#getModel) return hasValidAnnotatedMember("model", action, paramDepth + 1); }