From 3bb7ff2b8549459f7f8ea68cef8f459d945d0f4d Mon Sep 17 00:00:00 2001 From: ljacqu Date: Thu, 14 Apr 2016 11:48:24 +0200 Subject: [PATCH] #653 Empty salt column causes error when retrieving password - Handle potentially empty salt column in MySQL and SQLite - Create unit tests reflecting these cases --- .../fr/xephi/authme/datasource/MySQL.java | 18 ++++----- .../fr/xephi/authme/datasource/SQLite.java | 22 +++++------ .../AbstractDataSourceIntegrationTest.java | 37 ++++++++++++++++++- .../datasource/MySqlIntegrationTest.java | 4 +- .../datasource/SQLiteIntegrationTest.java | 3 +- 5 files changed, 59 insertions(+), 25 deletions(-) diff --git a/src/main/java/fr/xephi/authme/datasource/MySQL.java b/src/main/java/fr/xephi/authme/datasource/MySQL.java index 75655cd72..236d22e1e 100644 --- a/src/main/java/fr/xephi/authme/datasource/MySQL.java +++ b/src/main/java/fr/xephi/authme/datasource/MySQL.java @@ -274,20 +274,20 @@ public class MySQL implements DataSource { @Override public HashedPassword getPassword(String user) { - String sql = "SELECT " + col.PASSWORD + "," + col.SALT + " FROM " + tableName - + " WHERE " + col.NAME + "=?;"; - ResultSet rs = null; + boolean useSalt = !col.SALT.isEmpty(); + String sql = "SELECT " + col.PASSWORD + + (useSalt ? ", " + col.SALT : "") + + " FROM " + tableName + " WHERE " + col.NAME + "=?;"; try (Connection con = getConnection(); PreparedStatement pst = con.prepareStatement(sql)) { pst.setString(1, user.toLowerCase()); - rs = pst.executeQuery(); - if (rs.next()) { - return new HashedPassword(rs.getString(col.PASSWORD), - !col.SALT.isEmpty() ? rs.getString(col.SALT) : null); + try (ResultSet rs = pst.executeQuery()) { + if (rs.next()) { + return new HashedPassword(rs.getString(col.PASSWORD), + useSalt ? rs.getString(col.SALT) : null); + } } } catch (SQLException ex) { logSqlException(ex); - } finally { - close(rs); } return null; } diff --git a/src/main/java/fr/xephi/authme/datasource/SQLite.java b/src/main/java/fr/xephi/authme/datasource/SQLite.java index a310b7b67..dfc747404 100644 --- a/src/main/java/fr/xephi/authme/datasource/SQLite.java +++ b/src/main/java/fr/xephi/authme/datasource/SQLite.java @@ -154,22 +154,20 @@ public class SQLite implements DataSource { @Override public HashedPassword getPassword(String user) { - PreparedStatement pst = null; - ResultSet rs = null; - try { - pst = con.prepareStatement("SELECT " + col.PASSWORD + "," + col.SALT - + " FROM " + tableName + " WHERE " + col.NAME + "=?"); + boolean useSalt = !col.SALT.isEmpty(); + String sql = "SELECT " + col.PASSWORD + + (useSalt ? ", " + col.SALT : "") + + " FROM " + tableName + " WHERE " + col.NAME + "=?"; + try (PreparedStatement pst = con.prepareStatement(sql)) { pst.setString(1, user); - rs = pst.executeQuery(); - if (rs.next()) { - return new HashedPassword(rs.getString(col.PASSWORD), - !col.SALT.isEmpty() ? rs.getString(col.SALT) : null); + try (ResultSet rs = pst.executeQuery()) { + if (rs.next()) { + return new HashedPassword(rs.getString(col.PASSWORD), + useSalt ? rs.getString(col.SALT) : null); + } } } catch (SQLException ex) { logSqlException(ex); - } finally { - close(rs); - close(pst); } return null; } diff --git a/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java b/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java index 0231a8d63..30f3484ed 100644 --- a/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java +++ b/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java @@ -22,7 +22,11 @@ import static org.junit.Assume.assumeThat; */ public abstract class AbstractDataSourceIntegrationTest { - protected abstract DataSource getDataSource(); + protected DataSource getDataSource() { + return getDataSource("salt"); + } + + protected abstract DataSource getDataSource(String saltColumn); @Test public void shouldReturnIfAuthIsAvailableOrNot() { @@ -56,6 +60,22 @@ public abstract class AbstractDataSourceIntegrationTest { assertThat(userPassword, equalToHash("b28c32f624a4eb161d6adc9acb5bfc5b", "f750ba32")); } + @Test + public void shouldReturnPasswordWithEmptySaltColumn() { + // given + DataSource dataSource = getDataSource(""); + + // when + HashedPassword bobbyPassword = dataSource.getPassword("bobby"); + HashedPassword invalidPassword = dataSource.getPassword("doesNotExist"); + HashedPassword userPassword = dataSource.getPassword("user"); + + // then + assertThat(bobbyPassword, equalToHash("$SHA$11aa0706173d7272$dbba966")); + assertThat(invalidPassword, nullValue()); + assertThat(userPassword, equalToHash("b28c32f624a4eb161d6adc9acb5bfc5b")); + } + @Test public void shouldGetAuth() { // given @@ -133,6 +153,21 @@ public abstract class AbstractDataSourceIntegrationTest { assertThat(dataSource.getPassword("user"), equalToHash(newHash)); } + @Test + public void shouldUpdatePasswordWithNoSalt() { + // given + DataSource dataSource = getDataSource(""); + HashedPassword newHash = new HashedPassword("new_hash", "1241"); + + // when + boolean response1 = dataSource.updatePassword("user", newHash); + boolean response2 = dataSource.updatePassword("non-existent-name", new HashedPassword("asdfasdf", "a1f34ec")); + + // then + assertThat(response1 && response2, equalTo(true)); + assertThat(dataSource.getPassword("user"), equalToHash("new_hash")); + } + @Test public void shouldRemovePlayerAuth() { // given diff --git a/src/test/java/fr/xephi/authme/datasource/MySqlIntegrationTest.java b/src/test/java/fr/xephi/authme/datasource/MySqlIntegrationTest.java index 81e2e122b..88c6601c7 100644 --- a/src/test/java/fr/xephi/authme/datasource/MySqlIntegrationTest.java +++ b/src/test/java/fr/xephi/authme/datasource/MySqlIntegrationTest.java @@ -53,7 +53,6 @@ public class MySqlIntegrationTest extends AbstractDataSourceIntegrationTest { }); set(DatabaseSettings.MYSQL_DATABASE, "h2_test"); set(DatabaseSettings.MYSQL_TABLE, "authme"); - set(DatabaseSettings.MYSQL_COL_SALT, "salt"); ConsoleLoggerTestInitializer.setupLogger(); Path sqlInitFile = TestHelper.getJarPath("/datasource-integration/sql-initialize.sql"); @@ -80,7 +79,8 @@ public class MySqlIntegrationTest extends AbstractDataSourceIntegrationTest { } @Override - protected DataSource getDataSource() { + protected DataSource getDataSource(String saltColumn) { + when(settings.getProperty(DatabaseSettings.MYSQL_COL_SALT)).thenReturn(saltColumn); return new MySQL(settings, hikariSource); } diff --git a/src/test/java/fr/xephi/authme/datasource/SQLiteIntegrationTest.java b/src/test/java/fr/xephi/authme/datasource/SQLiteIntegrationTest.java index 2ef4e15a5..1ec9d9901 100644 --- a/src/test/java/fr/xephi/authme/datasource/SQLiteIntegrationTest.java +++ b/src/test/java/fr/xephi/authme/datasource/SQLiteIntegrationTest.java @@ -75,7 +75,8 @@ public class SQLiteIntegrationTest extends AbstractDataSourceIntegrationTest { } @Override - protected DataSource getDataSource() { + protected DataSource getDataSource(String saltColumn) { + when(settings.getProperty(DatabaseSettings.MYSQL_COL_SALT)).thenReturn(saltColumn); return new SQLite(settings, con); }