[GitHub] [knox] pzampino commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO

[GitBox <gi...@apache.org>]

pzampino commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r939161193


##########
gateway-server/src/test/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifierTest.java:
##########
@@ -48,25 +52,25 @@ private GatewayConfig mockConfig(Set<String> privilegedUsers, Set<String> nonPri
 
 
   @Test
-  public void userIsInNeitherOfTheGroups() {
+  public void userIsInNeitherOfTheGroups() throws ServiceLifecycleException {
     GatewayConfig config = mockConfig(new HashSet<>(Arrays.asList("admin")), new HashSet<>(Arrays.asList("tom", "guest")), 3, 2);
-    verifier.init(config);
+    verifier.init(config, options);
     for (int i = 0; i < 4; i++) {

Review Comment:
   What is the intent of this loop to verify the same user? Is it for testing the limit(s) defined in the mockConfig? If so, it would be better to reference the same constant for both the creation of the mockConfig and the subsequent loop. Some comments might be helpful here as well, even if only a message in the assertion.



##########
gateway-server/src/test/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifierTest.java:
##########
@@ -48,25 +52,25 @@ private GatewayConfig mockConfig(Set<String> privilegedUsers, Set<String> nonPri
 
 
   @Test
-  public void userIsInNeitherOfTheGroups() {
+  public void userIsInNeitherOfTheGroups() throws ServiceLifecycleException {

Review Comment:
   Generally, in Knox, test methods begin with the prefix "test". While it may seem unnecessary with the @Test annotation, it is the pattern followed throughout the codebase.



##########
gateway-server/src/test/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifierTest.java:
##########
@@ -18,22 +18,26 @@
 package org.apache.knox.gateway.session.control;
 
 import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
 import org.easymock.EasyMock;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Map;
 import java.util.Set;
 
 public class ConcurrentSessionVerifierTest {
-
   private ConcurrentSessionVerifier verifier;
+  private Map<String, String> options;
 
   @Before
   public void setUp() {
-    verifier = ConcurrentSessionVerifier.getInstance();
+    verifier = new ConcurrentSessionVerifier();
+    options = new HashMap<>();

Review Comment:
   Does this Map ever get populated? If not, you could replace this with Collections.emptyMap().



##########
gateway-server/src/test/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifierTest.java:
##########
@@ -106,25 +110,25 @@ public void userIsNotPrivileged() {
   }
 
   @Test
-  public void privilegedLimitIsZero() {
+  public void privilegedLimitIsZero() throws ServiceLifecycleException {
     GatewayConfig config = mockConfig(new HashSet<>(Arrays.asList("admin")), new HashSet<>(Arrays.asList("tom", "guest")), 0, 2);
-    verifier.init(config);
+    verifier.init(config, options);
 
     Assert.assertFalse(verifier.verifySessionForUser("admin"));
   }
 
   @Test
-  public void nonPrivilegedLimitIsZero() {
+  public void nonPrivilegedLimitIsZero() throws ServiceLifecycleException {
     GatewayConfig config = mockConfig(new HashSet<>(Arrays.asList("admin")), new HashSet<>(Arrays.asList("tom", "guest")), 3, 0);
-    verifier.init(config);
+    verifier.init(config, options);
 
     Assert.assertFalse(verifier.verifySessionForUser("tom"));
   }
 
   @Test
-  public void sessionsDoNotGoToNegative() {
+  public void sessionsDoNotGoToNegative() throws ServiceLifecycleException {
     GatewayConfig config = mockConfig(new HashSet<>(Arrays.asList("admin")), new HashSet<>(Arrays.asList("tom", "guest")), 2, 2);
-    verifier.init(config);
+    verifier.init(config, options);
 
     Assert.assertNull(verifier.getUserConcurrentSessionCount("admin"));

Review Comment:
   Why does this return null instead of zero?



-- 
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: dev-unsubscribe@knox.apache.org

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