Very basic PHP session handling

Question

I’ve got my first real PHP web application up and running. It is working as expected, but being my first time, I’m sure there is plenty of room for improvement.

Right now, there is one particular aspect that I’d like some Code Review love on: my session handling class.

I haven’t used any frameworks this time around, because I’m afraid I’d miss out on learning what is really going on and that I wouldn't appreciate what the framework is doing for me without doing it the ‘hard way’ first.

My app consists of about 8 pages that require the user to be logged in to view.

My goal with this was to create a chunk of code that I could plop onto the top of each page, that would take care of:

• starting the php session
• ensuring the user is logged in
• loading the user’s permissions
• if not logged in, send them to the login page
• after logging in, send them to the original destination page
• if they are already logged in when at the login page, bypass and go to the default page

The main areas I’m interested in getting feedback are:

• Are there any functional flaws I haven’t noticed yet?
• What problems am I going to run into doing it this way?
• Should this class be doing more?
• Should it be doing less?
• What aspects could be more efficient?
• Should I be passing my $db object to the $session object?

I’m looking forward to getting feedback and learning from the wisdom of the community.

Thanks!

---

EDIT: I'm aware that passwords should never be stored in plain text. I'll be fixing that soon.

---

There are 3 different ‘types’ of pages:

• Login page - this is the only page accessible when a user is not logged in
• Logout page - logs the user out and redirects to login page
• Regular page - all other pages are this type. user must be logged in to view these pages (I’ll use dashboard.php as the example)

_classes.php

<?php

sessionHandler::$config['loginPage'] = "login.php";
sessionHandler::$config['logoutPage'] = "logout.php";
sessionHandler::$config['afterLoginPage'] = "dashboard.php";
sessionHandler::$config['afterLogoutPage'] = "login.php";
sessionHandler::$config['permissionsRefreshInterval'] = 300;  // seconds

class sessionHandler
{
    public static $config;

    function __construct($special = NULL)
    {
        session_start();

        if ($special == 'LOGOUT') {
            // This is the logout page, clear the session and
            // send the user to the afterLogout page

            session_destroy();   // clear session files on server
            $_SESSION = NULL;    // clear session variable for this session

            $this->sendToPage(sessionHandler::$config['afterLogoutPage']);
        }

        $userID = $_SESSION['userID'];

        if ($special == 'LOGIN' && $userID) {
            // user is already logged in, so bypass the login page
            $this->sendToPage(sessionHandler::$config['afterLoginPage']);
        }

        if (!$userID && ($special != 'LOGIN')) {
            // If there's no session (no $userID) AND this
            // isn't the login page, send them to the login page

            sessionHandler::sendToLoginPage();
        }

        $nextRefreshTime = sessionHandler::$config['permissionsRefreshInterval'] + $_SESSION['lastPermissionRefreshTime'];
        if (time() > $nextRefreshTime) {
            sessionHandler::loadUserPermissions($userID);
        }

    }

    function sendToPage($page)
    {
        header("Location: $page");
    }

    function sendToLoginPage()
    {
        // redirect user to login page.
        // pass along current page, so we can
        // redirect them after a successful login

        $sourcePage = basename($_SERVER['REQUEST_URI']); // #TODO: make this work with deeper urls example.com/path/file.php

        if ($sourcePage == sessionHandler::$config['logoutPage']) {
            // don't do this if they are coming from the logout page
            $sourcePage = NULL;
        } else {
            $sourcePage = '?d=' . urlencode($sourcePage);
        }

        $loginPage = sessionHandler::$config['loginPage'] . $sourcePage;

        $this->sendToPage($loginPage);
    }

    static function loadUserPermissions($userid)
    {
        if (!$userid) {
            return FALSE;
        } else {
            $query = "SELECT userID, firstName, lastName, email, phone, permissions FROM users WHERE userID = ? AND active = 1;";
            $user = $GLOBALS['db']->SQL_SINGLE($query, Array($userid));

            if ($user) {
                $_SESSION['userID'] = $user['userID'];
                $_SESSION['displayName'] = $user['firstName'] . ' ' . $user['lastName'];
                $_SESSION['userEmail'] = $user['email'];
                $_SESSION['userPhone'] = $user['phone'];

                // currently I'm just storing user permissions as a comma separated
                // list in the DB.  I'll make it more elegant in the future

                $permissions = explode(",", $user['permissions']);
                foreach ($permissions as $p) {
                    $_SESSION['permissions'][$p] = 1;
                }

                $_SESSION['lastPermissionRefreshTime'] = time();

                return TRUE;
            } else {
                return FALSE;
            }
        }
    }

    function p_check($permission, $echo = NULL)
    {
        // if user has the named permission, then echo the supplied HTML code
        if ($_SESSION['permissions'][$permission]) {
            echo $echo;
            return TRUE;
        } else {
            return FALSE;
        }
    }

    static function login($username, $password)
    {
        // validates user credentials and loads up user properties

        $user = $GLOBALS['db']->SQL_SINGLE("SELECT userID FROM users WHERE username = ? AND password = ? AND active = 1;", Array($username, $password));

        if ($user) {
            $_SESSION['sessionStartTime'] = time();
            sessionHandler::loadUserPermissions($user['userID']);
            return TRUE;

        } else {
            return FALSE;
        }
    }
}

login.php

<?PHP
require_once('_classes.php');
$db = new db_controller(); // use this globally for all DB work.
$session = new sessionHandler('LOGIN');
?>
<html>

<body>

<div>
    <div>
        <div>
            <img src="img/logo.png">
        </div>

        <h3>Welcome to myApp</h3>
        <p>Login in to see it in action!</p>

        <form id="loginForm">
            <input name="username">
            <input type="password" name="password">
            <button type="submit">Login</button>
        </form>
    </div>
</div>

<!-- scripts -->
<script src="js/jquery-2.1.1.js"></script>
<script src="js/bootstrap.min.js"></script>
<script>
    $('#loginForm').submit(function (e) {
        e.preventDefault();
        $.ajax({
            type: 'POST',
            url: '_functions.php?function=login',
            data: $('form').serialize(),
            dataType: 'JSON',
            success: function (data) {
                if (data['success']) {
                    // determine if the user has been redirected from another
                    // page prior to getting to the login page, if so, send them
                    // back to it instead of the default page
                    var urlParams = {};window.location.search.replace(/[?&]+([^=&]+)=([^&]*)/gi, function (str, key, value) {urlParams[key] = value;                    });
                    var redirectPage = decodeURIComponent(urlParams['d']);

                    if (redirectPage == 'undefined') {
                        // if there is not redirect parameter on the URL,
                        // then send them to the default page
                        redirectPage = data['redirectPage'];
                    }

                    window.location.replace(redirectPage);
                } else {
                    alert(data['error']);
                }
            }
        });
    });

</script>

</body>

</html>

logout.php

<?PHP
    require_once('_classes.php');
    $db = new db_controller();
    $session = new sessionHandler('LOGOUT');
?>

dashboard.php

<?PHP
require_once('_classes.php');
$db = new db_controller(); // use this globally for all DB work.
$session = new sessionHandler();
?>
<html>

<body>

