kuzjka commented on code in PR #504: URL: https://github.com/apache/santuario-xml-security-java/pull/504#discussion_r2505277416
########## src/main/java/org/apache/xml/security/utils/XMLUtils.java: ########## Review Comment: The fact that `XMLUtils` is a utility class with static methods, and the configuration can only be done with system properties at the class load time makes it hard to test. That's why the test contains custom class loader which reloads the class in each test, allowing to run static fields initialization in the middle of the test. To build a sort of integration test against public API e.g. `XMLSignature`, I believe we will need to load `XMLSignature` and all the classes it depends on with this custom classloader. There are additional considerations regarding Jigsaw module system: since the new classloader loads classes in its own module, all packages from `xmlsec` which are not exported cannot be used by this classloader. I can try to implement such a test, but I'm afraid it will be totally unreadable... Intuitively, a good test interacts with the code in a way it is intended to be used. In our case, parts of the public interface are system properties. It looks proper to run the test suite with different JVMs started with different properties. Probably, Maven has something to achieve this, I'll investigate. On the other hand, we can make it more testable (and probably a little more convenient for usage) if we provide a way to set Base64 options in runtime, for example: * refer to system properties and resolve the options each time we do base64 encoding (e.g. inside `encodeToString` etc.) * add public setters for `base64FormattingOptions` and `ignoreLineBreaks` * add public setter for `base64Encoder` - then we don't need new options at all. We can default to `Base64.getEncoder()` / `Base64.getMimeEncoder()` depending on `ignoreLineBreaks`, and if the clients need custom formatting - they just provide any implementation of `Base64.Encoder` However, I'm not aware of security implications behind this... All other inline comments were addressed in 8a598d0. Thank you for checking the code carefully. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
