#39 ✓resolved

valid_password? checkes for match with the currently changed state of the password

Reported by LacKac | February 12th, 2009 @ 10:11 AM

If valid_password? is called on an unsaved object which has an updated password field, then it validates the attempted password with the new unsaved password which in my opinion is the wrong behaviour. It should still validate with the original password.

E.g. this could be used to check the old password when the user wants to change her password. Three fields would be submitted: old_password, password and password_confirmation, which would all be set on the user object by the update_attributes method. Then somewhere in the validation process a method would check if old_password is valid. By that time password and password_confirmation are set and crypted_password and password_salt are also set accordingly. This causes a problem when valid_password? is called since it will encrypt with the new password_salt and check against the new crypted_password value.

A suggested fix for this is commited to my fork at github: http://github.com/lackac/authlog... (a test is provided as well)

Comments and changes to this ticket

  • Ben Johnson

    Ben Johnson February 12th, 2009 @ 01:25 PM

    • State changed from “new” to “open”

    I agree with you, but the problem is that not everyone has dirty attributes. They might have an older version of AR that doesn't have it. I guess in that instance I could fall back to using the current password.

  • Ben Johnson

    Ben Johnson February 13th, 2009 @ 04:13 PM

    Alright, so I took a stab at implementing this and it causes problems with new records. That being said, what is wrong with checking the password before your change it? I would imagine that would make the most sense anyways, instead of the other way aropund. What do you think?

  • LacKac

    LacKac February 14th, 2009 @ 12:21 PM

    What problems are we talking about? I'd argue with checking first making the most sense. Changing the password is an update on the account object really, which needs an extra validation. I would put this into the model this way not needing anything extra in the controller.

    I guess the compatibility problem could be solved by saving the crypted_password and password_salt fields to an instance variable on first modification of these values and using these if they exist when validating the password. For new records none of these is necessary, since we surely don't have an old password to validate.

    I'll try to do this modifications later tonight and submit a patch if it works out.

  • Ben Johnson

    Ben Johnson February 17th, 2009 @ 03:18 AM

    Sounds good, it should be an easy change, but I was having issues with my tests. I'll see if I can work on it more as well, but help would be appreciated. Thanks!

  • LacKac

    LacKac March 16th, 2009 @ 06:08 AM

    I tried to follow up and make it compatible with Rails 2.0 and below (pre Dirty feature), when I noticed that you use the dirty featureset too in validations to check if the email of login field was changed.

    So I guess there's nothing wrong using it for old password validation as well.

  • Ben Johnson

    Ben Johnson March 23rd, 2009 @ 03:19 AM

    Sorry for the delay. I'm kind of on the fence with this, but I am leaning towards your side. How about this, if you can update your fork, make the changes, and have the tests pass then I'll include. I also agree that the "dirty" feature can be assumed installed, since I am using it throughout Authlogic.

  • Ben Johnson

    Ben Johnson April 3rd, 2009 @ 01:21 AM

    • State changed from “open” to “resolved”

    Please let me know if this is still an issue.

  • Aaron

    Aaron April 20th, 2009 @ 07:22 PM

    I'd like to see this change implemented as well. I just ran into the same error in my application:

    def validate_on_update unless self.old_password.nil?

    self.errors.add(:old_password, "Wrong password") unless valid_password?(old_password)

    end end

    This checks against the new password when user.password and user.password_confirmation are sent instead of checking against the old one

  • Ben Johnson

    Ben Johnson April 22nd, 2009 @ 06:18 PM

    I decided to add better support for this. See this commit: http://github.com/binarylogic/au...

  • wichit44214
  • yolo

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

Object based authentication solution that handles all of the non sense for you. It's as easy as ActiveRecord is with a database.

People watching this ticket