NIFI-2403

- Ensuring uniqueness on user identities and group names
- Ensure errors when updating a group are displayed.
- This closes #724

nifi-framework-api/src/main/java/org/apache/nifi/authorization/AbstractPolicyBasedAuthorizer.java

@@ -69,8 +69,21 @@ public abstract class AbstractPolicyBasedAuthorizer implements Authorizer {
         // ensure that only one policy per resource-action exists
         for (AccessPolicy accessPolicy : getAccessPolicies()) {
             if (policyExists(accessPolicy)) {
-                throw new AuthorizerCreationException("Found multiple policies for " + accessPolicy.getResource()
-                        + " with " + accessPolicy.getAction());
+                throw new AuthorizerCreationException(String.format("Found multiple policies for '%s' with '%s'.", accessPolicy.getResource(), accessPolicy.getAction()));
+            }
+        }
+
+        // ensure that only one group exists per identity
+        for (User user : getUsers()) {
+            if (tenantExists(user.getIdentifier(), user.getIdentity())) {
+                throw new AuthorizerCreationException(String.format("Found multiple users/user groups with identity '%s'.", user.getIdentity()));
+            }
+        }
+
+        // ensure that only one group exists per identity
+        for (Group group : getGroups()) {
+            if (tenantExists(group.getIdentifier(), group.getName())) {
+                throw new AuthorizerCreationException(String.format("Found multiple users/user groups with name '%s'.", group.getName()));
             }
         }
     }
@@ -100,6 +113,31 @@ public abstract class AbstractPolicyBasedAuthorizer implements Authorizer {
         return false;
     }
 
+    /**
+     * Checks if another user exists with the same identity.
+     *
+     * @param identifier identity of the user
+     * @param identity identity of the user
+     * @return true if another user exists with the same identity, false otherwise
+     */
+    private boolean tenantExists(final String identifier, final String identity) {
+        for (User user : getUsers()) {
+            if (!user.getIdentifier().equals(identifier)
+                    && user.getIdentity().equals(identity)) {
+                return true;
+            }
+        }
+
+        for (Group group : getGroups()) {
+            if (!group.getIdentifier().equals(identifier)
+                    && group.getName().equals(identity)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     @Override
     public final AuthorizationResult authorize(AuthorizationRequest request) throws AuthorizationAccessException {
         final UsersAndAccessPolicies usersAndAccessPolicies = getUsersAndAccessPolicies();
@@ -112,7 +150,7 @@ public abstract class AbstractPolicyBasedAuthorizer implements Authorizer {
 
         final User user = usersAndAccessPolicies.getUser(request.getIdentity());
         if (user == null) {
-            return AuthorizationResult.denied("Unknown user with identity " + request.getIdentity());
+            return AuthorizationResult.denied(String.format("Unknown user with identity '%s'.", request.getIdentity()));
         }
 
         final Set<Group> userGroups = usersAndAccessPolicies.getGroups(user.getIdentity());
@@ -151,8 +189,23 @@ public abstract class AbstractPolicyBasedAuthorizer implements Authorizer {
      * @param group the Group to add
      * @return the added Group
      * @throws AuthorizationAccessException if there was an unexpected error performing the operation
+     * @throws IllegalStateException if a group with the same name already exists
      */
-    public abstract Group addGroup(Group group) throws AuthorizationAccessException;
+    public final synchronized Group addGroup(Group group) throws AuthorizationAccessException {
+        if (tenantExists(group.getIdentifier(), group.getName())) {
+            throw new IllegalStateException(String.format("User/user group already exists with the identity '%s'.", group.getName()));
+        }
+        return doAddGroup(group);
+    }
+
+    /**
+     * Adds a new group.
+     *
+     * @param group the Group to add
+     * @return the added Group
+     * @throws AuthorizationAccessException if there was an unexpected error performing the operation
+     */
+    public abstract Group doAddGroup(Group group) throws AuthorizationAccessException;
 
     /**
      * Retrieves a Group by id.
@@ -169,8 +222,23 @@ public abstract class AbstractPolicyBasedAuthorizer implements Authorizer {
      * @param group an updated group instance
      * @return the updated group instance, or null if no matching group was found
      * @throws AuthorizationAccessException if there was an unexpected error performing the operation
+     * @throws IllegalStateException if there is already a group with the same name
      */
-    public abstract Group updateGroup(Group group) throws AuthorizationAccessException;
+    public final synchronized Group updateGroup(Group group) throws AuthorizationAccessException {
+        if (tenantExists(group.getIdentifier(), group.getName())) {
+            throw new IllegalStateException(String.format("User/user group already exists with the identity '%s'.", group.getName()));
+        }
+        return doUpdateGroup(group);
+    }
+
+    /**
+     * The group represented by the provided instance will be updated based on the provided instance.
+     *
+     * @param group an updated group instance
+     * @return the updated group instance, or null if no matching group was found
+     * @throws AuthorizationAccessException if there was an unexpected error performing the operation
+     */
+    public abstract Group doUpdateGroup(Group group) throws AuthorizationAccessException;
 
     /**
      * Deletes the given group.
@@ -196,8 +264,23 @@ public abstract class AbstractPolicyBasedAuthorizer implements Authorizer {
      * @param user the user to add
      * @return the user that was added
      * @throws AuthorizationAccessException if there was an unexpected error performing the operation
+     * @throws IllegalStateException if there is already a user with the same identity
      */
-    public abstract User addUser(User user) throws AuthorizationAccessException;
+    public final synchronized User addUser(User user) throws AuthorizationAccessException {
+        if (tenantExists(user.getIdentifier(), user.getIdentity())) {
+            throw new IllegalStateException(String.format("User/user group already exists with the identity '%s'.", user.getIdentity()));
+        }
+        return doAddUser(user);
+    }
+
+    /**
+     * Adds the given user.
+     *
+     * @param user the user to add
+     * @return the user that was added
+     * @throws AuthorizationAccessException if there was an unexpected error performing the operation
+     */
+    public abstract User doAddUser(User user) throws AuthorizationAccessException;
 
     /**
      * Retrieves the user with the given identifier.
@@ -223,8 +306,23 @@ public abstract class AbstractPolicyBasedAuthorizer implements Authorizer {
      * @param user an updated user instance
      * @return the updated user instance, or null if no matching user was found
      * @throws AuthorizationAccessException if there was an unexpected error performing the operation
+     * @throws IllegalStateException if there is already a user with the same identity
      */
-    public abstract User updateUser(User user) throws AuthorizationAccessException;
+    public final synchronized User updateUser(final User user) throws AuthorizationAccessException {
+        if (tenantExists(user.getIdentifier(), user.getIdentity())) {
+            throw new IllegalStateException(String.format("User/user group already exists with the identity '%s'.", user.getIdentity()));
+        }
+        return doUpdateUser(user);
+    }
+
+    /**
+     * The user represented by the provided instance will be updated based on the provided instance.
+     *
+     * @param user an updated user instance
+     * @return the updated user instance, or null if no matching user was found
+     * @throws AuthorizationAccessException if there was an unexpected error performing the operation
+     */
+    public abstract User doUpdateUser(User user) throws AuthorizationAccessException;
 
     /**
      * Deletes the given user.
@@ -252,8 +350,7 @@ public abstract class AbstractPolicyBasedAuthorizer implements Authorizer {
      */
     public final synchronized AccessPolicy addAccessPolicy(AccessPolicy accessPolicy) throws AuthorizationAccessException {
         if (policyExists(accessPolicy)) {
-            throw new IllegalStateException("Found multiple policies for " + accessPolicy.getResource()
-                    + " with " + accessPolicy.getAction());
+            throw new IllegalStateException(String.format("Found multiple policies for '%s' with '%s'.", accessPolicy.getResource(), accessPolicy.getAction()));
         }
         return doAddAccessPolicy(accessPolicy);
     }

nifi-framework-api/src/test/java/org/apache/nifi/authorization/MockPolicyBasedAuthorizer.java

@@ -0,0 +1,183 @@
+/*
+ * 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.exception.AuthorizationAccessException;
+import org.apache.nifi.authorization.exception.AuthorizerCreationException;
+import org.apache.nifi.authorization.exception.AuthorizerDestructionException;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+/**
+ * Mock implementation of AbstractPolicyBasedAuthorizer.
+ */
+public class MockPolicyBasedAuthorizer extends AbstractPolicyBasedAuthorizer {
+
+    private Set<Group> groups = new HashSet<>();
+    private Set<User> users = new HashSet<>();
+    private Set<AccessPolicy> policies = new HashSet<>();
+
+    public MockPolicyBasedAuthorizer() {
+
+    }
+
+    public MockPolicyBasedAuthorizer(Set<Group> groups, Set<User> users, Set<AccessPolicy> policies) {
+        if (groups != null) {
+            this.groups.addAll(groups);
+        }
+        if (users != null) {
+            this.users.addAll(users);
+        }
+        if (policies != null) {
+            this.policies.addAll(policies);
+        }
+    }
+
+    @Override
+    public Group doAddGroup(Group group) throws AuthorizationAccessException {
+        groups.add(group);
+        return group;
+    }
+
+    @Override
+    public Group getGroup(String identifier) throws AuthorizationAccessException {
+        return groups.stream().filter(g -> g.getIdentifier().equals(identifier)).findFirst().get();
+    }
+
+    @Override
+    public Group doUpdateGroup(Group group) throws AuthorizationAccessException {
+        deleteGroup(group);
+        return addGroup(group);
+    }
+
+    @Override
+    public Group deleteGroup(Group group) throws AuthorizationAccessException {
+        groups.remove(group);
+        return group;
+    }
+
+    @Override
+    public Set<Group> getGroups() throws AuthorizationAccessException {
+        return groups;
+    }
+
+    @Override
+    public User doAddUser(User user) throws AuthorizationAccessException {
+        users.add(user);
+        return user;
+    }
+
+    @Override
+    public User getUser(String identifier) throws AuthorizationAccessException {
+        return users.stream().filter(u -> u.getIdentifier().equals(identifier)).findFirst().get();
+    }
+
+    @Override
+    public User getUserByIdentity(String identity) throws AuthorizationAccessException {
+        return users.stream().filter(u -> u.getIdentity().equals(identity)).findFirst().get();
+    }
+
+    @Override
+    public User doUpdateUser(User user) throws AuthorizationAccessException {
+        deleteUser(user);
+        return addUser(user);
+    }
+
+    @Override
+    public User deleteUser(User user) throws AuthorizationAccessException {
+        users.remove(user);
+        return user;
+    }
+
+    @Override
+    public Set<User> getUsers() throws AuthorizationAccessException {
+        return users;
+    }
+
+    @Override
+    protected AccessPolicy doAddAccessPolicy(AccessPolicy accessPolicy) throws AuthorizationAccessException {
+        policies.add(accessPolicy);
+        return accessPolicy;
+    }
+
+    @Override
+    public AccessPolicy getAccessPolicy(String identifier) throws AuthorizationAccessException {
+        return policies.stream().filter(p -> p.getIdentifier().equals(identifier)).findFirst().get();
+    }
+
+    @Override
+    public AccessPolicy updateAccessPolicy(AccessPolicy accessPolicy) throws AuthorizationAccessException {
+        deleteAccessPolicy(accessPolicy);
+        return addAccessPolicy(accessPolicy);
+    }
+
+    @Override
+    public AccessPolicy deleteAccessPolicy(AccessPolicy policy) throws AuthorizationAccessException {
+        policies.remove(policy);
+        return policy;
+    }
+
+    @Override
+    public Set<AccessPolicy> getAccessPolicies() throws AuthorizationAccessException {
+        return policies;
+    }
+
+    @Override
+    public UsersAndAccessPolicies getUsersAndAccessPolicies() throws AuthorizationAccessException {
+        return new UsersAndAccessPolicies() {
+            @Override
+            public AccessPolicy getAccessPolicy(String resourceIdentifier, RequestAction action) {
+                return null;
+            }
+
+            @Override
+            public User getUser(String identity) {
+                return getUserByIdentity(identity);
+            }
+
+            @Override
+            public Set<Group> getGroups(String userIdentity) {
+                User user = getUserByIdentity(userIdentity);
+                if (user == null) {
+                    return new HashSet<>();
+                } else {
+                    return groups.stream()
+                            .filter(g -> g.getUsers().contains(user.getIdentifier()))
+                            .collect(Collectors.toSet());
+                }
+            }
+        };
+    }
+
+    @Override
+    public void initialize(AuthorizerInitializationContext initializationContext) throws AuthorizerCreationException {
+
+    }
+
+    @Override
+    public void doOnConfigured(AuthorizerConfigurationContext configurationContext) throws AuthorizerCreationException {
+
+    }
+
+    @Override
+    public void preDestruction() throws AuthorizerDestructionException {
+
+    }
+
+}

nifi-framework-api/src/test/java/org/apache/nifi/authorization/TestAbstractPolicyBasedAuthorizer.java

@@ -16,9 +16,8 @@
  */
 package org.apache.nifi.authorization;
 
-import org.apache.nifi.authorization.exception.AuthorizationAccessException;
+import org.apache.nifi.authorization.MockPolicyBasedAuthorizer;
 import org.apache.nifi.authorization.exception.AuthorizerCreationException;
-import org.apache.nifi.authorization.exception.AuthorizerDestructionException;
 import org.junit.Assert;
 import org.junit.Test;
 import org.mockito.Mockito;
@@ -251,7 +250,7 @@ public class TestAbstractPolicyBasedAuthorizer {
 
         assertEquals(authorizer1.getFingerprint(), authorizer2.getFingerprint());
 
-        System.out.println(authorizer1.getFingerprint());
+        //System.out.println(authorizer1.getFingerprint());
     }
 
     @Test
@@ -304,7 +303,7 @@ public class TestAbstractPolicyBasedAuthorizer {
 
         // make a second authorizer using the memory-backed implementation so we can inherit the fingerprint
         // and then compute a new fingerprint to compare them
-        AbstractPolicyBasedAuthorizer authorizer2 = new MemoryPolicyBasedAuthorizer();
+        AbstractPolicyBasedAuthorizer authorizer2 = new MockPolicyBasedAuthorizer();
         authorizer2.inheritFingerprint(fingerprint1);
 
         // computer the fingerprint of the second authorizer and it should be the same as the first
@@ -329,117 +328,209 @@ public class TestAbstractPolicyBasedAuthorizer {
         Assert.assertTrue(fingerprint.length() > 0);
     }
 
-    /**
-     * An AbstractPolicyBasedAuthorizer that stores everything in memory.
-     */
-    private static final class MemoryPolicyBasedAuthorizer extends AbstractPolicyBasedAuthorizer {
+    @Test(expected = AuthorizerCreationException.class)
+    public void testOnConfiguredWhenPoliciesWithSameResourceAndAction() {
+        User user1 = new User.Builder().identifier("user-id-1").identity("user-1").build();
 
-        private Set<Group> groups = new HashSet<>();
-        private Set<User> users = new HashSet<>();
-        private Set<AccessPolicy> policies = new HashSet<>();
+        AccessPolicy policy1 = new AccessPolicy.Builder()
+                .identifier("policy-id-1")
+                .resource("resource1")
+                .action(RequestAction.READ)
+                .addUser(user1.getIdentifier())
+                .build();
 
-        @Override
-        public Group addGroup(Group group) throws AuthorizationAccessException {
-            groups.add(group);
-            return group;
+        AccessPolicy policy2 = new AccessPolicy.Builder()
+                .identifier("policy-id-2")
+                .resource("resource1")
+                .action(RequestAction.READ)
+                .addUser(user1.getIdentifier())
+                .build();
+
+        Set<AccessPolicy> policies = new LinkedHashSet<>();
+        policies.add(policy1);
+        policies.add(policy2);
+
+        Set<User> users = new LinkedHashSet<>();
+        users.add(user1);
+
+        AuthorizerConfigurationContext context = Mockito.mock(AuthorizerConfigurationContext.class);
+        AbstractPolicyBasedAuthorizer authorizer = new MockPolicyBasedAuthorizer(new HashSet<>(), users, policies);
+        authorizer.onConfigured(context);
     }
 
-        @Override
-        public Group getGroup(String identifier) throws AuthorizationAccessException {
-            throw new UnsupportedOperationException();
+    @Test(expected = AuthorizerCreationException.class)
+    public void testOnConfiguredWhenUsersWithSameIdentity() {
+        User user1 = new User.Builder().identifier("user-id-1").identity("user-1").build();
+        User user2 = new User.Builder().identifier("user-id-2").identity("user-1").build();
+
+        Set<User> users = new LinkedHashSet<>();
+        users.add(user1);
+        users.add(user2);
+
+        AuthorizerConfigurationContext context = Mockito.mock(AuthorizerConfigurationContext.class);
+        AbstractPolicyBasedAuthorizer authorizer = new MockPolicyBasedAuthorizer(new HashSet<>(), users, new HashSet<>());
+        authorizer.onConfigured(context);
     }
 
-        @Override
-        public Group updateGroup(Group group) throws AuthorizationAccessException {
-            throw new UnsupportedOperationException();
+    @Test(expected = AuthorizerCreationException.class)
+    public void testOnConfiguredWhenGroupsWithSameName() {
+        Group group1 = new Group.Builder().identifier("group-id-1").name("group-1").build();
+        Group group2 = new Group.Builder().identifier("group-id-2").name("group-1").build();
+
+        Set<Group> groups = new LinkedHashSet<>();
+        groups.add(group1);
+        groups.add(group2);
+
+        AuthorizerConfigurationContext context = Mockito.mock(AuthorizerConfigurationContext.class);
+        AbstractPolicyBasedAuthorizer authorizer = new MockPolicyBasedAuthorizer(groups, new HashSet<>(), new HashSet<>());
+        authorizer.onConfigured(context);
     }
 
-        @Override
-        public Group deleteGroup(Group group) throws AuthorizationAccessException {
-            groups.remove(group);
-            return group;
-        }
+    @Test
+    public void testAddPoliciesWithSameResourceAndAction() {
+        AuthorizerConfigurationContext context = Mockito.mock(AuthorizerConfigurationContext.class);
+        AbstractPolicyBasedAuthorizer authorizer = new MockPolicyBasedAuthorizer();
+        authorizer.onConfigured(context);
 
-        @Override
-        public Set<Group> getGroups() throws AuthorizationAccessException {
-            return groups;
-        }
+        User user1 = new User.Builder().identifier("user-id-1").identity("user-1").build();
+        authorizer.addUser(user1);
 
-        @Override
-        public User addUser(User user) throws AuthorizationAccessException {
-            users.add(user);
-            return user;
-        }
+        AccessPolicy policy1 = new AccessPolicy.Builder()
+                .identifier("policy-id-1")
+                .resource("resource1")
+                .action(RequestAction.READ)
+                .addUser(user1.getIdentifier())
+                .build();
+        authorizer.addAccessPolicy(policy1);
 
-        @Override
-        public User getUser(String identifier) throws AuthorizationAccessException {
-            throw new UnsupportedOperationException();
-        }
+        AccessPolicy policy2 = new AccessPolicy.Builder()
+                .identifier("policy-id-2")
+                .resource("resource1")
+                .action(RequestAction.READ)
+                .addUser(user1.getIdentifier())
+                .build();
 
-        @Override
-        public User getUserByIdentity(String identity) throws AuthorizationAccessException {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public User updateUser(User user) throws AuthorizationAccessException {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public User deleteUser(User user) throws AuthorizationAccessException {
-            users.remove(user);
-            return user;
-        }
-
-        @Override
-        public Set<User> getUsers() throws AuthorizationAccessException {
-            return users;
-        }
-
-        @Override
-        protected AccessPolicy doAddAccessPolicy(AccessPolicy accessPolicy) throws AuthorizationAccessException {
-            policies.add(accessPolicy);
-            return accessPolicy;
-        }
-
-        @Override
-        public AccessPolicy getAccessPolicy(String identifier) throws AuthorizationAccessException {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public AccessPolicy updateAccessPolicy(AccessPolicy accessPolicy) throws AuthorizationAccessException {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public AccessPolicy deleteAccessPolicy(AccessPolicy policy) throws AuthorizationAccessException {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public Set<AccessPolicy> getAccessPolicies() throws AuthorizationAccessException {
-            return policies;
-        }
-
-        @Override
-        public UsersAndAccessPolicies getUsersAndAccessPolicies() throws AuthorizationAccessException {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public void initialize(AuthorizerInitializationContext initializationContext) throws AuthorizerCreationException {
+        try {
+            authorizer.addAccessPolicy(policy2);
+            Assert.fail("Should have thrown exception");
+        } catch (IllegalStateException e) {
 
         }
-
-        @Override
-        public void doOnConfigured(AuthorizerConfigurationContext configurationContext) throws AuthorizerCreationException {
-
     }
 
-        @Override
-        public void preDestruction() throws AuthorizerDestructionException {
+    @Test
+    public void testAddUsersWithSameIdentity() {
+        AuthorizerConfigurationContext context = Mockito.mock(AuthorizerConfigurationContext.class);
+        AbstractPolicyBasedAuthorizer authorizer = new MockPolicyBasedAuthorizer();
+        authorizer.onConfigured(context);
+
+        User user1 = new User.Builder().identifier("user-id-1").identity("user-1").build();
+        authorizer.addUser(user1);
+
+        User user2 = new User.Builder().identifier("user-id-2").identity("user-1").build();
+
+        try {
+            authorizer.addUser(user2);
+            Assert.fail("Should have thrown exception");
+        } catch (IllegalStateException e) {
+
+        }
+    }
+
+    @Test
+    public void testAddGroupsWithSameName() {
+        AuthorizerConfigurationContext context = Mockito.mock(AuthorizerConfigurationContext.class);
+        AbstractPolicyBasedAuthorizer authorizer = new MockPolicyBasedAuthorizer();
+        authorizer.onConfigured(context);
+
+        Group group1 = new Group.Builder().identifier("group-id-1").name("group-1").build();
+        authorizer.addGroup(group1);
+
+        Group group2 = new Group.Builder().identifier("group-id-2").name("group-1").build();
+
+        try {
+            authorizer.addGroup(group2);
+            Assert.fail("Should have thrown exception");
+        } catch (IllegalStateException e) {
+
+        }
+    }
+
+    @Test
+    public void testAddUsersWithSameIdentityAsGroupName() {
+        AuthorizerConfigurationContext context = Mockito.mock(AuthorizerConfigurationContext.class);
+        AbstractPolicyBasedAuthorizer authorizer = new MockPolicyBasedAuthorizer();
+        authorizer.onConfigured(context);
+
+        Group group1 = new Group.Builder().identifier("group-id-1").name("abc").build();
+        authorizer.addGroup(group1);
+
+        User user = new User.Builder().identifier("user-id-2").identity("abc").build();
+
+        try {
+            authorizer.addUser(user);
+            Assert.fail("Should have thrown exception");
+        } catch (IllegalStateException e) {
+
+        }
+    }
+
+    @Test
+    public void testAddGroupWithSameNameAsUserIdentity() {
+        AuthorizerConfigurationContext context = Mockito.mock(AuthorizerConfigurationContext.class);
+        AbstractPolicyBasedAuthorizer authorizer = new MockPolicyBasedAuthorizer();
+        authorizer.onConfigured(context);
+
+        User user = new User.Builder().identifier("user-id-2").identity("abc").build();
+        authorizer.addUser(user);
+
+        Group group1 = new Group.Builder().identifier("group-id-1").name("abc").build();
+        try {
+            authorizer.addGroup(group1);
+            Assert.fail("Should have thrown exception");
+        } catch (IllegalStateException e) {
+
+        }
+    }
+
+    @Test
+    public void testUpdateUserWithSameIdentity() {
+        AuthorizerConfigurationContext context = Mockito.mock(AuthorizerConfigurationContext.class);
+        AbstractPolicyBasedAuthorizer authorizer = new MockPolicyBasedAuthorizer();
+        authorizer.onConfigured(context);
+
+        User user1 = new User.Builder().identifier("user-id-1").identity("abc").build();
+        authorizer.addUser(user1);
+
+        User user2 = new User.Builder().identifier("user-id-2").identity("xyz").build();
+        authorizer.addUser(user2);
+
+        try {
+            User user1Updated = new User.Builder().identifier("user-id-1").identity("xyz").build();
+            authorizer.updateUser(user1Updated);
+            Assert.fail("Should have thrown exception");
+        } catch (IllegalStateException e) {
+
+        }
+    }
+
+    @Test
+    public void testUpdateGroupWithSameName() {
+        AuthorizerConfigurationContext context = Mockito.mock(AuthorizerConfigurationContext.class);
+        AbstractPolicyBasedAuthorizer authorizer = new MockPolicyBasedAuthorizer();
+        authorizer.onConfigured(context);
+
+        Group group1 = new Group.Builder().identifier("group-id-1").name("abc").build();
+        authorizer.addGroup(group1);
+
+        Group group2 = new Group.Builder().identifier("group-id-2").name("xyz").build();
+        authorizer.addGroup(group2);
+
+        try {
+            Group group1Updated = new Group.Builder().identifier("group-id-1").name("xyz").build();
+            authorizer.updateGroup(group1Updated);
+            Assert.fail("Should have thrown exception");
+        } catch (IllegalStateException e) {
 
         }
     }

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

@@ -292,7 +292,7 @@ public class AuthorizerFactoryBean implements FactoryBean, DisposableBean, Autho
             AbstractPolicyBasedAuthorizer policyBasedAuthorizer = (AbstractPolicyBasedAuthorizer) baseAuthorizer;
             return new AbstractPolicyBasedAuthorizer() {
                 @Override
-                public Group addGroup(Group group) throws AuthorizationAccessException {
+                public Group doAddGroup(Group group) throws AuthorizationAccessException {
                     try (final NarCloseable narCloseable = NarCloseable.withNarLoader()) {
                         return policyBasedAuthorizer.addGroup(group);
                     }
@@ -306,7 +306,7 @@ public class AuthorizerFactoryBean implements FactoryBean, DisposableBean, Autho
                 }
 
                 @Override
-                public Group updateGroup(Group group) throws AuthorizationAccessException {
+                public Group doUpdateGroup(Group group) throws AuthorizationAccessException {
                     try (final NarCloseable narCloseable = NarCloseable.withNarLoader()) {
                         return policyBasedAuthorizer.updateGroup(group);
                     }
@@ -327,7 +327,7 @@ public class AuthorizerFactoryBean implements FactoryBean, DisposableBean, Autho
                 }
 
                 @Override
-                public User addUser(User user) throws AuthorizationAccessException {
+                public User doAddUser(User user) throws AuthorizationAccessException {
                     try (final NarCloseable narCloseable = NarCloseable.withNarLoader()) {
                         return policyBasedAuthorizer.addUser(user);
                     }
@@ -348,7 +348,7 @@ public class AuthorizerFactoryBean implements FactoryBean, DisposableBean, Autho
                 }
 
                 @Override
-                public User updateUser(User user) throws AuthorizationAccessException {
+                public User doUpdateUser(User user) throws AuthorizationAccessException {
                     try (final NarCloseable narCloseable = NarCloseable.withNarLoader()) {
                         return policyBasedAuthorizer.updateUser(user);
                     }

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

@@ -688,7 +688,7 @@ public class FileAuthorizer extends AbstractPolicyBasedAuthorizer {
     // ------------------ Groups ------------------
 
     @Override
-    public synchronized Group addGroup(Group group) throws AuthorizationAccessException {
+    public synchronized Group doAddGroup(Group group) throws AuthorizationAccessException {
         if (group == null) {
             throw new IllegalArgumentException("Group cannot be null");
         }
@@ -726,7 +726,7 @@ public class FileAuthorizer extends AbstractPolicyBasedAuthorizer {
     }
 
     @Override
-    public synchronized Group updateGroup(Group group) throws AuthorizationAccessException {
+    public synchronized Group doUpdateGroup(Group group) throws AuthorizationAccessException {
         if (group == null) {
             throw new IllegalArgumentException("Group cannot be null");
         }
@@ -826,7 +826,7 @@ public class FileAuthorizer extends AbstractPolicyBasedAuthorizer {
     // ------------------ Users ------------------
 
     @Override
-    public synchronized User addUser(final User user) throws AuthorizationAccessException {
+    public synchronized User doAddUser(final User user) throws AuthorizationAccessException {
         if (user == null) {
             throw new IllegalArgumentException("User cannot be null");
         }
@@ -870,7 +870,7 @@ public class FileAuthorizer extends AbstractPolicyBasedAuthorizer {
     }
 
     @Override
-    public synchronized User updateUser(final User user) throws AuthorizationAccessException {
+    public synchronized User doUpdateUser(final User user) throws AuthorizationAccessException {
         if (user == null) {
             throw new IllegalArgumentException("User cannot be null");
         }

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

@@ -163,7 +163,6 @@
             <artifactId>nifi-mock</artifactId>
             <scope>test</scope>
         </dependency>
-
         <dependency>
             <groupId>org.spockframework</groupId>
             <artifactId>spock-core</artifactId>

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

@@ -0,0 +1,183 @@
+/*
+ * 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.exception.AuthorizationAccessException;
+import org.apache.nifi.authorization.exception.AuthorizerCreationException;
+import org.apache.nifi.authorization.exception.AuthorizerDestructionException;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+/**
+ * Mock implementation of AbstractPolicyBasedAuthorizer.
+ */
+public class MockPolicyBasedAuthorizer extends AbstractPolicyBasedAuthorizer {
+
+    private Set<Group> groups = new HashSet<>();
+    private Set<User> users = new HashSet<>();
+    private Set<AccessPolicy> policies = new HashSet<>();
+
+    public MockPolicyBasedAuthorizer() {
+
+    }
+
+    public MockPolicyBasedAuthorizer(Set<Group> groups, Set<User> users, Set<AccessPolicy> policies) {
+        if (groups != null) {
+            this.groups.addAll(groups);
+        }
+        if (users != null) {
+            this.users.addAll(users);
+        }
+        if (policies != null) {
+            this.policies.addAll(policies);
+        }
+    }
+
+    @Override
+    public Group doAddGroup(Group group) throws AuthorizationAccessException {
+        groups.add(group);
+        return group;
+    }
+
+    @Override
+    public Group getGroup(String identifier) throws AuthorizationAccessException {
+        return groups.stream().filter(g -> g.getIdentifier().equals(identifier)).findFirst().get();
+    }
+
+    @Override
+    public Group doUpdateGroup(Group group) throws AuthorizationAccessException {
+        deleteGroup(group);
+        return addGroup(group);
+    }
+
+    @Override
+    public Group deleteGroup(Group group) throws AuthorizationAccessException {
+        groups.remove(group);
+        return group;
+    }
+
+    @Override
+    public Set<Group> getGroups() throws AuthorizationAccessException {
+        return groups;
+    }
+
+    @Override
+    public User doAddUser(User user) throws AuthorizationAccessException {
+        users.add(user);
+        return user;
+    }
+
+    @Override
+    public User getUser(String identifier) throws AuthorizationAccessException {
+        return users.stream().filter(u -> u.getIdentifier().equals(identifier)).findFirst().get();
+    }
+
+    @Override
+    public User getUserByIdentity(String identity) throws AuthorizationAccessException {
+        return users.stream().filter(u -> u.getIdentity().equals(identity)).findFirst().get();
+    }
+
+    @Override
+    public User doUpdateUser(User user) throws AuthorizationAccessException {
+        deleteUser(user);
+        return addUser(user);
+    }
+
+    @Override
+    public User deleteUser(User user) throws AuthorizationAccessException {
+        users.remove(user);
+        return user;
+    }
+
+    @Override
+    public Set<User> getUsers() throws AuthorizationAccessException {
+        return users;
+    }
+
+    @Override
+    protected AccessPolicy doAddAccessPolicy(AccessPolicy accessPolicy) throws AuthorizationAccessException {
+        policies.add(accessPolicy);
+        return accessPolicy;
+    }
+
+    @Override
+    public AccessPolicy getAccessPolicy(String identifier) throws AuthorizationAccessException {
+        return policies.stream().filter(p -> p.getIdentifier().equals(identifier)).findFirst().get();
+    }
+
+    @Override
+    public AccessPolicy updateAccessPolicy(AccessPolicy accessPolicy) throws AuthorizationAccessException {
+        deleteAccessPolicy(accessPolicy);
+        return addAccessPolicy(accessPolicy);
+    }
+
+    @Override
+    public AccessPolicy deleteAccessPolicy(AccessPolicy policy) throws AuthorizationAccessException {
+        policies.remove(policy);
+        return policy;
+    }
+
+    @Override
+    public Set<AccessPolicy> getAccessPolicies() throws AuthorizationAccessException {
+        return policies;
+    }
+
+    @Override
+    public UsersAndAccessPolicies getUsersAndAccessPolicies() throws AuthorizationAccessException {
+        return new UsersAndAccessPolicies() {
+            @Override
+            public AccessPolicy getAccessPolicy(String resourceIdentifier, RequestAction action) {
+                return null;
+            }
+
+            @Override
+            public User getUser(String identity) {
+                return getUserByIdentity(identity);
+            }
+
+            @Override
+            public Set<Group> getGroups(String userIdentity) {
+                User user = getUserByIdentity(userIdentity);
+                if (user == null) {
+                    return new HashSet<>();
+                } else {
+                    return groups.stream()
+                            .filter(g -> g.getUsers().contains(user.getIdentifier()))
+                            .collect(Collectors.toSet());
+                }
+            }
+        };
+    }
+
+    @Override
+    public void initialize(AuthorizerInitializationContext initializationContext) throws AuthorizerCreationException {
+
+    }
+
+    @Override
+    public void doOnConfigured(AuthorizerConfigurationContext configurationContext) throws AuthorizerCreationException {
+
+    }
+
+    @Override
+    public void preDestruction() throws AuthorizerDestructionException {
+
+    }
+
+}

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/TestFlowController.java

@@ -21,6 +21,7 @@ import org.apache.nifi.admin.service.AuditService;
 import org.apache.nifi.authorization.AbstractPolicyBasedAuthorizer;
 import org.apache.nifi.authorization.AccessPolicy;
 import org.apache.nifi.authorization.Group;
+import org.apache.nifi.authorization.MockPolicyBasedAuthorizer;
 import org.apache.nifi.authorization.RequestAction;
 import org.apache.nifi.authorization.User;
 import org.apache.nifi.cluster.protocol.DataFlow;
@@ -40,17 +41,14 @@ import org.junit.Test;
 import org.mockito.Mockito;
 
 import java.nio.charset.StandardCharsets;
-import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.Set;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.any;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 public class TestFlowController {
@@ -58,15 +56,20 @@ public class TestFlowController {
     private FlowController controller;
     private AbstractPolicyBasedAuthorizer authorizer;
     private StandardFlowSynchronizer standardFlowSynchronizer;
+    private FlowFileEventRepository flowFileEventRepo;
+    private AuditService auditService;
+    private StringEncryptor encryptor;
+    private NiFiProperties properties;
+    private BulletinRepository bulletinRepo;
 
     @Before
     public void setup() {
         System.setProperty(NiFiProperties.PROPERTIES_FILE_PATH, "src/test/resources/nifi.properties");
 
-        final FlowFileEventRepository flowFileEventRepo = Mockito.mock(FlowFileEventRepository.class);
-        final AuditService auditService = Mockito.mock(AuditService.class);
-        final StringEncryptor encryptor = StringEncryptor.createEncryptor();
-        final NiFiProperties properties = NiFiProperties.getInstance();
+        flowFileEventRepo = Mockito.mock(FlowFileEventRepository.class);
+        auditService = Mockito.mock(AuditService.class);
+        encryptor = StringEncryptor.createEncryptor();
+        properties = NiFiProperties.getInstance();
         properties.setProperty(NiFiProperties.PROVENANCE_REPO_IMPLEMENTATION_CLASS, MockProvenanceRepository.class.getName());
         properties.setProperty("nifi.remote.input.socket.port", "");
         properties.setProperty("nifi.remote.input.secure", "");
@@ -107,12 +110,9 @@ public class TestFlowController {
         policies1.add(policy1);
         policies1.add(policy2);
 
-        authorizer = Mockito.mock(AbstractPolicyBasedAuthorizer.class);
-        when(authorizer.getGroups()).thenReturn(groups1);
-        when(authorizer.getUsers()).thenReturn(users1);
-        when(authorizer.getAccessPolicies()).thenReturn(policies1);
+        authorizer = new MockPolicyBasedAuthorizer(groups1, users1, policies1);
 
-        final BulletinRepository bulletinRepo = Mockito.mock(BulletinRepository.class);
+        bulletinRepo = Mockito.mock(BulletinRepository.class);
         controller = FlowController.createStandaloneInstance(flowFileEventRepo, properties, authorizer, auditService, encryptor, bulletinRepo);
 
         standardFlowSynchronizer = new StandardFlowSynchronizer(StringEncryptor.createEncryptor());
@@ -132,10 +132,7 @@ public class TestFlowController {
 
         controller.synchronize(standardFlowSynchronizer, proposedDataFlow);
 
-        // had a problem verifying the call to inheritFingerprint didn't happen, so just verify none of the add methods got called
-        verify(authorizer, times(0)).addUser(any(User.class));
-        verify(authorizer, times(0)).addGroup(any(Group.class));
-        //verify(authorizer, times(0)).addAccessPolicy(any(AccessPolicy.class));
+        assertEquals(authFingerprint, authorizer.getFingerprint());
     }
 
     @Test(expected = UninheritableFlowException.class)
@@ -146,6 +143,7 @@ public class TestFlowController {
         when(proposedDataFlow.getAuthorizerFingerprint()).thenReturn(authFingerprint.getBytes(StandardCharsets.UTF_8));
 
         controller.synchronize(standardFlowSynchronizer, proposedDataFlow);
+        assertNotEquals(authFingerprint, authorizer.getFingerprint());
     }
 
     @Test(expected = UninheritableFlowException.class)
@@ -163,13 +161,13 @@ public class TestFlowController {
         final DataFlow proposedDataFlow = Mockito.mock(DataFlow.class);
         when(proposedDataFlow.getAuthorizerFingerprint()).thenReturn(authFingerprint.getBytes(StandardCharsets.UTF_8));
 
-        // reset the authorizer so it returns empty fingerprint
-        when(authorizer.getUsers()).thenReturn(new HashSet<User>());
-        when(authorizer.getGroups()).thenReturn(new HashSet<Group>());
-        when(authorizer.getAccessPolicies()).thenReturn(new HashSet<AccessPolicy>());
+        authorizer = new MockPolicyBasedAuthorizer();
+        assertNotEquals(authFingerprint, authorizer.getFingerprint());
 
+        controller.shutdown(true);
+        controller = FlowController.createStandaloneInstance(flowFileEventRepo, properties, authorizer, auditService, encryptor, bulletinRepo);
         controller.synchronize(standardFlowSynchronizer, proposedDataFlow);
-        verify(authorizer, times(1)).inheritFingerprint(authFingerprint);
+        assertEquals(authFingerprint, authorizer.getFingerprint());
     }
 
     @Test

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/dao/impl/StandardPolicyBasedAuthorizerDAO.java

@@ -53,7 +53,7 @@ public class StandardPolicyBasedAuthorizerDAO implements AccessPolicyDAO, UserGr
         } else {
             this.authorizer = new AbstractPolicyBasedAuthorizer() {
                 @Override
-                public Group addGroup(final Group group) throws AuthorizationAccessException {
+                public Group doAddGroup(final Group group) throws AuthorizationAccessException {
                     throw new IllegalStateException(MSG_NON_ABSTRACT_POLICY_BASED_AUTHORIZER);
                 }
 
@@ -63,7 +63,7 @@ public class StandardPolicyBasedAuthorizerDAO implements AccessPolicyDAO, UserGr
                 }
 
                 @Override
-                public Group updateGroup(final Group group) throws AuthorizationAccessException {
+                public Group doUpdateGroup(final Group group) throws AuthorizationAccessException {
                     throw new IllegalStateException(MSG_NON_ABSTRACT_POLICY_BASED_AUTHORIZER);
                 }
 
@@ -78,7 +78,7 @@ public class StandardPolicyBasedAuthorizerDAO implements AccessPolicyDAO, UserGr
                 }
 
                 @Override
-                public User addUser(final User user) throws AuthorizationAccessException {
+                public User doAddUser(final User user) throws AuthorizationAccessException {
                     throw new IllegalStateException(MSG_NON_ABSTRACT_POLICY_BASED_AUTHORIZER);
                 }
 
@@ -93,7 +93,7 @@ public class StandardPolicyBasedAuthorizerDAO implements AccessPolicyDAO, UserGr
                 }
 
                 @Override
-                public User updateUser(final User user) throws AuthorizationAccessException {
+                public User doUpdateUser(final User user) throws AuthorizationAccessException {
                     throw new IllegalStateException(MSG_NON_ABSTRACT_POLICY_BASED_AUTHORIZER);
                 }
 

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/groovy/org/apache/nifi/web/dao/impl/StandardPolicyBasedAuthorizerDAOSpec.groovy

@@ -250,13 +250,16 @@ class StandardPolicyBasedAuthorizerDAOSpec extends Specification {
         noExceptionThrown()
 
         then:
-        1 * authorizer.addGroup(userGroup) >> userGroup
+        1 * authorizer.getUsers() >> users
+        1 * authorizer.getGroups() >> groups
+        1 * authorizer.doAddGroup(userGroup) >> userGroup
         0 * _
         result?.equals userGroup
 
         where:
-        userGroup                                                                                              | _
-        new Group.Builder().identifier('user-group-id-1').name('user-group-id-1').addUser('user-id-1').build() | _
+        userGroup                                                       | users         | groups
+        new Group.Builder().identifier('user-group-id-1')
+                .name('user-group-id-1').addUser('user-id-1').build()   | [] as Set     | [] as Set
     }
 
     @Unroll
@@ -324,13 +327,16 @@ class StandardPolicyBasedAuthorizerDAOSpec extends Specification {
 
         then:
         1 * authorizer.getGroup(requestDTO.id) >> userGroup
-        1 * authorizer.updateGroup(userGroup) >> userGroup
+        1 * authorizer.getUsers() >> users
+        1 * authorizer.getGroups() >> groups
+        1 * authorizer.doUpdateGroup(userGroup) >> userGroup
         0 * _
         result?.equals(userGroup)
 
         where:
-        userGroup                                                                                              | _
-        new Group.Builder().identifier('user-group-id-1').name('user-group-id-1').addUser('user-id-1').build() | _
+        userGroup                                                     | users       | groups
+        new Group.Builder().identifier('user-group-id-1')
+                .name('user-group-id-1').addUser('user-id-1').build() | [] as Set   | [] as Set
     }
 
     @Unroll
@@ -417,13 +423,16 @@ class StandardPolicyBasedAuthorizerDAOSpec extends Specification {
         noExceptionThrown()
 
         then:
-        1 * authorizer.addUser(user) >> user
+        1 * authorizer.getUsers() >> users
+        1 * authorizer.getGroups() >> groups
+        1 * authorizer.doAddUser(user) >> user
         0 * _
         result?.equals user
 
         where:
-        user                                                                                                     | _
-        new User.Builder().identifier('user-id-1').identity('user identity').build() | _
+        user                                        | users         | groups
+        new User.Builder().identifier('user-id-1')
+                .identity('user identity').build()  | [] as Set     | [] as Set
     }
 
     @Unroll
@@ -491,13 +500,16 @@ class StandardPolicyBasedAuthorizerDAOSpec extends Specification {
 
         then:
         1 * authorizer.getUser(requestDTO.id) >> user
-        1 * authorizer.updateUser(user) >> user
+        1 * authorizer.getUsers() >> users
+        1 * authorizer.getGroups() >> groups
+        1 * authorizer.doUpdateUser(user) >> user
         0 * _
         result?.equals(user)
 
         where:
-        user                                                                                                     | _
-        new User.Builder().identifier('user-id-1').identity('user identity').build() | _
+        user                                        | users         | groups
+        new User.Builder().identifier('user-id-1')
+                .identity('user identity').build()  | [] as Set     | [] as Set
     }
 
     @Unroll

nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/users/nf-users-table.js

@@ -365,7 +365,7 @@ nf.UsersTable = (function () {
             contentType: 'application/json'
         }).done(function (groupEntity) {
             nf.UsersTable.loadUsersTable();
-        });
+        }).fail(nf.Common.handleAjaxError);
     };
 
     /**