#1188 Create and keep encryption method instance (#1191)

- Avoid creating the same object over and over; instead keep it and only change it on settings reload

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

@@ -1,7 +1,6 @@
 package fr.xephi.authme.events;
 
 import fr.xephi.authme.security.crypts.EncryptionMethod;
-import org.bukkit.event.Event;
 import org.bukkit.event.HandlerList;
 
 /**
@@ -13,22 +12,19 @@ public class PasswordEncryptionEvent extends CustomEvent {
 
     private static final HandlerList handlers = new HandlerList();
     private EncryptionMethod method;
-    private String playerName;
 
     /**
      * Constructor.
      *
      * @param method The method used to encrypt the password
-     * @param playerName The name of the player
      */
-    public PasswordEncryptionEvent(EncryptionMethod method, String playerName) {
+    public PasswordEncryptionEvent(EncryptionMethod method) {
         super(false);
         this.method = method;
-        this.playerName = playerName;
     }
 
     /**
-     * Return the list of handlers, equivalent to {@link #getHandlers()} and required by {@link Event}.
+     * Return the list of handlers, equivalent to {@link #getHandlers()} and required by {@link org.bukkit.event.Event}.
      *
      * @return The list of handlers
      */
@@ -58,14 +54,4 @@ public class PasswordEncryptionEvent extends CustomEvent {
     public void setMethod(EncryptionMethod method) {
         this.method = method;
     }
-
-    /**
-     * Return the name of the player the event has been fired for.
-     *
-     * @return The player name
-     */
-    public String getPlayerName() {
-        return playerName;
-    }
-
 }

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

@@ -29,9 +29,9 @@ public class PasswordSecurity implements Reloadable {
     private PluginManager pluginManager;
 
     @Inject
-    private Factory<EncryptionMethod> hashAlgorithmFactory;
+    private Factory<EncryptionMethod> encryptionMethodFactory;
 
-    private HashAlgorithm algorithm;
+    private EncryptionMethod encryptionMethod;
     private Collection<HashAlgorithm> legacyAlgorithms;
 
     /**
@@ -40,7 +40,8 @@ public class PasswordSecurity implements Reloadable {
     @PostConstruct
     @Override
     public void reload() {
-        this.algorithm = settings.getProperty(SecuritySettings.PASSWORD_HASH);
+        HashAlgorithm algorithm = settings.getProperty(SecuritySettings.PASSWORD_HASH);
+        this.encryptionMethod = initializeEncryptionMethodWithEvent(algorithm);
         this.legacyAlgorithms = settings.getProperty(SecuritySettings.LEGACY_HASHES);
     }
 
@@ -54,8 +55,7 @@ public class PasswordSecurity implements Reloadable {
      */
     public HashedPassword computeHash(String password, String playerName) {
         String playerLowerCase = playerName.toLowerCase();
-        EncryptionMethod method = initializeEncryptionMethodWithEvent(algorithm, playerLowerCase);
+        return encryptionMethod.computeHash(password, playerLowerCase);
-        return method.computeHash(password, playerLowerCase);
     }
 
     /**
@@ -81,14 +81,13 @@ public class PasswordSecurity implements Reloadable {
      * @return True if the password matches, false otherwise
      */
     public boolean comparePassword(String password, HashedPassword hashedPassword, String playerName) {
-        EncryptionMethod method = initializeEncryptionMethodWithEvent(algorithm, playerName);
         String playerLowerCase = playerName.toLowerCase();
-        return methodMatches(method, password, hashedPassword, playerLowerCase)
+        return methodMatches(encryptionMethod, password, hashedPassword, playerLowerCase)
             || compareWithLegacyHashes(password, hashedPassword, playerLowerCase);
     }
 
     /**
-     * Compare the given hash with all available encryption methods to support
+     * Compare the given hash with the configured legacy encryption methods to support
      * the migration to a new encryption method. Upon a successful match, the password
      * will be hashed with the new encryption method and persisted.
      *
@@ -96,13 +95,13 @@ public class PasswordSecurity implements Reloadable {
      * @param hashedPassword The encrypted password to test the clear-text password against
      * @param playerName     The name of the player
      *
-     * @return True if there was a password match with another encryption method, false otherwise
+     * @return True if there was a password match with a configured legacy encryption method, false otherwise
      */
     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);
+                hashAndSavePasswordWithNewAlgorithm(password, playerName);
                 return true;
             }
         }
@@ -132,13 +131,12 @@ public class PasswordSecurity implements Reloadable {
      * 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 EncryptionMethod initializeEncryptionMethodWithEvent(HashAlgorithm algorithm, String playerName) {
+    private EncryptionMethod initializeEncryptionMethodWithEvent(HashAlgorithm algorithm) {
         EncryptionMethod method = initializeEncryptionMethod(algorithm);
-        PasswordEncryptionEvent event = new PasswordEncryptionEvent(method, playerName);
+        PasswordEncryptionEvent event = new PasswordEncryptionEvent(method);
         pluginManager.callEvent(event);
         return event.getMethod();
     }
@@ -154,12 +152,11 @@ public class PasswordSecurity implements Reloadable {
         if (HashAlgorithm.CUSTOM.equals(algorithm) || HashAlgorithm.PLAINTEXT.equals(algorithm)) {
             return null;
         }
-        return hashAlgorithmFactory.newInstance(algorithm.getClazz());
+        return encryptionMethodFactory.newInstance(algorithm.getClazz());
     }
 
-    private void hashPasswordForNewAlgorithm(String password, String playerName) {
+    private void hashAndSavePasswordWithNewAlgorithm(String password, String playerName) {
-        HashedPassword hashedPassword = initializeEncryptionMethodWithEvent(algorithm, playerName)
+        HashedPassword hashedPassword = encryptionMethod.computeHash(password, playerName);
-            .computeHash(password, playerName);
         dataSource.updatePassword(playerName, hashedPassword);
     }
 

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

@@ -61,10 +61,6 @@ public final class SecuritySettings implements SettingsHolder {
     public static final Property<HashAlgorithm> PASSWORD_HASH =
         newProperty(HashAlgorithm.class, "settings.security.passwordHash", HashAlgorithm.SHA256);
 
-    @Comment("Salt length for the SALTED2MD5 MD5(MD5(password)+salt)")
-    public static final Property<Integer> DOUBLE_MD5_SALT_LENGTH =
-        newProperty("settings.security.doubleMD5SaltLength", 8);
-
     @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.",
@@ -75,6 +71,10 @@ public final class SecuritySettings implements SettingsHolder {
     public static final Property<Set<HashAlgorithm>> LEGACY_HASHES =
         new EnumSetProperty<>(HashAlgorithm.class, "settings.security.legacyHashes");
 
+    @Comment("Salt length for the SALTED2MD5 MD5(MD5(password)+salt)")
+    public static final Property<Integer> DOUBLE_MD5_SALT_LENGTH =
+        newProperty("settings.security.doubleMD5SaltLength", 8);
+
     @Comment("Number of rounds to use if passwordHash is set to PBKDF2. Default is 10000")
     public static final Property<Integer> PBKDF2_NUMBER_OF_ROUNDS =
         newProperty("settings.security.pbkdf2Rounds", 10000);

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

@@ -57,7 +57,10 @@ public class HashAlgorithmIntegrationTest {
         // given / when / then
         for (HashAlgorithm algorithm : HashAlgorithm.values()) {
             if (!HashAlgorithm.CUSTOM.equals(algorithm) && !HashAlgorithm.PLAINTEXT.equals(algorithm)) {
-                EncryptionMethod method = injector.newInstance(algorithm.getClazz());
+                EncryptionMethod method = injector.createIfHasDependencies(algorithm.getClazz());
+                if (method == null) {
+                    fail("Could not create '" + algorithm.getClazz() + "' - forgot to provide some class?");
+                }
                 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()));

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

@@ -13,6 +13,7 @@ import fr.xephi.authme.initialization.factory.Factory;
 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.Md5;
 import fr.xephi.authme.settings.Settings;
 import fr.xephi.authme.settings.properties.HooksSettings;
 import fr.xephi.authme.settings.properties.SecuritySettings;
@@ -21,7 +22,6 @@ import org.bukkit.plugin.PluginManager;
 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.stubbing.Answer;
@@ -32,12 +32,14 @@ import java.util.Set;
 import static com.google.common.collect.Sets.newHashSet;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.equalToIgnoringCase;
+import static org.hamcrest.Matchers.instanceOf;
 import static org.junit.Assert.assertThat;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.hamcrest.MockitoHamcrest.argThat;
@@ -166,7 +168,6 @@ public class PasswordSecurityTest {
         // then
         assertThat(result, equalTo(false));
         verify(dataSource).getPassword(playerName);
-        verify(pluginManager, never()).callEvent(any(Event.class));
         verify(method, never()).comparePassword(anyString(), any(HashedPassword.class), anyString());
     }
 
@@ -204,7 +205,7 @@ public class PasswordSecurityTest {
     }
 
     @Test
-    public void shouldTryAllMethodsAndFail() {
+    public void shouldTryLegacyMethodsAndFail() {
         // given
         HashedPassword password = new HashedPassword("hashNotMatchingAnyMethod", "someBogusSalt");
         String playerName = "asfd";
@@ -240,11 +241,9 @@ public class PasswordSecurityTest {
 
         // then
         assertThat(result, equalTo(hashedPassword));
-        ArgumentCaptor<PasswordEncryptionEvent> captor = ArgumentCaptor.forClass(PasswordEncryptionEvent.class);
+        // Check that an event was fired twice: once on test setup, and once because we called reload()
-        verify(pluginManager).callEvent(captor.capture());
+        verify(pluginManager, times(2)).callEvent(any(PasswordEncryptionEvent.class));
-        PasswordEncryptionEvent event = captor.getValue();
         assertThat(Joomla.class.equals(caughtClassInEvent), equalTo(true));
-        assertThat(event.getPlayerName(), equalTo(usernameLowerCase));
     }
 
     @Test
@@ -263,7 +262,8 @@ public class PasswordSecurityTest {
         // then
         assertThat(result, equalTo(false));
         verify(dataSource, never()).getAuth(anyString());
-        verify(pluginManager).callEvent(any(PasswordEncryptionEvent.class));
+        // Check that an event was fired twice: once on test setup, and once because we called reload()
+        verify(pluginManager, times(2)).callEvent(any(PasswordEncryptionEvent.class));
         verify(method, never()).comparePassword(anyString(), any(HashedPassword.class), anyString());
     }
 
@@ -273,13 +273,14 @@ public class PasswordSecurityTest {
         given(settings.getProperty(SecuritySettings.PASSWORD_HASH)).willReturn(HashAlgorithm.MD5);
         given(settings.getProperty(SecuritySettings.LEGACY_HASHES))
             .willReturn(newHashSet(HashAlgorithm.CUSTOM, HashAlgorithm.BCRYPT));
+        reset(pluginManager); // reset behavior when the event is emitted to check that we create an instance of Md5.java
 
         // when
         passwordSecurity.reload();
 
         // then
-        assertThat(ReflectionTestUtils.getFieldValue(PasswordSecurity.class, passwordSecurity, "algorithm"),
+        assertThat(ReflectionTestUtils.getFieldValue(PasswordSecurity.class, passwordSecurity, "encryptionMethod"),
-            equalTo(HashAlgorithm.MD5));
+            instanceOf(Md5.class));
         Set<HashAlgorithm> legacyHashesSet = newHashSet(HashAlgorithm.CUSTOM, HashAlgorithm.BCRYPT);
         assertThat(ReflectionTestUtils.getFieldValue(PasswordSecurity.class, passwordSecurity, "legacyAlgorithms"),
             equalTo(legacyHashesSet));