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.
This commit is contained in:
Marvin S. Addison
parent
ec0c7d5162
commit
ee2b719098
|
|
@ -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.</li>
|
||||
* <li>cacheTimeout (optional) - Assertion cache timeout in minutes.</li>
|
||||
* <li>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.</li>
|
||||
* <li>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.</li>
|
||||
* </ul>
|
||||
*
|
||||
* <p>
|
||||
|
|
@ -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:
|
||||
* <ul>
|
||||
* <li>{@link Calendar#HOUR}</li>
|
||||
* <li>{@link Calendar#MINUTE}</li>
|
||||
* <li>{@link Calendar#SECOND}</li>
|
||||
* <li>{@link Calendar#MILLISECOND}</li>
|
||||
* </ul>
|
||||
*/
|
||||
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.</li>
|
||||
* <li>cacheTimeout (optional) - assertion cache timeout in minutes.</li>
|
||||
* <li>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.</li>
|
||||
* <li>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.</li>
|
||||
* </ul>
|
||||
*/
|
||||
public final void initialize(final Subject subject, final CallbackHandler handler, final Map<String,?> state, final Map<String, ?> options) {
|
||||
public final void initialize(
|
||||
final Subject subject,
|
||||
final CallbackHandler handler,
|
||||
final Map<String,?> state,
|
||||
final Map<String, ?> options) {
|
||||
|
||||
this.assertion = null;
|
||||
this.callbackHandler = handler;
|
||||
this.subject = subject;
|
||||
this.sharedState = (Map<String,Object>) state;
|
||||
this.sharedState = (Map<String, Object>) state;
|
||||
this.sharedState = new HashMap<String, Object>(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<String,?> propertyMap) {
|
||||
CommonUtils.assertTrue(propertyMap.containsKey("casServerUrlPrefix"), "Required property casServerUrlPrefix not found.");
|
||||
CommonUtils.assertTrue(
|
||||
propertyMap.containsKey("casServerUrlPrefix"), "Required property casServerUrlPrefix not found.");
|
||||
|
||||
final Class<TicketValidator> 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<Map.Entry<TicketCredential, Assertion>> 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<Map.Entry<TicketCredential, Assertion>> 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();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -208,8 +208,8 @@ public class CasLoginModuleTests {
|
|||
final String FAILURE_RESPONSE = "<cas:serviceResponse xmlns:cas='http://www.yale.edu/tp/cas'><cas:authenticationFailure code=\"INVALID_TICKET\">Ticket ST-12345-ABCDEFGHIJKLMNOPQRSTUVWXYZ-hosta not recognized</cas:authenticationFailure></cas:serviceResponse>";
|
||||
|
||||
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);
|
||||
|
|
|
|||
Loading…
Reference in New Issue