tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brian Burch <br...@pingtoo.com>
Subject FormAuthenticatorTest for cases without cookies - implementation issues
Date Wed, 26 Sep 2012 14:01:14 GMT
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?

Thanks for reading this...

Brian




---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message