PHP login/signup validation class

Question

I am working on a blog with PHP, specially in order to practice OOP.

Since the blogging system will handle users, i thought that a Validation class would be a good idea in order to not repeat code for both login and signup validation.

So far it works, but as you may notice, i'm using static methods from other class here, the UsersTable class, and i feel this is a "code smell".

validation.php:

class Validation
{
    private static $emailValidationRegex = '/^[^\s@]+@[^\s@]+\.[^\s@]+/';

    public static function fieldIsEmpty($field)
    {
        return empty($field);
    }

    public static function validateEmail($email)
    {
        return preg_match(self::$emailValidationRegex, $email);
    }

    public static function usernameExists($username)
    {
        if (UsersTable::getUserByUsername($username))
        {
            return true;
        }

        return false;
    }

    public static function emailExists($email)
    {
        if (UsersTable::getUserByEmail($email))
        {
            return true;
        }

        return false;
    }

    public static function redirectUser($validUser)
    {
        if ($validUser)
        {
            header('Location: ../index.php');
        }
        else
        {
            header('Location: ../login-page.php');
        }
    }

    public static function loginValidation($usernameOrEmail, $password)
    {
        $validUser = false;

        if (!self::fieldIsEmpty($usernameOrEmail) && !self::fieldIsEmpty($password))
        {
            $user = null;

            if (self::validateEmail($usernameOrEmail))
            {
                if (self::emailExists($usernameOrEmail))
                {
                    $user = UsersTable::getUserByEmail($usernameOrEmail);
                }
            }
            else
            {
                if (self::usernameExists($usernameOrEmail))
                {
                    $user = UsersTable::getUserByUsername($usernameOrEmail);
                }
            }

            if ($user !== null)
            {
                if (password_verify($password, $user['password']))
                {
                    $validUser = true;
                }
            }
        }

        self::redirectUser($validUser);
    }

    public static function signupValidation($email, $username, $password, 
                                            $passwordConfirmation, $name)
    {
        echo 'Working so far!';
    }
}

users-table.php:

class UsersTable
{
    public static function getAllUsers()
    {
        $statement = "SELECT * FROM `users`";
        $query = Database::query($statement);

        if ($query)
        {
            return $query->fetchAll(PDO::FETCH_ASSOC);
        }
    }

    public static function getUserByID($id)
    {
        $statement = "SELECT * FROM `users` WHERE `id` = '$id' "
                    . "LIMIT 1";
        $query = Database::query($statement);

        if ($query)
        {
            return $query->fetch(PDO::FETCH_ASSOC);
        }
    }

    public static function getUserByEmail($email)
    {
        $statement = "SELECT * FROM `users` WHERE `email` = '$email' "
                    . "LIMIT 1";
        $query = Database::query($statement);

        if ($query)
        {
            return $query->fetch(PDO::FETCH_ASSOC);
        }
    }

    public static function getUserByUsername($username)
    {
        $statement = "SELECT * FROM `users` WHERE `username` = '$username' "
                    . "LIMIT 1";
        $query = Database::query($statement);

        if ($query)
        {
            return $query->fetch(PDO::FETCH_ASSOC);
        }
    }

    public static function addUser($name, $email, $username, $password)
    {
        $hashedPassword = password_hash($password, PASSWORD_DEFAULT);

        $statement = "INSERT INTO `users` (`name`, `email`, `username`, "
                    . " `password`) VALUES ('$name', '$email', '$username', "
                    . "'$hashedPassword')";

        $query = Database::query($statement);
    }
}

login.php:

require_once('../config.php');
require_once('database.php');
require_once('users-table.php');
require_once('validation.php');

Database::connect();

Validation::loginValidation($_POST['username-email'], $_POST['password']);

Answer 1

It's a very good intention but rather bad implementation. The first and foremost problem is

SQL injection

