#930 Registration captcha: update message shown to player on failed captcha

- Show message with new captcha code when a captcha has failed
- Requires implementation of circular dependency handler (initial draft)
This commit is contained in:
ljacqu 2018-01-05 00:17:22 +01:00
parent c8d82a23e0
commit 7cf3f6d77b
9 changed files with 201 additions and 11 deletions

View File

@ -12,6 +12,7 @@ import fr.xephi.authme.initialization.OnShutdownPlayerSaver;
import fr.xephi.authme.initialization.OnStartupTasks; import fr.xephi.authme.initialization.OnStartupTasks;
import fr.xephi.authme.initialization.SettingsProvider; import fr.xephi.authme.initialization.SettingsProvider;
import fr.xephi.authme.initialization.TaskCloser; import fr.xephi.authme.initialization.TaskCloser;
import fr.xephi.authme.initialization.circulardependency.CircularDependencyInitializer;
import fr.xephi.authme.listener.BlockListener; import fr.xephi.authme.listener.BlockListener;
import fr.xephi.authme.listener.EntityListener; import fr.xephi.authme.listener.EntityListener;
import fr.xephi.authme.listener.PlayerListener; import fr.xephi.authme.listener.PlayerListener;
@ -216,6 +217,7 @@ public class AuthMe extends JavaPlugin {
// Set all service fields on the AuthMe class // Set all service fields on the AuthMe class
instantiateServices(injector); instantiateServices(injector);
injector.newInstance(CircularDependencyInitializer.class).initializeCircularDependencies();
// Convert deprecated PLAINTEXT hash entries // Convert deprecated PLAINTEXT hash entries
MigrationService.changePlainTextToSha256(settings, database, new Sha256()); MigrationService.changePlainTextToSha256(settings, database, new Sha256());

View File

@ -72,6 +72,8 @@ public abstract class AbstractCaptchaManager implements SettingsDependent, HasCl
captchaCodes.remove(nameLowerCase); captchaCodes.remove(nameLowerCase);
processSuccessfulCode(nameLowerCase); processSuccessfulCode(nameLowerCase);
return true; return true;
} else {
processUnsuccessfulCode(nameLowerCase);
} }
return false; return false;
} }
@ -106,6 +108,14 @@ public abstract class AbstractCaptchaManager implements SettingsDependent, HasCl
*/ */
protected abstract void processSuccessfulCode(String nameLower); protected abstract void processSuccessfulCode(String nameLower);
/**
* Called when a player has failed the captcha code.
*
* @param nameLower the player's name (all lowercase)
*/
protected void processUnsuccessfulCode(String nameLower) {
}
/** /**
* Returns the number of minutes a generated captcha code should live for before it may expire. * Returns the number of minutes a generated captcha code should live for before it may expire.
* *

View File

@ -1,8 +1,13 @@
package fr.xephi.authme.data; package fr.xephi.authme.data;
import fr.xephi.authme.data.limbo.LimboService;
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.Settings;
import fr.xephi.authme.settings.properties.SecuritySettings; import fr.xephi.authme.settings.properties.SecuritySettings;
import fr.xephi.authme.util.expiring.ExpiringSet; import fr.xephi.authme.util.expiring.ExpiringSet;
import org.bukkit.Bukkit;
import org.bukkit.entity.Player;
import javax.inject.Inject; import javax.inject.Inject;
import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeUnit;
@ -10,10 +15,12 @@ import java.util.concurrent.TimeUnit;
/** /**
* Captcha manager for registration. * Captcha manager for registration.
*/ */
public class RegistrationCaptchaManager extends AbstractCaptchaManager { public class RegistrationCaptchaManager extends AbstractCaptchaManager implements HasCircularDependency {
private static final int MINUTES_VALID_FOR_REGISTRATION = 30; private static final int MINUTES_VALID_FOR_REGISTRATION = 30;
private LimboService limboService;
private final ExpiringSet<String> verifiedNamesForRegistration = private final ExpiringSet<String> verifiedNamesForRegistration =
new ExpiringSet<>(MINUTES_VALID_FOR_REGISTRATION, TimeUnit.MINUTES); new ExpiringSet<>(MINUTES_VALID_FOR_REGISTRATION, TimeUnit.MINUTES);
private boolean isEnabled; private boolean isEnabled;
@ -46,8 +53,19 @@ public class RegistrationCaptchaManager extends AbstractCaptchaManager {
verifiedNamesForRegistration.add(nameLower); verifiedNamesForRegistration.add(nameLower);
} }
@Override
protected void processUnsuccessfulCode(String nameLower) {
final Player player = Bukkit.getPlayerExact(nameLower); // TODO #930: Pass in player!
limboService.resetMessageTask(player, false);
}
@Override @Override
protected int minutesBeforeCodeExpires(Settings settings) { protected int minutesBeforeCodeExpires(Settings settings) {
return MINUTES_VALID_FOR_REGISTRATION; return MINUTES_VALID_FOR_REGISTRATION;
} }
@InjectAfterInitialization
public void setLimboService(LimboService limboService) {
this.limboService = limboService;
}
} }

