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

Refactored validations rules for has_secure_password #13772

Merged

Conversation

chancancode
Copy link
Member

It primarily fixes #13753, but also addresses a few other issues I encountered along the way:

  1. When validation is enabled, and a model is saved without a password with user.save(validate: false), a runtime error is raised
  2. It is not possible to clear the password with user.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 with password= for that rather than encourage users to mess with with password_digest= directly)
  3. When validation is enabled, it is possible to save a record without a password(_digest) (bug number 2 makes this not very plausible now, but if that's fixe, you'll be able to do 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 a password_digest at the same time, so it passes) and updating an existing model (when you fetch an existing record from the DB, it'll have password_digest set, even though the virtual attribute password is nil).

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 if password= is working correctly password_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 empty password_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

@chancancode
Copy link
Member Author

I forgot to mention, authenticate will not work when password_digest is nil. But that's probably expected if you try to authenticate against a user that didn't have a password, so I think that's fine. In any case, that behaviour is not new in this PR.

@lukesarnacki
Copy link
Contributor

This looks awesome 👍 :)

@why-el
Copy link
Contributor

why-el commented Jan 20, 2014

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.
Copy link
Member

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! 👏 ❤️

Copy link
Member Author

Choose a reason for hiding this comment

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

You're correct sir!

@ghost ghost assigned chancancode Jan 20, 2014
@rafaelfranca
Copy link
Member

:shipit:


before_create { raise "Password digest missing on new record" if password_digest.blank? }
validates_confirmation_of :password, if: ->{ self.password.present? }
Copy link
Contributor

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

@chancancode
Copy link
Member Author

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

chancancode added a commit that referenced this pull request Jan 25, 2014
…r_good

Refactored validations rules for has_secure_password
@chancancode chancancode merged commit 94ce514 into rails:master Jan 25, 2014
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

Successfully merging this pull request may close these issues.

has_secure_password validations bypass Base.save(validate: false)
6 participants