-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Refactored validations rules for has_secure_password #13772
Refactored validations rules for has_secure_password #13772
Conversation
I forgot to mention, |
This looks awesome 👍 :) |
looks good to me. |
# This ensures the model has a password by checking whether the password_digest | ||
# is present, so that this works with both new and existing records. However, | ||
# when there is an error, the message is added to the password attribute instead | ||
# so that the error message will makes sense to the end-user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is "will make" here, no ?
Awesome work so far Godfrey! 👏 ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct sir!
|
||
before_create { raise "Password digest missing on new record" if password_digest.blank? } | ||
validates_confirmation_of :password, if: ->{ self.password.present? } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't need self
here I think
Still haven't had a chance to review the existing commits yet, but other than that and the few minor things noted above, I think this is good to go. So, I'll do a full review asap, and when I'm done I'll probably merge this. If you have any (potential) objections, let me know soon :) |
…r_good Refactored validations rules for has_secure_password
It primarily fixes #13753, but also addresses a few other issues I encountered along the way:
user.save(validate: false)
, a runtime error is raiseduser.password = nil
, even though the docs on that method seems to suggest otherwise. (When you don't have validations on, it makes sense that you should be allowed to clear the password, and I think it's better to stick withpassword=
for that rather than encourage users to mess with withpassword_digest=
directly)user.update!(password: nil)
)Reason for these bugs
I traced the git history, and I think in multiple attempts to fix the validation messages, the intention of the original validations were lost in translation.
If I am reading the history right, I think the original intention of the validation rules is to make sure your user model will always have a password.
In that regard, the original rules are actually spot on. It covers the case when you create a new model (when you set
password=
, it'll generate apassword_digest
at the same time, so it passes) and updating an existing model (when you fetch an existing record from the DB, it'll havepassword_digest
set, even though the virtual attributepassword
isnil
).The only problem with that rule is that the error message will becomes "password digest can't be blank" which is obviously not what you want to show the users. When trying to fix this problem (#6185, #6215; cc @erichmenge @josevalim) we misinterpreted the intention of the rule and ended up adding an unnecessary
before_create
callback and missed the update case (bug 3). (It's unnecessary because ifpassword=
is working correctlypassword_digest
will always be non-empty on create.)So I rewrote the tests to spell out all the possible cases and refactored the rules. The end-result also passes all but two (invalid) test cases in the old test suite.
The only thing that changed (besides the bugs were fixed) is that having a non-empty
password
but an emptypassword_confirmation
now gives "Password confirmation does not match Password" instead of "Password confirmation can't be blank". This seems fine to me. That rule was added in #10694 (cc @steveklabnik), and it seems like the primary driver of that PR is just to cover the "create a new user with validation and a blank password confirmation" scenario which is covered here. If the original message is desirable, I can easily add it back.I'm not too sure about the extra-verbose tests, but that's what I used to give me safety during my refactor, so I left them there.
Marked as [WIP] because I need to go through all the commits again to make sure I didn't unfix any old bugs by accident.cc @carlosantoniodasilva