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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
