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
    • Rank:
      46285

      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.

        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: