views:

220

answers:

3

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.

+3  A: 
  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.

Zurahn
The way I have it now, all of the admin functionality is in its own controller. It's index() is simply a login form. Successful login sets the session variable. If the login fails, or if someone tries accessing any of the controller's public methods, I simply want to redirect them away. I think changing the destination to the site's home controller would be better. I'm just trying to wall off the admin section as much as possible.
kevinmajor1
[`die() - This language construct is equivalent to exit().`](http://php.net/manual/en/function.die.php)
deceze
+2  A: 

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.

slacker
+1  A: 

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! :)

danp