On Thu, 9 May 2024 12:49:09 GMT, Nizar Benalla <[email protected]> wrote:
>> Passes Tier 1-3
>> Please review this change that aims to fix a bug when parsing the client's
>> request.
>>
>> RFC 9110 states
>>
>>> 11. HTTP Authentication 11.1. Authentication Scheme
>> HTTP provides a general framework for access control and authentication, via
>> an extensible set of challenge-response authentication schemes, which can be
>> used by a server to challenge a client request and by a client to provide
>> authentication information. It uses a **case-insensitive** token to identify
>> the authentication scheme:
>> ```auth-scheme = token```
>>
>> But in `BasicAuthenticator#authenticate` it was done in a case sensitive
>> manner
>>
>> TIA
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Declare `ServerAuthenticator.invoked` as volatile
test/jdk/com/sun/net/httpserver/BasicAuthToken.java line 28:
> 26: * @bug 8144100
> 27: * @summary checking token sent by client should be done in
> case-insensitive manner
> 28: * @run main/othervm BasicAuthToken
Hello Nizar, I couldn't spot anything in the test that forces the use of
"othervm". Is the othervm intentional? If not, I would recommend changing it to
just "main".
test/jdk/com/sun/net/httpserver/BasicAuthToken.java line 91:
> 89:
> 90: if (!statusLine.startsWith("HTTP/1.1 200")) {
> 91: throw new RuntimeException("Basic Authentication failed:
> case-insensitive token" +
Nit: Might be better to just state:
throw new RuntimeException("unexpected status line: " + statusLine);
test/jdk/com/sun/net/httpserver/BasicAuthToken.java line 94:
> 92: " used to identify authentication scheme sent by
> client parsed incorrectly");
> 93: }
> 94: assert ServerAuthenticator.wasChecked() : "Authenticator was
> not correctly invoked";
As far as I know, the jtreg tests that we launch (through make) will always
have asserts enabled. So I think using `assert` here is OK. However, to be
consistent with other conditional checks in this test, I think it would be
better to change this to a if block, something like:
if (!ServerAuthenticator.wasChecked()) {
throw new RuntimeException("Authenticator wasn't invoked");
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19133#discussion_r1596400791
PR Review Comment: https://git.openjdk.org/jdk/pull/19133#discussion_r1596401728
PR Review Comment: https://git.openjdk.org/jdk/pull/19133#discussion_r1596404582
