User Authentication PHP script

Question

few days ago i made small PHP Authentication API with all basic functionalities (log in, log out, registration, getting authenticated member) and i got suggestions that i should use prepared statements for better performance and SQL Injection defense, so i made some changes to my script and it looks like this, can anyone confirm if it is good, did i miss something and is there anything else that i can improve here.

<?php require_once '../dbConnect.php'; session_start(); $object = json_decode(file_get_contents("php://input"), true); if (isset($object['email']) && isset($object['password'])) { $email = $object['email']; $password = $object['password']; $stmt = $mysqli->prepare("select id, password from members where email = ?"); $stmt->bind_param("s", $email); $stmt->execute(); $stmt->bind_result($id, $password); $stmt->fetch(); if($id) { if (password_verify($object['password'], $password)) { $message = array('message' => 'Authentication Successful!'); $_SESSION["id"] = $id; echo json_encode($message); } else { $message = array('message' => 'Wrong Credentials, Authentication failed!'); session_destroy(); http_response_code(400); echo json_encode($message); } } else { session_destroy(); http_response_code(406); } $mysqli->close(); } else { session_destroy(); http_response_code(400); } ?> 

Answer 1

SQL Injection defense, so i made some changes to my script and it looks like this, can anyone confirm if it is good

Yes, you are using prepared statements correctly. You didn't post all your updated code, so just remember to always use prepared statements and to never directly put any variables in the query.

Returning early

Your code is quite nested, which makes it difficult to read. If I want to know what happens in the else cases, I have to try to find the matching else, which can be difficult.

If you return early / introduce guard clauses, you can avoid this:

if (!isset($object['email']) || !isset($object['password'])) { session_destroy(); http_response_code(400); return; } $email = $object['email']; $password = $object['password']; $stmt = $mysqli->prepare("select id, password from members where email = ?"); if(!$stmt) { session_destroy(); http_response_code(400); return; } $stmt->bind_param("s", $email); $stmt->execute(); $stmt->bind_result($id, $password); $stmt->fetch(); if(!$id) { session_destroy(); http_response_code(406); } if (password_verify($object['password'], $password)) { $message = array('message' => 'Authentication Successful!'); $_SESSION["id"] = $id; echo json_encode($message); } else { $message = array('message' => 'Wrong Credentials, Authentication failed!'); session_destroy(); http_response_code(400); echo json_encode($message); } $mysqli->close(); 

Structure

Now, you sill have some duplication (session destroy and response code setting) which you may want to remove, for example by returning error codes or success messages from the function and only setting it once from the calling code.

Your code also does a bit too much for my taste (db interaction, printing, retrieving user input, session management, etc). You could introduce functions such as getUserByEmail, etc to structure your code.

Misc

• 4xx are client side errors, but a non-working prepare is actually a server error; you should return a 500.
• 406 has to do with content encoding, so it is definitely the wrong code to send when a user is not found. It is also generally good practice to handle wrong username and wrong password the same way as to not leak information.
• Upper-case all your SQL keywords to make queries easier to read.