From 923020bf071352acac0732a3658f3d5e46ecb55d Mon Sep 17 00:00:00 2001 From: ljacqu Date: Tue, 1 Dec 2015 22:17:18 +0100 Subject: [PATCH] Reduce duplication in Log filter implementations --- .../fr/xephi/authme/output/ConsoleFilter.java | 21 ++--- .../fr/xephi/authme/output/Log4JFilter.java | 50 ++++-------- .../xephi/authme/output/LogFilterHelper.java | 34 ++++++++ .../authme/output/ConsoleFilterTest.java | 77 +++++++++++++++++++ .../permission/AdminPermissionTest.java | 6 +- 5 files changed, 139 insertions(+), 49 deletions(-) create mode 100644 src/main/java/fr/xephi/authme/output/LogFilterHelper.java create mode 100644 src/test/java/fr/xephi/authme/output/ConsoleFilterTest.java diff --git a/src/main/java/fr/xephi/authme/output/ConsoleFilter.java b/src/main/java/fr/xephi/authme/output/ConsoleFilter.java index be1c6bec7..975cc4cc3 100644 --- a/src/main/java/fr/xephi/authme/output/ConsoleFilter.java +++ b/src/main/java/fr/xephi/authme/output/ConsoleFilter.java @@ -4,7 +4,7 @@ import java.util.logging.Filter; import java.util.logging.LogRecord; /** - * Console filter Class + * Console filter to replace sensitive AuthMe commands with a generic message. * * @author Xephi59 */ @@ -12,20 +12,15 @@ public class ConsoleFilter implements Filter { @Override public boolean isLoggable(LogRecord record) { - try { - if (record == null || record.getMessage() == null) - return true; - String logM = record.getMessage().toLowerCase(); - if (!logM.contains("issued server command:")) - return true; - 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 true; - String playerName = record.getMessage().split(" ")[0]; - record.setMessage(playerName + " issued an AuthMe command!"); - return true; - } catch (NullPointerException npe) { + if (record == null || record.getMessage() == null) { return true; } + + if (LogFilterHelper.isSensitiveAuthMeCommand(record.getMessage())) { + String playerName = record.getMessage().split(" ")[0]; + record.setMessage(playerName + " issued an AuthMe command"); + } + return true; } } diff --git a/src/main/java/fr/xephi/authme/output/Log4JFilter.java b/src/main/java/fr/xephi/authme/output/Log4JFilter.java index aa43beed1..89d77ebe9 100644 --- a/src/main/java/fr/xephi/authme/output/Log4JFilter.java +++ b/src/main/java/fr/xephi/authme/output/Log4JFilter.java @@ -1,8 +1,8 @@ package fr.xephi.authme.output; -import fr.xephi.authme.util.StringUtils; import org.apache.logging.log4j.Level; import org.apache.logging.log4j.Marker; +import org.apache.logging.log4j.core.Filter; import org.apache.logging.log4j.core.LogEvent; import org.apache.logging.log4j.core.Logger; import org.apache.logging.log4j.message.Message; @@ -12,14 +12,7 @@ import org.apache.logging.log4j.message.Message; * * @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 "}; +public class Log4JFilter implements Filter { /** * Constructor. @@ -28,13 +21,12 @@ public class Log4JFilter implements org.apache.logging.log4j.core.Filter { } /** - * Validates a Message instance and returns the {@link Result} value - * depending depending on whether the message contains sensitive AuthMe - * data. + * Validate a Message instance and return the {@link Result} value + * depending on whether the message contains sensitive AuthMe data. * - * @param message the Message object to verify + * @param message The Message object to verify * - * @return the Result value + * @return The Result value */ private static Result validateMessage(Message message) { if (message == null) { @@ -44,24 +36,17 @@ public class Log4JFilter implements org.apache.logging.log4j.core.Filter { } /** - * Validates a message and returns the {@link Result} value depending - * depending on whether the message contains sensitive AuthMe data. + * Validate a message and return the {@link Result} value depending + * on whether the message contains sensitive AuthMe data. * - * @param message the message to verify + * @param message The message to verify * - * @return the Result value + * @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; + return LogFilterHelper.isSensitiveAuthMeCommand(message) + ? Result.DENY + : Result.NEUTRAL; } @Override @@ -73,14 +58,12 @@ public class Log4JFilter implements org.apache.logging.log4j.core.Filter { } @Override - public Result filter(Logger arg0, Level arg1, Marker arg2, String message, - Object... arg4) { + public Result filter(Logger arg0, Level arg1, Marker arg2, String message, Object... arg4) { return validateMessage(message); } @Override - public Result filter(Logger arg0, Level arg1, Marker arg2, Object message, - Throwable arg4) { + public Result filter(Logger arg0, Level arg1, Marker arg2, Object message, Throwable arg4) { if (message == null) { return Result.NEUTRAL; } @@ -88,8 +71,7 @@ public class Log4JFilter implements org.apache.logging.log4j.core.Filter { } @Override - public Result filter(Logger arg0, Level arg1, Marker arg2, Message message, - Throwable arg4) { + public Result filter(Logger arg0, Level arg1, Marker arg2, Message message, Throwable arg4) { return validateMessage(message); } diff --git a/src/main/java/fr/xephi/authme/output/LogFilterHelper.java b/src/main/java/fr/xephi/authme/output/LogFilterHelper.java new file mode 100644 index 000000000..605283ac2 --- /dev/null +++ b/src/main/java/fr/xephi/authme/output/LogFilterHelper.java @@ -0,0 +1,34 @@ +package fr.xephi.authme.output; + +import fr.xephi.authme.util.StringUtils; + +/** + * Service class for the log filters. + */ +public final class LogFilterHelper { + + private static final String ISSUED_COMMAND_TEXT = "issued server command:"; + + private static final String[] COMMANDS_TO_SKIP = {"/login ", "/l ", "/reg ", "/changepassword ", + "/unregister ", "/authme register ", "/authme changepassword ", "/authme reg ", "/authme cp ", + "/register "}; + + private LogFilterHelper() { + // Util class + } + + /** + * Validate a message and return whether the message contains a sensitive AuthMe command. + * + * @param message The message to verify + * + * @return True if it is a sensitive AuthMe command, false otherwise + */ + public static boolean isSensitiveAuthMeCommand(String message) { + if (message == null) { + return false; + } + String lowerMessage = message.toLowerCase(); + return lowerMessage.contains(ISSUED_COMMAND_TEXT) && StringUtils.containsAny(lowerMessage, COMMANDS_TO_SKIP); + } +} diff --git a/src/test/java/fr/xephi/authme/output/ConsoleFilterTest.java b/src/test/java/fr/xephi/authme/output/ConsoleFilterTest.java new file mode 100644 index 000000000..8975068f6 --- /dev/null +++ b/src/test/java/fr/xephi/authme/output/ConsoleFilterTest.java @@ -0,0 +1,77 @@ +package fr.xephi.authme.output; + +import org.junit.Test; +import org.mockito.Mockito; + +import java.util.logging.LogRecord; + +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Test for {@link ConsoleFilter}. + */ +public class ConsoleFilterTest { + + private final ConsoleFilter filter = new ConsoleFilter(); + + private static final String SENSITIVE_COMMAND = "User issued server command: /login test test"; + private static final String NORMAL_COMMAND = "User issued server command: /motd 2"; + + + @Test + public void shouldReplaceSensitiveRecord() { + // given + LogRecord record = createRecord(SENSITIVE_COMMAND); + + // when + boolean result = filter.isLoggable(record); + + // then + assertThat(result, equalTo(true)); + verify(record).setMessage("User issued an AuthMe command"); + } + + @Test + public void shouldNotFilterRegularCommand() { + // given + LogRecord record = createRecord(NORMAL_COMMAND); + + // when + boolean result = filter.isLoggable(record); + + // then + assertThat(result, equalTo(true)); + verify(record, never()).setMessage("User issued an AuthMe command"); + } + + @Test + public void shouldManageRecordWithNullMessage() { + // given + LogRecord record = createRecord(null); + + // when + boolean result = filter.isLoggable(record); + + // then + assertThat(result, equalTo(true)); + verify(record, never()).setMessage("User issued an AuthMe command"); + } + + + /** + * Creates a mock of {@link LogRecord} and sets it to return the given message. + * + * @param message The message to set. + * + * @return Mock of LogRecord + */ + private static LogRecord createRecord(String message) { + LogRecord record = Mockito.mock(LogRecord.class); + when(record.getMessage()).thenReturn(message); + return record; + } +} diff --git a/src/test/java/fr/xephi/authme/permission/AdminPermissionTest.java b/src/test/java/fr/xephi/authme/permission/AdminPermissionTest.java index c3a89b723..9a8625ac0 100644 --- a/src/test/java/fr/xephi/authme/permission/AdminPermissionTest.java +++ b/src/test/java/fr/xephi/authme/permission/AdminPermissionTest.java @@ -20,7 +20,8 @@ public class AdminPermissionTest { // when/then for (AdminPermission permission : AdminPermission.values()) { if (!permission.getNode().startsWith(requiredPrefix)) { - fail("The permission '" + permission + "' does not start with the required prefix '" + requiredPrefix + "'"); + fail("The permission '" + permission + "' does not start with the required prefix '" + + requiredPrefix + "'"); } } } @@ -33,7 +34,8 @@ public class AdminPermissionTest { // when/then for (AdminPermission permission : AdminPermission.values()) { if (!permission.getNode().contains(requiredBranch)) { - fail("The permission '" + permission + "' does not contain with the required branch '" + requiredBranch + "'"); + fail("The permission '" + permission + "' does not contain with the required branch '" + + requiredBranch + "'"); } } }