Handle ConcurrentModificationException

- Cleanup exception usage
- Retry on ConcurrentModificationException
- Add RoleManager interface (some implementation still TODO)

community/security/src/main/java/org/neo4j/server/security/auth/FileUserRepository.java

@@ -44,6 +44,8 @@ public class FileUserRepository extends LifecycleAdapter implements UserReposito
 {
     private final Path authFile;
 
+    // TODO: We could improve concurrency by using a ReadWriteLock
+
     /** Quick lookup of users by name */
     private final Map<String, User> usersByName = new ConcurrentHashMap<>();
     private final Log log;

enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java

@@ -30,11 +30,9 @@
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentSkipListSet;
 
-import org.neo4j.kernel.api.security.exception.IllegalCredentialsException;
 import org.neo4j.kernel.lifecycle.LifecycleAdapter;
 import org.neo4j.logging.Log;
 import org.neo4j.logging.LogProvider;
-//import org.neo4j.server.security.auth.UserRepository;
 import org.neo4j.server.security.auth.exception.ConcurrentModificationException;
 
 import static java.nio.file.StandardCopyOption.ATOMIC_MOVE;
@@ -49,6 +47,8 @@ public class FileRoleRepository extends LifecycleAdapter implements RoleReposito
 {
     private final Path roleFile;
 
+    // TODO: We could improve concurrency by using a ReadWriteLock
+
     /** Quick lookup of roles by name */
     private final Map<String,RoleRecord> rolesByName = new ConcurrentHashMap<>();
     private final Map<String, SortedSet<String>> rolesByUsername = new ConcurrentHashMap<>();
@@ -88,11 +88,11 @@ public void start() throws Throwable
     }
 
     @Override
-    public void create( RoleRecord role ) throws IllegalCredentialsException, IOException
+    public void create( RoleRecord role ) throws IllegalArgumentException, IOException
     {
         if ( !isValidName( role.name() ) )
         {
-            throw new IllegalCredentialsException( "'" + role.name() + "' is not a valid role name." );
+            throw new IllegalArgumentException( "'" + role.name() + "' is not a valid role name." );
         }
 
         synchronized (this)
@@ -102,7 +102,7 @@ public void create( RoleRecord role ) throws IllegalCredentialsException, IOExce
             {
                 if ( other.name().equals( role.name() ) )
                 {
-                    throw new IllegalCredentialsException( "The specified role already exists" );
+                    throw new IllegalArgumentException( "The specified role already exists" );
                 }
             }
 
@@ -122,7 +122,7 @@ public void update( RoleRecord existingRole, RoleRecord updatedRole ) throws Con
         // Assert input is ok
         if ( !existingRole.name().equals( updatedRole.name() ) )
         {
-            throw new IllegalArgumentException( "updatedRole has a different name" );
+            throw new IllegalArgumentException( "updated role has a different name" );
         }
 
         synchronized (this)

enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileUserRealm.java

@@ -139,10 +139,10 @@ int numberOfUsers()
         return userRepository.numberOfUsers();
     }
 
-    User newUser( String username, String initialPassword, boolean requirePasswordChange ) throws
-            IOException, IllegalCredentialsException, ConcurrentModificationException
+    User newUser( String username, String initialPassword, boolean requirePasswordChange )
+            throws IOException, IllegalCredentialsException
     {
-        assertValidName( username );
+        assertValidUsername( username );
 
         User user = new User.Builder()
                 .withName( username )
@@ -154,40 +154,78 @@ User newUser( String username, String initialPassword, boolean requirePasswordCh
         return user;
     }
 
-    RoleRecord newRole( String roleName, String... users ) throws
-            IOException, IllegalCredentialsException, ConcurrentModificationException
+    RoleRecord newRole( String roleName, String... users ) throws IOException, IllegalCredentialsException
     {
-        assertValidName( roleName );
+        assertValidRoleName( roleName );
+        for (String username : users)
+        {
+            assertValidUsername( username );
+        }
 
         SortedSet<String> userSet = new TreeSet<String>( Arrays.asList( users ) );
+
         RoleRecord role = new RoleRecord.Builder().withName( roleName ).withUsers( userSet ).build();
         roleRepository.create( role );
 
         return role;
     }
 
-    private void addUserToRole( User user, String roleName )
-            throws IOException, IllegalCredentialsException, ConcurrentModificationException
+    void addUserToRole( String username, String roleName ) throws IOException
     {
+        assertValidUsername( username );
+        assertValidRoleName( roleName );
+
+        // TODO: FIXME This is not atomic. E.g. the user could be deleted between here and updating the role
+        User user = userRepository.findByName( username );
+        if ( user == null )
+        {
+            throw new IllegalArgumentException( "User " + username + " does not exist." );
+        }
+
+        // TODO: FIXME This is not atomic. E.g. the role could be created between here and create
+        // In that case we would get an IllegalArgumentException, but what we want is a
+        // ConcurrentModificationException and then retry. We should use a RoleRepositoty.createOrUpdate instead.
         RoleRecord role = roleRepository.findByName( roleName );
         if ( role == null )
         {
-            RoleRecord newRole = new RoleRecord( roleName, user.name() );
+            RoleRecord newRole = new RoleRecord( roleName, username );
             roleRepository.create( newRole );
         }
         else
         {
-            RoleRecord newRole = role.augment().withUser( user.name() ).build();
-            roleRepository.update( role, newRole );
+            RoleRecord newRole = role.augment().withUser( username ).build();
+            try
+            {
+                roleRepository.update( role, newRole );
+            }
+            catch ( ConcurrentModificationException e )
+            {
+                // Try again
+                addUserToRole( username, roleName );
+            }
         }
     }
 
-    private void assertValidName( String name )
+    void removeUserFromRole( String username, String rolename ) throws IOException
+    {
+        // TODO
+    }
+
+    private void assertValidUsername( String name )
     {
         if ( !userRepository.isValidName( name ) )
         {
             throw new IllegalArgumentException(
                     "User name contains illegal characters. Please use simple ascii characters and numbers." );
         }
     }
+
+    private void assertValidRoleName( String name )
+    {
+        if ( !roleRepository.isValidName( name ) )
+        {
+            throw new IllegalArgumentException(
+                    "Role name contains illegal characters. Please use simple ascii characters and numbers." );
+        }
+    }
 }

enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleManager.java

@@ -0,0 +1,29 @@
+/*
+ * Copyright (c) 2002-2016 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Neo4j is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.server.security.enterprise.auth;
+
+import java.io.IOException;
+
+public interface RoleManager
+{
+    void addUserToRole( String username, String roleName ) throws IOException;
+
+    void removeUserFromRole( String username, String roleName ) throws IOException;
+}

enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRepository.java

@@ -22,7 +22,6 @@
 import java.io.IOException;
 import java.util.Set;
 
-import org.neo4j.kernel.api.security.exception.IllegalCredentialsException;
 import org.neo4j.kernel.lifecycle.Lifecycle;
 import org.neo4j.server.security.auth.exception.ConcurrentModificationException;
 
@@ -38,9 +37,9 @@ public interface RoleRepository extends Lifecycle
     /**
      * Create a role, given that the roles token is unique.
      * @param role the new role object
-     * @throws IllegalCredentialsException if the role name is not valid
+     * @throws IllegalArgumentException if the role name is not valid
      */
-    void create( RoleRecord role ) throws IllegalCredentialsException, IOException;
+    void create( RoleRecord role ) throws IllegalArgumentException, IOException;
 
     /**
      * Update a role, given that the role token is unique.

enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/ShiroAuthManager.java

@@ -40,9 +40,8 @@
 import org.neo4j.server.security.auth.RateLimitedAuthenticationStrategy;
 import org.neo4j.server.security.auth.User;
 import org.neo4j.server.security.auth.UserRepository;
-import org.neo4j.server.security.auth.exception.ConcurrentModificationException;
 
-public class ShiroAuthManager extends BasicAuthManager
+public class ShiroAuthManager extends BasicAuthManager implements RoleManager
 {
     private final SecurityManager securityManager;
     private final EhCacheManager cacheManager;
@@ -108,32 +107,14 @@ public User newUser( String username, String initialPassword, boolean requirePas
     {
         assertAuthEnabled();
 
-        try
-        {
-            return realm.newUser( username, initialPassword, requirePasswordChange );
-
-        }
-        catch ( ConcurrentModificationException e )
-        {
-            // TODO: Try again
-            return null;
-        }
+        return realm.newUser( username, initialPassword, requirePasswordChange );
     }
 
     public RoleRecord newRole( String roleName, String... users ) throws IOException, IllegalCredentialsException
     {
         assertAuthEnabled();
 
-        try
-        {
-            return realm.newRole( roleName, users );
-
-        }
-        catch ( ConcurrentModificationException e )
-        {
-            // TODO: Try again
-            return null;
-        }
+        return realm.newRole( roleName, users );
     }
 
 
@@ -184,4 +165,16 @@ public void setPassword( AuthSubject authSubject, String username, String passwo
 
         setUserPassword( username, password );
     }
+
+    @Override
+    public void addUserToRole( String username, String roleName ) throws IOException
+    {
+        realm.addUserToRole( username, roleName );
+    }
+
+    @Override
+    public void removeUserFromRole( String username, String roleName ) throws IOException
+    {
+        realm.removeUserFromRole( username, roleName );
+    }
 }

enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/InMemoryRoleRepository.java

@@ -48,7 +48,7 @@ public Set<String> findByUsername( String username )
     }
 
     @Override
-    public void create( RoleRecord role ) throws IllegalCredentialsException
+    public void create( RoleRecord role )
     {
         synchronized (this)
         {
@@ -57,7 +57,7 @@ public void create( RoleRecord role ) throws IllegalCredentialsException
             {
                 if ( other.name().equals( role.name() ) )
                 {
-                    throw new IllegalCredentialsException( "The specified role already exists" );
+                    throw new IllegalArgumentException( "The specified role already exists" );
                 }
             }