Improved thread-safety of background auth file read from disk

- now reads snapshots of the user and role filess until they are consistent, then atomically replaced user- and role repository contents in one go
- InternalFlatFileRealm now only starts read from disk on start, and stops reading on stop

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

@@ -25,6 +25,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
@@ -35,10 +36,11 @@
 public abstract class AbstractUserRepository extends LifecycleAdapter implements UserRepository
 {
     /** Quick lookup of users by name */
-    protected final Map<String, User> usersByName = new ConcurrentHashMap<>();
+    private final Map<String, User> usersByName = new ConcurrentHashMap<>();
 
     /** Master list of users */
     protected volatile List<User> users = new ArrayList<>();
+    protected AtomicLong lastLoaded = new AtomicLong( 0L );
 
     private final Pattern usernamePattern = Pattern.compile( "^[a-zA-Z0-9_]+$" );
 
@@ -58,10 +60,7 @@ public User getUserByName( String username )
     @Override
     public void create( User user ) throws InvalidArgumentsException, IOException
     {
-        if ( !isValidUsername( user.name() ) )
-        {
-            throw new InvalidArgumentsException( "'" + user.name() + "' is not a valid user name." );
-        }
+        assertValidUsername( user.name() );
 
         synchronized (this)
         {
@@ -74,27 +73,33 @@ public void create( User user ) throws InvalidArgumentsException, IOException
                 }
             }
 
+            persistUsers();
             users.add( user );
-
-            saveUsers();
-
             usersByName.put( user.name(), user );
         }
     }
 
-    /**
-     * Override this in the implementing class to persist users
-     *
-     * @throws IOException
-     */
-    protected abstract void saveUsers() throws IOException;
+    @Override
+    public void setUsers( ListSnapshot<User> usersSnapshot ) throws InvalidArgumentsException, IOException
+    {
+        for ( User user : usersSnapshot.values )
+        {
+            assertValidUsername( user.name() );
+        }
 
-    /**
-     * Override this in the implementing class to persist users
-     *
-     * @throws IOException
-     */
-    abstract void loadUsers() throws IOException;
+        synchronized (this)
+        {
+            clear();
+
+            this.users.addAll( usersSnapshot.values );
+            this.lastLoaded.set( usersSnapshot.timestamp );
+
+            for ( User user : users )
+            {
+                usersByName.put( user.name(), user );
+            }
+        }
+    }
 
     @Override
     public void update( User existingUser, User updatedUser )
@@ -131,7 +136,7 @@ public void update( User existingUser, User updatedUser )
 
             users = newUsers;
 
-            saveUsers();
+            persistUsers();
 
             usersByName.put( updatedUser.name(), updatedUser );
         }
@@ -159,7 +164,7 @@ public synchronized boolean delete( User user ) throws IOException
         {
             users = newUsers;
 
-            saveUsers();
+            persistUsers();
 
             usersByName.remove( user.name() );
         }
@@ -183,4 +188,27 @@ public synchronized Set<String> getAllUsernames()
     {
         return users.stream().map( User::name ).collect( Collectors.toSet() );
     }
+
+    /**
+     * Override this in the implementing class to persist users
+     *
+     * @throws IOException
+     */
+    protected abstract void persistUsers() throws IOException;
+
+    /**
+     * Override this in the implementing class to load users
+     *
+     * @returns a timestamped snapshot of users, or null if the backing file did not exist
+     * @throws IOException
+     */
+    protected abstract ListSnapshot<User> readPersistedUsers() throws IOException;
+
+    protected void assertValidUsername( String username ) throws InvalidArgumentsException
+    {
+        if ( !isValidUsername( username ) )
+        {
+            throw new InvalidArgumentsException( "'" + username + "' is not a valid user name." );
+        }
+    }
 }

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

@@ -22,6 +22,7 @@
 import java.io.File;
 import java.io.IOException;
 import java.util.List;
+import java.util.stream.Collectors;
 
 import org.neo4j.io.fs.FileSystemAbstraction;
 import org.neo4j.logging.Log;
@@ -36,7 +37,6 @@ public class FileUserRepository extends AbstractUserRepository
 {
     private final File authFile;
     private final FileSystemAbstraction fileSystem;
-    private long lastLoaded;
 
     // TODO: We could improve concurrency by using a ReadWriteLock
 
@@ -55,46 +55,53 @@ public FileUserRepository( FileSystemAbstraction fileSystem, File file, LogProvi
     public void start() throws Throwable
     {
         clear();
-        loadUsers();
+        ListSnapshot<User> onDiskUsers = readPersistedUsers();
+        if ( onDiskUsers != null )
+        {
+            setUsers( onDiskUsers );
+        }
     }
 
-    void loadUsers() throws IOException
+    @Override
+    protected ListSnapshot<User> readPersistedUsers() throws IOException
     {
         if ( fileSystem.fileExists( authFile ) )
         {
-            List<User> loadedUsers;
+            long readTime;
+            List<User> readUsers;
             try
             {
-                lastLoaded = fileSystem.lastModifiedTime( authFile );
-                loadedUsers = serialization.loadRecordsFromFile( fileSystem, authFile );
-            } catch ( FormatException e )
+                readTime = fileSystem.lastModifiedTime( authFile );
+                readUsers = serialization.loadRecordsFromFile( fileSystem, authFile );
+            }
+            catch ( FormatException e )
             {
-                log.error( "Failed to read authentication file \"%s\" (%s)", authFile.getAbsolutePath(),
-                        e.getMessage() );
+                log.error( "Failed to read authentication file \"%s\" (%s)",
+                        authFile.getAbsolutePath(), e.getMessage() );
                 throw new IllegalStateException( "Failed to read authentication file: " + authFile );
             }
 
-            clear();
-            users = loadedUsers;
-            for ( User user : users )
-            {
-                usersByName.put( user.name(), user );
-            }
+            return new ListSnapshot<>( readTime, readUsers );
         }
+        return null;
     }
 
     @Override
-    protected void saveUsers() throws IOException
+    protected void persistUsers() throws IOException
     {
         serialization.saveRecordsToFile( fileSystem, authFile, users );
     }
 
     @Override
-    public void reloadIfNeeded() throws IOException
+    public ListSnapshot<User> getPersistedSnapshot() throws IOException
     {
-        if ( lastLoaded < fileSystem.lastModifiedTime( authFile ) )
+        if ( lastLoaded.get() < fileSystem.lastModifiedTime( authFile ) )
+        {
+            return readPersistedUsers();
+        }
+        synchronized ( this )
         {
-            loadUsers();
+            return new ListSnapshot<>( lastLoaded.get(), users.stream().collect( Collectors.toList() ) );
         }
     }
 }

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

@@ -0,0 +1,34 @@
+/*
+ * 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 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.server.security.auth;
+
+import java.util.List;
+
+public class ListSnapshot<T>
+{
+    public final long timestamp;
+    public final List<T> values;
+
+    public ListSnapshot( long timestamp, List<T> values )
+    {
+        this.timestamp = timestamp;
+        this.values = values;
+    }
+}

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

@@ -51,6 +51,14 @@ public interface UserRepository extends Lifecycle
      */
     void create( User user ) throws InvalidArgumentsException, IOException;
 
+    /**
+     * Replaces the users in the repository with the given users.
+     * @param users the new users
+     * @throws InvalidArgumentsException if any username is not valid
+     * @throws IOException if the underlying storage for users fails
+     */
+    void setUsers( ListSnapshot<User> users ) throws InvalidArgumentsException, IOException;
+
     /**
      * Update a user, given that the users token is unique.
      * @param existingUser the existing user object, which must match the current state in this repository
@@ -77,5 +85,10 @@ void update( User existingUser, User updatedUser )
 
     Set<String> getAllUsernames();
 
-    void reloadIfNeeded() throws IOException;
+    /**
+     * Returns a snapshot of the current persisted user repository
+     * @return a snapshot of the current persisted user repository
+     * @throws IOException
+     */
+    ListSnapshot<User> getPersistedSnapshot() throws IOException;
 }

community/security/src/test/java/org/neo4j/server/security/auth/InMemoryUserRepository.java

@@ -25,20 +25,20 @@
 public class InMemoryUserRepository extends AbstractUserRepository
 {
     @Override
-    protected void saveUsers() throws IOException
+    protected void persistUsers() throws IOException
     {
         // Nothing to do
     }
 
     @Override
-    void loadUsers() throws IOException
+    protected ListSnapshot<User> readPersistedUsers() throws IOException
     {
-        // Nothing to do
+        return null;
     }
 
     @Override
-    public void reloadIfNeeded()
+    public ListSnapshot<User> getPersistedSnapshot() throws IOException
     {
-        // Nothing to do
+        return null;
     }
 }