RFR 8050427 LoginContext tests to cover JDK-4703361

[Weijun Wang]

Looks fine. I'm glad you also fix the trailing whitespaces.

Thanks
Max

On 09/24/2015 10:11 AM, Amanda Jiang wrote:
> Hi Max,
>
> Thanks for your suggestions!  Two places were fixed and I also edited
> codes a little bit to make test clear.
> Please check latest webrev:
> http://cr.openjdk.java.net/~amjiang/8050427/webrev.03/
>
> Thanks,
> Amanda
>
> On 9/22/15, 6:45 PM, Weijun Wang wrote:
>> OK, the code is much easier to understand now.
>>
>> In some places, there are modern and simple ways to code. For example,
>> in SmartLoginModule.java,
>>
>>  170         // verify the username/password
>>  171         boolean usernameCorrect = false;
>>  172         boolean passwordCorrect = true;
>>  173         if (username.equals(myUsername)) {
>>  174             usernameCorrect = true;
>>  175         }
>>  176
>>  177         if (!usernameCorrect || password.length != 13) {
>>  178             passwordCorrect = false;
>>  179         }
>>  180
>>  181         for (int i = 0; i < password.length; i++) {
>>  182             if (password[i] != myPassword[i]) {
>>  183                 passwordCorrect = false;
>>  184             }
>>  185
>>
>> can be as simple as
>>
>>   if (username.equals(myUsername)
>>       && Arrays.equals(password, myPassword))
>>
>> Also, in MyConfiguration.java:
>>
>>   52         AppConfigurationEntry.LoginModuleControlFlag[] flags
>>   53                 = new
>> AppConfigurationEntry.LoginModuleControlFlag[2];
>>   54         flags[0] = optionOrder ? OPTIONAL : REQUIRED;
>>   55         flags[1] = optionOrder ? SUFFICIENT : REQUIRED;
>>   56         ptAE[0] = new AppConfigurationEntry("SmartLoginModule",
>>   57                 flags[0],
>>   58                 map);
>>   59         ptAE[1] = new AppConfigurationEntry("DummyLoginModule",
>>   60                 flags[1],
>>   61                 map);
>>
>> It seems unnecessary to introduce the flags variable.
>>
>> Thanks
>> Max
>>
>> On 09/22/2015 11:49 PM, Amanda Jiang wrote:
>>> Hi Max,
>>>
>>> Thanks for your comments ,please check updated webrev and my replies
>>> inline.
>>> webrev: http://cr.openjdk.java.net/~amjiang/8050427/webrev.02/
>>>
>>> Regards,
>>> Amanda
>>>
>>> On 8/17/15, 12:29 AM, Weijun Wang wrote:
>>>> There are something I don't understand.
>>>>
>>>> MyConfiguration.java:
>>>>
>>>> - optionOrder: Is it possible to make this an argument of the
>>>> constructor?
>>>      Fixed, made this an argument of the constructor.
>>>>
>>>> - getConfiguration/setConfiguration: If these are useless, why adding
>>>> these methods?
>>>      Removed useless methods.
>>>>
>>>> SmartLoginModule.java:
>>>>
>>>> - initialize: Why don't you use the callbackHandler argument?
>>>      See below.
>>>>
>>>> - shouldSucceed: It is always very confusing to make a field
>>>> accessible from outside a class.
>>>      Originally the test tried to test with different password ,
>>> "shouldSucceed" is used to control which password will be set.
>>>      MycallbackHandler is called in initialize( ) method and
>>> "shouldSucceed" is parsed as an argument to control which password would
>>> be set to "PasswdCallback"
>>>
>>>   123         this.callbackHandler = new MyCallbackHandler(myUsername,
>>> myPassword,
>>>   124                 shouldSucceed);
>>>
>>>   341     public MyCallbackHandler(String username, char[] password,
>>> boolean action) {
>>>   342         super();
>>>   343         userName = username;
>>>   344         this.password = password;
>>>   345         this.action = action;
>>>   346     }
>>>
>>>   361                 PasswordCallback pc = (PasswordCallback) callback;
>>>   362                 if (action) {
>>>   363                     pc.setPassword(password);
>>>   364                 } else {
>>>   365                     pc.setPassword(wrongpd);
>>>   366                 }
>>>
>>>      I agree with you this may be confusing, so I simplify this test,
>>> "shouldSucceed" value is remove, please check updated webrev.
>>>>
>>>> DynamicConfigurationTest.java:
>>>>
>>>> - test: why the if checks are based on both isNegative and success?
>>>> Why not only on isNegative? If you want to test 2 stages (initialize
>>>> and login), you can provide 2 isNegative flags.
>>>           "success" was for controlling correct/wrong passwords,
>>> "isNegative" was for checking if exception is expected.
>>>            Tests are re-organized , so I do not use these two flags
>>> now,  please check updated webrev.
>>>>
>>>> Thanks
>>>> Max
>>>>
>>>>
>>>> On 08/17/2015 01:39 PM, Amanda Jiang wrote:
>>>>> Hi All,
>>>>>
>>>>> Please be free to review these new tests for Dynamic configuration of
>>>>> authentication modules.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8050427
>>>>> webrev:http://cr.openjdk.java.net/~amjiang/8050427/webrev.01/
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Amanda
>>>>>
>>>>>
>>>
>>>
>>>
>