Require new passwords to meet security requirements. #20
Conversation
|
Just realized that the db-level validation will probably cause issues for users with existing bad passwords - will add logic to prompt users to change their bad passwords on login to this PR. |
| @user.password = params[:password] | ||
| else | ||
| redirect_to({:action => "show", :id => @user.id}, {:notice => "Password does not meet our security requirements. Please use a password at least 8 characters long, including a number and a special character ($@%^!*). Help keep Trans Lifeline safe!"}) | ||
| save = false |
There was a problem hiding this comment.
A more readable way might be to remove the save variable, and then just have an early return here, which lets you remove the outer if statement below.
app/models/user.rb
Outdated
| @@ -16,6 +16,7 @@ class User < ActiveRecord::Base | |||
|
|
|||
| validates_confirmation_of :password, :unless => :no_password_required | |||
| validates_presence_of :password, :unless => :no_password_required | |||
| validates :password, :secure_password => true | |||
There was a problem hiding this comment.
I really like the use of ActiveModel::EachValidator here 😸
app/controllers/users_controller.rb
Outdated
| @user.reload | ||
| login(@user) | ||
| redirect_to dashboard_url | ||
| if not UsersHelper.secure_password?(params[:user].slice(:password)) |
There was a problem hiding this comment.
if not else is generally discouraged. You can flip the inner code blocks and remove the not.
There is an argument to be made for leaving it as is (it has our early exit at the start and then the meat of the method after). In that case, you can change if not to unless, which is more idiomatic.
…not meet security requirements
eldenseropian
force-pushed
the
elden-passwords
branch
from
af894bd to
8f6b8c3
Compare
|
Thank you for these changes! |
|
@jademcgough @sarahmaeve, can I merge this? |
New passwords must now be at least 8 characters long, and contain a number and a special character ($@%^!*).
This diff validates passwords both on the client side and when storing them to the DB. In addition to unit tests, I manually verified that changing a user's password in the "My Info" > "Administration" enforces the security requirements. I was not able to simulate setting the password on account creation locally, so the code to enforce it is there but has not been tested.