MDL-53772 externallib: Fix busted webservices context handling

Fix:
$PAGE->context must be reset when calling validate_context

Improve:
Provide wrapper for calling an external function

The wrapper correctly checks the function parameters and return type against
the description of the external function, and stores the PAGE and COURSE global
state variables, restoring them before the function returns.

Fix: buggy unit tests.

These tests are expecting debugging from a bug that was fixed, and calling web
service functions with no user or session.

lib/ajax/service.php

@@ -41,66 +41,15 @@ if ($requests === null) {
 }
 $responses = array();
 
-
 foreach ($requests as $request) {
     $response = array();
     $methodname = clean_param($request['methodname'], PARAM_ALPHANUMEXT);
     $index = clean_param($request['index'], PARAM_INT);
     $args = $request['args'];
 
-    try {
+    $response = external_api::call_external_function($methodname, $args, true);
-        $externalfunctioninfo = external_function_info($methodname);
+    $responses[$index] = $response;
-
+    if ($response['error']) {
-        if (!$externalfunctioninfo->allowed_from_ajax) {
-            error_log('This external function is not available to ajax. Failed to call "' . $methodname . '"');
-            throw new moodle_exception('servicenotavailable', 'webservice');
-        }
-
-        // Do not allow access to write or delete webservices as a public user.
-        if ($externalfunctioninfo->loginrequired) {
-            if (defined('NO_MOODLE_COOKIES') && NO_MOODLE_COOKIES) {
-                error_log('Set "loginrequired" to false in db/service.php when calling entry point service-nologin.php. ' .
-                          'Failed to call "' . $methodname . '"');
-                throw new moodle_exception('servicenotavailable', 'webservice');
-            }
-            if (!isloggedin()) {
-                error_log('This external function is not available to public users. Failed to call "' . $methodname . '"');
-                throw new moodle_exception('servicenotavailable', 'webservice');
-            } else {
-                require_sesskey();
-            }
-        }
-
-        // Validate params, this also sorts the params properly, we need the correct order in the next part.
-        $callable = array($externalfunctioninfo->classname, 'validate_parameters');
-        $params = call_user_func($callable,
-                                 $externalfunctioninfo->parameters_desc,
-                                 $args);
-
-        // Execute - gulp!
-        $callable = array($externalfunctioninfo->classname, $externalfunctioninfo->methodname);
-        $result = call_user_func_array($callable,
-                                       array_values($params));
-
-        // Validate the return parameters.
-        if ($externalfunctioninfo->returns_desc !== null) {
-            $callable = array($externalfunctioninfo->classname, 'clean_returnvalue');
-            $result = call_user_func($callable, $externalfunctioninfo->returns_desc, $result);
-        }
-
-        $response['error'] = false;
-        $response['data'] = $result;
-        $responses[$index] = $response;
-    } catch (Exception $e) {
-        $jsonexception = get_exception_info($e);
-        unset($jsonexception->a);
-        if (!debugging('', DEBUG_DEVELOPER)) {
-            unset($jsonexception->debuginfo);
-            unset($jsonexception->backtrace);
-        }
-        $response['error'] = true;
-        $response['exception'] = $jsonexception;
-        $responses[$index] = $response;
         // Do not process the remaining requests.
         break;
     }

lib/externallib.php

@@ -35,98 +35,7 @@ defined('MOODLE_INTERNAL') || die();
  * @since Moodle 2.0
  */
 function external_function_info($function, $strictness=MUST_EXIST) {
-    global $DB, $CFG;
+    return external_api::external_function_info($function, $strictness);
-
-    if (!is_object($function)) {
-        if (!$function = $DB->get_record('external_functions', array('name'=>$function), '*', $strictness)) {
-            return false;
-        }
-    }
-
-    // First try class autoloading.
-    if (!class_exists($function->classname)) {
-        // Fallback to explicit include of externallib.php.
-        $function->classpath = empty($function->classpath) ? core_component::get_component_directory($function->component).'/externallib.php' : $CFG->dirroot.'/'.$function->classpath;
-        if (!file_exists($function->classpath)) {
-            throw new coding_exception('Cannot find file with external function implementation: ' . $function->classname);
-        }
-        require_once($function->classpath);
-        if (!class_exists($function->classname)) {
-            throw new coding_exception('Cannot find external class');
-        }
-    }
-
-    $function->ajax_method = $function->methodname.'_is_allowed_from_ajax';
-    $function->parameters_method = $function->methodname.'_parameters';
-    $function->returns_method    = $function->methodname.'_returns';
-    $function->deprecated_method = $function->methodname.'_is_deprecated';
-
-    // make sure the implementaion class is ok
-    if (!method_exists($function->classname, $function->methodname)) {
-        throw new coding_exception('Missing implementation method of '.$function->classname.'::'.$function->methodname);
-    }
-    if (!method_exists($function->classname, $function->parameters_method)) {
-        throw new coding_exception('Missing parameters description');
-    }
-    if (!method_exists($function->classname, $function->returns_method)) {
-        throw new coding_exception('Missing returned values description');
-    }
-    if (method_exists($function->classname, $function->deprecated_method)) {
-        if (call_user_func(array($function->classname, $function->deprecated_method)) === true) {
-            $function->deprecated = true;
-        }
-    }
-    $function->allowed_from_ajax = false;
-
-    // fetch the parameters description
-    $function->parameters_desc = call_user_func(array($function->classname, $function->parameters_method));
-    if (!($function->parameters_desc instanceof external_function_parameters)) {
-        throw new coding_exception('Invalid parameters description');
-    }
-
-    // fetch the return values description
-    $function->returns_desc = call_user_func(array($function->classname, $function->returns_method));
-    // null means void result or result is ignored
-    if (!is_null($function->returns_desc) and !($function->returns_desc instanceof external_description)) {
-        throw new coding_exception('Invalid return description');
-    }
-
-    //now get the function description
-    //TODO MDL-31115 use localised lang pack descriptions, it would be nice to have
-    //      easy to understand descriptions in admin UI,
-    //      on the other hand this is still a bit in a flux and we need to find some new naming
-    //      conventions for these descriptions in lang packs
-    $function->description = null;
-    $servicesfile = core_component::get_component_directory($function->component).'/db/services.php';
-    if (file_exists($servicesfile)) {
-        $functions = null;
-        include($servicesfile);
-        if (isset($functions[$function->name]['description'])) {
-            $function->description = $functions[$function->name]['description'];
-        }
-        if (isset($functions[$function->name]['testclientpath'])) {
-            $function->testclientpath = $functions[$function->name]['testclientpath'];
-        }
-        if (isset($functions[$function->name]['type'])) {
-            $function->type = $functions[$function->name]['type'];
-        }
-        if (isset($functions[$function->name]['ajax'])) {
-            $function->allowed_from_ajax = $functions[$function->name]['ajax'];
-        } else if (method_exists($function->classname, $function->ajax_method)) {
-            if (call_user_func(array($function->classname, $function->ajax_method)) === true) {
-                debugging('External function ' . $function->ajax_method . '() function is deprecated.' .
-                          'Set ajax=>true in db/service.php instead.', DEBUG_DEVELOPER);
-                $function->allowed_from_ajax = true;
-            }
-        }
-        if (isset($functions[$function->name]['loginrequired'])) {
-            $function->loginrequired = $functions[$function->name]['loginrequired'];
-        } else {
-            $function->loginrequired = true;
-        }
-    }
-
-    return $function;
 }
 
 /**
@@ -161,6 +70,195 @@ class external_api {
     /** @var stdClass context where the function calls will be restricted */
     private static $contextrestriction;
 
+    /**
+     * Returns detailed function information
+     *
+     * @param string|object $function name of external function or record from external_function
+     * @param int $strictness IGNORE_MISSING means compatible mode, false returned if record not found, debug message if more found;
+     *                        MUST_EXIST means throw exception if no record or multiple records found
+     * @return stdClass description or false if not found or exception thrown
+     * @since Moodle 2.0
+     */
+    public static function external_function_info($function, $strictness=MUST_EXIST) {
+        global $DB, $CFG;
+
+        if (!is_object($function)) {
+            if (!$function = $DB->get_record('external_functions', array('name' => $function), '*', $strictness)) {
+                return false;
+            }
+        }
+
+        // First try class autoloading.
+        if (!class_exists($function->classname)) {
+            // Fallback to explicit include of externallib.php.
+            if (empty($function->classpath)) {
+                $function->classpath = core_component::get_component_directory($function->component).'/externallib.php';
+            } else {
+                $function->classpath = $CFG->dirroot.'/'.$function->classpath;
+            }
+            if (!file_exists($function->classpath)) {
+                throw new coding_exception('Cannot find file with external function implementation');
+            }
+            require_once($function->classpath);
+            if (!class_exists($function->classname)) {
+                throw new coding_exception('Cannot find external class');
+            }
+        }
+
+        $function->ajax_method = $function->methodname.'_is_allowed_from_ajax';
+        $function->parameters_method = $function->methodname.'_parameters';
+        $function->returns_method    = $function->methodname.'_returns';
+        $function->deprecated_method = $function->methodname.'_is_deprecated';
+
+        // Make sure the implementaion class is ok.
+        if (!method_exists($function->classname, $function->methodname)) {
+            throw new coding_exception('Missing implementation method of '.$function->classname.'::'.$function->methodname);
+        }
+        if (!method_exists($function->classname, $function->parameters_method)) {
+            throw new coding_exception('Missing parameters description');
+        }
+        if (!method_exists($function->classname, $function->returns_method)) {
+            throw new coding_exception('Missing returned values description');
+        }
+        if (method_exists($function->classname, $function->deprecated_method)) {
+            if (call_user_func(array($function->classname, $function->deprecated_method)) === true) {
+                $function->deprecated = true;
+            }
+        }
+        $function->allowed_from_ajax = false;
+
+        // Fetch the parameters description.
+        $function->parameters_desc = call_user_func(array($function->classname, $function->parameters_method));
+        if (!($function->parameters_desc instanceof external_function_parameters)) {
+            throw new coding_exception('Invalid parameters description');
+        }
+
+        // Fetch the return values description.
+        $function->returns_desc = call_user_func(array($function->classname, $function->returns_method));
+        // Null means void result or result is ignored.
+        if (!is_null($function->returns_desc) and !($function->returns_desc instanceof external_description)) {
+            throw new coding_exception('Invalid return description');
+        }
+
+        // Now get the function description.
+
+        // TODO MDL-31115 use localised lang pack descriptions, it would be nice to have
+        // easy to understand descriptions in admin UI,
+        // on the other hand this is still a bit in a flux and we need to find some new naming
+        // conventions for these descriptions in lang packs.
+        $function->description = null;
+        $servicesfile = core_component::get_component_directory($function->component).'/db/services.php';
+        if (file_exists($servicesfile)) {
+            $functions = null;
+            include($servicesfile);
+            if (isset($functions[$function->name]['description'])) {
+                $function->description = $functions[$function->name]['description'];
+            }
+            if (isset($functions[$function->name]['testclientpath'])) {
+                $function->testclientpath = $functions[$function->name]['testclientpath'];
+            }
+            if (isset($functions[$function->name]['type'])) {
+                $function->type = $functions[$function->name]['type'];
+            }
+            if (isset($functions[$function->name]['ajax'])) {
+                $function->allowed_from_ajax = $functions[$function->name]['ajax'];
+            } else if (method_exists($function->classname, $function->ajax_method)) {
+                if (call_user_func(array($function->classname, $function->ajax_method)) === true) {
+                    debugging('External function ' . $function->ajax_method . '() function is deprecated.' .
+                              'Set ajax=>true in db/service.php instead.', DEBUG_DEVELOPER);
+                    $function->allowed_from_ajax = true;
+                }
+            }
+            if (isset($functions[$function->name]['loginrequired'])) {
+                $function->loginrequired = $functions[$function->name]['loginrequired'];
+            } else {
+                $function->loginrequired = true;
+            }
+        }
+
+        return $function;
+    }
+
+    /**
+     * Call an external function validating all params/returns correctly.
+     *
+     * Note that an external function may modify the state of the current page, so this wrapper
+     * saves and restores tha PAGE and COURSE global variables before/after calling the external function.
+     *
+     * @param string $function A webservice function name.
+     * @param array $args Params array (named params)
+     * @param boolean $ajaxonly If true, an extra check will be peformed to see if ajax is required.
+     * @return array containing keys for error (bool), exception and data.
+     */
+    public static function call_external_function($function, $args, $ajaxonly=false) {
+        global $PAGE, $COURSE, $CFG, $SITE;
+
+        require_once($CFG->libdir . "/pagelib.php");
+
+        $externalfunctioninfo = self::external_function_info($function);
+
+        $currentpage = $PAGE;
+        $currentcourse = $COURSE;
+        $response = array();
+
+        try {
+
+            $PAGE = new moodle_page();
+            $COURSE = clone($SITE);
+
+            if ($ajaxonly && !$externalfunctioninfo->allowed_from_ajax) {
+                throw new moodle_exception('servicenotavailable', 'webservice');
+            }
+
+            // Do not allow access to write or delete webservices as a public user.
+            if ($externalfunctioninfo->loginrequired) {
+                if (defined('NO_MOODLE_COOKIES') && NO_MOODLE_COOKIES && !PHPUNIT_TEST) {
+                    throw new moodle_exception('servicenotavailable', 'webservice');
+                }
+                if (!isloggedin()) {
+                    throw new moodle_exception('servicenotavailable', 'webservice');
+                } else {
+                    require_sesskey();
+                }
+            }
+
+            // Validate params, this also sorts the params properly, we need the correct order in the next part.
+            $callable = array($externalfunctioninfo->classname, 'validate_parameters');
+            $params = call_user_func($callable,
+                                     $externalfunctioninfo->parameters_desc,
+                                     $args);
+
+            // Execute - gulp!
+            $callable = array($externalfunctioninfo->classname, $externalfunctioninfo->methodname);
+            $result = call_user_func_array($callable,
+                                           array_values($params));
+
+            // Validate the return parameters.
+            if ($externalfunctioninfo->returns_desc !== null) {
+                $callable = array($externalfunctioninfo->classname, 'clean_returnvalue');
+                $result = call_user_func($callable, $externalfunctioninfo->returns_desc, $result);
+            }
+
+            $response['error'] = false;
+            $response['data'] = $result;
+        } catch (Exception $e) {
+            $exception = get_exception_info($e);
+            unset($exception->a);
+            if (!debugging('', DEBUG_DEVELOPER)) {
+                unset($exception->debuginfo);
+                unset($exception->backtrace);
+            }
+            $response['error'] = true;
+            $response['exception'] = $exception;
+            // Do not process the remaining requests.
+        }
+
+        $PAGE = $currentpage;
+        $COURSE = $currentcourse;
+
+        return $response;
+    }
+
     /**
      * Set context restriction for all following subsequent function calls.
      *
@@ -359,7 +457,7 @@ class external_api {
      * @since Moodle 2.0
      */
     public static function validate_context($context) {
-        global $CFG;
+        global $CFG, $PAGE;
 
         if (empty($context)) {
             throw new invalid_parameter_exception('Context does not exist');
@@ -382,10 +480,10 @@ class external_api {
             }
         }
 
-        if ($context->contextlevel >= CONTEXT_COURSE) {
+        $PAGE->reset_theme_and_output();
-            list($context, $course, $cm) = get_context_info_array($context->id);
+        list($unused, $course, $cm) = get_context_info_array($context->id);
-            require_login($course, false, $cm, false, true);
+        require_login($course, false, $cm, false, true);
-        }
+        $PAGE->set_context($context);
     }
 
     /**

lib/pagelib.php

@@ -981,7 +981,6 @@ class moodle_page {
             }
             return;
         }
-
         // Ideally we should set context only once.
         if (isset($this->_context) && $context->id !== $this->_context->id) {
             $current = $this->_context->contextlevel;
@@ -993,11 +992,7 @@ class moodle_page {
             } else {
                 // We do not want devs to do weird switching of context levels on the fly because we might have used
                 // the context already such as in text filter in page title.
-                // This is explicitly allowed for webservices though which may
+                debugging("Coding problem: unsupported modification of PAGE->context from {$current} to {$context->contextlevel}");
-                // call "external_api::validate_context on many contexts in a single request.
-                if (!WS_SERVER) {
-                    debugging("Coding problem: unsupported modification of PAGE->context from {$current} to {$context->contextlevel}");
-                }
             }
         }
 
@@ -1560,6 +1555,22 @@ class moodle_page {
         $this->_wherethemewasinitialised = debug_backtrace();
     }
 
+    /**
+     * Reset the theme and output for a new context. This only makes sense from
+     * external::validate_context(). Do not cheat.
+     *
+     * @return string the name of the theme that should be used on this page.
+     */
+    public function reset_theme_and_output() {
+        global $COURSE, $SITE;
+
+        $COURSE = clone($SITE);
+        $this->_theme = null;
+        $this->_wherethemewasinitialised = null;
+        $this->_course = null;
+        $this->_context = null;
+    }
+
     /**
      * Work out the theme this page should use.
      *

lib/tests/externallib_test.php

@@ -354,6 +354,46 @@ class core_externallib_testcase extends advanced_testcase {
         // The extra course passed is not returned.
         $this->assertArrayNotHasKey($c4->id, $courses);
     }
+
+
+    public function test_call_external_function() {
+        global $PAGE, $COURSE;
+
+        $this->resetAfterTest(true);
+
+        // Call some webservice functions and verify they are correctly handling $PAGE and $COURSE.
+        // First test a function that calls validate_context outside a course.
+        $this->setAdminUser();
+        $category = $this->getDataGenerator()->create_category();
+        $params = array(
+            'contextid' => context_coursecat::instance($category->id)->id,
+            'name' => 'aaagrrryyy',
+            'idnumber' => '',
+            'description' => ''
+        );
+        $cohort1 = $this->getDataGenerator()->create_cohort($params);
+        $cohort2 = $this->getDataGenerator()->create_cohort();
+
+        $beforepage = $PAGE;
+        $beforecourse = $COURSE;
+        $params = array('cohortids' => array($cohort1->id, $cohort2->id));
+        $result = external_api::call_external_function('core_cohort_get_cohorts', $params);
+
+        $this->assertSame($beforepage, $PAGE);
+        $this->assertSame($beforecourse, $COURSE);
+
+        // Now test a function that calls validate_context inside a course.
+        $course = $this->getDataGenerator()->create_course();
+
+        $beforepage = $PAGE;
+        $beforecourse = $COURSE;
+        $params = array('courseid' => $course->id, 'options' => array());
+        $result = external_api::call_external_function('core_enrol_get_enrolled_users', $params);
+
+        $this->assertSame($beforepage, $PAGE);
+        $this->assertSame($beforecourse, $COURSE);
+    }
+
 }
 
 /*

lib/upgrade.txt

@@ -3,6 +3,8 @@ information provided here is intended especially for developers.
 
 === 3.1 ===
 
+* External functions that are not calling external_api::validate_context are buggy and will now generate
+  exceptions. Previously they were only generating warnings in the webserver error log.
 * The moodle/blog:associatecourse and moodle/blog:associatemodule capabilities has been removed.
 * The following functions has been finally deprecated and can not be used any more:
     - profile_display_badges()

mod/forum/tests/externallib_test.php

@@ -346,10 +346,6 @@ class mod_forum_external_testcase extends externallib_advanced_testcase {
         $discussions = mod_forum_external::get_forum_discussions(array($forum1->id, $forum2->id));
         $discussions = external_api::clean_returnvalue(mod_forum_external::get_forum_discussions_returns(), $discussions);
         $this->assertEquals($expecteddiscussions, $discussions);
-        // Some debugging is going to be produced, this is because we switch PAGE contexts in the get_forum_discussions function,
-        // the switch happens when the validate_context function is called inside a foreach loop.
-        // See MDL-41746 for more information.
-        $this->assertDebuggingCalled();
 
         // Remove the users post from the qanda forum and ensure they can still see the discussion.
         $DB->delete_records('forum_posts', array('id' => $discussion2reply1->id));
@@ -365,7 +361,6 @@ class mod_forum_external_testcase extends externallib_advanced_testcase {
         } catch (moodle_exception $e) {
             $this->assertEquals('nopermissions', $e->errorcode);
         }
-        $this->assertDebuggingCalled();
 
         // Unenrol user from second course.
         $enrol->unenrol_user($instance2, $user1->id);
@@ -857,8 +852,6 @@ class mod_forum_external_testcase extends externallib_advanced_testcase {
             mod_forum_external::add_discussion_post($discussion->firstpost, 'some subject', 'some text here...');
             $this->fail('Exception expected due to invalid permissions for posting.');
         } catch (moodle_exception $e) {
-            // Expect debugging since we are switching context, and this is something WS_SERVER mode don't like.
-            $this->assertDebuggingCalled();
             $this->assertEquals('nopostforum', $e->errorcode);
         }
 

user/tests/externallib_test.php

@@ -818,6 +818,7 @@ class core_user_externallib_testcase extends externallib_advanced_testcase {
         $device2['pushid'] = "0987654321";
         $device2['appid'] = "other.app.com";
 
+        $this->setAdminUser();
         // Create a user device using the external API function.
         core_user_external::add_user_device($device['appid'], $device['name'], $device['model'], $device['platform'],
                                             $device['version'], $device['pushid'], $device['uuid']);