Password validation in Java

Question

I just wrote this password validation code function that checks to see if a password will be accepted in AD or not. I'm not sure this is the best way to do it, but for now it works fine. I would love some suggestions on how to make it better.

public String validateNewPass(String pass1, String pass2){
        StringBuilder retVal = new StringBuilder();

        if(pass1.length() < 1 || pass2.length() < 1 )retVal.append("Empty fields <br>");

        if (pass1 != null && pass2 != null) {

            if (pass1.equals(pass2)) {
                logger.info(pass1 + " = " + pass2);

                pass1 = pass2;
                boolean hasUppercase = !pass1.equals(pass1.toLowerCase());
                boolean hasLowercase = !pass1.equals(pass1.toUpperCase());
                boolean hasNumber = pass1.matches(".*\\d.*");
                boolean noSpecialChar = pass1.matches("[a-zA-Z0-9 ]*");

                if (pass1.length() < 11) {
                    logger.info(pass1 + " is length < 11");
                    retVal.append("Password is too short. Needs to have 11 characters <br>");
                }

                if (!hasUppercase) {
                    logger.info(pass1 + " <-- needs uppercase");
                    retVal.append("Password needs an upper case <br>");
                }

                if (!hasLowercase) {
                    logger.info(pass1 + " <-- needs lowercase");
                    retVal.append("Password needs a lowercase <br>");
                }

                if (!hasNumber) {
                    logger.info(pass1 + "<-- needs a number");
                    retVal.append("Password needs a number <br>");
                }

                if(noSpecialChar){
                    logger.info(pass1 + "<-- needs a specail character");
                    retVal.append("Password needs a special character i.e. !,@,#, etc.  <br>");
                }
            }else{
                logger.info(pass1 + " != " + pass2);
                retVal.append("Passwords don't match<br>");
            }
        }else{
            logger.info("Passwords = null");
            retVal.append("Passwords Null <br>");
        }
        if(retVal.length() == 0){
            logger.info("Password validates");
            retVal.append("Success");
        }

        return retVal.toString();

    }

**Note: These lines are to ensure a different case to handle a null vs empty string

