Password reset functionality in Codeigniter 3

Question

I am working on an online newspaper/blogging application with CodeIgniter 3.1.8 and Twig. (I use the Twig template engine only for the front-end views).

The application, of course, has a registration and login system, which includes the password reset functionality.

The Passwordreset controller:

class Passwordreset extends CI_Controller
{
    public function __construct()
    {
        parent::__construct();
    }
    
    private $sender_email = "[email protected]";
    private $sender_name = "Razvan Zamfir";
    private $user_email = '';
    private $subject = 'Pasword reset link';
    private $token = '';
    private $reset_url = '';
    private $reset_link = '';
    private $body = '';
    
    public function index()
    {
        // Display form
        $data               = $this->Static_model->get_static_data();
        $data['pages']      = $this->Pages_model->get_pages();
        $data['tagline']    = 'Reset your password';
        $data['categories'] = $this->Categories_model->get_categories();
        
        // Form validation rules
        $this->form_validation->set_rules('email', 'Email', 'required|trim|valid_email');
        $this->form_validation->set_error_delimiters('<p class="error-message">', '</p>');
        
        if (!$this->form_validation->run()) {
            $this->load->view('dashboard/partials/header', $data);
            $this->load->view('auth/passwordreset');
            $this->load->view('dashboard/partials/footer');
        } else {
            if ($this->Usermodel->email_exists()) {
                
                //Get user email
                $this->user_email = $this->input->post('email');
                
                //create token
                $this->token = md5(str_shuffle($this->user_email));
                
                //create url
                $this->reset_url = base_url('newpassword/') . $this->token;
                
                //create reset link
                $this->reset_link = '<a href="' . $this->reset_url . '">password reset link</a>';
                
                $this->body = "Here is your <strong>" . $this->reset_link . "</strong>. After clicking it you will be redirected to a page on the website where you will be able to set a new pasword.";
                
                // Update paswword reset token
                $this->updateToken($this->user_email, $this->token);
                
                // Send mail and rediect
                $this->sendResetMail();
                
            } else {
                $this->session->set_flashdata('email_non_existent', "The email you provided does not exist in our database");
            }
            redirect('newpassword');
        }
    }
    
    public function updateToken($user_email, $token)
    {
        $user_email = $this->user_email;
        $token      = $this->token;
        $this->Usermodel->update_token($user_email, $token);
    }
    
    public function sendResetMail()
    {
        $config              = array();
        $config['protocol']  = 'smtp';
        $config['smtp_host'] = 'smtp.yourdomain.com';
        $config['smtp_user'] = '[email protected]';
        $config['smtp_pass'] = '******';
        $config['smtp_port'] = 25;
        $config['charset']   = 'utf-8';
        $config['mailtype']  = 'html';
        $config['newline']   = "\r\n";
        
        if (!$this->load->is_loaded('email')) {
            $this->load->library('email', $config);
        } else {
            $this->email->initialize($config);
        }
        
        // Build the body and meta data of the email message
        $this->email->from($this->sender_email, $this->sender_name);
        $this->email->to($this->user_email);
        $this->email->subject($this->subject);
        
        $this->email->message($this->body);
        
        if ($this->email->send()) {
            $this->session->set_flashdata('reset_mail_confirm', "A pasword reset link was send to the email address $this->user_email");
        } else {
            $this->session->set_flashdata('reset_mail_fail', "Our atempt to send a pasword reset link to $this->user_email has failed");
        }
    }
}

The passwordreset.php view:

<?php echo form_open(base_url(' ')); ?>
    <div class="form-group <?php if(form_error('email')) echo 'has-error';?>">
        <input type="text" name="email" id="email" class="form-control" placeholder="Email" value="<?php echo set_value('email')?>">
        <?php if(form_error('email')) echo form_error('email'); ?> 
    </div>

    <div class="form-group mb-2">
        <input type="submit" value="Reset password" class="btn btn-block btn-md btn-success">
    </div>            
<?php echo form_close(); ?>

The Newpassword controller:

class Newpassword extends CI_Controller
{
    public function index($token = NULL)
    {
        $data               = $this->Static_model->get_static_data();
        $data['pages']      = $this->Pages_model->get_pages();
        $data['tagline']    = 'New password';
        $data['categories'] = $this->Categories_model->get_categories();
        $data['token']      = $token;
        
        // Form validation rules
        $this->form_validation->set_rules('password', 'Password', 'required|min_length[6]');
        $this->form_validation->set_rules('cpassword', 'Confirm password', 'required|matches[password]');
        $this->form_validation->set_error_delimiters('<p class="error-message">', '</p>');
        
        if (!$this->form_validation->run()) {
            $this->load->view('dashboard/partials/header', $data);
            $this->load->view('auth/newpassword');
            $this->load->view('dashboard/partials/footer');
        } else {
            // Encrypt new password
            $enc_password = password_hash($this->input->post('password'), PASSWORD_DEFAULT);
            
            if ($this->Usermodel->set_new_password($token, $enc_password)) {
                $this->session->set_flashdata("new_password_success", "Your new password was set. You can login");
                redirect('login');
            } else {
                $this->session->set_flashdata("new_password_fail", "We have failed updating your password");
                redirect('/newpassword/' . $token);
            }
        }
    }
}

The newpassword.php view:

<?php echo form_open(base_url('newpassword/'. $token)); ?>
    <div class="form-group <?php if(form_error('password')) echo 'has-error';?>">
    <input type="password" name="password" id="password" class="form-control" placeholder="Password">
    <?php if(form_error('password')) echo form_error('password'); ?> 
    </div>
    <div class="form-group <?php if(form_error('cpassword')) echo 'has-error';?>">
    <input type="password" name="cpassword" id="cpassword" class="form-control" placeholder="Confirm password">
    <?php if(form_error('cpassword')) echo form_error('cpassword'); ?> 
    </div>
    <div class="form-group mb-2">
    <input type="submit" value="Set password" class="btn btn-block btn-md btn-success">
    </div>            
<?php echo form_close(); ?>

In the model I have:

public function email_exists() {    
    $query = $this->db->get_where('authors', ['email' => $this->input->post('email')]);
    return $query->num_rows() > 0;
}
//more code

public function update_token($user_email, $token) {
    return $this->db
        ->where('email', $user_email)
        // insert token (make it diffrent from NULL)
        ->update('authors', array('token' => $token));
}

public function set_new_password($token, $enc_password) {
    return $this->db
        ->where('token', $token)
        // set new password and reset token to NULL
        ->update('authors', array('password' => $enc_password, 'token' => NULL));
}

My main concern is security, but I will appreciate any constructive criticism. Thanks! :)

Answer 1

Some food for thought:

• I don't see any benefit to str_shuffle() why bother? Why bother referencing the email? If you need a random string, populate one in the shortest way with satisfactory randomness for the task.

• I generally advise against any html markup being generated anywhere other than the view. I'd rather see your controller's properties contain raw data only.

• updateToken() needs a rethink. Why would you bother passing any values into the method if you are going to immediately and unconditionally overwrite them? Do you mean the inverse of those declarations? You should also avoid declaring single-use variables unless there is an obvious benefit in doing so.

• You are making repeated calls of form_error() (containing the same string instead of calling it once and using its value multiple times). This isn't about performance, it is about not repeating yourself.

• Model method email_exists() should never receive any superglobals directly. You need to optimize your methods for utility. Imagine if you need to call this method elsewhere -- you may or may not have a $_POST element with that exact name. So, you absolutely need to collect all submitted values in the controller and the controller distributes the data to the models.

   public function email_exists(string $email): bool
   {
       return (bool)this->db->where(['email' => $email])->count_all_results('authors');
   }
  

  Apply this advice consistently throughout your project.

• CodeIgniter's update() method can receive the where data as its third parameter. Use the third parameter to afford the removal of the where() call.

• Use data type declarations for all inputs and return values for all methods -- this really improves the readability and maintainability of a project.

• I recommend always returning something when a method attempts to change the database. For INSERTs, I return the autogenerated id (primary key). When performing a UPDATE or DELETE, I return the affected rows.