You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bu...@apache.org on 2013/06/15 17:01:40 UTC
[Bug 55101] New: BasicAuthenticator parser and associated unit tests
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: Brian@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55101] BasicAuthenticator parser and associated unit tests
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55101
--- Comment #5 from Mark Thomas <ma...@apache.org> ---
Thanks. Applied to trunk.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55101] BasicAuthenticator parser and associated unit tests
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55101
--- Comment #1 from Brian Burch <Br...@PingToo.com> ---
Created attachment 30436
--> https://issues.apache.org/bugzilla/attachment.cgi?id=30436&action=edit
remove TODOs with more tolerant parser
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55101] BasicAuthenticator parser and associated unit tests
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55101
Mark Thomas <ma...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--- Comment #3 from Mark Thomas <ma...@apache.org> ---
Thanks for the patches. They have been applied to trunk and will be included in
Tomcat 8.0.0 onwards.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55101] BasicAuthenticator parser and associated unit tests
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55101
--- Comment #6 from Jackie Rosen <ja...@hushmail.com> ---
*** Bug 260998 has been marked as a duplicate of this bug. ***
Seen from the domain http://volichat.com
Page where seen: http://volichat.com/adult-chat-rooms
Marked for reference. Resolved as fixed @bugzilla.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55101] BasicAuthenticator parser and associated unit tests
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55101
--- Comment #2 from Brian Burch <Br...@PingToo.com> ---
Created attachment 30437
--> https://issues.apache.org/bugzilla/attachment.cgi?id=30437&action=edit
new test class for Basic authorization parser and Base64 decoder
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 55101] BasicAuthenticator parser and associated unit tests
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=55101
--- Comment #4 from Brian Burch <Br...@PingToo.com> ---
Created attachment 30636
--> https://issues.apache.org/bugzilla/attachment.cgi?id=30636&action=edit
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org