huge icon indicating copy to clipboard operation
huge copied to clipboard

[broken] add session concurrency switch, allow users being logged in from multiple devices (or not)

Open panique opened this issue 8 years ago • 17 comments

After implementing https://github.com/panique/huge/pull/693 a user cannot be logged in from two different browsers / devices at the same time. This is a security improvement, but surely also a deal-breaker for lots of applications.

There definitely should be a switch that allows the developer to decide if to use or not use this feature.

panique avatar Sep 16 '15 21:09 panique

Would very much welcome this feature, and indeed with the -option- to explicitly allow or disallow.

What I would also like, however, is to be able to override a site-wide default setting -per user-.

JFJanssen avatar Sep 18 '15 09:09 JFJanssen

I think that this issue shouldn't be closed. It's marked as TODO but the switch isn't implemented yet ;)

slaveek avatar Dec 01 '15 21:12 slaveek

Okay good point, but this feature does not really work anyway... I'll come back to this

panique avatar Dec 01 '15 22:12 panique

For this project it might be right that this is just a all on or all off feature because the most sites don't need to restrict user from accessing with multiple devices simultaneously. And the systems that do need to restrict that should be doing it on a blanket scale. Users generally should NOT control the level of security for a site.

What would be practical to do in huge is doing this restriction selectively based on the user_account_type as that might be something that could be generalized. However even that gets really project specific and implementing it would mean that developers must go and modify parts of core for their select user groups. This is probably not desirable as it makes it so that you can't make huge start working right off the bat.

So I think leaving the concurrency restriction as is will be fine for most use cases and those that want the user_account_type/role base setting can easily implement it in the Auth::checkSessionConcurrency(); by checking the logged in user's type. The other case of individual user defined, well, like I said not very good for security.

geozak avatar Dec 02 '15 00:12 geozak

I've tried to make it works some weeks ago, but nothing solve this.

I've tried to add in the database new table with device_id (random code as other content, user_id+device_id make it unique, and each user can create more than 1 record with different device, and so different tokens, remember me, etc.)

But no working solution until now. I'll retry in next days. Write here only to know if others are working on this / if someone is nearer to a solution or want to propose some schema (example: avoid new tables, or any other advice).

Thank you.

ghost avatar Jan 13 '16 18:01 ghost

Same here, also doesn't work for me.

The feature was implemented via https://github.com/panique/huge/pull/693, in case somebody want to the see the exact code. I've just reopened the ticket, so if somebody has a solution, feel free to commit! Thanks guys!

panique avatar Jan 13 '16 21:01 panique

A small step forward (perhaps too rudimentary?). Now I can login with multiple device with no problem. I can also login with remember me.

Problem: remember me works only with the last device that used the function. So, user should choose where to stay always on. This because we need database changes to manage multiple remember me, as stated above.

Edited files:

core/Controller.php Line 24 delete (remove the check):

        Auth::checkSessionConcurrency();

core/Auth.php Line 68 delete (remove the function, no need it anymore):

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

core/Session.php Line 105 delete (remove the function, no need it anymore):

    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 on_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;
    }

Updated 16 Jan 2016. It works, remember me works with 1 device only (the last that login with the function). And multiple devices update the only session_id value on users row.

I'll try with new table called "sessions" to manage each device data like remember me and session id.

ghost avatar Jan 13 '16 23:01 ghost

Maybe we are... It seems work but need some time to test it better. Here the edits:

