Skip to content

Commit

Permalink
Refactor user and role name validation
Browse files Browse the repository at this point in the history 
Clean up login logging testing to support new log format
  • Loading branch information
@Mats-SX @fickludd
Mats-SX authored and fickludd committed Sep 12, 2016
1 parent f15b7cf commit 8010ec5
Show file tree
Hide file tree
Showing 13 changed files with 143 additions and 101 deletions.
25 changes: 25 additions & 0 deletions community/kernel/src/test/java/org/neo4j/test/assertion/Assert.java
View file
Expand Up @@ -26,16 +26,41 @@
import java.util.Objects;
import java.util.concurrent.TimeUnit;

import org.neo4j.function.ThrowingAction;
import org.neo4j.function.ThrowingSupplier;
import org.neo4j.helpers.ArrayUtil;
import org.neo4j.helpers.Strings;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.StringContains.containsString;
import static org.junit.Assert.fail;

public final class Assert
{
private Assert()
{
}

public static <E extends Exception> void assertException( ThrowingAction<E> f, Class typeOfException,
String partOfErrorMessage ) throws Exception
{
try
{
f.apply();
}
catch ( Exception e )
{
if ( typeOfException.isInstance( e ) )
{
assertThat( e.getMessage(), containsString( partOfErrorMessage ) );
}
else
{
fail( "Got unexpected exception " + e.getClass() + "\nExpected: " + typeOfException );
}
}
}

public static void assertObjectOrArrayEquals( Object expected, Object actual )
{
assertObjectOrArrayEquals( "", expected, actual );
Expand Down
21 changes: 11 additions & 10 deletions community/security/src/main/java/org/neo4j/server/security/auth/AbstractUserRepository.java
View file
Expand Up @@ -174,9 +174,18 @@ public synchronized int numberOfUsers()
}

@Override
public boolean isValidUsername( String username )
public void assertValidUsername( String username ) throws InvalidArgumentsException
{
return usernamePattern.matcher( username ).matches();
if ( username == null || username.isEmpty() )
{
throw new InvalidArgumentsException( "The provided username is empty." );
}
if ( !usernamePattern.matcher( username ).matches() )
{
throw new InvalidArgumentsException(
"Username '" + username +
"' contains illegal characters. Use simple ascii characters and numbers." );
}
}

@Override
Expand All @@ -199,12 +208,4 @@ public synchronized Set<String> getAllUsernames()
* @throws IOException
*/
protected abstract ListSnapshot<User> readPersistedUsers() throws IOException;

protected void assertValidUsername( String username ) throws InvalidArgumentsException
{
if ( !isValidUsername( username ) )
{
throw new InvalidArgumentsException( "'" + username + "' is not a valid user name." );
}
}
}
16 changes: 1 addition & 15 deletions community/security/src/main/java/org/neo4j/server/security/auth/BasicAuthManager.java
View file
Expand Up @@ -124,7 +124,7 @@ public BasicAuthSubject login( Map<String,Object> authToken ) throws InvalidAuth
public User newUser( String username, String initialPassword, boolean requirePasswordChange )
throws IOException, InvalidArgumentsException
{
assertValidUsername( username );
userRepository.assertValidUsername( username );

passwordPolicy.validatePassword( initialPassword );

Expand Down Expand Up @@ -202,20 +202,6 @@ public Set<String> getAllUsernames()
return userRepository.getAllUsernames();
}

private void assertValidUsername( String name ) throws InvalidArgumentsException
{
if ( name.isEmpty() )
{
throw new InvalidArgumentsException( "The provided user name is empty." );
}
if ( !userRepository.isValidUsername( name ) )
{
throw new InvalidArgumentsException(
"User name '" + name +
"' contains illegal characters. Use simple ascii characters and numbers." );
}
}

