RFR: 8144100: Incorrect case-sensitive equality in com.sun.net.httpserver.BasicAuthenticator [v2]

[Jaikiran Pai]

On Thu, 9 May 2024 12:49:09 GMT, Nizar Benalla <duke at openjdk.org> 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