Merge pull request #4906 from eclipse/jetty-9.4.x-4903-WebSocketPublicModifier

Issue #4903 - give better errors for non public javax.websocket endpoints

jetty-websocket/javax-websocket-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/ContainerDefaultConfigurator.java

@@ -72,7 +72,10 @@ public final class ContainerDefaultConfigurator extends Configurator
         }
         catch (Exception e)
         {
-            throw new InstantiationException(String.format("%s: %s", e.getClass().getName(), e.getMessage()));
+            String errorMsg = String.format("%s: %s", e.getClass().getName(), e.getMessage());
+            InstantiationException instantiationException = new InstantiationException(errorMsg);
+            instantiationException.initCause(e);
+            throw instantiationException;
         }
     }
 

jetty-websocket/javax-websocket-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/JsrCreator.java

@@ -159,8 +159,7 @@ public class JsrCreator implements WebSocketCreator
         }
         catch (InstantiationException e)
         {
-            if (LOG.isDebugEnabled())
+            LOG.warn("Unable to create websocket: " + config.getEndpointClass().getName(), e);
-                LOG.debug("Unable to create websocket: " + config.getEndpointClass().getName(), e);
             return null;
         }
     }

jetty-websocket/javax-websocket-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/ServerContainer.java

@@ -34,6 +34,7 @@ import org.eclipse.jetty.util.annotation.ManagedObject;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 import org.eclipse.jetty.websocket.common.events.EventDriverFactory;
+import org.eclipse.jetty.websocket.common.util.ReflectUtils;
 import org.eclipse.jetty.websocket.jsr356.ClientContainer;
 import org.eclipse.jetty.websocket.jsr356.JsrSessionFactory;
 import org.eclipse.jetty.websocket.jsr356.annotations.AnnotatedEndpointScanner;
@@ -129,7 +130,7 @@ public class ServerContainer extends ClientContainer implements javax.websocket.
         {
             if (deferredEndpointClasses == null)
             {
-                deferredEndpointClasses = new ArrayList<Class<?>>();
+                deferredEndpointClasses = new ArrayList<>();
             }
             deferredEndpointClasses.add(endpointClass);
         }
@@ -137,6 +138,9 @@ public class ServerContainer extends ClientContainer implements javax.websocket.
 
     private void addEndpoint(ServerEndpointMetadata metadata) throws DeploymentException
     {
+        if (!ReflectUtils.isDefaultConstructable(metadata.getEndpointClass()))
+            throw new DeploymentException("Cannot access default constructor for the Endpoint class");
+
         JsrCreator creator = new JsrCreator(this, metadata, this.configuration.getFactory().getExtensionFactory());
         this.configuration.addMapping("uri-template|" + metadata.getPath(), creator);
     }
@@ -157,7 +161,7 @@ public class ServerContainer extends ClientContainer implements javax.websocket.
         {
             if (deferredEndpointConfigs == null)
             {
-                deferredEndpointConfigs = new ArrayList<ServerEndpointConfig>();
+                deferredEndpointConfigs = new ArrayList<>();
             }
             deferredEndpointConfigs.add(config);
         }
