5
\$\begingroup\$

I'm currently trying a Password checker for password in Decorator Pattern, which will point out what the current password missing to be a strong password (my example will check for 8 characters, at least 1 uppercase, 1 lowercase, 1 number).

Please review and help to improve this code.

interface Password {
    String getPassword();
    boolean isValid();
    String getValidationErrors();
}

class BasePassword implements Password {
    private final String password;

    public BasePassword(String password) {
        this.password = password;
    }

    @Override
    public String getPassword() {
        return password;
    }

    @Override
    public boolean isValid() {
        return true;
    }

    @Override
    public String getValidationErrors() {
        return "";
    }
}

abstract class PasswordDecorator implements Password {
    protected final Password password;

    public PasswordDecorator(Password password) {
        this.password = password;
    }

    @Override
    public String getPassword() {
        return password.getPassword();
    }

    @Override
    public abstract boolean isValid();

    @Override
    public String getValidationErrors() {
        return password.getValidationErrors();
    }

}

class MinLengthValidator extends PasswordDecorator {
    private static final int MIN_LENGTH = 8;

    public MinLengthValidator(Password password) {
        super(password);
    }

    @Override
    public boolean isValid() {
        return password.getPassword().length() >= MIN_LENGTH;
    }

    @Override
    public String getValidationErrors() {
        String err = super.getValidationErrors();
        if (!isValid()) {
            err = err + "\n" + "+ Need at least 8 or more characters";
        }
        return err;
    }
}

class UppercaseValidator extends PasswordDecorator {
    public UppercaseValidator(Password password) {
        super(password);
    }

    @Override
    public boolean isValid() {
        return password.getPassword().chars().anyMatch(Character::isUpperCase);
    }

    @Override
    public String getValidationErrors() {
        String err = super.getValidationErrors();
        if (!isValid()) {
            err = err + "\n" + "+ Need at least one uppercase character";
        }
        return err;
    }
}

class NumberValidator extends PasswordDecorator {
    public NumberValidator(Password password) {
        super(password);
    }

    @Override
    public boolean isValid() {
        return password.getPassword().chars().anyMatch(Character::isDigit);
    }

    @Override
    public String getValidationErrors() {
        String err = super.getValidationErrors();
        if (!isValid()) {
            err = err + "\n" + "+ Need at least one number";
        }
        return err;
    }
}

class LowercaseValidator extends PasswordDecorator {
    public LowercaseValidator(Password password) {
        super(password);
    }

    @Override
    public boolean isValid() {
        return password.getPassword().chars().anyMatch(Character::isLowerCase);
    }

    @Override
    public String getValidationErrors() {
        String err = super.getValidationErrors();
        if (!isValid()) {
            err = err + "\n" + "+ Need at least one lowercase character";
        }
        return err;
    }
}

class PasswordValidator {
    public static String validate(String password) {
        Password decoratedPassword = new BasePassword(password);
        decoratedPassword = new MinLengthValidator(decoratedPassword);
        decoratedPassword = new UppercaseValidator(decoratedPassword);
        decoratedPassword = new NumberValidator(decoratedPassword);
        decoratedPassword = new LowercaseValidator(decoratedPassword);

        return decoratedPassword.getValidationErrors();
    }
}

public class Main {
    public static void main(String[] args) {
        String strongPassword = "StrongP@ss123";
        String weakPassword = "weakpas";

        System.out.println(PasswordValidator.validate(strongPassword)); //won't print anything
        System.out.println(PasswordValidator.validate(weakPassword));
    }
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ This might be beside the point of this example, but I’ll bring it up: it’s silly to care so much about strong passwords that you check them like this, then store them as plain text. :) \$\endgroup\$
    – Davislor
    Apr 16 at 22:11
  • \$\begingroup\$ @Davislor I feel like you have made an incorrect assumption as I did not see any code regarding storage in the question. It is fundamentally a necessity for the password to exist in unhashed format in RAM if it is to be checked against complexity requirements before storage. \$\endgroup\$ Apr 18 at 4:44
  • \$\begingroup\$ @TorbenPutkonen Perhaps. In that case, I'd actually suggest an Optional return value, which returns a String if and only if it contains a valid password. Then, the password checks repeatedly apply .filter to the Optional<String>. \$\endgroup\$
    – Davislor
    Apr 22 at 23:47
  • \$\begingroup\$ @Davislor That would be an Optional abuse, as this class is not part of a public interface. Anyway, this is a bit beyond the point since there are better approaches, as other reviewers have pointed out. \$\endgroup\$ Apr 23 at 4:11

3 Answers 3

3
\$\begingroup\$

Overall, your implementation looks good.

The example you've chosen is slightly contrived (you're reinventing the wheel in regard to validation, and passwords should be either immediately discarded if invalid or hashed, no point in keeping them in plain text regardless of their validity), but assuming that the code is written only in order to get familiar with the pattern, it's fine.

Design considerations

In the Decorator pattern we need to have a base contract, which is usually an interface, let's call it Foo (sometimes, for example in java.io, an abstract class can be used in this role).

There should be at least one basic implementation of Foo (sometimes way more than one). And several decorating implementations which have a field of type Foo.

The purpose of the pattern is to avoid the combinatorial explosion of types, i.e. there are a lot of combinations of additional features (which decorators represent) and basic implementations so that if every sensible combination would be implemented separately we end up with tons of classes. And that's what you've demonstrated: you have a basic implementation and 4 decorators as an equivalent of 16 classes, representing different combinations of features.

Also note, that the abstract decorator is not mandatory. Yes, in the GoF book there are concrete decorators and an abstract one, but any abstraction introduced in the code should be there to help in managing the complexity (not the opposite).

In your case, all method defined by the base interface are overridden in every decorator. Extending PasswordDecorator in decorating implementations only safes you from placing the field of type Password in each implementation, which is not a lot and if delegate Password would show up explicitly in a decorator it wouldn't be a bad thing.

But you can make the abstract decorator more useful by defining the logic for combining error messages and checking the validity inside it:

public abstract class PasswordDecorator implements Password {
    protected final Password delegate;
    
    public PasswordDecorator(Password delegate) {
        this.delegate = delegate;
    }
    
    @Override
    public String getPassword() {
        return delegate.getPassword();
    }
    
    public boolean isValid() {
        return verifyValidity() && delegate.isValid();
    }
    
    protected abstract boolean verifyValidity();
    
    @Override
    public String getValidationErrors() {
        return getError()
            + System.lineSeparator()
            + delegate.getValidationErrors();
    }
    
    protected abstract String getError();
}

While each concrete decorator would mind only performing its own validity check and issuing only an error message related to this check:

public class MinLengthValidator extends PasswordDecorator {
    private static final int MIN_LENGTH = 8;
    
    public MinLengthValidator(Main.Password password) {
        super(password);
    }
    
    @Override
    protected boolean verifyValidity() {
        return delegate.getPassword().length() >= MIN_LENGTH;
    }
    
    @Override
    protected String getError() {
        return "Need at least %s or more characters".formatted(MIN_LENGTH);
    }
}

Note that isValid() implementations in decorators in the code shared in the question are broken because they do not delegate the call to wrapped Password, but only performing their specific check.

Naming

You would advise using the name delegate (or something similar) instead of password, because it describes its role by telling that it's an object to which the wrapping decorator eventually delegates method calls performed on it.

And also because things like this password.getPassword() look slightly confusing for the reader.

Don't assume system-dependent properties

Line separator depends on the OS of the hosting machine (it might be "\n" or "\r\n").

Use System.lineSeparator().

Try to stay away from objects with invalid state

The idea of having validation in place and at the same time having a type that treats both valid and invalid data as normal isn't looking quite right.

As a consequence, a developer dialing with such a type is always forced to check whether the state of an instance is valid or not.

It's better to disallow illegal states to begin with.

And if you chose to do so, methods isValid() and getValidationErrors() would no longer be needed inside the Password (in fact, there would be no need in the Password object it itself). The proper place for these functionalities would be a separate stateless type:

public interface Validator<T> {
    void validate(T t) throws ValidationException;
    boolean isValid(T value);
    String getValidationErrors();
}

Note that we have an additional method validate which is intended to be used in the client code, and also isValid now expects an object to be validated as a parameter.

Here's the abstract decorator:

abstract class PasswordValidator implements Validator<String> {
    protected final Validator<? super String> delegate;
    
    public PasswordValidator (Validator<? super String> delegate) {
        this.delegate = delegate;
    }
    
    @Override
    public void validate(String value) {
        if (!isValid(value)) {
            throw new ValidationException(getValidationErrors());
        }
    }
    
    @Override
    public boolean isValid(String value) {
        return verifyValidity(value) && delegate.isValid(value);
    }
    
    protected abstract boolean verifyValidity(String value);
    
    @Override
    public java.lang.String getValidationErrors() {
        return getError()
            + System.lineSeparator()
            + delegate.getValidationErrors();
    }
    
    protected abstract String getError();
}

Concrete decorator example:

public class MinLengthValidator extends PasswordValidator {
    private static final int MIN_LENGTH = 8;
    
    public MinLengthValidator(Validator<? super String> delegate) {
        super(delegate);
    }
    
    @Override
    public boolean verifyValidity(String value) {
        return value.length() >= MIN_LENGTH;
    }
    
    @Override
    protected String getError() {
        return "Need at least %s or more characters".formatted(MIN_LENGTH);
    }
}

Validation exception is expected to carry the error messages issued by a validator:

public class ValidationException extends RuntimeException {
    public ValidationException(String message) {
        super(message);
    }
}

And the client code might look like this:

public class PasswordManagementService {
    private final Main.AbstractPasswordValidator validator;
    // ...
    
    public PasswordManagementService(Main.AbstractPasswordValidator validator) {
        this.validator = validator;
    }
    
    public void updatePassword(String newPassword) {
        validator.validate(newPassword); // if it's invalid the execution would terminate
        
        // proceed with business logic: 
        // generate the passwords hash, update the database entry
    }
}

The password is not coupled with the logic of validation. You can still have Password as a separate type if you wish, but it gives no benefit, in popular security frameworks, like Spring Security, hashed password is only an attribute of a type representing an authenticated User (or the user that is trying to authenticate).

Validators are stateless and can be instantiated only once, and then reused and safely shared between different threads.

And yet it's not the optimal approach. Yes we've implemented Decorator pattern, but in comparison with the annotation based validation, it is very rigid and requires a lot more code (see the caveat with the link to Jakarta Bean Validation API at very beginning of the answer).

It would be cleaner to sprinkle a couple of annotations in the DTO carrying the password.

\$\endgroup\$
0
4
\$\begingroup\$

same name, different concept

class BasePassword implements Password {
    private final String password;
    ...

abstract class PasswordDecorator implements Password {
    protected final Password password;

Consider calling that first one passwordStr. Then people are more likely to reason about this code correctly, and telephone conversations about it will go more smoothly.

storage

You don't actually have to allocate password storage at each layer. You could strip down this Public API so it follows a functional approach, applying a series of conjuncts to an input parameter.

magic number

class MinLengthValidator extends PasswordDecorator {
    private static final int MIN_LENGTH = 8;
            ...
            err = err + "\n" + "+ Need at least 8 or more characters";
                                                ↑

Please don't do that. You have a lovely manifest constant defined. Use it. When we bump up the minimum to 9, the generated text should automatically track that.

conjunct

    @Override
    public boolean isValid() {
        return password.getPassword().length() >= MIN_LENGTH;
    }

    @Override
    public String getValidationErrors() {
        String err = super.getValidationErrors();
        if (!isValid()) {
            err = err + "...

For stacked decorators, this feels wrong.

The string describing validation errors gets longer, great. But, likely because the test code at the end never inspects it, the validity bits don't stack, each corresponds to just a single test. We might prefer this, but then the string would be wrong:

    @Override
    public boolean isValid() {
        return super.isValid()
            && password.getPassword().length() >= MIN_LENGTH;
    }

I think the appropriate thing to do is remove .isValid() from the Public API you're designing, so the app can't call this internal implementation detail. And then it's up to you whether each decorator finds it useful to break it out as a helper. And the app can test whether the error string is zero length.

Alternatively define separate predicates, one for whether this layer's check passed, another for whether all checks have passed.

BTW, this composition certainly does look beautiful.

        Password decoratedPassword = new BasePassword(password);
        decoratedPassword = new MinLengthValidator(decoratedPassword);
        decoratedPassword = new UppercaseValidator(decoratedPassword);
        decoratedPassword = new NumberValidator(decoratedPassword);
        decoratedPassword = new LowercaseValidator(decoratedPassword);

stacked documentation

PasswordValidator definitely needs at least one sentence of /** javadoc */ describing the promise it makes to a caller.

security

It's worth noting that silly rules which encourage e.g. "123Password" are very different from examining "how much entropy does this have?" Low-entropy credentials are not suitable for securing a deployed system.

\$\endgroup\$
3
\$\begingroup\$

Constructing the decorators and using the last one in the same place defeats the purpose of a decorator.

You usualy want one part of your application to consume the interface. While some other part of your application decides which implementations to instatiate and how to compose them.

TBH password validation is not a great example for decorator use. But anyway...

interface PasswordFactory {
    Password createPassword(String plain);
}

class PasswordValidator {
...
    public String validate(String password) {
        Password decooratedPassword = this.factory.createPassword(password);

        return decoratedPassword.getValidationErrors();
    }
}

But you know, at this point you can just

interface PasswordValidator {
    String validate(String password);
}

and forget the factory and the value wrapper as those just add unnecesary intermediate step.

Well a single string might not be the best to represent multiple errors, but others addressed that already.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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