QPID-4334: addressed Keith's review comments. Also moved PlainConfigurationTest to the correct package.

Applied patch from Philip Harvey <phil@philharveyonline.com>.

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1391431 13f79535-47bb-0310-9956-ffa450edef68

8 files changed, 211 insertions, 88 deletions

diff --git a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/AclRulePredicates.java b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/AclRulePredicates.java
index 0ea6b2fc9c..45af85be6c 100644
--- a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/AclRulePredicates.java
+++ b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/AclRulePredicates.java
@@ -61,7 +61,10 @@ public class AclRulePredicates
             _properties.put(property, value);
         }
 
-        _logger.debug("Parsed " + property + " with value " + value);
+        if (_logger.isDebugEnabled())
+        {
+            _logger.debug("Parsed " + property + " with value " + value);
+        }
     }
 
     private void checkFirewallRuleNotAlreadyDefined(String key, String value)
diff --git a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/Action.java b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/Action.java
index d244af480a..4fff0bebf5 100644
--- a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/Action.java
+++ b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/Action.java
@@ -43,9 +43,9 @@ import org.apache.qpid.server.security.access.Operation;
  */
 public class Action
 {
-    private Operation _operation;
-    private ObjectType _object;
-    private ObjectProperties _properties;
+    private final Operation _operation;
+    private final ObjectType _object;
+    private final ObjectProperties _properties;
 
     public Action(Operation operation)
     {
@@ -64,9 +64,9 @@ public class Action
 
     public Action(Operation operation, ObjectType object, ObjectProperties properties)
     {
-        setOperation(operation);
-        setObjectType(object);
-        setProperties(properties);
+        _operation = operation;
+        _object = object;
+        _properties = properties;
     }
 
     public Operation getOperation()
@@ -74,44 +74,63 @@ public class Action
         return _operation;
     }
 
-    public void setOperation(Operation operation)
-    {
-        _operation = operation;
-    }
-
     public ObjectType getObjectType()
     {
         return _object;
     }
 
-    public void setObjectType(ObjectType object)
+    public ObjectProperties getProperties()
     {
-        _object = object;
+        return _properties;
     }
 
-    public ObjectProperties getProperties()
+    public boolean isAllowed()
     {
-        return _properties;
+        return _object.isAllowed(_operation);
     }
 
-    public void setProperties(ObjectProperties properties)
+    public boolean matches(Action a)
     {
-        _properties = properties;
+        if (!operationsMatch(a))
+        {
+            return false;
+        }
+
+        if (!objectTypesMatch(a))
+        {
+            return false;
+        }
+
+        if (!propertiesMatch(a))
+        {
+            return false;
+        }
+
+        return true;
     }
 
-    public boolean isAllowed()
+    private boolean operationsMatch(Action a)
     {
-        return _object.isAllowed(_operation);
+        return Operation.ALL == a.getOperation() || getOperation() == a.getOperation();
     }
 
-    /** @see Comparable#compareTo(Object) */
-    public boolean matches(Action a)
+    private boolean objectTypesMatch(Action a)
     {
-        boolean operationMatches = Operation.ALL == a.getOperation() || getOperation() == a.getOperation();
-        boolean objectTypeMatches = ObjectType.ALL == a.getObjectType() || getObjectType() == a.getObjectType();
-        boolean propertiesMatch = _properties.matches(a.getProperties());
+        return ObjectType.ALL == a.getObjectType() || getObjectType() == a.getObjectType();
+    }
 
-        return (operationMatches && objectTypeMatches && propertiesMatch);
+    private boolean propertiesMatch(Action a)
+    {
+        boolean propertiesMatch = false;
+        if (_properties != null)
+        {
+            propertiesMatch = _properties.matches(a.getProperties());
+        }
+        else if (a.getProperties() == null)
+        {
+            propertiesMatch = true;
+        }
+        return propertiesMatch;
     }
 
     @Override
diff --git a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/PlainConfiguration.java b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/PlainConfiguration.java
index 9f56b05e0f..86f8ca3217 100644
--- a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/PlainConfiguration.java
+++ b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/config/PlainConfiguration.java
@@ -49,20 +49,21 @@ public class PlainConfiguration extends AbstractConfiguration
     public static final String ACL = "acl";
     public static final String CONFIG = "config";
 
-    public static final String UNRECOGNISED_INITIAL_MSG = "Unrecognised initial token '%s' at line %d";
-    public static final String NOT_ENOUGH_TOKENS_MSG = "Not enough tokens at line %d";
-    public static final String NUMBER_NOT_ALLOWED_MSG = "Number not allowed before '%s' at line %d";
-    public static final String CANNOT_LOAD_MSG = "Cannot load config file %s";
-    public static final String PREMATURE_CONTINUATION_MSG = "Premature continuation character at line %d";
-    public static final String PREMATURE_EOF_MSG = "Premature end of file reached at line %d";
-    public static final String PARSE_TOKEN_FAILED_MSG = "Failed to parse token at line %d";
-    public static final String CONFIG_NOT_FOUND_MSG = "Cannot find config file %s";
-    public static final String NOT_ENOUGH_ACL_MSG = "Not enough data for an acl at line %d";
-    public static final String NOT_ENOUGH_CONFIG_MSG = "Not enough data for config at line %d";
-    public static final String BAD_ACL_RULE_NUMBER_MSG = "Invalid rule number at line %d";
-    public static final String PROPERTY_KEY_ONLY_MSG = "Incomplete property (key only) at line %d";
-    public static final String PROPERTY_NO_EQUALS_MSG = "Incomplete property (no equals) at line %d";
-    public static final String PROPERTY_NO_VALUE_MSG = "Incomplete property (no value) at line %d";
+    static final String UNRECOGNISED_INITIAL_MSG = "Unrecognised initial token '%s' at line %d";
+    static final String NOT_ENOUGH_TOKENS_MSG = "Not enough tokens at line %d";
+    static final String NUMBER_NOT_ALLOWED_MSG = "Number not allowed before '%s' at line %d";
+    static final String CANNOT_LOAD_MSG = "Cannot load config file %s";
+    static final String CANNOT_CLOSE_MSG = "Cannot close config file %s";
+    static final String PREMATURE_CONTINUATION_MSG = "Premature continuation character at line %d";
+    static final String PREMATURE_EOF_MSG = "Premature end of file reached at line %d";
+    static final String PARSE_TOKEN_FAILED_MSG = "Failed to parse token at line %d";
+    static final String CONFIG_NOT_FOUND_MSG = "Cannot find config file %s";
+    static final String NOT_ENOUGH_ACL_MSG = "Not enough data for an acl at line %d";
+    static final String NOT_ENOUGH_CONFIG_MSG = "Not enough data for config at line %d";
+    static final String BAD_ACL_RULE_NUMBER_MSG = "Invalid rule number at line %d";
+    static final String PROPERTY_KEY_ONLY_MSG = "Incomplete property (key only) at line %d";
+    static final String PROPERTY_NO_EQUALS_MSG = "Incomplete property (no equals) at line %d";
+    static final String PROPERTY_NO_VALUE_MSG = "Incomplete property (no value) at line %d";
 
     private StreamTokenizer _st;
 
@@ -77,6 +78,7 @@ public class PlainConfiguration extends AbstractConfiguration
         RuleSet ruleSet = super.load();
 
         File file = getFile();
+        FileReader fileReader = null;
 
         try
         {
@@ -85,7 +87,8 @@ public class PlainConfiguration extends AbstractConfiguration
                 _logger.debug("About to load ACL file " + file);
             }
 
-            _st = new StreamTokenizer(new BufferedReader(new FileReader(file)));
+            fileReader = new FileReader(file);
+            _st = new StreamTokenizer(new BufferedReader(fileReader));
             _st.resetSyntax(); // setup the tokenizer
 
             _st.commentChar(COMMENT); // single line comments
@@ -210,6 +213,21 @@ public class PlainConfiguration extends AbstractConfiguration
         {
             throw new ConfigurationException(String.format(CANNOT_LOAD_MSG, file.getName()), ioe);
         }
+        finally
+        {
+            if(fileReader != null)
+            {
+                try
+                {
+                    fileReader.close();
+                }
+                catch (IOException e)
+                {
+                    throw new ConfigurationException(String.format(CANNOT_CLOSE_MSG, file.getName()), e);
+                }
+            }
+        }
+
 
         return ruleSet;
     }
