mirror of
https://github.com/AuthMe/AuthMeReloaded.git
synced 2024-11-03 01:00:18 +01:00
Cleanup: avoid injecting Injector directly
- Inject SingletonStore to restrict the possible functions - Refactor PasswordSecurityTest to correspond to the usual way of testing
This commit is contained in:
ljacqu
parent
d19748fe5b
commit
7dbf5551c9
@ -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<Reloadable> reloadableStore;
|
||||
|
||||
@Inject
|
||||
private SingletonStore<SettingsDependent> settingsDependentStore;
|
||||
|
||||
@Override
|
||||
public void executeCommand(CommandSender sender, List<String> 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));
|
||||
}
|
||||
}
|
||||
|
||||
@ -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<HasCleanup> hasCleanupStore;
|
||||
|
||||
CleanupTask() {
|
||||
}
|
||||
|
||||
@Override
|
||||
public void run() {
|
||||
for (HasCleanup service : injector.retrieveAllOfType(HasCleanup.class)) {
|
||||
service.performCleanup();
|
||||
}
|
||||
hasCleanupStore.retrieveAllOfType()
|
||||
.forEach(HasCleanup::performCleanup);
|
||||
}
|
||||
}
|
||||
|
||||
@ -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<Reloadable> reloadableStore;
|
||||
|
||||
@Mock
|
||||
private SingletonStore<SettingsDependent> 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<SettingsDependent> 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.<String>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.<String>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<Reloadable>());
|
||||
given(injector.retrieveAllOfType(SettingsDependent.class)).willReturn(new ArrayList<SettingsDependent>());
|
||||
given(reloadableStore.retrieveAllOfType()).willReturn(Collections.emptyList());
|
||||
given(settingsDependentStore.retrieveAllOfType()).willReturn(Collections.emptyList());
|
||||
|
||||
// when
|
||||
command.executeCommand(sender, Collections.<String>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")));
|
||||
}
|
||||
|
||||
|
||||
@ -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<HashAlgorithm> 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);
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
@ -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<HasCleanup> hasCleanupStore;
|
||||
|
||||
@Test
|
||||
public void shouldPerformCleanup() {
|
||||
// given
|
||||
List<HasCleanup> 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);
|
||||
}
|
||||
}
|
||||
|
||||
@ -57,7 +57,7 @@ public class EncryptionMethodInfoGatherer {
|
||||
|
||||
private static MethodDescription createDescription(HashAlgorithm algorithm) {
|
||||
Class<? extends EncryptionMethod> clazz = algorithm.getClazz();
|
||||
EncryptionMethod method = injector.newInstance(clazz);
|
||||
EncryptionMethod method = injector.createIfHasDependencies(clazz);
|
||||
if (method == null) {
|
||||
throw new NullPointerException("Method for '" + algorithm + "' is null");
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user