View File

@ -1,5 +1,6 @@
package fr.xephi.authme.data.limbo; package fr.xephi.authme.data.limbo;
import fr.xephi.authme.data.RegistrationCaptchaManager;
import fr.xephi.authme.data.auth.PlayerCache; import fr.xephi.authme.data.auth.PlayerCache;
import fr.xephi.authme.message.MessageKey; import fr.xephi.authme.message.MessageKey;
import fr.xephi.authme.message.Messages; import fr.xephi.authme.message.Messages;
@ -33,6 +34,9 @@ class LimboPlayerTaskManager {
@Inject @Inject
private PlayerCache playerCache; private PlayerCache playerCache;
@Inject
private RegistrationCaptchaManager registrationCaptchaManager;
LimboPlayerTaskManager() { LimboPlayerTaskManager() {
} }
@ -41,14 +45,14 @@ class LimboPlayerTaskManager {
* *
* @param player the player * @param player the player
* @param limbo the associated limbo player of the player * @param limbo the associated limbo player of the player
* @param isRegistered whether the player is registered or not * @param isRegistered whether the player is registered or not (needed to determine the message in the task)
* (false shows "please register", true shows "please log in")
*/ */
void registerMessageTask(Player player, LimboPlayer limbo, boolean isRegistered) { void registerMessageTask(Player player, LimboPlayer limbo, boolean isRegistered) {
int interval = settings.getProperty(RegistrationSettings.MESSAGE_INTERVAL); int interval = settings.getProperty(RegistrationSettings.MESSAGE_INTERVAL);
MessageKey key = getMessageKey(isRegistered); MessageResult messageResult = getMessageKey(player.getName(), isRegistered);
if (interval > 0) { if (interval > 0) {
MessageTask messageTask = new MessageTask(player, messages.retrieve(key)); String[] joinMessage = messages.retrieveSingle(messageResult.messageKey, messageResult.args).split("\n");
MessageTask messageTask = new MessageTask(player, joinMessage);
bukkitService.runTaskTimer(messageTask, 2 * TICKS_PER_SECOND, interval * TICKS_PER_SECOND); bukkitService.runTaskTimer(messageTask, 2 * TICKS_PER_SECOND, interval * TICKS_PER_SECOND);
limbo.setMessageTask(messageTask); limbo.setMessageTask(messageTask);
} }
@ -84,14 +88,28 @@ class LimboPlayerTaskManager {
/** /**
* Returns the appropriate message key according to the registration status and settings. * Returns the appropriate message key according to the registration status and settings.
* *
* @param name the player's name
* @param isRegistered whether or not the username is registered * @param isRegistered whether or not the username is registered
* @return the message key to display to the user * @return the message key to display to the user
*/ */
private static MessageKey getMessageKey(boolean isRegistered) { private MessageResult getMessageKey(String name, boolean isRegistered) {
if (isRegistered) { if (isRegistered) {
return MessageKey.LOGIN_MESSAGE; return new MessageResult(MessageKey.LOGIN_MESSAGE);
} else if (registrationCaptchaManager.isCaptchaRequired(name)) {
final String captchaCode = registrationCaptchaManager.getCaptchaCodeOrGenerateNew(name);
return new MessageResult(MessageKey.CAPTCHA_FOR_REGISTRATION_REQUIRED, captchaCode);
} else { } else {
return MessageKey.REGISTER_MESSAGE; return new MessageResult(MessageKey.REGISTER_MESSAGE);
}
}
private static final class MessageResult {
private final MessageKey messageKey;
private final String[] args;
MessageResult(MessageKey messageKey, String... args) {
this.messageKey = messageKey;
this.args = args;
} }
} }
} }

View File

@ -0,0 +1,69 @@
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;
}
}

View File

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

View File

@ -0,0 +1,21 @@
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 {
}

View File

@ -6,8 +6,11 @@ import ch.jalu.injector.InjectorBuilder;
import com.google.common.io.Files; import com.google.common.io.Files;
import fr.xephi.authme.api.v3.AuthMeApi; import fr.xephi.authme.api.v3.AuthMeApi;
import fr.xephi.authme.command.CommandHandler; import fr.xephi.authme.command.CommandHandler;
import fr.xephi.authme.data.RegistrationCaptchaManager;
import fr.xephi.authme.data.limbo.LimboService;
import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.datasource.DataSource;
import fr.xephi.authme.initialization.DataFolder; import fr.xephi.authme.initialization.DataFolder;
import fr.xephi.authme.initialization.circulardependency.CircularDependencyInitializer;
import fr.xephi.authme.listener.BlockListener; import fr.xephi.authme.listener.BlockListener;
import fr.xephi.authme.permission.PermissionsManager; import fr.xephi.authme.permission.PermissionsManager;
import fr.xephi.authme.process.Management; import fr.xephi.authme.process.Management;
@ -39,6 +42,7 @@ import java.util.logging.Logger;
import static fr.xephi.authme.settings.properties.AuthMeSettingsRetriever.buildConfigurationData; import static fr.xephi.authme.settings.properties.AuthMeSettingsRetriever.buildConfigurationData;
import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.Matchers.sameInstance;
import static org.junit.Assert.assertThat; import static org.junit.Assert.assertThat;
import static org.mockito.BDDMockito.given; import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mock;
@ -108,6 +112,7 @@ public class AuthMeInitializationTest {
// when // when
authMe.instantiateServices(injector); authMe.instantiateServices(injector);
authMe.registerEventListeners(injector); authMe.registerEventListeners(injector);
injector.newInstance(CircularDependencyInitializer.class).initializeCircularDependencies();
// then // then
// Take a few samples and ensure that they are not null // Take a few samples and ensure that they are not null
@ -121,6 +126,17 @@ public class AuthMeInitializationTest {
assertThat(injector.getIfAvailable(PermissionsManager.class), not(nullValue())); assertThat(injector.getIfAvailable(PermissionsManager.class), not(nullValue()));
assertThat(injector.getIfAvailable(ProcessSyncPlayerLogin.class), not(nullValue())); assertThat(injector.getIfAvailable(ProcessSyncPlayerLogin.class), not(nullValue()));
assertThat(injector.getIfAvailable(PurgeService.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));
}
} }

View File

@ -1,6 +1,7 @@
package fr.xephi.authme.data.limbo; package fr.xephi.authme.data.limbo;
import fr.xephi.authme.TestHelper; import fr.xephi.authme.TestHelper;
import fr.xephi.authme.data.RegistrationCaptchaManager;
import fr.xephi.authme.data.auth.PlayerCache; import fr.xephi.authme.data.auth.PlayerCache;
import fr.xephi.authme.message.MessageKey; import fr.xephi.authme.message.MessageKey;
import fr.xephi.authme.message.Messages; import fr.xephi.authme.message.Messages;
@ -56,6 +57,9 @@ public class LimboPlayerTaskManagerTest {
@Mock @Mock
private PlayerCache playerCache; private PlayerCache playerCache;
@Mock
private RegistrationCaptchaManager registrationCaptchaManager;
@BeforeClass @BeforeClass
public static void setupLogger() { public static void setupLogger() {
TestHelper.setupLogger(); TestHelper.setupLogger();
@ -67,7 +71,7 @@ public class LimboPlayerTaskManagerTest {
Player player = mock(Player.class); Player player = mock(Player.class);
LimboPlayer limboPlayer = mock(LimboPlayer.class); LimboPlayer limboPlayer = mock(LimboPlayer.class);
MessageKey key = MessageKey.REGISTER_MESSAGE; MessageKey key = MessageKey.REGISTER_MESSAGE;
given(messages.retrieve(key)).willReturn(new String[]{"Please register!"}); given(messages.retrieveSingle(key)).willReturn("Please register!");
int interval = 12; int interval = 12;
given(settings.getProperty(RegistrationSettings.MESSAGE_INTERVAL)).willReturn(interval); given(settings.getProperty(RegistrationSettings.MESSAGE_INTERVAL)).willReturn(interval);
@ -76,7 +80,7 @@ public class LimboPlayerTaskManagerTest {
// then // then
verify(limboPlayer).setMessageTask(any(MessageTask.class)); verify(limboPlayer).setMessageTask(any(MessageTask.class));
verify(messages).retrieve(key); verify(messages).retrieveSingle(key);
verify(bukkitService).runTaskTimer( verify(bukkitService).runTaskTimer(
any(MessageTask.class), eq(2L * TICKS_PER_SECOND), eq((long) interval * TICKS_PER_SECOND)); any(MessageTask.class), eq(2L * TICKS_PER_SECOND), eq((long) interval * TICKS_PER_SECOND));
} }
@ -99,11 +103,14 @@ public class LimboPlayerTaskManagerTest {
@Test @Test
public void shouldCancelExistingMessageTask() { public void shouldCancelExistingMessageTask() {
// given // given
String name = "rats";
Player player = mock(Player.class); Player player = mock(Player.class);
given(player.getName()).willReturn(name);
LimboPlayer limboPlayer = new LimboPlayer(null, true, Collections.singletonList("grp"), false, 0.1f, 0.0f); LimboPlayer limboPlayer = new LimboPlayer(null, true, Collections.singletonList("grp"), false, 0.1f, 0.0f);
MessageTask existingMessageTask = mock(MessageTask.class); MessageTask existingMessageTask = mock(MessageTask.class);
limboPlayer.setMessageTask(existingMessageTask); limboPlayer.setMessageTask(existingMessageTask);
given(settings.getProperty(RegistrationSettings.MESSAGE_INTERVAL)).willReturn(8); given(settings.getProperty(RegistrationSettings.MESSAGE_INTERVAL)).willReturn(8);
given(messages.retrieveSingle(MessageKey.REGISTER_MESSAGE)).willReturn("Please register!");
// when // when
limboPlayerTaskManager.registerMessageTask(player, limboPlayer, false); limboPlayerTaskManager.registerMessageTask(player, limboPlayer, false);
@ -111,10 +118,32 @@ public class LimboPlayerTaskManagerTest {
// then // then
assertThat(limboPlayer.getMessageTask(), not(nullValue())); assertThat(limboPlayer.getMessageTask(), not(nullValue()));
assertThat(limboPlayer.getMessageTask(), not(sameInstance(existingMessageTask))); assertThat(limboPlayer.getMessageTask(), not(sameInstance(existingMessageTask)));
verify(messages).retrieve(MessageKey.REGISTER_MESSAGE); verify(registrationCaptchaManager).isCaptchaRequired(name);
verify(messages).retrieveSingle(MessageKey.REGISTER_MESSAGE);
verify(existingMessageTask).cancel(); verify(existingMessageTask).cancel();
} }
@Test
public void shouldInitializeMessageTaskWithCaptchaMessage() {
// given
String name = "race";
Player player = mock(Player.class);
given(player.getName()).willReturn(name);
LimboPlayer limboPlayer = new LimboPlayer(null, true, Collections.singletonList("grp"), false, 0.1f, 0.0f);
given(settings.getProperty(RegistrationSettings.MESSAGE_INTERVAL)).willReturn(12);
given(registrationCaptchaManager.isCaptchaRequired(name)).willReturn(true);
String captcha = "M032";
given(registrationCaptchaManager.getCaptchaCodeOrGenerateNew(name)).willReturn(captcha);
given(messages.retrieveSingle(MessageKey.CAPTCHA_FOR_REGISTRATION_REQUIRED, captcha)).willReturn("Need to use captcha");
// when
limboPlayerTaskManager.registerMessageTask(player, limboPlayer, false);
// then
assertThat(limboPlayer.getMessageTask(), not(nullValue()));
verify(messages).retrieveSingle(MessageKey.CAPTCHA_FOR_REGISTRATION_REQUIRED, captcha);
}
@Test @Test
public void shouldRegisterTimeoutTask() { public void shouldRegisterTimeoutTask() {
// given // given