Added PR feedback

neo4j · Sep 23, 2016 · d01e066 · d01e066
1 parent 7520bec
commit d01e066
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 18 deletions.
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
@@ -29,6 +29,9 @@
 import org.neo4j.logging.LogProvider;
 import org.neo4j.server.security.auth.exception.FormatException;
 
+import static org.neo4j.server.security.auth.ListSnapshot.FROM_MEMORY;
+import static org.neo4j.server.security.auth.ListSnapshot.FROM_PERSISTED;
+
 /**
  * Stores user auth data. In memory, but backed by persistent storage so changes to this repository will survive
  * JVM restarts and crashes.
@@ -81,7 +84,7 @@ protected ListSnapshot<User> readPersistedUsers() throws IOException
                 throw new IllegalStateException( "Failed to read authentication file: " + authFile );
             }
 
-            return new ListSnapshot<>( readTime, readUsers, true );
+            return new ListSnapshot<>( readTime, readUsers, FROM_PERSISTED );
         }
         return null;
     }
@@ -101,7 +104,7 @@ public ListSnapshot<User> getPersistedSnapshot() throws IOException
         }
         synchronized ( this )
         {
-            return new ListSnapshot<>( lastLoaded.get(), users.stream().collect( Collectors.toList() ), false );
+            return new ListSnapshot<>( lastLoaded.get(), users.stream().collect( Collectors.toList() ), FROM_MEMORY );
         }
     }
 }
diff --git a/community/security/src/main/java/org/neo4j/server/security/auth/ListSnapshot.java b/community/security/src/main/java/org/neo4j/server/security/auth/ListSnapshot.java
@@ -25,13 +25,13 @@ public class ListSnapshot<T>
 {
     private final long timestamp;
     private final List<T> values;
-    private final boolean updated;
+    private final boolean fromPersisted;
 
-    public ListSnapshot( long timestamp, List<T> values, boolean updated )
+    public ListSnapshot( long timestamp, List<T> values, boolean fromPersisted )
     {
         this.timestamp = timestamp;
         this.values = values;
-        this.updated = updated;
+        this.fromPersisted = fromPersisted;
     }
 
     public long timestamp()
@@ -44,8 +44,11 @@ public List<T> values()
         return values;
     }
 
-    public boolean updated()
+    public boolean fromPersisted()
     {
-        return updated;
+        return fromPersisted;
     }
+
+    public static final boolean FROM_PERSISTED = true;
+    public static final boolean FROM_MEMORY = false;
 }
diff --git a/.../security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java b/.../security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java
@@ -30,6 +30,9 @@
 import org.neo4j.server.security.auth.ListSnapshot;
 import org.neo4j.server.security.auth.exception.FormatException;
 
+import static org.neo4j.server.security.auth.ListSnapshot.FROM_MEMORY;
+import static org.neo4j.server.security.auth.ListSnapshot.FROM_PERSISTED;
+
 /**
  * Stores role data. In memory, but backed by persistent storage so changes to this repository will survive
  * JVM restarts and crashes.
@@ -77,7 +80,7 @@ protected ListSnapshot<RoleRecord> readPersistedRoles() throws IOException
                 throw new IllegalStateException( "Failed to read role file '" + roleFile + "'." );
             }
 
-            return new ListSnapshot<>( readTime, readRoles, true );
+            return new ListSnapshot<>( readTime, readRoles, FROM_PERSISTED );
         }
         return null;
     }
@@ -97,7 +100,7 @@ public ListSnapshot<RoleRecord> getPersistedSnapshot() throws IOException
         }
         synchronized ( this )
         {
-            return new ListSnapshot<>( lastLoaded.get(), roles.stream().collect( Collectors.toList() ), false );
+            return new ListSnapshot<>( lastLoaded.get(), roles.stream().collect( Collectors.toList() ), FROM_MEMORY );
         }
     }
 }
diff --git a/...curity/src/main/java/org/neo4j/server/security/enterprise/auth/InternalFlatFileRealm.java b/...curity/src/main/java/org/neo4j/server/security/enterprise/auth/InternalFlatFileRealm.java
@@ -161,25 +161,28 @@ private void readFilesFromDisk( int attemptLeft, java.util.List<String> failures
         try
         {
             final boolean valid;
+            final boolean needsUpdate;
             synchronized ( this )
             {
                 ListSnapshot<User> users = userRepository.getPersistedSnapshot();
                 ListSnapshot<RoleRecord> roles = roleRepository.getPersistedSnapshot();
-                boolean needsUpdate = users.updated() || roles.updated();
+
-                valid = !needsUpdate || RoleRepository.validate( users.values(), roles.values() );
+                needsUpdate = users.fromPersisted() || roles.fromPersisted();
+                valid = needsUpdate && RoleRepository.validate( users.values(), roles.values() );
+
                 if ( valid )
                 {
-                    if ( users.updated() )
+                    if ( users.fromPersisted() )
                     {
                         userRepository.setUsers( users );
                     }
-                    if ( roles.updated() )
+                    if ( roles.fromPersisted() )
                     {
                         roleRepository.setRoles( roles );
                     }
                 }
             }
-            if ( !valid )
+            if ( needsUpdate && !valid )
             {
                 failures.add( "Role-auth file combination not valid." );
                 Thread.sleep( 10 );
@@ -555,8 +558,6 @@ public void setUserPassword( String username, String password, boolean requirePa
     @Override
     public void suspendUser( String username ) throws IOException, InvalidArgumentsException
     {
-        // This method is not synchronized as it only modifies the UserRepository, which is synchronized in itself
-        // If user is modified between getUserByName and update, we get ConcurrentModificationException and try again
         User user = getUser( username );
         if ( !user.hasFlag( IS_SUSPENDED ) )
         {
@@ -581,8 +582,6 @@ public void suspendUser( String username ) throws IOException, InvalidArgumentsE
     public void activateUser( String username, boolean requirePasswordChange )
             throws IOException, InvalidArgumentsException
     {
-        // This method is not synchronized as it only modifies the UserRepository, which is synchronized in itself
-        // If user is modified between getUserByName and update, we get ConcurrentModificationException and try again
         User user = getUser( username );
         if ( user.hasFlag( IS_SUSPENDED ) )
         {