From 019390dfe050baa5209b5a1a01f56cba77bc9110 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Fri, 20 Nov 2015 23:00:13 +0100 Subject: [PATCH 1/2] Add unit test dependencies and create test for Log4JFilter Note that the new dependencies in the pom have the scope set to test, so they will not be included into the built artifact. A first test class illustrates the general way unit tests can be set up with JUnit, Mockito and Hamcrest matchers. --- pom.xml | 34 +++- .../java/fr/xephi/authme/Log4JFilterTest.java | 186 ++++++++++++++++++ 2 files changed, 214 insertions(+), 6 deletions(-) create mode 100644 test/main/java/fr/xephi/authme/Log4JFilterTest.java diff --git a/pom.xml b/pom.xml index 3dcdaa12..9b8fd761 100644 --- a/pom.xml +++ b/pom.xml @@ -76,6 +76,8 @@ + src/main/java + test/main/java org.apache.maven.plugins @@ -589,13 +591,33 @@ true - - - net.ricecode - string-similarity - 1.0.0 + + + junit + junit + test + 4.12 + + + org.hamcrest + java-hamcrest + test + 2.0.0.0 + + + org.mockito + mockito-core + test + 2.0.5-beta + + + + + net.ricecode + string-similarity + 1.0.0 compile true - + diff --git a/test/main/java/fr/xephi/authme/Log4JFilterTest.java b/test/main/java/fr/xephi/authme/Log4JFilterTest.java new file mode 100644 index 00000000..e7d11410 --- /dev/null +++ b/test/main/java/fr/xephi/authme/Log4JFilterTest.java @@ -0,0 +1,186 @@ +package fr.xephi.authme; + +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.when; + +import org.apache.logging.log4j.core.Filter.Result; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.message.Message; +import org.junit.Test; +import org.mockito.Mockito; + +/** + * Test for {@link Log4JFilter}. + */ +public class Log4JFilterTest { + + private final Log4JFilter log4JFilter = new Log4JFilter(); + + private static final String SENSITIVE_COMMAND = "User issued server command: /login pass pass"; + private static final String NORMAL_COMMAND = "User issued server command: /help"; + private static final String OTHER_COMMAND = "Starting the server... Write /l for logs"; + + // --------- + // Test the filter(LogEvent) method + // --------- + @Test + public void shouldFilterSensitiveLogEvent() { + // given + Message message = mockMessage(SENSITIVE_COMMAND); + LogEvent event = Mockito.mock(LogEvent.class); + when(event.getMessage()).thenReturn(message); + + // when + Result result = log4JFilter.filter(event); + + // then + assertThat(result, equalTo(Result.DENY)); + } + + @Test + public void shouldNotFilterIrrelevantLogEvent() { + // given + Message message = mockMessage(NORMAL_COMMAND); + LogEvent event = Mockito.mock(LogEvent.class); + when(event.getMessage()).thenReturn(message); + + // when + Result result = log4JFilter.filter(event); + + // then + assertThat(result, equalTo(Result.NEUTRAL)); + } + + @Test + public void shouldNotFilterLogEventWithNullMessage() { + // given + Message message = mockMessage(null); + LogEvent event = Mockito.mock(LogEvent.class); + when(event.getMessage()).thenReturn(message); + + // when + Result result = log4JFilter.filter(event); + + // then + assertThat(result, equalTo(Result.NEUTRAL)); + } + + @Test + public void shouldNotFilterWhenLogEventIsNull() { + // given / when + Result result = log4JFilter.filter(null); + + // then + assertThat(result, equalTo(Result.NEUTRAL)); + } + + // ---------- + // Test filter(Logger, Level, Marker, String, Object...) + // ---------- + @Test + public void shouldFilterSensitiveStringMessage() { + // given / when + Result result = log4JFilter.filter(null, null, null, SENSITIVE_COMMAND, new Object[0]); + + // then + assertThat(result, equalTo(Result.DENY)); + } + + @Test + public void shouldNotFilterNormalStringMessage() { + // given / when + Result result = log4JFilter.filter(null, null, null, NORMAL_COMMAND, new Object[0]); + + // then + assertThat(result, equalTo(Result.NEUTRAL)); + } + + @Test + public void shouldReturnNeutralForNullMessage() { + // given / when + Result result = log4JFilter.filter(null, null, null, null, new Object[0]); + + // then + assertThat(result, equalTo(Result.NEUTRAL)); + } + + // -------- + // Test filter(Logger, Level, Marker, Object, Throwable) + // -------- + @Test + public void shouldFilterSensitiveObjectMessage() { + // given / when + Result result = log4JFilter.filter(null, null, null, SENSITIVE_COMMAND, new Exception()); + + // then + assertThat(result, equalTo(Result.DENY)); + } + + @Test + public void shouldNotFilterNullObjectParam() { + // given / when + Result result = log4JFilter.filter(null, null, null, null, new Exception()); + + // then + assertThat(result, equalTo(Result.NEUTRAL)); + } + + @Test + public void shouldNotFilterIrrelevantMessage() { + // given / when + Result result = log4JFilter.filter(null, null, null, OTHER_COMMAND, new Exception()); + + // then + assertThat(result, equalTo(Result.NEUTRAL)); + } + + // -------- + // Test filter(Logger, Level, Marker, Message, Throwable) + // -------- + @Test + public void shouldFilterSensitiveMessage() { + // given + Message message = mockMessage(SENSITIVE_COMMAND); + + // when + Result result = log4JFilter.filter(null, null, null, message, new Exception()); + + // then + assertThat(result, equalTo(Result.DENY)); + } + + @Test + public void shouldNotFilterNonSensitiveMessage() { + // given + Message message = mockMessage(NORMAL_COMMAND); + + // when + Result result = log4JFilter.filter(null, null, null, message, new Exception()); + + // then + assertThat(result, equalTo(Result.NEUTRAL)); + } + + @Test + public void shouldNotFilterNullMessage() { + // given / when + Result result = log4JFilter.filter(null, null, null, null, new Exception()); + + // then + assertThat(result, equalTo(Result.NEUTRAL)); + } + + /** + * Mocks a {@link Message} object and makes it return the given formatted message. + * + * @param formattedMessage the formatted message the mock should return + * @return Message mock + */ + private static Message mockMessage(String formattedMessage) { + Message message = Mockito.mock(Message.class); + when(message.getFormattedMessage()).thenReturn(formattedMessage); + return message; + } + +} From 84de22c9c0241361c69a44bba9e0e84605ec9813 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Fri, 20 Nov 2015 23:17:50 +0100 Subject: [PATCH 2/2] Refactor Log4JFilter and improve branch coverage --- .../java/fr/xephi/authme/Log4JFilter.java | 103 +++++++++--------- .../fr/xephi/authme/util/StringUtils.java | 12 ++ .../java/fr/xephi/authme/Log4JFilterTest.java | 48 +++++++- 3 files changed, 112 insertions(+), 51 deletions(-) diff --git a/src/main/java/fr/xephi/authme/Log4JFilter.java b/src/main/java/fr/xephi/authme/Log4JFilter.java index dccaee6f..365b2cec 100644 --- a/src/main/java/fr/xephi/authme/Log4JFilter.java +++ b/src/main/java/fr/xephi/authme/Log4JFilter.java @@ -6,80 +6,50 @@ import org.apache.logging.log4j.core.LogEvent; import org.apache.logging.log4j.core.Logger; import org.apache.logging.log4j.message.Message; +import fr.xephi.authme.util.StringUtils; + /** - * + * Implements a filter for Log4j to skip sensitive AuthMe commands. * @author Xephi59 */ public class Log4JFilter implements org.apache.logging.log4j.core.Filter { + + /** List of commands (lower-case) to skip. */ + private static final String[] COMMANDS_TO_SKIP = { "/login ", "/l ", "/reg ", "/changepassword ", + "/unregister ", "/authme register ", "/authme changepassword ", "/authme reg ", "/authme cp ", + "/register " }; + /** Constructor. */ public Log4JFilter() { } @Override public Result filter(LogEvent record) { - try { - if (record == null || record.getMessage() == null) - return Result.NEUTRAL; - String logM = record.getMessage().getFormattedMessage().toLowerCase(); - if (!logM.contains("issued server command:")) - return Result.NEUTRAL; - if (!logM.contains("/login ") && !logM.contains("/l ") && !logM.contains("/reg ") && !logM.contains("/changepassword ") && !logM.contains("/unregister ") && !logM.contains("/authme register ") && !logM.contains("/authme changepassword ") && !logM.contains("/authme reg ") && !logM.contains("/authme cp ") && !logM.contains("/register ")) - return Result.NEUTRAL; - return Result.DENY; - } catch (NullPointerException npe) { - return Result.NEUTRAL; - } + if (record == null) { + return Result.NEUTRAL; + } + return validateMessage(record.getMessage()); } @Override public Result filter(Logger arg0, Level arg1, Marker arg2, String message, Object... arg4) { - try { - if (message == null) - return Result.NEUTRAL; - String logM = message.toLowerCase(); - if (!logM.contains("issued server command:")) - return Result.NEUTRAL; - if (!logM.contains("/login ") && !logM.contains("/l ") && !logM.contains("/reg ") && !logM.contains("/changepassword ") && !logM.contains("/unregister ") && !logM.contains("/authme register ") && !logM.contains("/authme changepassword ") && !logM.contains("/authme reg ") && !logM.contains("/authme cp ") && !logM.contains("/register ")) - return Result.NEUTRAL; - return Result.DENY; - } catch (NullPointerException npe) { - return Result.NEUTRAL; - } + return validateMessage(message); } @Override public Result filter(Logger arg0, Level arg1, Marker arg2, Object message, Throwable arg4) { - try { - if (message == null) - return Result.NEUTRAL; - String logM = message.toString().toLowerCase(); - if (!logM.contains("issued server command:")) - return Result.NEUTRAL; - if (!logM.contains("/login ") && !logM.contains("/l ") && !logM.contains("/reg ") && !logM.contains("/changepassword ") && !logM.contains("/unregister ") && !logM.contains("/authme register ") && !logM.contains("/authme changepassword ") && !logM.contains("/authme reg ") && !logM.contains("/authme cp ") && !logM.contains("/register ")) - return Result.NEUTRAL; - return Result.DENY; - } catch (NullPointerException npe) { - return Result.NEUTRAL; - } + if (message == null) { + return Result.NEUTRAL; + } + return validateMessage(message.toString()); } @Override public Result filter(Logger arg0, Level arg1, Marker arg2, Message message, Throwable arg4) { - try { - if (message == null) - return Result.NEUTRAL; - String logM = message.getFormattedMessage().toLowerCase(); - if (!logM.contains("issued server command:")) - return Result.NEUTRAL; - if (!logM.contains("/login ") && !logM.contains("/l ") && !logM.contains("/reg ") && !logM.contains("/changepassword ") && !logM.contains("/unregister ") && !logM.contains("/authme register ") && !logM.contains("/authme changepassword ") && !logM.contains("/authme reg ") && !logM.contains("/authme cp ") && !logM.contains("/register ")) - return Result.NEUTRAL; - return Result.DENY; - } catch (NullPointerException npe) { - return Result.NEUTRAL; - } + return validateMessage(message); } @Override @@ -92,4 +62,39 @@ public class Log4JFilter implements org.apache.logging.log4j.core.Filter { return Result.NEUTRAL; } + /** + * Validates a Message instance and returns the {@link Result} value + * depending depending on whether the message contains sensitive AuthMe + * data. + * + * @param message the Message object to verify + * @return the Result value + */ + private static Result validateMessage(Message message) { + if (message == null) { + return Result.NEUTRAL; + } + return validateMessage(message.getFormattedMessage()); + } + + /** + * Validates a message and returns the {@link Result} value depending + * depending on whether the message contains sensitive AuthMe data. + * + * @param message the message to verify + * @return the Result value + */ + private static Result validateMessage(String message) { + if (message == null) { + return Result.NEUTRAL; + } + + String lowerMessage = message.toLowerCase(); + if (lowerMessage.contains("issued server command:") + && StringUtils.containsAny(lowerMessage, COMMANDS_TO_SKIP)) { + return Result.DENY; + } + return Result.NEUTRAL; + } + } diff --git a/src/main/java/fr/xephi/authme/util/StringUtils.java b/src/main/java/fr/xephi/authme/util/StringUtils.java index eab7ea15..da6f91ba 100644 --- a/src/main/java/fr/xephi/authme/util/StringUtils.java +++ b/src/main/java/fr/xephi/authme/util/StringUtils.java @@ -25,4 +25,16 @@ public class StringUtils { // Determine the difference value, return the result return Math.abs(service.score(first, second) - 1.0); } + + public static boolean containsAny(String str, String... pieces) { + if (str == null) { + return false; + } + for (String piece : pieces) { + if (str.contains(piece)) { + return true; + } + } + return false; + } } diff --git a/test/main/java/fr/xephi/authme/Log4JFilterTest.java b/test/main/java/fr/xephi/authme/Log4JFilterTest.java index e7d11410..5614eb51 100644 --- a/test/main/java/fr/xephi/authme/Log4JFilterTest.java +++ b/test/main/java/fr/xephi/authme/Log4JFilterTest.java @@ -52,6 +52,20 @@ public class Log4JFilterTest { assertThat(result, equalTo(Result.NEUTRAL)); } + @Test + public void shouldNotFilterNonCommandLogEvent() { + // given + Message message = mockMessage(OTHER_COMMAND); + LogEvent event = Mockito.mock(LogEvent.class); + when(event.getMessage()).thenReturn(message); + + // when + Result result = log4JFilter.filter(event); + + // then + assertThat(result, equalTo(Result.NEUTRAL)); + } + @Test public void shouldNotFilterLogEventWithNullMessage() { // given @@ -96,6 +110,15 @@ public class Log4JFilterTest { assertThat(result, equalTo(Result.NEUTRAL)); } + @Test + public void shouldNotFilterNonCommandStringMessage() { + // given / when + Result result = log4JFilter.filter(null, null, null, OTHER_COMMAND, new Object[0]); + + // then + assertThat(result, equalTo(Result.NEUTRAL)); + } + @Test public void shouldReturnNeutralForNullMessage() { // given / when @@ -120,7 +143,7 @@ public class Log4JFilterTest { @Test public void shouldNotFilterNullObjectParam() { // given / when - Result result = log4JFilter.filter(null, null, null, null, new Exception()); + Result result = log4JFilter.filter(null, null, null, (Object) null, new Exception()); // then assertThat(result, equalTo(Result.NEUTRAL)); @@ -135,6 +158,15 @@ public class Log4JFilterTest { assertThat(result, equalTo(Result.NEUTRAL)); } + @Test + public void shouldNotFilterNonSensitiveCommand() { + // given / when + Result result = log4JFilter.filter(null, null, null, NORMAL_COMMAND, new Exception()); + + // then + assertThat(result, equalTo(Result.NEUTRAL)); + } + // -------- // Test filter(Logger, Level, Marker, Message, Throwable) // -------- @@ -162,10 +194,22 @@ public class Log4JFilterTest { assertThat(result, equalTo(Result.NEUTRAL)); } + @Test + public void shouldNotFilterNonCommandMessage() { + // given + Message message = mockMessage(OTHER_COMMAND); + + // when + Result result = log4JFilter.filter(null, null, null, message, new Exception()); + + // then + assertThat(result, equalTo(Result.NEUTRAL)); + } + @Test public void shouldNotFilterNullMessage() { // given / when - Result result = log4JFilter.filter(null, null, null, null, new Exception()); + Result result = log4JFilter.filter(null, null, null, (Message) null, new Exception()); // then assertThat(result, equalTo(Result.NEUTRAL));