Command line password generator

Question

I built a random password generator in Java which is meant to be run from the command line. It accepts options for:

• Length
• Upper case letters
• Lower case letters
• Special characters
• Numbers

The project is on BitBucket.

Example program execution :

java -jar program.jar -length 12 -number -upper -lower -special

This generates a 12 character long password with upper and lower case letters, numbers, and special characters.

What I think :

• I find my Parser class to be quite messy. It would be hard to add a new parameter with different properties. How would you do it?

Parser.java

package passManager;
import java.util.ArrayList;
import java.util.Arrays;
public class Parser {
 public static ArrayList<String> makeArgList(String args[]) {
 ArrayList<String> argList = new ArrayList<String>();
 boolean allArgsValid = true;
 // Checks if each arg is valid
 for (int i = 0; i < args.length && allArgsValid; i++) {
 // If it starts with - and is not in the list of parameters, bad
 if (args[i].startsWith("-") && !isInArgumentsList(args[i], Constants.VALID_ARGS)) {
 System.out.println("Invalid parameter: " + args[i] + ". Exiting.");
 allArgsValid = false;
 // If it's not a parameter and it isn't a number, bad
 } else if (!isInArgumentsList(args[i], Constants.VALID_ARGS) && !args[i].matches("^\\d+$")) {
 System.out.println("Invalid parameter: " + args[i] + ". Exiting.");
 allArgsValid = false;
 // Verify the length parameter
 } else if (args[i].equals("-length")) {
 // If length is the only parameter (-length or -length 5 only is
 // bad)
 if (i == 0 && (i + 1 == args.length || i + 2 == args.length)) {
 System.out.println("Length cannot be the only parameter. Exiting.");
 allArgsValid = false;
 // If what comes after -length is 0 or not a number
 } else if (!args[i + 1].matches("^\\d+$")) {
 System.out.println("Length must be a number. Exiting.");
 allArgsValid = false;
 }
 // If it starts with a number
 } else if (args[0].matches("^\\d+$")) {
 System.out.println("First argument cannot be number: " + args[i] + ". Exiting.");
 allArgsValid = false;
 // If it's a lone number
 } else if (args[i].matches("^\\d+$") && !args[i - 1].equals("-length")) {
 System.out.println("Invalid parameter: " + args[i] + ". Exiting.");
 allArgsValid = false;
 }
 }
 // Check that there must be a length argument, and at least another
 // arg.
 if (!Arrays.asList(args).contains("-length")) {
 if (args.length == 0) {
 System.out.println("Length and one other parameter are needed. Exiting.");
 allArgsValid = false;
 } else {
 System.out.println("Length is needed. Exiting.");
 allArgsValid = false;
 }
 }
 // If every arg was valid, then we can add all of them to the final list
 if (allArgsValid) {
 for (int i = 0; i < args.length; i++) {
 String cleanedElement = args[i].toString().replace("-", "");
 argList.add(cleanedElement);
 }
 return argList;
 }
 return null; // All args were not valid
}
private static boolean isInArgumentsList(String letter, String valid_args[]) {
 boolean valid = false;
 for (int i = 0; i < valid_args.length; i++) {
 if (letter.equals(valid_args[i])) {
 valid = true;
 }
 }
 return valid;
}

}

Main.java

public class Main {
public static void main(String args[]) {
 ArrayList<String> argList = new ArrayList<String>();
 argList = Parser.makeArgList(args);
 if (!(argList == null)) {
 System.out.println("\n+ Password Generated with the Following Attributes : ");
 for (int i = 0; i < argList.size(); i++) {
 System.out.println(". " + argList.get(i));
 }
 System.out.println("\n+ Your Generated Password is : ");
 System.out.println(Generator.generatePassword(argList));
 }
}

Generator.java

