Authentication using PDO

Question

I'm new to PDO, so I was wondering if you guys could check over my PDO code for efficiency (and if the PHP can be improved)

public function ValidateUser($username = '', $password = '')
{
    $prefix = "SELECT ";
    $type = 'COUNT(id)';
    $suffix = " FROM users WHERE username = :username LIMIT 1";
    if($grabUser = $this->db->prepare($prefix.$type.$suffix))
    { 
        $grabUser->bindParam(":username", $username, PDO::PARAM_STR);
        $grabUser->execute();
        if(count($grabUser->fetchColumn()) <= 0)
        {
            return array(0,0);
        }
    }
    else {
        return array(0,0);
    }

    $type = 'salt';
    if($grabUser = $this->db->prepare($prefix.$type.$suffix))
    { 
        $grabUser->bindParam(":username", $username, PDO::PARAM_STR);
        $grabUser->execute();
        $salt = $grabUser->fetch()['salt'];
        $password = $this->core->blueHash($password, $salt);
    }
    else {
        return array(0,0);
    }


    $stmt = "SELECT COUNT(id) FROM users WHERE username = :username AND password = :password LIMIT 1";
    if($checkFinal = $this->db->prepare($stmt))
    { 
        $checkFinal->bindParam(":username", $username, PDO::PARAM_STR);
        $checkFinal->bindParam(":password", $password, PDO::PARAM_STR);
        $checkFinal->execute();
        if(!count($checkFinal->fetchColumn()) > 0)
        {
            return array(0,0);
        }
        return array(count($checkFinal->fetchColumn()),$password);
    }
    return array(0,0);
}

This code checks that the supplied username and password are correct and if so, an array is returned.

EDIT: AJAX file where ValidateUser() is used:

<?php
require_once "../../_global.php";
$core->ajaxCheck();

header("Content-Type: application/json");
$json = array();

if(USER_ID <= 0 && !LOGGED_IN)
{
    // DO ISSET CHECK :((
    $username = $core->input($_POST['name']);
    $password = $core->input($_POST['pword']);

    $valid = $users->ValidateUser($username, $password);
    if($valid[0] == 0)
    {
        $json[0] = "error";
        $json[1] = "ajax";
        $json[2] = "These details are incorrect.";
    }
    elseif($valid[0] > 0)
    {
        $_SESSION['login']['username'] = $users->GrabUserVar($users->Name2id($username),"username");
        $_SESSION['login']['hash'] = $valid[1];
        $_SESSION['login']['social'] = false;
        $json[0] = "success";
    }
}
else {
    $json[0] = "error";
    $json[1] = "ajax";
    $json[2] = "You are already signed in.";
}
echo json_encode($json);
exit;
?>

EDIT2: Code that is run on each page to check whether the session username and password hash match that of the database:

session_start();
if(isset($_SESSION['login']['username']) && isset($_SESSION['login']['hash']))
{
    $userN = $_SESSION['login']['username'];
    $userH = $_SESSION['login']['hash'];
    if($users->CheckUser($userN, $userH) > 0)
    {
        define('LOGGED_IN',true);
        define('USER_ID',$users->Name2id($userN));
        $cUser = $users->createIndividual(USER_ID);
        define('USER_NAME',$cUser->getUsername());
        define('USER_HASH',$userH);
    }
    else {
        define('LOGGED_IN',false);
        define('USER_ID',0);
        define('USER_NAME','Guest');
        define('USER_HASH',0);
        session_destroy();
    }
}
else {
    define('LOGGED_IN',false);
    define('USER_ID',0);
    define('USER_NAME','Guest');
    define('USER_HASH',0);
}

Answer 1

Efficiency

You currently perform three queries:

• check if a user with that name exists
• get salt for that username
• check if user with that username and password exist

You can perform all these actions with one query:

SELECT salt, password FROM users WHERE username = :username LIMIT 1;

Then just hash the supplied password with the salt from the db, and compare the result to the password from the db. It might look something like this:

if (hash_equals($grabUser->fetch()['password'], $this->core->blueHash($password, $grabUser->fetch()['salt'])) {
    echo 'valid password';
}

Return Early

If you negate your if clauses, you can safe one level of nesting, which I think results in more readable code, because nesting decreases readability, and because it's clearer to what if clause the else clause belongs. Eg:

if(!($grabUser = $this->db->prepare($prefix.$type.$suffix))) 
{
    return array(0,0);
}

$grabUser->bindParam(":username", $username, PDO::PARAM_STR);
$grabUser->execute();
if(count($grabUser->fetchColumn()) <= 0)
{
    return array(0,0);
}

Misc

• it's unclear why you are returning an array. It seems to always contain [1, $userPassword]. Why do you need to return the password? And wouldn't it be easier to just return true or false instead of 1 or 0 inside an array? At least I would add a PHPDoc style comment explaining what is returned.
• I would extract some of the code to its own function to increase readability and avoid duplication. eg you could have fetchFromUser($columnName, $username).
• I don't know what password hashing you currently use, but I would think about switching to bcrypt. It's easier to use (you don't have to manage salts, etc), and is currently considered secure.