PDO Login/Register system I've been working on

Question

I'm relatively new to PHP and programming as whole. I'm sure my code could be better in a lot of ways, most of which I can't fully grasp at the moment. As it stands I'm trying to avoid the general noobie mistakes(whatever they may be). And also trying to code better and cleaner so that other people can understand without too much hassle.

This is a simple login system I've been working on for the past few days.

Be harsh, please, point out as many flaws as you can. And, if possible, offer solutions to fix said flaws.

Link on GitHub to the code.

The general area of interest would be the classes.

---

Login class:

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

/**
* TODO: Redesign this class. It's too bloated with functions and does waaay too many things for just one class!
*/
class Login
{

    public function __construct(){
        session_start();
        /**
        * TODO: Add remember me cookie
        * uncomment this when it's done
        if(isset($_POST['rememberMe'])){
            $_COOKIE['rememberMe'] = TRUE;
        }
        */

        /**
        * If a session exists
        * Just redirect user to logged_in page.
        * Else, do login
        */
        if((isset($_SESSION['id'])) && (isset($_SESSION['username']))){
            header('Location: logged_in.php');
        }

        if(isset($_POST['login'])){
            $this->doLogin();
        }
    }

    /**
    * log in with post data
    */
    private function doLogin(){
         if( ! empty($_POST['username']) &&
             ! empty($_POST['password'])){

                $username = ($_POST['username']);
                $password = ($_POST['password']);

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

                /**
                * PHP Version 5.4.31 doesn't support password_hash()
                * so an extension called password_compat is used
                * Link to lib - https://github.com/ircmaxell/password_compat
                */
                require_once 'password_compat/lib/password.php';

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

                $hash = $sqlQuery->fetch();

                $passwordVerify = password_verify($password, $hash['login_password']);

                /**
                * Note that the variables used here
                * come from the db_tables.php file
                * not from user input
                */
                $sqlQuery = $dbPDO->prepare("SELECT $loginUsername, $loginId FROM $tableName WHERE $loginUsername=:username");

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

                $userVerify = $sqlQuery->fetch();

                if(($passwordVerify == 1) && ($userVerify['login_username'] == $username)){

                    /**
                    * Great, the user's logged in
                    * Time to set the session and redirect him
                    */
                    $_SESSION['id'] = $userVerify['login_id'];
                    $_SESSION['username'] = $userVerify['login_username'];
                    header('Location: logged_in.php');
                } else {
                    echo '<p> The username or password do not match any registered users.</p>';
                }
        } else {
            echo '<p> You must fill in all fields.</p>';
        }

    }

}

---

Register Class:

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

/**
* TODO: Redesign this class. It's too bloated with functions and does waaay too many things for just one class!
*/
class Register
{
    private $errors = array();

    private $username = NULL;

    private $password = NULL;

    public function __construct(){

        require_once 'forms/register_form.php';

        if(isset($_POST['register'])){
            $this->doRegister();
        }
    }

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

    /**
    * @bool regex match
    * The username must contain alphanumeric chars
    * and must be between 5 and 20 chars long
    * Example of valid username: thebrownfox
    */
    private function isUsernameValid($username){
        return (preg_match('/^[a-z\d_]{5,20}$/i', trim($username)));
    }

    /**
    * @bool regex match 
    * The password requires at least one uppercase latter, at least one lower case letter
    * at least one number and at least any of the following characters: ! @ # $
    * Length must be between 5 and 40 characters long.
    * Example of valid password: Thequickbrown200!
    */
    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(){

    if( ! empty($_POST['username']) &&
        ! empty($_POST['password']) &&
        ! empty($_POST['email'])    &&
        ! empty($_POST['passwordAgain'])){

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

            /**
            * @bool
            */
            if($this->isEmailValid($_POST['email'])){
                $email = trim($_POST['email']);
            } else {
                $errors[] = '<p>Email is invalid</p>';
            }
            /**
            * Check weather the password is valid
            */
            if($this->isPasswordValid($_POST['password'])){
                $password = trim($_POST['password']);
                $passwordAgain = trim($_POST['passwordAgain']);

                if($password != $passwordAgain){
                    $errors[] = '<p>The passwords do not match.<p>';
                }
                /**
                * PHP Version 5.4.31 doesn't support password_hash()
                * so I used an extension called password_compat
                * Link to lib - https://github.com/ircmaxell/password_compat
                */
                require 'password_compat/lib/password.php';

                $password = password_hash($password, PASSWORD_BCRYPT);
            } else {
                $errors[] = '<p>The password must be between 5 and 40 characters long, must contain at least one number, at least one letter and at least one non Alphanumeric character.</p>';
            }


    } else {
        $errors[] = '<p>Some fields are empty.</p>';
    }

    /**
    * Check if there are any errors so far.
    * I can't check after the queries because if some fields
    * are empty PHP would slap me with an error
* Also this is bad coding, i need to split up this class
    */
    if (!empty($errors)) {
        foreach($errors as $error){
            echo $error;
        }
        return;
    }

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

    /**
    * The queries below are repetitive
    * in order to display an error for
    * username or email
    * I think I could marked the columns 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));
    $doesUsernameExist = $sqlQuery->fetch(PDO::FETCH_ASSOC);

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

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

    /**
    * Does email already exist?
    */
    if($doesEmailExist['login_email'] == $email){
        $errors[] = '<p>That email is already registered!</p>';
    }

    /**
    * This foreach below is bad coding
    * I need to split up my code into several
    * classes, this class is doing too much
    */
    if (!empty($errors)) {
        foreach($errors as $error){
            echo $error;
        }
        return;
    }

    /**
    * Here we actually register the user
    * He passed all the checks and we can safely
    * 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 successful!</p>';
        } else {
            die('<p> There was an error in the registration process.</p>');
        }                         
    }
} 

---

Recover Password Class:

<?php
/**
* Handles the password recovery
*/

/**
* TODO: Redesign this class. It's too bloated with functions and does waaay too many things for just one class!
*/
class RecoverPassword
{
    public function __construct(){
        if(isset($_POST['recoverPassword'])){
            $this->doRecover();
        }
    }

    /**
    * @bool
    * 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;
    }

    private function doRecover(){
        if( ! empty($_POST['email'])){
            if($this->isEmailValid($_POST['email'])){
                $email = $_POST['email'];

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

                /**
                * Note that the variables used here
                * come from the db_tables.php file
                * not from user input
                */
                $sqlQuery = $dbPDO->prepare("SELECT $loginEmail FROM $tableName WHERE $loginEmail= :email");
                $sqlQuery->execute(array(':email' => $email));

                $doesEmailExist = $sqlQuery->fetch(PDO::FETCH_ASSOC);

                /**
                * Does the email exist?
                */
                if($doesEmailExist){
                    //echo $doesEmailExist['login_email'];
                    echo '<p>The email exists. Currently the recover password email will not send due to not having a proper SMPT configured.</p>';
                    /**
                    * TODO: Make this work. Configure SMTP server -> SwitfMail
                    * Swift Mailer is downloaded and configured now.
                    * Uncomment this when I get a chance to test
                    * require_once 'SendMailRecoverPassword.php';
                    * $swift = new RecoverPasswordFunctions();
                    */

                } else {
                    echo '<p>Email doesn\'t exist!</p>';
                }

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

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

---

Change Password Class:

<?php
/**
* Handles the password change
*/

/**
* TODO: Redesign this class. It's too bloated with functions and does waaay too many things for just one class!
*/

class ChangePassword
{
    /**
    * Instead of echoing each error on it's own
    * the errors are added to an array and iterate
    * thru the array before doing the updating
    * in the database. If the array is not empty
    * return to end the function essentially killing the script
    */
    private $errors = array();

    public function __construct(){

        if(isset($_POST['resetPassword'])){
            $this->doResetPassword();
        } 
    }

    private function doResetPassword(){
        /**
        * Here nothing "interesting"
        * happens. Just some checks to make sure the user entered the right
        * information into the fields. If not add said errors to previously
        * mentioned array.
        */
        if(empty($_POST['passwordCurrent']) &&
           empty($_POST['passwordNew']) &&
           empty($_POST['passwordNewAgain'])){

            $errors[] = '<p>Some fields are empty</p>';

        } else {
            $passwordCurrent    = trim($_POST['passwordCurrent']);
            $passwordNew        = trim($_POST['passwordNew']);
            $passwordNewAgain   = trim($_POST['passwordNewAgain']);

            if($passwordNew != $passwordNewAgain){
                $errors[] = '<p>Passwords do not match.</p>';
            }   

            /**
            * See further down some notes about 
            * this function
            */
            if( ! $this->isPasswordValid($passwordNew)){
                $errors[] = '<p>The password must be between 5 and 40 characters long, must contain at least one number, at least one letter and at least one non Alphanumeric character.</p>';
            }

            if($passwordNew == $passwordCurrent){
                $errors[] = '<p>Your new password cannot be the same as your old password.</p>';
            }

        }

        /**
        * If this array of errors is empty the user passed all the checks.
        * If not, a return happens, ending the script
        */
        if( ! empty($errors)){
            foreach($errors as $error){
                echo $error;
            }
            return;
        }

        /**
        * Here the user passed the checks
        * It's safe to update the database
        * with the new password
        */
        $this->insertPassword($passwordNew);
        $_SESSION = array();
        session_destroy();
    }

    private function insertPassword($passwordNew){
        /**
        * Go up one directory.
        * Conncatenate this var with paths from certain files
        */
        $upOneDir = realpath(__DIR__ . '/..');

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

        /**
        * PHP Version 5.4.31 doesn't support password_hash()
        * so I used an extension called password_compat
        * Link to lib - https://github.com/ircmaxell/password_compat
        */
        require $upOneDir . '/password_compat/lib/password.php';

        /**
        * NOTE: Hash strength is set to default(10)
        */
        $password = password_hash($passwordNew, PASSWORD_BCRYPT);

        /**
        * The username ID is dependent on the Session id
        * which is set to the username ID - in the database
        * you can find the username ID as equal to login_id
        * each ID is unique and each username is unique
        */
        $usernameId = $_SESSION['id'];

        $sqlQuery = $dbPDO->prepare("UPDATE $tableName SET $loginPassword=:passwordNew WHERE $loginId=:usernameId");
        $result = $sqlQuery->execute(array(':passwordNew' => $password,
                                           ':usernameId'  => $usernameId));
        if($result){
            echo '<p>Successfully changed the password</p>';
        } else {
            echo '<p>An error occurred while changing the password</p>';
        }

    }

    /**
    * @bool regex match 
    * The password requires at least one uppercase letter, at least one lower case letter
    * at least one number and at least any of the following characters: ! @ # $
    * Length must be between 5 and 40 characters long.
    * Example of valid password: Thequickbrown200!
    */
    private function isPasswordValid($password){
        return (preg_match('/^(?=.*\d)(?=.*[A-Za-z])[0-9A-Za-z!@#$%]{5,40}$/', trim($password)));
    }

}

---

Before anybody points it out:

• The classes do way to many things. I realize that. In fact some kind enough people have pointed it out. I should redesign them and separate the responsibilities. But as it stands I don't know how to properly do that. It's more of "thinking"(if that makes sense) problem rather than a coding problem.

• My comments may not be the best they can be. Again, I don't know any better as it stands. And I went with the idea that some comments are better than none.

---

Totally off-topic question. Feel free to ignore it completely, I honestly just don't know where else to ask. In about 3 days I'll be interviewing for a Junior PHP dev position(yeah, I know, I don't believe it either).

Judging purely by this code, where do I stand?

I realize this question is so open for interpretation and skewed by each persons point of view, that it doesn't even have a close-to-accurate answer. Hench the reason I suggested that it be ignored if it's too vague.

Answer 1

First of, good luck with your interview!

Over thinking and not thinking enough

Writing code is easy, but writing code that will last longer then a minute is hard. You obviously know how to code. And you have a notion of separation of concern. You comment your code (not 100%, but still. you comment).

But still, your code is crap. Sorry, but it really is. You are abusing classes so hard that kittens are dying. Not to speak of you overall design. You did some thinking, took the wrong decisions and then did some more thinking in that wrong direction.

Those useless objects

• None of your objects have public methods. So what can I do with them?
• Take away your is* methods and every class just has 1 method. And it is private but called in the __construct.

So to sump up: you wrote 4 functions that do, well nearly everything. For instance your Login::doLogin, it knows:

• how the data is send to the application ($_POST stuff)
• where you database files are located and what they do (require stuff)
• that php5.4.31 doesn't support password_hash (I didn't even know that)
• how to query a database (no very good at it, he performs the same query twice but retrieves different data.)
• How to handle a use session ($_SESSION stuff)
• How to construct html and when to output it to the user
• How to send a header along with the response.
• how to validate input

And then you write 'A simple login system'. But one you can't use with no public methods. And one that does everything his way. You want the output in a different language? Well go f@ck yourself! Oh, you want a different kind of database? Well, not-my-problem. You want to change where you save your db methods? Haha, suck3r

What you did write is a cocky login system that can't be reused without touching it (sounds so wrong).

The design is bad

When a single method does so much, something is wrong. And if it is the only method in a class, and private. Then kittens will die. So let's fix it by going back to the drawing board. And I literally mean pen and paper. Every good code starts on a drawing board (or equivalent), not an editor.

What do you actually want to make?

What is a Login? The word itself is very vague. Do you want a login form? Or a system that let's you login using different auth-providers (facebook, google, stackexchange,...) or is it a UserSession handler? OR should it be a huge piece of code that does all that and some more all in one method?

Validation is hard!

In SOLID, the first character stands for Single Responsibility. Every little class should be small. Not small as in kb, but small as in 'what it does'. It shouldn't do multiple things at the same time. Do you need Validation? Then create a Validator class:

class Validator
{
    private $rules;

    public function addRule($rule)
    {
        $this->rules[] = $regex;
    }

    public function validate($value)
    {
        foreach ($this->rules as $rule) {
            if ( 1 !== preg_match($rule, $value) ) {
                return false;
            }
        }
        return true;
    }
}

We can use it as follows:

$passwordValidator = new Validator();

//passwords shouldn't restrict the usage of certain characters.
//neither should they use a max-length.
$passwordValidator->addRule('/[.]{5,40}$/i'); //is it long enough?
$passwordValidator->addRule('[0-9]+'); //at least one digit
$passwordValidator->addRule('[A-Z]+'); //at least one uppercase
$passwordValidator->addRule('[a-z]+'); //at least one lowercase

//let's validate a given input
$passwordValidator->validate($_POST['password']); //boolean

Later on you could then extend your validator to accept a meta-syntax instead of raw regex. Laravel has some nice ones.

Having a valdiator object gives us the great advantage of removing those pointless isEmailValid checks in your classes and use our validation objects instead.

Now we have our validator, we can go on with our Authentication class. An Authentication class should give us the ability to login a user, check if the credentials are correct and also retrieve the currently logged in user. It would look something like this:

class Auth
{

    /**
     * @param  String $username The username
     * @param  String $password the password
     * @return boolean          succesfull login?
     */
    public function login($username, $password)

    /**
     * Log the user out
     * @return void
     */
    public function logout()

    /**
     * @return boolean is the user logged in?
     */
    public function check()

    /**
     * @return User the currently logged in user
     * @return null IF !check()
     */
    public function user()

}

Our Auth class however needs some other stuff to work. It needs a Session handler and a UserRepository - something that fetches user objects for us. We thus add a __construct that asks for those objects:

class Auth
{
    public function __construct(SessionHandler $session, UserRepository $user)
}

See How I typehint? I tell PHP what I need. This way I am sure that I am getting the correct objects. But what are they? Let's define them in an interface:

interface SessionHandler
{
    public function get($key);
    public function set($key, $value);
    public function has($key);
    public function unset($key);
}

interface UserRepository
{
    public function getBy($key, $value);
}

Now we have our interfaces, we know how to use our objects passed into the Auth class. So let's finish it:

class Auth
{
    private $session;
    private $user;

    public function __construct(SessionHandler $session, UserRepository $user)
    {
        $this->session = $session;
        $this->user = $user;
    }

    /**
     * @param  String $username The username
     * @param  String $password the password
     * @return boolean          succesfull login?
     */
    public function login($username, $password)
    {
        $user = $this->user->getBy('username', $username);

        if (!isset($user)) {
            return false;
        }

        if (!password_verify($password, $user->password)) {
            return false;
        }

        $this->session->set('auth_user_id', $user->id);

        return true;
    }

    /**
     * Log the user out
     * @return void
     */
    public function logout()
    {
        $this->setssion->unset('auth_user_id');
    }

    /**
     * @return boolean is the user logged in?
     */
    public function check()
    {
        return $this->session->has('auth_user_id');
    }

    /**
     * @return User the currently logged in user
     * @return null IF !check()
     */
    public function user()
    {
        $this->user->getBy('id', $this->session->get('auth_user_id'));
    }

}

As you can see, our Authentication class now gives us some nice methods to work with. Gives us access to our user object and is completely loosely coupled from the rest of our application. What we now need to do is write a UserRepository that handles all the PDO stuff of our users. And a SessionHandler that acts as an Adapter/Wrapper for our $_SESSION (or maybe replace $_SESSION with cookie + databaserows).

Our UserRepository will have a dependency on a PDO object. So as it in the construct. And because a UserRepository handles Users only it can have a property $table set to 'users'.

I didn't write the rest of the code because I think you are perfectly capable of doing it your self. Just remember that you are writing little pieces of code that will eventually help you in solving a problem.

Good luck!