-
Notifications
You must be signed in to change notification settings - Fork 370
Update the logging properties to opt-out of the prefix events #844 #845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
8b68f75
d3f2a2f
6d8f307
7822d6f
2e2d0e5
9ca8473
c9d5b78
54a7463
17219b7
0e9bb76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -407,6 +407,10 @@ Logger.UserInfo=true | |
# Determines whether ESAPI should log the session id and client IP. | ||
Logger.ClientInfo=true | ||
|
||
# Determines whether ESAPI should log the prefix of [EVENT_TYPE - APPLICATION NAME]. | ||
# If all above Logger entries are set to false and LogIgnorePrefix is true, then the output would be the same like if no ESAPI was used | ||
Logger.LogIgnorePrefix=true | ||
jeremiahjstacey marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @jeremiahjstacey about the non-intuitive property name. But also we can't have this new property be set in a manner that is going to change the default log output as it has previously existed since ESAPI 2.0. As project co-lead, I'm flat out not going to allow that. So whatever the default is set to, the logs have to be formatted as they were before. The only times we break backward compatibility is to:
We've already learned that people do not read the release notes. Instead, they post GitHub issues, send the team private emails, post on Stack Overflow, etc. and we are left trying to provide answers. So, no, the default logging behavior is not going to change on my watch. If people like yourself want it changed, you will have to tweak the value in your application's private copy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, VERY important note: We can't assume that people will update their ESAPI.properties file. They in fact often don't until something goes wrong. So be sure to test it to make sure that it behaves as it does in 2.5.4.0 and earlier even if the completely omit that new property file. We can't be changing behavior and more importantly, we don't want to throw a ConfigurationException if that property is missing. |
||
|
||
#=========================================================================== | ||
# ESAPI Intrusion Detection | ||
# | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -111,6 +111,7 @@ public final class PropNames { | |
public static final String LOG_ENCODING_REQUIRED = "Logger.LogEncodingRequired"; | ||
public static final String LOG_APPLICATION_NAME = "Logger.LogApplicationName"; | ||
public static final String LOG_SERVER_IP = "Logger.LogServerIP"; | ||
public static final String LOG_IGNORE_PREFIX = "Logger.LogIgnorePrefix"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the property name changes as per @jeremiahjstacey's suggestion, then please rename this variable in a corresponding fashion as well. |
||
|
||
public static final String VALIDATION_PROPERTIES = "Validator.ConfigurationFile"; | ||
public static final String VALIDATION_PROPERTIES_MULTIVALUED = "Validator.ConfigurationFile.MultiValued"; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,18 +30,27 @@ public class EventTypeLogSupplier // implements Supplier<String> | |
{ | ||
/** EventType reference to supply log representation of. */ | ||
private final EventType eventType; | ||
/** Whether to log or not the event type */ | ||
private boolean ignoreLogEventType = false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you take @jeremiahjstacey's suggestion, maybe instead it makes more sense to call this |
||
|
||
/** | ||
* Ctr | ||
* | ||
* @param evtyp EventType reference to supply log representation for | ||
* @param eventType EventType reference to supply log representation for | ||
kwwall marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
public EventTypeLogSupplier(EventType evtyp) { | ||
this.eventType = evtyp == null ? Logger.EVENT_UNSPECIFIED : evtyp; | ||
public EventTypeLogSupplier(EventType eventType) { | ||
this.eventType = eventType == null ? Logger.EVENT_UNSPECIFIED : eventType; | ||
} | ||
|
||
// @Override -- Uncomment when we switch to Java 8 as minimal baseline. | ||
public String get() { | ||
if (this.ignoreLogEventType) { | ||
return ""; | ||
} | ||
return eventType.toString(); | ||
} | ||
jeremiahjstacey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public void setIgnoreLogEventType(boolean ignoreLogEventType) { | ||
this.ignoreLogEventType = ignoreLogEventType; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
package org.owasp.esapi.logging.appender; | ||
|
||
import static org.junit.Assert.assertEquals; | ||
|
||
import org.junit.Test; | ||
import org.junit.runner.RunWith; | ||
import org.junit.runners.Parameterized; | ||
import org.owasp.esapi.Logger; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Collection; | ||
import java.util.List; | ||
|
||
@RunWith(Parameterized.class) | ||
public class EventTypeLogSupplierIgnoreEventTypeTest { | ||
|
||
@Parameterized.Parameters (name="{0} -> {1}") | ||
public static Collection<Object[]> assembleTests() { | ||
List<Object[]> paramSets = new ArrayList<>(); | ||
paramSets.add(new Object[] {Logger.EVENT_FAILURE,""}); | ||
paramSets.add(new Object[] {Logger.EVENT_SUCCESS,""}); | ||
paramSets.add(new Object[] {Logger.EVENT_UNSPECIFIED,""}); | ||
paramSets.add(new Object[] {Logger.SECURITY_AUDIT,""}); | ||
paramSets.add(new Object[] {Logger.SECURITY_FAILURE,""}); | ||
paramSets.add(new Object[] {Logger.SECURITY_SUCCESS,""}); | ||
paramSets.add(new Object[] {null, ""}); | ||
|
||
return paramSets; | ||
} | ||
|
||
private final Logger.EventType eventType; | ||
private final String expectedResult; | ||
|
||
public EventTypeLogSupplierIgnoreEventTypeTest(Logger.EventType eventType, String result) { | ||
this.eventType = eventType; | ||
this.expectedResult = result; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Add blank line here for improved readability. |
||
@Test | ||
public void testEventTypeLogIgnoreEventType() { | ||
EventTypeLogSupplier supplier = new EventTypeLogSupplier(eventType); | ||
supplier.setIgnoreLogEventType(true); | ||
assertEquals(expectedResult, supplier.get()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm missing something here. Are you saying that for this new property to have the effect of omitting the event type, that ALL of these other
Logger.*
property names from lines 400 through 408 inclusive MUST be false? That seems overly complicated to me.