From 82b312084dc35b61ca8d6cf37e12056ac06c355e Mon Sep 17 00:00:00 2001 From: Mats Rydberg Date: Fri, 2 Sep 2016 16:10:10 +0200 Subject: [PATCH] Add tests for failed attempts to list users and roles --- .../security/auth/AbstractUserRepository.java | 2 +- .../enterprise/auth/AuthProcedures.java | 15 +-- .../auth/StandardEnterpriseAuthSubject.java | 2 +- .../auth/AuthProceduresLoggingTest.java | 119 +++++++++++++++++- 4 files changed, 123 insertions(+), 15 deletions(-) diff --git a/community/security/src/main/java/org/neo4j/server/security/auth/AbstractUserRepository.java b/community/security/src/main/java/org/neo4j/server/security/auth/AbstractUserRepository.java index 6b5d2de571eac..064947b559a91 100644 --- a/community/security/src/main/java/org/neo4j/server/security/auth/AbstractUserRepository.java +++ b/community/security/src/main/java/org/neo4j/server/security/auth/AbstractUserRepository.java @@ -54,7 +54,7 @@ public void clear() @Override public User getUserByName( String username ) { - return usersByName.get( username ); + return username == null ? null : usersByName.get( username ); } @Override 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 5fdb42fa2ce35..b8e7afcf30538 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 @@ -193,7 +193,7 @@ public void deleteUser( @Name( "username" ) String username ) throws InvalidArgu kickoutUser( username, "deletion" ); } - void kickoutUser( String username, String reason ) + private void kickoutUser( String username, String reason ) { try { @@ -203,7 +203,7 @@ void kickoutUser( String username, String reason ) catch ( Exception e ) { securityLog.error( authSubject, "failed to terminate running transaction and bolt connections for " + - "user `%s` following %s. %s", username, reason, e.getMessage() ); + "user `%s` following %s: %s", username, reason, e.getMessage() ); throw e; } } @@ -287,7 +287,7 @@ public Stream listUsers() throws InvalidArgumentsException, IOExcept } catch ( Exception e ) { - securityLog.error( authSubject, "tried to list users. %s", e.getMessage() ); + securityLog.error( authSubject, "tried to list users: %s", e.getMessage() ); throw e; } } @@ -310,7 +310,7 @@ public Stream listRoles() throws InvalidArgumentsException, IOExcept } catch ( Exception e ) { - securityLog.error( authSubject, "tried to list roles. %s", e.getMessage() ); + securityLog.error( authSubject, "tried to list roles: %s", e.getMessage() ); throw e; } } @@ -327,7 +327,7 @@ public Stream listRolesForUser( @Name( "username" ) String usernam } catch ( Exception e ) { - securityLog.error( authSubject, "tried to list roles for user `%s`. %s", username, e.getMessage() ); + securityLog.error( authSubject, "tried to list roles for user `%s`: %s", username, e.getMessage() ); throw e; } } @@ -344,7 +344,7 @@ public Stream listUsersForRole( @Name( "roleName" ) String roleNam } catch ( Exception e ) { - securityLog.error( authSubject, "tried to list users for role `%s`. %s", roleName, e.getMessage() ); + securityLog.error( authSubject, "tried to list users for role `%s`: %s", roleName, e.getMessage() ); throw e; } } @@ -507,12 +507,13 @@ private StandardEnterpriseAuthSubject ensureAdminAuthSubject() private StandardEnterpriseAuthSubject ensureSelfOrAdminAuthSubject( String username ) throws InvalidArgumentsException { StandardEnterpriseAuthSubject subject = StandardEnterpriseAuthSubject.castOrFail( authSubject ); - subject.getUserManager().getUser( username ); if ( subject.isAdmin() || subject.doesUsernameMatch( username ) ) { + subject.getUserManager().getUser( username ); return subject; } + throw new AuthorizationViolationException( PERMISSION_DENIED ); } diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/StandardEnterpriseAuthSubject.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/StandardEnterpriseAuthSubject.java index 30377c59e99b1..fe6e77637b8b6 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/StandardEnterpriseAuthSubject.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/StandardEnterpriseAuthSubject.java @@ -106,7 +106,7 @@ public boolean isAdmin() public boolean doesUsernameMatch( String username ) { Object principal = shiroSubject.getPrincipal(); - return principal != null && username.equals( principal ); + return principal != null && username != null && username.equals( principal ); } @Override 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 a0f476970468e..827846d165946 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 @@ -36,7 +36,6 @@ import org.neo4j.server.security.auth.AuthenticationStrategy; import org.neo4j.server.security.auth.BasicPasswordPolicy; import org.neo4j.server.security.auth.InMemoryUserRepository; -import org.neo4j.test.assertion.Assert; import static org.mockito.Mockito.mock; @@ -45,10 +44,11 @@ import static org.neo4j.server.security.enterprise.auth.PredefinedRolesBuilder.ADMIN; import static org.neo4j.server.security.enterprise.auth.PredefinedRolesBuilder.ARCHITECT; import static org.neo4j.server.security.enterprise.auth.PredefinedRolesBuilder.READER; +import static org.neo4j.test.assertion.Assert.assertException; public class AuthProceduresLoggingTest { - private AuthProcedures authProcedures; + private TestAuthProcedures authProcedures; private AssertableLogProvider log = null; private AuthSubject matsSubject = null; @@ -481,14 +481,115 @@ public void shouldLogUnauthorizedDeletingRole() throws Exception log.assertExactly( error( "[mats]: tried to delete role `%s`: %s", ADMIN, PERMISSION_DENIED ) ); } + @Test + public void shouldLogIfUnexpectedErrorTerminatingTransactions() throws Exception + { + // Given + authProcedures.createUser( "johan", "neo4j", false ); + authProcedures.failTerminateTransaction(); + log.clear(); + + // When + assertException( () -> authProcedures.deleteUser( "johan" ), RuntimeException.class, "Unexpected error" ); + + // Then + log.assertExactly( + info( "[admin]: deleted user `%s`", "johan" ), + error( "[admin]: failed to terminate running transaction and bolt connections for user `%s` following %s: %s", + "johan", "deletion", "Unexpected error" ) + ); + } + + @Test + public void shouldLogUnauthorizedListUsers() throws Exception + { + // Given + authProcedures.authSubject = matsSubject; + + // When + catchAuthorizationViolation( () -> authProcedures.listUsers() ); + + log.assertExactly( error( "[mats]: tried to list users: %s", PERMISSION_DENIED ) ); + } + + @Test + public void shouldLogUnauthorizedListRoles() throws Exception + { + // Given + authProcedures.authSubject = matsSubject; + + // When + catchAuthorizationViolation( () -> authProcedures.listRoles() ); + + log.assertExactly( error( "[mats]: tried to list roles: %s", PERMISSION_DENIED ) ); + } + + @Test + public void shouldLogFailureToListRolesForUser() throws Exception + { + // Given + + // When + catchInvalidArguments( () -> authProcedures.listRolesForUser( null ) ); + catchInvalidArguments( () -> authProcedures.listRolesForUser( "" ) ); + catchInvalidArguments( () -> authProcedures.listRolesForUser( "nonExistent" ) ); + + log.assertExactly( + error( "[admin]: tried to list roles for user `%s`: %s", null, "User 'null' does not exist." ), + error( "[admin]: tried to list roles for user `%s`: %s", "", "User '' does not exist." ), + error( "[admin]: tried to list roles for user `%s`: %s", "nonExistent", "User 'nonExistent' does not exist." ) + ); + } + + @Test + public void shouldLogUnauthorizedListRolesForUser() throws Exception + { + // Given + authProcedures.authSubject = matsSubject; + + // When + catchAuthorizationViolation( () -> authProcedures.listRolesForUser( "user" ) ); + + log.assertExactly( error( "[mats]: tried to list roles for user `%s`: %s", "user", PERMISSION_DENIED ) ); + } + + @Test + public void shouldLogFailureToListUsersForRole() throws Exception + { + // Given + + // When + catchInvalidArguments( () -> authProcedures.listUsersForRole( null ) ); + catchInvalidArguments( () -> authProcedures.listUsersForRole( "" ) ); + catchInvalidArguments( () -> authProcedures.listUsersForRole( "nonExistent" ) ); + + log.assertExactly( + error( "[admin]: tried to list users for role `%s`: %s", null, "Role 'null' does not exist." ), + error( "[admin]: tried to list users for role `%s`: %s", "", "Role '' does not exist." ), + error( "[admin]: tried to list users for role `%s`: %s", "nonExistent", "Role 'nonExistent' does not exist." ) + ); + } + + @Test + public void shouldLogUnauthorizedListUsersForRole() throws Exception + { + // Given + authProcedures.authSubject = matsSubject; + + // When + catchAuthorizationViolation( () -> authProcedures.listUsersForRole( "role" ) ); + + log.assertExactly( error( "[mats]: tried to list users for role `%s`: %s", "role", PERMISSION_DENIED ) ); + } + private void catchInvalidArguments( ThrowingAction f ) throws Exception { - Assert.assertException( f, InvalidArgumentsException.class, "" ); + assertException( f, InvalidArgumentsException.class, "" ); } private void catchAuthorizationViolation( ThrowingAction f ) throws Exception { - Assert.assertException( f, AuthorizationViolationException.class, "" ); + assertException( f, AuthorizationViolationException.class, "" ); } private AssertableLogProvider.LogMatcher info( String message, String... arguments ) @@ -563,14 +664,20 @@ public Object getPrincipal() private static class TestAuthProcedures extends AuthProcedures { - @Override - void kickoutUser( String username, String reason ) + private boolean failTerminateTransactions = false; + + void failTerminateTransaction() { + failTerminateTransactions = true; } @Override Stream terminateTransactionsForValidUser( String username ) { + if ( failTerminateTransactions ) + { + throw new RuntimeException( "Unexpected error" ); + } return Stream.empty(); }