From 1d89bef7a25626f97b306c1a1b61297dd20e5be8 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 20 Aug 2012 10:21:07 +1000 Subject: [PATCH] jetty-9 fixed context restart for constraint test --- .../security/ConstraintSecurityHandler.java | 4 +- .../authentication/LoginAuthenticator.java | 19 ++++--- .../jetty/security/ConstraintTest.java | 44 +++------------ .../jetty/server/handler/ContextHandler.java | 56 ++++++++----------- .../jetty/server/LocalConnectorTest.java | 15 +++++ 5 files changed, 58 insertions(+), 80 deletions(-) diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java b/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java index ebd441616b1..d323fce2a72 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java @@ -236,10 +236,8 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr @Override protected void doStop() throws Exception { - _constraintMap.clear(); - _constraintMappings.clear(); - _roles.clear(); super.doStop(); + _constraintMap.clear(); } protected void processConstraintMapping(ConstraintMapping mapping) 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 aefd171814f..e5d033ae0f9 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 @@ -74,17 +74,20 @@ public abstract class LoginAuthenticator implements Authenticator { HttpSession httpSession = request.getSession(false); - synchronized (httpSession) + if (_renewSession && httpSession!=null) { - //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) { - HttpSession newSession = AbstractSessionManager.renewSession(request, httpSession,true); - LOG.debug("renew {}->{}",httpSession.getId(),newSession.getId()); - httpSession=newSession; + //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 (httpSession.getAttribute(AbstractSessionManager.SESSION_KNOWN_ONLY_TO_AUTHENTICATED)!=Boolean.TRUE) + { + HttpSession newSession = AbstractSessionManager.renewSession(request, httpSession,true); + LOG.debug("renew {}->{}",httpSession.getId(),newSession.getId()); + httpSession=newSession; + } } - return httpSession; } + return httpSession; } } 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 973fbf0f016..df47eaeeab9 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 @@ -305,10 +305,6 @@ 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(); @@ -783,9 +779,8 @@ public class ConstraintTest _security.setHandler(check); _security.setAuthenticator(new BasicAuthenticator()); _security.setStrict(false); - _server.start(); - System.out.println(_server.dump()); + _server.start(); String response; response = _connector.getResponses("GET /ctx/noauth/info HTTP/1.0\r\n\r\n", 100000, TimeUnit.MILLISECONDS); @@ -798,40 +793,12 @@ public class ConstraintTest _server.stop(); - /* - * FIXME: this seems to indicate there is an issue with the way the server is stopping and starting now - * - * Note that ConstraintSecurityHandler loses all of its brains when the server starts and stops, but that - * change was made in 2/2011 and this wasn't exposed til now, which seems to indicate that previously - * when the server stopped that doStop() didn't make it down to the constraint handler...and now it does. - * - * also, seems to be an issue in local connector, I had to add a new one for it to be able to work here as well - * so issues in stop/start there as well - */ - - _connector = new LocalConnector(_server); - _server.setConnectors(new Connector[]{_connector}); - ContextHandler _context = new ContextHandler(); - SessionHandler _session = new SessionHandler(); - - _context.setContextPath("/ctx"); - _server.setHandler(_context); - _context.setHandler(_session); - - _security = new ConstraintSecurityHandler(); - _session.setHandler(_security); - RequestHandler _handler = new RequestHandler(); - _security.setHandler(_handler); - RoleRefHandler roleref = new RoleRefHandler(); + roleref.setHandler(_security.getHandler()); _security.setHandler(roleref); roleref.setHandler(check); - - _security.setConstraintMappings(getConstraintMappings(),getKnownRoles()); - - _server.start(); - System.out.println(_server.dump()); + _server.start(); response = _connector.getResponses("GET /ctx/auth/info HTTP/1.0\r\n" + "Authorization: Basic " + B64Code.encode("user2:password") + "\r\n" + @@ -885,6 +852,7 @@ public class ConstraintTest } private class RequestHandler extends AbstractHandler { + @Override public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response ) throws IOException, ServletException { baseRequest.setHandled(true); @@ -916,16 +884,19 @@ public class ConstraintTest UserIdentity.Scope scope = new UserIdentity.Scope() { + @Override public String getContextPath() { return "/"; } + @Override public String getName() { return "someServlet"; } + @Override public Map getRoleRefMap() { Map map = new HashMap<>(); @@ -949,6 +920,7 @@ public class ConstraintTest private class RoleCheckHandler extends AbstractHandler { + @Override public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response ) throws IOException, ServletException { ((Request) request).setHandled(true); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 15c02c9d2a7..4592964427d 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -164,13 +164,9 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server. private Object _requestAttributeListeners; private Map _managedAttributes; private String[] _protectedTargets; - - private boolean _shutdown = false; - - private boolean _available = true; - private volatile int _availability; // 0=STOPPED, 1=AVAILABLE, 2=SHUTDOWN, 3=UNAVAILABLE - - private final static int __STOPPED = 0, __AVAILABLE = 1, __SHUTDOWN = 2, __UNAVAILABLE = 3; + + public enum Availability { AVAILABLE,SHUTDOWN,UNAVAILABLE}; + private volatile Availability _availability; /* ------------------------------------------------------------ */ /** @@ -629,12 +625,15 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server. /** * @return true if this context is accepting new requests */ - @ManagedAttribute("false if this context is accepting new requests. true for graceful shutdown, which allows existing requests to complete") + @ManagedAttribute("true for graceful shutdown, which allows existing requests to complete") public boolean isShutdown() { - synchronized (this) + switch(_availability) { - return !_shutdown; + case SHUTDOWN: + return true; + default: + return false; } } @@ -644,13 +643,10 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server. * requests can complete, but no new requests are accepted. * */ + @Override public void shutdown() { - synchronized (this) - { - _shutdown = true; - _availability = isRunning() ? __SHUTDOWN : __STOPPED; - } + _availability = isRunning() ? Availability.SHUTDOWN : Availability.UNAVAILABLE; } /* ------------------------------------------------------------ */ @@ -659,10 +655,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server. */ public boolean isAvailable() { - synchronized (this) - { - return _available; - } + return _availability==Availability.AVAILABLE; } /* ------------------------------------------------------------ */ @@ -673,8 +666,10 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server. { synchronized (this) { - _available = available; - _availability = isRunning()?(_shutdown?__SHUTDOWN:_available?__AVAILABLE:__UNAVAILABLE):__STOPPED; + if (available && isRunning()) + _availability = Availability.AVAILABLE; + else if (!available || !isRunning()) + _availability = Availability.UNAVAILABLE; } } @@ -697,7 +692,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server. @Override protected void doStart() throws Exception { - _availability = __STOPPED; + _availability = Availability.UNAVAILABLE; if (_contextPath == null) throw new IllegalStateException("Null contextPath"); @@ -726,10 +721,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server. // defers the calling of super.doStart() startContext(); - synchronized(this) - { - _availability = _shutdown?__SHUTDOWN:_available?__AVAILABLE:__UNAVAILABLE; - } + _availability = Availability.AVAILABLE; } finally { @@ -806,7 +798,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server. @Override protected void doStop() throws Exception { - _availability = __STOPPED; + _availability = Availability.UNAVAILABLE; ClassLoader old_classloader = null; Thread current_thread = null; @@ -867,10 +859,8 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server. switch (_availability) { - case __STOPPED: - case __SHUTDOWN: - return false; - case __UNAVAILABLE: + case UNAVAILABLE: + case SHUTDOWN: baseRequest.setHandled(true); response.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE); return false; @@ -1517,8 +1507,8 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server. b.append(s.charAt(0)).append('.'); } } - b.append(getClass().getSimpleName()); - b.append('{').append(getContextPath()).append(',').append(getBaseResource()); + b.append(getClass().getSimpleName()).append('@').append(Integer.toString(hashCode(),16)); + b.append('{').append(getContextPath()).append(',').append(getBaseResource()).append(',').append(_availability); if (vhosts != null && vhosts.length > 0) b.append(',').append(vhosts[0]); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/LocalConnectorTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/LocalConnectorTest.java index 0f8bcbdd360..0cad00e388a 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/LocalConnectorTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/LocalConnectorTest.java @@ -58,6 +58,21 @@ public class LocalConnectorTest assertThat(response,containsString("pathInfo=/R1")); } + @Test + public void testStopStart() throws Exception + { + String response=_connector.getResponses("GET /R1 HTTP/1.0\r\n\r\n"); + assertThat(response,containsString("HTTP/1.1 200 OK")); + assertThat(response,containsString("pathInfo=/R1")); + + _server.stop(); + _server.start(); + + response=_connector.getResponses("GET /R2 HTTP/1.0\r\n\r\n"); + assertThat(response,containsString("HTTP/1.1 200 OK")); + assertThat(response,containsString("pathInfo=/R2")); + } + @Test public void testTwoGETs() throws Exception {