This is an automated email from the ASF dual-hosted git repository.
jgresock pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/nifi.git
The following commit(s) were added to refs/heads/main by this push:
new 9237b4e NIFI-9722: Do not throw an Exception from
verifyCanUpdateProperties when property descriptor & parameter descriptor's
sensitivities don't match - instead allow the set to happen and let processor
become invalid. Also, allow values such as abc#{param} for ghost processors.
9237b4e is described below
commit 9237b4e923eeac6016d0ac605abf36c745ef0c63
Author: Mark Payne <[email protected]>
AuthorDate: Wed Feb 23 16:18:22 2022 -0500
NIFI-9722: Do not throw an Exception from verifyCanUpdateProperties when
property descriptor & parameter descriptor's sensitivities don't match -
instead allow the set to happen and let processor become invalid. Also, allow
values such as abc#{param} for ghost processors.
Signed-off-by: Joe Gresock <[email protected]>
This closes #5795.
---
.../nifi/controller/AbstractComponentNode.java | 24 ++--
.../nifi/controller/TestAbstractComponentNode.java | 134 ++++++++++++++++-----
2 files changed, 116 insertions(+), 42 deletions(-)
diff --git
a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java
b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java
index 943a3d3..edf26e4 100644
---
a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java
+++
b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java
@@ -331,7 +331,11 @@ public abstract class AbstractComponentNode implements
ComponentNode {
final PropertyDescriptor descriptor =
getPropertyDescriptor(propertyName);
- if (descriptor.isSensitive()) {
+ // We don't want to allow a sensitive property to reference a
parameter unless the value is solely a parameter reference. I.e.,
+ // #{abc} is ok but password#{abc} is not.
+ // However, for "ghost" components (isExtensionMissing() == true)
we need to allow this, because we consider all properties sensitive.
+ // If we don't allow this, we'll fail to even create the ghost
component.
+ if (descriptor.isSensitive() && !isExtensionMissing()) {
if (referenceList.size() > 1) {
throw new IllegalArgumentException("The property '" +
descriptor.getDisplayName() + "' cannot reference more than one Parameter
because it is a sensitive property.");
}
@@ -342,22 +346,12 @@ public abstract class AbstractComponentNode implements
ComponentNode {
throw new IllegalArgumentException("The property '" +
descriptor.getDisplayName() + "' is a sensitive property so it can reference a
Parameter only if there is no other " +
"context around the value. For instance, the value
'#{abc}' is allowed but 'password#{abc}' is not allowed.");
}
-
- final ParameterContext parameterContext =
getParameterContext();
- if (parameterContext != null) {
- final Optional<Parameter> parameter =
parameterContext.getParameter(reference.getParameterName());
- if (parameter.isPresent() &&
!parameter.get().getDescriptor().isSensitive()) {
- throw new IllegalArgumentException("The property
'" + descriptor.getDisplayName() + "' is a sensitive property, so it can only
reference Parameters that are sensitive.");
- }
- }
}
}
- if (descriptor.getControllerServiceDefinition() != null) {
- if (!referenceList.isEmpty()) {
- throw new IllegalArgumentException("The property '" +
descriptor.getDisplayName() + "' cannot reference a Parameter because the
property is a Controller Service reference. " +
- "Allowing Controller Service references to make use of
Parameters could result in security issues and a poor user experience. As a
result, this is not allowed.");
- }
+ if (descriptor.getControllerServiceDefinition() != null &&
!referenceList.isEmpty()) {
+ throw new IllegalArgumentException("The property '" +
descriptor.getDisplayName() + "' cannot reference a Parameter because the
property is a Controller Service reference. " +
+ "Allowing Controller Service references to make use of
Parameters could result in security issues and a poor user experience. As a
result, this is not allowed.");
}
}
}
@@ -811,7 +805,9 @@ public abstract class AbstractComponentNode implements
ComponentNode {
.valid(false)
.explanation("Property references Parameter '" +
paramName + "' but the currently selected Parameter Context does not have a
Parameter with that name")
.build());
+ continue;
}
+
final Optional<Parameter> parameterRef =
parameterContext.getParameter(paramName);
if (parameterRef.isPresent()) {
final ParameterDescriptor parameterDescriptor =
parameterRef.get().getDescriptor();
diff --git
a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/test/java/org/apache/nifi/controller/TestAbstractComponentNode.java
b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/test/java/org/apache/nifi/controller/TestAbstractComponentNode.java
index 4db5892..d43a450 100644
---
a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/test/java/org/apache/nifi/controller/TestAbstractComponentNode.java
+++
b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/test/java/org/apache/nifi/controller/TestAbstractComponentNode.java
@@ -25,7 +25,9 @@ import org.apache.nifi.components.PropertyDescriptor;
import org.apache.nifi.components.PropertyValue;
import org.apache.nifi.components.ValidationContext;
import org.apache.nifi.components.ValidationResult;
+import org.apache.nifi.components.Validator;
import org.apache.nifi.components.validation.EnablingServiceValidationResult;
+import org.apache.nifi.components.validation.ValidationState;
import org.apache.nifi.components.validation.ValidationStatus;
import org.apache.nifi.components.validation.ValidationTrigger;
import org.apache.nifi.controller.service.ControllerServiceNode;
@@ -55,6 +57,9 @@ import java.util.concurrent.atomic.AtomicLong;
import java.util.function.Function;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
public class TestAbstractComponentNode {
@@ -66,6 +71,7 @@ public class TestAbstractComponentNode {
assertEquals(ValidationStatus.VALIDATING, status);
}
+ @Test
public void testOnParametersModified() {
final AtomicLong validationCount = new AtomicLong(0L);
final ValidationTrigger validationTrigger = new ValidationTrigger() {
@@ -89,10 +95,6 @@ public class TestAbstractComponentNode {
}
};
- final Map<String, String> properties = new HashMap<>();
- properties.put("abc", "#{abc}");
- node.setProperties(properties);
-
final ParameterContext context = Mockito.mock(ParameterContext.class);
final ParameterDescriptor paramDescriptor = new
ParameterDescriptor.Builder()
.name("abc")
@@ -100,18 +102,73 @@ public class TestAbstractComponentNode {
.sensitive(false)
.build();
final Parameter param = new Parameter(paramDescriptor, "123");
- Mockito.doReturn(param).when(context).getParameter("abc");
+ Mockito.doReturn(Optional.of(param)).when(context).getParameter("abc");
+ node.setParameterContext(context);
+
+ final Map<String, String> properties = new HashMap<>();
+ properties.put("abc", "#{abc}");
+ node.setProperties(properties);
+
+ assertEquals(1, propertyModifications.size());
+ PropertyModification mod = propertyModifications.get(0);
+ assertNull(mod.getPreviousValue());
+ assertEquals("123", mod.getUpdatedValue());
+ propertyModifications.clear();
final Map<String, ParameterUpdate> updatedParameters = new HashMap<>();
- updatedParameters.put("abc", new MockParameterUpdate("abc", "xyz",
"123", false));
+ updatedParameters.put("abc", new MockParameterUpdate("abc",
"old-value", "123", false));
node.onParametersModified(updatedParameters);
assertEquals(1, propertyModifications.size());
- final PropertyModification mod = propertyModifications.get(0);
- assertEquals("xyz", mod.getPreviousValue());
+ mod = propertyModifications.get(0);
+ assertEquals("old-value", mod.getPreviousValue());
assertEquals("123", mod.getUpdatedValue());
}
+ @Test
+ public void testMismatchedSensitiveFlags() {
+ final LocalComponentNode node = new LocalComponentNode();
+
+ final ParameterContext context = Mockito.mock(ParameterContext.class);
+ final ParameterDescriptor paramDescriptor = new
ParameterDescriptor.Builder()
+ .name("abc")
+ .description("")
+ .sensitive(true)
+ .build();
+ final Parameter param = new Parameter(paramDescriptor, "123");
+ Mockito.doReturn(Optional.of(param)).when(context).getParameter("abc");
+ node.setParameterContext(context);
+
+ final String propertyValue = "#{abc}";
+ final PropertyDescriptor propertyDescriptor = new
PropertyDescriptor.Builder()
+ .name("abc")
+ .sensitive(false)
+ .dynamic(true)
+ .addValidator(Validator.VALID)
+ .build();
+
+ final Map<String, String> properties = new HashMap<>();
+ properties.put("abc", propertyValue);
+ node.verifyCanUpdateProperties(properties);
+ node.setProperties(properties);
+
+ final ValidationContext validationContext =
Mockito.mock(ValidationContext.class);
+
Mockito.when(validationContext.getProperties()).thenReturn(Collections.singletonMap(propertyDescriptor,
propertyValue));
+
Mockito.when(validationContext.getAllProperties()).thenReturn(properties);
+
Mockito.when(validationContext.isDependencySatisfied(Mockito.any(PropertyDescriptor.class),
Mockito.any(Function.class))).thenReturn(true);
+
Mockito.when(validationContext.getReferencedParameters(Mockito.anyString())).thenReturn(Collections.singleton("abc"));
+
Mockito.when(validationContext.isParameterDefined("abc")).thenReturn(true);
+
+ final ValidationState validationState =
node.performValidation(validationContext);
+ assertSame(ValidationStatus.INVALID, validationState.getStatus());
+
+ final Collection<ValidationResult> results =
validationState.getValidationErrors();
+ assertEquals(1, results.size());
+ final ValidationResult result = results.iterator().next();
+ assertFalse(result.isValid());
+
assertTrue(result.getExplanation().toLowerCase().contains("sensitivity"));
+ }
+
@Test(timeout = 10000)
public void testValidationTriggerPaused() throws InterruptedException {
final AtomicLong validationCount = new AtomicLong(0L);
@@ -191,30 +248,17 @@ public class TestAbstractComponentNode {
return context;
}
- private static class ValidationControlledAbstractComponentNode extends
AbstractComponentNode {
- private final long pauseMillis;
+ private static class LocalComponentNode extends AbstractComponentNode {
private volatile ParameterContext paramContext = null;
- public ValidationControlledAbstractComponentNode(final long
pauseMillis, final ValidationTrigger validationTrigger) {
- this(pauseMillis, validationTrigger,
Mockito.mock(ControllerServiceProvider.class));
+ public LocalComponentNode() {
+ this(Mockito.mock(ControllerServiceProvider.class),
Mockito.mock(ValidationTrigger.class));
}
- public ValidationControlledAbstractComponentNode(final long
pauseMillis, final ValidationTrigger validationTrigger, final
ControllerServiceProvider controllerServiceProvider) {
+ public LocalComponentNode(final ControllerServiceProvider
controllerServiceProvider, final ValidationTrigger validationTrigger) {
super("id", Mockito.mock(ValidationContextFactory.class),
controllerServiceProvider, "unit test component",
-
ValidationControlledAbstractComponentNode.class.getCanonicalName(),
Mockito.mock(ComponentVariableRegistry.class),
Mockito.mock(ReloadComponent.class),
- Mockito.mock(ExtensionManager.class), validationTrigger,
false);
-
- this.pauseMillis = pauseMillis;
- }
-
- @Override
- protected Collection<ValidationResult>
computeValidationErrors(ValidationContext context) {
- try {
- Thread.sleep(pauseMillis);
- } catch (final InterruptedException ie) {
- }
-
- return null;
+
ValidationControlledAbstractComponentNode.class.getCanonicalName(),
Mockito.mock(ComponentVariableRegistry.class),
Mockito.mock(ReloadComponent.class),
+ Mockito.mock(ExtensionManager.class), validationTrigger,
false);
}
@Override
@@ -228,7 +272,17 @@ public class TestAbstractComponentNode {
@Override
public ConfigurableComponent getComponent() {
- return Mockito.mock(ConfigurableComponent.class);
+ final ConfigurableComponent component =
Mockito.mock(ConfigurableComponent.class);
+
Mockito.when(component.getPropertyDescriptor(Mockito.anyString())).thenAnswer(invocation
-> {
+ final String propertyName = invocation.getArgument(0,
String.class);
+ return new PropertyDescriptor.Builder()
+ .name(propertyName)
+ .addValidator(Validator.VALID)
+ .dynamic(true)
+ .build();
+ });
+
+ return component;
}
@Override
@@ -290,6 +344,30 @@ public class TestAbstractComponentNode {
}
}
+
+ private static class ValidationControlledAbstractComponentNode extends
LocalComponentNode {
+ private final long pauseMillis;
+
+ public ValidationControlledAbstractComponentNode(final long
pauseMillis, final ValidationTrigger validationTrigger) {
+ this(pauseMillis, validationTrigger,
Mockito.mock(ControllerServiceProvider.class));
+ }
+
+ public ValidationControlledAbstractComponentNode(final long
pauseMillis, final ValidationTrigger validationTrigger, final
ControllerServiceProvider controllerServiceProvider) {
+ super(controllerServiceProvider, validationTrigger);
+ this.pauseMillis = pauseMillis;
+ }
+
+ @Override
+ protected Collection<ValidationResult>
computeValidationErrors(ValidationContext context) {
+ try {
+ Thread.sleep(pauseMillis);
+ } catch (final InterruptedException ie) {
+ }
+
+ return null;
+ }
+ }
+
private static class PropertyModification {
private final PropertyDescriptor propertyDescriptor;
private final String previousValue;
