#930 Register captcha: avoid circular dependency by handling limbo message in captcha command

- Set limbo message in captcha command (as is done for login captcha)
- Add clarifying comments to captcha command
- Remove classes handling circular dependencies

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());

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<String> 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);
     }
 }

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<String> 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;
-    }
 }

src/main/java/fr/xephi/authme/initialization/circulardependency/CircularDependencyInitializer.java

@@ -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;
-    }
-}

src/main/java/fr/xephi/authme/initialization/circulardependency/HasCircularDependency.java

@@ -1,7 +0,0 @@
-package fr.xephi.authme.initialization.circulardependency;
-
-/**
- * Marker interface for classes with {@link HasCircularDependency} methods.
- */
-public interface HasCircularDependency {
-}

src/main/java/fr/xephi/authme/initialization/circulardependency/InjectAfterInitialization.java

@@ -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.
- * <p>
- * 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 {
-}

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));
     }
 }

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);

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")