diff --git a/src/main/java/fr/xephi/authme/AuthMe.java b/src/main/java/fr/xephi/authme/AuthMe.java index 3b037bfb1..77f419b00 100644 --- a/src/main/java/fr/xephi/authme/AuthMe.java +++ b/src/main/java/fr/xephi/authme/AuthMe.java @@ -338,7 +338,7 @@ public class AuthMe extends JavaPlugin { } database.reload(); messages.reload(newSettings.getMessagesFile()); - passwordSecurity.reload(newSettings); + passwordSecurity.reload(); spawnLoader.initialize(newSettings); } diff --git a/src/main/java/fr/xephi/authme/converter/RakamakConverter.java b/src/main/java/fr/xephi/authme/converter/RakamakConverter.java index 289e11beb..79de45ad7 100644 --- a/src/main/java/fr/xephi/authme/converter/RakamakConverter.java +++ b/src/main/java/fr/xephi/authme/converter/RakamakConverter.java @@ -4,7 +4,6 @@ import fr.xephi.authme.AuthMe; import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.cache.auth.PlayerAuth; import fr.xephi.authme.datasource.DataSource; -import fr.xephi.authme.security.HashAlgorithm; import fr.xephi.authme.security.PasswordSecurity; import fr.xephi.authme.security.crypts.HashedPassword; import fr.xephi.authme.settings.Settings; @@ -35,7 +34,6 @@ public class RakamakConverter implements Converter { @Override // TODO ljacqu 20151229: Restructure this into smaller portions public void run() { - HashAlgorithm hash = Settings.getPasswordHash; boolean useIP = Settings.rakamakUseIp; String fileName = Settings.rakamakUsers; String ipFileName = Settings.rakamakUsersIp; @@ -64,7 +62,7 @@ public class RakamakConverter implements Converter { while ((line = users.readLine()) != null) { if (line.contains("=")) { String[] arguments = line.split("="); - HashedPassword hashedPassword = passwordSecurity.computeHash(hash, arguments[1], arguments[0]); + HashedPassword hashedPassword = passwordSecurity.computeHash(arguments[1], arguments[0]); playerPSW.put(arguments[0], hashedPassword); } diff --git a/src/main/java/fr/xephi/authme/security/PasswordSecurity.java b/src/main/java/fr/xephi/authme/security/PasswordSecurity.java index da506b6bb..94a8abeb4 100644 --- a/src/main/java/fr/xephi/authme/security/PasswordSecurity.java +++ b/src/main/java/fr/xephi/authme/security/PasswordSecurity.java @@ -8,46 +8,75 @@ import fr.xephi.authme.settings.NewSetting; import fr.xephi.authme.settings.properties.SecuritySettings; import org.bukkit.plugin.PluginManager; +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; + /** * Manager class for password-related operations. */ public class PasswordSecurity { + private final NewSetting settings; private HashAlgorithm algorithm; private boolean supportOldAlgorithm; private final DataSource dataSource; private final PluginManager pluginManager; public PasswordSecurity(DataSource dataSource, NewSetting settings, PluginManager pluginManager) { + this.settings = settings; this.algorithm = settings.getProperty(SecuritySettings.PASSWORD_HASH); this.supportOldAlgorithm = settings.getProperty(SecuritySettings.SUPPORT_OLD_PASSWORD_HASH); this.dataSource = dataSource; this.pluginManager = pluginManager; } + /** + * Compute the hash of the configured algorithm for the given password and username. + * + * @param password The password to hash + * @param playerName The player's name + * + * @return The password hash + */ public HashedPassword computeHash(String password, String playerName) { - return computeHash(algorithm, password, playerName); - } - - public HashedPassword computeHash(HashAlgorithm algorithm, String password, String playerName) { String playerLowerCase = playerName.toLowerCase(); - EncryptionMethod method = initializeEncryptionMethod(algorithm, playerLowerCase); + EncryptionMethod method = initializeEncryptionMethodWithEvent(algorithm, playerLowerCase); return method.computeHash(password, playerLowerCase); } + /** + * Check if the given password matches the player's stored password. + * + * @param password The password to check + * @param playerName The player to check for + * + * @return True if the password is correct, false otherwise + */ public boolean comparePassword(String password, String playerName) { HashedPassword auth = dataSource.getPassword(playerName); return auth != null && comparePassword(password, auth, playerName); } + /** + * Check if the given password matches the given hashed password. + * + * @param password The password to check + * @param hashedPassword The hashed password to check against + * @param playerName The player to check for + * + * @return True if the password matches, false otherwise + */ public boolean comparePassword(String password, HashedPassword hashedPassword, String playerName) { - EncryptionMethod method = initializeEncryptionMethod(algorithm, playerName); + EncryptionMethod method = initializeEncryptionMethodWithEvent(algorithm, playerName); String playerLowerCase = playerName.toLowerCase(); return methodMatches(method, password, hashedPassword, playerLowerCase) || supportOldAlgorithm && compareWithAllEncryptionMethods(password, hashedPassword, playerLowerCase); } - public void reload(NewSetting settings) { + /** + * Reload the configuration. + */ + public void reload() { this.algorithm = settings.getProperty(SecuritySettings.PASSWORD_HASH); this.supportOldAlgorithm = settings.getProperty(SecuritySettings.SUPPORT_OLD_PASSWORD_HASH); } @@ -63,11 +92,10 @@ public class PasswordSecurity { * * @return True if there was a password match with another encryption method, false otherwise */ - private boolean compareWithAllEncryptionMethods(String password, HashedPassword hashedPassword, - String playerName) { + private boolean compareWithAllEncryptionMethods(String password, HashedPassword hashedPassword, String playerName) { for (HashAlgorithm algorithm : HashAlgorithm.values()) { if (!HashAlgorithm.CUSTOM.equals(algorithm)) { - EncryptionMethod method = initializeEncryptionMethodWithoutEvent(algorithm); + EncryptionMethod method = initializeEncryptionMethod(algorithm, settings); if (methodMatches(method, password, hashedPassword, playerName)) { hashPasswordForNewAlgorithm(password, playerName); return true; @@ -85,6 +113,7 @@ public class PasswordSecurity { * @param password The password to check * @param hashedPassword The hash to check against * @param playerName The name of the player + * * @return True if the password matched, false otherwise */ private static boolean methodMatches(EncryptionMethod method, String password, @@ -103,33 +132,45 @@ public class PasswordSecurity { * * @return The encryption method */ - private EncryptionMethod initializeEncryptionMethod(HashAlgorithm algorithm, String playerName) { - EncryptionMethod method = initializeEncryptionMethodWithoutEvent(algorithm); + private EncryptionMethod initializeEncryptionMethodWithEvent(HashAlgorithm algorithm, String playerName) { + EncryptionMethod method = initializeEncryptionMethod(algorithm, settings); PasswordEncryptionEvent event = new PasswordEncryptionEvent(method, playerName); pluginManager.callEvent(event); return event.getMethod(); } /** - * Initialize the encryption method corresponding to the given hash algorithm. + * Initialize the encryption method associated with the given hash algorithm. * * @param algorithm The algorithm to retrieve the encryption method for + * @param settings The settings instance to pass to the constructor if required * - * @return The associated encryption method + * @return The associated encryption method, or null if CUSTOM / deprecated */ - private static EncryptionMethod initializeEncryptionMethodWithoutEvent(HashAlgorithm algorithm) { + public static EncryptionMethod initializeEncryptionMethod(HashAlgorithm algorithm, + NewSetting settings) { try { - return HashAlgorithm.CUSTOM.equals(algorithm) || HashAlgorithm.PLAINTEXT.equals(algorithm) - ? null - : algorithm.getClazz().newInstance(); - } catch (InstantiationException | IllegalAccessException e) { + if (HashAlgorithm.CUSTOM.equals(algorithm) || HashAlgorithm.PLAINTEXT.equals(algorithm)) { + return null; + } + Constructor> constructor = algorithm.getClazz().getConstructors()[0]; + Class>[] parameters = constructor.getParameterTypes(); + if (parameters.length == 0) { + return (EncryptionMethod) constructor.newInstance(); + } else if (parameters.length == 1 && parameters[0] == NewSetting.class) { + return (EncryptionMethod) constructor.newInstance(settings); + } else { + throw new UnsupportedOperationException("Did not find default constructor or constructor with settings " + + "parameter in class " + algorithm.getClazz().getSimpleName()); + } + } catch (InstantiationException | IllegalAccessException | InvocationTargetException e) { throw new UnsupportedOperationException("Constructor for '" + algorithm.getClazz().getSimpleName() + "' could not be invoked. (Is there no default constructor?)", e); } } private void hashPasswordForNewAlgorithm(String password, String playerName) { - HashedPassword hashedPassword = initializeEncryptionMethod(algorithm, playerName) + HashedPassword hashedPassword = initializeEncryptionMethodWithEvent(algorithm, playerName) .computeHash(password, playerName); dataSource.updatePassword(playerName, hashedPassword); } diff --git a/src/main/java/fr/xephi/authme/security/crypts/BCRYPT.java b/src/main/java/fr/xephi/authme/security/crypts/BCRYPT.java index 181571b35..a114adc09 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/BCRYPT.java +++ b/src/main/java/fr/xephi/authme/security/crypts/BCRYPT.java @@ -5,7 +5,8 @@ import fr.xephi.authme.security.crypts.description.HasSalt; import fr.xephi.authme.security.crypts.description.Recommendation; import fr.xephi.authme.security.crypts.description.SaltType; import fr.xephi.authme.security.crypts.description.Usage; -import fr.xephi.authme.settings.Settings; +import fr.xephi.authme.settings.NewSetting; +import fr.xephi.authme.settings.properties.HooksSettings; import fr.xephi.authme.util.StringUtils; @@ -13,6 +14,12 @@ import fr.xephi.authme.util.StringUtils; @HasSalt(value = SaltType.TEXT) // length depends on Settings.bCryptLog2Rounds public class BCRYPT implements EncryptionMethod { + private final int bCryptLog2Rounds; + + public BCRYPT(NewSetting settings) { + this.bCryptLog2Rounds = settings.getProperty(HooksSettings.BCRYPT_LOG2_ROUND); + } + @Override public String computeHash(String password, String salt, String name) { return BCryptService.hashpw(password, salt); @@ -36,7 +43,7 @@ public class BCRYPT implements EncryptionMethod { @Override public String generateSalt() { - return BCryptService.gensalt(Settings.bCryptLog2Rounds); + return BCryptService.gensalt(bCryptLog2Rounds); } @Override diff --git a/src/main/java/fr/xephi/authme/security/crypts/EncryptionMethod.java b/src/main/java/fr/xephi/authme/security/crypts/EncryptionMethod.java index 26efc1561..b1c0b0034 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/EncryptionMethod.java +++ b/src/main/java/fr/xephi/authme/security/crypts/EncryptionMethod.java @@ -2,6 +2,10 @@ package fr.xephi.authme.security.crypts; /** * Public interface for custom password encryption methods. + *
+ * Note that {@link fr.xephi.authme.security.PasswordSecurity} requires classes implementing this interface
+ * to either have the default constructor or an accessible constructor with one parameter of type
+ * {@link fr.xephi.authme.settings.NewSetting}.
*/
public interface EncryptionMethod {
@@ -31,9 +35,9 @@ public interface EncryptionMethod {
/**
* Check whether the given hash matches the clear-text password.
*
- * @param password The clear-text password to verify
+ * @param password The clear-text password to verify
* @param hashedPassword The hash to check the password against
- * @param name The player name to do the check for (sometimes required for generating the salt)
+ * @param name The player name to do the check for (sometimes required for generating the salt)
*
* @return True if the password matches, false otherwise
*/
diff --git a/src/main/java/fr/xephi/authme/security/crypts/SALTED2MD5.java b/src/main/java/fr/xephi/authme/security/crypts/SALTED2MD5.java
index 613bcd3f8..27066f7a2 100644
--- a/src/main/java/fr/xephi/authme/security/crypts/SALTED2MD5.java
+++ b/src/main/java/fr/xephi/authme/security/crypts/SALTED2MD5.java
@@ -5,7 +5,8 @@ import fr.xephi.authme.security.crypts.description.HasSalt;
import fr.xephi.authme.security.crypts.description.Recommendation;
import fr.xephi.authme.security.crypts.description.SaltType;
import fr.xephi.authme.security.crypts.description.Usage;
-import fr.xephi.authme.settings.Settings;
+import fr.xephi.authme.settings.NewSetting;
+import fr.xephi.authme.settings.properties.SecuritySettings;
import static fr.xephi.authme.security.HashUtils.md5;
@@ -13,6 +14,12 @@ import static fr.xephi.authme.security.HashUtils.md5;
@HasSalt(value = SaltType.TEXT) // length defined by Settings.saltLength
public class SALTED2MD5 extends SeparateSaltMethod {
+ private final int saltLength;
+
+ public SALTED2MD5(NewSetting settings) {
+ saltLength = settings.getProperty(SecuritySettings.DOUBLE_MD5_SALT_LENGTH);
+ }
+
@Override
public String computeHash(String password, String salt, String name) {
return md5(md5(password) + salt);
@@ -20,7 +27,7 @@ public class SALTED2MD5 extends SeparateSaltMethod {
@Override
public String generateSalt() {
- return RandomString.generateHex(Settings.saltLength);
+ return RandomString.generateHex(saltLength);
}
}
diff --git a/src/main/java/fr/xephi/authme/settings/Settings.java b/src/main/java/fr/xephi/authme/settings/Settings.java
index 7b6ce411b..b5d5c3872 100644
--- a/src/main/java/fr/xephi/authme/settings/Settings.java
+++ b/src/main/java/fr/xephi/authme/settings/Settings.java
@@ -45,8 +45,7 @@ public final class Settings {
rakamakUsers, rakamakUsersIp, defaultWorld, crazyloginFileName;
public static int getWarnMessageInterval, getSessionTimeout,
getRegistrationTimeout, getMaxNickLength, getMinNickLength,
- getNonActivatedGroup, maxLoginTry, captchaLength, saltLength,
- bCryptLog2Rounds, getMaxLoginPerIp;
+ getNonActivatedGroup, maxLoginTry, captchaLength, getMaxLoginPerIp;
protected static FileConfiguration configFile;
/**
@@ -108,11 +107,9 @@ public final class Settings {
removePassword = configFile.getBoolean("Security.console.removePassword", true);
maxLoginTry = configFile.getInt("Security.captcha.maxLoginTry", 5);
captchaLength = configFile.getInt("Security.captcha.captchaLength", 5);
- saltLength = configFile.getInt("settings.security.doubleMD5SaltLength", 8);
multiverse = load(HooksSettings.MULTIVERSE);
bungee = load(HooksSettings.BUNGEECORD);
getForcedWorlds = configFile.getStringList("settings.restrictions.ForceSpawnOnTheseWorlds");
- bCryptLog2Rounds = configFile.getInt("ExternalBoardOptions.bCryptLog2Round", 10);
defaultWorld = configFile.getString("Purge.defaultWorld", "world");
enableProtection = configFile.getBoolean("Protection.enableProtection", false);
countries = configFile.getStringList("Protection.countries");
diff --git a/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java b/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java
index 2472c6592..976a7a445 100644
--- a/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java
+++ b/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java
@@ -1,10 +1,11 @@
package fr.xephi.authme.security;
-import fr.xephi.authme.security.crypts.HashedPassword;
import fr.xephi.authme.security.crypts.EncryptionMethod;
-import fr.xephi.authme.settings.Settings;
+import fr.xephi.authme.security.crypts.HashedPassword;
+import fr.xephi.authme.settings.NewSetting;
+import fr.xephi.authme.settings.properties.HooksSettings;
+import fr.xephi.authme.settings.properties.SecuritySettings;
import fr.xephi.authme.util.StringUtils;
-import fr.xephi.authme.util.WrapperMock;
import org.junit.BeforeClass;
import org.junit.Test;
@@ -12,19 +13,25 @@ import java.util.HashSet;
import java.util.Set;
import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.mock;
/**
* Integration test for {@link HashAlgorithm}.
*/
public class HashAlgorithmIntegrationTest {
+ private static NewSetting settings;
+
@BeforeClass
public static void setUpWrapper() {
- WrapperMock.createInstance();
- Settings.bCryptLog2Rounds = 8;
- Settings.saltLength = 16;
+ settings = mock(NewSetting.class);
+ given(settings.getProperty(HooksSettings.BCRYPT_LOG2_ROUND)).willReturn(8);
+ given(settings.getProperty(SecuritySettings.DOUBLE_MD5_SALT_LENGTH)).willReturn(16);
}
@Test
@@ -47,8 +54,10 @@ public class HashAlgorithmIntegrationTest {
public void shouldBeAbleToInstantiateEncryptionAlgorithms() throws InstantiationException, IllegalAccessException {
// given / when / then
for (HashAlgorithm algorithm : HashAlgorithm.values()) {
- if (!HashAlgorithm.CUSTOM.equals(algorithm)) {
- EncryptionMethod method = algorithm.getClazz().newInstance();
+ if (!HashAlgorithm.CUSTOM.equals(algorithm) && !HashAlgorithm.PLAINTEXT.equals(algorithm)) {
+ EncryptionMethod method = PasswordSecurity.initializeEncryptionMethod(algorithm, settings);
+ assertThat("Encryption method for algorithm '" + algorithm + "' is not null",
+ method, not(nullValue()));
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 de223e6f2..470b8a2d5 100644
--- a/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java
+++ b/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java
@@ -1,22 +1,25 @@
package fr.xephi.authme.security;
import fr.xephi.authme.ReflectionTestUtils;
-import fr.xephi.authme.cache.auth.PlayerAuth;
+import fr.xephi.authme.TestHelper;
import fr.xephi.authme.datasource.DataSource;
import fr.xephi.authme.events.PasswordEncryptionEvent;
-import fr.xephi.authme.security.crypts.HashedPassword;
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.PHPBB;
import fr.xephi.authme.settings.NewSetting;
+import fr.xephi.authme.settings.properties.HooksSettings;
import fr.xephi.authme.settings.properties.SecuritySettings;
-import fr.xephi.authme.util.WrapperMock;
import org.bukkit.event.Event;
import org.bukkit.plugin.PluginManager;
import org.junit.Before;
+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.runners.MockitoJUnitRunner;
import org.mockito.stubbing.Answer;
import static org.hamcrest.Matchers.equalTo;
@@ -35,19 +38,24 @@ import static org.mockito.Mockito.verify;
/**
* Test for {@link PasswordSecurity}.
*/
+@RunWith(MockitoJUnitRunner.class)
public class PasswordSecurityTest {
+ @Mock
private PluginManager pluginManager;
+ @Mock
private DataSource dataSource;
+ @Mock
private EncryptionMethod method;
private Class> caughtClassInEvent;
+ @BeforeClass
+ public static void setUpTest() {
+ TestHelper.setupLogger();
+ }
+
@Before
public void setUpMocks() {
- WrapperMock.createInstance();
- pluginManager = mock(PluginManager.class);
- dataSource = mock(DataSource.class);
- method = mock(EncryptionMethod.class);
caughtClassInEvent = null;
// When the password encryption event is emitted, replace the encryption method with our mock.
@@ -97,7 +105,6 @@ public class PasswordSecurityTest {
String playerLowerCase = playerName.toLowerCase();
String clearTextPass = "passw0Rd1";
- PlayerAuth auth = mock(PlayerAuth.class);
given(dataSource.getPassword(playerName)).willReturn(password);
given(method.comparePassword(clearTextPass, password, playerLowerCase)).willReturn(false);
PasswordSecurity security =
@@ -165,6 +172,25 @@ public class PasswordSecurityTest {
verify(dataSource).updatePassword(playerLowerCase, newPassword);
}
+ @Test
+ public void shouldTryAllMethodsAndFail() {
+ // given
+ HashedPassword password = new HashedPassword("hashNotMatchingAnyMethod", "someBogusSalt");
+ String playerName = "asfd";
+ String clearTextPass = "someInvalidPassword";
+ given(dataSource.getPassword(playerName)).willReturn(password);
+ given(method.comparePassword(clearTextPass, password, playerName)).willReturn(false);
+ PasswordSecurity security =
+ new PasswordSecurity(dataSource, mockSettings(HashAlgorithm.MD5, true), pluginManager);
+
+ // when
+ boolean result = security.comparePassword(clearTextPass, playerName);
+
+ // then
+ assertThat(result, equalTo(false));
+ verify(dataSource, never()).updatePassword(anyString(), any(HashedPassword.class));
+ }
+
@Test
public void shouldHashPassword() {
// given
@@ -188,28 +214,6 @@ public class PasswordSecurityTest {
assertThat(event.getPlayerName(), equalTo(usernameLowerCase));
}
- @Test
- public void shouldHashPasswordWithGivenAlgorithm() {
- // given
- String password = "TopSecretPass#112525";
- String username = "someone12";
- HashedPassword hashedPassword = new HashedPassword("~T!est#Hash", "__someSalt__");
- given(method.computeHash(password, username)).willReturn(hashedPassword);
- PasswordSecurity security =
- new PasswordSecurity(dataSource, mockSettings(HashAlgorithm.JOOMLA, true), pluginManager);
-
- // when
- HashedPassword result = security.computeHash(HashAlgorithm.PHPBB, password, username);
-
- // then
- assertThat(result, equalTo(hashedPassword));
- ArgumentCaptor