Syntax Problem

I admit I am stuck. I am converting a legacy php site to ROR. The old
site uses MD5, a security flaw waiting to happen. I upgraded to AAA
and am adding code in the session_controller.rb file to see if
crypted_password is blank. If it is, crypt and check the password
against the old md5 version and write it in the new crypted_password
field.

Here is my code add on before the logged_in?:
def create
    # if no password use old md5 and fill in with sha2
    user = User.find_by_login(params[:login])
    if #{user.crypted_password} == nil
        #check_md5?
(params[:password],#{user.old_salt},#{user.hashed_password})
        @p = Digest::MD5::hexdigest("#{params[:password]}")
        @s = Digest::MD5::hexdigest(user.old_salt)
        @pw = Digest::MD5.hexdigest(@s+@p)
      if @pw == #{user.hashed_password}
        flash.now[:notice] = "Passed"
        # need to write pw
        #user.crypted_password = self.encrypt("#{@params[:password]}")
        user.salt = user.old_salt
        user.save
        redirect_back_or_default('/')
      end
    end
  end

This doesn't seem to work (I know, it is not DRY), but the 2 passwords
are equal. I admit, I am new to ruby so I am sure it is my TCL'sm
working against me.

Thanks

Do you see any errors in the logs? When you say "it doesn't work", what
doesn't work about it? What happens if you do this

def create
    # if no password use old md5 and fill in with sha2
    user = User.find_by_login(params[:login])
    if user.crypted_password.blank?
        p Digest::MD5::hexdigest(params[:password])
        s = Digest::MD5::hexdigest(user.old_salt)
        pw = Digest::MD5.hexdigest(s+p)
      if pw == user.hashed_password
        flash.now[:notice] = "Passed"
        user.update_attributes!(:crypted_password => self.encrypt(params[:password]), :salt => user.old_salt)
        redirect_back_or_default('/')
      end
    end
  end

I didn't check this code for syntax errors, but it should point you in
the right direction. One thing to note, #{} only needs to surround
variables when you are interpolating them in a string and you only need
to preface variables with @ if you want them to be instance variables
and not local variables.

-Bill

tresero wrote:

`if user.crypted_password.blank?

should be

``if user && user.crypted_password.blank?`

-Bill

William Pratt wrote:

Just a suggestion, I would think it would be better to write a
migration to convert the users table to use the new password than to
convert on the fly like this.

try:

         if @pw == user.hashed_password
...

The # starts a comment, and since you are left with a line ending with
== what you have is effectively:

         if @pw == flash.now[:notice] = "Passed"

Nicolas,
I can't migrate the passwords, they are md5, a very insecure hash. I
am converting to SHA2, both hashes are one-way, they can't be
unencrypted.

Did the version I posted work at all? Do you get any errors in the logs?

-Bill

I can get it to partially work. I am just starting work again for the
day. I will post when it is done.
It does not error, but the db update doesn't happen. I am debugging in
script/console.

Thanks for pointing me in the right direction. I come from many years
of OpenACS/AOLServer so ROR is new to me.

Jon

Ah, yes, I see the issue :slight_smile:

I think I need a new approach.
What I envision is if a user has a login and the crypted_password
field is blank, compare the entered password to the old md5'd
password. If that is correct, automatically generate a new hash and
write the crypted password and salt fields.
The first part I have done, since I am new to rails, the second is the
hold up. I can't figure out how to make AAA hash and write the fields.
I am 90% there but still not working.
No errors are thrown, just no data is written.

I think I need a new approach.
What I envision is if a user has a login and the crypted_password
field is blank, compare the entered password to the old md5'd
password. If that is correct, automatically generate a new hash and
write the crypted password and salt fields.
The first part I have done, since I am new to rails, the second is the
hold up. I can't figure out how to make AAA hash and write the fields.
I am 90% there but still not working.
No errors are thrown, just no data is written.

Have you still got #{ ... } in your code, as Rick said ? That's just a comment so does nothing (it's only in a string that #{} indicates ruby code to be executed).

Fred

I would take a different approach and do this all in the model rather
than the controller - this would make it far easier to unit test as
well. I'm assuming you are using something like acts_as_authenticated
or restful_authentication. Return the session controller to it's
original state and do something like this in your model. Of course
this is not tested, but I feel the approach is valid. The nice thing
is, you can play with this at the console too if you wish.

# user model

def self.authenticate(login, password, activated=true)
  user = find_by_login(login)
  if user.crypted_password.blank?
    user.deprecated_authenticate(login, password)
  else
    find_by_login_and_password_hash_and_activated(login,
Digest::SHA1.hexdigest(password + PASSWORD_SALT), activated)
  end
end

def deprecated_authenticate(login, password)
  md5_password = Digest::MD5::hexdigest(password)
  md5_salt = Digest::MD5::hexdigest(user.old_salt)
  password = Digest::MD5.hexdigest(md5_salt + md5_password)
  if hashed_password == password
    convert_password
    user
  else
    false
  end
end

def convert(password)
  self.password = password
  self.password_confirmation = password
  self.save!
end

Thank you Nicolas,
That is exactly what I was beginning to think. I am not used to the
MVC concepts yet, but I will gladly share this when it is done, I am
sure this isn't the first time
it has been needed.

Jon

Are you using acts_as_authenticated? I ask because your column and
method names looked similar. If you are, this can be done by calling
user.update_attributes!(:password => params[:password],
:password_confirmation => params[:password]). Acts_as_authenticated
will create and store the hash for you.

-Bill

tresero wrote:

Yes I am using restful_authentication.
Here is the code I have in controller/user.rb, changed code only.

def self.authenticate(login, password)
    u = find_by_login(login) # need to get the salt
    if u.crypted_password.blank?
      u.deprecated_authenticate?(login, password,u.old_salt)
    else
      u && u.authenticated?(password) ? u : nil
    end
  end

def deprecated_authenticate?(login, password, old_salt)
      md5_password = Digest::MD5::hexdigest(password)
      md5_salt = Digest::MD5::hexdigest(old_salt)
      password = Digest::MD5.hexdigest(md5_salt + md5_password)
      if hashed_password == password
        true
      else
        false
      end
    end

This doesn't write the user password or salt though.

OK, I got this working with one glitch. I can't get the salt to be
generated/written to the db.
Everything works, but there is no salt.

If you would like to post the code for your model where it converts
the password and save it I might be able to work out why it's not
saving the salt. Also you could simplfy this line:

      if hashed_password == password
        true
      else
        false
      end

to simply this:

hashed_password == password

it will return true or false.

Cheers,
Nicholas

Nicolas,
Here is the code in user.rb, as I said, it works as is, but isn't
saving the hash. I assume it is just not using it.

def self.authenticate(login, password)
    u = find_by_login(login) # need to get the salt
    if u.crypted_password.blank?
      if u.deprecated_authenticate?(login, password,u.old_salt)
        u.update_attributes!(:password =>
password,:password_confirmation => password)
        logger.info 'deprecated true'
      else
        logger.info 'deprecated false'
      end
      u && u.authenticated?(password) ? u : nil
    else
      u && u.authenticated?(password) ? u : nil
    end
  end

I think I have found your problem :slight_smile: If you check the protected method
encrypt_password it only creates the salt if the record is new:

self.salt = Digest::SHA1.hexdigest("--#{Time.now.to_s}--#{login}--")
if new_record?

so if you create a salt first before updating the attributes you
should be good to go:

for example:

u.convert_password(password) # rather than update_attributes!
(:password => password,:password_confirmation => password)

def convert_password(password)
self.salt = Digest::SHA1.hexdigest("--#{Time.now.to_s}--#{login}--")
update_attributes(:password => password,:password_confirmation =>
password)
save!
end

Creating is method that describes what you are doing is always a good
way to go - makes the code more self-documenting.