On Wed, 16 Oct 2024 10:48:46 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:
> 
>   updated apidoc

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

> 135:                     .executor(ex);
> 136: 
> 137:             //if (!useHeader) {

do you plan to keep this commented-out code?

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

> 146:                     .build();
> 147: 
> 148:             HttpResponse<String> resp = client.send(req, 
> HttpResponse.BodyHandlers.ofString());

Can you assert that the response code is as expected?

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

> 148:             HttpResponse<String> resp = client.send(req, 
> HttpResponse.BodyHandlers.ofString());
> 149:             if (useHeader) {
> 150:                 assertTrue(h.authValue() == null, "Expected user set 
> header to be set");

authValue is null if the handler was never called; is that what we want to 
check here?

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

> 152:                 System.out.println("h2Test: using user set header OK");
> 153:             } else {
> 154:                 assertTrue(!h.authValue().equals(encoded), "Expected 
> user set header to not be set");

could you check that the authValue is equal to the expected value?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21408#discussion_r1803139315
PR Review Comment: https://git.openjdk.org/jdk/pull/21408#discussion_r1803146782
PR Review Comment: https://git.openjdk.org/jdk/pull/21408#discussion_r1803141600
PR Review Comment: https://git.openjdk.org/jdk/pull/21408#discussion_r1803144480

Reply via email to