You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/04/13 18:07:56 UTC
[GitHub] [nifi] markap14 commented on a diff in pull request #5958: NIFI-9895 Allow parameters to reference controller services
markap14 commented on code in PR #5958:
URL: https://github.com/apache/nifi/pull/5958#discussion_r849445707
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/authorization/AuthorizeControllerServiceReference.java:
##########
@@ -88,9 +91,17 @@ public static void authorizeControllerServiceReferences(final Map<String, String
// if this descriptor identifies a controller service
if (propertyDescriptor.getControllerServiceDefinition() != null) {
- final String currentValue = authorizable.getValue(propertyDescriptor);
final String proposedValue = entry.getValue();
+ ComponentNode componentNode = (ComponentNode) authorizable.getAuthorizable();
Review Comment:
This is not a safe cast. If we are sure that `ComponentAuthorizable#getAuthorizable` will always return an `ComponentNode`, we should update the interface to return a `ComponentNode` instead of an `Authorizable`
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ParameterContextResource.java:
##########
@@ -390,12 +397,62 @@ public Response submitParameterContextUpdate(
requestRevision,
lookup -> {
// Verify READ and WRITE permissions for user, for the Parameter Context itself
- final Authorizable parameterContext = lookup.getParameterContext(contextId);
+ final ParameterContext parameterContext = lookup.getParameterContext(contextId);
parameterContext.authorize(authorizer, RequestAction.READ, user);
parameterContext.authorize(authorizer, RequestAction.WRITE, user);
// Verify READ and WRITE permissions for user, for every component that is affected
affectedComponents.forEach(component -> authorizeAffectedComponent(component, lookup, user, true, true));
+
+ requestEntity.getComponent().getParameters().forEach(parameterEntity -> {
+ String parameterName = parameterEntity.getParameter().getName();
+ List<ParameterReferencedControllerServiceData> referencedControllerServiceDataSet = parameterContext
+ .getParameterReferenceManager()
+ .getReferencedControllerServiceData(parameterContext, parameterName);
+
+ Set<? extends Class<? extends ControllerService>> referencedControllerServiceTypes = referencedControllerServiceDataSet
+ .stream()
+ .map(ParameterReferencedControllerServiceData::getReferencedControllerServiceType)
+ .collect(Collectors.toSet());
+
+ if (referencedControllerServiceTypes.size() > 1) {
+ throw new IllegalStateException("Parameter is used by multiple different types of controller service references");
+ } else if (!referencedControllerServiceTypes.isEmpty()) {
+ Optional.ofNullable(parameterEntity)
+ .map(ParameterEntity::getParameter)
+ .map(ParameterDTO::getName)
+ .flatMap(parameterContext::getParameter)
+ .map(Parameter::getValue)
+ .map(lookup::getControllerService)
+ .map(ComponentAuthorizable::getAuthorizable)
+ .ifPresent(controllerServiceAuthorizable -> {
+ controllerServiceAuthorizable.authorize(authorizer, RequestAction.READ, user);
+ controllerServiceAuthorizable.authorize(authorizer, RequestAction.WRITE, user);
+ });
+
+ Optional.ofNullable(parameterEntity)
+ .map(ParameterEntity::getParameter)
+ .map(ParameterDTO::getValue)
+ .map(lookup::getControllerService)
+ .map(ComponentAuthorizable::getAuthorizable)
+ .ifPresent(controllerServiceAuthorizable -> {
+ controllerServiceAuthorizable.authorize(authorizer, RequestAction.READ, user);
+ controllerServiceAuthorizable.authorize(authorizer, RequestAction.WRITE, user);
+
+ if (
+ !referencedControllerServiceTypes
+ .stream()
+ .findFirst()
+ .get()
+ .isAssignableFrom(
+ ((StandardControllerServiceNode) controllerServiceAuthorizable).getComponent().getClass()
+ )
+ ) {
+ throw new IllegalArgumentException("New Parameter value attempts to reference an incompatible controller service");
+ }
+ });
+ }
+ });
Review Comment:
We should be preferring procedural/imperative coding over functional coding where it makes sense. As a general rule of thumb, a lambda should not exceed 3-5 lines of code.
See Google's Guava's recommendations/explanations here as to why: https://github.com/google/guava/wiki/FunctionalExplained
As well as Effective Java by Joshua Bloch, where he discusses the some reasons for using functional style judiciously and keeping lambdas to only a few lines.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java:
##########
@@ -460,18 +455,14 @@ private void setProperty(final String name, final PropertyConfiguration property
// If it previously referenced a Controller Service, we need to also remove that reference.
// It is okay if the new & old values are the same - we just unregister the component/descriptor and re-register it.
if (descriptor.getControllerServiceDefinition() != null) {
- if (oldConfiguration != null) {
- final String oldEffectiveValue = oldConfiguration.getEffectiveValue(getParameterContext());
- final ControllerServiceNode oldNode = serviceProvider.getControllerServiceNode(oldEffectiveValue);
- if (oldNode != null) {
- oldNode.removeReference(this, descriptor);
- }
- }
-
- final ControllerServiceNode newNode = serviceProvider.getControllerServiceNode(effectiveValue);
- if (newNode != null) {
- newNode.addReference(this, descriptor);
- }
+ Optional.ofNullable(oldConfiguration)
+ .map(_oldConfiguration -> _oldConfiguration.getEffectiveValue(getParameterContext()))
+ .map(oldEffectiveValue -> serviceProvider.getControllerServiceNode(oldEffectiveValue))
+ .ifPresent(oldNode -> oldNode.removeReference(this, descriptor));
+
+ Optional.ofNullable(effectiveValue)
+ .map(_effectiveValue -> serviceProvider.getControllerServiceNode(_effectiveValue))
+ .ifPresent(newNode -> newNode.addReference(this, descriptor));
Review Comment:
Why the change here from imperative to functional style? We should be preferring imperative coding style unless there's a very significant reason to use functional.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/authorization/AuthorizeControllerServiceReference.java:
##########
@@ -109,17 +120,54 @@ public static void authorizeControllerServiceReferences(final Map<String, String
final ParameterTokenList tokenList = parser.parseTokens(proposedValue);
final boolean referencesParameter = !tokenList.toReferenceList().isEmpty();
if (referencesParameter) {
- throw new IllegalArgumentException("The property '" + propertyDescriptor.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.");
+ authorizeControllerServiceReference(authorizable, authorizer, lookup, user, propertyDescriptor, proposedEffectiveValue);
+ } else {
+ final Authorizable newServiceAuthorizable = lookup.getControllerService(proposedValue).getAuthorizable();
+ newServiceAuthorizable.authorize(authorizer, RequestAction.READ, user);
}
-
- final Authorizable newServiceAuthorizable = lookup.getControllerService(proposedValue).getAuthorizable();
- newServiceAuthorizable.authorize(authorizer, RequestAction.READ, user);
}
}
}
}
}
}
+
+ /**
+ * Authorizes a proposed new controller service reference.
+ *
+ * @param authorizable authorizable that may reference a controller service
+ * @param authorizer authorizer
+ * @param lookup lookup
+ * @param user user
+ * @param propertyDescriptor the propertyDescriptor referencing a controller service
+ * @param proposedEffectiveValue the new proposed value (id of the controller service) to use as a reference
+ */
+ public static void authorizeControllerServiceReference(
+ ComponentAuthorizable authorizable,
+ Authorizer authorizer,
+ AuthorizableLookup lookup,
+ NiFiUser user,
+ PropertyDescriptor propertyDescriptor,
+ String proposedEffectiveValue
+ ) {
+ final String currentValue = authorizable.getValue(propertyDescriptor);
+
+ if (authorizable.getAuthorizable() instanceof ComponentNode) {
+ authorize(authorizer, lookup, user, currentValue);
+ authorize(authorizer, lookup, user, proposedEffectiveValue);
+ } else {
+ throw new IllegalArgumentException("The property '" + propertyDescriptor.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.");
Review Comment:
This is no longer an accurate message for the Exception, as we're now allowing this and providing sufficient authorization in order to avoid any security issues.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/registry/flow/mapping/NiFiRegistryFlowMapper.java:
##########
@@ -822,7 +817,42 @@ private void mapParameterContext(final ParameterContext parameterContext, final
parameterContexts.put(versionedContext.getName(), versionedContext);
}
+ private Set<VersionedParameter> mapParameters(ParameterContext parameterContext) {
+ final Set<VersionedParameter> parameters = parameterContext.getParameters().entrySet().stream()
+ .map(descriptorAndParameter -> {
+ VersionedParameter versionedParameter;
+
+ ParameterDescriptor parameterDescriptor = descriptorAndParameter.getKey();
+ Parameter parameter = descriptorAndParameter.getValue();
+
+ if (this.flowMappingOptions.isMapControllerServiceReferencesToVersionedId()) {
+ List<ParameterReferencedControllerServiceData> referencedControllerServiceData = parameterContext
+ .getParameterReferenceManager()
+ .getReferencedControllerServiceData(parameterContext, parameterDescriptor.getName());
+
+ if (referencedControllerServiceData.isEmpty()) {
+ versionedParameter = mapParameter(parameter);
+ } else {
+ versionedParameter = mapParameter(
+ parameter,
+ getId(Optional.ofNullable(referencedControllerServiceData.get(0).getVersionedServiceId()), parameter.getValue())
+ );
+ }
+ } else {
+ versionedParameter = mapParameter(parameter);
+ }
+
+ return versionedParameter;
+ })
Review Comment:
We should avoid large lambdas in favor or imperative/procedural coding. See additional comments below on this. It's simple enough to create a method that takes in a ParameterDescriptor and a Parameter and returns a Versioned Parameter and then call that from the lambda so that the code is more readable and IDE/debugger friendly. Or to simply use a `for-each` loop here to iterate over the entry set.
--
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