if(pass1.length() < 1 || pass2.length() < 1 )retVal.append("Empty fields <br>");

        if (pass1 != null && pass2 != null) {

This begs for the question: why do you have to restrict the character set of passwords? That makes them much more insecure. You shouldn't be storing them in plain text anyway, and hash functions will yield equal length hashes for any input length. — alexia, Sep 20, 2014 at 17:15

janos · Accepted Answer · 2014-09-20 22:26:56Z

Buggy behavior

First you're checking the length of the passwords, and then if they are null:

if(pass1.length() < 1 || pass2.length() < 1 )retVal.append("Empty fields <br>");

if (pass1 != null && pass2 != null) {

This is not going to work well: if any of the passwords were null, you would get a NullPointerException when you check length.

Also, a better way to check if a string is empty is using pass1.isEmpty().

Also, this is pointless and potentially confusing:

pass1 = pass2;

Simplify the validation logic

It would be better and more efficient to create private final Pattern members that are compiled regular expressions, and reusable multiple times:

private final Pattern hasUppercase = Pattern.compile("[A-Z]");
private final Pattern hasLowercase = Pattern.compile("[a-z]");
private final Pattern hasNumber = Pattern.compile("\\d");
private final Pattern hasSpecialChar = Pattern.compile("[^a-zA-Z0-9 ]");

For example, this returns true if pass1 contains an uppercase character:

hasUppercase.matcher(pass1).find()

Notice the patter for hasSpecialChar: match non-alphabetic, non-digit, non-space.

Suggested implementation

Based on the above tips, you can simplify your implementation like this:

private final Pattern hasUppercase = Pattern.compile("[A-Z]");
private final Pattern hasLowercase = Pattern.compile("[a-z]");
private final Pattern hasNumber = Pattern.compile("\\d");
private final Pattern hasSpecialChar = Pattern.compile("[^a-zA-Z0-9 ]");

public String validateNewPass(String pass1, String pass2) {
    if (pass1 == null || pass2 == null) {
        logger.info("Passwords = null");
        return "One or both passwords are null";
    }

    StringBuilder retVal = new StringBuilder();

    if (pass1.isEmpty() || pass2.isEmpty()) {
        retVal.append("Empty fields <br>");
    }

    if (pass1.equals(pass2)) {
        logger.info(pass1 + " = " + pass2);

        if (pass1.length() < 11) {
            logger.info(pass1 + " is length < 11");
            retVal.append("Password is too short. Needs to have 11 characters <br>");
        }

        if (!hasUppercase.matcher(pass1).find()) {
            logger.info(pass1 + " <-- needs uppercase");
            retVal.append("Password needs an upper case <br>");
        }

        if (!hasLowercase.matcher(pass1).find()) {
            logger.info(pass1 + " <-- needs lowercase");
            retVal.append("Password needs a lowercase <br>");
        }

        if (!hasNumber.matcher(pass1).find()) {
            logger.info(pass1 + "<-- needs a number");
            retVal.append("Password needs a number <br>");
        }

        if (!hasSpecialChar.matcher(pass1).find()) {
            logger.info(pass1 + "<-- needs a specail character");
            retVal.append("Password needs a special character i.e. !,@,#, etc.  <br>");
        }
    } else {
        logger.info(pass1 + " != " + pass2);
        retVal.append("Passwords don't match<br>");
    }
    if (retVal.length() == 0) {
        logger.info("Password validates");
        retVal.append("Success");
    }

    return retVal.toString();
}

Support for unicode

If you want to allow unicode characters in the password, define the patterns using the special java character classes, as documented in the javadoc:

private final Pattern hasUppercase = Pattern.compile("\\p{javaUpperCase}");
private final Pattern hasLowercase = Pattern.compile("\\p{javaLowerCase}");
private final Pattern hasNumber = Pattern.compile("\\p{javaDigit}");
private final Pattern hasSpecialChar = Pattern.compile("[^\\p{javaLetterOrDigit} ]");

In some security cases, you may want to use a char[] to store passwords instead of a String, because Strings can be interned, substringed, etc., and referenced forever. On the other hand, char[]s won't. For an example, cf. JPasswordField#getPassword. — wchargin, Sep 19, 2014 at 1:25
@WChargin Substring does not refer to the same array anymore (unless you're using an old JRE). Strings can't be erased, array can. — maaartinus, Sep 19, 2014 at 1:34
@maaartinus right; forgot they changed that. Thanks for the correction. The principle still holds, of course. — wchargin, Sep 19, 2014 at 3:45
I would bail out immediately if the passwords don't match, rather than stuffing the main logic of the entire function into an indented if block. — AKHolland, Sep 19, 2014 at 19:45

rolfl · Accepted Answer · 2014-09-18 21:14:07Z

Your code is begging for the use of the Chain Of Responsibility patern (or rather, a variation of it).

But first, some observations:

You dereference the passwords before you check them for null:

public String validateNewPass(String pass1, String pass2){
        if(pass1.length() < 1 || pass2.length() < 1 )retVal.append("Empty fields <br>");
        if (pass1 != null && pass2 != null) {

if either password was null, you would get a NullPointerException before you check to see whether they are null.

Use String.isEmpty() instead of the length() calls.

Alternate algorithm

What you have is a bunch of requirements, which each need to be met, in order for the password to be appropriate. In Java8 this would be done with Functions, but, let's use an interface for the moment:

interface PasswordRule {
    boolean passRule(String password);
    String failMessage();
}

I would introduce an an abstract class, for convenience, and it would look like:

abstract static class BaseRule implements PasswordRule {
    private final String message;
    BaseRule(String message) {
        this.message = message;
    }

    public String failMessage() {
        return message;
    }
}

OK, so now you need to create an instance for each password rule:

private static final PasswordRule[] RULES = {
        new BaseRule("Password is too short. Needs to have 11 characters") {

            @Override
            public boolean passRule(String password) {
                return password.length() >= 11;
            }
        },

        new BaseRule("Password needs an upper case") {

            private final Pattern ucletter = Pattern.compile(".*[\\p{Lu}].*");
            @Override
            public boolean passRule(String password) {
                return ucletter.matcher(password).matches();
            }
        },

        /// .... more rules.
};

Now, your method becomes:

public static String validateNewPass(String pass1, String pass2){
    if(pass1 == null || pass2 == null) {
        //logger.info("Passwords = null");
        return "Passwords Null <br>";
    }

    if (pass1.isEmpty() || pass2.isEmpty()) {
        return "Empty fields <br>";
    }

    if (!pass1.equals(pass2)) {
        //logger.info(pass1 + " != " + pass2);
        return "Passwords don't match<br>";        
    }

    StringBuilder retVal = new StringBuilder();

    boolean pass = true;
    for (PasswordRule rule : RULES) {
        if (!rule.passRule(pass1)) {
            // logger.info(pass + "<--- " + rule.failMessage());
            retVal.append(rule.failMessage()).append(" <br>");
            pass = false;
        }
    }

    return pass ? "success" : retVal.toString();
}

As you add or refine the rules for the passwords, all you have to do is modify the RULES array.

Ingo Bürk · Accepted Answer · 2014-09-19 05:56:58Z

Something that I don't think has been mentioned:

Don't return strings like that. The logic should be separate from the representation. So do something like

public static enum PasswordValidationResult {
    SUCCESS, IS_EMPTY, DO_NOT_MATCH, TOO_SHORT, MISSING_UPPERCASE, MISSING_LOWERCASE, MISSING_NUMBER, MISSING_SPECIAL_CHARACTER
}

public PasswordValidationResult validatePassword( String password, String repeatedPassword ) {
    // ...
}

In fact, I would personally go for an even more sophisticated approach that also separates the types of results to make changes to the logic even easier. But this might be over-engineering it at this point. You get the idea.

maaartinus · Accepted Answer · 2014-09-19 01:31:37Z

In general, not bad.

logger.info(pass1 + " = " + pass2);

Logging passwords may be a security issue.

boolean hasUppercase = !pass1.equals(pass1.toLowerCase());

If you wanted to know if there's a black swan, would you paint all swans white and look if something changed?

boolean hasNumber = pass1.matches(".*\\d.*");
boolean noSpecialChar = pass1.matches("[a-zA-Z0-9 ]*");

Each of those lines would be fine, but together... 3 different ways for doing about the same thing. There are regexes for everything (ok, not really):

boolean hasUppercase = pass1.matches(".*[A-Z].*");

(assuming ASCII, otherwise there's a unicode group, too)

boolean hasSpecialChar = pass1.matches(".*[^0-9A-Za-z].*");

I negated you condition (so it's like the others) and the ^ negates the chargroup.

 logger.info(pass1 + " is length < 11");

A proper logger allows to write

 logger.info("{} is length < 11", pass1);

It's a bit more readable and much faster in case logging is off (and most logging is off most of the time).

The code is pretty repetitive. Can't you the returned value instead of the very similar message?

logger.info("Passwords = null");

Most of the time the best null handling is

Preconditions.checkNotNull(pass1);

which throws an NPE if passed null. Most methods should NOT allow null and throwing ASAP is the best - you fix the problem immediately and don't have to hunt it down later. Use

Strings.nullToEmpty(s);

to get rid of null strings where you can't throw.

Preconditions and Strings looks like Guava which I don't see mentioned anywhere. — DannyMo, Sep 18, 2014 at 22:31
@DannyMo Right. But these two (without formatting arguments) are so obvious, that it's hardly worth linking. — maaartinus, Sep 19, 2014 at 1:28

Malachi · Accepted Answer · 2014-09-22 13:26:32Z

I took your code and added some returns to it. you were running through all the if statements even when an entire field was missing.

I also added a return "success" if I hit that (which is probably checking all the expressions twice now if one isn't true) it dumps out as a success and will save some processing time.

this is what I came up with.

public String validateNewPass(String pass1, String pass2){
    StringBuilder retVal = new StringBuilder();

    if(pass1.length() < 1 || pass2.length() < 1 ){
        return "Empty fields <br>";
    }

    if (pass1 != null && pass2 != null) {

        if (pass1.equals(pass2)) {
            logger.info(pass1 + " = " + pass2);

            pass1 = pass2;
            boolean hasUppercase = !pass1.equals(pass1.toLowerCase());
            boolean hasLowercase = !pass1.equals(pass1.toUpperCase());
            boolean hasNumber = pass1.matches(".*\\d.*");
            boolean noSpecialChar = pass1.matches("[a-zA-Z0-9 ]*");

            if (hasUppercase && hasLowercase && hasNumber && !noSpecialChar && pass1.length) {
                logger.info("Password validates");
                return "success";
            }

            if (pass1.length() < 11) {
                logger.info(pass1 + " is length < 11");
                retVal.append("Password is too short. Needs to have 11 characters <br>");
            }

            if (!hasUppercase) {
                logger.info(pass1 + " <-- needs uppercase");
                retVal.append("Password needs an upper case <br>");
            }

            if (!hasLowercase) {
                logger.info(pass1 + " <-- needs lowercase");
                retVal.append("Password needs a lowercase <br>");
            }

            if (!hasNumber) {
                logger.info(pass1 + "<-- needs a number");
                retVal.append("Password needs a number <br>");
            }

            if(noSpecialChar){
                logger.info(pass1 + "<-- needs a specail character");
                retVal.append("Password needs a special character i.e. !,@,#, etc.  <br>");
            }
        }else{
            logger.info(pass1 + " != " + pass2);
            retVal.append("Passwords don't match<br>");
        }
    }else{
        logger.info("Passwords = null");
        return "Passwords Null <br>";
    }

    if(retVal.length() == 0){
        logger.info("Password validates");
        retVal.append("Success");
    }

    return retVal.toString();
}

You could probably return in every check. like if there isn't an uppercase, but that could get monotonous if you fail the checks one at a time.

So, instead I only returned after checks for things that would halt the rest of the checks entirely, which are

one of the password strings is null
one of the password strings is empty
the strings don't match
and success on all the rest of the checks

That last one is really iffy on whether or not it is redundant, because all of those are being checked twice if one of the checks fails. but the checks are actually only done once and then the value is stored in a boolean variable so we are only checking the value of the variable now which means that technically the check itself only happens once.

To do all of this I reversed some of the if statements and returned as early as I could without losing functionality or creating monotony. I also moved the null checking so that it is the first thing that is done, otherwise there will be a NullPointerException

By doing that we end up with this

public String validateNewPass(String pass1, String pass2){
    StringBuilder retVal = new StringBuilder();
    if (pass1 == null || pass2 == null) {
        logger.info("Passwords = null");
        return "Passwords Null <br>";
    }

    if(pass1.length() < 1 || pass2.length() < 1 ){
        return "Empty fields <br>";
    }

    if (!pass1.equals(pass2)) {
        logger.info(pass1 + " != " + pass2);
        return "Passwords don't match<br>";
    }

    logger.info(pass1 + " = " + pass2);

    pass1 = pass2;
    boolean hasUppercase = !pass1.equals(pass1.toLowerCase());
    boolean hasLowercase = !pass1.equals(pass1.toUpperCase());
    boolean hasNumber = pass1.matches(".*\\d.*");
    boolean noSpecialChar = pass1.matches("[a-zA-Z0-9 ]*");

    if (hasUppercase && hasLowercase && hasNumber && !noSpecialChar && pass1.length < 11) {
        logger.info("Password validates");
        return "success";
    }

    if (pass1.length() < 11) {
        logger.info(pass1 + " is length < 11");
        retVal.append("Password is too short. Needs to have 11 characters <br>");
    }

    if (!hasUppercase) {
        logger.info(pass1 + " <-- needs uppercase");
        retVal.append("Password needs an upper case <br>");
    }

    if (!hasLowercase) {
        logger.info(pass1 + " <-- needs lowercase");
        retVal.append("Password needs a lowercase <br>");
    }

    if (!hasNumber) {
        logger.info(pass1 + "<-- needs a number");
        retVal.append("Password needs a number <br>");
    }

    if(noSpecialChar){
        logger.info(pass1 + "<-- needs a specail character");
        retVal.append("Password needs a special character i.e. !,@,#, etc.  <br>");
    }

    if(retVal.length() == 0){
        logger.info("Password validates");
        return "Success";
    }

    return retVal.toString();
}

I realized that the very last if statement should never get evaluated as true, because everything has already been checked, so it is a redundant check, we can also get rid of this.

Returning after every check would yield a horrible user experience: imagine you enter a password that satisfies none of the constraints and than have to fix them one at a time, resubmitting in between. Better list everything at once. — 11684, Sep 19, 2014 at 17:10
@11684, I agree that is why I only returned after major ones that need to be remedied before any of the other checks can happen: either is null, either is empty, one doesn't match the other, and return on success (even though this may perform all the checks already it is readable and clear. and we could actually get rid of the very last if statement, because it should never evaluate to true because that is already covered. — Malachi, Sep 19, 2014 at 17:29
No need for extra parentheses when negating a function call. if(!foo.equals(baz)) will work fine. — Ingo Bürk, Sep 20, 2014 at 23:41
Thank you @IngoBürk, I am always nervous about how the compiler is going to interpret things, so I use Parenthesis to make sure that I know what order things are going to happen in. I did that in other places in the code, don't know why I didn't in that specific instance... I will edit it though. — Malachi, Sep 22, 2014 at 13:25

Community · Accepted Answer · 2017-04-13 12:40:52Z

Almost everything as been said about your code., but there is at least one last thing to say that is very important, that maaartinus mention in his answer : don't log passwords. Ever.

In this nature, passwords should be secret and known only to the holder of the account. I hope when you're storing your password you will do everything to protect it (hash, salt, etc). The problem is if you logged it, no matter how you protected it when you stored it, it will be logged forever in clear text. Logs are generally accessible and can be read by people that should probably not see the password of your user. This is a security issue and could be a severe security issue.

If you needed some output, you could use logger.debug which is most of time not activated in production, but keep in mind that you should not trust logging configurations in terms of security. The best option is to remove the password from the logging.

Glad to hear that. I'll leave my answer as a "warning" for future reader! — Marc-Andre, Sep 22, 2014 at 14:11

Kirill Rakhman · Accepted Answer · 2014-09-20 12:36:56Z

0

Besides all the other useful answers, I would suggest splitting up the password validation from the HTML generation. You could define an enum with values for the different types of errors + success and return one of those. Then you can easily write some unit tests to verify the correct behaviour of this method. Finally a separate method would call the validation routine and then generate the appropriate HTML.

answered Sep 20, 2014 at 12:36

Kirill Rakhman

1825 bronze badges

2

\$\begingroup\$ This is exactly what I wrote in my answer. \$\endgroup\$
– Ingo Bürk
Sep 20, 2014 at 23:42

Add a comment |

Stack Exchange Network

Password validation in Java

7 Answers 7

Buggy behavior

Simplify the validation logic

Suggested implementation

Support for unicode

Your Answer

Not the answer you're looking for? Browse other questions tagged
java
performance
validation
or ask your own question.

Linked

Hot Network Questions

Password validation in Java

7 Answers 7

Buggy behavior

Simplify the validation logic

Suggested implementation

Support for unicode

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged javaperformancevalidation or ask your own question.

Linked

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
java
performance
validation
or ask your own question.