Security of login and register system

Question

I need some help with my register and log in functions. I'm not sure if I have understood bcrypt correctly either. I'm doubting the security of it, what do you think?

Is this good, or are there any changes I should make? Have I missed anything?

For the database table users, I created (ID, username and password[char:60]):

function loginAccount() {
 global $connect;
 $username = mysqli_real_escape_string($connect, $_POST['username']);
 $password = mysqli_real_escape_string($connect, $_POST['password']);
 $existingHashFromDb = "";
 if(!isset($existingHashFromDb)) {
 // existing password from database is not set
 } else {
 $login_sql = "SELECT password FROM users WHERE username = '".$username."'";
 $login_result = mysqli_query($connect, $login_sql);
 if($login_result) {
 while($row = mysqli_fetch_assoc($login_result)) {
 $existingHashFromDb = $row['password'];
 }
 # Check if the hash of the entered login password, matches the stored hash.
 # The salt and the cost factor will be extracted from $existingHashFromDb.
 $isPasswordCorrect = password_verify($password, $existingHashFromDb);
 if($isPasswordCorrect) {
 // you are logged in
 echo "<p>Welcome <a href=\"#{$username}\">" . ucfirst($username) . "</a></p>";
 // logout?
 echo "<p><a href=\"?logout\">Logout</a></p>";
 } else {
 echo "<p>Password did not match.</p>";
 }
 } else {
 // query failed
 mysqli_free_result($login_result);
 mysqli_close($connect);
 }
 }
}
function registerAccount() {
 global $connect;
 $username = mysqli_real_escape_string($connect, $_POST['username']);
 $password = mysqli_real_escape_string($connect, $_POST['password']);
 $password_again = mysqli_real_escape_string($connect, $_POST['password_again']);
 # This way you can define a cost factor (by default 10). Increasing the
 # cost factor by 1, doubles the needed time to calculate the hash value.
 $hashToStoreInDb = password_hash($password, PASSWORD_BCRYPT, array("cost" => 11));
 # See if the username already exists in the database
 $userexist_query = "SELECT username FROM users WHERE username = '".$username."'";
 $userexist_result = mysqli_query($connect, $userexist_query);
 if($userexist_result) {
 if(mysqli_num_rows($userexist_result) >= 1) {
 echo "<p>Username already exists!</p>";
 } else {
 # See if the password and password_again is matching.
 if($password == $password_again) {
 $register_query = "INSERT INTO users (username, password) VALUES ('$username', '$hashToStoreInDb')";
 $register_result = mysqli_query($connect, $register_query);
 if($register_result) {
 echo "<p><a href=\"#{$username}\">" . ucfirst($username) . "</a> has been created! <br> Password: <b>{$hashToStoreInDb}</b></p>";
 } else {
 // query failed
 mysqli_free_result($register_result);
 mysqli_close($connect);
 }
 } else {
 echo "<p>Password does not match!</p>";
 }
 }
 } else {
 // query failed
 mysqli_free_result($userexist_result);
 mysqli_close($connect);
 }
}

• Kid Diamond: Globals are not really a good sign.
• Dave Jarvis: Use PDO instead of direct mysql calls.
• Emil: il check out PDO sure, but beside that, did i do the bcrypt correct? and why is not globals a good sign?, i simply have an init.php file that includes db.php with the mysql connect variable $connect only, how is that a security flaw?
• Alex L: Here on Code Review, this type of question (PHP user login/register) is very common. Have you taken a look at previously answered questions which relate to yours? I guarantee you will learn at least one thing.
• Michael Zedeler: You should absolutely not use patterns like "SELECT password FROM users WHERE username = '".$username."'";. Use bound parameters in stead. (And don't try escaping anything.) php.net/manual/en/mysqli-stmt.bind-param.php

Answer 1

As others have mentioned, use the PDO library for database work and there is no reason to use gobals. Another thing I would highly recommend, is to use exceptions, they're amazing. I'm glad to see you using the password_* library though.

I have wrote a simple authentication class to get you started, and should by no means be used in a production environment.

class Auth
{
 private $dbh;
 private $config;
 private $user;
 public function __construct(PDO $dbh, array $config = array())
 {
 $this->dbh = $dbh;
 $this->config = $config;
 session_start();
 if ($this->isLoggedIn()) {
 $this->user = $this->getLoggedInUser();
 }
 }
 public function login($username, $password)
 {
 // check if user is already logged in
 // now validate your input data
 $user = $this->findUserByUsername($username);
 var_dump($user);
 if ($user) {
 if (password_verify($password, $user->password)) {
 $_SESSION['username'] = $user->username;
 $this->user = $user;
 return true;
 }
 throw new Exception('The password is incorrect');
 }
 throw new Exception("The username {$username} could not be found");
 }
 public function register(array $data)
 {
 // data validation goes here
 if ($data['password'] !== $data['password_again']) {
 throw new Exception("The passwords do not match");
 }
 if ($this->findUserByUsername($data['username'])) {
 throw new Exception('The username ' . $data['username'] . ' already exists');
 }
 $hash = password_hash($data['password'], PASSWORD_BCRYPT, array('cost' => 11));
 $sql = 'INSERT INTO users(username, password) VALUES (:username, :password)';
 $stmt = $this->dbh->prepare($sql);
 $stmt->bindParam(':username', $data['username'], PDO::PARAM_STR);
 $stmt->bindParam(':password', $hash, PDO::PARAM_STR);
 $result = $stmt->execute();
 if ($result) {
 $this->login($data['username'], $data['password']);
 return true;
 }
 throw new Exception('Something went seriously wrong');
 }
 public function logout()
 {
 if ($this->isLoggedIn()) {
 unset($_SESSION['username']);
 return true;
 }
 return false;
 }
 public function isLoggedIn()
 {
 return array_key_exists('username', $_SESSION);
 }
 public function getLoggedInUser()
 {
 if ($this->user === null) {
 $this->user = $this->findUserByUsername($_SESSION['username']);
 }
 return $this->user;
 }
 protected function findUserByUsername($username)
 {
 $stmt = $this->dbh->prepare('SELECT * FROM users WHERE username = :username');
 $stmt->bindParam(':username', $username, PDO::PARAM_STR);
 $stmt->execute();
 return $stmt->fetch(PDO::FETCH_OBJ);
 }
}
$dbh = new PDO('mysql:host=localhost;dbname=test', 'root', '');
$auth = new Auth($dbh);
try {
 // login example
 //$auth->login($_POST['username'], $_POST['password']);
 // register example
 //$auth->register($_POST);
} catch (Exception $e) {
 die($e->getMessage());
}
// logout
if (isset($_GET['logout'])) {
 $auth->logout();
}
// check if logged in
if ($auth->isLoggedIn()) {
 $username = $auth->getLoggedInUser()->username;
 echo '<p>Welcome <a href=#' . $username . '>' . ucfirst($username) . '</a></p>';
 echo '<p><a href="?logout">Logout</a></p>';
}

• smt: You should extract "session login" logic into separate protected method and probably introduce some storage interface. Anyway I've impressed by this simple class it's the best I've seen in PlainOldPhp without usage of any framework. It is both concise and logical.
• Nathan Bishop: Thanks @smt for your advice and praise. Normally, the classes I write are more separated however, I was strapped for time and only wanted to provide a class for OP, that wraps his existing logic into something reusable and takes advantage of PDO.