unit test debugging under Netbeans

[Brian Burch]

When I submitted a bug and fix recently, I promised to develop some unit 
tests to demonstrate the behaviour of my change.


I don't want to get into a *#!&ing contest here, but I switched away 
from eclipse a long time ago, when IBM threw in the towel over not 
having a pure java workbench. Having invested a lot of energy in 
understanding netbeans I'm a committed (but not always happy) user.


I've been using netbeans successfully to debug tomcat and used it 
extensively when investigating

https://issues.apache.org/bugzilla/show_bug.cgi?id=52303.

I didn't realise you had a section on the tomcat wiki
http://wiki.apache.org/tomcat/FAQ/Developing#Q3
because I'm more than comfortable enough to wade through setting up a 
netbeans project for something as complex as tomcat from scratch.


However, I was a bit surprised to discover I couldn't simply "wire up" 
netbeans to compile, run and debug any of the tomcat unit tests!


netbeans provides three standard ide actions for what it calls 
"free-form ant projects", such as tomcat. These ide actions expect the 
ant build.xml for the project to have three corresponding targets that 
can be "connected" to these actions.


I would be happy to update the wiki to describe this process, but I 
don't want to write a load of stuff about modifying the checked-out 
build.xml when it would be MUCH easier to commit my changes to the trunk 
first. My changes are intended to be completely safe when ant runs 
outside netbeans.


I'll just describe the changes for now, to see what kind of reaction I get:

1. existing runtests macro: netbeans looks for a brief non-file junit 
formatter so that it can hoover junit results when running a single unit 
test class and then prettily present them within the workbench gui. I 
have added such a formatter, but made it conditional upon running under 
netbeans and wired it to the run-single ide action.


2. new target compile-selected-files-in-test: the name of this target is 
defined by netbeans convention. I have a cut-n-paste mod of the existing 
test-compile target that accepts a list of classes to compile, because I 
couldn't see a trivial change to your existing target.


3. new target debug-selected-file-in-test: this is a cut-n-paste mod of 
the runtests macro. I need to run junit under the jvm debugger and 
attach the netbeans nbjpdastart ant task to let the gui control the test.


So... can I submit a patch for these three changes to build.xml? I am 
sure this will make some of you nervous, but it seems the cleanest 
approach to me.


How (specifically) should it be submitted - bugzilla?

Regards,

Brian


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

Re: Improving wiki security

[Brian Burch]

On 29/12/11 02:30, Mark Thomas wrote:

Given we see almost as many spam changes as valid ones, is it time for this:

http://wiki.apache.org/general/OurWikiFarm#per_wiki_access_control_-_tighten_your_wiki_just_a_little.2C_benefit_just_a_lot


I already expected to ask for permission before making any changes. I am 
surprised to hear it is currently open access. I am not surprised to 
hear that this generosity and trust is frequently abused. Sad world...


+1

Brian

p.s. can I be added to the list of wiki contributors!

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

Re: unit test debugging under Netbeans

[Brian Burch]

On 28/12/11 23:18, Mark Thomas wrote:

On 28/12/2011 10:49, Brian Burch wrote:

So... can I submit a patch for these three changes to build.xml? I am
sure this will make some of you nervous, but it seems the cleanest
approach to me.


I have no issues with "tweaking" build.xml if it makes integration
easier with Eclipse, NetBeans, a.n. other IDE.

The more invasive the changes the less happy I am likely to be with
them. I'd need to see the proposed patch to comment any further.


Thanks for your quick reply. I am encouraged by your positive comments 
and agree 100% with your stated objectives.


I feel the decision to fold the test/build.xml back into the main 
build.xml when moving from version 6 to 7 was a good thing. When 
starting to work on an unfamiliar project, it is slightly easier to 
understand a complex build when all the properties, paths, targets, etc, 
are in one place. In a small way it encouraged me to investigate the 
netbeans support changes.



Anything IDE specific would be expected to be placed in
res/ide-support/. I have no issues adding anything in
there. One possibility would be a NetBeans specific build.xml that
pulled in what it needed from the main one and added the extra stuff
(with the possibility still to tweak the main script as required).


Yes, I agree entirely. I am trying to keep my nbproject/project.xml as 
generic as possible. I will look carefully at res/eclipse and use it as 
a style model (particularly the source path for tomcat dependencies).


My local build.xml mods are currently working fine for me, so I want to 
make progress on my new unit tests. I am not in a hurry to modify the 
trunk, so I will absorb your thoughts and fine-tune my proposed change 
over the next couple of weeks. (It doesn't seem as if there are a lot of 
netbeans users eager to start debugging tomcat unit tests!)


(Konstantin made some valuable specific comments, so I will reply to 
those soon.)


Thanks,

Brian

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

Re: [GUMP@vmgump]: Project tomcat-trunk-validate (in module tomcat-trunk) failed

[Brian Burch]

On 28/12/11 14:40, Bill Barker wrote:

To whom it may engage...


This caught my attention, but I am puzzled (see details below).


This is an automated request, but not an unsolicited one. For
more information please visit http://gump.apache.org/nagged.html,
and/or contact the folk at gene...@gump.apache.org.

Project tomcat-trunk-validate has an issue affecting its community integration.
This issue affects 1 projects.
The current state of this project is 'Failed', with reason 'Build Failed'.
For reference only, the following projects are affected by this:
 - tomcat-trunk-validate :  Tomcat 8.x, a web server implementing Java 
Servlet 3.1,
 ...


Full details are available at:
 
http://vmgump.apache.org/gump/public/tomcat-trunk/tomcat-trunk-validate/index.html

That said, some information snippets are provided here.

The following annotations (debug/informational/warning/error messages) were 
provided:
  -DEBUG- Dependency on checkstyle exists, no need to add for property 
checkstyle.jar.
  -INFO- Failed with reason build failed



The following work was performed:
http://vmgump.apache.org/gump/public/tomcat-trunk/tomcat-trunk-validate/gump_work/build_tomcat-trunk_tomcat-trunk-validate.html
Work Name: build_tomcat-trunk_tomcat-trunk-validate (Type: Build)
Work ended in a state of : Failed
Elapsed: 28 secs
Command Line: /usr/lib/jvm/java-6-openjdk/bin/java -Djava.awt.headless=true 
-Dbuild.sysclasspath=only org.apache.tools.ant.Main 
-Dgump.merge=/srv/gump/public/gump/work/merge.xml 
-Dcheckstyle.jar=/srv/gump/public/workspace/checkstyle/target/checkstyle-5.6-SNAPSHOT.jar
 -Dexecute.validate=true validate
[Working Directory: /srv/gump/public/workspace/tomcat-trunk]
CLASSPATH: 
/usr/lib/jvm/java-6-openjdk/lib/tools.jar:/srv/gump/public/workspace/ant/dist/lib/ant.jar:/srv/gump/public/workspace/ant/dist/lib/ant-launcher.jar:/srv/gump/public/workspace/ant/dist/lib/ant-jmf.jar:/srv/gump/public/workspace/ant/dist/lib/ant-junit.jar:/srv/gump/public/workspace/ant/dist/lib/ant-swing.jar:/srv/gump/public/workspace/ant/dist/lib/ant-apache-resolver.jar:/srv/gump/public/workspace/ant/dist/lib/ant-apache-xalan2.jar:/srv/gump/public/workspace/xml-commons/java/build/resolver.jar:/srv/gump/public/workspace/checkstyle/target/checkstyle-5.6-SNAPSHOT.jar:/srv/gump/public/workspace/apache-commons/beanutils/dist/commons-beanutils-28122011.jar:/srv/gump/public/workspace/apache-commons/cli/target/commons-cli-1.3-SNAPSHOT.jar:/srv/gump/public/workspace/apache-commons/exec/target/commons-exec-1.1.1-SNAPSHOT.jar:/srv/gump/public/workspace/apache-commons/validator/dist/commons-validator-28122011.jar:/srv/gump/public/workspace/junit/dist/junit-28122011.jar:/srv/gu

mp

  
/public/workspace/junit/dist/junit-dep-28122011.jar:/srv/gump/public/workspace/google-guava/build/dist/guava-28122011/guava-28122011.jar:/srv/gump/public/workspace/apache-commons/logging/target/commons-logging-28122011.jar:/srv/gump/public/workspace/apache-commons/logging/target/commons-logging-api-28122011.jar:/srv/gump/public/workspace/commons-collections-3.x/target/commons-collections-3.3-SNAPSHOT.jar:/srv/gump/packages/antlr/antlr-3.1.3.jar:/srv/gump/public/workspace/jdom/build/jdom.jar:/srv/gump/public/workspace/velocity-engine/bin/velocity-28122011.jar:/srv/gump/public/workspace/velocity-engine/bin/velocity-28122011-dep.jar:/srv/gump/packages/javamail-1.4/mail.jar:/srv/gump/packages/javamail-1.4/lib/mailapi.jar:/srv/gump/packages/jaf-1.1ea/activation.jar
-
Buildfile: /srv/gump/public/workspace/tomcat-trunk/build.xml

download-validate:

proxyflags:

setproxy:

testexist:
  [echo] Testing  for 
/srv/gump/public/workspace/checkstyle/target/checkstyle-5.6-SNAPSHOT.jar

downloadzip:

validate:
 [mkdir] Created dir: 
/srv/gump/public/workspace/tomcat-trunk/output/res/checkstyle
[checkstyle] Running Checkstyle 5.6-SNAPSHOT on 2099 files
[checkstyle] 
/srv/gump/public/workspace/tomcat-trunk/java/org/apache/catalina/authenticator/NonLoginAuthenticator.java:27:
 Wrong order for 'org.apache.catalina.Session' import.


The sort order looks OK to me, but I am just a fallible human. Is this a 
case of Checkstyle being backlevel?


http://sourceforge.net/tracker/?func=detail&atid=397078&aid=2952881&group_id=29721

http://checkstyle.sourceforge.net/releasenotes.html

says this bug was fixed in 5.2, and the log above implies we are using 5.6.

I don't know how to fix the style error, but I can't help feeling 
responsible.



[checkstyle] 
/srv/gump/public/workspace/tomcat-trunk/java/org/apache/catalina/authenticator/NonLoginAuthenticator.java:50:
 Line matches the illegal pattern '\s+$'.


line 50 currently appears to be trivial and identical to line 47, which 
is apparently considered to be acceptable!?! What is wrong?



BUILD FAILED
/srv/gump/public/workspace/tomcat-trunk/build.xml:447: Got 2 errors and 0 
warnings.

Total time: 28 seconds
-

To subscribe to this inf

Re: [GUMP@vmgump]: Project tomcat-trunk-validate (in module tomcat-trunk) failed

[Brian Burch]

On 29/12/11 15:50, Bill Barker wrote:




Thanks for looking at the two errors for me, Bill. It was hard to see 
your comments because the formatting of your reply appeared mangled when 
I received it. I have snipped out everything except the important bits 
below...




"Brian Burch" wrote in message news:4efbea99.1020...@pingtoo.com...

On 28/12/11 14:40, Bill Barker wrote:

To whom it may engage...


This caught my attention, but I am puzzled (see details below).

-> Buildfile:
/srv/gump/public/workspace/tomcat-trunk/build.xml>> download-validate:>>
proxyflags:>> setproxy:>> testexist:> [echo] Testing
for/srv/gump/public/workspace/checkstyle/target/checkstyle-5.6-SNAPSHOT.jar>>
downloadzip:>> validate:> [mkdir] Created
dir:/srv/gump/public/workspace/tomcat-trunk/output/res/checkstyle>
[checkstyle] Running Checkstyle 5.6-SNAPSHOT on 2099 files>
[checkstyle]/srv/gump/public/workspace/tomcat-trunk/java/org/apache/catalina/authenticator/NonLoginAuthenticator.java:27:
Wrong order for'org.apache.catalina.Session' import.>>The sort order
looks OK to me, but I am just a fallible human. Is this a case of
Checkstyle being
backlevel?>>http://sourceforge.net/tracker/?func=detail&atid=397078&aid=2952881&group_id=29721>>http://checkstyle.sourceforge.net/releasenotes.html>>says
this bug was fixed in 5.2, and the log above implies we are using
5.6.>>I don't know how to fix the style error, but I can't help
feeling responsible.



The import is clearly out of order. It should go ahead of the
other org.apache.catalina.xxx.yyy imports.


Thanks for explaining, Bill. It wasn't clear to me because I am not 
familiar with that particular checkstyle module.



[checkstyle]/srv/gump/public/workspace/tomcat-trunk/java/org/apache/catalina/authenticator/NonLoginAuthenticator.java:50:
Line matches the >illegal pattern '\s+$'.>>line 50 currently appears to
be trivial and identical to line 47, which is apparently considered to be
acceptable!?! What is wrong?



Running the validate target on Windows
doesn't flag this one for me(admittedly using 5.5 checkstyle instead of
5.6-SNAPSHOT). At a guess,maybe it got a CRLF line ending somehow, so it
gets flagged by Gump runningon Linux but not on Windows. Of course, it
could also be due to using5.6-SNAPSHOT and a bug in checkstyle. I'm not
interested enough to try itmyself on Linux, but that would be a check.


After checking out the latest source, I copy-n-pasted line 47 over the 
top of line 50 under linux, but svn diff does not detect the change.


/res/checkstyle/checkstyle.xml has the following test:

  


  

... but I cannot see any trailing white space on line 50 (or 
48,49,51,52). Perhaps someone fixed it without me noticing?





BUILD FAILED>> /srv/gump/public/workspace/tomcat-trunk/build.xml:447:
Got 2 errors and 0warnings.>>> Total time: 28 seconds>
-


Would an authorised developer kindly apply the following patch to 
resolve the import order problem for me?


===
--- java/org/apache/catalina/authenticator/NonLoginAuthenticator.java 
(revision 1225420)
+++ java/org/apache/catalina/authenticator/NonLoginAuthenticator.java 
(working copy)

@@ -22,9 +22,9 @@

 import javax.servlet.http.HttpServletResponse;

+import org.apache.catalina.Session;
 import org.apache.catalina.connector.Request;
 import org.apache.catalina.deploy.LoginConfig;
-import org.apache.catalina.Session;

After applying this change locally, it passes the checkstyle rules as 
currently defined in the trunk and implemented by checkstyle version 5.5 
downloaded as a tomcat dependency.


Thanks,

Brian

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

Re: unit test debugging under Netbeans

[Brian Burch]

On 28/12/11 22:27, Konstantin Kolinko wrote:

Thanks very much for your time and valuable thoughts, Konstantin.


2011/12/28 Brian Burch:



However, I was a bit surprised to discover I couldn't simply "wire up"
netbeans to compile, run and debug any of the tomcat unit tests!

netbeans provides three standard ide actions for what it calls "free-form
ant projects", such as tomcat. These ide actions expect the ant build.xml
for the project to have three corresponding targets that can be "connected"
to these actions.


Are you saying that NetBeans cannot run an arbitrary user-chosen
target of build.xml, but needs them to have specific names in English?


I was saying that, but your question prompted me to check more carefully 
and now I see I was wrong.


Netbeans automatically creates a build.xml and project.xml for most 
kinds of new projects using "standard" target names and wiring them to 
standard ide actions.


However, when "introducing" an existing project to netbeans as what it 
calls a "free-form ant project", all the (rather confusing and 
out-of-date) documentation assumes that you will adhere to the existing 
naming conventions. I can now see that an ide action in project.xml can 
be associated with any valid build target.


In other words, I do not need to create new targets simply to achieve a 
redirect to an existing target that already does the desired job.



It cannot run an arbitrary target under debugger?


As far as I understand, that question makes sense with eclipse, but not 
with netbeans.


Because netbeans projects are (always and only?) manipulated through 
their ant build.xml (either created by wizards, or pre-defined in 
free-form projects), all projects need a target that will start the jvm 
in debugging mode and then call the nbjpdastart task to connect to the 
debugger socket. If not, then you can't debug.


When I want to debug tomcat itself, I just run "catalina jpda start" 
(thus running it outside netbeans) and tell netbeans to start a debugger 
session to connect to the target jvm. However, when I want to debug a 
unit test under the netbeans ide, I believe I need a suitable ant 
target. However, I'll give your question more thought...



Note, that you can specify a single test name in build.properties.


Yes, I noticed that was the case. My netbeans project.xml already 
assigns the ${test.entry} property to the selected fully-qualified 
classname before calling ant to run my two new single test targets.



I would be happy to update the wiki to describe this process, but I don't
want to write a load of stuff about modifying the checked-out build.xml when
it would be MUCH easier to commit my changes to the trunk first. My changes
are intended to be completely safe when ant runs outside netbeans.

I'll just describe the changes for now, to see what kind of reaction I get:

1. existing runtests macro: netbeans looks for a brief non-file junit
formatter so that it can hoover junit results when running a single unit
test class and then prettily present them within the workbench gui. I have
added such a formatter, but made it conditional upon running under netbeans
and wired it to the run-single ide action.


Formatter is configurable since 7.0.24.


Silly me! A ${test.formatter} property is defined immediately after 
${test.name}! However, the runtests macro assumes I want usefile="true", 
but that prevents netbeans from intercepting the results.


I will think about whether I really want both formatters active on the 
same netbeans test run (as in my current version). If not, I need to 
work out how to inject a value without the user having to change it in 
build.properties, where it would affect non-netbeans test runs.



2. new target compile-selected-files-in-test: the name of this target is
defined by netbeans convention. I have a cut-n-paste mod of the existing
test-compile target that accepts a list of classes to compile, because I
couldn't see a trivial change to your existing target.


You must compile all files and pack them into jars etc. as needed. It
is not possible to compile a single file.


I was talking about compiling a selection-list of unit test classes. I 
remember that eclipse compiles classes on-the-fly. Netbeans /appears/ to 
do the same thing, but it actually only compiles the class in the editor 
window (to show compilation errors), not into the project build 
directory. When you run an individual test class under netbeans, it MUST 
have been previously compiled by the ant build.xml into the correct 
output directory. This process seems to be working fine for me already 
because netbeans/ant submits the individual class file to run under the 
jvm/junit.