@Override
public UserManager getUserManager()
{
Expand Down
9 changes: 7 additions & 2 deletions community/security/src/main/java/org/neo4j/server/security/auth/UserRepository.java
View file
Expand Up @@ -80,8 +80,13 @@ void update( User existingUser, User updatedUser )

int numberOfUsers();

/** Utility for API consumers to tell if #create() will accept a given username */
boolean isValidUsername( String username );
/**
* Asserts whether the given username is valid or not. A valid username is non-null, non-empty, and contains
* only simple ascii characters.
* @param username the username to be tested.
* @throws InvalidArgumentsException if the username was invalid.
*/
void assertValidUsername( String username ) throws InvalidArgumentsException;

Set<String> getAllUsernames();

Expand Down
6 changes: 3 additions & 3 deletions community/security/src/test/java/org/neo4j/server/security/auth/AuthProceduresIT.java
View file
Expand Up @@ -134,10 +134,10 @@ public void shouldCreateUserWithDefault() throws Exception
@Test
public void shouldNotCreateUserIfInvalidUsername() throws Exception
{
assertFail( admin, "CALL dbms.security.createUser('', '1234', true)", "The provided user name is empty." );
assertFail( admin, "CALL dbms.security.createUser('', '1234', true)", "The provided username is empty." );
assertFail( admin, "CALL dbms.security.createUser('&%ss!', '1234', true)",
"User name '&%ss!' contains illegal characters." );
assertFail( admin, "CALL dbms.security.createUser('&%ss!', '', true)", "User name '&%ss!' contains illegal characters." );
"Username '&%ss!' contains illegal characters." );
assertFail( admin, "CALL dbms.security.createUser('&%ss!', '', true)", "Username '&%ss!' contains illegal characters." );
}

@Test
Expand Down
37 changes: 28 additions & 9 deletions community/security/src/test/java/org/neo4j/server/security/auth/FileUserRepositoryTest.java
View file
Expand Up @@ -37,6 +37,18 @@
import org.neo4j.graphdb.mockfs.DelegatingFileSystemAbstraction;
import org.neo4j.io.fs.DelegateFileSystemAbstraction;
import org.neo4j.io.fs.FileSystemAbstraction;
import com.google.common.jimfs.Configuration;
import com.google.common.jimfs.Jimfs;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;

import org.neo4j.io.fs.DelegatingFileSystem;
import org.neo4j.io.fs.DelegatingFileSystemProvider;
import org.neo4j.kernel.api.security.exception.InvalidArgumentsException;
import org.neo4j.logging.AssertableLogProvider;
import org.neo4j.logging.LogProvider;
import org.neo4j.logging.NullLogProvider;
Expand All @@ -49,9 +61,10 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import static org.neo4j.test.assertion.Assert.assertException;

@RunWith(Parameterized.class)
public class FileUserRepositoryTest
{
Expand Down Expand Up @@ -132,14 +145,20 @@ public void shouldNotAllowComplexNames() throws Exception
FileUserRepository users = new FileUserRepository( fs, authFile, logProvider );

// When
assertTrue( users.isValidUsername( "neo4j" ) );
assertTrue( users.isValidUsername( "johnosbourne" ) );
assertTrue( users.isValidUsername( "john_osbourne" ) );

assertFalse( users.isValidUsername( ":" ) );
assertFalse( users.isValidUsername( "" ) );
assertFalse( users.isValidUsername( "john osbourne" ) );
assertFalse( users.isValidUsername( "john:osbourne" ) );
users.assertValidUsername( "neo4j" );
users.assertValidUsername( "johnosbourne" );
users.assertValidUsername( "john_osbourne" );

assertException( () -> users.assertValidUsername( null ), InvalidArgumentsException.class,
"The provided username is empty." );
assertException( () -> users.assertValidUsername( "" ), InvalidArgumentsException.class,
"The provided username is empty." );
assertException( () -> users.assertValidUsername( ":" ), InvalidArgumentsException.class,
"Username ':' contains illegal characters. Use simple ascii characters and numbers." );
assertException( () -> users.assertValidUsername( "with space" ), InvalidArgumentsException.class,
"Username 'with space' contains illegal characters. Use simple ascii characters and numbers." );
assertException( () -> users.assertValidUsername( "with:colon" ), InvalidArgumentsException.class,
"Username 'with:colon' contains illegal characters. Use simple ascii characters and numbers." );
}

@Test
Expand Down
16 changes: 12 additions & 4 deletions ...urity/src/main/java/org/neo4j/server/security/enterprise/auth/AbstractRoleRepository.java
View file
Expand Up @@ -73,7 +73,7 @@ public Set<String> getRoleNamesByUsername( String username )
}

