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/06/14 23:49:02 UTC
[GitHub] [knox] harshiljhaveri opened a new pull request, #594: KNOX-2762
harshiljhaveri opened a new pull request, #594:
URL: https://github.com/apache/knox/pull/594
## What changes were proposed in this pull request?
Whitespaces around delimiters were causing problems while specifying composite auth provider names. The parseProviderNames function has been modified to trim provider names before passing it to the Deployment Contributor function.
## How was this patch tested?
Ran automated unit/integration tests to ensure provider names did not contain whitespaces. Also ran manual tests to ensure no malfunctions.
[Knox Contributing Process](https://cwiki.apache.org/confluence/display/KNOX/Contribution+Process#ContributionProcess-GithubWorkflow) reviewed.
--
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 merged pull request #594: KNOX-2762
Posted by GitBox <gi...@apache.org>.
moresandeep merged PR #594:
URL: https://github.com/apache/knox/pull/594
--
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 #594: KNOX-2762
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #594:
URL: https://github.com/apache/knox/pull/594#discussion_r898334503
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -56,17 +56,23 @@ public void contributeFilter( DeploymentContext context, Provider provider, Serv
Map<String, String> providerParams = provider.getParams();
String providerNames = providerParams.get("composite.provider.names");
+ if (!providerNames.isEmpty()) {
Review Comment:
I'd use StringUtils over String here because the first one is `null` safe. Even better, I think `parseProviderNames` should return a collection and not a String array.
##########
gateway-provider-security-authz-composite/src/test/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzProviderTest.java:
##########
@@ -64,4 +64,16 @@ public void testParsingProviderNames() throws Exception {
assertEquals(providerNames[1], "SomeOther");
assertEquals(providerNames[2], "TheOtherOne");
}
+
+ @Test
+ public void testingParsingProviderNames() throws Exception {
+ String testnames = " AclsAuthz , SomeOther , TheOtherOne ,";
Review Comment:
I'd modify this sample string to cover all cases:
- whitespace just before the name
- whitespace just after the name
- whitespace before and after the name
- more whitespaces
- no whitespace at all
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -56,17 +56,23 @@ public void contributeFilter( DeploymentContext context, Provider provider, Serv
Map<String, String> providerParams = provider.getParams();
String providerNames = providerParams.get("composite.provider.names");
+ if (!providerNames.isEmpty()) {
String[] names = parseProviderNames(providerNames);
for (String name : names) {
getProviderSpecificParams(resource, params, providerParams, name);
DeploymentFactory.getProviderContributor("authorization", name)
- .contributeFilter(context, provider, service, resource, params);
+ .contributeFilter(context, provider, service, resource, params);
params.clear();
}
+ }
}
String[] parseProviderNames(String providerNames) {
- return providerNames.split(",\\s*");
+ String[] b = providerNames.split("\\s*,\\s*");
Review Comment:
Please avoid using parameter names like `b`. It's just against clean code principles.
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -56,17 +56,23 @@ public void contributeFilter( DeploymentContext context, Provider provider, Serv
Map<String, String> providerParams = provider.getParams();
String providerNames = providerParams.get("composite.provider.names");
+ if (!providerNames.isEmpty()) {
String[] names = parseProviderNames(providerNames);
for (String name : names) {
getProviderSpecificParams(resource, params, providerParams, name);
DeploymentFactory.getProviderContributor("authorization", name)
- .contributeFilter(context, provider, service, resource, params);
+ .contributeFilter(context, provider, service, resource, params);
params.clear();
}
+ }
Review Comment:
This additional `if` statement modifies the indentation here so that content within that condition should be intended more.
--
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 #594: KNOX-2762
Posted by GitBox <gi...@apache.org>.
moresandeep commented on PR #594:
URL: https://github.com/apache/knox/pull/594#issuecomment-1156857449
@harshiljhaveri can you post a followup PR with suggested changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on pull request #594: KNOX-2762
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on PR #594:
URL: https://github.com/apache/knox/pull/594#issuecomment-1156845298
@harshiljhaveri @moresandeep - I'd like to ask you to re-open this one and address my review comments. Sorry, I did not add them before, I just haven't had time today morning.
Thanks for your understanding!
--
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 #594: KNOX-2762
Posted by GitBox <gi...@apache.org>.
moresandeep commented on PR #594:
URL: https://github.com/apache/knox/pull/594#issuecomment-1156856334
Sure, thanks for the comments @smolnar82!
--
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