Login Controller

Question

Here's one more controller I'm trying to get reviewed for. Any thoughts?

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

class Login extends CI_Controller { 

public function __construct()
{
    parent::__construct();
    $this->load->model('users/usersmodel');
}

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 =  '<script src='.base_url().'assets/'. $this->config->item('defaultTemplate') .'/js/validate/login.js></script>';//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('xtr') == "yes") 
    {                                   
        redirect('cpanel');
    } 
    else
    {
        $bodyContent = $this->config->item('defaultTemplate') ."/usermanagement/forms/login_form";//which view file
        $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['siteTitle'] = $siteTitle;//defaults can be changed via models/metatags.php
    $this->data['bodyType'] = $bodyType;
    $this->data['bodyContent'] = $bodyContent;
    $this->load->view($this->config->item('defaultTemplate') .'/usermanagement/index', $this->data);
}

function submit()
{
    // Sets validation rules for the login form
    $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');

    // Checks to see if login form was submitted properly
    if ($this->form_validation->run() === false)
    {
        $outputArray = array('error' => 'yes', 'message' => 'There was a problem submitting the form! Please refresh the window and try again!');  
    }
    else
    {
        if (is_null($userData = $this->usersmodel->getUserByUsername($this->input->post('username'))))
        {
            // Username was not found in the database
            $outputArray = array('error' => 'yes', 'message' => 'Incorrect username and password combination!');  
        }
        else
        {
            // Checks to see if user has exceeded max login attempts
            if ($this->auth->isMaxLoginAttemptsExceeded($userData->userID))
            {
                // Max was exceeded and sends email to account holder
                $outputArray = array('error' => 'yes', 'message' => 'Your account is currently locked, we appologize for the inconvienence. You must wait 10 minutes before you can login again! An email was sent to the owner of this account!');  
                $userData = array('userID' => $userData->userID, 'firstName' => $userData->firstName, 'lastName' => $userData->lastName, 'email' => $userData->email, 'username' => $userData->username);
                $this->auth->sendEmail($this->config->item('defaultTemplate'), 'maxlogins', 'KOW Manager Account Locked', $userData);  
            }
            else
            {
                // Matches user's status for validity
                switch($userData->usersStatusesID)
                {
                    // Registered not validated
                    case 1:
                        $outputArray = array('error' => 'yes', 'message' => 'Sorry you must verify your account before logging in!');  
                        break;
                    // Account suspended
                    case 3:  
                        $outputArray = array('error' => 'yes', 'message' => 'Your account has been suspended!');  
                        break;
                    // Account Banned
                    case 4:
                        $outputArray = array('error' => 'yes', 'message' => 'Your account is currently banned!');  
                        break;
                    // Account Deleted
                    case 5:  
                        $outputArray = array('error' => 'yes', 'message' => 'Your account has been deleted!');  
                        break;
                    // Registered and validated
                    default:
                        // Checks to see if login was successful
                        if ($this->auth->login($this->input->post('username'), $this->input->post('password'), $this->input->post('remember')))
                        {
                            // Login was successful
                            $outputArray = array('success' => 'Yes', 'message' => 'Sending to control panel!');    
                        }
                        else
                        {
                            // Login failed
                            $outputArray = array('error' => 'yes', 'message' => 'Incorrect username and password combination!');  
                        } 
                }
            }

        }
    }
    echo json_encode($outputArray);      
}
}

/* End of file login.php */ 
/* Location: ./application/controllers/login.php */ 

KOW Auth:

/**
* Login user on the site. Return TRUE if login is successful
* (user exists and activated, password is correct), otherwise FALSE.
*
* @param       string
* @param       string
* @param       integer
* @return      array
*/
function login($username, $password, $remember)
{  
    $this->ci->load->model('users/usersmodel');

    if ($userData = $this->ci->usersmodel->getUserByUsername($username))
    {
        $regenFromPostPW = $this->ci->genfunc->reGenPassHash($password, $userData->passwordHash);
        if ($regenFromPostPW == $userData->password)
        {
            $this->ci->session->set_userdata(array(
                            'xtr'       => 'yes',
                            'userID'    => $userData->userID,
                            'username'  => $userData->username,
                            'role'      => $userData->usersRolesID
            ));
            if ($remember == 1)
            {
               //$this->create_autologin($user_data->user_id);
            }
            $this->clearLoginUserAttempts($userData->userID);
            $this->session->all_userdata();

            //$this->ci->users->insert_session($this->session->userdata('session_id'), $this->session->userdata('user_id'), $this->genfunc->getRealIpAddr(), $this->session->userdata('user_agent'));
            return $userData;
        }
        else
        {
            $this->increaseLoginUserAttempt($userData->userID);
        }
    }
    else
    {
        return null;
    }

}

Answer 1

I've already reviewed some of your other code, so I will only add new items in an attempt not to repeat posts

Booleans

There is no reason to recreate a feature that already exists. By checking for a "yes" or "no" or a 1 or 0 value you are recreating a boolean. Simpler to just use TRUE/FALSE this way you can check your variables like so.

if ($this->session->userdata('xtr'))
//instead of
if ($this->session->userdata('xtr') == "yes")

//OR
if ($remember)
//instead of
if ($remember == 1)

Redirects

I'm not familiar with CodeIgniter, but if redirect() interrupts the current code flow and physically redirects to a new page, then there is no reason to use an else statement. As it will never be processed. Save yourself from those indents.

Variables in Statements

Defining a variable in a statement is bad! There are few exceptions to this rule, and even those exceptions can be avoided. Declaring a variable in any kind of statement is wrong. A couple of reasons for this. First, it could be accidental and your IDE and PHP will have no way of telling you because it is valid code. Second, it adds unnecessary complexity to the statement that hinders legibility.

$postUser = $this->input->post('username');
$userData = $this->usermodel->getUserByUsername($postUser);
if (is_null($userData ))

And I already mentioned is_null() in my last review :)

outputArray

I've already mentioned how I think the "error" element should be a boolean. This is to serve two purposes. First, was the one I already mentioned, about not recreating a feature. The second was to do away with the "success" element. No reason to have two separate elements that accomplish more or less the same thing. If there is an error, then success is obviously false. If there is no error, then success is obviously true. So you see, you can just use the "error" element for both.

There is also unnecessary duplication here. I would create a default $outputArray at the beginning of my submit() method that contains default values. Then, instead of recreating the array every time I wanted to set the message, I would just set the "message" directly. The only time you need to do anything extra is when you have "success" in which case you need to change the value of "error" to false.

$outputArray = array(
    'error' => TRUE,
    'message' => 'There was a problem submitting the form! Please refresh the window and try again!'
);

if ($this->form_validation->run() !== FALSE) {
    //etc...

    if ($this->auth->login($this->input->post('username'), $this->input->post('password'), $this->input->post('remember')))
    {
        // Login was successful
        $outputArray['error'] = FALSE;
        $outputArray['message'] = 'Sending to control panel!';
    }
    else
    {
        // Login failed
        $outputArray['message'] = 'Incorrect username and password combination!';
    } 
}

Default Values

The cool thing about if/else statements is that the else is usually unnecessary. When coding, be a pessimist. Assume the worst will happen and perform that action by default. Then you only have to plan for success. You could technically do it the other way around, but better this way in case you miss something. That way the script is already prepared to handle an error.

So here's an example using the code from above...

$outputArray['message'] = 'Incorrect username and password combination!';

if ($this->auth->login($this->input->post('username'), $this->input->post('password'), $this->input->post('remember')))
{
    // Login was successful
    $outputArray['error'] = FALSE;
    $outputArray['message'] = 'Sending to control panel!';
}

Shortening Your Code

This line is rather bulky...

if ($this->auth->login($this->input->post('username'), $this->input->post('password'), $this->input->post('remember')))

You don't really use these anywhere else, but I still think it would be worth it to set these up as variables to improve legibility. Since you do, use the "username" only once more, I would set them towards the top of the method so that they can be used in both places. So...

$username = $this->input->post('username');
$password = $this->input->post('password');
$remember = $this->input->post('remember');
//usage
if ($this->auth->login($username, $password, $remember))

Methods

Your submit() and login() methods have not been defined as public/private/protected. By default it is set to public, but you should not depend upon PHP always providing a default, or the same default.

Magical toarray Method

Well, not so magical, because it doesn't exist, but you can get the same functionality out of PHP's get_object_vars() function. So, assuming that the only public properties you have in the $userData class are the ones you are seeking, or at the very least you do not mind having extra unused properties in your $userData array, you can get the same results for your $userData array like so:

$userData = get_object_vars($userData);
//instead of...
$userData = array('userID' => $userData->userID, 'firstName' => $userData->firstName, 'lastName' => $userData->lastName, 'email' => $userData->email, 'username' => $userData->username);

Final Thoughts

What is "xtr"? Your variables should be a bit more descriptive than that.

UPDATE

Methods

A public method or property is one you can access from outside the current scope of a class. Whether in the traditional $class-> method or the static class:: method. In other words, if you were to load this class into another file and create a new instance of it, that file will be able to use any public methods or properties simply by calling the class.

Private and protected methods and properties are just the opposite. Both are only accessible from within the class themselves. You can only call them within the class via $this-> or statically via class::. If you were to load this class into another file and create a new instance of it, all private and protected methods and properties would be invisible to it and inaccessible. They are only available within the class' scope. So if the file used the public method getPrivateData to retrieve the private property $privateData it would not know where that information came from and would not be able to specifically ask for the $privateData property. This is ideal for information you do not wish someone to have access to, such as a password or something else critical in nature. The difference between private and protected is that a protected method or property is used for class inheritance and only a child class and the parent class can access them whereas private methods and properties are only accessible by the parent and invisible to the child.

If this is not clear enough you'll have to look at the PHP documentation.

NULL

If you are specifically looking for NULL you should use the is_null() function or the absolute equality === operator instead of loose equality == operator. Doing a loose comparison of NULL will return TRUE for empty strings, "null", "NULL", null, NULL, and 0, not sure about FALSE... So you might as well use a boolean comparison (! $userData) as to use the loose comparison. They will both return more or less the same thing and the later is cleaner.

if ($userData === null)
//OR
if (is_null($userData))

One Last Thing

As I mentioned earlier, if you are a "coding pessimist", there is no need for all these else statements. We set the $outputArray['message'] with a default value that handled the first error should it occur. This is being pessimistic. We are assuming the code will fail and have already set up our code to handle it should it do so. We laugh at the optimists for they assume their code will work and will eventually be proven wrong when they meet an error they never expected. We do not want to be optimistic. We pity the hesitant coder for their many if/else statements are hard to read and their code heavily indented. We do not want to be hesitant. We are pessimists and we have handled all eventualities before they occur. We tell our code it is wrong and challenge it to "Trial by If Statements". And we do it again and again until we are satisfied. Each time telling it how it is wrong and what it can do to get better. We are emotionally abusive to our code, but it will grow the stronger for it. Only once our code has proven itself and passed all of our tests will we declare it fit and can be proud of it.

Excuse my fun little rant, but if you read it and take it to heart you'll have the right mindset. To clarify, once inside each if statement our code has literally told us it is not wrong and the previous statement about it (error) is no longer valid. So we do it again and again until there are no more if statements. Because we assume our code is wrong there is no need for else statements, and should some unforseen error occur we don't have to do anything extra to it because we have already assumed it is wrong. This can be done with any if/else block.