panique / huge

Simple user-authentication solution, embedded into a small framework.
2.14k stars 789 forks source link

Session validation improvement #885

Open kristuff opened 4 years ago

kristuff commented 4 years ago

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) :

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