Jump to content

Login Controller


Xtremer360

Recommended Posts

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;
}

Link to comment
https://forums.phpfreaks.com/topic/267205-login-controller/#findComment-1370105
Share on other sites

Archived

This topic is now archived and is closed to further replies.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.