Fix failing tests

- Handle null or empty string credentials - Add more unit tests
neo4j · Oct 3, 2018 · be15127 · be15127
1 parent a1a3e68
commit be15127
Show file tree

Hide file tree

Showing 8 changed files with 159 additions and 6 deletions.
diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/decoder/InitMessageDecoder.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/decoder/InitMessageDecoder.java
@@ -68,7 +68,7 @@ private static Map<String,Object> readAuthToken( Neo4jPack.Unpacker unpacker ) t
         authTokenValue.foreach( ( key, value ) ->
         {
             Object convertedValue = AuthToken.CREDENTIALS.equals( key ) || AuthToken.NEW_CREDENTIALS.equals( key ) ?
-                                    writer.sensitiveValueAsObject( value, AuthToken.CREDENTIALS ) :
+                                    writer.sensitiveValueAsObject( value, key ) :
                                     writer.valueAsObject( value );
             tokenMap.put( key, convertedValue );
         } );

diff --git a/...nity/bolt/src/main/java/org/neo4j/bolt/v1/messaging/decoder/PrimitiveOnlyValueWriter.java b/...nity/bolt/src/main/java/org/neo4j/bolt/v1/messaging/decoder/PrimitiveOnlyValueWriter.java
@@ -19,6 +19,8 @@
  */
 package org.neo4j.bolt.v1.messaging.decoder;
 
+import org.apache.commons.lang3.ArrayUtils;
+
 import java.time.LocalDate;
 import java.time.LocalDateTime;
 import java.time.LocalTime;
@@ -32,8 +34,11 @@
 import org.neo4j.values.AnyValue;
 import org.neo4j.values.AnyValueWriter;
 import org.neo4j.values.storable.CoordinateReferenceSystem;
+import org.neo4j.values.storable.StringValue;
 import org.neo4j.values.storable.UTF8StringValue;
 
