#930 Extract common captcha functionality into abstract superclass

- Create AbstractCaptchaManager
- Add tests
This commit is contained in:
ljacqu 2017-12-01 23:40:20 +01:00
parent 33904c09e9
commit 1a60036592
7 changed files with 325 additions and 146 deletions

View File

@ -0,0 +1,116 @@
package fr.xephi.authme.data;
import fr.xephi.authme.initialization.HasCleanup;
import fr.xephi.authme.initialization.SettingsDependent;
import fr.xephi.authme.settings.Settings;
import fr.xephi.authme.settings.properties.SecuritySettings;
import fr.xephi.authme.util.RandomStringUtils;
import fr.xephi.authme.util.expiring.ExpiringMap;
import java.util.concurrent.TimeUnit;
/**
* Manages captcha codes.
*/
public abstract class AbstractCaptchaManager implements SettingsDependent, HasCleanup {
// Note: Proper expiration is set in reload(), which is also called on initialization
private final ExpiringMap<String, String> captchaCodes = new ExpiringMap<>(0, TimeUnit.MINUTES);
private int captchaLength;
/**
* Constructor.
*
* @param settings the settings instance
*/
public AbstractCaptchaManager(Settings settings) {
initialize(settings);
}
/**
* Returns whether the given player is required to solve a captcha.
*
* @param name the name of the player to verify
* @return true if the player has to solve a captcha, false otherwise
*/
public abstract boolean isCaptchaRequired(String name);
/**
* Returns the stored captcha for the player or generates and saves a new one.
*
* @param name the player's name
* @return the code the player is required to enter
*/
public String getCaptchaCodeOrGenerateNew(String name) {
String code = captchaCodes.get(name.toLowerCase());
return code == null ? generateCode(name) : code;
}
/**
* Generates a code for the player and returns it.
*
* @param name the name of the player to generate a code for
* @return the generated code
*/
public String generateCode(String name) {
String code = RandomStringUtils.generate(captchaLength);
captchaCodes.put(name.toLowerCase(), code);
return code;
}
/**
* Checks the given code against the existing one and resets the player's auth failure count upon success.
*
* @param name the name of the player to check
* @param code the supplied code
* @return true if the code matches or if no captcha is required for the player, false otherwise
*/
public boolean checkCode(String name, String code) {
final String nameLowerCase = name.toLowerCase();
String savedCode = captchaCodes.get(nameLowerCase);
if (savedCode != null && savedCode.equalsIgnoreCase(code)) {
captchaCodes.remove(nameLowerCase);
processSuccessfulCode(nameLowerCase);
return true;
}
return false;
}
private void initialize(Settings settings) {
captchaLength = settings.getProperty(SecuritySettings.CAPTCHA_LENGTH);
captchaCodes.setExpiration(minutesBeforeCodeExpires(settings), TimeUnit.MINUTES);
}
/**
* Called on initialization and on reload.
*
* @param settings the settings instance
*/
@Override
public void reload(Settings settings) {
// Note ljacqu 20171201: Use initialize() as an in-between method so that we can call it in the constructor
// without causing any trouble to a child that may extend reload -> at the point of calling, the child's fields
// would not yet be initialized.
initialize(settings);
}
@Override
public void performCleanup() {
captchaCodes.removeExpiredEntries();
}
/**
* Called when a player has successfully solved the captcha.
*
* @param nameLower the player's name (all lowercase)
*/
protected abstract void processSuccessfulCode(String nameLower);
/**
* Returns the number of minutes a generated captcha code should live for before it may expire.
*
* @param settings the settings instance
* @return number of minutes that the code is valid for
*/
protected abstract int minutesBeforeCodeExpires(Settings settings);
}

View File

