Skip to content

Commit

Permalink
Add tests for failed attempts to list users and roles
Browse files Browse the repository at this point in the history
  • Loading branch information
@Mats-SX @fickludd
Mats-SX authored and fickludd committed Sep 12, 2016
1 parent b3a5c83 commit 82b3120
Show file tree
Hide file tree
Showing 4 changed files with 123 additions and 15 deletions.
2 changes: 1 addition & 1 deletion community/security/src/main/java/org/neo4j/server/security/auth/AbstractUserRepository.java
View file
Expand Up @@ -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
Expand Down
15 changes: 8 additions & 7 deletions ...rise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AuthProcedures.java
View file
Expand Up @@ -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
{
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -287,7 +287,7 @@ public Stream<UserResult> 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;
}
}
Expand All @@ -310,7 +310,7 @@ public Stream<RoleResult> 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;
}
}
Expand All @@ -327,7 +327,7 @@ public Stream<StringResult> 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;
}
}
Expand All @@ -344,7 +344,7 @@ public Stream<StringResult> 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;
}
}
Expand Down Expand Up @@ -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 );
}

Expand Down
2 changes: 1 addition & 1 deletion ...rc/main/java/org/neo4j/server/security/enterprise/auth/StandardEnterpriseAuthSubject.java
View file
Expand Up @@ -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
Expand Down
119 changes: 113 additions & 6 deletions ...ty/src/test/java/org/neo4j/server/security/enterprise/auth/AuthProceduresLoggingTest.java
View file
Expand Up @@ -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;

Expand All @@ -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;

Expand Down Expand Up @@ -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<Exception> f ) throws Exception
{
Assert.assertException( f, InvalidArgumentsException.class, "" );
assertException( f, InvalidArgumentsException.class, "" );
}

private void catchAuthorizationViolation( ThrowingAction<Exception> f ) throws Exception
{
Assert.assertException( f, AuthorizationViolationException.class, "" );
assertException( f, AuthorizationViolationException.class, "" );
}

private AssertableLogProvider.LogMatcher info( String message, String... arguments )
Expand Down Expand Up @@ -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<TransactionTerminationResult> terminateTransactionsForValidUser( String username )
{
if ( failTerminateTransactions )
{
throw new RuntimeException( "Unexpected error" );
}
return Stream.empty();
}

Expand Down

0 comments on commit 82b3120

Please sign in to comment.