Various cleanups of Handler.insertHandler (#10792)

* Various cleanups of Handler.insertHandler * Added missing call to relinkHandlers() in setHandler() after calling super. * Moved call to relinkHandlers() in insertHandler(), as the various setSession|Security|ServletHandler() already call relinkHandlers(). Signed-off-by: Simone Bordet <[email protected]> --------- Signed-off-by: Simone Bordet <[email protected]> Co-authored-by: Simone Bordet <[email protected]>
jetty · Nov 13, 2023 · 49b3442 · 49b3442
1 parent 2c35f5a
commit 49b3442
Show file tree

Hide file tree

Showing 12 changed files with 258 additions and 91 deletions.
diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Handler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Handler.java
@@ -325,8 +325,8 @@ default List<Handler> getHandlers()
         }
 
         /**
-         * <p>Inserts the given {@code Handler} (and its chain of {@code Handler}s)
-         * in front of the child of this {@code Handler}.</p>
+         * <p>Inserts the given {@code Handler} (and possible chain of {@code Handler}s)
+         * between this {@code Handler} and its current {@link #getHandler() child}.
          * <p>For example, if this {@code Handler} {@code A} has a child {@code B},
          * inserting {@code Handler} {@code X} built as a chain {@code Handler}s
          * {@code X-Y-Z} results in the structure {@code A-X-Y-Z-B}.</p>
@@ -335,11 +335,7 @@ default List<Handler> getHandlers()
          */
         default void insertHandler(Singleton handler)
         {
-            Singleton tail = handler;
-            while (tail.getHandler() instanceof Wrapper)
-            {
-                tail = (Wrapper)tail.getHandler();
-            }
+            Singleton tail = handler.getTail();
             if (tail.getHandler() != null)
                 throw new IllegalArgumentException("bad tail of inserted wrapper chain");
 

diff --git a/...-ee10/jetty-ee10-jaspi/src/test/java/org/eclipse/jetty/ee10/security/jaspi/JaspiTest.java b/...-ee10/jetty-ee10-jaspi/src/test/java/org/eclipse/jetty/ee10/security/jaspi/JaspiTest.java
@@ -135,7 +135,7 @@ public void before() throws Exception
         JaspiAuthenticatorFactory jaspiAuthFactory = new JaspiAuthenticatorFactory();
 
         ConstraintSecurityHandler security = new ConstraintSecurityHandler();
-        context.setHandler(security);
+        context.setSecurityHandler(security);
         security.setAuthenticatorFactory(jaspiAuthFactory);
 
         Constraint constraint = new Constraint.Builder()
@@ -152,7 +152,7 @@ public void before() throws Exception
         other.setContextPath("/other");
         other.addServlet(new TestServlet(), "/");
         ConstraintSecurityHandler securityOther = new ConstraintSecurityHandler();
-        other.setHandler(securityOther);
+        other.setSecurityHandler(securityOther);
         securityOther.setAuthenticatorFactory(jaspiAuthFactory);
         securityOther.addConstraintMapping(mapping);
 

diff --git a/...etty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java b/...etty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java
@@ -953,6 +953,7 @@ else if (handler instanceof ServletHandler)
             if (handler != null)
                 LOG.warn("ServletContextHandler.setHandler should not be called directly. Use insertHandler or setSessionHandler etc.");
             super.setHandler(handler);
+            relinkHandlers();
         }
     }
 
@@ -1128,18 +1129,18 @@ protected ServletContextRequest newServletContextRequest(ServletChannel servletC
     }
 
     @Override
-    protected ServletContextRequest wrapRequest(Request request, Response response)
+    protected ContextRequest wrapRequest(Request request, Response response)
     {
         // Need to ask directly to the Context for the pathInContext, rather than using
         // Request.getPathInContext(), as the request is not yet wrapped in this Context.
         String decodedPathInContext = URIUtil.decodePath(getContext().getPathInContext(request.getHttpURI().getCanonicalPath()));
 
         MatchedResource<ServletHandler.MappedServlet> matchedResource = _servletHandler.getMatchedServlet(decodedPathInContext);
         if (matchedResource == null)
-            return null;
+            return wrapNoServlet(request, response);
         ServletHandler.MappedServlet mappedServlet = matchedResource.getResource();
         if (mappedServlet == null)
-            return null;
+            return wrapNoServlet(request, response);
 
         // Get a servlet request, possibly from a cached version in the channel attributes.
         Attributes cache = request.getComponents().getCache();
@@ -1161,12 +1162,20 @@ protected ServletContextRequest wrapRequest(Request request, Response response)
         return servletContextRequest;
     }
 
