5
\$\begingroup\$

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.)

\$\endgroup\$

migrated from stackoverflow.com Aug 15 '11 at 12:43

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ Have you tried using oAuth? \$\endgroup\$ – rickyduck Aug 15 '11 at 9:54
  • \$\begingroup\$ Seems perfectly valid to me. Is there a real question? \$\endgroup\$ – user410932 Aug 15 '11 at 9:57
  • 1
    \$\begingroup\$ Tip: Don't use exceptions to signal a standard case (auth failed), better have a auth state you can ask for (authenticated, unauthenticated). \$\endgroup\$ – hakre Aug 15 '11 at 10:06
  • \$\begingroup\$ @hakre: I'm afraid some page forgets to check the auth state … thus the exception, that way it is garuanteed to abort the script \$\endgroup\$ – knittl Aug 15 '11 at 10:11
  • \$\begingroup\$ @nkittl: If that's the case, you should implement authentication on another layer probably. Just saying, you can use exceptions to gain something like a signal, however, keep in mind that it's not the right tool for the job. Might give you more problems than solutions in the end. Better check why some scripts are not checking authentication and think about twice. \$\endgroup\$ – hakre Aug 15 '11 at 10:17
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Crypt::hash uses a randomly generated user-unique salt (let's assume the usage of crypt). Wrapper class for session seems overhead to me, but was actually in use. 4) good point. 5) if the server was compromised then I've lost already, imo. the redirect call should probably be wrapped in a refresh() \$\endgroup\$ – knittl Aug 15 '11 at 10:17
  • \$\begingroup\$ But how Crypt::hash knows the specific salt for the user in question? It can generate random salts during registration, but what about log-in time, i.e. checking that salt? \$\endgroup\$ – battal Aug 15 '11 at 10:36
  • \$\begingroup\$ it's stored as part of the password. crypt does the same thing \$\endgroup\$ – knittl Aug 15 '11 at 10:40
  • \$\begingroup\$ Ok, sorry for asking about something obvious :) \$\endgroup\$ – Halil Özgür Aug 16 '11 at 6:32
2
\$\begingroup\$

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy