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
