Improved suspend/activate based on PR feedback.

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

@@ -19,9 +19,8 @@
  */
 package org.neo4j.server.security.auth;
 
-import java.util.HashSet;
-import java.util.Iterator;
-import java.util.Set;
+import java.util.SortedSet;
+import java.util.TreeSet;
 
 /**
  * Controls authorization and authentication for an individual user.
@@ -39,12 +38,12 @@ public class User
     /** Authentication credentials used by the built in username/password authentication scheme */
     private final Credential credential;
 
-    /** Set of flags, eg. require_password_change */
-    private final HashSet<String> flags;
+    /** Set of flags, eg. password_change_required */
+    private final SortedSet<String> flags;
 
     public static final String PASSWORD_CHANGE_REQUIRED = "password_change_required";
 
-    private User( String name, Credential credential, HashSet<String> flags )
+    private User( String name, Credential credential, SortedSet<String> flags )
     {
         this.name = name;
         this.credential = credential;
@@ -62,7 +61,7 @@ public Credential credentials()
     }
 
     public boolean hasFlag(String flag) { return flags.contains(flag); }
-    public Iterator<String> getFlags() { return flags.iterator(); }
+    public Iterable<String> getFlags() { return flags; }
 
     public boolean passwordChangeRequired() { return flags.contains( PASSWORD_CHANGE_REQUIRED ); }
 
@@ -83,7 +82,7 @@ public boolean equals( Object o )
 
         User user = (User) o;
 
-        if ( !setsAreEquals( flags, user.flags ) )
+        if ( !flags.equals( user.flags ) )
         {
             return false;
         }
@@ -122,7 +121,7 @@ public static class Builder
     {
         private String name;
         private Credential credential = Credential.INACCESSIBLE;
-        private HashSet<String> flags = new HashSet<>();
+        private TreeSet<String> flags = new TreeSet<>();
 
         public Builder() { }
 
@@ -162,21 +161,4 @@ public User build()
             return new User(name, credential, flags );
         }
     }
-
-    private static boolean setsAreEquals( Set<String> set1, Set<String> set2 )
-    {
-        if ( set1.size() != set2.size() )
-        {
-            return false;
-        }
-
-        for ( String element : set1 )
-        {
-            if ( !set2.contains( element ) )
-            {
-                return false;
-            }
-        }
-        return true;
-    }
 }

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

@@ -20,9 +20,7 @@
 package org.neo4j.server.security.auth;
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
-import java.util.Iterator;
 import java.util.List;
 
 import org.neo4j.string.HexString;
@@ -73,9 +71,10 @@ public List<User> deserializeUsers( byte[] bytes ) throws FormatException
 
     private String serialize( User user )
     {
-        return join( userSeparator, user.name(),
+        return String.join( userSeparator,
+                user.name(),
                 serialize( user.credentials() ),
-                join( ",", user.getFlags() )
+                String.join( ",", user.getFlags() )
             );
     }
 
@@ -108,7 +107,7 @@ private String serialize( Credential cred )
     {
         String encodedSalt = HexString.encodeHexString( cred.salt() );
         String encodedPassword = HexString.encodeHexString( cred.passwordHash() );
-        return join( credentialSeparator, Credential.DIGEST_ALGO, encodedPassword, encodedSalt );
+        return String.join( credentialSeparator, Credential.DIGEST_ALGO, encodedPassword, encodedSalt );
     }
 
     private Credential deserializeCredentials( String part, int lineNumber ) throws FormatException
@@ -126,22 +125,4 @@ private Credential deserializeCredentials( String part, int lineNumber ) throws
         byte[] decodedSalt = HexString.decodeHexString( split[2] );
         return new Credential( decodedSalt, decodedPassword );
     }
-
-    private String join( String separator, String... segments )
-    {
-        return join(separator, Arrays.asList(segments).iterator() );
-    }
-
-    private String join( String separator, Iterator<String> segments )
-    {
-        StringBuilder sb = new StringBuilder();
-        boolean afterFirst = false;
-        while (segments.hasNext())
-        {
-            if ( afterFirst ) { sb.append( separator ); }
-            else              { afterFirst = true;      }
-            sb.append( segments.next() );
-        }
-        return sb.toString();
-    }
 }

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

@@ -90,7 +90,7 @@ public void deleteUser( @Name( "username" ) String username ) throws IllegalCred
 
     @PerformsDBMS
     @Procedure( "dbms.suspendUser" )
