Add toggle for policy validation #65
Conversation
New static method on Policy to enable/disable schema validation. It's enabled by default.
|
Works for me and ESAPI. |
|
In my opinion, after this polishing to XSD validation, the only thing we could add to release is a cherry-pick from 1.5.13 and minor changes to remove some SpotBug warnings. So future changes can be applied over 1.6.x and we don’t need to release more intermediate 1.5.x branches. |
This implementation ntends to follow this steps: 1) Parsing with schema validation on and, if successful, check is validation is disabled. If it is disabled then issue a warning that it should not be. 2) If validation fails, remember the exception and then try to instantiate the parser again with validation off. If that also fails, throw that exception. If it succeeds, extract the error message from the saved exception and issue that as a warning. That procedure is just to try to help users with the policy change. Eventually, the changes on this commit should be reverted to always validate.
|
@davewichers, the changes on last commit seem to work. My mind broke refactoring to avoid copying code and I'm not sure this is the best approach/solution as Java is not my strength but looks and works ok. Aside, the |
|
Clearly the first thing to do would be to write a few test case for an"" tag. But since it is not in the presenting in the XSD, that's another good reason for providing a mechanism to disable the XML validation check. That is more likely to affect direct users of AntiSamy more than it is users of AntiSamy via ESAPI. I doubt ESAPI users were even aware you could include other policy files. I know I wasn't aware. |
| @@ -120,6 +120,8 @@ | |||
| * XML Schema for policy validation | |||
| */ | |||
| private static Schema schema = null; | |||
| private static boolean validateSchema = true; | |||
| private static SAXException savedSchemaValidationException; | |||
There was a problem hiding this comment.
Seems strange to me to have this be a class variable. Can't it be passed as a parameter to the methods that need it from the caller?
There was a problem hiding this comment.
It would require an API change. All Policy.getPolicy() calls would need a new Boolean parameter to pass down and the method to enable/disable validation would be removed as it won’t be needed anymore.
There was a problem hiding this comment.
Sorry. I was referring to savedSchemaValidationException, not the boolean, which I think is fine. I'm testing/making changes and I'll address my concern if I easily can. Hold on for now.
… work, and check it in. There are a number of ToDo's left for test case creation and JavaDocs needed for new methods.
|
I pulled in this pull request manually, tested it, modified it, and then pushed the changes directly to the 1.6.0 branch, making this pull request obsolete. So I'm closing it. There are still a number of ToDos still left to be done. |
Just for the record, this is related to issue #58.