diff --git a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/firewall/AccessControlFirewallException.java b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/firewall/AccessControlFirewallException.java
index efae7f5653..d08a052efd 100644
--- a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/firewall/AccessControlFirewallException.java
+++ b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/firewall/AccessControlFirewallException.java
@@ -25,19 +25,23 @@ public class AccessControlFirewallException extends RuntimeException
     /** serialVersionUID */
     private static final long serialVersionUID = 4526157149690917805L;
 
-    public AccessControlFirewallException() {
-	    super();
+    public AccessControlFirewallException()
+    {
+        super();
     }
 
-    public AccessControlFirewallException(String message) {
-	    super(message);
+    public AccessControlFirewallException(String message)
+    {
+        super(message);
     }
 
-    public AccessControlFirewallException(String message, Throwable cause) {
+    public AccessControlFirewallException(String message, Throwable cause)
+    {
         super(message, cause);
     }
 
-    public AccessControlFirewallException(Throwable cause) {
+    public AccessControlFirewallException(Throwable cause)
+    {
         super(cause);
     }
 }
\ No newline at end of file
diff --git a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/firewall/HostnameFirewallRule.java b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/firewall/HostnameFirewallRule.java
index 9d60c6d745..fb13426fbb 100644
--- a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/firewall/HostnameFirewallRule.java
+++ b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/firewall/HostnameFirewallRule.java
@@ -65,13 +65,12 @@ public class HostnameFirewallRule implements FirewallRule
         String hostname = getHostname(remote);
         if (hostname == null)
         {
-            throw new AccessControlFirewallException("DNS lookup failed");
+            throw new AccessControlFirewallException("DNS lookup failed for address " + remote);
         }
         for (Pattern pattern : _hostnamePatterns)
         {
             boolean hostnameMatches = pattern.matcher(hostname).matches();
 
-
             if (hostnameMatches)
             {
                 if(_logger.isDebugEnabled())
@@ -114,6 +113,7 @@ public class HostnameFirewallRule implements FirewallRule
         }
         catch (Exception e)
         {
+            _logger.warn("Unable to look up hostname from address " + remote, e);
             return null;
         }
         finally
diff --git a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/firewall/InetNetwork.java b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/firewall/InetNetwork.java
index 52516af84c..2e979b38f1 100644
--- a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/firewall/InetNetwork.java
+++ b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/firewall/InetNetwork.java
@@ -32,7 +32,7 @@ class InetNetwork
 
     public InetNetwork(InetAddress ip, InetAddress netmask)
     {
-        network = maskIP(ip, netmask);
+        this.network = maskIP(ip, netmask);
         this.netmask = netmask;
     }
 
@@ -46,20 +46,25 @@ class InetNetwork
         return network.equals(maskIP(ip, netmask));
     }
 
+    @Override
     public String toString()
     {
         return network.getHostAddress() + "/" + netmask.getHostAddress();
     }
 
+    @Override
     public int hashCode()
     {
         return maskIP(network, netmask).hashCode();
     }
 
+    @Override
     public boolean equals(Object obj)
     {
-        return (obj != null) && (obj instanceof InetNetwork) &&
-                ((((InetNetwork)obj).network.equals(network)) && (((InetNetwork)obj).netmask.equals(netmask)));
+        return  (obj != null) &&
+                (obj instanceof InetNetwork) &&
+                ((InetNetwork)obj).network.equals(network) &&
+                ((InetNetwork)obj).netmask.equals(netmask);
     }
 
     public static InetNetwork getFromString(String netspec) throws java.net.UnknownHostException
@@ -81,7 +86,8 @@ class InetNetwork
             }
         }
 
-        return new InetNetwork(InetAddress.getByName(netspec.substring(0, netspec.indexOf('/'))),
+        return new InetNetwork(
+                InetAddress.getByName(netspec.substring(0, netspec.indexOf('/'))),
                 InetAddress.getByName(netspec.substring(netspec.indexOf('/') + 1)));
     }
 
@@ -89,15 +95,17 @@ class InetNetwork
     {
         try
         {
-            return getByAddress(new byte[]
-            {
-                (byte) (mask[0] & ip[0]),
-                (byte) (mask[1] & ip[1]),
-                (byte) (mask[2] & ip[2]),
-                (byte) (mask[3] & ip[3])
-            });
+            return getByAddress(
+                new byte[]
+                {
+                    (byte) (mask[0] & ip[0]),
+                    (byte) (mask[1] & ip[1]),
+                    (byte) (mask[2] & ip[2]),
+                    (byte) (mask[3] & ip[3])
+                }
+            );
         }
-        catch(Exception _) {}
+        catch (Exception _)
         {
             return null;
         }
@@ -150,34 +158,9 @@ class InetNetwork
                 Integer.toString(mask >>  0 & 0xFF, 10);
     }
 
-    private static java.lang.reflect.Method getByAddress = null;
-
-    static {
-        try {
-            Class<?> inetAddressClass = Class.forName("java.net.InetAddress");
-            Class<?>[] parameterTypes = { byte[].class };
-            getByAddress = inetAddressClass.getMethod("getByAddress", parameterTypes);
-        } catch (Exception e) {
-            getByAddress = null;
-        }
-    }
-
     private static InetAddress getByAddress(byte[] ip) throws java.net.UnknownHostException
     {
-        InetAddress addr = null;
-        if (getByAddress != null)
-        {
-            try
-            {
-                addr = (InetAddress) getByAddress.invoke(null, new Object[] { ip });
-            }
-            catch (IllegalAccessException e)
-            {
-            }
-            catch (java.lang.reflect.InvocationTargetException e)
-            {
-            }
-        }
+        InetAddress addr = InetAddress.getByAddress(ip);
 
         if (addr == null) {
             addr = InetAddress.getByName
@@ -188,6 +171,7 @@ class InetNetwork
                     Integer.toString(ip[3] & 0xFF, 10)
                    );
         }
+
         return addr;
     }
 }
\ No newline at end of file
diff --git a/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/config/ActionTest.java b/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/config/ActionTest.java
new file mode 100644
index 0000000000..00e06106bf
--- /dev/null
+++ b/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/config/ActionTest.java
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.qpid.server.security.access.config;
+
+import static org.mockito.Mockito.*;
+
+import org.apache.qpid.server.security.access.ObjectProperties;
+import org.apache.qpid.server.security.access.ObjectType;
+import org.apache.qpid.server.security.access.Operation;
+
+import junit.framework.TestCase;
+
+public class ActionTest extends TestCase
+{
+    private ObjectProperties _properties1 = mock(ObjectProperties.class);
+    private ObjectProperties _properties2 = mock(ObjectProperties.class);
+
+    public void testMatchesReturnsTrueForMatchingActions()
+    {
+        when(_properties1.matches(_properties2)).thenReturn(true);
+
+        assertMatches(
+                new Action(Operation.CONSUME, ObjectType.QUEUE, _properties1),
+                new Action(Operation.CONSUME, ObjectType.QUEUE, _properties2));
+    }
+
+    public void testMatchesReturnsFalseWhenOperationsDiffer()
+    {
+        assertDoesntMatch(
+                new Action(Operation.CONSUME, ObjectType.QUEUE, _properties1),
+                new Action(Operation.CREATE, ObjectType.QUEUE, _properties1));
+    }
+
+    public void testMatchesReturnsFalseWhenOperationTypesDiffer()
+    {
+        assertDoesntMatch(
+                new Action(Operation.CREATE, ObjectType.QUEUE, _properties1),
+                new Action(Operation.CREATE, ObjectType.EXCHANGE, _properties1));
+    }
+
+    public void testMatchesReturnsFalseWhenOperationPropertiesDiffer()
+    {
+        assertDoesntMatch(
+                new Action(Operation.CREATE, ObjectType.QUEUE, _properties1),
+                new Action(Operation.CREATE, ObjectType.QUEUE, _properties2));
+    }
+
+    public void testMatchesReturnsFalseWhenMyOperationPropertiesIsNull()
+    {
+        assertDoesntMatch(
+                new Action(Operation.CREATE, ObjectType.QUEUE, (ObjectProperties)null),
+                new Action(Operation.CREATE, ObjectType.QUEUE, _properties1));
+    }
+
+    public void testMatchesReturnsFalseWhenOtherOperationPropertiesIsNull()
+    {
+        assertDoesntMatch(
+                new Action(Operation.CREATE, ObjectType.QUEUE, _properties1),
+                new Action(Operation.CREATE, ObjectType.QUEUE, (ObjectProperties)null));
+    }
+
+    public void testMatchesReturnsTrueWhenBothOperationPropertiesAreNull()
+    {
+        assertMatches(
+                new Action(Operation.CREATE, ObjectType.QUEUE, (ObjectProperties)null),
+                new Action(Operation.CREATE, ObjectType.QUEUE, (ObjectProperties)null));
+    }
+
+    private void assertMatches(Action action1, Action action2)
+    {
+        assertTrue(action1 + " should match " + action2, action1.matches(action2));
+    }
+
+    private void assertDoesntMatch(Action action1, Action action2)
+    {
+        assertFalse(action1 + " should not match " + action2, action1.matches(action2));
+    }
+
+}
diff --git a/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/PlainConfigurationTest.java b/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/config/PlainConfigurationTest.java
index fa9e96ba1e..21d8ff4400 100644
--- a/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/PlainConfigurationTest.java
+++ b/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/config/PlainConfigurationTest.java
@@ -16,7 +16,7 @@
  *  specific language governing permissions and limitations
  *  under the License.
  */
-package org.apache.qpid.server.security.access.plugins;
+package org.apache.qpid.server.security.access.config;
 
 import java.io.File;
 import java.io.FileNotFoundException;