Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Session validation improvement #889

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

kristuff
Copy link

@kristuff kristuff commented Jun 6, 2020

This is the change proposed here #885
By the way, fixed Config and Environment tests on travis with recent PHP versions and made phpunit verbose

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();
            }
        }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant