SEC-423: Fixed IllegalArguemntException being thrown by checking for null contextFromSessionObject

core/src/main/java/org/acegisecurity/context/HttpSessionContextIntegrationFilter.java

@@ -33,96 +33,126 @@ import org.springframework.beans.factory.InitializingBean;
 import org.springframework.util.Assert;
 import org.springframework.util.ReflectionUtils;
 
-
 /**
- * Populates the {@link SecurityContextHolder} with information obtained from the <code>HttpSession</code>.
+ * Populates the {@link SecurityContextHolder} with information obtained from
+ * the <code>HttpSession</code>.
  * 
  * <p>
- * The <code>HttpSession</code> will be queried to retrieve the <code>SecurityContext</code> that should be
+ * The <code>HttpSession</code> will be queried to retrieve the
- * stored against the <code>SecurityContextHolder</code> for the duration of the web request. At the end of the web
+ * <code>SecurityContext</code> that should be stored against the
- * request, any updates made to the <code>SecurityContextHolder</code> will be persisted back to the
+ * <code>SecurityContextHolder</code> for the duration of the web request. At
+ * the end of the web request, any updates made to the
+ * <code>SecurityContextHolder</code> will be persisted back to the
  * <code>HttpSession</code> by this filter.
  * </p>
  * <p>
- * If a valid <code>SecurityContext</code> cannot be obtained from the <code>HttpSession</code> for whatever
+ * If a valid <code>SecurityContext</code> cannot be obtained from the
- * reason, a fresh <code>SecurityContext</code> will be created and used instead. The created object will be of the
+ * <code>HttpSession</code> for whatever reason, a fresh
- * instance defined by the {@link #setContext(Class)} method (which defaults to {@link
+ * <code>SecurityContext</code> will be created and used instead. The created
+ * object will be of the instance defined by the {@link #setContext(Class)}
+ * method (which defaults to {@link
  * org.acegisecurity.context.SecurityContextImpl}.
  * </p>
  * <p>
- * No <code>HttpSession</code> will be created by this filter if one does not already exist. If at the end of
+ * No <code>HttpSession</code> will be created by this filter if one does not
- * the web request the <code>HttpSession</code> does not exist, a <code>HttpSession</code> will <b>only</b> be created
+ * already exist. If at the end of the web request the <code>HttpSession</code>
- * if the current contents of the <code>SecurityContextHolder</code> are not {@link
+ * does not exist, a <code>HttpSession</code> will <b>only</b> be created if
- * java.lang.Object#equals(java.lang.Object)} to a <code>new</code> instance of {@link #setContext(Class)}. This
+ * the current contents of the <code>SecurityContextHolder</code> are not
- * avoids needless <code>HttpSession</code> creation, but automates the storage of changes made to the
+ * {@link java.lang.Object#equals(java.lang.Object)} to a <code>new</code>
- * <code>SecurityContextHolder</code>. There is one exception to this rule, that is if the {@link
+ * instance of {@link #setContext(Class)}. This avoids needless
- * #forceEagerSessionCreation} property is <code>true</code>, in which case sessions will always be created
+ * <code>HttpSession</code> creation, but automates the storage of changes
- * irrespective of normal session-minimisation logic (the default is <code>false</code>, as this is resource intensive
+ * made to the <code>SecurityContextHolder</code>. There is one exception to
- * and not recommended).
+ * this rule, that is if the {@link #forceEagerSessionCreation} property is
+ * <code>true</code>, in which case sessions will always be created
+ * irrespective of normal session-minimisation logic (the default is
+ * <code>false</code>, as this is resource intensive and not recommended).
  * </p>
  * <p>
- * This filter will only execute once per request, to resolve servlet container (specifically Weblogic)
+ * This filter will only execute once per request, to resolve servlet container
- * incompatibilities.</p>
+ * (specifically Weblogic) incompatibilities.
- * <p>
- * If for whatever reason no <code>HttpSession</code> should <b>ever</b> be created (eg this filter is only
- * being used with Basic authentication or similar clients that will never present the same <code>jsessionid</code>
- * etc), the {@link #setAllowSessionCreation(boolean)} should be set to <code>false</code>. Only do this if you really
- * need to conserve server memory and ensure all classes using the <code>SecurityContextHolder</code> are designed to
- * have no persistence of the <code>SecurityContext</code> between web requests. Please note that if {@link
- * #forceEagerSessionCreation} is <code>true</code>, the <code>allowSessionCreation</code> must also be
- * <code>true</code> (setting it to <code>false</code> will cause a startup time error).
  * </p>
  * <p>
- * This filter MUST be executed BEFORE any authentication processing mechanisms. Authentication processing
+ * If for whatever reason no <code>HttpSession</code> should <b>ever</b> be
- * mechanisms (eg BASIC, CAS processing filters etc) expect the <code>SecurityContextHolder</code> to contain a valid
+ * created (eg this filter is only being used with Basic authentication or
+ * similar clients that will never present the same <code>jsessionid</code>
+ * etc), the {@link #setAllowSessionCreation(boolean)} should be set to
+ * <code>false</code>. Only do this if you really need to conserve server
+ * memory and ensure all classes using the <code>SecurityContextHolder</code>
+ * are designed to have no persistence of the <code>SecurityContext</code>
+ * between web requests. Please note that if {@link #forceEagerSessionCreation}
+ * is <code>true</code>, the <code>allowSessionCreation</code> must also be
+ * <code>true</code> (setting it to <code>false</code> will cause a startup
+ * time error).
+ * </p>
+ * <p>
+ * This filter MUST be executed BEFORE any authentication processing mechanisms.
+ * Authentication processing mechanisms (eg BASIC, CAS processing filters etc)
+ * expect the <code>SecurityContextHolder</code> to contain a valid
  * <code>SecurityContext</code> by the time they execute.
  * </p>
  * 
  * @author Ben Alex
  * @author Patrick Burleson
- * @version $Id$
+ * @version $Id: HttpSessionContextIntegrationFilter.java 1784 2007-02-24
+ * 21:00:24Z luke_t $
  */
 public class HttpSessionContextIntegrationFilter implements InitializingBean, Filter {
-    //~ Static fields/initializers =====================================================================================
+	// ~ Static fields/initializers
+	// =====================================================================================
 
 	protected static final Log logger = LogFactory.getLog(HttpSessionContextIntegrationFilter.class);
+
 	private static final String FILTER_APPLIED = "__acegi_session_integration_filter_applied";
+
 	public static final String ACEGI_SECURITY_CONTEXT_KEY = "ACEGI_SECURITY_CONTEXT";
 
-    //~ Instance fields ================================================================================================
+	// ~ Instance fields
+	// ================================================================================================
 
 	private Class context = SecurityContextImpl.class;
+
 	private Object contextObject;
 
 	/**
-     * Indicates if this filter can create a <code>HttpSession</code> if needed (sessions are always created
+	 * Indicates if this filter can create a <code>HttpSession</code> if
-     * sparingly, but setting this value to <code>false</code> will prohibit sessions from ever being created).
+	 * needed (sessions are always created sparingly, but setting this value to
-     * Defaults to <code>true</code>. Do not set to <code>false</code> if you are have set {@link
+	 * <code>false</code> will prohibit sessions from ever being created).
-     * #forceEagerSessionCreation} to <code>true</code>, as the properties would be in conflict.
+	 * Defaults to <code>true</code>. Do not set to <code>false</code> if
+	 * you are have set {@link #forceEagerSessionCreation} to <code>true</code>,
+	 * as the properties would be in conflict.
 	 */
 	private boolean allowSessionCreation = true;
 
 	/**
-     * Indicates if this filter is required to create a <code>HttpSession</code> for every request before
+	 * Indicates if this filter is required to create a <code>HttpSession</code>
-     * proceeding through the filter chain, even if the <code>HttpSession</code> would not ordinarily have been
+	 * for every request before proceeding through the filter chain, even if the
-     * created. By default this is <code>false</code>, which is entirely appropriate for most circumstances as you do
+	 * <code>HttpSession</code> would not ordinarily have been created. By
-     * not want a <code>HttpSession</code> created unless the filter actually needs one. It is envisaged the main
+	 * default this is <code>false</code>, which is entirely appropriate for
-     * situation in which this property would be set to <code>true</code> is if using other filters that depend on a
+	 * most circumstances as you do not want a <code>HttpSession</code>
-     * <code>HttpSession</code> already existing, such as those which need to obtain a session ID. This is only
+	 * created unless the filter actually needs one. It is envisaged the main
-     * required in specialised cases, so leave it set to <code>false</code> unless you have an actual requirement and
+	 * situation in which this property would be set to <code>true</code> is
-     * are conscious of the session creation overhead.
+	 * if using other filters that depend on a <code>HttpSession</code>
+	 * already existing, such as those which need to obtain a session ID. This
+	 * is only required in specialised cases, so leave it set to
+	 * <code>false</code> unless you have an actual requirement and are
+	 * conscious of the session creation overhead.
 	 */
 	private boolean forceEagerSessionCreation = false;
 
 	/**
-     * Indicates whether the <code>SecurityContext</code> will be cloned from the <code>HttpSession</code>. The
+	 * Indicates whether the <code>SecurityContext</code> will be cloned from
-     * default is to simply reference (ie the default is <code>false</code>). The default may cause issues if
+	 * the <code>HttpSession</code>. The default is to simply reference (ie
-     * concurrent threads need to have a different security identity from other threads being concurrently processed
+	 * the default is <code>false</code>). The default may cause issues if
-     * that share the same <code>HttpSession</code>. In most normal environments this does not represent an issue,
+	 * concurrent threads need to have a different security identity from other
-     * as changes to the security identity in one thread is allowed to affect the security identitiy in other
+	 * threads being concurrently processed that share the same
-     * threads associated with the same <code>HttpSession</code>. For unusual cases where this is not permitted,
+	 * <code>HttpSession</code>. In most normal environments this does not
-     * change this value to <code>true</code> and ensure the {@link #context} is set to a <code>SecurityContext</code>
+	 * represent an issue, as changes to the security identity in one thread is
-     * that implements {@link Cloneable} and overrides the <code>clone()</code> method.
+	 * allowed to affect the security identitiy in other threads associated with
+	 * the same <code>HttpSession</code>. For unusual cases where this is not
+	 * permitted, change this value to <code>true</code> and ensure the
+	 * {@link #context} is set to a <code>SecurityContext</code> that
+	 * implements {@link Cloneable} and overrides the <code>clone()</code>
+	 * method.
 	 */
 	private boolean cloneFromHttpSession = false;
 
@@ -138,14 +168,14 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi
 		this.contextObject = generateNewContext();
 	}
 
-    //~ Methods ========================================================================================================
+	// ~ Methods
+	// ========================================================================================================
 
 	public void afterPropertiesSet() throws Exception {
 		if ((this.context == null) || (!SecurityContext.class.isAssignableFrom(this.context))) {
-            throw new IllegalArgumentException(
+			throw new IllegalArgumentException("context must be defined and implement SecurityContext "
-                "context must be defined and implement SecurityContext "
+					+ "(typically use org.acegisecurity.context.SecurityContextImpl; existing class is " + this.context
-                + "(typically use org.acegisecurity.context.SecurityContextImpl; existing class is "
+					+ ")");
-                + this.context + ")");
 		}
 
 		if ((forceEagerSessionCreation == true) && (allowSessionCreation == false)) {
@@ -157,14 +187,16 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi
 	/**
 	 * Does nothing. We use IoC container lifecycle services instead.
 	 */
-    public void destroy() {}
+	public void destroy() {
+	}
 
-    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
+	public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException,
-        throws IOException, ServletException {
+			ServletException {
 		if ((request != null) && (request.getAttribute(FILTER_APPLIED) != null)) {
 			// ensure that filter is only applied once per request
 			chain.doFilter(request, response);
-        } else {
+		}
+		else {
 			if (request != null) {
 				request.setAttribute(FILTER_APPLIED, Boolean.TRUE);
 			}
@@ -174,13 +206,16 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi
 
 			try {
 				httpSession = ((HttpServletRequest) request).getSession(forceEagerSessionCreation);
-            } catch (IllegalStateException ignored) {}
+			}
+			catch (IllegalStateException ignored) {
+			}
 
 			if (httpSession != null) {
 				httpSessionExistedAtStartOfRequest = true;
 
 				Object contextFromSessionObject = httpSession.getAttribute(ACEGI_SECURITY_CONTEXT_KEY);
 
+				if (contextFromSessionObject != null) {
 					// Clone if required (see SEC-356)
 					if (cloneFromHttpSession) {
 						Assert.isInstanceOf(Cloneable.class, contextFromSessionObject,
@@ -191,12 +226,12 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi
 								m.setAccessible(true);
 							}
 							contextFromSessionObject = m.invoke(contextFromSessionObject, new Object[] {});
-                    } catch (Exception ex) {
+						}
+						catch (Exception ex) {
 							ReflectionUtils.handleReflectionException(ex);
 						}
 					}
 
-                if (contextFromSessionObject != null) {
 					if (contextFromSessionObject instanceof SecurityContext) {
 						if (logger.isDebugEnabled()) {
 							logger.debug("Obtained from ACEGI_SECURITY_CONTEXT a valid SecurityContext and "
@@ -204,9 +239,11 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi
 						}
 
 						SecurityContextHolder.setContext((SecurityContext) contextFromSessionObject);
-                    } else {
+					}
+					else {
 						if (logger.isWarnEnabled()) {
-                            logger.warn("ACEGI_SECURITY_CONTEXT did not contain a SecurityContext but contained: '"
+							logger
+									.warn("ACEGI_SECURITY_CONTEXT did not contain a SecurityContext but contained: '"
 											+ contextFromSessionObject
 											+ "'; are you improperly modifying the HttpSession directly "
 											+ "(you should always use SecurityContextHolder) or using the HttpSession attribute "
@@ -216,7 +253,8 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi
 
 						SecurityContextHolder.setContext(generateNewContext());
 					}
-                } else {
+				}
+				else {
 					if (logger.isDebugEnabled()) {
 						logger.debug("HttpSession returned null object for ACEGI_SECURITY_CONTEXT - new "
 								+ "SecurityContext instance associated with SecurityContextHolder");
@@ -224,7 +262,8 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi
 
 					SecurityContextHolder.setContext(generateNewContext());
 				}
-            } else {
+			}
+			else {
 				if (logger.isDebugEnabled()) {
 					logger.debug("No HttpSession currently exists - new SecurityContext instance "
 							+ "associated with SecurityContextHolder");
@@ -243,16 +282,21 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi
 
 			try {
 				chain.doFilter(request, response);
-            } catch (IOException ioe) {
+			}
+			catch (IOException ioe) {
 				throw ioe;
-            } catch (ServletException se) {
+			}
+			catch (ServletException se) {
 				throw se;
-            } finally {
+			}
+			finally {
 				// do clean up, even if there was an exception
 				// Store context back to HttpSession
 				try {
 					httpSession = ((HttpServletRequest) request).getSession(false);
-                } catch (IllegalStateException ignored) {}
+				}
+				catch (IllegalStateException ignored) {
+				}
 
 				if ((httpSession == null) && httpSessionExistedAtStartOfRequest) {
 					if (logger.isDebugEnabled()) {
@@ -265,30 +309,37 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi
 				if ((httpSession == null) && !httpSessionExistedAtStartOfRequest) {
 					if (!allowSessionCreation) {
 						if (logger.isDebugEnabled()) {
-                            logger.debug("The HttpSession is currently null, and the "
+							logger
+									.debug("The HttpSession is currently null, and the "
 											+ "HttpSessionContextIntegrationFilter is prohibited from creating an HttpSession "
 											+ "(because the allowSessionCreation property is false) - SecurityContext thus not "
 											+ "stored for next request");
 						}
-                    } else if (!contextObject.equals(SecurityContextHolder.getContext())) {
+					}
+					else if (!contextObject.equals(SecurityContextHolder.getContext())) {
 						if (logger.isDebugEnabled()) {
 							logger.debug("HttpSession being created as SecurityContextHolder contents are non-default");
 						}
 
 						try {
 							httpSession = ((HttpServletRequest) request).getSession(true);
-                        } catch (IllegalStateException ignored) {}
+						}
-                    } else {
+						catch (IllegalStateException ignored) {
+						}
+					}
+					else {
 						if (logger.isDebugEnabled()) {
-                            logger.debug(
+							logger
-                                    "HttpSession is null, but SecurityContextHolder has not changed from default: ' "
+									.debug("HttpSession is null, but SecurityContextHolder has not changed from default: ' "
 											+ SecurityContextHolder.getContext()
 											+ "'; not creating HttpSession or storing SecurityContextHolder contents");
 						}
 					}
 				}
 
-                // If HttpSession exists, store current SecurityContextHolder contents but only if SecurityContext has
+				// If HttpSession exists, store current
+				// SecurityContextHolder contents but only if
+				// SecurityContext has
 				// actually changed (see JIRA SEC-37)
 				if ((httpSession != null)
 						&& (SecurityContextHolder.getContext().hashCode() != contextWhenChainProceeded)) {
@@ -313,9 +364,11 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi
 	public SecurityContext generateNewContext() throws ServletException {
 		try {
 			return (SecurityContext) this.context.newInstance();
-        } catch (InstantiationException ie) {
+		}
+		catch (InstantiationException ie) {
 			throw new ServletException(ie);
-        } catch (IllegalAccessException iae) {
+		}
+		catch (IllegalAccessException iae) {
 			throw new ServletException(iae);
 		}
 	}
@@ -331,7 +384,8 @@ public class HttpSessionContextIntegrationFilter implements InitializingBean, Fi
 	 * 
 	 * @throws ServletException ignored
 	 */
-    public void init(FilterConfig filterConfig) throws ServletException {}
+	public void init(FilterConfig filterConfig) throws ServletException {
+	}
 
 	public boolean isAllowSessionCreation() {
 		return allowSessionCreation;