RFR 8199849: HttpServer/BasicAuthenticator: unicode bytes are not correctly handled

[Daniel Fuchs]

On 06/08/2019 17:20, Michael McMahon wrote:
> Thanks Daniel. Suggested changes made and new webrev at:
> 
> http://cr.openjdk.java.net/~michaelm/8199849/webrev.4/

Thanks Michael. LGTM now!

best regards,

-- daniel

> 
> - Michael.
> 
> On 06/08/2019, 14:37, Daniel Fuchs wrote:
>> Hi Michael, Patrick,
>>
>> BasicAuthentication.java:
>>
>>   The two constructors might benefit from a bit of refactoring
>>   allowing them to share the code that encode the credentials.
>>
>> BasicAuthenticator.java:
>>
>>   76         if ("".equals(realm))
>>
>>   I'd suggest using
>>
>>              if (realm.isEmpty())
>>
>>   If you do so you might also decide to add a comment that
>>   this is an implicit null check and decide to remove the
>>   previous line: Objects.requiresNonNull(realm).
>>
>> BasicAuthenticatorCharset.java:
>>
>>   This appears as a new test, yet it has a copyright
>>   line 2005, 2019, and two @bug refs?
>>
>>   typo: setAuthenticatonPW
>>
>>  146         // should fail once with UNICODE_PW and unsupporting 
>> character set
>>  147         if (failCount != 1)
>>  148             throw new RuntimeException("Fail count exceeded: 1 != 
>> " + failCount);
>>
>>   well... OK. But technically the test could succeed
>>   if the failing charset didn't fail but the passing
>>   charset did.
>>
>>   It would be much better to test that the charset that
>>   is expected to fail fails and that the charset that
>>   is expected to pass passes...
>>
>> TestHttpUnicode.java
>>
>>   Are the copyright years OK or is this just
>>   copy-paste mistake?
>>
>>     44     static class testAuthenticator extends 
>> java.net.Authenticator {
>>
>>   can this class start with an upper case please?
>>   it's a bit confusing given that the server authenticator is a
>>   stored in a variable named `testAuthenticator` as well.
>>
>>
>> Otherwise, looks good to me!
>>
>> best regards,
>>
>> -- daniel
>>
>>
>> On 06/08/2019 12:44, Michael McMahon wrote:
>>> This change went through a couple of rounds of review before, and I 
>>> got diverted to something else
>>> at the time. So, just picking it up now.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8199849
>>>
>>> Webrev: http://cr.openjdk.java.net/~michaelm/8199849/webrev.3/index.html
>>>
>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8215275
>>>
>>> The two tests were contributed by Patrick Concannon.
>>>
>>> Thanks,
>>> Michael.
>>