|Summary:||BasicAuthenticator parser and associated unit tests|
|Product:||Tomcat 8||Reporter:||Brian Burch <Brian>|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
new inner class to parse credentials
remove TODOs with more tolerant parser
new test class for Basic authorization parser and Base64 decoder
remove redundant test cases for unusual but valid credentials
Description Brian Burch 2013-06-15 15:01:40 UTC
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.
Comment 1 Brian Burch 2013-06-15 15:05:07 UTC
Created attachment 30436 [details] remove TODOs with more tolerant parser
Comment 2 Brian Burch 2013-06-15 15:06:08 UTC
Created attachment 30437 [details] new test class for Basic authorization parser and Base64 decoder
Comment 3 Mark Thomas 2013-06-20 20:36:38 UTC
Thanks for the patches. They have been applied to trunk and will be included in Tomcat 8.0.0 onwards.
Comment 4 Brian Burch 2013-07-28 13:32:49 UTC
Created attachment 30636 [details] remove redundant test cases for unusual but valid credentials r1495169 introduced a new BasicAuthenticator inner class to encapsulate parsing and decoding of Base64 credentials. It also added TestBasicAuthParser, which examines a far wider set of edge cases. This new change removes three tests that have now become complete duplicates of cases in TestBasicAuthParser. Also, the class-level comment has been expanded to explain the relationship between the two test classes.
Comment 5 Mark Thomas 2013-07-29 16:38:21 UTC
Thanks. Applied to trunk.