-
Notifications
You must be signed in to change notification settings - Fork 365
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
Update the logging properties to opt-out of the prefix events #844 #845
Changes from 8 commits
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 |
---|---|---|
|
@@ -76,6 +76,18 @@ public Boolean getBooleanProp(String propertyName) throws ConfigurationException | |
throw new ConfigurationException("Could not find property " + propertyName + " in configuration"); | ||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
@Override | ||
public Boolean getBooleanProp(String propertyName, Boolean defaultValue) { | ||
for (AbstractPrioritizedPropertyLoader loader : loaders) { | ||
return loader.getBooleanProp(propertyName, defaultValue); | ||
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 think the way this is currently implemented, I don't think it will ever reach the final return on line 87 here. Maybe I'm wrong about that. Do we have a test for that case? Of course, if we change it as per my previous comment, we may still need it. Or may need to add a try / catch here and just re-throw any |
||
} | ||
return defaultValue; | ||
} | ||
|
||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,6 +70,24 @@ public Boolean getBooleanProp(String propertyName) throws ConfigurationException | |
} | ||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
@Override | ||
public Boolean getBooleanProp(String propertyName, Boolean defaultValue) { | ||
String property = properties.getProperty(propertyName); | ||
if (property == null) { | ||
return defaultValue; | ||
} | ||
if (property.equalsIgnoreCase("true") || property.equalsIgnoreCase("yes")) { | ||
return true; | ||
} | ||
if (property.equalsIgnoreCase("false") || property.equalsIgnoreCase("no")) { | ||
return false; | ||
} | ||
return defaultValue; | ||
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. Depending on if we decide to change the behavior here in accordance to my first comment, we may wish to change line 88 to something along the following lines: if ( property != null ) {
throw new ConfigurationException("Property name '" + propertyName + "' has non-Boolean value of '" + property + "'.");
} |
||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,6 +86,24 @@ public Boolean getBooleanProp(String propertyName) throws ConfigurationException | |
} | ||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
@Override | ||
public Boolean getBooleanProp(String propertyName, Boolean defaultValue) { | ||
String property = properties.getProperty(propertyName); | ||
if (property == null) { | ||
return defaultValue; | ||
} | ||
if (property.equalsIgnoreCase("true") || property.equalsIgnoreCase("yes")) { | ||
return true; | ||
} | ||
if (property.equalsIgnoreCase("false") || property.equalsIgnoreCase("no")) { | ||
return false; | ||
} | ||
return defaultValue; | ||
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. Ditto here. Similar to comment on StandardEsapiPropertyLoader.java. |
||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1441,21 +1441,47 @@ public Boolean getBooleanProp(String propertyName) throws ConfigurationException | |
try { | ||
return esapiPropertyManager.getBooleanProp(propertyName); | ||
} catch (ConfigurationException ex) { | ||
String property = properties.getProperty( propertyName ); | ||
String property = properties.getProperty(propertyName); | ||
if ( property == null ) { | ||
throw new ConfigurationException( "SecurityConfiguration for " + propertyName + " not found in ESAPI.properties"); | ||
} | ||
if ( property.equalsIgnoreCase("true") || property.equalsIgnoreCase("yes" ) ) { | ||
if ( property.equalsIgnoreCase("true") || property.equalsIgnoreCase("yes") ) { | ||
return true; | ||
} | ||
if ( property.equalsIgnoreCase("false") || property.equalsIgnoreCase( "no" ) ) { | ||
if ( property.equalsIgnoreCase("false") || property.equalsIgnoreCase("no") ) { | ||
return false; | ||
} | ||
throw new ConfigurationException( "SecurityConfiguration for " + propertyName + " has incorrect " + | ||
"type"); | ||
} | ||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
* Looks for property in three configuration files in following order: | ||
* 1.) In file defined as org.owasp.esapi.opsteam system property | ||
* 2.) In file defined as org.owasp.esapi.devteam system property | ||
* 3.) In ESAPI.properties | ||
*/ | ||
@Override | ||
public Boolean getBooleanProp(String propertyName, Boolean defaultValue) { | ||
try { | ||
return esapiPropertyManager.getBooleanProp(propertyName); | ||
} catch (ConfigurationException ex) { | ||
String property = properties.getProperty(propertyName); | ||
if ( property == null ) { | ||
return defaultValue; | ||
} | ||
if ( property.equalsIgnoreCase("true") || property.equalsIgnoreCase("yes") ) { | ||
return true; | ||
} | ||
if ( property.equalsIgnoreCase("false") || property.equalsIgnoreCase("no") ) { | ||
return false; | ||
} | ||
return defaultValue; | ||
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. Ditto comment in EsapiPropertyManager.java about throwing |
||
} | ||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
* Looks for property in three configuration files in following order: | ||
|
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.
From the Javadoc: for this interface: "or has incorrect type".
Well, that is the $64k question, isn't it? What is the INTUITIVE behavior if one calls something like:
but somewhere, say in ESAPI.properties or elsewhere, we have defined
Then what is the expected / intuitive behavior for that call? I think that I'd expect a
ConfigurationException
to be thrown rather than havingpropValue
silently being set totrue
. Or maybe I shouldn't say that is the intuitive behavior, but rather the safe behavior. I think because copy/paste errors abound and it would be easy to make a typo with the property name, that's the only way that we're going to help people catch these bizarre accidental edge cases. Otherwise, we're going to cause them to curse at us because they'll have to figure up a debugger to figure out what the hell is going on in cases like this.@xeno6696 and @jeremiahjstacey - I'd like your opinion here. I know this requires yet another code change, but if we do a release and document it this way this (likely bug, IMO) will become a feature because changing it later on would break backward compatibility. If we were requiring JDK 9 or later we could just not export this method and keep it hidden, but since Java 8 is the minimal version, once we add this, it becomes available to the world.
I personally am leaning towards only using the default value if the property is not set at all; that is if
returns
null
, but I'd like to know what the 2 of you think. If you both think it's more intuitive have it return the default boolean value if the property name is found to have a non-Boolean value I will reluctantly go along with that.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.
Overall, I think this is more scope than we should put time into to modify the opt-out request from the original issue. I appreciate the initiative; however, we're adding this case to expand for something that could have been solved in two copies of an 6 line block.
This is overengineering in this scope. We're blocking value, and a new possible release for this API change.
My suggestion is to take it out. Use the duplicate snippet from my previous comment in the 2 factories and revert the changes in the DefaultSecurityConfiguration. Create another task for the default API that we can address (reuse this code if you want), and complete it in a non-blocking scope. We don't need this to fix the original problem, and as you can see the discussion of edge cases and caveats will continue to grow. Let's get the opt-out behavior in in a first form, then improve upon it in another task to expand the SecurityConfiguration API to allow defaults to be specified.
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 concur. If in the future, you want to do a new PR to get a new interface for
I am fine with that, but as my review of this as surfaced, this is a little more nuanced than intuition alone might have us believe. If we are going to add that a future time, I think we need to start out with a GitHub issue and discuss the specifications there first, before waiting for an implementation to discuss them. So, to proceed with this PR, please rip that part of the code out and follow Jeremiah's suggestion.