From 7edf9635e7c6c134e66148e0aaa50ae2b213fdd9 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Mon, 27 Apr 2009 06:06:42 +0000 Subject: [PATCH] Refactored to avoid use of SecurityContextimpl directly (similarly to approach in main servlet-based code). --- ...tSessionContextIntegrationInterceptor.java | 85 ++++++++++--------- ...ionContextIntegrationInterceptorTests.java | 19 ----- 2 files changed, 45 insertions(+), 59 deletions(-) diff --git a/portlet/src/main/java/org/springframework/security/portlet/PortletSessionContextIntegrationInterceptor.java b/portlet/src/main/java/org/springframework/security/portlet/PortletSessionContextIntegrationInterceptor.java index 2fedd10f21..ed99c2f8d7 100644 --- a/portlet/src/main/java/org/springframework/security/portlet/PortletSessionContextIntegrationInterceptor.java +++ b/portlet/src/main/java/org/springframework/security/portlet/PortletSessionContextIntegrationInterceptor.java @@ -18,20 +18,30 @@ package org.springframework.security.portlet; import java.lang.reflect.Method; -import javax.portlet.*; +import javax.portlet.ActionRequest; +import javax.portlet.ActionResponse; +import javax.portlet.EventRequest; +import javax.portlet.EventResponse; +import javax.portlet.PortletException; +import javax.portlet.PortletRequest; +import javax.portlet.PortletResponse; +import javax.portlet.PortletSession; +import javax.portlet.RenderRequest; +import javax.portlet.RenderResponse; +import javax.portlet.ResourceRequest; +import javax.portlet.ResourceResponse; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.InitializingBean; +import org.springframework.security.core.context.SecurityContext; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.web.context.HttpSessionSecurityContextRepository; +import org.springframework.security.web.context.SecurityContextPersistenceFilter; import org.springframework.util.Assert; import org.springframework.util.ReflectionUtils; import org.springframework.web.portlet.HandlerInterceptor; import org.springframework.web.portlet.ModelAndView; -import org.springframework.security.core.context.SecurityContext; -import org.springframework.security.core.context.SecurityContextHolder; -import org.springframework.security.core.context.SecurityContextImpl; -import org.springframework.security.web.context.SecurityContextPersistenceFilter; -import org.springframework.security.web.context.HttpSessionSecurityContextRepository; /** *

This interceptor populates the {@link SecurityContextHolder} with information obtained from the @@ -45,13 +55,13 @@ import org.springframework.security.web.context.HttpSessionSecurityContextReposi * *

If a valid SecurityContext cannot be obtained from the PortletSession for * whatever reason, a fresh SecurityContext 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.springframework.security.core.context.SecurityContextImpl}.

+ * will be of the instance defined by the {@link #setContext(Class)} method. If this hasn't been set, a call to + * {@link SecurityContextHolder#createEmptyContext()} will be used to create the instance. * *

A PortletSession may be created by this interceptor if one does not already exist. If at the * end of the portlet request the PortletSession does not exist, one will only be created if * the current contents of the SecurityContextHolder are not the {@link java.lang.Object#equals} - * to a new instance of {@link #context}. This avoids needless PortletSession creation, + * to a new instance of {@link #contextClass}. This avoids needless PortletSession creation, * and automates the storage of changes made to the SecurityContextHolder. There is one exception to * this rule, that is if the {@link #forceEagerSessionCreation} property is true, in which case * sessions will always be created irrespective of normal session-minimization logic (the default is @@ -92,8 +102,7 @@ import org.springframework.security.web.context.HttpSessionSecurityContextReposi * @since 2.0 * @version $Id$ */ -public class PortletSessionContextIntegrationInterceptor - implements InitializingBean, HandlerInterceptor { +public class PortletSessionContextIntegrationInterceptor implements InitializingBean, HandlerInterceptor { //~ Static fields/initializers ===================================================================================== @@ -106,7 +115,7 @@ public class PortletSessionContextIntegrationInterceptor //~ Instance fields ================================================================================================ - private Class context = SecurityContextImpl.class; + private Class contextClass; private Object contextObject; @@ -146,7 +155,7 @@ public class PortletSessionContextIntegrationInterceptor * allowed to affect the security identity in other threads associated with * the same PortletSession. For unusual cases where this is not * permitted, change this value to true and ensure the - * {@link #context} is set to a SecurityContext that + * {@link #contextClass} is set to a SecurityContext that * implements {@link Cloneable} and overrides the clone() * method. */ @@ -173,14 +182,6 @@ public class PortletSessionContextIntegrationInterceptor //~ Methods ======================================================================================================== public void afterPropertiesSet() throws Exception { - - // check that the value of context is legal - if ((this.context == null) || (!SecurityContext.class.isAssignableFrom(this.context))) { - throw new IllegalArgumentException("context must be defined and implement SecurityContext " - + "(typically use org.springframework.security.core.context.SecurityContextImpl; existing class is " - + this.context + ")"); - } - // check that session creation options make sense if ((forceEagerSessionCreation == true) && (allowSessionCreation == false)) { throw new IllegalArgumentException( @@ -265,16 +266,16 @@ public class PortletSessionContextIntegrationInterceptor portletSession = request.getPortletSession(forceEagerSessionCreation); } catch (IllegalStateException ignored) {} - // if there is a session, then see if there is a context to bring in + // if there is a session, then see if there is a contextClass to bring in if (portletSession != null) { // remember that the session already existed portletSessionExistedAtStartOfRequest = true; - // attempt to retrieve the context from the session + // attempt to retrieve the contextClass from the session Object contextFromSessionObject = portletSession.getAttribute(SPRING_SECURITY_CONTEXT_KEY, portletSessionScope()); - // if we got a context then place it into the holder + // if we got a contextClass then place it into the holder if (contextFromSessionObject != null) { // if we are supposed to clone it, then do so @@ -293,7 +294,7 @@ public class PortletSessionContextIntegrationInterceptor } } - // if what we got is a valid context then place it into the holder, otherwise create a new one + // if what we got is a valid contextClass then place it into the holder, otherwise create a new one if (contextFromSessionObject instanceof SecurityContext) { if (logger.isDebugEnabled()) logger.debug("Obtained from SPRING_SECURITY_CONTEXT a valid SecurityContext and " @@ -312,7 +313,7 @@ public class PortletSessionContextIntegrationInterceptor } else { - // there was no context in the session, so create a new context and put it in the holder + // there was no contextClass in the session, so create a new contextClass and put it in the holder if (logger.isDebugEnabled()) logger.debug("PortletSession returned null object for SPRING_SECURITY_CONTEXT - new " + "SecurityContext instance associated with SecurityContextHolder"); @@ -321,7 +322,7 @@ public class PortletSessionContextIntegrationInterceptor } else { - // there was no session, so create a new context and place it in the holder + // there was no session, so create a new contextClass and place it in the holder if (logger.isDebugEnabled()) logger.debug("No PortletSession currently exists - new SecurityContext instance " + "associated with SecurityContextHolder"); @@ -329,7 +330,7 @@ public class PortletSessionContextIntegrationInterceptor } - // place attributes onto the request to remember if the session existed and the hashcode of the context + // place attributes onto the request to remember if the session existed and the hashcode of the contextClass request.setAttribute(SESSION_EXISTED, new Boolean(portletSessionExistedAtStartOfRequest)); request.setAttribute(CONTEXT_HASHCODE, new Integer(SecurityContextHolder.getContext().hashCode())); @@ -341,7 +342,7 @@ public class PortletSessionContextIntegrationInterceptor PortletSession portletSession = null; - // retrieve the attributes that remember if the session existed and the hashcode of the context + // retrieve the attributes that remember if the session existed and the hashcode of the contextClass boolean portletSessionExistedAtStartOfRequest = ((Boolean)request.getAttribute(SESSION_EXISTED)).booleanValue(); int oldContextHashCode = ((Integer)request.getAttribute(CONTEXT_HASHCODE)).intValue(); @@ -368,7 +369,7 @@ public class PortletSessionContextIntegrationInterceptor + "(because the allowSessionCreation property is false) - SecurityContext thus not " + "stored for next request"); } - // if the context was changed during the request, then go ahead and create a session + // if the contextClass was changed during the request, then go ahead and create a session else if (!contextObject.equals(SecurityContextHolder.getContext())) { if (logger.isDebugEnabled()) logger.debug("PortletSession being created as SecurityContextHolder contents are non-default"); @@ -376,7 +377,7 @@ public class PortletSessionContextIntegrationInterceptor portletSession = request.getPortletSession(true); } catch (IllegalStateException ignored) {} } - // if nothing in the context changed, then don't bother to create a session + // if nothing in the contextClass changed, then don't bother to create a session else { if (logger.isDebugEnabled()) logger.debug("PortletSession is null, but SecurityContextHolder has not changed from default: ' " @@ -385,7 +386,7 @@ public class PortletSessionContextIntegrationInterceptor } } - // if the session exists and the context has changes, then store the context back into the session + // if the session exists and the contextClass has changes, then store the contextClass back into the session if ((portletSession != null) && (SecurityContextHolder.getContext().hashCode() != oldContextHashCode)) { portletSession.setAttribute(SPRING_SECURITY_CONTEXT_KEY, SecurityContextHolder.getContext(), portletSessionScope()); @@ -397,21 +398,25 @@ public class PortletSessionContextIntegrationInterceptor // remove the contents of the holder SecurityContextHolder.clearContext(); if (logger.isDebugEnabled()) - logger.debug("SecurityContextHolder set to new context, as request processing completed"); + logger.debug("SecurityContextHolder set to new contextClass, as request processing completed"); } /** * Creates a new SecurityContext object. The specific class is - * determined by the setting of the {@link #context} property. + * determined by the setting of the {@link #contextClass} property. * @return the new SecurityContext * @throws PortletException if the creation throws an InstantiationException or * an IllegalAccessException, then this method will wrap them in a * PortletException */ - public SecurityContext generateNewContext() throws PortletException { + SecurityContext generateNewContext() throws PortletException { + if (contextClass == null) { + return SecurityContextHolder.createEmptyContext(); + } + try { - return (SecurityContext) this.context.newInstance(); + return this.contextClass.newInstance(); } catch (InstantiationException ie) { throw new PortletException(ie); } catch (IllegalAccessException iae) { @@ -427,12 +432,12 @@ public class PortletSessionContextIntegrationInterceptor } - public Class getContext() { - return context; + public Class getContext() { + return contextClass; } - public void setContext(Class secureContext) { - this.context = secureContext; + public void setContext(Class secureContext) { + this.contextClass = secureContext; } public boolean isAllowSessionCreation() { diff --git a/portlet/src/test/java/org/springframework/security/portlet/PortletSessionContextIntegrationInterceptorTests.java b/portlet/src/test/java/org/springframework/security/portlet/PortletSessionContextIntegrationInterceptorTests.java index 0eaaccea49..787165ff3e 100644 --- a/portlet/src/test/java/org/springframework/security/portlet/PortletSessionContextIntegrationInterceptorTests.java +++ b/portlet/src/test/java/org/springframework/security/portlet/PortletSessionContextIntegrationInterceptorTests.java @@ -68,25 +68,6 @@ public class PortletSessionContextIntegrationInterceptorTests extends TestCase { interceptor.afterPropertiesSet(); } - public void testDetectsMissingOrInvalidContext() throws Exception { - PortletSessionContextIntegrationInterceptor interceptor = new PortletSessionContextIntegrationInterceptor(); - try { - interceptor.setContext(null); - interceptor.afterPropertiesSet(); - fail("Shown have thrown IllegalArgumentException"); - } catch (IllegalArgumentException expected) { - // ignore - } - try { - interceptor.setContext(Integer.class); - assertEquals(Integer.class, interceptor.getContext()); - interceptor.afterPropertiesSet(); - fail("Shown have thrown IllegalArgumentException"); - } catch (IllegalArgumentException expected) { - // ignore - } - } - public void testNormalRenderRequestProcessing() throws Exception { // Build an Authentication object we simulate came from PortletSession