Skip to content

Commit 6b81f97

Browse files
rwinchrwinch
committed
SEC-2114: Polishing Spring Based Cache
1 parent 01ea39c commit 6b81f97

File tree

6 files changed

+36
-116
lines changed

6 files changed

+36
-116
lines changed

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

+12-27
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,6 @@
1515
*/
1616
package org.springframework.security.acls.domain;
1717

18-
import net.sf.ehcache.CacheException;
19-
import net.sf.ehcache.Ehcache;
20-
import net.sf.ehcache.Element;
2118
import org.springframework.cache.Cache;
2219
import org.springframework.security.acls.model.AclCache;
2320
import org.springframework.security.acls.model.MutableAcl;
@@ -84,34 +81,12 @@ public void evictFromCache(ObjectIdentity objectIdentity) {
8481

8582
public MutableAcl getFromCache(ObjectIdentity objectIdentity) {
8683
Assert.notNull(objectIdentity, "ObjectIdentity required");
87-
88-
Cache.ValueWrapper element = null;
89-
90-
try {
91-
element = cache.get(objectIdentity);
92-
} catch (CacheException ignored) {}
93-
94-
if (element == null) {
95-
return null;
96-
}
97-
98-
return initializeTransientFields((MutableAcl)element.get());
84+
return getFromCache((Object)objectIdentity);
9985
}
10086

10187
public MutableAcl getFromCache(Serializable pk) {
10288
Assert.notNull(pk, "Primary key (identifier) required");
103-
104-
Cache.ValueWrapper element = null;
105-
106-
try {
107-
element = cache.get(pk);
108-
} catch (CacheException ignored) {}
109-
110-
if (element == null) {
111-
return null;
112-
}
113-
114-
return initializeTransientFields((MutableAcl) element.get());
89+
return getFromCache((Object)pk);
11590
}
11691

11792
public void putInCache(MutableAcl acl) {
@@ -127,6 +102,16 @@ public void putInCache(MutableAcl acl) {
127102
cache.put(acl.getId(), acl);
128103
}
129104

105+
private MutableAcl getFromCache(Object key) {
106+
Cache.ValueWrapper element = cache.get(key);
107+
108+
if (element == null) {
109+
return null;
110+
}
111+
112+
return initializeTransientFields((MutableAcl) element.get());
113+
}
114+
130115
private MutableAcl initializeTransientFields(MutableAcl value) {
131116
if (value instanceof AclImpl) {
132117
FieldUtils.setProtectedFieldValue("aclAuthorizationStrategy", value, this.aclAuthorizationStrategy);

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

+4-34
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package org.springframework.security.acls.jdbc;
22

33
import org.junit.After;
4-
import org.junit.AfterClass;
54
import org.junit.BeforeClass;
65
import org.junit.Test;
76
import org.springframework.cache.Cache;
@@ -17,15 +16,14 @@
1716
import org.springframework.security.core.context.SecurityContextHolder;
1817
import org.springframework.security.util.FieldUtils;
1918

20-
import java.io.*;
2119
import java.util.Map;
2220

2321
import static org.junit.Assert.*;
2422

2523
/**
26-
* Tests {@link org.springframework.security.acls.domain.EhCacheBasedAclCache}
24+
* Tests {@link org.springframework.security.acls.domain.SpringCacheBasedAclCache}
2725
*
28-
* @author Andrei Stefan
26+
* @author Marten Deinum
2927
*/
3028
public class SpringCacheBasedAclCacheTests {
3129
private static final String TARGET_CLASS = "org.springframework.security.acls.TargetObject";
@@ -55,6 +53,7 @@ public void constructorRejectsNullParameters() throws Exception {
5553
new SpringCacheBasedAclCache(null, null, null);
5654
}
5755

56+
@SuppressWarnings("rawtypes")
5857
@Test
5958
public void cacheOperationsAclWithoutParent() throws Exception {
6059
Cache cache = getCache();
@@ -98,7 +97,7 @@ public void cacheOperationsAclWithoutParent() throws Exception {
9897
assertEquals(realCache.size(), 0);
9998
}
10099

101-
@SuppressWarnings("unchecked")
100+
@SuppressWarnings("rawtypes")
102101
@Test
103102
public void cacheOperationsAclWithParent() throws Exception {
104103
Cache cache = getCache();
@@ -140,33 +139,4 @@ public void cacheOperationsAclWithParent() throws Exception {
140139
assertNotNull(FieldUtils.getFieldValue(parentAclFromCache, "aclAuthorizationStrategy"));
141140
assertEquals(parentAcl, myCache.getFromCache(identityParent));
142141
}
143-
144-
//~ Inner Classes ==================================================================================================
145-
146-
private class MockCache implements Cache {
147-
148-
@Override
149-
public String getName() {
150-
return "mockcache";
151-
}
152-
153-
@Override
154-
public Object getNativeCache() {
155-
return null;
156-
}
157-
158-
@Override
159-
public ValueWrapper get(Object key) {
160-
return null;
161-
}
162-
163-
@Override
164-
public void put(Object key, Object value) {}
165-
166-
@Override
167-
public void evict(Object key) {}
168-
169-
@Override
170-
public void clear() {}
171-
}
172142
}

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

+7-13
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717

1818
import org.apache.commons.logging.Log;
1919
import org.apache.commons.logging.LogFactory;
20-
import org.springframework.beans.factory.InitializingBean;
2120
import org.springframework.cache.Cache;
2221
import org.springframework.util.Assert;
2322

@@ -29,21 +28,24 @@
2928
* @since 3.2
3029
*
3130
*/
32-
public class SpringCacheBasedTicketCache implements StatelessTicketCache, InitializingBean {
31+
public class SpringCacheBasedTicketCache implements StatelessTicketCache {
3332
//~ Static fields/initializers =====================================================================================
3433

3534
private static final Log logger = LogFactory.getLog(SpringCacheBasedTicketCache.class);
3635

3736
//~ Instance fields ================================================================================================
3837

39-
private Cache cache;
38+
private final Cache cache;
4039

41-
//~ Methods ========================================================================================================
40+
//~ Constructors ===================================================================================================
4241

43-
public void afterPropertiesSet() throws Exception {
42+
public SpringCacheBasedTicketCache(Cache cache) throws Exception {
4443
Assert.notNull(cache, "cache mandatory");
44+
this.cache = cache;
4545
}
4646

47+
//~ Methods ========================================================================================================
48+
4749
public CasAuthenticationToken getByTicketId(final String serviceTicket) {
4850
final Cache.ValueWrapper element = serviceTicket != null ? cache.get(serviceTicket) : null;
4951

@@ -54,10 +56,6 @@ public CasAuthenticationToken getByTicketId(final String serviceTicket) {
5456
return element == null ? null : (CasAuthenticationToken) element.get();
5557
}
5658

57-
public Cache getCache() {
58-
return cache;
59-
}
60-
6159
public void putTicketInCache(final CasAuthenticationToken token) {
6260
String key = token.getCredentials().toString();
6361

@@ -79,8 +77,4 @@ public void removeTicketFromCache(final CasAuthenticationToken token) {
7977
public void removeTicketFromCache(final String serviceTicket) {
8078
cache.evict(serviceTicket);
8179
}
82-
83-
public void setCache(final Cache cache) {
84-
this.cache = cache;
85-
}
8680
}

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

+4-18
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,8 @@
1515

1616
package org.springframework.security.cas.authentication;
1717

18-
import org.junit.AfterClass;
1918
import org.junit.BeforeClass;
2019
import org.junit.Test;
21-
import org.springframework.cache.Cache;
2220
import org.springframework.cache.CacheManager;
2321
import org.springframework.cache.concurrent.ConcurrentMapCacheManager;
2422

@@ -35,6 +33,7 @@ public class SpringCacheBasedTicketCacheTests extends AbstractStatelessTicketCac
3533
private static CacheManager cacheManager;
3634

3735
//~ Methods ========================================================================================================
36+
3837
@BeforeClass
3938
public static void initCacheManaer() {
4039
cacheManager = new ConcurrentMapCacheManager();
@@ -43,9 +42,7 @@ public static void initCacheManaer() {
4342

4443
@Test
4544
public void testCacheOperation() throws Exception {
46-
SpringCacheBasedTicketCache cache = new SpringCacheBasedTicketCache();
47-
cache.setCache(cacheManager.getCache("castickets"));
48-
cache.afterPropertiesSet();
45+
SpringCacheBasedTicketCache cache = new SpringCacheBasedTicketCache(cacheManager.getCache("castickets"));
4946

5047
final CasAuthenticationToken token = getToken();
5148

@@ -62,19 +59,8 @@ public void testCacheOperation() throws Exception {
6259
assertNull(cache.getByTicketId("UNKNOWN_SERVICE_TICKET"));
6360
}
6461

65-
@Test
62+
@Test(expected = IllegalArgumentException.class)
6663
public void testStartupDetectsMissingCache() throws Exception {
67-
SpringCacheBasedTicketCache cache = new SpringCacheBasedTicketCache();
68-
69-
try {
70-
cache.afterPropertiesSet();
71-
fail("Should have thrown IllegalArgumentException");
72-
} catch (IllegalArgumentException expected) {
73-
assertTrue(true);
74-
}
75-
76-
Cache myCache = cacheManager.getCache("castickets");
77-
cache.setCache(myCache);
78-
assertEquals(myCache, cache.getCache());
64+
new SpringCacheBasedTicketCache(null);
7965
}
8066
}

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

+7-13
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,18 @@
22

33
import org.apache.commons.logging.Log;
44
import org.apache.commons.logging.LogFactory;
5-
import org.springframework.beans.factory.InitializingBean;
65
import org.springframework.cache.Cache;
76
import org.springframework.security.core.userdetails.UserCache;
87
import org.springframework.security.core.userdetails.UserDetails;
98
import org.springframework.util.Assert;
109

1110
/**
12-
* Caches {@link UserDetails} intances in a Spring defined {@link Cache}.
11+
* Caches {@link UserDetails} instances in a Spring defined {@link Cache}.
1312
*
1413
* @author Marten Deinum
1514
* @since 3.2
1615
*/
17-
public class SpringCacheBasedUserCache implements UserCache, InitializingBean {
16+
public class SpringCacheBasedUserCache implements UserCache {
1817

1918

2019
//~ Static fields/initializers =====================================================================================
@@ -23,17 +22,16 @@ public class SpringCacheBasedUserCache implements UserCache, InitializingBean {
2322

2423
//~ Instance fields ================================================================================================
2524

26-
private Cache cache;
25+
private final Cache cache;
2726

28-
//~ Methods ========================================================================================================
27+
//~ Constructors ===================================================================================================
2928

30-
public void afterPropertiesSet() throws Exception {
29+
public SpringCacheBasedUserCache(Cache cache) throws Exception {
3130
Assert.notNull(cache, "cache mandatory");
31+
this.cache = cache;
3232
}
3333

34-
public Cache getCache() {
35-
return cache;
36-
}
34+
//~ Methods ========================================================================================================
3735

3836
public UserDetails getUserFromCache(String username) {
3937
Cache.ValueWrapper element = username != null ? cache.get(username) : null;
@@ -67,8 +65,4 @@ public void removeUserFromCache(UserDetails user) {
6765
public void removeUserFromCache(String username) {
6866
cache.evict(username);
6967
}
70-
71-
public void setCache(Cache cache) {
72-
this.cache = cache;
73-
}
7468
}

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

+2-11
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,7 @@ private User getUser() {
6161

6262
@Test
6363
public void cacheOperationsAreSuccessful() throws Exception {
64-
SpringCacheBasedUserCache cache = new SpringCacheBasedUserCache();
65-
cache.setCache(getCache());
66-
cache.afterPropertiesSet();
64+
SpringCacheBasedUserCache cache = new SpringCacheBasedUserCache(getCache());
6765

6866
// Check it gets stored in the cache
6967
cache.putUserInCache(getUser());
@@ -80,13 +78,6 @@ public void cacheOperationsAreSuccessful() throws Exception {
8078

8179
@Test(expected = IllegalArgumentException.class)
8280
public void startupDetectsMissingCache() throws Exception {
83-
SpringCacheBasedUserCache cache = new SpringCacheBasedUserCache();
84-
85-
cache.afterPropertiesSet();
86-
fail("Should have thrown IllegalArgumentException");
87-
88-
Cache myCache = getCache();
89-
cache.setCache(myCache);
90-
assertEquals(myCache, cache.getCache());
81+
new SpringCacheBasedUserCache(null);
9182
}
9283
}

0 commit comments

Comments
 (0)