You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "exceptionfactory (via GitHub)" <gi...@apache.org> on 2023/05/30 18:30:59 UTC
[GitHub] [nifi] exceptionfactory opened a new pull request, #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
exceptionfactory opened a new pull request, #7313:
URL: https://github.com/apache/nifi/pull/7313
# Summary
[NIFI-11614](https://issues.apache.org/jira/browse/NIFI-11614) Improves property validation for the `JndiJmsConnectionFactoryProvider` Controller Service with a set of allowed URL schemes and disallowed Context Factory classes.
The current implementation only checks for a configured value with standard properties, but the new validation narrows the scope to provide better validation prior to enabling the provider.
The initial set of allowed URL schemes is based on the [ActiveMQ Using JMS](https://activemq.apache.org/components/artemis/documentation/latest/using-jms.html) documentation. The new URL validation narrows the scope of potential values, which should be noted in migration guidance, providing a clearer set of supported values
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [X] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [X] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [X] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [X] Pull Request based on current revision of the `main` branch
- [X] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [ ] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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
[GitHub] [nifi] exceptionfactory commented on pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #7313:
URL: https://github.com/apache/nifi/pull/7313#issuecomment-1572267552
Thanks for the additional feedback @turcsanyip!
On further consideration, the URL Scheme validation seems sufficient, so I removed the Context Factory class validation and associated documentation. I also adjusted the System property evaluation approach so that the unit tests could evaluate both scenarios.
--
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
[GitHub] [nifi] turcsanyip commented on a diff in pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
Posted by "turcsanyip (via GitHub)" <gi...@apache.org>.
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7313:
URL: https://github.com/apache/nifi/pull/7313#discussion_r1213328925
##########
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:
Removed this validator and associated documentation section.
--
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
[GitHub] [nifi] turcsanyip commented on pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
Posted by "turcsanyip (via GitHub)" <gi...@apache.org>.
turcsanyip commented on PR #7313:
URL: https://github.com/apache/nifi/pull/7313#issuecomment-1570052084
Will review...
--
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
[GitHub] [nifi] asfgit closed pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
URL: https://github.com/apache/nifi/pull/7313
--
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
[GitHub] [nifi] schnells commented on pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
Posted by "schnells (via GitHub)" <gi...@apache.org>.
schnells commented on PR #7313:
URL: https://github.com/apache/nifi/pull/7313#issuecomment-1642339407
Thanks @exceptionfactory for the quick response! I will absolutely have a look into the java system property.
I will create a Jira issue in the near future.
--
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
[GitHub] [nifi] schnells commented on pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
Posted by "schnells (via GitHub)" <gi...@apache.org>.
schnells commented on PR #7313:
URL: https://github.com/apache/nifi/pull/7313#issuecomment-1642293355
Thanks for implementing a validator. Because these are very useful.
Unfortunately, this currently prevents the use of salace and nifi. the solace messaging protocol (SMF) uses a url format like `smfs://<solace>:<port>`
Unfortunately the validator fails with `smfs` urls. Is it possible to include smfs in the validator list?
I would also open the necessary pull request. Because currently we cannot use nifi 1.22 in combination with solace.
--
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
[GitHub] [nifi] turcsanyip commented on a diff in pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
Posted by "turcsanyip (via GitHub)" <gi...@apache.org>.
turcsanyip commented on code in PR #7313:
URL: https://github.com/apache/nifi/pull/7313#discussion_r1211762688
##########
nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/java/org/apache/nifi/jms/cf/JndiJmsConnectionFactoryProperties.java:
##########
@@ -114,4 +119,67 @@ public static PropertyDescriptor getDynamicPropertyDescriptor(final String prope
.build();
}
+ private static class JndiJmsContextFactoryValidator implements Validator {
+ private static final String DISALLOWED_CONTEXT_FACTORY = "LdapCtxFactory";
+
+ @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)) {
+ builder.valid(false);
+ builder.explanation(String.format("Context Factory [%s] not allowed", DISALLOWED_CONTEXT_FACTORY));
+ } else {
+ builder.valid(true);
+ builder.explanation("Context Factory allowed");
+ }
+
+ return builder.build();
+ }
+ }
+
+ private static class JndiJmsProviderUrlValidator implements Validator {
+ /** JNDI JMS URL Allowed Schemes based on ActiveMQ Connection Factory */
+ private static final Set<String> ALLOWED_SCHEMES = Collections.unmodifiableSet(new LinkedHashSet<>(Arrays.asList(
+ "jgroups",
+ "tcp",
+ "udp",
+ "vm"
+ )));
Review Comment:
@exceptionfactory I have concerns that we can enumerate all supported schemes because they are vendor specific. E.g. WebLogic uses `t3(s)://`. Basically, it can be an arbitrary name.
I also understand that `ldap://` is problematic but WebSphere MQ relies on it when using JNDI (or the local `file://` can be used).
Do you have any idea how to handle these shortcomings while keeping the validation logic?
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7313:
URL: https://github.com/apache/nifi/pull/7313#discussion_r1211851261
##########
nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/java/org/apache/nifi/jms/cf/JndiJmsConnectionFactoryProperties.java:
##########
@@ -114,4 +119,67 @@ public static PropertyDescriptor getDynamicPropertyDescriptor(final String prope
.build();
}
+ private static class JndiJmsContextFactoryValidator implements Validator {
+ private static final String DISALLOWED_CONTEXT_FACTORY = "LdapCtxFactory";
+
+ @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)) {
+ builder.valid(false);
+ builder.explanation(String.format("Context Factory [%s] not allowed", DISALLOWED_CONTEXT_FACTORY));
+ } else {
+ builder.valid(true);
+ builder.explanation("Context Factory allowed");
+ }
+
+ return builder.build();
+ }
+ }
+
+ private static class JndiJmsProviderUrlValidator implements Validator {
+ /** JNDI JMS URL Allowed Schemes based on ActiveMQ Connection Factory */
+ private static final Set<String> ALLOWED_SCHEMES = Collections.unmodifiableSet(new LinkedHashSet<>(Arrays.asList(
+ "jgroups",
+ "tcp",
+ "udp",
+ "vm"
+ )));
Review Comment:
Thanks for highlighting these concerns @turcsanyip.
The restrictive approach to URL schemes does present some potential limitations.
I expanded the default list of allowed schemes, and I also introduced a Java System property that supports overriding the default values. I updated the additional details documentation to list the property name and describe how it can be configured. This approach will allow an administrator to customize validation for specific environments and custom drivers while providing default validation for general deployments.
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7313:
URL: https://github.com/apache/nifi/pull/7313#discussion_r1213329757
##########
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:
Thanks for pointing this out. I adjusted the System property evaluation and changed the test approach to support setting and clearing the System property in the context of a single test method.
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7313:
URL: https://github.com/apache/nifi/pull/7313#discussion_r1213328201
##########
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:
Thanks for highlighting this concern. On further evaluation, after implementing the ability to override the default validation using Java System properties, it seems better to limit the validation to the URL schemes. The latest commit removes this validation.
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7313:
URL: https://github.com/apache/nifi/pull/7313#discussion_r1213328568
##########
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:
Removed this validator as described.
--
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
[GitHub] [nifi] exceptionfactory commented on pull request #7313: NIFI-11614 Improve Validation for JndiJmsConnectionFactoryProvider
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #7313:
URL: https://github.com/apache/nifi/pull/7313#issuecomment-1642301552
Thanks for the feedback @schnells. As this pull request is closed, it would be helpful to create an Apache NiFi Jira issue for further discussion.
Adding `smfs` to the default list sounds like a reasonable improvement.
As an alternative for now, the Provider supports a Java System property that can be used to set the allowed URLs.
https://nifi.apache.org/docs/nifi-docs/components/org.apache.nifi/nifi-jms-processors-nar/1.22.0/org.apache.nifi.jms.cf.JndiJmsConnectionFactoryProvider/additionalDetails.html
In this scenario, the following line can be added to `bootstrap.conf` to allow `smfs`:
```
java.arg.jndiJmsUrlSchemesAllowed=-Dorg.apache.nifi.jms.cf.jndi.provider.url.schemes.allowed=smfs
```
--
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