@Override
public void create( RoleRecord role ) throws IllegalArgumentException, IOException
public void create( RoleRecord role ) throws InvalidArgumentsException, IOException
{
assertValidRoleName( role.name() );

Expand All @@ -84,7 +84,7 @@ public void create( RoleRecord role ) throws IllegalArgumentException, IOExcepti
{
if ( other.name().equals( role.name() ) )
{
throw new IllegalArgumentException( "The specified role '" + role.name() + "' already exists." );
throw new InvalidArgumentsException( "The specified role '" + role.name() + "' already exists." );
}
}

Expand Down Expand Up @@ -204,9 +204,17 @@ public synchronized int numberOfRoles()
}

@Override
public boolean isValidRoleName( String roleName )
public void assertValidRoleName( String name ) throws InvalidArgumentsException
{
return roleNamePattern.matcher( roleName ).matches();
if ( name == null || name.isEmpty() )
{
throw new InvalidArgumentsException( "The provided role name is empty." );
}
if ( !roleNamePattern.matcher( name ).matches() )
{
throw new InvalidArgumentsException(
"Role name '" + name + "' contains illegal characters. Use simple ascii characters and numbers." );
}
}

@Override
Expand Down
39 changes: 7 additions & 32 deletions ...curity/src/main/java/org/neo4j/server/security/enterprise/auth/InternalFlatFileRealm.java
View file
Expand Up @@ -368,6 +368,7 @@ private int numberOfRoles()
public User newUser( String username, String initialPassword, boolean requirePasswordChange )
throws IOException, InvalidArgumentsException
{
userRepository.assertValidUsername( username );
passwordPolicy.validatePassword( initialPassword );

User user = new User.Builder()
Expand All @@ -383,10 +384,10 @@ public User newUser( String username, String initialPassword, boolean requirePas
@Override
public RoleRecord newRole( String roleName, String... usernames ) throws IOException, InvalidArgumentsException
{
assertValidRoleName( roleName );
roleRepository.assertValidRoleName( roleName );
for ( String username : usernames )
{
assertValidUsername( username );
userRepository.assertValidUsername( username );
}

SortedSet<String> userSet = new TreeSet<>( Arrays.asList( usernames ) );
Expand Down Expand Up @@ -433,8 +434,8 @@ public RoleRecord getRole( String roleName ) throws InvalidArgumentsException
@Override
public void addRoleToUser( String roleName, String username ) throws IOException, InvalidArgumentsException
{
assertValidRoleName( roleName );
assertValidUsername( username );
roleRepository.assertValidRoleName( roleName );
userRepository.assertValidUsername( username );

synchronized ( this )
{
Expand All @@ -457,8 +458,8 @@ public void addRoleToUser( String roleName, String username ) throws IOException
@Override
public void removeRoleFromUser( String roleName, String username ) throws IOException, InvalidArgumentsException
{
assertValidRoleName( roleName );
assertValidUsername( username );
roleRepository.assertValidRoleName( roleName );
userRepository.assertValidUsername( username );

synchronized ( this )
{
Expand Down Expand Up @@ -630,32 +631,6 @@ private void removeUserFromAllRoles( String username ) throws IOException
}
}

private void assertValidUsername( String name ) throws InvalidArgumentsException
{
if ( name.isEmpty() )
{
throw new InvalidArgumentsException( "The provided user name is empty." );
}
if ( !userRepository.isValidUsername( name ) )
{
throw new InvalidArgumentsException(
"User name '" + name + "' contains illegal characters. Use simple ascii characters and numbers." );
}
}

private void assertValidRoleName( String name ) throws InvalidArgumentsException
{
if ( name.isEmpty() )
{
throw new InvalidArgumentsException( "The provided role name is empty." );
}
if ( !roleRepository.isValidRoleName( name ) )
{
throw new InvalidArgumentsException(
"Role name '" + name + "' contains illegal characters. Use simple ascii characters and numbers." );
}
}

private void assertNotPredefinedRoleName( String roleName ) throws InvalidArgumentsException
{
if ( PredefinedRolesBuilder.roles.keySet().contains( roleName ) )
Expand Down
6 changes: 4 additions & 2 deletions ...curity/src/main/java/org/neo4j/server/security/enterprise/auth/MultiRealmAuthManager.java
View file
Expand Up @@ -95,20 +95,22 @@ public EnterpriseAuthSubject login( Map<String,Object> authToken ) throws Invali
}
catch ( UnsupportedTokenException e )
{
// TODO: add test for this case
securityLog.error( "Unknown user failed to log in: %s", e.getMessage() );
throw new InvalidAuthTokenException( e.getCause().getMessage() );
}
catch ( ExcessiveAttemptsException e )
{
// NOTE: We only get this with single (internal) realm authentication
subject = new StandardEnterpriseAuthSubject( this,
new ShiroSubject( securityManager, AuthenticationResult.TOO_MANY_ATTEMPTS ) );
securityLog.info( subject, "Login fail for user `%s` - too many failed attempts." );
securityLog.error( "[%s]: failed to log in: too many failed attempts", token.getPrincipal().toString() );
}
catch ( AuthenticationException e )
{
subject = new StandardEnterpriseAuthSubject( this,
new ShiroSubject( securityManager, AuthenticationResult.FAILURE ) );
securityLog.info( subject, "Login fail for user `%s`" );
securityLog.error( "[%s]: failed to log in: invalid principal or credentials", token.getPrincipal().toString() );
}

return subject;
Expand Down
13 changes: 9 additions & 4 deletions ...rise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRepository.java
View file
Expand Up @@ -49,9 +49,9 @@ public interface RoleRepository extends Lifecycle
* Create a role, given that the roles token is unique.
*
* @param role the new role object
* @throws IllegalArgumentException if the role name is not valid or the role name already exists
* @throws InvalidArgumentsException if the role name is not valid or the role name already exists
*/
void create( RoleRecord role ) throws IllegalArgumentException, IOException;
void create( RoleRecord role ) throws InvalidArgumentsException, IOException;

/**
* Replaces the roles in the repository with the given roles.
Expand Down Expand Up @@ -81,8 +81,13 @@ void update( RoleRecord existingRole, RoleRecord updatedRole )

int numberOfRoles();

/** Utility for API consumers to tell if #create() will accept a given role name */
boolean isValidRoleName( String roleName );
/**
* Asserts whether the given role name is valid or not. A valid role name is non-null, non-empty, and contains
* only simple ascii characters.
* @param roleName the role name to be tested.
* @throws InvalidArgumentsException if the role name was invalid.
*/
void assertValidRoleName( String roleName ) throws InvalidArgumentsException;

void removeUserFromAllRoles( String username )
throws ConcurrentModificationException, IOException;
Expand Down
11 changes: 7 additions & 4 deletions ...est/java/org/neo4j/server/security/enterprise/auth/AuthProceduresInteractionTestBase.java
View file
Expand Up @@ -250,18 +250,21 @@ public void shouldCreateUserAndRequireNoPasswordChangeIfRequested() throws Excep
@Test
public void shouldNotCreateUserIfInvalidUsername() throws Exception
{
assertFail( adminSubject, "CALL dbms.security.createUser(null, '1234', true)",
"The provided username is empty." );
assertFail( adminSubject, "CALL dbms.security.createUser('', '1234', true)",
"The provided user name is empty." );
"The provided username is empty." );
assertFail( adminSubject, "CALL dbms.security.createUser('&%ss!', '1234', true)",
"User name '&%ss!' contains illegal characters." );
"Username '&%ss!' contains illegal characters." );
assertFail( adminSubject, "CALL dbms.security.createUser('&%ss!', '', true)",
"User name '&%ss!' contains illegal characters." );
"Username '&%ss!' contains illegal characters." );
}

@Test
public void shouldNotCreateUserIfInvalidPassword() throws Exception
{
assertFail( adminSubject, "CALL dbms.security.createUser('craig', '', true)", "A password cannot be empty." );
assertFail( adminSubject, "CALL dbms.security.createUser('craig', null, true)", "A password cannot be empty." );
}

@Test
Expand Down Expand Up @@ -551,7 +554,7 @@ public void shouldFailToRemoveNonExistentUserFromRole() throws Exception
testFailRemoveRoleFromUser( adminSubject, "thisRoleDoesNotExist", "Olivia", "User 'Olivia' does not exist." );
testFailRemoveRoleFromUser( adminSubject, "", "Olivia", "The provided role name is empty." );
testFailRemoveRoleFromUser( adminSubject, "", "", "The provided role name is empty." );
testFailRemoveRoleFromUser( adminSubject, PUBLISHER, "", "The provided user name is empty." );
testFailRemoveRoleFromUser( adminSubject, PUBLISHER, "", "The provided username is empty." );
}

@Test
Expand Down

0 comments on commit 8010ec5

Please sign in to comment.