Login Validation

Question

I'm hoping someone is willing and can take a minute to look over this function and tell me what they think of it, if it can be improved or what not. I tried commenting out what the purpose of everything is. I would like to note that something isn't quite right with the time_remaining and what not because what's supposed to happen when its been passed the 10 minutes the users account clears out the failed logins and redirects to the index function but doesn't.

EDIT: I have redone my code and was hoping someone could tell me what they thought of it now?

<?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');

class Usermanagement extends CI_Controller { 

public function __construct()
{
    parent::__construct();
}   

public function index()
{
    //Config Defaults Start
    $msgBoxMsgs = array();//msgType = dl, info, warn, note, msg
    $cssPageAddons = '';//If you have extra CSS for this view append it here
    $jsPageAddons = '';//If you have extra JS for this view append it here
    $metaAddons = '';//Sometimes there is a need for additional Meta Data such in the case of Facebook addon's
    $siteTitle = '';//alter only if you need something other than the default for this view.
    //Config Defaults Start


    //examples of how to use the message box system (css not included).
    //$msgBoxMsgs[] = array('msgType' => 'dl', 'theMsg' => 'This is a Blank Message Box...');

    /**********************************************************Your Coding Logic Here, Start*/

    if(!$this->session->userdata('logged_in'))
    {
        $bodyContent = "login";//which view file
    }
    else
    {
        redirect('cpanel/index');     
    }

    $bodyType = "full";//type of template

    /***********************************************************Your Coding Logic Here, End*/

    //Double checks if any default variables have been changed, Start.
    //If msgBoxMsgs array has anything in it, if so displays it in view, else does nothing.      
    if(count($msgBoxMsgs) !== 0)
    {
        $msgBoxes = $this->msgboxes->buildMsgBoxesOutput(array('display' => 'show', 'msgs' =>$msgBoxMsgs));
    }
    else
    {
        $msgBoxes = array('display' => 'none');
    }

    if($siteTitle == '')
    {
        $siteTitle = $this->metatags->SiteTitle(); //reads 
    }

    //Double checks if any default variables have been changed, End.

    $this->data['msgBoxes'] = $msgBoxes;
    $this->data['cssPageAddons'] = $cssPageAddons;//if there is any additional CSS to add from above Variable this will send it to the view.
    $this->data['jsPageAddons'] = $jsPageAddons;//if there is any addictional JS to add from the above variable this will send it to the view.
    $this->data['metaAddons'] = $metaAddons;//if there is any addictional meta data to add from the above variable this will send it to the view.
    $this->data['pageMetaTags'] = $this->metatags->MetaTags();//defaults can be changed via models/metatags.php
    $this->data['siteTitle'] = $siteTitle;//defaults can be changed via models/metatags.php
    $this->data['bodyType'] = $bodyType;
    $this->data['bodyContent'] = $bodyContent;
    $this->load->view('usermanagement/index', $this->data);
}

function login()
{
    // Set validation rules for login form fields
    $this->form_validation->set_rules('username', 'Username', 'trim|required|max_length[50]|xss_clean');
    $this->form_validation->set_rules('password', 'Password', 'trim|required|max_length[12]|xss_clean');

    // Verify login form was submitted
    if ($this->form_validation->run() == FALSE)
    {
        // Form was not submitted
        redirect('usermanagement/index');     
    }
    else
    {
        // Create variables from login form submission
        $username = $this->input->post('username');
        $password = $this->input->post('password');

        // Run function to check if username exists in database
        $user_data = $this->loggedin->get_user_data($username);

        // No username match in database
        if($user_data == false)
        {
            echo json_encode(array('error' => 'yes', 'msg' => 'Username not found, check your username and try again.', 'flags' => 3));
        }
        else 
        {
            // User was found in database

            // Check account status of username
            if ($user_data->users_statuses_id == 1)
            {
                echo json_encode(array('error' => 'yes', 'msg' => 'Account has not been authenticated. Check your email for verification instructions!', 'flags' => 3));    
            }
            elseif ($user_data->users_statuses_id == 3)
            {
                echo json_encode(array('error' => 'yes', 'msg' => 'Account has been suspended!', 'flags' => 3));
            }
            elseif ($user_data->users_statuses_id == 4)
            {
                echo json_encode(array('error' => 'yes', 'msg' => 'Account has been banned!', 'flags' => 3));
            }
            elseif ($user_data->users_statuses_id == 5)
            {
                echo json_encode(array('error' => 'yes', 'msg' => 'Account has been deleted!', 'flags' => 3));
            }
            else
            {
                // Create a user_id variable from returned data
                $user_id = $user_data->user_id;

                $lock_date = $user_data->lock_date;
                $current_time = time();
                $failed_logins = $user_data->failed_logins;

                // Check to see if account is currently locked
                if ($lock_date !== "0000-00-00 00:00:00") 
                { 
                    if((strtotime($lock_date) < $current_time && $failed_logins > 0)) 
                    {
                        $lock_date = strtotime($lock_date);

                        // See how much time is remaining efore user can login again
                        $time_difference = $this->genfunc->time_since($lock_date);

                        // Time must be
                        if ($time_difference <= 10) 
                        {
                            // Has not been 10 minutes yet
                            $time_remaining = 10 - $time_difference;
                            echo json_encode(array('error' => 'yes', 'msg' => 'Your account is currently locked, we appologize for the inconvienence. You must wait ' .$time_remaining.' minutes before you can log in again!', 'flags' => 3));
                        } 
                        else 
                        {
                            // Clear the lock
                            $this->loggedin->clear_login_attempts($user_id);
                            echo json_encode(array('error' => 'no', 'msg' => 'Your account is now unlocked, you may now log in again!', 'flags' => 3));                             
                        }
                    }                                                             
                }
                else
                {   
                    // Assign variables to returned data 
                    $passwordDB = $user_data->password;
                    $passwordDB2 = $user_data->password2;
                    $first_name = $user_data->first_name;
                    $last_name = $user_data->last_name;
                    $email = $user_data->email;
                    $users_roles_id = $user_data->users_roles_id;
                    $generated_password = $this->genfunc->reGenPassHash($password, $passwordDB2);
                    $date_time = $this->genfunc->unixToMySQL($current_time);

                    // Passwords match username in database
                    if ($passwordDB == $generated_password) 
                    {
                        // Run functions for user to update logins and clearing of failed attempts
                        $this->loggedin->update_logins($user_id);
                        $this->loggedin->clear_login_attempts($user_id); 

                        // Create variables about user and assign them to session for database entry
                        $op_system = $this->genfunc->getUserOS();
                        $user_ip = $this->genfunc->getRealIpAddr();
                        $user_browser = $this->session->userdata('user_agent');
                        $session_id = $this->session->userdata('session_id');
                        $this->loggedin->insert_session($user_id, $session_id, $user_ip, $user_browser, $date_time);

                        $this->session->set_userdata(array(
                            'session_id' => $session_id,
                            'logged_in' => TRUE,
                            'user_id' => $user_id,
                            'username' => $username,
                            'users_roles_id' => $users_roles_id

                        ));
                        redirect('cpanel/index');     
                    }
                    else 
                    {
                        // Passwords didn't match in database

                        // Take failed logins and compare it 
                        if ($failed_logins == 5) 
                        {
                            // Retrieve IP Address of user trying to hack into account
                            $hacker_ip_address = $this->genfunc->getRealIpAddr();

                            $this->loggedin->update_hacked_account($user_id, $hacker_ip_address, $date_time);
                            $my_domain_name = $this->genfunc->myDomainName();

                            // Email user new registration account
                            $sender_email = "[email protected]";
                            $reply_to = "[email protected]";
                            $recipient_email = $email; 
                            $email_subject = "KOW Manager Account Locked";

                            $email_body = 'Hello '.$first_name.' '.$last_name.' You, or someone using your account at '.$my_domain_name.', has attempted to hack into your account. If this is an error, ignore this email and you will be removed from our mailing list.<br /><br />Regards, '.$my_domain_name.' Team';

                            $this->genfunc->mailSomeone($email, $sender_email, $email_subject, $email_body);

                            echo json_encode(array('error' => 'yes', 'msg' => '"Your account is currently locked, we appologize for the inconvienence. This is a security messure implimented by to many failed login\'s! You must wait 10 minutes before you can login again!', 'flags' => 3));     


                        }
                        else
                        {
                            // Upate amount of failed logins
                            $this->loggedin->update_failed_logins($user_id);

                            $chances_left = 5 - $failed_logins;  
                            echo json_encode(array('error' => 'yes', 'msg' => 'Invalid Username and Password combination! You have ' .$chances_left. ' chances left to login succesfully or the account will be locked!', 'flags' => 3));     
                        }  
                    }                        
                }   
            }  
        }
    }
}

function logout()
{
   $this->session->sess_destroy();
   redirect('usermanagement/index');     
}       

}
/* End of file usermanagement.php */ 
/* Location: ./application/controllers/usermanagement.php */ 

