modifications to clean up spring-remoting support (was causing stack overflows)

git-svn-id: https://svn.apache.org/repos/asf/incubator/jsecurity/trunk@759607 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/core/src/main/java/org/apache/ki/session/mgt/AbstractValidatingSessionManager.java b/core/src/main/java/org/apache/ki/session/mgt/AbstractValidatingSessionManager.java
index 0e6aff7..2853eb4 100644
--- a/core/src/main/java/org/apache/ki/session/mgt/AbstractValidatingSessionManager.java
+++ b/core/src/main/java/org/apache/ki/session/mgt/AbstractValidatingSessionManager.java
@@ -218,7 +218,7 @@
             String msg = "The " + getClass().getName() + " implementation only supports validating " +
                 "Session implementations of the " + ValidatingSession.class.getName() + " interface.  " +
                 "Please either implement this interface in your session implementation or override the " +
-                getClass().getName() + ".validate(Session) method to perform validation.";
+                AbstractValidatingSessionManager.class.getName() + ".doValidate(Session) method to perform validation.";
             throw new IllegalStateException(msg);
         }
     }
diff --git a/core/src/main/java/org/apache/ki/session/mgt/DelegatingSession.java b/core/src/main/java/org/apache/ki/session/mgt/DelegatingSession.java
index 8a2f801..6ec301f 100644
--- a/core/src/main/java/org/apache/ki/session/mgt/DelegatingSession.java
+++ b/core/src/main/java/org/apache/ki/session/mgt/DelegatingSession.java
@@ -22,6 +22,7 @@
 import java.net.InetAddress;
 import java.util.Collection;
 import java.util.Date;
+import java.util.Collections;
 
 import org.apache.ki.session.InvalidSessionException;
 import org.apache.ki.session.ReplacedSessionException;
@@ -32,10 +33,10 @@
  * {@link org.apache.ki.session.Session Session}.
  * This implementation is basically a proxy to a server-side {@link SessionManager SessionManager},
  * which will return the proper results for each method call.
- *
+ * <p/>
  * <p>A <tt>DelegatingSession</tt> will cache data when appropriate to avoid a remote method invocation,
  * only communicating with the server when necessary.
- *
+ * <p/>
  * <p>Of course, if used in-process with a SessionManager business POJO, as might be the case in a
  * web-based application where the web classes and server-side business pojos exist in the same
  * JVM, a remote method call will not be incurred.
@@ -54,9 +55,7 @@
     private Date startTimestamp = null;
     private InetAddress hostAddress = null;
 
-    /**
-     * Handle to a server-side SessionManager.  See {@link #setSessionManager} for details.
-     */
+    /** Handle to a server-side SessionManager.  See {@link #setSessionManager} for details. */
     private SessionManager sessionManager = null;
 
 
@@ -85,7 +84,7 @@
      * probably be a remoting proxy which executes remote method invocations.  In a single-process
      * environment (e.g. a web  application deployed in the same JVM of the application server),
      * the <tt>SessionManager</tt> can be the actual business POJO implementation.
-     *
+     * <p/>
      * <p>You'll notice the {@link Session Session} interface and the {@link SessionManager}
      * interface are nearly identical.  This is to ensure the SessionManager can support
      * most method calls in the Session interface, via this handle/proxy technique.  The session
@@ -110,16 +109,12 @@
         this.id = id;
     }
 
-    /**
-     * @see org.apache.ki.session.Session#getId()
-     */
+    /** @see org.apache.ki.session.Session#getId() */
     public Serializable getId() {
         return id;
     }
 
