Moodle
  1. Moodle
  2. MDL-38202

session_stub->terminate_current prematurely calls session_write_close()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.7, 2.3.4, 2.4.1, 2.5
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Authentication, Course, General
    • Labels:
      None
    • Testing Instructions:
      Hide

      1/ somehow disable referer in your browser
      2/ loginas some user and go to course, click on your username to logout and verify you are returned to the same course
      3/ repeat 2/ at the forntpage

      Show
      1/ somehow disable referer in your browser 2/ loginas some user and go to course, click on your username to logout and verify you are returned to the same course 3/ repeat 2/ at the forntpage
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w12_MDL-38202_m25_loginas

      Description

      In a typical logininas scenario, when the instructor returns to their own perspective (reverting to $_SESSION['REALUSER']) by clicking on their username in the page header, they are forced to re-authenticate, and then are supposedly returned to page they were viewing.

      This only works accidentally, as the course/loginas.php attempts to set $SESSION->wantsurl before the redirect, but that assignment is discarded because session_write_close() was called as part of the flow of execution from requires_logout() to session_get_instance()->terminate_current().

      It accidentally works because login/index.php reconstructs $CFG->wantsurl from $_SERVER[HTTP_REFERER], and generally that works out fine since the instructor would expect to return to the page they were viewing.

      However, in the case of Shibboleth SSO, or any other auth plugin needing to examine the $CFG->wantsurl value in the loginpage_hook() method, it fails.

      Also, considering the terminate_current() method calls session_regenerate_id(), it can be assumed there may be some values yet to be placed in the new session--prevented by the session_write_close().

      At the very least, closing the newly created session should be optional, so that a script such as course/loginas.php can still use $SESSION.

      The slight change we made to the Shibboleth module checks the $SESSION->wantsurl, among other things, to issue a redirect to the Shibboleth protected page (deep links, etc.). We try to send our users directly to the IdP when possible, rather than having an extra stop on Moodle login page.

      Only suggested fix right now is to remove the call to session_write_close() at the end of session_stub->terminate_current(), and let the end of processing take care of closing the session.

        Gliffy Diagrams

          Activity

          Hide
          Petr Skoda added a comment -

          Thanks for the report. The session_write_close() is there for security reasons, I have used extra redirect to get fresh new session before setting the wanted URL.

          Show
          Petr Skoda added a comment - Thanks for the report. The session_write_close() is there for security reasons, I have used extra redirect to get fresh new session before setting the wanted URL.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Petr,

          Works as described.

          FYI: Network.http.sendRefererHeader = 0 in FF disables referrer.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Petr, Works as described. FYI: Network.http.sendRefererHeader = 0 in FF disables referrer.
          Hide
          Fred Woolard added a comment -

          Testing is successful with Petr's changes applied, along with my mods to the Shibboleth auth plugin. Thanks.

          Show
          Fred Woolard added a comment - Testing is successful with Petr's changes applied, along with my mods to the Shibboleth auth plugin. Thanks.
          Hide
          Petr Skoda added a comment -

          Thanks Fred for the confirmation.

          Show
          Petr Skoda added a comment - Thanks Fred for the confirmation.
          Hide
          Damyon Wiese added a comment -

          This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Thanks for your contributions!

          Show
          Damyon Wiese added a comment - This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads). Thanks for your contributions!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: