Skip to content

Session validation improvement #885

@kristuff

Description

@kristuff

When a user is suspended using AdminModel::setAccountSuspensionAndDeletionStatus() that internally calls AdminModel::resetUserSession() method, the feedback message says "The selected user has been successfully kicked out of the system (by resetting this user's session)",

That's not really true. In facts, the suspended user is still able to access protected pages until its session expires or he logouts. (Then, he is not able to login anymore as expected)

There is no way to kick out the user instantanitly (strictly speaking). On the other hand, it's possible, with a minor change, to not wait its session expires.

The Session::isConcurrentSessionExists() method that checks for session concurrency could be changed to Session::isSessionBroken() and check two things (with only one database call) :

  • session concurrency exists
  • or sessionId does not exist anymore in database

This way, the suspended user is kicked out as soon he tries to access another page.

Actual method in Session class:

public static function isConcurrentSessionExists()
{
        $session_id = session_id();
        $userId     = Session::get('user_id');

        if (isset($userId) && isset($session_id)) {

            $database = DatabaseFactory::getFactory()->getConnection();
            $sql = "SELECT session_id FROM users WHERE user_id = :user_id LIMIT 1";

            $query = $database->prepare($sql);
            $query->execute(array(":user_id" => $userId));

            $result = $query->fetch();
            $userSessionId = !empty($result)? $result->session_id: null;

            return $session_id !== $userSessionId;
        }
        return false;
}

Proposed:

public static function isSessionBroken()
// change function name ^^^^^^^^^^^^^^^^ just to be coherent
{
        $session_id = session_id();
        $userId     = Session::get('user_id');

        if (isset($userId) && isset($session_id)) {

            $database = DatabaseFactory::getFactory()->getConnection();
            $sql = "SELECT session_id FROM users WHERE user_id = :user_id LIMIT 1";

            $query = $database->prepare($sql);
            $query->execute(array(":user_id" => $userId));

            $result = $query->fetch();
            $userSessionId = !empty($result)? $result->session_id: null;

            return empty($userSessionId) || $session_id !== $userSessionId;
// and add this  ^^^^^^^^^^^^^^^^^^^^^^^^^^
        }
        return false;
}

and don't forget to change function in Auth class

public static function checkSessionConcurrency()
{
        if(Session::userIsLoggedIn()){
      //    if(Session::isConcurrentSessionExists()){
            if(Session::isSessionBroken()){
                LoginModel::logout();
                Redirect::home();
                exit();
            }
        }
}

I made that change in another project, and could make a PR.
Regards

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions