Thanks for all the help while I was developing the new test case for
https://issues.apache.org/bugzilla/show_bug.cgi?id=53584. The thread on the
users mailing list is called "AuthenticatorBase
setChangeSessionIdOnAuthentication without cookies".
I now have two new test cases working successfully against a
recently-updated trunk. I hope to use them in future as boilerplates, to
expand the set of tests, and also to examine SSO behaviour.
Before I open a bugzilla enhancement request and submit my patch files, I
would like to discuss my implementation decisions in general, to ensure that
my effort, and other developer's time, isn't wasted.
I found it necessary to modify both
org.apache.catalina.authenticator.FormAuthenticatorTest, and its parent
class org.apache.catalina.startup.SimpleHttpClient.
To save you looking it up, here is the unchanged class comment to
SimpleHttpClient:
/**
* Simple client for unit testing. It isn't robust, it isn't secure and
* should not be used as the basis for production code. Its only purpose
* is to do the bare minimum for the unit tests.
*/
FormAuthenticatorTest doesn't have a class-level comment at all, but I have
written a new one based on the conclusions in my thread on the users list.
I would have preferred to make fairly localised changes to both of these
classes, but the existing logic seems to incorporate some fundamental
assumptions that made my intention too difficult to implement.
I am not at all proud of my code, but I believe it a) works, b) is fairly
harmonious with the existing design, and c) is amenable to the extensions I
intend to add in due course.
Firstly, I've written several small blocks of parser logic for urls and http
headers, which are specific to the test environment and not very pretty. I
looked for suitable parsers to re-use in the various tomcat utils packages,
but haven't found them yet, even though tomcat MUST be doing similar work in
an elegant and robust manner. I haven't found any unit tests, either.
Then, I looked at the apache HttpClient project. An ideal solution would
have been to use the parsers from that project, or perhaps even the entire
client. This would have required starting with a blank sheet of paper, and I
am very reluctant to take that approach. I might have been swayed if I had
found HttpClient already in use by other tomcat unit tests, but I couldn't
find it in the dependencies and didn't want to add a new one.
Next, the current version only supports cookies, so I had to add some extra
boolean arguments to control the use of server and client cookies in each
test case. In my professional work, I would have use singleton inner classes
to achieve type-safety and make these crucial arguments self-documenting,
but this wouldn't be compatible with the existing style of the various
current authenticator unit test classes. Also, I wanted to make my initial
change as transparent as possible, so it could be reviewed (and accepted)
with as little effort from others as possible.
I didn't want to touch SimpleHttpClient at all, but that turned out to be
unavoidable. This class does most of the http header processing, and so it
seemed ugly to split this work between the two classes. On the other hand,
all the url handling is done by FormAuthenticatorTest, so it felt ugly to
start doing any of that work in SimpleHttpClient. The consequence is that
the two classes need to be cross-wired to some extent. This was always the
case, but I had to cross some more wires for the new test cases.
So, to summarise: I would like to make quite a bulky change to these two
classes. I am somewhat embarrassed by my ugly style and DIY approach to
parsing. Many of the line-changes in the patch are trivial, but a lot of
lines will be hit at once. I haven't gone mad with comments, but have tried
to add useful tips when an existing section of uncommented code didn't make
sense to me. On the other hand, to maintain similar look'n'feel, I haven't
been heavy-handed with comments in my new code. Of course, I will make sure
it passes checkstyle before I actually cut the patches!
To put things in perspective, the tests only demonstrate that Mark's fix
works - and that wasn't even in question. However, I'd like to get my change
committed soon, so that I can move forward with additional test cases.
What do you think? Should I publish and be damned, or would you like me to
do more work to eliminate some of my compromises?
