[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4441: ARTEMIS-4245 Expose web SNI settings

["gemmellr (via GitHub)" <gi...@apache.org>]

gemmellr commented on code in PR #4441:
URL: https://github.com/apache/activemq-artemis/pull/4441#discussion_r1178936369


##########
artemis-web/src/test/java/org/apache/activemq/cli/test/WebServerComponentTest.java:
##########
@@ -283,6 +298,40 @@ public void simpleSecureServer() throws Exception {
       Assert.assertFalse(webServerComponent.isStarted());
    }
 
+
+   @Test
+   public void testSimpleSecureServerWithSniHostCheckEnabled() throws Exception {
+      testSimpleSecureServerWithSniHostCheck(true);
+   }
+
+   @Test
+   public void testSimpleSecureServerWithSniHostCheckDisabled() throws Exception {
+      testSimpleSecureServerWithSniHostCheck(false);
+   }

Review Comment:
   Do we want to test the default behaviour is 'enabled' (i.e change the boolean to Boolean and pass null as well)?
   
   Currently nothing looks to be checking the default, since these 2 tests only verify it succeeds/fails using the IP address when the setting was _explicitly_ disabled or enabled.



##########
artemis-dto/src/main/java/org/apache/activemq/artemis/dto/BindingDTO.java:
##########
@@ -181,6 +187,22 @@ public void addApp(AppDTO app) {
       apps.add(app);
    }
 
+   public Boolean getSniHostCheck() {
+      return sniHostCheck;
+   }
+
+   public void setSniHostCheck(Boolean sniHostCheck) {
+      this.sniHostCheck = sniHostCheck;
+   }
+
+   public Boolean getSniRequired() {
+      return sniRequired;
+   }
+
+   public void setSniRequired(Boolean sniRequired) {
+      this.sniRequired = sniRequired;
+   }
+

Review Comment:
   org.apache.activemq.artemis.dto.test.BindingDTOTest.testDefault() should be updated to check these are null by default since the new default behaviour depends on it.



##########
artemis-web/src/test/java/org/apache/activemq/cli/test/WebServerComponentTest.java:
##########
@@ -215,11 +221,11 @@ public void testComponentStopStartBehavior() throws Exception {
       Assert.assertFalse(webServerComponent.isStarted());
    }
 
-   @Test
-   public void simpleSecureServer() throws Exception {
+   private WebServerComponent startSimpleSecureServer(String keyStorePath, String keyStorePassword, Boolean sniHostCheck) throws Exception {
       BindingDTO bindingDTO = new BindingDTO();
       bindingDTO.uri = "https://localhost:0";
       bindingDTO.keyStorePath = "./src/test/resources/server.keystore";
+      bindingDTO.setSniHostCheck(sniHostCheck);

Review Comment:
   might be nice to only set it if non-null?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org