model/LoginModel.php Line 208 update logout() with this (added call to cookie to retrieve $token and delete cookie+db on logout):

    public static function logout($cookie = NULL)
    {
        // do we have a cookie ?
        if (!$cookie || count (explode(':', $cookie)) !== 3){
            $token = NULL;
        }
        else {
            // check cookie's contents, check if cookie contents belong together or token is empty
            list ($cookie_user_id, $cookie_token, $cookie_hash) = explode(':', $cookie);

            // decrypt user id
            $cookie_user_id = Encryption::decrypt($cookie_user_id);

            if ($cookie_hash !== hash('sha256', $cookie_user_id . ':' . $cookie_token) OR empty($cookie_token) OR empty($cookie_user_id)) {
                $token = NULL;
            }
            else {
                $token = $cookie_token;
            }
        }

        $user_id = Session::get('user_id');

        self::deleteCookie($user_id, $token);

        $database = DatabaseFactory::getFactory()->getConnection();

        $sql = "DELETE FROM tokens WHERE token_remember_me = :token_remember_me AND user_id = :user_id LIMIT 1";
        $sth = $database->prepare($sql);
        $sth->execute(array(':token_remember_me' => $token, ':user_id' => $user_id));

        Session::destroy();
    }

Line 314 update setRememberMeInDatabaseAndCookie() with this (work on tokens table with insert, not users updates):

    public static function setRememberMeInDatabaseAndCookie($user_id)
    {
        $database = DatabaseFactory::getFactory()->getConnection();

        // generate 64 char random string
        $random_token_string = hash('sha256', mt_rand());

        // write that token into database
        $sql = "INSERT INTO tokens (user_id, token_remember_me) VALUES (:user_id, :token_remember_me)";
        $sth = $database->prepare($sql);
        $sth->execute(array(':token_remember_me' => $random_token_string, ':user_id' => $user_id));

        // generate cookie string that consists of user id, random string and combined hash of both
        // never expose the original user id, instead, encrypt it.
        $cookie_string_first_part = Encryption::encrypt($user_id) . ':' . $random_token_string;
        $cookie_string_hash       = hash('sha256', $user_id . ':' . $random_token_string);
        $cookie_string            = $cookie_string_first_part . ':' . $cookie_string_hash;

        // set cookie, and make it available only for the domain created on (to avoid XSS attacks, where the
        // attacker could steal your remember-me cookie string and would login itself).
        // If you are using HTTPS, then you should set the "secure" flag (the second one from right) to true, too.
        // @see http://www.php.net/manual/en/function.setcookie.php
        setcookie('remember_me', $cookie_string, time() + Config::get('COOKIE_RUNTIME'), Config::get('COOKIE_PATH'),
            Config::get('COOKIE_DOMAIN'), Config::get('COOKIE_SECURE'), Config::get('COOKIE_HTTP'));
    }

Line 348 update deleteCookie() with this (work on tokens table and delete, not update NULL on users):
    public static function deleteCookie($user_id = null, $token = null)
    {
        // is $user_id was set, then clear remember_me token in database
        if(isset($user_id)){

            $database = DatabaseFactory::getFactory()->getConnection();

            $sql = "DELETE FROM tokens WHERE token_remember_me = :token_remember_me AND user_id = :user_id LIMIT 1";
            $sth = $database->prepare($sql);
            $sth->execute(array(':token_remember_me' => $token, ':user_id' => $user_id));
        }

        // delete remember_me cookie in browser
        setcookie('remember_me', false, time() - (3600 * 24 * 3650), Config::get('COOKIE_PATH'),
            Config::get('COOKIE_DOMAIN'), Config::get('COOKIE_SECURE'), Config::get('COOKIE_HTTP'));
    }

