Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

TylerRick
Copy link
Contributor

@TylerRick TylerRick commented Apr 22, 2020

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'.

@TylerRick TylerRick force-pushed the validate_password_if_changed_not_if_required branch from af827ad to e0b442d Compare April 22, 2020 01:25
(even if password_required? returns false)
@TylerRick TylerRick force-pushed the validate_password_if_changed_not_if_required branch from e0b442d to c6623e6 Compare April 22, 2020 20:57
@TylerRick
Copy link
Contributor Author

@michaelbanfield Can we merge this? Or will I have to maintain my own fork? Did you have any things you would like changed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants