PHP 5 - Securing an admin area of a site

Question

I'm currently writing a couple of MVC sites using Kohana as my framework. Each has a simple admin area where the admin can upload and edit content. I'm currently storing the admin's user model in a session and checking whether or not they're an administrator with the following method:

private function checkAdmin()
{
    if (!isset($_SESSION['admin']) || $_SESSION['admin']->Level !== 'admin')
    {
        header('Location: /admin');
        exit;
    }
}

I call this method in all of the other Admin controller methods, like so:

public function writeReview()
{
    $this->checkAdmin();

    // rest of the method
}

I'm just wondering if there's anything I can do to improve this. It just seems like a single point of failure that's giving me a bad smell, so I'm a bit weary to move on before getting this nailed down.

Answer 1

If you want to let users share their logins fine, but otherwise generating a per session/login key and storing it in the DB will lock things down even further. This way, if someone logs in with your password, you'll get kicked out and instantly know that it's been compromised.

Other basic things to do - store dates of last login, IPs.. this kind of stuff. It's not just one single thing, but lots! :)

Answer 2

1. Your function appears to be redirecting to /admin only if the user is not an administrator. If that's the intended result, then fine.

2. -- Forget this, my mistake.

3. The checkAdmin() function, as it relies on a redirect, is only useful in situations where you want to redirect. If, for example, you are using this in the processing script (and you should be checking if it's an administrator in the processing script), you just want a return true or return false. I suggest that be the base function, and a redirect function call that, or alternative, accept and optional parameter to redirect.

Answer 3

If this is Kohana version 2.x, I would move the $this->checkAdmin(); into the constructor of your controller. If this is version 3.x, I would put it in the before() method. This will ensure that every route will be protected.