Skip to content

Commit 5e0fcf7

Browse files
jeremiahjstaceykwwall
jeremiahjstacey
authored and
kwwall
committed
AuthenticatorTest Stability #360 (#467)
* AuthenticatorTest Stability Several changes that appear to improve the stability of the overall test. First, broke testSetCurrentUserWithRequest into five uniquely named tests. One of these is used to verify a user with an expired account cannot log in. Fix here is to explicitly set the expiration time in the past to prevent the implementation from validating in the same millisecond. Second, updated the internal runnable in testSetCurrentUser to generate a more complex password. Found that the previous password would occasionally fail the FileBasedAuthenticator password strength check. I assumed it was because the strength value was being calculated as either 8 or 16. Stregth would evaluate to 8 if the 8-character string was composed of only one unique type of character (upper/lower/number/special), 16 if there were only 2 unique types. By setting the string length to 17 we explicitly avoid this potential failure in testing. Third, I had implemented a section of code in the tear-down behavior that cleared the userDB file and used Whitebox to null the 'userDB' attribute in the FileBasedAuthenticator instance. Even with that I was still seeing issues in testSetCurrentUser where the test would fail because the user account would already exist. To account for this, I modified all user names to be randomly generated Strings which will avoid the test unintentionally creating more than user by the same name. Finally, in testGetCurrentUser there was a value being tracked for the user compare 'result'. Though part of the tests' validation, it would not have failed the test since it was being checked within the Runnable context and not being propagated back to the test execution scope. I modified the implementation to add the boolean 'result' check to the ErrorCollector, which can fail the test if a mismatched user reference occurs. With these changes I can run this test in isolation 100/100 times successfully. * Authenticatortest Stability In addition to issues resolved in previous commit, an additional intermittent problem was identified having to do with file IO during testing. To account for the IO timing, the timeout of the implementation was extended from 10 seconds to 5 minutes. The timeout was originally applied to prevent the test from running indefinitely, without consideration for any performance requirements. The timeout extension still fulfills the original intent and accounts for issues we're currently experiencing. @kwwall confirmed that it works on Linux as well (where it had been timeout out with a 10 second timeout). Passed 100 out of 100 times there as well. Close issue #360
1 parent 074a6f2 commit 5e0fcf7

File tree

1 file changed

+109
-54
lines changed

1 file changed

+109
-54
lines changed

‎src/test/java/org/owasp/esapi/reference/AuthenticatorTest.java‎

Lines changed: 109 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
import static org.junit.Assert.fail;
2525
import static org.junit.Assume.assumeTrue;
2626

27-
import java.util.Date;
27+
import java.util.Calendar;
2828
import java.util.Set;
2929
import java.util.concurrent.CountDownLatch;
3030
import java.util.concurrent.Semaphore;
@@ -33,6 +33,7 @@
3333
import javax.servlet.http.HttpServletRequest;
3434
import javax.servlet.http.HttpServletResponse;
3535

36+
import org.hamcrest.core.IsEqual;
3637
import org.junit.After;
3738
import org.junit.Before;
3839
import org.junit.BeforeClass;
@@ -68,7 +69,7 @@ public class AuthenticatorTest {
6869
@Rule
6970
public ErrorCollector collector = new ErrorCollector();
7071
@Rule
71-
public Timeout testTimout = new Timeout(10, TimeUnit.SECONDS);
72+
public Timeout testTimout = new Timeout(5, TimeUnit.MINUTES);
7273
@Rule
7374
public TestName name = new TestName();
7475

@@ -207,13 +208,12 @@ public void cleanup() {
207208
assertFalse( currentUser.getAccountName().equals( user2.getAccountName() ) );
208209

209210
Runnable echo = new Runnable() {
210-
private int count = 1;
211-
private boolean result = false;
212211
public void run() {
213212
User a = null;
214213
try {
215214
String password = instance.generateStrongPassword();
216-
String accountName = "TestAccount" + count++;
215+
//Create account name using random strings to guarantee uniqueness among running threads.
216+
String accountName=ESAPI.randomizer().getRandomString(8, EncoderConstants.CHAR_ALPHANUMERICS);
217217
a = instance.getUser(accountName);
218218
if ( a != null ) {
219219
instance.removeUser(accountName);
@@ -225,7 +225,7 @@ public void run() {
225225
collector.addError(e);
226226
}
227227
User b = instance.getCurrentUser();
228-
result &= a.equals(b);
228+
collector.checkThat("Logged in user should equal original user", a.equals(b), new IsEqual<Boolean>(Boolean.TRUE));
229229
}
230230
};
231231
ThreadGroup tg = new ThreadGroup("test");
@@ -413,10 +413,7 @@ public void run() {
413413
*/
414414
@Test public void testSetCurrentUser() throws AuthenticationException, InterruptedException {
415415
System.out.println("setCurrentUser");
416-
System.err.println("AuthenticatorTest.setCurrentUser(): This test " +
417-
"occasionally fails due to some undiscovered race condition. " +
418-
"This has been reported as GitHub issue #360. Patches to fix welcome.");
419-
String user1 = ESAPI.randomizer().getRandomString(8, EncoderConstants.CHAR_UPPERS);
416+
String user1 = ESAPI.randomizer().getRandomString(8, EncoderConstants.CHAR_UPPERS);
420417
String user2 = ESAPI.randomizer().getRandomString(8, EncoderConstants.CHAR_UPPERS);
421418
User userOne = instance.createUser(user1, "getCurrentUser", "getCurrentUser");
422419
userOne.enable();
@@ -431,12 +428,13 @@ public void run() {
431428
assertFalse( currentUser.getAccountName().equals( userTwo.getAccountName() ) );
432429
final CountDownLatch latch = new CountDownLatch(10);
433430
Runnable echo = new Runnable() {
434-
private int count = 1;
435431
public void run() {
436432
User u=null;
437433
try {
438-
String password = ESAPI.randomizer().getRandomString(8, EncoderConstants.CHAR_ALPHANUMERICS);
439-
u = instance.createUser("test" + count++, password, password);
434+
//Increase pwd size to guarantee greater than (not "or equal to") 16 strength. See FileBasedAuthenticator 711-715
435+
String password = ESAPI.randomizer().getRandomString(17, EncoderConstants.CHAR_ALPHANUMERICS);
436+
String username = ESAPI.randomizer().getRandomString(8, EncoderConstants.CHAR_ALPHANUMERICS);
437+
u = instance.createUser(username, password, password);
440438
instance.setCurrentUser(u);
441439
ESAPI.getLogger("test").info( Logger.SECURITY_SUCCESS, "Got current user" );
442440
//If the user isn't removed every subsequent execution will fail because we cannot create a duplicate user of the same name!
@@ -457,53 +455,110 @@ public void run() {
457455
}
458456

459457

460-
/**
461-
* Test of setCurrentUser method, of class org.owasp.esapi.Authenticator.
462-
*
463-
* @throws AuthenticationException
464-
* the authentication exception
465-
*/
466-
@Test public void testSetCurrentUserWithRequest() throws AuthenticationException {
467-
System.out.println("setCurrentUser(req,resp)");
458+
459+
/**
460+
* Test of setCurrentUser method, of class org.owasp.esapi.Authenticator.
461+
*
462+
* @throws AuthenticationException
463+
* the authentication exception
464+
*/
465+
@Test public void testSetCurrentUserWithRequest() throws AuthenticationException {
468466
instance.logout(); // in case anyone is logged in
469-
String password = instance.generateStrongPassword();
470-
String accountName = ESAPI.randomizer().getRandomString(8, EncoderConstants.CHAR_ALPHANUMERICS);
471-
DefaultUser user = (DefaultUser) instance.createUser(accountName, password, password);
472-
user.enable();
473-
MockHttpServletRequest request = new MockHttpServletRequest();
474-
request.addParameter("username", accountName);
475-
request.addParameter("password", password);
476-
MockHttpServletResponse response = new MockHttpServletResponse();
477-
ESAPI.httpUtilities().setCurrentHTTP(request, response);
467+
String password = instance.generateStrongPassword();
468+
String accountName = ESAPI.randomizer().getRandomString(8, EncoderConstants.CHAR_ALPHANUMERICS);
469+
DefaultUser user = (DefaultUser) instance.createUser(accountName, password, password);
470+
user.enable();
471+
MockHttpServletRequest request = new MockHttpServletRequest();
472+
request.addParameter("username", accountName);
473+
request.addParameter("password", password);
474+
MockHttpServletResponse response = new MockHttpServletResponse();
475+
ESAPI.httpUtilities().setCurrentHTTP(request, response);
478476
User loggedIn = instance.login( request, response );
479477
User currentUser = instance.getCurrentUser();
480478
assertTrue(loggedIn.isLoggedIn());
481479
assertSame(currentUser, loggedIn);
482480
assertSame(user, loggedIn);
483-
try {
484-
user.disable();
485-
instance.login( request, response );
486-
fail();
487-
} catch( AuthenticationException e ) {
488-
// expected
489-
}
490-
try {
491-
user.enable();
492-
user.lock();
493-
instance.login( request, response );
494-
fail();
495-
} catch( AuthenticationException e ) {
496-
// expected
497-
}
498-
try {
499-
user.unlock();
500-
user.setExpirationTime( new Date() );
501-
instance.login( request, response );
502-
fail();
503-
} catch( AuthenticationException e ) {
504-
// expected
505-
}
506-
}
481+
}
482+
483+
/**
484+
* Test of setCurrentUser method, of class org.owasp.esapi.Authenticator.
485+
*
486+
* @throws AuthenticationException
487+
* the authentication exception
488+
*/
489+
@Test public void testSetCurrentUserWithRequestDisabledAccount() throws AuthenticationException {
490+
instance.logout(); // in case anyone is logged in
491+
String password = instance.generateStrongPassword();
492+
String accountName = ESAPI.randomizer().getRandomString(8, EncoderConstants.CHAR_ALPHANUMERICS);
493+
DefaultUser user = (DefaultUser) instance.createUser(accountName, password, password);
494+
user.enable();
495+
MockHttpServletRequest request = new MockHttpServletRequest();
496+
request.addParameter("username", accountName);
497+
request.addParameter("password", password);
498+
MockHttpServletResponse response = new MockHttpServletResponse();
499+
ESAPI.httpUtilities().setCurrentHTTP(request, response);
500+
try {
501+
user.disable();
502+
instance.login( request, response );
503+
fail("Disabled User Account should not be able to log in.");
504+
} catch( AuthenticationException e ) {
505+
// expected
506+
}
507+
}
508+
509+
/**
510+
* Test of setCurrentUser method, of class org.owasp.esapi.Authenticator.
511+
*
512+
* @throws AuthenticationException
513+
* the authentication exception
514+
*/
515+
@Test public void testSetCurrentUserWithRequestLockedAccount() throws AuthenticationException {
516+
instance.logout(); // in case anyone is logged in
517+
String password = instance.generateStrongPassword();
518+
String accountName = ESAPI.randomizer().getRandomString(8, EncoderConstants.CHAR_ALPHANUMERICS);
519+
DefaultUser user = (DefaultUser) instance.createUser(accountName, password, password);
520+
user.enable();
521+
MockHttpServletRequest request = new MockHttpServletRequest();
522+
request.addParameter("username", accountName);
523+
request.addParameter("password", password);
524+
MockHttpServletResponse response = new MockHttpServletResponse();
525+
ESAPI.httpUtilities().setCurrentHTTP(request, response);
526+
try {
527+
user.lock();
528+
instance.login( request, response );
529+
fail("Locked User Account should not be able to log in.");
530+
} catch( AuthenticationException e ) {
531+
// expected
532+
}
533+
}
534+
/**
535+
* Test of setCurrentUser method, of class org.owasp.esapi.Authenticator.
536+
*
537+
* @throws AuthenticationException
538+
* the authentication exception
539+
*/
540+
@Test public void testSetCurrentUserWithRequestExpiredAccount() throws AuthenticationException {
541+
instance.logout(); // in case anyone is logged in
542+
String password = instance.generateStrongPassword();
543+
String accountName = ESAPI.randomizer().getRandomString(8, EncoderConstants.CHAR_ALPHANUMERICS);
544+
DefaultUser user = (DefaultUser) instance.createUser(accountName, password, password);
545+
user.enable();
546+
MockHttpServletRequest request = new MockHttpServletRequest();
547+
request.addParameter("username", accountName);
548+
request.addParameter("password", password);
549+
MockHttpServletResponse response = new MockHttpServletResponse();
550+
ESAPI.httpUtilities().setCurrentHTTP(request, response);
551+
Calendar calendar = Calendar.getInstance();
552+
calendar.add(Calendar.MINUTE, -1);
553+
try {
554+
user.unlock();
555+
user.setExpirationTime( calendar.getTime() );
556+
instance.login( request, response );
557+
fail("Expired User account should not be allowed to log in.");
558+
} catch( AuthenticationException e ) {
559+
// expected
560+
}
561+
}
507562

508563

509564

0 commit comments

Comments
 (0)