[
https://issues.apache.org/jira/browse/WW-5352?focusedWorklogId=898144&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-898144
]
ASF GitHub Bot logged work on WW-5352:
--------------------------------------
Author: ASF GitHub Bot
Created on: 05/Jan/24 06:37
Start Date: 05/Jan/24 06:37
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on code in PR #831:
URL: https://github.com/apache/struts/pull/831#discussion_r1442537671
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -132,138 +133,149 @@ static private int countOGNLCharacters(String s) {
@Override
public String doIntercept(ActionInvocation invocation) throws Exception {
Object action = invocation.getAction();
- if (!(action instanceof NoParameters)) {
- ActionContext ac = invocation.getInvocationContext();
- HttpParameters parameters = retrieveParameters(ac);
+ if (action instanceof NoParameters) {
+ return invocation.invoke();
+ }
- if (LOG.isDebugEnabled()) {
- LOG.debug("Setting params {}",
normalizeSpace(getParameterLogMap(parameters)));
- }
+ ActionContext actionContext = invocation.getInvocationContext();
+ HttpParameters parameters = retrieveParameters(actionContext);
- if (parameters != null) {
- Map<String, Object> contextMap = ac.getContextMap();
- try {
- ReflectionContextState.setCreatingNullObjects(contextMap,
true);
- ReflectionContextState.setDenyMethodExecution(contextMap,
true);
-
ReflectionContextState.setReportingConversionErrors(contextMap, true);
-
- ValueStack stack = ac.getValueStack();
- setParameters(action, stack, parameters);
- } finally {
- ReflectionContextState.setCreatingNullObjects(contextMap,
false);
- ReflectionContextState.setDenyMethodExecution(contextMap,
false);
-
ReflectionContextState.setReportingConversionErrors(contextMap, false);
- }
- }
+ if (parameters == null) {
+ return invocation.invoke();
+ }
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Setting params {}",
normalizeSpace(getParameterLogMap(parameters)));
+ }
+
+ Map<String, Object> contextMap = actionContext.getContextMap();
+ batchSetReflectionContextState(contextMap, true);
+ try {
+ setParameters(action, actionContext.getValueStack(), parameters);
+ } finally {
+ batchSetReflectionContextState(contextMap, false);
}
+
return invocation.invoke();
}
/**
* Gets the parameter map to apply from wherever appropriate
*
- * @param ac The action context
+ * @param actionContext The action context
* @return The parameter map to apply
*/
- protected HttpParameters retrieveParameters(ActionContext ac) {
- return ac.getParameters();
+ protected HttpParameters retrieveParameters(ActionContext actionContext) {
+ return actionContext.getParameters();
}
/**
* Adds the parameters into context's ParameterMap
+ * <p>
+ * In this class this is a no-op, since the parameters were fetched from
the same location. In subclasses both this
+ * and {@link #retrieveParameters} should be overridden.
*
* @param ac The action context
* @param newParams The parameter map to apply
- * <p>
- * In this class this is a no-op, since the parameters
were fetched from the same location.
- * In subclasses both retrieveParameters() and
addParametersToContext() should be overridden.
- * </p>
*/
protected void addParametersToContext(ActionContext ac, Map<String, ?>
newParams) {
}
protected void setParameters(final Object action, ValueStack stack,
HttpParameters parameters) {
- HttpParameters params;
- Map<String, Parameter> acceptableParameters;
- if (ordered) {
- params =
HttpParameters.create().withComparator(getOrderedComparator()).withParent(parameters).build();
- acceptableParameters = new TreeMap<>(getOrderedComparator());
- } else {
- params = HttpParameters.create().withParent(parameters).build();
- acceptableParameters = new TreeMap<>();
- }
+ Map<String, Parameter> acceptableParameters =
toAcceptableParameters(parameters, action);
- for (Map.Entry<String, Parameter> entry : params.entrySet()) {
- String parameterName = entry.getKey();
- boolean isAcceptableParameter =
isAcceptableParameter(parameterName, action);
- isAcceptableParameter &=
isAcceptableParameterValue(entry.getValue(), action);
+ ValueStack newStack = toNewStack(stack);
+ batchSetReflectionContextState(newStack.getContext(), true);
+ setMemberAccessProperties(newStack);
- if (isAcceptableParameter) {
- acceptableParameters.put(parameterName, entry.getValue());
- }
+ setParametersOnStack(newStack, acceptableParameters, action);
+
+ if (newStack instanceof ClearableValueStack) {
+
stack.getActionContext().withConversionErrors(newStack.getActionContext().getConversionErrors());
}
+ addParametersToContext(ActionContext.getContext(),
acceptableParameters);
+ }
+
+ protected void batchSetReflectionContextState(Map<String, Object> context,
boolean value) {
+ ReflectionContextState.setCreatingNullObjects(context, value);
+ ReflectionContextState.setDenyMethodExecution(context, value);
+ ReflectionContextState.setReportingConversionErrors(context, value);
+ }
+
+ protected ValueStack toNewStack(ValueStack stack) {
ValueStack newStack = valueStackFactory.createValueStack(stack);
- boolean clearableStack = newStack instanceof ClearableValueStack;
- if (clearableStack) {
- //if the stack's context can be cleared, do that to prevent OGNL
- //from having access to objects in the stack, see XW-641
+ if (newStack instanceof ClearableValueStack) {
((ClearableValueStack) newStack).clearContextValues();
- Map<String, Object> context = newStack.getContext();
- ReflectionContextState.setCreatingNullObjects(context, true);
- ReflectionContextState.setDenyMethodExecution(context, true);
- ReflectionContextState.setReportingConversionErrors(context, true);
-
- //keep locale from original context
newStack.getActionContext().withLocale(stack.getActionContext().getLocale()).withValueStack(stack);
}
+ return newStack;
+ }
+
+ protected void setMemberAccessProperties(ValueStack stack) {
Review Comment:
I would avoid using _setter/getter_ pattern if this isn't a setter _per se_
- this also reduces a risk of using such method in expression evaluation. What
about using `applyMemberAccessProperties()` instead?
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -132,138 +133,149 @@ static private int countOGNLCharacters(String s) {
@Override
public String doIntercept(ActionInvocation invocation) throws Exception {
Object action = invocation.getAction();
- if (!(action instanceof NoParameters)) {
- ActionContext ac = invocation.getInvocationContext();
- HttpParameters parameters = retrieveParameters(ac);
+ if (action instanceof NoParameters) {
+ return invocation.invoke();
+ }
- if (LOG.isDebugEnabled()) {
- LOG.debug("Setting params {}",
normalizeSpace(getParameterLogMap(parameters)));
- }
+ ActionContext actionContext = invocation.getInvocationContext();
+ HttpParameters parameters = retrieveParameters(actionContext);
- if (parameters != null) {
- Map<String, Object> contextMap = ac.getContextMap();
- try {
- ReflectionContextState.setCreatingNullObjects(contextMap,
true);
- ReflectionContextState.setDenyMethodExecution(contextMap,
true);
-
ReflectionContextState.setReportingConversionErrors(contextMap, true);
-
- ValueStack stack = ac.getValueStack();
- setParameters(action, stack, parameters);
- } finally {
- ReflectionContextState.setCreatingNullObjects(contextMap,
false);
- ReflectionContextState.setDenyMethodExecution(contextMap,
false);
-
ReflectionContextState.setReportingConversionErrors(contextMap, false);
- }
- }
+ if (parameters == null) {
+ return invocation.invoke();
+ }
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Setting params {}",
normalizeSpace(getParameterLogMap(parameters)));
+ }
+
+ Map<String, Object> contextMap = actionContext.getContextMap();
+ batchSetReflectionContextState(contextMap, true);
+ try {
+ setParameters(action, actionContext.getValueStack(), parameters);
+ } finally {
+ batchSetReflectionContextState(contextMap, false);
}
+
return invocation.invoke();
}
/**
* Gets the parameter map to apply from wherever appropriate
*
- * @param ac The action context
+ * @param actionContext The action context
* @return The parameter map to apply
*/
- protected HttpParameters retrieveParameters(ActionContext ac) {
- return ac.getParameters();
+ protected HttpParameters retrieveParameters(ActionContext actionContext) {
+ return actionContext.getParameters();
}
/**
* Adds the parameters into context's ParameterMap
+ * <p>
+ * In this class this is a no-op, since the parameters were fetched from
the same location. In subclasses both this
+ * and {@link #retrieveParameters} should be overridden.
*
* @param ac The action context
* @param newParams The parameter map to apply
- * <p>
- * In this class this is a no-op, since the parameters
were fetched from the same location.
- * In subclasses both retrieveParameters() and
addParametersToContext() should be overridden.
- * </p>
*/
protected void addParametersToContext(ActionContext ac, Map<String, ?>
newParams) {
}
protected void setParameters(final Object action, ValueStack stack,
HttpParameters parameters) {
- HttpParameters params;
- Map<String, Parameter> acceptableParameters;
- if (ordered) {
- params =
HttpParameters.create().withComparator(getOrderedComparator()).withParent(parameters).build();
- acceptableParameters = new TreeMap<>(getOrderedComparator());
- } else {
- params = HttpParameters.create().withParent(parameters).build();
- acceptableParameters = new TreeMap<>();
- }
+ Map<String, Parameter> acceptableParameters =
toAcceptableParameters(parameters, action);
- for (Map.Entry<String, Parameter> entry : params.entrySet()) {
- String parameterName = entry.getKey();
- boolean isAcceptableParameter =
isAcceptableParameter(parameterName, action);
- isAcceptableParameter &=
isAcceptableParameterValue(entry.getValue(), action);
+ ValueStack newStack = toNewStack(stack);
+ batchSetReflectionContextState(newStack.getContext(), true);
+ setMemberAccessProperties(newStack);
- if (isAcceptableParameter) {
- acceptableParameters.put(parameterName, entry.getValue());
- }
+ setParametersOnStack(newStack, acceptableParameters, action);
+
+ if (newStack instanceof ClearableValueStack) {
+
stack.getActionContext().withConversionErrors(newStack.getActionContext().getConversionErrors());
}
+ addParametersToContext(ActionContext.getContext(),
acceptableParameters);
+ }
+
+ protected void batchSetReflectionContextState(Map<String, Object> context,
boolean value) {
+ ReflectionContextState.setCreatingNullObjects(context, value);
+ ReflectionContextState.setDenyMethodExecution(context, value);
+ ReflectionContextState.setReportingConversionErrors(context, value);
+ }
+
+ protected ValueStack toNewStack(ValueStack stack) {
ValueStack newStack = valueStackFactory.createValueStack(stack);
- boolean clearableStack = newStack instanceof ClearableValueStack;
- if (clearableStack) {
- //if the stack's context can be cleared, do that to prevent OGNL
- //from having access to objects in the stack, see XW-641
+ if (newStack instanceof ClearableValueStack) {
((ClearableValueStack) newStack).clearContextValues();
- Map<String, Object> context = newStack.getContext();
- ReflectionContextState.setCreatingNullObjects(context, true);
- ReflectionContextState.setDenyMethodExecution(context, true);
- ReflectionContextState.setReportingConversionErrors(context, true);
-
- //keep locale from original context
newStack.getActionContext().withLocale(stack.getActionContext().getLocale()).withValueStack(stack);
}
+ return newStack;
+ }
+
+ protected void setMemberAccessProperties(ValueStack stack) {
+ if (!(stack instanceof MemberAccessValueStack)) {
+ return;
+ }
+ ((MemberAccessValueStack)
stack).useAcceptProperties(acceptedPatterns.getAcceptedPatterns());
+ ((MemberAccessValueStack)
stack).useExcludeProperties(excludedPatterns.getExcludedPatterns());
+ }
+
+ protected Map<String, Parameter> toAcceptableParameters(HttpParameters
parameters, Object action) {
+ HttpParameters newParams = initNewHttpParameters(parameters);
+ Map<String, Parameter> acceptableParameters = initParameterMap();
+
+ for (Map.Entry<String, Parameter> entry : newParams.entrySet()) {
+ String parameterName = entry.getKey();
+ Parameter parameterValue = entry.getValue();
+ if (isAcceptableParameter(parameterName, action) &&
isAcceptableParameterValue(parameterValue, action)) {
+ acceptableParameters.put(parameterName, parameterValue);
+ }
+ }
+ return acceptableParameters;
+ }
- boolean memberAccessStack = newStack instanceof MemberAccessValueStack;
- if (memberAccessStack) {
- //block or allow access to properties
- //see WW-2761 for more details
- MemberAccessValueStack accessValueStack = (MemberAccessValueStack)
newStack;
-
accessValueStack.useAcceptProperties(acceptedPatterns.getAcceptedPatterns());
-
accessValueStack.useExcludeProperties(excludedPatterns.getExcludedPatterns());
+ protected Map<String, Parameter> initParameterMap() {
+ if (ordered) {
+ return new TreeMap<>(getOrderedComparator());
+ } else {
+ return new TreeMap<>();
}
+ }
+
+ protected HttpParameters initNewHttpParameters(HttpParameters parameters) {
+ if (ordered) {
+ return
HttpParameters.create().withComparator(getOrderedComparator()).withParent(parameters).build();
+ } else {
+ return HttpParameters.create().withParent(parameters).build();
+ }
+ }
- for (Map.Entry<String, Parameter> entry :
acceptableParameters.entrySet()) {
- String name = entry.getKey();
- Parameter value = entry.getValue();
+ protected void setParametersOnStack(ValueStack stack, Map<String,
Parameter> parameters, Object action) {
Review Comment:
`applyParametersOnStack`?
Issue Time Tracking
-------------------
Worklog Id: (was: 898144)
Time Spent: 3h 40m (was: 3.5h)
> Implement annotation mechanism for injectable fields via parameters
> -------------------------------------------------------------------
>
> Key: WW-5352
> URL: https://issues.apache.org/jira/browse/WW-5352
> Project: Struts 2
> Issue Type: Improvement
> Components: Core, Core Interceptors
> Reporter: Kusal Kithul-Godage
> Priority: Minor
> Fix For: 6.4.0
>
> Time Spent: 3h 40m
> Remaining Estimate: 0h
>
> struts.parameters.requireAnnotations
>
> Require an explicit annotation '@StrutsParameter' on one of:
> Getter/Setter/Field/ReturnType for injecting parameters.
>
> This mechanism is intended to be a more usable replacement for
> 'ParameterNameAware'
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
