| Summary: | TestNonLoginAndBasicAuthenticator does not test session timeout properly | ||
|---|---|---|---|
| Product: | Tomcat 7 | Reporter: | Brian Burch <Brian> |
| Component: | Catalina | Assignee: | Tomcat Developers Mailing List <dev> |
| Status: | RESOLVED FIXED | ||
| Severity: | minor | ||
| Priority: | P4 | ||
| Version: | trunk | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Attachments: |
Extensive update to the test class to demonstrate session timeout properly
New version of complete patch Complete replacement for previous patches Faster detection of expired session in timeout test case |
||
Sorry! I forgot to set a realistic priority to this bug. 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? Your logic makes sense to me so my preference would be some more comments. 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.
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!
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. 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.
Patch applied to trunk and 7.0.x and will be included in 7.0.34 onwards. Thanks again. |
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.