#358 Remove old methods on PasswordSecurity, unify hash + salt

- For encryption methods with a separate salt, the hash is useless without the salt, so hash and salt should always be persisted and retrieved together

src/main/java/fr/xephi/authme/api/API.java

@@ -121,13 +121,8 @@ public class API {
      * @return true if the password is correct, false otherwise
      */
     @Deprecated
-    public static boolean checkPassword(String playerName,
+    public static boolean checkPassword(String playerName, String passwordToCheck) {
-                                        String passwordToCheck) {
+        return isRegistered(playerName) && passwordSecurity.comparePassword(passwordToCheck, playerName);
-        if (!isRegistered(playerName))
-            return false;
-        String player = playerName.toLowerCase();
-        PlayerAuth auth = instance.getDataSource().getAuth(player);
-        return passwordSecurity.comparePassword(passwordToCheck, auth.getHash(), playerName);
     }
 
     /**

src/main/java/fr/xephi/authme/api/NewAPI.java

@@ -116,9 +116,11 @@ public class NewAPI {
     }
 
     /**
-     * @param playerName
+     * Return whether the player is registered.
      *
-     * @return true if player is registered
+     * @param playerName The player name to check
+     *
+     * @return true if player is registered, false otherwise
      */
     public boolean isRegistered(String playerName) {
         String player = playerName.toLowerCase();
@@ -134,12 +136,7 @@ public class NewAPI {
      * @return true if the password is correct, false otherwise
      */
     public boolean checkPassword(String playerName, String passwordToCheck) {
-        if (!isRegistered(playerName)) {
+        return isRegistered(playerName) && plugin.getPasswordSecurity().comparePassword(passwordToCheck, playerName);
-            return false;
-        }
-        String player = playerName.toLowerCase();
-        PlayerAuth auth = plugin.getDataSource().getAuth(player);
-        return plugin.getPasswordSecurity().comparePassword(passwordToCheck, auth.getHash(), playerName);
     }
 
     /**

src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java

@@ -136,9 +136,9 @@ public class AsynchronousLogin {
         if (pAuth == null || needsCaptcha())
             return;
 
-        String hash = pAuth.getHash();
         String email = pAuth.getEmail();
-        boolean passwordVerified = forceLogin || plugin.getPasswordSecurity().comparePassword(password, hash, realName);
+        boolean passwordVerified = forceLogin || plugin.getPasswordSecurity()
+            .comparePassword(password, pAuth.getHash(), pAuth.getSalt(), realName);
 
         if (passwordVerified && player.isOnline()) {
             PlayerAuth auth = PlayerAuth.builder()
@@ -147,7 +147,7 @@ public class AsynchronousLogin {
                 .ip(getIP())
                 .lastLogin(new Date().getTime())
                 .email(email)
-                .hash(hash)
+                .hash(pAuth.getHash())
                 .salt(pAuth.getSalt())
                 .build();
             database.updateSession(auth);

src/main/java/fr/xephi/authme/process/unregister/AsynchronousUnregister.java

@@ -2,6 +2,7 @@ package fr.xephi.authme.process.unregister;
 
 import fr.xephi.authme.AuthMe;
 import fr.xephi.authme.ConsoleLogger;
+import fr.xephi.authme.cache.auth.PlayerAuth;
 import fr.xephi.authme.cache.auth.PlayerCache;
 import fr.xephi.authme.cache.backup.JsonCache;
 import fr.xephi.authme.cache.limbo.LimboCache;
@@ -52,7 +53,9 @@ public class AsynchronousUnregister {
     }
 
     public void process() {
-        if (force || plugin.getPasswordSecurity().comparePassword(password, PlayerCache.getInstance().getAuth(name).getHash(), player.getName())) {
+        PlayerAuth cachedAuth = PlayerCache.getInstance().getAuth(name);
+        if (force || plugin.getPasswordSecurity().comparePassword(
+            password, cachedAuth.getHash(), cachedAuth.getSalt(), player.getName())) {
             if (!plugin.getDataSource().removeAuth(name)) {
                 m.send(player, MessageKey.ERROR);
                 return;

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

@@ -1,23 +1,19 @@
 package fr.xephi.authme.security;
 
-import fr.xephi.authme.AuthMe;
 import fr.xephi.authme.cache.auth.PlayerAuth;
 import fr.xephi.authme.datasource.DataSource;
 import fr.xephi.authme.events.PasswordEncryptionEvent;
 import fr.xephi.authme.security.crypts.EncryptionMethod;
 import fr.xephi.authme.security.crypts.HashResult;
-import fr.xephi.authme.settings.Settings;
 import org.bukkit.Bukkit;
 
-import java.security.NoSuchAlgorithmException;
 import java.util.HashMap;
 
 /**
+ * Manager class for password-related operations.
  */
 public class PasswordSecurity {
 
-    @Deprecated
-    public static final HashMap<String, String> userSalt = new HashMap<>();
     private final DataSource dataSource;
     private final HashAlgorithm algorithm;
     private final boolean supportOldAlgorithm;
@@ -28,49 +24,6 @@ public class PasswordSecurity {
         this.supportOldAlgorithm = supportOldAlgorithm;
     }
 
-    @Deprecated
-    public static boolean comparePasswordWithHash(String password, String hash,
-                                                  String playerName) throws NoSuchAlgorithmException {
-        HashAlgorithm algorithm = Settings.getPasswordHash;
-        EncryptionMethod method;
-        try {
-            if (algorithm != HashAlgorithm.CUSTOM) {
-                method = algorithm.getClazz().newInstance();
-            } else {
-                method = null;
-            }
-
-            PasswordEncryptionEvent event = new PasswordEncryptionEvent(method, playerName);
-            Bukkit.getPluginManager().callEvent(event);
-            method = event.getMethod();
-
-            if (method == null)
-                throw new NoSuchAlgorithmException("Unknown hash algorithm");
-
-            String salt = null;
-            if (method.hasSeparateSalt()) {
-                PlayerAuth auth = AuthMe.getInstance().getDataSource().getAuth(playerName);
-                if (auth == null) {
-                    // User is not in data source, so the result will invariably be wrong because an encryption
-                    // method with hasSeparateSalt() == true NEEDS the salt to evaluate the password
-                    return false;
-                }
-                salt = auth.getSalt();
-            }
-
-            if (method.comparePassword(hash, password, salt, playerName))
-                return true;
-
-            if (Settings.supportOldPassword) {
-                if (compareWithAllEncryptionMethod(password, hash, playerName))
-                    return true;
-            }
-        } catch (InstantiationException | IllegalAccessException e) {
-            throw new NoSuchAlgorithmException("Problem with this hash algorithm");
-        }
-        return false;
-    }
-
     public HashResult computeHash(String password, String playerName) {
         return computeHash(algorithm, password, playerName);
     }
@@ -80,71 +33,92 @@ public class PasswordSecurity {
         return method.computeHash(password, playerName);
     }
 
-    public boolean comparePassword(String hash, String password, String playerName) {
+    public boolean comparePassword(String password, String playerName) {
-        return comparePassword(algorithm, hash, password, playerName);
+        PlayerAuth auth = dataSource.getAuth(playerName);
+        if (auth != null) {
+            return comparePassword(auth.getHash(), auth.getSalt(), password, playerName);
+        }
+        return false;
     }
 
-    public boolean comparePassword(HashAlgorithm algorithm, String hash, String password, String playerName) {
+    public boolean comparePassword(String hash, String salt, String password, String playerName) {
         EncryptionMethod method = initializeEncryptionMethod(algorithm, playerName);
-        String salt = null;
+        // User is not in data source, so the result will invariably be wrong because an encryption
-        if (method.hasSeparateSalt()) {
+        // method with hasSeparateSalt() == true NEEDS the salt to evaluate the password
-            PlayerAuth auth = dataSource.getAuth(playerName);
+        if (method.hasSeparateSalt() && salt == null) {
-            if (auth == null) {
+            return false;
-                // User is not in data source, so the result will invariably be wrong because an encryption
-                // method with hasSeparateSalt() == true NEEDS the salt to evaluate the password
-                return false;
-            }
-            salt = auth.getSalt();
         }
-        return method.comparePassword(hash, password, salt, playerName);
+        return method.comparePassword(hash, password, salt, playerName)
-        // TODO #358: Add logic for Settings.supportOldPassword
+            || supportOldAlgorithm && compareWithAllEncryptionMethods(password, hash, salt, playerName);
     }
 
-    private EncryptionMethod initializeEncryptionMethod(HashAlgorithm algorithm, String playerName) {
+    /**
-        EncryptionMethod method;
+     * Compare the given hash with all available encryption methods to support the migration to a new encryption method.
-        try {
+     *
-            method = HashAlgorithm.CUSTOM.equals(algorithm)
+     * @param password   The clear-text password to check
-                ? null
+     * @param hash       The hash to text the password against
-                : algorithm.getClazz().newInstance();
+     * @param salt       The salt (or null if none available)
-        } catch (InstantiationException | IllegalAccessException e) {
+     * @param playerName The name of the player
-            throw new IllegalStateException("Constructor for '" + algorithm.getClazz()
+     * @return True if the
-                + "' could not be invoked. (Is there no default constructor?)", e);
+     */
-        }
+    private boolean compareWithAllEncryptionMethods(String password, String hash, String salt, String playerName) {
-
+        for (HashAlgorithm algorithm : HashAlgorithm.values()) {
-        PasswordEncryptionEvent event = new PasswordEncryptionEvent(method, playerName);
+            if (!HashAlgorithm.CUSTOM.equals(algorithm)) {
-        Bukkit.getPluginManager().callEvent(event);
+                EncryptionMethod method = initializeEncryptionMethodWithoutEvent(algorithm);
-        return event.getMethod();
+                if (method != null && method.comparePassword(hash, password, salt, playerName)) {
-    }
+                    hashPasswordForNewAlgorithm(password, playerName);
-
+                    return true;
-    @Deprecated
-    private static boolean compareWithAllEncryptionMethod(String password,
-                                                          String hash, String playerName) {
-        String salt;
-        PlayerAuth auth = AuthMe.getInstance().getDataSource().getAuth(playerName);
-        if (auth == null) {
-            salt = null;
-        } else {
-            salt = auth.getSalt();
-        }
-
-        for (HashAlgorithm algo : HashAlgorithm.values()) {
-            if (algo != HashAlgorithm.CUSTOM) {
-                try {
-                    EncryptionMethod method = algo.getClazz().newInstance();
-                    if (method.comparePassword(hash, password, salt, playerName)) {
-                        PlayerAuth nAuth = AuthMe.getInstance().getDataSource().getAuth(playerName);
-                        if (nAuth != null) {
-                           // nAuth.setHash(getHash(Settings.getPasswordHash, password, playerName));
-                            nAuth.setSalt(userSalt.containsKey(playerName) ? userSalt.get(playerName) : "");
-                            AuthMe.getInstance().getDataSource().updatePassword(nAuth);
-                            AuthMe.getInstance().getDataSource().updateSalt(nAuth);
-                        }
-                        return true;
-                    }
-                } catch (Exception ignored) {
                 }
             }
         }
         return false;
     }
+
+    /**
+     * Get the encryption method from the given {@link HashAlgorithm} value and emits a
+     * {@link PasswordEncryptionEvent}. The encryption method from the event is returned,
+     * 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 static EncryptionMethod initializeEncryptionMethod(HashAlgorithm algorithm, String playerName) {
+        EncryptionMethod method = initializeEncryptionMethodWithoutEvent(algorithm);
+        PasswordEncryptionEvent event = new PasswordEncryptionEvent(method, playerName);
+        Bukkit.getPluginManager().callEvent(event);
+        return event.getMethod();
+    }
+
+    /**
+     * Initialize the encryption method corresponding to the given hash algorithm.
+     *
+     * @param algorithm The algorithm to retrieve the encryption method for
+     * @return The associated encryption method
+     */
+    private static EncryptionMethod initializeEncryptionMethodWithoutEvent(HashAlgorithm algorithm) {
+        try {
+            return HashAlgorithm.CUSTOM.equals(algorithm)
+                ? null
+                : algorithm.getClazz().newInstance();
+        } catch (InstantiationException | IllegalAccessException 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) {
+        PlayerAuth auth = dataSource.getAuth(playerName);
+        if (auth != null) {
+            HashResult hashResult = initializeEncryptionMethod(algorithm, playerName)
+                .computeHash(password, playerName);
+
+            // TODO #358: updatePassword() should just take the HashResult..., or at least hash & salt. Idem for setHash
+            auth.setSalt(hashResult.getSalt());
+            auth.setHash(hashResult.getHash());
+            dataSource.updatePassword(auth);
+            dataSource.updateSalt(auth);
+        }
+    }
+
 }

src/main/java/fr/xephi/authme/security/crypts/WORDPRESS.java

@@ -11,7 +11,8 @@ import java.security.NoSuchAlgorithmException;
 import java.security.SecureRandom;
 import java.util.Arrays;
 
-@Recommendation(Usage.ACCEPTABLE)
+// TODO #391: Wordpress algorithm fails sometimes. Fix it and change the Recommendation to "ACCEPTABLE" if appropriate
+@Recommendation(Usage.DO_NOT_USE)
 @HasSalt(value = SaltType.TEXT, length = 9)
 // Note ljacqu 20151228: Wordpress is actually a salted algorithm but salt generation is handled internally
 // and isn't exposed to the outside, so we treat it as an unsalted implementation

src/main/java/fr/xephi/authme/task/ChangePasswordTask.java

@@ -34,7 +34,7 @@ public class ChangePasswordTask implements Runnable {
 
         final String name = player.getName().toLowerCase();
         PlayerAuth auth = PlayerCache.getInstance().getAuth(name);
-        if (passwordSecurity.comparePassword(oldPassword, auth.getHash(), player.getName())) {
+        if (passwordSecurity.comparePassword(oldPassword, auth.getHash(), auth.getSalt(), player.getName())) {
             HashResult hashResult = passwordSecurity.computeHash(newPassword, name);
             auth.setHash(hashResult.getHash());
             auth.setSalt(hashResult.getSalt());