Issue #4903 - fix validation on custom Configurator annotated endpoint

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
jetty · Jun 10, 2020 · f41f601 · f41f601
1 parent b22e306
commit f41f601
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 34 deletions.
diff --git a/...rc/main/java/org/eclipse/jetty/websocket/jsr356/server/AnnotatedServerEndpointConfig.java b/...rc/main/java/org/eclipse/jetty/websocket/jsr356/server/AnnotatedServerEndpointConfig.java
@@ -129,7 +129,7 @@ else if (anno.configurator() == ServerEndpointConfig.Configurator.class)
         {
             try
             {
-                resolvedConfigurator = anno.configurator().getDeclaredConstructor().newInstance();
+                resolvedConfigurator = anno.configurator().getConstructor().newInstance();
             }
             catch (Exception e)
             {

diff --git a/...-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/ServerContainer.java b/...-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/ServerContainer.java
@@ -118,26 +118,51 @@ public EndpointInstance newClientEndpointInstance(Object endpoint, ServerEndpoin
         return new EndpointInstance(endpoint, cec, metadata);
     }
 
-    @Override
-    public void addEndpoint(Class<?> endpointClass) throws DeploymentException
+    private void validateEndpointConfig(ServerEndpointConfig config) throws DeploymentException
     {
+        if (config == null)
+        {
+            throw new DeploymentException("Unable to deploy null ServerEndpointConfig");
+        }
+
+        ServerEndpointConfig.Configurator configurator = config.getConfigurator();
+        if (configurator == null)
+        {
+            throw new DeploymentException("Unable to deploy with null ServerEndpointConfig.Configurator");
+        }
+
+        Class<?> endpointClass = config.getEndpointClass();
         if (endpointClass == null)
         {
-            throw new DeploymentException("Unable to deploy null endpoint class");
+            throw new DeploymentException("Unable to deploy null endpoint class from ServerEndpointConfig: " + config.getClass().getName());
         }
 
-        if (isStarted() || isStarting())
+        if (configurator.getClass() == ContainerDefaultConfigurator.class)
         {
             if (!ReflectUtils.isDefaultConstructable(endpointClass))
             {
                 throw new DeploymentException("Cannot access default constructor for the class: " + endpointClass.getName());
             }
+        }
+    }
 
+    @Override
+    public void addEndpoint(Class<?> endpointClass) throws DeploymentException
+    {
+        if (endpointClass == null)
+        {
+            throw new DeploymentException("Unable to deploy null endpoint class");
+        }
+
+        if (isStarted() || isStarting())
+        {
             if (LOG.isDebugEnabled())
             {
                 LOG.debug("addEndpoint({})", endpointClass);
             }
+
             ServerEndpointMetadata metadata = getServerEndpointMetadata(endpointClass, null);
+            validateEndpointConfig(metadata.getConfig());
             addEndpoint(metadata);
         }
         else
@@ -159,39 +184,15 @@ private void addEndpoint(ServerEndpointMetadata metadata)
     @Override
     public void addEndpoint(ServerEndpointConfig config) throws DeploymentException
     {
-        if (config == null)
-        {
-            throw new DeploymentException("Unable to deploy null ServerEndpointConfig");
-        }
-
-        Class<?> endpointClass = config.getEndpointClass();
-        if (endpointClass == null)
-        {
-            throw new DeploymentException("Unable to deploy null endpoint class from ServerEndpointConfig: " + config.getClass().getName());
-        }
-
         if (isStarted() || isStarting())
         {
-            ServerEndpointConfig.Configurator configurator = config.getConfigurator();
-
-            if (configurator == null)
-            {
-                throw new DeploymentException("Unable to deploy with null ServerEndpointConfig.Configurator");
-            }
-
-            // only validate constructor and class modifiers on non-custom configurators
-            if (configurator.getClass() == ContainerDefaultConfigurator.class)
-            {
-                if (!ReflectUtils.isDefaultConstructable(endpointClass))
-                {
-                    throw new DeploymentException("Cannot access default constructor for the class: " + endpointClass.getName());
-                }
-            }
+            validateEndpointConfig(config);
 
             if (LOG.isDebugEnabled())
             {
                 LOG.debug("addEndpoint({}) path={} endpoint={}", config, config.getPath(), config.getEndpointClass());
             }
+
             ServerEndpointMetadata metadata = getServerEndpointMetadata(config.getEndpointClass(), config);
             addEndpoint(metadata);
         }

diff --git a/...et/jsr356/server/PrivateEndpointTest.java → ...socket/jsr356/server/AddEndpointTest.java b/...et/jsr356/server/PrivateEndpointTest.java → ...socket/jsr356/server/AddEndpointTest.java
@@ -18,12 +18,15 @@
 
 package org.eclipse.jetty.websocket.jsr356.server;
 
+import java.util.concurrent.TimeUnit;
+import javax.websocket.CloseReason;
 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.OnOpen;
 import javax.websocket.Session;
 import javax.websocket.WebSocketContainer;
 import javax.websocket.server.ServerEndpoint;
@@ -35,6 +38,8 @@
 import org.eclipse.jetty.util.component.LifeCycle;
 import org.eclipse.jetty.websocket.api.util.WSURI;
 import org.eclipse.jetty.websocket.jsr356.server.deploy.WebSocketServerContainerInitializer;
+import org.eclipse.jetty.websocket.jsr356.server.samples.BasicOpenCloseSocket;
+import org.hamcrest.Matchers;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -44,8 +49,9 @@
 import static org.hamcrest.Matchers.instanceOf;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
-public class PrivateEndpointTest
+public class AddEndpointTest
 {
     private Server server;
     private WebSocketContainer client;
@@ -96,6 +102,7 @@ public void onMessage(String message)
         }
     }
 
+    @SuppressWarnings("InnerClassMayBeStatic")
     private class CustomPrivateEndpoint extends Endpoint
     {
         @Override
@@ -104,6 +111,30 @@ public void onOpen(Session session, EndpointConfig config)
         }
     }
 
+    @SuppressWarnings("InnerClassMayBeStatic")
+    @ServerEndpoint(value = "/", configurator = CustomAnnotatedEndpointConfigurator.class)
+    public static class CustomAnnotatedEndpoint
+    {
+        public CustomAnnotatedEndpoint(String id)
+        {
+        }
+
+        @OnOpen
+        public void onOpen(Session session, EndpointConfig config)
+        {
+        }
+    }
+
+    public static class CustomAnnotatedEndpointConfigurator extends ServerEndpointConfig.Configurator
+    {
+        @SuppressWarnings("unchecked")
+        @Override
+        public <T> T getEndpointInstance(Class<T> endpointClass)
+        {
+            return (T)new CustomAnnotatedEndpoint("server");
+        }
+    }
+
     public static class CustomEndpoint extends Endpoint implements MessageHandler.Whole<String>
     {
         public CustomEndpoint(String id)
@@ -186,8 +217,12 @@ public <T> T getEndpointInstance(Class<T> endpointClass)
             }).build();
         start(container -> container.addEndpoint(config));
 
-        Session session = client.connectToServer(new CustomEndpoint("client"), WSURI.toWebsocket(server.getURI().resolve("/")));
+        BasicOpenCloseSocket clientEndpoint = new BasicOpenCloseSocket();
+        Session session = client.connectToServer(clientEndpoint, WSURI.toWebsocket(server.getURI().resolve("/")));
         assertNotNull(session);
+        session.close();
+        assertTrue(clientEndpoint.closeLatch.await(5, TimeUnit.SECONDS));
+        assertThat(clientEndpoint.closeReason.getCloseCode(), Matchers.is(CloseReason.CloseCodes.NORMAL_CLOSURE));
     }
 
     @Test
