Fix more tests

- Better handling of `null` credentials - Fix credentials handling in plugin cacheable authentication info - Add auth token matcher to test utils
neo4j · Oct 2, 2018 · 2cc0747 · 2cc0747
1 parent fa14b81
commit 2cc0747
Show file tree

Hide file tree

Showing 12 changed files with 130 additions and 37 deletions.
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/security/UserManager.java b/community/kernel/src/main/java/org/neo4j/kernel/api/security/UserManager.java
@@ -56,7 +56,10 @@ void setUserPassword( String username, byte[] password, boolean requirePasswordC
         @Override
         public User newUser( String username, byte[] initialPassword, boolean requirePasswordChange )
         {
-            Arrays.fill( initialPassword, (byte) 0 );
+            if ( initialPassword != null )
+            {
+                Arrays.fill( initialPassword, (byte) 0 );
+            }
             return null;
         }
 
@@ -81,7 +84,10 @@ public User silentlyGetUser( String username )
         @Override
         public void setUserPassword( String username, byte[] password, boolean requirePasswordChange )
         {
-            Arrays.fill( password, (byte) 0 );
+            if ( password != null )
+            {
+                Arrays.fill( password, (byte) 0 );
+            }
         }
 
         @Override

diff --git a/community/kernel/src/test/java/org/neo4j/test/AuthTokenUtil.java b/community/kernel/src/test/java/org/neo4j/test/AuthTokenUtil.java
@@ -19,6 +19,8 @@
  */
 package org.neo4j.test;
 
+import org.hamcrest.BaseMatcher;
+import org.hamcrest.Description;
 import org.mockito.ArgumentMatcher;
 
 import java.io.Serializable;
@@ -35,13 +37,21 @@
 
 public class AuthTokenUtil
 {
-    public static boolean matches( Map<String,Object> expected, Map<String,Object> actual )
+    @SuppressWarnings( "unchecked" )
+    public static boolean matches( Map<String,Object> expected, Object actualObject )
     {
-        if ( expected == null || actual == null )
+        if ( expected == null || actualObject == null )
         {
-            return expected == actual;
+            return expected == actualObject;
         }
 
+        if ( !(actualObject instanceof Map<?,?>) )
+        {
+            return false;
+        }
+
+        Map<String,Object> actual = (Map<String,Object>) actualObject;
+
         if ( expected.size() != actual.size() )
         {
             return false;
@@ -61,9 +71,9 @@ public static boolean matches( Map<String,Object> expected, Map<String,Object> a
                     return false;
                 }
             }
-            else if ( expectedValue == null ^ actualValue == null )
+            else if ( expectedValue == null || actualValue == null )
             {
-                return false;
+                return expectedValue == actualValue;
             }
             else if ( !expectedValue.equals( actualValue ) )
             {
@@ -92,6 +102,33 @@ public static void assertAuthTokenMatches( Map<String,Object> expected, Map<Stri
         } );
     }
 
+    public static class AuthTokenMatcher extends BaseMatcher<Map<String,Object>>
+    {
+        private final Map<String,Object> expectedValue;
+
+        public AuthTokenMatcher( Map<String,Object> expectedValue )
+        {
+            this.expectedValue = expectedValue;
+        }
+
+        @Override
+        public boolean matches( Object o )
+        {
+            return AuthTokenUtil.matches( expectedValue, o );
+        }
+
+        @Override
+        public void describeTo( Description description )
+        {
+            description.appendValue( this.expectedValue );
+        }
+    }
+
+    public static AuthTokenMatcher authTokenMatcher( Map<String,Object> authToken )
+    {
+        return new AuthTokenMatcher( authToken );
+    }
+
     public static class AuthTokenArgumentMatcher implements ArgumentMatcher<Map<String,Object>>, Serializable
     {
 
@@ -109,11 +146,11 @@ public boolean matches( Map<String,Object> actual )
 
         public String toString()
         {
-            return "authTokenMatcher(" + wanted + ")";
+            return "authTokenArgumentMatcher(" + wanted + ")";
         }
     }
 
-    public static Map<String,Object> authTokenMatcher( Map<String,Object> authToken )
+    public static Map<String,Object> authTokenArgumentMatcher( Map<String,Object> authToken )
     {
         mockingProgress().getArgumentMatcherStorage().reportMatcher( new AuthTokenArgumentMatcher( authToken ) );
         return null;

diff --git a/community/security/src/main/java/org/neo4j/server/security/auth/BasicAuthManager.java b/community/security/src/main/java/org/neo4j/server/security/auth/BasicAuthManager.java
@@ -164,7 +164,10 @@ public User newUser( String username, byte[] initialPassword, boolean requirePas
         finally
         {
             // Clear password
-            Arrays.fill( initialPassword, (byte) 0 );
+            if ( initialPassword != null )
+            {
+                Arrays.fill( initialPassword, (byte) 0 );
+            }
         }
     }
 
@@ -222,7 +225,10 @@ public void setUserPassword( String username, byte[] password, boolean requirePa
         finally
         {
             // Clear password
-            Arrays.fill( password, (byte) 0 );
+            if ( password != null )
+            {
+                Arrays.fill( password, (byte) 0 );
+            }
         }
     }
 

diff --git a/community/security/src/test/java/org/neo4j/server/security/auth/BasicAuthManagerTest.java b/community/security/src/test/java/org/neo4j/server/security/auth/BasicAuthManagerTest.java
@@ -396,7 +396,7 @@ protected AuthManager authManager()
 
     public static byte[] password( String passwordString )
     {
-        return passwordString.getBytes( StandardCharsets.UTF_8 );
+        return passwordString != null ? passwordString.getBytes( StandardCharsets.UTF_8 ) : null;
     }
 
     public static byte[] clearedPasswordWithSameLenghtAs( String passwordString )

diff --git a/community/server/src/test/java/org/neo4j/server/rest/dbms/AuthorizationFilterTest.java b/community/server/src/test/java/org/neo4j/server/rest/dbms/AuthorizationFilterTest.java
@@ -52,7 +52,7 @@
 import static org.neo4j.internal.kernel.api.security.LoginContext.AUTH_DISABLED;
 import static org.neo4j.logging.AssertableLogProvider.inLog;
 import static org.neo4j.server.security.auth.SecurityTestUtils.authToken;
-import static org.neo4j.test.AuthTokenUtil.authTokenMatcher;
+import static org.neo4j.test.AuthTokenUtil.authTokenArgumentMatcher;
 
 public class AuthorizationFilterTest
 {
@@ -176,7 +176,7 @@ public void shouldNotAuthorizeInvalidCredentials() throws Exception
         when( servletRequest.getContextPath() ).thenReturn( "/db/data" );
         when( servletRequest.getHeader( HttpHeaders.AUTHORIZATION ) ).thenReturn( "BASIC " + credentials );
         when( servletRequest.getRemoteAddr() ).thenReturn( "remote_ip_address" );
-        when( authManager.login( authTokenMatcher( authToken( "foo", "bar" ) ) ) ).thenReturn( loginContext );
+        when( authManager.login( authTokenArgumentMatcher( authToken( "foo", "bar" ) ) ) ).thenReturn( loginContext );
         when( loginContext.subject() ).thenReturn( authSubject );
         when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.FAILURE );
 
@@ -206,7 +206,7 @@ public void shouldAuthorizeWhenPasswordChangeRequiredForWhitelistedPath() throws
         when( servletRequest.getMethod() ).thenReturn( "GET" );
         when( servletRequest.getContextPath() ).thenReturn( "/user/foo" );
         when( servletRequest.getHeader( HttpHeaders.AUTHORIZATION ) ).thenReturn( "BASIC " + credentials );
-        when( authManager.login( authTokenMatcher( authToken( "foo", "bar" ) ) ) ).thenReturn( loginContext );
+        when( authManager.login( authTokenArgumentMatcher( authToken( "foo", "bar" ) ) ) ).thenReturn( loginContext );
         when( loginContext.subject() ).thenReturn( authSubject );
         when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.PASSWORD_CHANGE_REQUIRED );
 
@@ -231,7 +231,7 @@ public void shouldNotAuthorizeWhenPasswordChangeRequired() throws Exception
         when( servletRequest.getRequestURL() ).thenReturn( new StringBuffer( "http://bar.baz:7474/db/data/" ) );
         when( servletRequest.getRequestURI() ).thenReturn( "/db/data/" );
         when( servletRequest.getHeader( HttpHeaders.AUTHORIZATION ) ).thenReturn( "BASIC " + credentials );
-        when( authManager.login( authTokenMatcher( authToken( "foo", "bar" ) ) ) ).thenReturn( loginContext );
+        when( authManager.login( authTokenArgumentMatcher( authToken( "foo", "bar" ) ) ) ).thenReturn( loginContext );
         when( loginContext.subject() ).thenReturn( authSubject );
         when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.PASSWORD_CHANGE_REQUIRED );
 
@@ -261,7 +261,7 @@ public void shouldNotAuthorizeWhenTooManyAttemptsMade() throws Exception
         when( servletRequest.getMethod() ).thenReturn( "GET" );
         when( servletRequest.getContextPath() ).thenReturn( "/db/data" );
         when( servletRequest.getHeader( HttpHeaders.AUTHORIZATION ) ).thenReturn( "BASIC " + credentials );
-        when( authManager.login( authTokenMatcher( authToken( "foo", "bar" ) ) ) ).thenReturn( loginContext );
+        when( authManager.login( authTokenArgumentMatcher( authToken( "foo", "bar" ) ) ) ).thenReturn( loginContext );
         when( loginContext.subject() ).thenReturn( authSubject );
         when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.TOO_MANY_ATTEMPTS );
 
@@ -290,7 +290,7 @@ public void shouldAuthorizeWhenValidCredentialsSupplied() throws Exception
         when( servletRequest.getMethod() ).thenReturn( "GET" );
         when( servletRequest.getContextPath() ).thenReturn( "/db/data" );
         when( servletRequest.getHeader( HttpHeaders.AUTHORIZATION ) ).thenReturn( "BASIC " + credentials );
-        when( authManager.login( authTokenMatcher( authToken( "foo", "bar" ) ) ) ).thenReturn( loginContext );
+        when( authManager.login( authTokenArgumentMatcher( authToken( "foo", "bar" ) ) ) ).thenReturn( loginContext );
         when( loginContext.subject() ).thenReturn( authSubject );
         when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.SUCCESS );
 

diff --git a/...curity/src/main/java/org/neo4j/server/security/enterprise/auth/EnterpriseUserManager.java b/...curity/src/main/java/org/neo4j/server/security/enterprise/auth/EnterpriseUserManager.java
@@ -145,7 +145,10 @@ public Set<String> silentlyGetUsernamesForRole( String roleName )
         @Override
         public User newUser( String username, byte[] initialPassword, boolean requirePasswordChange )
         {
-            Arrays.fill( initialPassword, (byte) 0 );
+            if ( initialPassword != null )
+            {
+                Arrays.fill( initialPassword, (byte) 0 );
+            }
             return null;
         }
 
@@ -170,7 +173,10 @@ public User silentlyGetUser( String username )
         @Override
         public void setUserPassword( String username, byte[] password, boolean requirePasswordChange )
         {
-            Arrays.fill( password, (byte) 0 );
+            if ( password != null )
+            {
+                Arrays.fill( password, (byte) 0 );
+            }
         }
 
         @Override

diff --git a/...curity/src/main/java/org/neo4j/server/security/enterprise/auth/InternalFlatFileRealm.java b/...curity/src/main/java/org/neo4j/server/security/enterprise/auth/InternalFlatFileRealm.java
@@ -482,7 +482,10 @@ public User newUser( String username, byte[] initialPassword, boolean requirePas
         finally
         {
             // Clear password
-            Arrays.fill( initialPassword, (byte) 0 );
+            if ( initialPassword != null )
+            {
+                Arrays.fill( initialPassword, (byte) 0 );
+            }
         }
     }
 
@@ -676,7 +679,11 @@ public void setUserPassword( String username, byte[] password, boolean requirePa
         }
         finally
         {
-            Arrays.fill( password, (byte) 0 );
+            // Clear password
+            if ( password != null )
+            {
+                Arrays.fill( password, (byte) 0 );
+            }
         }
     }
 

diff --git a/.../security/src/main/java/org/neo4j/server/security/enterprise/auth/plugin/PluginRealm.java b/.../security/src/main/java/org/neo4j/server/security/enterprise/auth/plugin/PluginRealm.java
@@ -381,7 +381,11 @@ public boolean doCredentialsMatch( AuthenticationToken token, AuthenticationInfo
                     finally
                     {
                         // Clear credentials
-                        Arrays.fill( pluginApiAuthToken.credentials(), (char) 0 );
+                        char[] credentials = pluginApiAuthToken.credentials();
+                        if ( credentials != null )
+                        {
+                            Arrays.fill( credentials, (char) 0 );
+                        }
                     }
                 }
                 catch ( InvalidAuthTokenException e )
@@ -392,8 +396,15 @@ public boolean doCredentialsMatch( AuthenticationToken token, AuthenticationInfo
             else if ( info.getCredentials() != null )
             {
                 // Authentication info is originating from a CacheableAuthenticationInfo or a CacheableAuthInfo
-                return secureHasher.getHashedCredentialsMatcher()
+                PluginShiroAuthToken pluginShiroAuthToken = PluginShiroAuthToken.of( token );
-                        .doCredentialsMatch( PluginShiroAuthToken.of( token ), info );
+                try
+                {
+                    return secureHasher.getHashedCredentialsMatcher().doCredentialsMatch( pluginShiroAuthToken, info );
+                }
+                finally
+                {
+                    pluginShiroAuthToken.clearCredentials();
+                }
             }
             else
             {

diff --git a/.../src/main/java/org/neo4j/server/security/enterprise/auth/plugin/PluginShiroAuthToken.java b/.../src/main/java/org/neo4j/server/security/enterprise/auth/plugin/PluginShiroAuthToken.java
@@ -24,6 +24,9 @@
 
 import org.apache.shiro.authc.AuthenticationToken;
 
+import java.nio.ByteBuffer;
+import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
 import java.util.Map;
 
 import org.neo4j.server.security.enterprise.auth.ShiroAuthToken;
@@ -35,15 +38,28 @@
  */
 public class PluginShiroAuthToken extends ShiroAuthToken
 {
+    private final char[] credentials;
+
     private PluginShiroAuthToken( Map<String,Object> authTokenMap )
     {
         super( authTokenMap );
+        // Convert credentials UTF8 byte[] to char[] (this should not create any intermediate copies)
+        byte[] credentialsBytes = (byte[]) super.getCredentials();
+        credentials = credentialsBytes != null ? StandardCharsets.UTF_8.decode( ByteBuffer.wrap( credentialsBytes ) ).array() : null;
     }
 
     @Override
     public Object getCredentials()
     {
-        return ((String) super.getCredentials()).toCharArray();
+        return credentials;
+    }
+
+    void clearCredentials()
+    {
+        if ( credentials != null )
+        {
+            Arrays.fill( credentials, (char) 0 );
+        }
     }
 
     public static PluginShiroAuthToken of( ShiroAuthToken shiroAuthToken )