[note to self: the run-single and debug-single targets are dependent on 
the test-compile target, thus triggering a compilation of the selected 
single unit test file (if necessary)

problem using default Realm in new unit tests

[Brian Burch]

I am developing some new unit tests to validate SingleSignOn and 
Authenticator logic. I have used this existing test class as my template:


org.apache.catalina.authenticator.TestDigestAuthenticator

.. which extends org.apache.catalina.startup.TomcatBaseTest.

I noticed that TestDigestAuthenticator sets up its own MapRealm and 
assigns it to its single Context. I thought this logic was unnecessary, 
and so my own initial test logic simply used the default RealmBase 
provided by the underlying Tomcat instance. I add my test user and role 
to that. It worked fine with my simple cases, however...


To test SSO, I need to set up two Contexts under the same Realm. I see 
the following message in the output log:


INFO: The start() method was called on component [Realm[Simple]] after 
start() had already been called. The second call will be ignored.


I know it is an INFO message. I know the second start (and its 
associated stop) are ignored and therefore are harmless. However, I am 
reluctant to simply shrug and ignore it. My instincts tell me something 
isn't right.


I have done quite a lot of investigating, but the underlying logic is 
very hard for me to follow. Here is what I am sure about:


1. The message is ONLY emitted in tests that create two Contexts and 
each have the same Realm assigned with setRealm.


2. The message is NOT emitted at the time the Contexts are created and 
defined (servlets, security constraints, etc).


3. The message IS emitted after the Tomcat.start method is called.

4. The message is emitted by one of the two threads which are started on 
behalf of my two contexts. The messages are issued by the start and stop 
methods in the abstract class org.apache.catalina.util.LifecycleBase.


5. org.apache.catalina.realm.RealmBase extends 
org.apache.catalina.util.LifecycleMBeanBase which extends LifecycleBase.


My currently unanswered questions are:

1. Is this message normal? (I don't see it when I start multiple 
contexts under a real tomcat server, but perhaps the logging properties 
are different).


2. Why isn't the redundant startup of the Realm detected earlier and 
simply avoided (maybe the Threads are intended to race to be first with 
startup - but then I think the message should be debug level and not 
sound so scary).



Please don't waste your time investigating this for me. I am only asking 
the question because I don't want too get side-tracked if one of you 
already knows the answers to my questions. I'd like to settle the matter 
quickly and get back to my original task!


Thanks for listening,

Brian

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

Re: problem using default Realm in new unit tests

[Brian Burch]

On 06/01/12 11:47, Konstantin Kolinko wrote:

2012/1/6 Brian Burch:

I am developing some new unit tests to validate SingleSignOn and
Authenticator logic. I have used this existing test class as my template:

org.apache.catalina.authenticator.TestDigestAuthenticator

.. which extends org.apache.catalina.startup.TomcatBaseTest.

I noticed that TestDigestAuthenticator sets up its own MapRealm and assigns
it to its single Context. I thought this logic was unnecessary, and so my
own initial test logic simply used the default RealmBase provided by the
underlying Tomcat instance. I add my test user and role to that. It worked
fine with my simple cases, however...

To test SSO, I need to set up two Contexts under the same Realm. I see the
following message in the output log:

INFO: The start() method was called on component [Realm[Simple]] after
start() had already been called. The second call will be ignored.

I know it is an INFO message. I know the second start (and its associated
stop) are ignored and therefore are harmless. However, I am reluctant to
simply shrug and ignore it. My instincts tell me something isn't right.

I have done quite a lot of investigating, but the underlying logic is very
hard for me to follow. Here is what I am sure about:

1. The message is ONLY emitted in tests that create two Contexts and each
have the same Realm assigned with setRealm.

2. The message is NOT emitted at the time the Contexts are created and
defined (servlets, security constraints, etc).

3. The message IS emitted after the Tomcat.start method is called.

4. The message is emitted by one of the two threads which are started on
behalf of my two contexts. The messages are issued by the start and stop
methods in the abstract class org.apache.catalina.util.LifecycleBase.

5. org.apache.catalina.realm.RealmBase extends
org.apache.catalina.util.LifecycleMBeanBase which extends LifecycleBase.

My currently unanswered questions are:

1. Is this message normal? (I don't see it when I start multiple contexts
under a real tomcat server, but perhaps the logging properties are
different).

2. Why isn't the redundant startup of the Realm detected earlier and simply
avoided (maybe the Threads are intended to race to be first with startup -
but then I think the message should be debug level and not sound so scary).


Please don't waste your time investigating this for me. I am only asking the
question because I don't want too get side-tracked if one of you already
knows the answers to my questions. I'd like to settle the matter quickly and
get back to my original task!

Thanks for listening,



The message is expected. I would say that the configuration is wrong,
or at least unusual.

If you look at the default server.xml file you will note that the
  element there belongs to Engine. That is why it is started
once.


I agree, and this is what the TomcatBaseTest expects. If you "tickle" 
tomcat with TomcatBaseTest.getTomcatInstance() and then with 
Tomcat.getDefaultRealm(), a new default RealBase (memory) instance is 
definitely created.



When Contexts are created and their context.xml files are parsed, the
Contexts always get distinct new Realm instances.

Instead of assigning the same Realm instance to both Contexts you
should assign it to an upper container (I have not looked at the API
though).  Or maybe you can have different Realm instances, but which
connect to the same backing storage?


When the StandardEngine (extending ContainerBase) thread starts, I would 
expect percolating getRealm calls (from Context to Host to Engine) to 
eventually arrive at the already-defined Tomcat default Realm. HOWEVER, 
StandardEngine overrides the ContainerBase.getRealm method and ensures 
that an unconfigured JAAS Realm is instantiated as the default for the 
Context, because it cannot locate its parent (the field is certainly 
null, not tomcat), so it decides to use this JAAS as the backstop.


This looks to me as if some refactoring between tomcat 5 and 6 left the 
Tomcat default memory realm orphaned from the StandardEngine, which now 
operates independently and establishes a completely different default 
realm. Perhaps the right hand no longer knows what the left hand is 
doing


Funny thing is that I googled this:

http://tomcat.10.n6.nabble.com/default-realm-set-to-JAASRealm-in-StandardEngine-td2005479.html

... and your name (Konstantin) is all over it! Can you cast you mind 
back 4 months and try to give me a clue about the history of this change 
to the logic?


My feeling is that we need to stop StandardEngine from unilaterally 
creating an unconfigured JAAS default Realm... and help the poor thing 
find its true parent, i.e. tomcat, which has a perfectly serviceable 
default memory realm just waiting to be used!


(apologies for the mild sarcasm - I can see you are very busy on other 
issues),


Brian


--

Re: problem using default Realm in new unit tests

[Brian Burch]

Konstantin,

Thank you for your prompt and helpful response... at least I know I 
haven't overlooked something simple. With your encouragement, I'm happy 
to keep "fighting the alligators until I can get back to draining the 
swamp"!


On 10/01/12 02:56, Konstantin Kolinko wrote:

2012/1/9 Brian Burch:

On 06/01/12 11:47, Konstantin Kolinko wrote:


2012/1/6 Brian Burch:


I am developing some new unit tests to validate SingleSignOn and
Authenticator logic. I have used this existing test class as my template:

org.apache.catalina.authenticator.TestDigestAuthenticator

.. which extends org.apache.catalina.startup.TomcatBaseTest.

I noticed that TestDigestAuthenticator sets up its own MapRealm and
assigns
it to its single Context. I thought this logic was unnecessary, and so my
own initial test logic simply used the default RealmBase provided by the
underlying Tomcat instance. I add my test user and role to that. It
worked
fine with my simple cases, however...

To test SSO, I need to set up two Contexts under the same Realm. I see
the
following message in the output log:

INFO: The start() method was called on component [Realm[Simple]] after
start() had already been called. The second call will be ignored.

I know it is an INFO message. I know the second start (and its associated
stop) are ignored and therefore are harmless. However, I am reluctant to
simply shrug and ignore it. My instincts tell me something isn't right.

I have done quite a lot of investigating, but the underlying logic is
very
hard for me to follow. Here is what I am sure about:

1. The message is ONLY emitted in tests that create two Contexts and each
have the same Realm assigned with setRealm.

2. The message is NOT emitted at the time the Contexts are created and
defined (servlets, security constraints, etc).

3. The message IS emitted after the Tomcat.start method is called.

4. The message is emitted by one of the two threads which are started on
behalf of my two contexts. The messages are issued by the start and stop
methods in the abstract class org.apache.catalina.util.LifecycleBase.

5. org.apache.catalina.realm.RealmBase extends
org.apache.catalina.util.LifecycleMBeanBase which extends LifecycleBase.

My currently unanswered questions are:

1. Is this message normal? (I don't see it when I start multiple contexts
under a real tomcat server, but perhaps the logging properties are
different).

2. Why isn't the redundant startup of the Realm detected earlier and
simply
avoided (maybe the Threads are intended to race to be first with startup
-
but then I think the message should be debug level and not sound so
scary).


Please don't waste your time investigating this for me. I am only asking
the
question because I don't want too get side-tracked if one of you already
knows the answers to my questions. I'd like to settle the matter quickly
and
get back to my original task!

Thanks for listening,



The message is expected. I would say that the configuration is wrong,
or at least unusual.

If you look at the default server.xml file you will note that the
element there belongs to Engine. That is why it is started
once.



I agree, and this is what the TomcatBaseTest expects. If you "tickle" tomcat
with TomcatBaseTest.getTomcatInstance() and then with
Tomcat.getDefaultRealm(), a new default RealBase (memory) instance is
definitely created.



When Contexts are created and their context.xml files are parsed, the
Contexts always get distinct new Realm instances.

Instead of assigning the same Realm instance to both Contexts you
should assign it to an upper container (I have not looked at the API
though).  Or maybe you can have different Realm instances, but which
connect to the same backing storage?



When the StandardEngine (extending ContainerBase) thread starts, I would
expect percolating getRealm calls (from Context to Host to Engine) to
eventually arrive at the already-defined Tomcat default Realm. HOWEVER,
StandardEngine overrides the ContainerBase.getRealm method and ensures that
an unconfigured JAAS Realm is instantiated as the default for the Context,
because it cannot locate its parent (the field is certainly null, not
tomcat), so it decides to use this JAAS as the backstop.

This looks to me as if some refactoring between tomcat 5 and 6 left the
Tomcat default memory realm orphaned from the StandardEngine, which now
operates independently and establishes a completely different default realm.
Perhaps the right hand no longer knows what the left hand is doing

Funny thing is that I googled this:

http://tomcat.10.n6.nabble.com/default-realm-set-to-JAASRealm-in-StandardEngine-td2005479.html

... and your name (Konstantin) is all over it! Can you cast you mind back 4
months and try to give me a clue about the history of this change to the
logic?

My feeling is that we need to stop StandardEngine from unilaterally creating
an unconfigured JAAS default Realm.

Re: problem using default Realm in new unit tests

[Brian Burch]

On 10/01/12 13:21, Brian Burch wrote:

Konstantin,

Thank you for your prompt and helpful response... at least I know I
haven't overlooked something simple. With your encouragement, I'm happy
to keep "fighting the alligators until I can get back to draining the
swamp"!


I am working on this issue in my spare time while on holiday in 
Australia. I was testing a local fix yesterday which inserted the 
default MemoryRealm into the StandardEngine. I decided to go to bed and 
email my proposed change in the morning.


While I was sleeping, Mark committed r1230777 (and r1230767), which is 
very similar to mine, but was more bold. I was nervous (through 
ignorance) about the consequences for the standalone and embedded cases.


I have now reworked my new unit tests to work with Mark's change (they 
no longer need to explicitly trigger assignment of the default 
MemoryRealm), and they all run successfully. I will submit them soon.



Sorry to keep this topic open, but I have some outstanding questions 
following Mark's change, so perhaps someone could spare the time to 
answer them for me?


Tomcat.getEngine has not changed its overall behaviour: it creates the 
Engine when it doesn't already exist. However, it now also creates and 
assigns the default MemoryRealm to the new StandardEngine instance.


I can see the StandardEngine constructor is also called in both the 
MBeanFactory and Embedded classes. MBeanFactory, at first glance, seems 
to set the Realm on request, but not define a default for the Engine. 
The Embedded constructors will either explicitly set a Realm, or leave 
it uninitialised.


If this observation is correct, then there are real cases where the 
Engine can be created without a default realm, in which case 
StandardEngine will create a default JAASRealm as soon as the getRealm 
method is called. This JAASRealm MUST be properly configured in the jvm 
if an Exception is to be avoided.


Is it still meaningful to let the StandardEngine establish a default 
JAASRealm in getRealm under these circumstances, or would it be more 
consistent to use a default MemoryRealm? Anyone who wants to use a 
JAASRealm would surely have to configure it, so isn't it is a bit lazy 
to rely on implicit assignment. Surely these users would (should) 
explicitly define the JAASRealm in Embedded or MBeanFactory?


Perhaps the logic in StandardEngine.getRealm to catch these special 
cases and create a default JAASRealm is redundant?


Thanks,

Brian

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

Re: svn commit: r1183612 [2/2] - /tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java

[Brian Burch]

On 14/01/12 09:45, Filip Hanik - Dev Lists wrote:

No grudge held :)
I'm just bringing it up since very many files have been reformatted for
the sake of formatting. And when tracing down a problem, I, and I
suspect others too, often use SVN history to figure out what and how
changed. So a formatting change that makes satisfies a personal
formatting preference for one developer, down the line can make it a lot
harder to trace changes for others.


I don't want to throw petrol on glowing embers, and I realise my own 
efforts barely count. However, when I've been researching the history of 
changes in logic that I didn't fully understand, the svn history has 
been unhelpful and confusing. Now I understand why!


Brian



Filip

On 1/13/2012 3:34 PM, Konstantin Kolinko wrote:

2012/1/14 Filip Hanik - Dev Lists:

Commits like this, make it very difficult to trace down changes in
the SVN
history.
I know both Mark and Konstantin are very keen on formatting, to the
point
where it overrides the priority of tracing down changes.
I may be alone, but I'd prefer that we don't do this mega format
entire file
for no apparent purpose commits anymore.

It is technical issue due to broken git-svn client that committed this
file with wrong line-ends (violating Subversion protocol).

It was discussed here on dev@tomcat (and users@subversion) in October
2011. It is nothing to grudge about.

(Anyway to lookup the history further requires just several clicks in
viewvc to ask annotations starting with different revision.)

http://tomcat.markmail.org/thread/f4rdxrjvrenc6tg6
http://subversion.markmail.org/thread/uovs5c7mgcnyp4an



Filip

On 10/15/2011 3:47 AM, kkoli...@apache.org wrote:

Modified:
tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java?rev=1183612&r1=1183611&r2=1183612&view=diff


==

--- tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java
(original)
+++ tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java
Sat
Oct 15 09:47:39 2011
@@ -1,2514 +1,2514 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed
with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version
2.0
- * (the "License"); you may not use this file except in compliance
with
- * the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */

(no need to quote 2K of text)

Best regards,
Konstantin Kolinko

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





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



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

New unit tests for Authenticators and SingleSignOn

[Brian Burch]

I realise everyone will be busy on the 7.0.24 release, so don't let this 
distract you - it isn't urgent.


Before I started developing my unit tests for SSO, I wrote a test class 
for NonLoginAuthenticator and BasicAuthenticator. This test class checks 
the behaviour of both authenticators when the SSO valve is NOT present.


I decided a single class was better than two because a) there are very 
few cases to be tested with these very simple authenticators, and b) the 
SSO test class would be cloned from it and SSO needs to use multiple 
authenticators.


When someone has time, could you please take a look at my code and (if 
acceptable) commit it to the trunk. I also have two SSO test classes 
ready to go, but I will wait to see whether there is anything wrong with 
the first one.


Here is the source for the class, which passes checkstyle under ant 
validate on my system:



/*
 *  Licensed to the Apache Software Foundation (ASF) under one or more
 *  contributor license agreements.  See the NOTICE file distributed with
 *  this work for additional information regarding copyright ownership.
 *  The ASF licenses this file to You under the Apache License, Version 2.0
 *  (the "License"); you may not use this file except in compliance with
 *  the License.  You may obtain a copy of the License at
 *
 *  http://www.apache.org/licenses/LICENSE-2.0
 *
 *  Unless required by applicable law or agreed to in writing, software
 *  distributed under the License is distributed on an "AS IS" BASIS,
 *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or 
implied.

 *  See the License for the specific language governing permissions and
 *  limitations under the License.
 */
package org.apache.catalina.authenticator;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;

import org.junit.Test;

import org.apache.catalina.Context;
import org.apache.catalina.deploy.LoginConfig;
import org.apache.catalina.deploy.SecurityCollection;
import org.apache.catalina.deploy.SecurityConstraint;
import org.apache.catalina.startup.TesterServlet;
import org.apache.catalina.startup.Tomcat;
import org.apache.catalina.startup.TomcatBaseTest;
import org.apache.catalina.util.Base64;
import org.apache.tomcat.util.buf.ByteChunk;

/**
 * Test BasicAuthenticator and NonLoginAuthenticator when a
 * SingleSignOn Valve is not active.
 *
 * In the absence of SSO support, these two authenticator classes
 * both have quite simple behaviour. By testing them together, we
 * can make sure they operate independently and confirm that no
 * SSO logic has been accidentally triggered.
 *
 * @author Brian Burch
 */
public class TestNonLoginAndBasicAuthenticator extends TomcatBaseTest {

private static final String USER = "user";
private static final String PWD = "pwd";
private static final String ROLE = "role";

private static final String HTTP_PREFIX = "http://localhost:";;
private static final String CONTEXT_PATH_NOLOGIN = "/nologin";
private static final String CONTEXT_PATH_LOGIN = "/login";
private static final String URI_PROTECTED = "/protected";
private static final String URI_PUBLIC = "/anyoneCanAccess";

private static final int SHORT_TIMEOUT_SECS = 4;
private static final int LONG_TIMEOUT_SECS = 10;
private static final long LONG_TIMEOUT_DELAY_MSECS =
((LONG_TIMEOUT_SECS + 2) * 1000);

private static String CLIENT_AUTH_HEADER = "authorization";

/*
 * Try to access an unprotected resource in a webapp that
 * does not have a login method defined.
 * This should be permitted.
 */
@Test
public void testAcceptPublicNonLogin() throws Exception {
doTestNonLogin(USER, PWD, CONTEXT_PATH_NOLOGIN + URI_PUBLIC,
false, 200);
}

/*
 * Try to access a protected resource in a webapp that
 * does not have a login method defined.
 * This should be rejected with SC_FORBIDDEN 403 status.
 */
@Test
public void testRejectProtectedNonLogin() throws Exception {
doTestNonLogin(USER, PWD, CONTEXT_PATH_NOLOGIN + URI_PROTECTED,
true, 403);
}

/*
 * Try to access an unprotected resource in a webapp that
 * has a BASIC login method defined.
 * This should be permitted without a challenge.
 */
@Test
public void testAcceptPublicBasic() throws Exception {
doTestBasic(USER, PWD, CONTEXT_PATH_LOGIN + URI_PUBLIC,
false, 200, false, 200);
}

/*
 * Try to access a protected resource in a webapp that
 * has a BASIC login method defined. The access will be
 * challenged, authenticated and then permitted.
 */
@Test
pub

Re: New unit tests for Authenticators and SingleSignOn

[Brian Burch]

On 15/01/12 04:16, Mark Thomas wrote:

On 14/01/2012 08:24, Brian Burch wrote:

I realise everyone will be busy on the 7.0.24 release, so don't let this
distract you - it isn't urgent.


I had to fix a failing unit test so it was no bother.


Thanks very much, Mark.


I've added this to trunk and will port it to 7.0.x once I have taken a
closer look at it. The only change so far is to remove the author tag
which we no longer add in Tomcat. You'll get full credit in the commit
messages and the change log.


It wasn't my ego that made me put it there! I used one of the older 
tests as my template and tried to follow the style... this class doesn't 
look much like any junit test I've written before, but I believe 
maintaining project style is very important. Once a new test class works 
in the trunk, it will be most likely only looked at by people who are 
either fixing a new bug, or trying to understand an existing contract.


The only aspect that leaves me feeling slightly uncomfortable is my use 
of session timeouts. The main point of the SSO tests (as well as 
confirming obvious behaviour, of course), is to verify that SSO sessions 
time out properly. I did this by modelling the "real world", using http 
status codes and short timeouts, but making them long enough for the 
tomcat background thread to reliably clean them out (i.e. trigger 
listener methods) when expected.


I resisted the temptation to..
a) reconfigure the background thread to scan for expired sessions more 
frequently,

and b) write a test-only event listener to confirm session expiry.

My decision was to make the tests easily understandable by people who 
don't have much internals knowledge. Also, the authenticators are 
responsible for setting and returning the error status codes and I 
didn't like the idea of skipping that logic.


The price is a few extra 5-10 second sleeps, but I felt this was 
negligible compared to starting and stopping tomcat hundreds 
(thousands?) of times in a full test run.


