From 7dbf5551c913f2b83ca7b856826afcf6910c1d52 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Tue, 21 Mar 2017 22:59:21 +0100 Subject: [PATCH] Cleanup: avoid injecting Injector directly - Inject SingletonStore to restrict the possible functions - Refactor PasswordSecurityTest to correspond to the usual way of testing --- .../executable/authme/ReloadCommand.java | 15 +-- .../fr/xephi/authme/task/CleanupTask.java | 9 +- .../executable/authme/ReloadCommandTest.java | 35 +++---- .../authme/security/PasswordSecurityTest.java | 97 +++++++++---------- .../fr/xephi/authme/task/CleanupTaskTest.java | 8 +- .../EncryptionMethodInfoGatherer.java | 2 +- 6 files changed, 80 insertions(+), 86 deletions(-) diff --git a/src/main/java/fr/xephi/authme/command/executable/authme/ReloadCommand.java b/src/main/java/fr/xephi/authme/command/executable/authme/ReloadCommand.java index 5a3604f1..af0aaaca 100644 --- a/src/main/java/fr/xephi/authme/command/executable/authme/ReloadCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/authme/ReloadCommand.java @@ -1,12 +1,12 @@ package fr.xephi.authme.command.executable.authme; -import ch.jalu.injector.Injector; import fr.xephi.authme.AuthMe; import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.command.ExecutableCommand; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.initialization.Reloadable; import fr.xephi.authme.initialization.SettingsDependent; +import fr.xephi.authme.initialization.factory.SingletonStore; import fr.xephi.authme.message.MessageKey; import fr.xephi.authme.service.CommonService; import fr.xephi.authme.settings.Settings; @@ -25,9 +25,6 @@ public class ReloadCommand implements ExecutableCommand { @Inject private AuthMe plugin; - @Inject - private Injector injector; - @Inject private Settings settings; @@ -37,6 +34,12 @@ public class ReloadCommand implements ExecutableCommand { @Inject private CommonService commonService; + @Inject + private SingletonStore reloadableStore; + + @Inject + private SingletonStore settingsDependentStore; + @Override public void executeCommand(CommandSender sender, List arguments) { try { @@ -56,10 +59,10 @@ public class ReloadCommand implements ExecutableCommand { } private void performReloadOnServices() { - injector.retrieveAllOfType(Reloadable.class) + reloadableStore.retrieveAllOfType() .forEach(r -> r.reload()); - injector.retrieveAllOfType(SettingsDependent.class) + settingsDependentStore.retrieveAllOfType() .forEach(s -> s.reload(settings)); } } diff --git a/src/main/java/fr/xephi/authme/task/CleanupTask.java b/src/main/java/fr/xephi/authme/task/CleanupTask.java index 1a5bbdd6..f373c3e5 100644 --- a/src/main/java/fr/xephi/authme/task/CleanupTask.java +++ b/src/main/java/fr/xephi/authme/task/CleanupTask.java @@ -1,7 +1,7 @@ package fr.xephi.authme.task; -import ch.jalu.injector.Injector; import fr.xephi.authme.initialization.HasCleanup; +import fr.xephi.authme.initialization.factory.SingletonStore; import org.bukkit.scheduler.BukkitRunnable; import javax.inject.Inject; @@ -12,15 +12,14 @@ import javax.inject.Inject; public class CleanupTask extends BukkitRunnable { @Inject - private Injector injector; + private SingletonStore hasCleanupStore; CleanupTask() { } @Override public void run() { - for (HasCleanup service : injector.retrieveAllOfType(HasCleanup.class)) { - service.performCleanup(); - } + hasCleanupStore.retrieveAllOfType() + .forEach(HasCleanup::performCleanup); } } diff --git a/src/test/java/fr/xephi/authme/command/executable/authme/ReloadCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/authme/ReloadCommandTest.java index 3d9cf354..ad49600f 100644 --- a/src/test/java/fr/xephi/authme/command/executable/authme/ReloadCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/authme/ReloadCommandTest.java @@ -1,12 +1,12 @@ package fr.xephi.authme.command.executable.authme; -import ch.jalu.injector.Injector; import fr.xephi.authme.AuthMe; import fr.xephi.authme.TestHelper; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.datasource.DataSourceType; import fr.xephi.authme.initialization.Reloadable; import fr.xephi.authme.initialization.SettingsDependent; +import fr.xephi.authme.initialization.factory.SingletonStore; import fr.xephi.authme.message.MessageKey; import fr.xephi.authme.output.LogLevel; import fr.xephi.authme.service.CommonService; @@ -23,17 +23,14 @@ import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; import static org.hamcrest.Matchers.containsString; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.hamcrest.MockitoHamcrest.argThat; @@ -49,9 +46,6 @@ public class ReloadCommandTest { @Mock private AuthMe authMe; - @Mock - private Injector injector; - @Mock private Settings settings; @@ -61,6 +55,12 @@ public class ReloadCommandTest { @Mock private CommonService commandService; + @Mock + private SingletonStore reloadableStore; + + @Mock + private SingletonStore settingsDependentStore; + @BeforeClass public static void setUpLogger() { TestHelper.setupLogger(); @@ -83,11 +83,11 @@ public class ReloadCommandTest { mock(Reloadable.class), mock(Reloadable.class), mock(Reloadable.class)); List dependents = Arrays.asList( mock(SettingsDependent.class), mock(SettingsDependent.class)); - given(injector.retrieveAllOfType(Reloadable.class)).willReturn(reloadables); - given(injector.retrieveAllOfType(SettingsDependent.class)).willReturn(dependents); + given(reloadableStore.retrieveAllOfType()).willReturn(reloadables); + given(settingsDependentStore.retrieveAllOfType()).willReturn(dependents); // when - command.executeCommand(sender, Collections.emptyList()); + command.executeCommand(sender, Collections.emptyList()); // then verify(settings).reload(); @@ -99,16 +99,16 @@ public class ReloadCommandTest { public void shouldHandleReloadError() { // given CommandSender sender = mock(CommandSender.class); - doThrow(IllegalStateException.class).when(injector).retrieveAllOfType(Reloadable.class); + doThrow(IllegalStateException.class).when(reloadableStore).retrieveAllOfType(); given(settings.getProperty(DatabaseSettings.BACKEND)).willReturn(DataSourceType.MYSQL); given(dataSource.getType()).willReturn(DataSourceType.MYSQL); // when - command.executeCommand(sender, Collections.emptyList()); + command.executeCommand(sender, Collections.emptyList()); // then verify(settings).reload(); - verify(injector).retrieveAllOfType(Reloadable.class); + verify(reloadableStore).retrieveAllOfType(); verify(sender).sendMessage(argThat(containsString("Error occurred"))); verify(authMe).stopOrUnload(); } @@ -120,15 +120,16 @@ public class ReloadCommandTest { CommandSender sender = mock(CommandSender.class); given(settings.getProperty(DatabaseSettings.BACKEND)).willReturn(DataSourceType.MYSQL); given(dataSource.getType()).willReturn(DataSourceType.SQLITE); - given(injector.retrieveAllOfType(Reloadable.class)).willReturn(new ArrayList()); - given(injector.retrieveAllOfType(SettingsDependent.class)).willReturn(new ArrayList()); + given(reloadableStore.retrieveAllOfType()).willReturn(Collections.emptyList()); + given(settingsDependentStore.retrieveAllOfType()).willReturn(Collections.emptyList()); // when - command.executeCommand(sender, Collections.emptyList()); + command.executeCommand(sender, Collections.emptyList()); // then verify(settings).reload(); - verify(injector, times(2)).retrieveAllOfType(any(Class.class)); + verify(reloadableStore).retrieveAllOfType(); + verify(settingsDependentStore).retrieveAllOfType(); verify(sender).sendMessage(argThat(containsString("cannot change database type"))); } diff --git a/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java b/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java index cebde5da..6bd0b45a 100644 --- a/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java +++ b/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java @@ -2,11 +2,14 @@ package fr.xephi.authme.security; import ch.jalu.injector.Injector; import ch.jalu.injector.InjectorBuilder; +import ch.jalu.injector.testing.BeforeInjecting; +import ch.jalu.injector.testing.DelayedInjectionRunner; +import ch.jalu.injector.testing.InjectDelayed; import fr.xephi.authme.ReflectionTestUtils; import fr.xephi.authme.TestHelper; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.events.PasswordEncryptionEvent; -import fr.xephi.authme.initialization.factory.FactoryDependencyHandler; +import fr.xephi.authme.initialization.factory.Factory; import fr.xephi.authme.security.crypts.EncryptionMethod; import fr.xephi.authme.security.crypts.HashedPassword; import fr.xephi.authme.security.crypts.Joomla; @@ -15,14 +18,12 @@ import fr.xephi.authme.settings.properties.HooksSettings; import fr.xephi.authme.settings.properties.SecuritySettings; import org.bukkit.event.Event; import org.bukkit.plugin.PluginManager; -import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.invocation.InvocationOnMock; -import org.mockito.junit.MockitoJUnitRunner; import org.mockito.stubbing.Answer; import java.util.Collections; @@ -44,10 +45,11 @@ import static org.mockito.hamcrest.MockitoHamcrest.argThat; /** * Test for {@link PasswordSecurity}. */ -@RunWith(MockitoJUnitRunner.class) +@RunWith(DelayedInjectionRunner.class) public class PasswordSecurityTest { - private Injector injector; + @InjectDelayed + private PasswordSecurity passwordSecurity; @Mock private Settings settings; @@ -58,6 +60,9 @@ public class PasswordSecurityTest { @Mock private DataSource dataSource; + @Mock + private Factory hashAlgorithmFactory; + @Mock private EncryptionMethod method; @@ -68,7 +73,7 @@ public class PasswordSecurityTest { TestHelper.setupLogger(); } - @Before + @BeforeInjecting public void setUpMocks() { caughtClassInEvent = null; @@ -85,12 +90,24 @@ public class PasswordSecurityTest { return null; } }).when(pluginManager).callEvent(any(Event.class)); - injector = new InjectorBuilder() - .addHandlers(new FactoryDependencyHandler()) - .addDefaultHandlers("fr.xephi.authme").create(); + + given(settings.getProperty(SecuritySettings.PASSWORD_HASH)).willReturn(HashAlgorithm.BCRYPT); + given(settings.getProperty(SecuritySettings.LEGACY_HASHES)).willReturn(Collections.emptySet()); + given(settings.getProperty(HooksSettings.BCRYPT_LOG2_ROUND)).willReturn(8); + + Injector injector = new InjectorBuilder() + .addDefaultHandlers("fr.xephi.authme.security.crypts") + .create(); injector.register(Settings.class, settings); - injector.register(DataSource.class, dataSource); - injector.register(PluginManager.class, pluginManager); + + given(hashAlgorithmFactory.newInstance(any(Class.class))).willAnswer(invocation -> { + Object o = injector.createIfHasDependencies(invocation.getArgument(0)); + if (o == null) { + throw new IllegalArgumentException("Cannot create object of class '" + invocation.getArgument(0) + + "': missing class that needs to be provided?"); + } + return o; + }); } @Test @@ -104,11 +121,9 @@ public class PasswordSecurityTest { given(dataSource.getPassword(playerName)).willReturn(password); given(method.comparePassword(clearTextPass, password, playerLowerCase)).willReturn(true); - initSettings(HashAlgorithm.BCRYPT); - PasswordSecurity security = newPasswordSecurity(); // when - boolean result = security.comparePassword(clearTextPass, playerName); + boolean result = passwordSecurity.comparePassword(clearTextPass, playerName); // then assertThat(result, equalTo(true)); @@ -127,11 +142,9 @@ public class PasswordSecurityTest { given(dataSource.getPassword(playerName)).willReturn(password); given(method.comparePassword(clearTextPass, password, playerLowerCase)).willReturn(false); - initSettings(HashAlgorithm.CUSTOM); - PasswordSecurity security = newPasswordSecurity(); // when - boolean result = security.comparePassword(clearTextPass, playerName); + boolean result = passwordSecurity.comparePassword(clearTextPass, playerName); // then assertThat(result, equalTo(false)); @@ -145,13 +158,10 @@ public class PasswordSecurityTest { // given String playerName = "bobby"; String clearTextPass = "tables"; - given(dataSource.getPassword(playerName)).willReturn(null); - initSettings(HashAlgorithm.MD5); - PasswordSecurity security = newPasswordSecurity(); // when - boolean result = security.comparePassword(clearTextPass, playerName); + boolean result = passwordSecurity.comparePassword(clearTextPass, playerName); // then assertThat(result, equalTo(false)); @@ -175,12 +185,12 @@ public class PasswordSecurityTest { given(dataSource.getPassword(argThat(equalToIgnoringCase(playerName)))).willReturn(password); given(method.comparePassword(clearTextPass, password, playerLowerCase)).willReturn(false); given(method.computeHash(clearTextPass, playerLowerCase)).willReturn(newPassword); - initSettings(HashAlgorithm.MD5); + given(settings.getProperty(SecuritySettings.PASSWORD_HASH)).willReturn(HashAlgorithm.MD5); given(settings.getProperty(SecuritySettings.LEGACY_HASHES)).willReturn(newHashSet(HashAlgorithm.BCRYPT)); - PasswordSecurity security = newPasswordSecurity(); + passwordSecurity.reload(); // when - boolean result = security.comparePassword(clearTextPass, playerName); + boolean result = passwordSecurity.comparePassword(clearTextPass, playerName); // then assertThat(result, equalTo(true)); @@ -201,11 +211,13 @@ public class PasswordSecurityTest { String clearTextPass = "someInvalidPassword"; given(dataSource.getPassword(playerName)).willReturn(password); given(method.comparePassword(clearTextPass, password, playerName)).willReturn(false); - initSettings(HashAlgorithm.MD5); - PasswordSecurity security = newPasswordSecurity(); + given(settings.getProperty(SecuritySettings.PASSWORD_HASH)).willReturn(HashAlgorithm.MD5); + given(settings.getProperty(SecuritySettings.LEGACY_HASHES)).willReturn( + newHashSet(HashAlgorithm.DOUBLEMD5, HashAlgorithm.JOOMLA, HashAlgorithm.SMF, HashAlgorithm.SHA256)); + passwordSecurity.reload(); // when - boolean result = security.comparePassword(clearTextPass, playerName); + boolean result = passwordSecurity.comparePassword(clearTextPass, playerName); // then assertThat(result, equalTo(false)); @@ -220,11 +232,11 @@ public class PasswordSecurityTest { String usernameLowerCase = username.toLowerCase(); HashedPassword hashedPassword = new HashedPassword("$T$est#Hash", "__someSalt__"); given(method.computeHash(password, usernameLowerCase)).willReturn(hashedPassword); - initSettings(HashAlgorithm.JOOMLA); - PasswordSecurity security = newPasswordSecurity(); + given(settings.getProperty(SecuritySettings.PASSWORD_HASH)).willReturn(HashAlgorithm.JOOMLA); + passwordSecurity.reload(); // when - HashedPassword result = security.computeHash(password, username); + HashedPassword result = passwordSecurity.computeHash(password, username); // then assertThat(result, equalTo(hashedPassword)); @@ -242,11 +254,11 @@ public class PasswordSecurityTest { String username = "someone12"; HashedPassword hashedPassword = new HashedPassword("~T!est#Hash"); given(method.hasSeparateSalt()).willReturn(true); - initSettings(HashAlgorithm.XAUTH); - PasswordSecurity security = newPasswordSecurity(); + given(settings.getProperty(SecuritySettings.PASSWORD_HASH)).willReturn(HashAlgorithm.XAUTH); + passwordSecurity.reload(); // when - boolean result = security.comparePassword(password, hashedPassword, username); + boolean result = passwordSecurity.comparePassword(password, hashedPassword, username); // then assertThat(result, equalTo(false)); @@ -258,8 +270,6 @@ public class PasswordSecurityTest { @Test public void shouldReloadSettings() { // given - initSettings(HashAlgorithm.BCRYPT); - PasswordSecurity passwordSecurity = newPasswordSecurity(); given(settings.getProperty(SecuritySettings.PASSWORD_HASH)).willReturn(HashAlgorithm.MD5); given(settings.getProperty(SecuritySettings.LEGACY_HASHES)) .willReturn(newHashSet(HashAlgorithm.CUSTOM, HashAlgorithm.BCRYPT)); @@ -274,21 +284,4 @@ public class PasswordSecurityTest { assertThat(ReflectionTestUtils.getFieldValue(PasswordSecurity.class, passwordSecurity, "legacyAlgorithms"), equalTo(legacyHashesSet)); } - - private PasswordSecurity newPasswordSecurity() { - // Use this method to make sure we have all dependents of PasswordSecurity already registered as mocks - PasswordSecurity passwordSecurity = injector.createIfHasDependencies(PasswordSecurity.class); - if (passwordSecurity == null) { - throw new IllegalStateException("Cannot create PasswordSecurity directly! " - + "Did you forget to provide a dependency as mock?"); - } - return passwordSecurity; - } - - private void initSettings(HashAlgorithm algorithm) { - given(settings.getProperty(SecuritySettings.PASSWORD_HASH)).willReturn(algorithm); - given(settings.getProperty(SecuritySettings.LEGACY_HASHES)).willReturn(Collections.emptySet()); - given(settings.getProperty(HooksSettings.BCRYPT_LOG2_ROUND)).willReturn(8); - } - } diff --git a/src/test/java/fr/xephi/authme/task/CleanupTaskTest.java b/src/test/java/fr/xephi/authme/task/CleanupTaskTest.java index c58e8276..33cd360b 100644 --- a/src/test/java/fr/xephi/authme/task/CleanupTaskTest.java +++ b/src/test/java/fr/xephi/authme/task/CleanupTaskTest.java @@ -1,7 +1,7 @@ package fr.xephi.authme.task; -import ch.jalu.injector.Injector; import fr.xephi.authme.initialization.HasCleanup; +import fr.xephi.authme.initialization.factory.SingletonStore; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.InjectMocks; @@ -13,7 +13,6 @@ import java.util.List; import static java.util.Arrays.asList; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.only; import static org.mockito.Mockito.verify; /** @@ -26,13 +25,13 @@ public class CleanupTaskTest { private CleanupTask cleanupTask; @Mock - private Injector injector; + private SingletonStore hasCleanupStore; @Test public void shouldPerformCleanup() { // given List services = asList(mock(HasCleanup.class), mock(HasCleanup.class), mock(HasCleanup.class)); - given(injector.retrieveAllOfType(HasCleanup.class)).willReturn(services); + given(hasCleanupStore.retrieveAllOfType()).willReturn(services); // when cleanupTask.run(); @@ -41,6 +40,5 @@ public class CleanupTaskTest { verify(services.get(0)).performCleanup(); verify(services.get(1)).performCleanup(); verify(services.get(2)).performCleanup(); - verify(injector, only()).retrieveAllOfType(HasCleanup.class); } } diff --git a/src/test/java/tools/docs/hashmethods/EncryptionMethodInfoGatherer.java b/src/test/java/tools/docs/hashmethods/EncryptionMethodInfoGatherer.java index e327e484..0a1d3fa5 100644 --- a/src/test/java/tools/docs/hashmethods/EncryptionMethodInfoGatherer.java +++ b/src/test/java/tools/docs/hashmethods/EncryptionMethodInfoGatherer.java @@ -57,7 +57,7 @@ public class EncryptionMethodInfoGatherer { private static MethodDescription createDescription(HashAlgorithm algorithm) { Class clazz = algorithm.getClazz(); - EncryptionMethod method = injector.newInstance(clazz); + EncryptionMethod method = injector.createIfHasDependencies(clazz); if (method == null) { throw new NullPointerException("Method for '" + algorithm + "' is null"); }