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

Use zxcvbn-js directly instead of devise_zxcvbn #597

Merged
merged 2 commits into from Oct 24, 2016

Conversation

monfresh
Copy link
Contributor

Why:

  • The implementation of server-side password strength validation based
    on zxcvbn should be compatible with any authentication system.
  • Using the devise_zxcvbn gem means the validation is tightly coupled
    to Devise and to the User model, which is not ideal.
  • The devise_zxcvbn gem's inefficient code added over 2.5 minutes to
    our test suite.

How:

  • Add the validation code directly to our FormPasswordValidator
  • Optimize the code for performance
  • Use Figaro to configure the minimum password score, and to allow the
    validation to be disabled by default in the test environment. It can
    be overriden in specs that test the strength validation.

**Why**:
- The implementation of server-side password strength validation based
on zxcvbn should be compatible with any authentication system.

- Using the devise_zxcvbn gem means the validation is tightly coupled
to Devise and to the User model, which is not ideal.

- The devise_zxcvbn gem's inefficient code added over 2.5 minutes to
our test suite.

**How**:
- Add the validation code directly to our FormPasswordValidator
- Optimize the code for performance
- Use Figaro to configure the minimum password score, and to allow the
validation to be disabled by default in the test environment. It can
be overriden in specs that test the strength validation.
Copy link
Contributor

@pkarman pkarman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will confess I resisted this idea at first but it is consistent with how we validate lots of data model attributes. Just some small nit comments.

# The scores 0, 1, 2, 3 or 4 are given when the estimated crack time (seconds)
# is less than 10**2, 10**4, 10**6, 10**8, Infinity.
# Default minimum is 4 (best).
# https://github.com/bitzesty/devise_zxcvbn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably not refer to code we are not using.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Copy and paste sloppiness on my part.


private

def not_weak_password
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about strong_password instead of not_weak_password ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although I see naming consistency with the word weak

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that. I went with what the gem was using, and I was trying to figure out why they made that choice. I'm fine with changing to strong_password.

@monfresh
Copy link
Contributor Author

monfresh commented Oct 23, 2016

Thanks for the feedback. I addressed your comments in my second commit.

The main reason we don't want to include the validation in the User model is because it involves expensive calculations that don't make sense to run when the password is not being validated. This is what was slowing down our test suite, and would have negatively affected the performance of our app.

Because of the way we are breaking up the default Devise sign up form into two parts: email first, then password, there is no way to make the password_required? method return true only when the password is being created or updated, without resorting to hacks like setting an attr_accessor in the User model and setting it to true in the controller(s) where we want the validation to take effect. This is what I undid in #105.

The one downside of not having validations in the model is that someone with access to the console could potentially create a User with a weak password. I don't foresee anyone of us ever doing that, and if an intruder had access to our console, then that would be the least of our worries. If we really wanted to prevent that, one way I can think of is to use a Data Type class and serialize the password column to use that class. I haven't done anything like that personally, but I saw an interesting talk about it. Here are the slides.

Copy link
Contributor

@pkarman pkarman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@pkarman pkarman merged commit 3a30782 into master Oct 24, 2016
@pkarman pkarman deleted the disable-zxcvbn-in-test-env branch October 24, 2016 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants