Skip to content

Commit

Permalink
Added logging of changePassword calls
Browse files Browse the repository at this point in the history
- also included password_change_required state where appropriate
  • Loading branch information
fickludd committed Sep 12, 2016
1 parent 82b3120 commit f77846f
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 28 deletions.
Expand Up @@ -80,7 +80,8 @@ public void createUser(
{ {
StandardEnterpriseAuthSubject adminSubject = ensureAdminAuthSubject(); StandardEnterpriseAuthSubject adminSubject = ensureAdminAuthSubject();
adminSubject.getUserManager().newUser( username, password, requirePasswordChange ); adminSubject.getUserManager().newUser( username, password, requirePasswordChange );
securityLog.info( authSubject, "created user `%s`", username ); securityLog.info( authSubject, "created user `%s`%s", username,
requirePasswordChange ? ", with password change required" : "" );
} }
catch ( Exception e ) catch ( Exception e )
{ {
Expand All @@ -90,6 +91,27 @@ public void createUser(
} }


@Description( "Change the current user's password." ) @Description( "Change the current user's password." )
@Procedure( name = "dbms.security.changePassword", mode = DBMS )
public void changePassword(
@Name( "password" ) String password,
@Name( value = "requirePasswordChange", defaultValue = "false" ) boolean requirePasswordChange
)
throws InvalidArgumentsException, IOException
{
try
{
authSubject.setPassword( password, requirePasswordChange );
securityLog.info( authSubject, "changed password%s",
requirePasswordChange ? ", with password change required" : "" );
}
catch ( Exception e )
{
securityLog.error( authSubject, "tried to change password: %s", e.getMessage() );
throw e;
}
}

@Description( "Change the given user's password." )
@Procedure( name = "dbms.security.changeUserPassword", mode = DBMS ) @Procedure( name = "dbms.security.changeUserPassword", mode = DBMS )
public void changeUserPassword( public void changeUserPassword(
@Name( "username" ) String username, @Name( "username" ) String username,
Expand All @@ -104,9 +126,10 @@ public void changeUserPassword(
StandardEnterpriseAuthSubject enterpriseSubject = StandardEnterpriseAuthSubject.castOrFail( authSubject ); StandardEnterpriseAuthSubject enterpriseSubject = StandardEnterpriseAuthSubject.castOrFail( authSubject );
if ( enterpriseSubject.doesUsernameMatch( username ) ) if ( enterpriseSubject.doesUsernameMatch( username ) )
{ {
ownOrOther = "own password"; ownOrOther = "password";
enterpriseSubject.setPassword( newPassword, requirePasswordChange ); enterpriseSubject.setPassword( newPassword, requirePasswordChange );
securityLog.info( authSubject, "changed own password" ); securityLog.info( authSubject, "changed password%s",
requirePasswordChange ? ", with password change required" : "" );
} }
else if ( !enterpriseSubject.isAdmin() ) else if ( !enterpriseSubject.isAdmin() )
{ {
Expand All @@ -117,7 +140,8 @@ else if ( !enterpriseSubject.isAdmin() )
enterpriseSubject.getUserManager().setUserPassword( username, newPassword, requirePasswordChange ); enterpriseSubject.getUserManager().setUserPassword( username, newPassword, requirePasswordChange );
terminateTransactionsForValidUser( username ); terminateTransactionsForValidUser( username );
terminateConnectionsForValidUser( username ); terminateConnectionsForValidUser( username );
securityLog.info( authSubject, "changed password for user `%s`", username ); securityLog.info( authSubject, "changed password for user `%s`%s", username,
requirePasswordChange ? ", with password change required" : "" );
} }
} }
catch ( Exception e ) catch ( Exception e )
Expand Down
Expand Up @@ -83,8 +83,9 @@ public void shouldLogCreatingUser() throws Throwable
authProcedures.createUser( "mats", "el password", false ); authProcedures.createUser( "mats", "el password", false );


log.assertExactly( log.assertExactly(
info( "[admin]: created user `%s`", "andres" ), info( "[admin]: created user `%s`%s", "andres", ", with password change required" ),
info( "[admin]: created user `%s`", "mats" ) ); info( "[admin]: created user `%s`%s", "mats", "" )
);
} }


@Test @Test
Expand Down Expand Up @@ -121,7 +122,7 @@ public void shouldLogDeletingUser() throws Throwable
authProcedures.deleteUser( "andres" ); authProcedures.deleteUser( "andres" );


log.assertExactly( log.assertExactly(
info( "[admin]: created user `%s`", "andres" ), info( "[admin]: created user `%s`%s", "andres", "" ),
info( "[admin]: deleted user `%s`", "andres" ) ); info( "[admin]: deleted user `%s`", "andres" ) );
} }


