Commit 5b422b94 authored by Guus der Kinderen's avatar Guus der Kinderen

OF-1007: Don't over-optimize.

Instead of a Set, the existing code uses a Map with empty string values 'for performance'.
I am not buying it. Perhaps true in some performance test long, long ago, but there won't
be any significant performance increase in doing this (it's just as likely to introduce
a performance penalty). In any case, when we're running into a performance bottleneck here,
we've got bigger fish to fry.
parent 5e0c7e49
...@@ -333,17 +333,15 @@ public class IQAuthHandler extends IQHandler implements IQAuthInfo { ...@@ -333,17 +333,15 @@ public class IQAuthHandler extends IQHandler implements IQAuthInfo {
boolean forbidAccess = false; boolean forbidAccess = false;
try { try {
String hostAddress = session.getConnection().getHostAddress(); String hostAddress = session.getConnection().getHostAddress();
if (!LocalClientSession.getAllowedAnonymIPs().isEmpty() && if (!LocalClientSession.getWhitelistedAnonymousIPs().isEmpty() && !LocalClientSession.getWhitelistedAnonymousIPs().contains( hostAddress )) {
!LocalClientSession.getAllowedAnonymIPs().containsKey(hostAddress)) {
byte[] address = session.getConnection().getAddress(); byte[] address = session.getConnection().getAddress();
String range1 = (address[0] & 0xff) + "." + (address[1] & 0xff) + "." + String range1 = (address[0] & 0xff) + "." + (address[1] & 0xff) + "." + (address[2] & 0xff) + ".*";
(address[2] & 0xff) +
".*";
String range2 = (address[0] & 0xff) + "." + (address[1] & 0xff) + ".*.*"; String range2 = (address[0] & 0xff) + "." + (address[1] & 0xff) + ".*.*";
String range3 = (address[0] & 0xff) + ".*.*.*"; String range3 = (address[0] & 0xff) + ".*.*.*";
if (!LocalClientSession.getAllowedAnonymIPs().containsKey(range1) && if (!LocalClientSession.getWhitelistedAnonymousIPs().contains(range1) &&
!LocalClientSession.getAllowedAnonymIPs().containsKey(range2) && !LocalClientSession.getWhitelistedAnonymousIPs().contains(range2) &&
!LocalClientSession.getAllowedAnonymIPs().containsKey(range3)) { !LocalClientSession.getWhitelistedAnonymousIPs().contains(range3))
{
forbidAccess = true; forbidAccess = true;
} }
} }
......
...@@ -475,17 +475,16 @@ public class SASLAuthentication { ...@@ -475,17 +475,16 @@ public class SASLAuthentication {
boolean forbidAccess = false; boolean forbidAccess = false;
try { try {
String hostAddress = session.getConnection().getHostAddress(); String hostAddress = session.getConnection().getHostAddress();
if (!LocalClientSession.getAllowedAnonymIPs().isEmpty() && if (!LocalClientSession.getWhitelistedAnonymousIPs().isEmpty() &&
!LocalClientSession.getAllowedAnonymIPs().containsKey(hostAddress)) { !LocalClientSession.getWhitelistedAnonymousIPs().contains(hostAddress)) {
byte[] address = session.getConnection().getAddress(); byte[] address = session.getConnection().getAddress();
String range1 = (address[0] & 0xff) + "." + (address[1] & 0xff) + "." + String range1 = (address[0] & 0xff) + "." + (address[1] & 0xff) + "." + (address[2] & 0xff) + ".*";
(address[2] & 0xff) +
".*";
String range2 = (address[0] & 0xff) + "." + (address[1] & 0xff) + ".*.*"; String range2 = (address[0] & 0xff) + "." + (address[1] & 0xff) + ".*.*";
String range3 = (address[0] & 0xff) + ".*.*.*"; String range3 = (address[0] & 0xff) + ".*.*.*";
if (!LocalClientSession.getAllowedAnonymIPs().containsKey(range1) && if (!LocalClientSession.getWhitelistedAnonymousIPs().contains(range1) &&
!LocalClientSession.getAllowedAnonymIPs().containsKey(range2) && !LocalClientSession.getWhitelistedAnonymousIPs().contains(range2) &&
!LocalClientSession.getAllowedAnonymIPs().containsKey(range3)) { !LocalClientSession.getWhitelistedAnonymousIPs().contains(range3))
{
forbidAccess = true; forbidAccess = true;
} }
} }
......
...@@ -21,12 +21,7 @@ ...@@ -21,12 +21,7 @@
package org.jivesoftware.openfire.session; package org.jivesoftware.openfire.session;
import java.net.UnknownHostException; import java.net.UnknownHostException;
import java.util.Date; import java.util.*;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Locale;
import java.util.Map;
import java.util.StringTokenizer;
import org.jivesoftware.openfire.Connection; import org.jivesoftware.openfire.Connection;
import org.jivesoftware.openfire.SessionManager; import org.jivesoftware.openfire.SessionManager;
...@@ -71,11 +66,10 @@ public class LocalClientSession extends LocalSession implements ClientSession { ...@@ -71,11 +66,10 @@ public class LocalClientSession extends LocalSession implements ClientSession {
* Keep the list of IP address that are allowed to connect to the server. If the list is * Keep the list of IP address that are allowed to connect to the server. If the list is
* empty then anyone is allowed to connect to the server.<p> * empty then anyone is allowed to connect to the server.<p>
* *
* Note: Key = IP address or IP range; Value = empty string. A hash map is being used for * Note: value = IP address or IP range
* performance reasons.
*/ */
private static Map<String,String> allowedIPs = new HashMap<>(); private static Set<String> allowedIPs = new HashSet<>();
private static Map<String,String> allowedAnonymIPs = new HashMap<>(); private static Set<String> allowedAnonymIPs = new HashSet<>();
private boolean messageCarbonsEnabled; private boolean messageCarbonsEnabled;
...@@ -124,14 +118,13 @@ public class LocalClientSession extends LocalSession implements ClientSession { ...@@ -124,14 +118,13 @@ public class LocalClientSession extends LocalSession implements ClientSession {
StringTokenizer tokens = new StringTokenizer(allowed, ", "); StringTokenizer tokens = new StringTokenizer(allowed, ", ");
while (tokens.hasMoreTokens()) { while (tokens.hasMoreTokens()) {
String address = tokens.nextToken().trim(); String address = tokens.nextToken().trim();
allowedIPs.put(address, ""); allowedIPs.add( address );
} }
String allowedAnonym = JiveGlobals.getProperty(ConnectionSettings.Client.LOGIN_ANONYM_ALLOWED, ""); String allowedAnonym = JiveGlobals.getProperty(ConnectionSettings.Client.LOGIN_ANONYM_ALLOWED, "");
tokens = new StringTokenizer(allowedAnonym, ", "); tokens = new StringTokenizer(allowedAnonym, ", ");
while (tokens.hasMoreTokens()) { while (tokens.hasMoreTokens()) {
String address = tokens.nextToken().trim(); String address = tokens.nextToken().trim();
allowedAnonymIPs.put(address, ""); allowedAnonymIPs.add(address);
} }
} }
...@@ -142,19 +135,52 @@ public class LocalClientSession extends LocalSession implements ClientSession { ...@@ -142,19 +135,52 @@ public class LocalClientSession extends LocalSession implements ClientSession {
* non-anonymous users. * non-anonymous users.
* *
* @return the list of IP address that are allowed to connect to the server. * @return the list of IP address that are allowed to connect to the server.
* @deprecated Use #getWhitelistedIPs instead.
*/ */
public static Map<String, String> getAllowedIPs() { @Deprecated
return allowedIPs; public static Map<String, String> getAllowedIPs()
{
final Map<String, String> result = new HashMap<>();
for ( String item : allowedIPs )
{
result.put( item, null );
}
return result;
} }
/**
* Returns the list of IP address that are allowed to connect to the server. If the list is empty then anyone is
* allowed to connect to the server except for anonymous users that are subject to
* {@link #getWhitelistedAnonymousIPs()}. This list is used for both anonymous and non-anonymous users.
*
* @return the collection of IP address that are allowed to connect to the server. Never null, possibly empty.
*/
public static Set<String> getWhitelistedIPs() { return allowedIPs; }
/** /**
* Returns the list of IP address that are allowed to connect to the server for anonymous * Returns the list of IP address that are allowed to connect to the server for anonymous
* users. If the list is empty then anonymous will be only restricted by {@link #getAllowedIPs()}. * users. If the list is empty then anonymous will be only restricted by {@link #getAllowedIPs()}.
* *
* @return the list of IP address that are allowed to connect to the server. * @return the list of IP address that are allowed to connect to the server.
* @deprecated Use #getWhitelistedAnonymousIPs instead.
*/
public static Map<String, String> getAllowedAnonymIPs()
{
final Map<String, String> result = new HashMap<>();
for ( String item : allowedAnonymIPs )
{
result.put( item, null );
}
return result;
}
/**
* Returns the list of IP address that are allowed to connect to the server for anonymous users. If the list is
* empty then anonymous will be only restricted by {@link #getWhitelistedIPs()}.
*
* @return the collection of IP address that are allowed to connect to the server. Never null, possibly empty.
*/ */
public static Map<String, String> getAllowedAnonymIPs() { public static Set<String> getWhitelistedAnonymousIPs() {
return allowedAnonymIPs; return allowedAnonymIPs;
} }
...@@ -339,15 +365,12 @@ public class LocalClientSession extends LocalSession implements ClientSession { ...@@ -339,15 +365,12 @@ public class LocalClientSession extends LocalSession implements ClientSession {
// is authorized to connect to the server // is authorized to connect to the server
boolean forbidAccess = false; boolean forbidAccess = false;
try { try {
if (!allowedIPs.containsKey(connection.getHostAddress())) { if (!allowedIPs.contains(connection.getHostAddress())) {
byte[] address = connection.getAddress(); byte[] address = connection.getAddress();
String range1 = (address[0] & 0xff) + "." + (address[1] & 0xff) + "." + String range1 = (address[0] & 0xff) + "." + (address[1] & 0xff) + "." + (address[2] & 0xff) + ".*";
(address[2] & 0xff) +
".*";
String range2 = (address[0] & 0xff) + "." + (address[1] & 0xff) + ".*.*"; String range2 = (address[0] & 0xff) + "." + (address[1] & 0xff) + ".*.*";
String range3 = (address[0] & 0xff) + ".*.*.*"; String range3 = (address[0] & 0xff) + ".*.*.*";
if (!allowedIPs.containsKey(range1) && !allowedIPs.containsKey(range2) && if (!allowedIPs.contains(range1) && !allowedIPs.contains(range2) && !allowedIPs.contains(range3)) {
!allowedIPs.containsKey(range3)) {
forbidAccess = true; forbidAccess = true;
} }
} }
...@@ -366,8 +389,24 @@ public class LocalClientSession extends LocalSession implements ClientSession { ...@@ -366,8 +389,24 @@ public class LocalClientSession extends LocalSession implements ClientSession {
* non-anonymous users. * non-anonymous users.
* *
* @param allowed the list of IP address that are allowed to connect to the server. * @param allowed the list of IP address that are allowed to connect to the server.
* @deprecated Use setWhitelistedIPs instead.
*/ */
@Deprecated
public static void setAllowedIPs(Map<String, String> allowed) { public static void setAllowedIPs(Map<String, String> allowed) {
setWhitelistedIPs( allowed.keySet() );
}
/**
* Sets the list of IP address that are allowed to connect to the server. If the list is empty then anyone is
* allowed to connect to the server except for anonymous users that are subject to
* {@link #getWhitelistedAnonymousIPs()}. This list is used for both anonymous and non-anonymous users.
*
* @param allowed the list of IP address that are allowed to connect to the server. Can be empty, but not null.
*/
public static void setWhitelistedIPs(Set<String> allowed) {
if (allowed == null) {
throw new NullPointerException();
}
allowedIPs = allowed; allowedIPs = allowed;
if (allowedIPs.isEmpty()) { if (allowedIPs.isEmpty()) {
JiveGlobals.deleteProperty(ConnectionSettings.Client.LOGIN_ALLOWED); JiveGlobals.deleteProperty(ConnectionSettings.Client.LOGIN_ALLOWED);
...@@ -375,7 +414,7 @@ public class LocalClientSession extends LocalSession implements ClientSession { ...@@ -375,7 +414,7 @@ public class LocalClientSession extends LocalSession implements ClientSession {
else { else {
// Iterate through the elements in the map. // Iterate through the elements in the map.
StringBuilder buf = new StringBuilder(); StringBuilder buf = new StringBuilder();
Iterator<String> iter = allowedIPs.keySet().iterator(); Iterator<String> iter = allowedIPs.iterator();
if (iter.hasNext()) { if (iter.hasNext()) {
buf.append(iter.next()); buf.append(iter.next());
} }
...@@ -391,8 +430,23 @@ public class LocalClientSession extends LocalSession implements ClientSession { ...@@ -391,8 +430,23 @@ public class LocalClientSession extends LocalSession implements ClientSession {
* users. If the list is empty then anonymous will be only restricted by {@link #getAllowedIPs()}. * users. If the list is empty then anonymous will be only restricted by {@link #getAllowedIPs()}.
* *
* @param allowed the list of IP address that are allowed to connect to the server. * @param allowed the list of IP address that are allowed to connect to the server.
* @deprecated use #setWhitelistedAnonymousIPs instead.
*/ */
@Deprecated
public static void setAllowedAnonymIPs(Map<String, String> allowed) { public static void setAllowedAnonymIPs(Map<String, String> allowed) {
setWhitelistedAnonymousIPs( allowed.keySet() );
}
/**
* Sets the list of IP address that are allowed to connect to the server for anonymous users. If the list is empty
* then anonymous will be only restricted by {@link #getWhitelistedIPs()}.
*
* @param allowed the list of IP address that are allowed to connect to the server. Can be empty, but not null.
*/
public static void setWhitelistedAnonymousIPs(Set<String> allowed) {
if (allowed == null) {
throw new NullPointerException();
}
allowedAnonymIPs = allowed; allowedAnonymIPs = allowed;
if (allowedAnonymIPs.isEmpty()) { if (allowedAnonymIPs.isEmpty()) {
JiveGlobals.deleteProperty(ConnectionSettings.Client.LOGIN_ANONYM_ALLOWED); JiveGlobals.deleteProperty(ConnectionSettings.Client.LOGIN_ANONYM_ALLOWED);
...@@ -400,7 +454,7 @@ public class LocalClientSession extends LocalSession implements ClientSession { ...@@ -400,7 +454,7 @@ public class LocalClientSession extends LocalSession implements ClientSession {
else { else {
// Iterate through the elements in the map. // Iterate through the elements in the map.
StringBuilder buf = new StringBuilder(); StringBuilder buf = new StringBuilder();
Iterator<String> iter = allowedAnonymIPs.keySet().iterator(); Iterator<String> iter = allowedAnonymIPs.iterator();
if (iter.hasNext()) { if (iter.hasNext()) {
buf.append(iter.next()); buf.append(iter.next());
} }
...@@ -409,6 +463,7 @@ public class LocalClientSession extends LocalSession implements ClientSession { ...@@ -409,6 +463,7 @@ public class LocalClientSession extends LocalSession implements ClientSession {
} }
JiveGlobals.setProperty(ConnectionSettings.Client.LOGIN_ANONYM_ALLOWED, buf.toString()); JiveGlobals.setProperty(ConnectionSettings.Client.LOGIN_ANONYM_ALLOWED, buf.toString());
} }
} }
/** /**
......
...@@ -24,11 +24,8 @@ ...@@ -24,11 +24,8 @@
org.jivesoftware.util.ParamUtils" org.jivesoftware.util.ParamUtils"
errorPage="error.jsp" errorPage="error.jsp"
%> %>
<%@ page import="java.util.HashMap"%>
<%@ page import="java.util.Iterator"%>
<%@ page import="java.util.Map"%>
<%@ page import="java.util.StringTokenizer"%>
<%@ page import="java.util.regex.Pattern" %> <%@ page import="java.util.regex.Pattern" %>
<%@ page import="java.util.*" %>
<%@ taglib uri="http://java.sun.com/jsp/jstl/core" prefix="c" %> <%@ taglib uri="http://java.sun.com/jsp/jstl/core" prefix="c" %>
<%@ taglib uri="http://java.sun.com/jsp/jstl/fmt" prefix="fmt" %> <%@ taglib uri="http://java.sun.com/jsp/jstl/fmt" prefix="fmt" %>
...@@ -64,26 +61,26 @@ ...@@ -64,26 +61,26 @@
Pattern pattern = Pattern.compile("(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.)" + Pattern pattern = Pattern.compile("(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.)" +
"(?:(?:\\*|25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){2}" + "(?:(?:\\*|25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){2}" +
"(?:\\*|25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)"); "(?:\\*|25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)");
Map<String, String> newMap = new HashMap<String, String>(); Set<String> allowedSet = new HashSet<String>();
StringTokenizer tokens = new StringTokenizer(allowedIPs, ", "); StringTokenizer tokens = new StringTokenizer(allowedIPs, ", ");
while (tokens.hasMoreTokens()) { while (tokens.hasMoreTokens()) {
String address = tokens.nextToken().trim(); String address = tokens.nextToken().trim();
if (pattern.matcher(address).matches()) { if (pattern.matcher(address).matches()) {
newMap.put(address, ""); allowedSet.add(address);
} }
} }
Map<String, String> allowedAnonymMap = new HashMap<String, String>(); Set<String> allowedAnonymousSet = new HashSet<String>();
StringTokenizer tokens1 = new StringTokenizer(allowedAnonymIPs, ", "); StringTokenizer tokens1 = new StringTokenizer(allowedAnonymIPs, ", ");
while (tokens1.hasMoreTokens()) { while (tokens1.hasMoreTokens()) {
String address = tokens1.nextToken().trim(); String address = tokens1.nextToken().trim();
if (pattern.matcher(address).matches()) { if (pattern.matcher(address).matches()) {
allowedAnonymMap.put(address, ""); allowedAnonymousSet.add(address);
} }
} }
LocalClientSession.setAllowedIPs(newMap); LocalClientSession.setWhitelistedIPs( allowedSet );
LocalClientSession.setAllowedAnonymIPs(allowedAnonymMap); LocalClientSession.setWhitelistedAnonymousIPs( allowedAnonymousSet );
// Log the event // Log the event
webManager.logEvent("edited registration settings", "inband enabled = "+inbandEnabled+"\ncan change password = "+canChangePassword+"\nanon login = "+anonLogin+"\nallowed ips = "+allowedIPs); webManager.logEvent("edited registration settings", "inband enabled = "+inbandEnabled+"\ncan change password = "+canChangePassword+"\nanon login = "+anonLogin+"\nallowed ips = "+allowedIPs);
...@@ -95,7 +92,7 @@ ...@@ -95,7 +92,7 @@
anonLogin = authHandler.isAnonymousAllowed(); anonLogin = authHandler.isAnonymousAllowed();
// Encode the allowed IP addresses // Encode the allowed IP addresses
StringBuilder buf = new StringBuilder(); StringBuilder buf = new StringBuilder();
Iterator<String> iter = org.jivesoftware.openfire.session.LocalClientSession.getAllowedIPs().keySet().iterator(); Iterator<String> iter = org.jivesoftware.openfire.session.LocalClientSession.getWhitelistedIPs().iterator();
if (iter.hasNext()) { if (iter.hasNext()) {
buf.append(iter.next()); buf.append(iter.next());
} }
...@@ -105,7 +102,7 @@ ...@@ -105,7 +102,7 @@
allowedIPs = buf.toString(); allowedIPs = buf.toString();
StringBuilder buf1 = new StringBuilder(); StringBuilder buf1 = new StringBuilder();
Iterator<String> iter1 = org.jivesoftware.openfire.session.LocalClientSession.getAllowedAnonymIPs().keySet().iterator(); Iterator<String> iter1 = org.jivesoftware.openfire.session.LocalClientSession.getWhitelistedAnonymousIPs().iterator();
if (iter1.hasNext()) { if (iter1.hasNext()) {
buf1.append(iter1.next()); buf1.append(iter1.next());
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment