From 23b83d6a387522bf8c8a4ff3a24d219b3230a08f Mon Sep 17 00:00:00 2001 From: BenjaminAmos <24301287+BenjaminAmos@users.noreply.github.com> Date: Sun, 3 Mar 2024 07:49:35 +0000 Subject: [PATCH] feat: prevent serialisation of private component fields (#5208) * feat: prevent serialisation of private component fields * log line shorter for ComponentTypeHandlerFactory * feat(TypeHandlerLibrary): do not serialise private fields in ObjectFieldMapTypeHandler by default * fix: fix ObjectFieldMapTypeHandlerFactoryTest tests * fix: fix TypeSerializerTest tests * fix: fix VectorTypeSerializerTest tests * fix: fix BlockAreaTypeHandlerTest tests * style: fix checkstyle issues in TypeSerializerTest.java * fix: catch setter errors explicitly in ObjectFieldTypeMapHandler --------- Co-authored-by: soloturn Co-authored-by: jdrueckert --- .../serializers/TypeSerializerTest.java | 32 +++++++++---------- .../serializers/VectorTypeSerializerTest.java | 4 +-- .../mathTypes/BlockAreaTypeHandlerTest.java | 2 +- .../coreTypes/ObjectFieldMapTypeHandler.java | 24 ++++++++++++-- .../ObjectFieldMapTypeHandlerFactory.java | 19 ++++++++++- .../ObjectFieldMapTypeHandlerFactoryTest.java | 10 +++--- 6 files changed, 63 insertions(+), 28 deletions(-) diff --git a/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/TypeSerializerTest.java b/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/TypeSerializerTest.java index dcdfa70494a..87e6cba2d6c 100644 --- a/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/TypeSerializerTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/TypeSerializerTest.java @@ -110,14 +110,14 @@ void testJsonSerializeDeserialize() throws IOException { assertEquals(INSTANCE, deserializedInstance); } - private static final class SomeClass { + public static final class SomeClass { @SerializedName("generic-t") - private T data; - private final List list = Lists.newArrayList(); - private final Set> animals = Sets.newHashSet(); - private Animal singleAnimal; + public T data; + public List list = Lists.newArrayList(); + public Set> animals = Sets.newHashSet(); + public Animal singleAnimal; - private SomeClass(T data) { + SomeClass(T data) { this.data = data; } @@ -151,10 +151,10 @@ public String toString() { } @SuppressWarnings("checkstyle:FinalClass") - private static class Animal { - protected final T data; + public static class Animal { + public T data; - private Animal(T data) { + Animal(T data) { this.data = data; } @@ -176,11 +176,11 @@ public int hashCode() { } } - private static final class Dog extends Animal { - private final Vector3f tailPosition; - private final Vector3f headPosition; + public static final class Dog extends Animal { + public Vector3f tailPosition; + public Vector3f headPosition; - private Dog(T data, Vector3f tailPosition, Vector3f headPosition) { + Dog(T data, Vector3f tailPosition, Vector3f headPosition) { super(data); this.tailPosition = tailPosition; this.headPosition = headPosition; @@ -216,10 +216,10 @@ public int hashCode() { } } - private static final class Cheetah extends Animal { - private final Color spotColor; + public static final class Cheetah extends Animal { + public Color spotColor; - private Cheetah(T data, Color spotColor) { + Cheetah(T data, Color spotColor) { super(data); this.spotColor = spotColor; } diff --git a/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/VectorTypeSerializerTest.java b/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/VectorTypeSerializerTest.java index 0ad30391fb7..86e78c66440 100644 --- a/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/VectorTypeSerializerTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/VectorTypeSerializerTest.java @@ -59,13 +59,13 @@ void testSerializationConstant() { assertEquals(new Vector2f(1.0f, 2.0f), o.v3, .00001f); } - static class TestObject { + public static class TestObject { public Vector3f v1; public Vector2f v2; public Vector4f v3; } - static class TestObject2 { + public static class TestObject2 { public Vector3fc v1; public Vector4fc v2; public Vector2fc v3; diff --git a/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/BlockAreaTypeHandlerTest.java b/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/BlockAreaTypeHandlerTest.java index 15f88346aa5..973c488beec 100644 --- a/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/BlockAreaTypeHandlerTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/BlockAreaTypeHandlerTest.java @@ -55,7 +55,7 @@ public void testGsonSerialization() throws IOException { assertEquals(new BlockArea(0, 0, 1, 1), o.b2); } - static class TestObject { + public static class TestObject { public BlockArea b1; public BlockAreac b2; } diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java index 8cf6faa9944..e1f51f8d093 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java +++ b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java @@ -12,9 +12,12 @@ import org.terasology.persistence.typeHandling.TypeHandler; import org.terasology.persistence.typeHandling.TypeHandlerLibrary; import org.terasology.persistence.typeHandling.annotations.SerializedName; +import org.terasology.reflection.ReflectionUtil; import org.terasology.reflection.reflect.ObjectConstructor; import java.lang.reflect.Field; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Modifier; import java.util.Map; import java.util.Objects; import java.util.Optional; @@ -48,10 +51,17 @@ public PersistedData serializeNonNull(T value, PersistedDataSerializer serialize Object val; try { - val = field.get(value); + if (Modifier.isPrivate(field.getModifiers())) { + val = ReflectionUtil.findGetter(field).invoke(value); + } else { + val = field.get(value); + } } catch (IllegalAccessException e) { logger.error("Field {} is inaccessible", field); continue; + } catch (InvocationTargetException e) { + logger.error("Failed to invoke getter for field {}", field); + continue; } if (!Objects.equals(val, Defaults.defaultValue(field.getType()))) { @@ -93,7 +103,7 @@ public Optional deserialize(PersistedData data) { Field field = fieldByName.get(fieldName); if (field == null) { - logger.error("Cound not find field with name {}", fieldName); + logger.error("Could not find field with name {}", fieldName); continue; } @@ -101,7 +111,15 @@ public Optional deserialize(PersistedData data) { Optional fieldValue = handler.deserialize(entry.getValue()); if (fieldValue.isPresent()) { - field.set(result, fieldValue.get()); + if (Modifier.isPrivate(field.getModifiers())) { + try { + ReflectionUtil.findSetter(field).invoke(result); + } catch (InvocationTargetException e) { + logger.error("Failed to invoke setter for field {}", field); + } + } else { + field.set(result, fieldValue.get()); + } } else { logger.error("Could not deserialize field {}", field.getName()); } diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactory.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactory.java index 488f6404997..ea9a653bd2d 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactory.java +++ b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactory.java @@ -3,6 +3,8 @@ package org.terasology.persistence.typeHandling.coreTypes.factories; import com.google.common.collect.Maps; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.terasology.persistence.typeHandling.TypeHandler; import org.terasology.persistence.typeHandling.TypeHandlerContext; import org.terasology.persistence.typeHandling.TypeHandlerFactory; @@ -17,10 +19,14 @@ import java.lang.reflect.Type; import java.security.AccessController; import java.security.PrivilegedAction; +import java.util.Arrays; +import java.util.List; import java.util.Map; import java.util.Optional; public class ObjectFieldMapTypeHandlerFactory implements TypeHandlerFactory { + private static final Logger logger = LoggerFactory.getLogger(ObjectFieldMapTypeHandlerFactory.class); + private static final List PRIVATE_OVERRIDE_ANNOTATIONS = List.of("org.terasology.nui.LayoutConfig"); private ConstructorLibrary constructorLibrary; @@ -77,7 +83,18 @@ private Map getResolvedFields(TypeInfo typeInfo) { continue; } - field.setAccessible(true); + if (Arrays.stream(field.getAnnotations()) + .anyMatch(annotation -> PRIVATE_OVERRIDE_ANNOTATIONS.contains(annotation.getClass().getCanonicalName()))) { + field.setAccessible(true); + } else { + if (Modifier.isPrivate(field.getModifiers()) && ReflectionUtil.findGetter(field) == null + && ReflectionUtil.findSetter(field) == null) { + logger.atWarn().addArgument(field.getName()).addArgument(rawType.getTypeName()). + log("Field {}#{} will not be serialised. Terasology no longer supports serialising private fields."); + continue; + } + } + Type fieldType = ReflectionUtil.resolveType(type, field.getGenericType()); fields.put(field, fieldType); } diff --git a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java b/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java index 738dbfb45b5..45f40b575de 100644 --- a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java +++ b/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java @@ -105,12 +105,12 @@ public void testMultipleObjectsOfDifferentType() { } private static class SingleTypeClass { - private T t; + public T t; } private static class MultiTypeClass { - private T t; - private U u; + public T t; + public U u; } @edu.umd.cs.findbugs.annotations.SuppressFBWarnings( @@ -119,7 +119,7 @@ private static class MultiTypeClass { " creation based on member types of input class TypeInfo. ") @SuppressWarnings("PMD.UnusedPrivateField") private static class SomeClass { - private T t; - private List list; + public T t; + public List list; } }