#358 Start refactoring PasswordSecurity

- Add new methods temporarily to NewEncrMethod interface
   - No data source access within EncryptionMethod implementations
   - Generate the salt within the EncryptionMethod implementation
- Deprecate static methods on PasswordSecurity
- Adjust AbstractEncryptionMethodTest to test the classes with the new interface
- Add getter for data source instead of accessing field directly

src/main/java/fr/xephi/authme/AuthMe.java

@@ -190,7 +190,6 @@ public class AuthMe extends JavaPlugin {
 
     /**
      * Method called when the server enables the plugin.
-     * @see org.bukkit.plugin.Plugin#onEnable()
      */
     @Override
     public void onEnable() {
@@ -504,11 +503,6 @@ public class AuthMe extends JavaPlugin {
         }
     }
 
-    /**
-     * Method onDisable.
-     *
-     * @see org.bukkit.plugin.Plugin#onDisable()
-     */
     @Override
     public void onDisable() {
         // Save player data
@@ -544,9 +538,6 @@ public class AuthMe extends JavaPlugin {
         }
     }
 
-    /**
-     * Method setupDatabase.
-     */
     public void setupDatabase() throws Exception {
         if (database != null)
             database.close();
@@ -713,8 +704,8 @@ public class AuthMe extends JavaPlugin {
     }
 
     // Save Player Data
-    public void savePlayer(Player player) {
+    private void savePlayer(Player player) {
-        if ((Utils.isNPC(player)) || (Utils.isUnrestricted(player))) {
+        if (Utils.isNPC(player) || Utils.isUnrestricted(player)) {
             return;
         }
         String name = player.getName().toLowerCase();
@@ -829,10 +820,10 @@ public class AuthMe extends JavaPlugin {
 
     // Return the AuthMe spawn point
     private Location getAuthMeSpawn(Player player) {
-        if ((!database.isAuthAvailable(player.getName().toLowerCase()) || !player.hasPlayedBefore()) && (Spawn.getInstance().getFirstSpawn() != null)) {
+        if ((!database.isAuthAvailable(player.getName().toLowerCase()) || !player.hasPlayedBefore())
+            && (Spawn.getInstance().getFirstSpawn() != null)) {
             return Spawn.getInstance().getFirstSpawn();
-        }
+        } else if (Spawn.getInstance().getSpawn() != null) {
-        if (Spawn.getInstance().getSpawn() != null) {
             return Spawn.getInstance().getSpawn();
         }
         return player.getWorld().getSpawnLocation();
@@ -882,8 +873,7 @@ public class AuthMe extends JavaPlugin {
      *
      */
     @Deprecated
-    public void getVerygamesIp(final Player player)
+    public void getVerygamesIp(final Player player) {
-    {
     	final String name = player.getName().toLowerCase();
     	Bukkit.getScheduler().runTaskAsynchronously(plugin, new Runnable(){
 			@Override
@@ -979,4 +969,8 @@ public class AuthMe extends JavaPlugin {
         return management;
     }
 
+    public DataSource getDataSource() {
+        return database;
+    }
+
 }

src/main/java/fr/xephi/authme/command/CommandService.java

@@ -88,8 +88,7 @@ public class CommandService {
      * @return The used data source
      */
     public DataSource getDataSource() {
-        // TODO ljacqu 20151222: Add getter for .database and rename the field to dataSource
+        return authMe.getDataSource();
-        return authMe.database;
     }
 
     /**

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

@@ -119,8 +119,7 @@ public class AsynchronousLogin {
             return null;
         }
 
-        if (Settings.preventOtherCase && !player.getName().equals(pAuth.getRealName()))
+        if (Settings.preventOtherCase && !player.getName().equals(pAuth.getRealName())) {
-        {
         	// TODO: Add a message like : MessageKey.INVALID_NAME_CASE
         	m.send(player, MessageKey.USERNAME_ALREADY_ONLINE_ERROR);
         	return null;

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

@@ -3,6 +3,9 @@ package fr.xephi.authme.security;
 import fr.xephi.authme.security.crypts.EncryptionMethod;
 
 /**
+ * The list of hash algorithms supported by AuthMe. The implementing class must define a public
+ * constructor which takes either no arguments, or a DataSource object (when the salt is stored
+ * separately, writes to the database are necessary).
  */
 public enum HashAlgorithm {
 
@@ -36,7 +39,7 @@ public enum HashAlgorithm {
     SALTEDSHA512(fr.xephi.authme.security.crypts.SALTEDSHA512.class),
     CUSTOM(null);
 
-    final Class<? extends EncryptionMethod> clazz;
+    private final Class<? extends EncryptionMethod> clazz;
 
     /**
      * Constructor for HashAlgorithm.

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

@@ -2,37 +2,37 @@ 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.BCRYPT;
 import fr.xephi.authme.security.crypts.EncryptionMethod;
+import fr.xephi.authme.security.crypts.HashResult;
+import fr.xephi.authme.security.crypts.NewEncrMethod;
 import fr.xephi.authme.settings.Settings;
 import org.bukkit.Bukkit;
 
-import java.math.BigInteger;
-import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
-import java.security.SecureRandom;
 import java.util.HashMap;
 
 /**
  */
 public class PasswordSecurity {
 
+    @Deprecated
     public static final HashMap<String, String> userSalt = new HashMap<>();
-    private static final SecureRandom rnd = new SecureRandom();
+    private final DataSource dataSource;
 
-    public static String createSalt(int length)
+    public PasswordSecurity(DataSource dataSource) {
-        throws NoSuchAlgorithmException {
+        this.dataSource = dataSource;
-        byte[] msg = new byte[40];
-        rnd.nextBytes(msg);
-        MessageDigest sha1 = MessageDigest.getInstance("SHA1");
-        sha1.reset();
-        byte[] digest = sha1.digest(msg);
-        return String.format("%0" + (digest.length << 1) + "x", new BigInteger(1, digest)).substring(0, length);
     }
 
-    public static String getHash(HashAlgorithm alg, String password,
+    @Deprecated
-                                 String playerName) throws NoSuchAlgorithmException {
+    public static String createSalt(int length) {
+        return RandomString.generateHex(length);
+    }
+
+    @Deprecated
+    public static String getHash(HashAlgorithm alg, String password, String playerName) throws NoSuchAlgorithmException {
         EncryptionMethod method;
         try {
             if (alg != HashAlgorithm.CUSTOM)
@@ -126,6 +126,7 @@ public class PasswordSecurity {
         return method.computeHash(password, salt, playerName);
     }
 
+    @Deprecated
     public static boolean comparePasswordWithHash(String password, String hash,
                                                   String playerName) throws NoSuchAlgorithmException {
         HashAlgorithm algorithm = Settings.getPasswordHash;
@@ -157,6 +158,55 @@ public class PasswordSecurity {
         return false;
     }
 
+    public HashResult computeHash(HashAlgorithm algorithm, String password, String playerName) {
+        EncryptionMethod method1 = initializeEncryptionMethod(algorithm, playerName);
+        // TODO #358: Remove this check:
+        NewEncrMethod method;
+        if (method1 instanceof NewEncrMethod) {
+            method = (NewEncrMethod) method1;
+        } else {
+            throw new RuntimeException("TODO #358: Class not yet extended with NewEncrMethod methods");
+        }
+
+        return method.computeHash(password, playerName);
+    }
+
+    public boolean comparePassword(HashAlgorithm algorithm, String hash, String password, String playerName) {
+        EncryptionMethod method1 = initializeEncryptionMethod(algorithm, playerName);
+        // TODO #358: Remove this check:
+        NewEncrMethod method;
+        if (method1 instanceof NewEncrMethod) {
+            method = (NewEncrMethod) method1;
+        } else {
+            throw new RuntimeException("TODO #358: Class not yet extended with NewEncrMethod methods");
+        }
+
+        String salt = null;
+        if (method.hasSeparateSalt()) {
+            PlayerAuth auth = dataSource.getAuth(playerName);
+            salt = (auth != null) ? auth.getSalt() : null;
+        }
+        return method.comparePassword(hash, password, salt, playerName);
+        // TODO #358: Add logic for Settings.supportOldPassword
+    }
+
+    private EncryptionMethod initializeEncryptionMethod(HashAlgorithm algorithm, String playerName) {
+        EncryptionMethod method;
+        try {
+            method = HashAlgorithm.CUSTOM.equals(algorithm)
+                ? null
+                : algorithm.getClazz().newInstance();
+        } catch (InstantiationException | IllegalAccessException e) {
+            throw new IllegalStateException("Constructor for '" + algorithm.getClazz()
+                + "' could not be invoked. (Is it public with no arguments?)", e);
+        }
+
+        PasswordEncryptionEvent event = new PasswordEncryptionEvent(method, playerName);
+        Bukkit.getPluginManager().callEvent(event);
+        return event.getMethod();
+    }
+
+    @Deprecated
     private static boolean compareWithAllEncryptionMethod(String password,
                                                           String hash, String playerName) {
         for (HashAlgorithm algo : HashAlgorithm.values()) {

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

@@ -29,11 +29,8 @@ public interface EncryptionMethod {
      *
      * @return True if the password matches, false otherwise
      */
+    @Deprecated
     boolean comparePassword(String hash, String password, String playerName)
         throws NoSuchAlgorithmException;
 
-    // String generateSalt();
-
-    // String computeHash(String password, String name);
-
 }

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

@@ -0,0 +1,26 @@
+package fr.xephi.authme.security.crypts;
+
+/**
+ * The result of a hash computation.
+ */
+public class HashResult {
+    
+    /** The generated hash. */
+    private final String hash;
+    /** The generated salt; may be null if no salt is used or if the salt is included in the hash output. */
+    private final String salt;
+    
+    public HashResult(String hash, String salt) {
+        this.hash = hash;
+        this.salt = salt;
+    }
+    
+    public String getHash() {
+        return hash;
+    }
+    
+    public String getSalt() {
+        return salt;
+    }
+    
+}

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

@@ -0,0 +1,17 @@
+package fr.xephi.authme.security.crypts;
+
+/**
+ * Temporary interface for additional methods that will be added to {@link EncryptionMethod}.
+ * TODO #358: Move methods to EncryptionMethod interface and delete this.
+ */
+public interface NewEncrMethod extends EncryptionMethod {
+
+    HashResult computeHash(String password, String name);
+
+    String generateSalt();
+
+    boolean hasSeparateSalt();
+
+    boolean comparePassword(String hash, String password, String salt, String name);
+
+}

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

@@ -1,14 +1,20 @@
 package fr.xephi.authme.security.crypts;
 
 import fr.xephi.authme.AuthMe;
+import fr.xephi.authme.security.RandomString;
+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 java.math.BigInteger;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 
-/**
+@Recommendation(Usage.ACCEPTABLE) // presuming that length is something sensible (>= 8)
- */
+@HasSalt(value = SaltType.TEXT)   // length defined by Settings.saltLength
-public class SALTED2MD5 implements EncryptionMethod {
+public class SALTED2MD5 implements NewEncrMethod {
 
     private static String getMD5(String message)
         throws NoSuchAlgorithmException {
@@ -25,10 +31,41 @@ public class SALTED2MD5 implements EncryptionMethod {
         return getMD5(getMD5(password) + salt);
     }
 
+    @Override
+    public HashResult computeHash(String password, String name) {
+        try {
+            String salt = generateSalt();
+            return new HashResult(computeHash(password, salt, name), salt);
+        } catch (NoSuchAlgorithmException e) {
+            throw new RuntimeException(e); // TODO #358: Remove try-catch clause
+        }
+    }
+
     @Override
     public boolean comparePassword(String hash, String password,
                                    String playerName) throws NoSuchAlgorithmException {
         String salt = AuthMe.getInstance().database.getAuth(playerName).getSalt();
         return hash.equals(getMD5(getMD5(password) + salt));
     }
+
+    @Override
+    public boolean comparePassword(String hash, String password, String salt, String name) {
+        try {
+            return hash.equals(computeHash(password, salt, name));
+        } catch (NoSuchAlgorithmException e) {
+            throw new RuntimeException(e);
+            // TODO #358: Remove try-catch
+        }
+    }
+
+    @Override
+    public String generateSalt() {
+        return RandomString.generateHex(Settings.saltLength);
+    }
+
+    @Override
+    public boolean hasSeparateSalt() {
+        return true;
+    }
+
 }

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

@@ -39,7 +39,7 @@ public class RandomStringTest {
             String result = RandomString.generateHex(length);
             assertThat("Result '" + result + "' should have length " + length,
                 result.length(), equalTo(length));
-            assertThat("Result '" + result + "' should only have characters a-z, 0-9",
+            assertThat("Result '" + result + "' should only have characters a-f, 0-9",
                 badChars.matcher(result).matches(), equalTo(false));
         }
     }

src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java

@@ -7,7 +7,9 @@ import java.security.NoSuchAlgorithmException;
 import java.util.HashMap;
 import java.util.Map;
 
+import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
 /**
@@ -33,6 +35,8 @@ public abstract class AbstractEncryptionMethodTest {
     private EncryptionMethod method;
     /** Map with the hashes against which the entries in GIVEN_PASSWORDS are tested. */
     private Map<String, String> hashes;
+    /** The accompanying salts for the hashes in {@link #hashes} if necessary. Can be empty otherwise. */
+    private Map<String, String> salts;
 
     /**
      * Create a new test for the given encryption method.
@@ -45,12 +49,32 @@ public abstract class AbstractEncryptionMethodTest {
      */
     public AbstractEncryptionMethodTest(EncryptionMethod method, String hash0, String hash1,
                                         String hash2, String hash3) {
+        // TODO #358: Throw if method.hasSeparateSalt() is true
         this.method = method;
         hashes = new HashMap<>();
         hashes.put(GIVEN_PASSWORDS[0], hash0);
         hashes.put(GIVEN_PASSWORDS[1], hash1);
         hashes.put(GIVEN_PASSWORDS[2], hash2);
         hashes.put(GIVEN_PASSWORDS[3], hash3);
+        salts = new HashMap<>();
+    }
+
+    public AbstractEncryptionMethodTest(EncryptionMethod method, HashResult result0, HashResult result1,
+                                        HashResult result2, HashResult result3) {
+        // TODO #358: Throw if method.hasSeparateSalt() is false
+        this.method = method;
+
+        hashes = new HashMap<>();
+        hashes.put(GIVEN_PASSWORDS[0], result0.getHash());
+        hashes.put(GIVEN_PASSWORDS[1], result1.getHash());
+        hashes.put(GIVEN_PASSWORDS[2], result2.getHash());
+        hashes.put(GIVEN_PASSWORDS[3], result3.getHash());
+
+        salts = new HashMap<>();
+        salts.put(GIVEN_PASSWORDS[0], result0.getSalt());
+        salts.put(GIVEN_PASSWORDS[1], result1.getSalt());
+        salts.put(GIVEN_PASSWORDS[2], result2.getSalt());
+        salts.put(GIVEN_PASSWORDS[3], result3.getSalt());
     }
 
     @Test
@@ -74,7 +98,33 @@ public abstract class AbstractEncryptionMethodTest {
     }
 
     @Test
-    public void testPasswordEquality() {
+    public void testPasswordEquality() throws NoSuchAlgorithmException {
+        // TODO #358: Remove "throws NoSuchAlgorithmException" on method declaration
+        // TODO #358: Remove instanceof and use this code always
+        if (method instanceof NewEncrMethod) {
+            NewEncrMethod method1 = (NewEncrMethod) method;
+            for (String password : INTERNAL_PASSWORDS) {
+                HashResult result = method1.computeHash(password, USERNAME);
+                final String hash = result.getHash();
+                final String salt = result.getSalt();
+
+                // Check that the computeHash(password, salt, name) method has the same output for the returned salt
+                assertThat(hash, equalTo(method1.computeHash(password, salt, USERNAME)));
+
+                assertTrue("Generated hash for '" + password + "' should match password (hash = '" + hash + "')",
+                    method1.comparePassword(hash, password, salt, USERNAME));
+                if (!password.equals(password.toLowerCase())) {
+                    assertFalse("Lower-case of '" + password + "' should not match generated hash '" + hash + "'",
+                        method1.comparePassword(hash, password.toLowerCase(), salt, USERNAME));
+                }
+                if (!password.equals(password.toUpperCase())) {
+                    assertFalse("Upper-case of '" + password + "' should not match generated hash '" + hash + "'",
+                        method1.comparePassword(hash, password.toUpperCase(), salt, USERNAME));
+                }
+            }
+            return;
+        }
+
         for (String password : INTERNAL_PASSWORDS) {
             try {
                 String hash = method.computeHash(password, getSalt(method), USERNAME);
@@ -95,6 +145,15 @@ public abstract class AbstractEncryptionMethodTest {
     }
 
     private boolean doesGivenHashMatch(String password, EncryptionMethod method) {
+        // TODO #358: Remove casting checks and remove old code below
+        if (method instanceof NewEncrMethod) {
+            NewEncrMethod method1 = (NewEncrMethod) method;
+            String hash = hashes.get(password);
+            String salt = salts.get(password);
+            return method1.comparePassword(hash, password, salt, USERNAME);
+        }
+
+
         try {
             return method.comparePassword(hashes.get(password), password, USERNAME);
         } catch (NoSuchAlgorithmException e) {
@@ -129,7 +188,6 @@ public abstract class AbstractEncryptionMethodTest {
 
     // TODO #358: Remove this method and use the new salt method on the interface
     private static String getSalt(EncryptionMethod method) {
-        try {
         if (method instanceof BCRYPT) {
             return BCRYPT.gensalt();
         } else if (method instanceof MD5 || method instanceof WORDPRESS || method instanceof SMF
@@ -149,10 +207,8 @@ public abstract class AbstractEncryptionMethodTest {
         } else if (method instanceof WBB4) {
             return BCRYPT.gensalt(8);
         }
-        } catch (NoSuchAlgorithmException e) {
+        System.out.println("Note: Cannot generate salt for unknown encryption method '" + method + "'");
-            throw new RuntimeException(e);
+        return "";
-        }
-        throw new IllegalStateException("Unknown EncryptionMethod for salt generation");
     }
 
 }

src/test/java/fr/xephi/authme/security/crypts/SALTED2MD5Test.java

@@ -0,0 +1,26 @@
+package fr.xephi.authme.security.crypts;
+
+import fr.xephi.authme.settings.Settings;
+import fr.xephi.authme.util.WrapperMock;
+import org.junit.Before;
+
+/**
+ * Test for {@link SALTED2MD5}.
+ */
+public class SALTED2MD5Test extends AbstractEncryptionMethodTest {
+
+    @Before
+    public void setUpAlgorithm() {
+        WrapperMock.createInstance();
+        Settings.saltLength = 8;
+    }
+
+    public SALTED2MD5Test() {
+        super(new SALTED2MD5(),
+            new HashResult("9f3d13dc01a6fe61fd669954174399f3", "9b5f5749"),  // password
+            new HashResult("b28c32f624a4eb161d6adc9acb5bfc5b", "f750ba32"),  // PassWord1
+            new HashResult("38dcb83cc68424afe3cda012700c2bb1", "eb2c3394"),  // &^%te$t?Pw@_
+            new HashResult("ad25606eae5b760c8a2469d65578ac39", "04eee598")); // âË_3(íù*)
+    }
+
+}