mirror of
https://github.com/AuthMe/AuthMeReloaded.git
synced 2024-12-24 01:27:35 +01:00
#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
This commit is contained in:
parent
300a621e1c
commit
e85dbe81e5
@ -281,12 +281,6 @@ public class PlayerAuth {
|
|||||||
}
|
}
|
||||||
|
|
||||||
public EncryptedPassword getPassword() {
|
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;
|
return password;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -67,8 +67,8 @@ public class ChangePasswordAdminCommand implements ExecutableCommand {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
// TODO #358: Do we always pass lowercase name?? In that case we need to do that in PasswordSecurity!
|
EncryptedPassword encryptedPassword = commandService.getPasswordSecurity()
|
||||||
EncryptedPassword encryptedPassword = commandService.getPasswordSecurity().computeHash(playerPass, playerNameLowerCase);
|
.computeHash(playerPass, playerNameLowerCase);
|
||||||
auth.setPassword(encryptedPassword);
|
auth.setPassword(encryptedPassword);
|
||||||
|
|
||||||
if (!dataSource.updatePassword(auth)) {
|
if (!dataSource.updatePassword(auth)) {
|
||||||
|
@ -30,8 +30,9 @@ public class PasswordSecurity {
|
|||||||
}
|
}
|
||||||
|
|
||||||
public EncryptedPassword computeHash(HashAlgorithm algorithm, String password, String playerName) {
|
public EncryptedPassword computeHash(HashAlgorithm algorithm, String password, String playerName) {
|
||||||
EncryptionMethod method = initializeEncryptionMethod(algorithm, playerName);
|
String playerLowerCase = playerName.toLowerCase();
|
||||||
return method.computeHash(password, playerName);
|
EncryptionMethod method = initializeEncryptionMethod(algorithm, playerLowerCase);
|
||||||
|
return method.computeHash(password, playerLowerCase);
|
||||||
}
|
}
|
||||||
|
|
||||||
public boolean comparePassword(String password, String playerName) {
|
public boolean comparePassword(String password, String playerName) {
|
||||||
@ -52,8 +53,9 @@ public class PasswordSecurity {
|
|||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
return method.comparePassword(password, encryptedPassword, playerName)
|
String playerLowerCase = playerName.toLowerCase();
|
||||||
|| supportOldAlgorithm && compareWithAllEncryptionMethods(password, encryptedPassword, playerName);
|
return method.comparePassword(password, encryptedPassword, playerLowerCase)
|
||||||
|
|| supportOldAlgorithm && compareWithAllEncryptionMethods(password, encryptedPassword, playerLowerCase);
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
@ -16,10 +16,12 @@ import org.mockito.invocation.InvocationOnMock;
|
|||||||
import org.mockito.stubbing.Answer;
|
import org.mockito.stubbing.Answer;
|
||||||
|
|
||||||
import static org.hamcrest.Matchers.equalTo;
|
import static org.hamcrest.Matchers.equalTo;
|
||||||
|
import static org.hamcrest.Matchers.equalToIgnoringCase;
|
||||||
import static org.junit.Assert.assertThat;
|
import static org.junit.Assert.assertThat;
|
||||||
import static org.mockito.BDDMockito.given;
|
import static org.mockito.BDDMockito.given;
|
||||||
import static org.mockito.Matchers.any;
|
import static org.mockito.Matchers.any;
|
||||||
import static org.mockito.Matchers.anyString;
|
import static org.mockito.Matchers.anyString;
|
||||||
|
import static org.mockito.Matchers.argThat;
|
||||||
import static org.mockito.Mockito.doAnswer;
|
import static org.mockito.Mockito.doAnswer;
|
||||||
import static org.mockito.Mockito.doCallRealMethod;
|
import static org.mockito.Mockito.doCallRealMethod;
|
||||||
import static org.mockito.Mockito.mock;
|
import static org.mockito.Mockito.mock;
|
||||||
@ -64,12 +66,14 @@ public class PasswordSecurityTest {
|
|||||||
// given
|
// given
|
||||||
EncryptedPassword password = new EncryptedPassword("$TEST$10$SOME_HASH", null);
|
EncryptedPassword password = new EncryptedPassword("$TEST$10$SOME_HASH", null);
|
||||||
String playerName = "Tester";
|
String playerName = "Tester";
|
||||||
|
// Calls to EncryptionMethod are always with the lower-case version of the name
|
||||||
|
String playerLowerCase = playerName.toLowerCase();
|
||||||
String clearTextPass = "myPassTest";
|
String clearTextPass = "myPassTest";
|
||||||
|
|
||||||
PlayerAuth auth = mock(PlayerAuth.class);
|
PlayerAuth auth = mock(PlayerAuth.class);
|
||||||
given(auth.getPassword()).willReturn(password);
|
given(auth.getPassword()).willReturn(password);
|
||||||
given(dataSource.getAuth(playerName)).willReturn(auth);
|
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);
|
PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.BCRYPT, pluginManager, false);
|
||||||
|
|
||||||
// when
|
// when
|
||||||
@ -79,7 +83,7 @@ public class PasswordSecurityTest {
|
|||||||
assertThat(result, equalTo(true));
|
assertThat(result, equalTo(true));
|
||||||
verify(dataSource).getAuth(playerName);
|
verify(dataSource).getAuth(playerName);
|
||||||
verify(pluginManager).callEvent(any(PasswordEncryptionEvent.class));
|
verify(pluginManager).callEvent(any(PasswordEncryptionEvent.class));
|
||||||
verify(method).comparePassword(clearTextPass, password, playerName);
|
verify(method).comparePassword(clearTextPass, password, playerLowerCase);
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
@ -87,12 +91,13 @@ public class PasswordSecurityTest {
|
|||||||
// given
|
// given
|
||||||
EncryptedPassword password = new EncryptedPassword("$TEST$10$SOME_HASH", null);
|
EncryptedPassword password = new EncryptedPassword("$TEST$10$SOME_HASH", null);
|
||||||
String playerName = "My_PLayer";
|
String playerName = "My_PLayer";
|
||||||
|
String playerLowerCase = playerName.toLowerCase();
|
||||||
String clearTextPass = "passw0Rd1";
|
String clearTextPass = "passw0Rd1";
|
||||||
|
|
||||||
PlayerAuth auth = mock(PlayerAuth.class);
|
PlayerAuth auth = mock(PlayerAuth.class);
|
||||||
given(auth.getPassword()).willReturn(password);
|
given(auth.getPassword()).willReturn(password);
|
||||||
given(dataSource.getAuth(playerName)).willReturn(auth);
|
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);
|
PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.CUSTOM, pluginManager, false);
|
||||||
|
|
||||||
// when
|
// when
|
||||||
@ -102,7 +107,7 @@ public class PasswordSecurityTest {
|
|||||||
assertThat(result, equalTo(false));
|
assertThat(result, equalTo(false));
|
||||||
verify(dataSource).getAuth(playerName);
|
verify(dataSource).getAuth(playerName);
|
||||||
verify(pluginManager).callEvent(any(PasswordEncryptionEvent.class));
|
verify(pluginManager).callEvent(any(PasswordEncryptionEvent.class));
|
||||||
verify(method).comparePassword(clearTextPass, password, playerName);
|
verify(method).comparePassword(clearTextPass, password, playerLowerCase);
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
@ -127,10 +132,11 @@ public class PasswordSecurityTest {
|
|||||||
@Test
|
@Test
|
||||||
public void shouldTryOtherMethodsForFailedPassword() {
|
public void shouldTryOtherMethodsForFailedPassword() {
|
||||||
// given
|
// given
|
||||||
// BCRYPT2Y hash for "Test"
|
// BCRYPT hash for "Test"
|
||||||
EncryptedPassword password =
|
EncryptedPassword password =
|
||||||
new EncryptedPassword("$2y$10$2e6d2193f43501c926e25elvWlPmWczmrfrnbZV0dUZGITjYjnkkW");
|
new EncryptedPassword("$2y$10$2e6d2193f43501c926e25elvWlPmWczmrfrnbZV0dUZGITjYjnkkW");
|
||||||
String playerName = "somePlayer";
|
String playerName = "somePlayer";
|
||||||
|
String playerLowerCase = playerName.toLowerCase();
|
||||||
String clearTextPass = "Test";
|
String clearTextPass = "Test";
|
||||||
// MD5 hash for "Test"
|
// MD5 hash for "Test"
|
||||||
EncryptedPassword newPassword = new EncryptedPassword("0cbc6611f5540bd0809a388dc95a615b");
|
EncryptedPassword newPassword = new EncryptedPassword("0cbc6611f5540bd0809a388dc95a615b");
|
||||||
@ -139,9 +145,9 @@ public class PasswordSecurityTest {
|
|||||||
doCallRealMethod().when(auth).getPassword();
|
doCallRealMethod().when(auth).getPassword();
|
||||||
doCallRealMethod().when(auth).setPassword(any(EncryptedPassword.class));
|
doCallRealMethod().when(auth).setPassword(any(EncryptedPassword.class));
|
||||||
auth.setPassword(password);
|
auth.setPassword(password);
|
||||||
given(dataSource.getAuth(playerName)).willReturn(auth);
|
given(dataSource.getAuth(argThat(equalToIgnoringCase(playerName)))).willReturn(auth);
|
||||||
given(method.comparePassword(clearTextPass, password, playerName)).willReturn(false);
|
given(method.comparePassword(clearTextPass, password, playerLowerCase)).willReturn(false);
|
||||||
given(method.computeHash(clearTextPass, playerName)).willReturn(newPassword);
|
given(method.computeHash(clearTextPass, playerLowerCase)).willReturn(newPassword);
|
||||||
PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.MD5, pluginManager, true);
|
PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.MD5, pluginManager, true);
|
||||||
|
|
||||||
// when
|
// when
|
||||||
@ -149,9 +155,12 @@ public class PasswordSecurityTest {
|
|||||||
|
|
||||||
// then
|
// then
|
||||||
assertThat(result, equalTo(true));
|
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(pluginManager, times(2)).callEvent(any(PasswordEncryptionEvent.class));
|
||||||
verify(method).comparePassword(clearTextPass, password, playerName);
|
verify(method).comparePassword(clearTextPass, password, playerLowerCase);
|
||||||
verify(auth).setPassword(newPassword);
|
verify(auth).setPassword(newPassword);
|
||||||
|
|
||||||
ArgumentCaptor<PlayerAuth> captor = ArgumentCaptor.forClass(PlayerAuth.class);
|
ArgumentCaptor<PlayerAuth> captor = ArgumentCaptor.forClass(PlayerAuth.class);
|
||||||
@ -164,8 +173,9 @@ public class PasswordSecurityTest {
|
|||||||
// given
|
// given
|
||||||
String password = "MyP@ssword";
|
String password = "MyP@ssword";
|
||||||
String username = "theUserInTest";
|
String username = "theUserInTest";
|
||||||
|
String usernameLowerCase = username.toLowerCase();
|
||||||
EncryptedPassword encryptedPassword = new EncryptedPassword("$T$est#Hash", "__someSalt__");
|
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);
|
PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.JOOMLA, pluginManager, true);
|
||||||
|
|
||||||
// when
|
// when
|
||||||
@ -177,7 +187,7 @@ public class PasswordSecurityTest {
|
|||||||
verify(pluginManager).callEvent(captor.capture());
|
verify(pluginManager).callEvent(captor.capture());
|
||||||
PasswordEncryptionEvent event = captor.getValue();
|
PasswordEncryptionEvent event = captor.getValue();
|
||||||
assertThat(JOOMLA.class.equals(caughtClassInEvent), equalTo(true));
|
assertThat(JOOMLA.class.equals(caughtClassInEvent), equalTo(true));
|
||||||
assertThat(event.getPlayerName(), equalTo(username));
|
assertThat(event.getPlayerName(), equalTo(usernameLowerCase));
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
|
@ -16,7 +16,6 @@ import static org.junit.Assert.assertTrue;
|
|||||||
/**
|
/**
|
||||||
* Test for implementations of {@link EncryptionMethod}.
|
* Test for implementations of {@link EncryptionMethod}.
|
||||||
*/
|
*/
|
||||||
// TODO #358: Remove NoSuchAlgorithm try-catch-es when no longer necessary
|
|
||||||
public abstract class AbstractEncryptionMethodTest {
|
public abstract class AbstractEncryptionMethodTest {
|
||||||
|
|
||||||
/** The username used to query {@link EncryptionMethod#comparePassword}. */
|
/** The username used to query {@link EncryptionMethod#comparePassword}. */
|
||||||
|
Loading…
Reference in New Issue
Block a user