-    /**
-     * @see org.apache.ki.session.Session#getStartTimestamp()
-     */
+    /** @see org.apache.ki.session.Session#getStartTimestamp() */
     public Date getStartTimestamp() {
         if (startTimestamp == null) {
             try {
@@ -132,9 +127,7 @@
         return startTimestamp;
     }
 
-    /**
-     * @see org.apache.ki.session.Session#getLastAccessTime()
-     */
+    /** @see org.apache.ki.session.Session#getLastAccessTime() */
     public Date getLastAccessTime() {
         //can't cache - only business pojo knows the accurate time:
         try {
@@ -163,9 +156,7 @@
         }
     }
 
-    /**
-     * @see org.apache.ki.session.Session#getHostAddress()
-     */
+    /** @see org.apache.ki.session.Session#getHostAddress() */
     public InetAddress getHostAddress() {
         if (hostAddress == null) {
             try {
@@ -178,57 +169,51 @@
         return hostAddress;
     }
 
-    /**
-     * @see org.apache.ki.session.Session#touch()
-     */
+    /** @see org.apache.ki.session.Session#touch() */
     public void touch() throws InvalidSessionException {
         try {
             sessionManager.touch(id);
         } catch (ReplacedSessionException e) {
             this.id = e.getNewSessionId();
-            sessionManager.touch(id);
+            // No need to 'hit' the session manager again - a newly created session is 'touched' at the time of creation
         }
     }
 
-    /**
-     * @see org.apache.ki.session.Session#stop()
-     */
+    /** @see org.apache.ki.session.Session#stop() */
     public void stop() throws InvalidSessionException {
         try {
             sessionManager.stop(id);
         } catch (ReplacedSessionException e) {
             this.id = e.getNewSessionId();
+            //TODO - prevent sessionManager from creating new session when 'stop' is already requested.
             sessionManager.stop(id);
         }
     }
 
-    /**
-     * @see org.apache.ki.session.Session#getAttributeKeys
-     */
+    /** @see org.apache.ki.session.Session#getAttributeKeys */
+    @SuppressWarnings({"unchecked"})
     public Collection<Object> getAttributeKeys() throws InvalidSessionException {
         try {
             return sessionManager.getAttributeKeys(id);
         } catch (ReplacedSessionException e) {
             this.id = e.getNewSessionId();
-            return sessionManager.getAttributeKeys(id);
+            // No need to 'hit' the session manager again - a new session won't have any attributes:
+            return Collections.EMPTY_SET;
         }
     }
 
-    /**
-     * @see org.apache.ki.session.Session#getAttribute(Object key)
-     */
+    /** @see org.apache.ki.session.Session#getAttribute(Object key) */
     public Object getAttribute(Object key) throws InvalidSessionException {
         try {
             return sessionManager.getAttribute(id, key);
         } catch (ReplacedSessionException e) {
             this.id = e.getNewSessionId();
-            return sessionManager.getAttribute(id, key);
+            // No need to 'hit' the session manager again - a new session won't have any attributes
+            return null;
         }
     }
 
-    /**
-     * @see Session#setAttribute(Object key, Object value)
-     */
+    /** @see Session#setAttribute(Object key, Object value) */
     public void setAttribute(Object key, Object value) throws InvalidSessionException {
         if (value == null) {
             removeAttribute(key);
@@ -242,15 +227,14 @@
         }
     }
 
-    /**
-     * @see Session#removeAttribute(Object key)
-     */
+    /** @see Session#removeAttribute(Object key) */
     public Object removeAttribute(Object key) throws InvalidSessionException {
         try {
             return sessionManager.removeAttribute(id, key);
         } catch (ReplacedSessionException e) {
             this.id = e.getNewSessionId();
-            return sessionManager.removeAttribute(id, key);
+            // No need to 'hit' the session manager again - a new session won't have any attributes:
+            return null;
         }
     }
 }
diff --git a/samples/spring/src/main/webapp/WEB-INF/resources/jsecurity.jnlp.jsp b/samples/spring/src/main/webapp/WEB-INF/resources/jsecurity.jnlp.jsp
index 167644b..9006719 100644
--- a/samples/spring/src/main/webapp/WEB-INF/resources/jsecurity.jnlp.jsp
+++ b/samples/spring/src/main/webapp/WEB-INF/resources/jsecurity.jnlp.jsp
@@ -38,7 +38,7 @@
     <resources>
         <j2se version="1.5"/>
         <jar href="ki-spring-sample.jar"/>
-        <jar href="ki.jar"/>
+        <jar href="ki-all.jar"/>
         <jar href="spring.jar"/>
         <jar href="slf4j-api.jar"/>
         <property name="ki.session.id" value="${sessionId}"/>
diff --git a/support/spring/src/main/java/org/apache/ki/spring/remoting/SecureRemoteInvocationExecutor.java b/support/spring/src/main/java/org/apache/ki/spring/remoting/SecureRemoteInvocationExecutor.java
index 385cd9f..2b11286 100644
--- a/support/spring/src/main/java/org/apache/ki/spring/remoting/SecureRemoteInvocationExecutor.java
+++ b/support/spring/src/main/java/org/apache/ki/spring/remoting/SecureRemoteInvocationExecutor.java
@@ -20,6 +20,7 @@
 
 import java.io.Serializable;
 import java.lang.reflect.InvocationTargetException;
+import java.net.InetAddress;
 
 import org.springframework.remoting.support.DefaultRemoteInvocationExecutor;
 import org.springframework.remoting.support.RemoteInvocation;
@@ -77,7 +78,13 @@
     ============================================*/
     @SuppressWarnings({"unchecked"})
     public Object invoke(RemoteInvocation invocation, Object targetObject) throws NoSuchMethodException, IllegalAccessException, InvocationTargetException {
+
         try {
+            InetAddress inet = (InetAddress)invocation.getAttribute(SecureRemoteInvocationFactory.INET_ADDRESS_KEY);
+            if (inet != null) {
+                ThreadContext.bind(inet);
+            }
+            
             Serializable sessionId = invocation.getAttribute(SecureRemoteInvocationFactory.SESSION_ID_KEY);
             if (sessionId != null) {
                 ThreadContext.bindSessionId(sessionId);
@@ -88,6 +95,7 @@
                             "on an existing Session will not be available during the method invocatin.");
                 }
             }
+            
             ThreadContext.bind(securityManager);
             ThreadContext.bind(securityManager.getSubject());
 
diff --git a/support/spring/src/main/java/org/apache/ki/spring/remoting/SecureRemoteInvocationFactory.java b/support/spring/src/main/java/org/apache/ki/spring/remoting/SecureRemoteInvocationFactory.java
index 9b5359a..5576ca7 100644
--- a/support/spring/src/main/java/org/apache/ki/spring/remoting/SecureRemoteInvocationFactory.java
+++ b/support/spring/src/main/java/org/apache/ki/spring/remoting/SecureRemoteInvocationFactory.java
@@ -19,6 +19,7 @@
 package org.apache.ki.spring.remoting;
 
 import java.io.Serializable;
+import java.net.InetAddress;
 
 import org.aopalliance.intercept.MethodInvocation;
 import org.springframework.remoting.support.DefaultRemoteInvocationFactory;
@@ -29,7 +30,9 @@
 import org.slf4j.LoggerFactory;
 
 import org.apache.ki.SecurityUtils;
+import org.apache.ki.util.ThreadContext;
 import org.apache.ki.session.Session;
+import org.apache.ki.session.mgt.SessionManager;
 import org.apache.ki.subject.Subject;
 
 
@@ -50,6 +53,7 @@
     private static final Logger log = LoggerFactory.getLogger(SecureRemoteInvocationFactory.class);
 
     public static final String SESSION_ID_KEY = Session.class.getName() + "_ID_KEY";
+    public static final String INET_ADDRESS_KEY = InetAddress.class.getName() + "_KEY";
 
     private static final String SESSION_ID_SYSTEM_PROPERTY_NAME = "ki.session.id";
 
@@ -57,35 +61,63 @@
      * Creates a {@link RemoteInvocation} with the current session ID as an
      * {@link RemoteInvocation#getAttribute(String) attribute}.
      *
-     * @param methodInvocation the method invocation that the remote invocation should
-     *                         be based on.
+     * @param mi the method invocation that the remote invocation should be based on.
      * @return a remote invocation object containing the current session ID as an attribute.
      */
-    public RemoteInvocation createRemoteInvocation(MethodInvocation methodInvocation) {
+    public RemoteInvocation createRemoteInvocation(MethodInvocation mi) {
+
         Serializable sessionId = null;
-        Subject subject = SecurityUtils.getSubject();
-        if (subject != null) {
+        InetAddress inet = null;
+        boolean sessionManagerMethodInvocation = false;
+
+        //If the calling MI is for a remoting SessionManager proxy, we need to acquire the session ID from the method
+        //argument and NOT interact with SecurityUtils/subject.getSession to avoid a stack overflow
+        if (SessionManager.class.equals(mi.getMethod().getDeclaringClass())) {
+            sessionManagerMethodInvocation = true;
+            //for SessionManager calls, all method calls require the session id as the first argument, with
+            //the exception of 'start' that takes in an InetAddress.  So, ignore that one case:
+            Object firstArg = mi.getArguments()[0];
+            if (!(firstArg instanceof InetAddress)) {
+                sessionId = (Serializable) firstArg;
+            }
+        }
+
+        //tried the proxy.  If sessionId is still null, only then try the Subject:
+        if (sessionId == null && !sessionManagerMethodInvocation) {
+            Subject subject = SecurityUtils.getSubject();
             Session session = subject.getSession(false);
             if (session != null) {
+                inet = session.getHostAddress();                
                 sessionId = session.getId();
             }
         }
 
+        //No call to the sessionManager, and the Subject doesn't have a session.  Try a system property
+        //as a last result:
         if (sessionId == null) {
             if (log.isTraceEnabled()) {
                 log.trace("No Session found for the currently executing subject via subject.getSession(false).  " +
-                        "Attempting to revert back to the 'ki.session.id' system property...");
+                    "Attempting to revert back to the 'ki.session.id' system property...");
+            }
+            sessionId = System.getProperty(SESSION_ID_SYSTEM_PROPERTY_NAME);
+            if (sessionId == null && log.isTraceEnabled()) {
+                log.trace("No 'ki.session.id' system property found.  Heuristics have been exhausted; " +
+                    "RemoteInvocation will not contain a sessionId.");
             }
         }
-        sessionId = System.getProperty(SESSION_ID_SYSTEM_PROPERTY_NAME);
-        if (sessionId == null && log.isTraceEnabled()) {
-            log.trace("No 'ki.session.id' system property found.  Heuristics have been exhausted; " +
-                    "RemoteInvocation will not contain a sessionId.");
+
+        if ( inet == null ) {
+            //try thread context:
+            inet = ThreadContext.getInetAddress();
         }
-        RemoteInvocation ri = new RemoteInvocation(methodInvocation);
+
+        RemoteInvocation ri = new RemoteInvocation(mi);
         if (sessionId != null) {
             ri.addAttribute(SESSION_ID_KEY, sessionId);
         }
+        if ( inet != null ) {
+            ri.addAttribute(INET_ADDRESS_KEY, inet);
+        }
 
         return ri;
     }
diff --git a/support/spring/src/test/java/org/apache/ki/spring/SpringKiFilterTest.java b/support/spring/src/test/java/org/apache/ki/spring/SpringKiFilterTest.java
index 9c3a610..c06bb93 100644
--- a/support/spring/src/test/java/org/apache/ki/spring/SpringKiFilterTest.java
+++ b/support/spring/src/test/java/org/apache/ki/spring/SpringKiFilterTest.java
@@ -18,19 +18,16 @@
  */

 package org.apache.ki.spring;

 

-import java.util.HashMap;

-import java.util.Map;

-import javax.servlet.FilterConfig;

-import javax.servlet.ServletContext;

-

+import org.apache.ki.mgt.SecurityManager;

+import org.apache.ki.web.servlet.KiFilter;

 import static org.easymock.EasyMock.*;

 import org.junit.Test;

-import org.apache.ki.web.servlet.KiFilter;

 import org.springframework.web.context.WebApplicationContext;

 

-import org.apache.ki.mgt.SecurityManager;

-import org.apache.ki.spring.SpringIniWebConfiguration;

-import org.apache.ki.spring.SpringKiFilter;

+import javax.servlet.FilterConfig;

+import javax.servlet.ServletContext;

+import java.util.HashMap;

+import java.util.Map;

 

 

 /**

diff --git a/support/spring/src/test/java/org/apache/ki/spring/remoting/SecureRemoteInvocationFactoryTest.java b/support/spring/src/test/java/org/apache/ki/spring/remoting/SecureRemoteInvocationFactoryTest.java
new file mode 100644
index 0000000..f484d70
--- /dev/null
+++ b/support/spring/src/test/java/org/apache/ki/spring/remoting/SecureRemoteInvocationFactoryTest.java
@@ -0,0 +1,103 @@
+package org.apache.ki.spring.remoting;
+
+import org.aopalliance.intercept.MethodInvocation;
+import org.apache.ki.session.mgt.SessionManager;
+import org.apache.ki.subject.Subject;
+import org.apache.ki.util.ThreadContext;
+import static org.easymock.EasyMock.*;
+import org.junit.After;
+import static org.junit.Assert.*;
+import org.junit.Before;
+import org.junit.Test;
+import org.springframework.remoting.support.RemoteInvocation;
+
+import java.lang.reflect.Method;
+import java.net.InetAddress;
+import java.util.UUID;
+
+/**
+ * //TODO - Class JavaDoc!
+ *
+ * @author Les Hazlewood
+ * @since Mar 28, 2009 4:14:01 PM
+ */
+public class SecureRemoteInvocationFactoryTest {
+
+    @Before
+    public void setup() {
+        ThreadContext.clear();
+    }
+
+    protected void bind( Subject subject ) {
+        ThreadContext.bind(subject);
+    }
+
+    @After
+    public void tearDown() {
+        ThreadContext.clear();
+    }
+
+    protected Method getMethod(String name, Class clazz) {
+        Method[] methods = clazz.getMethods();
+        for( Method method : methods ) {
+            if ( method.getName().equals(name) ) {
+                return method;
+            }
+        }
+        throw new IllegalStateException( "'" + name + "' method should exist." );
+    }
+
+    @Test
+    public void testSessionManagerProxyStartRemoteInvocation() throws Exception {
+
+        SecureRemoteInvocationFactory factory = new SecureRemoteInvocationFactory();
+
+        MethodInvocation mi = createMock(MethodInvocation.class);
+        Method startMethod = getMethod("start", SessionManager.class);
+        expect(mi.getMethod()).andReturn(startMethod).anyTimes();
+        
+        Object[] args = {InetAddress.getLocalHost()};
+        expect(mi.getArguments()).andReturn(args).anyTimes();
+
+        replay(mi);
+
+        RemoteInvocation ri = factory.createRemoteInvocation(mi);
+
+        verify(mi);
+
+        assertNull(ri.getAttribute(SecureRemoteInvocationFactory.SESSION_ID_KEY));
+    }
+
+    @Test
+    public void testSessionManagerProxyNonStartRemoteInvocation() throws Exception {
+
+        SecureRemoteInvocationFactory factory = new SecureRemoteInvocationFactory();
+
+        MethodInvocation mi = createMock(MethodInvocation.class);
+        Method method = getMethod("isValid", SessionManager.class);
+        expect(mi.getMethod()).andReturn(method).anyTimes();
+
+        String dummySessionId = UUID.randomUUID().toString();
+        Object[] args = {dummySessionId};
+        expect(mi.getArguments()).andReturn(args).anyTimes();
+
+        replay(mi);
+
+        RemoteInvocation ri = factory.createRemoteInvocation(mi);
+
+        verify(mi);
+
+        assertEquals(ri.getAttribute(SecureRemoteInvocationFactory.SESSION_ID_KEY), dummySessionId);
+    }
+
+    /*@Test
+    public void testNonSessionManagerCall() throws Exception {
+
+        SecureRemoteInvocationFactory factory = new SecureRemoteInvocationFactory();
+
+        MethodInvocation mi = createMock(MethodInvocation.class);
+        Method method = getMethod("login", SecurityManager.class);
+        expect(mi.getMethod()).andReturn(method).anyTimes();
+    }*/
+
+}