From 908399e2716a5041932f70993a8005a1553f8630 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sat, 30 Apr 2016 10:44:32 +0200 Subject: [PATCH] #432 Injector - prevent static field injection, add more tests --- src/main/java/fr/xephi/authme/AuthMe.java | 4 +- .../AuthMeServiceInitializer.java | 15 ---- .../authme/initialization/BaseCommands.java | 2 +- .../initialization/ConstructorInjection.java | 11 ++- .../authme/initialization/DataFolder.java | 2 +- .../authme/initialization/FieldInjection.java | 33 +++++--- .../AuthMeServiceInitializerTest.java | 22 +++-- .../ConstructorInjectionTest.java | 81 +++++++++++++++++++ .../initialization/FieldInjectionTest.java | 61 ++++++++++++++ .../samples/CircularClasses.java | 2 +- .../initialization/samples/GammaService.java | 2 +- .../samples/InvalidPostConstruct.java | 2 +- .../samples/InvalidStaticFieldInjection.java | 17 ++++ .../samples/PostConstructTestClass.java | 2 +- 14 files changed, 211 insertions(+), 45 deletions(-) create mode 100644 src/test/java/fr/xephi/authme/initialization/ConstructorInjectionTest.java create mode 100644 src/test/java/fr/xephi/authme/initialization/samples/InvalidStaticFieldInjection.java diff --git a/src/main/java/fr/xephi/authme/AuthMe.java b/src/main/java/fr/xephi/authme/AuthMe.java index b69cd2990..b240edae3 100644 --- a/src/main/java/fr/xephi/authme/AuthMe.java +++ b/src/main/java/fr/xephi/authme/AuthMe.java @@ -253,8 +253,8 @@ public class AuthMe extends JavaPlugin { initializer.provide(DataFolder.class, getDataFolder()); // Register elements we instantiate manually - initializer.register(newSettings); - initializer.register(messages); + initializer.register(NewSetting.class, newSettings); + initializer.register(Messages.class, messages); initializer.register(DataSource.class, database); initializer.provide(BaseCommands.class, CommandInitializer.buildCommands(initializer)); diff --git a/src/main/java/fr/xephi/authme/initialization/AuthMeServiceInitializer.java b/src/main/java/fr/xephi/authme/initialization/AuthMeServiceInitializer.java index 560596b7d..576ea2277 100644 --- a/src/main/java/fr/xephi/authme/initialization/AuthMeServiceInitializer.java +++ b/src/main/java/fr/xephi/authme/initialization/AuthMeServiceInitializer.java @@ -9,7 +9,6 @@ import java.lang.annotation.Annotation; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Modifier; -import java.lang.reflect.Type; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -51,20 +50,6 @@ public class AuthMeServiceInitializer { return get(clazz, new HashSet>()); } - /** - * Registers an instantiation by its type. - * - * @param object the object to register - * @throws IllegalStateException if an object of the same type has already been registered - */ - public void register(Object object) { - if (object instanceof Type) { - throw new IllegalStateException("You tried to register a Type object: '" + object - + "'. This likely indicates an error. Please use register(Class, T) if really desired."); - } - storeObject(object); - } - /** * Register an object with a custom class (supertype). Use this for example to specify a * concrete implementation of an interface or an abstract class. diff --git a/src/main/java/fr/xephi/authme/initialization/BaseCommands.java b/src/main/java/fr/xephi/authme/initialization/BaseCommands.java index 156cb947c..8ff263abd 100644 --- a/src/main/java/fr/xephi/authme/initialization/BaseCommands.java +++ b/src/main/java/fr/xephi/authme/initialization/BaseCommands.java @@ -8,7 +8,7 @@ import java.lang.annotation.Target; /** * Annotation to denote the collection of AuthMe commands. */ -@Target(ElementType.PARAMETER) +@Target({ElementType.PARAMETER, ElementType.FIELD}) @Retention(RetentionPolicy.RUNTIME) public @interface BaseCommands { } diff --git a/src/main/java/fr/xephi/authme/initialization/ConstructorInjection.java b/src/main/java/fr/xephi/authme/initialization/ConstructorInjection.java index f77ae58cd..e80ea128c 100644 --- a/src/main/java/fr/xephi/authme/initialization/ConstructorInjection.java +++ b/src/main/java/fr/xephi/authme/initialization/ConstructorInjection.java @@ -1,5 +1,7 @@ package fr.xephi.authme.initialization; +import com.google.common.base.Preconditions; + import javax.inject.Inject; import javax.inject.Provider; import java.lang.annotation.Annotation; @@ -36,6 +38,7 @@ class ConstructorInjection implements Injection { @Override public T instantiateWith(Object... values) { + validateNoNullValues(values); try { return constructor.newInstance(values); } catch (InstantiationException | IllegalAccessException | InvocationTargetException e) { @@ -60,7 +63,7 @@ class ConstructorInjection implements Injection { * * @param clazz the class to process * @param the class' type - * @return injection constructor for the class + * @return injection constructor for the class, null if not applicable */ @SuppressWarnings("unchecked") private static Constructor getInjectionConstructor(Class clazz) { @@ -74,4 +77,10 @@ class ConstructorInjection implements Injection { return null; } + private static void validateNoNullValues(Object[] array) { + for (Object entry : array) { + Preconditions.checkNotNull(entry); + } + } + } diff --git a/src/main/java/fr/xephi/authme/initialization/DataFolder.java b/src/main/java/fr/xephi/authme/initialization/DataFolder.java index 33b879977..0288f45a7 100644 --- a/src/main/java/fr/xephi/authme/initialization/DataFolder.java +++ b/src/main/java/fr/xephi/authme/initialization/DataFolder.java @@ -8,7 +8,7 @@ import java.lang.annotation.Target; /** * Annotation for specifying the plugin's data folder. */ -@Target(ElementType.PARAMETER) +@Target({ElementType.PARAMETER, ElementType.FIELD}) @Retention(RetentionPolicy.RUNTIME) public @interface DataFolder { } diff --git a/src/main/java/fr/xephi/authme/initialization/FieldInjection.java b/src/main/java/fr/xephi/authme/initialization/FieldInjection.java index aafb0bde3..1e7973e23 100644 --- a/src/main/java/fr/xephi/authme/initialization/FieldInjection.java +++ b/src/main/java/fr/xephi/authme/initialization/FieldInjection.java @@ -8,6 +8,7 @@ import java.lang.annotation.Annotation; import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -57,6 +58,7 @@ class FieldInjection implements Injection { for (int i = 0; i < fields.length; ++i) { try { + Preconditions.checkNotNull(values[i]); fields[i].set(instance, values[i]); } catch (IllegalAccessException e) { throw new UnsupportedOperationException(e); @@ -65,15 +67,15 @@ class FieldInjection implements Injection { return instance; } - private static Class getFirstNonInjectAnnotation(Field field) { - for (Annotation annotation : field.getAnnotations()) { - if (annotation.annotationType() != Inject.class) { - return annotation.annotationType(); - } - } - return null; - } - + /** + * Returns a provider for a {@code FieldInjection} instance, i.e. a provides an object + * with which field injection can be performed on the given class if applicable. The provided + * value is {@code null} if field injection cannot be applied to the class. + * + * @param clazz the class to provide field injection for + * @param the class' type + * @return field injection provider for the given class + */ public static Provider> provide(final Class clazz) { return new Provider>() { @Override @@ -92,6 +94,10 @@ class FieldInjection implements Injection { List fields = new ArrayList<>(); for (Field field : clazz.getDeclaredFields()) { if (field.isAnnotationPresent(Inject.class)) { + if (Modifier.isStatic(field.getModifiers())) { + throw new IllegalStateException(String.format("Field '%s' in class '%s' is static but " + + "annotated with @Inject", field.getName(), clazz.getSimpleName())); + } field.setAccessible(true); fields.add(field); } @@ -99,6 +105,15 @@ class FieldInjection implements Injection { return fields; } + private static Class getFirstNonInjectAnnotation(Field field) { + for (Annotation annotation : field.getAnnotations()) { + if (annotation.annotationType() != Inject.class) { + return annotation.annotationType(); + } + } + return null; + } + private static Constructor getDefaultConstructor(Class clazz) { try { Constructor defaultConstructor = clazz.getDeclaredConstructor(); diff --git a/src/test/java/fr/xephi/authme/initialization/AuthMeServiceInitializerTest.java b/src/test/java/fr/xephi/authme/initialization/AuthMeServiceInitializerTest.java index 54789ef02..0541c7049 100644 --- a/src/test/java/fr/xephi/authme/initialization/AuthMeServiceInitializerTest.java +++ b/src/test/java/fr/xephi/authme/initialization/AuthMeServiceInitializerTest.java @@ -10,6 +10,7 @@ import fr.xephi.authme.initialization.samples.Duration; import fr.xephi.authme.initialization.samples.FieldInjectionWithAnnotations; import fr.xephi.authme.initialization.samples.InvalidClass; import fr.xephi.authme.initialization.samples.InvalidPostConstruct; +import fr.xephi.authme.initialization.samples.InvalidStaticFieldInjection; import fr.xephi.authme.initialization.samples.PostConstructTestClass; import fr.xephi.authme.initialization.samples.ProvidedClass; import fr.xephi.authme.initialization.samples.Size; @@ -34,7 +35,7 @@ public class AuthMeServiceInitializerTest { @Before public void setInitializer() { initializer = new AuthMeServiceInitializer(ALLOWED_PACKAGE); - initializer.register(new ProvidedClass("")); + initializer.register(ProvidedClass.class, new ProvidedClass("")); } @Test @@ -128,7 +129,7 @@ public class AuthMeServiceInitializerTest { @Test(expected = RuntimeException.class) public void shouldThrowForSecondRegistration() { // given / when / then - initializer.register(new ProvidedClass("")); + initializer.register(ProvidedClass.class, new ProvidedClass("")); } @Test(expected = RuntimeException.class) @@ -152,15 +153,6 @@ public class AuthMeServiceInitializerTest { initializer.register(String.class, null); } - @Test(expected = RuntimeException.class) - public void shouldThrowForRegisterOfType() { - // given / when / then - // this most likely means that the second argument was forgotten, so throw an error and force - // the API user to use the explicit register(Class.class, String.class) if really, really desired - // (Though for such generic types, an annotation would be a lot better) - initializer.register(String.class); - } - @Test public void shouldExecutePostConstructMethod() { // given @@ -215,7 +207,7 @@ public class AuthMeServiceInitializerTest { @Test(expected = RuntimeException.class) public void shouldThrowForAlreadyRegisteredClass() { // given - initializer.register(new BetaManager()); + initializer.register(BetaManager.class, new BetaManager()); // when / then initializer.register(BetaManager.class, new BetaManager()); @@ -233,4 +225,10 @@ public class AuthMeServiceInitializerTest { assertThat(singletonScoped, not(sameInstance(requestScoped))); } + @Test(expected = RuntimeException.class) + public void shouldThrowForStaticFieldInjection() { + // given / when / then + initializer.newInstance(InvalidStaticFieldInjection.class); + } + } diff --git a/src/test/java/fr/xephi/authme/initialization/ConstructorInjectionTest.java b/src/test/java/fr/xephi/authme/initialization/ConstructorInjectionTest.java new file mode 100644 index 000000000..dd21a5903 --- /dev/null +++ b/src/test/java/fr/xephi/authme/initialization/ConstructorInjectionTest.java @@ -0,0 +1,81 @@ +package fr.xephi.authme.initialization; + +import fr.xephi.authme.initialization.samples.AlphaService; +import fr.xephi.authme.initialization.samples.ClassWithAnnotations; +import fr.xephi.authme.initialization.samples.Duration; +import fr.xephi.authme.initialization.samples.GammaService; +import fr.xephi.authme.initialization.samples.InvalidClass; +import fr.xephi.authme.initialization.samples.ProvidedClass; +import fr.xephi.authme.initialization.samples.Size; +import org.junit.Test; + +import static org.hamcrest.Matchers.arrayContaining; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; +import static org.junit.Assert.assertThat; + +/** + * Test for {@link ConstructorInjection}. + */ +public class ConstructorInjectionTest { + + @Test + public void shouldReturnDependencies() { + // given + Injection injection = ConstructorInjection.provide(ClassWithAnnotations.class).get(); + + // when + Class[] dependencies = injection.getDependencies(); + Class[] annotations = injection.getDependencyAnnotations(); + + // then + assertThat(dependencies, arrayContaining(int.class, GammaService.class, long.class)); + assertThat(annotations, arrayContaining((Class) Size.class, null, Duration.class)); + } + + @Test + public void shouldInstantiate() { + // given + GammaService gammaService = new GammaService( + AlphaService.newInstance(new ProvidedClass(""))); + Injection injection = ConstructorInjection.provide(ClassWithAnnotations.class).get(); + + // when + ClassWithAnnotations instance = injection.instantiateWith(-112, gammaService, 19L); + + // then + assertThat(instance, not(nullValue())); + assertThat(instance.getSize(), equalTo(-112)); + assertThat(instance.getGammaService(), equalTo(gammaService)); + assertThat(instance.getDuration(), equalTo(19L)); + } + + @Test(expected = NullPointerException.class) + public void shouldThrowForNullValue() { + // given + Injection injection = ConstructorInjection.provide(ClassWithAnnotations.class).get(); + + // when / then + injection.instantiateWith(-112, null, 12L); + } + + @Test(expected = RuntimeException.class) + public void shouldThrowUponInstantiationError() { + // given + AlphaService alphaService = AlphaService.newInstance(new ProvidedClass("")); + Injection injection = ConstructorInjection.provide(InvalidClass.class).get(); + + // when + injection.instantiateWith(alphaService, 5); + } + + @Test + public void shouldReturnNullForNoConstructorInjection() { + // given / when + Injection injection = ConstructorInjection.provide(FieldInjection.class).get(); + + // then + assertThat(injection, nullValue()); + } +} diff --git a/src/test/java/fr/xephi/authme/initialization/FieldInjectionTest.java b/src/test/java/fr/xephi/authme/initialization/FieldInjectionTest.java index 5f0b45c20..9d2294ae7 100644 --- a/src/test/java/fr/xephi/authme/initialization/FieldInjectionTest.java +++ b/src/test/java/fr/xephi/authme/initialization/FieldInjectionTest.java @@ -1,15 +1,19 @@ package fr.xephi.authme.initialization; import fr.xephi.authme.initialization.samples.AlphaService; +import fr.xephi.authme.initialization.samples.BadFieldInjection; import fr.xephi.authme.initialization.samples.BetaManager; import fr.xephi.authme.initialization.samples.ClassWithAnnotations; import fr.xephi.authme.initialization.samples.Duration; import fr.xephi.authme.initialization.samples.FieldInjectionWithAnnotations; import fr.xephi.authme.initialization.samples.GammaService; +import fr.xephi.authme.initialization.samples.InvalidStaticFieldInjection; import fr.xephi.authme.initialization.samples.ProvidedClass; import fr.xephi.authme.initialization.samples.Size; import org.junit.Test; +import javax.inject.Inject; + import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; @@ -51,4 +55,61 @@ public class FieldInjectionTest { assertThat(betaManager.getDependencies(), arrayContaining(providedClass, gammaService, alphaService)); } + @Test + public void shouldProvideNullForImpossibleFieldInjection() { + // given / when + FieldInjection injection = FieldInjection.provide(BadFieldInjection.class).get(); + + // then + assertThat(injection, nullValue()); + } + + @Test(expected = RuntimeException.class) + public void shouldForwardExceptionDuringInstantiation() { + // given + FieldInjection injection = FieldInjection.provide(ThrowingConstructor.class).get(); + + // when / when + injection.instantiateWith(new ProvidedClass("")); + } + + @Test(expected = RuntimeException.class) + public void shouldThrowForInvalidFieldValue() { + // given + ProvidedClass providedClass = new ProvidedClass(""); + AlphaService alphaService = AlphaService.newInstance(providedClass); + GammaService gammaService = new GammaService(alphaService); + FieldInjection injection = FieldInjection.provide(BetaManager.class).get(); + + // when / then + // Correct order is provided, gamma, alpha + injection.instantiateWith(providedClass, alphaService, gammaService); + } + + @Test(expected = NullPointerException.class) + public void shouldThrowForNullValue() { + // given + ProvidedClass providedClass = new ProvidedClass(""); + AlphaService alphaService = AlphaService.newInstance(providedClass); + FieldInjection injection = FieldInjection.provide(BetaManager.class).get(); + + // when / then + // Correct order is provided, gamma, alpha + injection.instantiateWith(providedClass, null, alphaService); + } + + @Test(expected = RuntimeException.class) + public void shouldThrowForStaticFieldInjection() { + // given / when / then + FieldInjection.provide(InvalidStaticFieldInjection.class).get(); + } + + private static class ThrowingConstructor { + @Inject + private ProvidedClass providedClass; + + public ThrowingConstructor() { + throw new UnsupportedOperationException("Exception in constructor"); + } + } } diff --git a/src/test/java/fr/xephi/authme/initialization/samples/CircularClasses.java b/src/test/java/fr/xephi/authme/initialization/samples/CircularClasses.java index e9a1460ea..dc2313e35 100644 --- a/src/test/java/fr/xephi/authme/initialization/samples/CircularClasses.java +++ b/src/test/java/fr/xephi/authme/initialization/samples/CircularClasses.java @@ -5,7 +5,7 @@ import javax.inject.Inject; /** * Classes with circular dependencies. */ -public class CircularClasses { +public abstract class CircularClasses { public static final class Circular1 { @Inject diff --git a/src/test/java/fr/xephi/authme/initialization/samples/GammaService.java b/src/test/java/fr/xephi/authme/initialization/samples/GammaService.java index 9f9dd2297..9d6f9bfac 100644 --- a/src/test/java/fr/xephi/authme/initialization/samples/GammaService.java +++ b/src/test/java/fr/xephi/authme/initialization/samples/GammaService.java @@ -3,7 +3,7 @@ package fr.xephi.authme.initialization.samples; import javax.inject.Inject; /** - * Sample - class dependent on alpha and provided. + * Sample - class dependent on alpha service. */ public class GammaService { diff --git a/src/test/java/fr/xephi/authme/initialization/samples/InvalidPostConstruct.java b/src/test/java/fr/xephi/authme/initialization/samples/InvalidPostConstruct.java index c80a0c6cd..e97c4f984 100644 --- a/src/test/java/fr/xephi/authme/initialization/samples/InvalidPostConstruct.java +++ b/src/test/java/fr/xephi/authme/initialization/samples/InvalidPostConstruct.java @@ -6,7 +6,7 @@ import javax.inject.Inject; /** * Class with invalid @PostConstruct method. */ -public class InvalidPostConstruct { +public abstract class InvalidPostConstruct { public static final class WithParams { @Inject diff --git a/src/test/java/fr/xephi/authme/initialization/samples/InvalidStaticFieldInjection.java b/src/test/java/fr/xephi/authme/initialization/samples/InvalidStaticFieldInjection.java new file mode 100644 index 000000000..199b2f4a9 --- /dev/null +++ b/src/test/java/fr/xephi/authme/initialization/samples/InvalidStaticFieldInjection.java @@ -0,0 +1,17 @@ +package fr.xephi.authme.initialization.samples; + +import javax.inject.Inject; + +/** + * Sample class - attempted field injection on a static member. + */ +public class InvalidStaticFieldInjection { + + @Inject + private ProvidedClass providedClass; + @Inject + protected static AlphaService alphaService; + + InvalidStaticFieldInjection() { } + +} diff --git a/src/test/java/fr/xephi/authme/initialization/samples/PostConstructTestClass.java b/src/test/java/fr/xephi/authme/initialization/samples/PostConstructTestClass.java index a4e570324..0bec84ee3 100644 --- a/src/test/java/fr/xephi/authme/initialization/samples/PostConstructTestClass.java +++ b/src/test/java/fr/xephi/authme/initialization/samples/PostConstructTestClass.java @@ -4,7 +4,7 @@ import javax.annotation.PostConstruct; import javax.inject.Inject; /** - * Sample class for testing the execution of the @PostConstruct method. + * Sample class for testing the execution of @PostConstruct methods. */ public class PostConstructTestClass {