@@ -191,35 +195,43 @@ public class ServerContainer extends ClientContainer implements javax.websocket.
 
     public ServerEndpointMetadata getServerEndpointMetadata(final Class<?> endpoint, final ServerEndpointConfig config) throws DeploymentException
     {
-        ServerEndpointMetadata metadata = null;
+        try
+        {
+            ServerEndpointMetadata metadata;
+            ServerEndpoint anno = endpoint.getAnnotation(ServerEndpoint.class);
+            if (anno != null)
+            {
+                // Annotated takes precedence here
+                AnnotatedServerEndpointMetadata ametadata = new AnnotatedServerEndpointMetadata(this, endpoint, config);
+                AnnotatedEndpointScanner<ServerEndpoint, ServerEndpointConfig> scanner = new AnnotatedEndpointScanner<>(ametadata);
+                metadata = ametadata;
+                scanner.scan();
+            }
+            else if (Endpoint.class.isAssignableFrom(endpoint))
+            {
+                // extends Endpoint
+                @SuppressWarnings("unchecked")
+                Class<? extends Endpoint> eendpoint = (Class<? extends Endpoint>)endpoint;
+                metadata = new SimpleServerEndpointMetadata(eendpoint, config);
+            }
+            else
+            {
+                String err = "Not a recognized websocket [" + endpoint.getName() +
+                    "] does not extend @" + ServerEndpoint.class.getName() +
+                    " or extend from " + Endpoint.class.getName();
+                throw new DeploymentException(err);
+            }
 
-        ServerEndpoint anno = endpoint.getAnnotation(ServerEndpoint.class);
+            return metadata;
-        if (anno != null)
-        {
-            // Annotated takes precedence here
-            AnnotatedServerEndpointMetadata ametadata = new AnnotatedServerEndpointMetadata(this, endpoint, config);
-            AnnotatedEndpointScanner<ServerEndpoint, ServerEndpointConfig> scanner = new AnnotatedEndpointScanner<>(ametadata);
-            metadata = ametadata;
-            scanner.scan();
         }
-        else if (Endpoint.class.isAssignableFrom(endpoint))
+        catch (DeploymentException e)
         {
-            // extends Endpoint
+            throw e;
-            @SuppressWarnings("unchecked")
-            Class<? extends Endpoint> eendpoint = (Class<? extends Endpoint>)endpoint;
-            metadata = new SimpleServerEndpointMetadata(eendpoint, config);
         }
-        else
+        catch (Throwable t)
         {
-            StringBuilder err = new StringBuilder();
+            throw new DeploymentException(t.getMessage(), t);
-            err.append("Not a recognized websocket [");
-            err.append(endpoint.getName());
-            err.append("] does not extend @").append(ServerEndpoint.class.getName());
-            err.append(" or extend from ").append(Endpoint.class.getName());
-            throw new DeploymentException("Unable to identify as valid Endpoint: " + endpoint);
         }
-
-        return metadata;
     }
 
     @Override

jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/PrivateEndpointTest.java

@@ -0,0 +1,173 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.websocket.jsr356.server;
+
+import javax.websocket.ContainerProvider;
+import javax.websocket.DeploymentException;
+import javax.websocket.Endpoint;
+import javax.websocket.EndpointConfig;
+import javax.websocket.MessageHandler;
+import javax.websocket.OnMessage;
+import javax.websocket.Session;
+import javax.websocket.WebSocketContainer;
+import javax.websocket.server.ServerEndpoint;
+import javax.websocket.server.ServerEndpointConfig;
+
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.server.ServerConnector;
+import org.eclipse.jetty.servlet.ServletContextHandler;
+import org.eclipse.jetty.util.component.LifeCycle;
+import org.eclipse.jetty.websocket.jsr356.server.deploy.WebSocketServerContainerInitializer;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+public class PrivateEndpointTest
+{
+    private Server server;
+    private WebSocketContainer client;
+    private ServletContextHandler contextHandler;
+
+    @BeforeEach
+    public void before()
+    {
+        server = new Server();
+        ServerConnector connector = new ServerConnector(server);
+        server.addConnector(connector);
+
+        contextHandler = new ServletContextHandler();
+        contextHandler.setContextPath("/");
+        server.setHandler(contextHandler);
+        client = ContainerProvider.getWebSocketContainer();
+    }
+
+    @AfterEach
+    public void after() throws Exception
+    {
+        LifeCycle.stop(client);
+        server.stop();
+    }
+
+    public interface CheckedConsumer<T>
+    {
+        void accept(T t) throws DeploymentException;
+    }
+
+    public void start(CheckedConsumer<ServerContainer> containerConsumer) throws Exception
+    {
+        WebSocketServerContainerInitializer.configure(contextHandler, (context, container) -> containerConsumer.accept(container));
+        server.start();
+    }
+
+    private static class ServerSocket extends Endpoint implements MessageHandler.Whole<String>
+    {
+        @Override
+        public void onOpen(Session session, EndpointConfig config)
+        {
+            session.addMessageHandler(this);
+        }
+
+        @Override
+        public void onMessage(String message)
+        {
+        }
+    }
+
+    @SuppressWarnings("InnerClassMayBeStatic")
+    public class ServerSocketNonStatic extends Endpoint implements MessageHandler.Whole<String>
+    {
+        @Override
+        public void onOpen(Session session, EndpointConfig config)
+        {
+            session.addMessageHandler(this);
+        }
+
+        @Override
+        public void onMessage(String message)
+        {
+        }
+    }
+
+    @ServerEndpoint("/annotated")
+    private static class AnnotatedServerSocket
+    {
+        @OnMessage
+        public void onMessage(String message)
+        {
+        }
+    }
+
+    @ServerEndpoint("/annotatedMethod")
+    public static class AnnotatedServerMethod
+    {
+        @OnMessage
+        private void onMessage(String message)
+        {
+        }
+    }
+
+    @Test
+    public void testEndpoint()
+    {
+        RuntimeException error = assertThrows(RuntimeException.class, () ->
+            start(container -> container.addEndpoint(ServerEndpointConfig.Builder.create(ServerSocket.class, "/").build())));
+
+        assertThat(error.getCause(), instanceOf(DeploymentException.class));
+        DeploymentException deploymentException = (DeploymentException)error.getCause();
+        assertThat(deploymentException.getMessage(), containsString("Cannot access default constructor for the Endpoint class"));
+    }
+
+    @Test
+    public void testInnerEndpoint()
+    {
+        RuntimeException error = assertThrows(RuntimeException.class, () ->
+            start(container -> container.addEndpoint(ServerEndpointConfig.Builder.create(ServerSocketNonStatic.class, "/").build())));
+
+        assertThat(error.getCause(), instanceOf(DeploymentException.class));
+        DeploymentException deploymentException = (DeploymentException)error.getCause();
+        assertThat(deploymentException.getMessage(), containsString("Cannot access default constructor for the Endpoint class"));
+    }
+
+    @Test
+    public void testAnnotatedEndpoint()
+    {
+        RuntimeException error = assertThrows(RuntimeException.class, () ->
+            start(container -> container.addEndpoint(AnnotatedServerSocket.class)));
+
+        assertThat(error.getCause(), instanceOf(DeploymentException.class));
+        DeploymentException deploymentException = (DeploymentException)error.getCause();
+        assertThat(deploymentException.getMessage(), containsString("Cannot access default constructor for the Endpoint class"));
+    }
+
+    @Test
+    public void testAnnotatedMethod()
+    {
+        RuntimeException error = assertThrows(RuntimeException.class, () ->
+            start(container -> container.addEndpoint(AnnotatedServerMethod.class)));
+
+        assertThat(error.getCause(), instanceOf(DeploymentException.class));
+        DeploymentException deploymentException = (DeploymentException)error.getCause();
+        assertThat(deploymentException.getMessage(), containsString("Method modifier must be public"));
+    }
+}