From e85dbe81e5b77c97a52a4bcfb9d1c8962ac57ed0 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Wed, 30 Dec 2015 22:45:18 +0100 Subject: [PATCH] #358 Ensure lowercase player name, issue cleanup - Ensure within PasswordSecurity that the player name is lowercase - Remove check for MD5VB separate salt (we only generate hashes with the salt embedded, so either we need to change that or we don't need this check) - Remove obsolete TODO --- .../xephi/authme/cache/auth/PlayerAuth.java | 6 ---- .../authme/ChangePasswordAdminCommand.java | 4 +-- .../authme/security/PasswordSecurity.java | 10 +++--- .../authme/security/PasswordSecurityTest.java | 34 ++++++++++++------- .../crypts/AbstractEncryptionMethodTest.java | 1 - 5 files changed, 30 insertions(+), 25 deletions(-) diff --git a/src/main/java/fr/xephi/authme/cache/auth/PlayerAuth.java b/src/main/java/fr/xephi/authme/cache/auth/PlayerAuth.java index f172f221a..3b41e5115 100644 --- a/src/main/java/fr/xephi/authme/cache/auth/PlayerAuth.java +++ b/src/main/java/fr/xephi/authme/cache/auth/PlayerAuth.java @@ -281,12 +281,6 @@ public class PlayerAuth { } public EncryptedPassword getPassword() { - // TODO #358: Check whether this check is really necessary. It's been here since the first commit. - /*if (Settings.getPasswordHash == HashAlgorithm.MD5VB) { - if (salt != null && !salt.isEmpty() && Settings.getPasswordHash == HashAlgorithm.MD5VB) { - return "$MD5vb$" + salt + "$" + hash; - } - }*/ return password; } diff --git a/src/main/java/fr/xephi/authme/command/executable/authme/ChangePasswordAdminCommand.java b/src/main/java/fr/xephi/authme/command/executable/authme/ChangePasswordAdminCommand.java index 17f3b333c..da1a626da 100644 --- a/src/main/java/fr/xephi/authme/command/executable/authme/ChangePasswordAdminCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/authme/ChangePasswordAdminCommand.java @@ -67,8 +67,8 @@ public class ChangePasswordAdminCommand implements ExecutableCommand { return; } - // TODO #358: Do we always pass lowercase name?? In that case we need to do that in PasswordSecurity! - EncryptedPassword encryptedPassword = commandService.getPasswordSecurity().computeHash(playerPass, playerNameLowerCase); + EncryptedPassword encryptedPassword = commandService.getPasswordSecurity() + .computeHash(playerPass, playerNameLowerCase); auth.setPassword(encryptedPassword); if (!dataSource.updatePassword(auth)) { diff --git a/src/main/java/fr/xephi/authme/security/PasswordSecurity.java b/src/main/java/fr/xephi/authme/security/PasswordSecurity.java index ba1d2e02e..5f88921e1 100644 --- a/src/main/java/fr/xephi/authme/security/PasswordSecurity.java +++ b/src/main/java/fr/xephi/authme/security/PasswordSecurity.java @@ -30,8 +30,9 @@ public class PasswordSecurity { } public EncryptedPassword computeHash(HashAlgorithm algorithm, String password, String playerName) { - EncryptionMethod method = initializeEncryptionMethod(algorithm, playerName); - return method.computeHash(password, playerName); + String playerLowerCase = playerName.toLowerCase(); + EncryptionMethod method = initializeEncryptionMethod(algorithm, playerLowerCase); + return method.computeHash(password, playerLowerCase); } public boolean comparePassword(String password, String playerName) { @@ -52,8 +53,9 @@ public class PasswordSecurity { return false; } - return method.comparePassword(password, encryptedPassword, playerName) - || supportOldAlgorithm && compareWithAllEncryptionMethods(password, encryptedPassword, playerName); + String playerLowerCase = playerName.toLowerCase(); + return method.comparePassword(password, encryptedPassword, playerLowerCase) + || supportOldAlgorithm && compareWithAllEncryptionMethods(password, encryptedPassword, playerLowerCase); } /** diff --git a/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java b/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java index ae8e49e73..02fa207fc 100644 --- a/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java +++ b/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java @@ -16,10 +16,12 @@ import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.equalToIgnoringCase; 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.Matchers.argThat; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doCallRealMethod; import static org.mockito.Mockito.mock; @@ -64,12 +66,14 @@ public class PasswordSecurityTest { // given EncryptedPassword password = new EncryptedPassword("$TEST$10$SOME_HASH", null); String playerName = "Tester"; + // Calls to EncryptionMethod are always with the lower-case version of the name + String playerLowerCase = playerName.toLowerCase(); 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); + given(method.comparePassword(clearTextPass, password, playerLowerCase)).willReturn(true); PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.BCRYPT, pluginManager, false); // when @@ -79,7 +83,7 @@ public class PasswordSecurityTest { assertThat(result, equalTo(true)); verify(dataSource).getAuth(playerName); verify(pluginManager).callEvent(any(PasswordEncryptionEvent.class)); - verify(method).comparePassword(clearTextPass, password, playerName); + verify(method).comparePassword(clearTextPass, password, playerLowerCase); } @Test @@ -87,12 +91,13 @@ public class PasswordSecurityTest { // given EncryptedPassword password = new EncryptedPassword("$TEST$10$SOME_HASH", null); String playerName = "My_PLayer"; + String playerLowerCase = playerName.toLowerCase(); 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); + given(method.comparePassword(clearTextPass, password, playerLowerCase)).willReturn(false); PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.CUSTOM, pluginManager, false); // when @@ -102,7 +107,7 @@ public class PasswordSecurityTest { assertThat(result, equalTo(false)); verify(dataSource).getAuth(playerName); verify(pluginManager).callEvent(any(PasswordEncryptionEvent.class)); - verify(method).comparePassword(clearTextPass, password, playerName); + verify(method).comparePassword(clearTextPass, password, playerLowerCase); } @Test @@ -127,10 +132,11 @@ public class PasswordSecurityTest { @Test public void shouldTryOtherMethodsForFailedPassword() { // given - // BCRYPT2Y hash for "Test" + // BCRYPT hash for "Test" EncryptedPassword password = new EncryptedPassword("$2y$10$2e6d2193f43501c926e25elvWlPmWczmrfrnbZV0dUZGITjYjnkkW"); String playerName = "somePlayer"; + String playerLowerCase = playerName.toLowerCase(); String clearTextPass = "Test"; // MD5 hash for "Test" EncryptedPassword newPassword = new EncryptedPassword("0cbc6611f5540bd0809a388dc95a615b"); @@ -139,9 +145,9 @@ public class PasswordSecurityTest { 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); + given(dataSource.getAuth(argThat(equalToIgnoringCase(playerName)))).willReturn(auth); + given(method.comparePassword(clearTextPass, password, playerLowerCase)).willReturn(false); + given(method.computeHash(clearTextPass, playerLowerCase)).willReturn(newPassword); PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.MD5, pluginManager, true); // when @@ -149,9 +155,12 @@ public class PasswordSecurityTest { // then assertThat(result, equalTo(true)); - verify(dataSource, times(2)).getAuth(playerName); + // Note ljacqu 20151230: We need to check the player name in a case-insensitive way because the methods within + // PasswordSecurity may convert the name into all lower-case. This is desired because EncryptionMethod methods + // should only be invoked with all lower-case names. Data source is case-insensitive itself, so this is fine. + verify(dataSource, times(2)).getAuth(argThat(equalToIgnoringCase(playerName))); verify(pluginManager, times(2)).callEvent(any(PasswordEncryptionEvent.class)); - verify(method).comparePassword(clearTextPass, password, playerName); + verify(method).comparePassword(clearTextPass, password, playerLowerCase); verify(auth).setPassword(newPassword); ArgumentCaptor captor = ArgumentCaptor.forClass(PlayerAuth.class); @@ -164,8 +173,9 @@ public class PasswordSecurityTest { // given String password = "MyP@ssword"; String username = "theUserInTest"; + String usernameLowerCase = username.toLowerCase(); EncryptedPassword encryptedPassword = new EncryptedPassword("$T$est#Hash", "__someSalt__"); - given(method.computeHash(password, username)).willReturn(encryptedPassword); + given(method.computeHash(password, usernameLowerCase)).willReturn(encryptedPassword); PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.JOOMLA, pluginManager, true); // when @@ -177,7 +187,7 @@ public class PasswordSecurityTest { verify(pluginManager).callEvent(captor.capture()); PasswordEncryptionEvent event = captor.getValue(); assertThat(JOOMLA.class.equals(caughtClassInEvent), equalTo(true)); - assertThat(event.getPlayerName(), equalTo(username)); + assertThat(event.getPlayerName(), equalTo(usernameLowerCase)); } @Test diff --git a/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java b/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java index cb81164b5..2fd62173b 100644 --- a/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java +++ b/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java @@ -16,7 +16,6 @@ import static org.junit.Assert.assertTrue; /** * Test for implementations of {@link EncryptionMethod}. */ -// TODO #358: Remove NoSuchAlgorithm try-catch-es when no longer necessary public abstract class AbstractEncryptionMethodTest { /** The username used to query {@link EncryptionMethod#comparePassword}. */