Ensure synchronization

neo4j · Jul 5, 2016 · b4a04d4 · b4a04d4
1 parent f83ef3a
commit b4a04d4
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 58 deletions.
diff --git a/community/security/src/main/java/org/neo4j/server/security/auth/AbstractUserRepository.java b/community/security/src/main/java/org/neo4j/server/security/auth/AbstractUserRepository.java
@@ -74,15 +74,20 @@ public void create( User user ) throws IllegalArgumentException, IOException
         }
     }
 
-    public abstract void saveUsers() throws IOException;
+    /**
+     * Override this in the implementing class to persist users
+     *
+     * @throws IOException
+     */
+    protected abstract void saveUsers() throws IOException;
 
     @Override
     public void update( User existingUser, User updatedUser ) throws ConcurrentModificationException, IOException
     {
         // Assert input is ok
         if ( !existingUser.name().equals( updatedUser.name() ) )
         {
-            throw new IllegalArgumentException( "updatedUser has a different name" );
+            throw new IllegalArgumentException( "updated user has a different name" );
         }
 
         synchronized (this)
@@ -116,38 +121,36 @@ public void update( User existingUser, User updatedUser ) throws ConcurrentModif
     }
 
     @Override
-    public boolean delete( User user ) throws IOException
+    public synchronized boolean delete( User user ) throws IOException
     {
         boolean foundUser = false;
-        synchronized (this)
+        // Copy-on-write for the users list
+        List<User> newUsers = new ArrayList<>();
+        for ( User other : users )
         {
-            // Copy-on-write for the users list
-            List<User> newUsers = new ArrayList<>();
-            for ( User other : users )
+            if ( other.name().equals( user.name() ) )
             {
-                if ( other.name().equals( user.name() ) )
-                {
-                    foundUser = true;
-                } else
-                {
-                    newUsers.add( other );
-                }
+                foundUser = true;
             }
-
-            if ( foundUser )
+            else
             {
-                users = newUsers;
+                newUsers.add( other );
+            }
+        }
 
-                saveUsers();
+        if ( foundUser )
+        {
+            users = newUsers;
 
-                usersByName.remove( user.name() );
-            }
+            saveUsers();
+
+            usersByName.remove( user.name() );
         }
         return foundUser;
     }
 
     @Override
-    public int numberOfUsers()
+    public synchronized int numberOfUsers()
     {
         return users.size();
     }
@@ -159,7 +162,7 @@ public boolean isValidUsername( String username )
     }
 
     @Override
-    public Set<String> getAllUsernames()
+    public synchronized Set<String> getAllUsernames()
     {
         return users.stream().map( User::name ).collect( Collectors.toSet() );
     }

diff --git a/community/security/src/main/java/org/neo4j/server/security/auth/FileUserRepository.java b/community/security/src/main/java/org/neo4j/server/security/auth/FileUserRepository.java
@@ -60,7 +60,7 @@ public void start() throws Throwable
     }
 
     @Override
-    public void saveUsers() throws IOException
+    protected void saveUsers() throws IOException
     {
         saveUsersToFile();
     }

diff --git a/community/security/src/test/java/org/neo4j/server/security/auth/InMemoryUserRepository.java b/community/security/src/test/java/org/neo4j/server/security/auth/InMemoryUserRepository.java
@@ -32,7 +32,7 @@
 public class InMemoryUserRepository extends AbstractUserRepository
 {
     @Override
-    public void saveUsers() throws IOException
+    protected void saveUsers() throws IOException
     {
         // Nothing to do
     }

diff --git a/...urity/src/main/java/org/neo4j/server/security/enterprise/auth/AbstractRoleRepository.java b/...urity/src/main/java/org/neo4j/server/security/enterprise/auth/AbstractRoleRepository.java
@@ -134,36 +134,33 @@ public void update( RoleRecord existingRole, RoleRecord updatedRole )
     }
 
     @Override
-    public boolean delete( RoleRecord role ) throws IOException
+    public synchronized boolean delete( RoleRecord role ) throws IOException
     {
         boolean foundRole = false;
-        synchronized ( this )
+        // Copy-on-write for the roles list
+        List<RoleRecord> newRoles = new ArrayList<>();
+        for ( RoleRecord other : roles )
         {
-            // Copy-on-write for the roles list
-            List<RoleRecord> newRoles = new ArrayList<>();
-            for ( RoleRecord other : roles )
+            if ( other.name().equals( role.name() ) )
             {
-                if ( other.name().equals( role.name() ) )
-                {
-                    foundRole = true;
-                }
-                else
-                {
-                    newRoles.add( other );
-                }
+                foundRole = true;
             }
-
-            if ( foundRole )
+            else
             {
-                roles = newRoles;
+                newRoles.add( other );
+            }
+        }
 
-                saveRoles();
+        if ( foundRole )
+        {
+            roles = newRoles;
 
-                rolesByName.remove( role.name() );
-            }
+            saveRoles();
 
-            removeFromUserMap( role );
+            rolesByName.remove( role.name() );
         }
+
+        removeFromUserMap( role );
         return foundRole;
     }
 
@@ -175,7 +172,7 @@ public boolean delete( RoleRecord role ) throws IOException
     protected abstract void saveRoles() throws IOException;
 
     @Override
-    public int numberOfRoles()
+    public synchronized int numberOfRoles()
     {
         return roles.size();
     }
@@ -187,27 +184,24 @@ public boolean isValidRoleName( String roleName )
     }
 
     @Override
-    public void removeUserFromAllRoles( String username ) throws ConcurrentModificationException, IOException
+    public synchronized void removeUserFromAllRoles( String username ) throws ConcurrentModificationException, IOException
     {
-        synchronized ( this )
+        Set<String> roles = rolesByUsername.get( username );
+        if ( roles != null )
         {
-            Set<String> roles = rolesByUsername.get( username );
-            if ( roles != null )
+            // Since update() is modifying the set we create a copy for the iteration
+            List<String> rolesToRemoveFrom = new ArrayList<>( roles );
+            for ( String roleName : rolesToRemoveFrom )
             {
-                // Since update() is modifying the set we create a copy for the iteration
-                List<String> rolesToRemoveFrom = new ArrayList<>( roles );
-                for ( String roleName : rolesToRemoveFrom )
-                {
-                    RoleRecord role = rolesByName.get( roleName );
-                    RoleRecord newRole = role.augment().withoutUser( username ).build();
-                    update( role, newRole );
-                }
+                RoleRecord role = rolesByName.get( roleName );
+                RoleRecord newRole = role.augment().withoutUser( username ).build();
+                update( role, newRole );
             }
         }
     }
 
     @Override
-    public Set<String> getAllRoleNames()
+    public synchronized Set<String> getAllRoleNames()
     {
         return roles.stream().map( RoleRecord::name ).collect( Collectors.toSet() );
     }