validation should be checked anytime password is changed, even if password_required? returns false

password_required? should only be used as the condition for validates that enforce requiredness — that is, presence validations.

Take a look at lib/devise/models/validatable.rb. That is the way it is done there, which we should generally try be consistent with:

The only places validations are conditional on email_required?/password_required?:

validates_presence_of   :email, if: :email_required?
validates_presence_of     :password, if: :password_required?
validates_confirmation_of :password, if: :password_required?

all other validations are not conditional on requireness, but some of them are only checked if changing the field (email/password):

validates_uniqueness_of :email, allow_blank: true, case_sensitive: true, if: :will_save_change_to_email?
validates_format_of     :email, with: email_regexp, allow_blank: true, if: :will_save_change_to_email?
validates_length_of       :password, within: password_length, allow_blank: true

The length validation isn't conditional at all — I'm not sure why the email format validation is only checked if email changed, but password length is always checked. But it doesn't matter since it's cheap. Checking for pwned password, on the other hand, is a more expensive check to make, so it should only happen if email has actually changed.

P.S. We should also add tests that it only calls the pwned API when there is a change, but I didn't have time to figure that test out.

Context: I have a custom password_required? method defined, that returns false after a user is confirmed. I was surprised that this gem wasn't checking my password and wasn't adding errors even when changing to obviously-compromised passwords like 'password'.

michaelbanfield / devise-pwned_password

validation should be checked anytime password is changed, even if password_required? returns false #27