RFR 8199849: HttpServer/BasicAuthenticator: unicode bytes are not correctly handled
Michael McMahon
michael.x.mcmahon at oracle.com
Mon Aug 12 15:57:33 UTC 2019
I have made some more changes and there is a new webrev at:
http://cr.openjdk.java.net/~michaelm/8199849/webrev.5/
I would like to finalise the CSR soon also. So, any further
comments appreciated.
Thanks,
Michael
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/
>
> - 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.
>>
More information about the net-dev
mailing list