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 #885

Open
kristuff opened this issue May 1, 2020 · 0 comments
Open

Session validation improvement #885

kristuff opened this issue May 1, 2020 · 0 comments

Comments

@kristuff
Copy link

kristuff commented May 1, 2020

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

kristuff added a commit to kristuff/huge that referenced this issue Jun 6, 2020
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

No branches or pull requests

1 participant