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!
--
Posted via http://www.ruby-forum.com/.
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. On 4/14/06, Rob <rallen@huntel.net> wrote:> > 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! > > -- > Posted via http://www.ruby-forum.com/. > _______________________________________________ > Rails mailing list > Rails@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails >-------------- next part -------------- An HTML attachment was scrubbed... URL: http://wrath.rubyonrails.org/pipermail/rails/attachments/20060414/eedf5205/attachment.html
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
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! -- Posted via http://www.ruby-forum.com/.