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
    • Rank:
      48037

      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.

        Activity

        Hide
        Petr Škoda 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 Škoda 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 Škoda added a comment -

        Thanks Fred for the confirmation.

        Show
        Petr Škoda 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: