You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "turcsanyip (via GitHub)" <gi...@apache.org> on 2023/06/01 14:34:23 UTC
[GitHub] [nifi] turcsanyip commented on a diff in pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
turcsanyip commented on code in PR #7313:
URL: https://github.com/apache/nifi/pull/7313#discussion_r1213223437
##########
nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/java/org/apache/nifi/jms/cf/JndiJmsConnectionFactoryProperties.java:
##########
@@ -114,4 +126,108 @@ public static PropertyDescriptor getDynamicPropertyDescriptor(final String prope
.build();
}
+ private static class JndiJmsContextFactoryValidator implements Validator {
+ private static final String DEFAULT_DISALLOWED_CONTEXT_FACTORY = "LdapCtxFactory";
+
+ private static final String DISALLOWED_CONTEXT_FACTORY;
+
+ static {
+ DISALLOWED_CONTEXT_FACTORY = System.getProperty(CONTEXT_FACTORY_DISALLOWED_PROPERTY, DEFAULT_DISALLOWED_CONTEXT_FACTORY);
+ }
+
+ @Override
+ public ValidationResult validate(final String subject, final String input, final ValidationContext context) {
+ final ValidationResult.Builder builder = new ValidationResult.Builder().subject(subject).input(input);
+
+ if (input == null || input.isEmpty()) {
+ builder.valid(false);
+ builder.explanation("Context Factory is required");
+ } else if (input.endsWith(DISALLOWED_CONTEXT_FACTORY)) {
Review Comment:
Do we expect to disallow only one context factory in the future?
If the main target is `LdapCtxFactory`, then we could use a more explicit property like "disallow.ldap" with true/false.
Or we can make it generic via accepting multiple values. I don't think someone would want to disable another context factory but enable ldap at the same time.
##########
nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/java/org/apache/nifi/jms/cf/JndiJmsConnectionFactoryProperties.java:
##########
@@ -114,4 +126,108 @@ public static PropertyDescriptor getDynamicPropertyDescriptor(final String prope
.build();
}
+ private static class JndiJmsContextFactoryValidator implements Validator {
+ private static final String DEFAULT_DISALLOWED_CONTEXT_FACTORY = "LdapCtxFactory";
+
+ private static final String DISALLOWED_CONTEXT_FACTORY;
+
+ static {
+ DISALLOWED_CONTEXT_FACTORY = System.getProperty(CONTEXT_FACTORY_DISALLOWED_PROPERTY, DEFAULT_DISALLOWED_CONTEXT_FACTORY);
Review Comment:
It seems `LdapCtxFactory` can be enabled via the system property only if some dummy value is set. Empty string value should be handled as "allow anything".
##########
nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/resources/docs/org.apache.nifi.jms.cf.JndiJmsConnectionFactoryProvider/additionalDetails.html:
##########
@@ -91,5 +91,77 @@ <h2>Example:</h2>
the jar(s) containing the org.apache.activemq.jndi.ActiveMQInitialContextFactory class and the other JMS client classes can be found within the /opt/apache-activemq-5.15.2/lib/ directory.
</p>
+<h2>Property Validation</h2>
+
+<p>
+ The following component properties include additional validation to restrict allowed values:
+</p>
+
+<ul>
+ <li>JNDI Initial Context Factory Class</li>
+ <li>JNDI Provider URL</li>
+</ul>
+
+<h3>JNDI Initial Context Factory Class Validation</h3>
+
+<p>
+ The default validation for <code>JNDI Initial Context Factory Class</code> disallows the following class values:
+</p>
+
+<ul>
+ <li><code>com.sun.jndi.ldap.LdapCtxFactory</code></li>
+</ul>
+
+<p>
+ Specialized configurations can retrieve JNDI JMS settings from an LDAP server, requiring the use of the LDAP Context Factory.
+ Property validation for the Context Factory Class can be adjusted using Java System properties.
+</p>
+<p>
+ The following Java System property can be configured to override the default disallowed Context Factory Class:
+</p>
+
+<ul>
+ <li>
+ <code>org.apache.nifi.jms.cf.jndi.context.factory.disallowed</code>
+ </li>
+</ul>
+
+<h3>JNDI Provider URL Validation</h3>
+
+<p>
+ The default validation for <code>JNDI Provider URL</code> allows the following URL schemes:
+</p>
+
+<ul>
+ <li>file</li>
+ <li>jgroups</li>
+ <li>ssl</li>
+ <li>t3</li>
+ <li>tcp</li>
+ <li>udp</li>
+ <li>vm</li>
Review Comment:
The list is not in sync with the code. `t3s` needs to be added.
##########
nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/resources/docs/org.apache.nifi.jms.cf.JndiJmsConnectionFactoryProvider/additionalDetails.html:
##########
@@ -91,5 +91,77 @@ <h2>Example:</h2>
the jar(s) containing the org.apache.activemq.jndi.ActiveMQInitialContextFactory class and the other JMS client classes can be found within the /opt/apache-activemq-5.15.2/lib/ directory.
</p>
+<h2>Property Validation</h2>
+
+<p>
+ The following component properties include additional validation to restrict allowed values:
+</p>
+
+<ul>
+ <li>JNDI Initial Context Factory Class</li>
+ <li>JNDI Provider URL</li>
+</ul>
+
+<h3>JNDI Initial Context Factory Class Validation</h3>
+
+<p>
+ The default validation for <code>JNDI Initial Context Factory Class</code> disallows the following class values:
+</p>
+
+<ul>
+ <li><code>com.sun.jndi.ldap.LdapCtxFactory</code></li>
+</ul>
+
+<p>
+ Specialized configurations can retrieve JNDI JMS settings from an LDAP server, requiring the use of the LDAP Context Factory.
+ Property validation for the Context Factory Class can be adjusted using Java System properties.
+</p>
+<p>
+ The following Java System property can be configured to override the default disallowed Context Factory Class:
+</p>
+
+<ul>
+ <li>
+ <code>org.apache.nifi.jms.cf.jndi.context.factory.disallowed</code>
+ </li>
+</ul>
Review Comment:
I think it would be worth adding an example for the system property definition (as in case of the schemes property below). Also for how to enable `LdapCtxFactory`.
##########
nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/test/java/org/apache/nifi/jms/cf/JndiJmsConnectionFactoryProviderTest.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.jms.cf;
+
+import org.apache.nifi.reporting.InitializationException;
+import org.apache.nifi.util.NoOpProcessor;
+import org.apache.nifi.util.TestRunner;
+import org.apache.nifi.util.TestRunners;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class JndiJmsConnectionFactoryProviderTest {
+
+ private static final String SERVICE_ID = JndiJmsConnectionFactoryProvider.class.getSimpleName();
+
+ private static final String CONTEXT_FACTORY = "ContextFactory";
+
+ private static final String FACTORY_NAME = "ConnectionFactory";
+
+ private static final String TCP_PROVIDER_URL = "tcp://127.0.0.1";
+
+ private static final String LDAP_PROVIDER_URL = "ldap://127.0.0.1";
+
+ private static final String TEST_PROVIDER_URL = "test://127.0.0.1";
+
+ private static final String HOST_PORT_URL = "127.0.0.1:1024";
+
+ private static final String ALLOWED_URL_SCHEMES = "tcp test";
+
+ private static final String LDAP_CONTEXT_FACTORY = "com.sun.jndi.ldap.LdapCtxFactory";
+
+ private TestRunner runner;
+
+ private JndiJmsConnectionFactoryProvider provider;
+
+ @BeforeAll
+ static void setProperties() {
+ System.setProperty(JndiJmsConnectionFactoryProperties.URL_SCHEMES_ALLOWED_PROPERTY, ALLOWED_URL_SCHEMES);
+ }
+
+ @AfterAll
+ static void clearProperties() {
+ System.clearProperty(JndiJmsConnectionFactoryProperties.URL_SCHEMES_ALLOWED_PROPERTY);
+ }
+
+ @BeforeEach
+ void setRunner() throws InitializationException {
+ runner = TestRunners.newTestRunner(NoOpProcessor.class);
+ provider = new JndiJmsConnectionFactoryProvider();
+ runner.addControllerService(SERVICE_ID, provider);
+ }
+
+ @Test
+ void testPropertiesValid() {
+ setFactoryProperties();
+
+ runner.setProperty(provider, JndiJmsConnectionFactoryProperties.JNDI_PROVIDER_URL, TCP_PROVIDER_URL);
+
+ runner.assertValid(provider);
+ }
+
+ @Test
+ void testPropertiesInvalidUrlNotConfigured() {
+ setFactoryProperties();
+
+ runner.assertNotValid(provider);
+ }
+
+ @Test
+ void testPropertiesInvalidUrlScheme() {
+ setFactoryProperties();
+
+ runner.setProperty(provider, JndiJmsConnectionFactoryProperties.JNDI_PROVIDER_URL, LDAP_PROVIDER_URL);
+
+ runner.assertNotValid(provider);
+ }
+
+ @Test
+ void testPropertiesInvalidContextFactory() {
+ runner.setProperty(provider, JndiJmsConnectionFactoryProperties.JNDI_INITIAL_CONTEXT_FACTORY, LDAP_CONTEXT_FACTORY);
+ runner.setProperty(provider, JndiJmsConnectionFactoryProperties.JNDI_CONNECTION_FACTORY_NAME, FACTORY_NAME);
+ runner.setProperty(provider, JndiJmsConnectionFactoryProperties.JNDI_PROVIDER_URL, TCP_PROVIDER_URL);
+
+ runner.assertNotValid(provider);
+ }
+
+ @Test
+ void testPropertiesUrlSchemeSystemProperty() {
+ setFactoryProperties();
+
+ runner.setProperty(provider, JndiJmsConnectionFactoryProperties.JNDI_PROVIDER_URL, TEST_PROVIDER_URL);
+
+ runner.assertValid(provider);
+ }
Review Comment:
This test case tests the same as `testPropertiesValid()` because both `tcp` and `test` are set in the system property for all tests. It would be better to apply the system property for this test only (as its name would imply).
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org