Codeigniter 3 Registration and Login System

Question

I am working on a basic blog application in Codeigniter 3.1.8 and Bootstrap 4.

The application allows Registration and Login. I have concerns about the security level of the Registration system I have put together.

The Register controller:

class Register extends CI_Controller {
    public function __construct()
    {
        parent::__construct();
    }

    public function index() {
        $data = $this->Static_model->get_static_data();
        $data['pages'] = $this->Pages_model->get_pages();
        $data['tagline'] = 'Want to write for ' . $data['site_title'] . '? Create an account.';
        $data['categories'] = $this->Categories_model->get_categories();

        $this->form_validation->set_rules('first_name', 'First name', 'required');
        $this->form_validation->set_rules('last_name', 'Last name', 'required');
        $this->form_validation->set_rules('email', 'Email', 'required|trim|valid_email');
        $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_rules('terms', 'Terms and Conditions', 'required', array('required' => 'You have to accept the Terms and Conditions'));
        $this->form_validation->set_error_delimiters('<p class="error-message">', '</p>');

        // If validation fails
        if ($this->form_validation->run() === FALSE) {
            $this->load->view('partials/header', $data);
            $this->load->view('auth/register');
            $this->load->view('partials/footer');
        } else {
            // If the provided email does not already
            // exist in the authors table, register user
            if (!$this->Usermodel->email_exists()) {
                // Encrypt the password
                $enc_password = md5($this->input->post('password'));

                // Give the first author admin privileges
                if ($this->Usermodel->get_num_rows() < 1) {
                    $active = 1;
                    $is_admin = 1;
                } else {
                    $active = 0;
                    $is_admin = 0;
                }

                // Register user
                $this->Usermodel->register_user($enc_password, $active, $is_admin);

                if ($this->Usermodel->get_num_rows() == 1) {
                    $this->session->set_flashdata('user_registered', "You are now registered as an admin. You can sign in");
                } else {
                    $this->session->set_flashdata('user_registered', "You are now registered. Your account needs the admin's aproval before you can sign in.");
                }
                redirect('login');
            } else {
                // The user is already registered
                $this->session->set_flashdata('already_registered', "The email you provided already exists in our database. Please login.");
                redirect('login');
            }
        }
    }
}

The Usermodel model:

class Usermodel extends CI_Model {

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

    public function get_num_rows() {
        $query = $this->db->get('authors');
        return $query->num_rows(); 
    }

    public function getAuthors(){
        $query = $this->db->get('authors');
        return $query->result();
    }

    public function deleteAuthor($id) {
        return $this->db->delete('authors', array('id' => $id));
    }

    public function activateAuthor($id) {
        $author = null;
        $updateQuery = $this->db->where(['id' => $id, 'is_admin' => 0])->update('authors', array('active' => 1));
        if ($updateQuery !== false) {
        $authorQuery = $this->db->get_where('authors', array('id' => $id));
        $author = $authorQuery->row();
        }
        return $author;
    }

    public function deactivateAuthor($id) {
        $author = null;
        $updateQuery = $this->db->where(['id' => $id, 'is_admin' => 0])->update('authors', array('active' => 0));
        if ($updateQuery !== false) {
            $authorQuery = $this->db->get_where('authors', array('id' => $id));
            $author = $authorQuery->row();
        }
        return $author;
    }

    public function register_user($enc_password, $active, $is_admin) {
        // User data
        $data = [
            'first_name' => $this->input->post('first_name'),
            'last_name' => $this->input->post('last_name'),
            'email' => $this->input->post('email'),
            'password' => $enc_password,
            'register_date' => date('Y-m-d H:i:s'),
            'active' => $active,
            'is_admin' => $is_admin
        ];
        return $this->db->insert('authors', $data);
    }

    public function user_login($email, $password)
    {
        $query = $this->db->get_where('authors', ['email' => $email, 'password' => md5($password)]);
        return $query->row();
    }
}

UPDATE:

I have decided to post the login() method, from the Login controller, as changing the Register class would require changing the login accordingly:

public function login() {  
    $this->form_validation->set_rules('email', 'Email', 'required|trim|valid_email');
    $this->form_validation->set_rules('password', 'Password', 'required|trim');
    $this->form_validation->set_error_delimiters('<p class="error-message">', '</p>');
    if ($this->form_validation->run()) {
      $email = $this->input->post('email');
      $password = $this->input->post('password');
      $this->load->model('Usermodel');
      $current_user = $this->Usermodel->user_login($email, $password);
        // If we find a user
      if ($current_user) {
        // If the user found is active
        if ($current_user->active == 1) {
          $this->session->set_userdata(
           array(
            'user_id' => $current_user->id,
            'user_email' => $current_user->email,
            'user_first_name' => $current_user->first_name,
            'user_is_admin' => $current_user->is_admin,
            'user_active' => $current_user->active,
            'is_logged_in' => TRUE
            )
           );
          // After login, display flash message
          $this->session->set_flashdata('user_signin', 'You have signed in');
          //and redirect to the posts page
          redirect('/dashboard');  
        } else {
          // If the user found is NOT active
          $this->session->set_flashdata("login_failure_activation", "Your account has not been activated yet.");
          redirect('login'); 
        }
      } else {
        // If we do NOT find a user
        $this->session->set_flashdata("login_failure_incorrect", "Incorrect email or password.");
        redirect('login'); 
      }
    }
    else {
      $this->index();
    }
  }

Looking for feedback and improvement ideas.

Answer 1

There are a number of things I'd do differently and the first is a must.

First, using md5() to encode a password is universally considered a bad practice. It's been considered too weak for that purpose since at least 2004 if not earlier. Instead, use the PHP function password_hash.

I would encrypt the password in the model. In fact, I'd move almost all the registration logic to the model. Here's my version of Usermodel::register_user()

public function register_user()
{
    // Get the entire $_POST array with one call then extract what we need
    $posted = $this->input->post();

    $field_list = ['first_name', 'last_name', 'email', 'password'];
    foreach($field_list as $field){
        $data[$field] = $posted[$field];
    }

    $enc_password = password_hash($data['password']);

    // if password_hash fails it's time to bail
    if( ! $enc_password)
    {
        return false;
    }
    // update $data with encrypted password
    $data['password'] = $enc_password;

    // Put additional items in the $data array   
    $data['register_date'] = date('Y-m-d H:i:s');
    // CodeIgniter has a method to count records - use it. Remove get_num_rows() from model
    $data['is_admin'] = $this->db->count_all('authors') === 0 ? 1 : 0;
    $data['active'] = $data['is_admin'];

    if($inserted = $this->db->insert('authors', $data) === TRUE)
    {
        if($data['is_admin'] === 1)
        {
            $msg = "You are now registered as an admin. You can sign in";
        }
        else
        {
            $msg = "You are now registered. Your account needs the admin's approval before you can sign in.";
        }
        $this->session->set_flashdata('user_registered', $msg);
    }
    return $inserted;
}

With that new method and a little rearranging in the controller, we can make the code a bit more concise

public function index()
{
    $this->form_validation
        ->set_rules('first_name', 'First name', 'required')
        ->set_rules('last_name', 'Last name', 'required')
        ->set_rules('email', 'Email', 'required|trim|valid_email')
        ->set_rules('password', 'Password', 'required|min_length[6]')
        ->set_rules('cpassword', 'Confirm password', 'required|matches[password]')
        ->set_rules('terms', 'Terms and Conditions', 'required',
            array('required' => 'You have to accept the Terms and Conditions'))
        ->set_error_delimiters('<p class="error-message">', '</p>');

    if($this->form_validation->run())
    {
        // If the provided email isn't on record then register user
        if( ! $this->Usermodel->email_exists())
        {
            // Register user
            if($this->Usermodel->register_user())
            {
                // worked - go to login
                redirect('login');
            }
            // register_user() returned false - set an error message for use in 'auth/register' view
            $data['registration_error_msg'] = "Registration Failed! Please contact Administrator.";
        }
        else
        {
            // The user is already registered
            $this->session->set_flashdata('user_registered',
                "The email you provided already exists in our database. Please login.");
            redirect('login');
        }
    }

    // Validation or registration failed or it's the first load of this page
    $data = $this->Static_model->get_static_data();
    $data['pages'] = $this->Pages_model->get_pages();
    $data['tagline'] = 'Want to write for '.$data['site_title'].'? Create an account.';
    $data['categories'] = $this->Categories_model->get_categories();
    $this->load->view('partials/header', $data);
    $this->load->view('auth/register');
    $this->load->view('partials/footer');
}

Using password_hash() means the login logic needs to be refactored also and the PHP function password_verify() must be used. password_verify — confirms that a password matches a hash of that password. With that in mind, `Usermodel::user_login might look like this.

public function user_login()
{
    $email = filter_var($this->input->post('email'), FILTER_SANITIZE_EMAIL);

    if(filter_var($email, FILTER_VALIDATE_EMAIL))
    {
        $user = $this->db
            ->select('password, user_id, active')
            ->get_where('authors', ['email' => $email])
            ->row();

        if(password_verify($this->input->post('password'), $user->password))
        {
            if($user->active)
            {
                return $user->user_id;
            }
            $this->session->set_flashdata("login_failure_activation", "Your account has not been activated yet.");
        }
        else
        {
            $this->session->set_flashdata("login_failure_incorrect", "Incorrect email or password.");
        }
    }
    return false;
}

The above is going to mean that the controller's login() method will need to be reworked too.

An unbreakable security rule is "Never Trust User Input!"

So, any data sent to the server must be sanitized and validated. This is true even for some identifier (like a user_id) that is part of a URL that you put on the screen. With that in mind, you should always (at the very minimum) make sure that any model function that has a $id argument is getting the right datatype.

Let's assume your user_id type is an integer. For instance, the controller method that activates an author might make this kind of check before sending it to the model.

public function activate_author($id)
{
    if(filter_var($id, FILTER_VALIDATE_INT)) 
    {
        $this->Usermodel->activateAuthor($id)
        // other code 
    } 
    else 
    {
       //respond in a way that doesn't give a bad-actor too much info
    }

}

While CodeIgniter's validation library is quite good it doesn't go far enough. Particularly when it comes to sanitizing. A serious study of the PHP docs on Filters is recommended. There are lots of online tutorials on using filter_var too.

Further Reading on PHP Security:

A Gentle Introduction to Application Security

The 2018 Guide to Building Secure PHP Software

Checklist-Driven Security Considered Harmful

Implementing Secure User Authentication in PHP Applications with Long-Term Persistence

Answer 2

DFriend's answer is good, but it sounds like some more clarity is needed about how to use the built-in password_hash function, and there are some architectural things you could improve.

Is the email address the primary key for the authors table? That's not an excellent choice, but we'll roll with it.

To properly use password_hash, you must also use password_verify and password_needs_rehash. This means that you can't just re-hash the password and look for the hash in the database. You have to identify the user-row in the database, load the stored hash into the PHP layer, and check that against the provided password in question. I haven't tested the below, but it's more-or-less how I'd write it.

class Password_helper {
  const ALGO =  PASSWORD_DEFAULT;

  public static set_password(CI_Controller $CI, string $email, string $p):bool{
    // We could do any password-strength rules or setting of optional arguments here,
    // but I'll assume you're not doing that.
    return $CI->db->update(
      'authors',
      ['password' => password_hash($p, self::ALGO)],
      ['email' => $email]);
  }

  public static check_and_upkeep(CI_Controller $CI, string $email, string $p):bool{
    $_user = $CI->db->get_where(
      'authors',
      ['email' => $email], 
      1)
      ->result();
    if($_user){
      $user = $_user[0];
      if(password_verify($p, $user->password)){
        if(password_needs_rehash($user->password, self::ALGO)){
          self::set_password($CI, $email, $p);
        }
        return true;
      }
      else{
        return false;
      }
    }
    else{
      return false;
    }
  }
}

The above might not jive perfectly with the concept of a "model" you're using; CodeIgniter's "model" concept is frankly problematic. Also, to use the above, you'll need to insert the user to the database first, then set their password. That's fine; you can just temporarily set their password to "".

Another security detail:

The business of making the first author the (an?) administrator is not good. There's hypothetically a chance someone might snipe you, and anyway it'll be cluttering up the codebase forever.
Depending what you mean by "admin", you maybe shouldn't even have "admins" in the same table as "users". In any case, remove that whole business from the registration stack; you can set your first admin manually in the database using some other tool.

Other stuff:

• A key and specific role of the Controller "endpoint" function (index() for example) is to parse, sanitize, and validate input, all of it. Both you and DFriend are reading POST values down in the model layer; those values should be passed down as arguments. If the number of arguments starts to feel unworldly, you can make classes ("models") for different data structures like Authors.
• I imagine you've repeated that trio of $this->load->view() calls, and the associated construction of $data, several times throughout your site. Wrap all of that up in a single place, either as a method of a Controller base class, or as a method of a Loader extension class. Also note that you can load views from inside of views, so a single master-template may be better than having separate header and footer "partial" views.
• Similar to what I mentioned above, don't use $this->Usermodel->get_num_rows() == 1 to check if the user is an admin or is active; at that point you should have the user's data on hand and be able to check literally.
• I don't like flashdata, I'd rather use the url hash to pass a narrow range of messages to javascript running on the target page. But that's just personal preference.
• You could consolidate your (de)activateAuthor functions if you want. When they're called, you should have already validated that the target exists, so getting a falsy value from the DB would be grounds to throw an error.
• As much as I hate CI's "models", they'll be slightly less painful if you throw them all in your auto-load config file; then you don't need to load them explicitly.
• Your login() endpoint method calls the index() endpoint method if validation fails. That's an odd choice.
• Using typed function signatures will make your code easier to read and more likely to break in obvious ways. The advantage being that it's less likely to break in insidious ways. I strongly recommend this.
• In DFriend's code:
  • $inserted is assigned a value in the conditional of an if statement. Don't do that.
  • Don't rely on redirect or load->view() to stop execution. Even when it works it makes things harder to read. Use explicit return statements, or put the alternate path in an explicit else, or both.

Edit: Hi DFriend!

You raise several points; it sounded like you were hoping for a response.

• I like your point that brevity is part of clarity. The other point of clarity is saying what you mean, and saying it as code is briefer and stronger than leaving a comment.
• Sanitizing, validating, and parsing inputs isn't business logic, as I understand the phrase. A key role of the Controller is to handing inputs. If we take the framework for granted (as we would like to), then we conceptually enter the program in the Controller endpoint method. That method needs to know about inputs (POST values, query strings, url fragments, cookies) because that's where all further action will be directed from. The thing we can do to maintain separation of concerns is make sure nobody else needs to read those input sources.
• redirect() stops execution, but you have to know and remember that to make sense of code that relies on it. CI->load->view() doesn't stop execution, but is almost always the last call in the flow. Throwing a return; after all of them is a little verbose, but it's clear.
• It's a fact of life that there will be a lot of conditionals, and I really don't think there's a single pattern that will cover all cases. That said, if you're relying on a section of code only running if a prior if failed, then you're in an else section whether you say so or not.