fix: Check for equality of value instead of equality of instance. (#1101)

isCustomized returned the wrong value for all instances that have been serialized and read back as they obviously does not point to the default instance.

The solution is to check for equality of the values, not for equality of the instances. This is a minor change in behaviour: A setting that is created with the builder matching the exact same defaults is now considered to be not customized.
This solution has been preferred over a custom readResolve returning the DEFAULT when things match as it would otherwise be able to create an instance that identifies itself as customized but changed to not customized when read back, basically the oppositte of what is fixed here: An instances created as not customized must never be read back as customized.

A couple of tests needed to be adapted due to the fact that the previous test did not actually change the default value.

Author: michael-simons

driver/src/main/java/org/neo4j/driver/internal/SecuritySettings.java

@@ -21,6 +21,7 @@
 import java.io.IOException;
 import java.io.Serializable;
 import java.security.GeneralSecurityException;
+import java.util.Objects;
 
 import org.neo4j.driver.Config;
 import org.neo4j.driver.exceptions.ClientException;
@@ -59,7 +60,20 @@ public Config.TrustStrategy trustStrategy()
 
     private boolean isCustomized()
     {
-        return this != DEFAULT;
+        return !(DEFAULT.encrypted() == this.encrypted() && DEFAULT.hasEqualTrustStrategy( this ));
+    }
+
+    private boolean hasEqualTrustStrategy( SecuritySettings other )
+    {
+        Config.TrustStrategy t1 = this.trustStrategy;
+        Config.TrustStrategy t2 = other.trustStrategy;
+        if ( t1 == t2 )
+        {
+            return true;
+        }
+
+        return t1.isHostnameVerificationEnabled() == t2.isHostnameVerificationEnabled() && t1.strategy() == t2.strategy() &&
+                Objects.equals( t1.certFile(), t2.certFile() ) && t1.revocationStrategy() == t2.revocationStrategy();
     }
 
     public SecurityPlan createSecurityPlan( String uriScheme )

driver/src/test/java/org/neo4j/driver/internal/SecuritySettingsTest.java

@@ -18,21 +18,29 @@
  */
 package org.neo4j.driver.internal;
 
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.platform.commons.support.ReflectionSupport;
 
+import java.io.File;
+import java.io.IOException;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.util.stream.Stream;
 
 import org.neo4j.driver.Config;
 import org.neo4j.driver.exceptions.ClientException;
 import org.neo4j.driver.internal.security.SecurityPlan;
+import org.neo4j.driver.util.TestUtil;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.neo4j.driver.internal.RevocationStrategy.STRICT;
 import static org.neo4j.driver.internal.RevocationStrategy.NO_CHECKS;
+import static org.neo4j.driver.internal.RevocationStrategy.STRICT;
 import static org.neo4j.driver.internal.RevocationStrategy.VERIFY_IF_PRESENT;
 
 class SecuritySettingsTest
@@ -98,7 +106,7 @@ void testSelfSignedCertConfigDisablesHostnameVerification( String scheme ) throw
     void testThrowsOnUserCustomizedEncryption( String scheme )
     {
         SecuritySettings securitySettings = new SecuritySettings.SecuritySettingsBuilder()
-                .withoutEncryption()
+                .withEncryption()
                 .build();
 
         ClientException ex =
@@ -113,7 +121,7 @@ void testThrowsOnUserCustomizedEncryption( String scheme )
     void testThrowsOnUserCustomizedTrustConfiguration( String scheme )
     {
         SecuritySettings securitySettings = new SecuritySettings.SecuritySettingsBuilder()
-                .withTrustStrategy( Config.TrustStrategy.trustSystemCertificates() )
+                .withTrustStrategy( Config.TrustStrategy.trustAllCertificates() )
                 .build();
 
         ClientException ex =
@@ -218,4 +226,102 @@ void testRevocationCheckingDisabledByDefault( String scheme )
         assertEquals( NO_CHECKS, securityPlan.revocationStrategy() );
     }
 
+    @Nested
+    class SerializationTests
+    {
+        Method isCustomized = ReflectionSupport.findMethod( SecuritySettings.class, "isCustomized" ).orElseThrow(
+                () -> new RuntimeException( "This test requires isCustomized to be present." ) );
+
+        boolean isCustomized( SecuritySettings securitySettings )
+        {
+            isCustomized.setAccessible( true );
+            try
+            {
+                return (boolean) isCustomized.invoke( securitySettings );
+            }
+            catch ( IllegalAccessException | InvocationTargetException e )
+            {
+                throw new RuntimeException( e );
+            }
+        }
+
+        @Test
+        void defaultSettingsShouldNotBeCustomizedWhenReadBack() throws IOException, ClassNotFoundException
+        {
+            SecuritySettings securitySettings = new SecuritySettings.SecuritySettingsBuilder().build();
+
+            assertFalse( isCustomized( securitySettings ) );
+
+            SecuritySettings verify = TestUtil.serializeAndReadBack( securitySettings, SecuritySettings.class );
+
+            assertFalse( isCustomized( verify ) );
+        }
+
+        @Test
+        void defaultsShouldBeCheckCorrect() throws IOException, ClassNotFoundException
+        {
+            SecuritySettings securitySettings = new SecuritySettings.SecuritySettingsBuilder().withoutEncryption().withTrustStrategy(
+                    Config.TrustStrategy.trustSystemCertificates() ).build();
+
+            // The settings are still equivalent to the defaults, even if the builder has been used. It is not customized.
+            assertFalse( isCustomized( securitySettings ) );
+
+            SecuritySettings verify = TestUtil.serializeAndReadBack( securitySettings, SecuritySettings.class );
+
+            assertFalse( isCustomized( verify ) );
+        }
+
+        @Test
+        void shouldReadBackChangedEncryption() throws IOException, ClassNotFoundException
+        {
+            SecuritySettings securitySettings =
+                    new SecuritySettings.SecuritySettingsBuilder().withEncryption().withTrustStrategy( Config.TrustStrategy.trustSystemCertificates() ).build();
+
+            assertTrue( isCustomized( securitySettings ) );
+            assertTrue( securitySettings.encrypted() );
+
+            SecuritySettings verify = TestUtil.serializeAndReadBack( securitySettings, SecuritySettings.class );
+
+            assertTrue( isCustomized( verify ) );
+            assertTrue( securitySettings.encrypted() );
+        }
+
+        @Test
+        void shouldReadBackChangedStrategey() throws IOException, ClassNotFoundException
+        {
+            SecuritySettings securitySettings =
+                    new SecuritySettings.SecuritySettingsBuilder().withoutEncryption().withTrustStrategy( Config.TrustStrategy.trustAllCertificates() ).build();
+
+            // The settings are still equivalent to the defaults, even if the builder has been used. It is not customized.
+            assertTrue( isCustomized( securitySettings ) );
+            assertFalse( securitySettings.encrypted() );
+            assertEquals( Config.TrustStrategy.trustAllCertificates().strategy(), securitySettings.trustStrategy().strategy() );
+
+            SecuritySettings verify = TestUtil.serializeAndReadBack( securitySettings, SecuritySettings.class );
+
+            assertTrue( isCustomized( verify ) );
+            assertFalse( securitySettings.encrypted() );
+            assertEquals( Config.TrustStrategy.trustAllCertificates().strategy(), securitySettings.trustStrategy().strategy() );
+        }
+
+        @Test
+        void shouldReadBackChangedCertFile() throws IOException, ClassNotFoundException
+        {
+            SecuritySettings securitySettings = new SecuritySettings.SecuritySettingsBuilder().withoutEncryption().withTrustStrategy(
+                    Config.TrustStrategy.trustCustomCertificateSignedBy( new File( "some.cert" ) ) ).build();
+
+            // The settings are still equivalent to the defaults, even if the builder has been used. It is not customized.
+            assertTrue( isCustomized( securitySettings ) );
+            assertFalse( securitySettings.encrypted() );
+            assertEquals( Config.TrustStrategy.trustCustomCertificateSignedBy( new File( "some.cert" ) ).strategy(),
+                    securitySettings.trustStrategy().strategy() );
+
+            SecuritySettings verify = TestUtil.serializeAndReadBack( securitySettings, SecuritySettings.class );
+
+            assertTrue( isCustomized( verify ) );
+            assertFalse( securitySettings.encrypted() );
+            assertEquals( Config.TrustStrategy.trustCustomCertificateSignedBy( new File( "some.cert" ) ).strategy(),
+                    securitySettings.trustStrategy().strategy() );
+        }
+    }
 }