Search code examples
phpcodeigniterregistrationlogin-script

Codeigniter user registration: split login fail condition in 2 conditions to cover "account inactive" case


I have made a Registration and Login application with Codeigniter 3.

When someone fills the Registration form and submits it successfully, the "active" column of the "users" table receives the value 0, as visible in the image bellow:

enter image description here

Users will have to activate their accounts before being able to sign in.

In the Signin.php controller I have the signin() function:

public function signin()
{  
  $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">', '</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, $active);
    // Set the current user's data
    if ($current_user) {
     $this->session->set_userdata(
       array(
        'user_id' => $current_user->id,
        'user_email' => $current_user->email,
        'user_first_name' => $current_user->fname,
        'is_logged_in' => TRUE
        )
       );
     redirect('home');  
   } else {
      $this->session->set_flashdata("signin_failure", "Incorrect email or password");
      redirect('signin'); 
    }
  }
  else
  {
   $this->load->view('signin');
 }
} 

I want, instead of the line $this->session->set_flashdata("signin_failure", "Incorrect email or password"); in the code above, to be able to "split" the login failure condition in 2: Incorrect email or Password and account has not been activated.

if (condition here) {
      $this->session->set_flashdata("signin_failure", "Your account has not been activated");
    } else {
      $this->session->set_flashdata("signin_failure", "Incorrect email or password");
 }

My question: what should I put instead of condition here in the code above?

More specifically: how do I say: if the "active" column has the value 0 do $this->session->set_flashdata("signin_failure", "Your account has not been activated");?

The user_login() function inside the Usermodel:

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

UPDATE:

I came up with this:

public function signin()
  {  
  $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">', '</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->fname,
          'user_active' => $current_user->active,
          'is_logged_in' => TRUE
          )
         );
        redirect('home');  
      } else {
        // If the user found is NOT active
        $this->session->set_flashdata("signin_failure", "Your account has not been activated");
        redirect('signin'); 
      }
    } else {
      // If we do NOT find a user
      $this->session->set_flashdata("signin_failure", "Incorrect email or password");
      redirect('signin'); 
    }
  }
  else
  {
   $this->load->view('signin');
 }
}

but there is a flaw in it because even when the email and password are correct, but the user is inactive, the message is: "Incorrect email or password" Instead of "Your account has not been activated".


Solution

  • Just remove the check for active from user_login function in model. As you are already checking id the user is active or not in your controller. It should not impact your working.

    $query = $this->db->get_where('users', ['email' => $email, 'password' => md5($password)]);
    

    EDIT:

    Well elaborated answer Contribution by JayAdra in the Codeigniter forum here

    This is because your first if statement is:

    if ($current_user) { 
    

    Which will return false for an inactive user, as your query is:

    $query = $this->db->get_where('users', ['email' => $email, 'password' => md5($password), 'active' => 1]); 
    

    Notice, the check for "active" => 1, meaning it won't return any records for inactive users.

    So your first if statement returns false, hence going to the else clause which has:

    $this->session->set_flashdata("signin_failure", "Incorrect email or password"); 
    

    So you probably need to check if the user is active first, before checking if their username/password is correct.

    I'd suggest splitting your "user_login" function into two distinct functions. One to check if the user is active, and one to test the user/pass combo.

    Lastly, I noticed you're storing your password as md5 strings... this is a bad idea. It's not secure. Use bcrypt or similar.