devise-pwned_password
devise-pwned_password copied to clipboard
Rename ambiguous min_password_matches/min_password_matches_warn option to threshold… to match pwned gem
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:
- Look at the standard
length
validator::minimum
- The attribute cannot have less than the specified length. - Or look at https://github.com/bitzesty/devise_zxcvbn, which has a
min_password_score
: it really is the minimum score required for the record to be allowed to validate as "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:
-
max_password_matches
(andmax_password_matches_on_sign_in
) -
max_allowed_password_matches
-
max_permitted_password_matches
- (Or, to use this opportunity to bring this option in line with the other config options which all start with
pwned_password_…
:) -
pwned_password_max_matches
- (To be consistent with
threshold
option ofpwned
gem:) -
pwned_password_threshold
(andpwned_password_theshold_on_sign_in
)
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).