-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Support dynamic password lengths for Rails > 5.2 #5166
Conversation
Hello, @manojmj92! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information. |
032f163
to
ca66541
Compare
SourceLevel has finished reviewing this Pull Request and has found:
|
dcfc2c3
to
3fd43fc
Compare
Hello @tegon, could you kindly review? 🙂 |
@carlosantoniodasilva could you please review? 🙂 |
@manojmj92 sorry about the delay here, things have been a bit crazy but this is on my radar to get back to you. Thanks for the work. |
3fd43fc
to
188fe6c
Compare
Hello @carlosantoniodasilva, could you please review? 🙂 |
@manojmj92 yes, my apologies, this one slipped through the cracks, appreciate the ping. I've recently started to catch up on some of the issues/PRs here and this one is high on my radar, I'll get to it soon, just have a few other things to wrap up first. |
It would be helpful to see an example, maybe in the CHANGELOG. I found https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/models/user.rb#L488 which gave me a better idea of what's being proposed. |
@manojmj92 thanks for PR! I updated it (since we dropped Rails 5.2 support) here #5734 and merged. |
Hello,
Rails now supports length validator to have dynamic min and max values.
I feel this would be a nice addition to have Devise support this feature. I have made the change such that no current installations of Devise will have trouble with this change. Everything should work as perfectly as before.
This change is useful for instances where applications have the need to support dynamic password lengths, without a system restart. Such applications will just have to override the
password_length
method in their own classes to support dynamic password lengths.Real world use case: We have been trying to implement dynamic password length at GitLab. Implementation issue: https://gitlab.com/gitlab-org/gitlab/issues/36776
Right now, we have implemented the same using a monkey patch for the password length validator