On Sun, 29 Sep 2024 16:46:06 GMT, Michael McMahon <micha...@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
