nandorsoma commented on code in PR #6034: URL: https://github.com/apache/nifi/pull/6034#discussion_r875817192
########## nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/utils/UsmReader.java: ########## @@ -0,0 +1,119 @@ +/* + * 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.nifi.snmp.utils; + +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.deser.std.StdDeserializer; +import com.fasterxml.jackson.databind.module.SimpleModule; +import org.apache.nifi.processor.exception.ProcessException; +import org.snmp4j.security.UsmUser; +import org.snmp4j.smi.OctetString; + +import java.io.File; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.util.Arrays; +import java.util.List; +import java.util.Scanner; +import java.util.stream.Collectors; + +public interface UsmReader { Review Comment: This is an interesting solution, I've learned something new today. My only problem is that the interface can be extended and in such case it would be a bit confusing for me. To avoid that a final utility class would be better. But I don't feel strongly, probably it is because this is the first time I encounter this approach. ########## nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/utils/UsmReader.java: ########## @@ -0,0 +1,119 @@ +/* + * 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.nifi.snmp.utils; + +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.deser.std.StdDeserializer; +import com.fasterxml.jackson.databind.module.SimpleModule; +import org.apache.nifi.processor.exception.ProcessException; +import org.snmp4j.security.UsmUser; +import org.snmp4j.smi.OctetString; + +import java.io.File; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.util.Arrays; +import java.util.List; +import java.util.Scanner; +import java.util.stream.Collectors; + +public interface UsmReader { + + List<UsmUser> readUsm(String usmUsers); + + static UsmReader jsonFileUsmReader() { + return usmUsersFilePath -> { + final List<UsmUser> userDetails; + try (Scanner scanner = new Scanner(new File(usmUsersFilePath))) { + final String content = scanner.useDelimiter("\\Z").next(); + ObjectMapper mapper = new ObjectMapper(); + SimpleModule module = new SimpleModule(); + module.addDeserializer(UsmUser.class, new UsmUserDeserializer()); + mapper.registerModule(module); + userDetails = mapper.readValue(content, new TypeReference<List<UsmUser>>() { + }); + } catch (FileNotFoundException e) { + throw new ProcessException("USM user file not found, please check the file path and file permissions.", e); + } catch (JsonProcessingException e) { + throw new ProcessException("Could not parse USM user file, please check the processor details for examples.", e); + } + return userDetails; + }; + } + + static UsmReader jsonUsmReader() { + return usmUsersJson -> { + try { + ObjectMapper mapper = new ObjectMapper(); + SimpleModule module = new SimpleModule(); + module.addDeserializer(UsmUser.class, new UsmUserDeserializer()); + mapper.registerModule(module); + return mapper.readValue(usmUsersJson, new TypeReference<List<UsmUser>>() { + }); + } catch (JsonProcessingException e) { + throw new ProcessException("Could not parse USM user file, please check the processor details for examples.", e); + } + }; + } + + static UsmReader securityNamesUsmReader() { + return usmSecurityNames -> Arrays.stream(usmSecurityNames.trim().split(",")) + .map(securityName -> new UsmUser( + new OctetString(securityName), null, null, null, null) + ) + .collect(Collectors.toList()); + } + + class UsmUserDeserializer extends StdDeserializer<UsmUser> { + + public UsmUserDeserializer() { + this(null); + } + + public UsmUserDeserializer(Class<?> vc) { Review Comment: Do we need this constructor? Seems like it is not called from external code. ########## nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/processors/ListenTrapSNMP.java: ########## @@ -100,21 +129,66 @@ public class ListenTrapSNMP extends AbstractSessionFactoryProcessor { private volatile SNMPTrapReceiverHandler snmpTrapReceiverHandler; + @Override + public Collection<ValidationResult> customValidate(final ValidationContext validationContext) { + final List<ValidationResult> results = new ArrayList<>(super.customValidate(validationContext)); + + final boolean snmpUsersJsonFileIsSet = validationContext.getProperty(SNMP_USM_USERS_JSON_FILE).isSet(); + final boolean snmpUsersJsonIsSet = validationContext.getProperty(SNMP_USM_USERS_JSON).isSet(); + final boolean snmpUsersSecurityNames = validationContext.getProperty(SNMP_USM_SECURITY_NAMES).isSet(); + + final boolean isOnlyOneIsSetFromThree = Stream.of(snmpUsersJsonFileIsSet, snmpUsersJsonIsSet, snmpUsersSecurityNames).filter(a -> a).count() == 1; Review Comment: It is a bit hard to follow these variable names. ``` isOnlyOneIsSetFromThree could be isOnlyOneSourceSet isBothSetOrNotSetFromTwo could be isOnlyOneJsonSourceSet ``` What do you think? ########## nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/processors/ListenTrapSNMP.java: ########## @@ -65,14 +74,32 @@ public class ListenTrapSNMP extends AbstractSessionFactoryProcessor { .addValidator(StandardValidators.PORT_VALIDATOR) .build(); - public static final PropertyDescriptor SNMP_USM_USERS_FILE_PATH = new PropertyDescriptor.Builder() + public static final PropertyDescriptor SNMP_USM_USERS_JSON_FILE = new PropertyDescriptor.Builder() .name("snmp-usm-users-file-path") - .displayName("SNMP Users File Path") + .displayName("SNMP Users JSON File") .description("The path of the json file containing the user credentials for SNMPv3. Check Usage for more details.") - .required(true) - .defaultValue("") + .required(false) Review Comment: I'm wondering what happens if there is a flow with the older version, empty string is set for this property and user upgrades to the proposed version. If I'm not mistaken it could lead to errors in the validation because `validationContext.getProperty(SNMP_USM_USERS_JSON_FILE).isSet();` will return true, but I think this is not that what we want. ########## nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/pom.xml: ########## @@ -55,6 +55,12 @@ language governing permissions and limitations under the License. --> <groupId>com.fasterxml.jackson.core</groupId> <artifactId>jackson-databind</artifactId> </dependency> + <dependency> Review Comment: With the addition of this dependency I think you have to update the NOTICE file with jackson-databind, but I'm not an expert in licences. What do you think? ########## nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/processors/ListenTrapSNMP.java: ########## @@ -100,21 +129,66 @@ public class ListenTrapSNMP extends AbstractSessionFactoryProcessor { private volatile SNMPTrapReceiverHandler snmpTrapReceiverHandler; + @Override + public Collection<ValidationResult> customValidate(final ValidationContext validationContext) { + final List<ValidationResult> results = new ArrayList<>(super.customValidate(validationContext)); + + final boolean snmpUsersJsonFileIsSet = validationContext.getProperty(SNMP_USM_USERS_JSON_FILE).isSet(); + final boolean snmpUsersJsonIsSet = validationContext.getProperty(SNMP_USM_USERS_JSON).isSet(); + final boolean snmpUsersSecurityNames = validationContext.getProperty(SNMP_USM_SECURITY_NAMES).isSet(); + + final boolean isOnlyOneIsSetFromThree = Stream.of(snmpUsersJsonFileIsSet, snmpUsersJsonIsSet, snmpUsersSecurityNames).filter(a -> a).count() == 1; + final boolean isBothSetOrNotSetFromTwo = snmpUsersJsonFileIsSet && snmpUsersJsonIsSet || !snmpUsersJsonFileIsSet && !snmpUsersJsonIsSet; + final boolean isSnmpv3 = validationContext.getProperty(BasicProperties.SNMP_VERSION).getValue().equals(BasicProperties.SNMP_V3.getValue()); + final boolean isNoAuthNoPriv = validationContext.getProperty(V3SecurityProperties.SNMP_SECURITY_LEVEL).getValue().equals(V3SecurityProperties.NO_AUTH_NO_PRIV.getValue()); + + if (isSnmpv3 && isNoAuthNoPriv && isOnlyOneIsSetFromThree) { + results.add(new ValidationResult.Builder() + .subject("Provide snmpv3 USM Users JSON.") + .valid(false) + .explanation("Only one of '" + SNMP_USM_USERS_JSON_FILE.getDisplayName() + "' or '" + Review Comment: It is just a hunch but couldn't we display the 3 source field based on a 4th one where we can select the source? This way we could make the validation cleaner and there wouldn't be empty alternative properties (json file, json content, users list) on the ui which can lead to confusion. (I generally like to display as less property on the ui as it is possible.) ########## nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/processors/ListenTrapSNMP.java: ########## @@ -100,21 +129,66 @@ public class ListenTrapSNMP extends AbstractSessionFactoryProcessor { private volatile SNMPTrapReceiverHandler snmpTrapReceiverHandler; + @Override + public Collection<ValidationResult> customValidate(final ValidationContext validationContext) { + final List<ValidationResult> results = new ArrayList<>(super.customValidate(validationContext)); + + final boolean snmpUsersJsonFileIsSet = validationContext.getProperty(SNMP_USM_USERS_JSON_FILE).isSet(); + final boolean snmpUsersJsonIsSet = validationContext.getProperty(SNMP_USM_USERS_JSON).isSet(); + final boolean snmpUsersSecurityNames = validationContext.getProperty(SNMP_USM_SECURITY_NAMES).isSet(); + + final boolean isOnlyOneIsSetFromThree = Stream.of(snmpUsersJsonFileIsSet, snmpUsersJsonIsSet, snmpUsersSecurityNames).filter(a -> a).count() == 1; + final boolean isBothSetOrNotSetFromTwo = snmpUsersJsonFileIsSet && snmpUsersJsonIsSet || !snmpUsersJsonFileIsSet && !snmpUsersJsonIsSet; + final boolean isSnmpv3 = validationContext.getProperty(BasicProperties.SNMP_VERSION).getValue().equals(BasicProperties.SNMP_V3.getValue()); + final boolean isNoAuthNoPriv = validationContext.getProperty(V3SecurityProperties.SNMP_SECURITY_LEVEL).getValue().equals(V3SecurityProperties.NO_AUTH_NO_PRIV.getValue()); + + if (isSnmpv3 && isNoAuthNoPriv && isOnlyOneIsSetFromThree) { + results.add(new ValidationResult.Builder() + .subject("Provide snmpv3 USM Users JSON.") Review Comment: I'm wondering, why do we need to provide Users JSON when it is NO_AUTH_NO_PRIV therefore there is no authentication? What am I missing? ########## nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/processors/ListenTrapSNMP.java: ########## @@ -65,14 +74,32 @@ public class ListenTrapSNMP extends AbstractSessionFactoryProcessor { .addValidator(StandardValidators.PORT_VALIDATOR) .build(); - public static final PropertyDescriptor SNMP_USM_USERS_FILE_PATH = new PropertyDescriptor.Builder() Review Comment: Minor, I know for backward compatibility reasons you cannot rename the name of the PropertyDescriptor, but because of that I'd avoid confusion and I would leave the name of the variable in the original version. What do you think? ########## nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/utils/UsmReader.java: ########## @@ -0,0 +1,119 @@ +/* + * 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.nifi.snmp.utils; + +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.deser.std.StdDeserializer; +import com.fasterxml.jackson.databind.module.SimpleModule; +import org.apache.nifi.processor.exception.ProcessException; +import org.snmp4j.security.UsmUser; +import org.snmp4j.smi.OctetString; + +import java.io.File; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.util.Arrays; +import java.util.List; +import java.util.Scanner; +import java.util.stream.Collectors; + +public interface UsmReader { + + List<UsmUser> readUsm(String usmUsers); + + static UsmReader jsonFileUsmReader() { + return usmUsersFilePath -> { + final List<UsmUser> userDetails; + try (Scanner scanner = new Scanner(new File(usmUsersFilePath))) { + final String content = scanner.useDelimiter("\\Z").next(); + ObjectMapper mapper = new ObjectMapper(); + SimpleModule module = new SimpleModule(); + module.addDeserializer(UsmUser.class, new UsmUserDeserializer()); + mapper.registerModule(module); + userDetails = mapper.readValue(content, new TypeReference<List<UsmUser>>() { + }); + } catch (FileNotFoundException e) { + throw new ProcessException("USM user file not found, please check the file path and file permissions.", e); + } catch (JsonProcessingException e) { + throw new ProcessException("Could not parse USM user file, please check the processor details for examples.", e); + } + return userDetails; + }; + } + + static UsmReader jsonUsmReader() { + return usmUsersJson -> { + try { + ObjectMapper mapper = new ObjectMapper(); + SimpleModule module = new SimpleModule(); + module.addDeserializer(UsmUser.class, new UsmUserDeserializer()); + mapper.registerModule(module); + return mapper.readValue(usmUsersJson, new TypeReference<List<UsmUser>>() { + }); + } catch (JsonProcessingException e) { + throw new ProcessException("Could not parse USM user file, please check the processor details for examples.", e); + } + }; + } + + static UsmReader securityNamesUsmReader() { + return usmSecurityNames -> Arrays.stream(usmSecurityNames.trim().split(",")) + .map(securityName -> new UsmUser( + new OctetString(securityName), null, null, null, null) + ) + .collect(Collectors.toList()); + } + + class UsmUserDeserializer extends StdDeserializer<UsmUser> { + + public UsmUserDeserializer() { + this(null); + } + + public UsmUserDeserializer(Class<?> vc) { + super(vc); + } + + @Override + public UsmUser deserialize(JsonParser jp, DeserializationContext ctxt) throws IOException { + JsonNode node = jp.getCodec().readTree(jp); + String securityName = node.get("securityName").asText(); + String authProtocol = getTextOrNull(node, "authProtocol"); + String authPassphrase = getTextOrNull(node, "authPassphrase"); + String privProtocol = getTextOrNull(node, "privProtocol"); + String privPassphrase = getTextOrNull(node, "privPassphrase"); + + return new UsmUser( + new OctetString(securityName), + SNMPUtils.getAuth(authProtocol), + new OctetString(authPassphrase), Review Comment: This can fail with nullPtrException because in line 101 authPassphrase can be null. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
