rails/rails — pull request #13772 by chancancode (2014-01-20 13:41:35+00:00)
Title: [WIP] Refactored validations rules for has_secure_password

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](https://github.com/rails/rails/commit/bcf4e4f2b02157cecc1f1727a95cdf5bfa471771#diff-a6f9af64a6d420eb1103e16c0a0a8f04R38) 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