#850 Add setting specifying which password hashes should be checked

Author: ljacqu

docs/config.md

@@ -1,5 +1,5 @@
 <!-- AUTO-GENERATED FILE! Do not edit this directly -->
-<!-- File auto-generated on Sun Oct 30 12:57:15 CET 2016. See docs/config/config.tpl.md -->
+<!-- File auto-generated on Sun Nov 13 10:33:55 CET 2016. See docs/config/config.tpl.md -->
 
 ## AuthMe Configuration
 The first time you run AuthMe it will create a config.yml file in the plugins/AuthMe folder, 
@@ -245,10 +245,12 @@ settings:
         passwordHash: 'SHA256'
         # Salt length for the SALTED2MD5 MD5(MD5(password)+salt)
         doubleMD5SaltLength: 8
-        # If password checking return false, do we need to check with all
-        # other password algorithm to check an old password?
-        # AuthMe will update the password to the new password hash
-        supportOldPasswordHash: false
+        # 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.
+        # AuthMe will update the password to the new hash. Example:
+        # legacyHashes:
+        # - 'SHA1'
+        legacyHashes: []
         # Prevent unsafe passwords from being used; put them in lowercase!
         # You should always set 'help' as unsafePassword due to possible conflicts.
         # unsafePasswords:
@@ -461,4 +463,4 @@ To change settings on a running server, save your changes to config.yml and use
 
 ---
 