@@ -205,8 +240,25 @@ public <T> T getEndpointInstance(Class<T> endpointClass)
             }).build();
         start(container -> container.addEndpoint(config));
 
-        Session session = client.connectToServer(new CustomEndpoint("client"), WSURI.toWebsocket(server.getURI().resolve("/")));
+        BasicOpenCloseSocket clientEndpoint = new BasicOpenCloseSocket();
+        Session session = client.connectToServer(clientEndpoint, WSURI.toWebsocket(server.getURI().resolve("/")));
+        assertNotNull(session);
+        session.close();
+        assertTrue(clientEndpoint.closeLatch.await(5, TimeUnit.SECONDS));
+        assertThat(clientEndpoint.closeReason.getCloseCode(), Matchers.is(CloseReason.CloseCodes.NORMAL_CLOSURE));
+    }
+
+    @Test
+    public void testCustomAnnotatedEndpoint() throws Exception
+    {
+        start(container -> container.addEndpoint(CustomAnnotatedEndpoint.class));
+
+        BasicOpenCloseSocket clientEndpoint = new BasicOpenCloseSocket();
+        Session session = client.connectToServer(clientEndpoint, WSURI.toWebsocket(server.getURI().resolve("/")));
         assertNotNull(session);
+        session.close();
+        assertTrue(clientEndpoint.closeLatch.await(5, TimeUnit.SECONDS));
+        assertThat(clientEndpoint.closeReason.getCloseCode(), Matchers.is(CloseReason.CloseCodes.NORMAL_CLOSURE));
     }
 
     @Test

diff --git a/...src/test/java/org/eclipse/jetty/websocket/jsr356/server/samples/BasicOpenCloseSocket.java b/...src/test/java/org/eclipse/jetty/websocket/jsr356/server/samples/BasicOpenCloseSocket.java
@@ -18,6 +18,7 @@
 
 package org.eclipse.jetty.websocket.jsr356.server.samples;
 
+import javax.websocket.ClientEndpoint;
 import javax.websocket.CloseReason;
 import javax.websocket.OnClose;
 import javax.websocket.OnOpen;
@@ -26,6 +27,7 @@
 import org.eclipse.jetty.websocket.jsr356.server.TrackingSocket;
 
 @ServerEndpoint(value = "/basic")
+@ClientEndpoint
 public class BasicOpenCloseSocket extends TrackingSocket
 {
     @OnOpen