model/UserModel.php Line 325 update getUserDataByUserIdAndToken() with this (work with inner join to check token and retrieve all required data from users):

    public static function getUserDataByUserIdAndToken($user_id, $token)
    {
        $database = DatabaseFactory::getFactory()->getConnection();

        // get real token from database (and all other data)
        $query = $database->prepare("SELECT users.user_id, users.user_name, users.user_email, users.user_password_hash, users.user_active,
                                          users.user_account_type, users.user_has_avatar, users.user_failed_logins, users.user_last_failed_login
                                     FROM users
                                     INNER JOIN tokens
                                     ON users.user_id = tokens.user_id
                                     WHERE users.user_id = :user_id
                                       AND tokens.token_remember_me = :token_remember_me
                                       AND tokens.token_remember_men IS NOT NULL
                                       AND users.user_provider_type = :provider_type LIMIT 1");
        $query->execute(array(':user_id' => $user_id, ':token_remember_me' => $token, ':provider_type' => 'DEFAULT'));

        // return one row (we only have one result or nothing)
        return $query->fetch();
    }

controller/LoginController.php Line 40 and line 68 edit (now logout need cookie, we should not delete all remember me of user, only the current one):

            LoginModel::logout();

With this:

            LoginModel::logout(Request::cookie('remember_me'));

Database:

CREATE TABLE IF NOT EXISTS `tokens` (
  `token_id` int(11) NOT NULL AUTO_INCREMENT COMMENT 'auto incrementing token_id, unique index',
  `user_id` int(11) NOT NULL COMMENT 'user_id of the user',
  `token_remember_me` varchar(64) COLLATE utf8_unicode_ci DEFAULT NULL COMMENT 'remember-me cookie token',
  PRIMARY KEY (`token_id`)
) ENGINE=InnoDB  DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci ;

As before, now we should be able to DROP remember me column from users.

Probably we should also remove old remember me from database if it isn't used anymore and users don't call the logout func.

Let me know, night!

ghost avatar Jan 14 '16 00:01 ghost

How you are going to manage the session concurrency highly depends on the application and context. Most of the applications(i.e. Gamil) track concurrent sessions and notify the user whenever there is another logged in user, and ask if they want to sign out from the other systems. If they do, then log them out, and clear the session_id.

The feature implemented in huge prevents session concurrency for all users, and it works very well for me.

Now, there are three options, the first is to to let everyone login with multiple devices from multiple locations. You can do this by just comment the code Auth::checkSessionConcurrency(); in Controller.php. Easy and simple.

the second is to notify the user when there is a concurrent session, and ask them if they want to logout or not, instead of enforcing the logout method. This can be done in checkSessionConcurrency()method in Auth.php. Here you aren't going to track all the logged in sessions, you will just notify the user if the same account is already logged in somewhere else.

the last one is to track all the current concurrent sessions of an account, and store them somewhere in the database. Now, you can notify the user whenever there is a concurrent session, limit the number of concurrent sessions, user can view a list of when and where they logged in and so on. But, this requires someone to do it carefully.

Hope this helps

OmarElgabry avatar Jan 14 '16 01:01 OmarElgabry

The first one work, miss only remember me for all devices, because it consider always the unique value on user row.

About 2/3, how to check who's the real user? If the option is available in the website pages, who hack the login can remove real user session. Not so good.

Gmail system it's mistake for me. If someone log in your account, can read the mail and delete the received notification. But, it's not our problem, our access not involve user personal email.

So can be better something like: Store on each login session, remember token (if user want it), ip/browser info/other, and after login send mail to the user with a link to delete session if it comes by no known device.

Or something like facebook, I think better because avoid the login until the real user, by mail in that case, give his ok: After correct login with new device, show step2 on login that request "mail generated PIN code", something like 4/6/8 length numeric PIN code valid for X minutes. Cons: higher use of send mail system, and if mail system stop working, users can't login...

ghost avatar Jan 14 '16 10:01 ghost

I've not completely understand the users column "user_provider_type". Found only one discussione where seems that it help to login also with facebook.

I'm trying to create new table to manage in better way the user concurrent login (as before). I need to know if could be better to move this row in the new table as other data like session_id and remember_me_token, or it is general and should be the same for all devices?

CREATE TABLE IF NOT EXISTS `devices` (
  `device_id` int(11) NOT NULL AUTO_INCREMENT COMMENT 'auto incrementing device_id, unique index',
  `user_id` int(11) NOT NULL COMMENT 'user_id of the logged in user',
  `device_session_id` varchar(64) COLLATE utf8_unicode_ci DEFAULT NULL COMMENT 'remember-me cookie token, collect old users session_id',
  `device_remember_me` varchar(64) COLLATE utf8_unicode_ci DEFAULT NULL COMMENT 'remember-me hash, collect old users user_remember_me_token',
  `device_token` varchar(64) COLLATE utf8_unicode_ci DEFAULT NULL COMMENT 'device token, random as remember me token ',
  `device_hash` varchar(64) COLLATE utf8_unicode_ci DEFAULT NULL COMMENT 'device hash, collect result of sha512 $token.SERVER[HTTP_USER_AGENT]',
  `device_provider_type` text COLLATE utf8_unicode_ci,
) ENGINE=InnoDB  DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci ;

ghost avatar Jan 16 '16 12:01 ghost

@Vizzielli Sorry, user_provider_type is a relict from older times (2.x version) where the script was able to let users register / log in via third party services like Facebook, Linkedin, Google+, Twitter, Github etc. ! user_provider_type simply said via which service the user had registered, something like "FACEBOOK" or "TWITTER" or "DEFAULT" when the user registered simply via native email/username + password.

As this got more and more complex, and additionally Facebook renewed their API every 6 months, it became super-expensive (from workload perspective) to keep this project compatible. This is why the feature was removed, but the provider-type is still in use for no really good reason :)

The userprovider type is defined in the config like this:

Session::set('user_provider_type', 'DEFAULT');

(Regarding your other question: sorry, don't know!)

panique avatar Jan 16 '16 13:01 panique

Nice, I left the question before saying that it did not serve to anything to avoid saying inaccuracies like the other day.

I'll try also to remove this with this proposal. Thanks!

Update (sorry, don't know how to propose edited file directly on git): Remove user_provider_type

model/LoginModel.php
Line 242 remove:
        Session::set('user_provider_type', 'DEFAULT');
model/PasswordResetModel.php
Line 73 remove:
    AND user_provider_type = :provider_type
Line 77 remove:
    , ':provider_type' => 'DEFAULT'
Line 135 remove:
        AND user_provider_type = :user_provider_type
Line 138 remove:
        , ':user_provider_type' => 'DEFAULT'
Line 180 remove:
    AND user_provider_type = :user_provider_type
Line 184 remove:
    , ':user_provider_type' => 'DEFAULT'
Line 271 remove:
    AND user_provider_type = :user_provider_type
Line 274 remove:
    , ':user_provider_type' => 'DEFAULT'
model/RegistrationModel.php
Line 208 remove:
    , user_provider_type
Line 209 remove:
    , :user_provider_type
Line 215 remove:
        , ':user_provider_type' => 'DEFAULT'
model/UserModel.php
Line 92 remove:
    AND user_provider_type = :provider_type
Line 93 remove:
    , ':provider_type' => 'DEFAULT'
Line 280 remove:
        AND user_provider_type = :provider_type
Line 285 remove:
        , ':provider_type' => 'DEFAULT'
Line 305 remove:
        AND user_provider_type = :provider_type
Line 310 remove:
        , ':provider_type' => 'DEFAULT'
Line 335 remove:
        AND user_provider_type = :provider_type 
Line 336 remove:
        , ':provider_type' => 'DEFAULT'

ghost avatar Jan 16 '16 14:01 ghost

What is the default for session concurrency? I am considering using HUGE for front end authentication and authorization, however the app I am developing should not/cannot be executed from multiple logins.

Thanks,

Ray

rayj00 avatar Mar 15 '16 13:03 rayj00

Session concurrency works by default. Try to login in 2 different browsers and see how it works.

slaveek avatar Mar 15 '16 18:03 slaveek

Ok... So...

What is the solution?

mauro-balades avatar Jan 22 '22 14:01 mauro-balades

Hi, i think this project is not used anymore, as other frameworks do everything much much better now

panique avatar Jan 22 '22 18:01 panique