From f77846ff528dddf5f84b4c4e6584c6afdb886e8a Mon Sep 17 00:00:00 2001 From: fickludd Date: Fri, 2 Sep 2016 21:39:53 +0200 Subject: [PATCH] Added logging of changePassword calls - also included password_change_required state where appropriate --- .../enterprise/auth/AuthProcedures.java | 32 +++++++++++++-- .../auth/AuthProceduresLoggingTest.java | 39 +++++++++++++------ .../AuthScenariosInteractionTestBase.java | 4 +- .../auth/MultiRealmAuthManagerTest.java | 16 +++----- 4 files changed, 63 insertions(+), 28 deletions(-) diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AuthProcedures.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AuthProcedures.java index b8e7afcf30538..11798131fb747 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AuthProcedures.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AuthProcedures.java @@ -80,7 +80,8 @@ public void createUser( { StandardEnterpriseAuthSubject adminSubject = ensureAdminAuthSubject(); 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 ) { @@ -90,6 +91,27 @@ public void createUser( } @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 ) public void changeUserPassword( @Name( "username" ) String username, @@ -104,9 +126,10 @@ public void changeUserPassword( StandardEnterpriseAuthSubject enterpriseSubject = StandardEnterpriseAuthSubject.castOrFail( authSubject ); if ( enterpriseSubject.doesUsernameMatch( username ) ) { - ownOrOther = "own password"; + ownOrOther = "password"; 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() ) { @@ -117,7 +140,8 @@ else if ( !enterpriseSubject.isAdmin() ) enterpriseSubject.getUserManager().setUserPassword( username, newPassword, requirePasswordChange ); terminateTransactionsForValidUser( 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 ) diff --git a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthProceduresLoggingTest.java b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthProceduresLoggingTest.java index 827846d165946..79bb3ee912080 100644 --- a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthProceduresLoggingTest.java +++ b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthProceduresLoggingTest.java @@ -83,8 +83,9 @@ public void shouldLogCreatingUser() throws Throwable authProcedures.createUser( "mats", "el password", false ); log.assertExactly( - info( "[admin]: created user `%s`", "andres" ), - info( "[admin]: created user `%s`", "mats" ) ); + info( "[admin]: created user `%s`%s", "andres", ", with password change required" ), + info( "[admin]: created user `%s`%s", "mats", "" ) + ); } @Test @@ -121,7 +122,7 @@ public void shouldLogDeletingUser() throws Throwable authProcedures.deleteUser( "andres" ); log.assertExactly( - info( "[admin]: created user `%s`", "andres" ), + info( "[admin]: created user `%s`%s", "andres", "" ), info( "[admin]: deleted user `%s`", "andres" ) ); } @@ -149,7 +150,7 @@ public void shouldLogAddingRoleToUser() throws Throwable authProcedures.addRoleToUser( ARCHITECT, "mats" ); 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" ) ); } @@ -160,7 +161,7 @@ public void shouldLogFailureToAddRoleToUser() throws Throwable catchInvalidArguments( () -> authProcedures.addRoleToUser( "null", "mats" ) ); 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." ) ); } @@ -217,26 +218,33 @@ public void shouldLogUnauthorizedRemovingRole() throws Throwable } @Test - public void shouldLogPasswordChanges() throws IOException, InvalidArgumentsException + public void shouldLogUserPasswordChanges() throws IOException, InvalidArgumentsException { // Given authProcedures.createUser( "mats", "neo4j", true ); log.clear(); // When - authProcedures.changeUserPassword( "mats", "longerPassword", false ); + authProcedures.changeUserPassword( "mats", "longPassword", false ); + authProcedures.changeUserPassword( "mats", "longerPassword", true ); authProcedures.authSubject = matsSubject; authProcedures.changeUserPassword( "mats", "evenLongerPassword", false ); + authProcedures.changePassword( "superLongPassword", false ); + authProcedures.changePassword( "infinitePassword", true ); + // Then log.assertExactly( - info( "[admin]: changed password for user `%s`", "mats" ), - info( "[mats]: changed own password" ) + info( "[admin]: changed password for user `%s`%s", "mats", "" ), + 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 - public void shouldLogFailureToChangePassword() throws Throwable + public void shouldLogFailureToChangeUserPassword() throws Throwable { // Given authProcedures.createUser( "andres", "neo4j", true ); @@ -267,10 +275,17 @@ public void shouldLogFailureToChangeOwnPassword() throws Throwable catchInvalidArguments( () -> authProcedures.changeUserPassword( "mats", "neo4j", false ) ); catchInvalidArguments( () -> authProcedures.changeUserPassword( "mats", "", false ) ); + catchInvalidArguments( () -> authProcedures.changePassword( null, false ) ); + catchInvalidArguments( () -> authProcedures.changePassword( "", false ) ); + catchInvalidArguments( () -> authProcedures.changePassword( "neo4j", false ) ); + // Then 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", "own password", "A password cannot be empty." ) + error( "[mats]: tried to change %s: %s", "password", "Old password and new password cannot be the same." ), + 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." ) ); } diff --git a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthScenariosInteractionTestBase.java b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthScenariosInteractionTestBase.java index 3565e39c17510..58ff6b1ce54d1 100644 --- a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthScenariosInteractionTestBase.java +++ b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/AuthScenariosInteractionTestBase.java @@ -123,8 +123,8 @@ public void shouldLogSecurityEvents() throws Exception log.assertHasLine( "mats", "logged in" ); 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 own password: A password cannot be empty."); - log.assertHasLine( "mats", "changed own password"); + log.assertHasLine( "mats", "tried to change password: A password cannot be empty."); + log.assertHasLine( "mats", "changed password"); log.assertHasLine( "adminSubject", "removed role `reader` from user `mats`"); log.assertHasLine( "adminSubject", "deleted user `mats`"); } diff --git a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/MultiRealmAuthManagerTest.java b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/MultiRealmAuthManagerTest.java index dd0b9bd03fbfd..ce9cdb7cefd34 100644 --- a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/MultiRealmAuthManagerTest.java +++ b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/MultiRealmAuthManagerTest.java @@ -51,6 +51,8 @@ import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; 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.logging.AssertableLogProvider.inLog; import static org.neo4j.server.security.auth.SecurityTestUtils.authToken; @@ -195,7 +197,8 @@ public void shouldFailAuthenticationAndEscapeIfUserIsNotFound() throws Throwable // Then 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 @@ -241,15 +244,8 @@ public void shouldFailDeletingUnknownUser() throws Throwable manager.start(); // When - try - { - userManager.deleteUser( "unknown" ); - fail("Should throw exception on deleting unknown user"); - } - catch ( InvalidArgumentsException e ) - { - e.getMessage().equals( "User 'unknown' does not exist" ); - } + assertException( () -> userManager.deleteUser( "unknown" ), + InvalidArgumentsException.class, "User 'unknown' does not exist" ); // Then assertNotNull( users.getUserByName( "jake" ) );