4
\$\begingroup\$

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 help@help.com";
                    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

\$\endgroup\$
  • \$\begingroup\$ Not a major, but I find it much easier dealing with code when there's a standard naming convention across methods. For example, in your User class you have passwordValidator() and storeFormValues() - one is like a noun / name, the other like a verb / action. Instead, validatePassword() would make sense with storeFormValues(). \$\endgroup\$ – t-jam May 27 at 5:16
  • \$\begingroup\$ That makes sense I will change that thanks! Did you see any other problems? \$\endgroup\$ – Thomas Gregory May 28 at 8:35
2
\$\begingroup\$
  • 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.

\$\endgroup\$
  • \$\begingroup\$ As for storing values in session data would you instead use another function to get them if they are needed to be displayed on a page as appose to storing them in session? Also if im understanding correctly are you saying I should either return true/false or an array and not both? How would I know where it failed if it only returns true or false? \$\endgroup\$ – Thomas Gregory May 23 at 14:35
  • \$\begingroup\$ 1. Yes, you use the user's id from the session to query the database for what is needed for each page load. Because that value will be the PRIMARY KEY, the query will perform swiftly. 2. Yes, I recommend that pass truthy/falsey return values which share the same data type. If you want to reurn a populated array on failure, that's fine, just pass an empty array on success. This allows you to simply use if (!$returnValue) { ( or if ($returnValue) {) every time. \$\endgroup\$ – mickmackusa May 23 at 19:47
  • 1
    \$\begingroup\$ When if (!$booleanReturn) { (I mean you are returning literal true|false), you will know to access your $error class variable. ($instance->error) By the way instance is too vague of a variable name because one script may have multiple instances in play. Better to call it $user or something specific/relevant. \$\endgroup\$ – mickmackusa May 23 at 19:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.