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

[Daniel Jeliński]

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

This looks great.

src/java.net.http/share/classes/jdk/internal/net/http/AuthenticationFilter.java line 247:

> 245:         HttpRequestImpl req = r.request();
> 246: 
> 247:         if (req.getUserSetAuthFlag(SERVER) && status == 401) {

Suggestion:

        if (req.getUserSetAuthFlag(SERVER) && status == UNAUTHORIZED) {

src/java.net.http/share/classes/jdk/internal/net/http/AuthenticationFilter.java line 250:

> 248:             // return the response. We don't handle it.
> 249:             return null;
> 250:         } else if (req.getUserSetAuthFlag(PROXY) && status == 407) {

Suggestion:

        } else if (req.getUserSetAuthFlag(PROXY) && status == PROXY_UNAUTHORIZED) {

src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 781:

> 779:         HttpHeaders userh = filterHeaders(request.getUserHeaders());
> 780:         // Filter context restricted from userHeaders
> 781:         userh = HttpHeaders.of(userh.map(), (k, v) -> true);

Suggestion:

        userh = HttpHeaders.of(userh.map(), Utils.ACCEPT_ALL);

src/java.net.http/share/classes/jdk/internal/net/http/common/Utils.java line 218:

> 216: 
> 217:     private static final BiPredicate<String, String> HOST_RESTRICTED = (k,v) -> !"host".equalsIgnoreCase(k);
> 218:     public static final BiPredicate<String, String> PROXY_TUNNEL_RESTRICTED(HttpClient client)  {

Please merge these two. Also, the `client` parameter is no longer used.

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

> 152: 
> 153:             var proxyStr = proxyMock.getRequest(0);
> 154:             assertEquals(407, response.statusCode());

could you also assert that the proxy authenticator was not called here?

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

Marked as reviewed by djelinski (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21408#pullrequestreview-2362206218
PR Review Comment: https://git.openjdk.org/jdk/pull/21408#discussion_r1796584154
PR Review Comment: https://git.openjdk.org/jdk/pull/21408#discussion_r1796584577
PR Review Comment: https://git.openjdk.org/jdk/pull/21408#discussion_r1796588478
PR Review Comment: https://git.openjdk.org/jdk/pull/21408#discussion_r1796589417
PR Review Comment: https://git.openjdk.org/jdk/pull/21408#discussion_r1796595440