From d01e066fcb1a14612b41dd689f03631b6e92b1eb Mon Sep 17 00:00:00 2001 From: fickludd Date: Fri, 23 Sep 2016 14:37:20 +0200 Subject: [PATCH] Added PR feedback --- .../security/auth/FileUserRepository.java | 7 +++++-- .../server/security/auth/ListSnapshot.java | 13 ++++++++----- .../enterprise/auth/FileRoleRepository.java | 7 +++++-- .../enterprise/auth/InternalFlatFileRealm.java | 17 ++++++++--------- 4 files changed, 26 insertions(+), 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 index 21f490877ad18..886be22254ab0 100644 --- 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 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 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 index e97c1a1ab8432..2fe3f671e290a 100644 --- 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 { private final long timestamp; private final List values; - private final boolean updated; + private final boolean fromPersisted; - public ListSnapshot( long timestamp, List values, boolean updated ) + public ListSnapshot( long timestamp, List values, boolean fromPersisted ) { this.timestamp = timestamp; this.values = values; - this.updated = updated; + this.fromPersisted = fromPersisted; } public long timestamp() @@ -44,8 +44,11 @@ public List 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/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java index 0e87a45227bfa..aefc522a82246 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java +++ b/enterprise/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 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 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/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/InternalFlatFileRealm.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/InternalFlatFileRealm.java index 88cd605f6716c..bddcc0cd52d83 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/InternalFlatFileRealm.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/InternalFlatFileRealm.java @@ -161,25 +161,28 @@ private void readFilesFromDisk( int attemptLeft, java.util.List failures try { final boolean valid; + final boolean needsUpdate; synchronized ( this ) { ListSnapshot users = userRepository.getPersistedSnapshot(); ListSnapshot 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 ) ) {