 package passManager;
 import java.util.ArrayList;
 import java.util.Random;
 public class Generator {
 private static int length = 0;
 private static StringBuilder WORKING_SET = new StringBuilder();
 private static Random random = new Random();
 public static String generatePassword(ArrayList<String> argList) {
 StringBuilder tempPassword = new StringBuilder();
 String finalStringPassword;
 makeWorkingSet(argList);
 // TODO bug when only arg is length
 for (int i = 0; i < length; i++) {
 int randomPosition = random.nextInt(WORKING_SET.length());
 tempPassword.append(WORKING_SET.charAt(randomPosition));
 }
 finalStringPassword = tempPassword.toString();
 return finalStringPassword;
 }
 private static void makeWorkingSet(ArrayList<String> argList) {
 for (int i = 0; i < argList.size(); i++) {
 switch (argList.get(i)) {
 case "length":
 length = Integer.parseInt(argList.get(i + 1));
 break;
 case "upper":
 WORKING_SET.append(Constants.UPPERLETTERS);
 break;
 case "lower":
 WORKING_SET.append(Constants.LOWERLETTERS);
 break;
 case "special":
 WORKING_SET.append(Constants.SPECIAL);
 break;
 case "number":
 WORKING_SET.append(Constants.DIGITS);
 break;
 }
 }
 }
 }

Constants.java

package passManager;
public interface Constants {
 final String[] VALID_ARGS = { "-length", "-special", "-lower", "-upper", "-number" };
 final String LOWERLETTERS = "abcdefghijklmnopqrstuvwxyz";
 final String UPPERLETTERS = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
 final String DIGITS = "0123456789";
 final String SPECIAL = "!@#@$%^&*()";
}

Answer 1

Command-line parsing

Instead of writing your bespoke methods to parse command-line options, consider using a library that can do that for you quite effortlessly. A quick Google search throws up the likes of Apache Commons CLI and JCommander.

Interfaces over implementations

You are passing around ArrayList as a method argument or return type, but all your methods need to know is just the List interface. Making this change is preferred as it allows you to swap different List implementations if required. E.g. if you are writing unit tests that can rely on Arrays.asList("some", "text") to give you a List, instead of having to create an ArrayList and then populate it.

static fields

Generator.WORKING_SET is a static StringBuilder, but it is used in a decidedly non-static way. You are building a String to generate your characters from, so this String should reside within the method. Leaving it as a static field allows it to be abused quite easily, e.g. concurrent calls made to modify WORKING_SET with different acceptable values may not generate the right randomized results.

Answer 2

public class Generator {
 private static int length = 0;
 private static StringBuilder WORKING_SET = new StringBuilder();
 private static Random random = new Random();

Note that this uses java.util.Random.

From the docs:

Instances of java.util.Random are not cryptographically secure. Consider instead using SecureRandom to get a cryptographically secure pseudo-random number generator for use by security-sensitive applications.

Therefore for a password generator, java.security.SecureRandom should be used instead. Failure to do this potentially results in a password generator that generates passwords that could be predictable by an attacker.

Answer 3

Semantic mismatch in Constants

You are mixing semantics in the class Constants. On one hand you enumerate allowed input parameters on the other hand you enumerate allowed characters. They will be changed on different reasons. Separate them into two classes (Single responsibility principle)

Semantic encapsulation

Tie together what belongs together. Connect the allowed characters to their label.

Object vs Class

Favor objects over classes as they are more flexible. They can be extended and mocked. That useful if you want to put them under test or reuse them.

Argument preparation

Try to prepare all the input parameters as early as possible so further algorithms are able to work on proper typed values.

Invalid parameter

The output of "Invalid parameter" is redundant by three. Remove redundancy by extracting it to a method and/or provide a better exception handling.

First argument cannot be number

This information has no more value than "Invalid parameter". Remove it.

Case specific adaption of check concept and parsing

On one hand you are checking towards (!args[i + 1].matches("^\d+$")) on the other hand you are checking backwards (!args[i - 1].equals("-length")). Try to establish a concept where you are only going only one direction.

Insufficient check

Once you determined that a parameter only contains digit you do not check this value to the end as this digit chain may not be a valid Integer. You pass around a half checked value.

Parameter reusage

Do not reuse the parameters passed in. You are removing "-" instead of using well defined labels.

Duplicate checks

You are checking "-length" twice. Try to consolidate this check with proper parsing.

Exception handling

Do not return null as the indicator. Throw an exception as soon as you identified a problem.

General

Parsing is a tricky thing. To be flexible in Parsing you need to build up structure and/or generic algorithm. Things will become very complicated as the parameters have different natures. To handle every state with a monolithic if-clause is hard.

The example I will provide adresses some things I mentioned. But the real flexibility you will only gain with library support and/or a state machine. The solution I provide will go only with no library support, little generic and lots of structure.

Code

Structures for allowed chars

public abstract class AllowedChars {
 private static Random RANDOM = new Random(System.currentTimeMillis());
 public abstract String getLabel();
 public abstract String getChars();
 public char getRandomChar() {
 return getChars().charAt(RANDOM.nextInt(getChars().length()));
 }
}
public class UpperChars extends AllowedChars {
 public static final AllowedChars INSTANCE = new UpperChars();
 private UpperChars() {
 }
 @Override
 public String getLabel() {
 return "upper";
 }
 @Override
 public String getChars() {
 return "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
 }
}
public class LowerChars extends AllowedChars {
 public static final AllowedChars INSTANCE = new LowerChars();
 private LowerChars() {
 }
 @Override
 public String getLabel() {
 return "lower";
 }
 @Override
 public String getChars() {
 return "abcdefghijklmnopqrstuvwxyz";
 }
}
public class SpecialChars extends AllowedChars {
 public static final AllowedChars INSTANCE = new SpecialChars();
 private SpecialChars() {
 }
 @Override
 public String getLabel() {
 return "special";
 }
 @Override
 public String getChars() {
 return "!@#@$%^&*()";
 }
}
public class Digits extends AllowedChars {
 public static final AllowedChars INSTANCE = new Digits();
 private Digits() {
 }
 @Override
 public String getLabel() {
 return "digits";
 }
 @Override
 public String getChars() {
 return "0123456789";
 }
}

The non-generic parser

public class Parser {
 private static final String LENGTH = "length";
 private static Map<String, AllowedChars> allowedCharsMap;
 private Iterator<String> iterator;
 private Parser(String[] args) {
 this.iterator = Arrays.asList(args).iterator();
 }
 private Map<String, AllowedChars> getAllowedCharsMap() {
 if (allowedCharsMap == null) {
 allowedCharsMap = new HashMap<String, AllowedChars>();
 allowedCharsMap.put(Digits.INSTANCE.getLabel(), Digits.INSTANCE);
 allowedCharsMap.put(LowerChars.INSTANCE.getLabel(), LowerChars.INSTANCE);
 allowedCharsMap.put(UpperChars.INSTANCE.getLabel(), UpperChars.INSTANCE);
 allowedCharsMap.put(SpecialChars.INSTANCE.getLabel(), SpecialChars.INSTANCE);
 }
 return allowedCharsMap;
 }
 public static Parameters parseParameters(String[] args) {
 return new Parser(args).parse();
 }
 private Parameters parse() {
 Integer length = null;
 Set<AllowedChars> allowedCharsSet = new HashSet<>();
 while (iterator.hasNext()) {
 String arg = iterator.next();
 if (arg.equals("-" + LENGTH)) {
 if (length == null) {
 length = parseLength();
 } else {
 throw new AmbigiousParameter(LENGTH);
 }
 } else {
 allowedCharsSet.add(parseAllowedChars(arg));
 }
 }
 if (length == null) {
 throw new MissingMandatoryParameter(LENGTH);
 }
 if (allowedCharsSet.size() == 0) {
 throw new MissingSelectionParameter(1, getAllowedCharsMap().keySet());
 }
 return new Parameters(length, allowedCharsSet);
 }
 private AllowedChars parseAllowedChars(String arg) {
 String expectedLabel = arg.substring(1);
 AllowedChars allowedChars = getAllowedCharsMap().get(expectedLabel);
 if (allowedChars == null) {
 throw new InvalidParameter(arg);
 }
 return allowedChars;
 }
 private Integer parseLength() {
 Integer length = null;
 if (iterator.hasNext()) {
 String potentialLength = iterator.next();
 try {
 length = Integer.parseInt(potentialLength);
 } catch (NumberFormatException e) {
 throw new InvalidParameterValue(LENGTH, potentialLength);
 }
 }
 return length;
 }
}

The generic parser exceptions

public class ParameterParserException extends RuntimeException {
 private static final long serialVersionUID = 4350796869418734580L;
 public ParameterParserException(String message) {
 super(message);
 }
}
public class InvalidParameter extends ParameterParserException {
 private static final long serialVersionUID = 8671765412229715992L;
 private String parameter;
 public InvalidParameter(String parameter) {
 super("Invalid parameter: '" + parameter + "'.");
 this.parameter = parameter;
 }
 public String getParameter() {
 return parameter;
 }
}
public class InvalidParameterValue extends ParameterParserException {
 private static final long serialVersionUID = -2783627149750723327L;
 private String value;
 private String parameter;
 public InvalidParameterValue(String parameter, String value) {
 super("Invalid value '" + value + "' for parameter '" + parameter + "'.");
 this.parameter = parameter;
 this.value = value;
 }
 public String getValue() {
 return value;
 }
 public String getParameter() {
 return parameter;
 }
}
public class MissingMandatoryParameter extends ParameterParserException {
 private static final long serialVersionUID = 7046928730434886663L;
 private String parameter;
 public MissingMandatoryParameter(String parameter) {
 super("Missing mandatory parameter '" + parameter + "'.");
 this.parameter = parameter;
 }
 public String getParameter() {
 return this.parameter;
 }
}
public class MissingSelectionParameter extends ParameterParserException {
 private static final long serialVersionUID = -8931460092684913629L;
 private int amount;
 private Set<String> outOf;
 public MissingSelectionParameter(int amount, Set<String> outOf) {
 super("Missing parameter(s). Select " + amount + " out of " + asString(outOf) + ".");
 this.amount = amount;
 this.outOf = outOf;
 }
 public int getAmount() {
 return amount;
 }
 public Set<String> getOutOf() {
 return outOf;
 }
 private static String asString(Set<String> outOf) {
 StringBuffer sb = new StringBuffer();
 Iterator<String> iterator = outOf.iterator();
 while (iterator.hasNext()) {
 String parameter = iterator.next();
 sb.append("'" + parameter + "'");
 if (iterator.hasNext()) {
 sb.append(", ");
 }
 }
 return sb.toString();
 }
}
public class AmbigiousParameter extends ParameterParserException {
 private static final long serialVersionUID = -7988874882284105200L;
 private String parameter;
 public AmbigiousParameter(String parameter) {
 super("Parameter '" + parameter + "' is ambigious.");
 this.parameter = parameter;
 }
 public String getParameter() {
 return parameter;
 }
}

Password generator

public class PasswordGenerator {
 private static final Random RANDOM = new Random(System.currentTimeMillis());
 private Parameters parameters;
 private PasswordGenerator(Parameters parameters) {
 this.parameters = parameters;
 }
 public static String generate(Parameters parameters) {
 return new PasswordGenerator(parameters).generate();
 }
 private String generate() {
 StringBuffer sb = new StringBuffer();
 for (int i = 0; i < parameters.getLength(); i++) {
 sb.append(getRandomChar());
 }
 return sb.toString();
 }
 private char getRandomChar() {
 return getRandomAllowedChars().getRandomChar();
 }
 private AllowedChars getRandomAllowedChars() {
 return parameters.getAllowedCharsList().get(RANDOM.nextInt(parameters.getAllowedCharsList().size()));
 }
}

Code in Action

public class Main {
 public static void main(String[] args1) {
 try {
 String[] args = new String[] {"-length", "10", "-upper", "-lower"};
 Parameters parameters = Parser.parseParameters(args);
 System.out.println(PasswordGenerator.generate(parameters));
 } catch (ParameterParserException e) {
 System.out.println(e.getMessage());
 }
 }
}