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