Am I doing this right?

Rails
1

Hi everyone,

I am following along with the book and branched out a bit to try some
stuff on my own. I’ve been working on is a basic user management system

  • create a user, log in, log out, update your account, that sort of
    thing. I created a password changer that compares the old password
    (oldpw) against what’s currently in the database, then updates that
    record with the new password (newpw). The code in the controller goes
    like this:

def change_password
flash[:notice] = “”
if request.get?
@user = User.new
else
@user = User.find(session[:user_id])
if @user.password == @params[:oldpw]
if @params[:newpw] == “”
flash[:notice] = “Please supply a new password.”
else
@user.update_attribute(‘password’, @params[:newpw])
flash[:notice] = “Password updated.”
redirect_to(:action => “index”)
end
else
flash[:notice] = “Please verify your old password.”
end
end
end

And there is a corresponding form with a couple of text fields named
oldpw and newpw that the user works in. The code itself works just
fine, but I have this gut feeling I could have accomplished this task in
no time at all with one or two simple steps. Is there a more elegant
way to solve this problem? Thanks!

2

why are you creating a new user with the get request? I think it’s safe
to
assume that if someone is changing their password, they are already in
the
system. Also it looks like you’re storing passwords in plain text which
is a
very big “no-no”. Passwords should always be 1-way encrypted.

3

I don’t know about two steps, but I would certainly consider some of
the following:

  1. Store a hashed password, not a plain text password in the database
  2. flash will clear itself out automatically don’t bother with
    flash[:notice] = ‘’
  3. Unless you are using @user in your view, don’t bother initializing on
    a get
  4. In your User model provide a method to “validate” a user that also
    looks up the user – or better yet, an update password method that
    validates, and updates and sets errors on failures

Then your controller will be just a couple lines, because you moved
the logic to other (better) places.

pth

4

Hashing the password was next on the list of things to do - I presumed
it’d be as easy as just throwing in a subroutine and updating one or two
things so I moved that a little bit further down the list of things to
put in.

Thanks for the insight!