-1
\$\begingroup\$

I'm reviewing my code and trying to figure out how I can do some simplifying to flatten my code. Whether it be making simpler function out of the segment of code or by removing parts of it and wanted some opinions.

if ($regenerated_post_password !== $user_data->password)
    {
        /* Password from login from does not match user stored password */

        if ($failed_logins == 0)
        {
            /* First time user has not entered username and password successfully */
            $this->session->set_userdata('failed_logins', 1);
            $this->users_model->increase_login_attempt($this->input->ip_address(), $post_username, gmdate('Y-m-d H:i:s', time()));
            $this->output('Incorrect username and password credentials!', 'Incorrect Login Credentials', 'Error');
            return;
        }

        /* User has atleast one failed login attempt for the current session */

        if ($failed_logins !== 4)
        {
            /* User has a few more chances to get password right */
            $failed_logins++;
            $this->session->set_userdata('failed_logins', $failed_logins);
            $this->users_model->increase_login_attempt($this->input->ip_address(), $post_username, gmdate('Y-m-d H:i:s', time()));
            $this->output('Incorrect username and password credentials!', 'Incorrect Login Credentials', 'Error');
            return;
        }

        $this->users_model->lock_out_user($user_data->user_id, gmdate('Y-m-d H:i:s', time()+(60*15)));
        //$this->functions_model->send_email('maximum_failed_login_attempts_exceeded', $user_data->email_address, $user_data)
        $this->output('Your account is currently locked, we apologize for the inconvienence. You must wait 15 minutes before you can log in again! An email was sent to the owner of this account! Forgotten your username or password? <a href="forgotusername">Forgot Username</a> or <a href="forgotpassword">Forgot Password</a>', 'Account Locked', 'Error');
        return;
    }

The entire controller:

<?php

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

class Login extends Backend_Controller
{
public function __construct()
{
    parent::__construct();
}

public function index()
{
    Asset::js('theme::login.js');

    $this->template
    ->title('Login')
    ->set_layout('usermanagement')
    ->build('login');
}

public function form_is_valid()
{
    /* Set validation rules for post data */
    $this->form_validation->set_rules('username', 'Username', 'trim|required|xss_clean|min_length[6]|max_length[12]|regex_match[/[a-z0-9]/]');
    $this->form_validation->set_rules('password', 'Password', 'trim|required|xss_clean|min_length[6]|max_length[12]|regex_match[/[a-z0-9]/]');
    //$this->form_validation->set_rules('remember', 'Remember Me', 'trim|xss_clean|integer');

    /* Form validation passed */
    return $this->form_validation->run();
}

public function is_user_locked($user_lock_date, $user_id)
{
    if (strtotime($user_lock_date) > 0)
    {
        /* User is locked out */

        if (strtotime(gmdate('Y-m-d H:i:s', time())) < strtotime($user_lock_date))
        {
            /* User is still locked out */
            return TRUE;
        }
        else
        {
            /* User can be unlocked and form be resubmitted */
            $this->users_model->unlock_user($user_id);

            /* User is no longer locked out */
            return FALSE;
        }
    }
}

public function check_user_status($user_status_id)
{
    /* Match user status */
    switch ($user_status_id)
    {
        case 1:
            $this->output('Sorry you must verify your account before logging in!', 'Account Unverified', 'Error');
            break;
        case 3:
            $this->output('Your account has been suspended!', 'Account Suspended', 'Error');
            break;
        case 4:
            $this->output('Your account has been suspended!', 'Account Banned', 'Error');
            break;
        case 5:
            $this->output('Your account has been deleted!', 'Account Deleted', 'Error');
            break;
        default:
            return;
    }
}

public function output($message, $title, $status)
{
    switch (strtoupper($status))
    {
        default:
        case 'ERROR':
            $status = 'Error';
            break;
        case 'NOTICE':
            $status = 'Notice';
            break;
        case 'SUCCESS':
            $status = 'Success';
            break;
        }
        $this->output
            ->set_content_type('application/json')
            ->set_output(json_encode(array('output_status' => $status, 'output_title' => $title, 'output_message' => $message)));
}

public function start_user_session($user_id)
{
    /* Start session with user id and clear previous failed login attempts */
    $this->session->set_userdata('uid', $user_id);
    $this->session->unset_userdata('failed_logins');
    $this->users_model->insert_session($user_id, gmdate('Y-m-d H:i:s', time()));
    return;
}

public function submit()
{
    if (!$this->form_is_valid())
    {
        $this->output('The form did not validate successfully!', 'Form Not Validated', 'Error');
        return;
    }

    /* Post values from login form */
    $post_username = $this->input->post('username');
    $post_password = $this->input->post('password');

    /* Test to see value of posted login form */
    //echo '<pre>';
    //var_dump($post_username);
    //var_dump($post_password);
    //echo '</pre>';
    //die();

    /* Get user data from post username value */
    $user_data = $this->users_model->get_by('username', $post_username);

    /* Test to see value of $user_data */
    //echo '<pre>';
    //var_dump($user_data);
    //echo '</pre>';
    //die();

    if (count($user_data) == 0)
    {
        /* User was not found in database */
        $this->output('The user was not found in the database!', 'User Not Found', 'Error');
        return;
    }

    /* User was found in database */

    if ($this->is_user_locked($user_data->lock_date, $user_data->user_id))
    {
        /* User is locked from logging in from too many failed attempts */
        $this->output('This user account is currently locked!', 'Account Locked', 'Error');
        return;
    }

    if ($user_data->user_status_id != 2)
    {
        /* User has a status that is not allowed to proceed */
        $this->user_status_message($user_data->user_status_id);
    }

    /* User is registered and validated */

    $regenerated_post_password = $this->genfunc->reGenPassHash($post_password, $user_data->password_hash);

    $failed_logins = $this->session->userdata('failed_logins');

    if ($regenerated_post_password !== $user_data->password)
    {
        /* Password from login from does not match user stored password */

        if ($failed_logins == 0)
        {
            /* First time user has not entered username and password successfully */
            $this->session->set_userdata('failed_logins', 1);
            $this->users_model->increase_login_attempt($this->input->ip_address(), $post_username, gmdate('Y-m-d H:i:s', time()));
            $this->output('Incorrect username and password credentials!', 'Incorrect Login Credentials', 'Error');
            return;
        }

        /* User has atleast one failed login attempt for the current session */

        if ($failed_logins !== 4)
        {
            /* User has a few more chances to get password right */
            $failed_logins++;
            $this->session->set_userdata('failed_logins', $failed_logins);
            $this->users_model->increase_login_attempt($this->input->ip_address(), $post_username, gmdate('Y-m-d H:i:s', time()));
            $this->output('Incorrect username and password credentials!', 'Incorrect Login Credentials', 'Error');
            return;
        }

        $this->users_model->lock_out_user($user_data->user_id, gmdate('Y-m-d H:i:s', time()+(60*15)));
        //$this->functions_model->send_email('maximum_failed_login_attempts_exceeded', $user_data->email_address, $user_data)
        $this->output('Your account is currently locked, we apologize for the inconvienence. You must wait 15 minutes before you can log in again! An email was sent to the owner of this account! Forgotten your username or password? <a href="forgotusername">Forgot Username</a> or <a href="forgotpassword">Forgot Password</a>', 'Account Locked', 'Error');
        return;
    }

    /* Password from login form matches user stored password and user may login */

    $this->start_user_session($user_data->user_id);
    $this->output('Successful login! Sending you to the dashboard!', 'Login Sucessful', 'Success');
    return;
}
}
\$\endgroup\$
1
\$\begingroup\$

First thing to do is to notice the code repeted in the different branches. Then your code becomes :

if ($regenerated_post_password !== $user_data->password)
{   
        /* User has atleast one failed login attempt for the current session */

        if ($failed_logins < 4)
        {
                /* User has a few more chances to get password right */
                $failed_logins++;
                $this->session->set_userdata('failed_logins', $failed_logins);
                $this->users_model->increase_login_attempt($this->input->ip_address(), $post_username, gmdate('Y-m-d H:i:s', time()));
                $this->output('Incorrect username and password credentials!', 'Incorrect Login Credentials', 'Error');
        }
        else
        {
                $this->users_model->lock_out_user($user_data->user_id, gmdate('Y-m-d H:i:s', time()+(60*15)));
                //$this->functions_model->send_email('maximum_failed_login_attempts_exceeded', $user_data->email_address, $user_data)
                $this->output('Your account is currently locked, we apologize for the inconvienence. You must wait 15 minutes before you can log in again! An email was sent to the owner of this account! Forgotten your username or password? <a href="forgotusername">Forgot Username</a> or <a href="forgotpassword">Forgot Password</a>', 'Account Locked', 'Error');
        }
}

(I took this chance to replace !=4 with <4 just in case we ever reach something after 4 for whatever reason)

Then you could put the content of the else-block and the content of the if-block in functions on their own but this doesn't seem really required to me.

\$\endgroup\$
  • \$\begingroup\$ Shouldn't you be returning inside the if and else areas. \$\endgroup\$ – Kevin Smith Jun 6 '13 at 20:45
  • 1
    \$\begingroup\$ Well, I'm missing some context here but if needed you can add one after the else block. If you could show us the whole piece of code this is part of, then I could try to advice. \$\endgroup\$ – SylvainD Jun 7 '13 at 8:54
  • \$\begingroup\$ I'll edit the post with the entire controller. \$\endgroup\$ – Kevin Smith Jun 7 '13 at 17:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.