It depends on what required means here. I committed those because Matcher class is not thread safe. I must say I did not cross any issues before doing these changes. They are more thread safe issues prevention.
So I believe it's safer to do it this way. Though I'm not quite sure pattern.matcher is atomic. So maybe using synchronized could be required to guarantee thread safe. But contrary to the change I did in ProductEvents.java and changed again at r1541746, synchronized is not necessary here (nor in other changes I did in r1541641) It's the same behaviour (compiled pattern is not reused) than before whith a chance that pattern.matcher is atomic, thus prevents possible issues. What are your concerns? Jacques On Thursday, November 14, 2013 10:25 AM Jacopo Cappellato <[email protected]> wrote: > Are these changes required? Can we revert them? > > Jacopo > > > On Nov 13, 2013, at 7:04 PM, [email protected] wrote: > >> Modified: >> ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java >> URL: >> http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java?rev=1541641&r1=1541640&r2=1541641&view=diff >> ============================================================================== >> --- >> ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java >> (original) +++ >> ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java >> Wed Nov 13 18:04:14 2013 @@ -23,7 +23,6 @@ import >> java.sql.Timestamp; >> import java.util.List; >> import java.util.Locale; >> import java.util.Map; >> -import java.util.regex.Matcher; >> import java.util.regex.Pattern; >> >> import javax.transaction.Transaction; >> @@ -957,9 +956,7 @@ public class LoginServices { >> boolean usePasswordPattern = >> UtilProperties.getPropertyAsBoolean("security.properties", >> "security.login.password.pattern.enable", true); if >> (usePasswordPattern) { >> Pattern pattern = Pattern.compile(passwordPattern); >> - Matcher matcher = pattern.matcher(newPassword); >> - boolean matched = matcher.matches(); >> - if (!matched) { >> + if (!pattern.matcher(newPassword).matches()) { >> // This is a mix to handle the OOTB pattern which is >> only a fixed length >> Map<String, String> messageMap = >> UtilMisc.toMap("minPasswordLength", Integer.toString(minPasswordLength)); >> String passwordPatternMessage = >> UtilProperties.getPropertyValue("security.properties", >> >> Modified: >> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java >> URL: >> http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java?rev=1541641&r1=1541640&r2=1541641&view=diff >> ============================================================================== >> --- >> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java >> (original) +++ >> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java >> Wed Nov 13 18:04:14 2013 @@ -26,7 +26,6 @@ import >> java.sql.Timestamp; >> import java.util.List; >> import java.util.Map; >> import java.util.ServiceLoader; >> -import java.util.regex.Matcher; >> import java.util.regex.Pattern; >> >> import javax.servlet.ServletContext; >> @@ -925,9 +924,8 @@ public class LoginWorker { >> if (i == 0) { >> String cn = x500Map.get("CN"); >> cn = cn.replaceAll("\\\\", ""); >> - Matcher m = pattern.matcher(cn); >> - if (m.matches()) { >> - userLoginId = m.group(1); >> + if (pattern.matcher(cn).matches()) { >> + userLoginId = pattern.matcher(cn).group(1); >> } else { >> Debug.logInfo("Client certificate CN does >> not match pattern: [" + cnPattern + "]", module); >> }
