bugGNU FM - Bugs: bug #26264, 'Remember me' button on login page

 
 

bug #26264: 'Remember me' button on login page

Submitted by:  Carlos <karlos_rocks>
Submitted on:  Tue 21 Apr 2009 04:12:35 AM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: NoneStatus: Fixed
Privacy: PublicAssigned to: None
Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 01 May 2009 09:02:01 PM UTC, comment #15:

Fix commited in r1101.

Michael Sheldon <elleo>
Project Member
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.

Michael Sheldon <elleo>
Project Member
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);"

Michael Sheldon <elleo>
Project Member
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.

Michael Sheldon <elleo>
Project Member
Tue 28 Apr 2009 04:47:22 PM UTC, comment #11:

Pasted final patch.
Please apply.

(file #18044)

Not Available <piotrszulawski>
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.

Michael Sheldon <elleo>
Project Member
Tue 28 Apr 2009 03:41:33 PM UTC, comment #9:

Fixed

(file #18041)

Not Available <piotrszulawski>
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.

Michael Sheldon <elleo>
Project Member
Tue 28 Apr 2009 03:07:17 PM UTC, comment #7:

wrong patch, working is 26264_3 :/

(file #18039)

Not Available <piotrszulawski>
Tue 28 Apr 2009 03:03:01 PM UTC, comment #6:

Added new patch. It works.

(file #18038)

Not Available <piotrszulawski>
Tue 28 Apr 2009 12:26:50 PM UTC, comment #5:

Okay,

so i'll extend that.
Patch will be submited later.

Not Available <piotrszulawski>
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.

Michael Sheldon <elleo>
Project Member
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?

Not Available <piotrszulawski>
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.

Michael Sheldon <elleo>
Project Member
Tue 28 Apr 2009 10:16:51 AM UTC, comment #1:

Fixed,

here's a patch.

(file #18034)

Not Available <piotrszulawski>
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.

Carlos <karlos_rocks>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #18062:  26264.zip added by piotrszulawski (3KiB - application/zip)
file #18044:  26264_final.txt added by piotrszulawski (1KiB - text/plain)
file #18039:  26264_3.txt added by piotrszulawski (7KiB - text/plain)
file #18038:  26264_2.txt added by piotrszulawski (7KiB - text/plain)
file #18034:  26264.txt added by piotrszulawski (2KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by elleo (Posted a comment)
  • -unavailable- added by piotrszulawski (Updated the item)
  • -unavailable- added by karlos_rocks (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 01 May 2009 09:02:01 PM UTCelleoStatusNone=>Fixed
      Open/ClosedOpen=>Closed
    Thu 30 Apr 2009 09:09:59 AM UTCpiotrszulawskiAttached File-=>Added 26264.zip, #18062
    Wed 29 Apr 2009 03:51:33 PM UTCpiotrszulawskiAttached File-=>Added 26264_complete.txt, #18054
    Tue 28 Apr 2009 04:47:22 PM UTCpiotrszulawskiAttached File-=>Added 26264_final.txt, #18044
    Tue 28 Apr 2009 03:41:33 PM UTCpiotrszulawskiAttached File-=>Added 26264_correctedIndentation.txt, #18041
    Tue 28 Apr 2009 03:07:17 PM UTCpiotrszulawskiAttached File-=>Added 26264_3.txt, #18039
    Tue 28 Apr 2009 03:03:01 PM UTCpiotrszulawskiAttached File-=>Added 26264_2.txt, #18038
    Tue 28 Apr 2009 10:16:51 AM UTCpiotrszulawskiAttached File-=>Added 26264.txt, #18034

    Back to the top


    Powered by Savane 3.1-cleanup1