SEC-2114: Polishing Spring Based Cache

acl/src/main/java/org/springframework/security/acls/domain/SpringCacheBasedAclCache.java

@@ -15,9 +15,6 @@
  */
 package org.springframework.security.acls.domain;
 
-import net.sf.ehcache.CacheException;
-import net.sf.ehcache.Ehcache;
-import net.sf.ehcache.Element;
 import org.springframework.cache.Cache;
 import org.springframework.security.acls.model.AclCache;
 import org.springframework.security.acls.model.MutableAcl;
@@ -84,34 +81,12 @@ public class SpringCacheBasedAclCache implements AclCache {
 
     public MutableAcl getFromCache(ObjectIdentity objectIdentity) {
         Assert.notNull(objectIdentity, "ObjectIdentity required");
-
+        return getFromCache((Object)objectIdentity);
-        Cache.ValueWrapper element = null;
-
-        try {
-            element = cache.get(objectIdentity);
-        } catch (CacheException ignored) {}
-
-        if (element == null) {
-            return null;
-        }
-
-        return initializeTransientFields((MutableAcl)element.get());
     }
 
     public MutableAcl getFromCache(Serializable pk) {
         Assert.notNull(pk, "Primary key (identifier) required");
-
+        return getFromCache((Object)pk);
-        Cache.ValueWrapper element = null;
-
-        try {
-            element = cache.get(pk);
-        } catch (CacheException ignored) {}
-
-        if (element == null) {
-            return null;
-        }
-
-        return initializeTransientFields((MutableAcl) element.get());
     }
 
     public void putInCache(MutableAcl acl) {
@@ -127,6 +102,16 @@ public class SpringCacheBasedAclCache implements AclCache {
         cache.put(acl.getId(), acl);
     }
 
+    private MutableAcl getFromCache(Object key) {
+        Cache.ValueWrapper element = cache.get(key);
+
+        if (element == null) {
+            return null;
+        }
+
+        return initializeTransientFields((MutableAcl) element.get());
+    }
+
     private MutableAcl initializeTransientFields(MutableAcl value) {
         if (value instanceof AclImpl) {
             FieldUtils.setProtectedFieldValue("aclAuthorizationStrategy", value, this.aclAuthorizationStrategy);

acl/src/test/java/org/springframework/security/acls/jdbc/SpringCacheBasedAclCacheTests.java

@@ -1,7 +1,6 @@
 package org.springframework.security.acls.jdbc;
 
 import org.junit.After;
-import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.springframework.cache.Cache;
@@ -17,15 +16,14 @@ import org.springframework.security.core.authority.SimpleGrantedAuthority;
 import org.springframework.security.core.context.SecurityContextHolder;
 import org.springframework.security.util.FieldUtils;
 
-import java.io.*;
 import java.util.Map;
 
 import static org.junit.Assert.*;
 
 /**
- * Tests {@link org.springframework.security.acls.domain.EhCacheBasedAclCache}
+ * Tests {@link org.springframework.security.acls.domain.SpringCacheBasedAclCache}
  *
- * @author Andrei Stefan
+ * @author Marten Deinum
  */
 public class SpringCacheBasedAclCacheTests {
     private static final String TARGET_CLASS = "org.springframework.security.acls.TargetObject";
@@ -55,6 +53,7 @@ public class SpringCacheBasedAclCacheTests {
         new SpringCacheBasedAclCache(null, null, null);
     }
 
+    @SuppressWarnings("rawtypes")
     @Test
     public void cacheOperationsAclWithoutParent() throws Exception {
         Cache cache = getCache();
@@ -98,7 +97,7 @@ public class SpringCacheBasedAclCacheTests {
         assertEquals(realCache.size(), 0);
     }
 
-    @SuppressWarnings("unchecked")
+    @SuppressWarnings("rawtypes")
     @Test
     public void cacheOperationsAclWithParent() throws Exception {
         Cache cache = getCache();
@@ -140,33 +139,4 @@ public class SpringCacheBasedAclCacheTests {
         assertNotNull(FieldUtils.getFieldValue(parentAclFromCache, "aclAuthorizationStrategy"));
         assertEquals(parentAcl, myCache.getFromCache(identityParent));
     }
-
-    //~ Inner Classes ==================================================================================================
-
-    private class MockCache implements Cache {
-
-        @Override
-        public String getName() {
-            return "mockcache";
-        }
-
-        @Override
-        public Object getNativeCache() {
-            return null;
-        }
-
-        @Override
-        public ValueWrapper get(Object key) {
-            return null;
-        }
-
-        @Override
-        public void put(Object key, Object value) {}
-
-        @Override
-        public void evict(Object key) {}
-
-        @Override
-        public void clear() {}
-    }
 }

cas/src/main/java/org/springframework/security/cas/authentication/SpringCacheBasedTicketCache.java

@@ -17,7 +17,6 @@ package org.springframework.security.cas.authentication;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-import org.springframework.beans.factory.InitializingBean;
 import org.springframework.cache.Cache;
 import org.springframework.util.Assert;
 
@@ -29,21 +28,24 @@ import org.springframework.util.Assert;
  * @since 3.2
  *
  */
-public class SpringCacheBasedTicketCache implements StatelessTicketCache, InitializingBean {
+public class SpringCacheBasedTicketCache implements StatelessTicketCache {
     //~ Static fields/initializers =====================================================================================
 
     private static final Log logger = LogFactory.getLog(SpringCacheBasedTicketCache.class);
 
     //~ Instance fields ================================================================================================
 
-    private Cache cache;
+    private final Cache cache;
 
-    //~ Methods ========================================================================================================
+    //~ Constructors ===================================================================================================
 
-    public void afterPropertiesSet() throws Exception {
+    public SpringCacheBasedTicketCache(Cache cache) throws Exception {
         Assert.notNull(cache, "cache mandatory");
+        this.cache = cache;
     }
 
+    //~ Methods ========================================================================================================
+
     public CasAuthenticationToken getByTicketId(final String serviceTicket) {
         final Cache.ValueWrapper element = serviceTicket != null ? cache.get(serviceTicket) : null;
 
@@ -54,10 +56,6 @@ public class SpringCacheBasedTicketCache implements StatelessTicketCache, Initia
         return element == null ? null : (CasAuthenticationToken) element.get();
     }
 
-    public Cache getCache() {
-        return cache;
-    }
-
     public void putTicketInCache(final CasAuthenticationToken token) {
         String key = token.getCredentials().toString();
 
@@ -79,8 +77,4 @@ public class SpringCacheBasedTicketCache implements StatelessTicketCache, Initia
     public void removeTicketFromCache(final String serviceTicket) {
         cache.evict(serviceTicket);
     }
-
-    public void setCache(final Cache cache) {
-        this.cache = cache;
-    }
 }

cas/src/test/java/org/springframework/security/cas/authentication/SpringCacheBasedTicketCacheTests.java

@@ -15,10 +15,8 @@
 
 package org.springframework.security.cas.authentication;
 
-import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Test;
-import org.springframework.cache.Cache;
 import org.springframework.cache.CacheManager;
 import org.springframework.cache.concurrent.ConcurrentMapCacheManager;
 
@@ -35,6 +33,7 @@ public class SpringCacheBasedTicketCacheTests extends AbstractStatelessTicketCac
     private static CacheManager cacheManager;
 
     //~ Methods ========================================================================================================
+
     @BeforeClass
     public static void initCacheManaer() {
         cacheManager = new ConcurrentMapCacheManager();
@@ -43,9 +42,7 @@ public class SpringCacheBasedTicketCacheTests extends AbstractStatelessTicketCac
 
     @Test
     public void testCacheOperation() throws Exception {
-        SpringCacheBasedTicketCache cache = new SpringCacheBasedTicketCache();
+        SpringCacheBasedTicketCache cache = new SpringCacheBasedTicketCache(cacheManager.getCache("castickets"));
-        cache.setCache(cacheManager.getCache("castickets"));
-        cache.afterPropertiesSet();
 
         final CasAuthenticationToken token = getToken();
 
@@ -62,19 +59,8 @@ public class SpringCacheBasedTicketCacheTests extends AbstractStatelessTicketCac
         assertNull(cache.getByTicketId("UNKNOWN_SERVICE_TICKET"));
     }
 
-    @Test
+    @Test(expected = IllegalArgumentException.class)
     public void testStartupDetectsMissingCache() throws Exception {
-        SpringCacheBasedTicketCache cache = new SpringCacheBasedTicketCache();
+        new SpringCacheBasedTicketCache(null);
-
-        try {
-            cache.afterPropertiesSet();
-            fail("Should have thrown IllegalArgumentException");
-        } catch (IllegalArgumentException expected) {
-            assertTrue(true);
-        }
-
-        Cache myCache = cacheManager.getCache("castickets");
-        cache.setCache(myCache);
-        assertEquals(myCache, cache.getCache());
     }
 }

core/src/main/java/org/springframework/security/core/userdetails/cache/SpringCacheBasedUserCache.java

@@ -2,19 +2,18 @@ package org.springframework.security.core.userdetails.cache;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-import org.springframework.beans.factory.InitializingBean;
 import org.springframework.cache.Cache;
 import org.springframework.security.core.userdetails.UserCache;
 import org.springframework.security.core.userdetails.UserDetails;
 import org.springframework.util.Assert;
 
 /**
- * Caches {@link UserDetails} intances in a Spring defined {@link Cache}.
+ * Caches {@link UserDetails} instances in a Spring defined {@link Cache}.
  *
  * @author Marten Deinum
  * @since 3.2
  */
-public class SpringCacheBasedUserCache implements UserCache, InitializingBean {
+public class SpringCacheBasedUserCache implements UserCache {
 
 
     //~ Static fields/initializers =====================================================================================
@@ -23,17 +22,16 @@ public class SpringCacheBasedUserCache implements UserCache, InitializingBean {
 
     //~ Instance fields ================================================================================================
 
-    private Cache cache;
+    private final Cache cache;
 
-    //~ Methods ========================================================================================================
+    //~ Constructors ===================================================================================================
 
-    public void afterPropertiesSet() throws Exception {
+    public SpringCacheBasedUserCache(Cache cache) throws Exception {
         Assert.notNull(cache, "cache mandatory");
+        this.cache = cache;
     }
 
-    public Cache getCache() {
+    //~ Methods ========================================================================================================
-        return cache;
-    }
 
     public UserDetails getUserFromCache(String username) {
         Cache.ValueWrapper element = username != null ? cache.get(username) : null;
@@ -67,8 +65,4 @@ public class SpringCacheBasedUserCache implements UserCache, InitializingBean {
     public void removeUserFromCache(String username) {
         cache.evict(username);
     }
-
-    public void setCache(Cache cache) {
-        this.cache = cache;
-    }
 }

core/src/test/java/org/springframework/security/core/userdetails/cache/SpringCacheBasedUserCacheTests.java

@@ -61,9 +61,7 @@ public class SpringCacheBasedUserCacheTests {
 
     @Test
     public void cacheOperationsAreSuccessful() throws Exception {
-        SpringCacheBasedUserCache cache = new SpringCacheBasedUserCache();
+        SpringCacheBasedUserCache cache = new SpringCacheBasedUserCache(getCache());
-        cache.setCache(getCache());
-        cache.afterPropertiesSet();
 
         // Check it gets stored in the cache
         cache.putUserInCache(getUser());
@@ -80,13 +78,6 @@ public class SpringCacheBasedUserCacheTests {
 
     @Test(expected = IllegalArgumentException.class)
     public void startupDetectsMissingCache() throws Exception {
-        SpringCacheBasedUserCache cache = new SpringCacheBasedUserCache();
+        new SpringCacheBasedUserCache(null);
-
-        cache.afterPropertiesSet();
-        fail("Should have thrown IllegalArgumentException");
-
-        Cache myCache = getCache();
-        cache.setCache(myCache);
-        assertEquals(myCache, cache.getCache());
     }
 }