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

[Jaikiran Pai]

On Wed, 8 May 2024 04:23:47 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

test/jdk/com/sun/net/httpserver/BasicAuthToken.java line 28:

> 26:  * @bug 8144100
> 27:  * @run main/othervm BasicAuthToken
> 28:  * @summary checking token sent by client should be done in case-insensitive manner

As a guideline, jtreg recommended order of test definition tags is here https://openjdk.org/jtreg/tag-spec.html#ORDER. As per that the `@summary` should follow the `@bug` before the `@run`.

test/jdk/com/sun/net/httpserver/BasicAuthToken.java line 58:

> 56:         String realm = "someRealm";
> 57:         ServerAuthenticator authenticator = new ServerAuthenticator(realm);
> 58:         HttpServer server = HttpServer.create(new InetSocketAddress(0), 0);

Hello Nizar, we should use loopback address to bind the server to. That's the common practice we follow in our networking tests. So:


new InetSocketAddress(InetAddress.getLoopbackAddress(), 0)) ...

test/jdk/com/sun/net/httpserver/BasicAuthToken.java line 61:

> 59:         server.createContext(someContext, exchange -> {
> 60:             if (authenticator.authenticate(exchange) instanceof Authenticator.Failure) {
> 61:                 exchange.sendResponseHeaders(401, 0);

The second parameter here should be `-1` implying no response body. `0` implies chunked response.

test/jdk/com/sun/net/httpserver/BasicAuthToken.java line 65:

> 63:                 return;
> 64:             }
> 65:             exchange.sendResponseHeaders(200, 1);

Second parameter here should be `-1`. `1` implies the response will contain a body of 1 byte in length.

test/jdk/com/sun/net/httpserver/BasicAuthToken.java line 73:

> 71: 
> 72:     static void client(int port) throws Exception {
> 73:         try (Socket socket = new Socket("localhost", port)) {

Once you change the server to bind to loopback address, the Socket creation should then use loopback address too: `new Socket(InetAddress.getLoopbackAddress(), port)`

test/jdk/com/sun/net/httpserver/BasicAuthToken.java line 89:

> 87:             System.err.println(statusLine);
> 88: 
> 89:             if (statusLine.startsWith("HTTP/1.1 401")) {

It might be better to check that the status line is `200` and anything other than that should result in an exception from here.

test/jdk/com/sun/net/httpserver/BasicAuthToken.java line 97:

> 95: 
> 96:     static class ServerAuthenticator extends BasicAuthenticator {
> 97:         ServerAuthenticator(String realm) {

Since this test relies on the authenticator being invoked, just as an additional check, you might want to have a boolean field in this class (named something like `invoked`) which will be set to `true` only when the `checkCredentials` gets called. Then after checking the response code in the test, you can additionally assert that this field's value is `true`, just to be certain that this authenticator was indeed invoked.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19133#discussion_r1593895416
PR Review Comment: https://git.openjdk.org/jdk/pull/19133#discussion_r1593874356
PR Review Comment: https://git.openjdk.org/jdk/pull/19133#discussion_r1593886268
PR Review Comment: https://git.openjdk.org/jdk/pull/19133#discussion_r1593887118
PR Review Comment: https://git.openjdk.org/jdk/pull/19133#discussion_r1593896857
PR Review Comment: https://git.openjdk.org/jdk/pull/19133#discussion_r1593888199
PR Review Comment: https://git.openjdk.org/jdk/pull/19133#discussion_r1593891938