In a code written in 2018 it's a shame to have such a blatant one. Your Database::query method must support two arguments, one is an SQL statement and one is an array with data to bind. You can see an example implementation here. So the code would be

    $statement = "SELECT * FROM `users` WHERE `email` = ? LIMIT 1";
    $query = Database::query($statement, [$email]);

Same goes for all other queries. insert for example:

$statement = "INSERT INTO `users` (`name`, `email`, `username`, `password`) 
              VALUES (?,?,?,?)";
Database::query($statement,[$name, $email, $username, $hashedPassword]);

Fixing this is the most important thing that needs to be done. Other issues are less important, more on the proper OOP structure and code optimization.

Validation class

1. function fieldIsEmpty($field) is questionable, for two reasons:
   • running empty() against a variable that is deliberately set is overkill
   • a dedicated function to duplicate the empty()'s functionality? It could make sense but still I am not sure if it worth
2. function validateEmail($email) - there is a built-in function, filter_var with a FILTER_VALIDATE_EMAIL flag which should be better than a homebrewed regexp.
3. function redirectUser($validUser) doesn't belong to validation. What if you will need to implement a command-line utility to create a user? Where it will be supposed to redirect? It should be another class responsible to HTTP interaction.
4. function loginValidation doesn't belong to validation class either. It does validate a user, so it should belong to either a User class or a UserValidation class, which may inherit from the Validation class.
5. Same goes for usernameExists() and emailExists()

6. Also, loginValidation() is too verbose and making way too much SQL queries. The followed code should be enough (although still doing one extra query)

   public static function loginValidation($usernameOrEmail, $password)
   {
       $user = UsersTable::getUserByEmail($usernameOrEmail);
       if (!$user)
           $user = UsersTable::getUserByUsername($usernameOrEmail);
       }
       return ($user && password_verify($password, $user['password']));
   }
   

   again: a function named somethingValidation() under no circumstances should redirect anyone anywhere. It should return a boolean value that could be used elsewhere.

7. There are a lot of unnecessary conditions. Most functions xould be rewritten to return the result right away:

   public static function usernameExists($username)
   {
       return (bool)UsersTable::getUserByUsername($username));
   }
   public static function getUserByEmail($email)
   {
       $statement = "SELECT * FROM `users` WHERE `email` = ? LIMIT 1";
       return Database::query($statement, [$email])->fetch(PDO::FETCH_ASSOC);
   }
   

   and so on

login.php

Given the inmformation above, its code should be more like

if (UserValidation::loginValidation($_POST['username-email'], $_POST['password'])) {
    HTTP::redirect("../index.php");
} else  {
    HTTP::redirect("../login-page.php");
}

Answer 2

i'm using static methods from other class here, the UsersTable class, and i feel this is a "code smell".

Some people hate static methods, others prize them for the micro-optimization... if your method is idempotent (or, in the case of a query, not dependent upon class state) you're not in code smell territory.

validation.php

private static $emailValidationRegex = '/^[^\s@]+@[^\s@]+\.[^\s@]+/';

If the string never changes, why assign it to a variable? (Consider using a class constant if you feel the string must be separate or - better yet - just use the quoted string directly in the single place that it is needed)

x@y.z may try signing up - I hope you require that users confirm their email address in order to complete account activation if a valid email address is a business requirement.

users-table.php

$statement = "SELECT * FROM `users`";

This may qualify as a microptimization quibble, (depends upon your DDL) however, I'd recommend explicitly specifying the columns in queries to avoid an extra check against the information schema.

$statement = "SELECT * FROM `users` WHERE `id` = '$id' "
            . "LIMIT 1";

Don't invite Little Bobby Tables into your database - use prepared statements.

login.php

require_once('../config.php');
require_once('database.php');
require_once('users-table.php');
require_once('validation.php');

No PSR-4 Autoloader ..? (You'll want one - save yourself a lot of hassle in the future)

Note: I may have missed a few bugs, however, I will say that you're on the right track in terms of producing consistent, formatted code - keep at it!