Moodle
  1. Moodle
  2. MDL-26257

Shibboleth plugin overrides logout redirect url even user did not log in through Shibboleth

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.13, 2.0.4, 2.1.1, 2.2
    • Fix Version/s: 1.9.14, 2.0.5, 2.1.2
    • Component/s: Authentication
    • Labels:
      None
    • Testing Instructions:
      Hide

      This requires a Moodle installation with Shibboleth authentication set up, and Shibboleth log out page handler configured.

      • Create two user accounts, "A" and "B".
      • Set authentication to Manual for "A", to Shibboleth for "B".
      • Log in as "A" and log out again.

      VERIFY: Shibboleth log out page is not shown.

      • Log in as "B" and log out again.

      VERIFY: Shibboleth log out page is being shown.

      Show
      This requires a Moodle installation with Shibboleth authentication set up, and Shibboleth log out page handler configured. Create two user accounts, "A" and "B". Set authentication to Manual for "A", to Shibboleth for "B". Log in as "A" and log out again. VERIFY: Shibboleth log out page is not shown. Log in as "B" and log out again. VERIFY: Shibboleth log out page is being shown.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      15897

      Description

      When I have both manual accounts and Shibboleth authentication enabled, if I log in through a manual account in Moodle and then log out, I got redirected to the Shibboleth logout page. I only want to be redirected to this url when I logged in through Shibboleth, which will completely log me out of my idp. But if I log in from a manual account, I want to be redirected to the default logout page when logout.

      I guess one way to fix this is to change logoutpage_hook() in "auth/shibboleth/auth.php" to check for $USER->auth. Only override the global variable $redirect when $USER->auth == "shibboleth".

      Hope this will make it to the core soon. Thanks!

        Issue Links

          Activity

          Hide
          Carson Tam added a comment -

          Here is the code that fix this:

          diff -u -r1.29 auth.php
          --- auth/shibboleth/auth.php    17 Feb 2011 00:48:40 -0000      1.29
          +++ auth/shibboleth/auth.php    3 Mar 2011 01:29:48 -0000
          @@ -193,11 +193,12 @@
                *
                */
               function logoutpage_hook() {
          -        global $redirect;
          +        global $USER, $redirect;
          
                   // Only do this if logout handler is defined
                   if (
          -              isset($this->config->logout_handler)
          +              $USER->auth == $this->authtype
          +              && isset($this->config->logout_handler)
                         && !empty($this->config->logout_handler)
                      ){
                       // Check if there is an alternative logout return url defined
          
          Show
          Carson Tam added a comment - Here is the code that fix this: diff -u -r1.29 auth.php --- auth/shibboleth/auth.php 17 Feb 2011 00:48:40 -0000 1.29 +++ auth/shibboleth/auth.php 3 Mar 2011 01:29:48 -0000 @@ -193,11 +193,12 @@ * */ function logoutpage_hook() { - global $redirect; + global $USER, $redirect; // Only do this if logout handler is defined if ( - isset($ this ->config->logout_handler) + $USER->auth == $ this ->authtype + && isset($ this ->config->logout_handler) && !empty($ this ->config->logout_handler) ){ // Check if there is an alternative logout return url defined
          Hide
          Henning Bostelmann added a comment -

          I'm submitting a fix for peer review which is a slight variation of the one proposed above. Instead of checking $USER->auth, I check for $SESSION->shibsessionid, which is set on Shibboleth login. We've been using a similar solution on our installation for approx. 1 year.

          Show
          Henning Bostelmann added a comment - I'm submitting a fix for peer review which is a slight variation of the one proposed above. Instead of checking $USER->auth, I check for $SESSION->shibsessionid, which is set on Shibboleth login. We've been using a similar solution on our installation for approx. 1 year.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Thanks for the work on this issue.
          The solution looks good thanks Henning, feel free to put this up for integration when you want.

          There was one thing I noted for interest sake, the isset($this->config->logout_handler) check is necessary as the following !empty check will in turn check the variable is set as part of its operation.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Thanks for the work on this issue. The solution looks good thanks Henning, feel free to put this up for integration when you want. There was one thing I noted for interest sake, the isset($this->config->logout_handler) check is necessary as the following !empty check will in turn check the variable is set as part of its operation. Cheers Sam
          Hide
          Henning Bostelmann added a comment -

          Hi Sam, thanks for reviewing this so fast. Unfortunately, I can't send fixes to integration directly; you'll need to push the button for me. Cheers!

          Show
          Henning Bostelmann added a comment - Hi Sam, thanks for reviewing this so fast. Unfortunately, I can't send fixes to integration directly; you'll need to push the button for me. Cheers!
          Hide
          Sam Hemelryk added a comment -

          Thanks Henning, I've put this up for integration now and I've even gone as far as inserting it into the current integration with Apu assigned as the integrator.

          All yours Apu

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Henning, I've put this up for integration now and I've even gone as far as inserting it into the current integration with Apu assigned as the integrator. All yours Apu Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          Thanks for the patch.
          This has been stylishly integrated

          Show
          Aparup Banerjee added a comment - Thanks for the patch. This has been stylishly integrated
          Hide
          Rossiani Wijaya added a comment - - edited

          Received the following errors while testing this on 2.0:

          Parse error: syntax error, unexpected ')' in /m20/auth/shibboleth/auth.php on line 200
          

          The following error occurs in 2.0, 2.1 and master

          Notice: Undefined property: stdClass::$alt_login in /m20/auth/shibboleth/config.html on line 44
          

          Note: Functional test is still in progress.

          Show
          Rossiani Wijaya added a comment - - edited Received the following errors while testing this on 2.0: Parse error: syntax error, unexpected ')' in /m20/auth/shibboleth/auth.php on line 200 The following error occurs in 2.0, 2.1 and master Notice: Undefined property: stdClass::$alt_login in /m20/auth/shibboleth/config.html on line 44 Note: Functional test is still in progress.
          Hide
          Rossiani Wijaya added a comment -

          Found error in the code.

          Test failed

          Show
          Rossiani Wijaya added a comment - Found error in the code. Test failed
          Hide
          Aparup Banerjee added a comment -

          sorry! thats been fixed. please test again.
          that $alt_login notice is probably a separate issue.

          Show
          Aparup Banerjee added a comment - sorry! thats been fixed. please test again. that $alt_login notice is probably a separate issue.
          Hide
          moodle.com added a comment -

          Hi, Carson.

          Would you be able to get the latest integrated version and run the test given above?

          You can get it from the integration repository on git.moodle.org/integration.git or from the git repositories listed above.

          Let us know if it succeeded by commenting here.

          Show
          moodle.com added a comment - Hi, Carson. Would you be able to get the latest integrated version and run the test given above? You can get it from the integration repository on git.moodle.org/integration.git or from the git repositories listed above. Let us know if it succeeded by commenting here.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Got Shibboleth IDP + SP + apache_mod working here, really funny. Testing...

          Show
          Eloy Lafuente (stronk7) added a comment - Got Shibboleth IDP + SP + apache_mod working here, really funny. Testing...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Just defined the logout handler as /LOGOUT.

          One local user, on logout, is redirected to main page (no LOGOUT).

          One shibboleth user, on logout, is redirected to /LOGOUT.

          Show
          Eloy Lafuente (stronk7) added a comment - Just defined the logout handler as /LOGOUT. One local user, on logout, is redirected to main page (no LOGOUT). One shibboleth user, on logout, is redirected to /LOGOUT.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YTC !

          (aka, yay, thanks and ciao ) Closing.

          Show
          Eloy Lafuente (stronk7) added a comment - YTC ! (aka, yay, thanks and ciao ) Closing.

            People

            • Votes:
              7 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: