diff --git a/src/main/java/fr/xephi/authme/command/executable/email/RecoverEmailCommand.java b/src/main/java/fr/xephi/authme/command/executable/email/RecoverEmailCommand.java index 6f7923d7..26b0e2f9 100644 --- a/src/main/java/fr/xephi/authme/command/executable/email/RecoverEmailCommand.java +++ b/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; } diff --git a/src/main/java/fr/xephi/authme/output/MessageKey.java b/src/main/java/fr/xephi/authme/output/MessageKey.java index c445b9a2..c5a7cd36 100644 --- a/src/main/java/fr/xephi/authme/output/MessageKey.java +++ b/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; diff --git a/src/main/java/fr/xephi/authme/service/RecoveryCodeManager.java b/src/main/java/fr/xephi/authme/service/RecoveryCodeManager.java index bca8dd51..e37d2273 100644 --- a/src/main/java/fr/xephi/authme/service/RecoveryCodeManager.java +++ b/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 recoveryCodes = new ConcurrentHashMap<>(); + private Map 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; } } diff --git a/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java b/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java index fb1cf258..7d91f818 100644 --- a/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java +++ b/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java @@ -109,7 +109,7 @@ public class SecuritySettings implements SettingsHolder { public static final Property 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 RECOVERY_CODE_LENGTH = newProperty("Security.recoveryCode.length", 8); diff --git a/src/main/resources/config.yml b/src/main/resources/config.yml index 6cf97f9f..3221913b 100644 --- a/src/main/resources/config.yml +++ b/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 diff --git a/src/main/resources/messages/messages_en.yml b/src/main/resources/messages/messages_en.yml index f96db604..a942fe85 100644 --- a/src/main/resources/messages/messages_en.yml +++ b/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' diff --git a/src/test/java/fr/xephi/authme/command/executable/email/RecoverEmailCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/email/RecoverEmailCommandTest.java index 6185d0b2..d57693cf 100644 --- a/src/test/java/fr/xephi/authme/command/executable/email/RecoverEmailCommandTest.java +++ b/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 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() diff --git a/src/test/java/fr/xephi/authme/service/RecoveryCodeManagerTest.java b/src/test/java/fr/xephi/authme/service/RecoveryCodeManagerTest.java new file mode 100644 index 00000000..c606a743 --- /dev/null +++ b/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 getCodeMap() { + return ReflectionTestUtils.getFieldValue(RecoveryCodeManager.class, recoveryCodeManager, "recoveryCodes"); + } + + private void setCodeInMap(String player, String code, long expiration) { + Map map = getCodeMap(); + map.put(player, new ExpiringEntry(code, expiration)); + } +}