#472 Add translatable messages and unit tests

src/main/java/fr/xephi/authme/command/executable/email/RecoverEmailCommand.java

@@ -72,7 +72,7 @@ public class RecoverEmailCommand extends PlayerCommand {
         if (recoveryCodeManager.isRecoveryCodeNeeded()) {
             // Process /email recovery addr@example.com
             if (arguments.size() == 1) {
-                createAndSendRecoveryCode(playerName, email);
+                createAndSendRecoveryCode(player, email);
             } else {
                 // Process /email recovery addr@example.com 12394
                 processRecoveryCode(player, arguments.get(1), email);
@@ -82,15 +82,16 @@ public class RecoverEmailCommand extends PlayerCommand {
         }
     }
 
-    private void createAndSendRecoveryCode(String name, String email) {
-        String recoveryCode = recoveryCodeManager.generateCode(name);
-        sendMailSsl.sendRecoveryCode(name, email, recoveryCode);
+    private void createAndSendRecoveryCode(Player player, String email) {
+        String recoveryCode = recoveryCodeManager.generateCode(player.getName());
+        sendMailSsl.sendRecoveryCode(player.getName(), email, recoveryCode);
+        commandService.send(player, MessageKey.RECOVERY_CODE_SENT);
     }
 
     private void processRecoveryCode(Player player, String code, String email) {
         final String name = player.getName();
         if (!recoveryCodeManager.isCodeValid(name, code)) {
-            player.sendMessage("The recovery code is not correct! Use /email recovery [email] to generate a new one");
+            commandService.send(player, MessageKey.INCORRECT_RECOVERY_CODE);
             return;
         }
 

src/main/java/fr/xephi/authme/output/MessageKey.java

@@ -147,7 +147,11 @@ public enum MessageKey {
 
     KICK_FOR_ADMIN_REGISTER("kicked_admin_registered"),
 
-    INCOMPLETE_EMAIL_SETTINGS("incomplete_email_settings");
+    INCOMPLETE_EMAIL_SETTINGS("incomplete_email_settings"),
+
+    RECOVERY_CODE_SENT("recovery_code_sent"),
+
+    INCORRECT_RECOVERY_CODE("recovery_code_incorrect");
 
     private String key;
     private String[] tags;

src/main/java/fr/xephi/authme/service/RecoveryCodeManager.java

@@ -1,5 +1,6 @@
 package fr.xephi.authme.service;
 
+import com.google.common.annotations.VisibleForTesting;
 import fr.xephi.authme.initialization.SettingsDependent;
 import fr.xephi.authme.security.RandomString;
 import fr.xephi.authme.settings.Settings;
@@ -17,7 +18,7 @@ import static fr.xephi.authme.util.Utils.MILLIS_PER_HOUR;
  */
 public class RecoveryCodeManager implements SettingsDependent {
 
-    private Map<String, TimedEntry> recoveryCodes = new ConcurrentHashMap<>();
+    private Map<String, ExpiringEntry> recoveryCodes = new ConcurrentHashMap<>();
 
     private int recoveryCodeLength;
     private long recoveryCodeExpirationMillis;
@@ -27,24 +28,45 @@ public class RecoveryCodeManager implements SettingsDependent {
         reload(settings);
     }
 
+    /**
+     * @return whether recovery codes are enabled or not
+     */
     public boolean isRecoveryCodeNeeded() {
-        return recoveryCodeExpirationMillis > 0;
+        return recoveryCodeLength > 0 && recoveryCodeExpirationMillis > 0;
     }
 
+    /**
+     * Generates the recovery code for the given player.
+     *
+     * @param player the player to generate a code for
+     * @return the generated code
+     */
     public String generateCode(String player) {
         String code = RandomString.generateHex(recoveryCodeLength);
-        recoveryCodes.put(player, new TimedEntry(code, System.currentTimeMillis() + recoveryCodeExpirationMillis));
+        recoveryCodes.put(player, new ExpiringEntry(code, System.currentTimeMillis() + recoveryCodeExpirationMillis));
         return code;
     }
 
+    /**
+     * Checks whether the supplied code is valid for the given player.
+     *
+     * @param player the player to check for
+     * @param code the code to check
+     * @return true if the code matches and has not expired, false otherwise
+     */
     public boolean isCodeValid(String player, String code) {
-        TimedEntry entry = recoveryCodes.get(player);
+        ExpiringEntry entry = recoveryCodes.get(player);
         if (entry != null) {
             return code != null && code.equals(entry.getCode());
         }
         return false;
     }
 
+    /**
+     * Removes the player's recovery code if present.
+     *
+     * @param player the player
+     */
     public void removeCode(String player) {
         recoveryCodes.remove(player);
     }
@@ -55,17 +77,21 @@ public class RecoveryCodeManager implements SettingsDependent {
         recoveryCodeExpirationMillis = settings.getProperty(RECOVERY_CODE_HOURS_VALID) * MILLIS_PER_HOUR;
     }
 
-    private static final class TimedEntry {
+    /**
+     * Entry with an expiration.
+     */
+    @VisibleForTesting
+    static final class ExpiringEntry {
 
         private final String code;
         private final long expiration;
 
-        TimedEntry(String code, long expiration) {
+        ExpiringEntry(String code, long expiration) {
             this.code = code;
             this.expiration = expiration;
         }
 
-        public String getCode() {
+        String getCode() {
             return System.currentTimeMillis() < expiration ? code : null;
         }
     }

src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java

@@ -109,7 +109,7 @@ public class SecuritySettings implements SettingsHolder {
     public static final Property<Integer> TEMPBAN_MINUTES_BEFORE_RESET =
         newProperty("Security.tempban.minutesBeforeCounterReset", 480);
 
-    @Comment("Number of characters a recovery code should have")
+    @Comment("Number of characters a recovery code should have (0 to disable)")
     public static final Property<Integer> RECOVERY_CODE_LENGTH =
         newProperty("Security.recoveryCode.length", 8);
 

src/main/resources/config.yml

@@ -339,7 +339,7 @@ Security:
         # Default: 480 minutes (8 hours)
         minutesBeforeCounterReset: 480
     recoveryCode:
-        # Number of characters a recovery code should have
+        # Number of characters a recovery code should have (0 to disable)
         length: 8
         # How many hours is a recovery code valid for?
         validForHours: 4

src/main/resources/messages/messages_en.yml

@@ -70,3 +70,5 @@ accounts_owned_self: 'You own %count accounts:'
 accounts_owned_other: 'The player %name has %count accounts:'
 kicked_admin_registered: 'An admin just registered you; please log in again'
 incomplete_email_settings: 'Error: not all required settings are set for sending emails. Please contact an admin.'
+recovery_code_sent: 'A recovery code to reset your password has been sent to your email.'
+recovery_code_incorrect: 'The recovery code is not correct! Use /email recovery [email] to generate a new one'

src/test/java/fr/xephi/authme/command/executable/email/RecoverEmailCommandTest.java

@@ -251,6 +251,37 @@ public class RecoverEmailCommandTest {
         verify(commandService).send(sender, MessageKey.RECOVERY_EMAIL_SENT_MESSAGE);
     }
 
+    @Test
+    public void shouldGenerateNewPasswordWithoutRecoveryCode() {
+        // given
+        String name = "sh4rK";
+        Player sender = mock(Player.class);
+        given(sender.getName()).willReturn(name);
+        given(sendMailSsl.hasAllInformation()).willReturn(true);
+        given(playerCache.isAuthenticated(name)).willReturn(false);
+        String email = "shark@example.org";
+        PlayerAuth auth = newAuthWithEmail(email);
+        given(dataSource.getAuth(name)).willReturn(auth);
+        given(commandService.getProperty(EmailSettings.RECOVERY_PASSWORD_LENGTH)).willReturn(20);
+        given(passwordSecurity.computeHash(anyString(), eq(name)))
+            .willAnswer(invocation -> new HashedPassword((String) invocation.getArguments()[0]));
+        given(recoveryCodeManager.isRecoveryCodeNeeded()).willReturn(false);
+
+        // when
+        command.executeCommand(sender, Collections.singletonList(email));
+
+        // then
+        verify(sendMailSsl).hasAllInformation();
+        verify(dataSource).getAuth(name);
+        ArgumentCaptor<String> passwordCaptor = ArgumentCaptor.forClass(String.class);
+        verify(passwordSecurity).computeHash(passwordCaptor.capture(), eq(name));
+        String generatedPassword = passwordCaptor.getValue();
+        assertThat(generatedPassword, stringWithLength(20));
+        verify(dataSource).updatePassword(eq(name), any(HashedPassword.class));
+        verify(sendMailSsl).sendPasswordMail(name, email, generatedPassword);
+        verify(commandService).send(sender, MessageKey.RECOVERY_EMAIL_SENT_MESSAGE);
+    }
+
 
     private static PlayerAuth newAuthWithEmail(String email) {
         return PlayerAuth.builder()

src/test/java/fr/xephi/authme/service/RecoveryCodeManagerTest.java

@@ -0,0 +1,117 @@
+package fr.xephi.authme.service;
+
+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.service.RecoveryCodeManager.ExpiringEntry;
+import fr.xephi.authme.settings.Settings;
+import fr.xephi.authme.settings.properties.SecuritySettings;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+
+import java.util.Map;
+
+import static fr.xephi.authme.AuthMeMatchers.stringWithLength;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.nullValue;
+import static org.junit.Assert.assertThat;
+import static org.mockito.BDDMockito.given;
+
+/**
+ * Test for {@link RecoveryCodeManager}.
+ */
+@RunWith(DelayedInjectionRunner.class)
+public class RecoveryCodeManagerTest {
+
+    @InjectDelayed
+    private RecoveryCodeManager recoveryCodeManager;
+
+    @Mock
+    private Settings settings;
+
+    @BeforeInjecting
+    public void initSettings() {
+        given(settings.getProperty(SecuritySettings.RECOVERY_CODE_HOURS_VALID)).willReturn(4);
+        given(settings.getProperty(SecuritySettings.RECOVERY_CODE_LENGTH)).willReturn(5);
+    }
+
+    @Test
+    public void shouldBeDisabledForNonPositiveLength() {
+        assertThat(recoveryCodeManager.isRecoveryCodeNeeded(), equalTo(true));
+
+        // given
+        given(settings.getProperty(SecuritySettings.RECOVERY_CODE_LENGTH)).willReturn(0);
+
+        // when
+        recoveryCodeManager.reload(settings);
+
+        // then
+        assertThat(recoveryCodeManager.isRecoveryCodeNeeded(), equalTo(false));
+    }
+
+    @Test
+    public void shouldGenerateAndStoreCode() {
+        // given
+        String name = "Bobbers";
+
+        // when
+        recoveryCodeManager.generateCode(name);
+
+        // then
+        ExpiringEntry entry = getCodeMap().get(name);
+        assertThat(entry.getCode(), stringWithLength(5));
+    }
+
+    @Test
+    public void shouldNotConsiderExpiredCode() {
+        // given
+        String player = "Cat";
+        String code = "11F235";
+        setCodeInMap(player, code, System.currentTimeMillis() - 500);
+
+        // when
+        boolean result = recoveryCodeManager.isCodeValid(player, code);
+
+        // then
+        assertThat(result, equalTo(false));
+    }
+
+    @Test
+    public void shouldRecognizeCorrectCode() {
+        // given
+        String player = "dragon";
+        String code = recoveryCodeManager.generateCode(player);
+
+        // when
+        boolean result = recoveryCodeManager.isCodeValid(player, code);
+
+        // then
+        assertThat(result, equalTo(true));
+    }
+
+    @Test
+    public void shouldRemoveCode() {
+        // given
+        String player = "Tester";
+        String code = recoveryCodeManager.generateCode(player);
+
+        // when
+        recoveryCodeManager.removeCode(player);
+
+        // then
+        assertThat(recoveryCodeManager.isCodeValid(player, code), equalTo(false));
+        assertThat(getCodeMap().get(player), nullValue());
+    }
+
+
+    private Map<String, ExpiringEntry> getCodeMap() {
+        return ReflectionTestUtils.getFieldValue(RecoveryCodeManager.class, recoveryCodeManager, "recoveryCodes");
+    }
+
+    private void setCodeInMap(String player, String code, long expiration) {
+        Map<String, ExpiringEntry> map = getCodeMap();
+        map.put(player, new ExpiringEntry(code, expiration));
+    }
+}