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\$
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\$
  • \$\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\$ – keewee279 Jun 24 '15 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\$ – CD001 Jun 24 '15 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\$ – tim Jun 24 '15 at 9:54

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.