SEC-1141: ObjectIdentityImpl has incorrect hashCode implementation. Modified equals method to compare longValue of Number identifier types and use standard equals for other serializable identifiers.

acl/src/main/java/org/springframework/security/acls/objectidentity/ObjectIdentityImpl.java

@@ -17,7 +17,6 @@ package org.springframework.security.acls.objectidentity;
 import java.io.Serializable;
 import java.lang.reflect.Method;
 
-import org.springframework.security.acls.jdbc.LookupStrategy;
 import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
 
@@ -92,35 +91,38 @@ public class ObjectIdentityImpl implements ObjectIdentity {
     //~ Methods ========================================================================================================
 
     /**
-     * Important so caching operates properly.<P>Considers an object of the same class equal if it has the same
-     * <code>classname</code> and <code>id</code> properties.</p>
-     *
+     * Important so caching operates properly.
+     * <p>
+     * Considers an object of the same class equal if it has the same <code>classname</code> and
+     * <code>id</code> properties.
      * <p>
-     * Note that this class uses string equality for the identifier field, which ensures it better supports
-     * differences between {@link LookupStrategy} requirements and the domain object represented by this
-     * <code>ObjectIdentityImpl</code>.
-     * </p>
+     * Numeric identities (Integer and Long values) are considered equal if they are numerically equal. Other
+     * serializable types are evaluated using a simple equality.
      *
      * @param arg0 object to compare
      *
      * @return <code>true</code> if the presented object matches this object
      */
     public boolean equals(Object arg0) {
-        if (arg0 == null) {
-            return false;
-        }
-
-        if (!(arg0 instanceof ObjectIdentityImpl)) {
+        if (arg0 == null || !(arg0 instanceof ObjectIdentityImpl)) {
             return false;
         }
 
         ObjectIdentityImpl other = (ObjectIdentityImpl) arg0;
 
-        if (this.getIdentifier().toString().equals(other.getIdentifier().toString()) && this.getJavaType().equals(other.getJavaType())) {
-            return true;
+        if (identifier instanceof Number && other.identifier instanceof Number) {
+            // Integers and Longs with same value should be considered equal
+            if (((Number)identifier).longValue() != ((Number)other.identifier).longValue()) {
+                return false;
+            }
+        } else {
+            // Use plain equality for other serializable types
+            if (!identifier.equals(other.identifier)) {
+                return false;
+            }
         }
 
-        return false;
+        return javaType.equals(other.javaType);
     }
 
     public Serializable getIdentifier() {
@@ -145,7 +147,7 @@ public class ObjectIdentityImpl implements ObjectIdentity {
     }
 
     public String toString() {
-        StringBuffer sb = new StringBuffer();
+        StringBuilder sb = new StringBuilder();
         sb.append(this.getClass().getName()).append("[");
         sb.append("Java Type: ").append(this.javaType.getName());
         sb.append("; Identifier: ").append(this.identifier).append("]");

acl/src/test/java/org/springframework/security/acls/objectidentity/ObjectIdentityImplTests.java

@@ -9,10 +9,10 @@ import org.junit.Test;
  *
  * @author Andrei Stefan
  */
-public class ObjectIdentityTests {
+public class ObjectIdentityImplTests {
 
     private static final String DOMAIN_CLASS =
-        "org.springframework.security.acls.objectidentity.ObjectIdentityTests$MockIdDomainObject";
+        "org.springframework.security.acls.objectidentity.ObjectIdentityImplTests$MockIdDomainObject";
 
     //~ Methods ========================================================================================================
 
@@ -27,7 +27,7 @@ public class ObjectIdentityTests {
 
         // Check String-Serializable constructor required field
         try {
-            new ObjectIdentityImpl("", new Long(1));
+            new ObjectIdentityImpl("", Long.valueOf(1));
             fail("It should have thrown IllegalArgumentException");
         } catch (IllegalArgumentException expected) {
         }
@@ -42,7 +42,7 @@ public class ObjectIdentityTests {
 
         // The correct way of using String-Serializable constructor
         try {
-            new ObjectIdentityImpl(DOMAIN_CLASS, new Long(1));
+            new ObjectIdentityImpl(DOMAIN_CLASS, Long.valueOf(1));
         }
         catch (IllegalArgumentException notExpected) {
             fail("It shouldn't have thrown IllegalArgumentException");
@@ -54,10 +54,16 @@ public class ObjectIdentityTests {
             fail("It should have thrown IllegalArgumentException");
         }
         catch (IllegalArgumentException expected) {
-
         }
     }
 
+    @Test
+    public void gettersReturnExpectedValues() throws Exception {
+        ObjectIdentity obj = new ObjectIdentityImpl(DOMAIN_CLASS, Long.valueOf(1));
+        assertEquals(Long.valueOf(1), obj.getIdentifier());
+        assertEquals(MockIdDomainObject.class, obj.getJavaType());
+    }
+
     @Test
     public void testGetIdMethodConstraints() throws Exception {
         // Check the getId() method is present
@@ -99,53 +105,56 @@ public class ObjectIdentityTests {
 
     @Test(expected=IllegalStateException.class)
     public void testConstructorInvalidClassParameter() throws Exception {
-        new ObjectIdentityImpl("not.a.Class", new Long(1));
+        new ObjectIdentityImpl("not.a.Class", Long.valueOf(1));
     }
 
     @Test
     public void testEquals() throws Exception {
-        ObjectIdentity obj = new ObjectIdentityImpl(DOMAIN_CLASS, new Long(1));
+        ObjectIdentity obj = new ObjectIdentityImpl(DOMAIN_CLASS, Long.valueOf(1));
         MockIdDomainObject mockObj = new MockIdDomainObject();
-        mockObj.setId(new Long(1));
+        mockObj.setId(Long.valueOf(1));
 
         String string = "SOME_STRING";
         assertNotSame(obj, string);
         assertFalse(obj.equals(null));
         assertFalse(obj.equals("DIFFERENT_OBJECT_TYPE"));
-        assertFalse(obj.equals(new ObjectIdentityImpl(DOMAIN_CLASS,new Long(2))));
+        assertFalse(obj.equals(new ObjectIdentityImpl(DOMAIN_CLASS, Long.valueOf(2))));
         assertFalse(obj.equals(new ObjectIdentityImpl(
-                "org.springframework.security.acls.objectidentity.ObjectIdentityTests$MockOtherIdDomainObject",
-                new Long(1))));
-        assertEquals(new ObjectIdentityImpl(DOMAIN_CLASS,new Long(1)), obj);
+                "org.springframework.security.acls.objectidentity.ObjectIdentityImplTests$MockOtherIdDomainObject",
+                Long.valueOf(1))));
+        assertEquals(new ObjectIdentityImpl(DOMAIN_CLASS,Long.valueOf(1)), obj);
         assertEquals(obj, new ObjectIdentityImpl(mockObj));
     }
 
     @Test
-    public void testHashCode() throws Exception {
-        ObjectIdentity obj = new ObjectIdentityImpl(DOMAIN_CLASS, new Long(1));
-        assertEquals(new ObjectIdentityImpl(DOMAIN_CLASS, new Long(1)).hashCode(), obj.hashCode());
-        assertTrue(new ObjectIdentityImpl(
-                "org.springframework.security.acls.objectidentity.ObjectIdentityTests$MockOtherIdDomainObject",
-                new Long(1)).hashCode() != obj.hashCode());
-        assertTrue(new ObjectIdentityImpl(DOMAIN_CLASS, new Long(2)).hashCode() != obj.hashCode());
+    public void hashcodeIsDifferentForDifferentJavaTypes() throws Exception {
+        ObjectIdentity obj = new ObjectIdentityImpl(Object.class, Long.valueOf(1));
+        ObjectIdentity obj2 = new ObjectIdentityImpl(String.class, Long.valueOf(1));
+        assertFalse(obj.hashCode() == obj2.hashCode());
+    }
+
+    @Test
+    public void longAndIntegerIdsWithSameValueAreEqualAndHaveSameHashcode() {
+        ObjectIdentity obj = new ObjectIdentityImpl(Object.class, new Long(5));
+        ObjectIdentity obj2 = new ObjectIdentityImpl(Object.class, new Integer(5));
+
+        assertEquals(obj, obj2);
+        assertEquals(obj.hashCode(), obj2.hashCode());
     }
 
-/*    public void testHashCodeDifferentSerializableTypes() throws Exception {
-        ObjectIdentity obj = new ObjectIdentityImpl(
-                DOMAIN_CLASS, new Long(1));
-        assertEquals(new ObjectIdentityImpl(
-                DOMAIN_CLASS, "1")
-                .hashCode(), obj.hashCode());
-        assertEquals(new ObjectIdentityImpl(
-                DOMAIN_CLASS,
-                new Integer(1)).hashCode(), obj.hashCode());
-    }*/
+    @Test
+    public void equalStringIdsAreEqualAndHaveSameHashcode() throws Exception {
+        ObjectIdentity obj = new ObjectIdentityImpl(Object.class, "1000");
+        ObjectIdentity obj2 = new ObjectIdentityImpl(Object.class, "1000");
+        assertEquals(obj, obj2);
+        assertEquals(obj.hashCode(), obj2.hashCode());
+    }
 
     @Test
-    public void testGetters() throws Exception {
-        ObjectIdentity obj = new ObjectIdentityImpl(DOMAIN_CLASS, new Long(1));
-        assertEquals(new Long(1), obj.getIdentifier());
-        assertEquals(MockIdDomainObject.class, obj.getJavaType());
+    public void stringAndNumericIdsAreNotEqual() throws Exception {
+        ObjectIdentity obj = new ObjectIdentityImpl(Object.class, "1000");
+        ObjectIdentity obj2 = new ObjectIdentityImpl(Object.class, Long.valueOf(1000));
+        assertFalse(obj.equals(obj2));
     }
 
     //~ Inner Classes ==================================================================================================