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 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
