Moodle
  1. Moodle
  2. MDL-36773

core_user_get_users_by_id - ERRORCODE: invalidresponse

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.6, 2.3.3, 2.4
    • Fix Version/s: 2.2.7, 2.3.4, 2.4.1
    • Component/s: Web Services
    • Labels:
    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      phpunit moodlelib_testcase lib/tests/moodlelib_test.php
      If no issue caused by moodlelib_testcase::test_clean_param_timezone, then it works fine.

      Show
      phpunit moodlelib_testcase lib/tests/moodlelib_test.php If no issue caused by moodlelib_testcase::test_clean_param_timezone, then it works fine.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-36773-master

      Description

      When making web service calls to core_user_get_users_by_id, we're noticing that some users with timezones of numeric values (such as 10.0) are returning errors. We upgraded from Moodle 2.0.x and those were working fine before using the old moodle_user_get_users_by_id call. The difference seems to be the PARAM checking type, where before it was ALPHANUMEXT, now it is TIMEZONE.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Nathan Mares added a comment - - edited

            get_list_of_timezones() which is used to populate the timezone list for the user profile edit page appears to return invalid timezones which would be causing invalid timezones to be saved in user records.

            The comments around PARAM_TIMZONE say:

            PARAM_TIMEZONE - expected timezone. Timezone can be int +(0-13) or float +(0.5-12.5) or string seperated by '/' and can have '-' &/ '_' (eg. America/North_Dakota/New_Salem, America/Port-au-Prince)

            The bit I should highlight is the "int +-(0-13)"

            Currently get_list_of_timezones() returns some values that are invalid according to the regex for PARAM_TIMEZONE in clean_param.

            I knocked up a quick bit of code to test that the values returned are valid:

            <?php
             
            define('MOODLE_INTERNAL', 1);                                           
                                                                                          
            require_once('config.php');                                                
                                                         
            $timezones = get_list_of_timezones();    
                                                                               
            foreach ($timezones as $timezone=>$label) {                             
              if (clean_param($timezone, PARAM_TIMEZONE) != $timezone) {           
                echo "Invalid timezone $timezone<br>";                       
              }                                                              
            }
            

            I've included output below showing which ones didn't pass though the regex test:

            Invalid timezone -13.0
            Invalid timezone -12.0
            Invalid timezone -11.0
            Invalid timezone -10.0
            Invalid timezone 10.0
            Invalid timezone 11.0
            Invalid timezone 12.0
            Invalid timezone 13.0
            

            The regex test is for PARAM_TIMEZONE is this:

            /^(([+-]?(0?[0-9](\.[5|0])?|1[0-3]|1[0-2]\.5))|(99)|[[:alnum:]](\/?[[:alpha:]_-]))$/

            I see two possible solutions:

            1) Change get_list_of_timezones() to return valid timezones only (without trailing 0's) and optionally fix up broken data in people's mdl_user table; or
            2) Change that regex to allow the timezones returned by get_list_of_timezones(), e.g. /^(([+-]?(0?[0-9](\.[5|0])?|1[0-3]|1[0-2]\.[5|0]))|(99)|[[:alnum:]](\/?[[:alpha:]_-]))$/

            Show
            Nathan Mares added a comment - - edited get_list_of_timezones() which is used to populate the timezone list for the user profile edit page appears to return invalid timezones which would be causing invalid timezones to be saved in user records. The comments around PARAM_TIMZONE say: PARAM_TIMEZONE - expected timezone. Timezone can be int + (0-13) or float + (0.5-12.5) or string seperated by '/' and can have '-' &/ '_' (eg. America/North_Dakota/New_Salem, America/Port-au-Prince) The bit I should highlight is the "int +-(0-13)" Currently get_list_of_timezones() returns some values that are invalid according to the regex for PARAM_TIMEZONE in clean_param. I knocked up a quick bit of code to test that the values returned are valid: <?php   define('MOODLE_INTERNAL', 1); require_once('config.php'); $timezones = get_list_of_timezones(); foreach ($timezones as $timezone=>$label) { if (clean_param($timezone, PARAM_TIMEZONE) != $timezone) { echo "Invalid timezone $timezone<br>"; } } I've included output below showing which ones didn't pass though the regex test: Invalid timezone -13.0 Invalid timezone -12.0 Invalid timezone -11.0 Invalid timezone -10.0 Invalid timezone 10.0 Invalid timezone 11.0 Invalid timezone 12.0 Invalid timezone 13.0 The regex test is for PARAM_TIMEZONE is this: /^(( [+-] ?(0? [0-9] (\. [5|0] )?|1 [0-3] |1 [0-2] \.5))|(99)|[ [:alnum:] ] (\/?[ [:alpha:] _-]) )$/ I see two possible solutions: 1) Change get_list_of_timezones() to return valid timezones only (without trailing 0's) and optionally fix up broken data in people's mdl_user table; or 2) Change that regex to allow the timezones returned by get_list_of_timezones(), e.g. /^(( [+-] ?(0? [0-9] (\. [5|0] )?|1 [0-3] |1 [0-2] \. [5|0] ))|(99)|[ [:alnum:] ] (\/?[ [:alpha:] _-]) )$/
            Hide
            Nathan Mares added a comment -

            Actually that regex in option 2 was bad and would have failed for timezone "13.0".

            I've attached a patch which has a better (working) one and updated the unit tests in lib/tests/moodlelib_tests.php

            Show
            Nathan Mares added a comment - Actually that regex in option 2 was bad and would have failed for timezone "13.0". I've attached a patch which has a better (working) one and updated the unit tests in lib/tests/moodlelib_tests.php
            Hide
            Jérôme Mouneyrac added a comment - - edited

            Thanks Nathan, I think too that the regex is missing .0 check. Thanks for the fix, and for the unit test update
            Attach a git commit next time, it make it easy to keep record of the commit author.

            Show
            Jérôme Mouneyrac added a comment - - edited Thanks Nathan, I think too that the regex is missing .0 check. Thanks for the fix, and for the unit test update Attach a git commit next time, it make it easy to keep record of the commit author.
            Hide
            Nathan Mares added a comment -

            Hi Jerome - sorry, thought I had attached a commit. Will check next time.

            Just looking at you're diff now. Looks like you used my first regex. Its bad because it doesn't validate "-13.0" or "13.0" as timezones which are values returned from get_list_of_timezones().

            The one below (from the attached patch) is better because it allows an optional ".0" after any + or - integer-only value but doesn't permit values of "13.5" or "-13.5" which shouldn't pass.

            /^(([+-]?(0?[0-9](\.[5|0])?|1[0-3](\.0)?|1[0-2]\.5))|(99)|[[:alnum:]](\/?[[:alpha:]_-]))$/

            Show
            Nathan Mares added a comment - Hi Jerome - sorry, thought I had attached a commit. Will check next time. Just looking at you're diff now. Looks like you used my first regex. Its bad because it doesn't validate "-13.0" or "13.0" as timezones which are values returned from get_list_of_timezones(). The one below (from the attached patch) is better because it allows an optional ".0" after any + or - integer-only value but doesn't permit values of "13.5" or "-13.5" which shouldn't pass. /^(( [+-] ?(0? [0-9] (\. [5|0] )?|1 [0-3] (\.0)?|1 [0-2] \.5))|(99)|[ [:alnum:] ] (\/?[ [:alpha:] _-]) )$/
            Hide
            Jérôme Mouneyrac added a comment - - edited

            Ah thanks Nathan. I saw that and got influenced by phpdoc in the way, and forget the main reason I thought supporting all .0 is good. I think we should support all YX.0 in order to be consistent as we support X.0 (in case peerreviewer/integrator wonder why).
            Changing the regex.

            Show
            Jérôme Mouneyrac added a comment - - edited Ah thanks Nathan. I saw that and got influenced by phpdoc in the way, and forget the main reason I thought supporting all .0 is good. I think we should support all YX.0 in order to be consistent as we support X.0 (in case peerreviewer/integrator wonder why). Changing the regex.
            Hide
            Jérôme Mouneyrac added a comment -

            Sending to peer-review.

            Show
            Jérôme Mouneyrac added a comment - Sending to peer-review.
            Hide
            Rajesh Taneja added a comment -

            Thanks Jerome,
            Patch looks perfect. 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 Jerome, Patch looks perfect. 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 -

            Thanks Jerome, integrated to 22, 23, 24 and master.

            Show
            Dan Poltawski added a comment - Thanks Jerome, integrated to 22, 23, 24 and master.
            Hide
            Dan Poltawski added a comment -

            Working fine

            Show
            Dan Poltawski added a comment - Working fine
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: