PHP Login Class - Security and Efficiency Check

Question

I'm hoping someone would be able to identify if my code is prone to SQL injections, and just overall see if there is anything that could be done better, faster or more efficiently

Im very new to OOP and prepared statements so there might be some glaringly obvious problems but just let me know!

auth_class.php

require 'db.php';
session_start(); ///needs an open session to set the logged in value
class User {


    public $username = null;
    public $password = null;
    public $name = null;
    public $email = null;
    public $ftp = null;
    public $connection = null;


function __construct(){
 $this->connection = connect_db(); ///this is the basic database connection
}



public function passwordValidator($password){
        if (strlen($password) < 5) {
                $errors[] = "Password too short!";
        }

        if (!preg_match("#[0-9]+#", $password)) {
                $errors[] = "Password must include at least one number!";
        }

        if (!preg_match("#[a-zA-Z]+#", $password)) {
                $errors[] = "Password must include at least one letter!";
        }   

        if(empty($errors)){
            $this->password = password_hash($password, PASSWORD_DEFAULT);
            return;
        }else{
            return $errors;//this function checks the password and returns an array of errors it is checked on the other end
        }
}



 public function storeFormValues($username, $name, $email) {
    //This needs to check the values below to confirm that there is somthing there! Although they are set as required inputs
                    if(empty($username)){
                        $errors[] = "please Enter a username!";
                    }else{
                            if (!preg_match("/^[a-zA-Z ]*$/",$username)) {
                                $errors[] = "Only letters and white space allowed"; 
                            }else{
                                $this->username = $username;
                            }   
                    }

                    if(empty($name)){
                        $errors[] = "please Enter a name!";
                    }else{
                            if (!preg_match("/^[a-zA-Z ]*$/",$name)) {
                                $errors[] = "Only letters and white space allowed"; 
                            }else{
                                $this->name = $name;
                            }
                    }

                    if(empty($email)){
                        $errors[] = "please Enter a email!";
                    }else{
                        if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
                            $errors[] = "Invalid email format"; 
                        }else{
                            $this->email = $email;
                            $this->ftp = $email;
                        }
                    }

            if(empty($errors)){
                return; //no errors all stored fine
            }else{
                return $errors;//this function checks the password and returns an array of errors it is checked on the other end
            }

 }

 public function logout() {
        session_destroy();
        header("Location: ../login.php"); ///destroys all session data and redirects to login

 }


 public function userLogin($email, $password) {
            try{
                                $sql = "SELECT * FROM accounts where account_email = ?";
                                $stmt = $this->connection->prepare($sql);
                                $stmt->execute(array($email));

                                if ($stmt->rowCount() > 0){
                                        $output = $stmt->fetch();
                                            $hash = $output['account_password'];

                                            if (password_verify($password, $hash)) {
                                                    header_remove(); 
                                                    header('Location: ./home.php');
                                                    $_SESSION['loggedIn'] = 1;
                                                    $_SESSION['username'] = $output['account_username'];
                                                    $_SESSION['id'] = $output['account_id'];
                                                    $_SESSION['name'] = $output['account_name'];
                                                    $_SESSION['email'] = $output['account_email'];
                                                    return; //user logged in
                                            } else {
                                                    $_SESSION['loggedIn'] = 0;
                                                    $error[] = "Wrong Password!";
                                                    return $error; //user password didnt match
                                            }

                            }else{
                                $error[] = "User Not Found!"; ///no user found
                                return $error;
                            }

                }catch (PDOException $e){
                    //$e->getMessage();
                    $error[] = "There is an error Contact [email protected]";
                    return $error;
                }

     }





  public function userRegister() {
            //below is the basic code to add a user to the database
                try{    
                    $sql = "INSERT INTO accounts (account_name, account_password, account_username, account_email, ftp_user) VALUES (?,?,?,?,?)";
                    $stmt= $this->connection->prepare($sql);
                    $stmt->execute([$this->name, $this->password, $this->username, $this->email, $this->ftp]);


                    return;
                } catch (PDOException $e) {
                    //$error[] = "DataBase Error: The  could not be added.<br>".$e->getMessage();
                    $error[] = "We already have an account with that username!";
                    return $error;
                }
  }

public function changeUserData() {
        //below is the basic code to add a user to the database
            try{    
                $sql = "UPDATE accounts SET account_name=?, account_password=?, account_username=?, account_email =? ";
                $stmt= $this->connection->prepare($sql);
                $stmt->execute([$this->name, $this->password, $this->username, $this->email]);


                return;
            } catch (PDOException $e) {
                $error[] = "DataBase Error: ERROR.<br>".$e->getMessage();
                //$error[] = "We already have an account with that username!";
                return $error;
            }
}
}

db.php

function connect_db()
{
try
{


        $servername = "localhost";
        $username = "root";
        $password = "";

        try {
            $conn = new PDO("mysql:host=$servername;dbname=main_database", $username, $password);
            // set the PDO error mode to exception
            $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            //echo "Connected successfully"; 
            }
        catch(PDOException $e)
            {
            echo "Connection failed: " . $e->getMessage();
            }   
}
catch (PDOException $e)
{
    // Proccess error
    echo 'Cannot connect to database: ' . $e->getMessage();
}

return $conn;
}

And this is the section at the top of my login page.

include("./php/classes/auth_class.php");

if(isset($_SESSION['loggedIn']) && $_SESSION['loggedIn'] === 1) {
         header('Location: ./home.php');
}

$notification = NULL;

if(isset($_POST['submitlogin'])){
   $instance = new User();
   $loginValidate = $instance->userlogin($_POST['email'], $_POST['password']);

   if(empty($loginValidate)){
       //Login successfull
   }else{
      //print_r($loginValidate);
      //login unsucessfull
      $notification = $loginValidate;
   }
}


if(isset($_POST['submitregister'])){
   $instance = new User();
   $passwordValidate = $instance->passwordValidator($_POST['upass']);

   if(empty($passwordValidate)){
      $formValueValidator = $instance->storeFormValues($_POST['uname'],$_POST['fullname'], $_POST['uemail']);
         if(empty($formValueValidator)){
                  $registerValidate = $instance->userRegister();

                  if(empty($registerValidate)){
                     ///user registed fine
                     $notification = "Registration complete please login!";
                  }else{
                     //print_r($registerValidate); ///prints errors if found with register
                     $notification = $registerValidate;
                  }
         }else{
         ///unable to store form values 
         //print_r($formValueValidator);
         $notification = $formValueValidator;
         }

   }else{
      //print_r($passwordValidate); //unable to validate password
      $notification = $passwordValidate;
   }
}

Thanks in advance! Cheers, Greg

Answer 1

• Your three password requirements can be baked into one expression.

  if (!preg_match('/(?=.*[a-z])(?=.*\d).{5,}/i', $password)) {
      $this->errors[] = 'Passwords must have a minimum of 5 characters and contain at least one letter and at least on number';
      return false;
  }
  $this->password = password_hash($password, PASSWORD_DEFAULT);
  return true;
  

  Executing one function versus three improves efficiency, but not in a noticeable way. I like to have consistent/dependable return values (rather than returns that sometimes provide an iterable value and sometimes not).

• For "tin-foil hat" reasons, I like to advise that only non-personal data be stored in sessions (as much as possible) because of maliciousness called "Session Hijacking". In other words, save an arbitrary id, but not name, email, password, creditcard number, social security number, drivers license, library card, ...anything that might be valuable to bad people who like to spoof, hack, and trick others.

• I would do away with the rowCount() > 0, you only need to check if fetch() has any data in it. https://stackoverflow.com/a/37611531/2943403

• By returning boolean or possibly "truthy" / "falsey" values from your methods, you can reliably construct method calls that easily interpret their success. Consider in the future that you want to return the last inserted id or the number of affected rows from an update/delete query -- if you are passing an array of errors in some instances, you'll first need to assess the return's data type to determine how to handle it. For this reason, make a class variable ($errors) to gather any errors, and always return either iterable or non-iterable data.

• When checking the returns from your method calls, you are calling empty(), but you can simply use !$variable -- it will know the difference between a falsey null and a truthy array of one or more errors.