Bug 55101

Created attachment 30435 [details]
new inner class to parse credentials

Now that tomcat uses a single Base64 decoder, I returned to the issues discussed in https://issues.apache.org/bugzilla/show_bug.cgi?id=54729.

Mark's change to the primitive Basic parser in svn commit: r1459289 was my starting point, but I wanted to write a comprehensive set of unit tests that examined all the relevant edge cases under the new Base64 decoder.

I felt the most sympathetic approach would be to mirror the way DigestAuthenticator uses an inner class which encapsulates the parsing logic and provides trivial accessor methods for the authentication tokens. A suitable signature for the inner class would allow me to instantiate and test it exhaustively without having to haul up a tomcat instance for each test case. I accept the previous advice that the best framework for testing authentication is by running tomcat and examining the output to the client, but this overhead is not necessary in every single edge case.

As I developed the code, I quickly realised there would be no benefit in splitting the parser code (as DigestAuthenticator does), where some of it would have been moved to a new HttpParser.parseAuthorizationDigest method. The logic for parsing a Basic header seemed to be more clear when it was performed entirely by the new inner class. In particular, following Konstantin's comments, it would be unecessarily complex and slow to convert the ByteChunk to a StringReader for the required simple parsing process, and then returning the tokens as a HashMap. Having decided there was no point in slavishly following the parseAuthorizationDigest model, I have closed bz54729.

At this stage, I only made two small changes to TestNonLoginAndBasicAuthenticator. Some of the tests will eventually be removed as redundant, but there is no harm keeping them initially to demonstrate backward compatibility. Two of the test cases in this suite now "fail" because they anticipate rejection from the old parser, while the new parser is more tolerant - exactly as described in the existing TODOs (which are now removed).

My new test suite org.apache.catalina.authenticator.TestBasicAuthParser has three checkstyle errors that I am not sure how to resolve. I followed the "normal" junit style to have static imports for my two hamcrest assertions, but they are flagged with errors, e.g. "Using a static member import should be avoided - org.hamcrest.MatcherAssert.assertThat". Now that hamcrest core is no longer shipped within junit, do we need another checkstle rule?

I hope you find these changes acceptable.