Artem,
The change looks fine.
Thanks,
Michael
On 04/01/16 18:39, Artem Smotrakov wrote:
Hi Michael,
On 01/04/2016 02:28 AM, Michael McMahon wrote:
On 30/12/15 03:22, Artem Smotrakov wrote:
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 (",").
...
Hi Artem,
The BNF definitions used in RFC 2617 come from section 2.1 of RFC 2616
which allows linear white space between tokens.
#rule
A construct "#" is defined, similar to "*", for defining lists of
elements. The full form is "<n>#<m>element" indicating at least
<n> and at most <m> elements, each separated by one or more commas
(",") and OPTIONAL linear white space (LWS).
If I read it correctly, it means that a comma is used as a separator,
but there also may be a linear whitespace after a comma. So for
example "auth,auth-int" and "auth, auth-int" are equal, and should
result to a list of two elements. Current version of webrev seems to
follow it (first, it splits a string by a comma, and then ignores
white spaces):
http://cr.openjdk.java.net/~asmotrak/http_auth_digest/webrev.01/
Am I missing something?
Artem
So, I think we should be conservative and check for white-space.
- Michael.
Michael
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
