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

[Jaikiran Pai]

On Tue, 8 Oct 2024 15:43:35 GMT, Michael McMahon <michaelm at 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