#358 Create test for PasswordSecurity, create salt column if not exists

- Add test class for PasswordSecurity
- Check and create the salt column in MySQL and SQLite when necessary
- Add javadoc to some classes

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

@@ -212,7 +212,6 @@ public class AuthMe extends JavaPlugin {
 
         // Set up messages & password security
         messages = Messages.getInstance();
-        passwordSecurity = new PasswordSecurity(getDataSource(), Settings.getPasswordHash, Settings.supportOldPassword);
 
         // Connect to the database and setup tables
         try {
@@ -225,6 +224,9 @@ public class AuthMe extends JavaPlugin {
             return;
         }
 
+        passwordSecurity = new PasswordSecurity(getDataSource(), Settings.getPasswordHash,
+            Bukkit.getPluginManager(), Settings.supportOldPassword);
+
         // Set up the permissions manager and command handler
         permsMan = initializePermissionsManager();
         commandHandler = initializeCommandHandler(permsMan, messages, passwordSecurity);
@@ -525,7 +527,9 @@ public class AuthMe extends JavaPlugin {
         new PerformBackup(plugin).doBackup(PerformBackup.BackupCause.STOP);
 
         // Unload modules
-        moduleManager.unloadModules();
+        if (moduleManager != null) {
+            moduleManager.unloadModules();
+        }
 
         // Close the database
         if (database != null) {

src/main/java/fr/xephi/authme/datasource/MySQL.java

@@ -162,6 +162,15 @@ public class MySQL implements DataSource {
             }
             rs.close();
 
+            if (!columnSalt.isEmpty()) {
+                rs = md.getColumns(null, null, tableName, columnSalt);
+                if (!rs.next()) {
+                    st.executeUpdate("ALTER TABLE " + tableName
+                        + " ADD COLUMN " + columnSalt + " VARCHAR(255);");
+                }
+                rs.close();
+            }
+
             rs = md.getColumns(null, null, tableName, columnIp);
             if (!rs.next()) {
                 st.executeUpdate("ALTER TABLE " + tableName

src/main/java/fr/xephi/authme/datasource/SQLite.java

@@ -4,6 +4,7 @@ import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.cache.auth.PlayerAuth;
 import fr.xephi.authme.security.crypts.EncryptedPassword;
 import fr.xephi.authme.settings.Settings;
+import fr.xephi.authme.util.StringUtils;
 
 import java.sql.*;
 import java.util.ArrayList;
@@ -91,6 +92,13 @@ public class SQLite implements DataSource {
                 st.executeUpdate("ALTER TABLE " + tableName + " ADD COLUMN " + columnPassword + " VARCHAR(255) NOT NULL;");
             }
             rs.close();
+            if (!columnSalt.isEmpty()) {
+                rs = con.getMetaData().getColumns(null, null, tableName, columnSalt);
+                if (!rs.next()) {
+                    st.executeUpdate("ALTER TABLE " + tableName + " ADD COLUMN " + columnSalt + " VARCHAR(255);");
+                }
+                rs.close();
+            }
             rs = con.getMetaData().getColumns(null, null, tableName, columnIp);
             if (!rs.next()) {
                 st.executeUpdate("ALTER TABLE " + tableName + " ADD COLUMN " + columnIp + " VARCHAR(40) NOT NULL;");
@@ -200,7 +208,11 @@ public class SQLite implements DataSource {
         PreparedStatement pst = null;
         try {
             EncryptedPassword password = auth.getPassword();
-            if (columnSalt.isEmpty() && password.getSalt().isEmpty()) {
+            if (columnSalt.isEmpty()) {
+                if (!StringUtils.isEmpty(auth.getPassword().getSalt())) {
+                    ConsoleLogger.showError("Warning! Detected hashed password with separate salt but the salt column "
+                        + "is not set in the config!");
+                }
                 pst = con.prepareStatement("INSERT INTO " + tableName + "(" + columnName + "," + columnPassword +
                     "," + columnIp + "," + columnLastLogin + "," + columnRealName + ") VALUES (?,?,?,?,?);");
                 pst.setString(1, auth.getNickname());
@@ -592,13 +604,6 @@ public class SQLite implements DataSource {
         }
     }
 
-    /**
-     * Method purgeBanned.
-     *
-     * @param banned List<String>
-     *
-     * @see fr.xephi.authme.datasource.DataSource#purgeBanned(List<String>)
-     */
     @Override
     public void purgeBanned(List<String> banned) {
         PreparedStatement pst = null;

src/main/java/fr/xephi/authme/events/PasswordEncryptionEvent.java

@@ -5,16 +5,17 @@ import org.bukkit.event.Event;
 import org.bukkit.event.HandlerList;
 
 /**
- * This event is called when we need to compare or get an hash password, for set
- * a custom EncryptionMethod
+ * This event is called when we need to compare or hash password and allows
+ * third-party listeners to change the encryption method. This is typically
+ * done with the {@link fr.xephi.authme.security.HashAlgorithm#CUSTOM} setting.
  *
  * @author Xephi59
  */
 public class PasswordEncryptionEvent extends Event {
 
     private static final HandlerList handlers = new HandlerList();
-    private EncryptionMethod method = null;
-    private String playerName = "";
+    private EncryptionMethod method;
+    private String playerName;
 
     public PasswordEncryptionEvent(EncryptionMethod method, String playerName) {
         super(false);

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

@@ -9,33 +9,33 @@ import fr.xephi.authme.security.crypts.EncryptionMethod;
  */
 public enum HashAlgorithm {
 
-    MD5(fr.xephi.authme.security.crypts.MD5.class),
-    SHA1(fr.xephi.authme.security.crypts.SHA1.class),
-    SHA256(fr.xephi.authme.security.crypts.SHA256.class),
-    WHIRLPOOL(fr.xephi.authme.security.crypts.WHIRLPOOL.class),
-    XAUTH(fr.xephi.authme.security.crypts.XAUTH.class),
-    MD5VB(fr.xephi.authme.security.crypts.MD5VB.class),
-    PHPBB(fr.xephi.authme.security.crypts.PHPBB.class),
-    @Deprecated
-    PLAINTEXT(fr.xephi.authme.security.crypts.PLAINTEXT.class),
-    MYBB(fr.xephi.authme.security.crypts.MYBB.class),
-    IPB3(fr.xephi.authme.security.crypts.IPB3.class),
-    PHPFUSION(fr.xephi.authme.security.crypts.PHPFUSION.class),
-    SMF(fr.xephi.authme.security.crypts.SMF.class),
-    SALTED2MD5(fr.xephi.authme.security.crypts.SALTED2MD5.class),
-    JOOMLA(fr.xephi.authme.security.crypts.JOOMLA.class),
     BCRYPT(fr.xephi.authme.security.crypts.BCRYPT.class),
-    WBB3(fr.xephi.authme.security.crypts.WBB3.class),
-    WBB4(fr.xephi.authme.security.crypts.WBB4.class),
-    SHA512(fr.xephi.authme.security.crypts.SHA512.class),
+    BCRYPT2Y(fr.xephi.authme.security.crypts.BCRYPT2Y.class),
+    CRAZYCRYPT1(fr.xephi.authme.security.crypts.CRAZYCRYPT1.class),
     DOUBLEMD5(fr.xephi.authme.security.crypts.DOUBLEMD5.class),
+    IPB3(fr.xephi.authme.security.crypts.IPB3.class),
+    JOOMLA(fr.xephi.authme.security.crypts.JOOMLA.class),
+    MD5(fr.xephi.authme.security.crypts.MD5.class),
+    MD5VB(fr.xephi.authme.security.crypts.MD5VB.class),
+    MYBB(fr.xephi.authme.security.crypts.MYBB.class),
     PBKDF2(fr.xephi.authme.security.crypts.CryptPBKDF2.class),
     PBKDF2DJANGO(fr.xephi.authme.security.crypts.CryptPBKDF2Django.class),
-    WORDPRESS(fr.xephi.authme.security.crypts.WORDPRESS.class),
+    PHPBB(fr.xephi.authme.security.crypts.PHPBB.class),
+    PHPFUSION(fr.xephi.authme.security.crypts.PHPFUSION.class),
+    @Deprecated
+    PLAINTEXT(fr.xephi.authme.security.crypts.PLAINTEXT.class),
     ROYALAUTH(fr.xephi.authme.security.crypts.ROYALAUTH.class),
-    CRAZYCRYPT1(fr.xephi.authme.security.crypts.CRAZYCRYPT1.class),
-    BCRYPT2Y(fr.xephi.authme.security.crypts.BCRYPT2Y.class),
+    SALTED2MD5(fr.xephi.authme.security.crypts.SALTED2MD5.class),
     SALTEDSHA512(fr.xephi.authme.security.crypts.SALTEDSHA512.class),
+    SHA1(fr.xephi.authme.security.crypts.SHA1.class),
+    SHA256(fr.xephi.authme.security.crypts.SHA256.class),
+    SHA512(fr.xephi.authme.security.crypts.SHA512.class),
+    SMF(fr.xephi.authme.security.crypts.SMF.class),
+    WBB3(fr.xephi.authme.security.crypts.WBB3.class),
+    WBB4(fr.xephi.authme.security.crypts.WBB4.class),
+    WHIRLPOOL(fr.xephi.authme.security.crypts.WHIRLPOOL.class),
+    WORDPRESS(fr.xephi.authme.security.crypts.WORDPRESS.class),
+    XAUTH(fr.xephi.authme.security.crypts.XAUTH.class),
     CUSTOM(null);
 
     private final Class<? extends EncryptionMethod> clazz;

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

@@ -4,28 +4,60 @@ import java.math.BigInteger;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 
-
+/**
+ * Hashing utilities (interface for common hashing algorithms).
+ */
 public final class HashUtils {
 
     private HashUtils() {
     }
 
+    /**
+     * Generate the SHA-1 digest of the given message.
+     *
+     * @param message The message to hash
+     * @return The resulting SHA-1 digest
+     */
     public static String sha1(String message) {
         return hash(message, MessageDigestAlgorithm.SHA1);
     }
 
+    /**
+     * Generate the SHA-256 digest of the given message.
+     *
+     * @param message The message to hash
+     * @return The resulting SHA-256 digest
+     */
     public static String sha256(String message) {
         return hash(message, MessageDigestAlgorithm.SHA256);
     }
 
+    /**
+     * Generate the SHA-512 digest of the given message.
+     *
+     * @param message The message to hash
+     * @return The resulting SHA-512 digest
+     */
     public static String sha512(String message) {
         return hash(message, MessageDigestAlgorithm.SHA512);
     }
 
+    /**
+     * Generate the MD5 digest of the given message.
+     *
+     * @param message The message to hash
+     * @return The resulting MD5 digest
+     */
     public static String md5(String message) {
         return hash(message, MessageDigestAlgorithm.MD5);
     }
 
+    /**
+     * Return a {@link MessageDigest} instance for the given algorithm.
+     *
+     * @param algorithm The desired algorithm
+     * @return MessageDigest instance for the given algorithm
+     */
     public static MessageDigest getDigest(MessageDigestAlgorithm algorithm) {
         try {
             return MessageDigest.getInstance(algorithm.getKey());
@@ -35,6 +67,13 @@ public final class HashUtils {
         }
     }
 
+    /**
+     * Hash the message with the given algorithm and return the hash in its hexadecimal notation.
+     *
+     * @param message The message to hash
+     * @param algorithm The algorithm to hash the message with
+     * @return The digest in its hexadecimal representation
+     */
     private static String hash(String message, MessageDigestAlgorithm algorithm) {
         MessageDigest md = getDigest(algorithm);
         md.reset();

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

@@ -5,7 +5,7 @@ import fr.xephi.authme.datasource.DataSource;
 import fr.xephi.authme.events.PasswordEncryptionEvent;
 import fr.xephi.authme.security.crypts.EncryptedPassword;
 import fr.xephi.authme.security.crypts.EncryptionMethod;
-import org.bukkit.Bukkit;
+import org.bukkit.plugin.PluginManager;
 
 /**
  * Manager class for password-related operations.
@@ -14,11 +14,14 @@ public class PasswordSecurity {
 
     private final DataSource dataSource;
     private final HashAlgorithm algorithm;
+    private final PluginManager pluginManager;
     private final boolean supportOldAlgorithm;
 
-    public PasswordSecurity(DataSource dataSource, HashAlgorithm algorithm, boolean supportOldAlgorithm) {
+    public PasswordSecurity(DataSource dataSource, HashAlgorithm algorithm,
+                            PluginManager pluginManager, boolean supportOldAlgorithm) {
         this.dataSource = dataSource;
         this.algorithm = algorithm;
+        this.pluginManager = pluginManager;
         this.supportOldAlgorithm = supportOldAlgorithm;
     }
 
@@ -86,10 +89,10 @@ public class PasswordSecurity {
      * @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) {
+    private EncryptionMethod initializeEncryptionMethod(HashAlgorithm algorithm, String playerName) {
         EncryptionMethod method = initializeEncryptionMethodWithoutEvent(algorithm);
         PasswordEncryptionEvent event = new PasswordEncryptionEvent(method, playerName);
-        Bukkit.getPluginManager().callEvent(event);
+        pluginManager.callEvent(event);
         return event.getMethod();
     }
 

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

@@ -3,6 +3,9 @@ package fr.xephi.authme.security;
 import java.security.SecureRandom;
 import java.util.Random;
 
+/**
+ * Utility for generating random strings.
+ */
 public final class RandomString {
 
     private static final char[] chars = new char[36];
@@ -21,10 +24,23 @@ public final class RandomString {
     private RandomString() {
     }
 
+    /**
+     * Generate a string of the given length consisting of random characters within the range [0-9a-z].
+     *
+     * @param length The length of the random string to generate
+     * @return The random string
+     */
     public static String generate(int length) {
         return generate(length, chars.length);
     }
 
+    /**
+     * Generate a random hexadecimal string of the given length. In other words, the generated string
+     * contains characters only within the range [0-9a-f].
+     *
+     * @param length The length of the random string to generate
+     * @return The random hexadecimal string
+     */
     public static String generateHex(int length) {
         return generate(length, HEX_MAX_INDEX);
     }

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

@@ -0,0 +1,224 @@
+package fr.xephi.authme.security;
+
+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.EncryptedPassword;
+import fr.xephi.authme.security.crypts.EncryptionMethod;
+import fr.xephi.authme.security.crypts.JOOMLA;
+import fr.xephi.authme.security.crypts.PHPBB;
+import org.bukkit.event.Event;
+import org.bukkit.plugin.PluginManager;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertThat;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.doCallRealMethod;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+/**
+ * Test for {@link PasswordSecurity}.
+ */
+public class PasswordSecurityTest {
+
+    private PluginManager pluginManager;
+    private DataSource dataSource;
+    private EncryptionMethod method;
+    private Class<?> caughtClassInEvent;
+
+    @Before
+    public void setUpMocks() {
+        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.
+        doAnswer(new Answer<Void>() {
+            @Override
+            public Void answer(InvocationOnMock invocation) throws Throwable {
+                Object[] arguments = invocation.getArguments();
+                if (arguments[0] instanceof PasswordEncryptionEvent) {
+                    PasswordEncryptionEvent event = (PasswordEncryptionEvent) arguments[0];
+                    caughtClassInEvent = event.getMethod() != null ? event.getMethod().getClass() : null;
+                    event.setMethod(method);
+                }
+                return null;
+            }
+        }).when(pluginManager).callEvent(any(Event.class));
+    }
+
+    @Test
+    public void shouldReturnPasswordMatch() {
+        // given
+        EncryptedPassword password = new EncryptedPassword("$TEST$10$SOME_HASH", null);
+        String playerName = "Tester";
+        String clearTextPass = "myPassTest";
+
+        PlayerAuth auth = mock(PlayerAuth.class);
+        given(auth.getPassword()).willReturn(password);
+        given(dataSource.getAuth(playerName)).willReturn(auth);
+        given(method.comparePassword(clearTextPass, password, playerName)).willReturn(true);
+        PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.BCRYPT, pluginManager, false);
+
+        // when
+        boolean result = security.comparePassword(clearTextPass, playerName);
+
+        // then
+        assertThat(result, equalTo(true));
+        verify(dataSource).getAuth(playerName);
+        verify(pluginManager).callEvent(any(PasswordEncryptionEvent.class));
+        verify(method).comparePassword(clearTextPass, password, playerName);
+    }
+
+    @Test
+    public void shouldReturnPasswordMismatch() {
+        // given
+        EncryptedPassword password = new EncryptedPassword("$TEST$10$SOME_HASH", null);
+        String playerName = "My_PLayer";
+        String clearTextPass = "passw0Rd1";
+
+        PlayerAuth auth = mock(PlayerAuth.class);
+        given(auth.getPassword()).willReturn(password);
+        given(dataSource.getAuth(playerName)).willReturn(auth);
+        given(method.comparePassword(clearTextPass, password, playerName)).willReturn(false);
+        PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.CUSTOM, pluginManager, false);
+
+        // when
+        boolean result = security.comparePassword(clearTextPass, playerName);
+
+        // then
+        assertThat(result, equalTo(false));
+        verify(dataSource).getAuth(playerName);
+        verify(pluginManager).callEvent(any(PasswordEncryptionEvent.class));
+        verify(method).comparePassword(clearTextPass, password, playerName);
+    }
+
+    @Test
+    public void shouldReturnFalseIfPlayerDoesNotExist() {
+        // given
+        String playerName = "bobby";
+        String clearTextPass = "tables";
+
+        given(dataSource.getAuth(playerName)).willReturn(null);
+        PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.MD5, pluginManager, false);
+
+        // when
+        boolean result = security.comparePassword(clearTextPass, playerName);
+
+        // then
+        assertThat(result, equalTo(false));
+        verify(dataSource).getAuth(playerName);
+        verify(pluginManager, never()).callEvent(any(Event.class));
+        verify(method, never()).comparePassword(anyString(), any(EncryptedPassword.class), anyString());
+    }
+
+    @Test
+    public void shouldTryOtherMethodsForFailedPassword() {
+        // given
+        // BCRYPT2Y hash for "Test"
+        EncryptedPassword password =
+            new EncryptedPassword("$2y$10$2e6d2193f43501c926e25elvWlPmWczmrfrnbZV0dUZGITjYjnkkW");
+        String playerName = "somePlayer";
+        String clearTextPass = "Test";
+        // MD5 hash for "Test"
+        EncryptedPassword newPassword = new EncryptedPassword("0cbc6611f5540bd0809a388dc95a615b");
+
+        PlayerAuth auth = mock(PlayerAuth.class);
+        doCallRealMethod().when(auth).getPassword();
+        doCallRealMethod().when(auth).setPassword(any(EncryptedPassword.class));
+        auth.setPassword(password);
+        given(dataSource.getAuth(playerName)).willReturn(auth);
+        given(method.comparePassword(clearTextPass, password, playerName)).willReturn(false);
+        given(method.computeHash(clearTextPass, playerName)).willReturn(newPassword);
+        PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.MD5, pluginManager, true);
+
+        // when
+        boolean result = security.comparePassword(clearTextPass, playerName);
+
+        // then
+        assertThat(result, equalTo(true));
+        verify(dataSource, times(2)).getAuth(playerName);
+        verify(pluginManager, times(2)).callEvent(any(PasswordEncryptionEvent.class));
+        verify(method).comparePassword(clearTextPass, password, playerName);
+        verify(auth).setPassword(newPassword);
+
+        ArgumentCaptor<PlayerAuth> captor = ArgumentCaptor.forClass(PlayerAuth.class);
+        verify(dataSource).updatePassword(captor.capture());
+        assertThat(captor.getValue().getPassword(), equalTo(newPassword));
+    }
+
+    @Test
+    public void shouldHashPassword() {
+        // given
+        String password = "MyP@ssword";
+        String username = "theUserInTest";
+        EncryptedPassword encryptedPassword = new EncryptedPassword("$T$est#Hash", "__someSalt__");
+        given(method.computeHash(password, username)).willReturn(encryptedPassword);
+        PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.JOOMLA, pluginManager, true);
+
+        // when
+        EncryptedPassword result = security.computeHash(password, username);
+
+        // then
+        assertThat(result, equalTo(encryptedPassword));
+        ArgumentCaptor<PasswordEncryptionEvent> captor = ArgumentCaptor.forClass(PasswordEncryptionEvent.class);
+        verify(pluginManager).callEvent(captor.capture());
+        PasswordEncryptionEvent event = captor.getValue();
+        assertThat(JOOMLA.class.equals(caughtClassInEvent), equalTo(true));
+        assertThat(event.getPlayerName(), equalTo(username));
+    }
+
+    @Test
+    public void shouldHashPasswordWithGivenAlgorithm() {
+        // given
+        String password = "TopSecretPass#112525";
+        String username = "someone12";
+        EncryptedPassword encryptedPassword = new EncryptedPassword("~T!est#Hash", "__someSalt__");
+        given(method.computeHash(password, username)).willReturn(encryptedPassword);
+        PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.JOOMLA, pluginManager, true);
+
+        // when
+        EncryptedPassword result = security.computeHash(HashAlgorithm.PHPBB, password, username);
+
+        // then
+        assertThat(result, equalTo(encryptedPassword));
+        ArgumentCaptor<PasswordEncryptionEvent> captor = ArgumentCaptor.forClass(PasswordEncryptionEvent.class);
+        verify(pluginManager).callEvent(captor.capture());
+        PasswordEncryptionEvent event = captor.getValue();
+        assertThat(PHPBB.class.equals(caughtClassInEvent), equalTo(true));
+        assertThat(event.getPlayerName(), equalTo(username));
+    }
+
+    @Test
+    public void shouldSkipCheckIfMandatorySaltIsUnavailable() {
+        // given
+        String password = "?topSecretPass\\";
+        String username = "someone12";
+        EncryptedPassword encryptedPassword = new EncryptedPassword("~T!est#Hash");
+        given(method.computeHash(password, username)).willReturn(encryptedPassword);
+        given(method.hasSeparateSalt()).willReturn(true);
+        PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.XAUTH, pluginManager, true);
+
+        // when
+        boolean result = security.comparePassword(password, encryptedPassword, username);
+
+        // then
+        assertThat(result, equalTo(false));
+        verify(dataSource, never()).getAuth(anyString());
+        verify(pluginManager).callEvent(any(PasswordEncryptionEvent.class));
+        verify(method, never()).comparePassword(anyString(), any(EncryptedPassword.class), anyString());
+    }
+
+}