RFR: 8326949: Authorization header is removed when a proxy Authenticator is set on HttpClient

[Daniel Fuchs]

On Sun, 29 Sep 2024 16:46:06 GMT, Michael McMahon <michaelm at openjdk.org> wrote:

> This fix relaxes the constraints on user set authentication headers. Currently, any user set authentication headers are filtered out, if the HttpClient has an Authenticator set. The reason being that the authenticator is expected to manage authentication.  With this fix, it will be possible to use pre-emptive authentication through user set headers, even if an authenticator is set. The expected use case is where the authenticator would manage either proxy or server authentication and the user set headers would manage server authentication if the authenticator is managing proxy (or vice versa).
> If the pre-emptive authentication fails, then this behavior is disabled on further retries and it would be up to the authenticator to provide the right credentials then.
> 
> Thanks,
> Michael

The test seems to be missing a scenario where the provided preemptive credentials are wrong (or obsolete) and the stack then defaults to using the provided authenticator.

test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java line 90:

> 88:         try {
> 89: 
> 90:             var client = HttpClient.newBuilder()

Ideally should use try-with-resource to also close the client.

test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java line 99:

> 97:             var encoded = java.util.Base64.getEncoder().encodeToString(plainCreds.getBytes(US_ASCII));
> 98:             var badCreds = "user:wrong";
> 99:             var encoded1 = java.util.Base64.getEncoder().encodeToString(badCreds.getBytes(US_ASCII));

Is some part of the test missing? I don't see where `encoded1` is used.
Also should there be some assertion as to whether `ProxyAuth()` was called or not?

test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java line 127:

> 125:             var client = HttpClient.newBuilder()
> 126:                 .version(java.net.http.HttpClient.Version.HTTP_1_1)
> 127:                 .build();

should use try-with-resource to close the client

test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java line 155:

> 153:         serverMock.start();
> 154:         try {
> 155:             var client = HttpClient.newBuilder()

should use try-with-resource to close the client

test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java line 259:

> 257:         @Override
> 258:         public List<Proxy> select(URI uri) {
> 259:           return List.of(new Proxy(Proxy.Type.HTTP, new InetSocketAddress("localhost", port)));

It would be better to use `InetAddress.getLoopbackAddress()` rather than "localhost"

test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java line 282:

> 280:         protected PasswordAuthentication getPasswordAuthentication() {
> 281:             if (getRequestorType() != RequestorType.SERVER) {
> 282:                 // We only want to handle proxy authentication here

Suggestion:

                // We only want to handle server authentication here

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

PR Review: https://git.openjdk.org/jdk/pull/21249#pullrequestreview-2337431223
PR Review Comment: https://git.openjdk.org/jdk/pull/21249#discussion_r1781064380
PR Review Comment: https://git.openjdk.org/jdk/pull/21249#discussion_r1781059826
PR Review Comment: https://git.openjdk.org/jdk/pull/21249#discussion_r1781073209
PR Review Comment: https://git.openjdk.org/jdk/pull/21249#discussion_r1781074952
PR Review Comment: https://git.openjdk.org/jdk/pull/21249#discussion_r1781084303
PR Review Comment: https://git.openjdk.org/jdk/pull/21249#discussion_r1781086884