jetty-9 removed redirect race from form auth

5 files changed, 109 insertions, 43 deletions

diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java b/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java
index c1493c897e..57587e12b4 100644
--- a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java
+++ b/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java
@@ -43,6 +43,7 @@ import org.eclipse.jetty.server.Authentication.User;
 import org.eclipse.jetty.server.HttpChannel;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.UserIdentity;
+import org.eclipse.jetty.server.session.HashedSession;
 import org.eclipse.jetty.util.MultiMap;
 import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.URIUtil;
@@ -200,6 +201,7 @@ public class FormAuthenticator extends LoginAuthenticator
             return _deferred;
 
         HttpSession session = request.getSession(true);
+        System.err.println(session.getId()+((HashedSession)session).getAttributeMap());
 
         try
         {
@@ -208,31 +210,36 @@ public class FormAuthenticator extends LoginAuthenticator
             {
                 final String username = request.getParameter(__J_USERNAME);
                 final String password = request.getParameter(__J_PASSWORD);
-
+                
                 UserIdentity user = _loginService.login(username,password);
+                LOG.debug("jsecuritycheck {} {}",username,user);
                 if (user!=null)
                 {
                     session=renewSession(request,response);
 
                     // Redirect to original request
                     String nuri;
+                    FormAuthentication form_auth;
                     synchronized(session)
                     {
                         nuri = (String) session.getAttribute(__J_URI);
-                    }
 
-                    if (nuri == null || nuri.length() == 0)
-                    {
-                        nuri = request.getContextPath();
-                        if (nuri.length() == 0)
-                            nuri = URIUtil.SLASH;
+                        if (nuri == null || nuri.length() == 0)
+                        {
+                            nuri = request.getContextPath();
+                            if (nuri.length() == 0)
+                                nuri = URIUtil.SLASH;
+                        }
+
+                        Authentication cached=new SessionAuthentication(getAuthMethod(),user,password);
+                        session.setAttribute(SessionAuthentication.__J_AUTHENTICATED, cached);
+                        form_auth = new FormAuthentication(getAuthMethod(),user);
                     }
+                    LOG.debug("authenticated {}->{}",form_auth,nuri);
+
                     response.setContentLength(0);
                     response.sendRedirect(response.encodeRedirectURL(nuri));
-
-                    Authentication cached=new SessionAuthentication(getAuthMethod(),user,password);
-                    session.setAttribute(SessionAuthentication.__J_AUTHENTICATED, cached);
-                    return new FormAuthentication(getAuthMethod(),user);
+                    return form_auth;
                 }
 
                 // not authenticated
@@ -240,11 +247,13 @@ public class FormAuthenticator extends LoginAuthenticator
                     LOG.debug("Form authentication FAILED for " + StringUtil.printable(username));
                 if (_formErrorPage == null)
                 {
+                    LOG.debug("auth failed {}->403",username);
                     if (response != null)
                         response.sendError(HttpServletResponse.SC_FORBIDDEN);
                 }
                 else if (_dispatch)
                 {
+                    LOG.debug("auth failed {}=={}",username,_formErrorPage);
                     RequestDispatcher dispatcher = request.getRequestDispatcher(_formErrorPage);
                     response.setHeader(HttpHeader.CACHE_CONTROL.asString(),HttpHeaderValue.NO_CACHE.asString());
                     response.setDateHeader(HttpHeader.EXPIRES.asString(),1);
@@ -252,6 +261,7 @@ public class FormAuthenticator extends LoginAuthenticator
                 }
                 else
                 {
+                    LOG.debug("auth failed {}->{}",username,_formErrorPage);
                     response.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(),_formErrorPage)));
                 }
 