Expand Down Expand Up @@ -149,7 +150,7 @@ public void shouldLogAddingRoleToUser() throws Throwable
authProcedures.addRoleToUser( ARCHITECT, "mats" ); authProcedures.addRoleToUser( ARCHITECT, "mats" );


log.assertExactly( log.assertExactly(
info( "[admin]: created user `%s`", "mats" ), info( "[admin]: created user `%s`%s", "mats", "" ),
info( "[admin]: added role `%s` to user `%s`", ARCHITECT, "mats" ) ); info( "[admin]: added role `%s` to user `%s`", ARCHITECT, "mats" ) );
} }


Expand All @@ -160,7 +161,7 @@ public void shouldLogFailureToAddRoleToUser() throws Throwable
catchInvalidArguments( () -> authProcedures.addRoleToUser( "null", "mats" ) ); catchInvalidArguments( () -> authProcedures.addRoleToUser( "null", "mats" ) );


log.assertExactly( log.assertExactly(
info( "[admin]: created user `%s`", "mats" ), info( "[admin]: created user `%s`%s", "mats", "" ),
error( "[admin]: tried to add role `%s` to user `%s`: %s", "null", "mats", "Role 'null' does not exist." ) ); error( "[admin]: tried to add role `%s` to user `%s`: %s", "null", "mats", "Role 'null' does not exist." ) );
} }


Expand Down Expand Up @@ -217,26 +218,33 @@ public void shouldLogUnauthorizedRemovingRole() throws Throwable
} }


@Test @Test
public void shouldLogPasswordChanges() throws IOException, InvalidArgumentsException public void shouldLogUserPasswordChanges() throws IOException, InvalidArgumentsException
{ {
// Given // Given
authProcedures.createUser( "mats", "neo4j", true ); authProcedures.createUser( "mats", "neo4j", true );
log.clear(); log.clear();


// When // When
authProcedures.changeUserPassword( "mats", "longerPassword", false ); authProcedures.changeUserPassword( "mats", "longPassword", false );
authProcedures.changeUserPassword( "mats", "longerPassword", true );
authProcedures.authSubject = matsSubject; authProcedures.authSubject = matsSubject;
authProcedures.changeUserPassword( "mats", "evenLongerPassword", false ); authProcedures.changeUserPassword( "mats", "evenLongerPassword", false );


authProcedures.changePassword( "superLongPassword", false );
authProcedures.changePassword( "infinitePassword", true );

// Then // Then
log.assertExactly( log.assertExactly(
info( "[admin]: changed password for user `%s`", "mats" ), info( "[admin]: changed password for user `%s`%s", "mats", "" ),
info( "[mats]: changed own password" ) info( "[admin]: changed password for user `%s`%s", "mats", ", with password change required" ),
info( "[mats]: changed password%s", "" ),
info( "[mats]: changed password%s", "" ),
info( "[mats]: changed password%s", ", with password change required" )
); );
} }


