You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2022/08/01 14:41:15 UTC
[GitHub] [knox] MrtnBalazs opened a new pull request, #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
MrtnBalazs opened a new pull request, #615:
URL: https://github.com/apache/knox/pull/615
## What changes were proposed in this pull request?
`ConcurrentSessionVerifier` is changed to be used and initilaized like a service, because initializing it in `GatewayServer` `main()` felt out of place. It is also modified to store the tokens issued at login so the expiry can be tracked since we don t have an event to subscribe to when the tokens expire. The verifier was put in `WebSSOResource` and `WebSSOutResource` to count the issued tokens when logged in or out.
## How was this patch tested?
I tested it manually by configuring `gateway-site.xml` and logging in and out from different browsers.
Configuration:
```
<property>
<name>gateway.non.privileged.users</name>
<value>tom,guest</value>
</property>
<property>
<name>gateway.privileged.users</name>
<value>admin</value>
</property>
<property>
<name>gateway.privileged.users.concurrent.session.limit</name>
<value>2</value>
</property>
<property>
<name>gateway.non.privileged.users.concurrent.session.limit</name>
<value>1</value>
</property>
```
--
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
[GitHub] [knox] zeroflag commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r936529383
##########
gateway-service-knoxsso/pom.xml:
##########
@@ -81,5 +81,9 @@
<artifactId>gateway-test-utils</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.knox</groupId>
+ <artifactId>gateway-server</artifactId>
Review Comment:
Same as about. This can be deleted by inverting the dependencies.
--
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
[GitHub] [knox] MrtnBalazs commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r936365675
##########
gateway-service-knoxssout/pom.xml:
##########
@@ -68,5 +68,9 @@
<artifactId>gateway-test-utils</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.knox</groupId>
Review Comment:
The ConcurrentSessionVerifier is in gateway-server.
##########
gateway-service-knoxsso/pom.xml:
##########
@@ -81,5 +81,9 @@
<artifactId>gateway-test-utils</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.knox</groupId>
+ <artifactId>gateway-server</artifactId>
Review Comment:
The ConcurrentSessionVerifier is in gateway-server.
--
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
[GitHub] [knox] smolnar82 commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r948945491
##########
gateway-server/src/main/resources/META-INF/services/org.apache.knox.gateway.services.ServiceFactory:
##########
@@ -30,4 +30,5 @@ org.apache.knox.gateway.services.factory.ServiceRegistryServiceFactory
org.apache.knox.gateway.services.factory.SslServiceFactory
org.apache.knox.gateway.services.factory.TokenServiceFactory
org.apache.knox.gateway.services.factory.TokenStateServiceFactory
-org.apache.knox.gateway.services.factory.TopologyServiceFactory
\ No newline at end of file
+org.apache.knox.gateway.services.factory.TopologyServiceFactory
+org.apache.knox.gateway.services.factory.ConcurrentSessionVerifierFactory
Review Comment:
nit: this list is an alphabetically-ordered list. The new service factory should have been added in its appropriate line (line 20)
--
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
[GitHub] [knox] smolnar82 merged pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
smolnar82 merged PR #615:
URL: https://github.com/apache/knox/pull/615
--
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
[GitHub] [knox] MrtnBalazs closed pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs closed pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
URL: https://github.com/apache/knox/pull/615
--
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
[GitHub] [knox] MrtnBalazs commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r942118332
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class InMemoryConcurrentSessionVerifier implements ConcurrentSessionVerifier {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<SessionJWT>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ @Override
+ public boolean verifySessionForUser(String username, JWT jwtToken) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, sessionTokenSet) -> addTokenForUser(sessionTokenSet, jwtToken));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ return (int) concurrentSessionCounter
+ .getOrDefault(username, Collections.emptySet())
+ .stream()
+ .filter(each -> !each.hasExpired())
+ .count();
+ }
+
+ private boolean privilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (privilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return privilegedUsers.contains(username) && (validTokenNumber >= privilegedUserConcurrentSessionLimit);
+ }
+
+ private boolean nonPrivilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (nonPrivilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return nonPrivilegedUsers.contains(username) && (validTokenNumber >= nonPrivilegedUserConcurrentSessionLimit);
+ }
+
+ @Override
+ public void sessionEndedForUser(String username, String token) {
+ if (StringUtils.isNotBlank(token)) {
+ sessionCountModifyLock.lock();
+ try {
+ concurrentSessionCounter.computeIfPresent(username, (key, sessionTokenSet) -> removeTokenFromUser(sessionTokenSet, token));
Review Comment:
Yes, good idea.
--
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
[GitHub] [knox] zeroflag commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r936528737
##########
gateway-service-knoxssout/pom.xml:
##########
@@ -68,5 +68,9 @@
<artifactId>gateway-test-utils</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.knox</groupId>
Review Comment:
We can solve this by inverting the dependencies. Let's introduce an interface fo the ConcurrentSessionVerifier and put it under gateway-spi. In fact the ConcurrentSessionVerifier is already a good interface name, I would rename the implementation to something like InMemoryConcurrentSessionVerifier. So the implementation can stay in gateway-server and the interface in gateway-spi. WebSSOutResource and WebSSOResource only needs to know the interface and not the implementation. After that we can delete this dependency.
--
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
[GitHub] [knox] zeroflag commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r935525128
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.Service;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+import org.apache.knox.gateway.services.security.token.impl.JWTToken;
+
+import java.text.ParseException;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class ConcurrentSessionVerifier implements Service {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<String>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ public boolean verifySessionForUser(String username, String token) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, tokenSet) -> addTokenForUser(tokenSet, token));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ int result = 0;
+ Set<String> tokens = concurrentSessionCounter.getOrDefault(username, null);
+ if (tokens != null && !tokens.isEmpty()) {
+ for (String token : tokens) {
+ try {
+ JWT jwtToken = new JWTToken(token);
Review Comment:
We need to consider the cost of `new JWToken(token)`. I assume it parses the token everyt ime. Depending on how expensive is this, we might need to store the preparsed tokens or some metadata about the token.
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.Service;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+import org.apache.knox.gateway.services.security.token.impl.JWTToken;
+
+import java.text.ParseException;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class ConcurrentSessionVerifier implements Service {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<String>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ public boolean verifySessionForUser(String username, String token) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, tokenSet) -> addTokenForUser(tokenSet, token));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ int result = 0;
+ Set<String> tokens = concurrentSessionCounter.getOrDefault(username, null);
+ if (tokens != null && !tokens.isEmpty()) {
+ for (String token : tokens) {
+ try {
+ JWT jwtToken = new JWTToken(token);
+ Date expire = jwtToken.getExpiresDate();
+ if (expire == null || expire.after(new Date())) {
Review Comment:
I would put a `hasExpired()` method into JWTToken to handle this check internally.
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.Service;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+import org.apache.knox.gateway.services.security.token.impl.JWTToken;
+
+import java.text.ParseException;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class ConcurrentSessionVerifier implements Service {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<String>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ public boolean verifySessionForUser(String username, String token) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, tokenSet) -> addTokenForUser(tokenSet, token));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ int result = 0;
+ Set<String> tokens = concurrentSessionCounter.getOrDefault(username, null);
Review Comment:
The null check can be avoided here by using an emptySet as a default. The isEmpty check is not needed.
```java
for (String token: concurrentSessionCounter.getOrDefault(username, Collections.emptySet())) {
..
}
```
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.Service;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+import org.apache.knox.gateway.services.security.token.impl.JWTToken;
+
+import java.text.ParseException;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class ConcurrentSessionVerifier implements Service {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<String>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ public boolean verifySessionForUser(String username, String token) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, tokenSet) -> addTokenForUser(tokenSet, token));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ int result = 0;
+ Set<String> tokens = concurrentSessionCounter.getOrDefault(username, null);
+ if (tokens != null && !tokens.isEmpty()) {
+ for (String token : tokens) {
+ try {
+ JWT jwtToken = new JWTToken(token);
+ Date expire = jwtToken.getExpiresDate();
+ if (expire == null || expire.after(new Date())) {
+ result++;
+ }
+ } catch (ParseException ignore) {
Review Comment:
Under what circumstances do we get the ParseException. Depending on what it means we should either log it or throw it out.
##########
gateway-service-knoxsso/pom.xml:
##########
@@ -81,5 +81,9 @@
<artifactId>gateway-test-utils</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.knox</groupId>
+ <artifactId>gateway-server</artifactId>
Review Comment:
Which class is references from gateway-server from gateway-service-knoxssou?
##########
gateway-service-knoxssout/pom.xml:
##########
@@ -68,5 +68,9 @@
<artifactId>gateway-test-utils</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.knox</groupId>
Review Comment:
Which class is references from gateway-server from gateway-service-knoxssout?
--
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
[GitHub] [knox] pzampino commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by 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
[GitHub] [knox] zeroflag commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r938566104
##########
gateway-service-knoxssout/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOutResource.java:
##########
@@ -106,6 +111,23 @@ private boolean removeAuthenticationToken(HttpServletResponse response) {
}
response.addCookie(c);
+ Cookie[] cookies = request.getCookies();
+ if (cookies != null) {
+ Optional<Cookie> ssoCookie = Arrays.stream(cookies).filter(cookie -> cookie.getName().equals(cookieName)).findFirst();
Review Comment:
Could you extract the cookie finding logic in its own method?
Something like this:
```java
Optional<Cookie> findCookie(String cookieName) {
Cookie[] cookies = request.getCookies();
if (cookies != null) {
return Arrays.stream(cookies).filter(cookie -> cookie.getName().equals(cookieName)).findFirst();
} else {
return Optional.empty();
}
}
```
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,168 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class InMemoryConcurrentSessionVerifier implements ConcurrentSessionVerifier {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<SessionJWT>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ @Override
+ public boolean verifySessionForUser(String username, JWT JWToken) {
Review Comment:
Let's rename this parameter name to jwtToken
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,168 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class InMemoryConcurrentSessionVerifier implements ConcurrentSessionVerifier {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<SessionJWT>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ @Override
+ public boolean verifySessionForUser(String username, JWT JWToken) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, sessionTokenSet) -> addTokenForUser(sessionTokenSet, JWToken));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ return (int) concurrentSessionCounter
+ .getOrDefault(username, Collections.emptySet())
+ .stream()
+ .filter(each -> !each.hasExpired())
+ .count();
+ }
+
+ private boolean privilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (privilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return privilegedUsers.contains(username) && (validTokenNumber >= privilegedUserConcurrentSessionLimit);
+ }
+
+ private boolean nonPrivilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (nonPrivilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return nonPrivilegedUsers.contains(username) && (validTokenNumber >= nonPrivilegedUserConcurrentSessionLimit);
+ }
+
+ @Override
+ public void sessionEndedForUser(String username, String token) {
+ if (!token.isEmpty()) {
+ sessionCountModifyLock.lock();
+ try {
+ concurrentSessionCounter.computeIfPresent(username, (key, sessionTokenSet) -> removeTokenFromUser(sessionTokenSet, token));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ }
+ }
+
+ private Set<SessionJWT> removeTokenFromUser(Set<SessionJWT> sessionTokenSet, String token) {
+ sessionTokenSet.removeIf(sessionToken -> sessionToken.getToken().equals(token));
+ if (sessionTokenSet.isEmpty()) {
+ return null;
+ }
+ return sessionTokenSet;
+ }
+
+ private Set<SessionJWT> addTokenForUser(Set<SessionJWT> sessionTokenSet, JWT JWToken) {
Review Comment:
Let's rename the parameter name to jwtToken.
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,168 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class InMemoryConcurrentSessionVerifier implements ConcurrentSessionVerifier {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<SessionJWT>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ @Override
+ public boolean verifySessionForUser(String username, JWT JWToken) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, sessionTokenSet) -> addTokenForUser(sessionTokenSet, JWToken));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ return (int) concurrentSessionCounter
+ .getOrDefault(username, Collections.emptySet())
+ .stream()
+ .filter(each -> !each.hasExpired())
+ .count();
+ }
+
+ private boolean privilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (privilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return privilegedUsers.contains(username) && (validTokenNumber >= privilegedUserConcurrentSessionLimit);
+ }
+
+ private boolean nonPrivilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (nonPrivilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return nonPrivilegedUsers.contains(username) && (validTokenNumber >= nonPrivilegedUserConcurrentSessionLimit);
+ }
+
+ @Override
+ public void sessionEndedForUser(String username, String token) {
+ if (!token.isEmpty()) {
Review Comment:
Let's use StringUtils.isNotBlank instead. This one also checks if the token is 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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] zeroflag commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r939989584
##########
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:
As far as I remember, It was related to removing the session info from the map altogether so that it won't take any space in the Map when the counter goes zero, isn't it?
--
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
[GitHub] [knox] MrtnBalazs commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r939961400
##########
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:
Okay, i will keep that in mind.
--
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
[GitHub] [knox] MrtnBalazs commented on pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on PR #615:
URL: https://github.com/apache/knox/pull/615#issuecomment-1207982564
The plan is to implement the enabling/disabling in the next pull request.
--
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
[GitHub] [knox] zeroflag commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r936927521
##########
gateway-service-knoxssout/pom.xml:
##########
@@ -68,5 +68,9 @@
<artifactId>gateway-test-utils</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.knox</groupId>
Review Comment:
Cool, thanks.
--
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
[GitHub] [knox] MrtnBalazs commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r936838359
##########
gateway-service-knoxssout/pom.xml:
##########
@@ -68,5 +68,9 @@
<artifactId>gateway-test-utils</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.knox</groupId>
Review Comment:
Thats a good idea thank you, i implemented it.
##########
gateway-service-knoxsso/pom.xml:
##########
@@ -81,5 +81,9 @@
<artifactId>gateway-test-utils</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.knox</groupId>
+ <artifactId>gateway-server</artifactId>
Review Comment:
Thats a good idea thank you, i implemented it.
--
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
[GitHub] [knox] pzampino commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
pzampino commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r941771125
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class InMemoryConcurrentSessionVerifier implements ConcurrentSessionVerifier {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<SessionJWT>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ @Override
+ public boolean verifySessionForUser(String username, JWT jwtToken) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, sessionTokenSet) -> addTokenForUser(sessionTokenSet, jwtToken));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ return (int) concurrentSessionCounter
+ .getOrDefault(username, Collections.emptySet())
+ .stream()
+ .filter(each -> !each.hasExpired())
+ .count();
+ }
+
+ private boolean privilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (privilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return privilegedUsers.contains(username) && (validTokenNumber >= privilegedUserConcurrentSessionLimit);
+ }
+
+ private boolean nonPrivilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (nonPrivilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return nonPrivilegedUsers.contains(username) && (validTokenNumber >= nonPrivilegedUserConcurrentSessionLimit);
+ }
+
+ @Override
+ public void sessionEndedForUser(String username, String token) {
+ if (StringUtils.isNotBlank(token)) {
+ sessionCountModifyLock.lock();
+ try {
+ concurrentSessionCounter.computeIfPresent(username, (key, sessionTokenSet) -> removeTokenFromUser(sessionTokenSet, token));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ }
+ }
+
+ private Set<SessionJWT> removeTokenFromUser(Set<SessionJWT> sessionTokenSet, String token) {
+ sessionTokenSet.removeIf(sessionToken -> sessionToken.getToken().equals(token));
+ if (sessionTokenSet.isEmpty()) {
+ return null;
+ }
+ return sessionTokenSet;
+ }
+
+ private Set<SessionJWT> addTokenForUser(Set<SessionJWT> sessionTokenSet, JWT jwtToken) {
+ sessionTokenSet.add(new SessionJWT(jwtToken));
+ return sessionTokenSet;
+ }
+
+ @Override
+ public void init(GatewayConfig config, Map<String, String> options) throws ServiceLifecycleException {
+ this.privilegedUsers = config.getPrivilegedUsers();
+ this.nonPrivilegedUsers = config.getNonPrivilegedUsers();
Review Comment:
Does this represent a configured set of non-privileged users for whom a concurrent session limit will be applied?
If so, why can't any user who is NOT configured as a member of the privileged users set be treated as a non-privileged user? Why do we need a separate configuration for these non-privileged users? What happens to those users who are in neither configured set? Do they get as many sessions as they want?
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class InMemoryConcurrentSessionVerifier implements ConcurrentSessionVerifier {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<SessionJWT>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ @Override
+ public boolean verifySessionForUser(String username, JWT jwtToken) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, sessionTokenSet) -> addTokenForUser(sessionTokenSet, jwtToken));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ return (int) concurrentSessionCounter
+ .getOrDefault(username, Collections.emptySet())
+ .stream()
+ .filter(each -> !each.hasExpired())
+ .count();
+ }
+
+ private boolean privilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (privilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return privilegedUsers.contains(username) && (validTokenNumber >= privilegedUserConcurrentSessionLimit);
+ }
+
+ private boolean nonPrivilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (nonPrivilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return nonPrivilegedUsers.contains(username) && (validTokenNumber >= nonPrivilegedUserConcurrentSessionLimit);
+ }
+
+ @Override
+ public void sessionEndedForUser(String username, String token) {
+ if (StringUtils.isNotBlank(token)) {
+ sessionCountModifyLock.lock();
+ try {
+ concurrentSessionCounter.computeIfPresent(username, (key, sessionTokenSet) -> removeTokenFromUser(sessionTokenSet, token));
Review Comment:
I think this could result in a NullPointerException if the username is null. It may be good to validate that parameter along with the token?
##########
gateway-spi/src/main/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+import org.apache.knox.gateway.services.Service;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+
+public interface ConcurrentSessionVerifier extends Service {
+ boolean verifySessionForUser(String username, JWT JWToken);
Review Comment:
This verify method answers the question of whether or not the specified user is permitted to have a[nother] session? Some javadoc may be helpful here.
--
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
[GitHub] [knox] MrtnBalazs commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r942117470
##########
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:
The users who aren't configured for either of the groups could have sessions and they won't have entry in the map, but i think too that the method could return 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
[GitHub] [knox] MrtnBalazs commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r942261075
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class InMemoryConcurrentSessionVerifier implements ConcurrentSessionVerifier {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<SessionJWT>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ @Override
+ public boolean verifySessionForUser(String username, JWT jwtToken) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, sessionTokenSet) -> addTokenForUser(sessionTokenSet, jwtToken));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ return (int) concurrentSessionCounter
+ .getOrDefault(username, Collections.emptySet())
+ .stream()
+ .filter(each -> !each.hasExpired())
+ .count();
+ }
+
+ private boolean privilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (privilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return privilegedUsers.contains(username) && (validTokenNumber >= privilegedUserConcurrentSessionLimit);
+ }
+
+ private boolean nonPrivilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (nonPrivilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return nonPrivilegedUsers.contains(username) && (validTokenNumber >= nonPrivilegedUserConcurrentSessionLimit);
+ }
+
+ @Override
+ public void sessionEndedForUser(String username, String token) {
+ if (StringUtils.isNotBlank(token)) {
+ sessionCountModifyLock.lock();
+ try {
+ concurrentSessionCounter.computeIfPresent(username, (key, sessionTokenSet) -> removeTokenFromUser(sessionTokenSet, token));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ }
+ }
+
+ private Set<SessionJWT> removeTokenFromUser(Set<SessionJWT> sessionTokenSet, String token) {
+ sessionTokenSet.removeIf(sessionToken -> sessionToken.getToken().equals(token));
+ if (sessionTokenSet.isEmpty()) {
+ return null;
+ }
+ return sessionTokenSet;
+ }
+
+ private Set<SessionJWT> addTokenForUser(Set<SessionJWT> sessionTokenSet, JWT jwtToken) {
+ sessionTokenSet.add(new SessionJWT(jwtToken));
+ return sessionTokenSet;
+ }
+
+ @Override
+ public void init(GatewayConfig config, Map<String, String> options) throws ServiceLifecycleException {
+ this.privilegedUsers = config.getPrivilegedUsers();
+ this.nonPrivilegedUsers = config.getNonPrivilegedUsers();
Review Comment:
Yes, there are two groups privileged and non-privileged, each has a limit. Users who are in neither of the groups can have unlimited sessions. @smolnar82 mentioned at the begging of the task that there might be overlapping between the groups so we need to have two groups configured, but if you suggest that configuring one group is enough, than there might have been some misunderstanding in this topic.
--
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
[GitHub] [knox] smolnar82 commented on pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on PR #615:
URL: https://github.com/apache/knox/pull/615#issuecomment-1219343595
The follow-up JIRA is created here: https://issues.apache.org/jira/browse/KNOX-2790
--
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
[GitHub] [knox] zeroflag commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r935527290
##########
gateway-service-knoxssout/pom.xml:
##########
@@ -68,5 +68,9 @@
<artifactId>gateway-test-utils</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.knox</groupId>
Review Comment:
Which class is referenced from gateway-server from gateway-service-knoxssout?
##########
gateway-service-knoxsso/pom.xml:
##########
@@ -81,5 +81,9 @@
<artifactId>gateway-test-utils</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.knox</groupId>
+ <artifactId>gateway-server</artifactId>
Review Comment:
Which class is referenced from gateway-server from gateway-service-knoxssou?
--
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
[GitHub] [knox] zeroflag commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r937682920
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class InMemoryConcurrentSessionVerifier implements ConcurrentSessionVerifier {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<SessionJWT>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ @Override
+ public boolean verifySessionForUser(String username, JWT JWToken) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, sessionTokenSet) -> addTokenForUser(sessionTokenSet, JWToken));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ int result = 0;
+ Set<SessionJWT> tokens = concurrentSessionCounter.getOrDefault(username, Collections.emptySet());
+ for (SessionJWT token : tokens) {
+ if (!token.hasExpired()) {
+ result++;
+ }
+ }
+ return result;
+ }
+
+ private boolean privilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (privilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return privilegedUsers.contains(username) && (validTokenNumber >= privilegedUserConcurrentSessionLimit);
+ }
+
+ private boolean nonPrivilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (nonPrivilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return nonPrivilegedUsers.contains(username) && (validTokenNumber >= nonPrivilegedUserConcurrentSessionLimit);
+ }
+
+ @Override
+ public void sessionEndedForUser(String username, String token) {
+ if (!token.isEmpty()) {
+ sessionCountModifyLock.lock();
+ try {
+ concurrentSessionCounter.computeIfPresent(username, (key, sessionTokenSet) -> removeTokenFromUser(sessionTokenSet, token));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ }
+ }
+
+ private Set<SessionJWT> removeTokenFromUser(Set<SessionJWT> sessionTokenSet, String token) {
+ sessionTokenSet.removeIf(sessionToken -> sessionToken.getToken().equals(token));
+ if (sessionTokenSet.isEmpty()) {
+ return null;
+ }
+ return sessionTokenSet;
+ }
+
+ private Set<SessionJWT> addTokenForUser(Set<SessionJWT> sessionTokenSet, JWT JWToken) {
+ sessionTokenSet.add(new SessionJWT(JWToken));
+ return sessionTokenSet;
+ }
+
+ @Override
+ public void init(GatewayConfig config, Map<String, String> options) throws ServiceLifecycleException {
+ this.privilegedUsers = config.getPrivilegedUsers();
+ this.nonPrivilegedUsers = config.getNonPrivilegedUsers();
+ this.privilegedUserConcurrentSessionLimit = config.getPrivilegedUsersConcurrentSessionLimit();
+ this.nonPrivilegedUserConcurrentSessionLimit = config.getNonPrivilegedUsersConcurrentSessionLimit();
+ this.concurrentSessionCounter = new ConcurrentHashMap<>();
+ }
+
+ @Override
+ public void start() throws ServiceLifecycleException {
+
+ }
+
+ @Override
+ public void stop() throws ServiceLifecycleException {
+
+ }
+
+ Integer getUserConcurrentSessionCount(String username) {
+ int result = countValidTokensForUser(username);
+ return (result == 0) ? null : result;
+ }
+
+ public static class SessionJWT {
Review Comment:
We should add (generate) equals/hashCode methods for this. In IntelliJ you can click on Code / Generate and select Equals and Hash Code. The only field we need to include is `String token`.
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class InMemoryConcurrentSessionVerifier implements ConcurrentSessionVerifier {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<SessionJWT>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ @Override
+ public boolean verifySessionForUser(String username, JWT JWToken) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, sessionTokenSet) -> addTokenForUser(sessionTokenSet, JWToken));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
Review Comment:
I think it's a matter of preference but with the stream API this would look like this.
```java
return (int) concurrentSessionCounter
.getOrDefault(username, Collections.emptySet())
.stream()
.filter(each -> !each.hasExpired())
.count();
```
But for loop if fine too.
##########
gateway-service-knoxssout/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOutResource.java:
##########
@@ -106,6 +111,21 @@ private boolean removeAuthenticationToken(HttpServletResponse response) {
}
response.addCookie(c);
+ Cookie[] cookies = request.getCookies();
+ if (cookies != null) {
+ Optional<Cookie> SSOCookie = Arrays.stream(cookies).filter(cookie -> cookie.getName().equals(cookieName)).findFirst();
Review Comment:
I know it looks weird in both way, but I would rather name it `ssoCookie` than `SSOCookie`. It's still a local variable.
##########
gateway-service-knoxssout/src/main/java/org/apache/knox/gateway/service/knoxsso/KnoxSSOutMessages.java:
##########
@@ -25,4 +25,7 @@
public interface KnoxSSOutMessages {
@Message( level = MessageLevel.INFO, text = "There was a problem determining the SSO cookie domain - using default domain.")
void problemWithCookieDomainUsingDefault();
+
+ @Message(level = MessageLevel.DEBUG, text = "Could not find cookie with the name: {0} in the request to be removed from the concurrent session counter for user: {1}. ")
+ void couldNotFindCookieWithTokenToRemove(String cookieName, String username);
Review Comment:
I think this should be at least INFO or WARNING.
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class InMemoryConcurrentSessionVerifier implements ConcurrentSessionVerifier {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<SessionJWT>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ @Override
+ public boolean verifySessionForUser(String username, JWT JWToken) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, sessionTokenSet) -> addTokenForUser(sessionTokenSet, JWToken));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ int result = 0;
+ Set<SessionJWT> tokens = concurrentSessionCounter.getOrDefault(username, Collections.emptySet());
+ for (SessionJWT token : tokens) {
+ if (!token.hasExpired()) {
+ result++;
+ }
+ }
+ return result;
+ }
+
+ private boolean privilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (privilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return privilegedUsers.contains(username) && (validTokenNumber >= privilegedUserConcurrentSessionLimit);
+ }
+
+ private boolean nonPrivilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (nonPrivilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return nonPrivilegedUsers.contains(username) && (validTokenNumber >= nonPrivilegedUserConcurrentSessionLimit);
+ }
+
+ @Override
+ public void sessionEndedForUser(String username, String token) {
+ if (!token.isEmpty()) {
+ sessionCountModifyLock.lock();
+ try {
+ concurrentSessionCounter.computeIfPresent(username, (key, sessionTokenSet) -> removeTokenFromUser(sessionTokenSet, token));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ }
+ }
+
+ private Set<SessionJWT> removeTokenFromUser(Set<SessionJWT> sessionTokenSet, String token) {
+ sessionTokenSet.removeIf(sessionToken -> sessionToken.getToken().equals(token));
+ if (sessionTokenSet.isEmpty()) {
+ return null;
+ }
+ return sessionTokenSet;
+ }
+
+ private Set<SessionJWT> addTokenForUser(Set<SessionJWT> sessionTokenSet, JWT JWToken) {
+ sessionTokenSet.add(new SessionJWT(JWToken));
+ return sessionTokenSet;
+ }
+
+ @Override
+ public void init(GatewayConfig config, Map<String, String> options) throws ServiceLifecycleException {
+ this.privilegedUsers = config.getPrivilegedUsers();
+ this.nonPrivilegedUsers = config.getNonPrivilegedUsers();
+ this.privilegedUserConcurrentSessionLimit = config.getPrivilegedUsersConcurrentSessionLimit();
+ this.nonPrivilegedUserConcurrentSessionLimit = config.getNonPrivilegedUsersConcurrentSessionLimit();
+ this.concurrentSessionCounter = new ConcurrentHashMap<>();
+ }
+
+ @Override
+ public void start() throws ServiceLifecycleException {
+
+ }
+
+ @Override
+ public void stop() throws ServiceLifecycleException {
+
+ }
+
+ Integer getUserConcurrentSessionCount(String username) {
+ int result = countValidTokensForUser(username);
+ return (result == 0) ? null : result;
+ }
+
+ public static class SessionJWT {
+ private final Date expiry;
+ private final String token;
+
+ public SessionJWT(JWT token) {
+ this.expiry = token.getExpiresDate();
+ this.token = token.toString();
+ }
+
+ public Date getExpiry() {
Review Comment:
If this isn't used, let's remove it.
##########
gateway-service-knoxssout/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOutResource.java:
##########
@@ -106,6 +111,21 @@ private boolean removeAuthenticationToken(HttpServletResponse response) {
}
response.addCookie(c);
+ Cookie[] cookies = request.getCookies();
+ if (cookies != null) {
+ Optional<Cookie> SSOCookie = Arrays.stream(cookies).filter(cookie -> cookie.getName().equals(cookieName)).findFirst();
+ if (SSOCookie.isPresent()) {
+ GatewayServices gwServices =
+ (GatewayServices) request.getServletContext().getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE);
+ if (gwServices != null) {
+ ConcurrentSessionVerifier verifier = gwServices.getService(ServiceType.CONCURRENT_SESSION_VERIFIER);
+ verifier.sessionEndedForUser(request.getUserPrincipal().getName(), SSOCookie.get().getValue());
Review Comment:
Do we always have a ConcurrentSessionVerifier? I see at other places we check if it's null or not. Probably we need to add a null check here too.
--
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
[GitHub] [knox] pzampino commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
pzampino commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r941747723
##########
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:
Whether or not the count is stored in the map (implementation detail), the contract of the method could be to return zero. If there is no entry in the map, that also means there are zero sessions, correct?
--
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
[GitHub] [knox] MrtnBalazs commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r939962910
##########
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:
It is not populated, good idea to change it to Collections.emptyMap().
--
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
[GitHub] [knox] MrtnBalazs commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r940022614
##########
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:
Yes, because originally there were no tokens stored and we didn't have to count the not expired ones, so we just returned the value from the HashMap for the user, which returned null if the user was not in the HashMap. Now we have to count the not expired tokens so this function might be unnecessary, we could add countValidTokensForUser(username) function package visibility and use it in the test instead of getUserConcurrentSession(username).
--
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
[GitHub] [knox] MrtnBalazs commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r939974098
##########
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:
Originally it returned 0, but me and @smolnar82 reviewed the code and decided to return null instead. It was in the previous pull request: https://github.com/apache/knox/pull/608/commits/af16b58d8c3bcbd9c40841b7c92416ab0bff97f6#diff-cf1fb21462bed96d5f2087f2729a659a2b4141b20536e114a72b24f9d82d4356.
Unfortunately, we talked via Zoom so i can not remember the exact reason, but right now i can't see any reason why returning 0 would not be good.
--
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
[GitHub] [knox] smolnar82 commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r948925999
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class InMemoryConcurrentSessionVerifier implements ConcurrentSessionVerifier {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<SessionJWT>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ @Override
+ public boolean verifySessionForUser(String username, JWT jwtToken) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, sessionTokenSet) -> addTokenForUser(sessionTokenSet, jwtToken));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ return (int) concurrentSessionCounter
+ .getOrDefault(username, Collections.emptySet())
+ .stream()
+ .filter(each -> !each.hasExpired())
+ .count();
+ }
+
+ private boolean privilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (privilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return privilegedUsers.contains(username) && (validTokenNumber >= privilegedUserConcurrentSessionLimit);
+ }
+
+ private boolean nonPrivilegedUserCheckLimitReached(String username, int validTokenNumber) {
+ if (nonPrivilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return nonPrivilegedUsers.contains(username) && (validTokenNumber >= nonPrivilegedUserConcurrentSessionLimit);
+ }
+
+ @Override
+ public void sessionEndedForUser(String username, String token) {
+ if (StringUtils.isNotBlank(token)) {
+ sessionCountModifyLock.lock();
+ try {
+ concurrentSessionCounter.computeIfPresent(username, (key, sessionTokenSet) -> removeTokenFromUser(sessionTokenSet, token));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ }
+ }
+
+ private Set<SessionJWT> removeTokenFromUser(Set<SessionJWT> sessionTokenSet, String token) {
+ sessionTokenSet.removeIf(sessionToken -> sessionToken.getToken().equals(token));
+ if (sessionTokenSet.isEmpty()) {
+ return null;
+ }
+ return sessionTokenSet;
+ }
+
+ private Set<SessionJWT> addTokenForUser(Set<SessionJWT> sessionTokenSet, JWT jwtToken) {
+ sessionTokenSet.add(new SessionJWT(jwtToken));
+ return sessionTokenSet;
+ }
+
+ @Override
+ public void init(GatewayConfig config, Map<String, String> options) throws ServiceLifecycleException {
+ this.privilegedUsers = config.getPrivilegedUsers();
+ this.nonPrivilegedUsers = config.getNonPrivilegedUsers();
Review Comment:
https://issues.apache.org/jira/browse/KNOX-2789
--
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
[GitHub] [knox] MrtnBalazs commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r936320976
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.Service;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+import org.apache.knox.gateway.services.security.token.impl.JWTToken;
+
+import java.text.ParseException;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class ConcurrentSessionVerifier implements Service {
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Set<String>> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ public boolean verifySessionForUser(String username, String token) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ int validTokenNumber = countValidTokensForUser(username);
+ if (privilegedUserCheckLimitReached(username, validTokenNumber) || nonPrivilegedUserCheckLimitReached(username, validTokenNumber)) {
+ return false;
+ }
+ concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
+ concurrentSessionCounter.compute(username, (key, tokenSet) -> addTokenForUser(tokenSet, token));
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private int countValidTokensForUser(String username) {
+ int result = 0;
+ Set<String> tokens = concurrentSessionCounter.getOrDefault(username, null);
Review Comment:
Yes, thats way better.
--
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
[GitHub] [knox] MrtnBalazs commented on pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on PR #615:
URL: https://github.com/apache/knox/pull/615#issuecomment-1202345643
@zeroflag
--
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
[GitHub] [knox] MrtnBalazs commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r940003961
##########
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:
The idea was to prove that if the user is in neither of the groups, neither of the limits apply to the user. To test that i verify the user one more time than the higher limit. In a later commit i took out the for cycle because we give the function a token as well, and the tokens are class variables and pre issued in the setUp() function. I'll add some comments to clarify the test and rename it "testuserIsInNeitherOfTheGroupsCanBeLoggedInUnlimitedTimes".
--
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
[GitHub] [knox] MrtnBalazs commented on a diff in pull request #615: KNOX-2778 - Enforce concurrent session limit in KnoxSSO
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r942117938
##########
gateway-spi/src/main/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.session.control;
+
+import org.apache.knox.gateway.services.Service;
+import org.apache.knox.gateway.services.security.token.impl.JWT;
+
+public interface ConcurrentSessionVerifier extends Service {
+ boolean verifySessionForUser(String username, JWT JWToken);
Review Comment:
Yes, okay.
--
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