Open leegarner opened 3 years ago
I think it is more around how the long term cookie (LTC) is being handled. It is using the token code for validation. The sessions will stay active as long as someone is browsing, you can have multiple sessions per users - glFusion is using the IP, Browser and Session ID to validate a user.
But, the LTC is only using the IP address and the LTC value - and it is pulling IP from the 'users' table, so there can only be one associated with a user.
I think the best fix is to stop using tokens for both LTC and admin session - instead maybe another table, that allows tracking IP, browser and cookie value - similar to what sessions already does.
I'm also wondering of the Token code could be reworked - it is a pain to set tokens for everything - it would be nice if this was more automatic - store a token in the session vars - then either embed the token on all POST forms or tack onto the end of the URL for GET forms. This would tie the token to the session, so you could have full multiple sessions per user support...
OK, that's interesting. I did notice the remote_ip in the user's table and I wonder what that's really used for. I see it's updated at login and removed at logout but doesn't seem to do anything else. The sessions table also has the remote IP.
I only found one instance of $_USER['remote_ip']
in glFusion 2 and the plugins I have installed, it's passed to PLG_privacyExport() and passed through to all the plugins' privacy_export functions... but not one of them actually uses it anyway. Getting the IP from the sessions table would make sense, or even from $_SERVER for that matter.
I definitely agree on the token idea. That has caused me (and others) a few headaches figuring out why I have to keep logging in, usually a cookie domain issue. With the token fixed in the session, maybe it could even be one of the template auto-set variables so any form template that needs it would get it, like site_url.
This much shorter function seems to work. The check for DEMO_MODE seems to be geared towards removing all sessions for the user unless in demo mode, but interestingly the only reference to setting $_CONF['cookie_session']
is to expire it so I'm not sure that was used in demo mode. Probably deprecated. I haven't tested everything but I think the worst side effect of this might be to invalidate other sessions' tokens or something.
I suppose we could also include the userid check in the deletion, but it doesn't seem necessary.
function SESS_endUserSession($userid)
{
global $_TABLES;
$db = Database::getInstance();
if (session_id()) {
try {
$db->conn->delete($_TABLES['sessions'], array('md5_sess_id' => session_id()));
} catch(Throwable $e) {
$db->dbError($e->getMessage(),'...err...');
}
session_unset();
session_destroy();
}
return 1;
}
If a user is logged into multiple browsers, when they log out of one it logs out all sessions. This may arguably be more secure but the online shopping and banking sites that I checked do not do this.
I can see the value if someone accidentally leaves a public computer without logging out, so maybe an option to "clear all other sessions" in the account prefs, and perhaps do that automatically when the password is changed.
I'll look at updating lib-sessions.php but want to get some feedback on the approach.