MDL-60738 webservice: Clean theme and lang properly

Sometimes the "theme" and "lang" fields in the user and course tables in the database are set to incorrect values (uninstalled or non-existent themes and language packs). This makes Web Services functions to fail because the WS server validate the returned data using the validate_param function that clean parameters.
moodle · Nov 20, 2017 · dbe472f · dbe472f
1 parent f90fb83
commit dbe472f
Show file tree

Hide file tree

Showing 8 changed files with 60 additions and 13 deletions.
diff --git a/course/externallib.php b/course/externallib.php
@@ -504,10 +504,10 @@ public static function get_courses($options = array()) {
                 $courseinfo['groupmode'] = $course->groupmode;
                 $courseinfo['groupmodeforce'] = $course->groupmodeforce;
                 $courseinfo['defaultgroupingid'] = $course->defaultgroupingid;
-                $courseinfo['lang'] = $course->lang;
+                $courseinfo['lang'] = clean_param($course->lang, PARAM_LANG);
                 $courseinfo['timecreated'] = $course->timecreated;
                 $courseinfo['timemodified'] = $course->timemodified;
-                $courseinfo['forcetheme'] = $course->theme;
+                $courseinfo['forcetheme'] = clean_param($course->theme, PARAM_THEME);
                 $courseinfo['enablecompletion'] = $course->enablecompletion;
                 $courseinfo['completionnotify'] = $course->completionnotify;
                 $courseinfo['courseformatoptions'] = array();
@@ -1726,7 +1726,7 @@ public static function get_categories($criteria = array(), $addsubcategories = t
                         $categoryinfo['visible'] = $category->visible;
                         $categoryinfo['visibleold'] = $category->visibleold;
                         $categoryinfo['timemodified'] = $category->timemodified;
-                        $categoryinfo['theme'] = $category->theme;
+                        $categoryinfo['theme'] = clean_param($category->theme, PARAM_THEME);
                     }
 
                     $categoriesinfo[] = $categoryinfo;
@@ -3068,6 +3068,14 @@ public static function get_courses_by_field($field = '', $value = '') {
             foreach ($coursefields as $field) {
                 $coursesdata[$course->id][$field] = $course->{$field};
             }
+
+            // Clean lang and auth fields for external functions (it may content uninstalled themes or language packs).
+            if (isset($coursesdata[$course->id]['theme'])) {
+                $coursesdata[$course->id]['theme'] = clean_param($coursesdata[$course->id]['theme'], PARAM_THEME);
+            }
+            if (isset($coursesdata[$course->id]['lang'])) {
+                $coursesdata[$course->id]['lang'] = clean_param($coursesdata[$course->id]['lang'], PARAM_LANG);
+            }
         }
 
         return array(

diff --git a/course/tests/externallib_test.php b/course/tests/externallib_test.php
@@ -2209,6 +2209,21 @@ public function test_get_courses_by_field_invalid_courses() {
         $this->assertCount(0, $result['courses']);
     }
 
+    /**
+     * Test get_courses_by_field_invalid_theme_and_lang
+     */
+    public function test_get_courses_by_field_invalid_theme_and_lang() {
+        $this->resetAfterTest(true);
+        $this->setAdminUser();
+
+        $course = self::getDataGenerator()->create_course(array('theme' => 'kkt', 'lang' => 'kkl'));
+        $result = core_course_external::get_courses_by_field('id', $course->id);
+        $result = external_api::clean_returnvalue(core_course_external::get_courses_by_field_returns(), $result);
+        $this->assertEmpty($result['courses']['0']['theme']);
+        $this->assertEmpty($result['courses']['0']['lang']);
+    }
+
+
     public function test_check_updates() {
         global $DB;
         $this->resetAfterTest(true);

diff --git a/enrol/externallib.php b/enrol/externallib.php
@@ -342,7 +342,7 @@ public static function get_users_courses($userid) {
                 'summaryformat' => $course->summaryformat,
                 'format' => $course->format,
                 'showgrades' => $course->showgrades,
-                'lang' => $course->lang,
+                'lang' => clean_param($course->lang, PARAM_LANG),
                 'enablecompletion' => $course->enablecompletion,
                 'category' => $course->category,
                 'progress' => $progress,

diff --git a/enrol/tests/externallib_test.php b/enrol/tests/externallib_test.php
@@ -376,8 +376,12 @@ public function test_get_users_courses() {
             'enddate'          => $timenow + WEEKSECS
         );
 
+        $coursedata2 = array(
+            'lang'             => 'kk', // Check invalid language pack.
+        );
+
         $course1 = self::getDataGenerator()->create_course($coursedata1);
-        $course2 = self::getDataGenerator()->create_course();
+        $course2 = self::getDataGenerator()->create_course($coursedata2);
         $courses = array($course1, $course2);
 
         // Enrol $USER in the courses.
@@ -414,6 +418,9 @@ public function test_get_users_courses() {
                 foreach ($coursedata1 as $fieldname => $value) {
                     $this->assertEquals($courseenrol[$fieldname], $course1->$fieldname);
                 }
+            } else {
+                // Check language pack. Should be empty since an incorrect one was used when creating the course.
+                $this->assertEmpty($courseenrol['lang']);
             }
         }
     }

diff --git a/user/lib.php b/user/lib.php
@@ -563,6 +563,14 @@ function user_get_user_details($user, $course = null, array $userfields = array(
         }
     }
 
+    // Clean lang and auth fields for external functions (it may content uninstalled themes or language packs).
+    if (isset($userdetails['lang'])) {
+        $userdetails['lang'] = clean_param($userdetails['lang'], PARAM_LANG);
+    }
+    if (isset($userdetails['theme'])) {
+        $userdetails['theme'] = clean_param($userdetails['theme'], PARAM_THEME);
+    }
+
     return $userdetails;
 }
 

diff --git a/user/tests/externallib_test.php b/user/tests/externallib_test.php
@@ -222,8 +222,10 @@ public function test_get_users_by_field() {
             'descriptionformat' => FORMAT_MOODLE,
             'city' => 'Perth',
             'url' => 'http://moodle.org',
-            'country' => 'AU'
-            );
+            'country' => 'AU',
+            'lang' => 'kkl',
+            'theme' => 'kkt',
+        );
         $user1 = self::getDataGenerator()->create_user($user1);
         if (!empty($CFG->usetags)) {
             require_once($CFG->dirroot . '/user/editlib.php');
@@ -328,6 +330,11 @@ public function test_get_users_by_field() {
                 if (!empty($CFG->usetags) and !empty($generateduser->interests)) {
                     $this->assertEquals(implode(', ', $generateduser->interests), $returneduser['interests']);
                 }
+                // Check empty since incorrect values were used when creating the user.
+                if ($returneduser['id'] == $user1->id) {
+                    $this->assertEmpty($returneduser['lang']);
+                    $this->assertEmpty($returneduser['theme']);
+                }
             }
         }
 

diff --git a/user/tests/userlib_test.php b/user/tests/userlib_test.php
@@ -720,14 +720,16 @@ public function test_user_get_user_details() {
      * calling user_get_user_details() function.
      */
     public function test_user_get_user_details_missing_fields() {
+        global $CFG;
+
         $this->resetAfterTest(true);
         $this->setAdminUser(); // We need capabilities to view the data.
         $user = self::getDataGenerator()->create_user([
                                                           'auth'       => 'auth_something',
                                                           'confirmed'  => '0',
                                                           'idnumber'   => 'someidnumber',
-                                                          'lang'       => 'en_ar',
-                                                          'theme'      => 'mytheme',
+                                                          'lang'       => 'en',
+                                                          'theme'      => $CFG->theme,
                                                           'timezone'   => '50',
                                                           'mailformat' => '0',
                                                       ]);
@@ -737,8 +739,8 @@ public function test_user_get_user_details_missing_fields() {
         self::assertSame('auth_something', $got['auth']);
         self::assertSame('0', $got['confirmed']);
         self::assertSame('someidnumber', $got['idnumber']);
-        self::assertSame('en_ar', $got['lang']);
-        self::assertSame('mytheme', $got['theme']);
+        self::assertSame('en', $got['lang']);
+        self::assertSame($CFG->theme, $got['theme']);
         self::assertSame('50', $got['timezone']);
         self::assertSame('0', $got['mailformat']);
     }

diff --git a/webservice/externallib.php b/webservice/externallib.php
@@ -90,7 +90,7 @@ public static function get_site_info($serviceshortnames = array()) {
             'firstname' => $USER->firstname,
             'lastname' => $USER->lastname,
             'fullname' => fullname($USER),
-            'lang' => current_language(),
+            'lang' => clean_param(current_language(), PARAM_LANG),
             'userid' => $USER->id,
             'userpictureurl' => $profileimageurl->out(false),
             'siteid' => SITEID
@@ -214,7 +214,7 @@ public static function get_site_info_returns() {
                 'firstname'      => new external_value(PARAM_TEXT, 'first name'),
                 'lastname'       => new external_value(PARAM_TEXT, 'last name'),
                 'fullname'       => new external_value(PARAM_TEXT, 'user full name'),
-                'lang'           => new external_value(PARAM_LANG, 'user language'),
+                'lang'           => new external_value(PARAM_LANG, 'Current language.'),
                 'userid'         => new external_value(PARAM_INT, 'user id'),
                 'siteurl'        => new external_value(PARAM_RAW, 'site url'),
                 'userpictureurl' => new external_value(PARAM_URL, 'the user profile picture.