@@ -267,43 +277,51 @@ public class FormAuthenticator extends LoginAuthenticator
                     _loginService!=null &&
                     !_loginService.validate(((Authentication.User)authentication).getUserIdentity()))
                 {
-
+                    LOG.debug("auth revoked {}",authentication);
                     session.removeAttribute(SessionAuthentication.__J_AUTHENTICATED);
                 }
                 else
                 {
-                    String j_uri=(String)session.getAttribute(__J_URI);
-                    if (j_uri!=null)
+                    synchronized (session)
                     {
-                        MultiMap j_post = (MultiMap)session.getAttribute(__J_POST);
-                        if (j_post!=null)
+                        String j_uri=(String)session.getAttribute(__J_URI);
+                        if (j_uri!=null)
                         {
-                            StringBuffer buf = request.getRequestURL();
-                            if (request.getQueryString() != null)
-                                buf.append("?").append(request.getQueryString());
-
-                            if (j_uri.equals(buf.toString()))
+                            LOG.debug("auth retry {}->{}",authentication,j_uri);
+                            MultiMap<String> j_post = (MultiMap<String>)session.getAttribute(__J_POST);
+                            if (j_post!=null)
                             {
-                                // This is a retry of an original POST request
-                                // so restore method and parameters
-
-                                session.removeAttribute(__J_POST);
-                                Request base_request = HttpChannel.getCurrentHttpChannel().getRequest();
-                                base_request.setMethod(HttpMethod.POST,HttpMethod.POST.asString());
-                                base_request.setParameters(j_post);
+                                LOG.debug("auth rePOST {}->{}",authentication,j_uri);
+                                StringBuffer buf = request.getRequestURL();
+                                if (request.getQueryString() != null)
+                                    buf.append("?").append(request.getQueryString());
+
+                                if (j_uri.equals(buf.toString()))
+                                {
+                                    // This is a retry of an original POST request
+                                    // so restore method and parameters
+
+                                    session.removeAttribute(__J_POST);
+                                    Request base_request = HttpChannel.getCurrentHttpChannel().getRequest();
+                                    base_request.setMethod(HttpMethod.POST,HttpMethod.POST.asString());
+                                    base_request.setParameters(j_post);
+                                }
                             }
+                            else
+                                session.removeAttribute(__J_URI);
                         }
-                        else
-                            session.removeAttribute(__J_URI);
-
                     }
+                    LOG.debug("auth {}",authentication);
                     return authentication;
                 }
             }
 
             // if we can't send challenge
             if (DeferredAuthentication.isDeferred(response))
+            {
+                LOG.debug("auth deferred {}",session.getId());
                 return Authentication.UNAUTHENTICATED;
+            }
 
             // remember the current URI
             synchronized (session)
@@ -320,7 +338,7 @@ public class FormAuthenticator extends LoginAuthenticator
                     {
                         Request base_request = (req instanceof Request)?(Request)req:HttpChannel.getCurrentHttpChannel().getRequest();
                         base_request.extractParameters();
-                        session.setAttribute(__J_POST, new MultiMap(base_request.getParameters()));
+                        session.setAttribute(__J_POST, new MultiMap<String>(base_request.getParameters()));
                     }
                 }
             }
@@ -328,6 +346,7 @@ public class FormAuthenticator extends LoginAuthenticator
             // send the the challenge
             if (_dispatch)
             {
+                LOG.debug("challenge {}=={}",session.getId(),_formLoginPage);
                 RequestDispatcher dispatcher = request.getRequestDispatcher(_formLoginPage);
                 response.setHeader(HttpHeader.CACHE_CONTROL.asString(),HttpHeaderValue.NO_CACHE.asString());
                 response.setDateHeader(HttpHeader.EXPIRES.asString(),1);
@@ -335,11 +354,10 @@ public class FormAuthenticator extends LoginAuthenticator
             }
             else
             {
+                LOG.debug("challenge {}->{}",session.getId(),_formLoginPage);
                 response.sendRedirect(response.encodeRedirectURL(URIUtil.addPaths(request.getContextPath(),_formLoginPage)));
             }
             return Authentication.SEND_CONTINUE;
