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

Notice: Undefined variable: ldapconnection in CAS plugin (Authentication)

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.4, 2.3.1, 2.5.4, 2.6
    • Fix Version/s: 2.5.5, 2.6.2
    • Component/s: Authentication
    • Labels:
    • Testing Instructions:
      Hide
      1. Enable the CAS authentication plugin.
      2. Set a group creator filter.
      3. Ensure you can login using CAS without getting the following errors -

      Notice: Undefined variable: ldapconnection in /path/to/moodle/auth/cas/auth.php on line 380 
      

      OR

      Notice: Undefined variable: ldap_cookie in /var/www/moodle/auth/ldap/auth.php on line 1539
      

      Show
      Enable the CAS authentication plugin. Set a group creator filter. Ensure you can login using CAS without getting the following errors - Notice: Undefined variable: ldapconnection in /path/to/moodle/auth/cas/auth.php on line 380 OR Notice: Undefined variable: ldap_cookie in /var/www/moodle/auth/ldap/auth.php on line 1539
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-34319_master

      Description

      This Notice appears if CAS plugin is used.
      If I set a group creator filter (in CAS plugin configuration) and I try to login using CAS, I get this error :

      Notice: Undefined variable: ldapconnection in /path/to/moodle/auth/cas/auth.php on line 380 

      (I see PHP Notice because I have my moodle in debug mode). After that, CAS can't redirect me because this error is displayed on the screen. I could simply disable PHP Notices but I guess it would be better to fix this problem.

        Gliffy Diagrams

          Activity

          Hide
          salvetore Michael de Raadt added a comment -

          Thanks for reporting this.

          I've put that on the backlog.

          In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

          Show
          salvetore Michael de Raadt added a comment - Thanks for reporting this. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
          Hide
          poltawski Dan Poltawski added a comment - - edited

          Requesting peer review on this for cabmen, who has created a pull request on github for it (https://github.com/moodle/moodle/pull/76).

          Show
          poltawski Dan Poltawski added a comment - - edited Requesting peer review on this for cabmen, who has created a pull request on github for it ( https://github.com/moodle/moodle/pull/76 ).
          Hide
          markn Mark Nelson added a comment -

          Thanks for the patch.

          Two points -

          1. Testing instructions are missing.
          2. I think we should handle the situation where ldap_connect() doesn't return a valid LDAP connection. The function ldap_connect() throws an exception in this case, so you could add a try catch here, and if it fails set $ldapconnection to false, which both ldap_find_userdn() and ldap_isgroupmember() will handle fine.

          Regards,

          Mark

          Show
          markn Mark Nelson added a comment - Thanks for the patch. Two points - Testing instructions are missing. I think we should handle the situation where ldap_connect() doesn't return a valid LDAP connection. The function ldap_connect() throws an exception in this case, so you could add a try catch here, and if it fails set $ldapconnection to false, which both ldap_find_userdn() and ldap_isgroupmember() will handle fine. Regards, Mark
          Hide
          markn Mark Nelson added a comment -

          Hmm, for step 2, maybe we do want an exception to be thrown. However, I am just thinking that if CAS was working before prior to this fix, then does that mean the LDAP connection isn't completely necessary? Dan Poltawski, there is no assignee. If you agree with my comment above regarding handling the exception then some one else needs to take this, otherwise it can get put up for integration.

          Show
          markn Mark Nelson added a comment - Hmm, for step 2, maybe we do want an exception to be thrown. However, I am just thinking that if CAS was working before prior to this fix, then does that mean the LDAP connection isn't completely necessary? Dan Poltawski , there is no assignee. If you agree with my comment above regarding handling the exception then some one else needs to take this, otherwise it can get put up for integration.
          Hide
          markn Mark Nelson added a comment -

          I have assigned this issue to myself so we can get this fix into core.

          Show
          markn Mark Nelson added a comment - I have assigned this issue to myself so we can get this fix into core.
          Hide
          markn Mark Nelson added a comment -

          Note to integrator: Since this issue requires CAS to test (which may take some time to set-up) I have included testing MDL-43405 here as well (since it takes exactly the same steps to replicate - we are only making sure another notice is not displayed). So, MDL-43405 must be integrated as well before testing this.

          Show
          markn Mark Nelson added a comment - Note to integrator: Since this issue requires CAS to test (which may take some time to set-up) I have included testing MDL-43405 here as well (since it takes exactly the same steps to replicate - we are only making sure another notice is not displayed). So, MDL-43405 must be integrated as well before testing this.
          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
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          damyon Damyon Wiese added a comment -

          Thanks Ben and Mark,

          The fix looks perfect. Integrated to 25, 26 and master.

          Show
          damyon Damyon Wiese added a comment - Thanks Ben and Mark, The fix looks perfect. Integrated to 25, 26 and master.
          Hide
          dmonllao David Monllaó added a comment -

          It passes; tested in 25, 26 and master. Info about how to configure CAS added to the CAS VM readme

          Show
          dmonllao David Monllaó added a comment - It passes; tested in 25, 26 and master. Info about how to configure CAS added to the CAS VM readme
          Hide
          damyon Damyon Wiese added a comment -

          Parallel programmParallel programming is harding is hard

          Thanks for reporting/fixing/testing/improving Moodle. This issue has now been released.

          Show
          damyon Damyon Wiese added a comment - Parallel programmParallel programming is harding is hard Thanks for reporting/fixing/testing/improving Moodle. This issue has now been released.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Mar/14