MODEL :

<?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');

class Loggedin extends CI_Model 
{

/**
 * Loggedin::__construct()
 * 
 * @return
 */
function __construct()
{
    parent::__construct();
}

/**
 * Loggedin::get_user_data()
 * 
 * @param string $username Username that was posted via login script
 * @return $query->row OR false depending on if a user was found in the database
 */
public function get_user_data($username)
{
    $this->db->select('*');
    $this->db->from('users');
    $this->db->join('users_logins', 'users.user_id = users_logins.user_id');
    $this->db->where('users.username', $username);
    $query = $this->db->get();   

    if ($query->num_rows() > 0)
    {
        return $query->row();
    }
    else
    {
        return false;
    }
}

/**
 * Loggedin::clear_login_attempts()
 * 
 * @param int $user_id  $user_id of the account trying to be accessed
 */
public function clear_login_attempts($user_id)
{
    $this->db->set('lock_date', 'NULL');
    $this->db->set('hacker_ip_address', 'NULL');
    $this->db->set('failed_logins', '0');                                                
    $this->db->where('user_id', $user_id);   
    $this->db->update('users_logins');         
}

/**
 * Loggedin::update_logins()
 * 
 *
 */
public function update_logins($user_id)
{
    $this->db->query('UPDATE users_logins SET number_of_logins = number_of_logins+1 WHERE user_id ='.$user_id);           
}

/**
 * Loggedin::insert_session()
 * 
 * @param int $user_id  $user_id of the account trying to be accessed
 */
public function insert_session($user_id, $session_id, $user_ip, $user_browser, $current_time)
{
    $data = array(
       'user_id' => $user_id ,
       'session_id' => $session_id,
       'ip_address' => $user_ip,
       'user_agent' => $user_browser,
       'session_started' => $current_time                      
    );
    $this->db->insert('users_logins_sessions', $data); 
}  

/**
 * Loggedin::update_failed_logins()
 * 
 * @param int $user_id  $user_id of the account trying to be accessed
 */
public function update_failed_logins($user_id)
{
    $this->db->query('UPDATE users_logins SET failed_logins = failed_logins+1 WHERE user_id ='.$user_id);           
} 

/**
 * Loggedin::update_hacked_account()
 * 
 * 
 */
public function update_hacked_account($user_id, $hacker_ip_address, $current_time)
{
    $this->db->set('lock_date', $current_time);
    $this->db->set('hacker_ip_address', $hacker_ip_address);                                              
    $this->db->where('user_id', $user_id);   
    $this->db->update('users_logins');   
}   

}
/* End of file loggedin.php */
/* Location: ./app/models/loggedin.php */

