4
\$\begingroup\$

I'm pretty new to PHP and programming general so I'm quite sure my code is an absolute mess. My goal is to write good code, that others can understand just by looking at it.

This is a simple login system I've been working on. Critique at will.

Link to said code - GitHub

General area of interest are the classes:

<?php
/**
* Handles the login of the user
*/

class Login
{
    /**
    * This starts whenever an object of this class is created
    * For example $login = new Login(); 
    * Coincidently one just like this exists in index.php
    */
    public function __construct(){
        //create or read session
        session_start();

        /**
        *If a cookie exists on the users PC named user and id
        *Just redirect him to logged_in page.
        *TODO: I should really encrypt these cookies
        */
        if((isset($_COOKIE['user'])) && (isset($_COOKIE['id']))){
            header('Location: logged_in.php');
        }

        require_once 'forms/login_form.php';

        //If not logged it, log the user in
        if(isset($_POST['login'])){
            $this->doLogin();
        }

    }

    /**
    *@bool Return true or false
    * Note: This function repets in the other 2 classes
    * I should probably make it not repet
    */
    private function isEmpty($var){
        $var = trim($var);
        if (empty($var)){
            return true;
        }
        //default case
        return false;  
    }

    /**
    * log in with post data
    */
    public function doLogin(){
    if( ! $this->isEmpty($_POST['username']) || 
        ! $this->isEmpty($_POST['password'])){
            $username = ($_POST['username']);
            $password = ($_POST['password']);
            /*
            * sha1 is just for demonstration purposes
            * it will be replaced with password_hash once I update 
            * to PHP 5.4.x I'm currently using 5.3 due to Win XP limitations
            */
            $password = sha1($password);

            /**
            * Require the files needed
            */
            require_once 'db/db_connect.php';
            require_once 'db/db_tables.php';

            /**
            * Note that I'm using variables here
            * Don't worry, they don't come from user input
            * They come from the db_tables.php file
            */
            $sqlQuery = $dbPDO->prepare("SELECT $loginUsername, $loginEmail, $loginId
                                         FROM $tableName 
                                         WHERE $loginUsername=:username 
                                         AND $loginPassword=:password");

            $sqlQuery->execute(array(':username' => $username,
                                     ':password' => $password));

            /**
            * Note: I could have used PDO's rowCount() here but chose not to
            * There's no particular reason why I did that
            */
            $isInDb = $sqlQuery->fetch(PDO::FETCH_ASSOC);
            if( ! $isInDb){
                echo '<p> The username or password do not match any registered users </p>';
            } else {

                /**
                * Great, the user's logged in
                * Time to set the cookies and redirect him
                */
                setcookie('user', $isInDb['login_username']);
                setcookie('id', $isInDb['login_id']);
                $_SESSION['id'] = $isInDb['login_id'];
                header('Location: ' . 'logged_in.php');
            }
        } else {
            echo 'You must fill in all fields';
        }
    }
}

RecoverPassword class

<?php
/**
* Handles the password recovery
*/
class RecoverPassword
{
    /**
    * This gets execute whenever an object from this class is created
    */
    public function __construct(){
        if(isset($_POST['recoverPassword'])){
            $this->doRecover();
        }
    }

    /**
    * @bool checks if the email is in a valid format
    * Note: This has some flaws
    * For example using numbers in the name of the email MAY give back some false negatives
    * For more details see this question: https://stackoverflow.com/questions/3722831/does-phps-filter-var-filter-validate-email-actually-work
    */
    private function isEmailValid($email){
        if (filter_var(trim($email), FILTER_VALIDATE_EMAIL)){
            return true;
        }
        return false;
    }

    /**
    *@bool Return true or false
    * Note: This function repets in the other 2 classes
    * I should probably make it not repet
    */
    private function isEmpty($var){
        $var = trim($var);
        if (empty($var)){
            return true;
        }
        return false;  
    }

    /**
    * @bool Simple regex match
    */
    private function isPasswordValid($password){
        return (preg_match('/^(?=.*\d)(?=.*[A-Za-z])[0-9A-Za-z!@#$%]{5,40}$/', trim($password)));
    }

    /**
    * @bool Here is the actual recovery of the password...which doesn't work.
    */
    private function doRecover(){
        if( ! $this->isEmpty($_POST['email'])){
            if($this->isEmailValid($_POST['email'])){
                $email = $_POST['email'];
                /**
                * Require the connect to DB and tables files
                */
                require_once 'db/db_connect.php';
                require_once 'db/db_tables.php';

                /**
                * Note that I'm using variables here
                * Don't worry, they don't come from user input
                * They come from the db_tables.php file
                */
                $sqlQuery = $dbPDO->prepare("SELECT $loginEmail FROM $tableName WHERE $loginEmail= :email");
                $sqlQuery->execute(array(':email' => $email));

                /**
                * Note: I could have used PDO's rowCount() here but chose not to
                * There's no particular reason why I did that
                */
                $isInDb = $sqlQuery->fetch(PDO::FETCH_ASSOC);

                //Does the email exist?
                if( ! $isInDb){
                    die('<p>Email doesn\'t exist!</p>');
                } else {
                    echo 'The email exists. Currently the recover password email will not send due to not having a proper SMPT configured.';
                    //TODO - Add mail function to send recover password
                }

            } else {
                echo '<p>Email is invalid</p>';
            }

        } else {
            echo ('<p>Email field is empty!</p>');
        }
    }
}

Register class

<?php
/**
* Handles the register of the user
*/
class Register
{
    /**
    * This gets execute whenever an object from this class is created
    */
    public function __construct(){
        //Require the HTML part of page
        require_once 'forms/register_form.php';

        //If user presses the register button, register the user
        if(isset($_POST['register'])){
            $this->doRegister();
        }
    }

    /**
    *@bool Return true or false
    * Note: This function repets in the other 2 classes
    * I should probably make it not repet
    */
    private function isEmpty($var){
        $var = trim($var);
        if (empty($var)){
            return true;
        }
        //default case
        return false;  
    }

    /**
    * @bool
    */
    private function isEmailValid($email){
        #$email = trim($email);
        if (filter_var(trim($email), FILTER_VALIDATE_EMAIL)){
            return true;
        }
        //default case
        return false;
    }

    /**
    * @bool
    */
    private function isUsernameValid($username){
        return (preg_match('/^[a-z\d_]{5,20}$/i', trim($username)));
    }

    private function isPasswordValid($password){
        return (preg_match('/^(?=.*\d)(?=.*[A-Za-z])[0-9A-Za-z!@#$%]{5,40}$/', trim($password)));
    }

    /**
    * Here we register the user
    * Add him to the database etc etc
    */
    private function doRegister(){

    //Make sure the fields are not empty
    if( ! $this->isEmpty($_POST['username']) ||
        ! $this->isEmpty($_POST['password']) ||
        ! $this->isEmpty($_POST['email'])){
    } else {

        /**
        * I'm using die here because I chose not to do the
        * check using java-script or HTML
        * This is purely done for the hell of it
        * However, java-script or HTML would have been the better choice
        * This applies to all the die();s in this page
        */
        die('<p>Some fields are empty.</p>');
    }

    //Is the email valid?
    if($this->isEmailValid($_POST['email'])){
        $email = trim($_POST['email']);

    } else {
        die('<p>Email is invalid</p>');
    }

    //Is username valid?
    if($this->isUsernameValid($_POST['username'])){
        $username = trim($_POST['username']);
    } else {
        die('<p>The username must be between 5 and 20 characters long. And it can only contain numbers and letters!</p>');
    }

    if($this->isPasswordValid($_POST['password'])){
        $password = trim($_POST['password']);
        /*
        * The password is hashed with sha1 simply because I'm using
        * PHP 5.4 - I will replace it with password_hash() when I upgrade to PHP 5.4
        */
        $password = sha1($password);
    } else {
        die('<p>The password must be between 5 and 40 characters long, must contain at least one number,
        at least one letter, and any of the following characteres: ! @ # $ %</p>');
    }

    require_once 'db/db_connect.php';
    require_once 'db/db_tables.php';

    /**
    * The queries below are repetive
    * in order to display an error for
    * username or email
    * I think I could marked the coulmns in the DB
    * as Unique to make things easier however I didn't get around to trying that
    * and I'm not sure if it works
    */

    $sqlQuery = $dbPDO->prepare("SELECT $loginUsername FROM $tableName WHERE $loginUsername=:username");
    $sqlQuery->execute(array(':username' => $username));
    $isInDb = $sqlQuery->fetch(PDO::FETCH_ASSOC);

    if($isInDb['login_username'] == $username){
        die('<p>That username is already registered!</p>');
    }

    $sqlQuery = $dbPDO->prepare("SELECT $loginEmail FROM $tableName WHERE $loginEmail=:email");
    $sqlQuery->execute(array(':email' => $email));
    $isInDb = $sqlQuery->fetch(PDO::FETCH_ASSOC);

    //Does the email already exist?
    if($isInDb['login_email'] == $email){
        die('<p>That email is already registered!</p>');
    }
    /**
    * Here we actually register the user
    * He passed all the checks and we can sefely
    * insert him into the database
    */
    $sqlQuery = $dbPDO->prepare("INSERT INTO $tableName($loginUsername, $loginPassword, $loginEmail)
                                 VALUES (:username, :password, :email)");
        if($sqlQuery->execute(array(':username' => $username,
                                    ':password' => $password,
                                    ':email'    => $email))){
            echo '<p>Registration succesful!</p>';
        } else {
            die('<p> There was an error in the registration process.</p>');
        }                         
    }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ I looked over your scripts for possible SQL injection, and found them to be quite secure, in my opinion. Good job. \$\endgroup\$ – Phrancis Aug 29 '14 at 23:41
  • 1
    \$\begingroup\$ if((isset($_COOKIE['user'])) && (isset($_COOKIE['id']))){ header('Location: logged_in.php'); } Just a head's up, I wouldn't do such a thing. People could bypass a login screen by creating cookies on their own. I would rather use SESSIONS. \$\endgroup\$ – Abandoned Account Aug 30 '14 at 11:03
3
\$\begingroup\$

Just a couple of things I wanted to point out:

  • Your classes are violating the Single Responsibility Principle heavily. They have so many responsibilities; handling sessions, rendering templates, creating PDO objects, writing to the database, accessing superglobals, validating input, redirecting, and more.

  • Just because your code is encapsulated in classes does not make it OOP (if that was your intent). You have pretty much just wrapped procedural code into a class.

  • I would try to keep the logic in the constructors to a minimum.

  • No useful comments; they are stating the obvious. Do you really find that the comment //create or read session is necessary for session_start()? Or // This gets execute whenever an object from this class is created for __construct()? I think we both agree that they are self-explanatory.

I would abstract all your code into different classes to give each it's own responsibility. Then, with some type of controller, you could make them all work together (by utilizing dependency injection) to accomplish any goal (registering users, logging in, etc).

You might be interested in the MVC pattern as a starting point to guide you further in this abstraction process. It enforces business logic separation from UI logic. Your classes have traits of controllers of this pattern.

As for the rest, I think your code looks pretty clean.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the review. I'll rewrite to to be more clean and drop some of the comments. I read somewhere that a class should take care of one thing and one thing only. I could be way off with that statement though. Again, thanks for the input. Would it be ok to write an interface to take care of all the PDO stuff? \$\endgroup\$ – user2997488 Aug 29 '14 at 23:57
  • \$\begingroup\$ A class should have only one reason to change. Let's say I wanted to change the way sessions get handled, I would for example modify the HttpSession class and if I wanted to change how the rendering of templates was handled I would modify the View class. In your case, everything is mixed up in 1 class. It has multiple reasons to change, and that is what violates the SRP. Interfaces just define a contract, they do not contain logic so I don't see how they can "take care of all the PDO stuff". \$\endgroup\$ – Kid Diamond Aug 30 '14 at 0:00
1
\$\begingroup\$

Bug

I think here:

if( ! $this->isEmpty($_POST['username']) || 
    ! $this->isEmpty($_POST['password'])){

You mean && instead of ||.

duplicate code

As you already note, you define isEmpty three times, which is not good. Fox example, if you want to update it, you have to remember to update it in three places.

And isEmpty is not alone. You also have isEmailValid and isPasswordValid (without a note). If you follow @Kid Diamonds advise and make your code more OOP, this should be easy to fix.

Empty if block, Boolean Logic, and another Bug

if( ! $this->isEmpty($_POST['username']) ||
    ! $this->isEmpty($_POST['password']) ||
    ! $this->isEmpty($_POST['email'])){
} else {

It's better to avoid empty if blocks like those. Just flip the condition.

!(!A || !B || !C) <=> A && B && C

But we also see that your logic is wrong. It should have been

if($this->isEmpty($_POST['username']) ||
   $this->isEmpty($_POST['password']) ||
   $this->isEmpty($_POST['email'])){
} else {

And then flipped (!(A || B || C) <=> !A && !B && !C):

if( ! $this->isEmpty($_POST['username']) &&
    ! $this->isEmpty($_POST['password']) &&
    ! $this->isEmpty($_POST['email'])){
    // your code, no else needed
}

Die

It is bad practice to let a class just die. Throw an exception instead.

Notes

You have a lot of notes in your code. Really a lot. This makes reviewing your code a bit annoying.

Comments

Kid Diamond already pointed out that most of your comments are not too helpful, but I wanted to mention where I would have liked a comment:

  • isPasswordValid: the current comment is @bool Simple regex match, which is not helpful. But regexes are complicated, so I would have liked a comment saying how a valid password looks.
  • isEmpty: Return true or false is not helpful. But if you want to document all or most of your functions, you could for example write: checks if the trimmed input is empty, which would give the reader the additional information that " " is empty.
  • which doesn't work: [First of, it's best to not post code like that here, we require working code.] If you write a comment that code is not working, make sure to mention what is not working. And when does it happen (how can it be replicated)? And also, prepend TODO or a similar string the IDE can find and list.

Security

  • as you mention, sha1 is not really good enough.
  • I hope that you check the session in logged_in.php to see if a user is logged in. In Login you just check if a cookie with the name user and id is set, which anybody can set themselves. (I just checked your github code, and it doesn't seem that you do. It's not your submitted code, so I'm going to keep it short: this doesn't seem secure).
  • (also from your github): Don't hardcode your database credentials. I'm sure the password is only a dummy password, but will you always remember to change it? It's better to use an external configuration file (which then can be excluded from the repository). Also, store the file containing the credentials outside the web root.

These were a lot of points and it might come across as too negative (which is probably in the nature of a code review), but I generally think that your code is alright, especially for a beginner. So I'll end on two positive points: If you remove/fix your notes, your code will be easy to read (I think), and you use prepared statements everywhere, which is really good.

\$\endgroup\$

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.