Merge pull request #6857 from pontusmelke/3.0-fail-on-password-reuse

Reusing password should fail
neo4j · Apr 7, 2016 · 9af30da · 9af30da
2 parents 1d091f2 + 8eac823
commit 9af30da
Show file tree

Hide file tree

Showing 11 changed files with 115 additions and 21 deletions.
diff --git a/community/bolt/src/main/java/org/neo4j/bolt/security/auth/BasicAuthentication.java b/community/bolt/src/main/java/org/neo4j/bolt/security/auth/BasicAuthentication.java
@@ -29,6 +29,7 @@
 import org.neo4j.logging.LogProvider;
 import org.neo4j.server.security.auth.AuthSubject;
 import org.neo4j.server.security.auth.BasicAuthManager;
+import org.neo4j.server.security.auth.exception.IllegalCredentialsException;
 
 /**
  * Performs basic authentication with user name and password.
@@ -111,6 +112,10 @@ private AuthenticationResult update( String user, String password, String newPas
             {
                 throw new AuthenticationException( Status.Security.Unauthorized, identifier.get(), e.getMessage(), e );
             }
+            catch ( IllegalCredentialsException e )
+            {
+                throw new AuthenticationException(e.status(), identifier.get(), e.getMessage(), e );
+            }
             break;
         default:
             throw new AuthenticationException( Status.Security.Unauthorized, identifier.get() );

diff --git a/community/bolt/src/test/java/org/neo4j/bolt/v1/transport/integration/AuthenticationIT.java b/community/bolt/src/test/java/org/neo4j/bolt/v1/transport/integration/AuthenticationIT.java
@@ -201,6 +201,56 @@ public void shouldBeAbleToChangePasswordUsingBuiltInProcedure() throws Throwable
         assertThat( client, eventuallyRecieves( msgSuccess() ) );
     }
 
+    @Test
+    public void shouldFailWhenReusingTheSamePassword() throws Throwable
+    {
+        // When
+        client.connect( address )
+                .send( TransportTestUtil.acceptedVersions( 1, 0, 0, 0 ) )
+                .send( TransportTestUtil.chunk(
+                        init( "TestClient/1.1",
+                                map( "principal", "neo4j", "credentials", "neo4j", "scheme", "basic" ) ) ) );
+
+
+        // Then
+        assertThat( client, eventuallyRecieves( new byte[]{0, 0, 0, 1} ) );
+        assertThat( client, eventuallyRecieves( msgSuccess( Collections.singletonMap( "credentials_expired", true )) ) );
+
+        // When
+        client.send( TransportTestUtil.chunk(
+                run( "CALL sys.changePassword", Collections.singletonMap( "password", "neo4j" ) ),
+                pullAll() ) );
+
+        // Then
+        assertThat( client, eventuallyRecieves( msgFailure(Status.Request.Invalid,
+                "Old password and new password cannot be the same.") ) );
+    }
+
+    @Test
+    public void shouldFailWhenSubmittingEmptyPassword() throws Throwable
+    {
+        // When
+        client.connect( address )
+                .send( TransportTestUtil.acceptedVersions( 1, 0, 0, 0 ) )
+                .send( TransportTestUtil.chunk(
+                        init( "TestClient/1.1",
+                                map( "principal", "neo4j", "credentials", "neo4j", "scheme", "basic" ) ) ) );
+
+
+        // Then
+        assertThat( client, eventuallyRecieves( new byte[]{0, 0, 0, 1} ) );
+        assertThat( client, eventuallyRecieves( msgSuccess( Collections.singletonMap( "credentials_expired", true )) ) );
+
+        // When
+        client.send( TransportTestUtil.chunk(
+                run( "CALL sys.changePassword", Collections.singletonMap( "password", "" ) ),
+                pullAll() ) );
+
+        // Then
+        assertThat( client, eventuallyRecieves( msgFailure(Status.Request.Invalid,
+                "Password cannot be empty.") ) );
+    }
+
     @Test
     public void shouldNotBeAbleToReadWhenPasswordChangeRequired() throws Throwable
     {

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/AlterUserPasswordProcedure.java b/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/AlterUserPasswordProcedure.java
@@ -31,6 +31,7 @@
 import org.neo4j.kernel.api.proc.ProcedureSignature.ProcedureName;
 import org.neo4j.kernel.api.security.AccessMode;
 import org.neo4j.server.security.auth.AuthSubject;
+import org.neo4j.server.security.auth.exception.IllegalCredentialsException;
 
 import static org.neo4j.helpers.collection.Iterators.asRawIterator;
 import static org.neo4j.helpers.collection.Iterators.map;
@@ -71,5 +72,9 @@ public RawIterator<Object[],ProcedureException> apply( Context ctx, Object[] inp
             throw new ProcedureException( Status.Security.Forbidden, e,
                     "Failed to change the password for the provided username" );
         }
+        catch ( IllegalCredentialsException e )
+        {
+           throw new ProcedureException( e.status(), e, e.getMessage() );
+        }
     }
 }
diff --git a/community/security/src/main/java/org/neo4j/server/security/auth/AuthManager.java b/community/security/src/main/java/org/neo4j/server/security/auth/AuthManager.java
@@ -21,7 +21,7 @@
 
 import java.io.IOException;
 
-import org.neo4j.server.security.auth.exception.IllegalUsernameException;
+import org.neo4j.server.security.auth.exception.IllegalCredentialsException;
 
 /**
  * An AuthManager is used to do basic authentication and user management.
@@ -51,10 +51,10 @@ public interface AuthManager
      * @param requirePasswordChange Does the user need to change the initial password.
      * @return A new user with the provided credentials.
      * @throws IOException If user can't be serialized to disk.
-     * @throws IllegalUsernameException If the username is invalid.
+     * @throws IllegalCredentialsException If the username is invalid.
      */
     User newUser( String username, String initialPassword, boolean requirePasswordChange ) throws IOException,