@ -1,33 +1,26 @@
package fr.xephi.authme.data;
import fr.xephi.authme.initialization.HasCleanup;
import fr.xephi.authme.initialization.SettingsDependent;
import fr.xephi.authme.settings.Settings;
import fr.xephi.authme.settings.properties.SecuritySettings;
import fr.xephi.authme.util.RandomStringUtils;
import fr.xephi.authme.util.expiring.TimedCounter;
import javax.inject.Inject;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
/**
* Manager for the handling of captchas after too many failed login attempts.
*/
public class LoginCaptchaManager implements SettingsDependent, HasCleanup {
public class LoginCaptchaManager extends AbstractCaptchaManager {
private final TimedCounter<String> playerCounts;
private final ConcurrentHashMap<String, String> captchaCodes;
// Note: proper expiration is set in reload(), which is also called on initialization by the parent
private final TimedCounter<String> playerCounts = new TimedCounter<>(0, TimeUnit.MINUTES);
private boolean isEnabled;
private int threshold;
private int captchaLength;
@Inject
LoginCaptchaManager(Settings settings) {
this.captchaCodes = new ConcurrentHashMap<>();
long countTimeout = settings.getProperty(SecuritySettings.CAPTCHA_COUNT_MINUTES_BEFORE_RESET);
this.playerCounts = new TimedCounter<>(countTimeout, TimeUnit.MINUTES);
super(settings);
reload(settings);
}
@ -43,57 +36,9 @@ public class LoginCaptchaManager implements SettingsDependent, HasCleanup {
}
}
/**
* Returns whether the given player is required to solve a captcha before he can use /login again.
*
* @param name the name of the player to verify
* @return true if the player has to solve a captcha, false otherwise
*/
public boolean isCaptchaRequired(String name) {
return isEnabled && playerCounts.get(name.toLowerCase()) >= threshold;
}
/**
* Returns the stored captcha for the player or generates and saves a new one.
*
* @param name the player's name
* @return the code the player is required to enter
*/
public String getCaptchaCodeOrGenerateNew(String name) {
String code = captchaCodes.get(name.toLowerCase());
return code == null ? generateCode(name) : code;
}
/**
* Generates a code for the player and returns it.
*
* @param name the name of the player to generate a code for
* @return the generated code
*/
public String generateCode(String name) {
String code = RandomStringUtils.generate(captchaLength);
captchaCodes.put(name.toLowerCase(), code);
return code;
}
/**
* Checks the given code against the existing one and resets the player's auth failure count upon success.
*
* @param name the name of the player to check
* @param code the supplied code
* @return true if the code matches or if no captcha is required for the player, false otherwise
*/
public boolean checkCode(String name, String code) {
final String nameLowerCase = name.toLowerCase();
String savedCode = captchaCodes.get(nameLowerCase);
if (savedCode == null) {
return true;
} else if (savedCode.equalsIgnoreCase(code)) {
captchaCodes.remove(nameLowerCase);
playerCounts.remove(nameLowerCase);
return true;
}
return false;
@Override
public boolean isCaptchaRequired(String playerName) {
return isEnabled && playerCounts.get(playerName.toLowerCase()) >= threshold;
}
/**
@ -103,23 +48,33 @@ public class LoginCaptchaManager implements SettingsDependent, HasCleanup {
*/
public void resetLoginFailureCount(String name) {
if (isEnabled) {
captchaCodes.remove(name.toLowerCase());
playerCounts.remove(name.toLowerCase());
}
}
@Override
public void reload(Settings settings) {
super.reload(settings);
this.isEnabled = settings.getProperty(SecuritySettings.ENABLE_LOGIN_FAILURE_CAPTCHA);
this.threshold = settings.getProperty(SecuritySettings.MAX_LOGIN_TRIES_BEFORE_CAPTCHA);
this.captchaLength = settings.getProperty(SecuritySettings.CAPTCHA_LENGTH);
long countTimeout = settings.getProperty(SecuritySettings.CAPTCHA_COUNT_MINUTES_BEFORE_RESET);
playerCounts.setExpiration(countTimeout, TimeUnit.MINUTES);
}
@Override
public void performCleanup() {
super.performCleanup();
playerCounts.removeExpiredEntries();
}
@Override
protected void processSuccessfulCode(String nameLower) {
playerCounts.remove(nameLower);
}
@Override
protected int minutesBeforeCodeExpires(Settings settings) {
return settings.getProperty(SecuritySettings.CAPTCHA_COUNT_MINUTES_BEFORE_RESET);
}
}

View File

@ -1,98 +1,53 @@
package fr.xephi.authme.data;
import fr.xephi.authme.initialization.HasCleanup;
import fr.xephi.authme.initialization.SettingsDependent;
import fr.xephi.authme.settings.Settings;
import fr.xephi.authme.settings.properties.SecuritySettings;
import fr.xephi.authme.util.RandomStringUtils;
import fr.xephi.authme.util.expiring.ExpiringSet;
import javax.inject.Inject;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
/**
* Captcha handler for registration.
* Captcha manager for registration.
*/
public class RegistrationCaptchaManager implements SettingsDependent, HasCleanup {
public class RegistrationCaptchaManager extends AbstractCaptchaManager {
private static final int MINUTES_VALID_FOR_REGISTRATION = 30;
private final Map<String, String> captchaCodes;
private final ExpiringSet<String> verifiedNamesForRegistration;
private boolean isEnabledForRegistration;
private int captchaLength;
private final ExpiringSet<String> verifiedNamesForRegistration =
new ExpiringSet<>(MINUTES_VALID_FOR_REGISTRATION, TimeUnit.MINUTES);
private boolean isEnabled;
@Inject
RegistrationCaptchaManager(Settings settings) {
this.captchaCodes = new ConcurrentHashMap<>();
this.verifiedNamesForRegistration = new ExpiringSet<>(MINUTES_VALID_FOR_REGISTRATION, TimeUnit.MINUTES);
super(settings);
reload(settings);
}
/**
* Returns whether the given player is required to solve a captcha before he can register.
*
* @param name the name of the player to verify
* @return true if the player has to solve a captcha, false otherwise
*/
@Override
public boolean isCaptchaRequired(String name) {
return isEnabledForRegistration && !verifiedNamesForRegistration.contains(name.toLowerCase());
}
/**
* Returns the stored captcha for the player or generates and saves a new one.
*
* @param name the player's name
* @return the code the player is required to enter
*/
public String getCaptchaCodeOrGenerateNew(String name) {
String code = captchaCodes.get(name.toLowerCase());
return code == null ? generateCode(name) : code;
}
/**
* Generates a code for the player and returns it.
*
* @param name the name of the player to generate a code for
* @return the generated code
*/
public String generateCode(String name) {
String code = RandomStringUtils.generate(captchaLength);
captchaCodes.put(name.toLowerCase(), code);
return code;
}
/**
* Checks the given code against the existing one and resets the player's auth failure count upon success.
*
* @param name the name of the player to check
* @param code the supplied code
* @return true if the code matches or if no captcha is required for the player, false otherwise
*/
public boolean checkCode(String name, String code) {
final String nameLowerCase = name.toLowerCase();
String savedCode = captchaCodes.get(nameLowerCase);
if (savedCode == null) {
return true;
} else if (savedCode.equalsIgnoreCase(code)) {
captchaCodes.remove(nameLowerCase);
verifiedNamesForRegistration.add(nameLowerCase);
return true;
}
return false;
return isEnabled && !verifiedNamesForRegistration.contains(name.toLowerCase());
}
@Override
public void reload(Settings settings) {
this.isEnabledForRegistration = settings.getProperty(SecuritySettings.ENABLE_CAPTCHA_FOR_REGISTRATION);
this.captchaLength = settings.getProperty(SecuritySettings.CAPTCHA_LENGTH);
super.reload(settings);
this.isEnabled = settings.getProperty(SecuritySettings.ENABLE_CAPTCHA_FOR_REGISTRATION);
}
@Override
public void performCleanup() {
super.performCleanup();
verifiedNamesForRegistration.removeExpiredEntries();
}
@Override
protected void processSuccessfulCode(String nameLower) {
verifiedNamesForRegistration.add(nameLower);
}
@Override
protected int minutesBeforeCodeExpires(Settings settings) {
return MINUTES_VALID_FOR_REGISTRATION;
}
}

View File

@ -17,6 +17,7 @@ import java.util.Collections;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.only;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
@ -39,7 +40,7 @@ public class CaptchaCommandTest {
private PlayerCache playerCache;
@Mock
private CommonService commandService;
private CommonService commonService;
@Mock
private LimboService limboService;
@ -55,7 +56,7 @@ public class CaptchaCommandTest {
command.executeCommand(player, Collections.singletonList("123"));
// then
verify(commandService).send(player, MessageKey.ALREADY_LOGGED_IN_ERROR);
verify(commonService).send(player, MessageKey.ALREADY_LOGGED_IN_ERROR);
}
@Test
@ -65,14 +66,16 @@ 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);
// when
command.executeCommand(player, Collections.singletonList("1234"));
// then
verify(commandService).send(player, MessageKey.USAGE_LOGIN);
verify(commonService).send(player, MessageKey.USAGE_LOGIN);
verify(loginCaptchaManager).isCaptchaRequired(name);
verifyNoMoreInteractions(loginCaptchaManager);
verify(registrationCaptchaManager).isCaptchaRequired(name);
verifyNoMoreInteractions(loginCaptchaManager, registrationCaptchaManager);
}
@Test
@ -92,10 +95,10 @@ public class CaptchaCommandTest {
verify(loginCaptchaManager).isCaptchaRequired(name);
verify(loginCaptchaManager).checkCode(name, captchaCode);
verifyNoMoreInteractions(loginCaptchaManager);
verify(commandService).send(player, MessageKey.CAPTCHA_SUCCESS);
verify(commandService).send(player, MessageKey.LOGIN_MESSAGE);
verify(commonService).send(player, MessageKey.CAPTCHA_SUCCESS);
verify(commonService).send(player, MessageKey.LOGIN_MESSAGE);
verify(limboService).unmuteMessageTask(player);
verifyNoMoreInteractions(commandService);
verifyNoMoreInteractions(commonService);
}
@Test
@ -118,8 +121,47 @@ public class CaptchaCommandTest {
verify(loginCaptchaManager).checkCode(name, captchaCode);
verify(loginCaptchaManager).generateCode(name);
verifyNoMoreInteractions(loginCaptchaManager);
verify(commandService).send(player, MessageKey.CAPTCHA_WRONG_ERROR, newCode);
verifyNoMoreInteractions(commandService);
verify(commonService).send(player, MessageKey.CAPTCHA_WRONG_ERROR, newCode);
verifyNoMoreInteractions(commonService);
}
@Test
public void shouldVerifyWithRegisterCaptchaManager() {
// given
String name = "john";
Player player = mockPlayerWithName(name);
given(loginCaptchaManager.isCaptchaRequired(name)).willReturn(false);
given(registrationCaptchaManager.isCaptchaRequired(name)).willReturn(true);
String captchaCode = "A89Y3";
given(registrationCaptchaManager.checkCode(name, captchaCode)).willReturn(true);
// when
command.executeCommand(player, Collections.singletonList(captchaCode));
// then
verify(registrationCaptchaManager).checkCode(name, captchaCode);
verify(loginCaptchaManager, only()).isCaptchaRequired(name);
verify(commonService).send(player, MessageKey.CAPTCHA_SUCCESS);
verify(commonService).send(player, MessageKey.REGISTER_MESSAGE);
}
@Test
public void shouldHandleFailedRegisterCaptcha() {
// given
String name = "asfd";
Player player = mockPlayerWithName(name);
given(registrationCaptchaManager.isCaptchaRequired(name)).willReturn(true);
String captchaCode = "SFL3";
given(registrationCaptchaManager.checkCode(name, captchaCode)).willReturn(false);
given(registrationCaptchaManager.generateCode(name)).willReturn("new code");
// when
command.executeCommand(player, Collections.singletonList(captchaCode));
// then
verify(registrationCaptchaManager).checkCode(name, captchaCode);
verify(registrationCaptchaManager).generateCode(name);
verify(commonService).send(player, MessageKey.CAPTCHA_WRONG_ERROR, "new code");
}
private static Player mockPlayerWithName(String name) {

View File

@ -93,12 +93,13 @@ public class RegisterCommandTest {
public void shouldForwardToManagementForTwoFactor() {
// given
given(commonService.getProperty(SecuritySettings.PASSWORD_HASH)).willReturn(HashAlgorithm.TWO_FACTOR);
Player player = mock(Player.class);
Player player = mockPlayerWithName("test2");
// when
command.executeCommand(player, Collections.emptyList());
// then
verify(registrationCaptchaManager).isCaptchaRequired("test2");
verify(management).performRegister(eq(RegistrationMethod.TWO_FACTOR_REGISTRATION),
argThat(isEqualTo(TwoFactorRegisterParams.of(player))));
verifyZeroInteractions(emailService);
@ -210,12 +211,13 @@ public class RegisterCommandTest {
given(commonService.getProperty(RegistrationSettings.REGISTRATION_TYPE)).willReturn(RegistrationType.EMAIL);
given(commonService.getProperty(RegistrationSettings.REGISTER_SECOND_ARGUMENT)).willReturn(RegisterSecondaryArgument.CONFIRMATION);
given(emailService.hasAllInformation()).willReturn(true);
Player player = mock(Player.class);
Player player = mockPlayerWithName("brett");
// when
command.executeCommand(player, Arrays.asList(playerMail, playerMail));
// then
verify(registrationCaptchaManager).isCaptchaRequired("brett");
verify(validationService).validateEmail(playerMail);
verify(emailService).hasAllInformation();
verify(management).performRegister(eq(RegistrationMethod.EMAIL_REGISTRATION),
@ -240,12 +242,13 @@ public class RegisterCommandTest {
@Test
public void shouldPerformPasswordRegistration() {
// given
Player player = mock(Player.class);
Player player = mockPlayerWithName("newPlayer");
// when
command.executeCommand(player, Collections.singletonList("myPass"));
// then
verify(registrationCaptchaManager).isCaptchaRequired("newPlayer");
verify(management).performRegister(eq(RegistrationMethod.PASSWORD_REGISTRATION),
argThat(isEqualTo(PasswordRegisterParams.of(player, "myPass", null))));
}
@ -275,12 +278,13 @@ public class RegisterCommandTest {
given(commonService.getProperty(RegistrationSettings.REGISTER_SECOND_ARGUMENT)).willReturn(RegisterSecondaryArgument.EMAIL_OPTIONAL);
String email = "email@example.org";
given(validationService.validateEmail(email)).willReturn(false);
Player player = mock(Player.class);
Player player = mockPlayerWithName("Waaa");
// when
command.executeCommand(player, Arrays.asList("myPass", email));
// then
verify(registrationCaptchaManager).isCaptchaRequired("Waaa");
verify(validationService).validateEmail(email);
verify(commonService).send(player, MessageKey.INVALID_EMAIL);
verifyZeroInteractions(management);
@ -291,13 +295,38 @@ public class RegisterCommandTest {
// given
given(commonService.getProperty(RegistrationSettings.REGISTRATION_TYPE)).willReturn(RegistrationType.PASSWORD);
given(commonService.getProperty(RegistrationSettings.REGISTER_SECOND_ARGUMENT)).willReturn(RegisterSecondaryArgument.EMAIL_OPTIONAL);
Player player = mock(Player.class);
Player player = mockPlayerWithName("Doa");
// when
command.executeCommand(player, Collections.singletonList("myPass"));
// then
verify(registrationCaptchaManager).isCaptchaRequired("Doa");
verify(management).performRegister(eq(RegistrationMethod.PASSWORD_REGISTRATION),
argThat(isEqualTo(PasswordRegisterParams.of(player, "myPass", null))));
}
@Test
public void shouldRequestCaptcha() {
// given
given(registrationCaptchaManager.isCaptchaRequired(anyString())).willReturn(true);
String name = "Brian";
Player player = mockPlayerWithName(name);
String captcha = "AB923C";
given(registrationCaptchaManager.getCaptchaCodeOrGenerateNew(name)).willReturn(captcha);
// when
command.executeCommand(player, Arrays.asList("myPass", "myPass"));
// then
verify(registrationCaptchaManager).isCaptchaRequired(name);
verify(commonService).send(player, MessageKey.CAPTCHA_FOR_REGISTRATION_REQUIRED, captcha);
verifyZeroInteractions(management, validationService);
}
private static Player mockPlayerWithName(String name) {
Player player = mock(Player.class);
given(player.getName()).willReturn(name);
return player;
}
}

View File

@ -51,8 +51,8 @@ public class LoginCaptchaManagerTest {
assertThat(captchaCode.length(), equalTo(4));
assertThat(badResult, equalTo(false));
assertThat(goodResult, equalTo(true));
// Supplying correct code should clear the entry, and any code should be valid if no entry is present
assertThat(manager.checkCode(player, "bogus"), equalTo(true));
// Supplying correct code should clear the entry, and a code should be invalid if no entry is present
assertThat(manager.checkCode(player, "bogus"), equalTo(false));
}
@Test

View File

@ -0,0 +1,82 @@
package fr.xephi.authme.data;
import fr.xephi.authme.ReflectionTestUtils;
import fr.xephi.authme.settings.Settings;
import fr.xephi.authme.settings.properties.SecuritySettings;
import fr.xephi.authme.util.expiring.ExpiringMap;
import org.junit.Test;
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;
/**
* Test for {@link RegistrationCaptchaManager}.
*/
public class RegistrationCaptchaManagerTest {
@Test
public void shouldBeDisabled() {
// given
Settings settings = mock(Settings.class);
// Return false first time, and true after that
given(settings.getProperty(SecuritySettings.ENABLE_CAPTCHA_FOR_REGISTRATION))
.willReturn(false).willReturn(true);
given(settings.getProperty(SecuritySettings.CAPTCHA_LENGTH)).willReturn(12);
// when
RegistrationCaptchaManager captchaManager1 = new RegistrationCaptchaManager(settings);
RegistrationCaptchaManager captchaManager2 = new RegistrationCaptchaManager(settings);
// then
assertThat(captchaManager1.isCaptchaRequired("bob"), equalTo(false));
assertThat(captchaManager2.isCaptchaRequired("bob"), equalTo(true));
}
@Test
public void shouldVerifyCodeSuccessfully() {
// given
Settings settings = mock(Settings.class);
given(settings.getProperty(SecuritySettings.ENABLE_CAPTCHA_FOR_REGISTRATION)).willReturn(true);
given(settings.getProperty(SecuritySettings.CAPTCHA_LENGTH)).willReturn(12);
String captcha = "abc3";
RegistrationCaptchaManager captchaManager = new RegistrationCaptchaManager(settings);
getCodeMap(captchaManager).put("test", captcha);
// when
boolean isSuccessful = captchaManager.checkCode("TeSt", captcha);
// then
assertThat(isSuccessful, equalTo(true));
assertThat(getCodeMap(captchaManager).isEmpty(), equalTo(true));
assertThat(captchaManager.isCaptchaRequired("test"), equalTo(false));
}
@Test
public void shouldGenerateAndRetrieveCode() {
// given
Settings settings = mock(Settings.class);
given(settings.getProperty(SecuritySettings.ENABLE_CAPTCHA_FOR_REGISTRATION)).willReturn(true);
int captchaLength = 9;
given(settings.getProperty(SecuritySettings.CAPTCHA_LENGTH)).willReturn(captchaLength);
RegistrationCaptchaManager captchaManager = new RegistrationCaptchaManager(settings);
// when
String captcha1 = captchaManager.getCaptchaCodeOrGenerateNew("toast");
String captcha2 = captchaManager.getCaptchaCodeOrGenerateNew("Toast");
// then
assertThat(captcha1, equalTo(captcha2));
assertThat(captcha1, stringWithLength(captchaLength));
// when (2) / then (2)
assertThat(captchaManager.checkCode("toast", captcha1), equalTo(true));
}
private static ExpiringMap<String, String> getCodeMap(AbstractCaptchaManager captchaManager) {
return ReflectionTestUtils.getFieldValue(AbstractCaptchaManager.class, captchaManager, "captchaCodes");
}
}