Created attachment 30083 [details] svn diff: update to parser and new test class While working on https://issues.apache.org/bugzilla/show_bug.cgi?id=54190, I left several TODO's where the current BasicAuthenticator did not handle some test cases properly. Mark suggested I use the HttpParser.parseAuthorizationDigest method as a model for re-implementing Basic authentication. I am submitting a preliminary change which will permit moving the parsing logic out of BasicAuthenticator and into HttpParser. I have also written a new test class to exercise all relevant test cases in the new parser. I have implemented these changes in a manner that is 100% backward compatible, because I would like to get them committed before I submit my dependent changes to BasicAuthenticator and its own test class. When reviewing the attached patches, please take the following into consideration: 1. Although I started my implementation by trying to write efficient code, I eventually rejected that approach. The change I am now submitting sacrifices efficiency in favour of clarity. Its logic is analogous to that of the parseAuthorizationDigest method, even though the processing requirents are somewhat different. This has the advantage that the two Authenticator.authenticate methods will eventually have quite similar logic. 2. My new tests expose several deficiencies and bugs in org.apache.catalina.util.Base64. I might be mistaken, but I haven't been able to find a test class for Base64. Rather than fix the bugs and possibly trigger side effects in other users of the class, I have decided to implement some temporary defensive logic in the new parser. This is a bit ugly, but is not intended to be present for very long. 3. I would like to tackle the Base64 class later, by writing a full set of unit tests. I anticipate refactoring the two decode methods to eliminate duplicate logic, as well as providing a more efficient signature for use by parseAuthorizationBasic. I will refer to the latest commons Base64 class and its test cases. I will fix the current bugs and then be in a position to remove the defensive code in the parser. 4. I noticed Mark's recent updates, eg. commit: r1458187 to tomcat/trunk/java/org/apache/tomcat/util/http/fileupload/util/mime/Base64Decoder.java, and Konstantin's comments. I wonder how many different Base64 decoders are used by tomcat? http://commons.apache.org/proper/commons-codec/ Impetus section states there were once 34 different Base64 implementations in the apache repository. Perhaps this is a good time to look at consolidation, or even return to my earlier suggestion to define the commons jar as a dependency of tomcat?
Regarding your points: 1. Clarity vs efficiency is always a trade off. It depends how much efficiency has been lost. Do you have any performance numbers? 2. I don't see a test case either. I'd rather get any bugs in the decoder fixed than put sticking plasters over other bits of code. 3. I'd prefer to do this first. That code is only still in the code base because it support ByteChunk / CharChunk which should allow a more efficient interface with the rest of the Tomcat code base. It is probably time that that assumption is tested. 4. It is very unlikely we will be adding Commons Codec as a dependency to Tomcat. We may do an svn copy and rename much like we have done for FileUpload but the usefulness of that depends on the ByteChunk/CharChunk issues in 3. For FileUpload I am looking at replacing the decoder with the JVM implementation.
I've done some additional testing. On a warmed up system the JRE and Tomcat Base64 decoding implementations are comparable. Tomcat does warm up faster. Without warm up on 1000000 iterations (different inputs) Tomcat was approx. 3x faster but at a total of 200ms vs 600ms for 1000000 tests I'm not too worried by even that difference. I intend to proceed and remove Tomcat's internal base64 encoder in 8 and deprecate it in 7.0.x, In both cases I intend to replace the current uses of Tomcat's internal decoder with the JRE's.
You may want to have a look at the the cases at: http://greenbytes.de/tech/tc/httpauth/
This change is no longer appropriate. A better solution has been proposed. Please see https://issues.apache.org/bugzilla/show_bug.cgi?id=55101