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 2012/11/22 14:10:23 UTC
[Bug 54190] New: TestNonLoginAndBasicAuthenticator does not test
session timeout properly
https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
Bug ID: 54190
Summary: TestNonLoginAndBasicAuthenticator does not test
session timeout properly
Product: Tomcat 7
Version: trunk
Hardware: PC
OS: Linux
Status: NEW
Severity: normal
Priority: P2
Component: Catalina
Assignee: dev@tomcat.apache.org
Reporter: Brian@PingToo.com
Classification: Unclassified
Created attachment 29621
--> https://issues.apache.org/bugzilla/attachment.cgi?id=29621&action=edit
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.
--
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
Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by Brian Burch <br...@pingtoo.com>.
On 22/11/12 15:17, bugzilla@apache.org wrote:
> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
>
> --- Comment #2 from Mark Thomas <ma...@apache.org> ---
> 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?
Thanks for looking at my change so carefully and quickly.
You are not missing anything, Mark. Perhaps my comments section could be
clearer.
That particular test demonstrates something that I am sure you consider
obvious... tc does not have any mechanism for "remembering" the client's
successful authentication. The third call to doTestBasic, without
providing any credentials, gets the 401 status because the server didn't
have a cached session.
The only reason I coded that explicit 401/200/401/200 sequence was to
make it obviously and directly comparable with
testBasicLoginSessionPersistence, which gets 401/200/200/200.
Strictly speaking, I suppose the fourth doTestBasic is redundant, the
third is a bit like stating the obvious, and then the whole of
testAcceptProtectedBasic could be considered to be a duplicate.
Nevertheless, the only test that takes any time at all is the one that
involves a session timeout.
My main motive for "dotting the i's" was to make the behaviour clear to
a third person who might be trying to understand the way it works.
Do you prefer to make the comments clearer, or take out the redundant logic?
Brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
--- Comment #2 from Mark Thomas <ma...@apache.org> ---
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?
--
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 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
Brian Burch <Br...@PingToo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #29641|0 |1
is obsolete| |
--- Comment #5 from Brian Burch <Br...@PingToo.com> ---
Created attachment 29642
--> https://issues.apache.org/bugzilla/attachment.cgi?id=29642&action=edit
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!
--
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 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
Brian Burch <Br...@PingToo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |REOPENED
Resolution|FIXED |---
--
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 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
Brian Burch <Br...@PingToo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #29621|0 |1
is obsolete| |
--- Comment #4 from Brian Burch <Br...@PingToo.com> ---
Created attachment 29641
--> https://issues.apache.org/bugzilla/attachment.cgi?id=29641&action=edit
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.
--
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
Re: Seeking guidance on use of apache commons classes
Posted by Brian Burch <br...@pingtoo.com>.
On 04/01/13 19:58, Konstantin Kolinko wrote:
> 2013/1/4 Brian Burch <br...@pingtoo.com>:
>> On 29/11/12 16:11, Brian Burch wrote:
>>>
>>> On 29/11/12 14:37, bugzilla@apache.org wrote:
>>>>
>>>> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
>>
>>
>> Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
>> timeout properly
>>
>>>>
>>>> Mark Thomas <ma...@apache.org> changed:
>>>>
>>>> What |Removed |Added
>>>>
>>>> ----------------------------------------------------------------------------
>>>>
>>>> Status|NEW |RESOLVED
>>>> Resolution|--- |FIXED
>>>>
>>>> --- Comment #6 from Mark Thomas <ma...@apache.org> ---
>>
>> <snip/>
>>>
>>>
>>>> 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.
>>>
>>>
>>> Noted. Assume that I will look into it unless you hear otherwise. I
>>> won't open a bug against BasicAuthenticator yet.
>>
>>
>> My motive is the original thread above, but my question has a wider scope. I
>> have started a new thread, even if it doesn't run far, just to make it
>> easier for others to find.
>>
>> I started looking at Mark's suggestion and got a long way with the change.
>>
>> Currently, BasicAuthenticator.authenticate uses
>> org.apache.catalina.util.Base64.decode(ByteChunk,CharChunk).
>>
>> I decided to follow the pattern used by DigestAuthenticator and create a new
>> method
>> org.apache.tomcat.util.http.parser.HttpParser.parseAuthorizationBasic(StringReader).
>>
>> Once I got there, I discovered that org.apache.catalina.util.Base64 doesn't
>> currently have any methods to decode a StringReader or String object.
>>
>> It didn't take me long to find
>> org.apache.commons.codec.binary.Base64.Base64, which has all the right
>> methods to make my own change lightweight and in need of fewer unit tests.
>>
>> I then started hacking the build to use the commons Base64 jar as an
>> external dependency. Unfortunately, that turned out to be more difficult
>> than I expected, and I haven't yet finished the job.
>>
>> I had a lot of non-tomcat priorities and have only just gone back to my
>> work-in-progress. Before I go any further, I would like to find out what the
>> general policy is in this sort of situation:
>>
>> 1. we have code that uses a tomcat-specific utility class that has been
>> stable for a long time.
>>
>> 2. The function of that class is similar, but not identical, to that of one
>> in apache commons.
>>
>> Should we:
>>
>> a) extend the tomcat-specific utility class to provide methods for new logic
>> that is not related to an important bugfix (effectively increasing the
>> duplication of a commons class)
>>
>> or
>>
>> b) use the commons class for the new logic, but leave everything else
>> unchanged (effectively having two classes performing similar tasks)
>>
>> or
>>
>> c) use the commons class and eliminate any redundant logic in
>> tomcat-specific classes (our class wraps the commons class to provide extra
>> functionality but no duplication of core logic)
>>
>>
>> I'm happy to go in any of these 3 directions. My preference would be to use
>> (b) initially, and follow up with (c) soon after.
>>
>> Is there a policy, or a generally held opinion?
>>
>
> The general rule is that Tomcat cannot directly use the commons
> library, as it might clash with such use in a web application. (Though
> if you need them for tests only, such a clash should not be a matter).
>
> Whenever we need classes from commons, we copy them into our source
> tree into a different package (usually with "svn copy" to preserve
> history).
>
> Mark usually removes unused code from the copy, leaving only those
> bits that are actually used by Tomcat.
>
> Examples:
> org.apache.tomcat.util.http.fileupload
> org.apache.tomcat.util.bcel
> org.apache.tomcat.util.digester
>
>> Once I got there, I discovered that org.apache.catalina.util.Base64 doesn't
>> currently have any methods to decode a StringReader or String object.
>
> HTTP headers are converted from bytes to characters using ISO-8859-1
> encoding. (Actually they should be 7-bit ASCII). This conversion is
> reversible.
>
> If all you need it for is a test method, I think I would just convert
> the string to bytes and use the existing API. Do you need to support
> a Reader there?
In fact, the change is not aimed at test code - it will be used by
BasicAuthenticator, although there will also be new tests for the parser
method too.
Thanks very much for your helpful explanation. I understand the reasons,
but I had not thought about the problem that way before - it is almost
the opposite of my intuition. I had thought the dependent jars section
of the tomcat build was a kind of prototype of maven dependencies, and
so assumed that more external dependencies would be a good thing because
it would eliminate duplicate code. It is interesting to be shown a valid
counter-argument.
I will back out the work I've done already to use commons, and proceed
with an extension to the org.apache.catalina.util.Base64 class.
Regards,
Brian
> Best regards,
> Konstantin Kolinko
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Seeking guidance on use of apache commons classes
Posted by Mark Thomas <ma...@apache.org>.
On 04/01/2013 19:58, Konstantin Kolinko wrote:
> 2013/1/4 Brian Burch <br...@pingtoo.com>:
>> On 29/11/12 16:11, Brian Burch wrote:
>> Once I got there, I discovered that org.apache.catalina.util.Base64 doesn't
>> currently have any methods to decode a StringReader or String object.
>
> HTTP headers are converted from bytes to characters using ISO-8859-1
> encoding. (Actually they should be 7-bit ASCII). This conversion is
> reversible.
>
> If all you need it for is a test method, I think I would just convert
> the string to bytes and use the existing API. Do you need to support
> a Reader there?
Even in non-test code I'd be tempted to at least start with that
approach. Anything that handles String is going to have to convert to
bytes anyway.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Seeking guidance on use of apache commons classes
Posted by Konstantin Kolinko <kn...@gmail.com>.
2013/1/4 Brian Burch <br...@pingtoo.com>:
> On 29/11/12 16:11, Brian Burch wrote:
>>
>> On 29/11/12 14:37, bugzilla@apache.org wrote:
>>>
>>> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
>
>
> Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
> timeout properly
>
>>>
>>> Mark Thomas <ma...@apache.org> changed:
>>>
>>> What |Removed |Added
>>>
>>> ----------------------------------------------------------------------------
>>>
>>> Status|NEW |RESOLVED
>>> Resolution|--- |FIXED
>>>
>>> --- Comment #6 from Mark Thomas <ma...@apache.org> ---
>
> <snip/>
>>
>>
>>> 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.
>>
>>
>> Noted. Assume that I will look into it unless you hear otherwise. I
>> won't open a bug against BasicAuthenticator yet.
>
>
> My motive is the original thread above, but my question has a wider scope. I
> have started a new thread, even if it doesn't run far, just to make it
> easier for others to find.
>
> I started looking at Mark's suggestion and got a long way with the change.
>
> Currently, BasicAuthenticator.authenticate uses
> org.apache.catalina.util.Base64.decode(ByteChunk,CharChunk).
>
> I decided to follow the pattern used by DigestAuthenticator and create a new
> method
> org.apache.tomcat.util.http.parser.HttpParser.parseAuthorizationBasic(StringReader).
>
> Once I got there, I discovered that org.apache.catalina.util.Base64 doesn't
> currently have any methods to decode a StringReader or String object.
>
> It didn't take me long to find
> org.apache.commons.codec.binary.Base64.Base64, which has all the right
> methods to make my own change lightweight and in need of fewer unit tests.
>
> I then started hacking the build to use the commons Base64 jar as an
> external dependency. Unfortunately, that turned out to be more difficult
> than I expected, and I haven't yet finished the job.
>
> I had a lot of non-tomcat priorities and have only just gone back to my
> work-in-progress. Before I go any further, I would like to find out what the
> general policy is in this sort of situation:
>
> 1. we have code that uses a tomcat-specific utility class that has been
> stable for a long time.
>
> 2. The function of that class is similar, but not identical, to that of one
> in apache commons.
>
> Should we:
>
> a) extend the tomcat-specific utility class to provide methods for new logic
> that is not related to an important bugfix (effectively increasing the
> duplication of a commons class)
>
> or
>
> b) use the commons class for the new logic, but leave everything else
> unchanged (effectively having two classes performing similar tasks)
>
> or
>
> c) use the commons class and eliminate any redundant logic in
> tomcat-specific classes (our class wraps the commons class to provide extra
> functionality but no duplication of core logic)
>
>
> I'm happy to go in any of these 3 directions. My preference would be to use
> (b) initially, and follow up with (c) soon after.
>
> Is there a policy, or a generally held opinion?
>
The general rule is that Tomcat cannot directly use the commons
library, as it might clash with such use in a web application. (Though
if you need them for tests only, such a clash should not be a matter).
Whenever we need classes from commons, we copy them into our source
tree into a different package (usually with "svn copy" to preserve
history).
Mark usually removes unused code from the copy, leaving only those
bits that are actually used by Tomcat.
Examples:
org.apache.tomcat.util.http.fileupload
org.apache.tomcat.util.bcel
org.apache.tomcat.util.digester
> Once I got there, I discovered that org.apache.catalina.util.Base64 doesn't
> currently have any methods to decode a StringReader or String object.
HTTP headers are converted from bytes to characters using ISO-8859-1
encoding. (Actually they should be 7-bit ASCII). This conversion is
reversible.
If all you need it for is a test method, I think I would just convert
the string to bytes and use the existing API. Do you need to support
a Reader there?
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Seeking guidance on use of apache commons classes
Posted by Brian Burch <br...@pingtoo.com>.
On 29/11/12 16:11, Brian Burch wrote:
> On 29/11/12 14:37, bugzilla@apache.org wrote:
>> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
>>
>> Mark Thomas <ma...@apache.org> changed:
>>
>> What |Removed |Added
>> ----------------------------------------------------------------------------
>>
>> Status|NEW |RESOLVED
>> Resolution|--- |FIXED
>>
>> --- Comment #6 from Mark Thomas <ma...@apache.org> ---
<snip/>
>
>> 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.
>
> Noted. Assume that I will look into it unless you hear otherwise. I
> won't open a bug against BasicAuthenticator yet.
My motive is the original thread above, but my question has a wider
scope. I have started a new thread, even if it doesn't run far, just to
make it easier for others to find.
I started looking at Mark's suggestion and got a long way with the change.
Currently, BasicAuthenticator.authenticate uses
org.apache.catalina.util.Base64.decode(ByteChunk,CharChunk).
I decided to follow the pattern used by DigestAuthenticator and create a
new method
org.apache.tomcat.util.http.parser.HttpParser.parseAuthorizationBasic(StringReader).
Once I got there, I discovered that org.apache.catalina.util.Base64
doesn't currently have any methods to decode a StringReader or String
object.
It didn't take me long to find
org.apache.commons.codec.binary.Base64.Base64, which has all the right
methods to make my own change lightweight and in need of fewer unit tests.
I then started hacking the build to use the commons Base64 jar as an
external dependency. Unfortunately, that turned out to be more difficult
than I expected, and I haven't yet finished the job.
I had a lot of non-tomcat priorities and have only just gone back to my
work-in-progress. Before I go any further, I would like to find out what
the general policy is in this sort of situation:
1. we have code that uses a tomcat-specific utility class that has been
stable for a long time.
2. The function of that class is similar, but not identical, to that of
one in apache commons.
Should we:
a) extend the tomcat-specific utility class to provide methods for new
logic that is not related to an important bugfix (effectively increasing
the duplication of a commons class)
or
b) use the commons class for the new logic, but leave everything else
unchanged (effectively having two classes performing similar tasks)
or
c) use the commons class and eliminate any redundant logic in
tomcat-specific classes (our class wraps the commons class to provide
extra functionality but no duplication of core logic)
I'm happy to go in any of these 3 directions. My preference would be to
use (b) initially, and follow up with (c) soon after.
Is there a policy, or a generally held opinion?
tia,
Brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by Brian Burch <br...@pingtoo.com>.
On 29/11/12 16:46, Rainer Jung wrote:
> On 29.11.2012 17:11, Brian Burch wrote:
>> On 29/11/12 14:37, bugzilla@apache.org wrote:
>>> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
>
>> When I looked quickly at your change, the "-" and "+" lines appeared to
>> be identical to me, so I was puzzled.
>>
>> Could you give me an example of what kind of warnings you were getting -
>> I noticed that you commented on having more than 40, so just the most
>> common one or two would be a help.
>
> If we are talking about r1415178, then it is Boolean -> boolean.
Wow! That was clumsy. Lots of recent work on wikis caused me to
capitalise the data type without even realising. Then cut-n-paste
propagated it. I'll see if I can tell netbeans to stop that in future
cos I can't keep away from wikis!
I'm surprised that netbeans was happy to coerce the type back to boolean
for the call to doTestBasic without even squeaking a little bit. I've
learnt something valuable, so thanks for pointing it out.
> Regards,
>
> Rainer
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by Rainer Jung <ra...@kippdata.de>.
On 29.11.2012 17:11, Brian Burch wrote:
> On 29/11/12 14:37, bugzilla@apache.org wrote:
>> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
> When I looked quickly at your change, the "-" and "+" lines appeared to
> be identical to me, so I was puzzled.
>
> Could you give me an example of what kind of warnings you were getting -
> I noticed that you commented on having more than 40, so just the most
> common one or two would be a help.
If we are talking about r1415178, then it is Boolean -> boolean.
Regards,
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session timeout properly
Posted by Mark Thomas <ma...@apache.org>.
Brian Burch <br...@pingtoo.com> wrote:
>On 29/11/12 14:37, bugzilla@apache.org wrote:
>> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
>>
>> Mark Thomas <ma...@apache.org> changed:
>>
>> What |Removed |Added
>>
>----------------------------------------------------------------------------
>> Status|NEW |RESOLVED
>> Resolution|--- |FIXED
>>
>> --- Comment #6 from Mark Thomas <ma...@apache.org> ---
>> 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.
>
>Thanks Mark.
>
>I use netbeans, not eclipse. My change was clear of ide warnings and it
>
>also passed checkstyle.
>
>When I looked quickly at your change, the "-" and "+" lines appeared to
>
>be identical to me, so I was puzzled.
>
>Could you give me an example of what kind of warnings you were getting
They were all auto[un]boxing warnings. The fix was Boolean -> boolean.
The other change was removing the unused user and pwd parameters from the doTestBasic method.
We have a fairly strict set of warnings enabled although they aren't actually that far from the Eclipse defaults. In many cases fixing the warnings is purely cosmetic although they do catch the occasional subtle bug. One of the improvements for Tomcat 8 is getting the code base down to zero warnings. We are about 30 FindBugs warnings away from that - although there are some commented out Checkstyle settings I'd like to enabel that will add a few thousand ;)
Removing unused code is another area for improvement and there is some way to go on that.
Thanks again for your work on the authenticators,
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by Brian Burch <br...@pingtoo.com>.
On 29/11/12 14:37, bugzilla@apache.org wrote:
> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
>
> Mark Thomas <ma...@apache.org> changed:
>
> What |Removed |Added
> ----------------------------------------------------------------------------
> Status|NEW |RESOLVED
> Resolution|--- |FIXED
>
> --- Comment #6 from Mark Thomas <ma...@apache.org> ---
> 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.
Thanks Mark.
I use netbeans, not eclipse. My change was clear of ide warnings and it
also passed checkstyle.
When I looked quickly at your change, the "-" and "+" lines appeared to
be identical to me, so I was puzzled.
Could you give me an example of what kind of warnings you were getting -
I noticed that you commented on having more than 40, so just the most
common one or two would be a help.
Curiously, you haven't mentioned this kind of problem with my previous
patches, and all have been generated as svn diffs rather than ide diffs.
However, I have recently upgraded both netbeans and svn on my main system.
I probably need to adjust one of my netbeans settings to conform.
> 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.
Noted. Assume that I will look into it unless you hear otherwise. I
won't open a bug against BasicAuthenticator yet.
Regards,
Brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
Mark Thomas <ma...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--- Comment #6 from Mark Thomas <ma...@apache.org> ---
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.
--
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 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
--- Comment #7 from Brian Burch <Br...@PingToo.com> ---
Created attachment 29669
--> https://issues.apache.org/bugzilla/attachment.cgi?id=29669&action=edit
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.
--
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 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
Mark Thomas <ma...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|REOPENED |RESOLVED
Resolution|--- |FIXED
--- Comment #8 from Mark Thomas <ma...@apache.org> ---
Patch applied to trunk and 7.0.x and will be included in 7.0.34 onwards. Thanks
again.
--
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 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
Brian Burch <Br...@PingToo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Priority|P2 |P4
Severity|normal |minor
--- Comment #1 from Brian Burch <Br...@PingToo.com> ---
Sorry! I forgot to set a realistic priority to this bug.
--
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
Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by Brian Burch <br...@pingtoo.com>.
On 22/11/12 18:39, Mark Thomas wrote:
> On 22/11/2012 17:32, Brian Burch wrote:
>> On 22/11/12 16:46, bugzilla@apache.org wrote:
>>> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
>>>
>>> --- Comment #3 from Mark Thomas <ma...@apache.org> ---
>>> Your logic makes sense to me so my preference would be some more
>>> comments.
>>
>> I will think about our discussion and try to improve the comments and so
>> hopefully avoid confusing future readers. I will submit a new
>> self-contained patch and obsolete the current one.
>>
>>
>> Sorry to drift slightly (but not completely) off-topic. Could you give
>> me your off-the-cuff opinion on this:
>>
>> Context.setSessionTimeout(int timeoutInMinutes) obliges tests that need
>> to explore session timeout behaviour to hang the test process for at
>> least 60 seconds, although most of that delay would be unnecessary.
>>
>> I don't actually know how often the Manager (is that the right
>> component?) scans to expire Sessions, but it must do it more frequently
>> than once a minute.
>
> It is the manager. It scans (goes to look up frequency) every 60 seconds
> by default (10s for background process * 6 for processExpiresFrequency)
>
>> I realise the method signature is part of a public api, but do you have
>> a view on adding an alternative method to
>> org.apache.catalina.core.StandardContext so that unit tests could set a
>> session timeout in seconds?
>
> I'm neutral. The delay doesn't really bother me. If you do go this way,
> you'll need to modify processExpiresFrequency for the Manager as well so
> checks happen more often.
Perfect answer, thanks! If the Manager by default is scanning every 60
seconds, then it makes sense to live with a session timeout of 1 minute
and a test case wait of about 70 seconds. That is what I am doing at the
moment, and there isn't much point being clever unless a test suite is
going to call for more than a few timeouts.
> Mark
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by Mark Thomas <ma...@apache.org>.
On 22/11/2012 17:32, Brian Burch wrote:
> On 22/11/12 16:46, bugzilla@apache.org wrote:
>> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
>>
>> --- Comment #3 from Mark Thomas <ma...@apache.org> ---
>> Your logic makes sense to me so my preference would be some more
>> comments.
>
> I will think about our discussion and try to improve the comments and so
> hopefully avoid confusing future readers. I will submit a new
> self-contained patch and obsolete the current one.
>
>
> Sorry to drift slightly (but not completely) off-topic. Could you give
> me your off-the-cuff opinion on this:
>
> Context.setSessionTimeout(int timeoutInMinutes) obliges tests that need
> to explore session timeout behaviour to hang the test process for at
> least 60 seconds, although most of that delay would be unnecessary.
>
> I don't actually know how often the Manager (is that the right
> component?) scans to expire Sessions, but it must do it more frequently
> than once a minute.
It is the manager. It scans (goes to look up frequency) every 60 seconds
by default (10s for background process * 6 for processExpiresFrequency)
> I realise the method signature is part of a public api, but do you have
> a view on adding an alternative method to
> org.apache.catalina.core.StandardContext so that unit tests could set a
> session timeout in seconds?
I'm neutral. The delay doesn't really bother me. If you do go this way,
you'll need to modify processExpiresFrequency for the Manager as well so
checks happen more often.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by Brian Burch <br...@pingtoo.com>.
On 22/11/12 16:46, bugzilla@apache.org wrote:
> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
>
> --- Comment #3 from Mark Thomas <ma...@apache.org> ---
> Your logic makes sense to me so my preference would be some more comments.
I will think about our discussion and try to improve the comments and so
hopefully avoid confusing future readers. I will submit a new
self-contained patch and obsolete the current one.
Sorry to drift slightly (but not completely) off-topic. Could you give
me your off-the-cuff opinion on this:
Context.setSessionTimeout(int timeoutInMinutes) obliges tests that need
to explore session timeout behaviour to hang the test process for at
least 60 seconds, although most of that delay would be unnecessary.
I don't actually know how often the Manager (is that the right
component?) scans to expire Sessions, but it must do it more frequently
than once a minute.
I realise the method signature is part of a public api, but do you have
a view on adding an alternative method to
org.apache.catalina.core.StandardContext so that unit tests could set a
session timeout in seconds?
Thanks,
Brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
--- Comment #3 from Mark Thomas <ma...@apache.org> ---
Your logic makes sense to me so my preference would be some more comments.
--
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