-This page was automatically generated on the [AuthMe/AuthMeReloaded repository](https://github.com/AuthMe/AuthMeReloaded/tree/master/docs/) on Sun Oct 30 12:57:15 CET 2016
+This page was automatically generated on the [AuthMe/AuthMeReloaded repository](https://github.com/AuthMe/AuthMeReloaded/tree/master/docs/) on Sun Nov 13 10:33:55 CET 2016

src/main/java/fr/xephi/authme/security/PasswordSecurity.java

@@ -12,6 +12,7 @@
 
 import javax.annotation.PostConstruct;
 import javax.inject.Inject;
+import java.util.Collection;
 
 /**
  * Manager class for password-related operations.
@@ -31,7 +32,7 @@ public class PasswordSecurity implements Reloadable {
     private Injector injector;
 
     private HashAlgorithm algorithm;
-    private boolean supportOldAlgorithm;
+    private Collection<HashAlgorithm> legacyAlgorithms;
 
     /**
      * Load or reload the configuration.
@@ -40,7 +41,7 @@ public class PasswordSecurity implements Reloadable {
     @Override
     public void reload() {
         this.algorithm = settings.getProperty(SecuritySettings.PASSWORD_HASH);
-        this.supportOldAlgorithm = settings.getProperty(SecuritySettings.SUPPORT_OLD_PASSWORD_HASH);
+        this.legacyAlgorithms = settings.getProperty(SecuritySettings.LEGACY_HASHES);
     }
 
     /**
@@ -83,7 +84,7 @@ public boolean comparePassword(String password, HashedPassword hashedPassword, S
         EncryptionMethod method = initializeEncryptionMethodWithEvent(algorithm, playerName);
         String playerLowerCase = playerName.toLowerCase();
         return methodMatches(method, password, hashedPassword, playerLowerCase)
-            || supportOldAlgorithm && compareWithAllEncryptionMethods(password, hashedPassword, playerLowerCase);
+            || compareWithLegacyHashes(password, hashedPassword, playerLowerCase);
     }
 
     /**
@@ -97,14 +98,12 @@ public boolean comparePassword(String password, HashedPassword hashedPassword, S
      *
      * @return True if there was a password match with another encryption method, false otherwise
      */
-    private boolean compareWithAllEncryptionMethods(String password, HashedPassword hashedPassword, String playerName) {
-        for (HashAlgorithm algorithm : HashAlgorithm.values()) {
-            if (!HashAlgorithm.CUSTOM.equals(algorithm)) {
-                EncryptionMethod method = initializeEncryptionMethod(algorithm);
-                if (methodMatches(method, password, hashedPassword, playerName)) {
-                    hashPasswordForNewAlgorithm(password, playerName);
-                    return true;
-                }
+    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);
+                return true;
             }
         }
         return false;

src/main/java/fr/xephi/authme/settings/EnumSetProperty.java

@@ -0,0 +1,43 @@
+package fr.xephi.authme.settings;
+
+import com.github.authme.configme.properties.Property;
+import com.github.authme.configme.resource.PropertyResource;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * Property whose value is a set of entries of a given enum.
+ */
+// TODO https://github.com/AuthMe/ConfigMe/issues/27: Should be a Property of Set<E> type
+public class EnumSetProperty<E extends Enum<E>> extends Property<List<E>> {
+
+    private final Class<E> enumClass;
+
+    public EnumSetProperty(Class<E> enumClass, String path, List<E> defaultValue) {
+        super(path, defaultValue);
+        this.enumClass = enumClass;
+    }
+
+    @Override
+    protected List<E> getFromResource(PropertyResource resource) {
+        List<?> elements = resource.getList(getPath());
+        if (elements != null) {
+            return elements.stream()
+                .map(val -> toEnum(String.valueOf(val)))
+                .filter(e -> e != null)
+                .distinct()
+                .collect(Collectors.toList());
+        }
+        return null;
+    }
+
+    private E toEnum(String str) {
+        for (E e : enumClass.getEnumConstants()) {
+            if (str.equalsIgnoreCase(e.name())) {
+                return e;
+            }
+        }
+        return null;
+    }
+}

src/main/java/fr/xephi/authme/settings/SettingsMigrationService.java

@@ -7,6 +7,7 @@
 import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.output.LogLevel;
 import fr.xephi.authme.settings.properties.PluginSettings;
+import fr.xephi.authme.settings.properties.SecuritySettings;
 
 import java.io.File;
 import java.io.FileWriter;
@@ -49,6 +50,7 @@ protected boolean performMigrations(PropertyResource resource, List<Property<?>>
             | migrateForceSpawnSettings(resource)
             | changeBooleanSettingToLogLevelProperty(resource)
             | hasOldHelpHeaderProperty(resource)
+            | hasSupportOldPasswordProperty(resource)
             || hasDeprecatedProperties(resource);
     }
 
@@ -163,6 +165,16 @@ private static boolean hasOldHelpHeaderProperty(PropertyResource resource) {
         return false;
     }
 
+    private static boolean hasSupportOldPasswordProperty(PropertyResource resource) {
+        String path = "settings.security.supportOldPasswordHash";
+        if (resource.contains(path)) {
+            ConsoleLogger.warning("Property '" + path + "' is no longer supported. "
+                + "Use '" + SecuritySettings.LEGACY_HASHES.getPath() + "' instead.");
+            return true;
+        }
+        return false;
+    }
+
     /**
      * Checks for an old property path and moves it to a new path if present.
      *

src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java

@@ -4,7 +4,9 @@
 import com.github.authme.configme.SettingsHolder;
 import com.github.authme.configme.properties.Property;
 import fr.xephi.authme.security.HashAlgorithm;
+import fr.xephi.authme.settings.EnumSetProperty;
 
+import java.util.Collections;
 import java.util.List;
 
 import static com.github.authme.configme.properties.PropertyInitializer.newLowercaseListProperty;
@@ -74,11 +76,15 @@ public class SecuritySettings implements SettingsHolder {
     public static final Property<Integer> DOUBLE_MD5_SALT_LENGTH =
         newProperty("settings.security.doubleMD5SaltLength", 8);
 
-    @Comment({"If password checking return false, do we need to check with all",
-        "other password algorithm to check an old password?",
-        "AuthMe will update the password to the new password hash"})
-    public static final Property<Boolean> SUPPORT_OLD_PASSWORD_HASH =
-        newProperty("settings.security.supportOldPasswordHash", false);
+    @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.",
+        "AuthMe will update the password to the new hash. Example:",
+        "legacyHashes:",
+        "- 'SHA1'"
+    })
+    public static final Property<List<HashAlgorithm>> LEGACY_HASHES =
+        new EnumSetProperty<>(HashAlgorithm.class, "settings.security.legacyHashes", Collections.emptyList());
 
     @Comment({"Prevent unsafe passwords from being used; put them in lowercase!",
         "You should always set 'help' as unsafePassword due to possible conflicts.",

src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java

@@ -24,6 +24,10 @@
 import org.mockito.runners.MockitoJUnitRunner;
 import org.mockito.stubbing.Answer;
 
+import java.util.Collections;
+import java.util.List;
+
+import static com.google.common.collect.Lists.newArrayList;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.equalToIgnoringCase;
 import static org.junit.Assert.assertThat;
@@ -97,7 +101,7 @@ public void shouldReturnPasswordMatch() {
 
         given(dataSource.getPassword(playerName)).willReturn(password);
         given(method.comparePassword(clearTextPass, password, playerLowerCase)).willReturn(true);
-        initSettings(HashAlgorithm.BCRYPT, false);
+        initSettings(HashAlgorithm.BCRYPT);
         PasswordSecurity security = newPasswordSecurity();
 
         // when
@@ -120,7 +124,7 @@ public void shouldReturnPasswordMismatch() {
 
         given(dataSource.getPassword(playerName)).willReturn(password);
         given(method.comparePassword(clearTextPass, password, playerLowerCase)).willReturn(false);
-        initSettings(HashAlgorithm.CUSTOM, false);
+        initSettings(HashAlgorithm.CUSTOM);
         PasswordSecurity security = newPasswordSecurity();
 
         // when
@@ -140,7 +144,7 @@ public void shouldReturnFalseIfPlayerDoesNotExist() {
         String clearTextPass = "tables";
 
         given(dataSource.getPassword(playerName)).willReturn(null);
-        initSettings(HashAlgorithm.MD5, false);
+        initSettings(HashAlgorithm.MD5);
         PasswordSecurity security = newPasswordSecurity();
 
         // when
@@ -168,7 +172,8 @@ public void shouldTryOtherMethodsForFailedPassword() {
         given(dataSource.getPassword(argThat(equalToIgnoringCase(playerName)))).willReturn(password);
         given(method.comparePassword(clearTextPass, password, playerLowerCase)).willReturn(false);
         given(method.computeHash(clearTextPass, playerLowerCase)).willReturn(newPassword);
-        initSettings(HashAlgorithm.MD5, true);
+        initSettings(HashAlgorithm.MD5);
+        given(settings.getProperty(SecuritySettings.LEGACY_HASHES)).willReturn(newArrayList(HashAlgorithm.BCRYPT));
         PasswordSecurity security = newPasswordSecurity();
 
         // when
@@ -193,7 +198,7 @@ public void shouldTryAllMethodsAndFail() {
         String clearTextPass = "someInvalidPassword";
         given(dataSource.getPassword(playerName)).willReturn(password);
         given(method.comparePassword(clearTextPass, password, playerName)).willReturn(false);
-        initSettings(HashAlgorithm.MD5, true);
+        initSettings(HashAlgorithm.MD5);
         PasswordSecurity security = newPasswordSecurity();
 
         // when
@@ -212,7 +217,7 @@ public void shouldHashPassword() {
         String usernameLowerCase = username.toLowerCase();
         HashedPassword hashedPassword = new HashedPassword("$T$est#Hash", "__someSalt__");
         given(method.computeHash(password, usernameLowerCase)).willReturn(hashedPassword);
-        initSettings(HashAlgorithm.JOOMLA, true);
+        initSettings(HashAlgorithm.JOOMLA);
         PasswordSecurity security = newPasswordSecurity();
 
         // when
@@ -234,7 +239,7 @@ public void shouldSkipCheckIfMandatorySaltIsUnavailable() {
         String username = "someone12";
         HashedPassword hashedPassword = new HashedPassword("~T!est#Hash");
         given(method.hasSeparateSalt()).willReturn(true);
-        initSettings(HashAlgorithm.XAUTH, false);
+        initSettings(HashAlgorithm.XAUTH);
         PasswordSecurity security = newPasswordSecurity();
 
         // when
@@ -250,19 +255,20 @@ public void shouldSkipCheckIfMandatorySaltIsUnavailable() {
     @Test
     public void shouldReloadSettings() {
         // given
-        initSettings(HashAlgorithm.BCRYPT, false);
+        initSettings(HashAlgorithm.BCRYPT);
         PasswordSecurity passwordSecurity = newPasswordSecurity();
         given(settings.getProperty(SecuritySettings.PASSWORD_HASH)).willReturn(HashAlgorithm.MD5);
-        given(settings.getProperty(SecuritySettings.SUPPORT_OLD_PASSWORD_HASH)).willReturn(true);
+        List<HashAlgorithm> legacyHashes = newArrayList(HashAlgorithm.CUSTOM, HashAlgorithm.BCRYPT);
+        given(settings.getProperty(SecuritySettings.LEGACY_HASHES)).willReturn(legacyHashes);
 
         // when
         passwordSecurity.reload();
 
         // then
         assertThat(ReflectionTestUtils.getFieldValue(PasswordSecurity.class, passwordSecurity, "algorithm"),
-            equalTo((Object) HashAlgorithm.MD5));
-        assertThat(ReflectionTestUtils.getFieldValue(PasswordSecurity.class, passwordSecurity, "supportOldAlgorithm"),
-            equalTo((Object) Boolean.TRUE));
+            equalTo(HashAlgorithm.MD5));
+        assertThat(ReflectionTestUtils.getFieldValue(PasswordSecurity.class, passwordSecurity, "legacyAlgorithms"),
+            equalTo(legacyHashes));
     }
 
     private PasswordSecurity newPasswordSecurity() {
@@ -275,11 +281,10 @@ private PasswordSecurity newPasswordSecurity() {
         return passwordSecurity;
     }
 
-    private void initSettings(HashAlgorithm algorithm, boolean supportOldPassword) {
+    private void initSettings(HashAlgorithm algorithm) {
         given(settings.getProperty(SecuritySettings.PASSWORD_HASH)).willReturn(algorithm);
-        given(settings.getProperty(SecuritySettings.SUPPORT_OLD_PASSWORD_HASH)).willReturn(supportOldPassword);
+        given(settings.getProperty(SecuritySettings.LEGACY_HASHES)).willReturn(Collections.emptyList());
         given(settings.getProperty(HooksSettings.BCRYPT_LOG2_ROUND)).willReturn(8);
-        given(settings.getProperty(SecuritySettings.DOUBLE_MD5_SALT_LENGTH)).willReturn(16);
     }
 
 }