On Wed, 8 May 2024 04:23:47 GMT, Nizar Benalla <d...@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
