diff --git a/src/main/java/fr/xephi/authme/AuthMe.java b/src/main/java/fr/xephi/authme/AuthMe.java index 3e587d061..15a37ab48 100644 --- a/src/main/java/fr/xephi/authme/AuthMe.java +++ b/src/main/java/fr/xephi/authme/AuthMe.java @@ -12,7 +12,6 @@ import fr.xephi.authme.initialization.OnShutdownPlayerSaver; import fr.xephi.authme.initialization.OnStartupTasks; import fr.xephi.authme.initialization.SettingsProvider; import fr.xephi.authme.initialization.TaskCloser; -import fr.xephi.authme.initialization.circulardependency.CircularDependencyInitializer; import fr.xephi.authme.listener.BlockListener; import fr.xephi.authme.listener.EntityListener; import fr.xephi.authme.listener.PlayerListener; @@ -217,7 +216,6 @@ public class AuthMe extends JavaPlugin { // Set all service fields on the AuthMe class instantiateServices(injector); - injector.newInstance(CircularDependencyInitializer.class).initializeCircularDependencies(); // Convert deprecated PLAINTEXT hash entries MigrationService.changePlainTextToSha256(settings, database, new Sha256()); diff --git a/src/main/java/fr/xephi/authme/command/executable/captcha/CaptchaCommand.java b/src/main/java/fr/xephi/authme/command/executable/captcha/CaptchaCommand.java index d8e9e4a7d..b87af3cae 100644 --- a/src/main/java/fr/xephi/authme/command/executable/captcha/CaptchaCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/captcha/CaptchaCommand.java @@ -5,6 +5,7 @@ import fr.xephi.authme.data.auth.PlayerCache; import fr.xephi.authme.data.captcha.LoginCaptchaManager; import fr.xephi.authme.data.captcha.RegistrationCaptchaManager; import fr.xephi.authme.data.limbo.LimboService; +import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.message.MessageKey; import fr.xephi.authme.service.CommonService; import org.bukkit.entity.Player; @@ -12,6 +13,9 @@ import org.bukkit.entity.Player; import javax.inject.Inject; import java.util.List; +/** + * Captcha command, allowing a player to solve a captcha. + */ public class CaptchaCommand extends PlayerCommand { @Inject @@ -29,20 +33,29 @@ public class CaptchaCommand extends PlayerCommand { @Inject private LimboService limboService; + @Inject + private DataSource dataSource; + @Override public void runCommand(Player player, List arguments) { final String name = player.getName(); if (playerCache.isAuthenticated(name)) { + // No captcha is relevant if the player is logged in commonService.send(player, MessageKey.ALREADY_LOGGED_IN_ERROR); - } else if (loginCaptchaManager.isCaptchaRequired(name)) { + return; + } + + if (loginCaptchaManager.isCaptchaRequired(name)) { checkLoginCaptcha(player, arguments.get(0)); - } else if (registrationCaptchaManager.isCaptchaRequired(name)) { - checkRegisterCaptcha(player, arguments.get(0)); } else { - MessageKey errorMessage = playerCache.isAuthenticated(name) - ? MessageKey.ALREADY_LOGGED_IN_ERROR : MessageKey.USAGE_LOGIN; - commonService.send(player, errorMessage); + final boolean isPlayerRegistered = dataSource.isAuthAvailable(name); + if (!isPlayerRegistered && registrationCaptchaManager.isCaptchaRequired(name)) { + checkRegisterCaptcha(player, arguments.get(0)); + } else { + MessageKey errorMessage = isPlayerRegistered ? MessageKey.USAGE_LOGIN : MessageKey.USAGE_REGISTER; + commonService.send(player, errorMessage); + } } } @@ -67,5 +80,6 @@ public class CaptchaCommand extends PlayerCommand { String newCode = registrationCaptchaManager.getCaptchaCodeOrGenerateNew(player.getName()); commonService.send(player, MessageKey.CAPTCHA_WRONG_ERROR, newCode); } + limboService.resetMessageTask(player, false); } } diff --git a/src/main/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManager.java b/src/main/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManager.java index bce954298..3b751247e 100644 --- a/src/main/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManager.java +++ b/src/main/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManager.java @@ -1,10 +1,7 @@ package fr.xephi.authme.data.captcha; -import fr.xephi.authme.data.limbo.LimboService; import fr.xephi.authme.initialization.HasCleanup; import fr.xephi.authme.initialization.SettingsDependent; -import fr.xephi.authme.initialization.circulardependency.HasCircularDependency; -import fr.xephi.authme.initialization.circulardependency.InjectAfterInitialization; import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.properties.SecuritySettings; import fr.xephi.authme.util.expiring.ExpiringSet; @@ -16,13 +13,10 @@ import java.util.concurrent.TimeUnit; /** * Captcha manager for registration. */ -public class RegistrationCaptchaManager - implements CaptchaManager, SettingsDependent, HasCleanup, HasCircularDependency { +public class RegistrationCaptchaManager implements CaptchaManager, SettingsDependent, HasCleanup { private static final int MINUTES_VALID_FOR_REGISTRATION = 30; - private LimboService limboService; - private final ExpiringSet verifiedNamesForRegistration; private final CaptchaCodeStorage captchaCodeStorage; private boolean isEnabled; @@ -52,7 +46,6 @@ public class RegistrationCaptchaManager if (isCodeCorrect) { verifiedNamesForRegistration.add(nameLower); } - limboService.resetMessageTask(player, false); return isCodeCorrect; } @@ -69,9 +62,4 @@ public class RegistrationCaptchaManager verifiedNamesForRegistration.removeExpiredEntries(); captchaCodeStorage.removeExpiredEntries(); } - - @InjectAfterInitialization - public void setLimboService(LimboService limboService) { - this.limboService = limboService; - } } diff --git a/src/main/java/fr/xephi/authme/initialization/circulardependency/CircularDependencyInitializer.java b/src/main/java/fr/xephi/authme/initialization/circulardependency/CircularDependencyInitializer.java deleted file mode 100644 index beca17925..000000000 --- a/src/main/java/fr/xephi/authme/initialization/circulardependency/CircularDependencyInitializer.java +++ /dev/null @@ -1,69 +0,0 @@ -package fr.xephi.authme.initialization.circulardependency; - -import ch.jalu.injector.Injector; -import ch.jalu.injector.utils.ReflectionUtils; - -import javax.inject.Inject; -import java.lang.reflect.Method; -import java.lang.reflect.Modifier; - -/** - * Fix for circular dependencies: after initialization this class can be called - * to inject dependencies via {@link InjectAfterInitialization} methods. - */ -public class CircularDependencyInitializer { - - @Inject - private Injector injector; - - CircularDependencyInitializer() { - } - - /** - * Processes all known {@link HasCircularDependency} classes, invoking all methods - * annotated with {@link InjectAfterInitialization}. - */ - public void initializeCircularDependencies() { - for (HasCircularDependency hasCircularDependency : injector.retrieveAllOfType(HasCircularDependency.class)) { - processClass(hasCircularDependency); - } - } - - private void processClass(HasCircularDependency object) { - for (Method method : object.getClass().getDeclaredMethods()) { - if (method.isAnnotationPresent(InjectAfterInitialization.class)) { - Object resolvedObject = resolveParameterForMethodOrThrow(method); - ReflectionUtils.invokeMethod(method, object, resolvedObject); - } - } - } - - /** - * Validates that the given method is a valid {@link InjectAfterInitialization} method - * and resolves the parameter it should be passed (assumes a singleton of the given type is registered - * in the injector). Throws an exception if the parameter type is not a registered singleton. - * - * @param method the method to process - * @return object to pass to the initializer method - */ - private Object resolveParameterForMethodOrThrow(Method method) { - if (method.getParameterCount() != 1) { - throw new IllegalStateException("Method " + method.getDeclaringClass() + "#" + method.getName() - + " should have one parameter only"); - } else if (!Modifier.isPublic(method.getModifiers())) { - throw new IllegalStateException("Method " + method.getDeclaringClass() + "#" + method.getName() - + " should be public"); - } else if (method.getReturnType() != void.class) { - throw new IllegalStateException("Method " + method.getDeclaringClass() + "#" + method.getName() - + " must return void"); - } - - final Class requiredType = method.getParameterTypes()[0]; - final Object resolvedObject = injector.getIfAvailable(requiredType); - if (resolvedObject == null) { - throw new IllegalStateException("Failed to get parameter of type '" + requiredType - + "' for @InjectAfterInitialization method " + method.getDeclaringClass() + "#" + method.getName()); - } - return resolvedObject; - } -} diff --git a/src/main/java/fr/xephi/authme/initialization/circulardependency/HasCircularDependency.java b/src/main/java/fr/xephi/authme/initialization/circulardependency/HasCircularDependency.java deleted file mode 100644 index 4805b60ea..000000000 --- a/src/main/java/fr/xephi/authme/initialization/circulardependency/HasCircularDependency.java +++ /dev/null @@ -1,7 +0,0 @@ -package fr.xephi.authme.initialization.circulardependency; - -/** - * Marker interface for classes with {@link HasCircularDependency} methods. - */ -public interface HasCircularDependency { -} diff --git a/src/main/java/fr/xephi/authme/initialization/circulardependency/InjectAfterInitialization.java b/src/main/java/fr/xephi/authme/initialization/circulardependency/InjectAfterInitialization.java deleted file mode 100644 index cc35e56c0..000000000 --- a/src/main/java/fr/xephi/authme/initialization/circulardependency/InjectAfterInitialization.java +++ /dev/null @@ -1,21 +0,0 @@ -package fr.xephi.authme.initialization.circulardependency; - -import java.lang.annotation.ElementType; -import java.lang.annotation.Retention; -import java.lang.annotation.RetentionPolicy; -import java.lang.annotation.Target; - -/** - * Marks a method that should be invoked once injection has taken place. - * This is a fix for circular dependencies. - *

- * Methods with this annotation must have exactly one parameter whose type is a singleton - * registered in the injector. Classes with such methods must implement the {@link HasCircularDependency} - * marker interface. - * - * @see CircularDependencyInitializer - */ -@Retention(RetentionPolicy.RUNTIME) -@Target(ElementType.METHOD) -public @interface InjectAfterInitialization { -} diff --git a/src/test/java/fr/xephi/authme/AuthMeInitializationTest.java b/src/test/java/fr/xephi/authme/AuthMeInitializationTest.java index 64cbfa41f..6d1770d4e 100644 --- a/src/test/java/fr/xephi/authme/AuthMeInitializationTest.java +++ b/src/test/java/fr/xephi/authme/AuthMeInitializationTest.java @@ -6,11 +6,8 @@ import ch.jalu.injector.InjectorBuilder; import com.google.common.io.Files; import fr.xephi.authme.api.v3.AuthMeApi; import fr.xephi.authme.command.CommandHandler; -import fr.xephi.authme.data.captcha.RegistrationCaptchaManager; -import fr.xephi.authme.data.limbo.LimboService; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.initialization.DataFolder; -import fr.xephi.authme.initialization.circulardependency.CircularDependencyInitializer; import fr.xephi.authme.listener.BlockListener; import fr.xephi.authme.permission.PermissionsManager; import fr.xephi.authme.process.Management; @@ -42,7 +39,6 @@ import java.util.logging.Logger; import static fr.xephi.authme.settings.properties.AuthMeSettingsRetriever.buildConfigurationData; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; -import static org.hamcrest.Matchers.sameInstance; import static org.junit.Assert.assertThat; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; @@ -112,7 +108,6 @@ public class AuthMeInitializationTest { // when authMe.instantiateServices(injector); authMe.registerEventListeners(injector); - injector.newInstance(CircularDependencyInitializer.class).initializeCircularDependencies(); // then // Take a few samples and ensure that they are not null @@ -126,17 +121,5 @@ public class AuthMeInitializationTest { assertThat(injector.getIfAvailable(PermissionsManager.class), not(nullValue())); assertThat(injector.getIfAvailable(ProcessSyncPlayerLogin.class), not(nullValue())); assertThat(injector.getIfAvailable(PurgeService.class), not(nullValue())); - - assertCircularDependencyWasSet(injector); - } - - private void assertCircularDependencyWasSet(Injector injector) { - RegistrationCaptchaManager registrationCaptchaManager = injector.getIfAvailable(RegistrationCaptchaManager.class); - LimboService limboServiceOnCaptchaManager = ReflectionTestUtils.getFieldValue( - RegistrationCaptchaManager.class, registrationCaptchaManager, "limboService"); - LimboService limboServiceFromInjector = injector.getIfAvailable(LimboService.class); - - assertThat(limboServiceOnCaptchaManager, not(nullValue())); - assertThat(limboServiceOnCaptchaManager, sameInstance(limboServiceFromInjector)); } } diff --git a/src/test/java/fr/xephi/authme/command/executable/captcha/CaptchaCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/captcha/CaptchaCommandTest.java index 8c139508f..15b6dba2e 100644 --- a/src/test/java/fr/xephi/authme/command/executable/captcha/CaptchaCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/captcha/CaptchaCommandTest.java @@ -4,6 +4,7 @@ import fr.xephi.authme.data.auth.PlayerCache; import fr.xephi.authme.data.captcha.LoginCaptchaManager; import fr.xephi.authme.data.captcha.RegistrationCaptchaManager; import fr.xephi.authme.data.limbo.LimboService; +import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.message.MessageKey; import fr.xephi.authme.service.CommonService; import org.bukkit.entity.Player; @@ -45,6 +46,9 @@ public class CaptchaCommandTest { @Mock private LimboService limboService; + @Mock + private DataSource dataSource; + @Test public void shouldDetectIfPlayerIsLoggedIn() { // given @@ -66,7 +70,8 @@ public class CaptchaCommandTest { Player player = mockPlayerWithName(name); given(playerCache.isAuthenticated(name)).willReturn(false); given(loginCaptchaManager.isCaptchaRequired(name)).willReturn(false); - given(registrationCaptchaManager.isCaptchaRequired(name)).willReturn(false); + given(registrationCaptchaManager.isCaptchaRequired(name)).willReturn(true); + given(dataSource.isAuthAvailable(name)).willReturn(true); // when command.executeCommand(player, Collections.singletonList("1234")); @@ -74,7 +79,6 @@ public class CaptchaCommandTest { // then verify(commonService).send(player, MessageKey.USAGE_LOGIN); verify(loginCaptchaManager).isCaptchaRequired(name); - verify(registrationCaptchaManager).isCaptchaRequired(name); verifyNoMoreInteractions(loginCaptchaManager, registrationCaptchaManager); } @@ -164,6 +168,21 @@ public class CaptchaCommandTest { verify(commonService).send(player, MessageKey.CAPTCHA_WRONG_ERROR, "new code"); } + @Test + public void shouldShowRegisterUsageWhenRegistrationCaptchaIsSolved() { + // given + String name = "alice"; + Player player = mockPlayerWithName(name); + given(registrationCaptchaManager.isCaptchaRequired(name)).willReturn(false); + + // when + command.executeCommand(player, Collections.singletonList("test")); + + // then + verify(registrationCaptchaManager, only()).isCaptchaRequired(name); + verify(commonService).send(player, MessageKey.USAGE_REGISTER); + } + private static Player mockPlayerWithName(String name) { Player player = mock(Player.class); given(player.getName()).willReturn(name); diff --git a/src/test/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManagerTest.java b/src/test/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManagerTest.java index abd7d295e..4a71ab2ca 100644 --- a/src/test/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManagerTest.java +++ b/src/test/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManagerTest.java @@ -1,28 +1,23 @@ package fr.xephi.authme.data.captcha; import fr.xephi.authme.ReflectionTestUtils; -import fr.xephi.authme.data.limbo.LimboService; import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.properties.SecuritySettings; import fr.xephi.authme.util.expiring.ExpiringMap; import org.bukkit.entity.Player; import org.junit.Test; -import org.mockito.Mockito; import static fr.xephi.authme.AuthMeMatchers.stringWithLength; import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertThat; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; /** * Test for {@link RegistrationCaptchaManager}. */ public class RegistrationCaptchaManagerTest { - private LimboService limboService = Mockito.mock(LimboService.class); - @Test public void shouldBeDisabled() { // given @@ -50,7 +45,6 @@ public class RegistrationCaptchaManagerTest { String captcha = "abc3"; RegistrationCaptchaManager captchaManager = new RegistrationCaptchaManager(settings); - captchaManager.setLimboService(limboService); getCodeMap(captchaManager).put("test", captcha); Player player = mock(Player.class); @@ -63,7 +57,6 @@ public class RegistrationCaptchaManagerTest { assertThat(isSuccessful, equalTo(true)); assertThat(getCodeMap(captchaManager).isEmpty(), equalTo(true)); assertThat(captchaManager.isCaptchaRequired("test"), equalTo(false)); - verify(limboService).resetMessageTask(player, false); } @Test @@ -74,7 +67,6 @@ public class RegistrationCaptchaManagerTest { int captchaLength = 9; given(settings.getProperty(SecuritySettings.CAPTCHA_LENGTH)).willReturn(captchaLength); RegistrationCaptchaManager captchaManager = new RegistrationCaptchaManager(settings); - captchaManager.setLimboService(limboService); // when String captcha1 = captchaManager.getCaptchaCodeOrGenerateNew("toast"); @@ -90,7 +82,6 @@ public class RegistrationCaptchaManagerTest { // when (2) / then (2) assertThat(captchaManager.checkCode(player, captcha1), equalTo(true)); - verify(limboService).resetMessageTask(player, false); } @SuppressWarnings("unchecked")