RFR 8050427 LoginContext tests to cover JDK-4703361

[Weijun Wang]

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