-
-
         }
         catch (IOException | ServletException e)
         {
diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/LoginAuthenticator.java b/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/LoginAuthenticator.java
index f7e30647a9..a1f0501f78 100644
--- a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/LoginAuthenticator.java
+++ b/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/LoginAuthenticator.java
@@ -26,9 +26,15 @@ import org.eclipse.jetty.security.Authenticator;
 import org.eclipse.jetty.security.IdentityService;
 import org.eclipse.jetty.security.LoginService;
 import org.eclipse.jetty.server.session.AbstractSessionManager;
+import org.eclipse.jetty.server.session.HashSessionManager;
+import org.eclipse.jetty.server.session.HashedSession;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.Logger;
 
 public abstract class LoginAuthenticator implements Authenticator
-{
+{            
+    private static final Logger LOG = Log.getLogger(LoginAuthenticator.class);
+
     protected final DeferredAuthentication _deferred=new DeferredAuthentication(this);
     protected LoginService _loginService;
     protected IdentityService _identityService;
@@ -67,16 +73,20 @@ public abstract class LoginAuthenticator implements Authenticator
     protected HttpSession renewSession(HttpServletRequest request, HttpServletResponse response)
     {
         HttpSession httpSession = request.getSession(false);
-       
-        //if we should renew sessions, and there is an existing session that may have been seen by non-authenticated users
-        //(indicated by SESSION_SECURED not being set on the session) then we should change id
-        if (_renewSession && httpSession!=null && httpSession.getAttribute(AbstractSessionManager.SESSION_KNOWN_ONLY_TO_AUTHENTICATED)!=Boolean.TRUE)
+
+        synchronized (httpSession)
         {
-            synchronized (this)
+            //if we should renew sessions, and there is an existing session that may have been seen by non-authenticated users
+            //(indicated by SESSION_SECURED not being set on the session) then we should change id
+            if (_renewSession && httpSession!=null && httpSession.getAttribute(AbstractSessionManager.SESSION_KNOWN_ONLY_TO_AUTHENTICATED)!=Boolean.TRUE)
             {
-                httpSession = AbstractSessionManager.renewSession(request, httpSession,true);
+                System.err.println(((HashedSession)httpSession).getAttributeMap());
+                HttpSession newSession = AbstractSessionManager.renewSession(request, httpSession,true);
+                LOG.debug("renew {}->{}",httpSession.getId(),newSession.getId());
+                System.err.println(((HashedSession)newSession).getAttributeMap());
+                httpSession=newSession;
             }
+            return httpSession;
         }
-        return httpSession;
     }
 }
diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/SessionAuthentication.java b/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/SessionAuthentication.java
index 74f143b05f..4aa063f8a6 100644
--- a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/SessionAuthentication.java
+++ b/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/SessionAuthentication.java
@@ -114,23 +114,27 @@ public class SessionAuthentication implements Authentication.User, Serializable,
     @Override
     public String toString()
     {
-        return "Session"+super.toString();
+        return String.format("%s@%x{%s,%s}",this.getClass().getSimpleName(),hashCode(),_session==null?"-":_session.getId(),_userIdentity);
     }
 
+    @Override
     public void sessionWillPassivate(HttpSessionEvent se)
     {
     }
 
+    @Override
     public void sessionDidActivate(HttpSessionEvent se)
     {
         if (_session==null)
             _session=se.getSession();
     }
 
+    @Override
     public void valueBound(HttpSessionBindingEvent event)
     {
     }
 
+    @Override
     public void valueUnbound(HttpSessionBindingEvent event)
     {
         doLogout();
diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java
index 019b8736e2..11eabb3779 100644
--- a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java
+++ b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java
@@ -34,6 +34,7 @@ import javax.servlet.http.HttpServletResponse;
 
 import org.eclipse.jetty.security.authentication.BasicAuthenticator;
 import org.eclipse.jetty.security.authentication.FormAuthenticator;
+import org.eclipse.jetty.security.authentication.LoginAuthenticator;
 import org.eclipse.jetty.server.Connector;
 import org.eclipse.jetty.server.LocalConnector;
 import org.eclipse.jetty.server.Request;
@@ -44,6 +45,7 @@ import org.eclipse.jetty.server.handler.ContextHandler;
 import org.eclipse.jetty.server.handler.HandlerWrapper;
 import org.eclipse.jetty.server.session.SessionHandler;
 import org.eclipse.jetty.util.B64Code;
+import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.security.Constraint;
 import org.eclipse.jetty.util.security.Password;
 import org.junit.After;
@@ -303,6 +305,10 @@ public class ConstraintTest
     @Test
     public void testFormRedirect() throws Exception
     {
+        Log.getLogger(SecurityHandler.class).setDebugEnabled(true);
+        Log.getLogger(LoginAuthenticator.class).setDebugEnabled(true);
+        Log.getLogger(FormAuthenticator.class).setDebugEnabled(true);
+        
         _security.setAuthenticator(new FormAuthenticator("/testLoginPage","/testErrorPage",false));
         _security.setStrict(false);
         _server.start();
@@ -326,6 +332,7 @@ public class ConstraintTest
         assertThat(response,containsString(" 200 OK"));
         assertThat(response,containsString("URI=/ctx/testLoginPage"));
 
+        System.err.println("-- wrong password");
         response = _connector.getResponses("POST /ctx/j_security_check HTTP/1.0\r\n" +
                 "Cookie: JSESSIONID=" + session + "\r\n" +
                 "Content-Type: application/x-www-form-urlencoded\r\n" +
@@ -334,6 +341,7 @@ public class ConstraintTest
                 "j_username=user&j_password=wrong");
         assertThat(response,containsString("Location"));
 
+        System.err.println("-- right password");
         response = _connector.getResponses("POST /ctx/j_security_check HTTP/1.0\r\n" +
                 "Cookie: JSESSIONID=" + session + "\r\n" +
                 "Content-Type: application/x-www-form-urlencoded\r\n" +
@@ -345,9 +353,11 @@ public class ConstraintTest
         assertThat(response,containsString("/ctx/auth/info"));
         session = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf(";Path=/ctx"));
 
+        System.err.println("--");
         response = _connector.getResponses("GET /ctx/auth/info HTTP/1.0\r\n" +
                 "Cookie: JSESSIONID=" + session + "\r\n" +
                 "\r\n");
+        System.err.println("==");
         assertThat(response,startsWith("HTTP/1.1 200 OK"));
 
         response = _connector.getResponses("GET /ctx/admin/info HTTP/1.0\r\n" +
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSession.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSession.java
index d3dab5fd14..8c2278fc4b 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSession.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSession.java
@@ -111,6 +111,7 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
     }
     
     /* ------------------------------------------------------------- */
+    @Override
     public AbstractSession getSession()
     {
         return this;
@@ -124,8 +125,15 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
             return _accessed;
         }
     }
+
+    /* ------------------------------------------------------------- */
+    public Map<String,Object> getAttributeMap()
+    {
+        return _attributes;
+    }
     
     /* ------------------------------------------------------------ */
+    @Override
     public Object getAttribute(String name)
     {
         synchronized (this)
@@ -147,6 +155,7 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
 
     /* ------------------------------------------------------------ */
     @SuppressWarnings({ "unchecked" })
+    @Override
     public Enumeration<String> getAttributeNames()
     {
         synchronized (this)
@@ -173,12 +182,14 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
     }
 
     /* ------------------------------------------------------------- */
+    @Override
     public long getCreationTime() throws IllegalStateException
     {
         return _created;
     }
 
     /* ------------------------------------------------------------ */
+    @Override
     public String getId() throws IllegalStateException
     {
         return _manager._nodeIdInSessionId?_nodeId:_clusterId;
@@ -197,6 +208,7 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
     }
 
     /* ------------------------------------------------------------- */
+    @Override
     public long getLastAccessedTime() throws IllegalStateException
     {
         checkValid();
@@ -204,6 +216,7 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
     }
 
     /* ------------------------------------------------------------- */
+    @Override
     public int getMaxInactiveInterval()
     {
         checkValid();
@@ -214,6 +227,7 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
     /*
      * @see javax.servlet.http.HttpSession#getServletContext()
      */
+    @Override
     public ServletContext getServletContext()
     {
         return _manager._context;
@@ -221,6 +235,7 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
 
     /* ------------------------------------------------------------- */
     @Deprecated
+    @Override
     public HttpSessionContext getSessionContext() throws IllegalStateException
     {
         checkValid();
@@ -233,6 +248,7 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
      *             {@link #getAttribute}
      */
     @Deprecated
+    @Override
     public Object getValue(String name) throws IllegalStateException
     {
         return getAttribute(name);
@@ -244,6 +260,7 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
      *             {@link #getAttributeNames}
      */
     @Deprecated
+    @Override
     public String[] getValueNames() throws IllegalStateException
     {
         synchronized(this)
@@ -309,6 +326,7 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
     }
 
     /* ------------------------------------------------------------- */
+    @Override
     public void invalidate() throws IllegalStateException
     {
         // remove session from context and invalidate other sessions with same ID.
@@ -372,6 +390,7 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
     }
 
     /* ------------------------------------------------------------- */
+    @Override
     public boolean isNew() throws IllegalStateException
     {
         checkValid();
@@ -384,12 +403,14 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
      *             {@link #setAttribute}
      */
     @Deprecated
+    @Override
     public void putValue(java.lang.String name, java.lang.Object value) throws IllegalStateException
     {
         setAttribute(name,value);
     }
 
     /* ------------------------------------------------------------ */
+    @Override
     public void removeAttribute(String name)
     {
         setAttribute(name,null);
@@ -401,6 +422,7 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
      *             {@link #removeAttribute}
      */
     @Deprecated
+    @Override
     public void removeValue(java.lang.String name) throws IllegalStateException
     {
         removeAttribute(name);
@@ -419,6 +441,7 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
     }
     
     /* ------------------------------------------------------------ */
+    @Override
     public void setAttribute(String name, Object value)
     {
         Object old=null;
@@ -447,6 +470,7 @@ public abstract class AbstractSession implements AbstractSessionManager.SessionI
     }
 
     /* ------------------------------------------------------------- */
+    @Override
     public void setMaxInactiveInterval(int secs)
     {
         _maxIdleMs=(long)secs*1000L;