Moodle
  1. Moodle
  2. MDL-35029

Avoid session timeout for guest users

    Details

    • Testing Instructions:
      Hide

      Run the test in a browser with a single window

      1. Login as an admin
      2. Enable Users -> Permissions -> User policies -> autologinguests
      3. Enable Server -> Session handling -> dbsessions if it's not already enabled
      4. Set Front page -> Front page settings -> frontpage to "List of courses"
      5. Create or set a course without guest access
      6. Logout as admin
      7. Go to the database and delete all the mdl_sessions records with userid = 1
      8. Edit the database and set mdl_config->sessiontimeout to 1
      9. Without being logged (you will be automatically logged as a guest) go to the front page and click the course
      10. You will be redirected to enrol/index.php?id=NNN
      11. Reload the page after waiting a second, you SHOULD be on enrol/index.php?id=NNN instead of the login page
      12. Close the browser
      13. Go to the database and check the mdl_session table, there should be a record with userid = 1
      14. Wait for 5 seconds, run the cron and make sure that it finish successfully
      15. The mdl_session record with userid = 1 SHOULD NOT remain in the DB
      16. Probably you will want to change your $CFG->sessiontimeout value
      Show
      Run the test in a browser with a single window Login as an admin Enable Users -> Permissions -> User policies -> autologinguests Enable Server -> Session handling -> dbsessions if it's not already enabled Set Front page -> Front page settings -> frontpage to "List of courses" Create or set a course without guest access Logout as admin Go to the database and delete all the mdl_sessions records with userid = 1 Edit the database and set mdl_config->sessiontimeout to 1 Without being logged (you will be automatically logged as a guest) go to the front page and click the course You will be redirected to enrol/index.php?id=NNN Reload the page after waiting a second, you SHOULD be on enrol/index.php?id=NNN instead of the login page Close the browser Go to the database and check the mdl_session table, there should be a record with userid = 1 Wait for 5 seconds, run the cron and make sure that it finish successfully The mdl_session record with userid = 1 SHOULD NOT remain in the DB Probably you will want to change your $CFG->sessiontimeout value
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-35029_master
    • Rank:
      43638

      Description

      After talking with Petr about MDL-34965, seems a good option to remove the session timeout for guest users when auto login as guest is enabled

        Issue Links

          Activity

          Hide
          David Monllaó added a comment -

          WIP

          Show
          David Monllaó added a comment - WIP
          Hide
          David Monllaó added a comment -

          Attaching patch, the garbage collector works well with this change as it manages the guest sessions in a different manner than the regular user sessions (current code below)

          Getting DB sessions to delete

          $sql = "SELECT u.*, s.sid, s.timecreated AS s_timecreated, s.timemodified AS s_timemodified
                    FROM {user} u
                    JOIN {sessions} s ON s.userid = u.id
                   WHERE s.timemodified + ? < ? AND u.id <> ?";
          $params = array($maxlifetime, time(), $CFG->siteguest);
          

          Removing guest sessions

          $purgebefore = time() - $maxlifetime;
          // delete expired sessions for guest user account
          $DB->delete_records_select('sessions', 'userid = ? AND timemodified < ?', array($CFG->siteguest, $purgebefore));
          
          Show
          David Monllaó added a comment - Attaching patch, the garbage collector works well with this change as it manages the guest sessions in a different manner than the regular user sessions (current code below) Getting DB sessions to delete $sql = "SELECT u.*, s.sid, s.timecreated AS s_timecreated, s.timemodified AS s_timemodified FROM {user} u JOIN {sessions} s ON s.userid = u.id WHERE s.timemodified + ? < ? AND u.id <> ?"; $params = array($maxlifetime, time(), $CFG->siteguest); Removing guest sessions $purgebefore = time() - $maxlifetime; // delete expired sessions for guest user account $DB->delete_records_select('sessions', 'userid = ? AND timemodified < ?', array($CFG->siteguest, $purgebefore));
          Hide
          Petr Škoda added a comment -

          1/ I think the "AND" in condition is not correct, because that would be never true.

          2/ Why limit it to automatic login only?

          if (!isloggedin() or isguestuser($user)) {
            $ignoretimeout = true;
          } else {
            ... old code
          }
          

          3/ the session_gc() code should imho use longer timeout (5x for example) because otherwise the change in with $ignoretimeout may not have any effect.

          Show
          Petr Škoda added a comment - 1/ I think the "AND" in condition is not correct, because that would be never true. 2/ Why limit it to automatic login only? if (!isloggedin() or isguestuser($user)) { $ignoretimeout = true ; }  else { ... old code } 3/ the session_gc() code should imho use longer timeout (5x for example) because otherwise the change in with $ignoretimeout may not have any effect.
          Hide
          David Monllaó added a comment - - edited

          Thanks Petr,

          About 1) is correct because isloggedin() is always true, the session read handler is always executed before $USER is set, I've removed it since it's an unnecessary checking. I've added a *5 to the garbage collector when deleting guest sessions as proposed.

          I limited it to autologinguest according to the require_login() behaviour:

          If they are not logged in, then it redirects them to the site login unless
          $autologinguest is set and $CFG->autologinguests is set to 1 in which
          case they are automatically logged in as guests.

          If you think we don't need the autologinguests checking please comment and I remove it before submitting it to integration. Thanks in advance

          Show
          David Monllaó added a comment - - edited Thanks Petr, About 1) is correct because isloggedin() is always true, the session read handler is always executed before $USER is set, I've removed it since it's an unnecessary checking. I've added a *5 to the garbage collector when deleting guest sessions as proposed. I limited it to autologinguest according to the require_login() behaviour: If they are not logged in, then it redirects them to the site login unless $autologinguest is set and $CFG->autologinguests is set to 1 in which case they are automatically logged in as guests. If you think we don't need the autologinguests checking please comment and I remove it before submitting it to integration. Thanks in advance
          Hide
          Petr Škoda added a comment -

          Hi, my biggest question was why to do the longer timeouts only when !empty($CFG->autologinguests)? It might be also good to explain the 5x timeout in some comment, I supposed the most interesting are the security bits.

          Show
          Petr Škoda added a comment - Hi, my biggest question was why to do the longer timeouts only when !empty($CFG->autologinguests)? It might be also good to explain the 5x timeout in some comment, I supposed the most interesting are the security bits.
          Hide
          David Monllaó added a comment -

          Hi Petr,

          I've removed the CFG->autologinguests and added a comment about the x5 timeout

          Show
          David Monllaó added a comment - Hi Petr, I've removed the CFG->autologinguests and added a comment about the x5 timeout
          Hide
          Petr Škoda added a comment -

          +1

          Show
          Petr Škoda added a comment - +1
          Hide
          David Monllaó added a comment -

          Thanks Petr, submitting for integration

          Show
          David Monllaó added a comment - Thanks Petr, submitting for integration
          Hide
          David Monllaó added a comment -

          Changing the issue name according to the final solution

          Show
          David Monllaó added a comment - Changing the issue name according to the final solution
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks David, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks David, this has been integrated now
          Hide
          Rossiani Wijaya added a comment -

          This works great.

          Thanks David.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This works great. Thanks David. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for the hard work.

          These changes have been spread upstream and are already available in the git and cvs repositories.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work. These changes have been spread upstream and are already available in the git and cvs repositories. Ciao
          Hide
          Petr Škoda added a comment - - edited

          hi, I am just fixing a bug in the login page redirection, it has to be done only if the user is not logged in yet or is guest. for real users there is no point in redirecting to the login page...

          see MDL-36780

          Show
          Petr Škoda added a comment - - edited hi, I am just fixing a bug in the login page redirection, it has to be done only if the user is not logged in yet or is guest. for real users there is no point in redirecting to the login page... see MDL-36780

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: