Password generator using Decorator Pattern

Question

I made this password generator I'll use in a simple Android App.

I want that it generates a password using a truly random number source, but I don't have idea how to do it by now, so a let a seed parameter to generator class that will be created by that numbers source.

I used the decorator pattern to compose the source from which the chars will be picked up. So if I want a password in which the chars will be picked up from a source of numbers and lower case letters I will decorate it with NumericDomainRange and LowercaseDomainRange, but there is no warranty that the password generated will be composed of number and letters.

It must calculates the password entropy based on some algorithms, I have not implemented anything yet.

Please, review its overall design.

root package:

public class GenPass {
 public Password generate(int size, long seedFromTrulyRandomNumbersSource, DomainRange domainRange) {
 PasswordGenerator passwordGenerator = new SequencialPasswordGenerator(seedFromTrulyRandomNumbersSource);
 return passwordGenerator.generate(size, domainRange.getDomain());
 }
 public static void main(String[] args) {
 Password generatedPassword = new GenPass().generate(20, System.currentTimeMillis(), buildDomainRange());
 generatedPassword.setStrengthCalculator(getStrengthCalculator());
 System.out.println(generatedPassword);
 }
 private static PasswordEntropyStrengthCalculator getStrengthCalculator() {
 return new PasswordEntropyStrengthCalculator();
 }
 private static DomainRange buildDomainRange() {
 return new LowercaseDomainRange(new SpecialCharsDomainRange(new UppercaseDomainRange(new NumericDomainRange(new BaseDomainRange()))));
 }
}

---

password package:

public class Password implements HasStrength {
 private StrengthCalculator strengthCalculator = StrengthCalculator.EMPTY;
 private String password;
 public Password(String password) {
 this.password = password;
 }
 @Override
 public void setStrengthCalculator(StrengthCalculator strengthCalculator) {
 this.strengthCalculator = strengthCalculator;
 }
 @Override
 public double getStrength() {
 return strengthCalculator.calculate(password);
 }
 @Override
 public String toString() {
 return "Password{" +
 "strengthCalculator=" + getStrength() +
 ", password='" + password + '\'' +
 '}';
 }
}
public interface HasStrength {
 void setStrengthCalculator(StrengthCalculator strengthCalculator);
 double getStrength();
}
public class PasswordEntropyStrengthCalculator implements StrengthCalculator {
 @Override
 public double calculate(String password) {
 //TODO: to be implemented
 return 20;
 }
}
public interface StrengthCalculator {
 double calculate(String password);
 StrengthCalculator EMPTY = new StrengthCalculator() {
 @Override
 public double calculate(String password) {
 return -1;
 }
 };
}

---

generator package:

public interface PasswordGenerator {
 Password generate(int size, String domain);
}
public abstract class AbstractPasswordGenerator implements PasswordGenerator {
 private Random random;
 public AbstractPasswordGenerator(long seed) {
 this.random = new Random(seed);
 }
 protected int getRandomInt(int bound) {
 return random.nextInt(bound);
 }
}
public class SequencialPasswordGenerator extends AbstractPasswordGenerator {
 public SequencialPasswordGenerator(long seed) {
 super(seed);
 }
 @Override
 public Password generate(int size, String domain) {
 StringBuilder sb = new StringBuilder();
 for (int i = 0; i < size; i++) {
 int r = super.getRandomInt(domain.length());
 sb.append(domain.charAt(r));
 }
 return new Password(sb.toString());
 }
}

---

domainrange package:

public interface DomainRange {
 String getDomain();
}
public class BaseDomainRange implements DomainRange {
 @Override
 public String getDomain() {
 return "";
 }
}
public abstract class DomainRangeDecorator implements DomainRange {
 private DomainRange domainRange;
 public DomainRangeDecorator(DomainRange domainRange) {
 this.domainRange = domainRange;
 }
 @Override
 public String getDomain() {
 return domainRange.getDomain();
 }
}
public class LowercaseDomainRange extends DomainRangeDecorator {
 public LowercaseDomainRange(DomainRange domainRange) {
 super(domainRange);
 }
 @Override
 public String getDomain() {
 return super.getDomain() + Charset.ALPHA.getDomainRange();
 }
}
public class NumericDomainRange extends DomainRangeDecorator {
 public NumericDomainRange(DomainRange domainRange) {
 super(domainRange);
 }
 @Override
 public String getDomain() {
 return super.getDomain() + Charset.DIGITS.getDomainRange();
 }
}
public class SpecialCharsDomainRange extends DomainRangeDecorator {
 public SpecialCharsDomainRange(DomainRange domainRange) {
 super(domainRange);
 }
 @Override
 public String getDomain() {
 return super.getDomain().concat(Charset.SPECIAL.getDomainRange());
 }
}
public class UppercaseDomainRange extends DomainRangeDecorator {
 public UppercaseDomainRange(DomainRange domainRange) {
 super(domainRange);
 }
 @Override
 public String getDomain() {
 return super.getDomain().concat(Charset.ALPHA.getDomainRange().toUpperCase());
 }
}
public enum Charset {
 DIGITS("0123456789"), ALPHA("abcdefghijklmnopqrstuvxz"), SPECIAL("!#$%&()*+,-./:;<=>?@[]_|~");
 private String domainRange;
 Charset(String domainRange) {
 this.domainRange = domainRange;
 }
 public String getDomainRange() {
 return domainRange;
 }
}

• Zefick: You're writing a too complicated code. All you need is a one function using SecureRandom, may be with some arguments, but you have created a ton of classes and interfaces. Stop it until it is too late. Follow the KISS principle.
• alexpfx: I know it appear to be to much to a simple password generator, but I and almost all users cannot post real code here. So when people post a code like a tic-tac-toe solver or a 99 bottles of bear you have to abstract it and think of bigger things. And I shouldn't suggest you about it, it's implicit that this password generator could be whatever software functionality big or small that has to be easy to maintain and easy to expand when necessary, regardless of wheter this code is under the responsability of large teams or small teams.
• alexpfx: My request for code review is not to me to know that there is a mantra in software engineering that preaches keep the things simply. I know it and every software developer should know. But also all software engineers should know the difference about simplicity and simpleton, to rid the backs of other developers of having to mantain a simplistic code.
• alexpfx: Summarizing: It could be simple to make this same password generator using one or two methods and a bunch of IFs or a switch case structure, but I would never post it here because that would not allow me to learn anything.

Answer 1

I used the decorator pattern to compose the source from which the chars will be picked up. So if I want a password in which the chars will be picked up from a source of numbers and lower case letters I will decorate it with NumericDomainRange and LowercaseDomainRange, but there is no warranty that the password generated will be composed of number and letters.

I think the decorator pattern is the wrong approach to this. all you need is a CharacterSource interface and a list of its implementations where your PasswordGenerator selects one randomly when adding another char to the password:

interface CharacterSource {
 char get(int randomInt);
}

---

public class SequencialPasswordGenerator extends AbstractPasswordGenerator { 
 private final List<CharacterSource> characterSources
 public SequencialPasswordGenerator(long seed,
 List<CharacterSource> characterSources) {
 super(seed);
 this.characterSources=characterSources;
 }
 @Override
 public Password generate(int size, String domain) {
 StringBuilder sb = new StringBuilder();
 for (int i = 0; i < size; i++) {
 CharacterSource characterSource =
 characterSources.get(
 super.getRandomInt(characterSources.length()));
 int r = super.getRandomInt(domain.length());
 sb.append(characterSource.get(r));
 }
 return new Password(sb.toString());
 }
}

Answer 2

The Abstract class Decorator is too light

I find your DomainRangeDecorator abstract class a bit lacking in functionality. I understand you made it a backbone for all Domain Ranges, but what functionality does it centralize?

• It embeds the decorated DomainRange as a field (composite pattern, this is good)
• It forwards the getDomain method to that field (so it is not, in fact, abstract!)

If you go through the hassle of creating multiple classes and abstract prototypes, at least make it worthwhile. This class could do more to alleviate the other DomainRange implementations. For instance, you know the decorators will increase the Domain, and you know they'll do it by appending their allowed character set to the decorated character set. So make this 'append' operation centralized:

public abstract class DomainRangeDecorator implements DomainRange {
 private DomainRange domainRange;
 public DomainRangeDecorator(DomainRange domainRange) {
 this.domainRange = domainRange;
 }
 @Override
 public String getDomain() {
 return domainRange.getDomain().concat(additionalDomain());
 }
 protected abstract String additionalDomain();
}
public class UppercaseDomainRange extends DomainRangeDecorator {
 public UppercaseDomainRange(DomainRange domainRange) {
 super(domainRange);
 }
 @Override
 protected String additionalDomain() {
 return Charset.ALPHA.getDomainRange().toUpperCase();
 }
}

This is the point of inheritance: inheriting behaviours, not just avoiding to repeat implementation details. In your implementation, there was no behaviour to speak of.

Additionally, inheritance standardizes the behaviour: before, you had some classes append with + and some with concat. Now it's in one place, and you can enrich it (with a null check for example: don't trust your sub-classes!) and enjoy the added functionality everwhere else .

The PasswordGenerator is insufficient because the DomainRange lacks functionality

What does PasswordGenerator do right now? It picks chars at random in a list.

This gives no guarantee on the password. I could get abcd even though I prime the charset with:

return new LowercaseDomainRange(new SpecialCharsDomainRange(new UppercaseDomainRange(new NumericDomainRange(new BaseDomainRange()))));

Many website require at least a number, a symbol, a lowerscript etc.

You may well say:

That's OK, I'll add that functionality later, it's just like the Entropy Calculation.

But is it ok, though?

I believe you have a structural problem preventing you from solving the issue. Yes, you can check if the password matches some rules, and those rules could be "must contain a digit" as well as entropy must be >10". But how do you make sure your Generator has generated a password which does match the requested contraints?

You could loop until you find a satisfying password (which you need anyway to satisfy entropy checks), but solving many requirements like this would be inefficient.

Your Decorator structure is hiding away implementations (which is normal). The PasswordGenerator can only access basic DomainRange functionality, which does not include a constraint alleviation method like 'give me a char from your charset alpha/num/symbol. Even if you do expose such a method like getCharInRestrictedCharset(), you won't be able to satisfy them all, because the DomainRange may or may not be a decorator, and the decorated object may or may not be a decorator with constraints, etc, and you cannot access the inner DomainRanges for their own getCharInRestrictedCharset()` method.

By wrapping the charsets like Russian dolls, you've made the inner layers inaccessible. Making them accessible would break encapsulation.

---

If instead of an onion structure, the charsets were laid out "flat" (each accessible) then the password generator would be availble to pick from whichever charset it wants! Then constructing a password that satisfies constraints would be possible from the get-go.

You could have a List<DomainRange> in the PasswordGenerator. This way it can pick from a specific one to match a constraint, or from a random one.