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/23 09:00:20 UTC
[GitHub] [knox] MrtnBalazs opened a new pull request, #622: KNOX-2789 - Changed configuration names, removed the option to config…
MrtnBalazs opened a new pull request, #622:
URL: https://github.com/apache/knox/pull/622
## What changes were proposed in this pull request?
Introduced new group called `gateway.session.verification.unlimited.users` and changed the behaviour of the groups and the tests that check these behaviours. `gateway.non.privileged.users` group is now not configurable.
Also changed configuration names:
```
gateway.privileged.users -> gateway.session.verification.privileged.users
gateway.non.privileged.users.concurrent.session.limit -> gateway.session.verification.non.privileged.user.limit
gateway.privileged.users.concurrent.session.limit -> gateway.session.verification.privileged.user.limit
```
Previous behaviour:
If someone is configured privileged -> privileged limit applies
If someone is configured non-privileged -> non-privileged limit applies
If someone is not configured in either of the groups -> no limit applies
New behaviour:
If someone is configured privileged -> privileged limit applies
If someone is configured unlimited -> no limit applies
If someone is not configured in either of the groups -> non-privileged limit applies
## How was this patch tested?
I changed the existing unit tests to test the new behaviours.
I also tested the new behaviours manually with the following configurataions:
1. Nothing
2. Empty string for implementation configuration
```
<property>
<name>gateway.service.concurrentsessionverifier.impl</name>
<value></value>
</property>
```
3. Blank string for implementation
```
<property>
<name>gateway.service.concurrentsessionverifier.impl</name>
<value> </value>
</property>
```
4. Not valid implementation
```
<property>
<name>gateway.service.concurrentsessionverifier.impl</name>
<value>org.apache.knox.gateway.session.control.NotExistingConcurrentSessionVerifier</value>
</property>
```
5. Empty implementation configured
```
<property>
<name>gateway.service.concurrentsessionverifier.impl</name>
<value>org.apache.knox.gateway.session.control.EmptyConcurrentSessionVerifier</value>
</property>
```
6. InMemory implementation configured
```
<property>
<name>gateway.service.concurrentsessionverifier.impl</name>
<value>org.apache.knox.gateway.session.control.InMemoryConcurrentSessionVerifier</value>
</property>
```
7. Fully configured
```
<property>
<name>gateway.session.verification.unlimited.users</name>
<value>admin</value>
</property>
<property>
<name>gateway.session.verification.privileged.users</name>
<value>tom</value>
</property>
<property>
<name>gateway.session.verification.privileged.user.limit</name>
<value>2</value>
</property>
<property>
<name>gateway.session.verification.non.privileged.user.limit</name>
<value>1</value>
</property>
<property>
<name>gateway.session.verification.expired.tokens.cleaning.period</name>
<value>80</value>
</property>
<property>
<name>gateway.service.concurrentsessionverifier.impl</name>
<value>org.apache.knox.gateway.session.control.InMemoryConcurrentSessionVerifier</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] smolnar82 commented on a diff in pull request #622: KNOX-2789 - Changed configuration names, removed the option to config…
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #622:
URL: https://github.com/apache/knox/pull/622#discussion_r952457950
##########
gateway-server/src/test/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifierTest.java:
##########
@@ -316,25 +296,27 @@ public void testBackgroundThreadRemoveExpiredTokens() throws ServiceLifecycleExc
verifier.verifySessionForUser("admin", adminToken2);
JWT expiringAdminToken = tokenAuthority.issueToken(expiringJwtAttributesForAdmin);
verifier.verifySessionForUser("admin", expiringAdminToken);
- Assert.assertEquals(3, verifier.countValidTokensForUser("admin"));
- Thread.sleep(1100);
- Assert.assertEquals(2, verifier.countValidTokensForUser("admin"));
+ Assert.assertEquals(3, verifier.getTokenCountForUser("admin").intValue());
+ Thread.sleep(1050);
+ verifier.removeExpiredTokens();
+ Assert.assertEquals(2, verifier.getTokenCountForUser("admin").intValue());
JWTokenAttributes expiringJwtAttributesForTom = makeJwtAttribute("tom", true);
verifier.verifySessionForUser("tom", tomToken1);
verifier.verifySessionForUser("tom", tomToken2);
JWT expiringTomToken = tokenAuthority.issueToken(expiringJwtAttributesForTom);
verifier.verifySessionForUser("tom", expiringTomToken);
- Assert.assertEquals(3, verifier.countValidTokensForUser("tom"));
- Thread.sleep(1100);
- Assert.assertEquals(2, verifier.countValidTokensForUser("tom"));
+ Assert.assertEquals(3, verifier.getTokenCountForUser("tom").intValue());
+ Thread.sleep(1050);
Review Comment:
The same qq. here.
##########
gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java:
##########
@@ -822,7 +822,7 @@ public interface GatewayConfig {
Set<String> getPrivilegedUsers();
- Set<String> getNonPrivilegedUsers();
+ Set<String> getUnlimitedUsers();
Review Comment:
We may rename this to `getSessionVerificationUnlimitiedUsers` and the previous one to `getSessionVerificationPrivilegedUsers`. What do you think?
##########
gateway-server/src/test/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifierTest.java:
##########
@@ -273,41 +255,39 @@ public void testNegativeLimitMeansUnlimited() throws ServiceLifecycleException {
@Test
public void testExpiredTokensAreNotCounted() throws ServiceLifecycleException, TokenServiceException, InterruptedException {
- GatewayConfig config = mockConfig(new HashSet<>(Arrays.asList("admin")), new HashSet<>(Arrays.asList("tom", "guest")), 3, 3);
+ GatewayConfig config = mockConfig(Collections.emptySet(), new HashSet<>(Arrays.asList("admin")), 3, 3);
verifier.init(config, options);
JWTokenAttributes expiringJwtAttributesForTom = makeJwtAttribute("tom", true);
- JWT tomToken = tokenAuthority.issueToken(jwtAttributesForTom);
- verifier.verifySessionForUser("tom", tomToken);
+ verifier.verifySessionForUser("tom", tomToken1);
Assert.assertEquals(1, verifier.countValidTokensForUser("tom"));
- tomToken = tokenAuthority.issueToken(expiringJwtAttributesForTom);
- verifier.verifySessionForUser("tom", tomToken);
+ JWT expiringTomToken = tokenAuthority.issueToken(expiringJwtAttributesForTom);
+ verifier.verifySessionForUser("tom", expiringTomToken);
Assert.assertEquals(2, verifier.countValidTokensForUser("tom"));
- tomToken = tokenAuthority.issueToken(expiringJwtAttributesForTom);
- verifier.verifySessionForUser("tom", tomToken);
+ expiringTomToken = tokenAuthority.issueToken(expiringJwtAttributesForTom);
+ verifier.verifySessionForUser("tom", expiringTomToken);
Assert.assertEquals(3, verifier.countValidTokensForUser("tom"));
- Thread.sleep(1000L);
+ Thread.sleep(1100);
Review Comment:
As discussed offline, we may give a high enough cleaning period, and invoke the cleaning job manually here to avoid Thread.sleep. Have considered doing that?
##########
gateway-server/src/test/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifierTest.java:
##########
@@ -316,25 +296,27 @@ public void testBackgroundThreadRemoveExpiredTokens() throws ServiceLifecycleExc
verifier.verifySessionForUser("admin", adminToken2);
JWT expiringAdminToken = tokenAuthority.issueToken(expiringJwtAttributesForAdmin);
verifier.verifySessionForUser("admin", expiringAdminToken);
- Assert.assertEquals(3, verifier.countValidTokensForUser("admin"));
- Thread.sleep(1100);
- Assert.assertEquals(2, verifier.countValidTokensForUser("admin"));
+ Assert.assertEquals(3, verifier.getTokenCountForUser("admin").intValue());
+ Thread.sleep(1050);
Review Comment:
If you are invoking `verifier.removeExpiredTokens();` anyway, why do we need the Thread.sleep 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 #622: KNOX-2789 - Changed configuration names, removed the option to config…
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #622:
URL: https://github.com/apache/knox/pull/622#discussion_r952513040
##########
gateway-server/src/test/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifierTest.java:
##########
@@ -316,25 +296,27 @@ public void testBackgroundThreadRemoveExpiredTokens() throws ServiceLifecycleExc
verifier.verifySessionForUser("admin", adminToken2);
JWT expiringAdminToken = tokenAuthority.issueToken(expiringJwtAttributesForAdmin);
verifier.verifySessionForUser("admin", expiringAdminToken);
- Assert.assertEquals(3, verifier.countValidTokensForUser("admin"));
- Thread.sleep(1100);
- Assert.assertEquals(2, verifier.countValidTokensForUser("admin"));
+ Assert.assertEquals(3, verifier.getTokenCountForUser("admin").intValue());
+ Thread.sleep(1050);
Review Comment:
It is for waiting until the token expires, but i realised i could just issue already expired tokens and skip the waiting. That would probably check the same functionality. I will change it that way and remove the sleep (at the other places as well)
--
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 #622: KNOX-2789 - Changed configuration names, removed the option to config…
Posted by GitBox <gi...@apache.org>.
MrtnBalazs commented on code in PR #622:
URL: https://github.com/apache/knox/pull/622#discussion_r952514087
##########
gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java:
##########
@@ -822,7 +822,7 @@ public interface GatewayConfig {
Set<String> getPrivilegedUsers();
- Set<String> getNonPrivilegedUsers();
+ Set<String> getUnlimitedUsers();
Review Comment:
Okay, it gives a bit more information.
--
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 #622: KNOX-2789 - Changed configuration names, removed the option to config…
Posted by GitBox <gi...@apache.org>.
smolnar82 merged PR #622:
URL: https://github.com/apache/knox/pull/622
--
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