[
https://issues.apache.org/jira/browse/KNOX-2762?focusedWorklogId=781801&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-781801
]
ASF GitHub Bot logged work on KNOX-2762:
----------------------------------------
Author: ASF GitHub Bot
Created on: 15/Jun/22 19:28
Start Date: 15/Jun/22 19:28
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 781801)
Time Spent: 0.5h (was: 20m)
> Whitespaces around delimiters in composite provider names gives
> NullPointerException
> ------------------------------------------------------------------------------------
>
> Key: KNOX-2762
> URL: https://issues.apache.org/jira/browse/KNOX-2762
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
> Affects Versions: 1.6.0
> Reporter: Harshil Jhaveri
> Assignee: Harshil Jhaveri
> Priority: Minor
> Fix For: 2.0.0
>
> Attachments: NPE gateway log.png
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> When giving space before the delimiter for composite.provider.names in
> composite authorisation provider. The topology deployment is failing and
> returning a null pointer exception.
> The topology state:
> {code:java}
> <provider>
> <role>authorization</role>
> <name>CompositeAuthz</name>
> <enabled>true</enabled>
> <param>
> <name>composite.provider.names</name>
> <value>AclsAuthz ,XASecurePDPKnox</value>
> </param>
> <param>
> <name>AclsAuthz.ranger.acl</name>
> <value>*;;</value> </param></provider>
> </provider>{code}
> {color:#e8bf6a}CC:{color} [~pzampino] [~smore] [~smolnar]
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
