Moodle
  1. Moodle
  2. MDL-31053

"programmer error" in get_user_capability_course

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.3
    • Fix Version/s: 2.2.2
    • Component/s: Libraries
    • Labels:
    • Environment:
      PHP 5.3.6
    • Database:
      Any
    • Testing Instructions:
      Hide

      for developers: create script that uses get_user_capability_course() and execute it

      Show
      for developers: create script that uses get_user_capability_course() and execute it
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Rank:
      37467

      Description

      Perhaps this is untested, because it seems that core Moodle never calls get_user_capability_course(). It is seen here because it gets called from a third-party plugin, but the source of the error is in lib/accesslib.php, in get_user_capability_course(). in get_user_capability_course a query is made and the resulting stdClass objects are passed to has_capability(), but has_capability is expecting an object of type context.

      Error seen in Apache error log when the Lightwork client attempts to connect and get some information from Moodle:

      [Fri Jan 06 14:23:13 2012] [error] [client 127.0.0.1] Default exception handler: Coding error detected, it must be fixed by a programmer: PHP catchable fatal error Debug: Argument 2 passed to has_capability() must be an instance of context, instance of stdClass given, called in /private/var/www/postmoodle/lib/accesslib.php on line 3870 and defined\n* line 365 of /lib/setuplib.php: coding_exception thrown\n* line 348 of /lib/accesslib.php: call to default_error_handler()\n* line 3870 of /lib/accesslib.php: call to has_capability()\n* line 398 of /local/lightwork/lib/lw_marker.php: call to get_user_capability_course()\n* line 81 of /local/lightwork/lib/lw_marker.php: call to LW_Marker->load_courses()\n* line 362 of /local/lightwork/lib/lw_marker.php: call to LW_Marker->get_courses()\n* line 286 of /local/lightwork/ws/mdl_soapserver.class.php: call to LW_Marker->courses_modified_since()\n* line ? of unknownfile: call to mdl_soapserver->getCourses()\n* line 4087 of /local/lightwork/ws/lib/nusoap.php: call to call_user_func_array()\n* line 3718 of /local/lightwork/ws/lib/nusoap.php: call to nusoap_server->invoke_method()\n* line 51 of /local/lightwork/ws/service.php: call to nusoap_server->service()\n
      

      Here is a fix:

      diff --git a/lib/accesslib.php b/lib/accesslib.php
      index b7c0c94..91e4c28 100644
      --- a/lib/accesslib.php
      +++ b/lib/accesslib.php
      @@ -3866,15 +3866,16 @@ function get_user_capability_course($capability, $userid = null, $doanything = t
                                                ON (c.id=x.instanceid AND x.contextlevel=".CONTEXT_COURSE.")
                                       $orderby");
           // Check capability for each course in turn
      -    foreach ($rs as $coursecontext) {
      +    foreach ($rs as $courseinfo) {
      +        $coursecontext = context_course::instance($courseinfo->courseid);
               if (has_capability($capability, $coursecontext, $userid, $doanything)) {
                   // We've got the capability. Make the record look like a course record
                   // and store it
      -            $coursecontext->id = $coursecontext->courseid;
      -            unset($coursecontext->courseid);
      -            unset($coursecontext->contextlevel);
      -            unset($coursecontext->instanceid);
      -            $courses[] = $coursecontext;
      +            $courseinfo->id = $courseinfo->courseid;
      +            unset($courseinfo->courseid);
      +            unset($courseinfo->contextlevel);
      +            unset($courseinfo->instanceid);
      +            $courses[] = $courseinfo;
               }
           }
           $rs->close();
      

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for spotting that and providing a solution.

          Petr: I thought you might like to look at this one, otherwise, feel free to reassign.

          Show
          Michael de Raadt added a comment - Thanks for spotting that and providing a solution. Petr: I thought you might like to look at this one, otherwise, feel free to reassign.
          Hide
          Petr Škoda added a comment -

          Thanks a lot for the report and patch and sorry for the regression.

          Petr

          Show
          Petr Škoda added a comment - Thanks a lot for the report and patch and sorry for the regression. Petr
          Hide
          Petr Škoda added a comment -

          We should most probably deprecate this in favour of enrol_get_users_courses().

          Show
          Petr Škoda added a comment - We should most probably deprecate this in favour of enrol_get_users_courses().
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm, not sure if we have to deprecate that. We already deprecated get_user_courses_bycap() in favor or enrol_get_users_courses() (that perform both enrol AND cap checks).

          And get_user_capability_course() is pure cap check (enrol doesn't matter). Perhaps it can have sense to keep it there for now (comparisons, orphaned ras/caps, whatever...).

          Offtopic, while looking at this, I detected that the get_enrolled_users() documentation on top of the file, seems incorrect.

          Integrated (22 and master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, not sure if we have to deprecate that. We already deprecated get_user_courses_bycap() in favor or enrol_get_users_courses() (that perform both enrol AND cap checks). And get_user_capability_course() is pure cap check (enrol doesn't matter). Perhaps it can have sense to keep it there for now (comparisons, orphaned ras/caps, whatever...). Offtopic, while looking at this, I detected that the get_enrolled_users() documentation on top of the file, seems incorrect. Integrated (22 and master), thanks!
          Hide
          Petr Škoda added a comment -

          The problem here is that it loops all courses which in case of large sites is going to be a performance problem - that means it should not be acceptable to include this in official plugins.

          Show
          Petr Škoda added a comment - The problem here is that it loops all courses which in case of large sites is going to be a performance problem - that means it should not be acceptable to include this in official plugins.
          Hide
          Ankit Agarwal added a comment -

          Hi,
          This is working great.
          Passing!
          Thanks

          Show
          Ankit Agarwal added a comment - Hi, This is working great. Passing! Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now available in the git and cvs repositories.

          Consider the responsibility of your fingerprints engraved there for future generations!

          Thanks for the work, closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This is now available in the git and cvs repositories. Consider the responsibility of your fingerprints engraved there for future generations! Thanks for the work, closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: