On Tue, 8 Oct 2024 15:43:35 GMT, Michael McMahon <micha...@openjdk.org> wrote:
>> Hi, >> >> I closed https://github.com/openjdk/jdk/pull/21249 and am continuing the >> review on this PR. >> >> 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). >> >> A CSR will be filed to document this change. >> >> Thanks, >> Michael > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > test update Hello Michael, these changes look OK to me. I have added a few minor reviews comments in the test. Some of these files will need a copyright year update. test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java line 29: > 27: * @library /test/lib > 28: * @run main/othervm UserAuthWithAuthenticator > 29: * @summary Authorization header is removed when a proxy Authenticator is > set Nit - jtreg recommends the tag ordering as follows https://openjdk.org/jtreg/tag-spec.html#ORDER and I think it would be good to follow that recommendation here. test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java line 237: > 235: > 236: public Mocker(String[] responses) throws IOException { > 237: this.ss = new ServerSocket(0); Given some of the issues we see in our CIs, I think it would be better to use loopback address for this ServerSocket instead of the default wildcard address. test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java line 294: > 292: } catch (Exception e) { > 293: //e.printStackTrace(); > 294: } I think it might be OK to uncomment this and actually print the stacktrace, to help debug any unexpected failures. test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java line 333: > 331: protected PasswordAuthentication getPasswordAuthentication() { > 332: if (getRequestorType() != RequestorType.SERVER) { > 333: // We only want to handle proxy authentication here I suspect this is a typo - I think it should have been "We only want to handle server ...." ------------- Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21408#pullrequestreview-2362095907 PR Review Comment: https://git.openjdk.org/jdk/pull/21408#discussion_r1796509823 PR Review Comment: https://git.openjdk.org/jdk/pull/21408#discussion_r1796511540 PR Review Comment: https://git.openjdk.org/jdk/pull/21408#discussion_r1796512711 PR Review Comment: https://git.openjdk.org/jdk/pull/21408#discussion_r1796513968
