4
\$\begingroup\$

I have a login web page where a user enters their email and password and I need to check if:

  1. the entered email exists in the DB and
  2. the entered password matches the hashed one from the db.

The password in the DB was hashed with

$pw = password_hash($_POST["pw"], PASSWORD_BCRYPT);

and is stored in a VARCHAR(255) column. Email is set to primary and unique in the DB so there cannot be a duplicate record for this.

To achieve the above I came to the below which works as intended for me. However, since I am new to this and because of the double DB connection I was wondering if there is a way to shorten or otherwise improve this code. 

$email = $_POST["email"];
$pw = $_POST["pw"]; 

$stmt = $conn->prepare("SELECT email FROM Users WHERE email = ?");
$stmt->bind_param('s', $email);
$stmt->execute();
$result = $stmt->get_result();
if(mysqli_num_rows($result) == 0){
    echo "Email has not been registered yet";
}else{
    $stmt = $conn->prepare("SELECT pw FROM Users WHERE email = ? LIMIT 1");
    $stmt->bind_param('s', $email);
    $stmt->execute();
    $result = $stmt->get_result();
    $pwHashed = $result->fetch_assoc();
    if(password_verify($pw, $pwHashed["pw"])){
        echo "Password correct";
    }else{
        echo "Password incorrect";
    }   
}
\$\endgroup\$

1 Answer 1

Reset to default
3
\$\begingroup\$

You really don't need two queries to do this. You can just use your second query, and if it doesn't return any results, report that. Eg:

$stmt = $conn->prepare("SELECT pw FROM Users WHERE email = ? LIMIT 1");
$stmt->bind_param('s', $email);
$stmt->execute();
$result = $stmt->get_result();

if(mysqli_num_rows($result) == 0){
    echo "Email has not been registered yet";
}

$pwHashed = $result->fetch_assoc();
if(password_verify($pw, $pwHashed["pw"])){
    echo "Password correct";
}else{
    echo "Password incorrect";
}

Other than this, your code looks good. You use prepared statements, so it's secure, and your style is consistent and easy to read. I would probably write pw as password as it's more readable, but otherwise everything looks good.

\$\endgroup\$
3
  • \$\begingroup\$ Update: Just a thought on this: The above works great but I noticed that in this case it also returns "Password incorrect" in addition when the email is not registered which might be confusing so I will wrap the last 6 lines in an else statement to only check them if the email actually exists. Does that make sense ? \$\endgroup\$ Commented Jun 24, 2015 at 9:34
  • 1
    \$\begingroup\$ I'm not sure I'd mix OOP and procedural functions/methods and since it's possible for $stmt->get_result() to return false if something goes wrong, I'd change if(mysqli_num_rows($result) == 0) to if(!$result || !$result->num_rows). If that occurs either terminate the script, throw an Exception or put the password verification block (which requires $result to be a mysqli_result object) into an else block. \$\endgroup\$ Commented Jun 24, 2015 at 9:47
  • 1
    \$\begingroup\$ @keewee279 personally, I would put the whole thing in a function, and then return something (either the result string, or true/false for correct/not correct and throw exception for non-existing email, or have some sort of result object for the string, and then return false for email/pw incorrect and true for correct). And as CD001 said, it would be a good idea to handle errors (otherwise finding bugs will be hard) \$\endgroup\$ Commented Jun 24, 2015 at 9:54

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.