<div>
    <nav role="navigation">
        <div class="sidebar-collapse">
            <ul>
                <li><a href="#"><i class="fa fa-user"></i> <span>Home</span><span class="fa arrow"></span></a>
                    <ul class="nav nav-second-level collapse">
                        <li><a href="dashboard.php">My Dashboard</a></li>
                        <li><a href="record-multiple.php">Record Data</a></li>
                    </ul>
                </li>

                <?php $session->p_check('maintenance', '
                    <li>
                        <a href="#"><i class="fa fa-wrench"></i> <span class="nav-label">Maintenance</span><span class="fa arrow"></span></a>
                        <ul class="nav nav-second-level collapse">
                            <li><a href="maintenance.php?filter=overdue">Overdue <span>' . $statusCount['overdue'] . '</span></a></li>
                            <li><a href="maintenance.php?filter=due-soon">Due Soon <span>' . $statusCount['due soon'] . '</span></a></li>
                            <li><a href="maintenance.php?filter=good">Good <span>' . $statusCount['good'] . '</span></a></li>
                            <li><a href="maintenance.php?filter=no-history">No History <span>' . $statusCount['no history'] . '</span></a></li>
                            <li><a href="maintenance.php">All Equipment <span>' . $statusCount['all'] . '</span></a></li>
                        </ul>
                    </li>
                '); ?>

                <?php $session->p_check('equipment', '
                    <li>
                        <a href="#"><i class="fa fa-truck"></i> <span class="nav-label">Equipment</span><span class="fa arrow"></span></a>
                        <ul class="nav nav-second-level collapse">
                            <li><a href="equipment.php">Equipment List</a></li>
                            <li><a href="maintenance-plans.php">Maintenance Plans</a></li>
                            <li><a href="maintenance-log.php">Maintenance Log</a></li>
                        </ul>
                    </li>
                '); ?>

                <li><a href="#"><i class="fa fa-sign-out"></i> <span>Exit</span><span class="fa arrow"></span></a>
                    <ul class="nav nav-second-level collapse">
                        <li><a href="logout.php" id="logOut">Log Out</a></li>
                    </ul>
                </li>
            </ul>

        </div>
    </nav>
    <div id="page-wrapper" class="gray-bg">

    <div class="row wrapper wrapper-content">
        <div class="col-lg-12">

            <!-- Main Page Content......-->
            <!-- Main Page Content......-->
            <!-- Main Page Content......-->
            <!-- Main Page Content......-->
            <!-- Main Page Content......-->

        </div>
    </div>

    </div>
</div>

</body>
</html>

Answer 1

If sendToPage() is meant to be the end your processing, I would add an exit() immediately after header("Location..."). This will prevent unexpected bugs/behaviors as you modify other parts of your code that the parser is still traversing if you don't exit.

You assume that the existence of $_SESSION['userID'] means that it's safe to grant restricted access. This isn't necessarily true. You app is vulnerable to Cross-Site Request Forgeries (CSRF), meaning a malicious site can exploit the fact that your user never logged out to gain restricted access on behalf of the user. The malicious site only needs to get the victim to make a request to your site without realizing it. Because browsers send cookies automatically, the request will be misinterpreted as a legitimate logged in session. All it takes is something like this on a malicious site:

<img src="//yoursite.com/restricted.php?do=evil">

When the browser tries to load the image, it will send the cookies to your site and resume the session the user never bothered to log out of. I wrote about how to fix this (using headers, which are not sent automatically) on a recent question.

Now consider these lines

$session = new sessionHandler('LOGIN');
$session = new sessionHandler('LOGOUT');
if ($special == 'LOGIN'...

Hard-coding string parameters in this way is not a good idea. It's too easy to make a mistake (typos), it takes longer to write code as you have more options (you always have to go look up what you intended to use), and it's difficult to maintain because if you wanted to change "LOGIN" to "login", you would have to sift through lots of code.

A better approach is to define a class constant, like so (this would be in sessionHandler class):

const TYPE_LOGIN = 1;
const TYPE_LOGOUT = 2;
...

Then to use it like so:

$session = new sessionHandler(sessionHandler::TYPE_LOGIN);
if ($special === self::TYPE_LOGIN...

Now if you want to make a change, you only need to change 1 location. By the way, use === to check for equality unless you have a specific reason to use the looser ==. The latter can lead to unexpected results and hard to find bugs if you're using it mindlessly.

When I read the following line

if (time() > $nextRefreshTime) {
    sessionHandler::loadUserPermissions($userID);
}

I'm not sure why there is no else branch. Unless I'm missing something, you're saying that the user should be granted permissions only if her session is still fresh. What if it's not? If you mean for the process to end there, I would be explicit about it.

The following is taken from p_check():

if ($_SESSION['permissions'][$permission]) {
    echo $echo;
    return TRUE;
} else {
    return FALSE;
}

It's not a good idea to echo from within your functions. It makes the functions very inflexible. The better design would be for the calling code to echo whatever it wants if p_check()===true. This way, the function can be useful even in situations for which echoing is inappropriate.

Closing thoughts:

I am particularly concerned by your login function, login($username,$password). Your SQL query suggests that your passwords are stored in clear-text (since you're selecting rows whose password matches the user's submission). This is not good; it makes your users really vulnerable to abuse should you lose control of their data. It's a big reason you hear stories about people being hacked on multiple accounts in a short while. Users tend to use the same passwords across services, so never store passwords in the clear. When a user registers, hash the password before saving it in the DB:

$password = password_hash($password,PASSWORD_DEFAULT);

This is a salted hash, meaning that if two users have the same password, it will be stored as different strings. It becomes really hard for attackers to do as much damage.

At login, first select the account with the supplied username. If one such account is found, you then verify the cleartext password supplied against the hash stored in the DB:

password_verify($cleartext, $db_hashed_pw); 

If true is returned, you know that $cleartext is the original password that was hashed and stored. Here is a great intro to password hashing.