Writing a proper (and simple) auth/login class in PHP

Question

How to write and design a simple (but still proper and secure) login class for PHP?

Currently I'm checking whether there is a login request (user entered data into login form) or the session is already existent and contains $_SESSION['authenticated'] == TRUE;. If both checks failed the user is not (properly) logged in. In code:

class Auth {
    private function __construct() {
        // new login
        if(isset($_POST['login'], $_POST['username'], $_POST['password']) && validCSRFToken()) {
            $user = $_POST['username'];
            $password = $_POST['password'];

            if(($row = querySingle('
               SELECT `id`, `password`
               FROM `users` u
               WHERE `nick` = "?"',
               $user)))
               // Crypt::hash checks the salted password
               && Crypt::hash($password, $row['password'])) {
                // credential change (guest -> user) regenerate session id
                session_regenerate_id(TRUE);

                $_SESSION['authenticated'] = TRUE;
                $_SESSION['userid'] = $row['id'];
                $_SESSION['user'] = $user;

                // prevent 'your browser has to re-send some data to display this page'
                redirect($_SERVER['REQUEST_URI']);
            } else {
                $this->logout();
                throw new AuthException('login failed. wrong user/password');
            }
        } else if($_SESSION['authenticated'] && $_SESSION['userid'] > 0 && !empty($_SESSION['user'])) {
            // user already logged in, nothing to see here, move on
        } else {
                    // not logged in!
                    throw new AuthException('not logged in. please login');
        }
    }

    public function logout() {
        $this->authenticated = FALSE;
        $this->user = '';
        $this->userid = 0;

        $this->session->destroy();
        $this->session->regenerate_id(TRUE);
    }
}

An object of the Auth class has to be created on every page that requires authentication from users:

try {
  $auth = new Auth();

  /*
   * rest of the page
   */

} catch(AuthException $ex) {
  // something went wrong during login
  displayError();
  displayLoginForm();
  exit; // halt further execution of script
}

Is this the right way to do it? Do you see anything (seriously) flawed with this approach? Can users impersonate other users? Users getting logged out randomly? Users getting logged out by malicious attackers? (although CSRF checks are in place). I'm afraid, I might be missing something obvious.

Any improvements apreciated, thanks!

(Please don't suggest using a PHP framework like CodeIgniter, Zend Framework, Kohana, etc.)

Answer 1

At the first look, no big hole shines through. Some points that can be considered, on the order of appearance:

1. Instead of directly using the $_POST, you can take these as parameters, thus making it look like more of an API. Example usage: when you want to directly log in people who have just registered. You will not be tied to some particular $_POST parameters.

2. Crypt::hash ...: Not seeing the interior of this, one can't comment much but from the looks, the salt seems constant, unless you are querying the users table again inside this. A different (and preferably with a length matching the password) salt for each user is generally the way to go in security-conscious developer environments.

3. Similar to the #1, you can use a wrapper class for $_SESSION usage.

4. else if($_SESSION['authenticated'] && $_SESSION['userid'] > 0 && !empty($_SESSION['user'])): Instead of directly querying the $_SESSION, you can make another function like $this->isLoggedIn(), so you'd be more resistant to future session and log in changes (along with some other benefits).

5. redirect($_SERVER['REQUEST_URI']); Relying on a $_SERVER variable always looked insecure and unstable to me. You can use some checks here or use some of your internal navigation variables instead, in case you don't want to make it completely generic in this way.

Answer 2

I'd also add validation of 'login', 'username' and 'password' before running the query.