PHP Login System with persistent login

Question

I'm writing my first CMS (for personal use) and I'm trying to make it the most secure I can. This is my project structure:

.
+-- css
|   +-- images
|   +-- style.css
+-- includes
|   +-- config.php
|   +-- connection.php
|   +-- footer.php
|   +-- header.php
|   +-- session.php
|   +-- user.php
+-- dashboard.php
+-- login.php
+-- logout.php

I'm gonna write here what I have in each file:

config.php

<?php
    define('DB_HOST', 'localhost');
    define('DB_USER', 'root');
    define('DB_PASS', '');
    define('DB_NAME', 'cms');
?>

connection.php

<?php

include 'config.php';

class Connection{
    public function openConnection(){
        try{
            $db = new PDO('mysql:host='.DB_HOST.';dbname='.DB_NAME, DB_USER, DB_PASS);
            return $db;
        }catch (PDOException $e){
            return 'Database Error.';
        }
    }
}

?>

user.php

<?php

class User{

    private $userData;

    public function createUser($userId){
        $connection = new Connection;
        $db = $connection->openConnection();

        $stmt = $db->query("SELECT * FROM users WHERE user_id = '$userId'");

        $results = $stmt->fetch();
        $this->userData = $results;
    }

    public function getId(){
        return $this->userData['user_id'];
    }

    public function getUsername(){
        return $this->userData['user_username'];
    }
}

?>

session.php

<?php

include "includes/user.php";

class Session{
    public function createSession(){
        session_start();
    }

    public function logUser($userId){
        $_SESSION['userId'] = $userId;

        $selector = base64_encode(random_bytes(8));
        $token = bin2hex(random_bytes(32));

        $cookieValue = $selector.':'.base64_encode($token);
        $hashedToken = hash('sha256', $token);

        $timestamp = time() + (86400 * 14);

        setcookie('authToken', $cookieValue, $timestamp, NULL, NULL, NULL, true);

        $connection = new Connection;
        $db = $connection->openConnection();

        $stmt = $db->query("INSERT INTO logins (login_selector, login_token, login_userId, login_expires) VALUES ('$selector', '$hashedToken', '$userId', '$timestamp')");
    }

    public function relogUser($userId){
        $_SESSION['userId'] = $userId;
    }

    public function isLogged(){
        if(isset($_SESSION['userId'])){
            return true;
        }else{
            if(isset($_COOKIE['authToken'])){
                $connection = new Connection;
                $db = $connection->openConnection();

                list($selector, $token) = explode(':', $_COOKIE['authToken']);

                $stmt = $db->prepare('SELECT * FROM logins WHERE login_selector = :login_selector');
                $stmt->bindValue(':login_selector', $selector);

                $stmt->execute();
                $results = $stmt->fetch();

                if($results){
                    if(hash_equals($results['login_token'], hash('sha256', base64_decode($token)))){
                        $this->relogUser($results['login_userId']);
                    }else{
                        $this->logOut();
                        return false;
                    }
                }else{
                    return false;
                }
            }else{
                return false;
            }
        }
    }

    public function logOut(){
        $connection = new Connection;
        $db = $connection->openConnection();

        list($selector, $token) = explode(':', $_COOKIE['authToken']);

        $stmt = $db->prepare('DELETE FROM logins WHERE login_selector = :login_selector');
        $stmt->bindValue(':login_selector', $selector);

        $stmt->execute();

        $stmt = $db->prepare('DELETE FROM logins WHERE login_userId = :login_userId');
        $stmt->bindValue(':login_userId', $_SESSION['userId']);

        $stmt->execute();

        unset($_SESSION['userId']);
        setcookie('authToken', '', 1);
        unset($_COOKIE['authToken']);
    }

    public function getId(){
        return $_SESSION['userId'];
    }
}

?>

login.php

<?php
    include "includes/header.php";
    include "includes/connection.php";
    include "includes/session.php"; 

    $session = new Session;
    $session->createSession();

    if($session->isLogged()){
        header('Location: dashboard.php');
        exit();
    }

    $connection = new Connection;
    $db = $connection->openConnection();

    if(isset($_POST['username']) && isset($_POST['password'])){
        $username = $_POST['username'];
        $password = $_POST['password'];

        if(strlen($username) > 0 && strlen($password) > 0){
            $stmt = $db->prepare('SELECT * FROM users WHERE user_username = :username');
            $stmt->bindValue(':username', $username);

            $stmt->execute();
            $results = $stmt->fetch();

            if($results){
                $verify = password_verify($password, $results['user_password']);
                if($verify){
                    $session->logUser($results['user_id']);
                    header('Location: dashboard.php');
                }else{
                    //handle wrong password
                }
            }else{
                //handle no user found
            }
        }
    }
?>

<div class="wrapper">
    <div class="login">
        <form method="post" action="">
            <input type="text" name="username" placeholder="Username">
            <input type="password" name="password" placeholder="Password">

            <button>Log In</button>
        </form>
    </div>
</div>




<?php
    include "includes/footer.php";
?>

dashboard.php

<?php
    include "includes/header.php";
    include "includes/connection.php";
    include "includes/session.php";

    $session = new Session;
    $session->createSession();

    if(!$session->isLogged()){
        header('Location: login.php');
        exit();
    }

    $user = new User;
    $user->createUser($_SESSION['userId']);
?>

<div class="wrapper">
    <p>Secret page, testing!</p>
    <a href="logout.php">Log out</a>
</div>


<?php
    include "includes/footer.php";
?>

Ok, so a few points and things to clear:

• The header.php and footer.php is just the HTML body, head tags, etc.
• The users will be admins always, no roles whatsoever.
• It currently has nothing, just the login system so that's the focus point.

Having said that I want to point out that I have two major concerns about this.

---

1) The first one is the architecture of the code, the classes, etc.

I feel like there's a lot of unnecessary bits and inconsistencies in the code but I can't figure out a way to tidy it up.

When I first started to make it, I didn't use classes and didn't separate session and user files from the connection code so it was even worse than now but at least I'm progressing.

I want to use the MVC pattern to make it flexible, tidy, and easy to mantain.

What changes can I make to this?

---

2) The second one is the security of the code.

I'm striving to make this as secure as I can so I'm taking care of all the problems it could have but I'm not sure that I'm doing it right.

Right now I'm trying to prevent:

• SQL Injection, for that I'm using PDO with prepared statements.
• XSS I don't have that problem yet because the "MANAGEMENT" part isn't done yet but I have an idea of how to prevent it.

Also, I'm storing the password as hashes, and I'm using a persistent login technique I read from a blog, which uses a validator or token and a selector to prevent from:

• An attacker hijacking the cookie.
• Timing leaks (using hash_equals).
• User impersonation (if somehow an attacker manages to get the databases).

Is there anything I should change to make this more secure?

Answer 1

Ok, moving down the list...

• I would separate the content of the includes folder into 2 semantically named folders: includes and classes - or if you want to eventually turn this into an MVC, maybe views and classes would be more appropriate.
• It's not uncommon to put database credentials in constants, but truth is you should never have more than one connection to the database at a time, so if you're writing proper code, constants are not the way to go as PHP won't garbage collect them. I would suggest using an ini file and parsing it into an array with parse_ini_file.
• You Connection class is abusing OOP.. Your class is not constructing anything. It's a single static method that you didn't even bother to mark as static and which you're constructing with new. Use it as a static method.. public static function openConnection() then instead of using new to construct a class that has no constructor you can just do $db = Connection::openConnection()
• Don't return a string if your connection fails. It's just going to throw an error later attempting to call prepare method on a non-object or something like that. Just let PDO throw an error in your openConnection method. Don't bother catching the error unless you're going to try and fix it. By the way, don't you have to set the error mode with PDO::setAttribute to squeeze error messages out of PDO anyway. As is, I don't think your catch will ever be called.
• Your User class should be constructed in a constructor. Rename createUser to __construct, then instead of doing $user = new User(); $user->createUser($id); you can knock it out in one line: $user = new User($id); I feel like you should read this.
• Why are you storing all User properties in a single associative array? This is another example of how not to use OOP. If you were married to OOP you'd be arrested for spousal abuse. Each index in that array should be its own property.

There are a few other things that I could point out but they wouldn't really benefit you much until you get a slightly better grasp on OOP. You're doing great so far, don't give up, but read a little bit about OOP in PHP because you're missing some of the base concepts here.

Answer 2

As you are already using PDO, you should use prepare/execute logic to apply values to SQL statements. Currently they can be subject to an SQL injection.

Example:

Instead of

$stmt = $db->query("SELECT * FROM users WHERE user_id = '$userId'");

You should use this:

$stmt = $db->prepare("SELECT * FROM users WHERE user_id = ?");
$result = $stmt->execute([$UserId]);

Alternatively, use object-oriented query builders (Zend, Illuminate, etc), where it would look like this:

$result = DB::Select('*')->from('users')->where('user_id', $UserId);

Answer 3

You must protect against SQL injections. You said you know about that topic, so why are you using interpolated SQL statements at all? An SQL statement in the source code must never contain a ${var}, only :var and bound parameters.

Having some random bytes as the auth token is enough. There's no point in hashing random bytes.

The isLogged function must return something in every possible branch. One branch is missing a return true. You should not use nested if-else in that function, but rather just a sequence of if-return blocks.