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

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

Open
TylerRick opened this issue Apr 28, 2020 · 1 comment

Comments

@TylerRick
Copy link
Contributor

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 (and max_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 of pwned gem:)
  • pwned_password_threshold (and pwned_password_theshold_on_sign_in)
@TylerRick
Copy link
Contributor Author

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

@TylerRick TylerRick changed the title Rename ambiguous min_password_matches/min_password_matches_warn option to threshold…/max…/something Rename ambiguous min_password_matches/min_password_matches_warn option to threshold… to match pwned gem Apr 28, 2020
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

No branches or pull requests

1 participant