NIFI-4744 Detect incorrect authorizers config

Adds stricter checks in AuthorizerFactoryBean for unique ids within
a given type of provider and requires unique providers in composite
and composite-configurable user group providers. Failed validation
checks cause startup to fail. Adds test cases for these new rules.

This closes #2419.

Signed-off-by: Bryan Bende <bbende@apache.org>

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-authorizer/pom.xml

@@ -95,6 +95,18 @@
             <groupId>org.apache.nifi</groupId>
             <artifactId>nifi-properties-loader</artifactId>
         </dependency>
+
+        <!-- Spock testing dependencies-->
+        <dependency>
+            <groupId>org.spockframework</groupId>
+            <artifactId>spock-core</artifactId>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>cglib</groupId>
+            <artifactId>cglib-nodep</artifactId>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
 </project>

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-authorizer/src/main/java/org/apache/nifi/authorization/AuthorizerFactoryBean.java

@@ -44,6 +44,7 @@ import javax.xml.bind.JAXBContext;
 import javax.xml.bind.JAXBElement;
 import javax.xml.bind.JAXBException;
 import javax.xml.bind.Unmarshaller;
+import javax.xml.stream.XMLStreamException;
 import javax.xml.stream.XMLStreamReader;
 import javax.xml.transform.stream.StreamSource;
 import javax.xml.validation.Schema;
