Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-26257

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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:

      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!

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              carsontam 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
              carsontam 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
              bostelm 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
              bostelm 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
              samhemelryk 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
              samhemelryk 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
              bostelm 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
              bostelm 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
              samhemelryk 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
              samhemelryk 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
              nebgor Aparup Banerjee added a comment -

              Thanks for the patch.
              This has been stylishly integrated

              Show
              nebgor Aparup Banerjee added a comment - Thanks for the patch. This has been stylishly integrated
              Hide
              rwijaya 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
              rwijaya 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
              rwijaya Rossiani Wijaya added a comment -

              Found error in the code.

              Test failed

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

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

              Show
              nebgor Aparup Banerjee added a comment - sorry! thats been fixed. please test again. that $alt_login notice is probably a separate issue.
              Hide
              moodle.com 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 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

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

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Got Shibboleth IDP + SP + apache_mod working here, really funny. Testing...
              Hide
              stronk7 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
              stronk7 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

              YTC !

              (aka, yay, thanks and ciao ) Closing.

              Show
              stronk7 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:
                    Fix Release Date:
                    10/Oct/11