+    private ContextRequest wrapNoServlet(Request request, Response response)
+    {
+        Handler next = getServletHandler().getHandler();
+        if (next == null)
+            return null;
+        return super.wrapRequest(request, response);
+    }
+
     @Override
     protected ContextResponse wrapResponse(ContextRequest request, Response response)
     {
         if (request instanceof ServletContextRequest servletContextRequest)
             return servletContextRequest.getServletContextResponse();
-        throw new IllegalArgumentException();
+        return super.wrapResponse(request, response);
     }
 
     @Override
@@ -1662,26 +1671,17 @@ else if (handler instanceof ServletHandler)
             setServletHandler((ServletHandler)handler);
         else
         {
+            // We cannot call super.insertHandler here, because it uses this.setHandler
+            // which sets the servletHandlers next handler.
+            // This is the same insert code, but uses super.setHandler, which sets this
+            // handler's next handler.
             Singleton tail = handler.getTail();
             if (tail.getHandler() != null)
                 throw new IllegalArgumentException("bad tail of inserted wrapper chain");
-
-            // Skip any injected handlers
-            Singleton h = this;
-            while (h.getHandler() instanceof Singleton wrapper)
-            {
-                if (wrapper instanceof SessionHandler ||
-                    wrapper instanceof SecurityHandler ||
-                    wrapper instanceof ServletHandler)
-                    break;
-                h = wrapper;
-            }
-
-            Handler next = h.getHandler();
-            doSetHandler(h, handler);
-            doSetHandler(tail, next);
+            tail.setHandler(getHandler());
+            super.setHandler(handler);
+            relinkHandlers();
         }
