Reset a forgotten password

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
User avatar
Celauran
Moderator
Posts: 6427
Joined: Tue Nov 09, 2010 2:39 pm
Location: Montreal, Canada

Reset a forgotten password

Post by Celauran »

For reference, I'm using Kohana 3.2 Auth module with ORM driver. Yes, the controller is a little fatter than I'd like, but I don't much feel like mucking about in their user model. What I'd particularly like feedback on are action_forgotpassword() and reset_password(). They feel a little kludgy, but I'm not sure where to improve them. Thanks.

Code: Select all

<?php defined('SYSPATH') or die('No direct script access.');

class Controller_User extends Controller_Template_Basic
{
    public function action_index()
    {
        $this->template->title = "Account Overview";
        $this->template->content = View::factory('user/home')
             ->bind('user', $user);

        $user = Auth::instance()->get_user();

        // If user isn't logged in, redirect to login page
        if (!$user)
        {
            $this->request->redirect('user/login');
        }
    }

    public function action_login()
    {
        $this->template->title = "Login";
        $this->template->content = View::factory('user/login')
             ->bind('message', $message);

        if ($this->request->method() == HTTP_Request::POST)
        {
            $remember = array_key_exists('remember', $this->request->post());
            $user = Auth::instance()->login($this->request->post('username'), $this->request->post('password'), $remember);

            if ($user)
            {
                $this->request->redirect('user');
            }
            else
            {
                $message = "Login failed.";
            }
        }
    }

    public function action_logout()
    {
        Auth::instance()->logout();
        $this->request->redirect('user/login');
    }

    public function action_create()
    {
        $this->template->title = "Create an Account";
        $this->template->content = View::factory('user/create')
             ->bind('message', $message)
             ->bind('errors', $errors);

        if ($this->request->method() == HTTP_Request::POST)
        {
            try
            {
                $user = ORM::factory('user')->create_user($this->request->post(), array(
                    'username', 'password', 'email',
                ));

                $user->add('roles', ORM::factory('role', array('name' => 'login')));

                $message = "Account successfully created for user " . $this->request->post('username');
            }
            catch (ORM_Validation_Exception $e)
            {
                $errors = $e->errors('controllers');
            }
        }
    }

    public function action_update()
    {
        $this->template->title = "Update Account Information";
        $this->template->content = View::factory('user/update')
             ->bind('user', $user)
             ->bind('message', $message)
             ->bind('errors', $errors);

        $user = Auth::instance()->get_user();

        if ($this->request->method() == HTTP_Request::POST)
        {
            try
            {
                $user->update_user($this->request->post(), array(
                    'username', 'password', 'email',
                ));

                $message = "Account updated successfully.";
            }
            catch (ORM_Validation_Exception $e)
            {
                $errors = $e->errors('controllers');
            }
        }
    }

    public function action_forgotpassword()
    {
        $this->template->title = "Reset Forgotten Password";
        $this->template->content = View::factory('user/password')
             ->bind('message', $message);

        if ($this->request->method() == HTTP_Request::POST)
        {
            $user = ORM::factory('user')
                       ->where('username', '=', $this->request->post('username'))
                       ->where('email', '=', $this->request->post('email'))
                       ->find();

            if ($user->loaded())
            {
                $password_reset = $this->reset_password($user);

                if ($password_reset === TRUE)
                {
                    $message = "An email containing your new password has been sent.";
                }
                else
                {
                    $errors = $password_reset;
                }
            }
            else
            {
                $message = "User not found.";
            }
        }
    }

    public function reset_password(Model_User $user)
    {
        /**
         *  Model_User::update_user() requires an array as the first argument
         */
        $update['username'] = $user->username;
        $update['email']    = $user->email;
        $update['password'] = substr(uniqid(), 0, 7);

        try
        {
            $user->update_user($update, array('username', 'email', 'password'));
            
            $msg_body = file_get_contents('media/templates/forgot_password.txt');
            $msg_body = str_replace('{@PASSWORD}', $update['password'], $msg_body);

            if (!mail($user->email, "Password Reset", $msg_body))
            {
                $errors['email'] = "Could not send email.";
            }
        }
        catch (ORM_Validation_Exception $e)
        {
            $errors = $e->errors('controllers');
        }

        return (isset($errors)) ? $errors : TRUE;
    }
}

?>
User avatar
getmizanur
Forum Commoner
Posts: 71
Joined: Sun Sep 06, 2009 12:28 pm

Re: Reset a forgotten password

Post by getmizanur »

There is nothing to improve.
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Reset a forgotten password

Post by VladSun »

A temporal DoS attack can be established because you are resetting the old password and not creating a temporal (expiring) one.
This way the attacked user has to check his e-mail every time an attacker has reseted his password. Though, I don't see a message (and I think you don't record password resetting event) informing the user that password has been changed and sent, so it's very hard for the user to get access to the service after a single targeted attack. Not sure whether users' e-mails are shown in your site.

Better use a "temporal password" column instead of overwriting the "original" password. Then allow access with either of them, though "deleting" the other.
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
Celauran
Moderator
Posts: 6427
Joined: Tue Nov 09, 2010 2:39 pm
Location: Montreal, Canada

Re: Reset a forgotten password

Post by Celauran »

That's an awfully good idea. I hadn't really considered a password reset DoS attack. Thanks!
User avatar
getmizanur
Forum Commoner
Posts: 71
Joined: Sun Sep 06, 2009 12:28 pm

Re: Reset a forgotten password

Post by getmizanur »

how are you identifying if a request is a dos attack or a legitimate request? there is no way to be certain of this.

producing temporary expiry password is not the best solution. in scenarios where either the legitimate account user may suddenly remember his/her account particulars or the legitimate user is not aware of reset email being sent to them. if that is the case then you have locked out the legitimate user.

best way to defend against such multiple request from the same ip is to identify the browser identity and produce a captcha. this needs to be executed in your action_forgotpassword. as for reset_password(), you need to send uri to reset password screen via email.
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Reset a forgotten password

Post by VladSun »

getmizanur wrote:how are you identifying if a request is a dos attack or a legitimate request? there is no way to be certain of this.

producing temporary expiry password is not the best solution. in scenarios where either the legitimate account user may suddenly remember his/her account particulars or the legitimate user is not aware of reset email being sent to them. if that is the case then you have locked out the legitimate user.
Yes, that's the scenario I descried.
getmizanur wrote:best way to defend against such multiple request from the same ip is to identify the browser identity and produce a captcha. this needs to be executed in your action_forgotpassword. as for reset_password(), you need to send uri to reset password screen via email.
Browser identity can be faked. Common approach in bot attacks.
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
getmizanur
Forum Commoner
Posts: 71
Joined: Sun Sep 06, 2009 12:28 pm

Re: Reset a forgotten password

Post by getmizanur »

Yes, that's the scenario I descried.
oops! misunderstood.
Browser identity can be faked. Common approach in bot attacks.
yes, browser identity can be faked with bot attacks however it is a prevention which is better than nothing. alternative you can use this combined with apache dosevasive module. this last option is yet another prevention from dos attacks however there is no perfect solution to this sort of attack other than blocking the suspected ip address.
User avatar
Celauran
Moderator
Posts: 6427
Joined: Tue Nov 09, 2010 2:39 pm
Location: Montreal, Canada

Re: Reset a forgotten password

Post by Celauran »

getmizanur wrote:as for reset_password(), you need to send uri to reset password screen via email.
I like this idea, too. I could create a separate table to contain reset requests with expiring tokens. Requests to the reset URI without an argument (possibly also those without a valid argument) could then be redirected away or presented with a 403.
Post Reply