EDIT 2:

I have rewritten it all just to try and start over that way it'd be possible to accomplish all tasks in the correct logical order as well as with the smallest bit of code. I tend to fail at both of those things.

Revised code:

Controller:

function login_submit()
{
    $this->form_validation->set_rules('username', 'Username', 'trim|required|xss_clean');
    $this->form_validation->set_rules('password', 'Password', 'trim|required|xss_clean');
    $this->form_validation->set_rules('remember', 'Remember me', 'integer');

    $user_id = $this->users->get_user_id_by_username($this->input->post('username'));

    if ($user_id !== 0)
    {
        if ($this->kow_auth->is_max_login_attempts_exceeded($user_id))
        {
            echo json_encode(array('error' => 'yes', 'message' => 'Your account is currently locked, we appologize for the inconvienence. You must wait 10 minutes before you can login again!'));
        }
        else
        {
            $user_status = $this->users->get_user_status($user_id);

            if ($user_status == 1)
            {
                echo json_encode(array('error' => 'yes', 'message' => 'Sorry you must verify your account before logging in!'));                    
            }
            elseif ($user_status == 3)
            {
                echo json_encode(array('error' => 'yes', 'message' => 'Your account has been suspended!'));          
            }
            elseif ($user_status == 4)
            {
                echo json_encode(array('error' => 'yes', 'message' => 'Your account is currently banned!'));          
            } 
            elseif ($user_status == 5)
            {
                echo json_encode(array('error' => 'yes', 'message' => 'Your account has been deleted!'));     
            }
            else
            {

            }                                             
        }               
    } 
    else 
    {
        echo json_encode(array('error' => 'yes', 'message' => 'Incorrect username and password combination!'));
    }            

}

Models:

Users Model

<?php if (!defined('BASEPATH')) exit('No direct script access allowed');

/**
* Users
*
* This model represents user authentication data. It operates the following tables:
* - user account data,
* - user profiles
*
* @package  Kow_auth
* @author   Jeffrey Davidson
*/
class Users extends CI_Model
{
function __construct()
{
    parent::__construct();
}

function get_user_id_by_username($username)
{
    $this->db->select('user_id');
    $this->db->where('username', $username);
    $query = $this->db->get('users');

    if ($query->num_rows() > 0)
    {
        $row = $query->row();
        return $row->user_id;
    } 
    else
    {
        return 0;      
    }

}

function get_user_status($user_id)
{
    $this->db->select('users_statuses_id ');
    $this->db->where('user_id', $user_id);
    $query = $this->db->get('users');

    if ($query->num_rows() > 0)
    {
        $row = $query->row();
        return $row->users_statuses_id;
    } 
    else
    {
        return 0;      
    }

}
}

Login Attempts Model

<?php if (!defined('BASEPATH')) exit('No direct script access allowed');

/**
 * Login_attempts
 *
 *  This model serves to watch on all attempts to login on the site
 * (to protect the site from brute-force attack to user database)
 *
 * @package Kow_auth
 * @author  Jeffrey Davidson
 */
 class Login_attempts extends CI_Model
{
function __construct()
{
    parent::__construct();
}

function get_attempts_num($user_id)
{
    $this->db->select('failed_logins');
    $this->db->from('users_logins');
    $this->db->where('user_id', $user_id);
    $query = $this->db->get();

    if ($query->num_rows() > 0)
    {
        $row = $query->row();
        return $row->failed_logins;
    } 
    else
    {
        return 0;      
    }
}

}

Library:

<?php if (!defined('BASEPATH')) exit('No direct script access allowed');

/**
 * KOW Auth Library
 * Authentication Library for Code Igniter
 * @author Jeffrey Davidson
 * @version 1.0.0
 * @copyright 2012
 */

class Kow_auth
{

function __construct()
{
    //assign the CI superglobal to $CI
    $this->ci =& get_instance();             
}

function is_max_login_attempts_exceeded($user_id)
{
    $this->ci->load->model('kow_auth/login_attempts');
    $login_attempts = $this->ci->login_attempts->get_attempts_num($user_id);
    if ($login_attempts >= 5)
    {
        return true;
    }
    else
    {
        return false;
    }
}

} 

?>

Answer 1

Okey, first of all; I had a hard time determining if this was a login script or a registration script even if you explicitly said it was a script containing login validation. If I was you I would review the code and try to clear up any vagueness, because I got a headache trying to sort your code out. Here we go anyway:

At line 6: Why do you use xss_clean() if you're not inserting the data into the database for later display to the user? Use remove_invisible_characters() instead.

By the looks of it, you never do any credential checks against the database before starting the log-in routine. This is prone to brute force attacks if you let your attackers check if the username already exists before giving an error message such as 'Wrong username/password'. I tried to figure out exactly where you did the login function, but I couldn't find it.

What exactly is $user_login_data? Shouldn't this information already exist in $user_data? I would recommend using an independent method such as getUserData() instead of returning a data object by a function that is supposed to check if a username exists.

Why do you load first name, last name, e-mail address and two passwords? At this point I was so confused I could barely keep track of the script's purpose. It would remind more of a registration form.

You should use cleaner variable names such as $user_data instead of $user_data[0] everywhere, it can quickly get messy if you decide to change things.

What is $users_statuses_id? I would call this $status_code or something similar. Since variables bound to an object reside within it, there is no point in calling them $user_some_info since the object should already state the purpose well enough by it's name: $user->some_info.

It's hard to know what functions such as genfunc->time_since() does without knowing the code. Though you seem to be checking seconds instead of minutes. if ($time_difference >= 10) should be if ($time_difference >= (10*60)).

Why do you use genfunc->getRealIpAddr(); at one place and $_SERVER['REMOTE_ADDR']; another?

Vague function names: loggedin->update_hacked_account(), genfunc->mailSomeone()? etc.

Answer 2

Some notes:

1,

elseif ($user_data->users_statuses_id == 4)    

Instead of magic numbers you should use named constants. Consider creating an $user_data->isUserBanned() function also. The same is true for 'flags' => 3 and ($lock_date !== "0000-00-00 00:00:00") (the latter should be $user_data->isLocked()). It helps readers a lot and improves encapsulation.

2, Sender's e-mail address should be in a configuration file as well as the e-mail template.

3, Extract out smaller function. For example, code below

// Create variables about user and assign them to session for database entry    

should be in a function called storeUserDataInSession().

4, The code really needs flattening. Extracting out some functions would help.

Answer 3

Another recommendation apart from all of the good ones you have already received.

Don't create variables that are not variables.

                // Assign variables to returned data 
                $passwordDB = $user_data->password;
                $passwordDB2 = $user_data->password2;
                $first_name = $user_data->first_name;
                $last_name = $user_data->last_name;
                $email = $user_data->email;
                $users_roles_id = $user_data->users_roles_id;

These variables are assigned once and used once (or possibly twice), however they are never modified. They only abstract away the details of what is being done later in the function. Accessing the object properties is more descriptive than referring to local variables. When you have a local variable you have to track back to where it was last set to see its value. Also, there is less clutter without these lines that do very little.

Personally I like to handle data in larger chunks rather than lots of fields spread everywhere. I would be using arrays to hold the data and avoid line by line first name, last name, email access. Here is how I would do it for the if block that follows the variables i referred to above.

                // No need to Assign variable to returned data.

                if ($user_data->valid_password())
                {
                    // loggedin doesn't feel like a real object, it looks like
                    // something you are using to call functions with.  I would
                    // call it login_manager if it was managing the logins.
                    $this->loggedin->update_logins($user_id);
                    $this->loggedin->clear_login_attempts($user_id); 

                    // Again, I wouldn't Create variables about user and assign them to session for database entry

                    // This call makes loggedin do something to the session.
                    // loggedin should be called with things about logins, not
                    // with direct calls to modify the session.
                    // $this->loggedin->insert_session($user_id, $session_id, $user_ip, $user_browser, $date_time);

                    // The logic for this belongs in the session.
                    $this->session->set_userdata($user_data);
                    redirect('cpanel/index');     
                }

EDIT 2: This code looks a lot better. It is less cluttered by lots of field settings. Actually, I think it looks like its finished now. I can't see too much to critique.

One thing of interest is you specify the from in your SQL query in the Login_Attempts model, but in the Users model it seems to be part of the get?

Premature optimization is the root of all evil. So I'm not going to suggest that you combine all of your database queries into 1 query. What I would have done was have 1 model that combined the data from the two tables. The SQL would look something like:

SELECT * FROM users LEFT JOIN users_logins on users.user_id=users_logins.user_id WHERE users.username=?

* should possibly be replaced with the exact fields you wanted.

I suggest you keep it as you have it, but have only included this for you to remember if you find this to be a bottleneck. I would go straight to this sort of solution on heavily used parts of the system or anything within a highly nested loop.