michaelbanfield / devise-pwned_password

Devise extension that checks user passwords against the PwnedPasswords dataset
https://rubygems.org/gems/devise-pwned_password
MIT License
156 stars 29 forks source link

Rename ambiguous min_password_matches/min_password_matches_warn option to threshold… to match pwned gem #35

Open TylerRick opened 4 years ago

TylerRick commented 4 years ago

The min_password_matches option seems misnamed to me.

Model validations are usually worded in a way that describes what is required for the user to be valid:

Those examples show the usual usage of the word "minimum" in the context of validations.

So without knowing the context/background of this option, it would be easy to see this min_password_matches option in one's devise.rb config file, or in one's User model, and assume that it's saying the minimum number of matches that a password must have (is required to have) for a user to be considered valid, when in fact it is the opposite: it is the minimum number of matches beyond which the user will be considered invalid — or the maximum number of matches (exclusively) that are allowed/tolerated while still considering the user to be valid.

What would you think about renaming this option to be more in line with the wording used by https://github.com/bitzesty/devise_zxcvbn and most other validations?

While we're at it, I would like to rename min_password_matches_warn to something more in line with pwned_password_check_on_sign_in...

Ideas:

TylerRick commented 4 years ago

My preference would be to make this gem more like a wrapper for pwned's not_pwned validator (I may explore that idea in another PR), but where that's not possible, it would be nice to at least use the same names and options as that gem so that they're as similar as possible.

So I think pwned_password_threshold would be the name most in line with pwned.

If we renamed it to threshold, though, I would suggest we also change it to use a > check instead of a >= check. To make it work the same as pwned's threshold option, this number would be the maximum number of matches that is still considered valid (<=) — so to check if it is considered pwned/invalid, we would have to use the > operator to check if we've exceeded that threshold rather than >= like we're currently doing (to check if we've reached or exceeded the minimum).