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/07/14 15:33:53 UTC
[GitHub] [knox] MrtnBalazs opened a new pull request, #608: KNOX-2777 - I've added parameters for dealing with concurrent session…
MrtnBalazs opened a new pull request, #608:
URL: https://github.com/apache/knox/pull/608
## What changes were proposed in this pull request?
I’ve added implementation in `GatewayConfig` , `GatewayConfigImpl` and `GatewayTestConfig` to receive the following parameters from the gateway-site.xml file.
`gateway.non.privileged.users`
`gateway.privileged.users`
`gateway.privileged.user.concurrent.session.limit`
`gateway.non.privileged.user.concurrent.session.limit`
An example:
```
<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.user.concurrent.session.limit</name>
<value>3</value>
</property>
<property>
<name>gateway.non.privileged.user.concurrent.session.limit</name>
<value>2</value>
</property>
```
It is not yet used, but it is necessary for the jira 2778 which depends on this one.
I’ve also created `ConcurrentSessionVerifier` class which will be used to keep track of the concurrent sessions of the users and verify whether they fit in the limit (based on their group) or not.
It is not yet wired in, but also necessary for the jira 2778.
## How was this patch tested?
I’ve written unit tests for the new parameters in `GatewayConfigImplTest` class where I test the default parameters and what if the user adds normal and abnormal parameters. Result of the tests:
```
[INFO]
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.knox.gateway.config.impl.GatewayConfigImplTest
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.055 s - in org.apache.knox.gateway.config.impl.GatewayConfigImplTest
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 8.831 s
[INFO] Finished at: 2022-07-14T16:49:26+02:00
[INFO] ------------------------------------------------------------------------
```
I’ve also written unit tests for the `ConcurrentSessionVerifier` class in the `ConcurrentSessionVerifierTest` class. These tests test the following:
-The user is in neither of the two groups
-The user is in both groups
-User is privileged and goes over the session limit, then ends a session and goes over the limit again
-Same with non privileged users
-One of the limits is 0
-We end sessions before even creating them
Result of the tests:
```
[INFO]
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.knox.gateway.util.ConcurrentSessionVerifierTest
[INFO] Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.003 s - in org.apache.knox.gateway.util.ConcurrentSessionVerifierTest
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 7, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 5.304 s
[INFO] Finished at: 2022-07-14T17:07:34+02:00
[INFO] ------------------------------------------------------------------------
```
--
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] moresandeep commented on pull request #608: KNOX-2777 - Implement concurrent session verifier
Posted by GitBox <gi...@apache.org>.
moresandeep commented on PR #608:
URL: https://github.com/apache/knox/pull/608#issuecomment-1184764573
Would be better to update the title indicating this is just adding configs to support this feature i.e. "Add configurations for concurrent session verifier feature" - feel free to change the wordings.
--
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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r924568288
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.util;
+
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new ConcurrentHashMap<>();
+ private static final Object lock = new Object();
+
+ private ConcurrentSessionVerifier(){}
+
+ public static synchronized void init(Set<String> privilegedUsers, Set<String> nonPrivilegedUsers, int privilegedUserConcurrentSessionLimit, int nonPrivilegedUserConcurrentSessionLimit){
+ ConcurrentSessionVerifier.privilegedUsers = privilegedUsers;
+ ConcurrentSessionVerifier.nonPrivilegedUsers = nonPrivilegedUsers;
+ ConcurrentSessionVerifier.privilegedUserConcurrentSessionLimit = privilegedUserConcurrentSessionLimit;
+ ConcurrentSessionVerifier.nonPrivilegedUserConcurrentSessionLimit = nonPrivilegedUserConcurrentSessionLimit;
+ concurrentSessionCounter = new ConcurrentHashMap<>();
+ }
+
+ public static boolean verifySessionForUser(String username){
+ if(!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+ synchronized(lock) {
+ if (!concurrentSessionCounter.containsKey(username)) {
Review Comment:
Thank you, it's way nicer that way
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.util;
+
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new ConcurrentHashMap<>();
+ private static final Object lock = new Object();
+
+ private ConcurrentSessionVerifier(){}
+
+ public static synchronized void init(Set<String> privilegedUsers, Set<String> nonPrivilegedUsers, int privilegedUserConcurrentSessionLimit, int nonPrivilegedUserConcurrentSessionLimit){
+ ConcurrentSessionVerifier.privilegedUsers = privilegedUsers;
+ ConcurrentSessionVerifier.nonPrivilegedUsers = nonPrivilegedUsers;
+ ConcurrentSessionVerifier.privilegedUserConcurrentSessionLimit = privilegedUserConcurrentSessionLimit;
+ ConcurrentSessionVerifier.nonPrivilegedUserConcurrentSessionLimit = nonPrivilegedUserConcurrentSessionLimit;
+ concurrentSessionCounter = new ConcurrentHashMap<>();
+ }
+
+ public static boolean verifySessionForUser(String username){
+ if(!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+ synchronized(lock) {
+ if (!concurrentSessionCounter.containsKey(username)) {
+ concurrentSessionCounter.put(username, 0);
+ }
+ if ((privilegedUsers.contains(username) && !(concurrentSessionCounter.get(username) < privilegedUserConcurrentSessionLimit)) || (nonPrivilegedUsers.contains(username) && !(concurrentSessionCounter.get(username) < nonPrivilegedUserConcurrentSessionLimit))) {
+ return false;
+ }
+ incrementConcurrentSessionCount(username);
+ }
+ return true;
+ }
+
+ private static synchronized void incrementConcurrentSessionCount(String username){
+ int count = concurrentSessionCounter.get(username);
Review Comment:
Same
--
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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r921948366
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.util;
+
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new HashMap<>();
+
+ private ConcurrentSessionVerifier(){}
+
+ public static void init(Set<String> _privilegedUsers, Set<String> _nonPrivilegedUsers, int _privilegedUserConcurrentSessionLimit, int _nonPrivilegedUserConcurrentSessionLimit){
+ privilegedUsers = _privilegedUsers;
+ nonPrivilegedUsers = _nonPrivilegedUsers;
+ privilegedUserConcurrentSessionLimit = _privilegedUserConcurrentSessionLimit;
+ nonPrivilegedUserConcurrentSessionLimit = _nonPrivilegedUserConcurrentSessionLimit;
+ concurrentSessionCounter = new HashMap<>();
+ }
+
+ public static boolean verifySessionForUser(String username){
+ if(!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+ if(!concurrentSessionCounter.containsKey(username)){
+ concurrentSessionCounter.put(username, 0);
+ }
+
+ if((privilegedUsers.contains(username) && !(concurrentSessionCounter.get(username) < privilegedUserConcurrentSessionLimit)) || (nonPrivilegedUsers.contains(username) && !(concurrentSessionCounter.get(username) < nonPrivilegedUserConcurrentSessionLimit))){
+ return false;
+ }
+
+ incrementConcurrentSessionCount(username);
+ return true;
+ }
+
+ private static void incrementConcurrentSessionCount(String username){
+ int count = concurrentSessionCounter.get(username);
+ count++;
+ concurrentSessionCounter.put(username, count);
+ }
+
+ public static void sessionEndedForUser(String username){
Review Comment:
Yes, this class should be thread safe
--
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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r922044913
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.util;
+
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new HashMap<>();
+
+ private ConcurrentSessionVerifier(){}
+
+ public static void init(Set<String> _privilegedUsers, Set<String> _nonPrivilegedUsers, int _privilegedUserConcurrentSessionLimit, int _nonPrivilegedUserConcurrentSessionLimit){
+ privilegedUsers = _privilegedUsers;
+ nonPrivilegedUsers = _nonPrivilegedUsers;
+ privilegedUserConcurrentSessionLimit = _privilegedUserConcurrentSessionLimit;
+ nonPrivilegedUserConcurrentSessionLimit = _nonPrivilegedUserConcurrentSessionLimit;
+ concurrentSessionCounter = new HashMap<>();
+ }
+
+ public static boolean verifySessionForUser(String username){
Review Comment:
I modified it to be thread safe.
--
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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r922043423
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.util;
+
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new HashMap<>();
Review Comment:
Yes changed it to ConcurrentHashMap.
--
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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r925412941
##########
gateway-spi-common/src/main/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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 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 {
+ public static final ConcurrentSessionVerifier INSTANCE = new ConcurrentSessionVerifier();
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Integer> concurrentSessionCounter;
+ private Lock sessionCountModifyLock = new ReentrantLock();
Review Comment:
nit: this can be final.
--
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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r924449899
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.util;
+
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+public class ConcurrentSessionVerifier {
Review Comment:
Instead of using static methods and static fields, let's have a normal non static class with instance variables and non static methods. ~~Constants like `privilegedUserConcurrentSessionLimit`/`privilegedUserConcurrentSessionLimit` should be kept as statics but~~ `concurrentSessionCounter` should be non static.
Then we can create one instance of this class and use that at each places where needed. Either by creating an instance somewhere in GatewayServer's `main()` and passing that around or by making a singleton. I prefer the former but that's up to how the code is structured. We can decide that later when we start using 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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r922044312
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.util;
+
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new HashMap<>();
+
+ private ConcurrentSessionVerifier(){}
+
+ public static void init(Set<String> _privilegedUsers, Set<String> _nonPrivilegedUsers, int _privilegedUserConcurrentSessionLimit, int _nonPrivilegedUserConcurrentSessionLimit){
+ privilegedUsers = _privilegedUsers;
+ nonPrivilegedUsers = _nonPrivilegedUsers;
+ privilegedUserConcurrentSessionLimit = _privilegedUserConcurrentSessionLimit;
+ nonPrivilegedUserConcurrentSessionLimit = _nonPrivilegedUserConcurrentSessionLimit;
+ concurrentSessionCounter = new HashMap<>();
Review Comment:
Yes changed it to ConcurrentHashMap.
--
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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r922042977
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -293,6 +293,14 @@ public class GatewayConfigImpl extends Configuration implements GatewayConfig {
private static final String GATEWAY_DATABASE_VERIFY_SERVER_CERT = GATEWAY_CONFIG_FILE_PREFIX + ".database.ssl.verify.server.cert";
private static final String GATEWAY_DATABASE_TRUSTSTORE_FILE = GATEWAY_CONFIG_FILE_PREFIX + ".database.ssl.truststore.file";
+ // Concurrent session properties
+ private static final String GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT = GATEWAY_CONFIG_FILE_PREFIX + ".privileged.user.concurrent.session.limit";
Review Comment:
Changed it.
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -293,6 +293,14 @@ public class GatewayConfigImpl extends Configuration implements GatewayConfig {
private static final String GATEWAY_DATABASE_VERIFY_SERVER_CERT = GATEWAY_CONFIG_FILE_PREFIX + ".database.ssl.verify.server.cert";
private static final String GATEWAY_DATABASE_TRUSTSTORE_FILE = GATEWAY_CONFIG_FILE_PREFIX + ".database.ssl.truststore.file";
+ // Concurrent session properties
+ private static final String GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT = GATEWAY_CONFIG_FILE_PREFIX + ".privileged.user.concurrent.session.limit";
+ private static final String GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT = GATEWAY_CONFIG_FILE_PREFIX + ".non.privileged.user.concurrent.session.limit";
+ private static final int GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT = 3;
+ private static final int GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT = 2;
+ private static final String GATEWAY_PRIVILEGED_USERS = GATEWAY_CONFIG_FILE_PREFIX + ".privileged.users";
+ private static final String GATEWAY_NON_PRIVILEGED_USERS = GATEWAY_CONFIG_FILE_PREFIX + ".non.privileged.users";
Review Comment:
Changed it.
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -1335,4 +1343,49 @@ public int getJettyMaxFormKeys() {
return getInt(JETTY_MAX_FORM_KEYS, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}
+ @Override
+ public int getPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
Review Comment:
Added them.
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -1335,4 +1343,49 @@ public int getJettyMaxFormKeys() {
return getInt(JETTY_MAX_FORM_KEYS, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}
+ @Override
+ public int getPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public int getNonPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
Review Comment:
Added them.
--
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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r925488891
##########
gateway-spi-common/src/main/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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 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 {
+ public static final ConcurrentSessionVerifier INSTANCE = new ConcurrentSessionVerifier();
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Integer> concurrentSessionCounter;
+ private Lock sessionCountModifyLock = new ReentrantLock();
Review Comment:
Then we would have to give them value in the constructor which is private, because of the singleton pattern and in the test cases we would not be able to initialize the same verifier multiple times.
--
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 merged pull request #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
zeroflag merged PR #608:
URL: https://github.com/apache/knox/pull/608
--
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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r926475787
##########
gateway-spi-common/src/main/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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 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 {
+ public static final ConcurrentSessionVerifier INSTANCE = new ConcurrentSessionVerifier();
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Integer> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ private ConcurrentSessionVerifier() {
+ }
+
+ public static ConcurrentSessionVerifier getInstance() {
+ return INSTANCE;
+ }
+
+ public void init(GatewayConfig config) {
+ this.privilegedUsers = config.getPrivilegedUsers();
+ this.nonPrivilegedUsers = config.getNonPrivilegedUsers();
+ this.privilegedUserConcurrentSessionLimit = config.getPrivilegedUsersConcurrentSessionLimit();
+ this.nonPrivilegedUserConcurrentSessionLimit = config.getNonPrivilegedUsersConcurrentSessionLimit();
+ this.concurrentSessionCounter = new ConcurrentHashMap<>();
+ }
+
+ public boolean verifySessionForUser(String username) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ concurrentSessionCounter.putIfAbsent(username, 0);
+ if (privilegedUserCheckLimitReached(username) || nonPrivilegedUserCheckLimitReached(username)) {
+ return false;
+ }
+ concurrentSessionCounter.compute(username, (key, value) -> value + 1);
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private boolean privilegedUserCheckLimitReached(String username) {
+ if (privilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return privilegedUsers.contains(username) && (concurrentSessionCounter.get(username) >= privilegedUserConcurrentSessionLimit);
+ }
+
+ private boolean nonPrivilegedUserCheckLimitReached(String username) {
+ if (nonPrivilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return nonPrivilegedUsers.contains(username) && (concurrentSessionCounter.get(username) >= nonPrivilegedUserConcurrentSessionLimit);
+ }
+
+ public void sessionEndedForUser(String username) {
+ if (concurrentSessionCounter.containsKey(username)) {
+ sessionCountModifyLock.lock();
+ try {
+ int count = concurrentSessionCounter.get(username);
Review Comment:
We can probably use something like this here:
```java
m.computeIfPresent(username, (user,counter) -> counter - 1);
```
But if the counter reaches zero, I think we should delete the user from the map altogether.
--
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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r926499605
##########
gateway-spi-common/src/main/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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 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 {
+ public static final ConcurrentSessionVerifier INSTANCE = new ConcurrentSessionVerifier();
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Integer> concurrentSessionCounter;
+ private final Lock sessionCountModifyLock = new ReentrantLock();
+
+ private ConcurrentSessionVerifier() {
+ }
+
+ public static ConcurrentSessionVerifier getInstance() {
+ return INSTANCE;
+ }
+
+ public void init(GatewayConfig config) {
+ this.privilegedUsers = config.getPrivilegedUsers();
+ this.nonPrivilegedUsers = config.getNonPrivilegedUsers();
+ this.privilegedUserConcurrentSessionLimit = config.getPrivilegedUsersConcurrentSessionLimit();
+ this.nonPrivilegedUserConcurrentSessionLimit = config.getNonPrivilegedUsersConcurrentSessionLimit();
+ this.concurrentSessionCounter = new ConcurrentHashMap<>();
+ }
+
+ public boolean verifySessionForUser(String username) {
+ if (!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+
+ sessionCountModifyLock.lock();
+ try {
+ concurrentSessionCounter.putIfAbsent(username, 0);
+ if (privilegedUserCheckLimitReached(username) || nonPrivilegedUserCheckLimitReached(username)) {
+ return false;
+ }
+ concurrentSessionCounter.compute(username, (key, value) -> value + 1);
+ } finally {
+ sessionCountModifyLock.unlock();
+ }
+ return true;
+ }
+
+ private boolean privilegedUserCheckLimitReached(String username) {
+ if (privilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return privilegedUsers.contains(username) && (concurrentSessionCounter.get(username) >= privilegedUserConcurrentSessionLimit);
+ }
+
+ private boolean nonPrivilegedUserCheckLimitReached(String username) {
+ if (nonPrivilegedUserConcurrentSessionLimit < 0) {
+ return false;
+ }
+ return nonPrivilegedUsers.contains(username) && (concurrentSessionCounter.get(username) >= nonPrivilegedUserConcurrentSessionLimit);
+ }
+
+ public void sessionEndedForUser(String username) {
+ if (concurrentSessionCounter.containsKey(username)) {
+ sessionCountModifyLock.lock();
+ try {
+ int count = concurrentSessionCounter.get(username);
Review Comment:
I agree; this is a better approach. Nice catch, Attila! :)
--
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] moresandeep commented on a diff in pull request #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
moresandeep commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r921454648
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -293,6 +293,14 @@ public class GatewayConfigImpl extends Configuration implements GatewayConfig {
private static final String GATEWAY_DATABASE_VERIFY_SERVER_CERT = GATEWAY_CONFIG_FILE_PREFIX + ".database.ssl.verify.server.cert";
private static final String GATEWAY_DATABASE_TRUSTSTORE_FILE = GATEWAY_CONFIG_FILE_PREFIX + ".database.ssl.truststore.file";
+ // Concurrent session properties
+ private static final String GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT = GATEWAY_CONFIG_FILE_PREFIX + ".privileged.user.concurrent.session.limit";
Review Comment:
`priviledged.user` can be a static variable.
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -293,6 +293,14 @@ public class GatewayConfigImpl extends Configuration implements GatewayConfig {
private static final String GATEWAY_DATABASE_VERIFY_SERVER_CERT = GATEWAY_CONFIG_FILE_PREFIX + ".database.ssl.verify.server.cert";
private static final String GATEWAY_DATABASE_TRUSTSTORE_FILE = GATEWAY_CONFIG_FILE_PREFIX + ".database.ssl.truststore.file";
+ // Concurrent session properties
+ private static final String GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT = GATEWAY_CONFIG_FILE_PREFIX + ".privileged.user.concurrent.session.limit";
+ private static final String GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT = GATEWAY_CONFIG_FILE_PREFIX + ".non.privileged.user.concurrent.session.limit";
+ private static final int GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT = 3;
+ private static final int GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT = 2;
+ private static final String GATEWAY_PRIVILEGED_USERS = GATEWAY_CONFIG_FILE_PREFIX + ".privileged.users";
+ private static final String GATEWAY_NON_PRIVILEGED_USERS = GATEWAY_CONFIG_FILE_PREFIX + ".non.privileged.users";
Review Comment:
nit: `non.priviledged.user` can be a static variable (using the previous static variable) :)
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -1335,4 +1343,49 @@ public int getJettyMaxFormKeys() {
return getInt(JETTY_MAX_FORM_KEYS, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}
+ @Override
+ public int getPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
Review Comment:
nit: brackets :)
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -1335,4 +1343,49 @@ public int getJettyMaxFormKeys() {
return getInt(JETTY_MAX_FORM_KEYS, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}
+ @Override
+ public int getPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public int getNonPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public Set<String> getPrivilegedUsers(){
+ Set<String> set = new HashSet<>();
+
+ String value = get( GATEWAY_PRIVILEGED_USERS );
+ if ((value != null) && (!value.trim().equals("")) ) {
Review Comment:
You can use `StringUtils.isBlank(value)` here (commons3 utils)
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -1335,4 +1343,49 @@ public int getJettyMaxFormKeys() {
return getInt(JETTY_MAX_FORM_KEYS, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}
+ @Override
+ public int getPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public int getNonPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
Review Comment:
nit: brackets
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -1335,4 +1343,49 @@ public int getJettyMaxFormKeys() {
return getInt(JETTY_MAX_FORM_KEYS, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}
+ @Override
+ public int getPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public int getNonPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public Set<String> getPrivilegedUsers(){
+ Set<String> set = new HashSet<>();
+
+ String value = get( GATEWAY_PRIVILEGED_USERS );
+ if ((value != null) && (!value.trim().equals("")) ) {
+ List<String> temp = new ArrayList<>(Arrays.asList(value.split(",")));
+ temp.forEach(e -> { temp.set(temp.indexOf(e), e.trim()); });
Review Comment:
Interesting, you don't get a ConcurrentModificationException here modifying list in place.
You can slightly improve it by
`temp.replaceAll(String::trim)` or if this does not work
`temp.stream().map(String::trim).collect(Collectors.toList())`
P.S. pardon any syntax errors I did not check it in my IDE
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -1335,4 +1343,49 @@ public int getJettyMaxFormKeys() {
return getInt(JETTY_MAX_FORM_KEYS, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}
+ @Override
+ public int getPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public int getNonPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public Set<String> getPrivilegedUsers(){
+ Set<String> set = new HashSet<>();
+
+ String value = get( GATEWAY_PRIVILEGED_USERS );
+ if ((value != null) && (!value.trim().equals("")) ) {
+ List<String> temp = new ArrayList<>(Arrays.asList(value.split(",")));
+ temp.forEach(e -> { temp.set(temp.indexOf(e), e.trim()); });
+ set.addAll(temp);
+ }
+
+ return set;
+ }
+
+ @Override
+ public Set<String> getNonPrivilegedUsers(){
Review Comment:
All the same comments as above.
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.util;
+
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new HashMap<>();
+
+ private ConcurrentSessionVerifier(){}
+
+ public static void init(Set<String> _privilegedUsers, Set<String> _nonPrivilegedUsers, int _privilegedUserConcurrentSessionLimit, int _nonPrivilegedUserConcurrentSessionLimit){
Review Comment:
We don't use _ in front of variable names, I see that you did not use it elsewhere so avoid using it just to be more consistant.
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.util;
+
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new HashMap<>();
+
+ private ConcurrentSessionVerifier(){}
+
+ public static void init(Set<String> _privilegedUsers, Set<String> _nonPrivilegedUsers, int _privilegedUserConcurrentSessionLimit, int _nonPrivilegedUserConcurrentSessionLimit){
+ privilegedUsers = _privilegedUsers;
+ nonPrivilegedUsers = _nonPrivilegedUsers;
+ privilegedUserConcurrentSessionLimit = _privilegedUserConcurrentSessionLimit;
+ nonPrivilegedUserConcurrentSessionLimit = _nonPrivilegedUserConcurrentSessionLimit;
+ concurrentSessionCounter = new HashMap<>();
+ }
+
+ public static boolean verifySessionForUser(String username){
+ if(!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+ if(!concurrentSessionCounter.containsKey(username)){
+ concurrentSessionCounter.put(username, 0);
+ }
+
+ if((privilegedUsers.contains(username) && !(concurrentSessionCounter.get(username) < privilegedUserConcurrentSessionLimit)) || (nonPrivilegedUsers.contains(username) && !(concurrentSessionCounter.get(username) < nonPrivilegedUserConcurrentSessionLimit))){
+ return false;
+ }
+
+ incrementConcurrentSessionCount(username);
+ return true;
+ }
+
+ private static void incrementConcurrentSessionCount(String username){
Review Comment:
same comment as above.
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.util;
+
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new HashMap<>();
Review Comment:
The name suggests this is not thread safe, should this be `ConcurrentHashMap` ?
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -1335,4 +1343,49 @@ public int getJettyMaxFormKeys() {
return getInt(JETTY_MAX_FORM_KEYS, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}
+ @Override
+ public int getPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public int getNonPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public Set<String> getPrivilegedUsers(){
+ Set<String> set = new HashSet<>();
+
+ String value = get( GATEWAY_PRIVILEGED_USERS );
+ if ((value != null) && (!value.trim().equals("")) ) {
+ List<String> temp = new ArrayList<>(Arrays.asList(value.split(",")));
+ temp.forEach(e -> { temp.set(temp.indexOf(e), e.trim()); });
+ set.addAll(temp);
Review Comment:
Wait, if you are adding it to set, just add the trimmed value to set, easier and cleaner.
`temp.forEach(e -> { set.add(e.trim()) });`
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.util;
+
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new HashMap<>();
+
+ private ConcurrentSessionVerifier(){}
+
+ public static void init(Set<String> _privilegedUsers, Set<String> _nonPrivilegedUsers, int _privilegedUserConcurrentSessionLimit, int _nonPrivilegedUserConcurrentSessionLimit){
+ privilegedUsers = _privilegedUsers;
+ nonPrivilegedUsers = _nonPrivilegedUsers;
+ privilegedUserConcurrentSessionLimit = _privilegedUserConcurrentSessionLimit;
+ nonPrivilegedUserConcurrentSessionLimit = _nonPrivilegedUserConcurrentSessionLimit;
+ concurrentSessionCounter = new HashMap<>();
+ }
+
+ public static boolean verifySessionForUser(String username){
Review Comment:
i am scared of this method. This is static, so we have to be very careful about the state here especially since we are dealing with usernames. Should this be synchronized? the data structures used in the method are not thread safe which will cause issues.
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.util;
+
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new HashMap<>();
+
+ private ConcurrentSessionVerifier(){}
+
+ public static void init(Set<String> _privilegedUsers, Set<String> _nonPrivilegedUsers, int _privilegedUserConcurrentSessionLimit, int _nonPrivilegedUserConcurrentSessionLimit){
+ privilegedUsers = _privilegedUsers;
+ nonPrivilegedUsers = _nonPrivilegedUsers;
+ privilegedUserConcurrentSessionLimit = _privilegedUserConcurrentSessionLimit;
+ nonPrivilegedUserConcurrentSessionLimit = _nonPrivilegedUserConcurrentSessionLimit;
+ concurrentSessionCounter = new HashMap<>();
Review Comment:
The name suggests this is not thread safe, should this be `ConcurrentHashMap` ?
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -1335,4 +1343,49 @@ public int getJettyMaxFormKeys() {
return getInt(JETTY_MAX_FORM_KEYS, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}
+ @Override
+ public int getPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public int getNonPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public Set<String> getPrivilegedUsers(){
+ Set<String> set = new HashSet<>();
Review Comment:
set is not a good name since it is too close to the built in type Set
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.util;
+
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new HashMap<>();
+
+ private ConcurrentSessionVerifier(){}
+
+ public static void init(Set<String> _privilegedUsers, Set<String> _nonPrivilegedUsers, int _privilegedUserConcurrentSessionLimit, int _nonPrivilegedUserConcurrentSessionLimit){
+ privilegedUsers = _privilegedUsers;
+ nonPrivilegedUsers = _nonPrivilegedUsers;
+ privilegedUserConcurrentSessionLimit = _privilegedUserConcurrentSessionLimit;
+ nonPrivilegedUserConcurrentSessionLimit = _nonPrivilegedUserConcurrentSessionLimit;
+ concurrentSessionCounter = new HashMap<>();
+ }
+
+ public static boolean verifySessionForUser(String username){
+ if(!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+ if(!concurrentSessionCounter.containsKey(username)){
+ concurrentSessionCounter.put(username, 0);
+ }
+
+ if((privilegedUsers.contains(username) && !(concurrentSessionCounter.get(username) < privilegedUserConcurrentSessionLimit)) || (nonPrivilegedUsers.contains(username) && !(concurrentSessionCounter.get(username) < nonPrivilegedUserConcurrentSessionLimit))){
+ return false;
+ }
+
+ incrementConcurrentSessionCount(username);
+ return true;
+ }
+
+ private static void incrementConcurrentSessionCount(String username){
+ int count = concurrentSessionCounter.get(username);
+ count++;
+ concurrentSessionCounter.put(username, count);
+ }
+
+ public static void sessionEndedForUser(String username){
Review Comment:
same comment as above.
--
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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r921495654
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.util;
+
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new HashMap<>();
+
+ private ConcurrentSessionVerifier(){}
+
+ public static void init(Set<String> _privilegedUsers, Set<String> _nonPrivilegedUsers, int _privilegedUserConcurrentSessionLimit, int _nonPrivilegedUserConcurrentSessionLimit){
Review Comment:
Is there any other conventions, you use, when you can't put `this` in front of the variable when giving it the same value as the parameter or should i come up with different names for the parameters?
--
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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r922043218
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -1335,4 +1343,49 @@ public int getJettyMaxFormKeys() {
return getInt(JETTY_MAX_FORM_KEYS, ContextHandler.DEFAULT_MAX_FORM_KEYS);
}
+ @Override
+ public int getPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public int getNonPrivilegedUserConcurrentSessionLimit(){
+ int limit = getInt(GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT, GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT);
+ if(limit < 0)
+ return GATEWAY_NON_PRIVILEGED_USER_CONCURRENT_SESSION_LIMIT_DEFAULT;
+ else
+ return limit;
+ }
+
+ @Override
+ public Set<String> getPrivilegedUsers(){
+ Set<String> set = new HashSet<>();
+
+ String value = get( GATEWAY_PRIVILEGED_USERS );
+ if ((value != null) && (!value.trim().equals("")) ) {
Review Comment:
Fixed 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] zeroflag commented on a diff in pull request #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r924458961
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.util;
+
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new ConcurrentHashMap<>();
+ private static final Object lock = new Object();
+
+ private ConcurrentSessionVerifier(){}
+
+ public static synchronized void init(Set<String> privilegedUsers, Set<String> nonPrivilegedUsers, int privilegedUserConcurrentSessionLimit, int nonPrivilegedUserConcurrentSessionLimit){
+ ConcurrentSessionVerifier.privilegedUsers = privilegedUsers;
+ ConcurrentSessionVerifier.nonPrivilegedUsers = nonPrivilegedUsers;
+ ConcurrentSessionVerifier.privilegedUserConcurrentSessionLimit = privilegedUserConcurrentSessionLimit;
+ ConcurrentSessionVerifier.nonPrivilegedUserConcurrentSessionLimit = nonPrivilegedUserConcurrentSessionLimit;
+ concurrentSessionCounter = new ConcurrentHashMap<>();
+ }
+
+ public static boolean verifySessionForUser(String username){
+ if(!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+ synchronized(lock) {
+ if (!concurrentSessionCounter.containsKey(username)) {
Review Comment:
This is probably can be simplified by using `computeIfAbsent` / `putIfAbsent` / `compute` methods.
For example:
```java
int count = concurrentSessionCounter.computeIfAbsent(username, name -> 0);
if (...) {
}
```
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.util;
+
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+public class ConcurrentSessionVerifier {
Review Comment:
Instead of using static methods and static fields, let's have a normal non static class with instance variables and non static methods. Constants like `privilegedUserConcurrentSessionLimit`/`privilegedUserConcurrentSessionLimit` should be kept as statics but `concurrentSessionCounter` should be non static.
Then we can create one instance of this class and use that at each places where needed. Either by creating an instance somewhere in GatewayServer's `main()` and passing that around or by making a singleton. I prefer the former but that's up to how the code is structured. We can decide that later when we start using it.
##########
gateway-util-common/src/main/java/org/apache/knox/gateway/util/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.util;
+
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+public class ConcurrentSessionVerifier {
+ private static Set<String> privilegedUsers = new HashSet<>();
+ private static Set<String> nonPrivilegedUsers = new HashSet<>();;
+ private static int privilegedUserConcurrentSessionLimit = 3;
+ private static int nonPrivilegedUserConcurrentSessionLimit = 2;
+ private static Map<String, Integer> concurrentSessionCounter = new ConcurrentHashMap<>();
+ private static final Object lock = new Object();
+
+ private ConcurrentSessionVerifier(){}
+
+ public static synchronized void init(Set<String> privilegedUsers, Set<String> nonPrivilegedUsers, int privilegedUserConcurrentSessionLimit, int nonPrivilegedUserConcurrentSessionLimit){
+ ConcurrentSessionVerifier.privilegedUsers = privilegedUsers;
+ ConcurrentSessionVerifier.nonPrivilegedUsers = nonPrivilegedUsers;
+ ConcurrentSessionVerifier.privilegedUserConcurrentSessionLimit = privilegedUserConcurrentSessionLimit;
+ ConcurrentSessionVerifier.nonPrivilegedUserConcurrentSessionLimit = nonPrivilegedUserConcurrentSessionLimit;
+ concurrentSessionCounter = new ConcurrentHashMap<>();
+ }
+
+ public static boolean verifySessionForUser(String username){
+ if(!privilegedUsers.contains(username) && !nonPrivilegedUsers.contains(username)) {
+ return true;
+ }
+ synchronized(lock) {
+ if (!concurrentSessionCounter.containsKey(username)) {
+ concurrentSessionCounter.put(username, 0);
+ }
+ if ((privilegedUsers.contains(username) && !(concurrentSessionCounter.get(username) < privilegedUserConcurrentSessionLimit)) || (nonPrivilegedUsers.contains(username) && !(concurrentSessionCounter.get(username) < nonPrivilegedUserConcurrentSessionLimit))) {
+ return false;
+ }
+ incrementConcurrentSessionCount(username);
+ }
+ return true;
+ }
+
+ private static synchronized void incrementConcurrentSessionCount(String username){
+ int count = concurrentSessionCounter.get(username);
Review Comment:
Similarly, see if you can use `compute` to simplify this. For example:
```java
concurrentSessionCounter.compute(username, (key ,value) -> value + 1);
```
--
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 #608: KNOX-2777 - Add configurations for concurrent session verifier feature
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #608:
URL: https://github.com/apache/knox/pull/608#discussion_r925500083
##########
gateway-spi-common/src/main/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifier.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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 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 {
+ public static final ConcurrentSessionVerifier INSTANCE = new ConcurrentSessionVerifier();
+ private Set<String> privilegedUsers;
+ private Set<String> nonPrivilegedUsers;
+ private int privilegedUserConcurrentSessionLimit;
+ private int nonPrivilegedUserConcurrentSessionLimit;
+ private Map<String, Integer> concurrentSessionCounter;
+ private Lock sessionCountModifyLock = new ReentrantLock();
Review Comment:
Ok. Made it final.
--
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