From ee2b71909846af8dd46e8b903803b59ff834a6eb Mon Sep 17 00:00:00 2001 From: "Marvin S. Addison" Date: Thu, 26 Jul 2012 10:25:06 -0400 Subject: [PATCH] CASC-166 Address code review feedback. Use ConcurrentHashMap to avoid explicit synchronization. Use TimeUnit to allow more user-friendly configuration of the units of the cache timeout (e.g. MINUTES, SECONDS) and rename option from cacheTimeoutUnits to cacheTimeoutUnit for consistency. --- .../jasig/cas/client/jaas/CasLoginModule.java | 108 ++++++++---------- .../cas/client/jaas/CasLoginModuleTests.java | 4 +- 2 files changed, 49 insertions(+), 63 deletions(-) diff --git a/cas-client-core/src/main/java/org/jasig/cas/client/jaas/CasLoginModule.java b/cas-client-core/src/main/java/org/jasig/cas/client/jaas/CasLoginModule.java index 565d0ec..02454e8 100644 --- a/cas-client-core/src/main/java/org/jasig/cas/client/jaas/CasLoginModule.java +++ b/cas-client-core/src/main/java/org/jasig/cas/client/jaas/CasLoginModule.java @@ -34,6 +34,7 @@ import java.util.HashSet; import java.util.Iterator; import java.util.Map; import java.util.Set; +import java.util.concurrent.TimeUnit; import javax.security.auth.Subject; import javax.security.auth.callback.Callback; @@ -84,10 +85,8 @@ import org.jasig.cas.client.validation.TicketValidator; * for JAAS providers that attempt to periodically reauthenticate to renew principal. * Since CAS tickets are one-time-use, a cached assertion must be provided on reauthentication. *
  • cacheTimeout (optional) - Assertion cache timeout in minutes.
    • - *
  • cacheTimeoutUnits (optional) - Assertion cache timeout units. - * MUST be one of following: - * Calendar.HOUR(10), Calendar.MINUTE(12), Calendar.SECOND(13), Calendar.MILLISECOND(14). - * Default unit is minutes.
    • + *
  • cacheTimeoutUnit (optional) - Assertion cache timeout unit. Must be one of {@link TimeUnit} enumeration + * names, e.g. DAYS, HOURS, MINUTES, SECONDS, MILLISECONDS. Default unit is MINUTES.
    • * * *

    @@ -139,7 +138,7 @@ public class CasLoginModule implements LoginModule { public static final int DEFAULT_CACHE_TIMEOUT = 480; /** Default assertion cache timeout unit is minutes. */ - public static final int DEFAULT_CACHE_TIMEOUT_UNITS = Calendar.MINUTE; + public static final TimeUnit DEFAULT_CACHE_TIMEOUT_UNIT = TimeUnit.MINUTES; /** * Stores mapping of ticket to assertion to support JAAS providers that @@ -191,16 +190,8 @@ public class CasLoginModule implements LoginModule { /** Assertion cache timeout in minutes */ protected int cacheTimeout = DEFAULT_CACHE_TIMEOUT; - /** - * Units of cache timeout. Must be one of following {@link Calendar} time unit constants: - *

    - */ - protected int cacheTimeoutUnits = DEFAULT_CACHE_TIMEOUT_UNITS; + /** Units of cache timeout. */ + protected TimeUnit cacheTimeoutUnit = DEFAULT_CACHE_TIMEOUT_UNIT; /** * Initializes the CAS login module. @@ -222,17 +213,20 @@ public class CasLoginModule implements LoginModule { * which by default are single use, reauthentication fails. Assertion caching addresses this * behavior. *
  • cacheTimeout (optional) - assertion cache timeout in minutes.
    • - *
  • cacheTimeoutUnits (optional) - Assertion cache timeout units. - * MUST be one of following: - * Calendar.HOUR(10), Calendar.MINUTE(12), Calendar.SECOND(13), Calendar.MILLISECOND(14). - * Default unit is minutes.
    • + *
  • cacheTimeoutUnit (optional) - Assertion cache timeout unit. Must be one of {@link TimeUnit} enumeration + * names, e.g. DAYS, HOURS, MINUTES, SECONDS, MILLISECONDS. Default unit is MINUTES.
    • * */ - public final void initialize(final Subject subject, final CallbackHandler handler, final Map state, final Map options) { + public final void initialize( + final Subject subject, + final CallbackHandler handler, + final Map state, + final Map options) { + this.assertion = null; this.callbackHandler = handler; this.subject = subject; - this.sharedState = (Map) state; + this.sharedState = (Map) state; this.sharedState = new HashMap(state); String ticketValidatorClass = null; @@ -268,15 +262,9 @@ public class CasLoginModule implements LoginModule { } else if ("cacheTimeout".equals(key)) { this.cacheTimeout = Integer.parseInt((String) options.get(key)); log.debug("Set cacheTimeout=" + this.cacheTimeout); - } else if ("cacheTimeoutUnits".equals(key)) { - final int units = Integer.parseInt((String) options.get(key)); - if (units == Calendar.HOUR || units == Calendar.MINUTE || - units == Calendar.SECOND || units == Calendar.MILLISECOND) { - this.cacheTimeoutUnits = units; - log.debug("Set cacheTimeoutUnits=" + this.cacheTimeoutUnits); - } else { - throw new IllegalArgumentException("Invalid time unit constant " + units); - } + } else if ("cacheTimeoutUnit".equals(key)) { + this.cacheTimeoutUnit = Enum.valueOf(TimeUnit.class, (String) options.get(key)); + log.debug("Set cacheTimeoutUnit=" + this.cacheTimeoutUnit); } } @@ -325,20 +313,19 @@ public class CasLoginModule implements LoginModule { throw (LoginException) new LoginException("IO exception in callback handler: " + e).initCause(e); } catch (final UnsupportedCallbackException e) { log.info("Login failed due to unsupported callback: " + e); - throw (LoginException) new LoginException("Callback handler does not support PasswordCallback and TextInputCallback.").initCause(e); + throw (LoginException) new LoginException( + "Callback handler does not support PasswordCallback and TextInputCallback.").initCause(e); } if (ticketCallback.getPassword() != null) { this.ticket = new TicketCredential(new String(ticketCallback.getPassword())); - final String service = CommonUtils.isNotBlank(serviceCallback.getName()) ? serviceCallback.getName() : this.service; + final String service = CommonUtils.isNotBlank( + serviceCallback.getName()) ? serviceCallback.getName() : this.service; if (this.cacheAssertions) { - // Multiple threads may be accessing the static cache concurrently - synchronized (ASSERTION_CACHE) { - if (ASSERTION_CACHE.get(ticket) != null) { - log.debug("Assertion found in cache."); - this.assertion = ASSERTION_CACHE.get(ticket); - } + this.assertion = ASSERTION_CACHE.get(ticket); + if (this.assertion != null) { + log.debug("Assertion found in cache."); } } @@ -346,7 +333,8 @@ public class CasLoginModule implements LoginModule { log.debug("CAS assertion is null; ticket validation required."); if (CommonUtils.isBlank(service)) { log.info("Login failed because required CAS service parameter not provided."); - throw new LoginException("Neither login module nor callback handler provided required service parameter."); + throw new LoginException( + "Neither login module nor callback handler provided required service parameter."); } try { if (log.isDebugEnabled()) { @@ -413,7 +401,8 @@ public class CasLoginModule implements LoginModule { throw new LoginException("Ticket credential not found."); } - final AssertionPrincipal casPrincipal = new AssertionPrincipal(this.assertion.getPrincipal().getName(), this.assertion); + final AssertionPrincipal casPrincipal = new AssertionPrincipal( + this.assertion.getPrincipal().getName(), this.assertion); this.subject.getPrincipals().add(casPrincipal); // Add group containing principal as sole member @@ -458,10 +447,7 @@ public class CasLoginModule implements LoginModule { if (log.isDebugEnabled()) { log.debug("Caching assertion for principal " + this.assertion.getPrincipal()); } - // Multiple threads may be accessing the static cache concurrently - synchronized (ASSERTION_CACHE) { - ASSERTION_CACHE.put(this.ticket, this.assertion); - } + ASSERTION_CACHE.put(this.ticket, this.assertion); } } else { // Login must have failed if there is no assertion defined @@ -522,10 +508,12 @@ public class CasLoginModule implements LoginModule { * @return Ticket validator with properties set. */ private TicketValidator createTicketValidator(final String className, final Map propertyMap) { - CommonUtils.assertTrue(propertyMap.containsKey("casServerUrlPrefix"), "Required property casServerUrlPrefix not found."); + CommonUtils.assertTrue( + propertyMap.containsKey("casServerUrlPrefix"), "Required property casServerUrlPrefix not found."); final Class validatorClass = ReflectUtils.loadClass(className); - final TicketValidator validator = ReflectUtils.newInstance(validatorClass, propertyMap.get("casServerUrlPrefix")); + final TicketValidator validator = ReflectUtils.newInstance( + validatorClass, propertyMap.get("casServerUrlPrefix")); try { final BeanInfo info = Introspector.getBeanInfo(validatorClass); @@ -570,7 +558,8 @@ public class CasLoginModule implements LoginModule { } else if (long.class.equals(pd.getPropertyType())) { return new Long(value); } else { - throw new IllegalArgumentException("No conversion strategy exists for property " + pd.getName() + " of type " + pd.getPropertyType()); + throw new IllegalArgumentException( + "No conversion strategy exists for property " + pd.getName() + " of type " + pd.getPropertyType()); } } @@ -598,21 +587,18 @@ public class CasLoginModule implements LoginModule { if (log.isDebugEnabled()) { log.debug("Cleaning assertion cache of size " + ASSERTION_CACHE.size()); } - // Multiple threads may be accessing the static cache concurrently - synchronized (ASSERTION_CACHE) { - final Iterator> iter = ASSERTION_CACHE.entrySet().iterator(); - final Calendar cutoff = Calendar.getInstance(); - cutoff.add(this.cacheTimeoutUnits, -this.cacheTimeout); - while (iter.hasNext()) { - final Assertion assertion = iter.next().getValue(); - final Calendar created = Calendar.getInstance(); - created.setTime(assertion.getValidFromDate()); - if (created.before(cutoff)) { - if (log.isDebugEnabled()) { - log.debug("Removing expired assertion for principal " + assertion.getPrincipal()); - } - iter.remove(); + final Iterator> iter = ASSERTION_CACHE.entrySet().iterator(); + final Calendar cutoff = Calendar.getInstance(); + cutoff.setTimeInMillis(System.currentTimeMillis() - this.cacheTimeoutUnit.toMillis(this.cacheTimeout)); + while (iter.hasNext()) { + final Assertion assertion = iter.next().getValue(); + final Calendar created = Calendar.getInstance(); + created.setTime(assertion.getValidFromDate()); + if (created.before(cutoff)) { + if (log.isDebugEnabled()) { + log.debug("Removing expired assertion for principal " + assertion.getPrincipal()); } + iter.remove(); } } } diff --git a/cas-client-core/src/test/java/org/jasig/cas/client/jaas/CasLoginModuleTests.java b/cas-client-core/src/test/java/org/jasig/cas/client/jaas/CasLoginModuleTests.java index 1f84d23..ede32b0 100644 --- a/cas-client-core/src/test/java/org/jasig/cas/client/jaas/CasLoginModuleTests.java +++ b/cas-client-core/src/test/java/org/jasig/cas/client/jaas/CasLoginModuleTests.java @@ -208,8 +208,8 @@ public class CasLoginModuleTests { final String FAILURE_RESPONSE = "Ticket ST-12345-ABCDEFGHIJKLMNOPQRSTUVWXYZ-hosta not recognized"; options.put("cacheAssertions", "true"); - // Cache timeout is 1 second (Calendar.SECOND=13) - options.put("cacheTimeoutUnits", "13"); + // Cache timeout is 1 second + options.put("cacheTimeoutUnit", "SECONDS"); options.put("cacheTimeout", "1"); server.content = SUCCESS_RESPONSE.getBytes(server.encoding);