Login Controller

[Xtremer360]

Trying to see what improvements I can make to this code. More specifically the login function.

 

http://pastebin.com/T2AgFQHE

[Christian F.]

While I'm not too familiar with how CI works, I did notice the following bits:

[code=php:0]$this->form_validation->set_rules('username', 'Username', 'trim|required|xss_clean|strtolower');
$this->form_validation->set_rules('password', 'Password', 'trim|required|xss_clean');

If I'm reading this correctly, you're making the username lowercase, and trimming + checking for XSS (JavaScript, or more?) in both the username and password. Not only is manipulating the username bad, but manipulating and putting artificial limits on the password is even worse. The password is being hashed, and should never be printed out anywhere in the code. So any limits you put on it has no other effect than to reduce entropy, and thus weaken your users security.

 

Then we have the multi-level nesting with late exiting, like this:

if ($regen_post_password == $user_data->password) 
{
$profile_data = $this->users_model->get_profile_data ($user_data->user_id);
if ($profile_data) 
{
	$this->ci->session->set_userdata (array ('xtr' => 'yes', 'user_id' => $user_data->user_id, 'username' => $user_data->username, 'role' => $user_data->user_roles_id, 'default_roster_id' => $profile_data->default_roster_id));
	$attempts_cleared = $this->users_model->clear_login_attempts ($this->ci->input->ip_addess, $post_username);
	if ($attempts_cleared) 
	{
		$session_inserted = $this->users_model->insert_session ($this->ci->session->userdata ('session_id'), $this->ci->session->userdata ('user_id'), $this->ci->input->ip_address (), $this->ci->session->userdata ('user_agent'));
		if ($session_inserted) 
		{
			return TRUE;
		}
	} 
	else 
	{
		return NULL;
	}
} 
else 
{
	if (!$this->is_max_login_attempts_exceeded ($post_username)) 
	{
		if ($this->ci->users_model->increase_login_attempt ($this->ci->input->ip_address (), $post_username)) 
		{
			return NULL;
		}
	} 
	else 
	{
		return FALSE;
	}
}
}

Not only are the else blocks unnecessary when using return (or any other function that halts execution of the function/script), but the multi-level nesting and late exiting makes the script harder to read than it should be. I strongly recommend using early exiting instead:

function ....  {
$regen_post_password = $this->genfunc->reGenPassHash( $post_password, $user_data->password_hash );

if ($regen_post_password != $user_data->password) {
	return FALSE;
}

$profile_data = $this->users_model->get_profile_data ($user_data->user_id);

if ($profile_data) {
	$this->ci->session->set_userdata (array ('xtr' => 'yes', 'user_id' => $user_data->user_id, 'username' => $user_data->username, 'role' => $user_data->user_roles_id, 'default_roster_id' => $profile_data->default_roster_id));

	$attempts_cleared = $this->users_model->clear_login_attempts ($this->ci->input->ip_addess, $post_username);
	if (!$attempts_cleared) {
		return NULL;
	}

	$session_inserted = $this->users_model->insert_session ($this->ci->session->userdata ('session_id'), $this->ci->session->userdata ('user_id'), $this->ci->input->ip_address (), $this->ci->session->userdata ('user_agent'));
	if ($session_inserted) {
		return TRUE;
	}
} else {
	if ($this->is_max_login_attempts_exceeded ($post_username)) {
		return FALSE;
	}

	if ($this->ci->users_model->increase_login_attempt ($this->ci->input->ip_address (), $post_username)) {
		return NULL;
	}
}

return FALSE;
}