-            IllegalUsernameException;
+            IllegalCredentialsException;
 
     /**
      * Delete the given user
@@ -108,7 +108,7 @@ public AuthenticationResult getAuthenticationResult()
                 }
 
                 @Override
-                public void setPassword( String password ) throws IOException
+                public void setPassword( String password ) throws IOException, IllegalCredentialsException
                 {
                 }
 
@@ -140,7 +140,7 @@ public String name()
 
         @Override
         public User newUser( String username, String initialPassword, boolean requirePasswordChange )
-                throws IOException, IllegalUsernameException
+                throws IOException, IllegalCredentialsException
         {
             return new User.Builder(  )
                     .withName( username )

diff --git a/community/security/src/main/java/org/neo4j/server/security/auth/AuthSubject.java b/community/security/src/main/java/org/neo4j/server/security/auth/AuthSubject.java
@@ -22,12 +22,19 @@
 import java.io.IOException;
 
 import org.neo4j.kernel.api.security.AccessMode;
+import org.neo4j.server.security.auth.exception.IllegalCredentialsException;
 
 public interface AuthSubject extends AccessMode
 {
     void logout();
 
     AuthenticationResult getAuthenticationResult();
 
-    void setPassword( String password ) throws IOException;
+    /**
+     * Set the password for the AuthSubject
+     * @param password The new password
+     * @throws IOException If the new credentials cannot be serialized to disk.
+     * @throws IllegalCredentialsException If the new password is invalid.
+     */
+    void setPassword( String password ) throws IOException, IllegalCredentialsException;
 }
diff --git a/community/security/src/main/java/org/neo4j/server/security/auth/BasicAuthManager.java b/community/security/src/main/java/org/neo4j/server/security/auth/BasicAuthManager.java
@@ -25,7 +25,7 @@
 import org.neo4j.graphdb.security.AuthorizationViolationException;
 import org.neo4j.kernel.lifecycle.LifecycleAdapter;
 import org.neo4j.server.security.auth.exception.ConcurrentModificationException;
-import org.neo4j.server.security.auth.exception.IllegalUsernameException;
+import org.neo4j.server.security.auth.exception.IllegalCredentialsException;
 
 /**
  * Manages server authentication and authorization.
@@ -90,7 +90,8 @@ public AuthSubject login( String username, String password )
     }
 
     @Override
-    public User newUser( String username, String initialPassword, boolean requirePasswordChange ) throws IOException, IllegalUsernameException
+    public User newUser( String username, String initialPassword, boolean requirePasswordChange ) throws IOException,
+            IllegalCredentialsException
     {
         assertAuthEnabled();
         assertValidName( username );

diff --git a/community/security/src/main/java/org/neo4j/server/security/auth/BasicAuthSubject.java b/community/security/src/main/java/org/neo4j/server/security/auth/BasicAuthSubject.java
@@ -22,6 +22,7 @@
 import java.io.IOException;
 
 import org.neo4j.kernel.api.security.AccessMode;
+import org.neo4j.server.security.auth.exception.IllegalCredentialsException;
 
 public class BasicAuthSubject implements AuthSubject
 {
@@ -59,9 +60,18 @@ public AuthenticationResult getAuthenticationResult()
         return authenticationResult;
     }
 
+    /**
+     * Sets a new password for the BasicAuthSubject.
+     *
+     * @param password The new password
+     * @throws IOException If the new user credentials cannot be stored on disk.
+     * @throws IllegalCredentialsException If password is invalid, e.g. if the new password is the same as the current.
+     */
     @Override
