Moodle
  1. Moodle
  2. MDL-36138

Fix CAS Authentication Strict Standards warnings

    Details

    • Testing Instructions:
      Hide

      Pre-requisite:

      1. CAS and LDAP server configured to be used with moodle
      2. Enable debug developer mode

      TEST

      1. Log in as user who is not in moodle, but in CAS/LDAP.
      2. Make sure you don't see any php strict warnings.
      Show
      Pre-requisite: CAS and LDAP server configured to be used with moodle Enable debug developer mode TEST Log in as user who is not in moodle, but in CAS/LDAP. Make sure you don't see any php strict warnings.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36138-master
    • Rank:
      44910

      Description

      When we first connect with our CAS Authentication method with the version 2.3.2 of Moodle those warning appears:

      Strict Standards: Non-static method phpCAS::client() should not be called statically, assuming $this from incompatible context in D:\xampp\htdocs\moodle\auth\cas\auth.php on line 190
      
      Strict Standards: Non-static method phpCAS::traceBegin() should not be called statically, assuming $this from incompatible context in D:\xampp\htdocs\moodle\auth\cas\CAS\CAS.php on line 345
      
      Strict Standards: Non-static method phpCAS::backtrace() should not be called statically, assuming $this from incompatible context in D:\xampp\htdocs\moodle\auth\cas\CAS\CAS.php on line 556
      

      Its only a short list but the problem is appear because of the function who are not called as public static function in the class phpCAS in auth/cas/CAS/CAS.php

      Another warning is about to not initialize the global var $frm in the function loginpage_hook() in auth/cas/auth.php like this:

      $frm = new stdClass();
      

        Issue Links

          Activity

          Hide
          Gilles-Philippe Leblanc added a comment -

          By the way the version of phpCAS is quite old ... v.1.1.3. Would it not be better to update at the latest version?

          Show
          Gilles-Philippe Leblanc added a comment - By the way the version of phpCAS is quite old ... v.1.1.3. Would it not be better to update at the latest version?
          Hide
          Petr Škoda added a comment -

          Hello and thanks for the report and patch. I agree it seems it might be better to import the latest version of all external libraries. I am sorry, I do not know who should be reviewing this patch, I do not think there is any permanent maintainer for the authentication plugin. Maybe posting in moodle.org forums might help to attract attention of other users of this plugin and maybe somebody might volunteer to maintain it. I saw several other CAS related issues, I believe it would be better to address all issues at the same time and let other sites test it before it gets integrated into official moodle distribution.

          Show
          Petr Škoda added a comment - Hello and thanks for the report and patch. I agree it seems it might be better to import the latest version of all external libraries. I am sorry, I do not know who should be reviewing this patch, I do not think there is any permanent maintainer for the authentication plugin. Maybe posting in moodle.org forums might help to attract attention of other users of this plugin and maybe somebody might volunteer to maintain it. I saw several other CAS related issues, I believe it would be better to address all issues at the same time and let other sites test it before it gets integrated into official moodle distribution.
          Hide
          Petr Škoda added a comment -

          I have added our LDAP guru here too because this may be related to auth_ldap too, any ideas Iñaki?

          Show
          Petr Škoda added a comment - I have added our LDAP guru here too because this may be related to auth_ldap too, any ideas Iñaki?
          Hide
          Iñaki Arenaza added a comment -

          Hi,

          this is not related to auth_ldap at all. But Gillies-Philippe is right, our version is old (and even has a couple of known vulnerabilities, with CVEs included).

          I've been a bit hesitant to upgrade to newer versions because they did at lest one backwards-incompatible change, if you are using CAS with proxied applications. The question is I don't know if anybody out there is using Moodle with CAS and proxied applications. I don't even know if it has ever worked (I've never tested it myself).

          So the question is, how to proceed with this? Upgrading is highly recommended, but we risk breaking some (very few?) setups.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi, this is not related to auth_ldap at all. But Gillies-Philippe is right, our version is old (and even has a couple of known vulnerabilities, with CVEs included). I've been a bit hesitant to upgrade to newer versions because they did at lest one backwards-incompatible change, if you are using CAS with proxied applications. The question is I don't know if anybody out there is using Moodle with CAS and proxied applications. I don't even know if it has ever worked (I've never tested it myself). So the question is, how to proceed with this? Upgrading is highly recommended, but we risk breaking some (very few?) setups. Saludos. Iñaki.
          Hide
          Petr Škoda added a comment -

          One of the options is to upgrade right after the next release (next month, 2.5dev) and tell people to test it and wait&see if somebody complains. Theoretically if nothing pops up we could bacport it to 2.3.x sometime next year.

          Show
          Petr Škoda added a comment - One of the options is to upgrade right after the next release (next month, 2.5dev) and tell people to test it and wait&see if somebody complains. Theoretically if nothing pops up we could bacport it to 2.3.x sometime next year.
          Hide
          Michael de Raadt added a comment -

          Hi, Gilles-Philippe.

          If you'd like to put this forward for peer review, please click "Request peer review" instead of "Start peer review".

          Show
          Michael de Raadt added a comment - Hi, Gilles-Philippe. If you'd like to put this forward for peer review, please click "Request peer review" instead of "Start peer review".
          Hide
          Adam Olley added a comment -

          Looks like these two are the same thing.

          Show
          Adam Olley added a comment - Looks like these two are the same thing.
          Hide
          Adam Olley added a comment - - edited

          Line 342 of auth/cas/CAS/CAS.php appears to have a little bit extra whitespace than it should (EDIT: actually, its using spaces where the CAS class uses tabs, I guess we should keep consistent within the 3rd party lib and use a tab?).
          Commits should probably mention the component as "auth_cas" rather than "auth".

          phpCAS update should definitely wait till 2.5dev to do it. Till then, this fix of the signatures seems to do the trick as far as cleaning up warnings goes.

          Show
          Adam Olley added a comment - - edited Line 342 of auth/cas/CAS/CAS.php appears to have a little bit extra whitespace than it should (EDIT: actually, its using spaces where the CAS class uses tabs, I guess we should keep consistent within the 3rd party lib and use a tab?). Commits should probably mention the component as "auth_cas" rather than "auth". phpCAS update should definitely wait till 2.5dev to do it. Till then, this fix of the signatures seems to do the trick as far as cleaning up warnings goes.
          Hide
          Gilles-Philippe Leblanc added a comment -

          Spaces on line 342 have been replaced by a tab character. Everything was recommitted with the mention auth_cas.

          Show
          Gilles-Philippe Leblanc added a comment - Spaces on line 342 have been replaced by a tab character. Everything was recommitted with the mention auth_cas.
          Hide
          Rajesh Taneja added a comment -

          Hello Petr and Iñaki,

          Just wondering if we can have this patch now in stable, so people don't see strict warnings and once we upgrade we can decide to backport (if need be.)

          Show
          Rajesh Taneja added a comment - Hello Petr and Iñaki, Just wondering if we can have this patch now in stable, so people don't see strict warnings and once we upgrade we can decide to backport (if need be.)
          Hide
          Iñaki Arenaza added a comment -

          +1 from me.

          Show
          Iñaki Arenaza added a comment - +1 from me.
          Hide
          Rajesh Taneja added a comment -

          Thanks Gilles-Philippe,

          Patch looks good. Pushing for integration review.
          [y] Syntax
          [y] Output
          [y] Whitespace
          [-] Language
          [-] Databases
          [y] Testing
          [-] Security
          [-] Documentation
          [y] Git
          [y] Sanity check

          Show
          Rajesh Taneja added a comment - Thanks Gilles-Philippe, Patch looks good. Pushing for integration review. [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing [-] Security [-] Documentation [y] Git [y] Sanity check
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Gilles-Philippe Leblanc added a comment -

          Rebase done.

          Show
          Gilles-Philippe Leblanc added a comment - Rebase done.
          Hide
          Dan Poltawski added a comment -

          Integrated into master.

          Show
          Dan Poltawski added a comment - Integrated into master.
          Hide
          Dan Poltawski added a comment -

          Testing based on code review only.

          Note to CAS users watching this issue: we are in desperate need to upgrade the phpcas library bunleded with Moodle to the new version of the library as its very outdated.

          We really need people to test this change (as well as publicising it on the forums). If anyone is able to step forward for this, please shout!

          Show
          Dan Poltawski added a comment - Testing based on code review only. Note to CAS users watching this issue: we are in desperate need to upgrade the phpcas library bunleded with Moodle to the new version of the library as its very outdated. We really need people to test this change (as well as publicising it on the forums). If anyone is able to step forward for this, please shout!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: