User auth class and security

Question

Just curious if my authentication is secure enough, really. Also wondering if I should ditch the secret variable and just use password everywhere instead.

Note: the method logic must remain seperated because the same User object will have CRUD methods added at a later date.

The table structure:

CREATE TABLE IF NOT EXISTS `users` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `username` varchar(255) NOT NULL,
  `password` varchar(255) NOT NULL,
  `secret` varchar(255) DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB  DEFAULT CHARSET=latin1 AUTO_INCREMENT=2 ;

The class:

class User {
    public $db;
    public $id;
    public $username;
    public $password;
    public $secret;
    public $signed_in = false;

    function __construct ($db) {
        $this->db = $db;
    }

    function redirect ($url) {
        header('Location: '.$url);
        die();
    }

    function exists () {
        $sql = "select * from `users` where `username`=:username";
        $stm = $this->db->prepare($sql);
        $stm->bindParam(':username', $_POST['username']);
        $stm->execute();
        $res = $stm->fetch();

        if (!empty($res['password'])) {
            if (password_verify($_POST['password'], $res['password'])) {
                $this->id = $res['id'];
                $this->username = $res['username'];
                return true;
            }
        }

        return false;
    }

    function signIn () {
        $this->secret = password_hash($_POST['password'], PASSWORD_DEFAULT);

        if ($_POST['remember']) {
            setcookie('username', $this->username, time() + 31556926, '/');
            setcookie('secret', $this->secret, time() + 31556926, '/');
        }       
        $_SESSION['username'] = $this->username;
        $_SESSION['secret'] = $this->secret;

        $sql = "update `users` set `secret`=:secret where `id`=:id";
        $stm = $this->db->prepare($sql);
        $stm->bindParam(':secret', $this->secret);
        $stm->bindParam(':id', $this->id);
        $stm->execute();
    }

    function signedIn () {
        if (!empty($_COOKIE['username'])) {
            $this->username = $_COOKIE['username'];
            $this->secret = $_COOKIE['secret'];
        }
        else if (!empty($_SESSION['username'])) {
            $this->username = $_SESSION['username'];
            $this->secret = $_SESSION['secret'];
        }

        $sql = "select * from `users` where `username`=:username and `secret`=:secret";
        $stm = $this->db->prepare($sql);
        $stm->bindParam(':username', $this->username);
        $stm->bindParam(':secret', $this->secret);
        $stm->execute();
        $res = $stm->fetch();

        if ($res) {
            array_walk_recursive($res, 'sanitize');
            foreach ($res as $k => $v) {
                if (property_exists($this, $k)) {
                    $this->$k = $v;
                }
            }
            $this->signed_in = true;
        }
    }
}

How the class is used at the top of all pages in the app:

$user = new User($db); // create user object, $db is a PDO object
$user->signedIn(); // determine if they're signed in, populate $user->signed_in boolean

Signing the user in via a form:

if (isset($_POST['username'])) {
    if ($user->exists()) {
        $user->signIn();
        $user->redirect('dashboard.php');
    }
}

Any feedback is greatly appreciated. Like I said my primary concern here is security.

Answer 1

1. Don't store passwords on the client side. If your database leak, no one can stop the attacker from sending directly the hash instead of the password. In this case the attacker don't need to know the password at all.
2. You really want a method redirect in your User class? Think about SRP.
3. Your User class is half Model and half DAO. And you don't want that. A model full of query SQL isn't a good model. A data access object handles this separation between SQL and Model.

Answer 2

Structure

I think your interface is slightly confusing. Classes should be reusable without actually looking at the code; the method signatures and comments should be self-explanatory.

Without actually looking at the code, here is what a user of your class would see:

function redirect ($url);
function exists ();
function signIn ();
function signedIn ();

Intuitively, this is how I would use this:

// ignore exists(), I don't want to check if a user exists right now
$user->signIn(); // who am I signing in? Where does the user input come from?
[...]
if ($user->signedIn()) {
// do stuff
}

But that's not correct. As there are no comments in your code, I need to actually look at the code itself.

Another problem with usability is that your methods do not accept parameters, but expect global variables to be set. But I have no idea what values have to be set, which again means that I have to look at your code to use the class. It would be better to pass these values to the methods, eg exists($user, $pass)

Another problem is naming: Your exists method doesn't just check if a user exists, it actually validates login credentials. So it should be called something like validate($user, $pass). Also, methods that check if something is true generally start with is, so signedIn should be isSignedIn.

Also, the user of your class shouldn't need to call exists explicitly. Why isn't signIn doing it for me? signIn doesn't seem to be usable without exists, and exists doesn't seem to provide that many benefits on it's own. I would probably make it a private method.

Misc

• @Federico is right, redirect is a very generic method, it should not be in such a specific class. Otherwise, you will have a redirect method in a lot of different places, which means that if you want to change it, you would need to change it in multiple places.
• I also think that @Federico is partly right about the password hash. secret is the same value that is checked against password via password_verify when logging a user in. It just uses a different salt, which is why the result is different; still this doesn't seem like a good idea. If the cookie ever leaks, an attacker may bruteforce the hash and thus gain the users password; just generate a random value for this.
• make your cookies httpOnly
• SQL keywords should be all uppercase to increase readability.
• don't select *, select the specific columns you need instead to increase readability (and performance, slightly).
• like @Federico, I would use an approach that is a bit more MVC-like; I think a cleaner structure increases testability, reusability and readability.