[
https://issues.apache.org/jira/browse/WW-5343?focusedWorklogId=891833&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-891833
]
ASF GitHub Bot logged work on WW-5343:
--------------------------------------
Author: ASF GitHub Bot
Created on: 22/Nov/23 14:20
Start Date: 22/Nov/23 14:20
Worklog Time Spent: 10m
Work Description: kusalk commented on code in PR #791:
URL: https://github.com/apache/struts/pull/791#discussion_r1402127733
##########
core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java:
##########
@@ -133,7 +134,6 @@ public class DefaultConfiguration implements Configuration {
Map<String, Object> constants = new HashMap<>();
constants.put(StrutsConstants.STRUTS_DEVMODE, Boolean.FALSE);
constants.put(StrutsConstants.STRUTS_OGNL_LOG_MISSING_PROPERTIES,
Boolean.FALSE);
- constants.put(StrutsConstants.STRUTS_OGNL_ENABLE_EVAL_EXPRESSION,
Boolean.FALSE);
Review Comment:
This constant is not required for bootstrapping
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -84,27 +77,15 @@ public class OgnlUtil {
private final OgnlGuard ognlGuard;
private boolean devMode;
- private boolean enableExpressionCache = true;
+ private boolean enableExpressionCache;
private boolean enableEvalExpression;
- private Set<String> excludedClasses = emptySet();
Review Comment:
All the configuration is now injected directly into `SecurityMemberAccess`
except for devMode configuration
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -175,166 +156,110 @@ protected void setEnableEvalExpression(String
evalExpression) {
}
}
- @Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false)
+ /**
+ * @deprecated since 6.4.0, no replacement.
+ */
+ @Deprecated
protected void setExcludedClasses(String commaDelimitedClasses) {
- excludedClasses = toNewClassesSet(excludedClasses,
commaDelimitedClasses);
}
@Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required
= false)
protected void setDevModeExcludedClasses(String commaDelimitedClasses) {
- devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses,
commaDelimitedClasses);
+ this.devModeExcludedClasses = commaDelimitedClasses;
}
- private static Set<String> toClassesSet(String newDelimitedClasses) throws
ConfigurationException {
Review Comment:
Moved these utility methods into their own class
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -175,166 +156,110 @@ protected void setEnableEvalExpression(String
evalExpression) {
}
}
- @Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false)
+ /**
+ * @deprecated since 6.4.0, no replacement.
+ */
+ @Deprecated
protected void setExcludedClasses(String commaDelimitedClasses) {
- excludedClasses = toNewClassesSet(excludedClasses,
commaDelimitedClasses);
}
@Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required
= false)
protected void setDevModeExcludedClasses(String commaDelimitedClasses) {
- devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses,
commaDelimitedClasses);
+ this.devModeExcludedClasses = commaDelimitedClasses;
}
- private static Set<String> toClassesSet(String newDelimitedClasses) throws
ConfigurationException {
- Set<String> classNames =
commaDelimitedStringToSet(newDelimitedClasses);
- validateClasses(classNames, OgnlUtil.class.getClassLoader());
- return unmodifiableSet(classNames);
- }
-
- private static Set<String> toNewClassesSet(Set<String> oldClasses, String
newDelimitedClasses) throws ConfigurationException {
- Set<String> classNames =
commaDelimitedStringToSet(newDelimitedClasses);
- validateClasses(classNames, OgnlUtil.class.getClassLoader());
- Set<String> excludedClasses = new HashSet<>(oldClasses);
- excludedClasses.addAll(classNames);
- return unmodifiableSet(excludedClasses);
- }
-
- private static void validateClasses(Set<String> classNames, ClassLoader
validatingClassLoader) throws ConfigurationException {
- for (String className : classNames) {
- try {
- validatingClassLoader.loadClass(className);
- } catch (ClassNotFoundException e) {
- throw new ConfigurationException("Cannot load class for
exclusion/exemption configuration: " + className, e);
- }
- }
- }
-
- @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS,
required = false)
+ /**
+ * @deprecated since 6.4.0, no replacement.
+ */
+ @Deprecated
protected void setExcludedPackageNamePatterns(String
commaDelimitedPackagePatterns) {
- excludedPackageNamePatterns =
toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns);
}
@Inject(value =
StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required =
false)
protected void setDevModeExcludedPackageNamePatterns(String
commaDelimitedPackagePatterns) {
- devModeExcludedPackageNamePatterns =
toNewPatternsSet(devModeExcludedPackageNamePatterns,
commaDelimitedPackagePatterns);
+ this.devModeExcludedPackageNamePatterns =
commaDelimitedPackagePatterns;
}
- private static Set<Pattern> toNewPatternsSet(Set<Pattern> oldPatterns,
String newDelimitedPatterns) throws ConfigurationException {
- Set<String> patterns = commaDelimitedStringToSet(newDelimitedPatterns);
- Set<Pattern> newPatterns = new HashSet<>(oldPatterns);
- for (String pattern: patterns) {
- try {
- newPatterns.add(Pattern.compile(pattern));
- } catch (PatternSyntaxException e) {
- throw new ConfigurationException("Excluded package name
patterns could not be parsed due to invalid regex: " + pattern, e);
- }
- }
- return unmodifiableSet(newPatterns);
- }
-
- @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, required =
false)
+ /**
+ * @deprecated since 6.4.0, no replacement.
+ */
+ @Deprecated
protected void setExcludedPackageNames(String commaDelimitedPackageNames) {
- excludedPackageNames = toNewPackageNamesSet(excludedPackageNames,
commaDelimitedPackageNames);
}
@Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES,
required = false)
protected void setDevModeExcludedPackageNames(String
commaDelimitedPackageNames) {
- devModeExcludedPackageNames =
toNewPackageNamesSet(devModeExcludedPackageNames, commaDelimitedPackageNames);
- }
-
- private static Set<String> toPackageNamesSet(String
newDelimitedPackageNames) throws ConfigurationException {
- Set<String> packageNames =
commaDelimitedStringToSet(newDelimitedPackageNames)
- .stream().map(s -> strip(s, ".")).collect(toSet());
- validatePackageNames(packageNames);
- return unmodifiableSet(packageNames);
- }
-
- private static Set<String> toNewPackageNamesSet(Collection<String>
oldPackageNames, String newDelimitedPackageNames) throws ConfigurationException
{
- Set<String> packageNames =
commaDelimitedStringToSet(newDelimitedPackageNames)
- .stream().map(s -> strip(s, ".")).collect(toSet());
- validatePackageNames(packageNames);
- Set<String> newPackageNames = new HashSet<>(oldPackageNames);
- newPackageNames.addAll(packageNames);
- return unmodifiableSet(newPackageNames);
+ this.devModeExcludedPackageNames = commaDelimitedPackageNames;
}
- private static void validatePackageNames(Collection<String> packageNames) {
- if (packageNames.stream().anyMatch(s ->
Pattern.compile("\\s").matcher(s).find())) {
- throw new ConfigurationException("Excluded package names could not
be parsed due to erroneous whitespace characters: " + packageNames);
- }
- }
-
- @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES,
required = false)
+ /**
+ * @deprecated since 6.4.0, no replacement.
+ */
+ @Deprecated
public void setExcludedPackageExemptClasses(String commaDelimitedClasses) {
- excludedPackageExemptClasses = toClassesSet(commaDelimitedClasses);
}
@Inject(value =
StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required =
false)
public void setDevModeExcludedPackageExemptClasses(String
commaDelimitedClasses) {
- devModeExcludedPackageExemptClasses =
toClassesSet(commaDelimitedClasses);
+ this.devModeExcludedPackageExemptClasses = commaDelimitedClasses;
}
+ /**
+ * @deprecated since 6.4.0, no replacement.
+ */
+ @Deprecated
public Set<String> getExcludedClasses() {
- return excludedClasses;
+ return toClassesSet(container.getInstance(String.class,
StrutsConstants.STRUTS_EXCLUDED_CLASSES));
Review Comment:
Deprecated getters are still functional for API compatibility
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java:
##########
@@ -77,38 +77,91 @@ public class OgnlValueStack implements Serializable,
ValueStack, ClearableValueS
private boolean devMode;
private boolean logMissingProperties;
+ /**
+ * @since 6.4.0
+ */
+ protected OgnlValueStack(ValueStack vs,
+ XWorkConverter xworkConverter,
+ CompoundRootAccessor accessor,
+ TextProvider prov,
+ SecurityMemberAccess securityMemberAccess) {
+ setRoot(xworkConverter,
+ accessor,
+ vs != null ? new CompoundRoot(vs.getRoot()) : new
CompoundRoot(),
+ securityMemberAccess);
+ if (prov != null) {
+ push(prov);
+ }
+ }
+
+ /**
+ * @since 6.4.0
+ */
+ protected OgnlValueStack(XWorkConverter xworkConverter,
CompoundRootAccessor accessor, TextProvider prov, SecurityMemberAccess
securityMemberAccess) {
+ this(null, xworkConverter, accessor, prov, securityMemberAccess);
+ }
+
+ /**
+ * @since 6.4.0
+ */
+ protected OgnlValueStack(ValueStack vs, XWorkConverter xworkConverter,
CompoundRootAccessor accessor, SecurityMemberAccess securityMemberAccess) {
+ this(vs, xworkConverter, accessor, null, securityMemberAccess);
+ }
+
+ /**
+ * @deprecated since 6.4.0, use {@link #OgnlValueStack(ValueStack,
XWorkConverter, CompoundRootAccessor, TextProvider, SecurityMemberAccess)}
instead.
+ */
+ @Deprecated
+ protected OgnlValueStack(ValueStack vs,
+ XWorkConverter xworkConverter,
+ CompoundRootAccessor accessor,
+ TextProvider prov,
+ boolean allowStaticFieldAccess) {
+ this(vs, xworkConverter, accessor, prov, new
SecurityMemberAccess(allowStaticFieldAccess));
+ }
+
+ /**
+ * @deprecated since 6.4.0, use {@link #OgnlValueStack(XWorkConverter,
CompoundRootAccessor, TextProvider, SecurityMemberAccess)} instead.
+ */
+ @Deprecated
protected OgnlValueStack(XWorkConverter xworkConverter,
CompoundRootAccessor accessor, TextProvider prov, boolean
allowStaticFieldAccess) {
- setRoot(xworkConverter, accessor, new CompoundRoot(),
allowStaticFieldAccess);
- push(prov);
+ this(xworkConverter, accessor, prov, new
SecurityMemberAccess(allowStaticFieldAccess));
}
+ /**
+ * @deprecated since 6.4.0, use {@link #OgnlValueStack(ValueStack,
XWorkConverter, CompoundRootAccessor, SecurityMemberAccess)} instead.
+ */
+ @Deprecated
protected OgnlValueStack(ValueStack vs, XWorkConverter xworkConverter,
CompoundRootAccessor accessor, boolean allowStaticFieldAccess) {
- setRoot(xworkConverter, accessor, new CompoundRoot(vs.getRoot()),
allowStaticFieldAccess);
+ this(vs, xworkConverter, accessor, new
SecurityMemberAccess(allowStaticFieldAccess));
}
@Inject
protected void setOgnlUtil(OgnlUtil ognlUtil) {
this.ognlUtil = ognlUtil;
- securityMemberAccess.useExcludedClasses(ognlUtil.getExcludedClasses());
Review Comment:
No more fragile configuration setting duplicated in multiple places
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java:
##########
@@ -464,48 +517,42 @@ public int size() {
return root.size();
}
+ /**
+ * Retained for serializability - see {@link
com.opensymphony.xwork2.ognl.OgnlValueStackTest#testSerializable}
+ */
private Object readResolve() {
// TODO: this should be done better
ActionContext ac = ActionContext.getContext();
Container cont = ac.getContainer();
XWorkConverter xworkConverter = cont.getInstance(XWorkConverter.class);
CompoundRootAccessor accessor = (CompoundRootAccessor)
cont.getInstance(PropertyAccessor.class, CompoundRoot.class.getName());
TextProvider prov = cont.getInstance(TextProvider.class, "system");
- final boolean allowStaticField =
BooleanUtils.toBoolean(cont.getInstance(String.class,
StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS));
- OgnlValueStack aStack = new OgnlValueStack(xworkConverter, accessor,
prov, allowStaticField);
+ SecurityMemberAccess sma =
cont.getInstance(SecurityMemberAccess.class);
+ OgnlValueStack aStack = new OgnlValueStack(xworkConverter, accessor,
prov, sma);
aStack.setOgnlUtil(cont.getInstance(OgnlUtil.class));
- aStack.setRoot(xworkConverter, accessor, this.root, allowStaticField);
-
+ aStack.setRoot(xworkConverter, accessor, this.root, sma);
return aStack;
}
-
public void clearContextValues() {
//this is an OGNL ValueStack so the context will be an OgnlContext
//it would be better to make context of type OgnlContext
((OgnlContext) context).getValues().clear();
}
- @Deprecated
- public void setAcceptProperties(Set<Pattern> acceptedProperties) {
- securityMemberAccess.useAcceptProperties(acceptedProperties);
- }
-
public void useAcceptProperties(Set<Pattern> acceptedProperties) {
securityMemberAccess.useAcceptProperties(acceptedProperties);
}
- @Deprecated
- public void setExcludeProperties(Set<Pattern> excludeProperties) {
- securityMemberAccess.useExcludeProperties(excludeProperties);
- }
-
public void useExcludeProperties(Set<Pattern> excludeProperties) {
securityMemberAccess.useExcludeProperties(excludeProperties);
}
- @Inject
Review Comment:
Not sure why this was injected again given it's already passed into the
constructor
##########
core/src/main/java/com/opensymphony/xwork2/util/MemberAccessValueStack.java:
##########
@@ -31,15 +31,19 @@ public interface MemberAccessValueStack {
* @deprecated please use {@link #useExcludeProperties(Set)}
*/
@Deprecated
- void setExcludeProperties(Set<Pattern> excludeProperties);
+ default void setExcludeProperties(Set<Pattern> excludeProperties) {
+ useExcludeProperties(excludeProperties);
Review Comment:
Lifted the deprecated implementations into the interface for cleanliness
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -922,8 +855,7 @@ protected Map<String, Object> createDefaultContext(Object
root, ClassResolver cl
resolver = container.getInstance(CompoundRootAccessor.class);
}
- SecurityMemberAccess memberAccess = new
SecurityMemberAccess(allowStaticFieldAccess);
- memberAccess.disallowProxyMemberAccess(disallowProxyMemberAccess);
+ SecurityMemberAccess memberAccess =
container.getInstance(SecurityMemberAccess.class);
Review Comment:
This is a prototype bean so will be a new instance per context
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java:
##########
@@ -105,10 +98,10 @@ protected void setContainer(Container container) throws
ClassNotFoundException {
}
/**
- * Retrieve allowStaticFieldAccess state from the container (allows for
lazy fetching)
+ * @deprecated since 6.4.0, no replacement.
*/
+ @Deprecated
Review Comment:
Couldn't see any use for this method
##########
core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilStrutsTest.java:
##########
@@ -31,33 +31,6 @@ public void setUp() throws Exception {
ognlUtil = container.getInstance(OgnlUtil.class);
}
- public void testDefaultExcludes() {
Review Comment:
Deleted tests whose functionality is covered elsewhere
##########
core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java:
##########
@@ -1180,43 +1178,6 @@ public void
testOgnlValueStackFromOgnlValueStackFactoryDefaultConfig() {
assertNull("accessed private field (result not null) ?",
accessedValue);
}
- /**
- * Test a raw OgnlValueStackFactory and OgnlValueStack generated by it
- * when no static access flags are set (not present in configuration).
- */
- public void testOgnlValueStackFromOgnlValueStackFactoryNoFlagsSet() {
Review Comment:
This flag is now a required constant so this test is no longer applicable
##########
core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java:
##########
@@ -58,6 +62,82 @@ protected void assignNewSma(boolean allowStaticFieldAccess) {
sma = new SecurityMemberAccess(allowStaticFieldAccess);
}
+ private <T> T reflectField(String fieldName) throws IllegalAccessException
{
+ return (T) FieldUtils.readField(sma, fieldName, true);
+ }
+
+ @Test
+ public void defaultExclusionList() throws Exception {
Review Comment:
Moved/rewrote some tests here from `OgnlUtilTest`
##########
core/src/main/java/org/apache/struts2/StrutsConstants.java:
##########
@@ -234,6 +234,8 @@ public final class StrutsConstants {
/** The name of the parameter to determine whether static field access
will be allowed in OGNL expressions or not */
public static final String STRUTS_ALLOW_STATIC_FIELD_ACCESS =
"struts.ognl.allowStaticFieldAccess";
+ public static final String STRUTS_MEMBER_ACCESS =
"struts.securityMemberAccess";
Review Comment:
Extension point!
Issue Time Tracking
-------------------
Worklog Id: (was: 891833)
Time Spent: 0.5h (was: 20m)
> Make SecurityMemberAccess extensible and a prototype bean
> ---------------------------------------------------------
>
> Key: WW-5343
> URL: https://issues.apache.org/jira/browse/WW-5343
> Project: Struts 2
> Issue Type: Improvement
> Components: Core
> Reporter: Kusal Kithul-Godage
> Priority: Minor
> Fix For: 6.4.0
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