@@ -56,9 +57,11 @@ import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.net.URL;
 import java.net.URLClassLoader;
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
 
 /**
  * Factory bean for loading the configured authorizer.
@@ -123,6 +126,9 @@ public class AuthorizerFactoryBean implements FactoryBean, DisposableBean, UserG
 
                     // create each user group provider
                     for (final org.apache.nifi.authorization.generated.UserGroupProvider userGroupProvider : authorizerConfiguration.getUserGroupProvider()) {
+                        if (userGroupProviders.containsKey(userGroupProvider.getIdentifier())) {
+                            throw new Exception("Duplicate User Group Provider identifier in Authorizers configuration: " + userGroupProvider.getIdentifier());
+                        }
                         userGroupProviders.put(userGroupProvider.getIdentifier(), createUserGroupProvider(userGroupProvider.getIdentifier(), userGroupProvider.getClazz()));
                     }
 
@@ -134,6 +140,9 @@ public class AuthorizerFactoryBean implements FactoryBean, DisposableBean, UserG
 
                     // create each access policy provider
                     for (final org.apache.nifi.authorization.generated.AccessPolicyProvider accessPolicyProvider : authorizerConfiguration.getAccessPolicyProvider()) {
+                        if (accessPolicyProviders.containsKey(accessPolicyProvider.getIdentifier())) {
+                            throw new Exception("Duplicate Access Policy Provider identifier in Authorizers configuration: " + accessPolicyProvider.getIdentifier());
+                        }
                         accessPolicyProviders.put(accessPolicyProvider.getIdentifier(), createAccessPolicyProvider(accessPolicyProvider.getIdentifier(), accessPolicyProvider.getClazz()));
                     }
 
@@ -145,6 +154,9 @@ public class AuthorizerFactoryBean implements FactoryBean, DisposableBean, UserG
 
                     // create each authorizer
                     for (final org.apache.nifi.authorization.generated.Authorizer authorizer : authorizerConfiguration.getAuthorizer()) {
+                        if (authorizers.containsKey(authorizer.getIdentifier())) {
+                            throw new Exception("Duplicate Authorizer identifier in Authorizers configuration: " + authorizer.getIdentifier());
+                        }
                         authorizers.put(authorizer.getIdentifier(), createAuthorizer(authorizer.getIdentifier(), authorizer.getClazz(),authorizer.getClasspath()));
                     }
 
@@ -184,7 +196,7 @@ public class AuthorizerFactoryBean implements FactoryBean, DisposableBean, UserG
                 unmarshaller.setSchema(schema);
                 final JAXBElement<Authorizers> element = unmarshaller.unmarshal(xsr, Authorizers.class);
                 return element.getValue();
-            } catch (SAXException | JAXBException e) {
+            } catch (XMLStreamException | SAXException | JAXBException e) {
                 throw new Exception("Unable to load the authorizer configuration file at: " + authorizersConfigurationFile.getAbsolutePath(), e);
             }
         } else {
@@ -478,38 +490,44 @@ public class AuthorizerFactoryBean implements FactoryBean, DisposableBean, UserG
 
     @Override
     public void destroy() throws Exception {
-        if (authorizer != null) {
-            Exception error = null;
+        List<Exception> errors = new ArrayList<>();
 
+        if (authorizers != null) {
+            authorizers.forEach((identifier, object) -> {
                 try {
-                authorizer.preDestruction();
-            } catch (final Exception e) {
-                error = e;
+                    object.preDestruction();
+                } catch (Exception e) {
+                    errors.add(e);
+                    logger.error("Error pre-destructing {}: {}", identifier, e);
+                }
+            });
         }
 
-            if (authorizer instanceof ManagedAuthorizer) {
-                final AccessPolicyProvider accessPolicyProvider = ((ManagedAuthorizer) authorizer).getAccessPolicyProvider();
-                if (accessPolicyProvider != null) {
+        if (accessPolicyProviders != null) {
+            accessPolicyProviders.forEach((identifier, object) -> {
                 try {
-                        accessPolicyProvider.preDestruction();
-                    } catch (final Exception e) {
-                        error = e;
+                    object.preDestruction();
+                } catch (Exception e) {
+                    errors.add(e);
+                    logger.error("Error pre-destructing {}: {}", identifier, e);
+                }
+            });
         }
 
-                    final UserGroupProvider userGroupProvider = accessPolicyProvider.getUserGroupProvider();
-                    if (userGroupProvider != null) {
+        if (userGroupProviders != null) {
+            userGroupProviders.forEach((identifier, object) -> {
                 try {
-                            userGroupProvider.preDestruction();
-                        } catch (final Exception e) {
-                            error = e;
-                        }
-                    }
+                    object.preDestruction();
+                } catch (Exception e) {
+                    errors.add(e);
+                    logger.error("Error pre-destructing {}: {}", identifier, e);
                 }
+            });
         }
 
-            if (error != null) {
-                throw error;
-            }
+        if (!errors.isEmpty()) {
+            List<String> errorMessages = errors.stream().map(Throwable::toString).collect(Collectors.toList());
+            throw new AuthorizerDestructionException("One or more providers encountered a pre-destruction error: " + StringUtils.join(errorMessages, "; "), errors.get(0));
         }
     }
 

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-authorizer/src/test/groovy/org/apache/nifi/authorization/AuthorizerFactoryBeanSpec.groovy

@@ -0,0 +1,72 @@
+/*
+ * 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.authorization
+
+import org.apache.nifi.authorization.resource.ResourceFactory
+import org.apache.nifi.util.NiFiProperties
+import spock.lang.Specification
+
+class AuthorizerFactoryBeanSpec extends Specification {
+
+    def mockProperties = Mock(NiFiProperties)
+
+    AuthorizerFactoryBean authorizerFactoryBean
+
+    // runs before every feature method
+    def setup() {
+        authorizerFactoryBean = new AuthorizerFactoryBean()
+        authorizerFactoryBean.setProperties(mockProperties)
+    }
+
+    // runs after every feature method
+    def cleanup() {}
+
+    // runs before the first feature method
+    def setupSpec() {}
+
+    // runs after the last feature method
+    def cleanupSpec() {}
+
+    def "create default authorizer"() {
+
+        setup: "properties indicate nifi-registry is unsecured"
+        mockProperties.getProperty(NiFiProperties.WEB_HTTPS_PORT) >> ""
+
+        when: "getAuthorizer() is first called"
+        def authorizer = (Authorizer) authorizerFactoryBean.getObject()
+
+        then: "the default authorizer is returned"
+        authorizer != null
+
+        and: "any authorization request made to that authorizer is approved"
+        def authorizationResult = authorizer.authorize(getTestAuthorizationRequest())
+        authorizationResult.result == AuthorizationResult.Result.Approved
+
+    }
+
+    // Helper methods
+
+    private static AuthorizationRequest getTestAuthorizationRequest() {
+        return new AuthorizationRequest.Builder()
+                .resource(ResourceFactory.getFlowResource())
+                .action(RequestAction.WRITE)
+                .accessAttempt(false)
+                .anonymous(true)
+                .build()
+    }
+
+}

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-authorization/src/main/java/org/apache/nifi/authorization/CompositeConfigurableUserGroupProvider.java

@@ -21,9 +21,12 @@ import org.apache.nifi.authorization.exception.AuthorizerCreationException;
 import org.apache.nifi.authorization.exception.AuthorizerDestructionException;
 import org.apache.nifi.authorization.exception.UninheritableAuthorizationsException;
 import org.apache.nifi.components.PropertyValue;
+import org.apache.nifi.util.StringUtils;
 
 import java.util.HashSet;
+import java.util.Map;
 import java.util.Set;
+import java.util.regex.Matcher;
 
 public class CompositeConfigurableUserGroupProvider extends CompositeUserGroupProvider implements ConfigurableUserGroupProvider {
 
@@ -61,6 +64,17 @@ public class CompositeConfigurableUserGroupProvider extends CompositeUserGroupPr
             throw new AuthorizerCreationException(String.format("The Configurable User Group Provider is not configurable: %s", configurableUserGroupProviderKey));
         }
 
+        // Ensure that the ConfigurableUserGroupProvider is not also listed as one of the providers for the CompositeUserGroupProvider
+        for (Map.Entry<String, String> entry : configurationContext.getProperties().entrySet()) {
+            Matcher matcher = USER_GROUP_PROVIDER_PATTERN.matcher(entry.getKey());
+            if (matcher.matches() && !StringUtils.isBlank(entry.getValue())) {
+                final String userGroupProviderKey = entry.getValue();
+                if (userGroupProviderKey.equals(configurableUserGroupProviderKey.getValue())) {
+                    throw new AuthorizerCreationException(String.format("Duplicate provider in Composite Configurable User Group Provider configuration: %s", userGroupProviderKey));
+                }
+            }
+        }
+
         configurableUserGroupProvider = (ConfigurableUserGroupProvider) userGroupProvider;
 
         // configure the CompositeUserGroupProvider

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-authorization/src/main/java/org/apache/nifi/authorization/CompositeUserGroupProvider.java

@@ -67,6 +67,10 @@ public class CompositeUserGroupProvider implements UserGroupProvider {
                     throw new AuthorizerCreationException(String.format("Unable to locate the configured User Group Provider: %s", userGroupProviderKey));
                 }
 
+                if (userGroupProviders.contains(userGroupProvider)) {
+                    throw new AuthorizerCreationException(String.format("Duplicate provider in Composite User Group Provider configuration: %s", userGroupProviderKey));
+                }
+
                 userGroupProviders.add(userGroupProvider);
             }
         }

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-authorization/src/test/java/org/apache/nifi/authorization/CompositeConfigurableUserGroupProviderTest.java

@@ -20,11 +20,14 @@ import org.apache.nifi.attribute.expression.language.StandardPropertyValue;
 import org.apache.nifi.authorization.exception.AuthorizerCreationException;
 import org.junit.Test;
 
+import java.util.HashMap;
+import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import static org.apache.nifi.authorization.CompositeConfigurableUserGroupProvider.PROP_CONFIGURABLE_USER_GROUP_PROVIDER;
+import static org.apache.nifi.authorization.CompositeUserGroupProvider.PROP_USER_GROUP_PROVIDER_PREFIX;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
@@ -64,6 +67,32 @@ public class CompositeConfigurableUserGroupProviderTest extends CompositeUserGro
         });
     }
 
+    @Test(expected = AuthorizerCreationException.class)
+    public void testDuplicateProviders() throws Exception {
+
+        // Mock UserGroupProviderLookup
+        UserGroupProvider configurableUserGroupProvider = getConfigurableUserGroupProvider();
+        final UserGroupProviderLookup ugpLookup = mock(UserGroupProviderLookup.class);
+        when(ugpLookup.getUserGroupProvider(eq(CONFIGURABLE_USER_GROUP_PROVIDER))).thenReturn(configurableUserGroupProvider);
+
+        // Mock AuthorizerInitializationContext
+        final AuthorizerInitializationContext initializationContext = mock(AuthorizerInitializationContext.class);
+        when(initializationContext.getUserGroupProviderLookup()).thenReturn(ugpLookup);
+
+        // Mock AuthorizerConfigurationContext to introduce the duplicate provider ids
+        final AuthorizerConfigurationContext configurationContext = mock(AuthorizerConfigurationContext.class);
+        when(configurationContext.getProperty(PROP_CONFIGURABLE_USER_GROUP_PROVIDER)).thenReturn(new StandardPropertyValue(CONFIGURABLE_USER_GROUP_PROVIDER, null));
+        Map<String, String> configurationContextProperties = new HashMap<>();
+        configurationContextProperties.put(PROP_USER_GROUP_PROVIDER_PREFIX + "1", CONFIGURABLE_USER_GROUP_PROVIDER);
+        configurationContextProperties.put(PROP_USER_GROUP_PROVIDER_PREFIX + "2", NOT_CONFIGURABLE_USER_GROUP_PROVIDER);
+        when(configurationContext.getProperties()).thenReturn(configurationContextProperties);
+
+        // configure (should throw exception)
+        CompositeConfigurableUserGroupProvider provider = new CompositeConfigurableUserGroupProvider();
+        provider.initialize(initializationContext);
+        provider.onConfigured(configurationContext);
+    }
+
     @Test
     public void testConfigurableUserGroupProviderOnly() throws Exception {
         final UserGroupProvider userGroupProvider = initCompositeUserGroupProvider(new CompositeConfigurableUserGroupProvider(), lookup -> {

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-authorization/src/test/java/org/apache/nifi/authorization/CompositeUserGroupProviderTest.java

@@ -62,6 +62,12 @@ public class CompositeUserGroupProviderTest extends CompositeUserGroupProviderTe
         compositeUserGroupProvider.onConfigured(configurationContext);
     }
 
+    @Test(expected = AuthorizerCreationException.class)
+    public void testDuplicateProviders() throws Exception {
+        UserGroupProvider duplicatedUserGroupProvider = getUserGroupProviderOne();
+        initCompositeUserGroupProvider(new CompositeUserGroupProvider(), null, null, duplicatedUserGroupProvider, duplicatedUserGroupProvider);
+    }
+
     @Test
     public void testOneProvider() throws Exception {
         final UserGroupProvider userGroupProvider = initCompositeUserGroupProvider(new CompositeUserGroupProvider(), null, null, getUserGroupProviderOne());