https://issues.apache.org/bugzilla/show_bug.cgi?id=55101
Bug ID: 55101
Summary: BasicAuthenticator parser and associated unit tests
Product: Tomcat 8
Version: trunk
Hardware: PC
OS: Linux
Status: NEW
Severity: enhancement
Priority: P2
Component: Catalina
Assignee: dev@tomcat.apache.org
Reporter: br...@pingtoo.com
Created attachment 30435
--> https://issues.apache.org/bugzilla/attachment.cgi?id=30435&action=edit
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
