PHP Signup & Login

Question

I want everything to be well and secure with my authentication code. I've done many signup and login systems before but just for testing purposes, and didn't take into consideration all the security measures like SQL injection, password hashing etc... This is the code. If you can, please point out any function that could be enhanced.

Signup:

public function setUser(){

$fullName = htmlspecialchars($_POST['fullName']);
$email = htmlspecialchars($_POST['email']);
$password = htmlspecialchars($_POST['password']);
$DOB = htmlspecialchars($_POST['DOB']);
$nationality = htmlspecialchars($_POST['nationality']);
$workTitle = htmlspecialchars($_POST['workTitle']);
$workPlace = htmlspecialchars($_POST['workPlace']);
$phoneNumber = htmlspecialchars($_POST['phoneNumber']);
$userType = htmlspecialchars($_POST['userType']);


//PASSWORD HASHING
$hashedPassword = password_hash($password, PASSWORD_DEFAULT);

if($userType == "Investor"){
  $accNumber = "A" . sprintf("%06d", mt_rand(1, 999999));
}else{
  $accNumber = "B" . sprintf("%06d", mt_rand(1, 999999));
}

try {

  $sql = "INSERT INTO user (fullName, email, password, DOB, nationality, workTitle, workPlace, phoneNumber, userType, accNumber) VALUES (?,?,?,?,?,?,?,?,?,?)";
  $stmt = $this->connect()->prepare($sql);
  $stmt->execute([$fullName, $email, $hashedPassword, $DOB, $nationality, $workTitle, $workPlace, $phoneNumber, $userType, $accNumber]);

} catch (PDOException $e) {
  throw new Exception($e->getMessage());
}
header('location:login.php');

} //setUser()

Login:

//RETRIEVE HASHED PASSWORD FROM DB
public function getPassword($email){
  $sql = "SELECT password FROM user WHERE email = ?";
  $stmt = $this->connect()->prepare($sql);
  $stmt->execute([$email]);

  if($stmt->rowCount()){
    while($row = $stmt->fetch()){
      return $row['password'];
    }
  }
} //getPassword()

//LOGIN THE USER
public function loginUser(){

$email = htmlspecialchars($_POST['email']);
$password = htmlspecialchars($_POST['password']);
$hashedPassword = $this->getPassword($email);

//Check if hashed password is equal to password
if(password_verify($password,$hashedPassword)){
  try {
    $sql = "SELECT * FROM user WHERE email = ?";
    $stmt = $this->connect()->prepare($sql);
    $stmt->execute([$email]);

    if($stmt->rowCount()){
      while($row = $stmt->fetch()){
        $_SESSION['userID'] = $row['userID'];
        $_SESSION['fullName'] = $row['fullName'];
        $_SESSION['email'] = $row['email'];
        $_SESSION['DOB'] = $row['DOB'];
        $_SESSION['nationality'] = $row['nationality'];
        $_SESSION['workTitle'] = $row['workTitle'];
        $_SESSION['workPlace'] = $row['workPlace'];
        $_SESSION['phoneNumber'] = $row['phoneNumber'];
        $_SESSION['userType'] = $row['userType'];
        $_SESSION['accNumber'] = $row['accNumber'];
        $_SESSION['status'] = $row['status'];
        $_SESSION['profilePic'] = $row['profilePic'];
        header('location:dashboard.php');
      }
    }

  } catch (\Exception $e) {
    throw new Exception($e->getMessage);
  }
}else{
  echo"Invalid email or password";
}

} //loginUser()

I haven't worked on validation yet, but I will. I just need to make sure that everything else is good to go.

Answer 1

I can spot only one issue related to security:

$accNumber = "A" . sprintf("%06d", mt_rand(1, 999999));

there is a fair chance for two customers to get the same account number. I would make it simply a letter + row id.

The rest of your code is pretty secure but there is a lot of useless or superfluous or misplaced or inefficient stuff.

$fullName = htmlspecialchars($_POST['fullName']);
$email = htmlspecialchars($_POST['email']);
$password = htmlspecialchars($_POST['password']);
...

this stuff is misplaced. htmlspecialchars() belongs to output; it has nothing to do with user registration. Neither does $_POST. What if you will have to create a command line utility to create a user? A setUser() function should accept user data as a parameter, and where this data comes from is not this function's concern.

throw new Exception($e->getMessage());

inside a catch block is superfluous and useless. If you leave the exception alone, it will be thrown all the same!

$this->connect()

is misplaced. Given this class related to a user, whatever database connection stuff is alien to him. A user is not a database. A user is a human. Indeed they have a name, a phone number, a workplace... But a database connection?! Do you have a connect() method? So shouldn't your users as well. A database connection should be a service provided through class constructor.

header('location:login.php');

again, whatever HTTP stuff is none of the setUser's concern, it should be moved elsewhere.

So in the end setUser should be like this:

public function setUser($fullName, $email, $password, $DOB, $nationality, $workTitle, $workPlace, $phoneNumber, $userType) {

    $hashedPassword = password_hash($password, PASSWORD_DEFAULT);

    if($userType == "Investor"){
      $accNumber = "A";
    }else{
      $accNumber = "B";
    }

    $sql = "INSERT INTO user (fullName, email, password, DOB, nationality, workTitle, workPlace, phoneNumber, userType) VALUES (?,?,?,?,?,?,?,?,?,?)";
      $stmt = $this->db->prepare($sql);
      $stmt->execute([$fullName, $email, $hashedPassword, $DOB, $nationality, $workTitle, $workPlace, $phoneNumber, $userType]);
      $id = $this->db->lastInsertId();
      $sql = "UPDATE user SET accNumber = concat(?, id) WHERE id = ?";
      $stmt = $this->db->prepare($sql)->execute([$accNumber, $id]);
}

login

public function getPassword($email){

looks useless and superfluous. Why would you want a distinct function to get a users hash? Especially if you would call another query to get all user's info? Actually, whole login process takes just a few lines

if($stmt->rowCount()){

is superfluous

while($row = $stmt->fetch()){

is superfluous. When a single row is expected, no loop is required

$_SESSION['userID'] = $row['userID'];

is painfully repetitive. And polluting your session as well. Why not to do just

$_SESSION['user'] = $row;

it will make your session tidy. When you wjll have to add a shopping cart, it wont be mixed with user data.

echo"Invalid email or password";

such a function should never do any output as well.

in the end it should be like

public function loginUser($email, $password){
    $stmt = $pdo->prepare("SELECT * FROM users WHERE email = ?");
    $stmt->execute([$email]);
    $user = $stmt->fetch();

    if ($user && password_verify($password, $user['password']))
    {
        $_SESSION['user'] = $user;
        return true;
    }
}

then used like this

if ($user->loginUser($_POST['email'], $_POST['password'])) {
    header("Location: somewhere");
    exit;
} else {
    echo"Invalid email or password";
}

Answer 2

I agree with Your common Sense's responses, and a few other points:

Selecting multiple rows for single account

It appears that loginUser() selects all rows from the user table where the value in the email column matches the bound parameter. Presumably the values in that column are unique but even if the database has a constraint on that column, the PHP code should probably only allow one record to be returned from that query and update the session data. So the query could select one row, and instead of calling fetch() in a while loop, just call fetchAll() and ensure that the return value is an array and has one and only one element.

Then for the code currently in the while statement, that could be consolidating by looping over the fields, which could be defined in a constant:

const USER_FIELDS_TO_COPY_TO_SESSION = ['userID', 'fullName' ....];

Then use that list of fields when assigning values:

if($stmt->rowCount() == 1){
    $row = $stmt->fetch();
    foreach(self::USER_FIELDS_TO_COPY_TO_SESSION as $field) {
        $_SESSION[$field] = $row[$field];
    }
}

Or if all values in that row can be put in the session, just use array_merge().

if($stmt->rowCount() == 1){
    $row = $stmt->fetch();
    $_SESSION = array_merge($_SESSION, $row);
}

Validation

For validation, filter_var() can be used for validating and sanitizing inputs- perhaps you already knew that.

Repeated code

For a block of logic like this:

if($userType == "Investor"){
  $accNumber = "A" . sprintf("%06d", mt_rand(1, 999999));
}else{
  $accNumber = "B" . sprintf("%06d", mt_rand(1, 999999));
}

One could argue that the redundancies should be reduced. One way to do that is to only have the values that change inside the branches, and then the common code follow. Take the following re-written code for an example:

$prefix = 'B'; //default
if($userType == "Investor"){
  $prefix = "A";      
} 
$accNumber = prefix . sprintf("%06d", mt_rand(1, 999999));  

This way, the call to sprintf() only appears once. The advantage here is if values needed to be updated (e.g. the values passed to mt_rand()) that would only require updating one line instead of two. While this is a short block of code, the concept can apply to much larger blocks as well.

While the re-written line has the same number of lines as the orignal, it could be simplified using a ternary operator:

$accNumber = ($userType == "Investor" ? "A" : "B") . sprintf("%06d", mt_rand(1, 999999));   

Though some might argue that might be less readable, and can make the length of the line longer than is preferential.

Answer 3

I see a lot of SRP violation.

setUser() - I think the function name itself is ambiguous and so open to SRP issues being added. It should "set the user", but that could be all manner of things.

And there are multiple things in that function which are needed by the function, but shouldn't be defined in there, just the result requested from elsewhere.

For example you do $hashedPassword = password_hash(), this should be another function. How is defining how you hash passwords anything to do with "setUser"?

function hashThePassword($password)
{
    return password_hash($password, PASSWORD_DEFAULT);
}

Then you can call this whenever needed, and if you ever change how it works, it's one place to be changed.

As was pointed out, htmlspecialchars() is for browser output (entities). When you save data, it should be secure, but "raw" in that it's not styled or organised in anyway, because you may need to use the data in different ways.

if($userType == "Investor"){ this smells like it should be elsewhere. This is not "setUser" but "getUsersAccountNumber", again I see another function needed:

function getUsersAccountNumber($userType)
{
    if ($userType == "Investor") {
      return "A" . sprintf("%06d", mt_rand(1, 999999));
    } else {
      return "B" . sprintf("%06d", mt_rand(1, 999999));
    }
}

This is then something you can call in various other places and any future changes will be done in one place and ripple down. (That said, I don't profess to understand why a "user type" would determine their account number, and there being only 2 possible ones as well.)

---

loginUser():
I don't like the name. What is the task of "login a user"? There are various possible tasks so this is open to have SRP issues. In fact you already check the password, which this function needs to do, but it shouldn't define how the password checking is done. It's not a task for "logging them in", so should be a separate function.

In that function I see:
$hashedPassword = $this->getPassword($email);

Where is this object from? I see no class in your code. That aside, get password ($email)? Even if this is all legit, the function should be called getHashedPassword(), especially around other POST data as it's wide open to being confusing as to what data this is.

---

Just a couple of hints really. Name functions what they do and only what they do, when you then try to add something that doesn't fit within the functions name, it's probably a separate centralised and shared function that you need to write and call in this one.

I'd also suggest getting in to the habit of using PSR2, even loosely. You'll benefit from it later on.