+import static org.neo4j.values.storable.NoValue.NO_VALUE;
+
 /**
  * {@link AnyValueWriter Writer} that allows to convert {@link AnyValue} to any primitive Java type. It explicitly
  * prohibits conversion of nodes, relationships, spatial and temporal types. They are not expected in auth token map.
@@ -52,6 +57,14 @@ public Object sensitiveValueAsObject( AnyValue value, String sensitiveKey )
         {
             return ((UTF8StringValue) value).bytes();
         }
+        else if ( value == NO_VALUE )
+        {
+            return null;
+        }
+        else if ( value instanceof StringValue && ((StringValue) value).equals( "" ) )
+        {
+            return ArrayUtils.EMPTY_BYTE_ARRAY;
+        }
         throw new UnsupportedOperationException(
                 String.format( "INIT message authentication token field '%s' should be a UTF-8 encoded string", sensitiveKey ) );
     }

diff --git a/community/bolt/src/test/java/org/neo4j/bolt/v1/messaging/decoder/InitMessageDecoderTest.java b/community/bolt/src/test/java/org/neo4j/bolt/v1/messaging/decoder/InitMessageDecoderTest.java
@@ -21,15 +21,22 @@
 
 import org.junit.jupiter.api.Test;
 
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Map;
+
 import org.neo4j.bolt.messaging.Neo4jPack.Unpacker;
 import org.neo4j.bolt.messaging.RequestMessage;
 import org.neo4j.bolt.messaging.RequestMessageDecoder;
 import org.neo4j.bolt.runtime.BoltResponseHandler;
 import org.neo4j.bolt.v1.messaging.Neo4jPackV1;
 import org.neo4j.bolt.v1.messaging.request.InitMessage;
 import org.neo4j.bolt.v1.packstream.PackedInputArray;
+import org.neo4j.kernel.api.security.AuthToken;
+import org.neo4j.string.UTF8;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
 import static org.mockito.Mockito.mock;
 import static org.neo4j.bolt.v1.messaging.util.MessageMatchers.serialize;
 import static org.neo4j.helpers.collection.MapUtil.map;
@@ -67,4 +74,132 @@ void shouldDecodeAckFailure() throws Exception
         RequestMessage deserializedMessage = decoder.decode( unpacker );
         assertEquals( originalMessage, deserializedMessage );
     }
+
+    @Test
+    void shouldDecodeAuthTokenWithStringCredentials() throws Exception
+    {
+        testShouldDecodeAuthToken( AuthToken.CREDENTIALS, "password" );
+    }
+
+    @Test
+    void shouldDecodeAuthTokenWithEmptyStringCredentials() throws Exception
+    {
+        testShouldDecodeAuthToken( AuthToken.CREDENTIALS, "" );
+    }
+
+    @Test
+    void shouldDecodeAuthTokenWithNullCredentials() throws Exception
+    {
+        testShouldDecodeAuthToken( AuthToken.CREDENTIALS, null );
+    }
+
+    @Test
+    void shouldDecodeAuthTokenWithStringNewCredentials() throws Exception
+    {
+        testShouldDecodeAuthToken( AuthToken.NEW_CREDENTIALS, "password" );
+    }
+
+    @Test
+    void shouldDecodeAuthTokenWithEmptyStringNewCredentials() throws Exception
+    {
+        testShouldDecodeAuthToken( AuthToken.NEW_CREDENTIALS, "" );
+    }
+
+    @Test
+    void shouldDecodeAuthTokenWithNullNewCredentials() throws Exception
+    {
+        testShouldDecodeAuthToken( AuthToken.NEW_CREDENTIALS, null );
+    }
+
+    @Test
+    void shouldFailToDecodeAuthTokenWithCredentialsOfUnsupportedTypes() throws Exception
+    {
+        for ( Object value : valuesWithInvalidTypes )
+        {
+            testShouldFailToDecodeAuthToken( AuthToken.CREDENTIALS, value,
+                    "INIT message authentication token field '" + AuthToken.CREDENTIALS + "' should be a UTF-8 encoded string" );
+        }
+    }
+
+    @Test
+    void shouldFailToDecodeAuthTokenWithNewCredentialsOfUnsupportedType() throws Exception
+    {
+        for ( Object value : valuesWithInvalidTypes )
+        {
+            testShouldFailToDecodeAuthToken( AuthToken.NEW_CREDENTIALS, value,
+                    "INIT message authentication token field '" + AuthToken.NEW_CREDENTIALS + "' should be a UTF-8 encoded string" );
+        }
+    }
+
+    private static Object[] valuesWithInvalidTypes = {
+            // This is not an exhaustive list
+            new char[]{ 'p', 'a', 's', 's' },
+            Collections.emptyList(),
+            Collections.emptyMap()
+    };
+
+    private void testShouldDecodeAuthToken( String fieldName, Object fieldValue ) throws Exception
+    {
+        Neo4jPackV1 neo4jPack = new Neo4jPackV1();
+        InitMessage originalMessage = new InitMessage( "My Driver", map( AuthToken.PRINCIPAL, "neo4j", fieldName, fieldValue ) );
+
+        PackedInputArray innput = new PackedInputArray( serialize( neo4jPack, originalMessage ) );
+        Unpacker unpacker = neo4jPack.newUnpacker( innput );
+
+        // these two steps are executed before decoding in order to select a correct decoder
+        unpacker.unpackStructHeader();
+        unpacker.unpackStructSignature();
+
+        RequestMessage deserializedMessage = decoder.decode( unpacker );
+        assertInitMessageMatches( originalMessage, deserializedMessage );
+    }
+
+    private void testShouldFailToDecodeAuthToken( String fieldName, Object fieldValue, String expectedErrorMessage ) throws Exception
+    {
+        Neo4jPackV1 neo4jPack = new Neo4jPackV1();
+        InitMessage originalMessage = new InitMessage( "My Driver",
+                map( AuthToken.PRINCIPAL, "neo4j", fieldName, fieldValue ) );
+
+        PackedInputArray innput = new PackedInputArray( serialize( neo4jPack, originalMessage ) );
+        Unpacker unpacker = neo4jPack.newUnpacker( innput );
+
+        // these two steps are executed before decoding in order to select a correct decoder
+        unpacker.unpackStructHeader();
+        unpacker.unpackStructSignature();
+
+        try
+        {
+            decoder.decode( unpacker );
+            fail( "Expected UnsupportedOperationException" );
+        }
+        catch ( UnsupportedOperationException e )
+        {
+            // Expected
+            assertEquals( e.getMessage(), expectedErrorMessage );
+        }
+    }
+
+    private static void assertInitMessageMatches( InitMessage expected, RequestMessage actual )
+    {
+        assertEquals( expected.userAgent(), ((InitMessage) actual).userAgent() );
+        assertAuthTokenMatches( expected.authToken(), ((InitMessage) actual).authToken() );
+    }
+
+    private static void assertAuthTokenMatches( Map<String,Object> expected, Map<String,Object> actual )
+    {
+        assertEquals( expected.keySet(), actual.keySet() );
+        expected.forEach( ( key, expectedValue ) ->
+        {
+            Object actualValue = actual.get( key );
+            if ( AuthToken.containsSensitiveInformation( key ) )
+            {
+                byte[] expectedByteArray = expectedValue != null ? UTF8.encode( (String) expectedValue ) : null;
+                Arrays.equals( expectedByteArray, (byte[]) actualValue );
+            }
+            else
+            {
+                assertEquals( expectedValue, actualValue );
+            }
+        } );
+    }
 }
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/security/AuthToken.java b/community/kernel/src/main/java/org/neo4j/kernel/api/security/AuthToken.java
@@ -93,6 +93,11 @@ else if ( value instanceof Map )
         }
     }
 
+    static boolean containsSensitiveInformation( String key )
+    {
+        return CREDENTIALS.equals( key ) || NEW_CREDENTIALS.equals( key );
+    }
+
     static void clearCredentials( Map<String,Object> authToken )
     {
         Object credentials = authToken.get( CREDENTIALS );

diff --git a/community/security/src/main/java/org/neo4j/server/security/auth/AuthProcedures.java b/community/security/src/main/java/org/neo4j/server/security/auth/AuthProcedures.java
@@ -59,7 +59,7 @@ public void createUser(
     {
         // TODO: Deprecate this and create a new procedure that takes password as a byte[]
         securityContext.assertCredentialsNotExpired();
-        userManager.newUser( username, UTF8.encode( password ), requirePasswordChange );
+        userManager.newUser( username, password != null ? UTF8.encode( password ) : null, requirePasswordChange );
     }
 
     @Description( "Delete the specified user." )

diff --git a/community/server/src/main/java/org/neo4j/server/rest/dbms/AuthorizationEnabledFilter.java b/community/server/src/main/java/org/neo4j/server/rest/dbms/AuthorizationEnabledFilter.java
@@ -157,7 +157,7 @@ public void doFilter( ServletRequest servletRequest, ServletResponse servletResp
     private LoginContext authenticate( String username, String password ) throws InvalidAuthTokenException
     {
         AuthManager authManager = authManagerSupplier.get();
-        Map<String,Object> authToken = newBasicAuthToken( username, UTF8.encode( password ) );
+        Map<String,Object> authToken = newBasicAuthToken( username, password != null ? UTF8.encode( password ) : null );
         return authManager.login( authToken );
     }
 

diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/LdapRealm.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/LdapRealm.java
@@ -338,7 +338,7 @@ protected AuthenticationInfo createAuthenticationInfo( AuthenticationToken token
 
         if ( isAuthenticationCachingEnabled() )
         {
-            SimpleHash hashedCredentials = secureHasher.hash( ((String) token.getCredentials()).getBytes() );
+            SimpleHash hashedCredentials = secureHasher.hash( (byte[]) token.getCredentials() );
             return new ShiroAuthenticationInfo( token.getPrincipal(), hashedCredentials.getBytes(),
                     hashedCredentials.getSalt(), getName(), AuthenticationResult.SUCCESS );
         }

diff --git a/...ity/src/main/java/org/neo4j/server/security/enterprise/auth/UserManagementProcedures.java b/...ity/src/main/java/org/neo4j/server/security/enterprise/auth/UserManagementProcedures.java
@@ -47,7 +47,7 @@ public void createUser( @Name( "username" ) String username, @Name( "password" )
             throws InvalidArgumentsException, IOException
     {
         // TODO: Deprecate this and create a new procedure that takes password as a byte[]
-        userManager.newUser( username, UTF8.encode( password ), requirePasswordChange );
+        userManager.newUser( username, password != null ? UTF8.encode( password ) : null, requirePasswordChange );
     }
 
     @Deprecated
@@ -191,7 +191,7 @@ public void deleteRole( @Name( "roleName" ) String roleName ) throws InvalidArgu
     private void setUserPassword( String username, String newPassword, boolean requirePasswordChange )
             throws IOException, InvalidArgumentsException
     {
-        userManager.setUserPassword( username, UTF8.encode( newPassword ), requirePasswordChange );
+        userManager.setUserPassword( username, newPassword != null ? UTF8.encode( newPassword ) : null, requirePasswordChange );
         if ( securityContext.subject().hasUsername( username ) )
         {
             securityContext.subject().setPasswordChangeNoLongerRequired();