Session Control

Question

I am implementing my first app using MVC.

I would like to know if the approach I thought for the session system is a good and scalable one.

Well, basically a session is stored in my MySQL database and it is represented as a Session Model object. I also created a SessionDataMapper that makes the connection between the model layer and the database layer (is what I just said right?) for the Session object.

class Session extends Model{
    private $id;
    private $timeCreated;
    private $timeUpdated;
    private $userId;
    private $ip;


    public function __construct($sessionData){
        ... some initialization (hidden for better reading)
    }

    ... some getter methods (hidden for better code reading)

}

_

class SessionDataMapper extends DataMapper{
    private $connection;
    private static $sessionTimeout = 2*60*60; //2 hours

    public function __construct(){
        $this->connection = (new \Utils\DatabaseConnection())->getConnection();
    }

    /*
    * @returns \Models\Session or null
    */
    public function create($sessionId){
        //On date retrieve deletes the expired sessions
        $query = $this->connection->prepare("BEGIN TRANSACTION;
            DELETE FROM sessions WHERE time_updated <= :expirationTime;
            SELECT * FROM sessions WHERE session_id = :sessionId;
            COMMIT;"); 

        $query->bindParam(":expirationTime", time() - self::$sessionTimeout, PDO::PARAM_STR);
        $query->bindParam(":sessionId", $sessionId, PDO::PARAM_STR);

        if($query->execute()){
            $sessionData = $query->fetch(PDO::FETCH_OBJ); 

            if($sessionData != false){
                return new Session(array(
                        "sessionId" => $sessionData->session_id,
                        "timeCreated" => $sessionData->time_created,
                        "timeUpdated" => $sessionData->time_updated,
                        "userId" => $sessionData->user_id,
                        "ip" => $sessionData->ip
                    ));
            }else{
                return null;
            }
        }else{
            return null;
        }
    }

    /*
    * @returns boolean
    */
    public function save(Session $session){
    }

    /*
    * @returns boolean
    */
    public function insert(Session $session){
        $query = $this->connection->prepare("INSERT INTO sessions (session_id, time_created, time_updated, user_id, ip) VALUES
            (:sessionId, :timeCreated, :timeUpdated, :userId, :ip);");          
        $query->bindParam(":sessionId", $session->getId(), PDO::PARAM_STR);
        $query->bindParam(":timeCreated", $session->getTimeCreated(), PDO::PARAM_STR);
        $query->bindParam(":timeUpdated", $session->getTimeUpdated(), PDO::PARAM_STR);
        $query->bindParam(":userId", $session->getUserId(), PDO::PARAM_STR);
        $query->bindParam(":ip", $session->getIp(), PDO::PARAM_STR);

        return $query->execute();
    }       

    /*
    * @returns void
    */
    public function delete(Session $session){

    }

}

I would like to focus on my SessionDataMapper::create() method. As you can see I added a bit logic to make it remove the expired sessions when a session is retrieved. Is this a good approach? If not what would you recommend?

Also if you have any comments to the rest of the code, they are appreciated.

Thanks!

Answer 1

As you can see I added a bit logic to make it remove the expired sessions when a session is retrieved. Is this a good approach? If not what would you recommend?

From a design standpoint, it doesn't seem ideal; create should create something, not clean something up. It makes the method unnecessarily complex and the code more difficult to maintain. (if there is a problem in the session maintenance, will you remember to look in this method, which doesn't sound as if it would be relevant?)

I would at least separate the cleanup code into its own method which is then called.

Personally, I would use PHPs inbuilt session handler to take care of this though (see eg here). It's the same idea - old sessions are cleaned when creating new sessions - but the code can be properly separated, and the garbage collection is more sophisticated and configurable.

Other than that:

• returning null as error will make debugging very difficult, and also result in a lot of difficult to read if (x == null) checks. Use exceptions instead.
• I would not create the database connection here, but pass it to either the constructor or each method instead, to decouple the code.
• time() - self::$sessionTimeout: Is this supposed to be a +?
• I would extract the session timeout to a separate config file, it makes it easier to later reconfigure various aspects if they are all in one place.
• think about switching your ifs around and possibly returning early to reduce nesting and make it easier to read.
• is there a reason that the queries are in a transaction? If not, I would separate them.
• Your documentation could use some work. Side effects such as session maintenance on create should eg definitely be mentioned. It should also explain in which cases null is returned.

Answer 2

You are likely doing yourself a great disservice if you are not extending upon PHP's SessionHandler object (or implementing against SessionHandlerInterface) if trying to make an object-oriented, custom session handler. As it stands right now, your method names do not really map to the expected method names to be used for session_set_save_handler(), which makes it unclear how you are expecting this class to interact with those built-in PHP hooks used by session set save handler.

---

With regard to your database model, it seems odd to base any session deletion off of time_updated value. I would consider explicitly storing expiry datetime in your session object, rather than trying to back-calculate against last time session was updated.

I don't understand why you would tie session instantiation and session data garbage collection together as you have done.

---

You show nothing that indicates you can handle session ID forwarding for cases where session ID regeneration has taken place. When request comes in asynchronously after a session id regeneration event, can you gracefully forward it?

---

It is not clear to me that you fully understand the difference between garbage collection of session data (i.e. bulk deletion of non-active records) and destroying session data. Your implementation is a little naive in terms of handling session destruction and garbage collection. To implement sessions in a secure and resilient way, one should strongly consider thinking of session destruction as nothing more than setting session data as invalid. That session data might need to remain (i.e. not be garbage-collected) in order to handle things such as race conditions from asynchronous requests, such that you can maintain good user experience in your application. Most mature implementations require setting of timestamp-based controls around data validity and a separate process for data deletion (garbage collection).

For me, the database datamodel should include the following at a minimum:

• session_id - unique session identifier as generated by PHP
• valid_until - datetime expressing time after which session date is to be considered invalid. By default, this timestamp would be updated after every session access to be current_timestamp + session_timeout but could be set to current_timestamp for cases where data is being invalidated (via session destruction or session ID regeneration).
• gc_after - datetime expressing time after which a record should be eligible for garbage collection (as it is no longer needed in the system at all). By default this would also change on session writes like valid_until but would differ from the value of that field once session data has been marked as invalid, at which point it's value would be valid_until + session_gc_delay (which may be a matter of a few seconds to allow the record to persist for session id forwarding).
• replaced_by_id - session ID used for forwarding old session to new session ID for example if the was a session ID regeneration event.
• data - the session data itself

---

In an overall sense, session management in PHP is a bit painful. I have actually spent a good amount of time in this space and have created an open source PHP session library While you are free to use this library, I am hoping that at least some of the "Readme" information on the linked library page can point you in the right direction to literature related to proper session management in PHP.