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 2021/08/24 09:26:55 UTC
[GitHub] [knox] zeroflag opened a new pull request #484: KNOX-2646. The tokenLimitPerUser check doesn't always work.
zeroflag opened a new pull request #484:
URL: https://github.com/apache/knox/pull/484
## What changes were proposed in this pull request?
We have an equality check when checking the max number of tokens per user. But if the user already have N number of tokens, and later the admin changes the gateway.knox.token.limit.per.user to a smaller number then this check will never trigger.
## How was this patch tested?
1. Having no limit and generating N tokens
2. Changing gateway.knox.token.limit.per.user to a lower number than N and restarting the service
3. Trying to generate one more token and getting a limit exception
--
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 pull request #484: KNOX-2646. The tokenLimitPerUser check doesn't always work.
Posted by GitBox <gi...@apache.org>.
zeroflag commented on pull request #484:
URL: https://github.com/apache/knox/pull/484#issuecomment-904619229
cc: @pzampino @moresandeep @lmccay
--
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 change in pull request #484: KNOX-2646. The tokenLimitPerUser check doesn't always work.
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #484:
URL: https://github.com/apache/knox/pull/484#discussion_r695548940
##########
File path: gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java
##########
@@ -741,4 +741,7 @@ private String safeGetMessage(Throwable t) {
return message != null ? message : "null";
}
+ void setTokenLimitPerUser(int tokenLimitPerUser) { // visible for testing
Review comment:
I managed to change the limit without the accessor method or reflection.
--
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 change in pull request #484: KNOX-2646. The tokenLimitPerUser check doesn't always work.
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #484:
URL: https://github.com/apache/knox/pull/484#discussion_r694995989
##########
File path: gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java
##########
@@ -741,4 +741,7 @@ private String safeGetMessage(Throwable t) {
return message != null ? message : "null";
}
+ void setTokenLimitPerUser(int tokenLimitPerUser) { // visible for testing
Review comment:
I'm not a fan of any of those (adding the accessor / using reflection) but thought this way is a little bit better. With reflection I need to reference the field name as a String which is not refactoring friendly.
I'll see if I can reconfigure the mocks and call the init() on the TokenResource again to change the limit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino merged pull request #484: KNOX-2646. The tokenLimitPerUser check doesn't always work.
Posted by GitBox <gi...@apache.org>.
pzampino merged pull request #484:
URL: https://github.com/apache/knox/pull/484
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #484: KNOX-2646. The tokenLimitPerUser check doesn't always work.
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #484:
URL: https://github.com/apache/knox/pull/484#discussion_r694971191
##########
File path: gateway-service-knoxtoken/src/test/java/org/apache/knox/gateway/service/knoxtoken/TokenServiceResourceTest.java
##########
@@ -986,6 +986,30 @@ public void testUnlimitedTokensPerUser() throws Exception {
testLimitingTokensPerUser(String.valueOf("-1"), 100);
}
+ @Test
+ public void testTokenLimitChangeAfterAlreadyHavingTokens() throws Exception {
+ Map<String, String> contextExpectations = new HashMap<>();
+ contextExpectations.put(KNOX_TOKEN_USER_LIMIT, "-1");
+ configureCommonExpectations(contextExpectations, Boolean.TRUE);
+ TokenResource tr = new TokenResource();
+ tr.request = request;
+ tr.context = context;
+ tr.init();
+ // already have N tokens
+ int numberOfPreExistingTokens = 5;
+ for (int i = 0; i < numberOfPreExistingTokens; i++) {
+ tr.doGet();
+ }
+ Response getKnoxTokensResponse = tr.getUserTokens(USER_NAME);
+ Collection<String> tokens = ((Map<String, Collection<String>>) JsonUtils.getObjectFromJsonString(getKnoxTokensResponse.getEntity().toString()))
+ .get("tokens");
+ assertEquals(tokens.size(), numberOfPreExistingTokens);
+ // change the limit and try generate one more
+ tr.setTokenLimitPerUser(numberOfPreExistingTokens -1);
+ Response response = tr.doGet();
+ assertTrue(response.getEntity().toString().contains("Unable to get token - token limit exceeded."));
+ }
+
@Test
public void tesTokenLimitPerUserExceeded() throws Exception {
Review comment:
Could you also correct this test name to tes**t**TokenLimitPerUserExceeded ?
##########
File path: gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java
##########
@@ -741,4 +741,7 @@ private String safeGetMessage(Throwable t) {
return message != null ? message : "null";
}
+ void setTokenLimitPerUser(int tokenLimitPerUser) { // visible for testing
Review comment:
You could also use reflection in the test to set this, rather than adding a method ONLY for testing.
--
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 change in pull request #484: KNOX-2646. The tokenLimitPerUser check doesn't always work.
Posted by GitBox <gi...@apache.org>.
zeroflag commented on a change in pull request #484:
URL: https://github.com/apache/knox/pull/484#discussion_r694995517
##########
File path: gateway-service-knoxtoken/src/test/java/org/apache/knox/gateway/service/knoxtoken/TokenServiceResourceTest.java
##########
@@ -986,6 +986,30 @@ public void testUnlimitedTokensPerUser() throws Exception {
testLimitingTokensPerUser(String.valueOf("-1"), 100);
}
+ @Test
+ public void testTokenLimitChangeAfterAlreadyHavingTokens() throws Exception {
+ Map<String, String> contextExpectations = new HashMap<>();
+ contextExpectations.put(KNOX_TOKEN_USER_LIMIT, "-1");
+ configureCommonExpectations(contextExpectations, Boolean.TRUE);
+ TokenResource tr = new TokenResource();
+ tr.request = request;
+ tr.context = context;
+ tr.init();
+ // already have N tokens
+ int numberOfPreExistingTokens = 5;
+ for (int i = 0; i < numberOfPreExistingTokens; i++) {
+ tr.doGet();
+ }
+ Response getKnoxTokensResponse = tr.getUserTokens(USER_NAME);
+ Collection<String> tokens = ((Map<String, Collection<String>>) JsonUtils.getObjectFromJsonString(getKnoxTokensResponse.getEntity().toString()))
+ .get("tokens");
+ assertEquals(tokens.size(), numberOfPreExistingTokens);
+ // change the limit and try generate one more
+ tr.setTokenLimitPerUser(numberOfPreExistingTokens -1);
+ Response response = tr.doGet();
+ assertTrue(response.getEntity().toString().contains("Unable to get token - token limit exceeded."));
+ }
+
@Test
public void tesTokenLimitPerUserExceeded() throws Exception {
Review comment:
Sure.
--
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