Do you approve of my design decision?

I will wait a couple of days in case you have any detailed observations 
(and changes). Then I'll make the next SSO-enabled test look consistent 
and submit it.


Brian


Mark

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



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

Re: svn commit: r1231550 - in /tomcat/tc7.0.x/trunk: ./ test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java

[Brian Burch]

On 15/01/12 04:24, ma...@apache.org wrote:

Author: markt
Date: Sat Jan 14 18:24:27 2012
New Revision: 1231550

URL: http://svn.apache.org/viewvc?rev=1231550&view=rev
Log:
Fix warnings


Curious... I don't get any compiler warnings and checkstyle said it was 
OK. I presume your compiler didn't like only ever passing the same 
static final variables as arguments.


I'm not sure whether this test class, or any based on it (style-wise) 
will ever need to provide more than one set of credentials. Wouldn't it 
have been better to pass credentials as an argument?


Also, I have an SSO variant of this test class that uses DIGEST 
authentication. It builds the credentials string in a different format.


To summarise, without trying to pick a fight, I would prefer to pass the 
userid and password into the generalised utility methods as arguments, 
rather than have those methods assume there will only ever be one value 
for each variable.


Can you see a way that keeps me and your compiler happy at the same 
time, please?


Sorry!

Brian


Modified:
 tomcat/tc7.0.x/trunk/   (props changed)
 
tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java

Propchange: tomcat/tc7.0.x/trunk/
--
--- svn:mergeinfo (original)
+++ svn:mergeinfo Sat Jan 14 18:24:27 2012
@@ -1 +1 @@
-/tomcat/trunk:1156115,1156171,1156276,1156304,1156519,1156530,1156602,1157015,1157018,1157151,1157198,1157204,1157810,1157832,1157834,1157847,1157908,1157939,1158155,1158160,1158176,1158195,1158198-1158199,1158227,1158331,1158334-1158335,1158426,1160347,1160592,1160611,1160619,1160626,1160639,1160652,1160720-1160721,1160772,1160774,1160776,1161303,1161310,1161322,1161339,1161486,1161540,1161549,1161584,1162082,1162149,1162169,1162721,1162769,1162836,1162932,1163630,1164419,1164438,1164469,1164480,1164567,1165234,1165247-1165248,1165253,1165273,1165282,1165309,1165331,1165338,1165347,1165360-1165361,1165367-1165368,1165602,1165608,1165677,1165693,1165721,1165723,1165728,1165730,1165738,1165746,1165765,1165777,1165918,1165921,1166077,1166150-1166151,1166290,1166366,1166620,1166686,1166693,1166752,1166757,1167368,1167394,1169447,1170647,1171692,1172233-1172234,1172236,1172269,1172278,1172282,1172556,1172610,1172664,1172689,1172711,1173020-1173021,1173082,1173088,1173090,11730

96

  
,1173241,1173256,1173288,117,1173342,1173461,1173614,1173630,1173659,1173722,1174061,1174239,1174322,1174325,1174329-1174330,1174337-1174339,1174343,1174353,1174799,1174882,1174884,1174975,1174983,1175155,1175158,1175167,1175182,1175190,1175201,1175272,1175275,1175283,1175582,1175589-1175590,1175594,1175602,1175613,1175633,1175690,1175713,1175798,1175889,1175896,1175907,1176584,1176590,1176799,1177050,1177060,1177125,1177152,1177160,1177245,1177850,1177862,1177978,1178209,1178228,1178233,1178449,1178542,1178681,1178684,1178721,1179268,1179274,1180261,1180865,1180891,1180894,1180907,1181028,1181123,1181125,1181136,1181291,1181743,1182796,1183078,1183105,1183142,1183328,1183339-1183340,1183492-1183494,1183605,1184917,1184919,1185018,1185020,1185200,1185588,1185626,1185756,1185758,1186011,1186042-1186045,1186104,1186123,1186137,1186153,1186254,1186257,1186377-1186379,1186479-1186480,1186712,1186743,1186750,1186763,1186890-1186892,1186894,1186949,1187018,1187027-1187028,1

187

  
381,1187753,1187755,1187775,1187801,1187806,1187809,1187827,1188301,1188303-1188305,1188399,1188822,1188930-1188931,1189116,1189129,1189183,1189240,1189256,1189386,1189413-1189414,1189477,1189685,1189805,1189857,1189864,1189882,1190034,1190185,1190279,1190339,1190371,1190388-1190389,1190474,1190481,1194915,1195222-1195223,1195531,1195899,1195905,1195943,1195949,1195953,1195955,1195965,1195968,1196175,1196212,1196223,1196304-1196305,1196735,1196825,1196827,1197158,1197261,1197263,1197299-1197300,1197305,1197339-1197340,1197343,1197382,1197386-1197387,1197480,1197578,1198497,1198528,1198552,1198602,1198604,1198607,1198622,1198640,1198696,1198707,1199418,1199432,1199436,1199513,1199529,1199980,116,1200056,1200089,1200106-1200107,1200263,1200316,1200320,1200398-1200399,1200445-1200446,1200555,1200627,1200696,1200725,1200937,1200941,1201069,1201087,1201180,1201235-1201237,1201508,1201521,1201542,1201545-1201546,1201548,1201555-1201556,1201568,1201576,1201608,1201921-120192

2,1

  
201931,1202035,1202039,1202271,1202565,1202578,1202705,1202828,1202860,1203047-1203052,1203078,1203091,1203253,1203278,1204182,1204856,1204867,1204936,1204938,1204982,1205033,1205065,1205082,1205097,1205112,1206200,1207692,1208046,1208073,1208096,1208114,1208145,1208772,1209194,1209277-1209278,1209686-1209731,1210894,1212091,1212095,1212099,1212118,1213469,1213906,1214853,1214855,1214864,1215115,1215118-1215119,1215121,1220293,1220295,1221038,1221842,1222189,101,176,1222300,1222690,1222850,1222852,1222855,1224607,1224617,1224648-1224652,1224657,1224662-1224663,1224682,1224801,1224910,122500

svn ignore for netbeans

[Brian Burch]

When using netbeans as an ide, it will always create a subdirectory 
called nbproject inside the project root directory. That means svn 
status will show this local directory as "?" (not under version control).


There are no circumstances where someone should be allowed to commit the 
nbproject directory or its contents. When I get around to providing 
sample netbeans project files for tomcat, they should be put in 
res/netbeans with instructions on how to deploy them (into the nbproject 
directory) and customise their contents.


Would it be possible to add the nbproject directory name to the 
svn:ignore property of the trunk? I can see you have already done this 
for the log and work directories.


Thanks,

Brian

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

Re: svn commit: r1183612 [2/2] - /tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java

[Brian Burch]

On 18/01/12 08:26, William A. Rowe Jr. wrote:

On 1/13/2012 8:30 PM, Brian Burch wrote:

On 14/01/12 09:45, Filip Hanik - Dev Lists wrote:

No grudge held :)
I'm just bringing it up since very many files have been reformatted for
the sake of formatting. And when tracing down a problem, I, and I
suspect others too, often use SVN history to figure out what and how
changed. So a formatting change that makes satisfies a personal
formatting preference for one developer, down the line can make it a lot
harder to trace changes for others.


I don't want to throw petrol on glowing embers, and I realise my own efforts 
barely count.
However, when I've been researching the history of changes in logic that I 
didn't fully
understand, the svn history has been unhelpful and confusing. Now I understand 
why!


One very useful trick is svn diff -x --ignore-all-whitespace -x 
--ignore-eol-style


Thanks for the tip, Will.

I use Netbeans as my IDE: on the miscellaneous properties tabbed pane 
for the diff command, I already had the checkbox set for "ignore leading 
and trailing whitespace", but didn't have "ignore changes in inner 
whitespace" set.


It is hard to remember exactly what was causing me confusion at the 
time, but that must have been a contributory factor. I suspect seeing 
changes that were "no change" confused me and drove me to the command 
line, where I saw the changes were HUGE and yet for no apparent 
difference - just loads of lines deleted and re-added.


I hope it is already policy that "editorial mass changes" should be 
clearly marked in the commit message text and should not alter any 
logic, signatures, etc.


Brian

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

Re: Detecting failures of unit tests

[Brian Burch]

On 19/01/12 07:12, Christopher Schultz wrote:

All,

I was testing 7.0.25 and "ant test" reports BUILD SUCCESSFUL but I
started looking at the TEST-*.txt files that are emitted and I was
wondering about a few things.

First, I should probably be look at the bottom of the file for the junit
summary that looks like this:

Testsuite: org.apache.tomcat.util.threads.TestLimitLatch
Tests run: 5, Failures: 0, Errors: 0, Time elapsed: 2.545 sec

Observing the "Failures: 0, Errors: 0" indicates that the tests have all
passed correctly, right?

I'm asking because I can also see things like this:

TEST-org.apache.catalina.connector.TestMaxConnections.APR.txt:INFO:
There were [4] passed requests and [2] connection failures

Obviously, that's an INFO line, but it does indicate a "failure" of some
kind.

There are also some log lines like this:

TEST-org.apache.catalina.startup.TestListener.NIO.txt:SEVERE: Context
[/] startup failed due to previous errors

Does that merely indicate that the test itself caused a failure, and
that the failure-to-startup was intentional?

Similarly:

TEST-org.apache.catalina.tribes.group.interceptors.TestNonBlockingCoordinator.APR.txt:org.apache.catalina.tribes.ChannelException:
Send failed, and sender is disconnected. Not retrying.; Faulty
members:tcp://{127, 0, 0, 1}:4005;

Again, all test files say "Failures: 0, Errors: 0" so I guess everything
is okay. It's just tough to see those log lines without asking.

Thanks,
-chris


1. I noticed this kind of message when I was looking for problems in my 
own new tomcat tests. Because they didn't originate from my own tests, I 
didn't follow it up. Sorry...


2. I've seen similar situations in other projects, but I don't know if 
my explanation is relevant to the tomcat tests... in all previous cases, 
my tests have been complex enough to require starting another thread so 
I could have both a client and server active during each individual test 
method of the class.


3. The tomcat tests (at least the ones I have worked on) subclass 
TomcatBaseTest, which starts Tomcat in a new thread, which puts them 
into the category I am talking about.


4. As far as I remember, it is possible for these sub-threads to "fail", 
or even interfere with each-other (clashes for port numbers or other 
shared resources), or even interfere with other completely different 
test classes doing the same kind of thing, while the main thread happily 
passes all its junit assertions and reports "success".


5. Forgive me if this is too vague, or too simplistic to be helpful. 
Your comment reminded me of myself (about 10 years ago) when everyone 
else was happily reading successful junit summaries and I started 
looking at the log files generated by the tests. I found the messages 
(like yours) difficult to understand, and even more difficult to 
diagnose - set a debugger break point in the sub-thread and you will 
screw up the timing of the tests, thus never even arriving at the 
troublesome code. I often had to catch the exception when it was thrown, 
then analyse the call stack - just like the old days of reading core dumps!


6. If you are still reading this, then I'll cheer you up by saying about 
5 in 6 of these "problems" came down to artefacts of the test design, 
rather than bugs in the logic actually under test. Unfortunately, I had 
to debug and fix all of them before I could stop worrying!


I hope this war story has been helpful.

Brian



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

Re: New unit tests for Authenticators and SingleSignOn

[Brian Burch]

I have now finished the second in my set of new unit test classes. This 
one examines the SingleSignOn interactions between the NonLogin and 
Basic Authenticators.


It is consistent with the various changes made to my previously 
submitted test class, 
org.apache.catalina.authenticator.TestNonLoginAndBasicAuthenticator.


The new class passes CheckStyle and runs successfully against the latest 
trunk (At revision 1234681). If someone can spare the time, please 
verify it and then commit it to the trunk. Obviously, it isn't urgent 
because all the tests pass and it verifies the fix to bug 52303 is 
working properly).


Thanks,

Brian



/*
 *  Licensed to the Apache Software Foundation (ASF) under one or more
 *  contributor license agreements.  See the NOTICE file distributed with
 *  this work for additional information regarding copyright ownership.
 *  The ASF licenses this file to You under the Apache License, Version 2.0
 *  (the "License"); you may not use this file except in compliance with
 *  the License.  You may obtain a copy of the License at
 *
 *  http://www.apache.org/licenses/LICENSE-2.0
 *
 *  Unless required by applicable law or agreed to in writing, software
 *  distributed under the License is distributed on an "AS IS" BASIS,
 *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or 
implied.

 *  See the License for the specific language governing permissions and
 *  limitations under the License.
 */
package org.apache.catalina.authenticator;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;

import org.junit.Test;

import org.apache.catalina.Context;
import org.apache.catalina.deploy.LoginConfig;
import org.apache.catalina.deploy.SecurityCollection;
import org.apache.catalina.deploy.SecurityConstraint;
import org.apache.catalina.startup.TesterServlet;
import org.apache.catalina.startup.Tomcat;
import org.apache.catalina.startup.TomcatBaseTest;
import org.apache.catalina.util.Base64;
import org.apache.tomcat.util.buf.ByteChunk;

/**
 * Test BasicAuthenticator and NonLoginAuthenticator when a
 * SingleSignOn Valve is active.
 *
 * 
 * In the absence of SSO support, a webapp using NonLoginAuthenticator
 * simply cannot access protected resources. These tests exercise the
 * the way successfully authenticating a different webapp under the
 * BasicAuthenticator triggers the additional SSO logic for both webapps.
 */
