Bug 54190 - TestNonLoginAndBasicAuthenticator does not test session timeout properly
TestNonLoginAndBasicAuthenticator does not test session timeout properly
Status: RESOLVED FIXED
Product: Tomcat 7
Classification: Unclassified
Component: Catalina
trunk
PC Linux
: P4 minor (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2012-11-22 13:10 UTC by Brian Burch
Modified: 2012-12-03 12:11 UTC (History)
0 users



Attachments
Extensive update to the test class to demonstrate session timeout properly (19.13 KB, patch)
2012-11-22 13:10 UTC, Brian Burch
Details | Diff
New version of complete patch (26.24 KB, patch)
2012-11-26 18:28 UTC, Brian Burch
Details | Diff
Complete replacement for previous patches (26.02 KB, patch)
2012-11-26 20:07 UTC, Brian Burch
Details | Diff
Faster detection of expired session in timeout test case (7.91 KB, patch)
2012-12-03 11:25 UTC, Brian Burch
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burch 2012-11-22 13:10:23 UTC
Created attachment 29621 [details]
Extensive update to the test class to demonstrate session timeout properly

While working on a new test case for a different Authenticator, I decided to follow the timeout test case in this class. Although all the test cases currently run successfully, I discovered three fundamental flaws in the existing timeout test case:

1. The BasicAuthenticator does not create a session by default, so there was no session to actually timeout.
2. Context.setSessionTimeout() was called with a timeout in seconds, but this method expects a timeout argument in minutes.
3. The presence of 401 Unauthorized status was intended to confirm a session timeout, but it was erroneously succeeding because no credentials were supplied when attempting to re-access the protected resource.

The attached patch is quite extensive, but cannot easily be broken into smaller units:
1. The AuthenticatorBase.setAlwaysUseSession variable can now be manipulated by test cases.
2. The doTestBasic method has been reimplemented so that it only makes a single HTTP GET request (instead of two).
3. doTestBasic can now be controlled to authenticate or not, and it will also harvest a session cookie and can also supply that cookie in subsequent requests.
4. The doTestNonLogin method can be controlled to send a saved session cookie.
5. The erroneous timeout test case has been reimplemented has been renamed and properly commented to explain that it is not testing a timeout.
6. A new session persistence test case has been added.
7. A new session persistence timeout test case has been added.
8. Raw boolean control flags have been replaced with self-documenting constants.
9. Helpful comments have been added in some places where the logic is not self-evident.

The enclosed patch file is backward compatible and passes checkstyle.
Comment 1 Brian Burch 2012-11-22 13:11:57 UTC
Sorry! I forgot to set a realistic priority to this bug.
Comment 2 Mark Thomas 2012-11-22 15:17:01 UTC
testBasicLoginWithoutSession() seems to repeat the same pair of tests but the comments suggest that something different should happen the second time. What am I missing?
Comment 3 Mark Thomas 2012-11-22 16:46:06 UTC
Your logic makes sense to me so my preference would be some more comments.
Comment 4 Brian Burch 2012-11-26 18:28:00 UTC
Created attachment 29641 [details]
New version of complete patch

This new version is a significant improvement over the previous:
1. Numeric http status has been replaced by standard api contstants.
2. the complex signatures of the doTestXxx methods have been simplified and made more self-documenting.
3. more comments have been added and some comments reworded.
4. code has been cleaned up and validation improved.
5. several new test cases have been added with backward-compatible behaviour (i.e. they succeed at the moment) and TODO comments have been added to cross-reference suggested improvements that would make the BasicAuthenticator more tolerant in future.
Comment 5 Brian Burch 2012-11-26 20:07:58 UTC
Created attachment 29642 [details]
Complete replacement for previous patches

Aesthetic improvements to the inner class, which will probbaly make Mark wonder why I didn't do them before... the answer is: I hadn't gone back to review the inner class once it was working. I must leave this code alone now!
Comment 6 Mark Thomas 2012-11-29 14:37:59 UTC
Thanks. Patch applied to trunk and 7.0.x and will be included in 7.0.34 onwards.

Applying the patch generated a bunch of IDE warnings (we try to keep the code clean of those) which were fixed by r1415178 and r1415179.

If you start looking at the TODOs, I suggest you take a look at  org.apache.tomcat.util.http.parser.HttpParser#parseAuthorizationDigest()

I suspect a new parseAuthorizationBasic() method is the way to go as that should handle the various whitespace issues noted.
Comment 7 Brian Burch 2012-12-03 11:25:42 UTC
Created attachment 29669 [details]
Faster detection of expired session in timeout test case

Reduce run time of testBasicLoginSessionTimeout() from 130 to 80 seconds, based on suggestion (b) from Konstantin Kolinko.
Comment 8 Mark Thomas 2012-12-03 12:11:33 UTC
Patch applied to trunk and 7.0.x and will be included in 7.0.34 onwards. Thanks again.