Various cleanups of Handler.insertHandler

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");
 

jetty-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);
 

jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java

@@ -939,23 +939,6 @@ public boolean addEventListener(EventListener listener)
         return false;
     }
 
-    @Override
-    public void setHandler(Handler handler)
-    {
-        if (handler instanceof SessionHandler)
-            setSessionHandler((SessionHandler)handler);
-        else if (handler instanceof SecurityHandler)
-            setSecurityHandler((SecurityHandler)handler);
-        else if (handler instanceof ServletHandler)
-            setServletHandler((ServletHandler)handler);
-        else
-        {
-            if (handler != null)
-                LOG.warn("ServletContextHandler.setHandler should not be called directly. Use insertHandler or setSessionHandler etc.");
-            super.setHandler(handler);
-        }
-    }
-
     private void doSetHandler(Singleton wrapper, Handler handler)
     {
         if (wrapper == this)
@@ -1128,18 +1111,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 +1144,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
@@ -1647,6 +1638,22 @@ public void setServletHandler(ServletHandler servletHandler)
         relinkHandlers();
     }
 
+    @Override
+    public void setHandler(Handler handler)
+    {
+        if (handler instanceof SessionHandler && getHandler() instanceof SessionHandler)
+            setSessionHandler((SessionHandler)handler);
+        else if (handler instanceof SecurityHandler && getHandler() instanceof SecurityHandler)
+            setSecurityHandler((SecurityHandler)handler);
+        else if (handler instanceof ServletHandler && getHandler() instanceof ServletHandler)
+            setServletHandler((ServletHandler)handler);
+        else
+        {
+            LOG.warn("setHandler should not be used. Use explicit setters or insertHandler");
+            super.setHandler(handler);
+        }
+    }
+
     /**
      * Insert a HandlerWrapper before the first Session, Security or ServletHandler
      * but after any other HandlerWrappers.
@@ -1662,24 +1669,15 @@ 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();
     }

jetty-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);
     }
 
     /**

jetty-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();
+        }
     }
 }

jetty-ee10/jetty-ee10-servlet/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