-    public void setPassword( String password ) throws IOException
+    public void setPassword( String password ) throws IOException, IllegalCredentialsException
     {
+        validatePassword( password );
+
         authManager.setPassword( this, user.name(), password );
     }
 
@@ -93,4 +103,20 @@ public String name()
     {
         return accessMode.name();
     }
+
+    /*
+     * This is really some very basic password policy and that functionality should probably be
+     * refactored out of the BasicAuthSubject.
+     */
+    private void validatePassword( String password ) throws IllegalCredentialsException
+    {
+        if (password == null || password.isEmpty() )
+        {
+            throw new IllegalCredentialsException( "Password cannot be empty." );
+        }
+        if ( user.credentials().matchesPassword( password ) )
+        {
+            throw new IllegalCredentialsException( "Old password and new password cannot be the same." );
+        }
+    }
 }
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
@@ -31,7 +31,7 @@
 import org.neo4j.logging.Log;
 import org.neo4j.logging.LogProvider;
 import org.neo4j.server.security.auth.exception.ConcurrentModificationException;
-import org.neo4j.server.security.auth.exception.IllegalUsernameException;
+import org.neo4j.server.security.auth.exception.IllegalCredentialsException;
 
 import static java.nio.file.StandardCopyOption.ATOMIC_MOVE;
 import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
@@ -75,11 +75,11 @@ public void start() throws Throwable
     }
 
     @Override
-    public void create( User user ) throws IllegalUsernameException, IOException
+    public void create( User user ) throws IllegalCredentialsException, IOException
     {
         if ( !isValidName( user.name() ) )
         {
-            throw new IllegalUsernameException( "'" + user.name() + "' is not a valid user name." );
+            throw new IllegalCredentialsException( "'" + user.name() + "' is not a valid user name." );
         }
 
         synchronized (this)
@@ -89,7 +89,7 @@ public void create( User user ) throws IllegalUsernameException, IOException
             {
                 if ( other.name().equals( user.name() ) )
                 {
-                    throw new IllegalUsernameException( "The specified user already exists" );
+                    throw new IllegalCredentialsException( "The specified user already exists" );
                 }
             }
 

diff --git a/community/security/src/main/java/org/neo4j/server/security/auth/UserRepository.java b/community/security/src/main/java/org/neo4j/server/security/auth/UserRepository.java
@@ -22,7 +22,7 @@
 import java.io.IOException;
 
 import org.neo4j.server.security.auth.exception.ConcurrentModificationException;
-import org.neo4j.server.security.auth.exception.IllegalUsernameException;
+import org.neo4j.server.security.auth.exception.IllegalCredentialsException;
 
 /**
  * A component that can store and retrieve users. Implementations must be thread safe.
@@ -34,9 +34,9 @@ public interface UserRepository
     /**
      * Create a user, given that the users token is unique.
      * @param user the new user object
-     * @throws IllegalUsernameException if the username is not valid
+     * @throws IllegalCredentialsException if the username is not valid
      */
-    void create( User user ) throws IllegalUsernameException, IOException;
+    void create( User user ) throws IllegalCredentialsException, IOException;
 
     /**
      * Update a user, given that the users token is unique.

diff --git a/...h/exception/IllegalUsernameException.java → ...xception/IllegalCredentialsException.java b/...h/exception/IllegalUsernameException.java → ...xception/IllegalCredentialsException.java
@@ -21,11 +21,11 @@
 
 import org.neo4j.kernel.api.exceptions.Status;
 
-public class IllegalUsernameException extends Exception implements Status.HasStatus
+public class IllegalCredentialsException extends Exception implements Status.HasStatus
 {
     private final Status status;
 
-    public IllegalUsernameException( String message )
+    public IllegalCredentialsException( String message )
     {
         super(message);
         this.status = Status.Request.Invalid;

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
@@ -22,7 +22,7 @@
 import java.util.concurrent.ConcurrentHashMap;
 
 import org.neo4j.server.security.auth.exception.ConcurrentModificationException;
-import org.neo4j.server.security.auth.exception.IllegalUsernameException;
+import org.neo4j.server.security.auth.exception.IllegalCredentialsException;
 
 /** A user repository implementation that just stores users in memory */
 public class InMemoryUserRepository implements UserRepository
@@ -36,7 +36,7 @@ public User findByName( String name )
     }
 
     @Override
-    public void create( User user ) throws IllegalUsernameException
+    public void create( User user ) throws IllegalCredentialsException
     {
         synchronized (this)
         {
@@ -45,7 +45,7 @@ public void create( User user ) throws IllegalUsernameException
             {
                 if ( other.name().equals( user.name() ) )
                 {
-                    throw new IllegalUsernameException( "The specified user already exists" );
+                    throw new IllegalCredentialsException( "The specified user already exists" );
                 }
             }