diff --git a/course/tests/courselib_test.php b/course/tests/courselib_test.php index 31ab68d79be..3a835b1f1e7 100644 --- a/course/tests/courselib_test.php +++ b/course/tests/courselib_test.php @@ -4550,7 +4550,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 0, 'limit' => 0, 'offset' => 0, - 'expecteddbqueries' => 1, + 'expecteddbqueries' => 4, 'expectedresult' => $buildexpectedresult(0, 0) ], 'less than query limit' => [ @@ -4558,7 +4558,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 2, 'limit' => 0, 'offset' => 0, - 'expecteddbqueries' => 1, + 'expecteddbqueries' => 2, 'expectedresult' => $buildexpectedresult(2, 0) ], 'more than query limit' => [ @@ -4566,7 +4566,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 7, 'limit' => 0, 'offset' => 0, - 'expecteddbqueries' => 3, + 'expecteddbqueries' => 4, 'expectedresult' => $buildexpectedresult(7, 0) ], 'limit less than query limit' => [ @@ -4574,7 +4574,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 7, 'limit' => 2, 'offset' => 0, - 'expecteddbqueries' => 1, + 'expecteddbqueries' => 2, 'expectedresult' => $buildexpectedresult(2, 0) ], 'limit less than query limit with offset' => [ @@ -4582,7 +4582,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 7, 'limit' => 2, 'offset' => 2, - 'expecteddbqueries' => 1, + 'expecteddbqueries' => 2, 'expectedresult' => $buildexpectedresult(2, 2) ], 'limit less than total' => [ @@ -4590,7 +4590,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 9, 'limit' => 6, 'offset' => 0, - 'expecteddbqueries' => 2, + 'expecteddbqueries' => 3, 'expectedresult' => $buildexpectedresult(6, 0) ], 'less results than limit' => [ @@ -4598,7 +4598,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 9, 'limit' => 20, 'offset' => 0, - 'expecteddbqueries' => 3, + 'expecteddbqueries' => 4, 'expectedresult' => $buildexpectedresult(9, 0) ], 'less results than limit exact divisible' => [ @@ -4606,7 +4606,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 9, 'limit' => 20, 'offset' => 0, - 'expecteddbqueries' => 4, + 'expecteddbqueries' => 5, 'expectedresult' => $buildexpectedresult(9, 0) ], 'less results than limit with offset' => [ @@ -4614,7 +4614,7 @@ class core_course_courselib_testcase extends advanced_testcase { 'totalcourses' => 9, 'limit' => 10, 'offset' => 5, - 'expecteddbqueries' => 2, + 'expecteddbqueries' => 3, 'expectedresult' => $buildexpectedresult(4, 5) ], ]; diff --git a/course/tests/externallib_test.php b/course/tests/externallib_test.php index 99693eb2a2d..231c6fba399 100644 --- a/course/tests/externallib_test.php +++ b/course/tests/externallib_test.php @@ -2765,7 +2765,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { /** * Test cases for the get_enrolled_courses_by_timeline_classification test. */ - public function get_get_enrolled_courses_by_timeline_classification_test_cases() { + public function get_get_enrolled_courses_by_timeline_classification_test_cases():array { $now = time(); $day = 86400; @@ -2864,6 +2864,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 2, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => [], 'expectednextoffset' => 0 ], @@ -2873,6 +2874,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 0, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => ['afuture', 'bfuture', 'cfuture', 'dfuture', 'efuture'], 'expectednextoffset' => 15 ], @@ -2881,6 +2883,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 2, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => ['afuture', 'bfuture'], 'expectednextoffset' => 4 ], @@ -2889,6 +2892,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 2, 'offset' => 2, + 'sort' => 'shortname ASC', 'expectedcourses' => ['bfuture', 'cfuture'], 'expectednextoffset' => 7 ], @@ -2897,6 +2901,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 5, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => ['afuture', 'bfuture', 'cfuture', 'dfuture', 'efuture'], 'expectednextoffset' => 13 ], @@ -2905,6 +2910,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 10, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => ['afuture', 'bfuture', 'cfuture', 'dfuture', 'efuture'], 'expectednextoffset' => 15 ], @@ -2913,6 +2919,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'future', 'limit' => 10, 'offset' => 5, + 'sort' => 'shortname ASC', 'expectedcourses' => ['cfuture', 'dfuture', 'efuture'], 'expectednextoffset' => 15 ], @@ -2921,6 +2928,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'all', 'limit' => 0, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => [ 'afuture', 'ainprogress', @@ -2945,6 +2953,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'all', 'limit' => 5, 'offset' => 0, + 'sort' => 'shortname ASC', 'expectedcourses' => [ 'afuture', 'ainprogress', @@ -2959,6 +2968,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'all', 'limit' => 5, 'offset' => 5, + 'sort' => 'shortname ASC', 'expectedcourses' => [ 'bpast', 'cfuture', @@ -2973,9 +2983,161 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { 'classification' => 'all', 'limit' => 5, 'offset' => 50, + 'sort' => 'shortname ASC', 'expectedcourses' => [], 'expectednextoffset' => 50 ], + 'all limit and offset with sort ul.timeaccess desc' => [ + 'coursedata' => $coursedata, + 'classification' => 'inprogress', + 'limit' => 0, + 'offset' => 0, + 'sort' => 'ul.timeaccess desc', + 'expectedcourses' => [ + 'ainprogress', + 'binprogress', + 'cinprogress', + 'dinprogress', + 'einprogress' + ], + 'expectednextoffset' => 15 + ], + 'all limit and offset with sort sql injection for sort or 1==1' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => 'ul.timeaccess desc or 1==1', + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with sql injection of sort a custom one' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "ul.timeaccess LIMIT 1--", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong sort direction' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "ul.timeaccess abcdasc", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid sort direction in $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong sort direction' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "ul.timeaccess.foo ascd", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid sort direction in $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong sort param' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "foobar", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong field name' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "ul.foobar", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong field separator' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "ul.timeaccess.foo", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong field separator #' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "ul#timeaccess", + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong field separator $' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => 'ul$timeaccess', + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with wrong field name' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => 'timeaccess123', + 'expectedcourses' => [], + 'expectednextoffset' => 0, + 'expectedexception' => 'Invalid $sort parameter in enrol_get_my_courses()' + ], + 'all limit and offset with no sort direction for ul' => [ + 'coursedata' => $coursedata, + 'classification' => 'inprogress', + 'limit' => 0, + 'offset' => 0, + 'sort' => "ul.timeaccess", + 'expectedcourses' => ['ainprogress', 'binprogress', 'cinprogress', 'dinprogress', 'einprogress'], + 'expectednextoffset' => 15, + ], + 'all limit and offset with valid field name and no prefix, test for ul' => [ + 'coursedata' => $coursedata, + 'classification' => 'inprogress', + 'limit' => 0, + 'offset' => 0, + 'sort' => "timeaccess", + 'expectedcourses' => ['ainprogress', 'binprogress', 'cinprogress', 'dinprogress', 'einprogress'], + 'expectednextoffset' => 15, + ], + 'all limit and offset with valid field name and no prefix' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "fullname", + 'expectedcourses' => ['bpast', 'cpast', 'dfuture', 'dpast', 'efuture'], + 'expectednextoffset' => 10, + ], + 'all limit and offset with valid field name and no prefix and with sort direction' => [ + 'coursedata' => $coursedata, + 'classification' => 'all', + 'limit' => 5, + 'offset' => 5, + 'sort' => "fullname desc", + 'expectedcourses' => ['bpast', 'cpast', 'dfuture', 'dpast', 'efuture'], + 'expectednextoffset' => 10, + ], ]; } @@ -2987,16 +3149,20 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { * @param string $classification Timeline classification * @param int $limit Maximum number of results * @param int $offset Offset the unfiltered courses result set by this amount + * @param string $sort sort the courses * @param array $expectedcourses Expected courses in result * @param int $expectednextoffset Expected next offset value in result + * @param string|null $expectedexception Expected exception string */ public function test_get_enrolled_courses_by_timeline_classification( $coursedata, $classification, $limit, $offset, + $sort, $expectedcourses, - $expectednextoffset + $expectednextoffset, + $expectedexception = null ) { $this->resetAfterTest(); $generator = $this->getDataGenerator(); @@ -3013,6 +3179,11 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { $this->setUser($student); + if (isset($expectedexception)) { + $this->expectException('coding_exception'); + $this->expectExceptionMessage($expectedexception); + } + // NOTE: The offset applies to the unfiltered full set of courses before the classification // filtering is done. // E.g. In our example if an offset of 2 is given then it would mean the first @@ -3021,7 +3192,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { $classification, $limit, $offset, - 'shortname ASC' + $sort ); $result = external_api::clean_returnvalue( core_course_external::get_enrolled_courses_by_timeline_classification_returns(), @@ -3032,7 +3203,7 @@ class core_course_externallib_testcase extends externallib_advanced_testcase { return $course['shortname']; }, $result['courses']); - $this->assertEquals($expectedcourses, $actual); + $this->assertEqualsCanonicalizing($expectedcourses, $actual); $this->assertEquals($expectednextoffset, $result['nextoffset']); } diff --git a/lib/enrollib.php b/lib/enrollib.php index a6c01b2004e..7f4809b0a42 100644 --- a/lib/enrollib.php +++ b/lib/enrollib.php @@ -567,6 +567,11 @@ function enrol_get_my_courses($fields = null, $sort = null, $limit = 0, $coursei $offset = 0, $excludecourses = []) { global $DB, $USER, $CFG; + // Allowed prefixes and field names. + $allowedprefixesandfields = ['c' => array_keys($DB->get_columns('course')), + 'ul' => array_keys($DB->get_columns('user_lastaccess')), + 'ue' => array_keys($DB->get_columns('user_enrolments'))]; + // Re-Arrange the course sorting according to the admin settings. $sort = enrol_get_courses_sortingsql($sort); @@ -599,28 +604,63 @@ function enrol_get_my_courses($fields = null, $sort = null, $limit = 0, $coursei $orderby = ""; $sort = trim($sort); $sorttimeaccess = false; - $allowedsortprefixes = array('c', 'ul', 'ue'); if (!empty($sort)) { $rawsorts = explode(',', $sort); $sorts = array(); foreach ($rawsorts as $rawsort) { $rawsort = trim($rawsort); - if (preg_match('/^ul\.(\S*)\s(asc|desc)/i', $rawsort, $matches)) { - if (strcasecmp($matches[2], 'asc') == 0) { - $sorts[] = 'COALESCE(ul.' . $matches[1] . ', 0) ASC'; - } else { - $sorts[] = 'COALESCE(ul.' . $matches[1] . ', 0) DESC'; + // Make sure that there are no more white spaces in sortparams after explode. + $sortparams = array_values(array_filter(explode(' ', $rawsort))); + // If more than 2 values present then throw coding_exception. + if (isset($sortparams[2])) { + throw new coding_exception('Invalid $sort parameter in enrol_get_my_courses()'); + } + // Check the sort ordering if present, at the beginning. + if (isset($sortparams[1]) && (preg_match("/^(asc|desc)$/i", $sortparams[1]) === 0)) { + throw new coding_exception('Invalid sort direction in $sort parameter in enrol_get_my_courses()'); + } + + $sortfield = $sortparams[0]; + $sortdirection = $sortparams[1] ?? 'asc'; + if (strpos($sortfield, '.') !== false) { + $sortfieldparams = explode('.', $sortfield); + // Check if more than one dots present in the prefix field. + if (isset($sortfieldparams[2])) { + throw new coding_exception('Invalid $sort parameter in enrol_get_my_courses()'); } - $sorttimeaccess = true; - } else if (strpos($rawsort, '.') !== false) { - $prefix = explode('.', $rawsort); - if (in_array($prefix[0], $allowedsortprefixes)) { - $sorts[] = trim($rawsort); + list($prefix, $fieldname) = [$sortfieldparams[0], $sortfieldparams[1]]; + // Check if the field name matches with the allowed prefix. + if (array_key_exists($prefix, $allowedprefixesandfields) && + (in_array($fieldname, $allowedprefixesandfields[$prefix]))) { + if ($prefix === 'ul') { + $sorts[] = "COALESCE({$prefix}.{$fieldname}, 0) {$sortdirection}"; + $sorttimeaccess = true; + } else { + // Check if the field name that matches with the prefix and just append to sorts. + $sorts[] = $rawsort; + } } else { throw new coding_exception('Invalid $sort parameter in enrol_get_my_courses()'); } } else { - $sorts[] = 'c.'.trim($rawsort); + // Check if the field name matches with $allowedprefixesandfields. + $found = false; + foreach (array_keys($allowedprefixesandfields) as $prefix) { + if (in_array($sortfield, $allowedprefixesandfields[$prefix])) { + if ($prefix === 'ul') { + $sorts[] = "COALESCE({$prefix}.{$sortfield}, 0) {$sortdirection}"; + $sorttimeaccess = true; + } else { + $sorts[] = "{$prefix}.{$sortfield} {$sortdirection}"; + } + $found = true; + break; + } + } + if (!$found) { + // The param is not found in $allowedprefixesandfields. + throw new coding_exception('Invalid $sort parameter in enrol_get_my_courses()'); + } } } $sort = implode(',', $sorts);