-        relinkHandlers();
     }
 
     /**

diff --git a/...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHandler.java b/...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHandler.java
@@ -452,16 +452,21 @@ protected IdentityService getIdentityService()
     @Override
     public boolean handle(Request request, Response response, Callback callback) throws Exception
     {
-        // We will always have a ServletContextRequest as we must be within a ServletContextHandler
+        // We have a ServletContextRequest only when an enclosing ServletContextHandler matched a Servlet
         ServletChannel servletChannel = Request.get(request, ServletContextRequest.class, ServletContextRequest::getServletChannel);
+        if (servletChannel != null)
+        {
+            if (LOG.isDebugEnabled())
+                LOG.debug("handle {} {} {} {} {}", this, servletChannel, request, response, callback);
 
-        if (LOG.isDebugEnabled())
-            LOG.debug("handle {} {} {} {}", this, request, response, callback);
+            // But request, response and/or callback may have been wrapped after the ServletContextHandler, so update the channel.
+            servletChannel.associate(request, response, callback);
+            servletChannel.handle();
+            return true;
+        }
 
-        // But request, response and/or callback may have been wrapped after the ServletContextHandler, so update the channel.
-        servletChannel.associate(request, response, callback);
-        servletChannel.handle();
-        return true;
+        // Otherwise, there is no matching servlet so we pass to our next handler (if any)
+        return super.handle(request, response, callback);
     }
 
     /**

diff --git a/...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/SessionHandler.java b/...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/SessionHandler.java
@@ -386,7 +386,17 @@ public SessionHandler()
     @Override
     public ManagedSession getManagedSession(Request request)
     {
-        return Request.as(request, ServletContextRequest.class).getManagedSession();
+        ServletContextRequest servletContextRequest = Request.as(request, ServletContextRequest.class);
+        if (servletContextRequest != null)
+            return servletContextRequest.getManagedSession();
+
+        NonServletSessionRequest nonServletSessionRequest = Request.as(request, NonServletSessionRequest.class);
+        if (nonServletSessionRequest != null)
+            return nonServletSessionRequest.getManagedSession();
+
+        if (request.getSession(false) instanceof ManagedSession managedSession)
+            return managedSession;
+        return null;
     }
 
     /**
@@ -674,21 +684,61 @@ public boolean handle(Request request, Response response, Callback callback) thr
         if (next == null)
             return false;
 
-        ServletContextRequest servletContextRequest = Request.as(request, ServletContextRequest.class);
         addSessionStreamWrapper(request);
 
         // find and set the session if one exists
         RequestedSession requestedSession = resolveRequestedSessionId(request);
-        servletContextRequest.setRequestedSession(requestedSession);
+
+        ServletContextRequest servletContextRequest = Request.as(request, ServletContextRequest.class);
+        if (servletContextRequest == null)
+            request = new NonServletSessionRequest(request, response, requestedSession);
+        else
+            servletContextRequest.setRequestedSession(requestedSession);
 
         // Handle changed ID or max-age refresh, but only if this is not a redispatched request
         HttpCookie cookie = access(requestedSession.session(), request.getConnectionMetaData().isSecure());
         if (cookie != null)
+            Response.putCookie(response, cookie);
+
+        return next.handle(request, response, callback);
+    }
+
+    private class NonServletSessionRequest extends Request.Wrapper
+    {
+        private final Response _response;
+        private RequestedSession _session;
+
+        public NonServletSessionRequest(Request request, Response response, RequestedSession requestedSession)
         {
-            ServletContextResponse servletContextResponse = servletContextRequest.getServletContextResponse();
-            Response.putCookie(servletContextResponse, cookie);
+            super(request);
+            _response = response;
+            _session = requestedSession;
         }
 
-        return next.handle(request, response, callback);
+        @Override
+        public Session getSession(boolean create)
+        {
+            ManagedSession session = _session.session();
+
+            if (session != null || !create)
+                return session;
+
+            newSession(getWrapped(), _session.sessionId(), ms ->
+                _session = new RequestedSession(ms, _session.sessionId(), true));
+
+            session = _session.session();
+            if (session == null)
+                throw new IllegalStateException("Create session failed");
+
+            HttpCookie cookie = getSessionCookie(session, isSecure());
+            if (cookie != null)
+                Response.replaceCookie(_response, cookie);
+            return session;
+        }
+
+        ManagedSession getManagedSession()
+        {
+            return _session.session();
+        }
     }
 }
diff --git a/...vlet/src/main/java/org/eclipse/jetty/ee10/servlet/security/ConstraintSecurityHandler.java b/...vlet/src/main/java/org/eclipse/jetty/ee10/servlet/security/ConstraintSecurityHandler.java
@@ -33,8 +33,6 @@
 import jakarta.servlet.annotation.ServletSecurity.EmptyRoleSemantic;
 import jakarta.servlet.annotation.ServletSecurity.TransportGuarantee;
 import org.eclipse.jetty.ee10.servlet.ServletContextHandler;
-import org.eclipse.jetty.ee10.servlet.security.ConstraintAware;
-import org.eclipse.jetty.ee10.servlet.security.ConstraintMapping;
 import org.eclipse.jetty.http.pathmap.MappedResource;
 import org.eclipse.jetty.http.pathmap.MatchedResource;
 import org.eclipse.jetty.http.pathmap.PathMappings;
@@ -186,7 +184,7 @@ public static List<ConstraintMapping> removeConstraintMappingsForPath(String pat
     }
 
     /**
-     * Generate Constraints and ContraintMappings for the given url pattern and ServletSecurityElement
+     * Generate Constraints and ConstraintMappings for the given url pattern and ServletSecurityElement
      *
      * @param name the name
      * @param pathSpec the path spec

diff --git a/...-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletContextHandlerTest.java b/...-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletContextHandlerTest.java
@@ -71,6 +71,7 @@
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpTester;
 import org.eclipse.jetty.http.pathmap.MatchedResource;
+import org.eclipse.jetty.io.Content;
 import org.eclipse.jetty.logging.StacklessLogging;
 import org.eclipse.jetty.security.Constraint;
 import org.eclipse.jetty.security.SecurityHandler;
@@ -104,6 +105,7 @@
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.nullValue;
+import static org.hamcrest.Matchers.sameInstance;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
@@ -1965,17 +1967,16 @@ public void testReplaceServletHandlerWithoutServlet() throws Exception
     }
 
     @Test
-    public void testReplaceHandler() throws Exception
+    public void testInsertHandler() throws Exception
     {
-        ServletContextHandler servletContextHandler = new ServletContextHandler();
+        ServletContextHandler servletContextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS | ServletContextHandler.SECURITY);
         ServletHolder sh = new ServletHolder(new TestServlet());
         servletContextHandler.addServlet(sh, "/foo");
         final AtomicBoolean contextInit = new AtomicBoolean(false);
         final AtomicBoolean contextDestroy = new AtomicBoolean(false);
 
         servletContextHandler.addEventListener(new ServletContextListener()
         {
-
             @Override
             public void contextInitialized(ServletContextEvent sce)
             {
@@ -1990,15 +1991,26 @@ public void contextDestroyed(ServletContextEvent sce)
                     contextDestroy.set(true);
             }
         });
-        ServletHandler shandler = servletContextHandler.getServletHandler();
-
         ResourceHandler rh = new ResourceHandler();
         rh.setBaseResource(ResourceFactory.of(rh).newResource(Paths.get(".")));
-
         servletContextHandler.insertHandler(rh);
-        assertEquals(shandler, servletContextHandler.getServletHandler());
-        assertEquals(rh, servletContextHandler.getHandler());
-        assertEquals(rh.getHandler(), shandler);
+
+        Handler last = new Handler.Abstract()
+        {
+            @Override
+            public boolean handle(Request request, Response response, Callback callback)
+            {
+                return false;
+            }
+        };
+        servletContextHandler.getServletHandler().setHandler(last);
+
+        assertThat(servletContextHandler.getHandler(), sameInstance(rh));
+        assertThat(rh.getHandler(), instanceOf(SessionHandler.class));
+        assertThat(servletContextHandler.getSessionHandler().getHandler(), instanceOf(SecurityHandler.class));
+        assertThat(servletContextHandler.getSecurityHandler().getHandler(), instanceOf(ServletHandler.class));
+        assertThat(servletContextHandler.getServletHandler().getHandler(), sameInstance(last));
+
         _server.setHandler(servletContextHandler);
         _server.start();
         assertTrue(contextInit.get());
@@ -2007,7 +2019,7 @@ public void contextDestroyed(ServletContextEvent sce)
     }
 
     @Test
-    public void testFallThrough() throws Exception
+    public void testFallThroughContextHandler() throws Exception
     {
         Handler.Sequence list = new Handler.Sequence();
         _server.setHandler(list);
@@ -2038,6 +2050,37 @@ public boolean handle(Request request, Response response, Callback callback) thr
         assertThat(response, Matchers.containsString("404 Fell Through"));
     }
 
+    @Test
+    public void testFallThroughServletHandler() throws Exception
+    {
+        ServletContextHandler root = new ServletContextHandler("/", ServletContextHandler.SESSIONS | ServletContextHandler.SECURITY);
+        _server.setHandler(root);
+
+        ServletHandler servlet = root.getServletHandler();
+        servlet.setEnsureDefaultServlet(false);
+        servlet.addServletWithMapping(HelloServlet.class, "/hello/*");
+
+        servlet.setHandler(new Handler.Abstract()
+        {
+            @Override
+            public boolean handle(Request request, Response response, Callback callback) throws Exception
+            {
+                Content.Sink.write(response, true, "Fell Through Handler: " + request.getSession(true).getId(), callback);
+                return true;
+            }
+        });
+
+        _server.start();
+
+        String response = _connector.getResponse("GET /hello HTTP/1.0\r\n\r\n");
+        assertThat(response, Matchers.containsString("200 OK"));
+
+        response = _connector.getResponse("GET /other HTTP/1.0\r\n\r\n");
+        assertThat(response, Matchers.containsString("200 OK"));
+        assertThat(response, Matchers.containsString("Set-Cookie: JSESSIONID="));
+        assertThat(response, Matchers.containsString("Fell Through Handler"));
+    }
+
     /**
      * Test behavior of new {@link org.eclipse.jetty.util.Decorator}, with
      * new DecoratedObjectFactory class