Forum: Ruby on Rails Am I doing this right?

Announcement (2017-05-07): is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see and for other Rails- und Ruby-related community platforms.
Rob (Guest)
on 2006-04-14 23:29
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 = User.find(session[:user_id])
    if @user.password == @params[:oldpw]
      if @params[:newpw] == ""
        flash[:notice] = "Please supply a new password."
        @user.update_attribute('password', @params[:newpw])
        flash[:notice] = "Password updated."
        redirect_to(:action => "index")
      flash[:notice] = "Please verify your old password."

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!
Shane S. (Guest)
on 2006-04-15 00:34
(Received via mailing list)
why are you creating a new user with the get request? I think it's safe
assume that if someone is changing their password, they are already in
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.
Patrick H. (Guest)
on 2006-04-15 00:50
(Received via mailing list)
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.

Rob (Guest)
on 2006-04-15 01:31
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!
This topic is locked and can not be replied to.