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

Fix regression in has_secure_password. #10694

Merged
merged 1 commit into from May 30, 2013

Conversation

Projects
None yet
6 participants
@steveklabnik
Member

steveklabnik commented May 20, 2013

If the confirmation was blank, but the password wasn't, it would still save.

Sent via a PR for feedback. Password stuff is tricky.

/cc @josevalim, @senny

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny May 20, 2013

Member

thanks Steve!

Member

senny commented May 20, 2013

thanks Steve!

@senny

View changes

Show outdated Hide outdated activemodel/test/cases/secure_password_test.rb
@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 20, 2013

Member

@senny you can thank the readers of "Rails 4 in Action" :)

Member

steveklabnik commented May 20, 2013

@senny you can thank the readers of "Rails 4 in Action" :)

@josevalim

View changes

Show outdated Hide outdated activemodel/lib/active_model/secure_password.rb
@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim May 20, 2013

Contributor

❤️ 💚 💙 💛 💜

Contributor

josevalim commented May 20, 2013

❤️ 💚 💙 💛 💜

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 20, 2013

Member

Uhhhh, so this isn't actually working right now. Unsure what's up.

Member

steveklabnik commented May 20, 2013

Uhhhh, so this isn't actually working right now. Unsure what's up.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 20, 2013

Member

Please review again, @josevalim @senny @rafaelfranca <3

Member

steveklabnik commented May 20, 2013

Please review again, @josevalim @senny @rafaelfranca <3

@raykrueger

This comment has been minimized.

Show comment
Hide comment
@raykrueger

raykrueger May 20, 2013

I was just going to start working on a fix for that, glad I searched issues first! I'd been banging my head on this for a bit thinking I did something wrong heh. Thanks for all your work Steve.

raykrueger commented May 20, 2013

I was just going to start working on a fix for that, glad I searched issues first! I'd been banging my head on this for a bit thinking I did something wrong heh. Thanks for all your work Steve.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca
Member

rafaelfranca commented May 21, 2013

:shipit:

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 21, 2013

Member

CHANGELOG added.

Ugh, so last minute checks, there is one other test failing:

ConfirmationValidationTest#test_no_title_confirmation [/home/pairing/rails/activemodel/test/cases/validations/confirmation_validation_test.rb:24]:
Failed assertion, no message given.

Unsure what that is right this second.

Member

steveklabnik commented May 21, 2013

CHANGELOG added.

Ugh, so last minute checks, there is one other test failing:

ConfirmationValidationTest#test_no_title_confirmation [/home/pairing/rails/activemodel/test/cases/validations/confirmation_validation_test.rb:24]:
Failed assertion, no message given.

Unsure what that is right this second.

@josevalim

View changes

Show outdated Hide outdated activemodel/lib/active_model/validations/confirmation.rb
@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim May 21, 2013

Contributor

@steveklabnik have you tried fixing this issue by just running the confirmation validation if the password is present? Basically, replacing this line:

https://github.com/rails/rails/blob/master/activemodel/lib/active_model/secure_password.rb#L59

By something like:

validates_confirmation_of :password, if: lambda { |m| m.password.present? }
Contributor

josevalim commented May 21, 2013

@steveklabnik have you tried fixing this issue by just running the confirmation validation if the password is present? Basically, replacing this line:

https://github.com/rails/rails/blob/master/activemodel/lib/active_model/secure_password.rb#L59

By something like:

validates_confirmation_of :password, if: lambda { |m| m.password.present? }
@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 21, 2013

Member

I have not. Let me try that now, that seems simpler, thanks.

Member

steveklabnik commented May 21, 2013

I have not. Let me try that now, that seems simpler, thanks.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 21, 2013

Member

Updated trying that method (but a different conditional, since that one isn't right), but it still fails:

SecurePasswordTest#test_will_not_save_if_confirmation_is_blank_but_password_is_not [/Users/steve/src/rails/activemodel/test/cases/secure_password_test.rb:101]:
Expected true to be nil or false

Still not sure. This bug makes me feel stupid, it should be so easy.

Member

steveklabnik commented May 21, 2013

Updated trying that method (but a different conditional, since that one isn't right), but it still fails:

SecurePasswordTest#test_will_not_save_if_confirmation_is_blank_but_password_is_not [/Users/steve/src/rails/activemodel/test/cases/secure_password_test.rb:101]:
Expected true to be nil or false

Still not sure. This bug makes me feel stupid, it should be so easy.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 21, 2013

Member

Thanks to help from @pnc, this now works.

:shipit: ?

Also, this should get backported to 4-0-stable and 4-0-0, I think?

Member

steveklabnik commented May 21, 2013

Thanks to help from @pnc, this now works.

:shipit: ?

Also, this should get backported to 4-0-stable and 4-0-0, I think?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 22, 2013

Member

Yes. It should.

Seems good to me. @josevalim?

Member

rafaelfranca commented May 22, 2013

Yes. It should.

Seems good to me. @josevalim?

Fix regression in has_secure_password.
If the confirmation was blank, but the password wasn't, it would still save.

steveklabnik added a commit that referenced this pull request May 30, 2013

Merge pull request #10694 from steveklabnik/hsp_regression
Fix regression in has_secure_password.

@steveklabnik steveklabnik merged commit 87f3eb6 into rails:master May 30, 2013

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik
Member

steveklabnik commented May 30, 2013

:shipit:

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 30, 2013

Member

In 4-0-0 as b965ce3 and in 4-0-stable as 4e76051.

Member

steveklabnik commented May 30, 2013

In 4-0-0 as b965ce3 and in 4-0-stable as 4e76051.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment