diff --git a/src/main/java/fr/xephi/authme/events/PasswordEncryptionEvent.java b/src/main/java/fr/xephi/authme/events/PasswordEncryptionEvent.java index 6e710ddce..2ad872262 100644 --- a/src/main/java/fr/xephi/authme/events/PasswordEncryptionEvent.java +++ b/src/main/java/fr/xephi/authme/events/PasswordEncryptionEvent.java @@ -1,7 +1,6 @@ package fr.xephi.authme.events; import fr.xephi.authme.security.crypts.EncryptionMethod; -import org.bukkit.event.Event; import org.bukkit.event.HandlerList; /** @@ -13,22 +12,19 @@ public class PasswordEncryptionEvent extends CustomEvent { private static final HandlerList handlers = new HandlerList(); private EncryptionMethod method; - private String playerName; /** * Constructor. * * @param method The method used to encrypt the password - * @param playerName The name of the player */ - public PasswordEncryptionEvent(EncryptionMethod method, String playerName) { + public PasswordEncryptionEvent(EncryptionMethod method) { super(false); this.method = method; - this.playerName = playerName; } /** - * Return the list of handlers, equivalent to {@link #getHandlers()} and required by {@link Event}. + * Return the list of handlers, equivalent to {@link #getHandlers()} and required by {@link org.bukkit.event.Event}. * * @return The list of handlers */ @@ -58,14 +54,4 @@ public class PasswordEncryptionEvent extends CustomEvent { public void setMethod(EncryptionMethod method) { this.method = method; } - - /** - * Return the name of the player the event has been fired for. - * - * @return The player name - */ - public String getPlayerName() { - return playerName; - } - } diff --git a/src/main/java/fr/xephi/authme/security/PasswordSecurity.java b/src/main/java/fr/xephi/authme/security/PasswordSecurity.java index d29471089..ebccb63de 100644 --- a/src/main/java/fr/xephi/authme/security/PasswordSecurity.java +++ b/src/main/java/fr/xephi/authme/security/PasswordSecurity.java @@ -29,9 +29,9 @@ public class PasswordSecurity implements Reloadable { private PluginManager pluginManager; @Inject - private Factory hashAlgorithmFactory; + private Factory encryptionMethodFactory; - private HashAlgorithm algorithm; + private EncryptionMethod encryptionMethod; private Collection legacyAlgorithms; /** @@ -40,7 +40,8 @@ public class PasswordSecurity implements Reloadable { @PostConstruct @Override public void reload() { - this.algorithm = settings.getProperty(SecuritySettings.PASSWORD_HASH); + HashAlgorithm algorithm = settings.getProperty(SecuritySettings.PASSWORD_HASH); + this.encryptionMethod = initializeEncryptionMethodWithEvent(algorithm); this.legacyAlgorithms = settings.getProperty(SecuritySettings.LEGACY_HASHES); } @@ -54,8 +55,7 @@ public class PasswordSecurity implements Reloadable { */ public HashedPassword computeHash(String password, String playerName) { String playerLowerCase = playerName.toLowerCase(); - EncryptionMethod method = initializeEncryptionMethodWithEvent(algorithm, playerLowerCase); - return method.computeHash(password, playerLowerCase); + return encryptionMethod.computeHash(password, playerLowerCase); } /** @@ -81,14 +81,13 @@ public class PasswordSecurity implements Reloadable { * @return True if the password matches, false otherwise */ public boolean comparePassword(String password, HashedPassword hashedPassword, String playerName) { - EncryptionMethod method = initializeEncryptionMethodWithEvent(algorithm, playerName); String playerLowerCase = playerName.toLowerCase(); - return methodMatches(method, password, hashedPassword, playerLowerCase) + return methodMatches(encryptionMethod, password, hashedPassword, playerLowerCase) || compareWithLegacyHashes(password, hashedPassword, playerLowerCase); } /** - * Compare the given hash with all available encryption methods to support + * Compare the given hash with the configured legacy encryption methods to support * the migration to a new encryption method. Upon a successful match, the password * will be hashed with the new encryption method and persisted. * @@ -96,13 +95,13 @@ public class PasswordSecurity implements Reloadable { * @param hashedPassword The encrypted password to test the clear-text password against * @param playerName The name of the player * - * @return True if there was a password match with another encryption method, false otherwise + * @return True if there was a password match with a configured legacy encryption method, false otherwise */ private boolean compareWithLegacyHashes(String password, HashedPassword hashedPassword, String playerName) { for (HashAlgorithm algorithm : legacyAlgorithms) { EncryptionMethod method = initializeEncryptionMethod(algorithm); if (methodMatches(method, password, hashedPassword, playerName)) { - hashPasswordForNewAlgorithm(password, playerName); + hashAndSavePasswordWithNewAlgorithm(password, playerName); return true; } } @@ -132,13 +131,12 @@ public class PasswordSecurity implements Reloadable { * which may have been changed by an external listener. * * @param algorithm The algorithm to retrieve the encryption method for - * @param playerName The name of the player a password will be hashed for * * @return The encryption method */ - private EncryptionMethod initializeEncryptionMethodWithEvent(HashAlgorithm algorithm, String playerName) { + private EncryptionMethod initializeEncryptionMethodWithEvent(HashAlgorithm algorithm) { EncryptionMethod method = initializeEncryptionMethod(algorithm); - PasswordEncryptionEvent event = new PasswordEncryptionEvent(method, playerName); + PasswordEncryptionEvent event = new PasswordEncryptionEvent(method); pluginManager.callEvent(event); return event.getMethod(); } @@ -154,12 +152,11 @@ public class PasswordSecurity implements Reloadable { if (HashAlgorithm.CUSTOM.equals(algorithm) || HashAlgorithm.PLAINTEXT.equals(algorithm)) { return null; } - return hashAlgorithmFactory.newInstance(algorithm.getClazz()); + return encryptionMethodFactory.newInstance(algorithm.getClazz()); } - private void hashPasswordForNewAlgorithm(String password, String playerName) { - HashedPassword hashedPassword = initializeEncryptionMethodWithEvent(algorithm, playerName) - .computeHash(password, playerName); + private void hashAndSavePasswordWithNewAlgorithm(String password, String playerName) { + HashedPassword hashedPassword = encryptionMethod.computeHash(password, playerName); dataSource.updatePassword(playerName, hashedPassword); } 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 33482dbb2..22ac16607 100644 --- a/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java +++ b/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java @@ -61,10 +61,6 @@ public final class SecuritySettings implements SettingsHolder { public static final Property PASSWORD_HASH = newProperty(HashAlgorithm.class, "settings.security.passwordHash", HashAlgorithm.SHA256); - @Comment("Salt length for the SALTED2MD5 MD5(MD5(password)+salt)") - public static final Property DOUBLE_MD5_SALT_LENGTH = - newProperty("settings.security.doubleMD5SaltLength", 8); - @Comment({ "If a password check fails, AuthMe will also try to check with the following hash methods.", "Use this setting when you change from one hash method to another.", @@ -75,6 +71,10 @@ public final class SecuritySettings implements SettingsHolder { public static final Property> LEGACY_HASHES = new EnumSetProperty<>(HashAlgorithm.class, "settings.security.legacyHashes"); + @Comment("Salt length for the SALTED2MD5 MD5(MD5(password)+salt)") + public static final Property DOUBLE_MD5_SALT_LENGTH = + newProperty("settings.security.doubleMD5SaltLength", 8); + @Comment("Number of rounds to use if passwordHash is set to PBKDF2. Default is 10000") public static final Property PBKDF2_NUMBER_OF_ROUNDS = newProperty("settings.security.pbkdf2Rounds", 10000); diff --git a/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java b/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java index 2d5368197..4f3704175 100644 --- a/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java +++ b/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java @@ -57,7 +57,10 @@ public class HashAlgorithmIntegrationTest { // given / when / then for (HashAlgorithm algorithm : HashAlgorithm.values()) { if (!HashAlgorithm.CUSTOM.equals(algorithm) && !HashAlgorithm.PLAINTEXT.equals(algorithm)) { - EncryptionMethod method = injector.newInstance(algorithm.getClazz()); + EncryptionMethod method = injector.createIfHasDependencies(algorithm.getClazz()); + if (method == null) { + fail("Could not create '" + algorithm.getClazz() + "' - forgot to provide some class?"); + } HashedPassword hashedPassword = method.computeHash("pwd", "name"); assertThat("Salt should not be null if method.hasSeparateSalt(), and vice versa. Method: '" + method + "'", StringUtils.isEmpty(hashedPassword.getSalt()), equalTo(!method.hasSeparateSalt())); diff --git a/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java b/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java index 6bd0b45ad..441180760 100644 --- a/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java +++ b/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java @@ -13,6 +13,7 @@ import fr.xephi.authme.initialization.factory.Factory; import fr.xephi.authme.security.crypts.EncryptionMethod; import fr.xephi.authme.security.crypts.HashedPassword; import fr.xephi.authme.security.crypts.Joomla; +import fr.xephi.authme.security.crypts.Md5; import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.properties.HooksSettings; import fr.xephi.authme.settings.properties.SecuritySettings; @@ -21,7 +22,6 @@ import org.bukkit.plugin.PluginManager; import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; -import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; @@ -32,12 +32,14 @@ import java.util.Set; import static com.google.common.collect.Sets.newHashSet; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.equalToIgnoringCase; +import static org.hamcrest.Matchers.instanceOf; import static org.junit.Assert.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.hamcrest.MockitoHamcrest.argThat; @@ -166,7 +168,6 @@ public class PasswordSecurityTest { // then assertThat(result, equalTo(false)); verify(dataSource).getPassword(playerName); - verify(pluginManager, never()).callEvent(any(Event.class)); verify(method, never()).comparePassword(anyString(), any(HashedPassword.class), anyString()); } @@ -204,7 +205,7 @@ public class PasswordSecurityTest { } @Test - public void shouldTryAllMethodsAndFail() { + public void shouldTryLegacyMethodsAndFail() { // given HashedPassword password = new HashedPassword("hashNotMatchingAnyMethod", "someBogusSalt"); String playerName = "asfd"; @@ -240,11 +241,9 @@ public class PasswordSecurityTest { // then assertThat(result, equalTo(hashedPassword)); - ArgumentCaptor captor = ArgumentCaptor.forClass(PasswordEncryptionEvent.class); - verify(pluginManager).callEvent(captor.capture()); - PasswordEncryptionEvent event = captor.getValue(); + // Check that an event was fired twice: once on test setup, and once because we called reload() + verify(pluginManager, times(2)).callEvent(any(PasswordEncryptionEvent.class)); assertThat(Joomla.class.equals(caughtClassInEvent), equalTo(true)); - assertThat(event.getPlayerName(), equalTo(usernameLowerCase)); } @Test @@ -263,7 +262,8 @@ public class PasswordSecurityTest { // then assertThat(result, equalTo(false)); verify(dataSource, never()).getAuth(anyString()); - verify(pluginManager).callEvent(any(PasswordEncryptionEvent.class)); + // Check that an event was fired twice: once on test setup, and once because we called reload() + verify(pluginManager, times(2)).callEvent(any(PasswordEncryptionEvent.class)); verify(method, never()).comparePassword(anyString(), any(HashedPassword.class), anyString()); } @@ -273,13 +273,14 @@ public class PasswordSecurityTest { given(settings.getProperty(SecuritySettings.PASSWORD_HASH)).willReturn(HashAlgorithm.MD5); given(settings.getProperty(SecuritySettings.LEGACY_HASHES)) .willReturn(newHashSet(HashAlgorithm.CUSTOM, HashAlgorithm.BCRYPT)); + reset(pluginManager); // reset behavior when the event is emitted to check that we create an instance of Md5.java // when passwordSecurity.reload(); // then - assertThat(ReflectionTestUtils.getFieldValue(PasswordSecurity.class, passwordSecurity, "algorithm"), - equalTo(HashAlgorithm.MD5)); + assertThat(ReflectionTestUtils.getFieldValue(PasswordSecurity.class, passwordSecurity, "encryptionMethod"), + instanceOf(Md5.class)); Set legacyHashesSet = newHashSet(HashAlgorithm.CUSTOM, HashAlgorithm.BCRYPT); assertThat(ReflectionTestUtils.getFieldValue(PasswordSecurity.class, passwordSecurity, "legacyAlgorithms"), equalTo(legacyHashesSet));