[
https://issues.apache.org/jira/browse/KNOX-2778?focusedWorklogId=798549&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-798549
]
ASF GitHub Bot logged work on KNOX-2778:
----------------------------------------
Author: ASF GitHub Bot
Created on: 05/Aug/22 21:03
Start Date: 05/Aug/22 21:03
Worklog Time Spent: 10m
Work Description: 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?
Issue Time Tracking
-------------------
Worklog Id: (was: 798549)
Time Spent: 2.5h (was: 2h 20m)
> Enforce concurrent session limit in KnoxSSO
> -------------------------------------------
>
> Key: KNOX-2778
> URL: https://issues.apache.org/jira/browse/KNOX-2778
> Project: Apache Knox
> Issue Type: Sub-task
> Components: Server
> Affects Versions: 2.0.0
> Reporter: Sandor Molnar
> Assignee: Balazs Marton
> Priority: Major
> Fix For: 2.0.0
>
> Time Spent: 2.5h
> Remaining Estimate: 0h
>
> Once, KNOX-2777 is ready, the next step is to wire that verifier
> implementation into the KnoxSSO flow such as it throws an authorization error
> (FORBIDDEN; 403) when a user tries to log in to UIs (both Knox's own UIs or
> UIs proxied by Knox) but that user exceeds the configured concurrent session
> limit.
> Basic logout handling should be covered too:
> * manually clicking on the logout button
> * subscribing to a session timeout event (you may want to talk to [~smore]
> about this)
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