-    public void suspendUser( @Name( "username" ) String username ) throws IOException, ConcurrentModificationException
+    public void suspendUser( @Name( "username" ) String username ) throws IOException
     {
         ShiroAuthSubject shiroSubject = ShiroAuthSubject.castOrFail( authSubject );
         if ( !shiroSubject.isAdmin() )
@@ -102,7 +102,7 @@ public void suspendUser( @Name( "username" ) String username ) throws IOExceptio
 
     @PerformsDBMS
     @Procedure( "dbms.activateUser" )
-    public void activateUser( @Name( "username" ) String username ) throws IOException, ConcurrentModificationException
+    public void activateUser( @Name( "username" ) String username ) throws IOException
     {
         ShiroAuthSubject shiroSubject = ShiroAuthSubject.castOrFail( authSubject );
         if ( !shiroSubject.isAdmin() )

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

@@ -57,6 +57,10 @@ public class FileUserRealm extends AuthorizingRealm
     private final UserRepository userRepository;
     private final RoleRepository roleRepository;
 
+    /**
+     * This flag is used in the same way as User.PASSWORD_CHANGE_REQUIRED, but it's
+     * placed here because of user suspension not being a part of community edition
+     */
     public static final String IS_SUSPENDED = "is_suspended";
 
     private final CredentialsMatcher credentialsMatcher =
@@ -279,23 +283,55 @@ boolean deleteUser( String username ) throws IOException
         return result;
     }
 
-    void suspendUser( String username ) throws IOException, ConcurrentModificationException
+    void suspendUser( String username ) throws IOException
     {
-        User user = userRepository.findByName( username );
-        if ( user != null && !user.hasFlag( IS_SUSPENDED ) )
+        synchronized ( this )
         {
-            User suspendedUser = user.augment().withFlag( IS_SUSPENDED ).build();
-            userRepository.update( user, suspendedUser );
+            User user = userRepository.findByName( username );
+            if ( user == null )
+            {
+                throw new IllegalArgumentException( "User " + username + " does not exist." );
+            }
+            if ( !user.hasFlag( IS_SUSPENDED ) )
+            {
+                User suspendedUser = user.augment().withFlag( IS_SUSPENDED ).build();
+                try
+                {
+                    userRepository.update( user, suspendedUser );
+                }
+                catch ( ConcurrentModificationException e )
+                {
+                    // Due to the synchronized (this) we should never get this exception unless the user repository
+                    // is modified outside this neo instance. In that case we do not know how to proceed.
+                    throw new RuntimeException( "Unexpected failure", e );
+                }
+            }
         }
     }
 
-    void activateUser( String username ) throws IOException, ConcurrentModificationException
+    void activateUser( String username ) throws IOException
     {
-        User user = userRepository.findByName( username );
-        if ( user != null && user.hasFlag( IS_SUSPENDED ) )
+        synchronized ( this )
         {
-            User activatedUser = user.augment().withoutFlag( IS_SUSPENDED ).build();
-            userRepository.update( user, activatedUser );
+            User user = userRepository.findByName( username );
+            if ( user == null )
+            {
+                throw new IllegalArgumentException( "User " + username + " does not exist." );
+            }
+            if ( user.hasFlag( IS_SUSPENDED ) )
+            {
+                User activatedUser = user.augment().withoutFlag( IS_SUSPENDED ).build();
+                try
+                {
+                    userRepository.update( user, activatedUser );
+                }
+                catch ( ConcurrentModificationException e )
+                {
+                    // Due to the synchronized (this) we should never get this exception unless the user repository
+                    // is modified outside this neo instance. In that case we do not know how to proceed.
+                    throw new RuntimeException( "Unexpected failure", e );
+                }
+            }
         }
     }
 

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

@@ -213,13 +213,13 @@ public boolean deleteUser( String username ) throws IOException
         return realm.deleteUser( username );
     }
 
-    void suspendUser( String username ) throws IOException, ConcurrentModificationException
+    void suspendUser( String username ) throws IOException
     {
         assertAuthEnabled();
         realm.suspendUser( username );
     }
 
-    void activateUser( String username ) throws IOException, ConcurrentModificationException
+    void activateUser( String username ) throws IOException
     {
         assertAuthEnabled();
         realm.activateUser( username );

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

@@ -569,7 +569,9 @@ public void userSuspension1() throws Exception
 
     /*
     Admin creates user Henrik with password bar
+    Admin adds user Henrik to role Reader
     Henrik logs in with correct password → ok
+    Henrik starts and completes transaction with read query → ok
     Admin suspends user Henrik
     Henrik’s session is terminated
     Henrik logs in with correct password → fail
@@ -584,7 +586,7 @@ public void userSuspension2() throws Exception
         testSuccessfulReadAction( subject, 3L );
         testCallEmpty( db, adminSubject, "CALL dbms.suspendUser('Henrik')" );
         testFailReadAction( subject, 3L );
-        // TODO: Check that user session is terminated
+        // TODO: Check that user session is terminated instead of checking failed read
         subject = manager.login( "Henrik", "bar" );
         assertEquals( AuthenticationResult.FAILURE, subject.getAuthenticationResult() );
     }