Fri 01 May 2009 09:02:01 PM UTC, comment #15:
Fix commited in r1101.
|
Wed 29 Apr 2009 08:42:37 PM UTC, comment #14:
Also, looking at the manual page for session_cache_expiry it's clear that this needs to be called before session_start(), so it'd make more sense to just set a year's PHP session expiry in auth.php prior to the session_start and rely on the database expiration for earlier timeouts.
|
Wed 29 Apr 2009 08:38:45 PM UTC, comment #13:
Sorry, my mistake, I didn't realise session_cache_expire is handled in minutes instead of seconds, so you can ignore point 4 (also savannah seems to have tried to apply some mark-down, interpreting my asterisks as a signal for 'bold', so that's maybe not as clear as it should be).
So the "session_cache_expire($expiration);" line should be "session_cache_expire($expiration / 60);"
|
Wed 29 Apr 2009 08:30:20 PM UTC, comment #12:
Hi, almost there now; just a few of things:
1) The indentation is inconsistent, you're mixing tabs and spaces.
2) You never get $register out of the $_POST array, so this code will only work when auto registration of variables is switched on (and any reasonably secure PHP configuration should have this switched off).
3) You never set the session_cache_expiry for the non-remembered login, so the amount of time the user is logged in is inconsistent with the time set in the database. (This can be fixed by following point 5)
4) You set a different php session expiry time to the database expiry (36586400 is not the same as 6024*365) (also corrected by point 5).
5) You should avoid duplication of queries where possible; a better approach would just be to do something like:
if (isset($_POST['register'])) {
$expiration = 31536000; // 1 year
} else {
$expiration = 86400; // 1 day
}
$mdb2->query('INSERT INTO Scrobble_Sessions (username, sessionid, expires) VALUES ('
.$mdb2->quote($username, 'text').', '
.$mdb2->quote($session_id, 'text').', '
.$mdb2->quote(time()+$expiration, 'integer').')');
session_cache_expire($expiration);
This means that the query and the expiration times only need to be changed in one place if they need changing in the future and avoid the possibility for simple mistakes like setting the database and php session times differently.
|
Tue 28 Apr 2009 04:47:22 PM UTC, comment #11:
Pasted final patch.
Please apply.
(file #18044)
|
Tue 28 Apr 2009 04:11:04 PM UTC, comment #10:
Hi, the additional cookie is unnecessary, we already have the authentication information in the session; you should just be making the session cookie expiration longer (something like the session_cache_expire() function is probably what you're looking for). All the authentication checks are on the ID in the session, so the user would still get logged out when the session cookie expired.
|
Tue 28 Apr 2009 03:41:33 PM UTC, comment #9:
Fixed
(file #18041)
|
Tue 28 Apr 2009 03:29:33 PM UTC, comment #8:
Hi, could you fix your patch so that it doesn't change the indentation of the whole file; it makes it very difficult to see what you've actually changed.
|
Tue 28 Apr 2009 03:07:17 PM UTC, comment #7:
wrong patch, working is 26264_3 :/
(file #18039)
|
Tue 28 Apr 2009 03:03:01 PM UTC, comment #6:
Added new patch. It works.
(file #18038)
|
Tue 28 Apr 2009 12:26:50 PM UTC, comment #5:
Okay,
so i'll extend that.
Patch will be submited later.
|
Tue 28 Apr 2009 12:25:11 PM UTC, comment #4:
Unless you extend the expiration time of the cookie it will be automatically expired by the browser. Without the cookie we don't know what the user's authentication token is, so they're no longer authenticated.
|
Tue 28 Apr 2009 12:20:12 PM UTC, comment #3:
Okay - the time wasn't calculated properly - my bad.
Session should be set for 1 month.
Unless you have other idea?
I was thinking about setting cookie and checking at startup is that cookie set but i've decided to use Scrobble_Session expiry date.
If the seession will live for about 1 mont or longer (1 year) you will have not to login.
Right?
|
Tue 28 Apr 2009 12:15:22 PM UTC, comment #2:
Hi, I don't think your patch will really make any difference; all it does it change the database expiry time from 7 days to 9.314 days (why this amount of time?) when the user selects the "remember me" option. The database expiry time isn't really the main issue here (and such a small change in it seems meaningless).
What really needs to be changed is the cookie expiration time.
|
Tue 28 Apr 2009 10:16:51 AM UTC, comment #1:
Fixed,
here's a patch.
(file #18034)
|
Tue 21 Apr 2009 04:12:35 AM UTC, original submission:
It's needed a 'remember me' button, so it's not need to log in every time that you visit the site.
|