Hi Michael,
Thanks for review, it looks like BNF notation uses only a comma as a
separator
http://www.w3.org/Notation.html
...
<l>#<m>element
indicating at least l and at most m elements, each separated by one or
more commas (",").
...
And here is "qop" definition from https://tools.ietf.org/html/rfc2617
...
qop-options = "qop" "=" <"> 1#qop-value <">
qop-value = "auth" | "auth-int" | token
...
Please take a look at updated webrev:
http://cr.openjdk.java.net/~asmotrak/http_auth_digest/webrev.01/
Artem
On 12/22/2015 05:59 AM, Michael McMahon wrote:
Hi Artem,
On 04/12/15 11:41, Artem Smotrakov wrote:
Hello,
Please review this small fix for DigestAuthentication class.
1. Added a check in DigestAuthentication.setNonce(String) that nonce
is not null. NPE may happen if a buggy HTTP server returns
"WWW-Authenticate" header which doesn't contain a "nonce" field.
According to RFCs 2069 [1] and 2617 [2], this is not expected
behaviour, but it would be better if an HTTP client threw a checked
IOException instead of NPE.
That's fine.
2. Updated DigestAuthentication.setQop(String) method to accept both
a whitespace and a comma as a delimiter. RFC 2617 [2] says that "qop"
may contain more than one token, but it doesn't specify a delimiter
for "qop" field in "WWW-Authenticate" header. There is an example of
"WWW-Authenticate" header in RFC 2617 [2] where a comma is used as a
delimiter of value in "qop" field.
It looks like the BNF specification mandates a comma and optional
linear white space.
So, the old code was buggy, but we didn't see the problem because
there is typically
only at most ever one value used for the qop field. But, to be
strictly correct, we would
have to check for TABs also. So, I think the correct behavior is to
delimit using comma
and remove any white space
- Michael.
3. Added a test for Digest authentication.
Bug: https://bugs.openjdk.java.net/browse/JDK-8138990
Webrev: http://cr.openjdk.java.net/~asmotrak/http_auth_digest/webrev.00/
[1] https://tools.ietf.org/html/rfc2069
[2] https://tools.ietf.org/html/rfc2617
Artem
