Password hashing and database

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
5
down vote

favorite












I would like to ask someone if the code i will put here is Secured for SQL injection and my password hashing and checking while user is logging in is secure and done well.



My Database connection :



$hostname='localhost';
$username='root';
$password='';

try
$dbh = new PDO("mysql:host=$hostname;dbname=myshop",$username,$password);
$dbh->query ('SET NAMES utf8');
$dbh->query ('SET CHARACTER_SET utf8_unicode_ci');
$dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); // <== add this line
catch (PDOException $e)
echo $e->getMessage();



My registration script :



include_once 'config.php';

$data = file_get_contents("php://input");
$data = json_decode($data, true);

try
$stmt = $dbh->prepare("INSERT INTO `accounts` (`account_id`, `firstname`, `lastname`, `username`, `password`, `email`, `city`, `postalcode`, `adress`, `country`, `role`)
VALUES (NULL, :firstname, :lastname, :username, :password, :email, :city, :postalcode, :adress, :country, 0) ");
$stmt->bindParam(':firstname', $data['firstname']);
$stmt->bindParam(':lastname', $data['lastname']);
$stmt->bindParam(':username', $data['username']);
$stmt->bindParam(':password', password_hash($data['password'], PASSWORD_DEFAULT));
$stmt->bindParam(':email', $data['email']);
$stmt->bindParam(':city', $data['city']);
$stmt->bindParam(':postalcode', $data['postalcode']);
$stmt->bindParam(':adress', $data['adress']);
$stmt->bindParam(':country', $data['country']);
$stmt->execute();
catch (PDOException $e)
if (strpos($e->getMessage(), "for key 'login'") !== false)
echo 'Duplicate login entry';
exit;
if (strpos($e->getMessage(), "for key 'email'") !== false)
echo 'Duplicate e-mail entry';
exit;

else
throw $e;


echo 'Success';


My login script :



include_once 'config.php';

$data = file_get_contents("php://input");
$data = json_decode($data, true);

try
$stmt = $dbh->prepare("SELECT * from accounts WHERE
username=:username");
$stmt->bindParam(':username', $data['username']);
$stmt->execute();
catch (PDOException $e)
throw $e;


$rows = $stmt->rowCount();
$result = $stmt->fetchAll(PDO::FETCH_ASSOC);
$json = json_encode($result);

if ($rows === 1)
if (password_verify($data['password'], $result[0]['password']))
echo $json;
exit;

else
echo 'Wrong password';
exit;

if ($rows === 0)
echo 'No account';
exit;

else  












up vote
5
down vote

favorite












I would like to ask someone if the code i will put here is Secured for SQL injection and my password hashing and checking while user is logging in is secure and done well.



My Database connection :



$hostname='localhost';
$username='root';
$password='';

try
$dbh = new PDO("mysql:host=$hostname;dbname=myshop",$username,$password);
$dbh->query ('SET NAMES utf8');
$dbh->query ('SET CHARACTER_SET utf8_unicode_ci');
$dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); // <== add this line
catch (PDOException $e)
echo $e->getMessage();



My registration script :



include_once 'config.php';

$data = file_get_contents("php://input");
$data = json_decode($data, true);

try
$stmt = $dbh->prepare("INSERT INTO `accounts` (`account_id`, `firstname`, `lastname`, `username`, `password`, `email`, `city`, `postalcode`, `adress`, `country`, `role`)
VALUES (NULL, :firstname, :lastname, :username, :password, :email, :city, :postalcode, :adress, :country, 0) ");
$stmt->bindParam(':firstname', $data['firstname']);
$stmt->bindParam(':lastname', $data['lastname']);
$stmt->bindParam(':username', $data['username']);
$stmt->bindParam(':password', password_hash($data['password'], PASSWORD_DEFAULT));
$stmt->bindParam(':email', $data['email']);
$stmt->bindParam(':city', $data['city']);
$stmt->bindParam(':postalcode', $data['postalcode']);
$stmt->bindParam(':adress', $data['adress']);
$stmt->bindParam(':country', $data['country']);
$stmt->execute();
catch (PDOException $e)
if (strpos($e->getMessage(), "for key 'login'") !== false)
echo 'Duplicate login entry';
exit;
if (strpos($e->getMessage(), "for key 'email'") !== false)
echo 'Duplicate e-mail entry';
exit;

else
throw $e;


echo 'Success';


My login script :



include_once 'config.php';

$data = file_get_contents("php://input");
$data = json_decode($data, true);

try
$stmt = $dbh->prepare("SELECT * from accounts WHERE
username=:username");
$stmt->bindParam(':username', $data['username']);
$stmt->execute();
catch (PDOException $e)
throw $e;


$rows = $stmt->rowCount();
$result = $stmt->fetchAll(PDO::FETCH_ASSOC);
$json = json_encode($result);

if ($rows === 1)
if (password_verify($data['password'], $result[0]['password']))
echo $json;
exit;

else
echo 'Wrong password';
exit;

if ($rows === 0)
echo 'No account';
exit;

else
echo 'Something went wrong';
exit;






share catch (PDOException $e)
throw $e;



I fancy you just copy-pasted this part from the insert example. But if you think on it, it will become clear that this code will behave exactly the same without try, catch and throw operators! So they should be simple taken out.



$rows = $stmt->rowCount();


is essentially useless, as you have the fetch result that can be used instead.



$result = $stmt->fetchAll(PDO::FETCH_ASSOC);


fetchAll() doesn't seem to be a wise choice, as it adds unnecessary dimension to the resulting array. fetch() should be used instead.






share catch (PDOException $e)
throw $e;



I fancy you just copy-pasted this part from the insert example. But if you think on it, it will become clear that this code will behave exactly the same without try, catch and throw operators! So they should be simple taken out.



$rows = $stmt->rowCount();


is essentially useless, as you have the fetch result that can be used instead.



$result = $stmt->fetchAll(PDO::FETCH_ASSOC);


fetchAll() doesn't seem to be a wise choice, as it adds unnecessary dimension to the resulting array. fetch() should be used instead.






share|improve this answer















Your code is secure against SQL injection but it's largely inefficient. And this army of bindParams's takes the largest part in it. All you need is just to send an array you already have directly into execute():



$stmt->execute($data);


Registration



On the other hand, your use of try and catch operator for insert is proper, which is a rare sight. The only objection is error messages that shouldn't be bluntly echoed out. Better to collect them and show nicely along with a form. So I would make the whole registration code as



$error = '';
$data['password'] = password_hash($data['password'], PASSWORD_DEFAULT);
try
$stmt = $dbh->prepare("INSERT INTO `accounts` (`account_id`, `firstname`, `lastname`, `username`, `password`, `email`, `city`, `postalcode`, `adress`, `country`, `role`)
VALUES (NULL, :firstname, :lastname, :username, :password, :email, :city, :postalcode, :adress, :country, 0) ");
$stmt->execute($data);
catch (PDOException $e)
if (strpos($e->getMessage(), "for key 'login'"))
$error = 'Duplicate login entry';
elseif (strpos($e->getMessage(), "for key 'email'"))
$error = 'Duplicate e-mail entry';
else
throw $e;


if ($error)
echo $error;
else
header("Location: ...");
exit;



Login



The login code could be improved as well. I've got a ready made example, Authenticating a user using PDO and password_verify(), you may check it out for the actual code. Here I will just comment on some parts



} catch (PDOException $e) 
throw $e;



I fancy you just copy-pasted this part from the insert example. But if you think on it, it will become clear that this code will behave exactly the same without try, catch and throw operators! So they should be simple taken out.



$rows = $stmt->rowCount();


is essentially useless, as you have the fetch result that can be used instead.



$result = $stmt->fetchAll(PDO::FETCH_ASSOC);


fetchAll() doesn't seem to be a wise choice, as it adds unnecessary dimension to the resulting array. fetch() should be used instead.







share|improve this answer















share|improve this answer



share|improve this answer








edited Apr 2 at 10:14









t3chb0t

32.1k54195




32.1k54195











answered Apr 2 at 9:51









Your Common Sense

2,415524




2,415524











  • Thank you for your review of my code and a very good response. I will try to workout my code with all your tips and give u feedback about how it went ! :)
    – SupremeDEV
    Apr 3 at 12:59
















  • Thank you for your review of my code and a very good response. I will try to workout my code with all your tips and give u feedback about how it went ! :)
    – SupremeDEV
    Apr 3 at 12:59















Thank you for your review of my code and a very good response. I will try to workout my code with all your tips and give u feedback about how it went ! :)
– SupremeDEV
Apr 3 at 12:59




Thank you for your review of my code and a very good response. I will try to workout my code with all your tips and give u feedback about how it went ! :)
– SupremeDEV
Apr 3 at 12:59












 

draft saved


draft discarded


























 


draft saved


draft discarded














StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f190303%2fpassword-hashing-and-database%23new-answer', 'question_page');

);

Post as a guest













































































Popular posts from this blog

Greedy Best First Search implementation in Rust

Function to Return a JSON Like Objects Using VBA Collections and Arrays

C++11 CLH Lock Implementation