6
\$\begingroup\$

I implemented a login system, with session, using CodeIgniter. If the session doesn't exist, redirect to login page. Please review, and let me know what can be done to make it better.

view (login.php)

<body>
<?php echo form_open(base_url('verify'),['id' => 'loginForm', 'name' => 'loginForm', 'method' => 'post'])?>
    <div class="login wrap">
      <input type="text" name="email" id="email" placeholder="Email" pattern="^([a-zA-Z0-9_\-\.]+)@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.)|(([a-zA-Z0-9\-]+\.)+))([a-zA-Z]{2,4}|[0-9]{1,3})(\]?)$">

      <input type="password" name="password" id="password" placeholder="Password">

      <!-- <input type="button" value="Help!" /> -->
      <input type="submit" value="Log in">
    </div>
<?php echo form_close("\n")?>

verify.php (controller)

class Verify extends CI_Controller
{
public function __construct()
{
    parent::__construct();

    $this->load->helper(['form', 'url', 'security']);
    $this->load->library(['form_validation', 'encryption','session']);
    $this->load->model('User_Model');

}

public function index()
{
    $this->form_validation->set_rules('email','Email','required|trim|xss_clean');
    $this->form_validation->set_rules('password','Password','required|trim|xss_clean|callback_check_password');

    if ($this->form_validation->run() === FALSE) {
        $this->load->view('admin/login');

    } else {
        redirect(base_url('dashboard/dash'));

    }
}

/*login process*/

public function check_password($password)
{
    $email = $this->input->input_stream('email', TRUE);
    $password = hash('sha256', $password);

    $result = $this->User_Model->login($email,$password);
    if($result != null){
        foreach ($result as $row) {
            $sess_data = [
                'username' => $row->username,
                'user_id' => $row->id
            ];
            $this->session->set_userdata('logged_in', $sess_data);
        }
        return TRUE;
    } else {
        $this->form_validation->set_message('check_database', 'invalid username and password');
        return FALSE;
    }
}


public function logout($page = 'login')
{
    $this->session->unset_userdata('logged_in');
    session_destroy();
    redirect(base_url('login'),'refresh');
}
}

Dashboard.php (dashboard controller)

class Dashboard extends Check_Logged
{
public function __construct()
{
    parent::__construct();

    $this->load->helper(['form', 'url']);
    $this->load->library(['form_validation', 'encryption']);

}

public function index()
{
    if ($this->logged === TRUE) {
        redirect('dashboard/dash');
    } else {
        $this->load->view('admin/login');
    }
}

public function login($page = 'login')
{
        if ($this->logged === TRUE) {
            redirect('dashboard/dash');
        } else {
            $this->load->view('admin/'.$page);
        }
        $this->load->view('admin/'.$page);

}

public function dash($page = 'dashboard')
{
    if ($this->logged === TRUE) {
        $this->load->view('admin/'.$page);
    } else {
        redirect('login');
    }
}

public function logout($page = 'login')
{
   $this->session->unset_userdata('logged_in');
    session_destroy();
    redirect(base_url('login'),'refresh');
}

public function portfolio($page = 'portfolio')
{
    if ($this->logged === TRUE) {
        $this->load->view('admin/'.$page);
    } else {
        redirect('login');
    }
}

public function team($page = 'team')
{
    if ($this->logged === TRUE) {
        $this->load->view('admin/'.$page);
    } else {
        redirect('login');
    }
}

}

and check_Logged.php (controller)

class Check_Logged extends CI_Controller
{
    public $logged = '';
    public function __construct()
    {
        parent::__construct();

        $this->load->helper(['form', 'url', 'security']);
        $this->load->library(['session', 'form_validation']);

        $this->logged = $this->session_check();
    }


    /*
     * Check the Session status
     * if session exist return true
     * else return false
     * */

    public function session_check()
    {
        if($this->session->userdata('logged_in') != null)
            return TRUE;
        else
            return FALSE;
    }
}
\$\endgroup\$

2 Answers 2

Reset to default
3
\$\begingroup\$
  • Validating emails is hard, and it's not a good idea to do it on your own with a regex. Here are a couple of valid email addresses you would not allow (there are a lot more): "foo"@bar.com, [email protected], !#$%&'*+-/=?^_{|}[email protected]. And even though it's so strict, it still lets through invalid addresses such as [email protected]] (because the regex is actually broken).
  • Use type="email" and let the browser handle the validation for you.
  • Don't use sha256, it's too fast. Use bcrypt or similar instead.
  • I wouldn't call xss_clean on user input. XSS should be prevented when printing data, not when receiving it. This is especially important for passwords, as xss_clean will weaken some passwords - possibly considerably.
  • Your controllers aren't named all that well. What does verify verify? and does it only verify? Check_Logged could also be better named. Maybe BaseAdminController or something.
  • all your methods are public, even though quite a lot of them could be private, making your classes more complicated than they need to be.
  • Having to check if the user is logged in in each of the admin controller methods seems annoying, and could easily be forgotten. I would handle that case in the base controller Check_Logged, and then show the login page.
  • You handle the not-logged-in case differently in different methods, without any apparent reason. Sometimes it's $this->load->view('admin/login');, sometimes redirect('login');, or redirect(base_url('login'),'refresh');. If there is a reason for the difference, I would comment on it. Otherwise, handle the same thing the same way to avoid confusion.
\$\endgroup\$
2
\$\begingroup\$

  1. It seems weird that in check_password the login would return multiple rows. I would expect only one user to be associated with a particular emails/password login.

  2. In your controller you have duplicated this code three times:

    if ($this->logged === TRUE) {
    $this->load->view('admin/'.$page);
} else {
    redirect('login');
}

    This should be extracted into a single common function which is called by each of the controller actions.

  3. This seems weird:

    if ($this->logged === TRUE) {
    redirect('dashboard/dash');
} else {
    $this->load->view('admin/'.$page);
}
$this->load->view('admin/'.$page);

    So if the user is not logged in you load the view and then you load the view again unconditionally?

  4. I'm not PHP expert but I'm pretty sure session_check can be condensed into

    return $this->session->userdata('logged_in') != null;

  5. logged is not a very good name for the property which seems to indicate whether or not the user is logged in. is_logged_in seems to be more expressive.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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