@Test @Test
public void shouldLogFailureToChangePassword() throws Throwable public void shouldLogFailureToChangeUserPassword() throws Throwable
{ {
// Given // Given
authProcedures.createUser( "andres", "neo4j", true ); authProcedures.createUser( "andres", "neo4j", true );
Expand Down Expand Up @@ -267,10 +275,17 @@ public void shouldLogFailureToChangeOwnPassword() throws Throwable
catchInvalidArguments( () -> authProcedures.changeUserPassword( "mats", "neo4j", false ) ); catchInvalidArguments( () -> authProcedures.changeUserPassword( "mats", "neo4j", false ) );
catchInvalidArguments( () -> authProcedures.changeUserPassword( "mats", "", false ) ); catchInvalidArguments( () -> authProcedures.changeUserPassword( "mats", "", false ) );


catchInvalidArguments( () -> authProcedures.changePassword( null, false ) );
catchInvalidArguments( () -> authProcedures.changePassword( "", false ) );
catchInvalidArguments( () -> authProcedures.changePassword( "neo4j", false ) );

// Then // Then
log.assertExactly( log.assertExactly(
error( "[mats]: tried to change %s: %s", "own password", "Old password and new password cannot be the same." ), error( "[mats]: tried to change %s: %s", "password", "Old password and new password cannot be the same." ),
error( "[mats]: tried to change %s: %s", "own password", "A password cannot be empty." ) error( "[mats]: tried to change %s: %s", "password", "A password cannot be empty." ),
error( "[mats]: tried to change password: %s", "A password cannot be empty." ),
error( "[mats]: tried to change password: %s", "A password cannot be empty." ),
error( "[mats]: tried to change password: %s", "Old password and new password cannot be the same." )
); );
} }


Expand Down
Expand Up @@ -123,8 +123,8 @@ public void shouldLogSecurityEvents() throws Exception
log.assertHasLine( "mats", "logged in" ); log.assertHasLine( "mats", "logged in" );
log.assertHasLine( "adminSubject", "added role `reader` to user `mats`" ); log.assertHasLine( "adminSubject", "added role `reader` to user `mats`" );
log.assertHasLine( "mats", "tried to change password for user `neo4j`: " + PERMISSION_DENIED); log.assertHasLine( "mats", "tried to change password for user `neo4j`: " + PERMISSION_DENIED);
log.assertHasLine( "mats", "tried to change own password: A password cannot be empty."); log.assertHasLine( "mats", "tried to change password: A password cannot be empty.");
log.assertHasLine( "mats", "changed own password"); log.assertHasLine( "mats", "changed password");
log.assertHasLine( "adminSubject", "removed role `reader` from user `mats`"); log.assertHasLine( "adminSubject", "removed role `reader` from user `mats`");
log.assertHasLine( "adminSubject", "deleted user `mats`"); log.assertHasLine( "adminSubject", "deleted user `mats`");
} }
Expand Down
Expand Up @@ -51,6 +51,8 @@
import static org.junit.Assert.fail; import static org.junit.Assert.fail;
import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when; import static org.mockito.Mockito.when;

import static org.neo4j.helpers.Strings.escape;
import static org.neo4j.helpers.collection.MapUtil.map; import static org.neo4j.helpers.collection.MapUtil.map;
import static org.neo4j.logging.AssertableLogProvider.inLog; import static org.neo4j.logging.AssertableLogProvider.inLog;
import static org.neo4j.server.security.auth.SecurityTestUtils.authToken; import static org.neo4j.server.security.auth.SecurityTestUtils.authToken;
Expand Down Expand Up @@ -195,7 +197,8 @@ public void shouldFailAuthenticationAndEscapeIfUserIsNotFound() throws Throwable


// Then // Then
assertThat( result, equalTo( AuthenticationResult.FAILURE ) ); assertThat( result, equalTo( AuthenticationResult.FAILURE ) );
logProvider.assertExactly( error( "[%s]: failed to log in: invalid principal or credentials", "unknown\n\t\r\"haxx0r\"" ) ); logProvider.assertExactly( error( "[%s]: failed to log in: invalid principal or credentials",
escape( "unknown\n\t\r\"haxx0r\"" ) ) );
} }


@Test @Test
Expand Down Expand Up @@ -241,15 +244,8 @@ public void shouldFailDeletingUnknownUser() throws Throwable
manager.start(); manager.start();


// When // When
try assertException( () -> userManager.deleteUser( "unknown" ),
{ InvalidArgumentsException.class, "User 'unknown' does not exist" );
userManager.deleteUser( "unknown" );
fail("Should throw exception on deleting unknown user");
}
catch ( InvalidArgumentsException e )
{
e.getMessage().equals( "User 'unknown' does not exist" );
}


// Then // Then
assertNotNull( users.getUserByName( "jake" ) ); assertNotNull( users.getUserByName( "jake" ) );
Expand Down

0 comments on commit f77846f

Please sign in to comment.