public class TestSSOnonLoginAndBasicAuthenticator extends TomcatBaseTest {

private static final String USER = "user";
private static final String PWD = "pwd";
private static final String ROLE = "role";

private static final String HTTP_PREFIX = "http://localhost:";;
private static final String CONTEXT_PATH_NOLOGIN = "/nologin";
private static final String CONTEXT_PATH_LOGIN = "/login";
private static final String URI_PROTECTED = "/protected";
private static final String URI_PUBLIC = "/anyoneCanAccess";

private static final int SHORT_TIMEOUT_SECS = 4;
private static final long SHORT_TIMEOUT_DELAY_MSECS =
((SHORT_TIMEOUT_SECS + 3) * 1000);
private static final int LONG_TIMEOUT_SECS = 10;
private static final long LONG_TIMEOUT_DELAY_MSECS =
((LONG_TIMEOUT_SECS + 5) * 1000);

private static String CLIENT_AUTH_HEADER = "authorization";
private static String SERVER_COOKIES = "Set-Cookie";
private static String BROWSER_COOKIES = "Cookie";

private List cookies;

/*
 * Try to access an unprotected resource without an established
 * SSO session.
 * This should be permitted.
 */
@Test
public void testAcceptPublicNonLogin() throws Exception {
doTestNonLogin(CONTEXT_PATH_NOLOGIN + URI_PUBLIC,
false, false, 200);
}

/*
 * Try to access a protected resource without an established
 * SSO session.
 * This should be rejected with SC_FORBIDDEN 403 status.
 */
@Test
public void testRejectProtectedNonLogin() throws Exception {
doTestNonLogin(CONTEXT_PATH_NOLOGIN + URI_PROTECTED,
false, true, 403);
}

/*
 * Logon to access a protected resource using BASIC authentication,
 * which will establish an SSO session.
 * Wait until the SSO session times-out, then try to re-access
 * the resource.
 * This should be rejected with SC_FORBIDDEN 401 status, which
 * will then be followed by successful re-authentication.
 */
@Test
public void testBasicLoginSessionTimeout() throws Exception {
doTestBasic(USER, PWD, CONTEXT_PATH_LOGIN + URI_PROTECTED,
true, 401, false, 200);
// wait long enough for my session to expire
Thread.sleep(SHORT_TIMEOUT_DELAY_MSECS);
doTestBasic(USER, PWD, CONTEXT_PATH_LOGI

New unit tests for Authenticators and SingleSignOn

[Brian Burch]

I have finished the third in my series of new unit tests, which examines 
the SingleSignOn interactions between the Digest and NonLogin 
Authenticators. Because each Authenticator class has its own SSO logic, 
I feel it is sensible to test that logic explicitly. However, there is 
no need to duplicate ALL of the authentication logic already tested by 
the "non-SSO" test classes.


The new test class passes CheckStyle and runs successfully against the 
trunk (at revision 1295439).


It is not urgent, but would someone please verify the code and commit 
the new class for me?



/*
 *  Licensed to the Apache Software Foundation (ASF) under one or more
 *  contributor license agreements.  See the NOTICE file distributed with
 *  this work for additional information regarding copyright ownership.
 *  The ASF licenses this file to You under the Apache License, Version 2.0
 *  (the "License"); you may not use this file except in compliance with
 *  the License.  You may obtain a copy of the License at
 *
 *  http://www.apache.org/licenses/LICENSE-2.0
 *
 *  Unless required by applicable law or agreed to in writing, software
 *  distributed under the License is distributed on an "AS IS" BASIS,
 *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or 
implied.

 *  See the License for the specific language governing permissions and
 *  limitations under the License.
 */
package org.apache.catalina.authenticator;

import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;

import org.junit.Test;

import org.apache.catalina.Context;
import org.apache.catalina.deploy.LoginConfig;
import org.apache.catalina.deploy.SecurityCollection;
import org.apache.catalina.deploy.SecurityConstraint;
import org.apache.catalina.startup.TesterServlet;
import org.apache.catalina.startup.Tomcat;
import org.apache.catalina.startup.TomcatBaseTest;
import org.apache.catalina.util.MD5Encoder;
import org.apache.tomcat.util.buf.ByteChunk;

/**
 * Test DigestAuthenticator and NonLoginAuthenticator when a
 * SingleSignOn Valve is active.
 *
 * 
 * In the absence of SSO support, a webapp using NonLoginAuthenticator
 * simply cannot access protected resources. These tests exercise the
 * the way successfully authenticating a different webapp under the
 * DigestAuthenticator triggers the additional SSO logic for both webapps.
 *
 * 
 * Note: these tests are intended to exercise the SSO logic of the
 * Authenticator, but not to comprehensively test all of its logic paths.
 * That is the responsibility of the non-SSO test suite.
 */
public class TestSSOnonLoginAndDigestAuthenticator extends TomcatBaseTest {

private static final String USER = "user";
private static final String PWD = "pwd";
private static final String ROLE = "role";

private static final String HTTP_PREFIX = "http://localhost:";;
private static final String CONTEXT_PATH_NOLOGIN = "/nologin";
private static final String CONTEXT_PATH_DIGEST = "/digest";
private static final String URI_PROTECTED = "/protected";
private static final String URI_PUBLIC = "/anyoneCanAccess";

private static final int SHORT_TIMEOUT_SECS = 4;
private static final long SHORT_TIMEOUT_DELAY_MSECS =
((SHORT_TIMEOUT_SECS + 3) * 1000);
private static final int LONG_TIMEOUT_SECS = 10;
private static final long LONG_TIMEOUT_DELAY_MSECS =
((LONG_TIMEOUT_SECS + 2) * 1000);

private static final String CLIENT_AUTH_HEADER = "authorization";
private static final String OPAQUE = "opaque";
private static final String NONCE = "nonce";
private static final String REALM = "realm";
private static final String CNONCE = "cnonce";

private static String NC1 = "0001";
private static String NC2 = "0002";
private static String QOP = "auth";

private static String SERVER_COOKIES = "Set-Cookie";
private static String BROWSER_COOKIES = "Cookie";

private List cookies;

/**
 * Try to access an unprotected resource without an
 * established SSO session.
 * This should be permitted.
 */
@Test
public void testAcceptPublicNonLogin() throws Exception {
doTestNonLogin(CONTEXT_PATH_NOLOGIN + URI_PUBLIC,
   true, false, 200);
}

/*
 * Try to access a protected resource without an established
 * SSO session.
 * This should be rejected with SC_FORBIDDEN 403 status.
 */
@Test
public void testRejectProtectedNonLogin() throws Exception {
doTestNonLogin(CONTEXT_PATH_NOLOGIN + URI_PROTECTED,
   false, true, 403);
}

/**
 * Logon to access a protected resource using DIGEST authentication,
 * which will establish an SSO session.
  

Re: svn commit: r1297158 - in /tomcat/trunk/test/org/apache/catalina/authenticator: TestDigestAuthenticator.java TestNonLoginAndBasicAuthenticator.java TestSSOnonLoginAndBasicAuthenticator.java

[Brian Burch]

On 05/03/12 18:45, ma...@apache.org wrote:

Author: markt
Date: Mon Mar  5 18:45:27 2012
New Revision: 1297158

URL: http://svn.apache.org/viewvc?rev=1297158&view=rev
Log:
Fix some test failures now response bodies are available for error
responses.

Modified:
 
tomcat/trunk/test/org/apache/catalina/authenticator/TestDigestAuthenticator.java
 
tomcat/trunk/test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java
 
tomcat/trunk/test/org/apache/catalina/authenticator/TestSSOnonLoginAndBasicAuthenticator.java




My proposed new test class has not yet been committed:

org.apache.catalina.authenticator.TestSSOnonLoginAndDigestAuthenticator

I have already updated it to make it consistent with Mark's changes (above).


However, this new version of my test is failing in a peculiar manner and 
I am currently investigating the problem.


I will make any further comments on the original thread "New unit tests 
for Authenticators and SingleSignOn" in the hope of avoiding wasted 
effort or confusion.


Brian

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

Re: svn commit: r1297158 - in /tomcat/trunk/test/org/apache/catalina/authenticator: TestDigestAuthenticator.java TestNonLoginAndBasicAuthenticator.java TestSSOnonLoginAndBasicAuthenticator.java

[Brian Burch]

On 06/03/12 14:58, Konstantin Kolinko wrote:

2012/3/6 Brian Burch:

However, this new version of my test is failing in a peculiar manner and I
am currently investigating the problem.

I will make any further comments on the original thread "New unit tests for
Authenticators and SingleSignOn" in the hope of avoiding wasted effort or
confusion.


It is better to propose new code and parches through Bugzilla.
That way there are lesser chances that it is lost in the flood of e-mails.

Best regards,
Konstantin Kolinko


Thanks for your advice, Konstantin. I hadn't thought of bugzilla because 
I didn't think it was appropriate for new unit test code. Now that I 
have rtfm'd, I see that I was wrong.


Just for the record, the "failing in a peculiar manner" was my fault, I 
didn't spot the crucial bc.recycle() Mark had added prior to reusing the 
ByteChunk in the second getUrl call.


Brian

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

Re: svn commit: r1303338 - /tomcat/trunk/java/org/apache/catalina/realm/RealmBase.java

[Brian Burch]

On 21/03/12 10:00, ma...@apache.org wrote:

Author: markt
Date: Wed Mar 21 10:00:52 2012
New Revision: 1303338

URL: http://svn.apache.org/viewvc?rev=1303338&view=rev
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=52953
When using DIGEST auth, digests are always represented using lower case hex 
characters


I realise this particular change is trivial, but because I hadn't 
updated my sandbox of the trunk for a couple of weeks, I decided to add 
a new unit test for bug 52954 in:


org.apache.catalina.authenticator.TestDigestAuthenticator.

No-one should be surprised to hear the new test case is currently 
failing on my system with 401 status - it simply confirms the bug exists 
in my sandbox:


Last Changed Author: markt
Last Changed Rev: 1297213
Last Changed Date: 2012-03-05 20:20:00 + (Mon, 05 Mar 2012)


I'm not in a hurry to update my sandbox because I am confident svn 
commit r1303338 fixes the reported bug.


However, this test class hasn't changed for quite a long time, so before 
I submit my change, I intend to look for any other corner-cases that 
might also have been missed. If anyone else is planning to do similar 
work, please let me know so that I don't waste my time!


Regards,

Brian


Modified:
 tomcat/trunk/java/org/apache/catalina/realm/RealmBase.java

Modified: tomcat/trunk/java/org/apache/catalina/realm/RealmBase.java
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/realm/RealmBase.java?rev=1303338&r1=1303337&r2=1303338&view=diff
==
--- tomcat/trunk/java/org/apache/catalina/realm/RealmBase.java (original)
+++ tomcat/trunk/java/org/apache/catalina/realm/RealmBase.java Wed Mar 21 
10:00:52 2012
@@ -27,6 +27,7 @@ import java.security.NoSuchAlgorithmExce
  import java.security.Principal;
  import java.security.cert.X509Certificate;
  import java.util.ArrayList;
+import java.util.Locale;

  import javax.servlet.http.HttpServletResponse;

@@ -381,7 +382,8 @@ public abstract class RealmBase extends
String qop, String realm,
String md5a2) {

-String md5a1 = getDigest(username, realm);
+// In digest auth, digests are always lower case
+String md5a1 = getDigest(username, realm).toLowerCase(Locale.ENGLISH);
  if (md5a1 == null)
  return null;
  String serverDigestValue;



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



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

Re: svn commit: r1303339 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/realm/RealmBase.java webapps/docs/changelog.xml

[Brian Burch]

Sorry I haven't been able to quote the details of this commit made by 
markt a month ago, but I didn't keep a copy in my inbox.


I previously submitted an enhancement to the corresponding test suite
https://issues.apache.org/bugzilla/show_bug.cgi?id=53096
I fully expected all my test cases would succeed against mark's trivial 
bugfix.


I recently brought my trunk sandbox up to svn: r1329909 and was puzzled 
to discover the relevant test case still FAILED!


I've done a lot of digging and debugging, and come to the conclusion 
this problem is more subtle than originally thought. Does anyone know 
whether the current fix has been validated against a real android 
device? I suspect it doesn't work.


The issues are:

1. the one line change:
-String md5a1 = getDigest(username, realm);
+// In digest auth, digests are always lower case
+String md5a1 = getDigest(username, 
realm).toLowerCase(Locale.ENGLISH);


If I remember correctly, the intention was to be make tomcat more 
tolerant of clients that presented digest strings with upper case 
hexadecimal digits provided they were otherwise correct. The change in 
r1329909 seems to me as if it does nothing of relevance to that objective.


2. Client digest responses require an outer hash which will wrap one or 
two inner hashes... /if/ the inner hashes are (incorrectly) represented 
with upper case hex digits, then the outer hash will inevitably be 
different to the (correct situation) where they are represented with 
lower case hex digits. This difference is independent of whether the 
HTTP Digest header delivers its hashes using lower or upper case hex digits.


3. I have reworked the TestDigestAuthenticator class to explore these 
alternatives. I have also drafted a change to RealmBase that "fixes" 
situation 1 above, but it still fails with both cases of situation 2.


4. My suspicion is that /if/ the android digest algorithm (incorrectly) 
uses upper case hex digits with its outer hashes, it will probably also 
do the same with its inner hashes. If I am right, the two failing tests 
in situation 3 will not work until RealmBase has even more compensatory 
logic.


I can explain and explore this in more detail, but I need a strategic 
decision first: do we back out the change and go to wontfix (i.e. wont 
be tolerant), or do we proceed with trying to be flexible? There must be 
a lot of androids out there carrying the "non standard" DIGEST logic 
(whatever that might really be).


Regards,

Brian

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

Re: svn commit: r1303339 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/realm/RealmBase.java webapps/docs/changelog.xml

[Brian Burch]

On 24/04/12 21:34, Mark Thomas wrote:

On 24/04/2012 21:11, Mark Thomas wrote:

On 24/04/2012 20:51, Brian Burch wrote:

Sorry I haven't been able to quote the details of this commit made by
markt a month ago, but I didn't keep a copy in my inbox.

I previously submitted an enhancement to the corresponding test suite
https://issues.apache.org/bugzilla/show_bug.cgi?id=53096
I fully expected all my test cases would succeed against mark's trivial
bugfix.

I recently brought my trunk sandbox up to svn: r1329909 and was puzzled
to discover the relevant test case still FAILED!

I've done a lot of digging and debugging, and come to the conclusion
this problem is more subtle than originally thought. Does anyone know
whether the current fix has been validated against a real android
device? I suspect it doesn't work.


I certainly didn't at the time, but I can quite easily.


The issues are:

1. the one line change:
-String md5a1 = getDigest(username, realm);
+// In digest auth, digests are always lower case
+String md5a1 = getDigest(username,
realm).toLowerCase(Locale.ENGLISH);

If I remember correctly, the intention was to be make tomcat more
tolerant of clients that presented digest strings with upper case
hexadecimal digits provided they were otherwise correct. The change in
r1329909 seems to me as if it does nothing of relevance to that objective.


Agreed.

(if that was the objective).

However, that was not the objective. The objective was to handle
programs that created hashes for the user database that used capitals.

The Android DIGEST auth issue was related to URIs. See
https://issues.apache.org/bugzilla/show_bug.cgi?id=52954


Let me see what happens with 2.3.5 and 4.0.3 and decide then.

Watch this space...


BZ 52954 is not an issue in Android 4.0.3 but it is in 2.3.5. Given that
2.3.x is by far the most prevalent Android version at the moment we
should certainly take a look at fixing BZ 52954.

Upper/lower case digests from android clients is a red herring.


I don't understand exactly what you mean by that. Didn't the original 
report say android http clients were sending HTTP Digest hex strings 
with upper case A-F, or was I just dreaming?


r1329909 implied it was trying to resolve that issue, even if it was 
also resolving others. Unfortunately, r1329909 pointlessly manipulates a 
hex digest string that is already certain to contain lower case hex digits.


For what it is worth, here is my patch to achieve the "apparently 
desired" objective (my "situation 1"). You'll see that it also cleans up 
a debug parameter list by a) removing duplication of clientDigest and b) 
fixing a typo in a field label.



Index: java/org/apache/catalina/realm/RealmBase.java
===
--- java/org/apache/catalina/realm/RealmBase.java   (revision 1330187)
+++ java/org/apache/catalina/realm/RealmBase.java   (working copy)
@@ -383,7 +383,7 @@
   String md5a2) {

 // In digest auth, digests are always lower case
-String md5a1 = getDigest(username, 
realm).toLowerCase(Locale.ENGLISH);

+String md5a1 = getDigest(username, realm);
 if (md5a1 == null)
 return null;
 String serverDigestValue;
@@ -409,14 +409,14 @@
 }

 if (log.isDebugEnabled()) {
-log.debug("Digest : " + clientDigest + " Username:" + username
-+ " ClientSigest:" + clientDigest + " nonce:" + nonce
+log.debug("ClientDigest: " + clientDigest +
+" Username:" + username + " nonce:" + nonce
 + " nc:" + nc + " cnonce:" + cnonce + " qop:" + qop
 + " realm:" + realm + "md5a2:" + md5a2
-+ " Server digest:" + serverDigest);
++ " ServerDigest:" + serverDigest);
 }

-if (serverDigest.equals(clientDigest)) {
+if (serverDigest.equalsIgnoreCase(clientDigest)) {
 return getPrincipal(username);
 }

I hope I'm not treading on your toes!

BTW I have a new version of the unit test class that examines all of my 
"situations", but I'll hang onto it until you have clarified the real 
problem - obviously we don't need any failing test cases for situations 
we don't intend to support!


Regards,

Brian

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

Re: svn commit: r1303339 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/realm/RealmBase.java webapps/docs/changelog.xml

[Brian Burch]

On 25/04/12 12:32, Mark Thomas wrote:

On 25/04/2012 12:03, Brian Burch wrote:

On 24/04/12 21:34, Mark Thomas wrote:

On 24/04/2012 21:11, Mark Thomas wrote:

On 24/04/2012 20:51, Brian Burch wrote:

Sorry I haven't been able to quote the details of this commit made by
markt a month ago, but I didn't keep a copy in my inbox.

I previously submitted an enhancement to the corresponding test suite
https://issues.apache.org/bugzilla/show_bug.cgi?id=53096
I fully expected all my test cases would succeed against mark's trivial
bugfix.

I recently brought my trunk sandbox up to svn: r1329909 and was puzzled
to discover the relevant test case still FAILED!

I've done a lot of digging and debugging, and come to the conclusion
this problem is more subtle than originally thought. Does anyone know
whether the current fix has been validated against a real android
device? I suspect it doesn't work.


I certainly didn't at the time, but I can quite easily.


The issues are:

1. the one line change:
-String md5a1 = getDigest(username, realm);
+// In digest auth, digests are always lower case
+String md5a1 = getDigest(username,
realm).toLowerCase(Locale.ENGLISH);

If I remember correctly, the intention was to be make tomcat more
tolerant of clients that presented digest strings with upper case
hexadecimal digits provided they were otherwise correct. The change in
r1329909 seems to me as if it does nothing of relevance to that
objective.


Agreed.

(if that was the objective).

However, that was not the objective. The objective was to handle
programs that created hashes for the user database that used capitals.

The Android DIGEST auth issue was related to URIs. See
https://issues.apache.org/bugzilla/show_bug.cgi?id=52954


Let me see what happens with 2.3.5 and 4.0.3 and decide then.

Watch this space...


BZ 52954 is not an issue in Android 4.0.3 but it is in 2.3.5. Given that
2.3.x is by far the most prevalent Android version at the moment we
should certainly take a look at fixing BZ 52954.

Upper/lower case digests from android clients is a red herring.


I don't understand exactly what you mean by that. Didn't the original
report say android http clients were sending HTTP Digest hex strings
with upper case A-F, or was I just dreaming?


You were dreaming. Go read BZ 52964.


Not quite dreaming... it seems I created an imaginary cocktail of two bugs:-

BZ 52953  Unlike BASIC Authentication, DIGEST mode does not work if the 
hash is stored in uppercase


and

BZ 52954  Allowing for broken android HTTP DIGEST support

I thought (wrongly) they were both referring to android clients.


r1329909 implied it was trying to resolve that issue, even if it was
also resolving others. Unfortunately, r1329909 pointlessly manipulates a
hex digest string that is already certain to contain lower case hex digits.


No it didn't. And that isn't the right revision number either. We are
discussing this:
http://svn.apache.org/viewvc?view=revision&revision=r1303339


Careless of me to imply the change in question was the latest at the 
time I did my update. I'm glad you were paying attention and I hope I 
didn't waste any of your time.


It now makes sense to me why your r1303339 hit the server-side 
recreation of the expected client digest. You were not addressing the 
case where an upper-case hex digit had arrived from an http client. I 
apologise for muddying the water.



For what it is worth, here is my patch to achieve the "apparently
desired" objective (my "situation 1"). You'll see that it also cleans up
a debug parameter list by a) removing duplication of clientDigest and b)
fixing a typo in a field label.

That isn't the problem. Android has no problems sending digests in lower
case s required by the spec.


My suggested patch had three objectives (I assumed you would break it 
into smaller commits):


1. Would you please clean up the debug message parameters (as proposed 
in my diff)? It seems overkill to open a new bug for this trivial change.


2. In the case where an http client has sent an invalid digest string 
(anything that is not a lower case hex digit) rfc2617 seems to me to 
mandate http status 400 (syntax error, do not retry), but my tests show 
tomcat is sending 401 (unauthorized) when I inject "A" (or even "G") 
into the digest string. That looks like a new bug to me - what do you think?


3. My revised opinion is that r1303339 is effective, but tackles the 
user database problem too far down the track. RealmBase.getDigest(String 
username, String realmName) has two cases. When a cleartext password is 
available it can be trusted to return an rfc-compliant digest from 
org.apache.catalina.util.MD5Encoder.encode(). However, if the concrete 
implementation of getPassword returns a pre-digested password, getDigest 
should not simply trust the returned String to be rfc-compliant. I t

Re: [Bug 53601] tomcat7 build fails with jdk1.6

[Brian Burch]

On 25/07/12 12:55, bugzi...@apache.org wrote:

https://issues.apache.org/bugzilla/show_bug.cgi?id=53601

Konstantin Kolinko  changed:

What|Removed |Added

  Status|NEW |RESOLVED
  Resolution|--- |INVALID
  OS||All

--- Comment #1 from Konstantin Kolinko  ---
You are wrong.

1). It is true that Tomcat trunk (aka Tomcat 8) builds only with jdk1.7.
It is as intended. It is expected to implement the Servlet 3.1 specification.

2). Tomcat 7 builds with jdk1.6 and this status quo has not changed.

Anyway your report noticeably lack of details. You should ask on the mailing
list first, before playing around with Bugzilla.


I thought I would step through the latest trunk BUILDING.TXT 
instructions because I haven't updated my sandbox for at least a month.



Apache Ant(TM) version 1.8.2 compiled on December 3 2011
Trying the default build file: build.xml
Buildfile: /home/brian/sandboxApache/tomcat8/trunk/build.xml
Detected Java version: 1.6 in: /usr/lib/jvm/java-6-sun-1.6.0.26/jre
Detected OS: Linux

compile:
[javac] Compiling 307 source files to 
/home/brian/sandboxApache/tomcat8/trunk/output/classes

[javac] javac: invalid target release: 1.7
[javac] Usage: javac  
[javac] use -help for a list of possible options

BUILD FAILED

I wondered whether we still need to tell people how to get a JDK...
if they haven't got one already, and aren't familiar with building 
complex projects, they probably shouldn't be encouraged to start 
building tomcat from the trunk! In the end, I thought the best 
compromise would be to just reference a couple of popular JDKs - we 
don't want to seem partisan, but equally don't want to get dragged into 
a debate about which is "best".


It might be helpful to list the JDKs that are used to build and test the 
production releases, but I don't know what they are.


Here is the trivial change to avoid this kind of confusion for the moment!

Index: BUILDING.txt
===
--- BUILDING.txt(revision 1366303)
+++ BUILDING.txt(working copy)
@@ -37,9 +37,10 @@

  1. If the JDK is already installed, skip to (2).

- 2. Download a Java Development Kit (JDK) release (version 1.6.x or later)
-from:
-
+ 2. Download a Java Development Kit (JDK) release (version 1.7.x or later)
+e.g.
+http://openjdk.java.net/install/index.html
+or
 http://www.oracle.com/technetwork/java/javase/downloads/index.html

  3. Install the JDK according to the instructions included with the 
release.



I hope my contribution is both simple and helpful.

Brian





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

Re: [Bug 53601] tomcat7 build fails with jdk1.6

[Brian Burch]

On 07/08/12 22:46, bugzi...@apache.org wrote:

https://issues.apache.org/bugzilla/show_bug.cgi?id=53601

Mark Thomas  changed:

What|Removed |Added

  Status|REOPENED|RESOLVED
  Resolution|--- |INVALID

--- Comment #4 from Mark Thomas  ---
trunk != 7.0.x/trunk hence closing this as INVALID. Maybe we need to start an
8.0.x BZ project for 8.0.x specific issues.


That is correct. I used the bugzilla web interface and discovered that 
it would not allow me to post against the latest trunk (aka 8.0), only 
against 7.0. I decided the most helpful action was to reopen the 
original (misdirected) 7.0 bug report.


There really should be an 8.0 option, otherwise we run the risk of 
further (possibly more significant) confusion in future.



I have updated the building instructions for trunk and also included a
reference to OpenJDK and other JDK vendors although on this occasion I did not
use the provided patch.


The new wording is fine with me. I only provided a patch in case the 
developers were all busy and would have preferred a "proposed default" 
wording.


BTW my patch was against a 8.0 trunk revision number, so that should 
have disambiguated it. The specific "bug" and change were obviously 
trivial, but the meta-topic is not. I hope this follow up does not cause 
offence - I'm just trying to be helpful.



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

Re: [Bug 53584] Forms authentication without cookies requires double submission in 6.0.33

[Brian Burch]

On 07/08/12 22:33, bugzi...@apache.org wrote:

https://issues.apache.org/bugzilla/show_bug.cgi?id=53584

Mark Thomas  changed:

What|Removed |Added

  OS||All

--- Comment #1 from Mark Thomas  ---
Thanks for an excellent bug report. The issue was a real pleasure to
investigate - not just because the root cause was interesting but because I
could focus on the interesting bits rather than having to waste time trying to
build the test WAR using the current flavour of the month for scm and/or build
tool and/or source layout. Simple WARs are *SO* much easier to work with.

The clear steps to re-create the issue were also extremely helpful. So again,
thank-you.

The root cause is that as of 6.0.33 path parameters are included the value
returned from HttpServletRequest.getRequestURI(). During the FORM auth, one of
the checks post authentication is "Does the current URI equal the original
URI?" The problem is that the current URI always contains the session ID as a
path parameter whereas the first time through the authentication the original
URI does not.

This issue also affects trunk and 7.0.x.

I have fixed this issue in trunk and 7.0.x for 7.0.30 onwards and proposed the
fix for 6.0.x.


Mark,

I have intermittently observed a similar problem with tc6 and tc7 over 
the last couple of years. It has been on my own list of things to 
investigate, but so far my various efforts haven't allowed me to 
reproduce it on demand and analyse it fully.


Bug 53584 deals with the case where the session id is not transmitted in 
a cookie, but my situation does use cookies. Reading about your 
investigation and solution suggests to me that the underlying problem is 
not directly related to the "no cookie" case, but you mention a mismatch 
in the uri as the underlying cause. I feel it is possible that my own 
case also involves a uri mismatch, but I need to understand this 
particular bug and its fix before I can decide.


Even though it is fixed, I would like to write a unit test case for this 
particular bug. Once done, I can use it as a template to simulate my own 
situation and see whether that has been fixed too.


I would like to start developing the first test soon, but you could save 
me some time. Based on your current understanding, would you mind 
outlining the minimal conditions needed to trigger this particular 
failure case?


Thanks,

Brian


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

Re: [Bug 53584] Forms authentication without cookies requires double submission in 6.0.33

[Brian Burch]

On 14/08/12 16:58, Mark Thomas wrote:

Brian Burch  wrote:


On 07/08/12 22:33, bugzi...@apache.org wrote:

https://issues.apache.org/bugzilla/show_bug.cgi?id=53584

Mark Thomas  changed:

 What|Removed |Added




   OS||All

--- Comment #1 from Mark Thomas  ---
Thanks for an excellent bug report. The issue was a real pleasure to
investigate - not just because the root cause was interesting but

because I

could focus on the interesting bits rather than having to waste time

trying to

build the test WAR using the current flavour of the month for scm

and/or build

tool and/or source layout. Simple WARs are *SO* much easier to work

with.


The clear steps to re-create the issue were also extremely helpful.

So again,

thank-you.

The root cause is that as of 6.0.33 path parameters are included the

value

returned from HttpServletRequest.getRequestURI(). During the FORM

auth, one of

the checks post authentication is "Does the current URI equal the

original

URI?" The problem is that the current URI always contains the session

ID as a

path parameter whereas the first time through the authentication the

original

URI does not.

This issue also affects trunk and 7.0.x.

I have fixed this issue in trunk and 7.0.x for 7.0.30 onwards and

proposed the

fix for 6.0.x.


Mark,

I have intermittently observed a similar problem with tc6 and tc7 over
the last couple of years. It has been on my own list of things to
investigate, but so far my various efforts haven't allowed me to
reproduce it on demand and analyse it fully.

Bug 53584 deals with the case where the session id is not transmitted
in
a cookie, but my situation does use cookies. Reading about your
investigation and solution suggests to me that the underlying problem
is
not directly related to the "no cookie" case, but you mention a
mismatch
in the uri as the underlying cause. I feel it is possible that my own
case also involves a uri mismatch, but I need to understand this
particular bug and its fix before I can decide.

Even though it is fixed, I would like to write a unit test case for
this
particular bug. Once done, I can use it as a template to simulate my
own
situation and see whether that has been fixed too.

I would like to start developing the first test soon, but you could
save
me some time. Based on your current understanding, would you mind
outlining the minimal conditions needed to trigger this particular
failure case?

Thanks,

Brian


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


Steps to reproduce are provided in the bug report. The root cause was that the 
redirect after authentication contained a path parameter (the session id) which 
was not present in the saved request hence the URIs did not match.

I don't see any way this could occur when cookies are used unless something in 
the request path is injecting path parameters into the URI.


Thanks for your opinions. I'll get on with the new test - it will be 
useful to avoid regression. (I extrapolate from your comment that my own 
problem might turn out to be an error in the configuration or user code).



Mark

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




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

FormAuthenticatorTest for cases without cookies - implementation issues

[Brian Burch]

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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: FormAuthenticatorTest for cases without cookies - implementation issues

[Brian Burch]

On 30/09/12 21:56, Konstantin Kolinko wrote:

2012/9/26 Brian Burch :

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?



1. If you a set of changes (a, b, c) and you cannot separate them into
distinct patches,  I would prefer to see (a), (a+b) and (a+b+c).

Seeing (a+b+c) is usually also good enough if you can explain the
changes such that a committer would be able to separate them.


Thanks for thinking about my worries, Konstantin. I appreciate you 
spending time ana

Re: FormAuthenticatorTest for cases without cookies - implementation issues

[Brian Burch]

On 30/09/12 23:46, Brian Burch wrote:

On 30/09/12 21:56, Konstantin Kolinko wrote:

2012/9/26 Brian Burch :

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?



1. If you a set of changes (a, b, c) and you cannot separate them into
distinct patches,  I would prefer to see (a), (a+b) and (a+b+c).

Seeing (a+b+c) is usually also good enough if you can explain the
changes such that a committer would be able to separate them.


Thanks for thinking about my worries, Ko

Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session timeout properly

[Brian Burch]

On 22/11/12 15:17, bugzi...@apache.org wrote:

https://issues.apache.org/bugzilla/show_bug.cgi?id=54190

--- Comment #2 from Mark Thomas  ---
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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session timeout properly

[Brian Burch]

On 22/11/12 16:46, bugzi...@apache.org wrote:

https://issues.apache.org/bugzilla/show_bug.cgi?id=54190

--- Comment #3 from Mark Thomas  ---
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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session timeout properly

[Brian Burch]

On 22/11/12 18:39, Mark Thomas wrote:

On 22/11/2012 17:32, Brian Burch wrote:

On 22/11/12 16:46, bugzi...@apache.org wrote:

https://issues.apache.org/bugzilla/show_bug.cgi?id=54190

--- Comment #3 from Mark Thomas  ---
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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org




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

Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session timeout properly

[Brian Burch]

On 29/11/12 14:37, bugzi...@apache.org wrote:

https://issues.apache.org/bugzilla/show_bug.cgi?id=54190

Mark Thomas  changed:

What|Removed |Added

  Status|NEW |RESOLVED
  Resolution|--- |FIXED

--- Comment #6 from Mark Thomas  ---
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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session timeout properly

[Brian Burch]

On 29/11/12 16:46, Rainer Jung wrote:

On 29.11.2012 17:11, Brian Burch wrote:

On 29/11/12 14:37, bugzi...@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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org




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

Re: svn commit: r1415184 - in /tomcat/tc7.0.x/trunk: ./ test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java webapps/docs/changelog.xml

[Brian Burch]

On 02/12/12 22:00, Konstantin Kolinko wrote:

2012/11/29  :

Author: markt
Date: Thu Nov 29 14:35:02 2012
New Revision: 1415184

URL: http://svn.apache.org/viewvc?rev=1415184&view=rev
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
Improve unit tests.
Patch by Brian Burch.

Modified:
 tomcat/tc7.0.x/trunk/   (props changed)
 
tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java
 tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml

Propchange: tomcat/tc7.0.x/trunk/
--
   Merged /tomcat/trunk:r1415177-1415179




+private static final int SHORT_TIMEOUT_MINS = 1;
+private static final int LONG_TIMEOUT_MINS = 2;
+private static final int MANAGER_SCAN_DELAY_SECS = 60;
+private static final int EXTRA_DELAY_SECS = 5;
+private static final long TIMEOUT_DELAY_MSECS =
+(((SHORT_TIMEOUT_MINS * 60)
++ MANAGER_SCAN_DELAY_SECS + EXTRA_DELAY_SECS) * 1000);


...


+// allow the session to time out and lose authentication
+Thread.sleep(TIMEOUT_DELAY_MSECS);



According to Buildbot logs, testBasicLoginSessionTimeout() runs for 2
minutes (125 seconds), mainly due to a sleep() call.

I wish there were a way to speed up this test.

My thoughts
a) Maybe use reflection to reduce StandardSession.lastAccessedTime and
thisAccessedTime by some fixed amount (6) instead of waiting for
that time to pass.


That would speed up the test... but it sounds like adding a time machine 
to the test class! My feeling is this would add inappropriate logical 
complexity to a test that has always created and destroyed a tomcat 
instance for each test case (there are now 15).


However, I intend to replicate most of the improvements from this test 
class into the other authenticator tests, so I am already apprehensive 
about adding too many 60+ second delays to the entire suite.


Mark and I briefly discussed adding a new protected method for use by 
these timeout tests:


org.apache.catalina.session.StandardSession.setSessionTimeoutSecs(int secs)

What do you think?


b) Maybe call ManagerBase.setProcessExpiresFrequency(1) instead of the
default value (6)  and reduce MANAGER_SCAN_DELAY_SECS from 60 to 10.


Mark and I discussed this idea earlier, but I was reluctant to increase 
complexity in an already radical change to the original.


Your suggestion showed me how to achieve faster detection in a fairly 
simple manner, so I have reopened the bug and provided a new patch that 
shaves 50 seconds off the run time of the timeout test. Thanks!



Best regards,
Konstantin Kolinko

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




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

Re: svn commit: r1415184 - in /tomcat/tc7.0.x/trunk: ./ test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java webapps/docs/changelog.xml

[Brian Burch]

On 03/12/12 11:44, Brian Burch wrote:

On 02/12/12 22:00, Konstantin Kolinko wrote:



According to Buildbot logs, testBasicLoginSessionTimeout() runs for 2
minutes (125 seconds), mainly due to a sleep() call.

I wish there were a way to speed up this test.

My thoughts
a) Maybe use reflection to reduce StandardSession.lastAccessedTime and
thisAccessedTime by some fixed amount (6) instead of waiting for
that time to pass.


That would speed up the test... but it sounds like adding a time machine
to the test class! My feeling is this would add inappropriate logical
complexity to a test that has always created and destroyed a tomcat
instance for each test case (there are now 15).

However, I intend to replicate most of the improvements from this test
class into the other authenticator tests, so I am already apprehensive
about adding too many 60+ second delays to the entire suite.

Mark and I briefly discussed adding a new protected method for use by
these timeout tests:

org.apache.catalina.session.StandardSession.setSessionTimeoutSecs(int secs)


Silly me! Sorry if that left you wondering what I meant. That particular 
method already exists, is public, and is called setSessionTimeout.


What I meant to suggest was this:

org.apache.catalina.core.StandardContext.setSessionTimeoutSecs(int timeout)

because it would be much simpler to modify the Context in the test 
setUp, rather than each individual Session.



What do you think?






Best regards,
Konstantin Kolinko

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




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



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

Re: svn commit: r1415184 - in /tomcat/tc7.0.x/trunk: ./ test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java webapps/docs/changelog.xml

[Brian Burch]

On 03/12/12 16:22, Brian Burch wrote:

On 03/12/12 11:44, Brian Burch wrote:

On 02/12/12 22:00, Konstantin Kolinko wrote:



According to Buildbot logs, testBasicLoginSessionTimeout() runs for 2
minutes (125 seconds), mainly due to a sleep() call.

I wish there were a way to speed up this test.

My thoughts
a) Maybe use reflection to reduce StandardSession.lastAccessedTime and
thisAccessedTime by some fixed amount (6) instead of waiting for
that time to pass.


That would speed up the test... but it sounds like adding a time machine
to the test class! My feeling is this would add inappropriate logical
complexity to a test that has always created and destroyed a tomcat
instance for each test case (there are now 15).

However, I intend to replicate most of the improvements from this test
class into the other authenticator tests, so I am already apprehensive
about adding too many 60+ second delays to the entire suite.

Mark and I briefly discussed adding a new protected method for use by
these timeout tests:

org.apache.catalina.session.StandardSession.setSessionTimeoutSecs(int
secs)


Silly me! Sorry if that left you wondering what I meant. That particular
method already exists, is public, and is called setSessionTimeout.

What I meant to suggest was this:

org.apache.catalina.core.StandardContext.setSessionTimeoutSecs(int timeout)

because it would be much simpler to modify the Context in the test
setUp, rather than each individual Session.


What do you think?


The change seemed almost trivial to me, but I couldn't work out how to 
implement it without changing the public api, i.e.


I have only found one place where it is used:

org.apache.catalina.session.ManagerBase.setContext(Context context)

has this logic:

// Register with the new Context (if any)
if (this.context != null) {
setMaxInactiveInterval(this.context.getSessionTimeout() * 60);
this.context.addPropertyChangeListener(this);
}

I wanted to move the "60" back into StandardContext, but that doesn't 
work because it means changing the org.apache.catalina.Context api to 
work in seconds instead of minutes. ManagerBase and StandardContext are 
in different packages, so a protected accessor for the timeout in 
seconds wouldn't work either.


On the other hand, I wondered about defining an alternative timeout in 
seconds within the ManagerBase, which would only be used by unit tests. 
ManagerBase would use its own timeout if non-zero, otherwise use the 
Context value in minutes.


I would really appreciate some guidance on the best way to proceed. 
(Perhaps it is better to keep things simple by living with an 80 seconds 
test time?)







Best regards,
Konstantin Kolinko

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




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



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



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

Re: [VOTE] Release Apache Tomcat 8.0.3

[Brian Burch]

On 7.2.2014 19:16, Mark Thomas wrote:

The proposed Apache Tomcat 8.0.3 release is now available for voting.

 ...

The proposed 8.0.3 release is:
[ ] Broken - do not release
[ ] Alpha  - go ahead and release as 8.0.3 (alpha)
[ ] Beta   - go ahead and release as 8.0.3 (beta)
[X] Stable - go ahead and release as 8.0.3 (stable)


Ubuntu 13.04 32-bit,
java version "1.7.0_51"
OpenJDK Runtime Environment (IcedTea 2.4.4) (7u51-2.4.4-0ubuntu0.13.10.1)
OpenJDK Server VM (build 24.45-b08, mixed mode)

All tests run successfully with NIO and BIO.

Congratulations!

Brian

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

Tomcat Wiki Source Code Download

[Brian Burch]

With reference to my (OP) Users mailing list thread with exactly the 
same title as this...


I have attached an svn diff for a minor change to the README.txt in the 
root directory of the site svn repository:-


https://svn.apache.org/repos/asf/tomcat/site/trunk

Please feel free to change it if you think my wording could be improved.

Once committed, I intend to make a corresponding change to the more 
important web page which points to the site repository, i.e.


http://tomcat.apache.org/source.html

I hope this represents a small improvement,

Brian
Index: README.txt
===
--- README.txt  (revision 1875534)
+++ README.txt  (working copy)
@@ -4,13 +4,24 @@
 ---
 
 ***NOTE***
-DO NOT EDIT THE .html files in the docs directory.
-Please follow the directions below for updating the website.
+You might be puzzled when you find this checkout does not contain the xml or 
html
+file or files you were hoping to update. This is because the external apache 
Tomcat
+web site appears to be a single entity, but it is constructed from several 
distinct
+segments.
+
+Each major version of Tomcat ships with, and is capable of hosting its own
+documentation on its own local web site. Therefore, the source files for those 
web
+pages can be found in the xdocs subdirectory of the specific version of Tomcat.
 ***NOTE***
 
 The Tomcat web site is based on .xml files which are transformed
 into .html files using XSLT and Ant.
 
+***NOTE***
+DO NOT EDIT THE .html files in the docs directory!
+Please follow the directions below for updating the website.
+***NOTE***
+
 In order to make modifications to the Tomcat web site, you need to first check 
out
 the Tomcat site from SVN. To check out the Tomcat site into a sub-directory
 called tomcat-site in the current directory:


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

Re: [Tomcat Wiki] Update of "FAQ/Linux_Unix" by KonstantinKolinko

[Brian Burch]

On 03/03/14 11:16, Emmanuel Bourg wrote:

Le 03/03/2014 11:21, Apache Wiki a écrit :


   That is because each of these packages distributes the files of Tomcat
   in different places on the disk, sets different environment variables,
   sets different links from one directory to the other in the filesystem, etc..
+ Moreover, some of those packages are notably outdated.


But some people try hard to make them current ! :)


That is good to hear!


Tomcat 7.0.52 is in Debian unstable [1] and I plan to backport it to the
stable distribution which is stuck with 7.0.28 + security fixes. The
package for Tomcat 8 is ready [2] and will be uploaded once EasyMock has
been upgraded.


I like the way the debian/ubuntu package installs, but I moved away from 
7.0.28 a very long time ago. My present strategy is to build my 
preferred version (currently 7.0.52) from source, then deploy it to 
replace the 7.0.28 specific content. Using CATALINA_HOME and 
CATALINA_BASE as recommended by the tc-devs certainly makes this 
approach less painful.


As you will have noticed already, the main responders on the tc-users 
list have learned the hard way that it is difficult to help people who 
are stuck on an obsolete version because they took the "safe and easy" 
option to install their O/S distribution maintainer's "stable" package.


If you need some help (perhaps QA testing), please let me know.

Thanks for the work so far, and the good intentions!

Brian


Emmanuel Bourg

[1] https://packages.debian.org/sid/tomcat7
[2] http://anonscm.debian.org/gitweb/?p=pkg-java/tomcat8.git


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




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

Re: [Tomcat Wiki] Update of "FAQ/Linux_Unix" by KonstantinKolinko

[Brian Burch]

On 17/03/14 13:13, Emmanuel Bourg wrote:

Le 03/03/2014 13:40, Brian Burch a écrit :


If you need some help (perhaps QA testing), please let me know.


Thank you for offering your help Brian. Tomcat 7.0.52 is now available
for Debian Wheezy through the backports [1]. Tomcat 8 has been submitted
and is awaiting to be reviewed. If it goes well it may be available in
the backports in one month.

Emmanuel Bourg

[1] https://packages.debian.org/source/wheezy-backports/tomcat7


Hi, Emmanuel.

I have downloaded your packages and will try them out during this week.

I suspect the tomcat dev list is not the right place to discuss testing 
these packages in detail. Unless you say otherwise, I will email you 
directly with any observations.


Regards,

Brian



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




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

Seeking guidance on use of apache commons classes

[Brian Burch]

On 29/11/12 16:11, Brian Burch wrote:

On 29/11/12 14:37, bugzi...@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  changed:

What|Removed |Added


  Status|NEW |RESOLVED
  Resolution|--- |FIXED

--- Comment #6 from Mark Thomas  ---





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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: Seeking guidance on use of apache commons classes

[Brian Burch]

On 04/01/13 19:58, Konstantin Kolinko wrote:

2013/1/4 Brian Burch :

On 29/11/12 16:11, Brian Burch wrote:


On 29/11/12 14:37, bugzi...@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  changed:

 What|Removed |Added



   Status|NEW |RESOLVED
   Resolution|--- |FIXED

--- Comment #6 from Mark Thomas  ---







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 clas

Re: [Bug 54729] new HttpParser.parseAuthorizationBasic method

[Brian Burch]

On 20/03/13 10:17, bugzi...@apache.org wrote:

https://issues.apache.org/bugzilla/show_bug.cgi?id=54729


Sorry I could not reply more quickly, Mark, but I am currently in 
Australia and I am probably asleep while you are working.


Judging by the flurry of activity in this area, I conclude my change is 
no longer necessary and you might want to close this bug report.


Just for completeness, I'll briefly answer your comments and the later 
one from Konstantin.



--- Comment #1 from Mark Thomas  ---
Regarding your points:
1. Clarity vs efficiency is always a trade off. It depends how much efficiency
has been lost. Do you have any performance numbers?


You were quick to write a performance test! Given more time, I would 
have explained that I (like Konstantin) did not like the conversion from 
MessageBytes to a StringReader, and did not like the way I had created a 
second StringReader to parse the decoded the Base64 blob. However, I 
felt the performance hit would be small relative to the original 
implementation, although it would create extra short-lived instances.


My earlier approach had attempted to push the MessageBytes into the new 
HttpParser method, but that meant the parsing logic for BASIC became 
very different to that of DIGEST. I couldn't see a way to square the 
circle, so I trashed it and started again on a more harmonious path.



2. I don't see a test case either. I'd rather get any bugs in the decoder fixed
than put sticking plasters over other bits of code.


I've worked with Base64 implementations a lot in the past, in many 
different languages. However, the last time I looked (showing my age!) 
the java class was in a sun.* package and well worth avoiding. When I 
researched the apache commons version, I did not realise it had been 
superseded by javax.xml.bind.DatatypeConverter.


I've just noticed the discussion on the dev list "Use of JAXB api to 
process Base64 in Tomcat 7 and 8". I understand the arguments presented 
to not use this converter, but if you back out your change and reinstate 
org.apache.catalina.util.Base64, then we just go back to where I started 
with a broken implementation and no unit tests



3. I'd prefer to do this first. That code is only still in the code base
because it support ByteChunk / CharChunk which should allow a more efficient
interface with the rest of the Tomcat code base. It is probably time that that
assumption is tested.


I understand. However, the two decode methods look superficially quite 
similar to me. There must be scope for refactoring them, writing a 
proper unit test suite, and fixing the bugs that I wrapped in my 
suggested change to Basic Authenticator. That is a fair amount of work 
to achieve little more than just copying another implementation and add 
some novel method signatures. Wrappering a robust implementation feels 
like a better approach to me.



4. It is very unlikely we will be adding Commons Codec as a dependency to
Tomcat. We may do an svn copy and rename much like we have done for FileUpload
but the usefulness of that depends on the ByteChunk/CharChunk issues in 3. For
FileUpload I am looking at replacing the decoder with the JVM implementation.


I didn't follow your FileUpload change closely, especially before you 
switched to the DatatypeConverter. Did you basically clone the commons 
class? If yes, surely it would be better to put that code in tomcat.util 
- FileUpload could use it from there and I suspect the 
ByteChunk/CharChunk users would only need some pre/post processing.


It seems to me there are enough developers (in the same time zone) 
working on these issues. I don't have a lot of time to spend on tomcat 
at the moment, and it is fairly disheartening to find my efforts in what 
seemed to be a quiet backwater have been washed away by a flood. I'll 
step back until the dust settles! Let me know if I can help.


Regards,

Brian




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

Re: [Bug 54729] new HttpParser.parseAuthorizationBasic method

[Brian Burch]

On 21/03/13 09:15, Mark Thomas wrote:

On 21/03/2013 07:56, Brian Burch wrote:

On 20/03/13 10:17, bugzi...@apache.org wrote:

https://issues.apache.org/bugzilla/show_bug.cgi?id=54729


Sorry I could not reply more quickly, Mark, but I am currently in
Australia and I am probably asleep while you are working.


Not a problem. One of the reasons the ASF insists on using mailing lists
rather than teleconferences is to ensure everyone in all timezones has
an equal chance to participate.


Judging by the flurry of activity in this area, I conclude my change is
no longer necessary and you might want to close this bug report.


You know that better than I. My understanding from reading your report
was that there were a number of issues of which Base64 was just one and
one that was making fixing the others look a little ugly.


Just for completeness, I'll briefly answer your comments and the later
one from Konstantin.


--- Comment #1 from Mark Thomas  ---
Regarding your points:
1. Clarity vs efficiency is always a trade off. It depends how much
efficiency
has been lost. Do you have any performance numbers?


You were quick to write a performance test! Given more time, I would
have explained that I (like Konstantin) did not like the conversion from
MessageBytes to a StringReader, and did not like the way I had created a
second StringReader to parse the decoded the Base64 blob. However, I
felt the performance hit would be small relative to the original
implementation, although it would create extra short-lived instances.

My earlier approach had attempted to push the MessageBytes into the new
HttpParser method, but that meant the parsing logic for BASIC became
very different to that of DIGEST. I couldn't see a way to square the
circle, so I trashed it and started again on a more harmonious path.


The conversions aren't ideal but I think they are a price worth paying
for not having to maintain another Base64 encoder/decoder.


2. I don't see a test case either. I'd rather get any bugs in the
decoder fixed
than put sticking plasters over other bits of code.


I've worked with Base64 implementations a lot in the past, in many
different languages. However, the last time I looked (showing my age!)
the java class was in a sun.* package and well worth avoiding. When I
researched the apache commons version, I did not realise it had been
superseded by javax.xml.bind.DatatypeConverter.

I've just noticed the discussion on the dev list "Use of JAXB api to
process Base64 in Tomcat 7 and 8". I understand the arguments presented
to not use this converter, but if you back out your change and reinstate
org.apache.catalina.util.Base64, then we just go back to where I started
with a broken implementation and no unit tests


I have a plan B in mind for that. More details on that thread.


3. I'd prefer to do this first. That code is only still in the code base
because it support ByteChunk / CharChunk which should allow a more
efficient
interface with the rest of the Tomcat code base. It is probably time
that that
assumption is tested.


I understand. However, the two decode methods look superficially quite
similar to me. There must be scope for refactoring them, writing a
proper unit test suite, and fixing the bugs that I wrapped in my
suggested change to Basic Authenticator. That is a fair amount of work
to achieve little more than just copying another implementation and add
some novel method signatures. Wrappering a robust implementation feels
like a better approach to me.


+1. I'm all for code re-use.


4. It is very unlikely we will be adding Commons Codec as a dependency to
Tomcat. We may do an svn copy and rename much like we have done for
FileUpload
but the usefulness of that depends on the ByteChunk/CharChunk issues
in 3. For
FileUpload I am looking at replacing the decoder with the JVM
implementation.


I didn't follow your FileUpload change closely, especially before you
switched to the DatatypeConverter. Did you basically clone the commons
class? If yes, surely it would be better to put that code in tomcat.util
- FileUpload could use it from there and I suspect the
ByteChunk/CharChunk users would only need some pre/post processing.


The Base64 decoder in FileUpload was copied from Geronimo to Commons and
then I updated Tomcat's copy of FileUpload. That Base64 code has issues
of it's own. Looking for a solution to that prior to the 7.0.39 release
was co-incidental with you looking at Base64 for BASIC auth.


It seems to me there are enough developers (in the same time zone)
working on these issues. I don't have a lot of time to spend on tomcat
at the moment, and it is fairly disheartening to find my efforts in what
seemed to be a quiet backwater have been washed away by a flood. I'll
step back until the dust settles! Let me know if I can help.


I'm sorry you feel that your efforts have been wasted. My intention was
to get you a

Re: [Bug 54729] new HttpParser.parseAuthorizationBasic method

[Brian Burch]

On 21/03/13 09:33, Julian Reschke wrote:

On 2013-03-21 10:15, Mark Thomas wrote:

...
I'm sorry you feel that your efforts have been wasted. My intention was
to get you a robust Base64 implementation to work with for the other
improvements you had in mind for BASIC auth. If there are other
improvements you have in mind for BASIC auth then your help there would
be much appreciated.
...


Related to that: I'd love to hear feedback on
.


Thanks for pointing it out to me, Julian. At first glance it seems to 
very relevant and interesting. I'll read it carefully and get back to you.


Brian



Best regards, Julian

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



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

Re: [Bug 54729] new HttpParser.parseAuthorizationBasic method

[Brian Burch]

On 22/03/13 01:11, Brian Burch wrote:


I'll keep this enhancement open until I've had time to think properly...
although your new Basic "parser" has returned to BasicAuthenticator,
there might still be some merit in moving it to HttpParser and keeping
my proposed test suite, especially now you have written a performance test!


It looks as if the dust has settled...

I noticed the commons Base64 unit tests were not ported, so effectively 
the only tests we have currently are very high-level and so carry a lot 
of overhead, e.g. TestNonLoginAndBasicAuthenticator hauls up and tears 
down at least one tomcat instance for every test case.


I've looked at the code Mark committed for Base64 and I don't feel 
attracted to the idea of me porting the relevant sections of the commons 
test suite. On the other hand, I also don't feel comfortable simply 
retooling my proposed tests for basic auth in HttpParser to act as an 
embryo test suite for the new Base64 class - that would imply much more 
than it would deliver.


I feel most comfortable with the idea of more-or-less retaining my 
existing proposed test suite. It doesn't require a tomcat instance to 
run, and yet it makes a reasonably complete attempt at covering all 
permissible, tolerable and illegal cases of basic authentication. I know 
that approach doesn't cover FileUploader, or other base64 users, but we 
don't have any open bugs in those areas either - just a few todo's in my 
authentication tests.


I am just a contributor, with no ambition to become more. I value the 
work done by the developers and want to give something back when 
possible. Therefore, I want my proposals to feel comfortable to you guys 
and not require a lot of rework to match your style.


Assuming you agree with me so far, I would appreciate your opinions on 
how I should proceed. The main decision hinges on style, I think...


I like the idea of retaining a HttpParser.parseAuthorizationBasic 
method, but the obvious and the most useful signature would be 
(MessageBytes authorization). This would make a neat division between 
the higher-level server logic and the low-level handling of the specific 
authorization header. The drawback is the signature would have 
absolutely no similarity to 
HttpParser.parseAuthorizationDigest(StringReader). It feels a bit like 
fitting a square peg into a round hole, but at least they are on the 
same block of wood.


An alternative would be to refactor the existing rather minimal parser 
logic into a protected method such as 
BasicAuthenticator.parseAuthorization((MessageBytes authorization). This 
new method could be explicitly called by new test cases in 
TestNonLoginAndBasicAuthenticator, thus avoiding the tomcat haul-up/down 
overhead.


Alternatively, it might be simpler to pass a ByteChunk argument to the 
chosen new method.


Let me know what you think - I won't start work until I'm sure I know 
where to go!


Regards,

Brian




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

Re: [VOTE] Release Apache Tomcat 7.0.40

[Brian Burch]

On 05.05.2013 12:44, Mark Thomas wrote:

The proposed Apache Tomcat 7.0.40 release is now available for voting.

It can be obtained from:
https://dist.apache.org/repos/dist/dev/tomcat/tomcat-7/v7.0.40/
The Maven staging repo is:
https://repository.apache.org/content/repositories/orgapachetomcat-001/
The svn tag is:
http://svn.apache.org/repos/asf/tomcat/tc7.0.x/tags/TOMCAT_7_0_40/

The proposed 7.0.40 release is:
[ ] Broken - do not release
[X] Stable - go ahead and release as 7.0.40 Stable


I would like to use this release as early as possible, so I picked up 
the source, built and tested it. I noticed some errors which do not 
matter in my environment, but because I haven't tested any earlier 
releases, I don't know whether they are generally acceptable. I couldn't 
find a reference to them being known problems, and (of course) it could 
be a "user error".


environment:

Linux version 3.5.0-28-lowlatency (buildd@roseapple) (gcc version 4.7.2 
(Ubuntu/Linaro 4.7.2-2ubuntu1) ) #32-Ubuntu SMP PREEMPT Fri Apr 26 
11:05:36 UTC 2013 (Ubuntu 3.5.0-28.32-lowlatency 3.5.7.9)


brian@bacchus:~$ /usr/lib/jvm/java-6-openjdk-i386/bin/java -version
java version "1.6.0_27"
OpenJDK Runtime Environment (IcedTea6 1.12.3) (6b27-1.12.3-0ubuntu1~12.10.1)
OpenJDK Server VM (build 20.0-b12, mixed mode)


I have /real/ test failures for both BIO and NIO in:
TEST-org.apache.catalina.tribes.group.interceptors.TestNonBlockingCoordinator.?IO.txt
TEST-org.apache.catalina.tribes.group.interceptors.TestOrderInterceptor.?IO.txt
TEST-org.apache.catalina.tribes.group.interceptors.TestTcpFailureDetector.?IO.txt
TEST-org.apache.catalina.tribes.group.TestGroupChannelMemberArrival.?IO.txt

for example, one of them is:

Testcase: testMemberArrival took 7.191 sec FAILED
Checking member arrival length (Listener-10) expected:<9> but was:<0>
junit.framework.AssertionFailedError: Checking member arrival length 
(Listener-10) expected:<9> but was:<0>
at 
org.apache.catalina.tribes.group.TestGroupChannelMemberArrival.testMemberArrival(TestGroupChannelMemberArrival.java:80)



I don't want to waste anybody's time, but perhaps you could let me know 
whether the errors are significant? If you think it is because I've made 
a simple error in my configuration, perhaps you point me in the right 
direction to resolve it.


Thanks,

Brian

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

Re: [VOTE] Release Apache Tomcat 7.0.40

[Brian Burch]

On 06/05/13 15:27, Konstantin Kolinko wrote:

2013/5/6 Brian Burch :

On 05.05.2013 12:44, Mark Thomas wrote:


The proposed Apache Tomcat 7.0.40 release is now available for voting.

It can be obtained from:
https://dist.apache.org/repos/dist/dev/tomcat/tomcat-7/v7.0.40/
The Maven staging repo is:
https://repository.apache.org/content/repositories/orgapachetomcat-001/
The svn tag is:
http://svn.apache.org/repos/asf/tomcat/tc7.0.x/tags/TOMCAT_7_0_40/

The proposed 7.0.40 release is:
[ ] Broken - do not release
[X] Stable - go ahead and release as 7.0.40 Stable



I would like to use this release as early as possible, so I picked up the
source, built and tested it. I noticed some errors which do not matter in my
environment, but because I haven't tested any earlier releases, I don't know
whether they are generally acceptable. I couldn't find a reference to them
being known problems, and (of course) it could be a "user error".

environment:

Linux version 3.5.0-28-lowlatency (buildd@roseapple) (gcc version 4.7.2
(Ubuntu/Linaro 4.7.2-2ubuntu1) ) #32-Ubuntu SMP PREEMPT Fri Apr 26 11:05:36
UTC 2013 (Ubuntu 3.5.0-28.32-lowlatency 3.5.7.9)

brian@bacchus:~$ /usr/lib/jvm/java-6-openjdk-i386/bin/java -version
java version "1.6.0_27"
OpenJDK Runtime Environment (IcedTea6 1.12.3) (6b27-1.12.3-0ubuntu1~12.10.1)
OpenJDK Server VM (build 20.0-b12, mixed mode)


I have /real/ test failures for both BIO and NIO in:
TEST-org.apache.catalina.tribes.group.interceptors.TestNonBlockingCoordinator.?IO.txt
TEST-org.apache.catalina.tribes.group.interceptors.TestOrderInterceptor.?IO.txt
TEST-org.apache.catalina.tribes.group.interceptors.TestTcpFailureDetector.?IO.txt
TEST-org.apache.catalina.tribes.group.TestGroupChannelMemberArrival.?IO.txt

for example, one of them is:

Testcase: testMemberArrival took 7.191 sec FAILED
Checking member arrival length (Listener-10) expected:<9> but was:<0>
junit.framework.AssertionFailedError: Checking member arrival length
(Listener-10) expected:<9> but was:<0>
 at
org.apache.catalina.tribes.group.TestGroupChannelMemberArrival.testMemberArrival(TestGroupChannelMemberArrival.java:80)


I don't want to waste anybody's time, but perhaps you could let me know
whether the errors are significant? If you think it is because I've made a
simple error in my configuration, perhaps you point me in the right
direction to resolve it.


Tribes is communication layer that is used to implement clustering
(aka "ha", high availability)
http://tomcat.apache.org/tomcat-7.0-doc/tribes/introduction.html

If you do not use clustering, then those do not matter for you.


You can compare your test log files with ones produced by buildbot:
http://ci.apache.org/projects/tomcat/tomcat7/logs/

If you want to (re)run a single test from the testsuite, BUILDING.txt
says how to do so.

Maybe some network or security configuration at your box prevents
those tests from working? IPv6? Any error messages in the logs?

The tests respect the "conf/logging.properties" file, so it is
possible to enable fine logging if you need so.


Thank you for your suggestions, Konstantin. Thanks also to Rainer for 
his suggestions.


My primary objective was to raise the issue at this opportune moment, 
just in case it turned out to be a general problem.


I am satisfied with your theory that the failures I see are related to 
multicast support on my particular development system. As I said at the 
start, I do not need tribes support on my production system.


When I deploy the new release on my production system, hopefully later 
this week, I will run all the tests there too. It (now) seems quite 
likely that I will observe different behaviour under the different 
execution environment.


If anything strikes me as strange, then I will raise the issue, with 
more diagnostics, on the users list.


In the meantime, please discount my report when considering the 
stability of 7.0.40.


Thanks,

Brian


Best regards,
Konstantin Kolinko

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




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

Re: svn commit: r1484409 - in /tomcat/trunk: ./ res/ide-support/netbeans/ webapps/docs/

[Brian Burch]

On 20/05/13 10:53, ma...@apache.org wrote:

Author: markt
Date: Mon May 20 09:53:25 2013
New Revision: 1484409

URL: http://svn.apache.org/r1484409
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54899
Provide an initial implementation of NetBeans support.
Patch provided by Brian Burch.

Added:
 tomcat/trunk/res/ide-support/netbeans/
 tomcat/trunk/res/ide-support/netbeans/README   (with props)
 tomcat/trunk/res/ide-support/netbeans/nb-tomcat-build.properties   (with 
props)
 tomcat/trunk/res/ide-support/netbeans/nb-tomcat-project.properties   (with 
props)
 tomcat/trunk/res/ide-support/netbeans/nb-tomcat.xml   (with props)
 tomcat/trunk/res/ide-support/netbeans/project.xml   (with props)



Thanks very much Mark - I now have a sensible base on which to explore 
further improvements.


BTW.. I tried to preserve the implicit and explicit NetBeans copyright 
notices on its auto-generated templates, while adding suitable 
copyrights for the new apache content... did you conclude that my 
amateur attempts were good enough? Simply passing our primitive code 
style checks is probably not good enough!


Regards,

Brian

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

Re: svn commit: r1484409 - in /tomcat/trunk: ./ res/ide-support/netbeans/ webapps/docs/

[Brian Burch]

On 20/05/13 12:16, Mark Thomas wrote:

On 20/05/2013 11:16, Brian Burch wrote:

On 20/05/13 10:53, ma...@apache.org wrote:

Author: markt
Date: Mon May 20 09:53:25 2013
New Revision: 1484409

URL: http://svn.apache.org/r1484409
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54899
Provide an initial implementation of NetBeans support.
Patch provided by Brian Burch.

Added:
  tomcat/trunk/res/ide-support/netbeans/
  tomcat/trunk/res/ide-support/netbeans/README   (with props)

tomcat/trunk/res/ide-support/netbeans/nb-tomcat-build.properties
(with props)

tomcat/trunk/res/ide-support/netbeans/nb-tomcat-project.properties
(with props)
  tomcat/trunk/res/ide-support/netbeans/nb-tomcat.xml   (with props)
  tomcat/trunk/res/ide-support/netbeans/project.xml   (with props)



Thanks very much Mark - I now have a sensible base on which to explore
further improvements.

BTW.. I tried to preserve the implicit and explicit NetBeans copyright
notices on its auto-generated templates, while adding suitable
copyrights for the new apache content... did you conclude that my
amateur attempts were good enough? Simply passing our primitive code
style checks is probably not good enough!


I don't see any copyright notices. I don't see any license headers either.

I do see a section that looks similar to our NOTICE file.

>

These files appear to be auto-generated based on input you have
provided. The only content I'm concerned about are the comments. Are
they all yours? If so, I'll just remove the NetBeans notice. If not,
we'll need to remove any comments that aren't yours and the notice.


I cut'n'pasted the NOTICE into each of the files, then wrapped them in 
the appropriate delimiters to render them as xml or Properties comments 
as appropriate.


You are correct that the skeletons of the two xml files were 
auto-generated based on my input, but they currently contain my attempt 
at a NetBeans attribution for the following reasons:


** project.xml is my adaptation of the default file created by NetBeans 
for a Free Form project that does not already have one. The default 
auto-generated file does NOT contain a NetBeans copyright notice, but it 
does reference netbeans schemas and is, in my naive opinion, implicitly 
copyrighted.


** nb-project.xml was created by me from scratch, but based on my 
examination of the xml files NetBeans auto-generates to support a 
project which needs to be built under a different jdk to the one 
netbeans is running under (aka the default platform). I felt I had used 
sufficient look-and-feel in my file that it would be honest to 
acknowledge the work done by the NetBeans developers on the originals.


In other words, I believe my intent is correct... but I am not confident 
I have achieved my objective. I hoped you guys could put my work right 
if necessary.



Mark


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




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

Re: svn commit: r1484409 - in /tomcat/trunk: ./ res/ide-support/netbeans/ webapps/docs/

[Brian Burch]

On 20/05/13 21:19, Mark Thomas wrote:

On 20/05/2013 18:32, Brian Burch wrote:

On 20/05/13 12:16, Mark Thomas wrote:



These files appear to be auto-generated based on input you have
provided. The only content I'm concerned about are the comments. Are
they all yours? If so, I'll just remove the NetBeans notice. If not,
we'll need to remove any comments that aren't yours and the notice.


I cut'n'pasted the NOTICE into each of the files, then wrapped them in
the appropriate delimiters to render them as xml or Properties comments
as appropriate.

You are correct that the skeletons of the two xml files were
auto-generated based on my input, but they currently contain my attempt
at a NetBeans attribution for the following reasons:

** project.xml is my adaptation of the default file created by NetBeans
for a Free Form project that does not already have one. The default
auto-generated file does NOT contain a NetBeans copyright notice, but it
does reference netbeans schemas and is, in my naive opinion, implicitly
copyrighted.


NetBeans does not have any copyright in that project.xml.

Any comments in the XSDs are copyrightable. If you strip out all the
comments from the XSD what is left is not copyrightable. Copyright in
any document that uses the XSD rests with the person that wrote the
document (you).


** nb-project.xml was created by me from scratch, but based on my
examination of the xml files NetBeans auto-generates to support a
project which needs to be built under a different jdk to the one
netbeans is running under (aka the default platform). I felt I had used
sufficient look-and-feel in my file that it would be honest to
acknowledge the work done by the NetBeans developers on the originals.


The look and feel is driven as much by the requirements of having to
conform to the XSD. For the same reasons as above, the only copyright in
that file is yours.

The notices, while well meaning, are a) unnecessary and b) likely to
cause confusion for end users about how they are licensed.

I'll remove the notices you added.


Thanks for explaining, Mark. Thanks also for taking the time to make the 
changes.


Thanks also to Konstantin for working through the content and 
standardising the terminology and filenames. I'll be more careful next time.


With respect to the three XSDs referenced in my project.xml, these urls 
all return 404 Not Found status. (I forget how many years ago I first 
discovered this).


While researching some of the problems I had developing a design for 
tomcat that actually worked, I found other people who had similar 
problems with their own Free Form projects. I never got to the bottom of 
it, but I formed the distinct impression that NetBeans has its own 
internal xml parser for these files, and it doesn't always work the way 
one might expect. Anyone looking closely at my initial version who 
wonders why something hasn't been done more elegantly should keep this 
in mind.


Brian


Mark


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




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

Re: svn commit: r1484409 - in /tomcat/trunk: ./ res/ide-support/netbeans/ webapps/docs/

[Brian Burch]

On 21/05/13 10:19, Mark Thomas wrote:

On 21/05/2013 09:48, Brian Burch wrote:

With respect to the three XSDs referenced in my project.xml, these urls
all return 404 Not Found status. (I forget how many years ago I first
discovered this).


That will be because they are URIs, not URLs.

If you want to get a copy, add ".xsd" to the end of the URI. That isn't
a general rule but seems to work for these XSDs.


Thanks! That worked. They do have copyright notices, just as you predicted.


While researching some of the problems I had developing a design for
tomcat that actually worked, I found other people who had similar
problems with their own Free Form projects. I never got to the bottom of
it, but I formed the distinct impression that NetBeans has its own
internal xml parser for these files, and it doesn't always work the way
one might expect.


NetBeans will have local copies of those XSDs and will use a resolver
knows where they are - much like Tomcat has local copies of the XSDs
used by web.xml and uses those local copies when validating input.


I now have some new-to-me and useful input when working on future 
improvements. Thanks again.




Mark


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




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

Request for wiki update permission

[Brian Burch]

I have just registered a wiki name of BrianBurch. Could I please be 
given permission to update the tomcat wiki? (I need to bring the page on 
NetBeans support up to date with http://svn.apache.org/r1484409)


Thanks,

Brian

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

Re: svn commit: r1492563 - in /tomcat/tc7.0.x/trunk: ./ build.properties.default build.xml modules/jdbc-pool/build.properties.default modules/jdbc-pool/build.xml webapps/docs/changelog.xml

[Brian Burch]

Sorry, but I cannot quote the relevant section of the original email 
because I had already deleted my copy.


This commit replaces the dependency on junit 4.8.2 with 4.11. It causes 
collateral damage to the current version of netbeans support. It is on 
my todo list to come up with a less fragile solution, but in the 
meantime could someone please commit the following change on my behalf?


brian@schizo:~/sandboxApache/tomcat8/trunk$ svn diff
Index: res/ide-support/netbeans/project.xml
===
--- res/ide-support/netbeans/project.xml(revision 1492631)
+++ res/ide-support/netbeans/project.xml(working copy)
@@ -189,9 +189,9 @@
 
 test
 
-mode="compile">output/classes:output/testclasses:${base.path}/junit4.8.2/junit-4.8.2.jar
+mode="compile">output/classes:output/testclasses:${base.path}/junit-4.11/junit-4.11.jar

 1.7
 
 
 
-
\ No newline at end of file
+

BTW.. I don't understand the "no newline" warning - I tried my diff 
against new copies with and without a trailing newline, but got the 
message in both cases.


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

Re: svn commit: r1492647 - in /tomcat/trunk/res/ide-support/netbeans: nb-tomcat-build.properties project.xml

[Brian Burch]

On 13/06/13 13:44, kkoli...@apache.org wrote:

Author: kkolinko
Date: Thu Jun 13 12:44:30 2013
New Revision: 1492647

URL: http://svn.apache.org/r1492647
Log:
Update JUnit version in NetBeans files.
Hamcrest library is now in its own jar.

Modified:
 tomcat/trunk/res/ide-support/netbeans/nb-tomcat-build.properties
 tomcat/trunk/res/ide-support/netbeans/project.xml

Modified: tomcat/trunk/res/ide-support/netbeans/nb-tomcat-build.properties
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/res/ide-support/netbeans/nb-tomcat-build.properties?rev=1492647&r1=1492646&r2=1492647&view=diff
==
--- tomcat/trunk/res/ide-support/netbeans/nb-tomcat-build.properties (original)
+++ tomcat/trunk/res/ide-support/netbeans/nb-tomcat-build.properties Thu Jun 13 
12:44:30 2013
@@ -37,7 +37,7 @@ nb-test.io-method=org.apache.coyote.http
  # it is not possible to retrieve the classpaths from the build to
  # use in the NetBeans targets, so they must be explicitly declared

-nb-test.classpath=${test.classes}:${tomcat.build}/webapps/examples/WEB-INF/classes:${base.path}/junit4.8.2/junit-4.8.2.jar:${base.path}/ecj-4.2.2/ecj-4.2.2.jar:${tomcat.classes}
+nb-test.classpath=${test.classes}:${tomcat.build}/webapps/examples/WEB-INF/classes:${base.path}/junit-4.11/junit-4.11.jar:${base.path}/hamcrest-1.3/hamcrest-core-1.3.jar:${base.path}/ecj-4.2.2/ecj-4.2.2.jar:${tomcat.classes}

  # Extra properties used by the Tomcat project additional NetBeans targets.


Modified: tomcat/trunk/res/ide-support/netbeans/project.xml
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/res/ide-support/netbeans/project.xml?rev=1492647&r1=1492646&r2=1492647&view=diff
==
--- tomcat/trunk/res/ide-support/netbeans/project.xml (original)
+++ tomcat/trunk/res/ide-support/netbeans/project.xml Thu Jun 13 12:44:30 2013
@@ -189,7 +189,7 @@
  
  test
  
-output/classes:output/testclasses:${base.path}/junit4.11/junit-4.11.jar
+output/classes:output/testclasses:${base.path}/junit-4.11/junit-4.11.jar:${base.path}/hamcrest-1.3/hamcrest-core-1.3.jar
  1.7
  
  



Thanks, Konstantin!

I had noticed the arrival of an explicit dependency on hamcrest when we 
moved to junit 4.11, but because I didn't know why I decided to only 
include the new junit jar in the netbeans source classpath.


I have just read 
http://stackoverflow.com/questions/5569394/how-to-use-junit-and-hamcrest-together, 
and can see the full explanation (including the fact that complications 
associated with junit-dep.jar have gone away with 4.11).


I now understand that your change was necessary to avoid netbeans 
flagging syntax errors on some test classes.




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




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

Re: svn commit: r1492563 - in /tomcat/tc7.0.x/trunk: ./ build.properties.default build.xml modules/jdbc-pool/build.properties.default modules/jdbc-pool/build.xml webapps/docs/changelog.xml

[Brian Burch]

On 13/06/13 13:57, Konstantin Kolinko wrote:

2013/6/13 Brian Burch :

Sorry, but I cannot quote the relevant section of the original email because
I had already deleted my copy.

This commit replaces the dependency on junit 4.8.2 with 4.11. It causes
collateral damage to the current version of netbeans support. It is on my
todo list to come up with a less fragile solution, but in the meantime could
someone please commit the following change on my behalf?

brian@schizo:~/sandboxApache/tomcat8/trunk$ svn diff
Index: res/ide-support/netbeans/project.xml
===
--- res/ide-support/netbeans/project.xml(revision 1492631)
+++ res/ide-support/netbeans/project.xml(working copy)
@@ -189,9 +189,9 @@
  
  test
  
-output/classes:output/testclasses:${base.path}/junit4.8.2/junit-4.8.2.jar
+output/classes:output/testclasses:${base.path}/junit-4.11/junit-4.11.jar
  1.7
  
  
  
-
\ No newline at end of file
+



Done.
http://svn.apache.org/r1492647

BTW,  nb-tomcat-project.properties  file contains:
ant.includes= ... ${ant.home}/lib/junit.jar

Does one really need that copy of JUnit jar?
There is no junit.jar file in my copy of Ant 1.9.1.


That is a good point, so thanks for pointing it out. I have just 
discovered that my copy was installed as part of the 1.8.2 ant-optional 
debian package.


I need to take a long, thoughtful look at junit specifically. The 
situation is particularly tricky because it needs to used consistently:
a) under command-line ant builds for tomcat8, but not necessarily for 
other projects.
b) under the netbeans source editor syntax checker for tomcat8, but not 
for other projects (netbeans ships with its own copy of junit).
c) when building, executing or debugging tomcat8 under netbeans, which 
uses its own copy of ant to run my nb-project.xml targets that, in turn, 
run the tomcat8 ant targets.


I know I currently have some problems in this area, e.g. my own netbeans 
is not intercepting tomcat8's junit results to be graphically presented 
on the workbench.


(I feel there isn't a lot I can learn by looking at the way eclipse 
works, but I hardly ever use it so can't be sure.)



BTW.. I don't understand the "no newline" warning - I tried my diff against
new copies with and without a trailing newline, but got the message in both
cases.


It is just an information message, as otherwise the textual diff
format cannot show the difference differentiate between missing and
present newline on the last line of a file.

As the message is just after '-' and before '+' it means that the old
file ends with '' character without any trailing newline.
It might be that your text editor adds one.

For XML files the presence of a newline at the end of the file does not matter.


That is good to know. I wanted to ensure this initial release of 
netbeans support did not have any nasty style problems that might be 
carried forward into future changes. (e.g. I wondered whether this might 
be an svn property issue.) I will stop worrying!



Best regards,
Konstantin Kolinko

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




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

Tomcat 7.0.41 JNDIRealm revision 1491394

[Brian Burch]

I eventually got round to integration testing of 7.0.41 yesterday and 
was baffled to find I couldn't logon!


To cut a long debugging story short, revision 1491394 has a bug that was 
introduced as part of the standardisation of our Base64 handling. I 
wasn't sure whether I ought to open a new bug...


Here is the diff that works for me:


Index: java/org/apache/catalina/realm/JNDIRealm.java
===
--- java/org/apache/catalina/realm/JNDIRealm.java   (revision 1491394)
+++ java/org/apache/catalina/realm/JNDIRealm.java   (working copy)
@@ -1573,9 +1573,10 @@
 password = password.substring(5);
 md.reset();

md.update(credentials.getBytes(Charset.defaultCharset()));
-byte[] decoded = Base64.decodeBase64(md.digest());
+byte[] digest = md.digest();
+byte[] base64 = Base64.encodeBase64(digest);
 String digestedPassword =
-new String(decoded, B2CConverter.ISO_8859_1);
+new String(base64, B2CConverter.ISO_8859_1);
 validated = password.equals(digestedPassword);
 }
 } else if (password.startsWith("{SSHA}")) {



BTW. The code is identical in trunk, so this patch works there too.


Thinks... pity some of this stuff doesn't have some lightweight unit tests.

Sorry to be a informal with this notification, but I thought timeliness 
was more important than style!


Brian

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

Re: Tomcat 7.0.41 JNDIRealm revision 1491394

[Brian Burch]

On 27/06/13 18:51, Konstantin Kolinko wrote:

2013/6/27 Brian Burch :

I eventually got round to integration testing of 7.0.41 yesterday and was
baffled to find I couldn't logon!

To cut a long debugging story short, revision 1491394 has a bug that was
introduced as part of the standardisation of our Base64 handling. I wasn't
sure whether I ought to open a new bug...


Your numbering is wrong, that revision is not ours. It was this one:
http://svn.apache.org/viewvc?diff_format=l&view=revision&revision=1459346


Yes, I see what you mean and agree. I'm puzzled that my svn diff 
reported the wrong revision, which is a pity because I put it in the new 
thread subject and we will have to live with it unless you can 
retrospectively edit the entries?


Thanks for finding the correct revision.




Here is the diff that works for me:


Index: java/org/apache/catalina/realm/JNDIRealm.java
===
--- java/org/apache/catalina/realm/JNDIRealm.java   (revision 1491394)
+++ java/org/apache/catalina/realm/JNDIRealm.java   (working copy)
@@ -1573,9 +1573,10 @@
  password = password.substring(5);
  md.reset();

md.update(credentials.getBytes(Charset.defaultCharset()));
-byte[] decoded = Base64.decodeBase64(md.digest());
+byte[] digest = md.digest();
+byte[] base64 = Base64.encodeBase64(digest);
  String digestedPassword =
-new String(decoded, B2CConverter.ISO_8859_1);
+new String(base64, B2CConverter.ISO_8859_1);
  validated = password.equals(digestedPassword);
  }
  } else if (password.startsWith("{SSHA}")) {




In short,  s/ decodeBase64 / encodeBase64 /.


Yes. I wanted to break the logic up because I needed to encode/decode my 
test cases by hand!


I didn't trust my own external cribs and suspected I was digging into a 
difference between the sun and openjdk digest engines. 
Pencil/paper/hexadecimal calculator...


 I didn't want to go round the loop of another source 
change/build/deploy/test/debug/diff. You got the idea, just as I expected.


Imagine my surprise when I got 80% through the exercise and spotted the 
"backwards" choice of Base64 method!



It is fun that {MD5}&{SHA} branch and {SSHA} branch there use
different approaches there
(encoding the user-supplied password vs. decoding the stored one).


Actually, I can sort-of understand the person adding SSHA doing his/her 
own thing and not wanting to change logic that is stable and works 
(well, it did in 7.0.28!).


If we are looking for a vote, I prefer the way MD5/SHA does it. It takes 
the user-supplied cleartext and digest/encodes it in a forward 
direction, such that the directory hash is never exposed in situations 
where the supplied value is incorrect.



BTW. The code is identical in trunk, so this patch works there too.


Thinks... pity some of this stuff doesn't have some lightweight unit tests.

Sorry to be a informal with this notification, but I thought timeliness was
more important than style!


So will you deal with it from now on, or would you like me to open 
bugs on tc7 and tc8?


Brian



Best regards,
Konstantin Kolinko

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




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

Re: Tomcat 7.0.41 JNDIRealm revision 1491394

[Brian Burch]

On 27/06/13 21:03, Mark Thomas wrote:

On 27/06/2013 19:23, Brian Burch wrote:


So will you deal with it from now on, or would you like me to open
bugs on tc7 and tc8?


Fixed. It would be good if you could confirm the fix is correct.


I checked out the tc7 trunk. The source of JNDIRealm was exactly as I 
expected to fix the bug. I replaced my patched catalina.jar with the 
differently-sized version built from the trunk.


I confirm that authentication of SHA hashed passwords from an LDAP 
directory is working again.


Brian


Mark


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




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

Re: [VOTE] Release Apache Tomcat 7.0.42

[Brian Burch]

On 02/07/13 10:18, Mark Thomas wrote:

The proposed Apache Tomcat 7.0.42 release is now available for voting.

It can be obtained from:
https://dist.apache.org/repos/dist/dev/tomcat/tomcat-7/v7.0.42/
The Maven staging repo is:
https://repository.apache.org/content/repositories/orgapachetomcat-098/
The svn tag is:
http://svn.apache.org/repos/asf/tomcat/tc7.0.x/tags/TOMCAT_7_0_42/

The proposed 7.0.42 release is:
[ ] Broken - do not release
[ ] Stable - go ahead and release as 7.0.42 Stable

Cheers,

Mark

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



[X] Stable - go ahead and release as 7.0.42 Stable

Validate, build, release OK on linux java-6-openjdk
Tests OK on linux java-7-openjdk
Running OK on linux java-6-sun

also, JNDIRealm with LDAP hashed passwords is fine.

Brian


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

Puzzled by Access Valve Logging

[Brian Burch]

While working on 
https://issues.apache.org/bugzilla/show_bug.cgi?id=55215, I was 
surprised to discover my log files generated by AccessLogValve do not 
seem to be handled by log4j.


I've worked with the various Authenticator Valves and all I could 
remember was they used the juli Logger services, which are now being 
handled by log4j as I expect.


Because I had forgotten to change server.xml, my entry still looks like 
this:




I checked docs/config/valve.html for guidance on using juli or log4j, 
but couldn't find any clues. These, and other, logging-related 
parameters are only documented for AccessLogValve and 
ExtendedAccessLogValve.


I then looked at the source in tc8 trunk. At first glance, the class 
seems implement a self-contained logging system, complete with daily 
roll-over logic.


I went back as far as the tc5 source in January 2007. There have been 
quite a few changes, but the general idea hasn't changed significantly 
since then.


In fact, the current tc8 source seems to me to use both juli and the 
self-contained logging println service, e.g.


/**
 * Log the specified message to the log file, switching files if 
the date

 * has changed since the previous log call.
 *
 * @param message Message to be logged
 */
public void log(CharArrayWriter message) {

rotate();

/* In case something external rotated the file instead */
if (checkExists) {
synchronized (this) {
if (currentLogFile != null && !currentLogFile.exists()) {
try {
close(false);
} catch (Throwable e) {
ExceptionUtils.handleThrowable(e);

log.info(sm.getString("accessLogValve.closeFail"), e);
}

/* Make sure date is correct */
dateStamp = fileDateFormatter.format(
new Date(System.currentTimeMillis()));

open();
}
}
}

// Log this message
try {
synchronized(this) {
if (writer != null) {
message.writeTo(writer);
writer.println("");
if (!buffered) {
writer.flush();
}
}
}
} catch (IOException ioe) {
log.warn(sm.getString(
"accessLogValve.writeFail", message.toString()), ioe);
}
}


Am I being stupid? Have I overlooked something obvious?

If not, does anyone have any historical information about this 
implementation? My first thought is that this Valve should simply use 
juli (or log4j via the juli adapter) throughout, just the way the other 
valves already do.


Brian

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

Re: Puzzled by Access Valve Logging

[Brian Burch]

On 13/07/13 00:10, Konstantin Kolinko wrote:

2013/7/12 Brian Burch :

While working on https://issues.apache.org/bugzilla/show_bug.cgi?id=55215, I
was surprised to discover my log files generated by AccessLogValve do not
seem to be handled by log4j.

I've worked with the various Authenticator Valves and all I could remember
was they used the juli Logger services, which are now being handled by log4j
as I expect.

Because I had forgotten to change server.xml, my entry still looks like
this:



I checked docs/config/valve.html for guidance on using juli or log4j, but
couldn't find any clues. These, and other, logging-related parameters are
only documented for AccessLogValve and ExtendedAccessLogValve.

I then looked at the source in tc8 trunk. At first glance, the class seems
implement a self-contained logging system, complete with daily roll-over
logic.

I went back as far as the tc5 source in January 2007. There have been quite
a few changes, but the general idea hasn't changed significantly since then.

In fact, the current tc8 source seems to me to use both juli and the
self-contained logging println service, e.g.

 /**
  * Log the specified message to the log file, switching files if the
date
  * has changed since the previous log call.
  *
  * @param message Message to be logged
  */
 public void log(CharArrayWriter message) {

 rotate();

 /* In case something external rotated the file instead */
 if (checkExists) {
 synchronized (this) {
 if (currentLogFile != null && !currentLogFile.exists()) {
 try {
 close(false);
 } catch (Throwable e) {
 ExceptionUtils.handleThrowable(e);

log.info(sm.getString("accessLogValve.closeFail"), e);
 }

 /* Make sure date is correct */
 dateStamp = fileDateFormatter.format(
 new Date(System.currentTimeMillis()));

 open();
 }
 }
 }

 // Log this message
 try {
 synchronized(this) {
 if (writer != null) {
 message.writeTo(writer);
 writer.println("");
 if (!buffered) {
 writer.flush();
 }
 }
 }
 } catch (IOException ioe) {
 log.warn(sm.getString(
 "accessLogValve.writeFail", message.toString()), ioe);
 }
 }


Am I being stupid? Have I overlooked something obvious?

If not, does anyone have any historical information about this
implementation? My first thought is that this Valve should simply use juli
(or log4j via the juli adapter) throughout, just the way the other valves
already do.



It is a different feature, with different requirements.

The essential requirement for an access log is that it has to handle a
large continuous stream of data with low overhead. Note the buffering
and flushing.

Wiring the above to an arbitrary logging system through Apache Commons
Logging wrapper would just add several layers of overhead and
complicate the configuration.


A generic logging system has different requirements. It is used to log
errors and warnings (which usually do not occur often). It needs
highly configurable filtering and minimal overhead when logging is
disabled.


Thanks for your answer. I am a bit surprised the class-level comments do 
not mention this issue, but I suppose that is because the class has been 
around for such a long time.



That is the essence. I think your question really belongs to the users@ list.


I spent some time thinking about the best place to pose my question. I 
chose the dev list because it was about the design of a specific part of 
tc code which looked anomalous to me, and which might or might not 
benefit from change.


I could not foresee your answer, which I agree makes the topic more 
appropriate for the users list. Do you want to move the Q&A, possibly 
with some editing?


I think this subject deserves mention in the log4j section of 
logging.html, and probably also valve.html. Do you agree?


Thanks for taking the time to explain.

Brian


Best regards,
Konstantin Kolinko

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




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

Re: Use (or not) of @SuppressWarnings

[Brian Burch]

On 07/08/13 14:14, Nick Williams wrote:


On Aug 7, 2013, at 2:41 AM, Mark Thomas wrote:


For trunk we have been running a policy of zero warnings in the code.
This has helped to highlight issues as code is edited as any warnings
are immediately clear. Obviously, this depends on what warnings are enabled.

Currently, we use Eclipse's "Ignore unavoidable generic type problems."
Recently a couple of issues has been highlighted with this:
1. Other IDEs might not have this setting.
2. javac does not have this setting
3. Some of the problems Eclipse excludes are avoidable (well, sort of
avoidable as avoiding them requires using JRE methods that themselves
have @SuppressWarnings annotations).

In favour of the current situation is that it reduces clutter in the
code base slightly.

While I am all for reducing clutter in the code base, there do appear to
be good reasons for disabling the "Ignore unavoidable generic type
problems." and using @SuppressWarnings instead.

Personally, I am happy with the current settings but not unhappy to
change. I guess that makes me +0 on changing. What does everyone else think?


(Non-binding)

As stated earlier, I am fundamentally opposed to the concept of using an 
IDE-specific setting to decide how a project writes code. This practically 
forces all developers to use that IDE exclusively for that project. If I'm 
writing Tomcat code in IntelliJ IDE and I create a warning, I have no idea 
whether I should correct the warning or not, because I have no idea what 
Eclipse has to say about it.

I do, however, know that the warning will show up when I compile, and I think that's what 
matters most. We should make efforts to eliminate all warnings that show up when we 
compile, whether Eclipse calls them "unavoidable" or not.

My $0.02.


OK, I'll add my non-binding penny.

I use netbeans exclusively. I get warnings about import order from 
almost every tc source file. Periodically I scratch around looking for 
the rules template so I can change them to conform with the ant 
checkstyle rules, which are happy with the current import order. So far, 
no luck, so I live with netbeans nagging and resist the temptation to 
let it re-order the imports for me.


I think ant should compile without warnings. I think we should all put 
up with whatever our ide's bleat about... or figure out how to coerce 
them to follow the external/common build code style rules.



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




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

Re: [Bug 55372] Bind JPDA_ADDRESS by default to localhost

[Brian Burch]

On 07/08/13 09:32, bugzi...@apache.org wrote:

https://issues.apache.org/bugzilla/show_bug.cgi?id=55372

--- Comment #5 from Michael Osipov <1983-01...@gmx.net> ---
(In reply to Mark Thomas from comment #4)

[..]

res/ide-support/netbeans/README.txt. Was that intentional? Though, I do not
know how to port forward a port with RDP.


netbeans I know nothing about.


This is a user guide. Nothing crucial but examples should resemble the
catalina.sh settings.


The appropriate section of the netbeans README is:

   The external Tomcat instance must be started with its jvm enabled for
   debugging by adding extra arguments to JAVA_OPTS, e.g.
   -Xdebug -Xrunjdwp:transport=dt_socket,address=8000,server=y,suspend=n

This paragraph is (I think) ide-agnostic, and refers to setting up the 
server for debugging by any local or remote client.


I use nix and my catalina.sh is started by /etc/init.d/tomcatX, which 
primes JAVA_OPTS when I want debugging. I've just checked catalina.sh 
and noticed there are four JPDA_* parameters too.


My first thought is to leave the README alone. Anyone wanting to use 
netbeans to debug tomcat on a different host ought not to be spoon-fed. 
On the other hand, given the comment is related to a change where by 
default tc will not listen on any interface except lo, perhaps I should 
say so in the README. How far is far enough? wdyt?


Brian


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

Re: Use (or not) of @SuppressWarnings

[Brian Burch]

On 07/08/13 19:49, Konstantin Kolinko wrote:

2013/8/7 Brian Burch :


I use netbeans exclusively. I get warnings about import order from almost
every tc source file. Periodically I scratch around looking for the rules
template so I can change them to conform with the ant checkstyle rules,
which are happy with the current import order. So far, no luck, so I live
with netbeans nagging and resist the temptation to let it re-order the
imports for me.


Imports order is not IDE-specific. It is enforced via checkstyle.


Yes, I said that with different words in my closing paragraph. I 
followed the order of Jeremy's response, but perhaps I could have been 
clearer if I had said that first. checkstyle defines the style that 
everyone's code must follow - regardless of what our IDEs might think.



The order of import groups should be configurable in your IDE.


I agree with you!


I have never used NetBeans, but there is a screenshot of configuration dialog:
http://plugins.netbeans.org/data/images/1307540397_organizeImports.png


That was extremely kind of you, Konstantin. Thank you for taking the 
time to try helping me. I had not previously found your screen shot - 
probably because I have google set to only give me English and French.


It was useful because my primitive German was good enough to translate 
the tab title and search for the plugin. Unfortunately, that quickly 
took me back over well-trodden ground. The Organize Imports plugin does 
not exist for netbeans 7.1.x. The quoted reason is it has been 
"incorporated into NetBeans".


My netbeans options for the internal editor does not even have the 
Organize Imports tab shown in your screen shot! I can find customisation 
for hints about imports that break the current rules, but I can't find 
the actual rules!


Please don't worry any more. I will live with the warnings until I 
eventually work out how to change the rules. Rest assured that as soon 
as I succeed, I will add the recipe to README.txt and the wiki.


Brian


Best regards,
Konstantin Kolinko

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




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

Re: Need guidance for writing unit tests for 55317

[Brian Burch]

On 20/08/13 14:32, Violeta Georgieva wrote:

2013/8/20 Nick Williams wrote:


I'm working on implementing bugzilla 55317. It's a pretty important

change (weaving) to a pretty import class (WebappClassLoader), so it
obviously needs some unit tests. However, I need some guidance:


1) I've never gotten all the existing tests to pass on my machine. Last

time I ran them it took 45 minutes (holy crap) and about 1/3 of them
failed. This is obviously something wrong with my configuration and not
with Tomcat. I'd like to avoid running all tests for this reason and only
run the particular tests I'm working on. How would I do this? Is this even
possible with Ant?





It is explained in BUILDING.txt
"(7.2) Running a single test
It is possible to run a single JUnit test class by adding the "test.entry"
property to the build.properties file. The property specifies the name of
the test class.

For example:

 test.entry=org.apache.catalina.util.TestServerInfo"


e.g. this works for me..

ant 
-Dtest.name=org.apache.catalina.authenticator.TestNonLoginAndBasicAuthenticator 
test



Also I run them directly in IDE as JUnit tests.


+1 - works for the individual test classes under netbeans. Most people 
seem to use eclipse, but I can't speak for them.


Regards,

Brian



Regards
Violeta




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

Re: [Bug 55517] Resynch classpaths for netbeans support

[Brian Burch]

On 03/09/13 13:16, bugzi...@apache.org wrote:

https://issues.apache.org/bugzilla/show_bug.cgi?id=55517

Mark Thomas  changed:

What|Removed |Added

  Status|NEW |RESOLVED
  Resolution|--- |FIXED

--- Comment #3 from Mark Thomas  ---
Fixed. Thanks for the update. It will be in 8.0.0-RC2 onwards.

For future reference, just the patch in diff -u format is fine


You were just too quick for me! I posted the entire project.xml by 
mistake, before I realised it was not the patch file I had already 
created. I obsoleted the full copy with the diff - which I suppose you 
missed.


Then you missed the second diff attachment for the properties file!

That was an amazing race condition for a trivial change. Sorry for any 
inconvenience.


Brian



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

Re: svn commit: r1519804 - /tomcat/trunk/build.xml

[Brian Burch]

2013/9/3  :
> Author: markt
> Date: Tue Sep  3 19:26:17 2013
> New Revision: 1519804
>
> URL: http://svn.apache.org/r1519804
> Log:
> Tomcat only uses package-info.java files for Javadoc so don't bother 
creating empty class

files from them.
>
> Modified:
> tomcat/trunk/build.xml
>

1. Docs [1] say that those files are created so that Ant will stop
recompiling them.

Beware that you re-compilation time may increase.

2. The createMissingPackageInfoClass option is @since Ant 1.8.3.

BUILDING.txt for trunk says that we require Ant 1.8.x.
I think it is the time to update that.
My opinion is that we may say that we require 1.9.2 (the current
version of Ant).

[1] http://ant.apache.org/manual/Tasks/javac.html

Best regards,
Konstantin Kolinko


Sorry to say that I overlooked Konstantin's comments when this post was 
published. I've just been bitten when I tried building the trunk on a 
different machine to my usual system.


.. both systems have:
Apache Ant(TM) version 1.8.2 compiled on May 18 2012

.. this system builds perfectly:
java-6-openjdk-i386 javac 1.6.0_27

.. and this one fails:
java-7-openjdk-i386 javac 1.7.0_25

sandboxApache/tomcat8/trunk/build.xml:615: javac doesn't support the 
"createMissingPackageInfoClass" attribute



https://issues.apache.org/bugzilla/show_bug.cgi?id=52096

says the bug was fixed in ant 1.8.3, but I haven't upgraded to see 
whether it resolves this particular build failure.


An update to BUILDING.txt would be trivial - hardly worth a new bug 
report. BUT what (as Konstantin said above) should be put instead of 
"1.8.x"?


Regards,

Brian

> Modified: tomcat/trunk/build.xml
> URL: 
http://svn.apache.org/viewvc/tomcat/trunk/build.xml?rev=1519804&r1=1519803&r2=1519804&view=diff
> 
==

> --- tomcat/trunk/build.xml (original)
> +++ tomcat/trunk/build.xml Tue Sep  3 19:26:17 2013
> @@ -611,7 +611,8 @@
> optimize="${compile.optimize}"
> excludes="**/.svn/**"
> encoding="ISO-8859-1"
> -   includeAntRuntime="true" >
> +   includeAntRuntime="true"
> +   createMissingPackageInfoClass="false" >
>
>
>  
>
>
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>

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

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

Re: 8.0.x / 7.0.x progress

[Brian Burch]

On 02/10/13 09:21, Mark Thomas wrote:

On 01/10/2013 22:56, Konstantin Preißer wrote:

Hi Mark,


-Original Message-
From: Mark Thomas [mailto:ma...@apache.org]
Sent: Tuesday, October 1, 2013 8:39 PM
To: Tomcat Developers List
Subject: 8.0.x / 7.0.x progress

Pulling together information from multiple threads:

8.0.x trunk appears to be stable (i.e. no longer crashes) but the
FormAuthenticator unit tests are very slow (24 minutes). This needs to
be resolved.


I noticed something strange with the Snake WebSocket example when running 
current trunk (r1528211) on Windows (x64) with 64-bit version of tcnative-1.dll 
(version 1.1.29 - from Mladen: 
http://people.apache.org/~mturk/native/tomcat-native-win32-x86_64-r1528132.zip).


I've noticed a number of oddities as well. Fixing these is next on my
TODO list in case they highlight further issues with the APR/native
connector.


When I did some work on this class, I remember discussing how one or two 
of the FormAuthenticator test cases took a while to run because they 
have to wait for a timeout, but I haven't paid attention to run-times 
recently.


I have a current and also a 3-week old version of tc8 trunk, and both 
TestFormAuthenticator runs take over 4 minutes - I don't remember them 
taking that long.


However, I am running my tests ONLY under nio... both apr and bio are 
disabled in my build.properties.


If you want me to take a look, let me know. On the other hand, if you 
think it is caused by your recent changes, I'll leave it alone for now.


Brian


Mark

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




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

Re: 8.0.x / 7.0.x progress

[Brian Burch]

On 02/10/13 12:27, Mark Thomas wrote:

On 02/10/2013 12:23, Brian Burch wrote:

On 02/10/13 09:21, Mark Thomas wrote:

On 01/10/2013 22:56, Konstantin Preißer wrote:

Hi Mark,


-Original Message-
From: Mark Thomas [mailto:ma...@apache.org]
Sent: Tuesday, October 1, 2013 8:39 PM
To: Tomcat Developers List
Subject: 8.0.x / 7.0.x progress

Pulling together information from multiple threads:

8.0.x trunk appears to be stable (i.e. no longer crashes) but the
FormAuthenticator unit tests are very slow (24 minutes). This needs to
be resolved.


I noticed something strange with the Snake WebSocket example when
running current trunk (r1528211) on Windows (x64) with 64-bit version
of tcnative-1.dll (version 1.1.29 - from Mladen:
http://people.apache.org/~mturk/native/tomcat-native-win32-x86_64-r1528132.zip).



I've noticed a number of oddities as well. Fixing these is next on my
TODO list in case they highlight further issues with the APR/native
connector.


When I did some work on this class, I remember discussing how one or two
of the FormAuthenticator test cases took a while to run because they
have to wait for a timeout, but I haven't paid attention to run-times
recently.

I have a current and also a 3-week old version of tc8 trunk, and both
TestFormAuthenticator runs take over 4 minutes - I don't remember them
taking that long.

However, I am running my tests ONLY under nio... both apr and bio are
disabled in my build.properties.

If you want me to take a look, let me know. On the other hand, if you
think it is caused by your recent changes, I'll leave it alone for now.


Thanks for the offer. The problem was caused by my changes and should
now be fixed providing a native library from 1.1.x trunk is used.


I just updated to r1528436, verified build.properties.default was 
pointing to tomcat-native-1.1.28 and not over-ridden by 
build.properties. I ran a clean, build and run of TestFormAuthenticator 
under nio only... still 4 minutes.


testTimoutWithCookies is expected to take a long time, but 73 secs seems 
too long for me.


twelve other tests each take roughly 10 seconds, which also feels a bit 
too long.


If you are happy, then good. I'm happy because the tests still run 
sucessfully.


I intend to revisit these authenticator test suites soon for a clean-up, 
so I'll see whether I can reduce the run times without making them more 
complicated than necessary.


Brian


Mark


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




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