From f41f601e1971691f4f4f27d6f9c9faf99749a4c3 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 11 Jun 2020 09:58:49 +1000 Subject: [PATCH] Issue #4903 - fix validation on custom Configurator annotated endpoint Signed-off-by: Lachlan Roberts --- .../server/AnnotatedServerEndpointConfig.java | 2 +- .../jsr356/server/ServerContainer.java | 63 ++++++++++--------- ...EndpointTest.java => AddEndpointTest.java} | 58 ++++++++++++++++- .../server/samples/BasicOpenCloseSocket.java | 2 + 4 files changed, 90 insertions(+), 35 deletions(-) rename jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/{PrivateEndpointTest.java => AddEndpointTest.java} (78%) diff --git a/jetty-websocket/javax-websocket-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/AnnotatedServerEndpointConfig.java b/jetty-websocket/javax-websocket-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/AnnotatedServerEndpointConfig.java index 306e0a9390a..2d23b87d5a2 100644 --- a/jetty-websocket/javax-websocket-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/AnnotatedServerEndpointConfig.java +++ b/jetty-websocket/javax-websocket-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/AnnotatedServerEndpointConfig.java @@ -129,7 +129,7 @@ public class AnnotatedServerEndpointConfig implements ServerEndpointConfig { try { - resolvedConfigurator = anno.configurator().getDeclaredConstructor().newInstance(); + resolvedConfigurator = anno.configurator().getConstructor().newInstance(); } catch (Exception e) { diff --git a/jetty-websocket/javax-websocket-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/ServerContainer.java b/jetty-websocket/javax-websocket-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/ServerContainer.java index b65698be0aa..8ddf395038c 100644 --- a/jetty-websocket/javax-websocket-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/ServerContainer.java +++ b/jetty-websocket/javax-websocket-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/ServerContainer.java @@ -118,6 +118,34 @@ public class ServerContainer extends ClientContainer implements javax.websocket. return new EndpointInstance(endpoint, cec, metadata); } + 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 from ServerEndpointConfig: " + config.getClass().getName()); + } + + 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 { @@ -128,16 +156,13 @@ public class ServerContainer extends ClientContainer implements javax.websocket. if (isStarted() || isStarting()) { - if (!ReflectUtils.isDefaultConstructable(endpointClass)) - { - throw new DeploymentException("Cannot access default constructor for the class: " + endpointClass.getName()); - } - if (LOG.isDebugEnabled()) { LOG.debug("addEndpoint({})", endpointClass); } + ServerEndpointMetadata metadata = getServerEndpointMetadata(endpointClass, null); + validateEndpointConfig(metadata.getConfig()); addEndpoint(metadata); } else @@ -159,39 +184,15 @@ public class ServerContainer extends ClientContainer implements javax.websocket. @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/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/PrivateEndpointTest.java b/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/AddEndpointTest.java similarity index 78% rename from jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/PrivateEndpointTest.java rename to jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/AddEndpointTest.java index 55555d320ee..bd8492ffdd3 100644 --- a/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/PrivateEndpointTest.java +++ b/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/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.servlet.ServletContextHandler; 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.containsString; 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 class PrivateEndpointTest } } + @SuppressWarnings("InnerClassMayBeStatic") private class CustomPrivateEndpoint extends Endpoint { @Override @@ -104,6 +111,30 @@ public class PrivateEndpointTest } } + @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 getEndpointInstance(Class endpointClass) + { + return (T)new CustomAnnotatedEndpoint("server"); + } + } + public static class CustomEndpoint extends Endpoint implements MessageHandler.Whole { public CustomEndpoint(String id) @@ -186,8 +217,12 @@ public class PrivateEndpointTest }).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 class PrivateEndpointTest }).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/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/samples/BasicOpenCloseSocket.java b/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/samples/BasicOpenCloseSocket.java index c156bc39b21..fb5bce5f3b0 100644 --- a/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/samples/BasicOpenCloseSocket.java +++ b/jetty-websocket/javax-websocket-server-impl/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 javax.websocket.server.ServerEndpoint; import org.eclipse.jetty.websocket.jsr356.server.TrackingSocket; @ServerEndpoint(value = "/basic") +@ClientEndpoint public class BasicOpenCloseSocket extends TrackingSocket { @OnOpen