Mailing-List: contact commons-httpclient-dev-help@jakarta.apache.org;
 run by ezmlm
Date: 21 Mar 2003 13:43:21 -0000
Message-ID: <20030321134321.11519.qmail@nagoya.betaversion.org>
From: bugzilla@apache.org
To: commons-httpclient-dev@jakarta.apache.org
Cc: 
Subject: DO NOT REPLY [Bug 17884]  -
    Multiple DIGEST authentication attempts with same credentials

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://nagoya.apache.org/bugzilla/show_bug.cgi?id=17884>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://nagoya.apache.org/bugzilla/show_bug.cgi?id=17884

Multiple DIGEST authentication attempts with same credentials


------- Additional Comments From adrian@ephox.com  2003-03-21 13:43 -------
Hi Oleg,
A few comments as I come across things:

* The NTLM class is to become package access in the near future, but this 
patch requires it to be public.  Probably should move the NTLM class into the 
auth package.  Final say on when this happens is up to Jandalf but I'm all for 
doing it now while you're refactoring.  Alternatively the NTLM class could be 
rolled into the NTLMScheme class, but it may make the class a bit big.

* The realm for NTLM has previously been considered to be the hostname of the 
server being connected to.  Without this assumption it is impossible to 
connect to two NTLM servers in parallel and requires work arounds even when 
connecting sequentially.  We could document getRealm to indicate that if it 
returns null, the realm should be considered to be the host name of the 
server, but I would prefer to see the domain name returned by getRealm.

* In NTLMScheme, there is a class variable called ntmlchallenge, it probably 
should be ntlmchallenge (note lm/ml difference).

* AuthChallengeParser.extractScheme returns the result in all lower case, 
however getSchemeName() returns the name with varying capitals. It seems 
neater to have AuthChallengeParser.extractScheme().equals(scheme.getSchemeName
()) if the scheme matches the challenge.

* There's currently no way to register a new scheme to be used, eventually we 
should add one, though pluggable auth modules wasn't scheduled until 2.1 or 
later (the lack of this ability probably renders the above point moot).

* Considering that at some point we are going to make schemes pluggable, 
should we add a "canHandle" method which can be used to determine if the 
Scheme can handle the given authentication challenge?

* Following along with the above, HttpAuthenticator.selectAuthScheme could be 
slightly more generic by using either the canHandle method if one is added or 
by using the getSchemeName if not to determine which scheme to return.  
Admittedly this code may get messy as unfortunately there's no way to enforce 
a class to have a constructor which takes just a String parameter.

* Authenticator seems to have almost exclusively become a wrapper for 
HttpAuthenticator.  Should we make it a full wrapper and depreciate the class? 
(Definitely don't remove it until after 2.0 though)

* Maybe I'm just up to late but there seems to be some redundant code on line 
216 of Authenticator:

headers = (Header [])headerlist.toArray(new Header[headerlist.size()]);
for (int i = 0; i < headers.length; i++) {
    headers[i] = (Header)headerlist.get(i);
}

It seems to me that toArray should copy the contents of the list into the 
array before returning, so there's then no need for the for loop.

Generally I think the architecture looks really good, I haven't really gone 
over the actual code implementation and haven't had a chance to set up tests 
yet, but will hopefully get to that tomorrow.  For now it's time for bed.

Hope that gives you something to think about anyway.