Uploaded image for project: '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

      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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            dmonllao David Monllaó added a comment -

            WIP

            Show
            dmonllao David Monllaó added a comment - WIP
            Hide
            dmonllao 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
            dmonllao 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            dmonllao 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
            dmonllao 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            dmonllao David Monllaó added a comment -

            Hi Petr,

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

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

            +1

            Show
            skodak Petr Skoda added a comment - +1
            Hide
            dmonllao David Monllaó added a comment -

            Thanks Petr, submitting for integration

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

            Changing the issue name according to the final solution

            Show
            dmonllao David Monllaó added a comment - Changing the issue name according to the final solution
            Hide
            stronk7 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
            stronk7 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
            samhemelryk Sam Hemelryk added a comment -

            Thanks David, this has been integrated now

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

            This works great.

            Thanks David.

            Test passed.

            Show
            rwijaya Rossiani Wijaya added a comment - This works great. Thanks David. Test passed.
            Hide
            stronk7 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
            stronk7 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  3/Dec/12